Add jwt auth filter config proto#530
Conversation
|
@kyessenov @mattklein123 who should review this? |
|
I will fix the test later. |
|
|
||
| syntax = "proto3"; | ||
|
|
||
| package Envoy.Http.JwtAuth.Config; |
There was a problem hiding this comment.
Please follow Envoy API Style Guide, specifically package == path, no _ in package name.
| // bookstore_web.apps.googleusercontent.com | ||
| // jwks_uri: https://example.com/.well-known/jwks.json | ||
| // | ||
| message JWT { |
| repeated string jwt_params = 8; | ||
|
|
||
| // This field is specific for Envoy proxy implementation. | ||
| // It is the cluster name in the Envoy config for the jwks_uri. |
There was a problem hiding this comment.
What is cluster and what is for?
| } | ||
|
|
||
| // Determines how to apply auth policies for individual requests. | ||
| message Config { |
There was a problem hiding this comment.
The term seems too generic. It doesn't seem to mean anything.
| // This message defines a pattern to match a HTTP request. | ||
| // A pattern is matched only if both http_method and path_match are matched. | ||
| message HttpPattern { | ||
| // Define a HTTP method. |
| // 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 |
There was a problem hiding this comment.
I don't think we need to restate the top-level LICENSE or attribute copyright here.
| // | ||
| // Example, | ||
| // | ||
| // issuer: https://example.com |
There was a problem hiding this comment.
Have you look at the docs output to see how this YAML renders? Should you use RST YAML directives? https://thomas-cokelaer.info/tutorials/sphinx/rest_syntax.html#code-block-directive
There was a problem hiding this comment.
Try to follow it. Not sure how to view the generated doc output
| string http_method = 1; | ||
|
|
||
| // Defines a path match | ||
| oneof path_match { |
There was a problem hiding this comment.
Can you reuse the RouteMatch protos in https://github.com/envoyproxy/data-plane-api/blob/master/envoy/api/v2/route/route.proto? Seems at the code level it should be possible to refactor/share to make this easy to implement and consistent.
There was a problem hiding this comment.
Updated to use HeaderMatcher
|
@qiwzhang Yes, I will review it now. |
diemtvu
left a comment
diemtvu
left a comment
There was a problem hiding this comment.
Just curious, what does v2 mean here? I don't know the naming convention, but found it a bit confusing as there is no v1 previously.
|
@diemtvu v2 was chosen to indicate the base version since the entire xDS as proto started from v2. Here it should mean that the filter config is versioned at v2. It would be confusing to explain v1 since for xDS v1 means REST/JSON-based config. |
| string jwks_uri = 3; | ||
|
|
||
| // If false, remove the JWT and do not forward to the backend. | ||
| bool forward_jwt = 4; |
There was a problem hiding this comment.
Is this "true" by default? In proto3, "false" is a better choice for a default bool value.
There was a problem hiding this comment.
no, default is false. default is not to forward already verified token to backend.
| load("//bazel:api_build_system.bzl", "api_proto_library") | ||
|
|
||
| api_proto_library( | ||
| name = "jwt_auth", |
There was a problem hiding this comment.
Consider renaming "jwt_auth" to "jwt_authn" if it is for authentication.
| string jwks_uri_cluster = 7; | ||
| } | ||
|
|
||
| // This is the Envoy filter config for JSON Web Token authentication. |
There was a problem hiding this comment.
For better cohesion, consider decoupling Envoy filter config from JWT authentication config.
There was a problem hiding this comment.
The filer is to do JWT authentication, so its config will be the JWT authentication config.
|
@htuch could you approve this? Code porting need this proto file. |
| // - bookstore_android.apps.googleusercontent.com | ||
| // bookstore_web.apps.googleusercontent.com | ||
| // jwks_uri: https://example.com/.well-known/jwks.json | ||
| // |
There was a problem hiding this comment.
nit: might be worth adding not-implemented-hide to indicate that this is not yet implemented in Envoy.
See, for example: https://github.com/envoyproxy/data-plane-api/blob/master/envoy/config/filter/accesslog/v2/accesslog.proto#L30
| // For example:: | ||
| // | ||
| // headers: | ||
| // name: :method |
There was a problem hiding this comment.
I think this should be:
headers:
- name: :method
|
Send email to envoy-user group: https://groups.google.com/forum/#!topic/envoy-dev/stCUfEITnqE |
| // | ||
| // x-goog-iap-jwt-assertion: <JWT>. | ||
| // | ||
| repeated string jwt_headers = 6; |
There was a problem hiding this comment.
for header, you might need a message here like:
message JwtHeader {
string header_name = 1;
string header_value_prefix = 2;
}
For example, authorization header will come with prefix Bearer, while x-google-iap-jwt-assertion won't.
|
|
||
| // This is the Envoy filter config for JSON Web Token authentication. | ||
| // [#not-implemented-hide:] | ||
| message JSONWebTokenAuthentication { |
There was a problem hiding this comment.
Document what the format of header that the filter will add after authentication.
| // <https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata>`_ | ||
| // | ||
| // Example: https://www.googleapis.com/oauth2/v1/certs | ||
| string jwks_uri = 3; |
| // If the request includes a JWT, it must match one of the JWT listed | ||
| // here with the same issuer. | ||
| // | ||
| repeated JSONWebToken jwts = 2; |
| // - name: :path | ||
| // regex_match: /.* | ||
| // | ||
| repeated envoy.api.v2.route.HeaderMatcher headers = 6; |
| // exact_match: /healthz | ||
| // | ||
| repeated HttpMatcher bypass_jwt = 3; | ||
| } |
There was a problem hiding this comment.
You might need an action here (or in JSONWebToken) to define what the filter do when it cannot verify a JWT or a JWT doesn't exists.
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
To build docs to inspect output, run ./ci/run_envoy_docker.sh './ci/do_ci.sh bazel.docs'.
| // jwks_uri: https://example.com/.well-known/jwks.json | ||
| // | ||
| // [#not-implemented-hide:] | ||
| message JSONWebToken { |
There was a problem hiding this comment.
Prefer JsonWebToken to match other capitalization in API.
| // <https://tools.ietf.org/html/rfc7519#section-4.1.3>`_. that are allowed to | ||
| // access. A JWT containing any of these audiences will be accepted. | ||
| // | ||
| // Example:: |
There was a problem hiding this comment.
Can you use the YAML syntax? E.g. https://raw.githubusercontent.com/envoyproxy/data-plane-api/a66448b203e3a4f8ebbea0a5b7beede5c25640f6/docs/root/faq/sni.rst
| // | ||
| repeated string audiences = 2; | ||
|
|
||
| // URL of the provider's public key set to validate signature of the |
There was a problem hiding this comment.
Should this be URL of the provider's public key. Set to validate signature of the...?
| // It is the cluster name in the Envoy "cluster_manager" config section. | ||
| // In order for Envoy to call "jwks_uri", its host has to be specified | ||
| // as a "cluster" in the config for each jwks_uri. | ||
| string jwks_uri_cluster = 4; |
There was a problem hiding this comment.
This seems strange. Yes, Envoy needs a cluster to call out to, but in that case, jwks_uri should just be the path suffix, the host part of the URL isn't used?
Also, how is this supposed to be used normally? Are we going to be calling out to a dedicated internal server for jwks_uri, or is this arbitrary Internet callout?
There was a problem hiding this comment.
Arbitrary Internet callout
I like to keep pks_uri so that it not only specify the path, also specify protocol, such as http or https
cluster only specify port: unusually is tcp://x.x.x.x:443
There was a problem hiding this comment.
While cluster does know whether it is a SSL or not (tls_context), we still need protocol and hostname from jwks_uri for :scheme and :authority header.
There was a problem hiding this comment.
One should use gwks_uri with discovery service to find the cluster. I don't think the JWT itself should have this information. URI can contain arbitrary amount of information, such as base?cluster=xxx
There was a problem hiding this comment.
To me, this is a pretty weird situation, where we might not want to specify a fixed cluster if it's arbitrary Internet callout. It's similar to what's done for the gRPC service https://github.com/envoyproxy/data-plane-api/blob/master/envoy/api/v2/core/grpc_service.proto#L32, where we have a cluster-less URI and the library just connects directly. We probably would need to make use of the pending cluster late binding support in envoyproxy/envoy#2740, but we could create a dynamic cluster on-the-fly to use for this URI. @mattklein123 also for thoughts.
There was a problem hiding this comment.
We will implemented this in phases. In phase 1, before Envoy supports Lazy config loading or dynamic cluster, a dedicated jwks cluster is required. It can be appended in the jwks_uri as "JWKS_URI?cluster=name". So this field is removed.
In the phase2, if lazy config loading is supported, this requirement is removed.
There was a problem hiding this comment.
I strongly disagree with the idea to include ?cluster= in to jwks_uri, jwks_uri can be a arbitrary URI (including query parameter), the cluster is really just envoy term, so it has to be a separate field and has nothing to do with jwks_uri.
Also in the case that control-plane handles OIDC discovery, jwks_uri is coming directly from the discovery document, while cluster is generated by control-plane, please do not mix them.
@htuch this is just like a EnvoyGrpc message, (i.e. cluster name). if we want a general EnvoyHttp alike message in core that is a different issue.
There was a problem hiding this comment.
I think we should have an EnvoyHttp source or the like. This is a weird hack in the API IMHO, we shouldn't do it as is.
There was a problem hiding this comment.
This is somewhat related to envoyproxy/envoy#1606. I agree with @htuch that we should add an EnvoyHttp message to allow for generic callout. For now that message can just have a static cluster option, but we can enhance it later to do completely independent DNS resolution/caching and generic calls.
There was a problem hiding this comment.
@qiwzhang Can you add an EnvoyHttp message into envoy core?
| string jwks_uri_cluster = 4; | ||
|
|
||
| // Duration after which the cached public key should be expired. The | ||
| // system wide default is applied if no duration is explicitly |
There was a problem hiding this comment.
Where does the system wide default come from?
There was a problem hiding this comment.
will update to
hard-coded default of 5 minutes
| google.protobuf.Duration public_key_cache_duration = 5; | ||
|
|
||
| // If false, the JWT is removed in the request after a success verification. | ||
| // Set it true, if don't want it to be removed. |
There was a problem hiding this comment.
Specify what the default behavior is.
| google.protobuf.BoolValue forward_jwt = 6; | ||
|
|
||
| // If true, the request is allowed if the JWT verification fails | ||
| // Default is not allowed. |
| // For example, if config is:: | ||
| // | ||
| // jwt_params: | ||
| // jwt_token |
| // The value prefix. The value format is "prefix<token>" | ||
| // For example, for "Authorization: Bearer <token>", | ||
| // value_prefix="Bearer " with a space at the end. | ||
| string value_prefix = 2; |
There was a problem hiding this comment.
Is this always what you want, or would you prefer regex? No strong opinion, just curious.
There was a problem hiding this comment.
Usually we want the whole value. Only "Authorization: Bearer " has a "Bearer " prefix. regex is over-kill.
| // - name: :path | ||
| // regex_match: /.* | ||
| // | ||
| repeated envoy.api.v2.route.HeaderMatcher headers = 1; |
There was a problem hiding this comment.
Can you comment (in this PR review) on why this was preferred over RouteMatch?
There was a problem hiding this comment.
Initially I felt that RouteMatch is too complicated, especially its "runtime" field, it may not apply here.
Well, I look it over again. I changed my mind, we could just use it directly. Will update
|
|
||
| // URL of the provider's public key set to validate signature of the | ||
| // JWT. See `OpenID | ||
| // URL of the provider's public key server. The public key is used to validate |
There was a problem hiding this comment.
jwks stands for Json Web Key Set: https://tools.ietf.org/html/rfc7517#appendix-A
There was a problem hiding this comment.
Updated the comment. Thanks
|
I think we should document the higher level design somewhere, either in README.md in the same directory or at the top of the proto file. As an Envoy filter, I would like to see a clear contract here:
For example, |
|
|
||
| syntax = "proto3"; | ||
|
|
||
| package envoy.config.filter.http.jwt_authn.v2; |
There was a problem hiding this comment.
No "_" in package name please.
There was a problem hiding this comment.
@wora this is necessary if the path has underscore for consistency and some Go stuff.
| // jwks_uri: https://example.com/.well-known/jwks.json | ||
| // | ||
| // [#not-implemented-hide:] | ||
| message JsonWebToken { |
There was a problem hiding this comment.
I think Jwt is better just like Url. Nobody spells out Universal Resource Locator these days.
| // It is the cluster name in the Envoy "cluster_manager" config section. | ||
| // In order for Envoy to call "jwks_uri", its host has to be specified | ||
| // as a "cluster" in the config for each jwks_uri. | ||
| string jwks_uri_cluster = 4; |
There was a problem hiding this comment.
One should use gwks_uri with discovery service to find the cluster. I don't think the JWT itself should have this information. URI can contain arbitrary amount of information, such as base?cluster=xxx
| // If false, the JWT is removed in the request after a success verification. | ||
| // Set it true, if don't want it to be removed. | ||
| // Default value is false which is to remove JWT. | ||
| google.protobuf.BoolValue forward_jwt = 6; |
There was a problem hiding this comment.
If default is false, just use bool.
|
|
||
| // If true, the request is allowed if the JWT verification fails. | ||
| // Default value is false; JWT verification failed request will be rejected. | ||
| google.protobuf.BoolValue allow_failed_jwt = 7; |
There was a problem hiding this comment.
If default is false, use bool.
| // If the request includes a JWT, it must match one of the JWT listed | ||
| // here with the same issuer. | ||
| // | ||
| repeated JsonWebToken jwts = 1; |
There was a problem hiding this comment.
This is not really a JWT, it is more like JwtRule.
|
A README.md is added to document the high level design. |
| load("//bazel:api_build_system.bzl", "api_proto_library") | ||
|
|
||
| api_proto_library( | ||
| name = "jwt_authn", |
There was a problem hiding this comment.
it should be authn. "authz" is for authorization. This proxy is only for token verification, it is authentication, it is "authn".
|
Hi level comment: This is great.. We need to propagate this context as part of the trace headers, as that makes it very easy to transport these custom headers across hops. Thoughts? |
|
|
||
| ## HTTP header to pass sucessfully verified JWT | ||
|
|
||
| If a JWT has been suceessfully verified, its payload will be passed to the backend in the new HTTP header "sec-istio-auth-userinfo". Its value is base64 encoded JSON. |
There was a problem hiding this comment.
Is this istio specific? We should use more generic location. On a related note, is this possible to config the output location (or not output at all)?
There was a problem hiding this comment.
Good point. I will add such config for each JWT.
There was a problem hiding this comment.
This documentation is still Istio specific.
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
| // | ||
| // If it is not specified, the payload will not be forwarded. If multiple JWT payloads needed to | ||
| // be forwarded, distinct header names are required. | ||
| string forward_payload_header = 8; |
There was a problem hiding this comment.
Still waiting for this one to be made clearer, I still can't get clear in my head what the above sentence means given this is a singleton. If it's confusing me (and admittedly, I'm not an expert in this area), I think it will confuse others, so can you try and re-express this to reduce the chance of this?
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
| // .. code-block:: yaml | ||
| // | ||
| // local_jwks: | ||
| // - inline_string: "ACADADADADA" |
There was a problem hiding this comment.
really last nit: make this realistic?
There was a problem hiding this comment.
It is too big to put a good jwks here
|
I'm going to merge to allow @qiwzhang to make forward progress. Since this is |
9 participants
This is the first step to upstream Istio JWT auth filter to Envoy.
Istio JWT auth filter is here [https://github.com/istio/proxy/tree/master/src/envoy/http/jwt_auth]
The issue to upstream to envoy is: envoyproxy/envoy#2514