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.

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

fix(openid-connect): use_jwks breaking authentication header #10670

Conversation

jesse-r-s-hines
Copy link
Contributor

@jesse-r-s-hines jesse-r-s-hines commented Dec 19, 2023

Description

When using the openid-connect plugin with bearer_only as false, the plugin will normally authenticate both requests with the session cookie and requests with an explicit Authorization: Bearer header. This is very useful to allow both browser based and programmatic access to some resource.

However, if you set use_jwks instead of explicitly setting the public key, requests with a proper Authorization: Bearer header still get the 302 Found response.

Fix by adding conf.use_jwks to the check.

Fixes #10669

@jesse-r-s-hines jesse-r-s-hines force-pushed the fix-openid-connect-use-jwks-auth-header branch from cca43f2 to 183193f Compare December 19, 2023 15:54
@jesse-r-s-hines jesse-r-s-hines changed the title openid-connect: fix use_jwks breaking authentication header fix: fix openid-connect use_jwks breaking authentication header Dec 19, 2023
@jesse-r-s-hines jesse-r-s-hines force-pushed the fix-openid-connect-use-jwks-auth-header branch from 183193f to 4e96732 Compare December 19, 2023 20:01
@monkeyDluffy6017 monkeyDluffy6017 changed the title fix: fix openid-connect use_jwks breaking authentication header fix(openid-connect): use_jwks breaking authentication header Dec 21, 2023
@monkeyDluffy6017
Copy link
Contributor

@jesse-r-s-hines
Copy link
Contributor Author

I'm still a bit confused on how the APISIX tests work, but I've added a test that should check for the bug in #10670.

@jesse-r-s-hines jesse-r-s-hines force-pushed the fix-openid-connect-use-jwks-auth-header branch 2 times, most recently from 222c842 to 0c3801a Compare December 28, 2023 16:28
@jesse-r-s-hines
Copy link
Contributor Author

I can't replicate the linting failure by running make lint locally, how do I get it to tell me what's causing the failure?

@kayx23
Copy link
Member

kayx23 commented Dec 29, 2023

@jesse-r-s-hines
Copy link
Contributor Author

I've tried that, reindex makes no change to the file when I run it.

@@ -1241,7 +1345,7 @@ passed



=== TEST 33: Access route to validate "x-userinfo" in request header
=== TEST 35: Access route to validate "x-userinfo" in request header
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest put your newly added test cases to the end of the file, so that you won't need to modify the index of the rest test cases.
The index of test cases below this one are not correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh of course, I'm not sure how I missed that. I've updated the test file.

@monkeyDluffy6017 monkeyDluffy6017 added wait for update wait for the author's response in this issue/PR and removed need test cases user responded labels Jan 2, 2024
@jesse-r-s-hines jesse-r-s-hines force-pushed the fix-openid-connect-use-jwks-auth-header branch from 0c3801a to 90f9828 Compare January 2, 2024 15:46
If you use jwks instead of explicitly setting the public key, requests
with a proper `Authorization: Bearer` header would still get the
`302 Found` response. Fix by adding `conf.use_jwks` to the check.
@jesse-r-s-hines jesse-r-s-hines force-pushed the fix-openid-connect-use-jwks-auth-header branch from 90f9828 to 07e0cb3 Compare January 2, 2024 16:23
@jesse-r-s-hines
Copy link
Contributor Author

I believe the linting should pass now.

@kayx23 kayx23 removed the wait for update wait for the author's response in this issue/PR label Jan 4, 2024
@moonming moonming merged commit 2eabf70 into apache:master Jan 9, 2024
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: Openid-connect plugin use_jwks breaks Authorization headers when bearer_only is true
4 participants