Skip to content
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.

Sign up for GitHub

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

Jump to bottom

jwt_authn: fix a bug where JWT with wrong issuer is allowed in allow_missing case #15194

Merged
merged 3 commits into from Feb 26, 2021
Merged

jwt_authn: fix a bug where JWT with wrong issuer is allowed in allow_missing case #15194

merged 3 commits into from Feb 26, 2021

Conversation

qiwzhang
Copy link
Contributor

@qiwzhang qiwzhang commented Feb 25, 2021 •
edited

When allow_missing is used inside RequiresAny, the requests with JWT with wrong issuer are accepted. This is a bug, allow_missing should only allow requests without any JWT.

This change fixed the above issue by preserving JwtUnknownIssuer in allow_missing case.

Change details:

  • JwtUnkownIssuer error got converted to JwtMissing in error aggregation inside VerifyAny object. This is the root cause. Such conversion is not needed.
  • Fix is to preserve JwtUknownIssuer in the error aggregation.

Risk Level: Low
Testing: unit-tested
Docs Changes: N/A
Release Notes: Yes

@qiwzhang
Retain JwtUnknownIssuer error for allowing_missing
fc1644e 
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@qiwzhang
update release note
8f09154 
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@qiwzhang qiwzhang requested a review from lizan as a code owner February 25, 2021 19:52
@qiwzhang
Copy link
Contributor Author

@yangminzhu could you help to review it?

@htuch
Copy link
Member

htuch commented Feb 25, 2021

@lizan tagging for a pass from you, thanks!

lizan
lizan previously approved these changes Feb 25, 2021
View reviewed changes
@qiwzhang
Update comment
57a2fb4 
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@qiwzhang qiwzhang dismissed stale reviews from yangminzhu and lizan via 57a2fb4 February 25, 2021 20:59
yangminzhu
yangminzhu approved these changes Feb 25, 2021
View reviewed changes
Copy link
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.

LGTM, thanks for the fix!

@asraa asraa mentioned this pull request Feb 25, 2021
Merged
@asraa asraa merged commit ea39e3c into envoyproxy:main Feb 26, 2021
@Shikugawa Shikugawa added the backport/approved Approved backports to stable releases label Feb 26, 2021
@myidpt myidpt mentioned this pull request Feb 26, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yangminzhu yangminzhu yangminzhu approved these changes

@asraa asraa asraa approved these changes

@lizan lizan lizan left review comments

Assignees

@lizan lizan

Labels
backport/approved Approved backports to stable releases
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@qiwzhang @htuch @lizan @yangminzhu @asraa @Shikugawa