@dtsek could you please add test cases as well?

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.

An alternative would be to provide manually two JWTs, one with a correct aud and one with different aud, but is it enough?

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, define new services (not recommended because #11068) or use a mock server.

I forgot to share this link that shows how test cases can be written: https://apisix.apache.org/docs/apisix/internal/testing-framework/

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.

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 are using JWTs and the client_id as value of the aud claim, but that's not a requirement and not 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 (see https://github.com/zmartzone/lua-resty-openidc/blob/9f3a4fcade930f6f38ee0cb43cabf50cebffbcc9/lib/resty/openidc.lua#L1820). It might be a better way to add a configurable audience/general claim validation to solve this problem?

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
   ?

Yes @dtsek , I'm suggesting additional config variables and reuse of existing methods. Let me illustrate and hope it makes it more clear :-)

The plugin is using lua-resty-openidc 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)
2. Instantiate the validator(s) according to the configuration, chain them if its multiple instances
3. 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"
}

Example config:

{
  "jwt-validators" : [
    {
      "claim" : "aud",
      "type": "equals",
      "argument" : "your_client_id_or_any_other_value"
    }
  ]
}

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the [email protected] list. Thank you for your contributions.

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.

Hi all, the issue described in this PR has been resolved in #11987 and released in version 3.12.0.