Cleanup pyUmbral codebase and resolve various issues by tuxxy · Pull Request #127 · nucypher/pyUmbral

tuxxy · 2018-04-23T17:49:10Z

What this does:

Moves some OpenSSL functions out of BigNum (now CurveBN) and Point to the openssl module and resolves the third TODO in Clean-up the codebase #119.
These operations include:
1. BN init/creation
2. Getting curve domain parameters (group, order, generator)
3. Checking if a BN is on a provided curve
4. Converting a Python int to a BN and checking if the new BN is on a curve
5. EC_POINT init/creation
6. EC_POINT creation via affine coordinates
7. Getting affine coordinates via an EC_POINT.
Renames BigNum to CurveBN and resolves the first TODO in Clean-up the codebase #119.
Resolves Cache UmbralPublicKey in UmbralPrivateKey.get_pubkey()? #121 by caching the public key on UmbralPrivateKey.
Resolves Calculate expected lengths of Capsule and fragment classes #56 by adding get_size classmethods to CurveBN, Point, KFrag, CapsuleFrag, and ChallengeResponse.
Resolves BigNum.__hash__ - is this what we want? #116 by changing Capsule._attached_cfrags to a list.

Use BN_cmp instead of BN_is_zero since it's not exposed Add EC_POINT operations to openssl module

Change all references from BigNum to CurveBN

CorrectnessProof. Fix syntax error

cygnusv

Good work, tuxxy :) Just some minor comments, apart from the remaining issue of CurveBN vs BigNum (which we decided to leave it as that for the moment)

cygnusv · 2018-04-24T20:51:24Z

tests/test_keys/test_umbral_keys.py

-def test_private_key_serialization(random_ec_bignum1):
-    priv_key = random_ec_bignum1
+def test_private_key_serialization(random_ec_curvebn1):
+    priv_key = random_ec_curvebn1


random_ec_curvebn1 reads strange. The c in ec is already "curve", so perhaps you should drop ec from the name. Apart from that – and I don't want to add fuel to the fire – I have to say I don't see immediately that curvebn1 is a BigNum, since the bn bit doesn't stand out. Perhaps random_curve_bn1?

⛽️ 🔥

Yeah, I noticed this when I was refactoring it. I can change it now, or when I pull CurveBN out later. Is there a preference?

This also refactors the pytest fixtures, if we change it too.

cygnusv · 2018-04-24T20:53:48Z

tests/test_primitives/conftest.py

-    def check_bignum_ctypes(*bignums):
-        for bn in bignums:
+    def check_curvebn_ctypes(*curvebns):
+        for bn in curvebns:


There's definitely an issue when CurveBN is all in lowercase...

Yeah, this is true.

cygnusv · 2018-04-24T21:06:10Z

umbral/curvebn.py

+class CurveBN(object):
    """
-    Represents an OpenSSL BIGNUM except more Pythonic
+    Represents an OpenSSL Bignum modulo the order of a curve.


Perhaps we should add here that some operations only work modulo a prime

Something like, "Some of these operations only work with a prime modulus, ie: a prime order curve."?

Something like that, yes

cygnusv · 2018-04-24T21:09:54Z

umbral/curvebn.py

    def gen_rand(cls, curve: ec.EllipticCurve = None):
        """
-        Returns a BigNum object with a cryptographically secure BigNum based
+        Returns a CurveBN object with a cryptographically secure Bignum based


What's a Bignum here? you mean an OpenSSL BIGNUM or just a generic Big Number?

Yeah, an OpenSSL BIGNUM.

cygnusv · 2018-04-24T21:18:27Z

umbral/fragments.py

+        point_size = Point.get_size(curve)
+
+        return (bn_size * 4) + (point_size * 2)
+


Just great!

cygnusv · 2018-04-24T21:21:10Z

umbral/fragments.py

+        """
+        Returns the size (in bytes) of a CapsuleFrag given the curve.
+        If no curve is provided, it will use the default curve.
+        """


We should mention that the CapsuleFrag contains a CorrectnessProof object of variable size, which is not considered in this method

cygnusv · 2018-04-24T21:27:23Z

umbral/openssl.py

+    ec_order = _get_ec_order_by_curve_nid(curve_nid)
+
+    # is_zero workaround since BN_is_zero is not exposed yet.
+    is_zero = backend._lib.BN_cmp(check_bn, backend._int_to_bn(0))


is_zero should be greater_than_zero, right?

I think below this line I check that is_zero == 1, right?

What I mean is that the name is_zero doesn't make sense. You are not testing if check_bn == 0, but getting a comparison that returns -1, 0 or 1.

🤔 I haven't thought about it like that. Given the comment above it, it should be enough to determine, but I think I might prefer bn_is_zero over greater_than_zero. Thoughts?

Ah OK, I see what you mean here. You intention is to validate that check_bn is different than 0, but you end up comparing if check_bn > 0. Both are equivalent for BIGNUMs. In any case, the name is_zero doesn't seem very appropriate, because you end up testing if check_bn is greater than 0.

Specifically, I am computing whether this statement is true:
0 < check_bn < ec_order

How do you mean that check_bn > 0 and is_zero are both equivalent?

cygnusv · 2018-04-24T21:46:21Z

umbral/point.py

+        If no curve is provided, it uses the default curve.
+        """
+        curve = curve if curve is not None else default_curve()
+        return get_curve_keysize_bytes(curve)


Shouldn't the size be +1 for compressed points, in order to account for the 0x02 (or 0x03) byte at the beginning? Apart from that, there is no guarantee that the underlying field order has the same size than keys, although I can't think of a counterexample.

Maybe the comment should say that this is the size of a curve field element (e.g., a coordinate of the point), which is the size of a compressed point without the 0x02 byte

Oh, yep. You're absolutely right, good catch.

cygnusv · 2018-04-24T22:12:58Z

umbral/utils.py

 def get_curve_keysize_bytes(curve):
+    # We use the ceil operation to fit all bytes on curve sizes where eight is
+    # not evenly divisible.
    return int(math.ceil(curve.key_size / 8.00))


I think it's more common to see this expressed as (curve.key_size + 7) // 8

See e.g., https://github.com/pyca/cryptography/blob/master/src/cryptography/utils.py#L66

It also seems to be much faster:

>>> timeit.repeat('for i in range(1024): (i+7)//8', number=10000, repeat=3) [2.6502825429779477, 2.350924202008173, 2.525679681042675] >>> timeit.repeat('for i in range(1024): int(math.ceil(i/8.00))', number=10000, setup='import math') [6.333626834035385, 6.332635344995651, 6.0212114860187285]

Very nice, thanks!

Remove unused Capsule._contents

michwill

Read through, looks great! Of course, keeping in mind what we discussed today

cygnusv · 2018-04-25T10:47:00Z

umbral/curvebn.py

+        order = openssl._get_ec_order_by_curve_nid(curve_nid)

+        new_rand_bn = openssl._get_new_BN()
        rand_res = backend._lib.BN_rand_range(new_rand_bn, order)


I just noticed that this allows the BN to be 0, which we don't want (we want it to be in the interval [1, q-1], for q the order of the curve). A better way would be to call BN_rand_range(new_rand_bn, q_minus_1) and then add 1 to the result. This ensures the resulting BN is in [1, q-1]

BN_rand_range generates numbers where 0 <= RAND < Order.

So, it's still possible that a number can be generated as 0, but not as the actual order.

…ated bn is on curve; update CurveBN docstring

tuxxy force-pushed the cleanup branch from 87935f5 to a60973b Compare April 23, 2018 19:10

tuxxy changed the title ~~[WIP] Cleanup code by moving some OpenSSL operations out of BigNum and Point~~ [WIP] Cleanup pyUmbral codebase and resolve various issues Apr 23, 2018

tuxxy force-pushed the cleanup branch 7 times, most recently from 64df4cc to cdcf50f Compare April 23, 2018 19:36

tuxxy requested review from cygnusv, jMyles and michwill April 23, 2018 19:39

tuxxy force-pushed the cleanup branch from ee73ea8 to ba8c0e3 Compare April 24, 2018 18:23

tuxxy added 4 commits April 24, 2018 12:24

Create openssl module and add OpenSSL helper utilities

920b705

Use BN_cmp instead of BN_is_zero since it's not exposed Add EC_POINT operations to openssl module

Implement OpenSSL module in BigNum, Point, and key classes

6a1cb68

Rename BigNum -> CurveBN

923ad29

Change all references from BigNum to CurveBN

Cache pubkey on UmbralPrivateKey for nucypher#121

2dfb69e

tuxxy force-pushed the cleanup branch 2 times, most recently from af358fb to b8af78a Compare April 24, 2018 18:48

tuxxy changed the title ~~[WIP] Cleanup pyUmbral codebase and resolve various issues~~ Cleanup pyUmbral codebase and resolve various issues Apr 24, 2018

Add get_size classmethods for CurveBN, Point, fragments, and

150714f

CorrectnessProof. Fix syntax error

tuxxy force-pushed the cleanup branch from d6f748f to db38a4e Compare April 24, 2018 22:28

cygnusv approved these changes Apr 24, 2018

View reviewed changes

Remove __hash__ on CurveBN and make Capsule._attached_cfrags a list

6c2dd86

Remove unused Capsule._contents

tuxxy force-pushed the cleanup branch from db38a4e to 6c2dd86 Compare April 24, 2018 22:39

tuxxy added 3 commits April 24, 2018 16:52

Use better method for determining keysize in bytes (thanks @cygnusv)

92ec0f2

Include y-byte in Point.get_size

2b8fe61

Improve docstrings a bit

7eec2a0

michwill approved these changes Apr 25, 2018

View reviewed changes

cygnusv reviewed Apr 25, 2018

View reviewed changes

Make openssl _bn_is_on_curve vars more clear; check if randomly gener…

8261399

…ated bn is on curve; update CurveBN docstring

tuxxy merged commit 60c7ef6 into nucypher:master Apr 27, 2018

tuxxy deleted the cleanup branch April 27, 2018 16:58

		point_size = Point.get_size(curve)

		return (bn_size * 4) + (point_size * 2)

Conversation

tuxxy commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this does:

Uh oh!

cygnusv 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!

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!

tuxxy Apr 24, 2018 • 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!

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!

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!

michwill left a comment

Choose a reason for hiding this comment

Uh oh!

cygnusv Apr 25, 2018 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tuxxy commented Apr 23, 2018 •

edited

Loading

tuxxy Apr 24, 2018 •

edited

Loading

cygnusv Apr 25, 2018 •

edited

Loading