Merge pull request #4438 from hashicorp/bugfix/keyring-acl

ACL Bug Fixes

acl/acl.go

@@ -328,6 +328,34 @@ type PolicyACL struct {
 	operatorRule string
 }
 
+func enforce(rule string, requiredPermission string) (allow, recurse bool) {
+	switch rule {
+	case PolicyWrite:
+		// grants read, list and write permissions
+		return true, false
+	case PolicyList:
+		// grants read and list permissions
+		if requiredPermission == PolicyList || requiredPermission == PolicyRead {
+			return true, false
+		} else {
+			return false, false
+		}
+	case PolicyRead:
+		// grants just read permissions
+		if requiredPermission == PolicyRead {
+			return true, false
+		} else {
+			return false, false
+		}
+	case PolicyDeny:
+		// explicit denial - do not recurse
+		return false, false
+	default:
+		// need to recurse as there was no specific policy set
+		return false, true
+	}
+}
+
 // New is used to construct a policy based ACL from a set of policies
 // and a parent policy to resolve missing cases.
 func New(parent ACL, policy *Policy, sentinel sentinel.Evaluator) (*PolicyACL, error) {
@@ -433,14 +461,9 @@ func (p *PolicyACL) ACLModify() bool {
 // node.
 func (p *PolicyACL) AgentRead(node string) bool {
 	// Check for an exact rule or catch-all
-	_, rule, ok := p.agentRules.LongestPrefix(node)
+	if _, rule, ok := p.agentRules.LongestPrefix(node); ok {
-
+		if allow, recurse := enforce(rule.(string), PolicyRead); !recurse {
-	if ok {
+			return allow
-		switch rule {
-		case PolicyRead, PolicyWrite:
-			return true
-		default:
-			return false
 		}
 	}
 
@@ -455,11 +478,8 @@ func (p *PolicyACL) AgentWrite(node string) bool {
 	_, rule, ok := p.agentRules.LongestPrefix(node)
 
 	if ok {
-		switch rule {
+		if allow, recurse := enforce(rule.(string), PolicyWrite); !recurse {
-		case PolicyWrite:
+			return allow
-			return true
-		default:
-			return false
 		}
 	}
 
@@ -477,15 +497,12 @@ func (p *PolicyACL) Snapshot() bool {
 func (p *PolicyACL) EventRead(name string) bool {
 	// Longest-prefix match on event names
 	if _, rule, ok := p.eventRules.LongestPrefix(name); ok {
-		switch rule {
+		if allow, recurse := enforce(rule.(string), PolicyRead); !recurse {
-		case PolicyRead, PolicyWrite:
+			return allow
-			return true
-		default:
-			return false
 		}
 	}
 
-	// Nothing matched, use parent
+	// No matching rule, use the parent.
 	return p.parent.EventRead(name)
 }
 
@@ -494,7 +511,9 @@ func (p *PolicyACL) EventRead(name string) bool {
 func (p *PolicyACL) EventWrite(name string) bool {
 	// Longest-prefix match event names
 	if _, rule, ok := p.eventRules.LongestPrefix(name); ok {
-		return rule == PolicyWrite
+		if allow, recurse := enforce(rule.(string), PolicyWrite); !recurse {
+			return allow
+		}
 	}
 
 	// No match, use parent
@@ -512,14 +531,10 @@ func (p *PolicyACL) IntentionDefaultAllow() bool {
 // intention is allowed.
 func (p *PolicyACL) IntentionRead(prefix string) bool {
 	// Check for an exact rule or catch-all
-	_, rule, ok := p.intentionRules.LongestPrefix(prefix)
+	if _, rule, ok := p.intentionRules.LongestPrefix(prefix); ok {
-	if ok {
 		pr := rule.(PolicyRule)
-		switch pr.aclPolicy {
+		if allow, recurse := enforce(pr.aclPolicy, PolicyRead); !recurse {
-		case PolicyRead, PolicyWrite:
+			return allow
-			return true
-		default:
-			return false
 		}
 	}
 
@@ -531,14 +546,10 @@ func (p *PolicyACL) IntentionRead(prefix string) bool {
 // intention is allowed.
 func (p *PolicyACL) IntentionWrite(prefix string) bool {
 	// Check for an exact rule or catch-all
-	_, rule, ok := p.intentionRules.LongestPrefix(prefix)
+	if _, rule, ok := p.intentionRules.LongestPrefix(prefix); ok {
-	if ok {
 		pr := rule.(PolicyRule)
-		switch pr.aclPolicy {
+		if allow, recurse := enforce(pr.aclPolicy, PolicyWrite); !recurse {
-		case PolicyWrite:
+			return allow
-			return true
-		default:
-			return false
 		}
 	}
 
@@ -549,14 +560,10 @@ func (p *PolicyACL) IntentionWrite(prefix string) bool {
 // KeyRead returns if a key is allowed to be read
 func (p *PolicyACL) KeyRead(key string) bool {
 	// Look for a matching rule
-	_, rule, ok := p.keyRules.LongestPrefix(key)
+	if _, rule, ok := p.keyRules.LongestPrefix(key); ok {
-	if ok {
 		pr := rule.(PolicyRule)
-		switch pr.aclPolicy {
+		if allow, recurse := enforce(pr.aclPolicy, PolicyRead); !recurse {
-		case PolicyRead, PolicyWrite, PolicyList:
+			return allow
-			return true
-		default:
-			return false
 		}
 	}
 
@@ -567,14 +574,10 @@ func (p *PolicyACL) KeyRead(key string) bool {
 // KeyList returns if a key is allowed to be listed
 func (p *PolicyACL) KeyList(key string) bool {
 	// Look for a matching rule
-	_, rule, ok := p.keyRules.LongestPrefix(key)
+	if _, rule, ok := p.keyRules.LongestPrefix(key); ok {
-	if ok {
 		pr := rule.(PolicyRule)
-		switch pr.aclPolicy {
+		if allow, recurse := enforce(pr.aclPolicy, PolicyList); !recurse {
-		case PolicyList, PolicyWrite:
+			return allow
-			return true
-		default:
-			return false
 		}
 	}
 
@@ -585,13 +588,12 @@ func (p *PolicyACL) KeyList(key string) bool {
 // KeyWrite returns if a key is allowed to be written
 func (p *PolicyACL) KeyWrite(key string, scope sentinel.ScopeFn) bool {
 	// Look for a matching rule
-	_, rule, ok := p.keyRules.LongestPrefix(key)
+	if _, rule, ok := p.keyRules.LongestPrefix(key); ok {
-	if ok {
 		pr := rule.(PolicyRule)
-		switch pr.aclPolicy {
+		if allow, recurse := enforce(pr.aclPolicy, PolicyWrite); !recurse {
-		case PolicyWrite:
+			if allow {
-			return p.executeCodePolicy(&pr.sentinelPolicy, scope)
+				return p.executeCodePolicy(&pr.sentinelPolicy, scope)
-		default:
+			}
 			return false
 		}
 	}
@@ -636,48 +638,48 @@ func (p *PolicyACL) KeyWritePrefix(prefix string) bool {
 // KeyringRead is used to determine if the keyring can be
 // read by the current ACL token.
 func (p *PolicyACL) KeyringRead() bool {
-	switch p.keyringRule {
+	if allow, recurse := enforce(p.keyringRule, PolicyRead); !recurse {
-	case PolicyRead, PolicyWrite:
+		return allow
-		return true
-	case PolicyDeny:
-		return false
-	default:
-		return p.parent.KeyringRead()
 	}
+
+	return p.parent.KeyringRead()
 }
 
 // KeyringWrite determines if the keyring can be manipulated.
 func (p *PolicyACL) KeyringWrite() bool {
-	if p.keyringRule == PolicyWrite {
+	if allow, recurse := enforce(p.keyringRule, PolicyWrite); !recurse {
-		return true
+		return allow
 	}
+
 	return p.parent.KeyringWrite()
 }
 
 // OperatorRead determines if the read-only operator functions are allowed.
 func (p *PolicyACL) OperatorRead() bool {
-	switch p.operatorRule {
+	if allow, recurse := enforce(p.operatorRule, PolicyRead); !recurse {
-	case PolicyRead, PolicyWrite:
+		return allow
-		return true
-	case PolicyDeny:
-		return false
-	default:
-		return p.parent.OperatorRead()
 	}
+
+	return p.parent.OperatorRead()
+}
+
+// OperatorWrite determines if the state-changing operator functions are
+// allowed.
+func (p *PolicyACL) OperatorWrite() bool {
+	if allow, recurse := enforce(p.operatorRule, PolicyWrite); !recurse {
+		return allow
+	}
+
+	return p.parent.OperatorWrite()
 }
 
 // NodeRead checks if reading (discovery) of a node is allowed
 func (p *PolicyACL) NodeRead(name string) bool {
 	// Check for an exact rule or catch-all
-	_, rule, ok := p.nodeRules.LongestPrefix(name)
+	if _, rule, ok := p.nodeRules.LongestPrefix(name); ok {
-
-	if ok {
 		pr := rule.(PolicyRule)
-		switch pr.aclPolicy {
+		if allow, recurse := enforce(pr.aclPolicy, PolicyRead); !recurse {
-		case PolicyRead, PolicyWrite:
+			return allow
-			return true
-		default:
-			return false
 		}
 	}
 
@@ -688,15 +690,10 @@ func (p *PolicyACL) NodeRead(name string) bool {
 // NodeWrite checks if writing (registering) a node is allowed
 func (p *PolicyACL) NodeWrite(name string, scope sentinel.ScopeFn) bool {
 	// Check for an exact rule or catch-all
-	_, rule, ok := p.nodeRules.LongestPrefix(name)
+	if _, rule, ok := p.nodeRules.LongestPrefix(name); ok {
-
-	if ok {
 		pr := rule.(PolicyRule)
-		switch pr.aclPolicy {
+		if allow, recurse := enforce(pr.aclPolicy, PolicyWrite); !recurse {
-		case PolicyWrite:
+			return allow
-			return true
-		default:
-			return false
 		}
 	}
 
@@ -704,27 +701,13 @@ func (p *PolicyACL) NodeWrite(name string, scope sentinel.ScopeFn) bool {
 	return p.parent.NodeWrite(name, scope)
 }
 
-// OperatorWrite determines if the state-changing operator functions are
-// allowed.
-func (p *PolicyACL) OperatorWrite() bool {
-	if p.operatorRule == PolicyWrite {
-		return true
-	}
-	return p.parent.OperatorWrite()
-}
-
 // PreparedQueryRead checks if reading (listing) of a prepared query is
 // allowed - this isn't execution, just listing its contents.
 func (p *PolicyACL) PreparedQueryRead(prefix string) bool {
 	// Check for an exact rule or catch-all
-	_, rule, ok := p.preparedQueryRules.LongestPrefix(prefix)
+	if _, rule, ok := p.preparedQueryRules.LongestPrefix(prefix); ok {
-
+		if allow, recurse := enforce(rule.(string), PolicyRead); !recurse {
-	if ok {
+			return allow
-		switch rule {
-		case PolicyRead, PolicyWrite:
-			return true
-		default:
-			return false
 		}
 	}
 
@@ -736,14 +719,9 @@ func (p *PolicyACL) PreparedQueryRead(prefix string) bool {
 // prepared query is allowed.
 func (p *PolicyACL) PreparedQueryWrite(prefix string) bool {
 	// Check for an exact rule or catch-all
-	_, rule, ok := p.preparedQueryRules.LongestPrefix(prefix)
+	if _, rule, ok := p.preparedQueryRules.LongestPrefix(prefix); ok {
-
+		if allow, recurse := enforce(rule.(string), PolicyWrite); !recurse {
-	if ok {
+			return allow
-		switch rule {
-		case PolicyWrite:
-			return true
-		default:
-			return false
 		}
 	}
 
@@ -754,14 +732,10 @@ func (p *PolicyACL) PreparedQueryWrite(prefix string) bool {
 // ServiceRead checks if reading (discovery) of a service is allowed
 func (p *PolicyACL) ServiceRead(name string) bool {
 	// Check for an exact rule or catch-all
-	_, rule, ok := p.serviceRules.LongestPrefix(name)
+	if _, rule, ok := p.serviceRules.LongestPrefix(name); ok {
-	if ok {
 		pr := rule.(PolicyRule)
-		switch pr.aclPolicy {
+		if allow, recurse := enforce(pr.aclPolicy, PolicyRead); !recurse {
-		case PolicyRead, PolicyWrite:
+			return allow
-			return true
-		default:
-			return false
 		}
 	}
 
@@ -772,14 +746,10 @@ func (p *PolicyACL) ServiceRead(name string) bool {
 // ServiceWrite checks if writing (registering) a service is allowed
 func (p *PolicyACL) ServiceWrite(name string, scope sentinel.ScopeFn) bool {
 	// Check for an exact rule or catch-all
-	_, rule, ok := p.serviceRules.LongestPrefix(name)
+	if _, rule, ok := p.serviceRules.LongestPrefix(name); ok {
-	if ok {
 		pr := rule.(PolicyRule)
-		switch pr.aclPolicy {
+		if allow, recurse := enforce(pr.aclPolicy, PolicyWrite); !recurse {
-		case PolicyWrite:
+			return allow
-			return true
-		default:
-			return false
 		}
 	}
 
@@ -790,14 +760,9 @@ func (p *PolicyACL) ServiceWrite(name string, scope sentinel.ScopeFn) bool {
 // SessionRead checks for permission to read sessions for a given node.
 func (p *PolicyACL) SessionRead(node string) bool {
 	// Check for an exact rule or catch-all
-	_, rule, ok := p.sessionRules.LongestPrefix(node)
+	if _, rule, ok := p.sessionRules.LongestPrefix(node); ok {
-
+		if allow, recurse := enforce(rule.(string), PolicyRead); !recurse {
-	if ok {
+			return allow
-		switch rule {
-		case PolicyRead, PolicyWrite:
-			return true
-		default:
-			return false
 		}
 	}
 
@@ -808,14 +773,9 @@ func (p *PolicyACL) SessionRead(node string) bool {
 // SessionWrite checks for permission to create sessions for a given node.
 func (p *PolicyACL) SessionWrite(node string) bool {
 	// Check for an exact rule or catch-all
-	_, rule, ok := p.sessionRules.LongestPrefix(node)
+	if _, rule, ok := p.sessionRules.LongestPrefix(node); ok {
-
+		if allow, recurse := enforce(rule.(string), PolicyWrite); !recurse {
-	if ok {
+			return allow
-		switch rule {
-		case PolicyWrite:
-			return true
-		default:
-			return false
 		}
 	}