From 514db45d1a204d367c81c25f78707f619fd29dfa Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Mon, 27 Jul 2020 14:51:29 -0700 Subject: [PATCH 1/3] Allow OIDC Bearer Tokens without emails This reverts to functionality before #499 where an OIDC provider could be used with `--skip-jwt-bearer-tokens` and tokens without an email or profileURL would still be valid. This logic mirrors `middleware.createSessionStateFromBearerToken` which used to be the universal logic before #499. --- CHANGELOG.md | 1 + providers/oidc.go | 4 +-- providers/oidc_test.go | 76 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5361b2be..03e5de7e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ - [#718](https://github.com/oauth2-proxy/oauth2-proxy/pull/718) Allow Logging to stdout with separate Error Log Channel - [#690](https://github.com/oauth2-proxy/oauth2-proxy/pull/690) Address GoSec security findings & remediate (@NickMeves) - [#689](https://github.com/oauth2-proxy/oauth2-proxy/pull/689) Fix finicky logging_handler_test from time drift (@NickMeves) +- [#700](https://github.com/oauth2-proxy/oauth2-proxy/pull/700) Allow OIDC Bearer auth IDTokens to have empty email claim & profile URL (@NickMeves) - [#699](https://github.com/oauth2-proxy/oauth2-proxy/pull/699) Align persistence ginkgo tests with conventions (@NickMeves) - [#696](https://github.com/oauth2-proxy/oauth2-proxy/pull/696) Preserve query when building redirect - [#561](https://github.com/oauth2-proxy/oauth2-proxy/pull/561) Refactor provider URLs to package level vars (@JoelSpeed) diff --git a/providers/oidc.go b/providers/oidc.go index e456db76..1255b4ac 100644 --- a/providers/oidc.go +++ b/providers/oidc.go @@ -231,7 +231,6 @@ func getOIDCHeader(accessToken string) http.Header { } func (p *OIDCProvider) findClaimsFromIDToken(ctx context.Context, idToken *oidc.IDToken, accessToken string, profileURL string) (*OIDCClaims, error) { - claims := &OIDCClaims{} // Extract default claims. if err := idToken.Claims(&claims); err != nil { @@ -250,7 +249,8 @@ func (p *OIDCProvider) findClaimsFromIDToken(ctx context.Context, idToken *oidc. // userID claim was not present or was empty in the ID Token if claims.UserID == "" { if profileURL == "" { - return nil, fmt.Errorf("id_token did not contain user ID claim (%q)", p.UserIDClaim) + claims.UserID = claims.Subject + return claims, nil } // If the userinfo endpoint profileURL is defined, then there is a chance the userinfo diff --git a/providers/oidc_test.go b/providers/oidc_test.go index 12b62a47..b0268138 100644 --- a/providers/oidc_test.go +++ b/providers/oidc_test.go @@ -60,6 +60,22 @@ var defaultIDToken idTokenClaims = idTokenClaims{ }, } +var minimalIDToken idTokenClaims = idTokenClaims{ + "", + "", + "", + "", + jwt.StandardClaims{ + Audience: "https://test.myapp.com", + ExpiresAt: time.Now().Add(time.Duration(5) * time.Minute).Unix(), + Id: "id-some-id", + IssuedAt: time.Now().Unix(), + Issuer: "https://issuer.example.com", + NotBefore: 0, + Subject: "minimal", + }, +} + type fakeKeySetStub struct{} func (fakeKeySetStub) VerifySignature(_ context.Context, jwt string) (payload []byte, err error) { @@ -253,6 +269,66 @@ func TestOIDCProviderRefreshSessionIfNeededWithIdToken(t *testing.T) { assert.Equal(t, refreshToken, existingSession.RefreshToken) } +func TestCreateSessionStateFromBearerToken(t *testing.T) { + const profileURLEmail = "janed@me.com" + + testCases := map[string]struct { + IDToken idTokenClaims + ProfileURL bool + ExpectedEmail string + }{ + "Default IDToken": { + IDToken: defaultIDToken, + ProfileURL: true, + ExpectedEmail: profileURLEmail, + }, + "Minimal IDToken with no OIDC Profile URL": { + IDToken: minimalIDToken, + ProfileURL: false, + ExpectedEmail: "", + }, + "Minimal IDToken with OIDC Profile URL": { + IDToken: minimalIDToken, + ProfileURL: true, + ExpectedEmail: profileURLEmail, + }, + } + for testName, tc := range testCases { + t.Run(testName, func(t *testing.T) { + jsonResp := []byte(fmt.Sprintf(`{"email":"%s"}`, profileURLEmail)) + server, provider := newTestSetup(jsonResp) + defer server.Close() + if !tc.ProfileURL { + provider.ProfileURL = &url.URL{} + } + + rawIDToken, err := newSignedTestIDToken(tc.IDToken) + assert.NoError(t, err) + + keyset := fakeKeySetStub{} + verifier := oidc.NewVerifier("https://issuer.example.com", keyset, + &oidc.Config{ClientID: "https://test.myapp.com", SkipExpiryCheck: true}) + + idToken, err := verifier.Verify(context.Background(), rawIDToken) + assert.NoError(t, err) + + ss, err := provider.CreateSessionStateFromBearerToken(context.Background(), rawIDToken, idToken) + assert.NoError(t, err) + + if tc.ExpectedEmail != "" { + assert.Equal(t, tc.ExpectedEmail, ss.Email) + assert.NotEqual(t, ss.Email, ss.User) + } else { + assert.Equal(t, tc.IDToken.Subject, ss.Email) + assert.Equal(t, ss.Email, ss.User) + } + assert.Equal(t, rawIDToken, ss.IDToken) + assert.Equal(t, rawIDToken, ss.AccessToken) + assert.Equal(t, "", ss.RefreshToken) + }) + } +} + func TestOIDCProvider_findVerifiedIdToken(t *testing.T) { server, provider := newTestSetup([]byte("")) From dcc75410a80a206212735ef24bc0f6ea8005fe40 Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Tue, 28 Jul 2020 09:00:27 -0700 Subject: [PATCH 2/3] Handle claim finding differently in bearer vs standard IDTokens --- providers/oidc.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/providers/oidc.go b/providers/oidc.go index 1255b4ac..acf48f55 100644 --- a/providers/oidc.go +++ b/providers/oidc.go @@ -157,7 +157,7 @@ func (p *OIDCProvider) createSessionState(ctx context.Context, token *oauth2.Tok newSession = &sessions.SessionState{} } else { var err error - newSession, err = p.createSessionStateInternal(ctx, token.Extra("id_token").(string), idToken, token) + newSession, err = p.createSessionStateInternal(ctx, token.Extra("id_token").(string), idToken, token, false) if err != nil { return nil, err } @@ -172,7 +172,7 @@ func (p *OIDCProvider) createSessionState(ctx context.Context, token *oauth2.Tok } func (p *OIDCProvider) CreateSessionStateFromBearerToken(ctx context.Context, rawIDToken string, idToken *oidc.IDToken) (*sessions.SessionState, error) { - newSession, err := p.createSessionStateInternal(ctx, rawIDToken, idToken, nil) + newSession, err := p.createSessionStateInternal(ctx, rawIDToken, idToken, nil, true) if err != nil { return nil, err } @@ -185,7 +185,7 @@ func (p *OIDCProvider) CreateSessionStateFromBearerToken(ctx context.Context, ra return newSession, nil } -func (p *OIDCProvider) createSessionStateInternal(ctx context.Context, rawIDToken string, idToken *oidc.IDToken, token *oauth2.Token) (*sessions.SessionState, error) { +func (p *OIDCProvider) createSessionStateInternal(ctx context.Context, rawIDToken string, idToken *oidc.IDToken, token *oauth2.Token, bearer bool) (*sessions.SessionState, error) { newSession := &sessions.SessionState{} @@ -197,7 +197,7 @@ func (p *OIDCProvider) createSessionStateInternal(ctx context.Context, rawIDToke accessToken = token.AccessToken } - claims, err := p.findClaimsFromIDToken(ctx, idToken, accessToken, p.ProfileURL.String()) + claims, err := p.findClaimsFromIDToken(ctx, idToken, accessToken, p.ProfileURL.String(), bearer) if err != nil { return nil, fmt.Errorf("couldn't extract claims from id_token (%v)", err) } @@ -230,7 +230,7 @@ func getOIDCHeader(accessToken string) http.Header { return header } -func (p *OIDCProvider) findClaimsFromIDToken(ctx context.Context, idToken *oidc.IDToken, accessToken string, profileURL string) (*OIDCClaims, error) { +func (p *OIDCProvider) findClaimsFromIDToken(ctx context.Context, idToken *oidc.IDToken, accessToken string, profileURL string, bearer bool) (*OIDCClaims, error) { claims := &OIDCClaims{} // Extract default claims. if err := idToken.Claims(&claims); err != nil { @@ -249,8 +249,11 @@ func (p *OIDCProvider) findClaimsFromIDToken(ctx context.Context, idToken *oidc. // userID claim was not present or was empty in the ID Token if claims.UserID == "" { if profileURL == "" { - claims.UserID = claims.Subject - return claims, nil + if bearer { + claims.UserID = claims.Subject + return claims, nil + } + return nil, fmt.Errorf("id_token did not contain user ID claim (%q)", p.UserIDClaim) } // If the userinfo endpoint profileURL is defined, then there is a chance the userinfo From 0645e19c240984259d0d0a44ef46f8c24db9530c Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Sun, 9 Aug 2020 18:18:02 -0700 Subject: [PATCH 3/3] Cleanup internalSession params & handle profileURL Bearer case better `findClaimsFromIDToken` would always have a `nil` access token and not be able to hit the userinfo endpoint in Bearer case. If access token is nil, default to legacy `session.Email = claim.Subject` that all JWT bearers used to have, even if a valid profileURL is present. --- providers/oidc.go | 34 ++++++++++++++++++---------------- providers/oidc_test.go | 29 ++++++++--------------------- 2 files changed, 26 insertions(+), 37 deletions(-) diff --git a/providers/oidc.go b/providers/oidc.go index acf48f55..8f2f02b9 100644 --- a/providers/oidc.go +++ b/providers/oidc.go @@ -157,7 +157,7 @@ func (p *OIDCProvider) createSessionState(ctx context.Context, token *oauth2.Tok newSession = &sessions.SessionState{} } else { var err error - newSession, err = p.createSessionStateInternal(ctx, token.Extra("id_token").(string), idToken, token, false) + newSession, err = p.createSessionStateInternal(ctx, idToken, token) if err != nil { return nil, err } @@ -172,7 +172,7 @@ func (p *OIDCProvider) createSessionState(ctx context.Context, token *oauth2.Tok } func (p *OIDCProvider) CreateSessionStateFromBearerToken(ctx context.Context, rawIDToken string, idToken *oidc.IDToken) (*sessions.SessionState, error) { - newSession, err := p.createSessionStateInternal(ctx, rawIDToken, idToken, nil, true) + newSession, err := p.createSessionStateInternal(ctx, idToken, nil) if err != nil { return nil, err } @@ -185,24 +185,22 @@ func (p *OIDCProvider) CreateSessionStateFromBearerToken(ctx context.Context, ra return newSession, nil } -func (p *OIDCProvider) createSessionStateInternal(ctx context.Context, rawIDToken string, idToken *oidc.IDToken, token *oauth2.Token, bearer bool) (*sessions.SessionState, error) { +func (p *OIDCProvider) createSessionStateInternal(ctx context.Context, idToken *oidc.IDToken, token *oauth2.Token) (*sessions.SessionState, error) { newSession := &sessions.SessionState{} if idToken == nil { return newSession, nil } - accessToken := "" - if token != nil { - accessToken = token.AccessToken - } - claims, err := p.findClaimsFromIDToken(ctx, idToken, accessToken, p.ProfileURL.String(), bearer) + claims, err := p.findClaimsFromIDToken(ctx, idToken, token) if err != nil { return nil, fmt.Errorf("couldn't extract claims from id_token (%v)", err) } - newSession.IDToken = rawIDToken + if token != nil { + newSession.IDToken = token.Extra("id_token").(string) + } newSession.Email = claims.UserID // TODO Rename SessionState.Email to .UserID in the near future @@ -230,7 +228,7 @@ func getOIDCHeader(accessToken string) http.Header { return header } -func (p *OIDCProvider) findClaimsFromIDToken(ctx context.Context, idToken *oidc.IDToken, accessToken string, profileURL string, bearer bool) (*OIDCClaims, error) { +func (p *OIDCProvider) findClaimsFromIDToken(ctx context.Context, idToken *oidc.IDToken, token *oauth2.Token) (*OIDCClaims, error) { claims := &OIDCClaims{} // Extract default claims. if err := idToken.Claims(&claims); err != nil { @@ -248,11 +246,15 @@ func (p *OIDCProvider) findClaimsFromIDToken(ctx context.Context, idToken *oidc. // userID claim was not present or was empty in the ID Token if claims.UserID == "" { - if profileURL == "" { - if bearer { - claims.UserID = claims.Subject - return claims, nil - } + // BearerToken case, allow empty UserID + // ProfileURL checks below won't work since we don't have an access token + if token == nil { + claims.UserID = claims.Subject + return claims, nil + } + + profileURL := p.ProfileURL.String() + if profileURL == "" || token.AccessToken == "" { return nil, fmt.Errorf("id_token did not contain user ID claim (%q)", p.UserIDClaim) } @@ -261,7 +263,7 @@ func (p *OIDCProvider) findClaimsFromIDToken(ctx context.Context, idToken *oidc. // Make a query to the userinfo endpoint, and attempt to locate the email from there. respJSON, err := requests.New(profileURL). WithContext(ctx). - WithHeaders(getOIDCHeader(accessToken)). + WithHeaders(getOIDCHeader(token.AccessToken)). Do(). UnmarshalJSON() if err != nil { diff --git a/providers/oidc_test.go b/providers/oidc_test.go index b0268138..9e96752d 100644 --- a/providers/oidc_test.go +++ b/providers/oidc_test.go @@ -274,23 +274,18 @@ func TestCreateSessionStateFromBearerToken(t *testing.T) { testCases := map[string]struct { IDToken idTokenClaims - ProfileURL bool + ExpectedUser string ExpectedEmail string }{ "Default IDToken": { IDToken: defaultIDToken, - ProfileURL: true, - ExpectedEmail: profileURLEmail, + ExpectedUser: defaultIDToken.Subject, + ExpectedEmail: defaultIDToken.Email, }, - "Minimal IDToken with no OIDC Profile URL": { + "Minimal IDToken with no email claim": { IDToken: minimalIDToken, - ProfileURL: false, - ExpectedEmail: "", - }, - "Minimal IDToken with OIDC Profile URL": { - IDToken: minimalIDToken, - ProfileURL: true, - ExpectedEmail: profileURLEmail, + ExpectedUser: minimalIDToken.Subject, + ExpectedEmail: minimalIDToken.Subject, }, } for testName, tc := range testCases { @@ -298,9 +293,6 @@ func TestCreateSessionStateFromBearerToken(t *testing.T) { jsonResp := []byte(fmt.Sprintf(`{"email":"%s"}`, profileURLEmail)) server, provider := newTestSetup(jsonResp) defer server.Close() - if !tc.ProfileURL { - provider.ProfileURL = &url.URL{} - } rawIDToken, err := newSignedTestIDToken(tc.IDToken) assert.NoError(t, err) @@ -315,13 +307,8 @@ func TestCreateSessionStateFromBearerToken(t *testing.T) { ss, err := provider.CreateSessionStateFromBearerToken(context.Background(), rawIDToken, idToken) assert.NoError(t, err) - if tc.ExpectedEmail != "" { - assert.Equal(t, tc.ExpectedEmail, ss.Email) - assert.NotEqual(t, ss.Email, ss.User) - } else { - assert.Equal(t, tc.IDToken.Subject, ss.Email) - assert.Equal(t, ss.Email, ss.User) - } + assert.Equal(t, tc.ExpectedUser, ss.User) + assert.Equal(t, tc.ExpectedEmail, ss.Email) assert.Equal(t, rawIDToken, ss.IDToken) assert.Equal(t, rawIDToken, ss.AccessToken) assert.Equal(t, "", ss.RefreshToken)