From 794c9ae119a93d5f83fa8f4f40c1db6b4c6a7d2b Mon Sep 17 00:00:00 2001 From: Richa Jain Date: Tue, 10 Sep 2024 21:06:52 +0800 Subject: [PATCH 1/6] In case decryption fails, do not error out --- ipa-core/src/cli/crypto.rs | 113 +++++++++++++++++++------------------ 1 file changed, 59 insertions(+), 54 deletions(-) diff --git a/ipa-core/src/cli/crypto.rs b/ipa-core/src/cli/crypto.rs index 69acd1e11..2f77238d0 100644 --- a/ipa-core/src/cli/crypto.rs +++ b/ipa-core/src/cli/crypto.rs @@ -18,7 +18,7 @@ use crate::{ U128Conversions, }, hpke::{KeyRegistry, PrivateKeyOnly}, - report::{EncryptedOprfReport, EventType, OprfReport, DEFAULT_KEY_ID}, + report::{EncryptedOprfReport, EventType, InvalidReportError, OprfReport, DEFAULT_KEY_ID}, secret_sharing::IntoShares, test_fixture::{ipa::TestRawDataRecord, Reconstruct}, }; @@ -146,7 +146,7 @@ impl DecryptedReports { } impl Iterator for DecryptedReports { - type Item = OprfReport; + type Item = Result, InvalidReportError>; fn next(&mut self) -> Option { let mut line = String::new(); @@ -154,8 +154,7 @@ impl Iterator for DecryptedReports { let encrypted_report_bytes = hex::decode(line.trim()).unwrap(); let enc_report = EncryptedOprfReport::from_bytes(encrypted_report_bytes.as_slice()).unwrap(); - let dec_report: OprfReport = - enc_report.decrypt(&self.key_registry).unwrap(); + let dec_report = enc_report.decrypt(&self.key_registry); Some(dec_report) } else { None @@ -182,57 +181,63 @@ pub async fn decrypt_and_reconstruct(args: DecryptArgs) -> Result<(), BoxError> .open(args.output_file)?, ); - for (dec_report1, (dec_report2, dec_report3)) in - decrypted_reports1.zip(decrypted_reports2.zip(decrypted_reports3)) + for (idx, (dec_report1, (dec_report2, dec_report3))) in decrypted_reports1 + .zip(decrypted_reports2.zip(decrypted_reports3)) + .enumerate() { - let timestamp = [ - dec_report1.timestamp, - dec_report2.timestamp, - dec_report3.timestamp, - ] - .reconstruct() - .as_u128(); - - let match_key = [ - dec_report1.match_key, - dec_report2.match_key, - dec_report3.match_key, - ] - .reconstruct() - .as_u128(); - - // these aren't reconstucted, so we explictly make sure - // they are consistent across all three files, then set - // it to the first one (without loss of generality) - assert_eq!(dec_report1.event_type, dec_report2.event_type); - assert_eq!(dec_report2.event_type, dec_report3.event_type); - let is_trigger_report = dec_report1.event_type == EventType::Trigger; - - let breakdown_key = [ - dec_report1.breakdown_key, - dec_report2.breakdown_key, - dec_report3.breakdown_key, - ] - .reconstruct() - .as_u128(); - - let trigger_value = [ - dec_report1.trigger_value, - dec_report2.trigger_value, - dec_report3.trigger_value, - ] - .reconstruct() - .as_u128(); - - writeln!( - writer, - "{},{},{},{},{}", - timestamp, - match_key, - u8::from(is_trigger_report), - breakdown_key, - trigger_value, - )?; + match (dec_report1, dec_report2, dec_report3) { + (Ok(dec_report1), Ok(dec_report2), Ok(dec_report3)) => { + let timestamp = [ + dec_report1.timestamp, + dec_report2.timestamp, + dec_report3.timestamp, + ] + .reconstruct() + .as_u128(); + + let match_key = [ + dec_report1.match_key, + dec_report2.match_key, + dec_report3.match_key, + ] + .reconstruct() + .as_u128(); + + // these aren't reconstucted, so we explictly make sure + // they are consistent across all three files, then set + // it to the first one (without loss of generality) + assert_eq!(dec_report1.event_type, dec_report2.event_type); + assert_eq!(dec_report2.event_type, dec_report3.event_type); + let is_trigger_report = dec_report1.event_type == EventType::Trigger; + + let breakdown_key = [ + dec_report1.breakdown_key, + dec_report2.breakdown_key, + dec_report3.breakdown_key, + ] + .reconstruct() + .as_u128(); + + let trigger_value = [ + dec_report1.trigger_value, + dec_report2.trigger_value, + dec_report3.trigger_value, + ] + .reconstruct() + .as_u128(); + + writeln!( + writer, + "{},{},{},{},{}", + timestamp, + match_key, + u8::from(is_trigger_report), + breakdown_key, + trigger_value, + )?; + } + _ => println!("Decryption failed for record no {idx}"), + } } Ok(()) From 9baca445710c0cdd01471e03508b806e046a5d71 Mon Sep 17 00:00:00 2001 From: Richa Jain Date: Fri, 13 Sep 2024 13:11:43 +0800 Subject: [PATCH 2/6] In case of an error it, log the error in the file and continue to process other rows --- ipa-core/src/cli/crypto.rs | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/ipa-core/src/cli/crypto.rs b/ipa-core/src/cli/crypto.rs index 2f77238d0..a195fef89 100644 --- a/ipa-core/src/cli/crypto.rs +++ b/ipa-core/src/cli/crypto.rs @@ -180,7 +180,7 @@ pub async fn decrypt_and_reconstruct(args: DecryptArgs) -> Result<(), BoxError> .create_new(true) .open(args.output_file)?, ); - + let mut first_error = None; for (idx, (dec_report1, (dec_report2, dec_report3))) in decrypted_reports1 .zip(decrypted_reports2.zip(decrypted_reports3)) .enumerate() @@ -236,11 +236,33 @@ pub async fn decrypt_and_reconstruct(args: DecryptArgs) -> Result<(), BoxError> trigger_value, )?; } - _ => println!("Decryption failed for record no {idx}"), + (Err(e1), _, _) => { + writeln!(writer, "Decryption failed Record: {idx} Reason:{e1}",)?; + eprintln!("Decryption failed Record: {idx} Reason:{e1}"); + if first_error.is_none() { + first_error = Some(e1); + } + } + (Ok(_), Err(e2), _) => { + writeln!(writer, "Decryption failed Record: {idx} Reason:{e2}",)?; + eprintln!("Decryption failed Record: {idx} Reason:{e2}"); + if first_error.is_none() { + first_error = Some(e2); + } + } + (Ok(_), Ok(_), Err(e3)) => { + writeln!(writer, "Decryption failed Record: {idx} Reason:{e3}",)?; + eprintln!("Decryption failed Record: {idx} Reason:{e3}"); + if first_error.is_none() { + first_error = Some(e3); + } + } } } - - Ok(()) + match first_error { + None => Ok(()), + Some(err) => Err(Box::new(err)), + } } #[cfg(test)] From bd65cb53b558fa01ec2323163d14387578b53bd2 Mon Sep 17 00:00:00 2001 From: Richa Jain Date: Fri, 13 Sep 2024 19:06:11 +0800 Subject: [PATCH 3/6] Fixing test --- ipa-core/src/cli/crypto.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/ipa-core/src/cli/crypto.rs b/ipa-core/src/cli/crypto.rs index a195fef89..84871776a 100644 --- a/ipa-core/src/cli/crypto.rs +++ b/ipa-core/src/cli/crypto.rs @@ -180,7 +180,7 @@ pub async fn decrypt_and_reconstruct(args: DecryptArgs) -> Result<(), BoxError> .create_new(true) .open(args.output_file)?, ); - let mut first_error = None; + let mut first_error = Ok(()); for (idx, (dec_report1, (dec_report2, dec_report3))) in decrypted_reports1 .zip(decrypted_reports2.zip(decrypted_reports3)) .enumerate() @@ -236,33 +236,33 @@ pub async fn decrypt_and_reconstruct(args: DecryptArgs) -> Result<(), BoxError> trigger_value, )?; } + // error handling in case decryption failed (Err(e1), _, _) => { writeln!(writer, "Decryption failed Record: {idx} Reason:{e1}",)?; eprintln!("Decryption failed Record: {idx} Reason:{e1}"); - if first_error.is_none() { - first_error = Some(e1); + if first_error.is_ok() { + first_error = Err(e1); } } (Ok(_), Err(e2), _) => { writeln!(writer, "Decryption failed Record: {idx} Reason:{e2}",)?; eprintln!("Decryption failed Record: {idx} Reason:{e2}"); - if first_error.is_none() { - first_error = Some(e2); + if first_error.is_ok() { + first_error = Err(e2); } } (Ok(_), Ok(_), Err(e3)) => { writeln!(writer, "Decryption failed Record: {idx} Reason:{e3}",)?; eprintln!("Decryption failed Record: {idx} Reason:{e3}"); - if first_error.is_none() { - first_error = Some(e3); + if first_error.is_ok() { + first_error = Err(e3); } } } } - match first_error { - None => Ok(()), - Some(err) => Err(Box::new(err)), - } + + first_error.unwrap(); + Ok(()) } #[cfg(test)] From 40b2760c81a462f9b1f4b90ea2362e715dad44fa Mon Sep 17 00:00:00 2001 From: Erik Taubeneck Date: Mon, 16 Sep 2024 15:57:02 -0700 Subject: [PATCH 4/6] move error printing into iterator trait --- ipa-core/src/cli/crypto.rs | 48 +++++++++++++++----------------------- 1 file changed, 19 insertions(+), 29 deletions(-) diff --git a/ipa-core/src/cli/crypto.rs b/ipa-core/src/cli/crypto.rs index 84871776a..8428f477d 100644 --- a/ipa-core/src/cli/crypto.rs +++ b/ipa-core/src/cli/crypto.rs @@ -129,8 +129,10 @@ async fn build_hpke_registry( } struct DecryptedReports { + filename: PathBuf, reader: BufReader, key_registry: KeyRegistry, + iter_index: usize, } impl DecryptedReports { @@ -139,8 +141,10 @@ impl DecryptedReports { .unwrap_or_else(|e| panic!("unable to open file {filename:?}. {e}")); let reader = BufReader::new(file); Self { + filename: filename.to_path_buf(), reader, key_registry, + iter_index: 0, } } } @@ -151,11 +155,22 @@ impl Iterator for DecryptedReports { fn next(&mut self) -> Option { let mut line = String::new(); if self.reader.read_line(&mut line).unwrap() > 0 { + self.iter_index += 1; let encrypted_report_bytes = hex::decode(line.trim()).unwrap(); let enc_report = EncryptedOprfReport::from_bytes(encrypted_report_bytes.as_slice()).unwrap(); let dec_report = enc_report.decrypt(&self.key_registry); - Some(dec_report) + match dec_report { + Ok(dec_report) => Some(Ok(dec_report)), + Err(e) => { + eprintln!( + "Decryption failed: File: {0}. Record: {1}. Error: {e}.", + self.filename.display(), + self.iter_index + ); + Some(Err(e)) + } + } } else { None } @@ -180,10 +195,8 @@ pub async fn decrypt_and_reconstruct(args: DecryptArgs) -> Result<(), BoxError> .create_new(true) .open(args.output_file)?, ); - let mut first_error = Ok(()); - for (idx, (dec_report1, (dec_report2, dec_report3))) in decrypted_reports1 - .zip(decrypted_reports2.zip(decrypted_reports3)) - .enumerate() + for (dec_report1, (dec_report2, dec_report3)) in + decrypted_reports1.zip(decrypted_reports2.zip(decrypted_reports3)) { match (dec_report1, dec_report2, dec_report3) { (Ok(dec_report1), Ok(dec_report2), Ok(dec_report3)) => { @@ -236,32 +249,9 @@ pub async fn decrypt_and_reconstruct(args: DecryptArgs) -> Result<(), BoxError> trigger_value, )?; } - // error handling in case decryption failed - (Err(e1), _, _) => { - writeln!(writer, "Decryption failed Record: {idx} Reason:{e1}",)?; - eprintln!("Decryption failed Record: {idx} Reason:{e1}"); - if first_error.is_ok() { - first_error = Err(e1); - } - } - (Ok(_), Err(e2), _) => { - writeln!(writer, "Decryption failed Record: {idx} Reason:{e2}",)?; - eprintln!("Decryption failed Record: {idx} Reason:{e2}"); - if first_error.is_ok() { - first_error = Err(e2); - } - } - (Ok(_), Ok(_), Err(e3)) => { - writeln!(writer, "Decryption failed Record: {idx} Reason:{e3}",)?; - eprintln!("Decryption failed Record: {idx} Reason:{e3}"); - if first_error.is_ok() { - first_error = Err(e3); - } - } + (_, _, _) => {} } } - - first_error.unwrap(); Ok(()) } From ce50fbfa6fd9d837d48fd913dae8228193af505f Mon Sep 17 00:00:00 2001 From: Erik Taubeneck Date: Mon, 16 Sep 2024 19:15:39 -0700 Subject: [PATCH 5/6] fix clippy error --- ipa-core/src/cli/crypto.rs | 105 ++++++++++++++++++------------------- 1 file changed, 52 insertions(+), 53 deletions(-) diff --git a/ipa-core/src/cli/crypto.rs b/ipa-core/src/cli/crypto.rs index 8428f477d..c7c16ab40 100644 --- a/ipa-core/src/cli/crypto.rs +++ b/ipa-core/src/cli/crypto.rs @@ -141,7 +141,7 @@ impl DecryptedReports { .unwrap_or_else(|e| panic!("unable to open file {filename:?}. {e}")); let reader = BufReader::new(file); Self { - filename: filename.to_path_buf(), + filename: filename.clone(), reader, key_registry, iter_index: 0, @@ -198,58 +198,57 @@ pub async fn decrypt_and_reconstruct(args: DecryptArgs) -> Result<(), BoxError> for (dec_report1, (dec_report2, dec_report3)) in decrypted_reports1.zip(decrypted_reports2.zip(decrypted_reports3)) { - match (dec_report1, dec_report2, dec_report3) { - (Ok(dec_report1), Ok(dec_report2), Ok(dec_report3)) => { - let timestamp = [ - dec_report1.timestamp, - dec_report2.timestamp, - dec_report3.timestamp, - ] - .reconstruct() - .as_u128(); - - let match_key = [ - dec_report1.match_key, - dec_report2.match_key, - dec_report3.match_key, - ] - .reconstruct() - .as_u128(); - - // these aren't reconstucted, so we explictly make sure - // they are consistent across all three files, then set - // it to the first one (without loss of generality) - assert_eq!(dec_report1.event_type, dec_report2.event_type); - assert_eq!(dec_report2.event_type, dec_report3.event_type); - let is_trigger_report = dec_report1.event_type == EventType::Trigger; - - let breakdown_key = [ - dec_report1.breakdown_key, - dec_report2.breakdown_key, - dec_report3.breakdown_key, - ] - .reconstruct() - .as_u128(); - - let trigger_value = [ - dec_report1.trigger_value, - dec_report2.trigger_value, - dec_report3.trigger_value, - ] - .reconstruct() - .as_u128(); - - writeln!( - writer, - "{},{},{},{},{}", - timestamp, - match_key, - u8::from(is_trigger_report), - breakdown_key, - trigger_value, - )?; - } - (_, _, _) => {} + if let (Ok(dec_report1), Ok(dec_report2), Ok(dec_report3)) = + (dec_report1, dec_report2, dec_report3) + { + let timestamp = [ + dec_report1.timestamp, + dec_report2.timestamp, + dec_report3.timestamp, + ] + .reconstruct() + .as_u128(); + + let match_key = [ + dec_report1.match_key, + dec_report2.match_key, + dec_report3.match_key, + ] + .reconstruct() + .as_u128(); + + // these aren't reconstucted, so we explictly make sure + // they are consistent across all three files, then set + // it to the first one (without loss of generality) + assert_eq!(dec_report1.event_type, dec_report2.event_type); + assert_eq!(dec_report2.event_type, dec_report3.event_type); + let is_trigger_report = dec_report1.event_type == EventType::Trigger; + + let breakdown_key = [ + dec_report1.breakdown_key, + dec_report2.breakdown_key, + dec_report3.breakdown_key, + ] + .reconstruct() + .as_u128(); + + let trigger_value = [ + dec_report1.trigger_value, + dec_report2.trigger_value, + dec_report3.trigger_value, + ] + .reconstruct() + .as_u128(); + + writeln!( + writer, + "{},{},{},{},{}", + timestamp, + match_key, + u8::from(is_trigger_report), + breakdown_key, + trigger_value, + )?; } } Ok(()) From 2f0e0c15d8bb8008a6b040e423f8a7a4391ee2ce Mon Sep 17 00:00:00 2001 From: Richa Jain Date: Mon, 28 Oct 2024 14:47:51 +0800 Subject: [PATCH 6/6] Adding deryption failure logging --- ipa-core/src/cli/crypto/decrypt.rs | 133 +++++++++++++++++------------ 1 file changed, 79 insertions(+), 54 deletions(-) diff --git a/ipa-core/src/cli/crypto/decrypt.rs b/ipa-core/src/cli/crypto/decrypt.rs index 63ec8e3f9..b68de8973 100644 --- a/ipa-core/src/cli/crypto/decrypt.rs +++ b/ipa-core/src/cli/crypto/decrypt.rs @@ -14,7 +14,7 @@ use crate::{ U128Conversions, }, hpke::{KeyRegistry, PrivateKeyOnly}, - report::{EncryptedOprfReport, EventType, OprfReport}, + report::{EncryptedOprfReport, EventType, InvalidReportError, OprfReport}, test_fixture::Reconstruct, }; @@ -104,54 +104,58 @@ impl DecryptArgs { for (dec_report1, (dec_report2, dec_report3)) in decrypted_reports1.zip(decrypted_reports2.zip(decrypted_reports3)) { - let timestamp = [ - dec_report1.timestamp, - dec_report2.timestamp, - dec_report3.timestamp, - ] - .reconstruct() - .as_u128(); - - let match_key = [ - dec_report1.match_key, - dec_report2.match_key, - dec_report3.match_key, - ] - .reconstruct() - .as_u128(); - - // these aren't reconstucted, so we explictly make sure - // they are consistent across all three files, then set - // it to the first one (without loss of generality) - assert_eq!(dec_report1.event_type, dec_report2.event_type); - assert_eq!(dec_report2.event_type, dec_report3.event_type); - let is_trigger_report = dec_report1.event_type == EventType::Trigger; - - let breakdown_key = [ - dec_report1.breakdown_key, - dec_report2.breakdown_key, - dec_report3.breakdown_key, - ] - .reconstruct() - .as_u128(); - - let trigger_value = [ - dec_report1.trigger_value, - dec_report2.trigger_value, - dec_report3.trigger_value, - ] - .reconstruct() - .as_u128(); - - writeln!( - writer, - "{},{},{},{},{}", - timestamp, - match_key, - u8::from(is_trigger_report), - breakdown_key, - trigger_value, - )?; + if let (Ok(dec_report1), Ok(dec_report2), Ok(dec_report3)) = + (dec_report1, dec_report2, dec_report3) + { + let timestamp = [ + dec_report1.timestamp, + dec_report2.timestamp, + dec_report3.timestamp, + ] + .reconstruct() + .as_u128(); + + let match_key = [ + dec_report1.match_key, + dec_report2.match_key, + dec_report3.match_key, + ] + .reconstruct() + .as_u128(); + + // these aren't reconstucted, so we explictly make sure + // they are consistent across all three files, then set + // it to the first one (without loss of generality) + assert_eq!(dec_report1.event_type, dec_report2.event_type); + assert_eq!(dec_report2.event_type, dec_report3.event_type); + let is_trigger_report = dec_report1.event_type == EventType::Trigger; + + let breakdown_key = [ + dec_report1.breakdown_key, + dec_report2.breakdown_key, + dec_report3.breakdown_key, + ] + .reconstruct() + .as_u128(); + + let trigger_value = [ + dec_report1.trigger_value, + dec_report2.trigger_value, + dec_report3.trigger_value, + ] + .reconstruct() + .as_u128(); + + writeln!( + writer, + "{},{},{},{},{}", + timestamp, + match_key, + u8::from(is_trigger_report), + breakdown_key, + trigger_value, + )?; + } } Ok(()) @@ -159,22 +163,34 @@ impl DecryptArgs { } struct DecryptedReports { + filename: PathBuf, reader: BufReader, key_registry: KeyRegistry, + iter_index: usize, } impl Iterator for DecryptedReports { - type Item = OprfReport; + type Item = Result, InvalidReportError>; fn next(&mut self) -> Option { let mut line = String::new(); if self.reader.read_line(&mut line).unwrap() > 0 { + self.iter_index += 1; let encrypted_report_bytes = hex::decode(line.trim()).unwrap(); let enc_report = EncryptedOprfReport::from_bytes(encrypted_report_bytes.as_slice()).unwrap(); - let dec_report: OprfReport = - enc_report.decrypt(&self.key_registry).unwrap(); - Some(dec_report) + let dec_report = enc_report.decrypt(&self.key_registry); + match dec_report { + Ok(dec_report) => Some(Ok(dec_report)), + Err(e) => { + eprintln!( + "Decryption failed: File: {0}. Record: {1}. Error: {e}.", + self.filename.display(), + self.iter_index + ); + Some(Err(e)) + } + } } else { None } @@ -187,8 +203,10 @@ impl DecryptedReports { .unwrap_or_else(|e| panic!("unable to open file {filename:?}. {e}")); let reader = BufReader::new(file); Self { + filename: filename.clone(), reader, key_registry, + iter_index: 0, } } } @@ -203,6 +221,10 @@ async fn build_hpke_registry( #[cfg(test)] mod tests { + use std::{ + fs::File, + io::{BufRead, BufReader}, + }; use tempfile::tempdir; @@ -240,7 +262,6 @@ mod tests { } #[tokio::test] - #[should_panic = "called `Result::unwrap()` on an `Err` value: Crypt(Other)"] async fn decrypt_bad_private_key() { let input_file = sample_data::write_csv(sample_data::test_ipa_data().take(10)).unwrap(); @@ -274,5 +295,9 @@ mod tests { .decrypt_and_reconstruct() .await .unwrap(); + + let file = File::open(decrypt_output).unwrap(); + let reader = BufReader::new(file); + assert_eq!(reader.lines().count(), 0); } }