Feat/key did secp256r1 now includes curves secp384r1 and secp521r1 as well #1884
Conversation
* commit a temporary file called backup * rebase to april 9th, and bring in desireable changes to may19th * remove tests for null and related functions because they are not needed * Add "bigint-mod-arith": "^3.0.0" to key-did-resolver package.json * add a try and finally blocks for pubKeyBytesToXY add tests to show errors for functions that are dependencies * removed try and finally blocks from secp256r1.ts * refactored code to get linter to pass * simplify TypeError conditional in pubKeyBytesToXY since null loosely equals undefined * add additional tests for malformed did:keys , add comments to secp256r1.ts code * move test for functions in the test file to the end. Clean up and add additional tests to null. Assume this covers undefined. * add tests for undefined case for functions declared for use in test file. Fix functions to allow undefined cases to pass. * modify js-ceramic tsconfig.json to include target of ES2020 to allow for support of BigInt in package key-did-resolver * remove old target for ES2018 js-ceramic in main tsconfig.json, and replace with ES2020 (removing duplicates) * remove target: ES2020 from key-did-resolver package, since it is redundant with parent js-ceramic tsconfig.json * first run at adding comments to src/secp256r1.ts . Incomplete. * added documentation annotations for src/secp256r1.ts. May rewrite after forcing functions to throw custom errors for null and undefined * updated code and tests * updated the tests and code for more specific error responses. This is a work in progress. * update tests and code for publicKeyIntToUint8ArrayPointPair function * updated tests to pass * updated publicKeyIntToUint8ArrayPointPair and publicKeyIntToXY parameter description since redundant with error message * add function with tests to test to see if input is a Uint8Array : testUint8Array * update testUint8Array with typing info to pass linter test * added function to test for hex string * update publicKeyHexToUint8ArrayPointPair with better error checking * updated publicKeyToXY to handle only hex strings and throw and error otherwise * update pubKeyBytesToHex to only allow for Uint8Arrays and throw and error if otherwise * Add error checking for ECPointDecompress so it only allows Uint8Arrays * Add error checking for publicKeyBytesToXY so it only allows Uint8Arrays * Refactor publicKeyIntToXY and publicKeyIntToUint8ArrayPointPair in sec/secp256r1.ts * refactored logic in secp256r1.ts NOT(A AND B) = NOT(A) OR NOT(B)
… in key-did-resolver/src/index.ts
There was a problem hiding this comment.
Thanks for your work on this! We are currently in the process on migrating to ES2020 and ESM, once that's done I will give this a much more thorough review and we can merge this.
Are we currently using the test vectors from https://github.com/w3c-ccg/did-method-key/tree/main/test-vectors ?
When matching against https://github.com/w3c-ccg/did-method-key/tree/main/test-vectors I have. P-384: P-521: I believe these may be already in the tests. This was a good exercise because it did turn up something. Although the x,y coordinates in base64url resolve like the test vectors, I notice my json-ld response is in a slightly different form. I first notice that I have no @base property. I was scared the other day. I went to https://did.key.transmute.industries/generate/secp521r1 and noticed a difference when I threw in the did:keys generated into my code and tried to resolve to the base64url points, but as far as the test vectors concerned the keys and the resolved points have been consistent. Also, what I put in from the library elliptic to create a did:key is what I get out. I think this is something I have to talk to Orie about. The w3c ccg test vectors have been accepted by the community. I have not delved into the specifics of his code. I was just playing around on his site after I found it looking for did:keys. |
|
Adding a @base part of the json-ld would require a change to all key:did curves. secp256k1, ed25519, P-256, P-384, P-521. |
|
This is somewhat related. Is https://github.com/ceramicnetwork/key-did-provider-ed25519 like the key-did-provider library that you want to have? https://github.com/ceramicnetwork/key-did-provider-ed25519 . I noticed that this is the only one that is available on npm. https://github.com/ceramicnetwork/key-did-provider-secp256k1 exists on github, but not npm. I think https://github.com/bshambaugh/did-key-creator is okay, but it does not have JWE or JWK, and it does not expose a constructor, just a few functions that allow one to create did:keys. This is related to: #1884 (comment) |
|
Here is my 'back of the envelope' calculation to create did:keys from the npm elliptic library and then resolve: This code produces did:keys that resolve to what the committed code produces. The code shows that the BigINT x,y values are the same before and after compression as well as before did:key creation (straight from the elliptic library output). I know of no way to prove that my P-521 commit is incorrect. TODO: revisit Transmute's did:key. |
The way to do that is to show that you handle the test vectors here: https://w3c-ccg.github.io/did-method-key/#p-521 If the test vectors are wrong, you can open a PR to change them... The spec is the source of truth, not individual implementations... we should fix the spec first, if there is a problem... then open issues on any implementations that are not spec conformant. |
I am matching the x,y base64url values for each did:key. I have some questions about the spec, so I will ask there {https://github.com/w3c-ccg/did-method-key/}. |
|
@bshambaugh We just merged the Ceramic 2.0 branch into develop (with ESM support 🎉 ). Can you please merge develop into this branch and solve any conflicts? After that I will take anther look at this PR and hopefully merge it! |
|
I might have done things incorrectly by changing the base to develop and then resolving conflicts. I was trying to bring in Stephanie's changes. Tests are being run now by CircleCI. I did modifications through the browser so I hope that everything works smoothly. I might try to figure out how to get this code running locally with these changes to double check. I'm trying to track this TypeError error message inconsistency down: |
|
My index.test.ts file was much larger too. |
…ToDidDoc' of undefined' to 'TypeError: Cannot read property 'keyToDidDoc' of undefined'
…amic into feat/key-did-secp256r1 There were changes that Stephanie Hunh made to key-did-resolver that needed to be made to respect merge conflicts
…mat, import directly
|
I'm investigating the media type for the key-did-resolver tests (e.g: https://github.com/bshambaugh/js-ceramic/blob/feat/key-did-secp256r1/packages/key-did-resolver/src/__tests__/__snapshots__/index.test.ts.snap#L374-L410) |
To the best of my knowledge, what I have seems okay. I talked to Orie Steele. |
|
Ya'll should probably try to send someone to the VC WG calls. We discussed this issue today: There is debate of the future of |
|
"Final" 2AM thought: I added some support for P-384 and P-521 to the resolver to help out. There also was not consensus on whether to use compressed keys yet, so support for uncompressed and raw keys (without 04 prefix) for lack of better word was added in addition to compressed. Discussions here drove https://github.com/multiformats/multicodec/blob/master/table.csv to be updated to be compressed. As everything was in flux, this arose: I kept the legacy support for uncompressed and compressed keys, but made a note in the readme: I know initially you (oed) did not like the idea of uncompressed and raw keys, but I explained the context and you were okay with it even though I think we are both in agreement that they could cause confusion. The short reason is: On the CCG list there appears to be some discussion about independent review of NIST curves used for this application (maybe Ver Creds?) by SRI (if I recall). There also is discussion about P-384 for support of large organizations using hardware security modules. I know Ceramic's focus has not been on the NIST curves (for the most part). The whole PR has been driven by Blinky. I'm now looking at did-jwt to build a key-did-provider following EIP2844 to finish stuff up (and calling this okay for now). I see: decentralized-identity/did-jwt#170 with mention to reorganize the code. I am not sure how this will be followed. A package structure like js-ceramic? We might need to catch up in a talk. Meanwhile, I'm doing what I can. decentralized-identity/did-jwt#212 is where this is going on. All of the updates are coming in from my fork. I'm driving through the did-jwt code trying to make the best sense I can. Basically, I'm setting this up to add a ES256 signer so I can work with an essentially remote HSM (https://github.com/decentralized-identity/did-jwt/blob/master/docs/guides/index.md#creating-custom-signers-for-integrating-with-hsm). Due to splitting stuff up into different files, I imagine also that ES384/p384, ES512/p521 could be easily added (the hope) as well as RSA and BBS+? by interested parties. I wasn't there when stuff was spun out of uPort (I presume)..so I'm just running through guessing trying to get something operational. |
…section in key-did-resolver readme, update jest to latest version to run test
|
I'll forget. The VCWG is moving toward secp384r1 support for legacy HSM. https://lists.w3.org/Archives/Public/public-credentials/2022Jan/0186.html . Also, I recall that large cloud providers like Azure support all three NiST curves. I propose helping out with the WG if desired. @OR13 . I'm not with 3box/ceramic, so it's up to them. It might be a way to keep this code up to date. |
| import * as u8a from 'uint8arrays' | ||
| import * as bigintModArith from 'bigint-mod-arith' | ||
|
|
||
| import * as nist_weierstrass_common from './nist_weierstrass_common' |
There was a problem hiding this comment.
| import * as nist_weierstrass_common from './nist_weierstrass_common' | |
| import * as nistWeierstrassCommon from './nist_weierstrass_common.js' |
| import * as u8a from 'uint8arrays' | ||
| import * as bigintModArith from 'bigint-mod-arith' | ||
|
|
||
| import * as nist_weierstrass_common from './nist_weierstrass_common' |
There was a problem hiding this comment.
| import * as nist_weierstrass_common from './nist_weierstrass_common' | |
| import * as nistWeierstrassCommon from './nist_weierstrass_common.js' |
…6r1,secp384r1,and secp521r1).ts'
|
Item 3 at #1441 (comment) . Is still a work in progress. I made a note: "Would someone be willing to check this out for me? At present I believe I am implicitly importing as an ESM [1] by using the "type":"module" in my package.json . However, my reviewer still reports problems on their machine. #1441 (comment) . An alternative would be to use .mjs extensions instead of "type":"module" in my package.json .The PR #1884 is the new pull request, and my commits are coming from my fork at https://github.com/bshambaugh/js-ceramic/tree/feat/key-did-secp256r1 . I'm not sure how to sandbox this. It works fine on my machine, but I could change to .mjs just 'cause.[1] https://dev.to/bnb/implicit-esm-in-node-js-with-type-module-np " Useful Refs: https://github.com/juanelas/bigint-mod-arith import * as bigintModArith from 'bigint-mod-arith' The appropriate version for browser or node is automatically exported. |
|
Hey, @bshambaugh . Made an effort to rebase your branch onto the latest develop, and played with it. Apparently, all the tests are ok (even runs in ESM mode), despite what CircleCI says. I am convinced, this could be merged. |
|
Hey thanks @ukstv. Could the CircleCI issue be for another package? I saw the raw circleCI output for this PR, which shows the tests for the code in the package passing, but something outside garbled with it such as with stream-ids as not passing. This presents itself as an unclear delineation in the CircleCI tests. Compare when I run the key-did-resolver tests on my own machine on their own (see picture below) to a link to the raw CircleCI output for this PR: https://circleci.com/api/v1.1/project/github/ceramicnetwork/js-ceramic/5462/output/111/0?file=true&allocation-id=6203ea99d400852d4e16b211-0-build%2F5EAE6C79 |
Feat/key did secp256r1 now includes curves secp384r1 and secp521r1 as well
Description
Code has been extended to include Secp384r1 and Secp521r1 and refactored to put common functions into a separate file nist_weierstrass_common .
I will double check this code in the next day or so. Right now, a nap.