search

goreleaser / nfpm

nFPM is Not FPM - a simple deb, rpm, apk, ipk, and arch linux packager written in Go
https://nfpm.goreleaser.com/
MIT License
2.14k stars 157 forks source link

feat: update protonmail/crypto #680

Closed caarlos0 closed 1 year ago

caarlos0 commented 1 year ago

As expected, this breaks signing for centos 8. Not expected: it also broke it for centos 9.

Fedora seems to still work though.

Not sure what, if anything, we can do about this? Should we revert back to the main go/crypto package which supposedly still works?

cloudflare-pages[bot] commented 1 year ago

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8caff64
Status: ✅  Deploy successful!
Preview URL: https://295ab7c6.nfpm.pages.dev
Branch Preview URL: https://crypto.nfpm.pages.dev

View logs

djgilcrease commented 1 year ago

I am unsure why this would break contos, we are using sha256 for signing nor sha1 which GopenPGP still supports

caarlos0 commented 1 year ago

I am unsure why this would break contos, we are using sha256 for signing nor sha1 which GopenPGP still supports

as far as I gathered, centos still uses an old version of the signature, which is supported by golang/x/crypto/openpgp, but not by the protonmail fork (because it is very old)

https://github.com/sassoftware/go-rpmutils/pull/21#issuecomment-1100266607

codecov[bot] commented 1 year ago

Codecov Report

Merging #680 (8caff64) into main (d8d067c) will increase coverage by 0.04%. The diff coverage is 92.85%.

@@            Coverage Diff             @@
##             main     #680      +/-   ##
==========================================
+ Coverage   75.81%   75.86%   +0.04%     
==========================================
  Files          14       14              
  Lines        2956     2962       +6     
==========================================
+ Hits         2241     2247       +6     
  Misses        506      506              
  Partials      209      209
Impacted Files Coverage Δ
internal/sign/pgp.go 65.71% <88.88%> (+0.39%) :arrow_up:
rpm/rpm.go 70.39% <100.00%> (+0.43%) :arrow_up:
djgilcrease commented 1 year ago

I am unsure why this would break contos, we are using sha256 for signing nor sha1 which GopenPGP still supports

as far as I gathered, centos still uses an old version of the signature, which is supported by golang/x/crypto/openpgp, but not by the protonmail fork (because it is very old)

sassoftware/go-rpmutils#21 (comment)

https://github.com/ProtonMail/go-crypto/issues/164 maybe try replace github.com/ProtonMail/go-crypto => github.com/pgpkeys-eu/go-crypto v0.0.0-20230506215654-16de0cc09494

caarlos0 commented 1 year ago

hmmm still getting 

#11 0.562 /tmp/foo.rpm: digests SIGNATURES NOT OK

so maybe there is something else wrong

caarlos0 commented 1 year ago

Okay, for reference:

❯ docker run -it --rm quay.io/centos/centos:stream9-minimal rpm --version
RPM version 4.16.1.3

❯ docker run -it --rm quay.io/centos/centos:stream8 rpm --version
RPM version 4.14.3

❯ docker run -it --rm fedora:34 rpm --version
RPM version 4.16.1.3

❯ docker run -it --rm fedora:35 rpm --version
RPM version 4.17.1

❯ docker run -it --rm fedora:36 rpm --version
RPM version 4.17.1

❯ docker run -it --rm fedora:37 rpm --version
RPM version 4.18.1

❯ docker run -it --rm fedora:38 rpm --version
RPM version 4.18.1

RPM 4.18 seems to work, the older versions don't.

caarlos0 commented 1 year ago

4.18 changelog: https://rpm.org/wiki/Releases/4.18.0

there's a lot of signature and openpgp related changes and fixes.

I'm leaning towards supporting RPM 4.18+ for signed packages for now on, and that's it 🤔

caarlos0 commented 1 year ago

so it seems rpm 4.17+ is OK 🤔

caarlos0 commented 1 year ago

okay, so, porting back to the deprecated crypto package works, as long as you don't need to set the signing key id, because apparently there is no way of setting it in that lib

soo... I made a compromise:

which I think is pretty confusing... and not sure if whether or not is a good idea at all

eager to hear what you all think @djgilcrease @erikgeiser

caarlos0 commented 1 year ago

PS: the very same tests pass in my machine 🤔 Fake news

caarlos0 commented 1 year ago

interesting... https://github.com/rpm-software-management/rpm/issues/2351

so, basically, the old package breaks new RPMs, the new version breaks old RPMs... so is just a matter of choosing which ones to break?

:pain:

caarlos0 commented 1 year ago

another alternative (that honestly I almost think is better) is to copy the needed code from the versions that work to our codebase 🤔

caarlos0 commented 1 year ago

continuing my investigation:

So, I think we can presume that's something related to the critical bit of the signatures, right?

caarlos0 commented 1 year ago

okay, this will do the trick: https://github.com/ProtonMail/go-crypto/pull/175