From 20b2c6a2017ac0f370a7916b7fd2104575427347 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 18 May 2022 18:27:37 -0700 Subject: [PATCH 1/9] Extract cert storer methods from AuthDB To be able to extend the AuthDB with methods that also extend the provisioner we need to either create a new method or to split the interface. This change splits the interface so we can have a cleaner implementation. --- db/db.go | 9 +++++++-- db/simple.go | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/db/db.go b/db/db.go index eccaf801..8cd1db0f 100644 --- a/db/db.go +++ b/db/db.go @@ -50,14 +50,19 @@ type AuthDB interface { Revoke(rci *RevokedCertificateInfo) error RevokeSSH(rci *RevokedCertificateInfo) error GetCertificate(serialNumber string) (*x509.Certificate, error) - StoreCertificate(crt *x509.Certificate) error UseToken(id, tok string) (bool, error) IsSSHHost(name string) (bool, error) - StoreSSHCertificate(crt *ssh.Certificate) error GetSSHHostPrincipals() ([]string, error) Shutdown() error } +// CertificateStorer is an extension of AuthDB that allows to store +// certificates. +type CertificateStorer interface { + StoreCertificate(crt *x509.Certificate) error + StoreSSHCertificate(crt *ssh.Certificate) error +} + // DB is a wrapper over the nosql.DB interface. type DB struct { nosql.DB diff --git a/db/simple.go b/db/simple.go index 0e5426ec..a7e38de9 100644 --- a/db/simple.go +++ b/db/simple.go @@ -20,7 +20,7 @@ type SimpleDB struct { usedTokens *sync.Map } -func newSimpleDB(c *Config) (AuthDB, error) { +func newSimpleDB(c *Config) (*SimpleDB, error) { db := &SimpleDB{} db.usedTokens = new(sync.Map) return db, nil From de99c3cac00385ee20b92c1e74eaf7185ba69ce5 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 18 May 2022 18:30:53 -0700 Subject: [PATCH 2/9] Report provisioner and parent on linkedca --- authority/linkedca.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/authority/linkedca.go b/authority/linkedca.go index 0552f2d1..9daba353 100644 --- a/authority/linkedca.go +++ b/authority/linkedca.go @@ -292,11 +292,22 @@ func (c *linkedCaClient) StoreRenewedCertificate(parent *x509.Certificate, fullc return errors.Wrap(err, "error posting certificate") } -func (c *linkedCaClient) StoreSSHCertificate(crt *ssh.Certificate) error { +func (c *linkedCaClient) StoreSSHCertificate(prov provisioner.Interface, crt *ssh.Certificate) error { ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) defer cancel() _, err := c.client.PostSSHCertificate(ctx, &linkedca.SSHCertificateRequest{ Certificate: string(ssh.MarshalAuthorizedKey(crt)), + Provisioner: createProvisionerIdentity(prov), + }) + return errors.Wrap(err, "error posting ssh certificate") +} + +func (c *linkedCaClient) StoreRenewedSSHCertificate(parent, crt *ssh.Certificate) error { + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + _, err := c.client.PostSSHCertificate(ctx, &linkedca.SSHCertificateRequest{ + Certificate: string(ssh.MarshalAuthorizedKey(crt)), + ParentCertificate: string(ssh.MarshalAuthorizedKey(parent)), }) return errors.Wrap(err, "error posting ssh certificate") } From c8d7ad7ab92d2f3963ef6e9559d89d7c1562f7fc Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 18 May 2022 18:33:22 -0700 Subject: [PATCH 3/9] Fix store certificates methods with new interface --- authority/tls.go | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/authority/tls.go b/authority/tls.go index d23b0da7..fd21ae98 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -365,28 +365,31 @@ func (a *Authority) Rekey(oldCert *x509.Certificate, pk crypto.PublicKey) ([]*x5 // `StoreCertificate(...*x509.Certificate) error` instead of just // `StoreCertificate(*x509.Certificate) error`. func (a *Authority) storeCertificate(prov provisioner.Interface, fullchain []*x509.Certificate) error { - type linkedChainStorer interface { + type certificateChainStorer interface { StoreCertificateChain(provisioner.Interface, ...*x509.Certificate) error } - type certificateChainStorer interface { + type certificateChainSimpleStorer interface { StoreCertificateChain(...*x509.Certificate) error } + // Store certificate in linkedca switch s := a.adminDB.(type) { - case linkedChainStorer: - return s.StoreCertificateChain(prov, fullchain...) case certificateChainStorer: + return s.StoreCertificateChain(prov, fullchain...) + case certificateChainSimpleStorer: return s.StoreCertificateChain(fullchain...) } // Store certificate in local db switch s := a.db.(type) { - case linkedChainStorer: - return s.StoreCertificateChain(prov, fullchain...) case certificateChainStorer: + return s.StoreCertificateChain(prov, fullchain...) + case certificateChainSimpleStorer: return s.StoreCertificateChain(fullchain...) + case db.CertificateStorer: + return s.StoreCertificate(fullchain[0]) default: - return a.db.StoreCertificate(fullchain[0]) + return nil } } @@ -398,15 +401,21 @@ func (a *Authority) storeRenewedCertificate(oldCert *x509.Certificate, fullchain type renewedCertificateChainStorer interface { StoreRenewedCertificate(*x509.Certificate, ...*x509.Certificate) error } + // Store certificate in linkedca if s, ok := a.adminDB.(renewedCertificateChainStorer); ok { return s.StoreRenewedCertificate(oldCert, fullchain...) } + // Store certificate in local db - if s, ok := a.db.(renewedCertificateChainStorer); ok { + switch s := a.db.(type) { + case renewedCertificateChainStorer: return s.StoreRenewedCertificate(oldCert, fullchain...) + case db.CertificateStorer: + return s.StoreCertificate(fullchain[0]) + default: + return nil } - return a.db.StoreCertificate(fullchain[0]) } // RevokeOptions are the options for the Revoke API. From 293586079af39d21b10510ee6ebc472c33ff4028 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 18 May 2022 18:33:53 -0700 Subject: [PATCH 4/9] Store provisioner with SignSSH This change also allows to store the old certificate on renewal on linkedca or if the db interface supports it. --- authority/ssh.go | 59 +++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 51 insertions(+), 8 deletions(-) diff --git a/authority/ssh.go b/authority/ssh.go index 0521ab58..bb69f9db 100644 --- a/authority/ssh.go +++ b/authority/ssh.go @@ -161,8 +161,13 @@ func (a *Authority) SignSSH(ctx context.Context, key ssh.PublicKey, opts provisi // Set backdate with the configured value opts.Backdate = a.config.AuthorityConfig.Backdate.Duration + var prov provisioner.Interface for _, op := range signOpts { switch o := op.(type) { + // Capture current provisioner + case provisioner.Interface: + prov = o + // add options to NewCertificate case provisioner.SSHCertificateOptions: certOptions = append(certOptions, o.Options(opts)...) @@ -276,7 +281,7 @@ func (a *Authority) SignSSH(ctx context.Context, key ssh.PublicKey, opts provisi } } - if err = a.storeSSHCertificate(cert); err != nil && err != db.ErrNotImplemented { + if err = a.storeSSHCertificate(prov, cert); err != nil && err != db.ErrNotImplemented { return nil, errs.Wrap(http.StatusInternalServerError, err, "authority.SignSSH: error storing certificate in db") } @@ -340,7 +345,7 @@ func (a *Authority) RenewSSH(ctx context.Context, oldCert *ssh.Certificate) (*ss return nil, errs.Wrap(http.StatusInternalServerError, err, "signSSH: error signing certificate") } - if err = a.storeSSHCertificate(cert); err != nil && err != db.ErrNotImplemented { + if err = a.storeRenewedSSHCertificate(oldCert, cert); err != nil && err != db.ErrNotImplemented { return nil, errs.Wrap(http.StatusInternalServerError, err, "renewSSH: error storing certificate in db") } @@ -419,21 +424,59 @@ func (a *Authority) RekeySSH(ctx context.Context, oldCert *ssh.Certificate, pub } } - if err = a.storeSSHCertificate(cert); err != nil && err != db.ErrNotImplemented { + if err = a.storeRenewedSSHCertificate(oldCert, cert); err != nil && err != db.ErrNotImplemented { return nil, errs.Wrap(http.StatusInternalServerError, err, "rekeySSH; error storing certificate in db") } return cert, nil } -func (a *Authority) storeSSHCertificate(cert *ssh.Certificate) error { +func (a *Authority) storeSSHCertificate(prov provisioner.Interface, cert *ssh.Certificate) error { type sshCertificateStorer interface { - StoreSSHCertificate(crt *ssh.Certificate) error + StoreSSHCertificate(provisioner.Interface, *ssh.Certificate) error } - if s, ok := a.adminDB.(sshCertificateStorer); ok { + + // Store certificate in admindb or linkedca + switch s := a.adminDB.(type) { + case sshCertificateStorer: + return s.StoreSSHCertificate(prov, cert) + case db.CertificateStorer: return s.StoreSSHCertificate(cert) } - return a.db.StoreSSHCertificate(cert) + + // Store certificate in localdb + switch s := a.db.(type) { + case sshCertificateStorer: + return s.StoreSSHCertificate(prov, cert) + case db.CertificateStorer: + return s.StoreSSHCertificate(cert) + default: + return nil + } +} + +func (a *Authority) storeRenewedSSHCertificate(parent, cert *ssh.Certificate) error { + type sshRenewerCertificateStorer interface { + StoreRenewedSSHCertificate(parent, cert *ssh.Certificate) error + } + + // Store certificate in admindb or linkedca + switch s := a.adminDB.(type) { + case sshRenewerCertificateStorer: + return s.StoreRenewedSSHCertificate(parent, cert) + case db.CertificateStorer: + return s.StoreSSHCertificate(cert) + } + + // Store certificate in localdb + switch s := a.db.(type) { + case sshRenewerCertificateStorer: + return s.StoreRenewedSSHCertificate(parent, cert) + case db.CertificateStorer: + return s.StoreSSHCertificate(cert) + default: + return nil + } } // IsValidForAddUser checks if a user provisioner certificate can be issued to @@ -511,7 +554,7 @@ func (a *Authority) SignSSHAddUser(ctx context.Context, key ssh.PublicKey, subje } cert.Signature = sig - if err = a.storeSSHCertificate(cert); err != nil && err != db.ErrNotImplemented { + if err = a.storeRenewedSSHCertificate(subject, cert); err != nil && err != db.ErrNotImplemented { return nil, errs.Wrap(http.StatusInternalServerError, err, "signSSHAddUser: error storing certificate in db") } From e7d7eb1a947bd60cc70fc67abe775c9f6060a29e Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 18 May 2022 18:42:42 -0700 Subject: [PATCH 5/9] Add provisioner as a signOption for SSH --- authority/provisioner/aws.go | 1 + authority/provisioner/azure.go | 1 + authority/provisioner/gcp.go | 1 + authority/provisioner/jwk.go | 1 + authority/provisioner/k8sSA.go | 1 + authority/provisioner/nebula.go | 1 + authority/provisioner/noop.go | 2 +- authority/provisioner/oidc.go | 1 + authority/provisioner/x5c.go | 1 + 9 files changed, 9 insertions(+), 1 deletion(-) diff --git a/authority/provisioner/aws.go b/authority/provisioner/aws.go index 8433fde5..afc61dd7 100644 --- a/authority/provisioner/aws.go +++ b/authority/provisioner/aws.go @@ -747,6 +747,7 @@ func (p *AWS) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption, signOptions = append(signOptions, templateOptions) return append(signOptions, + p, // Validate user SignSSHOptions. sshCertOptionsValidator(defaults), // Set the validity bounds if not set. diff --git a/authority/provisioner/azure.go b/authority/provisioner/azure.go index 438ab5b3..b6f7ec91 100644 --- a/authority/provisioner/azure.go +++ b/authority/provisioner/azure.go @@ -418,6 +418,7 @@ func (p *Azure) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOptio signOptions = append(signOptions, templateOptions) return append(signOptions, + p, // Validate user SignSSHOptions. sshCertOptionsValidator(defaults), // Set the validity bounds if not set. diff --git a/authority/provisioner/gcp.go b/authority/provisioner/gcp.go index 94c19e17..a116312d 100644 --- a/authority/provisioner/gcp.go +++ b/authority/provisioner/gcp.go @@ -425,6 +425,7 @@ func (p *GCP) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption, signOptions = append(signOptions, templateOptions) return append(signOptions, + p, // Validate user SignSSHOptions. sshCertOptionsValidator(defaults), // Set the validity bounds if not set. diff --git a/authority/provisioner/jwk.go b/authority/provisioner/jwk.go index 336736db..de592941 100644 --- a/authority/provisioner/jwk.go +++ b/authority/provisioner/jwk.go @@ -257,6 +257,7 @@ func (p *JWK) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption, } return append(signOptions, + p, // Set the validity bounds if not set. &sshDefaultDuration{p.ctl.Claimer}, // Validate public key diff --git a/authority/provisioner/k8sSA.go b/authority/provisioner/k8sSA.go index e2dbf840..28be0d5c 100644 --- a/authority/provisioner/k8sSA.go +++ b/authority/provisioner/k8sSA.go @@ -275,6 +275,7 @@ func (p *K8sSA) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOptio signOptions := []SignOption{templateOptions} return append(signOptions, + p, // Require type, key-id and principals in the SignSSHOptions. &sshCertOptionsRequireValidator{CertType: true, KeyID: true, Principals: true}, // Set the validity bounds if not set. diff --git a/authority/provisioner/nebula.go b/authority/provisioner/nebula.go index 38a2409f..cde5857c 100644 --- a/authority/provisioner/nebula.go +++ b/authority/provisioner/nebula.go @@ -250,6 +250,7 @@ func (p *Nebula) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOpti } return append(signOptions, + p, templateOptions, // Checks the validity bounds, and set the validity if has not been set. &sshLimitDuration{p.ctl.Claimer, crt.Details.NotAfter}, diff --git a/authority/provisioner/noop.go b/authority/provisioner/noop.go index 39661e54..9ccd0c8c 100644 --- a/authority/provisioner/noop.go +++ b/authority/provisioner/noop.go @@ -50,7 +50,7 @@ func (p *noop) AuthorizeRevoke(ctx context.Context, token string) error { } func (p *noop) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption, error) { - return []SignOption{}, nil + return []SignOption{p}, nil } func (p *noop) AuthorizeSSHRenew(ctx context.Context, token string) (*ssh.Certificate, error) { diff --git a/authority/provisioner/oidc.go b/authority/provisioner/oidc.go index 9f389b29..e64d98d9 100644 --- a/authority/provisioner/oidc.go +++ b/authority/provisioner/oidc.go @@ -434,6 +434,7 @@ func (o *OIDC) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption } return append(signOptions, + o, // Set the validity bounds if not set. &sshDefaultDuration{o.ctl.Claimer}, // Validate public key diff --git a/authority/provisioner/x5c.go b/authority/provisioner/x5c.go index 69576da5..b9ae24c5 100644 --- a/authority/provisioner/x5c.go +++ b/authority/provisioner/x5c.go @@ -312,6 +312,7 @@ func (p *X5C) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption, } return append(signOptions, + p, // Checks the validity bounds, and set the validity if has not been set. &sshLimitDuration{p.ctl.Claimer, claims.chains[0][0].NotAfter}, // Validate public key. From a627f2144039e9b2e56020e18ced2f7b6b80c2cf Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 18 May 2022 18:51:36 -0700 Subject: [PATCH 6/9] Fix AuthorizeSSHSign tests with extra SignOption --- authority/authorize_test.go | 2 +- authority/provisioner/aws_test.go | 1 - authority/provisioner/k8sSA_test.go | 3 ++- authority/provisioner/ssh_test.go | 1 + authority/provisioner/x5c_test.go | 5 +++-- 5 files changed, 7 insertions(+), 5 deletions(-) diff --git a/authority/authorize_test.go b/authority/authorize_test.go index 087318be..b221d0de 100644 --- a/authority/authorize_test.go +++ b/authority/authorize_test.go @@ -1034,7 +1034,7 @@ func TestAuthority_authorizeSSHSign(t *testing.T) { } } else { if assert.Nil(t, tc.err) { - assert.Len(t, 8, got) // number of provisioner.SignOptions returned + assert.Len(t, 9, got) // number of provisioner.SignOptions returned } } }) diff --git a/authority/provisioner/aws_test.go b/authority/provisioner/aws_test.go index 0d9b5d4d..d12d0626 100644 --- a/authority/provisioner/aws_test.go +++ b/authority/provisioner/aws_test.go @@ -813,7 +813,6 @@ func TestAWS_AuthorizeSSHSign(t *testing.T) { } else if assert.NotNil(t, got) { cert, err := signSSHCertificate(tt.args.key, tt.args.sshOpts, got, signer.Key.(crypto.Signer)) if (err != nil) != tt.wantSignErr { - t.Errorf("SignSSH error = %v, wantSignErr %v", err, tt.wantSignErr) } else { if tt.wantSignErr { diff --git a/authority/provisioner/k8sSA_test.go b/authority/provisioner/k8sSA_test.go index 1eff379d..2458babb 100644 --- a/authority/provisioner/k8sSA_test.go +++ b/authority/provisioner/k8sSA_test.go @@ -368,9 +368,10 @@ func TestK8sSA_AuthorizeSSHSign(t *testing.T) { } else { if assert.Nil(t, tc.err) { if assert.NotNil(t, opts) { - assert.Len(t, 7, opts) + assert.Len(t, 8, opts) for _, o := range opts { switch v := o.(type) { + case Interface: case sshCertificateOptionsFunc: case *sshCertOptionsRequireValidator: assert.Equals(t, v, &sshCertOptionsRequireValidator{CertType: true, KeyID: true, Principals: true}) diff --git a/authority/provisioner/ssh_test.go b/authority/provisioner/ssh_test.go index c530cd3c..90271443 100644 --- a/authority/provisioner/ssh_test.go +++ b/authority/provisioner/ssh_test.go @@ -53,6 +53,7 @@ func signSSHCertificate(key crypto.PublicKey, opts SignSSHOptions, signOpts []Si for _, op := range signOpts { switch o := op.(type) { + case Interface: // add options to NewCertificate case SSHCertificateOptions: certOptions = append(certOptions, o.Options(opts)...) diff --git a/authority/provisioner/x5c_test.go b/authority/provisioner/x5c_test.go index f28fcc7c..3bcf30d1 100644 --- a/authority/provisioner/x5c_test.go +++ b/authority/provisioner/x5c_test.go @@ -769,6 +769,7 @@ func TestX5C_AuthorizeSSHSign(t *testing.T) { nw := now() for _, o := range opts { switch v := o.(type) { + case Interface: case sshCertOptionsValidator: tc.claims.Step.SSH.ValidAfter.t = time.Time{} tc.claims.Step.SSH.ValidBefore.t = time.Time{} @@ -799,9 +800,9 @@ func TestX5C_AuthorizeSSHSign(t *testing.T) { tot++ } if len(tc.claims.Step.SSH.CertType) > 0 { - assert.Equals(t, tot, 10) + assert.Equals(t, tot, 11) } else { - assert.Equals(t, tot, 8) + assert.Equals(t, tot, 9) } } } From dd985ce154a4abd7733e303679df2698b365c07b Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 19 May 2022 18:41:13 -0700 Subject: [PATCH 7/9] Clarify errors when sending renewed certificates --- authority/linkedca.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/authority/linkedca.go b/authority/linkedca.go index 9daba353..fd5c0a81 100644 --- a/authority/linkedca.go +++ b/authority/linkedca.go @@ -289,7 +289,7 @@ func (c *linkedCaClient) StoreRenewedCertificate(parent *x509.Certificate, fullc PemCertificateChain: serializeCertificateChain(fullchain[1:]...), PemParentCertificate: serializeCertificateChain(parent), }) - return errors.Wrap(err, "error posting certificate") + return errors.Wrap(err, "error posting renewed certificate") } func (c *linkedCaClient) StoreSSHCertificate(prov provisioner.Interface, crt *ssh.Certificate) error { @@ -309,7 +309,7 @@ func (c *linkedCaClient) StoreRenewedSSHCertificate(parent, crt *ssh.Certificate Certificate: string(ssh.MarshalAuthorizedKey(crt)), ParentCertificate: string(ssh.MarshalAuthorizedKey(parent)), }) - return errors.Wrap(err, "error posting ssh certificate") + return errors.Wrap(err, "error posting renewed ssh certificate") } func (c *linkedCaClient) Revoke(crt *x509.Certificate, rci *db.RevokedCertificateInfo) error { From 1ad75a3bdb10c77de7d8dc671b5000d2c27ac563 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 19 May 2022 18:51:51 -0700 Subject: [PATCH 8/9] Skip failing test for now This test fails randomly on VMs, there's an issue to fix this so skipping it for now --- ca/bootstrap_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/ca/bootstrap_test.go b/ca/bootstrap_test.go index 9aaa5f1f..9eb3b2ea 100644 --- a/ca/bootstrap_test.go +++ b/ca/bootstrap_test.go @@ -370,6 +370,7 @@ func TestBootstrapClient(t *testing.T) { } func TestBootstrapClientServerRotation(t *testing.T) { + t.Skipf("skip until we fix https://github.com/smallstep/certificates/issues/873") reset := setMinCertDuration(1 * time.Second) defer reset() From eebbd65dd534091bdbaa610988f278eb56a27c5a Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Fri, 20 May 2022 12:03:36 -0700 Subject: [PATCH 9/9] Fix linter error --- ca/bootstrap_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ca/bootstrap_test.go b/ca/bootstrap_test.go index 9eb3b2ea..10e22519 100644 --- a/ca/bootstrap_test.go +++ b/ca/bootstrap_test.go @@ -7,6 +7,7 @@ import ( "net" "net/http" "net/http/httptest" + "os" "reflect" "strings" "sync" @@ -370,7 +371,9 @@ func TestBootstrapClient(t *testing.T) { } func TestBootstrapClientServerRotation(t *testing.T) { - t.Skipf("skip until we fix https://github.com/smallstep/certificates/issues/873") + if os.Getenv("CI") == "true" { + t.Skipf("skip until we fix https://github.com/smallstep/certificates/issues/873") + } reset := setMinCertDuration(1 * time.Second) defer reset()