Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

ref(envelope): Introduce an item type for security reports #617

Merged
merged 4 commits into from
Jun 9, 2020

Conversation

jan-auer
Copy link
Member

@jan-auer jan-auer commented Jun 8, 2020

Introduces a separate item type for security events: ItemType::Security. This type allows for easy detection of security events without parsing the body. Other than that, it behaves exactly the same as ItemType::Event.

To create this item in event ingestion, the apply_to_event family of functions also sets the event type. The main processing function then uses this event type to create the appropriate item.

Raw security reports as submitted by the browser go into ItemType::RawSecurity. In ingestion, this item is parsed, transformed into a security event and then put into ItemType::Security.

Changelog (Internal): Security events (CSP, Expect-CT, Expect-Staple, and HPKP) are now placed into a dedicated security item in envelopes, rather than the generic event item. This allows for quick detection of the event type for rate limiting.

@jan-auer jan-auer requested a review from a team June 8, 2020 15:43
@jan-auer jan-auer self-assigned this Jun 8, 2020
@jan-auer jan-auer force-pushed the ref/envelope-security-item branch from 2247ca8 to 36b9957 Compare June 8, 2020 16:11
@jan-auer
Copy link
Member Author

jan-auer commented Jun 9, 2020

I'm not particularly proud about the changes in relay-general. Ideally, we would be able to infer the EventType without writing it into the payload. However, we need it even in external Relays, because otherwise they could not put it into the appropriate Envelope Item for forwarding.

@untitaker what do you think of this approach? Would you prefer something different?

@untitaker
Copy link
Member

I think this is fine, I am not sure I understand the concern.

Ideally, we would be able to infer the EventType without writing it into the payload.

We have dedicated interfaces for each of the security types so this type of inference seems almost trivial? But honestly I don't care if we materialize the event type upfront

* master:
  ref(quotas): Remove support for legacy quotas (#616)
  feat(server): Implement an HTTP outcomes producer (#592)
@jan-auer
Copy link
Member Author

jan-auer commented Jun 9, 2020

Moved the event type into relay-server now, so that it is closer to the requirement. There is a comment now describing why this is done.

@jan-auer jan-auer merged commit 425706f into master Jun 9, 2020
@jan-auer jan-auer deleted the ref/envelope-security-item branch June 9, 2020 08:56
jan-auer added a commit that referenced this pull request Jun 9, 2020
Introduces a separate item type for security events:
`ItemType::Security`. This type allows for easy detection of security
events without parsing the body. Other than that, it behaves exactly the
same as `ItemType::Event`.

To create this item in event ingestion, the `apply_to_event` family of
functions also sets the event type. The main processing function then
uses this event type to create the appropriate item.

Raw security reports as submitted by the browser go into
`ItemType::RawSecurity`. In ingestion, this item is parsed, transformed
into a security event and then put into `ItemType::Security`.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants