fix,tests: Fixed bug where attest cert could not be RSA3072 or RSA4096, removed obsolete tests and consolidated Piv tests #230

DennisDyallo · 2025-05-05T10:35:53Z

Description

This PR fixes a bug where attestation cert could not be RSA3072 or RSA4096, as well as updates a number of tests that were made obsolete in recent refactors around ECPublicKey, ECPrivateKey, RSAPublicKey and RSAPrivateKey.

Additional changes:

logging: Added logging when changing pin or puk
refactor: refactor: Proper parsing of X509Certificate2 around Piv attestation and importing of attest certificate
refactor: Consolidated logic around key size and key definition lookup in Piv
refactor: Consolidated logic around GetMetadata in Piv
docs: Removed legacy documentation regarding valid key slots for creating attestations

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?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Test configuration:

OS version: Arch Linux
Firmware version: 5.7.4, 5.4.3
Yubikey model¹: USB A, USB C

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

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

add tests to verify private key with piv and asn encoding/decoding works add tests to verify functionality of RSAPublicKey.CreateFromPkcs8

refactor rsaformat refactor cryptosupport

… components

use TestKeys class use common base class for Piv remove SampleKeyPairs.cs removed CertConverter

github-actions · 2025-05-05T10:40:10Z

Test Results: Windows

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

Results for commit dbb6718.

♻️ This comment has been updated with latest results.

github-actions · 2025-05-05T10:40:19Z

Test Results: Ubuntu

2 files 2 suites 13s ⏱️
3 889 tests 3 889 ✅ 0 💤 0 ❌
3 891 runs 3 891 ✅ 0 💤 0 ❌

Results for commit dbb6718.

♻️ This comment has been updated with latest results.

github-actions · 2025-05-05T10:40:28Z

Test Results: MacOS

2 files 2 suites 9s ⏱️
3 889 tests 3 889 ✅ 0 💤 0 ❌
3 891 runs 3 891 ✅ 0 💤 0 ❌

Results for commit dbb6718.

♻️ This comment has been updated with latest results.

Yubico.YubiKey/tests/integration/Yubico/YubiKey/DeviceResetTests.cs

Yubico.YubiKey/src/Yubico/YubiKey/Fido2/Commands/CredentialManagementCommand.cs

remove unnecessary test on size of certs reduced test logic remove unused class add default pin and puk to base class

remove overlapping test add docs

fix tests with auth add complex pin and puk

- added field DefaultManagementKey - early exit when fail TryVerifyPin - use range operator

…essful (PivSession) - additional: added param docs for mutualAuthentication

AttestTests - added comment on CreateAttestationStatement Added test for Ed25519 attest Fix tests in PivSessionTests

- lookup of key definitions - determine valid attestation cert

Copilot

Pull Request Overview

This pull request fixes a bug preventing RSA3072 and RSA4096 attestation certificates from being used, removes obsolete tests, and refactors several command and cryptographic methods to improve clarity and consistency. Key changes include:

Bug fix and consolidation of key size and definition lookup in PIV commands.
Refactoring of obsolete test invocations and adjustments in exception handling.
Code style improvements such as replacing explicit equality checks with negated Boolean expressions and updating inline comments.

Reviewed Changes

Copilot reviewed 148 out of 148 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
Yubico.YubiKey/src/Yubico/YubiKey/Piv/Commands/GetMetadataCommand.cs	Removed explicit initialization of _slotNumber in the parameterless constructor.
Yubico.YubiKey/src/Yubico/YubiKey/Piv/Commands/CreateAttestationStatementCommand.cs	Replaced equality checks with negated conditions for slot number validation.
Yubico.YubiKey/src/Yubico/YubiKey/Piv/Commands/AuthenticateSignCommand.cs	Refactored algorithm determination into a separate method using LINQ.
Yubico.YubiKey/src/Yubico/YubiKey/Fido2/Commands/*	Updated ReadOnlyMemory constructions to use full authParam data.
Yubico.YubiKey/src/Yubico/YubiKey/Cryptography/RSAPublicKey.cs	Moved private key parameter validation into CreateFromParameters and updated key definition lookup.
Yubico.YubiKey/src/Yubico/YubiKey/Cryptography/KeyDefinitions.cs	Introduced a convenience method for RSA modulus length and clarified ECDSA OID error handling.
Yubico/YubiKey/src/Yubico/YubiKey/Cryptography/ECPublicKey.cs	Updated return types for public key creation methods for better type clarity.
Yubico.YubiKey/src/Yubico/YubiKey/Cryptography/AsnUtilities.cs	Revised bit clamping checks for X25519 with updated (but inconsistent) inline comments.
Yubico.YubiKey/src/Yubico/YubiKey/Cryptography/AsnPublicKeyEncoder.cs	Changed exception type and message for missing RSA parameters.
Various examples under /examples/PivSampleCode/	Added pragma warning disables around obsolete API usage.

Comments suppressed due to low confidence (1)

Yubico.YubiKey/src/Yubico/YubiKey/Cryptography/AsnPublicKeyEncoder.cs:105

[nitpick] The exception thrown for missing RSA parameter values now uses InvalidOperationException with a different message. Consider aligning the exception type and message with similar methods to ensure consistency in error reporting.

if (parameters.Exponent == null ||

Yubico.YubiKey/src/Yubico/YubiKey/Piv/Commands/GetMetadataCommand.cs

Yubico.YubiKey/src/Yubico/YubiKey/Cryptography/AsnUtilities.cs

tests: fixed comments and code restructure

# Conflicts: # Yubico.YubiKey/src/Yubico/YubiKey/Piv/PivSession.Attestation.cs # Yubico.YubiKey/src/Yubico/YubiKey/Piv/PivSession.Crypto.cs # Yubico.YubiKey/tests/integration/Yubico/YubiKey/Piv/ImportTests.cs # Yubico.YubiKey/tests/unit/Yubico/YubiKey/Cryptography/ECPrivateKeyTests.cs # Yubico.YubiKey/tests/unit/Yubico/YubiKey/Cryptography/ECPublicKeyTests.cs # Yubico.YubiKey/tests/unit/Yubico/YubiKey/Cryptography/RSAPublicKeyTests.cs # Yubico.YubiKey/tests/unit/Yubico/YubiKey/Piv/PivEncoderDecoderTests.cs # Yubico.YubiKey/tests/unit/Yubico/YubiKey/Piv/PivPrivateKeyTests.cs # Yubico.YubiKey/tests/unit/Yubico/YubiKey/Piv/PivPublicKeyTests.cs # Yubico.YubiKey/tests/utilities/Yubico/YubiKey/TestUtilities/KeyConverter.cs # Yubico.YubiKey/tests/utilities/Yubico/YubiKey/TestUtilities/PivKeyExtensions.cs # Yubico.YubiKey/tests/utilities/Yubico/YubiKey/TestUtilities/SampleKeyPairs.cs # Yubico.YubiKey/tests/utilities/Yubico/YubiKey/TestUtilities/TestKeyExtensions.cs

AdamVe

LGTM. I ran the integration tests locally also. See my comments as ideas for future improvements.

Yubico.YubiKey/src/Yubico/YubiKey/Piv/PivSession.Attestation.cs

Yubico.YubiKey/tests/integration/Yubico/YubiKey/Fido2/LargeBlobTests.cs

AdamVe · 2025-05-27T14:37:06Z

Yubico.YubiKey/tests/integration/Yubico/YubiKey/Piv/PinNoCollectorTests.cs

+        public void ChangeRetryCounts_Succeeds(
+            StandardTestDevice testDeviceType)


out of curiosity, is this the way the formatter works on the c# code?

I dont know honestly, this is the way the formatter works with the current .editorconfig and Rider, so I dont question it too much. Im sure it could be optimized. I would only want it to split into rows if the params are larger than 2. Now I think it splits when it reaches a current viewport width

Yubico.YubiKey/tests/integration/Yubico/YubiKey/Piv/PivSessionIntegrationTestBase.cs

remove comments, revert test change, simpler complexpin and complexpuk

github-actions · 2025-05-27T22:35:12Z

Package	Line Rate	Branch Rate	Complexity	Health
Yubico.Core	40%	31%	4365	➖
Yubico.YubiKey	51%	46%	20595	➖
Summary	49% (35397 / 72330)	44% (8637 / 19755)	24960	➖

Minimum allowed line rate is 40%

DennisDyallo and others added 20 commits April 22, 2025 14:51

tests: added ability to select usbcbio

2472f0e

wip: remake tests

3f735f2

samples: fix sample project

f5ef846

Remove incorrect auth token truncation from various commands

3d1dbc6

commits from PR204

1971953

typos, other minor fixes

1f51a6b

misc: add comments, clarify return type

74838b0

tests: remove bio requirement on fido tests

fcd00ff

tests: improved usability of TestKeys

59ea863

docs: fix wording

30b3db7

misc: separate into files

4131d0e

tests: wording, misc

4dea521

tests: added tests for RSA keys

836635d

add tests to verify private key with piv and asn encoding/decoding works add tests to verify functionality of RSAPublicKey.CreateFromPkcs8

tests: refactor

0ec46e1

refactor rsaformat refactor cryptosupport

change: EncodeToSubjectPublicKeyInfo will throw on missing public key…

3c1632f

… components

tests: refactor CryptoSupport.cs

07ea148

tests: refactored tests

c3a6621

use TestKeys class use common base class for Piv remove SampleKeyPairs.cs removed CertConverter

tests: update cert data to include CA extensions

23b6ee9

tests: remove obsolete class

46295ec

tests: piv test use reusable base class and all pass

9b82282

DennisDyallo commented May 5, 2025

View reviewed changes

Yubico.YubiKey/tests/integration/Yubico/YubiKey/DeviceResetTests.cs Outdated Show resolved Hide resolved

DennisDyallo commented May 5, 2025

View reviewed changes

Yubico.YubiKey/src/Yubico/YubiKey/Fido2/Commands/CredentialManagementCommand.cs Show resolved Hide resolved

DennisDyallo added 5 commits May 7, 2025 13:49

misc: removed comments

37fe763

misc: Rename comment

0e0d65b

tests: fixed some tests

745b9b1

tests: refactor piv tests to reuse base class

80f09a9

tests: improved PivSession tests

279fd05

remove unnecessary test on size of certs reduced test logic remove unused class add default pin and puk to base class

DennisDyallo added 5 commits May 15, 2025 16:50

tests: misc Scp test changes

8c52f4e

tests:

c268c1d

remove overlapping test add docs

tests: PivSession

32256bf

fix tests with auth add complex pin and puk

tests: Fix PinOnlyWithResetTests.cs for devices gt v5.7

96196da

refactor: PivSession

a7b54cc

- added field DefaultManagementKey - early exit when fail TryVerifyPin - use range operator

DennisDyallo force-pushed the dennisdyallo/tests branch from 7164b9e to a7b54cc Compare May 15, 2025 16:29

DennisDyallo added 8 commits May 16, 2025 17:33

fix: bug where SDK would print a failure log even though we were succ…

abf8c71

…essful (PivSession) - additional: added param docs for mutualAuthentication

tests: minor refactor for tests in PivSession

32f35d8

misc: added logging for changing pin / puk

d307713

refactor: reuse logic for GetMetadata (PivSession)

ace1a0a

tests:

46999f0

AttestTests - added comment on CreateAttestationStatement Added test for Ed25519 attest Fix tests in PivSessionTests

refactor: minor work on Piv Attestation and RSAPublicKey

5c5e93a

- lookup of key definitions - determine valid attestation cert

misc: code cleanup

7bca86e

misc: remove add default management key

78b1f2c

DennisDyallo changed the title ~~tests: Updated obsolete tests~~ fix,tests: Fixed bug where attest cert could not be RSA3072 or RSA4096, removed obsolete tests and consolidated Piv tests May 26, 2025

DennisDyallo requested a review from Copilot May 26, 2025 00:02

Copilot AI reviewed May 26, 2025

View reviewed changes

Yubico.YubiKey/src/Yubico/YubiKey/Piv/Commands/GetMetadataCommand.cs Show resolved Hide resolved

Yubico.YubiKey/src/Yubico/YubiKey/Cryptography/AsnUtilities.cs Show resolved Hide resolved

DennisDyallo added 7 commits May 26, 2025 12:14

tests: PivSessionIntegrationTestBase.cs can now manage Fips credentials

2705379

tests: rename class

3eea339

misc: added logging when generate key

4e3aade

tests: added FIPS tests for generate

9b2a69f

test: comments, restructure

68fecf7

tests: fixed comments and code restructure

Merge branch 'develop' into dennisdyallo/tests

b3f5446

AdamVe approved these changes May 27, 2025

View reviewed changes

misc: pr feedback

92da903

remove comments, revert test change, simpler complexpin and complexpuk

DennisDyallo merged commit 76e5dd6 into develop May 27, 2025
4 checks passed

DennisDyallo deleted the dennisdyallo/tests branch May 27, 2025 22:32

DennisDyallo mentioned this pull request Jun 25, 2025

Release 1.13.2 #259

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix,tests: Fixed bug where attest cert could not be RSA3072 or RSA4096, removed obsolete tests and consolidated Piv tests #230

fix,tests: Fixed bug where attest cert could not be RSA3072 or RSA4096, removed obsolete tests and consolidated Piv tests #230

Uh oh!

DennisDyallo commented May 5, 2025 •

edited

Loading

Uh oh!

github-actions bot commented May 5, 2025 •

edited

Loading

Uh oh!

github-actions bot commented May 5, 2025 •

edited

Loading

Uh oh!

github-actions bot commented May 5, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

AdamVe left a comment

Uh oh!

Uh oh!

Uh oh!

AdamVe May 27, 2025

Uh oh!

DennisDyallo May 27, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented May 27, 2025

Uh oh!

Uh oh!

		public void ChangeRetryCounts_Succeeds(
		StandardTestDevice testDeviceType)

fix,tests: Fixed bug where attest cert could not be RSA3072 or RSA4096, removed obsolete tests and consolidated Piv tests #230

fix,tests: Fixed bug where attest cert could not be RSA3072 or RSA4096, removed obsolete tests and consolidated Piv tests #230

Uh oh!

Conversation

DennisDyallo commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How has this been tested?

Checklist:

Footnotes

Uh oh!

github-actions bot commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results: Windows

Uh oh!

github-actions bot commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results: Ubuntu

Uh oh!

github-actions bot commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results: MacOS

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

AdamVe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AdamVe May 27, 2025

Choose a reason for hiding this comment

Uh oh!

DennisDyallo May 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented May 27, 2025

Uh oh!

Uh oh!

DennisDyallo commented May 5, 2025 •

edited

Loading

github-actions bot commented May 5, 2025 •

edited

Loading

github-actions bot commented May 5, 2025 •

edited

Loading

github-actions bot commented May 5, 2025 •

edited

Loading