Integrate dynamic secret injection with component runtime #577
Conversation
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR implements dynamic secret injection for Wassette components, enabling secrets to be provided to running components via IPC without requiring server restart. The implementation adds an in-memory secret store that takes precedence over file-based secrets, integrates with the IPC server for runtime management, and enhances error messages to guide users when secrets are missing.
Key Changes
- In-memory secret storage using
SecretStringfor secure handling with zeroization - Secret precedence chain: environment variables > IPC-injected secrets > file-based secrets > policy defaults
- IPC commands route to new secret injection methods with fallback logic for deletion
- Enhanced error messages detect missing environment variables and suggest CLI commands
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
tests/secret_injection_integration_test.rs | Comprehensive integration tests for secret injection, precedence, removal, listing, and component isolation |
crates/wassette/src/wasistate.rs | Updated comments to clarify environment variable precedence over secrets |
crates/wassette/src/secrets.rs | Implements in-memory secret store with inject_secret(), remove_memory_secret(), get_all_secrets(), and list_all_secrets() |
crates/wassette/src/policy_internal.rs | Updated to use get_all_secrets() for merged file and memory secrets in template building and policy operations |
crates/wassette/src/lib.rs | Added regex-based error detection for missing environment variables with enhanced error messages |
crates/wassette/src/ipc_server.rs | Routes IPC commands to memory-based operations with fallback to file-based for deletion |
crates/wassette/src/ipc_protocol.rs | Added #[serde(default)] to show_values field for backward compatibility |
crates/wassette/Cargo.toml | Added secrecy, zeroize, and regex dependencies |
Cargo.lock | Lock file updates for new dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for pattern in &patterns { | ||
| if let Ok(re) = regex::Regex::new(pattern) { | ||
| for cap in re.captures_iter(error_message) { | ||
| if let Some(var_name) = cap.get(1) { | ||
| missing_vars.push(var_name.as_str().to_string()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The regex compilation is inside the loop, which means each pattern is compiled multiple times for each error message. This is inefficient and could impact performance if many errors are processed.
Consider using lazy_static or once_cell to compile the regexes once and reuse them:
use once_cell::sync::Lazy; static ENV_VAR_PATTERNS: Lazy<Vec<regex::Regex>> = Lazy::new(|| { vec![ regex::Regex::new(r"environment variable not found:\s*([A-Z_][A-Z0-9_]*)").unwrap(), regex::Regex::new(r"missing environment variable\s*([A-Z_][A-Z0-9_]*)").unwrap(), // ... ] });Alternatively, the error handling if let Ok(re) silently ignores regex compilation errors, which could hide bugs. Since these are static patterns, they should be validated at compile time or initialization.
| if let Some(secret) = component_secrets.remove(key) { | ||
| // Zeroize the secret before dropping | ||
| let mut exposed = secret.expose_secret().to_string(); | ||
| exposed.zeroize(); | ||
| info!( | ||
| "Removed memory secret '{}' for component: {}", | ||
| key, component_id | ||
| ); | ||
| Ok(()) |
There was a problem hiding this comment.
Security concern: The zeroization logic has a flaw. Creating a new owned String from expose_secret() means the original secret data in SecretString is not zeroized - only the copy is. The SecretString itself should handle zeroization when dropped.
This manual zeroization is unnecessary and misleading. The correct approach is to simply drop the SecretString, which will automatically handle zeroization internally (as it implements Drop with zeroization):
pub async fn remove_memory_secret(&self, component_id: &str, key: &str) -> Result<()> { let mut store = self.memory_store.write().await; if let Some(component_secrets) = store.get_mut(component_id) { if component_secrets.remove(key).is_some() { // SecretString handles zeroization automatically when dropped info!( "Removed memory secret '{}' for component: {}", key, component_id ); Ok(()) } else { Err(anyhow!( "Memory secret '{}' not found for component: {}", key, component_id )) } } else { Err(anyhow!( "No memory secrets found for component: {}", component_id )) } }| // Try to delete from memory store first | ||
| match secrets_manager | ||
| .remove_memory_secret(&component_id, &key) | ||
| .await | ||
| .context("Failed to delete secret")?; | ||
| Ok(IpcResponse::success(format!( | ||
| "Secret '{}' deleted from component '{}'", | ||
| key, component_id | ||
| ))) | ||
| { | ||
| Ok(_) => Ok(IpcResponse::success(format!( | ||
| "Secret '{}' deleted from component '{}'", | ||
| key, component_id | ||
| ))), | ||
| Err(_) => { | ||
| // If not in memory, try file-based | ||
| secrets_manager | ||
| .delete_component_secrets(&component_id, std::slice::from_ref(&key)) | ||
| .await | ||
| .context("Failed to delete secret")?; | ||
| Ok(IpcResponse::success(format!( | ||
| "Secret '{}' deleted from component '{}'", | ||
| key, component_id | ||
| ))) | ||
| } | ||
| } |
There was a problem hiding this comment.
[nitpick] Silent error swallowing: When remove_memory_secret fails (line 174), the error is silently ignored (the Err(_) pattern on line 181 doesn't log or report why it failed). This makes debugging difficult when a user tries to delete a secret and it's unclear whether it wasn't found in memory or there was another issue.
Consider logging the error before falling back:
match secrets_manager .remove_memory_secret(&component_id, &key) .await { Ok(_) => Ok(IpcResponse::success(format!( "Secret '{}' deleted from component '{}'", key, component_id ))), Err(memory_err) => { // Log that memory delete failed, trying file-based debug!( "Memory secret not found, trying file-based: {}", memory_err ); secrets_manager .delete_component_secrets(&component_id, std::slice::from_ref(&key)) .await .context("Failed to delete secret")?; Ok(IpcResponse::success(format!( "Secret '{}' deleted from component '{}'", key, component_id ))) } }| /// Inject a secret into in-memory store (dynamic secret injection via IPC) | ||
| pub async fn inject_secret( | ||
| &self, | ||
| component_id: &str, | ||
| key: String, | ||
| value: String, | ||
| ) -> Result<()> { | ||
| let secret_value = SecretString::new(value.into()); | ||
| let mut store = self.memory_store.write().await; | ||
| | ||
| let component_secrets = store | ||
| .entry(component_id.to_string()) | ||
| .or_insert_with(HashMap::new); | ||
| component_secrets.insert(key.clone(), secret_value); | ||
| | ||
| info!("Injected secret '{}' for component: {}", key, component_id); | ||
| Ok(()) | ||
| } | ||
| | ||
| /// Remove a secret from in-memory store | ||
| pub async fn remove_memory_secret(&self, component_id: &str, key: &str) -> Result<()> { | ||
| let mut store = self.memory_store.write().await; | ||
| | ||
| if let Some(component_secrets) = store.get_mut(component_id) { | ||
| if let Some(secret) = component_secrets.remove(key) { | ||
| // Zeroize the secret before dropping | ||
| let mut exposed = secret.expose_secret().to_string(); | ||
| exposed.zeroize(); | ||
| info!( | ||
| "Removed memory secret '{}' for component: {}", | ||
| key, component_id | ||
| ); | ||
| Ok(()) | ||
| } else { | ||
| Err(anyhow!( | ||
| "Memory secret '{}' not found for component: {}", | ||
| key, | ||
| component_id | ||
| )) | ||
| } | ||
| } else { | ||
| Err(anyhow!( | ||
| "No memory secrets found for component: {}", | ||
| component_id | ||
| )) | ||
| } | ||
| } | ||
| | ||
| /// Get all secrets for a component (merged from file and memory) | ||
| /// Returns a combined map with memory secrets taking precedence over file-based secrets | ||
| pub async fn get_all_secrets(&self, component_id: &str) -> Result<HashMap<String, String>> { | ||
| let mut merged = HashMap::new(); | ||
| | ||
| // Start with file-based secrets (lower precedence) | ||
| let file_secrets = self.load_component_secrets(component_id).await?; | ||
| merged.extend(file_secrets); | ||
| | ||
| // Overlay memory secrets (higher precedence) | ||
| let store = self.memory_store.read().await; | ||
| if let Some(memory_secrets) = store.get(component_id) { | ||
| for (key, secret_value) in memory_secrets { | ||
| merged.insert(key.clone(), secret_value.expose_secret().to_string()); | ||
| } | ||
| } | ||
| | ||
| Ok(merged) | ||
| } | ||
| | ||
| /// List all secrets for a component (both file-based and memory) | ||
| pub async fn list_all_secrets( | ||
| &self, | ||
| component_id: &str, | ||
| show_values: bool, | ||
| ) -> Result<HashMap<String, Option<String>>> { | ||
| let mut result = HashMap::new(); | ||
| | ||
| // Add file-based secrets | ||
| let file_secrets = self | ||
| .list_component_secrets(component_id, show_values) | ||
| .await?; | ||
| result.extend(file_secrets); | ||
| | ||
| // Add memory secrets | ||
| let store = self.memory_store.read().await; | ||
| if let Some(memory_secrets) = store.get(component_id) { | ||
| for (key, secret_value) in memory_secrets { | ||
| if show_values { | ||
| result.insert(key.clone(), Some(secret_value.expose_secret().to_string())); | ||
| } else { | ||
| result.insert(key.clone(), None); | ||
| } | ||
| } | ||
| } | ||
| | ||
| Ok(result) | ||
| } |
There was a problem hiding this comment.
[nitpick] Missing documentation: The new functions inject_secret, remove_memory_secret, get_all_secrets, and list_all_secrets lack comprehensive documentation. While they have basic doc comments, they should document:
- Security characteristics: That secrets are stored in memory and lost on restart
- Precedence behavior: That memory secrets override file-based secrets
- Concurrency guarantees: That operations are atomic with respect to each other
- Error conditions: When each error type is returned
Example for inject_secret:
/// Inject a secret into the in-memory store (dynamic secret injection via IPC). /// /// Memory secrets are: /// - Stored only in RAM and lost on server restart /// - Take precedence over file-based secrets with the same key /// - Immediately available to components without reload /// - Zeroized when removed or on drop /// /// # Arguments /// * `component_id` - The component to associate the secret with /// * `key` - The secret's environment variable name /// * `value` - The secret value (will be wrapped in SecretString) /// /// # Thread Safety /// This operation is atomic - concurrent calls will not corrupt the store. | // WASI error: "environment variable not found: API_KEY" | ||
| r"environment variable not found:\s*([A-Z_][A-Z0-9_]*)", | ||
| // Generic: "missing environment variable API_KEY" | ||
| r"missing environment variable\s*([A-Z_][A-Z0-9_]*)", | ||
| // env::var error: "environment variable `API_KEY` not found" | ||
| r"environment variable `([A-Z_][A-Z0-9_]*)` not found", | ||
| // Other variations | ||
| r"variable\s+([A-Z_][A-Z0-9_]*)\s+not\s+found", |
There was a problem hiding this comment.
The regex patterns are overly restrictive. They only match environment variables that start with uppercase letters and contain only uppercase letters, numbers, and underscores (e.g., API_KEY). This will miss environment variables with lowercase letters or mixed case (e.g., Path, HOME, myVar), which are valid in many contexts.
Consider using a more flexible pattern like:
r"environment variable not found:\s*([A-Za-z_][A-Za-z0-9_]*)"Or if you specifically want to detect only uppercase variables (as suggested by the naming convention), add a comment explaining this decision.
| // WASI error: "environment variable not found: API_KEY" | |
| r"environment variable not found:\s*([A-Z_][A-Z0-9_]*)", | |
| // Generic: "missing environment variable API_KEY" | |
| r"missing environment variable\s*([A-Z_][A-Z0-9_]*)", | |
| // env::var error: "environment variable `API_KEY` not found" | |
| r"environment variable `([A-Z_][A-Z0-9_]*)` not found", | |
| // Other variations | |
| r"variable\s+([A-Z_][A-Z0-9_]*)\s+not\s+found", | |
| // WASI error: "environment variable not found: API_KEY" (now matches any valid env var name) | |
| r"environment variable not found:\s*([A-Za-z_][A-Za-z0-9_]*)", | |
| // Generic: "missing environment variable API_KEY" | |
| r"missing environment variable\s*([A-Za-z_][A-Za-z0-9_]*)", | |
| // env::var error: "environment variable `API_KEY` not found" | |
| r"environment variable `([A-Za-z_][A-Za-z0-9_]*)` not found", | |
| // Other variations | |
| r"variable\s+([A-Za-z_][A-Za-z0-9_]*)\s+not\s+found", |
| // Add file-based secrets | ||
| let file_secrets = self | ||
| .list_component_secrets(component_id, show_values) | ||
| .await?; | ||
| result.extend(file_secrets); | ||
| | ||
| // Add memory secrets | ||
| let store = self.memory_store.read().await; | ||
| if let Some(memory_secrets) = store.get(component_id) { | ||
| for (key, secret_value) in memory_secrets { | ||
| if show_values { | ||
| result.insert(key.clone(), Some(secret_value.expose_secret().to_string())); | ||
| } else { | ||
| result.insert(key.clone(), None); | ||
| } |
There was a problem hiding this comment.
[nitpick] Potential issue with precedence in listing: When a secret key exists in both file-based and memory stores, the list_all_secrets function will add the file-based secret first (line 429), then overwrite it with the memory secret (line 436). While this produces the correct result (memory secrets take precedence), it's less efficient and less clear than necessary.
Consider checking for duplicates to avoid unnecessary work:
// Add file-based secrets let file_secrets = self .list_component_secrets(component_id, show_values) .await?; result.extend(file_secrets); // Add/override with memory secrets (higher precedence) let store = self.memory_store.read().await; if let Some(memory_secrets) = store.get(component_id) { for (key, secret_value) in memory_secrets { let value = if show_values { Some(secret_value.expose_secret().to_string()) } else { None }; result.insert(key.clone(), value); // Explicitly shows override behavior } }This makes the precedence behavior more explicit and matches the pattern used in get_all_secrets.
| // Add file-based secrets | |
| let file_secrets = self | |
| .list_component_secrets(component_id, show_values) | |
| .await?; | |
| result.extend(file_secrets); | |
| // Add memory secrets | |
| let store = self.memory_store.read().await; | |
| if let Some(memory_secrets) = store.get(component_id) { | |
| for (key, secret_value) in memory_secrets { | |
| if show_values { | |
| result.insert(key.clone(), Some(secret_value.expose_secret().to_string())); | |
| } else { | |
| result.insert(key.clone(), None); | |
| } | |
| // Add file-based secrets (lower precedence) | |
| let file_secrets = self | |
| .list_component_secrets(component_id, show_values) | |
| .await?; | |
| result.extend(file_secrets); | |
| // Overlay memory secrets (higher precedence) | |
| let store = self.memory_store.read().await; | |
| if let Some(memory_secrets) = store.get(component_id) { | |
| for (key, secret_value) in memory_secrets { | |
| let value = if show_values { | |
| Some(secret_value.expose_secret().to_string()) | |
| } else { | |
| None | |
| }; | |
| result.insert(key.clone(), value); // Explicitly shows override behavior |
| /// Get all secrets for a component (merged from file and memory) | ||
| /// Returns a combined map with memory secrets taking precedence over file-based secrets | ||
| pub async fn get_all_secrets(&self, component_id: &str) -> Result<HashMap<String, String>> { | ||
| let mut merged = HashMap::new(); | ||
| | ||
| // Start with file-based secrets (lower precedence) | ||
| let file_secrets = self.load_component_secrets(component_id).await?; | ||
| merged.extend(file_secrets); | ||
| | ||
| // Overlay memory secrets (higher precedence) | ||
| let store = self.memory_store.read().await; | ||
| if let Some(memory_secrets) = store.get(component_id) { | ||
| for (key, secret_value) in memory_secrets { | ||
| merged.insert(key.clone(), secret_value.expose_secret().to_string()); | ||
| } | ||
| } | ||
| | ||
| Ok(merged) | ||
| } | ||
| | ||
| /// List all secrets for a component (both file-based and memory) | ||
| pub async fn list_all_secrets( | ||
| &self, | ||
| component_id: &str, | ||
| show_values: bool, | ||
| ) -> Result<HashMap<String, Option<String>>> { | ||
| let mut result = HashMap::new(); | ||
| | ||
| // Add file-based secrets | ||
| let file_secrets = self | ||
| .list_component_secrets(component_id, show_values) | ||
| .await?; | ||
| result.extend(file_secrets); | ||
| | ||
| // Add memory secrets | ||
| let store = self.memory_store.read().await; | ||
| if let Some(memory_secrets) = store.get(component_id) { | ||
| for (key, secret_value) in memory_secrets { | ||
| if show_values { | ||
| result.insert(key.clone(), Some(secret_value.expose_secret().to_string())); | ||
| } else { | ||
| result.insert(key.clone(), None); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[nitpick] Potential data race in secret exposure: Multiple concurrent calls to get_all_secrets or list_all_secrets will each call expose_secret() on the same SecretString instances (lines 410, 436). While secrecy crate's expose_secret() is safe to call concurrently (it only provides a reference), creating multiple String copies of secrets in memory increases the attack surface.
Consider one of the following approaches:
- Document that this is acceptable for the use case (secrets need to be exposed to pass to WASM anyway)
- Add rate limiting or caching to reduce the number of simultaneous exposures
- Add a comment explaining the security trade-off
Since the primary use case is providing secrets to components, and they will be exposed anyway, option 1 (documentation) is likely sufficient.
Implements IPC-based dynamic secret injection allowing secrets to be provided to running components without server restart. Secrets injected via IPC are immediately available to components with proper precedence over file-based secrets.
Changes
In-memory secret store (
secrets.rs)RwLock<HashMap<String, HashMap<String, SecretString>>>for runtime secret storageinject_secret(),remove_memory_secret(),get_all_secrets(),list_all_secrets()secrecy::SecretStringand are zeroized on removalSecret precedence (
wasistate.rs,policy_internal.rs)IPC integration (
ipc_server.rs)SetSecret→inject_secret()stores in memoryDeleteSecret→ tries memory first, falls back to fileListSecrets→ returns combined viewError enhancement (
lib.rs)Usage
Dependencies
secrecy = "0.10"- Secure secret handlingzeroize = "1.8"- Memory zeroizationregex = "1.11"- Error pattern matchingOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.