search

nucypher / pyUmbral

NuCypher's reference implementation of Umbral (threshold proxy re-encryption) using OpenSSL and Cryptography.io
https://pyumbral.readthedocs.io
GNU General Public License v3.0
284 stars 71 forks source link

Stateless API #258

Closed fjarri closed 3 years ago

fjarri commented 4 years ago

Plan:

To finalize:

tuxxy commented 4 years ago

@fjarri This is a great start! Some thoughts on the present draft:

The thing that sticks out to me the most is the lack of a unified API for decryption. It's nice being able to keep a single encrypt and decrypt method for usability and API simplicity.

fjarri commented 4 years ago

Yes, I wasn't certain about this change. The rationale was the following:

Originally, decrypt() took a Capsule object, and depending on its contents (whether it had cfrags or not), the decryption method was chosen.

Now, we basically have one function that decrypts a capsule, and another that decrypts a list of cfrags (and the capsule is only used for verification). Besides having different parameters, they are used in different situations, so the user never has to write something along the lines of

if condition:
    decrypt_original()
else:
    decrypt_reencrypted()

It's always either one or the other. So I thought that giving them different names instead of having dynamic dispatch inside decrypt() (and a more complicated type signature with Union and Optional) would make the API more logical.

Since decrypt_reencrypted() is being used much more often, it could inherit the name decrypt(), but that's details.

fjarri commented 4 years ago

This PR feels clever to me.

Somehow I feel it's not a compliment :)

It masks the mutation of Capsule by instantiating a separate manifestation of the same knowledge, PreparedCapsule.

No, I would say that it's the same knowledge plus the information to verify it. The capsule does not change. I think there's too much verification going on in the code as it is, and making it explicit will help eliminate some of excessive checks.

It then masks the state of a PreparedCapsule by directing the user to two different methods for decryption, which repeat some logic.

See the comment about the logic above. The "repeating logic" is just type checks. The actual logic is different (_open_capsule vs _decapsulate_original), as is the set of parameters.

A Capsule is a stateful concept; rewriting the code to mask that creates, rather than removes, a layer of indirection.

I would disagree here. A capsule is two curve points and one scalar. Nothing changes inside it. It can be used with or without cfrags (involving different algorithms), with or without verification keys.

This seems to be a reopening of the essence of #38 by another name. I think #38 was properly merged, and that this PR is properly closed wontfix.

I would disagree with that as well. PR #38 was essentially rolled back/fixed later, because now the reconstruct() logic is contained in _decapsulate_reencrypted(), and, as it turns out, no additional ReconstructedCapsule class is necessary — its data is ephemeral and doesn't need to be saved.

jMyles commented 3 years ago

No, I would say that it's the same knowledge plus the information to verify it

Well yeah. The question is how to semantically represent this in a way that tells a true story about this knowledge plus the information to verify it.

I would disagree here. A capsule is two curve points and one scalar. Nothing changes inside it. It can be used with or without cfrags (involving different algorithms), with or without verification keys.

The metaphor I imagine is that, indeed, nothing changes inside it, but that it has slots for Bob to plug in CFrags to the outside of it, and it eventually glows with activation, in when enough have been plugged. The plugging-in of the CFrags, the activation, and eventually the accessibility of the information contained therein (whether we picture it as Bob grasping it, or the "chimney" notion of plaintext being emitted as an industrial product) are all part of the lifecycle of this object - all mutations.

I would disagree with that as well. PR #38 was essentially rolled back/fixed later, because now the reconstruct() logic is contained in _decapsulate_reencrypted(), and, as it turns out, no additional ReconstructedCapsule class is necessary — its data is ephemeral and doesn't need to be saved.

Be that as it may, it still divides the Capsule into two, as if it disappears in the middle of Bob's custody and a new one appearing in its place. And it splits PRE.decrypt in almost the same fashion as it was split prior to #38

This PR seems to cost richness in the metaphor. I find it more difficult to reason about, and more difficult to remember the lifecycle of the objects in question. It makes it perhaps easier for a computer to understand, but harder for a human to. Presently, I can picture everything about what Bob does and why; it aligns logically with the primitives in use at each stage.

fjarri commented 3 years ago

The metaphor I imagine is that, indeed, nothing changes inside it, but that it has slots for Bob to plug in CFrags to the outside of it, and it eventually glows with activation, in when enough have been plugged. The plugging-in of the CFrags, the activation, and eventually the accessibility of the information contained therein (whether we picture it as Bob grasping it, or the "chimney" notion of plaintext being emitted as an industrial product) are all part of the lifecycle of this object - all mutations.

This PR seems to cost richness in the metaphor.

Yes, that's exactly where we disagree. My position is that our code has way too many metaphors. There's no need to call 1 + 2 + 3 "a journey of a plus operator, and a sum becoming alive", or write it as s = Sum(); s.add(1); s.add(2); s.add(3); s.calculate(). Sometimes a banana is just a banana. It was not my initial intention to remove any metaphors, but, as a matter of fact, it is a welcome side effect.

Be that as it may, it still divides the Capsule into two, as if it disappears in the middle of Bob's custody and a new one appearing in its place.

Nope, the capsule is immutable and therefore exactly the same (and thus doesn't need to "reappear" anywhere - you can just reuse it if you wish). It's in the mutable case where you have a new capsule appearing, but you can't tell that it's a new capsule because it's the same object, yet with different contents.

I find it more difficult to reason about, and more difficult to remember the lifecycle of the objects in question.

If the objects are immutable, they have no lifecycle, and you don't need to think about it. It makes things simpler.

Anyway, I understand that you're a firm -1 on immutability, and I'm out of arguments. Anyone else wants to chime in?

cygnusv commented 3 years ago

@fjarri let's try to merge the stateless API PR as is, up to the new changes to accommodate umbral v2, and include these latter changes in a new PR.

fjarri commented 3 years ago

@cygnusv Done, created #263

Not sure what the CI fails are about. Tests run fine locally.

KPrasch commented 3 years ago

The failing CI tests are showing python 3.5 compatibility, and benchmarking in python 3.6. Perhaps you do not reproduce these failures locally because you are using a different version of python, and not executing the benchmarks?

fjarri commented 3 years ago

The failing CI in 3.5 is actually because of BytestringSplitter which uses f-strings, so that should perhaps be updated.

The other one is pysha3 failing to build, and also it's kind of strange with the versions:

Creating a virtualenv for this project…
Pipfile: /home/circleci/pyUmbral-36/Pipfile
Using /usr/bin/python3 (3.7.3) to create virtualenv…
created virtual environment CPython3.7.3.final.0-64 in 917ms

So it's really 3.7 and not 3.6...

fjarri commented 3 years ago

I'm actually not sure this PR is even that important now, seeing as how #263 will be a big refactor even compared to it, so it might as well be a refactor compared to the main branch.

fjarri commented 3 years ago

Deprecated in favor of #263. RIP.