Made internal keyids consistent

[KOLANICH]

Description of the changes being introduced by the pull request:

It was noticed that internal keyids for ECDSA keys get inconsistent when imported from PEM because hazmat either adds nonsignificant whitespaces or not. securesystemlib currently forms internal keyids by hashing PEM representations (which is IMHO incorrect), so the keyids are affected by nonsignificant changes. This PR just strips the PEM representations of trailing whitespaces. The proper solution to the problem is to generate the keyids from significant portions of keys only and to make them compatible to other impls, but it is not my job.

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 - not my job, the lib was undertested initially
• Docs have been added for the bug fix or new feature - not needed, no API changes

[jku]

I think this looks ok. Can you confirm if this fixes a practical issue or if it's just a cosmetic fix: even if cryptography sometimes includes whitespace in the pem, why do we care? Does something break?

I think roughly everyone agrees that keyids are not ideal: IMO they should not be tied to key content at all and should be just identifiers. If I recall correctly TAP-12 is the attempt at making that the rule in TUF.

[KOLANICH]

Can you confirm if this fixes a practical issue or if it's just a cosmetic fix: even if cryptography sometimes includes whitespace in the pem, why do we care? Does something break?

It breaks the tests for importing SSH keys. When one imports a public key to hazmat and exports them to PEM, and one imports a private key to hazmat, converts it to public one and exports to PEM, the PEM reprs differ by a line break character in the end. Currently securesystemslib computes keyids as hashes of PEM reprs.

[jku]

I see. The PR seems fine to me but my suggestion is that you should not expect the keyid to be reproducible like that: I don't think the implementation is very robust so things like modifying values in securesystemslib.settings will break that reproducibility...

[KOLANICH]

The PR seems fine to me but my suggestion is that you should not expect the keyid to be reproducible like that

I know, for example base64 reprs of keys usually ignore line breaks, so changing the line wrap width will also break the keyids. Ideally the field should be immediately renamed into an internal_inconsistent_keyid to make it clear that people should not rely on them (and break the apps of those who relies), and then implement consistent keyids matching the ones external tools print and phase out the internal keyids completely.

So this PR just relieves some of the sympthoms.

[lukpueh]

Thanks for the patch, @KOLANICH! I think the patch makes sense, but I wonder if it is all that helpful, given the more substantial issues with keyids discussed above.

If this is currently only needed for #451, then I suggest to defer until we know how to proceed with #451. See #451 (comment)

[KOLANICH]

If this is currently only needed for #451

It is, but I'm not sure that working around the lack of this patch via monkey-patching makes sense. Of course the keyids will be fixed somewhen and all the impls (including tuf) relying on them them will break. But this is inevitable (an alternative to keep compatibility to already broken and inconsistent keyid calculation IMHO makes no sense). When this happens I'd have to (and other repo owners, if there is any, it will likely not gain large popularity before the New Year, so OK) just regenerate the tuf repo for my tool from scratch and reimport the repos (should be easy, the tool currently uses ToFU trust model).

[KOLANICH]

Worked around for 1 key type with monkey-patching.

https://github.com/KOLANICH/securesystemslib_KOLANICH.py

[jku]

I don't think I agree with the commit message: No-one seems to be interested enough to actually implement the keyid generation (for all keytypes) in the way current TUF spec defines it -- in other words the spec should be changed to not have the strict requirements and just require that keyids are unique per delegating role. I think in future securesystemslib should make it clear that users should not expect the generated keyids to be any kind of "stable API": it's just a way to get hopefully unique identifiers.

That said, the actual patch seems fine: it only modifies the output from key generation, so should not break code? Let's either merge or not, and then close this PR.