Documentation consistency for PQC types#117323
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones |
There was a problem hiding this comment.
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 forThrowIfDisposed. - Refine XML
<summary>/<value>tags for the ML-KEMAlgorithmproperty. - Enhance MLDsa docs with FIPS-204
<para>,<value>, and<exception>tags; adjust theSignsummary.
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 theThrowIfDisposedmethod 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 throwsArgumentNullExceptionas documented.
/// <exception cref="ArgumentNullException">
|
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? |
|
@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? |
|
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: |
|
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. |
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.