Skip to content

Commit 270fa9f

Browse files
rhansenbnfinet
rhansen
and
bnfinet
committed
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 <git@bnf.net>
1 parent 8298dda commit 270fa9f

11 files changed

Lines changed: 104 additions & 21 deletions

File tree

‎.defaults.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ vouch:
4242
# key:
4343

4444
headers:
45+
sub: X-Vouch-Sub
4546
jwt: X-Vouch-Token
4647
user: X-Vouch-User
4748
success: X-Vouch-Success

‎config/config.yml_example‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,8 @@ vouch:
4747

4848
# whiteList (optional) allows only the listed usernames - VOUCH_WHITELIST
4949
# usernames are usually email addresses (google, most oidc providers) or login/username for github and github enterprise
50+
# if a user can change their info including email address this might be a bad idea
51+
# see https://github.com/vouch/vouch-proxy/issues/309 and https://openid.net/specs/openid-connect-core-1_0.html#ClaimStability
5052
whiteList:
5153
- bob@yourdomain.com
5254
- alice@yourdomain.com

‎handlers/handlers_test.go‎

Lines changed: 44 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,12 @@ func setUp(configFile string) {
4545

4646
func TestVerifyUserPositiveUserInWhiteList(t *testing.T) {
4747
setUp("/config/testing/handler_whitelist.yml")
48-
user := &structs.User{Username: "test@example.com", Email: "test@example.com", Name: "Test Name"}
48+
user := &structs.User{
49+
Sub: "testsub",
50+
Username: "test@example.com",
51+
Email: "test@example.com",
52+
Name: "Test Name",
53+
}
4954
ok, err := verifyUser(*user)
5055
assert.True(t, ok)
5156
assert.Nil(t, err)
@@ -54,7 +59,12 @@ func TestVerifyUserPositiveUserInWhiteList(t *testing.T) {
5459
func TestVerifyUserPositiveAllowAllUsers(t *testing.T) {
5560
setUp("/config/testing/handler_allowallusers.yml")
5661

57-
user := &structs.User{Username: "testuser", Email: "test@example.com", Name: "Test Name"}
62+
user := &structs.User{
63+
Sub: "testsub",
64+
Username: "testuser",
65+
Email: "test@example.com",
66+
Name: "Test Name",
67+
}
5868

5969
ok, err := verifyUser(*user)
6070
assert.True(t, ok)
@@ -63,7 +73,12 @@ func TestVerifyUserPositiveAllowAllUsers(t *testing.T) {
6373

6474
func TestVerifyUserPositiveByEmail(t *testing.T) {
6575
setUp("/config/testing/handler_email.yml")
66-
user := &structs.User{Username: "testuser", Email: "test@example.com", Name: "Test Name"}
76+
user := &structs.User{
77+
Sub: "testsub",
78+
Username: "testuser",
79+
Email: "test@example.com",
80+
Name: "Test Name",
81+
}
6782
ok, err := verifyUser(*user)
6883
assert.True(t, ok)
6984
assert.Nil(t, err)
@@ -73,7 +88,12 @@ func TestVerifyUserPositiveByTeam(t *testing.T) {
7388
setUp("/config/testing/handler_teams.yml")
7489

7590
// cfg.Cfg.TeamWhiteList = append(cfg.Cfg.TeamWhiteList, "org1/team2", "org1/team1")
76-
user := &structs.User{Username: "testuser", Email: "test@example.com", Name: "Test Name"}
91+
user := &structs.User{
92+
Sub: "testsub",
93+
Username: "testuser",
94+
Email: "test@example.com",
95+
Name: "Test Name",
96+
}
7797
user.TeamMemberships = append(user.TeamMemberships, "org1/team3")
7898
user.TeamMemberships = append(user.TeamMemberships, "org1/team1")
7999
ok, err := verifyUser(*user)
@@ -83,7 +103,12 @@ func TestVerifyUserPositiveByTeam(t *testing.T) {
83103

84104
func TestVerifyUserNegativeByTeam(t *testing.T) {
85105
setUp("/config/testing/handler_teams.yml")
86-
user := &structs.User{Username: "testuser", Email: "test@example.com", Name: "Test Name"}
106+
user := &structs.User{
107+
Sub: "testsub",
108+
Username: "testuser",
109+
Email: "test@example.com",
110+
Name: "Test Name",
111+
}
87112
// cfg.Cfg.TeamWhiteList = append(cfg.Cfg.TeamWhiteList, "org1/team1")
88113

89114
ok, err := verifyUser(*user)
@@ -94,7 +119,12 @@ func TestVerifyUserNegativeByTeam(t *testing.T) {
94119
func TestVerifyUserPositiveNoDomainsConfigured(t *testing.T) {
95120
setUp("/config/testing/handler_nodomains.yml")
96121

97-
user := &structs.User{Username: "testuser", Email: "test@example.com", Name: "Test Name"}
122+
user := &structs.User{
123+
Sub: "testsub",
124+
Username: "testuser",
125+
Email: "test@example.com",
126+
Name: "Test Name",
127+
}
98128
cfg.Cfg.Domains = make([]string, 0)
99129
ok, err := verifyUser(*user)
100130

@@ -104,7 +134,12 @@ func TestVerifyUserPositiveNoDomainsConfigured(t *testing.T) {
104134

105135
func TestVerifyUserNegative(t *testing.T) {
106136
setUp("/config/testing/test_config.yml")
107-
user := &structs.User{Username: "testuser", Email: "test@example.com", Name: "Test Name"}
137+
user := &structs.User{
138+
Sub: "testsub",
139+
Username: "testuser",
140+
Email: "test@example.com",
141+
Name: "Test Name",
142+
}
108143
ok, err := verifyUser(*user)
109144

110145
assert.False(t, ok)
@@ -115,6 +150,7 @@ func TestVerifyUserNegative(t *testing.T) {
115150
// it should live there but circular imports are resolved if it lives here
116151
var (
117152
u1 = structs.User{
153+
Sub: "testsub",
118154
Username: "test@testing.com",
119155
Name: "Test Name",
120156
}
@@ -140,6 +176,7 @@ func init() {
140176
// log.SetLevel(log.DebugLevel)
141177

142178
lc = jwtmanager.VouchClaims{
179+
Sub: u1.Sub,
143180
Username: u1.Username,
144181
CustomClaims: customClaims.Claims,
145182
PAccessToken: t1.PAccessToken,

‎handlers/validate.go‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,8 @@ import (
2525
)
2626

2727
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")
3030
)
3131

3232
// ValidateRequestHandler /validate
@@ -45,8 +45,8 @@ func ValidateRequestHandler(w http.ResponseWriter, r *http.Request) {
4545
return
4646
}
4747

48-
if claims.Username == "" {
49-
send401or200PublicAccess(w, r, errNoUser)
48+
if claims.Sub == "" {
49+
send401or200PublicAccess(w, r, errNoSub)
5050
return
5151
}
5252

@@ -59,7 +59,10 @@ func ValidateRequestHandler(w http.ResponseWriter, r *http.Request) {
5959
}
6060

6161
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+
}
6366
w.Header().Add(cfg.Cfg.Headers.Success, "true")
6467

6568
if cfg.Cfg.Headers.AccessToken != "" && claims.PAccessToken != "" {

‎handlers/validate_test.go‎

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,12 @@ import (
2828

2929
func BenchmarkValidateRequestHandler(b *testing.B) {
3030
setUp("/config/testing/handler_email.yml")
31-
user := &structs.User{Username: "testuser", Email: "test@example.com", Name: "Test Name"}
31+
user := &structs.User{
32+
Sub: "testsub",
33+
Username: "testuser",
34+
Email: "test@example.com",
35+
Name: "Test Name",
36+
}
3237
tokens := structs.PTokens{}
3338
customClaims := structs.CustomClaims{}
3439

@@ -67,7 +72,12 @@ func TestValidateRequestHandlerPerf(t *testing.T) {
6772
}
6873

6974
setUp("/config/testing/handler_email.yml")
70-
user := &structs.User{Username: "testuser", Email: "test@example.com", Name: "Test Name"}
75+
user := &structs.User{
76+
Sub: "testsub",
77+
Username: "testuser",
78+
Email: "test@example.com",
79+
Name: "Test Name",
80+
}
7181
tokens := structs.PTokens{}
7282
customClaims := structs.CustomClaims{}
7383

@@ -155,7 +165,12 @@ func TestValidateRequestHandlerWithGroupClaims(t *testing.T) {
155165

156166
tokens := structs.PTokens{}
157167

158-
user := &structs.User{Username: "testuser", Email: "test@example.com", Name: "Test Name"}
168+
user := &structs.User{
169+
Sub: "testsub",
170+
Username: "testuser",
171+
Email: "test@example.com",
172+
Name: "Test Name",
173+
}
159174
vpjwt, err := jwtmanager.NewVPJWT(*user, customClaims, tokens)
160175
assert.NoError(t, err)
161176

@@ -208,7 +223,12 @@ func TestJWTCacheHandler(t *testing.T) {
208223
setUp("/config/testing/handler_logout_url.yml")
209224
handler := jwtmanager.JWTCacheHandler(http.HandlerFunc(ValidateRequestHandler))
210225

211-
user := &structs.User{Username: "testuser", Email: "test@example.com", Name: "Test Name"}
226+
user := &structs.User{
227+
Sub: "testsub",
228+
Username: "testuser",
229+
Email: "test@example.com",
230+
Name: "Test Name",
231+
}
212232
tokens := structs.PTokens{}
213233
customClaims := structs.CustomClaims{}
214234

‎pkg/cfg/cfg.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ type Config struct {
7474
}
7575

7676
Headers struct {
77+
Sub string `mapstructure:"sub"`
7778
JWT string `mapstructure:"jwt"`
7879
User string `mapstructure:"user"`
7980
QueryString string `mapstructure:"querystring"`

‎pkg/cfg/cfg_test.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -130,8 +130,7 @@ func Test_configureFromEnvCfg(t *testing.T) {
130130
t.Cleanup(cleanupEnv)
131131
// each of these env vars holds a..
132132
// string
133-
// get all the values
134-
senv := []string{"VOUCH_LISTEN", "VOUCH_JWT_ISSUER", "VOUCH_JWT_SECRET", "VOUCH_HEADERS_JWT",
133+
senv := []string{"VOUCH_LISTEN", "VOUCH_JWT_ISSUER", "VOUCH_JWT_SECRET", "VOUCH_HEADERS_JWT", "VOUCH_HEADERS_SUB",
135134
"VOUCH_HEADERS_USER", "VOUCH_HEADERS_QUERYSTRING", "VOUCH_HEADERS_REDIRECT", "VOUCH_HEADERS_SUCCESS", "VOUCH_HEADERS_ERROR",
136135
"VOUCH_HEADERS_CLAIMHEADER", "VOUCH_HEADERS_ACCESSTOKEN", "VOUCH_HEADERS_IDTOKEN", "VOUCH_COOKIE_NAME", "VOUCH_COOKIE_DOMAIN",
137136
"VOUCH_COOKIE_SAMESITE", "VOUCH_TESTURL", "VOUCH_SESSION_NAME", "VOUCH_SESSION_KEY", "VOUCH_DOCUMENT_ROOT"}
@@ -168,7 +167,7 @@ func Test_configureFromEnvCfg(t *testing.T) {
168167

169168
// run the thing
170169
configureFromEnv()
171-
scfg := []string{Cfg.Listen, Cfg.JWT.Issuer, Cfg.JWT.Secret, Cfg.Headers.JWT,
170+
scfg := []string{Cfg.Listen, Cfg.JWT.Issuer, Cfg.JWT.Secret, Cfg.Headers.JWT, Cfg.Headers.Sub,
172171
Cfg.Headers.User, Cfg.Headers.QueryString, Cfg.Headers.Redirect, Cfg.Headers.Success, Cfg.Headers.Error,
173172
Cfg.Headers.ClaimHeader, Cfg.Headers.AccessToken, Cfg.Headers.IDToken, Cfg.Cookie.Name, Cfg.Cookie.Domain,
174173
Cfg.Cookie.SameSite, Cfg.TestURL, Cfg.Session.Name, Cfg.Session.Key, Cfg.DocumentRoot,

‎pkg/jwtmanager/jwtmanager.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ const comma = ","
3333

3434
// VouchClaims jwt Claims specific to vouch
3535
type VouchClaims struct {
36+
Sub string `json:"sub"`
3637
Username string `json:"username"`
3738
CustomClaims map[string]interface{}
3839
PAccessToken string
@@ -79,6 +80,7 @@ func NewVPJWT(u structs.User, customClaims structs.CustomClaims, ptokens structs
7980
// User`token`
8081
// u.PrepareUserData()
8182
claims := VouchClaims{
83+
u.Sub,
8284
u.Username,
8385
customClaims.Claims,
8486
ptokens.PAccessToken,

‎pkg/jwtmanager/jwtmanager_test.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424

2525
var (
2626
u1 = structs.User{
27+
Sub: "testsub",
2728
Username: "test@testing.com",
2829
Name: "Test Name",
2930
}
@@ -49,6 +50,7 @@ func init() {
4950
Configure()
5051

5152
lc = VouchClaims{
53+
u1.Sub,
5254
u1.Username,
5355
customClaims.Claims,
5456
t1.PAccessToken,

‎pkg/providers/github/github_test.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,7 @@ func TestGetUserInfo(t *testing.T) {
171171
{
172172
"avatar_url": "avatar-url",
173173
"email": "email@example.com",
174+
"id": 123456789,
174175
"login": "myusername",
175176
"name": "name"
176177
}
@@ -188,6 +189,7 @@ func TestGetUserInfo(t *testing.T) {
188189
err := provider.GetUserInfo(nil, user, &structs.CustomClaims{}, &structs.PTokens{})
189190

190191
assert.Nil(t, err)
192+
assert.Equal(t, "123456789", user.Sub)
191193
assert.Equal(t, "myusername", user.Username)
192194
assert.Equal(t, []string{"myOtherOrg", "myorg/myteam"}, user.TeamMemberships)
193195

0 commit comments

Comments
 (0)