diff --git a/authority/authorize.go b/authority/authorize.go index 8d1f878a..91a07353 100644 --- a/authority/authorize.go +++ b/authority/authorize.go @@ -6,6 +6,7 @@ import ( "crypto/x509" "encoding/hex" "net/http" + "strconv" "strings" "time" @@ -291,6 +292,19 @@ func (a *Authority) authorizeRenew(cert *x509.Certificate) error { return nil } +// authorizeSSHCertificate returns an error if the given certificate is revoked. +func (a *Authority) authorizeSSHCertificate(ctx context.Context, cert *ssh.Certificate) error { + serial := strconv.FormatUint(cert.Serial, 10) + isRevoked, err := a.db.IsSSHRevoked(serial) + if err != nil { + return errs.Wrap(http.StatusInternalServerError, err, "authority.authorizeSSHCertificate", errs.WithKeyVal("serialNumber", serial)) + } + if isRevoked { + return errs.Unauthorized("authority.authorizeSSHCertificate: certificate has been revoked", errs.WithKeyVal("serialNumber", serial)) + } + return nil +} + // authorizeSSHSign loads the provisioner from the token, checks that it has not // been used again and calls the provisioner AuthorizeSSHSign method. Returns a // list of methods to apply to the signing flow. diff --git a/authority/provisioner/sshpop.go b/authority/provisioner/sshpop.go index 8bc76edf..99974ff1 100644 --- a/authority/provisioner/sshpop.go +++ b/authority/provisioner/sshpop.go @@ -8,7 +8,6 @@ import ( "time" "github.com/pkg/errors" - "github.com/smallstep/certificates/db" "github.com/smallstep/certificates/errs" "go.step.sm/crypto/jose" "golang.org/x/crypto/ssh" @@ -30,7 +29,6 @@ type SSHPOP struct { Type string `json:"type"` Name string `json:"name"` Claims *Claims `json:"claims,omitempty"` - db db.AuthDB claimer *Claimer audiences Audiences sshPubKeys *SSHKeys @@ -102,7 +100,6 @@ func (p *SSHPOP) Init(config Config) error { } p.audiences = config.Audiences.WithFragment(p.GetIDForToken()) - p.db = config.DB p.sshPubKeys = config.SSHKeys return nil } @@ -110,6 +107,8 @@ func (p *SSHPOP) Init(config Config) error { // authorizeToken performs common jwt authorization actions and returns the // claims for case specific downstream parsing. // e.g. a Sign request will auth/validate different fields than a Revoke request. +// +// Checking for certificate revocation has been moved to the authority package. func (p *SSHPOP) authorizeToken(token string, audiences []string) (*sshPOPPayload, error) { sshCert, jwt, err := ExtractSSHPOPCert(token) if err != nil { @@ -117,14 +116,6 @@ func (p *SSHPOP) authorizeToken(token string, audiences []string) (*sshPOPPayloa "sshpop.authorizeToken; error extracting sshpop header from token") } - // Check for revocation. - if isRevoked, err := p.db.IsSSHRevoked(strconv.FormatUint(sshCert.Serial, 10)); err != nil { - return nil, errs.Wrap(http.StatusInternalServerError, err, - "sshpop.authorizeToken; error checking checking sshpop cert revocation") - } else if isRevoked { - return nil, errs.Unauthorized("sshpop.authorizeToken; sshpop certificate is revoked") - } - // Check validity period of the certificate. n := time.Now() if sshCert.ValidAfter != 0 && time.Unix(int64(sshCert.ValidAfter), 0).After(n) { diff --git a/authority/provisioner/sshpop_test.go b/authority/provisioner/sshpop_test.go index 5d51b90e..79d82e00 100644 --- a/authority/provisioner/sshpop_test.go +++ b/authority/provisioner/sshpop_test.go @@ -11,7 +11,6 @@ import ( "github.com/pkg/errors" "github.com/smallstep/assert" - "github.com/smallstep/certificates/db" "github.com/smallstep/certificates/errs" "go.step.sm/crypto/jose" "go.step.sm/crypto/pemutil" @@ -83,52 +82,9 @@ func TestSSHPOP_authorizeToken(t *testing.T) { err: errors.New("sshpop.authorizeToken; error extracting sshpop header from token: extractSSHPOPCert; error parsing token: "), } }, - "fail/error-revoked-db-check": func(t *testing.T) test { - p, err := generateSSHPOP() - assert.FatalError(t, err) - p.db = &db.MockAuthDB{ - MIsSSHRevoked: func(sn string) (bool, error) { - return false, errors.New("force") - }, - } - cert, jwk, err := createSSHCert(&ssh.Certificate{CertType: ssh.UserCert}, sshSigner) - assert.FatalError(t, err) - tok, err := generateSSHPOPToken(p, cert, jwk) - assert.FatalError(t, err) - return test{ - p: p, - token: tok, - code: http.StatusInternalServerError, - err: errors.New("sshpop.authorizeToken; error checking checking sshpop cert revocation: force"), - } - }, - "fail/cert-already-revoked": func(t *testing.T) test { - p, err := generateSSHPOP() - assert.FatalError(t, err) - p.db = &db.MockAuthDB{ - MIsSSHRevoked: func(sn string) (bool, error) { - return true, nil - }, - } - cert, jwk, err := createSSHCert(&ssh.Certificate{CertType: ssh.UserCert}, sshSigner) - assert.FatalError(t, err) - tok, err := generateSSHPOPToken(p, cert, jwk) - assert.FatalError(t, err) - return test{ - p: p, - token: tok, - code: http.StatusUnauthorized, - err: errors.New("sshpop.authorizeToken; sshpop certificate is revoked"), - } - }, "fail/cert-not-yet-valid": func(t *testing.T) test { p, err := generateSSHPOP() assert.FatalError(t, err) - p.db = &db.MockAuthDB{ - MIsSSHRevoked: func(sn string) (bool, error) { - return false, nil - }, - } cert, jwk, err := createSSHCert(&ssh.Certificate{ CertType: ssh.UserCert, ValidAfter: uint64(time.Now().Add(time.Minute).Unix()), @@ -146,11 +102,6 @@ func TestSSHPOP_authorizeToken(t *testing.T) { "fail/cert-past-validity": func(t *testing.T) test { p, err := generateSSHPOP() assert.FatalError(t, err) - p.db = &db.MockAuthDB{ - MIsSSHRevoked: func(sn string) (bool, error) { - return false, nil - }, - } cert, jwk, err := createSSHCert(&ssh.Certificate{ CertType: ssh.UserCert, ValidBefore: uint64(time.Now().Add(-time.Minute).Unix()), @@ -168,11 +119,6 @@ func TestSSHPOP_authorizeToken(t *testing.T) { "fail/no-signer-found": func(t *testing.T) test { p, err := generateSSHPOP() assert.FatalError(t, err) - p.db = &db.MockAuthDB{ - MIsSSHRevoked: func(sn string) (bool, error) { - return false, nil - }, - } cert, jwk, err := createSSHCert(&ssh.Certificate{CertType: ssh.HostCert}, sshSigner) assert.FatalError(t, err) tok, err := generateSSHPOPToken(p, cert, jwk) @@ -187,11 +133,6 @@ func TestSSHPOP_authorizeToken(t *testing.T) { "fail/error-parsing-claims-bad-sig": func(t *testing.T) test { p, err := generateSSHPOP() assert.FatalError(t, err) - p.db = &db.MockAuthDB{ - MIsSSHRevoked: func(sn string) (bool, error) { - return false, nil - }, - } cert, _, err := createSSHCert(&ssh.Certificate{CertType: ssh.UserCert}, sshSigner) assert.FatalError(t, err) otherJWK, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) @@ -208,11 +149,6 @@ func TestSSHPOP_authorizeToken(t *testing.T) { "fail/invalid-claims-issuer": func(t *testing.T) test { p, err := generateSSHPOP() assert.FatalError(t, err) - p.db = &db.MockAuthDB{ - MIsSSHRevoked: func(sn string) (bool, error) { - return false, nil - }, - } cert, jwk, err := createSSHCert(&ssh.Certificate{CertType: ssh.UserCert}, sshSigner) assert.FatalError(t, err) tok, err := generateToken("foo", "bar", testAudiences.Sign[0], "", @@ -228,11 +164,6 @@ func TestSSHPOP_authorizeToken(t *testing.T) { "fail/invalid-audience": func(t *testing.T) test { p, err := generateSSHPOP() assert.FatalError(t, err) - p.db = &db.MockAuthDB{ - MIsSSHRevoked: func(sn string) (bool, error) { - return false, nil - }, - } cert, jwk, err := createSSHCert(&ssh.Certificate{CertType: ssh.UserCert}, sshSigner) assert.FatalError(t, err) tok, err := generateToken("foo", p.GetName(), "invalid-aud", "", @@ -248,11 +179,6 @@ func TestSSHPOP_authorizeToken(t *testing.T) { "fail/empty-subject": func(t *testing.T) test { p, err := generateSSHPOP() assert.FatalError(t, err) - p.db = &db.MockAuthDB{ - MIsSSHRevoked: func(sn string) (bool, error) { - return false, nil - }, - } cert, jwk, err := createSSHCert(&ssh.Certificate{CertType: ssh.UserCert}, sshSigner) assert.FatalError(t, err) tok, err := generateToken("", p.GetName(), testAudiences.Sign[0], "", @@ -268,11 +194,6 @@ func TestSSHPOP_authorizeToken(t *testing.T) { "ok": func(t *testing.T) test { p, err := generateSSHPOP() assert.FatalError(t, err) - p.db = &db.MockAuthDB{ - MIsSSHRevoked: func(sn string) (bool, error) { - return false, nil - }, - } cert, jwk, err := createSSHCert(&ssh.Certificate{CertType: ssh.UserCert}, sshSigner) assert.FatalError(t, err) tok, err := generateSSHPOPToken(p, cert, jwk) @@ -330,11 +251,6 @@ func TestSSHPOP_AuthorizeSSHRevoke(t *testing.T) { "fail/subject-not-equal-serial": func(t *testing.T) test { p, err := generateSSHPOP() assert.FatalError(t, err) - p.db = &db.MockAuthDB{ - MIsSSHRevoked: func(sn string) (bool, error) { - return false, nil - }, - } cert, jwk, err := createSSHCert(&ssh.Certificate{CertType: ssh.UserCert}, sshSigner) assert.FatalError(t, err) tok, err := generateToken("foo", p.GetName(), testAudiences.SSHRevoke[0], "", @@ -350,11 +266,6 @@ func TestSSHPOP_AuthorizeSSHRevoke(t *testing.T) { "ok": func(t *testing.T) test { p, err := generateSSHPOP() assert.FatalError(t, err) - p.db = &db.MockAuthDB{ - MIsSSHRevoked: func(sn string) (bool, error) { - return false, nil - }, - } cert, jwk, err := createSSHCert(&ssh.Certificate{Serial: 123455, CertType: ssh.UserCert}, sshSigner) assert.FatalError(t, err) tok, err := generateToken("123455", p.GetName(), testAudiences.SSHRevoke[0], "", @@ -419,11 +330,6 @@ func TestSSHPOP_AuthorizeSSHRenew(t *testing.T) { "fail/not-host-cert": func(t *testing.T) test { p, err := generateSSHPOP() assert.FatalError(t, err) - p.db = &db.MockAuthDB{ - MIsSSHRevoked: func(sn string) (bool, error) { - return false, nil - }, - } cert, jwk, err := createSSHCert(&ssh.Certificate{CertType: ssh.UserCert}, sshUserSigner) assert.FatalError(t, err) tok, err := generateToken("foo", p.GetName(), testAudiences.SSHRenew[0], "", @@ -439,11 +345,6 @@ func TestSSHPOP_AuthorizeSSHRenew(t *testing.T) { "ok": func(t *testing.T) test { p, err := generateSSHPOP() assert.FatalError(t, err) - p.db = &db.MockAuthDB{ - MIsSSHRevoked: func(sn string) (bool, error) { - return false, nil - }, - } cert, jwk, err := createSSHCert(&ssh.Certificate{Serial: 123455, CertType: ssh.HostCert}, sshHostSigner) assert.FatalError(t, err) tok, err := generateToken("123455", p.GetName(), testAudiences.SSHRenew[0], "", @@ -511,11 +412,6 @@ func TestSSHPOP_AuthorizeSSHRekey(t *testing.T) { "fail/not-host-cert": func(t *testing.T) test { p, err := generateSSHPOP() assert.FatalError(t, err) - p.db = &db.MockAuthDB{ - MIsSSHRevoked: func(sn string) (bool, error) { - return false, nil - }, - } cert, jwk, err := createSSHCert(&ssh.Certificate{CertType: ssh.UserCert}, sshUserSigner) assert.FatalError(t, err) tok, err := generateToken("foo", p.GetName(), testAudiences.SSHRekey[0], "", @@ -531,11 +427,6 @@ func TestSSHPOP_AuthorizeSSHRekey(t *testing.T) { "ok": func(t *testing.T) test { p, err := generateSSHPOP() assert.FatalError(t, err) - p.db = &db.MockAuthDB{ - MIsSSHRevoked: func(sn string) (bool, error) { - return false, nil - }, - } cert, jwk, err := createSSHCert(&ssh.Certificate{Serial: 123455, CertType: ssh.HostCert}, sshHostSigner) assert.FatalError(t, err) tok, err := generateToken("123455", p.GetName(), testAudiences.SSHRekey[0], "", diff --git a/authority/ssh.go b/authority/ssh.go index 3b03fd7e..1c873279 100644 --- a/authority/ssh.go +++ b/authority/ssh.go @@ -249,7 +249,11 @@ func (a *Authority) SignSSH(ctx context.Context, key ssh.PublicKey, opts provisi // RenewSSH creates a signed SSH certificate using the old SSH certificate as a template. func (a *Authority) RenewSSH(ctx context.Context, oldCert *ssh.Certificate) (*ssh.Certificate, error) { if oldCert.ValidAfter == 0 || oldCert.ValidBefore == 0 { - return nil, errs.BadRequest("rewnewSSH: cannot renew certificate without validity period") + return nil, errs.BadRequest("renewSSH: cannot renew certificate without validity period") + } + + if err := a.authorizeSSHCertificate(ctx, oldCert); err != nil { + return nil, err } backdate := a.config.AuthorityConfig.Backdate.Duration @@ -319,6 +323,10 @@ func (a *Authority) RekeySSH(ctx context.Context, oldCert *ssh.Certificate, pub return nil, errs.BadRequest("rekeySSH; cannot rekey certificate without validity period") } + if err := a.authorizeSSHCertificate(ctx, oldCert); err != nil { + return nil, err + } + backdate := a.config.AuthorityConfig.Backdate.Duration duration := time.Duration(oldCert.ValidBefore-oldCert.ValidAfter) * time.Second now := time.Now() diff --git a/authority/ssh_test.go b/authority/ssh_test.go index 8ca26af0..e468ecf0 100644 --- a/authority/ssh_test.go +++ b/authority/ssh_test.go @@ -750,6 +750,11 @@ func TestAuthority_RekeySSH(t *testing.T) { now := time.Now().UTC() a := testAuthority(t) + a.db = &db.MockAuthDB{ + MIsSSHRevoked: func(sn string) (bool, error) { + return false, nil + }, + } type test struct { auth *Authority @@ -763,6 +768,56 @@ func TestAuthority_RekeySSH(t *testing.T) { code int } tests := map[string]func(t *testing.T) *test{ + "fail/is-revoked": func(t *testing.T) *test { + auth := testAuthority(t) + auth.db = &db.MockAuthDB{ + MIsSSHRevoked: func(sn string) (bool, error) { + return true, nil + }, + } + return &test{ + auth: auth, + userSigner: signer, + hostSigner: signer, + cert: &ssh.Certificate{ + Serial: 1234567890, + ValidAfter: uint64(now.Unix()), + ValidBefore: uint64(now.Add(time.Hour).Unix()), + CertType: ssh.UserCert, + ValidPrincipals: []string{"foo", "bar"}, + KeyId: "foo", + }, + key: pub, + signOpts: []provisioner.SignOption{}, + err: errors.New("authority.authorizeSSHCertificate: certificate has been revoked"), + code: http.StatusUnauthorized, + } + }, + "fail/is-revoked-error": func(t *testing.T) *test { + auth := testAuthority(t) + auth.db = &db.MockAuthDB{ + MIsSSHRevoked: func(sn string) (bool, error) { + return false, errors.New("an error") + }, + } + return &test{ + auth: auth, + userSigner: signer, + hostSigner: signer, + cert: &ssh.Certificate{ + Serial: 1234567890, + ValidAfter: uint64(now.Unix()), + ValidBefore: uint64(now.Add(time.Hour).Unix()), + CertType: ssh.UserCert, + ValidPrincipals: []string{"foo", "bar"}, + KeyId: "foo", + }, + key: pub, + signOpts: []provisioner.SignOption{}, + err: errors.New("authority.authorizeSSHCertificate: an error"), + code: http.StatusInternalServerError, + } + }, "fail/opts-type": func(t *testing.T) *test { return &test{ userSigner: signer, @@ -831,6 +886,9 @@ func TestAuthority_RekeySSH(t *testing.T) { "fail/db-store": func(t *testing.T) *test { return &test{ auth: testAuthority(t, WithDatabase(&db.MockAuthDB{ + MIsSSHRevoked: func(sn string) (bool, error) { + return false, nil + }, MStoreSSHCertificate: func(cert *ssh.Certificate) error { return errors.New("force") },