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

divergence between implementations serializing ed25519 public keys #42

Closed
erickt opened this issue Apr 9, 2019 · 16 comments · Fixed by #47
Closed

divergence between implementations serializing ed25519 public keys #42

erickt opened this issue Apr 9, 2019 · 16 comments · Fixed by #47

Comments

@erickt
Copy link
Contributor

erickt commented Apr 9, 2019

There is some ambiguity in the spec regarding how to encode ed25519's public portion of the keyval:

The 'ed25519' format is:

  { "keytype" : "ed25519",
    "scheme" : "ed25519",
    "keyval" : {"public" : PUBLIC}
  }

where PUBLIC is a 32-byte string.

While the ed25519 public key is a 32-byte string, I could find no implementation that's serializing the public key to JSON as an array of integers. Instead, implementations are serializing it into a JSON string, but there is a fork in how this is implemented:

Can you provide any guidance on what serialization format we should be using?

edit: turns out rust-tuf uses base64url instead of base64standard for encoding.

@awwad
Copy link
Contributor

awwad commented Apr 9, 2019

That's a good point & question.

The document format section of the specification defines the format as PEM for RSA and ECDSA, but simply says that the ed25519 format should be a "32-byte string", which fits neither the hex string that we use nor the base64 encoding that others apparently use....

@awwad
Copy link
Contributor

awwad commented Apr 9, 2019

@JustinCappos:

  1. Do you agree that it's wise for the spec to define the formats for key values for each key type? (as is done for RSA and ECDSA, but not quite for ed25519 currently)
  2. To what extent should we alter the reference implementation's key serialization to accommodate rust-tuf, notary, and hackage?

The options, basically, are the 44-byte base64 they use, the 64-byte hex string we use, or a 32-byte int. The spec needs to be corrected in every case.

@awwad
Copy link
Contributor

awwad commented Apr 9, 2019

Adding @mnm678 as well, who's working on Uptane profiles/wire-line formats and so might have some input.

@JustinCappos
Copy link
Member

@JustinCappos:

  1. Do you agree that it's wise for the spec to define the formats for key values for each key type? (as is done for RSA and ECDSA, but not quite for ed25519 currently)
  2. To what extent should we alter the reference implementation's key serialization to accommodate rust-tuf, notary, and hackage?

The options, basically, are the 44-byte base64 they use, the 64-byte hex string we use, or a 32-byte int. The spec needs to be corrected in every case.

If we're moving to profiles in TUF, then deciding that format does seem to fit there. It feels easier to me for us to change ours, unless @trishankkarthik disagrees.

@erickt
Copy link
Contributor Author

erickt commented Apr 9, 2019

The options, basically, are the 44-byte base64 they use, the 64-byte hex string we use, or a 32-byte int. The spec needs to be corrected in every case.

I would suggest against using a native JSON 32-byte integer. Many json parsers don't support integers larger than 64 bits.

@awwad
Copy link
Contributor

awwad commented Apr 10, 2019

I suppose it comes down to base64, then. It's a breaking change, so I'll pair this with the key ID calculation changes (that I expect we'll manage to confirm...) for the next major release.

Base64 standard (not base64url) makes the most sense to me. Eric, do you have any thoughts there? Does that switch make sense to you?

@JustinCappos: does this seem like a TAP? :( The existing text didn't really make sense ("32-byte string"), and nobody's using it, so perhaps it's just a TAP-less correction...?

@erickt
Copy link
Contributor Author

erickt commented Apr 10, 2019

@awwad: as best as I can tell, @heartsucker switched all encoding from hex to base64url in theupdateframework/rust-tuf#83. They used base64url over base64 due to these values showing up in URLs and filesystems for target files. I'm not sure if there was any particular reason for using base64url for things inside the metadata, that probably just happened out of using the same codepath for both encodings.

@heartsucker, is this correct?

@trishankatdatadog
Copy link
Member

I agree with @JustinCappos we should be compatible with (go|rust)-tuf, notary, hackage, etc

@erickt
Copy link
Contributor Author

erickt commented Apr 11, 2019

It looks like there's also divergence on how implementations are encoding the snapshot/timestamp/ targets role hashes, both in the metadata and for the hash-prefixed files:

Also, if we were changing ed25519 metadata to use base64, should the spec also change the keyid and path_hash_prefixes to be base64? The spec specifically calls out that they are hex encoded.

@JustinCappos
Copy link
Member

JustinCappos commented Apr 11, 2019 via email

@awwad
Copy link
Contributor

awwad commented Apr 16, 2019

@erickt

Also, if we were changing ed25519 metadata to use base64, should the spec also change the keyid and path_hash_prefixes to be base64? The spec specifically calls out that they are hex encoded.

Unlike the spec's instructions for ed25519 key values, where the spec was clearly wrong, the keyid and path_hash_prefixes being hex weren't mistakes, I don't think. Why switch? Is that what the other implementations do?

@erickt
Copy link
Contributor Author

erickt commented Apr 17, 2019

@awwad: Regarding keyid, almost everyone is using the spec:

  • python-tuf, hackage, and notary is using hex(sha256(cjson(key))).
  • rust-tuf uses sha256(cjson(encoded(pub_key_bytes))). I'm in the process to porting it to use hex(sha256(cjson(key))) though.

Regarding path_hash_prefixes, the only implementation I could find that uses it is python-tuf, and I couldn't find where it actually generates that value, only where it consumes it.

Personally I'm fine keeping the current behavior. We could make the "bytes to string" conversion more consistent for these fields once profiles are fleshed out if we really wanted to.

@heartsucker
Copy link

The reason I made these changes was because in past discussion it sounded like there was said that there was no expected compatibility between implementations. With that in mind, I made opinionated choices based on what I thought made more sense than the original python impl.

@erickt
Copy link
Contributor Author

erickt commented Jul 31, 2019

Hello everyone, just a bump of this ticket since I'm working on this aspect of rust-tuf. Is the plan to update the spec to state that the ed25519 public key and role hashes should use hex encoding, and possibly switch to using base64 encoding later on with a new profile?

@trishankatdatadog
Copy link
Member

I think for now let's go along with hex encoding (python-tuf, hackage, go-tuf, notary) instead of base64. (Notary uses base64 elsewhere, like sigs and hashes, but we'll figure this out later.) If someone really complains about this, we'll figure it out later.

erickt added a commit to erickt/tuf-specification that referenced this issue Aug 1, 2019
According to theupdateframework#42, the spec should explicitly state that the ed25519
public key, and the target hash values are hex encoded values,
since that is used by the majority of tuf implementations.
@erickt
Copy link
Contributor Author

erickt commented Aug 1, 2019

I just submitted #47 to update the spec to reflect hex encoding the ed25519 public key, and target file hashes.

erickt added a commit to erickt/tuf-specification that referenced this issue Aug 1, 2019
According to theupdateframework#42, the spec should explicitly state that the ed25519
public key, and the target hash values are hex encoded values,
since that is used by the majority of tuf implementations.
erickt added a commit to erickt/rust-tuf that referenced this issue Aug 1, 2019
According to theupdateframework/specification#42,
the signature and file hash values should be hex encoded.

Change-Id: I14710a062fae3adfc4bc6b86fbbf533ce1b97070
erickt added a commit to erickt/rust-tuf that referenced this issue Aug 2, 2019
According to theupdateframework/specification#42,
the signature and file hash values should be hex encoded.

Change-Id: I14710a062fae3adfc4bc6b86fbbf533ce1b97070
erickt added a commit to erickt/rust-tuf that referenced this issue Aug 2, 2019
According to theupdateframework/specification#42,
the signature and file hash values should be hex encoded.

Change-Id: I14710a062fae3adfc4bc6b86fbbf533ce1b97070
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 a pull request may close this issue.

5 participants