diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 6ff4ce61..4003f9c2 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -125,7 +125,7 @@ func TestAuthorizationEndpoint(t *testing.T) { oauthStore := oidc.NullStorage{} hmacSecret := []byte("some secret - must have at least 32 bytes") require.GreaterOrEqual(t, len(hmacSecret), 32, "fosite requires that hmac secrets have at least 32 bytes") - oauthHelper := oidc.FositeOauth2Helper(oauthStore, hmacSecret) + oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecret) happyCSRF := "test-csrf" happyPKCE := "test-pkce" diff --git a/internal/oidc/callback/callback_handler.go b/internal/oidc/callback/callback_handler.go index 24780869..5b725623 100644 --- a/internal/oidc/callback/callback_handler.go +++ b/internal/oidc/callback/callback_handler.go @@ -36,13 +36,9 @@ const ( // downstreamGroupsClaim is what we will use to encode the groups in the downstream OIDC ID token // information. downstreamGroupsClaim = "groups" - - // The lifetime of an issued downstream ID token. - downstreamIDTokenLifetime = time.Minute * 5 ) func NewHandler( - downstreamIssuer string, idpListGetter oidc.IDPListGetter, oauthHelper fosite.OAuth2Provider, stateDecoder, cookieDecoder oidc.Decoder, @@ -97,13 +93,7 @@ func NewHandler( return err } - openIDSession := makeDownstreamSession( - downstreamIssuer, - downstreamAuthParams.Get("client_id"), - downstreamAuthParams.Get("nonce"), - username, - groups, - ) + openIDSession := makeDownstreamSession(username, groups) authorizeResponder, err := oauthHelper.NewAuthorizeResponse(r.Context(), authorizeRequester, openIDSession) if err != nil { plog.WarningErr("error while generating and saving authcode", err, "upstreamName", upstreamIDPConfig.GetName()) @@ -297,16 +287,11 @@ func getGroupsFromUpstreamIDToken( return groups, nil } -func makeDownstreamSession(issuer, clientID, nonce, username string, groups []string) *openid.DefaultSession { - now := time.Now() +func makeDownstreamSession(username string, groups []string) *openid.DefaultSession { + now := time.Now().UTC() openIDSession := &openid.DefaultSession{ Claims: &jwt.IDTokenClaims{ - Issuer: issuer, Subject: username, - Audience: []string{clientID}, - Nonce: nonce, - ExpiresAt: now.Add(downstreamIDTokenLifetime), - IssuedAt: now, RequestedAt: now, AuthTime: now, }, diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index 588eddf9..5f1d16ba 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -430,10 +430,10 @@ func TestCallbackEndpoint(t *testing.T) { } hmacSecret := []byte("some secret - must have at least 32 bytes") require.GreaterOrEqual(t, len(hmacSecret), 32, "fosite requires that hmac secrets have at least 32 bytes") - oauthHelper := oidc.FositeOauth2Helper(oauthStore, hmacSecret) + oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecret) idpListGetter := oidctestutil.NewIDPListGetter(&test.idp) - subject := NewHandler(downstreamIssuer, idpListGetter, oauthHelper, happyStateCodec, happyCookieCodec) + subject := NewHandler(idpListGetter, oauthHelper, happyStateCodec, happyCookieCodec) req := httptest.NewRequest(test.method, test.path, nil) if test.csrfCookie != "" { req.Header.Set("Cookie", test.csrfCookie) @@ -480,7 +480,6 @@ func TestCallbackEndpoint(t *testing.T) { test.wantDownstreamIDTokenSubject, test.wantDownstreamIDTokenGroups, test.wantDownstreamRequestedScopes, - test.wantDownstreamNonce, ) validatePKCEStorage( @@ -688,7 +687,6 @@ func validateAuthcodeStorage( wantDownstreamIDTokenSubject string, wantDownstreamIDTokenGroups []string, wantDownstreamRequestedScopes []string, - wantDownstreamNonce string, ) (*fosite.Request, *openid.DefaultSession) { t.Helper() @@ -740,14 +738,22 @@ func validateAuthcodeStorage( require.NotContains(t, actualClaims.Extra, "groups") } - // Check the rest of the downstream ID token's claims. - require.Equal(t, downstreamIssuer, actualClaims.Issuer) - require.Equal(t, []string{downstreamClientID}, actualClaims.Audience) - require.Equal(t, wantDownstreamNonce, actualClaims.Nonce) - testutil.RequireTimeInDelta(t, time.Now().Add(time.Minute*5), actualClaims.ExpiresAt, timeComparisonFudgeFactor) - testutil.RequireTimeInDelta(t, time.Now(), actualClaims.IssuedAt, timeComparisonFudgeFactor) - testutil.RequireTimeInDelta(t, time.Now(), actualClaims.RequestedAt, timeComparisonFudgeFactor) - testutil.RequireTimeInDelta(t, time.Now(), actualClaims.AuthTime, timeComparisonFudgeFactor) + // Check the rest of the downstream ID token's claims. Fosite wants us to set these (in UTC time). + testutil.RequireTimeInDelta(t, time.Now().UTC(), actualClaims.RequestedAt, timeComparisonFudgeFactor) + testutil.RequireTimeInDelta(t, time.Now().UTC(), actualClaims.AuthTime, timeComparisonFudgeFactor) + requestedAtZone, _ := actualClaims.RequestedAt.Zone() + require.Equal(t, "UTC", requestedAtZone) + authTimeZone, _ := actualClaims.AuthTime.Zone() + require.Equal(t, "UTC", authTimeZone) + + // Fosite will set these fields for us in the token endpoint based on the store session + // information. Therefore, we assert that they are empty because we want the library to do the + // lifting for us. + require.Empty(t, actualClaims.Issuer) + require.Nil(t, actualClaims.Audience) + require.Empty(t, actualClaims.Nonce) + require.Zero(t, actualClaims.ExpiresAt) + require.Zero(t, actualClaims.IssuedAt) // These are not needed yet. require.Empty(t, actualClaims.JTI) diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index 8b73e662..017b9249 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -85,9 +85,10 @@ func PinnipedCLIOIDCClient() *fosite.DefaultOpenIDConnectClient { } } -func FositeOauth2Helper(oauthStore interface{}, hmacSecretOfLengthAtLeast32 []byte) fosite.OAuth2Provider { +func FositeOauth2Helper(oauthStore interface{}, issuer string, hmacSecretOfLengthAtLeast32 []byte) fosite.OAuth2Provider { oauthConfig := &compose.Config{ EnforcePKCEForPublicClients: true, + IDTokenIssuer: issuer, } return compose.Compose( diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index deb1cfc4..7c6403b9 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -71,7 +71,7 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) { // Use NullStorage for the authorize endpoint because we do not actually want to store anything until // the upstream callback endpoint is called later. - oauthHelper := oidc.FositeOauth2Helper(oidc.NullStorage{}, []byte("some secret - must have at least 32 bytes")) // TODO replace this secret + oauthHelper := oidc.FositeOauth2Helper(oidc.NullStorage{}, incomingProvider.Issuer(), []byte("some secret - must have at least 32 bytes")) // TODO replace this secret // TODO use different codecs for the state and the cookie, because: // 1. we would like to state to have an embedded expiration date while the cookie does not need that @@ -86,7 +86,7 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) { m.providerHandlers[authURL] = auth.NewHandler(incomingProvider.Issuer(), m.idpListGetter, oauthHelper, csrftoken.Generate, pkce.Generate, nonce.Generate, encoder, encoder) callbackURL := strings.ToLower(incomingProvider.IssuerHost()) + "/" + incomingProvider.IssuerPath() + oidc.CallbackEndpointPath - m.providerHandlers[callbackURL] = callback.NewHandler(incomingProvider.Issuer(), m.idpListGetter, oauthHelper, encoder, encoder) + m.providerHandlers[callbackURL] = callback.NewHandler(m.idpListGetter, oauthHelper, encoder, encoder) plog.Debug("oidc provider manager added or updated issuer", "issuer", incomingProvider.Issuer()) }