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

bump github.com/theupdateframework/go-tuf to v0.6.0 and update deprecated package #3128

Closed

Conversation

cpanato
Copy link
Member

@cpanato cpanato commented Jul 21, 2023

Summary

  • bump github.com/theupdateframework/go-tuf to v0.6.0 and update deprecated package

@mtrmac
Copy link
Contributor

mtrmac commented Jul 21, 2023

To be explicit, after theupdateframework/go-tuf#467 this changes the parameters in generated keys in such a way that the private keys can’t be used by older binaries (new binaries which update to this …/encrypted can continue to consume old encrypted private keys).

I don’t have a strong opinion on whether to make this change, I would just like to make sure it is an intentional one.

@cpanato
Copy link
Member Author

cpanato commented Jul 24, 2023

@haydentherapper
Copy link
Contributor

Thanks for the heads up @mtrmac. If I'm understanding correctly, this would be a major breaking change for Cosign since all previously generated keys would become unusable.

My feeling is that the outdated scrypt parameters are not a high priority risk because the encrypted private key is hard to brute force, so we should not break existing key users. If we want to start a migration to this new library, we should a) split the marshal function into Read and Create, b) switch Create to use the new encrypted library, c) have Read try to decrypt with both the new and old encrypted library, d) update places where we call into marshalKey to use either Read or Create.

@znewman01 fyi

@mtrmac
Copy link
Contributor

mtrmac commented Jul 24, 2023

If I'm understanding correctly, this would be a major breaking change for Cosign since all previously generated keys would become unusable.

(Warning: I didn’t actually test that.) No, I understand the breakage goes the other way: all newly generated keys would not be usable by old Cosign (and Podman) binaries.

@mtrmac
Copy link
Contributor

mtrmac commented Jul 24, 2023

If we want to start a migration to this new library

To keep the old compatible format, use EncryptWithCustomKDFParameters(…, Legacy). The new Decrypt can read both the old and the new format. No need to keep both implementations around.

(And that wouldn’t help anyway, because the version in go-tuf was also updated exactly the same way. Calling the old implementation would require pinning a previous version of go-tuf, or forking.)

@haydentherapper
Copy link
Contributor

To keep the old compatible format, use EncryptWithCustomKDFParameters(…, Legacy). The new Decrypt can read both the old and the new format. No need to keep both implementations around.

Sweet, that would be perfect, let's use that for now. It is a little annoying for the verifier if anyone is distributing their Cosign public key and the verifier needs to update Cosign, so I'd prefer we do that in a separate PR after some more discussion around semver.

@mtrmac
Copy link
Contributor

mtrmac commented Jul 24, 2023

It is a little annoying for the verifier if anyone is distributing their Cosign public key

This only affects the private key . It is annoying all the same, but perhaps fewer people.

@znewman01
Copy link
Contributor

Yeah, the only issue with the change in key format is if

  1. You create a key with Cosign after this change.
  2. You're trying to read the private key with an old version of Cosign.

In general, I'm comfortable saying we don't need backwards compat for new keys. But in the interest of getting this merged, let's just use the EncryptWithCustomKDFParameters(…, Legacy) for now and open an issue to change that.

@znewman01
Copy link
Contributor

Note: don't merge until we understand theupdateframework/go-tuf#527 fully

@haydentherapper
Copy link
Contributor

sigstore/sigstore has been updated to v1.7.2 with a fix, or we can wait for go-tuf's 0.6.1 patch. Let's use EncryptWithCustomKDFParameters for now.

@haydentherapper
Copy link
Contributor

Closing in favor of #3183

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.

4 participants