Allow blocking arbitrary traffic without redeploying #1806
Conversation
This generalizes the `block_ips` module to allow checking against any header instead of just the `X-Real-Ip` header. This does not enable more complex matching such as regexes, but it does let us apply the simple string matching we have today to any header. Additionally, which headers we check against is now configured by an environment variable, which is a comma separated list in the form `Header=VALUE_ENV_VAR`. So to keep the behavior we had today, one would set `BLOCKED_TRAFFIC="X-Real-Ip=BLOCKED_IPS"`. This means that when we see traffic patterns that we need to block which can be handled by the existing logic, we can now begin applying it to patterns we didn't previosuly anticipate without requiring a redeploy
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
I'm wondering – why do we implement IP blocking and rate limiting in the backend server? Wouldn't it be easier to include this in the nginx configuration? |
|
Rate limiting is performed at the NGINX level unless it can't be. The only case that isn't handled in nginx is for publishing new crates. Determining whether We have another rate limit for publishes, which is less strict (and mostly to ensure we don't get rate limited by GH) which is handled in nginx, and I'm planning on opening a PR to add a few more at the nginx level soon. In general that's where we prefer to handle rate limiting, and I don't think it's correct to say we aren't handling it there. As for why blocking is done in app code, that's mostly a function of familiarity amongst the team. Blocking traffic is something that primarily happens during incident response, so having it happen where folks who are on call are most familiar with things is important to make sure we can effectively respond to these incidents quickly. My understanding is that nginx isn't really meant for that use case, and while it can be used for IP blocking (it couldn't when we first introduced the app level since $remote_addr is not correct by default, but I believe we do have the appropriate extensions configured for it to handle it now) -- It still isn't really set up to do the sort of arbitrary blocking that I'm referring to here, especially not without a redeploy. That said, I'm open to being proven wrong and moving this to nginx if everyone handling ops is up to date |
|
@sgrif Thanks for the detailed explanation. The rate-limiting setup completely makes sense to me now – if we need to connect to the DB, we can only do this in the backend, and the common case is already handled by nginx. IP blocking can be configured in nginx using So overall I understand the reasoning for implementing this ourselves, and it seems like the best approach for now. However, if we go ahead and implement more general HTTP filtering rules, we may get to the point where switching to a different reverse proxy like HAProxy would be worthwhile. We don't have a ton of logic in the nginx configuration, so the migration should be rather straightforward. Somewhat related, do we have any ops documentation for crates.io? If you want to get more people on the on-call rota, we should have documentation for common tasks. |
Yes, I agree. This is very much a "simplest thing that works for our needs right now".
A little bit, but it's sparse and scattered. Once we start bringing more folks on I'm going to make sure we have a playbook that they can reference |
|
I think you closed this by accident. |
|
Thank you |
|
Looks good to me. |
|
@sgrif I pushed 2 commits to this branch; one adding a bit of docs and one adding a happy path test. If my new commits look good to you, feel free to merge this. |
|
Looks great. Thanks! @bors: r+ |
|
📌 Commit 8b7611b has been approved by |
Allow blocking arbitrary traffic without redeploying This generalizes the `block_ips` module to allow checking against any header instead of just the `X-Real-Ip` header. This does not enable more complex matching such as regexes, but it does let us apply the simple string matching we have today to any header. Additionally, which headers we check against is now configured by an environment variable, which is a comma separated list in the form `Header=VALUE_ENV_VAR`. So to keep the behavior we had today, one would set `BLOCKED_TRAFFIC="X-Real-Ip=BLOCKED_IPS"`. This means that when we see traffic patterns that we need to block which can be handled by the existing logic, we can now begin applying it to patterns we didn't previosuly anticipate without requiring a redeploy
|
☀️ Test successful - checks-travis |
This generalizes the
block_ipsmodule to allow checking against anyheader instead of just the
X-Real-Ipheader. This does not enable morecomplex matching such as regexes, but it does let us apply the simple
string matching we have today to any header.
Additionally, which headers we check against is now configured by an
environment variable, which is a comma separated list in the form
Header=VALUE_ENV_VAR. So to keep the behavior we had today, one wouldset
BLOCKED_TRAFFIC="X-Real-Ip=BLOCKED_IPS".This means that when we see traffic patterns that we need to block which
can be handled by the existing logic, we can now begin applying it to
patterns we didn't previosuly anticipate without requiring a redeploy