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.

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

Add support for DIDs #144

Merged
merged 55 commits into from
Sep 27, 2023
Merged

Add support for DIDs #144

merged 55 commits into from
Sep 27, 2023

Conversation

OR13
Copy link
Contributor

@OR13 OR13 commented Aug 25, 2023

  • This PR starts with a copy of what is in vc-data-integrity proofs.
  • There are lots of details that are not related to W3C Verifiable Credentials.
  • The guidance on private information in key material is insufficient.
  • The document encourages industry fragmentation by not providing recommendations that align with industry adopted standards.
  • The document lacks examples that address issues with unregistered media types.
  • We will need to address reference issues for terms defined in DID Core vis respec / x-ref.
  • We will need to adjust the "number of contexts" issues associated with controller documents that are verifiable credentials. (This has to be fixed in vc-data-model, in order for examples to be valid).

Preview | Diff

index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@OR13 OR13 requested a review from TallTed August 26, 2023 12:55
@OR13

This comment was marked as resolved.

Copy link
Member

@msporny msporny left a comment

Choose a reason for hiding this comment

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

I'd like to merge this PR as it is, I believe I have captured issue markers for the discussions that are not resolved.

It looks like this PR is attempting to rewrite how DID Documents work, it has certainly removed large sections of content that exist in the DID Core specification. None of the issues raised (AFAICT) are marked as before-CR, and that's problematic given the modification to DID Core concepts. We need to understand if this PR has WG consensus, I doubt it does, as I don't think people in the WG would be ok w/ modifying DID Core in a way that goes outside of the bounds of our WG Charter.

Concrete change requests: State that what is done in these sections is a subclass'ing of what DID Core and/or Data Integrity specifies and highlight the differences. Duplicating content across multiple specifications is highly problematic, ensure that people understand why certain sections have been duplicated.

@OR13
Copy link
Contributor Author

OR13 commented Sep 14, 2023

@msporny given that controller documents are not specific to DIDs or blockchains, would changing all the examples to use HTTPS URLs address your concerns?

Part of the challenge in referring to DID Core is that there is nothing in DID Core that actually enables interoperable discovery of key material, which is what we need to enable interoperable verifiable credentials... and this led to formal objections.

Data integrity redefines DID Core here:

And attempts to address this interoperable key discover challenge here:

