Skip to content

Commit

Permalink
Add more aggressive rate limiting for publishing new crates
Browse files Browse the repository at this point in the history
I think the limit we'll probably set to start is 1 req/10s with a burst
of 30. The error message will tell folks they can either wait for {time
until next token} or email us to get the limit increased for them. This
is limited per user instead of per ip since rotating your user is harder
than rotating your IP. It's stored in the DB since this is only for
publishing new crates, which is slow enough already that the DB load of
rate limiting there shouldn't matter.

I needed to update to Rust 1.33 to get `Duration::as_millis` (note: the
way we're using this feature causes UB if the rate limit is slower than
1 request per 292471208 years. I assume this is not a problem) I needed
to update to Diesel 1.4.2 to get a fix for
diesel-rs/diesel#2017

The algorithm used is pretty much the standard token bucket algorithm.
It's *slightly* different in how we set `tokens = max(0, tokens - 1) +
tokens_to_add` instead of `tokens = max(0, tokens_to_add + 1)`. This is
because the usual implementation checks available tokens before
subtracting them (and thus never persists if there aren't enough tokens
available). Since we're doing this in a single query, and we can *only*
return the final, persisted value, we have to change the calculation
slightly to make sure that a user who is out of tokens gets `1` back
after the rate limit.

A side effect of all of this is that our token count is actually offset
by 1. 0 means the user is not only out of tokens, but that we just tried
to take a token and couldn't. 1 means an empty bucket, and a full bucket
would technically be burst + 1. The alternative would be -1 meaning the
user is actually out of tokens, but since we only ever refill the bucket
when we're trying to take a token, we never actually persist a full
bucket. I figured a range of 0...burst made more sense than -1..burst.
  • Loading branch information
sgrif committed May 30, 2019
1 parent e8130f3 commit 676efab
Show file tree
Hide file tree
Showing 24 changed files with 1,079 additions and 83 deletions.
2 changes: 1 addition & 1 deletion RustConfig
Original file line number Diff line number Diff line change
@@ -1 +1 @@
VERSION=1.32.0
VERSION=1.33.0
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
DROP TABLE publish_limit_buckets;
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
CREATE TABLE publish_limit_buckets(
user_id INTEGER PRIMARY KEY NOT NULL REFERENCES users,
tokens INTEGER NOT NULL,
last_refill TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP
);
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
DROP TABLE publish_rate_overrides;
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
CREATE TABLE publish_rate_overrides (
user_id INTEGER PRIMARY KEY REFERENCES users,
burst INTEGER NOT NULL
);
2 changes: 1 addition & 1 deletion src/bin/update-downloads.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ mod test {
name: "foo",
..Default::default()
}
.create_or_update(conn, None, user_id)
.create_or_update(conn, None, user_id, None)
.unwrap();
let version = NewVersion::new(
krate.id,
Expand Down
3 changes: 3 additions & 0 deletions src/config.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::publish_rate_limit::PublishRateLimit;
use crate::{env, uploaders::Uploader, Env, Replica};
use std::path::PathBuf;
use url::Url;
Expand All @@ -16,6 +17,7 @@ pub struct Config {
pub max_unpack_size: u64,
pub mirror: Replica,
pub api_protocol: String,
pub publish_rate_limit: PublishRateLimit,
}

impl Default for Config {
Expand Down Expand Up @@ -132,6 +134,7 @@ impl Default for Config {
max_unpack_size: 512 * 1024 * 1024, // 512 MB max when decompressed
mirror,
api_protocol,
publish_rate_limit: Default::default(),
}
}
}
7 changes: 6 additions & 1 deletion src/controllers/krate/publish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,12 @@ pub fn publish(req: &mut dyn Request) -> CargoResult<Response> {
};

let license_file = new_crate.license_file.as_ref().map(|s| &**s);
let krate = persist.create_or_update(&conn, license_file, user.id)?;
let krate = persist.create_or_update(
&conn,
license_file,
user.id,
Some(&app.config.publish_rate_limit),
)?;

let owners = krate.owners(&conn)?;
if user.rights(req.app(), &owners)? < Rights::Publish {
Expand Down
2 changes: 2 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,10 @@ pub mod email;
pub mod git;
pub mod github;
pub mod middleware;
mod publish_rate_limit;
pub mod render;
pub mod schema;
mod test_util;
pub mod uploaders;
pub mod util;

Expand Down
5 changes: 2 additions & 3 deletions src/models/category.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,12 +183,11 @@ impl<'a> NewCategory<'a> {
#[cfg(test)]
mod tests {
use super::*;
use crate::test_util::pg_connection_no_transaction;
use diesel::connection::SimpleConnection;

fn pg_connection() -> PgConnection {
let database_url =
dotenv::var("TEST_DATABASE_URL").expect("TEST_DATABASE_URL must be set to run tests");
let conn = PgConnection::establish(&database_url).unwrap();
let conn = pg_connection_no_transaction();
// These tests deadlock if run concurrently
conn.batch_execute("BEGIN; LOCK categories IN ACCESS EXCLUSIVE MODE")
.unwrap();
Expand Down
5 changes: 5 additions & 0 deletions src/models/krate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use crate::models::{
use crate::views::{EncodableCrate, EncodableCrateLinks};

use crate::models::helpers::with_count::*;
use crate::publish_rate_limit::PublishRateLimit;
use crate::schema::*;

/// Hosts in this list are known to not be hosting documentation,
Expand Down Expand Up @@ -105,6 +106,7 @@ impl<'a> NewCrate<'a> {
conn: &PgConnection,
license_file: Option<&'a str>,
uploader: i32,
rate_limit: Option<&PublishRateLimit>,
) -> CargoResult<Crate> {
use diesel::update;

Expand All @@ -115,6 +117,9 @@ impl<'a> NewCrate<'a> {
// To avoid race conditions, we try to insert
// first so we know whether to add an owner
if let Some(krate) = self.save_new_crate(conn, uploader)? {
if let Some(rate_limit) = rate_limit {
rate_limit.check_rate_limit(uploader, conn)?;
}
return Ok(krate);
}

Expand Down
2 changes: 1 addition & 1 deletion src/models/user.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ pub struct User {
pub gh_id: i32,
}

#[derive(Insertable, Debug)]
#[derive(Insertable, Debug, Default)]
#[table_name = "users"]
pub struct NewUser<'a> {
pub gh_id: i32,
Expand Down
Loading

0 comments on commit 676efab

Please sign in to comment.