Update intention ACL enforcement

This also changes how intention:read is granted. Before the Intention.List RPC would allow viewing an intention if the token had intention:read on the destination. However Intention.Match allowed viewing if access was allowed for either the source or dest side. Now Intention.List and Intention.Get fall in line with Intention.Matches previous behavior.

Due to this being done a few different places ACL enforcement for a singular intention is now done with the CanRead and CanWrite methods on the intention itself.

acl/authorizer.go

@@ -242,3 +242,12 @@ func Enforce(authz Authorizer, rsc Resource, segment string, access string, ctx
 
 	return Deny, fmt.Errorf("Invalid access level for %s resource: %s", rsc, access)
 }
+
+func NewAuthorizerFromRules(id string, revision uint64, rules string, syntax SyntaxVersion, conf *Config, meta *EnterprisePolicyMeta) (Authorizer, error) {
+	policy, err := NewPolicyFromSource(id, revision, rules, syntax, conf, meta)
+	if err != nil {
+		return nil, err
+	}
+
+	return NewPolicyAuthorizer([]*Policy{policy}, conf)
+}

agent/consul/acl.go

@@ -1344,20 +1344,9 @@ func (f *aclFilter) filterCoordinates(coords *structs.Coordinates) {
 // We prune entries the user doesn't have access to, and we redact any tokens
 // if the user doesn't have a management token.
 func (f *aclFilter) filterIntentions(ixns *structs.Intentions) {
-	var authzContext acl.AuthorizerContext
-	// Otherwise, we need to see what the token has access to.
 	ret := make(structs.Intentions, 0, len(*ixns))
 	for _, ixn := range *ixns {
-		// check for acl:read in either the destination or source
-		ixn.FillAuthzContext(&authzContext)
-		aclRead := f.authorizer.ACLRead(&authzContext) == acl.Allow
-
-		// If no prefix ACL applies to this then filter it, since
-		// we know at this point the user doesn't have a management
-		// token, otherwise see what the policy says.
-		prefix, ok := ixn.GetACLPrefix()
-
-		if !aclRead && (!ok || f.authorizer.IntentionRead(prefix, &authzContext) != acl.Allow) {
+		if !ixn.CanRead(f.authorizer) {
 			f.logger.Printf("[DEBUG] consul: dropping intention %q from result due to ACLs", ixn.ID)
 			continue
 		}

agent/consul/intention_endpoint.go

@@ -10,8 +10,8 @@ import (
 	"github.com/hashicorp/consul/agent/connect"
 	"github.com/hashicorp/consul/agent/consul/state"
 	"github.com/hashicorp/consul/agent/structs"
+	"github.com/hashicorp/consul/lib"
 	"github.com/hashicorp/go-memdb"
-	"github.com/hashicorp/go-uuid"
 )
 
 var (
@@ -25,6 +25,17 @@ type Intention struct {
 	srv *Server
 }
 
+func (s *Intention) checkIntentionID(id string) (bool, error) {
+	state := s.srv.fsm.State()
+	if _, ixn, err := state.IntentionGet(nil, id); err != nil {
+		return false, err
+	} else if ixn != nil {
+		return false, nil
+	}
+
+	return true, nil
+}
+
 // Apply creates or updates an intention in the data store.
 func (s *Intention) Apply(
 	args *structs.IntentionRequest,
@@ -46,6 +57,31 @@ func (s *Intention) Apply(
 		args.Intention = &structs.Intention{}
 	}
 
+	// Get the ACL token for the request for the checks below.
+	var entMeta structs.EnterpriseMeta
+	authz, err := s.srv.ResolveTokenAndDefaultMeta(args.Token, &entMeta, nil)
+	if err != nil {
+		return err
+	}
+
+	switch args.Op {
+	case structs.IntentionOpCreate, structs.IntentionOpUpdate:
+		// in this case we should validate that the token has
+		// permissions to write the incoming intention. Later we
+		// must also check that for updates, the token also has
+		// write permissions on that token.
+		if !args.Intention.CanWrite(authz) {
+			if args.Op == structs.IntentionOpCreate {
+				s.srv.logger.Printf("[WARN] consul.intention: Intention creation denied due to ACLs")
+			} else {
+				s.srv.logger.Printf("[WARN] consul.intention: Operation on intention %q denied due to ACLs", args.Intention.ID)
+			}
+			return acl.ErrPermissionDenied
+		}
+	default:
+		// other ops permissions will be checked later
+	}
+
 	// If no ID is provided, generate a new ID. This must be done prior to
 	// appending to the Raft log, because the ID is not deterministic. Once
 	// the entry is in the log, the state update MUST be deterministic or
@@ -55,44 +91,15 @@ func (s *Intention) Apply(
 			return fmt.Errorf("ID must be empty when creating a new intention")
 		}
 
-		state := s.srv.fsm.State()
-		for {
-			var err error
-			args.Intention.ID, err = uuid.GenerateUUID()
-			if err != nil {
-				s.srv.logger.Printf("[ERR] consul.intention: UUID generation failed: %v", err)
-				return err
-			}
-
-			_, ixn, err := state.IntentionGet(nil, args.Intention.ID)
-			if err != nil {
-				s.srv.logger.Printf("[ERR] consul.intention: intention lookup failed: %v", err)
-				return err
-			}
-			if ixn == nil {
-				break
-			}
+		args.Intention.ID, err = lib.GenerateUUID(s.checkIntentionID)
+		if err != nil {
+			return err
 		}
-
 		// Set the created at
 		args.Intention.CreatedAt = time.Now().UTC()
 	}
 	*reply = args.Intention.ID
 
-	// Get the ACL token for the request for the checks below.
-	rule, err := s.srv.ResolveToken(args.Token)
-	if err != nil {
-		return err
-	}
-
-	// Perform the ACL check
-	if prefix, ok := args.Intention.GetACLPrefix(); ok {
-		if rule != nil && rule.IntentionWrite(prefix, nil) != acl.Allow {
-			s.srv.logger.Printf("[WARN] consul.intention: Operation on intention '%s' denied due to ACLs", args.Intention.ID)
-			return acl.ErrPermissionDenied
-		}
-	}
-
 	// If this is not a create, then we have to verify the ID.
 	if args.Op != structs.IntentionOpCreate {
 		state := s.srv.fsm.State()
@@ -104,13 +111,12 @@ func (s *Intention) Apply(
 			return fmt.Errorf("Cannot modify non-existent intention: '%s'", args.Intention.ID)
 		}
 
-		// Perform the ACL check that we have write to the old prefix too,
-		// which must be true to perform any rename.
-		if prefix, ok := ixn.GetACLPrefix(); ok {
-			if rule != nil && rule.IntentionWrite(prefix, nil) != acl.Allow {
-				s.srv.logger.Printf("[WARN] consul.intention: Operation on intention '%s' denied due to ACLs", args.Intention.ID)
-				return acl.ErrPermissionDenied
-			}
+		// Perform the ACL check that we have write to the old intention too,
+		// which must be true to perform any rename. This is the only ACL enforcement
+		// done for deletions and a secondary enforcement for updates.
+		if !ixn.CanWrite(authz) {
+			s.srv.logger.Printf("[WARN] consul.intention: Operation on intention %q denied due to ACLs", args.Intention.ID)
+			return acl.ErrPermissionDenied
 		}
 	}
 
@@ -122,13 +128,7 @@ func (s *Intention) Apply(
 		args.Intention.SourceType = structs.IntentionSourceConsul
 	}
 
-	// Until we support namespaces, we force all namespaces to be default
-	if args.Intention.SourceNS == "" {
-		args.Intention.SourceNS = structs.IntentionDefaultNamespace
-	}
-	if args.Intention.DestinationNS == "" {
-		args.Intention.DestinationNS = structs.IntentionDefaultNamespace
-	}
+	args.Intention.DefaultNamespaces(&entMeta)
 
 	// Validate. We do not validate on delete since it is valid to only
 	// send an ID in that case.
@@ -240,10 +240,18 @@ func (s *Intention) Match(
 	}
 
 	if rule != nil {
-		// We go through each entry and test the destination to check if it
-		// matches.
+		var authzContext acl.AuthorizerContext
+		// Go through each entry to ensure we have intention:read for the resource.
+
+		// TODO - should we do this instead of filtering the result set? This will only allow
+		// queries for which the token has intention:read permissions on the requested side
+		// of the service. Should it instead return all matches that it would be able to list.
+		// if so we should remove this and call filterACL instead. Based on how this is used
+		// its probably fine. If you have intention read on the source just do a source type
+		// matching, if you have it on the dest then perform a dest type match.
 		for _, entry := range args.Match.Entries {
-			if prefix := entry.Name; prefix != "" && rule.IntentionRead(prefix, nil) != acl.Allow {
+			entry.FillAuthzContext(&authzContext)
+			if prefix := entry.Name; prefix != "" && rule.IntentionRead(prefix, &authzContext) != acl.Allow {
 				s.srv.logger.Printf("[WARN] consul.intention: Operation on intention prefix '%s' denied due to ACLs", prefix)
 				return acl.ErrPermissionDenied
 			}
@@ -307,9 +315,14 @@ func (s *Intention) Check(
 
 	// Perform the ACL check. For Check we only require ServiceRead and
 	// NOT IntentionRead because the Check API only returns pass/fail and
-	// returns no other information about the intentions used.
+	// returns no other information about the intentions used. We could check
+	// both the source and dest side but only checking dest also has the nice
+	// benefit of only returning a passing status if the token would be able
+	// to discover the dest service and connect to it.
 	if prefix, ok := query.GetACLPrefix(); ok {
-		if rule != nil && rule.ServiceRead(prefix, nil) != acl.Allow {
+		var authzContext acl.AuthorizerContext
+		query.FillAuthzContext(&authzContext)
+		if rule != nil && rule.ServiceRead(prefix, &authzContext) != acl.Allow {
 			s.srv.logger.Printf("[WARN] consul.intention: test on intention '%s' denied due to ACLs", prefix)
 			return acl.ErrPermissionDenied
 		}