theupdateframework / tuf-on-ci

A TUF repository and signing tool
Other
20 stars 11 forks source link

Fix noncompliant keyids #338

Closed jku closed 4 months ago

jku commented 4 months ago

Fixes #336 , relates to #292

This change will ensure that a a keyid fix can be done in a single signing event

This approach (just switching the all keyids) looks surprising but should be 100% spec compliant. Example:

jku commented 4 months ago

I'm still going to do a bit more manual testing but I think this will work

kommendorkapten commented 4 months ago

Code looks reasonable! Waiting for more test results.

jku commented 4 months ago

Test in https://github.com/jku/tuf-demo/pull/107:

jku commented 4 months ago

awslabs/tough client does now "succeed" in that root is considered valid but

$ tuftool download --root 3.root.json -m https://jku.github.io/tuf-demo/metadata/ -t https://jku.github.io/tuf-demo/targets -n file1.txt out/`
Failed to load repository: Failed to parse timestamp metadata: missing field `length` at line 14 column 4

This one I'm not as inclined to "fix" in tuf-on-ci. I don't know if my decision to avoid lengths and hashes in timestamp and snapshot is the best choice but both options seem to have upsides and downsides and both are spec compliant

jku commented 4 months ago

@kommendorkapten I'm done I think: As far as I can tell the code is good. and the results metadata update is solid. awslabs/tough tuftool still doesn't fully work, but the specific issue of noncompliant keyids seems to gets solved by this code

jku commented 4 months ago

Hmm, actually tuf-on-ci-repo-import could now also modify the keyids immediately when the custom metadata is added... I'll try to add this in this PR.

jku commented 4 months ago

Hmm, actually tuf-on-ci-repo-import could now also modify the keyids immediately when the custom metadata is added... I'll try to add this in this PR.

the change is simple but I think I have to rerun the root-signing import test to verify this

jku commented 4 months ago

Hmm, actually tuf-on-ci-repo-import could now also modify the keyids immediately when the custom metadata is added... I'll try to add this in this PR.

There is a complication in the root case:

jku commented 4 months ago

Example import signing event https://github.com/jku/root-signing-test/pull/16

kommendorkapten commented 4 months ago

combining these two is an issue: how can tuf-on-ci-sign figure out it needs to sign for two keyids when both import and keyid fix have been done?

Maybe a hack but what if tuf-on-ci creates an in-memory representation of the yubikey using the "old format", i.e without x-tuf-on-ci-keyowner, and compares with the current root, and if a match, when signing it writes the signature to both entries?

jku commented 4 months ago

Maybe a hack but what if tuf-on-ci creates an in-memory representation of the yubikey using the "old format", i.e without x-tuf-on-ci-keyowner, and compares with the current root, and if a match, when signing it writes the signature to both entries?

We discussed this live but documenting for others: this is pretty much what the current PR does. For each signer it takes the "tuf-on-ci managed key", calculates keyid without the custom metadata, and looks for keys with that keyid in the non-managed (N-1) metadata. These keys will then also be used to sign.