Publish TransferableKeyMaterial's single-read consumption surface (Consume/KeyMaterialReader/Discard) — closes #28

[moisesja]

Closes #28.

What

Publishes TransferableKeyMaterial's single-read store-side acceptance surface and hardens the
arbitrary-callback boundary it creates:

• KeyMaterialReader<T>, Consume<T>(KeyMaterialReader<T>), and Discard() are public, so an
  ICapableKeyStore implemented outside this assembly (HSM/KMS/Vault) can accept routed imports.
  ReadCount remains internal.
• Consume invokes the external reader outside the material lock, avoiding application/store
  lock-order deadlocks while preserving exactly-one read and zeroization in finally.
• A concurrent Dispose/Discard latches the instance unreadable before returning. Only the
  physical wipe defers until an already-running reader releases the borrowed spans.
• Post-disposal documentation now accurately names KeyType, PublicKey, and Consume as the
  throwing data-access members; IsConsumed stays readable and cleanup remains idempotent.

Re-review fixes

• The external acceptance store now serializes replay lookup, alias validation, material
  acceptance, custody commit, and receipt commit under one transaction gate. Concurrent exact
  retries produce one original mutation, one private read, one stable receipt, and replay every
  other call without corrupting either dictionary.
• The replay regression has a test-only miss probe that makes removal of the gate fail
  deterministically. Workers are background threads and share one 15-second deadline.
• NFR-3 and the input-validation sweep now define the implementable callback execution boundary:
  any exception escaping caller-delegate execution—including dependencies invoked by that
  delegate—propagates unchanged; NetCrypto/dependency failures outside the callback still require
  normalization.
• The hostile-reader matrix asserts exact exception identity/type or exact nonthrowing behavior,
  including a parser-created FormatException, while requiring spend and wipe on every callback
  exit.
• The public-surface guard preserves overload multiplicity and pins the sole non-static generic
  Consume<T> plus the exact KeyMaterialReader<T>.Invoke signature.
• The test store validates and defensively copies its wrapping key, so caller mutation cannot
  corrupt the custody proof.
• The 1.7.0 changelog compare links, replay/conflict semantics, background-thread test harness,
  and parked-reader disposal regression from the prior review round remain fixed.

Tests and proof

• tests/NetCrypto.ExternalStore.Tests is deliberately outside InternalsVisibleTo; compilation
  and execution prove that a third-party store can implement routed import using only public API.
• Five material-state concurrency/deadlock regressions plus one atomic concurrent-replay
  regression use bounded waits.
• Eight hostile-reader cases cover direct and dependency-originated callback exceptions,
  re-entry, cleanup calls, copying, and wipe/spend postconditions.
• Genuine-regression checks:
  • removing the store gate makes the exact-replay test fail at the concurrent commit invariant
    with dictionary corruption; restoring it passes;
  • removing the wrapping-key clone makes the defensive-copy test fail at byte 0; restoring it
    passes;
  • the earlier state-machine reverts fail the parked-reader latch and lock-order tests.

Adversarial gate

The final scratch harness completed 6,735 checks with no remaining blocker:

• 1,200 coordinated exact concurrent imports: one original, all other replays, one reader entry,
  one receipt/custody record, one stable instance ID, correct unwrap, zero failures;
• same-ID conflicts, fresh-ID alias collisions, mixed races, reader failure/healthy retry, and 200
  real import-versus-dispose races;
• 1,000 application/store lock-order probes with no deadlock and correct zeroization;
• same-instance propagation of direct and parser-created forbidden exception types across the
  callback boundary, with NetCrypto-owned null validation unchanged;
• mutable-buffer, malformed-key, identity-consistency, replay-material, and public-surface attacks.

Validation at 816bcca

• Warning-as-error solution build: 0 warnings, 0 errors.
• Native-present CI-filtered local suite: 1,231 NetCrypto.Tests + 21
  NetCrypto.ExternalStore.Tests = 1,252 passed, 0 failed.
• API coverage check: passed.
• A raw unfiltered local run selects five BbsAbsent tests while this machine has the native
  library; the repository's authoritative Category!=BbsAbsent command above is green.
• GitHub Actions: all four jobs passed—the macOS, Ubuntu, and Windows native/sample matrix
  plus the supported no-native leg.

SemVer: additive public API requires a minor release; version remains 1.7.0.

[moisesja]

Review — LGTM ✅

