diff --git a/acl/acl.go b/acl/acl.go index 3dfab6340e..cd3b6df053 100644 --- 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 index 14c1904d0d..800c3744d9 100644 --- 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 {