improvements to ACL comparisons

- canonicalize hostname and glob to lowercase before comparison
- ignore legal trailing dot in comparison
- reject globs matching all hostnames (#145)
- improve error message on glob validation errors
- add comments clarifying intentions
- add unit tests

pkg/smokescreen/acl/v1/acl.go

@@ -66,7 +66,7 @@ func (acl *ACL) Add(svc string, r Rule) error {
 		return err
 	}
 
-	err = acl.ValidateDomains(r.DomainGlobs)
+	err = acl.ValidateDomainGlobs(svc, r.DomainGlobs)
 	if err != nil {
 		return err
 	}
@@ -158,7 +158,7 @@ func (acl *ACL) DisablePolicies(actions []string) error {
 // and is not utilizing a disabled enforcement policy.
 func (acl *ACL) Validate() error {
 	for svc, r := range acl.Rules {
-		err := acl.ValidateDomains(r.DomainGlobs)
+		err := acl.ValidateDomainGlobs(svc, r.DomainGlobs)
 		if err != nil {
 			return err
 		}
@@ -170,24 +170,28 @@ func (acl *ACL) Validate() error {
 	return nil
 }
 
-// ValidateDomains takes a slice of domains and verifies they conform to
-// smokescreen's domain glob policy.
+// ValidateDomainGlobs takes a slice of domain globs and verifies they conform to smokescreen's
+// domain glob policy.
 //
-// Domains can only contain a single wildcard prefix
-// Domains cannot be represented as a sole wildcard
-func (acl *ACL) ValidateDomains(domains []string) error {
-	for _, d := range domains {
-		if d == "" {
+// Wildcards are valid only at the beginning of a domain glob, and only a single wildcard per glob
+// pattern is allowed. Globs must include text after a wildcard.
+func (acl *ACL) ValidateDomainGlobs(svc string, globs []string) error {
+	for _, glob := range globs {
+		if glob == "" {
 			return fmt.Errorf("glob cannot be empty")
 		}
 
-		if !strings.HasPrefix(d, "*.") && strings.HasPrefix(d, "*") {
-			return fmt.Errorf("%v: domain glob must represent a full prefix (sub)domain", d)
+		if glob == "*" || glob == "*." {
+			return fmt.Errorf("%v: %v: domain glob must not match everything", svc, glob)
 		}
 
-		domainToCheck := strings.TrimPrefix(d, "*")
+		if !strings.HasPrefix(glob, "*.") && strings.HasPrefix(glob, "*") {
+			return fmt.Errorf("%v: %v: domain glob must represent a full prefix (sub)domain", svc, glob)
+		}
+
+		domainToCheck := strings.TrimPrefix(glob, "*")
 		if strings.Contains(domainToCheck, "*") {
-			return fmt.Errorf("%v: domain globs are only supported as prefix", d)
+			return fmt.Errorf("%v: %v: domain globs are only supported as prefix", svc, glob)
 		}
 	}
 	return nil
@@ -221,13 +225,24 @@ func (acl *ACL) Rule(service string) *Rule {
 	return acl.DefaultRule
 }
 
+// hostMatchesGlob matches a hostname string against a domain glob after
+// converting both to a canonical form (lowercase with trailing dots removed).
+//
+// domainGlob should already have been passed through ACL.Validate().
 func hostMatchesGlob(host string, domainGlob string) bool {
-	if domainGlob != "" && domainGlob[0] == '*' {
-		suffix := domainGlob[1:]
-		if strings.HasSuffix(host, suffix) {
+	if host == "" {
+		return false
+	}
+
+	h := strings.TrimRight(strings.ToLower(host), ".")
+	g := strings.TrimRight(strings.ToLower(domainGlob), ".")
+
+	if strings.HasPrefix(g, "*.") {
+		suffix := g[1:]
+		if strings.HasSuffix(h, suffix) {
 			return true
 		}
-	} else if domainGlob == host {
+	} else if g == h {
 		return true
 	}
 	return false

pkg/smokescreen/acl/v1/acl_test.go

@@ -1,3 +1,4 @@
+//go:build !nounit
 // +build !nounit
 
 package acl
@@ -144,6 +145,22 @@ var testCases = map[string]struct {
 		"host matched rule in global deny list",
 		"automation",
 	},
+	"deny from global denylist trailing dot open service": {
+		"sample_config_with_global.yaml",
+		"open-dummy-srv",
+		"badexample2.com.",
+		Deny,
+		"host matched rule in global deny list",
+		"automation",
+	},
+	"deny from global denylist case mismatch open service": {
+		"sample_config_with_global.yaml",
+		"open-dummy-srv",
+		"bAdExAmPlE2.cOm",
+		Deny,
+		"host matched rule in global deny list",
+		"automation",
+	},
 	"deny from conflicting lists open service": {
 		"sample_config_with_global.yaml",
 		"open-dummy-srv",
@@ -220,20 +237,28 @@ func TestACLMalformedPolicyDisable(t *testing.T) {
 	assert.Error(t, err)
 }
 
-func TestACLAddInvalidDomain(t *testing.T) {
+func TestACLAddInvalidGlob(t *testing.T) {
 	a := assert.New(t)
 
+	invalidGlobs := []string{
+		"*.*.stripe.com", // multiple wildcards
+		"*",              // matches everything
+		"*.",             // matches everything
+	}
+
 	acl := &ACL{
 		Rules: make(map[string]Rule),
 	}
 
-	r := Rule{
-		Project:     "security",
-		Policy:      Open,
-		DomainGlobs: []string{"*.*.stripe.com"},
-	}
+	for _, glob := range invalidGlobs {
+		err := acl.Add("acl", Rule{
+			Project:     "security",
+			Policy:      Open,
+			DomainGlobs: []string{glob},
+		})
 
-	a.Error(acl.Add("acl", r))
+		a.Errorf(err, "did not reject invalid glob %q", glob)
+	}
 }
 
 func TestACLAddExistingRule(t *testing.T) {
@@ -253,3 +278,65 @@ func TestACLAddExistingRule(t *testing.T) {
 	a.NoError(acl.Add(svc, r))
 	a.Error(acl.Add(svc, r))
 }
+
+// TestHostMatchesGlob tests that hostnames and variants match as expected against domain globs.
+// Does not test hostnames against globs that do not conform to the glob policy of
+// ACL.ValidateDomainGlobs(), as these would already have been rejected during ACL validation.
+func TestHostMatchesGlob(t *testing.T) {
+	globs := map[string]struct {
+		hostname string
+		glob     string
+		match    bool
+	}{
+		"simple match": {
+			"example.com",
+			"example.com",
+			true,
+		},
+		"leading wildcard matches first component": {
+			"contrived.example.com",
+			"*.example.com",
+			true,
+		},
+		"leading wildcard matches first two components": {
+			"more.contrived.example.com",
+			"*.example.com",
+			true,
+		},
+		"wildcard after leading component": {
+			"login.eu.example.com",
+			"login.*.example.com",
+			false,
+		},
+		"trailing dot": {
+			"example.com.",
+			"example.com",
+			true,
+		},
+		"uppercase host with lowercase glob": {
+			"EXAMPLE.COM",
+			"example.com",
+			true,
+		},
+		"lowercase host with uppercase glob": {
+			"example.com",
+			"EXAMPLE.COM",
+			true,
+		},
+		"empty hostname": {
+			"",
+			"example.com",
+			false,
+		},
+	}
+
+	a := assert.New(t)
+	for name, g := range globs {
+		t.Run(name, func(t *testing.T) {
+			a.Equal(
+				g.match,
+				hostMatchesGlob(g.hostname, g.glob),
+			)
+		})
+	}
+}