implement vault injection#9883
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @networkbm on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
|
@cla-bot check |
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @networkbm on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
There was a problem hiding this comment.
Overview
Reviewed the vault injection PR adding warp_vault and wiring oz vault inject into the app CLI. The current implementation has build, correctness, and security blockers before it can merge.
Concerns
warp_vault::configis private while the app imports it, so the workspace build fails outside the crate-only test target.- Secret injection uses unsafe process-wide environment mutation, which cannot update the user's parent shell and cannot uphold Rust 2024's safety contract in Warp's multithreaded process.
- Partial CLI overrides silently fall back to configured mappings, and the missing-config error points users to an unimplemented
oz vault initcommand.
Security
- Unsafe global environment mutation for secrets can race with other threads and leaves secret handling dependent on process-wide mutable state; use a scoped session or child-command environment mechanism instead.
Verdict
Found: 2 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| @@ -0,0 +1,8 @@ | |||
| mod config; | |||
There was a problem hiding this comment.
🚨 [CRITICAL] config is private, but app/src/ai/agent_sdk/vault.rs imports warp_vault::config::{...} and warp_vault::config::SecretMapping, so the workspace build fails. Expose the module or re-export the used types and update the app imports.
| for mapping in mappings { | ||
| let secret = provider.fetch(&mapping.path, &mapping.env_var).await?; | ||
| unsafe { | ||
| std::env::set_var(&secret.env_var, secret.value()); |
There was a problem hiding this comment.
🚨 [CRITICAL] [SECURITY] This unsafe process-wide set_var cannot uphold Rust 2024's safety contract in the Warp app because other threads or the AWS runtime may read env concurrently, and it only mutates the Oz process rather than the user's parent shell. Route injected env through a controlled session or child-command mechanism instead of global env mutation.
| rt.block_on(async { | ||
| let config = VaultConfig::load()?; | ||
|
|
||
| let mappings = if let (Some(path), Some(env_var)) = (args.path, args.env_var) { |
There was a problem hiding this comment.
path or only --as silently falls back to config mappings, so a typo or missing argument can inject the wrong configured secrets. Reject partial overrides with clap requirements or an explicit error.
| let path = config_path(); | ||
| let contents = fs::read_to_string(&path).with_context(|| { | ||
| format!( | ||
| "no config found at {} — run 'oz vault init' to get started", |
There was a problem hiding this comment.
oz vault init, but this PR only registers the inject subcommand. Add the init command or replace the message with actionable manual config instructions.
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR adds an oz vault inject command, CLI wiring, and a new warp_vault crate for fetching AWS Secrets Manager values and printing shell exports.
Concerns
- Explicit
path/--asinvocation still requires~/.warp/vault.toml, so the advertised one-off path cannot work without a config file.
Security
- The generated shell fragment emits unvalidated environment variable names, allowing shell injection when users evaluate the output.
- Secret-bearing export commands are printed to stdout, so direct invocation exposes secrets in terminal scrollback/log capture.
- The redactor duplicates registered secrets into a non-zeroizing
HashSet<String>.
Verdict
Found: 0 critical, 4 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| VaultCommand::Inject(args) => { | ||
| let rt = tokio::runtime::Runtime::new()?; | ||
| rt.block_on(async { | ||
| let config = VaultConfig::load()?; |
There was a problem hiding this comment.
oz vault inject <path> --as <env> fail when ~/.warp/vault.toml is absent, despite the CLI advertising that path/--as can be used instead. Defer config loading until config-backed mappings are needed or allow AWS defaults for one-off injections.
|
|
||
| for secret in &secrets { | ||
| let escaped = secret.value().replace('\'', "'\\''"); | ||
| println!("export {}='{}'", secret.env_var, escaped); |
There was a problem hiding this comment.
env_var is emitted into an eval-able shell fragment without validation; a mapping like FOO=$(cmd) or FOO;cmd will execute when users eval the output. Reject names that are not valid shell identifiers before printing.
|
|
||
| for secret in &secrets { | ||
| let escaped = secret.value().replace('\'', "'\\''"); | ||
| println!("export {}='{}'", secret.env_var, escaped); |
There was a problem hiding this comment.
oz vault inject directly puts secrets in terminal scrollback and logs. Use a shell-integration path that suppresses display or make the command require a non-displaying invocation mode.
| } | ||
|
|
||
| pub fn register(&mut self, value: String) { | ||
| self.known_set.insert(value.clone()); |
There was a problem hiding this comment.
value.clone() stores a second plaintext copy in HashSet<String> that is not zeroized, so registered secrets can remain in heap memory after Redactor drops. Avoid the non-zeroizing set or store only zeroizing values.
There was a problem hiding this comment.
Overview
This PR adds an oz vault inject command, CLI wiring, and a new warp_vault crate for fetching AWS Secrets Manager values and printing shell exports.
Concerns
- Explicit
path/--asinvocation still requires~/.warp/vault.toml, so the advertised one-off path cannot work without a config file.
Security
- The generated shell fragment emits unvalidated environment variable names, allowing shell injection when users evaluate the output.
- Secret-bearing export commands are printed to stdout, so direct invocation exposes secrets in terminal scrollback/log capture.
- The redactor duplicates registered secrets into a non-zeroizing
HashSet<String>.
Verdict
Found: 0 critical, 4 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| VaultCommand::Inject(args) => { | ||
| let rt = tokio::runtime::Runtime::new()?; | ||
| rt.block_on(async { | ||
| let config = VaultConfig::load()?; |
There was a problem hiding this comment.
oz vault inject <path> --as <env> fail when ~/.warp/vault.toml is absent, despite the CLI advertising that path/--as can be used instead. Defer config loading until config-backed mappings are needed or allow AWS defaults for one-off injections.
|
|
||
| for secret in &secrets { | ||
| let escaped = secret.value().replace('\'', "'\\''"); | ||
| println!("export {}='{}'", secret.env_var, escaped); |
There was a problem hiding this comment.
env_var is emitted into an eval-able shell fragment without validation; a mapping like FOO=$(cmd) or FOO;cmd will execute when users eval the output. Reject names that are not valid shell identifiers before printing.
|
|
||
| for secret in &secrets { | ||
| let escaped = secret.value().replace('\'', "'\\''"); | ||
| println!("export {}='{}'", secret.env_var, escaped); |
There was a problem hiding this comment.
oz vault inject directly puts secrets in terminal scrollback and logs. Use a shell-integration path that suppresses display or make the command require a non-displaying invocation mode.
| } | ||
|
|
||
| pub fn register(&mut self, value: String) { | ||
| self.known_set.insert(value.clone()); |
There was a problem hiding this comment.
value.clone() stores a second plaintext copy in HashSet<String> that is not zeroized, so registered secrets can remain in heap memory after Redactor drops. Avoid the non-zeroizing set or store only zeroizing values.
Description
Adds
oz vault inject- a new CLI command that fetches secrets from AWS Secrets Manager and injects them as environment variables in-memory, without exposing them in shell history or scrollback.Linked Issue
Closes #9878
ready-to-specorready-to-implement.Testing
Added 10 unit tests covering:
cargo test -p warp_vault — 10/10 passing
cargo clippy -p warp_vault — no warnings
cargo fmt -p warp_vault — clean
Agent Mode
CHANGELOG-NEW-FEATURE: Add
oz vault injectto fetch secrets from AWS Secrets Manager and inject them into the shell session without exposing them in history or scrollback