From 6440870a8065e69679d9a40838ce9e369964816b Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Tue, 18 Jan 2022 14:39:21 +0100 Subject: [PATCH] Clean up, improve test cases and coverage --- acme/api/order.go | 0 authority/provisioner/acme.go | 0 authority/provisioner/jwk.go | 0 authority/provisioner/options.go | 4 +- authority/provisioner/policy.go | 4 +- authority/provisioner/sign_options.go | 0 policy/engine.go | 138 +++----- policy/engine_test.go | 484 +++++++++++++++++++++++++- policy/options.go | 90 +++-- policy/options_test.go | 126 +++++++ 10 files changed, 723 insertions(+), 123 deletions(-) mode change 100755 => 100644 acme/api/order.go mode change 100755 => 100644 authority/provisioner/acme.go mode change 100755 => 100644 authority/provisioner/jwk.go mode change 100755 => 100644 authority/provisioner/options.go mode change 100755 => 100644 authority/provisioner/sign_options.go diff --git a/acme/api/order.go b/acme/api/order.go old mode 100755 new mode 100644 diff --git a/authority/provisioner/acme.go b/authority/provisioner/acme.go old mode 100755 new mode 100644 diff --git a/authority/provisioner/jwk.go b/authority/provisioner/jwk.go old mode 100755 new mode 100644 diff --git a/authority/provisioner/options.go b/authority/provisioner/options.go old mode 100755 new mode 100644 index 7c516f6d..55750d79 --- a/authority/provisioner/options.go +++ b/authority/provisioner/options.go @@ -90,7 +90,7 @@ func (o *X509Options) GetDeniedNameOptions() *DeniedX509NameOptions { // AllowedX509NameOptions models the allowed names type AllowedX509NameOptions struct { DNSDomains []string `json:"dns,omitempty"` - IPRanges []string `json:"ip,omitempty"` // TODO(hs): support IPs as well as ranges + IPRanges []string `json:"ip,omitempty"` EmailAddresses []string `json:"email,omitempty"` URIDomains []string `json:"uri,omitempty"` } @@ -98,7 +98,7 @@ type AllowedX509NameOptions struct { // DeniedX509NameOptions models the denied names type DeniedX509NameOptions struct { DNSDomains []string `json:"dns,omitempty"` - IPRanges []string `json:"ip,omitempty"` // TODO(hs): support IPs as well as ranges + IPRanges []string `json:"ip,omitempty"` EmailAddresses []string `json:"email,omitempty"` URIDomains []string `json:"uri,omitempty"` } diff --git a/authority/provisioner/policy.go b/authority/provisioner/policy.go index 5fe31935..2780d3c4 100644 --- a/authority/provisioner/policy.go +++ b/authority/provisioner/policy.go @@ -19,7 +19,7 @@ func newX509PolicyEngine(x509Opts *X509Options) (policy.X509NamePolicyEngine, er if allowed != nil && allowed.HasNames() { options = append(options, policy.WithPermittedDNSDomains(allowed.DNSDomains), - policy.WithPermittedCIDRs(allowed.IPRanges), // TODO(hs): support IPs in addition to ranges + policy.WithPermittedIPsOrCIDRs(allowed.IPRanges), policy.WithPermittedEmailAddresses(allowed.EmailAddresses), policy.WithPermittedURIDomains(allowed.URIDomains), ) @@ -29,7 +29,7 @@ func newX509PolicyEngine(x509Opts *X509Options) (policy.X509NamePolicyEngine, er if denied != nil && denied.HasNames() { options = append(options, policy.WithExcludedDNSDomains(denied.DNSDomains), - policy.WithExcludedCIDRs(denied.IPRanges), // TODO(hs): support IPs in addition to ranges + policy.WithExcludedIPsOrCIDRs(denied.IPRanges), policy.WithExcludedEmailAddresses(denied.EmailAddresses), policy.WithExcludedURIDomains(denied.URIDomains), ) diff --git a/authority/provisioner/sign_options.go b/authority/provisioner/sign_options.go old mode 100755 new mode 100644 diff --git a/policy/engine.go b/policy/engine.go index 10da6bc9..f850ecf3 100755 --- a/policy/engine.go +++ b/policy/engine.go @@ -15,33 +15,25 @@ import ( "golang.org/x/crypto/ssh" ) -type CertificateInvalidError struct { - Reason x509.InvalidReason +type NamePolicyReason int + +const ( + // NotAuthorizedForThisName results when an instance of + // NamePolicyEngine determines that there's a constraint which + // doesn't permit a DNS or another type of SAN to be signed + // (or otherwise used). + NotAuthorizedForThisName NamePolicyReason = iota +) + +type NamePolicyError struct { + Reason NamePolicyReason Detail string } -func (e CertificateInvalidError) Error() string { +func (e NamePolicyError) 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: + case NotAuthorizedForThisName: 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" } @@ -126,7 +118,7 @@ func removeDuplicates(strSlice []string) []string { keys := make(map[string]bool) result := []string{} for _, item := range strSlice { - if _, value := keys[item]; !value { + if _, value := keys[item]; !value && item != "" { // skip empty constraints keys[item] = true result = append(result, item) } @@ -206,8 +198,8 @@ 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, emails, usernames := splitPrincipals(cert.ValidPrincipals) - if err := e.validateNames(dnsNames, []net.IP{}, emails, []*url.URL{}, usernames); err != nil { + dnsNames, ips, emails, usernames := splitPrincipals(cert.ValidPrincipals) + if err := e.validateNames(dnsNames, ips, emails, []*url.URL{}, usernames); err != nil { return false, err } return true, nil @@ -233,14 +225,17 @@ 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) { +// splitPrincipals splits SSH certificate principals into DNS names, emails and usernames. +func splitPrincipals(principals []string) (dnsNames []string, ips []net.IP, emails, usernames []string) { 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 { @@ -260,7 +255,6 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA return nil } - // TODO: return our own type(s) of error? // TODO: implement check that requires at least a single name in all of the SANs + subject? // TODO: set limit on total of all names validated? In x509 there's a limit on the number of comparisons @@ -277,9 +271,9 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA // then return error, because DNS should be explicitly configured to be allowed in that case. In case there are // (other) excluded constraints, we'll allow a DNS (implicit allow; currently). if e.numberOfDNSDomainConstraints == 0 && e.totalNumberOfPermittedConstraints > 0 { - return CertificateInvalidError{ - Reason: x509.CANotAuthorizedForThisName, - Detail: fmt.Sprintf("dns %q is not permitted by any constraint", dns), // TODO(hs): change this error (message) + return NamePolicyError{ + Reason: NotAuthorizedForThisName, + Detail: fmt.Sprintf("dns %q is not explicitly permitted by any constraint", dns), } } if _, ok := domainToReverseLabels(dns); !ok { @@ -295,9 +289,9 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA for _, ip := range ips { if e.numberOfIPRangeConstraints == 0 && e.totalNumberOfPermittedConstraints > 0 { - return CertificateInvalidError{ - Reason: x509.CANotAuthorizedForThisName, - Detail: fmt.Sprintf("ip %q is not permitted by any constraint", ip.String()), + return NamePolicyError{ + Reason: NotAuthorizedForThisName, + Detail: fmt.Sprintf("ip %q is not explicitly permitted by any constraint", ip.String()), } } if err := checkNameConstraints("ip", ip.String(), ip, @@ -310,9 +304,9 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA for _, email := range emailAddresses { if e.numberOfEmailAddressConstraints == 0 && e.totalNumberOfPermittedConstraints > 0 { - return CertificateInvalidError{ - Reason: x509.CANotAuthorizedForThisName, - Detail: fmt.Sprintf("email %q is not permitted by any constraint", email), + return NamePolicyError{ + Reason: NotAuthorizedForThisName, + Detail: fmt.Sprintf("email %q is not explicitly permitted by any constraint", email), } } mailbox, ok := parseRFC2821Mailbox(email) @@ -329,9 +323,9 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA for _, uri := range uris { if e.numberOfURIDomainConstraints == 0 && e.totalNumberOfPermittedConstraints > 0 { - return CertificateInvalidError{ - Reason: x509.CANotAuthorizedForThisName, - Detail: fmt.Sprintf("uri %q is not permitted by any constraint", uri.String()), + return NamePolicyError{ + Reason: NotAuthorizedForThisName, + Detail: fmt.Sprintf("uri %q is not explicitly permitted by any constraint", uri.String()), } } if err := checkNameConstraints("uri", uri.String(), uri, @@ -342,23 +336,11 @@ 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), + return NamePolicyError{ + Reason: NotAuthorizedForThisName, + Detail: fmt.Sprintf("username principal %q is not explicity permitted by any constraint", username), } } // TODO: some validation? I.e. allowed characters? @@ -370,7 +352,7 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA } } - // TODO: when the error is not nil and returned up in the above, we can add + // TODO(hs): 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). // TODO(hs): validate other types of SANs? The Go std library skips those. @@ -382,8 +364,7 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA // checkNameConstraints checks that a name, of type nameType is permitted. // 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. +// to the match function. // SOURCE: https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go func checkNameConstraints( nameType string, @@ -394,26 +375,19 @@ func checkNameConstraints( 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, + return NamePolicyError{ + Reason: NotAuthorizedForThisName, Detail: err.Error(), } } if match { - return CertificateInvalidError{ - Reason: x509.CANotAuthorizedForThisName, + return NamePolicyError{ + Reason: NotAuthorizedForThisName, Detail: fmt.Sprintf("%s %q is excluded by constraint %q", nameType, name, constraint), } } @@ -421,18 +395,13 @@ func checkNameConstraints( 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, + return NamePolicyError{ + Reason: NotAuthorizedForThisName, Detail: err.Error(), } } @@ -443,8 +412,8 @@ func checkNameConstraints( } if !ok { - return CertificateInvalidError{ - Reason: x509.CANotAuthorizedForThisName, + return NamePolicyError{ + Reason: NotAuthorizedForThisName, Detail: fmt.Sprintf("%s %q is not permitted by any constraint", nameType, name), } } @@ -651,7 +620,6 @@ func (e *NamePolicyEngine) matchDomainConstraint(domain, constraint string) (boo } // Block domains that start with just a period - // TODO(hs): check if we should allow domains starting with "." at all; not sure if this is allowed in x509 names and certs. if domain[0] == '.' { return false, nil } @@ -744,19 +712,11 @@ func matchIPConstraint(ip net.IP, constraint *net.IPNet) (bool, error) { // } // } - // if isIPv4(ip) != isIPv4(constraint.IP) { // TODO(hs): this check seems to do what the above intended to do? - // return false, nil - // } - contained := constraint.Contains(ip) // TODO(hs): validate that this is the correct behavior; also check IPv4-in-IPv6 (again) return contained, nil } -func isIPv4(ip net.IP) bool { - return ip.To4() != nil -} - // SOURCE: https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go func (e *NamePolicyEngine) matchEmailConstraint(mailbox rfc2821Mailbox, constraint string) (bool, error) { // TODO(hs): handle literal wildcard case for emails? Does that even make sense? @@ -817,5 +777,9 @@ func (e *NamePolicyEngine) matchURIConstraint(uri *url.URL, constraint string) ( // matchUsernameConstraint performs a string literal match against a constraint. func matchUsernameConstraint(username, constraint string) (bool, error) { + // allow any plain principal username + if constraint == "*" { + return true, nil + } return strings.EqualFold(username, constraint), nil } diff --git a/policy/engine_test.go b/policy/engine_test.go index 2b3484ab..bea231ea 100755 --- a/policy/engine_test.go +++ b/policy/engine_test.go @@ -8,11 +8,11 @@ import ( "testing" "github.com/smallstep/assert" + "golang.org/x/crypto/ssh" ) // TODO(hs): the functionality in the policy engine is a nice candidate for trying fuzzing on // TODO(hs): more complex uses cases that combine multiple names and permitted/excluded entries -// TODO(hs): check errors (reasons) are as expected func TestNamePolicyEngine_matchDomainConstraint(t *testing.T) { tests := []struct { @@ -135,6 +135,22 @@ func TestNamePolicyEngine_matchDomainConstraint(t *testing.T) { want: false, wantErr: false, }, + { + name: "false/idna-internationalized-domain-name", + engine: &NamePolicyEngine{}, + domain: "JP納豆.例.jp", // Example value from https://www.w3.org/International/articles/idn-and-iri/ + constraint: ".例.jp", + want: false, + wantErr: true, + }, + { + name: "false/idna-internationalized-domain-name-constraint", + engine: &NamePolicyEngine{}, + domain: "xn--jp-cd2fp15c.xn--fsq.jp", // Example value from https://www.w3.org/International/articles/idn-and-iri/ + constraint: ".例.jp", + want: false, + wantErr: true, + }, { name: "ok/empty-constraint", engine: &NamePolicyEngine{}, @@ -169,6 +185,22 @@ func TestNamePolicyEngine_matchDomainConstraint(t *testing.T) { want: true, wantErr: false, }, + { + name: "ok/different-case", + engine: &NamePolicyEngine{}, + domain: "WWW.EXAMPLE.com", + constraint: "www.example.com", + want: true, + wantErr: false, + }, + { + name: "ok/idna-internationalized-domain-name-punycode", + engine: &NamePolicyEngine{}, + domain: "xn--jp-cd2fp15c.xn--fsq.jp", // Example value from https://www.w3.org/International/articles/idn-and-iri/ + constraint: ".xn--fsq.jp", + want: true, + wantErr: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -413,6 +445,17 @@ func TestNamePolicyEngine_matchEmailConstraint(t *testing.T) { want: true, wantErr: false, }, + { + name: "ok/different-case", + engine: &NamePolicyEngine{}, + mailbox: rfc2821Mailbox{ + local: "mail", + domain: "EXAMPLE.com", + }, + constraint: "example.com", // "wildcard" for 'example.com' + want: true, + wantErr: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -558,6 +601,17 @@ func TestNamePolicyEngine_matchURIConstraint(t *testing.T) { want: true, wantErr: false, }, + { + name: "ok/different-case", + engine: &NamePolicyEngine{}, + uri: &url.URL{ + Scheme: "https", + Host: "www.EXAMPLE.local", + }, + constraint: ".example.local", // using x509 period as the "wildcard"; expects a single subdomain + want: true, + wantErr: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -573,7 +627,23 @@ func TestNamePolicyEngine_matchURIConstraint(t *testing.T) { } } -func TestNamePolicyEngine_AreCertificateNamesAllowed(t *testing.T) { +func extractSANs(cert *x509.Certificate, includeSubject bool) []string { + sans := []string{} + sans = append(sans, cert.DNSNames...) + for _, ip := range cert.IPAddresses { + sans = append(sans, ip.String()) + } + sans = append(sans, cert.EmailAddresses...) + for _, uri := range cert.URIs { + sans = append(sans, uri.String()) + } + if includeSubject && cert.Subject.CommonName != "" { + sans = append(sans, cert.Subject.CommonName) + } + return sans +} + +func TestNamePolicyEngine_X509_AllAllowed(t *testing.T) { tests := []struct { name string options []NamePolicyOption @@ -2048,14 +2118,422 @@ func TestNamePolicyEngine_AreCertificateNamesAllowed(t *testing.T) { t.Run(tt.name, func(t *testing.T) { engine, err := New(tt.options...) assert.FatalError(t, err) - got, err := engine.AreCertificateNamesAllowed(tt.cert) // TODO: perform tests on CSR, sans, etc. too + got, err := engine.AreCertificateNamesAllowed(tt.cert) if (err != nil) != tt.wantErr { t.Errorf("NamePolicyEngine.AreCertificateNamesAllowed() error = %v, wantErr %v", err, tt.wantErr) return } + if err != nil { + assert.NotEquals(t, "", err.Error()) // TODO(hs): implement a more specific error comparison? + } if got != tt.want { t.Errorf("NamePolicyEngine.AreCertificateNamesAllowed() = %v, want %v", got, tt.want) } + + // Perform the same tests for a CSR, which are similar to Certificates + csr := &x509.CertificateRequest{ + Subject: tt.cert.Subject, + DNSNames: tt.cert.DNSNames, + EmailAddresses: tt.cert.EmailAddresses, + IPAddresses: tt.cert.IPAddresses, + URIs: tt.cert.URIs, + } + got, err = engine.AreCSRNamesAllowed(csr) + if (err != nil) != tt.wantErr { + t.Errorf("NamePolicyEngine.AreCSRNamesAllowed() error = %v, wantErr %v", err, tt.wantErr) + return + } + if err != nil { + assert.NotEquals(t, "", err.Error()) + } + if got != tt.want { + t.Errorf("NamePolicyEngine.AreCSRNamesAllowed() = %v, want %v", got, tt.want) + } + + // Perform the same tests for a slice of SANs + includeSubject := engine.verifySubjectCommonName // copy behavior of the engine when Subject has to be included as a SAN + sans := extractSANs(tt.cert, includeSubject) + got, err = engine.AreSANsAllowed(sans) + if (err != nil) != tt.wantErr { + t.Errorf("NamePolicyEngine.AreSANsAllowed() error = %v, wantErr %v", err, tt.wantErr) + return + } + if err != nil { + assert.NotEquals(t, "", err.Error()) + } + if got != tt.want { + t.Errorf("NamePolicyEngine.AreSANsAllowed() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) { + tests := []struct { + name string + options []NamePolicyOption + cert *ssh.Certificate + want bool + wantErr bool + }{ + { + name: "fail/with-permitted-dns-domain", + options: []NamePolicyOption{ + WithPermittedDNSDomain("*.local"), + }, + cert: &ssh.Certificate{ + ValidPrincipals: []string{ + "host.example.com", + }, + }, + want: false, + wantErr: true, + }, + { + name: "fail/with-excluded-dns-domain", + options: []NamePolicyOption{ + WithExcludedDNSDomain("*.local"), + }, + cert: &ssh.Certificate{ + ValidPrincipals: []string{ + "host.local", + }, + }, + want: false, + wantErr: true, + }, + { + name: "fail/with-permitted-ip", + options: []NamePolicyOption{ + WithPermittedCIDR("127.0.0.1/24"), + }, + cert: &ssh.Certificate{ + ValidPrincipals: []string{ + "192.168.0.22", + }, + }, + want: false, + wantErr: true, + }, + { + name: "fail/with-excluded-ip", + options: []NamePolicyOption{ + WithExcludedCIDR("127.0.0.1/24"), + }, + cert: &ssh.Certificate{ + ValidPrincipals: []string{ + "127.0.0.0", + }, + }, + want: false, + wantErr: true, + }, + { + name: "fail/with-permitted-email", + options: []NamePolicyOption{ + WithPermittedEmailAddress("@example.com"), + }, + cert: &ssh.Certificate{ + ValidPrincipals: []string{ + "mail@local", + }, + }, + want: false, + wantErr: true, + }, + { + name: "fail/with-excluded-email", + options: []NamePolicyOption{ + WithExcludedEmailAddress("@example.com"), + }, + cert: &ssh.Certificate{ + ValidPrincipals: []string{ + "mail@example.com", + }, + }, + want: false, + wantErr: true, + }, + { + name: "fail/with-permitted-principals", + options: []NamePolicyOption{ + WithPermittedPrincipals([]string{"user"}), + }, + cert: &ssh.Certificate{ + ValidPrincipals: []string{ + "root", + }, + }, + want: false, + wantErr: true, + }, + { + name: "fail/with-excluded-principals", + options: []NamePolicyOption{ + WithExcludedPrincipals([]string{"user"}), + }, + cert: &ssh.Certificate{ + ValidPrincipals: []string{ + "user", + }, + }, + want: false, + wantErr: true, + }, + { + name: "fail/with-permitted-principal-as-mail", + options: []NamePolicyOption{ + WithPermittedPrincipals([]string{"ops"}), + }, + cert: &ssh.Certificate{ + ValidPrincipals: []string{ + "ops@work", // this is (currently) parsed as an email-like principal; not allowed with just "ops" as the permitted principal + }, + }, + want: false, + wantErr: true, + }, + { + name: "fail/principal-with-permitted-dns-domain", // when only DNS is permitted, username principals are not allowed. + options: []NamePolicyOption{ + WithPermittedDNSDomain("*.local"), + }, + cert: &ssh.Certificate{ + ValidPrincipals: []string{ + "user", + }, + }, + want: false, + wantErr: true, + }, + { + name: "fail/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{ + ValidPrincipals: []string{ + "user", + }, + }, + want: false, + wantErr: true, + }, + { + name: "fail/principal-with-permitted-email", // when only emails are permitted, username principals are not allowed. + options: []NamePolicyOption{ + WithPermittedEmailAddress("@example.com"), + }, + cert: &ssh.Certificate{ + ValidPrincipals: []string{ + "user", + }, + }, + want: false, + wantErr: true, + }, + { + name: "fail/combined-user", + options: []NamePolicyOption{ + WithPermittedEmailAddress("@smallstep.com"), + WithExcludedEmailAddress("root@smallstep.com"), + }, + cert: &ssh.Certificate{ + ValidPrincipals: []string{ + "someone@smallstep.com", + "someone", + }, + }, + want: false, + wantErr: true, + }, + { + name: "fail/combined-user-with-excluded-user-principal", + options: []NamePolicyOption{ + WithPermittedEmailAddress("@smallstep.com"), + WithExcludedPrincipals([]string{"root"}), + }, + cert: &ssh.Certificate{ + ValidPrincipals: []string{ + "someone@smallstep.com", + "root", + }, + }, + want: false, + wantErr: true, + }, + { + name: "ok/with-permitted-dns-domain", + options: []NamePolicyOption{ + WithPermittedDNSDomain("*.local"), + }, + cert: &ssh.Certificate{ + ValidPrincipals: []string{ + "host.local", + }, + }, + want: true, + wantErr: false, + }, + { + name: "ok/with-excluded-dns-domain", + options: []NamePolicyOption{ + WithExcludedDNSDomain("*.example.com"), + }, + cert: &ssh.Certificate{ + ValidPrincipals: []string{ + "host.local", + }, + }, + want: true, + wantErr: false, + }, + { + name: "ok/with-permitted-ip", + options: []NamePolicyOption{ + WithPermittedCIDR("127.0.0.1/24"), + }, + cert: &ssh.Certificate{ + ValidPrincipals: []string{ + "127.0.0.33", + }, + }, + want: true, + wantErr: false, + }, + { + name: "ok/with-excluded-ip", + options: []NamePolicyOption{ + WithExcludedCIDR("127.0.0.1/24"), + }, + cert: &ssh.Certificate{ + ValidPrincipals: []string{ + "192.168.0.35", + }, + }, + want: true, + wantErr: false, + }, + { + name: "ok/with-permitted-email", + options: []NamePolicyOption{ + WithPermittedEmailAddress("@example.com"), + }, + cert: &ssh.Certificate{ + ValidPrincipals: []string{ + "mail@example.com", + }, + }, + want: true, + wantErr: false, + }, + { + name: "ok/with-excluded-email", + options: []NamePolicyOption{ + WithExcludedEmailAddress("@example.com"), + }, + cert: &ssh.Certificate{ + ValidPrincipals: []string{ + "mail@local", + }, + }, + want: true, + wantErr: false, + }, + { + name: "ok/with-permitted-principals", + options: []NamePolicyOption{ + WithPermittedPrincipals([]string{"*"}), + }, + cert: &ssh.Certificate{ + ValidPrincipals: []string{ + "user", + }, + }, + want: true, + wantErr: false, + }, + { + name: "ok/with-excluded-principals", + options: []NamePolicyOption{ + WithExcludedPrincipals([]string{"user"}), + }, + cert: &ssh.Certificate{ + ValidPrincipals: []string{ + "root", + }, + }, + want: true, + wantErr: false, + }, + { + name: "ok/combined-user", + options: []NamePolicyOption{ + WithPermittedEmailAddress("@smallstep.com"), + WithPermittedPrincipals([]string{"*"}), // without specifying the wildcard, "someone" would not be allowed. + WithExcludedEmailAddress("root@smallstep.com"), + }, + cert: &ssh.Certificate{ + ValidPrincipals: []string{ + "someone@smallstep.com", + "someone", + }, + }, + want: true, + wantErr: false, + }, + { + name: "ok/combined-user-with-excluded-user-principal", + options: []NamePolicyOption{ + WithPermittedEmailAddress("@smallstep.com"), + WithExcludedEmailAddress("root@smallstep.com"), + WithExcludedPrincipals([]string{"root"}), // unlike the previous test, this implicitly allows any other username principal + }, + cert: &ssh.Certificate{ + ValidPrincipals: []string{ + "someone@smallstep.com", + "someone", + }, + }, + want: true, + wantErr: false, + }, + { + name: "ok/combined-simple-all", + 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{ + ValidPrincipals: []string{ + "example.local", + "127.0.0.1", + "user@example.local", + "user", + }, + }, + want: true, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + engine, err := New(tt.options...) + assert.FatalError(t, err) + got, err := engine.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) + } }) } } diff --git a/policy/options.go b/policy/options.go index f628a083..b1fbba70 100755 --- a/policy/options.go +++ b/policy/options.go @@ -204,6 +204,42 @@ func AddExcludedCIDRs(cidrs []string) NamePolicyOption { } } +func WithPermittedIPsOrCIDRs(ipsOrCIDRs []string) NamePolicyOption { + return func(e *NamePolicyEngine) error { + networks := make([]*net.IPNet, len(ipsOrCIDRs)) + for i, ipOrCIDR := range ipsOrCIDRs { + _, nw, err := net.ParseCIDR(ipOrCIDR) + if err == nil { + networks[i] = nw + } else if ip := net.ParseIP(ipOrCIDR); ip != nil { + networks[i] = networkFor(ip) + } else { + return errors.Errorf("cannot parse permitted constraint %q as IP nor CIDR", ipOrCIDR) + } + } + e.permittedIPRanges = networks + return nil + } +} + +func WithExcludedIPsOrCIDRs(ipsOrCIDRs []string) NamePolicyOption { + return func(e *NamePolicyEngine) error { + networks := make([]*net.IPNet, len(ipsOrCIDRs)) + for i, ipOrCIDR := range ipsOrCIDRs { + _, nw, err := net.ParseCIDR(ipOrCIDR) + if err == nil { + networks[i] = nw + } else if ip := net.ParseIP(ipOrCIDR); ip != nil { + networks[i] = networkFor(ip) + } else { + return errors.Errorf("cannot parse excluded constraint %q as IP nor CIDR", ipOrCIDR) + } + } + e.excludedIPRanges = networks + return nil + } +} + func WithPermittedCIDR(cidr string) NamePolicyOption { return func(e *NamePolicyEngine) error { _, nw, err := net.ParseCIDR(cidr) @@ -228,16 +264,7 @@ func AddPermittedCIDR(cidr string) NamePolicyOption { func WithPermittedIP(ip net.IP) NamePolicyOption { return func(e *NamePolicyEngine) error { - var mask net.IPMask - if !isIPv4(ip) { - mask = net.CIDRMask(128, 128) - } else { - mask = net.CIDRMask(32, 32) - } - nw := &net.IPNet{ - IP: ip, - Mask: mask, - } + nw := networkFor(ip) e.permittedIPRanges = []*net.IPNet{nw} return nil } @@ -245,16 +272,7 @@ func WithPermittedIP(ip net.IP) NamePolicyOption { func AddPermittedIP(ip net.IP) NamePolicyOption { return func(e *NamePolicyEngine) error { - var mask net.IPMask - if !isIPv4(ip) { - mask = net.CIDRMask(128, 128) - } else { - mask = net.CIDRMask(32, 32) - } - nw := &net.IPNet{ - IP: ip, - Mask: mask, - } + nw := networkFor(ip) e.permittedIPRanges = append(e.permittedIPRanges, nw) return nil } @@ -540,9 +558,7 @@ func AddExcludedURIDomain(uriDomain string) NamePolicyOption { func WithPermittedPrincipals(principals []string) NamePolicyOption { return func(g *NamePolicyEngine) error { - // for _, principal := range principals { - // // TODO: validation? - // } + // TODO(hs): normalize and parse principal into the right type? Seems the safe thing to do. g.permittedPrincipals = principals return nil } @@ -550,16 +566,32 @@ func WithPermittedPrincipals(principals []string) NamePolicyOption { func WithExcludedPrincipals(principals []string) NamePolicyOption { return func(g *NamePolicyEngine) error { - // for _, principal := range principals { - // // TODO: validation? - // } + // TODO(hs): normalize and parse principal into the right type? Seems the safe thing to do. g.excludedPrincipals = principals return nil } } +func networkFor(ip net.IP) *net.IPNet { + var mask net.IPMask + if !isIPv4(ip) { + mask = net.CIDRMask(128, 128) + } else { + mask = net.CIDRMask(32, 32) + } + nw := &net.IPNet{ + IP: ip, + Mask: mask, + } + return nw +} + +func isIPv4(ip net.IP) bool { + return ip.To4() != nil +} + func normalizeAndValidateDNSDomainConstraint(constraint string) (string, error) { - normalizedConstraint := strings.TrimSpace(constraint) + normalizedConstraint := strings.ToLower(strings.TrimSpace(constraint)) if strings.Contains(normalizedConstraint, "..") { return "", errors.Errorf("domain constraint %q cannot have empty labels", constraint) } @@ -576,7 +608,7 @@ func normalizeAndValidateDNSDomainConstraint(constraint string) (string, error) } func normalizeAndValidateEmailConstraint(constraint string) (string, error) { - normalizedConstraint := strings.TrimSpace(constraint) + normalizedConstraint := strings.ToLower(strings.TrimSpace(constraint)) if strings.Contains(normalizedConstraint, "*") { return "", fmt.Errorf("email constraint %q cannot contain asterisk", constraint) } @@ -601,7 +633,7 @@ func normalizeAndValidateEmailConstraint(constraint string) (string, error) { } func normalizeAndValidateURIDomainConstraint(constraint string) (string, error) { - normalizedConstraint := strings.TrimSpace(constraint) + normalizedConstraint := strings.ToLower(strings.TrimSpace(constraint)) if strings.Contains(normalizedConstraint, "..") { return "", errors.Errorf("URI domain constraint %q cannot have empty labels", constraint) } diff --git a/policy/options_test.go b/policy/options_test.go index 5e84d20e..7f417887 100644 --- a/policy/options_test.go +++ b/policy/options_test.go @@ -33,6 +33,18 @@ func Test_normalizeAndValidateDNSDomainConstraint(t *testing.T) { want: "", wantErr: true, }, + { + name: "false/idna-internationalized-domain-name", + constraint: ".例.jp", // Example value from https://www.w3.org/International/articles/idn-and-iri/ + want: "", + wantErr: true, + }, + { + name: "false/idna-internationalized-domain-name-constraint", + constraint: ".例.jp", // Example value from https://www.w3.org/International/articles/idn-and-iri/ + want: "", + wantErr: true, + }, { name: "ok/wildcard", constraint: "*.local", @@ -45,6 +57,12 @@ func Test_normalizeAndValidateDNSDomainConstraint(t *testing.T) { want: "example.local", wantErr: false, }, + { + name: "ok/idna-internationalized-domain-name-punycode", + constraint: ".xn--fsq.jp", // Example value from https://www.w3.org/International/articles/idn-and-iri/ + want: ".xn--fsq.jp", + wantErr: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -297,6 +315,42 @@ func TestNew(t *testing.T) { wantErr: true, } }, + "fail/with-permitted-ipsOrCIDRs-cidr": func(t *testing.T) test { + return test{ + options: []NamePolicyOption{ + WithPermittedIPsOrCIDRs([]string{"127.0.0.1//24"}), + }, + want: nil, + wantErr: true, + } + }, + "fail/with-permitted-ipsOrCIDRs-ip": func(t *testing.T) test { + return test{ + options: []NamePolicyOption{ + WithPermittedIPsOrCIDRs([]string{"127.0.0:1"}), + }, + want: nil, + wantErr: true, + } + }, + "fail/with-excluded-ipsOrCIDRs-cidr": func(t *testing.T) test { + return test{ + options: []NamePolicyOption{ + WithExcludedIPsOrCIDRs([]string{"127.0.0.1//24"}), + }, + want: nil, + wantErr: true, + } + }, + "fail/with-excluded-ipsOrCIDRs-ip": func(t *testing.T) test { + return test{ + options: []NamePolicyOption{ + WithExcludedIPsOrCIDRs([]string{"127.0.0:1"}), + }, + want: nil, + wantErr: true, + } + }, "fail/with-permitted-cidr": func(t *testing.T) test { return test{ options: []NamePolicyOption{ @@ -828,6 +882,48 @@ func TestNew(t *testing.T) { wantErr: false, } }, + "ok/with-permitted-ipsOrCIDRs-cidr": func(t *testing.T) test { + _, nw1, err := net.ParseCIDR("127.0.0.1/24") + assert.FatalError(t, err) + _, nw2, err := net.ParseCIDR("192.168.0.31/32") + assert.FatalError(t, err) + options := []NamePolicyOption{ + WithPermittedIPsOrCIDRs([]string{"127.0.0.1/24", "192.168.0.31"}), + } + return test{ + options: options, + want: &NamePolicyEngine{ + permittedIPRanges: []*net.IPNet{ + nw1, nw2, + }, + numberOfIPRangeConstraints: 2, + totalNumberOfPermittedConstraints: 2, + totalNumberOfConstraints: 2, + }, + wantErr: false, + } + }, + "ok/with-excluded-ipsOrCIDRs-cidr": func(t *testing.T) test { + _, nw1, err := net.ParseCIDR("127.0.0.1/24") + assert.FatalError(t, err) + _, nw2, err := net.ParseCIDR("192.168.0.31/32") + assert.FatalError(t, err) + options := []NamePolicyOption{ + WithExcludedIPsOrCIDRs([]string{"127.0.0.1/24", "192.168.0.31"}), + } + return test{ + options: options, + want: &NamePolicyEngine{ + excludedIPRanges: []*net.IPNet{ + nw1, nw2, + }, + numberOfIPRangeConstraints: 2, + totalNumberOfExcludedConstraints: 2, + totalNumberOfConstraints: 2, + }, + wantErr: false, + } + }, "ok/with-permitted-cidr": func(t *testing.T) test { _, nw1, err := net.ParseCIDR("127.0.0.1/24") assert.FatalError(t, err) @@ -1322,6 +1418,36 @@ func TestNew(t *testing.T) { wantErr: false, } }, + "ok/with-permitted-principals": func(t *testing.T) test { + options := []NamePolicyOption{ + WithPermittedPrincipals([]string{"root", "ops"}), + } + return test{ + options: options, + want: &NamePolicyEngine{ + permittedPrincipals: []string{"root", "ops"}, + numberOfPrincipalConstraints: 2, + totalNumberOfPermittedConstraints: 2, + totalNumberOfConstraints: 2, + }, + wantErr: false, + } + }, + "ok/with-excluded-principals": func(t *testing.T) test { + options := []NamePolicyOption{ + WithExcludedPrincipals([]string{"root", "ops"}), + } + return test{ + options: options, + want: &NamePolicyEngine{ + excludedPrincipals: []string{"root", "ops"}, + numberOfPrincipalConstraints: 2, + totalNumberOfExcludedConstraints: 2, + totalNumberOfConstraints: 2, + }, + wantErr: false, + } + }, } for name, prep := range tests { tc := prep(t)