From 72e2c4eb2e7a943a59b8de401bf678ed6043e3da Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 21 Sep 2022 18:35:18 -0700 Subject: [PATCH] Render proper policy and constrains errors --- authority/internal/constraints/constraints.go | 17 +++++++++++--- authority/ssh.go | 14 +++--------- authority/tls.go | 22 +++++++++---------- policy/engine.go | 20 ++++++++++++++++- 4 files changed, 47 insertions(+), 26 deletions(-) diff --git a/authority/internal/constraints/constraints.go b/authority/internal/constraints/constraints.go index 29b5be56..2755e102 100644 --- a/authority/internal/constraints/constraints.go +++ b/authority/internal/constraints/constraints.go @@ -6,6 +6,8 @@ import ( "net" "net/http" "net/url" + + "github.com/smallstep/certificates/errs" ) var oidExtensionNameConstraints = []int{2, 5, 29, 30} @@ -23,9 +25,18 @@ func (e ConstraintError) Error() string { return e.Detail } -// StatusCode implements an status coder interface. -func (e ConstraintError) StatusCode() int { - return http.StatusForbidden +// As implements the As(any) bool interface and allows to use "errors.As()" to +// convert the ConstraintError to an errs.Error. +func (e ConstraintError) As(v any) bool { + if err, ok := v.(**errs.Error); ok { + *err = &errs.Error{ + Status: http.StatusForbidden, + Msg: e.Detail, + Err: e, + } + return true + } + return false } // Engine implements a constraint validator for DNS names, IP addresses, Email diff --git a/authority/ssh.go b/authority/ssh.go index 55861ce7..1b243b39 100644 --- a/authority/ssh.go +++ b/authority/ssh.go @@ -6,7 +6,6 @@ import ( "crypto/x509" "encoding/binary" "errors" - "fmt" "net/http" "strings" "time" @@ -20,7 +19,6 @@ import ( "github.com/smallstep/certificates/authority/provisioner" "github.com/smallstep/certificates/db" "github.com/smallstep/certificates/errs" - policy "github.com/smallstep/certificates/policy" "github.com/smallstep/certificates/templates" ) @@ -255,15 +253,9 @@ func (a *Authority) SignSSH(ctx context.Context, key ssh.PublicKey, opts provisi // Check if authority is allowed to sign the certificate if err := a.isAllowedToSignSSHCertificate(certTpl); err != nil { - var pe *policy.NamePolicyError - if errors.As(err, &pe) && pe.Reason == policy.NotAllowed { - return nil, &errs.Error{ - // NOTE: custom forbidden error, so that denied name is sent to client - // as well as shown in the logs. - Status: http.StatusForbidden, - Err: fmt.Errorf("authority not allowed to sign: %w", err), - Msg: fmt.Sprintf("The request was forbidden by the certificate authority: %s", err.Error()), - } + var ee *errs.Error + if errors.As(err, &ee) { + return nil, ee } return nil, errs.InternalServerErr(err, errs.WithMessage("authority.SignSSH: error creating ssh certificate"), diff --git a/authority/tls.go b/authority/tls.go index 5f0bdd26..4f5570b7 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -28,7 +28,6 @@ import ( casapi "github.com/smallstep/certificates/cas/apiv1" "github.com/smallstep/certificates/db" "github.com/smallstep/certificates/errs" - "github.com/smallstep/certificates/policy" ) // GetTLSOptions returns the tls options configured. @@ -213,15 +212,9 @@ func (a *Authority) Sign(csr *x509.CertificateRequest, signOpts provisioner.Sign // Check if authority is allowed to sign the certificate if err := a.isAllowedToSignX509Certificate(leaf); err != nil { - var pe *policy.NamePolicyError - if errors.As(err, &pe) && pe.Reason == policy.NotAllowed { - return nil, errs.ApplyOptions(&errs.Error{ - // NOTE: custom forbidden error, so that denied name is sent to client - // as well as shown in the logs. - Status: http.StatusForbidden, - Err: fmt.Errorf("authority not allowed to sign: %w", err), - Msg: fmt.Sprintf("The request was forbidden by the certificate authority: %s", err.Error()), - }, opts...) + var ee *errs.Error + if errors.As(err, &ee) { + return nil, ee } return nil, errs.InternalServerErr(err, errs.WithKeyVal("csr", csr), @@ -358,7 +351,14 @@ func (a *Authority) Rekey(oldCert *x509.Certificate, pk crypto.PublicKey) ([]*x5 // Check if the certificate is allowed to be renewed, policies or // constraints might change over time. if err := a.isAllowedToSignX509Certificate(newCert); err != nil { - return nil, err + var ee *errs.Error + if errors.As(err, &ee) { + return nil, ee + } + return nil, errs.InternalServerErr(err, + errs.WithKeyVal("serialNumber", oldCert.SerialNumber.String()), + errs.WithMessage("error renewing certificate"), + ) } resp, err := a.x509CAService.RenewCertificate(&casapi.RenewCertificateRequest{ diff --git a/policy/engine.go b/policy/engine.go index 4a0baa8f..c02fd7a9 100755 --- a/policy/engine.go +++ b/policy/engine.go @@ -4,11 +4,13 @@ import ( "crypto/x509" "fmt" "net" + "net/http" "net/url" + "go.step.sm/crypto/x509util" "golang.org/x/crypto/ssh" - "go.step.sm/crypto/x509util" + "github.com/smallstep/certificates/errs" ) type NamePolicyReason int @@ -62,6 +64,22 @@ func (e *NamePolicyError) Error() string { } } +// As implements the As(any) bool interface and allows to use "errors.As()" to +// convert a NotAllowed NamePolicyError to an errs.Error. +func (e *NamePolicyError) As(v any) bool { + if e.Reason == NotAllowed { + if err, ok := v.(**errs.Error); ok { + *err = &errs.Error{ + Status: http.StatusForbidden, + Msg: fmt.Sprintf("The request was forbidden by the certificate authority: %s", e.Error()), + Err: e, + } + return true + } + } + return false +} + func (e *NamePolicyError) Detail() string { return e.detail }