-
Notifications
You must be signed in to change notification settings - Fork 216
Add ML-DSA private keys to table. #399
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
Open
msporny
wants to merge
4
commits into
multiformats:master
Choose a base branch
from
msporny:patch-3
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+6
−0
Open
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
One thing worth noting: FIPS 204 and RFC 9881 define three distinct encodings for ML-DSA private keys:
32-byte seed (recommended, used by JOSE/COSE)
full expanded key
combined form with both
The -priv entries here don't specify which encoding they represent. Consistent with other private key codecs in this table, so not a blocker, but leaves room for implementation divergence.
@msporny did you have a specific encoding in mind? The W3C spec linked in this PR lists private key sizes of 2,560 / 4,032 / 4,896 bytes for ML-DSA-44/65/87, which matches the expanded key format, but doesn't say so explicitly. Worth making that explicit in your spec + why you use expanded and not the compact seed?
@vmx do you feel we should we should clarify here from the start?
If so, should we go with explanded or seed?
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.
Yes, agree with the changes. Thank you for the catch!
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.
Hi @vmx, that makes sense to me if the purpose of this if for holding private keys, in, say, an HSM, for use in signing. Otherwise we could just have a single code point for the 32 byte seed.
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.
Is the seed 32 bytes for all security levels? I'm wondering if we need to provide a hint to the software wrt. what the expected security strength level should be in the generated key. For example,
secretKeyMultibasedoesn't tell you the security strength level of they expanded key, and it might be a good idea to do that since the information doesn't exist anywhere else. IOW, we might need three more entries: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.
My understanding is that the seed size is the same for all variants here. I think we should go with the "seed key format (32 bytes)" second suggested change above from @lidel.
I think it's clear that having the seed version of these is useful so we should certainly define that. I don't know that expanded format will be useful in the future, but it may be. Minimally, the seed version should be defined (for all 3, so 3 different entries in the table) -- and if we think the expanded version is necessary that would be 3 more (total of 6).
So, +1 for defining mldsa-44|65|87-priv as the seed version. IIRC, the ed25519 and x25519 private keys are also defined as their 32-byte seed variants here (and then those are expanded internally for use -- yes, it's a much simpler expansion, but the concept is similar).
Uh oh!
There was an error while loading. Please reload this page.
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 wonder if it would be (also?) useful option to avoid the chore of registering every combination for every key and instead delegate as much is possible to existing RFCs.
For example, with codes proposed in:
we could represent both seed or expanded formats, or even both. https://www.rfc-editor.org/rfc/rfc9881#section-6 defines the ASN.1 encoding of ML-DSA private keys inside PKCS#8. For ML-DSA-44 its:
Fine to have both ways, but in practice, I find self-describing containers pretty interoperable (easy to find a library that knows how to read/write PKCS#8 etc)
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.
The implementation burden is much higher if ASN.1/DER (PKCS#8) is added as a requirement.
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.
Yes, agreed, -1 to re-using the optionality in RFC 9881. We're trying to keep the header values simple so there is no confusion (or extra parsing) needed past the Multikey header. It's either seed bytes or an expandedKey. The "both" option in ASN.1 is not something we want to introduce because you then run the possibility of someone reading in a "both" as an expanded key (which would be an implementation bug, but one that we'd like to avoid).
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 updated the PR to include the seeds (and their expected expanded form) as well:
https://github.com/multiformats/multicodec/pull/399/changes#diff-bf5b449ed8c1850371f42808a186b5c5089edd0025700505a6b8f426cd54a6e4R224-R226