Add revised filter config for proxy_wasm authn filter #1536

Shikugawa · 2020-07-18T03:50:46Z

This is highly related to istio/proxy#2800.

Shikugawa · 2020-07-18T03:51:06Z

Shikugawa · 2020-07-18T04:09:43Z

envoy/config/filter/http/authn/v2alpha2/config.proto

+  // Map from issuer to location of the payload that is emitted by Jwt filter.
+  // This information is added by pilot when construct and add Jwt and
+  // authN filters.
+  map<string, string> jwt_output_payload_locations = 3;


Should we place this on here?

lizan · 2020-07-22T05:39:23Z

@istio/wg-security-maintainers PTAL

yangminzhu · 2020-07-30T22:33:38Z

envoy/config/filter/http/authn/v2alpha2/config.proto

@@ -0,0 +1,50 @@
+// Copyright 2020 Istio Authors


Sorry for the late reply, this get lost in my inbox :(

A high level comment, should we move this API to the proxy repo instead of putting it in the istio/api again? This is internal filter API and should not be in the istio/api (user-facing) repo in the first place, we could take this chance to clean it up.

(this caused a lot confusions before, cc @costinm @incfly )

Yes, there is no benefit in having it in istio/api. You would need to manually generate pb.go files in istio/istio anyways and keep that in sync with proxy version.

yangminzhu · 2020-07-30T22:34:01Z

envoy/config/filter/http/authn/v2alpha2/config.proto

+  // trust domains.
+  // Note, the istio authn filter only validates the trust domain when mTLS is
+  // used, In other words, this field has no effect for plaintext traffic.
+  bool skip_validate_trust_domain = 4;


I think there is another change from @incfly for adding a list of strings for trust domain validation.

yangminzhu · 2020-07-30T22:38:04Z

envoy/config/filter/http/authn/v2alpha2/config.proto

+// authentication policy on Envoy.
+message FilterConfig {
+  // Filter configuration for RequestAuthentication, which supports Jwt authentication.
+  istio.security.v1beta1.RequestAuthentication request_authentication = 1;


is it possible to not to depend on istio/api? technically the internal filter config doesn't need to depend on the user-facing API, and the filter also probably have a simpler API?

just optional as we can always iterate on this part later.

It is needed by istio/istio, hopefully we don't need to import istio/proxy?

It would be very nice if the new authn filter doesn't depend on istio/api but has its own definition in istio/proxy. I think it's not very clear at this moment how would istio/istio depend on the filter API that is defined in istio/proxy, right? the authn filter seems to be the only case here?

Maybe I am wrong actually. Istio currently will try to read config dumps/things configured through EnvoyFilter, so if those are in use we need to import it. If this is some opaque blob for WASM or something then its fine. Basically if @type: foo shows up anywhere, we need to import foo I think

Please don't mix istio and envoy APIs. They use incompatible protobufs (gogo in istio, golang in xds), so it just causes confusion down the line.

+1. don't see why it's needed. (miss this thread before).

If we use istio/api independent envoy filter api, it will be placed similar api definition there. (like RequestAuthentication/PeerAuthentication) Is it acceptable? I understand that this filter api should be placed on istio/proxy but don't understand not to depend on this.

howardjohn · 2020-07-31T02:23:13Z

envoy/config/filter/http/authn/v2alpha2/config.proto

+// authentication policy on Envoy.
+message FilterConfig {
+  // Filter configuration for RequestAuthentication, which supports Jwt authentication.
+  istio.security.v1beta1.RequestAuthentication request_authentication = 1;


How is the filter going to consume RequestAuthentication directly, when the API requires control plane processing for jwksUrl?

I'm confusing about we should consider about jwks_url here. That will be processed finely on EnvoyFilter. https://github.com/envoyproxy/envoy/blob/cf2df8cbe1f5aff726d6b8ea54d5ad716023b70d/api/envoy/extensions/filters/http/jwt_authn/v3/config.proto#L197

In current code, we do not do jwksUri fetching in envoy, its done in the control plane

howardjohn · 2020-07-31T02:24:50Z

envoy/config/filter/http/authn/v2alpha2/config.proto

+  // mTLS authentication for sidecar to sidecar workload.
+  istio.security.v1beta1.PeerAuthentication peer_authentication = 2;
+
+  // Map from issuer to location of the payload that is emitted by Jwt filter.


what is a location?

I think that this field is not needed anymore. https://github.com/istio/istio/blob/536c38d179a770e736e8b2e9063feda89032af75/pilot/pkg/security/authn/v1beta1/policy_applier.go#L141. I will destroy this on the next version of AuthN filter API.

mandarjog · 2020-07-31T03:23:39Z

If this is going to passed thru api without the need for the control plane to understand it, we don’t need it here.

costinm · 2020-07-31T14:22:14Z

I don't think any of the envoy filter configs are really 'private' - it should probably move upstream to envoy, among all other envoy configs. But it must be treated as an api - maybe only used directly by EnvoyFilter users and control planes, but still API. But private or not - the defaults should be secure by default to avoid mistakes like we've seen in the past. It is extremely hard to change those - due to versioning and upgrades and control plane variants (istiod is just one implementation of Istio APIs - others are possible). I think the confusion is around what is an API and what is not. Contracts between components and services with multiple implementations are APIs in my mind. The injection URL format is an API as well ( there is a separate thread on this). Maybe not an API intended for end users but vendors and integrators - but still API.

…

On Thu, Jul 30, 2020, 15:38 Yangmin Zhu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In envoy/config/filter/http/authn/v2alpha2/config.proto <#1536 (comment)>: > @@ -0,0 +1,50 @@ +// Copyright 2020 Istio Authors Sorry for the late reply, this get lost in my inbox :( A high level comment, should we move this API to the proxy repo instead of putting it in the istio/api again? This is internal filter API and should not be in the istio/api (user-facing) repo in the first place, we could take this chance to clean it up. (this caused a lot confusions before, cc @costinm <https://github.com/costinm> @incfly <https://github.com/incfly> ) ------------------------------ In envoy/config/filter/http/authn/v2alpha2/config.proto <#1536 (comment)>: > + // mTLS authentication for sidecar to sidecar workload. + istio.security.v1beta1.PeerAuthentication peer_authentication = 2; + + // Map from issuer to location of the payload that is emitted by Jwt filter. + // This information is added by pilot when construct and add Jwt and + // authN filters. + map<string, string> jwt_output_payload_locations = 3; + + // Skips validating the peer's trust domain. + // By default, the istio authn filter will reject the request if the peer and + // the local service is not in the same trust domain. + // Set this field to true to skip the validation and allows peers from any + // trust domains. + // Note, the istio authn filter only validates the trust domain when mTLS is + // used, In other words, this field has no effect for plaintext traffic. + bool skip_validate_trust_domain = 4; I think there is another change from @incfly <https://github.com/incfly> for adding a list of strings for trust domain validation. ------------------------------ In envoy/config/filter/http/authn/v2alpha2/config.proto <#1536 (comment)>: > + +import "security/v1beta1/request_authentication.proto"; +import "security/v1beta1/peer_authentication.proto"; + +// $title: Internal API for authentication implementation on Envoy. + +package istio.envoy.config.filter.http.authn.v2alpha2; + + +option go_package = "istio.io/api/envoy/config/filter/http/authn/v2alpha2"; + +// FilterConfig is the config for Istio-specific filter that is used to enforce +// authentication policy on Envoy. +message FilterConfig { + // Filter configuration for RequestAuthentication, which supports Jwt authentication. + istio.security.v1beta1.RequestAuthentication request_authentication = 1; is it possible to not to depend on istio/api? technically the internal filter config doesn't need to depend on the user-facing API, and the filter also probably have a simpler API? just optional as we can always iterate on this part later. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1536 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2TTUNKJ2WKGMLJ4PILR6HY6JANCNFSM4O7KRUNQ> .

costinm · 2020-07-31T14:27:14Z

Someone generates this proto - and envoy consumes it. The generator may have multiple implementations. The consumer can also have multiple implementations ( there is at least one other data plane proxy using XDS compatible API). It doesn't matter if WASM or c++ or go is used to implement it. To me that's an API that needs to be treated as such.

…

On Thu, Jul 30, 2020, 20:23 mandarjog ***@***.***> wrote: If this is going to passed thru api without the need for the control plane to understand it, we don’t need it here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1536 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2W4NPXWH7NYOU6FZMTR6I2MRANCNFSM4O7KRUNQ> .

mandarjog · 2020-07-31T15:12:06Z

I agree about it being a filter api, and should be reviewed. But it does need to be defined as “istio” api here.

howardjohn · 2020-07-31T15:13:11Z

@mandarjog who is actually configuring this? The user? The control plane? something else?

mandarjog · 2020-07-31T17:00:04Z

@yangminzhu is this a passthru api or does it need controlplane intervention?

incfly · 2020-07-31T17:02:42Z

I am a bit confused. So question to @yangminzhu and @Shikugawa

why we put Istio API(RequestAuthentication and PeerAuthentication) inlined in the new WASM based authn filter API? Certain things like workload selector does not make sense to the filter at all. Are there some design/thread on this? I clicked through the linked PR but didn't find any clue.

costinm · 2020-07-31T18:47:54Z

What do you mean passthrough ? Someone must create it.

…

On Fri, Jul 31, 2020, 10:03 Jianfei Hu ***@***.***> wrote: I am a bit confused. So question to @yangminzhu <https://github.com/yangminzhu> and @Shikugawa <https://github.com/Shikugawa> why we put Istio API inlined in the new WASM based authn filter API? Certain things like workload selector does not make sense to the filter at all. Are there some design/thread on this? I clicked through the linked PR but didn't find any clue. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1536 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2WOCUD4A2EBGTOQIGDR6L2MHANCNFSM4O7KRUNQ> .

yangminzhu · 2020-07-31T20:46:16Z

@mandarjog who is actually configuring this? The user? The control plane? something else?

@howardjohn this is only configured by control plane (istiod).

@yangminzhu is this a passthru api or does it need controlplane intervention?

@mandarjog not sure what you mean for passthru api. This will be used by control plane (istiod) to configure the authn filter to do the work (e.g. extract attributes).

howardjohn · 2020-07-31T20:47:49Z

@yangminzhu if this is configured by istiod then istiod needs to import the proto?

kyessenov · 2020-07-31T20:50:06Z

@howardjohn this is only configured by control plane (istiod).

In that case, we need to produce golang protos since it's going to be embedded in xDS config dumps, etc. istio/api produces gogo.

@mandarjog not sure what you mean for passthru api. This will be used by control plane (istiod) to configure the authn filter to do the work (e.g. extract attributes).

Pass through means it's just pasted as opaque blob and not interpreted by istiod. It doesn't sound like the case anymore. Still, I don't think this should be user visible, and it is not a user API at all.

kyessenov · 2020-07-31T20:51:13Z

Related question: we should consider not using proto for the Wasm plugin. You are taking a hit of 1MB extra binary size just to parse 4 fields in a JSON.

yangminzhu · 2020-07-31T20:52:52Z

Yes, I mentioned the similar thing in the other PR in istio/api. 1) the (new) authn filter define its filter API purely in istio/proxy, no dependency on istio/api 2) istiod configures the (new) authn filter via the filter API. This part seems a bit tricking because it requires dependency on istio/proxy?

…

On Fri, Jul 31, 2020 at 1:48 PM John Howard ***@***.***> wrote: @yangminzhu <https://github.com/yangminzhu> if this is configured by istiod then istiod needs to import the proto? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1536 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQWO2HYLGOOIWECR3XNIJ3R6MUYLANCNFSM4O7KRUNQ> .

yangminzhu · 2020-07-31T20:56:31Z

On Fri, Jul 31, 2020 at 1:51 PM Kuat ***@***.***> wrote: Related question: we should consider not using proto for the Wasm plugin. You are taking a hit of 1MB extra binary size just to parse 4 fields in a JSON.

Thanks for the suggestion, Kuat. I think that's another factor we should take into consideration when evaluating whether or not we should have the (new) authn filter in wasm, currently I believe this is not decided yet and we're still doing evaluation. It may be worth investing some time to see if we can get it upstreamed in Envoy, so it's just normal Envoy filters and we don't need to worry about any of these dependencies.

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1536 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQWO2DSPKUR2RDAQLKIWA3R6MVFBANCNFSM4O7KRUNQ> .

kyessenov · 2020-07-31T20:59:08Z

@yangminzhu I agree that we should consider upstreaming if it is possible. But if that doesn't happen, then the only choice is Wasm ABI, so a lot of the questions raised here are moot (why even use proto if you don't have to).

envoy/config/filter/http/authn/v2alpha2/config.proto

Shikugawa · 2020-08-09T08:08:48Z

@istio/wg-security-maintainers I guessed about the usecase of new authentication filter and concluded that our requirements about this are,

To construct new (and simple) authn filter config definition.
Remove the dependencies on the user visible istio/api.
Put these filter API (like intermediate representation) on istio/proxy.
Only to have .pb.go on istio/istio.

So all we have to do is,

Create intermediate proxy filter config definition.
Implement conversion layer from user visible API (which is defined on security/v1beta1) to intermediate proxy filter config definition when delivering to data plane on istio/istio.

Is my idea correct? cc. @howardjohn @kyessenov @mandarjog

yangminzhu · 2020-08-17T20:23:00Z

@istio/wg-security-maintainers I guessed about the usecase of new authentication filter and concluded that our requirements about this are,

To construct new (and simple) authn filter config definition.

Remove the dependencies on the user visible istio/api.

Correct, this is one of the big benefits of the new authn filter.

Put these filter API (like intermediate representation) on istio/proxy.

Only to have .pb.go on istio/istio.

I'm not sure if we can put the original .pb.go in istio/proxy have a way to auto import it in the istio/istio repo as a copy. Hope we can have a way to make this maintenance easier instead of requiring manually coping the file between the two repos.

