From 55fbcfb3be6c24836911864303c3151b4c7a7a14 Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Thu, 29 Apr 2021 15:44:21 +0900 Subject: [PATCH 01/29] 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 2cbaee9c1dd67644b93f0316c48ebd294430cd0d Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 29 Apr 2021 15:55:22 -0700 Subject: [PATCH 02/29] Allow to use an alternative interface to store renewed certs. This can be useful to know if a certificate has been renewed and link one certificate with the 'parent'. --- authority/tls.go | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/authority/tls.go b/authority/tls.go index bc160ad0..b7b2f936 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -263,7 +263,7 @@ func (a *Authority) Rekey(oldCert *x509.Certificate, pk crypto.PublicKey) ([]*x5 } fullchain := append([]*x509.Certificate{resp.Certificate}, resp.CertificateChain...) - if err = a.storeCertificate(fullchain); err != nil { + if err = a.storeRenewedCertificate(oldCert, fullchain); err != nil { if err != db.ErrNotImplemented { return nil, errs.Wrap(http.StatusInternalServerError, err, "authority.Rekey; error storing certificate in db", opts...) } @@ -287,6 +287,19 @@ func (a *Authority) storeCertificate(fullchain []*x509.Certificate) error { return a.db.StoreCertificate(fullchain[0]) } +// storeRenewedCertificate allows to use an extension of the db.AuthDB interface +// that can log if a certificate has been renewed or rekeyed. +// +// TODO: at some point we should implement this in the standard implementation. +func (a *Authority) storeRenewedCertificate(oldCert *x509.Certificate, fullchain []*x509.Certificate) error { + if s, ok := a.db.(interface { + StoreRenewedCertificate(*x509.Certificate, ...*x509.Certificate) error + }); ok { + return s.StoreRenewedCertificate(oldCert, fullchain...) + } + return a.db.StoreCertificate(fullchain[0]) +} + // RevokeOptions are the options for the Revoke API. type RevokeOptions struct { Serial string From 5846314f881561a7ecda4782f6621fc546d3d47d Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 29 Apr 2021 16:06:45 -0700 Subject: [PATCH 03/29] Add missing Rekey method to the ca.Client Fixes #315 --- ca/client.go | 30 +++++++++++++++++++++ ca/client_test.go | 69 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+) diff --git a/ca/client.go b/ca/client.go index 19f758f1..2292c41e 100644 --- a/ca/client.go +++ b/ca/client.go @@ -616,6 +616,36 @@ retry: return &sign, nil } +// Rekey performs the rekey request to the CA and returns the api.SignResponse +// struct. +func (c *Client) Rekey(req *api.RekeyRequest, tr http.RoundTripper) (*api.SignResponse, error) { + var retried bool + body, err := json.Marshal(req) + if err != nil { + return nil, errors.Wrap(err, "error marshaling request") + } + + u := c.endpoint.ResolveReference(&url.URL{Path: "/rekey"}) + client := &http.Client{Transport: tr} +retry: + resp, err := client.Post(u.String(), "application/json", bytes.NewReader(body)) + if err != nil { + return nil, errs.Wrapf(http.StatusInternalServerError, err, "client.Rekey; client POST %s failed", u) + } + if resp.StatusCode >= 400 { + if !retried && c.retryOnError(resp) { + retried = true + goto retry + } + return nil, readError(resp.Body) + } + var sign api.SignResponse + if err := readJSON(resp.Body, &sign); err != nil { + return nil, errs.Wrapf(http.StatusInternalServerError, err, "client.Rekey; error reading %s", u) + } + return &sign, nil +} + // Revoke performs the revoke request to the CA and returns the api.RevokeResponse // struct. func (c *Client) Revoke(req *api.RevokeRequest, tr http.RoundTripper) (*api.RevokeResponse, error) { diff --git a/ca/client_test.go b/ca/client_test.go index dbba4d4c..30669e6e 100644 --- a/ca/client_test.go +++ b/ca/client_test.go @@ -529,6 +529,75 @@ func TestClient_Renew(t *testing.T) { } } +func TestClient_Rekey(t *testing.T) { + ok := &api.SignResponse{ + ServerPEM: api.Certificate{Certificate: parseCertificate(certPEM)}, + CaPEM: api.Certificate{Certificate: parseCertificate(rootPEM)}, + CertChainPEM: []api.Certificate{ + {Certificate: parseCertificate(certPEM)}, + {Certificate: parseCertificate(rootPEM)}, + }, + } + + request := &api.RekeyRequest{ + CsrPEM: api.CertificateRequest{CertificateRequest: parseCertificateRequest(csrPEM)}, + } + + tests := []struct { + name string + request *api.RekeyRequest + response interface{} + responseCode int + wantErr bool + err error + }{ + {"ok", request, ok, 200, false, nil}, + {"unauthorized", request, errs.Unauthorized("force"), 401, true, errors.New(errs.UnauthorizedDefaultMsg)}, + {"empty request", &api.RekeyRequest{}, errs.BadRequest("force"), 400, true, errors.New(errs.BadRequestDefaultMsg)}, + {"nil request", nil, errs.BadRequest("force"), 400, true, errors.New(errs.BadRequestDefaultMsg)}, + } + + srv := httptest.NewServer(nil) + defer srv.Close() + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c, err := NewClient(srv.URL, WithTransport(http.DefaultTransport)) + if err != nil { + t.Errorf("NewClient() error = %v", err) + return + } + + srv.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + api.JSONStatus(w, tt.response, tt.responseCode) + }) + + got, err := c.Rekey(tt.request, nil) + if (err != nil) != tt.wantErr { + fmt.Printf("%+v", err) + t.Errorf("Client.Renew() error = %v, wantErr %v", err, tt.wantErr) + return + } + + switch { + case err != nil: + if got != nil { + t.Errorf("Client.Renew() = %v, want nil", got) + } + + sc, ok := err.(errs.StatusCoder) + assert.Fatal(t, ok, "error does not implement StatusCoder interface") + assert.Equals(t, sc.StatusCode(), tt.responseCode) + assert.HasPrefix(t, tt.err.Error(), err.Error()) + default: + if !reflect.DeepEqual(got, tt.response) { + t.Errorf("Client.Renew() = %v, want %v", got, tt.response) + } + } + }) + } +} + func TestClient_Provisioners(t *testing.T) { ok := &api.ProvisionersResponse{ Provisioners: provisioner.List{}, From 861ef80e0d8f22351407a9cbb18510947e3b0305 Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Fri, 30 Apr 2021 08:44:41 +0900 Subject: [PATCH 04/29] 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 05/29] 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 06/29] 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 07/29] 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 08/29] 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 09/29] 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 25325b69706da897a65b105b2822453d2eedb82d Mon Sep 17 00:00:00 2001 From: Carl Tashian Date: Mon, 3 May 2021 16:18:56 -0700 Subject: [PATCH 10/29] Revert systemd renewer unit change that was incorrect This reverts commit 75f24a103adeee22a24e2b5e3bcd664c8d43340c. --- systemd/cert-renewer@.service | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/systemd/cert-renewer@.service b/systemd/cert-renewer@.service index 0cac0fbf..f38951b5 100644 --- a/systemd/cert-renewer@.service +++ b/systemd/cert-renewer@.service @@ -26,7 +26,7 @@ ExecStart=/usr/bin/step ca renew --force $CERT_LOCATION $KEY_LOCATION ; Try to reload or restart the systemd service that relies on this cert-renewer ; If the relying service doesn't exist, forge ahead. -ExecStartPost=-systemctl try-reload-or-restart %i +ExecStartPost=/usr/bin/env bash -c "if ! systemctl --quiet is-enabled %i.service ; then exit 0; fi; systemctl try-reload-or-restart %i" [Install] WantedBy=multi-user.target From e5b206c1de8ce4515419524f71f28e3b5ff8bd32 Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Tue, 4 May 2021 13:47:17 +0900 Subject: [PATCH 11/29] 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 bc4bf224e88d957e4b8177532a6a98b66589cd7e Mon Sep 17 00:00:00 2001 From: max furman Date: Tue, 4 May 2021 11:29:53 -0700 Subject: [PATCH 12/29] [action] Add needs-triage labeler --- .github/needs-triage-labeler.yml | 2 ++ .github/workflows/labeler.yml | 14 ++++++++++++++ 2 files changed, 16 insertions(+) create mode 100644 .github/needs-triage-labeler.yml create mode 100644 .github/workflows/labeler.yml diff --git a/.github/needs-triage-labeler.yml b/.github/needs-triage-labeler.yml new file mode 100644 index 00000000..4f507a77 --- /dev/null +++ b/.github/needs-triage-labeler.yml @@ -0,0 +1,2 @@ +needs triage: +- "**" diff --git a/.github/workflows/labeler.yml b/.github/workflows/labeler.yml new file mode 100644 index 00000000..9311145c --- /dev/null +++ b/.github/workflows/labeler.yml @@ -0,0 +1,14 @@ +name: labeler +on: + pull_request: + branches: + - master + +jobs: + label: + runs-on: ubuntu-latest + steps: + - uses: actions/labeler@v2 + with: + repo-token: "${{ secrets.GITHUB_TOKEN }}" + configuration-path: .github/needs-triage-labeler.yml From 9a156d22104d5fd75acf310c2ac9d029bd5d119f Mon Sep 17 00:00:00 2001 From: max furman Date: Tue, 4 May 2021 12:30:05 -0700 Subject: [PATCH 13/29] Remove distribution doc. --- distribution.md | 143 ------------------------------------------------ 1 file changed, 143 deletions(-) delete mode 100644 distribution.md diff --git a/distribution.md b/distribution.md deleted file mode 100644 index cf4306f5..00000000 --- a/distribution.md +++ /dev/null @@ -1,143 +0,0 @@ -# Distribution - -This section describes how to build and deploy publicly available releases of -the Step CA. - -## Creating A New Release - -New releases are (almost) entirely built and deployed by Travis-CI. Creating a new -release is as simple as pushing a new github tag. - -**Definitions**: - -* **Standard Release**: ready for public use. no `-rc*` suffix on the version. -e.g. `v1.0.2` -* **Release Candidate**: not ready for public use, still testing. must have a -`-rc*` suffix. e.g. `v1.0.2-rc` or `v1.0.2-rc.4` - ---- -1. **Tag it!** - - 1. Find the most recent tag. - - ``` - $> git fetch --tags - $> git tag - ``` - - The new tag needs to be the logical successor of the most recent existing tag. - See [versioning](#versioning) section for more information on version numbers. - - 2. Select the type and value of the next tag. - - Is the new release a *release candidate* or a *standard release*? - - 1. **Release Candidate** - - If the most recent tag is a standard release, say `v1.0.2`, then the version - of the next release candidate should be `v1.0.3-rc.1`. If the most recent tag - is a release candidate, say `v1.0.2-rc.3`, then the version of the next - release candidate should be `v1.0.2-rc.4`. - - 2. Standard Release - - If the most recent tag is a standard release, say `v1.0.2`, then the version - of the next standard release should be `v1.0.3`. If the most recent tag - is a release candidate, say `v1.0.2-rc.3`, then the version of the next - standard release should be `v1.0.3`. - - - 3. Create a local tag. - - ``` - # standard release - $> git tag v1.0.3 - ...or - # release candidate - $> git tag v1.0.3-rc.1 - ``` - - 4. Push the new tag to the remote origin. - - ``` - # standard release - $> git push origin tag v1.0.3 - ...or - # release candidate - $> git push origin tag v1.0.3-rc.1 - ``` - -2. **Check the build status at** -[Travis-CI](https://travis-ci.com/smallstep/certificates/builds/). - - Travis will begin by verifying that there are no compilation or linting errors - and then run the unit tests. Assuming all the checks have passed, Travis will - build Darwin and Linux artifacts (for easily installing `step`) and upload them - as part of the [Github Release](https://github.com/smallstep/certificates/releases). - - Travis will build and upload the following artifacts: - - * **step-ca_1.0.3_amd64.deb**: debian package for installation on linux. - * **step-ca_linux_1.0.3_amd64.tar.gz**: tarball containing a statically compiled linux binary. - * **step-ca_darwin_1.0.3_amd64.tar.gz**: tarball containing a statically compiled darwin binary. - * **step-ca_1.0.3.tar.gz**: tarball containing a git archive of the full repo. - -3. **Update the AUR Arch Linux package** - - > **NOTE**: if you plan to release `cli` next then you can skip this step. - -

-    $ cd archlinux
-
-    # Get up to date...
-    $ git pull origin master
-    $ make
-
-    $ ./update --ca v1.0.3
-    
- -4. **Update the Helm packages** - - > **NOTE**: This is an optional step, only necessary if we want to release a - > new helm package. - - Once we have the docker images, we can release a new version in our Helm - [repository](https://smallstep.github.io/helm-charts/) we need to pull the - [helm-charts](https://github.com/smallstep/helm-charts) project, and change the - following: - - * On step-certificates/Chart.yaml: - * Increase the `version` number (Helm Chart version). - * Set the `appVersion` to the step-certificates version. - * On step-certificates/values.yaml: - * Set the docker tag `image.tag` to the appropriate version. - - Then create the step-certificates package running: - -

-    $ helm package ./step-certificates
-    
- - A new file like `step-certificates-.tgz` will be created. - Now commit and push your changes (don't commit the tarball) to the master - branch of `smallstep/helm-charts` - - Next checkout the `gh-pages` branch. `git add` the new tar-ball and update - the index.yaml using the `helm repo index` command: - -

-    $ git checkout gh-pages
-    $ git pull origin gh-pages
-    $ git add "step-certificates-.tgz"
-    $ helm repo index --merge index.yaml --url https://smallstep.github.io/helm-charts/ .
-    $ git commit -a -m "Add package for step-certificates vX.Y.Z"
-    $ git push origin gh-pages
-    
- -***All Done!*** - -## Versioning - -We use [SemVer](http://semver.org/) for versioning. See the -[tags on this repository](https://github.com/smallstep/certificates) for all -available versions. From 8c709fe3c2a4d3fd8cd08f35491d75eb17d898d4 Mon Sep 17 00:00:00 2001 From: max furman Date: Tue, 4 May 2021 14:45:11 -0700 Subject: [PATCH 14/29] Init config on load | Add wrapper for cli --- authority/config.go | 2 ++ ca/identity/identity.go | 6 ++++++ 2 files changed, 8 insertions(+) diff --git a/authority/config.go b/authority/config.go index 86a5c80c..b02bc2be 100644 --- a/authority/config.go +++ b/authority/config.go @@ -146,6 +146,8 @@ func LoadConfiguration(filename string) (*Config, error) { return nil, errors.Wrapf(err, "error parsing %s", filename) } + c.init() + return &c, nil } diff --git a/ca/identity/identity.go b/ca/identity/identity.go index 08a70c7f..0f022dd7 100644 --- a/ca/identity/identity.go +++ b/ca/identity/identity.go @@ -134,6 +134,12 @@ func WriteDefaultIdentity(certChain []api.Certificate, key crypto.PrivateKey) er return nil } +// WriteIdentityCertificate writes the identity certificate to disk. +func WriteIdentityCertificate(certChain []api.Certificate) error { + filename := filepath.Join(identityDir, "identity.crt") + return writeCertificate(filename, certChain) +} + // writeCertificate writes the given certificate on disk. func writeCertificate(filename string, certChain []api.Certificate) error { buf := new(bytes.Buffer) From bb1e051b278650cd7f7cfe00df91070e74aca21c Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Wed, 5 May 2021 08:12:17 +0900 Subject: [PATCH 15/29] 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") } From 09a21fef26630f9cddd9f78500ef164bc0d7983d Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Thu, 29 Apr 2021 15:44:21 +0900 Subject: [PATCH 16/29] 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 79eec83f3e414d7cbf4abb30f9a4355c8392c9e7 Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Fri, 30 Apr 2021 08:44:41 +0900 Subject: [PATCH 17/29] 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 48666792c776403881ad576536bef93440efb08c Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Fri, 30 Apr 2021 09:14:28 +0900 Subject: [PATCH 18/29] 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 f730c0bec4b13700362d0c2728a5066ab7450945 Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Fri, 30 Apr 2021 09:41:06 +0900 Subject: [PATCH 19/29] 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 aafac179a576ee669af10313edcdb906827b7724 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 29 Apr 2021 18:30:26 -0700 Subject: [PATCH 20/29] 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 46c1dc80fb098cff964138606706a1b2387eab1d Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 29 Apr 2021 18:40:04 -0700 Subject: [PATCH 21/29] 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 08e5ec6ad1007ccd8a44eb3f479a9fbaaddf18b8 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 29 Apr 2021 18:47:17 -0700 Subject: [PATCH 22/29] 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 21732f213b4033e27cbdb752c93a8e15e58f1e3c Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Tue, 4 May 2021 13:47:17 +0900 Subject: [PATCH 23/29] 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 decf0fc8ceeb8466d3e61040fffcc1b5aed73e26 Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Wed, 5 May 2021 08:12:17 +0900 Subject: [PATCH 24/29] 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") } From 9e00b82bdf6c65f29e4ebba62f4eb0dd5398bc19 Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Wed, 5 May 2021 08:49:03 +0900 Subject: [PATCH 25/29] Revert `oidc_test.go` Moving the `preferred_username` to a separate PR --- authority/provisioner/oidc_test.go | 5 ----- authority/provisioner/provisioner.go | 2 +- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/authority/provisioner/oidc_test.go b/authority/provisioner/oidc_test.go index 85c6b1a9..9c4b3f4c 100644 --- a/authority/provisioner/oidc_test.go +++ b/authority/provisioner/oidc_test.go @@ -514,8 +514,6 @@ 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 := generateOIDCToken("subject", "the-issuer", p3.ClientID, "root@example.com", "", time.Now(), &keys.Keys[0]) assert.FatalError(t, err) @@ -576,9 +574,6 @@ 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}, diff --git a/authority/provisioner/provisioner.go b/authority/provisioner/provisioner.go index 6ffab03d..c05d68ab 100644 --- a/authority/provisioner/provisioner.go +++ b/authority/provisioner/provisioner.go @@ -346,7 +346,7 @@ func DefaultIdentityFunc(ctx context.Context, p Interface, email string, usernam switch k := p.(type) { case *OIDC: // OIDC principals would be: - // 1. Preferred usernames. + // ~~1. Preferred usernames.~~ Note: Under discussion, currently disabled // 2. Sanitized local. // 3. Raw local (if different). // 4. Email address. From 1d2445e1d89a9c8fb47076c761051498861731be Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Wed, 5 May 2021 10:12:38 +0900 Subject: [PATCH 26/29] Removed the variadic username Could be useful later on, but for the current PR changes should be minimized --- authority/options.go | 2 +- authority/provisioner/oidc.go | 1 - authority/provisioner/oidc_test.go | 4 ++-- authority/provisioner/provisioner.go | 6 +++--- 4 files changed, 6 insertions(+), 7 deletions(-) diff --git a/authority/options.go b/authority/options.go index 9626f48e..9594f989 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, usernames ...string) (*provisioner.Identity, error)) Option { +func WithGetIdentityFunc(fn func(ctx context.Context, p provisioner.Interface, email 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 33988a0a..be6d18c6 100644 --- a/authority/provisioner/oidc.go +++ b/authority/provisioner/oidc.go @@ -44,7 +44,6 @@ type openIDPayload struct { 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"` diff --git a/authority/provisioner/oidc_test.go b/authority/provisioner/oidc_test.go index 9c4b3f4c..48f879a8 100644 --- a/authority/provisioner/oidc_test.go +++ b/authority/provisioner/oidc_test.go @@ -500,10 +500,10 @@ 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, usernames ...string) (*Identity, error) { + p4.getIdentityFunc = func(ctx context.Context, p Interface, email string) (*Identity, error) { return &Identity{Usernames: []string{"max", "mariano"}}, nil } - p5.getIdentityFunc = func(ctx context.Context, p Interface, email string, usernames ...string) (*Identity, error) { + p5.getIdentityFunc = func(ctx context.Context, p Interface, email string) (*Identity, error) { return nil, errors.New("force") } // Additional test needed for empty usernames and duplicate email and usernames diff --git a/authority/provisioner/provisioner.go b/authority/provisioner/provisioner.go index c05d68ab..197bd26c 100644 --- a/authority/provisioner/provisioner.go +++ b/authority/provisioner/provisioner.go @@ -337,12 +337,12 @@ 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) +type GetIdentityFunc func(ctx context.Context, p Interface, email string) (*Identity, error) // 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) { +func DefaultIdentityFunc(ctx context.Context, p Interface, email string) (*Identity, error) { switch k := p.(type) { case *OIDC: // OIDC principals would be: @@ -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 := []string{name} if i := strings.LastIndex(email, "@"); i >= 0 { usernames = append(usernames, email[:i]) } From f38a72a62b08deceee4462ac4af27523b28bfb3a Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Wed, 5 May 2021 10:17:08 +0900 Subject: [PATCH 27/29] Leftover from previous commit --- authority/provisioner/provisioner_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/authority/provisioner/provisioner_test.go b/authority/provisioner/provisioner_test.go index 267fb7d1..bea2ab11 100644 --- a/authority/provisioner/provisioner_test.go +++ b/authority/provisioner/provisioner_test.go @@ -145,7 +145,7 @@ func TestDefaultIdentityFunc(t *testing.T) { for name, get := range tests { t.Run(name, func(t *testing.T) { tc := get(t) - identity, err := DefaultIdentityFunc(context.Background(), tc.p, tc.email, tc.usernames...) + identity, err := DefaultIdentityFunc(context.Background(), tc.p, tc.email) if err != nil { if assert.NotNil(t, tc.err) { assert.Equals(t, tc.err.Error(), err.Error()) From c2d30f726075265a0dd65f1f957b4dbbbf9dbec8 Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Wed, 5 May 2021 10:29:47 +0900 Subject: [PATCH 28/29] gofmt everything --- authority/provisioner/oidc.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/authority/provisioner/oidc.go b/authority/provisioner/oidc.go index be6d18c6..46e1c623 100644 --- a/authority/provisioner/oidc.go +++ b/authority/provisioner/oidc.go @@ -40,13 +40,13 @@ 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"` - 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"` + Hd string `json:"hd"` + Nonce string `json:"nonce"` + Groups []string `json:"groups"` } // OIDC represents an OAuth 2.0 OpenID Connect provider. From d7eec869c2d7c7cc614c932434335c077548fc3f Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Wed, 5 May 2021 10:37:30 +0900 Subject: [PATCH 29/29] Fix the previous tests --- authority/provisioner/provisioner_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/authority/provisioner/provisioner_test.go b/authority/provisioner/provisioner_test.go index bea2ab11..3d895277 100644 --- a/authority/provisioner/provisioner_test.go +++ b/authority/provisioner/provisioner_test.go @@ -122,7 +122,7 @@ func TestDefaultIdentityFunc(t *testing.T) { p: &OIDC{}, email: "john@smallstep.com", usernames: []string{"johnny"}, - identity: &Identity{Usernames: []string{"johnny", "john", "john@smallstep.com"}}, + identity: &Identity{Usernames: []string{"john", "john@smallstep.com"}}, } }, "ok usernames": func(t *testing.T) test { @@ -130,7 +130,7 @@ func TestDefaultIdentityFunc(t *testing.T) { p: &OIDC{}, email: "john@smallstep.com", usernames: []string{"johnny", "js", "", "johnny", ""}, - identity: &Identity{Usernames: []string{"johnny", "js", "john", "john@smallstep.com"}}, + identity: &Identity{Usernames: []string{"john", "john@smallstep.com"}}, } }, "ok empty username": func(t *testing.T) test {