Add RFC 8628 device authorization grant support - #6647
Conversation
Headless MCP clients (remote dev hosts, CI-adjacent operator boxes) cannot complete the browser-based authorization-code callback this auth server currently requires. RFC 8628 (Device Authorization Grant) lets such a client obtain a device/user code, hand the user code to a human for out-of-band verification, and poll for a token without ever receiving a redirect itself. This is the storage foundation only, mirroring the existing PendingAuthorizationStorage shape: - DeviceRequest/DeviceRequestStatus and the DeviceCodeStorage interface (types.go), embedded into Storage alongside PendingAuthorizationStorage. - MemoryStorage and RedisStorage implementations, each keyed by both device_code (canonical) and user_code (secondary index), TTL-bound via DefaultDeviceRequestTTL. - ErrInvalidState distinguishes "already authorized/denied" from not-found/expired, so a stale verification-page resubmission can never clobber a request the token endpoint already consumed. No HTTP endpoints, token-endpoint grant handler, or config/CRD surface yet -- those land in follow-up PRs once this storage layer is in. Generated with [Claude Code](https://claude.com/claude-code)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6647 +/- ##
==========================================
+ Coverage 79.09% 79.18% +0.09%
==========================================
Files 785 788 +3
Lines 78381 78941 +560
==========================================
+ Hits 61992 62506 +514
- Misses 16384 16430 +46
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jhrozek
left a comment
There was a problem hiding this comment.
Ran a multi-angle review (security, RFC 8628 compliance, ToolHive conventions, architecture/doc-sync) plus cross-checked a separate independent review against the actual code. Summary: 3 HIGH inline (+1 more HIGH-severity gap noted below, in a file this PR doesn't touch so it can't be attached inline), 2 LOW-MEDIUM, 3 LOW.
The storage/test engineering itself is solid (memory+Redis parity tests, correctly regenerated mocks, RFC-correct response shapes/error vocabulary/discovery metadata, single-use redemption in the memory backend, no secret leakage). The blockers are all in three places: Redis-backend state-transition atomicity, missing client authentication on /oauth/device_authorization, and the fact that the feature has no way to be completed by a real client yet (no verification-page route, and no way to register a device-code-capable client — see the inline comment on device_authorization.go:98 for detail on the latter, since pkg/authserver/server/registration/dcr.go isn't touched by this PR and can't take an inline comment). None of this is a design problem — it reads like a deliberately staged PR — but it should either land together or be called out explicitly with DeviceFlowEnabled kept off until the rest lands.
Also needs a look (not part of this diff, so no inline comment): docs/arch/11-auth-server-storage.md documents the storage interfaces and config flags for the other opt-in grants (JWT-bearer, token-exchange) in detail but wasn't updated for the new DeviceCodeStorage interface, DeviceRequest lifecycle, or DeviceFlowEnabled/DeviceCodeInterval config knobs.
DeleteDeviceRequest and updateDeviceRequest did an unconditional get-then-write, so two concurrent callers could both observe the pre-mutation state and both commit: a device_code could be redeemed twice, or a double-click authorize/deny could silently overwrite the first transition. Both now run inside a Redis WATCH/MULTI transaction, mirroring UpsertDCRIssuedClient, so the loser retries against the now-updated record instead of racing ahead. Also store LastPolledAt at nanosecond precision instead of truncating to whole seconds, since enforcePollInterval compares it against MinInterval and a poll landing fractionally under the interval must not appear to have waited long enough merely due to storage rounding. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Every other optional storage capability (DCRCredentialStore, AssertionJWTConsumer) is asserted out of Storage at the boundary rather than embedded, so a future backend need not implement methods for a capability it never enables. DeviceCodeStorage broke that pattern and, more concretely, made CIMD/SPIFFE-decorated storage stop satisfying it: those decorators only implement the narrower Storage interface they wrap, not the embedded extras. handlers.NewHandler and the deviceflow factory wiring now resolve DeviceCodeStorage via storage.Unwrap(stor).(storage.DeviceCodeStorage), the same unwrap-then-assert pattern already used for DCRCredentialStore. Handler gains a deviceStorage field so DeviceAuthorizationHandler goes through it instead of the narrowed Storage interface. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
/oauth/device_authorization only did a bare GetClient lookup, with no client_secret check, letting anyone who learns a confidential client's client_id mint device_code/user_code pairs on its behalf. RFC 8628 Section 3.1 requires the same client authentication the token endpoint uses. Route through h.provider's AuthenticateClient (asserted via a local clientAuthenticator interface, since fosite.OAuth2Provider does not expose it) so client_secret_basic/post, private_key_jwt, and the SPIFFE dispatcher are all enforced identically, while public clients still identify by client_id alone. Also fix a modulo bias in generateUserCode: reduce with '%' mapped crypto/rand bytes onto the 30-character charset unevenly (256 % 30 = 16 leftover values). Reject and redraw bytes >= 240 instead so every retained byte maps uniformly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
DeviceCodeInterval is plumbed all the way to the token-endpoint handler but embeddedauthserver.go's Config construction never sets it -- only DeviceFlowEnabled is wired through today, so the field is reachable only from tests that construct Config directly. Note this on the field so a future reader does not assume it is already exposed end-to-end. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Regenerate mock_storage.go via 'task gen' so MockStorage no longer carries the DeviceCodeStorage methods removed from Storage. - Reword a doc comment to avoid a codespell false positive on "[c]lient" (bracket-lowercase mid-sentence quoting reads as "lient" once codespell strips the brackets). - Warm fosite.Config's lazily-initialized secrets hasher once, single- threaded, before TestDeviceAuthorizationHandler_ClientAuthentication spawns parallel subtests sharing one provider -- fosite.Config. GetSecretsHasher has no internal synchronization around that lazy init, so concurrent first calls raced on it under -race. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Add storage for device-flow verification-page logins The RFC 8628 verification page needs to correlate two short-lived, single-use pieces of state: the upstream IDP login attempt in flight (state -> device_code/user_code plus PKCE/nonce) and, after that login resolves, the identity awaiting an explicit Approve/Deny decision. Add PendingDeviceLoginStorage and PendingDeviceConfirmationStorage, modeled directly on the existing PendingAuthorizationStorage (same shape of problem: short-lived, state-keyed correlation data with no new architectural pattern needed), implemented for both MemoryStorage and RedisStorage and embedded into Storage alongside PendingAuthorizationStorage. The confirmation record is deliberately addressed only by an opaque, unguessable token handed to the browser as a hidden form field -- the resolved identity itself is never round-tripped through the browser as editable form data, closing an obvious tamper vector. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Allow DCR registration of device_code-only clients pkg/authserver/server/registration/dcr.go's grant-type allowlist only ever permitted authorization_code/refresh_token, so there was no way to dynamically register a client allowed to use the RFC 8628 device_code grant -- a real client could never be provisioned to use the device flow added in #6647. Add a device-code-only carve-out (grant_types exactly {device_code} or {device_code, refresh_token}) parallel to the existing private_key_jwt carve-out: redirect_uris and response_types are not required, since a device-code client never receives a redirect. Mixing device_code into an authorization_code registration is intentionally not covered -- such a request still needs a redirect_uri/response_type as before. FilterPublicGrantTypes/FilterPublicResponseTypes (used by the CIMD client-metadata-document path) share the same allowlist and now start admitting device_code too; existing tests that asserted device_code was silently filtered out are updated to reflect it being kept. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Add the device flow verification page Closes the gap left by #6647: /oauth/device_authorization mints a device_code/user_code pair whose verification_uri points at GET /oauth/device, but that route didn't exist -- storage. MarkDeviceRequestAuthorized/MarkDeviceRequestDenied were only ever called by the integration test, directly against storage. Add DeviceVerificationHandler (GET/POST /oauth/device) and DeviceVerificationConfirmHandler (POST /oauth/device/confirm), backed by html/template pages (the first HTML surface in this package -- every other handler writes JSON). The flow: enter/confirm the user_code, redirect to the first configured upstream IDP to authenticate, land on an explicit Approve/Deny confirmation screen, then call MarkDeviceRequestAuthorized or MarkDeviceRequestDenied. The upstream IDP's redirect_uri is fixed per upstream at construction (see upstream.OAuth2Config.RedirectURI / AuthCodeURL), so there is no way to register a distinct callback path for this second login flow. CallbackHandler now dispatches between the OAuth-client authorization_code flow and a device-flow login based on which pending record the callback's state parameter matches, falling back to completeDeviceLogin (device_verification.go) when it doesn't match a PendingAuthorization. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Drive the device flow integration test through the real HTTP page TestIntegration_DeviceFlow_FullHappyPath previously simulated the not-yet-built verification page by calling storage. MarkDeviceRequestAuthorized directly. Now that the page exists, wire the test through a real mock upstream IDP (mockoidc) and the actual HTTP flow: submit user_code, follow the upstream login redirect, land on the shared /oauth/callback endpoint, extract the confirm_token from the rendered confirmation page, and POST the approval -- exercising the exact path a real device-flow client and human now go through end to end, closing the loop the original PR's integration test left open. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix device flow review findings from PR #6682 The deny action on the confirmation page always reported success even when the underlying storage write failed, so a user believed they had denied a device whose request could remain authorized. The shared /oauth/callback dispatch also collapsed a genuine storage outage into the same "not found" path used for an expired state, masking backend incidents as client errors. Neither the verification page's user_code guesses nor its confirm_token replay had rate limiting or clickjacking protection, unlike the existing device_authorization endpoint. - Mirror the approve branch's error handling on deny; log at Warn. - Distinguish not-found/expired from real storage errors in the callback dispatch and the user_code re-checks, returning 500 on a genuine backend failure instead of a generic 400. - Rate-limit POST /oauth/device, matching /oauth/device_authorization. - Add anti-framing/no-sniff headers to the verification page renders. - Add tests for deny-then-replay and a concurrent confirm_token race. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Restyle device flow pages with ToolHive brand palette Give the three device-flow HTML pages (verify, confirm, result) a proper visual identity instead of unstyled markup: ToolHive/Stacklok brand colors, light/dark support via prefers-color-scheme, pill buttons, and a card layout, with no external font/CDN dependency so the pages still work in air-gapped deployments. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Use oauthproto grant-type constants in DCR allowlist authorization_code and refresh_token were hardcoded as string literals in dcr.go even though oauthproto already exported constants for both, unlike the device_code entry added alongside them. Use the constants everywhere for consistency. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Move device flow page rendering into its own file Split the HTML templates, page-data structs, and render* helpers out of device_verification.go into device_verification_render.go, so the request-handling logic isn't interleaved with ~290 lines of markup and CSS. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Carry upstream tokens on PendingDeviceConfirmation Device-flow sessions have no session id until confirm time, so the tokens exchanged at login had nowhere to be written yet and were being dropped entirely -- any downstream lookup by session id (token injection, refresh) found nothing for a device-flow session. - Add UpstreamTokens/Synthetic fields to PendingDeviceConfirmation so the handler layer can persist them once a session id exists. - Extract newStoredUpstreamTokens from marshalUpstreamTokensWithTTL so both call sites share the same epoch-time conversion. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix device-flow session/error handling and IDP routing - Persist the upstream tokens captured at login under the device grant's final session id on approval (see the storage commit just before this one) -- previously nothing ever called StoreUpstreamTokens for a device-flow session. - Give handleUpstreamError a device-flow fallback: an upstream IDP error (e.g. the human denies at the upstream login screen) left the DeviceRequest Pending until the device_code itself expired, since nothing marked it denied on that path. - Require exactly one configured upstream at submit time instead of silently defaulting to h.upstreams[0] -- device flow has no client_id/redirect_uri of its own to route per-client, so a multi-upstream deployment would otherwise authenticate against whichever upstream is configured first. - Fix a garbled doc comment on DeviceVerificationHandler. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Slice the first rune, not byte, for confirm-page initials fields[0][:1] sliced the first byte of a display name, splitting multi-byte UTF-8 sequences and rendering invalid UTF-8/replacement characters in the avatar badge for non-ASCII names. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Rate-limit /oauth/device per IP instead of per process deviceVerificationLimiter was a single process-wide bucket, unlike the machine-driven, one-shot endpoints it was modeled on (registerLimiter et al). /oauth/device is a human-facing login page a legitimate user retries against, so one caller burning the shared burst starved every other concurrent device-flow login on the server. Add a small per-IP limiter (with inline idle-entry eviction, no background goroutine to manage) and key the gate on RemoteAddr -- deliberately not X-Forwarded-For, which is attacker-controlled without a trusted proxy list. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Add tests for device-flow review fixes Covers: upstream tokens retrievable by session id after approval, denial on upstream IDP error, the single-upstream requirement, non-ASCII initials, and per-IP rate-limiter scoping. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Summary
Headless MCP clients (remote dev hosts, CI-adjacent operator boxes, and
similar non-desktop environments) cannot complete the browser-based
authorization-code callback this embedded auth server currently requires to
obtain a user grant. This PR adds OAuth 2.0 Device Authorization Grant
support (RFC 8628): such a client can obtain a device/user code pair, hand
the user code to a human for out-of-band verification on any browser, and
poll for a token without ever receiving a redirect itself.
Everything needed to actually mint a token is here, end to end:
DeviceRequest/DeviceRequestStatusand theDeviceCodeStorageinterface (storage/types.go), embedded intoStoragealongside
PendingAuthorizationStorage;MemoryStorageandRedisStorageimplementations, each keyed by both
device_code(canonical) anduser_code(secondary index), TTL-bound viaDefaultDeviceRequestTTL(10minutes).
ErrInvalidStatedistinguishes "already authorized/denied" fromnot-found/expired, so a stale verification-page resubmission can never
clobber a request the token endpoint already consumed.
POST /oauth/device_authorization(handlers/device_authorization.go)— issues the device_code/user_code pair per RFC 8628 §3.1/§3.2, rate
limited like
/oauth/register, only mounted when the newDeviceFlowEnabledconfig flag is set.urn:ietf:params:oauth:grant-type:device_codetoken grant(
server/deviceflow) — afosite.TokenEndpointHandlerenforcing RFC 8628§3.5 polling semantics (
authorization_pending,slow_down,expired_token,access_denied), wired intobuildProvideralongside theexisting token-exchange/JWT-bearer factories, issuing both an access token
and (when the client supports
refresh_token) a refresh token, andconsuming the device_code so it cannot be redeemed twice.
device_authorization_endpointand the grant type areadvertised only when
DeviceFlowEnabledis set.RunConfig.DeviceFlowEnabled(off by default, matching everyother optional grant in this codebase), threaded through
runner/embeddedauthserver.go.Explicitly not in this PR: the human-facing verification page
(
GET /oauth/device) that would actually callMarkDeviceRequestAuthorized/MarkDeviceRequestDeniedfrom a real upstream-IdP login — the integrationtest drives that transition directly against storage to prove the rest of
the pipeline end to end. Relatedly,
pkg/authserver/server/registration/dcr.go'sdefaultGrantTypes/allowedGrantTypesonly ever permitauthorization_code/refresh_tokenfor dynamically registered clients, sothere is also no supported way yet to register a client allowed to use the
device_codegrant. CRD/operator exposure (cmd/thv-operator/api/v1beta1)is also out of scope, matching the existing precedent for
IdentityFromTokenConfig(config lands inpkg/authserverfirst, operatorsurface follows separately).
DeviceFlowEnabledmust stay off in any real deployment until bothfollow-ups above land. With it on,
/oauth/device_authorizationhappilyadvertises a working RFC 8628 grant, but there is no route for a human to
complete the verification step and no way to register a client permitted to
use the grant — so a real client would poll forever and 404 at the one step
it actually needs. This flag exists today only so the storage/grant-handler
layers can be reviewed and tested end to end (as the integration test does,
by driving the authorization transition directly against storage).
Related: stacklok/stacklok-enterprise-platform#4127 (the enterprise
distribution issue that motivated this — Connector Gateway's embedded auth
server is this package; AI Gateway's half of that issue is unrelated
client-side
thv llmwork, tracked separately). Not linked withFixessince the verification-page follow-up is still needed before that issue is
fully addressed.
Type of change
Test plan
Unit tests (
task test)Linting (
task lint-fix)pkg/authserver/storage: store/load by both codes, duplicateuser-code/device-code rejection, not-found, TTL expiry, authorize/deny
transitions and
ErrInvalidStateon a repeat transition, last-polled-atupdate, delete removing both indexes, concurrent same-user-code store.
pkg/authserver/server/deviceflow: pending →authorization_pending,denied →
access_denied, unknown/wrong-client device_code →invalid_grant, expired →expired_token, polling faster than theconfigured interval →
slow_down, authorized → access + refresh tokensissued with the stored scopes/audience, and a second redemption of the
same device_code fails (single-use).
pkg/authserver/integration_test.go:TestIntegration_DeviceFlow_FullHappyPath(device_authorization → simulate operator authorization via storage →
poll token endpoint → success, then re-poll →
invalid_grant) andTestIntegration_DeviceAuthorizationEndpoint_Disabled(route not mountedwhen the feature flag is off).
Verified independently (not just by the implementing session): full-repo
go build ./...clean;go test -race ./pkg/authserver/... ./pkg/oauthproto/...green across every package; the two new integration tests re-run explicitly
with
-count=1(cache bypassed) both pass. A full-repotask testfailsonly on a pre-existing, already-broken, untracked file
(
pkg/authserver/integration_threeupstreams_repro_test.go, someone'sin-progress work never committed) — confirmed independently broken with or
without this PR's changes present, and not part of this PR's diff.
Changes
pkg/authserver/storage/types.go,memory.go,redis.go,redis_keys.goDeviceCodeStorageinterface + both backend implementationspkg/authserver/server/deviceflow/*.gopkg/authserver/server/handlers/device_authorization.goPOST /oauth/device_authorizationpkg/authserver/server/handlers/handler.gopkg/authserver/server/handlers/discovery.gopkg/authserver/server/provider.go,server_impl.goDeviceFlowEnabled/DeviceCodeIntervalplumbing, factory registrationpkg/authserver/config.go,runner/embeddedauthserver.goRunConfig.DeviceFlowEnabledpkg/oauthproto/constants.go,discovery.gopkg/authserver/storage/mocks/mock_storage.goDoes this introduce a user-facing change?
Yes, but dormant by default: operators can opt in to RFC 8628 device-flow
support for the embedded auth server via
DeviceFlowEnabled(off bydefault). Until the follow-up verification-page PR lands, an authorized
device request can only be produced by calling storage directly (as the
integration test does) — there is no way for a real end user to complete
the human-verification step yet, so this flag has no usable effect for real
traffic until that follow-up ships.
Implementation plan
Approved implementation plan
This PR was planned as a 5-step sequence, landed here as ONE PR per an
explicit decision to not split upstream work across multiple PRs (all steps
below are in this PR except step 4):
DeviceCodeStorageinterface plus memory and Redisimplementations and unit tests.
POST /oauth/device_authorizationhandler, rate-limited like
/oauth/register, plus discovery metadata.fosite.TokenEndpointHandlerforgrant_type=urn:ietf:params:oauth:grant-type:device_code.GET /oauth/device,reusing the existing
authorize.go/callback.goupstream-loginmachinery; binds the resolved identity to the matching device-code row.
DeviceFlowEnabledflag (in this PR);an architecture-doc addition is still pending. CRD/operator exposure is
an explicit sibling follow-up, matching the
IdentityFromTokenConfigprecedent.
Special notes for reviewers
(well over the usual 400-line/10-file guideline) by explicit decision —
splitting a working end-to-end grant across several upstream PRs would
leave intermediate PRs shipping dead code with no caller, which is worse
than a larger, but fully coherent, single review.
fosite.ErrTokenExpiredwas deliberately NOT reused for theexpired_tokencase: itsErrorFieldis"invalid_token"(RFC 6750§3.1's bearer-token vocabulary), not RFC 8628 §3.5's
"expired_token".A local
ErrExpiredTokensentinel is defined indeviceflow/errors.goinstead, to emit the wire-correct error code.
fosite.TokenEndpointHandlerin this repo's pinned fosite (v0.49.0) hasno separate
CanHandleRequestmethod — onlyCanHandleTokenEndpointRequest,HandleTokenEndpointRequest,CanSkipClientAuth, andPopulateTokenEndpointResponse.HandleTokenEndpointRequest,not
PopulateTokenEndpointResponse— confirmed fosite calls the formerexactly once per token request before the latter, so this is the correct
single-use enforcement point.
refresh_tokengranttype (no
offline_access-scope gating yet) — a reasonable futurerefinement, not required for this PR's scope.
integration_threeupstreams_repro_test.gomentioned aboveis not part of this diff (never
git add-ed) — flagging it only so CIfailures on
mainaren't confused with this PR if that file is evercommitted elsewhere.
review and are tracked as required follow-up work before
DeviceFlowEnabledcan be turned on for real traffic; everything else raised in that review
(Redis transaction atomicity, client authentication on
/oauth/device_authorization, theDeviceCodeStorageinterfacesegregation, and a
generateUserCodemodulo bias) has been fixed in thisPR.
Generated with Claude Code