Auto merge of #3858 - rust-lang:revert-3790-pa-rate-limit-updates, r=pietroalbini

Revert "Move new versions rate limits to the application"

Reverts #3790

This has been causing issues in production 😢

see https://sentry.io/organizations/rust-lang/issues/2594868566/

Author: bors

config/nginx.conf.erb

@@ -176,6 +176,8 @@ http {
 	client_body_timeout 30;
 	client_max_body_size 50m;
 
+	limit_req_zone $remote_addr zone=publish:10m rate=1r/m;
+
 	upstream app_server {
 		server localhost:8888 fail_timeout=0;
 	}
@@ -259,5 +261,12 @@ http {
 				proxy_pass http://app_server;
 			}
 		<% end %>
+
+		location ~ ^/api/v./crates/new$ {
+			proxy_pass http://app_server;
+
+			limit_req zone=publish burst=30 nodelay;
+			limit_req_status 429;
+		}
 	}
 }

migrations/2021-07-18-125813_add_rate_limit_action/down.sql

@@ -8,7 +8,6 @@ use crate::downloads_counter::DownloadsCounter;
 use crate::email::Emails;
 use crate::github::GitHubClient;
 use crate::metrics::{InstanceMetrics, ServiceMetrics};
-use crate::rate_limiter::RateLimiter;
 use diesel::r2d2;
 use oauth2::basic::BasicClient;
 use reqwest::blocking::Client;
@@ -44,8 +43,6 @@ pub struct App {
     /// Metrics related to this specific instance of the service
     pub instance_metrics: InstanceMetrics,
 
-    pub rate_limiter: RateLimiter,
-
     /// A configured client for outgoing HTTP requests
     ///
     /// In production this shares a single connection pool across requests.  In tests
@@ -168,7 +165,6 @@ impl App {
             read_only_replica_database: replica_database,
             github,
             github_oauth,
-            rate_limiter: RateLimiter::new(config.rate_limiter.clone()),
             config,
             downloads_counter: DownloadsCounter::new(),
             emails: Emails::from_environment(),

migrations/2021-07-18-125813_add_rate_limit_action/up.sql

@@ -1,7 +1,5 @@
-use crate::rate_limiter::{LimitedAction, RateLimiterConfig};
+use crate::publish_rate_limit::PublishRateLimit;
 use crate::{env, env_optional, uploaders::Uploader, Env};
-use std::collections::HashMap;
-use std::time::Duration;
 
 mod base;
 mod database_pools;
@@ -18,6 +16,7 @@ pub struct Server {
     pub gh_base_url: String,
     pub max_upload_size: u64,
     pub max_unpack_size: u64,
+    pub publish_rate_limit: PublishRateLimit,
     pub blocked_traffic: Vec<(String, Vec<String>)>,
     pub max_allowed_page_offset: u32,
     pub page_offset_ua_blocklist: Vec<String>,
@@ -28,7 +27,6 @@ pub struct Server {
     pub metrics_authorization_token: Option<String>,
     pub use_test_database_pool: bool,
     pub instance_metrics_log_every_seconds: Option<u64>,
-    pub rate_limiter: HashMap<LimitedAction, RateLimiterConfig>,
 }
 
 impl Default for Server {
@@ -66,34 +64,12 @@ impl Default for Server {
             .split(',')
             .map(ToString::to_string)
             .collect();
-
         let page_offset_ua_blocklist = match env_optional::<String>("WEB_PAGE_OFFSET_UA_BLOCKLIST")
         {
             None => vec![],
             Some(s) if s.is_empty() => vec![],
             Some(s) => s.split(',').map(String::from).collect(),
         };
-
-        // Dynamically load the configuration for all the rate limiting actions. See
-        // `src/rate_limiter.rs` for their definition.
-        let mut rate_limiter = HashMap::new();
-        for action in LimitedAction::VARIANTS {
-            rate_limiter.insert(
-                *action,
-                RateLimiterConfig {
-                    rate: Duration::from_secs(
-                        env_optional(&format!(
-                            "RATE_LIMITER_{}_RATE_SECONDS",
-                            action.env_var_key()
-                        ))
-                        .unwrap_or_else(|| action.default_rate_seconds()),
-                    ),
-                    burst: env_optional(&format!("RATE_LIMITER_{}_BURST", action.env_var_key()))
-                        .unwrap_or_else(|| action.default_burst()),
-                },
-            );
-        }
-
         Server {
             db: DatabasePools::full_from_environment(),
             base: Base::from_environment(),
@@ -103,6 +79,7 @@ impl Default for Server {
             gh_base_url: "https://api.github.com".to_string(),
             max_upload_size: 10 * 1024 * 1024, // 10 MB default file upload size limit
             max_unpack_size: 512 * 1024 * 1024, // 512 MB max when decompressed
+            publish_rate_limit: Default::default(),
             blocked_traffic: blocked_traffic(),
             max_allowed_page_offset: env_optional("WEB_MAX_ALLOWED_PAGE_OFFSET").unwrap_or(200),
             page_offset_ua_blocklist,
@@ -119,7 +96,6 @@ impl Default for Server {
             metrics_authorization_token: dotenv::var("METRICS_AUTHORIZATION_TOKEN").ok(),
             use_test_database_pool: false,
             instance_metrics_log_every_seconds: env_optional("INSTANCE_METRICS_LOG_EVERY_SECONDS"),
-            rate_limiter,
         }
     }
 }

src/app.rs

@@ -11,7 +11,6 @@ use crate::models::{
     NewVersion, Rights, VersionAction,
 };
 
-use crate::rate_limiter::LimitedAction;
 use crate::render;
 use crate::schema::*;
 use crate::util::errors::{cargo_err, AppResult};
@@ -73,16 +72,6 @@ pub fn publish(req: &mut dyn RequestExt) -> EndpointResult {
         ))
     })?;
 
-    // Check the rate limits on the endpoint. Publishing a new crate uses a different (stricter)
-    // limiting pool than publishing a new version of an existing crate to prevent mass squatting.
-    let limited_action = if Crate::by_name(&new_crate.name).execute(&*conn)? == 0 {
-        LimitedAction::PublishNew
-    } else {
-        LimitedAction::PublishExisting
-    };
-    app.rate_limiter
-        .check_rate_limit(user.id, limited_action, &conn)?;
-
     // Create a transaction on the database, if there are no errors,
     // commit the transactions to record a new or updated crate.
     conn.transaction(|| {
@@ -118,7 +107,8 @@ pub fn publish(req: &mut dyn RequestExt) -> EndpointResult {
         };
 
         let license_file = new_crate.license_file.as_deref();
-        let krate = persist.create_or_update(&conn, user.id)?;
+        let krate =
+            persist.create_or_update(&conn, user.id, Some(&app.config.publish_rate_limit))?;
 
         let owners = krate.owners(&conn)?;
         if user.rights(req.app(), &owners)? < Rights::Publish {

src/config.rs

@@ -438,7 +438,7 @@ mod tests {
                 name: "foo",
                 ..NewCrate::default()
             }
-            .create_or_update(conn, user.id)
+            .create_or_update(conn, user.id, None)
             .expect("failed to create crate");
 
             Self {

src/controllers/krate/publish.rs

@@ -44,13 +44,12 @@ pub mod git;
 pub mod github;
 pub mod metrics;
 pub mod middleware;
-pub mod rate_limiter;
+mod publish_rate_limit;
 pub mod render;
 pub mod schema;
 pub mod tasks;
 mod test_util;
 pub mod uploaders;
-#[macro_use]
 pub mod util;
 
 pub mod controllers;

src/downloads_counter.rs

@@ -1,14 +1,23 @@
-use crate::models::{ApiToken, User, Version};
-use crate::schema::*;
 use chrono::NaiveDateTime;
 use diesel::prelude::*;
+use diesel::{
+    deserialize::{self, FromSql},
+    pg::Pg,
+    serialize::{self, Output, ToSql},
+    sql_types::Integer,
+};
+use std::io::Write;
 
-pg_enum! {
-    pub enum VersionAction {
-        Publish = 0,
-        Yank = 1,
-        Unyank = 2,
-    }
+use crate::models::{ApiToken, User, Version};
+use crate::schema::*;
+
+#[derive(Debug, Clone, Copy, PartialEq, FromSqlRow, AsExpression)]
+#[repr(i32)]
+#[sql_type = "Integer"]
+pub enum VersionAction {
+    Publish = 0,
+    Yank = 1,
+    Unyank = 2,
 }
 
 impl From<VersionAction> for &'static str {
@@ -29,6 +38,23 @@ impl From<VersionAction> for String {
     }
 }
 
+impl FromSql<Integer, Pg> for VersionAction {
+    fn from_sql(bytes: Option<&[u8]>) -> deserialize::Result<Self> {
+        match <i32 as FromSql<Integer, Pg>>::from_sql(bytes)? {
+            0 => Ok(VersionAction::Publish),
+            1 => Ok(VersionAction::Yank),
+            2 => Ok(VersionAction::Unyank),
+            n => Err(format!("unknown version action: {}", n).into()),
+        }
+    }
+}
+
+impl ToSql<Integer, Pg> for VersionAction {
+    fn to_sql<W: Write>(&self, out: &mut Output<'_, W, Pg>) -> serialize::Result {
+        ToSql::<Integer, Pg>::to_sql(&(*self as i32), out)
+    }
+}
+
 #[derive(Debug, Clone, Copy, Queryable, Identifiable, Associations)]
 #[belongs_to(Version)]
 #[belongs_to(User, foreign_key = "user_id")]

src/lib.rs

@@ -1,3 +1,7 @@
+use diesel::deserialize::{self, FromSql};
+use diesel::pg::Pg;
+use diesel::sql_types::Integer;
+
 use crate::models::{Crate, Version};
 use crate::schema::*;
 
@@ -28,10 +32,23 @@ pub struct ReverseDependency {
     pub name: String,
 }
 
-pg_enum! {
-    pub enum DependencyKind {
-        Normal = 0,
-        Build = 1,
-        Dev = 2,
+#[derive(Copy, Clone, Serialize, Deserialize, Debug, FromSqlRow)]
+#[serde(rename_all = "lowercase")]
+#[repr(u32)]
+pub enum DependencyKind {
+    Normal = 0,
+    Build = 1,
+    Dev = 2,
+    // if you add a kind here, be sure to update `from_row` below.
+}
+
+impl FromSql<Integer, Pg> for DependencyKind {
+    fn from_sql(bytes: Option<&[u8]>) -> deserialize::Result<Self> {
+        match <i32 as FromSql<Integer, Pg>>::from_sql(bytes)? {
+            0 => Ok(DependencyKind::Normal),
+            1 => Ok(DependencyKind::Build),
+            2 => Ok(DependencyKind::Dev),
+            n => Err(format!("unknown kind: {}", n).into()),
+        }
     }
 }

src/models/action.rs

@@ -15,6 +15,7 @@ use crate::models::{
 use crate::util::errors::{cargo_err, AppResult};
 
 use crate::models::helpers::with_count::*;
+use crate::publish_rate_limit::PublishRateLimit;
 use crate::schema::*;
 
 #[derive(Debug, Queryable, Identifiable, Associations, Clone, Copy)]
@@ -92,7 +93,12 @@ pub struct NewCrate<'a> {
 }
 
 impl<'a> NewCrate<'a> {
-    pub fn create_or_update(self, conn: &PgConnection, uploader: i32) -> AppResult<Crate> {
+    pub fn create_or_update(
+        self,
+        conn: &PgConnection,
+        uploader: i32,
+        rate_limit: Option<&PublishRateLimit>,
+    ) -> AppResult<Crate> {
         use diesel::update;
 
         self.validate()?;
@@ -102,6 +108,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);
             }