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

Be less strict in keyid validity check ? #766

Open
jku opened this issue Apr 26, 2024 · 3 comments
Open

Be less strict in keyid validity check ? #766

jku opened this issue Apr 26, 2024 · 3 comments

Comments

@jku
Copy link

jku commented Apr 26, 2024

I'm wondering if you'd accept a patch that makes the key deserialization checks a little bit less strict...

I've tried to write down all of the the context in this spec issue theupdateframework/specification#305 but the short of it is that

  • clients calculating the keyid from the canonicalized key content (see deserialize_keys()) as the spec instructs seems to have no security benefit
  • it does have significant potential for compatibility issues
  • and can be a headache for repository implementations

The practical reason I'm filing this now is this:

  • tuf-on-ci (the repository implementation I maintain) had a bug that produced keys with keyids that are not strictly conformant:
  • I intend to fix this bug but in the mean time root-signing-staging (Sigstores staging TUF repository) already has metadata with these keyids
  • none of the existing sigstore clients seem to verify the keyid calculation so we did not notice, and as documented in the linked issue there seems to be rough consensus in the TUF community that keyid calculation is not useful

So I'm trying to clean up my mess, wondering if (in addition to fixing the bug in tuf-on-ci) part of that could be a patch to tough that removes the keyid calculation check from the key deserialization as a way to improve practical compatibility between the implementations.

@rpkelly
Copy link
Contributor

rpkelly commented May 7, 2024

Hi, thanks for reaching out! We're reviewing this proposal with our security team and will be able to give you an answer once we've gone through that process.

@webern
Copy link
Contributor

webern commented May 13, 2024

Still reviewing. In reading the TAP12 proposal, as I understand it, I agree with the change. Most likely we would prefer to see the change incorporated into the spec before making the change here.

One thing not entirely clear to me @jku, are there downstream users of this library that are blocked by the enforcement of the KeyID calculation? I saw that sigstore uses this library in its "experiemental" Rust client. Is that causing blockers or failures now?

@jku
Copy link
Author

jku commented May 14, 2024

Cheers, I can understand that view point. I will likely try to push the linked spec issue forward before the TAP to make the spec intent clear but your point applies there as well: you'd rather see it in the spec first...

So the situation is that

  • sigstore staging TUF repo now has these incorrect keyids in their current root metadata versions, this is unlikely to revert
  • this is annoying but not a complete blocker since A) this is a staging repo, we don't intend to reproduce the issue in production and B) once we figure out how to create new root metadata that is compliant in a smooth upgrade, the experimental rust client can again use the staging repo as long as it starts with root N+1

This is blocking the developers of the rust sigstore client from using sigstores staging infrastructure at the moment but we should be able to work around the situation.

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

No branches or pull requests

3 participants