From 0ef511d1f3e2ffb82b8261c2c9181e3877bf0bfd Mon Sep 17 00:00:00 2001 From: Benjamin Foote Date: Fri, 4 Dec 2020 16:16:10 -0800 Subject: [PATCH 1/7] remove ID, a vestige of the db --- pkg/providers/github/github.go | 1 - pkg/providers/github/github_test.go | 1 - pkg/providers/openstax/openstax.go | 1 - pkg/structs/structs.go | 22 +++++----------------- 4 files changed, 5 insertions(+), 20 deletions(-) diff --git a/pkg/providers/github/github.go b/pkg/providers/github/github.go index 8fa7e932..a935088e 100644 --- a/pkg/providers/github/github.go +++ b/pkg/providers/github/github.go @@ -75,7 +75,6 @@ func (me Provider) GetUserInfo(r *http.Request, user *structs.User, customClaims user.Email = ghUser.Email user.Name = ghUser.Name user.Username = ghUser.Username - user.ID = ghUser.ID // user = &ghUser.User diff --git a/pkg/providers/github/github_test.go b/pkg/providers/github/github_test.go index 1caf5abc..8bace3d7 100644 --- a/pkg/providers/github/github_test.go +++ b/pkg/providers/github/github_test.go @@ -171,7 +171,6 @@ func TestGetUserInfo(t *testing.T) { Username: "test", CreatedOn: 123, Email: "email@example.com", - ID: 1, LastUpdate: 123, Name: "name", }, diff --git a/pkg/providers/openstax/openstax.go b/pkg/providers/openstax/openstax.go index b9ccf24d..c872cee1 100644 --- a/pkg/providers/openstax/openstax.go +++ b/pkg/providers/openstax/openstax.go @@ -63,7 +63,6 @@ func (Provider) GetUserInfo(r *http.Request, user *structs.User, customClaims *s user.Email = oxUser.Email user.Name = oxUser.Name user.Username = oxUser.Username - user.ID = oxUser.ID user.PrepareUserData() return nil } diff --git a/pkg/structs/structs.go b/pkg/structs/structs.go index bccc0180..374c3683 100644 --- a/pkg/structs/structs.go +++ b/pkg/structs/structs.go @@ -10,8 +10,6 @@ OR CONDITIONS OF ANY KIND, either express or implied. package structs -import "strconv" - // CustomClaims Temporary struct storing custom claims until JWT creation. type CustomClaims struct { Claims map[string]interface{} @@ -24,18 +22,11 @@ type UserI interface { // User is inherited. type User struct { - // TODO: set Provider here so that we can pass it to db - // populated by db (via mapstructure) or from provider (via json) - // Provider string `json:"provider",mapstructure:"provider"` - Username string `json:"username" mapstructure:"username"` - Name string `json:"name" mapstructure:"name"` - Email string `json:"email" mapstructure:"email"` - CreatedOn int64 `json:"createdon"` - LastUpdate int64 `json:"lastupdate"` - // don't populate ID from json https://github.com/vouch/vouch-proxy/issues/185 - ID int `json:"-" mapstructure:"id"` - // jwt.StandardClaims - + Username string `json:"username"` + Name string `json:"name"` + Email string `json:"email"` + CreatedOn int64 `json:"createdon"` + LastUpdate int64 `json:"lastupdate"` TeamMemberships []string } @@ -201,8 +192,6 @@ func (u *AlibabaUser) PrepareUserData() { u.Username = u.Data.Username u.Name = u.Data.Nickname u.Email = u.Data.Email - id, _ := strconv.Atoi(u.Data.ID) - u.ID = id } // AliData `data` subobject of Alibaba User response @@ -212,7 +201,6 @@ type AliData struct { Username string `json:"username"` Nickname string `json:"nickname"` Email string `json:"email"` - ID string `json:"ou_id"` Phone string `json:"phone_number"` OuName string `json:"ou_name"` } From 2f8a037d0494cab3629d149cb51ecda47b615725 Mon Sep 17 00:00:00 2001 From: Benjamin Foote Date: Sun, 6 Dec 2020 18:42:54 -0800 Subject: [PATCH 2/7] Delete unused GoogleUser struct --- pkg/structs/structs.go | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/pkg/structs/structs.go b/pkg/structs/structs.go index 374c3683..343ad09f 100644 --- a/pkg/structs/structs.go +++ b/pkg/structs/structs.go @@ -63,28 +63,6 @@ func (u *AzureUser) PrepareUserData() { } } -// GoogleUser is a retrieved and authentiacted user from Google. -// unused! -// TODO: see if these should be pointers to the *User object as per -// https://golang.org/doc/effective_go.html#embedding -type GoogleUser struct { - User - Sub string `json:"sub"` - GivenName string `json:"given_name"` - FamilyName string `json:"family_name"` - Profile string `json:"profile"` - Picture string `json:"picture"` - EmailVerified bool `json:"email_verified"` - Gender string `json:"gender"` - HostDomain string `json:"hd"` - // jwt.StandardClaims -} - -// PrepareUserData implement PersonalData interface -func (u *GoogleUser) PrepareUserData() { - u.Username = u.Email -} - // ADFSUser Active Directory user record type ADFSUser struct { User From 5b332a34187cae53ff3fc968e107e76e0565e9b5 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sun, 21 Nov 2021 19:12:04 -0500 Subject: [PATCH 3/7] Delete unused fields from AliData --- pkg/structs/structs.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/structs/structs.go b/pkg/structs/structs.go index 343ad09f..b11a320a 100644 --- a/pkg/structs/structs.go +++ b/pkg/structs/structs.go @@ -179,8 +179,6 @@ type AliData struct { Username string `json:"username"` Nickname string `json:"nickname"` Email string `json:"email"` - Phone string `json:"phone_number"` - OuName string `json:"ou_name"` } // Team has members and provides acess to sites From bab98645056ad6b8c70f55c7c7584e22e6e06286 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sun, 21 Nov 2021 19:56:44 -0500 Subject: [PATCH 4/7] GitHub: Simplify User object copy --- pkg/providers/github/github.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/pkg/providers/github/github.go b/pkg/providers/github/github.go index a935088e..bb4c3573 100644 --- a/pkg/providers/github/github.go +++ b/pkg/providers/github/github.go @@ -72,11 +72,7 @@ func (me Provider) GetUserInfo(r *http.Request, user *structs.User, customClaims log.Debug(user) ghUser.PrepareUserData() - user.Email = ghUser.Email - user.Name = ghUser.Name - user.Username = ghUser.Username - - // user = &ghUser.User + *user = ghUser.User toOrgAndTeam := func(orgAndTeam string) (string, string) { split := strings.Split(orgAndTeam, "/") From 2efe90c39103be94495574bf0e6dd27120f8385a Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sun, 21 Nov 2021 19:57:31 -0500 Subject: [PATCH 5/7] GitHub: Fix user info sample used for testing GitHub doesn't provide username, createdon, lastupdate, or sub in the user info JSON it returns. --- pkg/providers/github/github_test.go | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/pkg/providers/github/github_test.go b/pkg/providers/github/github_test.go index 8bace3d7..e5649b58 100644 --- a/pkg/providers/github/github_test.go +++ b/pkg/providers/github/github_test.go @@ -11,7 +11,6 @@ OR CONDITIONS OF ANY KIND, either express or implied. package github import ( - "encoding/json" "net/http" "regexp" "testing" @@ -166,17 +165,16 @@ func TestGetOrgMembershipStateFromGitHubNoOrgAccess(t *testing.T) { func TestGetUserInfo(t *testing.T) { setUp() - userInfoContent, _ := json.Marshal(structs.GitHubUser{ - User: structs.User{ - Username: "test", - CreatedOn: 123, - Email: "email@example.com", - LastUpdate: 123, - Name: "name", - }, - Login: "myusername", - Picture: "avatar-url", - }) + // Use JSON directly (instead of populating a struct and converting to JSON) to reduce the chances + // of a mismatch between what GitHub provides and what is expected. + userInfoContent := []byte(` + { + "avatar_url": "avatar-url", + "email": "email@example.com", + "login": "myusername", + "name": "name" + } + `) mockResponse(urlEquals(cfg.GenOAuth.UserInfoURL+token.AccessToken), http.StatusOK, map[string]string{}, userInfoContent) cfg.Cfg.TeamWhiteList = append(cfg.Cfg.TeamWhiteList, "myOtherOrg", "myorg/myteam") From 8298dda72029f62d7c35a5211aed49ab67f4acf5 Mon Sep 17 00:00:00 2001 From: Benjamin Foote Date: Sun, 6 Dec 2020 18:41:55 -0800 Subject: [PATCH 6/7] GitHub: Improve debug logging --- pkg/providers/github/github.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/pkg/providers/github/github.go b/pkg/providers/github/github.go index bb4c3573..aca3cadf 100644 --- a/pkg/providers/github/github.go +++ b/pkg/providers/github/github.go @@ -66,10 +66,7 @@ func (me Provider) GetUserInfo(r *http.Request, user *structs.User, customClaims log.Error(err) return err } - log.Debug("getUserInfoFromGitHub ghUser") - log.Debug(ghUser) - log.Debug("getUserInfoFromGitHub user") - log.Debug(user) + log.Debugf("getUserInfoFromGitHub ghUser %+v", ghUser) ghUser.PrepareUserData() *user = ghUser.User @@ -111,8 +108,7 @@ func (me Provider) GetUserInfo(r *http.Request, user *structs.User, customClaims } } - log.Debug("getUserInfoFromGitHub") - log.Debug(user) + log.Debugf("getUserInfoFromGitHub user: %+v", user) return nil } From 270fa9fe5f1ceee7cc1e04d8a060f3c19e12c6d0 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 12 Aug 2020 01:02:02 -0400 Subject: [PATCH 7/7] Check for `sub` claim, not `username`, when validating Not all IdPs provide a `username` or `email` claim in the UserInfo response, and many IdPs allow users to change their username or email address. Co-authored-by: Benjamin Foote --- .defaults.yml | 1 + config/config.yml_example | 2 ++ handlers/handlers_test.go | 51 +++++++++++++++++++++++++---- handlers/validate.go | 13 +++++--- handlers/validate_test.go | 28 +++++++++++++--- pkg/cfg/cfg.go | 1 + pkg/cfg/cfg_test.go | 5 ++- pkg/jwtmanager/jwtmanager.go | 2 ++ pkg/jwtmanager/jwtmanager_test.go | 2 ++ pkg/providers/github/github_test.go | 2 ++ pkg/structs/structs.go | 18 ++++++++-- 11 files changed, 104 insertions(+), 21 deletions(-) diff --git a/.defaults.yml b/.defaults.yml index d6cea00c..32e7abd0 100644 --- a/.defaults.yml +++ b/.defaults.yml @@ -42,6 +42,7 @@ vouch: # key: headers: + sub: X-Vouch-Sub jwt: X-Vouch-Token user: X-Vouch-User success: X-Vouch-Success diff --git a/config/config.yml_example b/config/config.yml_example index 3b37a55d..317ef83d 100644 --- a/config/config.yml_example +++ b/config/config.yml_example @@ -47,6 +47,8 @@ vouch: # whiteList (optional) allows only the listed usernames - VOUCH_WHITELIST # usernames are usually email addresses (google, most oidc providers) or login/username for github and github enterprise + # if a user can change their info including email address this might be a bad idea + # see https://github.com/vouch/vouch-proxy/issues/309 and https://openid.net/specs/openid-connect-core-1_0.html#ClaimStability whiteList: - bob@yourdomain.com - alice@yourdomain.com diff --git a/handlers/handlers_test.go b/handlers/handlers_test.go index 8c399c61..5a92f6fc 100644 --- a/handlers/handlers_test.go +++ b/handlers/handlers_test.go @@ -45,7 +45,12 @@ func setUp(configFile string) { func TestVerifyUserPositiveUserInWhiteList(t *testing.T) { setUp("/config/testing/handler_whitelist.yml") - user := &structs.User{Username: "test@example.com", Email: "test@example.com", Name: "Test Name"} + user := &structs.User{ + Sub: "testsub", + Username: "test@example.com", + Email: "test@example.com", + Name: "Test Name", + } ok, err := verifyUser(*user) assert.True(t, ok) assert.Nil(t, err) @@ -54,7 +59,12 @@ func TestVerifyUserPositiveUserInWhiteList(t *testing.T) { func TestVerifyUserPositiveAllowAllUsers(t *testing.T) { setUp("/config/testing/handler_allowallusers.yml") - user := &structs.User{Username: "testuser", Email: "test@example.com", Name: "Test Name"} + user := &structs.User{ + Sub: "testsub", + Username: "testuser", + Email: "test@example.com", + Name: "Test Name", + } ok, err := verifyUser(*user) assert.True(t, ok) @@ -63,7 +73,12 @@ func TestVerifyUserPositiveAllowAllUsers(t *testing.T) { func TestVerifyUserPositiveByEmail(t *testing.T) { setUp("/config/testing/handler_email.yml") - user := &structs.User{Username: "testuser", Email: "test@example.com", Name: "Test Name"} + user := &structs.User{ + Sub: "testsub", + Username: "testuser", + Email: "test@example.com", + Name: "Test Name", + } ok, err := verifyUser(*user) assert.True(t, ok) assert.Nil(t, err) @@ -73,7 +88,12 @@ func TestVerifyUserPositiveByTeam(t *testing.T) { setUp("/config/testing/handler_teams.yml") // cfg.Cfg.TeamWhiteList = append(cfg.Cfg.TeamWhiteList, "org1/team2", "org1/team1") - user := &structs.User{Username: "testuser", Email: "test@example.com", Name: "Test Name"} + user := &structs.User{ + Sub: "testsub", + Username: "testuser", + Email: "test@example.com", + Name: "Test Name", + } user.TeamMemberships = append(user.TeamMemberships, "org1/team3") user.TeamMemberships = append(user.TeamMemberships, "org1/team1") ok, err := verifyUser(*user) @@ -83,7 +103,12 @@ func TestVerifyUserPositiveByTeam(t *testing.T) { func TestVerifyUserNegativeByTeam(t *testing.T) { setUp("/config/testing/handler_teams.yml") - user := &structs.User{Username: "testuser", Email: "test@example.com", Name: "Test Name"} + user := &structs.User{ + Sub: "testsub", + Username: "testuser", + Email: "test@example.com", + Name: "Test Name", + } // cfg.Cfg.TeamWhiteList = append(cfg.Cfg.TeamWhiteList, "org1/team1") ok, err := verifyUser(*user) @@ -94,7 +119,12 @@ func TestVerifyUserNegativeByTeam(t *testing.T) { func TestVerifyUserPositiveNoDomainsConfigured(t *testing.T) { setUp("/config/testing/handler_nodomains.yml") - user := &structs.User{Username: "testuser", Email: "test@example.com", Name: "Test Name"} + user := &structs.User{ + Sub: "testsub", + Username: "testuser", + Email: "test@example.com", + Name: "Test Name", + } cfg.Cfg.Domains = make([]string, 0) ok, err := verifyUser(*user) @@ -104,7 +134,12 @@ func TestVerifyUserPositiveNoDomainsConfigured(t *testing.T) { func TestVerifyUserNegative(t *testing.T) { setUp("/config/testing/test_config.yml") - user := &structs.User{Username: "testuser", Email: "test@example.com", Name: "Test Name"} + user := &structs.User{ + Sub: "testsub", + Username: "testuser", + Email: "test@example.com", + Name: "Test Name", + } ok, err := verifyUser(*user) assert.False(t, ok) @@ -115,6 +150,7 @@ func TestVerifyUserNegative(t *testing.T) { // it should live there but circular imports are resolved if it lives here var ( u1 = structs.User{ + Sub: "testsub", Username: "test@testing.com", Name: "Test Name", } @@ -140,6 +176,7 @@ func init() { // log.SetLevel(log.DebugLevel) lc = jwtmanager.VouchClaims{ + Sub: u1.Sub, Username: u1.Username, CustomClaims: customClaims.Claims, PAccessToken: t1.PAccessToken, diff --git a/handlers/validate.go b/handlers/validate.go index bc01bf32..83bcebcf 100644 --- a/handlers/validate.go +++ b/handlers/validate.go @@ -25,8 +25,8 @@ import ( ) var ( - errNoJWT = errors.New("no jwt found in request") - errNoUser = errors.New("no User found in jwt") + errNoJWT = errors.New("no jwt found in request") + errNoSub = errors.New("no 'sub' found in jwt") ) // ValidateRequestHandler /validate @@ -45,8 +45,8 @@ func ValidateRequestHandler(w http.ResponseWriter, r *http.Request) { return } - if claims.Username == "" { - send401or200PublicAccess(w, r, errNoUser) + if claims.Sub == "" { + send401or200PublicAccess(w, r, errNoSub) return } @@ -59,7 +59,10 @@ func ValidateRequestHandler(w http.ResponseWriter, r *http.Request) { } generateCustomClaimsHeaders(w, claims) - w.Header().Add(cfg.Cfg.Headers.User, claims.Username) + w.Header().Add(cfg.Cfg.Headers.Sub, claims.Sub) + if claims.Username != "" { + w.Header().Add(cfg.Cfg.Headers.User, claims.Username) + } w.Header().Add(cfg.Cfg.Headers.Success, "true") if cfg.Cfg.Headers.AccessToken != "" && claims.PAccessToken != "" { diff --git a/handlers/validate_test.go b/handlers/validate_test.go index 1889d6f9..fd2bd590 100644 --- a/handlers/validate_test.go +++ b/handlers/validate_test.go @@ -28,7 +28,12 @@ import ( func BenchmarkValidateRequestHandler(b *testing.B) { setUp("/config/testing/handler_email.yml") - user := &structs.User{Username: "testuser", Email: "test@example.com", Name: "Test Name"} + user := &structs.User{ + Sub: "testsub", + Username: "testuser", + Email: "test@example.com", + Name: "Test Name", + } tokens := structs.PTokens{} customClaims := structs.CustomClaims{} @@ -67,7 +72,12 @@ func TestValidateRequestHandlerPerf(t *testing.T) { } setUp("/config/testing/handler_email.yml") - user := &structs.User{Username: "testuser", Email: "test@example.com", Name: "Test Name"} + user := &structs.User{ + Sub: "testsub", + Username: "testuser", + Email: "test@example.com", + Name: "Test Name", + } tokens := structs.PTokens{} customClaims := structs.CustomClaims{} @@ -155,7 +165,12 @@ func TestValidateRequestHandlerWithGroupClaims(t *testing.T) { tokens := structs.PTokens{} - user := &structs.User{Username: "testuser", Email: "test@example.com", Name: "Test Name"} + user := &structs.User{ + Sub: "testsub", + Username: "testuser", + Email: "test@example.com", + Name: "Test Name", + } vpjwt, err := jwtmanager.NewVPJWT(*user, customClaims, tokens) assert.NoError(t, err) @@ -208,7 +223,12 @@ func TestJWTCacheHandler(t *testing.T) { setUp("/config/testing/handler_logout_url.yml") handler := jwtmanager.JWTCacheHandler(http.HandlerFunc(ValidateRequestHandler)) - user := &structs.User{Username: "testuser", Email: "test@example.com", Name: "Test Name"} + user := &structs.User{ + Sub: "testsub", + Username: "testuser", + Email: "test@example.com", + Name: "Test Name", + } tokens := structs.PTokens{} customClaims := structs.CustomClaims{} diff --git a/pkg/cfg/cfg.go b/pkg/cfg/cfg.go index 063e0b47..bd11a743 100644 --- a/pkg/cfg/cfg.go +++ b/pkg/cfg/cfg.go @@ -74,6 +74,7 @@ type Config struct { } Headers struct { + Sub string `mapstructure:"sub"` JWT string `mapstructure:"jwt"` User string `mapstructure:"user"` QueryString string `mapstructure:"querystring"` diff --git a/pkg/cfg/cfg_test.go b/pkg/cfg/cfg_test.go index caf13c25..7f19f42d 100644 --- a/pkg/cfg/cfg_test.go +++ b/pkg/cfg/cfg_test.go @@ -130,8 +130,7 @@ func Test_configureFromEnvCfg(t *testing.T) { t.Cleanup(cleanupEnv) // each of these env vars holds a.. // string - // get all the values - senv := []string{"VOUCH_LISTEN", "VOUCH_JWT_ISSUER", "VOUCH_JWT_SECRET", "VOUCH_HEADERS_JWT", + senv := []string{"VOUCH_LISTEN", "VOUCH_JWT_ISSUER", "VOUCH_JWT_SECRET", "VOUCH_HEADERS_JWT", "VOUCH_HEADERS_SUB", "VOUCH_HEADERS_USER", "VOUCH_HEADERS_QUERYSTRING", "VOUCH_HEADERS_REDIRECT", "VOUCH_HEADERS_SUCCESS", "VOUCH_HEADERS_ERROR", "VOUCH_HEADERS_CLAIMHEADER", "VOUCH_HEADERS_ACCESSTOKEN", "VOUCH_HEADERS_IDTOKEN", "VOUCH_COOKIE_NAME", "VOUCH_COOKIE_DOMAIN", "VOUCH_COOKIE_SAMESITE", "VOUCH_TESTURL", "VOUCH_SESSION_NAME", "VOUCH_SESSION_KEY", "VOUCH_DOCUMENT_ROOT"} @@ -168,7 +167,7 @@ func Test_configureFromEnvCfg(t *testing.T) { // run the thing configureFromEnv() - scfg := []string{Cfg.Listen, Cfg.JWT.Issuer, Cfg.JWT.Secret, Cfg.Headers.JWT, + scfg := []string{Cfg.Listen, Cfg.JWT.Issuer, Cfg.JWT.Secret, Cfg.Headers.JWT, Cfg.Headers.Sub, Cfg.Headers.User, Cfg.Headers.QueryString, Cfg.Headers.Redirect, Cfg.Headers.Success, Cfg.Headers.Error, Cfg.Headers.ClaimHeader, Cfg.Headers.AccessToken, Cfg.Headers.IDToken, Cfg.Cookie.Name, Cfg.Cookie.Domain, Cfg.Cookie.SameSite, Cfg.TestURL, Cfg.Session.Name, Cfg.Session.Key, Cfg.DocumentRoot, diff --git a/pkg/jwtmanager/jwtmanager.go b/pkg/jwtmanager/jwtmanager.go index cdbd5c39..bfe21de8 100644 --- a/pkg/jwtmanager/jwtmanager.go +++ b/pkg/jwtmanager/jwtmanager.go @@ -33,6 +33,7 @@ const comma = "," // VouchClaims jwt Claims specific to vouch type VouchClaims struct { + Sub string `json:"sub"` Username string `json:"username"` CustomClaims map[string]interface{} PAccessToken string @@ -79,6 +80,7 @@ func NewVPJWT(u structs.User, customClaims structs.CustomClaims, ptokens structs // User`token` // u.PrepareUserData() claims := VouchClaims{ + u.Sub, u.Username, customClaims.Claims, ptokens.PAccessToken, diff --git a/pkg/jwtmanager/jwtmanager_test.go b/pkg/jwtmanager/jwtmanager_test.go index 8be3e6a1..14a2a04f 100644 --- a/pkg/jwtmanager/jwtmanager_test.go +++ b/pkg/jwtmanager/jwtmanager_test.go @@ -24,6 +24,7 @@ import ( var ( u1 = structs.User{ + Sub: "testsub", Username: "test@testing.com", Name: "Test Name", } @@ -49,6 +50,7 @@ func init() { Configure() lc = VouchClaims{ + u1.Sub, u1.Username, customClaims.Claims, t1.PAccessToken, diff --git a/pkg/providers/github/github_test.go b/pkg/providers/github/github_test.go index e5649b58..efe0d632 100644 --- a/pkg/providers/github/github_test.go +++ b/pkg/providers/github/github_test.go @@ -171,6 +171,7 @@ func TestGetUserInfo(t *testing.T) { { "avatar_url": "avatar-url", "email": "email@example.com", + "id": 123456789, "login": "myusername", "name": "name" } @@ -188,6 +189,7 @@ func TestGetUserInfo(t *testing.T) { err := provider.GetUserInfo(nil, user, &structs.CustomClaims{}, &structs.PTokens{}) assert.Nil(t, err) + assert.Equal(t, "123456789", user.Sub) assert.Equal(t, "myusername", user.Username) assert.Equal(t, []string{"myOtherOrg", "myorg/myteam"}, user.TeamMemberships) diff --git a/pkg/structs/structs.go b/pkg/structs/structs.go index b11a320a..12cb8cef 100644 --- a/pkg/structs/structs.go +++ b/pkg/structs/structs.go @@ -10,6 +10,8 @@ OR CONDITIONS OF ANY KIND, either express or implied. package structs +import "strconv" + // CustomClaims Temporary struct storing custom claims until JWT creation. type CustomClaims struct { Claims map[string]interface{} @@ -22,6 +24,7 @@ type UserI interface { // User is inherited. type User struct { + Sub string `json:"sub"` Username string `json:"username"` Name string `json:"name"` Email string `json:"email"` @@ -35,12 +38,20 @@ func (u *User) PrepareUserData() { if u.Username == "" { u.Username = u.Email } + if u.Sub == "" { + // TODO: SECURITY VULNERABILITY: Using Username for Sub is dangerous if the provider allows the + // user to change their username. It is particularly dangerous if the provider does not set + // Username because it would likely be trivial for an attacker to impersonate another user by + // temporarily changing their email address to the victim's email address. It would be better to + // automatically fail authentication if Sub is empty and force all provider integrations to + // provide a stable identifier. + u.Sub = u.Username + } } // AzureUser is a retrieved and authenticated user from Azure AD type AzureUser struct { User - Sub string `json:"sub"` UPN string `json:"upn"` PreferredUsername string `json:"preferred_username"` } @@ -66,7 +77,6 @@ func (u *AzureUser) PrepareUserData() { // ADFSUser Active Directory user record type ADFSUser struct { User - Sub string `json:"sub"` UPN string `json:"upn"` // UniqueName string `json:"unique_name"` // PwdExp string `json:"pwd_exp"` @@ -83,6 +93,7 @@ func (u *ADFSUser) PrepareUserData() { // GitHubUser is a retrieved and authentiacted user from GitHub. type GitHubUser struct { User + Id int `json:"id"` Login string `json:"login"` Picture string `json:"avatar_url"` // jwt.StandardClaims @@ -95,6 +106,8 @@ type GitHubTeamMembershipState struct { // PrepareUserData implement PersonalData interface func (u *GitHubUser) PrepareUserData() { + // Sub is populated from Id, not Login, because GitHub allows users to change their login. + u.Sub = strconv.Itoa(u.Id) // always use the u.Login as the u.Username u.Username = u.Login } @@ -167,6 +180,7 @@ type AlibabaUser struct { // PrepareUserData implement PersonalData interface func (u *AlibabaUser) PrepareUserData() { + u.Sub = u.Data.Sub u.Username = u.Data.Username u.Name = u.Data.Nickname u.Email = u.Data.Email