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.

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

Fix noncompliant keyids #338

Merged
merged 3 commits into from
May 27, 2024
Merged

Conversation

jku
Copy link
Member

@jku jku commented May 17, 2024

Fixes #336 , relates to #292

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

  • Make sure signing works with multiple keys (the UI for identifying which key is used isn't there yet but that's ok since so far this will only be used for the non-compliant keyid fix where the actual signing key is the same one, just with two different keyids)
  • Add a hidden --force-compliant-keyids flag to delegate: this will update all keyids in the role and will mark all affected delegations to be resigned

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

  • root v1 has one root signer with key A using non-compliant keyid abcd. The metadata signatures contains a sig from abcd
  • tuf-on-ci-delegate --force-compliant-keyids sign/fix-keyids root is used: the signing event now contains a root v2 with one root signer with key A using valid keyid efgh (same key, different keyid)
  • the signer runs tuf-on-ci-sign sign/fix-keyids as usual: this signs twice with A; The metadata signatures now contain a sig from abcd and a sig from efgh
  • when client see root v2 they will ensure it is signed by old signers (abcd) and new signers (efgh). The fact that this happens to be the same key should not be an issue (since the keys are part of two different threshold calculations)

jku added 2 commits May 17, 2024 14:24
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.
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
Copy link
Member Author

jku commented May 17, 2024

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

@kommendorkapten
Copy link
Member

Code looks reasonable! Waiting for more test results.

@jku
Copy link
Member Author

jku commented May 20, 2024

Test in jku/tuf-demo#107:

  • delegate and sign worked as expected
  • online sign seems fine with the changed keyids
  • smoke test is good
  • if I can easily build the rust client (which has the keyid calculation) I will test that one as well

@jku
Copy link
Member Author

jku commented May 21, 2024

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

jku commented May 21, 2024

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

jku commented May 23, 2024

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 jku marked this pull request as draft May 23, 2024 10:52
@jku

This comment was marked as outdated.

@jku
Copy link
Member Author

jku commented May 23, 2024

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:

  • the import works nicely if the signers for root N and N+1 are the same (tuf-on-ci can only know who the N+1 signers are but this does not matter because the signers are the same)
  • the keyid fix with tuf-on-ci-delegate --force-compliant-keyids also work nicely since from tuf-on-ci perspective the N and N+1 signers are completely different: this leads to two signatures for each signer whose keyids was modified
  • 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?

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 jku force-pushed the fix-noncompliant-keyids branch from 650fad9 to 5f3d3fa Compare May 24, 2024 14:29
@jku jku marked this pull request as ready for review May 24, 2024 14:30
@jku
Copy link
Member Author

jku commented May 24, 2024

Example import signing event jku/root-signing-test#16

  • this is a root-signing lookalike where I am signer for everything
  • note how the keys remain same, but keyids change because the keys custom metadata changed
  • the old keyid still signs root because of the special case code

@kommendorkapten
Copy link
Member

kommendorkapten commented May 27, 2024

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

jku commented May 27, 2024

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.

@jku jku merged commit 8c503d9 into theupdateframework:main May 27, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

signer: add feature to fix non-compliant keyids
2 participants