Use TargetRef for Authorization Policy Enforcement (istio#46560)

* Add temporary comments for work to be done Signed-off-by: Whitney Griffith <[email protected]> * modify ListAuthorizationPolicies() to use targetRef instead of workloadSelector when in ambient mode Signed-off-by: Whitney Griffith <[email protected]> * add targetRef testcases to authorization_test.go Signed-off-by: Whitney Griffith <[email protected]> * address comments Signed-off-by: Whitney Griffith <[email protected]> * address comments Signed-off-by: Whitney Griffith <[email protected]> * ListAuthorizationPolicies expects a proxyInfo struct Signed-off-by: Whitney Griffith <[email protected]> * Refactor ListAuthorizationPolicies() Signed-off-by: Whitney Griffith <[email protected]> * address comments Signed-off-by: Whitney Griffith <[email protected]> * address comments Signed-off-by: Whitney Griffith <[email protected]> * refactor to apply targetRef to ingress gateways Signed-off-by: Whitney Griffith <[email protected]> * Add release notes Signed-off-by: Whitney Griffith <[email protected]> * Address tests errors Signed-off-by: Whitney Griffith <[email protected]> * Rename workload selector options struct Signed-off-by: Whitney Griffith <[email protected]> * Refactor to use getPolicymatcher Signed-off-by: Whitney Griffith <[email protected]> * Export WorkloadSelectorOpts fields Signed-off-by: Whitney Griffith <[email protected]> * Fix linter Signed-off-by: Whitney Griffith <[email protected]> * Update authorization tests Signed-off-by: Whitney Griffith <[email protected]> * Update authorization tests Signed-off-by: Whitney Griffith <[email protected]> * update AuthorizationPolicy in test data Signed-off-by: Whitney Griffith <[email protected]> * update AuthorizationPolicy in policy attachment test data Signed-off-by: Whitney Griffith <[email protected]> * Address comments Signed-off-by: Whitney Griffith <[email protected]> * Address comments Signed-off-by: Whitney Griffith <[email protected]> --------- Signed-off-by: Whitney Griffith <[email protected]> Signed-off-by: Whitney Griffith <[email protected]>
detailyang · Oct 5, 2023 · e333e90 · e333e90
1 parent 12ada53
commit e333e90
Show file tree

Hide file tree

Showing 11 changed files with 216 additions and 78 deletions.
diff --git a/pilot/pkg/model/authorization.go b/pilot/pkg/model/authorization.go
@@ -66,42 +66,57 @@ type AuthorizationPoliciesResult struct {
 }
 
 // ListAuthorizationPolicies returns authorization policies applied to the workload in the given namespace.
-func (policy *AuthorizationPolicies) ListAuthorizationPolicies(namespace string, workload labels.Instance) AuthorizationPoliciesResult {
-	ret := AuthorizationPoliciesResult{}
+func (policy *AuthorizationPolicies) ListAuthorizationPolicies(selectionOpts WorkloadSelectionOpts) AuthorizationPoliciesResult {
+	configs := AuthorizationPoliciesResult{}
 	if policy == nil {
-		return ret
+		return configs
 	}
+	rootNamespace := policy.RootNamespace
+	namespace := selectionOpts.Namespace
+	workloadLabels := selectionOpts.WorkloadLabels
+	var lookupInNamespaces []string
 
-	var namespaces []string
-	if policy.RootNamespace != "" {
-		namespaces = append(namespaces, policy.RootNamespace)
-	}
-	// To prevent duplicate policies in case root namespace equals proxy's namespace.
-	if namespace != policy.RootNamespace {
-		namespaces = append(namespaces, namespace)
+	if namespace != rootNamespace {
+		// Only check the root namespace if the (workload) namespace is not already the root namespace
+		// to avoid double inclusion.
+		lookupInNamespaces = []string{rootNamespace, namespace}
+	} else {
+		lookupInNamespaces = []string{namespace}
 	}
 
-	for _, ns := range namespaces {
+	for _, ns := range lookupInNamespaces {
 		for _, config := range policy.NamespaceToPolicies[ns] {
 			spec := config.Spec
-			selector := labels.Instance(spec.GetSelector().GetMatchLabels())
-			if selector.SubsetOf(workload) {
-				switch config.Spec.GetAction() {
-				case authpb.AuthorizationPolicy_ALLOW:
-					ret.Allow = append(ret.Allow, config)
-				case authpb.AuthorizationPolicy_DENY:
-					ret.Deny = append(ret.Deny, config)
-				case authpb.AuthorizationPolicy_AUDIT:
-					ret.Audit = append(ret.Audit, config)
-				case authpb.AuthorizationPolicy_CUSTOM:
-					ret.Custom = append(ret.Custom, config)
-				default:
-					log.Errorf("ignored authorization policy %s.%s with unsupported action: %s",
-						config.Namespace, config.Name, config.Spec.GetAction())
+			switch getPolicyMatcher(gvk.AuthorizationPolicy, config.Name, selectionOpts, spec) {
+			case policyMatchSelector:
+				selector := labels.Instance(spec.GetSelector().GetMatchLabels())
+				if selector.SubsetOf(workloadLabels) {
+					configs = updateAuthorizationPoliciesResult(configs, config)
 				}
+			case policyMatchDirect:
+				configs = updateAuthorizationPoliciesResult(configs, config)
 			}
 		}
 	}
 
-	return ret
+	return configs
+}
+
+func updateAuthorizationPoliciesResult(configs AuthorizationPoliciesResult, config AuthorizationPolicy) AuthorizationPoliciesResult {
+	log.Infof("applying authorization policy %s.%s",
+		config.Namespace, config.Name)
+	switch config.Spec.GetAction() {
+	case authpb.AuthorizationPolicy_ALLOW:
+		configs.Allow = append(configs.Allow, config)
+	case authpb.AuthorizationPolicy_DENY:
+		configs.Deny = append(configs.Deny, config)
+	case authpb.AuthorizationPolicy_AUDIT:
+		configs.Audit = append(configs.Audit, config)
+	case authpb.AuthorizationPolicy_CUSTOM:
+		configs.Custom = append(configs.Custom, config)
+	default:
+		log.Errorf("ignored authorization policy %s.%s with unsupported action: %s",
+			config.Namespace, config.Name, config.Spec.GetAction())
+	}
+	return configs
 }
diff --git a/pilot/pkg/model/authorization_test.go b/pilot/pkg/model/authorization_test.go
@@ -25,6 +25,8 @@ import (
 	authpb "istio.io/api/security/v1beta1"
 	selectorpb "istio.io/api/type/v1beta1"
 	"istio.io/istio/pkg/config"
+	"istio.io/istio/pkg/config/constants"
+	"istio.io/istio/pkg/config/labels"
 	"istio.io/istio/pkg/config/mesh"
 	"istio.io/istio/pkg/config/schema/collection"
 	"istio.io/istio/pkg/config/schema/gvk"
@@ -53,11 +55,19 @@ func TestAuthorizationPolicies_ListAuthorizationPolicies(t *testing.T) {
 	}
 	policyWithSelector := proto.Clone(policy).(*authpb.AuthorizationPolicy)
 	policyWithSelector.Selector = &selectorpb.WorkloadSelector{
-		MatchLabels: map[string]string{
+		MatchLabels: labels.Instance{
 			"app":     "httpbin",
 			"version": "v1",
 		},
 	}
+	policyWithTargetRef := proto.Clone(policy).(*authpb.AuthorizationPolicy)
+	policyWithTargetRef.TargetRef = &selectorpb.PolicyTargetReference{
+		Group:     gvk.KubernetesGateway.Group,
+		Kind:      gvk.KubernetesGateway.Kind,
+		Name:      "my-gateway",
+		Namespace: "bar",
+	}
+
 	denyPolicy := proto.Clone(policy).(*authpb.AuthorizationPolicy)
 	denyPolicy.Action = authpb.AuthorizationPolicy_DENY
 
@@ -68,32 +78,50 @@ func TestAuthorizationPolicies_ListAuthorizationPolicies(t *testing.T) {
 	customPolicy.Action = authpb.AuthorizationPolicy_CUSTOM
 
 	cases := []struct {
-		name           string
-		ns             string
-		workloadLabels map[string]string
-		configs        []config.Config
-		wantDeny       []AuthorizationPolicy
-		wantAllow      []AuthorizationPolicy
-		wantAudit      []AuthorizationPolicy
-		wantCustom     []AuthorizationPolicy
+		name          string
+		selectionOpts WorkloadSelectionOpts
+		configs       []config.Config
+		wantDeny      []AuthorizationPolicy
+		wantAllow     []AuthorizationPolicy
+		wantAudit     []AuthorizationPolicy
+		wantCustom    []AuthorizationPolicy
 	}{
 		{
-			name:      "no policies",
-			ns:        "foo",
+			name: "no policies",
+			selectionOpts: WorkloadSelectionOpts{
+				Namespace: "foo",
+			},
 			wantAllow: nil,
 		},
 		{
 			name: "no policies in namespace foo",
-			ns:   "foo",
+			selectionOpts: WorkloadSelectionOpts{
+				Namespace: "foo",
+			},
 			configs: []config.Config{
 				newConfig("authz-1", "bar", policy),
 				newConfig("authz-2", "bar", policy),
 			},
 			wantAllow: nil,
 		},
+		{
+			name: "no policies with a targetRef in namespace foo",
+			selectionOpts: WorkloadSelectionOpts{
+				Namespace: "foo",
+				WorkloadLabels: labels.Instance{
+					constants.GatewayNameLabel: "my-gateway",
+				},
+			},
+			configs: []config.Config{
+				newConfig("authz-1", "bar", policyWithTargetRef),
+			},
+			wantAllow: nil,
+		},
 		{
 			name: "one allow policy",
-			ns:   "bar",
+			selectionOpts: WorkloadSelectionOpts{
+				Namespace: "bar",
+			},
 			configs: []config.Config{
 				newConfig("authz-1", "bar", policy),
 			},
@@ -107,7 +135,9 @@ func TestAuthorizationPolicies_ListAuthorizationPolicies(t *testing.T) {
 		},
 		{
 			name: "one deny policy",
-			ns:   "bar",
+			selectionOpts: WorkloadSelectionOpts{
+				Namespace: "bar",
+			},
 			configs: []config.Config{
 				newConfig("authz-1", "bar", denyPolicy),
 			},
@@ -121,7 +151,9 @@ func TestAuthorizationPolicies_ListAuthorizationPolicies(t *testing.T) {
 		},
 		{
 			name: "one audit policy",
-			ns:   "bar",
+			selectionOpts: WorkloadSelectionOpts{
+				Namespace: "bar",
+			},
 			configs: []config.Config{
 				newConfig("authz-1", "bar", auditPolicy),
 			},
@@ -135,7 +167,9 @@ func TestAuthorizationPolicies_ListAuthorizationPolicies(t *testing.T) {
 		},
 		{
 			name: "one custom policy",
-			ns:   "bar",
+			selectionOpts: WorkloadSelectionOpts{
+				Namespace: "bar",
+			},
 			configs: []config.Config{
 				newConfig("authz-1", "bar", customPolicy),
 			},
@@ -149,7 +183,9 @@ func TestAuthorizationPolicies_ListAuthorizationPolicies(t *testing.T) {
 		},
 		{
 			name: "two policies",
-			ns:   "bar",
+			selectionOpts: WorkloadSelectionOpts{
+				Namespace: "bar",
+			},
 			configs: []config.Config{
 				newConfig("authz-1", "foo", policy),
 				newConfig("authz-1", "bar", policy),
@@ -170,7 +206,9 @@ func TestAuthorizationPolicies_ListAuthorizationPolicies(t *testing.T) {
 		},
 		{
 			name: "mixing allow, deny, and audit policies",
-			ns:   "bar",
+			selectionOpts: WorkloadSelectionOpts{
+				Namespace: "bar",
+			},
 			configs: []config.Config{
 				newConfig("authz-1", "bar", policy),
 				newConfig("authz-2", "bar", denyPolicy),
@@ -204,12 +242,33 @@ func TestAuthorizationPolicies_ListAuthorizationPolicies(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "targetRef is an exact match",
+			selectionOpts: WorkloadSelectionOpts{
+				Namespace: "bar",
+				WorkloadLabels: labels.Instance{
+					constants.GatewayNameLabel: "my-gateway",
+				},
+			},
+			configs: []config.Config{
+				newConfig("authz-1", "bar", policyWithTargetRef),
+			},
+			wantAllow: []AuthorizationPolicy{
+				{
+					Name:      "authz-1",
+					Namespace: "bar",
+					Spec:      policyWithTargetRef,
+				},
+			},
+		},
 		{
 			name: "selector exact match",
-			ns:   "bar",
-			workloadLabels: map[string]string{
-				"app":     "httpbin",
-				"version": "v1",
+			selectionOpts: WorkloadSelectionOpts{
+				Namespace: "bar",
+				WorkloadLabels: labels.Instance{
+					"app":     "httpbin",
+					"version": "v1",
+				},
 			},
 			configs: []config.Config{
 				newConfig("authz-1", "bar", policyWithSelector),
@@ -224,11 +283,13 @@ func TestAuthorizationPolicies_ListAuthorizationPolicies(t *testing.T) {
 		},
 		{
 			name: "selector subset match",
-			ns:   "bar",
-			workloadLabels: map[string]string{
-				"app":     "httpbin",
-				"version": "v1",
-				"env":     "dev",
+			selectionOpts: WorkloadSelectionOpts{
+				Namespace: "bar",
+				WorkloadLabels: labels.Instance{
+					"app":     "httpbin",
+					"version": "v1",
+					"env":     "dev",
+				},
 			},
 			configs: []config.Config{
 				newConfig("authz-1", "bar", policyWithSelector),
@@ -241,12 +302,27 @@ func TestAuthorizationPolicies_ListAuthorizationPolicies(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "targetRef is not a match",
+			selectionOpts: WorkloadSelectionOpts{
+				Namespace: "bar",
+				WorkloadLabels: labels.Instance{
+					constants.GatewayNameLabel: "my-gateway2",
+				},
+			},
+			configs: []config.Config{
+				newConfig("authz-1", "bar", policyWithTargetRef),
+			},
+			wantAllow: nil,
+		},
 		{
 			name: "selector not match",
-			ns:   "bar",
-			workloadLabels: map[string]string{
-				"app":     "httpbin",
-				"version": "v2",
+			selectionOpts: WorkloadSelectionOpts{
+				Namespace: "bar",
+				WorkloadLabels: labels.Instance{
+					"app":     "httpbin",
+					"version": "v2",
+				},
 			},
 			configs: []config.Config{
 				newConfig("authz-1", "bar", policyWithSelector),
@@ -255,10 +331,12 @@ func TestAuthorizationPolicies_ListAuthorizationPolicies(t *testing.T) {
 		},
 		{
 			name: "namespace not match",
-			ns:   "foo",
-			workloadLabels: map[string]string{
-				"app":     "httpbin",
-				"version": "v1",
+			selectionOpts: WorkloadSelectionOpts{
+				Namespace: "foo",
+				WorkloadLabels: labels.Instance{
+					"app":     "httpbin",
+					"version": "v1",
+				},
 			},
 			configs: []config.Config{
 				newConfig("authz-1", "bar", policyWithSelector),
@@ -267,7 +345,9 @@ func TestAuthorizationPolicies_ListAuthorizationPolicies(t *testing.T) {
 		},
 		{
 			name: "root namespace",
-			ns:   "bar",
+			selectionOpts: WorkloadSelectionOpts{
+				Namespace: "bar",
+			},
 			configs: []config.Config{
 				newConfig("authz-1", "istio-config", policy),
 			},
@@ -281,7 +361,9 @@ func TestAuthorizationPolicies_ListAuthorizationPolicies(t *testing.T) {
 		},
 		{
 			name: "root namespace equals config namespace",
-			ns:   "istio-config",
+			selectionOpts: WorkloadSelectionOpts{
+				Namespace: "istio-config",
+			},
 			configs: []config.Config{
 				newConfig("authz-1", "istio-config", policy),
 			},
@@ -295,7 +377,9 @@ func TestAuthorizationPolicies_ListAuthorizationPolicies(t *testing.T) {
 		},
 		{
 			name: "root namespace and config namespace",
-			ns:   "bar",
+			selectionOpts: WorkloadSelectionOpts{
+				Namespace: "bar",
+			},
 			configs: []config.Config{
 				newConfig("authz-1", "istio-config", policy),
 				newConfig("authz-2", "bar", policy),
@@ -319,7 +403,7 @@ func TestAuthorizationPolicies_ListAuthorizationPolicies(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			authzPolicies := createFakeAuthorizationPolicies(tc.configs)
 
-			result := authzPolicies.ListAuthorizationPolicies(tc.ns, tc.workloadLabels)
+			result := authzPolicies.ListAuthorizationPolicies(tc.selectionOpts)
 			if !reflect.DeepEqual(tc.wantAllow, result.Allow) {
 				t.Errorf("wantAllow:%v\n but got: %v\n", tc.wantAllow, result.Allow)
 			}

diff --git a/pilot/pkg/model/push_context.go b/pilot/pkg/model/push_context.go
@@ -1815,6 +1815,7 @@ func (ps *PushContext) setDestinationRules(configs []config.Config) {
 	ps.destinationRuleIndex.rootNamespaceLocal = rootNamespaceLocalDestRules
 }
 
+// pre computes all AuthorizationPolicies per namespace
 func (ps *PushContext) initAuthorizationPolicies(env *Environment) {
 	ps.AuthzPolicies = GetAuthorizationPolicies(env)
 }