fix(auth): accept pre-hashed client secrets on the internal registration path
Problem
gitlab-rails replicates OAuth applications into IAM via DrainWorker → InternalOAuthClientsService.CreateClient. Rails can only send the SHA-512 digest of a client secret (128-char lowercase hex), so plaintexts are unrecoverable. The old proto contract declared client_secret as plaintext and Registry.CreateClient re-hashed it, storing sha512(digest). The real client's plaintext could never authenticate, and the digest itself became a working IAM credential for anyone able to read oauth_applications.secret — all failing silently.
Changes
- proto/auth/auth.proto: renamed field 2 from
client_secrettohashed_client_secreton both the CreateClient and UpsertClient requests, with validation pattern^[0-9a-f]{128}$. Field 13 isbool dynamicfrom !602 (merged) (merged; rebased on top). - Registry: CreateClient and UpsertClient take a
Registrationinput (request object separated from the domain object, per review) carrying the digest; they validate its shape instead of re-hashing, and the registry no longer takes a hasher.Clientstays a pure domain object —Secretalways means the stored hash bytes. - Validation: digest required for every client, public included — mirrors gitlab-rails, which sends a secret for every application, and matches the contract UpsertClient shipped with on main.
- HTTP endpoints removed (per review, instead of migrating them): the deprecated
/oauth2/internal/clientsroutes, their handlers and JSON types, and the private-mux wiring that existed only for them are deleted; the setup guides register clients over gRPC, with digest payloads and a shasum recipe. - Tests: validation table, digest round-trip, and rejection cases through the registry and gRPC handler; unit/l2-integration/l1-integration all pass locally.
Prior-art
fosite v0.49.0 has no registration flow; auth-time compare (checkClientSecret) untouched. Hydra never accepts pre-hashed secrets — deliberate deviation forced by Doorkeeper's hash-at-rest. Wire format matches Gitlab::DoorkeeperSecretStoring::Sha512Hash; internal replication API, not RFC 7591 (DCR is !560 (merged)).
Rollout
- The rename fails buf breaking with two violations (field name, json_name);
breaking_protoisallow_failure: true, documented there as temporary, so it reports yellow rather than blocking. The loud break is on the Rails side: an unknownclient_secretkeyword once gitlab-iam-grpc regenerates (can ride !473 (merged)'s regen). - The consumer-side follow-ups (Rails gem regen + DrainWorker kwarg, pre-fix row re-drain/backfill, the sandbox-config provisioning script, !560 (merged) hashing its own secrets) are tracked in gitlab-org/gitlab#628063 (closed).
Open questions / decisions to confirm
- Reuse field 2 vs. reserve it and move the digest to 14: reused. Reserving would have left 15 as the only remaining single-byte field number (protobuf encodes 1-15 in one tag byte), spent on a hazard that cannot occur since no caller ever holds a plaintext. Raised by @skundapur in review.
- Secret requiredness for public clients: originally this MR allowed empty; after rebasing onto UpsertClient it adopts main's stricter rule — required even for public clients, since Rails always sends one.
- Validation pins lowercase hex only vs. normalized: lowercase only (rejects uppercase, matching Doorkeeper output).
- Auto-detecting whether an incoming secret is hashed (store as-is) or plaintext (hash it): rejected. A plaintext that happens to be 128 hex characters would be mistaken for a digest and stored unhashed, and the value carries no marker saying which it is, so the field name
hashed_client_secretdeclares it explicitly instead. - Keeping the name
client_secretand changing only its documented meaning to a SHA-512 digest: rejected. That is wire- and source-compatible, so nothing anywhere flags it and the name keeps inviting plaintext from future callers. What shipped renames the field while keeping the number: generated code breaks loudly at the call site, while the wire stays compatible with what Rails already sends.