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 rotation and recovery sections from previous PR #569

Merged
merged 13 commits into from
Jan 31, 2021

Conversation

OR13
Copy link
Contributor

@OR13 OR13 commented Jan 23, 2021

Previous PR was corrupted with merge conflicts, this was an easier way for me to get a clean change-set...

All feedback as existing on the previous PR has been implemented (but not accepted)
#548

Specifically @dhh1128 please request any changes on this PR that I failed to address.


Preview | Diff

@OR13 OR13 requested a review from dhh1128 January 23, 2021 15:18
index.html Outdated
time the compromised verification method was registered, to the time it was revoked.
</p>

<p class="note">
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 added this note in an attempt to align how we describe revocation with how the impact of the revocation of the solar winds code signing certificate is described.

@OR13
Copy link
Contributor Author

OR13 commented Jan 23, 2021

@dlongley you left comments but did not request changes previously, if you would not mind either requesting changes or approving the PR, that would help me make sure it meets your needs.

@OR13
Copy link
Contributor Author

OR13 commented Jan 23, 2021

@csuwildcat @selfissued anyone from Microsoft want to add some comments about revocation and why its valuable to did core?

Copy link
Contributor

@dhh1128 dhh1128 left a comment

Choose a reason for hiding this comment

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

This PR essentially ignores all the feedback I gave on the previous PR. It is just as unacceptable to me, and for exactly the same reasons, as the previous PR where I commented in detail and provided alternate verbiage.

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
<p class="advisement">
Cryptographic verifications associated with revoked verification
methods should be considered invalid regardless of when revocation
occurs.
Copy link
Contributor

Choose a reason for hiding this comment

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

@dhh1128, could you make a suggestion here? I presume you'd like this language tweaked to be more clear that verifiers may decide to continue to honor proofs previously made and verified in the past.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, in essence, I think he wants to allow verifiers to ignore that keys have been revoked... which is their prerogative.

OR13 and others added 4 commits January 23, 2021 14:06
Co-authored-by: Dave Longley <dlongley@digitalbazaar.com>
Co-authored-by: Dave Longley <dlongley@digitalbazaar.com>
Co-authored-by: Dave Longley <dlongley@digitalbazaar.com>
Co-authored-by: Dave Longley <dlongley@digitalbazaar.com>
@OR13
Copy link
Contributor Author

OR13 commented Jan 23, 2021

I had a good chat with @dhh1128 on this, decision was to split out "revocation" into a separate PR, I will also try to better address his concerns on that section in isolation from the other changes, should make it easier to review.

@OR13
Copy link
Contributor Author

OR13 commented Jan 23, 2021

I am removing

<section>
      <h2>Verification Method Revocation</h2>
  
      <p>Verification method revocation is a reactive security measure.</p>

      <p>
        Verification method revocation applies only to the current or latest
        verison of a DID Document.
      </p>
  
      <p>
        If a verification method is no longer exclusively accessible to the
        controller or parties trusted to act on behalf of the controller, it
        should be revoked immediatly to reduce the risk of masquerading, theft
        and fraud.
      </p>
  
      <p class="advisement">
        It is considered a best practice to support key revocation.
      </p>
  
      <p class="advisement">
        A controller should immediately revoke any verification method that is believed to
        be compromised.
      </p>
  
      <p class="advisement">
        Cryptographic verifications associated with revoked verification
        methods should be considered invalid regardless of when revocation
        occurs.
      </p>
  
      <p class="note">
        As described in Section <a href="#verification-method-types"></a>,
        absence of a verification method is the only form of revocation that
        applies to all DID Methods.
      </p>
  
      <p>
        Section <a href="#method-operations"></a> specifies the
        <a>DID</a> operations to be supported by a
        <a>DID method</a> specification, including
        <a href="#update">update</a> and <a href="#deactivate">deactivate</a>
        which MAY be used to remove verification method from a DID Document.
      </p>

      <p>Not all DID Methods support verification method revocation.</p>
  
      <p>
        Even if a verification method is present in a DID Document, additional
        information such as a public key revocation certificate, or external
        allow or deny list might be used to determine if a verification method
        has been revoked.
      </p>
  
      <p class="note">
        Compromise of a verification method allows the attacker access to the verification 
        relationship of the controller, for example authentication.
        The attacker is indistinguishable from the legitimate controller from the 
        time the compromised verification method was registered, to the time it was revoked.
      </p>

      <p class="note">
        The day-to-day operation of any software relying on a compromised verification method 
        might be impacted by a user’s operating system, antivirus, or endpoint protection software 
        when the verification method is publicly revoked.
      </p>

    </section>

@OR13 OR13 changed the title Add revocation and recovery sections from previous PR Add rotation and recovery sections from previous PR Jan 23, 2021
@OR13 OR13 requested a review from dhh1128 January 23, 2021 20:16
@OR13
Copy link
Contributor Author

OR13 commented Jan 23, 2021

@dhh1128 I have moved the main section still under dispute to #570 and added your proposed language as a note. If you have no objections to this PR, please approve it, and request any changes on the one that focuses on revocation.

Copy link
Contributor

@dhh1128 dhh1128 left a comment

Choose a reason for hiding this comment

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

I am content with this PR with the minor proviso that I would prefer to have two words deleted, as I noted separately.

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

TallTed commented Jan 26, 2021

What does "previous PR" mean? Please reference specific PR that is being replaced by this one.

@TallTed
Copy link
Member

TallTed commented Jan 26, 2021

Also, please resolve merge conflicts, so that further review and suggestions here aren't moot.

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.

some small suggestions.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
OR13 and others added 2 commits January 29, 2021 16:34
Co-authored-by: Brent Zundel <brent.zundel@gmail.com>
@OR13
Copy link
Contributor Author

OR13 commented Jan 29, 2021

I believe I have implemented all requested changes.

index.html Outdated Show resolved Hide 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.

Minor nits, merging them, then merging this PR.

@OR13 please pay particular attention to one MUST that I removed -- we can't put normative statements in this section, and in addition, I'd suggest that the normative statement should be tested by this WG anyway.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Co-authored-by: Josh Mandel <jmandel@alum.mit.edu>
@msporny
Copy link
Member

msporny commented Jan 31, 2021

Editorial, multiple reviews, changes requested and made, no objections, merging.

@msporny msporny merged commit 3962377 into main Jan 31, 2021
@msporny msporny deleted the feat/revocation-recovery-issue-386 branch February 21, 2021 21:24
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.

7 participants