Remove Info from plaintext Hybrid Reports#1487
Remove Info from plaintext Hybrid Reports#1487tyurek wants to merge 5 commits intoprivate-attribution:mainfrom
Conversation
tyurek
changed the title
Codecov ReportAttention: Patch coverage is
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. |
| 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()))?; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
ipa-core/src/report/hybrid_info.rs
Outdated
| 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() |
There was a problem hiding this comment.
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
|
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. |
| match_key: match_key_share, | ||
| breakdown_key: breakdown_key_share, | ||
| info: HybridImpressionInfo::new(key_id), | ||
| //info: HybridImpressionInfo::new(key_id), |
There was a problem hiding this comment.
| //info: HybridImpressionInfo::new(key_id), |
| let mut out = Vec::with_capacity(usize::from(self.encrypted_len())); | ||
| self.encrypt_to(key_id, key_registry, rng, &mut out)?; | ||
| debug_assert_eq!(out.len(), usize::from(self.encrypted_len())); | ||
| let mut out = Vec::with_capacity(usize::from(self.ciphertext_len() + u16::try_from(info.byte_len()).unwrap())); |
There was a problem hiding this comment.
there seems to be a fair amount of similarities between this code and the code above. Any way to reduce the copy-paste?
2 participants
create_hybrid_info()function to TestHybridRecord so that we can do the initial encryption