fix: Fixed bug importing PIV private key in legacy classes #231

DennisDyallo · 2025-05-05T15:06:35Z

This PR fixes #228 where one could where a legacy PivPrivateKey would not be handled properly in the new ImportPrivateKey(byte slotNumber, IPrivateKey). [1] as well as #227, when creating a PivEccPrivateKey(privateKeyBytes, PivAlgorithm) where the PivAlgorithm would not be set. [2]

Additionally, deprecating older key types (PivEccPublic, PivEccPrivateKey, etc.) in favor of modern implementations (ECPublicKey, ECPrivateKey, RSAPublicKey, etc.) and refining code structure. Below are the most significant changes grouped by theme:

Summarized by Copilot:

Deprecation of Legacy Key Types

Marked PivEccPublic, PivEccPrivateKey, PivRsaPublic, and PivRsaPrivateKey as obsolete, encouraging the use of ECPublicKey, ECPrivateKey, RSAPublicKey, and similar implementations instead. This affects multiple classes, including PivEccPrivateKey, PivEccPublicKey, and PivPrivateKey. [1] [2] [3] [4]

Memory Handling Improvements

Updated ZeroingMemoryHandle to use Memory<byte> instead of byte[], simplifying the class and improving memory safety. Removed unnecessary methods like CopyTo and Slice.
Changed calls to use .Span when accessing Memory<byte> in AsnPrivateKeyEncoder.cs to improve compatibility and performance. [1] [2]

New Constructor and Validation Enhancements

Added a new constructor to EcdsaVerify for ECPublicKey with validation for key type and curve.
Refactored size validation logic for ECC private keys into dedicated helper methods in PivEccPrivateKey. [1] [2]

Obsolete Method and Class Updates

Deprecated methods in PivKeyEncoder for encoding keys, redirecting users to KeyExtensions for modern alternatives. [1] [2]
Updated KeyExtensions to handle obsolete PivPublicKey and PivPrivateKey with warnings for backward compatibility.

Code Cleanup and Refactoring

Simplified and clarified logic in multiple places, such as removing redundant comments and unused code in PivEccPublicKey and PivMetadata. [1] [2]

github-actions · 2025-05-05T15:11:10Z

Test Results: Windows

2 files 2 suites 9s ⏱️
3 964 tests 3 964 ✅ 0 💤 0 ❌
3 966 runs 3 966 ✅ 0 💤 0 ❌

Results for commit 5b1352d.

♻️ This comment has been updated with latest results.

github-actions · 2025-05-05T15:11:20Z

Test Results: Ubuntu

2 files 2 suites 14s ⏱️
3 956 tests 3 956 ✅ 0 💤 0 ❌
3 958 runs 3 958 ✅ 0 💤 0 ❌

Results for commit 5b1352d.

♻️ This comment has been updated with latest results.

github-actions · 2025-05-05T15:11:29Z

Test Results: MacOS

2 files 2 suites 8s ⏱️
3 956 tests 3 956 ✅ 0 💤 0 ❌
3 958 runs 3 958 ✅ 0 💤 0 ❌

Results for commit 5b1352d.

♻️ This comment has been updated with latest results.

DennisDyallo · 2025-05-26T10:58:45Z

Yubico.YubiKey/src/Yubico/YubiKey/Piv/Converters/KeyExtensions.cs

-            _ => throw new ArgumentException("The type conversion for the specified key type is not supported", nameof(parameters))
+
+            #pragma warning disable CS0618 // Type or member is obsolete
+            PivPublicKey p => p.PivEncodedPublicKey.ToArray(),


This is the bugfix

DennisDyallo · 2025-05-26T10:58:58Z

Yubico.YubiKey/src/Yubico/YubiKey/Piv/Converters/KeyExtensions.cs

-            _ => throw new ArgumentException("The type conversion for the specified key type is not supported", nameof(parameters))
+
+            #pragma warning disable CS0618 // Type or member is obsolete
+            PivPrivateKey p => p.EncodedPrivateKey.ToArray(),


This is the bugfix

Copilot

Pull Request Overview

This PR fixes a bug in importing legacy PIV private keys and deprecates old Piv* key types in favor of modern IPublicKey/IPrivateKey implementations, while improving memory handling and code structure.

Deprecate legacy PivEcc*/PivRsa* types and update obsolete messages.
Replace raw byte arrays with Memory<byte> and Span<byte> in critical classes for better safety.
Add helper constructors and validation, plus code cleanup and new extension methods.

Reviewed Changes

Copilot reviewed 31 out of 31 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
ECPublicKeyTests.cs	Wrap obsolete `AsPivPublicKey` calls with warning pragmas
ECPrivateKeyTests.cs	Wrap obsolete `AsPivPrivateKey` calls with warning pragmas
ImportTests.cs	Add integration test for legacy `PivEccPrivateKey` import
PivSession.KeyPairs.cs	Use `ZeroingMemoryHandle` for key import and update obsolete message
PivSession.Crypto.cs	Update obsolete attribute message
PivSession.Attestation.cs	Update obsolete attribute message
PivRsaPublicKey.cs	Mark `PivRsaPublicKey` obsolete
PivRsaPrivateKey.cs	Mark `PivRsaPrivateKey` obsolete
PivPublicKey.cs	Refactor `Create`, add `ExportSubjectPublicKeyInfo`, update obsolete
PivPrivateKey.cs	Mark `PivPrivateKey` obsolete
PivMetadata.cs	Update obsolete attribute message
PivEccPublicKey.cs	Mark class obsolete, remove redundant comments
PivEccPrivateKey.cs	Add size validation helpers
Converters/PivKeyEncoder.cs	Convert to static, deprecate methods
Converters/KeyExtensions.cs	Replace `PivKeyEncoder` calls with extension methods
Commands/ImportAsymmetricKeyCommand.cs	Update obsolete attribute message
Commands/GenerateKeyPairResponse.cs	Disable obsolete warnings around legacy return types
Cryptography/ZeroingMemoryHandle.cs	Rewrite to use `Memory<byte>`, remove enumerator interfaces
Cryptography/EcdsaVerify.cs	Add `EcpPublicKey` constructor and preserve legacy constructor
Cryptography/AsnPrivateKeyEncoder.cs	Switch to `.Span` for private key octet string writes

Comments suppressed due to low confidence (5)

Yubico.YubiKey/src/Yubico/YubiKey/Piv/Converters/PivKeyEncoder.cs:82

The error message says "Unsupported public key type" in the private-key encoder. It should read "Unsupported private key type" to accurately describe the failure.

 _ => throw new ArgumentException("Unsupported public key type."),

Yubico.YubiKey/src/Yubico/YubiKey/Piv/PivEccPublicKey.cs:216

[nitpick] A comment explaining why YubiKeyEncodedKey is set using a slice was removed. Reintroduce a brief note (e.g. "// The metadata-encoded key is the nested slice of PivEncodedKey") to aid future readers.

PivEncodedKey = tlvWriter.Encode();

Yubico.YubiKey/src/Yubico/YubiKey/Piv/Converters/KeyExtensions.cs:33

The XML summary still refers to a "byte array" but the method now returns Memory<byte>. Update the documentation to reflect the change in return type.

/// <returns>A BER encoded byte array containing the encoded public key.</returns>

Yubico.YubiKey/src/Yubico/YubiKey/Cryptography/EcdsaVerify.cs:176

A new constructor for ECPublicKey was added but no unit tests verify its behavior. Consider adding tests that cover passing valid and invalid ECPublicKey instances.

public EcdsaVerify(ECPublicKey publicKey)

Yubico.YubiKey/src/Yubico/YubiKey/Piv/PivPublicKey.cs:150

The new ExportSubjectPublicKeyInfo override should be covered by unit tests to ensure it returns a valid SPKI encoding for both RSA and ECC cases.

public override byte[] ExportSubjectPublicKeyInfo()

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

