Repository navigation
Add OAuth 2.1 auth and HTTPS to cve-intel serve - #10
Conversation
With CVE_INTEL_OAUTH_ISSUER and CVE_INTEL_OAUTH_RESOURCE set, every
request to /mcp and /api/v1 needs a bearer token. Tokens are JWTs checked
against the issuer's JWKS (from OIDC discovery of the configured issuer,
cached, refetched on unknown kid at most every 30 s): asymmetric algs
only, exact iss, aud containing the resource (or CVE_INTEL_OAUTH_AUDIENCE),
exp/nbf/iat, non-empty sub, and optional required scopes (scope or scp).
Failures get 401 with WWW-Authenticate: Bearer resource_metadata=...,
403 insufficient_scope, or 503 if keys can't load. RFC 9728 metadata is
served at /.well-known/oauth-protected-resource/mcp. Every other path
needs a token, including unknown ones, so a route added later can't be
public by accident. /health stays open but only answers {"status":"ok"};
the full report (an uncached NVD call, which keys are set) moves to
/api/v1/health. JWKS refreshes are serialised: a burst of unknown kids
waits for one fetch, at most every 30 s, and keys already held survive
a failed refresh. The key-fetching client fails closed if it can't be
built, and "use": "enc" keys never verify. Startup fails if discovery or the JWKS can't be loaded, or the
discovered issuer differs.
CVE_INTEL_OAUTH_REQUIRED_ROLES adds authorization: a user needs one of
the listed roles or groups (Keycloak realm or client roles, groups,
Entra app roles; exact matches), else a plain 403 access_denied that
doesn't name the allowed roles. Scope and role lists take spaces or
commas.
Without OAuth, serve now refuses a non-loopback HOST (Docker's 0.0.0.0
included) unless CVE_INTEL_ALLOW_UNAUTHENTICATED=1. serve logs rejection
reasons at info level; `--debug serve` logs the whole exchange
(discovery, JWKS, metadata served, each token's header and standard
claims, every check and the challenge sent). Each accepted request logs
who made it and each MCP tool call its tool, as an access log. Tokens
appear only as a SHA-256 fingerprint; the token, signature and
Authorization header are never logged, even when malformed (a bare
token or a tab separator), and a test fails if they are. Claim values
are logged escaped, and scopes are checked against RFC 6749's charset
because they are quoted into WWW-Authenticate.
The .env loader took everything after `=` as the value, and .env.example puts a comment after nearly every key. Copying it to .env, as the README says, set NVD_API_KEY and GITHUB_TOKEN to their comment text, pointed CACHE_DB_PATH at a comment, and gave HOST a value serve refuses. On CVE_INTEL_OAUTH_REQUIRED_ROLES the comment's words became extra allowed roles, widening access. parse_dotenv_line now ends an unquoted value at a `#` that starts it or follows whitespace (`abc#def` keeps its `#`), ends a quoted value at its closing quote, and skips lines whose key is empty or would make set_var panic. docker run --env-file never strips comments, so .env.example now puts every comment on its own line and leaves HOST/PORT commented out at their defaults (an active HOST overrode the image's 0.0.0.0). A test fails if the example gains an inline comment or an active HOST line.
- Escape claim text in the --debug log with clean_text. It is read before the signature check, and resource_access keys went out raw, so any client could write terminal escapes (OSC 52, line erase) to it. - Require iss to be a single string; jsonwebtoken also accepts an array that contains the issuer. - Refuse tokens with a crit header (RFC 7515 4.1.11), which jsonwebtoken ignores. - Refuse a blank sub. - Treat SCOPES_SUPPORTED or LEEWAY_SECS without ISSUER as a partial config error, like the other OAuth settings. - Cap discovery and JWKS bodies at 1 MiB while streaming them. - Once keys pass their TTL, a request with a known key uses them at once unless it is the one due to refresh, so a slow issuer delays at most one request per 30 s instead of every request. - Docs: the exact algorithm list, leeway on exp and its 0-600 s range, and what --debug logs. Each new test was seen to fail with its fix reverted.
OAuth providers and MCP clients expect an https:// resource URL, so serve needed a TLS-terminating proxy in front of it. With CVE_INTEL_TLS_CERT and CVE_INTEL_TLS_KEY set it now speaks TLS itself. - rustls with the ring provider already in the tree (tokio-rustls added with ring only), TLS 1.2 and 1.3, ALPN http/1.1. - Only one variable set, an unreadable file, an encrypted key or a key that doesn't match the certificate stops startup; there is no fallback to plain HTTP. - Handshakes run in their own tasks with a 10 s limit, so stalled clients can't block the accept loop. - The files are re-read every minute and a valid renewed pair is served without a restart; a broken pair is logged once and the old one kept. - Key file contents never reach logs or errors, including the PEM parser's own error text, which can quote a line as a byte list. Docs: README "Serving HTTPS" (certbot, permissions, ports, Docker), SECURITY.md, .env.example and the Dockerfile.
|
Hi @Fazmin, |
Fazmin
left a comment
There was a problem hiding this comment.
Thanks Paul, this is a big one and it's really well done. Keeping the server as a pure resource server is the right call, and I like that the auth layer wraps everything including the 404 fallback, so a new route can't end up public by accident. The algorithm allow-list, the exact iss check and failing closed at startup all look solid. The .env catch was a great find too, and it's good to know you've run it against a real Keycloak.
I'm happy with the loopback rule, even though it changes things for existing Docker setups. It's the safer default, and the error message tells people exactly what to do.
A few things I'd like sorted before merging. I reproduced 1 to 5 with quick tests against the router.
1. A client that hangs up can freeze the key set (auth.rs ~L596 and ~L620). The JWKS refresh runs inside the request's future, and last is stamped before the fetch. So every 30 s someone can send a token with a random kid and close the socket while the fetch is in flight. Hyper drops the handler, the refresh is cancelled, and last still blocks the next one for 30 s. No valid token is needed. In my test, a token from a newly rotated-in key got a 401 (200 without the disconnects), and a rotated-out key past its TTL kept working. Running the refresh in a detached tokio::spawn that waiters join would fix it.
2. REQUIRED_ROLES can let the wrong people in (auth.rs ~L188). It uses the scope parser, which splits on whitespace, but Keycloak role and group names can contain spaces. CVE_INTEL_OAUTH_REQUIRED_ROLES="CVE Admin,/Security Team" becomes CVE, Admin, /Security and Team, so anyone with a role called Admin gets in and the real role never matches. Separately, a value with only separators (,, say from a template that rendered empty) gives an empty list, which turns the check off, so any valid token gets in. AUDIENCE already refuses that. Could roles split on commas only, and could a set-but-empty list stop startup? That goes for REQUIRED_SCOPES too. The README (~L1080) would need updating as well, since it says spaces work.
3. jsonwebtoken 9.3 has a known advisory, and it reaches us through nbf (auth.rs ~L696). CVE-2026-25537 / GHSA-h395-gr6q-cpjc is fixed in 10.3.0: a claim of the wrong JSON type counts as absent. exp is safe because it's a required claim, but nbf isn't. A correctly signed token with a future nbf got a 401 as a number, and a 200 as a string or as 1e30. It does need the issuer to sign such a token, so it's not an open door. It's still a gap against the documented nbf check, and our audit job doesn't flag it, though other scanners will. As far as I can tell 10.x drops ring for rust_crypto or aws_lc_rs, so I can see why you're on 9.3. Either moving up or rejecting a non-integer nbf next to the iat check works for me, your call. If we stay on 9.x, a comment explaining why would help, since people will ask (it's a bit awkward for a CVE tool).
4. One key we can't parse fails the whole JWKS (auth.rs ~L526). It's parsed as a single JwkSet, so one extra key with alg: ES512, alg: ECDH-ES, crv: X25519 or alg: ML-DSA-65 gives "not a valid key set", even when the RS256 key next to it is fine. At startup, serve refuses to run. If the issuer adds such a key later, every refresh fails with just a warning and new keys get rejected. RFC 7517 section 5 says to ignore keys you don't understand, so reading keys as Vec<Value> and skipping the ones that don't parse as Jwk should do it.
5. There's no header timeout or connection limit (https.rs ~L375). The 10 s limit only covers the TLS handshake. axum::serve sets no hyper timer, so a client that sends half a header just stays connected. Mine was still open after 40 s. I tested that on plain TCP, but it's the same hyper builder after TLS. Since the token check only runs once headers are parsed, an unauthenticated client can hold sockets until file descriptors run out. This wasn't a big deal on loopback, but now that the server is documented for network exposure I'd like a header-read timeout and a connection cap in place.
6. The new .env parser quietly cuts some quoted values (config.rs ~L198). KEY='it's' becomes it, and PASS="pa\"ss" becomes pa\. The old loader kept the text in both cases. You've documented the # rule, but PASS=#abc123 going empty is just as quiet, and since this affects everyone's existing .env, a mangled credential would be hard to track down. Could we handle \" inside double quotes and print a warning (without the value) when something gets cut?
7. The Keycloak walkthrough doesn't work when pasted (README ~L1104). The variables are plain assignments on separate lines, so they never reach HOST=0.0.0.0 cve-intel serve. OAuth stays off and the server refuses to start, telling them to turn on the OAuth they just set up. export or \ continuations, like the HTTPS example, would fix it.
Two I'd like your take on:
- ID tokens (
auth.rs~L690):typis only logged, so nothing confirms it's an access token. With the default audience that's fine, but ifCVE_INTEL_OAUTH_AUDIENCEis the app or client ID, as the README suggests for Entra, an ID token for that client passes every check. Requiringat+jwtwon't work, since as far as I know most providers don't send it by default. Could we refuse tokens carryingnonceorat_hash, or Keycloak'styp: "ID"? - How long stale keys are trusted (
auth.rs~L630): if refreshes keep failing,held_keytrusts cached keys indefinitely, including one the issuer has withdrawn. I agree with keeping them through a short outage, but would a limit of a few TTLs, then 503, make sense?
Smaller things, none blocking:
- Startup only checks that the JWKS has some key. A set with only
use: enckeys passes and logs "1 signing key(s)", then every token gets a 401, and a normal Keycloak realm is reported as 2 signing keys (auth.rs~L532,rest.rs~L195). Counting with the same filter asfind_keyand failing on zero would fix both. - The discovered
jwks_uriis cached for the life of the process (auth.rs~L502), so if the issuer moves it, refreshes fail until a restart. Clearing it after a failed fetch would help. servenow logs at Info withrustlsallowed (cli.rs~L706), so rustls'swarn!lines for things like an illegal SNI show up by default, unlike the--debugonly comment inhttps.rs. Scanners could fill the log with those.claims_summarydoest - nowon an unverified claim (auth.rs~L830). With--debugin a build with overflow checks,exp: i64::MINpanicked before any signature check.saturating_subfixes it.auth_schemelogs any first word up to 16 letters as the scheme (auth.rs~L795), so a short alphabetic API key sent bare would land in the debug log. An allow-list of known schemes would close that.- Tidying:
is_loopback_urlduplicates the one inagent.rs~L926,require_tokencallsbearer()twice and each request computesfingerprint()twice. The OPTIONS bypass also looks unreachable, sinceCorsLayeranswers OPTIONS first.
Really nice work, especially for something this size. Looking forward to the next round!
A client that hung up while a JWKS refresh was in flight cancelled it with the refresh time already stamped, so for 30 s no refresh could run and a newly rotated-in key was refused. The refresh now runs in its own task that holds the refresh lock until it finishes; requests only wait on it. Other key-handling fixes: - The JWKS is parsed key by key, and keys of an unknown type or algorithm are skipped (RFC 7517 section 5) instead of failing the set. Startup counts only keys that can verify tokens and fails on none. - Held keys are trusted for at most 4 h while refreshes fail, then requests get 503. A failed fetch from a discovered jwks_uri runs discovery again and retries at once if it moved. - jsonwebtoken goes from 9.3 to 11.1 (GHSA-h395-gr6q-cpjc: a string or out-of-range nbf counted as absent). Its signature checks use ring through a verify-only provider (jwt_crypto.rs), since the built-in backends would add aws-lc or the rsa crate (RUSTSEC-2023-0071). - ID tokens are refused: a Keycloak typ other than Bearer, a Cognito token_use other than access, or a nonce/at_hash/c_hash/s_hash claim. Configuration and logging: - REQUIRED_ROLES splits on commas only, since role and group names may contain spaces. REQUIRED_ROLES or REQUIRED_SCOPES set but empty stops startup instead of turning the check off. - The debug log names only known Authorization schemes, and claims_summary no longer overflows on extreme times. - Without --debug, serve shows only its own log records; rustls, hyper and reqwest lines need --debug. - is_loopback_url moves to cve_core::validators. require_token parses the header and computes the fingerprint once, and the OPTIONS bypass is gone: CorsLayer answers every OPTIONS request first. serve now runs its own hyper HTTP/1.1 loop, plain or TLS, with a 10 s header-read timeout and at most 512 connections, counted from accept, so slow or silent clients can't exhaust file descriptors before the token check runs. The .env parser no longer cuts values silently. Inside double quotes, \" and \\ are escapes; a quoted value ends at the first closing quote followed by nothing, whitespace or a comment; # starts a comment only after whitespace; export is accepted. Any cut text or ignored line prints one warning naming the line and key, never the value. README: the Keycloak walkthrough uses export so its settings reach serve.
|
Thanks @Fazmin, that's a thorough review, and the repros made it easy. Everything is addressed in the new commit. Item by item:
Your two questions:
Smaller things:
Each new test was checked by reverting its fix: all 13 reverts made a test fail. --PK |
|
Merged, thanks so much Paul! That was a huge piece of work and a really careful round of fixes. Writing the small ring provider instead of pulling in a second crypto stack was a nice call, and the hang-up test is exactly what I was hoping for. I'll call out the new loopback rule in the release notes, so Docker users know why their container stops starting and what the one-line fix is. Two small things to keep in mind for later, no rush at all:
Really appreciate everything you've contributed here. Looking forward to whatever you send next! |
Why
cve-intel servehas no authentication and permissive CORS. That is fine on loopback, but it means the server can't be shared with a team or run in production, and the Docker image already listens on0.0.0.0with nothing in front of it. MCP clients (Claude Code, Cursor, VS Code) now support the MCP authorization spec, so the server can sit behind an existing identity provider without any custom login code. Those clients and providers also expect the server's URL to behttps://, which until now meant putting a TLS-terminating proxy in front of it, so the server can now serve HTTPS itself.Approach
The server acts as an OAuth 2.1 resource server only. It never issues tokens, never sees a password and needs no client secret. It validates JWT access tokens against the issuer's published keys and advertises where to log in through RFC 9728 protected-resource metadata, which is what the MCP authorization spec requires. OAuth is off unless
CVE_INTEL_OAUTH_ISSUERandCVE_INTEL_OAUTH_RESOURCEare set.Decisions worth reviewing:
servenow refuses a non-loopbackHOST(Docker's0.0.0.0included) unlessCVE_INTEL_ALLOW_UNAUTHENTICATED=1. Existing container deployments need either OAuth or that flag. The Dockerfile comments and README explain both./healthand the discovery document stay open./healthis trimmed to{"status":"ok"}when OAuth is on. The full report made an uncached NVD call and showed which keys are set, so it moves toGET /api/v1/health, which needs a token.503, never a pass. While refreshes fail, the keys already held are trusted for at most 4 hours, then requests get503too.REQUIRED_ROLESorREQUIRED_SCOPESset but empty stops startup instead of turning its check off. The key-fetching HTTP client fails closed if it can't be built.CVE_INTEL_OAUTH_REQUIRED_ROLESlimits which users get in (Keycloak realm/client roles, groups, Entra app roles; exact matches, comma-separated because names may contain spaces). A refused user gets a plain403 access_deniedthat doesn't name the allowed roles. Scopes limit which clients connect, not which users.typ: at+jwt, so the claims decide: a Keycloaktypother thanBearer, a Cognitotoken_useother thanaccess, or (without those) anonce,at_hash,c_hashors_hashclaim.nbfof the wrong type (GHSA-h395-gr6q-cpjc). From 10.0 on it needs a crypto provider, and its built-in ones add aws-lc or thersacrate, which failscargo auditon RUSTSEC-2023-0071. Sojwt_crypto.rsis a small verify-only provider on ring, the library rustls already uses here. It refuses to sign.serveruns its own hyper HTTP/1.1 loop instead ofaxum::serve, which sets no timer. Every connection, plain or TLS, must send a complete header within 10 s, and at most 512 are open at once. Both are constants, not settings.CVE_INTEL_TLS_CERTandCVE_INTEL_TLS_KEYset,servespeaks TLS itself; a proxy in front still works as before. Certificates come from the operator (certbot, a corporate CA, cert-manager); there is no built-in ACME client. Renewed files are picked up without a restart. HTTPS doesn't change who may connect: a non-loopback bind still needs OAuth or the opt-out..envloader fix (affects everyone, not only OAuth). The loader read inline comments as part of the value, and.env.examplehas one after nearly every key. So copying it to.env, as the README says, setNVD_API_KEYandGITHUB_TOKENto comment text, and onCVE_INTEL_OAUTH_REQUIRED_ROLESit would have added the comment's words as allowed roles. Unquoted values now end at a#that follows whitespace (abc#defandPASS=#abcare kept). Quoted values end at the first closing quote followed by nothing, whitespace or a comment, so'it's'stays whole, and\"and\\are escapes inside double quotes.export KEY=is accepted. Any cut text or ignored line prints one warning naming the line and key, never the value..env.exampleputs comments on their own lines, becausedocker run --env-filenever strips them.How it works
crates/cve-server/src/auth.rs): asymmetric algorithms only (RS256/384/512, PS256/384/512, ES256, ES384, EdDSA); nocritheader (RFC 7515 4.1.11; jsonwebtoken ignores it);issa single string equal to the configured issuer (jsonwebtoken alone also accepts an array containing it);audcontaining the resource (orCVE_INTEL_OAUTH_AUDIENCE);exp/nbf/iatwith a configurable leeway of 0-600 s; asubthat isn't blank; optional required scopes fromscopeorscp; not an ID token. Anexpornbfthat isn't a number in range rejects the token. Keys marked"use": "enc", symmetric keys and keys naming another algorithm never verify. Signatures are checked by ring throughjwt_crypto.rs.CVE_INTEL_OAUTH_JWKS_URI), cached for an hour, refetched on an unknownkidat most every 30 seconds. Refreshes are serialised, so a burst of forged tokens with random key IDs waits for one fetch instead of flooding the issuer. Each refresh runs in its own task, so a client hanging up can't cancel it and leave the 30 s limit blocking a newly rotated-in key. Keys of a type or algorithm the server doesn't understand are skipped (RFC 7517 section 5). Keys already held survive a failed refresh for up to 4 hours. A failed fetch from a discoveredjwks_uriruns discovery again, so a moved key set is found without a restart. Once the keys pass their TTL, a request with a known key uses them at once unless it is the one due to refresh, so a slow or unreachable issuer delays at most one request per 30 s. Discovery and JWKS bodies are capped at 1 MiB while streaming.401withWWW-Authenticate: Bearer resource_metadata=...,403 insufficient_scope,403 access_denied, or503when keys can't load. Metadata is served at/.well-known/oauth-protected-resource/mcp.--debug servelogs the whole exchange (discovery, JWKS, metadata served, each token's header and standard claims, every check, the challenge sent). Each accepted request is logged with its user and client, and each MCP tool call with its tool name. Tokens appear only as a SHA-256 fingerprint; the token, signature andAuthorizationheader are never logged, even when malformed. Claim text is escaped withclean_text(control characters and bidi overrides shown as\u{..}), since the debug log shows it before the signature is checked. A non-bearerAuthorizationheader is logged only as a known scheme name. Without--debug,serveshows only its own records; rustls, hyper and reqwest lines (a scanner's illegal SNI, say) need--debug. Scopes are checked against RFC 6749's character set because they are quoted intoWWW-Authenticate.crates/cve-server/src/https.rs): rustls with the ring provider already in the tree, TLS 1.2 and 1.3, ALPNhttp/1.1(axum is built without HTTP/2). There is no fallback to plain HTTP: only one variable set, an unreadable or oversized file, an encrypted key, or a key that doesn't match the certificate stops startup with a message naming the file. Each handshake runs in its own task with a 10 s limit, so stalled clients can't block the accept loop, and then joins the shared connection loop inlisten.rs(header timeout, connection cap). The certificate is the TLS config's resolver; the files are re-read every minute, and a changed pair is swapped in only if it loads and matches, otherwise the error is logged once and the previous certificate stays. Key file contents never reach logs or errors; the PEM parser's own error text is dropped for the key because it can quote a line as a byte list. Startup logs the certificate's SHA-256 fingerprint inopenssl x509 -fingerprintform.rest.rsloads the OAuth config at startup, refuses an unauthenticated non-loopback bind, and adds the auth middleware and routes.cli.rsturns on info-level logging forserveso operators see refused requests.jsonwebtoken11.1 (no default features, no crypto backend; ring throughjwt_crypto.rs),ring(already in the tree through rustls),sha2,base64,url, andreqwestfrom the workspace,tokio-rustls(ring only, so rustls keeps one crypto provider), andhyper/hyper-util(already in the tree through axum) for the connection loop.cargo auditreports nothing new, and OSV has no advisories for jsonwebtoken 11.1..env.exampleand the Dockerfile describe the new settings and the loopback rule.Testing:
cargo fmt --check, clippy with-D warnings,cargo test -p cve-core -p cve-server, andcargo check -p cve-intel-desktop.tests/oauth.rs(19 tests) drives the real router with test keys (RSA, EC, Ed25519) and a rotated JWKS: valid and invalid tokens on MCP and REST, wrong issuer/audience/algorithm, anissarray, acritheader, expiry, scopes, roles, key rotation, a forged token carrying terminal escapes, an oversized JWKS, a slow issuer with stale keys, the/healthsplit and the metadata document. Added in review: a client hanging up mid-refresh (Fazmin's repro: 401 before the fix, 200 after), a JWKS with ES512/X25519/ML-DSA/enc keys, held keys expiring after the max age, a movedjwks_uri, malformednbf/exp, ID tokens, and CORS preflights needing no token. Fixtures and how they were generated are intests/fixtures/README.md.auth.rscover config parsing (including partial configs, comma-only roles and empty required lists), role and scope matching across providers' claim shapes, ID-token markers, JWKS parsing and key selection,critdetection and the logging rules. 2 injwt_crypto.rssign with ring and verify all nine algorithms, plus tampered messages, wrong keys, wrong key types, HMAC and signing (refused). The integration tests capture the server's logs and fail if any rejected token, or any raw control or bidi character, appears in them.tests/https.rs(4 tests) serves the real router over TLS to a rustls client that trusts only a test CA: MCP over HTTPS, no answer to plain HTTP, stalled handshakes not blocking other clients, a stalled handshake dropped at the limit, and a renewed certificate served without a restart (with a half-finished renewal kept on the old one). 5 unit tests inhttps.rscover config parsing, ECDSA PKCS#8 and RSA PKCS#1 keys, mismatched, swapped, missing, oversized and encrypted files, and fail if malformed key files' contents appear in any error. Each test was seen to fail with the code it guards broken.openssl s_clientandcurl: TLS 1.0/1.1 refused,h2-only ALPN refused, full chains served, the startup fingerprint matching openssl's, a real renewal picked up within a minute, and a broken one warned about once.tests/limits.rs(6 tests) runs the real connection loop with small limits: half a header, a silent connect and an idle keep-alive connection are closed (over plain HTTP and TLS), and with a cap of 2 a third client waits until one closes. Against the real binary, all three timeouts closed at 10.0 s.config.rstests the.envline parser with a table of cases (quotes, escapes,#rules,export, CRLF), checks that no warning ever contains a value, and fails if.env.examplegains an inline comment or an activeHOSTline.auth.rs, plus the.envparser and the connection limits).alg:none, HS256 key confusion, embeddedjwk/jku/x5u, malformed claims, path variants (/MCP,//mcp,/%6dcp,..), header shapes, and 200 concurrent unknownkids (no extra JWKS fetches).