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(hono/jwk): JWK Auth Middleware #3826

Merged
merged 34 commits into from
Feb 6, 2025
Merged

feat(hono/jwk): JWK Auth Middleware #3826

merged 34 commits into from
Feb 6, 2025

Conversation

Beyondo
Copy link
Contributor

@Beyondo Beyondo commented Jan 13, 2025

Hello, I recently needed a JWK middleware for my projects, but I figured contributing it to hono has the potential to save me and others a lot of time.

Middleware Features:

  • Set options.keys to a static array of public keys HonoJsonWebKey[] in code.
  • Set options.keys to an async function that returns a Promise<HonoJsonWebKey[]> for flexibility
  • Set options.jwks_uri to fetch JWKs from a URI, after which it appends those fetched keys to provided keys if any
  • Set an optional init parameter (only used for jwks_uri)—useful if your host supports caching through custom init options.

Added extra:

  • Added JwtHeaderRequiresKid exception. Since the middleware requires presence of a kid field in the header in order to select the correct key.
  • Added Jwt.verifyFromJwks util function (batteries included).

Addressed issues:

Other code changes:

  • Typescript-extended JsonWebKey to have kid?: string (This is a standard: https://datatracker.ietf.org/doc/html/rfc7515#section-4.1.4)
  • Added kid?: string to TokenHeader
  • Fixed a bug in Jwt.sign(payload, privateKey, alg) where privateKey.alg was always ignored and only alg parameter was considered.

Suggestion: Removal of the default 'HS256' value from Jwt.sign and make alg optional, so that if neither privateKey.alg nor alg is defined, throw an error. My 'philosophy' for this is that it would be misleading for hono to display = 'HS256' when the privateKey can internally have a different algorithm, such as when it is of type JsonWebKey or CryptoKey (Though we only care about JsonWebKey here). Making alg a "fallback" explicit parameter could be better.

Example Usage:

Using jwks_uri

import { jwk } from "hono/jwk";
app.use("/backend-api/*", jwk({ jwks_uri: "https://example-backend.hono.dev/.well-known/jwks.json" }));

Using jwks_uri and an optional init

import { jwk } from "hono/jwk";
app.use("/backend-api/*", jwk({
  jwks_uri: "https://example.hono.dev/.well-known/jwks.json"
}, {
  headers: {
    "My-Custom-Header": "Hello Hono!"
  },
  cf: { cacheTtl: 3600, cacheEverything: true } // Only for Cloudflare Workers
}));

Using keys from an array

[
  {
    "kid": "VpNx_Ar8pXsh50UjqBD_XtYLj4k9Z72iakeOQFJYIpg",
    "kty": "RSA",
    "use": "sig",
    "alg": "RS256",
    "e": "AQAB",
    "n": "4anIpNGZ-C5rJfRty8qQt8wTheFDj9wchPA..."
  },
  {
    "kid": "Rw6zp1oTEbQ9qUIFnhHGadtstXhKYrCmkqgkNEG8uUc",
    "kty": "RSA",
    "use": "sig",
    "alg": "RS256",
    "e": "AQAB",
    "n": "vKetN8i4_ORAWAUVR1pCjVMFkNzwC1Qjy..."
  }
]
import keys from './keys.json';
import { jwk } from "hono/jwk";
app.use("/backend-api/*", jwk({ keys: keys }));

Using keys as an async function (custom logic / caching)

$router.use("/auth/*", jwk({
  keys: async () => {
    /* ... Custom caching logic ... */
    return cached_response.keys;
  }
}));

Tests:

I adapted a lot from JWT's own units tests to make sure some basic JWT validation exists, then added the actual validation for the JWK middleware's own functionality which is mostly: (1) getting keys through various methods, (2) selecting the appropriate JWK based on kid (without fully decoding the JWT—only the header), and then (3) verifying using the utility Jwt.verify. Step 2 and 3 are done by Jwt.verifyFromJwks.

JWK Middleware Test

hono> npx src/middleware/jwk/index.test.ts

 DEV  v2.1.1 V:/Repos/hono
      Coverage enabled with v8

 ✓ src/middleware/jwk/index.test.ts (16)
   ✓ JWK (16)
     ✓ Credentials in header (10)
       ✓ Should not authorize requests with missing access token
       ✓ Should authorize from a static array passed to options.keys (key 1)
       ✓ Should authorize from a static array passed to options.keys (key 2)
       ✓ Should authorize with Unicode payload from a static array passed to options.keys
       ✓ Should authorize from a function passed to options.keys
       ✓ Should authorize from a URI remotely fetched from options.jwks_uri
       ✓ Should not authorize requests with invalid Unicode payload in header
       ✓ Should not authorize requests with malformed token structure in header
       ✓ Should not authorize requests without authorization in nested JWK middleware
       ✓ Should authorize requests with authorization in nested JWK middleware
     ✓ Credentials in cookie (5)
       ✓ Should not authorize requests with missing access token
       ✓ Should authorize from a static array passed to options.keys
       ✓ Should authorize with Unicode payload from a static array passed to options.keys
       ✓ Should not authorize requests with invalid Unicode payload in cookie
       ✓ Should not authorize requests with malformed token structure in cookie
     ✓ Error handling with `cause` (1)
       ✓ Should not authorize

 Test Files  1 passed (1)
      Tests  16 passed (16)

Full Test

hono> npm test
...
...
...
 Test Files  9 passed (9)
      Tests  415 passed | 3 skipped (418)

  • [✅] Add tests (src/middleware/jwk/index.test.ts)
  • [✅] Run tests (Above)
  • [✅] bun run format:fix && bun run lint:fix to format the code (Done)
  • [✅] Add TSDoc/JSDoc to document the code (src/middleware/jwk/jwk.ts)

Feel free to contribute more unit tests, extend and/or modify for the better.

…yAlg fallback, add decodeHeaders utility

- Added "kid" (Key ID) for TokenHeader
- Fixed Jwt.sign() ignoring privateKey.alg
- Renamed `alg` parameter to `KEYAlg` to differentiate between privateKey.alg
- Added utility function `decodeHeaders` to decode only JWT headers
Hono JWK Middleware main features:
- Ability to provide a list of public JWKs to the keys parameter as a simple javascript array []
- Ability to provide a URL in the jwks_uri parameter to fetch keys from + an optional RequestInit (useful for caching if your cloud provider has a modified fetch that supports it, or if you simply want to modify the request)
- Ability to provide an async function that returns an array to the keys parameter instead of a direct array, so that it is possible to implement own custom caching layer without a separate middleware
- Allows setting a keys directory for multi-key auth systems
- Allows auth endpoints to be always updated with the Auth provider's public JWKs directory (often `.well-known/jwks.json`) which makes key rotations without disruptions possible

Todo:
- More tests.
Added /auth-keys-fn/* & /.well-known/jwks.json testing endpoints to the router.
Copy link

codecov bot commented Jan 13, 2025

Codecov Report

Attention: Patch coverage is 91.37931% with 15 lines in your changes missing coverage. Please review.

Project coverage is 90.79%. Comparing base (2ead4d8) to head (c9919a0).
Report is 29 commits behind head on next.

Files with missing lines Patch % Lines
src/utils/jwt/jwt.ts 87.30% 8 Missing ⚠️
src/middleware/jwk/jwk.ts 96.07% 4 Missing ⚠️
src/utils/jwt/types.ts 50.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             next    #3826      +/-   ##
==========================================
- Coverage   91.71%   90.79%   -0.92%     
==========================================
  Files         160      164       +4     
  Lines       10195    10434     +239     
  Branches     2885     3061     +176     
==========================================
+ Hits         9350     9474     +124     
- Misses        844      959     +115     
  Partials        1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Beyondo
Copy link
Contributor Author

Beyondo commented Jan 14, 2025

Note that the test code jwk/index.test.ts was 519 lines so it inflated the actual aggregated changes of this PR, + jsdoc comments, and + the keys.test.json file and so on. The core changes are probably < 200 lines and thus much more mangeable to review.

I removed some commented out tests to give a clearer picture (Now at 307 lines of test code). But they did remind me—we need some signed cookie unit tests if we're gonna make sure to support everything in jwt/index.test.ts too. Though I don't think it's exactly a priority and could be implemented later.

@yusukebe
Copy link
Member

Hi @Beyondo

Thanks! I'll check this later.

@Beyondo
Copy link
Contributor Author

Beyondo commented Jan 14, 2025

@yusukebe No problem! I originally named the new interface ExtendedJsonWebKey, though I do think HonoJsonWebKey might also work better since the only addition is the optional kid?, making it function just like the normal JsonWebKey if needed, so we might need some opinion on this.

Take your time! I'm using the fork temporarily, so no rush at all.

@yusukebe
Copy link
Member

Hey @Code-Hex ! I'll review this later, but can you take a look at it too?

@Code-Hex
Copy link
Contributor

@yusukebe I'm sorry I missed this PR.
I will check tomorrow 🙏

Copy link
Contributor

@Code-Hex Code-Hex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was mostly LGTM!

Also removed redundant "import type {} from '../..'" from 3 different files:
- jwk/index.ts
- jwt/index.ts
- request-id/index.ts
Copy link
Contributor

@Code-Hex Code-Hex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yusukebe

@yusukebe
Copy link
Member

@Code-Hex Thank you for the review!

Hi, @Beyondo, Almost all are good, but the test coverage is decreased. Of course, it should not be 100%, but is there any chance to add more tests?

@Beyondo
Copy link
Contributor Author

Beyondo commented Jan 31, 2025

Hey, @yusukebe ! I improved test coverage by around +15% focusing on testing the important parts, such as signed cookies and missing kid exception and a few others, while the rest of unconvered lines are fairly trivial just by looking at them but also a bit tedious to write tests for; I will cover more of them as soon as I got time

@yusukebe
Copy link
Member

yusukebe commented Feb 2, 2025

@Beyondo

Thank you so much! If it's ready, please ping me. I'll review it.

Note: Moved more code from `hono/jwk` to the `verifyFromJwks` function for backends that require JWK verification logic beyond just a middleware
@Beyondo
Copy link
Contributor Author

Beyondo commented Feb 2, 2025

@yusukebe Yo, everything, at least JWK-related, is covered & all hono tests pass.

I think it should be decent enough for review now

@yusukebe
Copy link
Member

yusukebe commented Feb 2, 2025

@Beyondo

Thank you for your hard work! Almost done. I've left comments.

Copy link
Member

@yusukebe yusukebe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@yusukebe
Copy link
Member

yusukebe commented Feb 4, 2025

Hi @Beyondo

Looks good. Thank you for your hard work! I'll merge it into the next branch for the next minor version and release it. Maybe soon!

@yusukebe yusukebe changed the base branch from main to next February 6, 2025 13:14
@yusukebe yusukebe merged commit 6837649 into honojs:next Feb 6, 2025
16 checks passed
@nikeee
Copy link

nikeee commented Feb 13, 2025

I am using this middleware and it works without any issues, thanks!

My application has several endpoints that have optional authentication. However, this middleware throws an unauthenticated exception if there is no authentication available. Would it be possible to support this use-case somehow? Or am I doing something wrong?

Also I don't know if I'm holding it wrong, but

const payload = c.get("jwtPayload");

is reported as an error by the type checker ("jwtPayload" not matching any overload). This does not happen with @hono/sentry: c.get("sentry").

Adding this to the d.ts of jwk fixes that:

declare module 'hono' {
	interface ContextVariableMap {
		jwtPayload: unknown; // Maybe we can even be more precise with an object structure
	}
}

However, that fix won't work if there would be an optional jwtPayload.

Edit:
Avoiding the use of ContextVariableMap, one seems to be able to use createMiddleware:

	return createMiddleware<{
		Variables: {
			jwtPayload: JWTPayload | undefined;
		};
	}>(async function jwk(ctx, next) {

@Beyondo
Copy link
Contributor Author

Beyondo commented Feb 13, 2025

@nikeee I would personally attempt to separate anonymous endpoints, but addressing the question—you could create a custom middleware, say jwkOptional, that wraps around jwk but resumes on failure to achieve the optional authentication.

const jwkOptional = (options, init) => {
  const jwkMiddleware = jwk(options, init);
  return async (c, next) => {
    try {
      await jwkMiddleware(c, next);
    } catch (err) {
      await next();
    }
  };
};

With types

const jwkOptional = (options: {
  keys?: HonoJsonWebKey[] | (() => Promise < HonoJsonWebKey[] > );
  jwks_uri?: string;
  cookie?: string | {
    key: string;
    secret?: string | BufferSource;
    prefixOptions?: CookiePrefixOptions;
  };
}, init?: RequestInit) => {
  const jwkMiddleware = jwk(options, init);
  return async (c: Context, next: Next) => {
    try {
      await jwkMiddleware(c, next);
    } catch (err) {
      await next();
    }
  };
};

You could also modify it to set a boolean such as is_authenticated in the request's Context, but I suppose checking if jwtPayload is defined is enough too.

@nikeee
Copy link

nikeee commented Feb 13, 2025

you could create a custom middleware, say jwkOptional, that wraps around jwk but resumes on failure to achieve the optional authentication.

I thought about that approach, but I ended up doing it in reverse: rewriting JWK to make it JWTPayload | undefined and creating a seperate middleware that checks for undefined and throws. I noticed it becomes cumbersome if I still want to throw an unauthorized if there is a token - but an invalid one. Using try-catch, I have to dissect the exception.

This works for the thing I'm currently building, but recommending this approach to hono's library itself would be inconsistent with the JWT middleware.

Regarding the context: Do you think it is useful to change the return of jwk to something like this?

return createMiddleware<{
	Variables: {
		jwtPayload: JWTPayload;
	};
}>(async function jwk(ctx, next) {

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.

4 participants