-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement Simple API to PRE and add UmbralDEM class #29
Conversation
Actually return key
Had a call with David, we discussed how we're implemting the needed API changes. We're going to use Umbral for Michael's API request. This DEM will be kept simple and simply use NaCl Salsa20-Poly1305 SecretBox. Add KEYSIZE attr on UmbralDEM
tests/test_umbral.py
Outdated
|
||
priv_key = keys.UmbralPrivateKey.gen_key(params) | ||
pub_key = priv_key.get_pub_key(params) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps change priv_key
to priv_key_alice
. The same for pub_key
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a leap forward to the target API discussed a couple of months ago. There are of course certain aspects that could be changed (e.g., do we want PRE to be a class or a module?), but I really think we can live with this as-is.
tests/test_umbral.py
Outdated
priv_key_bob, pub_key_bob, pub_key, capsule, enc_data | ||
) | ||
assert reenc_dec_data == plain_data | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, this is completely in line with Michael's API.
umbral/umbral.py
Outdated
|
||
Returns the plaintext of the data. | ||
""" | ||
# TODO: Do we take only the recipient's private key and form pubkey? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can do that, since the public key can be deterministically derived from the private key.
@pytest.mark.parametrize("N,threshold", parameters) | ||
def test_simple_api(N, threshold): | ||
params = umbral.UmbralParameters() | ||
pre = umbral.PRE(params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just getting going on a more thorough review. Overall, this is a huge improvement from 24 hours ago!
I'll make Issues for things that are, at least to me, outstanding. I started on this line, with #31.
tests/test_umbral.py
Outdated
rekeys, _ = pre.split_rekey(priv_key_alice, pub_key_bob, threshold, N) | ||
for rekey in rekeys: | ||
cFrag = pre.reencrypt(rekey, capsule) | ||
capsule.attach_cfrag(cFrag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah interesting. So we attach cFrag to exactly the original capsule, not a brand new one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You attach it, then call capsule.reconstruct
which returns a new capsule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple, but powerful. Awesome first step to make a simple API
pub_key_bob = priv_key_bob.get_pub_key(params) | ||
|
||
plain_data = b'attack at dawn' | ||
enc_data, capsule = pre.encrypt(pub_key_alice, plain_data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, we really don't want to allow the user to optionally use their own symmetric tooling here? I think this is a big decision decision to be made without discussion.
Although, maybe it makes sense to do it this way for now and add the option later if we want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, this can eventually be handled by having a default dem as a class attribute (well, at least on the basic level).
E.g.: dem = UmbralDEM(...)
->
class PRE(object):
...
DEM = UmbralDEM
... dem = PRE.DEM(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably be for the best, currently, if the user can't easily extend their own DEM. Umbral is defined to use Salsa20-Poly1305, but we'll be switching to ChaCha20-Poly1305 to allow for us to add in authenticated data for more security in the overall system.
For that reason, users really shouldn't try to roll their own crypto here. Though, they are free to write their own DEM if they like.
@cygnusv can elaborate on this more. We have an issue open for this as #30.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well yeah, I think this adds +1 to having cls.DEM
, but will wait on @jMyles opinion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer for (in fact, I think that it only makes sense for) PRE to be a module, not a class. So maybe instead we have:
pre.set_dem_interface(MyCustomDEMClass)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We really need the KMS to have one standard DEM method. There isn't any good reason to have to make anything more complex when we have figured the one great way to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tuxxy: Oh yeah, I think we all agree there. I think this concern is more about making sure we have flexibility to build things other than the KMS.
The way you've written this, I think we can probably just add an additional interface later without too much trouble. So I think we can actually just move with this as-is.
tests/test_umbral.py
Outdated
@@ -84,7 +113,7 @@ def test_cfrag_serialization(): | |||
priv_key = pre.gen_priv() | |||
pub_key = pre.priv2pub(priv_key) | |||
|
|||
_, capsule = pre.encapsulate(pub_key) | |||
_, capsule = pre._encapsulate(pub_key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like _
as a name in tests - do you think it makes sense to just call this _key_bytes
?
tests/test_umbral.py
Outdated
@@ -84,7 +113,7 @@ def test_cfrag_serialization(): | |||
priv_key = pre.gen_priv() | |||
pub_key = pre.priv2pub(priv_key) | |||
|
|||
_, capsule = pre.encapsulate(pub_key) | |||
_, capsule = pre._encapsulate(pub_key) | |||
kfrags, _ = pre.split_rekey(priv_key, pub_key, 1, 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here - maybe call this _verification_keys
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huge step in a great direction.
Add more unused variables
What this does:
encrypt
-- Encrypts data using KEM/DEM.decrypt
-- Decrypts data using KEM/DEM.decrypt_reencrypted
-- Decrypts threshold encrypted data using KEM/DEM.reencrypt
-- Already in thePRE
class.split_rekey
-- Already in thePRE
class. NOTE: ReturnsvKeys
, disregard this for the time being via_
operator.reconstruct
-- Already in theCapsule
class. It is required to use the above methods; therefore is exposed in a similar fashion already.split_rekey
method to allow forUmbralPrivateKey
andUmbralPublicKey
use.gen_key
method toUmbralPrivateKey
.get_pub_key
toUmbralPrivateKey
to calculate public key for the private key.PRE
:_encapsulate
_decapsulate_reencrypted
UmbralDEM
:UmbralParameters
.@cygnusv and I had a call around 02:00MST/01:00PST and discussed the needs for the
UmbralDEM
class as well as the simple API implemented here.The simple API is implemented in the core Umbral class
PRE
. Subpaths need to be handled on the KMS side. This does not implement or use any challenge methods.NOTE: This merges to the
kms-depend
branch. After merging this branch, continue to merge to themaster
branch.Good night.