Add configuration option for trusted proxies#1126
Conversation
|
@jeschneiderhan thanks for the PR, I think the idea makes sense 👍 |
|
@st0012 sounds good! Thanks! |
|
@jeschneiderhan sorry for the delay, we have several bug fixes needed to be shipped this week, which contains a few breaking changes. so we cut the I'll review and merge it once we confirm the |
51a9074 to
b40e709
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 4-2 #1126 +/- ##
=======================================
Coverage 98.48% 98.49%
=======================================
Files 102 102
Lines 4565 4589 +24
=======================================
+ Hits 4496 4520 +24
Misses 69 69 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@st0012 thanks for the update. I've rebased off of master and resolved the conflicts. |
| client_ips = ips_from(@client_ip) | ||
| real_ips = ips_from(@real_ip) | ||
| forwarded_ips = ips_from(@forwarded_for) | ||
| forwarded_ips = ips_from(@forwarded_for).reverse |
There was a problem hiding this comment.
can you add a comment on why we need to call reverse here?
There was a problem hiding this comment.
👍 Comment added.
From the Mozilla docs:
If a request goes through multiple proxies, the IP addresses of each successive proxy is listed. This means, the right-most IP address is the IP address of the most recent proxy and the left-most IP address is the IP address of the originating client.
When processing these values for a client IP address, one typically starts from the rightmost value (the last proxy) and walks backward until the first IP not in a trusted IP range is found. This is used as the client IP address since nothing before that can be "trusted" to have recorded legitimate values.
The ActiveDispatch code reverses the list prior to processing, for example.
|
Thanks for the comments, @st0012. I'm hoping to find time to address them this afternoon. |
* Add `trusted_proxies` configuration option to sentry-ruby * Add existing ActionDispatch `trusted_proxies` values
* Isolate trusted proxy test configuration * Add comments to explain why we reverse the forwarded_for ip list * Call `uniq` on the trusted proxy list * Rename `filter_local_addresses(ips) to filter_trusted_proxy_addresses(ips)
b40e709 to
caef6c7
Compare
|
@st0012 I think I got to everything 👍 |
st0012
left a comment
There was a problem hiding this comment.
I left some comments on updating the test cases. otherwise all look good now 👍
st0012
left a comment
There was a problem hiding this comment.
@jeschneiderhan thanks for the great work ❤️ I'll resolve the conflicts 😉
|
I think the check that failed may have been a transient error. Can we re-run it? |
|
@jeschneiderhan the failure is actually inherited from the |
* Add ThreadsInterface (#1178) * Add ThreadsInterface * Update changelog * Inspect exception cause by default & don't exclude ActiveJob::DeserializationError (#1180) * Turn on inspect_exception_causes_for_exclusion by default With this config turned on, we can avoid matching the surface exceptions in integrations, which could cause issues like #1071. Solves #642. * Remove ActiveJob::DeserializationError from ignored list Since the previous commit solves #642, this commit can remove ActiveJob::DeserializationError from the ignored exceptions list. Solves #1071. * Update async document * Update changelog * Make sentry-rails a Rails engine and provide default job class for async (#1181) * Make sentry-rails a Rails engine too * Add Sentry::SendEventJob Instead of letting users defining their SentryJob class, we should provide a default job class for them. * Update document and example for the new job class * Update changelog * Add configuration option for trusted proxies (#1126) * Add configuration option for trusted proxies * Add `trusted_proxies` configuration option to sentry-ruby * Add existing ActionDispatch `trusted_proxies` values * Address some PR feedback * Isolate trusted proxy test configuration * Add comments to explain why we reverse the forwarded_for ip list * Call `uniq` on the trusted proxy list * Rename `filter_local_addresses(ips) to filter_trusted_proxy_addresses(ips) * Remove duplicated hash entry * Update some tests after PR feedback * retrigger checks Co-authored-by: Stan Lo <stan001212@gmail.com> * Only define SendEventJob when ActiveJob is defined * Allow users to configure ActiveJob adapters to ignore (#1256) * Allow users to configure ActiveJob adapters to ignore * Update changelog * Add sidekiq adapter to sentry-rails' ignored adapters list (#1257) * Add sidekiq adapter to sentry-rails' ignored adapters list * Update changelog * Tag queue name and jid on sidekiq events (#1258) * Add queue name and jid to event tags * Update changelog * Tag job_id and provider_job_id on ActiveJob events (#1259) * Refactor/test ActiveJob's context data * Tag job_id and provider_job_id on ActiveJob events * Update changelog * Add ability to have many post initialization callbacks (#1261) * Add ability to have many post initialization callbacks * Revert version bumping, fix codestyle and rewrite rspec test * Remove dependenciy bumping from sentry-sidekiq * Add entries to CHANGELOG * Support config.before_breadcrumb (#1253) * Support config.before_breadcrumb Example: ``` config.before_breadcrumb = lambda do |breadcrumb, hint| breadcrumb.message = "foo" breadcrumb end ``` * Update changelog * Update sentry-ruby's changelog * Update sentry-rails' changelog * Update sentry-sidekiq's changelog * Rename ignored_active_job_adapters to skippable_job_adapters (#1264) * Update sentry-ruby's changelog Co-authored-by: Jon-Erik Schneiderhan <45184220+jeschneiderhan@users.noreply.github.com> Co-authored-by: Valentine Kiselev <mrexox@outlook.com>
* Add configuration option for trusted proxies * Add `trusted_proxies` configuration option to sentry-ruby * Add existing ActionDispatch `trusted_proxies` values * Address some PR feedback * Isolate trusted proxy test configuration * Add comments to explain why we reverse the forwarded_for ip list * Call `uniq` on the trusted proxy list * Rename `filter_local_addresses(ips) to filter_trusted_proxy_addresses(ips) * Remove duplicated hash entry * Update some tests after PR feedback * retrigger checks Co-authored-by: Stan Lo <stan001212@gmail.com>
Description
When Sentry calculates the IP address for a user, it will skip addresses that fall in a predetermined list of private IP address ranges: https://github.com/getsentry/sentry-ruby/blob/master/sentry-ruby/lib/sentry/utils/real_ip.rb#L10-L17. Some people run services behind proxies that are not part of these ranges, which currently results in the IP address of the proxy being used as the user IP address.
For example, Let's assume I have a service running behind Cloudflare. When a request from a client (
2.2.2.2) reaches my application, it has headers like the following:Right now, if the request throws an error, the Sentry will have a user IP address of
172.68.54.247since it is the first address encountered that is not in the default list of IPs to skip.ActionDispatch allows the specification of
trusted_proxiesvia theconfig.action_dispatch.trusted_proxiessetting. Any IP ranges specified will be added to default list of private address ranges (See remote_ip.rb for more information). I've attempted to provide a similar setting to thesentry-rubyconfiguration. It also updates thesentry-railsrailtie to automatically add anyconfig.action_dispatch.trusted_proxiesvalues to the sentry-rubytrusted_proxies.With these settings, it is possible to add the IP ranges of the non-private proxies that you trust. These IPs will be skipped, and in the example above, will result in Sentries using the correct user IP (
2.2.2.2in the above example).I also noticed that the
X-Forwarded-Forheader is being iterated from the beginning of the string. Although this works a lot of the time, I think the usual way to process this header is to iterate from right to left, taking the first IP that is not in the trusted list. The remote_ip code reverses the list prior to processing, for example. I added aforwarded_ips.reversecall below to match that behavior.I hope that all makes sense!