Our OAuth 2.0 authorization-code + PKCE flow (in `auth/oauth/`) wraps Ory fosite with custom login/consent managers and a Postgres-backed storage layer. The wiring is functional but has multiple production-readiness gaps relative to Hydra. This issue tracks the requirements to close those gaps. The most acute problems: - ~~PKCE is not enforced (`EnforcePKCE: false`, no per-client `force_pkce`, no `EnforcePKCEForPublicClients`).~~ - incorrect. `doorkeeper` does not enforce PKCE. 🚫 - Code reuse does **not** invalidate the grant chain — `InvalidateAuthorizeCodeSession` flips `active=false`; the access/refresh tokens issued from the code stay live. - No transaction boundary around code consumption + token issuance. - Tokens, codes, and PKCE challenges are stored in plaintext by signature (Hydra hashes signatures with SHA-384). - Sensitive logging via the standard `log.Printf` from inside the token endpoint and consent/login handlers. ## 1. PKCE handler (RFC 7636, RFC 9700 §2.1.1) - [ ] ~~Reject `plain` challenge method by default. Mirror Hydra's `EnablePKCEPlainChallengeMethod` config; default `S256` only.~~ - incorrect. `doorkeeper` does enable `plain`. 🚫 - [ ] ~~Reject unknown `code_challenge_method` values at `/authorize`. Currently silently accepted via the same `fallthrough`.~~ - incorrect. Unkown methods are rejected. - [ ] ~~Add `EnforcePKCE` and `EnforcePKCEForPublicClients` config; default both `true`~~ - incorrect. `doorkeeper` does not enforce PKCE. 🚫 - [ ] ~~Add a per-client `force_pkce` setting. Add the column to `clients.Client` and surface through the `fosite.Client` adapter.~~ - incorrect. `doorkeeper` doesn't support this 🚫 - [ ] Proper error messages on failure paths. - [ ] ~~Gate the legacy behavior per-client. Add a `client.allow_short_pkce` flag (or equivalent allowlist) so the relaxed 20-char rule applies only to the specific legacy clients that need it.~~ - incorrect. `doorkeeper does not gate on PKCE length 🚫 - [ ] Use `crypto/subtle.ConstantTimeCompare` for the verifier/challenge match. ## 2. Authorization code lifecycle (RFC 6749 §4.1, RFC 9700) - [ ] On code reuse, revoke every access and refresh token issued from the code. On `ErrInvalidatedAuthorizeCode`, also call `accesstokens.RevokeByRequestID` and `refreshtokens.RevokeByRequestID`. - [ ] Wrap code consumption + token issuance in a single DB transaction. Without it, a crash between invalidation and token persistence leaves the system in an inconsistent state. - [ ] Store hashed values for signatures. ## 3. Authorize endpoint - [ ] Validate `redirect_uri` and `client_id` before any error redirect. - [ ] Honor `prompt=consent` even for trusted clients. OIDC requires re-prompting when `prompt=consent`. - [ ] Validate `response_type` against the client's registered list. - [ ] Decide on wildcard scopes. `WildcardScopeStrategy` lets a client registered with `*` request anything. Switch to `ExactScopeStrategy`. - [ ] Drive `AllowInsecureHTTP` from environment. Default `false` in production; allow the RFC 8252 §7.3 loopback exception. - [ ] Drop empty form fields on rebuilt authorize URLs. Don't re-emit `code_challenge_method=""` when not set. - [ ] ~~Warn (or require) when `state` is missing for public clients. RFC 9700 §2.1 defense-in-depth.~~ - incorrect, `doorkeeper` does not require `state` for public clients. 🚫 ## 4. Token endpoint and client authentication - [ ] ~~Persist `token_endpoint_auth_method` per client. Support `client_secret_basic`, `client_secret_post`, `none`, `private_key_jwt`, `client_secret_jwt`. `clients.Client` has no such field today.~~ - incorrect, `doorkeeper` has no such fields. 🚫 - [ ] ~~Reject `client_secret_basic` over plain HTTP when `AllowInsecureHTTP` is false.~~ - incorrect, not supported by `doorkeeper` 🚫 - [ ] Implement atomic refresh token rotation - `Storage.RotateRefreshToken` is currently two non-transactional DB calls (access then refresh). Wrap the two revokes in a transaction. - [ ] ~~Implement refresh-token rotation with reuse detection. On rotation revoke the prior refresh token plus its access token; on reuse of an already-rotated refresh token, revoke the entire grant chain (RFC 9700 §4.13.2).~~ - incorrect. `doorkeeper` doesn't support reuse detection. 🚫 - [ ] ~~Enforce per-client `grant_types`. A client without `"refresh_token"` in its grant list must not receive one.~~ - incorrect, `doorkeeper` doesn't support this 🚫 - [ ] Add end to end tests with access and refresh tokens. ## 5. Audience Handling <details> <summary>⚠️<strong>Out of scope, not PKCE-related. Moved to <a href="https://gitlab.com/gitlab-org/gitlab/-/work_items/604551">#604551</a> </strong></summary> - [ ] ~~Add Audience []string to clients.Client. Wire through fosite.DefaultClient.Audience so GetAudience() returns it.~~ - [ ] ~~Intersect `ar.GetRequestedAudience()` with `ar.GetClient().GetAudience()` before granting; reject the request if any requested audience is outside the allowlist (or just don't grant it).~~ - [ ] ~~Validate every consentDecision.GrantedAudience entry against both ar.GetRequestedAudience() and ar.GetClient().GetAudience(). A consent decision must never expand the audience set beyond what the client requested or what the client is allowed to request.~~ - [ ] ~~Decide whether you want DefaultAudienceMatchingStrategy (URI-prefix matching) or ExactAudienceMatchingStrategy (string equality). Hydra has both. Exact matching is safer if your audience values are client IDs / opaque strings; URI-prefix is appropriate only if audiences are namespaced URLs.~~ - [ ] ~~Persist granted_audiences.~~ - [ ] ~~Add the aud claim to the access token / ID token from `granted_audiences`. Confirm Session.AccessToken / Session.IDToken carry it.~~ </details> ## 6. Logging, errors, and observability - [ ] Replace `log.Printf` with structured `logrus.WithField`. Free-form `%v` of attacker-controlled error text enables log injection (CWE-117). - [ ] Never log `code_verifier`, `code_challenge`, `code`, or `client_secret`. - [ ] Strip `error_debug` from client responses. Don't expose server internals; keep `WithDebug` strings server-side only. - [ ] Use OAuth-spec error codes consistently. RFC 6749 §5.2 wants `{"error", "error_description"}`. - [ ] Add OpenTelemetry traces and metrics around `/oauth2/authorize` and `/oauth2/token`. - [ ] Propagate `request_id` end-to-end (login → consent → authorize → token) for the audit trail. ## 7. General security hardening - [ ] Bound request-body size and parse timeouts. - [ ] Per-endpoint context timeouts for federation, IDP, and database calls. - [ ] Strict-Transport-Security header and `Host` allowlist. - [ ] Hash client secrets before insert. ## 8. Routable tokens and `giat_` prefix (Cells architecture) <details> <summary>⚠️<strong>Out of scope, not PKCE-related. Moved to <a href="https://gitlab.com/gitlab-org/gitlab/-/work_items/604548">#604548</a> </strong></summary> All tokens generated by the authorization-code flow — authorization codes, access tokens, and refresh tokens — must be routable by the Cells HTTP router and carry a consistent, recognizable prefix. ### 8.1 `giat_` prefix on all generated tokens - [ ] ~~**Authorization codes** — prefix raw value with `giat_ac_` before encoding/signing. The prefix must survive the HMAC-signature step so it is visible in the opaque token string returned to the client.~~ - [ ] ~~**Access tokens** — prefix raw value with `giat_at_`. Ensures Workhorse and the HTTP router can identify IAM-issued access tokens by inspection without decoding the JWT/opaque body.~~ - [ ] ~~**Refresh tokens** — prefix raw value with `giat_rt_`. Allows the router to distinguish refresh tokens from access tokens and route accordingly.~~ - [ ] ~~Implement prefix injection in the fosite token-generation hook (override `fosite.CoreStrategy` or wrap `enigma.HMACStrategy.GenerateHMACKey`). Do **not** patch individual call sites.~~ - [ ] ~~Strip the prefix before HMAC verification so existing signature logic is unaffected.~~ - [ ] ~~Update token-length validation: any minimum-length checks must account for the added prefix bytes.~~ - [ ] ~~Never log the full prefixed token value — log only the prefix + first 6 chars for correlation.~~ ### 8.2 Cell-ID embedding for routability - [ ] ~~Embed the originating **cell ID** into each token so the HTTP router can route it.~~ - ~~For **opaque tokens** (authorization codes, refresh tokens): encode `<prefix><cell_id_b64>.<hmac_payload>` — the cell ID occupies a fixed-width field between the prefix and the HMAC payload.~~ - ~~For **JWT access tokens**: add a `cell_id` claim to the JWT body. The router reads this claim after base64-decoding the payload (no signature verification needed for routing).~~ - [ ] ~~The cell ID must be sourced from the Topology Service `Classify` endpoint.~~ ### 8.3 Router integration contract - [ ] ~~Document (in a code comment on the strategy type) the exact byte layout of each token type so Workhorse / the HTTP router can parse it without importing IAM packages:~~ - ~~`giat_ac_<cell_id_b64url>.<hmac_signature_b64url>`~~ - ~~`giat_at_<jwt_header>.<jwt_payload>.<jwt_sig>` (JWT; cell_id in payload)~~ - ~~`giat_rt_<cell_id_b64url>.<hmac_signature_b64url>`~~ </details>