Fix part of PR comments

authority/provisioner/options.go

@@ -58,10 +58,10 @@ type X509Options struct {
 	TemplateData json.RawMessage `json:"templateData,omitempty"`
 
 	// AllowedNames contains the SANs the provisioner is authorized to sign
-	AllowedNames *AllowedX509NameOptions `json:"allow,omitempty"`
+	AllowedNames *X509NameOptions `json:"allow,omitempty"`
 
 	// DeniedNames contains the SANs the provisioner is not authorized to sign
-	DeniedNames *DeniedX509NameOptions `json:"deny,omitempty"`
+	DeniedNames *X509NameOptions `json:"deny,omitempty"`
 }
 
 // HasTemplate returns true if a template is defined in the provisioner options.
@@ -71,7 +71,7 @@ func (o *X509Options) HasTemplate() bool {
 
 // GetAllowedNameOptions returns the AllowedNameOptions, which models the
 // SANs that a provisioner is authorized to sign x509 certificates for.
-func (o *X509Options) GetAllowedNameOptions() *AllowedX509NameOptions {
+func (o *X509Options) GetAllowedNameOptions() *X509NameOptions {
 	if o == nil {
 		return nil
 	}
@@ -80,23 +80,15 @@ func (o *X509Options) GetAllowedNameOptions() *AllowedX509NameOptions {
 
 // GetDeniedNameOptions returns the DeniedNameOptions, which models the
 // SANs that a provisioner is NOT authorized to sign x509 certificates for.
-func (o *X509Options) GetDeniedNameOptions() *DeniedX509NameOptions {
+func (o *X509Options) GetDeniedNameOptions() *X509NameOptions {
 	if o == nil {
 		return nil
 	}
 	return o.DeniedNames
 }
 
-// AllowedX509NameOptions models the allowed names
+// X509NameOptions models the X509 name policy configuration.
-type AllowedX509NameOptions struct {
+type X509NameOptions struct {
-	DNSDomains     []string `json:"dns,omitempty"`
-	IPRanges       []string `json:"ip,omitempty"`
-	EmailAddresses []string `json:"email,omitempty"`
-	URIDomains     []string `json:"uri,omitempty"`
-}
-
-// DeniedX509NameOptions models the denied names
-type DeniedX509NameOptions struct {
 	DNSDomains     []string `json:"dns,omitempty"`
 	IPRanges       []string `json:"ip,omitempty"`
 	EmailAddresses []string `json:"email,omitempty"`
@@ -105,16 +97,7 @@ type DeniedX509NameOptions struct {
 
 // HasNames checks if the AllowedNameOptions has one or more
 // names configured.
-func (o *AllowedX509NameOptions) HasNames() bool {
+func (o *X509NameOptions) HasNames() bool {
-	return len(o.DNSDomains) > 0 ||
-		len(o.IPRanges) > 0 ||
-		len(o.EmailAddresses) > 0 ||
-		len(o.URIDomains) > 0
-}
-
-// HasNames checks if the DeniedNameOptions has one or more
-// names configured.
-func (o *DeniedX509NameOptions) HasNames() bool {
 	return len(o.DNSDomains) > 0 ||
 		len(o.IPRanges) > 0 ||
 		len(o.EmailAddresses) > 0 ||

authority/provisioner/policy.go

@@ -50,7 +50,8 @@ func newSSHPolicyEngine(sshOpts *SSHOptions) (policy.SSHNamePolicyEngine, error)
 	allowed := sshOpts.GetAllowedNameOptions()
 	if allowed != nil && allowed.HasNames() {
 		options = append(options,
-			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.
+			policy.WithPermittedDNSDomains(allowed.DNSDomains),
+			policy.WithPermittedIPsOrCIDRs(allowed.IPRanges),
 			policy.WithPermittedEmailAddresses(allowed.EmailAddresses),
 			policy.WithPermittedPrincipals(allowed.Principals),
 		)
@@ -59,7 +60,8 @@ func newSSHPolicyEngine(sshOpts *SSHOptions) (policy.SSHNamePolicyEngine, error)
 	denied := sshOpts.GetDeniedNameOptions()
 	if denied != nil && denied.HasNames() {
 		options = append(options,
-			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.
+			policy.WithExcludedDNSDomains(denied.DNSDomains),
+			policy.WithExcludedIPsOrCIDRs(denied.IPRanges),
 			policy.WithExcludedEmailAddresses(denied.EmailAddresses),
 			policy.WithExcludedPrincipals(denied.Principals),
 		)

authority/provisioner/sign_options.go

@@ -424,11 +424,9 @@ func (v *x509NamePolicyValidator) Valid(cert *x509.Certificate, _ SignOptions) e
 	if v.policyEngine == nil {
 		return nil
 	}
+
 	_, err := v.policyEngine.AreCertificateNamesAllowed(cert)
-	if err != nil {
+	return err
-		return err
-	}
-	return nil
 }
 
 var (

authority/provisioner/sign_ssh_options.go

@@ -454,6 +454,7 @@ type sshNamePolicyValidator struct {
 // newSSHNamePolicyValidator return a new SSH allow/deny validator.
 func newSSHNamePolicyValidator(engine policy.SSHNamePolicyEngine) *sshNamePolicyValidator {
 	return &sshNamePolicyValidator{
+		// TODO: should we use two engines, one for host certs; another for user certs?
 		policyEngine: engine,
 	}
 }
@@ -464,14 +465,9 @@ func (v *sshNamePolicyValidator) Valid(cert *ssh.Certificate, _ SignSSHOptions)
 	if v.policyEngine == nil {
 		return nil
 	}
-	// TODO(hs): should this perform checks only for hosts vs. user certs depending on context?
+
-	// The current best practice is to have separate provisioners for hosts and users, and thus
-	// separate policy engines for the principals that are allowed.
 	_, err := v.policyEngine.ArePrincipalsAllowed(cert)
-	if err != nil {
+	return err
-		return err
-	}
-	return nil
 }
 
 // sshCertTypeUInt32

authority/provisioner/ssh_options.go

@@ -35,22 +35,16 @@ type SSHOptions struct {
 	TemplateData json.RawMessage `json:"templateData,omitempty"`
 
 	// AllowedNames contains the names the provisioner is authorized to sign
-	AllowedNames *AllowedSSHNameOptions `json:"allow,omitempty"`
+	AllowedNames *SSHNameOptions `json:"allow,omitempty"`
 
 	// DeniedNames contains the names the provisioner is not authorized to sign
-	DeniedNames *DeniedSSHNameOptions `json:"deny,omitempty"`
+	DeniedNames *SSHNameOptions `json:"deny,omitempty"`
 }
 
-// AllowedSSHNameOptions models the allowed names
+// SSHNameOptions models the SSH name policy configuration.
-type AllowedSSHNameOptions struct {
+type SSHNameOptions struct {
-	DNSDomains     []string `json:"dns,omitempty"`
-	EmailAddresses []string `json:"email,omitempty"`
-	Principals     []string `json:"principal,omitempty"`
-}
-
-// DeniedSSHNameOptions models the denied names
-type DeniedSSHNameOptions struct {
 	DNSDomains     []string `json:"dns,omitempty"`
+	IPRanges       []string `json:"ip,omitempty"`
 	EmailAddresses []string `json:"email,omitempty"`
 	Principals     []string `json:"principal,omitempty"`
 }
@@ -62,7 +56,7 @@ func (o *SSHOptions) HasTemplate() bool {
 
 // GetAllowedNameOptions returns the AllowedSSHNameOptions, which models the
 // names that a provisioner is authorized to sign SSH certificates for.
-func (o *SSHOptions) GetAllowedNameOptions() *AllowedSSHNameOptions {
+func (o *SSHOptions) GetAllowedNameOptions() *SSHNameOptions {
 	if o == nil {
 		return nil
 	}
@@ -71,24 +65,16 @@ func (o *SSHOptions) GetAllowedNameOptions() *AllowedSSHNameOptions {
 
 // GetDeniedNameOptions returns the DeniedSSHNameOptions, which models the
 // names that a provisioner is NOT authorized to sign SSH certificates for.
-func (o *SSHOptions) GetDeniedNameOptions() *DeniedSSHNameOptions {
+func (o *SSHOptions) GetDeniedNameOptions() *SSHNameOptions {
 	if o == nil {
 		return nil
 	}
 	return o.DeniedNames
 }
 
-// HasNames checks if the AllowedSSHNameOptions has one or more
+// HasNames checks if the SSHNameOptions has one or more
 // names configured.
-func (o *AllowedSSHNameOptions) HasNames() bool {
+func (o *SSHNameOptions) HasNames() bool {
-	return len(o.DNSDomains) > 0 ||
-		len(o.EmailAddresses) > 0 ||
-		len(o.Principals) > 0
-}
-
-// HasNames checks if the DeniedSSHNameOptions has one or more
-// names configured.
-func (o *DeniedSSHNameOptions) HasNames() bool {
 	return len(o.DNSDomains) > 0 ||
 		len(o.EmailAddresses) > 0 ||
 		len(o.Principals) > 0

policy/engine.go

@@ -197,7 +197,10 @@ func (e *NamePolicyEngine) IsIPAllowed(ip net.IP) (bool, error) {
 
 // ArePrincipalsAllowed verifies that all principals in an SSH certificate are allowed.
 func (e *NamePolicyEngine) ArePrincipalsAllowed(cert *ssh.Certificate) (bool, error) {
-	dnsNames, ips, emails, usernames := splitPrincipals(cert.ValidPrincipals)
+	dnsNames, ips, emails, usernames, err := splitSSHPrincipals(cert)
+	if err != nil {
+		return false, err
+	}
 	if err := e.validateNames(dnsNames, ips, emails, []*url.URL{}, usernames); err != nil {
 		return false, err
 	}
@@ -213,34 +216,45 @@ func appendSubjectCommonName(subject pkix.Name, dnsNames *[]string, ips *[]net.I
 	if commonName == "" {
 		return
 	}
-	if ip := net.ParseIP(commonName); ip != nil {
+	subjectDNSNames, subjectIPs, subjectEmails, subjectURIs := x509util.SplitSANs([]string{commonName})
-		*ips = append(*ips, ip)
+	*dnsNames = append(*dnsNames, subjectDNSNames...)
-	} else if u, err := url.Parse(commonName); err == nil && u.Scheme != "" {
+	*ips = append(*ips, subjectIPs...)
-		*uris = append(*uris, u)
+	*emails = append(*emails, subjectEmails...)
-	} else if strings.Contains(commonName, "@") {
+	*uris = append(*uris, subjectURIs...)
-		*emails = append(*emails, commonName)
-	} else {
-		*dnsNames = append(*dnsNames, commonName)
-	}
 }
 
 // splitPrincipals splits SSH certificate principals into DNS names, emails and usernames.
-func splitPrincipals(principals []string) (dnsNames []string, ips []net.IP, emails, usernames []string) {
+func splitSSHPrincipals(cert *ssh.Certificate) (dnsNames []string, ips []net.IP, emails, usernames []string, err error) {
 	dnsNames = []string{}
 	ips = []net.IP{}
 	emails = []string{}
 	usernames = []string{}
-	for _, principal := range principals {
+	var uris []*url.URL
-		if strings.Contains(principal, "@") {
+	switch cert.CertType {
-			emails = append(emails, principal)
+	case ssh.HostCert:
-		} else if ip := net.ParseIP(principal); ip != nil {
+		dnsNames, ips, emails, uris = x509util.SplitSANs(cert.ValidPrincipals)
-			ips = append(ips, ip)
+		switch {
-		} else if len(strings.Split(principal, ".")) > 1 {
+		case len(emails) > 0:
-			dnsNames = append(dnsNames, principal)
+			err = fmt.Errorf("Email(-like) principals %v not expected in SSH Host certificate ", emails)
-		} else {
+		case len(uris) > 0:
-			usernames = append(usernames, principal)
+			err = fmt.Errorf("URL principals %v not expected in SSH Host certificate ", uris)
 		}
+	case ssh.UserCert:
+		// re-using SplitSANs results in anything that can't be parsed as an IP, URI or email
+		// to be considered a username. This allows usernames like h.slatman to be present
+		// in the SSH certificate. We're exluding IPs and URIs, because they can be confusing
+		// when used in a SSH user certificate.
+		usernames, ips, emails, uris = x509util.SplitSANs(cert.ValidPrincipals)
+		switch {
+		case len(ips) > 0:
+			err = fmt.Errorf("IP principals %v not expected in SSH User certificate ", ips)
+		case len(uris) > 0:
+			err = fmt.Errorf("URL principals %v not expected in SSH User certificate ", uris)
+		}
+	default:
+		err = fmt.Errorf("unexpected SSH certificate type %d", cert.CertType)
 	}
+
 	return
 }
 

policy/engine_test.go

@@ -7,6 +7,7 @@ import (
 	"net/url"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/smallstep/assert"
 	"golang.org/x/crypto/ssh"
 )
@@ -2177,11 +2178,12 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 		wantErr bool
 	}{
 		{
-			name: "fail/with-permitted-dns-domain",
+			name: "fail/host-with-permitted-dns-domain",
 			options: []NamePolicyOption{
 				WithPermittedDNSDomain("*.local"),
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.HostCert,
 				ValidPrincipals: []string{
 					"host.example.com",
 				},
@@ -2190,11 +2192,12 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "fail/with-excluded-dns-domain",
+			name: "fail/host-with-excluded-dns-domain",
 			options: []NamePolicyOption{
 				WithExcludedDNSDomain("*.local"),
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.HostCert,
 				ValidPrincipals: []string{
 					"host.local",
 				},
@@ -2203,11 +2206,12 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "fail/with-permitted-ip",
+			name: "fail/host-with-permitted-ip",
 			options: []NamePolicyOption{
 				WithPermittedCIDR("127.0.0.1/24"),
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.HostCert,
 				ValidPrincipals: []string{
 					"192.168.0.22",
 				},
@@ -2216,11 +2220,12 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "fail/with-excluded-ip",
+			name: "fail/host-with-excluded-ip",
 			options: []NamePolicyOption{
 				WithExcludedCIDR("127.0.0.1/24"),
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.HostCert,
 				ValidPrincipals: []string{
 					"127.0.0.0",
 				},
@@ -2229,11 +2234,12 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "fail/with-permitted-email",
+			name: "fail/user-with-permitted-email",
 			options: []NamePolicyOption{
 				WithPermittedEmailAddress("@example.com"),
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.UserCert,
 				ValidPrincipals: []string{
 					"mail@local",
 				},
@@ -2242,11 +2248,12 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "fail/with-excluded-email",
+			name: "fail/user-with-excluded-email",
 			options: []NamePolicyOption{
 				WithExcludedEmailAddress("@example.com"),
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.UserCert,
 				ValidPrincipals: []string{
 					"mail@example.com",
 				},
@@ -2255,11 +2262,39 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "fail/with-permitted-principals",
+			name: "fail/host-with-permitted-principals",
+			options: []NamePolicyOption{
+				WithPermittedPrincipals([]string{"localhost"}),
+			},
+			cert: &ssh.Certificate{
+				CertType: ssh.HostCert,
+				ValidPrincipals: []string{
+					"host",
+				},
+			},
+			want:    false,
+			wantErr: true,
+		},
+		{
+			name: "fail/host-with-excluded-principals",
+			options: []NamePolicyOption{
+				WithExcludedPrincipals([]string{"localhost"}),
+			},
+			cert: &ssh.Certificate{
+				ValidPrincipals: []string{
+					"localhost",
+				},
+			},
+			want:    false,
+			wantErr: true,
+		},
+		{
+			name: "fail/user-with-permitted-principals",
 			options: []NamePolicyOption{
 				WithPermittedPrincipals([]string{"user"}),
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.UserCert,
 				ValidPrincipals: []string{
 					"root",
 				},
@@ -2268,11 +2303,12 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "fail/with-excluded-principals",
+			name: "fail/user-with-excluded-principals",
 			options: []NamePolicyOption{
 				WithExcludedPrincipals([]string{"user"}),
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.UserCert,
 				ValidPrincipals: []string{
 					"user",
 				},
@@ -2281,11 +2317,12 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "fail/with-permitted-principal-as-mail",
+			name: "fail/user-with-permitted-principal-as-mail",
 			options: []NamePolicyOption{
 				WithPermittedPrincipals([]string{"ops"}),
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.UserCert,
 				ValidPrincipals: []string{
 					"ops@work", // this is (currently) parsed as an email-like principal; not allowed with just "ops" as the permitted principal
 				},
@@ -2294,11 +2331,12 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "fail/principal-with-permitted-dns-domain", // when only DNS is permitted, username principals are not allowed.
+			name: "fail/host-principal-with-permitted-dns-domain", // when only DNS is permitted, username principals are not allowed.
 			options: []NamePolicyOption{
 				WithPermittedDNSDomain("*.local"),
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.HostCert,
 				ValidPrincipals: []string{
 					"user",
 				},
@@ -2307,11 +2345,12 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "fail/principal-with-permitted-ip-range", // when only IPs are permitted, username principals are not allowed.
+			name: "fail/host-principal-with-permitted-ip-range", // when only IPs are permitted, username principals are not allowed.
 			options: []NamePolicyOption{
 				WithPermittedCIDR("127.0.0.1/24"),
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.HostCert,
 				ValidPrincipals: []string{
 					"user",
 				},
@@ -2320,11 +2359,12 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "fail/principal-with-permitted-email", // when only emails are permitted, username principals are not allowed.
+			name: "fail/user-principal-with-permitted-email", // when only emails are permitted, username principals are not allowed.
 			options: []NamePolicyOption{
 				WithPermittedEmailAddress("@example.com"),
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.UserCert,
 				ValidPrincipals: []string{
 					"user",
 				},
@@ -2339,6 +2379,7 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 				WithExcludedEmailAddress("root@smallstep.com"),
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.UserCert,
 				ValidPrincipals: []string{
 					"someone@smallstep.com",
 					"someone",
@@ -2354,6 +2395,7 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 				WithExcludedPrincipals([]string{"root"}),
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.UserCert,
 				ValidPrincipals: []string{
 					"someone@smallstep.com",
 					"root",
@@ -2363,11 +2405,40 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "ok/with-permitted-dns-domain",
+			name: "ok/host-with-permitted-user-principals",
+			options: []NamePolicyOption{
+				WithPermittedEmailAddress("@work"),
+			},
+			cert: &ssh.Certificate{
+				CertType: ssh.HostCert,
+				ValidPrincipals: []string{
+					"example.work",
+				},
+			},
+			want:    false,
+			wantErr: true,
+		},
+		{
+			name: "ok/user-with-permitted-user-principals",
 			options: []NamePolicyOption{
 				WithPermittedDNSDomain("*.local"),
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.UserCert,
+				ValidPrincipals: []string{
+					"herman@work",
+				},
+			},
+			want:    false,
+			wantErr: true,
+		},
+		{
+			name: "ok/host-with-permitted-dns-domain",
+			options: []NamePolicyOption{
+				WithPermittedDNSDomain("*.local"),
+			},
+			cert: &ssh.Certificate{
+				CertType: ssh.HostCert,
 				ValidPrincipals: []string{
 					"host.local",
 				},
@@ -2376,11 +2447,12 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "ok/with-excluded-dns-domain",
+			name: "ok/host-with-excluded-dns-domain",
 			options: []NamePolicyOption{
 				WithExcludedDNSDomain("*.example.com"),
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.HostCert,
 				ValidPrincipals: []string{
 					"host.local",
 				},
@@ -2389,11 +2461,12 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "ok/with-permitted-ip",
+			name: "ok/host-with-permitted-ip",
 			options: []NamePolicyOption{
 				WithPermittedCIDR("127.0.0.1/24"),
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.HostCert,
 				ValidPrincipals: []string{
 					"127.0.0.33",
 				},
@@ -2402,11 +2475,12 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "ok/with-excluded-ip",
+			name: "ok/host-with-excluded-ip",
 			options: []NamePolicyOption{
 				WithExcludedCIDR("127.0.0.1/24"),
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.HostCert,
 				ValidPrincipals: []string{
 					"192.168.0.35",
 				},
@@ -2415,11 +2489,12 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "ok/with-permitted-email",
+			name: "ok/user-with-permitted-email",
 			options: []NamePolicyOption{
 				WithPermittedEmailAddress("@example.com"),
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.UserCert,
 				ValidPrincipals: []string{
 					"mail@example.com",
 				},
@@ -2428,11 +2503,12 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "ok/with-excluded-email",
+			name: "ok/user-with-excluded-email",
 			options: []NamePolicyOption{
 				WithExcludedEmailAddress("@example.com"),
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.UserCert,
 				ValidPrincipals: []string{
 					"mail@local",
 				},
@@ -2441,11 +2517,12 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "ok/with-permitted-principals",
+			name: "ok/user-with-permitted-principals",
 			options: []NamePolicyOption{
 				WithPermittedPrincipals([]string{"*"}),
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.UserCert,
 				ValidPrincipals: []string{
 					"user",
 				},
@@ -2454,11 +2531,12 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "ok/with-excluded-principals",
+			name: "ok/user-with-excluded-principals",
 			options: []NamePolicyOption{
 				WithExcludedPrincipals([]string{"user"}),
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.UserCert,
 				ValidPrincipals: []string{
 					"root",
 				},
@@ -2474,6 +2552,7 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 				WithExcludedEmailAddress("root@smallstep.com"),
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.UserCert,
 				ValidPrincipals: []string{
 					"someone@smallstep.com",
 					"someone",
@@ -2490,6 +2569,7 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 				WithExcludedPrincipals([]string{"root"}), // unlike the previous test, this implicitly allows any other username principal
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.UserCert,
 				ValidPrincipals: []string{
 					"someone@smallstep.com",
 					"someone",
@@ -2499,23 +2579,18 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "ok/combined-simple-all",
+			name: "ok/combined-host",
 			options: []NamePolicyOption{
 				WithPermittedDNSDomain("*.local"),
 				WithPermittedCIDR("127.0.0.1/24"),
-				WithPermittedEmailAddress("@example.local"),
-				WithPermittedPrincipals([]string{"user"}),
 				WithExcludedDNSDomain("badhost.local"),
 				WithExcludedCIDR("127.0.0.128/25"),
-				WithExcludedEmailAddress("badmail@example.local"),
-				WithExcludedPrincipals([]string{"root"}),
 			},
 			cert: &ssh.Certificate{
+				CertType: ssh.HostCert,
 				ValidPrincipals: []string{
 					"example.local",
-					"127.0.0.1",
+					"127.0.0.31",
-					"user@example.local",
-					"user",
 				},
 			},
 			want:    true,
@@ -2537,3 +2612,162 @@ func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
 		})
 	}
 }
+
+type result struct {
+	wantDNSNames  []string
+	wantIps       []net.IP
+	wantEmails    []string
+	wantUsernames []string
+}
+
+func emptyResult() result {
+	return result{
+		wantDNSNames:  []string{},
+		wantIps:       []net.IP{},
+		wantEmails:    []string{},
+		wantUsernames: []string{},
+	}
+}
+
+func Test_splitSSHPrincipals(t *testing.T) {
+	type test struct {
+		cert    *ssh.Certificate
+		r       result
+		wantErr bool
+	}
+	var tests = map[string]func(t *testing.T) test{
+		"fail/unexpected-cert-type": func(t *testing.T) test {
+			r := emptyResult()
+			return test{
+				cert: &ssh.Certificate{
+					CertType: uint32(0),
+				},
+				r:       r,
+				wantErr: true,
+			}
+		},
+		"fail/user-ip": func(t *testing.T) test {
+			r := emptyResult()
+			r.wantIps = []net.IP{net.ParseIP("127.0.0.1")} // this will still be in the result
+			return test{
+				cert: &ssh.Certificate{
+					CertType:        ssh.UserCert,
+					ValidPrincipals: []string{"127.0.0.1"},
+				},
+				r:       r,
+				wantErr: true,
+			}
+		},
+		"fail/user-uri": func(t *testing.T) test {
+			r := emptyResult()
+			return test{
+				cert: &ssh.Certificate{
+					CertType:        ssh.UserCert,
+					ValidPrincipals: []string{"https://host.local/"},
+				},
+				r:       r,
+				wantErr: true,
+			}
+		},
+		"fail/host-email": func(t *testing.T) test {
+			r := emptyResult()
+			r.wantEmails = []string{"ops@work"} // this will still be in the result
+			return test{
+				cert: &ssh.Certificate{
+					CertType:        ssh.HostCert,
+					ValidPrincipals: []string{"ops@work"},
+				},
+				r:       r,
+				wantErr: true,
+			}
+		},
+		"fail/host-uri": func(t *testing.T) test {
+			r := emptyResult()
+			return test{
+				cert: &ssh.Certificate{
+					CertType:        ssh.HostCert,
+					ValidPrincipals: []string{"https://host.local/"},
+				},
+				r:       r,
+				wantErr: true,
+			}
+		},
+		"ok/host-dns": func(t *testing.T) test {
+			r := emptyResult()
+			r.wantDNSNames = []string{"host.example.com"}
+			return test{
+				cert: &ssh.Certificate{
+					CertType:        ssh.HostCert,
+					ValidPrincipals: []string{"host.example.com"},
+				},
+				r: r,
+			}
+		},
+		"ok/host-ip": func(t *testing.T) test {
+			r := emptyResult()
+			r.wantIps = []net.IP{net.ParseIP("127.0.0.1")}
+			return test{
+				cert: &ssh.Certificate{
+					CertType:        ssh.HostCert,
+					ValidPrincipals: []string{"127.0.0.1"},
+				},
+				r: r,
+			}
+		},
+		"ok/user-localhost": func(t *testing.T) test {
+			r := emptyResult()
+			r.wantUsernames = []string{"localhost"} // when type is User cert, this is considered a username; not a DNS
+			return test{
+				cert: &ssh.Certificate{
+					CertType:        ssh.UserCert,
+					ValidPrincipals: []string{"localhost"},
+				},
+				r: r,
+			}
+		},
+		"ok/user-username-with-period": func(t *testing.T) test {
+			r := emptyResult()
+			r.wantUsernames = []string{"x.joe"}
+			return test{
+				cert: &ssh.Certificate{
+					CertType:        ssh.UserCert,
+					ValidPrincipals: []string{"x.joe"},
+				},
+				r: r,
+			}
+		},
+		"ok/user-maillike": func(t *testing.T) test {
+			r := emptyResult()
+			r.wantEmails = []string{"ops@work"}
+			return test{
+				cert: &ssh.Certificate{
+					CertType:        ssh.UserCert,
+					ValidPrincipals: []string{"ops@work"},
+				},
+				r: r,
+			}
+		},
+	}
+	for name, prep := range tests {
+		tt := prep(t)
+		t.Run(name, func(t *testing.T) {
+			gotDNSNames, gotIps, gotEmails, gotUsernames, err := splitSSHPrincipals(tt.cert)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("splitSSHPrincipals() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !cmp.Equal(tt.r.wantDNSNames, gotDNSNames) {
+				t.Errorf("splitSSHPrincipals() DNS names diff =\n%s", cmp.Diff(tt.r.wantDNSNames, gotDNSNames))
+			}
+			if !cmp.Equal(tt.r.wantIps, gotIps) {
+				t.Errorf("splitSSHPrincipals() IPs diff =\n%s", cmp.Diff(tt.r.wantIps, gotIps))
+			}
+			if !cmp.Equal(tt.r.wantEmails, gotEmails) {
+				t.Errorf("splitSSHPrincipals() Emails diff =\n%s", cmp.Diff(tt.r.wantEmails, gotEmails))
+			}
+			if !cmp.Equal(tt.r.wantUsernames, gotUsernames) {
+				t.Errorf("splitSSHPrincipals() Usernames diff =\n%s", cmp.Diff(tt.r.wantUsernames, gotUsernames))
+			}
+		})
+	}
+}

policy/options.go

@@ -602,7 +602,7 @@ func normalizeAndValidateDNSDomainConstraint(constraint string) (string, error)
 		return "", errors.Errorf("domain constraint %q can only have wildcard as starting character", constraint)
 	}
 	if _, ok := domainToReverseLabels(normalizedConstraint); !ok {
-		return "", errors.Errorf("cannot parse permitted domain constraint %q", constraint)
+		return "", errors.Errorf("cannot parse domain constraint %q", constraint)
 	}
 	return normalizedConstraint, nil
 }