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

feat(openid-connect): add jwt audience validator #11987

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

bzp2010
Copy link
Contributor

@bzp2010 bzp2010 commented Feb 22, 2025

Description

Add JWT audience authentication to the OpenID Connect plugin, which allows:

  • Asserts that the claim must exist, otherwise the request is rejected.
  • Asserts that it should be equal to or contain the client_id to comply with the OIDC specification requirements, otherwise the request is rejected.
  • The claim can be customized.

Fixes #11968 #11059

One of the developers in #11059 mentioned that it is possible to use some of the APIs in jwt-validators to implement JWT validation in lua-resty-openidc, but it doesn't work, that library only works with local verification that uses a public key, and not with the Introspection API. We have to implement the functionality directly in the plugin code to support it in both scenarios.

To keep compatibility, these features are not turned on by default and it is up to you to decide if you want to turn them on. Although the OIDC spec requires this to be the default behavior.

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@bzp2010 bzp2010 marked this pull request as ready for review February 22, 2025 18:21
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. enhancement New feature or request labels Feb 22, 2025
moonming
moonming previously approved these changes Feb 22, 2025
nic-chen
nic-chen previously approved these changes Feb 23, 2025
Copy link
Member

@nic-chen nic-chen left a comment

Choose a reason for hiding this comment

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

LGTM

},
match_with_client_id = {
type = "boolean",
description = "audience must euqal to or includes client_id",
Copy link
Member

Choose a reason for hiding this comment

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

From the code, it can only be equals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, if the match is true but not required, the correlation check with client_id is only performed if the aud value is not empty.

return 403, core.json.encode(error_response)
end
elseif conf.client_id ~= audience_value then
core.log.error("OIDC introspection failed: ",
Copy link
Member

Choose a reason for hiding this comment

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

Compared to error, warn level seems more appropriate for these logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, but the rest of the code uses error, so I'm choosing to follow the precedent, should we break that "convention"?

@bzp2010
Copy link
Contributor Author

bzp2010 commented Feb 23, 2025

We should consider the design of the configuration items more carefully, as it may not align with ergonomic best practices. I'd appreciate your feedback. From all the people who are concerned about this, not just the maintainers themselves.

@bzp2010 bzp2010 dismissed stale reviews from nic-chen and moonming via be0eb1c February 23, 2025 06:04
@bzp2010 bzp2010 requested a review from nic-chen February 23, 2025 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add extra validation if audience claim exists in the Bearer token
3 participants