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

Registering did:snplab did method #340

Closed
wants to merge 4 commits into from
Closed

Conversation

jestun-snp
Copy link
Contributor

@jestun-snp jestun-snp commented Oct 1, 2021

Registering did:snplab did method request


Preview | Diff

index.html Outdated
@@ -4344,6 +4344,23 @@ <h1>DID Methods</h1>
<a href="https://workday.github.io/work-did-method-spec/">Workday DID Method</a>
</td>
</tr>
<tr>
<td>
did:snplab:
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to sorted order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing out. I made the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's still at the end. The whole row should be in method name order up in the did:s* part.

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 have put the did:snplab between did:sirius and did:sol.

Thanks.

index.html Outdated
@@ -4344,6 +4344,23 @@ <h1>DID Methods</h1>
<a href="https://workday.github.io/work-did-method-spec/">Workday DID Method</a>
</td>
</tr>
<tr>
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix indent.

Suggested change
<tr>
<tr>

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.

move to sorted order.

1. Fix URL for SNPLab DID method spec
2. Fix the indentation issue.
Put did:snplab between did:sirius and did:sol
Remove # in the URL to DID method specification
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.

The specification is missing the sections on Update and Deactivate. The Security and Privacy considerations section is exceedingly light. Please review https://w3c.github.io/did-core/#method-operations and at least address the issues stated in this change request.

@iherman
Copy link
Member

iherman commented Nov 9, 2021

The issue was discussed in a meeting on 2021-11-04

  • no resolutions were taken
View the transcript

2.2. Registering did:snplab did method (pr did-spec-registries#340)

See github pull request did-spec-registries#340.

Manu Sporny: People should read change requests as -- the Editors need it cleared for it to go in..

@kdenhartog
Copy link
Member

I just gave a read through this under the eye of someone who might implement this and here's some points I caught:

  1. In section 1.b it's stated that base54 is used first and then in 2.b.ii it states base58 is used. Can you clarify which should be used?

  2. When is it acceptable to use the 22 character MSI versus the 44 character MSI? Would the 22 character MSI and the 44 character MSI that are the same encoded public key be equivilant in this method or should they be recognized as separate entities? If they are considered equivalent how should an implementer communicate this?

  3. Section 2 mentions base54 again, so thinking base58 in 2.b.ii is the incorrect one. Also is there a normative reference to what the character set of base54 is that can be included in this specification? I've never heard of it before.

  4. The @context property is being included in the examples doesn't include the context for the verification methods used so the publicKeyBase58 property will be dropped by a JSON-LD processor. Also the publicKeyBase58 is no longer a compliant property to be used with did-core specification so you should look to moving to either the JsonWebKey2020 verification method so that publicKeyJwk can be used or utilize the Ed25519VerificationKey2020 so that publicKeyMultibase can be used.

  5. In section 3.a it states that Create and Delete operations require the usage of the x-auth-key HTTP header, but it makes no mention of the update operation. Is this because this method doesn't support updates or is this a gap in that anyone is allowed to update anyone else's did? Also, you'd probably want to normatively define this is as a MUST be used so that other implementers know that this property is being used to meet the requirements of DID-Core in 8.2

  6. It's stated that the method should call a specific endpoint to perform a create operation for this method. Is it expected that this is normatively defined so that no other implementer could implement a VDR gateway to anchor to the network? Also is the architecture of the VDR proprietary for this method since it's not clear how the call to the gateway is performing the anchoring to the blockchain network. Additionally, it wasn't clear to me what the underlying blockchain network is.

  7. The examples in the create should follow the points raised in 4 above as well. In this case, if the VDR gateway accepts invalid JSON-LD when performing the creation you'll end up with a garbage in garbage out scenario where relying parties who are utilizing the JSON-LD representation will be accidentally dropping the public key listed in the example when running it through a JSON-LD processor. You have a few ways in which you could go about solving this problem. You could either opt to not support the JSON-LD representation and normatively define this, you could perform a validation up front before anchoring to the network to prevent this property from being dropped, or you could leave this as a GIGO scenario. I'd highly recommend option 1 or option 2 in this scenario but it is not technically in-compliant to use option 3. It just means that you're going to run into interoperability problems when you move into higher layers such as validating JSON-LD verifiable credentials.

  8. It's not clear to me in the read operation if the endpoint is being normatively defined as what MUST be used to resolve a did for the did:snplab method or if that's meant to be a non-normative example.

  9. You should take a look at section 7 of the DID-Core specification as well. The injecting of the created and updated time into the DID Document itself is not the most common way that this is being done now. Instead it's commonly expected that these properties will be included in the DID Document Metadata portion of the DID resolution data model. There's no easy examples to look at in there to better understand it, so I'd suggest either reading through that section entirely or taking a look at the DID Resolution draft specification which has additional details and examples about this process.

  10. In section 4.a you state "Personal user information will not be stored in the SNPLab block chain or Auth server" however if you're taking a GIGO approach like mentioned in point 7 you'd not necessarily be able to enforce this as a VDR gateway. Automatically checking the did document for PII is a rather difficult process because the DID Controller could choose to add that data into their document and it's very hard to identify what's PII from what's more generic metadata automatically. For example, what if I made the fragment query my name? rather than verkey like the example uses. That would be constituted as PII being stored if you're using a GIGO scenario. So I think you're going to need to further expand on the points here in section 4. Take a look at RFC6973 for additional details about how to thoroughly document the privacy considerations section and take a look at RFC3552 section 5 for the subsequent security considerations section.

Overall, my belief is that this method is not ready to be registered. There's still some aspects around the section 8 requirements of the DID-Core spec which are missing, but beyond that there's a lot of difficulty here for me as a prospective implementer to be able to interoperate with this method at this point and I wouldn't be able to write my own implementation as this document is currently written.

Copy link
Member

@kdenhartog kdenhartog left a comment

Choose a reason for hiding this comment

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

Requesting changes based on the details provided in the comments above. Also note, the registration process is going to change (this is the first time we've updated it - sorry about the inconvenience this will bring) so this PR will need to be updated once the spec has been updated.

@mprorock
Copy link
Contributor

mprorock commented Dec 7, 2021

Reviewed on call @jestun-snp would you mind resolving conflicts so we can re-review?

@iherman
Copy link
Member

iherman commented Dec 7, 2021

The issue was discussed in a meeting on 2021-12-07

  • no resolutions were taken
View the transcript

4.1. Registering did:snplab did method (pr did-spec-registries#340)

See github pull request did-spec-registries#340.

Michael Prorock: there are change requests from some folks and change request from others..
… looks like changes have been addressed.

Manu Sporny: +1, this has changes requested, waiting for PR submitter..

Orie Steele: please comment on the PR..

Manu Sporny: I thought we were still waiting on them....

Michael Prorock: there are still conflicts.

Manu Sporny: their security are privacy section needs work..

Michael Prorock: I will ping the PR.

@jestun-snp jestun-snp closed this Dec 8, 2021
@jestun-snp
Copy link
Contributor Author

We will prepare changes and re-request.

Thanks.

@peacekeeper peacekeeper mentioned this pull request Dec 16, 2021
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.

None yet

7 participants