Adding tests to verify that encryption of match key happened correctly for ios side of in app browser #1043
Conversation
…y for ios side of in app browser
There was a problem hiding this comment.
@akoshelev i am storing this blob file in test_data folder. Is this correct place to store it?
There was a problem hiding this comment.
I am tempted to use something like rust-lang/cargo#2841 (comment) for large test files. If it is really small, perhaps you could hex-encode it and store it inside the test file
There was a problem hiding this comment.
I would still prefer for it to sit in a separate file as then we can compare this file with what we generate on client side by simply taking a diff. On the iOS side, I am going to dump a file with the contents and leaving an action item to validate the contents each time a change is made in the implementation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1043 +/- ##
=========================================
+ Coverage 0 90.61% +90.61%
=========================================
Files 0 174 +174
Lines 0 26265 +26265
=========================================
+ Hits 0 23800 +23800
- Misses 0 2465 +2465 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I am tempted to use something like rust-lang/cargo#2841 (comment) for large test files. If it is really small, perhaps you could hex-encode it and store it inside the test file
| /// `ct_mk`: Enc(`match_key`) | ||
| /// associated data of `ct_mk`: `key_id`, `epoch`, `event_type`, `site_domain`, | ||
| #[derive(Copy, Clone, Eq, PartialEq)] | ||
| pub struct EncryptedMatchKeyReport<B> |
There was a problem hiding this comment.
I feel like this is a lot of code just to test the match key decryption. Is it possible to pad the match key encryption with dummy secret shares of ts, event type, etc and use our generic report struct for this purpose?
There was a problem hiding this comment.
I have not examined them side by side, but this code looks similar to the existing report parsing code. I agree with @akoshelev that it might be better to fill out dummy data to be able to reuse the existing structs. Or, if there is a new report struct that we need for real-world use cases, then maybe non-test code should be added to handle that report format.
| #[test] | ||
| fn test_matchkey_decrypt_encrypt() { | ||
| let mut rng = thread_rng(); | ||
| let pk = hex::decode("92a6fb666c37c008defd74abf3204ebea685742eab8347b08e2f7c759893947a") |
There was a problem hiding this comment.
maybe you could avoid copy-pasting the same key material and set it up once as a constant for all tests.
but probably if you could use GenericReport you don't need to test the encryption roundtrip
| /// `ct_mk`: Enc(`match_key`) | ||
| /// associated data of `ct_mk`: `key_id`, `epoch`, `event_type`, `site_domain`, | ||
| #[derive(Copy, Clone, Eq, PartialEq)] | ||
| pub struct EncryptedMatchKeyReport<B> |
There was a problem hiding this comment.
I have not examined them side by side, but this code looks similar to the existing report parsing code. I agree with @akoshelev that it might be better to fill out dummy data to be able to reuse the existing structs. Or, if there is a new report struct that we need for real-world use cases, then maybe non-test code should be added to handle that report format.
There was a problem hiding this comment.
Given that this module is only for tests, maybe name it matchkey_test or something like that? If the necessary APIs are all public, another option is an integration test in ipa-core/tests.
There was a problem hiding this comment.
I'm not sure how others feel about this, but I'd be in favor of moving away from mod.rs files, i.e. not doing this rename.
|
Closing this in favor of #1071 |
No description provided.