Check for sub claim, not username, when validating

rhansen · bnfinet · rhansen · commit 270fa9fe5f1c · 2021-11-21T20:16:18.000-05:00
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 &lt;git@bnf.net&gt;
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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

Original file line number	Diff line number	Diff line change
`@@ -25,8 +25,8 @@ import (`
`25`	`25`	`)`
`26`	`26`
`27`	`27`	`var (`
`28`		`- errNoJWT = errors.New("no jwt found in request")`
`29`		`- errNoUser = errors.New("no User found in jwt")`
	`28`	`+ errNoJWT = errors.New("no jwt found in request")`
	`29`	`+ errNoSub = errors.New("no 'sub' found in jwt")`
`30`	`30`	`)`
`31`	`31`
`32`	`32`	`// ValidateRequestHandler /validate`
`@@ -45,8 +45,8 @@ func ValidateRequestHandler(w http.ResponseWriter, r *http.Request) {`
`45`	`45`	`return`
`46`	`46`	`}`
`47`	`47`
`48`		`- if claims.Username == "" {`
`49`		`- send401or200PublicAccess(w, r, errNoUser)`
	`48`	`+ if claims.Sub == "" {`
	`49`	`+ send401or200PublicAccess(w, r, errNoSub)`
`50`	`50`	`return`
`51`	`51`	`}`
`52`	`52`
`@@ -59,7 +59,10 @@ func ValidateRequestHandler(w http.ResponseWriter, r *http.Request) {`
`59`	`59`	`}`
`60`	`60`
`61`	`61`	`generateCustomClaimsHeaders(w, claims)`
`62`		`- w.Header().Add(cfg.Cfg.Headers.User, claims.Username)`
	`62`	`+ w.Header().Add(cfg.Cfg.Headers.Sub, claims.Sub)`
	`63`	`+ if claims.Username != "" {`
	`64`	`+ w.Header().Add(cfg.Cfg.Headers.User, claims.Username)`
	`65`	`+ }`
`63`	`66`	`w.Header().Add(cfg.Cfg.Headers.Success, "true")`
`64`	`67`
`65`	`68`	`if cfg.Cfg.Headers.AccessToken != "" && claims.PAccessToken != "" {`
Original file line number	Diff line number	Diff line change
`@@ -74,6 +74,7 @@ type Config struct {`
`74`	`74`	`}`
`75`	`75`
`76`	`76`	`Headers struct {`
	`77`	+ Sub string `mapstructure:"sub"`
`77`	`78`	JWT string `mapstructure:"jwt"`
`78`	`79`	User string `mapstructure:"user"`
`79`	`80`	QueryString string `mapstructure:"querystring"`
Original file line number	Diff line number	Diff line change
`@@ -171,6 +171,7 @@ func TestGetUserInfo(t *testing.T) {`
`171`	`171`	`{`
`172`	`172`	`"avatar_url": "avatar-url",`
`173`	`173`	`"email": "[email protected]",`
	`174`	`+ "id": 123456789,`
`174`	`175`	`"login": "myusername",`
`175`	`176`	`"name": "name"`
`176`	`177`	`}`
`@@ -188,6 +189,7 @@ func TestGetUserInfo(t *testing.T) {`
`188`	`189`	`err := provider.GetUserInfo(nil, user, &structs.CustomClaims{}, &structs.PTokens{})`
`189`	`190`
`190`	`191`	`assert.Nil(t, err)`
	`192`	`+ assert.Equal(t, "123456789", user.Sub)`
`191`	`193`	`assert.Equal(t, "myusername", user.Username)`
`192`	`194`	`assert.Equal(t, []string{"myOtherOrg", "myorg/myteam"}, user.TeamMemberships)`
`193`	`195`