refactor: Simplify device events, cryptography, YubiHSM, and WebAuthn APIs - #643
Conversation
Replace the hand-maintained nativeAotSdkProjects inventory with discovery over src/<Module>/src/*.csproj. A newly added module is now held to the Native AOT contract automatically: it must carry the IsYubiKitSdkLibrary opt-in and be referenced by the verification host, or native-aot-contract-qa fails and names the offending project path. The reported library count is derived from the discovered set rather than a hard-coded literal, and an empty discovery result is itself a failure so the per-project checks cannot pass vacuously.
Rename every YubiHSM password parameter to its plain name (credentialPassword, derivationPassword, currentPassword, newPassword, password) across the interface, implementation, and module docs. The UTF-8 encoding contract and the borrowed-buffer ownership contract stay where they belong, in the XML documentation for each parameter. Split ICredentialPrompt.cs so CredentialKind and CredentialPromptContext each live in a correspondingly named file. Namespace, accessibility, and member bodies are unchanged.
…it tags The four ASN.1 EC key encoders/decoders each carried their own copy of the uncompressed-point rules, and two of them were incomplete: AsnPublicKeyDecoder indexed a zero-length BIT STRING and threw IndexOutOfRangeException instead of CryptographicException, and neither encoder validated caller-supplied points or coordinates against the named curve at all. Move the point-shape rule into AsnUtilities and give it two throwing wrappers that keep the exception taxonomy intact: malformed encoded key data stays a CryptographicException, bad caller input to an encoder is an ArgumentException. Compressed and hybrid points are rejected rather than decompressed, matching the Rust yubikit crate. Also fix the private-key encoder to write the optional RFC 5915 publicKey field as an EXPLICIT [1] BIT STRING. It previously used an implicit tag, which ECDsa.ImportPkcs8PrivateKey rejects, so any private key carrying its public point could not be imported by .NET at all. The decoder now reads the explicit form directly and still accepts the implicit form written by earlier builds.
…ctory WebAuthnClient took its enterprise RP IDs and credential prompt as loose optional constructor parameters, and its prompt-attempt cap was a public const that callers could read but not change. Collect all three on a WebAuthnClientOptions record instead. MaxPromptAttempts is validated as it is assigned, so a non-positive limit fails when the options are built rather than part-way through a ceremony that has already reached the authenticator. BREAKING: WebAuthnClient.MaxPromptAttempts, the enterpriseRpIds/prompt constructor parameters, and the positional CreateWebAuthnClientAsync compatibility overload are removed. Use WebAuthnClientOptions.
…thnBackend IWebAuthnBackend exists so the client's ceremony logic can be tested without an authenticator. It was public, which made a test seam look like an extension point and put CTAP-shaped request records on the WebAuthn surface. Internalize the contract (IWebAuthnBackend, the two Backend*Request records, PinUvAuthMethod, PinUvAuthTokenSession) and the concrete implementation, and rename FidoSessionWebAuthnBackend to WebAuthnBackend now that it is the only one. Unit tests already see internals via the repo-wide Directory.Build.targets rule; DynamicProxyGenAssembly2 is added so NSubstitute can still mock the interface. BREAKING: these types are no longer part of the public API.
MakeCredentialStreamAsync and GetAssertionStreamAsync only ever emitted Processing followed by Finished or Failed, so they duplicated the awaitable ceremony methods while adding a producer task, a channel, and a second error path. Both the Rust and Python yubikit clients expose direct ceremony methods instead, so removing the streams moves this SDK toward canonical behaviour. No replacement callback is introduced: the unified interaction model is deferred. Input still comes from pinBytes or WebAuthnClientOptions.CredentialPrompt, and abandonment still comes from the cancellation token. Two stream tests covered behaviour with no non-stream equivalent (a backend CTAP failure surfacing as a typed error, and a PIN-needing ceremony with no prompt configured); they are converted to direct MakeCredentialAsync tests. The rest covered stream mechanics only and are deleted with Client/Status/. BREAKING: MakeCredentialStreamAsync, GetAssertionStreamAsync, WebAuthnStatus and its subtypes, and StatusChannel are removed.
WebAuthnClient.cs was over 1200 lines and mixed both ceremonies, PIN/UV token acquisition, validation, and error mapping in one file. Split it into partial files along those seams. WebAuthnClient.cs keeps construction, disposal, and the shared zeroing helpers. Pure move: the multiset of non-blank class-body lines is identical before and after, and the WebAuthn unit suite reports 199 passing on both sides.
…ders Two encoder input paths accepted things they should not. EncodeToPkcs8(ECParameters) treated a half-supplied public point as no point at all: if exactly one of Q.X and Q.Y was null it omitted the optional RFC 5915 [1] publicKey field and returned a perfectly valid private key encoding that had quietly dropped the coordinate the caller did supply. Omit [1] only when both coordinates are null; a half point is now an ArgumentException. This also brings it in line with the public-key encoder, which already rejected it. The shared point-shape helper sized coordinates via KeyDefinitions.GetByOid, which is a general OID resolver rather than an EC-curve one. It also resolves X25519, Ed25519, AES and Triple DES, several of which have a 32-byte definition, so P-256 sized coordinates carrying a non-EC algorithm OID passed validation and were then emitted as an id-ecPublicKey key whose curve parameter is not a curve. Gate on Oids.IsECDsaCurve instead, and split the resolver into a decode flavour that throws CryptographicException and an encode flavour that throws ArgumentException, so a genuinely unsupported curve no longer surfaces from an encoder as NotSupportedException. The curve is now also checked at the ECParameters boundary rather than only inside the point check, because a private key with no public point never reaches the point check.
DeviceEventBroadcaster and DeviceEventStream became one DeviceEventHub: Publish fans out into per-watcher bounded channels with TryWrite and never runs consumer code, so a slow, abandoned, or throwing watcher can no longer block or interrupt UpdateCache while the monitor holds its publication gate. WatchAsync is now the only device change stream. DeviceChanges is removed outright with no shim, which also drops the last System.Reactive reference and lets IYubiKeyDeviceRepository and IYubiKeyDeviceMonitorService go - both had a single implementation each. Per-watcher independence is preserved exactly: lazy subscription, a 256-event buffer that faults only its own watcher on overflow, cancellation isolation, and normal completion on repository disposal. Lifecycle tests that need an in-flight publication use the new PublishAdmittedForTest seam, since a consumer can no longer hold one. The monitor's generation, debounce, polling, listener, and retry model is unchanged; only delivery moved.
…pted token ClientPin allocates the decrypted PIN/UV auth token and hands it to WebAuthnBackend, which passed it to PinUvAuthTokenSession. The session copied it, so disposing the session cleared only the private clone and left the original plaintext token live in managed memory until GC. The session now adopts the array instead of copying it, so there is exactly one live copy and it is zeroed whenever the session is disposed - which the ceremony does in a finally, on success, failure and cancellation alike. Replaces GetAssertion_PinTokenZeroedAfterMethodReturns, which asserted Assert.True(true) against an authenticator that never needed a token, with three tests that force token acquisition and inspect the session's owned buffer after a successful, a failed and a cancelled ceremony, plus direct ownership tests on the session itself. Also documents why the pinUvAuthParam copies in WebAuthnBackend are load-bearing: ExcludeListPreflight reuses one buffer across chunk probes, so the backend must not zero the caller's array.
Discovery alone left two gaps the previous hand-maintained inventory had covered. Both are now checked against the verification host instead of a list. - A library that moves out of the src/<Module>/src/ shape used to drop silently out of the per-project loop and pass green with a smaller count. Every ProjectReference the host declares under src/ must now resolve to a discovered SDK library, which also catches a reference left behind by a deleted module. - A project reference with no rooted type lets ILC trim the assembly out of the host's closure, so the publish verifies nothing for it. The entry-type list is now a project -> type map and a discovered library with no mapping fails. Host references are parsed with XDocument rather than substring-matched: a commented-out ProjectReference, or a file name appearing only in the host csproj's header comment, satisfied the old check while referencing nothing. The Cli.* exclusion becomes a closed two-name set. Any other Cli.* module at the SDK layout now fails as unclassified rather than being silently skipped, and the excluded set is printed alongside the count. The discovered set is unchanged: the same 10 SDK libraries are verified.
The repository-level overflow test started its second watcher after the fault, so at overflow time exactly one watcher was subscribed. It pinned "overflow faults" and "publication continues", never isolation - breaking per-watcher isolation in the hub left it green. It now subscribes the healthy watcher before the burst and paces publication the way the hub-level test does, then asserts that watcher received all 511 events and is still enumerating. Delivery to a watcher whose channel is already terminated now returns immediately on a volatile flag. An abandoned enumerator stays in the publisher's snapshot for the process lifetime, and every publish was allocating and formatting a fresh overflow exception for it - 688 bytes per event, inside the monitor's publication gate. WatcherCount is deliberately left counting terminated-but-registered watchers; it is the diagnostic that makes that leak visible. The hardware watch test was disposing its enumerator with a MoveNextAsync still in flight on the timeout path, which throws NotSupportedException and would have replaced the assertion message an operator needs. The hot-plug teardown no longer insists the pump ended in cancellation specifically. Document on WatchAsync that the caller must dispose the enumerator.
… token buffers Two paths were guarded only by comments, with an existing test standing in for coverage that did not exist. WebAuthnBackend's .ToArray() of the caller's pinUvAuthParam is load-bearing: ExcludeListPreflight hoists one parameter above its chunk loop and rebuilds a request around that same buffer for every chunk, so a backend that zeroed the caller's array would send an all-zero parameter from chunk two onwards. The one test naming the multi-chunk scenario substitutes IWebAuthnBackend and never executes WebAuthnBackend, so it stayed green through that mutation. WebAuthnBackendTests now drives the concrete backend over a substituted IFidoSession and pins both halves of the contract: the caller's buffer survives, and the copy the backend put in the CTAP options is cleared. The MakeCredential token path had no buffer-lifecycle coverage at all. Deleting the pre-flight tokenSession.Dispose() orphans the decrypted token minted before pre-flight, and nothing failed. The existing PuatRequired test drives four acquisitions through that code but hands out new byte[32], which is already all-zero, and asserts nothing about them. Three ceremony tests now hand out a distinct sentinel buffer per acquisition and assert every issued buffer is zeroed, on success, on a backend failure, and across the PuatRequired retry. Also hardens two helpers that had sham-guard shapes. TokenBufferAssert.Zeroed passed vacuously on a zero-length buffer; it now rejects one. The ZeroMemory helpers in WebAuthnBackend and WebAuthnClient silently skipped when the memory was not array-backed; they now Debug.Assert instead. They cannot throw because every call site is a finally block.
The rule is 'only format files you changed', not a restriction on the subcommand. Also records that formatting a non-project file exits 0 without checking anything, so it must not be reported as a passing gate.
Five quanta were developed in isolation, so a few concepts ended up expressed more than once in the merged tree. None of these change behaviour; each removes a copy a reader would otherwise have to hold. WebAuthn production: `ZeroMemory(ReadOnlyMemory<byte>?)` existed verbatim, comment and all, in both WebAuthnClient and WebAuthnBackend. It is now `Util.SensitiveMemory.Zero`, next to the existing Base64Url helper. WebAuthn tests: MockFido2Responses is the shared fixture home, but WebAuthnClientMakeCredentialTests still carried a byte-for-byte copy of CreateMockMakeCredentialResponse and its three builders, and there were three separate ways to build a mock GetAssertionResponse. The shared builder now takes the optional numberOfCredentials / signature / user that the private copies needed, and the copies are gone. Core tests: GetNamedCurve and BuildUncompressedPoint each appeared in two of the Asn*Tests files; both now live in EcTestSupport. UncompressedPoint deliberately keeps its own implementation rather than calling AsnUtilities.BuildUncompressedEcPoint, so encoder tests do not check that helper against itself.
Seven findings from the merged-branch audit, each self-contained. YubiHsm credential-password validation threw with the internal helper's own parameter name, so ArgumentException.ParamName reported "password" — a name no public parameter uses. Callers filtering on it got nothing usable. The public surface spells the parameter three ways (credentialPassword, currentPassword, newPassword), so the name is now threaded from each entry point. AsnPrivateKeyEncoder checked the curve OID and the public point strictly but wrote the private scalar D straight through. A P-384 curve with a 32-byte D and a valid 97-byte point produced a well-formed-looking RFC 5915 ECPrivateKey whose privateKey OCTET STRING contradicted the curve beside it, with no exception. The ECParameters path now validates D against the curve. The raw-value overload stays permissive: its EC branch has no production caller, and the only key types that do reach it (Curve25519) are already checked exactly. YubiKeyDeviceManager.DisposeAsync cleared the repository before disposing it. Disposal already clears, so the Clear() call bought nothing and opened a window in which a publication that outlived the monitor's bounded drain could find a live-but-emptied repository, diff against an empty cache, and emit Added for every attached device after DisposeAsync had returned — contradicting the documented guarantee that no device event escapes a disposed manager. DeviceEventHub.WatcherCount documented "live subscriptions" while counting every registered watcher including overflow-terminated ones. The count is right (a leaked watcher should stay visible); the doc was wrong. AsnUtilities claimed a CryptographicException-for-malformed-data taxonomy that the decoders do not follow for unsupported curves: both reject the curve first, with their own exception types, leaving the CryptographicException branch unreachable from production. The remark now scopes the taxonomy to point shape and names curve rejection as each decoder's own concern, and the tests covering that branch say plainly that no caller reaches it. Two YubiKeyManagerStaticTests started real HID and PC/SC listeners inside a unit test, contending for the process-wide four-slot discovery-worker pool and making unrelated tests fail intermittently with "discovery worker capacity is saturated". The watcher-independence assertion moves onto the manager seam, where fake listeners let it also assert the part it previously had to omit — the surviving watcher keeps receiving. The post-shutdown recreation test drops an assertion about ambient machine state in favour of one about the facade. The facade's remaining lifecycle tests have no injection seam by design, so the class joins the collection that already serializes discovery-pool consumers. Finally, the codemapper skill still taught IObservable<DeviceEvent> DeviceChanges, the last reference anywhere to a removed API.
…window Dispose() emptied the cache but left the HasData flag set, so a disposed repository reported a cache it no longer held. Reset it alongside the clear, and correct the HasData doc: it is a cache-validity flag, not a count, so a scan that found nothing legitimately sets it. The disposal-window regression test only released its parked publication after DisposeAsync had returned, by which point both the old and new orderings had already disposed the repository - it passed either way. Add a teardown hook on the manager so a test can resume a parked publication at the exact point the manager is about to dispose the repository, after every other teardown step, and assert nothing is emitted. Retitle the old test to say what it actually pins: the post-return path. Delete the orphaned Repository.Clear(). It had no production callers and its doc still recommended the shutdown usage that turned out to be unsafe.
…ameters Finish the naming convention started in YubiHSM Auth. The SDK carried two spellings for the same kind of UTF-8 secret across sibling modules; every public secret parameter is now named plainly, and the UTF-8 encoding contract plus the borrowed-buffer ownership contract live in the XML documentation for each parameter rather than in the identifier. Fido2: ClientPin.SetPinAsync, ChangePinAsync, GetPinTokenAsync and GetPinUvAuthTokenUsingPinAsync take pin, currentPin and newPin. OpenPgp: IOpenPgpSession/OpenPgpSession VerifyPinAsync, VerifyAdminAsync, ChangePinAsync, ChangeAdminAsync, SetResetCodeAsync and ResetPinAsync take pin, currentPin, newPin and resetCode; the public abstract Kdf.Process takes pin. Oath: IOathSession.DeriveKey takes password. Several OpenPgp members had no <param> documentation at all, so the UTF-8 contract had been carried solely by the identifier. Those docs are now written out, along with the caller's ownership and clearing obligation. Also fixes a latent ArgumentException.ParamName defect surfaced by the rename. ClientPin.ValidatePin is a shared private helper, so SetPinAsync and ChangePinAsync reported the helper's own local name instead of the public parameter the caller supplied. The helper now takes an explicit paramName, matching HsmAuthSession.ValidateCredentialPassword. Names and documentation only. No encoding, ownership, padding, zeroization, or CTAP/OpenPGP wire behaviour changed; no ZeroMemory call or try/finally was added, removed or moved. Breaking for named arguments only, with no [Obsolete] aliases, per the alpha posture.
ValidatePin reports the caller's public parameter name, but the existing ClientPinTests only asserted the exception type, so a wrong nameof would have gone unnoticed. Assert the exact ParamName for SetPinAsync (newPin), both ChangePinAsync routes (currentPin, newPin), GetPinTokenAsync (pin) and GetPinUvAuthTokenUsingPinAsync (pin). Three existing SetPinAsync tests and both ChangePinAsync tests were extended in place; GetPinTokenAsync and GetPinUvAuthTokenUsingPinAsync had no length-validation test at all, so those two are new.
Our event architecture wins; upstream's device-identity behaviour is carried in. - YubiKeyDeviceRepository.UpdateCache keeps our DeviceEventHub fan-out while gaining upstream's serial-substitution republish and DeviceInfo retention. - DeviceChanges/RecordingObserver stay deleted; upstream tests written against the observer harness are ported to WatchAsync/DeviceEventWatcher with every assertion preserved. - Upstream's flat device model (YubiKeyDevice, connection slots) arrives intact. Verified: 12/12 projects, 2757 tests, 0 failed; resilience 75/0; AOT 10+2.
- Rename ListenerEvents_..._PerTransport to _EvictingEvidenceGlobally; upstream reworded the doc to say eviction is global but left the old name. - Strip a stray trailing newline (.editorconfig sets insert_final_newline=false). - Point hid-listener-rescan-hints evidence at DeviceEventHub.cs, replacing the deleted broadcaster/stream files. - Clarify that UpdateCache orders publication at enqueue, not delivery.
Publication never runs consumer code, but the resumption thread is not guaranteed: a buffered event can complete MoveNextAsync synchronously on the caller's thread. The previous text promised background-thread delivery.
AGENTS.md becomes a pointer to CLAUDE.md per upstream's consolidation; the operational detail it used to carry now lives in TOOLCHAIN.md (verified: total:, RequiresUserPresence, No tests matched all present there). Our dotnet format whitespace guidance moves to CLAUDE.md, which is now canonical.
Coverage and CRAP
CRAP decreased by 194. Note CRAP increased in: Core (+15). |
| /// </summary> | ||
| public bool IsRetry { get; init; } | ||
|
|
||
| /// <summary> |
|
Hardware verification complete — Rig: single authorized USB YubiKey, SN 103, FW 5.8.0. Control run first (no human interaction, to prove the test is not a sham guard): waited the full 120 s and failed correctly with Ceremony run — PASSED (28 s): The distinct object references ( This also validates the lazy-subscription fix applied during the port — One caveat worth recording: an earlier attempt appeared to pass in 3 s immediately after the rig was re-arranged, because discovery was still settling and residual insertion events satisfied both waits. Valid test, invalid ceremony. Anyone re-running this should let the rig settle first and check the timeline timestamps look human-paced. |
What this does
Alpha API cleanup across device events, Native AOT validation, ASN.1 EC handling, YubiHSM/credential naming, and WebAuthn. Driven by an agreed ideal-state artifact with 54 acceptance criteria; all 54 have direct evidence.
Based on
yubikit@c3b7e206. Built as five parallel work quanta in isolated worktrees, each cross-vendor reviewed, then merged with zero conflicts, followed by a simplification pass, a whole-diff audit, and one owner-requested follow-up.Breaking changes are intentional and unwrapped — this is alpha, and no compatibility shims were added.
Changes by area
Device events —
WatchAsyncis now the only public device-change stream. PublicDeviceChanges(IObservable) is gone, along withDeviceEventBroadcaster,DeviceEventStream, and two unused interfaces. One internalDeviceEventHubbacks delivery, preserving per-watcher bounded buffers, 256-event overflow isolation, lazy subscription, cancellation isolation, and normal completion on disposal.System.Reactiveis dropped from the dependency graph entirely.Native AOT — the hand-maintained
nativeAotSdkProjectsinventory is replaced by filesystem discovery. The contract now also checks the reverse direction (a host-referenced project that stops matching the SDK layout fails), parsesProjectReferencewithXDocumentinstead of a substring match, and forces an entry-type decision for any newly discovered library.ASN.1 / EC — one shared point-shape validator across all four encoders/decoders. Fixes an
IndexOutOfRangeExceptionon empty public points, adds curve-specific coordinate and private-scalar length validation, rejects non-EC OIDs that previously produced a valid-lookingid-ecPublicKeyencoding, and emits RFC 5915[1]as an explicitly tagged BIT STRING so keys round-trip through the .NET importer for P-256/384/521.WebAuthn — adds
WebAuthnClientOptions(CredentialPrompt,EnterpriseRpIds,MaxPromptAttempts, default 3). The backend seam is internal,FidoSessionWebAuthnBackendis renamedWebAuthnBackend, the ceremony status streams andWebAuthnStatushierarchy are removed, and the 1,255-line client is split into partials by concern. A unified interaction model stays deferred; no replacement callback API was introduced.Secret-parameter naming — the
...Utf8suffix is dropped from public secret parameters across YubiHSM, Oath, OpenPgp, and Fido2. The UTF-8 contract and caller-ownership/clearing rules move into XML docs; several OpenPgp members had no<param>docs at all and now do.Notable fixes found along the way
ClientPin's decrypted token was cloned intoPinUvAuthTokenSessionand the original abandoned to the GC. Ownership now transfers, so exactly one copy exists and it is zeroed deterministically.ArgumentException.ParamNamenamed nothing. BothClientPin.ValidatePinandHsmAuthSessionreported an internal helper's local name. Now every public entry point reports its own parameter, pinned by tests.YubiKeyManagerStaticTestswere spinning up real PC/SC and HID listeners inside a unit-test project, failing intermittently withdiscovery worker capacity is saturated. Serialised into the existing discovery-admission collection; the suite is now stable across repeated runs.Verification
dotnet toolchain.cs builddotnet toolchain.cs testdotnet toolchain.cs -- resilience --fastdotnet toolchain.cs native-aot-contract-qaCore 1211/0/3 · Fido2 430/0 · WebAuthn 213/0 · Piv 193/0 · YubiOtp 155/0 · Oath 106/0 · OpenPgp 105/0 · Management 91/0 · YubiHsm 85/0 · SecurityDomain 47/0 · Cli.Shared 43/0 · Cli.Commands 9/0
No integration tests were run — hardware was in use by other work. Integration projects compile.
Known follow-ups (deliberately not in this PR)
WebAuthnClient.Registration.csallocatespinUvAuthParambeforeBuild*ExtensionsCborcan throw, so an extension-pipeline failure leaves a per-request HMAC unzeroed. Wants its own change across both ceremony paths.YubiKeyManagerStaticTestsis stable but still non-hermetic; a hermetic fix needs aninternal staticmanager-factory seam.verification/NativeAotVerification/Program.csis compiled by no build step in the repo; the AOT contract validates it by text matching only.AsnPublicKeyDecoderthrowsNotSupportedExceptionfor an unsupported-but-well-formed curve on decode — reviewed and accepted, but an open taxonomy question.Review notes
Reviewers may want to look hardest at
DeviceEventHubconcurrency, the RFC 5915 tagging change, and the PIN token ownership transfer. Each was independently verified cross-vendor, but they are the three places where a subtle mistake would matter most.Update: merged with
yubikit(device-identity / flat device model)yubikitadvanced 28 commits while this branch was in review, including PR #629 (flat device model + device identity) and PR #641 (agent-doc consolidation). Both are now merged in.Merge policy: our event architecture wins, upstream's device-identity behavior is carried in. Upstream never touched
DeviceEventBroadcaster,DeviceEventStream, or either dead interface — it only used them — so the deletions were uncontested. Only ~40 lines of production code genuinely overlapped.What was carried in from upstream:
DeviceInfometadata retention inUpdateCache, rewired ontoDeviceEventHubYubiKeyDevice, connection slots) intactWatchAsync/DeviceEventWatcherwith every assertion preservedMerge verification
Because a silently-dropped upstream behavior would not fail any test, the resolution was verified structurally rather than only behaviorally:
YubiKeyDeviceRepository.cspatch(ours→merge) == patch(base→theirs)and vice versa, byte-exactDeviceIdentityContractTests33→33,HotPlugIdentityContractTests10→10serialContradiction,DeviceInforetention, or hub delivery each goes red — the second through the ported upstream tests, proving they are not sham guardsPost-merge gate
12/12 projects, 2757 tests, 0 failed · Core 1261/0/3 · Fido2 449/0 · resilience 75/0 · AOT 10+2 · build 0 errors.
Still needs a human
HotPlugIdentityContractTestsis ported and compiles but was never executed — it is hardware-gated and the YubiKeys were in use. Please run it against real hardware before merging.