Seed in the skeleton of ML-DSA based on current prototyping #112891
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones |
There was a problem hiding this comment.
Copilot reviewed 6 out of 11 changed files in this pull request and generated no comments.
Files not reviewed (5)
- src/libraries/System.Security.Cryptography/src/Resources/Strings.resx: Language not supported
- src/libraries/System.Security.Cryptography/src/System.Security.Cryptography.csproj: Language not supported
- src/libraries/Common/src/System/Security/Cryptography/Oids.cs: Evaluated as low risk
- src/libraries/Common/src/System/Experimentals.cs: Evaluated as low risk
- src/libraries/Common/src/System/Security/Cryptography/KeyFormatHelper.Encrypted.cs: Evaluated as low risk
src/libraries/Common/src/System/Security/Cryptography/KeyFormatHelper.Encrypted.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/System/Security/Cryptography/KeyFormatHelper.Encrypted.cs
Show resolved
Hide resolved
| { | ||
| fixed (byte* pointer = source) | ||
| { | ||
| using (PointerMemoryManager<byte> manager = new(pointer, source.Length)) |
There was a problem hiding this comment.
I wonder if we should just make a Span version of DecryptPkcs8. We at least push down the use of PointerMemoryManager there.
At the same time I also don't think it would be too hard to make one that just naturally works on Span?
Don't need to do anything in this PR, can be a follow up.
There was a problem hiding this comment.
Yeah, there's a fair amount of "can we move this down?" that I had while stubbing this out. Some of it, like doing a PKCS8 encryption envelope over bytes instead of AsnWriter, just looked too big for this particular seed-things change.
I'll probably be looking at changing how we do storage in the Asn types in the next couple of weeks, to see if we can get rid of the Memory requirement that I forced upon us all those years ago, (and we can get span-friendlier)
| /// The algorithm-specific import failed. | ||
| /// </para> | ||
| /// </exception> | ||
| public static bool TryImportSubjectPublicKeyInfo(ReadOnlySpan<byte> source, [NotNullWhen(true)] out MLDsa? imported) |
There was a problem hiding this comment.
Overall, I like that the import methods are static now, instead of creating an instance and importing a key in to it.
The one downside to it that I can see is here for the Try variants is that it makes it a little clunky to put a using around imported. I think its fine, and I suspect most people just won't use the Try.
| { | ||
| ThrowIfNotSupported(); | ||
|
|
||
| // TODO, probably in PemKeyHelper. |
There was a problem hiding this comment.
It's not clear to me why this needs to be done any differently that what is done for other algorithms?
Why do we need to "try all of them"?
There was a problem hiding this comment.
When I noticed (right before putting up the PR) that the IImportExportShape didn't have PEM, I started by leaving the Try off and just leaving a comment as to how it might work, considering that it has three failure modes: 1) no viable PEM was found, 2) encrypted private key was found, 3) viable PEM was found, but didn't indicate the algorithm.
If we want to say that static ImportFromPem works like RSA's instance ImportFromPem, and it's "there better only be one semi-viable one"... that seems sound. So then the Try returns false for "no viable input, or the only viable input wasn't ML-DSA", and throws for corrupt data or too many viable options. Which feels weird. So maybe we don't want the Try at all. Which then suggests maybe we don't want the other Try methods at all.
The Try for SPKI/P8 are mostly for an audience like SignTool. "Sign the exe using this key" (or verify). If it also has a cert (like Authenticode) then there's no guessing; but if it's something purely key-based (like COSE) then there's more guessing involved. Since it's niche, maybe the best answer is to just cut all the Try-Imports until there's a better demonstrated need.
The main reason I think people do cascading "try-catch RSA, try-catch ECDSA, ..." is because they're all AsymmetricAlgorithm; and once we take that crutch away they need to think more about how their data flows... so maybe it's not necessary.
| /// Represents a specific algorithm within the ML-DSA family. | ||
| /// </summary> | ||
| [Experimental(Experimentals.PostQuantumCryptographyDiagId)] | ||
| internal struct MLDsaAlgorithm : IEquatable<MLDsaAlgorithm> |
There was a problem hiding this comment.
I keep waffling if this should be a struct or a class. Struct makes sense, but makes dealing with Name kind of a pain because of default(MLDsaAlgorithm). This is always in the back of my mind when doing anything that touches HashAlgorithmName.
There was a problem hiding this comment.
I think the main reason we use structs for strongly typed strings is so you don't feel compelled to optimize caching them when your data comes externally.
We kind of don't want data coming externally here... (a 3rd party extension can't meaningfully say they implement some ML-DSA variant the base class doesn't, since it'll throw when failing to resolve the parameter set)... so maybe we want a numeric enum, or a class string-enum.
There was a problem hiding this comment.
My thought would be make it a sealed class.
I agree that it is kind of nonsense for external data here - ParameterSetInfo is not going to know what to do with anything other than what we already know about. If we have a class, then
defaultis null, and the documentation and runtime checks areThrowIfNull- We could have no public constructors, or, we can validate in the constructor names we know about, so that way everyone that consumes
MLDsaAlgorithmdoesn't have to check if the name is something we know about.
There was a problem hiding this comment.
Would you give the same treatment to the spiritual equivalent for all of ML-KEM, Composite ML-DSA, SLH-DSA, and Composite SLH-DSA?
Once it's locking down like this, I feel like we'd just merge the ParameterSetInfo data into it (so long as it's internal data it only matters to reflection where it lives)
There was a problem hiding this comment.
Would you give the same treatment to the spiritual equivalent for all of ML-KEM, Composite ML-DSA, SLH-DSA, and Composite SLH-DSA?
Yes.
Once it's locking down like this, I feel like we'd just merge the ParameterSetInfo data into it
That actually seems quite reasonable now. It can all be internal.
This seeds a lot of untested boilerplate code as non-public types in System.Security.Cryptography.
When we start work on ML-DSA, we can take this, make MLDsa and MLDsaAlgorithm public, start to write tests, and wire up an implementation. We can also take this and clone+customize it for the other PQC algorithms.
It does not define all span-vs-array overloads for all spanified members, just one candidate per method group.
As the PQC experiment needs to make it further before we can commit to the shape, it is both starting without API Review, and pre-emptively applying the Experimental attribute.