Switch from golang.org/x/crypto/openpgp to github.com/ProtonMail/go-crypto

[MasonM]

This replaces golang.org/x/crypto/openpgp with github.com/ProtonMail/go-crypto, since the former has been deprecated (see https://github.com/golang/go/issues/44226), and the latter is a (mostly) backwards-compatible fork. I'm not a cryptographer and haven't audited github.com/ProtonMail/go-crypto for correctness, but it's already used in several other HashiCorp projects (https://github.com/search?q=org%3Ahashicorp+go-crypto&type=code), so I assume it's been reviewed and approved by HashiCorp.

One improvement this brings is support for ECC keys, which has been requested before (https://github.com/hashicorp/terraform-docs-common/issues/106). I've already verified this works correctly with a private registry.

There is one change in the fork that's arguably backwards-incompatible: CheckDetachedSignature() will now return an error if the key used to sign the signature is expired (see https://github.com/ProtonMail/go-crypto/pull/60). This caused the TestNewSignatureAuthentication_success test to fail, since it was signed with an expired subkey. You can verify this on the command line with these commands, which replicates the test:

$ echo $'fea4227271ebf7d9e2b61b89ce2328c7262acd9fd190e1fd6d15a591abfa848e  terraform-provider-null_3.1.0_darwin_amd64.zip\n9ebf4d9704faba06b3ec7242c773c0fbfe12d62db7d00356d4f55385fc69bfb2  terraform-provider-null_3.1.0_darwin_arm64.zip\na6576c81adc70326e4e1c999c04ad9ca37113a6e925aefab4765e5a5198efa7e  terraform-provider-null_3.1.0_freebsd_386.zip\n5f9200bf708913621d0f6514179d89700e9aa3097c77dac730e8ba6e5901d521  terraform-provider-null_3.1.0_freebsd_amd64.zip\nfc39cc1fe71234a0b0369d5c5c7f876c71b956d23d7d6f518289737a001ba69b  terraform-provider-null_3.1.0_freebsd_arm.zip\nc797744d08a5307d50210e0454f91ca4d1c7621c68740441cf4579390452321d  terraform-provider-null_3.1.0_linux_386.zip\n53e30545ff8926a8e30ad30648991ca8b93b6fa496272cd23b26763c8ee84515  terraform-provider-null_3.1.0_linux_amd64.zip\ncecb6a304046df34c11229f20a80b24b1603960b794d68361a67c5efe58e62b8  terraform-provider-null_3.1.0_linux_arm64.zip\ne1371aa1e502000d9974cfaff5be4cfa02f47b17400005a16f14d2ef30dc2a70  terraform-provider-null_3.1.0_linux_arm.zip\na8a42d13346347aff6c63a37cda9b2c6aa5cc384a55b2fe6d6adfa390e609c53  terraform-provider-null_3.1.0_windows_386.zip\n02a1675fd8de126a00460942aaae242e65ca3380b5bb192e8773ef3da9073fd2  terraform-provider-null_3.1.0_windows_amd64.zip' > SHA256SUMS

$ echo -n 'wsFcBAABCAAQBQJgga+GCRCwtEEJdoW2dgAAo0YQAAW911BGDr2WHLo5NwcZenwHyxL5DX9g+4BknKbc/WxRC1hD8Afi3eygZk1yR6eT4Gp2HyNOwCjGL1PTONBumMfj9udIeuX8onrJMMvjFHh+bORGxBi4FKr4V3b2ZV1IYOjWMEyyTGRDvwSCdxBkp3apH3s2xZLmRoAj84JZ4KaxGF7hlT0j4IkNyQKd2T5cCByN9DV80+x+HtzaOieFwJL97iyGj6aznXfKfslK6S4oIrVTwyLTrQbxSxA0LsdUjRPHnJamL3sFOG77qUEUoXG3r61yi5vWV4P5gCH/+C+VkfGHqaB1s0jHYLxoTEXtwthe66MydDBPe2Hd0J12u9ppOIeK3leeb4uiixWIirNdpWyjr/LU1KKWPxsDqMGYJ9TexyWkXjEpYmIEiY1Rxar8jrLh+FqVAhxRJajjgSRu5pZj50CNeKmmbyolLhPCmICjYYU/xKPGXSyDFqonVVyMWCSpO+8F38OmwDQHIk5AWyc8hPOAZ+g5N95cfUAzEqlvmNvVHQIU40Y6/Ip2HZzzFCLKQkMP1aDakYHq5w4ZO/ucjhKuoh1HDQMuMnZSu4eo2nMTBzYZnUxwtROrJZF1t103avbmP2QE/GaPvLIQn7o5WMV3ZcPCJ+szzzby7H2e33WIynrY/95ensBxh7mGFbcQ1C59b5o7viwIaaY2' | base64 -d > SHA256SUMS.sig

$ gpg --verify SHA256SUMS.sig SHA256SUMS
gpg: Signature made Thu Apr 22 10:16:54 2021 PDT
gpg:                using RSA key B0B441097685B676
gpg: Good signature from "HashiCorp Security (hashicorp.com/security) <[email protected]>" [unknown]
gpg: Note: This key has expired!
Primary key fingerprint: C874 011F 0AB4 0511 0D02  1055 3436 5D94 72D7 468F
     Subkey fingerprint: B36C BA91 A2C0 730C 435F  C280 B0B4 4109 7685 B676

I removed TestNewSignatureAuthentication_success entirely, since I don't have access to the private key to generate a new signature, and it's largely a duplicate of TestSignatureAuthentication_success anyway. If someone can give me an updated signature, I'm happy to restore the test.

Target Release

1.4.x

Draft CHANGELOG entry

NEW FEATURES

• Support ECC signing keys for providers published to a Terraform Registry

[hashicorp-cla]

All committers have signed the CLA.

[MasonM]

@alisdair Would you mind reviewing this when you have the chance? Thanks in advance!

[crw]

@MasonM this was on the list to triage in October and got missed - human error, my apologies. I will bring this back into the queue and also run this past our security team. Thanks!

[crw]

@MasonM, the feedback from the security team is as follows. Although there are no strong objections to using the ProtonMail/go-crypto, there are also no strong motivators to make that change at this time. Adding support for ECC introduces an additional "attack surface," meaning if it is not a product requirement, the balance of caution leads us to not make a change.

Do you have a specific use case for the ECC key feature being used with private registries? If so, I can make the "product requirement" case with our product manager to at least put this in the backlog. I noticed the original issue does not have any upvotes, but it is also in a relatively obscure repository and is speaking more towards the HashiCorp Registry, the roadmap of which we do not control.

In any case, thanks for this contribution and sparking this conversation. I am happy to keep this alive in the backlog just in case we come around on the need for ECC keys.

[MasonM]

@crw Thanks for getting back to me. The use case I have is integrating with a private Terraform registry at my workplace. The registry uses a custom implementation of the Provider Registry Protocol, similar to https://github.com/outsideris/citizen. The registry protocol requires a GPG signing key, but it doesn't specify it has to be an RSA key. So, when I went to publish a provider to the registry, I used an ECC key, since that's the default in OpenPGP. The registry accepted the ECC key fine, but Terraform gave the following error when trying to use it:

$ terraform init

Initializing the backend...

Initializing provider plugins...
- Finding registry.example.com/example/example versions matching "0.0.2"...
- Installing registry.example.com/example/example v0.0.2...
╷
│ Error: Failed to install provider
│
│ Error while installing registry.example.com/example/example v0.0.2: error decoding signing key: openpgp: unsupported feature: public key type: 22
╵

I was able to workaround that by switching to an RSA key, but I think this is a rather confusing gotcha. If the security team doesn't feel this use case is important enough to warrant the additional attack surface of ECC support, then that's perfectly understandable, but I would suggest updating the protocol specification to make it clear that ECC keys are not allowed. In any case, thank you for your time!

[MasonM]

There was a duplicate PR for this entered last week: https://github.com/hashicorp/terraform/pull/33131

The one difference is that instead of removing the TestNewSignatureAuthentication_success test, it mocks out the clock to avoid signature expiration issues. Personally, I think it's better to just delete it, since it's duplicating TestSignatureAuthentication_success, but I'm happy to copy over that to this PR if anyone disagrees.

[rolandshoemaker]

@crw I know your Security team has already weighed in on this (per https://github.com/hashicorp/terraform/pull/32056#issuecomment-1402906963), but does the additional context provided in https://github.com/hashicorp/terraform/pull/33131#issuecomment-1531749336 move the needle at all?

We (the Go Security team) would really like to see people moving away from golang.org/x/crypto/openpgp for the good of the ecosystem, but also for selfish reasons as we are forced to carry internal patches for this.

[crw]

Hi @rolandshoemaker, thanks for that additional context.

but also for selfish reasons as we are forced to carry internal patches for this.

This is also important context. I'll re-raise with the team.

[liamcervante]

Hi @MasonM, would you mind pulling the latest from main and resolving the conflicts in go.mod and go.sum. I will then look into merging this!

[liamcervante]

Actually looking at the two PRs, 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!

[github-actions[bot]]

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.