Jwt_authn config update: Support per route Jwt requirements

[qiwzhang]

Signed-off-by: Wayne Zhang qiwzhang@google.com

Description:
Based on the requirement discussion from #2514.
Change the Jwt_authn config to support different requirement based on route match.

Risk Level: Low

[qiwzhang]

@llchan please help to review this config change to see if it works for your cases.
@diemtvu, @lizan Please help to review this change.

[lizan]

Just delete this instead of DEPRECATED since no one is using this? same below.

[qiwzhang]

Unfortunately, Istio 0.8 is using this one. I am pretty sure nobody is using the "bypass" field.

[diemtvu]

We actually do
https://github.com/istio/istio/blob/0214aee9cb9c75afa59e83364c7acd52afa01656/pilot/pkg/networking/plugin/authn/authentication.go#L128

[diemtvu]

Can you add comment to clarify if all are required or just one.

[qiwzhang]

All are required. Updated the comment.

[diemtvu]

We actually do
https://github.com/istio/istio/blob/0214aee9cb9c75afa59e83364c7acd52afa01656/pilot/pkg/networking/plugin/authn/authentication.go#L128

[diemtvu]

It's not quite clear to me why we need this bool? Can we simply put empty required_issuers list?

[qiwzhang]

Sound likes a good idea. I will remove it.

[llchan]

@qiwzhang This works for me, but will you also include the ability to configure at the route level with per-route-filters as discussed on the issue thread? I can see other people wanting that. If we support both it should cover a pretty wide variety of setups.

EDIT: maybe spoke too soon, have a few comments that I'll add inline.

[llchan]

I would maybe rename this, since they are no longer rules but provider defintions.

[llchan]

Actually I don't know anymore, maybe rules is okay, you guys can discuss internally what the naming should be.

[qiwzhang]

Changed to Provider

[llchan]

What's the plan with other kinds of request matching (e.g. header matching for CORS options requests)?

[qiwzhang]

envoy.api.v2.route.RouteMatch supports header matching in additional to path matching.

[llchan]

Not sure how to comment on parts of the file that aren't part of the diff so I'll do it here.

I think the issuer/audience definitions (aka JwtRule currently) need an additional user-specified key/id that is referred to when specifying the requirements below. The issuer url/email alone may not be flexible enough because the audience is also important when validating a token. For example, if I have one issuer https://issuer.com that issues tokens for two audiences foo.com and bar.com, those are two different specs and the user should be able to refer to them independently in the route requirements.

[qiwzhang]

I added "audiences" in JwtRequirement to override the one in the JwtProvider. You can set different audiences in the per-route requirement.

[qiwzhang]

@llchan, @lizan, @htuch PTAL. I like to get this in. Thanks

[llchan]

I think the YAML examples above are a bit incorrect ("requires" field, dashes should be on issuer lines), could you double check @qiwzhang?

[qiwzhang]

Done

[llchan]

Same as above, check the YAML, I think "requires" fields are a little off.

[qiwzhang]

Done

[llchan]

Again, check the YAML, I think both match and requires need some tweaks.

[qiwzhang]

Done

[llchan]

If I'm reading the comment correctly, this means we're AND-ing all the requirements? Is that more useful than OR-ing the requirements? I'd think that a typical request has one JWT and would thus only match one issuer, so it's unlikely for AND-ed requirements to be satisfied unless they all have the same issuer and only differ in audience + the JWT has multiple audiences defined. Having OR-ed requirements means the route can accept tokens from multiple issuers, which seems like a more common use case. Thoughts?

[qiwzhang]

I think AND-ing the requirement will be more useful. If you ever want to verify two JWT from different issuers, you most likely want both of them to pass, not just one of them. For example, Google cloud supports IAP and end-user token, some requests want both to pass, it will be: [iap, end_user_jwt], some only want iap, it will be [iap], even the requests may carry end_user_jwt, but it is not required. It is very hard for me to imaging a request only wants either iap or end_user_jwt.

[llchan]

I see, so the request carries multiple JWTs in different headers/params? If so AND-ing makes sense, because otherwise there isn't a good way to specify it.

[lizan]

@qiwzhang we do OR-ed requirements in Google Cloud, https://github.com/googleapis/googleapis/blob/master/google/api/auth.proto#L61 . It is useful to support AND-ing when you have multiple JWTs. However, more common cases we've seen are: your service serve both internal service-to-service traffic and ingress traffic, then you need to specify JWT signed by a service account OR End-user JWT issued by A OR End-user JWT issued by B.

[llchan]

Agree with @lizan , I'd prefer OR too, at least at the top level. Just seems more common to allow multiple issuers to authenticate a user.

However, I could see multi-JWT requests being used, so perhaps we can come up with something that satisfies both? I'd rather start off with a slightly more verbose config vs change it down the road.

A few misc notes that come to mind for multi-JWT configuration:

• We could do a "2-d" list, where the top level iterates over OR-ed requirements and the inner is AND-ed. Or maybe use a union or oneof?
• We'd probably want a way to map headers to requirements explicitly, otherwise the implementation would have to try each of the N^2 pairs. So either the requirements need to specify the from_headers or from_params, or the requirements need ids and there needs to be some mapping elsewhere. EDIT: nevermind, this probably isn't necessary: in typical cases the JWT issuer claim should make it easy to look up the correct provider to use to validate.

[lizan]

Let's not make things too complicate, we might be fine with OR here, but if there are multiple JWTs, validate all of them if possible, and let authZ (RBAC) to handle the complex AND/OR rules.

[llchan]

I vaguely remember someone else commenting on this field and recommending that it be removed (rather than deprecated) since this config is still in the alpha stage. Was there a final decision on that? It seems that this behavior is pretty trivial to specify as a single catch-all requirement.

[qiwzhang]

Istio is using it (as internal config between its components, not a user facing config). So we could not remove it now, but like to remove it once Istio code is changed to use new "rules" field.

[llchan]

Got it, sounds good.

[qiwzhang]

@llchan could you approve this PR?
@lizan @htuch could you PTAL?

[qiwzhang]

OK, I added 2D "or_jwt" and "and_jwt" in the JwtRequiement. PATL

[lizan]

add validate rule that this must not be empty.

I feel there should be better name than and_jwt / or_jwt...

[qiwzhang]

Added validate rule, and renamed to "one_of" and "all_of"

[qiwzhang]

Format failure is from someone else files.

ERROR: clang-format check failed for file: ./api/envoy/config/grpc_credentials/v2alpha/file_based_metadata.proto

[lizan]

@qiwzhang #3517 should fix the format ci, which is broken on master now.

[llchan]

Overall I think this is reasonable. It feels a bit verbose and seem to be overreaching a bit into authz, but at least it's pretty clear what it's doing.

That said, I'm not 100% sold on it, so I'll suggest another option that isn't quite as flexible, but is far more concise for the common use case while supporting simple AND-ed requirements:

- rules:
  - match: { prefix: "/foo" }
    requires_any: [{ issuer: "iss-A" }, { issuer: "iss-B" }]
  - match: { prefix: "/bar" }
    requires_all: [{ issuer: "iss-A" }, { issuer: "iss-B" }]

Compare that to the equivalent one_of/all_of config:

- rules:
  - match: { prefix: "/foo" }
    requires: { one_of: [{ all_of: [{ issuer: "iss-A" }] }, { all_of: [{ issuer: "iss-B" }] }] }
  - match: { prefix: "/bar" }
    requires: { one_of: [{ all_of: [{ issuer: "iss-A" }, { issuer: "iss-B" }] }] }

This should be doable with a simple oneof between requires_any and requires_all. Or requires_one if that's better.

FWIW I'm also in favor of what @lizan suggested: drop AND-ed requirement support at this layer and just use a simple OR requirement, but validate all JWTs presented. That would certainly suffice for me, but since I'm not familiar with the multi-JWT use case I'll defer that decision to you guys.

EDIT: update to the proposal above, we could also support a single requirement like so, allowing for a very clean rule table for simple use cases.

- rules:
  - match: { prefix: "/baz" }
    requires: { issuer: "iss-A" }
  - match: { prefix: "/foo" }
    requires_any: [{ issuer: "iss-A" }, { issuer: "iss-B" }]
  - match: { prefix: "/bar" }
    requires_all: [{ issuer: "iss-A" }, { issuer: "iss-B" }]

[nickrmc83]

@qiwzhang I've added some comments. Feel free to push back as I've reviewed from the perspective of re-using some of the functionality in a different context.

[nickrmc83]

@qiwzhang From a configuration PoV the only part of this change that doesn't exactly meet my usecase is that the JwtProvider type describes both the verification and forwarding parameters for a JWT. As a result filters that wish to re-use just the verification logic are also required to set the forward rule to true (as false is the default) so that the headers are not mutated. It'd be nice if either forwarding JWT headers was the default behaviour or alternatively and preferentially there was a clear type separation between verification configuration and forwarding configuration. The latter case would allow other filters to expose just the verification configuration as part of their own config. For example:

message VerificationRules {
  string issuer = 1 [(validate.rules).string.min_bytes = 1];
  ...
}

message FrowardingRules {
  bool forward = 1;
  string forward_payload_header = 2;
  ...
}

message JwtProvider {
  VerificationRules verification_rules = 1;
  ForwardingRules forwarding_rules = 2;
}

[qiwzhang]

Not clear on what is requested. Yes, current Provider has combined VerificationRule and ForwardRule. But not clear the benefits of separating them.
If jwt_authn filter is used alone, it needs both VerificationRule and ForwardRule. If jwt_authn filter is used only for verification by another filter (like your or Istio-authn), you still need to set ForwardRule for jwt_authn filer. For example, istio_authn doesn't need origin token, so its "forward" is false, just use its payload so it will use "forward_payload_header". If your filter needs origin token, you just set "forward" to true.

Maybe you will not use jwt_authn filter as a whole filter, you will just link its code as c++ library into your filter? I don't think it is a good way to go.

[nickrmc83]

Maybe you will not use jwt_authn filter as a whole filter, you will just link its code as c++ library
into your filter? I don't think it is a good way to go.

Unfortunately I think the only approach for my filter is to include and link the jwt_authn code directly and invoke it in the same way as http/jwt_authn/http_filter.cc:decode_headers. The reason for this is because the JWT on which I need verification performed is obtained by my filter via a filter-initiated external asynchronous HTTP request. As a result the JWT that is acquired does not pass through the filter chain. So using the code as a filter is not really an option. As such I need to make my filter expose the necessary config to perform verification and ideally not expose parameters that are not relevant whilst not redefining similar structures that already exist. In my case the forward field is not applicable. A solution to that is simply to document that setting this field within my filter will have no effect. And that's fine. So disregard my initial request but I will be wanting to directly invoke the Authenticator code.

[nickrmc83]

For use by other filters, is it possible to have an onComplete callback that passes a const reference/pointer to the token as well as the status? In my uses case (OpenID Connect) I need to check some additional JWT claims to validate against XSRF attacks. These claims are potentially use case specific so pushing them into the Authenticator would be wrong.

[qiwzhang]

Clearly, you were thinking about linking jwt_authn code into your filter.

I will prefer to explore if you can use jwt_authn code as a filter. Your filter can use its verification result. Istio_authn originally linked jwt_authn code into its filter, it makes the filter very complicated since it needs to deal with aync callbacks. We abandoned that idea. Now it will not touch jwt_authn filter, just config it to behave the way istio_authn wanted and uses its result.

Now the result is passed by HTTP headers. We can change it to use requestinfo.dynamidMetadata to pass the result. https://github.com/envoyproxy/envoy/blob/master/include/envoy/request_info/request_info.h#L282

[lizan]

In (near) future we will make the verified JWT into metadata, that will allow JWT works with RBAC filter, see #3638 too. I think that will address your use case? @nickrmc83

[nickrmc83]

@qiwzhang @lizan see my earlier comment about why I can't really re-use the code as a stand-alone filter and why I would like to directly invoke the Authenticator verification code. I suggest pushing forward this PR and discussing my usage in a PR I raise as I think my needs will become clearer.

[qiwzhang]

@nickrmc83 . It seems that we agreed that your user case is so special, we should not adjust jwt_authn config for that.
You may raise a separate PR for the Authenticator class code change to separate verification with token forwarding

[qiwzhang]

@lizan I addressed your comment of moving "audiences" to RequiredProviider by adding a new field in JwtRequirement onof field: "provider_with_audiences". Users can config audiences in JwtProvider, they can also override them in JwtRequirement.

[qiwzhang]

@lizan @htuch This PR is ready for review. This PR will just get the config change in first. Its added "JwtRequirement" feature is not implemented yet. I will use a separate PR to implement it.

[htuch]

LGTM modulo nits. Looks quite neat. Would be good to obtain approval from @lizan and @nickrmc83 as well.

[htuch]

Out of curiosity, would this work if we just inline it into the above fields? E.g. if we had repeasted JwtRequirement requires_any = 3 [constraints];? I'm not familiar with the proto rules on self-reference.

[qiwzhang]

Not, it will not work, proto "oneof" doesn't "repeated" field.

[htuch]

Nit: I think what you want is "that requests matching the path prefix don't require JWT authentication."

[qiwzhang]

Done

[htuch]

Nit: "the rest of the requirements"

[qiwzhang]

Removed

[htuch]

Nit: "even if the verification fails"

[qiwzhang]

Removed

[htuch]

I think this is pretty surprising behavior; is there no way to design this based on principle-of-least-surprise? E.g. add an explicit "continue_request_on_verification_fail" field?

[qiwzhang]

Thanks for the input. Agreed. I removed it and add "allow_missing_or_failed" as one of "oneof" in the JwtRequirement.

[nickrmc83]

LGTM

[htuch]

Thanks for the work put into making this rock.

[atali]

Hello guys,

I would like to support multi-tenancy on our microservice. Once the user is authenticated, my JWT will contains a field like tenant_id: 12323232 . I would like to route the request based on that jwt field to the specific tenant's microservice. Do you think it will work on what you have done ?

[lizan]

@atali can you open an issue with more details of your use case? Currently it is not possible to route request based on JWT claims (this PR is reverse, route-based JWT configuration).

[potatop]

Is this still being worked on? If not, I would like to help.

[qiwzhang]

@potatop could you be specific on which feature you like to help?
for routing based on jwt fileds, I am not aware of anybody is working on it.
for supporting per-route requirement: config proto is in, but implementation did not start yet.
Well, contribution to any of above is welcomed.

[potatop]

Yes, I was talking about the RequirementRule functionality.

[potatop]

Can I get some clarification on desired behavior? Sorry if this has been answered before.

• An empty JwtRequirement mean verification is not required. Is this the same as
  allow_missing_or_failed: true?
• If no requirements matched a path, what should be the behavior? Currently, the verifying provider is selected based on the issuer found on the request token. This is the desired behavior or should some error be returned with 401?

[qiwzhang]

• yes, it is almost the same, but there are different; the former will not do any jwt verification, the latter will verify all jwts in the request, but allow fails and missing.

• if no requirements matched a path, it will be the same as empty JwtRequirement; not any jwt verification.

[lizan]

@potatop can you open a new issue for further questions/feature requests?

[potatop]

@qiwzhang Thanks
@lizan Will do.