include keyid in public ed25519/ecdsa public key files

[shibumi]

Please fill in the fields below to submit a pull request. The more information
that is provided, the better.

Fixes issue: in-toto/specification#33

Description of the changes being introduced by the pull request:

In the past we did not include the keyid in public ed25519/ecdsa key files. This commit changes this behavior and introduces a new argument to the format_keyval_to_metadata function. The argument is optional,
so this should not have any affect on using format_keyval_to_metadata for keyid generation.

Please verify and check that the pull request fulfils the following
requirements:

• The code follows the Code Style Guidelines
• Tests have been added for the bug fix or new feature
• Docs have been added for the bug fix or new feature

Do we need further tests/documentation for this change? The new argument is optional, so it should be API stable, however the generated public key itself may be different from now on. I have not tested this PR with in-toto-keygen yet.

[shibumi]

is this check here is enough? Maybe we want to validate the keyid?

[shibumi]

Do you know why we never included the keyid here? It doesn't make sense for me, because if we don't provide a filepath we are even naming the keypair files <KEYID>.{pub,key}

[coveralls]

Coverage increased (+0.002%) to 98.946% when pulling a8b3a2e on shibumi:shibumi/fix-keyids-in-ed25519-public-keys into 22a3c4e on secure-systems-lab:master.

[coveralls]

Coverage increased (+0.002%) to 98.946% when pulling a8b3a2e on shibumi:shibumi/fix-keyids-in-ed25519-public-keys into 22a3c4e on secure-systems-lab:master.

[shibumi]

mh funny that the coverage increased, I didn't even push tests?!

[shibumi]

Ok, I have tested the PR with in-toto with the following requirements.txt file for in-toto:

attrs==19.3.0
cffi==1.14.0              # via cryptography, pynacl
colorama==0.4.3           # via securesystemslib
cryptography==2.9.2         # via securesystemslib
enum34==1.1.10             # via cryptography
ipaddress==1.0.23         # via cryptography
iso8601==0.1.12
pathspec==0.8.0
pycparser==2.20           # via cffi
pynacl==1.4.0             # via securesystemslib
python-dateutil==2.8.1
git+git://github.com/shibumi/securesystemslib@shibumi/fix-keyids-in-ed25519-public-keys#egg=securesystemslib
six==1.15.0
subprocess32==3.5.4       # via securesystemslib

Then I did:

$ python setup.py build
$ python setup.py install

Then:

(in-toto) chris motoko ~/export/github/in-toto 17:01:51 6cfa36cf shibumi/fix-ed25519-keyid-in-public-key  
❯ cat bob.pub 
{"keytype": "ed25519", "scheme": "ed25519", "keyid": "a1bcfbc3cf506b2a2ac974b679da58a3bde354a4acd23998ba2fdb60c011ba17", "keyid_hash_algorithms": ["sha256", "sha512"], "keyval": {"public": "07dea440b3e151f02a4e80296863f75a0bc0e46f61d7f4e43dd8c441ebb96ce5"}}%                                                             (in-toto) chris motoko ~/export/github/in-toto 17:01:54 6cfa36cf shibumi/fix-ed25519-keyid-in-public-key  
❯ cat bob 
{"keytype": "ed25519", "scheme": "ed25519", "keyid": "a1bcfbc3cf506b2a2ac974b679da58a3bde354a4acd23998ba2fdb60c011ba17", "keyid_hash_algorithms": ["sha256", "sha512"], "keyval": {"public": "07dea440b3e151f02a4e80296863f75a0bc0e46f61d7f4e43dd8c441ebb96ce5", "private": "5aaa95fc5b72bbf1792a8021f7622034d63b8e3099cbe91f577b161fe3042e8c"}}%    

As you can see, we do include keyid's now for ed25519. Does in-toto-keygen support ecdsa?? Because the help states otherwise..

[lukpueh]

Thanks for pointing out this inconsistency and for proposing a fix, @shibumi! Your initiative is highly appreciated, however, I am against changing our custom public key format without a discussion why we came up with this format in the first place, and if the assumptions from back then still hold true.

If we make a potentially breaking change, we might want to consider switching to a standard format altogether (we do use PEM for RSA keys on disk), or at least revise the included fields, e.g. why do ed25519/ecdsa private keys on disk include a keyid field and public keys don't, or why do both not include the keyid_hash_algorithms field, etc.... (see theupdateframework/python-tuf#848 for discussion about the latter field).

Right now, securesystemslib and its dependents do know how to handle their keys. We can load public keys from disk (without keyid), keep them in memory (with keyid) and add them to in-toto layout metadata (with keyid), or in tuf root/targets metadata (without keyid).

It would be good to make it very clear, when we use which format why. The various securesystemslib.formats.*KEY_SCHEMAs, are not particularly helpful for that. Some of them require a keyid and other don't which (unexpectedly) makes it optional (see #183 (comment)).

Moreover, the way we currently use the "serialization" function format_keyval_to_metadata looks like a lot of unnecessary work, e.g.:

# simplified and slightly exaggerated for emphasis
a = key["a"]
b = key["b"]
c = key["c"]
metadata = format_keyval_to_metadata(a, b, c)
# metadata is {"a": "...", "b": "...", c: "...", d: "..."}

cc @mnm678, @SantiagoTorres, @trishankatdatadog

[shibumi]

Thanks for pointing out this inconsistency and for proposing a fix, @shibumi! Your initiative is highly appreciated, however, I am against changing our custom public key format without a discussion why we came up with this format in the first place, and if the assumptions from back then still hold true.

Don't worry, I am good :D This was more of a quick fix. The discussion alone about this topic is more important. Do we want to keep this discussion in the PR or shall I compile an issue for it?

[lukpueh]

An issue would be most welcome! Feel free to copy-paste whatever I wrote here or on slack. :)

Btw. we already had a related issue, but I think I closed it due to it being partially addressed by our GPG subpackage and RSA key files being standard PEM. Here it is: #55

[shibumi]

closing this PR, because discussion should be moved to the related issue.