From 2cf9ad8230ad1490a46654fb324e5d577118ca43 Mon Sep 17 00:00:00 2001 From: Steven Smiley Date: Fri, 13 Sep 2024 21:52:18 -0700 Subject: [PATCH 01/13] feat: add manual cache eviction --- aws_secretsmanager_agent/src/cache_manager.rs | 11 ++- aws_secretsmanager_agent/src/config.rs | 43 +++++++++++- aws_secretsmanager_agent/src/server.rs | 68 +++++++++++-------- aws_secretsmanager_caching/src/lib.rs | 11 +++ 4 files changed, 103 insertions(+), 30 deletions(-) diff --git a/aws_secretsmanager_agent/src/cache_manager.rs b/aws_secretsmanager_agent/src/cache_manager.rs index 85849fc..9f8a169 100644 --- a/aws_secretsmanager_agent/src/cache_manager.rs +++ b/aws_secretsmanager_agent/src/cache_manager.rs @@ -91,8 +91,17 @@ impl CacheManager { } } } -} + pub async fn evict_entry(&self, secret_id: &str) -> Result<(), HttpError> { + match self.0.invalidate(secret_id) { + Ok(_) => Ok(()), + Err(e) => { + error!("Failed to evict secret {}: {:?}", secret_id, e); + Err(HttpError(500, err_response("EvictionError", "Failed to evict secret"))) + } + } + } +} /// Private helper to format in internal service error response. #[doc(hidden)] fn int_err() -> HttpError { diff --git a/aws_secretsmanager_agent/src/config.rs b/aws_secretsmanager_agent/src/config.rs index 9f19f16..6ad5c57 100644 --- a/aws_secretsmanager_agent/src/config.rs +++ b/aws_secretsmanager_agent/src/config.rs @@ -35,6 +35,7 @@ struct ConfigFile { path_prefix: String, max_conn: String, region: Option, + allow_eviction: String, } /// The log levels supported by the daemon. @@ -93,6 +94,8 @@ pub struct Config { /// The AWS Region that will be used to send the Secrets Manager request to. region: Option, + + allow_eviction: bool, } /// The default configuration options. @@ -134,7 +137,8 @@ impl Config { )? .set_default("path_prefix", DEFAULT_PATH_PREFIX)? .set_default("max_conn", DEFAULT_MAX_CONNECTIONS)? - .set_default("region", DEFAULT_REGION)?; + .set_default("region", DEFAULT_REGION)? + .set_default("allow_eviction", "false")?; // Merge the config overrides onto the default configurations, if provided. config = match file_path { @@ -228,6 +232,10 @@ impl Config { self.region.as_ref() } + pub fn allow_eviction(&self) -> bool { + self.allow_eviction + } + /// Private helper that fills in the Config instance from the specified /// config overrides (or defaults). /// @@ -275,6 +283,7 @@ impl Config { None, )?, region: config_file.region, + allow_eviction: parse_bool(&config_file.allow_eviction, "Invalid allow_eviction value")?, }; // Additional validations. @@ -292,6 +301,14 @@ impl Config { } } +fn parse_bool(value: &str, error_msg: &str) -> Result> { + match value.to_lowercase().as_str() { + "true" => Ok(true), + "false" => Ok(false), + _ => Err(error_msg.into()), + } +} + /// Private helper to convert a string to number and perform range checks, returning a custom error on failure. /// /// # Arguments @@ -357,6 +374,7 @@ mod tests { path_prefix: String::from(DEFAULT_PATH_PREFIX), max_conn: String::from(DEFAULT_MAX_CONNECTIONS), region: None, + allow_eviction: String::from("false"), } } @@ -382,6 +400,7 @@ mod tests { assert_eq!(config.clone().path_prefix(), DEFAULT_PATH_PREFIX); assert_eq!(config.clone().max_conn(), 800); assert_eq!(config.clone().region(), None); + assert_eq!(config.allow_eviction(), false); } /// Tests the config overrides are applied correctly from the provided config file. @@ -406,6 +425,7 @@ mod tests { assert_eq!(config.clone().path_prefix(), "/other"); assert_eq!(config.clone().max_conn(), 10); assert_eq!(config.clone().region(), Some(&"us-west-2".to_string())); + assert_eq!(config.allow_eviction(), true); } /// Tests that an Err is returned when an invalid value is provided in one of the configurations. @@ -597,4 +617,25 @@ mod tests { )) .unwrap(); } + + #[test] + fn test_validate_allow_eviction() { + let valid_values = vec!["true", "false", "True", "False", "TRUE", "FALSE"]; + for value in valid_values { + let config = ConfigFile { + allow_eviction: value.to_string(), + ..get_default_config_file() + }; + assert!(Config::build(config).is_ok()); + } + + let invalid_values = vec!["yes", "no", "1", "0", "invalid"]; + for value in invalid_values { + let config = ConfigFile { + allow_eviction: value.to_string(), + ..get_default_config_file() + }; + assert!(Config::build(config).is_err()); + } + } } diff --git a/aws_secretsmanager_agent/src/server.rs b/aws_secretsmanager_agent/src/server.rs index 4c1929a..43d5784 100644 --- a/aws_secretsmanager_agent/src/server.rs +++ b/aws_secretsmanager_agent/src/server.rs @@ -25,18 +25,19 @@ pub struct Server { ssrf_headers: Arc>, path_prefix: Arc, max_conn: usize, + allow_eviction: bool, } /// Handle incoming HTTP requests. /// -/// Implements the HTTP handler. Each incomming request is handled in its own +/// Implements the HTTP handler. Each incoming request is handled in its own /// thread. impl Server { /// Create a server instance. /// /// # Arguments /// - /// * `listener` - The TcpListener to use to accept incomming requests. + /// * `listener` - The TcpListener to use to accept incoming requests. /// * `cfg` - The config object to use for options such header names. /// /// # Returns @@ -54,6 +55,7 @@ impl Server { ssrf_headers: Arc::new(cfg.ssrf_headers()), path_prefix: Arc::new(cfg.path_prefix()), max_conn: cfg.max_conn(), + allow_eviction: cfg.allow_eviction(), }) } @@ -87,11 +89,11 @@ impl Server { Ok(()) } - /// Private helper to process the incomming request body and format a response. + /// Private helper to process the incoming request body and format a response. /// /// # Arguments /// - /// * `req` - The incomming HTTP request. + /// * `req` - The incoming HTTP request. /// * `count` - The number of concurrent requets being handled. /// /// # Returns @@ -118,11 +120,11 @@ impl Server { } } - /// Parse an incomming request and provide the response data. + /// Parse an incoming request and provide the response data. /// /// # Arguments /// - /// * `req` - The incomming HTTP request. + /// * `req` - The incoming HTTP request. /// * `count` - The number of concurrent requets being handled. /// /// # Returns @@ -135,15 +137,13 @@ impl Server { req: &Request, count: usize, ) -> Result { - self.validate_max_conn(req, count)?; // Verify connection limits are not exceeded - self.validate_token(req)?; // Check for a valid SSRF token - self.validate_method(req)?; // Allow only GET requests + self.validate_max_conn(req, count)?; + self.validate_token(req)?; + self.validate_method(req)?; - match req.uri().path() { - "/ping" => Ok("healthy".into()), // Standard health check - - // Lambda extension style query - "/secretsmanager/get" => { + match (req.method(), req.uri().path()) { + (&Method::GET, "/ping") => Ok("healthy".into()), // Standard health check + (&Method::GET, "/secretsmanager/get") => { // Lambda extension style query let qry = GSVQuery::try_from_query(&req.uri().to_string())?; Ok(self .cache_mgr @@ -154,9 +154,8 @@ impl Server { ) .await?) } - // Path style request - path if path.starts_with(self.path_prefix.as_str()) => { + (&Method::GET, path) if path.starts_with(self.path_prefix.as_str()) => { let qry = GSVQuery::try_from_path_query(&req.uri().to_string(), &self.path_prefix)?; Ok(self .cache_mgr @@ -167,17 +166,31 @@ impl Server { ) .await?) } + (&Method::POST, "/evict") => { + if !self.allow_eviction { + return Err(HttpError(403, "Eviction not allowed".into())); + } + self.handle_eviction_request(req).await + } _ => Err(HttpError(404, "Not found".into())), } } - /// Verify the incomming request does not exceed the maximum connection limit. + async fn handle_eviction_request(&self, req: &Request) -> Result { + let body_bytes = hyper::body::to_bytes(req.into_body()).await.map_err(|_| HttpError(400, "Invalid request body".into()))?; + let secret_id = String::from_utf8(body_bytes.to_vec()).map_err(|_| HttpError(400, "Invalid secret ID".into()))?; + + self.cache_mgr.evict_entry(&secret_id).await?; + Ok(format!("Successfully evicted secret: {}", secret_id)) + } + + /// Verify the incoming request does not exceed the maximum connection limit. /// /// The limit is not enforced for ping/health checks. /// /// # Arguments /// - /// * `req` - The incomming HTTP request. + /// * `req` - The incoming HTTP request. /// * `count` - The number of concurrent requets being handled. /// /// # Returns @@ -209,7 +222,7 @@ impl Server { /// /// # Arguments /// - /// * `req` - The incomming HTTP request. + /// * `req` - The incoming HTTP request. /// /// # Returns /// @@ -241,22 +254,21 @@ impl Server { Err(HttpError(403, "Bad Token".into())) } - /// Verify the request is using the GET HTTP verb. + /// Verify the request is using the GET or POST HTTP verb. /// /// # Arguments /// - /// * `req` - The incomming HTTP request. + /// * `req` - The incoming HTTP request. /// /// # Returns /// - /// * `Ok(())` - If the GET verb/method is use. - /// * `Err((u16, String))` - A 405 error codde and message when GET is not used. + /// * `Ok(())` - If the GET or POST verb/method is use. + /// * `Err((u16, String))` - A 405 error codde and message when GET or POST is not used. #[doc(hidden)] fn validate_method(&self, req: &Request) -> Result<(), HttpError> { - if *req.method() == Method::GET { - return Ok(()); + match *req.method() { + Method::GET | Method::POST => Ok(()), + _ => Err(HttpError(405, "Method not allowed".into())), } - - Err(HttpError(405, "Not allowed".into())) } -} +} \ No newline at end of file diff --git a/aws_secretsmanager_caching/src/lib.rs b/aws_secretsmanager_caching/src/lib.rs index c8a5ca6..0d5a5d0 100644 --- a/aws_secretsmanager_caching/src/lib.rs +++ b/aws_secretsmanager_caching/src/lib.rs @@ -296,6 +296,17 @@ impl SecretsManagerCachingClient { Ok(false) } + + /// Invalidates a secret in the cache, forcing a refresh on the next retrieval. + /// + /// # Arguments + /// + /// * `secret_id` - The ARN or name of the secret to invalidate. + pub async fn invalidate(&self, secret_id: &str) -> Result<(), Box> { + let mut write_lock = self.store.write().await; + write_lock.invalidate(secret_id)?; + Ok(()) + } } #[cfg(test)] From 3f3fc3f5043f9f6f0c15b8d906af20777b6177b4 Mon Sep 17 00:00:00 2001 From: Steven Smiley <53946040+StevenSmiley@users.noreply.github.com> Date: Sat, 14 Sep 2024 07:18:23 -0700 Subject: [PATCH 02/13] Change default to allow --- aws_secretsmanager_agent/src/config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws_secretsmanager_agent/src/config.rs b/aws_secretsmanager_agent/src/config.rs index 6ad5c57..dad1116 100644 --- a/aws_secretsmanager_agent/src/config.rs +++ b/aws_secretsmanager_agent/src/config.rs @@ -138,7 +138,7 @@ impl Config { .set_default("path_prefix", DEFAULT_PATH_PREFIX)? .set_default("max_conn", DEFAULT_MAX_CONNECTIONS)? .set_default("region", DEFAULT_REGION)? - .set_default("allow_eviction", "false")?; + .set_default("allow_eviction", "true")?; // Merge the config overrides onto the default configurations, if provided. config = match file_path { From 7010b2fd2eb6556add68f7c5b387da702a310172 Mon Sep 17 00:00:00 2001 From: Steven Smiley Date: Sat, 14 Sep 2024 07:30:02 -0700 Subject: [PATCH 03/13] update tests for default allow --- aws_secretsmanager_agent/src/config.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/aws_secretsmanager_agent/src/config.rs b/aws_secretsmanager_agent/src/config.rs index dad1116..ed6e6fd 100644 --- a/aws_secretsmanager_agent/src/config.rs +++ b/aws_secretsmanager_agent/src/config.rs @@ -374,7 +374,7 @@ mod tests { path_prefix: String::from(DEFAULT_PATH_PREFIX), max_conn: String::from(DEFAULT_MAX_CONNECTIONS), region: None, - allow_eviction: String::from("false"), + allow_eviction: String::from("true"), } } @@ -400,7 +400,7 @@ mod tests { assert_eq!(config.clone().path_prefix(), DEFAULT_PATH_PREFIX); assert_eq!(config.clone().max_conn(), 800); assert_eq!(config.clone().region(), None); - assert_eq!(config.allow_eviction(), false); + assert_eq!(config.allow_eviction(), true); } /// Tests the config overrides are applied correctly from the provided config file. From e161d6b80dfbcfde47c1bdee47941a9b9ed7fbf0 Mon Sep 17 00:00:00 2001 From: Steven Smiley Date: Sat, 14 Sep 2024 08:15:15 -0700 Subject: [PATCH 04/13] feat: implement function to remove value from cache --- aws_secretsmanager_agent/src/cache_manager.rs | 2 +- .../src/secret_store/memory_store/cache.rs | 26 +++++++++++- .../src/secret_store/memory_store/mod.rs | 40 +++++++++++++++++++ .../src/secret_store/mod.rs | 8 ++++ 4 files changed, 74 insertions(+), 2 deletions(-) diff --git a/aws_secretsmanager_agent/src/cache_manager.rs b/aws_secretsmanager_agent/src/cache_manager.rs index 9f8a169..e9ba645 100644 --- a/aws_secretsmanager_agent/src/cache_manager.rs +++ b/aws_secretsmanager_agent/src/cache_manager.rs @@ -93,7 +93,7 @@ impl CacheManager { } pub async fn evict_entry(&self, secret_id: &str) -> Result<(), HttpError> { - match self.0.invalidate(secret_id) { + match self.0.remove_secret_value(secret_id) { Ok(_) => Ok(()), Err(e) => { error!("Failed to evict secret {}: {:?}", secret_id, e); diff --git a/aws_secretsmanager_caching/src/secret_store/memory_store/cache.rs b/aws_secretsmanager_caching/src/secret_store/memory_store/cache.rs index 02b4bea..99299f6 100644 --- a/aws_secretsmanager_caching/src/secret_store/memory_store/cache.rs +++ b/aws_secretsmanager_caching/src/secret_store/memory_store/cache.rs @@ -48,8 +48,17 @@ impl Cache { { self.entries.get(key) } -} + /// Removes a key-value pair from the cache. + /// Returns the value if the key was present in the cache, None otherwise. + pub fn remove(&mut self, key: &Q) -> Option + where + Q: ?Sized + Hash + Eq, + K: Borrow, + { + self.entries.remove(key) + } +} #[cfg(test)] mod tests { use super::*; @@ -113,4 +122,19 @@ mod tests { let items: Vec = cache.entries.iter().map(|t| (*t.1)).collect(); assert_eq!(items, [2]); } + + #[test] + fn remove_evicts_entry() { + let mut cache = TestIntCache::new(NonZeroUsize::new(4).unwrap()); + + cache.insert("test1".to_string(), 1); + cache.insert("test2".to_string(), 2); + + assert_eq!(cache.remove("test1"), Some(1)); + assert_eq!(cache.len(), 1); + assert_eq!(cache.get("test1"), None); + assert_eq!(cache.get("test2"), Some(&2)); + + assert_eq!(cache.remove("non_existent"), None); + } } diff --git a/aws_secretsmanager_caching/src/secret_store/memory_store/mod.rs b/aws_secretsmanager_caching/src/secret_store/memory_store/mod.rs index df405dd..9c7630a 100644 --- a/aws_secretsmanager_caching/src/secret_store/memory_store/mod.rs +++ b/aws_secretsmanager_caching/src/secret_store/memory_store/mod.rs @@ -94,6 +94,21 @@ impl SecretStore for MemoryStore { Ok(()) } + + fn remove_secret_value( + &mut self, + secret_id: &str, + version_id: Option<&str>, + version_stage: Option<&str>, + ) -> Result<(), SecretStoreError> { + let key = Key { + secret_id: secret_id.to_string(), + version_id: version_id.map(String::from), + version_stage: version_stage.map(String::from), + }; + self.gsv_cache.remove(&key); + Ok(()) + } } /// Write the secret value to the store @@ -275,4 +290,29 @@ mod tests { Err(e) => panic!("Unexpected error: {}", e), } } + + #[test] + fn memory_store_remove_secret_value() { + let mut store = MemoryStore::default(); + + store_secret(&mut store, None, None, None); + + // Verify the secret exists + assert!(store.get_secret_value(NAME, None, None).is_ok()); + + // Remove the secret + assert!(store.remove_secret_value(NAME, None, None).is_ok()); + + // Verify the secret no longer exists + match store.get_secret_value(NAME, None, None) { + Err(SecretStoreError::ResourceNotFound) => (), + _ => panic!("Expected ResourceNotFound error"), + } + + // Attempt to remove a non-existent secret + match store.remove_secret_value("non_existent", None, None) { + Err(SecretStoreError::ResourceNotFound) => (), + _ => panic!("Expected ResourceNotFound error"), + } + } } diff --git a/aws_secretsmanager_caching/src/secret_store/mod.rs b/aws_secretsmanager_caching/src/secret_store/mod.rs index f566727..d7af003 100644 --- a/aws_secretsmanager_caching/src/secret_store/mod.rs +++ b/aws_secretsmanager_caching/src/secret_store/mod.rs @@ -29,6 +29,14 @@ pub trait SecretStore: Debug + Send + Sync { version_stage: Option, data: GetSecretValueOutputDef, ) -> Result<(), SecretStoreError>; + + /// Remove the secret value from the store + fn remove_secret_value( + &mut self, + secret_id: &str, + version_id: Option<&str>, + version_stage: Option<&str>, + ) -> Result<(), SecretStoreError>; } /// All possible error types From 755b09998268b876846aed52b48f6df5b381390c Mon Sep 17 00:00:00 2001 From: Steven Smiley Date: Sat, 14 Sep 2024 08:18:45 -0700 Subject: [PATCH 05/13] feat: use new remove function --- aws_secretsmanager_caching/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws_secretsmanager_caching/src/lib.rs b/aws_secretsmanager_caching/src/lib.rs index 0d5a5d0..8392ccf 100644 --- a/aws_secretsmanager_caching/src/lib.rs +++ b/aws_secretsmanager_caching/src/lib.rs @@ -304,7 +304,7 @@ impl SecretsManagerCachingClient { /// * `secret_id` - The ARN or name of the secret to invalidate. pub async fn invalidate(&self, secret_id: &str) -> Result<(), Box> { let mut write_lock = self.store.write().await; - write_lock.invalidate(secret_id)?; + write_lock.remove_secret_value(secret_id)?; Ok(()) } } From 1c8da861e4bad2382d5e732550f85579cc32c085 Mon Sep 17 00:00:00 2001 From: Steven Smiley Date: Sat, 14 Sep 2024 11:23:02 -0700 Subject: [PATCH 06/13] fix: support version and stage for removing secret value --- aws_secretsmanager_agent/src/cache_manager.rs | 11 ++++++++--- aws_secretsmanager_agent/src/server.rs | 17 +++++++++-------- aws_secretsmanager_caching/src/lib.rs | 15 +++++++++++---- 3 files changed, 28 insertions(+), 15 deletions(-) diff --git a/aws_secretsmanager_agent/src/cache_manager.rs b/aws_secretsmanager_agent/src/cache_manager.rs index e9ba645..5b482c2 100644 --- a/aws_secretsmanager_agent/src/cache_manager.rs +++ b/aws_secretsmanager_agent/src/cache_manager.rs @@ -92,9 +92,14 @@ impl CacheManager { } } - pub async fn evict_entry(&self, secret_id: &str) -> Result<(), HttpError> { - match self.0.remove_secret_value(secret_id) { - Ok(_) => Ok(()), + pub async fn evict_entry( + &self, + secret_id: &str, + version: Option<&str>, + label: Option<&str> + ) -> Result { + match self.0.remove_secret_value(secret_id, version, label).await { + Ok(_) => Ok("Secret successfully evicted".to_string()), Err(e) => { error!("Failed to evict secret {}: {:?}", secret_id, e); Err(HttpError(500, err_response("EvictionError", "Failed to evict secret"))) diff --git a/aws_secretsmanager_agent/src/server.rs b/aws_secretsmanager_agent/src/server.rs index 43d5784..766ca59 100644 --- a/aws_secretsmanager_agent/src/server.rs +++ b/aws_secretsmanager_agent/src/server.rs @@ -170,19 +170,20 @@ impl Server { if !self.allow_eviction { return Err(HttpError(403, "Eviction not allowed".into())); } - self.handle_eviction_request(req).await + let qry = GSVQuery::try_from_query(&req.uri().to_string())?; + Ok(self + .cache_mgr + .evict_entry( + &qry.secret_id, + qry.version_id.as_deref(), + qry.version_stage.as_deref(), + ) + .await?) } _ => Err(HttpError(404, "Not found".into())), } } - async fn handle_eviction_request(&self, req: &Request) -> Result { - let body_bytes = hyper::body::to_bytes(req.into_body()).await.map_err(|_| HttpError(400, "Invalid request body".into()))?; - let secret_id = String::from_utf8(body_bytes.to_vec()).map_err(|_| HttpError(400, "Invalid secret ID".into()))?; - - self.cache_mgr.evict_entry(&secret_id).await?; - Ok(format!("Successfully evicted secret: {}", secret_id)) - } /// Verify the incoming request does not exceed the maximum connection limit. /// diff --git a/aws_secretsmanager_caching/src/lib.rs b/aws_secretsmanager_caching/src/lib.rs index 8392ccf..b1b1807 100644 --- a/aws_secretsmanager_caching/src/lib.rs +++ b/aws_secretsmanager_caching/src/lib.rs @@ -297,14 +297,21 @@ impl SecretsManagerCachingClient { Ok(false) } - /// Invalidates a secret in the cache, forcing a refresh on the next retrieval. + /// Removes a secret in the cache, forcing a refresh on the next retrieval. /// /// # Arguments /// - /// * `secret_id` - The ARN or name of the secret to invalidate. - pub async fn invalidate(&self, secret_id: &str) -> Result<(), Box> { + /// * `secret_id` - The ARN or name of the secret to remove. + /// * `version_id` - The version id of the secret version to remove. + /// * `version_stage` - The staging label of the version of the secret to remove. + pub async fn remove_secret_value( + &self, + secret_id: &str, + version_id: Option<&str>, + version_stage: Option<&str>, + ) -> Result<(), Box> { let mut write_lock = self.store.write().await; - write_lock.remove_secret_value(secret_id)?; + write_lock.remove_secret_value(secret_id, version_id, version_stage)?; Ok(()) } } From d002ecdf34d74e0403adf2da131bc814aa622dbe Mon Sep 17 00:00:00 2001 From: Steven Smiley Date: Sat, 14 Sep 2024 11:39:32 -0700 Subject: [PATCH 07/13] remove allow_eviction config, not needed --- aws_secretsmanager_agent/src/config.rs | 43 +------------------------- aws_secretsmanager_agent/src/server.rs | 5 --- 2 files changed, 1 insertion(+), 47 deletions(-) diff --git a/aws_secretsmanager_agent/src/config.rs b/aws_secretsmanager_agent/src/config.rs index ed6e6fd..9f19f16 100644 --- a/aws_secretsmanager_agent/src/config.rs +++ b/aws_secretsmanager_agent/src/config.rs @@ -35,7 +35,6 @@ struct ConfigFile { path_prefix: String, max_conn: String, region: Option, - allow_eviction: String, } /// The log levels supported by the daemon. @@ -94,8 +93,6 @@ pub struct Config { /// The AWS Region that will be used to send the Secrets Manager request to. region: Option, - - allow_eviction: bool, } /// The default configuration options. @@ -137,8 +134,7 @@ impl Config { )? .set_default("path_prefix", DEFAULT_PATH_PREFIX)? .set_default("max_conn", DEFAULT_MAX_CONNECTIONS)? - .set_default("region", DEFAULT_REGION)? - .set_default("allow_eviction", "true")?; + .set_default("region", DEFAULT_REGION)?; // Merge the config overrides onto the default configurations, if provided. config = match file_path { @@ -232,10 +228,6 @@ impl Config { self.region.as_ref() } - pub fn allow_eviction(&self) -> bool { - self.allow_eviction - } - /// Private helper that fills in the Config instance from the specified /// config overrides (or defaults). /// @@ -283,7 +275,6 @@ impl Config { None, )?, region: config_file.region, - allow_eviction: parse_bool(&config_file.allow_eviction, "Invalid allow_eviction value")?, }; // Additional validations. @@ -301,14 +292,6 @@ impl Config { } } -fn parse_bool(value: &str, error_msg: &str) -> Result> { - match value.to_lowercase().as_str() { - "true" => Ok(true), - "false" => Ok(false), - _ => Err(error_msg.into()), - } -} - /// Private helper to convert a string to number and perform range checks, returning a custom error on failure. /// /// # Arguments @@ -374,7 +357,6 @@ mod tests { path_prefix: String::from(DEFAULT_PATH_PREFIX), max_conn: String::from(DEFAULT_MAX_CONNECTIONS), region: None, - allow_eviction: String::from("true"), } } @@ -400,7 +382,6 @@ mod tests { assert_eq!(config.clone().path_prefix(), DEFAULT_PATH_PREFIX); assert_eq!(config.clone().max_conn(), 800); assert_eq!(config.clone().region(), None); - assert_eq!(config.allow_eviction(), true); } /// Tests the config overrides are applied correctly from the provided config file. @@ -425,7 +406,6 @@ mod tests { assert_eq!(config.clone().path_prefix(), "/other"); assert_eq!(config.clone().max_conn(), 10); assert_eq!(config.clone().region(), Some(&"us-west-2".to_string())); - assert_eq!(config.allow_eviction(), true); } /// Tests that an Err is returned when an invalid value is provided in one of the configurations. @@ -617,25 +597,4 @@ mod tests { )) .unwrap(); } - - #[test] - fn test_validate_allow_eviction() { - let valid_values = vec!["true", "false", "True", "False", "TRUE", "FALSE"]; - for value in valid_values { - let config = ConfigFile { - allow_eviction: value.to_string(), - ..get_default_config_file() - }; - assert!(Config::build(config).is_ok()); - } - - let invalid_values = vec!["yes", "no", "1", "0", "invalid"]; - for value in invalid_values { - let config = ConfigFile { - allow_eviction: value.to_string(), - ..get_default_config_file() - }; - assert!(Config::build(config).is_err()); - } - } } diff --git a/aws_secretsmanager_agent/src/server.rs b/aws_secretsmanager_agent/src/server.rs index 766ca59..881881f 100644 --- a/aws_secretsmanager_agent/src/server.rs +++ b/aws_secretsmanager_agent/src/server.rs @@ -25,7 +25,6 @@ pub struct Server { ssrf_headers: Arc>, path_prefix: Arc, max_conn: usize, - allow_eviction: bool, } /// Handle incoming HTTP requests. @@ -55,7 +54,6 @@ impl Server { ssrf_headers: Arc::new(cfg.ssrf_headers()), path_prefix: Arc::new(cfg.path_prefix()), max_conn: cfg.max_conn(), - allow_eviction: cfg.allow_eviction(), }) } @@ -167,9 +165,6 @@ impl Server { .await?) } (&Method::POST, "/evict") => { - if !self.allow_eviction { - return Err(HttpError(403, "Eviction not allowed".into())); - } let qry = GSVQuery::try_from_query(&req.uri().to_string())?; Ok(self .cache_mgr From 624d2acb050a16358c658b1e6e52a2668f756fdf Mon Sep 17 00:00:00 2001 From: Steven Smiley Date: Sat, 14 Sep 2024 11:42:59 -0700 Subject: [PATCH 08/13] revert unintentionally removed comments --- aws_secretsmanager_agent/src/server.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/aws_secretsmanager_agent/src/server.rs b/aws_secretsmanager_agent/src/server.rs index 881881f..0838775 100644 --- a/aws_secretsmanager_agent/src/server.rs +++ b/aws_secretsmanager_agent/src/server.rs @@ -135,9 +135,9 @@ impl Server { req: &Request, count: usize, ) -> Result { - self.validate_max_conn(req, count)?; - self.validate_token(req)?; - self.validate_method(req)?; + self.validate_max_conn(req, count)?; // Verify connection limits are not exceeded + self.validate_token(req)?; // Check for a valid SSRF token + self.validate_method(req)?; // Allow only GET requests match (req.method(), req.uri().path()) { (&Method::GET, "/ping") => Ok("healthy".into()), // Standard health check From 871d82b97d61714cd31689810983c3fa34fdfa1a Mon Sep 17 00:00:00 2001 From: Steven Smiley Date: Sat, 14 Sep 2024 11:53:43 -0700 Subject: [PATCH 09/13] fix: throw error when trying to remove secret that doesn't exist --- .../src/secret_store/memory_store/mod.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/aws_secretsmanager_caching/src/secret_store/memory_store/mod.rs b/aws_secretsmanager_caching/src/secret_store/memory_store/mod.rs index 9c7630a..8748601 100644 --- a/aws_secretsmanager_caching/src/secret_store/memory_store/mod.rs +++ b/aws_secretsmanager_caching/src/secret_store/memory_store/mod.rs @@ -106,9 +106,12 @@ impl SecretStore for MemoryStore { version_id: version_id.map(String::from), version_stage: version_stage.map(String::from), }; - self.gsv_cache.remove(&key); - Ok(()) - } + if self.gsv_cache.remove(&key).is_none() { + Err(SecretStoreError::ResourceNotFound) + } else { + Ok(()) + } + } } /// Write the secret value to the store From 7c8e5fd2355ac6a174bb6a2797a6301f69b45035 Mon Sep 17 00:00:00 2001 From: Steven Smiley Date: Sat, 14 Sep 2024 11:58:41 -0700 Subject: [PATCH 10/13] update verb test to allow POST --- aws_secretsmanager_agent/src/main.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/aws_secretsmanager_agent/src/main.rs b/aws_secretsmanager_agent/src/main.rs index 429a250..891917b 100644 --- a/aws_secretsmanager_agent/src/main.rs +++ b/aws_secretsmanager_agent/src/main.rs @@ -842,9 +842,9 @@ mod tests { // Verify requests using the wrong verbs fail with 405. #[tokio::test] - async fn get_only() { + async fn get_and_post_only() { for verb in [ - "POST", "PUT", "PATCH", "DELETE", "HEAD", "CONNECT", "OPTIONS", "TRACE", + "PUT", "PATCH", "DELETE", "HEAD", "CONNECT", "OPTIONS", "TRACE", ] { let (status, _) = run_requests_with_verb(vec![(verb, "/secretsmanager/get?secretId=MyTest")]) From 5762f0c0077ff7fcc35bd75786c54f344930e166 Mon Sep 17 00:00:00 2001 From: Steven Smiley Date: Sat, 14 Sep 2024 12:00:36 -0700 Subject: [PATCH 11/13] fix comment --- aws_secretsmanager_agent/src/server.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws_secretsmanager_agent/src/server.rs b/aws_secretsmanager_agent/src/server.rs index 0838775..3081725 100644 --- a/aws_secretsmanager_agent/src/server.rs +++ b/aws_secretsmanager_agent/src/server.rs @@ -137,7 +137,7 @@ impl Server { ) -> Result { self.validate_max_conn(req, count)?; // Verify connection limits are not exceeded self.validate_token(req)?; // Check for a valid SSRF token - self.validate_method(req)?; // Allow only GET requests + self.validate_method(req)?; // Allow only GET and POST requests match (req.method(), req.uri().path()) { (&Method::GET, "/ping") => Ok("healthy".into()), // Standard health check From 345a36721e757d8b9e634b2f701fad5672057ba3 Mon Sep 17 00:00:00 2001 From: Steven Smiley Date: Sat, 14 Sep 2024 12:05:17 -0700 Subject: [PATCH 12/13] prepend secretsmanager to eviction path --- aws_secretsmanager_agent/src/server.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws_secretsmanager_agent/src/server.rs b/aws_secretsmanager_agent/src/server.rs index 3081725..00890cd 100644 --- a/aws_secretsmanager_agent/src/server.rs +++ b/aws_secretsmanager_agent/src/server.rs @@ -164,7 +164,7 @@ impl Server { ) .await?) } - (&Method::POST, "/evict") => { + (&Method::POST, "/secretsmanager/evict") => { let qry = GSVQuery::try_from_query(&req.uri().to_string())?; Ok(self .cache_mgr From 86d222d8771a03ac295a4daf68600ec51e21031d Mon Sep 17 00:00:00 2001 From: Steven Smiley Date: Tue, 8 Oct 2024 08:47:33 -0700 Subject: [PATCH 13/13] refactor: rename fn and add comments --- aws_secretsmanager_agent/src/cache_manager.rs | 25 ++++++++++++++++++- aws_secretsmanager_agent/src/server.rs | 2 +- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/aws_secretsmanager_agent/src/cache_manager.rs b/aws_secretsmanager_agent/src/cache_manager.rs index 5b482c2..0dfeaa2 100644 --- a/aws_secretsmanager_agent/src/cache_manager.rs +++ b/aws_secretsmanager_agent/src/cache_manager.rs @@ -92,7 +92,30 @@ impl CacheManager { } } - pub async fn evict_entry( + /// Evict a secret from the cache. + /// + /// # Arguments + /// + /// * `secret_id` - The name of the secret to evict. + /// * `version` - The version of the secret to evict. + /// * `label` - The label of the secret to evict. + /// + /// # Returns + /// + /// * `Ok(String)` - Success message. + /// * `Err((u16, String))` - The error code and message. + /// + /// # Errors + /// + /// * `HttpError` - The error returned from the SDK. + /// + /// # Example + /// + /// ``` + /// let cache_manager = CacheManager::new().await.unwrap(); + /// let message = cache_manager.evict("my-secret", None, None).unwrap(); + /// ``` + pub async fn evict( &self, secret_id: &str, version: Option<&str>, diff --git a/aws_secretsmanager_agent/src/server.rs b/aws_secretsmanager_agent/src/server.rs index 00890cd..9dc45ec 100644 --- a/aws_secretsmanager_agent/src/server.rs +++ b/aws_secretsmanager_agent/src/server.rs @@ -168,7 +168,7 @@ impl Server { let qry = GSVQuery::try_from_query(&req.uri().to_string())?; Ok(self .cache_mgr - .evict_entry( + .evict( &qry.secret_id, qry.version_id.as_deref(), qry.version_stage.as_deref(),