search

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
328 stars 99 forks source link

fix: IssuerKeyId non-critical #175

Closed caarlos0 closed 1 year ago

caarlos0 commented 1 year ago

So, let me start by saying I'm not crypto/gpg expert or anything. If this is wrong, I'd appreciate being pointed to the right direction 🙏

I maintain nFPM and GoReleaser, and, when golang.org/x/crypto was deprecated, I switched to your fork.

On commit cc34b1f it broke signatures for RPM versions, so I locked into the previous commit (c05353c).

After some time, I noticed that the latest commit (at the time, and now too) works RPM 4.17+ too.

I have been postponing investigating this properly for a couple of years (I think), today I finally went down the rabbit hole:

Finally, found it: a4f6767

It was the clue I needed to try to fix the whatever else is broken.

Then, through git blame, I found aef62243dce8452451d6dd6c009b22c58a6a50fa, which adds v5 signatures, but also changes the Issuer Key ID packet to critical on v4.

Change: https://github.com/ProtonMail/go-crypto/commit/aef62243dce8452451d6dd6c009b22c58a6a50fa#diff-65b30b147e6c8432806b9ff7fa5f3368af59ccbd942e99582137fa0cd46978f6L782

I'm not sure if that particular change is intentional or not, but reverting it (making issue key id non-critical in v4) fixes everything for all RPM versions.

Again, I don't know if this is correct per GPG spec and RPM is wrong, or the other way around, or something else, all I know is that this seems to fix thing and doesn't seem to break any tests.

Thanks for your time, and sorry about this whole novel of a writing.

Happy weekend!

caarlos0 commented 1 year ago

forgot to link issue in nfpm: https://github.com/goreleaser/nfpm/pull/680

caarlos0 commented 1 year ago

reading RFC 2440, the only thing about critical that I found is in the section 5.2.3.1, Signature Subpacket Specification:

Bit 7 of the subpacket type is the "critical" bit. If set, it denotes that the subpacket is one that is critical for the evaluator of the signature to recognize. If a subpacket is encountered that is marked critical but is unknown to the evaluating software, the evaluator SHOULD consider the signature to be in error.

An evaluator may "recognize" a subpacket, but not implement it. The purpose of the critical bit is to allow the signer to tell an evaluator that it would prefer a new, unknown feature to generate an error than be ignored.

It does not specify which fields are supposed to be critical or not... so... I don't know? Based on the explanation above it kind of makes sense for it to be, but since signing RPMs with GNUPG does not cause this issue, maybe its correct to let it unset for this field?

twiss commented 1 year ago

Hey :wave: Thanks for the investigation and PR!

This subpacket indeed doesn't strictly need to be critical, because even if an implementation ignores it and tries to verify the signature with a different key, it should fail anyway.

(That being said, I'm very surprised it wasn't supported in an implementation. But OK, at least it's fixed in the last version.)