Reviewed the diff as a security-relevant change (it publishes a private-key-material read path). The change is minimal (34/-7 across 4 files), purely accessibility modifiers on three members, with semantics unchanged. The safety argument in the PR body holds up under adversarial review:

No new extraction capability. A TransferableKeyMaterial is only minted by the public FromKeyPair/FromRawKey, i.e. by a party that already holds the raw secret, and it flows to exactly one store's import path. Anyone able to call Consume either already had the secret (to create the instance) or is the store it was handed to. Since KeyPair.PrivateKey is already public, publishing Consume/KeyMaterialReader/Discard adds no export surface that wasn't already reachable.

Guarantees are caller-independent. The once-only latch, zeroize-in-finally, and the IsConsumed signal are all enforced inside Consume under _gate, regardless of caller trust (TransferableKeyMaterial.cs:241). The delegate hands out a ReadOnlySpan<byte> — non-escapable by type — and ReadCount increments before the read so a throwing reader still counts as consumed and the material is wiped. Correct: exposed material is spent.

Thread-safety survives the wider exposure. _gate serializes Consume/Dispose/Discard; _consumed is checked and latched under the same lock, so there's no TOCTOU now that external code can race Consume against Dispose.

Packaging/hygiene checks:

• PublicAPI.Unshipped.txt is empty (header only) — baseline correctly promoted to Shipped, sorted; RS0016/RS0017 will stay clean.
• ReadCount correctly kept internal (test-only observability), so no over-exposure.
• SemVer bump to 1.7.0 and CHANGELOG entry are consistent with adding public API (minor).

Optional nit (non-blocking): these members are net-new public API surface, so per Keep a Changelog the entry arguably fits better under ### Added than ### Changed. Purely cosmetic — no need to hold the PR for it.

Nothing blocking here. CI (build-test on the three OSes + no-native) was still in progress at review time; worth a glance before merge, but the described local validation (0 warnings, 1221 passed) matches expectations for an accessibility-only change.

---

Generated by Claude Code

[moisesja]

Withdrawn: this change must go through the project's approval channels. The gap report and justification remain in #28.

[moisesja]

Restored at the owner's direction — branch re-pushed at the same head (514b8a8). Awaiting review/approval through the project's normal process; the platform keeps NetCrypto pinned at 1.6.0 (import path dormant) until a release ships.

[moisesja]

I would not merge this yet. CI is green and the production diff is localized, but the security-sensitive state machine and several claimed proofs are not correct.

I read the title/body, all three commits, the full 19-file diff, and issue #28; ran the 15 external-store tests plus 49 targeted TransferableKeyMaterial tests; and ran an external scratch exploit. Four blockers remain:

1. Dispose / Discard returns before the object is disposed. In src/NetCrypto/TransferableKeyMaterial.cs:234-247, _reading == true causes an immediate return without setting _consumed or wiping. Deterministic repro: park Consume in its callback, call Dispose, then inspect the object before releasing the reader. Actual result: IsConsumed == false; KeyType and PublicKey still succeed; the pinned secret remains live; and another Consume throws InvalidOperationException instead of the documented post-disposal ObjectDisposedException. This directly contradicts the method docs and the comment claiming the promised state already holds when the effect can be observed. tests/NetCrypto.ExternalStore.Tests/ConsumeConcurrencyTests.cs:62-90 waits until the reader finishes before checking IsConsumed, so it misses the bug. Latch terminal state before returning and defer only the physical wipe; add a parked-reader assertion before releasing the reader. Discard() inherits the same defect.

2. The deadlock regression can itself hang CI. tests/NetCrypto.ExternalStore.Tests/ConsumeConcurrencyTests.cs:32-57 creates foreground Threads. If the old ABBA bug returns, the bounded join assertion fails, but the two deadlocked foreground threads remain alive and prevent the testhost from exiting. That contradicts the class claim that regressions fail rather than hang. Use background threads or a harness that guarantees stuck workers cannot keep the process alive, and capture worker assertions/exceptions instead of throwing on raw threads.

3. The 1.7.0 release metadata is incomplete. This PR bumps NetCryptoVersion, adds a dated [1.7.0] heading, and promotes the surface into PublicAPI.Shipped.txt, but CHANGELOG.md:475-484 still defines [Unreleased] as v1.6.0...HEAD and has no [1.7.0] compare link. Add [1.7.0]: ...v1.6.0...v1.7.0 and move [Unreleased] to v1.7.0...HEAD, per netcrypto-prd.md:866.

