-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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: ", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"?
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. |
Description
Add JWT audience authentication to the OpenID Connect plugin, which allows:
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 inlua-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