Skip to content

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

Merged
merged 3 commits into from
Jun 11, 2025
Merged

ML-DSA+COSE #115158

merged 3 commits into from
Jun 11, 2025

Conversation

krwq
Copy link
Member

@krwq krwq commented Apr 29, 2025

TODO:

  • API Review with CoseKey:
- consider `DigitalSignatureAlgorithm : AsymmetricAlgorithm` or `CoseKey : AsymmetricAlgorithm`

```csharp
using System.Security.Cryptography.Cose;

public sealed partial class CoseKey
{
    internal CoseKey() { }
    public static CoseKey FromKey(ECDsa key, HashAlgorithmName hashAlgorithm);
    [Experimental("SYSLIB5006")]
    public static CoseKey FromKey(MLDsa key);
    public static CoseKey FromKey(RSA key, RSASignaturePadding signaturePadding, HashAlgorithmName hashAlgorithm);
}

public abstract partial class CoseMessage
{
    public bool VerifyDetached(CoseKey key, Stream detachedContent, ReadOnlySpan<byte> associatedData = default);
    public bool VerifyDetached(CoseKey key, ReadOnlySpan<byte> detachedContent, ReadOnlySpan<byte> associatedData = default);
    public Task<bool> VerifyDetachedAsync(CoseKey key, Stream detachedContent, ReadOnlyMemory<byte> associatedData = default, CancellationToken cancellationToken = default);
    public bool VerifyEmbedded(CoseKey key, ReadOnlySpan<byte> associatedData = default);
}

public sealed partial class CoseSignature
{
    public bool VerifyDetached(CoseKey key, Stream detachedContent, ReadOnlySpan<byte> associatedData = default);
    public bool VerifyDetached(CoseKey key, ReadOnlySpan<byte> detachedContent, ReadOnlySpan<byte> associatedData = default);
    public Task<bool> VerifyDetachedAsync(CoseKey key, Stream detachedContent, ReadOnlyMemory<byte> associatedData = default, CancellationToken cancellationToken = default);
    public bool VerifyEmbedded(CoseKey key, ReadOnlySpan<byte> associatedData = default);
}

public sealed partial class CoseSigner
{
    public CoseSigner(CoseKey key, CoseHeaderMap? protectedHeaders = null, CoseHeaderMap? unprotectedHeaders = null) { }
    public CoseKey CoseKey { get; }
}

@ghost

This comment was marked as off-topic.

1 similar comment
@ghost

This comment was marked as off-topic.

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.

@krwq krwq force-pushed the mldsa-cose-2 branch 2 times, most recently from 1597d69 to e7f7373 Compare June 9, 2025 10:17
@krwq krwq requested review from bartonjs and vcsjones June 9, 2025 10:41
Copy link
Member

@bartonjs bartonjs left a 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.

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; }
Copy link
Member

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.

Copy link
Member Author

@krwq krwq Jun 10, 2025

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

Comment on lines +49 to +55
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
Copy link
Member

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)
Copy link
Member

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.

Copy link
Member Author

@krwq krwq Jun 10, 2025

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
Copy link
Member

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?

Copy link
Member Author

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
Copy link
Member

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.*?

Copy link
Member

@bartonjs bartonjs left a 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.

@krwq krwq merged commit e0ab13d into dotnet:main Jun 11, 2025
86 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants