Skip to content

Conversation

vcsjones
Copy link
Member

@vcsjones vcsjones commented Jul 4, 2025

Noodling more with Copilot agents, I asked it to highlight any differences between the PQC types, functionality and purpose aside, like style. A couple of things seemed like they should be fixed like docs, so here we are.

@vcsjones vcsjones added this to the 10.0.0 milestone Jul 4, 2025
@vcsjones vcsjones self-assigned this Jul 4, 2025
@Copilot Copilot AI review requested due to automatic review settings July 4, 2025 17:24
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
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

Updates XML documentation comments for Post-Quantum Cryptography (PQC) key types to improve consistency, add FIPS references, and refine property and constructor docs across SlhDsa, ML-KEM, and ML-DSA classes.

  • Add FIPS-205 <para> references and restructure remarks in SlhDsa; removed lone doc for ThrowIfDisposed.
  • Refine XML <summary>/<value> tags for the ML-KEM Algorithm property.
  • Enhance MLDsa docs with FIPS-204 <para>, <value>, and <exception> tags; adjust the Sign summary.

Reviewed Changes

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

File Description
src/libraries/Common/src/System/Security/Cryptography/SlhDsa.cs Update <remarks> to include FIPS-205 refs and <para> tags; removed summary for ThrowIfDisposed.
src/libraries/Common/src/System/Security/Cryptography/MLKem.cs Clarify <summary> and <value> for the Algorithm property to emphasize the specific ML-KEM algorithm.
src/libraries/Common/src/System/Security/Cryptography/MLDsa.cs Add FIPS-204 <para> in remarks, introduce <value> for Algorithm, and document null-argument exception.
Comments suppressed due to low confidence (3)

src/libraries/Common/src/System/Security/Cryptography/SlhDsa.cs:70

  • Add an XML <summary> for the ThrowIfDisposed method to describe its behavior and maintain consistency with other cryptography helper methods.
        private protected void ThrowIfDisposed() => ObjectDisposedException.ThrowIf(_disposed, typeof(SlhDsa));

src/libraries/Common/src/System/Security/Cryptography/MLKem.cs:43

  • [nitpick] The <summary> and <value> descriptions are nearly identical; consider refining the summary to focus on property semantics and leaving implementation details to the <value> tag to avoid redundancy.
        ///   Gets the specific ML-KEM algorithm for this key.

src/libraries/Common/src/System/Security/Cryptography/MLDsa.cs:59

  • Add a unit test to verify that passing null to the MLDsa(MLDsaAlgorithm) constructor throws ArgumentNullException as documented.
        /// <exception cref="ArgumentNullException">

@MichalPetryka
Copy link
Contributor

Would it make sense to add a Remarks section to MLKem Decapsulate saying that it outputs garbage instead of failing for mismatched keys while you're changing the docs?

@vcsjones
Copy link
Member Author

vcsjones commented Jul 5, 2025

@MichalPetryka something like

/// <remarks>
///   Decapsulation can only decapsulate a shared secret created with the the decapsulation key's
///   corresponding encapsulation key. If the incorrect key is used, ML-KEM performs implcitly rejection.
///   Implicit rejection means an error will not be returned, but instead the shared secret will be incorrect.
/// </remarks>

?

I think documenting it is reasonable, what do you think @bartonjs?

@bartonjs
Copy link
Member

bartonjs commented Jul 8, 2025

I'm not sure that I'm convinced that the remark on Decapsulate is necessary; but if it's desired then what you propose is fine, @vcsjones.

Trying to de-nerd it slightly, but not sure I succeeded:

/// <remarks>
///   The Decapsulate operation does not throw for input values that
///   were not produced by the corresponding Encapsulate
///   operation.  Instead, it will produce a deterministic response
///   that does not correspond to anything.  Errors, therefore, will
///   have to be detected at some other layer.
///   For more information, see FIPS 203, Section 6.3.
/// </remarks>

@vcsjones
Copy link
Member Author

vcsjones commented Jul 8, 2025

I kind of mashed our two ideas together. I like your remark about deterministic and relevant FIPS documentation, I liked noting the specific term "implicit rejection" since that is searchable.

@vcsjones vcsjones enabled auto-merge (squash) July 9, 2025 15:55
@vcsjones vcsjones merged commit 1fb5178 into dotnet:main Jul 10, 2025
84 of 87 checks passed
@vcsjones vcsjones deleted the pqc-docs-fixes branch July 10, 2025 20:47
@github-actions github-actions bot locked and limited conversation to collaborators Aug 10, 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.

3 participants