Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refactor @web5/dids to support first class Key Management #393

Merged
merged 39 commits into from
Feb 1, 2024
Merged

Refactor @web5/dids to support first class Key Management #393

merged 39 commits into from
Feb 1, 2024

Conversation

frankhinek
Copy link
Contributor

@frankhinek frankhinek commented Jan 31, 2024 •
edited
Loading

Summary

The primary goal of this PR is to refactor the @web5/dids package to use the refactored @web5/crypto package that substantively changed how key management was approached. In addition to making key management integral to the DID method implementations, it also changes the DID method API to include methods for instantiating DID objects from existing DIDs/keys. The same pattern is being implemented across web5-js, web5-kt, web5-go, etc.

@frankhinek
Update Web5 Test Vectors to latest
073c0f3 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Fix issue with c8 not reporting code coverage and bump version
22c001f 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Remove unnecessary option in crypto tsconfig and enable strict mode f…
e17d5e8 
…or dids

Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Add JWK Set to crypto package
09083b2 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Add did:jwk and did:web implementations
7b51b76 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Add utility type to @web5/common
1187a09 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Improve DID Utility functions
b8cd77e 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Add FixedLengthArray utility type
b79dfff 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Add support for Base32Z to Convert utility
42bae59 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Improve JWK type
197181c 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Add InferKeyGeneratorAlgorithm utility type
2da26ee 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Remove did-resolver dependency
cf4cb33 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Temporarily disable old implementations
80ae0bd 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Update DID JWK and DHT implementations
de2cf33 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Rename createFromKeys to fromKeys and change default publishing behavior
6eff583 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Update JWK and DHT implementations
fcc2796 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Rename DidKeySet to PortableDid and add uri
0b0693f 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Reorganize tests and reintroduce DidResolver
be13b71 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Update package-lock.json
d14076a 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Rename P256 to Secp256r1 and add minimal tests
4148742 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Add docs for undocumented methods
53aa79b 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Finish reintroducing DID resolvers
00700f7 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Support secp256k1 as an algorithm identifier for generateKey()
cfb8a44 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Improve support for secp256r1
43f2507 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Rename to LocalKeyManager & AwsKeyManager and improve docs
0bfbc5b 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Fix Ed25519.validatePublicKey method signature
b7b8d79 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Refactor of DID Key
11b69ed 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Complete implementation of DID Key
7ec833e 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Add KeyCompressor type to @web5/crypto
29a27ab 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Complete refactor of DidIon
64120f0 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Remove old dependencies and bump versions
2ebbd77 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Resolve package dependency issue
6cb1abb 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek
Remove old test-vectors directory
c26d27e 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek frankhinek mentioned this pull request Jan 31, 2024
Closed
@frankhinek
Remove references to tags in the XChaCha20Poly1305 comments
38ba056 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
nitro-neal
nitro-neal reviewed Jan 31, 2024
View reviewed changes
packages/dids/src/methods/did-dht.ts
const did = await DidDht.fromPublicKeys({ keyManager, options, ...keySet });

// By default, publish the DID document to a DHT operations endpoint unless explicitly disabled.
did.metadata.published = options.publish ?? true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens when someone is on a plane and we try to publish?
timeout exception?

Copy link
Contributor Author

@frankhinek frankhinek Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As currently implemented, if the gateway/relay cannot be reached when attempting to publish an exception will be raised.

One option is to pass publish: false so that publishing isn't attempting (when calling create() or fromKeys())... but that assumes the application is able to detect a lack of network connectivity and adapt.

One of the downsides to the current approach is that if a DID is being created for the first time (calling create() to generate keys) and an exception is thrown during publishing, the newly generated keys aren't returned to the caller. By the time the exception is raised, the keys will have already been created and stored in the KeyManager store so they end up "orphaned" since the function raises an error before it can return the DID object.

I went back and forth on this when initially implementing, but another approach is to catch any errors that occur during publishing and return did.metadata.published: false. This way, the caller can detect and attempt to re-publish if they wish... and at least they have the DID URI and key references.

What are your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea that sounds reasonable.
I can't think of where it will be a huge deal for 'orphaned' keys will mess much up, they can just delete and make a new one.

Also you give them a lot of outs, setting publish to false.

If it turns into a big deal they can publish again later right?

I think leaving it the way it is is fine

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its a great point and the more I think about it... the more I think we ought to look at a results object for DID registration (create, update, and deactivate operations).

I'll leave it out for now and start a design doc to propose an approach.

Early thought is something that looks like: https://github.com/decentralized-identity/did-registration/blob/main/spec/spec.md#didregistrationmetadata

frankhinek and others added 2 commits January 31, 2024 16:01
@frankhinek
Remove duplication from DidDhtUtils.identifierToIdentityKey()
258446c 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek @nitro-neal
Correct typo in comments
53b6712 
Co-authored-by: nitro-neal <5314059+nitro-neal@users.noreply.github.com>
ianpatton
ianpatton reviewed Jan 31, 2024
View reviewed changes
packages/dids/src/methods/did-web.ts

// Replace ":" with "/" in the identifier and prepend "https://" to obtain the fully qualified
// domain name and optional path.
let baseUrl = `https://${parsedDid.id.replace(/:/g, '/')}`;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to use http:// if the uri is localhost, for local dev purposes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ianpatton Interesting point to consider. The did:web spec is fairly clear about requiring HTTPS. However, perhaps we could do something like a custom resolution option that allows for use of localhost endpoints that aren't secure.

@frankhinek
Update crypto README
0a97f05 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
nitro-neal
nitro-neal approved these changes Feb 1, 2024
View reviewed changes
Copy link
Contributor

@nitro-neal nitro-neal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow great job @frankhinek ! 🥇
I look forward to using this, I think it makes a lot of sense. Lots of new code and hard work.

@frankhinek
Correct typo, add DHT test, and remove unused file
a3e4448 
Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
@frankhinek frankhinek merged commit c9f3661 into main Feb 1, 2024
32 checks passed
@frankhinek frankhinek deleted the dids-new-crypto branch February 1, 2024 22:30
finn-tbd pushed a commit that referenced this pull request Mar 19, 2024
@frankhinek @nitro-neal @finn-tbd
Refactor @web5/dids to support first class Key Management (#393)
2cf43b5 
* Update Web5 Test Vectors to latest
* Fix issue with c8 not reporting code coverage and bump version
* Remove unnecessary option in crypto tsconfig and enable strict mode for dids
* Add JWK Set to crypto package
* Add did:jwk and did:web implementations
* Add utility type to @web5/common
* Improve DID Utility functions
* Add FixedLengthArray utility type
* Add support for Base32Z to Convert utility
* Improve JWK type
* Add InferKeyGeneratorAlgorithm utility type
* Remove did-resolver dependency
* Temporarily disable old implementations
* Update DID JWK and DHT implementations
* Rename createFromKeys to fromKeys and change default publishing behavior
* Update JWK and DHT implementations
* Rename DidKeySet to PortableDid and add uri
* Reorganize tests and reintroduce DidResolver
* Update package-lock.json
* Rename P256 to Secp256r1 and add minimal tests
* Add docs for undocumented methods
* Finish reintroducing DID resolvers
* Support secp256k1 as an algorithm identifier for generateKey()
* Improve support for secp256r1
* Rename to LocalKeyManager & AwsKeyManager and improve docs
* Fix Ed25519.validatePublicKey method signature
* Refactor of DID Key
* Complete implementation of DID Key
* Add KeyCompressor type to @web5/crypto
* Complete refactor of DidIon
* Complete adding secp256r1 test vectors
* Remove old dependencies and bump versions
* Resolve package dependency issue
* Remove old test-vectors directory
* Remove references to tags in the XChaCha20Poly1305 comments
* Remove duplication from DidDhtUtils.identifierToIdentityKey()
* Correct typo in comments
* Update crypto README
* Correct typo, add DHT test, and remove unused file

---------

Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
Co-authored-by: nitro-neal <5314059+nitro-neal@users.noreply.github.com>
finn-tbd pushed a commit that referenced this pull request Mar 19, 2024
@frankhinek @nitro-neal @finn-tbd
Refactor @web5/dids to support first class Key Management (#393)
6e308be 
* Update Web5 Test Vectors to latest
* Fix issue with c8 not reporting code coverage and bump version
* Remove unnecessary option in crypto tsconfig and enable strict mode for dids
* Add JWK Set to crypto package
* Add did:jwk and did:web implementations
* Add utility type to @web5/common
* Improve DID Utility functions
* Add FixedLengthArray utility type
* Add support for Base32Z to Convert utility
* Improve JWK type
* Add InferKeyGeneratorAlgorithm utility type
* Remove did-resolver dependency
* Temporarily disable old implementations
* Update DID JWK and DHT implementations
* Rename createFromKeys to fromKeys and change default publishing behavior
* Update JWK and DHT implementations
* Rename DidKeySet to PortableDid and add uri
* Reorganize tests and reintroduce DidResolver
* Update package-lock.json
* Rename P256 to Secp256r1 and add minimal tests
* Add docs for undocumented methods
* Finish reintroducing DID resolvers
* Support secp256k1 as an algorithm identifier for generateKey()
* Improve support for secp256r1
* Rename to LocalKeyManager & AwsKeyManager and improve docs
* Fix Ed25519.validatePublicKey method signature
* Refactor of DID Key
* Complete implementation of DID Key
* Add KeyCompressor type to @web5/crypto
* Complete refactor of DidIon
* Complete adding secp256r1 test vectors
* Remove old dependencies and bump versions
* Resolve package dependency issue
* Remove old test-vectors directory
* Remove references to tags in the XChaCha20Poly1305 comments
* Remove duplication from DidDhtUtils.identifierToIdentityKey()
* Correct typo in comments
* Update crypto README
* Correct typo, add DHT test, and remove unused file

---------

Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
Co-authored-by: nitro-neal <5314059+nitro-neal@users.noreply.github.com>
finn-tbd pushed a commit that referenced this pull request Mar 19, 2024
@frankhinek @nitro-neal @finn-tbd
Refactor @web5/dids to support first class Key Management (#393)
8d1a92b 
* Update Web5 Test Vectors to latest
* Fix issue with c8 not reporting code coverage and bump version
* Remove unnecessary option in crypto tsconfig and enable strict mode for dids
* Add JWK Set to crypto package
* Add did:jwk and did:web implementations
* Add utility type to @web5/common
* Improve DID Utility functions
* Add FixedLengthArray utility type
* Add support for Base32Z to Convert utility
* Improve JWK type
* Add InferKeyGeneratorAlgorithm utility type
* Remove did-resolver dependency
* Temporarily disable old implementations
* Update DID JWK and DHT implementations
* Rename createFromKeys to fromKeys and change default publishing behavior
* Update JWK and DHT implementations
* Rename DidKeySet to PortableDid and add uri
* Reorganize tests and reintroduce DidResolver
* Update package-lock.json
* Rename P256 to Secp256r1 and add minimal tests
* Add docs for undocumented methods
* Finish reintroducing DID resolvers
* Support secp256k1 as an algorithm identifier for generateKey()
* Improve support for secp256r1
* Rename to LocalKeyManager & AwsKeyManager and improve docs
* Fix Ed25519.validatePublicKey method signature
* Refactor of DID Key
* Complete implementation of DID Key
* Add KeyCompressor type to @web5/crypto
* Complete refactor of DidIon
* Complete adding secp256r1 test vectors
* Remove old dependencies and bump versions
* Resolve package dependency issue
* Remove old test-vectors directory
* Remove references to tags in the XChaCha20Poly1305 comments
* Remove duplication from DidDhtUtils.identifierToIdentityKey()
* Correct typo in comments
* Update crypto README
* Correct typo, add DHT test, and remove unused file

---------

Signed-off-by: Frank Hinek <frankhinek@users.noreply.github.com>
Co-authored-by: nitro-neal <5314059+nitro-neal@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ianpatton ianpatton ianpatton left review comments

@mistermoe mistermoe mistermoe approved these changes

@nitro-neal nitro-neal nitro-neal approved these changes

@csuwildcat csuwildcat Awaiting requested review from csuwildcat csuwildcat is a code owner

Assignees

@frankhinek frankhinek

Labels
key-mgmt Key Management package: dids @web5/dids package
Projects
None yet
Milestone
No milestone
4 participants
@frankhinek @mistermoe @nitro-neal @ianpatton