Re-encryption with correctness proofs by cygnusv · Pull Request #115 · nucypher/pyUmbral

cygnusv · 2018-04-17T10:44:16Z

What this does:

Renames ChallengeResponse to CorrectnessProof, and integrates it inside CFrags (Solves Integrate the challenge protocol as part of the re-encryption request/response #100)
Re-encryption now produces correctness proofs by default, although this can be disabled with the parameter provide_proof.
Re-encryption correctness is verified during decryption. If it fails, a new type of exception called UmbralCorrectnessError is raised, which includes a list of the CFrags that don't pass the correctness verification (so that Bob can take the action he considers more convenient).
Metadata is now integrated into the CorrectnessProof (and hence, into the CFrag). As a consequence, metadata is now a parameter of pre.reencrypt() (although optional).
Metadata can be any arbitrary bytes object (Closes Define format for re-encryption metadata? #120)
Tests for everything

…rify_correctness_proof

TODO: Figure out final size for CorrectnessProofs

…the incorrect cfrags

…ctness-proof

…out proofs and with/without metadata

…ctness-proof # Conflicts: # umbral/pre.py

tuxxy

Looks good, see my comments.

tuxxy · 2018-04-23T21:18:27Z

tests/test_keys/test_key_fragments.py

+                 'timestamp' : time.time(), 
+                 'capsule' : bytes(capsule), 
+               }
+    metadata = str(metadata).encode()


Obviously, we want a better form of serialization than just turning a dict into a string.

I presume this is being blocked due to not having a format for metadata in the KMS?

Well, to me this is just a rough example for testing purposes. We want something reasonable, beyond a simple b"this is the metadata of a request", but without being something too complex.

In a way, I think b"this is the metadata of a request" is better.

In this case, we're showing casting a dict, with an int for ursula_id, and time.time()? I mean, are we really nudging the user in a good direction here?

I think it's probably better to either structure metadata or leave it as flat, arbitrary bytes.

OK, let's go the the initial approach then.

tuxxy · 2018-04-23T21:27:18Z

umbral/fragments.py

+        sig = BigNum.from_bytes(data.read(key_size), curve)
+
+        metadata = data.read()
+        if metadata == bytes(0):


This is build a bit weirdly.

You can simply do metadata = data.read() or None. If there is no data, it will be b'' which loses precedence to None, then you don't have to set it to None.

That is unless you are setting metadata to be 0x00 and it will always have that byte, but I don't like that.

tuxxy · 2018-04-23T21:29:58Z

umbral/fragments.py

+            + sig              
+
+        if self.metadata is not None:
+            result = result + self.metadata


You can also do result += self.metadata or b''. This will concat the empty bytestring if self.metadata is None and removes the need for L135.

tuxxy · 2018-04-23T21:30:34Z

umbral/fragments.py


-        return cls(e1, v1, kfrag_id, eph_ni)
+        proof = data.read()
+        proof = CorrectnessProof.from_bytes(proof, curve) if proof != bytes(0) else None


Same logic applies here from what I listed above.

tuxxy · 2018-04-23T21:30:45Z

umbral/fragments.py

+        serialized_cfrag = e1 + v1 + kfrag_id + eph_ni
+
+        if self.proof is not None:
+            serialized_cfrag += self.proof.to_bytes()


As well as here.

mmm, no, it fails when self.proof is None. I'll revert it to the previous way.

tuxxy · 2018-04-23T21:36:34Z

umbral/pre.py


-def _open_capsule(capsule: Capsule, bob_private_key: UmbralPrivateKey,
-                  alice_pub_key: UmbralPublicKey, params: UmbralParameters=None) -> bytes:
+def _open_capsule(capsule: Capsule, bob_privkey: UmbralPrivateKey,


tuxxy · 2018-04-23T21:37:10Z

umbral/pre.py

-    priv_b = bob_private_key.bn_key
-    pub_b = priv_b * params.g
+    priv_b = bob_privkey.bn_key
+    pub_b = bob_privkey.get_pubkey().point_key


If my PR gets in before this one, you can do bob_privkey.pubkey.point_key because it gets cached.

tuxxy · 2018-04-23T21:37:55Z

umbral/pre.py

-    pub_a = alice_pub_key.point_key
+    pub_a = alice_pubkey.point_key
+
+    # TODO: Change dict for a list if issue #116 goes through


Is this an old TODO?

No, I added it here in light of issue #116

tuxxy · 2018-04-23T21:39:58Z

umbral/pre.py

-def _challenge(kfrag: KFrag, capsule: Capsule, 
-              cfrag: CapsuleFrag, challenge_metadata: bytes=None,
-              params: UmbralParameters=None) -> ChallengeResponse:
+def _prove_correctness(cfrag: CapsuleFrag, kfrag: KFrag, capsule: Capsule, 


I think _prove_correctness has to be above reencrypt for it to be used?

Might it also be better as a classmethod on CorrectnessProof?

tuxxy · 2018-04-23T21:43:49Z

umbral/pre.py

+class UmbralCorrectnessError(GenericUmbralError):
+    def __init__(self, message, offending_cfrags):
+        super().__init__(message)
+        self.offending_cfrags = offending_cfrags


Is there a need to capture these here even if we handle them ~L485?

The idea is to include them in the exception, so in the application (i.e., the KMS), Bob could know which cfrags were incorrect and act consequently.

michwill · 2018-04-24T00:54:51Z

tests/test_correctness.py

+        cfrag = pre.reencrypt(kfrag, capsule, metadata=metadata)
+        capsule.attach_cfrag(cfrag)
+
+        assert pre._verify_correctness(capsule, cfrag,


I like the simplicity of the interface!

michwill

Great!

jMyles

This is a clear step forward in terms of the tooling. I think that exactly how Bob can expect to benefit needs to be reflected with updated documentation.

jMyles · 2018-04-23T23:06:14Z

tests/test_correctness.py

+
+from umbral import pre, keys
+from umbral.point import Point
+from umbral.fragments import CorrectnessProof


Looks like you imported this and then didn't use it?

I was using it through pre.CorrectnessProof, which was it's previous location. I forgot to change it. Now it's fixed.

jMyles · 2018-04-24T03:09:11Z

tests/test_keys/test_key_fragments.py

+                 'timestamp' : time.time(), 
+                 'capsule' : bytes(capsule), 
+               }
+    metadata = str(metadata).encode()


In a way, I think b"this is the metadata of a request" is better.

In this case, we're showing casting a dict, with an int for ursula_id, and time.time()? I mean, are we really nudging the user in a good direction here?

I think it's probably better to either structure metadata or leave it as flat, arbitrary bytes.

jMyles · 2018-04-24T05:31:36Z

tests/test_correctness.py

+
+    # Alternatively, we can try to open the capsule directly.
+    # We should get an exception with an attached list of incorrect cfrags
+    with pytest.raises(pre.UmbralCorrectnessError) as exception_info:


Yeah, groovy.

jMyles · 2018-04-24T05:42:47Z

umbral/pre.py

+    try:
+        proof = cfrag.proof
+    except AttributeError:
+        raise ValueError("CFrag doesn't have a correctnes proof attached")


Untested - needs a test I think.

Actually, it's better to remove the try, since it doesn't fail when cfrag.proof is set to None, which will fail anyway a few lines below. So, I wrote a test that checks that the AttributeError is raised when there's no proof but one is expected. It's called test_decryption_fails_when_it_expects_a_proof_and_there_isnt.

jMyles · 2018-04-24T06:27:51Z

umbral/fragments.py


-        if self.metadata is not None:
-            result = result + self.metadata
+        result += self.metadata or b''


This is an odd line - the user is not allowed to use something falsey as metadata?

tuxxy

🐧 Looks good to merge.

cygnusv added 5 commits April 17, 2018 09:59

Merge remote-tracking branch 'nucypher/master'

bdc3a55

Refactored pre.ChallengeResponse into fragments.CorrectnessProof

1a8bb86

Adapted tests to CorrectnessProof

45b4529

pre._challenge() --> pre._prove_correctness

e275cb6

pre._check_challenge() --> pre._verify_correctness_proof()

3610004

cygnusv requested review from jMyles and tuxxy and removed request for jMyles and tuxxy April 17, 2018 10:44

cygnusv added 6 commits April 17, 2018 13:02

More renaming

a03837d

Proofs are now part of CFrags so we can drop it as an argument to _ve…

274699b

…rify_correctness_proof

TODO: Figure out final size for CFrags with proofs

071d4b1

Adapting tests to new setting with CorrectnessProof

fc6f7a1

Include metadata inside the CorrectnessProof

72a9e7b

Adapted tests to metadata inside CorrectnessProofs

2885a1f

TODO: Figure out final size for CorrectnessProofs

cygnusv mentioned this pull request Apr 18, 2018

Define format for re-encryption metadata? #120

Closed

cygnusv added 14 commits April 18, 2018 12:00

Remove metadata as parameter of pre.verify_correctness_proof()

094b8c9

Remove metadata as parameter of verification in tests

d87af0b

Now re-encryption correctness is always verified during decryption

0e064c4

Reformat

88961b3

Enter UmbralCorrectnessError

fca0026

No more Challenge for you

c13da88

I said no more Challenges!

b39b1ca

Check that an UmbralCorrectnessError is raised, and that it includes …

ab748c6

…the incorrect cfrags

Fixed failing test

3f03ce7

Merge remote-tracking branch 'nucypher/master' into cfrags-with-corre…

159b2dd

…ctness-proof

pre._verify_correctness_proof() --> pre._verify_correctness()

b725a5e

Refine correctness tests

bdab54b

Some renaming

b714f47

More informative names in _verify_correctness()

f646fdc

cygnusv added 3 commits April 22, 2018 23:43

Correctness proofs are now optional, although default setting is True

cb32648

Fix problem with serialization of proofs when optional

267e6ea

Tests for multiple possibilities of serialization of cfrags with/with…

036b285

…out proofs and with/without metadata

cygnusv requested review from KPrasch, jMyles, michwill and tuxxy April 22, 2018 22:16

cygnusv changed the title ~~[WIP] CFrags with correctness proof~~ Re-encryption with correctness proofs Apr 22, 2018

cygnusv mentioned this pull request Apr 23, 2018

Renaming internal attributes #125

Closed

cygnusv added 2 commits April 23, 2018 22:58

Merge remote-tracking branch 'nucypher/master' into cfrags-with-corre…

f1a847d

…ctness-proof # Conflicts: # umbral/pre.py

Fix call to hash_to_bn

7d9bc19

tuxxy reviewed Apr 23, 2018

View reviewed changes

cygnusv added 2 commits April 24, 2018 00:55

Make use of short-circuit logic

12c6b5e

Improve readability of valid_kfrag_signature check

4304a7d

michwill reviewed Apr 24, 2018

View reviewed changes

michwill approved these changes Apr 24, 2018

View reviewed changes

jMyles approved these changes Apr 24, 2018

View reviewed changes

jMyles reviewed Apr 24, 2018

View reviewed changes

Added check_proof parameter to pre.decrypt

02ea0aa

tuxxy approved these changes Apr 24, 2018

View reviewed changes

cygnusv added 3 commits April 24, 2018 10:23

Test for decryption fail when there is no proof but it was expected

316bb6b

Fixed import

97f77cf

A simpler example for re-encryption metadata

c7d6644

cygnusv merged commit 01d8e6f into nucypher:master Apr 24, 2018

cygnusv deleted the cfrags-with-correctness-proof branch April 24, 2018 08:36

tuxxy mentioned this pull request Apr 24, 2018

Integrate the challenge protocol as part of the re-encryption request/response #100

Closed

cygnusv mentioned this pull request May 8, 2018

Improved Fragments #140

Merged

Conversation

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

Uh oh!

What this does:

Uh oh!

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

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!

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

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

tuxxy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

cygnusv commented Apr 17, 2018 •

edited

Loading

cygnusv Apr 24, 2018 •

edited

Loading