search

inrupt / solid-client-authn-js

A client library for authenticating with Solid
https://solid-client-authn-js.vercel.app
Other
69 stars 42 forks source link

No support for IdPs using multiple JWK certificates #1799

Open dteleguin opened 2 years ago

dteleguin commented 2 years ago

Impacted package

Which packages do you think might be impacted by the bug ?

Bug description

It is not uncommon for OIDC IdPs to use multiple JWK keys, resulting in the array published at jwks_uri to have more than one element. For example:

(While Google, MS and Okta are of low relevance here, Ping, Keycloak and Connect2ID could potentially become Solid-OIDC compliant in the near future as they either already ship the necessary features like DPoP and PKCE or will start shipping them soon.)

Currently, this is not supported by the library as it would just pick the first key (IRedirectHandler.ts:73):

 // The JWKS should only contain the current key for the issuer.
  let jwk: JWK;
  try {
    jwk = (await jwksResponse.json()).keys[0] as JWK;
  } catch (e) {
    throw new Error(
      `Malformed JWKS for [${issuerIri}] at [${jwksIri}]: ${
        (e as WithMessage).message
      }`
    );
  }
  return jwk;

The statement about JWKS having to contain only the current key seems erroneous to me; I couldn't find any specification that would mandate the only key. Actually, the kid of the key should be matched against the corresponding header claim of the access/ID token.

@justinwb @jamiefiedler

Expected result

Successful validation of acess/ID tokens minted by an IdP using multiple keys.

Actual result

Validation would only succeed if the token was signed by the first key from the published JWKS set.

Environment

  System:
    OS: Linux 5.10 Mageia 8
    CPU: (16) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 1.80 GB / 15.48 GB
    Container: Yes
    Shell: 5.1.4 - /bin/bash
  Binaries:
    Node: 14.17.6 - /usr/bin/node
    npm: 6.14.15 - /usr/bin/npm
  Browsers:
    Chrome: 95.0.4638.69
    Firefox: 91.3.0esr
  npmPackages:
    @babel/core: ^7.12.16 => 7.12.16 
    @babel/plugin-proposal-class-properties: ^7.12.13 => 7.12.13 
    @babel/preset-react: ^7.12.13 => 7.12.13 
    @datapunt/matomo-tracker-react: ^0.3.1 => 0.3.1 
    @date-io/date-fns: ^1.3.13 => 1.3.13 
    @inrupt/eslint-config-base: ^0.0.4 => 0.0.4 
    @inrupt/eslint-config-react: ^0.0.4 => 0.0.4 
    @inrupt/prism-react-components: ^0.13.7 => 0.13.7 
    @inrupt/solid-client: ^1.15.0 => 1.15.0 
    @inrupt/solid-client-access-grants: ^0.3.3-fix2258-verification-endpoint-discovery-1412767200-263-1635864188.0 => 0.3.3-fix2258-verification-endpoint-discovery-1412767200-263-1635864188.0 
    @inrupt/solid-client-authn-browser: ^1.8.2 => 1.8.2 
    @inrupt/solid-ui-react: ^2.3.1 => 2.3.1 
    @material-ui/core: ^4.11.3 => 4.11.3 
    @material-ui/icons: ^4.11.2 => 4.11.2 
    @material-ui/lab: ^4.0.0-alpha.57 => 4.0.0-alpha.57 
    @material-ui/pickers: ^3.3.10 => 3.3.10 
    @sentry/node: ^6.1.0 => 6.1.0 
    @sentry/react: ^6.1.0 => 6.1.0 
    @sentry/webpack-plugin: ^1.14.0 => 1.14.0 
    @solid/lit-prism-patterns: ^0.13.7 => 0.13.7 
    @solid/lit-prism-theme-sdk-default: ^0.13.7 => 0.13.7 
    @testing-library/dom: ^7.29.4 => 7.29.4 
    @testing-library/jest-dom: ^5.11.9 => 5.11.9 
    @testing-library/react: ^11.2.5 => 11.2.5 
    @testing-library/react-hooks: ^5.1.2 => 5.1.2 
    @testing-library/user-event: ^12.7.1 => 12.7.1 
    @types/jest: ^26.0.20 => 26.0.20 
    @types/react: ^17.0.2 => 17.0.2 
    @types/react-table: ^7.0.28 => 7.0.28 
    @typescript-eslint/eslint-plugin: ^3.10.1 => 3.10.1 
    @typescript-eslint/parser: ^3.10.1 => 3.10.1 
    babel-eslint: ^10.1.0 => 10.1.0 
    babel-jest: ^26.6.3 => 26.6.3 
    date-fns: ^2.23.0 => 2.23.0 
    encoding: ^0.1.13 => 0.1.13 
    eslint: ^7.20.0 => 7.20.0 
    eslint-config-airbnb: ^18.2.0 => 18.2.0 
    eslint-config-airbnb-base: ^14.2.0 => 14.2.0 
    eslint-config-next: ^11.0.1 => 11.0.1 
    eslint-config-prettier: ^6.15.0 => 6.15.0 
    eslint-plugin-babel: ^5.3.1 => 5.3.1 
    eslint-plugin-import: ^2.22.1 => 2.22.1 
    eslint-plugin-jest: ^23.17.1 => 23.20.0 
    eslint-plugin-jsx-a11y: ^6.4.1 => 6.4.1 
    eslint-plugin-license-header: ^0.2.0 => 0.2.0 
    eslint-plugin-prettier: ^3.1.4 => 3.1.4 
    eslint-plugin-react: ^7.21.5 => 7.21.5 
    eslint-plugin-react-hooks: ^4.2.0 => 4.2.0 
    http-link-header: ^1.0.3 => 1.0.3 
    husky: ^4.3.7 => 4.3.7 
    jest: ^26.6.3 => 26.6.3 
    jest-localstorage-mock: ^2.4.3 => 2.4.3 
    jest-mock-extended: ^1.0.10 => 1.0.10 
    jest-raw-loader: ^1.0.1 => 1.0.1 
    jsdom: ^16.4.0 => 16.4.0 
    jsdom-global: 3.0.2 => 3.0.2 
    jss: ^10.5.1 => 10.5.1 
    jss-preset-default: ^10.5.1 => 10.5.1 
    license-checker: ^25.0.1 => 25.0.1 
    next: ^11.0.1 => 11.0.1 
    next-runtime-dotenv: ^1.4.0 => 1.4.0 
    nock: ^13.1.1 => 13.1.1 
    node-mocks-http: ^1.10.1 => 1.10.1 
    prettier: ^2.2.1 => 2.2.1 
    prop-types: ^15.7.2 => 15.7.2 
    raw-loader: ^4.0.2 => 4.0.2 
    rdf-namespaces: ^1.9.2 => 1.9.2 
    react: ^17.0.2 => 17.0.2 
    react-dom: ^17.0.2 => 17.0.2 
    react-id-generator: ^3.0.1 => 3.0.1 
    react-jss: ^10.4.0 => 10.4.0 
    react-table: ^7.6.3 => 7.6.3 
    react-test-renderer: ^17.0.1 => 17.0.1 
    react-transition-group: ^4.4.1 => 4.4.1 
    swr: ^0.4.2 => 0.4.2 
    typescript: ^4.4.3 => 4.4.3 
    uuid: ^8.3.1 => 8.3.1 
    vercel: ^21.2.3 => 21.2.3 
    whatwg-fetch: ^3.5.0 => 3.5.0 
  npmGlobalPackages:
    npm: 6.14.15
acoburn commented 2 years ago

Worth noting that it is not sufficient to match only against the kid value. The kty value also needs to match.

I have encountered OPs in the wild with multiple keys in a JWKS that use the same kid but which have different kty values, and the JWK specification (RFC 7517) specifically allows this.

nicolasmondada commented 2 years ago

Hi @dteleguin ,

Thanks for your suggestion. This has been added to our backlog and will be considered as an improvement to be added in in the future.

All the best, Nick.-