From 9e0edc7b50cec7311ce27f1a54ffdde2d7347f6e Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Thu, 24 Mar 2022 14:55:40 +0100 Subject: [PATCH] Add early authority policy evaluation to ACME order API --- .golangci.yml | 1 - acme/api/eab.go | 3 ++- acme/api/eab_test.go | 4 +++- acme/api/order.go | 24 ++++++++++++++++-------- acme/api/revoke_test.go | 4 ++++ acme/common.go | 1 + acme/order_test.go | 8 ++++++++ authority/policy.go | 3 +-- authority/provisioner/acme.go | 3 +++ authority/tls.go | 16 ++++++++++++++++ 10 files changed, 54 insertions(+), 13 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 59c58490..67aac2df 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -73,4 +73,3 @@ issues: - error strings should not be capitalized or end with punctuation or a newline - Wrapf call needs 1 arg but has 2 args - cs.NegotiatedProtocolIsMutual is deprecated - - rewrite if-else to switch statement diff --git a/acme/api/eab.go b/acme/api/eab.go index 3660d066..1780a173 100644 --- a/acme/api/eab.go +++ b/acme/api/eab.go @@ -4,8 +4,9 @@ import ( "context" "encoding/json" - "github.com/smallstep/certificates/acme" "go.step.sm/crypto/jose" + + "github.com/smallstep/certificates/acme" ) // ExternalAccountBinding represents the ACME externalAccountBinding JWS diff --git a/acme/api/eab_test.go b/acme/api/eab_test.go index dce9f36d..f9bce970 100644 --- a/acme/api/eab_test.go +++ b/acme/api/eab_test.go @@ -9,10 +9,12 @@ import ( "time" "github.com/pkg/errors" + + "go.step.sm/crypto/jose" + "github.com/smallstep/assert" "github.com/smallstep/certificates/acme" "github.com/smallstep/certificates/authority/provisioner" - "go.step.sm/crypto/jose" ) func Test_keysAreEqual(t *testing.T) { diff --git a/acme/api/order.go b/acme/api/order.go index 8fe37656..10f180a3 100644 --- a/acme/api/order.go +++ b/acme/api/order.go @@ -11,10 +11,12 @@ import ( "time" "github.com/go-chi/chi" + + "go.step.sm/crypto/randutil" + "github.com/smallstep/certificates/acme" "github.com/smallstep/certificates/api" "github.com/smallstep/certificates/authority/provisioner" - "go.step.sm/crypto/randutil" ) // NewOrderRequest represents the body for a NewOrder request. @@ -36,8 +38,8 @@ func (n *NewOrderRequest) Validate() error { if id.Type == acme.IP && net.ParseIP(id.Value) == nil { return acme.NewError(acme.ErrorMalformedType, "invalid IP address: %s", id.Value) } - // TODO: add some validations for DNS domains? - // TODO: combine the errors from this with allow/deny policy, like example error in https://datatracker.ietf.org/doc/html/rfc8555#section-6.7.1 + // TODO(hs): add some validations for DNS domains? + // TODO(hs): combine the errors from this with allow/deny policy, like example error in https://datatracker.ietf.org/doc/html/rfc8555#section-6.7.1 } return nil } @@ -99,23 +101,29 @@ func (h *Handler) NewOrder(w http.ResponseWriter, r *http.Request) { return } - // TODO(hs): this should also verify rules set in the Account (i.e. allowed/denied - // DNS and IPs; it's probably good to connect those to the EAB credentials and management? Or + // TODO(hs): the policy evaluation below should also verify rules set in the Account (i.e. allowed/denied + // DNS and IPs). It's probably good to connect those to the EAB credentials and management? Or // should we do it fully properly and connect them to the Account directly? The latter would allow // management of allowed/denied names based on just the name, without having bound to EAB. Still, // EAB is not illogical, because that's the way Accounts are connected to an external system and // thus make sense to also set the allowed/denied names based on that info. - // TODO: also perform check on the authority level here already, so that challenges are not performed - // and after that the CA fails to sign it. (i.e. h.ca function?) + // TODO(hs): gather all errors, so that we can build one response with subproblems; include the nor.Validate() + // error here too, like in example? for _, identifier := range nor.Identifiers { - // TODO: gather all errors, so that we can build subproblems; include the nor.Validate() error here too, like in example? + // evaluate the provisioner level policy orderIdentifier := provisioner.ACMEIdentifier{Type: provisioner.ACMEIdentifierType(identifier.Type), Value: identifier.Value} err = prov.AuthorizeOrderIdentifier(ctx, orderIdentifier) if err != nil { api.WriteError(w, acme.WrapError(acme.ErrorRejectedIdentifierType, err, "not authorized")) return } + // evaluate the authority level policy + err = h.ca.AreSANsAllowed(ctx, []string{identifier.Value}) + if err != nil { + api.WriteError(w, acme.WrapError(acme.ErrorRejectedIdentifierType, err, "not authorized")) + return + } } now := clock.Now() diff --git a/acme/api/revoke_test.go b/acme/api/revoke_test.go index 4ff54405..aa3dda10 100644 --- a/acme/api/revoke_test.go +++ b/acme/api/revoke_test.go @@ -282,6 +282,10 @@ func (m *mockCA) Sign(cr *x509.CertificateRequest, opts provisioner.SignOptions, return nil, nil } +func (m *mockCA) AreSANsAllowed(ctx context.Context, sans []string) error { + return nil +} + func (m *mockCA) IsRevoked(sn string) (bool, error) { if m.MockIsRevoked != nil { return m.MockIsRevoked(sn) diff --git a/acme/common.go b/acme/common.go index 9c5e732a..e0d96deb 100644 --- a/acme/common.go +++ b/acme/common.go @@ -12,6 +12,7 @@ import ( // CertificateAuthority is the interface implemented by a CA authority. type CertificateAuthority interface { Sign(cr *x509.CertificateRequest, opts provisioner.SignOptions, signOpts ...provisioner.SignOption) ([]*x509.Certificate, error) + AreSANsAllowed(ctx context.Context, sans []string) error IsRevoked(sn string) (bool, error) Revoke(context.Context, *authority.RevokeOptions) error LoadProvisionerByName(string) (provisioner.Interface, error) diff --git a/acme/order_test.go b/acme/order_test.go index 493b40b7..f1f28e40 100644 --- a/acme/order_test.go +++ b/acme/order_test.go @@ -268,6 +268,7 @@ func TestOrder_UpdateStatus(t *testing.T) { type mockSignAuth struct { sign func(csr *x509.CertificateRequest, signOpts provisioner.SignOptions, extraOpts ...provisioner.SignOption) ([]*x509.Certificate, error) + areSANsAllowed func(ctx context.Context, sans []string) error loadProvisionerByName func(string) (provisioner.Interface, error) ret1, ret2 interface{} err error @@ -282,6 +283,13 @@ func (m *mockSignAuth) Sign(csr *x509.CertificateRequest, signOpts provisioner.S return []*x509.Certificate{m.ret1.(*x509.Certificate), m.ret2.(*x509.Certificate)}, m.err } +func (m *mockSignAuth) AreSANsAllowed(ctx context.Context, sans []string) error { + if m.areSANsAllowed != nil { + return m.areSANsAllowed(ctx, sans) + } + return m.err +} + func (m *mockSignAuth) LoadProvisionerByName(name string) (provisioner.Interface, error) { if m.loadProvisionerByName != nil { return m.loadProvisionerByName(name) diff --git a/authority/policy.go b/authority/policy.go index 88f301e0..4f93899f 100644 --- a/authority/policy.go +++ b/authority/policy.go @@ -2,10 +2,9 @@ package authority import ( "context" + "errors" "fmt" - "github.com/pkg/errors" - "go.step.sm/linkedca" "github.com/smallstep/certificates/authority/admin" diff --git a/authority/provisioner/acme.go b/authority/provisioner/acme.go index ffd9cb38..2bcaeef2 100644 --- a/authority/provisioner/acme.go +++ b/authority/provisioner/acme.go @@ -3,6 +3,7 @@ package provisioner import ( "context" "crypto/x509" + "fmt" "net" "time" @@ -126,6 +127,8 @@ func (p *ACME) AuthorizeOrderIdentifier(ctx context.Context, identifier ACMEIden _, err = p.x509Policy.IsIPAllowed(net.ParseIP(identifier.Value)) case DNS: _, err = p.x509Policy.IsDNSAllowed(identifier.Value) + default: + err = fmt.Errorf("invalid ACME identifier type '%s' provided", identifier.Type) } return err diff --git a/authority/tls.go b/authority/tls.go index df38091c..867e2c51 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -245,6 +245,22 @@ func (a *Authority) isAllowedToSign(cert *x509.Certificate) (bool, error) { return a.x509Policy.AreCertificateNamesAllowed(cert) } +// AreSANsAllowed evaluates the provided sans against the +// authority X.509 policy. +func (a *Authority) AreSANsAllowed(ctx context.Context, sans []string) error { + + // no policy configured; return early + if a.x509Policy == nil { + return nil + } + + // evaluate the X.509 policy for the provided sans + var err error + _, err = a.x509Policy.AreSANsAllowed(sans) + + return err +} + // Renew creates a new Certificate identical to the old certificate, except // with a validity window that begins 'now'. func (a *Authority) Renew(oldCert *x509.Certificate) ([]*x509.Certificate, error) {