-
Notifications
You must be signed in to change notification settings - Fork 71
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
Developing a Signer for P-256. (work in progress) #212
Conversation
Excellent start! I agree that |
…p into files by curve
…les as suggested by linter
…ithms, add a not for secp256r1 support of in src/VerifierAlgorithm.common.ts
…ted for u8a, temporarily commiting for now
…_test.test.ts always has even keys, add tests for ES256 verifier and signer
…ierAlg/common_VerifierAlg.ts and __tests__/VerifierAlg/ES256VerifierAlg.test.ts
I strongly suggest you comment on w3c/vc-wg-charter#82. |
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 started to review this but it will take a few weeks, judging from the number of lines changed.
The test refactoring makes it extremely difficult to trust the code, since each value must be checked.
I appreciate all the work you put into this, but please discuss before dumping such big changes on a library used by thousands of projects.
src/SignerAlgorithm.ts
Outdated
} | ||
} | ||
} | ||
import * as ES256KSignerAlg from './SignerAlg/ES256KSignerAlg' |
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.
These imports can be rephrased as:
import * as ES256KSignerAlg from './SignerAlg/ES256KSignerAlg' | |
import { ES256KSignerAlg } from './SignerAlg/ES256KSignerAlg' |
and then the code below is simplified as well
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.
Understandable. It might have a bit to do with how my mind was working. I grasped things as, and after, I worked through them. I realize it was a huge refactor. It seemed necessary as otherwise the files were getting to get huge combining multiple signers and it would be hard to follow. I told Joel (@ oed) that I was trying to figure things out for my particular project. I need a remote signer over a radio connection. I see mention of constructing a remote signer. I went ahead and added ES256 and walked through the library so I'd have it if I need it when bringing in https://github.com/decentralized-identity/did-jwt-vc later. I'm also trying to build something like this (https://github.com/ceramicnetwork/key-did-provider-ed25519) but for a remote signer and with the P-256 curve (because I'm using a cryptographic co-processor that only provides this curve https://github.com/sparkfun/SparkFun_ATECCX08a_Arduino_Library). Yeah, I was kind of aggressive with my commit. I think I have a got to get this done mentality. I've been working on a particular (presently unfunded/hobby/or whatever project) for over a year, and get onto myself for not making enough progress to be successful with it. To my knowledge, adding NIST curves is not a big ceramic/3box initiative (they're focusing on scaling I think now). I ask questions from time to time, and I see this effort as the other half to js-ceramic/key-did-resolver. (https://github.com/ceramicnetwork/js-ceramic/tree/develop/packages/key-did-resolver/src) . I think this eventually got into the repo because I was relentless and they wanted to support active projects going for their ceramic mainnet initiative. I'll be patient. Maybe in time I'll discover that I did some work that did not need to be done. I realize other people have to deal with what I put out. Maybe I'm at a point where I can talk intelligently about it, if I did not realize before... apologies ...
fwiw, right now I'm looking at blockchain/cosmos.ts .... I'm trying to pull in a function that will default to secp256k1 , but will also allow for (P-256) secp256r1 because the publicKey needs to be compressed before construction in the rest of the function.
fwiw, the test files were broken up into smaller files, and parts of them were cloned (swapping the secp256r1 curve for the secp256k1 curve)
Again understandable... (+41,203 −9,451 ) is a huge change for a well used library. It kind of seemed like having fun on some trip, but then later getting the credit card bill. Since I am presently in busy beaver mode I'm sure I'll find some other things to do so no worries if you've got some other things to do as well (which I expect).
Edit: for the sake of transparency I reached out via e-mail to @kimdhamilton because I felt new to typescript and good code quality and this library and verifiable credentials specialization. We seem to get along well, but I'm not sure I should have done that privately. I think she uses this library in some of her other work, however I am not sure if she's ever reviewed or built on it before.
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.
no need to apologise :)
The bill of changes will be reduced once you remove package-lock.json
since this project uses yarn
instead, and further more after removing ES256-R
and related code.
It might not be worth trying to adapt the blockchain/cosmos bits since AFAIK there is no use of P-256 on the blockchains involved, only secp256k.
Still, there is very much to review, and it will take very long.
src/SignerAlgorithm.ts
Outdated
ES256K: ES256KSignerAlg(), | ||
ES256: ES256SignerAlg.ES256SignerAlg(), | ||
'ES256-R': ES256SignerAlg.ES256SignerAlg(true), | ||
ES256K: ES256KSignerAlg.ES256KSignerAlg(), |
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.
With the above suggestion, this would simplify to:
ES256K: ES256KSignerAlg.ES256KSignerAlg(), | |
ES256K: ES256KSignerAlg(), |
return signer | ||
} | ||
|
||
export function verifyRecoverableES256( |
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.
This is probably not needed since ES256-R is not needed
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.
agreed, ES256-R is not and probably will never be a thing.... ES256K-R is a thing already... even though it remains unregistered... I would discourage adding more "registered stuff" here.
src/VerifierAlgorithm.ts
Outdated
|
||
const secp256k1 = new EC('secp256k1') | ||
import * as Ed25519VerifierAlg from './VerifierAlg/Ed25519VerifierAlg' |
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.
flagging this for simplification of imports:
import * as Ed25519VerifierAlg from './VerifierAlg/Ed25519VerifierAlg' | |
import { Ed25519VerifierAlg } from './VerifierAlg/Ed25519VerifierAlg' |
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.
just to be clearer, all these imports should be simplified this way, not just this line
ethereumAddress?: string | ||
} | ||
|
||
interface VerificationMethodCommonSigner { |
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.
Why is this definition necessary since there already exists a VerificationMethod
definition?
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.
There were some other things in there like (https://github.com/bshambaugh/did-jwt/blob/master/src/__tests__/JWT/CommonSignerTest/CommonSignerTest.ts#L24-L75) that were added to avoid the use of any in the typescript file. This might need to be refactored. I guess I didn't realize how scary this was going to get.
export interface VerificationMethod {
id: string
type: string
controller: string
publicKeyBase58?: string
publicKeyBase64?: string
publicKeyJwk?: JsonWebKey
publicKeyHex?: string
publicKeyMultibase?: string
blockchainAccountId?: string
ethereumAddress?: string
}
source: https://github.com/decentralized-identity/did-resolver/blob/master/src/resolver.ts#L88-L99
I recall that the legacy versions were not matching the https://www.w3.org/TR/did-core/#did-document-properties spec..
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 reason I'm asking this is because the interface is already exported by did-resolver and you can extend it for the legacy use-case tests, making it more obvious which bits are considered legacy.
This is more a matter of preference, honestly, since we're dealing with tests here.
authentication?: VerificationMethodLegacyCommonSigner[] | ||
} | ||
|
||
type DIDDocumentCommonSigner = { |
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.
Why is this necessary?
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.
Short answer. I found the did-resolver library limited in terms of its interfaces and wanted to package the creation of did documents in functions. It seemed like a bit of a hack. I will get back to this.
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.
can you be more specific in where the interfaces are limiting?
The interfaces defined there are meant to mimic the DID spec, with some support for previous versions of the spec. If they are limiting, this might be to match the spec, but, if we missed something there, please raise an issue, instead of redefining these types.
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.
Okay, I put a question here: decentralized-identity/did-resolver#121 . If it helps the code to break up this way, it might be more streamlined to add the private key parameter "d" to the interface in the did-resolver. To be warned I may have seen an explicit reason given for leaving it out (but I am unsure where to look).
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.
There were some other things in there like (https://github.com/bshambaugh/did-jwt/blob/master/src/__tests__/JWT/CommonSignerTest/CommonSignerTest.ts#L24-L75) that were added to avoid the use of any in the typescript file. This might need to be refactored. I guess I didn't realize how scary this was going to get.
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 use of any
and unknown
in the test files is very much acceptable, as well as type casting for the sake of making tests simple to understand, especially for testing corner cases, as long as there exist some tests that run through the types as they are expressed or expected by the API.
You shouldn't feel the need to redefine big chunks of API just for testing corner cases.
Since these libraries are ultimately boiled down to plain JavaScript, it is even better to have tests that don't fit the neatly defined types that the IDE can warn you about.
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'm in the process of going through this, and thus made some changes to CommonSignerTest,ts, and could possibly do more. I posted a description of what I was up to in a slack channel, which helped guide my mind:
"CommonSignerTest.ts is an interesting file. It is a refactoring of part of https://github.com/decentralized-identity/did-jwt/blob/master/src/__tests__/JWT.test.ts . I split it out, while refactoring (the test, grin...).
Line 36 through 91 are interesting, and they were reformulated for use in a common file. Here is the new structure: https://github.com/bshambaugh/did-jwt/tree/master/src/__tests__/JWT . The const declarations that were
formally didDocLegacy, didDoc, and audDidDoc are now functions in ./CommonSignerTest/CommonSignerTest.ts in the new structure and CommonSignerTest.ts is called by JWT.ES256KSigner.test.ts and JWT.ES256Signer.test.ts .
I did this to avoid bloating of the test orignal test file. This makes my reviewer a bit bewildered. He is no longer sure what to trust. (#212 (review) the way the typescript editor in the Visual Studio IDE started complaining about my use of any. Interfaces came to the rescue. I was not entirely satisfied with the interfaces that came with the did-resolver npm package.
(https://github.com/decentralized-identity/did-resolver/blob/master/src/resolver.ts) , so I created a few of my own (https://github.com/bshambaugh/did-jwt/blob/master/src/__tests__/JWT/CommonSignerTest/CommonSignerTest.ts#L20-L36)
for legacy verification method and the private key (reason given here: decentralized-identity/did-resolver#121).
I also had to create some types for the legacy and more current did documents (https://github.com/bshambaugh/did-jwt/blob/master/src/__tests__/JWT/CommonSignerTest/CommonSignerTest.ts#L38-L62). In addition, I had to re-declare the
JsonWebKey interface since it was not exported from the did-resolver npm package (https://github.com/bshambaugh/did-jwt/blob/master/src/__tests__/JWT/CommonSignerTest/CommonSignerTest.ts#L6-L18)."
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.
tl;dr: maybe I should focus in on this:
"as well as type casting for the sake of making tests simple to understand, especially for testing corner cases, as long as there exist some tests that run through the types as they are expressed or expected by the API."
Do I need to write things in a different way? I had type any
for CreatedidDocLegacy , CreatedidDoc and CreateauddidDoc , but went the interface route when I got complaints from my IDE.
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.
Okay, CommonSignerTest.ts [1] is substantially simplified [1] (https://github.com/decentralized-identity/did-jwt/pull/212/files#diff-0a517e08539420acec2253799dfa947d00c7d2e854614a743dd72a08b9fd5664)
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.
@mirceanis , CommonSignerTest.ts is substantially cleaned up. I reused declarations from did-resolver where possible.
I am philosophically tremendously supportive of this week, but also need time to review. For reference, I believe this will also help us with satisfying more of the did-jwt-vc tests (https://github.com/decentralized-identity/JWS-Test-Suite). |
This library will need support for P-256 / ES256 or P-384 / ES384... especially if it needs to support at least 1 NIST approved signature format. |
…n-standard, what about cosmos??
I added an issue that provides a sampling of where things would be different: #219 . It is my belief that this re-organization in this PR could support ES384, ES512, RSA, etc... |
@decentralgabe is also working with the JWS-Test-Suite IIRC. He may find this PR interesting? Well anyway, I may have to come back to this in 10 days or so. |
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.
please remove package-lock.json as this project uses yarn.lock.
This will also to make this review less scary.
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 made a bit more progress on the review.
Still, this is going extremely slowly due to the massive refactoring of the tests. It would be much easier to review if we would have the original tests as intact as possible, and update them in a separate PR.
I am very tempted to close this PR and request that the tests are reverted, this is a huge time drain :(
* @param {Boolean} recoverable an optional flag to add the recovery param to the generated signatures | ||
* @return {Function} a configured signer function `(data: string | Uint8Array): Promise<string>` | ||
*/ | ||
export function ES256Signer(privateKey: string | Uint8Array, recoverable = false): Signer { |
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.
please remove the recoverable
parameter here, since ES256-R should not be added since there is no requirement for it.
@@ -85,8 +85,8 @@ export function toSealed(ciphertext: string, tag: string): Uint8Array { | |||
} | |||
|
|||
const hexMatcher = /^(0x)?([a-fA-F0-9]{64}|[a-fA-F0-9]{128})$/ | |||
const base58Matcher = /^([1-9A-HJ-NP-Za-km-z]{44}|[1-9A-HJ-NP-Za-km-z]{88})$/ | |||
const base64Matcher = /^([0-9a-zA-Z=\-_+/]{43}|[0-9a-zA-Z=\-_+/]{86})(={0,2})$/ | |||
const base58Matcher = /^([1-9A-HJ-NP-Za-km-z]{43,44}|[1-9A-HJ-NP-Za-km-z]{86,88})$/ |
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.
why is this change required?
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.
Sometimes the base58 and base64 matchers were conflicting as I recall. Here is the transcript:
/ debug log:
https://gist.github.com/bshambaugh/d0f87d471040399818eb7d258b660c49
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.
ok, if this is a problem let's address it in a separate issue describing the bug and with a test vector that fails with the existing matchers. Please revert this for this PR.
The keys being matched are of fixed size, there is no reason to have varying length here.
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 issue here is the change to the base58 matcher.
Please describe why this change was necessary with an example that fails with the current matcher.
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 created an issue: #222
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'll try to clean this up soon. My coffee ran out and tgat let me to let up on it a lot. As a result, I have really bad brain fog at the moment,and cannot really think clearly.
ethereumAddress?: string | ||
} | ||
|
||
interface VerificationMethodCommonSigner { |
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 reason I'm asking this is because the interface is already exported by did-resolver and you can extend it for the legacy use-case tests, making it more obvious which bits are considered legacy.
This is more a matter of preference, honestly, since we're dealing with tests here.
src/VerifierAlg/ES256KVerifierAlg.ts
Outdated
const secp256k1 = new EC('secp256k1') | ||
|
||
/* | ||
interface LegacyVerificationMethod extends VerificationMethod { |
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 prefer this simple extension of the publicly defined type as opposed to redefining the interfaces all together.
It seems clearer about what the legacy bits are.
src/VerifierAlgorithm.ts
Outdated
|
||
const secp256k1 = new EC('secp256k1') | ||
import * as Ed25519VerifierAlg from './VerifierAlg/Ed25519VerifierAlg' |
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.
just to be clearer, all these imports should be simplified this way, not just this line
authentication?: VerificationMethodLegacyCommonSigner[] | ||
} | ||
|
||
type DIDDocumentCommonSigner = { |
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 use of any
and unknown
in the test files is very much acceptable, as well as type casting for the sake of making tests simple to understand, especially for testing corner cases, as long as there exist some tests that run through the types as they are expressed or expected by the API.
You shouldn't feel the need to redefine big chunks of API just for testing corner cases.
Since these libraries are ultimately boiled down to plain JavaScript, it is even better to have tests that don't fit the neatly defined types that the IDE can warn you about.
|
||
describe('SignerAlgorithm', () => { | ||
|
||
it("returns a did doc legacy", () => { |
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 appreciate the thoroughness, but it feels like overkill to test the methods used during tests :)
expect(typeof SignerAlgorithm('ES256')).toEqual('function') | ||
}) | ||
|
||
it('supports ES256-R', () => { |
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.
please remove ES256-R
…did-resolver npm package, so import instead
… from did-resolver npm package
@bshambaugh, this PR continues to be very hard to follow. With such a massive set of changes, each individual "fix" to review comments is almost impossible to review because it has to be reviewed in context, which means going through the whole PR every time. It's probably hard for you as well to figure out where to fix things to get this PR approved and merged. Meanwhile, the branch you're on continues to drift apart from the main code base and it will be increasingly difficult to rebase your changes. There are a lot of conflicting files already. You've made lots of great contributions here, but also a lot of code has changed that shouldn't have, or at least shouldn't have changed in this PR. I would really love to see your contribution make it to the main codebase, so please consider closing this PR and opening another with as few changes as possible to unrelated code:
|
Okay, fair enough. I'll close this pull request and archive the code I have done. I have also been considering JSON Web Encryption that is not included in this PR. See RFC7516 (https://datatracker.ietf.org/doc/html/rfc7516) and https://github.com/decentralized-identity/did-jwt/blob/master/src/xc20pEncryption.ts . |
Thank you! Regarding JWE, there is plenty to be improved in that area as well. Please raise issues before posting PRs, though. In terms of refactoring, you have good ideas for splitting code and we will need that for #170 Thank you for your work! |
This may not be complete as is.
I am in the process of implementing a signer for a did:key provider for use with ceramic. it is like the lines below, which utilize this library.
https://github.com/ceramicnetwork/key-did-provider-ed25519/blob/main/src/index.ts#L53
https://github.com/ceramicnetwork/key-did-provider-secp256k1/blob/master/src/index.ts#L56
I will add to this PR as I visit JWT.(test).ts, VerifierAlgorithm.(test).ts, and SignerAlgorithm.(test).ts in the process of implementing https://eips.ethereum.org/EIPS/eip-2844 for the P-256 (secp256r1) key.
@oed has been reviewing a resolver that includes P-256 for ceramic ceramicnetwork/js-ceramic#1884 , so this is a heads up to him that I am in the process of working on this code base.
I am still not sure about the acronym I should use for the type:
Secp256r1VerificationKey2022 (Key like: https://lists.w3.org/Archives/Public/public-credentials/2018Mar/0104.html)
or P256Key2021(https://github.com/transmute-industries/did-key.js/blob/c9a478da12508fefd8018d82ab95638ea6980dd7/packages/did-key-test-vectors/src/secp256r1/did-key-secp256r1-case-1.json#L81)
sounds like a reasonable acronym to use in JWT.ts, however I only support application/did+json at the moment, not application/did+ld+json which seems to prefer publicKeyBase58 and an acronym like P256Key2021.
Thus, the less descriptive "JsonWebKey2020" seems like the way to go.
Note. ES256 and P-256 are mentioned here: https://www.iana.org/assignments/jose/jose.xhtml