From 36d1045e7ff61d2d363dedad89e78c8fa3ebce1d Mon Sep 17 00:00:00 2001 From: Danielle Tomlinson Date: Thu, 8 Nov 2018 12:09:00 -0800 Subject: [PATCH 1/4] acl: Add support for globbing namespaces This commit adds basic support for globbing namespaces in acl definitions. For concrete definitions, we merge all of the defined policies at load time, and perform a simple lookup later on. If an exact match of a concrete definition is found, we do not attempt to resolve globs. For glob definitions, we merge definitions of exact replicas of a glob. When loading a policy for a glob defintion, we choose the glob that has the closest match to the namespace we are resolving for. We define the closest match as the one with the _smallest character difference_ between the glob and the namespace we are matching. --- acl/acl.go | 106 ++++++++++++++++++++++++++++++++++++++++++++---- acl/acl_test.go | 93 ++++++++++++++++++++++++++++++++++++++++++ acl/policy.go | 2 +- 3 files changed, 191 insertions(+), 10 deletions(-) diff --git a/acl/acl.go b/acl/acl.go index 01f04062a2be..fcb46d230e86 100644 --- a/acl/acl.go +++ b/acl/acl.go @@ -2,8 +2,11 @@ package acl import ( "fmt" + "sort" + "strings" iradix "github.com/hashicorp/go-immutable-radix" + glob "github.com/ryanuber/go-glob" ) // ManagementACL is a singleton used for management tokens @@ -44,6 +47,9 @@ type ACL struct { // namespaces maps a namespace to a capabilitySet namespaces *iradix.Tree + // wildcardNamespaces maps a glob pattern of a namespace to a capabilitySet + wildcardNamespaces map[string]capabilitySet + agent string node string operator string @@ -75,18 +81,33 @@ func NewACL(management bool, policies []*Policy) (*ACL, error) { // Create the ACL object acl := &ACL{} nsTxn := iradix.New().Txn() + wns := make(map[string]capabilitySet) for _, policy := range policies { NAMESPACES: for _, ns := range policy.Namespaces { + // Should the namespace be matched using a glob? + globDefinition := strings.Contains(ns.Name, "*") + // Check for existing capabilities var capabilities capabilitySet - raw, ok := nsTxn.Get([]byte(ns.Name)) - if ok { - capabilities = raw.(capabilitySet) + + if globDefinition { + raw, ok := wns[ns.Name] + if ok { + capabilities = raw + } else { + capabilities = make(capabilitySet) + wns[ns.Name] = capabilities + } } else { - capabilities = make(capabilitySet) - nsTxn.Insert([]byte(ns.Name), capabilities) + raw, ok := nsTxn.Get([]byte(ns.Name)) + if ok { + capabilities = raw.(capabilitySet) + } else { + capabilities = make(capabilitySet) + nsTxn.Insert([]byte(ns.Name), capabilities) + } } // Deny always takes precedence @@ -123,6 +144,7 @@ func NewACL(management bool, policies []*Policy) (*ACL, error) { // Finalize the namespaces acl.namespaces = nsTxn.Commit() + acl.wildcardNamespaces = wns return acl, nil } @@ -139,13 +161,12 @@ func (a *ACL) AllowNamespaceOperation(ns string, op string) bool { } // Check for a matching capability set - raw, ok := a.namespaces.Get([]byte(ns)) + capabilities, ok := a.matchingCapabilitySet(ns) if !ok { return false } // Check if the capability has been granted - capabilities := raw.(capabilitySet) return capabilities.Check(op) } @@ -157,13 +178,12 @@ func (a *ACL) AllowNamespace(ns string) bool { } // Check for a matching capability set - raw, ok := a.namespaces.Get([]byte(ns)) + capabilities, ok := a.matchingCapabilitySet(ns) if !ok { return false } // Check if the capability has been granted - capabilities := raw.(capabilitySet) if len(capabilities) == 0 { return false } @@ -171,6 +191,74 @@ func (a *ACL) AllowNamespace(ns string) bool { return !capabilities.Check(PolicyDeny) } +// matchingCapabilitySet looks for a capabilitySet that matches the namespace, +// if no concrete definitions are found, then we return the closest matching +// glob. +// The closest matching glob is the one that has the smallest character +// difference between the namespace and the glob. +func (a *ACL) matchingCapabilitySet(ns string) (capabilitySet, bool) { + // Check for a concrete matching capability set + raw, ok := a.namespaces.Get([]byte(ns)) + if ok { + return raw.(capabilitySet), true + } + + // We didn't find a concrete match, so lets try and evaluate globs. + cs, ok := a.findClosestMatchingGlob(ns) + + return cs, ok +} + +type matchingGlob struct { + ns string + nsLen int + capabilitySet capabilitySet +} + +func (a *ACL) findClosestMatchingGlob(ns string) (capabilitySet, bool) { + // First, find all globs that match. + matchingGlobs := a.findAllMatchingWildcards(ns) + + // If none match, let's return. + if len(matchingGlobs) == 0 { + return capabilitySet{}, false + } + + // If a single matches, lets be efficient and return early. + if len(matchingGlobs) == 1 { + return matchingGlobs[0].capabilitySet, true + } + + nsLen := len(ns) + + // Stable sort the matched globs, based on the character difference between + // the glob definition and the requested namespace. This allows us to be + // more consistent about results based on the policy definition. + sort.SliceStable(matchingGlobs, func(i, j int) bool { + return (matchingGlobs[i].nsLen - nsLen) >= (matchingGlobs[j].nsLen - nsLen) + }) + + return matchingGlobs[0].capabilitySet, true +} + +func (a *ACL) findAllMatchingWildcards(ns string) []matchingGlob { + var matches []matchingGlob + + for k, v := range a.wildcardNamespaces { + isMatch := glob.Glob(string(k), ns) + if isMatch { + pair := matchingGlob{ + ns: k, + nsLen: len(k), + capabilitySet: v, + } + matches = append(matches, pair) + } + } + + return matches +} + // AllowAgentRead checks if read operations are allowed for an agent func (a *ACL) AllowAgentRead() bool { switch { diff --git a/acl/acl_test.go b/acl/acl_test.go index 50fa8b5763da..9a6c5904f8cb 100644 --- a/acl/acl_test.go +++ b/acl/acl_test.go @@ -262,3 +262,96 @@ func TestAllowNamespace(t *testing.T) { }) } } + +func TestWildcardNamespaceMatching(t *testing.T) { + tests := []struct { + Policy string + Allow bool + }{ + { // Wildcard matches + Policy: `namespace "prod-api-*" { policy = "write" }`, + Allow: true, + }, + { // Non globbed namespaces are not wildcards + Policy: `namespace "prod-api" { policy = "write" }`, + Allow: false, + }, + { // Concrete matches take precedence + Policy: `namespace "prod-api-services" { policy = "deny" } + namespace "prod-api-*" { policy = "write" }`, + Allow: false, + }, + { + Policy: `namespace "prod-api-*" { policy = "deny" } + namespace "prod-api-services" { policy = "write" }`, + Allow: true, + }, + { // The closest character match wins + Policy: `namespace "*-api-services" { policy = "deny" } + namespace "prod-api-*" { policy = "write" }`, // 5 vs 8 chars + Allow: false, + }, + } + + for _, tc := range tests { + t.Run(tc.Policy, func(t *testing.T) { + assert := assert.New(t) + + policy, err := Parse(tc.Policy) + assert.NoError(err) + assert.NotNil(policy.Namespaces) + + acl, err := NewACL(false, []*Policy{policy}) + assert.Nil(err) + + assert.Equal(tc.Allow, acl.AllowNamespace("prod-api-services")) + }) + } +} + +func TestACL_matchingCapabilitySet(t *testing.T) { + tests := []struct { + Policy string + NS string + MatchingGlobs []string + }{ + { + Policy: `namespace "production-*" { policy = "write" }`, + NS: "production-api", + MatchingGlobs: []string{"production-*"}, + }, + { + Policy: `namespace "prod-*" { policy = "write" }`, + NS: "production-api", + MatchingGlobs: nil, + }, + { + Policy: `namespace "production-*" { policy = "write" } + namespace "production-*-api" { policy = "deny" }`, + + NS: "production-admin-api", + MatchingGlobs: []string{"production-*", "production-*-api"}, + }, + } + + for _, tc := range tests { + t.Run(tc.Policy, func(t *testing.T) { + assert := assert.New(t) + + policy, err := Parse(tc.Policy) + assert.NoError(err) + assert.NotNil(policy.Namespaces) + + acl, err := NewACL(false, []*Policy{policy}) + assert.Nil(err) + + var namespaces []string + for _, cs := range acl.findAllMatchingWildcards(tc.NS) { + namespaces = append(namespaces, cs.ns) + } + + assert.Equal(tc.MatchingGlobs, namespaces) + }) + } + +} diff --git a/acl/policy.go b/acl/policy.go index 5631f94748f7..db45f11940ec 100644 --- a/acl/policy.go +++ b/acl/policy.go @@ -32,7 +32,7 @@ const ( ) var ( - validNamespace = regexp.MustCompile("^[a-zA-Z0-9-]{1,128}$") + validNamespace = regexp.MustCompile("^[a-zA-Z0-9-*]{1,128}$") ) // Policy represents a parsed HCL or JSON policy. From db635bf81176b986c644a71bd237b2c36052cd4f Mon Sep 17 00:00:00 2001 From: Danielle Tomlinson Date: Tue, 11 Dec 2018 17:35:51 +0100 Subject: [PATCH 2/4] fixup: Correctly sort based on distance, use iradix for ordering --- acl/acl.go | 36 ++++++++++++++++++++++-------------- acl/acl_test.go | 37 ++++++++++++++++++++++++++++++++++++- 2 files changed, 58 insertions(+), 15 deletions(-) diff --git a/acl/acl.go b/acl/acl.go index fcb46d230e86..25f111152684 100644 --- a/acl/acl.go +++ b/acl/acl.go @@ -48,7 +48,8 @@ type ACL struct { namespaces *iradix.Tree // wildcardNamespaces maps a glob pattern of a namespace to a capabilitySet - wildcardNamespaces map[string]capabilitySet + // We use an iradix for the purposes of ordered iteration. + wildcardNamespaces *iradix.Tree agent string node string @@ -81,7 +82,7 @@ func NewACL(management bool, policies []*Policy) (*ACL, error) { // Create the ACL object acl := &ACL{} nsTxn := iradix.New().Txn() - wns := make(map[string]capabilitySet) + wnsTxn := iradix.New().Txn() for _, policy := range policies { NAMESPACES: @@ -93,12 +94,12 @@ func NewACL(management bool, policies []*Policy) (*ACL, error) { var capabilities capabilitySet if globDefinition { - raw, ok := wns[ns.Name] + raw, ok := wnsTxn.Get([]byte(ns.Name)) if ok { - capabilities = raw + capabilities = raw.(capabilitySet) } else { capabilities = make(capabilitySet) - wns[ns.Name] = capabilities + wnsTxn.Insert([]byte(ns.Name), capabilities) } } else { raw, ok := nsTxn.Get([]byte(ns.Name)) @@ -144,7 +145,7 @@ func NewACL(management bool, policies []*Policy) (*ACL, error) { // Finalize the namespaces acl.namespaces = nsTxn.Commit() - acl.wildcardNamespaces = wns + acl.wildcardNamespaces = wnsTxn.Commit() return acl, nil } @@ -211,7 +212,7 @@ func (a *ACL) matchingCapabilitySet(ns string) (capabilitySet, bool) { type matchingGlob struct { ns string - nsLen int + difference int capabilitySet capabilitySet } @@ -229,13 +230,11 @@ func (a *ACL) findClosestMatchingGlob(ns string) (capabilitySet, bool) { return matchingGlobs[0].capabilitySet, true } - nsLen := len(ns) - // Stable sort the matched globs, based on the character difference between // the glob definition and the requested namespace. This allows us to be // more consistent about results based on the policy definition. sort.SliceStable(matchingGlobs, func(i, j int) bool { - return (matchingGlobs[i].nsLen - nsLen) >= (matchingGlobs[j].nsLen - nsLen) + return matchingGlobs[i].difference <= matchingGlobs[j].difference }) return matchingGlobs[0].capabilitySet, true @@ -244,17 +243,26 @@ func (a *ACL) findClosestMatchingGlob(ns string) (capabilitySet, bool) { func (a *ACL) findAllMatchingWildcards(ns string) []matchingGlob { var matches []matchingGlob - for k, v := range a.wildcardNamespaces { - isMatch := glob.Glob(string(k), ns) + nsLen := len(ns) + + a.wildcardNamespaces.Root().Walk(func(bk []byte, iv interface{}) bool { + k := string(bk) + v := iv.(capabilitySet) + + isMatch := glob.Glob(k, ns) if isMatch { + globLen := len(strings.Replace(k, glob.GLOB, "", -1)) pair := matchingGlob{ ns: k, - nsLen: len(k), + difference: nsLen - globLen, capabilitySet: v, } matches = append(matches, pair) } - } + + // We always want to walk the entire tree, never terminate early. + return false + }) return matches } diff --git a/acl/acl_test.go b/acl/acl_test.go index 9a6c5904f8cb..e1cad58ae398 100644 --- a/acl/acl_test.go +++ b/acl/acl_test.go @@ -309,7 +309,7 @@ func TestWildcardNamespaceMatching(t *testing.T) { } } -func TestACL_matchingCapabilitySet(t *testing.T) { +func TestACL_matchingCapabilitySet_returnsAllMatches(t *testing.T) { tests := []struct { Policy string NS string @@ -353,5 +353,40 @@ func TestACL_matchingCapabilitySet(t *testing.T) { assert.Equal(tc.MatchingGlobs, namespaces) }) } +} + +func TestACL_matchingCapabilitySet_difference(t *testing.T) { + tests := []struct { + Policy string + NS string + Difference int + }{ + { + Policy: `namespace "production-*" { policy = "write" }`, + NS: "production-api", + Difference: 3, + }, + { + Policy: `namespace "production-*" { policy = "write" }`, + NS: "production-admin-api", + Difference: 9, + }, + } + + for _, tc := range tests { + t.Run(tc.Policy, func(t *testing.T) { + assert := assert.New(t) + + policy, err := Parse(tc.Policy) + assert.NoError(err) + assert.NotNil(policy.Namespaces) + + acl, err := NewACL(false, []*Policy{policy}) + assert.Nil(err) + + matches := acl.findAllMatchingWildcards(tc.NS) + assert.Equal(tc.Difference, matches[0].difference) + }) + } } From 4e59d473f797b13a8ba1b66e978b7416eceef4c8 Mon Sep 17 00:00:00 2001 From: Danielle Tomlinson Date: Wed, 12 Dec 2018 12:43:16 +0100 Subject: [PATCH 3/4] fixup: Code Review --- acl/acl.go | 7 ++----- acl/acl_test.go | 22 +++++++++++++++++++++- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/acl/acl.go b/acl/acl.go index 25f111152684..f5a76ea03c41 100644 --- a/acl/acl.go +++ b/acl/acl.go @@ -205,9 +205,7 @@ func (a *ACL) matchingCapabilitySet(ns string) (capabilitySet, bool) { } // We didn't find a concrete match, so lets try and evaluate globs. - cs, ok := a.findClosestMatchingGlob(ns) - - return cs, ok + return a.findClosestMatchingGlob(ns) } type matchingGlob struct { @@ -251,10 +249,9 @@ func (a *ACL) findAllMatchingWildcards(ns string) []matchingGlob { isMatch := glob.Glob(k, ns) if isMatch { - globLen := len(strings.Replace(k, glob.GLOB, "", -1)) pair := matchingGlob{ ns: k, - difference: nsLen - globLen, + difference: nsLen - len(k) + strings.Count(k, glob.GLOB), capabilitySet: v, } matches = append(matches, pair) diff --git a/acl/acl_test.go b/acl/acl_test.go index e1cad58ae398..523208233977 100644 --- a/acl/acl_test.go +++ b/acl/acl_test.go @@ -288,7 +288,12 @@ func TestWildcardNamespaceMatching(t *testing.T) { }, { // The closest character match wins Policy: `namespace "*-api-services" { policy = "deny" } - namespace "prod-api-*" { policy = "write" }`, // 5 vs 8 chars + namespace "prod-api-*" { policy = "write" }`, // 4 vs 8 chars + Allow: false, + }, + { + Policy: `namespace "prod-api-*" { policy = "write" } + namespace "*-api-services" { policy = "deny" }`, // 4 vs 8 chars Allow: false, }, } @@ -371,6 +376,21 @@ func TestACL_matchingCapabilitySet_difference(t *testing.T) { NS: "production-admin-api", Difference: 9, }, + { + Policy: `namespace "production-**" { policy = "write" }`, + NS: "production-admin-api", + Difference: 9, + }, + { + Policy: `namespace "*" { policy = "write" }`, + NS: "production-admin-api", + Difference: 20, + }, + { + Policy: `namespace "*admin*" { policy = "write" }`, + NS: "production-admin-api", + Difference: 15, + }, } for _, tc := range tests { From f10dbbec54ec42e236f92953db5c5ace25d1339f Mon Sep 17 00:00:00 2001 From: Danielle Tomlinson Date: Wed, 12 Dec 2018 13:02:44 +0100 Subject: [PATCH 4/4] guides: Update for globbed namespace rules --- .../source/guides/security/acl.html.markdown | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/website/source/guides/security/acl.html.markdown b/website/source/guides/security/acl.html.markdown index 15ed4b11f6aa..205a42f67816 100644 --- a/website/source/guides/security/acl.html.markdown +++ b/website/source/guides/security/acl.html.markdown @@ -253,6 +253,36 @@ namespace "default" { } ``` +Namespaces definitions may also include globs, allowing a single policy definition to apply to a set of namespaces. For example, the below policy allows read access to most production namespaces, but allows write access to the "production-api" namespace, and rejects any access to the "production-web" namespace. + +``` +namespace "production-*" { + policy = "read" +} + +namespace "production-api" { + policy = "write" +} + +namespace "production-web" { + policy = "deny" +} +``` + +Namespaces are matched to their policies first by performing a lookup on any _exact match_, before falling back to performing a glob based lookup. When looking up namespaces by glob, the matching policy with the greatest number of matched characters will be chosen. For example: + +``` +namespace "*-web" { + policy = "deny" +} + +namespace "*" { + policy = "write" +} +``` + +Will evaluate to deny for `production-web`, because it is 9 characters different from the `"*-web"` rule, but 13 characters different from the `"*"` rule. + ### Node Rules The `node` policy controls access to the [Node API](/api/nodes.html) such as listing nodes or triggering a node drain.