Support for Ed25519 signing and X25519 encryption (cv25519) #254

rot42 · 2019-04-25T23:51:23Z

This fixes #221, fixes #222 and fixes #247
This closes #246, closes #258, closes #259, closes #263, closes #264, closes #265 and closes #266

Commod0re · 2019-06-07T19:14:20Z

wow this is great, sorry I haven't responded sooner, I'll review and merge a bunch of these PRs soon, seems I have an access issue to work out first

pgpy/packet/fields.py

Commod0re · 2019-06-07T22:21:55Z

pgpy/pgp.py

@@ -17,6 +17,12 @@

 import six

+# TODO: replace if six fixes this: https://github.com/benjaminp/six/issues/155


I think we'll just not support 3.3 anymore at this point since PSF doesn't

Commod0re

just a handful of quick style changes please, this is great work otherwise, thank you!

rot42 · 2019-06-09T21:08:08Z

Thanks again for the review! I've added a new commit with the requested changes.
I've left the collections_abc = collections.abc || collections import trick because Python 2.7 does not have collections.abc. This can be removed when you decide to drop support for Python 2.7.

rot42 · 2019-06-09T22:53:12Z

I've rebased on the current master for easier merge.

dkg · 2019-07-17T06:24:49Z

I really like the functionality being offered here, but this single-commit PR has huge diff, and contains a lot of changes that are not what the commit message says.

If it's possible to break out the changes into separate, more easily-reviewable commits, that would be great.

I understand that this change also increases the minimum version of python-cryptography required. that's probably fine for a new major version of pgpy, but might make it uninstallable on some older operating systems or python installations.

dkg · 2019-07-17T07:18:13Z

fwiw, many of the PRs i've contributed in the last couple hours (#263, #264, #265, #259, #258, #266 ) do take care of some of the non-25519-related bits in this mammoth patch. If those get merged, the diff that remains in this proposal will be simpler and more clearly focused.

I'm working on adding a Curve25519-specific patch as well, on top of those pieces of cleanup, since i think that will be a smaller change than the EdDSA changes (and it only requires a move to python-cryptography 2.5, not 2.6).

If we can get that merged, then we can focus on the EdDSA changes.

Commit e163a0c made creation and expiration date relative to current time which does not match content in revochiio.asc. Reverted commit and asserted the key as expired. test_10_exceptions.py: test_encrypt_sessionkey_wrongtype did not have a wrong sessionkey type and therefore was not raising TypeError. pytest.raises() message parameter is deprecated pytest-dev/pytest#4539 EllipticCurveOID does not have a name attribute, the name of the curve is stored in EllipticCurveOID.curve.name

rot42 · 2019-07-19T23:37:42Z

Split each fix, improvement and new feature in a separate commit.
Added tests with mixed RSA-Curve25519 key in order to test Curve25519 independently from Ed25519.
Fixed a few additional warnings

dkg · 2019-07-24T15:41:10Z

Thanks for these edits! This is looking good, and i hope that @Commod0re will give some feedback soon.

I noticed a handful of asserts in this series that look like they could be triggered based on arbitrary data that the code might be trying to ingest, in particular around the format of the ECPoint for different curves. assert seems pretty brittle. Could you convert those asserts to raises?

rot42 · 2019-07-25T19:03:04Z

I noticed a handful of asserts in this series that look like they could be triggered based on arbitrary data that the code might be trying to ingest, in particular around the format of the ECPoint for different curves. assert seems pretty brittle. Could you convert those asserts to raises?

@dkg done, I've added a new Exception type for incompatibilities between the used elliptic curve and the point compression format.

dkg · 2019-07-29T20:45:45Z

This all looks great. I've e-mailed @Commod0re privately to try to see whether he could merge this, but his e-mail address appears to be dead (google's mail exchanger says the "account is disabled").

That suggests that he might no longer be active, which would be sad.

If that's the case, i'm not sure what the right next step to do is. Perhaps we could fork it (in the traditional free software sense of "fork", not the github meaning) and try to release the new version separately with these new features? I would rather keep @Commod0re in the loop, but if he's non-responsive, i don't know what else to do.

Also, if we decide to fork, i'm not sure what name we should apply to the new project. The most obvious generic names are already taken in the python ecosystem:

OpenPGP
pgp

so perhaps "pgpy2" to acknowledge the legacy (and make it easier to move back if @Commod0re is revived and wants to pick this back up?

J-M0 · 2019-07-30T21:42:54Z

Hi, my name is James and I work as a developer at Security Innovation. I'll be helping out as a co-maintainer on this project. I have also reached out to @Commod0re for feedback about this project and am waiting to hear back.

If we don't hear back in a while, I'll start working on reviewing and approving pull requests.

dkg · 2019-07-31T15:55:38Z

@J-M0 thanks for the update, and for stepping up as a co-maintainer. It's much appreciated.

Note that if this series is merged, we can close several of the other pull requests i've made recently. so if you're trying to decide where to start, this PR is it :)

I'd be happy to go through and close the other PRs i've made that are duplicative of this series if it gets merged.

dkg · 2019-07-31T16:00:31Z

@J-M0 -- i notice that neither you nor @Commod0re are publicly listed as part of the SecurityInnovation group here on github. Only @BendixSpring and @joebasirico are listed. Perhaps you can convince them to add the two of you to the organization to make the affiliations clearer?

Commod0re · 2019-07-31T16:04:04Z

hey thanks for your patience with me on these PRs guys, I'm reviewing this one right now

Commod0re · 2019-07-31T16:20:44Z

I tell a small lie; I've gone through @dkg's linked PRs first, mostly because they're smaller and easier to go through quickly, which has created some merge conflicts on this PR. @rot42 can you check and resolve those merge conflicts, and then I'll review (and likely merge) this PR again?

Commod0re · 2019-08-01T04:25:05Z

Sorry, it looked like you and @dkg had collaborated somehow, I must have misunderstood. If those PRs are all just literal copy/pastes from this one, it should be very straightforward to resolve the merge conflicts. As I contribute to this project in my free time and without any form of compensation whatsoever, it's a lot more daunting to review huge monolithic PRs than smaller ones, as Daniel tried to explain.

But, listen, you realize that you wouldn't have owned any code contributed to this project to begin with, right? I don't understand why you think anyone stole anything from you, when you had already given it to SecurityInnovation by virtue of submitting this PR. That's something anyone contributing code to this project should bear in mind: if you submit a PR here, you are relinquishing all ownership of the contents of that PR to SecurityInnovation.

With that in mind, PGPy's github is not the place to get uppity about who owns what piece where, or whatever this is about, because I don't even own my contributions to PGPy. As I said this is a volunteer project, and that means sometimes work will be duplicated, toes will get stepped on, and miscommunications will happen. I'm really sorry I misunderstood what was going on and that it resulted in my hurting your feelings, but I'm also not cool with sudden drama like this. I don't want to see it here again, so I want to prove that I will always try to make up for it when this happens.

I really do appreciate that you put time and work into this, and have waited such a long time for me to review and merge it, so I am going to put a special thank you in the release notes for the work you've contributed here, especially since some of it was merged under another name. I'd like to be able to continue working with you on PGPy if you're willing, but I also understand if you don't.

dkg · 2019-08-01T13:44:25Z

@rot42, i didn't mean to "steal" any code, and i apologize if it came off that way. I did read through your PR and tried to isolate the specific changes that were distinct from the most important part of the work, so that the remaining changes could be evaluated on their own. I had already done some of that work (e.g. the cleanup to cryptography 1.5) before i saw your pull request. That said, your work on re-factoring the ECPoint class was incredibly useful for clarifiying (and making extensible) the code there for me, and is explicitly credited to you in 93a1ba3.

i never got to the point of adding ed25519 and curve25519 support. Your work in isolating that itself makes the codebase much better.

I personally commit to reviewing the changes and to resolving the merge conflicts which i helped contribute to, and i will rewrite them and publish a branch with appropriate Author credits for @rot42 including the 25519 work. @rot42 -- if you object to that i'll stop that work and try to rewrite them from scratch, though i think that would be sad.

@Commod0re , i appreciate your stepping back into this project and doing these reviews, but i want to remind you that "if you submit a PR here, you are relinquishing all ownership of the contents of that PR to SecurityInnovation" is not at all the case. There is no contributor license agreement on this project, it is BSD-3-clause licensed (see LICENSE), and the general understanding of BSD-3-clause license is that people's work remains owned by them, but they license that work to the general public under the same BSD-3-clause license. No one has relinquished ownership; what they have done is granted license to use, modify, redistribute, and redistribute modified copies, so long as that authorship is acknowledged.

I think that's all in line with what you plan to do as described above, but it's worth remembering that "ownership" has not been transfered or relinquished.

Commod0re · 2019-08-01T17:44:46Z

There is no contributor license agreement on this project, it is BSD-3-clause licensed (see LICENSE), and the general understanding of BSD-3-clause license is that people's work remains owned by them, but they license that work to the general public under the same BSD-3-clause license

look at the copyright notices plastered all over this repo, how many names do you see on there? SecurityInnovation is the only one, because contributing code to this project means they own it, and in court that copyright notice is the only thing that really matters for proof of ownership. There's a ton of legal precedent on both of those things already, but as we're all software devs and not lawyers, let's just use take as a friendly reminder that this is a "no ownership zone" where we're trying to make a cool thing together, not fight over who gets credit for what part.

anarcat · 2019-08-01T17:57:09Z

look at the copyright notices plastered all over this repo, how many names do you see on there?

I don't want to be a dick about this, but I did look, and there are very little of those copyright notices, actually:

$ grep -rl -E -e '(Copyright|(c)|©).*SecurityInnovation' -e '(Copyright|(c)|©).*Greene' *
docs/source/installation.rst
docs/source/conf.py
gentoo/pgpy-0.4.0.ebuild
LICENSE
pgpy/__pycache__/_author.cpython-35.pyc
pgpy/_author.py
pgpy/_author.pyc
README.rst
setup.py

The normal practice is to add one of those to the top of every source file, not just at those few key components...

Even then, my understanding of the legal workings of software copyright is that, regardless of those headers and statements, people still own their contributions to free software projects. This is the reason why DCO makes any sense at all: if ownership of an author's contribution were automatically transfered to the project, the Linux foundation would have never needed to bother with this.

This is very different, mind you, from the rights given to you (and everyone else, generally) by those contributors: you have the right to reuse the code and redistribute it and everything the BSD license gives you. But it does not give you or SecurityInnovation ownership to the code.

The fact that the copyright statements don't include other contributors doesn't mean they don't exist or that they own the copyright to their contributions. At least it's under those conditions that I contribute to free software projects and I'm sure that a lot of people share that (legal) point of view.

... that being said:

as we're all software devs and not lawyers, let's just use take as a friendly reminder that this is a "no ownership zone" where we're trying to make a cool thing together, not fight over who gets credit for what part.

Hear hear. :) Let's move on and get some code in! :)

dkg · 2019-08-01T18:05:16Z

i second anarcat's last sentiment ("hear hear") here -- and also, i don't think that anyone here is actually in disagreement over who gets what credit. All the contributions have been (and continue to be) deliberately acknowledged from what i've seen ☺

dkg · 2019-08-01T20:13:47Z

#270 provides a clean and mergable rebase of @rot42's work on top of the current master.