Re: [PR] feat(openid-connect): Add extra validation if audience claim exists in the Bearer token [apisix]
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-unsubscr...@apisix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] feat(openid-connect): Add extra validation if audience claim exists in the Bearer token [apisix]
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-unsubscr...@apisix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] feat(openid-connect): Add extra validation if audience claim exists in the Bearer token [apisix]
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-unsubscr...@apisix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] feat(openid-connect): Add extra validation if audience claim exists in the Bearer token [apisix]
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-unsubscr...@apisix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] feat(openid-connect): Add extra validation if audience claim exists in the Bearer token [apisix]
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-unsubscr...@apisix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] feat(openid-connect): Add extra validation if audience claim exists in the Bearer token [apisix]
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-unsubscr...@apisix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] feat(openid-connect): Add extra validation if audience claim exists in the Bearer token [apisix]
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-unsubscr...@apisix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] feat(openid-connect): Add extra validation if audience claim exists in the Bearer token [apisix]
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-unsubscr...@apisix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] feat(openid-connect): Add extra validation if audience claim exists in the Bearer token [apisix]
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-unsubscr...@apisix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] feat(openid-connect): Add extra validation if audience claim exists in the Bearer token [apisix]
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-unsubscr...@apisix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
