Skip to content

Commit f7542a5

Browse files
marainomaraino
committed
Move check of ssh revocation from provisioner to the authority.
1 parent 71f8019 commit f7542a5

File tree

5 files changed

+83
-121
lines changed

5 files changed

+83
-121
lines changed

Diff for: ‎authority/authorize.go

+14
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"crypto/x509"
77
"encoding/hex"
88
"net/http"
9+
"strconv"
910
"strings"
1011
"time"
1112

@@ -291,6 +292,19 @@ func (a *Authority) authorizeRenew(cert *x509.Certificate) error {
291292
return nil
292293
}
293294

295+
// authorizeSSHCertificate returns an error if the given certificate is revoked.
296+
func (a *Authority) authorizeSSHCertificate(ctx context.Context, cert *ssh.Certificate) error {
297+
serial := strconv.FormatUint(cert.Serial, 10)
298+
isRevoked, err := a.db.IsSSHRevoked(serial)
299+
if err != nil {
300+
return errs.Wrap(http.StatusInternalServerError, err, "authority.authorizeSSHCertificate", errs.WithKeyVal("serialNumber", serial))
301+
}
302+
if isRevoked {
303+
return errs.Unauthorized("authority.authorizeSSHCertificate: certificate has been revoked", errs.WithKeyVal("serialNumber", serial))
304+
}
305+
return nil
306+
}
307+
294308
// authorizeSSHSign loads the provisioner from the token, checks that it has not
295309
// been used again and calls the provisioner AuthorizeSSHSign method. Returns a
296310
// list of methods to apply to the signing flow.

Diff for: ‎authority/provisioner/sshpop.go

