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
337 stars 101 forks source link

What is the difference between fork and original golang/crypto? #21

Closed cristaloleg closed 4 years ago

cristaloleg commented 5 years ago

I think everyone will be happy to see a short summary in README what was changes.

Thank you for the lib 😉

twiss commented 5 years ago

Hi! :wave: Here's a current list of changes:

We're hoping to merge these changes back into the original, so I won't add them to the readme for now, but I hope that's useful anyway ^.^

cristaloleg commented 5 years ago

Oh, that's nice, thank you for the info!

(probably leaving this issue is a good idea, so everyone else may find it easily)

syadav2015 commented 5 years ago

Hi @twiss,

I have a bunch of enhancements to the golang/crypto package for supporting adding subkeys and generating revocation signatures (review) as well as supporting GNU dummy S2K for missing private keys used to import keyrings generated by gpg --export-secret-subkey.

However the upstream package does not have an active maintainer/reviewer causing the reviews to be in a limbo for months. Since there are plans to push changes from this fork upstream, could we sync up on the "Widening golang.org/x/crypto/openpgp maintainership" forum thread on how to keep the upstream package alive or whether this fork would be the primary place to add new features? Any comments on the existing reviews would be greatly appreciated.

twiss commented 5 years ago

Hey @syadav2015, thanks for reaching out!

I'll start with reviewing the S2K patch (hopefully tomorrow), as it's smaller. I'll leave any comments there and then post on the mailinglist see if we can get it merged there.

If that doesn't succeed, you could make a PR here, yeah.

syadav2015 commented 5 years ago

Thanks, @twiss!!

More than half of the patch for https://go-review.googlesource.com/c/crypto/+/161817 is the tests for the new API's so hopefully it's still a manageable review. Looking forward to your feedback.

twiss commented 5 years ago

Hey @syadav2015 :wave: I left a review on https://go-review.googlesource.com/c/crypto/+/191658, did you see it? It'd be nice to try to get it merged there, and we'd like to merge it here, too (and the other one as well, eventually). Although it seems like there's gonna be a merge conflict here for this one so that's gonna be a bit of extra work ^.^

syadav2015 commented 5 years ago

Thanks for the review!! Pushed an updated patchset with resolution for comments.

syadav2015 commented 5 years ago

Hi @twiss, I rebased my changes to resolve the merge conflict in the review and ensure the review is up to date. Please let me know if anything needs to be addressed in the reviews. I would really like to get these reviews merged if possible since there is another pull request waiting for these APIs to merge in before the PR is accepted. I really appreciate you taking time to review the changes 😄 .

syadav2015 commented 5 years ago

@twiss Sorry for the slight delay in response. Was on leave due to medical reasons. Have updated https://go-review.googlesource.com/c/crypto/+/191658. Hopefully the latest patchset should suffice.

twiss commented 5 years ago

@syadav2015 No worries, hope you're well! And thanks for all the patience with all the nitpicks :) I'll send another message to the mailing list trying to get it merged, and if not, I can merge it here instead.

ddevault commented 4 years ago

Do you have any central location where you're tracking the progress on upstreaming your changes?

twiss commented 4 years ago

Hey :wave: Our latest attempt at that was here: https://groups.google.com/d/msg/golang-openpgp/_P6AmeCmD9w/sDaiFm89EgAJ We haven't received a reply there.

At this point, if @syadav2015 is willing to rebase their changes, we'd be happy to merge them here directly. (If not, I might get to it myself eventually.) We will also continue to merge changes from upstream here.

mssun commented 4 years ago

We (passforios: https://github.com/mssun/passforios) are using github.com/ProtonMail/gopenpgp for PGP encryption/decryption. Recently, we want to migrate to V2 APIs (https://github.com/mssun/passforios/issues/373). However, the "GNU dummy S2K for missing private keys" issue bothers me. Originally, we use V1 gopenpgp library with a patch (https://github.com/mssun/passforios/blob/master/go/crypto.patch). However, for V2, I found that the forked crypto changes a lot. I cannot get the original patch work. If @syadav2015's patch for GNU dummy S2K issue can be merged. That's will be great!

zugzwang commented 4 years ago

Hello @mssun, thanks for reaching out!

The issue https://github.com/mssun/passforios/issues/373 is closed, did you manage to solve it? And the patch you provided is not accessible from this URL: https://github.com/mssun/passforios/blob/master/go/crypto.patch

twiss commented 4 years ago

Hey @mssun :wave: I'll merge @syadav2015's patch.

Edit: merged as a103d74632724e3f1ab8c4b1a792b028ad72c17e.

mssun commented 4 years ago

Yes, I have modified gopenpgp and crypto:

And migrate to v2 APIs:

jbouse commented 4 years ago

These are all great additions IMO and the one not mentioned which has me looking into this fork is the Encrypt() method on the PrivateKey to set the passphrase on the secret key before exporting. I have a routine working with the x/crypto/openpgp module that does not encrypt the secret key and I am trying to swap out with this ProtonMail/crypto/openpgp module but running into some issues during the build/get phase because of references to modules that are in the fork but referenced as golang.org/x/crypto modules. Could you provide some guidance on how best to swap out the modules until such time as they can be merged into x/crypto which I see is a path being pursued? I am using go.mod and what I get when I attempt to build is the following:

go: finding module for package github.com/ProtonMail/crypto/openpgp/packet go: found github.com/ProtonMail/crypto/openpgp/packet in github.com/ProtonMail/crypto v2.0.0+incompatible go: finding module for package golang.org/x/crypto/rsa go: finding module for package golang.org/x/crypto/openpgp/internal/algorithm go: finding module for package golang.org/x/crypto/openpgp/internal/encoding go: finding module for package golang.org/x/crypto/openpgp/ecdh go: finding module for package golang.org/x/crypto/openpgp/internal/ecc ../../go/pkg/mod/github.com/!proton!mail/crypto@v2.0.0+incompatible/openpgp/packet/encrypted_key.go:13:2: module golang.org/x/crypto@latest found (v0.0.0-20200510223506-06a226fb4e37), but does not contain package golang.org/x/crypto/openpgp/ecdh ../../go/pkg/mod/github.com/!proton!mail/crypto@v2.0.0+incompatible/openpgp/packet/packet.go:16:2: module golang.org/x/crypto@latest found (v0.0.0-20200510223506-06a226fb4e37), but does not contain package golang.org/x/crypto/openpgp/internal/algorithm ../../go/pkg/mod/github.com/!proton!mail/crypto@v2.0.0+incompatible/openpgp/packet/public_key.go:28:2: module golang.org/x/crypto@latest found (v0.0.0-20200510223506-06a226fb4e37), but does not contain package golang.org/x/crypto/openpgp/internal/ecc ../../go/pkg/mod/github.com/!proton!mail/crypto@v2.0.0+incompatible/openpgp/packet/encrypted_key.go:16:2: module golang.org/x/crypto@latest found (v0.0.0-20200510223506-06a226fb4e37), but does not contain package golang.org/x/crypto/openpgp/internal/encoding ../../go/pkg/mod/github.com/!proton!mail/crypto@v2.0.0+incompatible/openpgp/packet/encrypted_key.go:17:2: module golang.org/x/crypto@latest found (v0.0.0-20200510223506-06a226fb4e37), but does not contain package golang.org/x/crypto/rsa

Any assistance would be extremely appreciated as the task I'm working on has a lot of pressure from above to get it implemented.

zugzwang commented 4 years ago

Hello @jbouse! You could swap modules using replace directives on your go.mod. For instance,

replace golang.org/x/crypto => github.com/ProtonMail/crypto v0.0.0-20200416114516-1fa7f403fb9c

will effectively import commit 1fa7f40 of this fork whenever your code uses import "golang.org/x/crypto" (you can also use the master tag).

jbouse commented 4 years ago

@zugzwang okay that seems much simpler than I was trying to make it out to be. I was thinking it was more complicated. Just recently started working with Go so still learning. Thanks for the quick reply.

cristaloleg commented 4 years ago

Should we close this issue? 👀

jbouse commented 4 years ago

Yes, this can be considered closed.

On Fri, Jul 10, 2020 at 3:33 AM Oleg Kovalov notifications@github.com wrote:

Should we close this issue? 👀

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ProtonMail/crypto/issues/21#issuecomment-656531064, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAKHHTYXZWWSTNRMKV26L3R2272ZANCNFSM4HNFWARA .

ilius commented 2 years ago

Is it possible to remove dependency to golang.org/x/crypto?

twiss commented 2 years ago

No, since https://github.com/ProtonMail/go-crypto/pull/77 we use upstream x/crypto for some low-level crypto primitives, instead of maintaining them in this fork, to reduce the maintenance burden. We now only keep the parts that were added/changed, i.e. x/crypto/openpgp, and import unchanged parts from golang.org/x/crypto. But we only import the parts we need, which is cast5 and ripemd160 - arguably somewhat legacy algorithms, so we could think about making them optional if you don't need support for those, but that might be more effort than it's worth.