rpc: add option to whitelist ips in rate limiting
#3701
Conversation
niklasad1
changed the title
niklasad1
changed the title
whitelist hosts in rate limiting
| // X-Forwarded-For: 203.0.113.195,198.51.100.178 | ||
| if let Some(ips) = req.headers().get(&X_FORWARDED_FOR).and_then(|v| v.to_str().ok()) { | ||
| if let Some(proxy_ip) = ips.split_once(',').and_then(|(v, _)| IpAddr::from_str(v).ok()) { | ||
| // NOTE: we assume that ip addr is global |
There was a problem hiding this comment.
checking if the ip address is global is a PITA (would be nice with https://doc.rust-lang.org/std/net/struct.Ipv4Addr.html#method.is_global stabilized) but shouldn't be an issue in practice.
Enabling rate-limiting for a local rpc server doesn't make sense to me :)
|
Thank you, @niklasad1. It seems we had different interpretations of "whitelisted hosts". I was expecting to see a filter to check "origins", similar to Given that we have an IP-based filter, can we rename the flag from: to:
|
|
Ah right, checking the |
|
I'm a bit unsure about this now because it's easy for someone to just set the Thus, I think we need another CLI configuration that the node operation can set whether to trust certain proxies. If the node is not not behind a load balancer/reverse proxy then it doesn't make sense to trust |
Most proxies are set to drop X-Forwarded-For or X-Forwarded-Host by default. If a client tries to send My use case was a little bit different. I was planning to have two domains: |
niklasad1
changed the title
whitelist hosts in rate limitingwhitelist hosts in rate limiting
|
Got it but my only concern with the current code is that if a public node enables It would trivial for a client to just set So, I still think I want another flag for that to make it explicit but probably most likely that the server will be behind reverse proxy when rate-limiting is enabled. Lemme think about it a little more and see whether I can come up with something reasonable |
|
@niklasad1 do you have any update?
You can fool the server but not the proxy. If node is behind proxy (and it is always behind proxy since https certs are not supported) the |
oops, forgot about it, I'll take another iteration tomorrow |
niklasad1
changed the title
whitelist hosts in rate limitingwhitelist ips in rate limiting
Yes, my point was that if one doesn't run the rpc node behind a reverse proxy that could be dangerous to trust the proxy headers because they could be spoofed. I have now added another flag Are you okay with these changes @BulatSaif? |
There was a problem hiding this comment.
Codewise, this looks good to me so long as whitelisting based on IP is acceptable!
I do agree that the rpc_rate_limit_trust_proxy_headers flag is necessary; this prevents users who spin up nodes that are not in front of a load balancer from accidentally falling foul of malicious actors who realise they can pass those flags to bypass rate limiting :)
Sorry, I was ooo last week, just saw this. Yes, it is a good idea to add a flag to disable/enable trust to headers. I will test the PR on Monday and provide feedback. |
|
Why are we adding all this stuff deeply into the node? What again was the excuse for this? Why isn't this all just being done by some proxy in front of the RPC port? |
I agree that ideally this should be done by a proxy but we decided to add a simple rpc-rate-limit middleware into the node because it's a tricky because the most proxies works on the HTTP layer. Thus, when the connection is upgraded to websocket connection the rate-limit etc from HTTP proxy doesn't restrict each RPC call/WebSocket message. Thus, this PR just adds another CLI to disable such middleware for certain peers. I'm aware that https://github.com/AcalaNetwork/subway would work but I don't know how stable it's but would definitely be great to remove this from the node if possible. |
|
we are using Subway for almost a year for Acala production RPC service and it is pretty stable and it will be more stable if more people are using it. it is very unlikely for it to be less stable compare to related features implemented here |
Then we should start trying it. |
|
@niklasad1, I tested it locally. Is it possible to accept IP addresses with a mask (10.0.0.0/8) or how to set a range of IPs like The remaining functions are working as expected. Thank you |
I have changed it now and it requires the IP address to be in correct CIDR notation to support ranges good catch |
This PR adds two new CLI options to disable rate limiting for certain ip addresses and whether to trust "proxy header". After going back in forth I decided to use ip addr instead host because we don't want rely on the host header which can be spoofed but another solution is to resolve the ip addr from the socket to host name. Example: ```bash $ polkadot --rpc-rate-limit 10 --rpc-rate-limit-whitelisted-ips 127.0.0.1/8 --rpc-rate-limit-trust-proxy-headers ``` The ip addr is read from the HTTP proxy headers `Forwarded`, `X-Forwarded-For` `X-Real-IP` if `--rpc-rate-limit-trust-proxy-headers` is enabled if that is not enabled or the headers are not found then the ip address is read from the socket. //cc @BulatSaif can you test this and give some feedback on it?
This PR adds two new CLI options to disable rate limiting for certain ip addresses and whether to trust "proxy header". After going back in forth I decided to use ip addr instead host because we don't want rely on the host header which can be spoofed but another solution is to resolve the ip addr from the socket to host name. Example: ```bash $ polkadot --rpc-rate-limit 10 --rpc-rate-limit-whitelisted-ips 127.0.0.1/8 --rpc-rate-limit-trust-proxy-headers ``` The ip addr is read from the HTTP proxy headers `Forwarded`, `X-Forwarded-For` `X-Real-IP` if `--rpc-rate-limit-trust-proxy-headers` is enabled if that is not enabled or the headers are not found then the ip address is read from the socket. //cc @BulatSaif can you test this and give some feedback on it?
This PR adds two new CLI options to disable rate limiting for certain ip addresses and whether to trust "proxy header".
After going back in forth I decided to use ip addr instead host because we don't want rely on the host header which can be spoofed but another solution is to resolve the ip addr from the socket to host name.
Example:
The ip addr is read from the HTTP proxy headers
Forwarded,X-Forwarded-ForX-Real-IPif--rpc-rate-limit-trust-proxy-headersis enabled if that is not enabled or the headers are not found then the ip address is read from the socket.//cc @BulatSaif can you test this and give some feedback on it?