From 41fec1577d0439e3c6666615b378b1b7a8e1698e Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 17 Nov 2021 11:46:57 -0800 Subject: [PATCH 1/3] Report duration errors directly to the cli. --- authority/provisioner/sign_options.go | 35 +++++++++++++++++++---- authority/provisioner/sign_ssh_options.go | 16 +++++------ 2 files changed, 36 insertions(+), 15 deletions(-) diff --git a/authority/provisioner/sign_options.go b/authority/provisioner/sign_options.go index 764916b6..7268b63b 100644 --- a/authority/provisioner/sign_options.go +++ b/authority/provisioner/sign_options.go @@ -8,12 +8,15 @@ import ( "crypto/x509/pkix" "encoding/asn1" "encoding/json" + "fmt" "net" + "net/http" "net/url" "reflect" "time" "github.com/pkg/errors" + "github.com/smallstep/certificates/errs" "go.step.sm/crypto/x509util" ) @@ -369,6 +372,28 @@ func newValidityValidator(min, max time.Duration) *validityValidator { return &validityValidator{min: min, max: max} } +// TODO(mariano): refactor errs package to allow sending real errors to the +// user. +func badRequest(format string, args ...interface{}) error { + msg := fmt.Sprintf(format, args...) + return &errs.Error{ + Status: http.StatusBadRequest, + Msg: msg, + Err: errors.New(msg), + } +} + +// TODO(mariano): refactor errs package to allow sending real errors to the +// user. +func unauthorized(format string, args ...interface{}) error { + msg := fmt.Sprintf(format, args...) + return &errs.Error{ + Status: http.StatusUnauthorized, + Msg: msg, + Err: errors.New(msg), + } +} + // Valid validates the certificate validity settings (notBefore/notAfter) and // and total duration. func (v *validityValidator) Valid(cert *x509.Certificate, o SignOptions) error { @@ -381,22 +406,20 @@ func (v *validityValidator) Valid(cert *x509.Certificate, o SignOptions) error { d := na.Sub(nb) if na.Before(now) { - return errors.Errorf("notAfter cannot be in the past; na=%v", na) + return badRequest("notAfter cannot be in the past; na=%v", na) } if na.Before(nb) { - return errors.Errorf("notAfter cannot be before notBefore; na=%v, nb=%v", na, nb) + return badRequest("notAfter cannot be before notBefore; na=%v, nb=%v", na, nb) } if d < v.min { - return errors.Errorf("requested duration of %v is less than the authorized minimum certificate duration of %v", - d, v.min) + return unauthorized("requested duration of %v is less than the authorized minimum certificate duration of %v", d, v.min) } // NOTE: this check is not "technically correct". We're allowing the max // duration of a cert to be "max + backdate" and not all certificates will // be backdated (e.g. if a user passes the NotBefore value then we do not // 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+o.Backdate) + return unauthorized("requested duration of %v is more than the authorized maximum certificate duration of %v", d, v.max+o.Backdate) } return nil } diff --git a/authority/provisioner/sign_ssh_options.go b/authority/provisioner/sign_ssh_options.go index 158470d1..e594c952 100644 --- a/authority/provisioner/sign_ssh_options.go +++ b/authority/provisioner/sign_ssh_options.go @@ -335,11 +335,11 @@ type sshCertValidityValidator struct { func (v *sshCertValidityValidator) Valid(cert *ssh.Certificate, opts SignSSHOptions) error { switch { case cert.ValidAfter == 0: - return errors.New("ssh certificate validAfter cannot be 0") + return badRequest("ssh certificate validAfter cannot be 0") case cert.ValidBefore < uint64(now().Unix()): - return errors.New("ssh certificate validBefore cannot be in the past") + return badRequest("ssh certificate validBefore cannot be in the past") case cert.ValidBefore < cert.ValidAfter: - return errors.New("ssh certificate validBefore cannot be before validAfter") + return badRequest("ssh certificate validBefore cannot be before validAfter") } var min, max time.Duration @@ -351,9 +351,9 @@ func (v *sshCertValidityValidator) Valid(cert *ssh.Certificate, opts SignSSHOpti min = v.MinHostSSHCertDuration() max = v.MaxHostSSHCertDuration() case 0: - return errors.New("ssh certificate type has not been set") + return badRequest("ssh certificate type has not been set") default: - return errors.Errorf("unknown ssh certificate type %d", cert.CertType) + return badRequest("unknown ssh certificate type %d", cert.CertType) } // To not take into account the backdate, time.Now() will be used to @@ -362,11 +362,9 @@ func (v *sshCertValidityValidator) Valid(cert *ssh.Certificate, opts SignSSHOpti switch { case dur < min: - return errors.Errorf("requested duration of %s is less than minimum "+ - "accepted duration for selected provisioner of %s", dur, min) + return unauthorized("requested duration of %s is less than minimum accepted duration for selected provisioner of %s", dur, min) 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+opts.Backdate) + return unauthorized("requested duration of %s is greater than maximum accepted duration for selected provisioner of %s", dur, max+opts.Backdate) default: return nil } From 1aadd63cefc5d9281eb1644dcb505a96073cd2b6 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 17 Nov 2021 12:00:54 -0800 Subject: [PATCH 2/3] Use always badRequest on duration errors. --- authority/provisioner/sign_options.go | 15 ++------------- authority/provisioner/sign_ssh_options.go | 4 ++-- authority/tls_test.go | 2 +- 3 files changed, 5 insertions(+), 16 deletions(-) diff --git a/authority/provisioner/sign_options.go b/authority/provisioner/sign_options.go index 7268b63b..6b013fc8 100644 --- a/authority/provisioner/sign_options.go +++ b/authority/provisioner/sign_options.go @@ -383,17 +383,6 @@ func badRequest(format string, args ...interface{}) error { } } -// TODO(mariano): refactor errs package to allow sending real errors to the -// user. -func unauthorized(format string, args ...interface{}) error { - msg := fmt.Sprintf(format, args...) - return &errs.Error{ - Status: http.StatusUnauthorized, - Msg: msg, - Err: errors.New(msg), - } -} - // Valid validates the certificate validity settings (notBefore/notAfter) and // and total duration. func (v *validityValidator) Valid(cert *x509.Certificate, o SignOptions) error { @@ -412,14 +401,14 @@ func (v *validityValidator) Valid(cert *x509.Certificate, o SignOptions) error { return badRequest("notAfter cannot be before notBefore; na=%v, nb=%v", na, nb) } if d < v.min { - return unauthorized("requested duration of %v is less than the authorized minimum certificate duration of %v", d, v.min) + return badRequest("requested duration of %v is less than the authorized minimum certificate duration of %v", d, v.min) } // NOTE: this check is not "technically correct". We're allowing the max // duration of a cert to be "max + backdate" and not all certificates will // be backdated (e.g. if a user passes the NotBefore value then we do not // apply a backdate). This is good enough. if d > v.max+o.Backdate { - return unauthorized("requested duration of %v is more than the authorized maximum certificate duration of %v", d, v.max+o.Backdate) + return badRequest("requested duration of %v is more than the authorized maximum certificate duration of %v", d, v.max+o.Backdate) } return nil } diff --git a/authority/provisioner/sign_ssh_options.go b/authority/provisioner/sign_ssh_options.go index e594c952..878d3d02 100644 --- a/authority/provisioner/sign_ssh_options.go +++ b/authority/provisioner/sign_ssh_options.go @@ -362,9 +362,9 @@ func (v *sshCertValidityValidator) Valid(cert *ssh.Certificate, opts SignSSHOpti switch { case dur < min: - return unauthorized("requested duration of %s is less than minimum accepted duration for selected provisioner of %s", dur, min) + return badRequest("requested duration of %s is less than minimum accepted duration for selected provisioner of %s", dur, min) case dur > max+opts.Backdate: - return unauthorized("requested duration of %s is greater than maximum accepted duration for selected provisioner of %s", dur, max+opts.Backdate) + return badRequest("requested duration of %s is greater than maximum accepted duration for selected provisioner of %s", dur, max+opts.Backdate) default: return nil } diff --git a/authority/tls_test.go b/authority/tls_test.go index f1d1748d..9fb47f00 100644 --- a/authority/tls_test.go +++ b/authority/tls_test.go @@ -310,7 +310,7 @@ func TestAuthority_Sign(t *testing.T) { extraOpts: extraOpts, signOpts: _signOpts, err: errors.New("authority.Sign: requested duration of 25h0m0s is more than the authorized maximum certificate duration of 24h1m0s"), - code: http.StatusUnauthorized, + code: http.StatusBadRequest, } }, "fail validate sans when adding common name not in claims": func(t *testing.T) *signTest { From acd0bac02523ff35681e938c606f653b8d9844a6 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 17 Nov 2021 12:03:29 -0800 Subject: [PATCH 3/3] Remove extra and in comment. --- authority/provisioner/sign_options.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/authority/provisioner/sign_options.go b/authority/provisioner/sign_options.go index 6b013fc8..95f7fc39 100644 --- a/authority/provisioner/sign_options.go +++ b/authority/provisioner/sign_options.go @@ -384,7 +384,7 @@ func badRequest(format string, args ...interface{}) error { } // Valid validates the certificate validity settings (notBefore/notAfter) and -// and total duration. +// total duration. func (v *validityValidator) Valid(cert *x509.Certificate, o SignOptions) error { var ( na = cert.NotAfter.Truncate(time.Second)