From fafb6ae48c6c40aa011d87b61306abc48db8797b Mon Sep 17 00:00:00 2001 From: Ben Ransford Date: Sat, 27 Nov 2021 12:42:41 -0800 Subject: [PATCH] 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 | 49 +++++++++----- pkg/smokescreen/acl/v1/acl_test.go | 101 +++++++++++++++++++++++++++-- 2 files changed, 126 insertions(+), 24 deletions(-) diff --git a/pkg/smokescreen/acl/v1/acl.go b/pkg/smokescreen/acl/v1/acl.go index bf913c0b..a8536367 100644 --- a/pkg/smokescreen/acl/v1/acl.go +++ b/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 diff --git a/pkg/smokescreen/acl/v1/acl_test.go b/pkg/smokescreen/acl/v1/acl_test.go index d89542e9..19b26cb8 100644 --- a/pkg/smokescreen/acl/v1/acl_test.go +++ b/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), + ) + }) + } +}