From 1e5763031b236a0cdb0e81a7b56e691bb71b4061 Mon Sep 17 00:00:00 2001 From: max furman Date: Fri, 24 Jan 2020 13:42:00 -0800 Subject: [PATCH] Add backdate validation to sshCertValidityValidator. --- authority/provisioner/sign_options.go | 2 +- authority/provisioner/sign_ssh_options.go | 21 +++++------ .../provisioner/sign_ssh_options_test.go | 35 +++++++++++++++++-- authority/provisioner/ssh_test.go | 2 +- authority/ssh.go | 6 ++-- authority/ssh_test.go | 2 +- authority/tls_test.go | 2 +- 7 files changed, 47 insertions(+), 23 deletions(-) diff --git a/authority/provisioner/sign_options.go b/authority/provisioner/sign_options.go index ed049b6c..90c2cd40 100644 --- a/authority/provisioner/sign_options.go +++ b/authority/provisioner/sign_options.go @@ -290,7 +290,7 @@ func (v *validityValidator) Valid(cert *x509.Certificate, o Options) error { // apply a backdate). This is good enough. if d > v.max+o.Backdate { return errors.Errorf("requested duration of %v is more than the authorized maximum certificate duration of %v", - d, v.max) + d, v.max+o.Backdate) } return nil } diff --git a/authority/provisioner/sign_ssh_options.go b/authority/provisioner/sign_ssh_options.go index b0ab78ea..34bc069f 100644 --- a/authority/provisioner/sign_ssh_options.go +++ b/authority/provisioner/sign_ssh_options.go @@ -36,7 +36,7 @@ type SSHCertOptionModifier interface { // SSHCertValidator is the interface used to validate an SSH certificate. type SSHCertValidator interface { SignOption - Valid(cert *ssh.Certificate) error + Valid(cert *ssh.Certificate, opts SSHOptions) error } // SSHCertOptionsValidator is the interface used to validate the custom @@ -310,7 +310,7 @@ type sshCertValidityValidator struct { *Claimer } -func (v *sshCertValidityValidator) Valid(cert *ssh.Certificate) error { +func (v *sshCertValidityValidator) Valid(cert *ssh.Certificate, opts SSHOptions) error { switch { case cert.ValidAfter == 0: return errors.New("ssh certificate validAfter cannot be 0") @@ -336,20 +336,15 @@ func (v *sshCertValidityValidator) Valid(cert *ssh.Certificate) error { // To not take into account the backdate, time.Now() will be used to // calculate the duration if ValidAfter is in the past. - var dur time.Duration - if t := now().Unix(); t > int64(cert.ValidAfter) { - dur = time.Duration(int64(cert.ValidBefore)-t) * time.Second - } else { - dur = time.Duration(cert.ValidBefore-cert.ValidAfter) * time.Second - } + dur := time.Duration(cert.ValidBefore-cert.ValidAfter) * time.Second switch { case dur < min: return errors.Errorf("requested duration of %s is less than minimum "+ "accepted duration for selected provisioner of %s", dur, min) - case dur > max: + case dur > max+opts.Backdate: return errors.Errorf("requested duration of %s is greater than maximum "+ - "accepted duration for selected provisioner of %s", dur, max) + "accepted duration for selected provisioner of %s", dur, max+opts.Backdate) default: return nil } @@ -360,7 +355,7 @@ func (v *sshCertValidityValidator) Valid(cert *ssh.Certificate) error { type sshCertDefaultValidator struct{} // Valid returns an error if the given certificate does not contain the necessary fields. -func (v *sshCertDefaultValidator) Valid(cert *ssh.Certificate) error { +func (v *sshCertDefaultValidator) Valid(cert *ssh.Certificate, o SSHOptions) error { switch { case len(cert.Nonce) == 0: return errors.New("ssh certificate nonce cannot be empty") @@ -395,7 +390,7 @@ func (v *sshCertDefaultValidator) Valid(cert *ssh.Certificate) error { type sshDefaultPublicKeyValidator struct{} // Valid checks that certificate request common name matches the one configured. -func (v sshDefaultPublicKeyValidator) Valid(cert *ssh.Certificate) error { +func (v sshDefaultPublicKeyValidator) Valid(cert *ssh.Certificate, o SSHOptions) error { if cert.Key == nil { return errors.New("ssh certificate key cannot be nil") } @@ -425,7 +420,7 @@ func (v sshDefaultPublicKeyValidator) Valid(cert *ssh.Certificate) error { type sshCertKeyIDValidator string // Valid returns an error if the given certificate does not contain the necessary fields. -func (v sshCertKeyIDValidator) Valid(cert *ssh.Certificate) error { +func (v sshCertKeyIDValidator) Valid(cert *ssh.Certificate, o SSHOptions) error { if string(v) != cert.KeyId { return errors.Errorf("invalid ssh certificate KeyId; want %s, but got %s", string(v), cert.KeyId) } diff --git a/authority/provisioner/sign_ssh_options_test.go b/authority/provisioner/sign_ssh_options_test.go index c13e46da..940fc0e2 100644 --- a/authority/provisioner/sign_ssh_options_test.go +++ b/authority/provisioner/sign_ssh_options_test.go @@ -659,7 +659,7 @@ func Test_sshCertDefaultValidator_Valid(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if err := v.Valid(tt.cert); err != nil { + if err := v.Valid(tt.cert, SSHOptions{}); err != nil { if assert.NotNil(t, tt.err) { assert.HasPrefix(t, err.Error(), tt.err.Error()) } @@ -678,26 +678,31 @@ func Test_sshCertValidityValidator(t *testing.T) { tests := []struct { name string cert *ssh.Certificate + opts SSHOptions err error }{ { "fail/validAfter-0", &ssh.Certificate{CertType: ssh.UserCert}, + SSHOptions{}, errors.New("ssh certificate validAfter cannot be 0"), }, { "fail/validBefore-in-past", &ssh.Certificate{CertType: ssh.UserCert, ValidAfter: uint64(now().Unix()), ValidBefore: uint64(now().Add(-time.Minute).Unix())}, + SSHOptions{}, errors.New("ssh certificate validBefore cannot be in the past"), }, { "fail/validBefore-before-validAfter", &ssh.Certificate{CertType: ssh.UserCert, ValidAfter: uint64(now().Add(5 * time.Minute).Unix()), ValidBefore: uint64(now().Add(3 * time.Minute).Unix())}, + SSHOptions{}, errors.New("ssh certificate validBefore cannot be before validAfter"), }, { "fail/cert-type-not-set", &ssh.Certificate{ValidAfter: uint64(now().Unix()), ValidBefore: uint64(now().Add(10 * time.Minute).Unix())}, + SSHOptions{}, errors.New("ssh certificate type has not been set"), }, { @@ -707,6 +712,7 @@ func Test_sshCertValidityValidator(t *testing.T) { ValidAfter: uint64(now().Unix()), ValidBefore: uint64(now().Add(10 * time.Minute).Unix()), }, + SSHOptions{}, errors.New("unknown ssh certificate type 3"), }, { @@ -716,8 +722,19 @@ func Test_sshCertValidityValidator(t *testing.T) { ValidAfter: uint64(n.Unix()), ValidBefore: uint64(n.Add(4 * time.Minute).Unix()), }, + SSHOptions{Backdate: time.Second}, errors.New("requested duration of 4m0s is less than minimum accepted duration for selected provisioner of 5m0s"), }, + { + "ok/duration-exactly-min", + &ssh.Certificate{ + CertType: 1, + ValidAfter: uint64(n.Unix()), + ValidBefore: uint64(n.Add(5 * time.Minute).Unix()), + }, + SSHOptions{Backdate: time.Second}, + nil, + }, { "fail/duration>max", &ssh.Certificate{ @@ -725,7 +742,18 @@ func Test_sshCertValidityValidator(t *testing.T) { ValidAfter: uint64(n.Unix()), ValidBefore: uint64(n.Add(48 * time.Hour).Unix()), }, - errors.New("requested duration of 48h0m0s is greater than maximum accepted duration for selected provisioner of 24h0m0s"), + SSHOptions{Backdate: time.Second}, + errors.New("requested duration of 48h0m0s is greater than maximum accepted duration for selected provisioner of 24h0m1s"), + }, + { + "ok/duration-exactly-max", + &ssh.Certificate{ + CertType: 1, + ValidAfter: uint64(n.Unix()), + ValidBefore: uint64(n.Add(24*time.Hour + time.Second).Unix()), + }, + SSHOptions{Backdate: time.Second}, + nil, }, { "ok", @@ -734,12 +762,13 @@ func Test_sshCertValidityValidator(t *testing.T) { ValidAfter: uint64(now().Unix()), ValidBefore: uint64(now().Add(8 * time.Hour).Unix()), }, + SSHOptions{Backdate: time.Second}, nil, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if err := v.Valid(tt.cert); err != nil { + if err := v.Valid(tt.cert, tt.opts); err != nil { if assert.NotNil(t, tt.err) { assert.HasPrefix(t, err.Error(), tt.err.Error()) } diff --git a/authority/provisioner/ssh_test.go b/authority/provisioner/ssh_test.go index 84860a75..be102a1a 100644 --- a/authority/provisioner/ssh_test.go +++ b/authority/provisioner/ssh_test.go @@ -116,7 +116,7 @@ func signSSHCertificate(key crypto.PublicKey, opts SSHOptions, signOpts []SignOp // User provisioners validators for _, v := range validators { - if err := v.Valid(cert); err != nil { + if err := v.Valid(cert, opts); err != nil { return nil, err } } diff --git a/authority/ssh.go b/authority/ssh.go index 28066556..f47447d5 100644 --- a/authority/ssh.go +++ b/authority/ssh.go @@ -269,7 +269,7 @@ func (a *Authority) SignSSH(key ssh.PublicKey, opts provisioner.SSHOptions, sign // User provisioners validators for _, v := range validators { - if err := v.Valid(cert); err != nil { + if err := v.Valid(cert, opts); err != nil { return nil, errs.Wrap(http.StatusForbidden, err, "signSSH") } } @@ -428,9 +428,9 @@ func (a *Authority) RekeySSH(oldCert *ssh.Certificate, pub ssh.PublicKey, signOp } cert.Signature = sig - // Apply validators from provisioner.. + // Apply validators from provisioner. for _, v := range validators { - if err := v.Valid(cert); err != nil { + if err := v.Valid(cert, provisioner.SSHOptions{Backdate: backdate}); err != nil { return nil, errs.Wrap(http.StatusForbidden, err, "rekeySSH") } } diff --git a/authority/ssh_test.go b/authority/ssh_test.go index cc3f164c..b581740f 100644 --- a/authority/ssh_test.go +++ b/authority/ssh_test.go @@ -62,7 +62,7 @@ func (m sshTestCertModifier) Modify(cert *ssh.Certificate) error { type sshTestCertValidator string -func (v sshTestCertValidator) Valid(crt *ssh.Certificate) error { +func (v sshTestCertValidator) Valid(crt *ssh.Certificate, opts provisioner.SSHOptions) error { if v == "" { return nil } diff --git a/authority/tls_test.go b/authority/tls_test.go index 3fbd21bf..f946022f 100644 --- a/authority/tls_test.go +++ b/authority/tls_test.go @@ -178,7 +178,7 @@ func TestAuthority_Sign(t *testing.T) { csr: csr, extraOpts: extraOpts, signOpts: _signOpts, - err: errors.New("authority.Sign: requested duration of 25h0m0s is more than the authorized maximum certificate duration of 24h0m0s"), + err: errors.New("authority.Sign: requested duration of 25h0m0s is more than the authorized maximum certificate duration of 24h1m0s"), code: http.StatusUnauthorized, } },