If the problem is truly related to DIDs, I suggest the following options:

  1. Don't refer to DIDs at all in this document, only in data integrity (basically abandon this PR, and remove examples related to DIDs)
  2. Don't duplicate DID Core in either work item, but include informative examples, with no normative requirements (this section is already informative, this would require changes to data integrity)
  3. Address retrieving key material in the context of conventions associated with the security envelope (allow data integrity and vc-jose-cose to make recommendations regarding controller documents that lead to interoperability for the credential format, but that don't require interoperability at the controller document layer.

We are currently attempting path 3.

Concrete change requests: State that what is done in these sections is a subclass'ing of what DID Core and/or Data Integrity specifies and highlight the differences. Duplicating content across multiple specifications is highly problematic, ensure that people understand why certain sections have been duplicated.

I can't implement this suggestion because there is no "subclass'ing" happening here.

Controller documents have no normative concept of "type", and DID Documents are not always interpreted as JSON-LD, so the RDF class concept does not apply.... Perhaps you are asking for us to clarify that this section only applies to application/did+json ?

Can you comment on if you feel that 1 or 2 might be easier to achieve consensus on?

@msporny
Copy link
Member

msporny commented Sep 14, 2023

Can you comment on if you feel that 1 or 2 might be easier to achieve consensus on?

We were able to find a path forward at the VCWG F2F meeting at TPAC, I suggest reviewing the minutes to understand the path forward here.

@iherman
Copy link
Member

iherman commented Sep 15, 2023

The issue was discussed in a meeting on 2023-09-14

  • no resolutions were taken
View the transcript

6.1. Add support for DIDs (pr vc-jose-cose#144)

See github pull request vc-jose-cose#144.

Michael Jones: very discussed PR to-date. some requested changes - many addressed. manu is the person requesting changes now.

Joe Andrieu: could we get the github URL again?

Manu Sporny: one of the concerns are it duplicates content from DID-Core and data integrity specification and rewrites some of it. uses normative statements from did-core and puts DI content on top of it. DI spec is clear on the reason.
� since this PR is about DIDs to the specification, adding a normative dependency on DID-Core makes sense.
� request is to make clear that the content is largely copy past from other 2 documents, explain what is being changed from those text and why.
� or just copy paste.
� redefining DID-Core might be out of scope of the charter.

Michael Jones: normative change to DID-Core. subsetting normative statements in DID-Core. if you want to use controller doc with JOSE-COSE, refer to jwks and should not use multibase part, it is not a breaking change to did-core.

Brent Zundel: is it profile of DID controller section of did-core?

Michael Jones: yes.

Brent Zundel: manu, does clarifying that alleviate your concerns?

Manu Sporny: no, because the profile makes normative changes to did-core.
� if we were just to subset, putting a normative reference to did-core and clearly state what subsetting is.

Benjamin Young: +1 to normative references + subsetting.

Manu Sporny: completely disagree that the PR is ready to be merged today.
� if this is a profile with subsetting - great, but please say so.

Michael Jones: could you state that in the PR?

Manu Sporny: I think I said it in the PR comment.

Kristina Yasuda: looks like we have a way forward.

Ivan Herman: there is currently no normative reference to did-core Rec, which should be from your description.

Michael Jones: i will consult other editors.

@w3c w3c deleted a comment from bob420-svg Sep 16, 2023
@w3c w3c deleted a comment from bob420-svg Sep 16, 2023
index.html Show resolved Hide resolved
@w3c w3c deleted a comment from bob420-svg Sep 17, 2023
@w3c w3c deleted a comment from bob420-svg Sep 17, 2023
@w3c w3c deleted a comment from bob420-svg Sep 17, 2023
@w3c w3c deleted a comment from bob420-svg Sep 17, 2023
@w3c w3c deleted a comment from bob420-svg Sep 17, 2023
@w3c w3c deleted a comment from bob420-svg Sep 17, 2023
@OR13
Copy link
Contributor Author

OR13 commented Sep 18, 2023

@Sakurann @brentzundel can you summarize what actions need to be taken to get this PR merged?

@w3c w3c deleted a comment from bob420-svg Sep 19, 2023
@Sakurann
Copy link
Contributor

Sakurann commented Sep 26, 2023

can you summarize what actions need to be taken to get this PR merged?

@OR13 I think clarifying that this is a "profile" of how DID-Core defines controller documents/DID Docs (as opposed to this PR redefining controller docs) should unblock this PR. This probably means 1/ saying that the basis is how DID-Core defines DID Docs and 2/ clarifying additional requirements that vc-jose-cose puts on top of that (so no need to copy paste the entire DID Doc section).

@Sakurann
Copy link
Contributor

ok, I went back this thread and would like to expand on the comment above.

@msporny please do the same in vc-data-integrity by citing DID-Core and clarifying that vc-data-integrity is profiling it. vc-data-integrity defines controller documents outside DID Core in https://w3c.github.io/vc-data-integrity/#controller-documents and https://w3c.github.io/vc-data-integrity/#retrieve-verification-method. just to quote few statements from vc-data-integrity:

A string value that contains the data necessary to verify the digital proof using the verificationMethod specified. The contents of the value MUST be a [MULTIBASE]-encoded binary value.

A verification method MUST NOT contain multiple verification material properties for the same material. For example, expressing key material in a verification method using both publicKeyJwk and publicKeyMultibase at the same time is prohibited.

we need to have the same approaches across both security specs to prevent confusion.

@msporny
Copy link
Member

msporny commented Sep 26, 2023

@Sakurann wrote:

@msporny please do the same in vc-data-integrity by citing DID-Core and clarifying that vc-data-integrity is profiling it.

Yes, can do.

@w3c w3c deleted a comment from bob420-svg Sep 27, 2023
@Sakurann
Copy link
Contributor

@msporny please do the same in vc-data-integrity by citing DID-Core and clarifying that vc-data-integrity is profiling it.

Yes, can do.

thank you!

@OR13
Copy link
Contributor Author

OR13 commented Sep 27, 2023

need to put in an issue marker, then request a re-review.

@OR13
Copy link
Contributor Author

OR13 commented Sep 27, 2023

I had to remove some text regarding key agreement, and crypto suites to resolve respec issues, in 1ea7469

@OR13
Copy link
Contributor Author

OR13 commented Sep 27, 2023

@msporny can you re-review?

Copy link
Member

@msporny msporny left a comment

Choose a reason for hiding this comment

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

The approval of this PR is contingent on refinements I've made to the description in #160 to align with what I heard on the call. Given that the issue marker added for the controller documents section is not included in this PR, I couldn't suggest text -- so, rather than delaying things further, I've provided a concrete text update in the description in #160 (you can view the diff for these changes in the menu to the top right of the first comment in that issue).

If @OR13 and @selfissued are ok w/ that updated description, I approve this PR. If they are not, I do not approve of this PR (and suggest an alternative, by editing the description directly, so we can move this along).

@bob420-svg

This comment was marked as off-topic.

@selfissued selfissued merged commit 4677a03 into main Sep 27, 2023
1 check passed
@iherman
Copy link
Member

iherman commented Sep 28, 2023

The issue was discussed in a meeting on 2023-09-27

  • no resolutions were taken
View the transcript

1.1. Add support for DIDs (pr vc-jose-cose#144)

See github pull request vc-jose-cose#144.

Kristina Yasuda: We discussed at TPAC.
… vc-jose-cose and vc-di are adding text on how to retrieve the controller verification documents when a credential is being signed using DIDs.
… We've seen discrepancies. As a chair, I'd like to see both specifications citing directly did-core, and clarifying what additional requirements are needed on top of it.
… If editors can agree, cool, let's move on. If not, let's discuss.

Orie Steele: I wasn't in TPAC. Got an update. If the WG consensus is to have DI and vc-jose-cose controller defined documents, then they both say the same things or not.
… You can't simply cite did-core.
… A controller document is different than a did document.
… The former has URLs in it. The latter has DIDs.
… Let's make sure we don't miss that distinction.
… Our specs have URLs, not DIDs with fragments.
… Let's ensure that securing specifications are capable of specifying examples in the spec.
… We need to extend did-core if we're allowing URLs.

Orie Steele: dlongley noted (via remote): a DID document is essentially a subclass of a controller document -- we just had to define these things in reverse order in standards space for a variety of reasons.

Kristina Yasuda: Alternative path forward - don't cite did-core.
… Make it clear it's URLs. I think that's what DI does.
… Then vc-jose-cose should do the same.
… manu are you ok with that?

Manu Sporny: Agree with any direction that doesn't bifurcate what controller documents are.
… If vc-jose-cose subclasses more what DI does, that's fine.
… ONe way is to say which bits are on top of did-core. Another is to repeat a lot of text with specific deviations. We would need to be very careful.

Orie Steele: +1 manu.

Manu Sporny: Summary, fine with the concept. Want to make sure that the message is that both definitions are compatible with one another, just profiling did-core differently.

Orie Steele: manu that sounds good, afaik we have only removed text from data integrity.

Orie Steele: we are already duplicating text.. because URLs are not DID URLs.

Orie Steele: data integrity duplicated text first, vc-jose-cose just copied it, and removed data integrity specific language.

Kristina Yasuda: Base controller doc definition, before any profiling, are there disagreements there?

Orie Steele: I think I agree.
… It's weird to say that VCWG is defining the controller document.
… But as long as everyone's clear, this is an improvement.
… I'm supporting of that framing in language.
… But unsure if we can, or if there's consensus.
… To be clear, I think the section that DI defined is better than what's in did-core. Wouldn't like to see it taken out.
… Are we allowed to do it in the DI spec? If so, and we do it as well in vc-jose-cose, should we move it do VCDM?

Dave Longley: +1 it's weird, yes, but it's an artifact of history and standards process stuff, not technology ... and it was expected to happen.

Michael Jones: "Better than DID Core" :-).

Kristina Yasuda: I'll take an AI to confirm that it's in scope for this WG to be able to define the base controller document.

Orie Steele: turns out that being better than DID Core is a pretty low bar : ?

Orie Steele: +1 to moving the section to the core data model.... if we can.

Dave Longley: DID core and the work in this ecosystem had some pretty big constraints put onto it that had some adverse outcomes :).

Kristina Yasuda: Assuming it's true, any concerns for general controller document definition going to vcdm? And then DI and vc-jose-cose adding on top of it?

Manu Sporny: I don't think it belongs in the VCDM. This is about the securing mechanisms, and doesn't feel like the right place to put it.

Dave Longley: +1 "controller documents" has always been part of the data integrity / LD proofs work -- it makes sense to have it be there.

Manu Sporny: Agreed with everything except that one piece.
… I think it would be ok to duplicate.

Orie Steele: Google folks mentioned wanting to see verification in scope, and i thought i saw possible FO if we don't address it... seems like keeping controller documents in data integrity and duplicating text will open us up to some risk of FO.

Manu Sporny: As background, these concepts used to be part of linked data signature.

Dave Longley: some of this "let's put it in kinda the wrong place" arguments ... are how DID core got to be the way it is :).

Manu Sporny: Just so happened that they were also in did-core.
… But because of chartering reasons, we couldn't do it at the time.

Orie Steele: yeah, i agree dlongley, but sometimes you have to break legs to fix them.

Manu Sporny: So +1 to everything except putting in the VCDM.

Dave Longley: i'm saying DID core got its legs broken to begin with by this sort of thing.

Dmitri Zagidulin: @Orie - Google folks mentioned they'd be fine with it if verification was described in /parallel/ specs that would go to CR at the same time as VC DM 2.0. Which they are.

Dave Longley: Orie: it's fine if other things want to use controller docs that DI defines, those things should refer to that text -- or if those things don't want to just refer for some reason, they can copy the exact same text.

Orie Steele: dlongley yeah, i guess it seems like people don't like refering to data integrity to use controller documents... that seems to be the problem.

Orie Steele: if the definitions are shared, it makes sense to decouple imo.

Michael Jones: Generic comment. If we want something common, it should be in one place. We can create a key discovery spec.
… That's probably better than duplicating the text, which will likely get out of sync.

Manu Sporny: If everyone can agree that we lift the text from DI and put it in a different spec, this would lead into a long drawn out debate.
… And it would put us in a worst position.

selfissue: I hear you, procedurally.
… I was thinking of possible ways of doing so (in the base common doc). And then each spec defined how they're securing it.

