Skip to content

Conversation

vcsjones
Copy link
Member

After some backchanneling we discussed that the ImportEncryptedPkcs8PrivateKey(string, ReadOnlySpan<byte>) didn't have the same shape as ImportFromEncryptedPem(string, byte[]).

We decided that since the string overloads for password exist purely for ease-of-use for other downlevel platforms, the ImportEncryptedPkcs8PrivateKey one should take a byte array instead of a ReadOnlySpan. There is already an overload that accepts ReadOnlySpan<char>, ReadOnlySpan<byte>, so modern targets still have access to all the span APIs that they want.

Contributes to #113508

@Copilot Copilot AI review requested due to automatic review settings April 24, 2025 21:02
@ghost
Copy link

ghost commented Apr 24, 2025

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Apr 24, 2025

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

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 changes the signature of ImportEncryptedPkcs8PrivateKey to accept a byte array for the source parameter when a string password is provided, aligning its shape with related overloads.

  • Updated the managed API in System.Security.Cryptography.cs to use byte[] for the source parameter.
  • Adjusted tests in MLKemTests.cs to use the new overload and added tests for null argument handling.
  • Modified the implementation in MLKem.cs to perform an argument null check on the source parameter.

Reviewed Changes

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

File Description
src/libraries/System.Security.Cryptography/ref/System.Security.Cryptography.cs Updated the method signature to accept a byte array for the source parameter.
src/libraries/Common/tests/System/Security/Cryptography/MLKemTests.cs Updated test calls to use the new overload and added null argument tests.
src/libraries/Common/src/System/Security/Cryptography/MLKem.cs Updated the string overload to use byte[] and added a null check for the source parameter.

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.


Assert.Throws<CryptographicException>(() =>
MLKem.ImportEncryptedPkcs8PrivateKey("PLACEHOLDER", new ReadOnlySpan<byte>(ecP256Key)));
MLKem.ImportEncryptedPkcs8PrivateKey("PLACEHOLDER", ecP256Key));
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't have to change that many tests because most of the tests used a string for the password parameter forces binding to a specific overload. I just changed this one since the span-wrapping served no purpose anymore.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants