Add rule requirement feature to jwt_authn filter

[potatop]

Title: Add rule requirement feature to jwt_authn filter

Description: Add rule requirement feature based on config proto and discussions in #3381.
Requires type provider_and_audiences, requires_any, and requires_all is not implemented.

Risk Level: Medium

Testing: unit test and manual

[qiwzhang]

matchers_ should be created per filter config, not per request. Here Authenticator is created per-request.

You can move them here https://github.com/envoyproxy/envoy/blob/master/source/extensions/filters/http/jwt_authn/filter_config.h#L59

[lizan]

Thanks for taking this.

[lizan]

You can just ommit the parameter name.

[lizan]

also, make the parameter const reference?

[lizan]

snake_case_ for member variables

[lizan]

const ref

[lizan]

just do issuer_ == iss?

[lizan]

no need c_str

[lizan]

from the comment, you don't need a pair, just return a JwtMatcherConstPtr is enough

[lizan]

ConstSharedPtr

[lizan]

RequirementVerifierConstSharedPtr

[qiwzhang]

Ideally, we only need to extract the tokens for required providers. So Extractor should take a list of required_issuers.

[qiwzhang]

Current approach will be very difficult to extend to support multiple tokens. Especially with such requirements as

1. require A and B, or 2) require A or B.

Ideally, for 1), if A fail, not need to verify B. for 2) if A success, not need to verify B

But single token is the most common use case. We can worry about supporting multiple tokens later.
Please add // TODO

[qiwzhang]

can we move Matcher into a set of .h, and .cc file, and add some unit-test for them.
Same for Verifier.

[qiwzhang]

I think we need two functions:
requiredIssuers() to return required issuers. This one is need for Extractor to only extract required tokens.

staus verify(vector<pair<issuer, status>> issuer_status); // after required token are verified, passed their issuers and verified_status to verify if they satisfy the requirement.

[potatop]

1. To do this we need to move jwt parser to extractor?
2. OK I'll do that.

[qiwzhang]

@potatop thanks for helping out this feature. Here are some high level ideas how it should be done:

1. Eventually we need to support multiple tokens. For now, it is OK to only supports one token, but we need an approach that can be easily extend to support multiple one. Otherwise, it will be a totally re-write in order to support it.

2. Ideal data flow may look like this:
   a) find the requirement for a request, if no found, just pass the request.
   b) based on the specific requirement, it will try to extract one token and verify it, and make decision to reject or pass the request or continue to work on the next required token.

[qiwzhang]

Not need to list verifier.h here again since this lib depends on verifier_lib which already listed it.

[qiwzhang]

per google style guide: https://google.github.io/styleguide/cppguide.html
please add comment for each function in the header file.

[qiwzhang]

you could implement it here. move line 42- 47 here

[qiwzhang]

for is_allow_missing_or_failed, try to see if we don't need to make this as a special case.

[qiwzhang]

can we have one static function to create a matcher? hide of the logic of creating different matcher inside the function.

[qiwzhang]

This checking will not eliminate anything.

What I means is: when each request come, find its matched requirement, and required provider, only extract token for that provider or providers.

[potatop]

Yeah, I realized that later, so I already changed that. However, I'll probably back out all this With extract only returning tokens with expected issuers, this is probably not needed.

[qiwzhang]

@potatop please try to restructure the data flow to following:

a) find the requirement for a request, if no found, just pass the request.
b) based on the specific requirement, repeat the following:
while (next_provider()) {
extract the token for that provider and verify it,
make decision to reject or pass the request
}

[potatop]

I'm working to add more tests.

[potatop]

@qiwzhang I updated the code like you asked. Please take another look. Thanks

[qiwzhang]

I don't see it is used.

[qiwzhang]

Can we pass in a reference, not to make a copy

[qiwzhang]

please move these implementation inside the class, e.g. to line 49. It will be easier to read

[qiwzhang]

Ideally, not to check all headers or parameters. only check these headers and parameters specified by the required providers

[potatop]

done.

[qiwzhang]

In general, I don't like this implementation. It put all logic inside this class. The code is fairly complicated.

Please take a look rbac implementation in Envoy. https://github.com/envoyproxy/envoy/tree/master/source/extensions/filters/common/rbac

Please try to see if we can implement the matcher like that.

[potatop]

@qiwzhang What about this? Matchers are created at start up based on the rules configuration.

[qiwzhang]

This should be done by the Verifier.

The correct way is: in BaseVerifierImpl::completeWithStatus() function, before calling the context->callback, call context->cancel()

[potatop]

Ok.

[qiwzhang]

how about this one is called fetcher_. the one above it is called raw_fetcher_

[qiwzhang]

comment doesn't match the test

[qiwzhang]

LGTM

[potatop]

@lizan Can this be merged now?

[lizan]

Sorry I was on vacation last Friday, LGTM and I will take another pass tomorrow since this is huge.

LGTM

[potatop]

Ok. Thanks.

[potatop]

Many thanks @lizan @qiwzhang. Is there anything else I need to do to get this merged?

[lizan]

@potatop thanks, waiting one of senior maintainers to take a final pass and merge.

[mattklein123]

Did a high level skim / sanity check and LGTM.