Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for sub claim, not username, when validating #310

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .defaults.yml
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ vouch:
# key:

headers:
sub: X-Vouch-Sub
jwt: X-Vouch-Token
user: X-Vouch-User
success: X-Vouch-Success
Expand Down
2 changes: 2 additions & 0 deletions config/config.yml_example
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
51 changes: 44 additions & 7 deletions handlers/handlers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)

Expand All @@ -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)
Expand All @@ -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",
}
Expand All @@ -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,
Expand Down
13 changes: 8 additions & 5 deletions handlers/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}

Expand All @@ -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 != "" {
Expand Down
28 changes: 24 additions & 4 deletions handlers/validate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{}

Expand Down Expand Up @@ -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{}

Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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{}

Expand Down
1 change: 1 addition & 0 deletions pkg/cfg/cfg.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand Down
5 changes: 2 additions & 3 deletions pkg/cfg/cfg_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"}
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions pkg/jwtmanager/jwtmanager.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions pkg/jwtmanager/jwtmanager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (

var (
u1 = structs.User{
Sub: "testsub",
Username: "[email protected]",
Name: "Test Name",
}
Expand All @@ -49,6 +50,7 @@ func init() {
Configure()

lc = VouchClaims{
u1.Sub,
u1.Username,
customClaims.Claims,
t1.PAccessToken,
Expand Down
15 changes: 3 additions & 12 deletions pkg/providers/github/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -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, "/")
Expand Down Expand Up @@ -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
}

Expand Down
25 changes: 12 additions & 13 deletions pkg/providers/github/github_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ OR CONDITIONS OF ANY KIND, either express or implied.
package github

import (
"encoding/json"
"net/http"
"regexp"
"testing"
Expand Down Expand Up @@ -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")
Expand All @@ -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)

Expand Down
1 change: 0 additions & 1 deletion pkg/providers/openstax/openstax.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Loading