Sebastian Crane: If there is going to be objection in a separate spec, is there going to be disagreement between implementers?

Dave Longley: Orie: yeah, one resolution to that would be to compromise and let that go so we can proceed.

Sebastian Crane: Are we kicking the can down the road and doing a disservice to others?

Orie Steele: Json ld signatures were used in a couple of places.
… Still used in many places. It's valuable to put things where they can be shared without bias.
… Controller documents are the grandfathers of dids. We should make it clear that people can refer to that.
… If we can give that to the community, potentially a better outcome.
… More work for us, but a better outcome.

Kristina Yasuda: For now, let's have vc-jose-cose duplicate the text from DI with it's own requirements. That should be the starting point.
… Then I'll consult if we're willing to entertain an additional doc.

Manu Sporny: +1 on kristina's current proposal.

Kristina Yasuda: I'll come back to the WG after.
… Any objections?

Orie Steele: +1.

Michael Jones: +1.

Andres Uribe: +1.

Phillip Long: +1.

Dmitri Zagidulin: +1.

Dave Longley: +1 sounds like an ok way to proceed.

Kristina Yasuda: PR unblocked. Let's move to work item updates.

Michael Jones: Does that mean we can merge 144?

Manu Sporny: It's not just a copy/paste. It doesn't clarify what's being subclassed.

Orie Steele: sounds like the PR is still blocked.

Orie Steele: and there is no consensus on how to proceed, pending discussion on controller documents in the abstract.

Kristina Yasuda: Orie can you comment on the modified parts from DI?

Orie Steele: The PR started with a copy. Then there were many sections that were removed related to DI.
… After several rounds of suggestions, seems like the PR is irrecoverable. It will not achieve consensus.
… That's fine. Best path forward is giving us a single place that both specs can refer to.
… Not sure how to resolve. But the PR is blocked.

Kristina Yasuda: manu, What are the normative changes that were made that you are concerned about?

Michael Jones: +1.

Orie Steele: I can put in an issue marker.

Manu Sporny: That's not the concern. I would suggest to put an issue marker explaining that the WG is attempting to align the definitions between DI and vc-jose-cose.

Kristina Yasuda: Sounds like a way forward.

Michael Jones: Thanks all.

Kristina Yasuda: manu, if you can open an issue where you explain what text you'd like to be better aligned, that would be helpful.

Orie Steele: sounds good!

Orie Steele: PR will be unblocked after issue marker is added.

Kristina Yasuda: Sorry, ivan, some cleanup will need to be done :-).

@decentralgabe decentralgabe deleted the feat/controller-documents branch February 26, 2024 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants