From 6fff179e391dccd5127068d800da5bb170402a01 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Sat, 9 Jan 2021 14:27:35 -0500 Subject: [PATCH] Fetch claims from the user info endpoint if provided Signed-off-by: Monis Khan --- internal/upstreamoidc/upstreamoidc.go | 55 ++++++++- internal/upstreamoidc/upstreamoidc_test.go | 136 ++++++++++++++++++++- 2 files changed, 183 insertions(+), 8 deletions(-) diff --git a/internal/upstreamoidc/upstreamoidc.go b/internal/upstreamoidc/upstreamoidc.go index af7c9a7c..9e825a3d 100644 --- a/internal/upstreamoidc/upstreamoidc.go +++ b/internal/upstreamoidc/upstreamoidc.go @@ -9,18 +9,19 @@ import ( "net/http" "net/url" - "github.com/coreos/go-oidc" + coreosoidc "github.com/coreos/go-oidc" "golang.org/x/oauth2" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "go.pinniped.dev/internal/httputil/httperr" + "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/pkg/oidcclient/nonce" "go.pinniped.dev/pkg/oidcclient/oidctypes" "go.pinniped.dev/pkg/oidcclient/pkce" ) -func New(config *oauth2.Config, provider *oidc.Provider, client *http.Client) provider.UpstreamOIDCIdentityProviderI { +func New(config *oauth2.Config, provider *coreosoidc.Provider, client *http.Client) provider.UpstreamOIDCIdentityProviderI { return &ProviderConfig{Config: config, Provider: provider, Client: client} } @@ -31,7 +32,8 @@ type ProviderConfig struct { GroupsClaim string Config *oauth2.Config Provider interface { - Verifier(*oidc.Config) *oidc.IDTokenVerifier + Verifier(*coreosoidc.Config) *coreosoidc.IDTokenVerifier + UserInfo(ctx context.Context, tokenSource oauth2.TokenSource) (*coreosoidc.UserInfo, error) } Client *http.Client } @@ -63,7 +65,7 @@ func (p *ProviderConfig) GetGroupsClaim() string { func (p *ProviderConfig) ExchangeAuthcodeAndValidateTokens(ctx context.Context, authcode string, pkceCodeVerifier pkce.Code, expectedIDTokenNonce nonce.Nonce, redirectURI string) (*oidctypes.Token, error) { tok, err := p.Config.Exchange( - oidc.ClientContext(ctx, p.Client), + coreosoidc.ClientContext(ctx, p.Client), authcode, pkceCodeVerifier.Verifier(), oauth2.SetAuthURLParam("redirect_uri", redirectURI), @@ -80,7 +82,7 @@ func (p *ProviderConfig) ValidateToken(ctx context.Context, tok *oauth2.Token, e if !hasIDTok { return nil, httperr.New(http.StatusBadRequest, "received response missing ID token") } - validated, err := p.Provider.Verifier(&oidc.Config{ClientID: p.GetClientID()}).Verify(oidc.ClientContext(ctx, p.Client), idTok) + 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) } @@ -97,7 +99,11 @@ func (p *ProviderConfig) ValidateToken(ctx context.Context, tok *oauth2.Token, e var validatedClaims map[string]interface{} if err := validated.Claims(&validatedClaims); err != nil { - return nil, httperr.Wrap(http.StatusInternalServerError, "could not unmarshal claims", err) + return nil, httperr.Wrap(http.StatusInternalServerError, "could not unmarshal id token claims", err) + } + + if err := p.fetchUserInfo(ctx, tok, validatedClaims); err != nil { + return nil, httperr.Wrap(http.StatusInternalServerError, "could not fetch user info claims", err) } return &oidctypes.Token{ @@ -116,3 +122,40 @@ func (p *ProviderConfig) ValidateToken(ctx context.Context, tok *oauth2.Token, e }, }, nil } + +func (p *ProviderConfig) fetchUserInfo(ctx context.Context, tok *oauth2.Token, claims map[string]interface{}) error { + idTokenSubject, _ := claims[oidc.IDTokenSubjectClaim].(string) + if len(idTokenSubject) == 0 { + return nil // defer to existing ID token validation + } + + userInfo, err := p.Provider.UserInfo(coreosoidc.ClientContext(ctx, p.Client), oauth2.StaticTokenSource(tok)) + if err != nil { + // the user info endpoint is not required but we do not have a good way to probe if it was provided + const userInfoUnsupported = "oidc: user info endpoint is not supported by this provider" + if err.Error() == userInfoUnsupported { + return nil + } + + return httperr.Wrap(http.StatusInternalServerError, "could not get user info", err) + } + + // 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 + // 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 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) + } + + // 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) + } + + return nil +} diff --git a/internal/upstreamoidc/upstreamoidc_test.go b/internal/upstreamoidc/upstreamoidc_test.go index 946f618b..53061b23 100644 --- a/internal/upstreamoidc/upstreamoidc_test.go +++ b/internal/upstreamoidc/upstreamoidc_test.go @@ -6,10 +6,14 @@ package upstreamoidc import ( "context" "encoding/json" + "errors" + "fmt" "net/http" "net/http/httptest" + "reflect" "testing" "time" + "unsafe" "github.com/coreos/go-oidc" "github.com/golang/mock/gomock" @@ -52,10 +56,16 @@ func TestProviderConfig(t *testing.T) { // step crypto keypair key.pub key.priv --kty RSA --no-password --insecure --force && echo '{"nonce": "invalid-nonce"}' | step crypto jwt sign --key key.priv --aud test-client-id --sub test-user --subtle --kid="test-kid" --jti="test-jti" invalidNonceIDToken = "eyJhbGciOiJSUzI1NiIsImtpZCI6InRlc3Qta2lkIiwidHlwIjoiSldUIn0.eyJhdWQiOiJ0ZXN0LWNsaWVudC1pZCIsImlhdCI6MTYwMjI4Mzc0MSwianRpIjoidGVzdC1qdGkiLCJuYmYiOjE2MDIyODM3NDEsIm5vbmNlIjoiaW52YWxpZC1ub25jZSIsInN1YiI6InRlc3QtdXNlciJ9.PRpq-7j5djaIAkraL-8t8ad9Xm4hM8RW67gyD1VIe0BecWeBFxsTuh3SZVKM9zmcwTgjudsyn8kQOwipDa49IN4PV8FcJA_uUJZi2wiqGJUSTG2K5I89doV_7e0RM1ZYIDDW1G2heKJNW7MbKkX7iEPr7u4MyEzswcPcupbyDA-CQFeL95vgwawoqa6yO94ympTbozqiNfj6Xyw_nHtThQnstjWsJZ9s2mUgppZezZv4HZYTQ7c3e_bzwhWgCzh2CSDJn9_Ra_n_4GcVkpHbsHTP35dFsnf0vactPx6CAu6A1-Apk-BruCktpZ3B4Ercf1UnUOHdGqzQKJtqvB03xQ" //nolint: gosec + // step crypto keypair key.pub key.priv --kty RSA --no-password --insecure --force && echo '{"foo": "bar", "bat": "baz"}' | step crypto jwt sign --key key.priv --aud test-client-id --sub '' --subtle --kid="test-kid" --jti="test-jti" + invalidSubClaim = "eyJhbGciOiJSUzI1NiIsImtpZCI6InRlc3Qta2lkIiwidHlwIjoiSldUIn0.eyJhdWQiOiJ0ZXN0LWNsaWVudC1pZCIsImJhdCI6ImJheiIsImZvbyI6ImJhciIsImlhdCI6MTYxMDIxOTY5MCwianRpIjoidGVzdC1qdGkiLCJuYmYiOjE2MTAyMTk2OTB9.CXgUarh9A8QByF_ddw0W1Cldl_n1qmry2cZh9U0Avi5sl7hb1y22MadDLQslvnx0NKx6EdbwI-El7QxDy0SzwomJomFL7WNd5gGk-Ilq9O_emaHekbpphZ5kxyudsAGUYGxrg1zysv1k5JPhnLnOUMcE7wa0uPLDWnrlAMzqHvnbjI3lakZ8v4-dfAKUIUGi3ycwuAh9BdpydwAsSNOpGBM55-O8911dqVfZKiFNNUeHYE1qlnbhCz7_ykLrljao0nRBbEf9FXGolCdhIaglt0LtaZvll9T9StIbSpcRaBGuRm8toTezmhmHjU-iCc0jGeVKsp8eTyOuJllqDSS-uw" + // step crypto keypair key.pub key.priv --kty RSA --no-password --insecure --force && echo '{"foo": "bar", "bat": "baz"}' | step crypto jwt sign --key key.priv --aud test-client-id --sub test-user --subtle --kid="test-kid" --jti="test-jti" validIDToken = "eyJhbGciOiJSUzI1NiIsImtpZCI6InRlc3Qta2lkIiwidHlwIjoiSldUIn0.eyJhdWQiOiJ0ZXN0LWNsaWVudC1pZCIsImJhdCI6ImJheiIsImZvbyI6ImJhciIsImlhdCI6MTYwNjc2ODU5MywianRpIjoidGVzdC1qdGkiLCJuYmYiOjE2MDY3Njg1OTMsInN1YiI6InRlc3QtdXNlciJ9.DuqVZ7pGhHqKz7gNr4j2W1s1N8YrSltktH4wW19L4oD1OE2-O72jAnNj5xdjilsa8l7h9ox-5sMF0Tkh3BdRlHQK9dEtNm9tW-JreUnWJ3LCqUs-LZp4NG7edvq2sH_1Bn7O2_NQV51s8Pl04F60CndjQ4NM-6WkqDQTKyY6vJXU7idvM-6TM2HJZK-Na88cOJ9KIK37tL5DhcbsHVF47Dq8uPZ0KbjNQjJLAIi_1GeQBgc6yJhDUwRY4Xu6S0dtTHA6xTI8oSXoamt4bkViEHfJBp97LZQiNz8mku5pVc0aNwP1p4hMHxRHhLXrJjbh-Hx4YFjxtOnIq9t1mHlD4A" //nolint: gosec ) + // if the error string for unsupported user info changes, this will hopefully catch it + _, userInfoNotSupported := (&oidc.Provider{}).UserInfo(context.Background(), nil) + tests := []struct { name string authCode string @@ -63,6 +73,10 @@ func TestProviderConfig(t *testing.T) { returnIDTok string wantErr string wantToken oidctypes.Token + + userInfo *oidc.UserInfo + userInfoErr error + wantUserInfoCalled bool }{ { name: "exchange fails with network error", @@ -119,6 +133,8 @@ func TestProviderConfig(t *testing.T) { }, }, }, + userInfoErr: userInfoNotSupported, + wantUserInfoCalled: true, }, { name: "valid", @@ -146,6 +162,89 @@ func TestProviderConfig(t *testing.T) { }, }, }, + userInfoErr: userInfoNotSupported, + wantUserInfoCalled: true, + }, + { + name: "user info fetch error", + authCode: "valid", + returnIDTok: validIDToken, + wantErr: "could not fetch user info claims: could not get user info: some network error", + userInfoErr: errors.New("some network error"), + }, + { + name: "user info sub error", + authCode: "valid", + returnIDTok: validIDToken, + wantErr: "could not fetch user info claims: userinfo 'sub' claim (test-user-2) did not match id_token 'sub' claim (test-user)", + userInfo: &oidc.UserInfo{Subject: "test-user-2"}, + }, + { + name: "user info is not json", + authCode: "valid", + returnIDTok: validIDToken, + wantErr: "could not fetch user info claims: could not unmarshal user info claims: invalid character 'i' looking for beginning of value", + // claims is private field so we have to use hacks to set it + userInfo: forceUserInfoWithClaims("test-user", `invalid-json-data`), + }, + { + name: "valid with user info", + authCode: "valid", + returnIDTok: validIDToken, + wantToken: oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Expiry: metav1.Time{}, + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: validIDToken, + Expiry: metav1.Time{}, + Claims: map[string]interface{}{ + "foo": "awesomeness", // overwrite existing claim + "bat": "baz", + "aud": "test-client-id", + "iat": 1.606768593e+09, + "jti": "test-jti", + "nbf": 1.606768593e+09, + "sub": "test-user", + "groups": "fancy-group", // add a new claim + }, + }, + }, + // claims is private field so we have to use hacks to set it + userInfo: forceUserInfoWithClaims("test-user", `{"foo":"awesomeness","groups":"fancy-group"}`), + wantUserInfoCalled: true, + }, + { + name: "invalid sub claim", + authCode: "valid", + returnIDTok: invalidSubClaim, + wantToken: oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{ + Token: "test-access-token", + Expiry: metav1.Time{}, + }, + RefreshToken: &oidctypes.RefreshToken{ + Token: "test-refresh-token", + }, + IDToken: &oidctypes.IDToken{ + Token: invalidSubClaim, + Expiry: metav1.Time{}, + Claims: map[string]interface{}{ + "foo": "bar", + "bat": "baz", + "aud": "test-client-id", + "iat": 1.61021969e+09, + "jti": "test-jti", + "nbf": 1.61021969e+09, + // no sub claim + }, + }, + }, + wantUserInfoCalled: false, }, } for _, tt := range tests { @@ -188,7 +287,10 @@ func TestProviderConfig(t *testing.T) { }, Scopes: []string{"scope1", "scope2"}, }, - Provider: &mockProvider{}, + Provider: &mockProvider{ + userInfo: tt.userInfo, + userInfoErr: tt.userInfoErr, + }, } ctx := context.Background() @@ -201,6 +303,7 @@ func TestProviderConfig(t *testing.T) { } require.NoError(t, err) require.Equal(t, &tt.wantToken, tok) + require.Equal(t, tt.wantUserInfoCalled, p.Provider.(*mockProvider).called) }) } } @@ -225,6 +328,35 @@ func mockVerifier() *oidc.IDTokenVerifier { }) } -type mockProvider struct{} +type mockProvider struct { + called bool + userInfo *oidc.UserInfo + userInfoErr error +} func (m *mockProvider) Verifier(_ *oidc.Config) *oidc.IDTokenVerifier { return mockVerifier() } + +func (m *mockProvider) UserInfo(_ context.Context, tokenSource oauth2.TokenSource) (*oidc.UserInfo, error) { + m.called = true + + token, err := tokenSource.Token() + if err != nil { + return nil, err + } + if wantToken := "test-access-token"; wantToken != token.AccessToken { + return nil, fmt.Errorf("want token = %#v, got token = %#v", wantToken, token) + } + + return m.userInfo, m.userInfoErr +} + +func forceUserInfoWithClaims(subject string, claims string) *oidc.UserInfo { + userInfo := &oidc.UserInfo{Subject: subject} + + // this is some dark magic to set a private field + claimsField := reflect.ValueOf(userInfo).Elem().FieldByName("claims") + claimsPointer := (*[]byte)(unsafe.Pointer(claimsField.UnsafeAddr())) + *claimsPointer = []byte(claims) + + return userInfo +}