Skip to content
This repository has been archived by the owner on Oct 29, 2019. It is now read-only.

Add publicKeyHex as a valid publicKey format #270

Closed
wants to merge 4 commits into from

Conversation

gjgd
Copy link

@gjgd gjgd commented Aug 19, 2019

fixes #152

Added publicKeyHex as a valid publicKey format

The fact that publicKeyHex is missing from the did spec as a valid publicKey format is preventing us from adding did:elem to the universal resolver

Not sure if can just edit the v0.11 file or if I should create a v0.12, let me know!

@dmitrizagidulin
Copy link

dmitrizagidulin commented Aug 19, 2019

@gjgd Ok, so, we're about to submit a PR (parallel to this one) that explains the context versioning mechanism (in a README in the contexts/ folder), which seems to have buy-in from at least some of the devs maintaining the contexts.

With that in mind, I recommend - make a v0.12 version (in addition to the v0.11 - leave v0.11 unchanged, without the term), and add the new term to /that/ one.

@gjgd
Copy link
Author

gjgd commented Aug 19, 2019

Thanks @dmitrizagidulin , I put it in a separate file

"publicKey": {"@id": "sec:publicKey", "@type": "@id", "@container": "@set"},
"publicKeyBase58": "sec:publicKeyBase58",
"publicKeyPem": "sec:publicKeyPem",
"publicKeyHex": "sec:publicKeyHex",
Copy link
Contributor

Choose a reason for hiding this comment

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

We really need to move to publicKeyMultibase ... because that supports base16 (hex), base58, base64url, etc.

Can we do that instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably also switch to Multicodec encoding of public keys... we've been needing to do this for a very long time.

Choose a reason for hiding this comment

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

Agreed on both counts (multibase and multicodec). Although I wonder if it might cause less confusion to at least have a transition period where we support both a general publicKeyMultibase and the specific ones publicKeyBase58 etc, side by side.

Copy link
Author

Choose a reason for hiding this comment

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

We could add publicKeyMultibase but then it wouldn't be consistent to keep publicKeyBase58. The problem is removing publicKeyBase58 would cause retro-compatibility issue because it is already used by did methods (did:sov)

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I agree we could support them all side by side for now

Copy link
Author

Choose a reason for hiding this comment

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

Can we merge this PR as is, and create an issue for publicKeyMulticodec that can be discussed and solved a separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, we shouldn't merge. This should be a CCG decision... it has far too much of an impact on DID Method implementers to merge w/o consulting w/ the community first.

Copy link

@OR13 OR13 Aug 19, 2019

Choose a reason for hiding this comment

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

I don't see why we would not add support for the key types listed in the spec:
https://w3c-ccg.github.io/did-spec/#public-keys

I agree, that some are better than others (Personally I'm a fan of publicKeyPem and publicKeyJwk).

We should either remove the key types from the spec or support them. Saying that the community uses a key type, but then having it not be supported seems wrong.

I agree that the CCG should make the decision here.

Copy link
Member

Choose a reason for hiding this comment

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

I'm with @dmitrizagidulin and @OR13 on this.. I think publicKeyMulticodec is great. But considering that we just released the Community Group Final Report, we should have a context (either the current v0.11 or a new v0.12, doesn't matter so much I think) that contains all terms used in that released spec, including all the example public key properties (see § 5.3, this was last changed in 8dbfe62).

After that we could immediately start working on a new context 0.13 and have the discussion about removing everything except publicKeyMultibase / publicKeyMulticodec.

Copy link

Choose a reason for hiding this comment

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

I believe this PR resolves that issue. It need only be approved / merged.

This reverts commit 254ae33.
@OR13
Copy link

OR13 commented Sep 6, 2019

This PR is semi-blocking the DIF interop project from demonstrating interoperability between DDOs with publicKeyHex and publicKeyJwk.

Per the spec: https://w3c-ccg.github.io/did-spec/#public-keys

The following is a non-exhaustive list of public key properties used by the community: publicKeyPem, publicKeyJwk, publicKeyHex, publicKeyBase64, publicKeyBase58, publicKeyMultibase, ethereumAddress.

There does not appear to be a single commited / hosted context that supports all of these, but there are context that support publicKeyBase58.

Additionally https://w3id.org/did/v0.11 redirects to https://w3c-ccg.github.io/did-spec/contexts/did-v0.11.jsonld, is not a hashlink, and provides no assurance that v0.11 context is immutable.

It seems likely that without a W3 hosted context that supports community key types, community members are likely to host their own contexts, is that a good thing or a bad thing?

I have been assuming that the ideal scenario is that all DID Methods would try their best to support the same latest context.

Maybe it is preferred that each DID Method host their own context which describes the key types and custom properties present in their DDO?

It feels like a security risk to point to a context you don't control, am I wrong to assume that?

@OR13
Copy link

OR13 commented Sep 6, 2019

It seems like community hosting of contexts won't work... and this PR is not sufficient to fix this issue because:

[
      {
        "@id": "did:elem:eURSFEEv6J7s3TJ-jhT_ZS4uGRyCDbwc347EWlqpNgw",
        "https://w3id.org/security#publicKey": [
          {
            "@id": "did:elem:eURSFEEv6J7s3TJ-jhT_ZS4uGRyCDbwc347EWlqpNgw#key-0"
          },
          {
            "@id": "did:elem:eURSFEEv6J7s3TJ-jhT_ZS4uGRyCDbwc347EWlqpNgw#key-JUvpllMEYUZ2joO59UNui_XYDqxVqiFLLAJ8klWuPBw"
          }
        ]
      },
      {
        "@id": "did:elem:eURSFEEv6J7s3TJ-jhT_ZS4uGRyCDbwc347EWlqpNgw#key-0",
        "@type": [
          "https://w3id.org/security#EcdsaSecp256k1VerificationKey2019"
        ],
        "https://w3id.org/security#publicKeyHex": [
          {
            "@value": "027560af3387d375e3342a6968179ef3c6d04f5d33b2b611cf326d4708badd7770"
          }
        ]
      },
      {
        "@id": "did:elem:eURSFEEv6J7s3TJ-jhT_ZS4uGRyCDbwc347EWlqpNgw#key-JUvpllMEYUZ2joO59UNui_XYDqxVqiFLLAJ8klWuPBw",
        "@type": [
          "https://w3id.org/security#EcdsaSecp256k1VerificationKey2019"
        ],
        "https://w3id.org/security#publicKeyJwk": [
          {
            "@id": "_:b0"
          }
        ]
      }
    ]

https://web-payments.org/vocabs/security#EcdsaSecp256k1VerificationKey2019
https://w3id.org/security#publicKeyJwk

These links won't point to anything, but does that really matter?

If it does, then there seems to be a tight coupling between valid context updates and the html content of https://w3id.org...

Based on #272

It seems like we may want to add some additional criteria to the versioning requirements, namely that there not be any broken links in the contexts.

@OR13
Copy link

OR13 commented Sep 8, 2019

After a few more days of pondering, I created a self hosted solution for this.

There appears to be a tight coupling between contexts and their documentation.

When we update a context, there should be no broken links, new properties should be documented!

I don't think its possible to do that with the hosting setup that exists currently, or it will require several PRs across a number of repos, and I suspect that will lead to no actual documentation for contexts, as is the case currently. For example, where is the documentation for:

https://w3id.org/security/v2 ?

None of these properties are defined here: https://w3id.org/security#, these docs were last updated in 2016.

This PR: #207

Added properties to the context that are not documented, and some of these properties are actually not usable without #270 which adds support for the key types used by EcdsaSecp256k1Signature2019...

As the DID Spec moves along, I would encourage us to host context and documentation in the same place, so that contributors can fix issues like this easier in the future.

If the repo example I have linked is useful for that purpose, feel free to copy it:

transmute-industries/context#1

Just to be super clear, I want to use the W3 hosted did spec without extending it, and I want to contribute to it.

Contribution should not result in broken links, and should not need to be spread across multiple repos, because that will cause broken links, and slow down contribution.

Should I open issues for undocumented context properties in the did spec and cross link to these repo's?:

https://github.com/perma-id/w3id.org/blob/master/security/.htaccess
https://github.com/web-payments/web-payments.org

I'm not trying to be a pest, clean documentation will help us attract more contributors, and will make all of our lives easier. I'm happy to help accomplish this however I can.

@msporny
Copy link
Contributor

msporny commented Sep 8, 2019

@OR13 you're catching us at a really heavy time of conferences and travel. Almost everyone in the community is traveling to/from RWoT9, APConf2019, W3C TPAC 2019, and a variety of other things. There are answers to some of your questions and plans for others, but I need to catch a 5am flight and need to go to sleep. Others are in the same position. Can you request some time to talk about this on a CCG call in early October? Self-hosting the solution (or ideally just overloading the JSON-LD document loader) should work for you for the time being.

@OR13
Copy link

OR13 commented Sep 8, 2019

@msporny thanks! I will request some time on the call :)

@brentzundel
Copy link
Contributor

This repo is scheduled to be archived. The work has moved to the DID WG.
The artifacts in the DID WG repository (including the context document) share commit history with this repo, so it should be possible to raise this PR against that repo.

@peacekeeper
Copy link
Member

@gjgd as discussed at IIW29, do you think you could re-create this PR in the DIDWG repo: https://github.com/w3c/did-spec/pulls and then close it here?

@gjgd
Copy link
Author

gjgd commented Oct 5, 2019

Yes, I'm not sure we want to merge it as is anymore but it's done @peacekeeper
w3c/did-core#63

@peacekeeper
Copy link
Member

Thanks, we can continue to discuss at w3c/did-core#63. Closing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
context The did JSON-LD context, or contexts referenced therefrom
Projects
None yet
Development

Successfully merging this pull request may close these issues.

publicKeyJwk, publicKeyHex, publicKeyBase64, publicKeyBase58 missing from context.
7 participants