You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "dtsek (via GitHub)" <gi...@apache.org> on 2024/03/19 08:21:28 UTC
[PR] feat(openid-connect): Add extra validation if audience claim exists in the Bearer token [apisix]
dtsek opened a new pull request, #11059:
URL: https://github.com/apache/apisix/pull/11059
### Description
>The "aud" (audience) claim identifies the recipients that the JWT is intended for.
>https://www.rfc-editor.org/rfc/rfc7519#section-4.1.3
Issue: `openid-connect` currently does not validate if the audience claim matches the client_id which means that every valid JWT can authenticate using this plugin, even if it was generated for a different client.
Solution: This MR enhances the `introspect` function by adding a simple validation that the audience claim `aud` is the same as the provided `client_id`.
Note: There is no documentation that can be updated, unless a note should be added to explain this functionality in the page of the plugin.
Fixes # (issue)
https://github.com/apache/apisix/issues/11018
### Checklist
- [X] I have explained the need for this PR and the problem it solves
- [X] 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](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(openid-connect): Add extra validation if audience claim exists in the Bearer token [apisix]
Posted by "markusmueller (via GitHub)" <gi...@apache.org>.
markusmueller commented on PR #11059:
URL: https://github.com/apache/apisix/pull/11059#issuecomment-2045513351
Yes, I'm suggesting additional config variables and reuse of existing methods. Let me illustrate and hope it gets more clear :-)
The plugin is using [lua-resty-openidc](https://github.com/zmartzone/lua-resty-openidc/tree/9f3a4fcade930f6f38ee0cb43cabf50cebffbcc9) for JWT validation.
Instead of implementing your own claim validation the idea is to reuse existing methods:
1. Find a JSON representation of the JWT validators supported by `lua-resty-openidc` and add it to the plugin config (validators are implemented in `lua-resty-jwt` see for example: https://github.com/SkyLothar/lua-resty-jwt/blob/master/t/validators.t)
3. Instantiate the validator(s) according to the configuration, chain them if its multiple instances
4. Pass the validator(s) to https://github.com/apache/apisix/blob/4df549c21278fbb99a1efba160b2ac9119ce4e1f/apisix/plugins/openid-connect.lua#L373https://docs.github.com/github/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax
Rough draft of the additional config properties:
```{
"type": "array",
"items": {
"type": "object",
"properties": {
"type": {
"type": "string",
"title": "type",
"enum": [
"matches, equals"
]
},
"argument": {
"type": "string",
"title": "argument",
"description": "Argument for the validator, for example validator of type matches is accepting a regex",
"minLength": 1
},
"claim": {
"type": "string",
"title": "claim",
"description": "Name of the claim the validator will be applied to",
"minLength": 1
}
},
"title": "validator",
"required": [
"argument",
"claim",
"type"
]
},
"title": "jwt-validators",
"description": "Array of JWT validators applied to the JWT token"
}
```
Example config:
```
{
"jwt-validators" : [
{
"claim" : "aud",
"type": "equals",
"argument" : "your_client_id"
}
]
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(openid-connect): Add extra validation if audience claim exists in the Bearer token [apisix]
Posted by "Baoyuantop (via GitHub)" <gi...@apache.org>.
Baoyuantop commented on PR #11059:
URL: https://github.com/apache/apisix/pull/11059#issuecomment-3434764087
Hi all, the issue described in this PR has been resolved in https://github.com/apache/apisix/pull/11987 and released in version 3.12.0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(openid-connect): Add extra validation if audience claim exists in the Bearer token [apisix]
Posted by "dtsek (via GitHub)" <gi...@apache.org>.
dtsek commented on PR #11059:
URL: https://github.com/apache/apisix/pull/11059#issuecomment-2009518757
Hello @shreemaan-abhishek , could you give me some extra information regarding testing since I didn't see something relevant apart from the other tests?
In order to test this feature, I will need an IdP that can provide Bearer tokens for different clients so I can prove that the one with the valid `aud` field is the only one that passes.
Is there something available that I can use from your side? Otherwise I could take a look at something like https://www.oauth.com/playground/oidc.html but it will take some time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(openid-connect): Add extra validation if audience claim exists in the Bearer token [apisix]
Posted by "dtsek (via GitHub)" <gi...@apache.org>.
dtsek commented on PR #11059:
URL: https://github.com/apache/apisix/pull/11059#issuecomment-2036979664
Hi @markusmueller, so you propose something like additional variables in the config of the plugin to
1) enable verification of audience
2) specify the field that should be checked against aud
?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(openid-connect): Add extra validation if audience claim exists in the Bearer token [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on PR #11059:
URL: https://github.com/apache/apisix/pull/11059#issuecomment-2009203425
@dtsek the CI is faililng and could you please add test cases as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(openid-connect): Add extra validation if audience claim exists in the Bearer token [apisix]
Posted by "dtsek (via GitHub)" <gi...@apache.org>.
dtsek commented on PR #11059:
URL: https://github.com/apache/apisix/pull/11059#issuecomment-2014829171
Hello @shreemaan-abhishek , unfortunately I cannot provide for the time being a test case because in https://github.com/apache/apisix/blob/c0e3d9150f06c3140a52d145782085d26bc1ea67/ci/pod/keycloak/kcadm_configure_university.sh there is only one client being configured and I would like to avoid modifying many services outside of the plugin.
In order to create a test case to prove the bugfix I will need two valid clients. The correct one should get allowed by `openid-connect` while the other one should get denied. This is the scenario I was facing in my organization's environment and gave the inspiration for the security fix.
I am also having some issues testing locally the test cases, but it is more related to the needed openresty modules.
I can always provide some proof from my instance, but I imagine this is not enough for the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(openid-connect): Add extra validation if audience claim exists in the Bearer token [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on PR #11059:
URL: https://github.com/apache/apisix/pull/11059#issuecomment-2011150183
I forgot to share this link that shows how test cases can be written: https://apisix.apache.org/docs/apisix/internal/testing-framework/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(openid-connect): Add extra validation if audience claim exists in the Bearer token [apisix]
Posted by "markusmueller (via GitHub)" <gi...@apache.org>.
markusmueller commented on PR #11059:
URL: https://github.com/apache/apisix/pull/11059#issuecomment-2028423173
I came across this PR while working on another issue in the openid plugin. While I understand the need for an audience check in the use case mentioned I have concerns about the solution proposed with this change.
As far as I know neither openid nor oauth2 specify a format, existence or content of an audience claim (e.g. https://www.rfc-editor.org/rfc/rfc7519#section-4.1.3. `The interpretation of audience values is generally application specific.`).
Some IDPs use JWTs using the client_id as value of the aud claim, but that's not a requirement and the case for every IDP.
Hard-coding the check to the client_id configured in the plugin would break the plugin for some IDPs which are not using the client_id in an audience claim but still conforming to the spec.
The apisix openid plugin is using lua-resty-openidc to verify access tokens, the method `jwt_verify` is accepting jwt-validators which can be dynamic. It might be a better way to add a configurable audience/general claim validation to solve this problem?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(openid-connect): Add extra validation if audience claim exists in the Bearer token [apisix]
Posted by "shreemaan-abhishek (via GitHub)" <gi...@apache.org>.
shreemaan-abhishek commented on PR #11059:
URL: https://github.com/apache/apisix/pull/11059#issuecomment-2010046068
> could you give me some extra information regarding testing since I didn't see something relevant apart from the other tests?
You can use the config that didn't work before your bugfix to show that it works now in the tests. You can take motivation from the existing test cases in `t/plugins/openid-connect.t` and other test cases.
> I will need an IdP that can provide Bearer tokens for different clients so I can prove that the one with the valid aud field is the only one that passes.
You can use existing services defined as docker compose applications [here](https://github.com/apache/apisix/blob/master/ci/pod/docker-compose.plugin.yml), define new services (not recommended because #11068) or use a mock server.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(openid-connect): Add extra validation if audience claim exists in the Bearer token [apisix]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #11059:
URL: https://github.com/apache/apisix/pull/11059#issuecomment-2241552158
This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(openid-connect): Add extra validation if audience claim exists in the Bearer token [apisix]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #11059: feat(openid-connect): Add extra validation if audience claim exists in the Bearer token
URL: https://github.com/apache/apisix/pull/11059
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org