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

fix: IssuerKeyId non-critical #175

Merged
merged 1 commit into from
Jun 26, 2023
Merged

Conversation

caarlos0
Copy link
Contributor

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:

  • created tests signing and checking on several versions of fedora and centos, ranging RPM 4.14 - 4.18
  • made a sort of manual bisect of this project trying to find which commit fixed 4.17 so I could try to fix for other versions too

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 aef6224, which adds v5 signatures, but also changes the Issuer Key ID packet to critical on v4.

Change: aef6224#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!

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:

- created tests signing and checking on several versions of fedora and
  centos, ranging RPM 4.14 - 4.18
- made a sort of manual bisect of this project trying to find which
  commit fixed 4.17 so I could try to fix for other versions too

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 aef6224, which
adds v5 signatures, but also changes the Issuer Key ID packet to
critical on v4.

Change: ProtonMail@aef6224#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!

Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
@caarlos0
Copy link
Contributor Author

forgot to link issue in nfpm: goreleaser/nfpm#680

@caarlos0
Copy link
Contributor Author

caarlos0 commented Jun 25, 2023

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
Copy link
Member

twiss commented Jun 26, 2023

Hey 👋 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.)

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

Successfully merging this pull request may close these issues.

2 participants