-
Notifications
You must be signed in to change notification settings - Fork 91
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
Loosen IP address scrubbing in span.domain
tag
#3572
Comments
span.domain
tag
I wonder if we could get to an "80%" solution by hardcoding a list of"special" IPs. E.g. reserved IPs, DNS servers, .... Unless this does not cover the majority of the cases. |
I'd like to see private IPs being not redacted on SaaS and self-hosted. For public IPs, I think it's better if it's redacted on SaaS and not redacted on self-hosted. Since on self-hosted, it's a different environment, it's only possible that the outgoing request is made only for those defined on the |
Hey @jjbayer @gggritso seeing this PR I think I can implement this one.. partially. Right now we only allow
But considering what @jjbayer said, it's a good idea to whitelist "special IPs", with what I had in mind are:
Private IPv6 address has higher address count, so I believe that's out of the question. For popular IPs like DNS servers and such, I don't think there's any good use case for it to be included in a span. Although I might be wrong here. |
I think we should just use |
Private addresses can still be high cardinality, so I wouldn't unconditionally allowlist them all. IMO the simplest thing we can start with is add a global option that allows self-hosted users to disable IP scrubbing. @gggritso what do you think? |
I'm wondering about how "high" is the high cardinality for you?
Yes, this should also work, since self-hosted has known range. But the default should be set to |
Can't give you an exact number, but the purpose of the scrubbing is to merge spans into "meaningful" groups. So accepting every single private IP as a separate group would be too fine grained IMO, but we could do partial scrubbing for private IPs like the issue description proposes (e.g. |
We haven't heard a lot of complaints about this issue lately, so I don't have a strong opinion. I think starting with allowing self-hosted users to turn off scrubbing completely feels like a good solution even though it'll let people degrade their own experience with Sentry. Either that, or an IP allowlist for self-hosted folks seems like it'd solve a lot of these problems! |
I'd rather go with @gggritso's solution on whitelisting a certain IPs. I'm also thinking of specifying CIDRs? Something along the line of |
Sounds good to me. @aldy505 I'll add it to our backlog, I cannot make any promises on when this will be implemented though. |
I might be able to work on this later this weekend, can you point out to me somethings that I need to take a look at? (Other than this file https://github.com/getsentry/relay/blob/master/relay-event-normalization/src/normalize/span/description/mod.rs) |
@aldy505 that would be great and very welcome! Best would probably be to register an option for the allowlist on sentry side, add it to this list: https://github.com/getsentry/sentry/blob/23ccc70817cc6dbc43230ddd86c2f6329ecb34ab/src/sentry/relay/globalconfig.py#L13-L14, then declare it in relay as well: relay/relay-dynamic-config/src/global.rs Lines 106 to 110 in a440dbb
|
@jjbayer Quick question, since I need to implement the allowlist here: relay/relay-event-normalization/src/normalize/span/description/mod.rs Lines 228 to 234 in 6dc184f
And since we can't make |
I have no better idea than passing it all the way through the call stack, like we do here: relay/relay-server/src/services/processor.rs Line 1483 in 6dc184f
|
I see, thanks. Might do this tomorrow. |
As discussed on #3572. ### Legal Boilerplate Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms. --------- Co-authored-by: Joris Bayer <[email protected]>
The allowlist approach is now implemented and merged (see linked PRs). |
Awesome! |
…74195) Complementary relay config for getsentry/relay#3813, based on the discussion on getsentry/relay#3572 ### Legal Boilerplate Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms. --------- Co-authored-by: Joris Bayer <[email protected]>
Problem Statement
IP addresses like
8.8.8.8
get fully scrubbed out to*.*.*.*
. When users look at the requests module, all requests to IP addresses get mashed together.Solution Brainstorm
8.8.8.8
becomes*.*.8.8
or*.*.*.8
or8.8.*.*
)Product Area
Ingestion and Filtering
The text was updated successfully, but these errors were encountered: