feat: store verify-only client secrets as one-way verifiers by polewskm · Pull Request #531 · NCodeGroup/Identity

polewskm · 2026-11-03T01:59:57Z

A client_secret that its method only compares is stored as a one-way verifier, so even the holder of the root key can't read it back (ADR-1391).

Stored forms. A verifier is a symmetric secret with the verifier encoding inside the nce1. envelope. Only a secret whose method keys from it (client_secret_jwt) is stored as its UTF-8 octets. Every secret records its Origin: generated or supplied.
Open scheme vocabulary. Each scheme is an ISecretVerifierScheme, looked up through ISecretVerifierSchemeCatalog. The family registers two:
- $ncode-sha256$… for generated secrets;
- $ncode-pbkdf2-sha256$… for supplied secrets, with the password-hash parameters (600,000 iterations; a Production host can't set fewer).
One seam owns the rule. Method descriptors declare RequiresClientSecret and UsesClientSecretAsKey, both required. IClientSecretIssuer issues every client secret in the form its method needs. Its callers are the management API, dynamic client registration, the conformance and test seeds, and the bootstrap administrator.
The stored form follows the method. Every stored client names its own token_endpoint_auth_method. When the method changes, ReconcileClientSecrets converts reversible secrets to verifiers. When no secret can serve the new method, it issues a replacement, which the response shows once.
Verification.
- A compare method authenticates only against verifiers, and a key method only against reversible secrets. Comparisons are constant-time, and cheap verifiers are tried first.
- PBKDF2 verifications share one bounded ISecretVerificationGate, configured by MaxConcurrency, MaxQueueLength and QueueTimeout. When the gate is saturated, the request gets 503 temporarily_unavailable with Retry-After, and the secret is never compared.
Request objects. A client whose method only compares its secret can't register a symmetric request_object_signing_alg.
Console. It marks each verifier, because a verifier can't sign an assertion.

Closes #163

A client_secret issued to a client registered for client_secret_basic or client_secret_post is stored only as a self-describing verifier inside its nce1. envelope: $ncode-sha256$ for a server-generated secret and the password-hash PBKDF2 format for an operator-supplied one. Only a secret issued while the client is registered for client_secret_jwt, which keys its assertion from the secret, stays reversible. Verification is constant time for both forms. Dynamic client registration issues a new secret when an update switches a client to a method its secrets cannot serve. Closes #163

…crets # Conflicts: # docs/decisions/README.md

codecov · 2026-11-03T02:04:42Z

Codecov Report

❌ Patch coverage is 99.32432% with 7 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...rets.Persistence/Verifiers/SecretVerifierFormat.cs	90.47%	4 Missing ⚠️
...ication/Clients/DefaultOpenIdConfidentialClient.cs	66.66%	1 Missing ⚠️
...esourceServers/ResourceServerApiEndpointHandler.cs	0.00%	1 Missing ⚠️
...ersistence/Verifiers/Pbkdf2SecretVerifierScheme.cs	98.18%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #531      +/-   ##
==========================================
- Coverage   92.09%   90.80%   -1.30%     
==========================================
  Files        1495     1513      +18     
  Lines       62761    63642     +881     
  Branches      306      398      +92     
==========================================
- Hits        57798    57788      -10     
- Misses       4963     5854     +891

Flag	Coverage Δ
dotnet	`90.41% <99.32%> (-1.35%)`	⬇️
frontend	`99.08% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

📢 Thoughts on this report? Let us know!

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ncode-review

🤖 NCode Review at 40e34b9: 34 new findings on the changed lines.

Not on a changed line:

🟡 F21 Nit: Verifiers are unprotected and reparsed on every client resolution src/NCode.Identity.OpenId.Authentication/Clients/DefaultOpenIdClientResolver.cs:181. Each resolution calls DeserializeVerifiers, which unprotects and decodes every verifier, even on endpoints that never check a secret. Fix: No change needed now; cache the parsed verifiers by settings version if profiling shows a cost.
🟡 F34 Nit: Shared-secret encoding choice duplicated in two handlers src/NCode.Identity.OpenId.Management/Endpoints/Clients/ClientApiEndpointHandler.cs:1672. ClientApiEndpointHandler.GenerateClientSharedSecret and DefaultClientRegistrationEndpointHandler.GenerateClientSecret both choose GenerateReversibleSharedSecret or GenerateSharedSecret by a boolean and build the same GenerateSecretRequest shape. The rule that decides the stored form now lives in two places. Fix: Move the dispatch into ISecretGenerator or a shared helper and call it from both handlers.
🟡 F36 Nit: Reversible-or-verifier choice is duplicated in the callers of ISecretGenerator src/NCode.Identity.OpenId.Management/Endpoints/Clients/ClientApiEndpointHandler.cs:218. ClientApiEndpointHandler.GenerateClientSharedSecret and DefaultClientRegistrationEndpointHandler.GenerateClientSecret each branch on usesClientSecretAsKey between GenerateReversibleSharedSecret and GenerateSharedSecret. The same flag is also threaded through BuildClientSecrets. The seed handlers presumably repeat the choice. Fix: Put the decision behind one shared helper, or give ISecretGenerator a single overload that takes the storage form.

ncode-review · 2026-11-03T02:06:59Z

_{Superseded by the review at a63a0d3.}

…llow-up) Nest the single-file test helpers back as private sealed types, as the test rules require; assert the Retry-After and Cache-Control headers of the 503 refusal; use NullChangeToken.Singleton in the verifier scheme data sources; and note in ADR-1110 and ADR-1320 the renamed shared secret methods.

…n paths A client's stored shared secrets always match its registered method: a method that compares its secret authenticates only against verifiers, and a settings change through dynamic client registration or the management API converts a reversible secret to its verifier (by its recorded origin) or issues a secret the method can use, revealed once. IClientSecretIssuer resolves a stored client's method and owns the rule; one process-wide ISecretVerificationGate bounds every verification with a work factor; verifier schemes register once.

…n paths (follow-up) Every stored client names its token_endpoint_auth_method: the management API's create and settings update write the client's effective method into its document, so a server or tenant value never moves a stored client's method. The management settings update reconciles secrets only when the method changes, and ReconcileClientSecrets rewrites nothing when the method cannot be resolved; GetTokenEndpointAuthMethodAsync resolves null instead of a fallback. ClientSecretReconciliation drops the unread HasChanges, and the ADR-1110 and ADR-1320 status lines name the current generator methods.

…crets # Conflicts: # docs/decisions/README.md

Type the management console's secret wire mocks as SecretResource and CreatedSecretResource, so they carry isVerifier and keyId as the server sends them and the type checker catches the next contract change. Pin the settings update's two outcomes in the store: 204 resolves to null, and a client settings change that issues a secret resolves to its one-time CreatedSecretResource. Show the Verifier chip in the SecretTable story.

…crets # Conflicts: # CHANGELOG.md # docs/conformance/conformance-seeding.md

polewskm · 2026-11-03T06:35:26Z

Autopilot: The NCode Review findings that aren't on a changed line:

F21 (declined): No change, as the finding itself suggests. DefaultOpenIdClientResolver already unprotects and decodes the client's keys on every resolution (DeserializeSecrets), and verifiers take the same path. Caching parsed verifiers would keep the material in memory longer for no measured gain.
F34 (fixed): Fixed with F3: the choice between GenerateReversibleSharedSecret and GenerateSharedSecretVerifier lives only in IClientSecretIssuer, which both handlers call with the client's resolved method.
F36 (fixed): Fixed with F3. No ISecretGenerator caller branches between reversible and verifier any more, BuildClientSecrets and both seeds included. IClientSecretIssuer is the one shared helper.

Generated by Claude Code

polewskm · 2026-11-03T14:27:31Z

/review

ncode-review · 2026-11-03T14:28:48Z

🤖 Review complete

Verdict: ✅ Approve — Deep review (sensitive paths): Re-review of what changed since the last review at 40e34b9: 378 hunks in 125 files and changes taken back in 50 files, by correctness and security. No further findings above nits.

2 reviewers run — 0 findings (run details)

Reviewer	Model	Findings	Top
correctness	opus-5-5	0	🟢
security	opus-5-5	0	🟢

_{Trial: findings are posted as review threads and in the check run. $2.82 on Amazon Bedrock, 39s, at a63a0d3. (how to interact)}

📋 Outcomes

Merged at a63a0d3: 21 fixed, 13 dismissed, 0 acknowledged, 0 ignored.

Finding	Reviewer	Severity	Outcome
F1	performance	🟠 Should fix	✅ fixed
F2	architecture, rule-check	🟠 Should fix	👎 dismissed
F3	hygiene, architecture, rule-check	🟠 Should fix	✅ fixed
F4	performance	🟡 Nit	👎 dismissed
F5	intent	🟡 Nit	✅ fixed
F6	api-compat, rule-check	🟡 Nit	👎 dismissed
F7	api-compat	🟡 Nit	👎 dismissed
F8	data-model	🟡 Nit	👎 dismissed
F9	conventions, rule-check	🟡 Nit	✅ fixed
F10	conventions, rule-check	🟡 Nit	✅ fixed
F11	hygiene, rule-check	🟡 Nit	✅ fixed
F12	hygiene, conventions, rule-check	🟡 Nit	✅ fixed
F13	rule-check	🟡 Nit	✅ fixed
F14	rule-check	🟡 Nit	👎 dismissed
F15	hygiene	🟡 Nit	✅ fixed
F16	rule-check	🟡 Nit	👎 dismissed
F17	rule-check	🟡 Nit	✅ fixed
F18	rule-check	🟡 Nit	✅ fixed
F19	hygiene	🟡 Nit	✅ fixed
F20	hygiene	🟡 Nit	✅ fixed
F22	hygiene	🟡 Nit	👎 dismissed
F23	rule-check	🟡 Nit	✅ fixed
F24	rule-check	🟡 Nit	✅ fixed
F25	rule-check	🟡 Nit	✅ fixed
F26	rule-check	🟡 Nit	✅ fixed
F27	rule-check	🟡 Nit	✅ fixed
F28	rule-check	🟡 Nit	👎 dismissed
F29	rule-check	🟡 Nit	✅ fixed
F30	hygiene	🟡 Nit	👎 dismissed
F31	conventions	🟡 Nit	👎 dismissed
F32	hygiene	🟡 Nit	✅ fixed
F33	security	🟡 Nit	👎 dismissed
F35	rule-check	🟡 Nit	👎 dismissed
F37	rule-check	🟡 Nit	✅ fixed

polewskm · 2026-11-03T14:29:03Z

Autopilot: Ready to merge. NCode Review approves a63a0d3 with no findings, every review thread from the first round has a reply, CI is green (including the frontend job and Commit attribution), and the branch merges cleanly with main. It closes #163.

Generated by Claude Code

…tired settings-update validation from #531's tests Re-scaffold the OpenIdDbContext InitialCreate so it creates Secrets.Origin and remove the AddSecretOrigin migration the merge brought in, so the context keeps one migration (ADR-1394). The three client settings-update tests #531 added validate through ValidateSettings, since ValidateSettingsUpdate is gone (ADR-1396).

The secret-key coherence rule from #531 reads an algorithm's key types through KeyedAlgorithm.KeyTypes, since an algorithm now serves every key type it declares. The PersistedSecret initializers this branch adds set the required Origin, and the shared-secret curve test goes, because a SharedSecretRequest carries no curve.

polewskm added 4 commits November 3, 2026 00:09

fix: address the review of the client secret verifiers

7c2ec3d

Merge remote-tracking branch 'origin/main' into claude/hash-client-se…

e0c23d0

…crets # Conflicts: # docs/decisions/README.md

test: set up the secret validation and audit the merged handler expects

40e34b9

polewskm temporarily deployed to code-review November 3, 2026 02:00 — with GitHub Actions Inactive

polewskm mentioned this pull request Nov 3, 2026

Limit failed client authentication attempts by default #533

Open

ncode-review Bot reviewed Nov 3, 2026

View reviewed changes

polewskm changed the title ~~feat!: store verify-only client secrets as one-way verifiers~~ feat: store verify-only client secrets as one-way verifiers Nov 3, 2026

polewskm added 8 commits November 3, 2026 03:03

fix: address NCode Review findings on the client secret verifiers

3d14f1a

Merge remote-tracking branch 'origin/main' into claude/hash-client-se…

6a75a78

…crets # Conflicts: # docs/decisions/README.md

Merge remote-tracking branch 'origin/main' into claude/hash-client-se…

7ac1d7b

…crets # Conflicts: # CHANGELOG.md # docs/conformance/conformance-seeding.md

docs: describe how a dynamically registered client's secret is stored

a63a0d3

polewskm marked this pull request as ready for review November 3, 2026 14:28

polewskm temporarily deployed to code-review November 3, 2026 14:29 — with GitHub Actions Inactive

polewskm added this pull request to the merge queue Nov 3, 2026

polewskm mentioned this pull request Nov 3, 2026

refactor: keep one InitialCreate per database context and remove retired settings #544

Merged

Merged via the queue into main with commit e08c693 Nov 3, 2026
18 checks passed

polewskm deleted the claude/hash-client-secrets branch November 3, 2026 14:39

This was referenced Nov 3, 2026

feat: ECDH-ES over any registered key type, X25519/X448, and opt-in ES256K #532

Merged

refactor: fail closed on stored shapes the server doesn't write and start versions at their first released value #545

Merged

Conversation

polewskm commented Nov 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Nov 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ncode-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ncode-review Bot commented Nov 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

polewskm commented Nov 3, 2026

Uh oh!

polewskm commented Nov 3, 2026

Uh oh!

ncode-review Bot commented Nov 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 Review complete

📋 Outcomes

Uh oh!

polewskm commented Nov 3, 2026

Uh oh!

Uh oh!

This branch was previously deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

polewskm commented Nov 3, 2026 •

edited

Loading

codecov Bot commented Nov 3, 2026 •

edited

Loading

ncode-review Bot commented Nov 3, 2026 •

edited

Loading

ncode-review Bot commented Nov 3, 2026 •

edited

Loading