From 270fa9fe5f1ceee7cc1e04d8a060f3c19e12c6d0 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 12 Aug 2020 01:02:02 -0400 Subject: [PATCH] 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