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

add event type (is_trigger bit) to Info #598

Merged
merged 6 commits into from
Apr 18, 2023

Conversation

bmcase
Copy link
Member

@bmcase bmcase commented Apr 13, 2023

Implementing #593 to add an event type (is_trigger) to the associated data/Info. Right now have it with pub type and not an enum

Copy link
Collaborator

@akoshelev akoshelev left a comment

Choose a reason for hiding this comment

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

lgtm, we need to fix formatting issues and should be good to go

congrats @bmcase with your first PR!

src/hpke/mod.rs Outdated
@@ -373,7 +424,7 @@ mod tests {
let mut suite = EncryptionSuite::new(10, rng.clone());
// keep the originals, in case if we need to damage them
let (mut mkp_clone, mut site_domain_clone, mut helper_clone) = (mkp_origin.clone(), site_domain.clone(), helper_origin.clone());
let info = Info::new(0, 0, &mkp_origin, &site_domain, &helper_origin).unwrap();
let info = Info::new(0, 0, EventType::Source, &mkp_origin, &site_domain, &helper_origin).unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

this validates that source events will fail to decrypt with trigger bits, but does not validate the opposite.

We can do both and it is easy with proptest

  • generate a random bit
fn arbitrary_info_corruption(corrupted_info_field in 1..6,
                                         mkp_origin in "[a-z]{10}",
                                         site_domain in "[a-z]{10}",
                                         helper_origin in "[a-z]{10}",
                                         trigger_bit in 0_u8..=1, // <---
                                         seed: [u8; 32]) {
  • convert this bit to event type
let event_type = EventType::try_from(trigger_bit).unwrap();
  • flip the bit to induce trigger bit corruption
3 => Info {
    event_type: EventType::try_from(trigger_bit ^ 1).unwrap(),
    ..encryption.info
},

Copy link
Member Author

Choose a reason for hiding this comment

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

okay, I added this to the arbitrary_info_corruption test and I think fixed the format issue

src/hpke/info.rs Outdated
@@ -80,7 +84,8 @@ impl<'a> Info<'a> {
+ self.site_domain.len()
+ 4 // account for 4 delimiters
+ std::mem::size_of_val(&self.key_id)
+ std::mem::size_of_val(&self.epoch);
+ std::mem::size_of_val(&self.epoch)
+ std::mem::size_of_val( &self.event_type);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am genuinely surprised that rustfmt hasn't flagged that

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think the latest version has that; it should automatically update the PR, right? https://github.com/bmcase/raw-ipa/blob/main/src/hpke/info.rs

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like it is still there :)

                 -----v-----
std::mem::size_of_val( &self.event_type);

Copy link
Collaborator

Choose a reason for hiding this comment

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

to update this PR, you would need to push your changes again: git push

@akoshelev akoshelev merged commit 3a63ff9 into private-attribution:main Apr 18, 2023
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