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

Update Verification Method Rotation section. #741

Merged
merged 2 commits into from
Jun 27, 2021
Merged

Conversation

msporny
Copy link
Member

@msporny msporny commented May 16, 2021

Partial editorial cleanup to appendices tracked as issue #728.


Preview | Diff

@msporny msporny added cr-comment editorial Editors should update the spec then close labels May 16, 2021
@msporny msporny requested a review from dhh1128 May 16, 2021 20:19
index.html Outdated
<a>Verification method</a> rotation is a proactive security measure.
</li>
<li>
It is considered a best practice to perform <a>verification method</a> rotation
Copy link
Contributor

@dhh1128 dhh1128 May 18, 2021

Choose a reason for hiding this comment

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

Can we insert the hedge word "generally" here: "It is generally considered..."

I agree that this is a true generalization -- but I'm feeling like I want some nuance. See https://www.strongsalt.com/encryption-key-rotation-is-useless%E2%80%8A-%E2%80%8Aheres-why/ for an explanation of why not everyone agrees that this is a good idea. (In short: the more rotation, the more chances for human error and key loss.)

index.html Outdated
rotation.
</li>
<li>
<a>Verification method</a> rotation applies only to the current or latest
Copy link
Contributor

Choose a reason for hiding this comment

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

I think "applies" is vague. Better might be: "Verification method rotation manifests only as changes to the current..." or similar.

parties that are forced to continuously renew or refresh associated credentials.
</li>
<li>
Proofs or signatures that rely on <a>verification methods</a> that are not
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels at odds with verbiage in the revised revocation section, which says that omitting a verification method from a DID doc is the only universally supported method of revoking something. How do we square the two statements?

Copy link
Member Author

@msporny msporny May 29, 2021

Choose a reason for hiding this comment

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

I don't see how it's at odds. It feels aligned. Can you provide some concrete specification text that we could change this to?

Any suggestions @OR13, since it's your language, IIRC?

Copy link
Contributor

Choose a reason for hiding this comment

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

What I'm trying to point out here is that "verification methods that are not present in the latest version of a DID document" are required, in the revocation section, to be interpreted as revoked methods (if they ever existed at all). Thus, this sentence could be reworded to "Verification methods that are already revoked cannot be rotated."

<li>
A <a>controller</a> performs a rotation when they add a new <a>verification
method</a> that is meant to replace an existing <a>verification method</a> after
some time.
Copy link
Contributor

@dhh1128 dhh1128 May 18, 2021

Choose a reason for hiding this comment

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

Is rotation meant to communicate an intention to replace exactly at the moment of the rotation -- or at some indefinite point in the future? If the former interpretation is canonical, then isn't revocation of the old method automatic? If the latter, then how is the future moment determined -- when a separate revocation step is taken on the old method?

Copy link
Member Author

Choose a reason for hiding this comment

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

My expectation is that it's "at some indefinite point in the future"... which might be within microseconds of each other. The process, as I understand it might be:

  1. Add replacement key (k2).
  2. Remove old key (k1).

So, it can't be considered automatic all of the time (but perhaps, sometimes). Yes, the future moment could be thought of as when the old key is revoked.

@dhh1128 please take a shot at some concrete spec text, or @OR13 if you have any suggestions here, that would be great. We need that text before I can merge.

Copy link
Contributor

@dhh1128 dhh1128 Jun 4, 2021

Choose a reason for hiding this comment

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

If I understand correctly, under the definition you're advocating here, the only difference between "inserting" a new key and "rotating" a key is intention; there is no observable difference in DID doc state when the two operations complete. That is, only step 1 in your process is "rotation"; step 2 is "revocation" -- right? This feels weird to me. I thought "rotation" was the combination of adding a new key and removing an old one, as an atomic operation.

Copy link
Contributor

@dhh1128 dhh1128 Jun 4, 2021

Choose a reason for hiding this comment

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

The reason I'm not suggesting spec text here is because I think the definition is either confused or fundamentally wrong. If rotation is defined to mean "insert a key with the intention to revoke its older counterpart" -- but the actual revoking isn't part of rotation, only a difference of intention, then most of the benefits of "rotation" vanish, since it is the fulfilled intention of revocation, not the initial intention of rotating, that decreases the attack surface. If, on the other hand, we define rotation to mean "insert a key AND revoke its older counterpart", then saying the revocation is separable makes no sense, and most of the security benefits should appear in the revocation section, with only a note here about how rotation picks up those benefits since it includes revocation.

The introductory text "Rotation...enables the secret cryptographic material associated with an existing verification method to be deactivated or destroyed once a new verification method has been added to the DIDdocument" is trying to combine the semantics of the insertion and the revocation. This seems at odds with "it can't be considered automatic all of the time."

Copy link
Member Author

@msporny msporny Jun 8, 2021

Choose a reason for hiding this comment

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

I don't have strong opinions wrt. this PR. I was merely cleaning up text that was already there when I stepped on this landmine.

I believe other people submitted this text initially and I would like them to work through the details here. I remember it being @dhh1128 and @OR13 working on the initial PR here, but can't remember exactly.

I don't necessarily care about the outcome here as long as it's consistent. I do think @dhh1128 is raising good points, so I expect that anything that makes it in would address those concerns. To avoid a "too many cooks in the kitchen" scenario, I'm recusing myself from providing input.

If it helps, I can try to hunt down who initially worked on and refined this section of the specification.

Copy link
Member

@kdenhartog kdenhartog Jun 9, 2021

Choose a reason for hiding this comment

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

Did a quick lookup to save you the time @msporny

The original text was merged via PR #569 with an alternative originally proposed by @OR13 in #548 which was ultimately closed in favor of #569. This was originally intended to address issue #386

It looks like @dhh1128 and @OR13 were able to come to agreement after some back and forth on the editing of this. Hopefully the discussion between those two will provide some additional context on what allowed the current text to be satisfying to all parties at that time.

@TallTed
Copy link
Member

TallTed commented May 18, 2021

I am suddenly very confused.

I have understood Verification Method to be, roughly, "what is done with the crypto material", as opposed to "the crypto material" itself.

Rotating crypto material makes sense, for all the reasons discussed in this section. But rotating "what you do with the crypto material" doesn't make the same kind of sense --- or at least, needs different justifications.

Can anyone help me figure out how and where I have lost the plot?

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

msporny commented May 29, 2021

I am suddenly very confused.

I have understood Verification Method to be, roughly, "what is done with the crypto material", as opposed to "the crypto material" itself.
Can anyone help me figure out how and where I have lost the plot?

We just so happen to have a handy definition of the term here:

https://w3c.github.io/did-core/#dfn-verification-method

... and your summary isn't aligned with it. :)

Hope that helps.

@TallTed
Copy link
Member

TallTed commented May 30, 2021

Yes, @msporny, thank you. I was blurring DID methods with (at least) verification methods, the latter of which could apparently be described as "the crypto materials and what is done with them".

@msporny
Copy link
Member Author

msporny commented Jun 4, 2021

@dhh1128 -- there are action items for you in this PR. If we don't hear from you in 48 hours (which will be 7 days from the time we requested feedback from you), we're going to merge this editorial PR. We made the concrete changes you requested in this PR, and we can always put any other changes you want in a future PR.

@iherman
Copy link
Member

iherman commented Jun 8, 2021

The issue was discussed in a meeting on 2021-06-08

  • no resolutions were taken
View the transcript

5.6. Update Verification Method Rotation section

See github pull request #741.

Manu Sporny: might need more people to look
… not sure if that would help
… biggest issue is that it as editorial update and when it was made people are now re discussing the entirety of the text
… daniel hardman has concerns
… orie needs to respond
… it's their text
… but now there's disagreement over the text that they had initially put in there by daniel
… the default is the PR won't go in and the thing will be editorially messy
… I don't know if anyone can chat with daniel hardman and say we need him to propose concrete text and we need orie to talk with him to get there
… I don't have any strong preference one way or the other
… largely editorial stuff
… misunderstanding around text
… need orie and daniel to talk
… that's what will resolve it, floating for 23 days
… neither of them are here
… can anyone take an action to talk with daniel and/or orie to get them to resolve?

Brent Zundel: i can reach out to daniel
… we have completed the agenda in record time

@TallTed
Copy link
Member

TallTed commented Jun 9, 2021

I think I figured out where my earlier confusion really came from:

Rotation...enables the secret cryptographic material associated with an existing verification method to be deactivated or destroyed once a new verification method has been added to the DIDdocument.

I have't quite figured out how to reword this to eliminate the confusion.

Something about the new verification method being comprised of (not merely "associated with") both new secret cryptographic material and new "what is to be done with it", upon addition of which to the DID document, both the old secret cryptographic material and the old "what is to be done with it", which together comprise the old verification method, may (should? must?) be deleted....

@dhh1128
Copy link
Contributor

dhh1128 commented Jun 9, 2021

I don't love this text, but I am publicly clarifying that I don't object to merging it.

@msporny
Copy link
Member Author

msporny commented Jun 27, 2021

Editorial, multiple reviews, some changes requested and made, others not concrete enough to apply, no objections, merging.

@msporny msporny merged commit d619baf into main Jun 27, 2021
@msporny msporny deleted the msporny-sc-rotation branch July 11, 2021 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial Editors should update the spec then close
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants