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

replace golang.org/x/crypto/openpgp with github.com/ProtonMail/go-crypto/openpgp #1228

Merged
merged 4 commits into from
Nov 23, 2023

Conversation

paulcacheux
Copy link
Contributor

@paulcacheux paulcacheux commented Oct 31, 2023

Fixes some TODOs in the code

Requirements

All new code should be covered with tests, documentation should be updated. CI should pass.

Description of the Change

This PR replaces golang.org/x/crypto/openpgp which is unsupported (issue) with github.com/ProtonMail/go-crypto/openpgp.

This has the added bonus of adding support for new pgp key types

Checklist

  • unit-test added (if change is algorithm)
  • functional test added/updated (if change is functional)
  • man page updated (if applicable)
  • bash completion updated (if applicable)
  • documentation updated
  • author name in AUTHORS

@paulcacheux paulcacheux marked this pull request as ready for review October 31, 2023 13:18
@paulcacheux paulcacheux force-pushed the paulcacheux/proton-gpg branch from c968422 to 6773724 Compare November 2, 2023 10:26
Copy link

codecov bot commented Nov 2, 2023

Codecov Report

Merging #1228 (6773724) into master (f1649a6) will increase coverage by 0.03%.
The diff coverage is n/a.

❗ Current head 6773724 differs from pull request most recent head f1c1891. Consider uploading reports for the commit f1c1891 to get more accurate results

@@            Coverage Diff             @@
##           master    #1228      +/-   ##
==========================================
+ Coverage   74.85%   74.89%   +0.03%     
==========================================
  Files         143      143              
  Lines       16187    16179       -8     
==========================================
  Hits        12117    12117              
+ Misses       3134     3126       -8     
  Partials      936      936              
Files Coverage Δ
pgp/internal.go 56.97% <ø> (ø)
pgp/openpgp.go 61.48% <ø> (+3.15%) ⬆️

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@paulcacheux
Copy link
Contributor Author

@randombenj sorry for the direct ping. Is this something you would be interested in ?

Copy link
Contributor

@randombenj randombenj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change, I think this makes sense. Why was the V3 switch removed?

@@ -94,12 +92,6 @@ func checkDetachedSignature(keyring openpgp.KeyRing, signed, signature io.Reader
sigType = sig.SigType
creationTime = sig.CreationTime
pubKeyAlgo = sig.PubKeyAlgo
case *packet.SignatureV3:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this have any impact?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes something I should have made clearer in the PR description. Basically this means we would lose support for SignatureV3. They are old and nearly unsupported so I think it's your decision.

On the other hand, the current openpgp does not support ed25519 keys (example issue), and is not supported. So migrating has both upsides and downsides.

I searched for another maintained /x/crypto/openpgp alternatives that is still maintained without success sadly

Copy link
Contributor Author

@paulcacheux paulcacheux Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After double checking https://github.com/ProtonMail/go-crypto/blob/afb1ddc0824ce0052d72ac0d6917f144a1207424/openpgp/packet/packet.go#L339, I think there was already no way to read a SignatureV3 from a packet, so I think we don't lose anything.

Here is a public issue showing a similar intel golang/go#27679

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@neolynx any input on this, otherwise I think we can merge

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would go with that. We can test with old mirrors and repos, and readd it in case this brings any troubles...

@paulcacheux
Copy link
Contributor Author

Regarding the failing tests it seems the upstream repos were updated. Happy to rebase once main is green regarding those

@paulcacheux
Copy link
Contributor Author

@randombenj maybe we can fix the CI first before merging. How can I help in that direction ?

@neolynx neolynx self-requested a review November 22, 2023 19:14
@neolynx neolynx self-assigned this Nov 22, 2023
Copy link
Member

@neolynx neolynx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm :)

@randombenj
Copy link
Contributor

@paulcacheux as the tests don't fail because of your changes, I will merge this and then later on fix the tests on the main branch. Thanks for the contribution 🎉

@randombenj randombenj merged commit aeef41b into aptly-dev:master Nov 23, 2023
1 of 2 checks passed
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.

4 participants