Apply peer authentication policy by diemtvu · Pull Request #20829 · istio/istio

diemtvu · 2020-02-04T00:58:06Z

Implement logic to select peer authentication policy for workload: most specific scope, then most secure mode (i.e STRICT > PERMISSIVE > DISABLE).
Implement logic to apply peer authentication policy at workload level and above (namespace, mesh), and fallback to (alpha) authentication policy.
Port-level support and better auto mTLS will be taken care in the following up PRs.

Issue: #20746

incfly

i have two high level questions.

what's the rough idea of supporting auto mTLS? wire up the policy resolution with EDS code? every policy change is a full push to ensure the correctness?

when no port mtls is specified, what's the behavior, secure everything including pass through listeners? when port specified, only per port listener is secured? what if the port defined in the policy is not part of the existing workload's inbound listener, aka, :? is that considered as mis-configuration?

pilot/pkg/security/authn/v1beta1/policy_applier.go

incfly · 2020-02-04T15:48:27Z

pilot/pkg/security/authn/v1beta1/policy_applier.go

is this a valid use case? label selector && defined in root ns? just want to confirm.

any prior usage of this pattern? req authn || authz?

For the first question, yes (for now), as we don't have the way to prevent selector used in root namespace yet (need some change in validation webhook to know what is the root namespace, and it may not be useful given root namespace can be changed).

For the second, both request authn and authz are appendable, so they don't need this logic to disambiguation.

This is not a valid use case. WE dont respect workload specific config in the root namespace for sidecar or envoy filter. So lets follow the pattern for consistency and not create exceptions

Do you mean we can ignore them? (but not reject at validation time)

pilot/pkg/security/authn/v1beta1/policy_applier.go

tests/integration/security/testdata/beta-global-mtls-off.yaml

diemtvu · 2020-02-04T20:50:05Z

i have two high level questions.

what's the rough idea of supporting auto mTLS? wire up the policy resolution with EDS code? every policy change is a full push to ensure the correctness?

Right. May be we can change BuildLbEndpointMetadata to take into account the policy for that given workload.

when no port mtls is specified, what's the behavior, secure everything including pass through listeners? when port specified, only per port listener is secured? what if the port defined in the policy is not part of the existing workload's inbound listener, aka, :? is that considered as mis-configuration?

This is 3 questions :), so yes, yes (assuming the parent level is set to disable) and depends.

For the last one, I think we can either ignore it (base on assumption traffic to that port will get 503 anyway), or still create an inbound listener for that port with those setting. Not sure which one is the right answer yet, though I'm a bit leaning forward the later.

diemtvu · 2020-02-05T00:18:09Z

/test integ-security-k8s-tests_istio
/test integ-distroless-k8s-tests_istio

… take into account the policy

…remove old mesh policy during installation

hzxuzhonghu · 2020-02-05T03:04:29Z

pilot/pkg/networking/core/v1alpha3/cluster.go

would prefer abstracting to a fuction

Lets please not use the old logic. This is not only complex but also error prone. All we need is a simple function in the push context PeerAuthenticationPolicyForNamespace(ns). This will return either the global one or the namespace local one. If there is no global one, we assume permissive, just like the way we assume a default sidecar. We can reuse logic similar to the sidecar/envoy filter etc.

And we should NOT check the workload specific authN policy - the cluster can belong to a subset or to an entire service. As we agreed previously, if the user is sophisticated enough to use a per workload policy to disable peer authn, then she should also define destination rules, etc. to create the exception. This model will keep the implementation simple and robust, and most importantly scalable! We cannot be doing label based search for every possible service, for every sidecar in the system.

I agree. I implemented it this way so that we have something in case we don't have time to work on the better option. Anyway, I revert this now, and we will have a separate PR tackle the auto-mtls alone.

rshriram

Please change the logic in cluster.go. We cannot be querying by service labels and we cannot be computing the appropriate peer authn for every workload. All of this needs to be pre-computed per namespace and we should only be doing lookups. The first fix should be in push context, where you initialize peer authN. Also, we need to have a built in default peer authN if there is no global peer authN. This will ensure that all namespaces will always inherit the global one.

rshriram · 2020-02-05T16:57:36Z

pilot/pkg/networking/core/v1alpha3/cluster.go

Lets please not use the old logic. This is not only complex but also error prone. All we need is a simple function in the push context PeerAuthenticationPolicyForNamespace(ns). This will return either the global one or the namespace local one. If there is no global one, we assume permissive, just like the way we assume a default sidecar. We can reuse logic similar to the sidecar/envoy filter etc.

And we should NOT check the workload specific authN policy - the cluster can belong to a subset or to an entire service. As we agreed previously, if the user is sophisticated enough to use a per workload policy to disable peer authn, then she should also define destination rules, etc. to create the exception. This model will keep the implementation simple and robust, and most importantly scalable! We cannot be doing label based search for every possible service, for every sidecar in the system.

pilot/pkg/security/authn/factory/factory.go

rshriram · 2020-02-05T17:04:46Z

pilot/pkg/security/authn/v1beta1/policy_applier.go

+		return model.MTLSPermissive
+	}
+	switch peer.Mtls.Mode {
+	case v1beta1.PeerAuthentication_MutualTLS_DISABLE:


and what about the unset thing?

Good point. Now it will be more complicated :)

It is actually simple if you use the same logic as the Sidecar

rshriram · 2020-02-05T17:05:46Z

pilot/pkg/security/authn/v1beta1/policy_applier.go

This is not a valid use case. WE dont respect workload specific config in the root namespace for sidecar or envoy filter. So lets follow the pattern for consistency and not create exceptions

rshriram · 2020-02-05T17:14:30Z

pilot/pkg/security/authn/v1beta1/policy_applier.go

+			level += 2
+		}
+		if isStrictlyStronger(spec, configByScope[level]) {
+			configByScope[level] = cfg


This logic is memory intensive, allocating an array and destroying it for each query for each proxy in the system. It will also undergo a lot of changes when you introduce workload specific sidecars. To me, a better way is to reuse the logic that builds https://github.com/istio/istio/blob/master/pilot/pkg/model/push_context.go#L95 sidecarsByNamespace . And then as commented above, you simply have to query push context for the namespace's peer authN policy. Otherwise, this will become a performance bottleneck once again.

We already use that. This is a second round to pick from the short list (the input list is a list of config that match the workload) based on our precedent order rules.

You dont need the list of configs - that is my point. Take a look at the code in push context. It simply scans through a hash map and picks the appropriate config without allocating more memory. If there is no namespace level config, it automatically falls back to the global config which always exists even if the user did not define a global setting.

There are few things that the approach with sidecar doesn't work here:

We want to take into account the 'strength' of mTLS mode to break the tie.

We need to 'inherit' from parent for UNSET mode (thanks to your other comment :) )

I've updated this PR to handle UNSET, but I'm still working on simplify it a bit more, using the assumption that there is at most one namespace & mesh level policy (we already enforce that from validation), and also ignore policies in root namespace but have selector (as you suggested)

This is the pointer of array, []*model.Config, not the []model.Config, will that still be mem intensive concern?

I re-implement this function to take into account UNSET (which should be inherit from parent), and port-level settings (which need to be aggregated to the final policy, including resolve for UNSET). It seems more complicated now, unfortunately, though I think it has the correct behavior.

PTAL

…ch also consolidate port-level policies.

incfly · 2020-02-06T18:20:34Z

pilot/pkg/security/authn/v1beta1/policy_applier.go

+	// Review ports with UNSET mode and update the final workload level mTLS if it is stronger.
+	for port := range unsetPorts {
+		existing, exist := finalPolicy.PortLevelMtls[port]
+		if !exist || getMutualTLSMode(finalPolicy.Mtls) > getMutualTLSMode(existing) {


same here. might require changing the signature of isStritlyStrong to pass MutualTls rather than policy

pilot/pkg/security/authn/v1beta1/policy_applier.go

incfly · 2020-02-06T18:31:28Z

tests/integration/security/reachability_test.go

 					},
 				},
