feat(filter): add jwt_auth filter with JWKS validation - #769
feat(filter): add jwt_auth filter with JWKS validation#769yossiovadia wants to merge 615 commits into
Conversation
Signed-off-by: dependabot[bot] <support@github.com>
…s-proxy#223) Signed-off-by: dependabot[bot] <support@github.com>
…axis-proxy#225) Signed-off-by: Dimitri Saridakis <dimitri.saridakis@gmail.com> Signed-off-by: dimakis <dimitri.saridakis@gmail.com>
Signed-off-by: Sébastien Han <seb@redhat.com>
Signed-off-by: Sébastien Han <seb@redhat.com>
Signed-off-by: Alex Snaps <alex@wcgw.dev>
Signed-off-by: Sébastien Han <seb@redhat.com>
…axis-proxy#237) Signed-off-by: Sébastien Han <seb@redhat.com>
…xy#236) Signed-off-by: Sébastien Han <seb@redhat.com>
…praxis-proxy#241) Signed-off-by: Sébastien Han <seb@redhat.com>
…xis-proxy#253) Signed-off-by: Alex Snaps <alex@wcgw.dev>
…praxis-proxy#244) Signed-off-by: Sébastien Han <seb@redhat.com>
…raxis-proxy#235) Signed-off-by: Sébastien Han <seb@redhat.com>
…proxy#234) Signed-off-by: Sébastien Han <seb@redhat.com>
Signed-off-by: Sébastien Han <seb@redhat.com>
Signed-off-by: Sébastien Han <seb@redhat.com>
…xis-proxy#254) Signed-off-by: Sébastien Han <seb@redhat.com>
…axis-proxy#242) Signed-off-by: Sébastien Han <seb@redhat.com>
…y#247) Signed-off-by: Brent Salisbury <bsalisbu@redhat.com>
Signed-off-by: Brent Salisbury <bsalisbu@redhat.com>
Signed-off-by: Brent Salisbury <bsalisbu@redhat.com>
Signed-off-by: Aslak Knutsen <aslak@4fs.no>
Signed-off-by: Sébastien Han <seb@redhat.com>
Signed-off-by: Brent Salisbury <bsalisbu@redhat.com>
…nse-store (praxis-proxy#271) Signed-off-by: Sébastien Han <seb@redhat.com>
Signed-off-by: Sébastien Han <seb@redhat.com>
…roxy#272) Signed-off-by: Sébastien Han <seb@redhat.com>
Signed-off-by: Sébastien Han <seb@redhat.com>
Signed-off-by: Brent Salisbury <bsalisbu@redhat.com> Co-authored-by: Francisco Javier Arceo <farceo@redhat.com> Co-authored-by: Sébastien Han <seb@redhat.com>
…raxis-proxy#274) Signed-off-by: Sébastien Han <seb@redhat.com>
Signed-off-by: Shane Utt <shaneutt@linux.com>
Signed-off-by: Shane Utt <shaneutt@linux.com>
Signed-off-by: Shane Utt <shaneutt@linux.com>
…s-proxy#787) * fix(compat): align routing branches with terminal semantics Signed-off-by: Brent Salisbury <bsalisbu@redhat.com> * fix(compat): support Praxis main retry context fields Signed-off-by: Brent Salisbury <bsalisbu@redhat.com> * fix(compat): retain tracing guard Signed-off-by: Brent Salisbury <bsalisbu@redhat.com> --------- Signed-off-by: Brent Salisbury <bsalisbu@redhat.com>
…xy#775) Bumps [astral-sh/setup-uv](https://github.com/astral-sh/setup-uv) from 9.0.0 to 10.0.1. - [Release notes](https://github.com/astral-sh/setup-uv/releases) - [Commits](astral-sh/setup-uv@c771a70...20cfd1b) --- updated-dependencies: - dependency-name: astral-sh/setup-uv dependency-version: 10.0.1 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…axis-proxy#777) Bumps [github/codeql-action/init](https://github.com/github/codeql-action) from 4.37.6 to 4.37.7. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@5595cca...ff2f1c6) --- updated-dependencies: - dependency-name: github/codeql-action/init dependency-version: 4.37.7 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…raxis-proxy#776) Bumps [github/codeql-action/analyze](https://github.com/github/codeql-action) from 4.37.6 to 4.37.7. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@5595cca...ff2f1c6) --- updated-dependencies: - dependency-name: github/codeql-action/analyze dependency-version: 4.37.7 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Aslak Knutsen <aslak@4fs.no>
| - name: authenticated | ||
| filters: | ||
| - filter: jwt_auth | ||
| jwks_url: "http://127.0.0.1:8280/realms/ai-gateway/protocol/openid-connect/certs" |
There was a problem hiding this comment.
I would probably accept this configuration just if a flag is also set to allow http instead of https. Overall, IMO the intention of the user of not using https for something like a jwks endpoint should be a very conscious decision
| | `jwks_url` | string | yes | URL of the JWKS endpoint (JSON Web Key Set). The filter fetches public keys from here to verify JWT signatures. This channel is the filter's root of trust — it should be `https://` in any deployment where the proxy-to-`IdP` path is not already authenticated. A plain `http://` URL (other than loopback) is accepted but logs a warning at startup. | | ||
| | `issuer` | string | no | Expected `iss` (issuer) claim. If set, tokens from other issuers are rejected. | | ||
| | `audience` | string | no | Expected `aud` (audience) claim. If set, tokens not intended for this audience are rejected. | | ||
| | `claim_metadata` | object<string, string> | no | Maps JWT claim names to `filter_metadata` keys. The filter extracts these claims from verified tokens and writes them to `filter_metadata` for downstream filters (e.g. `external_metering`). Claims are intentionally NOT injected as upstream request headers — see the filter docs for the trusted-channel rationale. | |
There was a problem hiding this comment.
maybe not for now, but the metadata can be an arbitrary set of object<string, object>
As an example, Kubernetes JWT token for a SA can be decoded into:
{
"aud": [
"https://kubernetes.default.svc.cluster.local"
],
"exp": 1787326909,
"iat": 1787323309,
"iss": "https://kubernetes.default.svc.cluster.local",
"jti": "170964fe-f262-45f7-be60-594c62e70c7a",
"kubernetes.io": {
"namespace": "default",
"serviceaccount": {
"name": "mygateway",
"uid": "48c32fc6-a3c8-4e14-ad09-6219c381d4b4"
}
},
"nbf": 1787323309,
"sub": "system:serviceaccount:default:mygateway"
}I am not sure if it would make sense to support some more arbitrary structure, or something like a "jsonpath'ish" for these metadata
|
|
||
| | Field | Type | Required | Description | | ||
| |-------|------|---------|-------------| | ||
| | `jwks_url` | string | yes | URL of the JWKS endpoint (JSON Web Key Set). The filter fetches public keys from here to verify JWT signatures. This channel is the filter's root of trust — it should be `https://` in any deployment where the proxy-to-`IdP` path is not already authenticated. A plain `http://` URL (other than loopback) is accepted but logs a warning at startup. | |
There was a problem hiding this comment.
we will not now (or never) support symmetric JWT tokens, right? like a shared signing key. I will try to take a look if there is some existing architectural proposal for it, but though would be good to check
| @@ -0,0 +1,28 @@ | |||
| -----BEGIN PRIVATE KEY----- | |||
There was a problem hiding this comment.
this is very nit, but a fixture private key may cause lots of alarms on code scanners. Just consider maybe if it is possible to generate this during the test and cleanup after the test
| if config.jwks_url.is_empty() { | ||
| return Err("jwt_auth: jwks_url must not be empty".into()); | ||
| } | ||
| if config.claim_metadata.is_empty() { |
There was a problem hiding this comment.
is this really required? I mean, a inner filter could use just the subject for authorization
| } | ||
|
|
||
| let client = reqwest::Client::builder() | ||
| .timeout(Duration::from_secs(10)) |
There was a problem hiding this comment.
maybe make this configurable on the filter?
| // Fast path: key is cached and TTL hasn't expired. | ||
| { | ||
| let keys = self.keys.read().await; | ||
| let ttl_ok = keys.last_refresh.is_some_and(|t| t.elapsed() < DEFAULT_TTL); |
There was a problem hiding this comment.
nit (possible followup)
I think it makes sense to have the TTL also configurable. Overall:
- An insecure environment, public you may want to have the TTL lowered, in case you need to revoke the keypair signing the JWT tokens you can react fast
- A safe airgapped cluster could probably use of some bigger refresh time for the JWKS
| } | ||
| } | ||
|
|
||
| // Cooldown gate (lock-free): if we attempted a refresh very |
There was a problem hiding this comment.
I was wondering if the cooldown for failures should be somehow an exponential backoff until you hit the max REFRESH_COOLDOWN.
my concern here is that some unavailability on the jwks server will block requests for 30s, while those can be transitive. This could cause an overall unavailability of your authentication workflow.
There was a problem hiding this comment.
Good call — implemented in f2c78d8. A successful refresh keeps the 30s anti-stampede cooldown, but a failed one now backs off starting at 1s and doubling per consecutive failure, capped at 30s. So a transient JWKS blip recovers in ~1s instead of blocking auth for a full 30s, while a sustained outage still isn't hammered. Added a test that proves recovery after the short backoff (would fail under the old flat cooldown).
Adds a jwt_auth filter that validates JWT bearer tokens against a JWKS endpoint and injects verified claims as request headers. - Fetches and caches JWKS from a configured URL, keyed by kid - Validates signature, expiry, and issuer - Maps configured claims to request headers and filter metadata - Strips the bearer token before forwarding upstream - Supports RS256/384/512 and ES256 via jsonwebtoken (rust_crypto) Includes config validation, 11 unit tests (valid/expired/wrong-issuer/ unknown-kid/garbage/missing tokens, bearer extraction, metadata and header handling), an example config, filter docs, and an integration test. This is the temporary bridge discussed in praxis-proxy#708: it is self-contained and unblocks JWT/OIDC-fronted deployments today, while the core CPEX engine remains the long-term home for JWT validation. No other filter should depend on it. Closes praxis-proxy#708 Signed-off-by: Yossi Ovadia <yovadia@redhat.com>
…ders Addresses adversarial review of praxis-proxy#769: - CRITICAL: verify TLS certs on the JWKS fetch by default. The client hardcoded danger_accept_invalid_certs(true), making the root-of-trust channel MITM-able. Now opt-in via insecure_skip_tls_verify (logs a warning), and non-https/non-loopback JWKS URLs warn at startup. - Rename claim_headers -> claim_metadata to match the implementation: claims are written to filter_metadata, never injected as upstream headers (which would bypass identity_header_guard). Docs and example corrected to stop claiming "request headers". - Single-flight JWKS refresh: concurrent cache misses now share one fetch instead of stampeding the IdP. - Bound the JWKS response by Content-Length and a running chunk cap before buffering, not after. - Allow the full RSA/EC family when a JWK omits alg (avoids false 401s when the IdP signs with e.g. RS512), same key so no confusion risk. - Honest docs: TTL is stale-if-error, not a hard revocation bound; no-kid tokens are rejected; from_config fetches lazily. - Tests: add forged-signature rejection (the core property, previously untested), audience mismatch/match. 14 unit tests total. - Mark the filter as an explicit temporary bridge superseded by CPEX. Relates to praxis-proxy#708 Signed-off-by: Yossi Ovadia <yovadia@redhat.com>
…loopback - examples/README.md: the jwt-auth row still said "injects verified claims as request headers"; corrected to "writes verified claims to filter_metadata" to match the filter's behavior and config comment. - is_authenticated_url: url::host_str() returns IPv6 hosts bracketed, so the "::1" arm was dead and http://[::1] got a spurious MITM warning. Match "[::1]". Relates to praxis-proxy#708 Signed-off-by: Yossi Ovadia <yovadia@redhat.com>
Addresses review feedback (praxis-proxy#769): a flat 30s cooldown meant a transient JWKS-server blip blocked the whole auth path for a full 30s. Now a successful refresh keeps the 30s anti-stampede cooldown, but a failed refresh backs off from 1s, doubling per consecutive failure and capped at 30s — so a transient failure recovers in ~1s while a sustained outage still isn't hammered. Adds a test proving recovery after the short backoff (impossible under the flat cooldown). Signed-off-by: Yossi Ovadia <yovadia@redhat.com>
5143f11 to
f2c78d8
Compare
| let mut keys = self.keys.write().await; | ||
| keys.consecutive_failures = keys.consecutive_failures.saturating_add(1); | ||
| let shift = (keys.consecutive_failures - 1).min(5); | ||
| keys.retry_after = FAILURE_BACKOFF_INITIAL.saturating_mul(1 << shift).min(REFRESH_COOLDOWN); |
There was a problem hiding this comment.
not sure if this was just added or if I missed it, but resolves my comment on the backoff, thanks!
|
|
||
| /// Extract the JWT from the configured header. | ||
| /// | ||
| /// Handles both `Authorization: Bearer <token>` and raw |
There was a problem hiding this comment.
more like a consistency concern: would it make sense to not make user assumptions, but instead explicit choices?
As an example, per RFC-6750 the authorization header is always of format "Bearer" 1*SP b64token so a header like "x-api-key: bearer xpto" doesn't really match this expectations.
I would put on a different way:
- If the token_header is named "Authorization" you expect its value to be "Bearer <1 space only> value"
- If the token is something else, you get its value directly
In case some other format is required I would consider it a followup.
|
|
||
| debug!( | ||
| username = claims | ||
| .get("preferred_username") |
There was a problem hiding this comment.
this is some info from previous debug, or left intentionally?
praxis-bot
left a comment
There was a problem hiding this comment.
Re-Review: Exponential Backoff Changes
Reviewed commit f2c78d8 (exponential backoff on JWKS refresh failure) and found 2 new issues.
| Severity | Count |
|---|---|
| Large | 1 |
| Medium | 1 |
All findings have inline placement.
| // ----------------------------------------------------------------------------- | ||
| // Constants | ||
| // ----------------------------------------------------------------------------- | ||
|
|
There was a problem hiding this comment.
[Large] The exponential backoff starts from when the refresh STARTS, not when it COMPLETES. In refresh(), last_refresh is set at line 209 before the fetch begins. On success, it's correctly updated again at line 266 after the fetch completes, so the 30s cooldown starts from completion time. But on failure, this branch (line 181-191) does NOT update last_refresh again, so the backoff is measured from the START of the failed fetch.
This means if a JWKS fetch takes T seconds to timeout/fail and the configured backoff is B seconds, the effective backoff is max(0, B - T). For example, with the default 10-second HTTP timeout (line 106) and 1-second initial backoff (line 33), a slow or unresponsive IdP will have failed fetches retried immediately after each 10-second timeout, creating a stampede of long-running timeout requests instead of spacing them 1 second apart.
The test at line 583-635 doesn't catch this because it uses a 503 response (which fails immediately) rather than a timeout.
Fix: Add keys.last_refresh = Some(Instant::now()); after line 188 so the failure backoff starts from when the fetch completed, matching the success case behavior at line 266.
| let mut header = Header::new(Algorithm::RS256); | ||
| header.kid = Some(kid.to_owned()); | ||
| encode(&header, claims, key).unwrap() | ||
| } |
There was a problem hiding this comment.
[Medium] The exponential backoff test only verifies single-failure recovery. It doesn't test:
- Multiple consecutive failures growing the backoff exponentially (1s → 2s → 4s → 8s → 16s → 30s cap)
- The backoff cap at
REFRESH_COOLDOWN(30s) - Success resetting
consecutive_failuresto 0 after multiple failures - Backoff timing when the fetch takes significant time to fail (e.g., 5-10 second timeout)
Add tests covering these scenarios to ensure the backoff behaves correctly and prevent regressions. The multi-failure growth test is particularly important since that logic (line 186-188 in jwks.rs) has non-trivial math that should be verified.
shaneutt
left a comment
There was a problem hiding this comment.
Temporary hold here for some discussion
What
Adds a
jwt_authfilter that validates JWT bearer tokens against a JWKS endpoint and injects verified claims as request headers.kidjsonwebtokenwithrust_crypto)Why / relationship to #708
This is the temporary bridge discussed in #708. As noted there, JWT validation is not the long-term target architecture — the core CPEX engine is the intended home. But this filter is fully self-contained and unblocks JWT/OIDC-fronted deployments today. Per the discussion, no other filter should take a dependency on it, so it can be cleanly retired once CPEX covers this path.
Closes #708
Tests
tests/integration/.../examples/jwt_auth.rs): config parses, valid token allowed + stripped, missing/garbage tokens rejectedexamples/configs/jwt-auth.yaml), filter docs (docs/filters/jwt_auth.md), reference table entrycargo test,cargo clippy, andcargo +nightly fmt --checkall pass locally.Notes for reviewers
filters/src/jwt_auth/test_fixtures/rsa_private.pemis a throwaway RSA keypair generated solely to sign tokens in unit tests (include_str!, clearly labeled test-only). It signs nothing real and protects nothing — happy to switch to in-memory keygen if you'd prefer no committed key.jsonwebtoken'srust_cryptotransitive crypto crates (rsa, ecdsa, ed25519, p256/p384, der, spki, etc.).jsonwebtoken 10.4.0was already in the lock via the CPEX subtree; the diff is add-only (no packages removed).