Skip to content

jwt_authn: change allow_missing implementation under RequiresAny#13839

Merged
mattklein123 merged 5 commits intoenvoyproxy:masterfrom
qiwzhang:require-any-missing
Nov 12, 2020
Merged

jwt_authn: change allow_missing implementation under RequiresAny#13839
mattklein123 merged 5 commits intoenvoyproxy:masterfrom
qiwzhang:require-any-missing

Conversation

@qiwzhang
Copy link
Copy Markdown
Contributor

@qiwzhang qiwzhang commented Oct 30, 2020

This is to fix: #13458

The current implementation of allow_missing or allow_missing_or_failed under RequiresAny is buggy. This is how it implemented currently. If you have following JwtRequirement:

    require_any:
         provider_name:  A
         provider_name:  B
         allow_missing:

We create following 3 verifiers:

  1) a verifier for A, 
  2) a verifier for B,
  3) a verifier for A, B, with allow_missing. 

The 3) veirifier causes two problems:

  1. Token for provider A or B are verified twice.
  2. If 3) veirfier is done first (since it has allow_missing flag, it can be done first). Its successful return will abort 1) or 2). That is the root cause of Race Condition when multiple remote jwks providers defined along with allow_missing_or_failed #13458

This PR will make following changes:

  • Remember the return status for all children verifier for GroupVerifier, especially for AnyVerifier.
  • AnyVerifier will aggregate the final return based on its children result.
  • Not to create 3) verifier, instead mark the allow_missing or allow_failed flag in AnyVerifier. Use the flag in the final status aggregation.
  • Not need to pass parent_provider to Verifier::innerCreate(). It was only used when creating a AllowMissing or AllowFailed verifier under require_any. It is not needed any more.

Risk Level: Low. It only impacts allow_missing or allow_missing_or_failed flag under require_any. This is a rarely used feature.
Testing: unit-tested
Docs Changes: None
Release Notes: None

Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@qiwzhang qiwzhang requested a review from lizan as a code owner October 30, 2020 21:53
@qiwzhang
Copy link
Copy Markdown
Contributor Author

@yangminzhu Could you help to review this change? I believe Istio is the only project using this allow_missing or allow_missing_or_failed flag.

Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Copy link
Copy Markdown
Contributor

@yangminzhu yangminzhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this change the existing behavior of the API (allow_missing or allow_missing_or_failed)? From the description it looks like it's doing some internal optimization. But the other sentences seems implying it's going to change the API?

The code is really complicated and I will need some more time to take a closed look.

@qiwzhang
Copy link
Copy Markdown
Contributor Author

qiwzhang commented Nov 4, 2020

It doesn't change the API. It only changes internal optimization. And it only impacts the case where allow_missing and allow_missing_or_failed under require_any

API functionalities are guarded by many unit-tests; group_verifier_test and all_verifier_test.

@qiwzhang
Copy link
Copy Markdown
Contributor Author

@yangminzhu friending ping.....

Copy link
Copy Markdown
Contributor

@yangminzhu yangminzhu left a 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 looks good to me, only a few small comments.

@yangminzhu
Copy link
Copy Markdown
Contributor

It doesn't change the API. It only changes internal optimization. And it only impacts the case where allow_missing and allow_missing_or_failed under require_any

API functionalities are guarded by many unit-tests; group_verifier_test and all_verifier_test.

thanks for the confirm, just want to make sure we don't accidentally break the API behavior, also found the tests covering the Istio use cases in AllowMissingInOrListTest and AllowMissingInAndOfOrListTest so we should be good here.

Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@qiwzhang
Copy link
Copy Markdown
Contributor Author

Thanks @yangminzhu for your review. Updated the PR. PTAL

Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Race Condition when multiple remote jwks providers defined along with allow_missing_or_failed

4 participants