+				{
+					ConfigFile:          "beta-mtls-on.yaml",


would it help to have test cases where we have both beta and alpha policy? that can be a typical case during migration.

and what's the intended behavior? to me, it seems like if there's one beta policy instance (ns/mesh/workload) can be applied to a particular workload, all alpha policy is ignored, is that correct?

feel free to do it in follow-up PR and leave a TODO.

It's already there.

which one? I didn't see it in the e2e test. all config file only references the beta policy.

rshriram · 2020-02-06T18:40:20Z

pilot/pkg/security/authn/v1beta1/policy_applier.go

+					finalPolicy.PortLevelMtls[port] = mtls
+				}
+			}
+		}


You have to break out of the main for loop once you get a workload specific policy. Thsi will be the oldest policy. We cannot pick the strongest as it has the potential to break workloads that dont even belong to the team that wrote this policy.

incfly · 2020-02-06T19:38:42Z

pilot/pkg/security/authn/v1beta1/policy_applier.go

-		processedJwtRules: processedJwtRules,
-		alphaApplier:      alpha_applier.NewPolicyApplier(policy),
+		jwtPolicies:            jwtPolicies,
+		peerPolices:            peerPolicies,


do you need peerpolicies? seems to me you only need to store consolidatePeerPolicy in the applier.

diemtvu · 2020-02-06T19:39:06Z

Oh. I mean unit test. I'll add one later for e2e in different PR.

…

On Thu, Feb 6, 2020 at 11:30 AM Jianfei Hu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In tests/integration/security/reachability_test.go <#20829 (comment)>: > @@ -91,6 +91,45 @@ func TestReachability(t *testing.T) { return opts.PortName != "http" }, }, + { + ConfigFile: "beta-mtls-on.yaml", which one? I didn't see it in the e2e test. all config file only references the beta policy. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20829?email_source=notifications&email_token=AF7X24PDDSU4TU5ZS2QJAG3RBRQM3A5CNFSM4KPOV2LKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCUSIBEA#discussion_r376037061>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF7X24NNCFEDFPEXLXER7QDRBRQM3ANCNFSM4KPOV2LA> .

-- Diem Vu | Software Engineer | diemvu@google.com | +1 408-215-8127

diemtvu · 2020-02-06T19:40:49Z

I think we may need this for an analyzer tool.

…

On Thu, Feb 6, 2020 at 11:38 AM Jianfei Hu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pilot/pkg/security/authn/v1beta1/policy_applier.go <#20829 (comment)>: > @@ -146,9 +169,12 @@ func NewPolicyApplier(jwtPolicies []*model.Config, policy *authn_alpha_api.Polic }) return &v1beta1PolicyApplier{ - jwtPolicies: jwtPolicies, - processedJwtRules: processedJwtRules, - alphaApplier: alpha_applier.NewPolicyApplier(policy), + jwtPolicies: jwtPolicies, + peerPolices: peerPolicies, do you need peerpolicies? seems to me you only need to store consolidatePeerPolicy in the applier. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20829?email_source=notifications&email_token=AF7X24JO7W7C2D74MXJN3U3RBRRMJA5CNFSM4KPOV2LKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCUSJKXI#pullrequestreview-354719069>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF7X24JVQA76A6OIUWUOMWTRBRRMJANCNFSM4KPOV2LA> .

-- Diem Vu | Software Engineer | diemvu@google.com | +1 408-215-8127

incfly · 2020-02-06T19:43:38Z

pilot/pkg/security/authn/v1beta1/policy_applier_test.go

