Add support for ML-DSA to bedrock KMS module core#3
Conversation
| "@digitalbazaar/bls12-381-multikey": "^2.1.0", | ||
| "@digitalbazaar/ecdsa-multikey": "^1.8.0", | ||
| "@digitalbazaar/ed25519-multikey": "^1.3.1", | ||
| "@digitalbazaar/mldsa-multikey": "digitalbazaar/mldsa-multikey#main", |
There was a problem hiding this comment.
We need to make a release of mldsa-multikey as 1.0.0 and use that for the version here.
There was a problem hiding this comment.
Before we release 1.0.0 -- take a look at the inconsistency mentioned above. We should probably align to avoid dev frustration (and figure out which way to go).
| "@digitalbazaar/bls12-381-multikey": "^2.1.0", | ||
| "@digitalbazaar/ecdsa-multikey": "^1.8.0", | ||
| "@digitalbazaar/ed25519-multikey": "^1.3.1", | ||
| "@digitalbazaar/mldsa-multikey": "digitalbazaar/mldsa-multikey#main", |
There was a problem hiding this comment.
We need to make a release of mldsa-multikey as 1.0.0 and use that for the version here.
There was a problem hiding this comment.
It's been released.
| "@digitalbazaar/mldsa-multikey": "digitalbazaar/mldsa-multikey#main", | |
| "@digitalbazaar/mldsa-multikey": "^1.0.0", |
msporny
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3 +/- ##
==========================================
+ Coverage 95.70% 95.75% +0.05%
==========================================
Files 10 10
Lines 1000 1014 +14
==========================================
+ Hits 957 971 +14
Misses 43 43
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| } else if(type === 'urn:webkms:multikey:ML-DSA-44' || | ||
| type === 'urn:webkms:multikey:ML-DSA-65' || | ||
| type === 'urn:webkms:multikey:ML-DSA-87') { | ||
| keyPair = await MldsaMultikey.from({key}); |
There was a problem hiding this comment.
I see this call signature is different from the others -- just making a note of it in case we want to have a target to align (one way or another).
There was a problem hiding this comment.
You had noted that you wanted to switch over to named parameters in that library. I made the change due to the request. I think it still makes sense to use named parameters, but don't know if there was something else driving NOT using named parameters in that library for this particular interface?
It would be easier for me to change the mldsa-multikey library to align w/ the others, though I think the named parameter approach is better for external interfaces (in case we want to add other options in the future).
I'm fine either way, your call... maybe @davidlehn @gannan08 or @tminard have strong opinions here.
There was a problem hiding this comment.
Let's move toward named parameters. I'll file issues on the other multikey libs to support those.
| "@digitalbazaar/bls12-381-multikey": "^2.1.0", | ||
| "@digitalbazaar/ecdsa-multikey": "^1.8.0", | ||
| "@digitalbazaar/ed25519-multikey": "^1.3.1", | ||
| "@digitalbazaar/mldsa-multikey": "digitalbazaar/mldsa-multikey#main", |
There was a problem hiding this comment.
Before we release 1.0.0 -- take a look at the inconsistency mentioned above. We should probably align to avoid dev frustration (and figure out which way to go).
|
@msporny, also: needs a changelog entry. |
Co-authored-by: Dave Longley <dlongley@digitalbazaar.com>
dlongley
left a comment
dlongley
left a comment
There was a problem hiding this comment.
Thanks! I think this is ready to go once the mldsa-multikey 1.0 release is made and used here.
Co-authored-by: Dave Longley <dlongley@digitalbazaar.com>
4 participants
This PR adds support for ML-DSA (44, 65, and 87).
This PR shouldn't be merged until we make a release for mldsa-multikey and use that version in the package.json.