From becf124e048bc8f0e7b6227861c0ad25f96c011a Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Thu, 1 Sep 2022 09:09:22 -0400 Subject: [PATCH 1/8] Seam for --validate --- limitador-server/src/main.rs | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/limitador-server/src/main.rs b/limitador-server/src/main.rs index 14dd602e..a259440a 100644 --- a/limitador-server/src/main.rs +++ b/limitador-server/src/main.rs @@ -462,6 +462,12 @@ fn create_config() -> (Configuration, String) { .display_order(6) .help("Sets the level of verbosity"), ) + .arg( + Arg::with_name("validate") + .long("validate") + .display_order(7) + .help("Validates the LIMITS_FILE and exits") + ) .subcommand( SubCommand::with_name("memory") .display_order(1) @@ -552,6 +558,31 @@ fn create_config() -> (Configuration, String) { let matches = cmdline.get_matches(); let limits_file = matches.value_of("LIMITS_FILE").unwrap(); + + if matches.is_present("validate") { + let result = match std::fs::File::open(limits_file) { + Ok(f) => { + let parsed_limits: Result, _> = serde_yaml::from_reader(f); + match parsed_limits { + Ok(limits) => { + Ok(()) + } + Err(e) => Err(LimitadorServerError::ConfigFile(format!( + "Couldn't parse: {}", + e + ))), + } + } + Err(e) => Err(LimitadorServerError::ConfigFile(format!( + "Couldn't read file '{}': {}", + limits_file, + e + ))), + }; + println!("{:?}", result); + process::exit(0); + } + let storage = match matches.subcommand() { Some(("redis", sub)) => StorageConfiguration::Redis(RedisStorageConfiguration { url: sub.value_of("URL").unwrap().to_owned(), From 009119b0e7312de893876c72bc91704c7ed64cf6 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Thu, 1 Sep 2022 10:25:02 -0400 Subject: [PATCH 2/8] actually validate --- limitador-server/src/http_api/mod.rs | 2 ++ limitador-server/src/main.rs | 31 +++++++++++++++++++++------- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/limitador-server/src/http_api/mod.rs b/limitador-server/src/http_api/mod.rs index 5213b416..9bc37ede 100644 --- a/limitador-server/src/http_api/mod.rs +++ b/limitador-server/src/http_api/mod.rs @@ -4,4 +4,6 @@ #[allow(clippy::field_reassign_with_default)] mod request_types; +pub use request_types::Limit as LimitVO; + pub mod server; diff --git a/limitador-server/src/main.rs b/limitador-server/src/main.rs index a259440a..cbe6a898 100644 --- a/limitador-server/src/main.rs +++ b/limitador-server/src/main.rs @@ -560,29 +560,44 @@ fn create_config() -> (Configuration, String) { let limits_file = matches.value_of("LIMITS_FILE").unwrap(); if matches.is_present("validate") { - let result = match std::fs::File::open(limits_file) { + let error = match std::fs::File::open(limits_file) { Ok(f) => { let parsed_limits: Result, _> = serde_yaml::from_reader(f); match parsed_limits { Ok(limits) => { - Ok(()) + if limitador::limit::check_deprecated_syntax_usages_and_reset() { + eprintln!("Deprecated syntax for conditions corrected!\n") + } + + let output: Vec = + limits.iter().map(|l| l.into()).collect(); + match serde_yaml::to_string(&output) { + Ok(cfg) => { + println!("{}", cfg); + } + Err(err) => { + eprintln!("Config file is valid, but can't be output: {}", err); + } + } + process::exit(0); } - Err(e) => Err(LimitadorServerError::ConfigFile(format!( + Err(e) => LimitadorServerError::ConfigFile(format!( "Couldn't parse: {}", e - ))), + )), } } - Err(e) => Err(LimitadorServerError::ConfigFile(format!( + Err(e) => LimitadorServerError::ConfigFile(format!( "Couldn't read file '{}': {}", limits_file, e - ))), + )), }; - println!("{:?}", result); - process::exit(0); + eprintln!("{}", error); + process::exit(1); } + let storage = match matches.subcommand() { Some(("redis", sub)) => StorageConfiguration::Redis(RedisStorageConfiguration { url: sub.value_of("URL").unwrap().to_owned(), From 73e2bd4846a91f15314b538b1acdce88a8182b1b Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Thu, 1 Sep 2022 11:26:18 -0400 Subject: [PATCH 3/8] Hardened Condition.to_string() --- limitador-server/src/main.rs | 11 +++-------- limitador/src/limit.rs | 11 ++++++++--- limitador/src/storage/keys.rs | 2 +- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/limitador-server/src/main.rs b/limitador-server/src/main.rs index cbe6a898..b670f2b7 100644 --- a/limitador-server/src/main.rs +++ b/limitador-server/src/main.rs @@ -466,7 +466,7 @@ fn create_config() -> (Configuration, String) { Arg::with_name("validate") .long("validate") .display_order(7) - .help("Validates the LIMITS_FILE and exits") + .help("Validates the LIMITS_FILE and exits"), ) .subcommand( SubCommand::with_name("memory") @@ -581,23 +581,18 @@ fn create_config() -> (Configuration, String) { } process::exit(0); } - Err(e) => LimitadorServerError::ConfigFile(format!( - "Couldn't parse: {}", - e - )), + Err(e) => LimitadorServerError::ConfigFile(format!("Couldn't parse: {}", e)), } } Err(e) => LimitadorServerError::ConfigFile(format!( "Couldn't read file '{}': {}", - limits_file, - e + limits_file, e )), }; eprintln!("{}", error); process::exit(1); } - let storage = match matches.subcommand() { Some(("redis", sub)) => StorageConfiguration::Redis(RedisStorageConfiguration { url: sub.value_of("URL").unwrap().to_owned(), diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index f3d470ed..f646d6b2 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -239,9 +239,14 @@ impl From for String { fn from(condition: Condition) -> Self { let p = &condition.predicate; let predicate: String = p.clone().into(); + let quotes = if condition.operand.contains('"') { + '\'' + } else { + '"' + }; format!( - "{} {} '{}'", - condition.var_name, predicate, condition.operand + "{} {} {}{}{}", + condition.var_name, predicate, quotes, condition.operand, quotes ) } } @@ -967,6 +972,6 @@ mod tests { operand: "ok".to_string(), }; let result = serde_json::to_string(&condition).expect("Should serialize"); - assert_eq!(result, r#""foobar == 'ok'""#.to_string()); + assert_eq!(result, r#""foobar == \"ok\"""#.to_string()); } } diff --git a/limitador/src/storage/keys.rs b/limitador/src/storage/keys.rs index 1781b30c..c3498d10 100644 --- a/limitador/src/storage/keys.rs +++ b/limitador/src/storage/keys.rs @@ -63,7 +63,7 @@ mod tests { vec!["app_id"], ); assert_eq!( - "namespace:{example.com},counters_of_limit:{\"namespace\":\"example.com\",\"seconds\":60,\"conditions\":[\"req.method == 'GET'\"],\"variables\":[\"app_id\"]}", + "namespace:{example.com},counters_of_limit:{\"namespace\":\"example.com\",\"seconds\":60,\"conditions\":[\"req.method == \\\"GET\\\"\"],\"variables\":[\"app_id\"]}", key_for_counters_of_limit(&limit)) } } From 3167bfc7a9d08cf50e03ea0102a02faf9de5f7a7 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Tue, 6 Sep 2022 15:19:18 -0400 Subject: [PATCH 4/8] Added validation for `Limit.max_value` to be > 0 --- limitador-server/src/main.rs | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/limitador-server/src/main.rs b/limitador-server/src/main.rs index b670f2b7..140bffb7 100644 --- a/limitador-server/src/main.rs +++ b/limitador-server/src/main.rs @@ -205,16 +205,22 @@ impl Limiter { Ok(f) => { let parsed_limits: Result, _> = serde_yaml::from_reader(f); match parsed_limits { - Ok(limits) => { - match &self { - Self::Blocking(limiter) => limiter.configure_with(limits)?, - Self::Async(limiter) => limiter.configure_with(limits).await?, - } - if limitador::limit::check_deprecated_syntax_usages_and_reset() { - error!("You are using deprecated syntax for your conditions! See the migration guide https://kudrant.io/migration/limitador/constraints") + Ok(limits) => match first_zero_or_negative(&limits) { + None => { + match &self { + Self::Blocking(limiter) => limiter.configure_with(limits)?, + Self::Async(limiter) => limiter.configure_with(limits).await?, + } + if limitador::limit::check_deprecated_syntax_usages_and_reset() { + error!("You are using deprecated syntax for your conditions! See the migration guide https://kudrant.io/migration/limitador/constraints") + } + Ok(()) } - Ok(()) - } + Some(index) => Err(LimitadorServerError::ConfigFile(format!( + ".[{}]: invalid value for `max_value`: positive integer expected", + index + ))), + }, Err(e) => Err(LimitadorServerError::ConfigFile(format!( "Couldn't parse: {}", e @@ -230,6 +236,15 @@ impl Limiter { } } +fn first_zero_or_negative(limits: &[Limit]) -> Option { + for (index, limit) in limits.iter().enumerate() { + if limit.max_value() < 1 { + return Some(index); + } + } + None +} + #[actix_rt::main] async fn main() -> Result<(), Box> { let config = { From 29beab8fb8a0fb2d052be3e56f147754cd46ffb2 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Wed, 7 Sep 2022 11:37:03 -0400 Subject: [PATCH 5/8] 0 is used to block all traffic --- limitador-server/src/main.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/limitador-server/src/main.rs b/limitador-server/src/main.rs index 140bffb7..eb3b609f 100644 --- a/limitador-server/src/main.rs +++ b/limitador-server/src/main.rs @@ -205,7 +205,7 @@ impl Limiter { Ok(f) => { let parsed_limits: Result, _> = serde_yaml::from_reader(f); match parsed_limits { - Ok(limits) => match first_zero_or_negative(&limits) { + Ok(limits) => match first_negative(&limits) { None => { match &self { Self::Blocking(limiter) => limiter.configure_with(limits)?, @@ -236,9 +236,9 @@ impl Limiter { } } -fn first_zero_or_negative(limits: &[Limit]) -> Option { +fn first_negative(limits: &[Limit]) -> Option { for (index, limit) in limits.iter().enumerate() { - if limit.max_value() < 1 { + if limit.max_value() < 0 { return Some(index); } } From 1111e6f3608fabce3ef8f09eb36dc190192b19d1 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Thu, 8 Sep 2022 07:27:43 -0400 Subject: [PATCH 6/8] Added test for fn first_negative --- limitador-server/src/main.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/limitador-server/src/main.rs b/limitador-server/src/main.rs index eb3b609f..a517b25f 100644 --- a/limitador-server/src/main.rs +++ b/limitador-server/src/main.rs @@ -716,3 +716,29 @@ fn env_option_is_enabled(env_name: &str) -> bool { Err(_) => false, } } + +#[cfg(test)] +mod tests { + use crate::first_negative; + use limitador::limit::Limit; + + #[test] + fn finds_negative_limits() { + let variables: [&str; 0] = []; + let mut limits: Vec = vec![ + Limit::new::<_, &str>("foo", 42, 10, [], variables), + Limit::new::<_, &str>("foo", -42, 10, [], variables), + ]; + + assert_eq!(first_negative(&limits), Some(1)); + limits[0].set_max_value(-42); + assert_eq!(first_negative(&limits), Some(0)); + limits[1].set_max_value(42); + assert_eq!(first_negative(&limits), Some(0)); + limits[0].set_max_value(42); + assert_eq!(first_negative(&limits), None); + + let nothing: [Limit; 0] = []; + assert_eq!(first_negative(¬hing), None); + } +} From 9b33441ddd991bdbde01c3dedca8dc41c2d66be4 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Thu, 8 Sep 2022 10:25:53 -0400 Subject: [PATCH 7/8] Rename fn --- limitador-server/src/main.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/limitador-server/src/main.rs b/limitador-server/src/main.rs index a517b25f..37dfafcb 100644 --- a/limitador-server/src/main.rs +++ b/limitador-server/src/main.rs @@ -205,7 +205,7 @@ impl Limiter { Ok(f) => { let parsed_limits: Result, _> = serde_yaml::from_reader(f); match parsed_limits { - Ok(limits) => match first_negative(&limits) { + Ok(limits) => match find_first_negative_limit(&limits) { None => { match &self { Self::Blocking(limiter) => limiter.configure_with(limits)?, @@ -236,7 +236,7 @@ impl Limiter { } } -fn first_negative(limits: &[Limit]) -> Option { +fn find_first_negative_limit(limits: &[Limit]) -> Option { for (index, limit) in limits.iter().enumerate() { if limit.max_value() < 0 { return Some(index); @@ -719,7 +719,7 @@ fn env_option_is_enabled(env_name: &str) -> bool { #[cfg(test)] mod tests { - use crate::first_negative; + use crate::find_first_negative_limit; use limitador::limit::Limit; #[test] @@ -730,15 +730,15 @@ mod tests { Limit::new::<_, &str>("foo", -42, 10, [], variables), ]; - assert_eq!(first_negative(&limits), Some(1)); + assert_eq!(find_first_negative_limit(&limits), Some(1)); limits[0].set_max_value(-42); - assert_eq!(first_negative(&limits), Some(0)); + assert_eq!(find_first_negative_limit(&limits), Some(0)); limits[1].set_max_value(42); - assert_eq!(first_negative(&limits), Some(0)); + assert_eq!(find_first_negative_limit(&limits), Some(0)); limits[0].set_max_value(42); - assert_eq!(first_negative(&limits), None); + assert_eq!(find_first_negative_limit(&limits), None); let nothing: [Limit; 0] = []; - assert_eq!(first_negative(¬hing), None); + assert_eq!(find_first_negative_limit(¬hing), None); } } From 7a4ea16662b93a0d5368f944ab29dacd928a6910 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Thu, 8 Sep 2022 11:07:24 -0400 Subject: [PATCH 8/8] Validate for non-negatives too --- limitador-server/src/main.rs | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/limitador-server/src/main.rs b/limitador-server/src/main.rs index 37dfafcb..d4d07f21 100644 --- a/limitador-server/src/main.rs +++ b/limitador-server/src/main.rs @@ -579,23 +579,29 @@ fn create_config() -> (Configuration, String) { Ok(f) => { let parsed_limits: Result, _> = serde_yaml::from_reader(f); match parsed_limits { - Ok(limits) => { - if limitador::limit::check_deprecated_syntax_usages_and_reset() { - eprintln!("Deprecated syntax for conditions corrected!\n") - } - - let output: Vec = - limits.iter().map(|l| l.into()).collect(); - match serde_yaml::to_string(&output) { - Ok(cfg) => { - println!("{}", cfg); + Ok(limits) => match find_first_negative_limit(&limits) { + Some(index) => LimitadorServerError::ConfigFile(format!( + ".[{}]: invalid value for `max_value`: positive integer expected", + index + )), + None => { + if limitador::limit::check_deprecated_syntax_usages_and_reset() { + eprintln!("Deprecated syntax for conditions corrected!\n") } - Err(err) => { - eprintln!("Config file is valid, but can't be output: {}", err); + + let output: Vec = + limits.iter().map(|l| l.into()).collect(); + match serde_yaml::to_string(&output) { + Ok(cfg) => { + println!("{}", cfg); + } + Err(err) => { + eprintln!("Config file is valid, but can't be output: {}", err); + } } + process::exit(0); } - process::exit(0); - } + }, Err(e) => LimitadorServerError::ConfigFile(format!("Couldn't parse: {}", e)), } }