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

Remove Info from plaintext Hybrid Reports #1487

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

tyurek
Copy link
Collaborator

@tyurek tyurek commented Dec 10, 2024

  • Remove HybridInfo from being included in plaintext HybridReports by default.
  • HybridInfo is now a required argument for encryption
  • Because we want the info in some contexts, adds a function to optionally return both the report and the info when decrypting
  • Adds a create_hybrid_info() function to TestHybridRecord so that we can do the initial encryption
  • Adjusts encryption/decryption scripts to handle refactor. Output format is unchanged.

@tyurek tyurek changed the title [WIP] Remove Info from plaintext Hybrid Reports Remove Info from plaintext Hybrid Reports Dec 11, 2024
@tyurek tyurek marked this pull request as ready for review December 11, 2024 00:49
Copy link

codecov bot commented Dec 11, 2024

Codecov Report

Attention: Patch coverage is 95.69892% with 8 lines in your changes missing coverage. Please review.

Project coverage is 93.31%. Comparing base (bb543f9) to head (2df795f).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
ipa-core/src/report/hybrid.rs 91.01% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1487      +/-   ##
==========================================
+ Coverage   93.28%   93.31%   +0.03%     
==========================================
  Files         239      237       -2     
  Lines       43532    43599      +67     
==========================================
+ Hits        40608    40686      +78     
+ Misses       2924     2913      -11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

self.encryptor_pool.encrypt_share((report_id, i, share))?;
// todo: maybe a smart pointer to avoid cloning
self.encryptor_pool
.encrypt_share((report_id, i, share, info.clone()))?;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

probably some room for improvement here. What happens here is that we have three different reports (generated by share()), which all are encrypted with the same HybridInfo. I changed the encrypt_share function to take an info argument, but it really only needs a reference (I tried having it take a reference but ran into issues with lifetimes). Is it worth trying to use a smart pointer here? Or maybe rewrite encrypt_share to take three at a time but only one Info?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yea it feels like using references here is the right thing to do. But lifetimes can be tricky to handle. I can take a look at it later today

let out_len = std::mem::size_of_val(&self.key_id);
debug_assert_eq!(out_len, self.to_bytes().len(), "Serialization length estimation is incorrect and leads to extra allocation or wasted memory");
out_len
out_len.try_into().unwrap()
Copy link
Collaborator

Choose a reason for hiding this comment

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

One benefit of using usize here is that the caller cannot ignore the fact that report can be more than 65k. I thought we only need to do this cast once, when we encode the report length, so this module may not need to concern itself with such limitations

@tyurek
Copy link
Collaborator Author

tyurek commented Jan 2, 2025

Looking to wrap this one up. I put out an example commit here: 13fafea which modifies the encryptor to encrypt three shares per thread and use the same aad info for each. I could do either this or use a smart pointer, but doing it with lifetimes seems complicated.

@tyurek tyurek requested a review from akoshelev January 2, 2025 02:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants