[Decrypt script] In case decryption fails for a record, ignore and continue to decrypt #1270

richajaindce · 2024-09-10T13:19:56Z

In case a record fails decryption, we will log it on the console and the output file. However, we will let the processing complete for all the records before erroring out completely.

File output

Decryption failed Record: 0 Reason:en/decryption failure: Failed to open ciphertext
203,9361892166529446633,0,0,0
0,8452333888243494998,0,0,0
279,9361892166529446633,0,0,0
0,13796936852565581985,0,0,0
342,9361892166529446633,0,0,0
1,2805086832267079501,0,0,0
Decryption failed Record: 8 Reason:en/decryption failure: Failed to open ciphertext

Console output

Decryption failed Record: 0 Reason:en/decryption failure: Failed to open ciphertext
Decryption failed Record: 7 Reason:en/decryption failure: Failed to open ciphertext
Error: Crypt(Other)

FYI: i ended up not adding a flag for this as this is useful information while running a script

codecov · 2024-09-10T13:42:37Z

Codecov Report

Attention: Patch coverage is 83.78378% with 12 lines in your changes missing coverage. Please review.

Project coverage is 93.49%. Comparing base (f199c46) to head (bd65cb5).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
ipa-core/src/cli/crypto.rs	83.78%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1270      +/-   ##
==========================================
+ Coverage   92.47%   93.49%   +1.01%     
==========================================
  Files         201      202       +1     
  Lines       31925    32714     +789     
==========================================
+ Hits        29522    30585    +1063     
+ Misses       2403     2129     -274

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

akoshelev

Do we still want to use this command to verify end-to-end encryption cycle with iOS/Android? Swallowing errors may prevent us from catching issues here. Maybe we should provide an extra flag (disabled by default) that will instruct the CLI to skip over malformed reports

akoshelev · 2024-09-10T15:55:14Z

ipa-core/src/cli/crypto.rs

+                    trigger_value,
+                )?;
+            }
+            _ => println!("Decryption failed for record no {idx}"),


Suggested change

_ => println!("Decryption failed for record no {idx}"),

_ => eprintln!("Decryption failed for record no {idx}"),

FYI I ended up not adding a flag for this as this is useful information while decrypting rows @akoshelev

richajaindce · 2024-09-11T00:04:23Z

Do we still want to use this command to verify end-to-end encryption cycle with iOS/Android? Swallowing errors may prevent us from catching issues here. Maybe we should provide an extra flag (disabled by default) that will instruct the CLI to skip over malformed reports

Yes, let me add a flag to do this.
What we observed was that sometimes the public key is not propagated for all the users who are getting targeted for the ads. In that case, they would still be using the stale keys. In such cases, mpc should throw away the records it can't decrypt. This change will let us do sanity check our iOS/android/dataswarm implementation.

Having said that, is there a way to "throw away" non-decryptable records while running query today @akoshelev ?

eriktaubeneck · 2024-09-11T02:39:09Z

I'm not convinced this is the right approach. For example, if different records are not decryptable across helpers, it will result in errors downstream or potentially just incorrect results. Moreover, decryption happens independently across helpers, so there would be some need for helpersto align o. which events they dropped.

I think we should instead require the report collector to only include events encrypted towards a given HPN to be submitted, and to panic if any are not decryptable.

richajaindce · 2024-09-11T03:14:08Z

I'm not convinced this is the right approach. For example, if different records are not decryptable across helpers, it will result in errors downstream or potentially just incorrect results. Moreover, decryption happens independently across helpers, so there would be some need for helpersto align o. which events they dropped.

I think we should instead require the report collector to only include events encrypted towards a given HPN to be submitted, and to panic if any are not decryptable.

