-
Notifications
You must be signed in to change notification settings - Fork 56
Integrate dynamic secret injection with component runtime #577
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: copilot/implement-ipc-server-infrastructure
Are you sure you want to change the base?
Changes from all commits
39c2a7a
91ba9aa
03fe020
3c2dacb
b7d9af6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -319,6 +319,61 @@ pub struct ComponentInstance { | |||||||||||||||||||||||||||||||||
| package_docs: Option<Value>, | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| /// Attempt to extract missing environment variable names from error messages | ||||||||||||||||||||||||||||||||||
| fn extract_missing_env_vars(error_message: &str) -> Vec<String> { | ||||||||||||||||||||||||||||||||||
| let mut missing_vars = Vec::new(); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| // Common patterns for missing environment variable errors | ||||||||||||||||||||||||||||||||||
| let patterns = [ | ||||||||||||||||||||||||||||||||||
| // 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", | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+328
to
+335
|
||||||||||||||||||||||||||||||||||
| // 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", |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,8 +16,10 @@ use std::path::{Path, PathBuf}; | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use std::time::SystemTime; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use anyhow::{anyhow, Context, Result}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use secrecy::{ExposeSecret, SecretString}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use tokio::sync::RwLock; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use tracing::{debug, info, warn}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use zeroize::Zeroize; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Cache entry for component secrets | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[derive(Debug, Clone)] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -33,8 +35,11 @@ pub struct SecretCache { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub struct SecretsManager { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Directory where secrets are stored | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| secrets_dir: PathBuf, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Cache of component secrets | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Cache of component secrets from files | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cache: RwLock<HashMap<String, SecretCache>>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// In-memory secrets store for dynamically injected secrets | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Component ID → (Secret Key → Secret Value) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| memory_store: RwLock<HashMap<String, HashMap<String, SecretString>>>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| impl SecretsManager { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -43,6 +48,7 @@ impl SecretsManager { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Self { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| secrets_dir, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cache: RwLock::new(HashMap::new()), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| memory_store: RwLock::new(HashMap::new()), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -339,6 +345,103 @@ impl SecretsManager { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Ok(()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// 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(()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+373
to
+381
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } 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); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+425
to
+439
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 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 |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Silent error swallowing: When
remove_memory_secretfails (line 174), the error is silently ignored (theErr(_)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: