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

feat: update protonmail/crypto #680

Merged
merged 14 commits into from
Jun 26, 2023
Merged

feat: update protonmail/crypto #680

merged 14 commits into from
Jun 26, 2023

Conversation

caarlos0
Copy link
Member

@caarlos0 caarlos0 commented Jun 23, 2023

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?

Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jun 23, 2023

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

@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 23, 2023
@caarlos0 caarlos0 self-assigned this Jun 23, 2023
@caarlos0 caarlos0 added bug Something isn't working enhancement New feature or request labels Jun 23, 2023
@djgilcrease
Copy link
Contributor

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

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

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)

@codecov
Copy link

codecov bot commented Jun 23, 2023

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%) ⬆️
rpm/rpm.go 70.39% <100.00%> (+0.43%) ⬆️

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

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)

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

@caarlos0
Copy link
Member Author

hmmm still getting

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

so maybe there is something else wrong

@caarlos0
Copy link
Member Author

caarlos0 commented Jun 24, 2023

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

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

so it seems rpm 4.17+ is OK 🤔

Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 24, 2023
@caarlos0
Copy link
Member Author

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:

  • if the user set the signing key id, it'll use protonmail's fork and resulting packages will not work on old rpms
  • if the user don't set it, it uses the deprecated version, which works

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

caarlos0 commented Jun 25, 2023

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

@caarlos0
Copy link
Member Author

interesting... rpm-software-management/rpm#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:

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

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

@caarlos0
Copy link
Member Author

continuing my investigation:

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

@caarlos0
Copy link
Member Author

okay, this will do the trick: ProtonMail/go-crypto#175

Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
@caarlos0 caarlos0 merged commit aff8ca3 into main Jun 26, 2023
@caarlos0 caarlos0 deleted the crypto branch June 26, 2023 13:19
@github-actions github-actions bot added this to the v2.27.0 milestone Jun 26, 2023
@caarlos0 caarlos0 modified the milestones: v2.27.0, v2.31.0 Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants