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) diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index 7e8cd6d7..4a429696 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -88,10 +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 encoder = securecookie.New(encoderHashKey, encoderBlockKey) - encoder.SetSerializer(securecookie.JSONEncoder{}) + 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 csrfCookieEncoderHashKey = []byte("fake-csrf-hash-secret") // TODO replace this secret + var csrfCookieEncoder = securecookie.New(csrfCookieEncoderHashKey, nil) + 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, ) 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 {