Merge branch 'herman/allow-deny-next' into herman/allow-deny

.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

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

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) {

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()

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)

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)

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)

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"

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

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) {