Skip to content

Conversation

Litarnus
Copy link
Contributor

@Litarnus Litarnus commented May 26, 2025

This PR adds a signature to requests between relays which is used to verify if the request comes from a trusted relay.

It leverages the signature mechanism used by relay-auth which already includes a timestamp which can be checked for a max_age. The signature itself does not contain any data since we are not interested in data integrity and we just want to see if we can successfully verify the signature by any of the public keys that are added by the user.

Envelopes coming from internal relays are not checked because they are already in our infrastructure and are considered trusted.

The max_age for envelopes from external relays is set to 5 Minutes currently.

ref RELAY-17

@Litarnus Litarnus marked this pull request as ready for review May 28, 2025 09:02
@Litarnus Litarnus requested a review from a team as a code owner May 28, 2025 09:02
@Litarnus Litarnus marked this pull request as draft June 4, 2025 09:06
@Litarnus Litarnus requested a review from Dav1dde June 30, 2025 07:54
@Litarnus Litarnus requested a review from Dav1dde July 2, 2025 11:35
Copy link
Member

@Dav1dde Dav1dde left a comment

Choose a reason for hiding this comment

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

Good stuff on the integration test doc strings, we should do that more often.

I spent a bit more time thinking about internal Relays. Right now we would be signing requests but not verifying the signature (as the Relay is internal).

I think we have 2 ways forward here, we can also verify the new signature against the internal keys, or not sign requests when we know the upstream is also internal, which we might be able to auto detect or just make it configurable if outbound envelope requests should be signed.

I am not opposed to just dealing with this in a follow-up, but we should have an eye on the CPU utilization when rolling this out.

@Litarnus
Copy link
Contributor Author

Litarnus commented Jul 2, 2025

I am not opposed to just dealing with this in a follow-up, but we should have an eye on the CPU utilization when rolling this out.

I will create a follow-up ticket for this

@Litarnus Litarnus added this pull request to the merge queue Jul 3, 2025
Merged via the queue into master with commit a471317 Jul 3, 2025
28 checks passed
@Litarnus Litarnus deleted the martinl/trusted-relay-signature branch July 3, 2025 07:50
Litarnus added a commit to getsentry/sentry that referenced this pull request Jul 3, 2025
This PR introduces a new discard reason that will be shown in the UI:
`Invalid Signature`.

ref RELAY-17

See: getsentry/relay#4772
andrewshie-sentry pushed a commit to getsentry/sentry that referenced this pull request Jul 14, 2025
This PR introduces a new discard reason that will be shown in the UI:
`Invalid Signature`.

ref RELAY-17

See: getsentry/relay#4772
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.

3 participants