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

Simplify id property via separation of deterministic equivalence functionality #431

Closed
wants to merge 32 commits into from

Conversation

csuwildcat
Copy link
Contributor

@csuwildcat csuwildcat commented Oct 7, 2020

We can simplify the main id property language by separating the desired deterministic equivalence functionality into a separate property that more concretely addresses the need to express deterministic equivalence for 1+N variants of the same logical ID.


Preview | Diff

index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@csuwildcat
Copy link
Contributor Author

@msporny this is all related to the same set of issues around equivalence and canonical IDs, and this is the right solution for the need, not what was added to the id property definition.

This property isn't about the controller claiming two separate IDs, which need to bi-directionally confirm each other, are controlled by the same entity, it's one or more identifiers that are deterministic variants/aliases of the identifier that was resolved, which don't require any such bi-directional willing/subjective confirmation, because they're just "manifestations of the same thing*. There's literally 0 security vulnerability, in fact, to the contrary: the only values the field can be populated with are deterministic variants/aliases of the identifier that was resolved, which the method can guarantee are correct.

Here's an analogy: if I handed you a SHA2 and SHA3 hash of the same data, there's no subjective claim being made - they're deterministic equivalents/aliases to the same thing.

@csuwildcat
Copy link
Contributor Author

csuwildcat commented Oct 8, 2020

If we want to change the language of alsoKnownAs to only allow deterministic exact variants of the resolved identifier that can be trusted to be the same without any subject bi-directional hoops, as well as have the many-to-one handling directives of this PR, I'm game, but that will blow up their ability to throw in random, insecure assertions, so I'm guessing they won't want that. Multiple methods would benefit from this, and the id property can use simplification, so this is a win-win imo.

@OR13
Copy link
Contributor

OR13 commented Oct 8, 2020

I'd prefer OWL, sameAs :) for this.

what we want is the following properties

  • a list of identifiers that are to be treated the same
  • an assertion that this list can ONLY be set by the did method
  • a recommendation that RPs retain all of them

I would prefer sameAs with additional restrictions that get us these properties, but I am generally supportive of the idea behind this.

@troyronda
Copy link
Contributor

troyronda commented Oct 8, 2020

@OR13 I would add to that list to achieve the canonical DID capability: semantics for the preferred identifier to use.

@csuwildcat
Copy link
Contributor Author

@troyronda I don't think we should overload sameAs with some some sort of canonical preference semantic, because a Subject may not have one. I believe a separate, singular canonical/preferred reference for that equivalence need is a better idea. Can you open a separate PR for that?

@csuwildcat
Copy link
Contributor Author

One note about @OR13's second bullet: "an assertion that this list can ONLY be set by the did method" - and must ensure the IDs are securely, deterministically equivalent to populate them.

@OR13
Copy link
Contributor

OR13 commented Oct 9, 2020

and must ensure the IDs are securely, deterministically equivalent to populate them.

this is not testable, and I suggest not trying to add it.

or rather, a test for it would be method specific.... and so not suitable for did core... unless we mandated exactly how everyone must do this, which seems impossible.

index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@peacekeeper
Copy link
Contributor

This would address #421, right?

@tplooker
Copy link
Contributor

This would address #421, right?

Yes this is written to address this issue

@tplooker
Copy link
Contributor

+1 to the direction of this PR, the current language around the id property is problematic as documented in #421 but the behaviour it is attempting to enable with did's I think is sufficiently generalised and therefore a seperate property should be created.

I'd prefer OWL, sameAs :) for this.

As an alternative proposal, I don't disagree with this one, we would essentially be taking an existing property and applying the required constraints to its usage in a did document.

@troyronda
Copy link
Contributor

troyronda commented Oct 12, 2020

This would address #421, right?

@peacekeeper @tplooker This PR addresses the assertion of DID equivalence. However this PR does not address the canonical/preferred DID aspect. Both of these aspects are useful.

Copy link
Contributor

@OR13 OR13 left a comment

Choose a reason for hiding this comment

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

Not opposed to the idea of this, but definitely opposed to the current structure.

Prefer to see:

deterministicSameAs -> sameAs

Add a statement saying the only difference between DID Document representation for sameAs value should be controller and id

Add a statement saying the property can only be set by the did method, never the did controller.

Remove all normative statements burdening the RP.

@msporny
Copy link
Member

msporny commented Oct 26, 2020

Waiting for PR authors to respond to @OR13's requests. Also, merge conflicts.

@csuwildcat
Copy link
Contributor Author

csuwildcat commented Oct 26, 2020

@orie if there is no normative language that tells the RP these IDs are exactly equivalent synonyms that they must treat as such, it's pretty useless. I don't care about the name, but users of methods that can represent, associate, or otherwise tie DIDs together should have a clean way for RPs to be aware of this and interact with them.

@csuwildcat
Copy link
Contributor Author

@msporny seems like we may need a special topic call to get to the bottom of how to address any and all situations where there is a deterministically secure one-to-many equivalence.

csuwildcat and others added 11 commits November 30, 2020 15:01
Co-authored-by: Tobias Looker <tplooker@gmail.com>
Co-authored-by: Tobias Looker <tplooker@gmail.com>
Co-authored-by: Tobias Looker <tplooker@gmail.com>
Co-authored-by: Tobias Looker <tplooker@gmail.com>
Co-authored-by: Tobias Looker <tplooker@gmail.com>
Co-authored-by: Tobias Looker <tplooker@gmail.com>
Co-authored-by: Tobias Looker <tplooker@gmail.com>
Co-authored-by: Tobias Looker <tplooker@gmail.com>
Co-authored-by: Tobias Looker <tplooker@gmail.com>
Co-authored-by: Tobias Looker <tplooker@gmail.com>
Co-authored-by: Tobias Looker <tplooker@gmail.com>
index.html Outdated
Comment on lines 3553 to 3557
A resolving party MUST retain the values from the <code>id</code> and
<code><a>equivalentId</a></code> properties to ensure any subsequent
interactions with any of the values they contain are correctly handled
as logically equivalent (e.g. retain all variants in a database so an
interaction with any one maps to the same underlying account).
Copy link
Member

Choose a reason for hiding this comment

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

All of these statements are not testable -- we don't have conformance criteria for a "resolving party" in the spec. We probably don't want to slide down that slippery slope.

I'm fine w/ pulling in this PR as long as we remove the MUST here (and in each subsequent section).

We may not even need it -- given that this information is provided in the did document meta or resolver meta, the language probably belongs in those specifications. We can signal what we expect RPs to do here, but I don't think we can mandate any particular behavior.

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'm fine w/ merging this modulo the removal of the normative statements wrt. conformance criteria for "resolving parties" in the spec. We probably don't want to slide down that slippery slope.

@csuwildcat
Copy link
Contributor Author

csuwildcat commented Dec 1, 2020 via email

@OR13
Copy link
Contributor

OR13 commented Dec 1, 2020

does an implementation properly resolve a credential against the equivalent DIDs correctly mapped to each other - do they recognize them as representations of the same logical entity.

DID core does not have anything to do with credentials.... I'm not sure this is testable in the VC Data Model either, how is "do they recognize them as representations of the same logical entity." testable?

I can see writing tests of the form:

does resolve long form, return meta data that references short form... but I can't see a way for did core to prove the behavior of an RP in a test... I would be ok merging this as is assuming Microsoft handles writing the tests associated with these properties, and agrees to remove the normative requirements assuming they can't actually be tested :)

It seems a bit unfair to claim something is testable but then refuse to write any tests.

here are tests that prove that you can issue from 2 different identifiers, https://github.com/transmute-industries/vc.js/blob/master/packages/vc.js/src/vc-ld/__tests__/long-form-did.sanity.test.ts

how can these be modified to prove that the RP treated them as the same?

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

msporny commented Dec 1, 2020

@csuwildcat -- I added change suggestions that align with what we agreed to on the DID WG call today. The only items remaining for you are:

  1. Fix the merge conflicts by rebasing.
  2. Clean up the commit history by selectively collapsing items in the history that should be collapsed... like all the ~11 "Update index.html" commits. Try to keep attribution when you do that (by keeping the Github "Authored-By" comments in commit.

The second item is optional, if you don't do that, I'll just do one big squash-merge (but don't want to do that because it'll destroy who contributed what and when).

csuwildcat and others added 4 commits December 3, 2020 13:04
Co-authored-by: Manu Sporny <msporny@digitalbazaar.com>
Co-authored-by: Manu Sporny <msporny@digitalbazaar.com>
@csuwildcat
Copy link
Contributor Author

@msporny I think I merged the conflicting changes correctly, but don't feel great about my ability to do your second point without obliterating something with my sub-par Git skills 😬

@msporny
Copy link
Member

msporny commented Dec 3, 2020

@msporny I think I merged the conflicting changes correctly, but don't feel great about my ability to do your second point without obliterating something with my sub-par Git skills.

Two options 1) Give me write access to your did-core github repo where you're doing this PR from, or 2) I can pull your branch, rewrite history, ensure everything lines up and then merge that PR (which will be different from, but equivalent to this PR) and then close this PR (once that PR is merged).

The second option requires less coordination. The first option allows me to fix this PR and then merge this PR (as long as you give me push/force-push access to your repo). The end result will be the same commits and same changes to the spec, so your call @csuwildcat... that is, unless I screw up the PR. :P

Let me try the 2nd option and see where I get... and then you can decide if you want to force push that one onto this PR.

Copy link
Contributor

@peacekeeper peacekeeper left a comment

Choose a reason for hiding this comment

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

Looks good..

We may want to change some terminology later after this is merged (e.g. use "requesting party" instead of "resolving party", see #248).

@msporny
Copy link
Member

msporny commented Dec 6, 2020

Merged as PR #473. Closing.

@msporny msporny closed this Dec 6, 2020
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.