-
Notifications
You must be signed in to change notification settings - Fork 588
Add revised filter config for proxy_wasm authn filter #1536
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| // Copyright 2020 Istio Authors | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| syntax = "proto3"; | ||
|
|
||
| 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; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is needed by istio/istio, hopefully we don't need to import istio/proxy?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1. don't see why it's needed. (miss this thread before).
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is the filter going to consume RequestAuthentication directly, when the API requires control plane processing for jwksUrl?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In current code, we do not do jwksUri fetching in envoy, its done in the control plane |
||
|
|
||
| // Filter configuration for PeerAuthentication, which supports | ||
| // mTLS authentication for sidecar to sidecar workload. | ||
| istio.security.v1beta1.PeerAuthentication peer_authentication = 2; | ||
incfly marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // Map from issuer to location of the payload that is emitted by Jwt filter. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is a location?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| // This information is added by pilot when construct and add Jwt and | ||
| // authN filters. | ||
| map<string, string> jwt_output_payload_locations = 3; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we place this on here? |
||
|
|
||
| // 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; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there is another change from @incfly for adding a list of strings for trust domain validation. |
||
| } | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.