From ed96b597c7ca56903eef23be5f5846fdaf1cac95 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Mon, 29 Nov 2021 16:44:58 -0800 Subject: [PATCH] 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) }