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

Edge runtime compatible jwt plugin #3449

Open
aarne opened this issue Oct 24, 2024 · 8 comments
Open

Edge runtime compatible jwt plugin #3449

aarne opened this issue Oct 24, 2024 · 8 comments

Comments

@aarne
Copy link

aarne commented Oct 24, 2024

Is your feature request related to a problem? Please describe.

Currently @graphql-yoga/plugin-jwt is incompatible with most edge runtimes as it relies heavily on nodejs internals not available in js. This is coming from reliance on jsonwebtoken library.

Describe the solution you'd like

Looking at the code there are multiple possible options to replace the jsonwebtoken with for example jose, but i'm not sure if there is any appetite for doing this?

If this is something you would consider then im happy to look into this a bit more.

@aarne
Copy link
Author

aarne commented Oct 24, 2024

We can also think of bring your own jwt library type of integrations mby?

@EmrysMyrddin
Copy link
Collaborator

EmrysMyrddin commented Oct 28, 2024

Hi ! Thank you for raising this issue :-)

I'm really not against changing underlying implementation used for JWT. The only problem we will face, is that it will introduce breaking change in the configuration since we expose a lot of configuration related directly to the api of jsonwebtoken specifically.

That being said, breaking changes are not an hard stop. It's not our first major bump of this plugin after all. We just have to evaluate if the benefits are sufficient to justify breaking most "not that simple" configurations.

In my opinion, it's worth it if this allows Yoga to be more portable and runtime agnostic, which is one of it's goal.

@dotansimha What do you think ? You are the last one how heavily worked on this plugin.

@aarne
Copy link
Author

aarne commented Oct 31, 2024

The only 2 exposed part of jsonwebtoken/jwk is VerifyOptions and JwksClient.Options interfaces, as defined below

interface VerifyOptions {
  algorithms?: Algorithm[] | undefined;
  audience?: string | RegExp | Array<string | RegExp> | undefined;
  clockTimestamp?: number | undefined;
  clockTolerance?: number | undefined;
  /** return an object with the decoded `{ payload, header, signature }` instead of only the usual content of the payload. */
  complete?: boolean | undefined;
  issuer?: string | string[] | undefined;
  ignoreExpiration?: boolean | undefined;
  ignoreNotBefore?: boolean | undefined;
  jwtid?: string | undefined;
  /**
   * If you want to check `nonce` claim, provide a string value here.
   * It is used on Open ID for the ID Tokens. ([Open ID implementation notes](https://openid.net/specs/openid-connect-core-1_0.html#NonceNotes))
   */
  nonce?: string | undefined;
  subject?: string | undefined;
  maxAge?: string | number | undefined;
  allowInvalidAsymmetricKeyTypes?: boolean | undefined;
}

interface Options {
    jwksUri: string;
    rateLimit?: boolean;
    cache?: boolean;
    cacheMaxEntries?: number;
    cacheMaxAge?: number;
    jwksRequestsPerMinute?: number;
    proxy?: string;
    requestHeaders?: Headers;
    timeout?: number;
    requestAgent?: HttpAgent | HttpsAgent;
    fetcher?(jwksUri: string): Promise<{ keys: any }>;
    getKeysInterceptor?(): Promise<JSONWebKey[]>;
}

Jose used JWTVerifyOptions and RemoteJWKSetOptions is in essence very similar. But there are things that are a bit off ... like nonce and complete etc that make sense in a low lever interface but not to be exposed for the plugin.

As i see we have 3 options:

  1. Implement all options from VerifyOptions in jose
    • not breaking change
    • too much responsibility for the plugin
    • interface is not clean for our usecase
  2. Create a simpler verification options interface that we own
    • breaking change
    • also too much responsibility for the plugin
    • but at least the interface is clean (unless we get the abstraction wrong and then we need to start changing it again)
  3. Allow users to pass in the verify function and use decode from jose
    • also breaking change? unless we can somehow default back to jsonwebtoken?
    • more flexible interface, when needed

the new interface would looks something like this:

type JwtPluginOptions = {
 singingKeyProviders: AtleastOneItem<GetSigningKeyFunction>;
 tokenLookupLocations?: AtleastOneItem<ExtractTokenFunction>;
 tokenVerification?: VerifyOptions; // if we don't remove this then we can initiate the tokenVerificationFunction with jsonwebtoken.verify and potentially keep backwards compatibility?
 tokenVerificationFunction?: (token:string, key:string) => Promise<JwtPayload>;
}

I ignored the jwk part as its a separate function createRemoteJwksSigningKeyProvider under utils that is user replaceable. But we need to still somehow make the imports conditional so that for edge user have an option not to import it.

