TerminalWitchcraft / actix-ratelimit

Rate limiter framework for Actix web
MIT License
127 stars 25 forks source link

No ratelimit is forced (no HTTP 429 Error code) #15

Open kolbma opened 3 years ago

kolbma commented 3 years ago

I use JMeter for a lot of local threaded requests and the ratelimiter detects only a single connection although there are 100 hundred requests in a second.
I've also used a loop to get requests for some more seconds. No change.
I've checked this with wireshark and there are really 100 hundred new TCP connections.

Every response looks the same:

HTTP/1.1 200 OK
content-length: 244
connection: close
x-ratelimit-remaining: 9
content-type: application/json
x-ratelimit-reset: 60
x-ratelimit-limit: 10
date: Sun, 31 Jan 2021 02:17:50 GMT

My code looks like this

pub async fn main_server(bind: &'static str) -> std::io::Result<()> {
    println!("Starting server {}...", bind);

    env_logger::Builder::from_env(env_logger::Env::default().default_filter_or("info")).init();

    let store = MemoryStore::new();

    HttpServer::new(move || {
        App::new()
            .wrap(Logger::default())
            .wrap(
                RateLimiter::new(MemoryStoreActor::from(store.clone()).start())
                    .with_interval(Duration::from_secs(60))
                    .with_max_requests(10),
            )
            .wrap(Compress::default())
            .service(
                web::scope("/api/v1")
                    .guard(guard::All(guard::Get()).and(guard::Header(
                        header::CONTENT_TYPE.as_str(),
                        mime::APPLICATION_JSON.essence_str(),
                    )))
                    .service(srv_query),
            )
    })
    .bind(bind)?
    .run()
    .await
}

Looks like there is always executed the else-condition here: https://docs.rs/actix-ratelimit/0.3.1/src/actix_ratelimit/middleware.rs.html#220

Svjard commented 3 years ago

Nuance of running locally with actix. remote_addr() is returning the local peer socket so it sees "127.0.0.1:xxxx" where the xxxx will be changing on each connection. Try adjusting your code to add an identifier for comparison:

.wrap(
    RateLimiter::new(MemoryStoreActor::from(store.clone()).start())
        .with_interval(Duration::from_secs(60))
        .with_max_requests(10)
        .with_identifier(|req| {
            let c = req.connection_info().clone();
            let ip_parts: Vec<&str> = c.remote_addr().unwrap().split(":").collect();
            Ok(ip_parts[0].to_string())  // should now be "127.0.0.1"
        }),
    )
kolbma commented 3 years ago

Then there should be used realip_remote_addr. Or the rate limiter sees any reverse proxy as the client and there is the opposite effect that you'll block the service simply by a concurrent number of requests.
I've switched to a different crate, which seems to handle this for me.

TerminalWitchcraft commented 3 years ago

realip_remote_addr and remote_addr yield the same results, no matter if its deployed locally or remote(I deployed the example in cloud and cross checked for myself). The security drawback listed on docs.rs for realip_remote_addr is something that I didn't fully understand, hence I went with remote_addr as a safe option. The port definitely seems an issue. I'm thinking of stripping the port part for the default behavior in the next release as demonstrated by @Svjard . In any case, users still have access to the full req object.

Thanks for pointing this out.

kolbma commented 3 years ago

I'd like to explain the difference in a few words.
It is not uncommon that you don't put your webservice/application in the first row to clients. It is often the case that there is a reverse proxy or an api gateway. In these situations the remote ip is the local net ip address of these gates and not the real client ip. In the proxy protocol there is an http header extension where the proxy puts in the origin source ip of the client. Also cascades of proxying is possible. This header is the place which is used by real-remote-addr if available. But if there is no proxy in between which sets this header trustworthy for the application, an evil client can set the x-forwarded-for header by itself to any address and fools the application.

So I understand, the support of proxying might be a feature and it is ok to "workaround" only the actix "bug". I'm pretty sure it is not intended that actix returns a port on remote-addr in some situations. There is a peer-addr-method documented to get ports.