feat(secrets): opaque (non-extractable) signing & key agreement (#45)

[moisesja]

Closes #45. Lets DIDComm crypto delegate to a keystore (HSM / cloud KMS / OS keychain / MPC / NetCrypto.IKeyStore) so private keys never leave the secure boundary — unblocking non-extractable custody for downstream consumers (e.g. net-wallet-sdk Messaging).

This is single-repo wiring on top of the already-shipped DataProofs.Jose 1.1.0 async IEcdhKey seam (dataproofs#13). Only two operations ever need the private key — a raw JWS signature and an ECDH shared-secret derivation; everything downstream (Concat-KDF, A256KW, AEAD, header assembly, signature normalization) is public-data math and is untouched.

What changed

• New optional capability DidComm.Secrets.IOpaqueKeyResolver — ResolveSignerAsync(kid) -> ISigner, ResolveKeyAgreementAsync(kid) -> IEcdhKey. An ISecretsResolver MAY also implement it; the facade then routes signing (signed envelopes, inner JWS, from_prior) and ECDH (authcrypt send, authcrypt/anoncrypt receive) through these handles. Returns null per kid to fall back to the extractable path, so a wallet may mix opaque and extractable keys.
• Internal KeyOperationResolver unifies opaque + extractable; the extractable path is byte-for-byte unchanged.
• Unpack is now async end-to-end (EnvelopeReader.UnpackAsync): PeekRecipients -> FindPresent -> ResolveKeyAgreement -> ParseAsync, always invoking ParseAsync (a decoy IEcdhKey when unheld) so the decrypt is constant-work from ParseAsync inward. Retires the SyncSecretsAdapter sync-over-async bridge; FR-CONSIST-06 check now async. Public DidCommClient API unchanged.
• NetDidKeyStoreSecretsResolver implements IOpaqueKeyResolver over IKeyStore (+ KeyStoreEcdhKey), kid == alias with an optional mapping hook — now a sufficient sole resolver for an HSM-backed agent (surfaces public-only JWKs).
• FromPriorBuilder gains additive JwsSigner overloads (opaque rotation JWT).
• DI: AddDidComm honors a separately-registered IOpaqueKeyResolver; UseSecretsResolver surfaces the capability; facade auto-detects when the resolver itself implements it.
• Deps/version: DataProofsDotnet.Jose 1.0.1 → 1.1.0; library version → 1.1.0.

Security (adversarial review — 2 passes, both fixes re-verified CLOSED)

• HIGH — fixed: keystore derive faults (KeyNotFoundException / CryptographicException) escaped UnpackAsync raw, only on the held path (the in-process decoy can never throw them) → an exception-type recipient-possession oracle + FR-API-07 contract escape. Now folded into the uniform CryptoException("JWE could not be decrypted."). Regression test covers both fault types.
• MEDIUM — fixed: a redundant GetInfoAsync round-trip on the held receive path (timing on slow stores). Interface self-determines the curve in one lookup.

Behavior change (see CHANGELOG)

Malformed iv/tag/encrypted_key lengths on decrypt now surface as CryptoException (uniform), not MalformedMessageException — the dataproofs#12 constant-work hardening consumed here. Also lands the durable constant-work recipient selection (closes #42).

Verification

• Release build clean (0 warnings/errors, warnings-as-errors).
• 668 tests pass (533 Core + 135 Interop), incl. 12 new opaque-path tests: facade round-trips through a non-extractable InMemoryKeyStore (authcrypt / anoncrypt / signed / sign-then-encrypt / anoncrypt(authcrypt)), cross-custody interop (opaque ⇄ extractable), ECDH KAT-equivalence, opaque from_prior, and a no-d-leak assertion.

Acceptance criteria (#45)

• A wallet whose keys live only in a non-extractable IKeyStore can authcrypt / anoncrypt / sign on send and unpack on receive with no private bytes leaving the store.
• The extractable ISecretsResolver path is unchanged (back-compat).
• NetDidKeyStoreSecretsResolver is a sufficient sole resolver for HSM-backed stores.
• Interop round-trip tests pass driving the opaque path.

🤖 Generated with Claude Code

[moisesja]

Let me find the recently opened PR.

PR #46 — a security-sensitive change (opaque keystore crypto, constant-work decryption). Let me review the diff and files.

The diff is large. Let me get the file list and read the diff in chunks.

Let me extract the diffs for the security-critical core files.

Strong security awareness so far. Let me review the facade, the NetDid resolver, writer, and DI wiring.

This is a high-quality, security-conscious PR. Let me check a few things before forming my review: CI status, whether the architecture doc was kept in sync (per CLAUDE.md), and the package version bump.

CI is green on both Ubuntu and Windows. Let me verify the architecture-doc sync rule from CLAUDE.md and the test additions quickly.

I have a complete picture now. No codebase-architecture.md exists in the repo (only the PRD, which was updated), so that CLAUDE.md sync rule is moot. CI is green on both OSes, tests are comprehensive, and the security reasoning is strong. Let me post the review.

Approval isn't permitted for this integration (likely can't self-approve). Let me post it as a review comment instead.

Review submission is blocked for this integration. Let me post it as a regular PR comment instead.

I'm unable to post to GitHub — every write path (review submit, review comment, and plain PR comment) returns 403 Resource not accessible by integration, so this session's token lacks comment/write scope on moisesja/didcomm-dotnet. I'll deliver the review here instead.

---

Review — PR #46: opaque (non-extractable) signing & key agreement (#45)

I reviewed the full diff (core, NetDid adapter, DI, tests, docs). This is a strong, well-scoped PR — LGTM. CI is green on both ubuntu-latest and windows-latest, the seam is genuinely additive, the extractable path is preserved, and the public DidCommClient surface is unchanged. Security reasoning is the right shape and documented at the call sites: decoy IEcdhKey to equalize held-vs-unheld work, uniform CryptoException folding of opaque-handle faults, scalar zeroing via CryptographicOperations.ZeroMemory, single-lookup curve discovery.

Non-blocking points worth a reply rather than a change:

1. Residual recipient-possession timing oracle on the held path. ResolveRecipientKeyOrDecoyAsync is honest that constant-work only holds from ParseAsync inward: the held path runs FindPresentAsync + ResolveKeyAgreementAsync (→ a real GetInfoAsync backing-store round-trip) before ParseAsync, while the unheld path goes straight to the in-process decoy. Against a real HSM/KMS with variable latency that's a measurable held-vs-unheld signal. Mitigation is punted to the consumer + receive rejection floor (#35) — defensible, but it lives only in an internal XML comment. Is it surfaced where a downstream wallet author will see it (the public IOpaqueKeyResolver doc, or FR-SEC-06 in the PRD)?

2. Decoy is hard-coded X25519. Correctness of the unheld P-256/P-384/P-521 path rides entirely on JweParser.ParseAsync swapping in a curve-matched work decoy (dataproofs#12). Do the new tests exercise the unheld/decoy path on a non-X25519 curve, or only the held path? If only held, our side leaves the EC constant-work guarantee unverified.

3. Version jump 0.1.0 → 1.1.0 skips 1.0.0 — declaring first API stability and shipping a feature in one step. Intentional (tracking DataProofs 1.1.0)? A CHANGELOG one-liner would stop it reading as an accident later.

4. Exception-type change (MalformedMessageException → CryptoException for malformed iv/tag/encrypted_key) is the correct constant-work outcome and is documented — flagging only as catch-block-visible for consumers. Fine to bake into the inaugural 1.x.

The cross-custody (opaque ⇄ extractable) interop test is the right one to have — that mixed-wallet case is exactly what tends to regress. None of the above blocks merge.

---

If you'd like this posted to the PR, the session token needs comment/write scope on the repo (it currently has read-only access) — or you can paste it yourself.

[moisesja]

Thanks for the review — addressed in d4a33a7.

1. Residual prologue timing oracle (held path runs a backing-store round-trip before the constant-work decrypt). Valid — it was only in an internal comment. Surfaced it on the public IOpaqueKeyResolver XML docs (a <para> "Timing note for implementers") and in PRD FR-SEC-06, naming the mitigation (front the receive endpoint with auth / rate-limiter + the #35 receive-rejection floor). No code change — the signal is inherent to a network HSM/KMS whose lookup latency is existence-dependent; we document it rather than claim closure.

2. Decoy hard-coded X25519 → was the EC unheld/decoy path tested? Good catch — it wasn't. Added Unheld_recipient_on_every_curve_fails_uniformly_via_the_decoy_path as a theory over X25519 / P-256 / P-384 / P-521: pack anoncrypt to a recipient on each curve, unpack holding no key, assert the uniform CryptoException. This drives our X25519 decoy → ParseAsync work-curve (P-384, etc.) → its substituted curve-matched decoy → uniform failure, so the EC decoy path is now verified from our side, not just X25519. (The held EC path was already covered by the authcrypt curve theory.)

3. 0.1.0 → 1.1.0 looks like it skips 1.0.0. It doesn't, but the changelog made it read that way: 1.0.0 is the last published release (on nuget.org); the DidCommVersion build property had drifted to 0.1.0 in source after that release, and the 1.0.0 notes predate this changelog's per-version sectioning. Added a note under [1.1.0] stating exactly that, so 1.1.0 reads as the normal minor it is.

4. Exception-type change (MalformedMessageException → CryptoException for malformed iv/tag). Agreed it's the correct constant-work outcome; already in the CHANGELOG "Changed" section — no further action, baked into 1.x as you suggested.

672 tests pass (Release), build clean.