Skip to content

Document the ID-token gap and make the header timeout configurable - #11

Merged
Fazmin merged 3 commits into
Fazmin:mainfrom
pkilar:feat/serve-followups
Nov 11, 2026
Merged

Fazmin merged 3 commits into
Fazmin:mainfrom
pkilar:feat/serve-followups

Conversation

@pkilar

@pkilar pkilar commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

Why

These are the two follow-ups from Fazmin's note after merging #10:

  • ID tokens. The ID-token check can't catch an ID token that carries none of its markers. An Entra ID token from the code exchange, without a nonce, is one, so it passes when the audience is the signing-in client's own ID. The README didn't say so.
  • Keep-alive timeout. The fixed 10 s header-read timeout also closes idle keep-alive connections. A load balancer that keeps them open longer (AWS ALB: 60 s by default) can send a request on one just as the server closes it, and return a 502.

Approach

  • ID tokens: documentation only. Without a marker, such an ID token has nothing in its claims that tells it apart from an access token. The fix belongs in how Entra is set up:

    • Separate app registrations for the API and its clients, so a client's ID token fails the audience check.
    • Or a required scope, since Entra puts scp only in access tokens issued to users.

    Required roles don't help, because Entra puts app roles in ID tokens too. I checked these points against Microsoft's ID token and access token claims references.

  • Timeout: one setting, CVE_INTEL_HEADER_TIMEOUT_SECS. hyper's HTTP/1 server closes idle keep-alive connections with the same header-read timeout, so the two can't be set separately.

    • The name follows CVE_INTEL_OAUTH_LEEWAY_SECS.
    • It takes 1–3600 whole seconds. Unset or empty keeps 10 s, like the other settings.
    • Anything else stops serve at startup, so a typo like 65s can't quietly keep the default.
    • A longer value also gives a stalled client longer to hold a connection, so the README recommends raising it only behind a proxy or load balancer.

How it works

  • listen::Limits::from_env() / from_lookup() read the setting.

  • serve() parses it together with the TLS and OAuth settings, before any network call. It prints the value at startup when it isn't 10.

  • README, SECURITY.md and .env.example explain when to raise it, e.g. 65 behind an ALB. The README's ID-token paragraph and SECURITY.md now describe the gap, and a new "Entra ID and ID tokens" paragraph in the README gives the two setups.

  • Tests:

    • Unit tests for the parser: bounds, trimming, 65s, 1.5, 0, 3601, overflow.
    • tests/serve_settings.rs runs the cve-intel serve binary in an empty temp home. 65s must stop it with an error naming the setting, and 65 must print the startup line.

    Each test failed with its code reverted. That includes putting Limits::default() back in serve(), which the unit tests alone didn't catch.

  • Against the binary with the setting at 3, a silent connection and an idle keep-alive connection both closed at 3.0 s.

  • cargo fmt --check, clippy with -D warnings, cargo test -p cve-core -p cve-server (399 passed) and cargo check -p cve-intel-desktop all pass locally.

Links

The check refuses an ID token only by its markers (Keycloak typ, Cognito
token_use, nonce, at_hash, c_hash, s_hash). An Entra ID token from the
code exchange without a nonce has none, so it passes when the audience
is the signing-in client's own ID. The README and SECURITY.md now say
so, and the README gives the two Entra setups that close it: separate
app registrations for the API and its clients, or a required scope,
since Entra puts scp only in user access tokens.
hyper's header-read timeout also closes idle keep-alive connections, so
the fixed 10 s made a load balancer that keeps connections open longer
(AWS ALB: 60 s) occasionally send a request on a socket the server was
closing and answer it with a 502.

CVE_INTEL_HEADER_TIMEOUT_SECS sets it, 1-3600 whole seconds; unset or
empty keeps 10 s. Anything else stops serve at startup with an error
that names the variable, so a typo like "65s" can't quietly keep the
default. serve prints the value at startup when it isn't the default.
README, SECURITY.md and .env.example say when to raise it and what a
longer value costs.
The parser's unit tests can't see serve() itself, so putting
Limits::default() back in serve passed the whole suite. These tests run
the cve-intel binary with an empty temp home, the developer's CVE_*,
HOST and PORT removed, and proxies pointed at a closed port: "65s" must
stop serve with an error naming the setting and its value, and "65"
must print the startup line. Both fail with the serve change reverted.
@Fazmin
Fazmin merged commit 5c82414 into Fazmin:main Nov 11, 2026
5 checks passed
@Fazmin

Fazmin commented Nov 11, 2026

Copy link
Copy Markdown
Owner

Merged, thanks Paul! And sorry it took me a few days to get to this one, you were much quicker following up than I was.

Both changes are exactly right. You're right that the ID token gap is really a setup problem, and the Entra section explains the two fixes clearly. Making a typo like 65s stop serve instead of quietly keeping 10 s is a nice touch too.

One tiny thing for whenever you're next in there, not worth a PR of its own: tests/serve_settings.rs says .env is only read from the temp folder, but the loader also falls back to $HOME/.env. So on a machine with OAuth settings in ~/.env, the startup test could pick them up. Setting HOME (and USERPROFILE) to the temp folder too would keep it fully isolated.

Thanks again for all of this, it's been great working through these with you.

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.

2 participants