From b5db3f5706834b630d6b2e71a594b96cb86f3537 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 23 Nov 2021 11:52:55 -0800 Subject: [PATCH 01/13] Modify errs.ForbiddenErr to always return an error to the cli. --- api/api.go | 4 ++-- api/revoke.go | 2 +- api/sign.go | 2 +- api/ssh.go | 6 +++--- api/sshRekey.go | 4 ++-- api/sshRenew.go | 4 ++-- api/sshRevoke.go | 2 +- errs/error.go | 17 +++++++++++------ 8 files changed, 23 insertions(+), 18 deletions(-) diff --git a/api/api.go b/api/api.go index e057caaa..16e24bb2 100644 --- a/api/api.go +++ b/api/api.go @@ -348,7 +348,7 @@ func (h *caHandler) ProvisionerKey(w http.ResponseWriter, r *http.Request) { func (h *caHandler) Roots(w http.ResponseWriter, r *http.Request) { roots, err := h.Authority.GetRoots() if err != nil { - WriteError(w, errs.ForbiddenErr(err)) + WriteError(w, errs.ForbiddenErr(err, "error getting roots")) return } @@ -366,7 +366,7 @@ func (h *caHandler) Roots(w http.ResponseWriter, r *http.Request) { func (h *caHandler) Federation(w http.ResponseWriter, r *http.Request) { federated, err := h.Authority.GetFederation() if err != nil { - WriteError(w, errs.ForbiddenErr(err)) + WriteError(w, errs.ForbiddenErr(err, "error getting federated roots")) return } diff --git a/api/revoke.go b/api/revoke.go index 44d52cb9..25520e3e 100644 --- a/api/revoke.go +++ b/api/revoke.go @@ -96,7 +96,7 @@ func (h *caHandler) Revoke(w http.ResponseWriter, r *http.Request) { } if err := h.Authority.Revoke(ctx, opts); err != nil { - WriteError(w, errs.ForbiddenErr(err)) + WriteError(w, errs.ForbiddenErr(err, "error revoking certificate")) return } diff --git a/api/sign.go b/api/sign.go index a1e5b998..93c5f599 100644 --- a/api/sign.go +++ b/api/sign.go @@ -74,7 +74,7 @@ func (h *caHandler) Sign(w http.ResponseWriter, r *http.Request) { certChain, err := h.Authority.Sign(body.CsrPEM.CertificateRequest, opts, signOpts...) if err != nil { - WriteError(w, errs.ForbiddenErr(err)) + WriteError(w, errs.ForbiddenErr(err, "error signing certificate")) return } certChainPEM := certChainToPEM(certChain) diff --git a/api/ssh.go b/api/ssh.go index 43ee6b98..c9be1527 100644 --- a/api/ssh.go +++ b/api/ssh.go @@ -293,7 +293,7 @@ func (h *caHandler) SSHSign(w http.ResponseWriter, r *http.Request) { cert, err := h.Authority.SignSSH(ctx, publicKey, opts, signOpts...) if err != nil { - WriteError(w, errs.ForbiddenErr(err)) + WriteError(w, errs.ForbiddenErr(err, "error signing ssh certificate")) return } @@ -301,7 +301,7 @@ func (h *caHandler) SSHSign(w http.ResponseWriter, r *http.Request) { if addUserPublicKey != nil && authority.IsValidForAddUser(cert) == nil { addUserCert, err := h.Authority.SignSSHAddUser(ctx, addUserPublicKey, cert) if err != nil { - WriteError(w, errs.ForbiddenErr(err)) + WriteError(w, errs.ForbiddenErr(err, "error signing ssh certificate")) return } addUserCertificate = &SSHCertificate{addUserCert} @@ -326,7 +326,7 @@ func (h *caHandler) SSHSign(w http.ResponseWriter, r *http.Request) { certChain, err := h.Authority.Sign(cr, provisioner.SignOptions{}, signOpts...) if err != nil { - WriteError(w, errs.ForbiddenErr(err)) + WriteError(w, errs.ForbiddenErr(err, "error signing identity certificate")) return } identityCertificate = certChainToPEM(certChain) diff --git a/api/sshRekey.go b/api/sshRekey.go index 8d2ba5ee..b2c55509 100644 --- a/api/sshRekey.go +++ b/api/sshRekey.go @@ -68,7 +68,7 @@ func (h *caHandler) SSHRekey(w http.ResponseWriter, r *http.Request) { newCert, err := h.Authority.RekeySSH(ctx, oldCert, publicKey, signOpts...) if err != nil { - WriteError(w, errs.ForbiddenErr(err)) + WriteError(w, errs.ForbiddenErr(err, "error signing ssh certificate")) return } @@ -78,7 +78,7 @@ func (h *caHandler) SSHRekey(w http.ResponseWriter, r *http.Request) { identity, err := h.renewIdentityCertificate(r, notBefore, notAfter) if err != nil { - WriteError(w, errs.ForbiddenErr(err)) + WriteError(w, errs.ForbiddenErr(err, "error signing identity certificate")) return } diff --git a/api/sshRenew.go b/api/sshRenew.go index 5dfd5983..8d07ba01 100644 --- a/api/sshRenew.go +++ b/api/sshRenew.go @@ -60,7 +60,7 @@ func (h *caHandler) SSHRenew(w http.ResponseWriter, r *http.Request) { newCert, err := h.Authority.RenewSSH(ctx, oldCert) if err != nil { - WriteError(w, errs.ForbiddenErr(err)) + WriteError(w, errs.ForbiddenErr(err, "error signing ssh certificate")) return } @@ -70,7 +70,7 @@ func (h *caHandler) SSHRenew(w http.ResponseWriter, r *http.Request) { identity, err := h.renewIdentityCertificate(r, notBefore, notAfter) if err != nil { - WriteError(w, errs.ForbiddenErr(err)) + WriteError(w, errs.ForbiddenErr(err, "error signing identity certificate")) return } diff --git a/api/sshRevoke.go b/api/sshRevoke.go index cfc25f04..60f44f2a 100644 --- a/api/sshRevoke.go +++ b/api/sshRevoke.go @@ -75,7 +75,7 @@ func (h *caHandler) SSHRevoke(w http.ResponseWriter, r *http.Request) { opts.OTT = body.OTT if err := h.Authority.Revoke(ctx, opts); err != nil { - WriteError(w, errs.ForbiddenErr(err)) + WriteError(w, errs.ForbiddenErr(err, "error revoking ssh certificate")) return } diff --git a/errs/error.go b/errs/error.go index 60312313..2c1fe6a9 100644 --- a/errs/error.go +++ b/errs/error.go @@ -169,7 +169,8 @@ func StatusCodeError(code int, e error, opts ...Option) error { case http.StatusUnauthorized: return UnauthorizedErr(e, opts...) case http.StatusForbidden: - return ForbiddenErr(e, opts...) + opts = append(opts, withDefaultMessage(ForbiddenDefaultMsg)) + return NewErr(http.StatusForbidden, e, opts...) case http.StatusInternalServerError: return InternalServerErr(e, opts...) case http.StatusNotImplemented: @@ -199,12 +200,18 @@ var ( // BadRequestPrefix is the prefix added to the bad request messages that are // directly sent to the cli. BadRequestPrefix = "The request could not be completed: " + + // ForbiddenPrefix is the prefix added to the forbidden messates that are + // sent to the cli. + ForbiddenPrefix = "The request was forbidden by the certificate authority: " ) func formatMessage(status int, msg string) string { switch status { case http.StatusBadRequest: return BadRequestPrefix + msg + "." + case http.StatusForbidden: + return ForbiddenPrefix + msg + "." default: return msg } @@ -356,14 +363,12 @@ func UnauthorizedErr(err error, opts ...Option) error { // Forbidden creates a 403 error with the given format and arguments. func Forbidden(format string, args ...interface{}) error { - args = append(args, withDefaultMessage(ForbiddenDefaultMsg)) - return Errorf(http.StatusForbidden, format, args...) + return New(http.StatusForbidden, format, args...) } // ForbiddenErr returns an 403 error with the given error. -func ForbiddenErr(err error, opts ...Option) error { - opts = append(opts, withDefaultMessage(ForbiddenDefaultMsg)) - return NewErr(http.StatusForbidden, err, opts...) +func ForbiddenErr(err error, format string, args ...interface{}) error { + return NewError(http.StatusForbidden, err, format, args...) } // NotFound creates a 404 error with the given format and arguments. From bb26799583f0c139e0f176592eafe932cba39a5d Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 23 Nov 2021 12:04:51 -0800 Subject: [PATCH 02/13] Modify errs.Wrap with forbidden errors. --- authority/ssh.go | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/authority/ssh.go b/authority/ssh.go index 9c5405c4..d533374a 100644 --- a/authority/ssh.go +++ b/authority/ssh.go @@ -9,7 +9,6 @@ import ( "strings" "time" - "github.com/pkg/errors" "github.com/smallstep/certificates/authority/config" "github.com/smallstep/certificates/authority/provisioner" "github.com/smallstep/certificates/db" @@ -174,7 +173,7 @@ func (a *Authority) SignSSH(ctx context.Context, key ssh.PublicKey, opts provisi // validate the given SSHOptions case provisioner.SSHCertOptionsValidator: if err := o.Valid(opts); err != nil { - return nil, errs.Wrap(http.StatusForbidden, err, "authority.SignSSH") + return nil, errs.ForbiddenErr(err, "error validating ssh certificate options") } default: @@ -214,7 +213,7 @@ func (a *Authority) SignSSH(ctx context.Context, key ssh.PublicKey, opts provisi // Use provisioner modifiers. for _, m := range mods { if err := m.Modify(certTpl, opts); err != nil { - return nil, errs.Wrap(http.StatusForbidden, err, "authority.SignSSH") + return nil, errs.ForbiddenErr(err, "error creating ssh certificate") } } @@ -244,7 +243,7 @@ func (a *Authority) SignSSH(ctx context.Context, key ssh.PublicKey, opts provisi // User provisioners validators. for _, v := range validators { if err := v.Valid(cert, opts); err != nil { - return nil, errs.Wrap(http.StatusForbidden, err, "authority.SignSSH") + return nil, errs.ForbiddenErr(err, "error validating ssh certificate") } } @@ -382,7 +381,7 @@ func (a *Authority) RekeySSH(ctx context.Context, oldCert *ssh.Certificate, pub // Apply validators from provisioner. for _, v := range validators { if err := v.Valid(cert, provisioner.SignSSHOptions{Backdate: backdate}); err != nil { - return nil, errs.Wrap(http.StatusForbidden, err, "rekeySSH") + return nil, errs.ForbiddenErr(err, "error validating ssh certificate") } } @@ -407,12 +406,12 @@ func (a *Authority) storeSSHCertificate(cert *ssh.Certificate) error { // the given certificate. func IsValidForAddUser(cert *ssh.Certificate) error { if cert.CertType != ssh.UserCert { - return errors.New("certificate is not a user certificate") + return errs.Forbidden("certificate is not a user certificate") } switch len(cert.ValidPrincipals) { case 0: - return errors.New("certificate does not have any principals") + return errs.Forbidden("certificate does not have any principals") case 1: return nil case 2: @@ -421,9 +420,9 @@ func IsValidForAddUser(cert *ssh.Certificate) error { if strings.Index(cert.ValidPrincipals[1], "@") > 0 { return nil } - return errors.New("certificate does not have only one principal") + return errs.Forbidden("certificate does not have only one principal") default: - return errors.New("certificate does not have only one principal") + return errs.Forbidden("certificate does not have only one principal") } } @@ -433,7 +432,7 @@ func (a *Authority) SignSSHAddUser(ctx context.Context, key ssh.PublicKey, subje return nil, errs.NotImplemented("signSSHAddUser: user certificate signing is not enabled") } if err := IsValidForAddUser(subject); err != nil { - return nil, errs.Wrap(http.StatusForbidden, err, "signSSHAddUser") + return nil, err } nonce, err := randutil.ASCII(32) From 031d4d700009d4a8d5cbfdb67b8bfc89bcbf27d6 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 23 Nov 2021 17:52:17 -0800 Subject: [PATCH 03/13] Return BadRequest when validating sign options. --- authority/ssh.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/authority/ssh.go b/authority/ssh.go index d533374a..7b8f26e2 100644 --- a/authority/ssh.go +++ b/authority/ssh.go @@ -173,7 +173,7 @@ func (a *Authority) SignSSH(ctx context.Context, key ssh.PublicKey, opts provisi // validate the given SSHOptions case provisioner.SSHCertOptionsValidator: if err := o.Valid(opts); err != nil { - return nil, errs.ForbiddenErr(err, "error validating ssh certificate options") + return nil, errs.BadRequestErr(err, "error validating ssh certificate options") } default: From 1da7ea66464c8128d339da84646d348be88520b4 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 23 Nov 2021 17:52:39 -0800 Subject: [PATCH 04/13] Return always http errors in sign ssh options. --- authority/provisioner/sign_ssh_options.go | 61 ++++++++++++----------- 1 file changed, 33 insertions(+), 28 deletions(-) diff --git a/authority/provisioner/sign_ssh_options.go b/authority/provisioner/sign_ssh_options.go index 6cd38c59..80ff884e 100644 --- a/authority/provisioner/sign_ssh_options.go +++ b/authority/provisioner/sign_ssh_options.go @@ -58,6 +58,11 @@ func (o SignSSHOptions) Validate() error { if o.CertType != "" && o.CertType != SSHUserCert && o.CertType != SSHHostCert { return errs.BadRequest("unknown certificate type '%s'", o.CertType) } + for _, p := range o.Principals { + if p == "" { + return errs.BadRequest("principals cannot contain empty values") + } + } return nil } @@ -75,7 +80,7 @@ func (o SignSSHOptions) Modify(cert *ssh.Certificate, _ SignSSHOptions) error { case SSHHostCert: cert.CertType = ssh.HostCert default: - return errors.Errorf("ssh certificate has an unknown type - %s", o.CertType) + return errs.BadRequest("ssh certificate has an unknown type '%s'", o.CertType) } cert.KeyId = o.KeyID @@ -95,7 +100,7 @@ func (o SignSSHOptions) ModifyValidity(cert *ssh.Certificate) error { cert.ValidBefore = uint64(o.ValidBefore.RelativeTime(t).Unix()) } if cert.ValidAfter > 0 && cert.ValidBefore > 0 && cert.ValidAfter > cert.ValidBefore { - return errors.New("ssh certificate valid after cannot be greater than valid before") + return errs.BadRequest("ssh certificate validAfter cannot be greater than validBefore") } return nil } @@ -104,16 +109,16 @@ func (o SignSSHOptions) ModifyValidity(cert *ssh.Certificate) error { // ignores zero values. func (o SignSSHOptions) match(got SignSSHOptions) error { if o.CertType != "" && got.CertType != "" && o.CertType != got.CertType { - return errors.Errorf("ssh certificate type does not match - got %v, want %v", got.CertType, o.CertType) + return errs.BadRequest("ssh certificate type does not match - got %v, want %v", got.CertType, o.CertType) } if len(o.Principals) > 0 && len(got.Principals) > 0 && !containsAllMembers(o.Principals, got.Principals) { - return errors.Errorf("ssh certificate principals does not match - got %v, want %v", got.Principals, o.Principals) + return errs.BadRequest("ssh certificate principals does not match - got %v, want %v", got.Principals, o.Principals) } if !o.ValidAfter.IsZero() && !got.ValidAfter.IsZero() && !o.ValidAfter.Equal(&got.ValidAfter) { - return errors.Errorf("ssh certificate valid after does not match - got %v, want %v", got.ValidAfter, o.ValidAfter) + return errs.BadRequest("ssh certificate validAfter does not match - got %v, want %v", got.ValidAfter, o.ValidAfter) } if !o.ValidBefore.IsZero() && !got.ValidBefore.IsZero() && !o.ValidBefore.Equal(&got.ValidBefore) { - return errors.Errorf("ssh certificate valid before does not match - got %v, want %v", got.ValidBefore, o.ValidBefore) + return errs.BadRequest("ssh certificate validBefore does not match - got %v, want %v", got.ValidBefore, o.ValidBefore) } return nil } @@ -206,7 +211,7 @@ func (m *sshDefaultExtensionModifier) Modify(cert *ssh.Certificate, _ SignSSHOpt cert.Extensions["permit-user-rc"] = "" return nil default: - return errors.New("ssh certificate type has not been set or is invalid") + return errs.BadRequest("ssh certificate has an unknown type '%d'", cert.CertType) } } @@ -272,7 +277,7 @@ func (m *sshLimitDuration) Modify(cert *ssh.Certificate, o SignSSHOptions) error certValidAfter := time.Unix(int64(cert.ValidAfter), 0) if certValidAfter.After(m.NotAfter) { - return errors.Errorf("provisioning credential expiration (%s) is before requested certificate validAfter (%s)", + return errs.Forbidden("provisioning credential expiration (%s) is before requested certificate validAfter (%s)", m.NotAfter, certValidAfter) } @@ -285,7 +290,7 @@ func (m *sshLimitDuration) Modify(cert *ssh.Certificate, o SignSSHOptions) error } else { certValidBefore := time.Unix(int64(cert.ValidBefore), 0) if m.NotAfter.Before(certValidBefore) { - return errors.Errorf("provisioning credential expiration (%s) is before requested certificate validBefore (%s)", + return errs.Forbidden("provisioning credential expiration (%s) is before requested certificate validBefore (%s)", m.NotAfter, certValidBefore) } } @@ -319,11 +324,11 @@ type sshCertOptionsRequireValidator struct { func (v *sshCertOptionsRequireValidator) Valid(got SignSSHOptions) error { switch { case v.CertType && got.CertType == "": - return errors.New("ssh certificate certType cannot be empty") + return errs.BadRequest("ssh certificate certType cannot be empty") case v.KeyID && got.KeyID == "": - return errors.New("ssh certificate keyID cannot be empty") + return errs.BadRequest("ssh certificate keyID cannot be empty") case v.Principals && len(got.Principals) == 0: - return errors.New("ssh certificate principals cannot be empty") + return errs.BadRequest("ssh certificate principals cannot be empty") default: return nil } @@ -354,7 +359,7 @@ func (v *sshCertValidityValidator) Valid(cert *ssh.Certificate, opts SignSSHOpti case 0: return errs.BadRequest("ssh certificate type has not been set") default: - return errs.BadRequest("unknown ssh certificate type %d", cert.CertType) + return errs.BadRequest("unknown ssh certificate type '%d'", cert.CertType) } // To not take into account the backdate, time.Now() will be used to @@ -381,25 +386,25 @@ type sshCertDefaultValidator struct{} func (v *sshCertDefaultValidator) Valid(cert *ssh.Certificate, o SignSSHOptions) error { switch { case len(cert.Nonce) == 0: - return errors.New("ssh certificate nonce cannot be empty") + return errs.Forbidden("ssh certificate nonce cannot be empty") case cert.Key == nil: - return errors.New("ssh certificate key cannot be nil") + return errs.Forbidden("ssh certificate key cannot be nil") case cert.Serial == 0: - return errors.New("ssh certificate serial cannot be 0") + return errs.Forbidden("ssh certificate serial cannot be 0") case cert.CertType != ssh.UserCert && cert.CertType != ssh.HostCert: - return errors.Errorf("ssh certificate has an unknown type: %d", cert.CertType) + return errs.Forbidden("ssh certificate has an unknown type '%d'", cert.CertType) case cert.KeyId == "": - return errors.New("ssh certificate key id cannot be empty") + return errs.Forbidden("ssh certificate key id cannot be empty") case cert.ValidAfter == 0: - return errors.New("ssh certificate validAfter cannot be 0") + return errs.Forbidden("ssh certificate validAfter cannot be 0") case cert.ValidBefore < uint64(now().Unix()): - return errors.New("ssh certificate validBefore cannot be in the past") + return errs.Forbidden("ssh certificate validBefore cannot be in the past") case cert.ValidBefore < cert.ValidAfter: - return errors.New("ssh certificate validBefore cannot be before validAfter") + return errs.Forbidden("ssh certificate validBefore cannot be before validAfter") case cert.SignatureKey == nil: - return errors.New("ssh certificate signature key cannot be nil") + return errs.Forbidden("ssh certificate signature key cannot be nil") case cert.Signature == nil: - return errors.New("ssh certificate signature cannot be nil") + return errs.Forbidden("ssh certificate signature cannot be nil") default: return nil } @@ -411,25 +416,25 @@ type sshDefaultPublicKeyValidator struct{} // Valid checks that certificate request common name matches the one configured. func (v sshDefaultPublicKeyValidator) Valid(cert *ssh.Certificate, o SignSSHOptions) error { if cert.Key == nil { - return errors.New("ssh certificate key cannot be nil") + return errs.BadRequest("ssh certificate key cannot be nil") } switch cert.Key.Type() { case ssh.KeyAlgoRSA: _, in, ok := sshParseString(cert.Key.Marshal()) if !ok { - return errors.New("ssh certificate key is invalid") + return errs.BadRequest("ssh certificate key is invalid") } key, err := sshParseRSAPublicKey(in) if err != nil { - return err + return errs.BadRequestErr(err, "error parsing public key") } if key.Size() < keyutil.MinRSAKeyBytes { - return errors.Errorf("ssh certificate key must be at least %d bits (%d bytes)", + return errs.Forbidden("ssh certificate key must be at least %d bits (%d bytes)", 8*keyutil.MinRSAKeyBytes, keyutil.MinRSAKeyBytes) } return nil case ssh.KeyAlgoDSA: - return errors.New("ssh certificate key algorithm (DSA) is not supported") + return errs.BadRequest("ssh certificate key algorithm (DSA) is not supported") default: return nil } From a33709ce8df12143169dd1d6eb2187e66d58fac3 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 23 Nov 2021 18:06:18 -0800 Subject: [PATCH 05/13] Fix sign ssh options tests. --- authority/provisioner/sign_ssh_options.go | 4 ++-- authority/provisioner/sign_ssh_options_test.go | 14 +++++++------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/authority/provisioner/sign_ssh_options.go b/authority/provisioner/sign_ssh_options.go index 80ff884e..30293947 100644 --- a/authority/provisioner/sign_ssh_options.go +++ b/authority/provisioner/sign_ssh_options.go @@ -56,7 +56,7 @@ type SignSSHOptions struct { // Validate validates the given SignSSHOptions. func (o SignSSHOptions) Validate() error { if o.CertType != "" && o.CertType != SSHUserCert && o.CertType != SSHHostCert { - return errs.BadRequest("unknown certificate type '%s'", o.CertType) + return errs.BadRequest("certType '%s' is not valid", o.CertType) } for _, p := range o.Principals { if p == "" { @@ -359,7 +359,7 @@ func (v *sshCertValidityValidator) Valid(cert *ssh.Certificate, opts SignSSHOpti case 0: return errs.BadRequest("ssh certificate type has not been set") default: - return errs.BadRequest("unknown ssh certificate type '%d'", cert.CertType) + return errs.BadRequest("ssh certificate has an unknown type '%d'", cert.CertType) } // To not take into account the backdate, time.Now() will be used to diff --git a/authority/provisioner/sign_ssh_options_test.go b/authority/provisioner/sign_ssh_options_test.go index 3a1ff324..b59d6945 100644 --- a/authority/provisioner/sign_ssh_options_test.go +++ b/authority/provisioner/sign_ssh_options_test.go @@ -49,14 +49,14 @@ func TestSSHOptions_Modify(t *testing.T) { return test{ so: SignSSHOptions{CertType: "foo"}, cert: new(ssh.Certificate), - err: errors.Errorf("ssh certificate has an unknown type - foo"), + err: errors.Errorf("ssh certificate has an unknown type 'foo'"), } }, "fail/validAfter-greater-validBefore": func() test { return test{ so: SignSSHOptions{CertType: "user"}, cert: &ssh.Certificate{ValidAfter: uint64(15), ValidBefore: uint64(10)}, - err: errors.Errorf("ssh certificate valid after cannot be greater than valid before"), + err: errors.Errorf("ssh certificate validAfter cannot be greater than validBefore"), } }, "ok/user-cert": func() test { @@ -136,14 +136,14 @@ func TestSSHOptions_Match(t *testing.T) { return test{ so: SignSSHOptions{ValidAfter: NewTimeDuration(time.Now().Add(1 * time.Minute))}, cmp: SignSSHOptions{ValidAfter: NewTimeDuration(time.Now().Add(5 * time.Minute))}, - err: errors.Errorf("ssh certificate valid after does not match"), + err: errors.Errorf("ssh certificate validAfter does not match"), } }, "fail/validBefore": func() test { return test{ so: SignSSHOptions{ValidBefore: NewTimeDuration(time.Now().Add(1 * time.Minute))}, cmp: SignSSHOptions{ValidBefore: NewTimeDuration(time.Now().Add(5 * time.Minute))}, - err: errors.Errorf("ssh certificate valid before does not match"), + err: errors.Errorf("ssh certificate validBefore does not match"), } }, "ok/original-empty": func() test { @@ -394,7 +394,7 @@ func Test_sshDefaultExtensionModifier_Modify(t *testing.T) { return test{ modifier: sshDefaultExtensionModifier{}, cert: cert, - err: errors.New("ssh certificate type has not been set or is invalid"), + err: errors.New("ssh certificate has an unknown type '3'"), } }, "ok/host": func() test { @@ -518,7 +518,7 @@ func Test_sshCertDefaultValidator_Valid(t *testing.T) { "fail/unexpected-cert-type", // UserCert = 1, HostCert = 2 &ssh.Certificate{Nonce: []byte("foo"), Key: sshPub, CertType: 3, Serial: 1}, - errors.New("ssh certificate has an unknown type: 3"), + errors.New("ssh certificate has an unknown type '3'"), }, { "fail/empty-cert-key-id", @@ -725,7 +725,7 @@ func Test_sshCertValidityValidator(t *testing.T) { ValidBefore: uint64(now().Add(10 * time.Minute).Unix()), }, SignSSHOptions{}, - errors.New("unknown ssh certificate type 3"), + errors.New("ssh certificate has an unknown type '3'"), }, { "fail/duration Date: Tue, 23 Nov 2021 18:32:33 -0800 Subject: [PATCH 06/13] Return always http errors in sign options. --- authority/provisioner/sign_options.go | 61 ++++++++++++++------------- 1 file changed, 32 insertions(+), 29 deletions(-) diff --git a/authority/provisioner/sign_options.go b/authority/provisioner/sign_options.go index c4779ea3..167d371a 100644 --- a/authority/provisioner/sign_options.go +++ b/authority/provisioner/sign_options.go @@ -9,12 +9,14 @@ import ( "encoding/asn1" "encoding/json" "net" + "net/http" "net/url" "reflect" "time" "github.com/pkg/errors" "github.com/smallstep/certificates/errs" + "go.step.sm/crypto/keyutil" "go.step.sm/crypto/x509util" ) @@ -83,19 +85,19 @@ type emailOnlyIdentity string func (e emailOnlyIdentity) Valid(req *x509.CertificateRequest) error { switch { case len(req.DNSNames) > 0: - return errors.New("certificate request cannot contain DNS names") + return errs.BadRequest("certificate request cannot contain DNS names") case len(req.IPAddresses) > 0: - return errors.New("certificate request cannot contain IP addresses") + return errs.BadRequest("certificate request cannot contain IP addresses") case len(req.URIs) > 0: - return errors.New("certificate request cannot contain URIs") + return errs.BadRequest("certificate request cannot contain URIs") case len(req.EmailAddresses) == 0: - return errors.New("certificate request does not contain any email address") + return errs.BadRequest("certificate request does not contain any email address") case len(req.EmailAddresses) > 1: - return errors.New("certificate request contains too many email addresses") + return errs.BadRequest("certificate request contains too many email addresses") case req.EmailAddresses[0] == "": - return errors.New("certificate request cannot contain an empty email address") + return errs.BadRequest("certificate request cannot contain an empty email address") case req.EmailAddresses[0] != string(e): - return errors.Errorf("certificate request does not contain the valid email address, got %s, want %s", req.EmailAddresses[0], e) + return errs.BadRequest("certificate request does not contain the valid email address - got %s, want %s", req.EmailAddresses[0], e) default: return nil } @@ -108,12 +110,13 @@ type defaultPublicKeyValidator struct{} func (v defaultPublicKeyValidator) Valid(req *x509.CertificateRequest) error { switch k := req.PublicKey.(type) { case *rsa.PublicKey: - if k.Size() < 256 { - return errors.New("rsa key in CSR must be at least 2048 bits (256 bytes)") + if k.Size() < keyutil.MinRSAKeyBytes { + return errs.Forbidden("certificate request RSA key must be at least %d bits (%d bytes)", + 8*keyutil.MinRSAKeyBytes, keyutil.MinRSAKeyBytes) } case *ecdsa.PublicKey, ed25519.PublicKey: default: - return errors.Errorf("unrecognized public key of type '%T' in CSR", k) + return errs.BadRequest("certificate request key of type '%T' is not supported", k) } return nil } @@ -139,11 +142,12 @@ func (v publicKeyMinimumLengthValidator) Valid(req *x509.CertificateRequest) err case *rsa.PublicKey: minimumLengthInBytes := v.length / 8 if k.Size() < minimumLengthInBytes { - return errors.Errorf("rsa key in CSR must be at least %d bits (%d bytes)", v.length, minimumLengthInBytes) + return errs.Forbidden("certificate request RSA key must be at least %d bits (%d bytes)", + v.length, minimumLengthInBytes) } case *ecdsa.PublicKey, ed25519.PublicKey: default: - return errors.Errorf("unrecognized public key of type '%T' in CSR", k) + return errs.BadRequest("certificate request key of type '%T' is not supported", k) } return nil } @@ -158,7 +162,7 @@ func (v commonNameValidator) Valid(req *x509.CertificateRequest) error { return nil } if req.Subject.CommonName != string(v) { - return errors.Errorf("certificate request does not contain the valid common name; requested common name = %s, token subject = %s", req.Subject.CommonName, v) + return errs.BadRequest("certificate request does not contain the valid common name - got %s, want %s", req.Subject.CommonName, v) } return nil } @@ -176,7 +180,7 @@ func (v commonNameSliceValidator) Valid(req *x509.CertificateRequest) error { return nil } } - return errors.Errorf("certificate request does not contain the valid common name, got %s, want %s", req.Subject.CommonName, v) + return errs.BadRequest("certificate request does not contain the valid common name - got %s, want %s", req.Subject.CommonName, v) } // dnsNamesValidator validates the DNS names SAN of a certificate request. @@ -197,7 +201,7 @@ func (v dnsNamesValidator) Valid(req *x509.CertificateRequest) error { got[s] = true } if !reflect.DeepEqual(want, got) { - return errors.Errorf("certificate request does not contain the valid DNS names - got %v, want %v", req.DNSNames, v) + return errs.BadRequest("certificate request does not contain the valid DNS names - got %v, want %v", req.DNSNames, v) } return nil } @@ -220,7 +224,7 @@ func (v ipAddressesValidator) Valid(req *x509.CertificateRequest) error { got[ip.String()] = true } if !reflect.DeepEqual(want, got) { - return errors.Errorf("IP Addresses claim failed - got %v, want %v", req.IPAddresses, v) + return errs.BadRequest("certificate request does not contain the valid IP addresses - got %v, want %v", req.IPAddresses, v) } return nil } @@ -243,7 +247,7 @@ func (v emailAddressesValidator) Valid(req *x509.CertificateRequest) error { got[s] = true } if !reflect.DeepEqual(want, got) { - return errors.Errorf("certificate request does not contain the valid Email Addresses - got %v, want %v", req.EmailAddresses, v) + return errs.BadRequest("certificate request does not contain the valid email addresses - got %v, want %v", req.EmailAddresses, v) } return nil } @@ -266,7 +270,7 @@ func (v urisValidator) Valid(req *x509.CertificateRequest) error { got[u.String()] = true } if !reflect.DeepEqual(want, got) { - return errors.Errorf("URIs claim failed - got %v, want %v", req.URIs, v) + return errs.BadRequest("certificate request does not contain the valid URIs - got %v, want %v", req.URIs, v) } return nil } @@ -334,15 +338,15 @@ func (v profileLimitDuration) Modify(cert *x509.Certificate, so SignOptions) err backdate = -1 * so.Backdate } if notBefore.Before(v.notBefore) { - return errors.Errorf("requested certificate notBefore (%s) is before "+ - "the active validity window of the provisioning credential (%s)", + return errs.Forbidden( + "requested certificate notBefore (%s) is before the active validity window of the provisioning credential (%s)", notBefore, v.notBefore) } notAfter := so.NotAfter.RelativeTime(notBefore) if notAfter.After(v.notAfter) { - return errors.Errorf("requested certificate notAfter (%s) is after "+ - "the expiration of the provisioning credential (%s)", + return errs.Forbidden( + "requested certificate notAfter (%s) is after the expiration of the provisioning credential (%s)", notAfter, v.notAfter) } if notAfter.IsZero() { @@ -422,16 +426,15 @@ func newForceCNOption(forceCN bool) *forceCNOption { func (o *forceCNOption) Modify(cert *x509.Certificate, _ SignOptions) error { if !o.ForceCN { - // Forcing CN is disabled, do nothing to certificate return nil } + // Force the common name to be the first DNS if not provided. if cert.Subject.CommonName == "" { - if len(cert.DNSNames) > 0 { - cert.Subject.CommonName = cert.DNSNames[0] - } else { - return errors.New("Cannot force CN, DNSNames is empty") + if len(cert.DNSNames) == 0 { + return errs.Forbidden("cannot force common name, DNS names is empty") } + cert.Subject.CommonName = cert.DNSNames[0] } return nil @@ -456,7 +459,7 @@ func newProvisionerExtensionOption(typ Type, name, credentialID string, keyValue func (o *provisionerExtensionOption) Modify(cert *x509.Certificate, _ SignOptions) error { ext, err := createProvisionerExtension(o.Type, o.Name, o.CredentialID, o.KeyValuePairs...) if err != nil { - return err + return errs.NewError(http.StatusInternalServerError, err, "error creating certificate") } // Prepend the provisioner extension. In the auth.Sign code we will // force the resulting certificate to only have one extension, the @@ -477,7 +480,7 @@ func createProvisionerExtension(typ int, name, credentialID string, keyValuePair KeyValuePairs: keyValuePairs, }) if err != nil { - return pkix.Extension{}, errors.Wrapf(err, "error marshaling provisioner extension") + return pkix.Extension{}, errors.Wrap(err, "error marshaling provisioner extension") } return pkix.Extension{ Id: stepOIDProvisioner, From b9beab071d133c88354da80bd240e72abdc503ed Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 23 Nov 2021 18:43:36 -0800 Subject: [PATCH 07/13] Fix unit tests. --- authority/provisioner/sign_options_test.go | 12 ++++++------ authority/tls_test.go | 6 +++--- ca/ca_test.go | 4 ++-- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/authority/provisioner/sign_options_test.go b/authority/provisioner/sign_options_test.go index cf8f7a54..32b8e3c6 100644 --- a/authority/provisioner/sign_options_test.go +++ b/authority/provisioner/sign_options_test.go @@ -77,12 +77,12 @@ func Test_defaultPublicKeyValidator_Valid(t *testing.T) { { "fail/unrecognized-key-type", &x509.CertificateRequest{PublicKey: "foo"}, - errors.New("unrecognized public key of type 'string' in CSR"), + errors.New("certificate request key of type 'string' is not supported"), }, { "fail/rsa/too-short", shortRSA, - errors.New("rsa key in CSR must be at least 2048 bits (256 bytes)"), + errors.New("certificate request RSA key must be at least 2048 bits (256 bytes)"), }, { "ok/rsa", @@ -303,14 +303,14 @@ func Test_defaultSANsValidator_Valid(t *testing.T) { return test{ csr: &x509.CertificateRequest{EmailAddresses: []string{"max@fx.com", "mariano@fx.com"}}, expectedSANs: []string{"dcow@fx.com"}, - err: errors.New("certificate request does not contain the valid Email Addresses"), + err: errors.New("certificate request does not contain the valid email addresses"), } }, "fail/ipAddressesValidator": func() test { return test{ csr: &x509.CertificateRequest{IPAddresses: []net.IP{net.ParseIP("1.1.1.1"), net.ParseIP("127.0.0.1")}}, expectedSANs: []string{"127.0.0.1"}, - err: errors.New("IP Addresses claim failed"), + err: errors.New("certificate request does not contain the valid IP addresses"), } }, "fail/urisValidator": func() test { @@ -321,7 +321,7 @@ func Test_defaultSANsValidator_Valid(t *testing.T) { return test{ csr: &x509.CertificateRequest{URIs: []*url.URL{u1, u2}}, expectedSANs: []string{"urn:uuid:ddfe62ba-7e99-4bc1-83b3-8f57fe3e9959"}, - err: errors.New("URIs claim failed"), + err: errors.New("certificate request does not contain the valid URIs"), } }, "ok": func() test { @@ -512,7 +512,7 @@ func Test_forceCN_Option(t *testing.T) { Subject: pkix.Name{}, DNSNames: []string{}, }, - err: errors.New("Cannot force CN, DNSNames is empty"), + err: errors.New("cannot force common name, DNS names is empty"), } }, } diff --git a/authority/tls_test.go b/authority/tls_test.go index 03beb5c1..e61025a6 100644 --- a/authority/tls_test.go +++ b/authority/tls_test.go @@ -323,7 +323,7 @@ func TestAuthority_Sign(t *testing.T) { extraOpts: extraOpts, signOpts: signOpts, err: errors.New("authority.Sign: certificate request does not contain the valid DNS names - got [test.smallstep.com smallstep test], want [test.smallstep.com]"), - code: http.StatusUnauthorized, + code: http.StatusBadRequest, } }, "fail rsa key too short": func(t *testing.T) *signTest { @@ -348,8 +348,8 @@ ZYtQ9Ot36qc= csr: csr, extraOpts: extraOpts, signOpts: signOpts, - err: errors.New("authority.Sign: rsa key in CSR must be at least 2048 bits (256 bytes)"), - code: http.StatusUnauthorized, + err: errors.New("authority.Sign: certificate request RSA key must be at least 2048 bits (256 bytes)"), + code: http.StatusForbidden, } }, "fail store cert in db": func(t *testing.T) *signTest { diff --git a/ca/ca_test.go b/ca/ca_test.go index 1271659a..05dca027 100644 --- a/ca/ca_test.go +++ b/ca/ca_test.go @@ -200,8 +200,8 @@ ZEp7knvU2psWRw== return &signTest{ ca: ca, body: string(body), - status: http.StatusUnauthorized, - errMsg: errs.UnauthorizedDefaultMsg, + status: http.StatusBadRequest, + errMsg: errs.BadRequestPrefix, } }, "ok": func(t *testing.T) *signTest { From ff04873a2a60dbb7688fff955d2947cce9966ced Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 23 Nov 2021 18:58:16 -0800 Subject: [PATCH 08/13] Change the default error type to forbidden in Sign. The errors will also be propagated from sign options. --- authority/tls.go | 25 ++++++++++++++++++++----- authority/tls_test.go | 10 +++++----- 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/authority/tls.go b/authority/tls.go index 716d8956..7853198e 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -94,7 +94,10 @@ func (a *Authority) Sign(csr *x509.CertificateRequest, signOpts provisioner.Sign // Validate the given certificate request. case provisioner.CertificateRequestValidator: if err := k.Valid(csr); err != nil { - return nil, errs.Wrap(http.StatusUnauthorized, err, "authority.Sign", opts...) + return nil, errs.ApplyOptions( + errs.ForbiddenErr(err, "error validating certificate"), + opts..., + ) } // Validates the unsigned certificate template. @@ -131,26 +134,38 @@ func (a *Authority) Sign(csr *x509.CertificateRequest, signOpts provisioner.Sign // Set default subject if err := withDefaultASN1DN(a.config.AuthorityConfig.Template).Modify(leaf, signOpts); err != nil { - return nil, errs.Wrap(http.StatusUnauthorized, err, "authority.Sign", opts...) + return nil, errs.ApplyOptions( + errs.ForbiddenErr(err, "error creating certificate"), + opts..., + ) } for _, m := range certModifiers { if err := m.Modify(leaf, signOpts); err != nil { - return nil, errs.Wrap(http.StatusUnauthorized, err, "authority.Sign", opts...) + return nil, errs.ApplyOptions( + errs.ForbiddenErr(err, "error creating certificate"), + opts..., + ) } } // Certificate validation. for _, v := range certValidators { if err := v.Valid(leaf, signOpts); err != nil { - return nil, errs.Wrap(http.StatusUnauthorized, err, "authority.Sign", opts...) + return nil, errs.ApplyOptions( + errs.ForbiddenErr(err, "error validating certificate"), + opts..., + ) } } // Certificate modifiers after validation for _, m := range certEnforcers { if err := m.Enforce(leaf); err != nil { - return nil, errs.Wrap(http.StatusUnauthorized, err, "authority.Sign", opts...) + return nil, errs.ApplyOptions( + errs.ForbiddenErr(err, "error creating certificate"), + opts..., + ) } } diff --git a/authority/tls_test.go b/authority/tls_test.go index e61025a6..41354e8d 100644 --- a/authority/tls_test.go +++ b/authority/tls_test.go @@ -281,8 +281,8 @@ func TestAuthority_Sign(t *testing.T) { csr: csr, extraOpts: extraOpts, signOpts: signOpts, - err: errors.New("authority.Sign: default ASN1DN template cannot be nil"), - code: http.StatusUnauthorized, + err: errors.New("default ASN1DN template cannot be nil"), + code: http.StatusForbidden, } }, "fail create cert": func(t *testing.T) *signTest { @@ -309,7 +309,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 24h1m0s"), + err: errors.New("requested duration of 25h0m0s is more than the authorized maximum certificate duration of 24h1m0s"), code: http.StatusBadRequest, } }, @@ -322,7 +322,7 @@ func TestAuthority_Sign(t *testing.T) { csr: csr, extraOpts: extraOpts, signOpts: signOpts, - err: errors.New("authority.Sign: certificate request does not contain the valid DNS names - got [test.smallstep.com smallstep test], want [test.smallstep.com]"), + err: errors.New("certificate request does not contain the valid DNS names - got [test.smallstep.com smallstep test], want [test.smallstep.com]"), code: http.StatusBadRequest, } }, @@ -348,7 +348,7 @@ ZYtQ9Ot36qc= csr: csr, extraOpts: extraOpts, signOpts: signOpts, - err: errors.New("authority.Sign: certificate request RSA key must be at least 2048 bits (256 bytes)"), + err: errors.New("certificate request RSA key must be at least 2048 bits (256 bytes)"), code: http.StatusForbidden, } }, From c3f98fd04d201c7f65d1b6b8af9128677129f9f0 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 24 Nov 2021 11:32:35 -0800 Subject: [PATCH 09/13] Change some bad requests to forbidded. Change in the sign options bad requests to forbidded if is the provisioner the one adding a restriction, e.g. list of dns names, validity, ... --- authority/provisioner/sign_options.go | 32 +++++++++++------------ authority/provisioner/sign_ssh_options.go | 12 ++++----- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/authority/provisioner/sign_options.go b/authority/provisioner/sign_options.go index 167d371a..34b2e99b 100644 --- a/authority/provisioner/sign_options.go +++ b/authority/provisioner/sign_options.go @@ -85,19 +85,19 @@ type emailOnlyIdentity string func (e emailOnlyIdentity) Valid(req *x509.CertificateRequest) error { switch { case len(req.DNSNames) > 0: - return errs.BadRequest("certificate request cannot contain DNS names") + return errs.Forbidden("certificate request cannot contain DNS names") case len(req.IPAddresses) > 0: - return errs.BadRequest("certificate request cannot contain IP addresses") + return errs.Forbidden("certificate request cannot contain IP addresses") case len(req.URIs) > 0: - return errs.BadRequest("certificate request cannot contain URIs") + return errs.Forbidden("certificate request cannot contain URIs") case len(req.EmailAddresses) == 0: - return errs.BadRequest("certificate request does not contain any email address") + return errs.Forbidden("certificate request does not contain any email address") case len(req.EmailAddresses) > 1: - return errs.BadRequest("certificate request contains too many email addresses") + return errs.Forbidden("certificate request contains too many email addresses") case req.EmailAddresses[0] == "": - return errs.BadRequest("certificate request cannot contain an empty email address") + return errs.Forbidden("certificate request cannot contain an empty email address") case req.EmailAddresses[0] != string(e): - return errs.BadRequest("certificate request does not contain the valid email address - got %s, want %s", req.EmailAddresses[0], e) + return errs.Forbidden("certificate request does not contain the valid email address - got %s, want %s", req.EmailAddresses[0], e) default: return nil } @@ -162,7 +162,7 @@ func (v commonNameValidator) Valid(req *x509.CertificateRequest) error { return nil } if req.Subject.CommonName != string(v) { - return errs.BadRequest("certificate request does not contain the valid common name - got %s, want %s", req.Subject.CommonName, v) + return errs.Forbidden("certificate request does not contain the valid common name - got %s, want %s", req.Subject.CommonName, v) } return nil } @@ -180,7 +180,7 @@ func (v commonNameSliceValidator) Valid(req *x509.CertificateRequest) error { return nil } } - return errs.BadRequest("certificate request does not contain the valid common name - got %s, want %s", req.Subject.CommonName, v) + return errs.Forbidden("certificate request does not contain the valid common name - got %s, want %s", req.Subject.CommonName, v) } // dnsNamesValidator validates the DNS names SAN of a certificate request. @@ -201,7 +201,7 @@ func (v dnsNamesValidator) Valid(req *x509.CertificateRequest) error { got[s] = true } if !reflect.DeepEqual(want, got) { - return errs.BadRequest("certificate request does not contain the valid DNS names - got %v, want %v", req.DNSNames, v) + return errs.Forbidden("certificate request does not contain the valid DNS names - got %v, want %v", req.DNSNames, v) } return nil } @@ -224,7 +224,7 @@ func (v ipAddressesValidator) Valid(req *x509.CertificateRequest) error { got[ip.String()] = true } if !reflect.DeepEqual(want, got) { - return errs.BadRequest("certificate request does not contain the valid IP addresses - got %v, want %v", req.IPAddresses, v) + return errs.Forbidden("certificate request does not contain the valid IP addresses - got %v, want %v", req.IPAddresses, v) } return nil } @@ -247,7 +247,7 @@ func (v emailAddressesValidator) Valid(req *x509.CertificateRequest) error { got[s] = true } if !reflect.DeepEqual(want, got) { - return errs.BadRequest("certificate request does not contain the valid email addresses - got %v, want %v", req.EmailAddresses, v) + return errs.Forbidden("certificate request does not contain the valid email addresses - got %v, want %v", req.EmailAddresses, v) } return nil } @@ -270,7 +270,7 @@ func (v urisValidator) Valid(req *x509.CertificateRequest) error { got[u.String()] = true } if !reflect.DeepEqual(want, got) { - return errs.BadRequest("certificate request does not contain the valid URIs - got %v, want %v", req.URIs, v) + return errs.Forbidden("certificate request does not contain the valid URIs - got %v, want %v", req.URIs, v) } return nil } @@ -392,14 +392,14 @@ func (v *validityValidator) Valid(cert *x509.Certificate, o SignOptions) error { return errs.BadRequest("notAfter cannot be before notBefore; na=%v, nb=%v", na, nb) } if d < v.min { - return errs.BadRequest("requested duration of %v is less than the authorized minimum certificate duration of %v", d, v.min) + return errs.Forbidden("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 errs.BadRequest("requested duration of %v is more than the authorized maximum certificate duration of %v", d, v.max+o.Backdate) + return errs.Forbidden("requested duration of %v is more than the authorized maximum certificate duration of %v", d, v.max+o.Backdate) } return nil } @@ -432,7 +432,7 @@ func (o *forceCNOption) Modify(cert *x509.Certificate, _ SignOptions) error { // Force the common name to be the first DNS if not provided. if cert.Subject.CommonName == "" { if len(cert.DNSNames) == 0 { - return errs.Forbidden("cannot force common name, DNS names is empty") + return errs.BadRequest("cannot force common name, DNS names is empty") } cert.Subject.CommonName = cert.DNSNames[0] } diff --git a/authority/provisioner/sign_ssh_options.go b/authority/provisioner/sign_ssh_options.go index 30293947..01634dad 100644 --- a/authority/provisioner/sign_ssh_options.go +++ b/authority/provisioner/sign_ssh_options.go @@ -109,16 +109,16 @@ func (o SignSSHOptions) ModifyValidity(cert *ssh.Certificate) error { // ignores zero values. func (o SignSSHOptions) match(got SignSSHOptions) error { if o.CertType != "" && got.CertType != "" && o.CertType != got.CertType { - return errs.BadRequest("ssh certificate type does not match - got %v, want %v", got.CertType, o.CertType) + return errs.Forbidden("ssh certificate type does not match - got %v, want %v", got.CertType, o.CertType) } if len(o.Principals) > 0 && len(got.Principals) > 0 && !containsAllMembers(o.Principals, got.Principals) { - return errs.BadRequest("ssh certificate principals does not match - got %v, want %v", got.Principals, o.Principals) + return errs.Forbidden("ssh certificate principals does not match - got %v, want %v", got.Principals, o.Principals) } if !o.ValidAfter.IsZero() && !got.ValidAfter.IsZero() && !o.ValidAfter.Equal(&got.ValidAfter) { - return errs.BadRequest("ssh certificate validAfter does not match - got %v, want %v", got.ValidAfter, o.ValidAfter) + return errs.Forbidden("ssh certificate validAfter does not match - got %v, want %v", got.ValidAfter, o.ValidAfter) } if !o.ValidBefore.IsZero() && !got.ValidBefore.IsZero() && !o.ValidBefore.Equal(&got.ValidBefore) { - return errs.BadRequest("ssh certificate validBefore does not match - got %v, want %v", got.ValidBefore, o.ValidBefore) + return errs.Forbidden("ssh certificate validBefore does not match - got %v, want %v", got.ValidBefore, o.ValidBefore) } return nil } @@ -368,9 +368,9 @@ func (v *sshCertValidityValidator) Valid(cert *ssh.Certificate, opts SignSSHOpti switch { case dur < min: - return errs.BadRequest("requested duration of %s is less than minimum accepted duration for selected provisioner of %s", dur, min) + return errs.Forbidden("requested duration of %s is less than minimum accepted duration for selected provisioner of %s", dur, min) case dur > max+opts.Backdate: - return errs.BadRequest("requested duration of %s is greater than maximum accepted duration for selected provisioner of %s", dur, max+opts.Backdate) + return errs.Forbidden("requested duration of %s is greater than maximum accepted duration for selected provisioner of %s", dur, max+opts.Backdate) default: return nil } From d35848f7a98967c26bf7f78da948ffb223bda04a Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 24 Nov 2021 11:43:24 -0800 Subject: [PATCH 10/13] Fix unit tests. --- authority/tls_test.go | 4 ++-- ca/ca_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/authority/tls_test.go b/authority/tls_test.go index 41354e8d..1b5e4d01 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("requested duration of 25h0m0s is more than the authorized maximum certificate duration of 24h1m0s"), - code: http.StatusBadRequest, + code: http.StatusForbidden, } }, "fail validate sans when adding common name not in claims": func(t *testing.T) *signTest { @@ -323,7 +323,7 @@ func TestAuthority_Sign(t *testing.T) { extraOpts: extraOpts, signOpts: signOpts, err: errors.New("certificate request does not contain the valid DNS names - got [test.smallstep.com smallstep test], want [test.smallstep.com]"), - code: http.StatusBadRequest, + code: http.StatusForbidden, } }, "fail rsa key too short": func(t *testing.T) *signTest { diff --git a/ca/ca_test.go b/ca/ca_test.go index 05dca027..e4c35a90 100644 --- a/ca/ca_test.go +++ b/ca/ca_test.go @@ -200,8 +200,8 @@ ZEp7knvU2psWRw== return &signTest{ ca: ca, body: string(body), - status: http.StatusBadRequest, - errMsg: errs.BadRequestPrefix, + status: http.StatusForbidden, + errMsg: errs.ForbiddenPrefix, } }, "ok": func(t *testing.T) *signTest { From 9fd147f3da7a602a0c1500678b54340c040df749 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 2 Dec 2021 16:44:57 -0800 Subject: [PATCH 11/13] Change error message. --- api/sshRenew.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/sshRenew.go b/api/sshRenew.go index 8d07ba01..57b6f432 100644 --- a/api/sshRenew.go +++ b/api/sshRenew.go @@ -60,7 +60,7 @@ func (h *caHandler) SSHRenew(w http.ResponseWriter, r *http.Request) { newCert, err := h.Authority.RenewSSH(ctx, oldCert) if err != nil { - WriteError(w, errs.ForbiddenErr(err, "error signing ssh certificate")) + WriteError(w, errs.ForbiddenErr(err, "error renewing ssh certificate")) return } @@ -70,7 +70,7 @@ func (h *caHandler) SSHRenew(w http.ResponseWriter, r *http.Request) { identity, err := h.renewIdentityCertificate(r, notBefore, notAfter) if err != nil { - WriteError(w, errs.ForbiddenErr(err, "error signing identity certificate")) + WriteError(w, errs.ForbiddenErr(err, "error renewing identity certificate")) return } From 0cebde3db52d1f6a19982936a2983753fbc62684 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Fri, 3 Dec 2021 15:11:34 -0800 Subject: [PATCH 12/13] Change fallback message on RekeySSH. --- api/sshRekey.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/sshRekey.go b/api/sshRekey.go index b2c55509..8670f0bd 100644 --- a/api/sshRekey.go +++ b/api/sshRekey.go @@ -68,7 +68,7 @@ func (h *caHandler) SSHRekey(w http.ResponseWriter, r *http.Request) { newCert, err := h.Authority.RekeySSH(ctx, oldCert, publicKey, signOpts...) if err != nil { - WriteError(w, errs.ForbiddenErr(err, "error signing ssh certificate")) + WriteError(w, errs.ForbiddenErr(err, "error rekeying ssh certificate")) return } @@ -78,7 +78,7 @@ func (h *caHandler) SSHRekey(w http.ResponseWriter, r *http.Request) { identity, err := h.renewIdentityCertificate(r, notBefore, notAfter) if err != nil { - WriteError(w, errs.ForbiddenErr(err, "error signing identity certificate")) + WriteError(w, errs.ForbiddenErr(err, "error renewing identity certificate")) return } From e0fee84694be3ef42972e7261c38f6c94cc1b405 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Fri, 3 Dec 2021 15:24:42 -0800 Subject: [PATCH 13/13] Add comment about public key validator. --- authority/provisioner/sign_ssh_options.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/authority/provisioner/sign_ssh_options.go b/authority/provisioner/sign_ssh_options.go index 01634dad..a2ca78b1 100644 --- a/authority/provisioner/sign_ssh_options.go +++ b/authority/provisioner/sign_ssh_options.go @@ -414,6 +414,10 @@ func (v *sshCertDefaultValidator) Valid(cert *ssh.Certificate, o SignSSHOptions) type sshDefaultPublicKeyValidator struct{} // Valid checks that certificate request common name matches the one configured. +// +// TODO: this is the only validator that checks the key type. We should execute +// this before the signing. We should add a new validations interface or extend +// SSHCertOptionsValidator with the key. func (v sshDefaultPublicKeyValidator) Valid(cert *ssh.Certificate, o SignSSHOptions) error { if cert.Key == nil { return errs.BadRequest("ssh certificate key cannot be nil")