Skip to content

Commit

Permalink
Check for sub claim, not username, when validating
Browse files Browse the repository at this point in the history
Not all IdPs provide a `username` or `email` claim in the UserInfo
response.
  • Loading branch information
rhansen committed Aug 12, 2020
1 parent 7f2d675 commit 7feca14
Show file tree
Hide file tree
Showing 8 changed files with 83 additions and 19 deletions.
1 change: 1 addition & 0 deletions .defaults.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ vouch:
# key:

headers:
sub: X-Vouch-Sub
jwt: X-Vouch-Token
user: X-Vouch-User
success: X-Vouch-Success
Expand Down
51 changes: 44 additions & 7 deletions handlers/handlers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,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 @@ -55,7 +60,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 @@ -64,7 +74,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 @@ -74,7 +89,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 @@ -84,7 +104,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 @@ -95,7 +120,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 @@ -105,7 +135,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 @@ -116,6 +151,7 @@ func TestVerifyUserNegative(t *testing.T) {
// it should live there but circular imports are resolved if it lives here
var (
u1 = structs.User{
Sub: "test",
Username: "[email protected]",
Name: "Test Name",
}
Expand All @@ -141,6 +177,7 @@ func init() {
// log.SetLevel(log.DebugLevel)

lc = jwtmanager.VouchClaims{
u1.Sub,
u1.Username,
jwtmanager.Sites,
customClaims.Claims,
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 @@ -66,7 +71,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 @@ -153,7 +163,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",
}
userTokenString := jwtmanager.CreateUserTokenString(*user, customClaims, tokens)

req, err := http.NewRequest("GET", "/validate", nil)
Expand Down Expand Up @@ -205,7 +220,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 @@ -59,6 +59,7 @@ type Config struct {
}

Headers struct {
Sub string `mapstructure:"sub"`
JWT string `mapstructure:"jwt"`
User string `mapstructure:"user"`
QueryString string `mapstructure:"querystring"`
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 @@ import (

// VouchClaims jwt Claims specific to vouch
type VouchClaims struct {
Sub string `json:"sub"`
Username string `json:"username"`
Sites []string `json:"sites"` // tempting to make this a map but the array is fewer characters in the jwt
CustomClaims map[string]interface{}
Expand Down Expand Up @@ -78,6 +79,7 @@ func CreateUserTokenString(u structs.User, customClaims structs.CustomClaims, pt
// User`token`
// u.PrepareUserData()
claims := VouchClaims{
u.Sub,
u.Username,
Sites,
customClaims.Claims,
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 @@ -22,6 +22,7 @@ import (

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

lc = VouchClaims{
u1.Sub,
u1.Username,
Sites,
customClaims.Claims,
Expand Down
4 changes: 1 addition & 3 deletions pkg/structs/structs.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ type UserI interface {

// User is inherited.
type User struct {
Sub string `json:"sub"`
// TODO: set Provider here so that we can pass it to db
// populated by db (via mapstructure) or from provider (via json)
// Provider string `json:"provider",mapstructure:"provider"`
Expand All @@ -47,7 +48,6 @@ func (u *User) PrepareUserData() {
// AzureUser is a retrieved and authenticated user from Azure AD
type AzureUser struct {
User
Sub string `json:"sub"`
UPN string `json:"upn"`
}

Expand All @@ -71,7 +71,6 @@ func (u *AzureUser) PrepareUserData() {
// https://golang.org/doc/effective_go.html#embedding
type GoogleUser struct {
User
Sub string `json:"sub"`
GivenName string `json:"given_name"`
FamilyName string `json:"family_name"`
Profile string `json:"profile"`
Expand All @@ -90,7 +89,6 @@ func (u *GoogleUser) 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"`
Expand Down

0 comments on commit 7feca14

Please sign in to comment.