Skip to content

Conversation

bartonjs
Copy link
Member

  • Adjusted all public API on SlhDsa and SlhDsaAlgorithm to change ...SecretKey... to ...PrivateKey...
  • Adjusted API docs, code comments, and locals to reflect the rename
  • Adjusted test names, helpers, locals, etc. to align with the rename
    • Except for SecretKeySeed and SecretKeyPrf for the key generation tests
  • Did not get overzealous and do it for ML-DSA (that's a separate change)
  • Did not apply renames to any Interop types or the crypto shim

Fixes #117958

@bartonjs bartonjs added this to the 10.0.0 milestone Jul 30, 2025
@bartonjs bartonjs self-assigned this Jul 30, 2025
@Copilot Copilot AI review requested due to automatic review settings July 30, 2025 20:03
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 renames "SecretKey" to "PrivateKey" throughout the SLH-DSA cryptographic API to align with standard cryptographic terminology. The change affects public APIs, documentation, comments, and test code while maintaining consistency with established naming conventions where private keys are distinguished from public keys.

Key changes include:

  • Renaming all public API methods from ...SecretKey... to ...PrivateKey... patterns
  • Updating the SlhDsaAlgorithm.SecretKeySizeInBytes property to PrivateKeySizeInBytes
  • Adjusting test code, documentation, and variable names to reflect the new terminology

Reviewed Changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/libraries/System.Security.Cryptography/ref/System.Security.Cryptography.cs Updates public API surface to rename SecretKey methods/properties to PrivateKey equivalents
src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/SlhDsaOpenSsl.OpenSsl.cs Renames ExportSlhDsaSecretKeyCore to ExportSlhDsaPrivateKeyCore in OpenSSL implementation
src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/SlhDsaOpenSsl.NotSupported.cs Updates method signature in platform not supported implementation
src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/SlhDsaImplementation.OpenSsl.cs Renames ImportSecretKey to ImportPrivateKey and updates related logic
src/libraries/Common/src/System/Security/Cryptography/SlhDsa.cs Updates core SlhDsa class with new PrivateKey method names and property references
src/libraries/Common/src/System/Security/Cryptography/SlhDsaAlgorithm.cs Renames SecretKeySizeInBytes property to PrivateKeySizeInBytes
src/libraries/Common/src/System/Security/Cryptography/SlhDsaImplementation.cs Updates ImportSecretKey method name and related implementation
src/libraries/Common/src/System/Security/Cryptography/SlhDsaImplementation.Windows.cs Updates method signatures in Windows-specific implementation
src/libraries/Common/src/System/Security/Cryptography/SlhDsaImplementation.NotSupported.cs Updates method signatures in not-supported implementation
src/libraries/Common/src/System/Security/Cryptography/SlhDsaCng.cs Updates CNG implementation method signature
Multiple test files Updates test methods, helpers, and data structures to use PrivateKey terminology

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.

Copy link
Member

@vcsjones vcsjones left a comment

Choose a reason for hiding this comment

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

This is at least one resource string that needs to be changed (Argument_SecretKeyWrongSizeForAlgorithm) in both S.S.C. and M.B.C. You’ll have to split it from ML-DSA (or do ML-DSA in this PR)

@bartonjs
Copy link
Member Author

Argument_SecretKeyWrongSizeForAlgorithm

I did switch over SLH-DSA, and tried to delete the string. The only remaining hits for it are in ML-DSA (interestinly, MLDsaCng... the base class used a different string)

Copy link
Member

@vcsjones vcsjones left a comment

Choose a reason for hiding this comment

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

Seems correct to me, caveated on the fact that it’s harder to review for lines that should have changed but didn’t. But the public API surface looks right which is the important part.

@vcsjones
Copy link
Member

We may want to do a breaking change doc for this one (and the ML-DSA one, if that impacts your decision on breaking ML-DSA and SLH-DSA in to two different PRs)

@bartonjs
Copy link
Member Author

We may want to do a breaking change doc for this one

I feel like the boundary condition we set for these new+Experimental types was that we didn't need docs unless we changed something after RC1. Unless I took a month-long nap, I believe that means this is still doc-free.

@vcsjones
Copy link
Member

vcsjones commented Jul 30, 2025

When we did the Key to GetKey change on the CNG we waffled on it and decided "no" because it was a small change, but it was still on the table regardless of Experimental. This feels a little more substantial.

But as long as we made a conscious decision, sure.

@bartonjs
Copy link
Member Author

I'll be doing the ML-DSA changes next... and by then the guilt will probably sink in and I'll make the doc for both algorithms. Particularly since I just got bit by not checking that the Pkcs library built.

@bartonjs bartonjs merged commit 2611ca5 into dotnet:main Jul 31, 2025
87 checks passed
@bartonjs bartonjs deleted the slhdsa_name_alignment branch July 31, 2025 16:47
@github-actions github-actions bot locked and limited conversation to collaborators Aug 31, 2025
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.

[API Proposal]: SLH-DSA: Rename Secret Key to Private Key
3 participants