Add Weierstrass Curve 25519 #207

pranithan-kang · 2020-08-10T05:56:48Z

No description provided.

tomato42

What's the usefulness of a Weirstrass representation of Curve25519? It won't interoperate with X25519 or Ed25519 implementations without additional code (projecting the results back and forth between Weierstrass and Edwards representations)
- To be clear, I have nothing against adding support for X25519 or Ed25519, but I don't think the high level interface should expose the Weirstrass representation used on the backend
Test coverage missing
Please fix the email used in the commit: either change it to the one you use for github or add it as an alternative one in your profile (if you are the author)

tomato42 · 2020-08-10T10:34:36Z

src/ecdsa/__init__.py

    BRAINPOOLP320r1,
    BRAINPOOLP384r1,
    BRAINPOOLP512r1,
+    Curve25519Weier,


I'd prefer the full name "Curve25519Weierstrass"
but then I've found at least https://tools.ietf.org/id/draft-struik-lwip-curve-representations-00.html referring to it as "Wei25519"

That PR in Javascript library uses different name still... How does BouncyCastle call it?

Just Curve25519 :-)

yeah, though the docs are explicit that it's non-standard: https://github.com/bcgit/bc-java/blob/738dfc0132323d66ad27e7ec366666ed3e0638ab/core/src/main/javadoc/org/bouncycastle/math/ec/custom/djb/package.html

so I'd prefer the https://tools.ietf.org/html/draft-ietf-lwig-curve-representations-11 naming

src/ecdsa/ecdsa.py

tomato42 · 2020-08-10T10:44:14Z

src/ecdsa/ecdsa.py

+
+curve_25519_weier = ellipticcurve.CurveFp(_p, _a, _b, 1)
+generator_25519_weier = ellipticcurve.PointJacobi(
+    curve_25519_weier, _Gx, _Gy, 1, _r, generator=True)


nit: newline missing
nit: closing paren needs to be on next line, black will complain otherwise

tomato42 · 2020-08-10T10:45:13Z

src/ecdsa/curves.py

+Curve25519Weier = Curve("Curve25519Weier",
+                        ecdsa.curve_25519_weier,
+                        ecdsa.generator_25519_weier,
+                        (1, 3, 101, 110))


this is not the correct OID, this is an OID for X25519

I really do not know what is the actually OID of Weierstrass Form of Curve 25519 (aka wei25519). Please suggest

I don't know, that's the first time I've heard of people actually using Curve25519 in Weierstrass form...
Check what OID BouncyCastle uses when you write the key to PKCS#8 file?

I searched from the web. I really could not find the OID for Wei25519. How could I solve this situation?

Could you generate the Wei25519 key using BouncyCastle, save it to a PKCS#8 file and post here?

hmm, so both the private and public key save the curve parameters with the key, they don't use the name/OID; while valid, that's not supported in python-ecdsa: #39

Is there an api setting in BC to save using named_curve format (instead of the explicit format it used)?

I am not sure that I understand your question correctly. Do you mean to embed the curve name in the keys?

essentially yes

For ECDSA keys (which mathematically speaking are points on a 2D plane) an application that uses them needs to know on what curve they lay on, that information needs to be encoded in the private and public keys or transferred out of band. When it's included in the files, there are 3 possible encodings: explicit parameters, where the constants that define the curve are spelled out explicitly, as a set of integers; named_curve, where the curve is identified only as an OID, that usually is then transformed into human-readable name, like secp256r1; or "implicit", where the curve parameters are supposed to be taken from the CA certificate that signed the public key (I didn't see any software that supports it though)

See https://tools.ietf.org/html/rfc5480#section-2.1.1

in both files the curve parameters are encoded explicitly

A bit of quick googling suggests that the parameters needs to be created using X962NamedCurves.GetOid("prime256v1") : https://stackoverflow.com/a/9432083/462370 but I don't know if you're using .NET or the Java BC

Side note: the "-----BEGIN EC PRIVATE KEY-----" is the legacy, OpenSSL format, PKCS#8 has "-----BEGIN PRIVATE KEY-----" header, that being said both support the different ways of encoding the curve parameters

pranithan-kang · 2020-08-11T04:03:48Z

What's the usefulness of a Weirstrass representation of Curve25519? It won't interoperate with X25519 or Ed25519 implementations without additional code (projecting the results back and forth between Weierstrass and Edwards representations)

To be clear, I have nothing against adding support for X25519 or Ed25519, but I don't think the high level interface should expose the Weirstrass representation used on the backend

Test coverage missing

Please fix the email used in the commit: either change it to the one you use for github or add it as an alternative one in your profile (if you are the author)

I have to communicate with Bouncy Castle. This library implemented Curve25519 as Weierstrass Form.
I could not run the test with tox tool, how could I deal with this problem? Should I open new issue?
I changed it.

Thank you for your feedback.

src/ecdsa/ecdsa.py

tomato42 · 2020-08-11T10:11:15Z

src/ecdsa/curves.py

+Curve25519Weier = Curve("Curve25519Weier",
+                        ecdsa.curve_25519_weier,
+                        ecdsa.generator_25519_weier,
+                        (1, 3, 101, 110))


I don't know, that's the first time I've heard of people actually using Curve25519 in Weierstrass form...
Check what OID BouncyCastle uses when you write the key to PKCS#8 file?

src/ecdsa/ecdsa.py

tomato42 · 2020-08-11T10:15:50Z

src/ecdsa/__init__.py

    BRAINPOOLP320r1,
    BRAINPOOLP384r1,
    BRAINPOOLP512r1,
+    Curve25519Weier,


That PR in Javascript library uses different name still... How does BouncyCastle call it?

tomato42 · 2020-08-11T10:25:10Z

Test coverage missing

I could not run the test with tox tool, how could I deal with this problem? Should I open new issue?

With no details, I really can't help you. The tox config runs with different python versions, maybe try running with just the version you have installed? If you have python 3.7, then run it as tox -e py37. If that doesn't work try running the benchmark like tox -e speed; if that doesn't work, I'm guessing your tox installation is broken.

Please fix the email used in the commit: either change it to the one you use for github or add it as an alternative one in your profile (if you are the author)

I changed it.

I still see 37af1db with email unlinked to any github account... Are you sure you force pushed this change?

tomato42 · 2020-08-13T10:22:50Z

side note: adding interoperability tests with Bouncy Castle (like there are with OpenSSL) would be really awesome

Change name Curve25519Weier to Wei25519 refer from https://tools.ietf.org/html/draft-ietf-lwig-curve-representations-11

coveralls · 2020-08-14T08:49:26Z

Coverage increased (+0.005%) to 98.005% when pulling 7abad89 on pranithan-kang:master into 14e673d on warner:master.

coveralls · 2020-08-14T08:49:27Z

Coverage increased (+0.005%) to 98.005% when pulling 7abad89 on pranithan-kang:master into 14e673d on warner:master.

tomato42

Please fix flake warnings: https://travis-ci.org/github/warner/python-ecdsa/jobs/717847859#L374
Add unit tests (BouncyCastle generated keys and signatures we can compare or try to verify would be nice, online tests with BouncyCastle would be perfect)
we need to figure out OID handling, and that brings us back to how BC handles it, which is point 2

tomato42 · 2020-08-24T13:27:54Z

src/ecdsa/curves.py

+                        ecdsa.curve_wei_25519,
+                        ecdsa.generator_wei_25519,
+                        (1, 3, 101, 110), 
+                        "wei25519")


if a curve isn't supported in OpenSSL we shouldn't add its openssl_name, it's just that we didn't have curves like that before, so the test suite can't handle it correctly—i.e. empty openssl_name should be supported in test suite, the fact it's not is a bug

tomato42 · 2020-11-08T19:23:35Z

@pranithan-kang do you plan to continue work on this?

pranithan-kang · 2020-11-10T04:16:50Z

@tomato42 I have no idea how to deal with test suite and OpenSSL. This work is quite far from my ability. I think you can reject this pull request.

tomato42 · 2021-05-28T13:02:44Z

@pranithan-kang side note: I've just released 0.17.0 that includes support for loading curve parameters from files and both reading and writing keys with explicit curve parameters

pranithan-kang · 2021-06-03T05:39:51Z

Developer team adds the feature to include support for loading curve parameters from files and both reading and writing keys with explicit curve parameters as stated above

tomato42 requested changes Aug 10, 2020

View reviewed changes

tomato42 added the feature functionality to be implemented label Aug 10, 2020

tomato42 requested changes Aug 11, 2020

View reviewed changes

Add Weierstrass Curve 25519

a1c36b6

pranithan-kang force-pushed the master branch from 37af1db to a1c36b6 Compare August 13, 2020 04:29

Correct h parameter of Weierstrass form of Curve 25519.

cea8e94

Change name Curve25519Weier to Wei25519

7abad89

Change name Curve25519Weier to Wei25519 refer from https://tools.ietf.org/html/draft-ietf-lwig-curve-representations-11

tomato42 requested changes Aug 24, 2020

View reviewed changes

tomato42 reviewed Aug 24, 2020

View reviewed changes

tomato42 added the help wanted label May 28, 2021

pranithan-kang closed this Jun 3, 2021

Add Weierstrass Curve 25519 #207

Add Weierstrass Curve 25519 #207

Uh oh!

Conversation

pranithan-kang commented Aug 10, 2020

Uh oh!

tomato42 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranithan-kang Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranithan-kang commented Aug 11, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomato42 commented Aug 11, 2020

Uh oh!

tomato42 commented Aug 13, 2020

Uh oh!

coveralls commented Aug 14, 2020

Uh oh!

coveralls commented Aug 14, 2020

Uh oh!

tomato42 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomato42 commented Nov 8, 2020

Uh oh!

pranithan-kang commented Nov 10, 2020

Uh oh!

tomato42 commented May 28, 2021

Uh oh!

pranithan-kang commented Jun 3, 2021

Uh oh!

Uh oh!

tomato42 left a comment •

edited

Loading

pranithan-kang Aug 14, 2020 •

edited

Loading