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

publicKeyHex format unused by spec currently #236

Closed
kdenhartog opened this issue Mar 21, 2020 · 22 comments
Closed

publicKeyHex format unused by spec currently #236

kdenhartog opened this issue Mar 21, 2020 · 22 comments
Assignees
Labels
pending close Issue will be closed shortly if no objections

Comments

@kdenhartog
Copy link
Member

I was looking to see if when using publicKeyHex format if the publicKey was expected to be in compressed or non-compressed form and realized that currently none of the key types that are supported in the spec currently use publicKeyHex. This left me wondering should we keep it in here?

I'm fine with removing it, but it still leaves my original question for looking open, but with publicKeyBase58 instead.

When representing secp256k1 keys should we be doing this in compressed form or uncompressed form? From what I can tell most implementations use hex encoding in compressed form, so my recommendation would be that we represent it in compressed form as well.

@ChristopherA or @OR13 do you have any insights to share on this?

@OR13
Copy link
Contributor

OR13 commented Mar 22, 2020

publicKeyHex is currently not supported by the did core spec... so you need to create a custom context to use it... this impacts did:ethr, did:btcr, did:elem, did:ion, and probably others...

@selfissued @csuwildcat Sidetree use of this representation is IMO one of its most serious faults... nobody should be using publicKeyHex, when we have publicKeyJwk, and we can do conversion...

https://github.com/decentralized-identity/lds-ecdsa-secp256k1-2019.js/blob/master/packages/es256k-jws-ts/src/keyUtils.spec.ts#L35

Nobody should be using publicKeyHex... but if people do use it... it should always be documented as being the compressed form... For example, in the element sidetree context:

https://context.transmute.org/element/#publicKeyHex

@csuwildcat
Copy link
Contributor

This was how Sidetree worked prior to the JWK decision, so it's not a fault of Sidetree at all, it's the fault of a volatile spec process that created a frenetically moving target to implement against. This is just yet another example that underscores how smart a decision it is that we are now removing all reliance on the ever-shifting structure/format of a DID Document from all areas of Sidetree other than the final rendering stage.

@csuwildcat
Copy link
Contributor

Nobody should be using publicKeyHex... but if people do use it... it should always be documented as being the compressed form... For example, in the element sidetree context:

It is documented in the current (and new) spec as the compressed form: https://github.com/decentralized-identity/sidetree/blob/master/docs/protocol.md#add-public-keys

@OR13
Copy link
Contributor

OR13 commented Mar 22, 2020

It needs to be defined in a context file, in order to not cause errors when being present in the DID Document... For example, publicKeyJwk is currently not suported in any representation (JSON, JSON-LD, CBOR).... we need to either add publicKeyHex to the did core registry, or define it in a custom context, and use that.

This PR adds support for publicKeyJwk to the did core registry: w3c/did-spec-registries#20

I can implement a PR to add support for publicKeyHex if everyone wants that in in did core, but my expectation is that it will be met with resistance, and that we will need to define publicKeyHex somewhere else, if we want sidetree did documents to ship being spec compliant...

See also this issue, which tracks our support (or lack of) for properties used by sidetree did documents:

w3c/did-spec-registries#19

@mirceanis
Copy link

I can implement a PR to add support for publicKeyHex if everyone wants that in in did core, but my expectation is that it will be met with resistance, and that we will need to define publicKeyHex somewhere else, if we want sidetree did documents to ship being spec compliant...

Since so many DID methods already use this representation, it makes sense to add it.
Whether it is compressed or not should not matter that much since the compression is indicated in the first byte anyway, but I agree that it would be easier for it to be defined as a single one of the 2 formats.

@selfissued
Copy link
Contributor

We had many calls about key representations and the conclusions was to always support JWK and to support some ad-hoc representations for particular key formats, such as RSA and EC. For EC, the format chosen was base48.

Given that Hex has equivalent representations for both of these, we should not add Hex, as it is duplicative and less compact that the other two.

@kdenhartog
Copy link
Member Author

kdenhartog commented Mar 24, 2020

That was my understanding. Going back and rereading my statement it sounds a bit like I was trying to re-litigate this issue here which wasn't my intention nor do I don't think that would be productive given how long that discussion took.

I'll take an action item to remove references to publicKeyHex in the spec.

Which leaves the last questions of compressed or uncompressed form for secp256k1 keys. I'd push for compressed form to reduce the size of the keys, but I want to make sure it won't cause issues for folks using this key type. Specifically @awoie with ethr and @kimdhamilton with DID BTCR are the ones that come to mind. With bitcoin specifically, my understanding is that representing the key as a compressed form produces a different address then when using uncompressed form.

Also, for those wondering what I'm talking about see section 4.3.6 here: https://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.202.2977&rep=rep1&type=pdf

@peacekeeper
Copy link
Contributor

We have NOT made a final decision what the two supported formats for each key type would be. See the following note in the spec:

The Working Group is still debating whether the base encoding format used will be Base58 (Bitcoin) [BASE58], base64url [RFC7515], or base16 (hex) [RFC4648]. The entries in the table below currently assume PEM and Base58 (Bitcoin), but might change to base64url and/or base16 (hex) after the group achieves consensus on this particular issue.

I am under the impression that in the communities that use secp256k1-koblitz and secp256r1, hex encoding is more common than Base58 encoding. I have argued this during one of the key formats calls (see minutes here). Therefore I would be in favor of adding publicKeyHex to the registry, and to update the DID Core spec to make that the second supported format for those key types.

Someone who is most familiar with those key types should comment which encoding is most common. @ChristopherA ? @awoie ?

@kdenhartog
Copy link
Member Author

kdenhartog commented Mar 31, 2020

We have NOT made a final decision what the two supported formats for each key type would be. See the following note in the spec:

The Working Group is still debating whether the base encoding format used will be Base58 (Bitcoin) [BASE58], base64url [RFC7515], or base16 (hex) [RFC4648]. The entries in the table below currently assume PEM and Base58 (Bitcoin), but might change to base64url and/or base16 (hex) after the group achieves consensus on this particular issue.

Ahh I was under the impression that this discussion was concluded after the key format discussion took place and that was left in the spec and was stale. I'll hold off on creating a PR then.

@kdenhartog kdenhartog self-assigned this Mar 31, 2020
@kdenhartog kdenhartog added the question Further information is requested label Mar 31, 2020
@csuwildcat
Copy link
Contributor

csuwildcat commented Mar 31, 2020 via email

@OR13
Copy link
Contributor

OR13 commented Mar 31, 2020

So if we add support for publicKeyHex, this is the situation we will have:

EcdsaSecp256k1VerificationKey2019 may contain:

  1. publicKeyJwk
  2. publicKeyBase58
  3. publicKeyHex

There are already competing implementations that use publicKeyJwk and publicKeyBase58:

every time we add optionality to did core, a developer dies.... If you want to use a custom key format, you are free to define your own context to do so, as we have done for sidetree: https://identity.foundation/sidetree/docs/spec/#publickeyhex

The idea of adding things to did core, is to encourage their use... do we really want to encourage 3 competing key representations for secp256k1... ?

I don't. I want to encourage:

  1. publicKeyJwk for all supported key types
  2. publicKeyBase58 for experimental crypto

... everything else should have to handle conversion to the formats above ^

@peacekeeper
Copy link
Contributor

peacekeeper commented Apr 1, 2020

@OR13

do we really want to encourage 3 competing key representations for secp256k1... ?

Definitely not.. My understanding from the key formats calls was that each key type must support JWK, plus at most one additional format. That additional format should be whatever is the most common/popular format for the given key type. That's why we chose publicKeyPem for RSA.

All I'm saying is that I think that there was still an open question whether Base58 or Base16 (hex) should be the second supported format for secp256k1. Personally my impression is that while Base58 is obviously very common for Bitcoin and Ethereum addresses, the only Bitcoin and Ethereum public keys I have ever seen (outside the DID community) were always in Base16 (hex).

@ChristopherA
Copy link
Contributor

On Tue, Mar 31, 2020 at 10:20 PM Markus Sabadello wrote:

the only Bitcoin and Ethereum public keys I have ever seen (outside the DID community) were always in Base16 (hex).

That is correct. Public keys are rarely used and Bitcoin Core in particular always outputs it in hex.

— Christopher Allen [via iPhone]

@kdenhartog
Copy link
Member Author

@OR13

do we really want to encourage 3 competing key representations for secp256k1... ?

Definitely not.. My understanding from the key formats calls was that each key type must support JWK, plus at most one additional format. That additional format should be whatever is the most common/popular format for the given key type. That's why we chose publicKeyPem for RSA.

All I'm saying is that I think that there was still an open question whether Base58 or Base16 (hex) should be the second supported format for secp256k1. Personally my impression is that while Base58 is obviously very common for Bitcoin and Ethereum addresses, the only Bitcoin and Ethereum public keys I have ever seen (outside the DID community) were always in Base16 (hex).

I'm good with this approach.

It still leaves the remaining question open about compressed vs uncompressed format and where that should be defined.

@OR13
Copy link
Contributor

OR13 commented Apr 1, 2020

As someone who has used publicKeyHex a lot, I have always defined it in a custom method context.

I know it's frustrating to convert hex to a more usable format (for things other than currency transactions), but IMO it is a good thing to do, and you don't need to if you define a method specific context.

Nobody should be using the uncompressed version imo... but there is nothing stopping 2 methods form both supporting publicKeyHex and choosing different representations.

@kdenhartog
Copy link
Member Author

In bitcoin uncompressed keys produce different addresses than compressed and both are supported. That's the only reason I didn't opt for assuming they were always compressed. I also liked @msporny suggestion to just add a custom context of publicKeyHexUncompressed if someone REALLY needed it. Leaving only the last question of where does this need to be defined?

@ChristopherA
Copy link
Contributor

BTCR only uses compressed keys.

— Christopher Allen [via iPhone]

@kdenhartog
Copy link
Member Author

BTCR only uses compressed keys.

— Christopher Allen [via iPhone]

Thanks for clarifying that. I went off the example and the length of it to infer there, but didn't find any confirmation in the method spec.

@msporny
Copy link
Member

msporny commented Jun 2, 2020

The DID Specification Registries will point to a specification that defines publicKeyHex. At present, the assumption is that the W3C CCG will add this to the Security Vocabulary under its purview. If that doesn't happen, DIF will do it.

@OR13
Copy link
Contributor

OR13 commented Jun 2, 2020

prefer to see it defined here:

  1. https://github.com/w3c-ccg/security-vocab

  2. at DIF if we can't get it merged here ^

cc @awoie @ChristopherA if either of you feels like doing a PR against security vocab, so we can point to that... that would be great.

@msporny
Copy link
Member

msporny commented Jun 16, 2020

This should be processed in the DID Spec Registries repository as publicKeyHex will be an extension that is registered in DID Specification Registries. Move this discussion over there:

https://github.com/w3c/did-spec-registries/issues

Suggest closing this issue.

@msporny msporny added pending close Issue will be closed shortly if no objections and removed question Further information is requested labels Jun 16, 2020
@kdenhartog
Copy link
Member Author

Just opened a PR for publicKeyHex in the security vocab repository, have opened an issue in the DID Spec Registries to continue the discussion there, and will close this topic now. Thanks for the considerations of all in this topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending close Issue will be closed shortly if no objections
Projects
None yet
Development

No branches or pull requests

8 participants