Peer authentication beta API, aka mTLS beta #1241

diemtvu · 2020-01-22T01:02:20Z

No description provided.

rshriram · 2020-01-24T03:44:15Z

@diemtvu take a look at my PR for sidecar TLS termination (one way tls) #1248 . I think this API can be nicely folded into the Sidecar api so that the entire TLS thing is coherent. We can relax certain semantics of the sidecars (like mandatory egress, etc.) to accommodate the tls policy

rshriram · 2020-01-29T02:17:21Z

security/v1beta1/peer_authentication.proto

+  }
+
+  // Defines the mode of peer authentication.
+  Mode mode = 2;


you need to add per port override here as well. Currently we have a bug. if the authn policy is strict, it applies to all declared ingress ports on a sidecar even if the user wanted custom TLS on that ingress port, or no TLS for that matter.

You also need to surface the mechanism to identify the mesh mTLS using ALPNs or in case of @mdhume's use case, based on SNIs (wherein non sidecar clients are using istio mTLS certs to talk to servers with sidecar, but they cant send the ALPNs).

I'd like to postpone sni/alpn later, after we delivery the essential features. I've add MutualTls wrapper message, so that we can add configuration for them later.

For port override, I add a map from port (number) to MutualTls. I am a bit hesitant on this too, and would like to hide it first until we are sure it works well in all aspect. Wdyt?

security/v1beta1/peer_authentication.proto

costinm · 2020-01-29T23:01:18Z

security/v1beta1/peer_authentication.proto

+      PERMISSIVE = 0;
+
+      // Connection is an mTLS tunnel (TLS with client cert must be presented).
+      STRICT = 1;


Strict - and all access checks that are required on a connection - should be in the authorization API.

I think we have consensus that this is the place to have it - and we have time to implement it there (it should be the same effort as implementing this new API ).

The consensus actually is this API. Please check with @smawson and @louiscryan

costinm · 2020-01-29T23:01:50Z

security/v1beta1/peer_authentication.proto

+  message MutualTLS {
+    enum Mode {
+      // Connection can be either plaintext or mTLS tunnel.
+      PERMISSIVE = 0;


I don't think we want to continue to expose permissive as a user preference. AutoMTLS removes the need for permissive to be exposed and managed by user.

autoMtls solves different problem. We need this API to do what the existing alpha API doing today.

Lets call it as UNSET. We can then fallback to permissive internally if needed. But if we are going ot have permissive as a dedicated option, I certainly do not want this to be the default enum. The default enum in that case should be unset=0, as it allows for proper inheritance.

I can add UNSET, but want to use it for validation to make sure the mode must be set explicitly instead. We still need permissive, otherwise, overwrite policy for namespace/workload to permissive seems odd.

Inheritance can base on whether the message (MutualTLS) is set or not.

Wdyt?

costinm · 2020-01-29T23:03:12Z

security/v1beta1/peer_authentication.proto

+      STRICT = 1;
+
+      // Connection is not tunneled.
+      DISABLE = 2;


What does it mean ? We already have a mechanism to opt out a port from injection.

You mean we don't do auto-detection and other filters ? Plain text ? This is quite confusing for user, and should be part of Sidecar or networking APIs, since it doesn't apply to authentication ( there is no authentication on the plain text port).

It means mTLS is disable. I'm not sure what do you mean by the mechanism to opt out a port from injection and what does it have any thing to do in here.

costinm · 2020-01-29T23:50:49Z

Adding authorizations checks for the transport properties has been discussed and I don't remember anyone objecting to it. Adding a CRD and an API with just one setting doesn't make sense to me and I don't think any WG (security or networking) had discussed. At least the interactions to AutoMTLS and the direction of the networking should be covered in a networking WG or doc, Louis has an entire doc about better transport security, etc. Enforcing strict with a separate CRD instead of authorization - I think the Security WG can decide if it's the best option for the users. But exposing control over 'permissive' and non-mTLS settings: I don't think this is the right way to do it.

…

On Wed, Jan 29, 2020 at 3:17 PM Diem Vu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In security/v1beta1/peer_authentication.proto <#1241 (comment)>: > +// +genclient +// +k8s:deepcopy-gen=true +// --> +message PeerAuthentication { + // The selector determines the workloads to apply the ChannelAuthentication on. + // If not set, the policy will be applied to all workloads in the same namespace as the policy. + istio.type.v1beta1.WorkloadSelector selector = 1; + + // Mutual TLS settings. + message MutualTLS { + enum Mode { + // Connection can be either plaintext or mTLS tunnel. + PERMISSIVE = 0; + + // Connection is an mTLS tunnel (TLS with client cert must be presented). + STRICT = 1; The consensus actually is this API. Please check with @smawson <https://github.com/smawson> and @louiscryan <https://github.com/louiscryan> — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1241?email_source=notifications&email_token=AAAUR2XGGNRDEVDRCIHM2ZDRAIE7ZA5CNFSM4KJ553GKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCTSBLBA#discussion_r372684050>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2Q6OYI44FVCTDQT3E3RAIE7ZANCNFSM4KJ553GA> .

costinm · 2020-01-30T00:16:08Z

security/v1beta1/peer_authentication.proto

+message PeerAuthentication {
+  // The selector determines the workloads to apply the ChannelAuthentication on.
+  // If not set, the policy will be applied to all workloads in the same namespace as the policy.
+  istio.type.v1beta1.WorkloadSelector selector = 1;


Sorry, just realized: this will have very 'dramatic' impact on behavior, meaning that will select ports that are currently not selected - and break most of the stateful set.

We can't have users of the old API just switch to this API - and it's not even clear how it can work in general for those cases.

Do you mean the problem related to the pass-through inbound listener? I think we will need to secure all ports on workload with this API, and user may need to use permissive mode in those cases. It's better than today behavior, when users might not realize that their workloads are not protected and traffic is actually in plain-text.

security/v1beta1/peer_authentication.proto

rshriram · 2020-01-30T14:17:53Z

I am 80% okay with costin's proposal of moving the "enforce mesh security" mode to the authZ api (with the ability to add exceptions per port). We can define inheritance mechanisms for this setting alone in the authZ api. One less CRD to contend with. And paves the way for incremental upgrade to the new model.

How we do the mesh security is our internal implementation detail (mtls, proxy protocol plus some other wrapper, etc.). But from user point of view, it is a 3 way set: mesh-transport is UNSET, REQUIRED, DISABLED. Internally, we can assume unset == permissive at global level, and at individual namespace/workload level, it can inherit from the parent.

diemtvu · 2020-01-30T16:01:07Z

I also agree that authZ can address 80% of the need. But the remain 20% (e.g disable mTLS, the need for sni/alpn config) is the reason I think we still need this. No matter if they are edge cases or not, if we need to invent a new way to do so, the cost is the same, if not more.

It's true that when we have better security transport, more intuitive authZ or networking API, this one can be diminished. But it could easily be a few quarters from now, and I don't see why we have to stop this.

rshriram · 2020-01-30T18:06:20Z

security/v1beta1/peer_authentication.proto

+  // Mutual TLS settings.
+  message MutualTLS {
+    enum Mode {
+      INVALID = 0;


can you change this to UNSET ? Because this is an inherited field. So invalid doesn't make sense

I actually don't want inherit in enum, but inherit if the message type field mtls is not set. Using INVALID to require user to explicitly set it.

With proto3 we can't distinguish unset from set to 0, so the default would be INVALID, which doesn't make sense.

With port settings people can leave this unset just to change a port setting without changing the default.

So rename to UNSET seems fine.

I mean, if users want to inherit, they should not define mtls at all, e.g

selector: match: app: foo portLevelMtls: 8080: mode: DISABLE

Instead of

selector: match: app: foo mtls: {} portLevelMtls: 8080: mode: DISABLE

The INVALID is not accepted value (i.e we will reject with validation), which force the yaml must explicitly set the mode, so mtls: {} is not legit.

Anyway, I'm ok to change to UNSET if you think it's better to treat this as inherit.

FYI, I add above example to the API comment. Let me know if it make sense. I happy to change to UNSET otherwise.

Just change it to UNSET.

Also is there a reason other than distinguishing between UNSET and missing that we're nesting mode inside MutualTLS message? Could we just inline that and just have the enum?

Done.
The reason for MutualTLS message is for future extension. Not sure if we will need any, but I don't want to get into a dead end.

OK in that case UNSET is definitely right, because if you extend it we wouldn't be able to distinguish if they leave that unset but set some other field in MutualTLS. So LGTM.

nrjpoddar · 2020-01-30T18:17:15Z

It looks this PR and #1248 have overlapping concerns. Additionally, these are big API changes which are pretty close to release-1.5, can we ratify them after the release?

smawson · 2020-01-30T21:58:44Z

We'd like to get these changes into 1.5 and handle #1248 after, since this is really just moving authentication policy to Beta. This is all likely to change with the introduction of improved transport security, but in the meantime users need a beta-level way to control the behavior of the mTLS code.

@rshriram can you give this another look?

liminw · 2020-01-31T00:50:09Z

security/v1beta1/peer_authentication.proto

@@ -0,0 +1,151 @@
+// Copyright 2019 Istio Authors


liminw · 2020-01-31T00:59:42Z

security/v1beta1/peer_authentication.proto

+  // Mutual TLS settings.
+  message MutualTLS {
+    enum Mode {
+      // Inherit from parent.


Can we document the default value if this is mesh level setting (no parent in this case)?

liminw · 2020-01-31T01:00:37Z

security/v1beta1/peer_authentication.proto

+
+// PeerAuthentication defines how traffic will be tunneled (or not) to the sidecar.
+//
+// Examples:


Probably worth to give a mesh level policy example (i.e., use root namespace).

liminw

/lgtm

incfly · 2020-01-31T01:57:01Z

security/v1beta1/peer_authentication.proto

+//   namespace: foo
+// spec:
+//   selector:
+//     matchLabels:


doe this mean we need to do additional look up for auto mTLS? could that be done efficiently?

rshriram · 2020-01-31T16:22:20Z

We'd like to get these changes into 1.5 and handle #1248 after, since this is really just moving authentication policy to Beta. This is all likely to change with the introduction of improved transport security, but in the meantime users need a beta-level way to control the behavior of the mTLS code.

I spoke with @diemtvu offline. To make progress, I am okay with this API. I will trim the sidecar API to simply specify TLS settings per port for ports that have mTLS disabled in this API - as that is useful in VM context.

costinm · 2020-01-31T18:17:34Z

My primary requirement is to not include it by default in istio-system. If you absolutely can't have the default in mesh config and must have the extra complexity of inheritance ( on top of migration/interop with prev version) - at least don't force-install this new CR in istio-config. User may set it, like any other config - and I assume istioctl and tests will exist for cases of old version still installed and/or for migration.

…

On Fri, Jan 31, 2020, 08:22 Shriram Rajagopalan ***@***.***> wrote: We'd like to get these changes into 1.5 and handle #1248 <#1248> after, since this is really just moving authentication policy to Beta. This is all likely to change with the introduction of improved transport security, but in the meantime users need a beta-level way to control the behavior of the mTLS code. I spoke with @diemtvu <https://github.com/diemtvu> offline. To make progress, I am okay with this API. I will trim the sidecar API to simply specify TLS settings per port for ports that have mTLS disabled in this API - as that is useful in VM context. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1241?email_source=notifications&email_token=AAAUR2X72FPAT4EQ3CXE47DRARF33A5CNFSM4KJ553GKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKPFOEA#issuecomment-580802320>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2SMMI3O4RJHBBOZAXLRARF33ANCNFSM4KJ553GA> .

costinm · 2020-01-31T18:20:05Z

And to be clear - safe upgrade has been a requirement since 1.0. And multiple control planes has been approved by env wg - so any install-time resource should be compatible or made compatible in 1.6.

…

On Fri, Jan 31, 2020, 10:17 Costin Manolache ***@***.***> wrote: My primary requirement is to not include it by default in istio-system. If you absolutely can't have the default in mesh config and must have the extra complexity of inheritance ( on top of migration/interop with prev version) - at least don't force-install this new CR in istio-config. User may set it, like any other config - and I assume istioctl and tests will exist for cases of old version still installed and/or for migration. On Fri, Jan 31, 2020, 08:22 Shriram Rajagopalan ***@***.***> wrote: > We'd like to get these changes into 1.5 and handle #1248 > <#1248> after, since this is really > just moving authentication policy to Beta. This is all likely to change > with the introduction of improved transport security, but in the meantime > users need a beta-level way to control the behavior of the mTLS code. > > I spoke with @diemtvu <https://github.com/diemtvu> offline. To make > progress, I am okay with this API. I will trim the sidecar API to simply > specify TLS settings per port for ports that have mTLS disabled in this API > - as that is useful in VM context. > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#1241?email_source=notifications&email_token=AAAUR2X72FPAT4EQ3CXE47DRARF33A5CNFSM4KJ553GKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKPFOEA#issuecomment-580802320>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAAUR2SMMI3O4RJHBBOZAXLRARF33ANCNFSM4KJ553GA> > . >

* Peer authentication beta API, aka mTLS beta * Add missing gen file * Update CRD cue and re-gen" * Add port level mtls * Udate proto.lock after rebase * Remove port level settings for now. We might add it later * Re-gen * Apply comment suggestions * Add port level mTLS settings * Add example for inherit parent settings * Rename invalid to unset * Rename invalid to unset * Address comments * Address comments

diemtvu requested review from dcberg, geeknoid, linsun, louiscryan, rshriram and smawson as code owners January 22, 2020 01:02

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jan 22, 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 Jan 22, 2020

istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 22, 2020

diemtvu force-pushed the mtls-beta branch from 7591e74 to 66d03f6 Compare January 23, 2020 02:32

rshriram reviewed Jan 29, 2020

View reviewed changes

diemtvu added 4 commits January 28, 2020 22:29

Peer authentication beta API, aka mTLS beta

fe55473

Add missing gen file

ed0cd15

Update CRD cue and re-gen"

383ee45

Add port level mtls

c975f9a

diemtvu force-pushed the mtls-beta branch from ceee178 to c975f9a Compare January 29, 2020 06:29

diemtvu added 2 commits January 28, 2020 22:30

Udate proto.lock after rebase

d936dfb

Remove port level settings for now. We might add it later

dc1542f

diemtvu changed the title ~~[WIP] Peer authentication beta API, aka mTLS beta~~ Peer authentication beta API, aka mTLS beta Jan 29, 2020

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

diemtvu assigned smawson and rshriram Jan 29, 2020

Re-gen

d0894b3

smawson approved these changes Jan 29, 2020

View reviewed changes

security/v1beta1/peer_authentication.proto Outdated Show resolved Hide resolved

Apply comment suggestions

fda91bc

costinm reviewed Jan 29, 2020

View reviewed changes

costinm reviewed Jan 30, 2020

View reviewed changes

rshriram reviewed Jan 30, 2020

View reviewed changes

security/v1beta1/peer_authentication.proto Show resolved Hide resolved

Add port level mTLS settings

0ba46ab

rshriram reviewed Jan 30, 2020

View reviewed changes

nrjpoddar mentioned this pull request Jan 30, 2020

custom TLS termination at sidecar #1248

Merged

diemtvu added 3 commits January 30, 2020 11:57

Add example for inherit parent settings

be3137a

Rename invalid to unset

5c34e14

Rename invalid to unset

61c6632

liminw reviewed Jan 31, 2020

View reviewed changes

diemtvu added 2 commits January 30, 2020 17:26

Address comments

bf13095

Address comments

252589e

liminw approved these changes Jan 31, 2020

View reviewed changes

istio-testing merged commit 659a010 into istio:master Jan 31, 2020

incfly reviewed Jan 31, 2020

View reviewed changes

diemtvu mentioned this pull request Jan 31, 2020

Tracking: Authentication policy v1beta1 istio/istio#18399

Closed

6 tasks

Peer authentication beta API, aka mTLS beta #1241

Peer authentication beta API, aka mTLS beta #1241

Conversation

diemtvu commented Jan 22, 2020

rshriram commented Jan 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costinm commented Jan 29, 2020 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rshriram commented Jan 30, 2020

diemtvu commented Jan 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrjpoddar commented Jan 30, 2020

smawson commented Jan 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liminw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rshriram commented Jan 31, 2020

costinm commented Jan 31, 2020 via email

costinm commented Jan 31, 2020 via email