4. The external-store “replay” test is not a replay. tests/NetCrypto.ExternalStore.Tests/ExternalStoreImportTests.cs:72,76 uses different KeyOperationIds. WrappingExternalKeyStore.cs:53-59 ignores operation identity/fingerprints and labels any duplicate alias as replay. That contradicts FR-7b’s (NamespaceId, KeyMutationKind, KeyOperationId) idempotency contract and teaches implementors the wrong behavior. Reuse the same operation ID and implement enough ledger/fingerprint behavior to distinguish replay from conflict, or remove the replay claim and keep the direct external Discard test as #28’s proof.

Two additional false-confidence claims should be cleaned up:

• tests/NetCrypto.Tests/KeyStore/TransferableKeyMaterialTests.cs:23-35 is still named ExposesNoReadPathForPrivateMaterial and says no caller-reachable read path exists. This PR adds exactly such a path. The reflection filter passes only because it searches names like Private/Export and misses Consume. Rename/reframe it as “no getter/format/export surface” and explicitly assert the one sanctioned Consume path.
• tests/NetCrypto.Tests/NonFunctional/InputValidationFuzzTests.cs:306-342 claims hostile readers prove only contract exceptions escape, but every throwing case deliberately throws already-allowed InvalidOperationException. A callback throwing FormatException propagates raw FormatException (I reproduced it) while wiping correctly. Propagation is reasonable for arbitrary caller code; if that is the design, document it and remove the NFR-3 exception-normalization claim instead of pretending the test proves it.

What is fine: additive public API justifies 1.7.0; the three commit messages are clear; and the 1,118-line size is mostly justified tests/docs. The blockers above—not the nominal scope—are why this should not merge.

[moisesja]

All four blockers and both cleanups are addressed in 47593da. Every claim reproduced before fixing — none argued down.

1. Dispose/Discard during a live read — confirmed with your exact probe (parked reader → IsConsumed == false, metadata readable, Consume → InvalidOperationException). Fixed as you suggested: the terminal latch (_consumed = true) is taken immediately in the defer branch; only the physical wipe defers to the in-flight read's finally (wiping mid-read stays forbidden — that was the all-zero-key path). New regression DisposeDuringALiveRead_LatchesImmediately_AndDefersOnlyTheWipe asserts the full disposed state while the reader is still parked (IsConsumed == true; KeyType/PublicKey/Consume all ObjectDisposedException), then that the released read still observed the true bytes and the buffer wiped. Proven genuine by revert: fails at Expected material.IsConsumed to be True … but found False. XML docs and the CHANGELOG entry now say latch-now/wipe-later.

2. Deadlock regressions could hang CI — correct; the foreground threads would have kept the testhost alive after the failed bounded join. All workers in ConsumeConcurrencyTests are now IsBackground = true, and worker exceptions are captured into a queue and re-asserted on the test thread instead of thrown raw.

3. Release metadata — the compare-link fix landed in 5fbc785 ([Unreleased]: v1.7.0...HEAD + [1.7.0] link); this commit also aligns the Fixed entry's wording with the corrected dispose semantics.

4. The "replay" that wasn't — agreed, it taught the wrong contract. WrappingExternalKeyStore now keeps a mutation ledger keyed by KeyOperationId with a value-tuple fingerprint over (alias, keyType, publicKeyHex) — never the private half, since a replay must be decided without reading the secret: same id + same fingerprint → Replayed: true, Discard() unread, original receipt; same id + different fingerprint → KeyStoreException(IdempotencyConflict) with the material left usable; duplicate alias under a fresh id → refused before the read. The replay test now reuses the same operation id, and conflict + duplicate-alias cases are added.

False-confidence cleanups — both taken:

• ExposesNoReadPathForPrivateMaterial → TheOnlyReadPathIsConsume_AndThereIsNoGetterFormatOrExportSurface: keeps the no-getter/format/export name screen and adds a closed-list equivalence over the entire public surface, so Consume/Discard are asserted as the one sanctioned pair and any new read-shaped member fails the test.
• The hostile-reader sweep now pins the actual exception contract: NetCrypto's own refusals are the documented types, while reader-thrown exceptions — including your FormatException probe — propagate same-instance, unmodified (BeSameAs), with spend+wipe asserted on every path. That propagation rule is by design (the reader is the store's own code; NFR-3's normalization duty covers NetCrypto's validation of caller bytes, not the caller's delegate) and is now stated in Consume's XML remarks.

