refactor(applets)!: consolidate applet session public APIs - #642
Conversation
Coverage and CRAP
CRAP decreased by 9. Note CRAP increased in: Management (+20), Piv (+65), Fido2 (+4), Oath (+6), YubiOtp (+4), OpenPgp (+26), YubiHsm (+17), Cli.Commands (+1). |
f3b5a65 to
06a6507
Compare
032140a to
35f6fb0
Compare
8b73770 to
223ff54
Compare
|
Rebased onto current One thing worth flagging for reviewers: #639 ( That is the analyzer working as intended — the same pass previously caught ten stale or missing Core declarations after the earlier rebase. Verified on the rebased branch: build 0 warnings / 0 errors, 13/13 unit projects, pack 10 packages. Hardware: SecurityDomain 20/20, Management 40 passed / 0 failed, YubiHsm 11/11, OpenPgp 46/46, Piv 74 passed / 2 skipped, Fido2 30/30. |
|
Hi! Heads-up from #643 — I ran a merge preview between the two branches so the overlap is known ahead of time rather than discovered during the resolve. Posting the analysis here since this is where the resolution will happen. Owner decision already taken: #643 merges first, and #642 absorbs the conflicts when it catches up. Nothing is being asked of this PR right now. Overlap22 files are touched by both branches; 6 actually conflict. Merge base is
Worth noting up front: Oath auto-merges cleanly. An earlier note of mine claimed OpenPgp — mechanicalThe two branches changed orthogonal things on the same declarations, which is why git flags them:
Suggested resolution is "this PR's structure, #643's names" — the boolean split is the better API and should win; the rename and the ownership docs layer on top of it. One trap that cost me a few minutes: in WebAuthn — needs a decision, and one has been madeThis one is not a naming clash, it is two complementary consolidations that both want the parameter name
They cover genuinely different parameter groups, so neither is redundant. Owner decision: nest AlsoThis branch is currently 8 commits behind Happy to help with the resolve when the time comes — just ping me. |
Normalize the public surface of all eight applet sessions onto one recognizable grammar, using the 2.0 alpha window for a single coordinated breaking change. Each applet keeps its own protocol vocabulary; session creation, lifecycle state, interface parity, async shape, memory ownership, collections, and options thresholds now follow shared rules. Session creation - Add Core SessionCreationOptions carrying ProtocolConfiguration, ScpKeyParameters, PreferredConnectionType, and FirmwareVersionOverride. - Every applet factory is now (connection|device, SessionCreationOptions? = null, CancellationToken = default). PreferredConnectionType selects a connection in IYubiKey extensions and asserts in direct factories, via a single shared ApplicationSession helper rather than eight copies. - Expose ConnectionType on IApplicationSession; drop ManagementSession.Transport. Surface minimization and parity - Remove all seven applet DependencyInjection helpers and factory delegates. - Remove the duplicate PIV and YubiHSM TouchNotificationCallback types in favor of Action, and surface OnTouchRequired on both interfaces. - Bring concrete sessions and their I*Session contracts to exact parity. Operation shapes - Management: SetDeviceConfigOptions replaces the reboot plus lock-code positional tail; lock codes are borrowed ReadOnlyMemory. - PIV: PivKeyCreationOptions for key policies, PivUserVerification as a three-state enum replacing two ambiguous booleans, PivCertificateCompression replacing a compress flag, and consistent Utf8 parameter naming. - OATH: RequireTouch moves into CredentialData with a ParseUri overload so URI-parsed credentials can still set it; CalculateAll returns IReadOnlyDictionary. - OpenPGP: unified GenerateKeyAsync(KeyRef, AlgorithmAttributes) and an explicit split of the two PIN reset flows. - Security Domain: CaIdentifierType flags replace paired booleans. Fixes key-reference parsing that previously included the TLV tag and length. - YubiOTP: GetSerialAsync becomes GetSerialNumberAsync. - WebAuthn: adopts SessionCreationOptions and drops its compatibility overload. Compatibility enforcement - Add Microsoft.CodeAnalysis.PublicApiAnalyzers to the ten shipping SDK libraries with reviewed PublicAPI.Unshipped.txt declarations. PublicAPI.Shipped.txt stays empty until the surface is accepted. RS0026/RS0027 are suppressed only at specific pre-existing overload families, so new violations still fail the build. - Add a cross-module convention test project enforcing factory shape, async shape, interface parity, and memory and collection rules. Behavior is unchanged with one deliberate exception: no transport fallback, secure-channel, session ownership, or security lifecycle semantics were modified. The exception is Security Domain CA identifier decoding, which previously read the whole key-reference TLV instead of its value and so reported Kid=0x83, Kvn=0x02 for every entry on real hardware. The existing integration test asserted only a non-empty result and could not observe it, so it is joined by GetCaIdentifiersAsync_DecodesKeyReferenceValue, which pins the key identifier to the SCP11 set and was confirmed to fail against hardware before the fix. Deferred: the one-shot IYubiKey convenience taxonomy (CNV) and whether FirmwareVersionOverride belongs on the public surface (VER), plus two pre-existing mutable-array surfaces, all recorded in docs/architecture/applet-public-api.md and due before PublicAPI.Shipped.txt is populated. See docs/migration/v1-to-v2.md for the mechanical old-to-new table.
Two pre-existing integration failures, both unrelated to the applet API
consolidation and both reproducible on the base branch.
Security Domain left the device unusable by later suites. Tests that
import, rotate, or delete SCP03 key sets ended with no key set matching
Scp03KeyParameters.Default, so every later consumer of the default keys
failed secure-channel establishment with SW=0x6A88. The symptom appeared
in whichever suite ran next rather than in the test that caused it: the
Management SCP03 tests failed only when the Security Domain suite had run
first, which reads as an unrelated regression. Bisecting by test class
identified the SCP03 key tests and both SCP11 classes as the sources.
The three SCP03 key tests now restore the factory Security Domain in a
finally block. The SCP11 classes derive from a new
SecurityDomainStateRestoringTests base that restores it after every test,
since xUnit constructs the class once per test.
YubiHSM Auth had no byte-level coverage of the change-password instruction.
Probing every connected device showed the instruction works on a development
applet reporting 0.0.1, and is missing on another development build of the
same reported version, which answers SW=0x6D00:
SN=31683481 applet=5.7.4 -> NotSupportedException (correctly gated)
SN=125 applet=0.0.1 -> SW=0x6D00
SN=20260533 applet=5.4.3 -> NotSupportedException (correctly gated)
SN=103 applet=0.0.1 -> full flow OK
The instruction byte, P1 selectors, TLV order, and the 5.8.0 gate were all
verified against the canonical Rust implementation and are correct, so the
SDK is unchanged and the hardware test is left to fail on a key whose applet
omits the instruction. Two byte-level unit tests now pin the instruction
byte, both P1 selectors, and TLV order, so a genuine encoding regression
fails without hardware and is distinguishable from a deficient applet build.
Verified on hardware, running the suites back to back in the order that
previously poisoned state: SecurityDomain 20/20, Management 42 passed with
0 failed, Piv 76/76, Oath 18/18, OpenPgp 46/46, YubiOtp 10/10, YubiHsm 10
passed with 1 skipped, Fido2 26 passed with 4 skipped.
An alpha or beta YubiKey reports major version 0 and is built from a development branch at or ahead of 5.8.0, so every feature gate must accept it. FirmwareVersion.IsAtLeast and Feature.IsSupportedByFirmware already do. PivSession.GetConservativeDefaultManagementKeyType cancelled that allowance with an explicit !IsAlphaOrBeta before calling IsAtLeast(5, 7, 0), so a development key fell back to Triple-DES. Such a key defaults to AES-192, so the fallback picked an algorithm the key does not use whenever management key metadata was unavailable, both when opening a session and after a reset. Removing the exclusion leaves IsAtLeast to answer, which reports true for a development version and keeps released firmware below 5.7 on Triple-DES. Three unit tests pinned the previous behavior and now assert AES-192 for a 0.x version. Two tests were added so pre-5.7 firmware keeps explicit Triple-DES coverage on both the create and reset paths, which the old assertions had conflated with the development-version case. Document the property itself, since the rule was only implicit in the comparison helpers: an alpha or beta version means at least 5.8.0 and is newer than any release, !IsAlphaOrBeta && IsAtLeast(...) is the mistake to avoid, and the placeholder carries no build detail. An individual applet build can therefore lack a feature the version implies and answer SW=0x6D00. That is deliberately not handled in code, because detecting it would either break correctly built development keys or add a probe round trip to every gated call. The comment exists so the failure is recognised quickly as a deficient applet rather than a defective gate. Verified on hardware: Piv 76/76.
Reset re-SELECTs the applet to refresh cached state, and the SELECT response carries the applet's own version. YubiHSM Auth and OpenPGP adopted that version, discarding a FirmwareVersionOverride supplied through SessionCreationOptions at creation. The override then lapsed mid-session, so feature gates evaluated after a reset disagreed with the ones evaluated before it, with nothing in the API indicating the effective version had changed. OATH already treats reset as refreshing applet state rather than caller policy and preserves the effective version. Both applets now match, and the re-SELECT return value is explicitly discarded with the reason recorded. Found while using the override to establish, on hardware, whether the 5.8.0 gate on FeaturePasswordChange is correct. It is: with the gate bypassed so the device answers for itself, an applet reporting 5.7.4 and one reporting 5.4.3 both reject INS 0x0B with SW=0x6D00, matching the canonical Rust gate of 5.8.0 and the canonical Python device test condition of min_version(5, 8). Verified: a unit test pins the override across reset for YubiHSM Auth and fails against the previous behavior with the applet-reported 5.4.3. Hardware: OpenPgp 46/46; YubiHsm unchanged at 10 passed with the single expected failure on a key whose applet omits the instruction.
Rebasing onto yubikit picked up three merged changes that overlap this work - the device-event rework, the WebAuthn client options type, and the drop of the Utf8 suffix from secret parameter names. Six files conflicted textually; the rest of this commit is fallout that merged cleanly but did not compile or did not mean what it used to. The WebAuthn factory now carries both options objects instead of one. Trunk had folded enterprise RP IDs, the credential prompt and the prompt-attempt cap into WebAuthnClientOptions; this branch had folded SCP parameters, protocol configuration, preferred transport and the firmware override into SessionCreationOptions. Both named their new parameter "options", so the merged body passed one identifier to two methods expecting different types - a clean auto-merge that could not compile. They are kept separate, as "options" and "sessionOptions", because they are not the same concern: client options are equally meaningful on the public WebAuthnClient constructor, where the caller supplies their own session, while session options only make sense on the factory that creates one. Nesting them would put a property on WebAuthnClientOptions that is silently ignored on the constructor path. The OpenPgp reset API keeps this branch's split into ResetPinUsingResetCodeAsync and ResetPinUsingAdminAuthenticationAsync - better than the boolean flag it replaced - with trunk's parameter names and its buffer-ownership documentation. Kdf.cs was a pure name collision, both sides renaming the same parameter, so trunk's naming wins there too. The wire test keeps the new algorithm-attributes assertion this branch added; only its fake KDF override needed renaming, which is why the file could not simply be taken from either side. The PublicAPI tracking files introduced here record parameter names, so every signature touched by the Utf8 rename read as one symbol added and one removed. Those are reconciled mechanically against what the analyzer reported: 41 renames across Fido2, Oath, OpenPgp and YubiHsm, the removal of the deleted YubiKeyManager.DeviceChanges, and the WebAuthn surface catching up with the backend being internalized and the ceremony status streams removed. Two call-site fixes were semantic rather than textual. A YubiHsm test kept trunk's firmwareVersion argument against this branch's SessionCreationOptions signature, and now passes FirmwareVersionOverride. The WebAuthn transport and factory tests passed SessionCreationOptions to the parameter that is now WebAuthnClientOptions, and now name sessionOptions explicitly. FactoryShapeTests asserted a parameter that trunk deleted, so it could not have passed in any resolution. It now pins the two-options shape and records why the separation exists. Verified: build 0 errors 0 warnings; full unit suite 13 projects 2747 tests 0 failed.
223ff54 to
8bb6750
Compare
The remark predates the split into two options objects and still claimed that 'options' is validated and applied by CreateFidoSessionAsync. That is now sessionOptions; options is forwarded to the client instead. Both names resolve to real parameters, so paramref stayed valid and nothing - compiler, analyzer or test - could have flagged the false statement.
Deviation from the agreed design — please veto if you disagreeFlagging this at the top because it changes a public API shape and it was my call during conflict resolution, not a ratified decision. The agreed plan was to nest public async Task<WebAuthnClient> CreateWebAuthnClientAsync(
WebAuthnOrigin origin,
PublicSuffixChecker isPublicSuffix,
WebAuthnClientOptions? options = null, // forwarded to the client
SessionCreationOptions? sessionOptions = null, // used to create the session
CancellationToken cancellationToken = default)Why I changed it. The nesting decision was made from a summary that did not mention this: The two objects also have genuinely different lifetimes: one configures the client that is returned, the other configures a session this factory creates as a convenience. The honest case against, so you can weigh it:
Alternatives I did not take: nest but throw from the constructor when I also rewrote your contract test Happy to redo this either way; it is a signature and a test, cheap to flip now and expensive after release. What else the rebase involvedTrunk moved by three overlapping PRs while this was open (#643 device events +
The OpenPgp reset API keeps this branch's split into Verification: build 0 errors; #642 2747 tests / #644 2786 tests, 0 failed; |
… sessionOptions The factory takes two unrelated options objects, and neither should be the bare name "options". FactoryShapeTests.ValidateFactory reserves that name for SessionCreationOptions on every other factory in the SDK, so using it here for WebAuthnClientOptions would make one name mean two different types depending on which factory you are looking at. Naming both explicitly costs symmetry with the WebAuthnClient constructor, whose single options parameter keeps its name because there is nothing there to confuse it with, and buys an API where the reader never has to know which convention applies.
|
Update: the deviation above is ratified — two parameters it is, on the grounds that One follow-on from that same reasoning. Neither parameter is now named plain public async Task<WebAuthnClient> CreateWebAuthnClientAsync(
WebAuthnOrigin origin,
PublicSuffixChecker isPublicSuffix,
WebAuthnClientOptions? clientOptions = null,
SessionCreationOptions? sessionOptions = null,
CancellationToken cancellationToken = default)
The shape test now pins this and records why, so the next person does not have to rediscover it. Also fixed in passing: the Re-verified after the rename: build 0 errors 0 warnings, 13 projects 2747 tests 0 failed, formatting checked with a positive control. |
1 participant
What this does
Normalizes the public surface of all eight applet sessions onto one recognizable grammar, using the 2.0 alpha window for a single coordinated breaking change. Each applet keeps its own protocol vocabulary — session creation, lifecycle state, interface parity, async shape, memory ownership, collections, and options thresholds now follow shared rules.
Implements the accepted plan in
docs/plans/2026-09-04-applet-public-api-consolidation.md. The durable rules live in the newdocs/architecture/applet-public-api.md.Session creation
SessionCreationOptionscarryingProtocolConfiguration,ScpKeyParameters,PreferredConnectionType, andFirmwareVersionOverride.(connection|device, SessionCreationOptions? = null, CancellationToken = default).PreferredConnectionTypeselects a connection inIYubiKeyextensions and asserts in direct factories, through a single sharedApplicationSessionhelper rather than eight copies.ConnectionTypeis exposed onIApplicationSession;ManagementSession.Transportis removed.Surface minimization and parity
DependencyInjectionhelpers and factory delegates.TouchNotificationCallbacktypes in favor ofAction, and surfacedOnTouchRequiredon both interfaces.I*Sessioncontracts are now at exact parity, including parameter names.Operation shapes
SetDeviceConfigOptionsreplaces the reboot + lock-code positional tail; lock codes are borrowedReadOnlyMemoryPivKeyCreationOptionsfor key policies;PivUserVerificationthree-state enum replacing two ambiguous booleans;PivCertificateCompressionreplacing a compress flag; consistentUtf8parameter namingRequireTouchmoves intoCredentialDatawith aParseUrioverload so URI-parsed credentials can still set it;CalculateAllreturnsIReadOnlyDictionaryGenerateKeyAsync(KeyRef, AlgorithmAttributes); the two PIN reset flows are now explicit separate methodsCaIdentifierTypeflags replace paired booleansGetSerialAsyncbecomesGetSerialNumberAsyncSessionCreationOptionsand drops its compatibility overloadCompatibility enforcement
Microsoft.CodeAnalysis.PublicApiAnalyzersto the ten shipping SDK libraries with reviewedPublicAPI.Unshipped.txtdeclarations.PublicAPI.Shipped.txtstays empty until the surface is accepted.RS0026/RS0027are suppressed only at specific pre-existing overload families, with a justification at each site, so new violations still fail the build.This already earned its keep: rebasing onto the flat device model work surfaced seven stale and three missing Core declarations that the analyzer caught at build time.
Behavior
No transport fallback, secure-channel, session ownership, or security lifecycle semantics were changed — with one deliberate exception.
Security Domain CA identifier decoding was genuinely broken. The key reference arrives as TLV
83 02 KID KVN. The old code read the whole TLV rather than its value, so on real hardware every entry came back asKid=0x83, Kvn=0x02— the tag and length bytes. The existing integration test asserted onlyresult.Count > 0, which passes under both the broken and the fixed decoding, so it could never have caught this.This PR adds
GetCaIdentifiersAsync_DecodesKeyReferenceValue, which pins the key identifier to the SCP11 set. It was verified by reverting the fix and confirming it fails against hardware:Verification
Hardware integration
Run against authorized test devices (serials 31683481 / 125 / 20260533 / 103, firmware 5.4.3-5.8.0):
All eight suites were run back to back in the order that previously poisoned device state. Touch and user-presence lanes were not run.
Two pre-existing integration defects, now fixed
Both were confirmed reproducible on
origin/yubikitbefore being fixed, so neither was introduced here. They are in a separate commit (test(securitydomain,yubihsm)) since they are unrelated to the API consolidation.Security Domain left the device unusable by later suites. Tests that import, rotate, or delete SCP03 key sets ended with no key set matching
Scp03KeyParameters.Default, so any later consumer of the default keys failed withSW=0x6A88. The symptom surfaced in whichever suite ran next rather than in the test that caused it — Management's SCP03 tests failed only when the Security Domain suite had run first, which reads as an unrelated regression. I misdiagnosed it twice before bisecting by test class, which identified the SCP03 key tests and both SCP11 classes.The three SCP03 key tests now restore the factory Security Domain in a
finally. The SCP11 classes derive from a newSecurityDomainStateRestoringTestsbase that restores after every test, since xUnit constructs the class once per test.YubiHSM Auth had no byte-level coverage of the change-password instruction (
FeaturePasswordChange, 5.8.0, INS0x0B). Probing every connected device:A development applet reporting
0.0.1runs the whole flow successfully. A different development build of the same reported version omits the instruction. The instruction byte, both P1 selectors, TLV order, and the 5.8.0 gate were all verified against canonical Rust and are correct, so the SDK is unchanged.This is deliberately not worked around in code — a key whose applet omits the instruction should fail loudly. Two byte-level unit tests now pin the instruction byte, both P1 selectors, and TLV order, so a genuine encoding regression fails without hardware and is distinguishable from a deficient applet build.
The 5.8.0 gate was verified on hardware, not just against source
FeaturePasswordChangeis gated at 5.8.0. Canonical Rust gates at the same version; canonical Python has no gate inhsmauth.pyand delegates to that Rust layer, and its device test uses@condition.min_version(5, 8).Source agreement is not proof, and our own gate meant older keys never sent the command. Overriding the version so the device answers for itself:
A real 5.7.4 applet does not implement INS
0x0B, so the gate is correct and unchanged.Firmware override survived only in OATH
That experiment exposed a separate bug. Reset re-SELECTs the applet, and YubiHSM Auth and OpenPGP adopted the version from that response, discarding a
FirmwareVersionOverridesupplied at creation. The override lapsed mid-session, so feature gates after a reset disagreed with those before it, with nothing in the API signalling the change. OATH already treated reset as refreshing applet state rather than caller policy; both applets now match.This is relevant to the deferred VER decision: the override is not merely cosmetic, and its lifetime is part of the contract being deferred.
Alpha/beta firmware semantics
An alpha or beta key reports major version 0 and is at least 5.8.0, so every feature gate must accept it.
FirmwareVersion.IsAtLeastandFeature.IsSupportedByFirmwarealready did — butPivSession.GetConservativeDefaultManagementKeyTypecancelled that with an explicit!IsAlphaOrBeta &&beforeIsAtLeast(5, 7, 0):A development key defaults to AES-192, so the old fallback picked an algorithm the key does not use whenever management key metadata was unavailable — on both the create and reset paths. Three tests pinned the old behaviour and now assert AES-192; two were added so pre-5.7 firmware keeps explicit Triple-DES coverage, which the old assertions had conflated with the development-version case.
FirmwareVersion.IsAlphaOrBetais now documented with the rule (at least 5.8.0, newer than any release), the mistake to avoid (!IsAlphaOrBeta && IsAtLeast(...)), and the caveat that the placeholder carries no build detail — so an applet can lack a feature the version implies and answerSW=0x6D00. That is left to surface as a test failure, with the comment there so it is recognised quickly as a deficient applet rather than a defective gate.Migration
docs/migration/v1-to-v2.mdgains an alpha.1 → alpha.2 section with a mechanical old→new table covering all breaking changes plus the seven dependency-injection removals. Worth calling out:GetCaIdentifiersAsyncchangingArgumentException→ArgumentOutOfRangeExceptionis silent at compile time.Deferred (recorded in
docs/architecture/applet-public-api.md)All due before
PublicAPI.Shipped.txtis populated:IYubiKeyconvenience taxonomy. Existing wrappers were adapted but not expanded; 2 of 8 acceptSessionCreationOptionsbecause they previously tookScpKeyParameters.FirmwareVersionOverridebelongs on the public surface. Note Security Domain cannot detect firmware, so for that applet it is the only version source rather than an override.SlotConfigurationprotectedbyte[]fields,ApplicationIdsstaticbyte[]) — recorded so declaring them does not bless them.src/PublicApi/(test-only, no shipping assembly) and forwardingoptionsthrough the eight device extensions instead of rebuilding it.Review notes
This went through two cross-vendor engineer/reviewer loops and a Craftsman fit audit. The first review returned FAIL on three blockers (the analyzer suppression being a sham guard, a 12%-complete migration guide, and
CredentialData.RequireTouchremoving a working capability) — all fixed here.The reviewer's one remaining structural objection stands and is worth your view: at ~180 files this is large for one PR, and they proposed an 8-slice stack. It was kept as one commit because the PublicAPI analyzer couples the slices — declarations must match the emitted surface or the build breaks — so a clean split would need the analyzer added last. Happy to restructure if you'd rather review it in slices.