Create and resolve did:jwk #20

amika-sq · 2023-11-30T20:45:05Z

This is the first DID implementation within the crates/dids crate, so the PR is a little bit on the larger side. I promise the next DID implementations will be MUCH smaller 😅

The actual creation/resolution of the did:jwk itself is being handled by Spruce's did-jwk crate.

Beyond just create/resolve, this PR sets up the following structure:

Define traits for Did and DidMethod. This will allow us to implement other DID methods in the future.
Defines our own unique DidResolutionResult. Our upcoming test vectors expect a resolution result in a specific format. Spruce's resolution result doesn't match that format, so I defined our own that does.

Resolves #13

amika-sq · 2023-11-30T20:47:15Z

crates/dids/src/resolver.rs

+ async fn resolve_did_jwk() {
+ let did_uri = "did:jwk:eyJjcnYiOiJQLTI1NiIsImt0eSI6IkVDIiwieCI6ImFjYklRaXVNczNpOF91c3pFakoydHBUdFJNNEVVM3l6OTFQSDZDZEgyVjAiLCJ5IjoiX0tjeUxqOXZXTXB0bm1LdG00NkdxRHo4d2Y3NEk1TEtncmwyR3pIM25TRSJ9";
+ let result = DidResolver::resolve_uri(did_uri).await;
+ assert!(result.did_resolution_metadata.error.is_none());
+
+ let did_document = result.did_document.unwrap();
+ assert_eq!(did_document.id, did_uri);
+ }


I currently do not consume the test vector in this PR, as I wanted to keep it as small as I could. Going to be adding that in a follow-up PR to this. For now, I resolve the did:jwk as defined from the first vector here.

cool, thanks for keeping things concise here

amika-sq · 2023-11-30T20:48:21Z

crates/credentials/Cargo.toml

-serde = { version = "1.0.193", features = ["derive"] }
-serde_json = "1.0.108"
-serde_with = "3.4.0"
+serde = { workspace = true }
+serde_json = { workspace = true }
+serde_with = { workspace = true }


the credentials and dids crate both now depend on serde crates. Moved them to the workspace Cargo.toml file so that their versions are always in sync with each other.

amika-sq · 2023-11-30T20:52:43Z

crates/dids/src/method/jwk.rs

+ Ok(DidJwk { uri, key_manager })
+ }
+
+ async fn resolve_uri(did_uri: &str) -> DidResolutionResult {


There's a lot of code within TBD that use the term "did." Depending on where you're looking, it could mean one of two things:

The unique DID string

A structured representation of a DID

In this PR, I chose to always refer to the unique DID string as did_uri to hopefully help clarity while reading the code.

Looking for feedback on this!
Do you love it? Do you hate it? Happy to change if there's any strong feelings here.

Agreed with your decision. Which is to say, DID to me means (2) whereas (1) is a DID's uri.

Makes sense, it's where we landed on the KT side (in most places).

KendallWeihe · 2023-12-01T13:12:08Z

crates/dids/src/method/jwk.rs

+ assert!(did.uri.starts_with("did:jwk:"));
+ }
+
+ #[tokio::test]


So why tokio?

I presume it has something to do with us actually testing an external network integration

DidJwk resolution should not go over the network. Chad tells me that

The #[tokio::test] attribute is specific to the tokio runtime and is used to mark an asynchronous test function. It indicates that the following function is an asynchronous test that should be executed using the tokio runtime.

@andresuribe87 has it right.

Resolution is generic across all DID methods, and resolution CAN be an asynchronous operation for certain methods. So the DidMethod trait defines the resolve function as asynchronous.

While did:jwk does not resolve over the network, the function must be asynchronous to conform to the DidMethod trait.

We have a few options to test asynchronous functions:

Write a normal, synchronous, test. Whenever you need to call an asynchronous function within the test, block the thread until the asynchronous function completes.

Use tokio or async-std (amongst others).

Tokio is much more widely adopted, so I went with that. Happy to alter though if there's any strong opinions another way!

Makes sense to me 👍

crates/dids/src/resolver.rs

KendallWeihe

nice! lgtm!

andresuribe87

This is awesome, some question/suggestion from what we've seen in Kt.

crates/dids/src/did.rs

andresuribe87 · 2023-12-01T14:51:07Z

crates/dids/src/did.rs

+ fn key_manager(&self) -> &Arc<dyn KeyManager>;
+
+ async fn resolve(&self) -> DidResolutionResult {
+ DidResolver::resolve_uri(self.uri()).await


My only qualm with this is that consumers of the API have no way of specifying which HTTP client to use under the hood.

andresuribe87 · 2023-12-01T14:55:47Z

crates/dids/src/method/jwk.rs

+ let uri = SpruceDidJwkMethod
+ .generate(&Source::Key(&public_key.jwk()))
+ .ok_or(DidMethodError::DidCreationFailure(
+ "Failed to generate did:jwk".to_string(),
+ ))?;


DidJwk is dead simple. What do you think about implementing it instead of using the spruce dependency?

We could, but its also a standard.

My thought is: why re-invent the wheel here?
The dependency is 3.38 KB according to crates.io, which seemed like a good tradeoff to me.

One thing to note - the did:jwk spec is being moved to the IETF with some changes now (and more coming later). We will need to be agile with our implementation, so having more control is a benefit. For example, we'll soon need to add in optional did:dht resolution for jwk.

We can totally add any changes if/when needed! But right now, there's no difference. I'd be copy/pasting Spruce's implementation. If that's what we want, I can do it!

It also puts us in a better position for supply chain security by minimizing deps.

It also puts us in a better position for supply chain security by minimizing deps.

implementing did:jwk ourselves wouldn't put us in a position to drop spruce's sdk as a dependency

unless we have concerns about how did:jwk is currently implemented in spruce's sdk, i don't see why we'd implement it ourselves at the moment

andresuribe87 · 2023-12-01T15:02:36Z

crates/dids/src/method/jwk.rs

+
+/// Options that can be used to create a did:jwk DID
+pub struct DidJwkCreateOptions {
+ pub key_type: KeyType,


I actually found KeyType to be a bit confusing. Looking at the docs for KeyType didn't actually help.

Is KeyType meant to represent https://www.rfc-editor.org/rfc/rfc7518.html#section-6.1 ? Or is it meant to represent the name of the elliptic curve?

In kotlin, we defined the creation options as Algo and Curve, curious how/why this diverged?

I originally had it as KeyAlgorithm, but was recommended to rename it to KeyType here: #17 (comment)

Each of the algorithms use the same curve in Kotlin, so I just didn't add them to the Rust version yet. Can add them later if/when we use them.

As an outsider, I like the clarity of KeyAlgorithm over KeyType

Sounds good, I'll make a new PR to change it back!

These are not key algorithms, they are types of keys. Algorithms are used with types of keys to do key operations (like sign/verify, encrypt/decrypt). We should not deviate from industry terms here.

I'm still confused. @decentralgabe can you share the URL for the industry term that you are referencing?

I was asking if KeyType is https://www.rfc-editor.org/rfc/rfc7518.html#section-6.1 (which is in turn defined in https://www.rfc-editor.org/rfc/rfc7517.html#section-4.1). For reference, definitions is below

The "kty" (key type) parameter identifies the cryptographic algorithm
family used with the key, such as "RSA" or "EC".

yes I was referring to the concept similar to kty https://www.rfc-editor.org/rfc/rfc7518.html#section-6.1

andresuribe87 · 2023-12-01T15:06:29Z

crates/dids/src/method/jwk.rs

+ Ok(DidJwk { uri, key_manager })
+ }
+
+ async fn resolve_uri(did_uri: &str) -> DidResolutionResult {


Makes sense, it's where we landed on the KT side (in most places).

crates/dids/src/did.rs

andresuribe87 · 2023-12-01T15:19:18Z

crates/dids/src/did.rs

+ fn uri(&self) -> &str;
+ fn key_manager(&self) -> &Arc<dyn KeyManager>;
+
+ async fn resolve(&self) -> DidResolutionResult {


I think?

Suggested change

async fn resolve(&self) -> DidResolutionResult {

/// Returns a DidResolutionResult as specified in https://www.w3.org/TR/did-core/#did-resolution

async fn resolve(&self) -> DidResolutionResult {

andresuribe87 · 2023-12-01T15:20:10Z

crates/dids/src/method/mod.rs

+ options: CreateOptions,
+ ) -> Result<T, DidMethodError>;
+
+ /// Resolve a DID URI to a DID Document.


Suggested change

/// Resolve a DID URI to a DID Document.

/// Resolve a DID URI to a DidResolutionResult.

I'll update and submit a PR to the Kotlin side as well here.

andresuribe87 · 2023-12-01T15:20:54Z

crates/dids/src/resolver.rs

+ fn method_name(did_uri: &str) -> Option<&str> {
+ let parts: Vec<&str> = did_uri.split(':').collect();
+ if parts.len() < 3 || parts[0] != "did" {
+ return None;
+ };
+
+ Some(parts[1])
+ }


I would expect this code to be inside the Did trait. Thoughts?

The Did trait defines behavior for Did implementations. Each implementation is already defining the name it's implementing. This function is computing the method name from some arbitrarily provided string.

We could definitely move this into a utils module or something similar.

The answer is never "utils"

For the record, I agree. Just suggesting if we wanted to move it anywhere else, it doesn't really have a good home at the moment, because this is the only struct that deals with resolving arbitrary strings.

crates/dids/src/method/jwk.rs

andresuribe87

None of my comments are blocking, but I think there's good conversations that we need to figure out.

Create and resolve did:jwk

9738681

amika-sq requested review from frankhinek, andresuribe87, KendallWeihe, mistermoe, decentralgabe, phoebe-lew and diehuxx November 30, 2023 20:45

amika-sq commented Nov 30, 2023

View reviewed changes

amika-sq mentioned this pull request Nov 30, 2023

Consume did:jwk test vector #21

Open

KendallWeihe reviewed Dec 1, 2023

View reviewed changes

crates/dids/src/resolver.rs Show resolved Hide resolved

KendallWeihe approved these changes Dec 1, 2023

View reviewed changes

andresuribe87 reviewed Dec 1, 2023

View reviewed changes

add/update comments

a4e8290

amika-sq mentioned this pull request Dec 4, 2023

Rename KeyType -> KeyAlgorithm #34

Closed

andresuribe87 approved these changes Dec 4, 2023

View reviewed changes

amika-sq merged commit 7e12405 into main Dec 4, 2023

amika-sq deleted the did-jwk-finally branch December 4, 2023 20:59

amika-sq mentioned this pull request Dec 5, 2023

Implement our own did:jwk creation/resolution #41

Closed

	async fn resolve(&self) -> DidResolutionResult {
	/// Returns a DidResolutionResult as specified in https://www.w3.org/TR/did-core/#did-resolution
	async fn resolve(&self) -> DidResolutionResult {

	/// Resolve a DID URI to a DID Document.
	/// Resolve a DID URI to a DidResolutionResult.

Create and resolve did:jwk #20

Create and resolve did:jwk #20

Conversation

amika-sq commented Nov 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KendallWeihe left a comment

Choose a reason for hiding this comment

andresuribe87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresuribe87 left a comment

Choose a reason for hiding this comment