envoyproxy
jwt_authn: Set metadata irrespective of success/failure of JWT Verification
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
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.
:cat:
Caused by: https://github.com/envoyproxy/envoy/pull/34335 was opened by arulthileeban.
![repokitteh-read-only[bot] avatar](https://avatars.githubusercontent.com/in/16729?v=4 "Published by a pub.dev verified publisher"){kind=small}
@TAOXUY @lizan Could you please take a look? It's a simple fix for an issue that doesn't allow us to use "failed_status_in_metadata" config currently
@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!
/wait-any
@dio @qiwzhang Looks like you touched this feature in #18140 and #4707. The logic here has been successfully verified JWT . wondering what is your opinions on this PR? Thanks in advance!
@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
