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

ed25519 support #221

Closed
anarcat opened this issue Nov 7, 2017 · 7 comments
Closed

ed25519 support #221

anarcat opened this issue Nov 7, 2017 · 7 comments
Milestone

Comments

@anarcat
Copy link
Contributor

anarcat commented Nov 7, 2017

Hi!

I see that ed25519 support is missing from PGPy. Since it is a commonly used curve in GPG, I started looking into how it could be implemented here.

It seems the first problem is that support for that curve is missing from OpenSSL itself, as shown by openssl/openssl#487. According to openssl/openssl#487 (comment), a patch was merged in june to add support in master, which should take some time to propagate everywhere. The two patches are openssl/openssl#3585 and openssl/openssl#3503 which seem to indicate the magic word may be ED25519 but i'm not sure.

So I wonder if that could simply be implemented in PGPy's side with:

diff --git a/pgpy/_curves.py b/pgpy/_curves.py
index 9503075..8417d15 100644
--- a/pgpy/_curves.py
+++ b/pgpy/_curves.py
@@ -48,7 +48,13 @@ class BrainpoolP512R1(object):
     key_size = 512
 
 
+@utils.register_interface(ec.EllipticCurve)
+class Ed25519P256R1(object):
+    name = 'Ed25519'
+    key_size = 256
+
+
 # add these curves to the _CURVE_TYPES list
-for curve in [BrainpoolP256R1, BrainpoolP384R1, BrainpoolP512R1]:
+for curve in [BrainpoolP256R1, BrainpoolP384R1, BrainpoolP512R1, Ed25519P256R1]:
     if curve.name not in ec._CURVE_TYPES and curve.name in _openssl_get_supported_curves():
         ec._CURVE_TYPES[curve.name] = curve

would that make sense? or there are some fundamental things to change elsewhere in the code?

@Commod0re
Copy link
Contributor

Yes, the blocker is that the curve had not been implemented in OpenSSL until recently. I believe the correct patch for that file will be:

--- a/pgpy/_curves.py
+++ b/pgpy/_curves.py
@@ -48,7 +48,19 @@
     key_size = 512


+@utils.register_interface(ec.EllipticCurve)
+class X25519(object):
+    name = 'X25519'
+    key_size = 256
+
+
+@utils.register_interface(ec.EllipticCurve)
+class Ed25519(object):
+    name = 'ed25519'
+    key_size = 256
+
+
 # add these curves to the _CURVE_TYPES list
-for curve in [BrainpoolP256R1, BrainpoolP384R1, BrainpoolP512R1]:
+for curve in [BrainpoolP256R1, BrainpoolP384R1, BrainpoolP512R1, X25519, Ed25519]:
     if curve.name not in ec._CURVE_TYPES and curve.name in _openssl_get_supported_curves():
         ec._CURVE_TYPES[curve.name] = curve

pgpy/constants.py will need to be updated to also import X25519 and Ed25519 from _curves, and add references to them to class EllipticCurveOID. That should then be enough to start actually using those curves with PGPy, provided that your install of OpenSSL is new enough to include them. I will add these changes for the next release.

@Commod0re Commod0re self-assigned this Nov 7, 2017
@Commod0re Commod0re added this to the 0.5.0 milestone Nov 7, 2017
@Commod0re Commod0re modified the milestones: 0.5.0, 0.4.4 Nov 8, 2017
Commod0re added a commit that referenced this issue Nov 8, 2017
…t it as soon as people obtain builds of OpenSSL that include support for them
@juga0
Copy link

juga0 commented Nov 26, 2017

Would be ok to use donna25519 instead of openssl?.
As an example, this decrypt a session key packet using donna25519: PGPy/features/x25519.
It can be tested with PGPy25519_test

@anarcat
Copy link
Contributor Author

anarcat commented Nov 26, 2017

donna25519 is odd - it says:

The C implementation of Curve25519 that this uses was written in 2008. A lot has changed since then. If you're just looking for something that does elliptic curve Diffie-Hellman and heard you should use curve25519, you're probably significantly better served by NaCl or libsodium. Also take a look at PyCA's cryptography project.

But then PyCA is exactly what we're doing now...

@Commod0re
Copy link
Contributor

I would prefer to avoid adding temporary extra dependencies to a release, especially given that I haven't had enough time to actually do them in a timely fashion. I've got some code locally that is close for OpenSSL, but I'm not sure how PyCA is going to handle its interfaces for ed25519 yet.

@dkg
Copy link
Contributor

dkg commented Apr 28, 2018

what's the status on this? it's been several months and it would be great to show some interop with GnuPG and OpenPGP.js, which are both implementing 25519.

@dkg
Copy link
Contributor

dkg commented Apr 28, 2018

huh, i didn't mean to unassign you @Commod0re -- i don't know how that happened. sorry about that!

@dkg
Copy link
Contributor

dkg commented Dec 20, 2018

ping on this again @Commod0re, can you take a look at this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants