Added support for Brainpool r1 #61

shrir · 2016-10-31T17:33:53Z

Added support for Brainpool r1 curves. I might not have searched hard enough, but can't find the ecdsa test vectors for brainpool curves to add rfc6979 unit tests.

Note: requires openssl 1.0.2

fixes #61

tomato42 · 2016-11-02T12:16:39Z

first, looks like the tests should detect if the installed version of OpenSSL supports brainpool curves and skip them if that's not the case

second, could you add test vectors for the new curves, so that they can be tested without using OpenSSL?

ecdsa/curves.py

ecdsa/test_pyecdsa.py

TrinityTonic · 2017-10-27T11:05:57Z

Any updates on including more curves (such as brainpool) to this library?

tomato42 · 2018-09-04T21:31:50Z

@TrinityTonic I'd like to do that, but:

a). the issues I listed above need to be addressed
b). the PR needs to be rebased

tomato42

r-, as per above comments

tomato42 · 2018-09-10T12:46:44Z

Please rebase your PR on top of current master branch, then force push to the same branch as this PR is using.

Do not merge the master branch to this branch to update it, do not use github web interface to update the branch.

tomato42 · 2018-09-10T12:49:20Z

src/ecdsa/test_pyecdsa.py

+
+        if curvename not in OpenSSL.supported_curves:
+            return
+


this will cause the test to be reported as passing instead of skipped, please use pytest mechanism to skip the test case

the current code is using decorators:
https://github.com/warner/python-ecdsa/blob/e29ddec6042450335cb0490e8a53e440ab7f02c5/src/ecdsa/test_pyecdsa.py#L739-L740

tomato42

Test vectors are missing

tomato42 · 2018-09-10T12:53:42Z

please also add the curves to speed.py

tomato42 · 2018-09-10T13:03:02Z

I've noticed that in the opening comment you said that you can't find tests, what about the ones in RFC6932 and RFC7027?

alternatively, there is ECTester project that seems to support brainpool, so it could be used to generate some test cases

worst case, if there are none, please create them yourself – I like to see at least some safety net

merlokk · 2019-11-01T12:47:21Z

ecdh for brainpool test vectors
https://tools.ietf.org/html/rfc7027#appendix-A
https://tools.ietf.org/html/rfc6932#appendix-A.1

tomato42

please rebase on top of current master, the test coverage changed significantly so this PR is not mergable as-is

except that, there are a lot of lines that go over the 80 char limit, making reading side-by-side diffs rather frustrating

side note: I was considering if not to use the "BP" acronym, instead of "BRAINPOOL", in the names of the curves. I think that would improve usability. (note: it's not a request for a change, just a question if you considered it, or would be ok with the solution)

tomato42 · 2019-11-01T23:26:58Z

src/ecdsa/test_pyecdsa.py

+
+        if curvename not in OpenSSL.supported_curves:
+            return
+


the current code is using decorators:
https://github.com/warner/python-ecdsa/blob/e29ddec6042450335cb0490e8a53e440ab7f02c5/src/ecdsa/test_pyecdsa.py#L739-L740

tomato42 · 2019-11-01T23:28:58Z

src/ecdsa/test_pyecdsa.py

+        Z = dA * qB
+        self.assertEqual(Point(curve, x_qA, y_qA), qA)
+        self.assertEqual(Point(curve, x_qB, y_qB), qB)
+        self.assertTrue((dA * qB) == (dA * dB * generator) == (dB * dA * generator) == (dB * qA))


nit: too long line

tomato42 · 2019-11-01T23:29:29Z

src/ecdsa/test_pyecdsa.py

+            generator=BRAINPOOLP224r1.generator,
+            dA=int("7C4B7A2C8A4BAD1FBB7D79CC0955DB7C6A4660CA64CC4778159B495E", 16),
+            x_qA=int("B104A67A6F6E85E14EC1825E1539E8ECDBBF584922367DD88C6BDCF2", 16),
+            y_qA=int("46D782E7FDB5F60CD8404301AC5949C58EDB26BC68BA07695B750A94", 16),


those lines are too long too

still unchanged?

tomato42 · 2019-11-01T23:30:47Z

src/ecdsa/ecdsa.py

+_q = 0xE95E4A5F737059DC60DF5991D45029409E60FC09
+
+curve_brainpoolp160r1 = ellipticcurve.CurveFp( _p, _a, _b)
+generator_brainpoolp160r1 = ellipticcurve.Point( curve_brainpoolp160r1, _Gx, _Gy, _q)


nit: line too long

shrir · 2019-11-02T01:24:33Z

side note: I was considering if not to use the "BP" acronym, instead of "BRAINPOOL", in the names of the curves. I think that would improve usability. (note: it's not a request for a change, just a question if you considered it, or would be ok with the solution)

So curve_brainpoolp512r1 will be curve_bpp512r1 and BRAINPOOLP512r1 will become BPP512r1, personally I prefer BRAINPOOL but I don't have a strong opinion.

coveralls · 2019-11-02T07:57:00Z

Coverage increased (+0.1%) to 94.974% when pulling 2a4c0a8 on shrir:brainpool into e29ddec on warner:master.

coveralls · 2019-11-02T07:57:00Z

Coverage increased (+0.3%) to 95.092% when pulling 053cd9c on shrir:brainpool into e29ddec on warner:master.

tomato42 · 2019-11-02T09:19:08Z

src/ecdsa/test_pyecdsa.py

+        return self.do_test_from_openssl(BRAINPOOLP512r1)
+
+    @pytest.mark.skipif("" not in OPENSSL_SUPPORTED_CURVES,
+                        reason="system openssl does not support ")


A bit too aggressive search-and-replace?

src/ecdsa/test_pyecdsa.py

tomato42 · 2019-11-02T09:23:04Z

src/ecdsa/test_pyecdsa.py

+        self.assertEqual(len(s1), BRAINPOOLP512r1.verifying_key_length)
+        pub2 = VerifyingKey.from_string(s1, curve=BRAINPOOLP512r1)
+        self.assertTruePubkeysEqual(pub1, pub2)
+


same here, I don't see why it couldn't be a separate test case

tomato42 · 2019-11-02T09:30:29Z

side note: I was considering if not to use the "BP" acronym, instead of "BRAINPOOL", in the names of the curves. I think that would improve usability. (note: it's not a request for a change, just a question if you considered it, or would be ok with the solution)

So curve_brainpoolp512r1 will be curve_bpp512r1 and BRAINPOOLP512r1 will become BPP512r1, personally I prefer BRAINPOOL but I don't have a strong opinion.

ok, then the speed.py formatting needs to be fixed, now the output from brainpool curves is offset:

            siglen    keygen   keygen/s      sign     sign/s    verify   verify/s
  NIST192p:     48   0.03455s     28.95   0.01638s     61.04   0.03085s     32.41
  NIST224p:     56   0.04240s     23.58   0.02224s     44.96   0.04210s     23.75
  NIST256p:     64   0.05570s     17.95   0.02957s     33.82   0.05830s     17.15
  NIST384p:     96   0.13041s      7.67   0.06814s     14.68   0.13394s      7.47
  NIST521p:    132   0.26017s      3.84   0.13394s      7.47   0.26327s      3.80
 SECP256k1:     64   0.05512s     18.14   0.02923s     34.22   0.05771s     17.33
BRAINPOOLP160r1:     40   0.02136s     46.82   0.01090s     91.71   0.02103s     47.55
BRAINPOOLP192r1:     48   0.03185s     31.40   0.01659s     60.27   0.03185s     31.40
BRAINPOOLP224r1:     56   0.04961s     20.16   0.02185s     45.77   0.04310s     23.20
BRAINPOOLP256r1:     64   0.05872s     17.03   0.02899s     34.49   0.05661s     17.66
BRAINPOOLP320r1:     80   0.09441s     10.59   0.04773s     20.95   0.09699s     10.31
BRAINPOOLP384r1:     96   0.13630s      7.34   0.07119s     14.05   0.13544s      7.38
BRAINPOOLP512r1:    128   0.25540s      3.92   0.12683s      7.88   0.24935s      4.01

tomato42

looks good, thank you, and sorry for the nitpicking

tomato42 reviewed Nov 2, 2016

View reviewed changes

ecdsa/curves.py Outdated Show resolved Hide resolved

ecdsa/curves.py Outdated Show resolved Hide resolved

ecdsa/test_pyecdsa.py Outdated Show resolved Hide resolved

tomato42 requested changes Sep 4, 2018

View reviewed changes

tomato42 added the feature functionality to be implemented label Sep 5, 2018

tomato42 added this to the someday/future milestone Sep 5, 2018

tomato42 reviewed Sep 10, 2018

View reviewed changes

tomato42 requested changes Sep 10, 2018

View reviewed changes

tomato42 requested changes Nov 1, 2019

View reviewed changes

bhate and others added 5 commits November 2, 2019 02:10

Added support for Brainpool r1

99ecf70

Apply brainpool to new files after merge

9f0fe31

Add RFC 7027 test vectors

8b4309d

Skip tests if the curves are not supported by OpenSSL

b758369

Add RFC 6932 test vectors

b513c1d

shrir force-pushed the brainpool branch from 8952c43 to b513c1d Compare November 2, 2019 01:17

shrir added 3 commits November 2, 2019 02:30

Add brainpool curves to __all__

47a6da5

Remove leftover code

161630f

Add brainpool curves to __init__.py

053cd9c

shrir requested a review from tomato42 November 2, 2019 09:13

tomato42 reviewed Nov 2, 2019

View reviewed changes

src/ecdsa/test_pyecdsa.py Show resolved Hide resolved

tomato42 reviewed Nov 2, 2019

View reviewed changes

shrir added 2 commits November 2, 2019 12:15

Move some brainpool key tests in separate tests

a6d59be

Fix formatting in speed.py

2a4c0a8

shrir requested a review from tomato42 November 2, 2019 12:10

tomato42 approved these changes Nov 2, 2019

View reviewed changes

tomato42 modified the milestones: someday/future, v0.14 Nov 2, 2019

tomato42 merged commit a012d72 into tlsfuzzer:master Nov 2, 2019

tomato42 mentioned this pull request Nov 2, 2019

add Brainpool curves #4

Closed

Added support for Brainpool r1 #61

Added support for Brainpool r1 #61

Uh oh!

Conversation

shrir commented Oct 31, 2016 • edited by tomato42 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomato42 commented Nov 2, 2016

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TrinityTonic commented Oct 27, 2017

Uh oh!

tomato42 commented Sep 4, 2018

Uh oh!

tomato42 left a comment

Choose a reason for hiding this comment

Uh oh!

tomato42 commented Sep 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomato42 left a comment

Choose a reason for hiding this comment

Uh oh!

tomato42 commented Sep 10, 2018

Uh oh!

tomato42 commented Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

merlokk commented Nov 1, 2019

Uh oh!

tomato42 left a comment

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!

shrir commented Nov 2, 2019

Uh oh!

coveralls commented Nov 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomato42 commented Nov 2, 2019

Uh oh!

tomato42 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shrir commented Oct 31, 2016 •

edited by tomato42

Loading

tomato42 commented Sep 10, 2018 •

edited

Loading

coveralls commented Nov 2, 2019 •

edited

Loading