ProtonMail / go-crypto

Fork of go/x/crypto, providing an up-to-date OpenPGP implementation
https://pkg.go.dev/github.com/ProtonMail/go-crypto
BSD 3-Clause "New" or "Revised" License
334 stars 101 forks source link

Support for OpenPGP V3 signatures #164

Open haydentherapper opened 1 year ago

haydentherapper commented 1 year ago

I work on a project that verifies signed RPM packages, which still use OpenPGP V3 signatures. Both the Golang RPM library and our service's verifier have to use x/crypto/openpgp, but we'd like to migrate to using this library to pick up support for other key types, bug fixes, etc. Is there any interest in bringing back support for V3 signatures? We don't need generation of them, only verification.

twiss commented 1 year ago

Hey :wave: V3 signatures are really old, and even V4 signatures have been in the OpenPGP standard for almost two decades now, and a new version of the standard specifying V6 signatures is around the corner. Is there any possibility to suggest to the RPM folks to switch to V4 (or soon V6)? I think that should be tried first, otherwise we'll be stuck in the past forever.

haydentherapper commented 1 year ago

Thanks for the context! I'm going to try to find someone at RedHat who works on RPM, though I suspect some pushback to format changes since it's an old file type. Unfortunately we might still be stuck needing to verify older RPM packages, though if the argument can be made that V3 signatures are insecure, it might be sufficient to drop support for them. Do you happen to have any documentation that discusses any security concerns with V3 vs the newer formats?

andrewgdotcom commented 1 year ago

Lack of V3 support is a blocker for Hockeypuck. We need #165 but can't upgrade our dependency because of this.

andrewgdotcom commented 1 year ago

I think that should be tried first, otherwise we'll be stuck in the past forever.

Until the WG actively declares V3 unsound, we are obliged to handle v3 sigs in the wild. Even after everyone stops creating new ones, there is still value in verifying older ones.

haydentherapper commented 1 year ago

We have the same concern with RPM packages. Even if we can convince RPM to move off V3, we'll need to handle verification of existing packages.

andrewgdotcom commented 1 year ago

Support was removed in 18633fdee964ec24e222a86713a70a49f3315457, which was nearly three years ago.

twiss commented 1 year ago

Do you happen to have any documentation that discusses any security concerns with V3 vs the newer formats?

So - one issue is that V3 signatures only allow using RSA and DSA (and not ECDSA and EdDSA). Both of those algorithms are actually deprecated in draft-ietf-openpgp-crypto-refresh-08, which makes V3 signatures essentially indirectly deprecated as well (though perhaps that should be made more explicit).

To make matters worse, in my experience, V3 signatures are usually signed by 1024-bit keys. Though it's a slightly orthogonal point, the crypto refresh says:

An implementation MUST NOT encrypt, sign or verify using RSA keys of size less than 2048 bits.

We don't comply with this yet, but will have to do so in the future. At that point, I assume you'll again have the same issue of not being able to verify old signatures, and so will need some kind of migration path anyway, I think? So I'm not sure it's worth adding support for V3 signatures just for 2048+ bit RSA V3 signatures, of which I don't think there are that many?


We have the same concern with RPM packages. Even if we can convince RPM to move off V3, we'll need to handle verification of existing packages.

Someone told me last week that RPM at least as used by Fedora did move from generating V3 signatures to V4 signatures, and RPM now shows a warning when it's been asked to generate a V3 signature, which is something at least. Hopefully the number of new V3 signatures will thus decrease.


In general, we'd prefer to nudge signers to update their software in this way, rather than add back support for V3 signatures. But, I understand of course that if you have lots of V3 signatures today, that's an issue. I think, if you temporarily create a fork of go-crypto, adding back support during the time generating software is transitioning to V4, and simultaneously try to plan for no longer validating old insecure signatures, that would be ideal, IMHO :relaxed:

andrewgdotcom commented 1 year ago

It appears Sequoia added v3 support specifically because of this issue: https://sequoia-pgp.org/blog/2023/04/27/rpm-sequoia/

andrewgdotcom commented 1 year ago

RFC4880 specifically states:

   Implementations SHOULD accept V3 signatures.  Implementations SHOULD
   generate V4 signatures.

Since RFC4880 is (still!) the latest published standard, it is totally premature to remove support for V3 IMO. We should not remove support for things that are merely deprecated (not forbidden) in a WIP draft.

I tried to re-add support for V3 in the pgpkeys-eu fork, but I ran into a dependency issue that I can't track down the root cause of:

andrewg@barney s2k % go test
# github.com/ProtonMail/go-crypto/openpgp/s2k.test
golang.org/x/crypto/blake2b.supportsAVX2: relocation target runtime.support_avx2 not defined
golang.org/x/crypto/blake2b.supportsAVX: relocation target runtime.support_avx not defined
FAIL    github.com/ProtonMail/go-crypto/openpgp/s2k [build failed]
andrewg@barney s2k %

The internet suggests this can be fixed by updating a vendor dependency, but since there is no mention of "AVX" anywhere in the code I can't track down which indirect dependency is causing it.

twiss commented 1 year ago

Since RFC4880 is (still!) the latest published standard, it is totally premature to remove support for V3 IMO.

Yeah, I understand your point, but if we were to follow RFC4880 to the letter, there are many other quite absurd things we'd have to do, such as using TripleDES when a key doesn't indicate any algorithm preferences. It also says "Implementations MUST implement SHA-1", whereas we have been trying to move away from that as much as possible. In general, we have followed rfc4880bis (and now the crypto refresh) and deviated from RFC4880 already since the inception of this package, as the aim was to modernize the cryptography being used. We could probably make it more explicit in the README and project description that this is the goal, but I don't think we should change that goal back to following RFC4880 now.


Looking at https://github.com/pgpkeys-eu/go-crypto/commit/3286d90682e18c2cc7bf1b556f9ce557ea707f15, I think the issue is here (for example) - back when this change was made, this package was a fork of golang.org/x/crypto/openpgp, whereas now it's a standalone package; so references to golang.org/x/crypto/openpgp should be replaced by github.com/ProtonMail/go-crypto/openpgp everywhere.

andrewgdotcom commented 1 year ago

3286d90 is two commits behind HEAD, I discovered the current issue while attempting to fix those stale dependencies. Current HEAD is https://github.com/pgpkeys-eu/go-crypto/commit/f24b30100f85897dc2051fa8059a19099c347e89

twiss commented 1 year ago

Ah, sorry, I looked at that as it was linked from https://github.com/ProtonMail/go-crypto/pull/55. Running go test ./... in that branch, I don't get the same error, so I'm not sure what could be the cause, but I do get a bunch of other errors, related to checkSignatureDetails, which has been added recently to fix some missing security checks. You may need to add handling for (or a special version of the function for) v3 signatures there.

rolandshoemaker commented 1 year ago

@andrewgdotcom you're running into https://github.com/golang/go/issues/25098 which was fixed by https://github.com/golang/crypto/commit/ae8bce0, likely combined with usage of an older version of Go. What is your go version?

andrewgdotcom commented 1 year ago

@rolandshoemaker I'm using go1.18.3 darwin/amd64. I've opened a PR on my side to see what happens when the standard tests run on github, and they don't display the dependency issue (they have other issues of course!). I've cleaned my module cache locally but it doesn't help...

[EDIT] it seems there's something misconfigured in my go workspace, using a clean workspace got rid of the issue. Sorry for the noise.

andrewgdotcom commented 1 year ago

https://github.com/pgpkeys-eu/go-crypto now passes all the tests (including the restored V3 tests)

Run the following commands in your project to use the soft fork:

go mod edit -replace github.com/ProtonMail/go-crypto=github.com/pgpkeys-eu/go-crypto@40edd8c9dfc3
go mod tidy

(You can replace the 40edd8c9dfc3 with any more recent git commit ID)