jwt_authn: Set metadata irrespective of success/failure of JWT Verification#34335
jwt_authn: Set metadata irrespective of success/failure of JWT Verification#34335tyxia merged 1 commit intoenvoyproxy:mainfrom
Conversation
…cation Previously, metadata was only set for successful verification, restricting "failed_status_in_metadata". This change removes the restriction, allowing both "payload_in_metadata" and "failed_status_in_metadata" to be set as needed. "payload_in_metadata" is set only by AuthenticatorImpl:handleGoodJwt() which makes this restriction unnecessary earlier. Signed-off-by: Arul Thileeban Sagayam <arul.thilee@gmail.com>
|
Hi @arulthileeban, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
|
@TAOXUY can you PTAL? |
|
@kyessenov is OOO and probably won't be able to look at this PR. @TAOXUY Friendly ping on this as you are the code-owner. I will pass it onto a maintainer review once it is LGTY. Thanks! |
|
@tyxia Anything that can be done from my end to help get this PR going? It has been stuck for quite a while. If you are able to add the test I've added, to the upstream code, the setExtractedData callback will fail which is what sets the failed_status_in_metadata. This change is required to make this config work. Also, the "successfully verified JWT" logic was likely there earlier since metadata was only set for successful JWTs through "payload_in_metadata". The failed_status_in_metadata logic came later and this check was probably overlooked when adding that. The current tests also miss this. The payload_in_metadata is still set only by handleGoodJWT which makes this check redundant. HandleGoodJWT again is only called when a JWT cache hit happens or if verification succeeds |
There was a problem hiding this comment.
Thanks for detailed information @arulthileeban !
This CL LGTM, but I am not familiar with this extension so I think will need the extension owner/other maintainer's approval
@lizan Would you get a chance to look at this PR. Also cced @danieldradware (who added failed_status_to_metadata initially) and @qiwzhang(who reviewed the initial PR)
|
friendly ping @tyxia (since lizan has left maintainer rotation ) |
tyxia
left a comment
tyxia
left a comment
There was a problem hiding this comment.
LGTM
I think it is also good to have a pass from code owner/existing users
|
Would it be possible to get this fix into the upcoming release? |
kyessenov
left a comment
kyessenov
left a comment
There was a problem hiding this comment.
This makes sense to me. I'm not an extension owner, but I've been looking in depth into it recently.
8 participants
Commit Message: jwt_authn: Set metadata irrespective of success/failure of JWT Verification
Previously, metadata was only set for successful JWT verification, restricting "failed_status_in_metadata". This change removes the condition, allowing both "payload_in_metadata" and "failed_status_in_metadata" to be set as needed.
If this condition was added to restrict setting "payload_in_metadata" only on successful verification, it is unnecessary since "payload_in_metadata" is set only by AuthenticatorImpl:handleGoodJwt().
Risk Level: Low
Testing: Unit tests
Docs Changes: N/A
Release Notes:
Platform Specific Features: N/A
Fixes #32813