Fix to prevent allowing recursive KV deletions when we shouldn’t

hashicorp · May 22, 2019 · 36ebca1 · 36ebca1
1 parent 5457bca
commit 36ebca1
Show file tree

Hide file tree

Showing 2 changed files with 91 additions and 5 deletions.
diff --git a/acl/acl.go b/acl/acl.go
@@ -734,8 +734,10 @@ func (p *PolicyAuthorizer) KeyWrite(key string, scope sentinel.ScopeFn) bool {
 // delete everything under the prefix. First we must have "write"
 // on the prefix itself
 func (p *PolicyAuthorizer) KeyWritePrefix(prefix string) bool {
+	parentAllows := p.parent.KeyWritePrefix(prefix)
+
 	// Look for a matching rule that denies
-	prefixAllowed := true
+	prefixAllowed := parentAllows
 	found := false
 
 	// Look for a prefix rule that would apply to the prefix we are checking
@@ -755,6 +757,8 @@ func (p *PolicyAuthorizer) KeyWritePrefix(prefix string) bool {
 		return false
 	})
 
+	// This will be false if we had a prefix that didn't allow write or if
+	// there was no prefix rule and the parent policy would deny access.
 	if !prefixAllowed {
 		return false
 	}
@@ -763,7 +767,6 @@ func (p *PolicyAuthorizer) KeyWritePrefix(prefix string) bool {
 	// into account both prefix and exact match rules.
 	deny := false
 	p.keyRules.WalkPrefix(prefix, func(path string, leaf interface{}) bool {
-		found = true
 		rule := leaf.(*policyAuthorizerRadixLeaf)
 
 		if rule.prefix != nil && rule.prefix.(RulePolicy).aclPolicy != PolicyWrite {
@@ -783,13 +786,13 @@ func (p *PolicyAuthorizer) KeyWritePrefix(prefix string) bool {
 		return false
 	}
 
-	// If we had a matching rule, done
+	// If we had a matching prefix rule and it allowed writes, then we can allow the access
 	if found {
 		return true
 	}
 
-	// No matching rule, use the parent.
-	return p.parent.KeyWritePrefix(prefix)
+	// No matching rule, use the parent policy.
+	return parentAllows
 }
 
 // KeyringRead is used to determine if the keyring can be

diff --git a/acl/acl_test.go b/acl/acl_test.go
@@ -1704,6 +1704,89 @@ func TestACL(t *testing.T) {
 				{name: "WriteAllowed", check: checkAllowACLWrite},
 			},
 		},
+		{
+			name:          "KeyWritePrefixDefaultDeny",
+			defaultPolicy: DenyAll(),
+			policyStack: []*Policy{
+				&Policy{
+					KeyPrefixes: []*KeyPolicy{
+						&KeyPolicy{
+							Prefix: "fo",
+							Policy: PolicyRead,
+						},
+						&KeyPolicy{
+							Prefix: "foo/",
+							Policy: PolicyWrite,
+						},
+						&KeyPolicy{
+							Prefix: "bar/",
+							Policy: PolicyWrite,
+						},
+						&KeyPolicy{
+							Prefix: "baz/",
+							Policy: PolicyWrite,
+						},
+						&KeyPolicy{
+							Prefix: "test/",
+							Policy: PolicyWrite,
+						},
+					},
+					Keys: []*KeyPolicy{
+						&KeyPolicy{
+							Prefix: "foo/bar",
+							Policy: PolicyWrite,
+						},
+						&KeyPolicy{
+							Prefix: "bar/baz",
+							Policy: PolicyRead,
+						},
+					},
+				},
+			},
+			checks: []aclCheck{
+				// Ensure we deny access if the best match key_prefix rule does not grant
+				// write access (disregards both the default policy and any other rules with
+				// segments that may fall within the given kv prefix)
+				{name: "DeniedTopLevelPrefix", prefix: "foo", check: checkDenyKeyWritePrefix},
+				// Allow recursive KV writes when we have a prefix rule that allows it and no
+				// other rules with segments that fall within the requested kv prefix to be written.
+				{name: "AllowedTopLevelPrefix", prefix: "baz/", check: checkAllowKeyWritePrefix},
+				// Ensure we allow recursive KV writes when we have a prefix rule that would allow it
+				// and all other rules with segments prefixed by the kv prefix to be written also allow
+				// write access.
+				{name: "AllowedPrefixWithNestedWrite", prefix: "foo/", check: checkAllowKeyWritePrefix},
+				// Ensure that we deny recursive KV writes when they would be allowed for a prefix but
+				// denied by either an exact match rule or prefix match rule for a segment prefixed by
+				// the kv prefix being checked against.
+				{name: "DenyPrefixWithNestedRead", prefix: "bar/", check: checkDenyKeyWritePrefix},
+				// Ensure that the default deny policy is used when there is no key_prefix rule
+				// for the given kv segment regardless of any rules that would grant write access
+				// to segments prefixed by the kv prefix being checked against.
+				{name: "DenyNoPrefixMatch", prefix: "te", check: checkDenyKeyWritePrefix},
+			},
+		},
+		{
+			name:          "KeyWritePrefixDefaultAllow",
+			defaultPolicy: AllowAll(),
+			policyStack: []*Policy{
+				&Policy{
+					Keys: []*KeyPolicy{
+						&KeyPolicy{
+							Prefix: "foo/bar",
+							Policy: PolicyRead,
+						},
+					},
+				},
+			},
+			checks: []aclCheck{
+				// Ensure that we deny a key prefix write when a rule for a key within our prefix
+				// doesn't allow writing and the default policy is to allow
+				{name: "KeyWritePrefixDenied", prefix: "foo", check: checkDenyKeyWritePrefix},
+				// Ensure that the default allow policy is used when there is no prefix rule
+				// and there are no other rules regarding keys within that prefix to be written.
+				{name: "KeyWritePrefixAllowed", prefix: "bar", check: checkAllowKeyWritePrefix},
+			},
+		},
 	}
 
 	for _, tcase := range tests {