Skip to content

feat: Implement reading of VersionQualifier into YubikeyDeviceInfo #240

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jun 10, 2025

Conversation

DennisDyallo
Copy link
Collaborator

@DennisDyallo DennisDyallo commented Jun 9, 2025

Description

This pull request introduces several enhancements and new features to the Yubico.YubiKey library, focusing on firmware version handling, version qualifiers, and unit tests. The most important changes include adding a VersionQualifier class, updating YubiKeyDeviceInfo to support version qualifiers, and introducing comprehensive unit tests for these updates.

Fixes: YESDK-1451

Type of change

  • Refactor (non-breaking change which improves code quality or performance)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How has this been tested?

I have ran unit tests similar to those in the Java Yubikit SDK. I have yet to run them on actual keys.

Test configuration:

  • OS version: Linux
  • Firmware version: N/A
  • Yubikey model1: N/A

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have run dotnet format to format my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Firmware Version Enhancements:

  • New method to parse firmware versions from byte arrays: Added FirmwareVersion.FromBytes method to create a FirmwareVersion instance from a byte array containing major, minor, and patch versions. Includes validation for the byte array length. (FirmwareVersion.cs, Yubico.YubiKey/src/Yubico/YubiKey/FirmwareVersion.csR95-R115)

Version Qualifier Feature:

Integration with YubiKeyDeviceInfo:

  • Support for version qualifiers in YubiKeyDeviceInfo: Added a VersionQualifier property and logic to handle version qualifiers in the CreateFromResponseData method. The firmware version is overridden by the version qualifier when applicable. (YubiKeyDeviceInfo.cs, [1] [2]
  • Updated YubiKeyDeviceInfo constructor: Default VersionQualifier is initialized to Final with iteration 0. (YubiKeyDeviceInfo.cs, Yubico.YubiKey/src/Yubico/YubiKey/YubiKeyDeviceInfo.csR121)

Unit Tests:

  • Tests for VersionQualifier: Comprehensive unit tests for VersionQualifier class, covering initialization, string representation, equality, and hash code functionality. (VersionQualifierTests.cs, Yubico.YubiKey/tests/unit/Yubico/YubiKey/VersionQualifierTests.csR1-R97)
  • Refactored YubiKeyDeviceInfoTests: Improved test readability by introducing a DefaultInfo helper method to streamline test setup. (YubikeyDeviceInfoTests.cs, [1] [2] [3]

Footnotes

  1. See Yubikey models (Multi-protocol, Security Key, FIPS, Bio, YubiHSM, YubiHSM FIPS)

@DennisDyallo DennisDyallo requested a review from Copilot June 9, 2025 15:33
Copilot

This comment was marked as outdated.

Copy link

github-actions bot commented Jun 9, 2025

Test Results: Windows

    2 files      2 suites   9s ⏱️
3 905 tests 3 905 ✅ 0 💤 0 ❌
3 907 runs  3 907 ✅ 0 💤 0 ❌

Results for commit f15f0e4.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jun 9, 2025

Test Results: Ubuntu

    2 files      2 suites   16s ⏱️
3 897 tests 3 897 ✅ 0 💤 0 ❌
3 899 runs  3 899 ✅ 0 💤 0 ❌

Results for commit f15f0e4.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jun 9, 2025

Test Results: MacOS

    2 files      2 suites   11s ⏱️
3 897 tests 3 897 ✅ 0 💤 0 ❌
3 899 runs  3 899 ✅ 0 💤 0 ❌

Results for commit f15f0e4.

♻️ This comment has been updated with latest results.

@DennisDyallo DennisDyallo force-pushed the dennisdyallo/version-qualifier branch from dc9cf7e to 9789d88 Compare June 9, 2025 15:44
@DennisDyallo DennisDyallo requested a review from Copilot June 9, 2025 15:45
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for reading and representing firmware version qualifiers in YubiKeyDeviceInfo, enhances version parsing, and covers these changes with unit tests.

  • Introduce VersionQualifier class and corresponding TLV tag
  • Update YubiKeyDeviceInfo to parse, store, and override firmware version based on qualifiers
  • Add FromBytes to FirmwareVersion and comprehensive unit tests

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/unit/Yubico/YubiKey/YubikeyDeviceInfoTests.cs Refactor test helpers, replace DeviceInfoFor defaults, add qualifier parsing tests
tests/unit/Yubico/YubiKey/VersionQualifierTests.cs New unit tests for VersionQualifier behavior
src/Yubico/YubiKey/YubikeyDeviceManagementTags.cs Add VersionQualifierTag constant
src/Yubico/YubiKey/YubiKeyDeviceInfo.cs Parse and apply version qualifiers; add VersionName property
src/Yubico/YubiKey/VersionQualifier.cs Implement VersionQualifier class
src/Yubico/YubiKey/FirmwareVersion.cs Add FromBytes factory for 3-byte firmware version parsing
Comments suppressed due to low confidence (4)

Yubico.YubiKey/src/Yubico/YubiKey/YubiKeyDeviceInfo.cs:375

  • Using '!=' on a class compares references, not values, so this check will always be true (different instances). As a result, default qualifiers will never defer to second. Use !VersionQualifier.Equals(new VersionQualifier()) or compare against a shared default instance instead.
VersionQualifier = VersionQualifier != new VersionQualifier() ? VersionQualifier : second.VersionQualifier,

Yubico.YubiKey/src/Yubico/YubiKey/VersionQualifier.cs:60

  • The documentation states an upper bound of int.MaxValue, but the code actually allows up to uint.MaxValue. Update the doc to reflect the correct maximum or adjust the code to match.
/// <param name="iteration">The iteration number of the version qualifier, must be a non-negative value and less than or equal to int.MaxValue.</param>

Yubico.YubiKey/src/Yubico/YubiKey/YubiKeyDeviceInfo.cs:321

  • [nitpick] Local variable names in C# should be camelCase. Rename Logger to logger to follow naming conventions.
var Logger = Core.Logging.Log.GetLogger<YubiKeyDeviceInfo>();

Yubico.YubiKey/tests/unit/Yubico/YubiKey/YubikeyDeviceInfoTests.cs:263

  • DefaultInfo currently passes an empty array to CreateFromResponseData, which expects a Dictionary<int, ReadOnlyMemory>. Change this to pass an empty Dictionary<int, ReadOnlyMemory> (e.g., new Dictionary<int, ReadOnlyMemory>()).
private static YubiKeyDeviceInfo DefaultInfo() => YubiKeyDeviceInfo.CreateFromResponseData([]);

@DennisDyallo DennisDyallo marked this pull request as ready for review June 9, 2025 17:45
Copy link
Member

@AdamVe AdamVe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@DennisDyallo DennisDyallo self-assigned this Jun 10, 2025
@DennisDyallo DennisDyallo added the enhancement New feature or request label Jun 10, 2025
Copy link

Code Coverage

Package Line Rate Branch Rate Complexity Health
Yubico.Core 40% 31% 4365
Yubico.YubiKey 51% 46% 20700
Summary 49% (35538 / 72525) 44% (8706 / 19851) 25065

Minimum allowed line rate is 40%

@DennisDyallo DennisDyallo merged commit b169aa2 into develop Jun 10, 2025
12 checks passed
@DennisDyallo DennisDyallo deleted the dennisdyallo/version-qualifier branch June 10, 2025 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

2 participants