Validation at 47593da: -warnaserror clean; dotnet test --filter "Category!=BbsAbsent" → 1230 + 18 = 1248 passed, 0 failed; all 11 samples exit 0; ApiCoverageCheck green.

🤖 Generated with Claude Code

[moisesja]

Re-review at 48c1cb952b92597b39f3681fcb3eed6294b938b5: the four original blockers are fixed, but I still would not merge this head.

What I re-verified:

• The parked-reader Dispose/Discard race is fixed. Before releasing the reader, IsConsumed is now true, KeyType/PublicKey/Consume throw ObjectDisposedException, the live reader still sees the original bytes, and the buffers wipe on exit.
• The concurrency regressions now use background workers, bounded joins, and exception capture, so a reintroduced deadlock fails without pinning the testhost.
• The 1.7.0 changelog links are correct.
• Sequential replay/conflict/duplicate-alias behavior now uses the operation ID correctly.
• All four GitHub Actions jobs pass. Locally: warning-as-error solution build passes; tests pass 1230 + 18; the independent adversarial pass ran 1,000 eight-thread Consume/Dispose stampedes without a production state-machine failure.

Two blockers remain:

1. The new replay proof is not atomic or thread-safe. tests/NetCrypto.ExternalStore.Tests/WrappingExternalKeyStore.cs:31-39,63-86,112-126 performs _ledger.TryGetValue, _custody.ContainsKey, Consume, and writes to two ordinary Dictionary instances without a lock, despite calling itself an honest ICapableKeyStore and claiming custody + receipt + fingerprint are committed “together.” I independently ran 16 simultaneous exact imports with the same store, alias, operation ID, key type, public key, and private key. Expected: 1 original, 15 replays, 1 private read, 0 failures. Actual on the current head:

   BROKEN round=0 originals=2 replays=0 readerEntries=8 failures=14
   14 x InvalidOperationException: Operations that change non-concurrent collections must have exclusive access...
   

   This is the exact retry race idempotency exists to handle, and it disproves the new test double’s atomicity claim. Serialize the ledger lookup, alias check, acceptance, custody write, and receipt write under one store gate, then add a concurrent same-ID exact-replay regression.

2. The callback-exception contract contradicts the source of truth. TransferableKeyMaterial.cs:296-300 and the rewritten hostile-reader test now intentionally propagate a reader-thrown FormatException unchanged. That behavior is reasonable for arbitrary caller callback code, but netcrypto-prd.md:771-780 still says forbidden leaked exceptions “from any public method on any input” include System.FormatException and that all must become parameter-named ArgumentException/ArgumentNullException. The follow-up comment explains the intended carveout; the PRD does not. AGENTS.md makes the PRD the functionality source of truth. Add an explicit callback-originated-exception carveout to NFR-3 and keep the input-validation sweep procedure consistent with it.

Required proof/docs cleanup:

• TransferableKeyMaterialTests.cs:43-55 claims a closed public-surface proof, but it projects members to names and calls Distinct(). A second Consume overload collapses to the existing name and passes. Assert exact signatures or at least exactly one declared public Consume method.
• TransferableKeyMaterial.cs:235-237 (and the type-level wording) says every member except IsConsumed throws after disposal, but Dispose and Discard are intentionally idempotent/nonthrowing. Name the data-access members instead.
• Update the stale PR body: it still claims 15 external tests, four concurrency regressions, six hostile cases, and “only contract exceptions escape”; current evidence is 18 tests, five concurrency tests, seven hostile rows, and unmodified reader-exception propagation.

The production TransferableKeyMaterial state machine now looks sound. The remaining rejection is about the correctness of the new contract proof and keeping the normative contract honest—not a repeat of the fixed disposal bug.

[moisesja]

Re-review at 1074eb5: all previously reported blockers are resolved. The replay transaction is serialized and deterministically regression-tested; NFR-3 now matches the callback execution boundary; the public-surface and hostile-reader proofs are exact; disposal documentation is accurate; and the wrapping-key fixture defensively copies caller input. The independent adversarial pass completed 6,735 checks with no remaining blocker. Local validation passed 1,231 core + 21 external-store tests, and all four GitHub Actions jobs are green. I found no remaining issue that should block merge.