From ed96b597c7ca56903eef23be5f5846fdaf1cac95 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Mon, 29 Nov 2021 16:44:58 -0800 Subject: [PATCH 1/8] Check for subject matching with upstream refresh Signed-off-by: Margo Crawford --- .../provider/dynamic_upstream_idp_provider.go | 7 ++ internal/upstreamoidc/upstreamoidc.go | 110 +++++++++++++---- internal/upstreamoidc/upstreamoidc_test.go | 111 ++++++++++++++++++ 3 files changed, 208 insertions(+), 20 deletions(-) diff --git a/internal/oidc/provider/dynamic_upstream_idp_provider.go b/internal/oidc/provider/dynamic_upstream_idp_provider.go index 2c34dcbe..5207945b 100644 --- a/internal/oidc/provider/dynamic_upstream_idp_provider.go +++ b/internal/oidc/provider/dynamic_upstream_idp_provider.go @@ -103,6 +103,13 @@ type StoredRefreshAttributes struct { AdditionalAttributes map[string]string } +type StoredRefreshAttributes struct { + Username string + Subject string + DN string + AuthTime time.Time +} + type DynamicUpstreamIDPProvider interface { SetOIDCIdentityProviders(oidcIDPs []UpstreamOIDCIdentityProviderI) GetOIDCIdentityProviders() []UpstreamOIDCIdentityProviderI diff --git a/internal/upstreamoidc/upstreamoidc.go b/internal/upstreamoidc/upstreamoidc.go index 437eb6ef..abc2fd8e 100644 --- a/internal/upstreamoidc/upstreamoidc.go +++ b/internal/upstreamoidc/upstreamoidc.go @@ -7,6 +7,7 @@ package upstreamoidc import ( "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -129,6 +130,7 @@ func (p *ProviderConfig) ExchangeAuthcodeAndValidateTokens(ctx context.Context, return p.ValidateToken(ctx, tok, expectedIDTokenNonce) } +// TODO this is reused between the client and the supervisor... don't change it. func (p *ProviderConfig) PerformRefresh(ctx context.Context, refreshToken string) (*oauth2.Token, error) { // Use the provided HTTP client to benefit from its CA, proxy, and other settings. httpClientContext := coreosoidc.ClientContext(ctx, p.Client) @@ -236,6 +238,57 @@ func (p *ProviderConfig) tryRevokeRefreshToken( } } +func (p *ProviderConfig) ValidateRefresh(ctx context.Context, tok *oauth2.Token, storedAttributes provider.StoredRefreshAttributes) error { + idTok, hasIDTok := tok.Extra("id_token").(string) + var validatedClaims = make(map[string]interface{}) + if hasIDTok { + coreosConfig := &coreosoidc.Config{ClientID: p.GetClientID()} + coreosClientContext := coreosoidc.ClientContext(ctx, p.Client) + verifier := p.Provider.Verifier(coreosConfig) + validated, err := verifier.Verify(coreosClientContext, idTok) + if err != nil { + return httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err) + } + if err := validated.Claims(&validatedClaims); err != nil { + return httperr.Wrap(http.StatusInternalServerError, "could not unmarshal id token claims", err) + } + maybeLogClaims("claims from ID token", p.Name, validatedClaims) + } + + originalUpstreamSubject, err := extractUpstreamSubjectFromDownstream(storedAttributes.Subject) + if err != nil { + return httperr.Wrap(http.StatusInternalServerError, "could not parse stored subject", err) + } + + // it's okay to not have an id token. It's okay to have an id token with a subject. + // but if we have an id token without a subject that's a problem. + idTokenSubject, _ := validatedClaims[oidc.IDTokenSubjectClaim].(string) + switch { + case len(idTokenSubject) > 0: + // TODO url escape + if url.QueryEscape(idTokenSubject) != originalUpstreamSubject { + return httperr.Newf(http.StatusInternalServerError, "subject from id token did not match previous stored value. New subject: %s. Old subject: %s", idTokenSubject, originalUpstreamSubject) + } + case len(validatedClaims) == 0: + validatedClaims[oidc.IDTokenSubjectClaim] = originalUpstreamSubject + default: + return httperr.New(http.StatusInternalServerError, "id token did not have a subject") + } + + if err := p.maybeFetchUserInfoAndMergeClaims(ctx, tok, validatedClaims); err != nil { + return err + } + + return nil +} + +func extractUpstreamSubjectFromDownstream(downstreamSubject string) (string, error) { + if !strings.Contains(downstreamSubject, "?sub=") { + return "", errors.New("downstream subject did not contain original upstream subject") + } + return strings.Split(downstreamSubject, "?sub=")[1], nil +} + // ValidateToken will validate the ID token. It will also merge the claims from the userinfo endpoint response, // if the provider offers the userinfo endpoint. func (p *ProviderConfig) ValidateToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) { @@ -264,8 +317,11 @@ func (p *ProviderConfig) ValidateToken(ctx context.Context, tok *oauth2.Token, e } maybeLogClaims("claims from ID token", p.Name, validatedClaims) - if err := p.maybeFetchUserInfoAndMergeClaims(ctx, tok, validatedClaims); err != nil { - return nil, httperr.Wrap(http.StatusInternalServerError, "could not fetch user info claims", err) + idTokenSubject, _ := validatedClaims[oidc.IDTokenSubjectClaim].(string) + if len(idTokenSubject) > 0 { + if err := p.maybeFetchUserInfoAndMergeClaims(ctx, tok, validatedClaims); err != nil { + return nil, httperr.Wrap(http.StatusInternalServerError, "could not fetch user info claims", err) + } } return &oidctypes.Token{ @@ -286,29 +342,22 @@ func (p *ProviderConfig) ValidateToken(ctx context.Context, tok *oauth2.Token, e } func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, tok *oauth2.Token, claims map[string]interface{}) error { + // TODO separate this. + // extract: fetching userinfo + // - validate some userinfo? subject stuff: for refresh subjects must match but also match stored subject + // - extract: merging claims + // - deciding when to do each of those things idTokenSubject, _ := claims[oidc.IDTokenSubjectClaim].(string) - if len(idTokenSubject) == 0 { - return nil // defer to existing ID token validation - } - providerJSON := &struct { - UserInfoURL string `json:"userinfo_endpoint"` - }{} - if err := p.Provider.Claims(providerJSON); err != nil { - // this should never happen because we should have already parsed these claims at an earlier stage - return httperr.Wrap(http.StatusInternalServerError, "could not unmarshal discovery JSON", err) + userInfo, err := p.fetchUserInfo(ctx, tok) + if err != nil { + return err } - - // implementing the user info endpoint is not required, skip this logic when it is absent - if len(providerJSON.UserInfoURL) == 0 { + if userInfo == nil { return nil } - userInfo, err := p.Provider.UserInfo(coreosoidc.ClientContext(ctx, p.Client), oauth2.StaticTokenSource(tok)) - if err != nil { - return httperr.Wrap(http.StatusInternalServerError, "could not get user info", err) - } - + // TODO if there is no idTokenSubject, defer to checking the stored claims. // The sub (subject) Claim MUST always be returned in the UserInfo Response. // // NOTE: Due to the possibility of token substitution attacks (see Section 16.11), the UserInfo Response is not @@ -317,7 +366,7 @@ func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, t // the UserInfo Response values MUST NOT be used. // // http://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse - if len(userInfo.Subject) == 0 || userInfo.Subject != idTokenSubject { + if (len(idTokenSubject) > 0) && (len(userInfo.Subject) == 0 || userInfo.Subject != idTokenSubject) { return httperr.Newf(http.StatusUnprocessableEntity, "userinfo 'sub' claim (%s) did not match id_token 'sub' claim (%s)", userInfo.Subject, idTokenSubject) } @@ -331,6 +380,27 @@ func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, t return nil } +func (p *ProviderConfig) fetchUserInfo(ctx context.Context, tok *oauth2.Token) (*coreosoidc.UserInfo, error) { + providerJSON := &struct { + UserInfoURL string `json:"userinfo_endpoint"` + }{} + if err := p.Provider.Claims(providerJSON); err != nil { + // this should never happen because we should have already parsed these claims at an earlier stage + return nil, httperr.Wrap(http.StatusInternalServerError, "could not unmarshal discovery JSON", err) + } + + // implementing the user info endpoint is not required, skip this logic when it is absent + if len(providerJSON.UserInfoURL) == 0 { + return nil, nil + } + + userInfo, err := p.Provider.UserInfo(coreosoidc.ClientContext(ctx, p.Client), oauth2.StaticTokenSource(tok)) + if err != nil { + return nil, httperr.Wrap(http.StatusInternalServerError, "could not get user info", err) + } + return userInfo, nil +} + func maybeLogClaims(msg, name string, claims map[string]interface{}) { if plog.Enabled(plog.LevelAll) { // log keys and values at all level data, _ := json.Marshal(claims) // nothing we can do if it fails, but it really never should diff --git a/internal/upstreamoidc/upstreamoidc_test.go b/internal/upstreamoidc/upstreamoidc_test.go index f8906562..30f1318a 100644 --- a/internal/upstreamoidc/upstreamoidc_test.go +++ b/internal/upstreamoidc/upstreamoidc_test.go @@ -16,6 +16,8 @@ import ( "time" "unsafe" + "go.pinniped.dev/internal/oidc/provider" + "github.com/coreos/go-oidc/v3/oidc" "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" @@ -584,6 +586,115 @@ func TestProviderConfig(t *testing.T) { if tt.wantErr != "" { require.EqualError(t, err, tt.wantErr) + } + }) + } + }) + + t.Run("ValidateRefresh", func(t *testing.T) { + testTokenWithoutIDToken := &oauth2.Token{ + AccessToken: "test-access-token", + // the library sets the original refresh token into the result, even though the server did not return that + RefreshToken: "test-initial-refresh-token", + TokenType: "test-token-type", + Expiry: time.Now().Add(42 * time.Second), + } + + tests := []struct { + name string + token *oauth2.Token + storedAttributes provider.StoredRefreshAttributes + userInfo *oidc.UserInfo + rawClaims []byte + userInfoErr error + wantErr string + }{ + { + name: "id, access and refresh token returned from refresh", + token: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJuYW1lIjoiUGlubnkgVGhlU2VhbCIsImlzcyI6InNvbWUtaXNzdWVyIiwiaWF0IjoxNTE2MjM5MDIyfQ.qVMIsmrMLiMW7kJUNO4RpiBnkd6MYKdXXMMb6UWVXL0zXkalb1tP2DO3Yp73ZfWqCNdNEa1qqSJIR9F46cnAp04pZmOvVqTWCmaXaxvklRb_QzdlKi-3QNg7wEJ50a11vhUTJxLYrkH9pJpPILzd8Kx0yp43iEJbBG3nFVZFOSQOh7wCGUF4QkNLRMT9buq8DiypKPmjksVmObkF3AbvQ1IvaA_UPBmFvL-pun-TjUBR37xT7JI64wbczJJclThaqTcK6bQcbiBG4jonckYIIryoL8UUBSbxQGGz7lQK5_77Q4PTXAE3c5hrg-c6RK52YNBm-B5Gx7vAeBavrQJaPQ"}), + storedAttributes: provider.StoredRefreshAttributes{Subject: "some-issuer?sub=some-subject"}, + userInfo: forceUserInfoWithClaims("some-subject", `{"foo": "bar"}`), + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + }, + { + name: "id token exists but has no subject", + token: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJuYW1lIjoiUGlubnkgVGhlU2VhbCIsImlhdCI6MTUxNjIzOTAyMn0.FmkkY7MCnhD98F5Aupq_YdW54pH90O60O9SwjxWHZb6_k6LoJcjZEMyxba1Fg5tlGLOSqH7BffZOfMUZ5YqVeA7ZIZ9fhAiudGdJqKz5nI394xzR-EejGqBI0fTAuxa_0VsLSC5mV0hTctK01kvemPwMLhIUn_HHr0-khugPFZH8zlYxAFmBpe9YxWK5w1JcTWuSMjRn6_b1RDSgxpff-LcmvVz6akcnlb2j8Od7S9JinsSqKjb7zBjQmcVGT6BGdN43tsBJMEOzil6xpvsH_KIToWlyqmvCQFp_udcKWuUNNLdypGW4eMXvPF5GINMDhki0GZDoKLkPnHVk_mZFXw"}), + storedAttributes: provider.StoredRefreshAttributes{Subject: "some-issuer?sub=some-subject"}, + userInfo: &oidc.UserInfo{Subject: "some-subject"}, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + wantErr: "id token did not have a subject", + }, + { + name: "id token doesn't exist but userinfo does", + token: testTokenWithoutIDToken, + storedAttributes: provider.StoredRefreshAttributes{Subject: "some-issuer?sub=some-subject"}, + userInfo: forceUserInfoWithClaims("some-subject", `{"foo": "bar"}`), + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + }, + { + name: "subject in id token doesn't match stored subject", + token: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJuYW1lIjoiUGlubnkgVGhlU2VhbCIsImlzcyI6InNvbWUtaXNzdWVyIiwiaWF0IjoxNTE2MjM5MDIyfQ.qVMIsmrMLiMW7kJUNO4RpiBnkd6MYKdXXMMb6UWVXL0zXkalb1tP2DO3Yp73ZfWqCNdNEa1qqSJIR9F46cnAp04pZmOvVqTWCmaXaxvklRb_QzdlKi-3QNg7wEJ50a11vhUTJxLYrkH9pJpPILzd8Kx0yp43iEJbBG3nFVZFOSQOh7wCGUF4QkNLRMT9buq8DiypKPmjksVmObkF3AbvQ1IvaA_UPBmFvL-pun-TjUBR37xT7JI64wbczJJclThaqTcK6bQcbiBG4jonckYIIryoL8UUBSbxQGGz7lQK5_77Q4PTXAE3c5hrg-c6RK52YNBm-B5Gx7vAeBavrQJaPQ"}), + storedAttributes: provider.StoredRefreshAttributes{Subject: "some-issuer?sub=some-other-subject"}, + wantErr: "subject from id token did not match previous stored value. New subject: some-subject. Old subject: some-other-subject", + }, + { + name: "malformed stored subject", + token: testTokenWithoutIDToken, + storedAttributes: provider.StoredRefreshAttributes{Subject: "wrong-format"}, + wantErr: "could not parse stored subject: downstream subject did not contain original upstream subject", + }, + { + name: "subject in user info doesn't match stored subject", + token: testTokenWithoutIDToken, + storedAttributes: provider.StoredRefreshAttributes{Subject: "some-issuer?sub=some-other-subject"}, + userInfo: forceUserInfoWithClaims("some-subject", `{"foo": "bar"}`), + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + wantErr: "userinfo 'sub' claim (some-subject) did not match id_token 'sub' claim (some-other-subject)", // TODO is this a good enough error message?... + }, + { + name: "subject in id token doesn't match userinfo", + token: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJuYW1lIjoiUGlubnkgVGhlU2VhbCIsImlhdCI6MTUxNjIzOTAyMn0.P_97A2ONBt-YV0JHM3DDugNhG-codkK8fLU4EOdlGHcVBmWVbbW1g7-U0jjB9gcvUt24TS5z0TxEnKNUG8LiqS-8FbHGjv8DN1gAz4huBxNqmMYwzGsFNKDBcUMpv9DNFT-oIrxSf3pPYFlg7N0YKzbMPRwVaL1iQmzeQDCQieV3mDCWXpxpdLa1Mfj_NRsEe4027xOZYJn14pn83h92rG0VcoV2e39LuuEir-tRtDniY4WFIMPudNJdC0NwoMoRIQuHStCIN5AkbHPuhGQivqoObWX6RgNU18qb6CcCi86WhG45uHO77gfz5TbGsyVhr5LRBEZP5HPqsq5D6853zQ"}), + storedAttributes: provider.StoredRefreshAttributes{Subject: "some-issuer?sub=some-subject"}, + userInfo: &oidc.UserInfo{Subject: "some-other-subject"}, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + wantErr: "userinfo 'sub' claim (some-other-subject) did not match id_token 'sub' claim (some-subject)", + }, + { + name: "id token with format that isn't a jwt returned from refresh", + token: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "not-jwt-format"}), + userInfo: &oidc.UserInfo{Subject: "test-user-2"}, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + wantErr: "received invalid ID token: oidc: malformed jwt: square/go-jose: compact JWS format must have three parts", + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + p := ProviderConfig{ + Name: "test-name", + UsernameClaim: "test-username-claim", + GroupsClaim: "test-groups-claim", + Config: &oauth2.Config{ + ClientID: "test-client-id", + ClientSecret: "test-client-secret", + Endpoint: oauth2.Endpoint{ + AuthURL: "https://example.com", + TokenURL: "https://example.com", + AuthStyle: oauth2.AuthStyleInParams, + }, + Scopes: []string{"scope1", "scope2"}, + }, + Provider: &mockProvider{ + rawClaims: tt.rawClaims, + userInfo: tt.userInfo, + userInfoErr: tt.userInfoErr, + }, + } + + err := p.ValidateRefresh(context.Background(), tt.token, tt.storedAttributes) + if tt.wantErr != "" { + require.Error(t, err) + require.Equal(t, err.Error(), tt.wantErr) } else { require.NoError(t, err) } From 74b007ff66870de24e542864e2aa4f4d45564ba9 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Fri, 3 Dec 2021 16:11:53 -0800 Subject: [PATCH 2/8] Validate that issuer url and urls returned from discovery are https and that they have no query or fragment Signed-off-by: Ryan Richard --- .../oidc_upstream_watcher.go | 91 +++-- .../oidc_upstream_watcher_test.go | 338 +++++++++++++++++- 2 files changed, 384 insertions(+), 45 deletions(-) diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go index f2d3aa9f..909c07fb 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go @@ -324,6 +324,11 @@ func (c *oidcWatcherController) validateIssuer(ctx context.Context, upstream *v1 } } + _, issuerURLCondition := validateHTTPSURL(upstream.Spec.Issuer, "issuer", reasonUnreachable) + if issuerURLCondition != nil { + return issuerURLCondition + } + discoveredProvider, err = oidc.NewProvider(oidc.ClientContext(ctx, httpClient), upstream.Spec.Issuer) if err != nil { const klogLevelTrace = 6 @@ -359,46 +364,35 @@ func (c *oidcWatcherController) validateIssuer(ctx context.Context, upstream *v1 } } if additionalDiscoveryClaims.RevocationEndpoint != "" { - // Found a revocation URL. Try to parse it. - revocationURL, err := url.Parse(additionalDiscoveryClaims.RevocationEndpoint) - if err != nil { - return &v1alpha1.Condition{ - Type: typeOIDCDiscoverySucceeded, - Status: v1alpha1.ConditionFalse, - Reason: reasonInvalidResponse, - Message: fmt.Sprintf("failed to parse revocation endpoint URL: %v", err), - } - } - // Don't want to send refresh tokens to an insecure revocation endpoint, so require that it use https. - if revocationURL.Scheme != "https" { - return &v1alpha1.Condition{ - Type: typeOIDCDiscoverySucceeded, - Status: v1alpha1.ConditionFalse, - Reason: reasonInvalidResponse, - Message: fmt.Sprintf(`revocation endpoint URL scheme must be "https", not %q`, revocationURL.Scheme), - } + // Found a revocation URL. Validate it. + revocationURL, revocationURLCondition := validateHTTPSURL( + additionalDiscoveryClaims.RevocationEndpoint, + "revocation endpoint", + reasonInvalidResponse, + ) + if revocationURLCondition != nil { + return revocationURLCondition } // Remember the URL for later use. result.RevocationURL = revocationURL } - // Parse out and validate the discovered authorize endpoint. - authURL, err := url.Parse(discoveredProvider.Endpoint().AuthURL) - if err != nil { - return &v1alpha1.Condition{ - Type: typeOIDCDiscoverySucceeded, - Status: v1alpha1.ConditionFalse, - Reason: reasonInvalidResponse, - Message: fmt.Sprintf("failed to parse authorization endpoint URL: %v", err), - } + _, authorizeURLCondition := validateHTTPSURL( + discoveredProvider.Endpoint().AuthURL, + "authorization endpoint", + reasonInvalidResponse, + ) + if authorizeURLCondition != nil { + return authorizeURLCondition } - if authURL.Scheme != "https" { - return &v1alpha1.Condition{ - Type: typeOIDCDiscoverySucceeded, - Status: v1alpha1.ConditionFalse, - Reason: reasonInvalidResponse, - Message: fmt.Sprintf(`authorization endpoint URL scheme must be "https", not %q`, authURL.Scheme), - } + + _, tokenURLCondition := validateHTTPSURL( + discoveredProvider.Endpoint().TokenURL, + "token endpoint", + reasonInvalidResponse, + ) + if tokenURLCondition != nil { + return tokenURLCondition } // If everything is valid, update the result and set the condition to true. @@ -489,3 +483,32 @@ func truncateMostLongErr(err error) string { return msg[:max] + fmt.Sprintf(" [truncated %d chars]", len(msg)-max) } + +func validateHTTPSURL(maybeHTTPSURL, endpointType, reason string) (*url.URL, *v1alpha1.Condition) { + parsedURL, err := url.Parse(maybeHTTPSURL) + if err != nil { + return nil, &v1alpha1.Condition{ + Type: typeOIDCDiscoverySucceeded, + Status: v1alpha1.ConditionFalse, + Reason: reason, + Message: fmt.Sprintf("failed to parse %s URL: %v", endpointType, truncateMostLongErr(err)), + } + } + if parsedURL.Scheme != "https" { + return nil, &v1alpha1.Condition{ + Type: typeOIDCDiscoverySucceeded, + Status: v1alpha1.ConditionFalse, + Reason: reason, + Message: fmt.Sprintf(`%s URL scheme must be "https", not %q`, endpointType, parsedURL.Scheme), + } + } + if len(parsedURL.Query()) != 0 || parsedURL.Fragment != "" { + return nil, &v1alpha1.Condition{ + Type: typeOIDCDiscoverySucceeded, + Status: v1alpha1.ConditionFalse, + Reason: reason, + Message: fmt.Sprintf(`%s URL cannot contain query or fragment component`, endpointType), + } + } + return parsedURL, nil +} diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go index 72fa284f..8a50d13a 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go @@ -399,7 +399,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, Spec: v1alpha1.OIDCIdentityProviderSpec{ - Issuer: "invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee", + Issuer: "%invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee", Client: v1alpha1.OIDCClient{SecretName: testSecretName}, }, }}, @@ -410,11 +410,10 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { }}, wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ - `oidc-upstream-observer "msg"="failed to perform OIDC discovery" "error"="Get \"invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee/.well-known/openid-configuration\": unsupported protocol scheme \"\"" "issuer"="invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee" "name"="test-name" "namespace"="test-namespace"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="failed to perform OIDC discovery against \"invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee\":\nGet \"invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee/.well-known/openid-configuration\": unsupported protocol [truncated 9 chars]" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="failed to parse issuer URL: parse \"%invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee\": invalid URL escape \"%in\"" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, - `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="failed to perform OIDC discovery against \"invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee\":\nGet \"invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee/.well-known/openid-configuration\": unsupported protocol [truncated 9 chars]" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="failed to parse issuer URL: parse \"%invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee\": invalid URL escape \"%in\"" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, }, wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ @@ -435,8 +434,145 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { Status: "False", LastTransitionTime: now, Reason: "Unreachable", - Message: `failed to perform OIDC discovery against "invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee": -Get "invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee/.well-known/openid-configuration": unsupported protocol [truncated 9 chars]`, + Message: `failed to parse issuer URL: parse "%invalid-url-that-is-really-really-long-nanananananananannanananan-batman-nanananananananananananananana-batman-lalalalalalalalalal-batman-weeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee": invalid URL escape "%in"`, + }, + }, + }, + }}, + }, + { + name: "issuer is insecure http URL", + inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Spec: v1alpha1.OIDCIdentityProviderSpec{ + Issuer: strings.Replace(testIssuerURL, "https", "http", 1), + Client: v1alpha1.OIDCClient{SecretName: testSecretName}, + }, + }}, + inputSecrets: []runtime.Object{&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName}, + Type: "secrets.pinniped.dev/oidc-client", + Data: testValidSecretData, + }}, + wantErr: controllerlib.ErrSyntheticRequeue.Error(), + wantLogs: []string{ + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="issuer URL scheme must be \"https\", not \"http\"" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, + `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="issuer URL scheme must be \"https\", not \"http\"" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, + }, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, + wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Status: v1alpha1.OIDCIdentityProviderStatus{ + Phase: "Error", + Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidCondition, + { + Type: "ClientCredentialsValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded client credentials", + }, + { + Type: "OIDCDiscoverySucceeded", + Status: "False", + LastTransitionTime: now, + Reason: "Unreachable", + Message: `issuer URL scheme must be "https", not "http"`, + }, + }, + }, + }}, + }, + { + name: "issuer contains a query param", + inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Spec: v1alpha1.OIDCIdentityProviderSpec{ + Issuer: testIssuerURL + "?sub=foo", + Client: v1alpha1.OIDCClient{SecretName: testSecretName}, + }, + }}, + inputSecrets: []runtime.Object{&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName}, + Type: "secrets.pinniped.dev/oidc-client", + Data: testValidSecretData, + }}, + wantErr: controllerlib.ErrSyntheticRequeue.Error(), + wantLogs: []string{ + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="issuer URL cannot contain query or fragment component" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, + `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="issuer URL cannot contain query or fragment component" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, + }, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, + wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Status: v1alpha1.OIDCIdentityProviderStatus{ + Phase: "Error", + Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidCondition, + { + Type: "ClientCredentialsValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded client credentials", + }, + { + Type: "OIDCDiscoverySucceeded", + Status: "False", + LastTransitionTime: now, + Reason: "Unreachable", + Message: `issuer URL cannot contain query or fragment component`, + }, + }, + }, + }}, + }, + { + name: "issuer contains a fragment", + inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Spec: v1alpha1.OIDCIdentityProviderSpec{ + Issuer: testIssuerURL + "#fragment", + Client: v1alpha1.OIDCClient{SecretName: testSecretName}, + }, + }}, + inputSecrets: []runtime.Object{&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName}, + Type: "secrets.pinniped.dev/oidc-client", + Data: testValidSecretData, + }}, + wantErr: controllerlib.ErrSyntheticRequeue.Error(), + wantLogs: []string{ + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="issuer URL cannot contain query or fragment component" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, + `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="issuer URL cannot contain query or fragment component" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, + }, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, + wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Status: v1alpha1.OIDCIdentityProviderStatus{ + Phase: "Error", + Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidCondition, + { + Type: "ClientCredentialsValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded client credentials", + }, + { + Type: "OIDCDiscoverySucceeded", + Status: "False", + LastTransitionTime: now, + Reason: "Unreachable", + Message: `issuer URL cannot contain query or fragment component`, }, }, }, @@ -679,6 +815,147 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana }, }}, }, + { + name: "issuer returns insecure token URL", + inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Spec: v1alpha1.OIDCIdentityProviderSpec{ + Issuer: testIssuerURL + "/insecure-token-url", + TLS: &v1alpha1.TLSSpec{CertificateAuthorityData: testIssuerCABase64}, + Client: v1alpha1.OIDCClient{SecretName: testSecretName}, + }, + }}, + inputSecrets: []runtime.Object{&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName}, + Type: "secrets.pinniped.dev/oidc-client", + Data: testValidSecretData, + }}, + wantErr: controllerlib.ErrSyntheticRequeue.Error(), + wantLogs: []string{ + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="token endpoint URL scheme must be \"https\", not \"http\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, + `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="token endpoint URL scheme must be \"https\", not \"http\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, + }, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, + wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Status: v1alpha1.OIDCIdentityProviderStatus{ + Phase: "Error", + Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidCondition, + { + Type: "ClientCredentialsValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded client credentials", + }, + { + Type: "OIDCDiscoverySucceeded", + Status: "False", + LastTransitionTime: now, + Reason: "InvalidResponse", + Message: `token endpoint URL scheme must be "https", not "http"`, + }, + }, + }, + }}, + }, + { + name: "issuer returns no token URL", + inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Spec: v1alpha1.OIDCIdentityProviderSpec{ + Issuer: testIssuerURL + "/missing-token-url", + TLS: &v1alpha1.TLSSpec{CertificateAuthorityData: testIssuerCABase64}, + Client: v1alpha1.OIDCClient{SecretName: testSecretName}, + }, + }}, + inputSecrets: []runtime.Object{&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName}, + Type: "secrets.pinniped.dev/oidc-client", + Data: testValidSecretData, + }}, + wantErr: controllerlib.ErrSyntheticRequeue.Error(), + wantLogs: []string{ + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="token endpoint URL scheme must be \"https\", not \"\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, + `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="token endpoint URL scheme must be \"https\", not \"\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, + }, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, + wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Status: v1alpha1.OIDCIdentityProviderStatus{ + Phase: "Error", + Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidCondition, + { + Type: "ClientCredentialsValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded client credentials", + }, + { + Type: "OIDCDiscoverySucceeded", + Status: "False", + LastTransitionTime: now, + Reason: "InvalidResponse", + Message: `token endpoint URL scheme must be "https", not ""`, + }, + }, + }, + }}, + }, + { + name: "issuer returns no auth URL", + inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Spec: v1alpha1.OIDCIdentityProviderSpec{ + Issuer: testIssuerURL + "/missing-auth-url", + TLS: &v1alpha1.TLSSpec{CertificateAuthorityData: testIssuerCABase64}, + Client: v1alpha1.OIDCClient{SecretName: testSecretName}, + }, + }}, + inputSecrets: []runtime.Object{&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName}, + Type: "secrets.pinniped.dev/oidc-client", + Data: testValidSecretData, + }}, + wantErr: controllerlib.ErrSyntheticRequeue.Error(), + wantLogs: []string{ + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="authorization endpoint URL scheme must be \"https\", not \"\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, + `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="authorization endpoint URL scheme must be \"https\", not \"\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, + }, + wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, + wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Status: v1alpha1.OIDCIdentityProviderStatus{ + Phase: "Error", + Conditions: []v1alpha1.Condition{ + happyAdditionalAuthorizeParametersValidCondition, + { + Type: "ClientCredentialsValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded client credentials", + }, + { + Type: "OIDCDiscoverySucceeded", + Status: "False", + LastTransitionTime: now, + Reason: "InvalidResponse", + Message: `authorization endpoint URL scheme must be "https", not ""`, + }, + }, + }, + }}, + }, { name: "upstream with error becomes valid", inputUpstreams: []runtime.Object{&v1alpha1.OIDCIdentityProvider{ @@ -1253,6 +1530,7 @@ func newTestIssuer(t *testing.T) (string, string) { Issuer: testURL, AuthURL: "https://example.com/authorize", RevocationURL: "https://example.com/revoke", + TokenURL: "https://example.com/token", }) }) @@ -1263,6 +1541,7 @@ func newTestIssuer(t *testing.T) (string, string) { Issuer: testURL + "/valid-without-revocation", AuthURL: "https://example.com/authorize", RevocationURL: "", // none + TokenURL: "https://example.com/token", }) }) @@ -1270,8 +1549,9 @@ func newTestIssuer(t *testing.T) (string, string) { mux.HandleFunc("/invalid/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { w.Header().Set("content-type", "application/json") _ = json.NewEncoder(w).Encode(&providerJSON{ - Issuer: testURL + "/invalid", - AuthURL: "%", + Issuer: testURL + "/invalid", + AuthURL: "%", + TokenURL: "https://example.com/token", }) }) @@ -1282,6 +1562,7 @@ func newTestIssuer(t *testing.T) (string, string) { Issuer: testURL + "/invalid-revocation-url", AuthURL: "https://example.com/authorize", RevocationURL: "%", + TokenURL: "https://example.com/token", }) }) @@ -1289,18 +1570,52 @@ func newTestIssuer(t *testing.T) (string, string) { mux.HandleFunc("/insecure/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { w.Header().Set("content-type", "application/json") _ = json.NewEncoder(w).Encode(&providerJSON{ - Issuer: testURL + "/insecure", - AuthURL: "http://example.com/authorize", + Issuer: testURL + "/insecure", + AuthURL: "http://example.com/authorize", + TokenURL: "https://example.com/token", }) }) - // At "/insecure", serve an issuer that returns an insecure authorization URL (not https://). + // At "/insecure-revocation-url", serve an issuer that returns an insecure revocation URL (not https://). mux.HandleFunc("/insecure-revocation-url/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { w.Header().Set("content-type", "application/json") _ = json.NewEncoder(w).Encode(&providerJSON{ Issuer: testURL + "/insecure-revocation-url", AuthURL: "https://example.com/authorize", RevocationURL: "http://example.com/revoke", + TokenURL: "https://example.com/token", + }) + }) + + // At "/insecure-token-url", serve an issuer that returns an insecure token URL (not https://). + mux.HandleFunc("/insecure-token-url/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("content-type", "application/json") + _ = json.NewEncoder(w).Encode(&providerJSON{ + Issuer: testURL + "/insecure-token-url", + AuthURL: "https://example.com/authorize", + RevocationURL: "https://example.com/revoke", + TokenURL: "http://example.com/token", + }) + }) + + // At "/missing-token-url", serve an issuer that returns no token URL (is required by the spec unless it's an idp which only supports + // implicit flow, which we don't support). So for our purposes we need to always get a token url + mux.HandleFunc("/missing-token-url/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("content-type", "application/json") + _ = json.NewEncoder(w).Encode(&providerJSON{ + Issuer: testURL + "/missing-token-url", + AuthURL: "https://example.com/authorize", + RevocationURL: "https://example.com/revoke", + }) + }) + + // At "/missing-auth-url", serve an issuer that returns no auth URL, which is required by the spec. + mux.HandleFunc("/missing-auth-url/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("content-type", "application/json") + _ = json.NewEncoder(w).Encode(&providerJSON{ + Issuer: testURL + "/missing-auth-url", + RevocationURL: "https://example.com/revoke", + TokenURL: "https://example.com/token", }) }) @@ -1316,6 +1631,7 @@ func newTestIssuer(t *testing.T) (string, string) { Issuer: testURL + "/ends-with-slash/", AuthURL: "https://example.com/authorize", RevocationURL: "https://example.com/revoke", + TokenURL: "https://example.com/token", }) }) From b098435290dae066e076825e8766526d6f6b78e7 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Mon, 13 Dec 2021 16:40:13 -0800 Subject: [PATCH 3/8] Refactor validatetoken to handle refresh case without id token Signed-off-by: Margo Crawford --- .../mockupstreamoidcidentityprovider.go | 8 +- .../downstreamsession/downstream_session.go | 4 +- .../provider/dynamic_upstream_idp_provider.go | 9 +- internal/oidc/token/token_handler.go | 36 +- internal/oidc/token/token_handler_test.go | 90 ++++- .../testutil/oidctestutil/oidctestutil.go | 25 +- internal/upstreamoidc/upstreamoidc.go | 139 ++++---- internal/upstreamoidc/upstreamoidc_test.go | 308 +++++++++++++++--- pkg/oidcclient/login.go | 2 +- pkg/oidcclient/login_test.go | 6 +- 10 files changed, 458 insertions(+), 169 deletions(-) diff --git a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go index 046f1849..092aede5 100644 --- a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go +++ b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go @@ -230,16 +230,16 @@ func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) RevokeRefreshToken(arg0 } // ValidateToken mocks base method. -func (m *MockUpstreamOIDCIdentityProviderI) ValidateToken(arg0 context.Context, arg1 *oauth2.Token, arg2 nonce.Nonce) (*oidctypes.Token, error) { +func (m *MockUpstreamOIDCIdentityProviderI) ValidateToken(arg0 context.Context, arg1 *oauth2.Token, arg2 nonce.Nonce, arg3 bool) (*oidctypes.Token, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ValidateToken", arg0, arg1, arg2) + ret := m.ctrl.Call(m, "ValidateToken", arg0, arg1, arg2, arg3) ret0, _ := ret[0].(*oidctypes.Token) ret1, _ := ret[1].(error) return ret0, ret1 } // ValidateToken indicates an expected call of ValidateToken. -func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) ValidateToken(arg0, arg1, arg2 interface{}) *gomock.Call { +func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) ValidateToken(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ValidateToken", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).ValidateToken), arg0, arg1, arg2) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ValidateToken", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).ValidateToken), arg0, arg1, arg2, arg3) } diff --git a/internal/oidc/downstreamsession/downstream_session.go b/internal/oidc/downstreamsession/downstream_session.go index 5f0cf5f0..2aa66f5c 100644 --- a/internal/oidc/downstreamsession/downstream_session.go +++ b/internal/oidc/downstreamsession/downstream_session.go @@ -97,7 +97,7 @@ func getSubjectAndUsernameFromUpstreamIDToken( if err != nil { return "", "", err } - subject := downstreamSubjectFromUpstreamOIDC(upstreamIssuer, upstreamSubject) + subject := DownstreamSubjectFromUpstreamOIDC(upstreamIssuer, upstreamSubject) usernameClaimName := upstreamIDPConfig.GetUsernameClaim() if usernameClaimName == "" { @@ -176,7 +176,7 @@ func DownstreamLDAPSubject(uid string, ldapURL url.URL) string { return ldapURL.String() } -func downstreamSubjectFromUpstreamOIDC(upstreamIssuerAsString string, upstreamSubject string) string { +func DownstreamSubjectFromUpstreamOIDC(upstreamIssuerAsString string, upstreamSubject string) string { return fmt.Sprintf("%s?%s=%s", upstreamIssuerAsString, oidc.IDTokenSubjectClaim, url.QueryEscape(upstreamSubject)) } diff --git a/internal/oidc/provider/dynamic_upstream_idp_provider.go b/internal/oidc/provider/dynamic_upstream_idp_provider.go index 5207945b..41f9e608 100644 --- a/internal/oidc/provider/dynamic_upstream_idp_provider.go +++ b/internal/oidc/provider/dynamic_upstream_idp_provider.go @@ -74,7 +74,7 @@ type UpstreamOIDCIdentityProviderI interface { // ValidateToken will validate the ID token. It will also merge the claims from the userinfo endpoint response // into the ID token's claims, if the provider offers the userinfo endpoint. It returns the validated/updated // tokens, or an error. - ValidateToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) + ValidateToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool) (*oidctypes.Token, error) } type UpstreamLDAPIdentityProviderI interface { @@ -103,13 +103,6 @@ type StoredRefreshAttributes struct { AdditionalAttributes map[string]string } -type StoredRefreshAttributes struct { - Username string - Subject string - DN string - AuthTime time.Time -} - type DynamicUpstreamIDPProvider interface { SetOIDCIdentityProviders(oidcIDPs []UpstreamOIDCIdentityProviderI) GetOIDCIdentityProviders() []UpstreamOIDCIdentityProviderI diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index afaee0b1..2630fadb 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -16,6 +16,7 @@ import ( "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/psession" + "go.pinniped.dev/internal/upstreamoidc" ) var ( @@ -88,7 +89,7 @@ func upstreamRefresh(ctx context.Context, accessRequest fosite.AccessRequester, switch customSessionData.ProviderType { case psession.ProviderTypeOIDC: - return upstreamOIDCRefresh(ctx, customSessionData, providerCache) + return upstreamOIDCRefresh(ctx, session, providerCache) case psession.ProviderTypeLDAP: return upstreamLDAPRefresh(ctx, providerCache, session) case psession.ProviderTypeActiveDirectory: @@ -98,7 +99,8 @@ func upstreamRefresh(ctx context.Context, accessRequest fosite.AccessRequester, } } -func upstreamOIDCRefresh(ctx context.Context, s *psession.CustomSessionData, providerCache oidc.UpstreamIdentityProvidersLister) error { +func upstreamOIDCRefresh(ctx context.Context, session *psession.PinnipedSession, providerCache oidc.UpstreamIdentityProvidersLister) error { + s := session.Custom if s.OIDC == nil || s.OIDC.UpstreamRefreshToken == "" { return errorsx.WithStack(errMissingUpstreamSessionInternalError) } @@ -122,17 +124,31 @@ func upstreamOIDCRefresh(ctx context.Context, s *psession.CustomSessionData, pro // "the response body is the Token Response of Section 3.1.3.3 except that it might not contain an id_token." // https://openid.net/specs/openid-connect-core-1_0.html#RefreshTokenResponse _, hasIDTok := refreshedTokens.Extra("id_token").(string) - if hasIDTok { - // The spec is not 100% clear about whether an ID token from the refresh flow should include a nonce, and at - // least some providers do not include one, so we skip the nonce validation here (but not other validations). - _, err = p.ValidateToken(ctx, refreshedTokens, "") + + // The spec is not 100% clear about whether an ID token from the refresh flow should include a nonce, and at + // least some providers do not include one, so we skip the nonce validation here (but not other validations). + validatedTokens, err := p.ValidateToken(ctx, refreshedTokens, "", hasIDTok) + if err != nil { + return errorsx.WithStack(errUpstreamRefreshError.WithHintf( + "Upstream refresh returned an invalid ID token or UserInfo response.").WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + } + + claims := validatedTokens.IDToken.Claims + // if we have any claims at all, we better have a subject, and it better match the previous value. + // but it's possible that we don't because both returning a new refresh token on refresh and having a userinfo + // endpoint are optional. + if len(validatedTokens.IDToken.Claims) != 0 { + newSub := claims["sub"] + oldDownstreamSubject := session.Fosite.Claims.Subject + oldSub, err := upstreamoidc.ExtractUpstreamSubjectFromDownstream(oldDownstreamSubject) if err != nil { return errorsx.WithStack(errUpstreamRefreshError.WithHintf( - "Upstream refresh returned an invalid ID token.").WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + "Could not verify upstream refresh subject against previous value").WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + } + if oldSub != newSub { + return errorsx.WithStack(errUpstreamRefreshError.WithHintf( + "Subject in upstream refresh does not match previous value").WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) } - } else { - plog.Debug("upstream refresh request did not return a new ID token", - "providerName", s.ProviderName, "providerType", s.ProviderType, "providerUID", s.ProviderUID) } // Upstream refresh may or may not return a new refresh token. If we got a new refresh token, then update it in diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index 0c021de1..6fb6e2e8 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -22,6 +22,8 @@ import ( "testing" "time" + "go.pinniped.dev/pkg/oidcclient/oidctypes" + "github.com/ory/fosite" fositeoauth2 "github.com/ory/fosite/handler/oauth2" "github.com/ory/fosite/handler/openid" @@ -1046,7 +1048,13 @@ func TestRefreshGrant(t *testing.T) { { name: "happy path refresh grant with openid scope granted (id token returned)", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": "some-subject", + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, @@ -1062,7 +1070,11 @@ func TestRefreshGrant(t *testing.T) { { name: "happy path refresh grant without openid scope granted (no id token returned)", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{}, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "offline_access") }, @@ -1089,7 +1101,11 @@ func TestRefreshGrant(t *testing.T) { { name: "happy path refresh grant when the upstream refresh does not return a new ID token", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithRefreshedTokens(refreshedUpstreamTokensWithRefreshTokenWithoutIDToken()).Build()), + upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{}, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithRefreshTokenWithoutIDToken()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, @@ -1098,14 +1114,20 @@ func TestRefreshGrant(t *testing.T) { refreshRequest: refreshRequestInputs{ want: happyRefreshTokenResponseForOpenIDAndOfflineAccess( upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken), - nil, // expect ValidateToken is *not* called + refreshedUpstreamTokensWithRefreshTokenWithoutIDToken(), // expect ValidateToken is called ), }, }, { name: "happy path refresh grant when the upstream refresh does not return a new refresh token", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithRefreshedTokens(refreshedUpstreamTokensWithIDTokenWithoutRefreshToken()).Build()), + upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": "some-subject", + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDTokenWithoutRefreshToken()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, @@ -1121,7 +1143,13 @@ func TestRefreshGrant(t *testing.T) { { name: "when the refresh request adds a new scope to the list of requested scopes then it is ignored", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": "some-subject", + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, @@ -1140,7 +1168,13 @@ func TestRefreshGrant(t *testing.T) { { name: "when the refresh request removes a scope which was originally granted from the list of requested scopes then it is granted anyway", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": "some-subject", + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access pinniped:request-audience") }, @@ -1170,7 +1204,13 @@ func TestRefreshGrant(t *testing.T) { { name: "when the refresh request does not include a scope param then it gets all the same scopes as the original authorization request", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( - upstreamOIDCIdentityProviderBuilder().WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": "some-subject", + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), authcodeExchange: authcodeExchangeInputs{ customSessionData: initialUpstreamOIDCCustomSessionData(), modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, @@ -1545,7 +1585,39 @@ func TestRefreshGrant(t *testing.T) { wantErrorResponseBody: here.Doc(` { "error": "error", - "error_description": "Error during upstream refresh. Upstream refresh returned an invalid ID token." + "error_description": "Error during upstream refresh. Upstream refresh returned an invalid ID token or UserInfo response." + } + `), + }, + }, + }, + { + name: "when the upstream refresh returns an ID token with a different subject than the original", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder(). + WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()). + // This is the current format of the errors returned by the production code version of ValidateToken, see ValidateToken in upstreamoidc.go + WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "sub": "something-different", + }, + }, + }). + Build()), + authcodeExchange: authcodeExchangeInputs{ + customSessionData: initialUpstreamOIDCCustomSessionData(), + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), + wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()), + wantStatus: http.StatusUnauthorized, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "Error during upstream refresh. Subject in upstream refresh does not match previous value" } `), }, diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index 88e54a73..a4f0b31e 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -83,6 +83,12 @@ type ValidateTokenArgs struct { ExpectedIDTokenNonce nonce.Nonce } +type ValidateRefreshArgs struct { + Ctx context.Context + Tok *oauth2.Token + StoredAttributes provider.StoredRefreshAttributes +} + type TestUpstreamLDAPIdentityProvider struct { Name string ResourceUID types.UID @@ -170,6 +176,8 @@ type TestUpstreamOIDCIdentityProvider struct { ValidateTokenFunc func(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) + ValidateRefreshFunc func(ctx context.Context, tok *oauth2.Token, storedAttributes provider.StoredRefreshAttributes) error + exchangeAuthcodeAndValidateTokensCallCount int exchangeAuthcodeAndValidateTokensArgs []*ExchangeAuthcodeAndValidateTokenArgs passwordCredentialsGrantAndValidateTokensCallCount int @@ -180,6 +188,8 @@ type TestUpstreamOIDCIdentityProvider struct { revokeRefreshTokenArgs []*RevokeRefreshTokenArgs validateTokenCallCount int validateTokenArgs []*ValidateTokenArgs + validateRefreshCallCount int + validateRefreshArgs []*ValidateRefreshArgs } var _ provider.UpstreamOIDCIdentityProviderI = &TestUpstreamOIDCIdentityProvider{} @@ -278,6 +288,19 @@ func (u *TestUpstreamOIDCIdentityProvider) PerformRefresh(ctx context.Context, r return u.PerformRefreshFunc(ctx, refreshToken) } +func (u *TestUpstreamOIDCIdentityProvider) ValidateRefresh(ctx context.Context, tok *oauth2.Token, storedAttributes provider.StoredRefreshAttributes) error { + if u.validateRefreshArgs == nil { + u.validateRefreshArgs = make([]*ValidateRefreshArgs, 0) + } + u.validateRefreshCallCount++ + u.validateRefreshArgs = append(u.validateRefreshArgs, &ValidateRefreshArgs{ + Ctx: ctx, + Tok: tok, + StoredAttributes: storedAttributes, + }) + return u.ValidateRefreshFunc(ctx, tok, storedAttributes) +} + func (u *TestUpstreamOIDCIdentityProvider) RevokeRefreshToken(ctx context.Context, refreshToken string) error { if u.revokeRefreshTokenArgs == nil { u.revokeRefreshTokenArgs = make([]*RevokeRefreshTokenArgs, 0) @@ -312,7 +335,7 @@ func (u *TestUpstreamOIDCIdentityProvider) RevokeRefreshTokenArgs(call int) *Rev return u.revokeRefreshTokenArgs[call] } -func (u *TestUpstreamOIDCIdentityProvider) ValidateToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) { +func (u *TestUpstreamOIDCIdentityProvider) ValidateToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool) (*oidctypes.Token, error) { if u.validateTokenArgs == nil { u.validateTokenArgs = make([]*ValidateTokenArgs, 0) } diff --git a/internal/upstreamoidc/upstreamoidc.go b/internal/upstreamoidc/upstreamoidc.go index abc2fd8e..0ac4dbd5 100644 --- a/internal/upstreamoidc/upstreamoidc.go +++ b/internal/upstreamoidc/upstreamoidc.go @@ -13,6 +13,7 @@ import ( "net/http" "net/url" "strings" + "time" coreosoidc "github.com/coreos/go-oidc/v3/oidc" "golang.org/x/oauth2" @@ -113,7 +114,7 @@ func (p *ProviderConfig) PasswordCredentialsGrantAndValidateTokens(ctx context.C // There is no nonce to validate for a resource owner password credentials grant because it skips using // the authorize endpoint and goes straight to the token endpoint. const skipNonceValidation nonce.Nonce = "" - return p.ValidateToken(ctx, tok, skipNonceValidation) + return p.ValidateToken(ctx, tok, skipNonceValidation, true) } func (p *ProviderConfig) ExchangeAuthcodeAndValidateTokens(ctx context.Context, authcode string, pkceCodeVerifier pkce.Code, expectedIDTokenNonce nonce.Nonce, redirectURI string) (*oidctypes.Token, error) { @@ -127,10 +128,9 @@ func (p *ProviderConfig) ExchangeAuthcodeAndValidateTokens(ctx context.Context, return nil, err } - return p.ValidateToken(ctx, tok, expectedIDTokenNonce) + return p.ValidateToken(ctx, tok, expectedIDTokenNonce, true) } -// TODO this is reused between the client and the supervisor... don't change it. func (p *ProviderConfig) PerformRefresh(ctx context.Context, refreshToken string) (*oauth2.Token, error) { // Use the provided HTTP client to benefit from its CA, proxy, and other settings. httpClientContext := coreosoidc.ClientContext(ctx, p.Client) @@ -238,88 +238,54 @@ func (p *ProviderConfig) tryRevokeRefreshToken( } } -func (p *ProviderConfig) ValidateRefresh(ctx context.Context, tok *oauth2.Token, storedAttributes provider.StoredRefreshAttributes) error { - idTok, hasIDTok := tok.Extra("id_token").(string) - var validatedClaims = make(map[string]interface{}) - if hasIDTok { - coreosConfig := &coreosoidc.Config{ClientID: p.GetClientID()} - coreosClientContext := coreosoidc.ClientContext(ctx, p.Client) - verifier := p.Provider.Verifier(coreosConfig) - validated, err := verifier.Verify(coreosClientContext, idTok) - if err != nil { - return httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err) - } - if err := validated.Claims(&validatedClaims); err != nil { - return httperr.Wrap(http.StatusInternalServerError, "could not unmarshal id token claims", err) - } - maybeLogClaims("claims from ID token", p.Name, validatedClaims) - } - - originalUpstreamSubject, err := extractUpstreamSubjectFromDownstream(storedAttributes.Subject) - if err != nil { - return httperr.Wrap(http.StatusInternalServerError, "could not parse stored subject", err) - } - - // it's okay to not have an id token. It's okay to have an id token with a subject. - // but if we have an id token without a subject that's a problem. - idTokenSubject, _ := validatedClaims[oidc.IDTokenSubjectClaim].(string) - switch { - case len(idTokenSubject) > 0: - // TODO url escape - if url.QueryEscape(idTokenSubject) != originalUpstreamSubject { - return httperr.Newf(http.StatusInternalServerError, "subject from id token did not match previous stored value. New subject: %s. Old subject: %s", idTokenSubject, originalUpstreamSubject) - } - case len(validatedClaims) == 0: - validatedClaims[oidc.IDTokenSubjectClaim] = originalUpstreamSubject - default: - return httperr.New(http.StatusInternalServerError, "id token did not have a subject") - } - - if err := p.maybeFetchUserInfoAndMergeClaims(ctx, tok, validatedClaims); err != nil { - return err - } - - return nil -} - -func extractUpstreamSubjectFromDownstream(downstreamSubject string) (string, error) { +func ExtractUpstreamSubjectFromDownstream(downstreamSubject string) (string, error) { if !strings.Contains(downstreamSubject, "?sub=") { return "", errors.New("downstream subject did not contain original upstream subject") } - return strings.Split(downstreamSubject, "?sub=")[1], nil + return strings.SplitN(downstreamSubject, "?sub=", 2)[1], nil // TODO test for ?sub= occurring twice (imagine if you ran the supervisor with another supervisor as the upstream idp...) } // ValidateToken will validate the ID token. It will also merge the claims from the userinfo endpoint response, // if the provider offers the userinfo endpoint. -func (p *ProviderConfig) ValidateToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) { +// TODO check: +// - whether the userinfo response must exist (maybe just based on whether there is a refresh token??? +// - -> for the next story: userinfo has to exist but only if there isn't a refresh token. +func (p *ProviderConfig) ValidateToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool) (*oidctypes.Token, error) { + var validatedClaims = make(map[string]interface{}) + idTok, hasIDTok := tok.Extra("id_token").(string) - if !hasIDTok { - return nil, httperr.New(http.StatusBadRequest, "received response missing ID token") - } - validated, err := p.Provider.Verifier(&coreosoidc.Config{ClientID: p.GetClientID()}).Verify(coreosoidc.ClientContext(ctx, p.Client), idTok) - if err != nil { - return nil, httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err) - } - if validated.AccessTokenHash != "" { - if err := validated.VerifyAccessToken(tok.AccessToken); err != nil { + var idTokenExpiry time.Time + // if we require the id token, make sure we have it. + // also, if it exists but wasn't required, still make sure it passes these checks. + // nolint:nestif + if hasIDTok || requireIDToken { + if !hasIDTok { + return nil, httperr.New(http.StatusBadRequest, "received response missing ID token") + } + validated, err := p.Provider.Verifier(&coreosoidc.Config{ClientID: p.GetClientID()}).Verify(coreosoidc.ClientContext(ctx, p.Client), idTok) + if err != nil { return nil, httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err) } - } - if expectedIDTokenNonce != "" { - if err := expectedIDTokenNonce.Validate(validated); err != nil { - return nil, httperr.Wrap(http.StatusBadRequest, "received ID token with invalid nonce", err) + if validated.AccessTokenHash != "" { + if err := validated.VerifyAccessToken(tok.AccessToken); err != nil { + return nil, httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err) + } } + if expectedIDTokenNonce != "" { + if err := expectedIDTokenNonce.Validate(validated); err != nil { + return nil, httperr.Wrap(http.StatusBadRequest, "received ID token with invalid nonce", err) + } + } + if err := validated.Claims(&validatedClaims); err != nil { + return nil, httperr.Wrap(http.StatusInternalServerError, "could not unmarshal id token claims", err) + } + maybeLogClaims("claims from ID token", p.Name, validatedClaims) + idTokenExpiry = validated.Expiry } - - var validatedClaims map[string]interface{} - if err := validated.Claims(&validatedClaims); err != nil { - return nil, httperr.Wrap(http.StatusInternalServerError, "could not unmarshal id token claims", err) - } - maybeLogClaims("claims from ID token", p.Name, validatedClaims) - idTokenSubject, _ := validatedClaims[oidc.IDTokenSubjectClaim].(string) - if len(idTokenSubject) > 0 { - if err := p.maybeFetchUserInfoAndMergeClaims(ctx, tok, validatedClaims); err != nil { + + if len(idTokenSubject) > 0 || !requireIDToken { + if err := p.maybeFetchUserInfoAndMergeClaims(ctx, tok, validatedClaims, requireIDToken); err != nil { return nil, httperr.Wrap(http.StatusInternalServerError, "could not fetch user info claims", err) } } @@ -335,18 +301,13 @@ func (p *ProviderConfig) ValidateToken(ctx context.Context, tok *oauth2.Token, e }, IDToken: &oidctypes.IDToken{ Token: idTok, - Expiry: metav1.NewTime(validated.Expiry), + Expiry: metav1.NewTime(idTokenExpiry), Claims: validatedClaims, }, }, nil } -func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, tok *oauth2.Token, claims map[string]interface{}) error { - // TODO separate this. - // extract: fetching userinfo - // - validate some userinfo? subject stuff: for refresh subjects must match but also match stored subject - // - extract: merging claims - // - deciding when to do each of those things +func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, tok *oauth2.Token, claims map[string]interface{}, requireIDToken bool) error { idTokenSubject, _ := claims[oidc.IDTokenSubjectClaim].(string) userInfo, err := p.fetchUserInfo(ctx, tok) @@ -357,8 +318,9 @@ func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, t return nil } - // TODO if there is no idTokenSubject, defer to checking the stored claims. // The sub (subject) Claim MUST always be returned in the UserInfo Response. + // However there may not be an id token. If there is an ID token, we must + // check it against the userinfo's subject. // // NOTE: Due to the possibility of token substitution attacks (see Section 16.11), the UserInfo Response is not // guaranteed to be about the End-User identified by the sub (subject) element of the ID Token. The sub Claim in @@ -366,14 +328,29 @@ func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, t // the UserInfo Response values MUST NOT be used. // // http://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse - if (len(idTokenSubject) > 0) && (len(userInfo.Subject) == 0 || userInfo.Subject != idTokenSubject) { + checkIDToken := requireIDToken || len(idTokenSubject) > 0 + if checkIDToken && (len(userInfo.Subject) == 0 || userInfo.Subject != idTokenSubject) { return httperr.Newf(http.StatusUnprocessableEntity, "userinfo 'sub' claim (%s) did not match id_token 'sub' claim (%s)", userInfo.Subject, idTokenSubject) } + if !checkIDToken { + claims["sub"] = userInfo.Subject // do this so other validations can check this subject later + } + + idTokenIssuer := claims["iss"] + // merge existing claims with user info claims if err := userInfo.Claims(&claims); err != nil { return httperr.Wrap(http.StatusInternalServerError, "could not unmarshal user info claims", err) } + // The OIDC spec for user info response does not make any guarantees about the iss claim's existence or validity: + // "If signed, the UserInfo Response SHOULD contain the Claims iss (issuer) and aud (audience) as members. The iss value SHOULD be the OP's Issuer Identifier URL." + // See https://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse + // So we just ignore it and use it the version from the id token, which has stronger guarantees. + delete(claims, "iss") + if idTokenIssuer != nil { + claims["iss"] = idTokenIssuer + } maybeLogClaims("claims from ID token and userinfo", p.Name, claims) diff --git a/internal/upstreamoidc/upstreamoidc_test.go b/internal/upstreamoidc/upstreamoidc_test.go index 30f1318a..949a56d7 100644 --- a/internal/upstreamoidc/upstreamoidc_test.go +++ b/internal/upstreamoidc/upstreamoidc_test.go @@ -16,8 +16,6 @@ import ( "time" "unsafe" - "go.pinniped.dev/internal/oidc/provider" - "github.com/coreos/go-oidc/v3/oidc" "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" @@ -591,82 +589,292 @@ func TestProviderConfig(t *testing.T) { } }) - t.Run("ValidateRefresh", func(t *testing.T) { + t.Run("ValidateToken", func(t *testing.T) { + expiryTime := time.Now().Add(42 * time.Second) testTokenWithoutIDToken := &oauth2.Token{ AccessToken: "test-access-token", // the library sets the original refresh token into the result, even though the server did not return that RefreshToken: "test-initial-refresh-token", TokenType: "test-token-type", - Expiry: time.Now().Add(42 * time.Second), + Expiry: expiryTime, } + // generated from jwt.io + // sub: some-subject + // iss: some-issuer + // nonce: some-nonce + goodIDToken := "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJub25jZSI6InNvbWUtbm9uY2UiLCJpc3MiOiJzb21lLWlzc3VlciJ9.eGvzOihLUqzn3M4k6fHsToedgy7Fu89_Xu_u4mwMgRlIyRWZqmEMV76RVLnZd9Ihm9j_VpvrpirIkaj4JM9eRNfLX1n328cmBivBwnTKAzHuTm17dUKO5EvdTmQzmwnN0WZ8nWk4GfR7RzcvE1V8G9tIiWD8FkO3Dr-NR_zTun3N37onAazVLCmF0SDtATDfUH1ETqviHEp8xGx5HD5mv5T3HEjOuer5gxTEnfncef0LurBH3po-C0tXHKu74PD8x88CMJ1DLsRdCalnctwa850slKPkBSTP-ssh0JVg7cdMXoosVpwiXtKYaBkrhu8VS018aFP-cBbW0mYwsHmt3g" //nolint:gosec tests := []struct { name string - token *oauth2.Token - storedAttributes provider.StoredRefreshAttributes + tok *oauth2.Token + nonce nonce.Nonce + requireIDToken bool userInfo *oidc.UserInfo rawClaims []byte userInfoErr error wantErr string + wantMergedTokens *oidctypes.Token }{ { - name: "id, access and refresh token returned from refresh", - token: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJuYW1lIjoiUGlubnkgVGhlU2VhbCIsImlzcyI6InNvbWUtaXNzdWVyIiwiaWF0IjoxNTE2MjM5MDIyfQ.qVMIsmrMLiMW7kJUNO4RpiBnkd6MYKdXXMMb6UWVXL0zXkalb1tP2DO3Yp73ZfWqCNdNEa1qqSJIR9F46cnAp04pZmOvVqTWCmaXaxvklRb_QzdlKi-3QNg7wEJ50a11vhUTJxLYrkH9pJpPILzd8Kx0yp43iEJbBG3nFVZFOSQOh7wCGUF4QkNLRMT9buq8DiypKPmjksVmObkF3AbvQ1IvaA_UPBmFvL-pun-TjUBR37xT7JI64wbczJJclThaqTcK6bQcbiBG4jonckYIIryoL8UUBSbxQGGz7lQK5_77Q4PTXAE3c5hrg-c6RK52YNBm-B5Gx7vAeBavrQJaPQ"}), - storedAttributes: provider.StoredRefreshAttributes{Subject: "some-issuer?sub=some-subject"}, - userInfo: forceUserInfoWithClaims("some-subject", `{"foo": "bar"}`), - rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + name: "token with id, access and refresh tokens, valid nonce, and no userinfo", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}), + nonce: "some-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: goodIDToken, + Claims: map[string]interface{}{ + "iss": "some-issuer", + "nonce": "some-nonce", + "sub": "some-subject", + }, + }, + }, }, { - name: "id token exists but has no subject", - token: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJuYW1lIjoiUGlubnkgVGhlU2VhbCIsImlhdCI6MTUxNjIzOTAyMn0.FmkkY7MCnhD98F5Aupq_YdW54pH90O60O9SwjxWHZb6_k6LoJcjZEMyxba1Fg5tlGLOSqH7BffZOfMUZ5YqVeA7ZIZ9fhAiudGdJqKz5nI394xzR-EejGqBI0fTAuxa_0VsLSC5mV0hTctK01kvemPwMLhIUn_HHr0-khugPFZH8zlYxAFmBpe9YxWK5w1JcTWuSMjRn6_b1RDSgxpff-LcmvVz6akcnlb2j8Od7S9JinsSqKjb7zBjQmcVGT6BGdN43tsBJMEOzil6xpvsH_KIToWlyqmvCQFp_udcKWuUNNLdypGW4eMXvPF5GINMDhki0GZDoKLkPnHVk_mZFXw"}), - storedAttributes: provider.StoredRefreshAttributes{Subject: "some-issuer?sub=some-subject"}, - userInfo: &oidc.UserInfo{Subject: "some-subject"}, - rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - wantErr: "id token did not have a subject", + name: "id token not required but is provided", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}), + nonce: "some-nonce", + requireIDToken: false, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: goodIDToken, + Claims: map[string]interface{}{ + "iss": "some-issuer", + "nonce": "some-nonce", + "sub": "some-subject", + "name": "Pinny TheSeal", + }, + }, + }, }, { - name: "id token doesn't exist but userinfo does", - token: testTokenWithoutIDToken, - storedAttributes: provider.StoredRefreshAttributes{Subject: "some-issuer?sub=some-subject"}, - userInfo: forceUserInfoWithClaims("some-subject", `{"foo": "bar"}`), - rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + name: "token with id, access and refresh tokens, valid nonce, and userinfo with a value that doesn't exist in the id token", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}), + nonce: "some-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: goodIDToken, + Claims: map[string]interface{}{ + "iss": "some-issuer", + "nonce": "some-nonce", + "sub": "some-subject", + "name": "Pinny TheSeal", + }, + }, + }, }, { - name: "subject in id token doesn't match stored subject", - token: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJuYW1lIjoiUGlubnkgVGhlU2VhbCIsImlzcyI6InNvbWUtaXNzdWVyIiwiaWF0IjoxNTE2MjM5MDIyfQ.qVMIsmrMLiMW7kJUNO4RpiBnkd6MYKdXXMMb6UWVXL0zXkalb1tP2DO3Yp73ZfWqCNdNEa1qqSJIR9F46cnAp04pZmOvVqTWCmaXaxvklRb_QzdlKi-3QNg7wEJ50a11vhUTJxLYrkH9pJpPILzd8Kx0yp43iEJbBG3nFVZFOSQOh7wCGUF4QkNLRMT9buq8DiypKPmjksVmObkF3AbvQ1IvaA_UPBmFvL-pun-TjUBR37xT7JI64wbczJJclThaqTcK6bQcbiBG4jonckYIIryoL8UUBSbxQGGz7lQK5_77Q4PTXAE3c5hrg-c6RK52YNBm-B5Gx7vAeBavrQJaPQ"}), - storedAttributes: provider.StoredRefreshAttributes{Subject: "some-issuer?sub=some-other-subject"}, - wantErr: "subject from id token did not match previous stored value. New subject: some-subject. Old subject: some-other-subject", + name: "claims from userinfo override id token claims", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJuYW1lIjoiSm9obiBEb2UiLCJpc3MiOiJzb21lLWlzc3VlciIsIm5vbmNlIjoic29tZS1ub25jZSJ9.sBWi3_4cfGwrmMFZWkCghw4uvCnHN35h9xNX1gkwOtj6Oz_yKqpj7wfO4AqeWsRyrDGnkmIZbVuhAAJqPSi4GlNzN4NU8zh53PGDUpFlpDI1dvqDjIRb9iIEJpRIj34--Sz41H0ooxviIzvUdZFvQlaSzLOqgjR3ddHe2urhbtUuz_DsabP84AWo2DSg0y3ull6DRvk_DvzC6HNN8JwVi08fFvvV9BVq8kjdVeob7gajJkuGSTjsxNZGs5rbBuxBx0MZTQ8boR1fDNdG70GoIb4SsCoBSs7pZxtmGZPHInteY1SilHDDDmpQuE-LvSmvvPN_Cyk1d3eS-IR7hBbCAA"}), + nonce: "some-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJuYW1lIjoiSm9obiBEb2UiLCJpc3MiOiJzb21lLWlzc3VlciIsIm5vbmNlIjoic29tZS1ub25jZSJ9.sBWi3_4cfGwrmMFZWkCghw4uvCnHN35h9xNX1gkwOtj6Oz_yKqpj7wfO4AqeWsRyrDGnkmIZbVuhAAJqPSi4GlNzN4NU8zh53PGDUpFlpDI1dvqDjIRb9iIEJpRIj34--Sz41H0ooxviIzvUdZFvQlaSzLOqgjR3ddHe2urhbtUuz_DsabP84AWo2DSg0y3ull6DRvk_DvzC6HNN8JwVi08fFvvV9BVq8kjdVeob7gajJkuGSTjsxNZGs5rbBuxBx0MZTQ8boR1fDNdG70GoIb4SsCoBSs7pZxtmGZPHInteY1SilHDDDmpQuE-LvSmvvPN_Cyk1d3eS-IR7hBbCAA", + Claims: map[string]interface{}{ + "iss": "some-issuer", // takes the issuer from the ID token, since the userinfo one is unreliable. + "nonce": "some-nonce", + "sub": "some-subject", + "name": "Pinny TheSeal", + }, + }, + }, }, { - name: "malformed stored subject", - token: testTokenWithoutIDToken, - storedAttributes: provider.StoredRefreshAttributes{Subject: "wrong-format"}, - wantErr: "could not parse stored subject: downstream subject did not contain original upstream subject", + name: "token with id, access and refresh tokens and valid nonce, but userinfo has a different issuer", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}), + nonce: "some-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "iss": "some-other-issuer"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: goodIDToken, + Claims: map[string]interface{}{ + "iss": "some-issuer", // takes the issuer from the ID token, since the userinfo one is unreliable. + "nonce": "some-nonce", + "sub": "some-subject", + "name": "Pinny TheSeal", + }, + }, + }, }, { - name: "subject in user info doesn't match stored subject", - token: testTokenWithoutIDToken, - storedAttributes: provider.StoredRefreshAttributes{Subject: "some-issuer?sub=some-other-subject"}, - userInfo: forceUserInfoWithClaims("some-subject", `{"foo": "bar"}`), - rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - wantErr: "userinfo 'sub' claim (some-subject) did not match id_token 'sub' claim (some-other-subject)", // TODO is this a good enough error message?... + name: "token with no id token but valid userinfo", + tok: testTokenWithoutIDToken, + nonce: "", + requireIDToken: false, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "iss": "some-other-issuer"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: "", + Claims: map[string]interface{}{ + "sub": "some-subject", + "name": "Pinny TheSeal", + }, + }, + }, }, { - name: "subject in id token doesn't match userinfo", - token: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJuYW1lIjoiUGlubnkgVGhlU2VhbCIsImlhdCI6MTUxNjIzOTAyMn0.P_97A2ONBt-YV0JHM3DDugNhG-codkK8fLU4EOdlGHcVBmWVbbW1g7-U0jjB9gcvUt24TS5z0TxEnKNUG8LiqS-8FbHGjv8DN1gAz4huBxNqmMYwzGsFNKDBcUMpv9DNFT-oIrxSf3pPYFlg7N0YKzbMPRwVaL1iQmzeQDCQieV3mDCWXpxpdLa1Mfj_NRsEe4027xOZYJn14pn83h92rG0VcoV2e39LuuEir-tRtDniY4WFIMPudNJdC0NwoMoRIQuHStCIN5AkbHPuhGQivqoObWX6RgNU18qb6CcCi86WhG45uHO77gfz5TbGsyVhr5LRBEZP5HPqsq5D6853zQ"}), - storedAttributes: provider.StoredRefreshAttributes{Subject: "some-issuer?sub=some-subject"}, - userInfo: &oidc.UserInfo{Subject: "some-other-subject"}, - rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - wantErr: "userinfo 'sub' claim (some-other-subject) did not match id_token 'sub' claim (some-subject)", + name: "token with neither id token nor userinfo", + tok: testTokenWithoutIDToken, + nonce: "", + requireIDToken: false, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{}, + }, + }, }, { - name: "id token with format that isn't a jwt returned from refresh", - token: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "not-jwt-format"}), - userInfo: &oidc.UserInfo{Subject: "test-user-2"}, - rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - wantErr: "received invalid ID token: oidc: malformed jwt: square/go-jose: compact JWS format must have three parts", + name: "token with id, access and refresh tokens, valid nonce, and userinfo subject that doesn't match", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}), + nonce: "some-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + wantErr: "could not fetch user info claims: userinfo 'sub' claim (some-other-subject) did not match id_token 'sub' claim (some-subject)", + }, + { + name: "id token not required but is provided, and subjects don't match", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}), + nonce: "some-nonce", + requireIDToken: false, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + wantErr: "could not fetch user info claims: userinfo 'sub' claim (some-other-subject) did not match id_token 'sub' claim (some-subject)", + }, + { + name: "invalid id token", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "not-an-id-token"}), + nonce: "some-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + wantErr: "received invalid ID token: oidc: malformed jwt: square/go-jose: compact JWS format must have three parts", + }, + { + name: "invalid nonce", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}), + nonce: "some-other-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + wantErr: "received ID token with invalid nonce: invalid nonce (expected \"some-other-nonce\", got \"some-nonce\")", + }, + { + name: "expected to have id token, but doesn't", + tok: testTokenWithoutIDToken, + nonce: "some-other-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + wantErr: "received response missing ID token", + }, + { + name: "mismatched access token hash", + tok: testTokenWithoutIDToken, + nonce: "some-other-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + wantErr: "received response missing ID token", + }, + { + name: "id token missing subject, skip userinfo check", + tok: testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJuYW1lIjoiSm9obiBEb2UiLCJpc3MiOiJzb21lLWlzc3VlciIsIm5vbmNlIjoic29tZS1ub25jZSJ9.aIhrhikAnQ4Mb1g6RAT08qqflT2LLLi2yj4F2S4zud8nYad4tfEd2ITVJ4Njdjf70ubqyzZ6XxojtC4OqaWbDaQOcd95sd3PW58SYrf4NMvEStFkcMG0HMhJEZLVGnuJQstuq3G9h5Z5bFCkx4mFNo5ho_isBWyHpk-uF14duXXlIDB10SnyZ9dRbcmu-3mMOq0g4oCUPEDiHWkv-Rf70Mk0harL2xvcpxlSMLK4glDfiiki5gl6IReIo4rTVosXAqv3JmjLDeVLtJQRG6F8YcIlDCIfUEUfk0GeYacBVjoDIO570ywVJy1LGvyUuvgXNQUjq2JgzCfb8HWGp7iJdQ"}), + nonce: "some-nonce", + requireIDToken: true, + rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + wantMergedTokens: &oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Type: "test-token-type", + Expiry: metav1.NewTime(expiryTime), + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-initial-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJuYW1lIjoiSm9obiBEb2UiLCJpc3MiOiJzb21lLWlzc3VlciIsIm5vbmNlIjoic29tZS1ub25jZSJ9.aIhrhikAnQ4Mb1g6RAT08qqflT2LLLi2yj4F2S4zud8nYad4tfEd2ITVJ4Njdjf70ubqyzZ6XxojtC4OqaWbDaQOcd95sd3PW58SYrf4NMvEStFkcMG0HMhJEZLVGnuJQstuq3G9h5Z5bFCkx4mFNo5ho_isBWyHpk-uF14duXXlIDB10SnyZ9dRbcmu-3mMOq0g4oCUPEDiHWkv-Rf70Mk0harL2xvcpxlSMLK4glDfiiki5gl6IReIo4rTVosXAqv3JmjLDeVLtJQRG6F8YcIlDCIfUEUfk0GeYacBVjoDIO570ywVJy1LGvyUuvgXNQUjq2JgzCfb8HWGp7iJdQ", + Claims: map[string]interface{}{ + "iss": "some-issuer", + "name": "John Doe", + "nonce": "some-nonce", + }, + }, + }, }, } + for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { @@ -690,13 +898,13 @@ func TestProviderConfig(t *testing.T) { userInfoErr: tt.userInfoErr, }, } - - err := p.ValidateRefresh(context.Background(), tt.token, tt.storedAttributes) + gotTok, err := p.ValidateToken(context.Background(), tt.tok, tt.nonce, tt.requireIDToken) if tt.wantErr != "" { require.Error(t, err) - require.Equal(t, err.Error(), tt.wantErr) + require.Equal(t, tt.wantErr, err.Error()) } else { require.NoError(t, err) + require.Equal(t, tt.wantMergedTokens, gotTok) } }) } diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index 470ed3dd..941693e2 100644 --- a/pkg/oidcclient/login.go +++ b/pkg/oidcclient/login.go @@ -822,7 +822,7 @@ func (h *handlerState) handleRefresh(ctx context.Context, refreshToken *oidctype // The spec is not 100% clear about whether an ID token from the refresh flow should include a nonce, and at least // some providers do not include one, so we skip the nonce validation here (but not other validations). - return upstreamOIDCIdentityProvider.ValidateToken(ctx, refreshed, "") + return upstreamOIDCIdentityProvider.ValidateToken(ctx, refreshed, "", true) } func (h *handlerState) handleAuthCodeCallback(w http.ResponseWriter, r *http.Request) (err error) { diff --git a/pkg/oidcclient/login_test.go b/pkg/oidcclient/login_test.go index 2bf4620c..e55f8e03 100644 --- a/pkg/oidcclient/login_test.go +++ b/pkg/oidcclient/login_test.go @@ -406,7 +406,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo h.getProvider = func(config *oauth2.Config, provider *oidc.Provider, client *http.Client) provider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). - ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce("")). + ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true). Return(&testToken, nil) mock.EXPECT(). PerformRefresh(gomock.Any(), testToken.RefreshToken.Token). @@ -453,7 +453,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo h.getProvider = func(config *oauth2.Config, provider *oidc.Provider, client *http.Client) provider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). - ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce("")). + ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true). Return(nil, fmt.Errorf("some validation error")) mock.EXPECT(). PerformRefresh(gomock.Any(), "test-refresh-token-returning-invalid-id-token"). @@ -1648,7 +1648,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo h.getProvider = func(config *oauth2.Config, provider *oidc.Provider, client *http.Client) provider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). - ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce("")). + ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true). Return(&testToken, nil) mock.EXPECT(). PerformRefresh(gomock.Any(), testToken.RefreshToken.Token). From 0cd086cf9cac6c696abaa3d92cda071cb86544a5 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Tue, 14 Dec 2021 11:59:52 -0800 Subject: [PATCH 4/8] Check username claim is unchanged for oidc. Also add integration tests for claims changing. --- internal/oidc/token/token_handler.go | 18 ++++- internal/oidc/token/token_handler_test.go | 89 +++++++++++++++++++++- internal/upstreamoidc/upstreamoidc.go | 2 +- internal/upstreamoidc/upstreamoidc_test.go | 39 ++++++++++ test/integration/supervisor_login_test.go | 12 +-- 5 files changed, 145 insertions(+), 15 deletions(-) diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index 2630fadb..9e27be10 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -6,6 +6,7 @@ package token import ( "context" + "errors" "net/http" "github.com/ory/fosite" @@ -142,12 +143,23 @@ func upstreamOIDCRefresh(ctx context.Context, session *psession.PinnipedSession, oldDownstreamSubject := session.Fosite.Claims.Subject oldSub, err := upstreamoidc.ExtractUpstreamSubjectFromDownstream(oldDownstreamSubject) if err != nil { - return errorsx.WithStack(errUpstreamRefreshError.WithHintf( - "Could not verify upstream refresh subject against previous value").WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + return errorsx.WithStack(errUpstreamRefreshError.WithHintf("Upstream refresh failed."). + WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) } if oldSub != newSub { return errorsx.WithStack(errUpstreamRefreshError.WithHintf( - "Subject in upstream refresh does not match previous value").WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + "Upstream refresh failed.").WithWrap(errors.New("subject in upstream refresh does not match previous value")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + } + usernameClaim := p.GetUsernameClaim() + newUsername := claims[usernameClaim] + // its possible this won't be returned. + // but if it is, verify that it hasn't changed. + if newUsername != nil { + oldUsername := session.Fosite.Claims.Extra["username"] + if oldUsername != newUsername { + return errorsx.WithStack(errUpstreamRefreshError.WithHintf( + "Upstream refresh failed.").WithWrap(errors.New("username in upstream refresh does not match previous value")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + } } } diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index 6fb6e2e8..eb81a280 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -22,8 +22,6 @@ import ( "testing" "time" - "go.pinniped.dev/pkg/oidcclient/oidctypes" - "github.com/ory/fosite" fositeoauth2 "github.com/ory/fosite/handler/oauth2" "github.com/ory/fosite/handler/openid" @@ -54,6 +52,7 @@ import ( "go.pinniped.dev/internal/psession" "go.pinniped.dev/internal/testutil" "go.pinniped.dev/internal/testutil/oidctestutil" + "go.pinniped.dev/pkg/oidcclient/oidctypes" ) const ( @@ -1067,6 +1066,30 @@ func TestRefreshGrant(t *testing.T) { ), }, }, + { + name: "refresh grant with unchanged username claim", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( + upstreamOIDCIdentityProviderBuilder().WithUsernameClaim("username-claim").WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "some-claim": "some-value", + "sub": "some-subject", + "username-claim": goodUsername, + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + authcodeExchange: authcodeExchangeInputs{ + customSessionData: initialUpstreamOIDCCustomSessionData(), + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + }, + refreshRequest: refreshRequestInputs{ + want: happyRefreshTokenResponseForOpenIDAndOfflineAccess( + upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken), + refreshedUpstreamTokensWithIDAndRefreshTokens(), + ), + }, + }, { name: "happy path refresh grant without openid scope granted (no id token returned)", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( @@ -1617,7 +1640,67 @@ func TestRefreshGrant(t *testing.T) { wantErrorResponseBody: here.Doc(` { "error": "error", - "error_description": "Error during upstream refresh. Subject in upstream refresh does not match previous value" + "error_description": "Error during upstream refresh. Upstream refresh failed." + } + `), + }, + }, + }, + { + name: "refresh grant with claims but not the subject claim", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( + upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "some-claim": "some-value", + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + authcodeExchange: authcodeExchangeInputs{ + customSessionData: initialUpstreamOIDCCustomSessionData(), + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), + wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()), + wantStatus: http.StatusUnauthorized, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "Error during upstream refresh. Upstream refresh failed." + } + `), + }, + }, + }, + { + name: "refresh grant with changed username claim", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( + upstreamOIDCIdentityProviderBuilder().WithUsernameClaim("username-claim").WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "some-claim": "some-value", + "sub": "some-subject", + "username-claim": "some-changed-username", + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + authcodeExchange: authcodeExchangeInputs{ + customSessionData: initialUpstreamOIDCCustomSessionData(), + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), + wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()), + wantStatus: http.StatusUnauthorized, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "Error during upstream refresh. Upstream refresh failed." } `), }, diff --git a/internal/upstreamoidc/upstreamoidc.go b/internal/upstreamoidc/upstreamoidc.go index 0ac4dbd5..1c7b7d8c 100644 --- a/internal/upstreamoidc/upstreamoidc.go +++ b/internal/upstreamoidc/upstreamoidc.go @@ -242,7 +242,7 @@ func ExtractUpstreamSubjectFromDownstream(downstreamSubject string) (string, err if !strings.Contains(downstreamSubject, "?sub=") { return "", errors.New("downstream subject did not contain original upstream subject") } - return strings.SplitN(downstreamSubject, "?sub=", 2)[1], nil // TODO test for ?sub= occurring twice (imagine if you ran the supervisor with another supervisor as the upstream idp...) + return strings.SplitN(downstreamSubject, "?sub=", 2)[1], nil } // ValidateToken will validate the ID token. It will also merge the claims from the userinfo endpoint response, diff --git a/internal/upstreamoidc/upstreamoidc_test.go b/internal/upstreamoidc/upstreamoidc_test.go index 949a56d7..7d5552f4 100644 --- a/internal/upstreamoidc/upstreamoidc_test.go +++ b/internal/upstreamoidc/upstreamoidc_test.go @@ -910,6 +910,45 @@ func TestProviderConfig(t *testing.T) { } }) + t.Run("ExtractUpstreamSubjectFromDownstream", func(t *testing.T) { + tests := []struct { + name string + downstreamSubject string + wantUpstreamSubject string + wantErr string + }{ + { + name: "happy path", + downstreamSubject: "https://some-issuer?sub=some-subject", + wantUpstreamSubject: "some-subject", + }, + { + name: "subject in a subject", + downstreamSubject: "https://some-other-issuer?sub=https://some-issuer?sub=some-subject", + wantUpstreamSubject: "https://some-issuer?sub=some-subject", + }, + { + name: "doesn't contain sub=", + downstreamSubject: "something-invalid", + wantErr: "downstream subject did not contain original upstream subject", + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + actualUpstreamSubject, err := ExtractUpstreamSubjectFromDownstream(tt.downstreamSubject) + if tt.wantErr != "" { + require.Error(t, err) + require.Equal(t, tt.wantErr, err.Error()) + } else { + require.NoError(t, err) + require.Equal(t, tt.wantUpstreamSubject, actualUpstreamSubject) + } + }) + } + + }) + t.Run("ExchangeAuthcodeAndValidateTokens", func(t *testing.T) { tests := []struct { name string diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index b10f650e..db2f617e 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -87,10 +87,8 @@ func TestSupervisorLogin(t *testing.T) { }, requestAuthorization: requestAuthorizationUsingBrowserAuthcodeFlow, breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession, _, _ string) { - customSessionData := pinnipedSession.Custom - require.Equal(t, psession.ProviderTypeOIDC, customSessionData.ProviderType) - require.NotEmpty(t, customSessionData.OIDC.UpstreamRefreshToken) - customSessionData.OIDC.UpstreamRefreshToken = "invalid-updated-refresh-token" + fositeSessionData := pinnipedSession.Fosite + fositeSessionData.Claims.Subject = "wrong-subject" }, // the ID token Subject should include the upstream user ID after the upstream issuer name wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Issuer+"?sub=") + ".+", @@ -124,10 +122,8 @@ func TestSupervisorLogin(t *testing.T) { }, requestAuthorization: requestAuthorizationUsingBrowserAuthcodeFlow, breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession, _, _ string) { - customSessionData := pinnipedSession.Custom - require.Equal(t, psession.ProviderTypeOIDC, customSessionData.ProviderType) - require.NotEmpty(t, customSessionData.OIDC.UpstreamRefreshToken) - customSessionData.OIDC.UpstreamRefreshToken = "invalid-updated-refresh-token" + fositeSessionData := pinnipedSession.Fosite + fositeSessionData.Claims.Extra["username"] = "some-incorrect-username" }, wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Issuer+"?sub=") + ".+", wantDownstreamIDTokenUsernameToMatch: func(_ string) string { return "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Username) + "$" }, From c9cf13a01fbf5735010c9be4ff72fc1969b677c0 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Tue, 14 Dec 2021 15:27:08 -0800 Subject: [PATCH 5/8] Check for issuer if available Signed-off-by: Margo Crawford --- internal/oidc/token/token_handler.go | 17 +++++++----- internal/oidc/token/token_handler_test.go | 31 ++++++++++++++++++++++ internal/upstreamoidc/upstreamoidc.go | 7 ++--- internal/upstreamoidc/upstreamoidc_test.go | 21 ++++++++++++--- 4 files changed, 63 insertions(+), 13 deletions(-) diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index 9e27be10..9ec64dc5 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -141,7 +141,7 @@ func upstreamOIDCRefresh(ctx context.Context, session *psession.PinnipedSession, if len(validatedTokens.IDToken.Claims) != 0 { newSub := claims["sub"] oldDownstreamSubject := session.Fosite.Claims.Subject - oldSub, err := upstreamoidc.ExtractUpstreamSubjectFromDownstream(oldDownstreamSubject) + oldIss, oldSub, err := upstreamoidc.ExtractUpstreamSubjectAndIssuerFromDownstream(oldDownstreamSubject) if err != nil { return errorsx.WithStack(errUpstreamRefreshError.WithHintf("Upstream refresh failed."). WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) @@ -152,14 +152,17 @@ func upstreamOIDCRefresh(ctx context.Context, session *psession.PinnipedSession, } usernameClaim := p.GetUsernameClaim() newUsername := claims[usernameClaim] + oldUsername := session.Fosite.Claims.Extra["username"] // its possible this won't be returned. // but if it is, verify that it hasn't changed. - if newUsername != nil { - oldUsername := session.Fosite.Claims.Extra["username"] - if oldUsername != newUsername { - return errorsx.WithStack(errUpstreamRefreshError.WithHintf( - "Upstream refresh failed.").WithWrap(errors.New("username in upstream refresh does not match previous value")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) - } + if newUsername != nil && oldUsername != newUsername { + return errorsx.WithStack(errUpstreamRefreshError.WithHintf( + "Upstream refresh failed.").WithWrap(errors.New("username in upstream refresh does not match previous value")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + } + newIssuer := claims["iss"] + if newIssuer != nil && oldIss != newIssuer { + return errorsx.WithStack(errUpstreamRefreshError.WithHintf( + "Upstream refresh failed.").WithWrap(errors.New("issuer in upstream refresh does not match previous value")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) } } diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index eb81a280..b53987a4 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -1706,6 +1706,37 @@ func TestRefreshGrant(t *testing.T) { }, }, }, + { + name: "refresh grant with changed issuer claim", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( + upstreamOIDCIdentityProviderBuilder().WithUsernameClaim("username-claim").WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "some-claim": "some-value", + "sub": "some-subject", + "iss": "some-changed-issuer", + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + authcodeExchange: authcodeExchangeInputs{ + customSessionData: initialUpstreamOIDCCustomSessionData(), + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), + wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()), + wantStatus: http.StatusUnauthorized, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "Error during upstream refresh. Upstream refresh failed." + } + `), + }, + }, + }, { name: "upstream ldap refresh happy path", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&oidctestutil.TestUpstreamLDAPIdentityProvider{ diff --git a/internal/upstreamoidc/upstreamoidc.go b/internal/upstreamoidc/upstreamoidc.go index 1c7b7d8c..53a6eab6 100644 --- a/internal/upstreamoidc/upstreamoidc.go +++ b/internal/upstreamoidc/upstreamoidc.go @@ -238,11 +238,12 @@ func (p *ProviderConfig) tryRevokeRefreshToken( } } -func ExtractUpstreamSubjectFromDownstream(downstreamSubject string) (string, error) { +func ExtractUpstreamSubjectAndIssuerFromDownstream(downstreamSubject string) (string, string, error) { if !strings.Contains(downstreamSubject, "?sub=") { - return "", errors.New("downstream subject did not contain original upstream subject") + return "", "", errors.New("downstream subject did not contain original upstream subject") } - return strings.SplitN(downstreamSubject, "?sub=", 2)[1], nil + split := strings.SplitN(downstreamSubject, "?sub=", 2) + return split[0], split[1], nil } // ValidateToken will validate the ID token. It will also merge the claims from the userinfo endpoint response, diff --git a/internal/upstreamoidc/upstreamoidc_test.go b/internal/upstreamoidc/upstreamoidc_test.go index 7d5552f4..91152c59 100644 --- a/internal/upstreamoidc/upstreamoidc_test.go +++ b/internal/upstreamoidc/upstreamoidc_test.go @@ -910,22 +910,37 @@ func TestProviderConfig(t *testing.T) { } }) - t.Run("ExtractUpstreamSubjectFromDownstream", func(t *testing.T) { + t.Run("ExtractUpstreamSubjectAndIssuerFromDownstream", func(t *testing.T) { tests := []struct { name string downstreamSubject string wantUpstreamSubject string + wantUpstreamIssuer string wantErr string }{ { name: "happy path", downstreamSubject: "https://some-issuer?sub=some-subject", wantUpstreamSubject: "some-subject", + wantUpstreamIssuer: "https://some-issuer", + }, + { + name: "happy path but sub is empty string", // todo i think this should not be the responsibility of this function, even though it's undesirable behavior... + downstreamSubject: "https://some-issuer?sub=", + wantUpstreamSubject: "", + wantUpstreamIssuer: "https://some-issuer", + }, + { + name: "happy path but iss is empty string", + downstreamSubject: "?sub=some-subject", + wantUpstreamSubject: "some-subject", + wantUpstreamIssuer: "", }, { name: "subject in a subject", downstreamSubject: "https://some-other-issuer?sub=https://some-issuer?sub=some-subject", wantUpstreamSubject: "https://some-issuer?sub=some-subject", + wantUpstreamIssuer: "https://some-other-issuer", }, { name: "doesn't contain sub=", @@ -936,17 +951,17 @@ func TestProviderConfig(t *testing.T) { for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { - actualUpstreamSubject, err := ExtractUpstreamSubjectFromDownstream(tt.downstreamSubject) + actualUpstreamIssuer, actualUpstreamSubject, err := ExtractUpstreamSubjectAndIssuerFromDownstream(tt.downstreamSubject) if tt.wantErr != "" { require.Error(t, err) require.Equal(t, tt.wantErr, err.Error()) } else { require.NoError(t, err) require.Equal(t, tt.wantUpstreamSubject, actualUpstreamSubject) + require.Equal(t, tt.wantUpstreamIssuer, actualUpstreamIssuer) } }) } - }) t.Run("ExchangeAuthcodeAndValidateTokens", func(t *testing.T) { From f2d21449323131fb03471457e9149b9b8e5f876c Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Thu, 16 Dec 2021 12:53:49 -0800 Subject: [PATCH 6/8] rename ValidateToken to ValidateTokenAndMergeWithUserInfo to better reflect what it's doing Also changed a few comments and small things --- .../mockupstreamoidcidentityprovider.go | 11 ++-- .../downstreamsession/downstream_session.go | 4 +- .../provider/dynamic_upstream_idp_provider.go | 2 +- internal/oidc/token/token_handler.go | 2 +- internal/oidc/token/token_handler_test.go | 7 +-- .../testutil/oidctestutil/oidctestutil.go | 25 ++------ internal/upstreamoidc/upstreamoidc.go | 30 +++++----- internal/upstreamoidc/upstreamoidc_test.go | 57 ++++++++++--------- pkg/oidcclient/login.go | 2 +- 9 files changed, 62 insertions(+), 78 deletions(-) diff --git a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go index 092aede5..2a2a689e 100644 --- a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go +++ b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go @@ -14,11 +14,12 @@ import ( reflect "reflect" gomock "github.com/golang/mock/gomock" + oauth2 "golang.org/x/oauth2" + types "k8s.io/apimachinery/pkg/types" + nonce "go.pinniped.dev/pkg/oidcclient/nonce" oidctypes "go.pinniped.dev/pkg/oidcclient/oidctypes" pkce "go.pinniped.dev/pkg/oidcclient/pkce" - oauth2 "golang.org/x/oauth2" - types "k8s.io/apimachinery/pkg/types" ) // MockUpstreamOIDCIdentityProviderI is a mock of UpstreamOIDCIdentityProviderI interface. @@ -230,9 +231,9 @@ func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) RevokeRefreshToken(arg0 } // ValidateToken mocks base method. -func (m *MockUpstreamOIDCIdentityProviderI) ValidateToken(arg0 context.Context, arg1 *oauth2.Token, arg2 nonce.Nonce, arg3 bool) (*oidctypes.Token, error) { +func (m *MockUpstreamOIDCIdentityProviderI) ValidateTokenAndMergeWithUserInfo(arg0 context.Context, arg1 *oauth2.Token, arg2 nonce.Nonce, arg3 bool) (*oidctypes.Token, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ValidateToken", arg0, arg1, arg2, arg3) + ret := m.ctrl.Call(m, "ValidateTokenAndMergeWithUserInfo", arg0, arg1, arg2, arg3) ret0, _ := ret[0].(*oidctypes.Token) ret1, _ := ret[1].(error) return ret0, ret1 @@ -241,5 +242,5 @@ func (m *MockUpstreamOIDCIdentityProviderI) ValidateToken(arg0 context.Context, // ValidateToken indicates an expected call of ValidateToken. func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) ValidateToken(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ValidateToken", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).ValidateToken), arg0, arg1, arg2, arg3) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ValidateTokenAndMergeWithUserInfo", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).ValidateTokenAndMergeWithUserInfo), arg0, arg1, arg2, arg3) } diff --git a/internal/oidc/downstreamsession/downstream_session.go b/internal/oidc/downstreamsession/downstream_session.go index 2aa66f5c..5f0cf5f0 100644 --- a/internal/oidc/downstreamsession/downstream_session.go +++ b/internal/oidc/downstreamsession/downstream_session.go @@ -97,7 +97,7 @@ func getSubjectAndUsernameFromUpstreamIDToken( if err != nil { return "", "", err } - subject := DownstreamSubjectFromUpstreamOIDC(upstreamIssuer, upstreamSubject) + subject := downstreamSubjectFromUpstreamOIDC(upstreamIssuer, upstreamSubject) usernameClaimName := upstreamIDPConfig.GetUsernameClaim() if usernameClaimName == "" { @@ -176,7 +176,7 @@ func DownstreamLDAPSubject(uid string, ldapURL url.URL) string { return ldapURL.String() } -func DownstreamSubjectFromUpstreamOIDC(upstreamIssuerAsString string, upstreamSubject string) string { +func downstreamSubjectFromUpstreamOIDC(upstreamIssuerAsString string, upstreamSubject string) string { return fmt.Sprintf("%s?%s=%s", upstreamIssuerAsString, oidc.IDTokenSubjectClaim, url.QueryEscape(upstreamSubject)) } diff --git a/internal/oidc/provider/dynamic_upstream_idp_provider.go b/internal/oidc/provider/dynamic_upstream_idp_provider.go index 41f9e608..9986f018 100644 --- a/internal/oidc/provider/dynamic_upstream_idp_provider.go +++ b/internal/oidc/provider/dynamic_upstream_idp_provider.go @@ -74,7 +74,7 @@ type UpstreamOIDCIdentityProviderI interface { // ValidateToken will validate the ID token. It will also merge the claims from the userinfo endpoint response // into the ID token's claims, if the provider offers the userinfo endpoint. It returns the validated/updated // tokens, or an error. - ValidateToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool) (*oidctypes.Token, error) + ValidateTokenAndMergeWithUserInfo(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool) (*oidctypes.Token, error) } type UpstreamLDAPIdentityProviderI interface { diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index 9ec64dc5..b1888a18 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -128,7 +128,7 @@ func upstreamOIDCRefresh(ctx context.Context, session *psession.PinnipedSession, // The spec is not 100% clear about whether an ID token from the refresh flow should include a nonce, and at // least some providers do not include one, so we skip the nonce validation here (but not other validations). - validatedTokens, err := p.ValidateToken(ctx, refreshedTokens, "", hasIDTok) + validatedTokens, err := p.ValidateTokenAndMergeWithUserInfo(ctx, refreshedTokens, "", hasIDTok) if err != nil { return errorsx.WithStack(errUpstreamRefreshError.WithHintf( "Upstream refresh returned an invalid ID token or UserInfo response.").WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index b53987a4..24fea4ad 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -982,7 +982,6 @@ func TestRefreshGrant(t *testing.T) { want := happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(wantCustomSessionDataStored) // Should always try to perform an upstream refresh. want.wantUpstreamRefreshCall = happyOIDCUpstreamRefreshCall() - // Should only try to ValidateToken when there was an id token returned by the upstream refresh. if expectToValidateToken != nil { want.wantUpstreamOIDCValidateTokenCall = happyUpstreamValidateTokenCall(expectToValidateToken) } @@ -1137,7 +1136,7 @@ func TestRefreshGrant(t *testing.T) { refreshRequest: refreshRequestInputs{ want: happyRefreshTokenResponseForOpenIDAndOfflineAccess( upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken), - refreshedUpstreamTokensWithRefreshTokenWithoutIDToken(), // expect ValidateToken is called + refreshedUpstreamTokensWithRefreshTokenWithoutIDToken(), // expect ValidateTokenAndMergeWithUserInfo is called ), }, }, @@ -1592,7 +1591,7 @@ func TestRefreshGrant(t *testing.T) { name: "when the upstream refresh returns an invalid ID token during the refresh request", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder(). WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()). - // This is the current format of the errors returned by the production code version of ValidateToken, see ValidateToken in upstreamoidc.go + // This is the current format of the errors returned by the production code version of ValidateTokenAndMergeWithUserInfo, see ValidateTokenAndMergeWithUserInfo in upstreamoidc.go WithValidateTokenError(httperr.Wrap(http.StatusBadRequest, "some validate error", errors.New("some validate cause"))). Build()), authcodeExchange: authcodeExchangeInputs{ @@ -1618,7 +1617,7 @@ func TestRefreshGrant(t *testing.T) { name: "when the upstream refresh returns an ID token with a different subject than the original", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder(). WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()). - // This is the current format of the errors returned by the production code version of ValidateToken, see ValidateToken in upstreamoidc.go + // This is the current format of the errors returned by the production code version of ValidateTokenAndMergeWithUserInfo, see ValidateTokenAndMergeWithUserInfo in upstreamoidc.go WithValidatedTokens(&oidctypes.Token{ IDToken: &oidctypes.IDToken{ Claims: map[string]interface{}{ diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index a4f0b31e..aa682081 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -176,8 +176,6 @@ type TestUpstreamOIDCIdentityProvider struct { ValidateTokenFunc func(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) - ValidateRefreshFunc func(ctx context.Context, tok *oauth2.Token, storedAttributes provider.StoredRefreshAttributes) error - exchangeAuthcodeAndValidateTokensCallCount int exchangeAuthcodeAndValidateTokensArgs []*ExchangeAuthcodeAndValidateTokenArgs passwordCredentialsGrantAndValidateTokensCallCount int @@ -188,8 +186,6 @@ type TestUpstreamOIDCIdentityProvider struct { revokeRefreshTokenArgs []*RevokeRefreshTokenArgs validateTokenCallCount int validateTokenArgs []*ValidateTokenArgs - validateRefreshCallCount int - validateRefreshArgs []*ValidateRefreshArgs } var _ provider.UpstreamOIDCIdentityProviderI = &TestUpstreamOIDCIdentityProvider{} @@ -288,19 +284,6 @@ func (u *TestUpstreamOIDCIdentityProvider) PerformRefresh(ctx context.Context, r return u.PerformRefreshFunc(ctx, refreshToken) } -func (u *TestUpstreamOIDCIdentityProvider) ValidateRefresh(ctx context.Context, tok *oauth2.Token, storedAttributes provider.StoredRefreshAttributes) error { - if u.validateRefreshArgs == nil { - u.validateRefreshArgs = make([]*ValidateRefreshArgs, 0) - } - u.validateRefreshCallCount++ - u.validateRefreshArgs = append(u.validateRefreshArgs, &ValidateRefreshArgs{ - Ctx: ctx, - Tok: tok, - StoredAttributes: storedAttributes, - }) - return u.ValidateRefreshFunc(ctx, tok, storedAttributes) -} - func (u *TestUpstreamOIDCIdentityProvider) RevokeRefreshToken(ctx context.Context, refreshToken string) error { if u.revokeRefreshTokenArgs == nil { u.revokeRefreshTokenArgs = make([]*RevokeRefreshTokenArgs, 0) @@ -335,7 +318,7 @@ func (u *TestUpstreamOIDCIdentityProvider) RevokeRefreshTokenArgs(call int) *Rev return u.revokeRefreshTokenArgs[call] } -func (u *TestUpstreamOIDCIdentityProvider) ValidateToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool) (*oidctypes.Token, error) { +func (u *TestUpstreamOIDCIdentityProvider) ValidateTokenAndMergeWithUserInfo(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool) (*oidctypes.Token, error) { if u.validateTokenArgs == nil { u.validateTokenArgs = make([]*ValidateTokenArgs, 0) } @@ -556,10 +539,10 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToValidateToken( } } require.Equal(t, 1, actualCallCountAcrossAllOIDCUpstreams, - "should have been exactly one call to ValidateToken() by all OIDC upstreams", + "should have been exactly one call to ValidateTokenAndMergeWithUserInfo() by all OIDC upstreams", ) require.Equal(t, expectedPerformedByUpstreamName, actualNameOfUpstreamWhichMadeCall, - "ValidateToken() was called on the wrong OIDC upstream", + "ValidateTokenAndMergeWithUserInfo() was called on the wrong OIDC upstream", ) require.Equal(t, expectedArgs, actualArgs) } @@ -571,7 +554,7 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToValidateToken(t *tes actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.validateTokenCallCount } require.Equal(t, 0, actualCallCountAcrossAllOIDCUpstreams, - "expected exactly zero calls to ValidateToken()", + "expected exactly zero calls to ValidateTokenAndMergeWithUserInfo()", ) } diff --git a/internal/upstreamoidc/upstreamoidc.go b/internal/upstreamoidc/upstreamoidc.go index 53a6eab6..f9256f23 100644 --- a/internal/upstreamoidc/upstreamoidc.go +++ b/internal/upstreamoidc/upstreamoidc.go @@ -114,7 +114,7 @@ func (p *ProviderConfig) PasswordCredentialsGrantAndValidateTokens(ctx context.C // There is no nonce to validate for a resource owner password credentials grant because it skips using // the authorize endpoint and goes straight to the token endpoint. const skipNonceValidation nonce.Nonce = "" - return p.ValidateToken(ctx, tok, skipNonceValidation, true) + return p.ValidateTokenAndMergeWithUserInfo(ctx, tok, skipNonceValidation, true) } func (p *ProviderConfig) ExchangeAuthcodeAndValidateTokens(ctx context.Context, authcode string, pkceCodeVerifier pkce.Code, expectedIDTokenNonce nonce.Nonce, redirectURI string) (*oidctypes.Token, error) { @@ -128,7 +128,7 @@ func (p *ProviderConfig) ExchangeAuthcodeAndValidateTokens(ctx context.Context, return nil, err } - return p.ValidateToken(ctx, tok, expectedIDTokenNonce, true) + return p.ValidateTokenAndMergeWithUserInfo(ctx, tok, expectedIDTokenNonce, true) } func (p *ProviderConfig) PerformRefresh(ctx context.Context, refreshToken string) (*oauth2.Token, error) { @@ -243,15 +243,17 @@ func ExtractUpstreamSubjectAndIssuerFromDownstream(downstreamSubject string) (st return "", "", errors.New("downstream subject did not contain original upstream subject") } split := strings.SplitN(downstreamSubject, "?sub=", 2) + iss := split[0] + sub := split[1] + if iss == "" || sub == "" { + return "", "", errors.New("downstream subject was malformed") + } return split[0], split[1], nil } -// ValidateToken will validate the ID token. It will also merge the claims from the userinfo endpoint response, +// ValidateTokenAndMergeWithUserInfo will validate the ID token. It will also merge the claims from the userinfo endpoint response, // if the provider offers the userinfo endpoint. -// TODO check: -// - whether the userinfo response must exist (maybe just based on whether there is a refresh token??? -// - -> for the next story: userinfo has to exist but only if there isn't a refresh token. -func (p *ProviderConfig) ValidateToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool) (*oidctypes.Token, error) { +func (p *ProviderConfig) ValidateTokenAndMergeWithUserInfo(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool) (*oidctypes.Token, error) { var validatedClaims = make(map[string]interface{}) idTok, hasIDTok := tok.Extra("id_token").(string) @@ -281,7 +283,7 @@ func (p *ProviderConfig) ValidateToken(ctx context.Context, tok *oauth2.Token, e return nil, httperr.Wrap(http.StatusInternalServerError, "could not unmarshal id token claims", err) } maybeLogClaims("claims from ID token", p.Name, validatedClaims) - idTokenExpiry = validated.Expiry + idTokenExpiry = validated.Expiry // keep track of the id token expiry if we have an id token. Otherwise, it'll just be the zero value. } idTokenSubject, _ := validatedClaims[oidc.IDTokenSubjectClaim].(string) @@ -320,31 +322,27 @@ func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, t } // The sub (subject) Claim MUST always be returned in the UserInfo Response. - // However there may not be an id token. If there is an ID token, we must - // check it against the userinfo's subject. - // // NOTE: Due to the possibility of token substitution attacks (see Section 16.11), the UserInfo Response is not // guaranteed to be about the End-User identified by the sub (subject) element of the ID Token. The sub Claim in // the UserInfo Response MUST be verified to exactly match the sub Claim in the ID Token; if they do not match, // the UserInfo Response values MUST NOT be used. // // http://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse + // If there is no ID token and it is not required, we must assume that the caller is performing other checks + // to ensure the subject is correct. checkIDToken := requireIDToken || len(idTokenSubject) > 0 if checkIDToken && (len(userInfo.Subject) == 0 || userInfo.Subject != idTokenSubject) { return httperr.Newf(http.StatusUnprocessableEntity, "userinfo 'sub' claim (%s) did not match id_token 'sub' claim (%s)", userInfo.Subject, idTokenSubject) } - if !checkIDToken { - claims["sub"] = userInfo.Subject // do this so other validations can check this subject later - } - + // keep track of the issuer from the ID token idTokenIssuer := claims["iss"] // merge existing claims with user info claims if err := userInfo.Claims(&claims); err != nil { return httperr.Wrap(http.StatusInternalServerError, "could not unmarshal user info claims", err) } - // The OIDC spec for user info response does not make any guarantees about the iss claim's existence or validity: + // The OIDC spec for the UserInfo response does not make any guarantees about the iss claim's existence or validity: // "If signed, the UserInfo Response SHOULD contain the Claims iss (issuer) and aud (audience) as members. The iss value SHOULD be the OP's Issuer Identifier URL." // See https://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse // So we just ignore it and use it the version from the id token, which has stronger guarantees. diff --git a/internal/upstreamoidc/upstreamoidc_test.go b/internal/upstreamoidc/upstreamoidc_test.go index 91152c59..e5b90c80 100644 --- a/internal/upstreamoidc/upstreamoidc_test.go +++ b/internal/upstreamoidc/upstreamoidc_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package upstreamoidc @@ -589,7 +589,7 @@ func TestProviderConfig(t *testing.T) { } }) - t.Run("ValidateToken", func(t *testing.T) { + t.Run("ValidateTokenAndMergeWithUserInfo", func(t *testing.T) { expiryTime := time.Now().Add(42 * time.Second) testTokenWithoutIDToken := &oauth2.Token{ AccessToken: "test-access-token", @@ -646,7 +646,7 @@ func TestProviderConfig(t *testing.T) { nonce: "some-nonce", requireIDToken: false, rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`), wantMergedTokens: &oidctypes.Token{ AccessToken: &oidctypes.AccessToken{ Token: "test-access-token", @@ -673,7 +673,7 @@ func TestProviderConfig(t *testing.T) { nonce: "some-nonce", requireIDToken: true, rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`), wantMergedTokens: &oidctypes.Token{ AccessToken: &oidctypes.AccessToken{ Token: "test-access-token", @@ -700,7 +700,7 @@ func TestProviderConfig(t *testing.T) { nonce: "some-nonce", requireIDToken: true, rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`), wantMergedTokens: &oidctypes.Token{ AccessToken: &oidctypes.AccessToken{ Token: "test-access-token", @@ -727,7 +727,7 @@ func TestProviderConfig(t *testing.T) { nonce: "some-nonce", requireIDToken: true, rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "iss": "some-other-issuer"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "iss": "some-other-issuer", "sub": "some-subject"}`), wantMergedTokens: &oidctypes.Token{ AccessToken: &oidctypes.AccessToken{ Token: "test-access-token", @@ -754,7 +754,7 @@ func TestProviderConfig(t *testing.T) { nonce: "", requireIDToken: false, rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "iss": "some-other-issuer"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "iss": "some-other-issuer", "sub": "some-subject"}`), wantMergedTokens: &oidctypes.Token{ AccessToken: &oidctypes.AccessToken{ Token: "test-access-token", @@ -799,7 +799,7 @@ func TestProviderConfig(t *testing.T) { nonce: "some-nonce", requireIDToken: true, rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal", "sub": "some-other-subject"}`), wantErr: "could not fetch user info claims: userinfo 'sub' claim (some-other-subject) did not match id_token 'sub' claim (some-subject)", }, { @@ -808,7 +808,7 @@ func TestProviderConfig(t *testing.T) { nonce: "some-nonce", requireIDToken: false, rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal", "sub": "some-other-subject"}`), wantErr: "could not fetch user info claims: userinfo 'sub' claim (some-other-subject) did not match id_token 'sub' claim (some-subject)", }, { @@ -817,7 +817,7 @@ func TestProviderConfig(t *testing.T) { nonce: "some-nonce", requireIDToken: true, rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal", "sub": "some-other-subject"}`), wantErr: "received invalid ID token: oidc: malformed jwt: square/go-jose: compact JWS format must have three parts", }, { @@ -826,7 +826,7 @@ func TestProviderConfig(t *testing.T) { nonce: "some-other-nonce", requireIDToken: true, rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal", "sub": "some-other-subject"}`), wantErr: "received ID token with invalid nonce: invalid nonce (expected \"some-other-nonce\", got \"some-nonce\")", }, { @@ -835,7 +835,7 @@ func TestProviderConfig(t *testing.T) { nonce: "some-other-nonce", requireIDToken: true, rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`), wantErr: "received response missing ID token", }, { @@ -844,7 +844,7 @@ func TestProviderConfig(t *testing.T) { nonce: "some-other-nonce", requireIDToken: true, rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + userInfo: forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`), wantErr: "received response missing ID token", }, { @@ -853,7 +853,7 @@ func TestProviderConfig(t *testing.T) { nonce: "some-nonce", requireIDToken: true, rawClaims: []byte(`{"userinfo_endpoint": "not-empty"}`), - userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal"}`), + userInfo: forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal", "sub": "some-other-subject"}`), wantMergedTokens: &oidctypes.Token{ AccessToken: &oidctypes.AccessToken{ Token: "test-access-token", @@ -898,7 +898,7 @@ func TestProviderConfig(t *testing.T) { userInfoErr: tt.userInfoErr, }, } - gotTok, err := p.ValidateToken(context.Background(), tt.tok, tt.nonce, tt.requireIDToken) + gotTok, err := p.ValidateTokenAndMergeWithUserInfo(context.Background(), tt.tok, tt.nonce, tt.requireIDToken) if tt.wantErr != "" { require.Error(t, err) require.Equal(t, tt.wantErr, err.Error()) @@ -924,24 +924,27 @@ func TestProviderConfig(t *testing.T) { wantUpstreamSubject: "some-subject", wantUpstreamIssuer: "https://some-issuer", }, - { - name: "happy path but sub is empty string", // todo i think this should not be the responsibility of this function, even though it's undesirable behavior... - downstreamSubject: "https://some-issuer?sub=", - wantUpstreamSubject: "", - wantUpstreamIssuer: "https://some-issuer", - }, - { - name: "happy path but iss is empty string", - downstreamSubject: "?sub=some-subject", - wantUpstreamSubject: "some-subject", - wantUpstreamIssuer: "", - }, { name: "subject in a subject", downstreamSubject: "https://some-other-issuer?sub=https://some-issuer?sub=some-subject", wantUpstreamSubject: "https://some-issuer?sub=some-subject", wantUpstreamIssuer: "https://some-other-issuer", }, + { + name: "sub is empty string", + downstreamSubject: "https://some-issuer?sub=", + wantErr: "downstream subject was malformed", + }, + { + name: "iss is empty string", + downstreamSubject: "?sub=some-subject", + wantErr: "downstream subject was malformed", + }, + { + name: "empty string", + downstreamSubject: "", + wantErr: "downstream subject did not contain original upstream subject", + }, { name: "doesn't contain sub=", downstreamSubject: "something-invalid", diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index 941693e2..325ec4a4 100644 --- a/pkg/oidcclient/login.go +++ b/pkg/oidcclient/login.go @@ -822,7 +822,7 @@ func (h *handlerState) handleRefresh(ctx context.Context, refreshToken *oidctype // The spec is not 100% clear about whether an ID token from the refresh flow should include a nonce, and at least // some providers do not include one, so we skip the nonce validation here (but not other validations). - return upstreamOIDCIdentityProvider.ValidateToken(ctx, refreshed, "", true) + return upstreamOIDCIdentityProvider.ValidateTokenAndMergeWithUserInfo(ctx, refreshed, "", true) } func (h *handlerState) handleAuthCodeCallback(w http.ResponseWriter, r *http.Request) (err error) { From 29584619707d6dc6141a33b5d34d7db3ad2c1799 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Fri, 7 Jan 2022 15:04:58 -0800 Subject: [PATCH 7/8] Addressing PR feedback store issuer and subject in storage for refresh Clean up some constants Signed-off-by: Margo Crawford --- .../oidc_upstream_watcher.go | 6 +- .../oidc_upstream_watcher_test.go | 50 ++++++------ .../accesstoken/accesstoken_test.go | 6 +- .../authorizationcode/authorizationcode.go | 24 +++--- .../authorizationcode_test.go | 7 +- .../openidconnect/openidconnect_test.go | 4 +- internal/fositestorage/pkce/pkce_test.go | 4 +- .../refreshtoken/refreshtoken_test.go | 6 +- internal/oidc/auth/auth_handler.go | 18 ++++- internal/oidc/auth/auth_handler_test.go | 4 +- internal/oidc/callback/callback_handler.go | 13 ++- .../oidc/callback/callback_handler_test.go | 8 +- .../downstreamsession/downstream_session.go | 10 +-- .../provider/dynamic_upstream_idp_provider.go | 4 +- internal/oidc/token/token_handler.go | 32 ++++---- internal/oidc/token/token_handler_test.go | 21 ++--- internal/psession/pinniped_session.go | 4 +- internal/testutil/psession.go | 4 +- internal/upstreamoidc/upstreamoidc.go | 81 +++++++++---------- internal/upstreamoidc/upstreamoidc_test.go | 57 ------------- test/integration/supervisor_login_test.go | 6 +- 21 files changed, 178 insertions(+), 191 deletions(-) diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go index 909c07fb..4669cdc3 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package oidcupstreamwatcher implements a controller which watches OIDCIdentityProviders. @@ -499,7 +499,7 @@ func validateHTTPSURL(maybeHTTPSURL, endpointType, reason string) (*url.URL, *v1 Type: typeOIDCDiscoverySucceeded, Status: v1alpha1.ConditionFalse, Reason: reason, - Message: fmt.Sprintf(`%s URL scheme must be "https", not %q`, endpointType, parsedURL.Scheme), + Message: fmt.Sprintf(`%s URL '%s' must have "https" scheme, not %q`, endpointType, maybeHTTPSURL, parsedURL.Scheme), } } if len(parsedURL.Query()) != 0 || parsedURL.Fragment != "" { @@ -507,7 +507,7 @@ func validateHTTPSURL(maybeHTTPSURL, endpointType, reason string) (*url.URL, *v1 Type: typeOIDCDiscoverySucceeded, Status: v1alpha1.ConditionFalse, Reason: reason, - Message: fmt.Sprintf(`%s URL cannot contain query or fragment component`, endpointType), + Message: fmt.Sprintf(`%s URL '%s' cannot contain query or fragment component`, endpointType, maybeHTTPSURL), } } return parsedURL, nil diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go index 8a50d13a..62e308e2 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package oidcupstreamwatcher @@ -457,9 +457,9 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="issuer URL scheme must be \"https\", not \"http\"" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="issuer URL '` + strings.Replace(testIssuerURL, "https", "http", 1) + `' must have \"https\" scheme, not \"http\"" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, - `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="issuer URL scheme must be \"https\", not \"http\"" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="issuer URL '` + strings.Replace(testIssuerURL, "https", "http", 1) + `' must have \"https\" scheme, not \"http\"" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, }, wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ @@ -480,7 +480,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { Status: "False", LastTransitionTime: now, Reason: "Unreachable", - Message: `issuer URL scheme must be "https", not "http"`, + Message: `issuer URL '` + strings.Replace(testIssuerURL, "https", "http", 1) + `' must have "https" scheme, not "http"`, }, }, }, @@ -503,9 +503,9 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="issuer URL cannot contain query or fragment component" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="issuer URL '` + testIssuerURL + "?sub=foo" + `' cannot contain query or fragment component" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, - `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="issuer URL cannot contain query or fragment component" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="issuer URL '` + testIssuerURL + "?sub=foo" + `' cannot contain query or fragment component" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, }, wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ @@ -526,7 +526,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { Status: "False", LastTransitionTime: now, Reason: "Unreachable", - Message: `issuer URL cannot contain query or fragment component`, + Message: `issuer URL '` + testIssuerURL + "?sub=foo" + `' cannot contain query or fragment component`, }, }, }, @@ -549,9 +549,9 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="issuer URL cannot contain query or fragment component" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="issuer URL '` + testIssuerURL + "#fragment" + `' cannot contain query or fragment component" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, - `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="issuer URL cannot contain query or fragment component" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="issuer URL '` + testIssuerURL + "#fragment" + `' cannot contain query or fragment component" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`, }, wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ @@ -572,7 +572,7 @@ func TestOIDCUpstreamWatcherControllerSync(t *testing.T) { Status: "False", LastTransitionTime: now, Reason: "Unreachable", - Message: `issuer URL cannot contain query or fragment component`, + Message: `issuer URL '` + testIssuerURL + "#fragment" + `' cannot contain query or fragment component`, }, }, }, @@ -739,9 +739,9 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="authorization endpoint URL scheme must be \"https\", not \"http\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="authorization endpoint URL 'http://example.com/authorize' must have \"https\" scheme, not \"http\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, - `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="authorization endpoint URL scheme must be \"https\", not \"http\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="authorization endpoint URL 'http://example.com/authorize' must have \"https\" scheme, not \"http\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, }, wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ @@ -762,7 +762,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Status: "False", LastTransitionTime: now, Reason: "InvalidResponse", - Message: `authorization endpoint URL scheme must be "https", not "http"`, + Message: `authorization endpoint URL 'http://example.com/authorize' must have "https" scheme, not "http"`, }, }, }, @@ -786,9 +786,9 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="revocation endpoint URL scheme must be \"https\", not \"http\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="revocation endpoint URL 'http://example.com/revoke' must have \"https\" scheme, not \"http\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, - `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="revocation endpoint URL scheme must be \"https\", not \"http\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="revocation endpoint URL 'http://example.com/revoke' must have \"https\" scheme, not \"http\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, }, wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ @@ -809,7 +809,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Status: "False", LastTransitionTime: now, Reason: "InvalidResponse", - Message: `revocation endpoint URL scheme must be "https", not "http"`, + Message: `revocation endpoint URL 'http://example.com/revoke' must have "https" scheme, not "http"`, }, }, }, @@ -833,9 +833,9 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="token endpoint URL scheme must be \"https\", not \"http\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="token endpoint URL 'http://example.com/token' must have \"https\" scheme, not \"http\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, - `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="token endpoint URL scheme must be \"https\", not \"http\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="token endpoint URL 'http://example.com/token' must have \"https\" scheme, not \"http\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, }, wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ @@ -856,7 +856,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Status: "False", LastTransitionTime: now, Reason: "InvalidResponse", - Message: `token endpoint URL scheme must be "https", not "http"`, + Message: `token endpoint URL 'http://example.com/token' must have "https" scheme, not "http"`, }, }, }, @@ -880,9 +880,9 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="token endpoint URL scheme must be \"https\", not \"\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="token endpoint URL '' must have \"https\" scheme, not \"\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, - `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="token endpoint URL scheme must be \"https\", not \"\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="token endpoint URL '' must have \"https\" scheme, not \"\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, }, wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ @@ -903,7 +903,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Status: "False", LastTransitionTime: now, Reason: "InvalidResponse", - Message: `token endpoint URL scheme must be "https", not ""`, + Message: `token endpoint URL '' must have "https" scheme, not ""`, }, }, }, @@ -927,9 +927,9 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantLogs: []string{ `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, - `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="authorization endpoint URL scheme must be \"https\", not \"\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="authorization endpoint URL '' must have \"https\" scheme, not \"\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`, `oidc-upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="additionalAuthorizeParameters parameter names are allowed" "reason"="Success" "status"="True" "type"="AdditionalAuthorizeParametersValid"`, - `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="authorization endpoint URL scheme must be \"https\", not \"\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, + `oidc-upstream-observer "msg"="found failing condition" "error"="OIDCIdentityProvider has a failing condition" "message"="authorization endpoint URL '' must have \"https\" scheme, not \"\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`, }, wantResultingCache: []*oidctestutil.TestUpstreamOIDCIdentityProvider{}, wantResultingUpstreams: []v1alpha1.OIDCIdentityProvider{{ @@ -950,7 +950,7 @@ Get "` + testIssuerURL + `/valid-url-that-is-really-really-long-nananananananana Status: "False", LastTransitionTime: now, Reason: "InvalidResponse", - Message: `authorization endpoint URL scheme must be "https", not ""`, + Message: `authorization endpoint URL '' must have "https" scheme, not ""`, }, }, }, diff --git a/internal/fositestorage/accesstoken/accesstoken_test.go b/internal/fositestorage/accesstoken/accesstoken_test.go index 2623ea5f..5b503371 100644 --- a/internal/fositestorage/accesstoken/accesstoken_test.go +++ b/internal/fositestorage/accesstoken/accesstoken_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package accesstoken @@ -53,7 +53,7 @@ func TestAccessTokenStorage(t *testing.T) { }, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/access-token", @@ -122,7 +122,7 @@ func TestAccessTokenStorageRevocation(t *testing.T) { }, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/access-token", diff --git a/internal/fositestorage/authorizationcode/authorizationcode.go b/internal/fositestorage/authorizationcode/authorizationcode.go index 1d678ec5..e7c6655b 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode.go +++ b/internal/fositestorage/authorizationcode/authorizationcode.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package authorizationcode @@ -370,29 +370,33 @@ const ExpectedAuthorizeCodeSessionJSONFromFuzzing = `{ "providerName": "ʼn2ƋŢ觛ǂ焺nŐǛ", "providerType": "ɥ闣ʬ橳(ý綃ʃʚƟ覣k眐4", "oidc": { - "upstreamRefreshToken": "tC嵽痊w" + "upstreamRefreshToken": "tC嵽痊w", + "upstreamSubject": "a紽ǒ|鰽ŋ猊I", + "upstreamIssuer": "妬\u003e6鉢緋uƴŤȱʀ" }, "ldap": { - "userDN": "Ź榨Q|ôɵt毇妬\u003e6鉢緋", + "userDN": "Â?墖\u003cƬb獭潜Ʃ饾k|鬌R蜚蠣", "extraRefreshAttributes": { - "ď逳鞪?3)藵睋邔\u0026Ű惫蜀Ģ¡圔": "墀jMʥ", - "ƍ蛊ʚ£:設虝27": "b獭潜Ʃ饾k|鬌R蜚蠣麹概", - "藚ɏ¬Ê蒭堜]ȗ韚ʫ": "鷞aŚB碠k9帴ʘ赱" + "ȱ藚ɏ¬Ê蒭堜]ȗ韚ʫ": "鷞aŚB碠k9帴ʘ赱" } }, "activedirectory": { - "userDN": "0D餹sêĝɓ", + "userDN": "瑹xȢ~1Įx欼笝?úT妼", "extraRefreshAttributes": { - "摱ì": "bEǎ儯惝Io" + "iYn": "麹Œ颛", + "İ\u003e×1飞O+î艔垎0OƉǢIȽ齤士": "ȐĨf跞@)¿,ɭS隑ip偶" } } } }, "requestedAudience": [ - "Ł" + "應,Ɣ鬅X¤", + "¤.岵骘胲ƤkǦ" ], "grantedAudience": [ - "r" + "鸖I¶媁y衑拁Ȃ", + "社Vƅȭǝ*擦28Dž 甍 ć", + "bņ抰蛖a³2ʫ承dʬ)ġ,TÀqy_" ] }, "version": "2" diff --git a/internal/fositestorage/authorizationcode/authorizationcode_test.go b/internal/fositestorage/authorizationcode/authorizationcode_test.go index 006ed61f..72c4cfba 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode_test.go +++ b/internal/fositestorage/authorizationcode/authorizationcode_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package authorizationcode @@ -65,7 +65,7 @@ func TestAuthorizationCodeStorage(t *testing.T) { }, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"active":true,"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), + "pinniped-storage-data": []byte(`{"active":true,"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/authcode", @@ -84,7 +84,7 @@ func TestAuthorizationCodeStorage(t *testing.T) { }, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"active":false,"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), + "pinniped-storage-data": []byte(`{"active":false,"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/authcode", @@ -389,6 +389,7 @@ func TestFuzzAndJSONNewValidEmptyAuthorizeCodeSession(t *testing.T) { validSessionJSONBytes, err := json.MarshalIndent(validSession, "", "\t") require.NoError(t, err) authorizeCodeSessionJSONFromFuzzing := string(validSessionJSONBytes) + t.Log(authorizeCodeSessionJSONFromFuzzing) // the fuzzed session and storage session should have identical JSON require.JSONEq(t, authorizeCodeSessionJSONFromFuzzing, authorizeCodeSessionJSONFromStorage) diff --git a/internal/fositestorage/openidconnect/openidconnect_test.go b/internal/fositestorage/openidconnect/openidconnect_test.go index 1ceafa9e..ac680a60 100644 --- a/internal/fositestorage/openidconnect/openidconnect_test.go +++ b/internal/fositestorage/openidconnect/openidconnect_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package openidconnect @@ -52,7 +52,7 @@ func TestOpenIdConnectStorage(t *testing.T) { }, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/oidc", diff --git a/internal/fositestorage/pkce/pkce_test.go b/internal/fositestorage/pkce/pkce_test.go index e21dda33..b84798ce 100644 --- a/internal/fositestorage/pkce/pkce_test.go +++ b/internal/fositestorage/pkce/pkce_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package pkce @@ -52,7 +52,7 @@ func TestPKCEStorage(t *testing.T) { }, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/pkce", diff --git a/internal/fositestorage/refreshtoken/refreshtoken_test.go b/internal/fositestorage/refreshtoken/refreshtoken_test.go index ae7a1226..ce74793a 100644 --- a/internal/fositestorage/refreshtoken/refreshtoken_test.go +++ b/internal/fositestorage/refreshtoken/refreshtoken_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package refreshtoken @@ -52,7 +52,7 @@ func TestRefreshTokenStorage(t *testing.T) { }, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/refresh-token", @@ -122,7 +122,7 @@ func TestRefreshTokenStorageRevocation(t *testing.T) { }, }, Data: map[string][]byte{ - "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), + "pinniped-storage-data": []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`), "pinniped-storage-version": []byte("1"), }, Type: "storage.pinniped.dev/refresh-token", diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index f9760825..b08d39bc 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package auth provides a handler for the OIDC authorization endpoint. @@ -191,6 +191,20 @@ func handleAuthRequestForOIDCUpstreamPasswordGrant( fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error()), true, ) } + upstreamSubject, err := downstreamsession.ExtractStringClaimValue(oidc.IDTokenSubjectClaim, oidcUpstream.GetName(), token.IDToken.Claims) + if err != nil { + // Return a user-friendly error for this case which is entirely within our control. + return writeAuthorizeError(w, oauthHelper, authorizeRequester, + fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error()), true, + ) + } + upstreamIssuer, err := downstreamsession.ExtractStringClaimValue(oidc.IDTokenIssuerClaim, oidcUpstream.GetName(), token.IDToken.Claims) + if err != nil { + // Return a user-friendly error for this case which is entirely within our control. + return writeAuthorizeError(w, oauthHelper, authorizeRequester, + fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error()), true, + ) + } customSessionData := &psession.CustomSessionData{ ProviderUID: oidcUpstream.GetResourceUID(), @@ -198,6 +212,8 @@ func handleAuthRequestForOIDCUpstreamPasswordGrant( ProviderType: psession.ProviderTypeOIDC, OIDC: &psession.OIDCSessionData{ UpstreamRefreshToken: token.RefreshToken.Token, + UpstreamIssuer: upstreamIssuer, + UpstreamSubject: upstreamSubject, }, } return makeDownstreamSessionAndReturnAuthcodeRedirect(r, w, oauthHelper, authorizeRequester, subject, username, groups, customSessionData) diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 274ca0e9..845e39b4 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package auth @@ -470,6 +470,8 @@ func TestAuthorizationEndpoint(t *testing.T) { ProviderType: psession.ProviderTypeOIDC, OIDC: &psession.OIDCSessionData{ UpstreamRefreshToken: oidcPasswordGrantUpstreamRefreshToken, + UpstreamSubject: oidcUpstreamSubject, + UpstreamIssuer: oidcUpstreamIssuer, }, } diff --git a/internal/oidc/callback/callback_handler.go b/internal/oidc/callback/callback_handler.go index 22f37a55..1a5a3aee 100644 --- a/internal/oidc/callback/callback_handler.go +++ b/internal/oidc/callback/callback_handler.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package callback provides a handler for the OIDC callback endpoint. @@ -83,12 +83,23 @@ func NewHandler( return httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err) } + upstreamSubject, err := downstreamsession.ExtractStringClaimValue(oidc.IDTokenSubjectClaim, upstreamIDPConfig.GetName(), token.IDToken.Claims) + if err != nil { + return httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err) + } + upstreamIssuer, err := downstreamsession.ExtractStringClaimValue(oidc.IDTokenIssuerClaim, upstreamIDPConfig.GetName(), token.IDToken.Claims) + if err != nil { + return httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err) + } + openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, &psession.CustomSessionData{ ProviderUID: upstreamIDPConfig.GetResourceUID(), ProviderName: upstreamIDPConfig.GetName(), ProviderType: psession.ProviderTypeOIDC, OIDC: &psession.OIDCSessionData{ UpstreamRefreshToken: token.RefreshToken.Token, + UpstreamSubject: upstreamSubject, + UpstreamIssuer: upstreamIssuer, }, }) diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index 48e04afd..d14c159f 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package callback @@ -77,7 +77,11 @@ var ( ProviderUID: happyUpstreamIDPResourceUID, ProviderName: happyUpstreamIDPName, ProviderType: psession.ProviderTypeOIDC, - OIDC: &psession.OIDCSessionData{UpstreamRefreshToken: oidcUpstreamRefreshToken}, + OIDC: &psession.OIDCSessionData{ + UpstreamRefreshToken: oidcUpstreamRefreshToken, + UpstreamIssuer: oidcUpstreamIssuer, + UpstreamSubject: oidcUpstreamSubject, + }, } ) diff --git a/internal/oidc/downstreamsession/downstream_session.go b/internal/oidc/downstreamsession/downstream_session.go index 5f0cf5f0..265ed5c1 100644 --- a/internal/oidc/downstreamsession/downstream_session.go +++ b/internal/oidc/downstreamsession/downstream_session.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package downstreamsession provides some shared helpers for creating downstream OIDC sessions. @@ -89,11 +89,11 @@ func getSubjectAndUsernameFromUpstreamIDToken( ) (string, string, error) { // The spec says the "sub" claim is only unique per issuer, // so we will prepend the issuer string to make it globally unique. - upstreamIssuer, err := extractStringClaimValue(oidc.IDTokenIssuerClaim, upstreamIDPConfig.GetName(), idTokenClaims) + upstreamIssuer, err := ExtractStringClaimValue(oidc.IDTokenIssuerClaim, upstreamIDPConfig.GetName(), idTokenClaims) if err != nil { return "", "", err } - upstreamSubject, err := extractStringClaimValue(oidc.IDTokenSubjectClaim, upstreamIDPConfig.GetName(), idTokenClaims) + upstreamSubject, err := ExtractStringClaimValue(oidc.IDTokenSubjectClaim, upstreamIDPConfig.GetName(), idTokenClaims) if err != nil { return "", "", err } @@ -128,7 +128,7 @@ func getSubjectAndUsernameFromUpstreamIDToken( } } - username, err := extractStringClaimValue(usernameClaimName, upstreamIDPConfig.GetName(), idTokenClaims) + username, err := ExtractStringClaimValue(usernameClaimName, upstreamIDPConfig.GetName(), idTokenClaims) if err != nil { return "", "", err } @@ -136,7 +136,7 @@ func getSubjectAndUsernameFromUpstreamIDToken( return subject, username, nil } -func extractStringClaimValue(claimName string, upstreamIDPName string, idTokenClaims map[string]interface{}) (string, error) { +func ExtractStringClaimValue(claimName string, upstreamIDPName string, idTokenClaims map[string]interface{}) (string, error) { value, ok := idTokenClaims[claimName] if !ok { plog.Warning( diff --git a/internal/oidc/provider/dynamic_upstream_idp_provider.go b/internal/oidc/provider/dynamic_upstream_idp_provider.go index 9986f018..24b821e7 100644 --- a/internal/oidc/provider/dynamic_upstream_idp_provider.go +++ b/internal/oidc/provider/dynamic_upstream_idp_provider.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package provider @@ -71,7 +71,7 @@ type UpstreamOIDCIdentityProviderI interface { // RevokeRefreshToken will attempt to revoke the given token, if the provider has a revocation endpoint. RevokeRefreshToken(ctx context.Context, refreshToken string) error - // ValidateToken will validate the ID token. It will also merge the claims from the userinfo endpoint response + // ValidateTokenAndMergeWithUserInfo will validate the ID token. It will also merge the claims from the userinfo endpoint response // into the ID token's claims, if the provider offers the userinfo endpoint. It returns the validated/updated // tokens, or an error. ValidateTokenAndMergeWithUserInfo(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool) (*oidctypes.Token, error) diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index b1888a18..e0fc4408 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package token provides a handler for the OIDC token endpoint. @@ -17,7 +17,6 @@ import ( "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/psession" - "go.pinniped.dev/internal/upstreamoidc" ) var ( @@ -138,29 +137,27 @@ func upstreamOIDCRefresh(ctx context.Context, session *psession.PinnipedSession, // if we have any claims at all, we better have a subject, and it better match the previous value. // but it's possible that we don't because both returning a new refresh token on refresh and having a userinfo // endpoint are optional. - if len(validatedTokens.IDToken.Claims) != 0 { - newSub := claims["sub"] - oldDownstreamSubject := session.Fosite.Claims.Subject - oldIss, oldSub, err := upstreamoidc.ExtractUpstreamSubjectAndIssuerFromDownstream(oldDownstreamSubject) - if err != nil { - return errorsx.WithStack(errUpstreamRefreshError.WithHintf("Upstream refresh failed."). - WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + if len(validatedTokens.IDToken.Claims) != 0 { //nolint:nestif + newSub, hasSub := getString(claims, oidc.IDTokenSubjectClaim) + if !hasSub { + return errorsx.WithStack(errUpstreamRefreshError.WithHintf( + "Upstream refresh failed.").WithWrap(errors.New("subject in upstream refresh not found")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) } - if oldSub != newSub { + if s.OIDC.UpstreamSubject != newSub { return errorsx.WithStack(errUpstreamRefreshError.WithHintf( "Upstream refresh failed.").WithWrap(errors.New("subject in upstream refresh does not match previous value")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) } usernameClaim := p.GetUsernameClaim() - newUsername := claims[usernameClaim] - oldUsername := session.Fosite.Claims.Extra["username"] + newUsername, hasUsername := getString(claims, usernameClaim) + oldUsername := session.Fosite.Claims.Extra[oidc.DownstreamUsernameClaim] // its possible this won't be returned. // but if it is, verify that it hasn't changed. - if newUsername != nil && oldUsername != newUsername { + if hasUsername && oldUsername != newUsername { return errorsx.WithStack(errUpstreamRefreshError.WithHintf( "Upstream refresh failed.").WithWrap(errors.New("username in upstream refresh does not match previous value")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) } - newIssuer := claims["iss"] - if newIssuer != nil && oldIss != newIssuer { + newIssuer, hasIssuer := getString(claims, oidc.IDTokenIssuerClaim) + if hasIssuer && s.OIDC.UpstreamIssuer != newIssuer { return errorsx.WithStack(errUpstreamRefreshError.WithHintf( "Upstream refresh failed.").WithWrap(errors.New("issuer in upstream refresh does not match previous value")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) } @@ -178,6 +175,11 @@ func upstreamOIDCRefresh(ctx context.Context, session *psession.PinnipedSession, return nil } +func getString(m map[string]interface{}, key string) (string, bool) { + val, ok := m[key].(string) + return val, ok +} + func findOIDCProviderByNameAndValidateUID( s *psession.CustomSessionData, providerCache oidc.UpstreamIdentityProvidersLister, diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index 24fea4ad..2a8f039a 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package token @@ -57,6 +57,7 @@ import ( const ( goodIssuer = "https://some-issuer.com" + goodUpstreamSubject = "some-subject" goodClient = "pinniped-cli" goodRedirectURI = "http://127.0.0.1/callback" goodPKCECodeVerifier = "some-pkce-verifier-that-must-be-at-least-43-characters-to-meet-entropy-requirements" @@ -910,6 +911,8 @@ func TestRefreshGrant(t *testing.T) { ProviderType: oidcUpstreamType, OIDC: &psession.OIDCSessionData{ UpstreamRefreshToken: oidcUpstreamInitialRefreshToken, + UpstreamSubject: goodUpstreamSubject, + UpstreamIssuer: goodIssuer, }, } } @@ -1049,7 +1052,7 @@ func TestRefreshGrant(t *testing.T) { upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ IDToken: &oidctypes.IDToken{ Claims: map[string]interface{}{ - "sub": "some-subject", + "sub": goodUpstreamSubject, }, }, }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), @@ -1072,7 +1075,7 @@ func TestRefreshGrant(t *testing.T) { IDToken: &oidctypes.IDToken{ Claims: map[string]interface{}{ "some-claim": "some-value", - "sub": "some-subject", + "sub": goodUpstreamSubject, "username-claim": goodUsername, }, }, @@ -1146,7 +1149,7 @@ func TestRefreshGrant(t *testing.T) { upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ IDToken: &oidctypes.IDToken{ Claims: map[string]interface{}{ - "sub": "some-subject", + "sub": goodUpstreamSubject, }, }, }).WithRefreshedTokens(refreshedUpstreamTokensWithIDTokenWithoutRefreshToken()).Build()), @@ -1168,7 +1171,7 @@ func TestRefreshGrant(t *testing.T) { upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ IDToken: &oidctypes.IDToken{ Claims: map[string]interface{}{ - "sub": "some-subject", + "sub": goodUpstreamSubject, }, }, }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), @@ -1193,7 +1196,7 @@ func TestRefreshGrant(t *testing.T) { upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ IDToken: &oidctypes.IDToken{ Claims: map[string]interface{}{ - "sub": "some-subject", + "sub": goodUpstreamSubject, }, }, }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), @@ -1229,7 +1232,7 @@ func TestRefreshGrant(t *testing.T) { upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ IDToken: &oidctypes.IDToken{ Claims: map[string]interface{}{ - "sub": "some-subject", + "sub": goodUpstreamSubject, }, }, }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), @@ -1681,7 +1684,7 @@ func TestRefreshGrant(t *testing.T) { IDToken: &oidctypes.IDToken{ Claims: map[string]interface{}{ "some-claim": "some-value", - "sub": "some-subject", + "sub": goodUpstreamSubject, "username-claim": "some-changed-username", }, }, @@ -1712,7 +1715,7 @@ func TestRefreshGrant(t *testing.T) { IDToken: &oidctypes.IDToken{ Claims: map[string]interface{}{ "some-claim": "some-value", - "sub": "some-subject", + "sub": goodUpstreamSubject, "iss": "some-changed-issuer", }, }, diff --git a/internal/psession/pinniped_session.go b/internal/psession/pinniped_session.go index d7fd47df..bbcc4317 100644 --- a/internal/psession/pinniped_session.go +++ b/internal/psession/pinniped_session.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package psession @@ -62,6 +62,8 @@ const ( // OIDCSessionData is the additional data needed by Pinniped when the upstream IDP is an OIDC provider. type OIDCSessionData struct { UpstreamRefreshToken string `json:"upstreamRefreshToken"` + UpstreamSubject string `json:"upstreamSubject"` + UpstreamIssuer string `json:"upstreamIssuer"` } // LDAPSessionData is the additional data needed by Pinniped when the upstream IDP is an LDAP provider. diff --git a/internal/testutil/psession.go b/internal/testutil/psession.go index 28e65968..83aacb13 100644 --- a/internal/testutil/psession.go +++ b/internal/testutil/psession.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package testutil @@ -29,6 +29,8 @@ func NewFakePinnipedSession() *psession.PinnipedSession { ProviderName: "fake-provider-name", OIDC: &psession.OIDCSessionData{ UpstreamRefreshToken: "fake-upstream-refresh-token", + UpstreamSubject: "some-subject", + UpstreamIssuer: "some-issuer", }, }, } diff --git a/internal/upstreamoidc/upstreamoidc.go b/internal/upstreamoidc/upstreamoidc.go index f9256f23..29dd8cac 100644 --- a/internal/upstreamoidc/upstreamoidc.go +++ b/internal/upstreamoidc/upstreamoidc.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package upstreamoidc implements an abstraction of upstream OIDC provider interactions. @@ -7,7 +7,6 @@ package upstreamoidc import ( "context" "encoding/json" - "errors" "fmt" "io" "net/http" @@ -238,53 +237,19 @@ func (p *ProviderConfig) tryRevokeRefreshToken( } } -func ExtractUpstreamSubjectAndIssuerFromDownstream(downstreamSubject string) (string, string, error) { - if !strings.Contains(downstreamSubject, "?sub=") { - return "", "", errors.New("downstream subject did not contain original upstream subject") - } - split := strings.SplitN(downstreamSubject, "?sub=", 2) - iss := split[0] - sub := split[1] - if iss == "" || sub == "" { - return "", "", errors.New("downstream subject was malformed") - } - return split[0], split[1], nil -} - // ValidateTokenAndMergeWithUserInfo will validate the ID token. It will also merge the claims from the userinfo endpoint response, // if the provider offers the userinfo endpoint. func (p *ProviderConfig) ValidateTokenAndMergeWithUserInfo(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool) (*oidctypes.Token, error) { var validatedClaims = make(map[string]interface{}) - idTok, hasIDTok := tok.Extra("id_token").(string) var idTokenExpiry time.Time // if we require the id token, make sure we have it. // also, if it exists but wasn't required, still make sure it passes these checks. - // nolint:nestif - if hasIDTok || requireIDToken { - if !hasIDTok { - return nil, httperr.New(http.StatusBadRequest, "received response missing ID token") - } - validated, err := p.Provider.Verifier(&coreosoidc.Config{ClientID: p.GetClientID()}).Verify(coreosoidc.ClientContext(ctx, p.Client), idTok) - if err != nil { - return nil, httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err) - } - if validated.AccessTokenHash != "" { - if err := validated.VerifyAccessToken(tok.AccessToken); err != nil { - return nil, httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err) - } - } - if expectedIDTokenNonce != "" { - if err := expectedIDTokenNonce.Validate(validated); err != nil { - return nil, httperr.Wrap(http.StatusBadRequest, "received ID token with invalid nonce", err) - } - } - if err := validated.Claims(&validatedClaims); err != nil { - return nil, httperr.Wrap(http.StatusInternalServerError, "could not unmarshal id token claims", err) - } - maybeLogClaims("claims from ID token", p.Name, validatedClaims) - idTokenExpiry = validated.Expiry // keep track of the id token expiry if we have an id token. Otherwise, it'll just be the zero value. + idTokenExpiry, idTok, err := p.validateIDToken(ctx, tok, expectedIDTokenNonce, validatedClaims, requireIDToken) + if err != nil { + return nil, err } + idTokenSubject, _ := validatedClaims[oidc.IDTokenSubjectClaim].(string) if len(idTokenSubject) > 0 || !requireIDToken { @@ -310,10 +275,42 @@ func (p *ProviderConfig) ValidateTokenAndMergeWithUserInfo(ctx context.Context, }, nil } +func (p *ProviderConfig) validateIDToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, validatedClaims map[string]interface{}, requireIDToken bool) (time.Time, string, error) { + idTok, hasIDTok := tok.Extra("id_token").(string) + if !hasIDTok && !requireIDToken { + return time.Time{}, "", nil // exit early + } + + var idTokenExpiry time.Time + if !hasIDTok { + return time.Time{}, "", httperr.New(http.StatusBadRequest, "received response missing ID token") + } + validated, err := p.Provider.Verifier(&coreosoidc.Config{ClientID: p.GetClientID()}).Verify(coreosoidc.ClientContext(ctx, p.Client), idTok) + if err != nil { + return time.Time{}, "", httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err) + } + if validated.AccessTokenHash != "" { + if err := validated.VerifyAccessToken(tok.AccessToken); err != nil { + return time.Time{}, "", httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err) + } + } + if expectedIDTokenNonce != "" { + if err := expectedIDTokenNonce.Validate(validated); err != nil { + return time.Time{}, "", httperr.Wrap(http.StatusBadRequest, "received ID token with invalid nonce", err) + } + } + if err := validated.Claims(&validatedClaims); err != nil { + return time.Time{}, "", httperr.Wrap(http.StatusInternalServerError, "could not unmarshal id token claims", err) + } + maybeLogClaims("claims from ID token", p.Name, validatedClaims) + idTokenExpiry = validated.Expiry // keep track of the id token expiry if we have an id token. Otherwise, it'll just be the zero value. + return idTokenExpiry, idTok, nil +} + func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, tok *oauth2.Token, claims map[string]interface{}, requireIDToken bool) error { idTokenSubject, _ := claims[oidc.IDTokenSubjectClaim].(string) - userInfo, err := p.fetchUserInfo(ctx, tok) + userInfo, err := p.maybeFetchUserInfo(ctx, tok) if err != nil { return err } @@ -356,7 +353,7 @@ func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, t return nil } -func (p *ProviderConfig) fetchUserInfo(ctx context.Context, tok *oauth2.Token) (*coreosoidc.UserInfo, error) { +func (p *ProviderConfig) maybeFetchUserInfo(ctx context.Context, tok *oauth2.Token) (*coreosoidc.UserInfo, error) { providerJSON := &struct { UserInfoURL string `json:"userinfo_endpoint"` }{} diff --git a/internal/upstreamoidc/upstreamoidc_test.go b/internal/upstreamoidc/upstreamoidc_test.go index e5b90c80..90dc6f1f 100644 --- a/internal/upstreamoidc/upstreamoidc_test.go +++ b/internal/upstreamoidc/upstreamoidc_test.go @@ -910,63 +910,6 @@ func TestProviderConfig(t *testing.T) { } }) - t.Run("ExtractUpstreamSubjectAndIssuerFromDownstream", func(t *testing.T) { - tests := []struct { - name string - downstreamSubject string - wantUpstreamSubject string - wantUpstreamIssuer string - wantErr string - }{ - { - name: "happy path", - downstreamSubject: "https://some-issuer?sub=some-subject", - wantUpstreamSubject: "some-subject", - wantUpstreamIssuer: "https://some-issuer", - }, - { - name: "subject in a subject", - downstreamSubject: "https://some-other-issuer?sub=https://some-issuer?sub=some-subject", - wantUpstreamSubject: "https://some-issuer?sub=some-subject", - wantUpstreamIssuer: "https://some-other-issuer", - }, - { - name: "sub is empty string", - downstreamSubject: "https://some-issuer?sub=", - wantErr: "downstream subject was malformed", - }, - { - name: "iss is empty string", - downstreamSubject: "?sub=some-subject", - wantErr: "downstream subject was malformed", - }, - { - name: "empty string", - downstreamSubject: "", - wantErr: "downstream subject did not contain original upstream subject", - }, - { - name: "doesn't contain sub=", - downstreamSubject: "something-invalid", - wantErr: "downstream subject did not contain original upstream subject", - }, - } - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - actualUpstreamIssuer, actualUpstreamSubject, err := ExtractUpstreamSubjectAndIssuerFromDownstream(tt.downstreamSubject) - if tt.wantErr != "" { - require.Error(t, err) - require.Equal(t, tt.wantErr, err.Error()) - } else { - require.NoError(t, err) - require.Equal(t, tt.wantUpstreamSubject, actualUpstreamSubject) - require.Equal(t, tt.wantUpstreamIssuer, actualUpstreamIssuer) - } - }) - } - }) - t.Run("ExchangeAuthcodeAndValidateTokens", func(t *testing.T) { tests := []struct { name string diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index db2f617e..29a46e79 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package integration @@ -87,8 +87,8 @@ func TestSupervisorLogin(t *testing.T) { }, requestAuthorization: requestAuthorizationUsingBrowserAuthcodeFlow, breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession, _, _ string) { - fositeSessionData := pinnipedSession.Fosite - fositeSessionData.Claims.Subject = "wrong-subject" + pinnipedSessionData := pinnipedSession.Custom + pinnipedSessionData.OIDC.UpstreamIssuer = "wrong-issuer" }, // the ID token Subject should include the upstream user ID after the upstream issuer name wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Issuer+"?sub=") + ".+", From 2b744b2eef9d7da6fd643e53bad70d1d2f221fcf Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Wed, 12 Jan 2022 11:19:43 -0800 Subject: [PATCH 8/8] Add back comment about deferring validation when id token subject is missing --- internal/upstreamoidc/upstreamoidc.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/upstreamoidc/upstreamoidc.go b/internal/upstreamoidc/upstreamoidc.go index 29dd8cac..b9d371ed 100644 --- a/internal/upstreamoidc/upstreamoidc.go +++ b/internal/upstreamoidc/upstreamoidc.go @@ -253,6 +253,8 @@ func (p *ProviderConfig) ValidateTokenAndMergeWithUserInfo(ctx context.Context, idTokenSubject, _ := validatedClaims[oidc.IDTokenSubjectClaim].(string) if len(idTokenSubject) > 0 || !requireIDToken { + // only fetch userinfo if the ID token has a subject or if we are ignoring the id token completely. + // otherwise, defer to existing ID token validation if err := p.maybeFetchUserInfoAndMergeClaims(ctx, tok, validatedClaims, requireIDToken); err != nil { return nil, httperr.Wrap(http.StatusInternalServerError, "could not fetch user info claims", err) }