From 066bf320866de37f509ee647466b5c9a6666c8ff Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Tue, 25 Jan 2022 14:59:55 +0100 Subject: [PATCH] Fix part of PR comments --- authority/provisioner/options.go | 31 +-- authority/provisioner/policy.go | 6 +- authority/provisioner/sign_options.go | 6 +- authority/provisioner/sign_ssh_options.go | 10 +- authority/provisioner/ssh_options.go | 32 +-- policy/engine.go | 54 ++-- policy/engine_test.go | 290 +++++++++++++++++++--- policy/options.go | 2 +- 8 files changed, 322 insertions(+), 109 deletions(-) diff --git a/authority/provisioner/options.go b/authority/provisioner/options.go index 55750d79..257a2107 100644 --- a/authority/provisioner/options.go +++ b/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 -type AllowedX509NameOptions 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 { +// X509NameOptions models the X509 name policy configuration. +type X509NameOptions 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 { - 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 { +func (o *X509NameOptions) HasNames() bool { return len(o.DNSDomains) > 0 || len(o.IPRanges) > 0 || len(o.EmailAddresses) > 0 || diff --git a/authority/provisioner/policy.go b/authority/provisioner/policy.go index 2780d3c4..8a69e1e5 100644 --- a/authority/provisioner/policy.go +++ b/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), ) diff --git a/authority/provisioner/sign_options.go b/authority/provisioner/sign_options.go index a0e27f6d..7ca6cec4 100644 --- a/authority/provisioner/sign_options.go +++ b/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 nil + return err } var ( diff --git a/authority/provisioner/sign_ssh_options.go b/authority/provisioner/sign_ssh_options.go index e52d3aa7..e1853fa1 100644 --- a/authority/provisioner/sign_ssh_options.go +++ b/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 nil + return err } // sshCertTypeUInt32 diff --git a/authority/provisioner/ssh_options.go b/authority/provisioner/ssh_options.go index ada26d7d..91ce7126 100644 --- a/authority/provisioner/ssh_options.go +++ b/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 -type AllowedSSHNameOptions struct { - DNSDomains []string `json:"dns,omitempty"` - EmailAddresses []string `json:"email,omitempty"` - Principals []string `json:"principal,omitempty"` -} - -// DeniedSSHNameOptions models the denied names -type DeniedSSHNameOptions struct { +// SSHNameOptions models the SSH name policy configuration. +type SSHNameOptions 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 -// names configured. -func (o *AllowedSSHNameOptions) HasNames() bool { - return len(o.DNSDomains) > 0 || - len(o.EmailAddresses) > 0 || - len(o.Principals) > 0 -} - -// HasNames checks if the DeniedSSHNameOptions has one or more +// HasNames checks if the SSHNameOptions has one or more // names configured. -func (o *DeniedSSHNameOptions) HasNames() bool { +func (o *SSHNameOptions) HasNames() bool { return len(o.DNSDomains) > 0 || len(o.EmailAddresses) > 0 || len(o.Principals) > 0 diff --git a/policy/engine.go b/policy/engine.go index 4c102fd6..345d6282 100755 --- a/policy/engine.go +++ b/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 { - *ips = append(*ips, ip) - } else if u, err := url.Parse(commonName); err == nil && u.Scheme != "" { - *uris = append(*uris, u) - } else if strings.Contains(commonName, "@") { - *emails = append(*emails, commonName) - } else { - *dnsNames = append(*dnsNames, commonName) - } + subjectDNSNames, subjectIPs, subjectEmails, subjectURIs := x509util.SplitSANs([]string{commonName}) + *dnsNames = append(*dnsNames, subjectDNSNames...) + *ips = append(*ips, subjectIPs...) + *emails = append(*emails, subjectEmails...) + *uris = append(*uris, subjectURIs...) } // 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 { - if strings.Contains(principal, "@") { - emails = append(emails, principal) - } else if ip := net.ParseIP(principal); ip != nil { - ips = append(ips, ip) - } else if len(strings.Split(principal, ".")) > 1 { - dnsNames = append(dnsNames, principal) - } else { - usernames = append(usernames, principal) + var uris []*url.URL + switch cert.CertType { + case ssh.HostCert: + dnsNames, ips, emails, uris = x509util.SplitSANs(cert.ValidPrincipals) + switch { + case len(emails) > 0: + err = fmt.Errorf("Email(-like) principals %v not expected in SSH Host certificate ", emails) + case len(uris) > 0: + 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 } diff --git a/policy/engine_test.go b/policy/engine_test.go index bea231ea..9bc535ea 100755 --- a/policy/engine_test.go +++ b/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", - "user@example.local", - "user", + "127.0.0.31", }, }, 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)) + } + }) + } +} diff --git a/policy/options.go b/policy/options.go index b1fbba70..60bf2f72 100755 --- a/policy/options.go +++ b/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 }