From a1877434b21984b1a57839c2c31dca505bb7884e Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Thu, 26 Nov 2020 19:00:30 -0800 Subject: [PATCH 1/6] Refactor OIDC to EnrichSession --- pkg/validation/options.go | 2 +- providers/oidc.go | 328 +++++++++++++++++++------------------- providers/oidc_test.go | 284 +++++++++++++++++++++++++++++---- providers/util.go | 23 +++ providers/util_test.go | 49 +++++- 5 files changed, 489 insertions(+), 197 deletions(-) diff --git a/pkg/validation/options.go b/pkg/validation/options.go index fc89000b..0cea4b2b 100644 --- a/pkg/validation/options.go +++ b/pkg/validation/options.go @@ -274,7 +274,7 @@ func parseProviderInfo(o *options.Options, msgs []string) []string { p.SetRepository(o.BitbucketRepository) case *providers.OIDCProvider: p.AllowUnverifiedEmail = o.InsecureOIDCAllowUnverifiedEmail - p.UserIDClaim = o.UserIDClaim + p.EmailClaim = o.UserIDClaim p.GroupsClaim = o.OIDCGroupsClaim if p.Verifier == nil { msgs = append(msgs, "oidc provider requires an oidc issuer URL") diff --git a/providers/oidc.go b/providers/oidc.go index 704e3341..15020282 100644 --- a/providers/oidc.go +++ b/providers/oidc.go @@ -2,18 +2,17 @@ package providers import ( "context" - "encoding/json" + "errors" "fmt" "reflect" "strings" "time" - oidc "github.com/coreos/go-oidc" - "golang.org/x/oauth2" - + "github.com/coreos/go-oidc" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests" + "golang.org/x/oauth2" ) const emailClaim = "email" @@ -23,7 +22,7 @@ type OIDCProvider struct { *ProviderData AllowUnverifiedEmail bool - UserIDClaim string + EmailClaim string GroupsClaim string } @@ -36,10 +35,10 @@ func NewOIDCProvider(p *ProviderData) *OIDCProvider { var _ Provider = (*OIDCProvider)(nil) // Redeem exchanges the OAuth2 authentication token for an ID token -func (p *OIDCProvider) Redeem(ctx context.Context, redirectURL, code string) (s *sessions.SessionState, err error) { +func (p *OIDCProvider) Redeem(ctx context.Context, redirectURL, code string) (*sessions.SessionState, error) { clientSecret, err := p.GetClientSecret() if err != nil { - return + return nil, err } c := oauth2.Config{ @@ -52,23 +51,74 @@ func (p *OIDCProvider) Redeem(ctx context.Context, redirectURL, code string) (s } token, err := c.Exchange(ctx, code) if err != nil { - return nil, fmt.Errorf("token exchange: %v", err) + return nil, fmt.Errorf("token exchange failure: %v", err) } - // in the initial exchange the id token is mandatory - idToken, err := p.findVerifiedIDToken(ctx, token) + return p.createSession(ctx, token, false) +} + +// EnrichSessionState is called after Redeem to allow providers to enrich session fields +// such as User, Email, Groups with provider specific API calls. +func (p *OIDCProvider) EnrichSession(ctx context.Context, s *sessions.SessionState) error { + if p.ProfileURL.String() == "" { + if s.Email == "" { + return errors.New("id_token did not contain an email and profileURL is not defined") + } + return nil + } + + // Try to get missing emails or groups from a profileURL + if s.Email == "" || len(s.Groups) == 0 { + err := p.callProfileURL(ctx, s) + if err != nil { + logger.Errorf("Warning: Profile URL request failed: %v", err) + } + } + + // If a mandatory email wasn't set, error at this point. + if s.Email == "" { + return errors.New("neither the id_token nor the profileURL set an email") + } + return nil +} + +// callProfileURL enriches a session's Email & Groups via the JSON response of +// an OIDC profile URL +func (p *OIDCProvider) callProfileURL(ctx context.Context, s *sessions.SessionState) error { + respJSON, err := requests.New(p.ProfileURL.String()). + WithContext(ctx). + WithHeaders(makeOIDCHeader(s.AccessToken)). + Do(). + UnmarshalJSON() if err != nil { - return nil, fmt.Errorf("could not verify id_token: %v", err) - } else if idToken == nil { - return nil, fmt.Errorf("token response did not contain an id_token") + return err } - s, err = p.createSessionState(ctx, token, idToken) - if err != nil { - return nil, fmt.Errorf("unable to update session: %v", err) + email, err := respJSON.Get(p.EmailClaim).String() + if err == nil && s.Email == "" { + s.Email = email } - return + // Handle array & singleton groups cases + if len(s.Groups) == 0 { + groups, err := respJSON.Get(p.GroupsClaim).StringArray() + if err == nil { + s.Groups = groups + } else { + group, err := respJSON.Get(p.GroupsClaim).String() + if err == nil { + s.Groups = []string{group} + } + } + } + + return nil +} + +// ValidateSessionState checks that the session's IDToken is still valid +func (p *OIDCProvider) ValidateSession(ctx context.Context, s *sessions.SessionState) bool { + _, err := p.Verifier.Verify(ctx, s.IDToken) + return err == nil } // RefreshSessionIfNeeded checks if the session has expired and uses the @@ -83,14 +133,16 @@ func (p *OIDCProvider) RefreshSessionIfNeeded(ctx context.Context, s *sessions.S return false, fmt.Errorf("unable to redeem refresh token: %v", err) } - fmt.Printf("refreshed access token %s (expired on %s)\n", s, s.ExpiresOn) + logger.Printf("refreshed session: %s", s) return true, nil } -func (p *OIDCProvider) redeemRefreshToken(ctx context.Context, s *sessions.SessionState) (err error) { +// redeemRefreshToken uses a RefreshToken with the RedeemURL to refresh the +// Access Token and (probably) the ID Token. +func (p *OIDCProvider) redeemRefreshToken(ctx context.Context, s *sessions.SessionState) error { clientSecret, err := p.GetClientSecret() if err != nil { - return + return err } c := oauth2.Config{ @@ -109,19 +161,14 @@ func (p *OIDCProvider) redeemRefreshToken(ctx context.Context, s *sessions.Sessi return fmt.Errorf("failed to get token: %v", err) } - // in the token refresh response the id_token is optional - idToken, err := p.findVerifiedIDToken(ctx, token) - if err != nil { - return fmt.Errorf("unable to extract id_token from response: %v", err) - } - - newSession, err := p.createSessionState(ctx, token, idToken) + newSession, err := p.createSession(ctx, token, true) if err != nil { return fmt.Errorf("unable create new session state from response: %v", err) } - // It's possible that if the refresh token isn't in the token response the session will not contain an id token - // if it doesn't it's probably better to retain the old one + // It's possible that if the refresh token isn't in the token response the + // session will not contain an id token. + // If it doesn't it's probably better to retain the old one if newSession.IDToken != "" { s.IDToken = newSession.IDToken s.Email = newSession.Email @@ -135,102 +182,113 @@ func (p *OIDCProvider) redeemRefreshToken(ctx context.Context, s *sessions.Sessi s.CreatedAt = newSession.CreatedAt s.ExpiresOn = newSession.ExpiresOn - return -} - -func (p *OIDCProvider) findVerifiedIDToken(ctx context.Context, token *oauth2.Token) (*oidc.IDToken, error) { - - getIDToken := func() (string, bool) { - rawIDToken, _ := token.Extra("id_token").(string) - return rawIDToken, len(strings.TrimSpace(rawIDToken)) > 0 - } - - if rawIDToken, present := getIDToken(); present { - verifiedIDToken, err := p.Verifier.Verify(ctx, rawIDToken) - return verifiedIDToken, err - } - return nil, nil -} - -func (p *OIDCProvider) createSessionState(ctx context.Context, token *oauth2.Token, idToken *oidc.IDToken) (*sessions.SessionState, error) { - - var newSession *sessions.SessionState - - if idToken == nil { - newSession = &sessions.SessionState{} - } else { - var err error - newSession, err = p.createSessionStateInternal(ctx, idToken, token) - if err != nil { - return nil, err - } - } - - created := time.Now() - newSession.AccessToken = token.AccessToken - newSession.RefreshToken = token.RefreshToken - newSession.CreatedAt = &created - newSession.ExpiresOn = &token.Expiry - return newSession, nil + return nil } +// CreateSessionFromToken converts Bearer IDTokens into sessions func (p *OIDCProvider) CreateSessionFromToken(ctx context.Context, token string) (*sessions.SessionState, error) { idToken, err := p.Verifier.Verify(ctx, token) if err != nil { return nil, err } - newSession, err := p.createSessionStateInternal(ctx, idToken, nil) + ss, err := p.buildSessionFromClaims(idToken) if err != nil { return nil, err } - newSession.AccessToken = token - newSession.IDToken = token - newSession.RefreshToken = "" - newSession.ExpiresOn = &idToken.Expiry - - return newSession, nil -} - -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 + // Allow empty Email in Bearer case since we can't hit the ProfileURL + if ss.Email == "" { + ss.Email = ss.User } - claims, err := p.findClaimsFromIDToken(ctx, idToken, token) + ss.AccessToken = token + ss.IDToken = token + ss.RefreshToken = "" + ss.ExpiresOn = &idToken.Expiry + + return ss, nil +} + +// createSession takes an oauth2.Token and creates a SessionState from it. +// It alters behavior if called from Redeem vs Refresh +func (p *OIDCProvider) createSession(ctx context.Context, token *oauth2.Token, refresh bool) (*sessions.SessionState, error) { + idToken, err := p.findVerifiedIDToken(ctx, token) + if err != nil { + return nil, fmt.Errorf("could not verify id_token: %v", err) + } + + // IDToken is mandatory in Redeem but optional in Refresh + if idToken == nil && !refresh { + return nil, errors.New("token response did not contain an id_token") + } + + ss, err := p.buildSessionFromClaims(idToken) + if err != nil { + return nil, err + } + + ss.AccessToken = token.AccessToken + ss.RefreshToken = token.RefreshToken + ss.IDToken = getIDToken(token) + + created := time.Now() + ss.CreatedAt = &created + ss.ExpiresOn = &token.Expiry + + return ss, nil +} + +func (p *OIDCProvider) findVerifiedIDToken(ctx context.Context, token *oauth2.Token) (*oidc.IDToken, error) { + rawIDToken := getIDToken(token) + if strings.TrimSpace(rawIDToken) != "" { + return p.Verifier.Verify(ctx, rawIDToken) + } + return nil, nil +} + +// buildSessionFromClaims uses IDToken claims to populate a fresh SessionState +// with non-Token related fields. +func (p *OIDCProvider) buildSessionFromClaims(idToken *oidc.IDToken) (*sessions.SessionState, error) { + ss := &sessions.SessionState{} + + if idToken == nil { + return ss, nil + } + + claims, err := p.getClaims(idToken) if err != nil { return nil, fmt.Errorf("couldn't extract claims from id_token (%v)", err) } - if token != nil { - newSession.IDToken = token.Extra("id_token").(string) + ss.User = claims.Subject + ss.Email = claims.Email + ss.Groups = claims.Groups + + // TODO (@NickMeves) Deprecate for dynamic claim to session mapping + if pref, ok := claims.rawClaims["preferred_username"].(string); ok { + ss.PreferredUsername = pref } - newSession.Email = claims.UserID // TODO Rename SessionState.Email to .UserID in the near future - - newSession.User = claims.Subject - newSession.Groups = claims.Groups - newSession.PreferredUsername = claims.PreferredUsername - - verifyEmail := (p.UserIDClaim == emailClaim) && !p.AllowUnverifiedEmail + verifyEmail := (p.EmailClaim == emailClaim) && !p.AllowUnverifiedEmail if verifyEmail && claims.Verified != nil && !*claims.Verified { - return nil, fmt.Errorf("email in id_token (%s) isn't verified", claims.UserID) + return nil, fmt.Errorf("email in id_token (%s) isn't verified", claims.Email) } - return newSession, nil + return ss, nil } -// ValidateSessionState checks that the session's IDToken is still valid -func (p *OIDCProvider) ValidateSession(ctx context.Context, s *sessions.SessionState) bool { - _, err := p.Verifier.Verify(ctx, s.IDToken) - return err == nil +type OIDCClaims struct { + Subject string `json:"sub"` + Email string `json:"-"` + Groups []string `json:"-"` + Verified *bool `json:"email_verified"` + + rawClaims map[string]interface{} } -func (p *OIDCProvider) findClaimsFromIDToken(ctx context.Context, idToken *oidc.IDToken, token *oauth2.Token) (*OIDCClaims, error) { +// getClaims extracts IDToken claims into an OIDCClaims +func (p *OIDCProvider) getClaims(idToken *oidc.IDToken) (*OIDCClaims, error) { claims := &OIDCClaims{} // Extract default claims. @@ -242,86 +300,28 @@ func (p *OIDCProvider) findClaimsFromIDToken(ctx context.Context, idToken *oidc. return nil, fmt.Errorf("failed to parse all id_token claims: %v", err) } - userID := claims.rawClaims[p.UserIDClaim] - if userID != nil { - claims.UserID = fmt.Sprint(userID) - } - - claims.Groups = p.extractGroupsFromRawClaims(claims.rawClaims) - - // userID claim was not present or was empty in the ID Token - if claims.UserID == "" { - // 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) - } - - // If the userinfo endpoint profileURL is defined, then there is a chance the userinfo - // contents at the profileURL contains the email. - // Make a query to the userinfo endpoint, and attempt to locate the email from there. - respJSON, err := requests.New(profileURL). - WithContext(ctx). - WithHeaders(makeOIDCHeader(token.AccessToken)). - Do(). - UnmarshalJSON() - if err != nil { - return nil, err - } - - userID, err := respJSON.Get(p.UserIDClaim).String() - if err != nil { - return nil, fmt.Errorf("neither id_token nor userinfo endpoint contained user ID claim (%q)", p.UserIDClaim) - } - - claims.UserID = userID + email := claims.rawClaims[p.EmailClaim] + if email != nil { + claims.Email = fmt.Sprint(email) } + claims.Groups = p.extractGroups(claims.rawClaims) return claims, nil } -func (p *OIDCProvider) extractGroupsFromRawClaims(rawClaims map[string]interface{}) []string { +func (p *OIDCProvider) extractGroups(claims map[string]interface{}) []string { groups := []string{} - - rawGroups, ok := rawClaims[p.GroupsClaim].([]interface{}) + rawGroups, ok := claims[p.GroupsClaim].([]interface{}) if rawGroups != nil && ok { for _, rawGroup := range rawGroups { formattedGroup, err := formatGroup(rawGroup) if err != nil { - logger.Errorf("unable to format group of type %s with error %s", reflect.TypeOf(rawGroup), err) + logger.Errorf("Warning: unable to format group of type %s with error %s", + reflect.TypeOf(rawGroup), err) continue } groups = append(groups, formattedGroup) } } - return groups - -} - -func formatGroup(rawGroup interface{}) (string, error) { - group, ok := rawGroup.(string) - if !ok { - jsonGroup, err := json.Marshal(rawGroup) - if err != nil { - return "", err - } - group = string(jsonGroup) - } - return group, nil -} - -type OIDCClaims struct { - rawClaims map[string]interface{} - UserID string - Subject string `json:"sub"` - Verified *bool `json:"email_verified"` - PreferredUsername string `json:"preferred_username"` - Groups []string `json:"-"` } diff --git a/providers/oidc_test.go b/providers/oidc_test.go index 54df1b31..0cc6ef76 100644 --- a/providers/oidc_test.go +++ b/providers/oidc_test.go @@ -154,7 +154,7 @@ func newOIDCProvider(serverURL *url.URL) *OIDCProvider { p := &OIDCProvider{ ProviderData: providerData, - UserIDClaim: "email", + EmailClaim: "email", } return p @@ -225,7 +225,7 @@ func TestOIDCProviderRedeem_custom_userid(t *testing.T) { }) server, provider := newTestSetup(body) - provider.UserIDClaim = "phone_number" + provider.EmailClaim = "phone_number" defer server.Close() session, err := provider.Redeem(context.Background(), provider.RedeemURL.String(), "code1234") @@ -233,6 +233,256 @@ func TestOIDCProviderRedeem_custom_userid(t *testing.T) { assert.Equal(t, defaultIDToken.Phone, session.Email) } +func TestOIDCProvider_EnrichSession(t *testing.T) { + const ( + idToken = "Unchanged ID Token" + accessToken = "Unchanged Access Token" + refreshToken = "Unchanged Refresh Token" + ) + + testCases := map[string]struct { + ExistingSession *sessions.SessionState + EmailClaim string + GroupsClaim string + ProfileJSON map[string]interface{} + ExpectedError error + ExpectedSession *sessions.SessionState + }{ + "Already Populated": { + ExistingSession: &sessions.SessionState{ + User: "already", + Email: "already@populated.com", + Groups: []string{"already", "populated"}, + IDToken: idToken, + AccessToken: accessToken, + RefreshToken: refreshToken, + }, + EmailClaim: "email", + GroupsClaim: "groups", + ProfileJSON: map[string]interface{}{ + "email": "new@thing.com", + "groups": []string{"new", "thing"}, + }, + ExpectedError: nil, + ExpectedSession: &sessions.SessionState{ + User: "already", + Email: "already@populated.com", + Groups: []string{"already", "populated"}, + IDToken: idToken, + AccessToken: accessToken, + RefreshToken: refreshToken, + }, + }, + "Missing Email": { + ExistingSession: &sessions.SessionState{ + User: "missing.email", + Groups: []string{"already", "populated"}, + IDToken: idToken, + AccessToken: accessToken, + RefreshToken: refreshToken, + }, + EmailClaim: "email", + GroupsClaim: "groups", + ProfileJSON: map[string]interface{}{ + "email": "found@email.com", + "groups": []string{"new", "thing"}, + }, + ExpectedError: nil, + ExpectedSession: &sessions.SessionState{ + User: "missing.email", + Email: "found@email.com", + Groups: []string{"already", "populated"}, + IDToken: idToken, + AccessToken: accessToken, + RefreshToken: refreshToken, + }, + }, + + "Missing Email Only in Profile URL": { + ExistingSession: &sessions.SessionState{ + User: "missing.email", + IDToken: idToken, + AccessToken: accessToken, + RefreshToken: refreshToken, + }, + EmailClaim: "email", + GroupsClaim: "groups", + ProfileJSON: map[string]interface{}{ + "email": "found@email.com", + }, + ExpectedError: nil, + ExpectedSession: &sessions.SessionState{ + User: "missing.email", + Email: "found@email.com", + IDToken: idToken, + AccessToken: accessToken, + RefreshToken: refreshToken, + }, + }, + "Missing Email with Custom Claim": { + ExistingSession: &sessions.SessionState{ + User: "missing.email", + Groups: []string{"already", "populated"}, + IDToken: idToken, + AccessToken: accessToken, + RefreshToken: refreshToken, + }, + EmailClaim: "weird", + GroupsClaim: "groups", + ProfileJSON: map[string]interface{}{ + "weird": "weird@claim.com", + "groups": []string{"new", "thing"}, + }, + ExpectedError: nil, + ExpectedSession: &sessions.SessionState{ + User: "missing.email", + Email: "weird@claim.com", + Groups: []string{"already", "populated"}, + IDToken: idToken, + AccessToken: accessToken, + RefreshToken: refreshToken, + }, + }, + "Missing Email not in Profile URL": { + ExistingSession: &sessions.SessionState{ + User: "missing.email", + Groups: []string{"already", "populated"}, + IDToken: idToken, + AccessToken: accessToken, + RefreshToken: refreshToken, + }, + EmailClaim: "email", + GroupsClaim: "groups", + ProfileJSON: map[string]interface{}{ + "groups": []string{"new", "thing"}, + }, + ExpectedError: errors.New("neither the id_token nor the profileURL set an email"), + ExpectedSession: &sessions.SessionState{ + User: "missing.email", + Groups: []string{"already", "populated"}, + IDToken: idToken, + AccessToken: accessToken, + RefreshToken: refreshToken, + }, + }, + "Missing Groups": { + ExistingSession: &sessions.SessionState{ + User: "already", + Email: "already@populated.com", + Groups: []string{}, + IDToken: idToken, + AccessToken: accessToken, + RefreshToken: refreshToken, + }, + EmailClaim: "email", + GroupsClaim: "groups", + ProfileJSON: map[string]interface{}{ + "email": "new@thing.com", + "groups": []string{"new", "thing"}, + }, + ExpectedError: nil, + ExpectedSession: &sessions.SessionState{ + User: "already", + Email: "already@populated.com", + Groups: []string{"new", "thing"}, + IDToken: idToken, + AccessToken: accessToken, + RefreshToken: refreshToken, + }, + }, + "Missing Groups with Custom Claim": { + ExistingSession: &sessions.SessionState{ + User: "already", + Email: "already@populated.com", + Groups: nil, + IDToken: idToken, + AccessToken: accessToken, + RefreshToken: refreshToken, + }, + EmailClaim: "email", + GroupsClaim: "roles", + ProfileJSON: map[string]interface{}{ + "email": "new@thing.com", + "roles": []string{"new", "thing", "roles"}, + }, + ExpectedError: nil, + ExpectedSession: &sessions.SessionState{ + User: "already", + Email: "already@populated.com", + Groups: []string{"new", "thing", "roles"}, + IDToken: idToken, + AccessToken: accessToken, + RefreshToken: refreshToken, + }, + }, + "Missing Groups String Profile URL Response": { + ExistingSession: &sessions.SessionState{ + User: "already", + Email: "already@populated.com", + Groups: []string{}, + IDToken: idToken, + AccessToken: accessToken, + RefreshToken: refreshToken, + }, + EmailClaim: "email", + GroupsClaim: "groups", + ProfileJSON: map[string]interface{}{ + "email": "new@thing.com", + "groups": "singleton", + }, + ExpectedError: nil, + ExpectedSession: &sessions.SessionState{ + User: "already", + Email: "already@populated.com", + Groups: []string{"singleton"}, + IDToken: idToken, + AccessToken: accessToken, + RefreshToken: refreshToken, + }, + }, + "Missing Groups in both Claims and Profile URL": { + ExistingSession: &sessions.SessionState{ + User: "already", + Email: "already@populated.com", + IDToken: idToken, + AccessToken: accessToken, + RefreshToken: refreshToken, + }, + EmailClaim: "email", + GroupsClaim: "groups", + ProfileJSON: map[string]interface{}{ + "email": "new@thing.com", + }, + ExpectedError: nil, + ExpectedSession: &sessions.SessionState{ + User: "already", + Email: "already@populated.com", + IDToken: idToken, + AccessToken: accessToken, + RefreshToken: refreshToken, + }, + }, + } + for testName, tc := range testCases { + t.Run(testName, func(t *testing.T) { + jsonResp, err := json.Marshal(tc.ProfileJSON) + assert.NoError(t, err) + + server, provider := newTestSetup(jsonResp) + provider.ProfileURL, err = url.Parse(server.URL) + assert.NoError(t, err) + + provider.EmailClaim = tc.EmailClaim + provider.GroupsClaim = tc.GroupsClaim + defer server.Close() + + err = provider.EnrichSession(context.Background(), tc.ExistingSession) + assert.Equal(t, tc.ExpectedError, err) + assert.Equal(t, *tc.ExpectedSession, *tc.ExistingSession) + }) + } +} + func TestOIDCProviderRefreshSessionIfNeededWithoutIdToken(t *testing.T) { idToken, _ := newSignedTestIDToken(defaultIDToken) @@ -361,7 +611,7 @@ func TestOIDCProviderCreateSessionFromToken(t *testing.T) { } } -func TestOIDCProvider_findVerifiedIdToken(t *testing.T) { +func TestOIDCProvider_findVerifiedIDToken(t *testing.T) { server, provider := newTestSetup([]byte("")) @@ -397,31 +647,3 @@ func TestOIDCProvider_findVerifiedIdToken(t *testing.T) { assert.Equal(t, nil, err) assert.Equal(t, true, verifiedIDToken == nil) } - -func Test_formatGroup(t *testing.T) { - testCases := map[string]struct { - RawGroup interface{} - ExpectedFormattedGroupValue string - }{ - "String Group": { - RawGroup: "group", - ExpectedFormattedGroupValue: "group", - }, - "Map Group": { - RawGroup: map[string]string{"id": "1", "name": "Test"}, - ExpectedFormattedGroupValue: "{\"id\":\"1\",\"name\":\"Test\"}", - }, - "List Group": { - RawGroup: []string{"First", "Second"}, - ExpectedFormattedGroupValue: "[\"First\",\"Second\"]", - }, - } - - for testName, tc := range testCases { - t.Run(testName, func(t *testing.T) { - formattedGroup, err := formatGroup(tc.RawGroup) - assert.Nil(t, err) - assert.Equal(t, tc.ExpectedFormattedGroupValue, formattedGroup) - }) - } -} diff --git a/providers/util.go b/providers/util.go index b4b65ac6..acf20902 100644 --- a/providers/util.go +++ b/providers/util.go @@ -1,9 +1,12 @@ package providers import ( + "encoding/json" "fmt" "net/http" "net/url" + + "golang.org/x/oauth2" ) const ( @@ -55,3 +58,23 @@ func makeLoginURL(p *ProviderData, redirectURI, state string, extraParams url.Va a.RawQuery = params.Encode() return a } + +func getIDToken(token *oauth2.Token) string { + idToken, ok := token.Extra("id_token").(string) + if !ok { + return "" + } + return idToken +} + +func formatGroup(rawGroup interface{}) (string, error) { + group, ok := rawGroup.(string) + if !ok { + jsonGroup, err := json.Marshal(rawGroup) + if err != nil { + return "", err + } + group = string(jsonGroup) + } + return group, nil +} diff --git a/providers/util_test.go b/providers/util_test.go index 798df6cb..e14ff061 100644 --- a/providers/util_test.go +++ b/providers/util_test.go @@ -5,9 +5,10 @@ import ( "testing" . "github.com/onsi/gomega" + "golang.org/x/oauth2" ) -func TestMakeAuhtorizationHeader(t *testing.T) { +func Test_makeAuthorizationHeader(t *testing.T) { testCases := []struct { name string prefix string @@ -64,3 +65,49 @@ func TestMakeAuhtorizationHeader(t *testing.T) { }) } } + +func Test_getIDToken(t *testing.T) { + const idToken = "eyJfoobar.eyJfoobar.12345asdf" + g := NewWithT(t) + + token := &oauth2.Token{} + g.Expect(getIDToken(token)).To(Equal("")) + + extraToken := token.WithExtra(map[string]interface{}{ + "id_token": idToken, + }) + g.Expect(getIDToken(extraToken)).To(Equal(idToken)) +} + +func Test_formatGroup(t *testing.T) { + testCases := map[string]struct { + rawGroup interface{} + expected string + }{ + "String Group": { + rawGroup: "group", + expected: "group", + }, + "Numeric Group": { + rawGroup: 123, + expected: "123", + }, + "Map Group": { + rawGroup: map[string]string{"id": "1", "name": "Test"}, + expected: "{\"id\":\"1\",\"name\":\"Test\"}", + }, + "List Group": { + rawGroup: []string{"First", "Second"}, + expected: "[\"First\",\"Second\"]", + }, + } + + for testName, tc := range testCases { + t.Run(testName, func(t *testing.T) { + g := NewWithT(t) + formattedGroup, err := formatGroup(tc.rawGroup) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(formattedGroup).To(Equal(tc.expected)) + }) + } +} From 74ac4274c6643cb205336a86f6ef40d16805c59a Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Fri, 27 Nov 2020 16:17:59 -0800 Subject: [PATCH 2/6] Move generic OIDC functionality to be available to all providers --- pkg/validation/options.go | 5 +- providers/oidc.go | 98 +------- providers/oidc_test.go | 206 ++-------------- providers/provider_data.go | 109 ++++++++- providers/provider_data_test.go | 414 ++++++++++++++++++++++++++++++++ providers/provider_default.go | 2 + 6 files changed, 551 insertions(+), 283 deletions(-) create mode 100644 providers/provider_data_test.go diff --git a/pkg/validation/options.go b/pkg/validation/options.go index 0cea4b2b..652ada9e 100644 --- a/pkg/validation/options.go +++ b/pkg/validation/options.go @@ -233,7 +233,10 @@ func parseProviderInfo(o *options.Options, msgs []string) []string { p.ValidateURL, msgs = parseURL(o.ValidateURL, "validate", msgs) p.ProtectedResource, msgs = parseURL(o.ProtectedResource, "resource", msgs) - // Make the OIDC Verifier accessible to all providers that can support it + // Make the OIDC options available to all providers that support it + p.AllowUnverifiedEmail = o.InsecureOIDCAllowUnverifiedEmail + p.EmailClaim = o.UserIDClaim + p.GroupsClaim = o.OIDCGroupsClaim p.Verifier = o.GetOIDCVerifier() p.SetAllowedGroups(o.AllowedGroups) diff --git a/providers/oidc.go b/providers/oidc.go index 15020282..f90348d6 100644 --- a/providers/oidc.go +++ b/providers/oidc.go @@ -4,26 +4,17 @@ import ( "context" "errors" "fmt" - "reflect" - "strings" "time" - "github.com/coreos/go-oidc" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests" "golang.org/x/oauth2" ) -const emailClaim = "email" - // OIDCProvider represents an OIDC based Identity Provider type OIDCProvider struct { *ProviderData - - AllowUnverifiedEmail bool - EmailClaim string - GroupsClaim string } // NewOIDCProvider initiates a new OIDCProvider @@ -213,7 +204,7 @@ func (p *OIDCProvider) CreateSessionFromToken(ctx context.Context, token string) // createSession takes an oauth2.Token and creates a SessionState from it. // It alters behavior if called from Redeem vs Refresh func (p *OIDCProvider) createSession(ctx context.Context, token *oauth2.Token, refresh bool) (*sessions.SessionState, error) { - idToken, err := p.findVerifiedIDToken(ctx, token) + idToken, err := p.verifyIDToken(ctx, token) if err != nil { return nil, fmt.Errorf("could not verify id_token: %v", err) } @@ -238,90 +229,3 @@ func (p *OIDCProvider) createSession(ctx context.Context, token *oauth2.Token, r return ss, nil } - -func (p *OIDCProvider) findVerifiedIDToken(ctx context.Context, token *oauth2.Token) (*oidc.IDToken, error) { - rawIDToken := getIDToken(token) - if strings.TrimSpace(rawIDToken) != "" { - return p.Verifier.Verify(ctx, rawIDToken) - } - return nil, nil -} - -// buildSessionFromClaims uses IDToken claims to populate a fresh SessionState -// with non-Token related fields. -func (p *OIDCProvider) buildSessionFromClaims(idToken *oidc.IDToken) (*sessions.SessionState, error) { - ss := &sessions.SessionState{} - - if idToken == nil { - return ss, nil - } - - claims, err := p.getClaims(idToken) - if err != nil { - return nil, fmt.Errorf("couldn't extract claims from id_token (%v)", err) - } - - ss.User = claims.Subject - ss.Email = claims.Email - ss.Groups = claims.Groups - - // TODO (@NickMeves) Deprecate for dynamic claim to session mapping - if pref, ok := claims.rawClaims["preferred_username"].(string); ok { - ss.PreferredUsername = pref - } - - verifyEmail := (p.EmailClaim == emailClaim) && !p.AllowUnverifiedEmail - if verifyEmail && claims.Verified != nil && !*claims.Verified { - return nil, fmt.Errorf("email in id_token (%s) isn't verified", claims.Email) - } - - return ss, nil -} - -type OIDCClaims struct { - Subject string `json:"sub"` - Email string `json:"-"` - Groups []string `json:"-"` - Verified *bool `json:"email_verified"` - - rawClaims map[string]interface{} -} - -// getClaims extracts IDToken claims into an OIDCClaims -func (p *OIDCProvider) getClaims(idToken *oidc.IDToken) (*OIDCClaims, error) { - claims := &OIDCClaims{} - - // Extract default claims. - if err := idToken.Claims(&claims); err != nil { - return nil, fmt.Errorf("failed to parse default id_token claims: %v", err) - } - // Extract custom claims. - if err := idToken.Claims(&claims.rawClaims); err != nil { - return nil, fmt.Errorf("failed to parse all id_token claims: %v", err) - } - - email := claims.rawClaims[p.EmailClaim] - if email != nil { - claims.Email = fmt.Sprint(email) - } - claims.Groups = p.extractGroups(claims.rawClaims) - - return claims, nil -} - -func (p *OIDCProvider) extractGroups(claims map[string]interface{}) []string { - groups := []string{} - rawGroups, ok := claims[p.GroupsClaim].([]interface{}) - if rawGroups != nil && ok { - for _, rawGroup := range rawGroups { - formattedGroup, err := formatGroup(rawGroup) - if err != nil { - logger.Errorf("Warning: unable to format group of type %s with error %s", - reflect.TypeOf(rawGroup), err) - continue - } - groups = append(groups, formattedGroup) - } - } - return groups -} diff --git a/providers/oidc_test.go b/providers/oidc_test.go index 0cc6ef76..2651b4ea 100644 --- a/providers/oidc_test.go +++ b/providers/oidc_test.go @@ -2,42 +2,18 @@ package providers import ( "context" - "crypto/rand" - "crypto/rsa" - "encoding/base64" "encoding/json" "errors" - "fmt" "net/http" "net/http/httptest" "net/url" - "strings" "testing" - "time" "github.com/coreos/go-oidc" - "github.com/dgrijalva/jwt-go" - "github.com/stretchr/testify/assert" - "golang.org/x/oauth2" - "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" + "github.com/stretchr/testify/assert" ) -const accessToken = "access_token" -const refreshToken = "refresh_token" -const clientID = "https://test.myapp.com" -const secret = "secret" - -type idTokenClaims struct { - Name string `json:"name,omitempty"` - Email string `json:"email,omitempty"` - Phone string `json:"phone_number,omitempty"` - Picture string `json:"picture,omitempty"` - Groups interface{} `json:"groups,omitempty"` - OtherGroups interface{} `json:"other_groups,omitempty"` - jwt.StandardClaims -} - type redeemTokenResponse struct { AccessToken string `json:"access_token"` RefreshToken string `json:"refresh_token"` @@ -46,88 +22,12 @@ type redeemTokenResponse struct { IDToken string `json:"id_token,omitempty"` } -var defaultIDToken idTokenClaims = idTokenClaims{ - "Jane Dobbs", - "janed@me.com", - "+4798765432", - "http://mugbook.com/janed/me.jpg", - []string{"test:a", "test:b"}, - []string{"test:c", "test:d"}, - 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: "123456789", - }, -} - -var customGroupClaimIDToken idTokenClaims = idTokenClaims{ - "Jane Dobbs", - "janed@me.com", - "+4798765432", - "http://mugbook.com/janed/me.jpg", - []map[string]interface{}{ - { - "groupId": "Admin Group Id", - "roles": []string{"Admin"}, - }, - }, - []string{"test:c", "test:d"}, - 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: "123456789", - }, -} - -var minimalIDToken idTokenClaims = idTokenClaims{ - "", - "", - "", - "", - []string{}, - []string{}, - 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) { - decodeString, err := base64.RawURLEncoding.DecodeString(strings.Split(jwt, ".")[1]) - if err != nil { - return nil, err - } - tokenClaims := &idTokenClaims{} - err = json.Unmarshal(decodeString, tokenClaims) - - if err != nil || tokenClaims.Id == "this-id-fails-validation" { - return nil, fmt.Errorf("the validation failed for subject [%v]", tokenClaims.Subject) - } - - return decodeString, err -} - func newOIDCProvider(serverURL *url.URL) *OIDCProvider { providerData := &ProviderData{ ProviderName: "oidc", - ClientID: clientID, - ClientSecret: secret, + ClientID: oidcClientID, + ClientSecret: oidcSecret, LoginURL: &url.URL{ Scheme: serverURL.Scheme, Host: serverURL.Host, @@ -144,18 +44,17 @@ func newOIDCProvider(serverURL *url.URL) *OIDCProvider { Scheme: serverURL.Scheme, Host: serverURL.Host, Path: "/api"}, - Scope: "openid profile offline_access", + Scope: "openid profile offline_access", + EmailClaim: "email", + GroupsClaim: "groups", Verifier: oidc.NewVerifier( - "https://issuer.example.com", - fakeKeySetStub{}, - &oidc.Config{ClientID: clientID}, + oidcIssuer, + mockJWKS{}, + &oidc.Config{ClientID: oidcClientID}, ), } - p := &OIDCProvider{ - ProviderData: providerData, - EmailClaim: "email", - } + p := &OIDCProvider{ProviderData: providerData} return p } @@ -169,21 +68,6 @@ func newOIDCServer(body []byte) (*url.URL, *httptest.Server) { return u, s } -func newSignedTestIDToken(tokenClaims idTokenClaims) (string, error) { - key, _ := rsa.GenerateKey(rand.Reader, 2048) - standardClaims := jwt.NewWithClaims(jwt.SigningMethodRS256, tokenClaims) - return standardClaims.SignedString(key) -} - -func newOauth2Token() *oauth2.Token { - return &oauth2.Token{ - AccessToken: accessToken, - TokenType: "Bearer", - RefreshToken: refreshToken, - Expiry: time.Time{}.Add(time.Duration(5) * time.Second), - } -} - func newTestSetup(body []byte) (*httptest.Server, *OIDCProvider) { redeemURL, server := newOIDCServer(body) provider := newOIDCProvider(redeemURL) @@ -234,12 +118,6 @@ func TestOIDCProviderRedeem_custom_userid(t *testing.T) { } func TestOIDCProvider_EnrichSession(t *testing.T) { - const ( - idToken = "Unchanged ID Token" - accessToken = "Unchanged Access Token" - refreshToken = "Unchanged Refresh Token" - ) - testCases := map[string]struct { ExistingSession *sessions.SessionState EmailClaim string @@ -550,8 +428,6 @@ func TestOIDCProviderRefreshSessionIfNeededWithIdToken(t *testing.T) { } func TestOIDCProviderCreateSessionFromToken(t *testing.T) { - const profileURLEmail = "janed@me.com" - testCases := map[string]struct { IDToken idTokenClaims GroupsClaim string @@ -562,36 +438,35 @@ func TestOIDCProviderCreateSessionFromToken(t *testing.T) { "Default IDToken": { IDToken: defaultIDToken, GroupsClaim: "groups", - ExpectedUser: defaultIDToken.Subject, - ExpectedEmail: defaultIDToken.Email, + ExpectedUser: "123456789", + ExpectedEmail: "janed@me.com", ExpectedGroups: []string{"test:a", "test:b"}, }, "Minimal IDToken with no email claim": { IDToken: minimalIDToken, GroupsClaim: "groups", - ExpectedUser: minimalIDToken.Subject, - ExpectedEmail: minimalIDToken.Subject, + ExpectedUser: "123456789", + ExpectedEmail: "123456789", ExpectedGroups: []string{}, }, "Custom Groups Claim": { IDToken: defaultIDToken, - GroupsClaim: "other_groups", - ExpectedUser: defaultIDToken.Subject, - ExpectedEmail: defaultIDToken.Email, + GroupsClaim: "roles", + ExpectedUser: "123456789", + ExpectedEmail: "janed@me.com", ExpectedGroups: []string{"test:c", "test:d"}, }, - "Custom Groups Claim2": { - IDToken: customGroupClaimIDToken, + "Complex Groups Claim": { + IDToken: complexGroupsIDToken, GroupsClaim: "groups", - ExpectedUser: customGroupClaimIDToken.Subject, - ExpectedEmail: customGroupClaimIDToken.Email, + ExpectedUser: "123456789", + ExpectedEmail: "complex@claims.com", ExpectedGroups: []string{"{\"groupId\":\"Admin Group Id\",\"roles\":[\"Admin\"]}"}, }, } for testName, tc := range testCases { t.Run(testName, func(t *testing.T) { - jsonResp := []byte(fmt.Sprintf(`{"email":"%s"}`, profileURLEmail)) - server, provider := newTestSetup(jsonResp) + server, provider := newTestSetup([]byte(`{}`)) provider.GroupsClaim = tc.GroupsClaim defer server.Close() @@ -610,40 +485,3 @@ func TestOIDCProviderCreateSessionFromToken(t *testing.T) { }) } } - -func TestOIDCProvider_findVerifiedIDToken(t *testing.T) { - - server, provider := newTestSetup([]byte("")) - - defer server.Close() - - token := newOauth2Token() - signedIDToken, _ := newSignedTestIDToken(defaultIDToken) - tokenWithIDToken := token.WithExtra(map[string]interface{}{ - "id_token": signedIDToken, - }) - - verifiedIDToken, err := provider.findVerifiedIDToken(context.Background(), tokenWithIDToken) - assert.Equal(t, true, err == nil) - if verifiedIDToken == nil { - t.Fatal("verifiedIDToken is nil") - } - assert.Equal(t, defaultIDToken.Issuer, verifiedIDToken.Issuer) - assert.Equal(t, defaultIDToken.Subject, verifiedIDToken.Subject) - - // When the validation fails the response should be nil - defaultIDToken.Id = "this-id-fails-validation" - signedIDToken, _ = newSignedTestIDToken(defaultIDToken) - tokenWithIDToken = token.WithExtra(map[string]interface{}{ - "id_token": signedIDToken, - }) - - verifiedIDToken, err = provider.findVerifiedIDToken(context.Background(), tokenWithIDToken) - assert.Equal(t, errors.New("failed to verify signature: the validation failed for subject [123456789]"), err) - assert.Equal(t, true, verifiedIDToken == nil) - - // When there is no id token in the oauth token - verifiedIDToken, err = provider.findVerifiedIDToken(context.Background(), newOauth2Token()) - assert.Equal(t, nil, err) - assert.Equal(t, true, verifiedIDToken == nil) -} diff --git a/providers/provider_data.go b/providers/provider_data.go index 330df7ca..09eadd25 100644 --- a/providers/provider_data.go +++ b/providers/provider_data.go @@ -1,12 +1,18 @@ package providers import ( + "context" "errors" + "fmt" "io/ioutil" "net/url" + "reflect" + "strings" "github.com/coreos/go-oidc" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/logger" + "golang.org/x/oauth2" ) // ProviderData contains information required to configure all implementations @@ -27,7 +33,12 @@ type ProviderData struct { ClientSecretFile string Scope string Prompt string - Verifier *oidc.IDTokenVerifier + + // Common OIDC options for any OIDC-based providers to consume + AllowUnverifiedEmail bool + EmailClaim string + GroupsClaim string + Verifier *oidc.IDTokenVerifier // Universal Group authorization data structure // any provider can set to consume @@ -94,3 +105,99 @@ func defaultURL(u *url.URL, d *url.URL) *url.URL { } return &url.URL{} } + +// **************************************************************************** +// These private OIDC helper methods are available to any providers that are +// OIDC compliant +// **************************************************************************** + +// OIDCClaims is a struct to unmarshal the OIDC claims from an ID Token payload +type OIDCClaims struct { + Subject string `json:"sub"` + Email string `json:"-"` + Groups []string `json:"-"` + Verified *bool `json:"email_verified"` + + raw map[string]interface{} +} + +func (p *ProviderData) verifyIDToken(ctx context.Context, token *oauth2.Token) (*oidc.IDToken, error) { + rawIDToken := getIDToken(token) + if strings.TrimSpace(rawIDToken) != "" { + return p.Verifier.Verify(ctx, rawIDToken) + } + return nil, nil +} + +// buildSessionFromClaims uses IDToken claims to populate a fresh SessionState +// with non-Token related fields. +func (p *ProviderData) buildSessionFromClaims(idToken *oidc.IDToken) (*sessions.SessionState, error) { + ss := &sessions.SessionState{} + + if idToken == nil { + return ss, nil + } + + claims, err := p.getClaims(idToken) + if err != nil { + return nil, fmt.Errorf("couldn't extract claims from id_token (%v)", err) + } + + ss.User = claims.Subject + ss.Email = claims.Email + ss.Groups = claims.Groups + + // TODO (@NickMeves) Deprecate for dynamic claim to session mapping + if pref, ok := claims.raw["preferred_username"].(string); ok { + ss.PreferredUsername = pref + } + + // `email_verified` must be present and explicitly set to `false` to be + // considered unverified. + verifyEmail := (p.EmailClaim == emailClaim) && !p.AllowUnverifiedEmail + if verifyEmail && claims.Verified != nil && !*claims.Verified { + return nil, fmt.Errorf("email in id_token (%s) isn't verified", claims.Email) + } + + return ss, nil +} + +// getClaims extracts IDToken claims into an OIDCClaims +func (p *ProviderData) getClaims(idToken *oidc.IDToken) (*OIDCClaims, error) { + claims := &OIDCClaims{} + + // Extract default claims. + if err := idToken.Claims(&claims); err != nil { + return nil, fmt.Errorf("failed to parse default id_token claims: %v", err) + } + // Extract custom claims. + if err := idToken.Claims(&claims.raw); err != nil { + return nil, fmt.Errorf("failed to parse all id_token claims: %v", err) + } + + email := claims.raw[p.EmailClaim] + if email != nil { + claims.Email = fmt.Sprint(email) + } + claims.Groups = p.extractGroups(claims.raw) + + return claims, nil +} + +// extractGroups extracts groups from a claim to a list in a type safe manner +func (p *ProviderData) extractGroups(claims map[string]interface{}) []string { + groups := []string{} + rawGroups, ok := claims[p.GroupsClaim].([]interface{}) + if rawGroups != nil && ok { + for _, rawGroup := range rawGroups { + formattedGroup, err := formatGroup(rawGroup) + if err != nil { + logger.Errorf("Warning: unable to format group of type %s with error %s", + reflect.TypeOf(rawGroup), err) + continue + } + groups = append(groups, formattedGroup) + } + } + return groups +} diff --git a/providers/provider_data_test.go b/providers/provider_data_test.go new file mode 100644 index 00000000..4aed73eb --- /dev/null +++ b/providers/provider_data_test.go @@ -0,0 +1,414 @@ +package providers + +import ( + "context" + "crypto/rand" + "crypto/rsa" + "encoding/base64" + "encoding/json" + "errors" + "fmt" + "strings" + "testing" + "time" + + "github.com/coreos/go-oidc" + "github.com/dgrijalva/jwt-go" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" + . "github.com/onsi/gomega" + "golang.org/x/oauth2" +) + +const ( + idToken = "eyJfoobar123.eyJbaz987.IDToken" + accessToken = "eyJfoobar123.eyJbaz987.AccessToken" + refreshToken = "eyJfoobar123.eyJbaz987.RefreshToken" + + oidcIssuer = "https://issuer.example.com" + oidcClientID = "https://test.myapp.com" + oidcSecret = "SuperSecret123456789" + + failureTokenID = "this-id-fails-verification" +) + +var ( + verified = true + unverified = false + + standardClaims = jwt.StandardClaims{ + Audience: oidcClientID, + ExpiresAt: time.Now().Add(time.Duration(5) * time.Minute).Unix(), + Id: "id-some-id", + IssuedAt: time.Now().Unix(), + Issuer: oidcIssuer, + NotBefore: 0, + Subject: "123456789", + } + + defaultIDToken = idTokenClaims{ + Name: "Jane Dobbs", + Email: "janed@me.com", + Phone: "+4798765432", + Picture: "http://mugbook.com/janed/me.jpg", + Groups: []string{"test:a", "test:b"}, + Roles: []string{"test:c", "test:d"}, + Verified: &verified, + StandardClaims: standardClaims, + } + + complexGroupsIDToken = idTokenClaims{ + Name: "Complex Claim", + Email: "complex@claims.com", + Phone: "+5439871234", + Picture: "http://mugbook.com/complex/claims.jpg", + Groups: []map[string]interface{}{ + { + "groupId": "Admin Group Id", + "roles": []string{"Admin"}, + }, + }, + Roles: []string{"test:simple", "test:roles"}, + Verified: &verified, + StandardClaims: standardClaims, + } + + unverifiedIDToken = idTokenClaims{ + Name: "Mystery Man", + Email: "unverified@email.com", + Phone: "+4025205729", + Picture: "http://mugbook.com/unverified/email.jpg", + Groups: []string{"test:a", "test:b"}, + Roles: []string{"test:c", "test:d"}, + Verified: &unverified, + StandardClaims: standardClaims, + } + + minimalIDToken = idTokenClaims{ + StandardClaims: standardClaims, + } +) + +type idTokenClaims struct { + Name string `json:"preferred_username,omitempty"` + Email string `json:"email,omitempty"` + Phone string `json:"phone_number,omitempty"` + Picture string `json:"picture,omitempty"` + Groups interface{} `json:"groups,omitempty"` + Roles interface{} `json:"roles,omitempty"` + Verified *bool `json:"email_verified,omitempty"` + jwt.StandardClaims +} + +type mockJWKS struct{} + +func (mockJWKS) VerifySignature(_ context.Context, jwt string) ([]byte, error) { + decoded, err := base64.RawURLEncoding.DecodeString(strings.Split(jwt, ".")[1]) + if err != nil { + return nil, err + } + + tokenClaims := &idTokenClaims{} + err = json.Unmarshal(decoded, tokenClaims) + if err != nil || tokenClaims.Id == failureTokenID { + return nil, fmt.Errorf("the validation failed for subject [%v]", tokenClaims.Subject) + } + + return decoded, nil +} + +func newSignedTestIDToken(tokenClaims idTokenClaims) (string, error) { + key, _ := rsa.GenerateKey(rand.Reader, 2048) + standardClaims := jwt.NewWithClaims(jwt.SigningMethodRS256, tokenClaims) + return standardClaims.SignedString(key) +} + +func newTestOauth2Token() *oauth2.Token { + return &oauth2.Token{ + AccessToken: accessToken, + TokenType: "Bearer", + RefreshToken: refreshToken, + Expiry: time.Time{}.Add(time.Duration(5) * time.Second), + } +} + +func TestProviderData_verifyIDToken(t *testing.T) { + failureIDToken := defaultIDToken + failureIDToken.Id = failureTokenID + + testCases := map[string]struct { + IDToken *idTokenClaims + ExpectIDToken bool + ExpectedError error + }{ + "Valid ID Token": { + IDToken: &defaultIDToken, + ExpectIDToken: true, + ExpectedError: nil, + }, + "Invalid ID Token": { + IDToken: &failureIDToken, + ExpectIDToken: false, + ExpectedError: errors.New("failed to verify signature: the validation failed for subject [123456789]"), + }, + "Missing ID Token": { + IDToken: nil, + ExpectIDToken: false, + ExpectedError: nil, + }, + } + + for testName, tc := range testCases { + t.Run(testName, func(t *testing.T) { + g := NewWithT(t) + + token := newTestOauth2Token() + if tc.IDToken != nil { + idToken, err := newSignedTestIDToken(*tc.IDToken) + g.Expect(err).ToNot(HaveOccurred()) + token = token.WithExtra(map[string]interface{}{ + "id_token": idToken, + }) + } + + provider := &ProviderData{ + Verifier: oidc.NewVerifier( + oidcIssuer, + mockJWKS{}, + &oidc.Config{ClientID: oidcClientID}, + ), + } + verified, err := provider.verifyIDToken(context.Background(), token) + if err != nil { + g.Expect(err).To(Equal(tc.ExpectedError)) + } + + if tc.ExpectIDToken { + g.Expect(verified).ToNot(BeNil()) + g.Expect(*verified).To(BeAssignableToTypeOf(oidc.IDToken{})) + } else { + g.Expect(verified).To(BeNil()) + } + }) + } +} + +func TestProviderData_buildSessionFromClaims(t *testing.T) { + testCases := map[string]struct { + IDToken idTokenClaims + AllowUnverified bool + EmailClaim string + GroupsClaim string + ExpectedError error + ExpectedSession *sessions.SessionState + }{ + "Standard": { + IDToken: defaultIDToken, + AllowUnverified: false, + EmailClaim: "email", + GroupsClaim: "groups", + ExpectedSession: &sessions.SessionState{ + User: "123456789", + Email: "janed@me.com", + Groups: []string{"test:a", "test:b"}, + PreferredUsername: "Jane Dobbs", + }, + }, + "Unverified Denied": { + IDToken: unverifiedIDToken, + AllowUnverified: false, + EmailClaim: "email", + GroupsClaim: "groups", + ExpectedError: errors.New("email in id_token (unverified@email.com) isn't verified"), + }, + "Unverified Allowed": { + IDToken: unverifiedIDToken, + AllowUnverified: true, + EmailClaim: "email", + GroupsClaim: "groups", + ExpectedSession: &sessions.SessionState{ + User: "123456789", + Email: "unverified@email.com", + Groups: []string{"test:a", "test:b"}, + PreferredUsername: "Mystery Man", + }, + }, + "Complex Groups": { + IDToken: complexGroupsIDToken, + AllowUnverified: true, + EmailClaim: "email", + GroupsClaim: "groups", + ExpectedSession: &sessions.SessionState{ + User: "123456789", + Email: "complex@claims.com", + Groups: []string{"{\"groupId\":\"Admin Group Id\",\"roles\":[\"Admin\"]}"}, + PreferredUsername: "Complex Claim", + }, + }, + "Email Claim Switched": { + IDToken: unverifiedIDToken, + AllowUnverified: true, + EmailClaim: "phone_number", + GroupsClaim: "groups", + ExpectedSession: &sessions.SessionState{ + User: "123456789", + Email: "+4025205729", + Groups: []string{"test:a", "test:b"}, + PreferredUsername: "Mystery Man", + }, + }, + "Email Claim Switched to Non String": { + IDToken: unverifiedIDToken, + AllowUnverified: true, + EmailClaim: "roles", + GroupsClaim: "groups", + ExpectedSession: &sessions.SessionState{ + User: "123456789", + Email: "[test:c test:d]", + Groups: []string{"test:a", "test:b"}, + PreferredUsername: "Mystery Man", + }, + }, + "Email Claim Non Existent": { + IDToken: unverifiedIDToken, + AllowUnverified: true, + EmailClaim: "aksjdfhjksadh", + GroupsClaim: "groups", + ExpectedSession: &sessions.SessionState{ + User: "123456789", + Email: "", + Groups: []string{"test:a", "test:b"}, + PreferredUsername: "Mystery Man", + }, + }, + "Groups Claim Switched": { + IDToken: defaultIDToken, + AllowUnverified: false, + EmailClaim: "email", + GroupsClaim: "roles", + ExpectedSession: &sessions.SessionState{ + User: "123456789", + Email: "janed@me.com", + Groups: []string{"test:c", "test:d"}, + PreferredUsername: "Jane Dobbs", + }, + }, + "Groups Claim Non Existent": { + IDToken: defaultIDToken, + AllowUnverified: false, + EmailClaim: "email", + GroupsClaim: "alskdjfsalkdjf", + ExpectedSession: &sessions.SessionState{ + User: "123456789", + Email: "janed@me.com", + Groups: []string{}, + PreferredUsername: "Jane Dobbs", + }, + }, + } + for testName, tc := range testCases { + t.Run(testName, func(t *testing.T) { + g := NewWithT(t) + + provider := &ProviderData{ + Verifier: oidc.NewVerifier( + oidcIssuer, + mockJWKS{}, + &oidc.Config{ClientID: oidcClientID}, + ), + } + provider.AllowUnverifiedEmail = tc.AllowUnverified + provider.EmailClaim = tc.EmailClaim + provider.GroupsClaim = tc.GroupsClaim + + rawIDToken, err := newSignedTestIDToken(tc.IDToken) + g.Expect(err).ToNot(HaveOccurred()) + + idToken, err := provider.Verifier.Verify(context.Background(), rawIDToken) + g.Expect(err).ToNot(HaveOccurred()) + + ss, err := provider.buildSessionFromClaims(idToken) + if err != nil { + g.Expect(err).To(Equal(tc.ExpectedError)) + } + if ss != nil { + g.Expect(ss).To(Equal(tc.ExpectedSession)) + } + }) + } +} + +func TestProviderData_extractGroups(t *testing.T) { + testCases := map[string]struct { + Claims map[string]interface{} + GroupsClaim string + ExpectedGroups []string + }{ + "Standard String Groups": { + Claims: map[string]interface{}{ + "email": "this@does.not.matter.com", + "groups": []interface{}{"three", "string", "groups"}, + }, + GroupsClaim: "groups", + ExpectedGroups: []string{"three", "string", "groups"}, + }, + "Different Claim Name": { + Claims: map[string]interface{}{ + "email": "this@does.not.matter.com", + "roles": []interface{}{"three", "string", "roles"}, + }, + GroupsClaim: "roles", + ExpectedGroups: []string{"three", "string", "roles"}, + }, + "Numeric Groups": { + Claims: map[string]interface{}{ + "email": "this@does.not.matter.com", + "groups": []interface{}{1, 2, 3}, + }, + GroupsClaim: "groups", + ExpectedGroups: []string{"1", "2", "3"}, + }, + "Complex Groups": { + Claims: map[string]interface{}{ + "email": "this@does.not.matter.com", + "groups": []interface{}{ + map[string]interface{}{ + "groupId": "Admin Group Id", + "roles": []string{"Admin"}, + }, + 12345, + "Just::A::String", + }, + }, + GroupsClaim: "groups", + ExpectedGroups: []string{ + "{\"groupId\":\"Admin Group Id\",\"roles\":[\"Admin\"]}", + "12345", + "Just::A::String", + }, + }, + "Missing Groups": { + Claims: map[string]interface{}{ + "email": "this@does.not.matter.com", + }, + GroupsClaim: "groups", + ExpectedGroups: []string{}, + }, + } + for testName, tc := range testCases { + t.Run(testName, func(t *testing.T) { + g := NewWithT(t) + + provider := &ProviderData{ + Verifier: oidc.NewVerifier( + oidcIssuer, + mockJWKS{}, + &oidc.Config{ClientID: oidcClientID}, + ), + } + provider.GroupsClaim = tc.GroupsClaim + + groups := provider.extractGroups(tc.Claims) + g.Expect(groups).To(Equal(tc.ExpectedGroups)) + }) + } +} diff --git a/providers/provider_default.go b/providers/provider_default.go index 012a538c..69dddb06 100644 --- a/providers/provider_default.go +++ b/providers/provider_default.go @@ -13,6 +13,8 @@ import ( "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests" ) +const emailClaim = "email" + var ( // ErrNotImplemented is returned when a provider did not override a default // implementation method that doesn't have sensible defaults From eb56f24d6dba6170fde7ec74731e105ac57d947f Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Sat, 28 Nov 2020 12:33:05 -0800 Subject: [PATCH 3/6] Deprecate UserIDClaim in config and docs --- CHANGELOG.md | 2 ++ docs/docs/configuration/overview.md | 4 ++-- pkg/apis/options/options.go | 11 +++++++---- pkg/validation/options.go | 12 ++++++++---- providers/oidc.go | 2 +- providers/provider_data.go | 7 ++++++- providers/provider_default.go | 2 -- 7 files changed, 26 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f6065b4..74c272eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ## Important Notes +- [#936](https://github.com/oauth2-proxy/oauth2-proxy/pull/936) `--user-id-claim` option is deprecated and replaced by `--oidc-email-claim` - [#630](https://github.com/oauth2-proxy/oauth2-proxy/pull/630) Gitlab projects needs a Gitlab application with the extra `read_api` enabled - [#905](https://github.com/oauth2-proxy/oauth2-proxy/pull/905) Existing sessions from v6.0.0 or earlier are no longer valid. They will trigger a reauthentication. - [#826](https://github.com/oauth2-proxy/oauth2-proxy/pull/826) `skip-auth-strip-headers` now applies to all requests, not just those where authentication would be skipped. @@ -47,6 +48,7 @@ - [#630](https://github.com/oauth2-proxy/oauth2-proxy/pull/630) Add support for Gitlab project based authentication (@factorysh) - [#907](https://github.com/oauth2-proxy/oauth2-proxy/pull/907) Introduce alpha configuration option to enable testing of structured configuration (@JoelSpeed) - [#938](https://github.com/oauth2-proxy/oauth2-proxy/pull/938) Cleanup missed provider renaming refactor methods (@NickMeves) +- [#936](https://github.com/oauth2-proxy/oauth2-proxy/pull/936) Refactor OIDC Provider and support groups from Profile URL (@NickMeves) - [#925](https://github.com/oauth2-proxy/oauth2-proxy/pull/925) Fix basic auth legacy header conversion (@JoelSpeed) - [#916](https://github.com/oauth2-proxy/oauth2-proxy/pull/916) Add AlphaOptions struct to prepare for alpha config loading (@JoelSpeed) - [#923](https://github.com/oauth2-proxy/oauth2-proxy/pull/923) Support TLS 1.3 (@aajisaka) diff --git a/docs/docs/configuration/overview.md b/docs/docs/configuration/overview.md index 46e72d67..a91c6264 100644 --- a/docs/docs/configuration/overview.md +++ b/docs/docs/configuration/overview.md @@ -74,7 +74,8 @@ An example [oauth2-proxy.cfg](https://github.com/oauth2-proxy/oauth2-proxy/blob/ | `--insecure-oidc-skip-issuer-verification` | bool | allow the OIDC issuer URL to differ from the expected (currently required for Azure multi-tenant compatibility) | false | | `--oidc-issuer-url` | string | the OpenID Connect issuer URL, e.g. `"https://accounts.google.com"` | | | `--oidc-jwks-url` | string | OIDC JWKS URI for token verification; required if OIDC discovery is disabled | | -| `--oidc-groups-claim` | string | which claim contains the user groups | `"groups"` | +| `--oidc-email-claim` | string | which OIDC claim contains the user's email | `"email"` | +| `--oidc-groups-claim` | string | which OIDC claim contains the user groups | `"groups"` | | `--pass-access-token` | bool | pass OAuth access_token to upstream via X-Forwarded-Access-Token header. When used with `--set-xauthrequest` this adds the X-Auth-Request-Access-Token header to the response | false | | `--pass-authorization-header` | bool | pass OIDC IDToken to upstream via Authorization Bearer header | false | | `--pass-basic-auth` | bool | pass HTTP Basic Auth, X-Forwarded-User, X-Forwarded-Email and X-Forwarded-Preferred-Username information to upstream | true | @@ -128,7 +129,6 @@ An example [oauth2-proxy.cfg](https://github.com/oauth2-proxy/oauth2-proxy/blob/ | `--tls-cert-file` | string | path to certificate file | | | `--tls-key-file` | string | path to private key file | | | `--upstream` | string \| list | the http url(s) of the upstream endpoint, file:// paths for static files or `static://` for static response. Routing is based on the path | | -| `--user-id-claim` | string | which claim contains the user ID | \["email"\] | | `--allowed-group` | string \| list | restrict logins to members of this group (may be given multiple times) | | | `--validate-url` | string | Access token validation endpoint | | | `--version` | n/a | print version string | | diff --git a/pkg/apis/options/options.go b/pkg/apis/options/options.go index cf4e2414..46cdcedb 100644 --- a/pkg/apis/options/options.go +++ b/pkg/apis/options/options.go @@ -87,6 +87,7 @@ type Options struct { InsecureOIDCSkipIssuerVerification bool `flag:"insecure-oidc-skip-issuer-verification" cfg:"insecure_oidc_skip_issuer_verification"` SkipOIDCDiscovery bool `flag:"skip-oidc-discovery" cfg:"skip_oidc_discovery"` OIDCJwksURL string `flag:"oidc-jwks-url" cfg:"oidc_jwks_url"` + OIDCEmailClaim string `flag:"oidc-email-claim" cfg:"oidc_email_claim"` OIDCGroupsClaim string `flag:"oidc-groups-claim" cfg:"oidc_groups_claim"` LoginURL string `flag:"login-url" cfg:"login_url"` RedeemURL string `flag:"redeem-url" cfg:"redeem_url"` @@ -148,11 +149,12 @@ func NewOptions() *Options { SkipAuthPreflight: false, Prompt: "", // Change to "login" when ApprovalPrompt officially deprecated ApprovalPrompt: "force", - UserIDClaim: "email", InsecureOIDCAllowUnverifiedEmail: false, SkipOIDCDiscovery: false, Logging: loggingDefaults(), - OIDCGroupsClaim: "groups", + UserIDClaim: providers.OIDCEmailClaim, // Deprecated: Use OIDCEmailClaim + OIDCEmailClaim: providers.OIDCEmailClaim, + OIDCGroupsClaim: providers.OIDCGroupsClaim, } } @@ -226,7 +228,8 @@ func NewFlagSet() *pflag.FlagSet { flagSet.Bool("insecure-oidc-skip-issuer-verification", false, "Do not verify if issuer matches OIDC discovery URL") flagSet.Bool("skip-oidc-discovery", false, "Skip OIDC discovery and use manually supplied Endpoints") flagSet.String("oidc-jwks-url", "", "OpenID Connect JWKS URL (ie: https://www.googleapis.com/oauth2/v3/certs)") - flagSet.String("oidc-groups-claim", "groups", "which claim contains the user groups") + flagSet.String("oidc-groups-claim", providers.OIDCGroupsClaim, "which OIDC claim contains the user groups") + flagSet.String("oidc-email-claim", providers.OIDCEmailClaim, "which OIDC claim contains the user's email") flagSet.String("login-url", "", "Authentication endpoint") flagSet.String("redeem-url", "", "Token redemption endpoint") flagSet.String("profile-url", "", "Profile access endpoint") @@ -243,7 +246,7 @@ func NewFlagSet() *pflag.FlagSet { flagSet.String("pubjwk-url", "", "JWK pubkey access endpoint: required by login.gov") flagSet.Bool("gcp-healthchecks", false, "Enable GCP/GKE healthcheck endpoints") - flagSet.String("user-id-claim", "email", "which claim contains the user ID") + flagSet.String("user-id-claim", providers.OIDCEmailClaim, "(DEPRECATED for `oidc-email-claim`) which claim contains the user ID") flagSet.StringSlice("allowed-group", []string{}, "restrict logins to members of this group (may be given multiple times)") flagSet.AddFlagSet(cookieFlagSet()) diff --git a/pkg/validation/options.go b/pkg/validation/options.go index 652ada9e..d5e58312 100644 --- a/pkg/validation/options.go +++ b/pkg/validation/options.go @@ -235,10 +235,17 @@ func parseProviderInfo(o *options.Options, msgs []string) []string { // Make the OIDC options available to all providers that support it p.AllowUnverifiedEmail = o.InsecureOIDCAllowUnverifiedEmail - p.EmailClaim = o.UserIDClaim + p.EmailClaim = o.OIDCEmailClaim p.GroupsClaim = o.OIDCGroupsClaim p.Verifier = o.GetOIDCVerifier() + // TODO (@NickMeves) - Remove This + // Backwards Compatibility for Deprecated UserIDClaim option + if o.OIDCEmailClaim == providers.OIDCEmailClaim && + o.UserIDClaim != providers.OIDCEmailClaim { + p.EmailClaim = o.UserIDClaim + } + p.SetAllowedGroups(o.AllowedGroups) provider := providers.New(o.ProviderType, p) @@ -276,9 +283,6 @@ func parseProviderInfo(o *options.Options, msgs []string) []string { p.SetTeam(o.BitbucketTeam) p.SetRepository(o.BitbucketRepository) case *providers.OIDCProvider: - p.AllowUnverifiedEmail = o.InsecureOIDCAllowUnverifiedEmail - p.EmailClaim = o.UserIDClaim - p.GroupsClaim = o.OIDCGroupsClaim if p.Verifier == nil { msgs = append(msgs, "oidc provider requires an oidc issuer URL") } diff --git a/providers/oidc.go b/providers/oidc.go index f90348d6..d7d34700 100644 --- a/providers/oidc.go +++ b/providers/oidc.go @@ -42,7 +42,7 @@ func (p *OIDCProvider) Redeem(ctx context.Context, redirectURL, code string) (*s } token, err := c.Exchange(ctx, code) if err != nil { - return nil, fmt.Errorf("token exchange failure: %v", err) + return nil, fmt.Errorf("token exchange failed: %v", err) } return p.createSession(ctx, token, false) diff --git a/providers/provider_data.go b/providers/provider_data.go index 09eadd25..ae434515 100644 --- a/providers/provider_data.go +++ b/providers/provider_data.go @@ -15,6 +15,11 @@ import ( "golang.org/x/oauth2" ) +const ( + OIDCEmailClaim = "email" + OIDCGroupsClaim = "groups" +) + // ProviderData contains information required to configure all implementations // of OAuth2 providers type ProviderData struct { @@ -154,7 +159,7 @@ func (p *ProviderData) buildSessionFromClaims(idToken *oidc.IDToken) (*sessions. // `email_verified` must be present and explicitly set to `false` to be // considered unverified. - verifyEmail := (p.EmailClaim == emailClaim) && !p.AllowUnverifiedEmail + verifyEmail := (p.EmailClaim == OIDCEmailClaim) && !p.AllowUnverifiedEmail if verifyEmail && claims.Verified != nil && !*claims.Verified { return nil, fmt.Errorf("email in id_token (%s) isn't verified", claims.Email) } diff --git a/providers/provider_default.go b/providers/provider_default.go index 69dddb06..012a538c 100644 --- a/providers/provider_default.go +++ b/providers/provider_default.go @@ -13,8 +13,6 @@ import ( "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests" ) -const emailClaim = "email" - var ( // ErrNotImplemented is returned when a provider did not override a default // implementation method that doesn't have sensible defaults From ea5b8cc21fb6c8feac6226b1131c45e9a173df2f Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Sun, 29 Nov 2020 14:58:01 -0800 Subject: [PATCH 4/6] Support non-list and complex groups --- CHANGELOG.md | 1 + providers/oidc.go | 18 +++--- providers/oidc_test.go | 107 ++++++++++++++++++++++++++++---- providers/provider_data.go | 36 +++++++---- providers/provider_data_test.go | 20 ++++-- providers/util.go | 20 ++++++ 6 files changed, 166 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 74c272eb..84f54dec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,7 @@ - [#630](https://github.com/oauth2-proxy/oauth2-proxy/pull/630) Add support for Gitlab project based authentication (@factorysh) - [#907](https://github.com/oauth2-proxy/oauth2-proxy/pull/907) Introduce alpha configuration option to enable testing of structured configuration (@JoelSpeed) - [#938](https://github.com/oauth2-proxy/oauth2-proxy/pull/938) Cleanup missed provider renaming refactor methods (@NickMeves) +- [#816](https://github.com/oauth2-proxy/oauth2-proxy/pull/816) (via [#936](https://github.com/oauth2-proxy/oauth2-proxy/pull/936)) Support non-list group claims (@loafoe) - [#936](https://github.com/oauth2-proxy/oauth2-proxy/pull/936) Refactor OIDC Provider and support groups from Profile URL (@NickMeves) - [#925](https://github.com/oauth2-proxy/oauth2-proxy/pull/925) Fix basic auth legacy header conversion (@JoelSpeed) - [#916](https://github.com/oauth2-proxy/oauth2-proxy/pull/916) Add AlphaOptions struct to prepare for alpha config loading (@JoelSpeed) diff --git a/providers/oidc.go b/providers/oidc.go index d7d34700..98cefb4b 100644 --- a/providers/oidc.go +++ b/providers/oidc.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "reflect" "time" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" @@ -59,7 +60,7 @@ func (p *OIDCProvider) EnrichSession(ctx context.Context, s *sessions.SessionSta } // Try to get missing emails or groups from a profileURL - if s.Email == "" || len(s.Groups) == 0 { + if s.Email == "" || s.Groups == nil { err := p.callProfileURL(ctx, s) if err != nil { logger.Errorf("Warning: Profile URL request failed: %v", err) @@ -90,16 +91,15 @@ func (p *OIDCProvider) callProfileURL(ctx context.Context, s *sessions.SessionSt s.Email = email } - // Handle array & singleton groups cases if len(s.Groups) == 0 { - groups, err := respJSON.Get(p.GroupsClaim).StringArray() - if err == nil { - s.Groups = groups - } else { - group, err := respJSON.Get(p.GroupsClaim).String() - if err == nil { - s.Groups = []string{group} + for _, group := range coerceArray(respJSON, p.GroupsClaim) { + formatted, err := formatGroup(group) + if err != nil { + logger.Errorf("Warning: unable to format group of type %s with error %s", + reflect.TypeOf(group), err) + continue } + s.Groups = append(s.Groups, formatted) } } diff --git a/providers/oidc_test.go b/providers/oidc_test.go index 2651b4ea..7ac98634 100644 --- a/providers/oidc_test.go +++ b/providers/oidc_test.go @@ -68,7 +68,7 @@ func newOIDCServer(body []byte) (*url.URL, *httptest.Server) { return u, s } -func newTestSetup(body []byte) (*httptest.Server, *OIDCProvider) { +func newTestOIDCSetup(body []byte) (*httptest.Server, *OIDCProvider) { redeemURL, server := newOIDCServer(body) provider := newOIDCProvider(redeemURL) return server, provider @@ -85,7 +85,7 @@ func TestOIDCProviderRedeem(t *testing.T) { IDToken: idToken, }) - server, provider := newTestSetup(body) + server, provider := newTestOIDCSetup(body) defer server.Close() session, err := provider.Redeem(context.Background(), provider.RedeemURL.String(), "code1234") @@ -108,7 +108,7 @@ func TestOIDCProviderRedeem_custom_userid(t *testing.T) { IDToken: idToken, }) - server, provider := newTestSetup(body) + server, provider := newTestOIDCSetup(body) provider.EmailClaim = "phone_number" defer server.Close() @@ -247,7 +247,7 @@ func TestOIDCProvider_EnrichSession(t *testing.T) { ExistingSession: &sessions.SessionState{ User: "already", Email: "already@populated.com", - Groups: []string{}, + Groups: nil, IDToken: idToken, AccessToken: accessToken, RefreshToken: refreshToken, @@ -268,6 +268,89 @@ func TestOIDCProvider_EnrichSession(t *testing.T) { RefreshToken: refreshToken, }, }, + "Missing Groups with Complex Groups in Profile URL": { + ExistingSession: &sessions.SessionState{ + User: "already", + Email: "already@populated.com", + Groups: nil, + IDToken: idToken, + AccessToken: accessToken, + RefreshToken: refreshToken, + }, + EmailClaim: "email", + GroupsClaim: "groups", + ProfileJSON: map[string]interface{}{ + "email": "new@thing.com", + "groups": []map[string]interface{}{ + { + "groupId": "Admin Group Id", + "roles": []string{"Admin"}, + }, + }, + }, + ExpectedError: nil, + ExpectedSession: &sessions.SessionState{ + User: "already", + Email: "already@populated.com", + Groups: []string{"{\"groupId\":\"Admin Group Id\",\"roles\":[\"Admin\"]}"}, + IDToken: idToken, + AccessToken: accessToken, + RefreshToken: refreshToken, + }, + }, + "Missing Groups with Singleton Complex Group in Profile URL": { + ExistingSession: &sessions.SessionState{ + User: "already", + Email: "already@populated.com", + Groups: nil, + IDToken: idToken, + AccessToken: accessToken, + RefreshToken: refreshToken, + }, + EmailClaim: "email", + GroupsClaim: "groups", + ProfileJSON: map[string]interface{}{ + "email": "new@thing.com", + "groups": map[string]interface{}{ + "groupId": "Admin Group Id", + "roles": []string{"Admin"}, + }, + }, + ExpectedError: nil, + ExpectedSession: &sessions.SessionState{ + User: "already", + Email: "already@populated.com", + Groups: []string{"{\"groupId\":\"Admin Group Id\",\"roles\":[\"Admin\"]}"}, + IDToken: idToken, + AccessToken: accessToken, + RefreshToken: refreshToken, + }, + }, + "Empty Groups Claims": { + ExistingSession: &sessions.SessionState{ + User: "already", + Email: "already@populated.com", + Groups: []string{}, + IDToken: idToken, + AccessToken: accessToken, + RefreshToken: refreshToken, + }, + EmailClaim: "email", + GroupsClaim: "groups", + ProfileJSON: map[string]interface{}{ + "email": "new@thing.com", + "groups": []string{"new", "thing"}, + }, + ExpectedError: nil, + ExpectedSession: &sessions.SessionState{ + User: "already", + Email: "already@populated.com", + Groups: []string{}, + IDToken: idToken, + AccessToken: accessToken, + RefreshToken: refreshToken, + }, + }, "Missing Groups with Custom Claim": { ExistingSession: &sessions.SessionState{ User: "already", @@ -297,7 +380,7 @@ func TestOIDCProvider_EnrichSession(t *testing.T) { ExistingSession: &sessions.SessionState{ User: "already", Email: "already@populated.com", - Groups: []string{}, + Groups: nil, IDToken: idToken, AccessToken: accessToken, RefreshToken: refreshToken, @@ -346,7 +429,7 @@ func TestOIDCProvider_EnrichSession(t *testing.T) { jsonResp, err := json.Marshal(tc.ProfileJSON) assert.NoError(t, err) - server, provider := newTestSetup(jsonResp) + server, provider := newTestOIDCSetup(jsonResp) provider.ProfileURL, err = url.Parse(server.URL) assert.NoError(t, err) @@ -371,7 +454,7 @@ func TestOIDCProviderRefreshSessionIfNeededWithoutIdToken(t *testing.T) { RefreshToken: refreshToken, }) - server, provider := newTestSetup(body) + server, provider := newTestOIDCSetup(body) defer server.Close() existingSession := &sessions.SessionState{ @@ -405,7 +488,7 @@ func TestOIDCProviderRefreshSessionIfNeededWithIdToken(t *testing.T) { IDToken: idToken, }) - server, provider := newTestSetup(body) + server, provider := newTestOIDCSetup(body) defer server.Close() existingSession := &sessions.SessionState{ @@ -433,7 +516,7 @@ func TestOIDCProviderCreateSessionFromToken(t *testing.T) { GroupsClaim string ExpectedUser string ExpectedEmail string - ExpectedGroups interface{} + ExpectedGroups []string }{ "Default IDToken": { IDToken: defaultIDToken, @@ -447,7 +530,7 @@ func TestOIDCProviderCreateSessionFromToken(t *testing.T) { GroupsClaim: "groups", ExpectedUser: "123456789", ExpectedEmail: "123456789", - ExpectedGroups: []string{}, + ExpectedGroups: nil, }, "Custom Groups Claim": { IDToken: defaultIDToken, @@ -466,7 +549,7 @@ func TestOIDCProviderCreateSessionFromToken(t *testing.T) { } for testName, tc := range testCases { t.Run(testName, func(t *testing.T) { - server, provider := newTestSetup([]byte(`{}`)) + server, provider := newTestOIDCSetup([]byte(`{}`)) provider.GroupsClaim = tc.GroupsClaim defer server.Close() @@ -478,9 +561,9 @@ func TestOIDCProviderCreateSessionFromToken(t *testing.T) { assert.Equal(t, tc.ExpectedUser, ss.User) assert.Equal(t, tc.ExpectedEmail, ss.Email) + assert.Equal(t, tc.ExpectedGroups, ss.Groups) assert.Equal(t, rawIDToken, ss.IDToken) assert.Equal(t, rawIDToken, ss.AccessToken) - assert.Equal(t, tc.ExpectedGroups, ss.Groups) assert.Equal(t, "", ss.RefreshToken) }) } diff --git a/providers/provider_data.go b/providers/provider_data.go index ae434515..098e6192 100644 --- a/providers/provider_data.go +++ b/providers/provider_data.go @@ -189,20 +189,34 @@ func (p *ProviderData) getClaims(idToken *oidc.IDToken) (*OIDCClaims, error) { return claims, nil } -// extractGroups extracts groups from a claim to a list in a type safe manner +// extractGroups extracts groups from a claim to a list in a type safe manner. +// If the claim isn't present, `nil` is returned. If the groups claim is +// present but empty, `[]string{}` is returned. func (p *ProviderData) extractGroups(claims map[string]interface{}) []string { + rawClaim, ok := claims[p.GroupsClaim] + if !ok { + return nil + } + + // Handle traditional list-based groups as well as non-standard singleton + // based groups. Both variants support complex objects if needed. + var claimGroups []interface{} + switch raw := rawClaim.(type) { + case []interface{}: + claimGroups = raw + case interface{}: + claimGroups = []interface{}{raw} + } + groups := []string{} - rawGroups, ok := claims[p.GroupsClaim].([]interface{}) - if rawGroups != nil && ok { - for _, rawGroup := range rawGroups { - formattedGroup, err := formatGroup(rawGroup) - if err != nil { - logger.Errorf("Warning: unable to format group of type %s with error %s", - reflect.TypeOf(rawGroup), err) - continue - } - groups = append(groups, formattedGroup) + for _, rawGroup := range claimGroups { + formattedGroup, err := formatGroup(rawGroup) + if err != nil { + logger.Errorf("Warning: unable to format group of type %s with error %s", + reflect.TypeOf(rawGroup), err) + continue } + groups = append(groups, formattedGroup) } return groups } diff --git a/providers/provider_data_test.go b/providers/provider_data_test.go index 4aed73eb..f94c0db1 100644 --- a/providers/provider_data_test.go +++ b/providers/provider_data_test.go @@ -300,7 +300,7 @@ func TestProviderData_buildSessionFromClaims(t *testing.T) { ExpectedSession: &sessions.SessionState{ User: "123456789", Email: "janed@me.com", - Groups: []string{}, + Groups: nil, PreferredUsername: "Jane Dobbs", }, }, @@ -386,12 +386,20 @@ func TestProviderData_extractGroups(t *testing.T) { "Just::A::String", }, }, - "Missing Groups": { + "Missing Groups Claim Returns Nil": { Claims: map[string]interface{}{ "email": "this@does.not.matter.com", }, GroupsClaim: "groups", - ExpectedGroups: []string{}, + ExpectedGroups: nil, + }, + "Non List Groups": { + Claims: map[string]interface{}{ + "email": "this@does.not.matter.com", + "groups": "singleton", + }, + GroupsClaim: "groups", + ExpectedGroups: []string{"singleton"}, }, } for testName, tc := range testCases { @@ -408,7 +416,11 @@ func TestProviderData_extractGroups(t *testing.T) { provider.GroupsClaim = tc.GroupsClaim groups := provider.extractGroups(tc.Claims) - g.Expect(groups).To(Equal(tc.ExpectedGroups)) + if tc.ExpectedGroups != nil { + g.Expect(groups).To(Equal(tc.ExpectedGroups)) + } else { + g.Expect(groups).To(BeNil()) + } }) } } diff --git a/providers/util.go b/providers/util.go index acf20902..055d29db 100644 --- a/providers/util.go +++ b/providers/util.go @@ -6,6 +6,7 @@ import ( "net/http" "net/url" + "github.com/bitly/go-simplejson" "golang.org/x/oauth2" ) @@ -59,6 +60,8 @@ func makeLoginURL(p *ProviderData, redirectURI, state string, extraParams url.Va return a } +// getIDToken extracts an IDToken stored in the `Extra` fields of an +// oauth2.Token func getIDToken(token *oauth2.Token) string { idToken, ok := token.Extra("id_token").(string) if !ok { @@ -67,6 +70,8 @@ func getIDToken(token *oauth2.Token) string { return idToken } +// formatGroup coerces an OIDC groups claim into a string +// If it is non-string, marshal it into JSON. func formatGroup(rawGroup interface{}) (string, error) { group, ok := rawGroup.(string) if !ok { @@ -78,3 +83,18 @@ func formatGroup(rawGroup interface{}) (string, error) { } return group, nil } + +// coerceArray extracts a field from simplejson.Json that might be a +// singleton or a list and coerces it into a list. +func coerceArray(sj *simplejson.Json, key string) []interface{} { + array, err := sj.Get(key).Array() + if err == nil { + return array + } + + single := sj.Get(key).Interface() + if single == nil { + return nil + } + return []interface{}{single} +} From 42f6cef7d6571aa0ea5fea109a77177a069a950e Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Tue, 1 Dec 2020 12:01:42 -0800 Subject: [PATCH 5/6] Improve OIDC error handling --- providers/oidc.go | 15 +++++++++------ providers/provider_data.go | 5 ++++- providers/provider_data_test.go | 19 +++++++++++++++---- providers/provider_default.go | 8 ++++++++ 4 files changed, 36 insertions(+), 11 deletions(-) diff --git a/providers/oidc.go b/providers/oidc.go index 98cefb4b..cdeee3b2 100644 --- a/providers/oidc.go +++ b/providers/oidc.go @@ -206,12 +206,15 @@ func (p *OIDCProvider) CreateSessionFromToken(ctx context.Context, token string) func (p *OIDCProvider) createSession(ctx context.Context, token *oauth2.Token, refresh bool) (*sessions.SessionState, error) { idToken, err := p.verifyIDToken(ctx, token) if err != nil { - return nil, fmt.Errorf("could not verify id_token: %v", err) - } - - // IDToken is mandatory in Redeem but optional in Refresh - if idToken == nil && !refresh { - return nil, errors.New("token response did not contain an id_token") + switch err { + case ErrMissingIDToken: + // IDToken is mandatory in Redeem but optional in Refresh + if !refresh { + return nil, errors.New("token response did not contain an id_token") + } + default: + return nil, fmt.Errorf("could not verify id_token: %v", err) + } } ss, err := p.buildSessionFromClaims(idToken) diff --git a/providers/provider_data.go b/providers/provider_data.go index 098e6192..d8c9312b 100644 --- a/providers/provider_data.go +++ b/providers/provider_data.go @@ -129,9 +129,12 @@ type OIDCClaims struct { func (p *ProviderData) verifyIDToken(ctx context.Context, token *oauth2.Token) (*oidc.IDToken, error) { rawIDToken := getIDToken(token) if strings.TrimSpace(rawIDToken) != "" { + if p.Verifier == nil { + return nil, ErrMissingOIDCVerifier + } return p.Verifier.Verify(ctx, rawIDToken) } - return nil, nil + return nil, ErrMissingIDToken } // buildSessionFromClaims uses IDToken claims to populate a fresh SessionState diff --git a/providers/provider_data_test.go b/providers/provider_data_test.go index f94c0db1..80f6ecab 100644 --- a/providers/provider_data_test.go +++ b/providers/provider_data_test.go @@ -137,23 +137,33 @@ func TestProviderData_verifyIDToken(t *testing.T) { testCases := map[string]struct { IDToken *idTokenClaims + Verifier bool ExpectIDToken bool ExpectedError error }{ "Valid ID Token": { IDToken: &defaultIDToken, + Verifier: true, ExpectIDToken: true, ExpectedError: nil, }, "Invalid ID Token": { IDToken: &failureIDToken, + Verifier: true, ExpectIDToken: false, ExpectedError: errors.New("failed to verify signature: the validation failed for subject [123456789]"), }, "Missing ID Token": { IDToken: nil, + Verifier: true, ExpectIDToken: false, - ExpectedError: nil, + ExpectedError: ErrMissingIDToken, + }, + "OIDC Verifier not Configured": { + IDToken: &defaultIDToken, + Verifier: false, + ExpectIDToken: false, + ExpectedError: ErrMissingOIDCVerifier, }, } @@ -170,12 +180,13 @@ func TestProviderData_verifyIDToken(t *testing.T) { }) } - provider := &ProviderData{ - Verifier: oidc.NewVerifier( + provider := &ProviderData{} + if tc.Verifier { + provider.Verifier = oidc.NewVerifier( oidcIssuer, mockJWKS{}, &oidc.Config{ClientID: oidcClientID}, - ), + ) } verified, err := provider.verifyIDToken(context.Background(), token) if err != nil { diff --git a/providers/provider_default.go b/providers/provider_default.go index 012a538c..d3c6d113 100644 --- a/providers/provider_default.go +++ b/providers/provider_default.go @@ -22,6 +22,14 @@ var ( // code ErrMissingCode = errors.New("missing code") + // ErrMissingIDToken is returned when an oidc.Token does not contain the + // extra `id_token` field for an IDToken. + ErrMissingIDToken = errors.New("missing id_token") + + // ErrMissingOIDCVerifier is returned when a provider didn't set `Verifier` + // but an attempt to call `Verifier.Verify` was about to be made. + ErrMissingOIDCVerifier = errors.New("oidc verifier is not configured") + _ Provider = (*ProviderData)(nil) ) From d2ffef2c7e0a622295c42cb2fdd70ef14593d03c Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Tue, 1 Dec 2020 17:50:27 -0800 Subject: [PATCH 6/6] Use global OIDC fields for Gitlab --- pkg/validation/options.go | 1 - providers/gitlab.go | 25 +++++++++++-------------- providers/oidc.go | 29 +++++++++++++++-------------- providers/provider_data.go | 12 ++++++------ providers/util.go | 16 ++++++++-------- 5 files changed, 40 insertions(+), 43 deletions(-) diff --git a/pkg/validation/options.go b/pkg/validation/options.go index d5e58312..4fc0b0a4 100644 --- a/pkg/validation/options.go +++ b/pkg/validation/options.go @@ -287,7 +287,6 @@ func parseProviderInfo(o *options.Options, msgs []string) []string { msgs = append(msgs, "oidc provider requires an oidc issuer URL") } case *providers.GitLabProvider: - p.AllowUnverifiedEmail = o.InsecureOIDCAllowUnverifiedEmail p.Groups = o.GitLabGroup err := p.AddProjects(o.GitlabProjects) if err != nil { diff --git a/providers/gitlab.go b/providers/gitlab.go index 246dd78c..c5922abb 100644 --- a/providers/gitlab.go +++ b/providers/gitlab.go @@ -20,8 +20,6 @@ type GitLabProvider struct { Groups []string Projects []*GitlabProject - - AllowUnverifiedEmail bool } // GitlabProject represents a Gitlab project constraint entity @@ -103,7 +101,7 @@ func (p *GitLabProvider) Redeem(ctx context.Context, redirectURL, code string) ( if err != nil { return nil, fmt.Errorf("token exchange: %v", err) } - s, err = p.createSessionState(ctx, token) + s, err = p.createSession(ctx, token) if err != nil { return nil, fmt.Errorf("unable to update session: %v", err) } @@ -162,7 +160,7 @@ func (p *GitLabProvider) redeemRefreshToken(ctx context.Context, s *sessions.Ses if err != nil { return fmt.Errorf("failed to get token: %v", err) } - newSession, err := p.createSessionState(ctx, token) + newSession, err := p.createSession(ctx, token) if err != nil { return fmt.Errorf("unable to update session: %v", err) } @@ -255,22 +253,21 @@ func (p *GitLabProvider) AddProjects(projects []string) error { return nil } -func (p *GitLabProvider) createSessionState(ctx context.Context, token *oauth2.Token) (*sessions.SessionState, error) { - rawIDToken, ok := token.Extra("id_token").(string) - if !ok { - return nil, fmt.Errorf("token response did not contain an id_token") - } - - // Parse and verify ID Token payload. - idToken, err := p.Verifier.Verify(ctx, rawIDToken) +func (p *GitLabProvider) createSession(ctx context.Context, token *oauth2.Token) (*sessions.SessionState, error) { + idToken, err := p.verifyIDToken(ctx, token) if err != nil { - return nil, fmt.Errorf("could not verify id_token: %v", err) + switch err { + case ErrMissingIDToken: + return nil, fmt.Errorf("token response did not contain an id_token") + default: + return nil, fmt.Errorf("could not verify id_token: %v", err) + } } created := time.Now() return &sessions.SessionState{ AccessToken: token.AccessToken, - IDToken: rawIDToken, + IDToken: getIDToken(token), RefreshToken: token.RefreshToken, CreatedAt: &created, ExpiresOn: &idToken.Expiry, diff --git a/providers/oidc.go b/providers/oidc.go index cdeee3b2..df133f4d 100644 --- a/providers/oidc.go +++ b/providers/oidc.go @@ -49,7 +49,7 @@ func (p *OIDCProvider) Redeem(ctx context.Context, redirectURL, code string) (*s return p.createSession(ctx, token, false) } -// EnrichSessionState is called after Redeem to allow providers to enrich session fields +// EnrichSession is called after Redeem to allow providers to enrich session fields // such as User, Email, Groups with provider specific API calls. func (p *OIDCProvider) EnrichSession(ctx context.Context, s *sessions.SessionState) error { if p.ProfileURL.String() == "" { @@ -61,7 +61,7 @@ func (p *OIDCProvider) EnrichSession(ctx context.Context, s *sessions.SessionSta // Try to get missing emails or groups from a profileURL if s.Email == "" || s.Groups == nil { - err := p.callProfileURL(ctx, s) + err := p.enrichFromProfileURL(ctx, s) if err != nil { logger.Errorf("Warning: Profile URL request failed: %v", err) } @@ -74,9 +74,9 @@ func (p *OIDCProvider) EnrichSession(ctx context.Context, s *sessions.SessionSta return nil } -// callProfileURL enriches a session's Email & Groups via the JSON response of +// enrichFromProfileURL enriches a session's Email & Groups via the JSON response of // an OIDC profile URL -func (p *OIDCProvider) callProfileURL(ctx context.Context, s *sessions.SessionState) error { +func (p *OIDCProvider) enrichFromProfileURL(ctx context.Context, s *sessions.SessionState) error { respJSON, err := requests.New(p.ProfileURL.String()). WithContext(ctx). WithHeaders(makeOIDCHeader(s.AccessToken)). @@ -91,22 +91,23 @@ func (p *OIDCProvider) callProfileURL(ctx context.Context, s *sessions.SessionSt s.Email = email } - if len(s.Groups) == 0 { - for _, group := range coerceArray(respJSON, p.GroupsClaim) { - formatted, err := formatGroup(group) - if err != nil { - logger.Errorf("Warning: unable to format group of type %s with error %s", - reflect.TypeOf(group), err) - continue - } - s.Groups = append(s.Groups, formatted) + if len(s.Groups) > 0 { + return nil + } + for _, group := range coerceArray(respJSON, p.GroupsClaim) { + formatted, err := formatGroup(group) + if err != nil { + logger.Errorf("Warning: unable to format group of type %s with error %s", + reflect.TypeOf(group), err) + continue } + s.Groups = append(s.Groups, formatted) } return nil } -// ValidateSessionState checks that the session's IDToken is still valid +// ValidateSession checks that the session's IDToken is still valid func (p *OIDCProvider) ValidateSession(ctx context.Context, s *sessions.SessionState) bool { _, err := p.Verifier.Verify(ctx, s.IDToken) return err == nil diff --git a/providers/provider_data.go b/providers/provider_data.go index d8c9312b..a9a41232 100644 --- a/providers/provider_data.go +++ b/providers/provider_data.go @@ -128,13 +128,13 @@ type OIDCClaims struct { func (p *ProviderData) verifyIDToken(ctx context.Context, token *oauth2.Token) (*oidc.IDToken, error) { rawIDToken := getIDToken(token) - if strings.TrimSpace(rawIDToken) != "" { - if p.Verifier == nil { - return nil, ErrMissingOIDCVerifier - } - return p.Verifier.Verify(ctx, rawIDToken) + if strings.TrimSpace(rawIDToken) == "" { + return nil, ErrMissingIDToken } - return nil, ErrMissingIDToken + if p.Verifier == nil { + return nil, ErrMissingOIDCVerifier + } + return p.Verifier.Verify(ctx, rawIDToken) } // buildSessionFromClaims uses IDToken claims to populate a fresh SessionState diff --git a/providers/util.go b/providers/util.go index 055d29db..e6fdc344 100644 --- a/providers/util.go +++ b/providers/util.go @@ -73,15 +73,15 @@ func getIDToken(token *oauth2.Token) string { // formatGroup coerces an OIDC groups claim into a string // If it is non-string, marshal it into JSON. func formatGroup(rawGroup interface{}) (string, error) { - group, ok := rawGroup.(string) - if !ok { - jsonGroup, err := json.Marshal(rawGroup) - if err != nil { - return "", err - } - group = string(jsonGroup) + if group, ok := rawGroup.(string); ok { + return group, nil } - return group, nil + + jsonGroup, err := json.Marshal(rawGroup) + if err != nil { + return "", err + } + return string(jsonGroup), nil } // coerceArray extracts a field from simplejson.Json that might be a