Merge logic for X509 and SSH policy

authority/provisioner/policy.go

@@ -1,70 +1,69 @@
 package provisioner
 
 import (
-	sshpolicy "github.com/smallstep/certificates/policy/ssh"
+	"github.com/smallstep/certificates/policy"
-	x509policy "github.com/smallstep/certificates/policy/x509"
 )
 
 // newX509PolicyEngine creates a new x509 name policy engine
-func newX509PolicyEngine(x509Opts *X509Options) (*x509policy.NamePolicyEngine, error) {
+func newX509PolicyEngine(x509Opts *X509Options) (policy.X509NamePolicyEngine, error) {
 
 	if x509Opts == nil {
 		return nil, nil
 	}
 
-	options := []x509policy.NamePolicyOption{
+	options := []policy.NamePolicyOption{
-		x509policy.WithSubjectCommonNameVerification(), // enable x509 Subject Common Name validation by default
+		policy.WithSubjectCommonNameVerification(), // enable x509 Subject Common Name validation by default
 	}
 
 	allowed := x509Opts.GetAllowedNameOptions()
 	if allowed != nil && allowed.HasNames() {
 		options = append(options,
-			x509policy.WithPermittedDNSDomains(allowed.DNSDomains),
+			policy.WithPermittedDNSDomains(allowed.DNSDomains),
-			x509policy.WithPermittedCIDRs(allowed.IPRanges), // TODO(hs): support IPs in addition to ranges
+			policy.WithPermittedCIDRs(allowed.IPRanges), // TODO(hs): support IPs in addition to ranges
-			x509policy.WithPermittedEmailAddresses(allowed.EmailAddresses),
+			policy.WithPermittedEmailAddresses(allowed.EmailAddresses),
-			x509policy.WithPermittedURIDomains(allowed.URIDomains),
+			policy.WithPermittedURIDomains(allowed.URIDomains),
 		)
 	}
 
 	denied := x509Opts.GetDeniedNameOptions()
 	if denied != nil && denied.HasNames() {
 		options = append(options,
-			x509policy.WithExcludedDNSDomains(denied.DNSDomains),
+			policy.WithExcludedDNSDomains(denied.DNSDomains),
-			x509policy.WithExcludedCIDRs(denied.IPRanges), // TODO(hs): support IPs in addition to ranges
+			policy.WithExcludedCIDRs(denied.IPRanges), // TODO(hs): support IPs in addition to ranges
-			x509policy.WithExcludedEmailAddresses(denied.EmailAddresses),
+			policy.WithExcludedEmailAddresses(denied.EmailAddresses),
-			x509policy.WithExcludedURIDomains(denied.URIDomains),
+			policy.WithExcludedURIDomains(denied.URIDomains),
 		)
 	}
 
-	return x509policy.New(options...)
+	return policy.New(options...)
 }
 
 // newSSHPolicyEngine creates a new SSH name policy engine
-func newSSHPolicyEngine(sshOpts *SSHOptions) (*sshpolicy.NamePolicyEngine, error) {
+func newSSHPolicyEngine(sshOpts *SSHOptions) (policy.SSHNamePolicyEngine, error) {
 
 	if sshOpts == nil {
 		return nil, nil
 	}
 
-	options := []sshpolicy.NamePolicyOption{}
+	options := []policy.NamePolicyOption{}
 
 	allowed := sshOpts.GetAllowedNameOptions()
 	if allowed != nil && allowed.HasNames() {
 		options = append(options,
-			sshpolicy.WithPermittedDNSDomains(allowed.DNSDomains), // TODO(hs): be a bit more lenient w.r.t. the format of domains? I.e. allow "*.localhost" instead of the ".localhost", which is what Name Constraints do.
+			policy.WithPermittedDNSDomains(allowed.DNSDomains), // TODO(hs): be a bit more lenient w.r.t. the format of domains? I.e. allow "*.localhost" instead of the ".localhost", which is what Name Constraints do.
-			sshpolicy.WithPermittedEmailAddresses(allowed.EmailAddresses),
+			policy.WithPermittedEmailAddresses(allowed.EmailAddresses),
-			sshpolicy.WithPermittedPrincipals(allowed.Principals),
+			policy.WithPermittedPrincipals(allowed.Principals),
 		)
 	}
 
 	denied := sshOpts.GetDeniedNameOptions()
 	if denied != nil && denied.HasNames() {
 		options = append(options,
-			sshpolicy.WithExcludedDNSDomains(denied.DNSDomains), // TODO(hs): be a bit more lenient w.r.t. the format of domains? I.e. allow "*.localhost" instead of the ".localhost", which is what Name Constraints do.
+			policy.WithExcludedDNSDomains(denied.DNSDomains), // TODO(hs): be a bit more lenient w.r.t. the format of domains? I.e. allow "*.localhost" instead of the ".localhost", which is what Name Constraints do.
-			sshpolicy.WithExcludedEmailAddresses(denied.EmailAddresses),
+			policy.WithExcludedEmailAddresses(denied.EmailAddresses),
-			sshpolicy.WithExcludedPrincipals(denied.Principals),
+			policy.WithExcludedPrincipals(denied.Principals),
 		)
 	}
 
-	return sshpolicy.New(options...)
+	return policy.New(options...)
 }

authority/provisioner/provisioner.go

@@ -12,8 +12,7 @@ import (
 	"github.com/pkg/errors"
 	"github.com/smallstep/certificates/db"
 	"github.com/smallstep/certificates/errs"
-	sshpolicy "github.com/smallstep/certificates/policy/ssh"
+	"github.com/smallstep/certificates/policy"
-	x509policy "github.com/smallstep/certificates/policy/x509"
 	"golang.org/x/crypto/ssh"
 )
 
@@ -307,8 +306,8 @@ func SanitizeSSHUserPrincipal(email string) string {
 }
 
 type base struct {
-	x509PolicyEngine *x509policy.NamePolicyEngine
+	x509PolicyEngine policy.X509NamePolicyEngine
-	sshPolicyEngine  *sshpolicy.NamePolicyEngine
+	sshPolicyEngine  policy.SSHNamePolicyEngine
 }
 
 // AuthorizeSign returns an unimplemented error. Provisioners should overwrite

authority/provisioner/sign_options.go

@@ -16,7 +16,7 @@ import (
 
 	"github.com/pkg/errors"
 	"github.com/smallstep/certificates/errs"
-	x509policy "github.com/smallstep/certificates/policy/x509"
+	"github.com/smallstep/certificates/policy"
 	"go.step.sm/crypto/keyutil"
 	"go.step.sm/crypto/x509util"
 )
@@ -408,11 +408,11 @@ func (v *validityValidator) Valid(cert *x509.Certificate, o SignOptions) error {
 // x509NamePolicyValidator validates that the certificate (to be signed)
 // contains only allowed SANs.
 type x509NamePolicyValidator struct {
-	policyEngine *x509policy.NamePolicyEngine
+	policyEngine policy.X509NamePolicyEngine
 }
 
 // newX509NamePolicyValidator return a new SANs allow/deny validator.
-func newX509NamePolicyValidator(engine *x509policy.NamePolicyEngine) *x509NamePolicyValidator {
+func newX509NamePolicyValidator(engine policy.X509NamePolicyEngine) *x509NamePolicyValidator {
 	return &x509NamePolicyValidator{
 		policyEngine: engine,
 	}

authority/provisioner/sign_ssh_options.go

@@ -10,7 +10,7 @@ import (
 
 	"github.com/pkg/errors"
 	"github.com/smallstep/certificates/errs"
-	sshpolicy "github.com/smallstep/certificates/policy/ssh"
+	"github.com/smallstep/certificates/policy"
 	"go.step.sm/crypto/keyutil"
 	"golang.org/x/crypto/ssh"
 )
@@ -448,11 +448,11 @@ func (v sshDefaultPublicKeyValidator) Valid(cert *ssh.Certificate, o SignSSHOpti
 // sshNamePolicyValidator validates that the certificate (to be signed)
 // contains only allowed principals.
 type sshNamePolicyValidator struct {
-	policyEngine *sshpolicy.NamePolicyEngine
+	policyEngine policy.SSHNamePolicyEngine
 }
 
 // newSSHNamePolicyValidator return a new SSH allow/deny validator.
-func newSSHNamePolicyValidator(engine *sshpolicy.NamePolicyEngine) *sshNamePolicyValidator {
+func newSSHNamePolicyValidator(engine policy.SSHNamePolicyEngine) *sshNamePolicyValidator {
 	return &sshNamePolicyValidator{
 		policyEngine: engine,
 	}

policy/engine.go

@@ -1,4 +1,4 @@
-package x509policy
+package policy
 
 import (
 	"bytes"
@@ -12,6 +12,7 @@ import (
 
 	"github.com/pkg/errors"
 	"go.step.sm/crypto/x509util"
+	"golang.org/x/crypto/ssh"
 )
 
 type CertificateInvalidError struct {
@@ -47,7 +48,7 @@ func (e CertificateInvalidError) Error() string {
 
 // NamePolicyEngine can be used to check that a CSR or Certificate meets all allowed and
 // denied names before a CA creates and/or signs the Certificate.
-// TODO(hs): the x509 RFC also defines name checks on directory name; support that?
+// TODO(hs): the X509 RFC also defines name checks on directory name; support that?
 // TODO(hs): implement Stringer interface: describe the contents of the NamePolicyEngine?
 type NamePolicyEngine struct {
 
@@ -65,12 +66,15 @@ type NamePolicyEngine struct {
 	excludedEmailAddresses  []string
 	permittedURIDomains     []string
 	excludedURIDomains      []string
+	permittedPrincipals     []string
+	excludedPrincipals      []string
 
 	// some internal counts for housekeeping
 	numberOfDNSDomainConstraints      int
 	numberOfIPRangeConstraints        int
 	numberOfEmailAddressConstraints   int
 	numberOfURIDomainConstraints      int
+	numberOfPrincipalConstraints      int
 	totalNumberOfPermittedConstraints int
 	totalNumberOfExcludedConstraints  int
 	totalNumberOfConstraints          int
@@ -90,22 +94,25 @@ func New(opts ...NamePolicyOption) (*NamePolicyEngine, error) {
 	e.permittedIPRanges = removeDuplicateIPRanges(e.permittedIPRanges)
 	e.permittedEmailAddresses = removeDuplicates(e.permittedEmailAddresses)
 	e.permittedURIDomains = removeDuplicates(e.permittedURIDomains)
+	e.permittedPrincipals = removeDuplicates(e.permittedPrincipals)
 
 	e.excludedDNSDomains = removeDuplicates(e.excludedDNSDomains)
 	e.excludedIPRanges = removeDuplicateIPRanges(e.excludedIPRanges)
 	e.excludedEmailAddresses = removeDuplicates(e.excludedEmailAddresses)
 	e.excludedURIDomains = removeDuplicates(e.excludedURIDomains)
+	e.excludedPrincipals = removeDuplicates(e.excludedPrincipals)
 
 	e.numberOfDNSDomainConstraints = len(e.permittedDNSDomains) + len(e.excludedDNSDomains)
 	e.numberOfIPRangeConstraints = len(e.permittedIPRanges) + len(e.excludedIPRanges)
 	e.numberOfEmailAddressConstraints = len(e.permittedEmailAddresses) + len(e.excludedEmailAddresses)
 	e.numberOfURIDomainConstraints = len(e.permittedURIDomains) + len(e.excludedURIDomains)
+	e.numberOfPrincipalConstraints = len(e.permittedPrincipals) + len(e.excludedPrincipals)
 
 	e.totalNumberOfPermittedConstraints = len(e.permittedDNSDomains) + len(e.permittedIPRanges) +
-		len(e.permittedEmailAddresses) + len(e.permittedURIDomains)
+		len(e.permittedEmailAddresses) + len(e.permittedURIDomains) + len(e.permittedPrincipals)
 
 	e.totalNumberOfExcludedConstraints = len(e.excludedDNSDomains) + len(e.excludedIPRanges) +
-		len(e.excludedEmailAddresses) + len(e.excludedURIDomains)
+		len(e.excludedEmailAddresses) + len(e.excludedURIDomains) + len(e.excludedPrincipals)
 
 	e.totalNumberOfConstraints = e.totalNumberOfPermittedConstraints + e.totalNumberOfExcludedConstraints
 
@@ -151,7 +158,7 @@ func (e *NamePolicyEngine) AreCertificateNamesAllowed(cert *x509.Certificate) (b
 	if e.verifySubjectCommonName {
 		appendSubjectCommonName(cert.Subject, &dnsNames, &ips, &emails, &uris)
 	}
-	if err := e.validateNames(dnsNames, ips, emails, uris); err != nil {
+	if err := e.validateNames(dnsNames, ips, emails, uris, []string{}); err != nil {
 		return false, err
 	}
 	return true, nil
@@ -165,7 +172,7 @@ func (e *NamePolicyEngine) AreCSRNamesAllowed(csr *x509.CertificateRequest) (boo
 	if e.verifySubjectCommonName {
 		appendSubjectCommonName(csr.Subject, &dnsNames, &ips, &emails, &uris)
 	}
-	if err := e.validateNames(dnsNames, ips, emails, uris); err != nil {
+	if err := e.validateNames(dnsNames, ips, emails, uris, []string{}); err != nil {
 		return false, err
 	}
 	return true, nil
@@ -175,7 +182,7 @@ func (e *NamePolicyEngine) AreCSRNamesAllowed(csr *x509.CertificateRequest) (boo
 // The SANs are first split into DNS names, IPs, email addresses and URIs.
 func (e *NamePolicyEngine) AreSANsAllowed(sans []string) (bool, error) {
 	dnsNames, ips, emails, uris := x509util.SplitSANs(sans)
-	if err := e.validateNames(dnsNames, ips, emails, uris); err != nil {
+	if err := e.validateNames(dnsNames, ips, emails, uris, []string{}); err != nil {
 		return false, err
 	}
 	return true, nil
@@ -183,7 +190,7 @@ func (e *NamePolicyEngine) AreSANsAllowed(sans []string) (bool, error) {
 
 // IsDNSAllowed verifies a single DNS domain is allowed.
 func (e *NamePolicyEngine) IsDNSAllowed(dns string) (bool, error) {
-	if err := e.validateNames([]string{dns}, []net.IP{}, []string{}, []*url.URL{}); err != nil {
+	if err := e.validateNames([]string{dns}, []net.IP{}, []string{}, []*url.URL{}, []string{}); err != nil {
 		return false, err
 	}
 	return true, nil
@@ -191,7 +198,16 @@ func (e *NamePolicyEngine) IsDNSAllowed(dns string) (bool, error) {
 
 // IsIPAllowed verifies a single IP domain is allowed.
 func (e *NamePolicyEngine) IsIPAllowed(ip net.IP) (bool, error) {
-	if err := e.validateNames([]string{}, []net.IP{ip}, []string{}, []*url.URL{}); err != nil {
+	if err := e.validateNames([]string{}, []net.IP{ip}, []string{}, []*url.URL{}, []string{}); err != nil {
+		return false, err
+	}
+	return true, nil
+}
+
+// ArePrincipalsAllowed verifies that all principals in an SSH certificate are allowed.
+func (e *NamePolicyEngine) ArePrincipalsAllowed(cert *ssh.Certificate) (bool, error) {
+	dnsNames, emails, usernames := splitPrincipals(cert.ValidPrincipals)
+	if err := e.validateNames(dnsNames, []net.IP{}, emails, []*url.URL{}, usernames); err != nil {
 		return false, err
 	}
 	return true, nil
@@ -217,10 +233,27 @@ func appendSubjectCommonName(subject pkix.Name, dnsNames *[]string, ips *[]net.I
 	}
 }
 
+// splitPrincipals splits SSH certificate principals into DNS names, emails and user names.
+func splitPrincipals(principals []string) (dnsNames, emails, usernames []string) {
+	dnsNames = []string{}
+	emails = []string{}
+	usernames = []string{}
+	for _, principal := range principals {
+		if strings.Contains(principal, "@") {
+			emails = append(emails, principal)
+		} else if len(strings.Split(principal, ".")) > 1 {
+			dnsNames = append(dnsNames, principal)
+		} else {
+			usernames = append(usernames, principal)
+		}
+	}
+	return
+}
+
 // validateNames verifies that all names are allowed.
 // Its logic follows that of (a large part of) the (c *Certificate) isValid() function
 // in https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go
-func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailAddresses []string, uris []*url.URL) error {
+func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailAddresses []string, uris []*url.URL, usernames []string) error {
 
 	// nothing to compare against; return early
 	if e.totalNumberOfConstraints == 0 {
@@ -309,6 +342,34 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA
 		}
 	}
 
+	//"dns": ["*.smallstep.com"],
+	//"email": ["@smallstep.com", "@google.com"],
+	//"principal": ["max", "mariano", "mike"]
+	/* No regexes for now. But if we ever implement them, they'd probably look like this */
+	/*"principal": ["foo.smallstep.com", "/^*\.smallstep\.com$/"]*/
+
+	// Principals can be single user names (mariano, max, mike, ...), hostnames/domains (*.smallstep.com, host.smallstep.com, ...) and "emails" (max@smallstep.com, @smallstep.com, ...)
+	// All ValidPrincipals can thus be any one of those, and they can be mixed (mike@smallstep.com, mike, ...); we need to split this?
+	// Should we assume a generic engine, or can we do it host vs. user based? If host vs. user based, then it becomes easier w.r.t. dns; hosts will only be DNS, right?
+	// If we assume generic, we _may_ have a harder time distinguishing host vs. user certs. We propose to use host + user specific provisioners, though...
+	// Perhaps we can do some heuristics on the principal names vs. hostnames (i.e. when only a single label and no dot, then it's a user principal)
+
+	for _, username := range usernames {
+		if e.numberOfPrincipalConstraints == 0 && e.totalNumberOfPermittedConstraints > 0 {
+			return CertificateInvalidError{
+				Reason: x509.CANotAuthorizedForThisName,
+				Detail: fmt.Sprintf("username principal %q is not permitted by any constraint", username),
+			}
+		}
+		// TODO: some validation? I.e. allowed characters?
+		if err := checkNameConstraints("username", username, username,
+			func(parsedName, constraint interface{}) (bool, error) {
+				return matchUsernameConstraint(parsedName.(string), constraint.(string))
+			}, e.permittedPrincipals, e.excludedPrincipals); err != nil {
+			return err
+		}
+	}
+
 	// TODO: when the error is not nil and returned up in the above, we can add
 	// additional context to it (i.e. the cert or csr that was inspected).
 
@@ -753,3 +814,8 @@ func (e *NamePolicyEngine) matchURIConstraint(uri *url.URL, constraint string) (
 
 	return e.matchDomainConstraint(host, constraint)
 }
+
+// matchUsernameConstraint performs a string literal match against a constraint.
+func matchUsernameConstraint(username, constraint string) (bool, error) {
+	return strings.EqualFold(username, constraint), nil
+}

policy/engine_test.go

@@ -1,4 +1,4 @@
-package x509policy
+package policy
 
 import (
 	"crypto/x509"

policy/options.go

@@ -1,4 +1,4 @@
-package x509policy
+package policy
 
 import (
 	"fmt"
@@ -538,6 +538,26 @@ func AddExcludedURIDomain(uriDomain string) NamePolicyOption {
 	}
 }
 
+func WithPermittedPrincipals(principals []string) NamePolicyOption {
+	return func(g *NamePolicyEngine) error {
+		// for _, principal := range principals {
+		// 	// TODO: validation?
+		// }
+		g.permittedPrincipals = principals
+		return nil
+	}
+}
+
+func WithExcludedPrincipals(principals []string) NamePolicyOption {
+	return func(g *NamePolicyEngine) error {
+		// for _, principal := range principals {
+		// 	// TODO: validation?
+		// }
+		g.excludedPrincipals = principals
+		return nil
+	}
+}
+
 func normalizeAndValidateDNSDomainConstraint(constraint string) (string, error) {
 	normalizedConstraint := strings.TrimSpace(constraint)
 	if strings.Contains(normalizedConstraint, "..") {

policy/options_test.go

@@ -1,4 +1,4 @@
-package x509policy
+package policy
 
 import (
 	"net"

policy/ssh.go

@@ -0,0 +1,9 @@
+package policy
+
+import (
+	"golang.org/x/crypto/ssh"
+)
+
+type SSHNamePolicyEngine interface {
+	ArePrincipalsAllowed(cert *ssh.Certificate) (bool, error)
+}

policy/ssh/options.go

@@ -1,99 +0,0 @@
-package sshpolicy
-
-import (
-	"fmt"
-	"strings"
-
-	"github.com/pkg/errors"
-)
-
-type NamePolicyOption func(g *NamePolicyEngine) error
-
-func WithPermittedDNSDomains(domains []string) NamePolicyOption {
-	return func(g *NamePolicyEngine) error {
-		for _, domain := range domains {
-			if err := validateDNSDomainConstraint(domain); err != nil {
-				return errors.Errorf("cannot parse permitted domain constraint %q", domain)
-			}
-		}
-		g.permittedDNSDomains = domains
-		return nil
-	}
-}
-
-func WithExcludedDNSDomains(domains []string) NamePolicyOption {
-	return func(g *NamePolicyEngine) error {
-		for _, domain := range domains {
-			if err := validateDNSDomainConstraint(domain); err != nil {
-				return errors.Errorf("cannot parse excluded domain constraint %q", domain)
-			}
-		}
-		g.excludedDNSDomains = domains
-		return nil
-	}
-}
-
-func WithPermittedEmailAddresses(emailAddresses []string) NamePolicyOption {
-	return func(g *NamePolicyEngine) error {
-		for _, email := range emailAddresses {
-			if err := validateEmailConstraint(email); err != nil {
-				return err
-			}
-		}
-		g.permittedEmailAddresses = emailAddresses
-		return nil
-	}
-}
-
-func WithExcludedEmailAddresses(emailAddresses []string) NamePolicyOption {
-	return func(g *NamePolicyEngine) error {
-		for _, email := range emailAddresses {
-			if err := validateEmailConstraint(email); err != nil {
-				return err
-			}
-		}
-		g.excludedEmailAddresses = emailAddresses
-		return nil
-	}
-}
-
-func WithPermittedPrincipals(principals []string) NamePolicyOption {
-	return func(g *NamePolicyEngine) error {
-		// for _, principal := range principals {
-		// 	// TODO: validation?
-		// }
-		g.permittedPrincipals = principals
-		return nil
-	}
-}
-
-func WithExcludedPrincipals(principals []string) NamePolicyOption {
-	return func(g *NamePolicyEngine) error {
-		// for _, principal := range principals {
-		// 	// TODO: validation?
-		// }
-		g.excludedPrincipals = principals
-		return nil
-	}
-}
-
-func validateDNSDomainConstraint(domain string) error {
-	if _, ok := domainToReverseLabels(domain); !ok {
-		return errors.Errorf("cannot parse permitted domain constraint %q", domain)
-	}
-	return nil
-}
-
-func validateEmailConstraint(constraint string) error {
-	if strings.Contains(constraint, "@") {
-		_, ok := parseRFC2821Mailbox(constraint)
-		if !ok {
-			return fmt.Errorf("cannot parse email constraint %q", constraint)
-		}
-	}
-	_, ok := domainToReverseLabels(constraint)
-	if !ok {
-		return fmt.Errorf("cannot parse email domain constraint %q", constraint)
-	}
-	return nil
-}

policy/ssh/ssh.go

@@ -1,472 +0,0 @@
-package sshpolicy
-
-import (
-	"bytes"
-	"crypto/x509"
-	"fmt"
-	"reflect"
-	"strings"
-
-	"github.com/pkg/errors"
-	"golang.org/x/crypto/ssh"
-)
-
-type CertificateInvalidError struct {
-	Reason x509.InvalidReason
-	Detail string
-}
-
-func (e CertificateInvalidError) Error() string {
-	switch e.Reason {
-	// TODO: include logical errors for this package; exlude ones that don't make sense for its current use case?
-	// TODO: currently only CANotAuthorizedForThisName is used by this package; we're not checking the other things in CSRs in this package.
-	case x509.NotAuthorizedToSign:
-		return "not authorized to sign other certificates" // TODO: this one doesn't make sense for this pkg
-	case x509.Expired:
-		return "csr has expired or is not yet valid: " + e.Detail
-	case x509.CANotAuthorizedForThisName:
-		return "not authorized to sign for this name: " + e.Detail
-	case x509.CANotAuthorizedForExtKeyUsage:
-		return "not authorized for an extended key usage: " + e.Detail
-	case x509.TooManyIntermediates:
-		return "too many intermediates for path length constraint"
-	case x509.IncompatibleUsage:
-		return "csr specifies an incompatible key usage"
-	case x509.NameMismatch:
-		return "issuer name does not match subject from issuing certificate"
-	case x509.NameConstraintsWithoutSANs:
-		return "issuer has name constraints but csr doesn't have a SAN extension"
-	case x509.UnconstrainedName:
-		return "issuer has name constraints but csr contains unknown or unconstrained name: " + e.Detail
-	}
-	return "unknown error"
-}
-
-type NamePolicyEngine struct {
-	options                 []NamePolicyOption
-	permittedDNSDomains     []string
-	excludedDNSDomains      []string
-	permittedEmailAddresses []string
-	excludedEmailAddresses  []string
-	permittedPrincipals     []string // TODO: rename to usernames, as principals can be host, user@ (like mail) and usernames?
-	excludedPrincipals      []string
-}
-
-func New(opts ...NamePolicyOption) (*NamePolicyEngine, error) {
-
-	e := &NamePolicyEngine{} // TODO: embed an x509 engine instead of building it again?
-	e.options = append(e.options, opts...)
-	for _, option := range e.options {
-		if err := option(e); err != nil {
-			return nil, err
-		}
-	}
-
-	return e, nil
-}
-
-func (e *NamePolicyEngine) ArePrincipalsAllowed(cert *ssh.Certificate) (bool, error) {
-	dnsNames, emails, userNames := splitPrincipals(cert.ValidPrincipals)
-	if err := e.validateNames(dnsNames, emails, userNames); err != nil {
-		return false, err
-	}
-	return true, nil
-}
-
-func (e *NamePolicyEngine) validateNames(dnsNames, emails, userNames []string) error {
-	//"dns": ["*.smallstep.com"],
-	//"email": ["@smallstep.com", "@google.com"],
-	//"principal": ["max", "mariano", "mike"]
-	/* No regexes for now. But if we ever implement them, they'd probably look like this */
-	/*"principal": ["foo.smallstep.com", "/^*\.smallstep\.com$/"]*/
-
-	// Principals can be single user names (mariano, max, mike, ...), hostnames/domains (*.smallstep.com, host.smallstep.com, ...) and "emails" (max@smallstep.com, @smallstep.com, ...)
-	// All ValidPrincipals can thus be any one of those, and they can be mixed (mike@smallstep.com, mike, ...); we need to split this?
-	// Should we assume a generic engine, or can we do it host vs. user based? If host vs. user based, then it becomes easier w.r.t. dns; hosts will only be DNS, right?
-	// If we assume generic, we _may_ have a harder time distinguishing host vs. user certs. We propose to use host + user specific provisioners, though...
-	// Perhaps we can do some heuristics on the principal names vs. hostnames (i.e. when only a single label and no dot, then it's a user principal)
-
-	for _, dns := range dnsNames {
-		if _, ok := domainToReverseLabels(dns); !ok {
-			return errors.Errorf("cannot parse dns %q", dns)
-		}
-		if err := checkNameConstraints("dns", dns, dns,
-			func(parsedName, constraint interface{}) (bool, error) {
-				return matchDomainConstraint(parsedName.(string), constraint.(string))
-			}, e.permittedDNSDomains, e.excludedDNSDomains); err != nil {
-			return err
-		}
-	}
-
-	for _, email := range emails {
-		mailbox, ok := parseRFC2821Mailbox(email)
-		if !ok {
-			return fmt.Errorf("cannot parse rfc822Name %q", mailbox)
-		}
-		if err := checkNameConstraints("email", email, mailbox,
-			func(parsedName, constraint interface{}) (bool, error) {
-				return matchEmailConstraint(parsedName.(rfc2821Mailbox), constraint.(string))
-			}, e.permittedEmailAddresses, e.excludedEmailAddresses); err != nil {
-			return err
-		}
-	}
-
-	for _, userName := range userNames {
-		// TODO: some validation? I.e. allowed characters?
-		if err := checkNameConstraints("username", userName, userName,
-			func(parsedName, constraint interface{}) (bool, error) {
-				return matchUserNameConstraint(parsedName.(string), constraint.(string))
-			}, e.permittedPrincipals, e.excludedPrincipals); err != nil {
-			return err
-		}
-	}
-
-	return nil
-}
-
-// splitPrincipals splits SSH certificate principals into DNS names, emails and user names.
-func splitPrincipals(principals []string) (dnsNames, emails, userNames []string) {
-	dnsNames = []string{}
-	emails = []string{}
-	userNames = []string{}
-	for _, principal := range principals {
-		if strings.Contains(principal, "@") {
-			emails = append(emails, principal)
-		} else if len(strings.Split(principal, ".")) > 1 {
-			dnsNames = append(dnsNames, principal)
-		} else {
-			userNames = append(userNames, principal)
-		}
-	}
-	return
-}
-
-// checkNameConstraints checks that c permits a child certificate to claim the
-// given name, of type nameType. The argument parsedName contains the parsed
-// form of name, suitable for passing to the match function. The total number
-// of comparisons is tracked in the given count and should not exceed the given
-// limit.
-// SOURCE: https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go
-func checkNameConstraints(
-	nameType string,
-	name string,
-	parsedName interface{},
-	match func(parsedName, constraint interface{}) (match bool, err error),
-	permitted, excluded interface{}) error {
-
-	excludedValue := reflect.ValueOf(excluded)
-
-	// *count += excludedValue.Len()
-	// if *count > maxConstraintComparisons {
-	// 	return x509.CertificateInvalidError{c, x509.TooManyConstraints, ""}
-	// }
-
-	// TODO: fix the errors; return our own, because we don't have cert ...
-
-	for i := 0; i < excludedValue.Len(); i++ {
-		constraint := excludedValue.Index(i).Interface()
-		match, err := match(parsedName, constraint)
-		if err != nil {
-			return CertificateInvalidError{
-				Reason: x509.CANotAuthorizedForThisName,
-				Detail: err.Error(),
-			}
-		}
-
-		if match {
-			return CertificateInvalidError{
-				Reason: x509.CANotAuthorizedForThisName,
-				Detail: fmt.Sprintf("%s %q is excluded by constraint %q", nameType, name, constraint),
-			}
-		}
-	}
-
-	permittedValue := reflect.ValueOf(permitted)
-
-	// *count += permittedValue.Len()
-	// if *count > maxConstraintComparisons {
-	// 	return x509.CertificateInvalidError{c, x509.TooManyConstraints, ""}
-	// }
-
-	ok := true
-	for i := 0; i < permittedValue.Len(); i++ {
-		constraint := permittedValue.Index(i).Interface()
-		var err error
-		if ok, err = match(parsedName, constraint); err != nil {
-			return CertificateInvalidError{
-				Reason: x509.CANotAuthorizedForThisName,
-				Detail: err.Error(),
-			}
-		}
-
-		if ok {
-			break
-		}
-	}
-
-	if !ok {
-		return CertificateInvalidError{
-			Reason: x509.CANotAuthorizedForThisName,
-			Detail: fmt.Sprintf("%s %q is not permitted by any constraint", nameType, name),
-		}
-	}
-
-	return nil
-}
-
-// SOURCE: https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go
-func matchDomainConstraint(domain, constraint string) (bool, error) {
-	// The meaning of zero length constraints is not specified, but this
-	// code follows NSS and accepts them as matching everything.
-	if constraint == "" {
-		return true, nil
-	}
-
-	domainLabels, ok := domainToReverseLabels(domain)
-	if !ok {
-		return false, fmt.Errorf("cannot parse domain %q", domain)
-	}
-
-	// RFC 5280 says that a leading period in a domain name means that at
-	// least one label must be prepended, but only for URI and email
-	// constraints, not DNS constraints. The code also supports that
-	// behavior for DNS constraints.
-
-	mustHaveSubdomains := false
-	if constraint[0] == '.' {
-		mustHaveSubdomains = true
-		constraint = constraint[1:]
-	}
-
-	constraintLabels, ok := domainToReverseLabels(constraint)
-	if !ok {
-		return false, fmt.Errorf("cannot parse domain %q", constraint)
-	}
-
-	if len(domainLabels) < len(constraintLabels) ||
-		(mustHaveSubdomains && len(domainLabels) == len(constraintLabels)) {
-		return false, nil
-	}
-
-	for i, constraintLabel := range constraintLabels {
-		if !strings.EqualFold(constraintLabel, domainLabels[i]) {
-			return false, nil
-		}
-	}
-
-	return true, nil
-}
-
-// SOURCE: https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go
-func matchEmailConstraint(mailbox rfc2821Mailbox, constraint string) (bool, error) {
-	// If the constraint contains an @, then it specifies an exact mailbox name.
-	if strings.Contains(constraint, "@") {
-		constraintMailbox, ok := parseRFC2821Mailbox(constraint)
-		if !ok {
-			return false, fmt.Errorf("cannot parse constraint %q", constraint)
-		}
-		return mailbox.local == constraintMailbox.local && strings.EqualFold(mailbox.domain, constraintMailbox.domain), nil
-	}
-
-	// Otherwise the constraint is like a DNS constraint of the domain part
-	// of the mailbox.
-	return matchDomainConstraint(mailbox.domain, constraint)
-}
-
-// matchUserNameConstraint performs a string literal match against a constraint
-func matchUserNameConstraint(userName, constraint string) (bool, error) {
-	return userName == constraint, nil
-}
-
-// TODO: decrease code duplication: single policy engine again, with principals added, but not used in x509?
-// Not sure how I'd like to model that in Go, though: use (embedded) structs? interfaces? An x509 name policy engine
-// interface could expose the methods that are useful to x509; the SSH name policy engine interfaces could do the
-// same for SSH ones. One interface for both (with no methods?); then two, so that not all name policy options
-// can be executed on both types? The shared ones could then maybe use the one with no methods? But we need protect
-// it from being applied to just any type, of course. Not sure if Go allows us to do something like that, though.
-// Maybe some kind of dummy function helps there?
-
-// domainToReverseLabels converts a textual domain name like foo.example.com to
-// the list of labels in reverse order, e.g. ["com", "example", "foo"].
-// SOURCE: https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go
-func domainToReverseLabels(domain string) (reverseLabels []string, ok bool) {
-	for len(domain) > 0 {
-		if i := strings.LastIndexByte(domain, '.'); i == -1 {
-			reverseLabels = append(reverseLabels, domain)
-			domain = ""
-		} else {
-			reverseLabels = append(reverseLabels, domain[i+1:])
-			domain = domain[:i]
-		}
-	}
-
-	if len(reverseLabels) > 0 && reverseLabels[0] == "" {
-		// An empty label at the end indicates an absolute value.
-		return nil, false
-	}
-
-	for _, label := range reverseLabels {
-		if label == "" {
-			// Empty labels are otherwise invalid.
-			return nil, false
-		}
-
-		for _, c := range label {
-			if c < 33 || c > 126 {
-				// Invalid character.
-				return nil, false
-			}
-		}
-	}
-
-	return reverseLabels, true
-}
-
-// rfc2821Mailbox represents a “mailbox” (which is an email address to most
-// people) by breaking it into the “local” (i.e. before the '@') and “domain”
-// parts.
-// SOURCE: https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go
-type rfc2821Mailbox struct {
-	local, domain string
-}
-
-// parseRFC2821Mailbox parses an email address into local and domain parts,
-// based on the ABNF for a “Mailbox” from RFC 2821. According to RFC 5280,
-// Section 4.2.1.6 that's correct for an rfc822Name from a certificate: “The
-// format of an rfc822Name is a "Mailbox" as defined in RFC 2821, Section 4.1.2”.
-// SOURCE: https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go
-func parseRFC2821Mailbox(in string) (mailbox rfc2821Mailbox, ok bool) {
-	if in == "" {
-		return mailbox, false
-	}
-
-	localPartBytes := make([]byte, 0, len(in)/2)
-
-	if in[0] == '"' {
-		// Quoted-string = DQUOTE *qcontent DQUOTE
-		// non-whitespace-control = %d1-8 / %d11 / %d12 / %d14-31 / %d127
-		// qcontent = qtext / quoted-pair
-		// qtext = non-whitespace-control /
-		//         %d33 / %d35-91 / %d93-126
-		// quoted-pair = ("\" text) / obs-qp
-		// text = %d1-9 / %d11 / %d12 / %d14-127 / obs-text
-		//
-		// (Names beginning with “obs-” are the obsolete syntax from RFC 2822,
-		// Section 4. Since it has been 16 years, we no longer accept that.)
-		in = in[1:]
-	QuotedString:
-		for {
-			if in == "" {
-				return mailbox, false
-			}
-			c := in[0]
-			in = in[1:]
-
-			switch {
-			case c == '"':
-				break QuotedString
-
-			case c == '\\':
-				// quoted-pair
-				if in == "" {
-					return mailbox, false
-				}
-				if in[0] == 11 ||
-					in[0] == 12 ||
-					(1 <= in[0] && in[0] <= 9) ||
-					(14 <= in[0] && in[0] <= 127) {
-					localPartBytes = append(localPartBytes, in[0])
-					in = in[1:]
-				} else {
-					return mailbox, false
-				}
-
-			case c == 11 ||
-				c == 12 ||
-				// Space (char 32) is not allowed based on the
-				// BNF, but RFC 3696 gives an example that
-				// assumes that it is. Several “verified”
-				// errata continue to argue about this point.
-				// We choose to accept it.
-				c == 32 ||
-				c == 33 ||
-				c == 127 ||
-				(1 <= c && c <= 8) ||
-				(14 <= c && c <= 31) ||
-				(35 <= c && c <= 91) ||
-				(93 <= c && c <= 126):
-				// qtext
-				localPartBytes = append(localPartBytes, c)
-
-			default:
-				return mailbox, false
-			}
-		}
-	} else {
-		// Atom ("." Atom)*
-	NextChar:
-		for len(in) > 0 {
-			// atext from RFC 2822, Section 3.2.4
-			c := in[0]
-
-			switch {
-			case c == '\\':
-				// Examples given in RFC 3696 suggest that
-				// escaped characters can appear outside of a
-				// quoted string. Several “verified” errata
-				// continue to argue the point. We choose to
-				// accept it.
-				in = in[1:]
-				if in == "" {
-					return mailbox, false
-				}
-				fallthrough
-
-			case ('0' <= c && c <= '9') ||
-				('a' <= c && c <= 'z') ||
-				('A' <= c && c <= 'Z') ||
-				c == '!' || c == '#' || c == '$' || c == '%' ||
-				c == '&' || c == '\'' || c == '*' || c == '+' ||
-				c == '-' || c == '/' || c == '=' || c == '?' ||
-				c == '^' || c == '_' || c == '`' || c == '{' ||
-				c == '|' || c == '}' || c == '~' || c == '.':
-				localPartBytes = append(localPartBytes, in[0])
-				in = in[1:]
-
-			default:
-				break NextChar
-			}
-		}
-
-		if len(localPartBytes) == 0 {
-			return mailbox, false
-		}
-
-		// From RFC 3696, Section 3:
-		// “period (".") may also appear, but may not be used to start
-		// or end the local part, nor may two or more consecutive
-		// periods appear.”
-		twoDots := []byte{'.', '.'}
-		if localPartBytes[0] == '.' ||
-			localPartBytes[len(localPartBytes)-1] == '.' ||
-			bytes.Contains(localPartBytes, twoDots) {
-			return mailbox, false
-		}
-	}
-
-	if in == "" || in[0] != '@' {
-		return mailbox, false
-	}
-	in = in[1:]
-
-	// The RFC species a format for domains, but that's known to be
-	// violated in practice so we accept that anything after an '@' is the
-	// domain part.
-	if _, ok := domainToReverseLabels(in); !ok {
-		return mailbox, false
-	}
-
-	mailbox.local = string(localPartBytes)
-	mailbox.domain = in
-	return mailbox, true
-}

policy/ssh/ssh_test.go

@@ -1,261 +0,0 @@
-package sshpolicy
-
-import (
-	"testing"
-
-	"golang.org/x/crypto/ssh"
-)
-
-func TestNamePolicyEngine_ArePrincipalsAllowed(t *testing.T) {
-	type fields struct {
-		options                 []NamePolicyOption
-		permittedDNSDomains     []string
-		excludedDNSDomains      []string
-		permittedEmailAddresses []string
-		excludedEmailAddresses  []string
-		permittedPrincipals     []string
-		excludedPrincipals      []string
-	}
-	tests := []struct {
-		name    string
-		fields  fields
-		cert    *ssh.Certificate
-		want    bool
-		wantErr bool
-	}{
-		{
-			name: "fail/dns-permitted",
-			fields: fields{
-				permittedDNSDomains: []string{".local"},
-			},
-			cert: &ssh.Certificate{
-				ValidPrincipals: []string{"host.notlocal"},
-			},
-			want:    false,
-			wantErr: true,
-		},
-		{
-			name: "fail/dns-permitted",
-			fields: fields{
-				excludedDNSDomains: []string{".local"},
-			},
-			cert: &ssh.Certificate{
-				ValidPrincipals: []string{"host.local"},
-			},
-			want:    false,
-			wantErr: true,
-		},
-		{
-			name: "fail/mail-permitted",
-			fields: fields{
-				permittedEmailAddresses: []string{"example.local"},
-			},
-			cert: &ssh.Certificate{
-				ValidPrincipals: []string{"user@example.notlocal"},
-			},
-			want:    false,
-			wantErr: true,
-		},
-		{
-			name: "fail/mail-excluded",
-			fields: fields{
-				excludedEmailAddresses: []string{"example.local"},
-			},
-			cert: &ssh.Certificate{
-				ValidPrincipals: []string{"user@example.local"},
-			},
-			want:    false,
-			wantErr: true,
-		},
-		{
-			name: "fail/principal-permitted",
-			fields: fields{
-				permittedPrincipals: []string{"user1"},
-			},
-			cert: &ssh.Certificate{
-				ValidPrincipals: []string{"user2"},
-			},
-			want:    false,
-			wantErr: true,
-		},
-		{
-			name: "fail/principal-excluded",
-			fields: fields{
-				excludedPrincipals: []string{"user"},
-			},
-			cert: &ssh.Certificate{
-				ValidPrincipals: []string{"user"},
-			},
-			want:    false,
-			wantErr: true,
-		},
-		{
-			name: "fail/combined-complex-all-badhost.local",
-			fields: fields{
-				permittedDNSDomains:     []string{".local"},
-				permittedEmailAddresses: []string{"example.local"},
-				permittedPrincipals:     []string{"user"},
-				excludedDNSDomains:      []string{"badhost.local"},
-				excludedEmailAddresses:  []string{"badmail@example.local"},
-				excludedPrincipals:      []string{"baduser"},
-			},
-			cert: &ssh.Certificate{
-				ValidPrincipals: []string{
-					"user",
-					"user@example.local",
-					"badhost.local",
-				},
-			},
-			want:    false,
-			wantErr: true,
-		},
-		{
-			name:   "ok/no-constraints",
-			fields: fields{},
-			cert: &ssh.Certificate{
-				ValidPrincipals: []string{"host.example.com"},
-			},
-			want:    true,
-			wantErr: false,
-		},
-		{
-			name: "ok/dns-permitted",
-			fields: fields{
-				permittedDNSDomains: []string{".local"},
-			},
-			cert: &ssh.Certificate{
-				ValidPrincipals: []string{"example.local"},
-			},
-			want:    true,
-			wantErr: false,
-		},
-		{
-			name: "ok/dns-excluded",
-			fields: fields{
-				excludedDNSDomains: []string{".notlocal"},
-			},
-			cert: &ssh.Certificate{
-				ValidPrincipals: []string{"example.local"},
-			},
-			want:    true,
-			wantErr: false,
-		},
-		{
-			name: "ok/mail-permitted",
-			fields: fields{
-				permittedEmailAddresses: []string{"example.local"},
-			},
-			cert: &ssh.Certificate{
-				ValidPrincipals: []string{"user@example.local"},
-			},
-			want:    true,
-			wantErr: false,
-		},
-		{
-			name: "ok/mail-excluded",
-			fields: fields{
-				excludedEmailAddresses: []string{"example.notlocal"},
-			},
-			cert: &ssh.Certificate{
-				ValidPrincipals: []string{"user@example.local"},
-			},
-			want:    true,
-			wantErr: false,
-		},
-		{
-			name: "ok/principal-permitted",
-			fields: fields{
-				permittedPrincipals: []string{"user"},
-			},
-			cert: &ssh.Certificate{
-				ValidPrincipals: []string{"user"},
-			},
-			want:    true,
-			wantErr: false,
-		},
-		{
-			name: "ok/principal-excluded",
-			fields: fields{
-				excludedPrincipals: []string{"someone"},
-			},
-			cert: &ssh.Certificate{
-				ValidPrincipals: []string{"user"},
-			},
-			want:    true,
-			wantErr: false,
-		},
-		{
-			name: "ok/combined-simple-user-permitted",
-			fields: fields{
-				permittedEmailAddresses: []string{"example.local"},
-				permittedPrincipals:     []string{"user"},
-			},
-			cert: &ssh.Certificate{
-				ValidPrincipals: []string{
-					"user",
-					"user@example.local",
-				},
-			},
-			want:    true,
-			wantErr: false,
-		},
-		{
-			name: "ok/combined-simple-all-permitted",
-			fields: fields{
-				permittedDNSDomains:     []string{".local"},
-				permittedEmailAddresses: []string{"example.local"},
-				permittedPrincipals:     []string{"user"},
-			},
-			cert: &ssh.Certificate{
-				ValidPrincipals: []string{
-					"user",
-					"user@example.local",
-					"host.local",
-				},
-			},
-			want:    true,
-			wantErr: false,
-		},
-		{
-			name: "ok/combined-complex-all",
-			fields: fields{
-				permittedDNSDomains:     []string{".local"},
-				permittedEmailAddresses: []string{"example.local"},
-				permittedPrincipals:     []string{"user"},
-				excludedDNSDomains:      []string{"badhost.local"},
-				excludedEmailAddresses:  []string{"badmail@example.local"},
-				excludedPrincipals:      []string{"baduser"},
-			},
-			cert: &ssh.Certificate{
-				ValidPrincipals: []string{
-					"user",
-					"user@example.local",
-					"host.local",
-				},
-			},
-			want:    true,
-			wantErr: false,
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			e := &NamePolicyEngine{
-				options:                 tt.fields.options,
-				permittedDNSDomains:     tt.fields.permittedDNSDomains,
-				excludedDNSDomains:      tt.fields.excludedDNSDomains,
-				permittedEmailAddresses: tt.fields.permittedEmailAddresses,
-				excludedEmailAddresses:  tt.fields.excludedEmailAddresses,
-				permittedPrincipals:     tt.fields.permittedPrincipals,
-				excludedPrincipals:      tt.fields.excludedPrincipals,
-			}
-			got, err := e.ArePrincipalsAllowed(tt.cert)
-			if (err != nil) != tt.wantErr {
-				t.Errorf("NamePolicyEngine.ArePrincipalsAllowed() error = %v, wantErr %v", err, tt.wantErr)
-				return
-			}
-			if got != tt.want {
-				t.Errorf("NamePolicyEngine.ArePrincipalsAllowed() = %v, want %v", got, tt.want)
-			}
-		})
-	}
-}

policy/x509.go

@@ -0,0 +1,14 @@
+package policy
+
+import (
+	"crypto/x509"
+	"net"
+)
+
+type X509NamePolicyEngine interface {
+	AreCertificateNamesAllowed(cert *x509.Certificate) (bool, error)
+	AreCSRNamesAllowed(csr *x509.CertificateRequest) (bool, error)
+	AreSANsAllowed(sans []string) (bool, error)
+	IsDNSAllowed(dns string) (bool, error)
+	IsIPAllowed(ip net.IP) (bool, error)
+}