Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

keyids for new keys are not spec compliant #292

Closed
jku opened this issue Apr 26, 2024 · 5 comments · Fixed by #294
Closed

keyids for new keys are not spec compliant #292

jku opened this issue Apr 26, 2024 · 5 comments · Fixed by #294

Comments

@jku
Copy link
Member

jku commented Apr 26, 2024 •
edited
Loading

  • 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
Copy link
Member Author

jku commented Apr 26, 2024 •
edited
Loading

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
Copy link
Member Author

jku commented Apr 26, 2024

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

This was referenced Apr 26, 2024
Draft
Open
@kommendorkapten
Copy link
Member

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
Copy link
Member Author

jku commented Apr 29, 2024

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 theupdateframework/specification#305

@jku jku linked a pull request Apr 29, 2024 that will close this issue
signer: Make sure initial keyid is spec-compliant #294
Merged
@jku jku closed this as completed in #294 May 16, 2024
@jku jku mentioned this issue May 16, 2024
Closed
jku added a commit to jku/tuf-on-ci that referenced this issue May 17, 2024
@jku
signer: Allow multiple signatures
d3ff208 
This makes it possible to sign the same metadata twice:
* currently this is only useful when fixing the keyid compliance issue
  in root (see theupdateframework#292). Basically the user will be asked to sign with
  both the keyid from root N+1 and the keyid from root N.
* there are clear use cases with one signer with multiple keys in
  future (think e.g. key rotation).
jku added a commit to jku/tuf-on-ci that referenced this issue May 17, 2024
@jku
signer: Allow multiple signatures
653d58c 
This makes it possible to sign the same metadata twice:
* currently this is only useful when fixing the keyid compliance issue
  in root (see theupdateframework#292). Basically the user will be asked to sign with
  both the keyid from root N+1 and the keyid from root N.
* there are clear use cases with one signer with multiple keys in
  future (think e.g. key rotation).
jku added a commit to jku/tuf-on-ci that referenced this issue May 17, 2024
@jku
signer: Allow multiple signatures
6ad9ec3 
This makes it possible to sign the same metadata twice:
* currently this is only useful when fixing the keyid compliance issue
  in root (see theupdateframework#292). Basically the user will be asked to sign with
  both the keyid from root N+1 and the keyid from root N.
* there are clear use cases with one signer with multiple keys in
  future (think e.g. key rotation).
jku added a commit to jku/tuf-on-ci that referenced this issue May 17, 2024
@jku
signer: Allow multiple signatures
32ac6fa 
This makes it possible to sign the same metadata twice:
* currently this is only useful when fixing the keyid compliance issue
  in root (see theupdateframework#292). Basically the user will be asked to sign with
  both the keyid from root N+1 and the keyid from root N.
* there are clear use cases with one signer with multiple keys in
  future (think e.g. key rotation).
This was referenced May 17, 2024
Merged
Closed
jku added a commit that referenced this issue May 27, 2024
@jku
Fix noncompliant keyids (#338)
8c503d9 
* signer: add hidden flag to fix keyid issue in -delegate

This modifies keyids in place: The key contents remain the same
* Unfortunately keys get shuffled around since they are sorted by keyid
* The result should be that same HW key will now sign the "new" keyids
* for root, the HW key will sign for both new keyids and old keyid
  (so that both root N and N+1 will reach threshold)

this command can be run on "root" and "targets" (and will fix all keyids
defined in that roles metadata). New versions of the delegated roles
will then be created to make sure they get signed with new keyids.

* signer: Allow multiple signatures

This makes it possible to sign the same metadata twice:
* currently this is only useful when fixing the keyid compliance issue
  in root (see #292). Basically the user will be asked to sign with
  both the keyid from root N+1 and the keyid from root N.
* there are clear use cases with one signer with multiple keys in
  future (think e.g. key rotation).

* signer: Also handle keyid fix during repo import

Since the import adds custom metadata into existing keys,
keyids become non-compliant. Run force_compliant_keyids() for
imported roles too.

This very annoyingly requires a special case in _sign(): basically
a heuristic that figures that we want to sign with "previous version"
root keys if the keyid of this legacy key can be calculated from
"current version" root key by just removing the custom metadata.
@jku
Copy link
Member Author

jku commented May 27, 2024

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

@jku jku mentioned this issue Jul 1, 2024
Closed
@joshuagl joshuagl mentioned this issue Jul 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

signer: Make sure initial keyid is spec-compliant jku/tuf-on-ci
2 participants
@jku @kommendorkapten