From 6ec35891122099634b20fcb8cc931ef60e0b5897 Mon Sep 17 00:00:00 2001 From: aram price Date: Wed, 9 Dec 2020 16:29:25 -0800 Subject: [PATCH 1/5] Use recorder `Cookies()` helper - replaces hand-parsing of cookie strings --- internal/oidc/provider/manager/manager_test.go | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/internal/oidc/provider/manager/manager_test.go b/internal/oidc/provider/manager/manager_test.go index ed048a31..0720b719 100644 --- a/internal/oidc/provider/manager/manager_test.go +++ b/internal/oidc/provider/manager/manager_test.go @@ -104,17 +104,14 @@ func TestManager(t *testing.T) { redirectStateParam := parsedLocation.Query().Get("state") r.NotEmpty(redirectStateParam) - cookieValueAndDirectivesSplit := strings.SplitN(recorder.Header().Get("Set-Cookie"), ";", 2) - r.Len(cookieValueAndDirectivesSplit, 2) - cookieKeyValueSplit := strings.Split(cookieValueAndDirectivesSplit[0], "=") - r.Len(cookieKeyValueSplit, 2) - csrfCookieName := cookieKeyValueSplit[0] - r.Equal("__Host-pinniped-csrf", csrfCookieName) - csrfCookieValue := cookieKeyValueSplit[1] - r.NotEmpty(csrfCookieValue) + cookies := recorder.Result().Cookies() //nolint:bodyclose + r.Len(cookies, 1) + csrfCookie := cookies[0] + r.Equal("__Host-pinniped-csrf", csrfCookie.Name) + r.NotEmpty(csrfCookie.Value) // Return the important parts of the response so we can use them in our next request to the callback endpoint - return csrfCookieValue, redirectStateParam + return csrfCookie.Value, redirectStateParam } requireCallbackRequestToBeHandled := func(requestIssuer, requestURLSuffix, csrfCookieValue string) string { From e111ca02dacdecfe8e549dc0e6cd1aa916336dd8 Mon Sep 17 00:00:00 2001 From: aram price Date: Wed, 9 Dec 2020 17:20:57 -0800 Subject: [PATCH 2/5] Use the narrowest possible interface --- internal/oidc/auth/auth_handler.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index 61ab7529..36e42b25 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -136,7 +136,7 @@ func NewHandler( }) } -func readCSRFCookie(r *http.Request, codec oidc.Codec) csrftoken.CSRFToken { +func readCSRFCookie(r *http.Request, codec oidc.Decoder) csrftoken.CSRFToken { receivedCSRFCookie, err := r.Cookie(oidc.CSRFCookieName) if err != nil { // Error means that the cookie was not found @@ -214,7 +214,7 @@ func upstreamStateParam( return encodedStateParamValue, nil } -func addCSRFSetCookieHeader(w http.ResponseWriter, csrfValue csrftoken.CSRFToken, codec oidc.Codec) error { +func addCSRFSetCookieHeader(w http.ResponseWriter, csrfValue csrftoken.CSRFToken, codec oidc.Encoder) error { encodedCSRFValue, err := codec.Encode(oidc.CSRFCookieEncodingName, csrfValue) if err != nil { return httperr.Wrap(http.StatusInternalServerError, "error encoding CSRF cookie", err) From 4a5f8e30a806db188c73447434905722cbd3c0fd Mon Sep 17 00:00:00 2001 From: aram price Date: Wed, 9 Dec 2020 17:24:12 -0800 Subject: [PATCH 3/5] Use distinct `Encoder` for state and csrf data --- internal/oidc/provider/manager/manager.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index 7e8cd6d7..7fdce529 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -90,8 +90,12 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) { // 3. we would like *all* downstream providers to use the *same* signing key for the CSRF cookie (which doesn't need to be encrypted) because cookies are sent per-domain and our issuers can share a domain name (but have different paths) var encoderHashKey = []byte("fake-hash-secret") // TODO replace this secret var encoderBlockKey = []byte("16-bytes-aaaaaaa") // TODO replace this secret - var encoder = securecookie.New(encoderHashKey, encoderBlockKey) - encoder.SetSerializer(securecookie.JSONEncoder{}) + + var upstreamStateEncoder = securecookie.New(encoderHashKey, encoderBlockKey) + upstreamStateEncoder.SetSerializer(securecookie.JSONEncoder{}) + + var csrfCookieEncoder = securecookie.New(encoderHashKey, encoderBlockKey) + csrfCookieEncoder.SetSerializer(securecookie.JSONEncoder{}) m.providerHandlers[(issuerHostWithPath + oidc.WellKnownEndpointPath)] = discovery.NewHandler(issuer) @@ -104,15 +108,15 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) { csrftoken.Generate, pkce.Generate, nonce.Generate, - encoder, - encoder, + upstreamStateEncoder, + csrfCookieEncoder, ) m.providerHandlers[(issuerHostWithPath + oidc.CallbackEndpointPath)] = callback.NewHandler( m.idpListGetter, oauthHelperWithKubeStorage, - encoder, - encoder, + upstreamStateEncoder, + csrfCookieEncoder, issuer+oidc.CallbackEndpointPath, ) From f1f8ffa456136f802b073f9f689b8ed356b8cbfe Mon Sep 17 00:00:00 2001 From: aram price Date: Wed, 9 Dec 2020 17:26:48 -0800 Subject: [PATCH 4/5] Distinct `Encoder`'s use distinct keys --- internal/oidc/provider/manager/manager.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index 7fdce529..2cd8b18f 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -88,13 +88,14 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) { // 1. we would like to state to have an embedded expiration date while the cookie does not need that // 2. we would like each downstream provider to use different secrets for signing/encrypting the upstream state, not share secrets // 3. we would like *all* downstream providers to use the *same* signing key for the CSRF cookie (which doesn't need to be encrypted) because cookies are sent per-domain and our issuers can share a domain name (but have different paths) - var encoderHashKey = []byte("fake-hash-secret") // TODO replace this secret - var encoderBlockKey = []byte("16-bytes-aaaaaaa") // TODO replace this secret - - var upstreamStateEncoder = securecookie.New(encoderHashKey, encoderBlockKey) + var upstreamStateEncoderHashKey = []byte("fake-state-hash-secret") // TODO replace this secret + var upstreamStateEncoderBlockKey = []byte("16-bytes-STATE01") // TODO replace this secret + var upstreamStateEncoder = securecookie.New(upstreamStateEncoderHashKey, upstreamStateEncoderBlockKey) upstreamStateEncoder.SetSerializer(securecookie.JSONEncoder{}) - var csrfCookieEncoder = securecookie.New(encoderHashKey, encoderBlockKey) + var csrfCookieEncoderHashKey = []byte("fake-csrf-hash-secret") // TODO replace this secret + var csrfCookieEncoderBlockKey = []byte("16-bytes-CSRF012") // TODO replace this secret + var csrfCookieEncoder = securecookie.New(csrfCookieEncoderHashKey, csrfCookieEncoderBlockKey) csrfCookieEncoder.SetSerializer(securecookie.JSONEncoder{}) m.providerHandlers[(issuerHostWithPath + oidc.WellKnownEndpointPath)] = discovery.NewHandler(issuer) From 86c75b7a80beb7a02038672009be29ba7fc7e812 Mon Sep 17 00:00:00 2001 From: aram price Date: Wed, 9 Dec 2020 17:29:44 -0800 Subject: [PATCH 5/5] CSRF cookie is no longer encrypted --- internal/oidc/provider/manager/manager.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index 2cd8b18f..4a429696 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -94,8 +94,7 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) { upstreamStateEncoder.SetSerializer(securecookie.JSONEncoder{}) var csrfCookieEncoderHashKey = []byte("fake-csrf-hash-secret") // TODO replace this secret - var csrfCookieEncoderBlockKey = []byte("16-bytes-CSRF012") // TODO replace this secret - var csrfCookieEncoder = securecookie.New(csrfCookieEncoderHashKey, csrfCookieEncoderBlockKey) + var csrfCookieEncoder = securecookie.New(csrfCookieEncoderHashKey, nil) csrfCookieEncoder.SetSerializer(securecookie.JSONEncoder{}) m.providerHandlers[(issuerHostWithPath + oidc.WellKnownEndpointPath)] = discovery.NewHandler(issuer)