New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http: decide how users should use SetSessionTicketKeys with ServeTLS, etc #22018

Open
costela opened this Issue Sep 25, 2017 · 10 comments

Comments

Projects
None yet
7 participants
@costela

costela commented Sep 25, 2017

Short version:

Using tls.Config.SetSessionTicketKeys does not work with http.Server.ServeTLS (and all methods that build on it, like ListenAndServeTLS).
Problem arises because tls.Config gets cloned inside ServeTLS and there seems to be no way to reference the new one from "outside". If this behavior can't be changed, at least a mention in the docs might be nice! 😉

Long version:

What version of Go are you using (go version)?

1.9 (but problem seems to be present starting with 1.5)

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/costela/go"
GORACE=""
GOROOT="/usr/lib/go-1.9"
GOTOOLDIR="/usr/lib/go-1.9/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build167108121=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

Attempt to use tls.Config.SetSessionTicketKeys on a running server, which is explicitly mentioned as possible in the docs:

httpsServer := &http.Server{
    Addr:      "localhost:https",
    TLSConfig: &tls.Config{
        SessionTicketKey: [32]byte{...some hard-coded key...}
    },
}

go func() {
    keys := make([][32]byte, 1)
    for t := range time.Tick(10 * time.Second) {
        bytes, _ := t.MarshalBinary()
        copy(keys[0][:], bytes)
        tlsConfig.SetSessionTicketKeys(keys) // yes, I'm aware this is highly insecure ;)
	}
}()

log.Fatal(httpsServer.ListenAndServeTLS("some.crt", "some.key"))

And then checked to see if session-resumption works with:

$ openssl s_client -connect localhost:443 -sess_out session.tmp -quiet
$ openssl s_client -connect localhost:443 -sess_in session.tmp | grep Reused

What did you expect to see?

  • session being reused in the first 10 seconds (using the hard-coded initial key)
  • session being reused "inside" a 10 second tick (using the same time-based key)
  • session not being reused "between" two 10 second ticks (using different time-based keys)

What did you see instead?

Session was reused in all scenarios, consistent with what would be expected because of the tls.Config cloning mentioned in the "short" explanation above.

Why is this a problem?

Because there is no mention in the docs (or did I miss something?) that using ServeTLS and friends will cause your reference to tls.Config to become "useless", in the sense that it doesn't reflect the running server's state. Workaround would be to create your own net.Listener, but that means a lot of boilerplate code, e.g. replicating tcpKeepAliveListener, etc.

I think I understand why the clone might be necessary (I imagine it has to do with not changing it because it might be reused), but I can't think of a reason to not update the pointer in the http.Server struct to point to the new clone. Am I missing something obvious?
But even if I am, it would be nice to mention this "gotcha" in the docs (I'd be happy to prepare a small PR for that if there are no objections)

@ianlancetaylor ianlancetaylor changed the title from clarify cloning of tls.Config inside http.ServeTLS to net/http: clarify cloning of tls.Config inside http.ServeTLS Sep 25, 2017

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Sep 25, 2017

@tombergan

This comment has been minimized.

Contributor

tombergan commented Nov 3, 2017

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

@costela

This comment has been minimized.

costela commented Nov 6, 2017

@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.

@tombergan

This comment has been minimized.

Contributor

tombergan commented Nov 6, 2017

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

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 7, 2017

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
@costela

This comment has been minimized.

costela commented Nov 8, 2017

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.

@gopherbot

This comment has been minimized.

gopherbot commented Jan 5, 2018

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

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jan 5, 2018

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.

@bradfitz bradfitz modified the milestones: Go1.10, Go1.11 Jan 5, 2018

gopherbot pushed a commit that referenced this issue Jan 6, 2018

net/http: document cloning of Server.TLSConfig
Updates #22018

Change-Id: I8a85324e9d53dd4d279ed05cdb93f50d55cf767b
Reviewed-on: https://go-review.googlesource.com/86415
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@posener

This comment has been minimized.

posener commented Jul 20, 2018

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()

@bradfitz bradfitz changed the title from net/http: clarify cloning of tls.Config inside http.ServeTLS to net/http: decide how users should use SetSessionTicketKeys with ServeTLS, etc Jul 23, 2018

@bradfitz bradfitz removed the Documentation label Jul 23, 2018

@bradfitz bradfitz modified the milestones: Go1.11, Go1.12 Jul 23, 2018

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jul 23, 2018

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

@rsc rsc added the early-in-cycle label Nov 14, 2018

@rsc rsc modified the milestones: Go1.12, Go1.13 Nov 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment