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 extra validation if audience claim exists in the Bearer token #11059

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dtsek
Copy link

@dtsek dtsek commented Mar 19, 2024

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)
#11018

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)

@shreemaan-abhishek
Copy link
Contributor

shreemaan-abhishek commented Mar 20, 2024

@dtsek could you please add test cases as well?

@dtsek
Copy link
Author

dtsek commented Mar 20, 2024

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?

@shreemaan-abhishek
Copy link
Contributor

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.

@shreemaan-abhishek
Copy link
Contributor

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

@dtsek
Copy link
Author

dtsek commented Mar 22, 2024

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.

@markusmueller
Copy link
Contributor

markusmueller commented Mar 30, 2024

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?

@dtsek
Copy link
Author

dtsek commented Apr 4, 2024

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
    ?

@markusmueller
Copy link
Contributor

markusmueller commented Apr 9, 2024

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"
    }
  ]
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants