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

nginx: Remove obsolete real_ip configuration #7302

Closed
wants to merge 1 commit into from

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Oct 17, 2023

The configuration for this nginx module was original added in #1596, in combination with an IP-based limit_req rate limit. The rate limit was lately moved to the application layer (see #6875).

Since we no longer use the limit_req rate limit in the nginx config, it looks like we don't need the real_ip config anymore either.

Note that we should still receive and process the X-Forwarded-For header just like before, it just won't be processed by nginx anymore.

The configuration for this nginx module was original added in rust-lang#1596, in combination with an IP-based `limit_req` rate limit. The rate limit was lately moved to the application layer (see rust-lang#6875).

Since we no longer use the `limit_req` rate limit in the nginx config, it looks like we don't need the `real_ip` config anymore either.
@Turbo87 Turbo87 added C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear A-backend ⚙️ labels Oct 17, 2023
@syphar
Copy link
Member

syphar commented Oct 17, 2023

Aren't we also using the X-Real-IP header in the blocking-middleware?

@Turbo87
Copy link
Member Author

Turbo87 commented Oct 17, 2023

yeah, I just realized that while testing this on the staging environment. I've switched it over to reading the X-Forwarded-For header that the nginx config was referring to. Let's hope that this helps...

@Turbo87
Copy link
Member Author

Turbo87 commented Oct 17, 2023

oh, you meant the stuff in BLOCKED_TRAFFIC... yeah... right... that is also using X-Real-IP... 😩

@syphar
Copy link
Member

syphar commented Oct 17, 2023

since cloudfront was in set_real_ip_from I assume that heroku is behind CloudFront too, so we could set the header there?

Or block the IPs there?

@Turbo87
Copy link
Member Author

Turbo87 commented Oct 17, 2023

Let's hope that this helps...

it did not... this is more complicated than I had hoped 😅

I assume that heroku is behind CloudFront too, so we could set the header there?

Or block the IPs there?

I just took a quick look, and I couldn't find an option in CloudFront to block IPs on that level. My experience with AWS is limited thought, I'm probably just not looking in the right places :D

@Turbo87 Turbo87 closed this Oct 17, 2023
@Turbo87 Turbo87 deleted the real-ip branch October 17, 2023 11:35
@Turbo87 Turbo87 mentioned this pull request Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants