-
Notifications
You must be signed in to change notification settings - Fork 5k
ML-DSA+COSE #115158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ML-DSA+COSE #115158
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
1 similar comment
This comment was marked as off-topic.
This comment was marked as off-topic.
Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones |
...ies/System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseSignature.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/tests/SignVerify.AlgorithmSupport.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/tests/SignVerify.AlgorithmSupport.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/tests/SignVerify.AlgorithmSupport.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/tests/SignVerify.AlgorithmSupport.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/tests/SignVerify.AlgorithmSupport.cs
Outdated
Show resolved
Hide resolved
...y.Cryptography.Cose/src/System/Security/Cryptography/Cose/MLDsaAsymmetricAlgorithmWrapper.cs
Outdated
Show resolved
Hide resolved
...System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/ToBeSignedBuilder.cs
Show resolved
Hide resolved
src/libraries/Common/tests/System/Security/Cryptography/PlatformSupport.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/src/System.Security.Cryptography.Cose.csproj
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/src/System.Security.Cryptography.Cose.csproj
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/src/System.Security.Cryptography.Cose.csproj
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/tests/CoseTestKeyManager.cs
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/tests/CoseTestKeyManager.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/tests/CoseTestKeyManager.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/tests/CoseTestMultiSign.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/tests/CoseTestKey.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/tests/System/Security/Cryptography/PlatformSupport.cs
Outdated
Show resolved
Hide resolved
...ecurity.Cryptography.Cose/src/System/Security/Cryptography/Cose/PureDataToBeSignedBuilder.cs
Outdated
Show resolved
Hide resolved
1597d69
to
e7f7373
Compare
src/libraries/Common/tests/System/Security/Cryptography/PlatformSupport.cs
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/src/System.Security.Cryptography.Cose.csproj
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/src/System.Security.Cryptography.Cose.csproj
Outdated
Show resolved
Hide resolved
...ecurity.Cryptography.Cose/src/System/Security/Cryptography/Cose/PureDataToBeSignedBuilder.cs
Show resolved
Hide resolved
...raries/System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseSigner.cs
Outdated
Show resolved
Hide resolved
...libraries/System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseKey.cs
Show resolved
Hide resolved
...libraries/System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseKey.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Posting what I have so far, will continue reviewing tests soon.
...tests/System/Security/Cryptography/AlgorithmImplementations/MLDsa/MLDsaTestImplementation.cs
Outdated
Show resolved
Hide resolved
public sealed partial class CoseKey | ||
{ | ||
internal CoseKey() { } | ||
public static System.Security.Cryptography.Cose.CoseKey FromKey(System.Security.Cryptography.ECDsa key, System.Security.Cryptography.HashAlgorithmName hashAlgorithm) { throw null; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I think it feels odd to capture the hashAlgorithm here (and the padding type for RSA).
But I guess the CoseKey export formats all have a single KTY value, and the KTY values for ECDSA include the hash algorithm, and for RSA it also includes the padding mode.
This reduces CoseSigner to (CoseKey, protectedHeaders, unprotectedHeaders). If it was entirely new API I'd say that makes CoseSigner kind of unnecessary... but since it's already there, I don't see a reason to start trying to eliminate it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed about hash, although padding is a bit arguable to me since it also could be treated as part of the algorithm. We can discuss this further during API review but as is seems to align with COSE spec
src/libraries/System.Security.Cryptography.Cose/ref/System.Security.Cryptography.Cose.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/src/System.Security.Cryptography.Cose.csproj
Outdated
Show resolved
Hide resolved
...libraries/System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseKey.cs
Show resolved
Hide resolved
...libraries/System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseKey.cs
Show resolved
Hide resolved
.../System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseSign1Message.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/tests/CoseKeyTests.SignVerify.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/tests/CoseKeyTests.SignVerify.cs
Outdated
Show resolved
Hide resolved
...aries/System.Security.Cryptography.Cose/tests/System.Security.Cryptography.Cose.Tests.csproj
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/tests/CoseKeyTests.SignVerify.cs
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/tests/CoseKeyTests.SignVerify.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/tests/CoseKeyTests.SignVerify.cs
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/tests/CoseKeyTests.SignVerify.cs
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/tests/CoseKeyTests.SignVerify.cs
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/tests/CoseTestKeyManager.cs
Outdated
Show resolved
Hide resolved
CoseSign1Message embeddedMessage = CoseSign1Message.DecodeSign1(embeddedMessageBytes); | ||
CoseSign1Message detachedMessage = CoseSign1Message.DecodeSign1(detatchedMessageBytes); | ||
|
||
AssertExtensions.Throws<ArgumentNullException>("key", () => embeddedMessage.VerifyEmbedded((CoseKey)null!, ReadOnlySpan<byte>.Empty)); | ||
AssertExtensions.Throws<ArgumentNullException>("key", () => detachedMessage.VerifyDetached((CoseKey)null!, payload, Array.Empty<byte>())); | ||
AssertExtensions.Throws<ArgumentNullException>("key", () => detachedMessage.VerifyDetached((CoseKey)null!, payloadStream)); | ||
AssertExtensions.Throws<ArgumentNullException>("key", () => detachedMessage.VerifyDetachedAsync((CoseKey)null!, payloadStream)); // null check synchronously |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I sort of get why these are here... but they feel more like they belong with the verify tests. After all, we don't have a single file of all tests that use string
in the first parameter, or RSA
, or whatever.
|
||
[Theory] | ||
[MemberData(nameof(AllKeysAndSign1Implementations))] | ||
public void TestSignVerifySingleSignerAllAlgorithms(string keyId, CoseTestSign1 signerImplementation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, this feels like it should be with the existing signing tests, not tests regarding CoseKey.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit torn here - they're all based on AsymmetricAlgorithm... I'll see if I can fairly quickly do this change or not but will send it separately if at all
|
||
namespace System.Security.Cryptography.Cose.Tests | ||
{ | ||
public sealed class CoseTestMultiSign |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are already multi-sign tests, in CoseMultiSignMessageTests.*
. Why do we want/need a new class with substantially the same name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me it's the same reason why we need CoseKey when we already have AsymmetricAlgorithm
|
||
namespace System.Security.Cryptography.Cose.Tests | ||
{ | ||
public sealed class CoseTestSign1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, why do we have a new class with similarly named responsibility as CoseSign1MessageTests.*
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer that the CoseTestMultiSign.cs contents were merged into CoseMultiSignMessageTests.Sign.cs (and similar for other new overload-based testing) before merge... but since I'd rather unblock the SignedCms work (which needs the Microsoft.Bcl.Cryptography work from this PR) I'm signing off with the hope that they get merged together soon.
TODO: