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

Updates re controller property #116

Merged
merged 34 commits into from
Nov 23, 2024
Merged

Conversation

jandrieu
Copy link
Contributor

@jandrieu jandrieu commented Oct 31, 2024

Addresses #33 and #75


💥 Error: 500 Internal Server Error 💥

PR Preview failed to build. (Last tried on Nov 23, 2024, 4:01 PM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Spec Generator - Spec Generator is the web service used to build specs that rely on ReSpec.

🔗 [Related URL]([object Object])

Timed out after waiting 30000ms

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

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
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
Copy link
Member

@iherman iherman left a comment

Choose a reason for hiding this comment

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

General comment: there is a terminological issue in all the additions. The term "document's base identifier" and the "document's canonical URL" are used interchangeably., but none of the two terms are formally defined in the spec. My impression is that these terms do not really have a reasonable meaning beyond referring to the document's identifier (i.e., value of the id property). I would prefer to drop all occurrences.

index.html Outdated Show resolved Hide resolved
index.html Outdated
Comment on lines 593 to 597
It is expected that the subject referred to by the id of a controller document
be consistent over time, so that a set of credentials with a given identifer as
subject can be taken to be about the same entity. However, it *is* possible to
issue credentials with a subject identifier _without_ the subject even being
aware of it.
Copy link
Member

Choose a reason for hiding this comment

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

Note the repeated usage of "However" in the note which does not sound good to my ears...

index.html Outdated
Comment on lines 593 to 597
It is expected that the subject referred to by the id of a controller document
be consistent over time, so that a set of credentials with a given identifer as
subject can be taken to be about the same entity. However, it *is* possible to
issue credentials with a subject identifier _without_ the subject even being
aware of it.
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure the "however" part should be removed. @TallTed is right that it is not a "however"; I could imagine something like:

subject can be taken to be about the same entity, although it is...

index.html Outdated Show resolved Hide resolved
index.html Outdated
However, the White House meant "President Joe Biden", who is a physical person who
happened to be in office at the time the credential was issued. In contrast,
the park service meant "the current president, aka POTUS, whoever he is"; this second VC is not
about the Joe Biden as a human individual, it's about the role of President of
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to explicitly refer to Joe Biden under the current circumstances? Just saying... 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm open to a different story. Maybe a "spouse of Kelly Smith" could work.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest we remove this example because it does not clarify anything.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, this amount of exposition belongs in a Security Considerations section, not in a terminology section. The terminology definitions are, ideally, 1-3 sentences at most.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll move it to security considerations

Copy link
Contributor Author

@jandrieu jandrieu Nov 19, 2024

Choose a reason for hiding this comment

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

Moved and restated to refer to a teacher in an ambiguous manner.

index.html Outdated
controller document's verification methods are taken as cryptographic
assurance that the controller of the identifier created those proofs.
</p>
<p class="note" title="Identifier Controller v Document Controller">
Copy link
Member

Choose a reason for hiding this comment

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

Ah. I just realize that there is a whole note about document vs. identifier control. See may comment above: I am not sure if I agree with all this; in my view, the controller controls the document, the identifier may be provided by storage system and may not be under the control of the controller.

My proposal would be to remove that note altogether, and not touch the subject of the identifier control.

(From RDF point of view, which is underlying this model, the two resources that have two different URL-s are strictly different resources. The @id property of JSON-LD looks like a property like any other, but it is not: it is the way to "create" a resource with that given identifier.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. I just realize that there is a whole note about document vs. identifier control. See may comment above: I am not sure if I agree with all this; in my view, the controller controls the document, the identifier may be provided by storage system and may not be under the control of the controller.

This is the reason controller documents for non-DIDs is an exercise is rounding a square peg. IMO, the controller document loses so much of the cryptographic properties of good DID methods as to be practically useless. HOWEVER, it was in the interest of the "big tent" that we are attempting to cut out everything from DID Documents that makes them DIDly, yet somehow keep their value for non-DID URLs. But that just leaves you with the exact uncertainty that you raise: the resolving identifier of a non-DID URL has no guarantees to be related to the content of the controller document.

And yet, here we are.

My proposal would be to remove that note altogether, and not touch the subject of the identifier control.

My proposal would be to revert the VC Data Model to use the DID document specification instead of trying to force an innovation designed for decentralized systems to also make sense when used without the decentralizing part.

(From RDF point of view, which is underlying this model, the two resources that have two different URL-s are strictly different resources. The @id property of JSON-LD looks like a property like any other, but it is not: it is the way to "create" a resource with that given identifier.)

Sure, but that is exactly the root problem here. RDF does not allow you to make statements about identifiers, but that is precisely what we are doing with controller documents. My proposal would be to treat the controller document as JSON and define the properties to more clearly describe aspects of the identifier itself rather than imagining somehow that the subject is somehow involved. Instead, we'll probably continue to bump into the HttpRange14 error of thinking RDF is a coherent semantic space, with a weird caveat of "except for identifiers".

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
Comment on lines 334 to 336
A [=controller document=] specifies one or more
[=verification relationships=] and [=services=] for a single,
identifier, for which the current controller document is taken as authoritative.
Copy link
Contributor

@wip-abramson wip-abramson Nov 4, 2024

Choose a reason for hiding this comment

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

The end of this sentence does not flow well for me. Perhaps we could reorder.

Suggested change
A [=controller document=] specifies one or more
[=verification relationships=] and [=services=] for a single,
identifier, for which the current controller document is taken as authoritative.
A [=controller document=] is taken as authoritative over a single identifier for which it specifies one or more
[=verification relationships=] and [=service endpoints=].

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is incorrect. It should be zero or more service endpoints.

Copy link
Member

Choose a reason for hiding this comment

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

Lightly tweaking @wip-abramson's suggestion, incorporating @selfissued's comment

Suggested change
A [=controller document=] specifies one or more
[=verification relationships=] and [=services=] for a single,
identifier, for which the current controller document is taken as authoritative.
A [=controller document=] is taken as authoritative over a single identifier
for which it specifies one or more [=verification relationships=] and
zero or more [=service endpoints=].

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is outdated, but please review to see if the current language works for you @wip-abramson

jandrieu and others added 10 commits November 12, 2024 07:50
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ivan Herman <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ivan Herman <[email protected]>
Co-authored-by: Ivan Herman <[email protected]>
index.html Outdated Show resolved Hide resolved
@David-Chadwick
Copy link
Contributor

Can you add me as a reviewer please. Thanks

@TallTed
Copy link
Member

TallTed commented Nov 12, 2024

@David-Chadwick — You should generally be able to just go here and look to the big green "Review changes" button near the upper right corner. Note that you are already listed with the other Reviewers at upper right of this page. And I've now clicked the button to "Re-request review".

index.html Show resolved Hide resolved
such as update a [=controller document=] or use a cryptographic key to generate
a digital signature.
<p>
An entity that is capable of performing an action with a specific resource,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
An entity that is capable of performing an action with a specific resource,
An entity that is capable of performing an action with a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This suggestion would leave a broken sentence.

Can you try a new suggestion that addresses the flow of the remaining sentence?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what happened to my original proposal, but here is the revised one from PR#126

An entity that is [=authorized=] to perform any action on the associated resource such as updating the associated [=controller document=] or updating the associated [=verification method=].

index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated
It is expected that credential issuers will ask the subject for identifiers
and that those subjects would demonstrate proof of control before the issuer
issues the credential. However, there are valid cases where issuers will
use identifiers that are not provably under the control of the subject.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
use identifiers that are not provably under the control of the subject.
use identifiers that are not provably under the control of the subject, for example,
when a parent requests a verifiable credential for their child.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@David-Chadwick Marking this resolved, but please re-review to see if the updated language is good for you.

index.html 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
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Copy link
Contributor

@David-Chadwick David-Chadwick left a comment

Choose a reason for hiding this comment

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

Joe, I have removed my suggestions to have two different controller terms, because my revised definition of controller makes it quite clear to which object the controller refers.

such as update a [=controller document=] or use a cryptographic key to generate
a digital signature.
<p>
An entity that is capable of performing an action with a specific resource,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what happened to my original proposal, but here is the revised one from PR#126

An entity that is [=authorized=] to perform any action on the associated resource such as updating the associated [=controller document=] or updating the associated [=verification method=].

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Copy link
Member

@brentzundel brentzundel left a comment

Choose a reason for hiding this comment

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

In general the changes here are fine to me, and I appreciate the work that has gone into shaping the PR and incorporating the plentiful feedback.

The one addition that doesn't sit right with me is the reference to DID Core. Even though it is informative, it strikes me as odd that this document would point downstream and say what another document is probably going to do in profiling it. I could approve if those lines were removed.

Copy link
Collaborator

@selfissued selfissued left a comment

Choose a reason for hiding this comment

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

This probably changes more text than necessary, but I believe it's headed in the right direction.

@TallTed
Copy link
Member

TallTed commented Nov 22, 2024

Please merge my markup suggestions starting here, so that I can review the text.

jandrieu and others added 3 commits November 22, 2024 10:54
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
@jandrieu
Copy link
Contributor Author

In general the changes here are fine to me, and I appreciate the work that has gone into shaping the PR and incorporating the plentiful feedback.

The one addition that doesn't sit right with me is the reference to DID Core. Even though it is informative, it strikes me as odd that this document would point downstream and say what another document is probably going to do in profiling it. I could approve if those lines were removed.

If others agree, I don't mind removing it. However, language about profiling by DID Core was already in the document, as, I believe, a partial response to the TAG's inquiry about why do we need this spec at all instead of just referring to the DID Document specification.

@jandrieu
Copy link
Contributor Author

Please merge my markup suggestions starting here, so that I can review the text.

Done. Thank you for the clean up.

@jandrieu
Copy link
Contributor Author

@David-Chadwick I'm trying to incorporate your comment here: #116 (comment)

Not sure what happened to my original proposal, but here is the revised one from PR#126

Thanks for resurrecting that. With the number of changes, I wasn't able to incorporate your edits at the same time I adopted others.

An entity that is [=authorized=] to perform any action on the associated resource such as updating the associated [=controller document=] or updating the associated [=verification method=].

Either I don't understand your suggestion or I think there is still a miscommunication. The controller of a verification method cannot update the verification method in the controller document. They CAN create proofs suitable for that method, but they don't (necessarily) have the ability to update the "associated verification method".

Copy link
Member

@TallTed TallTed left a comment

Choose a reason for hiding this comment

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

More may follow after these are merged...

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
Comment on lines 610 to 611
[=identifier=] for any given purpose. See the section on Identifier Ambiguity
in Security Considerations.
Copy link
Member

Choose a reason for hiding this comment

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

Needs a citation/link to that Identifier Ambiguity in Security Considerations section.

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
@David-Chadwick
Copy link
Contributor

@jandrieu You said " The controller of a verification method cannot update the verification method in the controller document." I thought that we had agreed at the last meeting that this controller could update the verification method, e.g. by saying that the identifier belonged to a person named David Chadwick, whose address is in the UK. This is why I suggested adding the sentence "Obviously the controller cannot update the identifier..." which is true for all controllers of all objects (because it becomes a different object once you do that)

@msporny
Copy link
Member

msporny commented Nov 23, 2024

If others agree, I don't mind removing it. However, language about profiling by DID Core was already in the document, as, I believe, a partial response to the TAG's inquiry about why do we need this spec at all instead of just referring to the DID Document specification.

Yes, that's why the text was added. I agree that the up-ref is weird, but at the same time, I don't know how else to provide a concrete example. If the concern is the ref in the informative references section, I could just use a hyperlink instead? @brentzundel what would you like to happen here given that the text you'd like to remove was put in place to address a concern from the TAG. /cc @jyasskin @hadleybeeman

@msporny
Copy link
Member

msporny commented Nov 23, 2024

I thought that we had agreed at the last meeting that this controller could update the verification method

No, we definitely did not agree to that and here's why:

Sometimes, when the controller is both the controller of the controller document A and the verification method and both things are specified in the controller document A, then it is true that the controller can update both.

However, when a verification method is listed in controller document A, but the verification method's controller is a different identifier that lives in a different controller document B, the controller of A cannot necessarily update the verification method in document B. It is true that they can change the information in the verification method listed in controller document A, but then it ceases to be the verification method listed in controller document B (and will trigger a verification method mismatch for the same verification method identifier in document A and document B (per the verification method retrieval algorithm).

In other words, if I list your public key as being able to perform authentication in my controller document, you can authenticate as me, but I don't have any control over updating your public key.

Thanks, Ted!

Co-authored-by: Ted Thibodeau Jr <[email protected]>
index.html Outdated Show resolved Hide resolved
@msporny
Copy link
Member

msporny commented Nov 23, 2024

Yes, that's why the text was added. I agree that the up-ref is weird, but at the same time, I don't know how else to provide a concrete example. If the concern is the ref in the informative references section, I could just use a hyperlink instead? @brentzundel what would you like to happen here given that the text you'd like to remove was put in place to address a concern from the TAG. /cc @jyasskin @hadleybeeman

I'm going to merge this PR and try to address @brentzundel's desire in another commit (since I don't have the ability to change this PR). I need to do a lot of merges on the merge queue today and I need this PR to get in there so the other PRs don't further delay this PR. It looks like @jandrieu has addressed all the comments that were raised, I'll address @brentzundel's comment in a future commit (and refer to it in this thread), and any other disagreement seems to be either editorial or not having enough consensus to go into this PR.

@msporny
Copy link
Member

msporny commented Nov 23, 2024

Normative, multiple reviews, changes requested and made, no objections, remaining requests can go in a separate PR, merging.

@msporny msporny merged commit 817bf47 into w3c:main Nov 23, 2024
1 check passed
@msporny
Copy link
Member

msporny commented Nov 23, 2024

@brentzundel I issue markered your concern in this commit: 60a13f6

You can see it here:

https://w3c.github.io/controller-document/#introduction

@David-Chadwick
Copy link
Contributor

@msporny . I think you misunderstood what I was saying, as I was not saying that you could update my public key. To put it another way, if you list my public key as a verification method in your controller document A, I was saying that I can change the properties of my public key by adding my name and address to it, since I am the controller of this. Are you disagreeing with this?

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.

9 participants