[NO-MERGE] Unified Capsule instead of ReconstructedCapsule #24
Conversation
…cted; introducing concept of "contents."
…aybe kick this out to a separate method?
umbral/umbral.py
Outdated
|
|
||
| return key | ||
| d = hash_to_bn([xcomp, pub_key, xcomp * priv_key], self.params) | ||
| shared_key = capsule.reconstructed_ephemeral_points() * d |
There was a problem hiding this comment.
Don't do this, this is not clear at all with what's happening. It doesn't look like DH anymore.
| h = hash_to_bn([e, v], self.params) | ||
| inv_d = ~d | ||
| assert orig_pub_key * (s * inv_d) == capsule._point_eph_v_prime + ( | ||
| capsule._point_eph_e_prime * h), "Generic Umbral Error" |
There was a problem hiding this comment.
This is really hard to read, can you leave in the e_prime and v_prime vars?
umbral/umbral.py
Outdated
| def decapsulate_original(self, priv_key, capsule, key_length=32): | ||
| """Derive the same symmetric key""" | ||
|
|
||
| shared_key = (capsule.point_eph_e + capsule.point_eph_v) * priv_key | ||
| shared_key = capsule.original_ephemeral_points() * priv_key |
There was a problem hiding this comment.
Doesn't look like it's doing DH anymore. :/
umbral/umbral.py
Outdated
|
|
||
| return ReconstructedCapsule(e_prime, v_prime, eph_ni) | ||
|
|
||
| def to_bytes(self): |
There was a problem hiding this comment.
Why are you getting rid of to_bytes? Keep this for uniformity, it's how cryptographic APIs handle their bytestring representations.
umbral/umbral.py
Outdated
|
|
||
| @staticmethod | ||
| def from_bytes(data: bytes, curve: ec.EllipticCurve): | ||
| def _reconstructed_bytes(self): |
There was a problem hiding this comment.
Please keep it as from_bytes, we're going to also want this "public".
There was a problem hiding this comment.
I don't understand what you mean here. This isn't a constructor method like from_bytes. Maybe you misread?
There was a problem hiding this comment.
Ah, you're right. Probably will also want to do how you're doing from_bytes above with the boolean flag, except do it as to_bytes here.
umbral/umbral.py
Outdated
| return self._point_eph_e + self._point_eph_v | ||
|
|
||
| def reconstructed_ephemeral_points(self): | ||
| return self._point_eph_e_prime + self._point_eph_v_prime |
There was a problem hiding this comment.
Please rather call these vars individually for more readability. This also does point addition, did you notice that? Whenever that happens, it should happen explicitly and not hidden in a method call.
There was a problem hiding this comment.
...why?
I don't think that three fairly obscure var names are more (or even as) readable as a single method name.
There was a problem hiding this comment.
Because you're hiding point addition -- this does a few things that we don't want.
- Since this is a reference implementation, anywhere in the code should be really explicit when we're performing a curve operation.
- This is a dangerous operation and shouldn't be exposed as a simple method that the user can confuse for something else. This method is not clear that it performs addition, even with the operator overloading.
There was a problem hiding this comment.
Another issue here are the names, I just realized:
original_ephemeral_points -> It's not returning plural points, after the addition it returns one and they're not the original ones either. Same for reconstructed_ephemeral_points.
There was a problem hiding this comment.
OK, that's a good point.
I still think though that returning them from a single uniform method is much safer.
For the smaller userbase that is willing to assume the liability, the variables are available as private attributes. But the better approach is to provide a method that does the addition, the correct way, with the correct two values, and to make that method the public default.
So: how about if these two methods instead return a tuple of the points?
This also has the benefit of forcing the candor in the implementation that you're after, ie:
sum(capsule.original_ephemeral_points) * priv_key
There was a problem hiding this comment.
No, that's just not clear to me. Can we just leave it as it was? It makes it so much more clear.
There was a problem hiding this comment.
I think I'm interested in other perspectives here. @cygnusv, @michwill, @KPrasch?
What's the advantage of forcing the user to try to do it properly instead of giving them the method that does (but also providing the primitives)?
Consider:
shared_key = (capsule.point_eph_e + capsule.point_eph_v) * priv_key
Have I used the correct two points?
- Did I use the original if that's what I wanted?
- Did I use the reconstructed if that's what I wanted?
- Did I use both from the same set? I didn't intermix, did I?
- Am I sure I didn't let the
sigornoninteractive_pointslip in there? Definielyeandv, right?
...especially since there's no great way to repr Points from each other in such as way that makes clear their role.
Now, did I add them in the correct order?
Now consider:
shared_key = sum(capsule.original_ephemeral_points) * priv_key
There's no way to mess that up. I know that I have the original points, I know that I have the correct two values, that they're in the right order, that they're added properly. Nothing is left to chance.
What is bad about this? Are you suggesting that other libraries refuse to do this on principle? If so, I want to see the conversation that led to such a design decision - do you have a link?
There was a problem hiding this comment.
This isn't about making it easy for the user. It's about making it so that readers can read the paper, look at our code, and understand what's happening.
We don't need methods that hide operational logic in them. If someone is using our library to perform arithmetic on curves, they're using it wrong.
There was a problem hiding this comment.
OK, we decided not to do this. From Slack:
jmyles 5:51 PM
Getting back original_ephemeral_points for a minute:
5:51
Are you saying that this is never a public notion at all?
tux 5:51 PM
I don’t expect anyone to modify the underlying cryptography of this library. If they do, they probably shouldn’t.
jmyles 5:51 PM
Well, that's not what I asked. 🙂
dnunez 5:52 PM
they are in the test because we need to check the serialization
tux 5:52 PM
Right
dnunez 5:52 PM
but a user would never need to inspect those elements
jmyles 5:52 PM
I see
5:52
ok
dnunez 5:52 PM
for a user a capsule is atomic
jmyles 5:52 PM
So they are used by their respective decapsulate functions and never publicly?
5:52
Ahh
5:52
OK
dnunez 5:52 PM
yes
jmyles 5:52 PM
OK yeah, I withdraw then - I misunderstood.
dnunez 5:53 PM
we only use it in umbral, and in the tests of course
5:53
but there shouldn't be used outside that scope
jmyles 5:53 PM
I see.
5:53
Yeah, that completely makes sense now, reading again
dnunez 5:53 PM
in any case, I don't see why is this related with that method that returns the additon of e and v
jmyles 5:54 PM
Well, my drive was to help the user avoid doing that improperly.
5:54
(ie, with the original 'e' and 'v' when they wanted the reconstructed, etc)
5:55
See, this is part of the problem with writing the first time with too few _private things 🙂
dnunez 5:55 PM
ok, that possibly was a problem, yes
tux 5:56 PM
I thought we all knew that all those fragment pieces were parts of the ciphertext?
5:56
intrinsically private sorta 😅
dnunez 5:57 PM
it was clear to me, at least, but of course I designed it
5:57
and I have to admit that I'm new to python
tux 5:57 PM
Meh, don’t fret about Python privacy, it’s not real privacy.
dnunez 5:57 PM
the _private notation seems weird coming from Java
jmyles 5:57 PM
Yeah, I mean reading again, with the benefit of you're commentary, it makes sense
tux 5:58 PM
It just doesn’t import certain variables prefixed with _
dnunez 5:58 PM
since it's not real
jmyles 5:58 PM
But do you see what I was seeing?
dnunez 5:58 PM
not really
5:58
but I understand that there could have been misunderstandings
jmyles 5:58 PM
I saw shared_key = (capsule.point_eph_e + capsule.point_eph_v) * priv_key in a public method and thought, "well, if the user wanted to..."
michwill 5:58 PM
Well, you can have two underscores for real 🙂
tux 5:59 PM
throws papers
jmyles 5:59 PM
hahahaha
5:59
But seriously: the decapsulate methods are good candidates for __
5:59
for this reason
dnunez 5:59 PM
I don't see what's the potential problem with the user anyway
tux 6:00 PM
^ Yeah, this is really my main question
6:00
I don’t expect users to go muddling around in the code here.
dnunez 6:00 PM
how could the user screw things there?
tux 6:00 PM
It’s not like they’re gonna go, “Oh, well I need to add to the ciphertext!”
jmyles 6:00 PM
Well, what if they wrote a Bob who decapsulates the capsule in his super special way, but used the original e and the reconstrcuted v?
tux 6:00 PM
they should not do that
dnunez 6:00 PM
basically, it would fail
jmyles 6:01 PM
@Tux: if you feel so strongly, then let's have code saying so 🙂
6:01
@Tux: I agree they should not do that - that's why I wrote a method that does it the right way for them
6:01
(not yet realizing that they don't need to do it anyway)
dnunez 6:01 PM
but jmyles
6:01
your argument applies to every line of the code
jmyles 6:01 PM
No, only the public interfaces
dnunez 6:02 PM
I dont understand that
jmyles 6:02 PM
If Capsule.open calls __decapsulate_reconstructed, then it's absolutely obvious that the user isn't allowed to much with the latter without taking complete responsibility for their idiocy
6:02
🙂
dnunez 6:02 PM
ok...
jmyles 6:02 PM
I was trying to remove ReconstructedCapsule in a way that still forced the user to only be able to do the right thing.
6:03
And, at the time, I thought that I was seeing the user calling decapsulate directly, not using the public interfaces on Capsule (because yes, I spaced for a second and wasn't thinking of a capsule as an atomic structure)
| return cls(eph_e, eph_v, sig) | ||
|
|
||
| @classmethod | ||
| def from_reconstructed_bytes(cls, data: bytes, curve: ec.EllipticCurve): |
There was a problem hiding this comment.
Please rename to from_bytes for uniformity.
There was a problem hiding this comment.
Well, which one is from_bytes? This or from_original_bytes?
There was a problem hiding this comment.
I would remove both and leave in from_bytes, and offer a flag for is_reconstructed=False. When True, reconstruct the capsule.
There was a problem hiding this comment.
Ahh yes that sounds good.
umbral/umbral.py
Outdated
| # TODO: Raise an error here if Bob has gathered enough cFrags. | ||
| if self.contents and not force_reopen: | ||
| newly_opened = True | ||
| else: |
There was a problem hiding this comment.
This newly_opened and force_reopen doesn't really do much for me. It just adds some weird logic in an otherwise plain reference implementation. I don't understand what problem it's solving.
There was a problem hiding this comment.
Yeah I mostly agree. I just didn't want to leave the user without this functionality without at least sketching and discussing it first.
There was a problem hiding this comment.
I think this should probably stay out of this PR until David and I discuss how we're going to handle the symmetric operations when something like this might be possible.
| else: | ||
| self._reconstruct(pre=pre) | ||
| if not pre: | ||
| pre = PRE(UmbralParameters()) |
There was a problem hiding this comment.
UmbralParameters() should not be called here. There is only ONE set of parameters for the whole system, and UmbralParameters creates these parameters, where some of them are randomized. Therefore if we create new parameters in several places, things will start not to work. Perhaps I should have used some kind of singleton pattern for UmbralParameters, or something similar.
There was a problem hiding this comment.
@cygnusv absolutely - I was going to make an issue for this next.
Let's turn the PRE class into a module, and instantiate the one and only UmbralParamteres there.
Consider this a workaround until we do that.
jMyles
force-pushed
the
unifiedCapsule
branch
from
aaa706e
to
681baa6
Compare
jMyles
changed the title
jMyles
changed the title
What this does: