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

Edit Privacy and Considerations sections #37

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

clehner
Copy link
Member

@clehner clehner commented Nov 30, 2021

Continue editing from #20 and #26, applying minor changes in sections 5 and 6.

  • Improve case (capitalization) consistency
  • Fix typos
  • Reduce use of normative-sounding language
  • Attempt to improve some wording

Preview | Diff


Preview | Diff

@clehner clehner marked this pull request as ready for review November 30, 2021 14:56
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, otherwise LGTM.

index.html Outdated
permit tracing of a user in manner not understood or authorized by the
user
by some third party.
would permit tracing of a user by a third party
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
would permit tracing of a user by a third party
would enable tracking of a user, without consent, by a third party

Copy link
Member Author

Choose a reason for hiding this comment

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

Change committed in 26b848b

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussing further in #37 (comment)

index.html Outdated
Comment on lines 1231 to 1233
DID method implementations are encouraged to rotate keys and
identifiers as often as possible,
to avoid correlation.
Copy link
Member

Choose a reason for hiding this comment

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

I know that this was already in the implementer guide, but it makes no sense. If you have a DID that is static, it makes no sense to rotate keys often in order to avoid correlation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, "rotating keys" generally does not avoid correlation. Using different identifiers for different contexts does, however.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, key rotation is about mitigating compromise... not mitigating correlation....

But the part about identifiers is correct, generating a new short lived DID and using it for 1 purpose, and then rotating away from it without a network observing seeing that rotation does mitigate correlation.

did key comes to mind in this regard, I think this section can be improved.... very good catch and comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed: #37 (comment)

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.

Mostly minor language massage.

index.html Outdated
@@ -1162,7 +1162,7 @@ <h2>Privacy Considerations</h2>

<p>
Decentralized Identifiers, like any other technology, can be used to
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Decentralized Identifiers, like any other technology, can be used to
Decentralized Identifiers, like many other technologies, can be used to

Copy link
Member Author

Choose a reason for hiding this comment

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

Committed: 5c7153e

index.html Outdated
Comment on lines 1195 to 1196
DID method specifications are encouraged to have a section dedicated to
personal data, covering the extent to
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
DID method specifications are encouraged to have a section dedicated to
personal data, covering the extent to
DID method specifiers are encouraged to include a specification section
dedicated to personal data, covering the extent to

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 they are actually required to do this, by DID Core.... Privacy and Security considerations are requirements of a did method spec... maybe we can say some thing like "try and make these really impressive"... but still some folks will probably think they didn't go far enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

Committed suggested change in b5dc795.


I don't see "personal data" mentioned in these requirements:

But it is discussed in non-normative sections:

We could say that the personal data section is encouraged in addition to the Privacy and Security considerations sections which are required. Or would personal data be a subsection of Privacy and/or Security considerations?

I agree that more encouragement to develop these sections would be good.

index.html Outdated
Comment on lines 1206 to 1207
would permit tracing of a user by a third party
in manner not understood or authorized by the user.
Copy link
Member

@TallTed TallTed Nov 30, 2021

Choose a reason for hiding this comment

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

Suggested change
would permit tracing of a user by a third party
in manner not understood or authorized by the user.
would enable a third party to track a user without the user's authorization,
in a manner the user may not understand.

Copy link
Member Author

Choose a reason for hiding this comment

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

I applied the similar earlier change #37 (comment) (permit -> enable, tracing -> tracking, consent).
I like that this change uses the active voice.
But is it okay to not mention "authorized" here - i.e. that it is implied by "consent"?
Could use this change but change "consent" to "consent/authorization"?

Copy link
Member

Choose a reason for hiding this comment

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

[@clehner] Could use this change but change "consent" to "consent/authorization"?

Sure, s/consent/authorization/ applied above.

Copy link
Member Author

Choose a reason for hiding this comment

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

@TallTed
This change suggestion (diff) doesn't apply cleanly now, due to the changes in #37 (comment) (26b848b).
I see your newer change suggestion on this sentence as updated, here: #37 (comment).
The newer suggested phrasing is different from the one here.
Does the newer change suggestion supersede this one?

index.html Outdated
Comment on lines 1259 to 1260
Review any applicable local law when considering developing or
operating a decentralized identifier method.
</p>

<p>Consider <a href="https://gdpr.eu/">GDPR</a>.</p>

<p>Consider <a href="https://oag.ca.gov/privacy/ccpa">CCPA</a>.</p>

<p>Consider <a
href="https://www.bis.doc.gov/index.php/policy-guidance/encryption">EAR</a>.
</p>
operating a decentralized identifier method. Consider the following:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Review any applicable local law when considering developing or
operating a decentralized identifier method.
</p>
<p>Consider <a href="https://gdpr.eu/">GDPR</a>.</p>
<p>Consider <a href="https://oag.ca.gov/privacy/ccpa">CCPA</a>.</p>
<p>Consider <a
href="https://www.bis.doc.gov/index.php/policy-guidance/encryption">EAR</a>.
</p>
operating a decentralized identifier method. Consider the following:
Review any applicable local laws when considering developing or
operating a decentralized identifier method. Such local laws may
include, but are not limited to, the following:

