keyids for new keys are not spec compliant

theupdateframework / tuf-on-ci

A TUF repository and signing tool

Other

20 stars 11 forks source link

keyids for new keys are not spec compliant #292

Closed jku closed 3 months ago

jku commented 4 months ago

Spec currently demands that keyids match sha256 of the canonicalized key content and that clients verify that
- This is widely regarded as a mistake (see e.g. TAP 12) and the verification is typically not implemented in clients but the requirement is still in the spec... and apparently at least tuftool from tough project does the verification
- tuf-on-ci-sign currently
- generates a new key: this assigns a keyid that is correct
- later inserts a custom field (x-tuf-on-ci-keyowner or x-tuf-on-ci-online-uri) into the key : this makes the keyid incorrect WRT spec

jku commented 4 months ago

Fixing the initial key creation in tuf-on-ci seems doable: make a wrapper for modifying the custom fields, have the wrapper also re-calculate the keyid

There are a couple of issues though:

"fixing" metadata in existing tuf-on-ci repositories like roots-signing-staging seems tricky for root keys: we essentially want to do a key rotation from keyid 1 to keyid 2 where both keyids refer to same underlying key... but this is also explicitly banned by the spec
It seems we'd need a two step process:
- first rotate root keys to a temporary key
- then rotate to the original keys but now with compliant keyid
in the future, any changes to the fields of the key json (like an online key url changing) would require keyid change

jku commented 4 months ago

The alternative "solution" is to push for a spec change:

the requirement to verify keyid calculation seems to have no security benefits
most clients do not implement it
it adds one more complication to client workflow (which should be simpler, not more complicated)
it clearly seems to be a head ache for repository maintenance

kommendorkapten commented 4 months ago

This is quite unfortunate, and as @jku points out, having a good set of conformance tests would have helped us here, as apparently almost no client is verifying the key ids so this bug didn't get surfaced.

jku commented 4 months ago

having a good set of conformance tests would have helped us here, as apparently almost no client is verifying the key ids so this bug didn't get surfaced.

I'm probably partly to blame for that since I've been quite vocal about my distaste for the keyid calculation requirement... That said, if we had had a conformance test suite it would have pushed us to make the spec change or at least clearly document this (instead of just ignoring that bit of the spec).

I filed the spec issue as well https://github.com/theupdateframework/specification/issues/305

jku commented 3 months ago

Recap:

Some clients enforce the keyid calculation which means they will not accept repositories created before this fix
After these fixes, new keys are now created with compliant keyids
import-repository now changes keyids so they stay compliant
a --force-compliant-keyids flag was added to tuf-on-ci-delegate: This can be used to create a signing event that fixes already published root and targets metadata