From df8ffb35afef1dfc4cecedfa77ea03ee7ef190fb Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 5 Apr 2022 17:39:06 -0700 Subject: [PATCH 01/26] Remove unnecessary database in provisioner config. --- authority/provisioner/provisioner.go | 3 --- authority/provisioners.go | 1 - 2 files changed, 4 deletions(-) diff --git a/authority/provisioner/provisioner.go b/authority/provisioner/provisioner.go index 7438ea17..0d5cd41a 100644 --- a/authority/provisioner/provisioner.go +++ b/authority/provisioner/provisioner.go @@ -9,7 +9,6 @@ import ( "strings" "github.com/pkg/errors" - "github.com/smallstep/certificates/db" "github.com/smallstep/certificates/errs" "golang.org/x/crypto/ssh" ) @@ -212,8 +211,6 @@ type Config struct { Claims Claims // Audiences are the audiences used in the default provisioner, (JWK). Audiences Audiences - // DB is the interface to the authority DB client. - DB db.AuthDB // SSHKeys are the root SSH public keys SSHKeys *SSHKeys // GetIdentityFunc is a function that returns an identity that will be diff --git a/authority/provisioners.go b/authority/provisioners.go index a6ac5aa8..496421f5 100644 --- a/authority/provisioners.go +++ b/authority/provisioners.go @@ -103,7 +103,6 @@ func (a *Authority) generateProvisionerConfig(ctx context.Context) (provisioner. return provisioner.Config{ Claims: claimer.Claims(), Audiences: a.config.GetAudiences(), - DB: a.db, SSHKeys: &provisioner.SSHKeys{ UserKeys: sshKeys.UserKeys, HostKeys: sshKeys.HostKeys, From 41c6ded85e537c1f1a31080f3cbb8e1a9e364cdc Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 5 Apr 2022 18:00:01 -0700 Subject: [PATCH 02/26] Store in the db the provisioner that granted a cert. --- db/db.go | 50 +++++++++++++++++++++++------ db/db_test.go | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 129 insertions(+), 9 deletions(-) diff --git a/db/db.go b/db/db.go index 6d48723f..3427d2bb 100644 --- a/db/db.go +++ b/db/db.go @@ -8,20 +8,22 @@ import ( "time" "github.com/pkg/errors" + "github.com/smallstep/certificates/authority/provisioner" "github.com/smallstep/nosql" "github.com/smallstep/nosql/database" "golang.org/x/crypto/ssh" ) var ( - certsTable = []byte("x509_certs") - revokedCertsTable = []byte("revoked_x509_certs") - revokedSSHCertsTable = []byte("revoked_ssh_certs") - usedOTTTable = []byte("used_ott") - sshCertsTable = []byte("ssh_certs") - sshHostsTable = []byte("ssh_hosts") - sshUsersTable = []byte("ssh_users") - sshHostPrincipalsTable = []byte("ssh_host_principals") + certsTable = []byte("x509_certs") + certsToProvisionerTable = []byte("x509_certs_provisioner") + revokedCertsTable = []byte("revoked_x509_certs") + revokedSSHCertsTable = []byte("revoked_ssh_certs") + usedOTTTable = []byte("used_ott") + sshCertsTable = []byte("ssh_certs") + sshHostsTable = []byte("ssh_hosts") + sshUsersTable = []byte("ssh_users") + sshHostPrincipalsTable = []byte("ssh_host_principals") ) // ErrAlreadyExists can be returned if the DB attempts to set a key that has @@ -82,7 +84,7 @@ func New(c *Config) (AuthDB, error) { tables := [][]byte{ revokedCertsTable, certsTable, usedOTTTable, sshCertsTable, sshHostsTable, sshHostPrincipalsTable, sshUsersTable, - revokedSSHCertsTable, + revokedSSHCertsTable, certsToProvisionerTable, } for _, b := range tables { if err := db.CreateTable(b); err != nil { @@ -210,6 +212,36 @@ func (db *DB) StoreCertificate(crt *x509.Certificate) error { return nil } +type certsToProvionersData struct { + ID string `json:"id"` + Name string `json:"name"` + Type string `json:"type"` +} + +// StoreCertificateChain stores the leaf certificate and the provisioner that +// authorized the certificate. +func (d *DB) StoreCertificateChain(p provisioner.Interface, chain ...*x509.Certificate) error { + leaf := chain[0] + if err := d.StoreCertificate(leaf); err != nil { + return err + } + if p != nil { + b, err := json.Marshal(certsToProvionersData{ + ID: p.GetID(), + Name: p.GetName(), + Type: p.GetType().String(), + }) + if err != nil { + return errors.Wrap(err, "error marshaling json") + } + + if err := d.Set(certsToProvisionerTable, []byte(leaf.SerialNumber.String()), b); err != nil { + return errors.Wrap(err, "database Set error") + } + } + return nil +} + // UseToken returns true if we were able to successfully store the token for // for the first time, false otherwise. func (db *DB) UseToken(id, tok string) (bool, error) { diff --git a/db/db_test.go b/db/db_test.go index 40f59215..5a7e2d38 100644 --- a/db/db_test.go +++ b/db/db_test.go @@ -1,10 +1,14 @@ package db import ( + "crypto/x509" "errors" + "math/big" "testing" "github.com/smallstep/assert" + "github.com/smallstep/certificates/authority/provisioner" + "github.com/smallstep/nosql" "github.com/smallstep/nosql/database" ) @@ -158,3 +162,87 @@ func TestUseToken(t *testing.T) { }) } } + +func TestDB_StoreCertificateChain(t *testing.T) { + p := &provisioner.JWK{ + ID: "some-id", + Name: "admin", + Type: "JWK", + } + chain := []*x509.Certificate{ + {Raw: []byte("the certificate"), SerialNumber: big.NewInt(1234)}, + } + type fields struct { + DB nosql.DB + isUp bool + } + type args struct { + p provisioner.Interface + chain []*x509.Certificate + } + tests := []struct { + name string + fields fields + args args + wantErr bool + }{ + {"ok", fields{&MockNoSQLDB{ + MSet: func(bucket, key, value []byte) error { + switch string(bucket) { + case "x509_certs": + assert.Equals(t, key, []byte("1234")) + assert.Equals(t, value, []byte("the certificate")) + case "x509_certs_provisioner": + assert.Equals(t, key, []byte("1234")) + assert.Equals(t, value, []byte(`{"id":"some-id","name":"admin","type":"JWK"}`)) + default: + t.Errorf("unexpected bucket %s", bucket) + } + return nil + }, + }, true}, args{p, chain}, false}, + {"ok no provisioner", fields{&MockNoSQLDB{ + MSet: func(bucket, key, value []byte) error { + switch string(bucket) { + case "x509_certs": + assert.Equals(t, key, []byte("1234")) + assert.Equals(t, value, []byte("the certificate")) + default: + t.Errorf("unexpected bucket %s", bucket) + } + return nil + }, + }, true}, args{nil, chain}, false}, + {"fail store certificate", fields{&MockNoSQLDB{ + MSet: func(bucket, key, value []byte) error { + switch string(bucket) { + case "x509_certs": + return errors.New("test error") + default: + return nil + } + }, + }, true}, args{p, chain}, true}, + {"fail store provisioner", fields{&MockNoSQLDB{ + MSet: func(bucket, key, value []byte) error { + switch string(bucket) { + case "x509_certs_provisioner": + return errors.New("test error") + default: + return nil + } + }, + }, true}, args{p, chain}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + d := &DB{ + DB: tt.fields.DB, + isUp: tt.fields.isUp, + } + if err := d.StoreCertificateChain(tt.args.p, tt.args.chain...); (err != nil) != tt.wantErr { + t.Errorf("DB.StoreCertificateChain() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} From 7d6116c3d052bef1f2e723e67e177280b621f3c4 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 5 Apr 2022 19:24:53 -0700 Subject: [PATCH 03/26] Add GetCertificateData and refactor x509_certs_data. --- db/db.go | 63 +++++++++++++++++++++++++++++++--------------- db/db_test.go | 70 ++++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 110 insertions(+), 23 deletions(-) diff --git a/db/db.go b/db/db.go index 3427d2bb..a3ebb19f 100644 --- a/db/db.go +++ b/db/db.go @@ -15,15 +15,15 @@ import ( ) var ( - certsTable = []byte("x509_certs") - certsToProvisionerTable = []byte("x509_certs_provisioner") - revokedCertsTable = []byte("revoked_x509_certs") - revokedSSHCertsTable = []byte("revoked_ssh_certs") - usedOTTTable = []byte("used_ott") - sshCertsTable = []byte("ssh_certs") - sshHostsTable = []byte("ssh_hosts") - sshUsersTable = []byte("ssh_users") - sshHostPrincipalsTable = []byte("ssh_host_principals") + certsTable = []byte("x509_certs") + certsDataTable = []byte("x509_certs_data") + revokedCertsTable = []byte("revoked_x509_certs") + revokedSSHCertsTable = []byte("revoked_ssh_certs") + usedOTTTable = []byte("used_ott") + sshCertsTable = []byte("ssh_certs") + sshHostsTable = []byte("ssh_hosts") + sshUsersTable = []byte("ssh_users") + sshHostPrincipalsTable = []byte("ssh_host_principals") ) // ErrAlreadyExists can be returned if the DB attempts to set a key that has @@ -84,7 +84,7 @@ func New(c *Config) (AuthDB, error) { tables := [][]byte{ revokedCertsTable, certsTable, usedOTTTable, sshCertsTable, sshHostsTable, sshHostPrincipalsTable, sshUsersTable, - revokedSSHCertsTable, certsToProvisionerTable, + revokedSSHCertsTable, certsDataTable, } for _, b := range tables { if err := db.CreateTable(b); err != nil { @@ -204,6 +204,19 @@ func (db *DB) GetCertificate(serialNumber string) (*x509.Certificate, error) { return cert, nil } +// GetCertificateData returns the data stored for a provisioner +func (db *DB) GetCertificateData(serialNumber string) (*CertificateData, error) { + b, err := db.Get(certsDataTable, []byte(serialNumber)) + if err != nil { + return nil, errors.Wrap(err, "database Get error") + } + var data CertificateData + if err := json.Unmarshal(b, &data); err != nil { + return nil, errors.Wrap(err, "error unmarshaling json") + } + return &data, nil +} + // StoreCertificate stores a certificate PEM. func (db *DB) StoreCertificate(crt *x509.Certificate) error { if err := db.Set(certsTable, []byte(crt.SerialNumber.String()), crt.Raw); err != nil { @@ -212,7 +225,15 @@ func (db *DB) StoreCertificate(crt *x509.Certificate) error { return nil } -type certsToProvionersData struct { +// CertificateData is the JSON representation of the data stored in +// x509_certs_data table. +type CertificateData struct { + Provisioner *ProvisionerData `json:"provisioner,omitempty"` +} + +// ProvisionerData is the JSON representation of the provisioner stored in the +// x509_certs_data table. +type ProvisionerData struct { ID string `json:"id"` Name string `json:"name"` Type string `json:"type"` @@ -220,24 +241,26 @@ type certsToProvionersData struct { // StoreCertificateChain stores the leaf certificate and the provisioner that // authorized the certificate. -func (d *DB) StoreCertificateChain(p provisioner.Interface, chain ...*x509.Certificate) error { +func (db *DB) StoreCertificateChain(p provisioner.Interface, chain ...*x509.Certificate) error { leaf := chain[0] - if err := d.StoreCertificate(leaf); err != nil { + if err := db.StoreCertificate(leaf); err != nil { return err } + data := &CertificateData{} if p != nil { - b, err := json.Marshal(certsToProvionersData{ + data.Provisioner = &ProvisionerData{ ID: p.GetID(), Name: p.GetName(), Type: p.GetType().String(), - }) - if err != nil { - return errors.Wrap(err, "error marshaling json") } + } - if err := d.Set(certsToProvisionerTable, []byte(leaf.SerialNumber.String()), b); err != nil { - return errors.Wrap(err, "database Set error") - } + b, err := json.Marshal(data) + if err != nil { + return errors.Wrap(err, "error marshaling json") + } + if err := db.Set(certsDataTable, []byte(leaf.SerialNumber.String()), b); err != nil { + return errors.Wrap(err, "database Set error") } return nil } diff --git a/db/db_test.go b/db/db_test.go index 5a7e2d38..d7c58c9c 100644 --- a/db/db_test.go +++ b/db/db_test.go @@ -4,6 +4,7 @@ import ( "crypto/x509" "errors" "math/big" + "reflect" "testing" "github.com/smallstep/assert" @@ -192,9 +193,9 @@ func TestDB_StoreCertificateChain(t *testing.T) { case "x509_certs": assert.Equals(t, key, []byte("1234")) assert.Equals(t, value, []byte("the certificate")) - case "x509_certs_provisioner": + case "x509_certs_data": assert.Equals(t, key, []byte("1234")) - assert.Equals(t, value, []byte(`{"id":"some-id","name":"admin","type":"JWK"}`)) + assert.Equals(t, value, []byte(`{"provisioner":{"id":"some-id","name":"admin","type":"JWK"}}`)) default: t.Errorf("unexpected bucket %s", bucket) } @@ -207,6 +208,9 @@ func TestDB_StoreCertificateChain(t *testing.T) { case "x509_certs": assert.Equals(t, key, []byte("1234")) assert.Equals(t, value, []byte("the certificate")) + case "x509_certs_data": + assert.Equals(t, key, []byte("1234")) + assert.Equals(t, value, []byte(`{}`)) default: t.Errorf("unexpected bucket %s", bucket) } @@ -226,7 +230,7 @@ func TestDB_StoreCertificateChain(t *testing.T) { {"fail store provisioner", fields{&MockNoSQLDB{ MSet: func(bucket, key, value []byte) error { switch string(bucket) { - case "x509_certs_provisioner": + case "x509_certs_data": return errors.New("test error") default: return nil @@ -246,3 +250,63 @@ func TestDB_StoreCertificateChain(t *testing.T) { }) } } + +func TestDB_GetCertificateData(t *testing.T) { + type fields struct { + DB nosql.DB + isUp bool + } + type args struct { + serialNumber string + } + tests := []struct { + name string + fields fields + args args + want *CertificateData + wantErr bool + }{ + {"ok", fields{&MockNoSQLDB{ + MGet: func(bucket, key []byte) ([]byte, error) { + assert.Equals(t, bucket, []byte("x509_certs_data")) + assert.Equals(t, key, []byte("1234")) + return []byte(`{"provisioner":{"id":"some-id","name":"admin","type":"JWK"}}`), nil + }, + }, true}, args{"1234"}, &CertificateData{ + Provisioner: &ProvisionerData{ + ID: "some-id", Name: "admin", Type: "JWK", + }, + }, false}, + {"fail not found", fields{&MockNoSQLDB{ + MGet: func(bucket, key []byte) ([]byte, error) { + return nil, database.ErrNotFound + }, + }, true}, args{"1234"}, nil, true}, + {"fail db", fields{&MockNoSQLDB{ + MGet: func(bucket, key []byte) ([]byte, error) { + return nil, errors.New("an error") + }, + }, true}, args{"1234"}, nil, true}, + {"fail unmarshal", fields{&MockNoSQLDB{ + MGet: func(bucket, key []byte) ([]byte, error) { + return []byte(`{"bad-json"}`), nil + }, + }, true}, args{"1234"}, nil, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + db := &DB{ + DB: tt.fields.DB, + isUp: tt.fields.isUp, + } + got, err := db.GetCertificateData(tt.args.serialNumber) + if (err != nil) != tt.wantErr { + t.Errorf("DB.GetCertificateData() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("DB.GetCertificateData() = %v, want %v", got, tt.want) + } + }) + } +} From db337debcdc6ca59cde20900d114f0f48bc12a33 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 5 Apr 2022 19:25:47 -0700 Subject: [PATCH 04/26] Load provisioner from the database instead of the extension. --- authority/provisioners.go | 34 +++++++++++++++++++++++++++++----- authority/tls.go | 2 ++ 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/authority/provisioners.go b/authority/provisioners.go index 496421f5..c0eec520 100644 --- a/authority/provisioners.go +++ b/authority/provisioners.go @@ -13,6 +13,7 @@ import ( "github.com/smallstep/certificates/authority/admin" "github.com/smallstep/certificates/authority/config" "github.com/smallstep/certificates/authority/provisioner" + "github.com/smallstep/certificates/db" "github.com/smallstep/certificates/errs" "go.step.sm/cli-utils/step" "go.step.sm/cli-utils/ui" @@ -44,13 +45,36 @@ func (a *Authority) GetProvisioners(cursor string, limit int) (provisioner.List, // LoadProvisionerByCertificate returns an interface to the provisioner that // provisioned the certificate. func (a *Authority) LoadProvisionerByCertificate(crt *x509.Certificate) (provisioner.Interface, error) { + // Default implementation looks at the provisioner extension. + loadProvisioner := func() (provisioner.Interface, error) { + p, ok := a.provisioners.LoadByCertificate(crt) + if !ok { + return nil, admin.NewError(admin.ErrorNotFoundType, "unable to load provisioner from certificate") + } + return p, nil + } + + // Attempt to load the provisioner using the linked db + // TODO:(mariano) + + // Attempt to load the provisioner from the db + if db, ok := a.db.(interface { + GetCertificateData(string) (*db.CertificateData, error) + }); ok { + if data, err := db.GetCertificateData(crt.SerialNumber.String()); err == nil && data.Provisioner != nil { + loadProvisioner = func() (provisioner.Interface, error) { + p, ok := a.provisioners.Load(data.Provisioner.ID) + if !ok { + return nil, admin.NewError(admin.ErrorNotFoundType, "unable to load provisioner from certificate") + } + return p, nil + } + } + } + a.adminMutex.RLock() defer a.adminMutex.RUnlock() - p, ok := a.provisioners.LoadByCertificate(crt) - if !ok { - return nil, admin.NewError(admin.ErrorNotFoundType, "unable to load provisioner from certificate") - } - return p, nil + return loadProvisioner() } // LoadProvisionerByToken returns an interface to the provisioner that diff --git a/authority/tls.go b/authority/tls.go index 93bc0408..bebcdf1b 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -347,6 +347,8 @@ func (a *Authority) storeCertificate(prov provisioner.Interface, fullchain []*x5 // Store certificate in local db switch s := a.db.(type) { + case linkedChainStorer: + return s.StoreCertificateChain(prov, fullchain...) case certificateChainStorer: return s.StoreCertificateChain(fullchain...) default: From c55b27a2fc4420e4b6647ee264b31b5fc38cd675 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 7 Apr 2022 18:14:43 -0700 Subject: [PATCH 05/26] Refactor admin token to use with RAs. --- authority/authorize.go | 13 +++++------- authority/config/config.go | 12 +++++++++++ authority/config/config_test.go | 36 ++++++++++++++++++++++++++++++++ authority/linkedca.go | 22 ++++++++++++++++++++ authority/provisioners.go | 19 ++++++++++------- ca/adminClient.go | 37 +++++++++++++++++---------------- ca/client.go | 21 +++++++------------ go.mod | 2 +- go.sum | 2 -- 9 files changed, 114 insertions(+), 50 deletions(-) diff --git a/authority/authorize.go b/authority/authorize.go index 7c1c2ff6..0c1c6f22 100644 --- a/authority/authorize.go +++ b/authority/authorize.go @@ -130,22 +130,19 @@ func (a *Authority) AuthorizeAdminToken(r *http.Request, token string) (*linkedc // According to "rfc7519 JSON Web Token" acceptable skew should be no // more than a few minutes. if err := claims.ValidateWithLeeway(jose.Expected{ - Issuer: prov.GetName(), + Issuer: "step-admin-client/1.0", Time: time.Now().UTC(), }, time.Minute); err != nil { return nil, admin.WrapError(admin.ErrorUnauthorizedType, err, "x5c.authorizeToken; invalid x5c claims") } // validate audience: path matches the current path - if r.URL.Path != claims.Audience[0] { - return nil, admin.NewError(admin.ErrorUnauthorizedType, - "x5c.authorizeToken; x5c token has invalid audience "+ - "claim (aud); expected %s, but got %s", r.URL.Path, claims.Audience) + if !matchesAudience(claims.Audience, a.config.Audience(r.URL.Path)) { + return nil, admin.NewError(admin.ErrorUnauthorizedType, "x5c.authorizeToken; x5c token has invalid audience claim (aud)") } if claims.Subject == "" { - return nil, admin.NewError(admin.ErrorUnauthorizedType, - "x5c.authorizeToken; x5c token subject cannot be empty") + return nil, admin.NewError(admin.ErrorUnauthorizedType, "x5c.authorizeToken; x5c token subject cannot be empty") } var ( @@ -156,7 +153,7 @@ func (a *Authority) AuthorizeAdminToken(r *http.Request, token string) (*linkedc adminSANs := append([]string{leaf.Subject.CommonName}, leaf.DNSNames...) adminSANs = append(adminSANs, leaf.EmailAddresses...) for _, san := range adminSANs { - if adm, ok = a.LoadAdminBySubProv(san, claims.Issuer); ok { + if adm, ok = a.LoadAdminBySubProv(san, prov.GetName()); ok { adminFound = true break } diff --git a/authority/config/config.go b/authority/config/config.go index 2c437725..4ed7a1cb 100644 --- a/authority/config/config.go +++ b/authority/config/config.go @@ -304,6 +304,18 @@ func (c *Config) GetAudiences() provisioner.Audiences { return audiences } +// Audience returns the list of audiences for a given path. +func (c *Config) Audience(path string) []string { + audiences := make([]string, len(c.DNSNames)+1) + for i, name := range c.DNSNames { + hostname := toHostname(name) + audiences[i] = "https://" + hostname + path + } + // For backward compatibility + audiences[len(c.DNSNames)] = path + return audiences +} + func toHostname(name string) string { // ensure an IPv6 address is represented with square brackets when used as hostname if ip := net.ParseIP(name); ip != nil && ip.To4() == nil { diff --git a/authority/config/config_test.go b/authority/config/config_test.go index b921be13..5a05b3f6 100644 --- a/authority/config/config_test.go +++ b/authority/config/config_test.go @@ -2,6 +2,7 @@ package config import ( "fmt" + "reflect" "testing" "github.com/pkg/errors" @@ -317,3 +318,38 @@ func Test_toHostname(t *testing.T) { }) } } + +func TestConfig_Audience(t *testing.T) { + type fields struct { + DNSNames []string + } + type args struct { + path string + } + tests := []struct { + name string + fields fields + args args + want []string + }{ + {"ok", fields{[]string{ + "ca", "ca.example.com", "127.0.0.1", "::1", + }}, args{"/path"}, []string{ + "https://ca/path", + "https://ca.example.com/path", + "https://127.0.0.1/path", + "https://[::1]/path", + "/path", + }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := &Config{ + DNSNames: tt.fields.DNSNames, + } + if got := c.Audience(tt.args.path); !reflect.DeepEqual(got, tt.want) { + t.Errorf("Config.Audience() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/authority/linkedca.go b/authority/linkedca.go index 6a0800c2..1b920ce8 100644 --- a/authority/linkedca.go +++ b/authority/linkedca.go @@ -235,6 +235,28 @@ func (c *linkedCaClient) DeleteAdmin(ctx context.Context, id string) error { return errors.Wrap(err, "error deleting admin") } +func (c *linkedCaClient) GetCertificateData(serial string) (*db.CertificateData, error) { + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + + resp, err := c.client.GetCertificate(ctx, &linkedca.GetCertificateRequest{ + Serial: serial, + }) + if err != nil { + return nil, err + } + + var provisioner *db.ProvisionerData + if p := resp.Provisioner; p != nil { + provisioner = &db.ProvisionerData{ + ID: p.Id, Name: p.Name, Type: p.Type.String(), + } + } + return &db.CertificateData{ + Provisioner: provisioner, + }, nil +} + func (c *linkedCaClient) StoreCertificateChain(prov provisioner.Interface, fullchain ...*x509.Certificate) error { ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) defer cancel() diff --git a/authority/provisioners.go b/authority/provisioners.go index c0eec520..3713705f 100644 --- a/authority/provisioners.go +++ b/authority/provisioners.go @@ -54,14 +54,19 @@ func (a *Authority) LoadProvisionerByCertificate(crt *x509.Certificate) (provisi return p, nil } - // Attempt to load the provisioner using the linked db - // TODO:(mariano) - - // Attempt to load the provisioner from the db - if db, ok := a.db.(interface { + // certificateDataGetter is an interface that can be use to retrieve the + // provisioner from a db or a linked ca. + type certificateDataGetter interface { GetCertificateData(string) (*db.CertificateData, error) - }); ok { - if data, err := db.GetCertificateData(crt.SerialNumber.String()); err == nil && data.Provisioner != nil { + } + var cdg certificateDataGetter + if getter, ok := a.adminDB.(certificateDataGetter); ok { + cdg = getter + } else if getter, ok := a.db.(certificateDataGetter); ok { + cdg = getter + } + if cdg != nil { + if data, err := cdg.GetCertificateData(crt.SerialNumber.String()); err == nil && data.Provisioner != nil { loadProvisioner = func() (provisioner.Interface, error) { p, ok := a.provisioners.Load(data.Provisioner.ID) if !ok { diff --git a/ca/adminClient.go b/ca/adminClient.go index 5f3993b1..c3ba666f 100644 --- a/ca/adminClient.go +++ b/ca/adminClient.go @@ -23,7 +23,10 @@ import ( "google.golang.org/protobuf/encoding/protojson" ) -var adminURLPrefix = "admin" +const ( + adminURLPrefix = "admin" + adminIssuer = "step-admin-client/1.0" +) // AdminClient implements an HTTP client for the CA server. type AdminClient struct { @@ -35,7 +38,6 @@ type AdminClient struct { x5cCertFile string x5cCertStrs []string x5cCert *x509.Certificate - x5cIssuer string x5cSubject string } @@ -77,12 +79,11 @@ func NewAdminClient(endpoint string, opts ...ClientOption) (*AdminClient, error) x5cCertFile: o.x5cCertFile, x5cCertStrs: o.x5cCertStrs, x5cCert: o.x5cCert, - x5cIssuer: o.x5cIssuer, x5cSubject: o.x5cSubject, }, nil } -func (c *AdminClient) generateAdminToken(urlPath string) (string, error) { +func (c *AdminClient) generateAdminToken(aud *url.URL) (string, error) { // A random jwt id will be used to identify duplicated tokens jwtID, err := randutil.Hex(64) // 256 bits if err != nil { @@ -93,8 +94,8 @@ func (c *AdminClient) generateAdminToken(urlPath string) (string, error) { tokOptions := []token.Options{ token.WithJWTID(jwtID), token.WithKid(c.x5cJWK.KeyID), - token.WithIssuer(c.x5cIssuer), - token.WithAudience(urlPath), + token.WithIssuer(adminIssuer), + token.WithAudience(aud.String()), token.WithValidity(now, now.Add(token.DefaultValidity)), token.WithX5CCerts(c.x5cCertStrs), } @@ -205,7 +206,7 @@ func (c *AdminClient) GetAdminsPaginate(opts ...AdminOption) (*adminAPI.GetAdmin Path: "/admin/admins", RawQuery: o.rawQuery(), }) - tok, err := c.generateAdminToken(u.Path) + tok, err := c.generateAdminToken(u) if err != nil { return nil, errors.Wrapf(err, "error generating admin token") } @@ -260,7 +261,7 @@ func (c *AdminClient) CreateAdmin(createAdminRequest *adminAPI.CreateAdminReques return nil, errs.Wrap(http.StatusInternalServerError, err, "error marshaling request") } u := c.endpoint.ResolveReference(&url.URL{Path: "/admin/admins"}) - tok, err := c.generateAdminToken(u.Path) + tok, err := c.generateAdminToken(u) if err != nil { return nil, errors.Wrapf(err, "error generating admin token") } @@ -292,7 +293,7 @@ retry: func (c *AdminClient) RemoveAdmin(id string) error { var retried bool u := c.endpoint.ResolveReference(&url.URL{Path: path.Join(adminURLPrefix, "admins", id)}) - tok, err := c.generateAdminToken(u.Path) + tok, err := c.generateAdminToken(u) if err != nil { return errors.Wrapf(err, "error generating admin token") } @@ -324,7 +325,7 @@ func (c *AdminClient) UpdateAdmin(id string, uar *adminAPI.UpdateAdminRequest) ( return nil, errs.Wrap(http.StatusInternalServerError, err, "error marshaling request") } u := c.endpoint.ResolveReference(&url.URL{Path: path.Join(adminURLPrefix, "admins", id)}) - tok, err := c.generateAdminToken(u.Path) + tok, err := c.generateAdminToken(u) if err != nil { return nil, errors.Wrapf(err, "error generating admin token") } @@ -371,7 +372,7 @@ func (c *AdminClient) GetProvisioner(opts ...ProvisionerOption) (*linkedca.Provi default: return nil, errors.New("must set either name or id in method options") } - tok, err := c.generateAdminToken(u.Path) + tok, err := c.generateAdminToken(u) if err != nil { return nil, errors.Wrapf(err, "error generating admin token") } @@ -410,7 +411,7 @@ func (c *AdminClient) GetProvisionersPaginate(opts ...ProvisionerOption) (*admin Path: "/admin/provisioners", RawQuery: o.rawQuery(), }) - tok, err := c.generateAdminToken(u.Path) + tok, err := c.generateAdminToken(u) if err != nil { return nil, errors.Wrapf(err, "error generating admin token") } @@ -480,7 +481,7 @@ func (c *AdminClient) RemoveProvisioner(opts ...ProvisionerOption) error { default: return errors.New("must set either name or id in method options") } - tok, err := c.generateAdminToken(u.Path) + tok, err := c.generateAdminToken(u) if err != nil { return errors.Wrapf(err, "error generating admin token") } @@ -512,7 +513,7 @@ func (c *AdminClient) CreateProvisioner(prov *linkedca.Provisioner) (*linkedca.P return nil, errs.Wrap(http.StatusInternalServerError, err, "error marshaling request") } u := c.endpoint.ResolveReference(&url.URL{Path: path.Join(adminURLPrefix, "provisioners")}) - tok, err := c.generateAdminToken(u.Path) + tok, err := c.generateAdminToken(u) if err != nil { return nil, errors.Wrapf(err, "error generating admin token") } @@ -548,7 +549,7 @@ func (c *AdminClient) UpdateProvisioner(name string, prov *linkedca.Provisioner) return errs.Wrap(http.StatusInternalServerError, err, "error marshaling request") } u := c.endpoint.ResolveReference(&url.URL{Path: path.Join(adminURLPrefix, "provisioners", name)}) - tok, err := c.generateAdminToken(u.Path) + tok, err := c.generateAdminToken(u) if err != nil { return errors.Wrapf(err, "error generating admin token") } @@ -587,7 +588,7 @@ func (c *AdminClient) GetExternalAccountKeysPaginate(provisionerName, reference Path: p, RawQuery: o.rawQuery(), }) - tok, err := c.generateAdminToken(u.Path) + tok, err := c.generateAdminToken(u) if err != nil { return nil, errors.Wrapf(err, "error generating admin token") } @@ -623,7 +624,7 @@ func (c *AdminClient) CreateExternalAccountKey(provisionerName string, eakReques return nil, errs.Wrap(http.StatusInternalServerError, err, "error marshaling request") } u := c.endpoint.ResolveReference(&url.URL{Path: path.Join(adminURLPrefix, "acme/eab/", provisionerName)}) - tok, err := c.generateAdminToken(u.Path) + tok, err := c.generateAdminToken(u) if err != nil { return nil, errors.Wrapf(err, "error generating admin token") } @@ -655,7 +656,7 @@ retry: func (c *AdminClient) RemoveExternalAccountKey(provisionerName, keyID string) error { var retried bool u := c.endpoint.ResolveReference(&url.URL{Path: path.Join(adminURLPrefix, "acme/eab", provisionerName, "/", keyID)}) - tok, err := c.generateAdminToken(u.Path) + tok, err := c.generateAdminToken(u) if err != nil { return errors.Wrapf(err, "error generating admin token") } diff --git a/ca/client.go b/ca/client.go index 3a36fcd6..4aa66aac 100644 --- a/ca/client.go +++ b/ca/client.go @@ -116,7 +116,6 @@ type clientOptions struct { x5cCertFile string x5cCertStrs []string x5cCert *x509.Certificate - x5cIssuer string x5cSubject string } @@ -332,19 +331,13 @@ func WithAdminX5C(certs []*x509.Certificate, key interface{}, passwordFile strin } o.x5cCert = certs[0] - o.x5cSubject = o.x5cCert.Subject.CommonName - - for _, e := range o.x5cCert.Extensions { - if e.Id.Equal(stepOIDProvisioner) { - var prov stepProvisionerASN1 - if _, err := asn1.Unmarshal(e.Value, &prov); err != nil { - return errors.Wrap(err, "error unmarshaling provisioner OID from certificate") - } - o.x5cIssuer = string(prov.Name) - } - } - if o.x5cIssuer == "" { - return errors.New("provisioner extension not found in certificate") + switch leaf := certs[0]; { + case leaf.Subject.CommonName != "": + o.x5cSubject = leaf.Subject.CommonName + case len(leaf.DNSNames) > 0: + o.x5cSubject = leaf.DNSNames[0] + case len(leaf.EmailAddresses) > 0: + o.x5cSubject = leaf.EmailAddresses[0] } return nil diff --git a/go.mod b/go.mod index 16007f5b..85d21ceb 100644 --- a/go.mod +++ b/go.mod @@ -50,4 +50,4 @@ require ( // replace github.com/smallstep/nosql => ../nosql // replace go.step.sm/crypto => ../crypto // replace go.step.sm/cli-utils => ../cli-utils -// replace go.step.sm/linkedca => ../linkedca +replace go.step.sm/linkedca => ../linkedca diff --git a/go.sum b/go.sum index 108cfec9..47f84801 100644 --- a/go.sum +++ b/go.sum @@ -709,8 +709,6 @@ go.step.sm/cli-utils v0.7.0/go.mod h1:Ur6bqA/yl636kCUJbp30J7Unv5JJ226eW2KqXPDwF/ go.step.sm/crypto v0.9.0/go.mod h1:+CYG05Mek1YDqi5WK0ERc6cOpKly2i/a5aZmU1sfGj0= go.step.sm/crypto v0.15.3 h1:f3GMl+aCydt294BZRjTYwpaXRqwwndvoTY2NLN4wu10= go.step.sm/crypto v0.15.3/go.mod h1:3G0yQr5lQqfEG0CMYz8apC/qMtjLRQlzflL2AxkcN+g= -go.step.sm/linkedca v0.12.0 h1:FA18uJO5P6W2pklcezMs+w+N3dVbpKEE1LP9HLsJgg4= -go.step.sm/linkedca v0.12.0/go.mod h1:W59ucS4vFpuR0g4PtkGbbtXAwxbDEnNCg+ovkej1ANM= go.uber.org/atomic v1.3.2/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE= go.uber.org/atomic v1.4.0/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE= go.uber.org/atomic v1.5.0/go.mod h1:sABNBOSYdrvTF6hTgEIbc7YasKWGhgEQZyfxyTvoXHQ= From cca5679a111050548a6f98dd794e1dc72aedc63b Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 7 Apr 2022 18:29:38 -0700 Subject: [PATCH 06/26] Use branch dependency for linkedca --- go.mod | 4 ++-- go.sum | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/go.mod b/go.mod index 6a834552..9c0da54d 100644 --- a/go.mod +++ b/go.mod @@ -38,7 +38,7 @@ require ( go.mozilla.org/pkcs7 v0.0.0-20210826202110-33d05740a352 go.step.sm/cli-utils v0.7.0 go.step.sm/crypto v0.16.1 - go.step.sm/linkedca v0.12.0 + go.step.sm/linkedca v0.12.1-0.20220408003202-c7e1ed60beab golang.org/x/crypto v0.0.0-20211215153901-e495a2d5b3d3 golang.org/x/net v0.0.0-20220127200216-cd36cc0744dd google.golang.org/api v0.70.0 @@ -51,4 +51,4 @@ require ( // replace github.com/smallstep/nosql => ../nosql // replace go.step.sm/crypto => ../crypto // replace go.step.sm/cli-utils => ../cli-utils -replace go.step.sm/linkedca => ../linkedca +// replace go.step.sm/linkedca => ../linkedca diff --git a/go.sum b/go.sum index ba718b16..5dbb5735 100644 --- a/go.sum +++ b/go.sum @@ -711,6 +711,8 @@ go.step.sm/cli-utils v0.7.0/go.mod h1:Ur6bqA/yl636kCUJbp30J7Unv5JJ226eW2KqXPDwF/ go.step.sm/crypto v0.9.0/go.mod h1:+CYG05Mek1YDqi5WK0ERc6cOpKly2i/a5aZmU1sfGj0= go.step.sm/crypto v0.16.1 h1:4mnZk21cSxyMGxsEpJwZKKvJvDu1PN09UVrWWFNUBdk= go.step.sm/crypto v0.16.1/go.mod h1:3G0yQr5lQqfEG0CMYz8apC/qMtjLRQlzflL2AxkcN+g= +go.step.sm/linkedca v0.12.1-0.20220408003202-c7e1ed60beab h1:s7IZreqiQONduJQfHcv/SI0rqQty3NKmEuiMESwBZwU= +go.step.sm/linkedca v0.12.1-0.20220408003202-c7e1ed60beab/go.mod h1:W59ucS4vFpuR0g4PtkGbbtXAwxbDEnNCg+ovkej1ANM= go.uber.org/atomic v1.3.2/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE= go.uber.org/atomic v1.4.0/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE= go.uber.org/atomic v1.5.0/go.mod h1:sABNBOSYdrvTF6hTgEIbc7YasKWGhgEQZyfxyTvoXHQ= From 304bb5b97ae5f34b1bacf19db2d4aa1eeeba84f4 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 7 Apr 2022 18:31:41 -0700 Subject: [PATCH 07/26] Remove unused code. --- ca/client.go | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/ca/client.go b/ca/client.go index 4aa66aac..0bd93195 100644 --- a/ca/client.go +++ b/ca/client.go @@ -10,7 +10,6 @@ import ( "crypto/tls" "crypto/x509" "crypto/x509/pkix" - "encoding/asn1" "encoding/hex" "encoding/json" "encoding/pem" @@ -293,18 +292,6 @@ func WithCertificate(cert tls.Certificate) ClientOption { } } -var ( - stepOIDRoot = asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 37476, 9000, 64} - stepOIDProvisioner = append(asn1.ObjectIdentifier(nil), append(stepOIDRoot, 1)...) -) - -type stepProvisionerASN1 struct { - Type int - Name []byte - CredentialID []byte - KeyValuePairs []string `asn1:"optional,omitempty"` -} - // WithAdminX5C will set the given file as the X5C certificate for use // by the client. func WithAdminX5C(certs []*x509.Certificate, key interface{}, passwordFile string) ClientOption { From dfdc9c06ed49c5279330fca19d173a377a085f06 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 7 Apr 2022 18:33:13 -0700 Subject: [PATCH 08/26] Fix linter error importShadow --- authority/linkedca.go | 6 +++--- authority/provisioners.go | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/authority/linkedca.go b/authority/linkedca.go index 1b920ce8..29201164 100644 --- a/authority/linkedca.go +++ b/authority/linkedca.go @@ -246,14 +246,14 @@ func (c *linkedCaClient) GetCertificateData(serial string) (*db.CertificateData, return nil, err } - var provisioner *db.ProvisionerData + var pd *db.ProvisionerData if p := resp.Provisioner; p != nil { - provisioner = &db.ProvisionerData{ + pd = &db.ProvisionerData{ ID: p.Id, Name: p.Name, Type: p.Type.String(), } } return &db.CertificateData{ - Provisioner: provisioner, + Provisioner: pd, }, nil } diff --git a/authority/provisioners.go b/authority/provisioners.go index 3713705f..0dab24b9 100644 --- a/authority/provisioners.go +++ b/authority/provisioners.go @@ -275,7 +275,7 @@ func (a *Authority) RemoveProvisioner(ctx context.Context, id string) error { // CreateFirstProvisioner creates and stores the first provisioner when using // admin database provisioner storage. -func CreateFirstProvisioner(ctx context.Context, db admin.DB, password string) (*linkedca.Provisioner, error) { +func CreateFirstProvisioner(ctx context.Context, adminDB admin.DB, password string) (*linkedca.Provisioner, error) { if password == "" { pass, err := ui.PromptPasswordGenerate("Please enter the password to encrypt your first provisioner, leave empty and we'll generate one") if err != nil { @@ -318,7 +318,7 @@ func CreateFirstProvisioner(ctx context.Context, db admin.DB, password string) ( }, }, } - if err := db.CreateProvisioner(ctx, p); err != nil { + if err := adminDB.CreateProvisioner(ctx, p); err != nil { return nil, admin.WrapErrorISE(err, "error creating provisioner") } return p, nil From e53bd64861d6355c2817295ee137a152f7c17219 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Fri, 8 Apr 2022 11:13:42 -0700 Subject: [PATCH 09/26] Use release version of linkedca. --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 9c0da54d..e7653791 100644 --- a/go.mod +++ b/go.mod @@ -38,7 +38,7 @@ require ( go.mozilla.org/pkcs7 v0.0.0-20210826202110-33d05740a352 go.step.sm/cli-utils v0.7.0 go.step.sm/crypto v0.16.1 - go.step.sm/linkedca v0.12.1-0.20220408003202-c7e1ed60beab + go.step.sm/linkedca v0.13.0 golang.org/x/crypto v0.0.0-20211215153901-e495a2d5b3d3 golang.org/x/net v0.0.0-20220127200216-cd36cc0744dd google.golang.org/api v0.70.0 diff --git a/go.sum b/go.sum index 5dbb5735..034904b9 100644 --- a/go.sum +++ b/go.sum @@ -711,8 +711,8 @@ go.step.sm/cli-utils v0.7.0/go.mod h1:Ur6bqA/yl636kCUJbp30J7Unv5JJ226eW2KqXPDwF/ go.step.sm/crypto v0.9.0/go.mod h1:+CYG05Mek1YDqi5WK0ERc6cOpKly2i/a5aZmU1sfGj0= go.step.sm/crypto v0.16.1 h1:4mnZk21cSxyMGxsEpJwZKKvJvDu1PN09UVrWWFNUBdk= go.step.sm/crypto v0.16.1/go.mod h1:3G0yQr5lQqfEG0CMYz8apC/qMtjLRQlzflL2AxkcN+g= -go.step.sm/linkedca v0.12.1-0.20220408003202-c7e1ed60beab h1:s7IZreqiQONduJQfHcv/SI0rqQty3NKmEuiMESwBZwU= -go.step.sm/linkedca v0.12.1-0.20220408003202-c7e1ed60beab/go.mod h1:W59ucS4vFpuR0g4PtkGbbtXAwxbDEnNCg+ovkej1ANM= +go.step.sm/linkedca v0.13.0 h1:bRJEsyy3ZpHsSbDjKtaEBb3/vORzDuhS5+rQ2RsGopg= +go.step.sm/linkedca v0.13.0/go.mod h1:W59ucS4vFpuR0g4PtkGbbtXAwxbDEnNCg+ovkej1ANM= go.uber.org/atomic v1.3.2/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE= go.uber.org/atomic v1.4.0/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE= go.uber.org/atomic v1.5.0/go.mod h1:sABNBOSYdrvTF6hTgEIbc7YasKWGhgEQZyfxyTvoXHQ= From 1d1e09544785d07b9ff4abe95814b00a2a91786d Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Fri, 8 Apr 2022 13:06:29 -0700 Subject: [PATCH 10/26] Add tests for LoadProvisionerByCertificate. --- authority/provisioners.go | 50 +++++------ authority/provisioners_test.go | 147 +++++++++++++++++++++++++++++++++ db/db.go | 12 +++ 3 files changed, 185 insertions(+), 24 deletions(-) diff --git a/authority/provisioners.go b/authority/provisioners.go index 0dab24b9..7ff080ed 100644 --- a/authority/provisioners.go +++ b/authority/provisioners.go @@ -45,41 +45,43 @@ func (a *Authority) GetProvisioners(cursor string, limit int) (provisioner.List, // LoadProvisionerByCertificate returns an interface to the provisioner that // provisioned the certificate. func (a *Authority) LoadProvisionerByCertificate(crt *x509.Certificate) (provisioner.Interface, error) { - // Default implementation looks at the provisioner extension. - loadProvisioner := func() (provisioner.Interface, error) { - p, ok := a.provisioners.LoadByCertificate(crt) - if !ok { - return nil, admin.NewError(admin.ErrorNotFoundType, "unable to load provisioner from certificate") - } + a.adminMutex.RLock() + defer a.adminMutex.RUnlock() + if p, err := a.unsafeLoadProvisionerFromDatabase(crt); err == nil { return p, nil } + return a.unsafeLoadProvisionerFromExtension(crt) +} +func (a *Authority) unsafeLoadProvisionerFromExtension(crt *x509.Certificate) (provisioner.Interface, error) { + p, ok := a.provisioners.LoadByCertificate(crt) + if !ok || p.GetType() == 0 { + return nil, admin.NewError(admin.ErrorNotFoundType, "unable to load provisioner from certificate") + } + return p, nil +} + +func (a *Authority) unsafeLoadProvisionerFromDatabase(crt *x509.Certificate) (provisioner.Interface, error) { // certificateDataGetter is an interface that can be use to retrieve the // provisioner from a db or a linked ca. type certificateDataGetter interface { GetCertificateData(string) (*db.CertificateData, error) } - var cdg certificateDataGetter - if getter, ok := a.adminDB.(certificateDataGetter); ok { - cdg = getter - } else if getter, ok := a.db.(certificateDataGetter); ok { - cdg = getter + + var err error + var data *db.CertificateData + + if cdg, ok := a.adminDB.(certificateDataGetter); ok { + data, err = cdg.GetCertificateData(crt.SerialNumber.String()) + } else if cdg, ok := a.db.(certificateDataGetter); ok { + data, err = cdg.GetCertificateData(crt.SerialNumber.String()) } - if cdg != nil { - if data, err := cdg.GetCertificateData(crt.SerialNumber.String()); err == nil && data.Provisioner != nil { - loadProvisioner = func() (provisioner.Interface, error) { - p, ok := a.provisioners.Load(data.Provisioner.ID) - if !ok { - return nil, admin.NewError(admin.ErrorNotFoundType, "unable to load provisioner from certificate") - } - return p, nil - } + if err == nil && data != nil && data.Provisioner != nil { + if p, ok := a.provisioners.Load(data.Provisioner.ID); ok { + return p, nil } } - - a.adminMutex.RLock() - defer a.adminMutex.RUnlock() - return loadProvisioner() + return nil, admin.NewError(admin.ErrorNotFoundType, "unable to load provisioner from certificate") } // LoadProvisionerByToken returns an interface to the provisioner that diff --git a/authority/provisioners_test.go b/authority/provisioners_test.go index 81dc38bf..56cd16b1 100644 --- a/authority/provisioners_test.go +++ b/authority/provisioners_test.go @@ -1,13 +1,21 @@ package authority import ( + "context" + "crypto/x509" "errors" "net/http" + "reflect" "testing" + "time" "github.com/smallstep/assert" "github.com/smallstep/certificates/api/render" + "github.com/smallstep/certificates/authority/admin" "github.com/smallstep/certificates/authority/provisioner" + "github.com/smallstep/certificates/db" + "go.step.sm/crypto/jose" + "go.step.sm/crypto/keyutil" ) func TestGetEncryptedKey(t *testing.T) { @@ -67,6 +75,15 @@ func TestGetEncryptedKey(t *testing.T) { } } +type mockAdminDB struct { + admin.MockDB + MGetCertificateData func(string) (*db.CertificateData, error) +} + +func (c *mockAdminDB) GetCertificateData(sn string) (*db.CertificateData, error) { + return c.MGetCertificateData(sn) +} + func TestGetProvisioners(t *testing.T) { type gp struct { a *Authority @@ -104,3 +121,133 @@ func TestGetProvisioners(t *testing.T) { }) } } + +func TestAuthority_LoadProvisionerByCertificate(t *testing.T) { + _, priv, err := keyutil.GenerateDefaultKeyPair() + assert.FatalError(t, err) + csr := getCSR(t, priv) + + sign := func(a *Authority, extraOpts ...provisioner.SignOption) *x509.Certificate { + key, err := jose.ReadKey("testdata/secrets/step_cli_key_priv.jwk", jose.WithPassword([]byte("pass"))) + assert.FatalError(t, err) + token, err := generateToken("smallstep test", "step-cli", testAudiences.Sign[0], []string{"test.smallstep.com"}, time.Now(), key) + assert.FatalError(t, err) + ctx := provisioner.NewContextWithMethod(context.Background(), provisioner.SignMethod) + opts, err := a.Authorize(ctx, token) + assert.FatalError(t, err) + opts = append(opts, extraOpts...) + certs, err := a.Sign(csr, provisioner.SignOptions{}, opts...) + assert.FatalError(t, err) + return certs[0] + } + getProvisioner := func(a *Authority, name string) provisioner.Interface { + p, ok := a.provisioners.LoadByName(name) + if !ok { + t.Fatalf("provisioner %s does not exists", name) + } + return p + } + removeExtension := provisioner.CertificateEnforcerFunc(func(cert *x509.Certificate) error { + for i, ext := range cert.ExtraExtensions { + if ext.Id.Equal(provisioner.StepOIDProvisioner) { + cert.ExtraExtensions = append(cert.ExtraExtensions[:i], cert.ExtraExtensions[i+1:]...) + break + } + } + return nil + }) + + a0 := testAuthority(t) + + a1 := testAuthority(t) + a1.db = &db.MockAuthDB{ + MUseToken: func(id, tok string) (bool, error) { + return true, nil + }, + MGetCertificateData: func(serialNumber string) (*db.CertificateData, error) { + p, err := a1.LoadProvisionerByName("dev") + if err != nil { + t.Fatal(err) + } + return &db.CertificateData{ + Provisioner: &db.ProvisionerData{ + ID: p.GetID(), + Name: p.GetName(), + Type: p.GetType().String(), + }, + }, nil + }, + } + + a2 := testAuthority(t) + a2.adminDB = &mockAdminDB{ + MGetCertificateData: (func(s string) (*db.CertificateData, error) { + p, err := a2.LoadProvisionerByName("dev") + if err != nil { + t.Fatal(err) + } + return &db.CertificateData{ + Provisioner: &db.ProvisionerData{ + ID: p.GetID(), + Name: p.GetName(), + Type: p.GetType().String(), + }, + }, nil + }), + } + + a3 := testAuthority(t) + a3.db = &db.MockAuthDB{ + MUseToken: func(id, tok string) (bool, error) { + return true, nil + }, + MGetCertificateData: func(serialNumber string) (*db.CertificateData, error) { + return &db.CertificateData{ + Provisioner: &db.ProvisionerData{ + ID: "foo", Name: "foo", Type: "foo", + }, + }, nil + }, + } + + a4 := testAuthority(t) + a4.adminDB = &mockAdminDB{ + MGetCertificateData: func(serialNumber string) (*db.CertificateData, error) { + return &db.CertificateData{ + Provisioner: &db.ProvisionerData{ + ID: "foo", Name: "foo", Type: "foo", + }, + }, nil + }, + } + + type args struct { + crt *x509.Certificate + } + tests := []struct { + name string + authority *Authority + args args + want provisioner.Interface + wantErr bool + }{ + {"ok from certificate", a0, args{sign(a0)}, getProvisioner(a0, "step-cli"), false}, + {"ok from db", a1, args{sign(a1)}, getProvisioner(a1, "dev"), false}, + {"ok from admindb", a2, args{sign(a2)}, getProvisioner(a2, "dev"), false}, + {"fail from certificate", a0, args{sign(a0, removeExtension)}, nil, true}, + {"fail from db", a3, args{sign(a3, removeExtension)}, nil, true}, + {"fail from admindb", a4, args{sign(a4, removeExtension)}, nil, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := tt.authority.LoadProvisionerByCertificate(tt.args.crt) + if (err != nil) != tt.wantErr { + t.Errorf("Authority.LoadProvisionerByCertificate() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("Authority.LoadProvisionerByCertificate() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/db/db.go b/db/db.go index a3ebb19f..602e3623 100644 --- a/db/db.go +++ b/db/db.go @@ -359,6 +359,7 @@ type MockAuthDB struct { MRevoke func(rci *RevokedCertificateInfo) error MRevokeSSH func(rci *RevokedCertificateInfo) error MGetCertificate func(serialNumber string) (*x509.Certificate, error) + MGetCertificateData func(serialNumber string) (*CertificateData, error) MStoreCertificate func(crt *x509.Certificate) error MUseToken func(id, tok string) (bool, error) MIsSSHHost func(principal string) (bool, error) @@ -418,6 +419,17 @@ func (m *MockAuthDB) GetCertificate(serialNumber string) (*x509.Certificate, err return m.Ret1.(*x509.Certificate), m.Err } +// GetCertificateData mock. +func (m *MockAuthDB) GetCertificateData(serialNumber string) (*CertificateData, error) { + if m.MGetCertificateData != nil { + return m.MGetCertificateData(serialNumber) + } + if cd, ok := m.Ret1.(*CertificateData); ok { + return cd, m.Err + } + return nil, m.Err +} + // StoreCertificate mock. func (m *MockAuthDB) StoreCertificate(crt *x509.Certificate) error { if m.MStoreCertificate != nil { From af8fcf5b01251daadd06a058858123c266166542 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Fri, 8 Apr 2022 14:18:24 -0700 Subject: [PATCH 11/26] Use always LoadProvisionerByCertificate on authority package --- authority/authorize.go | 8 ++++---- authority/authorize_test.go | 23 +++++++++++++++++++++++ 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/authority/authorize.go b/authority/authorize.go index 0c1c6f22..95698b49 100644 --- a/authority/authorize.go +++ b/authority/authorize.go @@ -282,8 +282,8 @@ func (a *Authority) authorizeRenew(cert *x509.Certificate) error { if isRevoked { return errs.Unauthorized("authority.authorizeRenew: certificate has been revoked", opts...) } - p, ok := a.provisioners.LoadByCertificate(cert) - if !ok { + p, err := a.LoadProvisionerByCertificate(cert) + if err != nil { return errs.Unauthorized("authority.authorizeRenew: provisioner not found", opts...) } if err := p.AuthorizeRenew(context.Background(), cert); err != nil { @@ -383,8 +383,8 @@ func (a *Authority) AuthorizeRenewToken(ctx context.Context, ott string) (*x509. return nil, errs.InternalServerErr(err, errs.WithMessage("error validating renew token")) } - p, ok := a.provisioners.LoadByCertificate(leaf) - if !ok { + p, err := a.LoadProvisionerByCertificate(leaf) + if err != nil { return nil, errs.Unauthorized("error validating renew token: cannot get provisioner from certificate") } if err := a.UseToken(ott, p); err != nil { diff --git a/authority/authorize_test.go b/authority/authorize_test.go index a7bec277..c399eac4 100644 --- a/authority/authorize_test.go +++ b/authority/authorize_test.go @@ -847,6 +847,29 @@ func TestAuthority_authorizeRenew(t *testing.T) { cert: fooCrt, } }, + "ok/from db": func(t *testing.T) *authorizeTest { + a := testAuthority(t) + a.db = &db.MockAuthDB{ + MIsRevoked: func(key string) (bool, error) { + return false, nil + }, + MGetCertificateData: func(serialNumber string) (*db.CertificateData, error) { + p, ok := a.provisioners.LoadByName("step-cli") + if !ok { + t.Fatal("provisioner step-cli not found") + } + return &db.CertificateData{ + Provisioner: &db.ProvisionerData{ + ID: p.GetID(), + }, + }, nil + }, + } + return &authorizeTest{ + auth: a, + cert: fooCrt, + } + }, } for name, genTestCase := range tests { From 2ace3097b766f659aeebe3e8bb7398a7659cdb84 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Fri, 8 Apr 2022 14:29:20 -0700 Subject: [PATCH 12/26] Update changelog. --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 49e4b15e..8fafac5d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,13 +7,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## [Unreleased - 0.18.3] - DATE ### Added - Added support for renew after expiry using the claim `allowRenewAfterExpiry`. -- Added support for `extraNames` in X.509 templates. +- Added initial support for `extraNames` in X.509 templates. +- Added initial support for automatic configuration of linked RAs. ### Changed - Made SCEP CA URL paths dynamic - Support two latest versions of Go (1.17, 1.18) ### Deprecated ### Removed ### Fixed +- Fixed admin credentials on RAs. ### Security ## [0.18.2] - 2022-03-01 From 2fbff47acfeda882e07438e1d0800e6cf565b6ed Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Mon, 11 Apr 2022 12:18:44 -0700 Subject: [PATCH 13/26] Add missing return in test. --- ca/bootstrap_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/ca/bootstrap_test.go b/ca/bootstrap_test.go index 2332b4d4..9aaa5f1f 100644 --- a/ca/bootstrap_test.go +++ b/ca/bootstrap_test.go @@ -92,6 +92,7 @@ func mTLSMiddleware(next http.Handler, nonAuthenticatedPaths ...string) http.Han for _, s := range nonAuthenticatedPaths { if strings.HasPrefix(r.URL.Path, s) || strings.HasPrefix(r.URL.Path, "/1.0"+s) { next.ServeHTTP(w, r) + return } } isMTLS := r.TLS != nil && len(r.TLS.PeerCertificates) > 0 From c8c59d68f5988d3fefe4616c45f8f924165f4627 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Mon, 11 Apr 2022 12:19:42 -0700 Subject: [PATCH 14/26] Allow mTLS renewals if the provisioner extension does not exists. This fixes a backward compatibility issue with with the new LoadProvisionerByCertificate. --- authority/authorize.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/authority/authorize.go b/authority/authorize.go index 95698b49..6162fc0e 100644 --- a/authority/authorize.go +++ b/authority/authorize.go @@ -284,7 +284,13 @@ func (a *Authority) authorizeRenew(cert *x509.Certificate) error { } p, err := a.LoadProvisionerByCertificate(cert) if err != nil { - return errs.Unauthorized("authority.authorizeRenew: provisioner not found", opts...) + var ok bool + // For backward compatibility this method will also succeed if the + // provisioner does not have an extension. LoadByCertificate returns the + // noop provisioner if this happens, and it allow certificate renewals. + if p, ok = a.provisioners.LoadByCertificate(cert); !ok { + return errs.Unauthorized("authority.authorizeRenew: provisioner not found", opts...) + } } if err := p.AuthorizeRenew(context.Background(), cert); err != nil { return errs.Wrap(http.StatusInternalServerError, err, "authority.authorizeRenew", opts...) From 435bb8123bd37ca282abee17a66f39a1b06a85e6 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Mon, 11 Apr 2022 14:14:02 -0700 Subject: [PATCH 15/26] Upgrade codecov to v2 --- .github/workflows/test.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 64cb64cd..c0c9145a 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -59,8 +59,8 @@ jobs: - name: Codecov if: matrix.go == '1.18' - uses: codecov/codecov-action@v1.2.1 + uses: codecov/codecov-action@v2 with: - file: ./coverage.out # optional + files: ./coverage.out # optional name: codecov-umbrella # optional fail_ci_if_error: true # optional (default = false) From 1880b4b2d0f934d4dd11681288eb0034dd2b22fd Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Mon, 11 Apr 2022 14:21:14 -0700 Subject: [PATCH 16/26] Add codecov token. It shouldn't be necessary for public repos, but GitHub actions error suggests to add it. --- .github/workflows/test.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index c0c9145a..b24426a0 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -61,6 +61,7 @@ jobs: if: matrix.go == '1.18' uses: codecov/codecov-action@v2 with: + token: ${{ secrets.CODECOV_TOKEN }} files: ./coverage.out # optional name: codecov-umbrella # optional fail_ci_if_error: true # optional (default = false) From 00cd0f5f2194bd4a1573fdf3d105f5bbfe7c2e1d Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 12 Apr 2022 14:44:55 -0700 Subject: [PATCH 17/26] Apply suggestions from code review Co-authored-by: Herman Slatman --- authority/authorize.go | 2 +- authority/provisioners.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/authority/authorize.go b/authority/authorize.go index 6162fc0e..7dc58575 100644 --- a/authority/authorize.go +++ b/authority/authorize.go @@ -287,7 +287,7 @@ func (a *Authority) authorizeRenew(cert *x509.Certificate) error { var ok bool // For backward compatibility this method will also succeed if the // provisioner does not have an extension. LoadByCertificate returns the - // noop provisioner if this happens, and it allow certificate renewals. + // noop provisioner if this happens, and it allows certificate renewals. if p, ok = a.provisioners.LoadByCertificate(cert); !ok { return errs.Unauthorized("authority.authorizeRenew: provisioner not found", opts...) } diff --git a/authority/provisioners.go b/authority/provisioners.go index 7ff080ed..c20c7b68 100644 --- a/authority/provisioners.go +++ b/authority/provisioners.go @@ -62,7 +62,7 @@ func (a *Authority) unsafeLoadProvisionerFromExtension(crt *x509.Certificate) (p } func (a *Authority) unsafeLoadProvisionerFromDatabase(crt *x509.Certificate) (provisioner.Interface, error) { - // certificateDataGetter is an interface that can be use to retrieve the + // certificateDataGetter is an interface that can be used to retrieve the // provisioner from a db or a linked ca. type certificateDataGetter interface { GetCertificateData(string) (*db.CertificateData, error) From 0dc5646e319d5e1d911a72e2567f3c6c926264c1 Mon Sep 17 00:00:00 2001 From: Max Date: Tue, 12 Apr 2022 15:21:18 -0700 Subject: [PATCH 18/26] add Postgres to available databases in README --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 5c29ccdf..68883662 100644 --- a/README.md +++ b/README.md @@ -54,7 +54,7 @@ Setting up a *public key infrastructure* (PKI) is out of reach for many small te - [Short-lived certificates](https://smallstep.com/blog/passive-revocation.html) with automated enrollment, renewal, and passive revocation - Capable of high availability (HA) deployment using [root federation](https://smallstep.com/blog/step-v0.8.3-federation-root-rotation.html) and/or multiple intermediaries - Can operate as [an online intermediate CA for an existing root CA](https://smallstep.com/docs/tutorials/intermediate-ca-new-ca) -- [Badger, BoltDB, and MySQL database backends](https://smallstep.com/docs/step-ca/configuration#databases) +- [Badger, BoltDB, Postgres, and MySQL database backends](https://smallstep.com/docs/step-ca/configuration#databases) ### ⚙️ Many ways to automate From 0a5dc237dfc1c6619e00d7337e850eb212029ffa Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 12 Apr 2022 17:56:39 -0700 Subject: [PATCH 19/26] Fix typo in comment. --- authority/authorize.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/authority/authorize.go b/authority/authorize.go index 7dc58575..7121c55f 100644 --- a/authority/authorize.go +++ b/authority/authorize.go @@ -286,8 +286,9 @@ func (a *Authority) authorizeRenew(cert *x509.Certificate) error { if err != nil { var ok bool // For backward compatibility this method will also succeed if the - // provisioner does not have an extension. LoadByCertificate returns the - // noop provisioner if this happens, and it allows certificate renewals. + // certificate does not have a provisioner extension. LoadByCertificate + // returns the noop provisioner if this happens, and it allows + // certificate renewals. if p, ok = a.provisioners.LoadByCertificate(cert); !ok { return errs.Unauthorized("authority.authorizeRenew: provisioner not found", opts...) } From 3694ba30dce49ecdf8f60ca2a6a7ca7315b723b9 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 12 Apr 2022 18:42:27 -0700 Subject: [PATCH 20/26] Store certificate and provisioner in one transaction. --- db/db.go | 13 ++++++------ db/db_test.go | 57 ++++++++++++++++++--------------------------------- 2 files changed, 27 insertions(+), 43 deletions(-) diff --git a/db/db.go b/db/db.go index 602e3623..eccaf801 100644 --- a/db/db.go +++ b/db/db.go @@ -243,9 +243,7 @@ type ProvisionerData struct { // authorized the certificate. func (db *DB) StoreCertificateChain(p provisioner.Interface, chain ...*x509.Certificate) error { leaf := chain[0] - if err := db.StoreCertificate(leaf); err != nil { - return err - } + serialNumber := []byte(leaf.SerialNumber.String()) data := &CertificateData{} if p != nil { data.Provisioner = &ProvisionerData{ @@ -254,13 +252,16 @@ func (db *DB) StoreCertificateChain(p provisioner.Interface, chain ...*x509.Cert Type: p.GetType().String(), } } - b, err := json.Marshal(data) if err != nil { return errors.Wrap(err, "error marshaling json") } - if err := db.Set(certsDataTable, []byte(leaf.SerialNumber.String()), b); err != nil { - return errors.Wrap(err, "database Set error") + // Add certificate and certificate data in one transaction. + tx := new(database.Tx) + tx.Set(certsTable, serialNumber, leaf.Raw) + tx.Set(certsDataTable, serialNumber, b) + if err := db.Update(tx); err != nil { + return errors.Wrap(err, "database Update error") } return nil } diff --git a/db/db_test.go b/db/db_test.go index d7c58c9c..b4515a5b 100644 --- a/db/db_test.go +++ b/db/db_test.go @@ -188,53 +188,36 @@ func TestDB_StoreCertificateChain(t *testing.T) { wantErr bool }{ {"ok", fields{&MockNoSQLDB{ - MSet: func(bucket, key, value []byte) error { - switch string(bucket) { - case "x509_certs": - assert.Equals(t, key, []byte("1234")) - assert.Equals(t, value, []byte("the certificate")) - case "x509_certs_data": - assert.Equals(t, key, []byte("1234")) - assert.Equals(t, value, []byte(`{"provisioner":{"id":"some-id","name":"admin","type":"JWK"}}`)) - default: - t.Errorf("unexpected bucket %s", bucket) + MUpdate: func(tx *database.Tx) error { + if len(tx.Operations) != 2 { + t.Fatal("unexpected number of operations") } + assert.Equals(t, []byte("x509_certs"), tx.Operations[0].Bucket) + assert.Equals(t, []byte("1234"), tx.Operations[0].Key) + assert.Equals(t, []byte("the certificate"), tx.Operations[0].Value) + assert.Equals(t, []byte("x509_certs_data"), tx.Operations[1].Bucket) + assert.Equals(t, []byte("1234"), tx.Operations[1].Key) + assert.Equals(t, []byte(`{"provisioner":{"id":"some-id","name":"admin","type":"JWK"}}`), tx.Operations[1].Value) return nil }, }, true}, args{p, chain}, false}, {"ok no provisioner", fields{&MockNoSQLDB{ - MSet: func(bucket, key, value []byte) error { - switch string(bucket) { - case "x509_certs": - assert.Equals(t, key, []byte("1234")) - assert.Equals(t, value, []byte("the certificate")) - case "x509_certs_data": - assert.Equals(t, key, []byte("1234")) - assert.Equals(t, value, []byte(`{}`)) - default: - t.Errorf("unexpected bucket %s", bucket) + MUpdate: func(tx *database.Tx) error { + if len(tx.Operations) != 2 { + t.Fatal("unexpected number of operations") } + assert.Equals(t, []byte("x509_certs"), tx.Operations[0].Bucket) + assert.Equals(t, []byte("1234"), tx.Operations[0].Key) + assert.Equals(t, []byte("the certificate"), tx.Operations[0].Value) + assert.Equals(t, []byte("x509_certs_data"), tx.Operations[1].Bucket) + assert.Equals(t, []byte("1234"), tx.Operations[1].Key) + assert.Equals(t, []byte(`{}`), tx.Operations[1].Value) return nil }, }, true}, args{nil, chain}, false}, {"fail store certificate", fields{&MockNoSQLDB{ - MSet: func(bucket, key, value []byte) error { - switch string(bucket) { - case "x509_certs": - return errors.New("test error") - default: - return nil - } - }, - }, true}, args{p, chain}, true}, - {"fail store provisioner", fields{&MockNoSQLDB{ - MSet: func(bucket, key, value []byte) error { - switch string(bucket) { - case "x509_certs_data": - return errors.New("test error") - default: - return nil - } + MUpdate: func(tx *database.Tx) error { + return errors.New("test error") }, }, true}, args{p, chain}, true}, } From 4e4d4e882ffc9053112a2cc83b3799e6ad0cc7da Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 13 Apr 2022 14:50:06 -0700 Subject: [PATCH 21/26] Use a fixed string for renewal token issuer. --- authority/authorize.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/authority/authorize.go b/authority/authorize.go index 7121c55f..b0a1fab4 100644 --- a/authority/authorize.go +++ b/authority/authorize.go @@ -399,7 +399,7 @@ func (a *Authority) AuthorizeRenewToken(ctx context.Context, ott string) (*x509. } if err := claims.ValidateWithLeeway(jose.Expected{ - Issuer: p.GetName(), + Issuer: "step-ca-client/1.0", Subject: leaf.Subject.CommonName, Time: time.Now().UTC(), }, time.Minute); err != nil { From 674dc3c8443faac244ff0dea7a1d706227bfadda Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 13 Apr 2022 15:11:54 -0700 Subject: [PATCH 22/26] Rename unreleased claim to allowRenewalAfterExpiry for consistency. --- CHANGELOG.md | 2 +- authority/config/config.go | 28 ++++++++--------- authority/provisioner/claims.go | 40 ++++++++++++------------ authority/provisioner/controller.go | 4 +-- authority/provisioner/controller_test.go | 12 +++---- authority/provisioner/utils_test.go | 32 +++++++++---------- authority/provisioners.go | 14 ++++----- go.mod | 2 +- go.sum | 4 +-- 9 files changed, 69 insertions(+), 69 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 49e4b15e..f276240b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## [Unreleased - 0.18.3] - DATE ### Added -- Added support for renew after expiry using the claim `allowRenewAfterExpiry`. +- Added support for certificate renewals after expiry using the claim `allowRenewalAfterExpiry`. - Added support for `extraNames` in X.509 templates. ### Changed - Made SCEP CA URL paths dynamic diff --git a/authority/config/config.go b/authority/config/config.go index 2c437725..de4aa938 100644 --- a/authority/config/config.go +++ b/authority/config/config.go @@ -26,27 +26,27 @@ var ( DefaultBackdate = time.Minute // DefaultDisableRenewal disables renewals per provisioner. DefaultDisableRenewal = false - // DefaultAllowRenewAfterExpiry allows renewals even if the certificate is + // DefaultAllowRenewalAfterExpiry allows renewals even if the certificate is // expired. - DefaultAllowRenewAfterExpiry = false + DefaultAllowRenewalAfterExpiry = false // DefaultEnableSSHCA enable SSH CA features per provisioner or globally // for all provisioners. DefaultEnableSSHCA = false // GlobalProvisionerClaims default claims for the Authority. Can be overridden // by provisioner specific claims. GlobalProvisionerClaims = provisioner.Claims{ - MinTLSDur: &provisioner.Duration{Duration: 5 * time.Minute}, // TLS certs - MaxTLSDur: &provisioner.Duration{Duration: 24 * time.Hour}, - DefaultTLSDur: &provisioner.Duration{Duration: 24 * time.Hour}, - MinUserSSHDur: &provisioner.Duration{Duration: 5 * time.Minute}, // User SSH certs - MaxUserSSHDur: &provisioner.Duration{Duration: 24 * time.Hour}, - DefaultUserSSHDur: &provisioner.Duration{Duration: 16 * time.Hour}, - MinHostSSHDur: &provisioner.Duration{Duration: 5 * time.Minute}, // Host SSH certs - MaxHostSSHDur: &provisioner.Duration{Duration: 30 * 24 * time.Hour}, - DefaultHostSSHDur: &provisioner.Duration{Duration: 30 * 24 * time.Hour}, - EnableSSHCA: &DefaultEnableSSHCA, - DisableRenewal: &DefaultDisableRenewal, - AllowRenewAfterExpiry: &DefaultAllowRenewAfterExpiry, + MinTLSDur: &provisioner.Duration{Duration: 5 * time.Minute}, // TLS certs + MaxTLSDur: &provisioner.Duration{Duration: 24 * time.Hour}, + DefaultTLSDur: &provisioner.Duration{Duration: 24 * time.Hour}, + MinUserSSHDur: &provisioner.Duration{Duration: 5 * time.Minute}, // User SSH certs + MaxUserSSHDur: &provisioner.Duration{Duration: 24 * time.Hour}, + DefaultUserSSHDur: &provisioner.Duration{Duration: 16 * time.Hour}, + MinHostSSHDur: &provisioner.Duration{Duration: 5 * time.Minute}, // Host SSH certs + MaxHostSSHDur: &provisioner.Duration{Duration: 30 * 24 * time.Hour}, + DefaultHostSSHDur: &provisioner.Duration{Duration: 30 * 24 * time.Hour}, + EnableSSHCA: &DefaultEnableSSHCA, + DisableRenewal: &DefaultDisableRenewal, + AllowRenewalAfterExpiry: &DefaultAllowRenewalAfterExpiry, } ) diff --git a/authority/provisioner/claims.go b/authority/provisioner/claims.go index 2a3e2c61..96f19b37 100644 --- a/authority/provisioner/claims.go +++ b/authority/provisioner/claims.go @@ -24,8 +24,8 @@ type Claims struct { EnableSSHCA *bool `json:"enableSSHCA,omitempty"` // Renewal properties - DisableRenewal *bool `json:"disableRenewal,omitempty"` - AllowRenewAfterExpiry *bool `json:"allowRenewAfterExpiry,omitempty"` + DisableRenewal *bool `json:"disableRenewal,omitempty"` + AllowRenewalAfterExpiry *bool `json:"allowRenewalAfterExpiry,omitempty"` } // Claimer is the type that controls claims. It provides an interface around the @@ -44,22 +44,22 @@ func NewClaimer(claims *Claims, global Claims) (*Claimer, error) { // Claims returns the merge of the inner and global claims. func (c *Claimer) Claims() Claims { disableRenewal := c.IsDisableRenewal() - allowRenewAfterExpiry := c.AllowRenewAfterExpiry() + allowRenewalAfterExpiry := c.AllowRenewalAfterExpiry() enableSSHCA := c.IsSSHCAEnabled() return Claims{ - MinTLSDur: &Duration{c.MinTLSCertDuration()}, - MaxTLSDur: &Duration{c.MaxTLSCertDuration()}, - DefaultTLSDur: &Duration{c.DefaultTLSCertDuration()}, - MinUserSSHDur: &Duration{c.MinUserSSHCertDuration()}, - MaxUserSSHDur: &Duration{c.MaxUserSSHCertDuration()}, - DefaultUserSSHDur: &Duration{c.DefaultUserSSHCertDuration()}, - MinHostSSHDur: &Duration{c.MinHostSSHCertDuration()}, - MaxHostSSHDur: &Duration{c.MaxHostSSHCertDuration()}, - DefaultHostSSHDur: &Duration{c.DefaultHostSSHCertDuration()}, - EnableSSHCA: &enableSSHCA, - DisableRenewal: &disableRenewal, - AllowRenewAfterExpiry: &allowRenewAfterExpiry, + MinTLSDur: &Duration{c.MinTLSCertDuration()}, + MaxTLSDur: &Duration{c.MaxTLSCertDuration()}, + DefaultTLSDur: &Duration{c.DefaultTLSCertDuration()}, + MinUserSSHDur: &Duration{c.MinUserSSHCertDuration()}, + MaxUserSSHDur: &Duration{c.MaxUserSSHCertDuration()}, + DefaultUserSSHDur: &Duration{c.DefaultUserSSHCertDuration()}, + MinHostSSHDur: &Duration{c.MinHostSSHCertDuration()}, + MaxHostSSHDur: &Duration{c.MaxHostSSHCertDuration()}, + DefaultHostSSHDur: &Duration{c.DefaultHostSSHCertDuration()}, + EnableSSHCA: &enableSSHCA, + DisableRenewal: &disableRenewal, + AllowRenewalAfterExpiry: &allowRenewalAfterExpiry, } } @@ -109,14 +109,14 @@ func (c *Claimer) IsDisableRenewal() bool { return *c.claims.DisableRenewal } -// AllowRenewAfterExpiry returns if the renewal flow is authorized if the +// AllowRenewalAfterExpiry returns if the renewal flow is authorized if the // certificate is expired. If the property is not set within the provisioner // then the global value from the authority configuration will be used. -func (c *Claimer) AllowRenewAfterExpiry() bool { - if c.claims == nil || c.claims.AllowRenewAfterExpiry == nil { - return *c.global.AllowRenewAfterExpiry +func (c *Claimer) AllowRenewalAfterExpiry() bool { + if c.claims == nil || c.claims.AllowRenewalAfterExpiry == nil { + return *c.global.AllowRenewalAfterExpiry } - return *c.claims.AllowRenewAfterExpiry + return *c.claims.AllowRenewalAfterExpiry } // DefaultSSHCertDuration returns the default SSH certificate duration for the diff --git a/authority/provisioner/controller.go b/authority/provisioner/controller.go index a91ebaac..afd28dcc 100644 --- a/authority/provisioner/controller.go +++ b/authority/provisioner/controller.go @@ -124,7 +124,7 @@ func DefaultAuthorizeRenew(ctx context.Context, p *Controller, cert *x509.Certif if now.Before(cert.NotBefore) { return errs.Unauthorized("certificate is not yet valid" + " " + now.UTC().Format(time.RFC3339Nano) + " vs " + cert.NotBefore.Format(time.RFC3339Nano)) } - if now.After(cert.NotAfter) && !p.Claimer.AllowRenewAfterExpiry() { + if now.After(cert.NotAfter) && !p.Claimer.AllowRenewalAfterExpiry() { return errs.Unauthorized("certificate has expired") } @@ -144,7 +144,7 @@ func DefaultAuthorizeSSHRenew(ctx context.Context, p *Controller, cert *ssh.Cert if after := int64(cert.ValidAfter); after < 0 || unixNow < int64(cert.ValidAfter) { return errs.Unauthorized("certificate is not yet valid") } - if before := int64(cert.ValidBefore); cert.ValidBefore != uint64(ssh.CertTimeInfinity) && (unixNow >= before || before < 0) && !p.Claimer.AllowRenewAfterExpiry() { + if before := int64(cert.ValidBefore); cert.ValidBefore != uint64(ssh.CertTimeInfinity) && (unixNow >= before || before < 0) && !p.Claimer.AllowRenewalAfterExpiry() { return errs.Unauthorized("certificate has expired") } diff --git a/authority/provisioner/controller_test.go b/authority/provisioner/controller_test.go index 9fb90e9d..ebd38df1 100644 --- a/authority/provisioner/controller_test.go +++ b/authority/provisioner/controller_test.go @@ -160,13 +160,13 @@ func TestController_AuthorizeRenew(t *testing.T) { NotBefore: now, NotAfter: now.Add(time.Hour), }}, false}, - {"ok custom disabled", fields{&JWK{}, mustClaimer(t, &Claims{AllowRenewAfterExpiry: &trueValue}, globalProvisionerClaims), func(ctx context.Context, p *Controller, cert *x509.Certificate) error { + {"ok custom disabled", fields{&JWK{}, mustClaimer(t, &Claims{AllowRenewalAfterExpiry: &trueValue}, globalProvisionerClaims), func(ctx context.Context, p *Controller, cert *x509.Certificate) error { return nil }}, args{ctx, &x509.Certificate{ NotBefore: now, NotAfter: now.Add(time.Hour), }}, false}, - {"ok renew after expiry", fields{&JWK{}, mustClaimer(t, &Claims{AllowRenewAfterExpiry: &trueValue}, globalProvisionerClaims), nil}, args{ctx, &x509.Certificate{ + {"ok renew after expiry", fields{&JWK{}, mustClaimer(t, &Claims{AllowRenewalAfterExpiry: &trueValue}, globalProvisionerClaims), nil}, args{ctx, &x509.Certificate{ NotBefore: now.Add(-time.Hour), NotAfter: now.Add(-time.Minute), }}, false}, @@ -231,13 +231,13 @@ func TestController_AuthorizeSSHRenew(t *testing.T) { ValidAfter: uint64(now.Unix()), ValidBefore: uint64(now.Add(time.Hour).Unix()), }}, false}, - {"ok custom disabled", fields{&JWK{}, mustClaimer(t, &Claims{AllowRenewAfterExpiry: &trueValue}, globalProvisionerClaims), func(ctx context.Context, p *Controller, cert *ssh.Certificate) error { + {"ok custom disabled", fields{&JWK{}, mustClaimer(t, &Claims{AllowRenewalAfterExpiry: &trueValue}, globalProvisionerClaims), func(ctx context.Context, p *Controller, cert *ssh.Certificate) error { return nil }}, args{ctx, &ssh.Certificate{ ValidAfter: uint64(now.Unix()), ValidBefore: uint64(now.Add(time.Hour).Unix()), }}, false}, - {"ok renew after expiry", fields{&JWK{}, mustClaimer(t, &Claims{AllowRenewAfterExpiry: &trueValue}, globalProvisionerClaims), nil}, args{ctx, &ssh.Certificate{ + {"ok renew after expiry", fields{&JWK{}, mustClaimer(t, &Claims{AllowRenewalAfterExpiry: &trueValue}, globalProvisionerClaims), nil}, args{ctx, &ssh.Certificate{ ValidAfter: uint64(now.Add(-time.Hour).Unix()), ValidBefore: uint64(now.Add(-time.Minute).Unix()), }}, false}, @@ -296,7 +296,7 @@ func TestDefaultAuthorizeRenew(t *testing.T) { }}, false}, {"ok renew after expiry", args{ctx, &Controller{ Interface: &JWK{}, - Claimer: mustClaimer(t, &Claims{AllowRenewAfterExpiry: &trueValue}, globalProvisionerClaims), + Claimer: mustClaimer(t, &Claims{AllowRenewalAfterExpiry: &trueValue}, globalProvisionerClaims), }, &x509.Certificate{ NotBefore: now.Add(-time.Hour), NotAfter: now.Add(-time.Minute), @@ -354,7 +354,7 @@ func TestDefaultAuthorizeSSHRenew(t *testing.T) { }}, false}, {"ok renew after expiry", args{ctx, &Controller{ Interface: &JWK{}, - Claimer: mustClaimer(t, &Claims{AllowRenewAfterExpiry: &trueValue}, globalProvisionerClaims), + Claimer: mustClaimer(t, &Claims{AllowRenewalAfterExpiry: &trueValue}, globalProvisionerClaims), }, &ssh.Certificate{ ValidAfter: uint64(now.Add(-time.Hour).Unix()), ValidBefore: uint64(now.Add(-time.Minute).Unix()), diff --git a/authority/provisioner/utils_test.go b/authority/provisioner/utils_test.go index c55c58d2..3d032ea0 100644 --- a/authority/provisioner/utils_test.go +++ b/authority/provisioner/utils_test.go @@ -24,22 +24,22 @@ import ( ) var ( - defaultDisableRenewal = false - defaultAllowRenewAfterExpiry = false - defaultEnableSSHCA = true - globalProvisionerClaims = Claims{ - MinTLSDur: &Duration{5 * time.Minute}, - MaxTLSDur: &Duration{24 * time.Hour}, - DefaultTLSDur: &Duration{24 * time.Hour}, - MinUserSSHDur: &Duration{Duration: 5 * time.Minute}, // User SSH certs - MaxUserSSHDur: &Duration{Duration: 24 * time.Hour}, - DefaultUserSSHDur: &Duration{Duration: 16 * time.Hour}, - MinHostSSHDur: &Duration{Duration: 5 * time.Minute}, // Host SSH certs - MaxHostSSHDur: &Duration{Duration: 30 * 24 * time.Hour}, - DefaultHostSSHDur: &Duration{Duration: 30 * 24 * time.Hour}, - EnableSSHCA: &defaultEnableSSHCA, - DisableRenewal: &defaultDisableRenewal, - AllowRenewAfterExpiry: &defaultAllowRenewAfterExpiry, + defaultDisableRenewal = false + defaultAllowRenewalAfterExpiry = false + defaultEnableSSHCA = true + globalProvisionerClaims = Claims{ + MinTLSDur: &Duration{5 * time.Minute}, + MaxTLSDur: &Duration{24 * time.Hour}, + DefaultTLSDur: &Duration{24 * time.Hour}, + MinUserSSHDur: &Duration{Duration: 5 * time.Minute}, // User SSH certs + MaxUserSSHDur: &Duration{Duration: 24 * time.Hour}, + DefaultUserSSHDur: &Duration{Duration: 16 * time.Hour}, + MinHostSSHDur: &Duration{Duration: 5 * time.Minute}, // Host SSH certs + MaxHostSSHDur: &Duration{Duration: 30 * 24 * time.Hour}, + DefaultHostSSHDur: &Duration{Duration: 30 * 24 * time.Hour}, + EnableSSHCA: &defaultEnableSSHCA, + DisableRenewal: &defaultDisableRenewal, + AllowRenewalAfterExpiry: &defaultAllowRenewalAfterExpiry, } testAudiences = Audiences{ Sign: []string{"https://ca.smallstep.com/1.0/sign", "https://ca.smallstep.com/sign"}, diff --git a/authority/provisioners.go b/authority/provisioners.go index a6ac5aa8..c00ae179 100644 --- a/authority/provisioners.go +++ b/authority/provisioners.go @@ -437,8 +437,8 @@ func claimsToCertificates(c *linkedca.Claims) (*provisioner.Claims, error) { } pc := &provisioner.Claims{ - DisableRenewal: &c.DisableRenewal, - AllowRenewAfterExpiry: &c.AllowRenewAfterExpiry, + DisableRenewal: &c.DisableRenewal, + AllowRenewalAfterExpiry: &c.AllowRenewalAfterExpiry, } var err error @@ -476,18 +476,18 @@ func claimsToLinkedca(c *provisioner.Claims) *linkedca.Claims { } disableRenewal := config.DefaultDisableRenewal - allowRenewAfterExpiry := config.DefaultAllowRenewAfterExpiry + allowRenewalAfterExpiry := config.DefaultAllowRenewalAfterExpiry if c.DisableRenewal != nil { disableRenewal = *c.DisableRenewal } - if c.AllowRenewAfterExpiry != nil { - allowRenewAfterExpiry = *c.AllowRenewAfterExpiry + if c.AllowRenewalAfterExpiry != nil { + allowRenewalAfterExpiry = *c.AllowRenewalAfterExpiry } lc := &linkedca.Claims{ - DisableRenewal: disableRenewal, - AllowRenewAfterExpiry: allowRenewAfterExpiry, + DisableRenewal: disableRenewal, + AllowRenewalAfterExpiry: allowRenewalAfterExpiry, } if c.DefaultTLSDur != nil || c.MinTLSDur != nil || c.MaxTLSDur != nil { diff --git a/go.mod b/go.mod index 01ea550b..44d233bd 100644 --- a/go.mod +++ b/go.mod @@ -38,7 +38,7 @@ require ( go.mozilla.org/pkcs7 v0.0.0-20210826202110-33d05740a352 go.step.sm/cli-utils v0.7.0 go.step.sm/crypto v0.16.1 - go.step.sm/linkedca v0.12.0 + go.step.sm/linkedca v0.15.0 golang.org/x/crypto v0.0.0-20211215153901-e495a2d5b3d3 golang.org/x/net v0.0.0-20220127200216-cd36cc0744dd google.golang.org/api v0.70.0 diff --git a/go.sum b/go.sum index 42577048..a7bca88e 100644 --- a/go.sum +++ b/go.sum @@ -711,8 +711,8 @@ go.step.sm/cli-utils v0.7.0/go.mod h1:Ur6bqA/yl636kCUJbp30J7Unv5JJ226eW2KqXPDwF/ go.step.sm/crypto v0.9.0/go.mod h1:+CYG05Mek1YDqi5WK0ERc6cOpKly2i/a5aZmU1sfGj0= go.step.sm/crypto v0.16.1 h1:4mnZk21cSxyMGxsEpJwZKKvJvDu1PN09UVrWWFNUBdk= go.step.sm/crypto v0.16.1/go.mod h1:3G0yQr5lQqfEG0CMYz8apC/qMtjLRQlzflL2AxkcN+g= -go.step.sm/linkedca v0.12.0 h1:FA18uJO5P6W2pklcezMs+w+N3dVbpKEE1LP9HLsJgg4= -go.step.sm/linkedca v0.12.0/go.mod h1:W59ucS4vFpuR0g4PtkGbbtXAwxbDEnNCg+ovkej1ANM= +go.step.sm/linkedca v0.15.0 h1:lEkGRDY+u7FudGKt8yEo7nBy5OzceO9s3rl+/sZVL5M= +go.step.sm/linkedca v0.15.0/go.mod h1:W59ucS4vFpuR0g4PtkGbbtXAwxbDEnNCg+ovkej1ANM= go.uber.org/atomic v1.3.2/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE= go.uber.org/atomic v1.4.0/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE= go.uber.org/atomic v1.5.0/go.mod h1:sABNBOSYdrvTF6hTgEIbc7YasKWGhgEQZyfxyTvoXHQ= From 5f714f248527508ff45482c97165fe7d023d4657 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 13 Apr 2022 15:59:37 -0700 Subject: [PATCH 23/26] Fix tests for AuthorizeRenewToken --- authority/authorize_test.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/authority/authorize_test.go b/authority/authorize_test.go index c399eac4..cdcef1ad 100644 --- a/authority/authorize_test.go +++ b/authority/authorize_test.go @@ -1404,7 +1404,7 @@ func TestAuthority_AuthorizeRenewToken(t *testing.T) { t1, c1 := generateX5cToken(a1, signer, jose.Claims{ Audience: []string{"https://example.com/1.0/renew"}, Subject: "test.example.com", - Issuer: "step-cli", + Issuer: "step-ca-client/1.0", NotBefore: jose.NewNumericDate(now), Expiry: jose.NewNumericDate(now.Add(5 * time.Minute)), }, provisioner.CertificateEnforcerFunc(func(cert *x509.Certificate) error { @@ -1423,7 +1423,7 @@ func TestAuthority_AuthorizeRenewToken(t *testing.T) { t2, c2 := generateX5cToken(a1, signer, jose.Claims{ Audience: []string{"https://example.com/1.0/renew"}, Subject: "test.example.com", - Issuer: "step-cli", + Issuer: "step-ca-client/1.0", NotBefore: jose.NewNumericDate(now), Expiry: jose.NewNumericDate(now.Add(5 * time.Minute)), IssuedAt: jose.NewNumericDate(now), @@ -1443,7 +1443,7 @@ func TestAuthority_AuthorizeRenewToken(t *testing.T) { badSigner, _ := generateX5cToken(a1, otherSigner, jose.Claims{ Audience: []string{"https://example.com/1.0/renew"}, Subject: "test.example.com", - Issuer: "step-cli", + Issuer: "step-ca-client/1.0", NotBefore: jose.NewNumericDate(now), Expiry: jose.NewNumericDate(now.Add(5 * time.Minute)), }, provisioner.CertificateEnforcerFunc(func(cert *x509.Certificate) error { @@ -1462,7 +1462,7 @@ func TestAuthority_AuthorizeRenewToken(t *testing.T) { badProvisioner, _ := generateX5cToken(a1, signer, jose.Claims{ Audience: []string{"https://example.com/1.0/renew"}, Subject: "test.example.com", - Issuer: "step-cli", + Issuer: "step-ca-client/1.0", NotBefore: jose.NewNumericDate(now), Expiry: jose.NewNumericDate(now.Add(5 * time.Minute)), }, provisioner.CertificateEnforcerFunc(func(cert *x509.Certificate) error { @@ -1500,7 +1500,7 @@ func TestAuthority_AuthorizeRenewToken(t *testing.T) { badSubject, _ := generateX5cToken(a1, signer, jose.Claims{ Audience: []string{"https://example.com/1.0/renew"}, Subject: "bad-subject", - Issuer: "step-cli", + Issuer: "step-ca-client/1.0", NotBefore: jose.NewNumericDate(now), Expiry: jose.NewNumericDate(now.Add(5 * time.Minute)), }, provisioner.CertificateEnforcerFunc(func(cert *x509.Certificate) error { @@ -1519,7 +1519,7 @@ func TestAuthority_AuthorizeRenewToken(t *testing.T) { badNotBefore, _ := generateX5cToken(a1, signer, jose.Claims{ Audience: []string{"https://example.com/1.0/sign"}, Subject: "test.example.com", - Issuer: "step-cli", + Issuer: "step-ca-client/1.0", NotBefore: jose.NewNumericDate(now.Add(5 * time.Minute)), Expiry: jose.NewNumericDate(now.Add(10 * time.Minute)), }, provisioner.CertificateEnforcerFunc(func(cert *x509.Certificate) error { @@ -1538,7 +1538,7 @@ func TestAuthority_AuthorizeRenewToken(t *testing.T) { badExpiry, _ := generateX5cToken(a1, signer, jose.Claims{ Audience: []string{"https://example.com/1.0/sign"}, Subject: "test.example.com", - Issuer: "step-cli", + Issuer: "step-ca-client/1.0", NotBefore: jose.NewNumericDate(now.Add(-5 * time.Minute)), Expiry: jose.NewNumericDate(now.Add(-time.Minute)), }, provisioner.CertificateEnforcerFunc(func(cert *x509.Certificate) error { @@ -1557,7 +1557,7 @@ func TestAuthority_AuthorizeRenewToken(t *testing.T) { badIssuedAt, _ := generateX5cToken(a1, signer, jose.Claims{ Audience: []string{"https://example.com/1.0/sign"}, Subject: "test.example.com", - Issuer: "step-cli", + Issuer: "step-ca-client/1.0", NotBefore: jose.NewNumericDate(now), Expiry: jose.NewNumericDate(now.Add(5 * time.Minute)), IssuedAt: jose.NewNumericDate(now.Add(5 * time.Minute)), @@ -1577,7 +1577,7 @@ func TestAuthority_AuthorizeRenewToken(t *testing.T) { badAudience, _ := generateX5cToken(a1, signer, jose.Claims{ Audience: []string{"https://example.com/1.0/sign"}, Subject: "test.example.com", - Issuer: "step-cli", + Issuer: "step-ca-client/1.0", NotBefore: jose.NewNumericDate(now), Expiry: jose.NewNumericDate(now.Add(5 * time.Minute)), }, provisioner.CertificateEnforcerFunc(func(cert *x509.Certificate) error { From ad5aedfa6077fb1172adfc50f2fc07b2de67d06d Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 13 Apr 2022 16:00:15 -0700 Subject: [PATCH 24/26] Fix backward compatibility in AuthorizeAdminToken This commit validates both new and old issuers. --- authority/authorize.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/authority/authorize.go b/authority/authorize.go index b0a1fab4..fdf3941b 100644 --- a/authority/authorize.go +++ b/authority/authorize.go @@ -130,8 +130,7 @@ func (a *Authority) AuthorizeAdminToken(r *http.Request, token string) (*linkedc // According to "rfc7519 JSON Web Token" acceptable skew should be no // more than a few minutes. if err := claims.ValidateWithLeeway(jose.Expected{ - Issuer: "step-admin-client/1.0", - Time: time.Now().UTC(), + Time: time.Now().UTC(), }, time.Minute); err != nil { return nil, admin.WrapError(admin.ErrorUnauthorizedType, err, "x5c.authorizeToken; invalid x5c claims") } @@ -141,6 +140,12 @@ func (a *Authority) AuthorizeAdminToken(r *http.Request, token string) (*linkedc return nil, admin.NewError(admin.ErrorUnauthorizedType, "x5c.authorizeToken; x5c token has invalid audience claim (aud)") } + // validate issuer: old versions used the provisioner name, new version uses + // 'step-admin-client/1.0' + if claims.Issuer != "step-admin-client/1.0" && claims.Issuer != prov.GetName() { + return nil, admin.NewError(admin.ErrorUnauthorizedType, "x5c.authorizeToken; x5c token has invalid issuer claim (iss)") + } + if claims.Subject == "" { return nil, admin.NewError(admin.ErrorUnauthorizedType, "x5c.authorizeToken; x5c token subject cannot be empty") } From c066694c0cff29f9eae7e6f1aa05a842fc3b8781 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Mon, 18 Apr 2022 12:38:09 -0700 Subject: [PATCH 25/26] Allow renew token issuer to be the provisioner name. For consistency with AuthorizeAdminToken, AuthorizeRenewToken will allow the issuer to be either the fixed string 'step-ca-client/1.0' or the provisioner name. --- authority/authorize.go | 7 ++++++- authority/authorize_test.go | 20 ++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/authority/authorize.go b/authority/authorize.go index fdf3941b..7f9f456c 100644 --- a/authority/authorize.go +++ b/authority/authorize.go @@ -404,7 +404,6 @@ func (a *Authority) AuthorizeRenewToken(ctx context.Context, ott string) (*x509. } if err := claims.ValidateWithLeeway(jose.Expected{ - Issuer: "step-ca-client/1.0", Subject: leaf.Subject.CommonName, Time: time.Now().UTC(), }, time.Minute); err != nil { @@ -429,6 +428,12 @@ func (a *Authority) AuthorizeRenewToken(ctx context.Context, ott string) (*x509. return nil, errs.InternalServerErr(err, errs.WithMessage("error validating renew token: invalid audience claim (aud)")) } + // validate issuer: old versions used the provisioner name, new version uses + // 'step-ca-client/1.0' + if claims.Issuer != "step-ca-client/1.0" && claims.Issuer != p.GetName() { + return nil, admin.NewError(admin.ErrorUnauthorizedType, "error validating renew token: invalid issuer claim (iss)") + } + return leaf, nil } diff --git a/authority/authorize_test.go b/authority/authorize_test.go index cdcef1ad..0a1ef53c 100644 --- a/authority/authorize_test.go +++ b/authority/authorize_test.go @@ -1440,6 +1440,25 @@ func TestAuthority_AuthorizeRenewToken(t *testing.T) { }) return nil })) + t3, c3 := generateX5cToken(a1, signer, jose.Claims{ + Audience: []string{"https://example.com/1.0/renew"}, + Subject: "test.example.com", + Issuer: "step-cli", + NotBefore: jose.NewNumericDate(now), + Expiry: jose.NewNumericDate(now.Add(5 * time.Minute)), + }, provisioner.CertificateEnforcerFunc(func(cert *x509.Certificate) error { + cert.NotBefore = now + cert.NotAfter = now.Add(time.Hour) + b, err := asn1.Marshal(stepProvisionerASN1{int(provisioner.TypeJWK), []byte("step-cli"), nil, nil}) + if err != nil { + return err + } + cert.ExtraExtensions = append(cert.ExtraExtensions, pkix.Extension{ + Id: asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 37476, 9000, 64, 1}, + Value: b, + }) + return nil + })) badSigner, _ := generateX5cToken(a1, otherSigner, jose.Claims{ Audience: []string{"https://example.com/1.0/renew"}, Subject: "test.example.com", @@ -1607,6 +1626,7 @@ func TestAuthority_AuthorizeRenewToken(t *testing.T) { }{ {"ok", a1, args{ctx, t1}, c1, false}, {"ok expired cert", a1, args{ctx, t2}, c2, false}, + {"ok provisioner issuer", a1, args{ctx, t3}, c3, false}, {"fail token", a1, args{ctx, "not.a.token"}, nil, true}, {"fail token reuse", a1, args{ctx, t1}, nil, true}, {"fail token signature", a1, args{ctx, badSigner}, nil, true}, From 4770b405ba1e8147597e516ee6708615ec0293c7 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Mon, 18 Apr 2022 15:18:23 -0700 Subject: [PATCH 26/26] Drop any query string from the admin tokens This commit makes sure the admin token audience is passed without a query string (or any fragment). --- ca/adminClient.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ca/adminClient.go b/ca/adminClient.go index c3ba666f..72f62dd8 100644 --- a/ca/adminClient.go +++ b/ca/adminClient.go @@ -90,6 +90,13 @@ func (c *AdminClient) generateAdminToken(aud *url.URL) (string, error) { return "", err } + // Drop any query string parameter from the token audience + aud = &url.URL{ + Scheme: aud.Scheme, + Host: aud.Host, + Path: aud.Path, + } + now := time.Now() tokOptions := []token.Options{ token.WithJWTID(jwtID),