Copy link
Member Author

Choose a reason for hiding this comment

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

Committed: 5f088c6

index.html Outdated
Comment on lines 1299 to 1300
Avoid technologies declared <em>de facto</em> standard based on
particular implementations but lacking open standards.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Avoid technologies declared <em>de facto</em> standard based on
particular implementations but lacking open standards.
Avoid technologies declared <em>de facto</em> standards based on
particular implementations but lacking open standard technical
specifications.

Copy link
Member Author

Choose a reason for hiding this comment

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

Committed: c9d2bc6

index.html Outdated
store information other than a DID Document, which reduces the direct
Avoid requiring credential schemas to be stored on ledgers. Many DID
methods cannot
store information other than a DID document, which reduces the direct
interoperability, substitutability, and cost effectiveness of
solutions that make use of rare or poorly supported features such as
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
solutions that make use of rare or poorly supported features such as
solutions that make use of rarely or poorly supported features such as

Copy link
Member Author

Choose a reason for hiding this comment

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

Committed: 287afe9

@peacekeeper
Copy link
Contributor

Looks good, thanks for investing time in improving this.

index.html Outdated
</p>

<p class="advisement">
Support for legacy cryptography systems such as
Consider support for legacy cryptography systems such as
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
Consider support for legacy cryptography systems such as
Consider support for widely adopted cryptography systems such as

Copy link
Contributor

Choose a reason for hiding this comment

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

This might be controversial, but i think legacy has the wrong connotation here.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Consider support for legacy cryptography systems such as
Consider support for historically prevalent cryptography systems such as

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added both suggested phrasings, and removed the latter clause which I think is made redundant: c539bca

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.

@clehner this PR is excellent, please apply or dismiss the suggested changes and ping me for a re-review.

attacks have been performed in the wild due to improper selection of
random values in key material and other aspects of cryptography.
</section>

<section class="informative">
<h3>Zero Knowledge Proofs</h3>
<h3>Zero-Knowledge Proofs</h3>
Copy link
Member

Choose a reason for hiding this comment

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

this is not a comment on @clehner 's excellent editorial work.

Why does the DID Implementation Guide talk about zero-knowledge proofs at all? Perhaps to the extent that an issuer may wish to use a DID Document to publish the verification key for a zero-knowledge proof–capable VC, but beyond that, the information in this section seems to relate little to DIDs

Copy link
Member

Choose a reason for hiding this comment

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

DID documents are to contain such crypto material as may be necessary to communicate with the entity identified by that DID. DID method specifiers and implementers may use ZKPs for these purposes, which possibility I think makes this section just as relevant as any of the others here.

Copy link
Member

Choose a reason for hiding this comment

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

Really, my concern with the section is not that it talks about ZKPs and how they may be used with DIDs, my concern is that it doesn't.

The guidance here is to 1) use proper curves for ZKPs, 2) not use ZKP proof formats tied to specific ledger technology, and 3) not store schemas on ledgers.
What does this guidance have to do with DIDs?

Copy link
Member

Choose a reason for hiding this comment

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

@brentzundel -- This should perhaps become a new issue of its own?

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.

I approve, once the suggestions already made by others have been accepted.

clehner and others added 7 commits December 3, 2021 16:28
index.html Outdated Show resolved Hide resolved
index.html Outdated
Comment on lines 1232 to 1233
identifiers as often as possible,
to avoid correlation.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
identifiers as often as possible,
to avoid correlation.
identifiers as often as practical,
to avoid compromise and correlation, respectively.

Copy link
Member Author

Choose a reason for hiding this comment

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

Applied in 45d8529.

Related earlier discussion: #37 (comment)

@OR13
Copy link
Contributor

OR13 commented Dec 10, 2021

@TallTed would you mind resolving all change suggestions that have been addressed? or lmk if you feel this is good from your perspective?

index.html Outdated
</a> should be consulted when selecting curves for usage with zero
knowledge proofs,
href="https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-pairing-friendly-curves">
Pairing-Friendly Curves</a>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Pairing-Friendly Curves</a>
Pairing-Friendly Curves</a>,

Copy link
Member Author

Choose a reason for hiding this comment

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

Committed: 5b85935

@TallTed
Copy link
Member

TallTed commented Dec 10, 2021

[@OR13] @TallTed would you mind resolving all change suggestions that have been addressed? or lmk if you feel this is good from your perspective?

I have no "resolve" button anywhere. Another downside to applying suggestions externally to the thread where they're suggested, instead of using the "commit" button that's right there for the PR author (and possibly others).

clehner and others added 2 commits December 10, 2021 11:35
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
@clehner
Copy link
Member Author

clehner commented Mar 1, 2022

I believe the requested changes have been resolved.

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