From 0246e57d7fd4983724546acfa20d00af4a1f84e9 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Fri, 11 Dec 2020 11:11:10 -0500 Subject: [PATCH] Set lifespans on state and CSRF cooking encoding Signed-off-by: Andrew Keesler --- internal/oidc/dynamiccodec/codec.go | 24 ++++++++++++++++------- internal/oidc/dynamiccodec/codec_test.go | 23 +++++++++++++++++++--- internal/oidc/oidc.go | 5 +++++ internal/oidc/provider/manager/manager.go | 12 ++++++++++-- 4 files changed, 52 insertions(+), 12 deletions(-) diff --git a/internal/oidc/dynamiccodec/codec.go b/internal/oidc/dynamiccodec/codec.go index 13407f70..5168b2b7 100644 --- a/internal/oidc/dynamiccodec/codec.go +++ b/internal/oidc/dynamiccodec/codec.go @@ -6,6 +6,8 @@ package dynamiccodec import ( + "time" + "github.com/gorilla/securecookie" "go.pinniped.dev/internal/oidc" @@ -19,6 +21,7 @@ type KeyFunc func() []byte // Codec can dynamically encode and decode information by using a KeyFunc to get its keys // just-in-time. type Codec struct { + lifespan time.Duration signingKeyFunc KeyFunc encryptionKeyFunc KeyFunc } @@ -26,8 +29,12 @@ type Codec struct { // New creates a new Codec that will use the provided keyFuncs for its key source, and // use the securecookie.JSONEncoder. The securecookie.JSONEncoder is used because the default // securecookie.GobEncoder is less compact and more difficult to make forward compatible. -func New(signingKeyFunc, encryptionKeyFunc KeyFunc) *Codec { +// +// The returned Codec will make ensure that the encoded values will only be valid for the provided +// lifespan. +func New(lifespan time.Duration, signingKeyFunc, encryptionKeyFunc KeyFunc) *Codec { return &Codec{ + lifespan: lifespan, signingKeyFunc: signingKeyFunc, encryptionKeyFunc: encryptionKeyFunc, } @@ -35,14 +42,17 @@ func New(signingKeyFunc, encryptionKeyFunc KeyFunc) *Codec { // Encode implements oidc.Encode(). func (c *Codec) Encode(name string, value interface{}) (string, error) { - encoder := securecookie.New(c.signingKeyFunc(), c.encryptionKeyFunc()) - encoder.SetSerializer(securecookie.JSONEncoder{}) - return encoder.Encode(name, value) + return c.delegate().Encode(name, value) } // Decode implements oidc.Decode(). func (c *Codec) Decode(name string, value string, into interface{}) error { - decoder := securecookie.New(c.signingKeyFunc(), c.encryptionKeyFunc()) - decoder.SetSerializer(securecookie.JSONEncoder{}) - return decoder.Decode(name, value, into) + return c.delegate().Decode(name, value, into) +} + +func (c *Codec) delegate() *securecookie.SecureCookie { + codec := securecookie.New(c.signingKeyFunc(), c.encryptionKeyFunc()) + codec.MaxAge(int(c.lifespan.Seconds())) + codec.SetSerializer(securecookie.JSONEncoder{}) + return codec } diff --git a/internal/oidc/dynamiccodec/codec_test.go b/internal/oidc/dynamiccodec/codec_test.go index e85a77fe..e106a55d 100644 --- a/internal/oidc/dynamiccodec/codec_test.go +++ b/internal/oidc/dynamiccodec/codec_test.go @@ -6,6 +6,7 @@ package dynamiccodec import ( "strings" "testing" + "time" "github.com/stretchr/testify/require" ) @@ -13,6 +14,7 @@ import ( func TestCodec(t *testing.T) { tests := []struct { name string + lifespan time.Duration keys func(encoderSigningKey, encoderEncryptionKey, decoderSigningKey, decoderEncryptionKey *[]byte) wantEncoderErrorPrefix string wantDecoderError string @@ -41,6 +43,11 @@ func TestCodec(t *testing.T) { }, wantDecoderError: "securecookie: error - caused by: crypto/aes: invalid key size 27", }, + { + name: "aaa encoder times stuff out", + lifespan: time.Second, + wantDecoderError: "securecookie: expired timestamp", + }, { name: "bad encoder signing key", keys: func(encoderSigningKey, encoderEncryptionKey, decoderSigningKey, decoderEncryptionKey *[]byte) { @@ -82,7 +89,13 @@ func TestCodec(t *testing.T) { if test.keys != nil { test.keys(&encoderSigningKey, &encoderEncryptionKey, &decoderSigningKey, &decoderEncryptionKey) } - encoder := New(func() []byte { return encoderSigningKey }, + + lifespan := test.lifespan + if lifespan == 0 { + lifespan = time.Hour + } + + encoder := New(lifespan, func() []byte { return encoderSigningKey }, func() []byte { return encoderEncryptionKey }) encoded, err := encoder.Encode("some-name", "some-message") @@ -92,14 +105,18 @@ func TestCodec(t *testing.T) { } require.NoError(t, err) - decoder := New(func() []byte { return decoderSigningKey }, + if test.lifespan != 0 { + time.Sleep(test.lifespan + time.Second) + } + + decoder := New(lifespan, func() []byte { return decoderSigningKey }, func() []byte { return decoderEncryptionKey }) var decoded string err = decoder.Decode("some-name", encoded, &decoded) if test.wantDecoderError != "" { require.Error(t, err) - require.True(t, strings.HasPrefix(err.Error(), test.wantDecoderError)) + require.True(t, strings.HasPrefix(err.Error(), test.wantDecoderError), "expected %q to start with %q", err.Error(), test.wantDecoderError) return } require.NoError(t, err) diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index 233e93f1..0a798485 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -44,6 +44,11 @@ const ( // CSRFCookieEncodingName is the `name` passed to the encoder for encoding and decoding the CSRF // cookie contents. CSRFCookieEncodingName = "csrf" + + // CSRFCookieLifespan is the length of time that the CSRF cookie is valid. After this time, the + // Supervisor's authorization endpoint should give the browser a new CSRF cookie. We set it to + // a week so that it is unlikely to expire during a login. + CSRFCookieLifespan = time.Hour * 24 * 7 ) // Encoder is the encoding side of the securecookie.Codec interface. diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index b09448df..ad33b408 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -77,7 +77,11 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) { m.providers = oidcProviders m.providerHandlers = make(map[string]http.Handler) - var csrfCookieEncoder = dynamiccodec.New(m.cache.GetCSRFCookieEncoderHashKey, m.cache.GetCSRFCookieEncoderBlockKey) + var csrfCookieEncoder = dynamiccodec.New( + oidc.CSRFCookieLifespan, + m.cache.GetCSRFCookieEncoderHashKey, + m.cache.GetCSRFCookieEncoderBlockKey, + ) for _, incomingProvider := range oidcProviders { providerCache := m.cache.GetOIDCProviderCacheFor(incomingProvider.Issuer()) @@ -101,7 +105,11 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) { // For all the other endpoints, make another oauth helper with exactly the same settings except use real storage. oauthHelperWithKubeStorage := oidc.FositeOauth2Helper(oidc.NewKubeStorage(m.secretsClient), issuer, providerCache.GetTokenHMACKey, m.dynamicJWKSProvider, oidcTimeouts) - var upstreamStateEncoder = dynamiccodec.New(providerCache.GetStateEncoderHashKey, providerCache.GetStateEncoderBlockKey) + var upstreamStateEncoder = dynamiccodec.New( + oidcTimeouts.UpstreamStateParamLifespan, + providerCache.GetStateEncoderHashKey, + providerCache.GetStateEncoderBlockKey, + ) m.providerHandlers[(issuerHostWithPath + oidc.WellKnownEndpointPath)] = discovery.NewHandler(issuer)