From bb9e4f99ef96dfe9fe214fa6a0e8713c2bddc5c4 Mon Sep 17 00:00:00 2001 From: Bryan Stitt Date: Tue, 1 Nov 2022 19:12:57 +0000 Subject: [PATCH] fix some auth --- entities/src/user_tier.rs | 2 +- migration/src/m20221031_211916_clean_up.rs | 4 ++-- web3_proxy/src/app.rs | 6 +++--- web3_proxy/src/bin/web3_proxy.rs | 4 ++-- .../src/bin/web3_proxy_cli/check_config.rs | 18 +++++++++--------- web3_proxy/src/config.rs | 18 +++++++++--------- web3_proxy/src/frontend/authorization.rs | 17 +++++++++++++---- web3_proxy/src/frontend/users.rs | 5 ++--- web3_proxy/src/user_queries.rs | 17 +++++++++++++---- 9 files changed, 54 insertions(+), 37 deletions(-) diff --git a/entities/src/user_tier.rs b/entities/src/user_tier.rs index 885471dd..c3948b37 100644 --- a/entities/src/user_tier.rs +++ b/entities/src/user_tier.rs @@ -9,7 +9,7 @@ pub struct Model { #[sea_orm(primary_key)] pub id: u64, pub title: String, - pub max_requests_per_minute: Option, + pub max_requests_per_period: Option, pub max_concurrent_requests: Option, } diff --git a/migration/src/m20221031_211916_clean_up.rs b/migration/src/m20221031_211916_clean_up.rs index 65bce367..82a1df7a 100644 --- a/migration/src/m20221031_211916_clean_up.rs +++ b/migration/src/m20221031_211916_clean_up.rs @@ -64,14 +64,14 @@ impl MigrationTrait for Migration { ) .await?; - // on user_tier table, rename requests_per_minute to max_requests_per_minute + // on user_tier table, rename requests_per_minute to max_requests_per_period manager .alter_table( Table::alter() .table(Alias::new("user_tier")) .rename_column( Alias::new("requests_per_minute"), - Alias::new("max_requests_per_minute"), + Alias::new("max_requests_per_period"), ) .to_owned(), ) diff --git a/web3_proxy/src/app.rs b/web3_proxy/src/app.rs index a4ab0beb..fe910874 100644 --- a/web3_proxy/src/app.rs +++ b/web3_proxy/src/app.rs @@ -70,7 +70,7 @@ pub struct UserKeyData { /// if None, allow unlimited queries. inherited from the user_tier pub max_requests_per_period: Option, // if None, allow unlimited concurrent requests. inherited from the user_tier - pub max_concurrent_requests: Option, + pub max_concurrent_requests: Option, /// if None, allow any Origin pub allowed_origins: Option>, /// if None, allow any Referer @@ -399,7 +399,7 @@ impl Web3ProxyApp { // TODO: think about this unwrapping top_config .app - .public_requests_per_minute + .public_requests_per_period .unwrap_or(u64::MAX), 60.0, redis_pool.clone(), @@ -421,7 +421,7 @@ impl Web3ProxyApp { login_rate_limiter = Some(RedisRateLimiter::new( "web3_proxy", "login", - top_config.app.login_rate_limit_per_minute, + top_config.app.login_rate_limit_per_period, 60.0, redis_pool.clone(), )); diff --git a/web3_proxy/src/bin/web3_proxy.rs b/web3_proxy/src/bin/web3_proxy.rs index 471a5d03..d444d403 100644 --- a/web3_proxy/src/bin/web3_proxy.rs +++ b/web3_proxy/src/bin/web3_proxy.rs @@ -267,10 +267,10 @@ mod tests { let app_config = TopConfig { app: AppConfig { chain_id: 31337, - default_user_max_requests_per_minute: Some(6_000_000), + default_user_max_requests_per_period: Some(6_000_000), min_sum_soft_limit: 1, min_synced_rpcs: 1, - public_requests_per_minute: Some(1_000_000), + public_requests_per_period: Some(1_000_000), response_cache_max_bytes: 10_usize.pow(7), redirect_public_url: Some("example.com/".to_string()), redirect_user_url: Some("example.com/{{user_id}}".to_string()), diff --git a/web3_proxy/src/bin/web3_proxy_cli/check_config.rs b/web3_proxy/src/bin/web3_proxy_cli/check_config.rs index 89358b19..b5a43169 100644 --- a/web3_proxy/src/bin/web3_proxy_cli/check_config.rs +++ b/web3_proxy/src/bin/web3_proxy_cli/check_config.rs @@ -25,23 +25,23 @@ impl CheckConfigSubCommand { warn!("app.db_url is not set! Some features disabled") } - match top_config.app.public_requests_per_minute { + match top_config.app.public_requests_per_period { None => { - info!("app.public_requests_per_minute is None. Fully open to public requests!") + info!("app.public_requests_per_period is None. Fully open to public requests!") } Some(0) => { - info!("app.public_requests_per_minute is 0. Public requests are blocked!") + info!("app.public_requests_per_period is 0. Public requests are blocked!") } Some(_) => {} } - match top_config.app.default_user_max_requests_per_minute { + match top_config.app.default_user_max_requests_per_period { None => { - info!("app.default_user_requests_per_minute is None. Fully open to registered requests!") + info!("app.default_user_requests_per_period is None. Fully open to registered requests!") } - Some(0) => warn!("app.default_user_requests_per_minute is 0. Registered user's requests are blocked! Are you sure you want that?"), + Some(0) => warn!("app.default_user_requests_per_period is 0. Registered user's requests are blocked! Are you sure you want that?"), Some(_) => { - // TODO: make sure this isn't < anonymous requests per minute + // TODO: make sure this isn't < anonymous requests per period } } @@ -52,8 +52,8 @@ impl CheckConfigSubCommand { // TODO: check min_sum_soft_limit is a reasonable amount // TODO: check min_synced_rpcs is a reasonable amount - // TODO: check frontend_rate_limit_per_minute is a reasonable amount. requires redis - // TODO: check login_rate_limit_per_minute is a reasonable amount. requires redis + // TODO: check frontend_rate_limit_per_period is a reasonable amount. requires redis + // TODO: check login_rate_limit_per_period is a reasonable amount. requires redis if top_config.app.volatile_redis_url.is_none() { warn!("app.volatile_redis_url is not set! Some features disabled") diff --git a/web3_proxy/src/config.rs b/web3_proxy/src/config.rs index 7ab12fe6..cb6530f8 100644 --- a/web3_proxy/src/config.rs +++ b/web3_proxy/src/config.rs @@ -53,7 +53,7 @@ pub struct TopConfig { pub struct AppConfig { /// Request limit for allowed origins for anonymous users. /// These requests get rate limited by IP. - pub allowed_origin_requests_per_minute: HashMap, + pub allowed_origin_requests_per_period: HashMap, /// EVM chain id. 1 for ETH /// TODO: better type for chain_id? max of `u64::MAX / 2 - 36` https://github.com/ethereum/EIPs/issues/2294 @@ -74,7 +74,7 @@ pub struct AppConfig { /// Default request limit for registered users. /// 0 = block all requests /// None = allow all requests - pub default_user_max_requests_per_minute: Option, + pub default_user_max_requests_per_period: Option, /// Restrict user registration. /// None = no code needed @@ -87,8 +87,8 @@ pub struct AppConfig { /// Rate limit for the login entrypoint. /// This is separate from the rpc limits. - #[serde(default = "default_login_rate_limit_per_minute")] - pub login_rate_limit_per_minute: u64, + #[serde(default = "default_login_rate_limit_per_period")] + pub login_rate_limit_per_period: u64, /// The soft limit prevents thundering herds as new blocks are seen. #[serde(default = "default_min_sum_soft_limit")] @@ -107,8 +107,8 @@ pub struct AppConfig { /// Request limit for anonymous users. /// Some(0) = block all requests /// None = allow all requests - #[serde(default = "default_public_requests_per_minute")] - pub public_requests_per_minute: Option, + #[serde(default = "default_public_requests_per_period")] + pub public_requests_per_period: Option, /// RPC responses are cached locally #[serde(default = "default_response_cache_max_bytes")] @@ -150,7 +150,7 @@ fn default_public_max_concurrent_requests() -> Option { } /// 0 blocks anonymous requests by default. -fn default_public_requests_per_minute() -> Option { +fn default_public_requests_per_period() -> Option { Some(0) } @@ -159,8 +159,8 @@ fn default_bearer_token_max_concurrent_requests() -> u64 { 2 } -/// Having a low amount of requests per minute for login is safest. -fn default_login_rate_limit_per_minute() -> u64 { +/// Having a low amount of requests per period (usually minute) for login is safest. +fn default_login_rate_limit_per_period() -> u64 { 10 } diff --git a/web3_proxy/src/frontend/authorization.rs b/web3_proxy/src/frontend/authorization.rs index d7fda9fb..a65a8d74 100644 --- a/web3_proxy/src/frontend/authorization.rs +++ b/web3_proxy/src/frontend/authorization.rs @@ -464,7 +464,7 @@ impl Web3ProxyApp { let max_requests_per_period = origin .map(|origin| { self.config - .allowed_origin_requests_per_minute + .allowed_origin_requests_per_period .get(&origin.to_string()) .cloned() }) @@ -527,7 +527,16 @@ impl Web3ProxyApp { Some(rpc_key_model) => { // TODO: move these splits into helper functions // TODO: can we have sea orm handle this for us? - // let user_tier_model = rpc_key_model. + let user_model = user::Entity::find_by_id(rpc_key_model.user_id) + .one(&db_conn) + .await? + .expect("related user"); + + let user_tier_model = + user_tier::Entity::find_by_id(user_model.user_tier_id) + .one(&db_conn) + .await? + .expect("related user tier"); let allowed_ips: Option> = if let Some(allowed_ips) = rpc_key_model.allowed_ips { @@ -590,8 +599,8 @@ impl Web3ProxyApp { allowed_referers, allowed_user_agents, log_revert_chance: rpc_key_model.log_revert_chance, - max_concurrent_requests: None, // todo! user_tier_model.max_concurrent_requests, - max_requests_per_period: None, // todo! user_tier_model.max_requests_per_period, + max_concurrent_requests: user_tier_model.max_concurrent_requests, + max_requests_per_period: user_tier_model.max_requests_per_period, }) } None => Ok(UserKeyData::default()), diff --git a/web3_proxy/src/frontend/users.rs b/web3_proxy/src/frontend/users.rs index f01d2768..27f874ad 100644 --- a/web3_proxy/src/frontend/users.rs +++ b/web3_proxy/src/frontend/users.rs @@ -501,13 +501,12 @@ pub struct UserKeyManagement { description: Option, private_txs: Option, active: Option, - // TODO: enable log_revert_trace: Option, + // TODO: enable log_revert_trace: Option, allowed_ips: Option, allowed_origins: Option, allowed_referers: Option, allowed_user_agents: Option, - // do not allow! `requests_per_minute: Option,` - // do not allow! `max_concurrent_requests: Option,` + // do not allow! `user_tier: Option,` } /// `POST /user/keys` or `PUT /user/keys` -- Use a bearer token to create or update an existing key. diff --git a/web3_proxy/src/user_queries.rs b/web3_proxy/src/user_queries.rs index 8e6ab886..1301c580 100644 --- a/web3_proxy/src/user_queries.rs +++ b/web3_proxy/src/user_queries.rs @@ -32,23 +32,32 @@ async fn get_user_id_from_params( let bearer_cache_key = UserBearerToken::try_from(bearer)?.to_string(); // get the user id that is attached to this bearer token - redis_conn + let bearer_user_id = redis_conn .get::<_, u64>(bearer_cache_key) .await // TODO: this should be a 403 - .context("fetching rpc_key_id from redis with bearer_cache_key") + .context("fetching rpc_key_id from redis with bearer_cache_key")?; + + let user_id: u64 = user_id.parse().context("Parsing user_id param")?; + + if bearer_user_id != user_id { + // TODO: proper HTTP Status code + Err(anyhow::anyhow!("permission denied")) + } else { + Ok(bearer_user_id) + } } (_, None) => { // they have a bearer token. we don't care about it on public pages // 0 means all Ok(0) } - (None, Some(x)) => { + (None, Some(_)) => { // they do not have a bearer token, but requested a specific id. block // TODO: proper error code // TODO: maybe instead of this sharp edged warn, we have a config value? // TODO: check config for if we should deny or allow this - x.parse().context("Parsing user_id param") + Err(anyhow::anyhow!("permission denied")) } } }