From 55fbcfb3be6c24836911864303c3151b4c7a7a14 Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Thu, 29 Apr 2021 15:44:21 +0900 Subject: [PATCH 1/9] Implement #550 - Read `preferred_username` from token - Add `preferred_username` to the default Usernames - Check the `admin` array for admin groups that the user might belong to --- authority/provisioner/oidc.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/authority/provisioner/oidc.go b/authority/provisioner/oidc.go index fa3ca762..4141171c 100644 --- a/authority/provisioner/oidc.go +++ b/authority/provisioner/oidc.go @@ -44,6 +44,7 @@ type openIDPayload struct { AuthorizedParty string `json:"azp"` Email string `json:"email"` EmailVerified bool `json:"email_verified"` + Username string `json:"preferred_username"` Hd string `json:"hd"` Nonce string `json:"nonce"` Groups []string `json:"groups"` @@ -86,6 +87,21 @@ func (o *OIDC) IsAdmin(email string) bool { return false } +// IsAdmin returns true if the given groups is in the Admins allowlist, false +// otherwise. +func (o *OIDC) IsAdminGroup(groups []string) bool { + for _,g := range groups { + // The groups and emails can be in the same array for now, but consider + // making a specialized option later. + for _,gadmin := range o.Admins { + if g == gadmin { + return true + } + } + } + return false +} + func sanitizeEmail(email string) string { if i := strings.LastIndex(email, "@"); i >= 0 { email = email[:i] + strings.ToLower(email[i:]) @@ -377,6 +393,11 @@ func (o *OIDC) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption if err != nil { return nil, errs.Wrap(http.StatusInternalServerError, err, "oidc.AuthorizeSSHSign") } + // Reuse the contains function provided for simplicity + if !containsAllMembers(iden.Usernames, []string{claims.Username}){ + // Add preferred_username to the identity's Username + iden.Usernames = append(iden.Usernames, claims.Username) + } // Certificate templates. data := sshutil.CreateTemplateData(sshutil.UserCert, claims.Email, iden.Usernames) @@ -395,6 +416,9 @@ func (o *OIDC) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption // Use the default template unless no-templates are configured and email is // an admin, in that case we will use the parameters in the request. isAdmin := o.IsAdmin(claims.Email) + if !isAdmin && len(claims.Groups)>0 { + isAdmin = o.IsAdminGroup(claims.Groups) + } defaultTemplate := sshutil.DefaultTemplate if isAdmin && !o.Options.GetSSHOptions().HasTemplate() { defaultTemplate = sshutil.DefaultAdminTemplate From 861ef80e0d8f22351407a9cbb18510947e3b0305 Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Fri, 30 Apr 2021 08:44:41 +0900 Subject: [PATCH 2/9] Rename and reformat to PreferredUsername --- authority/provisioner/oidc.go | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/authority/provisioner/oidc.go b/authority/provisioner/oidc.go index 4141171c..0a85875e 100644 --- a/authority/provisioner/oidc.go +++ b/authority/provisioner/oidc.go @@ -40,14 +40,14 @@ func (c openIDConfiguration) Validate() error { // openIDPayload represents the fields on the id_token JWT payload. type openIDPayload struct { jose.Claims - AtHash string `json:"at_hash"` - AuthorizedParty string `json:"azp"` - Email string `json:"email"` - EmailVerified bool `json:"email_verified"` - Username string `json:"preferred_username"` - Hd string `json:"hd"` - Nonce string `json:"nonce"` - Groups []string `json:"groups"` + AtHash string `json:"at_hash"` + AuthorizedParty string `json:"azp"` + Email string `json:"email"` + EmailVerified bool `json:"email_verified"` + PreferredUsername string `json:"preferred_username"` + Hd string `json:"hd"` + Nonce string `json:"nonce"` + Groups []string `json:"groups"` } // OIDC represents an OAuth 2.0 OpenID Connect provider. @@ -90,10 +90,10 @@ func (o *OIDC) IsAdmin(email string) bool { // IsAdmin returns true if the given groups is in the Admins allowlist, false // otherwise. func (o *OIDC) IsAdminGroup(groups []string) bool { - for _,g := range groups { + for _, g := range groups { // The groups and emails can be in the same array for now, but consider // making a specialized option later. - for _,gadmin := range o.Admins { + for _, gadmin := range o.Admins { if g == gadmin { return true } @@ -394,9 +394,9 @@ func (o *OIDC) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption return nil, errs.Wrap(http.StatusInternalServerError, err, "oidc.AuthorizeSSHSign") } // Reuse the contains function provided for simplicity - if !containsAllMembers(iden.Usernames, []string{claims.Username}){ + if !containsAllMembers(iden.Usernames, []string{claims.PreferredUsername}) { // Add preferred_username to the identity's Username - iden.Usernames = append(iden.Usernames, claims.Username) + iden.Usernames = append(iden.Usernames, claims.PreferredUsername) } // Certificate templates. @@ -416,7 +416,7 @@ func (o *OIDC) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption // Use the default template unless no-templates are configured and email is // an admin, in that case we will use the parameters in the request. isAdmin := o.IsAdmin(claims.Email) - if !isAdmin && len(claims.Groups)>0 { + if !isAdmin && len(claims.Groups) > 0 { isAdmin = o.IsAdminGroup(claims.Groups) } defaultTemplate := sshutil.DefaultTemplate From bf364f0a5f31a7fea35b72cbc9a4759debbf2c7d Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Fri, 30 Apr 2021 09:14:28 +0900 Subject: [PATCH 3/9] Draft: adding usernames to GetIdentityFunc --- authority/options.go | 2 +- authority/provisioner/oidc.go | 7 +------ authority/provisioner/oidc_test.go | 5 +++-- authority/provisioner/provisioner.go | 7 ++++--- 4 files changed, 9 insertions(+), 12 deletions(-) diff --git a/authority/options.go b/authority/options.go index 9594f989..9626f48e 100644 --- a/authority/options.go +++ b/authority/options.go @@ -47,7 +47,7 @@ func WithDatabase(db db.AuthDB) Option { // WithGetIdentityFunc sets a custom function to retrieve the identity from // an external resource. -func WithGetIdentityFunc(fn func(ctx context.Context, p provisioner.Interface, email string) (*provisioner.Identity, error)) Option { +func WithGetIdentityFunc(fn func(ctx context.Context, p provisioner.Interface, email string, usernames ...string) (*provisioner.Identity, error)) Option { return func(a *Authority) error { a.getIdentityFunc = fn return nil diff --git a/authority/provisioner/oidc.go b/authority/provisioner/oidc.go index 0a85875e..787de317 100644 --- a/authority/provisioner/oidc.go +++ b/authority/provisioner/oidc.go @@ -389,15 +389,10 @@ func (o *OIDC) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption // Get the identity using either the default identityFunc or one injected // externally. - iden, err := o.getIdentityFunc(ctx, o, claims.Email) + iden, err := o.getIdentityFunc(ctx, o, claims.Email, claims.PreferredUsername) if err != nil { return nil, errs.Wrap(http.StatusInternalServerError, err, "oidc.AuthorizeSSHSign") } - // Reuse the contains function provided for simplicity - if !containsAllMembers(iden.Usernames, []string{claims.PreferredUsername}) { - // Add preferred_username to the identity's Username - iden.Usernames = append(iden.Usernames, claims.PreferredUsername) - } // Certificate templates. data := sshutil.CreateTemplateData(sshutil.UserCert, claims.Email, iden.Usernames) diff --git a/authority/provisioner/oidc_test.go b/authority/provisioner/oidc_test.go index b0e2f2f4..d203516c 100644 --- a/authority/provisioner/oidc_test.go +++ b/authority/provisioner/oidc_test.go @@ -500,12 +500,13 @@ func TestOIDC_AuthorizeSSHSign(t *testing.T) { assert.FatalError(t, p4.Init(config)) assert.FatalError(t, p5.Init(config)) - p4.getIdentityFunc = func(ctx context.Context, p Interface, email string) (*Identity, error) { + p4.getIdentityFunc = func(ctx context.Context, p Interface, email string, usernames ...string) (*Identity, error) { return &Identity{Usernames: []string{"max", "mariano"}}, nil } - p5.getIdentityFunc = func(ctx context.Context, p Interface, email string) (*Identity, error) { + p5.getIdentityFunc = func(ctx context.Context, p Interface, email string, usernames ...string) (*Identity, error) { return nil, errors.New("force") } + // Additional test needed for empty usernames and duplicate email and usernames t1, err := generateSimpleToken("the-issuer", p1.ClientID, &keys.Keys[0]) assert.FatalError(t, err) diff --git a/authority/provisioner/provisioner.go b/authority/provisioner/provisioner.go index aed1900a..8cf42953 100644 --- a/authority/provisioner/provisioner.go +++ b/authority/provisioner/provisioner.go @@ -337,10 +337,10 @@ type Permissions struct { } // GetIdentityFunc is a function that returns an identity. -type GetIdentityFunc func(ctx context.Context, p Interface, email string) (*Identity, error) +type GetIdentityFunc func(ctx context.Context, p Interface, email string, usernames ...string) (*Identity, error) // DefaultIdentityFunc return a default identity depending on the provisioner type. -func DefaultIdentityFunc(ctx context.Context, p Interface, email string) (*Identity, error) { +func DefaultIdentityFunc(ctx context.Context, p Interface, email string, usernames ...string) (*Identity, error) { switch k := p.(type) { case *OIDC: // OIDC principals would be: @@ -351,13 +351,14 @@ func DefaultIdentityFunc(ctx context.Context, p Interface, email string) (*Ident if !sshUserRegex.MatchString(name) { return nil, errors.Errorf("invalid principal '%s' from email '%s'", name, email) } - usernames := []string{name} + usernames := append(usernames, name) if i := strings.LastIndex(email, "@"); i >= 0 { if local := email[:i]; !strings.EqualFold(local, name) { usernames = append(usernames, local) } } usernames = append(usernames, email) + // Some remove duplicate function should be added return &Identity{ Usernames: usernames, }, nil From 8b1ab3021251d6676f9d385b4048615e51f4fa8f Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Fri, 30 Apr 2021 09:41:06 +0900 Subject: [PATCH 4/9] Sanitize usernames --- authority/provisioner/provisioner.go | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/authority/provisioner/provisioner.go b/authority/provisioner/provisioner.go index 8cf42953..30bf5d47 100644 --- a/authority/provisioner/provisioner.go +++ b/authority/provisioner/provisioner.go @@ -358,7 +358,7 @@ func DefaultIdentityFunc(ctx context.Context, p Interface, email string, usernam } } usernames = append(usernames, email) - // Some remove duplicate function should be added + usernames = SanitizeStringSlices(usernames) return &Identity{ Usernames: usernames, }, nil @@ -367,6 +367,21 @@ func DefaultIdentityFunc(ctx context.Context, p Interface, email string, usernam } } +func SanitizeStringSlices(original []string) []string { + output := []string{} + seen := make(map[string]bool) + for _, entry := range original { + if entry == "" { + continue + } + if _, value := seen[entry]; !value { + seen[entry] = true + output = append(output, entry) + } + } + return output +} + // MockProvisioner for testing type MockProvisioner struct { Mret1, Mret2, Mret3 interface{} From c8eb771a8ec57dd129560530a48288a0a1dbddd8 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 29 Apr 2021 18:30:26 -0700 Subject: [PATCH 5/9] Add test for oidc with preferred usernames. --- authority/provisioner/oidc.go | 2 +- authority/provisioner/oidc_test.go | 13 ++++--- authority/provisioner/provisioner.go | 19 ++++++----- authority/provisioner/provisioner_test.go | 37 ++++++++++++++++---- authority/provisioner/utils_test.go | 41 +++++++++++++++++++++++ 5 files changed, 92 insertions(+), 20 deletions(-) diff --git a/authority/provisioner/oidc.go b/authority/provisioner/oidc.go index 787de317..3b2d81bf 100644 --- a/authority/provisioner/oidc.go +++ b/authority/provisioner/oidc.go @@ -388,7 +388,7 @@ func (o *OIDC) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption } // Get the identity using either the default identityFunc or one injected - // externally. + // externally. Note that the PreferredUsername might be empty. iden, err := o.getIdentityFunc(ctx, o, claims.Email, claims.PreferredUsername) if err != nil { return nil, errs.Wrap(http.StatusInternalServerError, err, "oidc.AuthorizeSSHSign") diff --git a/authority/provisioner/oidc_test.go b/authority/provisioner/oidc_test.go index d203516c..85c6b1a9 100644 --- a/authority/provisioner/oidc_test.go +++ b/authority/provisioner/oidc_test.go @@ -514,8 +514,10 @@ func TestOIDC_AuthorizeSSHSign(t *testing.T) { assert.FatalError(t, err) failGetIdentityToken, err := generateSimpleToken("the-issuer", p5.ClientID, &keys.Keys[0]) assert.FatalError(t, err) + okPreferredUsername, err := generateOIDCToken("subject", "the-issuer", p1.ClientID, "name@smallstep.com", "lecris", time.Now(), &keys.Keys[0]) + assert.FatalError(t, err) // Admin email not in domains - okAdmin, err := generateToken("subject", "the-issuer", p3.ClientID, "root@example.com", []string{}, time.Now(), &keys.Keys[0]) + okAdmin, err := generateOIDCToken("subject", "the-issuer", p3.ClientID, "root@example.com", "", time.Now(), &keys.Keys[0]) assert.FatalError(t, err) // Empty email failEmail, err := generateToken("subject", "the-issuer", p3.ClientID, "", []string{}, time.Now(), &keys.Keys[0]) @@ -574,14 +576,17 @@ func TestOIDC_AuthorizeSSHSign(t *testing.T) { {"ok-emptyPrincipals-getIdentity", p4, args{okGetIdentityToken, SignSSHOptions{}, pub}, &SignSSHOptions{CertType: "user", Principals: []string{"max", "mariano"}, ValidAfter: NewTimeDuration(tm), ValidBefore: NewTimeDuration(tm.Add(userDuration))}, http.StatusOK, false, false}, + {"ok-preferred-username", p1, args{okPreferredUsername, SignSSHOptions{CertType: "user", KeyID: "name@smallstep.com", Principals: []string{"lecris"}}, pub}, + &SignSSHOptions{CertType: "user", Principals: []string{"lecris", "name", "name@smallstep.com"}, + ValidAfter: NewTimeDuration(tm), ValidBefore: NewTimeDuration(tm.Add(userDuration))}, http.StatusOK, false, false}, {"ok-options", p1, args{t1, SignSSHOptions{CertType: "user", Principals: []string{"name"}}, pub}, &SignSSHOptions{CertType: "user", Principals: []string{"name", "name@smallstep.com"}, ValidAfter: NewTimeDuration(tm), ValidBefore: NewTimeDuration(tm.Add(userDuration))}, http.StatusOK, false, false}, - {"admin-user", p3, args{okAdmin, SignSSHOptions{CertType: "user", KeyID: "root@example.com", Principals: []string{"root", "root@example.com"}}, pub}, + {"ok-admin-user", p3, args{okAdmin, SignSSHOptions{CertType: "user", KeyID: "root@example.com", Principals: []string{"root", "root@example.com"}}, pub}, expectedAdminOptions, http.StatusOK, false, false}, - {"admin-host", p3, args{okAdmin, SignSSHOptions{CertType: "host", KeyID: "smallstep.com", Principals: []string{"smallstep.com"}}, pub}, + {"ok-admin-host", p3, args{okAdmin, SignSSHOptions{CertType: "host", KeyID: "smallstep.com", Principals: []string{"smallstep.com"}}, pub}, expectedHostOptions, http.StatusOK, false, false}, - {"admin-options", p3, args{okAdmin, SignSSHOptions{CertType: "user", KeyID: "name", Principals: []string{"name"}}, pub}, + {"ok-admin-options", p3, args{okAdmin, SignSSHOptions{CertType: "user", KeyID: "name", Principals: []string{"name"}}, pub}, &SignSSHOptions{CertType: "user", Principals: []string{"name"}, ValidAfter: NewTimeDuration(tm), ValidBefore: NewTimeDuration(tm.Add(userDuration))}, http.StatusOK, false, false}, {"fail-rsa1024", p1, args{t1, SignSSHOptions{}, rsa1024.Public()}, expectedUserOptions, http.StatusOK, false, true}, diff --git a/authority/provisioner/provisioner.go b/authority/provisioner/provisioner.go index 30bf5d47..1ea48069 100644 --- a/authority/provisioner/provisioner.go +++ b/authority/provisioner/provisioner.go @@ -339,34 +339,35 @@ type Permissions struct { // GetIdentityFunc is a function that returns an identity. type GetIdentityFunc func(ctx context.Context, p Interface, email string, usernames ...string) (*Identity, error) -// DefaultIdentityFunc return a default identity depending on the provisioner type. +// DefaultIdentityFunc return a default identity depending on the provisioner +// type. For OIDC email is always present and the usernames might +// contain empty strings. func DefaultIdentityFunc(ctx context.Context, p Interface, email string, usernames ...string) (*Identity, error) { switch k := p.(type) { case *OIDC: // OIDC principals would be: - // 1. Sanitized local. - // 2. Raw local (if different). - // 3. Email address. + // 1. Preferred usernames. + // 2. Sanitized local. + // 3. Raw local (if different). + // 4. Email address. name := SanitizeSSHUserPrincipal(email) if !sshUserRegex.MatchString(name) { return nil, errors.Errorf("invalid principal '%s' from email '%s'", name, email) } usernames := append(usernames, name) if i := strings.LastIndex(email, "@"); i >= 0 { - if local := email[:i]; !strings.EqualFold(local, name) { - usernames = append(usernames, local) - } + usernames = append(usernames, email[:i]) } usernames = append(usernames, email) - usernames = SanitizeStringSlices(usernames) return &Identity{ - Usernames: usernames, + Usernames: SanitizeStringSlices(usernames), }, nil default: return nil, errors.Errorf("provisioner type '%T' not supported by identity function", k) } } +// SanitizeStringSlices removes duplicated an empty strings. func SanitizeStringSlices(original []string) []string { output := []string{} seen := make(map[string]bool) diff --git a/authority/provisioner/provisioner_test.go b/authority/provisioner/provisioner_test.go index bf99aa76..f1cf2ff5 100644 --- a/authority/provisioner/provisioner_test.go +++ b/authority/provisioner/provisioner_test.go @@ -62,10 +62,11 @@ func TestSanitizeSSHUserPrincipal(t *testing.T) { func TestDefaultIdentityFunc(t *testing.T) { type test struct { - p Interface - email string - err error - identity *Identity + p Interface + email string + usernames []string + err error + identity *Identity } tests := map[string]func(*testing.T) test{ "fail/unsupported-provisioner": func(t *testing.T) test { @@ -106,7 +107,7 @@ func TestDefaultIdentityFunc(t *testing.T) { return test{ p: &OIDC{}, email: "John@smallstep.com", - identity: &Identity{Usernames: []string{"john", "John@smallstep.com"}}, + identity: &Identity{Usernames: []string{"john", "John", "John@smallstep.com"}}, } }, "ok symbol": func(t *testing.T) test { @@ -116,11 +117,35 @@ func TestDefaultIdentityFunc(t *testing.T) { identity: &Identity{Usernames: []string{"john_doe", "John+Doe", "John+Doe@smallstep.com"}}, } }, + "ok username": func(t *testing.T) test { + return test{ + p: &OIDC{}, + email: "john@smallstep.com", + usernames: []string{"johnny"}, + identity: &Identity{Usernames: []string{"johnny", "john", "john@smallstep.com"}}, + } + }, + "ok usernames": func(t *testing.T) test { + return test{ + p: &OIDC{}, + email: "john@smallstep.com", + usernames: []string{"johnny", "js", ""}, + identity: &Identity{Usernames: []string{"johnny", "js", "john", "john@smallstep.com"}}, + } + }, + "ok empty username": func(t *testing.T) test { + return test{ + p: &OIDC{}, + email: "john@smallstep.com", + usernames: []string{""}, + identity: &Identity{Usernames: []string{"john", "john@smallstep.com"}}, + } + }, } for name, get := range tests { t.Run(name, func(t *testing.T) { tc := get(t) - identity, err := DefaultIdentityFunc(context.Background(), tc.p, tc.email) + identity, err := DefaultIdentityFunc(context.Background(), tc.p, tc.email, tc.usernames...) if err != nil { if assert.NotNil(t, tc.err) { assert.Equals(t, tc.err.Error(), err.Error()) diff --git a/authority/provisioner/utils_test.go b/authority/provisioner/utils_test.go index fb0eb9e7..534e83cf 100644 --- a/authority/provisioner/utils_test.go +++ b/authority/provisioner/utils_test.go @@ -773,6 +773,47 @@ func generateToken(sub, iss, aud string, email string, sans []string, iat time.T return jose.Signed(sig).Claims(claims).CompactSerialize() } +func generateOIDCToken(sub, iss, aud string, email string, preferredUsername string, iat time.Time, jwk *jose.JSONWebKey, tokOpts ...tokOption) (string, error) { + so := new(jose.SignerOptions) + so.WithType("JWT") + so.WithHeader("kid", jwk.KeyID) + + for _, o := range tokOpts { + if err := o(so); err != nil { + return "", err + } + } + + sig, err := jose.NewSigner(jose.SigningKey{Algorithm: jose.ES256, Key: jwk.Key}, so) + if err != nil { + return "", err + } + + id, err := randutil.ASCII(64) + if err != nil { + return "", err + } + + claims := struct { + jose.Claims + Email string `json:"email"` + PreferredUsername string `json:"preferred_username,omitempty"` + }{ + Claims: jose.Claims{ + ID: id, + Subject: sub, + Issuer: iss, + IssuedAt: jose.NewNumericDate(iat), + NotBefore: jose.NewNumericDate(iat), + Expiry: jose.NewNumericDate(iat.Add(5 * time.Minute)), + Audience: []string{aud}, + }, + Email: email, + PreferredUsername: preferredUsername, + } + return jose.Signed(sig).Claims(claims).CompactSerialize() +} + func generateX5CSSHToken(jwk *jose.JSONWebKey, claims *x5cPayload, tokOpts ...tokOption) (string, error) { so := new(jose.SignerOptions) so.WithType("JWT") From 9cc410b30835961a9b5e388d058890c44f3a66e5 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 29 Apr 2021 18:40:04 -0700 Subject: [PATCH 6/9] Use map[string]struct{} instead of map[string]bool --- authority/provisioner/provisioner.go | 4 ++-- authority/provisioner/provisioner_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/authority/provisioner/provisioner.go b/authority/provisioner/provisioner.go index 1ea48069..c994a5c2 100644 --- a/authority/provisioner/provisioner.go +++ b/authority/provisioner/provisioner.go @@ -370,13 +370,13 @@ func DefaultIdentityFunc(ctx context.Context, p Interface, email string, usernam // SanitizeStringSlices removes duplicated an empty strings. func SanitizeStringSlices(original []string) []string { output := []string{} - seen := make(map[string]bool) + seen := make(map[string]struct{}) for _, entry := range original { if entry == "" { continue } if _, value := seen[entry]; !value { - seen[entry] = true + seen[entry] = struct{}{} output = append(output, entry) } } diff --git a/authority/provisioner/provisioner_test.go b/authority/provisioner/provisioner_test.go index f1cf2ff5..267fb7d1 100644 --- a/authority/provisioner/provisioner_test.go +++ b/authority/provisioner/provisioner_test.go @@ -129,7 +129,7 @@ func TestDefaultIdentityFunc(t *testing.T) { return test{ p: &OIDC{}, email: "john@smallstep.com", - usernames: []string{"johnny", "js", ""}, + usernames: []string{"johnny", "js", "", "johnny", ""}, identity: &Identity{Usernames: []string{"johnny", "js", "john", "john@smallstep.com"}}, } }, From 484b30d0a15a96b55fc0dac478f37fd5700be369 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 29 Apr 2021 18:47:17 -0700 Subject: [PATCH 7/9] Fix IsAdminGroup comment. --- authority/provisioner/oidc.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/authority/provisioner/oidc.go b/authority/provisioner/oidc.go index 3b2d81bf..79a10ffd 100644 --- a/authority/provisioner/oidc.go +++ b/authority/provisioner/oidc.go @@ -87,8 +87,8 @@ func (o *OIDC) IsAdmin(email string) bool { return false } -// IsAdmin returns true if the given groups is in the Admins allowlist, false -// otherwise. +// IsAdminGroup returns true if the one group in the given list is in the Admins +// allowlist, false otherwise. func (o *OIDC) IsAdminGroup(groups []string) bool { for _, g := range groups { // The groups and emails can be in the same array for now, but consider From e5b206c1de8ce4515419524f71f28e3b5ff8bd32 Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Tue, 4 May 2021 13:47:17 +0900 Subject: [PATCH 8/9] Fix shadow issue in CI --- authority/provisioner/provisioner.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/authority/provisioner/provisioner.go b/authority/provisioner/provisioner.go index c994a5c2..6ffab03d 100644 --- a/authority/provisioner/provisioner.go +++ b/authority/provisioner/provisioner.go @@ -354,7 +354,7 @@ func DefaultIdentityFunc(ctx context.Context, p Interface, email string, usernam if !sshUserRegex.MatchString(name) { return nil, errors.Errorf("invalid principal '%s' from email '%s'", name, email) } - usernames := append(usernames, name) + usernames = append(usernames, name) if i := strings.LastIndex(email, "@"); i >= 0 { usernames = append(usernames, email[:i]) } From bb1e051b278650cd7f7cfe00df91070e74aca21c Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Wed, 5 May 2021 08:12:17 +0900 Subject: [PATCH 9/9] Revert using preferred_username It might present a security issue if the users can change this value for themselves. Needs further investigation --- authority/provisioner/oidc.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/authority/provisioner/oidc.go b/authority/provisioner/oidc.go index 79a10ffd..33988a0a 100644 --- a/authority/provisioner/oidc.go +++ b/authority/provisioner/oidc.go @@ -389,7 +389,8 @@ func (o *OIDC) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption // Get the identity using either the default identityFunc or one injected // externally. Note that the PreferredUsername might be empty. - iden, err := o.getIdentityFunc(ctx, o, claims.Email, claims.PreferredUsername) + // TBD: Would preferred_username present a safety issue here? + iden, err := o.getIdentityFunc(ctx, o, claims.Email) if err != nil { return nil, errs.Wrap(http.StatusInternalServerError, err, "oidc.AuthorizeSSHSign") }