Update README; add clarifying language around DIDs and VCs #122

decentralgabe · 2024-03-06T16:48:45Z

PR is ready for review. Two follow ups:

spec/did.md

Co-authored-by: Frank Hinek <[email protected]>

frankhinek · 2024-03-06T16:52:16Z

A few discussion topics that come to mind -- raising to discuss but not state a strong preference:

Any particular reason we don't require the verification relationship entries be fully qualified DID URIs? I'm trying to think of a case where we'd want to support relative DID URIs and haven't come up with one but I'm probably overlooking a scenario.
Should we require support for publicKeyMultibase in our SDKs or drop it?
It's worth noting that contentType property referenced here should never be present in DID Resolution Metadata unless the result is being returned by the resolveRepresentation function. Since our SDKs today only support the resolve function, we can likely leave it out for now.

decentralgabe · 2024-03-06T16:56:54Z

Responding in place.

Any particular reason we don't require the verification relationship entries be fully qualified DID URIs? I'm trying to think of a case where we'd want to support relative DID URIs and haven't come up with one but I'm probably overlooking a scenario.

Some DID methods, like did key don't do this. I know we already have impls for these so I didn't want to break them.

Should we require support for publicKeyMultibase in our SDKs or drop it?

Similarly, this is a requirement of our existing did:key support. We may encounter keys encoded like this with did:web but that's less of a concert to me.

It's worth noting that contentType property referenced here should never be present in DID Resolution Metadata unless the result is being returned by the resolveRepresentation function. Since our SDKs today only support the resolve function, we can likely leave it out for now.

Good call out. I will remove the property entirely.

frankhinek · 2024-03-06T17:02:08Z

Any particular reason we don't require the verification relationship entries be fully qualified DID URIs? I'm trying to think of a case where we'd want to support relative DID URIs and haven't come up with one but I'm probably overlooking a scenario.

Some DID methods, like did key don't do this. I know we already have impls for these so I didn't want to break them.

It appears that the DID Key method requires that the id property be used in verification relationships so spec compliant implementations should be using fully qualified DID URIs:
https://w3c-ccg.github.io/did-method-key/#document-creation-algorithm

Are there well used DID Key libs that use relative DID URIs that you're aware of?

decentralgabe · 2024-03-06T17:33:44Z

@frankhinek my mistake! you are correct. I will update the guidance to require fully qualified URIs.

README.md

jiyoonie9 · 2024-03-06T16:57:36Z

README.md

+| In-Memory Key Manager |
+| AWS KMS               |
+| Device Enclave        |
+| Keychain              |


keychain is the only one that's specific to swift, right? would it be appropriate to call that out in this one column of feature list?

Good question @jiyoontbd

I'm wondering if we should even include Device Enclave? I'm a newb when it comes to native mobile dev, but to my knowledge we can't store Ed25519 or secp256k1 keys in any iOS or Android device's enclave (only NIST P-256/secp256r1 is supported). Perhaps we leave out requiring the implementation of a Key Manager for mobile device secure enclaves?

On Keychain: Perhaps include both iOS and Android? As noted here and implemented here, it is possible to stored Ed25519 and secp256k1 keys in iOS Keychain.

Do we already have a preferred approach for Android? AFAIK Android Key Store has no support for Ed25519 or secp256k1. Perhaps an approach similar to the one Samsung took?

so right now we only have inmemory + aws kms (in all languages i think?), and ios keychain (swift only).

currently i'm not sure if android key management or general mobile device enclave should be listed in this column since:

we haven't implemented them yet and

we also have not decided how to write a key manager against either android phones or device enclaves

created issues here
TBD54566975/tbdex#271
TBD54566975/tbdex#272

please feel free to close these if needed, i am probably wrong / not up to date on this subject.

I'm going to keep spec text as-is until we have a clearer view of what we should support

jiyoonie9 · 2024-03-06T20:00:35Z

spec/did.md

+| `@context`    | Array of strings    | No       | Depends on the DID method. |
+| `controller`  | Array of strings    | No       | Depends on the DID method. Strings must be URIs. |
+| `alsoKnownAs`        | Array of strings | No   | Depends on the DID method. Strings must be URIs. |
+| `verificationMethod` | Array of [Verification Methods](#verification-method-data-model) | Yes | There must be at least one Verification Method in each DID Document. |


i want to say that there's a scenario in building did dht diddocument where i know the id of the verificationMethod (which makes their way into one of 5 array of strings below), but not the verif method itself... but not 100% sure.

either way, requiring this field to be present is a departure from the spec, yea?

yes...have an upcoming dht pr to make that bit clearer with required relationships

and yes this is a departure from the spec

without a key I don't see the point in a DID Doc (feel free to convince me otherwise)

this being a departure from the spec is a-ok with me!

i did think that bit of including the id but not linking it back to an actual vm was weird, so i'm on board with this.

also side note, i am also probably going to be refactoring dht stuff in kt, so if you have a kt pr in flight we should coordinate

Perhaps we've yet to encounter a practical scenario, but I agree with the above and don't see much utility for any use cases we're contemplating to have a DID Document that lacks at least one verificationMethod entry.

Additionally, I've personally not run into a case where "embedded" verification method usage in a verification relationship property is necessary. In other words, a scenario where the only solution is to include a verification method embedded in a verification relationship property and NOT within the verificationMethods array. I'd imagine the W3C spec contributors had some reason for including this in the DID Core spec, but I've come up blank in trying to identify the motivation?

Seems like a question we might ask later -- but only to be prepared for a future Jeopardy round where 'Obscure DID Chronicles' is the surprise category.

+1 to all of it

spec/did.md

jiyoonie9 · 2024-03-06T20:13:09Z

spec/did.md

+| `nextUpdate`  | String            | No       | [XML Datetime](https://www.w3.org/TR/xmlschema11-2/#dateTime) value for when the next update of the DID. |
+| `versionId`   | String            | No       | Represents the version of the last update operation. |
+| `nextversionId`| String           | No       | Represents the version of the next update operation. |
+| `equivalentId` | Array of Strings | No       | A stronger form of the `alsoKnownAs` property, guaranteed by the DID method. |


what do you mean by stronger form here? 😃

the language is taken directly from https://www.w3.org/TR/did-core/#did-document-metadata

open to suggestions to make it clearer

maybe providing an example would help

@decentralgabe Both Moe and I had included some form of an explanation for equivalentId in web5-go and web5-js, which are both based on the DID Core spec language.

Perhaps including the pre-registration / post-registration example that is referenced would be a happy medium that tries to provide more context without being overly verbose?

Could also link directly to this note https://www.w3.org/TR/did-core/#h-note-10

Pre-registraion/post-registration as the example case is really good, but may be too wordy for this doc (not sure?)

Co-authored-by: Jiyoon Koo <[email protected]>

mistermoe · 2024-03-07T06:21:33Z

spec/did.md

+
+| Property      | JSON Representation | Required | Notes          |
+| ------------- | ------------------- | -------- | -------------- |
+| `error`       |  String             | No       | Required if there was an error during resolution. One of [`invalidDid`, `notFound`, `representationNotSupported`]. |


thoughts on including methodNotSupported?

we also have internalError as one of the error enums

➕ 1 on adding methodNotSupported and internalError.

I also think we ought to consider registering additional errors in the DID Resolution Spec that are particularly relevant for DID DHT resolution:

invalidDidDocument: The DID document supplied does not conform to valid syntax.

invalidDidDocumentLength: The byte length of a DID document is not within an acceptable range, similar to the Web IDL / DOM RangeError

will add both

what should internalError mean?

@decentralgabe Missed this question. I've always interpreted internalError similar to the role HTTP Status code 500 plays. The (draft) spec text reads:

When an unexpected error occurs during DID Resolution or DID URL dereferencing, the value of the DID Resolution or DID URL Dereferencing Metadata error property MUST be internalError.

Given that the error is "unexpected" it covers cases of errors that aren't mapped to a more specific error code.

mistermoe · 2024-03-07T06:23:41Z

spec/did.md

+| `type`        | String              | Yes      | Must be a URI. |
+| `controller`  | String              | Yes      | Must be a URI. |
+| `publicKeyJwk`        | Object      | No       | Preferred. Represents a [JWK](https://www.w3.org/TR/did-core/#bib-rfc7517). Either this or `publicKeyMultibase` must be present. |
+| `publicKeyMultibase`  | String      | No       | Not preferred. Either this or `publicKeyJwk` must be present. |


are we cool with dropping this or adding it in later on? will be tough to find time to implement in dart. can find time if we feel it's important

To add my personal opinion to the question, I am comfortable dropping publicKeyMultibase as a requirement for the first iteration of this spec.

@decentralgabe if you know of any significant adoption of Multibase format for DIDs by US state DMVs or EU member states, that could be a reason to include. However, given our heavy reliance on the JWK format in every SDK we've built to date and that DID JWK and DID DHT currently have no plans to support verification material in any format other than publicKeyJwk, it doesn't seem unreasonable to exclude if it adds work to our plates.

Side note: Juan/Protocol Labs & Manu continue to work on getting the multibase spec ratified.

let's drop it

mistermoe

amazing work @decentralgabe this is super helpful

KendallWeihe · 2024-03-07T12:48:07Z

README.md

+| Signing and verification of Verifiable Credentials as `vc-jwt`   |
+| Signing and verification of Verifiable Presentations as `vp-jwt` |


Any relevant links (specs, docs, etc?) for vc-jwt and vp-jwt?

README.md

KendallWeihe · 2024-03-07T12:52:23Z

README.md

+| Feature                                                          |
+| ---------------------------------------------------------------- |
+| Data model                                                       |
+| Validation (data model, JSON Schema, status)                     |


Does status here refer to this https://www.w3.org/TR/vc-data-model-2.0/#status? (wondering why it's called out specifically)

oh I see because status is a property which is a set of specs (the status lists below) as opposed to just another property of the data model

KendallWeihe · 2024-03-07T12:53:13Z

README.md

+| Data model               | 
+| Signing and verification | 


No Validation feature here? (maybe safely assumed?)

+1 to validation ?
Also, are we going to implement the "decoys" functionality for sd-jwt?

I will add validation back

KendallWeihe · 2024-03-07T13:11:46Z

spec/did.md

+
+## DID Resolution Metadata Data Model
+
+DID Resolution Metadata is _always optional_.


Feeling dumb, what does it mean for this to be "always optional?" (same is true in the doc metadata below)

Does it mean that any web5 conformant implementation need not implement support for this data model, such that during application execution the data may exist for the given DID but the SDK need-not throw an exception?

yes exactly, I will clarify

spec/vc.md

nitro-neal · 2024-03-07T16:41:09Z

spec/vc.md

+| `id`          | String              | Yes      | A URI representing a unique identifier for the credential. Recommended to be of form `urn:uuid:3978344f-8596-4c3a-a978-8fcaba3903c5`. |
+| `type`        | Array of strings    | Yes      | Type(s) of the credential. Must include `VerifiableCredential`. |
+| `issuer`      | String              | Yes      | A DID representing a unique identifier for the entity that issued the credential. |
+| `issuanceDate`| String              | Yes      | [XML Datetime](https://www.w3.org/TR/xmlschema11-2/#dateTime) value for when the credential was issued. |


we will accept XmlSchema112 and RFC3339 and then represent internally and output only XmlSchema112 right?

yeah, in practice I've used rfc3339 which is a subset of ISO8601 which is a subset of xml date time

nitro-neal · 2024-03-07T16:42:56Z

spec/vc.md

+**Additional Notes:**
+- The `credentialSubject` property can be any JSON object. It is recommended that this object is defined by an associated `credentialSchema`.
+- No [JSON-LD processing](https://www.w3.org/TR/vc-data-model/#json-ld) is performed.
+- Embedded proofs, using the `proof` property must not be present.


if we accept an outside vc with proofs we can still accept it and just drop it?
Or there should be no such thing as a vcjwt with proofs too ?

should be no such thing

nitro-neal · 2024-03-07T16:45:19Z

spec/vc.md

+- The `credentialSubject` property can be any JSON object. It is recommended that this object is defined by an associated `credentialSchema`.
+- No [JSON-LD processing](https://www.w3.org/TR/vc-data-model/#json-ld) is performed.
+- Embedded proofs, using the `proof` property must not be present.
+- The `type` property may only contain `VerifiableCredential` or the URI of a JSON Schema, if one is used for the credential.


This is not how we do it as of now.

Currently:

const vc = await VerifiableCredential.create({ type : 'TBDeveloperCredential', subject : did.uri, issuer : did.uri, data : { legit: true } });

So from what you are saying our type "TBDeveloperCredential" cannot be added as a type unless we include the URI of the json schema is that correct?

If that is a hard rule then we will need to refactor our codebase

yes, this isn't useful IMO and breaks the spec
if we have a need for a concrete type to switch on we should be using a JSON Schema

nitro-neal · 2024-03-07T16:45:57Z

spec/vc.md

+- No [JSON-LD processing](https://www.w3.org/TR/vc-data-model/#json-ld) is performed.
+- Embedded proofs, using the `proof` property must not be present.
+- The `type` property may only contain `VerifiableCredential` or the URI of a JSON Schema, if one is used for the credential.
+- We do not support multiple credential subjects.


nitro-neal · 2024-03-07T16:48:14Z

spec/vc.md

+
+Following from [this data model](https://www.w3.org/community/reports/credentials/CG-FINAL-vc-status-list-2021-20230102/).
+
+**StatusList2021Entry**


Are you still going to add BitstringStatusListEntry section?

when we support v2 of the data model I will add an entire section on that and bitstring status list

spec/vc.md

nitro-neal

A few nuanced comments on the VC part, but great job! This doc will be a life saver

Co-authored-by: Kendall Weihe <[email protected]>

kirahsapong · 2024-03-07T23:36:24Z

spec/vc.md

+| `credentialSubject.id` | String     | Yes      | A DID representing a unique identifier for whom the credential's claims are made. |
+| `credentialStatus` | Object defined by [Credential Status](#credential-status) | No | Only to be used with [Status List 2021](https://www.w3.org/community/reports/credentials/CG-FINAL-vc-status-list-2021-20230102/). |
+| `credentialSchema` | Object defined by [Credential Schema](#credential-schema) | No | Recommended. Only to be used with the type [`JsonSchema`](https://w3c.github.io/vc-json-schema/#jsonschema). |
+| `evidence`    | Object              | No       | Any JSON object as per [Evidence](https://www.w3.org/TR/vc-data-model/#evidence). |


Each evidence scheme is identified by its type.

does this mean type is required and additional fields may be present?

The value of the evidence property MUST be one or more evidence schemes

since we have Object here, are we only accepting one evidence scheme? not an array?

this is the one I was least sure of I know we need evidence, but I'm not sure where.

yes each type field is required

I am open to making this an array of objects for future proofing or alternatively dropping it all together until we have more info wdyt @mistermoe

updating this to array and adding a note on not implementing evidence until we have a type we can rely upon

Co-authored-by: nitro-neal <[email protected]>

Co-authored-by: Kendall Weihe <[email protected]>

decentralgabe · 2024-03-13T04:08:51Z

Going to merge tomorrow pending objections/final review. cc: @mistermoe @frankhinek @phoebe-lew @KendallWeihe @nitro-neal @jiyoontbd @kirahsapong

jiyoonie9 · 2024-03-13T11:51:20Z

spec/vc-11.json

this one and vp-11.json are empty?

edit: actually all *.json in this pr is empty 🤔

yes, intending to address these in a follow up #126

jiyoonie9

awesome work, gabe!!! 🥇

decentralgabe · 2024-03-13T15:39:40Z

merging but open to putting additional feedback in an upcoming PR

gabe added 5 commits March 5, 2024 18:26

pr feedback

87c10b1

fix headings

d66375f

one more

b9ac37e

more language

013f96b

did language

bb2b313

frankhinek reviewed Mar 6, 2024

View reviewed changes

spec/did.md Outdated Show resolved Hide resolved

decentralgabe and others added 2 commits March 6, 2024 08:51

Update spec/did.md

9bd4aa3

Co-authored-by: Frank Hinek <[email protected]>

Merge branch 'main' into did-vc

19fca83

pr feedback

4c4050a

jiyoonie9 reviewed Mar 6, 2024

View reviewed changes

decentralgabe and others added 3 commits March 6, 2024 15:00

Update README.md

ab4d058

Co-authored-by: Jiyoon Koo <[email protected]>

add vc guidance

adea451

add vc text

2e5ce92

decentralgabe requested review from KendallWeihe and nitro-neal March 6, 2024 23:52

mistermoe reviewed Mar 7, 2024

View reviewed changes

mistermoe approved these changes Mar 7, 2024

View reviewed changes

KendallWeihe reviewed Mar 7, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

KendallWeihe reviewed Mar 7, 2024

View reviewed changes

README.md Show resolved Hide resolved

KendallWeihe reviewed Mar 7, 2024

View reviewed changes

spec/vc.md Outdated Show resolved Hide resolved

KendallWeihe reviewed Mar 7, 2024

View reviewed changes

spec/vc.md Outdated Show resolved Hide resolved

nitro-neal reviewed Mar 7, 2024

View reviewed changes

spec/vc.md Outdated Show resolved Hide resolved

nitro-neal approved these changes Mar 7, 2024

View reviewed changes

decentralgabe and others added 2 commits March 7, 2024 12:06

Update spec/vc.md

0c3fd9a

Co-authored-by: Kendall Weihe <[email protected]>

Update spec/vc.md

efc49f2

Co-authored-by: Kendall Weihe <[email protected]>

kirahsapong reviewed Mar 7, 2024

View reviewed changes

decentralgabe and others added 2 commits March 7, 2024 16:10

Update spec/vc.md

0d974da

Co-authored-by: nitro-neal <[email protected]>

Update README.md

60967f6

Co-authored-by: Kendall Weihe <[email protected]>

decentralgabe mentioned this pull request Mar 8, 2024

Specify validations / verifications for processing VCs #123

Open

KendallWeihe mentioned this pull request Mar 8, 2024

Document.Context mixed types decentralized-identity/web5-go#104

Closed

decentralgabe and others added 3 commits March 12, 2024 20:56

Update spec/vc.md

02b1f7c

Co-authored-by: Kendall Weihe <[email protected]>

pr feedback (#125)

7c8e6e1

additional comments

dd7d016

decentralgabe marked this pull request as ready for review March 13, 2024 04:06

decentralgabe requested review from phoebe-lew and amika-sq as code owners March 13, 2024 04:06

decentralgabe mentioned this pull request Mar 13, 2024

Define JSON schemas for specified DID and VC data models #126

Closed

jiyoonie9 reviewed Mar 13, 2024

View reviewed changes

jiyoonie9 approved these changes Mar 13, 2024

View reviewed changes

decentralgabe merged commit 42e4842 into main Mar 13, 2024

decentralgabe deleted the did-vc branch March 13, 2024 15:39

KendallWeihe mentioned this pull request Mar 15, 2024

Mixed type properties for DIDs and VCs decentralized-identity/web5-go#109

Open

1 task

KendallWeihe mentioned this pull request May 22, 2024

Convert did:dht services to dns records decentralized-identity/web5-rs#216

Merged

jiyoonie9 mentioned this pull request May 29, 2024

feat: add internal did:web resolution decentralized-identity/web5-rs#190

Merged

		\| Signing and verification of Verifiable Credentials as `vc-jwt` \|
		\| Signing and verification of Verifiable Presentations as `vp-jwt` \|


		## DID Resolution Metadata Data Model

		DID Resolution Metadata is _always optional_.


		Following from [this data model](https://www.w3.org/community/reports/credentials/CG-FINAL-vc-status-list-2021-20230102/).

		StatusList2021Entry

Update README; add clarifying language around DIDs and VCs #122

Update README; add clarifying language around DIDs and VCs #122

Conversation

decentralgabe commented Mar 6, 2024 • edited Loading

frankhinek commented Mar 6, 2024

decentralgabe commented Mar 6, 2024

frankhinek commented Mar 6, 2024

decentralgabe commented Mar 6, 2024

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

frankhinek Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistermoe 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

KendallWeihe Mar 7, 2024 • edited Loading

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

nitro-neal Mar 7, 2024 • edited Loading

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

nitro-neal 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

decentralgabe commented Mar 13, 2024

jiyoonie9 Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiyoonie9 left a comment

Choose a reason for hiding this comment

decentralgabe commented Mar 13, 2024

decentralgabe commented Mar 6, 2024 •

edited

Loading

frankhinek Mar 7, 2024 •

edited

Loading

KendallWeihe Mar 7, 2024 •

edited

Loading

nitro-neal Mar 7, 2024 •

edited

Loading

jiyoonie9 Mar 13, 2024 •

edited

Loading