Open aarne opened 1 month ago
We can also think of bring your own jwt library
type of integrations mby?
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.
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:
Allow users to pass in the verify function and use decode from jose
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.
... 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.
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;
},
})
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 ?
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:
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.
@EmrysMyrddin Should I start a PR for the second implementation?
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 onjsonwebtoken
library.Describe the solution you'd like
Looking at the code there are multiple possible options to replace the
jsonwebtoken
with for examplejose
, 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.