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

[cristaloleg]

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

Thank you for the lib 😉

[twiss]

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

• Expand ECC support
• Bump default and minimum key-derivation (S2K) cost parameters
• Default to AES128 and SHA256 instead of CAST5 and RIPEMD160 for keys without preferred algorithms, as required by rfc4880bis-06 (rfc4880 required 3DES and SHA1)
• Disallow generating RSA keys of less than 1024 bits (recommended by the audit)
• Fix and move Signature.KeyExpired to PublicKey.KeyExpired; add Signature.SigExpired (x/crypto/openpgp: Incorrect comparison when checking if PGP key is expired golang/go#22312)
• Detect expired signatures; though leaving it up to the caller whether they want to reject them
• Use latest-created valid self-signature instead of the last-appearing one; don't drop other self-signatures when re-serializing the key
• Add higher-level function to verify clearsigned messages, in order to be able to verify that the algorithm mentioned in the header matches the algorithm used in the signature (one of the findings of the audit)

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]

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]

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]

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]

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]

Hey @syadav2015 👋 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]

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

[syadav2015]

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]

@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]

@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]

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

[twiss]

Hey 👋 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]

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 (mssun/passforios#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]

Hello @mssun, thanks for reaching out!

The issue mssun/passforios#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]

Hey @mssun 👋 I'll merge @syadav2015's patch.

Edit: merged as a103d74.

[mssun]

Yes, I have modified gopenpgp and crypto:

• gopenpgp: https://github.com/mssun/gopenpgp/tree/gnu-dummy
• crypto: https://github.com/mssun/crypto/tree/gnu-dummy

And migrate to v2 APIs:

• Migrate to gopenpgp v2 and fix GNU dummy issue mssun/passforios#374

[jbouse]

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/[email protected]+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/[email protected]+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/[email protected]+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/[email protected]+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/[email protected]+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]

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]

@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]

Should we close this issue? 👀

[jbouse]

Yes, this can be considered closed.

…

On Fri, Jul 10, 2020 at 3:33 AM Oleg Kovalov ***@***.***> wrote: Should we close this issue? 👀 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#21 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAKHHTYXZWWSTNRMKV26L3R2272ZANCNFSM4HNFWARA> .

[ilius]

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

[twiss]

No, since #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.