Yubico.YubiKey/src/Yubico/YubiKey/Piv/PivEccPrivateKey.cs

Copilot · 2025-05-26T11:05:50Z

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

+    #pragma warning restore CS0618 // Type or member is obsolete
    {


The warning disable only wraps the class declaration, but the GetData() method still emits CS0618 warnings on the return type. Consider extending the pragma to cover GetData() or annotate it with [Obsolete] to fully suppress warnings.

Suggested change

#pragma warning restore CS0618 // Type or member is obsolete

{

{

#pragma warning disable CS0618 // Type or member is obsolete

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

AdamVe

LGTM

github-actions · 2025-05-26T14:23:24Z

Package	Line Rate	Branch Rate	Complexity	Health
Yubico.Core	40%	31%	4365	➖
Yubico.YubiKey	51%	46%	20592	➖
Summary	49% (35277 / 72303)	44% (8628 / 19761)	24957	➖

Minimum allowed line rate is 40%

DennisDyallo added 3 commits May 5, 2025 12:23

fix: fix conversion between PivPrivateKey and encoded formats

bf0cdbb

fix: fix setting of Algorithm in obsolete PivEccPrivateKey constructor

7fdc339

obsolete: set piv keys as obsolete

9c0008c

DennisDyallo changed the title ~~Dennisdyallo/1.13 fixes~~ fixes May 6, 2025

DennisDyallo added 3 commits May 7, 2025 12:56

refactor: change zeroMemoryHandle to work with Memory<byte>

933ab6d

docs: add remark for sensitive data

09dd9ae

test: add legacy test

22868ea

DennisDyallo changed the title ~~fixes~~ Fixed bug importing PIV private key in legacy classes and marked legacy types as obsolete May 26, 2025

DennisDyallo added 2 commits May 26, 2025 12:18

Update GenerateKeyPairResponse.cs

7f365ff

fix: EcdsaVerify cannot be Ed25519

5b1352d

DennisDyallo marked this pull request as ready for review May 26, 2025 10:23

DennisDyallo changed the title ~~Fixed bug importing PIV private key in legacy classes and marked legacy types as obsolete~~ fix: Fixed bug importing PIV private key in legacy classes May 26, 2025

DennisDyallo commented May 26, 2025

View reviewed changes

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

Copilot AI reviewed May 26, 2025

View reviewed changes

AdamVe approved these changes May 26, 2025

View reviewed changes

refactor: use KeyDefinition lookup for PivEccPrivateKey

a4b897f

DennisDyallo merged commit 3bcf999 into develop May 26, 2025
4 checks passed

DennisDyallo deleted the dennisdyallo/1.13-fixes branch May 26, 2025 14:22

DennisDyallo linked an issue May 26, 2025 that may be closed by this pull request

[BUG] PivEccPrivateKey does not set the Algorithm property when the algorithm parameter is passed to constructor #227

Closed

1 task

DennisDyallo mentioned this pull request May 26, 2025

[BUG] Unable to import PivPrivateKey into key slot #228

Closed

1 task

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: Fixed bug importing PIV private key in legacy classes #231

fix: Fixed bug importing PIV private key in legacy classes #231

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!

DennisDyallo May 26, 2025

Uh oh!

DennisDyallo May 26, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI May 26, 2025

Uh oh!

Uh oh!

AdamVe left a comment

Uh oh!

Uh oh!

github-actions bot commented May 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

		#pragma warning restore CS0618 // Type or member is obsolete
		{

fix: Fixed bug importing PIV private key in legacy classes #231

fix: Fixed bug importing PIV private key in legacy classes #231

Uh oh!

Conversation

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

Uh oh!

Deprecation of Legacy Key Types

Memory Handling Improvements

New Constructor and Validation Enhancements

Obsolete Method and Class Updates

Code Cleanup and Refactoring

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!

DennisDyallo May 26, 2025

Choose a reason for hiding this comment

Uh oh!

DennisDyallo May 26, 2025

Choose a reason for hiding this comment

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!

Copilot AI May 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AdamVe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented May 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

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