Skip to content
This repository has been archived by the owner on Feb 27, 2024. It is now read-only.

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

Open
kolbma opened this issue Jan 31, 2021 · 4 comments
Open

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

kolbma opened this issue Jan 31, 2021 · 4 comments
Assignees
Labels

Comments

@kolbma
Copy link

kolbma commented Jan 31, 2021

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
Copy link

Svjard commented Feb 6, 2021

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
Copy link
Author

kolbma commented Feb 8, 2021

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
Copy link
Owner

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
Copy link
Author

kolbma commented Feb 10, 2021

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants