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

proposal: x/crypto/openpgp: Remove or fix signing support #57797

Open
DemiMarie opened this issue Jan 14, 2023 · 4 comments
Open

proposal: x/crypto/openpgp: Remove or fix signing support #57797

DemiMarie opened this issue Jan 14, 2023 · 4 comments
Labels
Milestone

Comments

@DemiMarie
Copy link

x/crypto/openpgp may be deprecated, but the broken signatures that it generates are causing problems in the OpenPGP ecosystem. I recommend one of the following either making x/crypto/openpgp produce correct signatures, or removing signing support from x/crypto/openpgp altogether.

@gopherbot gopherbot added this to the Proposal milestone Jan 14, 2023
@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Jan 14, 2023

/cc @golang/security

@FiloSottile
Copy link
Contributor

We don't remove functionality per the Go 1 Compatibility Promise.

I'm open to applying fixes for ecosystem-disrupting issues, but from a quick look we have two not-great options:

  1. start rejecting incorrectly serialized MPIs, which means we will suddenly reject signatures we produced in a previous version, which is likely to break users
  2. keep accepting incorrectly serialized MPIs, but serialize them correctly, which is likely to cause bugs where a structure gets parsed from an incorrect encoding and re-serialized to a correct encoding (e.g. for hashing or signature verification), causing a mismatch, which is likely to break users

This kind of lose-lose problems are part of why we deprecated the package and encouraged moving to a different one.

@DemiMarie
Copy link
Author

We don't remove functionality per the Go 1 Compatibility Promise.

Is this package subject to that guarantee? Arguably, it isn’t really usable in practice, not least due to the lack of ed25519 support.

I'm open to applying fixes for ecosystem-disrupting issues, but from a quick look we have two not-great options:

  1. start rejecting incorrectly serialized MPIs, which means we will suddenly reject signatures we produced in a previous version, which is likely to break users
  2. keep accepting incorrectly serialized MPIs, but serialize them correctly, which is likely to cause bugs where a structure gets parsed from an incorrect encoding and re-serialized to a correct encoding (e.g. for hashing or signature verification), causing a mismatch, which is likely to break users

What about storing the original byte representation, so that round-trip stability is not a requirement? That was also recommended as a solution to problems with encoding/xml.

This kind of lose-lose problems are part of why we deprecated the package and encouraged moving to a different one.

Maybe go vet should throw a warning if this package is imported.

@FiloSottile
Copy link
Contributor

Is this package subject to that guarantee? Arguably, it isn’t really usable in practice, not least due to the lack of ed25519 support.

If it wasn't used it wouldn't matter what signatures it generates. x/crypto is a large module, and breaking functionality would block developers from upgrading other packages, including to apply security patches. It's not an option.

If we ever make a v2, the openpgp package will not be in it.

I'm open to applying fixes for ecosystem-disrupting issues, but from a quick look we have two not-great options:

  1. start rejecting incorrectly serialized MPIs, which means we will suddenly reject signatures we produced in a previous version, which is likely to break users
  2. keep accepting incorrectly serialized MPIs, but serialize them correctly, which is likely to cause bugs where a structure gets parsed from an incorrect encoding and re-serialized to a correct encoding (e.g. for hashing or signature verification), causing a mismatch, which is likely to break users

What about storing the original byte representation, so that round-trip stability is not a requirement? That was also recommended as a solution to problems with encoding/xml.

That is indeed the correct way to do things, but it's not how x/crypto/openpgp works, and changing it would be a significant effort. (Or maybe not, and someone can show that in a CL!)

Maybe go vet should throw a warning if this package is imported.

go vet does not show deprecation warnings, but staticcheck does, and it's on by default in gopls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

4 participants