So all we have to do is,

Create intermediate proxy filter config definition.

Implement conversion layer from user visible API (which is defined on security/v1beta1) to intermediate proxy filter config definition when delivering to data plane on istio/istio.

Correct, this is basically just to use the new authn filter in istiod, should be easy and straightforward to do since we only need to configure a few fields for the authn filter (the most complicated part today is the Envoy JWT fitler which is not affected by this change)

Is my idea correct? cc. @howardjohn @kyessenov @mandarjog

istio-policy-bot · 2024-05-15T23:32:33Z

🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2020-08-09. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.

Add revised filter config for proxy_wasm authn filter

e702b36

Shikugawa requested review from dcberg, duderino, linsun, louiscryan, rshriram and smawson as code owners July 18, 2020 03:50

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

istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 18, 2020

fix

8f41c82

Shikugawa commented Jul 18, 2020

View reviewed changes

Shikugawa added 2 commits July 18, 2020 13:11

fix

597c9cf

fix

482b3a6

Shikugawa mentioned this pull request Jul 28, 2020

authn: revised request authenticator istio/proxy#2952

Closed

yangminzhu reviewed Jul 30, 2020

View reviewed changes

howardjohn reviewed Jul 31, 2020

View reviewed changes

incfly reviewed Jul 31, 2020

View reviewed changes

envoy/config/filter/http/authn/v2alpha2/config.proto Show resolved Hide resolved

This was referenced Aug 8, 2020

authn: add revised peer authenticator Shikugawa/proxy#1

Open

authn: add revised peer authenticator istio/proxy#2978

Closed

howardjohn removed the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label May 15, 2024

istio-policy-bot closed this May 15, 2024

istio-policy-bot added the lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. label May 15, 2024

Add revised filter config for proxy_wasm authn filter #1536

Add revised filter config for proxy_wasm authn filter #1536

Conversation

Shikugawa commented Jul 18, 2020

Uh oh!

Shikugawa commented Jul 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizan commented Jul 22, 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!

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!

mandarjog commented Jul 31, 2020

Uh oh!

costinm commented Jul 31, 2020 via email

Uh oh!

costinm commented Jul 31, 2020 via email

Uh oh!

mandarjog commented Jul 31, 2020

Uh oh!

howardjohn commented Jul 31, 2020

Uh oh!

mandarjog commented Jul 31, 2020

Uh oh!

incfly commented Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

costinm commented Jul 31, 2020 via email

Uh oh!

yangminzhu commented Jul 31, 2020

Uh oh!

howardjohn commented Jul 31, 2020

Uh oh!

kyessenov commented Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kyessenov commented Jul 31, 2020

Uh oh!

yangminzhu commented Jul 31, 2020 via email

Uh oh!

yangminzhu commented Jul 31, 2020 via email

Uh oh!

kyessenov commented Jul 31, 2020

Uh oh!

Uh oh!

Shikugawa commented Aug 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yangminzhu commented Aug 17, 2020

Uh oh!

istio-policy-bot commented May 15, 2024

incfly commented Jul 31, 2020 •

edited

Loading

kyessenov commented Jul 31, 2020 •

edited

Loading

Shikugawa commented Aug 9, 2020 •

edited

Loading