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

Cleanup and clarification pass over the specification #91

Merged
merged 4 commits into from
Aug 26, 2024

Conversation

swcurran
Copy link
Collaborator

The following is a list of the changes made. Only one change is (arguably) a breaking change (removing the cryptosuite parameter -- it was missed earlier), and I don't think it deserves a new version. I'll have this as draft so that I can do a "refs/defs" check before I turn this over for processing.

  • Adds a definition for the eddsa-jsc-2022 cryptosuite
  • Updates the examples to reference the eddsa-jcs-2022 cryptosuite (just a text change)
  • Adds the implementations list to the header
  • Adds a section to the implementers guide about "High Assurance DIDs with DNS"
  • Adds a section to the implementers guide about "Watchers and Witnesses". I'm not sure the "watchers" part is needed.
  • Clean up of the specification.
    • Fix markdown lint warnings.
    • Changed places where I used "Verify that" into spec language "MUST".
    • Removed duplicate witness bullet point in describing the entries
    • Added/updated references to the hash algorithm verification -- e.g. getting the algorithm from the multihash and verifying the algorithm is one of the permitted values.
    • Removed the cryptosuite parameter. It should have been removed when the hash parameters was removed.
    • Made more precise (without changing!!) some of the processes -- SCID Generation/Verification, EntryHash, prerotation keys, patch handling and a bit on witnesses (more to come on that when a first implementation is done).
    • Updates the entryHash example.

I did not make a version change to the specification, although it's SLIGHTLY arguable --- I removed the cryptosuite parameter. I did note the change in the changelog.

I considered changing the MUST about the parameters to say "any parameter not listed in the spec MUST be ignored", but left it as the previous "There MUST NOT be any parameters not in this list". Should I make that change?

Signed-off-by: Stephen Curran <swcurran@gmail.com>
@swcurran swcurran marked this pull request as draft August 20, 2024 21:43
Signed-off-by: Stephen Curran <swcurran@gmail.com>
@swcurran swcurran marked this pull request as ready for review August 20, 2024 22:14
@swcurran
Copy link
Collaborator Author

I've updated the "refs" for the content, so this PR is now ready for review. Give it a look!

@swcurran
Copy link
Collaborator Author

@martipos and @bj-ms -- you can't officially provide a review that permits merging, but would love to get your review of the changes.

Copy link
Contributor

@brianorwhatever brianorwhatever left a comment

Choose a reason for hiding this comment

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

I think we want to use a proof chain so we have the witnesses signing on top of the DID Controllers signature. To do this we need to add an id to the controller's proof item and use that as previousProof in the witness proofs. (created #92)

spec/implementors_guide.md Outdated Show resolved Hide resolved
spec/implementors_guide.md Outdated Show resolved Hide resolved
log entry]]. Once the [[ref: witness]] has both verified and approved the change, they
express that approval by creating a [[ref: Data Integrity]] proof that is
chained to the [[ref: data integrity]] proof created by the [[ref: DID Controller]], and
send the proof back to the [[ref: DID Controller]]. Once the [[ref: DID Controller]] has a
Copy link
Contributor

Choose a reason for hiding this comment

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

This is where I'm concerned we've added state that isn't necessarily required. When the witness sends the proof back the DID Controller has to have been holding onto the current pending state (in memory, db, file, etc..) to know what to update and publish if correct. I would propose one of the following changes:

  • /.well-known/pending-did.jsonl being the log in it's pending state
  • allowing the DID controller to update their did.jsonl with the pending state as the last entry, and specifying how a resolver would interpret this state (ie fallback to previous version unless a param was included)
  • specifying they need to remember this pending state

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we do the third, and leave it up to an implementation to decide how to hold onto the state. I think it is implied now, but it could be made explicit.

The first option is possible, but I don’t see the need for a DID to HAVE to publish the pending change at a given place. If an implementation separates the DID creation and DID publishing, it means there is extra complexity in the DID publisher between normal and pending DID Logs.

I don’t like the second option because then a resolver (and the DID Publisher) has to know about witnesses, and have special handling around a normal and pending DID Log Entry.

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally don't like the third very much as without standardizing something we limit the possibilities of how we do the "please witness this" procedure. If there is a standard pattern (/.well-known/pending-did.jsonl or just in the did.jsonl) we can have the procedure be more of a quick nudge telling a witness there is a new update to sign instead of requiring the new update be sent in-band. this means if something goes wrong they are sort of stuck in pending state..

I don’t like the second option because then a resolver (and the DID Publisher) has to know about witnesses, and have special handling around a normal and pending DID Log Entry.

They have to know about witnesses no matter what or else they aren't DID method spec compliant.. The special handling wouldn't be special it would be checked every resolution

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dang — that was pretty quick work of my “clever” ideas. You are right on all points about what I said — especially the “special handling” one — doh! This is why meetings are good — my dumb ideas wouldn’t have been more than a passing comment on a recording vs. a set in stone GitHub comment. :-).

Rethinking — if we do add this, I prefer the idea of the pending-did.json, as it does standardize how the witnesses get access to the change. It doesn’t cover the reverse — how the the witness convey their proofs. Any thoughts on that?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's exactly where I've gotten to with playing with the code.. For this proof of concept I am actually just doing it all in-band (responding to a POST request to /witness with the proof) which means the witness needs to be online and an update is going to be pretty slow.. so I've already invented an endpoint.. maybe it could be a dual purpose endpoint? Let's leave this as is for now and can make these changes in the next revision after lessons learned as you mentioned

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. Which is why it was a good idea for you to put this into an issue, and dumb of me to not put my reply into that issue :-(. Oh well, we’ll get there! Keep going with the implementation and let’s see where we get!

spec/implementors_guide.md Outdated Show resolved Hide resolved
spec/specification.md Outdated Show resolved Hide resolved
spec/specification.md Outdated Show resolved Hide resolved
spec/specification.md Show resolved Hide resolved
@brianorwhatever
Copy link
Contributor

I might be wrong but I think the [[ref: log entries]] don't have a matching def. I think a find and replace to [[ref: DID Log Entries]] is required or better yet add log entries to that def

@swcurran
Copy link
Collaborator Author

Great stuff, @brianorwhatever — thanks for doing that. I’ll make the updates before merging. Looking forward to hearing what you learn about witnesses.

Signed-off-by: Stephen Curran <swcurran@gmail.com>
Copy link
Contributor

@martipos martipos left a comment

Choose a reason for hiding this comment

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

Thanks for the clarification and pass over! I didn't find any major discussion points and only added some minor changes mainly with focus on readability and refs, which is rather subjective. Hence, feel free to go with your own subjectivity and rejecting them. Either way, please take a look at line 224 (implementors_guide) as I am not sure I correctly understood what was meant in the first place, hence, potentially resulting in a wrong suggestion.

Signed-off-by: martipos 176692840+martipos@users.noreply.github.com

spec/implementors_guide.md Outdated Show resolved Hide resolved
spec/definitions.md Outdated Show resolved Hide resolved
spec/implementors_guide.md Outdated Show resolved Hide resolved
spec/implementors_guide.md Outdated Show resolved Hide resolved
spec/implementors_guide.md Outdated Show resolved Hide resolved
spec/implementors_guide.md Outdated Show resolved Hide resolved
spec/implementors_guide.md Outdated Show resolved Hide resolved
spec/definitions.md Outdated Show resolved Hide resolved
Signed-off-by: Stephen Curran <swcurran@gmail.com>
@swcurran
Copy link
Collaborator Author

Thanks @martipos — much appreciated. I made all the changes proposed except for leaving the “decentralized trust space” vs. “ecosystem”.

@swcurran swcurran merged commit a4070f3 into decentralized-identity:main Aug 26, 2024
1 check passed
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.

3 participants