Simplified API #159

cygnusv · 2018-05-31T13:02:28Z

What this does:

This PR provides a slightly simplified API for pyUmbral. In particular, removes all redundant arguments from public and private methods that could be obtained by other means (primarily, from a neighboring argument), such as params and correctness keys. The result is not only that the interface is simplified, but also that there is less surface for misuse.

Simplified public API for umbral.pre and umbral.fragments:
- Methods now only take Umbral keys as arguments, rather than primitive types (Point, CurveBN).
- Remove unnecessary arguments from public facing and internal methods when they can be extracted from a Capsule, UmbralPublicKey or UmbralPrivateKey.
- Adapts the test suite to new simplified API
Fixes a bug in Capsule.attach_cfrag() that allowed to attach incorrect CFrag
Adds a getter in Capsule for correctness keys
Removes pre.CHACHA20_KEY_SIZE constant and uses dem.DEM_KEYSIZE instead
Add __eq__ method to UmbralParameters

codecov · 2018-05-31T13:06:12Z

Codecov Report

Merging #159 into master will decrease coverage by 0.57%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master     #159      +/-   ##
==========================================
- Coverage    96.1%   95.53%   -0.58%     
==========================================
  Files          12       12              
  Lines        1129     1141      +12     
==========================================
+ Hits         1085     1090       +5     
- Misses         44       51       +7

Impacted Files	Coverage Δ
umbral/params.py	`100% <100%> (ø)`	⬆️
umbral/fragments.py	`93.96% <100%> (-0.21%)`	⬇️
umbral/keys.py	`95.86% <100%> (-0.12%)`	⬇️
umbral/point.py	`95.89% <100%> (+0.71%)`	⬆️
umbral/curvebn.py	`97.16% <100%> (-0.71%)`	⬇️
umbral/pre.py	`96.61% <90.9%> (-1.5%)`	⬇️
umbral/_pre.py	`96.2% <93.75%> (-0.82%)`	⬇️
umbral/signing.py	`89.28% <0%> (-3.58%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee35a52...fa4375d. Read the comment docs.

jMyles · 2018-05-31T16:11:25Z

umbral/pre.py


        components = splitter(capsule_bytes)
-        return cls(*components)
+        return cls(*components, params=params)


Ahh right, that's obviously needed. It's a little unnerving that, without this, we didn't have a failing test.

Yep, that's why I'm leaning towards removing redundant params as argument in some places (assuming there's a capsule or other structure already holding params) and making it non-optional in others (e.g., in Umbral key generation).

jMyles · 2018-05-31T16:12:00Z

umbral/pre.py

@@ -132,6 +132,8 @@ def _set_cfrag_correctness_key(self, key_type, key: UmbralPublicKey):
        if current_key is None:
            if key is None:
                raise TypeError("The {} key is not set and you didn't pass one.".format(key_type))
+            elif self._umbral_params != key.params:
+                raise TypeError("You are trying to set a key with different UmbralParameters.")


Come on man, this is what segfaults are for.

tuxxy · 2018-06-04T16:34:21Z

tests/test_capsule/test_capsule_operations.py

@@ -28,43 +35,52 @@ def test_capsule_creation(alices_keys):


 def test_capsule_equality():
-    one_capsule = Capsule(point_e=Point.gen_rand(),
+    params = default_params()


We pass this in a ton. Should we make this a pytest fixture?

Yes, we should. Perhaps in another PR?

tuxxy · 2018-06-04T16:35:19Z

tests/test_capsule/test_capsule_serializers.py

@@ -39,26 +40,29 @@ def test_activated_capsule_serialization(alices_keys, bobs_keys):
    delegating_pubkey = delegating_privkey.get_pubkey()
    signer_alice = Signer(signing_privkey)

-    receiving_privkey, receiving_pubkey = bobs_keys
+    params = delegating_privkey.params


This is an interesting paradigm where each key and capsule can have its own params.

This might be a great pattern in the future, but we should be careful with it for now.

Agreed on both points 100%. Interesting but be careful.

tuxxy · 2018-06-04T16:36:23Z

tests/test_simple_api.py

+
+
+@pytest.mark.parametrize("N, M", parameters)
+def test_lifecycle_with_serialization(N, M, curve=default_curve()):


👍 Looks really good.

tuxxy · 2018-06-04T16:38:08Z

umbral/fragments.py

@@ -148,7 +150,7 @@ def to_bytes(self) -> bytes:

        return result

-    def _bn_keytes__(self):
+    def _bytes__(self):


This needs to be __bytes__ :)

tuxxy · 2018-06-04T16:39:21Z

umbral/keys.py

@@ -117,6 +114,12 @@ def to_bytes(self, password: bytes=None, _scrypt_cost: int=20,

        return umbral_privkey

+    def __bytes__(self):


We don't offer __bytes__ on UmbralPrivateKey to prevent accidental misuse.

I think this should be removed.

I agree with @tuxxy here. What's the use case?

None. It was a silly move 🤦‍♂️ ... Fixed.

tuxxy · 2018-06-04T16:46:07Z

umbral/keys.py

@@ -82,8 +79,8 @@ def from_bytes(cls, key_bytes: bytes, params: UmbralParameters=None,
        bn_key = CurveBN.from_bytes(key_bytes, params.curve)
        return cls(bn_key, params)

-    def to_bytes(self, password: bytes=None, _scrypt_cost: int=20,
-                 encoder: Callable=None):
+    def to_bytes(self, password: bytes = None, _scrypt_cost: int = 20,


In instances where you set a default value with type in the function like the _scrypt_cost variable here, the equal sign should have no spaces such that:

_scrypt_cost: int=20

In cases where there is no type, such as None, it should have a space:

password: bytes = None

https://www.python.org/dev/peps/pep-0008/#other-recommendations

I'm not necessarily a huge fan of this PEP8 rule, as such I will leave it up to everyone else to decide how to do this. Generally, I'm in favor of not doing this as it just isn't necessary.

I used to think this was a really dumb style, but then I came around to it. The thing is: in kwargs, x=y looks like assignment. With the space around it, your eyes learn to treat the space as a type hint instead of argument assignment.

🤔 But it's not a type hint? It's a default assignment if the argument is not passed?

Eh? It is a type hint. Without the space, it looks like you're assigning the value None to the name bytes.

Oh, I see! Gotcha.

tuxxy · 2018-06-04T16:49:03Z

umbral/signing.py

@@ -21,7 +20,7 @@ class Signature:
    between (r, s) and DER formatting.
    """

-    def __init__(self, r: int, s: int):
+    def __init__(self, r: CurveBN, s: CurveBN):


If we're going to use CurveBN here, this should be in a separate PR. I'm not quite sure what the best options here are yet and I want to think about it more.

Yep, agreed.

tuxxy · 2018-06-04T16:49:37Z

umbral/signing.py


    def __bytes__(self):
-        return self.r.to_bytes(32, "big") + self.s.to_bytes(32, "big")
+        return self.r.to_bytes() + self.s.to_bytes()


jMyles

One of the best PRs in the history of this project. Bravo.

There are some outstanding concerns, though, so I'll wait a bit to approve.

jMyles · 2018-06-04T17:52:05Z

tests/test_capsule/test_capsule_operations.py

@@ -28,43 +35,52 @@ def test_capsule_creation(alices_keys):


 def test_capsule_equality():
-    one_capsule = Capsule(point_e=Point.gen_rand(),
+    params = default_params()


jMyles · 2018-06-04T17:53:18Z

tests/test_capsule/test_capsule_operations.py

-    one_capsule = Capsule(point_e=Point.gen_rand(),
+    params = default_params()
+
+    one_capsule = Capsule(params,


Wow - big change.

This is pretty cool I think.

But I'm left wondering: if Capsule is going to require params, does it perhaps make sense to make it private? Is there really ever a public reason for someone to init a Capsule?

jMyles · 2018-06-04T17:53:57Z

tests/test_capsule/test_capsule_operations.py

    delegating_privkey, _signing_privkey = alices_keys

-    sym_key, capsule = pre._encapsulate(delegating_privkey.get_pubkey().point_key)
+    sym_key, capsule = pre._encapsulate(delegating_privkey.get_pubkey())


Oh hallelujah. Holy shit what a relief.

jMyles · 2018-06-04T17:54:14Z

tests/test_capsule/test_capsule_operations.py

    assert len(sym_key) == 32

    # The symmetric key sym_key is perhaps used for block cipher here in a real-world scenario.
-    sym_key_2 = pre._decapsulate_original(delegating_privkey.bn_key, capsule)
+    sym_key_2 = pre._decapsulate_original(delegating_privkey, capsule)


oh my goodness this is seriously amazing. This is how pyUmbral has always wanted to read. (Although again, as I outlined in #161, I might advocate even going all the way down to just delegating_key or delegating_secret and dropping the "priv" idea).

jMyles · 2018-06-04T17:56:06Z

tests/test_capsule/test_capsule_serializers.py

@@ -39,26 +40,29 @@ def test_activated_capsule_serialization(alices_keys, bobs_keys):
    delegating_pubkey = delegating_privkey.get_pubkey()
    signer_alice = Signer(signing_privkey)

-    receiving_privkey, receiving_pubkey = bobs_keys
+    params = delegating_privkey.params


Agreed on both points 100%. Interesting but be careful.

* Instead, takes params from Capsules or UmbralPublicKeys * Makes params required in some places (Capsule.init, Capsule.from_bytes, etc) * Removes pre.CHACHA20_KEY_SIZE constant and use dem.DEM_KEYSIZE instead

* Functions in `pre` now only take Umbral keys as arguments, rather than primitive types (Point, CurveBN) * Remove unnecessary arguments from public facing and internal methods when they can be extracted from a Capsule, UmbralPublicKey or UmbralPrivateKey * Adds a getter in Capsule for correctness keys * Adapts the test suite to new simplified API

tuxxy

🐧

jMyles reviewed May 31, 2018

View reviewed changes

cygnusv force-pushed the params-in-capsule branch from c0d5c3b to 190b0f0 Compare June 4, 2018 08:11

cygnusv changed the title ~~[WIP] When possible, take params from Capsule~~ [WIP] Simplified API Jun 4, 2018

cygnusv force-pushed the params-in-capsule branch from 0d28fa6 to 09832ec Compare June 4, 2018 15:34

cygnusv requested review from jMyles, michwill, tuxxy and KPrasch June 4, 2018 16:06

cygnusv changed the title ~~[WIP] Simplified API~~ Simplified API Jun 4, 2018

tuxxy reviewed Jun 4, 2018

View reviewed changes

jMyles reviewed Jun 4, 2018

View reviewed changes

cygnusv added 8 commits June 5, 2018 02:11

Add __eq__ method to UmbralParameters

9170b70

Add params when deserializing Capsule

03b4138

Check that params match when setting correctness keys

c98f723

Remove keypairs fixtures from test_simple_api

09bed34

Remove params as an argument when possible

c736370

* Instead, takes params from Capsules or UmbralPublicKeys * Makes params required in some places (Capsule.init, Capsule.from_bytes, etc) * Removes pre.CHACHA20_KEY_SIZE constant and use dem.DEM_KEYSIZE instead

Adapts tests to new way to use params

d5b434c

Fixes bug in Capsule.attach_cfrag that allowed to attach incorrect CFrag

9e126bd

cygnusv force-pushed the params-in-capsule branch from 09832ec to 9e126bd Compare June 5, 2018 01:20

cygnusv added 3 commits June 5, 2018 03:49

Fix some functions definition according to PEP8

d70d21d

Fix __bytes__ definition in CorrectnessProof

d34fd6e

Adapt verify_kfrag to simplified API (I missed this in the main commit)

fa4375d

michwill approved these changes Jun 5, 2018

View reviewed changes

jMyles approved these changes Jun 5, 2018

View reviewed changes

cygnusv mentioned this pull request Jun 6, 2018

Serialization fixes for multicurve #162

Merged

tuxxy approved these changes Jun 6, 2018

View reviewed changes

cygnusv merged commit 9e3a037 into nucypher:master Jun 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplified API #159

Simplified API #159

cygnusv commented May 31, 2018 •

edited

Loading

codecov bot commented May 31, 2018 •

edited

Loading

jMyles May 31, 2018

cygnusv May 31, 2018

jMyles May 31, 2018

tuxxy Jun 4, 2018

jMyles Jun 4, 2018

cygnusv Jun 4, 2018

tuxxy Jun 4, 2018

jMyles Jun 4, 2018

tuxxy Jun 4, 2018

tuxxy Jun 4, 2018

cygnusv Jun 4, 2018

tuxxy Jun 4, 2018

jMyles Jun 4, 2018

cygnusv Jun 4, 2018

tuxxy Jun 4, 2018

jMyles Jun 4, 2018

tuxxy Jun 4, 2018

jMyles Jun 4, 2018

tuxxy Jun 4, 2018

tuxxy Jun 4, 2018

jMyles Jun 4, 2018

tuxxy Jun 4, 2018

jMyles left a comment

jMyles Jun 4, 2018

jMyles Jun 4, 2018

jMyles Jun 4, 2018

jMyles Jun 4, 2018

jMyles Jun 4, 2018

tuxxy left a comment



		@pytest.mark.parametrize("N, M", parameters)
		def test_lifecycle_with_serialization(N, M, curve=default_curve()):

		@@ -117,6 +114,12 @@ def to_bytes(self, password: bytes=None, _scrypt_cost: int=20,

		return umbral_privkey

		def __bytes__(self):

Simplified API #159

Simplified API #159

Conversation

cygnusv commented May 31, 2018 • edited Loading

What this does:

codecov bot commented May 31, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jMyles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuxxy left a comment

Choose a reason for hiding this comment

cygnusv commented May 31, 2018 •

edited

Loading

codecov bot commented May 31, 2018 •

edited

Loading