Duansg commented on PR #4279:
URL: https://github.com/apache/hertzbeat/pull/4279#issuecomment-5162797973
The problems this targets are real, and I verified each on master:
- oauth.rs:74 ships a hardcoded client_secret: Some("mcp-client-secret") for
the default client, published in the repo;
- oauth.rs:591 advertises code_challenge_methods_supported: ["S256"], but
code_verifier appears exactly once in the whole file — as a field declaration
on the token request struct (line 235) — and is never read. The server
advertises PKCE it does not enforce;
- dynamic client registration is unbounded.
The new PKCE implementation looks correct to me: S256 only, URL_SAFE_NO_PAD
over Sha256::digest, subtle::ct_eq for the comparison, verifier length and
charset per RFC 7636, and consuming the code on verifier failure so it can't
become a brute-force oracle. Single-use codes and refresh tokens, plus hard
cardinality caps on every remotely-growable map, are all good. Two issues
though.
Refresh token is removed before issuance can fail
exchange_refresh_token deletes the record first and issues afterwards:
let record =
refresh_tokens.remove(&request.refresh_token).ok_or(OAuthError::InvalidGrant)?;
drop(refresh_tokens);
self.issue_tokens(record.client_id, record.scope).await
issue_tokens can legitimately fail — TemporarilyUnavailable when
access_tokens.len() >= MAX_ACCESS_TOKENS or refresh_tokens.len() >=
MAX_REFRESH_TOKENS (both 4096), and InvalidClient if the client record has
expired and been swept. In every one of those cases the old refresh token is
already gone, so the client is permanently logged out and has to re-run the
full authorization flow. Near the caps this stops being a corner case and
becomes systematic — exactly when a server is busiest.
exchange_authorization_code has the same shape. It matters less there
because codes are single-use by design and the client can re-authorize, but
it's the same class of bug.
Suggestion: issue first, then remove; or restore the record on the error
path.
state has no length bound
state is copied verbatim into AuthorizationTransaction in
create_authorization_transaction with no validation. MAX_AUTH_TRANSACTIONS caps
the count at 4096, and MAX_OAUTH_BODY_BYTES bounds POST bodies — but /authorize
is a GET, so neither constrains the size of an individual state. 4096
transactions each holding an arbitrarily large state is a memory amplification
vector reachable before any authentication.
Suggestion: cap state (RFC 6749 doesn't mandate a size, but a few hundred
bytes is generous) and reject anything longer with invalid_request.
Hardening, not blocking
The approval endpoint has no failure throttling. validate_approval_secret
uses ct_eq, so timing is covered, but nothing rate-limits guesses. In practice
this is bounded by main.rs::approval_secret_for_mode, which requires
MCP_OAUTH_APPROVAL_SECRET to be set and at least 32 characters in production
mode — enumeration at that length isn't feasible. Worth noting though that 32
characters is a length floor, not an entropy floor, so a per-source failure
counter plus a warning log would still be cheap insurance for the operator who
sets 32 identical characters.
Also: refresh tokens are rotated but a replayed old token only returns
InvalidGrant — it doesn't revoke the token family. OAuth 2.1 / RFC 6819 suggest
treating reuse as a compromise signal and invalidating the descendants.
Optional, but it's nearly free given you already have client_id on the record.
On the size of this PR
This is a ~1,600-line rewrite of an authorization server bundling roughly
ten independent changes — PKCE, client authentication methods, default
credential removal, consent-nonce binding, redirect URI enforcement, body size
limits, TTLs across five object types, registration rate limiting,
trusted-proxy resolution, and client lifetime management. Each is defensible;
reviewing them as one unit is hard, and if any single piece needs to be
reverted the whole thing goes with it. I'd suggest splitting into a sequence —
the default-credential removal and PKCE enforcement alone would be an easy,
high-value first PR.
To be clear, that's a process concern and it doesn't substitute for the two
issues above, which I think need fixing regardless of how the work is packaged.
Minor
sha2, subtle and base64 are new dependencies; they'll need to go through the
usual ASF dependency license review for LICENSE/NOTICE.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
