adding aws secrets and dba features#12
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Write updated secret | ||
| vault | ||
| .write_secret(mount, path, new_data) | ||
| backend | ||
| .write_secret(path, new_data) | ||
| .await | ||
| .context("Failed to write rotated secret")?; |
There was a problem hiding this comment.
The rotated value is saved to the secret backend before any target password change is attempted. If update_password or the subsequent verification fails, the function returns an error but the secret store already contains the new value while the target system still uses the old password, leaving clients with unusable credentials. Consider updating/verifying the target first (or rolling back the secret) so that failures don’t produce a split‑brain state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull Request Overview
This PR adds significant new functionality to the secret rotation tool by introducing multi-backend support (AWS Secrets Manager alongside HashiCorp Vault) and target-based password rotation (PostgreSQL databases and REST APIs). The changes involve a major refactoring to support abstraction layers for both secret backends and rotation targets.
Key Changes:
- Added AWS Secrets Manager as an alternative backend to Vault
- Introduced target abstraction for rotating passwords in databases and APIs
- Refactored codebase into modular backend and target systems
- Updated CLI to support new backend and target options
Reviewed Changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
src/backends/mod.rs |
New module defining backend abstractions and exports |
src/backends/secret_backend.rs |
Core trait for secret management backends |
src/backends/vault.rs |
Vault backend implementation with wrapper |
src/backends/aws_secrets.rs |
AWS Secrets Manager backend implementation |
src/targets/mod.rs |
New module for password update targets |
src/targets/target.rs |
Core trait for password rotation targets |
src/targets/postgres.rs |
PostgreSQL database target implementation |
src/targets/api.rs |
REST API target implementation |
src/rotation.rs |
Updated rotation logic to support backends and targets |
src/config.rs |
Extended configuration for multiple backends and targets |
src/cli.rs |
New CLI module with command execution logic |
src/main.rs |
Simplified main entry point delegating to CLI module |
src/lib.rs |
Library interface with public exports |
src/env_updater.rs |
Minor improvements to environment variable handling |
Cargo.toml |
Added AWS SDK and PostgreSQL dependencies |
README.md |
Comprehensive documentation updates with examples |
examples/config.toml |
Example configuration with new options |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Try to find password key | ||
| secret | ||
| .data | ||
| .values() | ||
| .next() | ||
| .cloned() | ||
| .ok_or_else(|| anyhow::anyhow!("No password found in secret at {}", password_path))? |
There was a problem hiding this comment.
[nitpick] The read_secret method in AWS backend uses .values().next() to get the first password value when password_path is provided, but doesn't specify which key it expects. This could be ambiguous if the secret contains multiple keys.
Consider documenting the expected key name (e.g., "password") or allow configuration to specify which key to use for the admin password.
| // Try to find password key | |
| secret | |
| .data | |
| .values() | |
| .next() | |
| .cloned() | |
| .ok_or_else(|| anyhow::anyhow!("No password found in secret at {}", password_path))? | |
| // Use password_key if provided, otherwise default to "password" | |
| let password_key = config.password_key.as_deref().unwrap_or("password"); | |
| secret | |
| .data | |
| .get(password_key) | |
| .cloned() | |
| .ok_or_else(|| anyhow::anyhow!( | |
| "No password found in secret at {} with key '{}'. \ | |
| Please ensure the secret contains a '{}' field or set password_key in config.", | |
| password_path, password_key, password_key | |
| ))? |
| let _connection_handle = tokio::spawn(async move { | ||
| if let Err(e) = connection.await { | ||
| eprintln!("PostgreSQL connection error: {}", e); | ||
| } |
There was a problem hiding this comment.
Connection errors are printed to stderr using eprintln! instead of being logged through the tracing framework. This is inconsistent with the rest of the codebase which uses tracing::error! and similar macros.
Change to:
if let Err(e) = connection.await {
tracing::error!("PostgreSQL connection error: {}", e);
}| async fn verify_connection(&self, _username: &str, _password: &str, _database: Option<&str>) -> Result<()> { | ||
| // API targets may not support verification, or it could be done via a separate endpoint | ||
| // For now, we'll skip verification for API targets | ||
| info!("Verification not supported for API targets"); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The API target's verify_connection method always returns Ok(()) without actually verifying anything. This creates a false sense of security since callers expect this to validate the password update succeeded.
Consider either:
- Implementing actual verification (e.g., calling a verification endpoint if the API supports it)
- Removing this from the trait if not all targets can support it
- Documenting clearly that verification is not supported for API targets and may fail silently
| let mut metadata = match backend.read_metadata(path).await { | ||
| Ok(existing) => existing, | ||
| Err(e) => { | ||
| warn!( | ||
| "Failed to read existing metadata for {}/{}: {}. Proceeding with defaults.", | ||
| mount, path, e | ||
| "Failed to read existing metadata for {}: {}. Proceeding with defaults.", | ||
| path, e | ||
| ); | ||
| HashMap::new() | ||
| } |
There was a problem hiding this comment.
The metadata reading logic has changed from returning existing.custom_metadata.unwrap_or_default() to just existing. However, the return type remains HashMap<String, String> but read_metadata now returns this directly. This looks correct, but the comment on line 141-143 still refers to the old behavior mentioning "custom_metadata".
Update the comment to reflect the new behavior:
let mut metadata = match backend.read_metadata(path).await {
Ok(existing) => existing,
Err(e) => {
warn!(
"Failed to read existing metadata for {}: {}. Proceeding with defaults.",
path, e
);
HashMap::new()
}
};| .filter_map(|tag| { | ||
| tag.key() | ||
| .and_then(|k| tag.value().map(|v| (k.to_string(), v.to_string()))) | ||
| }) |
There was a problem hiding this comment.
[nitpick] Missing error handling for the case when .tags() returns tags but some tags have missing keys or values. The .filter_map() will silently skip these, which could lead to incomplete metadata without any indication to the user.
| .filter_map(|tag| { | |
| tag.key() | |
| .and_then(|k| tag.value().map(|v| (k.to_string(), v.to_string()))) | |
| }) | |
| .map(|tag| { | |
| match (tag.key(), tag.value()) { | |
| (Some(k), Some(v)) => Some((k.to_string(), v.to_string())), | |
| (key, value) => { | |
| // Log a warning if key or value is missing | |
| tracing::warn!( | |
| "Skipping AWS tag with missing key or value: key={:?}, value={:?}", | |
| key, | |
| value | |
| ); | |
| None | |
| } | |
| } | |
| }) | |
| .filter_map(|x| x) |
| fn default_api_method() -> String { | ||
| "POST".to_string() | ||
| } | ||
|
|
||
| fn default_password_field() -> String { | ||
| "password".to_string() | ||
| } | ||
|
|
||
| fn default_api_timeout() -> u64 { | ||
| 30 | ||
| } | ||
|
|
||
| fn default_db_port() -> u16 { | ||
| 5432 | ||
| } | ||
|
|
||
| fn default_ssl_mode() -> String { | ||
| "prefer".to_string() | ||
| } | ||
|
|
||
| fn default_backend() -> String { | ||
| "vault".to_string() | ||
| } | ||
|
|
||
| fn default_mount() -> String { | ||
| "secret".to_string() | ||
| } | ||
|
|
||
| fn default_aws_region() -> String { | ||
| "us-east-1".to_string() | ||
| } |
There was a problem hiding this comment.
[nitpick] The default functions for configuration fields are defined but not consistently used. default_mount() returns "secret" but the VaultConfig mount field doesn't use #[serde(default = "default_mount")], it's only added later. This inconsistency could lead to confusion about which defaults are actually applied.
| // Close the test connection by dropping it | ||
| drop(test_connection); |
There was a problem hiding this comment.
The test connection is created but never properly closed. The test_connection variable is dropped without being awaited, which means the connection might not be gracefully terminated. This could lead to lingering connections or resource leaks.
Consider awaiting the connection to ensure it's properly closed:
drop(test_client);
// Ensure connection is properly closed
let _ = test_connection.await;| // Close the test connection by dropping it | |
| drop(test_connection); | |
| // Ensure the test connection is properly closed by awaiting it | |
| let _ = test_connection.await; |
| async fn verify_connection(&self, username: &str, password: &str, database: Option<&str>) -> Result<()> { | ||
| info!("Verifying connection for user: {}", username); | ||
|
|
||
| let db_name = database.unwrap_or(&self.config.database); | ||
| let connection_string = Self::build_connection_string( | ||
| &self.config.host, | ||
| self.config.port, | ||
| username, | ||
| password, | ||
| db_name, |
There was a problem hiding this comment.
[nitpick] The database parameter in verify_connection is marked as Option<&str> but the function always uses either the provided value or falls back to self.config.database. This suggests the parameter should be required rather than optional, or the function signature could be simplified.
Consider either:
- Making it non-optional if it's always expected
- Documenting when and why callers would pass
NonevsSome(db_name)
| async fn verify_connection(&self, username: &str, password: &str, database: Option<&str>) -> Result<()> { | |
| info!("Verifying connection for user: {}", username); | |
| let db_name = database.unwrap_or(&self.config.database); | |
| let connection_string = Self::build_connection_string( | |
| &self.config.host, | |
| self.config.port, | |
| username, | |
| password, | |
| db_name, | |
| async fn verify_connection(&self, username: &str, password: &str, database: &str) -> Result<()> { | |
| info!("Verifying connection for user: {}", username); | |
| let connection_string = Self::build_connection_string( | |
| &self.config.host, | |
| self.config.port, | |
| username, | |
| password, | |
| database, |
| #[serde(default)] | ||
| pub targets: Option<TargetsConfig>, | ||
| } | ||
|
|
There was a problem hiding this comment.
The configuration has a confusing mix of optional backend configs. The vault field is now Option<VaultConfig> and aws is Option<AwsConfig>, but there's no validation to ensure at least one backend is configured when backend is set to a specific value. A user could set backend = "vault" without providing vault configuration, leading to a runtime error.
Consider adding a validation method that ensures the selected backend has its configuration present.
| impl Config { | |
| /// Validates that the selected backend has its configuration present. | |
| pub fn validate_backends(&self) -> Result<()> { | |
| match self.backend.as_str() { | |
| "vault" => { | |
| if self.vault.is_none() { | |
| anyhow::bail!("Backend 'vault' selected but no vault configuration provided."); | |
| } | |
| } | |
| "aws" => { | |
| if self.aws.is_none() { | |
| anyhow::bail!("Backend 'aws' selected but no AWS configuration provided."); | |
| } | |
| } | |
| _ => { | |
| // If other backends are supported, add checks here. | |
| } | |
| } | |
| Ok(()) | |
| } | |
| } |
these changes are for folder structure update to include how we have targets and providers.
also adding integration of AWS secrets and postgres database for a target