Skip to content

feat(server): support EC and EdDSA keys in OIDC JWKS validation - #2593

Open
lunarwhite wants to merge 1 commit into
NVIDIA:mainfrom
lunarwhite:oidc-jwks
Open

feat(server): support EC and EdDSA keys in OIDC JWKS validation#2593
lunarwhite wants to merge 1 commit into
NVIDIA:mainfrom
lunarwhite:oidc-jwks

Conversation

@lunarwhite

@lunarwhite lunarwhite commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

OIDC JWKS validation was RSA-only, so issuers that publish ES256/ES384 or EdDSA (Ed25519) signing keys (notably Okta ES256) failed with unknown signing key. This PR derives the algorithm for EC/OKP keys from each JWK's kty/crv, selects the RSA algorithm from the JWK's declared alg (defaulting to RS256), pins JWT header alg against the cached algorithm, requires standard claims, and documents the supported algorithms.

Related Issue

Closes #2196

Changes

  • Extend JWKS ingestion beyond RSA to EC (P-256 → ES256, P-384 → ES384) and OKP (Ed25519 → EdDSA)
  • Select the RSA algorithm from the JWK's declared alg (RS256/RS384/RS512/PS256/PS384/PS512), defaulting to RS256 when absent; reject genuinely contradictory declarations
  • Pin validation algorithm from the cached JWK; reject JWT header alg mismatches
  • Require iss, aud, exp, and sub on OIDC access tokens (aligns with workspace membership keyed by sub)
  • Skip unusable JWKs (use: enc, alg mismatch, key_ops without verify) and poison duplicate kids with conflicting algorithms
  • Preserve the existing key cache when a refresh yields zero usable keys, instead of wiping a working cache and failing all auth
  • Add wiremock unit coverage for RSA (incl. RS512 selection)/ES256/ES384/EdDSA and related rejection/resilience paths
  • Document supported algorithms in docs/reference/gateway-auth.mdx, docs/kubernetes/access-control.mdx, and architecture/gateway.md

Testing

  • mise run pre-commit passes
  • Unit tests added/updated
  • E2E tests added/updated (if applicable)

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable)

@copy-pr-bot

copy-pr-bot Bot commented Aug 3, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@mrunalp

mrunalp commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Should fix before merge

1. A fully-unusable JWKS wipes the working key cache and hard-fails all auth

refresh_keys commits the new map unconditionally:

if total > 0 && new_keys.is_empty() {
    warn!(total, loaded = 0, "JWKS refresh loaded zero usable signing keys");
} else {
    info!(count = new_keys.len(), "JWKS keys loaded");
}
*self.keys.write().await = new_keys;   // <-- committed even when empty

The PR detects the condition and then commits it anyway. This matters more now than before: the PR adds five new ways for a key to be dropped (use: enc, non-sig use, key_ops without sign/verify, alg mismatch, poison-pill). An issuer that adds one bad piece of metadata to all its keys — see #2 below — takes the gateway from "working" to "every OIDC request rejected", with only a warn! to explain it.

Suggested behavior: when total > 0 && new_keys.is_empty() and the existing cache is non-empty, keep the existing keys and log at error!. Still stamp last_refresh so the normal TTL cadence retries rather than fetching on every request.

Note that returning Err instead is worserefresh_if_stale propagates it and validate_token returns Status::internal, so all auth fails either way, just with a different error code.

2. alg is used as a veto but never as a selector, and RSA is hardcoded to RS256

parse_jwk derives RSA → RS256 unconditionally, then drops the key if the issuer declares anything else:

if let Some(ref declared) = key.alg
    && declared != jwk_alg_name(algorithm)
{
    return Err(SkipReason::AlgMismatch { .. });
}

I confirmed the consequence with two throwaway tests on the branch:

  • An RSA JWK carrying "alg": "RS512" is dropped entirely — even a token that key signed with RS256 now fails with unknown signing key. Before this PR that key loaded and that token validated.
  • RS384/RS512/PS256/PS384/PS512-signed tokens remain unsupported.

#2196 defers RS384/RS512 ("can follow via JWK alg later"), which is fine as scope — but the veto turns that deferral into an active regression for any issuer that publishes non-RS256 alg metadata on RSA keys. Combined with #1, a single such issuer-side config change empties the cache.

jsonwebtoken 9.3.1 supports all six RSA algorithms (Algorithm::{RS256,RS384,RS512,PS256,PS384,PS512}), so closing this is small — for kty: RSA, select from the declared alg when present and supported, default to RS256 when absent. The veto then only fires for genuinely contradictory combinations (an RSA key declaring ES256), which is what you actually want it to catch.

The "absent" branch is the mainstream path, not an edge case: Microsoft Entra ID publishes no alg field at all on any key in its JWKS (checked against login.microsoftonline.com/common/discovery/v2.0/keys — every key carries only kty, use, kid, x5t, n, e, x5c). Google publishes alg on all keys. So both shapes occur in production and both need to work.

3. jwk_alg_name's "unsupported" sentinel is a footgun feeding that comparison

fn jwk_alg_name(algorithm: Algorithm) -> &'static str {
    match algorithm { /* ... */ _ => "unsupported" }
}

The comparison in #2 is declared != jwk_alg_name(derived). If someone later adds an algorithm to parse_jwk but forgets the arm here, every key declaring that alg is silently dropped — the failure is total and looks like a JWKS problem.

Algorithm implements FromStr, so compare in the parsed domain instead and let unknown strings be an explicit skip reason:

match Algorithm::from_str(declared) {
    Ok(a) if a == algorithm => {}
    _ => return Err(SkipReason::AlgMismatch { .. }),
}

jwk_alg_name can then be reduced to log formatting (or replaced with {:?}).


Worth addressing

4. expect() in the gateway auth path

let kid = key.kid.as_ref().expect("parse_jwk ensures kid");

The invariant is real but lives in a different function, so a future edit to parse_jwk's ordering turns this into a gateway panic on JWKS refresh. Have parse_jwk return the kid it validated: Result<(String, DecodingKey, Algorithm), SkipReason>. Removes the invariant entirely and shortens the call site.

5. Docs: the RSA row overstates what is supported

docs/reference/gateway-auth.mdx presents the mapping as if kty/crv determines the algorithm:

JWK kty crv Algorithm
RSA RS256

For EC and OKP that is accurate. For RSA it is a limitation, not a derivation. Either fix per #2 or state plainly that RSA keys are validated as RS256 only and that RS384/RS512/PS* are not supported.

6. use and key_ops are given opposite strictness

if let Some(use_) = key.use_.as_deref().filter(|u| !u.is_empty())
    && use_ != "sig"
{
    return Err(SkipReason::UseNotSig);          // strict: must be exactly "sig"
}

if !key.key_ops.is_empty() && !key.key_ops.iter().any(|op| op == "sign" || op == "verify") {
    return Err(SkipReason::KeyOpsNotForVerify); // lenient: sign OR verify
}

These are RFC 7517's two mechanisms for expressing the same constraint, given opposite postures in adjacent lines. The gateway only ever verifies, so key_ops should require verify when present — ["sign"] on a public key is semantically incoherent, and sign is a distinct operation from verify per §4.3.

To be explicit about severity: this is a correctness and consistency fix, not a security fix. §4.3 lists sign with verify as an expressly permitted pairing, so accepting ["sign"] does not create the key-reuse exposure the RFC warns about. The combination that does — an encryption key used for verification — is already blocked, and key_ops_encrypt_only_skipped covers it.

The stronger case is one the current tests actively lock in the wrong way. key_ops_sign_and_encrypt_accepted asserts that ["sign", "encrypt"] is accepted, and that is precisely the pairing §4.3 rules out:

Multiple unrelated key operations SHOULD NOT be specified for a key because of the potential vulnerabilities associated with using the same key with multiple algorithms. Thus, the combinations "sign" with "verify", "encrypt" with "decrypt", and "wrapKey" with "unwrapKey" are permitted, but other combinations SHOULD NOT be used.

Requiring verify fixes both at once. Three test outcomes change, not just the sign-only one:

Test key_ops Now After
key_ops_verify_and_sign_accepted ["verify"] accept accept
key_ops_verify_and_sign_accepted ["sign"] accept reject
key_ops_sign_and_encrypt_accepted ["sign", "encrypt"] accept reject
key_ops_encrypt_only_skipped ["encrypt"] reject reject

Interop risk is low. Neither Google nor Microsoft Entra publishes a key_ops field at all — both rely on use: "sig" — so absent key_ops stays accepted and the mainstream providers are unaffected.


Nits

  • validation.algorithms = vec![cached_algorithm]; is redundant — Validation::new(alg) already sets algorithms: vec![alg].
  • rsa_test_key generates a fresh 2048-bit key on each of its ~12 call sites; the oidc tests take ~7s almost entirely on keygen. A static PEM constant would be faster and deterministic.
  • ec_coords_from_spki locates the public point by scanning backwards for a 0x04 byte. It works for the fixtures but will fail confusingly if a coordinate happens to contain 0x04 at the wrong offset. Since rcgen's KeyPair is already in dev-deps, consider slicing the point at its known SPKI offset instead of searching.
  • parse_jwk's first check (use == "enc") is subsumed by the second (use != "sig"); it exists only to produce a distinct log line. Fine, but a comment saying so would save the next reader a double-take. If Security feedback: SSH hardening and policy validation #7 lands, SkipReason::KeyOpsNotForVerify gets the same treatment for free.

Signed-off-by: Yuedong Wu <dwcn22@outlook.com>
@lunarwhite

Copy link
Copy Markdown
Contributor Author

@mrunalp Thanks for your review. I've incorporated all of them into the latest commit. I hardcoded RSA algo to RS256 only because I thought modifying it would expand the original request's scope (RSA vs EC, EdDSA). Now the full support has been added via jsonwebtoken as you suggested. PTAL once you get the chance.

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

Successfully merging this pull request may close these issues.

feat(auth): OIDC JWKS support for EC and EdDSA signing keys

2 participants