From c197f25437c8e5102316347d604f34b595c38720 Mon Sep 17 00:00:00 2001 From: Dennis Dyallo Date: Mon, 15 Jun 2026 20:47:58 +0200 Subject: [PATCH 01/25] test(perf): add YubiKit performance benchmarks --- Directory.Packages.props | 3 +- .../Program.cs | 241 ++++++++++++++++++ ...ubico.YubiKit.PerformanceBenchmarks.csproj | 23 ++ 3 files changed, 266 insertions(+), 1 deletion(-) create mode 100644 benchmarks/Yubico.YubiKit.PerformanceBenchmarks/Program.cs create mode 100644 benchmarks/Yubico.YubiKit.PerformanceBenchmarks/Yubico.YubiKit.PerformanceBenchmarks.csproj diff --git a/Directory.Packages.props b/Directory.Packages.props index 46740156f..57e558e1e 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -39,9 +39,10 @@ + - \ No newline at end of file + diff --git a/benchmarks/Yubico.YubiKit.PerformanceBenchmarks/Program.cs b/benchmarks/Yubico.YubiKit.PerformanceBenchmarks/Program.cs new file mode 100644 index 000000000..ee69aa93f --- /dev/null +++ b/benchmarks/Yubico.YubiKit.PerformanceBenchmarks/Program.cs @@ -0,0 +1,241 @@ +using BenchmarkDotNet.Attributes; +using BenchmarkDotNet.Columns; +using BenchmarkDotNet.Configs; +using BenchmarkDotNet.Diagnosers; +using BenchmarkDotNet.Environments; +using BenchmarkDotNet.Exporters; +using BenchmarkDotNet.Exporters.Csv; +using BenchmarkDotNet.Exporters.Json; +using BenchmarkDotNet.Jobs; +using BenchmarkDotNet.Reports; +using BenchmarkDotNet.Running; +using BenchmarkDotNet.Validators; +using Yubico.YubiKit.Core.Abstractions; +using Yubico.YubiKit.Core.Devices; +using Yubico.YubiKit.Core.Protocols.Fido.Hid; +using Yubico.YubiKit.Core.Protocols.SmartCard.Apdu; +using Yubico.YubiKit.Core.Sessions; +using Yubico.YubiKit.Core.Transports.Hid; +using Yubico.YubiKit.Core.Transports.SmartCard; +using Yubico.YubiKit.Management; + +var config = YubiKitBenchmarkConfig.Create(); +BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args, config); + +internal sealed class YubiKitBenchmarkConfig : ManualConfig +{ + private YubiKitBenchmarkConfig() + { + var runId = DateTimeOffset.UtcNow.ToString("yyyyMMdd-HHmmss"); + ArtifactsPath = Path.Combine(FindRepoRoot(), "artifacts", "benchmarks", runId); + + AddJob(Job.ShortRun + .WithRuntime(CoreRuntime.Core10_0) + .WithId("net10-short")); + + AddColumnProvider(DefaultColumnProviders.Instance); + AddDiagnoser(MemoryDiagnoser.Default); + AddDiagnoser(ThreadingDiagnoser.Default); + AddDiagnoser(ExceptionDiagnoser.Default); + AddDiagnoser(new EventPipeProfiler(EventPipeProfile.CpuSampling)); + AddExporter(MarkdownExporter.GitHub); + AddExporter(HtmlExporter.Default); + AddExporter(CsvExporter.Default); + AddExporter(CsvMeasurementsExporter.Default); + AddExporter(JsonExporter.Full); + AddExporter(RPlotExporter.Default); + AddValidator(JitOptimizationsValidator.FailOnError); + AddLogger(DefaultConfig.Instance.GetLoggers().ToArray()); + } + + public static IConfig Create() => new YubiKitBenchmarkConfig(); + + private static string FindRepoRoot() + { + var current = AppContext.BaseDirectory; + while (!string.IsNullOrEmpty(current)) + { + if (Directory.Exists(Path.Combine(current, ".git"))) + return current; + + current = Directory.GetParent(current)?.FullName; + } + + return Directory.GetCurrentDirectory(); + } +} + +public abstract class YubiKeyHardwareBenchmarkBase +{ + protected IYubiKey Device { get; private set; } = null!; + + protected void SetupDevice(ConnectionType requiredConnection) + { + Device = FindDevice(requiredConnection).GetAwaiter().GetResult(); + } + + [GlobalCleanup] + public void Cleanup() => YubiKeyManager.Shutdown(); + + private static async Task FindDevice(ConnectionType requiredConnection) + { + var devices = await YubiKeyManager.FindAllAsync(ConnectionType.All, forceRescan: true) + .ConfigureAwait(false); + + return devices.FirstOrDefault(device => device.SupportsConnection(requiredConnection)) + ?? throw new InvalidOperationException( + $"No connected YubiKey exposes {requiredConnection}. Connect the YubiKey 5.8 device before running benchmarks."); + } +} + +[MemoryDiagnoser] +[ThreadingDiagnoser] +[ExceptionDiagnoser] +public class YubiKeyDiscoveryBenchmarks +{ + [GlobalSetup] + public void Setup() + { + _ = YubiKeyManager.FindAllAsync(ConnectionType.All, forceRescan: true) + .GetAwaiter() + .GetResult(); + } + + [GlobalCleanup] + public void Cleanup() => YubiKeyManager.Shutdown(); + + [Benchmark(Baseline = true)] + public async Task CachedFindAll() + { + var devices = await YubiKeyManager.FindAllAsync(ConnectionType.All, forceRescan: false) + .ConfigureAwait(false); + return devices.Count; + } + + [Benchmark] + public async Task ForcedRescanFindAll() + { + var devices = await YubiKeyManager.FindAllAsync(ConnectionType.All, forceRescan: true) + .ConfigureAwait(false); + return devices.Count; + } + + [Benchmark] + public async Task ColdFindAll() + { + await YubiKeyManager.ShutdownAsync().ConfigureAwait(false); + var devices = await YubiKeyManager.FindAllAsync(ConnectionType.All, forceRescan: false) + .ConfigureAwait(false); + return devices.Count; + } +} + +[MemoryDiagnoser] +[ThreadingDiagnoser] +[ExceptionDiagnoser] +public class SmartCardManagementBenchmarks : YubiKeyHardwareBenchmarkBase +{ + [GlobalSetup] + public void Setup() => SetupDevice(ConnectionType.SmartCard); + + [Benchmark(Baseline = true)] + public async Task ConnectSmartCard() + { + await using var connection = await Device.ConnectAsync().ConfigureAwait(false); + return connection.SupportsExtendedApdu(); + } + + [Benchmark] + public async Task SelectManagementOverSmartCard() + { + var connection = await Device.ConnectAsync().ConfigureAwait(false); + using var protocol = PcscProtocolFactory.Create().Create(connection); + var response = await protocol.SelectAsync(ApplicationIds.Management).ConfigureAwait(false); + return response.Length; + } + + [Benchmark] + public async Task GetDeviceInfoOverSmartCard() + { + await using var session = await Device.CreateManagementSessionAsync( + preferredConnection: ConnectionType.SmartCard) + .ConfigureAwait(false); + + var info = await session.GetDeviceInfoAsync().ConfigureAwait(false); + return info.SerialNumber ?? 0; + } +} + +[MemoryDiagnoser] +[ThreadingDiagnoser] +[ExceptionDiagnoser] +public class FidoHidManagementBenchmarks : YubiKeyHardwareBenchmarkBase +{ + [GlobalSetup] + public void Setup() => SetupDevice(ConnectionType.HidFido); + + [Benchmark(Baseline = true)] + public async Task ConnectFidoHid() + { + await using var connection = await Device.ConnectAsync().ConfigureAwait(false); + return connection.PacketSize; + } + + [Benchmark] + public async Task CreateManagementSessionOverFidoHid() + { + await using var session = await Device.CreateManagementSessionAsync( + preferredConnection: ConnectionType.HidFido) + .ConfigureAwait(false); + + return session.FirmwareVersion.Major; + } + + [Benchmark] + public async Task GetDeviceInfoOverFidoHid() + { + await using var session = await Device.CreateManagementSessionAsync( + preferredConnection: ConnectionType.HidFido) + .ConfigureAwait(false); + + var info = await session.GetDeviceInfoAsync().ConfigureAwait(false); + return info.SerialNumber ?? 0; + } +} + +[MemoryDiagnoser] +[ThreadingDiagnoser] +[ExceptionDiagnoser] +public class OtpHidManagementBenchmarks : YubiKeyHardwareBenchmarkBase +{ + [GlobalSetup] + public void Setup() => SetupDevice(ConnectionType.HidOtp); + + [Benchmark(Baseline = true)] + public async Task ConnectOtpHid() + { + await using var connection = await Device.ConnectAsync().ConfigureAwait(false); + return connection.FeatureReportSize; + } + + [Benchmark] + public async Task CreateManagementSessionOverOtpHid() + { + await using var session = await Device.CreateManagementSessionAsync( + preferredConnection: ConnectionType.HidOtp) + .ConfigureAwait(false); + + return session.FirmwareVersion.Major; + } + + [Benchmark] + public async Task GetDeviceInfoOverOtpHid() + { + await using var session = await Device.CreateManagementSessionAsync( + preferredConnection: ConnectionType.HidOtp) + .ConfigureAwait(false); + + var info = await session.GetDeviceInfoAsync().ConfigureAwait(false); + return info.SerialNumber ?? 0; + } +} diff --git a/benchmarks/Yubico.YubiKit.PerformanceBenchmarks/Yubico.YubiKit.PerformanceBenchmarks.csproj b/benchmarks/Yubico.YubiKit.PerformanceBenchmarks/Yubico.YubiKit.PerformanceBenchmarks.csproj new file mode 100644 index 000000000..aa1de7ce1 --- /dev/null +++ b/benchmarks/Yubico.YubiKit.PerformanceBenchmarks/Yubico.YubiKit.PerformanceBenchmarks.csproj @@ -0,0 +1,23 @@ + + + + Exe + net10.0 + Yubico.YubiKit.PerformanceBenchmarks + Yubico.YubiKit.PerformanceBenchmarks + false + AnyCPU + pdbonly + true + + + + + + + + + + + + From 4b0e958b04010c3f505cbe1123782e35660ccd2b Mon Sep 17 00:00:00 2001 From: Dennis Dyallo Date: Tue, 16 Jun 2026 11:04:45 +0200 Subject: [PATCH 02/25] perf(otp): reduce HID write polling latency --- src/Core/src/Protocols/Otp/Hid/OtpHidProtocol.cs | 13 +++++++------ .../Protocols/Otp/Hid/OtpHidProtocolTests.cs | 4 ++++ 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/Core/src/Protocols/Otp/Hid/OtpHidProtocol.cs b/src/Core/src/Protocols/Otp/Hid/OtpHidProtocol.cs index 1352965d7..dd46eac5b 100644 --- a/src/Core/src/Protocols/Otp/Hid/OtpHidProtocol.cs +++ b/src/Core/src/Protocols/Otp/Hid/OtpHidProtocol.cs @@ -335,21 +335,22 @@ private async Task WriteFeatureReportAsync(ReadOnlyMemory buffer, Cancella /// /// Waits for the WRITE flag to be cleared (device ready to receive). - /// Uses "sleep-first" pattern: delay BEFORE checking the flag. - /// This gives the device time to process each frame before we poll. /// private async Task AwaitReadyToWriteAsync(CancellationToken cancellationToken) { - for (var i = 0; i < 20; i++) - { - // Sleep first - give device time to clear write flag - await Task.Delay(50, cancellationToken).ConfigureAwait(false); + var stopwatch = System.Diagnostics.Stopwatch.StartNew(); + const int timeLimitMs = 1000; + const int pollIntervalMs = 5; + while (stopwatch.ElapsedMilliseconds < timeLimitMs) + { var report = await ReadFeatureReportAsync(cancellationToken).ConfigureAwait(false); if ((report.Span[OtpConstants.FeatureReportDataSize] & OtpConstants.SlotWriteFlag) == 0) { return; } + + await Task.Delay(pollIntervalMs, cancellationToken).ConfigureAwait(false); } throw new TimeoutException("Timeout waiting for YubiKey to become ready to receive"); diff --git a/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Protocols/Otp/Hid/OtpHidProtocolTests.cs b/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Protocols/Otp/Hid/OtpHidProtocolTests.cs index 373d93950..e0f422fa4 100644 --- a/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Protocols/Otp/Hid/OtpHidProtocolTests.cs +++ b/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Protocols/Otp/Hid/OtpHidProtocolTests.cs @@ -1,6 +1,7 @@ // Copyright 2025 Yubico AB // Licensed under the Apache License, Version 2.0 (the "License"). +using System.Diagnostics; using Yubico.YubiKit.Core.Devices; using Yubico.YubiKit.Core.Protocols.Otp.Hid; using Yubico.YubiKit.Core.Transports.Hid; @@ -89,9 +90,12 @@ public async Task SendAndReceiveAsync_EmptyPayload_Succeeds() // Response: sequence incremented (no data response) mock.QueueReport([0x00, 0x05, 0x04, 0x03, 0x02, 0x00, 0x00, 0x00]); // progSeq=2 + var stopwatch = Stopwatch.StartNew(); var result = await protocol.SendAndReceiveAsync(0x13, ReadOnlyMemory.Empty, TestContext.Current.CancellationToken); + stopwatch.Stop(); Assert.Equal(6, result.Length); // Status-only response returns 6 status bytes + Assert.True(stopwatch.ElapsedMilliseconds < 200, $"Ready-to-write polling took {stopwatch.ElapsedMilliseconds}ms"); } [Fact] From 0a741bd81b2d93d48c17dea7c4b57603f0c13c45 Mon Sep 17 00:00:00 2001 From: Dennis Dyallo Date: Tue, 16 Jun 2026 17:10:23 +0200 Subject: [PATCH 03/25] fix(core): harden SmartCard listener recovery --- .../DesktopSmartCardDeviceListener.cs | 146 ++++++++++-- .../src/Transports/SmartCard/ISCardApi.cs | 56 +++++ ...pSmartCardDeviceListenerSCardErrorTests.cs | 222 ++++++++++++++++++ 3 files changed, 405 insertions(+), 19 deletions(-) create mode 100644 src/Core/src/Transports/SmartCard/ISCardApi.cs create mode 100644 src/Core/tests/Yubico.YubiKit.Core.UnitTests/Transports/SmartCard/DesktopSmartCardDeviceListenerSCardErrorTests.cs diff --git a/src/Core/src/Transports/SmartCard/DesktopSmartCardDeviceListener.cs b/src/Core/src/Transports/SmartCard/DesktopSmartCardDeviceListener.cs index 038f38864..0c71104bd 100644 --- a/src/Core/src/Transports/SmartCard/DesktopSmartCardDeviceListener.cs +++ b/src/Core/src/Transports/SmartCard/DesktopSmartCardDeviceListener.cs @@ -38,6 +38,8 @@ public sealed class DesktopSmartCardDeviceListener : ISmartCardDeviceListener private static readonly ILogger Logger = YubiKitLogging.CreateLogger(); + private readonly ISCardApi _sCardApi; + private readonly Action _sleep; private readonly Lock _syncLock = new(); private SCardContext? _context; private Thread? _listenerThread; @@ -55,11 +57,20 @@ public sealed class DesktopSmartCardDeviceListener : ISmartCardDeviceListener /// Creates a new instance. The listener does not start automatically - call /// after setting up the callback. /// - public DesktopSmartCardDeviceListener() + public DesktopSmartCardDeviceListener() : this(NativeSCardApi.Instance, Thread.Sleep) { // Lazy start - do nothing in constructor } + internal DesktopSmartCardDeviceListener(ISCardApi sCardApi, Action sleep) + { + ArgumentNullException.ThrowIfNull(sCardApi); + ArgumentNullException.ThrowIfNull(sleep); + + _sCardApi = sCardApi; + _sleep = sleep; + } + /// public void Start() { @@ -72,7 +83,7 @@ public void Start() try { - var result = NativeMethods.SCardEstablishContext(SCARD_SCOPE.USER, out var context); + var result = _sCardApi.SCardEstablishContext(SCARD_SCOPE.USER, out var context); if (result != ErrorCode.SCARD_S_SUCCESS) { Logger.LogWarning("Failed to establish SCard context: 0x{Result:X8}", result); @@ -112,8 +123,12 @@ public void Stop() { return; } + } + + StopListening(); - StopListening(); + lock (_syncLock) + { _knownReaders = null; Status = DeviceListenerStatus.Stopped; } @@ -127,7 +142,7 @@ private void EstablishBaseline() return; } - var result = NativeMethods.SCardListReaders(_context, null, out var currentReaders); + var result = _sCardApi.SCardListReaders(_context, null, out var currentReaders); if (result == ErrorCode.SCARD_E_NO_READERS_AVAILABLE) { @@ -149,22 +164,29 @@ private void StopListening() { _shouldStop = true; + SCardContext? context; + Thread? listenerThread; + lock (_syncLock) + { + context = _context; + listenerThread = _listenerThread; + _listenerThread = null; + } + // Signal the SCard context to cancel any blocking calls - if (_context is { IsInvalid: false }) + if (context is { IsInvalid: false }) { - _ = NativeMethods.SCardCancel(_context); + _ = _sCardApi.SCardCancel(context); } // Wait for the listener thread to exit - if (_listenerThread is not null && _listenerThread.IsAlive) + if (listenerThread is not null && listenerThread.IsAlive) { - if (!_listenerThread.Join(MaxDisposalWaitTime)) + if (!listenerThread.Join(MaxDisposalWaitTime)) { Logger.LogWarning("SmartCard listener thread did not exit within timeout"); } } - - _listenerThread = null; } private void ListenerThreadProc() @@ -179,7 +201,7 @@ private void ListenerThreadProc() } // Get current list of readers - var result = NativeMethods.SCardListReaders(_context, null, out var currentReaders); + var result = _sCardApi.SCardListReaders(_context, null, out var currentReaders); if (result == ErrorCode.SCARD_E_NO_READERS_AVAILABLE) { @@ -192,15 +214,11 @@ private void ListenerThreadProc() if (result != ErrorCode.SCARD_S_SUCCESS) { - if (result == ErrorCode.SCARD_E_CANCELLED || - result == ErrorCode.SCARD_E_SERVICE_STOPPED || - result == ErrorCode.SCARD_E_NO_SERVICE) + if (!HandleSCardFailure(result, "SCardListReaders")) { break; } - Logger.LogWarning("SCardListReaders failed: 0x{Result:X8}", result); - Thread.Sleep(CheckForChangesWaitTime); continue; } @@ -248,12 +266,17 @@ private void WaitForPnpChange() var pnpState = SCARD_READER_STATE.Create(PnpNotificationReaderName); var states = new[] { pnpState }; - _ = NativeMethods.SCardGetStatusChange( + var result = _sCardApi.SCardGetStatusChange( _context, (int)CheckForChangesWaitTime.TotalMilliseconds, states, states.Length); + if (result != ErrorCode.SCARD_S_SUCCESS && !_shouldStop) + { + _ = HandleSCardFailure(result, "SCardGetStatusChange"); + } + // Free the allocated string if (pnpState.ReaderName != IntPtr.Zero) { @@ -265,7 +288,7 @@ private void WaitForStatusChange(string[] readers) { if (_context is null || _context.IsInvalid || _shouldStop || readers.Length == 0) { - Thread.Sleep(CheckForChangesWaitTime); + _sleep(CheckForChangesWaitTime); return; } @@ -279,11 +302,16 @@ private void WaitForStatusChange(string[] readers) try { - _ = NativeMethods.SCardGetStatusChange( + var result = _sCardApi.SCardGetStatusChange( _context, (int)CheckForChangesWaitTime.TotalMilliseconds, allStates, allStates.Length); + + if (result != ErrorCode.SCARD_S_SUCCESS && !_shouldStop) + { + _ = HandleSCardFailure(result, "SCardGetStatusChange"); + } } finally { @@ -298,6 +326,86 @@ private void WaitForStatusChange(string[] readers) } } + private bool HandleSCardFailure(uint result, string operation) + { + if (_shouldStop || result == ErrorCode.SCARD_E_CANCELLED) + { + return false; + } + + Logger.LogWarning("{Operation} failed: 0x{Result:X8}", operation, result); + _sleep(CheckForChangesWaitTime); + + if (_shouldStop) + { + return false; + } + + if (IsRecoverableContextError(result)) + { + return TryReestablishContext(operation, result); + } + + return true; + } + + private bool TryReestablishContext(string operation, uint result) + { + SCardContext? oldContext; + lock (_syncLock) + { + oldContext = _context; + _context = null; + } + + try + { + oldContext?.Dispose(); + } + catch (Exception ex) + { + Logger.LogDebug( + ex, + "Failed to dispose stale SCard context after {Operation} returned 0x{Result:X8}", + operation, + result); + } + + var establishResult = _sCardApi.SCardEstablishContext(SCARD_SCOPE.USER, out var newContext); + if (establishResult != ErrorCode.SCARD_S_SUCCESS) + { + Logger.LogWarning( + "Failed to re-establish SCard context after {Operation} returned 0x{Result:X8}: 0x{EstablishResult:X8}", + operation, + result, + establishResult); + Status = DeviceListenerStatus.Error; + return false; + } + + lock (_syncLock) + { + if (_shouldStop) + { + newContext.Dispose(); + return false; + } + + _context = newContext; + Status = DeviceListenerStatus.Started; + } + + EstablishBaseline(); + return true; + } + + private static bool IsRecoverableContextError(uint result) => + result is ErrorCode.SCARD_E_INVALID_HANDLE + or ErrorCode.SCARD_E_SYSTEM_CANCELLED + or ErrorCode.ERROR_BROKEN_PIPE + or ErrorCode.SCARD_E_SERVICE_STOPPED + or ErrorCode.SCARD_E_NO_SERVICE; + private void OnDeviceEvent() { try diff --git a/src/Core/src/Transports/SmartCard/ISCardApi.cs b/src/Core/src/Transports/SmartCard/ISCardApi.cs new file mode 100644 index 000000000..a25113556 --- /dev/null +++ b/src/Core/src/Transports/SmartCard/ISCardApi.cs @@ -0,0 +1,56 @@ +// Copyright 2026 Yubico AB +// +// Licensed under the Apache License, Version 2.0 (the "License"). +// You may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using Yubico.YubiKit.Core.Native.Desktop.SCard; + +namespace Yubico.YubiKit.Core.Transports.SmartCard; + +internal interface ISCardApi +{ + uint SCardEstablishContext(SCARD_SCOPE scope, out SCardContext context); + + uint SCardListReaders(SCardContext context, string[]? groups, out string[] readerNames); + + uint SCardGetStatusChange( + SCardContext context, + int timeout, + SCARD_READER_STATE[] readerStates, + int readerStatesCount); + + uint SCardCancel(SCardContext context); +} + +internal sealed class NativeSCardApi : ISCardApi +{ + public static NativeSCardApi Instance { get; } = new(); + + private NativeSCardApi() + { + } + + public uint SCardEstablishContext(SCARD_SCOPE scope, out SCardContext context) => + NativeMethods.SCardEstablishContext(scope, out context); + + public uint SCardListReaders(SCardContext context, string[]? groups, out string[] readerNames) => + NativeMethods.SCardListReaders(context, groups, out readerNames); + + public uint SCardGetStatusChange( + SCardContext context, + int timeout, + SCARD_READER_STATE[] readerStates, + int readerStatesCount) => + NativeMethods.SCardGetStatusChange(context, timeout, readerStates, readerStatesCount); + + public uint SCardCancel(SCardContext context) => NativeMethods.SCardCancel(context); +} \ No newline at end of file diff --git a/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Transports/SmartCard/DesktopSmartCardDeviceListenerSCardErrorTests.cs b/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Transports/SmartCard/DesktopSmartCardDeviceListenerSCardErrorTests.cs new file mode 100644 index 000000000..1be70d5af --- /dev/null +++ b/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Transports/SmartCard/DesktopSmartCardDeviceListenerSCardErrorTests.cs @@ -0,0 +1,222 @@ +// Copyright 2026 Yubico AB +// +// Licensed under the Apache License, Version 2.0 (the "License"). +// You may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using Yubico.YubiKit.Core.Native.Desktop.SCard; +using Yubico.YubiKit.Core.Transports.SmartCard; + +namespace Yubico.YubiKit.Core.UnitTests.Transports.SmartCard; + +/// +/// No-hardware regression tests for listener retry storms. Persistent native failures must be +/// backoff-bounded before retry or recovery, so these tests intentionally use fakes and must not +/// skip when PC/SC or YubiKey hardware is unavailable. +/// +[Trait("Category", "RuntimeResilience")] +public sealed class DesktopSmartCardDeviceListenerSCardErrorTests +{ + [Fact] + public void WhenGetStatusChangeReturnsInvalidHandle_ListenerBacksOffBeforeRetrying() + { + var interop = new FakeSCardApi + { + ListReaders = _ => (ErrorCode.SCARD_S_SUCCESS, ["Reader A"]), + GetStatusChange = _ => ErrorCode.SCARD_E_INVALID_HANDLE + }; + using var sleeper = new BlockingSleeper(); + using var listener = new DesktopSmartCardDeviceListener(interop, sleeper.Sleep); + + listener.Start(); + + Assert.True(sleeper.WaitUntilSleeping(), "Listener should back off after an invalid-handle status change."); + Assert.Equal(1, interop.GetStatusChangeCalls); + Assert.Equal(1, sleeper.SleepCalls); + sleeper.Release(); + } + + [Fact] + public void WhenListReadersReturnsInvalidHandle_ListenerBacksOffBeforeRetrying() + { + var firstListReaders = true; + var interop = new FakeSCardApi + { + ListReaders = _ => + { + if (firstListReaders) + { + firstListReaders = false; + return (ErrorCode.SCARD_S_SUCCESS, ["Reader A"]); + } + + return (ErrorCode.SCARD_E_INVALID_HANDLE, []); + }, + GetStatusChange = _ => ErrorCode.SCARD_S_SUCCESS + }; + using var sleeper = new BlockingSleeper(); + using var listener = new DesktopSmartCardDeviceListener(interop, sleeper.Sleep); + + listener.Start(); + + Assert.True(sleeper.WaitUntilSleeping(), "Listener should back off after an invalid-handle reader list."); + Assert.Equal(2, interop.ListReadersCalls); + Assert.Equal(1, sleeper.SleepCalls); + sleeper.Release(); + } + + [Fact] + public void WhenContextReestablishmentFails_ListenerStopsAfterSingleBackoff() + { + var interop = new FakeSCardApi + { + EstablishContext = call => call == 1 ? ErrorCode.SCARD_S_SUCCESS : ErrorCode.SCARD_E_NO_SERVICE, + ListReaders = _ => (ErrorCode.SCARD_S_SUCCESS, ["Reader A"]), + GetStatusChange = _ => ErrorCode.SCARD_E_INVALID_HANDLE + }; + using var sleeper = new BlockingSleeper(); + using var listener = new DesktopSmartCardDeviceListener(interop, sleeper.Sleep); + + listener.Start(); + + Assert.True(sleeper.WaitUntilSleeping(), "Listener should back off before trying to re-establish context."); + Assert.Equal(1, interop.EstablishContextCalls); + sleeper.Release(); + + Assert.True( + SpinWait.SpinUntil(() => listener.Status == DeviceListenerStatus.Error, TimeSpan.FromSeconds(5)), + "Listener should enter Error status when context re-establishment fails."); + Assert.Equal(2, interop.EstablishContextCalls); + Assert.Equal(1, sleeper.SleepCalls); + } + + [Fact] + public void WhenContextReestablishmentSucceeds_ListenerRebuildsBaselineAndResumes() + { + var resumedStatusWait = new ManualResetEventSlim(); + var releaseResumedStatusWait = new ManualResetEventSlim(); + var getStatusResults = 0; + var interop = new FakeSCardApi + { + ListReaders = _ => (ErrorCode.SCARD_S_SUCCESS, ["Reader A"]), + GetStatusChange = _ => + { + if (Interlocked.Increment(ref getStatusResults) == 1) + { + return ErrorCode.SCARD_E_INVALID_HANDLE; + } + + resumedStatusWait.Set(); + releaseResumedStatusWait.Wait(TimeSpan.FromSeconds(5)); + return ErrorCode.SCARD_S_SUCCESS; + } + }; + using var sleeper = new BlockingSleeper(); + using var listener = new DesktopSmartCardDeviceListener(interop, sleeper.Sleep); + + try + { + listener.Start(); + + Assert.True(sleeper.WaitUntilSleeping(), "Listener should back off before re-establishing context."); + Assert.Equal(1, interop.EstablishContextCalls); + + sleeper.Release(); + + Assert.True( + resumedStatusWait.Wait(TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken), + "Listener should resume status monitoring after context recovery."); + Assert.Equal(DeviceListenerStatus.Started, listener.Status); + Assert.Equal(2, interop.EstablishContextCalls); + Assert.True(interop.ListReadersCalls >= 4, "Recovery should rebuild the baseline before resuming the listener loop."); + Assert.Equal(1, sleeper.SleepCalls); + } + finally + { + releaseResumedStatusWait.Set(); + resumedStatusWait.Dispose(); + releaseResumedStatusWait.Dispose(); + } + } + + private sealed class BlockingSleeper : IDisposable + { + private readonly ManualResetEventSlim _sleeping = new(); + private readonly ManualResetEventSlim _release = new(); + + public int SleepCalls { get; private set; } + + public void Sleep(TimeSpan _) + { + SleepCalls++; + _sleeping.Set(); + _release.Wait(TimeSpan.FromSeconds(5)); + } + + public bool WaitUntilSleeping() => _sleeping.Wait(TimeSpan.FromSeconds(5)); + + public void Release() => _release.Set(); + + public void Dispose() + { + _release.Set(); + _sleeping.Dispose(); + _release.Dispose(); + } + } + + private sealed class FakeSCardApi : ISCardApi + { + public Func EstablishContext { get; set; } = _ => ErrorCode.SCARD_S_SUCCESS; + + public Func ListReaders { get; set; } = + _ => (ErrorCode.SCARD_S_SUCCESS, []); + + public Func GetStatusChange { get; set; } = + _ => ErrorCode.SCARD_S_SUCCESS; + + public int EstablishContextCalls { get; private set; } + public int GetStatusChangeCalls { get; private set; } + public int ListReadersCalls { get; private set; } + + public uint SCardEstablishContext(SCARD_SCOPE scope, out SCardContext context) + { + EstablishContextCalls++; + context = new TestSCardContext(new IntPtr(EstablishContextCalls)); + return EstablishContext(EstablishContextCalls); + } + + public uint SCardListReaders(SCardContext context, string[]? groups, out string[] readerNames) + { + ListReadersCalls++; + var result = ListReaders(context); + readerNames = result.Readers; + return result.Result; + } + + public uint SCardGetStatusChange( + SCardContext context, + int timeout, + SCARD_READER_STATE[] readerStates, + int readerStatesCount) + { + GetStatusChangeCalls++; + return GetStatusChange(readerStates); + } + + public uint SCardCancel(SCardContext context) => ErrorCode.SCARD_S_SUCCESS; + + private sealed class TestSCardContext(IntPtr handle) : SCardContext(handle) + { + protected override bool ReleaseHandle() => true; + } + } +} \ No newline at end of file From 10c2c0419396e550e0eb6eee2ca28c986f89c781 Mon Sep 17 00:00:00 2001 From: Dennis Dyallo Date: Tue, 16 Jun 2026 17:10:43 +0200 Subject: [PATCH 04/25] test(core): add OTP polling resilience guard --- .../Protocols/Otp/Hid/OtpHidProtocolTests.cs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Protocols/Otp/Hid/OtpHidProtocolTests.cs b/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Protocols/Otp/Hid/OtpHidProtocolTests.cs index e0f422fa4..c0f17263f 100644 --- a/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Protocols/Otp/Hid/OtpHidProtocolTests.cs +++ b/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Protocols/Otp/Hid/OtpHidProtocolTests.cs @@ -9,7 +9,7 @@ namespace Yubico.YubiKit.Core.UnitTests.Protocols.Otp.Hid; /// -/// Unit tests for OtpHidProtocol to verify behavior before refactoring. +/// Unit tests for OtpHidProtocol behavior and no-hardware runtime resilience invariants. /// public class OtpHidProtocolTests { @@ -71,7 +71,8 @@ await Assert.ThrowsAsync( } [Fact] - public async Task SendAndReceiveAsync_EmptyPayload_Succeeds() + [Trait("Category", "RuntimeResilience")] + public async Task SendAndReceiveAsync_WhenReadyToWriteImmediately_DoesNotSleepBeforePolling() { var mock = new MockHidConnection(); var protocol = CreateProtocolWithMock(mock); @@ -95,6 +96,11 @@ public async Task SendAndReceiveAsync_EmptyPayload_Succeeds() stopwatch.Stop(); Assert.Equal(6, result.Length); // Status-only response returns 6 status bytes + Assert.Equal(10, mock.SentReports.Count); + + // This fake path is intentionally no-hardware and immediately write-ready. The old + // sleep-first loop added at least 10 x 50ms before these writes, so a loose 200ms budget + // catches that regression without relying on BenchmarkDotNet in normal unit runs. Assert.True(stopwatch.ElapsedMilliseconds < 200, $"Ready-to-write polling took {stopwatch.ElapsedMilliseconds}ms"); } From eb2ac820a6684a4d429bd47f3d90da347d458aba Mon Sep 17 00:00:00 2001 From: Dennis Dyallo Date: Tue, 16 Jun 2026 17:11:05 +0200 Subject: [PATCH 05/25] test(core): add runtime resilience static scanner --- .../RuntimeResilienceStaticScanTests.cs | 373 ++++++++++++++++++ 1 file changed, 373 insertions(+) create mode 100644 src/Core/tests/Yubico.YubiKit.Core.UnitTests/RuntimeResilience/RuntimeResilienceStaticScanTests.cs diff --git a/src/Core/tests/Yubico.YubiKit.Core.UnitTests/RuntimeResilience/RuntimeResilienceStaticScanTests.cs b/src/Core/tests/Yubico.YubiKit.Core.UnitTests/RuntimeResilience/RuntimeResilienceStaticScanTests.cs new file mode 100644 index 000000000..9b2c806a9 --- /dev/null +++ b/src/Core/tests/Yubico.YubiKit.Core.UnitTests/RuntimeResilience/RuntimeResilienceStaticScanTests.cs @@ -0,0 +1,373 @@ +// Copyright 2026 Yubico AB +// +// Licensed under the Apache License, Version 2.0 (the "License"). +// You may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using System.Text.RegularExpressions; + +namespace Yubico.YubiKit.Core.UnitTests.RuntimeResilience; + +[Trait("Category", "RuntimeResilience")] +public sealed class RuntimeResilienceStaticScanTests +{ + [Fact] + public void Scanner_FlagsIgnoredNativeStatusChangeResultInsideLoop() + { + const string source = """ + private void ListenerThreadProc() + { + while (!_shouldStop) + { + _ = NativeMethods.SCardGetStatusChange(_context, 1000, states, states.Length); + continue; + } + } + """; + + var finding = RuntimeResilienceScanner.Scan("OldSmartCardListener.cs", source) + .Single(static finding => finding.Category == "IgnoredNativeResultInLoop"); + + Assert.Equal("IgnoredNativeResultInLoop", finding.Category); + Assert.Equal(5, finding.LineNumber); + } + + [Fact] + public void Scanner_FlagsNativeFailureContinueWithoutBackoff() + { + const string source = """ + private void ListenerThreadProc() + { + while (!_shouldStop) + { + var result = NativeMethods.SCardListReaders(_context, null, out var readers); + if (result != ErrorCode.SCARD_S_SUCCESS) + { + continue; + } + } + } + """; + + var finding = Assert.Single(RuntimeResilienceScanner.Scan("OldSmartCardListener.cs", source)); + + Assert.Equal("NativeFailureContinueWithoutBackoff", finding.Category); + Assert.Equal(8, finding.LineNumber); + } + + [Fact] + public void Scanner_DoesNotFlagNativeFailureContinueAfterHandler() + { + const string source = """ + private void ListenerThreadProc() + { + while (!_shouldStop) + { + var result = NativeMethods.SCardListReaders(_context, null, out var readers); + if (result != ErrorCode.SCARD_S_SUCCESS) + { + if (!HandleSCardFailure(result, "SCardListReaders")) + { + break; + } + + continue; + } + } + } + """; + + Assert.Empty(RuntimeResilienceScanner.Scan("SafeSmartCardListener.cs", source)); + } + + [Fact] + public void Scanner_FlagsCatchRetryWithoutBackoff() + { + const string source = """ + private void ListenerThreadProc() + { + while (!_shouldStop) + { + try + { + PollNativeState(); + } + catch (SCardException) + { + continue; + } + } + } + """; + + var finding = Assert.Single(RuntimeResilienceScanner.Scan("OldRetryLoop.cs", source)); + + Assert.Equal("CatchRetryWithoutBackoff", finding.Category); + Assert.Equal(11, finding.LineNumber); + } + + [Fact] + public void Scanner_DoesNotFlagCatchRetryWithBackoff() + { + const string source = """ + private void ListenerThreadProc() + { + while (!_shouldStop) + { + try + { + PollNativeState(); + } + catch (SCardException) + { + _sleep(TimeSpan.FromSeconds(1)); + continue; + } + } + } + """; + + Assert.Empty(RuntimeResilienceScanner.Scan("SafeRetryLoop.cs", source)); + } + + [Fact] + public void Scanner_FlagsSleepBeforeReadyToWritePoll() + { + const string source = """ + private async Task AwaitReadyToWriteAsync(CancellationToken cancellationToken) + { + while (stopwatch.ElapsedMilliseconds < timeLimitMs) + { + await Task.Delay(50, cancellationToken).ConfigureAwait(false); + + var report = await ReadFeatureReportAsync(cancellationToken).ConfigureAwait(false); + if ((report.Span[OtpConstants.FeatureReportDataSize] & OtpConstants.SlotWriteFlag) == 0) + { + return; + } + } + } + """; + + var finding = Assert.Single(RuntimeResilienceScanner.Scan("OldOtpHidProtocol.cs", source)); + + Assert.Equal("SleepBeforeReadyToWritePoll", finding.Category); + Assert.Equal(5, finding.LineNumber); + } + + [Fact] + public void Scanner_CurrentCoreSource_HasNoFindings() + { + var repositoryRoot = FindRepositoryRoot(); + var sourceRoots = new[] + { + Path.Combine(repositoryRoot.FullName, "src", "Core", "src", "Protocols"), + Path.Combine(repositoryRoot.FullName, "src", "Core", "src", "Transports") + }; + + foreach (var sourceRoot in sourceRoots) + { + Assert.True(Directory.Exists(sourceRoot), $"Source root does not exist: {sourceRoot}"); + } + + var sourceFiles = sourceRoots + .SelectMany(root => Directory.EnumerateFiles(root, "*.cs", SearchOption.AllDirectories)) + .ToArray(); + + Assert.True(sourceFiles.Length > 0, "Current-source scan must cover at least one source file."); + + var findings = sourceFiles + .SelectMany(file => RuntimeResilienceScanner.Scan( + Path.GetRelativePath(repositoryRoot.FullName, file), + File.ReadAllText(file))) + .ToArray(); + + Assert.True(findings.Length == 0, FormatFindings(findings)); + } + + private static DirectoryInfo FindRepositoryRoot() + { + var current = new DirectoryInfo(AppContext.BaseDirectory); + while (current is not null && !File.Exists(Path.Combine(current.FullName, "toolchain.cs"))) + { + current = current.Parent; + } + + return current ?? throw new DirectoryNotFoundException("Could not locate repository root from test output path."); + } + + private static string FormatFindings(IEnumerable findings) => + string.Join(Environment.NewLine, findings.Select(static finding => + $"{finding.Path}:{finding.LineNumber}: {finding.Category}: {finding.Message}")); + + private static class RuntimeResilienceScanner + { + private static readonly Regex MethodDeclarationPattern = new( + @"\b(?[A-Z_a-z][\w]*)\s*\([^;]*\)\s*$", + RegexOptions.Compiled); + + public static IReadOnlyList Scan(string path, string source) + { + var findings = new List(); + var blocks = new Stack(); + var pendingLoop = false; + var pendingCatch = false; + var pendingMethod = (string?)null; + var currentMethod = (string?)null; + var methodDepth = -1; + var readyToWriteReadSeen = false; + var lines = source.Split('\n'); + + for (int i = 0; i < lines.Length; i++) + { + var lineNumber = i + 1; + var line = lines[i].Trim(); + var inLoop = blocks.Any(static block => block.IsLoop); + var inCatch = blocks.Any(static block => block.IsCatch); + + if (inLoop && IsIgnoredNativeResult(line)) + { + findings.Add(new Finding( + path, + lineNumber, + "IgnoredNativeResultInLoop", + "Native status result is ignored inside a loop; handle, back off, exit, or throttle.")); + } + + if (currentMethod == "AwaitReadyToWriteAsync") + { + if (!readyToWriteReadSeen && line.Contains("Task.Delay(", StringComparison.Ordinal)) + { + findings.Add(new Finding( + path, + lineNumber, + "SleepBeforeReadyToWritePoll", + "Ready-to-write polling sleeps before checking whether the device is already ready.")); + } + + if (line.Contains("ReadFeatureReportAsync", StringComparison.Ordinal)) + { + readyToWriteReadSeen = true; + } + } + + if (inLoop && line == "continue;" && RecentNativeFailureWithoutBackoff(lines, i)) + { + findings.Add(new Finding( + path, + lineNumber, + "NativeFailureContinueWithoutBackoff", + "Native failure path continues the loop without a visible backoff, exit, or failure handler.")); + } + + if (inLoop && inCatch && line == "continue;" && !RecentBackoffOrExit(lines, i)) + { + findings.Add(new Finding( + path, + lineNumber, + "CatchRetryWithoutBackoff", + "Catch block retries a listener loop without a visible backoff or exit.")); + } + + pendingLoop |= line.StartsWith("while (", StringComparison.Ordinal); + pendingCatch |= line.StartsWith("catch", StringComparison.Ordinal); + pendingMethod ??= MatchMethodName(line); + + foreach (var character in line) + { + if (character == '{') + { + blocks.Push(new Block(pendingLoop, pendingCatch)); + pendingLoop = false; + pendingCatch = false; + + if (pendingMethod is not null) + { + currentMethod = pendingMethod; + methodDepth = blocks.Count; + readyToWriteReadSeen = false; + pendingMethod = null; + } + } + else if (character == '}' && blocks.Count > 0) + { + if (blocks.Count == methodDepth) + { + currentMethod = null; + methodDepth = -1; + } + + blocks.Pop(); + } + } + } + + return findings; + } + + private static bool IsIgnoredNativeResult(string line) => + line.StartsWith("_ = ", StringComparison.Ordinal) + && (line.Contains("SCardGetStatusChange", StringComparison.Ordinal) + || line.Contains("SCardListReaders", StringComparison.Ordinal) + || line.Contains("SCardEstablishContext", StringComparison.Ordinal)); + + private static string? MatchMethodName(string line) + { + var match = MethodDeclarationPattern.Match(line); + if (!match.Success + || line.Contains('=') + || line.StartsWith("if ", StringComparison.Ordinal) + || line.StartsWith("while ", StringComparison.Ordinal) + || line.StartsWith("for ", StringComparison.Ordinal) + || line.StartsWith("foreach ", StringComparison.Ordinal) + || line.StartsWith("switch ", StringComparison.Ordinal) + || line.StartsWith("catch ", StringComparison.Ordinal)) + { + return null; + } + + return match.Groups["name"].Value; + } + + private static bool RecentNativeFailureWithoutBackoff(string[] lines, int index) + { + var window = RecentLines(lines, index); + return window.Any(static line => + line.Contains("NativeMethods.SCard", StringComparison.Ordinal) + || line.Contains("_sCardApi.SCard", StringComparison.Ordinal) + || line.Contains("result != ErrorCode.SCARD_S_SUCCESS", StringComparison.Ordinal)) + && !window.Any(IsBackoffExitOrHandler); + } + + private static bool RecentBackoffOrExit(string[] lines, int index) => + RecentLines(lines, index).Any(IsBackoffExitOrHandler); + + private static IEnumerable RecentLines(string[] lines, int index) + { + var start = Math.Max(0, index - 6); + for (int i = start; i <= index; i++) + { + yield return lines[i].Trim(); + } + } + + private static bool IsBackoffExitOrHandler(string line) => + line.Contains("_sleep(", StringComparison.Ordinal) + || line.Contains("Thread.Sleep", StringComparison.Ordinal) + || line.Contains("Task.Delay", StringComparison.Ordinal) + || line.Contains("HandleSCardFailure", StringComparison.Ordinal) + || line == "break;" + || line.StartsWith("return", StringComparison.Ordinal); + + private readonly record struct Block(bool IsLoop, bool IsCatch); + public readonly record struct Finding(string Path, int LineNumber, string Category, string Message); + } +} \ No newline at end of file From 21b85016f330f9e1203d22664c8329785ab480e1 Mon Sep 17 00:00:00 2001 From: Dennis Dyallo Date: Tue, 16 Jun 2026 17:11:30 +0200 Subject: [PATCH 06/25] docs: document runtime resilience workflow --- AGENTS.md | 2 + docs/learnings/runtime-resilience/README.md | 19 ++ .../phase-01-smartcard-listener.md | 47 +++++ .../phase-02-otp-polling.md | 46 +++++ .../phase-03-static-scanner.md | 49 +++++ docs/plans/runtime-resilience-harness.md | 188 ++++++++++++++++++ src/Core/CLAUDE.md | 4 + 7 files changed, 355 insertions(+) create mode 100644 docs/learnings/runtime-resilience/README.md create mode 100644 docs/learnings/runtime-resilience/phase-01-smartcard-listener.md create mode 100644 docs/learnings/runtime-resilience/phase-02-otp-polling.md create mode 100644 docs/learnings/runtime-resilience/phase-03-static-scanner.md create mode 100644 docs/plans/runtime-resilience-harness.md diff --git a/AGENTS.md b/AGENTS.md index 899992606..10c43b720 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -30,6 +30,7 @@ - Platform interop belongs under `src/Core/src/PlatformInterop/{Windows,MacOS,Linux,Desktop}` with safe handles and `SdkPlatformInfo` platform selection. - FIDO2 over USB primarily uses HID FIDO. SmartCard/CCID FIDO2 is supported over NFC and over USB when the FIDO2 AID is exposed; USB SmartCard FIDO2 requires firmware 5.8.0+. - `TlvBuilder` and `DisposableTlvList` must be disposed. +- Listener/native retry loops must block, back off, exit, or throttle on every failure path; add no-hardware fault-injection tests for persistent native errors. ## Hardware And Integration Tests - Integration tests require authorized YubiKey hardware. The shared test infrastructure reads `YubiKeyTests:AllowedSerialNumbers` from `appsettings.json`; an empty/missing allow list can hard-fail with `Environment.Exit(-1)`, and unauthorized devices are filtered out. @@ -52,6 +53,7 @@ - Do not add validation-only tests that just prove `ArgumentNullException.ThrowIfNull` or framework type checks work. - Do not add skipped placeholder tests. If behavior cannot be unit-tested, document the limitation and point to an integration path. - Use fake connections, queued APDU responses, and byte/vector assertions for protocol behavior; use integration tests only for real hardware behavior. +- Runtime resilience tests should use fakes/seams and must not self-skip because PC/SC, HID, or YubiKey hardware is unavailable. ## Git And Release Notes - The workflow file currently triggers on `yubikit` and `yubikit-applets`; verify the active plan/branch before assuming a PR base. diff --git a/docs/learnings/runtime-resilience/README.md b/docs/learnings/runtime-resilience/README.md new file mode 100644 index 000000000..9a14c230d --- /dev/null +++ b/docs/learnings/runtime-resilience/README.md @@ -0,0 +1,19 @@ +# Runtime Resilience Learnings + +This folder records phase closeouts for the runtime resilience workstream. Each phase captures what we found, how we got better at finding it, what worked, what did not, what review changed, and what remains risky. + +## Phases + +- [Phase 1: SmartCard Listener Recovery](phase-01-smartcard-listener.md) +- [Phase 2: OTP HID Ready-To-Write Polling](phase-02-otp-polling.md) +- [Phase 3: Static Runtime-Resilience Scanner](phase-03-static-scanner.md) + +## Closeout Rule + +Each future phase should close in this order: + +1. Implement the smallest useful slice. +2. Verify with focused and broader project gates. +3. Run cross-vendor review when the change affects runtime behavior or proof quality. +4. Write the phase learning document. +5. Commit the phase before starting the next one. diff --git a/docs/learnings/runtime-resilience/phase-01-smartcard-listener.md b/docs/learnings/runtime-resilience/phase-01-smartcard-listener.md new file mode 100644 index 000000000..ca5324d21 --- /dev/null +++ b/docs/learnings/runtime-resilience/phase-01-smartcard-listener.md @@ -0,0 +1,47 @@ +# Phase 1: SmartCard Listener Recovery + +## What We Found + +- `DesktopSmartCardDeviceListener` ignored native PC/SC return values in listener paths. +- Persistent `SCardGetStatusChange` or `SCardListReaders` failures could create retry storms without an explicit backoff. +- The original `Stop()` shape could hold `_syncLock` while joining a listener thread that also needed the same lock during recovery. + +## How We Found It + +- We traced the SmartCard listener loop after the BenchmarkDotNet pass showed no remaining large foreground performance target. +- Cato pushed the plan toward both `SCardGetStatusChange` and `SCardListReaders`, bounded recovery, deterministic no-hardware tests, and safe `_context` handling. +- A temporary pre-fix worktree proved the old shape could exceed 100 immediate `SCardGetStatusChange` calls in 500ms under simulated `SCARD_E_INVALID_HANDLE`. + +## What Got Better + +- Added an internal `ISCardApi` seam so native PC/SC failures can be injected without PC/SC services or YubiKey hardware. +- Added fake sleeper handshakes to prove the listener enters backoff before retry or recovery. +- Added `Category=RuntimeResilience` to make this gate targetable while keeping it in default test runs. + +## What Worked + +- No-hardware fault injection was faster and more reliable than trying to reproduce native service failure live. +- Cross-vendor review caught important concurrency and semantic gaps before commit. +- Reducing the invariant to “every native failure loop must back off, exit, or recover” made the tests specific and stable. + +## What Did Not Work + +- A broad diagnostics harness would have been premature; the first useful slice was just a seam and focused unit tests. +- Raw metrics alone were less useful than a hard behavioral invariant. + +## Review Findings And Fixes + +- DevTeam found a potential `Stop()`/`Join()` lock stall. We moved thread join outside the lock. +- DevTeam found the recovery-success path was untested. We added a test proving backoff, context re-establishment, baseline rebuild, and resumed status monitoring. +- DevTeam flagged that `SCARD_E_SERVICE_STOPPED` and `SCARD_E_NO_SERVICE` changed from terminal to recoverable after stale contexts. We documented that intent in the plan. + +## Remaining Risk + +- Recovery is deliberately bounded but can keep trying once per second if context establishment succeeds while later calls fail. +- `Status` remains a simple property written from multiple threads. Existing behavior is preserved, but stronger memory semantics could be considered separately. + +## Verification + +- Focused SmartCard resilience tests: 4 passed, 0 failed, 0 skipped. +- Core gate: 522 total, 520 succeeded, 2 expected hardware skips, 0 failed after all phase work. +- Format verification: clean except existing `Tests.TestProject` IL2026/IL3050 warnings. diff --git a/docs/learnings/runtime-resilience/phase-02-otp-polling.md b/docs/learnings/runtime-resilience/phase-02-otp-polling.md new file mode 100644 index 000000000..b91641821 --- /dev/null +++ b/docs/learnings/runtime-resilience/phase-02-otp-polling.md @@ -0,0 +1,46 @@ +# Phase 2: OTP HID Ready-To-Write Polling + +## What We Found + +- The OTP HID ready-to-write path had already produced a large benchmark win by avoiding sleep-first polling. +- The existing unit guard was useful, but it needed clearer resilience intent and proof that it catches the seeded regression. + +## How We Found It + +- BenchmarkDotNet showed `CreateManagementSessionOverOtpHid` improved from about 1.039s to about 28ms and `GetDeviceInfoOverOtpHid` from about 2.075s to about 56ms. +- We inspected `AwaitReadyToWriteAsync` and the fake HID test path. The fake path writes exactly ten feature reports for an empty payload frame. + +## What Got Better + +- Renamed the test to `SendAndReceiveAsync_WhenReadyToWriteImmediately_DoesNotSleepBeforePolling`. +- Added `Category=RuntimeResilience` so the gate can be selected with the rest of the resilience suite. +- Added `SentReports.Count == 10` so the timing budget is tied to a known fake workload shape. +- Red-green proved the guard: temporarily adding a 50ms sleep before each write-readiness poll failed the focused test at 526ms; restoring the implementation made it pass. + +## What Worked + +- A loose no-hardware unit budget was enough for this specific regression because the old write-side sleep-first behavior adds at least 500ms across ten writes. +- The timing check is not a microbenchmark; it is a seeded-regression tripwire with large headroom. + +## What Did Not Work + +- Fake call counts alone cannot catch this regression because the old and new write paths read and write the same number of HID reports. +- BenchmarkDotNet is good evidence for the optimization, but it is too expensive and environment-dependent as the default unit-test gate. + +## Review Findings And Fixes + +- DevTeam noted the plan overclaimed proof by arithmetic. We performed a red-green check and recorded the failure/pass evidence. +- DevTeam noted the class summary was stale. We updated it to describe behavior and runtime-resilience invariants. +- DevTeam noted the guard specifically covers write-readiness sleep-first regression, not every possible read-side polling regression. The plan now scopes the evidence accordingly. + +## Remaining Risk + +- The guard is intentionally tied to `AwaitReadyToWriteAsync`, not a general detector for all OTP read-side sleeps. +- If future refactors change frame chunking, the `SentReports.Count == 10` assertion will force the test to be revisited. + +## Verification + +- Focused OTP resilience test: 1 passed, 0 failed, 0 skipped after restoration. +- Temporary regression check: focused OTP test failed at 526ms with the injected 50ms sleep-first delay. +- Core gate: 522 total, 520 succeeded, 2 expected hardware skips, 0 failed after all phase work. +- Format verification: clean except existing `Tests.TestProject` IL2026/IL3050 warnings. diff --git a/docs/learnings/runtime-resilience/phase-03-static-scanner.md b/docs/learnings/runtime-resilience/phase-03-static-scanner.md new file mode 100644 index 000000000..d4cc3e5e1 --- /dev/null +++ b/docs/learnings/runtime-resilience/phase-03-static-scanner.md @@ -0,0 +1,49 @@ +# Phase 3: Static Runtime-Resilience Scanner + +## What We Found + +- A small static scanner can catch several high-risk runtime-resilience shapes without building a dedicated diagnostics project. +- The useful prototype shapes are ignored native results inside loops, native failure `continue` paths without visible backoff/exit/handler, catch/retry loops without backoff, and sleep-before-ready-to-write polling. + +## How We Found It + +- We started with seeded source fixtures instead of scanning the entire repository blindly. +- We added a current-source smoke check only after positive fixtures existed. +- DevTeam review exposed that green scanner tests can be misleading unless paired safe-negative fixtures prove the detector can distinguish safe from unsafe shapes. + +## What Got Better + +- Added `RuntimeResilienceStaticScanTests` with positive fixtures for all four target shapes. +- Added paired safe negative fixtures for native failure handling and catch retry with backoff. +- Added a non-empty current-source scan over Core Protocols and Transports so source-root moves or empty scans fail loudly. +- Documented that the current-source scan is a smoke gate only, not proof of comprehensive false-positive quality. + +## What Worked + +- Keeping the scanner test-local avoided premature toolchain or diagnostics infrastructure. +- Positive fixtures plus paired negative fixtures gave better proof quality than scanning live source alone. +- Cross-vendor review was especially useful for test-meaningfulness, not just implementation correctness. + +## What Did Not Work + +- The first scanner version overclaimed proof from a current-source zero-finding test. +- The first native-failure detector lacked a safe negative fixture, so it proved only that a regex fired, not that it distinguished safe handler paths. +- Raw line/brace scanning is brittle. It is acceptable for a prototype but not a durable analyzer. + +## Review Findings And Fixes + +- DevTeam found the current-source zero-finding test was near-vacuous. We added source-root existence and non-empty file-count guards and softened the plan language. +- DevTeam found the native-failure detector lacked a paired safe-negative fixture. We added `Scanner_DoesNotFlagNativeFailureContinueAfterHandler`. +- Follow-up review passed and called out one remaining prototype limitation: the native-failure detector uses a small line-window heuristic instead of block-aware analysis. We documented that limitation in the plan. + +## Remaining Risk + +- The scanner is line-window based and can miss long failure blocks or over-flag unrelated nearby native calls. +- The sleep-before-poll detector is currently name-coupled to `AwaitReadyToWriteAsync`. +- Promotion to a real runner should use block-aware scanning or Roslyn-style syntax analysis if this proves useful across more phases. + +## Verification + +- Focused scanner suite: 7 passed, 0 failed, 0 skipped. +- Core gate: 522 total, 520 succeeded, 2 expected hardware skips, 0 failed. +- Format verification: clean except existing `Tests.TestProject` IL2026/IL3050 warnings. diff --git a/docs/plans/runtime-resilience-harness.md b/docs/plans/runtime-resilience-harness.md new file mode 100644 index 000000000..0544d175c --- /dev/null +++ b/docs/plans/runtime-resilience-harness.md @@ -0,0 +1,188 @@ +# Runtime Resilience Harness Plan + +## Purpose + +Create a local-first workflow that catches real SDK production risks before they ship: native listener spin loops, sleep-first polling regressions, handle/file-descriptor leaks, recovery retry storms, and foreground operation performance regressions. + +The harness must prove value incrementally. Each slice must catch a known bug class or produce a concrete new finding before we build the next layer. + +## Operating Principles + +- Prefer hard pass/fail invariants over dashboards of raw metrics. +- Prefer no-hardware deterministic tests before live-hardware diagnostics. +- Prefer existing test and benchmark projects before creating new infrastructure. +- Promote to a dedicated diagnostics project only after reuse across at least three modules or bug classes is proven. +- Do not add public SDK APIs solely for diagnostics. +- Do not make destructive live YubiKey operations part of the default path. +- Keep the fast local path under 90 seconds. + +## Phase Closeout Gate + +Before starting the next phase: + +- Write a learning document under `docs/learnings/runtime-resilience/` covering what we found, how we got better at finding it, what worked, what failed, what review changed, and what remains risky. +- Run focused verification for the phase and the broader Core gate when code changed. +- Run cross-vendor review when the phase changes runtime behavior, proof quality, or scanner logic. +- Commit the phase as a logical work area before moving on. +- Do not let later-phase exploration accumulate uncommitted changes on top of an unclosed phase. + +## Cato Corrections + +- Split the plan into two tracks: CI-friendly no-hardware gates and local-only live-hardware diagnostics. +- Wire no-hardware fault-injection tests into ordinary CI eventually; do not let lack of hardware CI block those gates. +- Start from invariants, then collect only metrics needed to prove them. +- Avoid absolute live-hardware CPU/memory/latency thresholds unless they are baseline-relative and stable. +- Prioritize handle/file-descriptor leak invariants because they are high-signal and low-noise. +- Defer a new diagnostics project and reusable skill until the smaller harness catches enough real issues to justify them. + +## Known Bug Seeds + +- SmartCard listener stale-context spin: persistent `SCARD_E_INVALID_HANDLE` from `SCardGetStatusChange` could trigger rapid calls. +- OTP HID sleep-first polling: fixed 50 ms delay before readiness checks caused second-level management calls. + +## Phase 0: Prove The SmartCard Bug Class + +- [x] Add a temporary pre-fix diagnostic proving old listener behavior can exceed 100 immediate `SCardGetStatusChange` calls within 500 ms when `SCARD_E_INVALID_HANDLE` returns immediately. +- [x] Add no-hardware SmartCard fault-injection tests using an internal SCard seam. +- [x] Cover persistent `SCardGetStatusChange` invalid-handle behavior. +- [x] Cover persistent `SCardListReaders` invalid-handle behavior. +- [x] Cover failed context re-establishment so recovery cannot spin on `SCardEstablishContext`. +- [x] Run Cato on the SmartCard listener plan and incorporate concerns. +- [x] Run DevTeam cross-vendor review and incorporate actionable findings. +- [x] Verify focused SmartCard listener tests pass. +- [x] Verify Core tests pass without user-presence tests. + +Evidence: +- Pre-fix temp worktree diagnostic passed: old listener exceeded 100 `SCardGetStatusChange` calls within 500 ms under simulated invalid-handle returns. +- Focused regression tests passed: `DesktopSmartCardDeviceListenerSCardErrorTests`, 3 total, 3 succeeded. +- Core tests passed: 514 total, 512 succeeded, 2 skipped. +- Core build passed: 3 Core projects, 0 warnings, 0 errors. +- Format verification passed with only existing `Tests.TestProject` IL2026/IL3050 warnings. + +## Phase 1: Make The First Gate CI-Friendly + +- [x] Keep the SmartCard no-hardware fault-injection tests in the normal Core unit test path. +- [x] Confirm they do not self-skip when PC/SC is unavailable. +- [x] Add comments or documentation explaining the regression invariant: persistent native failures must be backoff-bounded. +- [x] Add a checklist item for future listener/native-loop PRs: every native-loop error path must block, back off, exit, or throttle. +- [x] Decide whether to tag these tests with a dedicated trait such as `RuntimeResilience` without excluding them from default unit test runs. + +Evidence: +- `DesktopSmartCardDeviceListenerSCardErrorTests` uses fake `ISCardApi` and fake sleeper handshakes, so it does not depend on PC/SC, HID, live YubiKeys, or platform services. +- Tests are tagged `Category=RuntimeResilience` for targeted execution, but they remain normal Core unit tests and are not excluded by default. +- `AGENTS.md` and `src/Core/CLAUDE.md` now document the listener/native-loop invariant for future PR review. +- `SCARD_E_SERVICE_STOPPED` and `SCARD_E_NO_SERVICE` are intentionally recoverable after an established context becomes stale; failed re-establishment still transitions the listener to `Error`. + +Proof of value required before moving on: +- Reverting the SmartCard listener fix must make the no-hardware tests fail or the pre-fix diagnostic pass as a bug detector. + +## Phase 2: Catch The OTP Sleep-First Bug Class + +- [x] Add a regression invariant for OTP HID ready-to-write polling. +- [x] Decide the best detector: existing unit timing assertion, BenchmarkDotNet baseline comparison, or a deterministic fake-call-count test. +- [x] Avoid fragile absolute wall-clock thresholds where call-count or fake sleeper assertions can prove the same behavior. +- [x] Record before/after evidence from the existing BenchmarkDotNet run: ~1.039 s to ~28 ms and ~2.075 s to ~56 ms. + +Decision: +- Use the no-hardware unit timing guard in `OtpHidProtocolTests.SendAndReceiveAsync_WhenReadyToWriteImmediately_DoesNotSleepBeforePolling` and tag it `Category=RuntimeResilience`. +- Do not use BenchmarkDotNet as the default regression gate; it is evidence for the optimization, but too expensive and environmental for every unit-test run. +- Do not use fake call count as the primary detector; the sleep-first regression preserves the same HID report count and only changes when the first read occurs. +- The 200ms budget is intentionally loose relative to the old 10 x 50ms minimum write-side delay, so it catches the seeded regression without being a microbenchmark. + +Evidence: +- Existing BenchmarkDotNet evidence: `CreateManagementSessionOverOtpHid` improved from ~1.039 s to ~28 ms, and `GetDeviceInfoOverOtpHid` improved from ~2.075 s to ~56 ms. +- Reintroducing a 50ms sleep before each ready-to-write poll across the 10 frame reports in the fake unit path would add at least 500ms and violate the 200ms runtime-resilience budget. +- Red-green proof: temporarily reintroducing the 50ms sleep-first ready-to-write delay made the focused test fail at 526ms; restoring the implementation made it pass again. + +Proof of value required before moving on: +- Reintroducing sleep-first polling must fail a test or exceed a stored benchmark budget. Satisfied by the red-green no-hardware unit budget above. + +## Phase 3: Static Native-Loop Screening + +- [x] Prototype a lightweight scanner for high-risk loop shapes. +- [x] Flag ignored native return values inside loops. +- [x] Flag `continue` paths after native failures without sleep/backoff/exit. +- [x] Flag `catch` plus retry loops without sleep/backoff/exit. +- [x] Flag fixed sleeps in protocol polling paths for manual review. +- [x] Keep output small: file, line, risk category, reason. + +Decision: +- Keep the prototype as Core no-hardware unit tests for now, not a separate diagnostics project or toolchain target. +- Seeded-source tests prove the scanner flags the historical SmartCard ignored-native-result loop shape and the OTP ready-to-write sleep-before-poll shape. +- A current-source scan over `src/Core/src/Protocols` and `src/Core/src/Transports` is a smoke gate only; explicit safe negative fixtures carry the false-positive proof for now. +- The native-failure detector intentionally uses a small line-window heuristic at prototype stage; promote to block-aware scanning only if Phase 5/6 turns this into a durable runner. + +Evidence: +- `RuntimeResilienceStaticScanTests.Scanner_FlagsIgnoredNativeStatusChangeResultInsideLoop` catches the old SmartCard-style ignored `SCardGetStatusChange` result inside a listener loop. +- `RuntimeResilienceStaticScanTests.Scanner_FlagsNativeFailureContinueWithoutBackoff` catches a native failure path that immediately continues the loop. +- `RuntimeResilienceStaticScanTests.Scanner_DoesNotFlagNativeFailureContinueAfterHandler` proves the matching safe handler/break/continue shape stays quiet. +- `RuntimeResilienceStaticScanTests.Scanner_FlagsCatchRetryWithoutBackoff` catches a catch/retry loop with no visible backoff or exit. +- `RuntimeResilienceStaticScanTests.Scanner_DoesNotFlagCatchRetryWithBackoff` proves the matching catch/backoff/continue shape stays quiet. +- `RuntimeResilienceStaticScanTests.Scanner_FlagsSleepBeforeReadyToWritePoll` catches a sleep-first `AwaitReadyToWriteAsync` shape. +- `RuntimeResilienceStaticScanTests.Scanner_CurrentCoreSource_HasNoFindings` verifies current Core Protocols/Transports source roots exist, cover files, and currently produce zero scanner findings. + +Proof of value required before moving on: +- Scanner must flag at least the old SmartCard shape or the OTP sleep-first pattern with low false-positive volume. Satisfied at prototype level by seeded positive fixtures, paired safe negative fixtures, and a non-empty current-source smoke scan. + +## Phase 4: Handle And File Descriptor Leak Invariant + +- [ ] Define a cross-platform metric for handles/file descriptors where possible. +- [ ] Add a small no-hardware fake test if live handle counts are too platform-specific. +- [ ] Add a live optional diagnostic for repeated connect/disconnect/listener start/stop cycles. +- [ ] Assert handles/fds return to baseline within a strict tolerance. + +Proof of value required before moving on: +- A deliberately leaked connection/listener handle must be detected locally. + +## Phase 5: Minimal Local Runner + +- [ ] Add a single toolchain entry point only after Phases 1-4 prove useful. +- [ ] Suggested shape: `dotnet toolchain.cs -- resilience --fast`. +- [ ] Fast mode should run no-hardware resilience tests, static scanner, and selected benchmark budget checks. +- [ ] Output should be pass/fail with paths to evidence, not a dashboard. +- [ ] Keep the fast path under 90 seconds. + +Proof of value required before moving on: +- One command catches at least the SmartCard and OTP seeded regressions. + +## Phase 6: Dedicated Diagnostics Project, Only If Justified + +- [ ] Create `diagnostics/Yubico.YubiKit.RuntimeDiagnostics` only if the same runner patterns are reused across at least three modules or bug classes. +- [ ] Add scenario registry only after multiple scenarios exist. +- [ ] Add JSON/markdown reports only for asserted invariants. +- [ ] Record device serial, model, firmware, and transport for every live result. +- [ ] Default to read-only, non-destructive operations. + +Do not build yet: +- Cross-module all-applet orchestration. +- General CPU/memory/GC graphing without invariants. +- Live-hardware CI integration. +- A reusable local audit skill. + +## Phase 7: Reusable Local Audit Skill, Last + +- [ ] Create a `yubikit-runtime-audit` skill only after the toolchain/local runner shape stabilizes. +- [ ] The skill should orchestrate existing commands, not invent parallel behavior. +- [ ] It should produce a concise report and recommend next probes. + +Proof of value required: +- The command-line harness is already useful without the skill. + +## Live-Hardware Safety Rules + +- [ ] Default live scenarios must be read-only. +- [ ] Mutating scenarios require an explicit opt-in flag. +- [ ] PIN, PUK, credential, reset, delete, and key-generation scenarios require a disposable test key declaration. +- [ ] Any test requiring touch/user presence must be marked and excluded from fast mode. +- [ ] Results must label hardware coverage as single-device unless a matrix was run. + +## Definition Of Useful + +A slice is useful only if it does at least one of these: + +- Catches a known seeded regression. +- Finds a new bug or suspicious runtime behavior. +- Blocks a realistic class of production regression with a stable invariant. +- Reduces manual investigation time with a clear pass/fail report. + +If a slice only produces metrics that nobody acts on, it is not useful and should be cut. diff --git a/src/Core/CLAUDE.md b/src/Core/CLAUDE.md index 7b6ab5562..bef68c3a2 100644 --- a/src/Core/CLAUDE.md +++ b/src/Core/CLAUDE.md @@ -69,6 +69,10 @@ If using DI, configure logging explicitly from the DI-provided `ILoggerFactory` ## Critical Patterns +### Listener and Native Retry Loops + +Background listeners and native/resource-manager retry loops must block, back off, exit, or throttle on every failure path. Do not ignore native return values inside loops unless another call in the same path provides a bounded wait. Persistent failures such as stale PC/SC handles must have no-hardware fault-injection tests that prove call cadence is backoff-bounded. + ### APDU Processing Pipeline The APDU processing pipeline uses the decorator pattern: From dad4326fa39addcab40e4ef1fc666315f04e6a54 Mon Sep 17 00:00:00 2001 From: Dennis Dyallo Date: Tue, 16 Jun 2026 17:27:40 +0200 Subject: [PATCH 07/25] fix(core): dispose SmartCard listener contexts on stop --- docs/learnings/runtime-resilience/README.md | 1 + .../phase-04-smartcard-context-leak.md | 47 +++++++++++++++++++ docs/plans/runtime-resilience-harness.md | 20 ++++++-- .../DesktopSmartCardDeviceListener.cs | 19 ++++++++ ...pSmartCardDeviceListenerSCardErrorTests.cs | 32 +++++++++++-- 5 files changed, 112 insertions(+), 7 deletions(-) create mode 100644 docs/learnings/runtime-resilience/phase-04-smartcard-context-leak.md diff --git a/docs/learnings/runtime-resilience/README.md b/docs/learnings/runtime-resilience/README.md index 9a14c230d..61fdd55f4 100644 --- a/docs/learnings/runtime-resilience/README.md +++ b/docs/learnings/runtime-resilience/README.md @@ -7,6 +7,7 @@ This folder records phase closeouts for the runtime resilience workstream. Each - [Phase 1: SmartCard Listener Recovery](phase-01-smartcard-listener.md) - [Phase 2: OTP HID Ready-To-Write Polling](phase-02-otp-polling.md) - [Phase 3: Static Runtime-Resilience Scanner](phase-03-static-scanner.md) +- [Phase 4: SmartCard Context Leak Invariant](phase-04-smartcard-context-leak.md) ## Closeout Rule diff --git a/docs/learnings/runtime-resilience/phase-04-smartcard-context-leak.md b/docs/learnings/runtime-resilience/phase-04-smartcard-context-leak.md new file mode 100644 index 000000000..f181f6ad1 --- /dev/null +++ b/docs/learnings/runtime-resilience/phase-04-smartcard-context-leak.md @@ -0,0 +1,47 @@ +# Phase 4: SmartCard Context Leak Invariant + +## What We Found + +- Repeated `Start(); Stop(); Start(); Stop(); Dispose();` on `DesktopSmartCardDeviceListener` established two PC/SC contexts but released only one. +- The previous context remained assigned after `Stop()` and was overwritten by the next `Start()`. +- This was a real no-hardware resource leak in the listener lifecycle, not just a hypothetical OS handle metric. + +## How We Found It + +- We inspected the Phase 1 listener lifecycle while looking for a cross-platform handle/fd invariant. +- Live OS handle counts were rejected for the default gate because they are platform-specific and noisy. +- A fake `SCardContext` release counter gave a deterministic substitute for “handles return to baseline.” + +## What Got Better + +- Added `WhenListenerRestarts_PreviousContextsAreDisposed` to assert fake context releases equal fake context establishments after restart and dispose. +- Fixed `StopListening()` to dispose the stopped context after the listener thread exits. +- Preserved safety on join timeout: if the listener thread does not exit, the context is intentionally not disposed to avoid use-after-free on the background thread. + +## What Worked + +- Red-green proof was immediate: the new invariant failed with 2 established and 1 released, then passed after the disposal fix. +- Counting fake context releases was more stable than reading live process handle/fd counts. +- DevTeam review validated the race safety and the “bounded leak beats use-after-free” decision on join timeout. + +## What Did Not Work + +- A generic live fd/handle diagnostic would have been premature without a runner and without a second resource class to compare. +- The fake invariant does not cover every lifecycle edge, such as a permanently wedged listener thread. + +## Review Findings And Fixes + +- DevTeam returned `pass` with no blocking findings. +- DevTeam suggested documenting the join-timeout branch. We added a comment explaining that the listener may still be using the native context, so the code prefers a bounded leak over disposing an active handle. + +## Remaining Risk + +- The join-timeout path intentionally leaks one context rather than risking use-after-free. +- Recovery-swap behavior is covered by Phase 1 tests, but not by a release-count stress loop. +- Live fd/handle diagnostics remain deferred until Phase 5 gives us a runner shape. + +## Verification + +- Focused SmartCard resilience tests: 5 passed, 0 failed, 0 skipped. +- Core gate: 523 total, 521 succeeded, 2 expected hardware skips, 0 failed. +- Format verification: clean except existing `Tests.TestProject` IL2026/IL3050 warnings. diff --git a/docs/plans/runtime-resilience-harness.md b/docs/plans/runtime-resilience-harness.md index 0544d175c..a290a1c58 100644 --- a/docs/plans/runtime-resilience-harness.md +++ b/docs/plans/runtime-resilience-harness.md @@ -126,13 +126,25 @@ Proof of value required before moving on: ## Phase 4: Handle And File Descriptor Leak Invariant -- [ ] Define a cross-platform metric for handles/file descriptors where possible. -- [ ] Add a small no-hardware fake test if live handle counts are too platform-specific. +- [x] Define a cross-platform metric for handles/file descriptors where possible. +- [x] Add a small no-hardware fake test if live handle counts are too platform-specific. - [ ] Add a live optional diagnostic for repeated connect/disconnect/listener start/stop cycles. -- [ ] Assert handles/fds return to baseline within a strict tolerance. +- [x] Assert handles/fds return to baseline within a strict tolerance. + +Decision: +- Use fake `SCardContext` release counts as the Phase 4 cross-platform metric. OS handle/fd counts are platform-specific and too noisy for the default no-hardware gate. +- Defer live optional diagnostics until there is a runner in Phase 5 or a second handle/fd class to exercise. +- Treat `ReleasedContextCalls == EstablishContextCalls` after restart/dispose as the strict no-hardware baseline-return invariant. + +Evidence: +- `WhenListenerRestarts_PreviousContextsAreDisposed` initially failed with 2 contexts established and only 1 released. +- `StopListening()` now disposes the stopped context only after the listener thread has joined; if join times out, it intentionally leaves the context alive to avoid disposing a handle the background thread may still use. +- After the fix, focused SmartCard tests passed: 5 total, 5 succeeded, 0 skipped. +- Core tests passed: 523 total, 521 succeeded, 2 skipped. +- DevTeam review returned `pass` and confirmed the leak-vs-use-after-free tradeoff. Proof of value required before moving on: -- A deliberately leaked connection/listener handle must be detected locally. +- A deliberately leaked connection/listener handle must be detected locally. Satisfied by the red-green fake-context release invariant above. ## Phase 5: Minimal Local Runner diff --git a/src/Core/src/Transports/SmartCard/DesktopSmartCardDeviceListener.cs b/src/Core/src/Transports/SmartCard/DesktopSmartCardDeviceListener.cs index 0c71104bd..b111fd1b3 100644 --- a/src/Core/src/Transports/SmartCard/DesktopSmartCardDeviceListener.cs +++ b/src/Core/src/Transports/SmartCard/DesktopSmartCardDeviceListener.cs @@ -180,13 +180,32 @@ private void StopListening() } // Wait for the listener thread to exit + var listenerStopped = true; if (listenerThread is not null && listenerThread.IsAlive) { if (!listenerThread.Join(MaxDisposalWaitTime)) { Logger.LogWarning("SmartCard listener thread did not exit within timeout"); + listenerStopped = false; } } + + if (!listenerStopped) + { + // The listener may still be using the native context. Prefer a bounded leak over + // disposing a handle that could still be active on the background thread. + return; + } + + lock (_syncLock) + { + if (ReferenceEquals(_context, context)) + { + _context = null; + } + } + + context?.Dispose(); } private void ListenerThreadProc() diff --git a/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Transports/SmartCard/DesktopSmartCardDeviceListenerSCardErrorTests.cs b/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Transports/SmartCard/DesktopSmartCardDeviceListenerSCardErrorTests.cs index 1be70d5af..89ebb295f 100644 --- a/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Transports/SmartCard/DesktopSmartCardDeviceListenerSCardErrorTests.cs +++ b/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Transports/SmartCard/DesktopSmartCardDeviceListenerSCardErrorTests.cs @@ -147,6 +147,25 @@ public void WhenContextReestablishmentSucceeds_ListenerRebuildsBaselineAndResume } } + [Fact] + public void WhenListenerRestarts_PreviousContextsAreDisposed() + { + var interop = new FakeSCardApi + { + ListReaders = _ => (ErrorCode.SCARD_E_NO_READERS_AVAILABLE, []) + }; + using var listener = new DesktopSmartCardDeviceListener(interop, _ => { }); + + listener.Start(); + listener.Stop(); + listener.Start(); + listener.Stop(); + listener.Dispose(); + + Assert.Equal(2, interop.EstablishContextCalls); + Assert.Equal(interop.EstablishContextCalls, interop.ReleasedContextCalls); + } + private sealed class BlockingSleeper : IDisposable { private readonly ManualResetEventSlim _sleeping = new(); @@ -186,11 +205,12 @@ private sealed class FakeSCardApi : ISCardApi public int EstablishContextCalls { get; private set; } public int GetStatusChangeCalls { get; private set; } public int ListReadersCalls { get; private set; } + public int ReleasedContextCalls { get; private set; } public uint SCardEstablishContext(SCARD_SCOPE scope, out SCardContext context) { EstablishContextCalls++; - context = new TestSCardContext(new IntPtr(EstablishContextCalls)); + context = new TestSCardContext(new IntPtr(EstablishContextCalls), this); return EstablishContext(EstablishContextCalls); } @@ -214,9 +234,15 @@ public uint SCardGetStatusChange( public uint SCardCancel(SCardContext context) => ErrorCode.SCARD_S_SUCCESS; - private sealed class TestSCardContext(IntPtr handle) : SCardContext(handle) + private void RecordReleasedContext() => ReleasedContextCalls++; + + private sealed class TestSCardContext(IntPtr handle, FakeSCardApi owner) : SCardContext(handle) { - protected override bool ReleaseHandle() => true; + protected override bool ReleaseHandle() + { + owner.RecordReleasedContext(); + return true; + } } } } \ No newline at end of file From 52d4d111079b469421e7dc1701879f22d24c41c2 Mon Sep 17 00:00:00 2001 From: Dennis Dyallo Date: Tue, 16 Jun 2026 17:37:15 +0200 Subject: [PATCH 08/25] test(core): add runtime resilience fast runner --- docs/learnings/runtime-resilience/README.md | 1 + .../phase-05-minimal-fast-runner.md | 53 +++++++++++++++++++ docs/plans/runtime-resilience-harness.md | 25 ++++++--- toolchain.cs | 50 ++++++++++++++++- 4 files changed, 122 insertions(+), 7 deletions(-) create mode 100644 docs/learnings/runtime-resilience/phase-05-minimal-fast-runner.md diff --git a/docs/learnings/runtime-resilience/README.md b/docs/learnings/runtime-resilience/README.md index 61fdd55f4..f96019d3a 100644 --- a/docs/learnings/runtime-resilience/README.md +++ b/docs/learnings/runtime-resilience/README.md @@ -8,6 +8,7 @@ This folder records phase closeouts for the runtime resilience workstream. Each - [Phase 2: OTP HID Ready-To-Write Polling](phase-02-otp-polling.md) - [Phase 3: Static Runtime-Resilience Scanner](phase-03-static-scanner.md) - [Phase 4: SmartCard Context Leak Invariant](phase-04-smartcard-context-leak.md) +- [Phase 5: Minimal Fast Runner](phase-05-minimal-fast-runner.md) ## Closeout Rule diff --git a/docs/learnings/runtime-resilience/phase-05-minimal-fast-runner.md b/docs/learnings/runtime-resilience/phase-05-minimal-fast-runner.md new file mode 100644 index 000000000..45630e4ed --- /dev/null +++ b/docs/learnings/runtime-resilience/phase-05-minimal-fast-runner.md @@ -0,0 +1,53 @@ +# Phase 5: Minimal Fast Runner + +## What We Found + +- Phases 1-4 produced enough useful no-hardware resilience gates to justify one local runner. +- The existing test infrastructure already had the needed primitive: `Category=RuntimeResilience`. +- A separate diagnostics project would still be premature. + +## How We Found It + +- We verified the individual gates first: SmartCard listener backoff/recovery, OTP polling, static scanner, and SmartCard context release. +- We then added one toolchain target that composes those gates instead of adding new test infrastructure. +- Red-green checks proved the runner fails on both an OTP sleep-first regression and a SmartCard context leak regression. + +## What Got Better + +- Added `dotnet toolchain.cs -- resilience --fast` as the single fast local command. +- The runner executes Core unit tests tagged `Category=RuntimeResilience`. +- The output remains pass/fail with concise evidence and elapsed time, not a dashboard. +- The runner requires `--fast` because no slower or live-hardware mode exists yet. + +## What Worked + +- Reusing xUnit traits and `toolchain.cs` avoided a new diagnostics project. +- The runner stayed well under the 90-second target, passing 13 runtime-resilience tests in 3.2s after final hardening. +- A single command caught both seeded regression classes tested in this phase. + +## What Did Not Work + +- Treating `--fast` as advisory was misleading. We changed it to a required flag. +- Mutating the shared `testFilter` without restoration was a future footgun. We restored it in a `finally` block after the runner completes. +- BenchmarkDotNet remains too expensive for the default fast path. + +## Review Findings And Fixes + +- DevTeam returned `pass` with non-blocking findings. +- We fixed the shared `testFilter` mutation risk by restoring the previous filter in `finally`. +- We fixed the `--fast` no-op risk by making the target fail immediately without `--fast`. + +## Remaining Risk + +- The runner currently hardcodes Core because all runtime-resilience gates are Core tests. +- `--project` and `--integration` are intentionally ignored by this fixed gate. +- Live-hardware diagnostics remain deferred. + +## Verification + +- `dotnet toolchain.cs -- resilience --fast`: 13 runtime-resilience tests passed, 0 failed, elapsed 3.2s after final hardening. +- `dotnet toolchain.cs -- resilience`: failed immediately with guidance because `--fast` is required. +- Temporary OTP regression: runner failed with scanner and timing failures. +- Temporary SmartCard leak regression: runner failed with the context-release invariant. +- Core gate: 523 total, 521 succeeded, 2 expected hardware skips, 0 failed before final docs update. +- Format verification: clean except existing `Tests.TestProject` IL2026/IL3050 warnings before final docs update. diff --git a/docs/plans/runtime-resilience-harness.md b/docs/plans/runtime-resilience-harness.md index a290a1c58..d6c61e808 100644 --- a/docs/plans/runtime-resilience-harness.md +++ b/docs/plans/runtime-resilience-harness.md @@ -148,14 +148,27 @@ Proof of value required before moving on: ## Phase 5: Minimal Local Runner -- [ ] Add a single toolchain entry point only after Phases 1-4 prove useful. -- [ ] Suggested shape: `dotnet toolchain.cs -- resilience --fast`. -- [ ] Fast mode should run no-hardware resilience tests, static scanner, and selected benchmark budget checks. -- [ ] Output should be pass/fail with paths to evidence, not a dashboard. -- [ ] Keep the fast path under 90 seconds. +- [x] Add a single toolchain entry point only after Phases 1-4 prove useful. +- [x] Suggested shape: `dotnet toolchain.cs -- resilience --fast`. +- [x] Fast mode should run no-hardware resilience tests, static scanner, and selected benchmark budget checks. +- [x] Output should be pass/fail with paths to evidence, not a dashboard. +- [x] Keep the fast path under 90 seconds. + +Decision: +- Add `resilience` as a `toolchain.cs` target instead of a separate diagnostics project. +- Require `--fast` because only the no-hardware fast mode exists today. +- Implement the runner by executing Core unit tests tagged `Category=RuntimeResilience`. +- Do not add BenchmarkDotNet to the default runner; the OTP unit timing guard is the fast budget check, and BenchmarkDotNet remains supporting evidence. + +Evidence: +- `dotnet toolchain.cs -- resilience --fast` passed 13 runtime-resilience tests in 3.2s after final hardening. +- Running `dotnet toolchain.cs -- resilience` fails immediately with guidance because `--fast` is required. +- A temporary OTP sleep-first regression made the runner fail with both scanner and OTP timing failures. +- A temporary SmartCard context leak regression made the runner fail with the context-release invariant. +- DevTeam review returned `pass`; we hardened the target afterward by restoring the captured `testFilter` in `finally` and making `--fast` required rather than advisory. Proof of value required before moving on: -- One command catches at least the SmartCard and OTP seeded regressions. +- One command catches at least the SmartCard and OTP seeded regressions. Satisfied by the red-green runner checks above. ## Phase 6: Dedicated Diagnostics Project, Only If Justified diff --git a/toolchain.cs b/toolchain.cs index b7e5a199e..b12046200 100755 --- a/toolchain.cs +++ b/toolchain.cs @@ -22,6 +22,7 @@ * restore - Restore NuGet dependencies * build - Build the solution (restores only if needed) * test - Run unit tests with summary output + * resilience - Run fast no-hardware runtime resilience gates * docs-qa - Validate active documentation hygiene * coverage - Run tests with code coverage * pack - Create NuGet packages @@ -43,6 +44,7 @@ * --project Build/test specific project only (partial match) * --integration Include integration tests (requires --project, unit tests only by default) * --smoke Smoke test mode: skip Slow and RequiresUserPresence tests + * --fast Required fast mode for resilience gates * * EXAMPLES: * dotnet toolchain.cs build @@ -51,6 +53,7 @@ * dotnet toolchain.cs docs-qa * dotnet toolchain.cs test --filter "FullyQualifiedName~MyTestClass" * dotnet toolchain.cs test --project Piv --filter "Method~Sign" + * dotnet toolchain.cs -- resilience --fast * dotnet toolchain.cs -- test --integration --project Piv --smoke (quick integration smoke test) * dotnet toolchain.cs coverage * dotnet toolchain.cs publish --package-version 1.0.0-preview.1 @@ -117,6 +120,7 @@ var projectFilter = GetArgument("--project"); var includeIntegration = HasFlag("--integration"); var smokeTest = HasFlag("--smoke"); +var fastMode = HasFlag("--fast"); // --smoke injects trait filters to skip slow and user-presence tests if (smokeTest) @@ -281,6 +285,47 @@ throw new InvalidOperationException($"{failCount} test project(s) failed during coverage collection"); }); +Target("resilience", () => +{ + PrintHeader("Running fast runtime resilience gates"); + + if (!fastMode) + { + throw new InvalidOperationException("The resilience target currently requires --fast; only fast no-hardware mode is implemented."); + } + + var previousFilter = testFilter; + try + { + testFilter = string.IsNullOrEmpty(previousFilter) + ? "Category=RuntimeResilience" + : $"{previousFilter}&Category=RuntimeResilience"; + + var projectsToTest = FilterToProject(testProjectInfos, "Core"); + if (projectsToTest is null) + return; + + var stopwatch = Stopwatch.StartNew(); + var results = RunTestProjects(projectsToTest); + stopwatch.Stop(); + + PrintTestSummary(results, "RUNTIME RESILIENCE"); + PrintInfo("Evidence: Core unit tests with Category=RuntimeResilience"); + PrintInfo($"Elapsed: {stopwatch.Elapsed.TotalSeconds:F1}s"); + + var failCount = results.Count(r => !r.Passed); + if (failCount > 0) + throw new InvalidOperationException($"{failCount} runtime resilience test project(s) failed"); + + if (results.Count > 0 && results.All(r => r.Skipped)) + throw new InvalidOperationException("No runtime resilience tests matched the specified filter"); + } + finally + { + testFilter = previousFilter; + } +}); + Target("docs-qa", () => { PrintHeader("Validating active documentation"); @@ -390,7 +435,7 @@ var bullseyeArgs = FilterBullseyeArgs(args, optionsWithValues: ["--project", "--filter", "--package-version", "--nuget-feed-name", "--nuget-feed-path", "--nuget-feed-url", "--nuget-api-key"], - flags: ["--integration", "--include-docs", "--dry-run", "--clean", "--smoke"]); + flags: ["--integration", "--include-docs", "--dry-run", "--clean", "--smoke", "--fast"]); await RunTargetsAndExitAsync(bullseyeArgs); // ─── Helper functions ────────────────────────────────────────────────────────── @@ -616,6 +661,7 @@ dotnet toolchain.cs -- build --project Piv Use when in doubt restore - Restore NuGet dependencies build - Build the solution (restores only if needed) test - Run unit tests with summary output + resilience - Run fast no-hardware runtime resilience gates docs-qa - Validate active documentation hygiene coverage - Run tests with code coverage pack - Create NuGet packages @@ -637,6 +683,7 @@ dotnet toolchain.cs -- build --project Piv Use when in doubt --project Build/test specific project only (partial match) --integration Include integration tests (requires --project) --smoke Smoke test mode: skip Slow and RequiresUserPresence tests + --fast Required fast mode for resilience gates -h, --help Show this help message EXAMPLES: @@ -646,6 +693,7 @@ dotnet toolchain.cs test dotnet toolchain.cs docs-qa dotnet toolchain.cs test --filter ""FullyQualifiedName~MyTestClass"" dotnet toolchain.cs test --project Piv --filter ""Method~Sign"" + dotnet toolchain.cs -- resilience --fast dotnet toolchain.cs -- test --integration --project Piv --smoke dotnet toolchain.cs coverage dotnet toolchain.cs publish --package-version 1.0.0-preview.1 From b24077c68120b71407dca175ea94ec1659a30faa Mon Sep 17 00:00:00 2001 From: Dennis Dyallo Date: Tue, 16 Jun 2026 17:41:32 +0200 Subject: [PATCH 09/25] docs: defer runtime diagnostics project --- docs/learnings/runtime-resilience/README.md | 1 + .../phase-06-diagnostics-project-deferred.md | 41 +++++++++++++++++++ docs/plans/runtime-resilience-harness.md | 13 ++++++ 3 files changed, 55 insertions(+) create mode 100644 docs/learnings/runtime-resilience/phase-06-diagnostics-project-deferred.md diff --git a/docs/learnings/runtime-resilience/README.md b/docs/learnings/runtime-resilience/README.md index f96019d3a..8f51e9fbd 100644 --- a/docs/learnings/runtime-resilience/README.md +++ b/docs/learnings/runtime-resilience/README.md @@ -9,6 +9,7 @@ This folder records phase closeouts for the runtime resilience workstream. Each - [Phase 3: Static Runtime-Resilience Scanner](phase-03-static-scanner.md) - [Phase 4: SmartCard Context Leak Invariant](phase-04-smartcard-context-leak.md) - [Phase 5: Minimal Fast Runner](phase-05-minimal-fast-runner.md) +- [Phase 6: Diagnostics Project Deferred](phase-06-diagnostics-project-deferred.md) ## Closeout Rule diff --git a/docs/learnings/runtime-resilience/phase-06-diagnostics-project-deferred.md b/docs/learnings/runtime-resilience/phase-06-diagnostics-project-deferred.md new file mode 100644 index 000000000..31ffacd3b --- /dev/null +++ b/docs/learnings/runtime-resilience/phase-06-diagnostics-project-deferred.md @@ -0,0 +1,41 @@ +# Phase 6: Diagnostics Project Deferred + +## What We Found + +- The fast runner is useful without a separate diagnostics project. +- Current resilience gates cover several bug classes, but they all live naturally as Core no-hardware unit tests. +- A diagnostics project would not yet add a new invariant, runner pattern, live scenario, or report that changes decisions. + +## How We Got Better At Finding + +- The previous phases improved detection by adding focused tests and a single toolchain target, not by adding infrastructure. +- The Phase 5 runner gave us the orchestration needed for the current gates. +- The Phase 6 decision gate prevented us from mistaking “we could build a project” for “a project is useful.” + +## What Worked + +- The plan’s promotion rule worked: build diagnostics only after reuse across modules or live scenarios justifies it. +- Existing unit-test output already gives the pass/fail signal we need. +- The `Category=RuntimeResilience` trait is enough scenario selection for now. + +## What Did Not Work + +- A scenario registry would be empty or duplicate test names today. +- JSON/markdown diagnostics reports would repeat test output without adding an asserted invariant. +- Live-hardware diagnostics are still undefined and should not be invented just to populate a project. + +## Review Findings And Fixes + +- DevTeam validated the defer decision but found the initial checklist representation misleading because deferred deliverables were marked complete. +- We changed Phase 6 to mark only the promotion-gate evaluation complete and leave the deferred diagnostics-project deliverables unchecked. +- DevTeam identified live OS-handle/fd diagnostics as the leading reopen trigger, so the plan now names that explicitly. + +## Remaining Risk + +- Deferring diagnostics means live fd/handle checks remain manual until a real live scenario is defined. +- The fast runner is Core-only. If another module adds runtime-resilience gates, the runner may need to widen before a diagnostics project is still justified. + +## Verification + +- Phase 5 runner evidence remains the verification substrate: `dotnet toolchain.cs -- resilience --fast` passed 13 runtime-resilience tests in under 90 seconds. +- No new code was added in this phase. diff --git a/docs/plans/runtime-resilience-harness.md b/docs/plans/runtime-resilience-harness.md index d6c61e808..3037a0d28 100644 --- a/docs/plans/runtime-resilience-harness.md +++ b/docs/plans/runtime-resilience-harness.md @@ -172,12 +172,25 @@ Proof of value required before moving on: ## Phase 6: Dedicated Diagnostics Project, Only If Justified +- [x] Evaluate promotion gate for `diagnostics/Yubico.YubiKit.RuntimeDiagnostics`. - [ ] Create `diagnostics/Yubico.YubiKit.RuntimeDiagnostics` only if the same runner patterns are reused across at least three modules or bug classes. - [ ] Add scenario registry only after multiple scenarios exist. - [ ] Add JSON/markdown reports only for asserted invariants. - [ ] Record device serial, model, firmware, and transport for every live result. - [ ] Default to read-only, non-destructive operations. +Decision: +- Do not create `diagnostics/Yubico.YubiKit.RuntimeDiagnostics` yet. +- Current gates cover multiple bug classes, but they are all no-hardware Core unit-test gates and already run through `dotnet toolchain.cs -- resilience --fast`. +- There is no live-hardware scenario registry yet, no repeated cross-module runner pattern, and no asserted JSON/markdown report that would add value over test output. +- Reopen this phase after a live OS-handle/fd diagnostic is approved, at least one live optional diagnostic exists, or another module-specific runtime-resilience gate needs orchestration outside unit tests. + +Evidence: +- The fast runner already catches the known SmartCard and OTP seeded regressions in under 90 seconds. +- No default live scenario is approved yet, so a diagnostics project would currently be empty orchestration. +- Keeping diagnostics deferred follows the Phase Closeout Gate and avoids building shelfware. +- DevTeam review validated the defer decision and corrected the checklist representation so deferred deliverables remain unchecked. + Do not build yet: - Cross-module all-applet orchestration. - General CPU/memory/GC graphing without invariants. From 0363a3d15c781a0609890cbf53188b2622614820 Mon Sep 17 00:00:00 2001 From: Dennis Dyallo Date: Tue, 16 Jun 2026 17:45:29 +0200 Subject: [PATCH 10/25] docs: defer runtime audit skill --- docs/learnings/runtime-resilience/README.md | 1 + .../phase-07-audit-skill-deferred.md | 40 +++++++++++++++++++ docs/plans/runtime-resilience-harness.md | 15 ++++++- 3 files changed, 55 insertions(+), 1 deletion(-) create mode 100644 docs/learnings/runtime-resilience/phase-07-audit-skill-deferred.md diff --git a/docs/learnings/runtime-resilience/README.md b/docs/learnings/runtime-resilience/README.md index 8f51e9fbd..8e56d262e 100644 --- a/docs/learnings/runtime-resilience/README.md +++ b/docs/learnings/runtime-resilience/README.md @@ -10,6 +10,7 @@ This folder records phase closeouts for the runtime resilience workstream. Each - [Phase 4: SmartCard Context Leak Invariant](phase-04-smartcard-context-leak.md) - [Phase 5: Minimal Fast Runner](phase-05-minimal-fast-runner.md) - [Phase 6: Diagnostics Project Deferred](phase-06-diagnostics-project-deferred.md) +- [Phase 7: Audit Skill Deferred](phase-07-audit-skill-deferred.md) ## Closeout Rule diff --git a/docs/learnings/runtime-resilience/phase-07-audit-skill-deferred.md b/docs/learnings/runtime-resilience/phase-07-audit-skill-deferred.md new file mode 100644 index 000000000..cb7020edd --- /dev/null +++ b/docs/learnings/runtime-resilience/phase-07-audit-skill-deferred.md @@ -0,0 +1,40 @@ +# Phase 7: Audit Skill Deferred + +## What We Found + +- A reusable audit skill is not justified yet. +- The current runtime-resilience workflow has one stable command: `dotnet toolchain.cs -- resilience --fast`. +- Without a diagnostics project, live diagnostic mode, or multi-command audit workflow, a skill would only wrap a single command. + +## How We Got Better At Finding + +- Earlier phases showed that useful gates should start as concrete tests and toolchain commands. +- Phase 6 prevented an empty diagnostics project; Phase 7 applies the same standard to skill creation. +- The closeout gate now treats “do not build yet” as a valid phase outcome when evidence does not justify more infrastructure. + +## What Worked + +- The command-line runner is simple enough that a skill would not reduce cognitive load yet. +- Keeping the workflow in `toolchain.cs` makes verification executable and repo-local. +- Deferring the skill avoids duplicating instructions that would drift from the runner. + +## What Did Not Work + +- Creating a skill before multiple commands or diagnostic modes exist would create maintenance surface without new detection power. +- A report recommendation layer would be speculative because the runner already gives direct pass/fail output. + +## Review Findings And Fixes + +- DevTeam validated the defer decision and found no concrete skill capability that adds value beyond `dotnet toolchain.cs -- resilience --fast` today. +- DevTeam noted that discoverability is the only current residual value a skill might add, which is a documentation concern rather than orchestration. +- We linked the reopen trigger to Phase 6 reopening because a diagnostics project or live diagnostic would change the skill calculus. + +## Remaining Risk + +- Users must know the direct command until a skill becomes useful. +- If Phase 6 reopens, live diagnostics are added, or multiple runner modes are added later, the skill decision should be revisited. + +## Verification + +- Phase 5 runner evidence remains the proof of value: `dotnet toolchain.cs -- resilience --fast` catches the known seeded regressions and runs under 90 seconds. +- No skill files were created in this phase. diff --git a/docs/plans/runtime-resilience-harness.md b/docs/plans/runtime-resilience-harness.md index 3037a0d28..350aa0952 100644 --- a/docs/plans/runtime-resilience-harness.md +++ b/docs/plans/runtime-resilience-harness.md @@ -199,12 +199,25 @@ Do not build yet: ## Phase 7: Reusable Local Audit Skill, Last +- [x] Evaluate promotion gate for a reusable `yubikit-runtime-audit` skill. - [ ] Create a `yubikit-runtime-audit` skill only after the toolchain/local runner shape stabilizes. - [ ] The skill should orchestrate existing commands, not invent parallel behavior. - [ ] It should produce a concise report and recommend next probes. +Decision: +- Do not create a `yubikit-runtime-audit` skill yet. +- The stable user interface is currently one command: `dotnet toolchain.cs -- resilience --fast`. +- A skill would mostly wrap that single command and restate the plan; it would not add meaningful orchestration until diagnostics or multiple runner modes exist. +- Reopen this phase after Phase 6 reopens, a diagnostics project exists, a live optional diagnostic exists, or a multi-command audit workflow exists. + +Evidence: +- The command-line harness is already useful without a skill. +- Phase 6 deferred the diagnostics project, so there is no scenario registry or report layer for a skill to orchestrate. +- Avoiding the skill follows the same anti-shelfware rule used for the diagnostics project. +- DevTeam review validated the defer decision and found no concrete skill capability that adds value beyond the single runner command today. + Proof of value required: -- The command-line harness is already useful without the skill. +- The command-line harness is already useful without the skill. Satisfied by Phase 5 red-green runner evidence. ## Live-Hardware Safety Rules From 58ad07cff79a71938c1e7e5b62d71017a69f8777 Mon Sep 17 00:00:00 2001 From: Dennis Dyallo Date: Tue, 16 Jun 2026 20:50:20 +0200 Subject: [PATCH 11/25] ci: add runtime resilience check --- .github/workflows/build.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 30ab2213c..6519cdf1d 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -55,6 +55,9 @@ jobs: - name: Run unit tests run: dotnet toolchain.cs test + - name: Run runtime resilience checks + run: dotnet toolchain.cs -- resilience --fast + - name: Pack NuGet packages run: dotnet toolchain.cs pack --package-version 2.0.0-preview.${{ github.run_number }} From 58c4ee9e588e2409aece24eb19b00cc102fe838f Mon Sep 17 00:00:00 2001 From: Dennis Dyallo Date: Tue, 16 Jun 2026 21:08:01 +0200 Subject: [PATCH 12/25] docs: fold runtime resilience learnings forward --- docs/learnings/runtime-resilience/README.md | 8 ++++++++ docs/plans/runtime-resilience-harness.md | 21 +++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/docs/learnings/runtime-resilience/README.md b/docs/learnings/runtime-resilience/README.md index 8e56d262e..1207ddf56 100644 --- a/docs/learnings/runtime-resilience/README.md +++ b/docs/learnings/runtime-resilience/README.md @@ -21,3 +21,11 @@ Each future phase should close in this order: 3. Run cross-vendor review when the change affects runtime behavior or proof quality. 4. Write the phase learning document. 5. Commit the phase before starting the next one. + +## What The Next Work Should Do + +- Use `dotnet toolchain.cs -- resilience --fast` as the default runtime-resilience verification command. +- Start from a concrete bug seed or missing invariant, not from a desire to add infrastructure. +- Prefer no-hardware proof first: fake seams, deterministic timing/resource-release invariants, or seeded scanner fixtures. +- Use BenchmarkDotNet for discovery and evidence, then convert proven foreground regressions into cheap gates. +- Keep the diagnostics project and audit skill deferred until the workflow grows beyond the current single-command runner. diff --git a/docs/plans/runtime-resilience-harness.md b/docs/plans/runtime-resilience-harness.md index 350aa0952..a50edf846 100644 --- a/docs/plans/runtime-resilience-harness.md +++ b/docs/plans/runtime-resilience-harness.md @@ -219,6 +219,27 @@ Evidence: Proof of value required: - The command-line harness is already useful without the skill. Satisfied by Phase 5 red-green runner evidence. +## Next-Step Rules From Current Learnings + +- Treat `dotnet toolchain.cs -- resilience --fast` as the default local and CI gate for any change that touches Core runtime loops, polling paths, recovery logic, or listener lifecycle cleanup. +- Start the next resilience slice only from a concrete bug seed, suspicious runtime behavior, or a specific missing invariant. Do not start from a desire to add more diagnostics infrastructure. +- Prefer the smallest new detector that proves the bug class: existing unit test, fake seam, seeded-source scanner fixture, or strict resource-release invariant. +- Use BenchmarkDotNet to discover or confirm a foreground performance problem, then convert the finding into the cheapest stable regression gate that can run without hardware. +- Keep new gates no-hardware by default. Add live optional diagnostics only when the bug class cannot be proven credibly with a fake seam or deterministic local invariant. +- Reopen Phase 6 only if at least one of these becomes true: + - a live OS handle or file-descriptor diagnostic is approved, + - another module needs runtime-resilience orchestration outside normal unit tests, + - multiple live optional scenarios exist and need a shared runner/report layer. +- Reopen Phase 7 only after Phase 6 reopens or the workflow grows beyond one stable command. +- Keep the closeout loop strict for every future slice: smallest useful change, focused verification, broader gate when code changed, cross-vendor review when behavior or proof changes, learning doc, then commit. + +## Current Recommended Next Slice Order + +1. Add another no-hardware invariant only when a new Core runtime risk is observed in review, profiling, or bug investigation. +2. If a real risk cannot be proven with the current seams, define one narrowly scoped live optional diagnostic and keep it read-only by default. +3. Only after step 2 produces a real orchestration need, reconsider a diagnostics project. +4. Only after step 3 creates multiple commands or modes, reconsider a reusable audit skill. + ## Live-Hardware Safety Rules - [ ] Default live scenarios must be read-only. From 2a4c67bcc36e3367e30a5d7dbc2d2d0b1d0a1b60 Mon Sep 17 00:00:00 2001 From: Dennis Dyallo Date: Wed, 17 Jun 2026 00:53:17 +0200 Subject: [PATCH 13/25] docs(core): require resilience runner for loop changes --- AGENTS.md | 1 + src/Core/CLAUDE.md | 2 ++ 2 files changed, 3 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 10c43b720..7ccdac324 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -10,6 +10,7 @@ - Do not run `dotnet build` directly. Use `dotnet toolchain.cs build` or `dotnet toolchain.cs build --project Piv`. - Do not run `dotnet test` directly. Unit tests mix xUnit v3/Microsoft Testing Platform and xUnit v2; `dotnet toolchain.cs test` detects and invokes the right runner. - Focus tests with both module and filter when possible: `dotnet toolchain.cs test --project Fido2 --filter "Method~Sign"`. +- When touching Core runtime loops, polling paths, recovery logic, or listener lifecycle cleanup, also run `dotnet toolchain.cs -- resilience --fast`. - `--integration` requires `--project`: `dotnet toolchain.cs -- test --integration --project Piv --smoke`. - `--smoke` skips `Slow` and `RequiresUserPresence`; agents should not run touch/insert/remove tests unless a human explicitly coordinates hardware. - Use `dotnet toolchain.cs -- --help` when arguments act strangely; `--help` and some options need the `--` separator. diff --git a/src/Core/CLAUDE.md b/src/Core/CLAUDE.md index bef68c3a2..67f3a5723 100644 --- a/src/Core/CLAUDE.md +++ b/src/Core/CLAUDE.md @@ -73,6 +73,8 @@ If using DI, configure logging explicitly from the DI-provided `ILoggerFactory` Background listeners and native/resource-manager retry loops must block, back off, exit, or throttle on every failure path. Do not ignore native return values inside loops unless another call in the same path provides a bounded wait. Persistent failures such as stale PC/SC handles must have no-hardware fault-injection tests that prove call cadence is backoff-bounded. +If a change touches Core runtime loops, polling paths, recovery logic, or listener lifecycle cleanup, run `dotnet toolchain.cs -- resilience --fast` in addition to the normal focused tests. Prefer adding or extending no-hardware `Category=RuntimeResilience` coverage before considering live diagnostics. + ### APDU Processing Pipeline The APDU processing pipeline uses the decorator pattern: From b933d2a462afbd92e6d141444b8363c5b0535304 Mon Sep 17 00:00:00 2001 From: Dennis Dyall Date: Tue, 23 Jun 2026 18:37:31 +0200 Subject: [PATCH 14/25] feat(core): add Windows HID transport --- .../src/Native/Windows/Cfgmgr32/CmDevice.cs | 4 +- .../src/Native/Windows/HidD/HidD.Interop.cs | 9 +- .../src/Native/Windows/HidD/HidDDevice.cs | 129 +++++++++++---- .../Windows/Kernel32/Kernel32.Interop.cs | 4 +- src/Core/src/Transports/Hid/FindHidDevices.cs | 16 +- src/Core/src/Transports/Hid/HidConstants.cs | 20 +++ .../Transports/Hid/Linux/LinuxHidDevice.cs | 3 +- .../Transports/Hid/MacOS/MacOSHidDevice.cs | 3 +- .../Hid/Windows/WindowsHidDevice.cs | 107 +++++++++++++ .../WindowsHidFeatureReportConnection.cs | 147 ++++++++---------- .../Windows/WindowsHidIOReportConnection.cs | 147 ++++++++---------- .../Transports/Hid/HidEnumerationTests.cs | 18 +-- 12 files changed, 373 insertions(+), 234 deletions(-) create mode 100644 src/Core/src/Transports/Hid/HidConstants.cs create mode 100644 src/Core/src/Transports/Hid/Windows/WindowsHidDevice.cs diff --git a/src/Core/src/Native/Windows/Cfgmgr32/CmDevice.cs b/src/Core/src/Native/Windows/Cfgmgr32/CmDevice.cs index 86dda5f8d..16ee11266 100644 --- a/src/Core/src/Native/Windows/Cfgmgr32/CmDevice.cs +++ b/src/Core/src/Native/Windows/Cfgmgr32/CmDevice.cs @@ -194,7 +194,7 @@ private void ResolveHidUsages() } } - private static IList GetDevicePaths(Guid classGuid, string? deviceInstanceId) + internal static IList GetDevicePaths(Guid classGuid, string? deviceInstanceId) { var errorCode = NativeMethods.CM_Get_Device_Interface_List_Size( out var bufferLength, @@ -216,7 +216,7 @@ private static IList GetDevicePaths(Guid classGuid, string? deviceInstan classGuid, deviceInstanceId, buffer, - bufferLength * 2, + bufferLength, NativeMethods.CM_GET_DEVICE_LIST.PRESENT ); diff --git a/src/Core/src/Native/Windows/HidD/HidD.Interop.cs b/src/Core/src/Native/Windows/HidD/HidD.Interop.cs index a4db41264..3822dfb9e 100644 --- a/src/Core/src/Native/Windows/HidD/HidD.Interop.cs +++ b/src/Core/src/Native/Windows/HidD/HidD.Interop.cs @@ -32,12 +32,12 @@ internal struct HIDD_ATTRIBUTES [StructLayout(LayoutKind.Sequential, Pack = 1)] internal unsafe struct HIDP_CAPS { - public fixed short Reserved[17]; public short Usage; public short UsagePage; public short InputReportByteLength; public short OutputReportByteLength; public short FeatureReportByteLength; + public fixed short Reserved[17]; public short NumberLinkCollectionNodes; public short NumberInputButtonCaps; @@ -53,7 +53,7 @@ internal unsafe struct HIDP_CAPS public short NumberFeatureDataIndices; } - + internal const int HidpStatusSuccess = 0x00110000; [LibraryImport(Libraries.Hid, SetLastError = true)] [DefaultDllImportSearchPaths(DllImportSearchPath.System32)] @@ -67,7 +67,7 @@ IntPtr PreparsedData [return: MarshalAs(UnmanagedType.Bool)] internal static partial bool HidD_GetAttributes( SafeFileHandle HidDeviceObject, - out HIDD_ATTRIBUTES Attributes + ref HIDD_ATTRIBUTES Attributes ); [LibraryImport(Libraries.Hid, SetLastError = true)] @@ -116,8 +116,7 @@ int BufferLength [LibraryImport(Libraries.Hid, SetLastError = true)] [DefaultDllImportSearchPaths(DllImportSearchPath.System32)] - [return: MarshalAs(UnmanagedType.Bool)] - internal static partial bool HidP_GetCaps( + internal static partial int HidP_GetCaps( IntPtr PreparsedData, ref HIDP_CAPS Capabilities ); diff --git a/src/Core/src/Native/Windows/HidD/HidDDevice.cs b/src/Core/src/Native/Windows/HidD/HidDDevice.cs index ee4eec6ec..25d61133d 100644 --- a/src/Core/src/Native/Windows/HidD/HidDDevice.cs +++ b/src/Core/src/Native/Windows/HidD/HidDDevice.cs @@ -17,16 +17,17 @@ namespace Yubico.YubiKit.Core.Native.Windows.HidD; -internal class HidDDevice : IHidDDevice +internal sealed class HidDDevice : IHidDDevice { + private const int ErrorAccessDenied = 5; private SafeFileHandle _handle; + private bool _disposed; public HidDDevice(string devicePath) { DevicePath = devicePath; - _handle = OpenHandleWithAccess(Kernel32.NativeMethods.DESIRED_ACCESS.NONE); - var capabilities = GetCapabilities(_handle); + _handle = OpenHandleForMetadata(out var capabilities); Usage = capabilities.Usage; UsagePage = capabilities.UsagePage; @@ -44,27 +45,21 @@ public HidDDevice(string devicePath) public short FeatureReportByteLength { get; } public void OpenIOConnection() - { - _handle.Dispose(); - _handle = OpenHandleWithAccess(Kernel32.NativeMethods.DESIRED_ACCESS.GENERIC_READ | - Kernel32.NativeMethods.DESIRED_ACCESS.GENERIC_WRITE); - } + => OpenReportConnection(); public void OpenFeatureConnection() - { - _handle.Dispose(); - _handle = OpenHandleWithAccess(Kernel32.NativeMethods.DESIRED_ACCESS.GENERIC_WRITE); - } + => OpenReportConnection(); public byte[] GetFeatureReport() { - if (_handle is null) throw new InvalidOperationException("ExceptionMessages.InvalidSafeFileHandle"); + EnsureOpenHandle(); var buffer = new byte[FeatureReportByteLength]; if (!NativeMethods.HidD_GetFeature(_handle, buffer, buffer.Length)) Marshal.ThrowExceptionForHR(Marshal.GetHRForLastWin32Error()); + // Windows includes the report ID byte; the SDK exposes only report payload bytes. var returnBuf = new byte[FeatureReportByteLength - 1]; Array.Copy(buffer, 1, returnBuf, 0, returnBuf.Length); @@ -73,11 +68,12 @@ public byte[] GetFeatureReport() public void SetFeatureReport(byte[] buffer) { - if (_handle is null) throw new InvalidOperationException("ExceptionMessages.InvalidSafeFileHandle"); + EnsureOpenHandle(); if (buffer.Length != FeatureReportByteLength - 1) - throw new InvalidOperationException("ExceptionMessages.InvalidReportBufferLength"); + throw new InvalidOperationException("The HID feature report buffer length is invalid."); + // Windows expects the report ID byte before the report payload. var sendBuf = new byte[buffer.Length + 1]; Array.Copy(buffer, 0, sendBuf, 1, buffer.Length); @@ -87,13 +83,14 @@ public void SetFeatureReport(byte[] buffer) public byte[] GetInputReport() { - if (_handle is null) throw new InvalidOperationException("ExceptionMessages.InvalidSafeFileHandle"); + EnsureOpenHandle(); var buffer = new byte[InputReportByteLength]; if (!Kernel32.NativeMethods.ReadFile(_handle, buffer, buffer.Length, out var bytesRead, IntPtr.Zero) || bytesRead != buffer.Length) Marshal.ThrowExceptionForHR(Marshal.GetHRForLastWin32Error()); + // Windows includes the report ID byte; the SDK exposes only report payload bytes. var returnBuf = new byte[InputReportByteLength - 1]; Array.Copy(buffer, 1, returnBuf, 0, returnBuf.Length); @@ -102,10 +99,12 @@ public byte[] GetInputReport() public void SetOutputReport(byte[] buffer) { - if (_handle is null) throw new InvalidOperationException("ExceptionMessages.InvalidSafeFileHandle"); + EnsureOpenHandle(); + if (buffer.Length != OutputReportByteLength - 1) - throw new InvalidOperationException("ExceptionMessages.InvalidReportBufferLength"); + throw new InvalidOperationException("The HID output report buffer length is invalid."); + // Windows expects the report ID byte before the report payload. var sendBuf = new byte[buffer.Length + 1]; Array.Copy(buffer, 0, sendBuf, 1, buffer.Length); @@ -120,12 +119,14 @@ private static NativeMethods.HIDP_CAPS GetCapabilities(SafeFileHandle safeHandle NativeMethods.HIDP_CAPS capabilities = new(); if (!NativeMethods.HidD_GetPreparsedData(safeHandle, out var preparsedData)) - Marshal.ThrowExceptionForHR(Marshal.GetHRForLastWin32Error()); + ThrowHidDWin32Failure(nameof(NativeMethods.HidD_GetPreparsedData), "Failed to get HID preparsed data."); try { - if (!NativeMethods.HidP_GetCaps(preparsedData, ref capabilities)) - Marshal.ThrowExceptionForHR(Marshal.GetHRForLastWin32Error()); + var result = NativeMethods.HidP_GetCaps(preparsedData, ref capabilities); + if (result != NativeMethods.HidpStatusSuccess) + throw new PlatformApiException(nameof(NativeMethods.HidP_GetCaps), result, + "Failed to get HID capabilities."); return capabilities; } @@ -140,32 +141,96 @@ private SafeFileHandle OpenHandleWithAccess(Kernel32.NativeMethods.DESIRED_ACCES var handle = Kernel32.NativeMethods.CreateFile( DevicePath, desiredAccess, - Kernel32.NativeMethods.FILE_SHARE.READWRITE, + Kernel32.NativeMethods.FILE_SHARE.ALL, IntPtr.Zero, Kernel32.NativeMethods.CREATION_DISPOSITION.OPEN_EXISTING, Kernel32.NativeMethods.FILE_FLAG.NORMAL, IntPtr.Zero ); - if (handle.IsInvalid) Marshal.ThrowExceptionForHR(Marshal.GetHRForLastWin32Error()); + if (handle.IsInvalid) + { + var error = Marshal.GetLastWin32Error(); + if (error == ErrorAccessDenied) + throw new UnauthorizedAccessException($"Access denied opening HID device '{DevicePath}'."); + + throw new PlatformApiException(nameof(Kernel32.NativeMethods.CreateFile), error, + $"Failed to open HID device '{DevicePath}'."); + } return handle; } + private SafeFileHandle OpenHandleForMetadata(out NativeMethods.HIDP_CAPS capabilities) + { + try + { + var handle = OpenHandleWithAccess(Kernel32.NativeMethods.DESIRED_ACCESS.NONE); + try + { + capabilities = GetCapabilities(handle); + return handle; + } + catch + { + handle.Dispose(); + throw; + } + } + catch (Exception ex) when (RequiresReadWriteMetadataHandle(ex)) + { + var handle = OpenReadWriteHandle(); + try + { + capabilities = GetCapabilities(handle); + return handle; + } + catch + { + handle.Dispose(); + throw; + } + } + } + + private void OpenReportConnection() + { + var handle = OpenReadWriteHandle(); + _handle.Dispose(); + _handle = handle; + } + + private SafeFileHandle OpenReadWriteHandle() + => OpenHandleWithAccess(Kernel32.NativeMethods.DESIRED_ACCESS.GENERIC_READ | + Kernel32.NativeMethods.DESIRED_ACCESS.GENERIC_WRITE); - private bool disposedValue; // To detect redundant calls + private static bool RequiresReadWriteMetadataHandle(Exception exception) + => exception is UnauthorizedAccessException; - protected virtual void Dispose(bool disposing) + private static void ThrowHidDWin32Failure(string source, string message) { - if (!disposedValue) - { - if (disposing) _handle.Dispose(); + var error = Marshal.GetLastWin32Error(); + if (error == ErrorAccessDenied) + throw new UnauthorizedAccessException(message); - disposedValue = true; - } + throw new PlatformApiException(source, error, message); } - // This code added to correctly implement the disposable pattern. - public void Dispose() => Dispose(true); + private void EnsureOpenHandle() + { + ObjectDisposedException.ThrowIf(_disposed, this); + + if (_handle.IsInvalid || _handle.IsClosed) + throw new InvalidOperationException($"The HID device handle for '{DevicePath}' is not open."); + } + + public void Dispose() + { + if (_disposed) + return; + + _handle.Dispose(); + _disposed = true; + } } \ No newline at end of file diff --git a/src/Core/src/Native/Windows/Kernel32/Kernel32.Interop.cs b/src/Core/src/Native/Windows/Kernel32/Kernel32.Interop.cs index 32cf96ec6..b38208684 100644 --- a/src/Core/src/Native/Windows/Kernel32/Kernel32.Interop.cs +++ b/src/Core/src/Native/Windows/Kernel32/Kernel32.Interop.cs @@ -153,7 +153,7 @@ internal enum FILE_FLAG [LibraryImport(Libraries.Kernel32, EntryPoint = "CreateFileW", SetLastError = true, - StringMarshalling = StringMarshalling.Utf8)] + StringMarshalling = StringMarshalling.Utf16)] [DefaultDllImportSearchPaths(DllImportSearchPath.System32)] internal static partial SafeFileHandle CreateFile( string lpFileName, @@ -188,4 +188,4 @@ internal static partial bool ReadFile( IntPtr mustBeZero ); -} \ No newline at end of file +} diff --git a/src/Core/src/Transports/Hid/FindHidDevices.cs b/src/Core/src/Transports/Hid/FindHidDevices.cs index 57c933578..b0e76e0f6 100644 --- a/src/Core/src/Transports/Hid/FindHidDevices.cs +++ b/src/Core/src/Transports/Hid/FindHidDevices.cs @@ -15,9 +15,9 @@ using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using System.Runtime.Versioning; -using Yubico.YubiKit.Core.Transports.Hid; using Yubico.YubiKit.Core.Transports.Hid.Linux; using Yubico.YubiKit.Core.Transports.Hid.MacOS; +using Yubico.YubiKit.Core.Transports.Hid.Windows; namespace Yubico.YubiKit.Core.Transports.Hid; @@ -28,8 +28,6 @@ public interface IFindHidDevices public class FindHidDevices(ILogger logger) : IFindHidDevices { - private const short YubicoVendorId = 0x1050; - public async Task> FindAllAsync(CancellationToken cancellationToken = default) => await Task.Run(FindAll, cancellationToken).ConfigureAwait(false); @@ -40,7 +38,7 @@ private IReadOnlyList FindAll() var allDevices = GetPlatformDevices(); var yubicoDevices = allDevices - .Where(d => d.DescriptorInfo.VendorId == YubicoVendorId) + .Where(d => d.DescriptorInfo.VendorId == HidConstants.YubicoVendorId) .ToList(); logger.LogDebug("Found {Count} Yubico HID devices", yubicoDevices.Count); @@ -60,7 +58,11 @@ private IReadOnlyList GetPlatformDevices() return FindAllLinux(); } - // Windows not yet implemented, return empty list + if (OperatingSystem.IsWindows()) + { + return FindAllWindows(); + } + return []; } @@ -82,6 +84,10 @@ private IReadOnlyList FindAllLinux() } } + [SupportedOSPlatform("windows")] + private static IReadOnlyList FindAllWindows() => + WindowsHidDevice.GetList(); + public static FindHidDevices Create(ILogger? logger = null) => new(logger ?? NullLogger.Instance); } \ No newline at end of file diff --git a/src/Core/src/Transports/Hid/HidConstants.cs b/src/Core/src/Transports/Hid/HidConstants.cs new file mode 100644 index 000000000..5bdbc598f --- /dev/null +++ b/src/Core/src/Transports/Hid/HidConstants.cs @@ -0,0 +1,20 @@ +// Copyright 2026 Yubico AB +// +// Licensed under the Apache License, Version 2.0 (the "License"). +// You may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +namespace Yubico.YubiKit.Core.Transports.Hid; + +internal static class HidConstants +{ + public const short YubicoVendorId = 0x1050; +} \ No newline at end of file diff --git a/src/Core/src/Transports/Hid/Linux/LinuxHidDevice.cs b/src/Core/src/Transports/Hid/Linux/LinuxHidDevice.cs index feb62f7bd..6696da70e 100644 --- a/src/Core/src/Transports/Hid/Linux/LinuxHidDevice.cs +++ b/src/Core/src/Transports/Hid/Linux/LinuxHidDevice.cs @@ -17,7 +17,6 @@ using Yubico.YubiKit.Core.Native; using Yubico.YubiKit.Core.Native.Linux.Libc; using Yubico.YubiKit.Core.Native.Linux.Udev; -using Yubico.YubiKit.Core.Transports.Hid; using LibcNativeMethods = Yubico.YubiKit.Core.Native.Linux.Libc.NativeMethods; using UdevNativeMethods = Yubico.YubiKit.Core.Native.Linux.Udev.NativeMethods; @@ -110,7 +109,7 @@ public static IReadOnlyList GetList() var descriptorInfo = ParseHidDescriptor(device); // Only include Yubico devices with supported interface types - if (descriptorInfo.VendorId == 0x1050 && + if (descriptorInfo.VendorId == HidConstants.YubicoVendorId && HidInterfaceClassifier.IsSupported(descriptorInfo)) { devices.Add(new LinuxHidDevice(descriptorInfo)); diff --git a/src/Core/src/Transports/Hid/MacOS/MacOSHidDevice.cs b/src/Core/src/Transports/Hid/MacOS/MacOSHidDevice.cs index faba34eb9..e1c698ba8 100644 --- a/src/Core/src/Transports/Hid/MacOS/MacOSHidDevice.cs +++ b/src/Core/src/Transports/Hid/MacOS/MacOSHidDevice.cs @@ -16,7 +16,6 @@ using System.Runtime.Versioning; using Yubico.YubiKit.Core.Native; using Yubico.YubiKit.Core.Native.MacOS.IOKitFramework; -using Yubico.YubiKit.Core.Transports.Hid; using CFNativeMethods = Yubico.YubiKit.Core.Native.MacOS.CoreFoundation.NativeMethods; using IOKitNativeMethods = Yubico.YubiKit.Core.Native.MacOS.IOKitFramework.NativeMethods; @@ -85,7 +84,7 @@ public static IReadOnlyList GetList() }; // Only include Yubico devices with supported interface types - if (descriptorInfo.VendorId == 0x1050 && + if (descriptorInfo.VendorId == HidConstants.YubicoVendorId && HidInterfaceClassifier.IsSupported(descriptorInfo)) { result.Add(new MacOSHidDevice(GetEntryId(device), descriptorInfo)); diff --git a/src/Core/src/Transports/Hid/Windows/WindowsHidDevice.cs b/src/Core/src/Transports/Hid/Windows/WindowsHidDevice.cs new file mode 100644 index 000000000..05c391cf5 --- /dev/null +++ b/src/Core/src/Transports/Hid/Windows/WindowsHidDevice.cs @@ -0,0 +1,107 @@ +// Copyright 2026 Yubico AB +// +// Licensed under the Apache License, Version 2.0 (the "License"). +// You may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using Microsoft.Extensions.Logging; +using System.Runtime.Versioning; +using Yubico.YubiKit.Core.Native; +using Yubico.YubiKit.Core.Native.Windows.Cfgmgr32; +using Cfgmgr32NativeMethods = Yubico.YubiKit.Core.Native.Windows.Cfgmgr32.NativeMethods; + +namespace Yubico.YubiKit.Core.Transports.Hid.Windows; + +/// +/// Windows implementation of a Human Interface Device (HID). +/// +[SupportedOSPlatform("windows")] +internal sealed class WindowsHidDevice : IHidDevice +{ + private static readonly ILogger Logger = YubiKitLogging.CreateLogger(); + + private readonly string _devicePath; + + private WindowsHidDevice(HidDescriptorInfo descriptorInfo) + { + DescriptorInfo = descriptorInfo; + InterfaceType = HidInterfaceClassifier.Classify(descriptorInfo); + _devicePath = descriptorInfo.DevicePath; + } + + public string ReaderName => _devicePath; + + public HidDescriptorInfo DescriptorInfo { get; } + + public HidInterfaceType InterfaceType { get; } + + public static IReadOnlyList GetList() + { + var devices = new List(); + + foreach (var interfacePath in CmDevice.GetDevicePaths(CmInterfaceGuid.Hid, null)) + { + if (string.IsNullOrEmpty(interfacePath)) + continue; + + try + { + var descriptorInfo = GetDescriptorInfo(interfacePath); + + if (descriptorInfo.VendorId == HidConstants.YubicoVendorId && + HidInterfaceClassifier.IsSupported(descriptorInfo)) + { + devices.Add(new WindowsHidDevice(descriptorInfo)); + } + } + catch (PlatformApiException ex) + { + Logger.LogDebug(ex, "Skipping HID interface {InterfacePath}: ConfigMgr property read failed", interfacePath); + // Ignore inaccessible or transient HID interfaces; one bad device must not abort discovery. + } + catch (NotSupportedException ex) + { + Logger.LogDebug(ex, "Skipping HID interface {InterfacePath}: unsupported or malformed ConfigMgr HID properties", interfacePath); + // Ignore HID interfaces with unsupported/malformed ConfigMgr properties. + } + } + + return devices; + } + + private static HidDescriptorInfo GetDescriptorInfo(string interfacePath) => + new() + { + UsagePage = GetInterfaceProperty(interfacePath, Cfgmgr32NativeMethods.DEVPKEY_DeviceInterface_HID_UsagePage), + Usage = GetInterfaceProperty(interfacePath, Cfgmgr32NativeMethods.DEVPKEY_DeviceInterface_HID_UsageId), + DevicePath = interfacePath, + VendorId = unchecked((short)GetInterfaceProperty(interfacePath, Cfgmgr32NativeMethods.DEVPKEY_DeviceInterface_HID_VendorId)), + ProductId = unchecked((short)GetInterfaceProperty(interfacePath, Cfgmgr32NativeMethods.DEVPKEY_DeviceInterface_HID_ProductId)) + }; + + private static T GetInterfaceProperty(string interfacePath, Cfgmgr32NativeMethods.DEVPROPKEY propertyKey) + { + var value = CmPropertyAccessHelper.TryGetProperty( + Cfgmgr32NativeMethods.CM_Get_Device_Interface_Property, + interfacePath, + propertyKey); + + return value is T typed + ? typed + : throw new NotSupportedException($"Missing or invalid HID interface property {propertyKey}."); + } + + public IHidConnection ConnectToFeatureReports() => + new WindowsHidFeatureReportConnection(_devicePath); + + public IHidConnection ConnectToIOReports() => + new WindowsHidIOReportConnection(_devicePath); +} \ No newline at end of file diff --git a/src/Core/src/Transports/Hid/Windows/WindowsHidFeatureReportConnection.cs b/src/Core/src/Transports/Hid/Windows/WindowsHidFeatureReportConnection.cs index 9e57f5f08..7fb7ba6d2 100644 --- a/src/Core/src/Transports/Hid/Windows/WindowsHidFeatureReportConnection.cs +++ b/src/Core/src/Transports/Hid/Windows/WindowsHidFeatureReportConnection.cs @@ -1,87 +1,66 @@ -// // Copyright 2025 Yubico AB -// // -// // Licensed under the Apache License, Version 2.0 (the "License"). -// // You may not use this file except in compliance with the License. -// // You may obtain a copy of the License at -// // -// // http://www.apache.org/licenses/LICENSE-2.0 -// // -// // Unless required by applicable law or agreed to in writing, software -// // distributed under the License is distributed on an "AS IS" BASIS, -// // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// // See the License for the specific language governing permissions and -// // limitations under the License. +// Copyright 2025 Yubico AB // -// using Yubico.YubiKit.Core.Native.Windows.HidD; +// Licensed under the Apache License, Version 2.0 (the "License"). +// You may not use this file except in compliance with the License. +// You may obtain a copy of the License at // -// namespace Yubico.YubiKit.Core.Core.Devices.Hid -// { -// internal sealed class WindowsHidFeatureReportConnection : IHidConnection -// { -// // The SDK device instance that created this connection instance. -// // private readonly WindowsHidDevice _device; -// // The underlying Windows HID device used for communication. -// private IHidDDevice HidDDevice { get; set; } +// http://www.apache.org/licenses/LICENSE-2.0 // -// public int InputReportSize { get; private set; } -// public int OutputReportSize { get; private set; } -// -// internal WindowsHidFeatureReportConnection(WindowsHidDevice device, string path) -// { -// _device = device; -// HidDDevice = new HidDDevice(path); -// SetupConnection(); -// } -// -// private void SetupConnection() -// { -// HidDDevice.OpenFeatureConnection(); -// InputReportSize = HidDDevice.FeatureReportByteLength; -// OutputReportSize = HidDDevice.FeatureReportByteLength; -// } -// -// public byte[] GetReport() -// { -// byte[] data = HidDDevice.GetFeatureReport(); -// -// _device.LogDeviceAccessTime(); -// -// return data; -// } -// -// public void SetReport(byte[] report) -// { -// HidDDevice.SetFeatureReport(report); -// _device.LogDeviceAccessTime(); -// } -// -// private bool disposedValue; // To detect redundant calls -// -// private void Dispose(bool disposing) -// { -// if (!disposedValue) -// { -// if (disposing) -// { -// HidDDevice.Dispose(); -// } -// -// disposedValue = true; -// } -// } -// -// ~WindowsHidFeatureReportConnection() -// { -// // Do not change this code. Put cleanup code in Dispose(bool disposing) above. -// Dispose(false); -// } -// -// // This code added to correctly implement the disposable pattern. -// public void Dispose() -// { -// // Do not change this code. Put cleanup code in Dispose(bool disposing) above. -// Dispose(true); -// GC.SuppressFinalize(this); -// } -// } -// } \ No newline at end of file +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using Yubico.YubiKit.Core.Devices; +using Yubico.YubiKit.Core.Native.Windows.HidD; + +namespace Yubico.YubiKit.Core.Transports.Hid.Windows; + +internal sealed class WindowsHidFeatureReportConnection : IHidConnection +{ + private readonly IHidDDevice _hidDDevice; + private bool _disposed; + + internal WindowsHidFeatureReportConnection(string path) + { + _hidDDevice = new HidDDevice(path); + _hidDDevice.OpenFeatureConnection(); + + // HidD report lengths include the report ID byte; IHidConnection sizes are payload-only. + InputReportSize = _hidDDevice.FeatureReportByteLength - 1; + OutputReportSize = _hidDDevice.FeatureReportByteLength - 1; + } + + public int InputReportSize { get; } + public int OutputReportSize { get; } + public ConnectionType Type => ConnectionType.Hid; + + public byte[] GetReport() + { + ObjectDisposedException.ThrowIf(_disposed, this); + return _hidDDevice.GetFeatureReport(); + } + + public void SetReport(byte[] report) + { + ObjectDisposedException.ThrowIf(_disposed, this); + ArgumentNullException.ThrowIfNull(report); + _hidDDevice.SetFeatureReport(report); + } + + public void Dispose() + { + if (_disposed) + return; + + _hidDDevice.Dispose(); + _disposed = true; + } + + public ValueTask DisposeAsync() + { + Dispose(); + return ValueTask.CompletedTask; + } +} \ No newline at end of file diff --git a/src/Core/src/Transports/Hid/Windows/WindowsHidIOReportConnection.cs b/src/Core/src/Transports/Hid/Windows/WindowsHidIOReportConnection.cs index bb9d86f80..1a1f835fb 100644 --- a/src/Core/src/Transports/Hid/Windows/WindowsHidIOReportConnection.cs +++ b/src/Core/src/Transports/Hid/Windows/WindowsHidIOReportConnection.cs @@ -1,87 +1,66 @@ -// // Copyright 2025 Yubico AB -// // -// // Licensed under the Apache License, Version 2.0 (the "License"). -// // You may not use this file except in compliance with the License. -// // You may obtain a copy of the License at -// // -// // http://www.apache.org/licenses/LICENSE-2.0 -// // -// // Unless required by applicable law or agreed to in writing, software -// // distributed under the License is distributed on an "AS IS" BASIS, -// // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// // See the License for the specific language governing permissions and -// // limitations under the License. +// Copyright 2025 Yubico AB // -// using Yubico.YubiKit.Core.Native.Windows.HidD; +// Licensed under the Apache License, Version 2.0 (the "License"). +// You may not use this file except in compliance with the License. +// You may obtain a copy of the License at // -// namespace Yubico.YubiKit.Core.Core.Devices.Hid -// { -// internal class WindowsHidIOReportConnection : IHidConnection -// { -// // The SDK device instance that created this connection instance. -// private readonly WindowsHidDevice _device; -// // The underlying Windows HID device used for communication. -// private IHidDDevice HidDDevice { get; set; } +// http://www.apache.org/licenses/LICENSE-2.0 // -// public int InputReportSize { get; private set; } -// public int OutputReportSize { get; private set; } -// -// internal WindowsHidIOReportConnection(WindowsHidDevice device, string path) -// { -// _device = device; -// HidDDevice = new HidDDevice(path); -// SetupConnection(); -// } -// -// private void SetupConnection() -// { -// HidDDevice.OpenIOConnection(); -// InputReportSize = HidDDevice.InputReportByteLength; -// OutputReportSize = HidDDevice.OutputReportByteLength; -// } -// -// public byte[] GetReport() -// { -// byte[] data = HidDDevice.GetInputReport(); -// -// _device.LogDeviceAccessTime(); -// -// return data; -// } -// -// public void SetReport(byte[] report) -// { -// HidDDevice.SetOutputReport(report); -// _device.LogDeviceAccessTime(); -// } -// -// private bool disposedValue; // To detect redundant calls -// -// protected virtual void Dispose(bool disposing) -// { -// if (!disposedValue) -// { -// if (disposing) -// { -// HidDDevice.Dispose(); -// } -// -// disposedValue = true; -// } -// } -// -// ~WindowsHidIOReportConnection() -// { -// // Do not change this code. Put cleanup code in Dispose(bool disposing) above. -// Dispose(false); -// } -// -// // This code added to correctly implement the disposable pattern. -// public void Dispose() -// { -// // Do not change this code. Put cleanup code in Dispose(bool disposing) above. -// Dispose(true); -// GC.SuppressFinalize(this); -// } -// } -// } \ No newline at end of file +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using Yubico.YubiKit.Core.Devices; +using Yubico.YubiKit.Core.Native.Windows.HidD; + +namespace Yubico.YubiKit.Core.Transports.Hid.Windows; + +internal sealed class WindowsHidIOReportConnection : IHidConnection +{ + private readonly IHidDDevice _hidDDevice; + private bool _disposed; + + internal WindowsHidIOReportConnection(string path) + { + _hidDDevice = new HidDDevice(path); + _hidDDevice.OpenIOConnection(); + + // HidD report lengths include the report ID byte; IHidConnection sizes are payload-only. + InputReportSize = _hidDDevice.InputReportByteLength - 1; + OutputReportSize = _hidDDevice.OutputReportByteLength - 1; + } + + public int InputReportSize { get; } + public int OutputReportSize { get; } + public ConnectionType Type => ConnectionType.Hid; + + public byte[] GetReport() + { + ObjectDisposedException.ThrowIf(_disposed, this); + return _hidDDevice.GetInputReport(); + } + + public void SetReport(byte[] report) + { + ObjectDisposedException.ThrowIf(_disposed, this); + ArgumentNullException.ThrowIfNull(report); + _hidDDevice.SetOutputReport(report); + } + + public void Dispose() + { + if (_disposed) + return; + + _hidDDevice.Dispose(); + _disposed = true; + } + + public ValueTask DisposeAsync() + { + Dispose(); + return ValueTask.CompletedTask; + } +} \ No newline at end of file diff --git a/src/Core/tests/Yubico.YubiKit.Core.IntegrationTests/Transports/Hid/HidEnumerationTests.cs b/src/Core/tests/Yubico.YubiKit.Core.IntegrationTests/Transports/Hid/HidEnumerationTests.cs index e23e400d7..303adb38b 100644 --- a/src/Core/tests/Yubico.YubiKit.Core.IntegrationTests/Transports/Hid/HidEnumerationTests.cs +++ b/src/Core/tests/Yubico.YubiKit.Core.IntegrationTests/Transports/Hid/HidEnumerationTests.cs @@ -13,14 +13,12 @@ // limitations under the License. using Microsoft.Extensions.Logging; -using System.Runtime.Versioning; using Xunit.Abstractions; using Yubico.YubiKit.Core.Devices; using Yubico.YubiKit.Core.Transports.Hid; namespace Yubico.YubiKit.Core.IntegrationTests.Transports.Hid; -[SupportedOSPlatform("macos")] public class HidEnumerationTests { private readonly ITestOutputHelper _output; @@ -35,12 +33,6 @@ public HidEnumerationTests(ITestOutputHelper output) [Trait("RequiresHardware", "false")] public async Task FindHidDevices_EnumeratesYubicoDevices() { - if (!OperatingSystem.IsMacOS()) - { - _output.WriteLine("Test skipped: not running on macOS"); - return; - } - var loggerFactory = LoggerFactory.Create(builder => builder.AddConsole().SetMinimumLevel(LogLevel.Debug)); @@ -53,7 +45,7 @@ public async Task FindHidDevices_EnumeratesYubicoDevices() foreach (var device in devices) { _output.WriteLine($" VID={device.DescriptorInfo.VendorId:X4} PID={device.DescriptorInfo.ProductId:X4} " + - $"Usage={device.DescriptorInfo.Usage:X4} UsagePage={device.DescriptorInfo.UsagePage}"); + $"Usage={device.DescriptorInfo.Usage:X4} UsagePage={device.DescriptorInfo.UsagePage:X4}"); } Assert.True(devices.Count >= 0, "Should not fail even if no devices present"); @@ -64,12 +56,6 @@ public async Task FindHidDevices_EnumeratesYubicoDevices() [Trait("RequiresHardware", "true")] public async Task FindYubiKeys_IncludesHidDevices() { - if (!OperatingSystem.IsMacOS()) - { - _output.WriteLine("Test skipped: not running on macOS"); - return; - } - var finder = FindYubiKeys.Create(); var yubiKeys = await finder.FindAllAsync(); @@ -86,4 +72,4 @@ public async Task FindYubiKeys_IncludesHidDevices() Assert.True(yubiKeys.Count >= 0, "Should not fail even if no devices present"); } -} \ No newline at end of file +} From db41b6a9820c1b98ac1556dd968bf5df1b27f0fa Mon Sep 17 00:00:00 2001 From: Dennis Dyall Date: Tue, 23 Jun 2026 18:55:05 +0200 Subject: [PATCH 15/25] fix(core): document Windows HID access failures --- docs/architecture/physical-device-model.md | 16 +++--- .../src/Native/Windows/HidD/HidDDevice.cs | 10 +++- .../Transports/Hid/HidEnumerationTests.cs | 10 +++- .../TestExtensions/FidoTestStateExtensions.cs | 57 +++++++++++++------ 4 files changed, 67 insertions(+), 26 deletions(-) diff --git a/docs/architecture/physical-device-model.md b/docs/architecture/physical-device-model.md index e6955950b..5911d9004 100644 --- a/docs/architecture/physical-device-model.md +++ b/docs/architecture/physical-device-model.md @@ -7,8 +7,9 @@ by `AvailableConnections`. This document explains the model, how to discover and metadata lives, how each applet picks a transport, and how to migrate code written against the old per-interface-handle model. -> **Platform note:** HID interface enumeration is implemented on macOS and Linux. On Windows, HID discovery -> is not yet implemented, so a YubiKey currently surfaces only its PC/SC (CCID) interface there. See +> **Platform note:** HID interface enumeration is implemented on macOS, Linux, and Windows. On Windows, +> opening HID report handles can fail with access denied if another process holds the interface or if the +> environment requires an elevated process. See > [Platform Support For HID Discovery](#platform-support-for-hid-discovery). See also: [event-driven device discovery](./event-driven-device-discovery.md) and the @@ -71,11 +72,12 @@ distinct keys, so one physical key can surface as more than one row. ### Platform Support For HID Discovery -HID interface enumeration (HID FIDO, HID OTP) is implemented on **macOS and Linux**. On **Windows** HID -enumeration is not yet implemented, so today a YubiKey is discovered through its PC/SC (CCID) interface only: -`AvailableConnections` will not include `HidFido`/`HidOtp`, HID connection filters return no devices, and a -composite USB key cannot merge HID interfaces it cannot see. The PC/SC SmartCard path works on all -platforms. This is a known platform residual tracked outside the composite-device model itself. +HID interface enumeration (HID FIDO, HID OTP) is implemented on **macOS, Linux, and Windows**. On Windows, +discovery uses ConfigMgr interface metadata and does not need to open report handles just to identify YubiKey +HID interfaces. Opening a HID report connection can still fail with `UnauthorizedAccessException` when Windows +denies access to the interface, for example because another process holds it exclusively or because the +current environment requires the process to run elevated as Administrator. The PC/SC SmartCard path works on +all platforms and is unaffected by HID report-handle access. ## Opening A Connection diff --git a/src/Core/src/Native/Windows/HidD/HidDDevice.cs b/src/Core/src/Native/Windows/HidD/HidDDevice.cs index 25d61133d..027d4c1b4 100644 --- a/src/Core/src/Native/Windows/HidD/HidDDevice.cs +++ b/src/Core/src/Native/Windows/HidD/HidDDevice.cs @@ -20,6 +20,9 @@ namespace Yubico.YubiKit.Core.Native.Windows.HidD; internal sealed class HidDDevice : IHidDDevice { private const int ErrorAccessDenied = 5; + private const string WindowsHidAccessDeniedGuidance = + "Windows denied access to the HID interface. The interface may be held exclusively by another process, " + + "or this environment may require running the process elevated as Administrator to open YubiKey HID reports."; private SafeFileHandle _handle; private bool _disposed; @@ -152,7 +155,10 @@ private SafeFileHandle OpenHandleWithAccess(Kernel32.NativeMethods.DESIRED_ACCES { var error = Marshal.GetLastWin32Error(); if (error == ErrorAccessDenied) - throw new UnauthorizedAccessException($"Access denied opening HID device '{DevicePath}'."); + { + throw new UnauthorizedAccessException( + $"Access denied opening HID device '{DevicePath}'. {WindowsHidAccessDeniedGuidance}"); + } throw new PlatformApiException(nameof(Kernel32.NativeMethods.CreateFile), error, $"Failed to open HID device '{DevicePath}'."); @@ -211,7 +217,7 @@ private static void ThrowHidDWin32Failure(string source, string message) { var error = Marshal.GetLastWin32Error(); if (error == ErrorAccessDenied) - throw new UnauthorizedAccessException(message); + throw new UnauthorizedAccessException($"{message} {WindowsHidAccessDeniedGuidance}"); throw new PlatformApiException(source, error, message); } diff --git a/src/Core/tests/Yubico.YubiKit.Core.IntegrationTests/Transports/Hid/HidEnumerationTests.cs b/src/Core/tests/Yubico.YubiKit.Core.IntegrationTests/Transports/Hid/HidEnumerationTests.cs index 303adb38b..31b640118 100644 --- a/src/Core/tests/Yubico.YubiKit.Core.IntegrationTests/Transports/Hid/HidEnumerationTests.cs +++ b/src/Core/tests/Yubico.YubiKit.Core.IntegrationTests/Transports/Hid/HidEnumerationTests.cs @@ -48,6 +48,14 @@ public async Task FindHidDevices_EnumeratesYubicoDevices() $"Usage={device.DescriptorInfo.Usage:X4} UsagePage={device.DescriptorInfo.UsagePage:X4}"); } + if (OperatingSystem.IsWindows()) + { + _output.WriteLine( + "Windows HID enumeration reads interface metadata without opening report handles. " + + "Access denied failures while opening HID reports usually mean the test host must run elevated as Administrator, " + + "or another process is holding the HID interface exclusively."); + } + Assert.True(devices.Count >= 0, "Should not fail even if no devices present"); } @@ -72,4 +80,4 @@ public async Task FindYubiKeys_IncludesHidDevices() Assert.True(yubiKeys.Count >= 0, "Should not fail even if no devices present"); } -} +} \ No newline at end of file diff --git a/src/Fido2/tests/Yubico.YubiKit.Fido2.IntegrationTests/TestExtensions/FidoTestStateExtensions.cs b/src/Fido2/tests/Yubico.YubiKit.Fido2.IntegrationTests/TestExtensions/FidoTestStateExtensions.cs index d6eee5035..b82f4cb14 100644 --- a/src/Fido2/tests/Yubico.YubiKit.Fido2.IntegrationTests/TestExtensions/FidoTestStateExtensions.cs +++ b/src/Fido2/tests/Yubico.YubiKit.Fido2.IntegrationTests/TestExtensions/FidoTestStateExtensions.cs @@ -16,7 +16,6 @@ using Yubico.YubiKit.Core.Devices; using Yubico.YubiKit.Core.Protocols.SmartCard.Apdu; using Yubico.YubiKit.Core.Protocols.SmartCard.Scp; -using Yubico.YubiKit.Core.Transports.SmartCard; using Yubico.YubiKit.Fido2.Ctap; using Yubico.YubiKit.Fido2.Pin; using Yubico.YubiKit.Tests.Shared; @@ -138,6 +137,10 @@ await clientPin.ChangePinAsync(tempPin, KnownTestPin, cancellationToken) } } + private static string GetWindowsHidAccessDeniedTestMessage(ConnectionType? preferredConnection) => + $"Windows denied access while opening a FIDO2 session over {preferredConnection ?? ConnectionType.HidFido}. " + + "If this test targets HID FIDO, run the test host elevated as Administrator or close software that may hold the HID interface exclusively."; + extension(YubiKeyTestState state) { /// @@ -166,17 +169,28 @@ public async Task WithFidoSessionAsync( ConnectionType? preferredConnection = null, CancellationToken cancellationToken = default) { - await using var session = await state.Device - .CreateFidoSessionAsync(scpKeyParams, configuration, preferredConnection, cancellationToken) - .ConfigureAwait(false); - - if (normalizePin) + FidoSession session; + try { - await NormalizePinAsync(session, cancellationToken) + session = await state.Device + .CreateFidoSessionAsync(scpKeyParams, configuration, preferredConnection, cancellationToken) .ConfigureAwait(false); } + catch (UnauthorizedAccessException ex) when (OperatingSystem.IsWindows()) + { + throw new UnauthorizedAccessException(GetWindowsHidAccessDeniedTestMessage(preferredConnection), ex); + } + + await using (session.ConfigureAwait(false)) + { + if (normalizePin) + { + await NormalizePinAsync(session, cancellationToken) + .ConfigureAwait(false); + } - await action(session).ConfigureAwait(false); + await action(session).ConfigureAwait(false); + } } /// @@ -205,18 +219,29 @@ public async Task WithFidoSessionAsync( ConnectionType? preferredConnection = null, CancellationToken cancellationToken = default) { - await using var session = await state.Device - .CreateFidoSessionAsync(scpKeyParams, configuration, preferredConnection, cancellationToken) - .ConfigureAwait(false); - - if (normalizePin) + FidoSession session; + try { - await NormalizePinAsync(session, cancellationToken) + session = await state.Device + .CreateFidoSessionAsync(scpKeyParams, configuration, preferredConnection, cancellationToken) .ConfigureAwait(false); } + catch (UnauthorizedAccessException ex) when (OperatingSystem.IsWindows()) + { + throw new UnauthorizedAccessException(GetWindowsHidAccessDeniedTestMessage(preferredConnection), ex); + } - var info = await session.GetInfoAsync(cancellationToken).ConfigureAwait(false); - await action(session, info).ConfigureAwait(false); + await using (session.ConfigureAwait(false)) + { + if (normalizePin) + { + await NormalizePinAsync(session, cancellationToken) + .ConfigureAwait(false); + } + + var info = await session.GetInfoAsync(cancellationToken).ConfigureAwait(false); + await action(session, info).ConfigureAwait(false); + } } } } \ No newline at end of file From 32158c95c114a90fefcc38187ec118cfcca08094 Mon Sep 17 00:00:00 2001 From: Dennis Dyall Date: Tue, 23 Jun 2026 19:50:05 +0200 Subject: [PATCH 16/25] fix(fido2): align CTAP status values with CTAP 2.2 --- src/Fido2/src/Ctap/CtapException.cs | 5 +++-- src/Fido2/src/Ctap/CtapStatus.cs | 15 +++++++++++-- .../FidoMakeCredentialTests.cs | 10 ++++----- .../CtapExceptionTests.cs | 22 ++++++++++++++++++- src/WebAuthn/src/Client/WebAuthnClient.cs | 5 +++-- .../PreviewSign/PreviewSignErrors.cs | 10 ++++++--- 6 files changed, 52 insertions(+), 15 deletions(-) diff --git a/src/Fido2/src/Ctap/CtapException.cs b/src/Fido2/src/Ctap/CtapException.cs index fb4ebbde0..1a9f8bfa3 100644 --- a/src/Fido2/src/Ctap/CtapException.cs +++ b/src/Fido2/src/Ctap/CtapException.cs @@ -115,8 +115,9 @@ public static void ThrowIfError(byte status) CtapStatus.PinAuthInvalid => "PIN authentication failed", CtapStatus.PinAuthBlocked => "PIN authentication blocked", CtapStatus.PinNotSet => "PIN not set", - CtapStatus.PuvathRequired => "PIN/UV auth token required", + CtapStatus.PuatRequired => "PIN/UV auth token required", CtapStatus.PinPolicyViolation => "PIN policy violation", + CtapStatus.PinTokenExpired => "PIN/UV auth token expired", CtapStatus.RequestTooLarge => "Request too large", CtapStatus.ActionTimeout => "Action timeout", CtapStatus.UpRequired => "User presence required", @@ -128,4 +129,4 @@ public static void ThrowIfError(byte status) CtapStatus.Other => "Unspecified error", _ => $"Unknown CTAP error: 0x{(byte)status:X2}" }; -} \ No newline at end of file +} diff --git a/src/Fido2/src/Ctap/CtapStatus.cs b/src/Fido2/src/Ctap/CtapStatus.cs index 70236120b..929a78bde 100644 --- a/src/Fido2/src/Ctap/CtapStatus.cs +++ b/src/Fido2/src/Ctap/CtapStatus.cs @@ -203,13 +203,24 @@ public enum CtapStatus : byte /// /// A pinUvAuthToken is required for the selected operation. /// - PuvathRequired = 0x36, + PuatRequired = 0x36, + + /// + /// A pinUvAuthToken is required for the selected operation. + /// + [Obsolete("Use PuatRequired instead.", false)] + PuvathRequired = PuatRequired, /// /// PIN policy violation. Currently only enforces minimum length. /// PinPolicyViolation = 0x37, + /// + /// The pinUvAuthToken has expired. + /// + PinTokenExpired = 0x38, + /// /// Authenticator cannot handle this request due to memory constraints. /// @@ -274,4 +285,4 @@ public enum CtapStatus : byte /// Vendor specific error range end. /// VendorLast = 0xFF, -} \ No newline at end of file +} diff --git a/src/Fido2/tests/Yubico.YubiKit.Fido2.IntegrationTests/FidoMakeCredentialTests.cs b/src/Fido2/tests/Yubico.YubiKit.Fido2.IntegrationTests/FidoMakeCredentialTests.cs index 7b5907435..2b1cf1326 100644 --- a/src/Fido2/tests/Yubico.YubiKit.Fido2.IntegrationTests/FidoMakeCredentialTests.cs +++ b/src/Fido2/tests/Yubico.YubiKit.Fido2.IntegrationTests/FidoMakeCredentialTests.cs @@ -308,7 +308,7 @@ await state.WithFidoSessionAsync(async session => // PinUvAuthProtocol = null }; - // Act & Assert: Expect CtapException with PuvathRequired or PinAuthInvalid + // Act & Assert: Expect CtapException with PuatRequired or PinAuthInvalid var ctapEx = await Assert.ThrowsAsync(async () => { await session.MakeCredentialAsync( @@ -320,13 +320,13 @@ await session.MakeCredentialAsync( }); // Different firmware versions may return different status codes: - // - PuvathRequired (0x36): Standard CTAP2 response when pinUvAuthToken is needed + // - PuatRequired (0x36): Standard CTAP2 response when pinUvAuthToken is needed // - PinInvalid (0x31): Some firmware versions return this when PIN auth is missing // - PinAuthInvalid (0x33): Alternative error when pinUvAuthParam is absent // - MissingParameter (0x14): Another possible response when required param is missing Assert.True( - ctapEx.Status is CtapStatus.PuvathRequired or CtapStatus.PinInvalid or CtapStatus.PinAuthInvalid or CtapStatus.MissingParameter, - $"Expected PuvathRequired/PinInvalid/PinAuthInvalid/MissingParameter, got {ctapEx.Status}"); + ctapEx.Status is CtapStatus.PuatRequired or CtapStatus.PinInvalid or CtapStatus.PinAuthInvalid or CtapStatus.MissingParameter, + $"Expected PuatRequired/PinInvalid/PinAuthInvalid/MissingParameter, got {ctapEx.Status}"); }); private static async Task CleanupCredentialAsync(FidoSession session, byte[] credentialId) @@ -350,4 +350,4 @@ private static async Task CleanupCredentialAsync(FidoSession session, byte[] cre // Cleanup failures should not fail the test } } -} \ No newline at end of file +} diff --git a/src/Fido2/tests/Yubico.YubiKit.Fido2.UnitTests/CtapExceptionTests.cs b/src/Fido2/tests/Yubico.YubiKit.Fido2.UnitTests/CtapExceptionTests.cs index 2dacef96b..244a91520 100644 --- a/src/Fido2/tests/Yubico.YubiKit.Fido2.UnitTests/CtapExceptionTests.cs +++ b/src/Fido2/tests/Yubico.YubiKit.Fido2.UnitTests/CtapExceptionTests.cs @@ -66,6 +66,24 @@ public void ThrowIfError_WithByte_ThrowsException() .Which.Status.Should().Be(CtapStatus.PinBlocked); } + [Theory] + [InlineData(CtapStatus.PinNotSet, 0x35)] + [InlineData(CtapStatus.PuatRequired, 0x36)] + [InlineData(CtapStatus.PinTokenExpired, 0x38)] + [InlineData(CtapStatus.ActionTimeout, 0x3A)] + [InlineData(CtapStatus.UpRequired, 0x3B)] + public void Ctap22StatusValues_MatchSpecification(CtapStatus status, byte expectedValue) => + ((byte)status).Should().Be(expectedValue); + + [Fact] + public void PuvathRequired_ObsoleteAlias_MapsToPuatRequired() + { +#pragma warning disable CS0618 + CtapStatus.PuvathRequired.Should().Be(CtapStatus.PuatRequired); + ((byte)CtapStatus.PuvathRequired).Should().Be(0x36); +#pragma warning restore CS0618 + } + [Theory] [InlineData(CtapStatus.InvalidCommand, "Invalid CTAP command")] [InlineData(CtapStatus.InvalidParameter, "Invalid parameter")] @@ -73,6 +91,8 @@ public void ThrowIfError_WithByte_ThrowsException() [InlineData(CtapStatus.CredentialExcluded, "excluded")] [InlineData(CtapStatus.NoCredentials, "No credentials")] [InlineData(CtapStatus.UserActionTimeout, "timeout")] + [InlineData(CtapStatus.PuatRequired, "token required")] + [InlineData(CtapStatus.PinTokenExpired, "token expired")] public void Constructor_WithKnownStatus_HasDescriptiveMessage(CtapStatus status, string expectedContains) { // Arrange & Act @@ -81,4 +101,4 @@ public void Constructor_WithKnownStatus_HasDescriptiveMessage(CtapStatus status, // Assert exception.Message.Should().ContainEquivalentOf(expectedContains); } -} \ No newline at end of file +} diff --git a/src/WebAuthn/src/Client/WebAuthnClient.cs b/src/WebAuthn/src/Client/WebAuthnClient.cs index 7aa66ab9e..e000a4360 100644 --- a/src/WebAuthn/src/Client/WebAuthnClient.cs +++ b/src/WebAuthn/src/Client/WebAuthnClient.cs @@ -806,7 +806,8 @@ private static WebAuthnClientError MapCtapStatusToWebAuthnError(CtapException ex { CtapStatus.PinAuthInvalid or CtapStatus.PinInvalid or CtapStatus.PinAuthBlocked or CtapStatus.PinBlocked or CtapStatus.PinPolicyViolation - or CtapStatus.PuvathRequired or CtapStatus.NotAllowed or CtapStatus.OperationDenied + or CtapStatus.PuatRequired or CtapStatus.PinTokenExpired + or CtapStatus.NotAllowed or CtapStatus.OperationDenied => new WebAuthnClientError(WebAuthnClientErrorCode.NotAllowed, ex.Message, ex), CtapStatus.KeyStoreFull or CtapStatus.LargeBlobStorageFull or CtapStatus.FpDatabaseFull or CtapStatus.LimitExceeded or CtapStatus.RequestTooLarge or CtapStatus.UserActionTimeout @@ -1037,4 +1038,4 @@ private static AuthenticationResponse BuildAuthenticationResponse( }; } -} \ No newline at end of file +} diff --git a/src/WebAuthn/src/Extensions/PreviewSign/PreviewSignErrors.cs b/src/WebAuthn/src/Extensions/PreviewSign/PreviewSignErrors.cs index 6bf13393c..75fec6bec 100644 --- a/src/WebAuthn/src/Extensions/PreviewSign/PreviewSignErrors.cs +++ b/src/WebAuthn/src/Extensions/PreviewSign/PreviewSignErrors.cs @@ -46,12 +46,16 @@ public static WebAuthnClientError MapCtapError(CtapException ex) => "previewSign: user presence required but not provided", ex), - // Note: CtapStatus constant is "PuvathRequired" (not "PuatRequired") - CtapStatus.PuvathRequired => new WebAuthnClientError( + CtapStatus.PuatRequired => new WebAuthnClientError( WebAuthnClientErrorCode.NotAllowed, "previewSign: PIN/UV auth token required", ex), + CtapStatus.PinTokenExpired => new WebAuthnClientError( + WebAuthnClientErrorCode.NotAllowed, + "previewSign: PIN/UV auth token expired", + ex), + CtapStatus.InvalidCredential => new WebAuthnClientError( WebAuthnClientErrorCode.InvalidRequest, "previewSign: signByCredential references unknown credential", @@ -67,4 +71,4 @@ public static WebAuthnClientError MapCtapError(CtapException ex) => $"previewSign CTAP error: {ex.Status}", ex) }; -} \ No newline at end of file +} From 85d6f0b3c935b8deacac6ce360a07e258318c337 Mon Sep 17 00:00:00 2001 From: Dennis Dyall Date: Tue, 23 Jun 2026 19:50:45 +0200 Subject: [PATCH 17/25] fix(core): use full PIV application identifier --- src/Core/src/Sessions/ApplicationIds.cs | 4 +-- .../Sessions/ApplicationIdsTests.cs | 29 +++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) create mode 100644 src/Core/tests/Yubico.YubiKit.Core.UnitTests/Sessions/ApplicationIdsTests.cs diff --git a/src/Core/src/Sessions/ApplicationIds.cs b/src/Core/src/Sessions/ApplicationIds.cs index 6e6713842..592d81634 100644 --- a/src/Core/src/Sessions/ApplicationIds.cs +++ b/src/Core/src/Sessions/ApplicationIds.cs @@ -22,7 +22,7 @@ public record ApplicationIds // Should be in Yubico.YubiKit public static readonly byte[] Fido2 = [0xA0, 0x00, 0x00, 0x06, 0x47, 0x2F, 0x00, 0x01]; public static readonly byte[] Oath = [0xA0, 0x00, 0x00, 0x05, 0x27, 0x21, 0x01]; public static readonly byte[] OpenPgp = [0xD2, 0x76, 0x00, 0x01, 0x24, 0x01]; - public static readonly byte[] Piv = [0xA0, 0x00, 0x00, 0x03, 0x08]; + public static readonly byte[] Piv = [0xA0, 0x00, 0x00, 0x03, 0x08, 0x00, 0x00, 0x10, 0x00]; public static readonly byte[] YubiHsmAuth = [0xA0, 0x00, 0x00, 0x05, 0x27, 0x21, 0x07, 0x01]; public static readonly byte[] SecurityDomain = [0xA0, 0x00, 0x00, 0x01, 0x51, 0x00, 0x00, 0x00]; -} \ No newline at end of file +} diff --git a/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Sessions/ApplicationIdsTests.cs b/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Sessions/ApplicationIdsTests.cs new file mode 100644 index 000000000..07c959d65 --- /dev/null +++ b/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Sessions/ApplicationIdsTests.cs @@ -0,0 +1,29 @@ +// Copyright 2026 Yubico AB +// +// Licensed under the Apache License, Version 2.0 (the "License"). +// You may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using Yubico.YubiKit.Core.Sessions; + +namespace Yubico.YubiKit.Core.UnitTests.Sessions; + +public class ApplicationIdsTests +{ + [Fact] + public void Piv_UsesRidPlusPixAid() + { + Assert.Equal(9, ApplicationIds.Piv.Length); + Assert.Equal( + new byte[] { 0xA0, 0x00, 0x00, 0x03, 0x08, 0x00, 0x00, 0x10, 0x00 }, + ApplicationIds.Piv); + } +} From 23bdbfb71481ba09413c943c0c7354d3b8088308 Mon Sep 17 00:00:00 2001 From: Dennis Dyall Date: Tue, 23 Jun 2026 20:54:50 +0200 Subject: [PATCH 18/25] docs(fido2): clarify CTAP status coverage --- src/Fido2/src/Ctap/CtapStatus.cs | 11 +++++++++-- .../FidoMakeCredentialTests.cs | 10 +++++----- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/Fido2/src/Ctap/CtapStatus.cs b/src/Fido2/src/Ctap/CtapStatus.cs index 929a78bde..2f2fd5d25 100644 --- a/src/Fido2/src/Ctap/CtapStatus.cs +++ b/src/Fido2/src/Ctap/CtapStatus.cs @@ -15,11 +15,18 @@ namespace Yubico.YubiKit.Fido2.Ctap; /// -/// CTAP2 status codes per FIDO Alliance specification. +/// CTAP2 status codes per FIDO Alliance specifications. /// /// /// -/// See: https://fidoalliance.org/specs/fido-v2.1-ps-20210615/fido-client-to-authenticator-protocol-v2.1-ps-errata-20220621.html#error-responses +/// This enum includes CTAP 2.1 status codes plus CTAP 2.2 status-code additions used by +/// newer authenticators. +/// +/// +/// CTAP 2.1: https://fidoalliance.org/specs/fido-v2.1-ps-20210615/fido-client-to-authenticator-protocol-v2.1-ps-errata-20220621.html#error-responses +/// +/// +/// CTAP 2.2 Review Draft: https://fidoalliance.org/specs/fido-v2.2-rd-20241003/fido-client-to-authenticator-protocol-v2.2-rd-20241003.html#error-responses /// /// public enum CtapStatus : byte diff --git a/src/Fido2/tests/Yubico.YubiKit.Fido2.IntegrationTests/FidoMakeCredentialTests.cs b/src/Fido2/tests/Yubico.YubiKit.Fido2.IntegrationTests/FidoMakeCredentialTests.cs index 2b1cf1326..7d384e40e 100644 --- a/src/Fido2/tests/Yubico.YubiKit.Fido2.IntegrationTests/FidoMakeCredentialTests.cs +++ b/src/Fido2/tests/Yubico.YubiKit.Fido2.IntegrationTests/FidoMakeCredentialTests.cs @@ -265,13 +265,13 @@ await session.MakeCredentialAsync( }); /// - /// Tests that MakeCredential throws PinRequired when PIN is required but not provided. + /// Tests that MakeCredential fails when PIN/UV auth is required but not provided. /// /// /// /// This test verifies the error path when the authenticator requires PIN/UV but the client - /// does not provide pinUvAuthParam. The authenticator should return CtapStatus.PinRequired - /// or CtapStatus.PinInvalid depending on firmware version. + /// does not provide pinUvAuthParam. The authenticator should return an error such as + /// CtapStatus.PuatRequired or CtapStatus.PinInvalid depending on firmware version. /// /// /// This closes the architectural gap where WebAuthn proved this error path on hardware @@ -281,7 +281,7 @@ await session.MakeCredentialAsync( [SkippableTheory] [WithYubiKey(ConnectionType = ConnectionType.HidFido)] [Trait(TestCategories.Category, TestCategories.RequiresUserPresence)] - public async Task MakeCredential_WhenPinRequiredButNotProvided_ThrowsInvalidParameter(YubiKeyTestState state) => + public async Task MakeCredential_WhenPinUvAuthRequiredButNotProvided_ThrowsCtapError(YubiKeyTestState state) => await state.WithFidoSessionAsync(async session => { // Arrange: Ensure PIN is set (this makes the device require PIN for operations) @@ -308,7 +308,7 @@ await state.WithFidoSessionAsync(async session => // PinUvAuthProtocol = null }; - // Act & Assert: Expect CtapException with PuatRequired or PinAuthInvalid + // Act & Assert: Expect a CTAP error when PIN/UV auth is required but absent. var ctapEx = await Assert.ThrowsAsync(async () => { await session.MakeCredentialAsync( From d265372ae2a8e7f9fdb7dde6af759a52e1ab1a5e Mon Sep 17 00:00:00 2001 From: Dennis Dyall Date: Wed, 24 Jun 2026 12:57:15 +0200 Subject: [PATCH 19/25] chore: code style --- .../src/Native/Windows/HidD/HidDDevice.cs | 27 ++++++++++++++++--- .../Apdu/Fakes/FakeSmartCardConnection.cs | 16 +++++------ 2 files changed, 31 insertions(+), 12 deletions(-) diff --git a/src/Core/src/Native/Windows/HidD/HidDDevice.cs b/src/Core/src/Native/Windows/HidD/HidDDevice.cs index 027d4c1b4..23b8696fb 100644 --- a/src/Core/src/Native/Windows/HidD/HidDDevice.cs +++ b/src/Core/src/Native/Windows/HidD/HidDDevice.cs @@ -60,7 +60,9 @@ public byte[] GetFeatureReport() var buffer = new byte[FeatureReportByteLength]; if (!NativeMethods.HidD_GetFeature(_handle, buffer, buffer.Length)) + { Marshal.ThrowExceptionForHR(Marshal.GetHRForLastWin32Error()); + } // Windows includes the report ID byte; the SDK exposes only report payload bytes. var returnBuf = new byte[FeatureReportByteLength - 1]; @@ -74,14 +76,18 @@ public void SetFeatureReport(byte[] buffer) EnsureOpenHandle(); if (buffer.Length != FeatureReportByteLength - 1) + { throw new InvalidOperationException("The HID feature report buffer length is invalid."); + } // Windows expects the report ID byte before the report payload. var sendBuf = new byte[buffer.Length + 1]; Array.Copy(buffer, 0, sendBuf, 1, buffer.Length); if (!NativeMethods.HidD_SetFeature(_handle, sendBuf, sendBuf.Length)) + { Marshal.ThrowExceptionForHR(Marshal.GetHRForLastWin32Error()); + } } public byte[] GetInputReport() @@ -91,7 +97,9 @@ public byte[] GetInputReport() var buffer = new byte[InputReportByteLength]; if (!Kernel32.NativeMethods.ReadFile(_handle, buffer, buffer.Length, out var bytesRead, IntPtr.Zero) || bytesRead != buffer.Length) + { Marshal.ThrowExceptionForHR(Marshal.GetHRForLastWin32Error()); + } // Windows includes the report ID byte; the SDK exposes only report payload bytes. var returnBuf = new byte[InputReportByteLength - 1]; @@ -105,7 +113,9 @@ public void SetOutputReport(byte[] buffer) EnsureOpenHandle(); if (buffer.Length != OutputReportByteLength - 1) + { throw new InvalidOperationException("The HID output report buffer length is invalid."); + } // Windows expects the report ID byte before the report payload. var sendBuf = new byte[buffer.Length + 1]; @@ -113,7 +123,9 @@ public void SetOutputReport(byte[] buffer) if (!Kernel32.NativeMethods.WriteFile(_handle, sendBuf, sendBuf.Length, out var bytesWritten, IntPtr.Zero) || bytesWritten != sendBuf.Length) + { Marshal.ThrowExceptionForHR(Marshal.GetHRForLastWin32Error()); + } } @@ -122,16 +134,17 @@ private static NativeMethods.HIDP_CAPS GetCapabilities(SafeFileHandle safeHandle NativeMethods.HIDP_CAPS capabilities = new(); if (!NativeMethods.HidD_GetPreparsedData(safeHandle, out var preparsedData)) + { ThrowHidDWin32Failure(nameof(NativeMethods.HidD_GetPreparsedData), "Failed to get HID preparsed data."); + } try { var result = NativeMethods.HidP_GetCaps(preparsedData, ref capabilities); - if (result != NativeMethods.HidpStatusSuccess) - throw new PlatformApiException(nameof(NativeMethods.HidP_GetCaps), result, + return result == NativeMethods.HidpStatusSuccess + ? capabilities + : throw new PlatformApiException(nameof(NativeMethods.HidP_GetCaps), result, "Failed to get HID capabilities."); - - return capabilities; } finally { @@ -217,7 +230,9 @@ private static void ThrowHidDWin32Failure(string source, string message) { var error = Marshal.GetLastWin32Error(); if (error == ErrorAccessDenied) + { throw new UnauthorizedAccessException($"{message} {WindowsHidAccessDeniedGuidance}"); + } throw new PlatformApiException(source, error, message); } @@ -227,13 +242,17 @@ private void EnsureOpenHandle() ObjectDisposedException.ThrowIf(_disposed, this); if (_handle.IsInvalid || _handle.IsClosed) + { throw new InvalidOperationException($"The HID device handle for '{DevicePath}' is not open."); + } } public void Dispose() { if (_disposed) + { return; + } _handle.Dispose(); _disposed = true; diff --git a/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Protocols/SmartCard/Apdu/Fakes/FakeSmartCardConnection.cs b/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Protocols/SmartCard/Apdu/Fakes/FakeSmartCardConnection.cs index c215ce090..116cf2d95 100644 --- a/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Protocols/SmartCard/Apdu/Fakes/FakeSmartCardConnection.cs +++ b/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Protocols/SmartCard/Apdu/Fakes/FakeSmartCardConnection.cs @@ -13,7 +13,7 @@ // limitations under the License. using Yubico.YubiKit.Core.Devices; -using Yubico.YubiKit.Core.Protocols.SmartCard.Apdu; + using Yubico.YubiKit.Core.Transports.SmartCard; namespace Yubico.YubiKit.Core.UnitTests.Protocols.SmartCard.Apdu.Fakes; @@ -46,16 +46,17 @@ public async Task> TransmitAndReceiveAsync( CancellationToken cancellationToken = default) { if (_disposed) + { throw new ObjectDisposedException(nameof(FakeSmartCardConnection)); + } cancellationToken.ThrowIfCancellationRequested(); TransmittedCommands.Add(command.ToArray()); - if (_responses.Count == 0) - throw new InvalidOperationException("No response enqueued for transmission"); - - return await Task.FromResult(_responses.Dequeue()); + return _responses.Count == 0 + ? throw new InvalidOperationException("No response enqueued for transmission") + : await Task.FromResult(_responses.Dequeue()); } public void Dispose() @@ -64,9 +65,8 @@ public void Dispose() _disposed = true; } - public async ValueTask DisposeAsync() - { + public ValueTask DisposeAsync() => // TODO release managed resources here - } + ValueTask.CompletedTask; } \ No newline at end of file From de5f0ed9cd0917f01e46e0a0be7ad75c9cc28c6b Mon Sep 17 00:00:00 2001 From: Dennis Dyall Date: Wed, 24 Jun 2026 14:49:44 +0200 Subject: [PATCH 20/25] fix(core): audit and fix Windows HID tests - Fix HidEnumerationTests: correct RequiresHardware trait, replace broken DeviceId prefix filter with SupportsConnection, strengthen trivially-true assertions, show Windows advisory only when 0 devices - Fix HidDeviceListenerDisposalTests: narrow catch(Exception) to catch(PlatformNotSupportedException), remove duplicate platform implementation test, add state assertions to idempotent tests - Fix HidDeviceListenerIntegrationTests: narrow exception handling, fix Create_StatusIsStarted (never called Start), start listener before testing for spurious events and disposal, remove duplicate Dispose_MultipleTimes test - Add HidInterfaceClassifierTests: 9 unit tests covering Classify, IsSupported, and GetReportType for FIDO, OTP, and Unknown types Co-Authored-By: Claude Opus 4.6 --- .editorconfig | 5 +- .../Hid/HidDeviceListenerIntegrationTests.cs | 81 ++++-------------- .../Transports/Hid/HidEnumerationTests.cs | 11 +-- .../Hid/HidDeviceListenerDisposalTests.cs | 84 ++----------------- .../Hid/HidInterfaceClassifierTests.cs | 72 ++++++++++++++++ 5 files changed, 105 insertions(+), 148 deletions(-) create mode 100644 src/Core/tests/Yubico.YubiKit.Core.UnitTests/Transports/Hid/HidInterfaceClassifierTests.cs diff --git a/.editorconfig b/.editorconfig index 225f45024..23ec362bd 100644 --- a/.editorconfig +++ b/.editorconfig @@ -129,7 +129,10 @@ csharp_style_prefer_readonly_struct = true csharp_style_prefer_readonly_struct_member = true # Code-block preferences -csharp_prefer_braces = false:when_multiline +csharp_prefer_braces = when_multiline:none +csharp_style_prefer_conditional_expression_over_return = true:suggestion +dotnet_diagnostic.IDE0046.severity = suggestion +dotnet_diagnostic.IDE0072.severity = suggestion csharp_prefer_simple_using_statement = true csharp_style_namespace_declarations = file_scoped csharp_style_prefer_method_group_conversion = true diff --git a/src/Core/tests/Yubico.YubiKit.Core.IntegrationTests/Transports/Hid/HidDeviceListenerIntegrationTests.cs b/src/Core/tests/Yubico.YubiKit.Core.IntegrationTests/Transports/Hid/HidDeviceListenerIntegrationTests.cs index 47f12bc88..41948301f 100644 --- a/src/Core/tests/Yubico.YubiKit.Core.IntegrationTests/Transports/Hid/HidDeviceListenerIntegrationTests.cs +++ b/src/Core/tests/Yubico.YubiKit.Core.IntegrationTests/Transports/Hid/HidDeviceListenerIntegrationTests.cs @@ -46,11 +46,6 @@ public void Create_ReturnsCorrectPlatformImplementation() _output.WriteLine($"Test skipped: {ex.Message}"); return; } - catch (Exception ex) - { - _output.WriteLine($"HID listener creation failed: {ex.Message}"); - return; - } try { @@ -78,9 +73,9 @@ public void Create_ReturnsCorrectPlatformImplementation() } [Fact] - public void Create_StatusIsStarted() + public void Start_TransitionsToStartedStatus() { - // Arrange & Act + // Arrange HidDeviceListener? listener = null; try { @@ -91,17 +86,17 @@ public void Create_StatusIsStarted() _output.WriteLine($"Test skipped: {ex.Message}"); return; } - catch (Exception ex) - { - _output.WriteLine($"Test skipped: {ex.Message}"); - return; - } try { - // Assert - Assert.Equal(DeviceListenerStatus.Started, listener.Status); - _output.WriteLine("HID listener created with Started status"); + // Act + listener.Start(); + + // Assert - Error is acceptable when CM_Register_Notification is blocked (CI/sandbox) + Assert.True( + listener.Status is DeviceListenerStatus.Started or DeviceListenerStatus.Error, + $"Expected Started or Error, but got {listener.Status}"); + _output.WriteLine($"HID listener status after Start(): {listener.Status}"); } finally { @@ -110,7 +105,7 @@ public void Create_StatusIsStarted() } [Fact] - public void EventSubscription_NoDeviceChange_NoEventsFired() + public void StartedListener_NoDeviceChange_NoSpuriousEvents() { // Arrange HidDeviceListener? listener = null; @@ -123,24 +118,20 @@ public void EventSubscription_NoDeviceChange_NoEventsFired() _output.WriteLine($"Test skipped: {ex.Message}"); return; } - catch (Exception ex) - { - _output.WriteLine($"Test skipped: {ex.Message}"); - return; - } var eventCount = 0; try { listener.DeviceEvent = () => Interlocked.Increment(ref eventCount); + listener.Start(); - // Act - wait briefly + // Act - wait briefly with listener running Thread.Sleep(500); - // Assert - no spurious events + // Assert - no spurious events while running Assert.Equal(0, eventCount); - _output.WriteLine("No spurious events fired during quiet period"); + _output.WriteLine("No spurious events fired during quiet period with listener running"); } finally { @@ -149,24 +140,20 @@ public void EventSubscription_NoDeviceChange_NoEventsFired() } [Fact] - public async Task Dispose_CompletesCleanly() + public async Task Dispose_AfterStart_CompletesCleanly() { // Arrange HidDeviceListener? listener = null; try { listener = HidDeviceListener.Create(); + listener.Start(); } catch (PlatformNotSupportedException ex) { _output.WriteLine($"Test skipped: {ex.Message}"); return; } - catch (Exception ex) - { - _output.WriteLine($"Test skipped: {ex.Message}"); - return; - } // Act var disposeTask = Task.Run(() => listener.Dispose()); @@ -175,38 +162,6 @@ public async Task Dispose_CompletesCleanly() // Assert Assert.True(completedTask == disposeTask, "Dispose should complete within 5 seconds"); Assert.Equal(DeviceListenerStatus.Stopped, listener.Status); - _output.WriteLine("Dispose completed cleanly within timeout"); - } - - [Fact] - public void Dispose_MultipleTimes_NoException() - { - // Arrange - HidDeviceListener? listener = null; - try - { - listener = HidDeviceListener.Create(); - } - catch (PlatformNotSupportedException ex) - { - _output.WriteLine($"Test skipped: {ex.Message}"); - return; - } - catch (Exception ex) - { - _output.WriteLine($"Test skipped: {ex.Message}"); - return; - } - - // Act & Assert - var exception = Record.Exception(() => - { - listener.Dispose(); - listener.Dispose(); - listener.Dispose(); - }); - - Assert.Null(exception); - _output.WriteLine("Multiple dispose calls succeeded without exception"); + _output.WriteLine("Dispose after Start() completed cleanly within timeout"); } } \ No newline at end of file diff --git a/src/Core/tests/Yubico.YubiKit.Core.IntegrationTests/Transports/Hid/HidEnumerationTests.cs b/src/Core/tests/Yubico.YubiKit.Core.IntegrationTests/Transports/Hid/HidEnumerationTests.cs index 31b640118..10b41a780 100644 --- a/src/Core/tests/Yubico.YubiKit.Core.IntegrationTests/Transports/Hid/HidEnumerationTests.cs +++ b/src/Core/tests/Yubico.YubiKit.Core.IntegrationTests/Transports/Hid/HidEnumerationTests.cs @@ -30,7 +30,7 @@ public HidEnumerationTests(ITestOutputHelper output) [Fact] [Trait("Category", "Integration")] - [Trait("RequiresHardware", "false")] + [Trait("RequiresHardware", "true")] public async Task FindHidDevices_EnumeratesYubicoDevices() { var loggerFactory = LoggerFactory.Create(builder => @@ -48,7 +48,7 @@ public async Task FindHidDevices_EnumeratesYubicoDevices() $"Usage={device.DescriptorInfo.Usage:X4} UsagePage={device.DescriptorInfo.UsagePage:X4}"); } - if (OperatingSystem.IsWindows()) + if (devices.Count == 0 && OperatingSystem.IsWindows()) { _output.WriteLine( "Windows HID enumeration reads interface metadata without opening report handles. " + @@ -56,7 +56,7 @@ public async Task FindHidDevices_EnumeratesYubicoDevices() "or another process is holding the HID interface exclusively."); } - Assert.True(devices.Count >= 0, "Should not fail even if no devices present"); + Assert.True(devices.Count > 0, "Expected at least one Yubico HID device with hardware present"); } [Fact] @@ -75,9 +75,10 @@ public async Task FindYubiKeys_IncludesHidDevices() _output.WriteLine($" DeviceId={yubiKey.DeviceId}"); } - var hidYubiKeys = yubiKeys.Where(yk => yk.DeviceId.StartsWith("hid:")).ToList(); + var hidYubiKeys = yubiKeys.Where(yk => yk.SupportsConnection(ConnectionType.Hid)).ToList(); _output.WriteLine($"Found {hidYubiKeys.Count} HID YubiKeys"); - Assert.True(yubiKeys.Count >= 0, "Should not fail even if no devices present"); + Assert.True(yubiKeys.Count > 0, "Expected at least one YubiKey with hardware present"); + Assert.True(hidYubiKeys.Count > 0, "Expected at least one YubiKey with HID interfaces"); } } \ No newline at end of file diff --git a/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Transports/Hid/HidDeviceListenerDisposalTests.cs b/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Transports/Hid/HidDeviceListenerDisposalTests.cs index 751fdb0a6..e48d4ad13 100644 --- a/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Transports/Hid/HidDeviceListenerDisposalTests.cs +++ b/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Transports/Hid/HidDeviceListenerDisposalTests.cs @@ -41,10 +41,6 @@ public void Constructor_DoesNotAutoStart() { return; } - catch (Exception) - { - return; - } try { @@ -73,17 +69,13 @@ public void Start_TransitionsToStartedStatus() { return; } - catch (Exception) - { - return; - } try { // Act listener.Start(); - // Assert - either Started or Error (if HID subsystem unavailable) + // Assert - either Started or Error (if HID subsystem unavailable in sandboxed/CI environments) Assert.True( listener.Status is DeviceListenerStatus.Started or DeviceListenerStatus.Error, $"Expected Started or Error, but got {listener.Status}"); @@ -110,10 +102,6 @@ public void Stop_TransitionsToStoppedStatus() { return; } - catch (Exception) - { - return; - } try { @@ -147,10 +135,6 @@ public void Start_CalledMultipleTimes_Idempotent() { return; } - catch (Exception) - { - return; - } try { @@ -164,6 +148,9 @@ public void Start_CalledMultipleTimes_Idempotent() // Assert Assert.Null(exception); + Assert.True( + listener.Status is DeviceListenerStatus.Started or DeviceListenerStatus.Error, + $"Expected Started or Error after multiple Start() calls, but got {listener.Status}"); } finally { @@ -187,10 +174,6 @@ public void Stop_CalledMultipleTimes_Idempotent() { return; } - catch (Exception) - { - return; - } try { @@ -204,6 +187,7 @@ public void Stop_CalledMultipleTimes_Idempotent() // Assert Assert.Null(exception); + Assert.Equal(DeviceListenerStatus.Stopped, listener.Status); } finally { @@ -227,10 +211,6 @@ public void DeviceEvent_NotFiredBeforeStart() { return; } - catch (Exception) - { - return; - } try { @@ -249,44 +229,6 @@ public void DeviceEvent_NotFiredBeforeStart() } } - - - /// - /// Verifies that the factory returns the correct platform-specific implementation. - /// - [Fact] - public void Create_ReturnsPlatformSpecificImplementation() - { - // Arrange & Act - HidDeviceListener? listener = null; - try - { - listener = HidDeviceListener.Create(); - } - catch (PlatformNotSupportedException) - { - return; - } - catch (Exception) - { - return; - } - - try - { - // Assert - Type should match current platform - var typeName = listener.GetType().Name; - var expectedNames = new[] { "WindowsHidDeviceListener", "MacOSHidDeviceListener", "LinuxHidDeviceListener" }; - Assert.Contains(typeName, expectedNames); - } - finally - { - listener?.Dispose(); - } - } - - - /// /// Verifies that Dispose completes within a reasonable timeout when listener is running. /// If this test hangs, the cancellation logic is broken. @@ -305,10 +247,6 @@ public async Task Dispose_AfterStart_CompletesWithinTimeout() { return; } - catch (Exception) - { - return; - } // Act & Assert - must complete within 10 seconds var disposeTask = Task.Run(() => listener.Dispose(), TestContext.Current.CancellationToken); @@ -334,10 +272,6 @@ public async Task Dispose_NeverStarted_CompletesImmediately() { return; } - catch (Exception) - { - return; - } // Act & Assert - should complete very quickly var disposeTask = Task.Run(() => listener.Dispose(), TestContext.Current.CancellationToken); @@ -363,10 +297,6 @@ public void Dispose_CalledMultipleTimes_NoException() { return; } - catch (Exception) - { - return; - } // Act & Assert var exception = Record.Exception(() => @@ -396,10 +326,6 @@ public void Dispose_SetsStatusToStopped() { return; } - catch (Exception) - { - return; - } // Act listener.Dispose(); diff --git a/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Transports/Hid/HidInterfaceClassifierTests.cs b/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Transports/Hid/HidInterfaceClassifierTests.cs new file mode 100644 index 000000000..83e166005 --- /dev/null +++ b/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Transports/Hid/HidInterfaceClassifierTests.cs @@ -0,0 +1,72 @@ +// Copyright 2025 Yubico AB +// +// Licensed under the Apache License, Version 2.0 (the "License"). +// You may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using Yubico.YubiKit.Core.Transports.Hid; + +namespace Yubico.YubiKit.Core.UnitTests.Transports.Hid; + +public class HidInterfaceClassifierTests +{ + private static HidDescriptorInfo MakeDescriptor(ushort usagePage, ushort usage) => + new() + { + UsagePage = usagePage, + Usage = usage, + DevicePath = "test", + VendorId = HidConstants.YubicoVendorId, + ProductId = 0x0407 + }; + + [Fact] + public void Classify_FidoDescriptor_ReturnsFido() => + Assert.Equal( + HidInterfaceType.Fido, + HidInterfaceClassifier.Classify(MakeDescriptor(0xF1D0, 0x0001))); + + [Fact] + public void Classify_OtpDescriptor_ReturnsOtp() => + Assert.Equal( + HidInterfaceType.Otp, + HidInterfaceClassifier.Classify(MakeDescriptor(0x0001, 0x0006))); + + [Fact] + public void Classify_UnknownDescriptor_ReturnsUnknown() => + Assert.Equal( + HidInterfaceType.Unknown, + HidInterfaceClassifier.Classify(MakeDescriptor(0x0001, 0x0001))); + + [Fact] + public void IsSupported_FidoDescriptor_ReturnsTrue() => + Assert.True(HidInterfaceClassifier.IsSupported(MakeDescriptor(0xF1D0, 0x0001))); + + [Fact] + public void IsSupported_OtpDescriptor_ReturnsTrue() => + Assert.True(HidInterfaceClassifier.IsSupported(MakeDescriptor(0x0001, 0x0006))); + + [Fact] + public void IsSupported_UnknownDescriptor_ReturnsFalse() => + Assert.False(HidInterfaceClassifier.IsSupported(MakeDescriptor(0x0001, 0x0001))); + + [Fact] + public void GetReportType_Fido_ReturnsInputOutput() => + Assert.Equal(HidReportType.InputOutput, HidInterfaceClassifier.GetReportType(HidInterfaceType.Fido)); + + [Fact] + public void GetReportType_Otp_ReturnsFeature() => + Assert.Equal(HidReportType.Feature, HidInterfaceClassifier.GetReportType(HidInterfaceType.Otp)); + + [Fact] + public void GetReportType_Unknown_ThrowsArgumentException() => + Assert.Throws(() => HidInterfaceClassifier.GetReportType(HidInterfaceType.Unknown)); +} From 51ac5b2d4f1ee12ee6015ac0cffc0c782a570e4e Mon Sep 17 00:00:00 2001 From: Dennis Dyall Date: Wed, 24 Jun 2026 14:50:47 +0200 Subject: [PATCH 21/25] refactor: code style and warnings --- src/Core/src/Native/SdkPlatformInfo.cs | 2 -- src/Core/src/Transports/Hid/FindHidDevices.cs | 27 +++++++------------ 2 files changed, 9 insertions(+), 20 deletions(-) diff --git a/src/Core/src/Native/SdkPlatformInfo.cs b/src/Core/src/Native/SdkPlatformInfo.cs index d133fe9e8..dcf263772 100644 --- a/src/Core/src/Native/SdkPlatformInfo.cs +++ b/src/Core/src/Native/SdkPlatformInfo.cs @@ -60,10 +60,8 @@ public static bool IsElevated { if (OperatingSystem == SdkPlatform.Windows) { -#pragma warning disable CA1416 return new WindowsPrincipal(WindowsIdentity.GetCurrent()).IsInRole(WindowsBuiltInRole .Administrator); -#pragma warning restore CA1416 } return false; diff --git a/src/Core/src/Transports/Hid/FindHidDevices.cs b/src/Core/src/Transports/Hid/FindHidDevices.cs index b0e76e0f6..a0c0c6e51 100644 --- a/src/Core/src/Transports/Hid/FindHidDevices.cs +++ b/src/Core/src/Transports/Hid/FindHidDevices.cs @@ -15,6 +15,7 @@ using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using System.Runtime.Versioning; +using Yubico.YubiKit.Core.Native; using Yubico.YubiKit.Core.Transports.Hid.Linux; using Yubico.YubiKit.Core.Transports.Hid.MacOS; using Yubico.YubiKit.Core.Transports.Hid.Windows; @@ -46,25 +47,15 @@ private IReadOnlyList FindAll() return yubicoDevices; } - private IReadOnlyList GetPlatformDevices() - { - if (OperatingSystem.IsMacOS()) - { - return FindAllMacOS(); - } - - if (OperatingSystem.IsLinux()) - { - return FindAllLinux(); - } - - if (OperatingSystem.IsWindows()) + private IReadOnlyList GetPlatformDevices() => + SdkPlatformInfo.OperatingSystem switch { - return FindAllWindows(); - } - - return []; - } + SdkPlatform.MacOS => FindAllMacOS(), + SdkPlatform.Linux => FindAllLinux(), + SdkPlatform.Windows => FindAllWindows(), + SdkPlatform.Unknown => [], + _ => [] + }; [SupportedOSPlatform("macos")] private static IReadOnlyList FindAllMacOS() => From 630c697843c88411d165c8089670a8f85cd48e1d Mon Sep 17 00:00:00 2001 From: Dennis Dyallo Date: Wed, 24 Jun 2026 15:27:36 +0200 Subject: [PATCH 22/25] Delete src/Core/tests/Yubico.YubiKit.Core.UnitTests/Sessions/ApplicationIdsTests.cs --- .../Sessions/ApplicationIdsTests.cs | 29 ------------------- 1 file changed, 29 deletions(-) delete mode 100644 src/Core/tests/Yubico.YubiKit.Core.UnitTests/Sessions/ApplicationIdsTests.cs diff --git a/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Sessions/ApplicationIdsTests.cs b/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Sessions/ApplicationIdsTests.cs deleted file mode 100644 index 07c959d65..000000000 --- a/src/Core/tests/Yubico.YubiKit.Core.UnitTests/Sessions/ApplicationIdsTests.cs +++ /dev/null @@ -1,29 +0,0 @@ -// Copyright 2026 Yubico AB -// -// Licensed under the Apache License, Version 2.0 (the "License"). -// You may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -using Yubico.YubiKit.Core.Sessions; - -namespace Yubico.YubiKit.Core.UnitTests.Sessions; - -public class ApplicationIdsTests -{ - [Fact] - public void Piv_UsesRidPlusPixAid() - { - Assert.Equal(9, ApplicationIds.Piv.Length); - Assert.Equal( - new byte[] { 0xA0, 0x00, 0x00, 0x03, 0x08, 0x00, 0x00, 0x10, 0x00 }, - ApplicationIds.Piv); - } -} From a0b78a6016f5fbad1beb088a165b0beb1ae332f7 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Fri, 26 Jun 2026 12:32:52 +0000 Subject: [PATCH 23/25] refactor(fido2): remove PuvathRequired backwards compat alias Co-authored-by: Dennis Dyallo --- src/Fido2/src/Ctap/CtapStatus.cs | 6 ------ .../Yubico.YubiKit.Fido2.UnitTests/CtapExceptionTests.cs | 9 --------- 2 files changed, 15 deletions(-) diff --git a/src/Fido2/src/Ctap/CtapStatus.cs b/src/Fido2/src/Ctap/CtapStatus.cs index 2f2fd5d25..c7baa5d12 100644 --- a/src/Fido2/src/Ctap/CtapStatus.cs +++ b/src/Fido2/src/Ctap/CtapStatus.cs @@ -212,12 +212,6 @@ public enum CtapStatus : byte /// PuatRequired = 0x36, - /// - /// A pinUvAuthToken is required for the selected operation. - /// - [Obsolete("Use PuatRequired instead.", false)] - PuvathRequired = PuatRequired, - /// /// PIN policy violation. Currently only enforces minimum length. /// diff --git a/src/Fido2/tests/Yubico.YubiKit.Fido2.UnitTests/CtapExceptionTests.cs b/src/Fido2/tests/Yubico.YubiKit.Fido2.UnitTests/CtapExceptionTests.cs index 244a91520..94fdc6367 100644 --- a/src/Fido2/tests/Yubico.YubiKit.Fido2.UnitTests/CtapExceptionTests.cs +++ b/src/Fido2/tests/Yubico.YubiKit.Fido2.UnitTests/CtapExceptionTests.cs @@ -75,15 +75,6 @@ public void ThrowIfError_WithByte_ThrowsException() public void Ctap22StatusValues_MatchSpecification(CtapStatus status, byte expectedValue) => ((byte)status).Should().Be(expectedValue); - [Fact] - public void PuvathRequired_ObsoleteAlias_MapsToPuatRequired() - { -#pragma warning disable CS0618 - CtapStatus.PuvathRequired.Should().Be(CtapStatus.PuatRequired); - ((byte)CtapStatus.PuvathRequired).Should().Be(0x36); -#pragma warning restore CS0618 - } - [Theory] [InlineData(CtapStatus.InvalidCommand, "Invalid CTAP command")] [InlineData(CtapStatus.InvalidParameter, "Invalid parameter")] From fd7d983ff14909cf62b46f26938e4c10504de1f4 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Mon, 29 Jun 2026 23:36:53 +0000 Subject: [PATCH 24/25] fix(core): use ArgumentException for HID report buffer length validation SetFeatureReport and SetOutputReport were throwing InvalidOperationException for caller-supplied buffers of the wrong length, which misrepresents the failure as object state rather than invalid input. Switch to ArgumentException and include expected vs actual lengths for actionable diagnostics. Co-authored-by: Dennis Dyallo --- src/Core/src/Native/Windows/HidD/HidDDevice.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/Core/src/Native/Windows/HidD/HidDDevice.cs b/src/Core/src/Native/Windows/HidD/HidDDevice.cs index 23b8696fb..8bbe70d35 100644 --- a/src/Core/src/Native/Windows/HidD/HidDDevice.cs +++ b/src/Core/src/Native/Windows/HidD/HidDDevice.cs @@ -77,7 +77,9 @@ public void SetFeatureReport(byte[] buffer) if (buffer.Length != FeatureReportByteLength - 1) { - throw new InvalidOperationException("The HID feature report buffer length is invalid."); + throw new ArgumentException( + $"The HID feature report buffer length is invalid. Expected {FeatureReportByteLength - 1} bytes, but got {buffer.Length}.", + nameof(buffer)); } // Windows expects the report ID byte before the report payload. @@ -114,7 +116,9 @@ public void SetOutputReport(byte[] buffer) if (buffer.Length != OutputReportByteLength - 1) { - throw new InvalidOperationException("The HID output report buffer length is invalid."); + throw new ArgumentException( + $"The HID output report buffer length is invalid. Expected {OutputReportByteLength - 1} bytes, but got {buffer.Length}.", + nameof(buffer)); } // Windows expects the report ID byte before the report payload. From 82b32a49b1746202067232bc2156667c9cbcd768 Mon Sep 17 00:00:00 2001 From: Dennis Dyall Date: Tue, 30 Jun 2026 13:47:26 +0200 Subject: [PATCH 25/25] fix(core): preserve SmartCard context on stuck listener --- .../DesktopSmartCardDeviceListener.cs | 8 +++++++ toolchain.cs | 22 ++++++++++++++++++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/Core/src/Transports/SmartCard/DesktopSmartCardDeviceListener.cs b/src/Core/src/Transports/SmartCard/DesktopSmartCardDeviceListener.cs index b111fd1b3..2d57bd0f4 100644 --- a/src/Core/src/Transports/SmartCard/DesktopSmartCardDeviceListener.cs +++ b/src/Core/src/Transports/SmartCard/DesktopSmartCardDeviceListener.cs @@ -194,6 +194,14 @@ private void StopListening() { // The listener may still be using the native context. Prefer a bounded leak over // disposing a handle that could still be active on the background thread. + lock (_syncLock) + { + if (ReferenceEquals(_context, context)) + { + _context = null; + } + } + return; } diff --git a/toolchain.cs b/toolchain.cs index b12046200..765079de8 100755 --- a/toolchain.cs +++ b/toolchain.cs @@ -23,6 +23,7 @@ * build - Build the solution (restores only if needed) * test - Run unit tests with summary output * resilience - Run fast no-hardware runtime resilience gates + * benchmark - Run performance benchmarks (manual, not CI) * docs-qa - Validate active documentation hygiene * coverage - Run tests with code coverage * pack - Create NuGet packages @@ -45,6 +46,7 @@ * --integration Include integration tests (requires --project, unit tests only by default) * --smoke Smoke test mode: skip Slow and RequiresUserPresence tests * --fast Required fast mode for resilience gates + * --benchmark-args Arguments passed to BenchmarkDotNet * * EXAMPLES: * dotnet toolchain.cs build @@ -54,6 +56,7 @@ * dotnet toolchain.cs test --filter "FullyQualifiedName~MyTestClass" * dotnet toolchain.cs test --project Piv --filter "Method~Sign" * dotnet toolchain.cs -- resilience --fast + * dotnet toolchain.cs benchmark --benchmark-args "--list flat" * dotnet toolchain.cs -- test --integration --project Piv --smoke (quick integration smoke test) * dotnet toolchain.cs coverage * dotnet toolchain.cs publish --package-version 1.0.0-preview.1 @@ -121,6 +124,7 @@ var includeIntegration = HasFlag("--integration"); var smokeTest = HasFlag("--smoke"); var fastMode = HasFlag("--fast"); +var benchmarkArgs = GetArgument("--benchmark-args") ?? ""; // --smoke injects trait filters to skip slow and user-presence tests if (smokeTest) @@ -326,6 +330,19 @@ } }); +Target("benchmark", () => +{ + PrintHeader("Running performance benchmarks"); + + var benchmarkProject = Path.Combine( + repoRoot, + "benchmarks", + "Yubico.YubiKit.PerformanceBenchmarks", + "Yubico.YubiKit.PerformanceBenchmarks.csproj"); + + Run("dotnet", $"run --project \"{benchmarkProject}\" -c {configuration} -- {benchmarkArgs}"); +}); + Target("docs-qa", () => { PrintHeader("Validating active documentation"); @@ -434,7 +451,7 @@ // Run Bullseye — strip all custom args so Bullseye only sees target names and its own flags var bullseyeArgs = FilterBullseyeArgs(args, optionsWithValues: ["--project", "--filter", "--package-version", "--nuget-feed-name", "--nuget-feed-path", - "--nuget-feed-url", "--nuget-api-key"], + "--nuget-feed-url", "--nuget-api-key", "--benchmark-args"], flags: ["--integration", "--include-docs", "--dry-run", "--clean", "--smoke", "--fast"]); await RunTargetsAndExitAsync(bullseyeArgs); @@ -662,6 +679,7 @@ dotnet toolchain.cs -- build --project Piv Use when in doubt build - Build the solution (restores only if needed) test - Run unit tests with summary output resilience - Run fast no-hardware runtime resilience gates + benchmark - Run performance benchmarks (manual, not CI) docs-qa - Validate active documentation hygiene coverage - Run tests with code coverage pack - Create NuGet packages @@ -684,6 +702,7 @@ dotnet toolchain.cs -- build --project Piv Use when in doubt --integration Include integration tests (requires --project) --smoke Smoke test mode: skip Slow and RequiresUserPresence tests --fast Required fast mode for resilience gates + --benchmark-args Arguments passed to BenchmarkDotNet -h, --help Show this help message EXAMPLES: @@ -694,6 +713,7 @@ dotnet toolchain.cs docs-qa dotnet toolchain.cs test --filter ""FullyQualifiedName~MyTestClass"" dotnet toolchain.cs test --project Piv --filter ""Method~Sign"" dotnet toolchain.cs -- resilience --fast + dotnet toolchain.cs benchmark --benchmark-args ""--list flat"" dotnet toolchain.cs -- test --integration --project Piv --smoke dotnet toolchain.cs coverage dotnet toolchain.cs publish --package-version 1.0.0-preview.1