+2-11
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import (
88
"time"
99

1010
"github.com/pkg/errors"
11-
"github.com/smallstep/certificates/db"
1211
"github.com/smallstep/certificates/errs"
1312
"go.step.sm/crypto/jose"
1413
"golang.org/x/crypto/ssh"
@@ -30,7 +29,6 @@ type SSHPOP struct {
3029
Type string `json:"type"`
3130
Name string `json:"name"`
3231
Claims *Claims `json:"claims,omitempty"`
33-
db db.AuthDB
3432
claimer *Claimer
3533
audiences Audiences
3634
sshPubKeys *SSHKeys
@@ -102,29 +100,22 @@ func (p *SSHPOP) Init(config Config) error {
102100
}
103101

104102
p.audiences = config.Audiences.WithFragment(p.GetIDForToken())
105-
p.db = config.DB
106103
p.sshPubKeys = config.SSHKeys
107104
return nil
108105
}
109106

110107
// authorizeToken performs common jwt authorization actions and returns the
111108
// claims for case specific downstream parsing.
112109
// e.g. a Sign request will auth/validate different fields than a Revoke request.
110+
//
111+
// Checking for certificate revocation has been moved to the authority package.
113112
func (p *SSHPOP) authorizeToken(token string, audiences []string) (*sshPOPPayload, error) {
114113
sshCert, jwt, err := ExtractSSHPOPCert(token)
115114
if err != nil {
116115
return nil, errs.Wrap(http.StatusUnauthorized, err,
117116
"sshpop.authorizeToken; error extracting sshpop header from token")
118117
}
119118

120-
// Check for revocation.
121-
if isRevoked, err := p.db.IsSSHRevoked(strconv.FormatUint(sshCert.Serial, 10)); err != nil {
122-
return nil, errs.Wrap(http.StatusInternalServerError, err,
123-
"sshpop.authorizeToken; error checking checking sshpop cert revocation")
124-
} else if isRevoked {
125-
return nil, errs.Unauthorized("sshpop.authorizeToken; sshpop certificate is revoked")
126-
}
127-
128119
// Check validity period of the certificate.
129120
n := time.Now()
130121
if sshCert.ValidAfter != 0 && time.Unix(int64(sshCert.ValidAfter), 0).After(n) {

Diff for: ‎authority/provisioner/sshpop_test.go

-109
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import (
1111

1212
"github.com/pkg/errors"
1313
"github.com/smallstep/assert"
14-
"github.com/smallstep/certificates/db"
1514
"github.com/smallstep/certificates/errs"
1615
"go.step.sm/crypto/jose"
1716
"go.step.sm/crypto/pemutil"
@@ -83,52 +82,9 @@ func TestSSHPOP_authorizeToken(t *testing.T) {
8382
err: errors.New("sshpop.authorizeToken; error extracting sshpop header from token: extractSSHPOPCert; error parsing token: "),
8483
}
8584
},
86-
"fail/error-revoked-db-check": func(t *testing.T) test {
87-
p, err := generateSSHPOP()
88-
assert.FatalError(t, err)
89-
p.db = &db.MockAuthDB{
90-
MIsSSHRevoked: func(sn string) (bool, error) {
91-
return false, errors.New("force")
92-
},
93-
}
94-
cert, jwk, err := createSSHCert(&ssh.Certificate{CertType: ssh.UserCert}, sshSigner)
95-
assert.FatalError(t, err)
96-
tok, err := generateSSHPOPToken(p, cert, jwk)
97-
assert.FatalError(t, err)
98-
return test{
99-
p: p,
100-
token: tok,
101-
code: http.StatusInternalServerError,
102-
err: errors.New("sshpop.authorizeToken; error checking checking sshpop cert revocation: force"),
103-
}
104-
},
105-
"fail/cert-already-revoked": func(t *testing.T) test {
106-
p, err := generateSSHPOP()
107-
assert.FatalError(t, err)
108-
p.db = &db.MockAuthDB{
109-
MIsSSHRevoked: func(sn string) (bool, error) {
110-
return true, nil
111-
},
112-
}
113-
cert, jwk, err := createSSHCert(&ssh.Certificate{CertType: ssh.UserCert}, sshSigner)
114-
assert.FatalError(t, err)
115-
tok, err := generateSSHPOPToken(p, cert, jwk)
116-
assert.FatalError(t, err)
117-
return test{
118-
p: p,
119-
token: tok,
120-
code: http.StatusUnauthorized,
121-
err: errors.New("sshpop.authorizeToken; sshpop certificate is revoked"),
122-
}
123-
},
12485
"fail/cert-not-yet-valid": func(t *testing.T) test {
12586
p, err := generateSSHPOP()
12687
assert.FatalError(t, err)
127-
p.db = &db.MockAuthDB{
128-
MIsSSHRevoked: func(sn string) (bool, error) {
129-
return false, nil
130-
},
131-
}
13288
cert, jwk, err := createSSHCert(&ssh.Certificate{
13389
CertType: ssh.UserCert,
13490
ValidAfter: uint64(time.Now().Add(time.Minute).Unix()),
@@ -146,11 +102,6 @@ func TestSSHPOP_authorizeToken(t *testing.T) {
146102
"fail/cert-past-validity": func(t *testing.T) test {
147103
p, err := generateSSHPOP()
148104
assert.FatalError(t, err)
149-
p.db = &db.MockAuthDB{
150-
MIsSSHRevoked: func(sn string) (bool, error) {
151-
return false, nil
152-
},
153-
}
154105
cert, jwk, err := createSSHCert(&ssh.Certificate{
155106
CertType: ssh.UserCert,
156107
ValidBefore: uint64(time.Now().Add(-time.Minute).Unix()),
@@ -168,11 +119,6 @@ func TestSSHPOP_authorizeToken(t *testing.T) {
168119
"fail/no-signer-found": func(t *testing.T) test {
169120
p, err := generateSSHPOP()
170121
assert.FatalError(t, err)
171-
p.db = &db.MockAuthDB{
172-
MIsSSHRevoked: func(sn string) (bool, error) {
173-
return false, nil
174-
},
175-
}
176122
cert, jwk, err := createSSHCert(&ssh.Certificate{CertType: ssh.HostCert}, sshSigner)
177123
assert.FatalError(t, err)
178124
tok, err := generateSSHPOPToken(p, cert, jwk)
@@ -187,11 +133,6 @@ func TestSSHPOP_authorizeToken(t *testing.T) {
187133
"fail/error-parsing-claims-bad-sig": func(t *testing.T) test {
188134
p, err := generateSSHPOP()
189135
assert.FatalError(t, err)
190-
p.db = &db.MockAuthDB{
191-
MIsSSHRevoked: func(sn string) (bool, error) {
192-
return false, nil
193-
},
194-
}
195136
cert, _, err := createSSHCert(&ssh.Certificate{CertType: ssh.UserCert}, sshSigner)
196137
assert.FatalError(t, err)
197138
otherJWK, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0)
@@ -208,11 +149,6 @@ func TestSSHPOP_authorizeToken(t *testing.T) {
208149
"fail/invalid-claims-issuer": func(t *testing.T) test {
209150
p, err := generateSSHPOP()
210151
assert.FatalError(t, err)
211-
p.db = &db.MockAuthDB{
212-
MIsSSHRevoked: func(sn string) (bool, error) {
213-
return false, nil
214-
},
215-
}
216152
cert, jwk, err := createSSHCert(&ssh.Certificate{CertType: ssh.UserCert}, sshSigner)
217153
assert.FatalError(t, err)
218154
tok, err := generateToken("foo", "bar", testAudiences.Sign[0], "",
@@ -228,11 +164,6 @@ func TestSSHPOP_authorizeToken(t *testing.T) {
228164
"fail/invalid-audience": func(t *testing.T) test {
229165
p, err := generateSSHPOP()
230166
assert.FatalError(t, err)
231-
p.db = &db.MockAuthDB{
232-
MIsSSHRevoked: func(sn string) (bool, error) {
233-
return false, nil
234-
},
235-
}
236167
cert, jwk, err := createSSHCert(&ssh.Certificate{CertType: ssh.UserCert}, sshSigner)
237168
assert.FatalError(t, err)
238169
tok, err := generateToken("foo", p.GetName(), "invalid-aud", "",
@@ -248,11 +179,6 @@ func TestSSHPOP_authorizeToken(t *testing.T) {
248179
"fail/empty-subject": func(t *testing.T) test {
249180
p, err := generateSSHPOP()
250181
assert.FatalError(t, err)
251-
p.db = &db.MockAuthDB{
252-
MIsSSHRevoked: func(sn string) (bool, error) {
253-
return false, nil
254-
},
255-
}
256182
cert, jwk, err := createSSHCert(&ssh.Certificate{CertType: ssh.UserCert}, sshSigner)
257183
assert.FatalError(t, err)
258184
tok, err := generateToken("", p.GetName(), testAudiences.Sign[0], "",
@@ -268,11 +194,6 @@ func TestSSHPOP_authorizeToken(t *testing.T) {
268194
"ok": func(t *testing.T) test {
269195
p, err := generateSSHPOP()
270196
assert.FatalError(t, err)
271-
p.db = &db.MockAuthDB{
272-
MIsSSHRevoked: func(sn string) (bool, error) {
273-
return false, nil
274-
},
275-
}
276197
cert, jwk, err := createSSHCert(&ssh.Certificate{CertType: ssh.UserCert}, sshSigner)
277198
assert.FatalError(t, err)
278199
tok, err := generateSSHPOPToken(p, cert, jwk)
@@ -330,11 +251,6 @@ func TestSSHPOP_AuthorizeSSHRevoke(t *testing.T) {
330251
"fail/subject-not-equal-serial": func(t *testing.T) test {
331252
p, err := generateSSHPOP()
332253
assert.FatalError(t, err)
333-
p.db = &db.MockAuthDB{
334-
MIsSSHRevoked: func(sn string) (bool, error) {
335-
return false, nil
336-
},
337-
}
338254
cert, jwk, err := createSSHCert(&ssh.Certificate{CertType: ssh.UserCert}, sshSigner)
339255
assert.FatalError(t, err)
340256
tok, err := generateToken("foo", p.GetName(), testAudiences.SSHRevoke[0], "",
@@ -350,11 +266,6 @@ func TestSSHPOP_AuthorizeSSHRevoke(t *testing.T) {
350266
"ok": func(t *testing.T) test {
351267
p, err := generateSSHPOP()
352268
assert.FatalError(t, err)
353-
p.db = &db.MockAuthDB{
354-
MIsSSHRevoked: func(sn string) (bool, error) {
355-
return false, nil
356-
},
357-
}
358269
cert, jwk, err := createSSHCert(&ssh.Certificate{Serial: 123455, CertType: ssh.UserCert}, sshSigner)
359270
assert.FatalError(t, err)
360271
tok, err := generateToken("123455", p.GetName(), testAudiences.SSHRevoke[0], "",
@@ -419,11 +330,6 @@ func TestSSHPOP_AuthorizeSSHRenew(t *testing.T) {
419330
"fail/not-host-cert": func(t *testing.T) test {
420331
p, err := generateSSHPOP()
421332
assert.FatalError(t, err)
422-
p.db = &db.MockAuthDB{
423-
MIsSSHRevoked: func(sn string) (bool, error) {
424-
return false, nil
425-
},
426-
}
427333
cert, jwk, err := createSSHCert(&ssh.Certificate{CertType: ssh.UserCert}, sshUserSigner)
428334
assert.FatalError(t, err)
429335
tok, err := generateToken("foo", p.GetName(), testAudiences.SSHRenew[0], "",
@@ -439,11 +345,6 @@ func TestSSHPOP_AuthorizeSSHRenew(t *testing.T) {
439345
"ok": func(t *testing.T) test {
440346
p, err := generateSSHPOP()
441347
assert.FatalError(t, err)
442-
p.db = &db.MockAuthDB{
443-
MIsSSHRevoked: func(sn string) (bool, error) {
444-
return false, nil
445-
},
446-
}
447348
cert, jwk, err := createSSHCert(&ssh.Certificate{Serial: 123455, CertType: ssh.HostCert}, sshHostSigner)
448349
assert.FatalError(t, err)
449350
tok, err := generateToken("123455", p.GetName(), testAudiences.SSHRenew[0], "",
@@ -511,11 +412,6 @@ func TestSSHPOP_AuthorizeSSHRekey(t *testing.T) {
511412
"fail/not-host-cert": func(t *testing.T) test {
512413
p, err := generateSSHPOP()
513414
assert.FatalError(t, err)
514-
p.db = &db.MockAuthDB{
515-
MIsSSHRevoked: func(sn string) (bool, error) {
516-
return false, nil
517-
},
518-
}
519415
cert, jwk, err := createSSHCert(&ssh.Certificate{CertType: ssh.UserCert}, sshUserSigner)
520416
assert.FatalError(t, err)
521417
tok, err := generateToken("foo", p.GetName(), testAudiences.SSHRekey[0], "",
@@ -531,11 +427,6 @@ func TestSSHPOP_AuthorizeSSHRekey(t *testing.T) {
531427
"ok": func(t *testing.T) test {
532428
p, err := generateSSHPOP()
533429
assert.FatalError(t, err)
534-
p.db = &db.MockAuthDB{
535-
MIsSSHRevoked: func(sn string) (bool, error) {
536-
return false, nil
537-
},
538-
}
539430
cert, jwk, err := createSSHCert(&ssh.Certificate{Serial: 123455, CertType: ssh.HostCert}, sshHostSigner)
540431
assert.FatalError(t, err)
541432
tok, err := generateToken("123455", p.GetName(), testAudiences.SSHRekey[0], "",

Diff for: ‎authority/ssh.go

+9-1
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,11 @@ func (a *Authority) SignSSH(ctx context.Context, key ssh.PublicKey, opts provisi
249249
// RenewSSH creates a signed SSH certificate using the old SSH certificate as a template.
250250
func (a *Authority) RenewSSH(ctx context.Context, oldCert *ssh.Certificate) (*ssh.Certificate, error) {
251251
if oldCert.ValidAfter == 0 || oldCert.ValidBefore == 0 {
252-
return nil, errs.BadRequest("rewnewSSH: cannot renew certificate without validity period")
252+
return nil, errs.BadRequest("renewSSH: cannot renew certificate without validity period")
253+
}
254+
255+
if err := a.authorizeSSHCertificate(ctx, oldCert); err != nil {
256+
return nil, err
253257
}
254258

255259
backdate := a.config.AuthorityConfig.Backdate.Duration
@@ -319,6 +323,10 @@ func (a *Authority) RekeySSH(ctx context.Context, oldCert *ssh.Certificate, pub
319323
return nil, errs.BadRequest("rekeySSH; cannot rekey certificate without validity period")
320324
}
321325

326+
if err := a.authorizeSSHCertificate(ctx, oldCert); err != nil {
327+
return nil, err
328+
}
329+
322330
backdate := a.config.AuthorityConfig.Backdate.Duration
323331
duration := time.Duration(oldCert.ValidBefore-oldCert.ValidAfter) * time.Second
324332
now := time.Now()

Diff for: ‎authority/ssh_test.go

+58
Original file line numberDiff line numberDiff line change
@@ -750,6 +750,11 @@ func TestAuthority_RekeySSH(t *testing.T) {
750750
now := time.Now().UTC()
751751

752752
a := testAuthority(t)
753+
a.db = &db.MockAuthDB{
754+
MIsSSHRevoked: func(sn string) (bool, error) {
755+
return false, nil
756+
},
757+
}
753758

754759
type test struct {
755760
auth *Authority
@@ -763,6 +768,56 @@ func TestAuthority_RekeySSH(t *testing.T) {
763768
code int
764769
}
765770
tests := map[string]func(t *testing.T) *test{
771+
"fail/is-revoked": func(t *testing.T) *test {
772+
auth := testAuthority(t)
773+
auth.db = &db.MockAuthDB{
774+
MIsSSHRevoked: func(sn string) (bool, error) {
775+
return true, nil
776+
},
777+
}
778+
return &test{
779+
auth: auth,
780+
userSigner: signer,
781+
hostSigner: signer,
782+
cert: &ssh.Certificate{
783+
Serial: 1234567890,
784+
ValidAfter: uint64(now.Unix()),
785+
ValidBefore: uint64(now.Add(time.Hour).Unix()),
786+
CertType: ssh.UserCert,
787+
ValidPrincipals: []string{"foo", "bar"},
788+
KeyId: "foo",
789+
},
790+
key: pub,
791+
signOpts: []provisioner.SignOption{},
792+
err: errors.New("authority.authorizeSSHCertificate: certificate has been revoked"),
793+
code: http.StatusUnauthorized,
794+
}
795+
},
796+
"fail/is-revoked-error": func(t *testing.T) *test {
797+
auth := testAuthority(t)
798+
auth.db = &db.MockAuthDB{
799+
MIsSSHRevoked: func(sn string) (bool, error) {
800+
return false, errors.New("an error")
801+
},
802+
}
803+
return &test{
804+
auth: auth,
805+
userSigner: signer,
806+
hostSigner: signer,
807+
cert: &ssh.Certificate{
808+
Serial: 1234567890,
809+
ValidAfter: uint64(now.Unix()),
810+
ValidBefore: uint64(now.Add(time.Hour).Unix()),
811+
CertType: ssh.UserCert,
812+
ValidPrincipals: []string{"foo", "bar"},
813+
KeyId: "foo",
814+
},
815+
key: pub,
816+
signOpts: []provisioner.SignOption{},
817+
err: errors.New("authority.authorizeSSHCertificate: an error"),
818+
code: http.StatusInternalServerError,
819+
}
820+
},
766821
"fail/opts-type": func(t *testing.T) *test {
767822
return &test{
768823
userSigner: signer,
@@ -831,6 +886,9 @@ func TestAuthority_RekeySSH(t *testing.T) {
831886
"fail/db-store": func(t *testing.T) *test {
832887
return &test{
833888
auth: testAuthority(t, WithDatabase(&db.MockAuthDB{
889+
MIsSSHRevoked: func(sn string) (bool, error) {
890+
return false, nil
891+
},
834892
MStoreSSHCertificate: func(cert *ssh.Certificate) error {
835893
return errors.New("force")
836894
},

0 commit comments

Comments
 (0)