key format specification is inconsistent

[shibumi]

Hi there,
Right now our specification specifies the following key format:

  { "keytype" : "<KEYTYPE>",
    "scheme" : "<SCHEME>",
    "keyval" : "<KEYVAL>" }

Source: https://github.com/in-toto/docs/blob/master/in-toto-spec.md#42-file-formats-general-principles

However, if you generate a new ed25519 key with in-toto-keygen.py you will end up with the following private and public key.
private:

{
  "keytype": "ed25519",
  "scheme": "ed25519",
  "keyid": "d7c0baabc90b7bf218aa67461ec0c3c7f13a8a5d8552859c8fafe41588be01cf",
  "keyid_hash_algorithms": ["sha256", "sha512"],
  "keyval": {
    "public": "8c93f633f2378cc64dd7cbb0ed35eac59e1f28065f90cbbddb59878436fec037",
    "private": "4cedf4d3369f8c83af472d0d329aedaa86265b74efb74b708f6a1ed23f290162"}
}

public:

{"keytype": "ed25519",
"scheme": "ed25519",
"keyid_hash_algorithms": ["sha256", "sha512"],
"keyval": {"public": "8c93f633f2378cc64dd7cbb0ed35eac59e1f28065f90cbbddb59878436fec037"}}

In the public key the keyID is missing. Is this on purpose?

[lukpueh]

Thanks for pointing this out, I'm completely against adding keyid_hash_algorithms to the spec, and I also vote for removing it from the reference implementation (see theupdateframework/python-tuf#848 for controversy around this field).

Regarding the keyid, it does not really seem necessary to include it in the key object, because each key object is identified by its keyid (as dict key) in the keys dictionary.

So I think we should remove it in the reference implementation, unless I'm missing a good reason (@SantiagoTorres?).

[nmiyake]

Chiming in since I'm noticing the same issue as well -- the spec doesn't mention keyid_hash_algorithms at all, but both the Python and Go implementations use it as part of the canonical JSON used to compute the key ID, so in order for other implementations to match this they must do so as well.

From a practical perspective it's likely not an issue if different ecosystems product different key IDs for the same key material, but as currently written the spec sounds declarative in how keys should be formatted, but the existing implementations don't match up (this is even more so for GPG keys -- see #56).

Ideally, I think that one of the following things should be done:

1. Update spec to note that key format/key value format/key ID as stated in spec are recommended, but that implementations of the spec may vary (and in this case it should be clear what the conceptual constraints need to be within an implementation)
2. Update the spec to match how the current reference implementations are implemented
3. Update the current reference implementations to match exactly what the spec dictates

[adityasaky]

I'd also prefer not to add it to the spec. I wondered if we wanted to allow additional fields but that's also going to lead to more incompatibility. I'm in favour of removing it from the implementations and making certain assumptions there instead. Can we track this in the implementations?

cc @lukpueh

[lukpueh]

Can we track this in the implementations?

👍

secure-systems-lab/securesystemslib#308 should be helpful to create more succinct issues of in python-tuf and in-toto.

[adityasaky]

Closing in favour of the sslib issue, feel free to reopen if we need to have a spec discussion as well!

[nmiyake]

@adityasaky revisiting this, it looks like there's still drift between the "implementations" in this org and the spec itself.

Section 4.2.1 of v1.0 of the in-toto spec is now unambiguous about the key format and key ID derivation:

All keys have the format:

{
  "keytype" : "<KEYTYPE>",
  "scheme" : "<SCHEME>",
  "keyval" : "<KEYVAL>"
}

...

The KEYID of a key is the hexadecimal encoding of the SHA-256 hash of the canonical JSON form of the public key.

However, the Java, Go, and Rust implementations all use the deprecated (and now undocumented/not part of the spec) field "keyid_hash_algorithms" as part of the key ID computation (and this value is also still in the Python demo file: https://github.com/in-toto/in-toto/blob/b372c94bdc38e267cbd02ec8ac91012e26682975/tests/demo_files/demo.layout.template#L38C12-L38C36)

Although removing this field from the computation will change the key IDs generated by those implementations, it seems like it should be done to resolve ambiguity and make those implementations match the spec as declared in v1.0 -- otherwise, other implementations that adhere to the spec will actually not match these implementations.

[adityasaky]

I think updating those specifications is worthwhile, though we're working on consolidating -golang with witness and go-witness. This shouldn't affect verifying old links against old layouts as the keyids would be internally consistent, but I suppose this breaks new links (post implementation update) verified against old layouts? I don't recall if the key ID is only used as a hint with a fallback option. cc @lukpueh