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

internal/getproviders: switch to ProtonMail/go-crypto/openpgp #33131

Closed

Conversation

rolandshoemaker
Copy link

Switches away from the deprecated, unmaintained golang.org/x/crypto/openpgp module, and replaces it with the (mostly) drop-in, maintained, github.com/ProtonMail/go-crypto/openpgp.

The only logic change here is that CheckDetachedSignature now requires a openpgp/packet.Config, and enforces key/signature expirations. This means we now need a way to supply a config during testing, so that we can continue to use keys/signatures which are actually expired.

This is part of a wider effort by the Go Security team to remove uses of golang.org/x/crypto/openpgp from the Go ecosystem.

Switches away from the deprecated, unmaintained golang.org/x/crypto/openpgp
module, and replaces it with the (mostly) drop-in, maintained,
github.com/ProtonMail/go-crypto/openpgp.

The only logic change here is that CheckDetachedSignature now requires a
openpgp/packet.Config, and enforces key/signature expirations. This
means we now need a way to supply a config during testing, so that we
can continue to use keys/signatures which are actually expired.

This is part of a wider effort by the Go Security team to remove uses of
golang.org/x/crypto/openpgp from the Go ecosystem.
@hashicorp-cla
Copy link

hashicorp-cla commented May 1, 2023

CLA assistant check
All committers have signed the CLA.

@radeksimko
Copy link
Member

Related: #32056

@rolandshoemaker
Copy link
Author

rolandshoemaker commented May 2, 2023

I understand this may not be a simple decision, so I'll try to provide some further context that may be valuable in terms of guiding the conversation. At Google we are working internally to replace all usages of golang.org/x/crypto/openpgp with github.com/ProtonMail/go-crypto/openpgp, the maintainers of which we have worked with to make the module as much of a drop-in replacement as possible.

golang.org/x/crypto/openpgp was deprecated for a handful of reasons (see golang/go#44226), namely that there were no active maintainers, and no appetite from the core Go team to take over ownership. While the Security team is willing to fix critical security issues that are reported to us, there are a number of known issues in the package that we have no intention of prioritizing (see this search), and a number of inherent API design flaws that cannot be solved in a backwards compatible way (hence why github.com/ProtonMail/go-crypto/openpgp cannot be a truly drop-in replacement).

While github.com/ProtonMail/go-crypto/openpgp does increase the scope of supported functionality, I believe it is on balance a security win to switch to, as it removes a number of openpgp foot-guns (namely it actually enforces key and signature validity periods, which golang.org/x/crypto/openpgp just ignores), and is actively maintained.

Feel free to send a message to [email protected] if there are any questions you'd like to ask privately.

@crw crw added the enhancement label May 2, 2023
@MasonM
Copy link

MasonM commented May 10, 2023

This appears to be a duplicate of #32056, except it lacks a test for ECC key support

@crw
Copy link
Contributor

crw commented May 10, 2023

Thanks @MasonM, we were aware although it helps to have the details posted in both tickets. I've left both open so as to not shut down the ongoing conversation, including the further arguments made by @rolandshoemaker in #33131 (comment).

@rolandshoemaker
Copy link
Author

Sorry @MasonM, I hadn't seen your PR when working on this, and am happy to defer to your change. @crw I will leave it up to you what to do with this change (i.e. if you want to leave it open for contextual purposes), but I'll move over to #32056 for further comments.

@crw
Copy link
Contributor

crw commented Jun 20, 2023

Thanks for that comment @rolandshoemaker - I am tracking both of these, and will keep both in the conversation when we are deciding which to review.

@liamcervante
Copy link
Member

Looking at the two PRs that fix this issue, I think it's easier for me to create a new one that merges both together as both carry parts that I think are useful. I've raised #33406 with that in mind, and we'll close everything out in there.

Thanks for raising this with us, we really do appreciate the support!

Copy link
Contributor

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants