From 7f1b5bd3d82e7ce6856296efd514e9ddaff4b774 Mon Sep 17 00:00:00 2001 From: Istio Automation Date: Tue, 17 Dec 2024 14:45:00 -0500 Subject: [PATCH 1/6] Update BASE_VERSION to 1.22-2024-12-17T19-01-34 (#54372) --- Makefile.core.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile.core.mk b/Makefile.core.mk index 8d2edb662832..e406260b5bdf 100644 --- a/Makefile.core.mk +++ b/Makefile.core.mk @@ -49,7 +49,7 @@ endif export VERSION # Base version of Istio image to use -BASE_VERSION ?= 1.22-2024-11-26T19-01-41 +BASE_VERSION ?= 1.22-2024-12-17T19-01-34 ISTIO_BASE_REGISTRY ?= gcr.io/istio-release export GO111MODULE ?= on From aa91038058791a28dba57a481dd3fb0dac6544d6 Mon Sep 17 00:00:00 2001 From: Istio Automation Date: Wed, 18 Dec 2024 20:29:33 -0500 Subject: [PATCH 2/6] Automator: update proxy@release-1.22 in istio/istio@release-1.22 (#54405) --- istio.deps | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/istio.deps b/istio.deps index eb23fdec9811..9f342783ecef 100644 --- a/istio.deps +++ b/istio.deps @@ -4,7 +4,7 @@ "name": "PROXY_REPO_SHA", "repoName": "proxy", "file": "", - "lastStableSHA": "41fcc90e64d41254d0e7b9e167d4ebe54902f72f" + "lastStableSHA": "3a5ace882d416a883b5269d1db077b3e1ca1f8b4" }, { "_comment": "", From 878307df9b44e171d3f6b6e933e1103944d50e8d Mon Sep 17 00:00:00 2001 From: Istio Automation Date: Thu, 2 Jan 2025 20:26:55 -0500 Subject: [PATCH 3/6] Automator: update proxy@release-1.22 in istio/istio@release-1.22 (#54535) --- istio.deps | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/istio.deps b/istio.deps index 9f342783ecef..f62788b9cf19 100644 --- a/istio.deps +++ b/istio.deps @@ -4,7 +4,7 @@ "name": "PROXY_REPO_SHA", "repoName": "proxy", "file": "", - "lastStableSHA": "3a5ace882d416a883b5269d1db077b3e1ca1f8b4" + "lastStableSHA": "b79502579bd44605d3ce9e1747ac3ed330beb947" }, { "_comment": "", From 16dfb62612f9f6de748436f3231ecdc009bf5c0e Mon Sep 17 00:00:00 2001 From: Keith Mattix II Date: Fri, 3 Jan 2025 14:46:58 -0600 Subject: [PATCH 4/6] PeerAuthentication Merging for Ambient (#54073) (#54462) * Stash * Stash * Implement merging for PeerAuthentication for correctness * Don't attach static strict if policy is merged * Testing: Peerauthn blocking traffic * Fix tests * Gen and lint * Add release note * Fix comments and test --------- Signed-off-by: Keith Mattix II --- .../controller/ambient/ambientindex_test.go | 96 ++++++++++++++++--- .../kube/controller/ambient/authorization.go | 79 +++++++++------ .../kube/controller/ambient/policies.go | 53 +++++++++- ...ermissive-root-permissive-workload-in.yaml | 21 ++++ ...n-permissive-root-permissive-workload.yaml | 0 ...rict-namespace-permissive-workload-in.yaml | 30 ++++++ ...-strict-namespace-permissive-workload.yaml | 12 +++ ...rmissive-namespace-strict-workload-in.yaml | 30 ++++++ ...-permissive-namespace-strict-workload.yaml | 11 +++ ...hn-strict-root-permissive-workload-in.yaml | 21 ++++ ...authn-strict-root-permissive-workload.yaml | 12 +++ ...r-authn-unset-port-mtls-permissive-in.yaml | 13 --- ...peer-authn-unset-port-mtls-permissive.yaml | 8 -- releasenotes/notes/53884.yaml | 9 ++ tests/integration/ambient/baseline_test.go | 75 ++++++++++++++- 15 files changed, 404 insertions(+), 66 deletions(-) create mode 100644 pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-permissive-workload-in.yaml create mode 100644 pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-permissive-workload.yaml create mode 100644 pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-strict-namespace-permissive-workload-in.yaml create mode 100644 pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-strict-namespace-permissive-workload.yaml create mode 100644 pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-root-permissive-namespace-strict-workload-in.yaml create mode 100644 pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-root-permissive-namespace-strict-workload.yaml create mode 100644 pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-root-permissive-workload-in.yaml create mode 100644 pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-root-permissive-workload.yaml delete mode 100644 pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-unset-port-mtls-permissive-in.yaml delete mode 100644 pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-unset-port-mtls-permissive.yaml create mode 100644 releasenotes/notes/53884.yaml diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/ambientindex_test.go b/pilot/pkg/serviceregistry/kube/controller/ambient/ambientindex_test.go index 4fb45ec27042..03b4027b82be 100644 --- a/pilot/pkg/serviceregistry/kube/controller/ambient/ambientindex_test.go +++ b/pilot/pkg/serviceregistry/kube/controller/ambient/ambientindex_test.go @@ -51,6 +51,7 @@ import ( "istio.io/istio/pkg/kube/kclient/clienttest" "istio.io/istio/pkg/kube/krt" "istio.io/istio/pkg/network" + "istio.io/istio/pkg/ptr" "istio.io/istio/pkg/slices" "istio.io/istio/pkg/test" "istio.io/istio/pkg/test/util/assert" @@ -870,10 +871,10 @@ func TestAmbientIndex_Policy(t *testing.T) { } }) s.assertEvent(t, s.podXdsName("pod1"), s.podXdsName("pod2"), xdsConvertedPeerAuthSelector) // Matching pods receive an event - // The policy should still be added since the effective policy is STRICT + // There should be no static strict policy because the permissive override should have the strict part in-line assert.Equal(t, s.lookup(s.addrXdsName("127.0.0.1"))[0].Address.GetWorkload().AuthorizationPolicies, - []string{fmt.Sprintf("istio-system/%s", staticStrictPolicyName), fmt.Sprintf("ns1/%s", model.GetAmbientPolicyConfigName(model.ConfigKey{ + []string{fmt.Sprintf("ns1/%s", model.GetAmbientPolicyConfigName(model.ConfigKey{ Kind: kind.PeerAuthentication, Name: selectorPolicyName, Namespace: "ns1", @@ -1179,15 +1180,88 @@ func TestRBACConvert(t *testing.T) { }, Spec: *((pol[0].Spec).(*auth.AuthorizationPolicy)), //nolint: govet }) - case gvk.PeerAuthentication: - o = convertPeerAuthentication(systemNS, &clientsecurityv1beta1.PeerAuthentication{ - TypeMeta: metav1.TypeMeta{}, - ObjectMeta: metav1.ObjectMeta{ - Name: pol[0].Name, - Namespace: pol[0].Namespace, - }, - Spec: *((pol[0].Spec).(*auth.PeerAuthentication)), //nolint: govet - }) + case gvk.PeerAuthentication: // we assume all crds in the same file are of the same type + var rootCfg, nsCfg, workloadCfg *config.Config + if len(pol) > 1 { + for _, p := range pol { + switch p.Namespace { + case systemNS: + if p.Spec.(*auth.PeerAuthentication).GetSelector() != nil { + t.Fatalf("unexpected selector in mesh-level policy %v", p) + } + if rootCfg == nil || p.GetCreationTimestamp().Before(rootCfg.GetCreationTimestamp()) { + rootCfg = ptr.Of(p) + } + default: + spec := p.Spec.(*auth.PeerAuthentication) + if spec.GetSelector() != nil && workloadCfg != nil && workloadCfg.Spec.(*auth.PeerAuthentication).GetSelector() != nil { + t.Fatalf("multiple workload-level policies %v and %v", p, workloadCfg) + } + + if spec.GetSelector() != nil { + // Workload policy + workloadCfg = ptr.Of(p) + } else if nsCfg == nil || p.GetCreationTimestamp().Before(nsCfg.GetCreationTimestamp()) { + // Namespace policy + nsCfg = ptr.Of(p) + } + } + } + } + + // Simple case + if len(pol) == 1 { + o = convertPeerAuthentication(systemNS, &clientsecurityv1beta1.PeerAuthentication{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Name: pol[0].Name, + Namespace: pol[0].Namespace, + }, + Spec: *((pol[0].Spec).(*auth.PeerAuthentication)), //nolint: govet + }, nil, nil) + } else { + var workloadPA, nsPA, rootPA *clientsecurityv1beta1.PeerAuthentication + if workloadCfg != nil { + workloadPA = &clientsecurityv1beta1.PeerAuthentication{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Name: workloadCfg.Name, + Namespace: workloadCfg.Namespace, + }, + Spec: *((workloadCfg.Spec).(*auth.PeerAuthentication)), //nolint: govet + } + } + if nsCfg != nil { + nsPA = &clientsecurityv1beta1.PeerAuthentication{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Name: nsCfg.Name, + Namespace: nsCfg.Namespace, + }, + Spec: *((nsCfg.Spec).(*auth.PeerAuthentication)), //nolint: govet + } + } + + if rootCfg != nil { + rootPA = &clientsecurityv1beta1.PeerAuthentication{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Name: rootCfg.Name, + Namespace: rootCfg.Namespace, + }, + Spec: *((rootCfg.Spec).(*auth.PeerAuthentication)), //nolint: govet + } + } + + if workloadPA == nil { + // We have a namespace policy and a root policy; send the NS policy as the workload policy + // It won't get far anyway since this convert function returns nil for non-workload policies + workloadPA = nsPA + nsPA = nil + } + + o = convertPeerAuthentication(systemNS, workloadPA, nsPA, rootPA) + } default: t.Fatalf("unknown kind %v", pol[0].GroupVersionKind) } diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/authorization.go b/pilot/pkg/serviceregistry/kube/controller/ambient/authorization.go index 72e3b6457066..3f61dd74106c 100644 --- a/pilot/pkg/serviceregistry/kube/controller/ambient/authorization.go +++ b/pilot/pkg/serviceregistry/kube/controller/ambient/authorization.go @@ -56,8 +56,18 @@ func (a *index) Policies(requested sets.Set[model.ConfigKey]) []model.WorkloadAu return res } +func getOldestPeerAuthn(policies []*securityclient.PeerAuthentication) *securityclient.PeerAuthentication { + var oldest *securityclient.PeerAuthentication + for _, pol := range policies { + if oldest == nil || pol.CreationTimestamp.Before(&oldest.CreationTimestamp) { + oldest = pol + } + } + return oldest +} + // convertedSelectorPeerAuthentications returns a list of keys corresponding to one or both of: -// [static STRICT policy, port-level STRICT policy] based on the effective PeerAuthentication policy +// [static STRICT policy, port-level (potentially merged) STRICT policy] based on the effective PeerAuthentication policy func convertedSelectorPeerAuthentications(rootNamespace string, configs []*securityclient.PeerAuthentication) []string { var meshCfg, namespaceCfg, workloadCfg *securityclient.PeerAuthentication for _, cfg := range configs { @@ -92,9 +102,7 @@ func convertedSelectorPeerAuthentications(rootNamespace string, configs []*secur // Process in mesh, namespace, workload order to resolve inheritance (UNSET) if meshCfg != nil { - if !isMtlsModeUnset(meshCfg.Spec.Mtls) { - isEffectiveStrictPolicy = isMtlsModeStrict(meshCfg.Spec.Mtls) - } + isEffectiveStrictPolicy = isMtlsModeStrict(meshCfg.Spec.Mtls) } if namespaceCfg != nil { @@ -109,12 +117,10 @@ func convertedSelectorPeerAuthentications(rootNamespace string, configs []*secur workloadSpec := &workloadCfg.Spec - // Regardless of if we have port-level overrides, if the workload policy is STRICT, then we need to reference our static STRICT policy if isMtlsModeStrict(workloadSpec.Mtls) { isEffectiveStrictPolicy = true } - // Regardless of if we have port-level overrides, if the workload policy is PERMISSIVE or DISABLE, then we shouldn't send our static STRICT policy if isMtlsModePermissive(workloadSpec.Mtls) || isMtlsModeDisable(workloadSpec.Mtls) { isEffectiveStrictPolicy = false } @@ -174,6 +180,8 @@ func convertedSelectorPeerAuthentications(rootNamespace string, configs []*secur Kind: kind.PeerAuthentication, Namespace: workloadCfg.Namespace, }) + // We DON'T want to send the static STRICT policy since the merged form of this policy will include the default STRICT mode + isEffectiveStrictPolicy = false } } else { // Permissive mesh or namespace policy @@ -215,15 +223,9 @@ func effectivePeerAuthenticationKeys(rootNamespace string, isEffectiveStringPoli return sets.SortedList(res) } -// convertPeerAuthentication converts a PeerAuthentication to an L4 authorization policy (i.e. security.Authorization) iff -// 1. the PeerAuthentication has a workload selector -// 2. The PeerAuthentication is NOT in the root namespace -// 3. There is a portLevelMtls policy (technically implied by 1) -// 4. If the top-level mode is PERMISSIVE or DISABLE, there is at least one portLevelMtls policy with mode STRICT -// -// STRICT policies that don't have portLevelMtls will be -// handled when the Workload xDS resource is pushed (a static STRICT-equivalent policy will always be pushed) -func convertPeerAuthentication(rootNamespace string, cfg *securityclient.PeerAuthentication) *security.Authorization { +// convertPeerAuthentication converts a PeerAuthentication to an L4 authorization policy (i.e. security.Authorization) +// taking into account the top level policies (and merging if necessary) +func convertPeerAuthentication(rootNamespace string, cfg, nsCfg, rootCfg *securityclient.PeerAuthentication) *security.Authorization { pa := &cfg.Spec mode := pa.GetMtls().GetMode() @@ -272,7 +274,7 @@ func convertPeerAuthentication(rootNamespace string, cfg *securityclient.PeerAut }, }, }) - case portMtlsMode == v1beta1.PeerAuthentication_MutualTLS_PERMISSIVE: + case portMtlsMode == v1beta1.PeerAuthentication_MutualTLS_PERMISSIVE, portMtlsMode == v1beta1.PeerAuthentication_MutualTLS_DISABLE: // Check top-level mode if mode == v1beta1.PeerAuthentication_MutualTLS_PERMISSIVE || mode == v1beta1.PeerAuthentication_MutualTLS_DISABLE { // we don't care; log and continue @@ -280,24 +282,16 @@ func convertPeerAuthentication(rootNamespace string, cfg *securityclient.PeerAut port, portMtlsMode, cfg.Namespace, cfg.Name, mode) continue } - foundNonStrictPortmTLS = true - // If the top level policy is STRICT, we need to add a rule for the port that exempts it from the deny policy - rules = append(rules, &security.Rules{ - Matches: []*security.Match{ - { - NotDestinationPorts: []uint32{port}, // if the incoming connection does not match this port, deny (notice there's no principals requirement) - }, - }, - }) - case portMtlsMode == v1beta1.PeerAuthentication_MutualTLS_DISABLE: - // Check top-level mode - if mode == v1beta1.PeerAuthentication_MutualTLS_PERMISSIVE || mode == v1beta1.PeerAuthentication_MutualTLS_DISABLE { + if mode == v1beta1.PeerAuthentication_MutualTLS_UNSET && ((nsCfg != nil && !isMtlsModeStrict(nsCfg.Spec.Mtls)) || + (nsCfg == nil && rootCfg != nil && !isMtlsModeStrict(rootCfg.Spec.Mtls)) || + (nsCfg == nil && rootCfg == nil)) { // we don't care; log and continue - log.Debugf("skipping port %s/%s for PeerAuthentication %s/%s for ambient since the parent mTLS mode is %s", - port, portMtlsMode, cfg.Namespace, cfg.Name, mode) + log.Debugf("skipping port %s/%s for PeerAuthentication %s/%s for ambient since it's not STRICT and the effective policy is not STRICT", + port, portMtlsMode, cfg.Namespace, cfg.Name) continue } + foundNonStrictPortmTLS = true // If the top level policy is STRICT, we need to add a rule for the port that exempts it from the deny policy @@ -336,6 +330,31 @@ func convertPeerAuthentication(rootNamespace string, cfg *securityclient.PeerAut Groups: []*security.Group{{Rules: rules}}, } + // We only need to merge if the effective policy is STRICT, the workload policy's mode is unstrict, and we have a non-strict port level policy + var shouldMergeStrict bool + // Merge if there's a STRICT root policy and the namespace policy is nil, UNSET or STRICT + if rootCfg != nil && isMtlsModeStrict(rootCfg.Spec.Mtls) && (nsCfg == nil || isMtlsModeUnset(nsCfg.Spec.Mtls) || isMtlsModeStrict(nsCfg.Spec.Mtls)) { + shouldMergeStrict = true + } else if nsCfg != nil && isMtlsModeStrict(nsCfg.Spec.Mtls) { // Merge if there's a STRICT namespace policy + shouldMergeStrict = true + } + + // If the effective policy (namespace or mesh) is STRICT and we have a non-strict port level policy, + // we need to merge that strictness into the workload policy so that the static strict policy + if shouldMergeStrict && foundNonStrictPortmTLS { + opol.Groups[0].Rules = append(opol.Groups[0].Rules, &security.Rules{ + Matches: []*security.Match{ + { + NotPrincipals: []*security.StringMatch{ + { + MatchType: &security.StringMatch_Presence{}, + }, + }, + }, + }, + }) + } + return opol } diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/policies.go b/pilot/pkg/serviceregistry/kube/controller/ambient/policies.go index 00a8cb499501..1d09c7f9069e 100644 --- a/pilot/pkg/serviceregistry/kube/controller/ambient/policies.go +++ b/pilot/pkg/serviceregistry/kube/controller/ambient/policies.go @@ -24,6 +24,7 @@ import ( "istio.io/istio/pilot/pkg/features" "istio.io/istio/pilot/pkg/model" "istio.io/istio/pkg/kube/krt" + "istio.io/istio/pkg/log" "istio.io/istio/pkg/slices" "istio.io/istio/pkg/spiffe" "istio.io/istio/pkg/workloadapi/security" @@ -48,9 +49,59 @@ func PolicyCollections( } }, krt.WithName("AuthzDerivedPolicies")) + PeerAuthByNamespace := krt.NewIndex(PeerAuths, func(p *securityclient.PeerAuthentication) []string { + if p.Spec.GetSelector() == nil { + return []string{p.GetNamespace()} + } + return nil + }) + + // Our derived PeerAuthentication policies are the effective (i.e. potentially merged) set of policies we will send down to ztunnel + // A policy is sent iff (if and only if): + // 1. the PeerAuthentication has a workload selector + // 2. The PeerAuthentication is NOT in the root namespace + // 3. There is a portLevelMtls policy (technically implied by 1) + // 4. If the top-level mode is PERMISSIVE or DISABLE, there is at least one portLevelMtls policy with mode STRICT + // + // STRICT policies that don't have portLevelMtls will be + // handled when the Workload xDS resource is pushed (a static STRICT-equivalent policy will always be pushed) + // + // As a corollary, if the effective top-level policy is STRICT, the workload policy.mode is UNSET + // and the portLevelMtls policy.mode is STRICT, we will merge the portLevelMtls policy with our static strict policy + // (which basically just looks like setting the workload policy.mode to STRICT). This is because our precedence order for policy + // requires that traffic matching *any* DENY policy is blocked, so attaching 2 polciies (the static strict policy + an exception) + // does not work (the traffic will be blocked despite the exception) PeerAuthDerivedPolicies := krt.NewCollection(PeerAuths, func(ctx krt.HandlerContext, i *securityclient.PeerAuthentication) *model.WorkloadAuthorization { meshCfg := krt.FetchOne(ctx, MeshConfig.AsCollection()) - pol := convertPeerAuthentication(meshCfg.GetRootNamespace(), i) + // violates case #1, #2, or #3 + if i.Namespace == meshCfg.GetRootNamespace() || i.Spec.GetSelector() == nil || len(i.Spec.PortLevelMtls) == 0 { + log.Debugf("skipping PeerAuthentication %s/%s for ambient since it isn't a workload policy with port level mTLS", i.Namespace, i.Name) + return nil + } + + var nsPol, rootPol *securityclient.PeerAuthentication + nsPols := PeerAuthByNamespace.Lookup(i.GetNamespace()) + rootPols := PeerAuthByNamespace.Lookup(meshCfg.GetRootNamespace()) + + switch len(nsPols) { + case 0: + nsPol = nil + case 1: + nsPol = nsPols[0] + default: + nsPol = getOldestPeerAuthn(nsPols) + } + + switch len(rootPols) { + case 0: + rootPol = nil + case 1: + rootPol = rootPols[0] + default: + rootPol = getOldestPeerAuthn(rootPols) + } + + pol := convertPeerAuthentication(meshCfg.GetRootNamespace(), i, nsPol, rootPol) if pol == nil { return nil } diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-permissive-workload-in.yaml b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-permissive-workload-in.yaml new file mode 100644 index 000000000000..f2ea35c5d249 --- /dev/null +++ b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-permissive-workload-in.yaml @@ -0,0 +1,21 @@ +apiVersion: security.istio.io/v1 +kind: PeerAuthentication +metadata: + name: default-mesh + namespace: istio-system +spec: + mtls: + mode: PERMISSIVE +--- +apiVersion: security.istio.io/v1 +kind: PeerAuthentication +metadata: + name: workload + namespace: foo +spec: + selector: + matchLabels: + app: a + portLevelMtls: + 9090: + mode: PERMISSIVE diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-permissive-workload.yaml b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-permissive-workload.yaml new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-strict-namespace-permissive-workload-in.yaml b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-strict-namespace-permissive-workload-in.yaml new file mode 100644 index 000000000000..376e97199616 --- /dev/null +++ b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-strict-namespace-permissive-workload-in.yaml @@ -0,0 +1,30 @@ +apiVersion: security.istio.io/v1 +kind: PeerAuthentication +metadata: + name: default-mesh + namespace: istio-system +spec: + mtls: + mode: PERMISSIVE +--- +apiVersion: security.istio.io/v1 +kind: PeerAuthentication +metadata: + name: default-foo + namespace: foo +spec: + mtls: + mode: STRICT +--- +apiVersion: security.istio.io/v1 +kind: PeerAuthentication +metadata: + name: workload + namespace: foo +spec: + selector: + matchLabels: + app: a + portLevelMtls: + 9090: + mode: PERMISSIVE diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-strict-namespace-permissive-workload.yaml b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-strict-namespace-permissive-workload.yaml new file mode 100644 index 000000000000..7abcb27893a7 --- /dev/null +++ b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-strict-namespace-permissive-workload.yaml @@ -0,0 +1,12 @@ +action: DENY +groups: +- rules: + - matches: + - notDestinationPorts: + - 9090 + - matches: + - notPrincipals: + - presence: {} +name: converted_peer_authentication_workload +namespace: foo +scope: WORKLOAD_SELECTOR diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-root-permissive-namespace-strict-workload-in.yaml b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-root-permissive-namespace-strict-workload-in.yaml new file mode 100644 index 000000000000..1f98a64d8748 --- /dev/null +++ b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-root-permissive-namespace-strict-workload-in.yaml @@ -0,0 +1,30 @@ +apiVersion: security.istio.io/v1 +kind: PeerAuthentication +metadata: + name: default-mesh + namespace: istio-system +spec: + mtls: + mode: STRICT +--- +apiVersion: security.istio.io/v1 +kind: PeerAuthentication +metadata: + name: default-foo + namespace: foo +spec: + mtls: + mode: PERMISSIVE +--- +apiVersion: security.istio.io/v1 +kind: PeerAuthentication +metadata: + name: workload + namespace: foo +spec: + selector: + matchLabels: + app: a + portLevelMtls: + 9090: + mode: STRICT diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-root-permissive-namespace-strict-workload.yaml b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-root-permissive-namespace-strict-workload.yaml new file mode 100644 index 000000000000..e6e3be4b97fb --- /dev/null +++ b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-root-permissive-namespace-strict-workload.yaml @@ -0,0 +1,11 @@ +action: DENY +groups: +- rules: + - matches: + - destinationPorts: + - 9090 + notPrincipals: + - presence: {} +name: converted_peer_authentication_workload +namespace: foo +scope: WORKLOAD_SELECTOR diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-root-permissive-workload-in.yaml b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-root-permissive-workload-in.yaml new file mode 100644 index 000000000000..5b91c25576e5 --- /dev/null +++ b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-root-permissive-workload-in.yaml @@ -0,0 +1,21 @@ +apiVersion: security.istio.io/v1 +kind: PeerAuthentication +metadata: + name: default-mesh + namespace: istio-system +spec: + mtls: + mode: STRICT +--- +apiVersion: security.istio.io/v1 +kind: PeerAuthentication +metadata: + name: workload + namespace: foo +spec: + selector: + matchLabels: + app: a + portLevelMtls: + 9090: + mode: PERMISSIVE diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-root-permissive-workload.yaml b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-root-permissive-workload.yaml new file mode 100644 index 000000000000..7abcb27893a7 --- /dev/null +++ b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-root-permissive-workload.yaml @@ -0,0 +1,12 @@ +action: DENY +groups: +- rules: + - matches: + - notDestinationPorts: + - 9090 + - matches: + - notPrincipals: + - presence: {} +name: converted_peer_authentication_workload +namespace: foo +scope: WORKLOAD_SELECTOR diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-unset-port-mtls-permissive-in.yaml b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-unset-port-mtls-permissive-in.yaml deleted file mode 100644 index 1c03b646e234..000000000000 --- a/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-unset-port-mtls-permissive-in.yaml +++ /dev/null @@ -1,13 +0,0 @@ -apiVersion: security.istio.io/v1beta1 -kind: PeerAuthentication -metadata: - name: strict-mtls -spec: - selector: - matchLabels: - app: a - mtls: - mode: UNSET - portLevelMtls: - 9090: - mode: PERMISSIVE diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-unset-port-mtls-permissive.yaml b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-unset-port-mtls-permissive.yaml deleted file mode 100644 index 0cb5212ecb38..000000000000 --- a/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-unset-port-mtls-permissive.yaml +++ /dev/null @@ -1,8 +0,0 @@ -action: DENY -groups: -- rules: - - matches: - - notDestinationPorts: - - 9090 -name: converted_peer_authentication_strict-mtls -scope: WORKLOAD_SELECTOR diff --git a/releasenotes/notes/53884.yaml b/releasenotes/notes/53884.yaml new file mode 100644 index 000000000000..cc541318a374 --- /dev/null +++ b/releasenotes/notes/53884.yaml @@ -0,0 +1,9 @@ +apiVersion: release-notes/v2 +kind: bug-fix +area: security +issue: + - 53884 +releaseNotes: +- | + **Fixed** an issue where Ambient `PeerAuthentication` policies were overly strict. + diff --git a/tests/integration/ambient/baseline_test.go b/tests/integration/ambient/baseline_test.go index 610d8edab980..d7893fe01991 100644 --- a/tests/integration/ambient/baseline_test.go +++ b/tests/integration/ambient/baseline_test.go @@ -810,8 +810,9 @@ spec: } src.CallOrFail(t, opt) }) - // globally peerauth == STRICT, but we have a port-specific allowlist that is PERMISSIVE, - // so anything hitting that port should not be rejected + // general workload peerauth == STRICT, but we have a port-specific allowlist that is PERMISSIVE, + // so anything hitting that port should not be rejected. + // NOTE: Using port 80 since that's what t.NewSubTest("strict-permissive-ports").Run(func(t framework.TestContext) { t.ConfigIstio().Eval(apps.Namespace.Name(), map[string]string{ "Destination": dst.Config().Service, @@ -829,13 +830,81 @@ spec: mtls: mode: STRICT portLevelMtls: - 8080: + 18080: mode: PERMISSIVE `).ApplyOrFail(t) opt = opt.DeepCopy() // Should pass for all workloads, in or out of mesh, targeting this port src.CallOrFail(t, opt) }) + + // global peer auth is strict, but we have a permissive port-level rule + t.NewSubTest("global-strict-permissive-workload-ports").Run(func(t framework.TestContext) { + t.ConfigIstio().YAML(i.Settings().SystemNamespace, ` +apiVersion: security.istio.io/v1 +kind: PeerAuthentication +metadata: + name: global-strict +spec: + mtls: + mode: STRICT + `).ApplyOrFail(t) + t.ConfigIstio().Eval(apps.Namespace.Name(), map[string]string{ + "Destination": dst.Config().Service, + "Source": src.Config().Service, + "Namespace": apps.Namespace.Name(), + }, ` +apiVersion: security.istio.io/v1 +kind: PeerAuthentication +metadata: + name: local-port-override +spec: + selector: + matchLabels: + app: "{{ .Destination }}" + portLevelMtls: + 18080: + mode: PERMISSIVE + `).ApplyOrFail(t) + opt = opt.DeepCopy() + // Should pass for all workloads, in or out of mesh, targeting this port + src.CallOrFail(t, opt) + }) + + t.NewSubTest("global-permissive-strict-workload-ports").Run(func(t framework.TestContext) { + t.ConfigIstio().YAML(i.Settings().SystemNamespace, ` +apiVersion: security.istio.io/v1 +kind: PeerAuthentication +metadata: + name: global-strict +spec: + mtls: + mode: PERMISSIVE + `).ApplyOrFail(t) + t.ConfigIstio().Eval(apps.Namespace.Name(), map[string]string{ + "Destination": dst.Config().Service, + "Source": src.Config().Service, + "Namespace": apps.Namespace.Name(), + }, ` +apiVersion: security.istio.io/v1 +kind: PeerAuthentication +metadata: + name: local-port-override +spec: + selector: + matchLabels: + app: "{{ .Destination }}" + portLevelMtls: + 18080: + mode: STRICT + `).ApplyOrFail(t) + opt = opt.DeepCopy() + if !src.Config().HasProxyCapabilities() && dst.Config().HasProxyCapabilities() { + // Expect deny if the dest is in the mesh (enforcing mTLS) but src is not (not sending mTLS) + opt.Check = CheckDeny + } + src.CallOrFail(t, opt) + }) }) }) } From c3e73828fbcb97e0d2adf3efc74f7daa8593fc89 Mon Sep 17 00:00:00 2001 From: Keith Mattix II Date: Fri, 10 Jan 2025 05:04:04 -0600 Subject: [PATCH 5/6] [release-1.22] Fix Ambient Peer Authn flake (#54566) (#54618) * Fix Ambient Peer Authn flake (#54566) * Peer authn flake fix Signed-off-by: Keith Mattix II * Add release note Signed-off-by: Keith Mattix II --------- Signed-off-by: Keith Mattix II * Sort portlevel mtls before converting Signed-off-by: Keith Mattix II --------- Signed-off-by: Keith Mattix II --- .../kube/controller/ambient/authorization.go | 75 +++++++++++++------ ...ve-port-mtls-strict-and-permissive-in.yaml | 15 ++++ ...ssive-port-mtls-strict-and-permissive.yaml | 10 +++ ...ve-namespace-strict-workload-ports-in.yaml | 32 ++++++++ ...ssive-namespace-strict-workload-ports.yaml | 17 +++++ ...set-namespace-mixed-workload-ports-in.yaml | 32 ++++++++ ...-unset-namespace-mixed-workload-ports.yaml | 11 +++ ...ct-port-mtls-strict-and-permissive-in.yaml | 15 ++++ ...trict-port-mtls-strict-and-permissive.yaml | 11 +++ ...ad-port-mtls-strict-and-permissive-in.yaml | 23 ++++++ ...kload-port-mtls-strict-and-permissive.yaml | 12 +++ releasenotes/notes/54146.yaml | 9 +++ tests/integration/ambient/baseline_test.go | 9 ++- 13 files changed, 245 insertions(+), 26 deletions(-) create mode 100644 pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-port-mtls-strict-and-permissive-in.yaml create mode 100644 pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-port-mtls-strict-and-permissive.yaml create mode 100644 pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-permissive-namespace-strict-workload-ports-in.yaml create mode 100644 pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-permissive-namespace-strict-workload-ports.yaml create mode 100644 pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-unset-namespace-mixed-workload-ports-in.yaml create mode 100644 pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-unset-namespace-mixed-workload-ports.yaml create mode 100644 pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-port-mtls-strict-and-permissive-in.yaml create mode 100644 pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-port-mtls-strict-and-permissive.yaml create mode 100644 pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-root-unset-workload-port-mtls-strict-and-permissive-in.yaml create mode 100644 pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-root-unset-workload-port-mtls-strict-and-permissive.yaml create mode 100644 releasenotes/notes/54146.yaml diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/authorization.go b/pilot/pkg/serviceregistry/kube/controller/ambient/authorization.go index 3f61dd74106c..84353ac9fd7a 100644 --- a/pilot/pkg/serviceregistry/kube/controller/ambient/authorization.go +++ b/pilot/pkg/serviceregistry/kube/controller/ambient/authorization.go @@ -25,6 +25,8 @@ import ( "istio.io/istio/pilot/pkg/model" "istio.io/istio/pkg/config/schema/kind" "istio.io/istio/pkg/log" + "istio.io/istio/pkg/maps" + "istio.io/istio/pkg/slices" "istio.io/istio/pkg/util/sets" "istio.io/istio/pkg/workloadapi/security" ) @@ -239,6 +241,7 @@ func convertPeerAuthentication(rootNamespace string, cfg, nsCfg, rootCfg *securi action := security.Action_DENY var rules []*security.Rules + var groups []*security.Group if mode == v1beta1.PeerAuthentication_MutualTLS_STRICT { rules = append(rules, &security.Rules{ @@ -259,18 +262,39 @@ func convertPeerAuthentication(rootNamespace string, cfg, nsCfg, rootCfg *securi // Note that this doesn't actually attach the policy to any workload; it just makes it available // to ztunnel in case a workload needs it. foundNonStrictPortmTLS := false - for port, mtls := range pa.PortLevelMtls { + keys := slices.Sort(maps.Keys(pa.PortLevelMtls)) + for _, port := range keys { + mtls := pa.PortLevelMtls[port] switch portMtlsMode := mtls.GetMode(); { case portMtlsMode == v1beta1.PeerAuthentication_MutualTLS_STRICT: - rules = append(rules, &security.Rules{ - Matches: []*security.Match{ + // If either: + // 1. The workload-level policy is STRICT + // 2. The workload level policy is nil/unset and the namespace-level policy is STRICT + // 3. The workload level policy is nil/unset and the namespace-level policy is nil/unset and the mesh-level policy is STRICT + // then we don't need to add a rule for this STRICT port since it will be enforced by the parent policy + if isMtlsModeStrict(pa.GetMtls()) || // #1 + (isMtlsModeUnset(pa.GetMtls()) && // First condition for #2 and #3 + (nsCfg != nil && isMtlsModeStrict(nsCfg.Spec.Mtls)) || // #2 + // #3 + ((nsCfg == nil || isMtlsModeUnset(nsCfg.Spec.Mtls)) && rootCfg != nil && isMtlsModeStrict(rootCfg.Spec.Mtls))) { + log.Debugf("skipping port %s/%s for PeerAuthentication %s/%s for ambient since the parent mTLS mode is %s", + port, portMtlsMode, cfg.Namespace, cfg.Name, mode) + continue + } + // Groups are OR'd so we need to create one for each STRICT workload port + groups = append(groups, &security.Group{ + Rules: []*security.Rules{ { - NotPrincipals: []*security.StringMatch{ + Matches: []*security.Match{ { - MatchType: &security.StringMatch_Presence{}, + NotPrincipals: []*security.StringMatch{ + { + MatchType: &security.StringMatch_Presence{}, + }, + }, + DestinationPorts: []uint32{port}, }, }, - DestinationPorts: []uint32{port}, }, }, }) @@ -313,23 +337,11 @@ func convertPeerAuthentication(rootNamespace string, cfg, nsCfg, rootCfg *securi return nil } - if len(rules) == 0 { - // we never added any rules; return + if len(rules) == 0 && len(groups) == 0 { + // we never added any rules or groups; return return nil } - opol := &security.Authorization{ - Name: model.GetAmbientPolicyConfigName(model.ConfigKey{ - Name: cfg.Name, - Kind: kind.PeerAuthentication, - Namespace: cfg.Namespace, - }), - Namespace: cfg.Namespace, - Scope: scope, - Action: action, - Groups: []*security.Group{{Rules: rules}}, - } - // We only need to merge if the effective policy is STRICT, the workload policy's mode is unstrict, and we have a non-strict port level policy var shouldMergeStrict bool // Merge if there's a STRICT root policy and the namespace policy is nil, UNSET or STRICT @@ -340,9 +352,10 @@ func convertPeerAuthentication(rootNamespace string, cfg, nsCfg, rootCfg *securi } // If the effective policy (namespace or mesh) is STRICT and we have a non-strict port level policy, - // we need to merge that strictness into the workload policy so that the static strict policy + // we need to merge that strictness into the workload policy so that the static strict policy can be elided + // from the workload's policy list. if shouldMergeStrict && foundNonStrictPortmTLS { - opol.Groups[0].Rules = append(opol.Groups[0].Rules, &security.Rules{ + rules = append(rules, &security.Rules{ Matches: []*security.Match{ { NotPrincipals: []*security.StringMatch{ @@ -355,6 +368,24 @@ func convertPeerAuthentication(rootNamespace string, cfg, nsCfg, rootCfg *securi }) } + if len(rules) > 0 { + groups = append(groups, &security.Group{ + Rules: rules, + }) + } + + opol := &security.Authorization{ + Name: model.GetAmbientPolicyConfigName(model.ConfigKey{ + Name: cfg.Name, + Kind: kind.PeerAuthentication, + Namespace: cfg.Namespace, + }), + Namespace: cfg.Namespace, + Scope: scope, + Action: action, + Groups: groups, + } + return opol } diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-port-mtls-strict-and-permissive-in.yaml b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-port-mtls-strict-and-permissive-in.yaml new file mode 100644 index 000000000000..71d9c0ad7153 --- /dev/null +++ b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-port-mtls-strict-and-permissive-in.yaml @@ -0,0 +1,15 @@ +apiVersion: security.istio.io/v1 +kind: PeerAuthentication +metadata: + name: permissive-strict-mtls +spec: + selector: + matchLabels: + app: a + mtls: + mode: PERMISSIVE + portLevelMtls: + 9090: + mode: STRICT + 8080: + mode: PERMISSIVE diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-port-mtls-strict-and-permissive.yaml b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-port-mtls-strict-and-permissive.yaml new file mode 100644 index 000000000000..b0e419ee4966 --- /dev/null +++ b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-port-mtls-strict-and-permissive.yaml @@ -0,0 +1,10 @@ +action: DENY +groups: +- rules: + - matches: + - destinationPorts: + - 9090 + notPrincipals: + - presence: {} +name: converted_peer_authentication_permissive-strict-mtls +scope: WORKLOAD_SELECTOR diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-permissive-namespace-strict-workload-ports-in.yaml b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-permissive-namespace-strict-workload-ports-in.yaml new file mode 100644 index 000000000000..66573a10c75d --- /dev/null +++ b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-permissive-namespace-strict-workload-ports-in.yaml @@ -0,0 +1,32 @@ +apiVersion: security.istio.io/v1 +kind: PeerAuthentication +metadata: + name: default-mesh + namespace: istio-system +spec: + mtls: + mode: PERMISSIVE +--- +apiVersion: security.istio.io/v1 +kind: PeerAuthentication +metadata: + name: default-foo + namespace: foo +spec: + mtls: + mode: PERMISSIVE +--- +apiVersion: security.istio.io/v1 +kind: PeerAuthentication +metadata: + name: workload + namespace: foo +spec: + selector: + matchLabels: + app: a + portLevelMtls: + 9090: + mode: STRICT + 8080: + mode: STRICT diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-permissive-namespace-strict-workload-ports.yaml b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-permissive-namespace-strict-workload-ports.yaml new file mode 100644 index 000000000000..2994070278f9 --- /dev/null +++ b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-permissive-namespace-strict-workload-ports.yaml @@ -0,0 +1,17 @@ +action: DENY +groups: +- rules: + - matches: + - destinationPorts: + - 8080 + notPrincipals: + - presence: {} +- rules: + - matches: + - destinationPorts: + - 9090 + notPrincipals: + - presence: {} +name: converted_peer_authentication_workload +namespace: foo +scope: WORKLOAD_SELECTOR diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-unset-namespace-mixed-workload-ports-in.yaml b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-unset-namespace-mixed-workload-ports-in.yaml new file mode 100644 index 000000000000..2ebae100e2c7 --- /dev/null +++ b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-unset-namespace-mixed-workload-ports-in.yaml @@ -0,0 +1,32 @@ +apiVersion: security.istio.io/v1 +kind: PeerAuthentication +metadata: + name: default-mesh + namespace: istio-system +spec: + mtls: + mode: PERMISSIVE +--- +apiVersion: security.istio.io/v1 +kind: PeerAuthentication +metadata: + name: default-foo + namespace: foo +spec: + mtls: + mode: UNSET +--- +apiVersion: security.istio.io/v1 +kind: PeerAuthentication +metadata: + name: workload + namespace: foo +spec: + selector: + matchLabels: + app: a + portLevelMtls: + 9090: + mode: STRICT + 8080: + mode: PERMISSIVE diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-unset-namespace-mixed-workload-ports.yaml b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-unset-namespace-mixed-workload-ports.yaml new file mode 100644 index 000000000000..e6e3be4b97fb --- /dev/null +++ b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-permissive-root-unset-namespace-mixed-workload-ports.yaml @@ -0,0 +1,11 @@ +action: DENY +groups: +- rules: + - matches: + - destinationPorts: + - 9090 + notPrincipals: + - presence: {} +name: converted_peer_authentication_workload +namespace: foo +scope: WORKLOAD_SELECTOR diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-port-mtls-strict-and-permissive-in.yaml b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-port-mtls-strict-and-permissive-in.yaml new file mode 100644 index 000000000000..59b82a9a1c9d --- /dev/null +++ b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-port-mtls-strict-and-permissive-in.yaml @@ -0,0 +1,15 @@ +apiVersion: security.istio.io/v1 +kind: PeerAuthentication +metadata: + name: permissive-strict-mtls +spec: + selector: + matchLabels: + app: a + mtls: + mode: STRICT + portLevelMtls: + 9090: + mode: STRICT + 8080: + mode: PERMISSIVE diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-port-mtls-strict-and-permissive.yaml b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-port-mtls-strict-and-permissive.yaml new file mode 100644 index 000000000000..81b511a95dca --- /dev/null +++ b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-port-mtls-strict-and-permissive.yaml @@ -0,0 +1,11 @@ +action: DENY +groups: +- rules: + - matches: + - notPrincipals: + - presence: {} + - matches: + - notDestinationPorts: + - 8080 +name: converted_peer_authentication_permissive-strict-mtls +scope: WORKLOAD_SELECTOR diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-root-unset-workload-port-mtls-strict-and-permissive-in.yaml b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-root-unset-workload-port-mtls-strict-and-permissive-in.yaml new file mode 100644 index 000000000000..d4d2533fce6a --- /dev/null +++ b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-root-unset-workload-port-mtls-strict-and-permissive-in.yaml @@ -0,0 +1,23 @@ +apiVersion: security.istio.io/v1 +kind: PeerAuthentication +metadata: + name: default-mesh + namespace: istio-system +spec: + mtls: + mode: STRICT +--- +apiVersion: security.istio.io/v1 +kind: PeerAuthentication +metadata: + name: workload + namespace: foo +spec: + selector: + matchLabels: + app: a + portLevelMtls: + 9090: + mode: PERMISSIVE + 8080: + mode: STRICT diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-root-unset-workload-port-mtls-strict-and-permissive.yaml b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-root-unset-workload-port-mtls-strict-and-permissive.yaml new file mode 100644 index 000000000000..7abcb27893a7 --- /dev/null +++ b/pilot/pkg/serviceregistry/kube/controller/ambient/testdata/peer-authn-strict-root-unset-workload-port-mtls-strict-and-permissive.yaml @@ -0,0 +1,12 @@ +action: DENY +groups: +- rules: + - matches: + - notDestinationPorts: + - 9090 + - matches: + - notPrincipals: + - presence: {} +name: converted_peer_authentication_workload +namespace: foo +scope: WORKLOAD_SELECTOR diff --git a/releasenotes/notes/54146.yaml b/releasenotes/notes/54146.yaml new file mode 100644 index 000000000000..0d8c665b28af --- /dev/null +++ b/releasenotes/notes/54146.yaml @@ -0,0 +1,9 @@ +apiVersion: release-notes/v2 +kind: bug-fix +area: security +issue: + - 54146 +releaseNotes: +- | + **Fixed** a bug in Ambient (only) where multiple STRICT port-level mTLS rules in a PeerAuthentication policy would effectively result + in a permissive policy due to incorrect evaluation logic (AND vs. OR). diff --git a/tests/integration/ambient/baseline_test.go b/tests/integration/ambient/baseline_test.go index d7893fe01991..348350f1cb4b 100644 --- a/tests/integration/ambient/baseline_test.go +++ b/tests/integration/ambient/baseline_test.go @@ -751,9 +751,6 @@ spec: http: maxRequestsPerConnection: 1`).ApplyOrFail(t) runTestContext(t, func(t framework.TestContext, src echo.Instance, dst echo.Instance, opt echo.CallOptions) { - if opt.Scheme != scheme.TCP { - return - } // Ensure we don't get stuck on old connections with old RBAC rules. This causes 45s test times // due to draining. opt.NewConnectionPerRequest = true @@ -864,6 +861,8 @@ spec: app: "{{ .Destination }}" portLevelMtls: 18080: + mode: PERMISSIVE + 19090: mode: PERMISSIVE `).ApplyOrFail(t) opt = opt.DeepCopy() @@ -876,7 +875,7 @@ spec: apiVersion: security.istio.io/v1 kind: PeerAuthentication metadata: - name: global-strict + name: global-permissive spec: mtls: mode: PERMISSIVE @@ -896,6 +895,8 @@ spec: app: "{{ .Destination }}" portLevelMtls: 18080: + mode: STRICT + 19090: mode: STRICT `).ApplyOrFail(t) opt = opt.DeepCopy() From 6abd1e5a6bb77b5b4e020f18b99bcc2e89cef305 Mon Sep 17 00:00:00 2001 From: Istio Automation Date: Tue, 14 Jan 2025 23:37:52 -0500 Subject: [PATCH 6/6] [release-1.22] make accesslog order stable (#54681) * make accesslog order stable * release notes --------- Co-authored-by: zirain --- pilot/pkg/model/telemetry.go | 5 +++++ pilot/pkg/model/telemetry_logging_test.go | 2 -- releasenotes/notes/54680.yaml | 8 ++++++++ 3 files changed, 13 insertions(+), 2 deletions(-) create mode 100644 releasenotes/notes/54680.yaml diff --git a/pilot/pkg/model/telemetry.go b/pilot/pkg/model/telemetry.go index 1891fd6cd061..b0ec71269253 100644 --- a/pilot/pkg/model/telemetry.go +++ b/pilot/pkg/model/telemetry.go @@ -284,6 +284,11 @@ func (t *Telemetries) AccessLogging(push *PushContext, proxy *Proxy, class netwo cfgs = append(cfgs, cfg) } + // Sort the access logs by provider name for deterministic ordering + sort.Slice(cfgs, func(i, j int) bool { + return cfgs[i].Provider.Name < cfgs[j].Provider.Name + }) + t.computedLoggingConfig[key] = cfgs return cfgs } diff --git a/pilot/pkg/model/telemetry_logging_test.go b/pilot/pkg/model/telemetry_logging_test.go index 9714638b22cf..95189ad51cc4 100644 --- a/pilot/pkg/model/telemetry_logging_test.go +++ b/pilot/pkg/model/telemetry_logging_test.go @@ -16,7 +16,6 @@ package model import ( "reflect" - "sort" "testing" accesslog "github.com/envoyproxy/go-control-plane/envoy/config/accesslog/v3" @@ -699,7 +698,6 @@ func TestAccessLogging(t *testing.T) { } got = append(got, p.Provider.Name) } - sort.Strings(got) } if !reflect.DeepEqual(got, tt.want) { t.Fatalf("got %v want %v", got, tt.want) diff --git a/releasenotes/notes/54680.yaml b/releasenotes/notes/54680.yaml new file mode 100644 index 000000000000..45f3d7f0a356 --- /dev/null +++ b/releasenotes/notes/54680.yaml @@ -0,0 +1,8 @@ +apiVersion: release-notes/v2 +kind: bug-fix +area: telemetry +issue: + - 54672 +releaseNotes: + - | + **Fixed** an issue that access log order instability causing connection draining.