From 4976c000b73f4d67ffd78fc648cb132587ed20ba Mon Sep 17 00:00:00 2001 From: freddygv Date: Fri, 12 Mar 2021 21:59:47 -0700 Subject: [PATCH] Refactor IntentionDecision This enables it to be called for many upstreams or downstreams of a service while only querying intentions once. Additionally, decisions are now optionally denied due to L7 permissions being present. This enables the function to be used to filter for potential upstreams/downstreams of a service. --- agent/consul/intention_endpoint.go | 26 ++- agent/consul/state/catalog.go | 33 ++-- agent/consul/state/intention.go | 66 ++++--- agent/consul/state/intention_test.go | 252 ++++++++++++++++++++++++--- 4 files changed, 300 insertions(+), 77 deletions(-) diff --git a/agent/consul/intention_endpoint.go b/agent/consul/intention_endpoint.go index b35167a18..58bb1a77d 100644 --- a/agent/consul/intention_endpoint.go +++ b/agent/consul/intention_endpoint.go @@ -8,7 +8,6 @@ import ( "github.com/armon/go-metrics" "github.com/armon/go-metrics/prometheus" "github.com/hashicorp/consul/acl" - "github.com/hashicorp/consul/agent/connect" "github.com/hashicorp/consul/agent/consul/state" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/lib" @@ -684,19 +683,6 @@ func (s *Intention) Check(args *structs.IntentionQueryRequest, reply *structs.In return fmt.Errorf("Invalid destination namespace %q: %v", query.DestinationNS, err) } - // Build the URI - var uri connect.CertURI - switch query.SourceType { - case structs.IntentionSourceConsul: - uri = &connect.SpiffeIDService{ - Namespace: query.SourceNS, - Service: query.SourceName, - } - - default: - return fmt.Errorf("unsupported SourceType: %q", query.SourceType) - } - // 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. We could check @@ -732,7 +718,17 @@ func (s *Intention) Check(args *structs.IntentionQueryRequest, reply *structs.In } state := s.srv.fsm.State() - decision, err := state.IntentionDecision(uri, query.DestinationName, query.DestinationNS, defaultDecision) + + entry := structs.IntentionMatchEntry{ + Namespace: query.SourceNS, + Name: query.SourceName, + } + _, intentions, err := state.IntentionMatchOne(nil, entry, structs.IntentionMatchSource) + if err != nil { + return fmt.Errorf("failed to query intentions for %s/%s", query.SourceNS, query.SourceName) + } + + decision, err := state.IntentionDecision(query.DestinationName, query.DestinationNS, intentions, structs.IntentionMatchDestination, defaultDecision, false) if err != nil { return fmt.Errorf("failed to get intention decision from (%s/%s) to (%s/%s): %v", query.SourceNS, query.SourceName, query.DestinationNS, query.DestinationName, err) diff --git a/agent/consul/state/catalog.go b/agent/consul/state/catalog.go index e525196c9..d91298def 100644 --- a/agent/consul/state/catalog.go +++ b/agent/consul/state/catalog.go @@ -11,7 +11,6 @@ import ( "github.com/mitchellh/copystructure" "github.com/hashicorp/consul/acl" - "github.com/hashicorp/consul/agent/connect" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/lib" @@ -2859,16 +2858,20 @@ func (s *Store) ServiceTopology( upstreamDecisions := make(map[string]structs.IntentionDecisionSummary) - // The given service is the source relative to upstreams - sourceURI := connect.SpiffeIDService{ + matchEntry := structs.IntentionMatchEntry{ Namespace: entMeta.NamespaceOrDefault(), - Service: service, + Name: service, + } + // The given service is a source relative to its upstreams + _, intentions, err := s.IntentionMatchOne(ws, matchEntry, structs.IntentionMatchSource) + if err != nil { + return 0, nil, fmt.Errorf("failed to query intentions for %s", sn.String()) } for _, un := range upstreamNames { - decision, err := s.IntentionDecision(&sourceURI, un.Name, un.NamespaceOrDefault(), defaultAllow) + decision, err := s.IntentionDecision(un.Name, un.NamespaceOrDefault(), intentions, structs.IntentionMatchDestination, defaultAllow, false) if err != nil { - return 0, nil, fmt.Errorf("failed to get intention decision from (%s/%s) to (%s/%s): %v", - sourceURI.Namespace, sourceURI.Service, un.Name, un.NamespaceOrDefault(), err) + return 0, nil, fmt.Errorf("failed to get intention decision from (%s) to (%s): %v", + sn.String(), un.String(), err) } upstreamDecisions[un.String()] = decision } @@ -2888,17 +2891,17 @@ func (s *Store) ServiceTopology( maxIdx = idx } + // The given service is a destination relative to its downstreams + _, intentions, err = s.IntentionMatchOne(ws, matchEntry, structs.IntentionMatchDestination) + if err != nil { + return 0, nil, fmt.Errorf("failed to query intentions for %s", sn.String()) + } downstreamDecisions := make(map[string]structs.IntentionDecisionSummary) for _, dn := range downstreamNames { - // Downstreams are the source relative to the given service - sourceURI := connect.SpiffeIDService{ - Namespace: dn.NamespaceOrDefault(), - Service: dn.Name, - } - decision, err := s.IntentionDecision(&sourceURI, service, entMeta.NamespaceOrDefault(), defaultAllow) + decision, err := s.IntentionDecision(dn.Name, dn.NamespaceOrDefault(), intentions, structs.IntentionMatchSource, defaultAllow, false) if err != nil { - return 0, nil, fmt.Errorf("failed to get intention decision from (%s/%s) to (%s/%s): %v", - sourceURI.Namespace, sourceURI.Service, service, dn.NamespaceOrDefault(), err) + return 0, nil, fmt.Errorf("failed to get intention decision from (%s) to (%s): %v", + dn.String(), sn.String(), err) } downstreamDecisions[dn.String()] = decision } diff --git a/agent/consul/state/intention.go b/agent/consul/state/intention.go index 6a62c618a..508870eb8 100644 --- a/agent/consul/state/intention.go +++ b/agent/consul/state/intention.go @@ -8,7 +8,6 @@ import ( "github.com/hashicorp/go-memdb" "github.com/hashicorp/consul/acl" - "github.com/hashicorp/consul/agent/connect" "github.com/hashicorp/consul/agent/structs" ) @@ -732,35 +731,19 @@ func (s *Store) LegacyIntentionDeleteAll(idx uint64) error { return tx.Commit() } -// IntentionDecision returns whether a connection should be allowed from a source URI to some destination -// It returns true or false for the enforcement, and also a boolean for whether +// IntentionDecision returns whether a connection should be allowed to a source or destination given a set of intentions. +// +// allowPermissions determines whether the presence of L7 permissions leads to a DENY decision. +// This should be false when evaluating a connection between a source and destination, but not the request that will be sent. func (s *Store) IntentionDecision( - srcURI connect.CertURI, dstName, dstNS string, defaultDecision acl.EnforcementDecision, + target, targetNS string, intentions structs.Intentions, matchType structs.IntentionMatchType, + defaultDecision acl.EnforcementDecision, allowPermissions bool, ) (structs.IntentionDecisionSummary, error) { - _, matches, err := s.IntentionMatch(nil, &structs.IntentionQueryMatch{ - Type: structs.IntentionMatchDestination, - Entries: []structs.IntentionMatchEntry{ - { - Namespace: dstNS, - Name: dstName, - }, - }, - }) - if err != nil { - return structs.IntentionDecisionSummary{}, err - } - if len(matches) != 1 { - // This should never happen since the documented behavior of the - // Match call is that it'll always return exactly the number of results - // as entries passed in. But we guard against misbehavior. - return structs.IntentionDecisionSummary{}, errors.New("internal error loading matches") - } - // Figure out which source matches this request. var ixnMatch *structs.Intention - for _, ixn := range matches[0] { - if _, ok := srcURI.Authorize(ixn); ok { + for _, ixn := range intentions { + if _, ok := AuthorizeIntentionTarget(target, targetNS, ixn, matchType); ok { ixnMatch = ixn break } @@ -778,7 +761,7 @@ func (s *Store) IntentionDecision( if len(ixnMatch.Permissions) > 0 { // If there are L7 permissions, DENY. // We are only evaluating source and destination, not the request that will be sent. - resp.Allowed = false + resp.Allowed = allowPermissions resp.HasPermissions = true } resp.ExternalSource = ixnMatch.Meta[structs.MetaExternalSource] @@ -792,6 +775,37 @@ func (s *Store) IntentionDecision( return resp, nil } +// AuthorizeIntentionTarget determines whether the destination is covered by the given intention +// and whether the intention action allows a connection. +func AuthorizeIntentionTarget(target, targetNS string, ixn *structs.Intention, matchType structs.IntentionMatchType) (bool, bool) { + switch matchType { + case structs.IntentionMatchDestination: + if ixn.DestinationNS != structs.WildcardSpecifier && ixn.DestinationNS != targetNS { + // Non-matching namespace + return false, false + } + + if ixn.DestinationName != structs.WildcardSpecifier && ixn.DestinationName != target { + // Non-matching name + return false, false + } + + case structs.IntentionMatchSource: + if ixn.SourceNS != structs.WildcardSpecifier && ixn.SourceNS != targetNS { + // Non-matching namespace + return false, false + } + + if ixn.SourceName != structs.WildcardSpecifier && ixn.SourceName != target { + // Non-matching name + return false, false + } + } + + // The name and namespace match, so the destination is covered + return ixn.Action == structs.IntentionActionAllow, true +} + // IntentionMatch returns the list of intentions that match the namespace and // name for either a source or destination. This applies the resolution rules // so wildcards will match any value. diff --git a/agent/consul/state/intention_test.go b/agent/consul/state/intention_test.go index 7da6c9a17..5f4ec6440 100644 --- a/agent/consul/state/intention_test.go +++ b/agent/consul/state/intention_test.go @@ -9,7 +9,6 @@ import ( "github.com/stretchr/testify/require" "github.com/hashicorp/consul/acl" - "github.com/hashicorp/consul/agent/connect" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/sdk/testutil" ) @@ -1760,16 +1759,19 @@ func TestStore_IntentionDecision(t *testing.T) { } tt := []struct { - name string - src string - dst string - defaultDecision acl.EnforcementDecision - expect structs.IntentionDecisionSummary + name string + src string + dst string + matchType structs.IntentionMatchType + defaultDecision acl.EnforcementDecision + allowPermissions bool + expect structs.IntentionDecisionSummary }{ { name: "no matching intention and default deny", src: "does-not-exist", dst: "ditto", + matchType: structs.IntentionMatchDestination, defaultDecision: acl.Deny, expect: structs.IntentionDecisionSummary{Allowed: false}, }, @@ -1777,13 +1779,15 @@ func TestStore_IntentionDecision(t *testing.T) { name: "no matching intention and default allow", src: "does-not-exist", dst: "ditto", + matchType: structs.IntentionMatchDestination, defaultDecision: acl.Allow, expect: structs.IntentionDecisionSummary{Allowed: true}, }, { - name: "denied with permissions", - src: "web", - dst: "redis", + name: "denied with permissions", + src: "web", + dst: "redis", + matchType: structs.IntentionMatchDestination, expect: structs.IntentionDecisionSummary{ Allowed: false, HasPermissions: true, @@ -1791,9 +1795,22 @@ func TestStore_IntentionDecision(t *testing.T) { }, }, { - name: "denied without permissions", - src: "api", - dst: "redis", + name: "allowed with permissions", + src: "web", + dst: "redis", + allowPermissions: true, + matchType: structs.IntentionMatchDestination, + expect: structs.IntentionDecisionSummary{ + Allowed: true, + HasPermissions: true, + HasExact: true, + }, + }, + { + name: "denied without permissions", + src: "api", + dst: "redis", + matchType: structs.IntentionMatchDestination, expect: structs.IntentionDecisionSummary{ Allowed: false, HasPermissions: false, @@ -1801,9 +1818,10 @@ func TestStore_IntentionDecision(t *testing.T) { }, }, { - name: "allowed from external source", - src: "api", - dst: "web", + name: "allowed from external source", + src: "api", + dst: "web", + matchType: structs.IntentionMatchDestination, expect: structs.IntentionDecisionSummary{ Allowed: true, HasPermissions: false, @@ -1812,9 +1830,21 @@ func TestStore_IntentionDecision(t *testing.T) { }, }, { - name: "allowed by source wildcard not exact", - src: "anything", - dst: "mysql", + name: "allowed by source wildcard not exact", + src: "anything", + dst: "mysql", + matchType: structs.IntentionMatchDestination, + expect: structs.IntentionDecisionSummary{ + Allowed: true, + HasPermissions: false, + HasExact: false, + }, + }, + { + name: "allowed by matching on source", + src: "web", + dst: "api", + matchType: structs.IntentionMatchSource, expect: structs.IntentionDecisionSummary{ Allowed: true, HasPermissions: false, @@ -1824,17 +1854,197 @@ func TestStore_IntentionDecision(t *testing.T) { } for _, tc := range tt { t.Run(tc.name, func(t *testing.T) { - uri := connect.SpiffeIDService{ - Service: tc.src, + entry := structs.IntentionMatchEntry{ Namespace: structs.IntentionDefaultNamespace, + Name: tc.src, } - decision, err := s.IntentionDecision(&uri, tc.dst, structs.IntentionDefaultNamespace, tc.defaultDecision) + _, intentions, err := s.IntentionMatchOne(nil, entry, structs.IntentionMatchSource) + if err != nil { + require.NoError(t, err) + } + decision, err := s.IntentionDecision(tc.dst, structs.IntentionDefaultNamespace, intentions, tc.matchType, tc.defaultDecision, tc.allowPermissions) require.NoError(t, err) require.Equal(t, tc.expect, decision) }) } } +func TestAuthorizeIntentionTarget(t *testing.T) { + cases := []struct { + name string + target string + targetNS string + ixn *structs.Intention + matchType structs.IntentionMatchType + auth bool + match bool + }{ + // Source match type + { + name: "match exact source, not matching namespace", + target: "web", + targetNS: structs.IntentionDefaultNamespace, + ixn: &structs.Intention{ + SourceName: "db", + SourceNS: "different", + }, + matchType: structs.IntentionMatchSource, + auth: false, + match: false, + }, + { + name: "match exact source, not matching name", + target: "web", + targetNS: structs.IntentionDefaultNamespace, + ixn: &structs.Intention{ + SourceName: "db", + SourceNS: structs.IntentionDefaultNamespace, + }, + matchType: structs.IntentionMatchSource, + auth: false, + match: false, + }, + { + name: "match exact source, allow", + target: "web", + targetNS: structs.IntentionDefaultNamespace, + ixn: &structs.Intention{ + SourceName: "web", + SourceNS: structs.IntentionDefaultNamespace, + Action: structs.IntentionActionAllow, + }, + matchType: structs.IntentionMatchSource, + auth: true, + match: true, + }, + { + name: "match exact source, deny", + target: "web", + targetNS: structs.IntentionDefaultNamespace, + ixn: &structs.Intention{ + SourceName: "web", + SourceNS: structs.IntentionDefaultNamespace, + Action: structs.IntentionActionDeny, + }, + matchType: structs.IntentionMatchSource, + auth: false, + match: true, + }, + { + name: "match exact sourceNS for wildcard service, deny", + target: "web", + targetNS: structs.IntentionDefaultNamespace, + ixn: &structs.Intention{ + SourceName: structs.WildcardSpecifier, + SourceNS: structs.IntentionDefaultNamespace, + Action: structs.IntentionActionDeny, + }, + matchType: structs.IntentionMatchSource, + auth: false, + match: true, + }, + { + name: "match exact sourceNS for wildcard service, allow", + target: "web", + targetNS: structs.IntentionDefaultNamespace, + ixn: &structs.Intention{ + SourceName: structs.WildcardSpecifier, + SourceNS: structs.IntentionDefaultNamespace, + Action: structs.IntentionActionAllow, + }, + matchType: structs.IntentionMatchSource, + auth: true, + match: true, + }, + + // Destination match type + { + name: "match exact destination, not matching namespace", + target: "web", + targetNS: structs.IntentionDefaultNamespace, + ixn: &structs.Intention{ + DestinationName: "db", + DestinationNS: "different", + }, + matchType: structs.IntentionMatchDestination, + auth: false, + match: false, + }, + { + name: "match exact destination, not matching name", + target: "web", + targetNS: structs.IntentionDefaultNamespace, + ixn: &structs.Intention{ + DestinationName: "db", + DestinationNS: structs.IntentionDefaultNamespace, + }, + matchType: structs.IntentionMatchDestination, + auth: false, + match: false, + }, + { + name: "match exact destination, allow", + target: "web", + targetNS: structs.IntentionDefaultNamespace, + ixn: &structs.Intention{ + DestinationName: "web", + DestinationNS: structs.IntentionDefaultNamespace, + Action: structs.IntentionActionAllow, + }, + matchType: structs.IntentionMatchDestination, + auth: true, + match: true, + }, + { + name: "match exact destination, deny", + target: "web", + targetNS: structs.IntentionDefaultNamespace, + ixn: &structs.Intention{ + DestinationName: "web", + DestinationNS: structs.IntentionDefaultNamespace, + Action: structs.IntentionActionDeny, + }, + matchType: structs.IntentionMatchDestination, + auth: false, + match: true, + }, + { + name: "match exact destinationNS for wildcard service, deny", + target: "web", + targetNS: structs.IntentionDefaultNamespace, + ixn: &structs.Intention{ + DestinationName: structs.WildcardSpecifier, + DestinationNS: structs.IntentionDefaultNamespace, + Action: structs.IntentionActionDeny, + }, + matchType: structs.IntentionMatchDestination, + auth: false, + match: true, + }, + { + name: "match exact destinationNS for wildcard service, allow", + target: "web", + targetNS: structs.IntentionDefaultNamespace, + ixn: &structs.Intention{ + DestinationName: structs.WildcardSpecifier, + DestinationNS: structs.IntentionDefaultNamespace, + Action: structs.IntentionActionAllow, + }, + matchType: structs.IntentionMatchDestination, + auth: true, + match: true, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + auth, match := AuthorizeIntentionTarget(tc.target, tc.targetNS, tc.ixn, tc.matchType) + assert.Equal(t, tc.auth, auth) + assert.Equal(t, tc.match, match) + }) + } +} + func disableLegacyIntentions(s *Store) error { return s.SystemMetadataSet(1, &structs.SystemMetadataEntry{ Key: structs.SystemMetadataIntentionFormatKey,