Fetch claims from the user info endpoint if provided

Signed-off-by: Monis Khan <mok@vmware.com>

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
+}

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
+}