+			},
+			want: nil,
+		},
+		{


there's a less common case: define port level settings in mesh/namespace policy, and see how that's inherited && combined with workload level policy. shall we add case for that?

no. It can be rejected by validation.

incfly

i left one more question on the behavior of port level settings defined in the mesh/ns policy. approving now.

incfly · 2020-02-06T23:18:24Z

pilot/pkg/security/authn/v1beta1/policy_applier.go

 func composePeerAuthentication(rootNamespace string, configs []*model.Config) *v1beta1.PeerAuthentication {
-	var meshPolicy, namespacePolicy *v1beta1.PeerAuthentication
+	var meshPolicy, namespacePolicy, workloadPolicy *v1beta1.PeerAuthentication
+	// Creation time associate with the selected workloadPolicy above. Intiial to max time (not set)


initial, typo

incfly · 2020-02-06T23:21:42Z

pilot/pkg/security/authn/v1beta1/policy_applier.go

+	if namespacePolicy != nil && !isMtlsModeUnset(namespacePolicy.Mtls) {
+		// If namespace policy is defined, update output policy to namespace policy. This means namespace
+		// policy overwrite mesh policy.
+		outputPolicy.Mtls = namespacePolicy.Mtls


wait, are we abandoning inherit stronger from namespace/mesh policy as well? i thought we only remove inherit for workload level policy.

No, it's never before. There is at most 1 namespace level, so no conflict. Same as mesh. Narrower scope always win.

incfly · 2020-02-06T23:22:17Z

pilot/pkg/security/authn/v1beta1/policy_applier.go

@@ -343,111 +344,71 @@ func getMutualTLSMode(mtls *v1beta1.PeerAuthentication_MutualTLS) model.MutualTL
 // replaced with config from workload-level, UNSET in workload-level config will be replaced with


i didn't see these section updates. i expect there' will be some.

Why this is need to be updated? It is just a conversion from user facing enum to our internal implementation so we can maintain the same semantic between different API version.

actually i mean line 341.

if there are more than 1 policy define
// port-level mTLS for the same port, the stronger one is used.

this is no longer true. we use the entire workload policy ordered by timestamp now.

incfly

istio/api doc shall have some updates as well i assume?

diemtvu · 2020-02-06T23:36:12Z

istio/api doc shall have some updates as well i assume?

We didn't say much about conflict resolution in the API :).

incfly

let's ship it!

diemtvu · 2020-02-07T00:19:46Z

/test e2e-mixer-no_auth_istio
/test /test integ-mixer-k8s-tests_istio

istio-testing · 2020-02-07T01:42:21Z

In response to a cherrypick label: #20829 failed to apply on top of branch "release-1.5":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	pilot/pkg/security/authn/policy_applier.go
M	pilot/pkg/security/authn/v1alpha1/policy_applier.go
M	pilot/pkg/security/authn/v1beta1/policy_applier.go
M	pilot/pkg/security/authn/v1beta1/policy_applier_test.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/security/authn/v1beta1/policy_applier_test.go
CONFLICT (content): Merge conflict in pilot/pkg/security/authn/v1beta1/policy_applier_test.go
Auto-merging pilot/pkg/security/authn/v1beta1/policy_applier.go
CONFLICT (content): Merge conflict in pilot/pkg/security/authn/v1beta1/policy_applier.go
Auto-merging pilot/pkg/security/authn/v1alpha1/policy_applier.go
CONFLICT (content): Merge conflict in pilot/pkg/security/authn/v1alpha1/policy_applier.go
Auto-merging pilot/pkg/security/authn/policy_applier.go
Patch failed at 0001 Apply beta peer authentication policy down to workload level

* Apply beta peer authentication policy down to workload level * Clean up * Lint * Check beta policy for auto mtls. This can be removed when EP metadata take into account the policy * Use explicit peerauthentication policy for permissive, as we haven't remove old mesh policy during installation * pilot/pkg/security/authn/v1beta1/policy_applier.go * Move all test for beta mTLS api to the end * Change to namespace policy * Revert cluster.go * Change peer authn consolidation algorithm for UNSET (inheritant mode) * Reimplement getMostSpecificConfig (now composePeerAuthentication) which also consolidate port-level policies. * Fix inheritance: do not inherit if it is weaker than the current mode * Remove debug logs * Change test policy to namespace level to make sure they are clean up properly with the existing test setup. * Address comment * Lint * Simplify logic to pick the oldest * fix typo * Update function comment

diemtvu requested a review from incfly February 4, 2020 00:58

diemtvu requested a review from a team as a code owner February 4, 2020 00:58

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Feb 4, 2020

istio-policy-bot added the area/security label Feb 4, 2020

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Feb 4, 2020

istio-testing added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 4, 2020

incfly reviewed Feb 4, 2020

View reviewed changes

diemtvu force-pushed the mtls-beta-p3 branch from 9e9227d to ddf5e50 Compare February 4, 2020 20:01

diemtvu requested a review from a team as a code owner February 4, 2020 20:01

diemtvu force-pushed the mtls-beta-p3 branch from ddf5e50 to a00790d Compare February 4, 2020 20:59

diemtvu changed the title ~~[WIP] Apply peer authentication policy~~ Apply peer authentication policy Feb 4, 2020

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Feb 4, 2020

diemtvu added 6 commits February 4, 2020 17:35

Apply beta peer authentication policy down to workload level

9f8326b

Clean up

18c676a

Lint

294aaf1

Check beta policy for auto mtls. This can be removed when EP metadata…

30e2338

… take into account the policy

Use explicit peerauthentication policy for permissive, as we haven't …

f1bd99d

…remove old mesh policy during installation

pilot/pkg/security/authn/v1beta1/policy_applier.go

6f05f9b

hzxuzhonghu reviewed Feb 5, 2020

View reviewed changes

Move all test for beta mTLS api to the end

6bf0db2

diemtvu force-pushed the mtls-beta-p3 branch from b5ac8ae to 6bf0db2 Compare February 5, 2020 04:28

Change to namespace policy

4a54c1d

rshriram suggested changes Feb 5, 2020

View reviewed changes

diemtvu added 3 commits February 5, 2020 10:10

Revert cluster.go

b8dec07

Change peer authn consolidation algorithm for UNSET (inheritant mode)

52a3ea2

Reimplement getMostSpecificConfig (now composePeerAuthentication) whi…

9c25073

…ch also consolidate port-level policies.

incfly mentioned this pull request Feb 5, 2020

Auto mTLS for beta peer authn policy. #20881

Closed

Fix inheritance: do not inherit if it is weaker than the current mode

67d38f5

incfly reviewed Feb 6, 2020

View reviewed changes

pilot/pkg/security/authn/v1beta1/policy_applier.go Outdated Show resolved Hide resolved

incfly reviewed Feb 6, 2020

View reviewed changes

rshriram reviewed Feb 6, 2020

View reviewed changes

Address comment

7d891ed

incfly reviewed Feb 6, 2020

View reviewed changes

Lint

c1bd838

incfly approved these changes Feb 6, 2020

View reviewed changes

Simplify logic to pick the oldest

2ca5e48

incfly reviewed Feb 6, 2020

View reviewed changes

diemtvu added 2 commits February 6, 2020 15:37

fix typo

f7110a3

Update function comment

0f10e7f

incfly approved these changes Feb 6, 2020

View reviewed changes

istio-testing merged commit 9053f47 into istio:master Feb 7, 2020

diemtvu deleted the mtls-beta-p3 branch February 10, 2020 21:48

shamsher31 mentioned this pull request May 15, 2020

The same documentation URL for AuthenticationPolicy (namespace-wide) display two resource type different depending on the localisation istio/istio.io#7316

Closed

		@@ -343,111 +344,71 @@ func getMutualTLSMode(mtls *v1beta1.PeerAuthentication_MutualTLS) model.MutualTL
		// replaced with config from workload-level, UNSET in workload-level config will be replaced with

Conversation

diemtvu commented Feb 4, 2020 • edited by istio-policy-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

incfly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

diemtvu commented Feb 4, 2020

Uh oh!

diemtvu commented Feb 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rshriram left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diemtvu commented Feb 6, 2020 via email

Uh oh!

diemtvu commented Feb 6, 2020 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

diemtvu commented Feb 4, 2020 •

edited by istio-policy-bot

Loading