Just wanted to check the purpose of this decrypt script? I assume it is for our own testing as a validation step before we can run a real test (where we won't be able to use this script due to no access to private keys). While testing our integration, before this change, the script just returned an error. However, it was able to decrypt most of them except a few rows due to public key mismatch.

With this change, hoping we can get to finer details with a single run. And we can gate this with an argument as Alex suggested. WDYT?

eriktaubeneck · 2024-09-11T04:40:08Z

it's concerning to me that we wouldn't know what keys an event was encrypted with. this util is to assist with testing, and it doesn't aid in that functionality if it succeeds on files that would fail
in an actual run. as I currently understand it, we assume all events submitted to the HPN are decryptable, so relaxing that assumption here may result in unexpected behavior.

given this is a testing util, we could provide better error handling, such as notifying the user as to which reports were not decryptable.

akoshelev · 2024-09-11T05:07:50Z

Having said that, is there a way to "throw away" non-decryptable records while running query today @akoshelev ?

We don't have it currently, but we discussed it with @danielmasny and this should be eventually in our roadmap. It requires communication across helpers, but generally they should be able to tell others to toss certain reports away. There should be some limits on this functionality (what if one helpers tells others to remove everything but two), but generally it is desirable to have I think

eriktaubeneck · 2024-09-11T05:39:00Z

it makes sense to eventually support (bits will inevitably get flipped occasionally). until we have that functionality, I think we should keep this util
in line with existing functionality, and we'd be better served by providing clearer error reporting for testing so that we can remove corrupt reports.

danielmasny · 2024-09-11T19:31:16Z

There are several things we would need, one is to sync with the other helpers that the report should be ignored. The second is that we also need to check the shares for consistency, i.e. both helper decrypt the same share. It would be quite bad if the whole query fails just because of a single bad report.

…cess other rows

eriktaubeneck

a few comments below, but this approach works much better. we need to make sure we log all the errors, and we may even want to differentiate the case where a given row is un-decryptable by all 3 helpers vs only an individual helper.

eriktaubeneck · 2024-09-13T22:39:10Z

ipa-core/src/cli/crypto.rs

+                )?;
+            }
+            // error handling in case decryption failed
+            (Err(e1), _, _) => {


this will end up only matching one of the errors, but it's probably useful to log all 3 if they fail, right?

eriktaubeneck · 2024-09-13T22:39:53Z

ipa-core/src/cli/crypto.rs

+            }
+            // error handling in case decryption failed
+            (Err(e1), _, _) => {
+                writeln!(writer, "Decryption failed Record: {idx} Reason:{e1}",)?;


do we actually want to write these to the file?

eriktaubeneck · 2024-09-13T22:40:19Z

ipa-core/src/cli/crypto.rs

-
-    for (dec_report1, (dec_report2, dec_report3)) in
-        decrypted_reports1.zip(decrypted_reports2.zip(decrypted_reports3))
+    let mut first_error = Ok(());


what is this for?

eriktaubeneck · 2024-09-13T22:50:25Z

ipa-core/src/cli/crypto.rs

-            breakdown_key,
-            trigger_value,
-        )?;
+        match (dec_report1, dec_report2, dec_report3) {


it may make sense to check each for errors explicitly up top, so that we make sure we log all 3. we'll want to make sure that all 3 are not decryptable. the suggestion is just a sketch, don't merge it directly.

Suggested change

match (dec_report1, dec_report2, dec_report3) {

if dec_report1.is_err() { # log here }

if dec_report2.is_err() { # log here }

if dec_report3.is_err() { # log here }

match (dec_report1, dec_report2, dec_report3) {

(Ok(dec_report1), Ok(dec_report2), Ok(dec_report3)) => { ... }

_ => { ... }

}

move error printing into iterator trait

fix clippy error

richajaindce force-pushed the decrypt branch from 1caf7d5 to afbe29c Compare September 10, 2024 13:21

In case decryption fails, do not error out

794c9ae

richajaindce force-pushed the decrypt branch from afbe29c to 794c9ae Compare September 10, 2024 13:27

akoshelev reviewed Sep 10, 2024

View reviewed changes

In case of an error it, log the error in the file and continue to pro…

9baca44

…cess other rows

richajaindce force-pushed the decrypt branch from 1e0395b to 9baca44 Compare September 13, 2024 05:46

Fixing test

bd65cb5

richajaindce force-pushed the decrypt branch from 4e68877 to bd65cb5 Compare September 13, 2024 13:59

eriktaubeneck reviewed Sep 13, 2024

View reviewed changes

eriktaubeneck and others added 4 commits September 16, 2024 15:57

move error printing into iterator trait

40b2760

Merge pull request #36 from eriktaubeneck/decrypt

95af9ff

move error printing into iterator trait

fix clippy error

ce50fbf

Merge pull request #37 from eriktaubeneck/decrypt2

f952513

fix clippy error

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Decrypt script] In case decryption fails for a record, ignore and continue to decrypt #1270

[Decrypt script] In case decryption fails for a record, ignore and continue to decrypt #1270

richajaindce commented Sep 10, 2024 •

edited

Loading

codecov bot commented Sep 10, 2024 •

edited

Loading

akoshelev left a comment

akoshelev Sep 10, 2024

richajaindce Sep 13, 2024 •

edited

Loading

richajaindce commented Sep 11, 2024

eriktaubeneck commented Sep 11, 2024

richajaindce commented Sep 11, 2024

eriktaubeneck commented Sep 11, 2024

akoshelev commented Sep 11, 2024

eriktaubeneck commented Sep 11, 2024

danielmasny commented Sep 11, 2024

eriktaubeneck left a comment

eriktaubeneck Sep 13, 2024

eriktaubeneck Sep 13, 2024

eriktaubeneck Sep 13, 2024

eriktaubeneck Sep 13, 2024

	_ => println!("Decryption failed for record no {idx}"),
	_ => eprintln!("Decryption failed for record no {idx}"),

-        match (dec_report1, dec_report2, dec_report3) {
+        if dec_report1.is_err() { # log here }
+        if dec_report2.is_err() { # log here }
+        if dec_report3.is_err() { # log here }
+        match (dec_report1, dec_report2, dec_report3) {
+            (Ok(dec_report1), Ok(dec_report2), Ok(dec_report3)) => { ... }
+            _ => { ... }
+        }

[Decrypt script] In case decryption fails for a record, ignore and continue to decrypt #1270

Are you sure you want to change the base?

[Decrypt script] In case decryption fails for a record, ignore and continue to decrypt #1270

Conversation

richajaindce commented Sep 10, 2024 • edited Loading

codecov bot commented Sep 10, 2024 • edited Loading

Codecov Report

akoshelev left a comment

Choose a reason for hiding this comment

akoshelev Sep 10, 2024

Choose a reason for hiding this comment

richajaindce Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

richajaindce commented Sep 11, 2024

eriktaubeneck commented Sep 11, 2024

richajaindce commented Sep 11, 2024

eriktaubeneck commented Sep 11, 2024

akoshelev commented Sep 11, 2024

eriktaubeneck commented Sep 11, 2024

danielmasny commented Sep 11, 2024

eriktaubeneck left a comment

Choose a reason for hiding this comment

eriktaubeneck Sep 13, 2024

Choose a reason for hiding this comment

eriktaubeneck Sep 13, 2024

Choose a reason for hiding this comment

eriktaubeneck Sep 13, 2024

Choose a reason for hiding this comment

eriktaubeneck Sep 13, 2024

Choose a reason for hiding this comment

richajaindce commented Sep 10, 2024 •

edited

Loading

codecov bot commented Sep 10, 2024 •

edited

Loading

richajaindce Sep 13, 2024 •

edited

Loading