Check for sub claim, not username, when validating

.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

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:
   - [email protected]
   - [email protected]

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: "[email protected]", Email: "[email protected]", Name: "Test Name"}
+	user := &structs.User{
+		Sub:      "testsub",
+		Username: "[email protected]",
+		Email:    "[email protected]",
+		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: "[email protected]", Name: "Test Name"}
+	user := &structs.User{
+		Sub:      "testsub",
+		Username: "testuser",
+		Email:    "[email protected]",
+		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: "[email protected]", Name: "Test Name"}
+	user := &structs.User{
+		Sub:      "testsub",
+		Username: "testuser",
+		Email:    "[email protected]",
+		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: "[email protected]", Name: "Test Name"}
+	user := &structs.User{
+		Sub:      "testsub",
+		Username: "testuser",
+		Email:    "[email protected]",
+		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: "[email protected]", Name: "Test Name"}
+	user := &structs.User{
+		Sub:      "testsub",
+		Username: "testuser",
+		Email:    "[email protected]",
+		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: "[email protected]", Name: "Test Name"}
+	user := &structs.User{
+		Sub:      "testsub",
+		Username: "testuser",
+		Email:    "[email protected]",
+		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: "[email protected]", Name: "Test Name"}
+	user := &structs.User{
+		Sub:      "testsub",
+		Username: "testuser",
+		Email:    "[email protected]",
+		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: "[email protected]",
 		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,

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 != "" {

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: "[email protected]", Name: "Test Name"}
+	user := &structs.User{
+		Sub:      "testsub",
+		Username: "testuser",
+		Email:    "[email protected]",
+		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: "[email protected]", Name: "Test Name"}
+	user := &structs.User{
+		Sub:      "testsub",
+		Username: "testuser",
+		Email:    "[email protected]",
+		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: "[email protected]", Name: "Test Name"}
+	user := &structs.User{
+		Sub:      "testsub",
+		Username: "testuser",
+		Email:    "[email protected]",
+		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: "[email protected]", Name: "Test Name"}
+	user := &structs.User{
+		Sub:      "testsub",
+		Username: "testuser",
+		Email:    "[email protected]",
+		Name:     "Test Name",
+	}
 	tokens := structs.PTokens{}
 	customClaims := structs.CustomClaims{}
 

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"`

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,

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,

pkg/jwtmanager/jwtmanager_test.go

@@ -24,6 +24,7 @@ import (
 
 var (
 	u1 = structs.User{
+		Sub:      "testsub",
 		Username: "[email protected]",
 		Name:     "Test Name",
 	}
@@ -49,6 +50,7 @@ func init() {
 	Configure()
 
 	lc = VouchClaims{
+		u1.Sub,
 		u1.Username,
 		customClaims.Claims,
 		t1.PAccessToken,

pkg/providers/github/github.go

@@ -66,18 +66,10 @@ 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.Email = ghUser.Email
-	user.Name = ghUser.Name
-	user.Username = ghUser.Username
-	user.ID = ghUser.ID
-
-	// user = &ghUser.User
+	*user = ghUser.User
 
 	toOrgAndTeam := func(orgAndTeam string) (string, string) {
 		split := strings.Split(orgAndTeam, "/")
@@ -116,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
 }
 

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,18 +165,17 @@ 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 protected]",
-			ID:         1,
-			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 protected]",
+			"id": 123456789,
+			"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")
@@ -191,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)
 

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
 }