CC @tombergan

That is unfortunate :-/ The TLSConfig is cloned to avoid a race (d931716) and I don't think we can undo that change. We can certainly document the behavior, though.

/cc @bradfitz

@tombergan thanks for the info!
I don't want to be a nuisance, but wouldn't updating the Server.TLSConfig pointer to point to the clone also be an option? As far as I understand the linked issue, the race relates to concurrent use of a single tls.Config, but this shouldn't preclude the pointer update. Am I missing some obvious overarching concurrency issue?
It just would be nice to be able to use the more comfortable wrapper functions while still being able to interact with the "live" tls.Config.

That would be an option. I don't know if any programs rely on Server.TLSConfig not being mutated -- it's certainly possible. I also don't know why it was originally decided that using the same tls.Config from multiple places should be allowed in the first place, especially given that a tls.Config can be mutated with methods like SetSessionTicketKeys, as you point out. Maybe Brad knows?

/cc @bradfitz

Ugh.

I don't know if any programs rely on Server.TLSConfig not being mutated

I'm sure if we start mutating it, we'll get new data race reports. I don't think that's an option. If anything, we probably need to either provide a mechanism (new API? new heuristic?) to not clone, or we need to add new API to get the current (cloned) TLS config.

I also don't know why it was originally decided that using the same tls.Config from multiple places should be allowed in the first place, especially given that a tls.Config can be mutated with methods like SetSessionTicketKeys, as you point out. Maybe Brad knows?

You're assuming there was ever a decision and we didn't just end up here by accident.

I think the general mutability of *tls.Config happened gradually over time.

I mean, here was go1's API surface here:

pkg crypto/tls, func Client(net.Conn, *Config) *Conn
pkg crypto/tls, func Dial(string, string, *Config) (*Conn, error)
pkg crypto/tls, func Listen(string, string, *Config) (net.Listener, error)
pkg crypto/tls, func NewListener(net.Listener, *Config) net.Listener
pkg crypto/tls, func Server(net.Conn, *Config) *Conn
pkg crypto/tls, method (*Config) BuildNameToCertificate()
pkg crypto/tls, type Config struct
pkg crypto/tls, type Config struct, Certificates []Certificate
pkg crypto/tls, type Config struct, CipherSuites []uint16
pkg crypto/tls, type Config struct, ClientAuth ClientAuthType
pkg crypto/tls, type Config struct, ClientCAs *x509.CertPool
pkg crypto/tls, type Config struct, InsecureSkipVerify bool
pkg crypto/tls, type Config struct, NameToCertificate map[string]*Certificate
pkg crypto/tls, type Config struct, NextProtos []string
pkg crypto/tls, type Config struct, Rand io.Reader
pkg crypto/tls, type Config struct, RootCAs *x509.CertPool
pkg crypto/tls, type Config struct, ServerName string
pkg crypto/tls, type Config struct, Time func() time.Time

pkg net/http, type Server struct, TLSConfig *tls.Config
pkg net/http, type Transport struct, TLSClientConfig *tls.Config

pkg net/http/httptest, type Server struct, TLS *tls.Config

hey @bradfitz , thanks for taking the time to chime in! :)

I'm sure if we start mutating it, we'll get new data race reports. I don't think that's an option.

Sounds reasonable.

If anything, we probably need to either provide a mechanism (new API? new heuristic?) to not clone, or we need to add new API to get the current (cloned) TLS config.

IMHO having a heuristic to decide whether to clone sounds like it could further complicate the issue: it would be even less obvious if our passed-in http.Server.TLSConfig instance is "live" or not. Maybe I misunderstood your suggestion?
On the other hand, an additional API sounds doable: something like http.Server.GetTLSConfig() - plus some more explicit documentation - would make clear that using the http.*Serve*() family of convenience functions takes liberties with the provided tls.Config.

To make the separation between configuration and runtime clearer, this new method could return an interface like the following, instead of a *tls.Config directly

type LiveTLSConfig interface {
    Clone() *tls.Config
    SetSessionTicketKeys(keys [][32]byte)
}

making it very explicit which operations are sane and safe in a "live" tls.Config.
I admittedly didn't give much thought to the client side, though. Don't know if it would make sense to have something similar on that side.

You're assuming there was ever a decision and we didn't just end up here by accident.
I think the general mutability of *tls.Config happened gradually over time.

The suggestion above would be one way to start addressing this semantic drift in the API, without braking current code. If it sounds workable at all to you, I'd take a shot at implementing it.

Otherwise, if you think this issue doesn't warrant increasing the API surface, I'd prepare a patch to at least clarify the docs about this gotcha.

Change https://golang.org/cl/86415 mentions this issue: net/http: document cloning of Server.TLSConfig

It's super late in the Go 1.10 cycle so I just mailed docs for now. In the docs I mentioned that the workaround to use SetSessionTicketKeys is to use http.Server.Serve instead, using the tls.Listen Listener instead.

We can consider this more for Go 1.11.

I meet the same kind of issue with the http2.ConfigureServer
I must do server.TLS = server.Config.TLSConfig in this piece of testing code:

	server := httptest.NewUnstartedServer(h)
	err := http2.ConfigureServer(server.Config, nil)
	if err != nil {
		t.Fatal(err)
	}

	// Copy the configured TLS of the *http.Server to the one used by StartTLS
	server.TLS = server.Config.TLSConfig

	server.StartTLS()

Sorry, I thought this was just a documentation bug (we already did some docs) so I pushed it off until later in the release. Now I see we might want to do more. I've removed the Documentation label and retitled for Go 1.12.

/cc @FiloSottile