Add IKeyStore.DeriveSharedSecretAsync (ECDH key agreement) (closes #11)#14
Merged
Conversation
IKeyStore exposed only signing operations, so a non-extractable key-agreement private key (HSM, OS keychain) could not participate in ECDH-based decryption — the recipient's scalar had to be extracted and fed to ICryptoProvider.DeriveSharedSecret as raw bytes, capping HSM-backed deployments to sign-only. Add a key-agreement counterpart to SignAsync: Task<byte[]> DeriveSharedSecretAsync(string alias, ReadOnlyMemory<byte> peerPublicKey, CancellationToken ct) It performs ECDH against a stored key-agreement private key and returns the raw shared secret Z (no KDF — the caller still owns the Concat-KDF/HKDF step, matching ICryptoProvider.DeriveSharedSecret). The private scalar never leaves the store, so a non-extractable / HSM-bound key can decrypt for JOSE ECDH-ES/ECDH-1PU and DIDComm anoncrypt/authcrypt. - IKeyStore: new member with HSM-first XML docs. - InMemoryKeyStore: delegates to ICryptoProvider.DeriveSharedSecret (X25519, P-256, P-384, P-521). - PublicAPI.Unshipped.txt: record both new public members. - Tests: cross-curve parity (store Z == raw Z == peer's Z) for all four curves; private scalar never surfaced; unknown alias -> KeyNotFoundException; non-ECDH key -> ArgumentException. - KeyAgreement sample: demonstrate store-bound Z equals the raw DeriveSharedSecret Z. - Updated the two other IKeyStore implementations (MiniKeyStore sample, RecordingKeyStore double), PRD FR-7, and the CHANGELOG. The new path delegates to the existing, already-tested DeriveSharedSecret, inheriting its on-curve / length validation; it adds no new crypto. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
moisesja
commented
Jun 14, 2026
moisesja
left a comment
Owner
Author
There was a problem hiding this comment.
Automated PR review
Looks good to merge once CI is green. This is a clean, minimal, well-scoped change.
What's solid
- Right design. A single additive method that delegates to the already-tested
ICryptoProvider.DeriveSharedSecretadds no new crypto and inherits its on-curve/length validation. Keeping the scalar inside the store boundary (mirroringSignAsync) and returning raw Z with the KDF left to the caller is consistent with the existing contract. Deferring the optionalIKeyAgreementfactory is the right call given the acceptance criteria are met by one method. - Test coverage. Cross-curve parity across all four ECDH curves (store Z == raw Z == peer's Z), scalar-never-exposed, unknown alias →
KeyNotFoundException, non-ECDH key →ArgumentException. Edge cases are covered. - Hygiene.
PublicAPI.Unshipped.txt, CHANGELOG, PRD FR-7, theMiniKeyStoresample, theRecordingKeyStoretest double, and theKeyAgreementsample are all updated in lockstep. Commit message is clear and links the issue.
One consideration (non-blocking)
- Adding an eighth member to the public
IKeyStoreinterface is a source/binary-breaking change for any external implementer. All in-repo implementers are updated here and the milestone is 1.1.0 (minor), so flagging only so it's a conscious choice — if external implementers are expected before 1.x stabilizes, a default interface method would preserve compatibility. Acceptable as-is for a foundation library at this stage.
Minor nit (optional)
- The interface XML doc lists
KeyNotFoundExceptionandArgumentException, butArgumentException.ThrowIfNullOrEmpty(alias)inInMemoryKeyStorealso throwsArgumentNullException/ArgumentExceptionfor a null/empty alias.ArgumentNullExceptionderives fromArgumentExceptionso it's technically covered, but a one-line<exception>note for the alias guard (asSignAsyncpresumably has) would be tidier.
Nice work — approving in spirit; leaving as a comment rather than a formal approval since CI is still in progress.
Generated by Claude Code
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #11.
IKeyStoreexposed only signing operations, so a non-extractable key-agreement private key (HSM, OS keychain) could not participate in ECDH-based decryption — the recipient's scalar had to be extracted and handed toICryptoProvider.DeriveSharedSecretas raw bytes. That capped HSM-backed deployments to sign-only and left the key-agreement key (the more sensitive long-lived secret for a messaging agent) extractable.This PR adds the encryption-side counterpart to
SignAsync:It performs ECDH against a stored key-agreement private key and returns the raw shared secret Z — no KDF, truncation, or normalization — exactly like
ICryptoProvider.DeriveSharedSecret, so the caller still owns the Concat-KDF / HKDF step. The private scalar never leaves the store, enabling fully non-extractable / HSM-bound decryption for JOSEECDH-ES/ECDH-1PUand DIDComm anoncrypt/authcrypt.Changes
IKeyStore— new eighth member with HSM-first XML docs (raw Z, caller owns the KDF, supported curves).InMemoryKeyStore— implements it by delegating toICryptoProvider.DeriveSharedSecret(X25519, P-256, P-384, P-521). The stored scalar is read only inside the store boundary, mirroringSignAsync.PublicAPI.Unshipped.txt— records both new public members (Roslyn public-API analyzer).store Z == raw DeriveSharedSecret Z == peer's Z), private scalar never surfaced, unknown alias →KeyNotFoundException, non-ECDH stored key →ArgumentException.KeyAgreementsample — demonstrates store-bound Z equals the rawDeriveSharedSecretZ (also satisfies the FR-17 API-coverage check).IKeyStoreimplementations (theMiniKeyStoresample → real delegation; theRecordingKeyStoretest double → forbids it, like every non-SignAsyncmember), PRD FR-7, and the CHANGELOG.Design notes
IKeyAgreementabstraction (analogous toISigner/CreateSignerAsync). Since the acceptance criteria are fully met by a single method and the issue is marked lower/forward-looking, I kept the surface minimal; a factory can be added later additively if a consumer needs a reusable agreement handle.DeriveSharedSecret, inheriting its on-curve and length validation (off-curve / wrong-length peer keys throwCryptographicException/ArgumentExceptionfrom the import).Acceptance criteria (issue #11)
ICryptoProvider.DeriveSharedSecretfor the extractable equivalent, without exposing the private scalar.Verification
dotnet build -warnaserrorclean; full suite 797 tests green; API coverage OK;KeyAgreementsample prints matching Z and exits 0.🤖 Generated with Claude Code