Add the support of bypassing JWT authn for CORS preflight requests#6181
Add the support of bypassing JWT authn for CORS preflight requests#6181lei-tang wants to merge 2 commits intoenvoyproxy:masterfrom
Conversation
lei-tang
force-pushed
the
support-bypass-jwt-authn-for-cors
branch
from
788fcd3 to
b6f5993
Compare
lei-tang
force-pushed
the
support-bypass-jwt-authn-for-cors
branch
from
b6f5993 to
180cd2d
Compare
There was a problem hiding this comment.
nit: getStringView instead of c_str()
There was a problem hiding this comment.
I think this bypass too much request by blindly accepting all OPTIONS method, OPTIONS are not only used for CORS, it need additional check or guarded by configuration.
There was a problem hiding this comment.
Do you have a recommendation for additional check?
There was a problem hiding this comment.
check all three header existence? https://developer.mozilla.org/en-US/docs/Glossary/Preflight_request
There was a problem hiding this comment.
btw I think this can be achieved with config change and deosn't require code change, no? @qiwzhang
Per the spec http://www.w3.org/TR/cors/#cross-origin-request-with-preflight-0, CORS pre-flight requests shouldn't include user credentials. Signed-off-by: Lei Tang <32078630+lei-tang@users.noreply.github.com>
lei-tang
force-pushed
the
support-bypass-jwt-authn-for-cors
branch
from
180cd2d to
e1219af
Compare
Signed-off-by: Lei Tang <32078630+lei-tang@users.noreply.github.com>
lei-tang
force-pushed
the
support-bypass-jwt-authn-for-cors
branch
from
2185d62 to
ed87358
Compare
|
@lizan you are right. We don't need to make such code change. If users want to bypass all OPTIONS, they can create a rule in the config to match all OPTIONS and bypass them. |
|
@arpitshah29 you can use route_match to specify a match condition, and by not providing a jwt_requirement to bypass jwt checking. I believe you can create a route_match to match all option calls. |
|
@qiwzhang - Thanks for quick response, While I understand what you are saying - but IMHO route match would be handy for a one-off scenario where jwt_authn needs suppression like health checker and so on... what about the case where we can have thousands of routes? won't that be an overhead to route match each and every route for exclusion? Appreciate your feedback. |
|
Route match is very flexible. For example if has path_prefix, and regex, if you define your path_prefix as "/", it will match all paths. |
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
4 participants
Description: Per the spec http://www.w3.org/TR/cors/#cross-origin-request-with-preflight-0,
CORS pre-flight requests shouldn't include user credentials. This PR adds the support of bypassing JWT authn for CORS flight requests.
Risk Level: low
Testing: test is in test/extensions/filters/http/jwt_authn/filter_integration_test.cc
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]