Update ML-DSA COSE public API #117011
Update ML-DSA COSE public API #117011
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones |
There was a problem hiding this comment.
Pull Request Overview
Updates the COSE public API by replacing static factory methods with constructors and adding new overloads for VerifyDetached to support CoseKey parameters.
- Replaced all
CoseKey.FromKey(...)calls withnew CoseKey(...)constructors. - Added
VerifyDetached(CoseKey, byte[], byte[]?)overloads (and docs) inCoseSignatureandCoseSign1Message. - Updated tests to use the new constructors and added null‐argument tests for detached content.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Security.Cryptography.Cose/tests/CoseTestHelpers.cs | Switched from FromKey factory calls to new CoseKey(...). |
| src/libraries/System.Security.Cryptography.Cose/tests/CoseSignerTests.cs | Updated tests to use the CoseKey constructor. |
| src/libraries/System.Security.Cryptography.Cose/tests/CoseKeyTests.SignVerify.cs | Updated sign/verify tests to use the new constructor. |
| src/libraries/System.Security.Cryptography.Cose/tests/CoseKeyTests.ArgumentValidation.cs | Switched to constructors and added null-detached‐content tests. |
| src/libraries/System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseSigner.cs | Updated internal instantiation to use the CoseKey constructor. |
| src/libraries/System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseSignature.cs | Added VerifyDetached(CoseKey, byte[], byte[]?) overload with XML docs. |
| src/libraries/System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseSign1Message.cs | Added VerifyDetached(CoseKey, byte[], byte[]?) overload with XML docs and updated <seealso>. |
| src/libraries/System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseKey.cs | Removed static FromKey methods, added public constructors, updated untrusted‐key logic. |
| src/libraries/System.Security.Cryptography.Cose/ref/System.Security.Cryptography.Cose.cs | Updated reference API: removed FromKey, added constructors and new VerifyDetached members. |
Comments suppressed due to low confidence (2)
src/libraries/System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseSignature.cs:247
- The XML doc for the parameter is incorrect: this overload takes a public verification key wrapped in a CoseKey. Update the description to reflect that it’s the public key used to verify the signature.
/// <param name="key">The private key used to sign the content.</param>
src/libraries/System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseSignature.cs:276
- Add an explicit null check for the optional parameter (e.g.
ArgumentNullException.ThrowIfNull(associatedData, nameof(associatedData));) before creating a ReadOnlySpan to ensure the correct parameter name is reported on null.
ArgumentNullException.ThrowIfNull(detachedContent);
src/libraries/System.Security.Cryptography.Cose/tests/CoseKeyTests.ArgumentValidation.cs
Outdated
Show resolved
Hide resolved
…ests.ArgumentValidation.cs Co-authored-by: Copilot <[email protected]>
| /// <param name="associatedData">The extra data associated with the signature, which must match the value provided during signing.</param> | ||
| /// <returns><see langword="true"/> if the signature is valid; otherwise, <see langword="false"/>.</returns> | ||
| /// <exception cref="ArgumentNullException"><paramref name="key"/> or <paramref name="detachedContent"/> is <see langword="null"/>.</exception> | ||
| /// <exception cref="ArgumentException"><paramref name="key"/> is of an unsupported type.</exception> |
There was a problem hiding this comment.
Is there a path that makes an ArgumentException for an unsupported kind of CoseKey? I feel like that's just leftover from the AsymmetricAlgorithm overloads.
Since the other overloads already have it, go ahead and leave this here; but if the question inspires followup... cool 😄
| /// <summary> | ||
| /// Verifies that the signature is valid for the message's content using the specified key. | ||
| /// </summary> | ||
| /// <param name="key">The private key used to sign the content.</param> |
There was a problem hiding this comment.
Looks like all of these Verify overloads demand the private key. Since they're all wrong, again, just leave this matching... but we should fix these at some point.
| @@ -137,6 +136,7 @@ internal CoseSign1Message() { } | |||
| public bool VerifyDetached(System.Security.Cryptography.AsymmetricAlgorithm key, byte[] detachedContent, byte[]? associatedData = null) { throw null; } | |||
| public bool VerifyDetached(System.Security.Cryptography.AsymmetricAlgorithm key, System.IO.Stream detachedContent, System.ReadOnlySpan<byte> associatedData = default(System.ReadOnlySpan<byte>)) { throw null; } | |||
| public bool VerifyDetached(System.Security.Cryptography.AsymmetricAlgorithm key, System.ReadOnlySpan<byte> detachedContent, System.ReadOnlySpan<byte> associatedData = default(System.ReadOnlySpan<byte>)) { throw null; } | |||
| public bool VerifyDetached(System.Security.Cryptography.Cose.CoseKey key, byte[] detachedContent, byte[]? associatedData = null) { throw null; } | |||
There was a problem hiding this comment.
Are these new overloads being given the same treatment as the AsymmetricAlgorithm-based ones?
I don't know if the AsymmetricAlgorithm ones just test the array ones and let that fall back to the span ones by ccov/inspection, or if they are testing them independently. Since I don't see these new methods being wired into tests (except the ArgumentNullException ones) it means this set is either testing span-only, or array-only (which, yes, hits the span ones by inspection/ccov).
There was a problem hiding this comment.
I was considering adding separate array and span tests but the AsymmetricAlgorithm overloads don't have them split. So I just went with code coverage which shows that the new entry paths are fully covered.
Addresses API review feedback from #116942