Skip to content
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

Revamping Ferveo Ciphertexts (breaking changes ⚠️ ) #149

Merged
merged 7 commits into from
Aug 22, 2023

Conversation

cygnusv
Copy link
Member

@cygnusv cygnusv commented Jul 28, 2023

Type of PR:

  • Bugfix
  • Feature
  • Documentation
  • Refactor
  • Other

Required reviews:

  • 1
  • 2
  • 3

What this does:
Some of the changes introduced by this PR, all of them related with how ciphertexts are produced and used:

Apart from this, bumps MSRV to 1.67.0, and associated clippy changes.

Why it's needed:
@jMyles and @KPrasch identified an important limitation of current ciphertexts and TDec requests construction. Essentially, the TDec request included the full ciphertext, which implies a big overhead even for relatively small media files (e.g. short audio clips). The main goal of this PR was to decouple the bulk of symmetric ciphertext from the validation procedure, using a hash instead. This enables very small TDec requests ( see #147). In the nucypher side, will allow to close nucypher/nucypher#3176 via nucypher/nucypher#3194

Notes for reviewers:

What should reviewers focus on?
Is there a particular commit/function/section of your PR that requires more attention from reviewers?

@cygnusv cygnusv force-pushed the thin branch 2 times, most recently from 7a90b1a to 1c814b8 Compare July 31, 2023 09:10
@codecov-commenter
Copy link

codecov-commenter commented Jul 31, 2023

Codecov Report

Merging #149 (4337c3c) into main (03b4e35) will increase coverage by 0.43%.
The diff coverage is 86.44%.

@@            Coverage Diff             @@
##             main     #149      +/-   ##
==========================================
+ Coverage   77.96%   78.40%   +0.43%     
==========================================
  Files          23       23              
  Lines        4979     4969      -10     
==========================================
+ Hits         3882     3896      +14     
+ Misses       1097     1073      -24     
Files Changed Coverage Δ
ferveo-common/src/lib.rs 0.00% <0.00%> (ø)
ferveo/src/bindings_python.rs 52.22% <0.00%> (+0.23%) ⬆️
ferveo/src/bindings_wasm.rs 0.00% <0.00%> (ø)
tpke/src/ciphertext.rs 100.00% <100.00%> (+10.58%) ⬆️
tpke/src/context.rs 98.03% <100.00%> (-0.15%) ⬇️
tpke/src/decryption.rs 69.92% <100.00%> (ø)

@cygnusv cygnusv changed the title [WIP] Thin Ciphertexts [WIP] Revamping Ferveo Ciphertexts (breaking changes ⚠️ ) Jul 31, 2023
@piotr-roslaniec
Copy link

piotr-roslaniec commented Jul 31, 2023

The error on CI is unrelated to this PR. In order to fix it, bump MSRV to 1.67.
Edit: Removed previous workaround.

Copy link

@piotr-roslaniec piotr-roslaniec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

(I realize this is WIP, please feel free to re-request a review anytime)

* Remove unused Ciphertext.construct_tag_hash()
* Refactor Ciphertext.check() to take the functionality of check_ciphertext_validity() function
@cygnusv cygnusv marked this pull request as ready for review August 1, 2023 10:04
@cygnusv cygnusv changed the title [WIP] Revamping Ferveo Ciphertexts (breaking changes ⚠️ ) Revamping Ferveo Ciphertexts (breaking changes ⚠️ ) Aug 1, 2023
@derekpierre
Copy link
Member

@cygnusv, so Ciphertext comprises of:

    #[serde_as(as = "serialization::SerdeAs")]
    pub commitment: E::G1Affine,
    // W
    #[serde_as(as = "serialization::SerdeAs")]
    pub auth_tag: E::G2Affine,
    // V
    #[serde(with = "serde_bytes")]
    pub ciphertext: Vec<u8>,

So from a dem_ciphertext and kem_ciphertext perspective, what is the kem_ciphertext here? i.e. what's the ciphertext data I would put in the ThresholdDecryptionRequest to send to Ursulas?

@cygnusv
Copy link
Member Author

cygnusv commented Aug 3, 2023

@cygnusv, so Ciphertext comprises of:

    #[serde_as(as = "serialization::SerdeAs")]
    pub commitment: E::G1Affine,
    // W
    #[serde_as(as = "serialization::SerdeAs")]
    pub auth_tag: E::G2Affine,
    // V
    #[serde(with = "serde_bytes")]
    pub ciphertext: Vec<u8>,

So from a dem_ciphertext and kem_ciphertext perspective, what is the kem_ciphertext here? i.e. what's the ciphertext data I would put in the ThresholdDecryptionRequest to send to Ursulas?

Yeah, good question. For the request you only need the commitment, the auth_tag, and the hash of the symmetric ciphertext.

BTW, I'm not sure the term Kem/Dem is technically correct once we introduce the hash of the Dem, but I don't have a better suggestion for the moment.

@derekpierre
Copy link
Member

Yeah, good question. For the request you only need the commitment, the auth_tag, and the hash of the symmetric ciphertext.

BTW, I'm not sure the term Kem/Dem is technically correct once we introduce the hash of the Dem, but I don't have a better suggestion for the moment.

@cygnusv, 👍 thanks for the clarification. Could we do a quick chat about this whenever you are around?

Copy link
Member

@derekpierre derekpierre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎸

@derekpierre
Copy link
Member

@cygnusv, 👍 thanks for the clarification. Could we do a quick chat about this whenever you are around?

Chatted with @cygnusv , and there will be a follow-up PR to hide some of the Ferveo-specific math details behind higher-level objects.

@cygnusv
Copy link
Member Author

cygnusv commented Aug 11, 2023

@cygnusv, 👍 thanks for the clarification. Could we do a quick chat about this whenever you are around?

Chatted with @cygnusv , and there will be a follow-up PR to hide some of the Ferveo-specific math details behind higher-level objects.

Added new issue #154 to track this

@jMyles
Copy link

jMyles commented Aug 13, 2023

@jMyles and @KPrasch identified an important limitation of current ciphertexts and TDec requests construction.

Credit to Fiddle Kuba ( @kubic71 ) as well.

@KPrasch KPrasch merged commit f44e1be into nucypher:main Aug 22, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Completed
6 participants