@aarne
Copy link
Author

aarne commented Oct 31, 2024

... looking at the code only and ignoring the migration effort, it might be better to break the compatibility :(

type JwtPluginOptions = {
 tokenLookupLocations: AtleastOneItem<ExtractTokenFunction>;
 tokenVerificationFunction: (token:string) => Promise<JwtPayload>;
}

then we can have 2 implementations for tokenVerificationFunction that encapsulate all verification logic and key lookup. Also users can swap out the implementation if they need it. Key lookup functions and signatures will also differ between validation implementations.

And while it would be possible to just ignore this part and implement the lookup logic directly in tokenVerificationFunction it would create more confusion for the users in the long run.

@aarne
Copy link
Author

aarne commented Oct 31, 2024

Example implementation https://github.com/aarne/graphql-yoga/tree/feat_jwt_external/packages/plugins/jwt/src ... keeps plugin clean and moves all jsonwebtoken related functions to separate file.

Im not sure what is the best distribution model ... separate package or separate export somehow?

Usage examples for both jsonwebtoken and jose

import { createJwtValidator } from '@graphql-yoga/plugin-jwt/jsonwebtoken'
import { jwtVerify } from "jose";

const pluginWithJsonwebtoken = useJwt({
      tokenVerificationFunction: createJwtValidator({
            singingKeyProviders: [createInlineSigningKeyProvider('topsecret')]
      })
})

const pluginWithJose = useJwt({
      tokenVerificationFunction: async (token) => {
            const secret = new TextEncoder().encode("topsecret");
            const payload = await jwtVerify(token, secret);
            return payload.payload;
      },
})

@EmrysMyrddin
Copy link
Collaborator

While I understand the logic behind this new API (composition is pretty good way to architecture things), It also complexifies the usage of the plugin :-/

Most of users will just want a JWT plugin for which you either give it a key, or a key store url, plus some validation against static values, like the audience. So the plugin API should offer to easily do this, without requiring the user to implement it. The main use case should be straight forward. Then we can add anything that allows for custom behavior (that why we just forward all options to any jsonwebtoken function, this way the user can fully customize the call to the lib).

Those uses case have to remain simple:

// Simple H256 use case
const yoga = createYoga({
  plugins: [
    useJwt({
      signingKeyProviders: [createInlineSigningKeyProvider(process.env.JWT_KEY)],
    })
  ]
})

// More complex case with a JWS
const yoga = createYoga({
  plugins: [
    useJwt({
      signingKeyProviders: [createRemoteJwksSigningKeyProvider({
        jwksUri: process.env.JWS_URL,
      })],
      tokenVerification: {
        audience: process.env.JWS_CLIENT_ID,
      },
    })
  ]
})

A way to allow what you want, could be to allow passing a function in tokenVerification option and add a decodeToken option.

Then, the plugin can use those provided functions instead of the default implementation relying on jsonwebtoken. We can even lazily import the jsonwebtoken library, so that it's not imported if both tokenVerification and decodeToken functions are provided. Which means we jsonwebtoken could become an optional dependency (if I well remember, option deps are installed by default, which should not break installation for people that don't want to swap the JWT implementation).

What do yo think ?

@aarne
Copy link
Author

aarne commented Nov 4, 2024

A way to allow what you want, could be to allow passing a function in tokenVerification option and add a decodeToken option.

This was the point where i started from :) Unfortunately this leaves some smells on the codebase, but the main use-case keeps working as is ... reworked code https://github.com/aarne/graphql-yoga/tree/feat_jwt_external2/packages/plugins/jwt/src

Things i'm not very happy about:

  • you still need to provide at least one singingKeyProviders entry
  • singingKeyProviders return signature is string making them less useful
  • decodeTokenHeader is mandatory for implementing tokenVerification (assuming you run on the edge)

so the jose implementation would look like this

const test = createTestServer({
      singingKeyProviders: [() => "ignore"],
      decodeTokenHeader: token => {
        return {
          kid: 'ignore',
        };
      },
      async tokenVerification(token, _ignore) {
        const rsaPublicKey = await jose.importJWK({....},'PS256')
        const payload = await jose.jwtVerify(token, rsaPublicKey);
        return payload.payload;
      },
});

We could make the singingKeyProviders and decodeTokenHeader optional, but then the code gets much more messy and harder to reason about. Also one can also just implement the decodeTokenHeader and singingKeyProviders but then you need to serialise the key to string and back which also sounds ugly.

@aarne
Copy link
Author

aarne commented Nov 15, 2024

@EmrysMyrddin Should I start a PR for the second implementation?

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

No branches or pull requests

2 participants