Make service auth a first-class identity alongside OAuth#59
Merged
Conversation
When a server is configured with both global OAuth and per-server serviceAuths, each authenticator independently rejects requests it doesn't recognize, so chaining them composes failures rather than successes. Either ServiceAuth eats valid OAuth tokens, or OAuth eats valid serviceAuths credentials, depending on chain order. Each authenticator becomes non-rejecting — sets context on success, passes through otherwise. A single RequireAuth gate at the end of the chain produces the 401 with the appropriate WWW-Authenticate (RFC 9728 Bearer when OAuth is enabled, Basic realm otherwise). Any one configured method succeeding now satisfies the route, regardless of chain order or which other methods are also configured.
Skipping bcrypt when the username didn't match a configured entry let an attacker enumerate valid usernames by measuring response time — configured users took ~50ms, unknown users returned in microseconds. Always run bcrypt against either the matched entry's hash or a dummy hash generated once at construction time. Authentication only succeeds when both the username matched and bcrypt verified the password.
Without this, configuring `tokens: [""]` lets `Authorization: Bearer ` (scheme with no token) authenticate against the empty entry.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Symmetric to the empty bearer token check. Without this, a literal
`password: ""` in the config would let `Authorization: Basic <base64("user:")>`
authenticate. Env var references are already rejected upstream by
ParseConfigValue when the variable is unset or empty, so this check
specifically catches the explicit-empty-string case.
dgellow
changed the title
Contributor
Author
|
/gemini review |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ServiceAuth requests showed up in logs as user="" and bypassed all per-user limits, because nothing populated the user identity for them. Each entry now has a `name` (required for bearer, defaults to username for basic) and resolves to a per-server identity like `<server>.<name>@serviceauth.mcpfront.alt`. Logs, session keys, and per-user limits all see this identity the same way they see an OAuth user's email. Names must be unique within a server's serviceAuths. The reserved domain is rejected if an OAuth identity provider ever returns it, so a real user can't impersonate a service identity.
dgellow
force-pushed
the
sam/auth-trier-gate
branch
from
427e3b5 to
043c9ff
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #58.
When a server is configured with both global OAuth and per-server
serviceAuths, the two authenticators previously fought each other: whichever ran first rejected anything it didn't recognize, even when the other one would have accepted it. Configuring service tokens alongside OAuth meant either OAuth users or service callers worked, never both, depending on chain order. The PR that closed the original report fixed one direction; this branch fixes both.Beyond that, service auth requests had no real downstream identity. They surfaced in logs as
user="", slipped past per-user session and connection limits, and weren't tracked in storage. To audit or rate-limit them you'd have had nothing to key on. EachserviceAuthsentry now carries aname(required for bearer, optional for basic — defaults to the username) that resolves to a per-server identity. That identity flows through every place that already used the OAuth user's email: logs, session keys, limit pools, audit trails. A bearer token shared across many CI runners shares one identity by design; separate entries get separate pools.A few security items got picked up along the way. Basic auth now runs bcrypt against a dummy hash on no-match, so response timing no longer reveals which usernames exist. Empty bearer tokens and empty basic passwords are rejected at config load — closes the case where a misconfigured
{"$env":"VAR"}would silently authenticateAuthorization: Bearer(no token). The identity domain used for service auth is reserved (RFC 9476's.alt); the OAuth flow refuses any IDP-claimed email in that domain, so a real user can't impersonate a service identity.Existing configs with bearer entries need a one-line addition (
"name": "...") — there's no sensible default for bearer since the array of tokens has no admin-visible label.