Skip to content

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

Merged
moonming merged 2 commits into
apache:masterfrom
jesse-r-s-hines:fix-openid-connect-use-jwks-auth-header
Jan 9, 2024
Merged

fix(openid-connect): use_jwks breaking authentication header#10670
moonming merged 2 commits into
apache:masterfrom
jesse-r-s-hines:fix-openid-connect-use-jwks-auth-header

Conversation

@jesse-r-s-hines

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

Copy link
Copy Markdown
Contributor

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 changed the title openid-connect: fix use_jwks breaking authentication header fix: fix openid-connect use_jwks breaking authentication header Dec 19, 2023
@juststillthinking juststillthinking changed the title fix: fix openid-connect use_jwks breaking authentication header fix(openid-connect): use_jwks breaking authentication header Dec 21, 2023
@juststillthinking

Copy link
Copy Markdown
Contributor

@jesse-r-s-hines

Copy link
Copy Markdown
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

Copy link
Copy Markdown
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

kayx23 commented Dec 29, 2023

Copy link
Copy Markdown
Member

@jesse-r-s-hines

Copy link
Copy Markdown
Contributor Author

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

Comment thread t/plugin/openid-connect.t Outdated


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

Copy link
Copy Markdown
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
Copy Markdown
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.

@juststillthinking juststillthinking 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
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

Copy link
Copy Markdown
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

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