From cc785bef5081efea00c5d07e579fe32cf4dd91ee Mon Sep 17 00:00:00 2001 From: Hung Le Date: Wed, 8 Jul 2026 16:06:33 +0530 Subject: [PATCH 1/2] feat(cli): add --secret-material-env to provider refresh configure --- crates/openshell-cli/src/main.rs | 12 +- crates/openshell-cli/src/run.rs | 112 +++++++++++++++-- .../tests/provider_commands_integration.rs | 118 ++++++++++++++++-- docs/sandboxes/manage-providers.mdx | 10 +- 4 files changed, 230 insertions(+), 22 deletions(-) diff --git a/crates/openshell-cli/src/main.rs b/crates/openshell-cli/src/main.rs index e02c7c9cd5..66484562d4 100644 --- a/crates/openshell-cli/src/main.rs +++ b/crates/openshell-cli/src/main.rs @@ -898,6 +898,11 @@ enum ProviderRefreshCommands { #[arg(long = "material", value_name = "KEY=VALUE")] material: Vec, + /// Secret refresh material resolved from the CLI environment + /// (`ENVVAR` defaults to `KEY`); `KEY` is auto-marked secret. + #[arg(long = "secret-material-env", value_name = "KEY[=ENVVAR]")] + secret_material_env: Vec, + /// Material keys that are secret and must not be exposed. #[arg(long = "secret-material-key", value_name = "KEY")] secret_material_keys: Vec, @@ -2922,6 +2927,7 @@ async fn main() -> Result<()> { credential_key, strategy, material, + secret_material_env, secret_material_keys, credential_expires_at, } => { @@ -2932,6 +2938,7 @@ async fn main() -> Result<()> { credential_key: &credential_key, strategy: strategy.as_str(), material: &material, + secret_material_env: &secret_material_env, secret_material_keys: &secret_material_keys, credential_expires_at_ms: credential_expires_at, }, @@ -4189,6 +4196,8 @@ mod tests { "oauth2-client-credentials", "--material", "tenant_id=abc", + "--secret-material-env", + "client_secret=GRAPH_CLIENT_SECRET", "--secret-material-key", "client_secret", "--credential-expires-at", @@ -4202,10 +4211,11 @@ mod tests { ProviderRefreshCommands::Configure { strategy: CliProviderRefreshStrategy::Oauth2ClientCredentials, credential_expires_at: Some(1_767_225_600_000), + ref secret_material_env, .. } )) - }) + }) if secret_material_env == &["client_secret=GRAPH_CLIENT_SECRET".to_string()] )); let rotate = Cli::try_parse_from([ diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index f56bb71512..67eeb9d7ad 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -4007,6 +4007,42 @@ pub fn parse_env_pairs(items: &[String]) -> Result> { Ok(map) } +/// Resolve `--secret-material-env KEY[=ENVVAR]` values from the CLI process +/// environment (`ENVVAR` defaults to `KEY`) so secrets never transit argv. +pub fn parse_secret_material_env_pairs(items: &[String]) -> Result> { + let mut map = HashMap::new(); + + for item in items { + let (key, env_name) = match item.split_once('=') { + Some((key, env_name)) => (key.trim(), env_name.trim()), + None => (item.trim(), item.trim()), + }; + if key.is_empty() { + return Err(miette::miette!("--secret-material-env key cannot be empty")); + } + if env_name.is_empty() { + return Err(miette::miette!( + "--secret-material-env {key} names an empty environment variable" + )); + } + + let value = std::env::var(env_name).map_err(|_| { + miette::miette!( + "--secret-material-env {key} requires local env var '{env_name}' to be set to a non-empty value" + ) + })?; + if value.trim().is_empty() { + return Err(miette::miette!( + "--secret-material-env {key} requires local env var '{env_name}' to be set to a non-empty value" + )); + } + + map.insert(key.to_string(), value); + } + + Ok(map) +} + fn is_valid_env_name(key: &str) -> bool { let mut bytes = key.bytes(); let Some(first) = bytes.next() else { @@ -5282,6 +5318,7 @@ pub struct ProviderRefreshConfigInput<'a> { pub credential_key: &'a str, pub strategy: &'a str, pub material: &'a [String], + pub secret_material_env: &'a [String], pub secret_material_keys: &'a [String], pub credential_expires_at_ms: Option, } @@ -5292,7 +5329,15 @@ pub async fn provider_refresh_config( tls: &TlsOptions, ) -> Result<()> { let strategy = provider_refresh_strategy(input.strategy)?; - let material = parse_key_value_pairs(input.material, "--material")?; + let mut material = parse_key_value_pairs(input.material, "--material")?; + let mut secret_material_keys = input.secret_material_keys.to_vec(); + // Env-resolved secrets override `--material` duplicates and are auto-marked secret. + for (key, value) in parse_secret_material_env_pairs(input.secret_material_env)? { + if !secret_material_keys.contains(&key) { + secret_material_keys.push(key.clone()); + } + material.insert(key, value); + } let mut client = grpc_client(server, tls).await?; let status = client .configure_provider_refresh(ConfigureProviderRefreshRequest { @@ -5300,7 +5345,7 @@ pub async fn provider_refresh_config( credential_key: input.credential_key.to_string(), strategy: strategy as i32, material, - secret_material_keys: input.secret_material_keys.to_vec(), + secret_material_keys, expires_at_ms: input.credential_expires_at_ms, }) .await @@ -7845,11 +7890,12 @@ mod tests { gateway_type_label, git_sync_files, http_health_check, import_local_package_mtls_bundle, inferred_provider_type, mtls_certs_exist_for_gateway, package_managed_tls_dirs, parse_cli_setting_value, parse_credential_expiry_cli_value, parse_credential_expiry_pairs, - parse_credential_pairs, parse_driver_config_json, plaintext_gateway_is_remote, - progress_step_from_metadata, provider_profile_allows_empty_credentials, - provisioning_timeout_message, ready_false_condition_message, refresh_status_header, - refresh_status_row, resolve_from, sandbox_should_persist, sandbox_upload_plan, - service_expose_status_error, service_url_for_gateway, + parse_credential_pairs, parse_driver_config_json, parse_secret_material_env_pairs, + plaintext_gateway_is_remote, progress_step_from_metadata, + provider_profile_allows_empty_credentials, provisioning_timeout_message, + ready_false_condition_message, refresh_status_header, refresh_status_row, resolve_from, + sandbox_should_persist, sandbox_upload_plan, service_expose_status_error, + service_url_for_gateway, }; use crate::TEST_ENV_LOCK; use hyper::StatusCode; @@ -7993,6 +8039,58 @@ mod tests { )); } + #[test] + fn parse_secret_material_env_pairs_reads_value_from_named_environment_variable() { + let _guard = EnvVarGuard::set("NAV_PARSE_SME_NAMED", "pem-material"); + + let parsed = + parse_secret_material_env_pairs(&["private_key=NAV_PARSE_SME_NAMED".to_string()]) + .expect("parse"); + assert_eq!(parsed.get("private_key"), Some(&"pem-material".to_string())); + } + + #[test] + fn parse_secret_material_env_pairs_defaults_env_name_to_key() { + let _guard = EnvVarGuard::set("NAV_PARSE_SME_KEY_ONLY", "key-only-material"); + + let parsed = parse_secret_material_env_pairs(&["NAV_PARSE_SME_KEY_ONLY".to_string()]) + .expect("parse"); + assert_eq!( + parsed.get("NAV_PARSE_SME_KEY_ONLY"), + Some(&"key-only-material".to_string()) + ); + } + + #[test] + fn parse_secret_material_env_pairs_rejects_missing_environment() { + let _guard = EnvVarGuard::unset("NAV_PARSE_SME_MISSING"); + + let err = + parse_secret_material_env_pairs(&["private_key=NAV_PARSE_SME_MISSING".to_string()]) + .expect_err("missing env should error"); + assert!(err.to_string().contains( + "requires local env var 'NAV_PARSE_SME_MISSING' to be set to a non-empty value" + )); + } + + #[test] + fn parse_secret_material_env_pairs_rejects_empty_environment_value() { + let _guard = EnvVarGuard::set("NAV_PARSE_SME_EMPTY", " "); + + let err = parse_secret_material_env_pairs(&["private_key=NAV_PARSE_SME_EMPTY".to_string()]) + .expect_err("blank env should error"); + assert!(err.to_string().contains( + "requires local env var 'NAV_PARSE_SME_EMPTY' to be set to a non-empty value" + )); + } + + #[test] + fn parse_secret_material_env_pairs_rejects_empty_key() { + let err = parse_secret_material_env_pairs(&["=NAV_PARSE_SME_NO_KEY".to_string()]) + .expect_err("empty key should error"); + assert!(err.to_string().contains("key cannot be empty")); + } + #[test] fn parse_credential_expiry_pairs_accepts_epoch_millis_and_rfc3339() { let parsed = parse_credential_expiry_pairs(&[ diff --git a/crates/openshell-cli/tests/provider_commands_integration.rs b/crates/openshell-cli/tests/provider_commands_integration.rs index 5a6e53eb15..76b0dc0bc2 100644 --- a/crates/openshell-cli/tests/provider_commands_integration.rs +++ b/crates/openshell-cli/tests/provider_commands_integration.rs @@ -62,6 +62,8 @@ enum ProviderRefreshRequestLog { Configure { provider_name: String, credential_key: String, + material: HashMap, + secret_material_keys: Vec, expires_at_ms: Option, }, Rotate { @@ -661,6 +663,8 @@ impl OpenShell for TestOpenShell { .push(ProviderRefreshRequestLog::Configure { provider_name: request.provider.clone(), credential_key: request.credential_key.clone(), + material: request.material.clone(), + secret_material_keys: request.secret_material_keys.clone(), expires_at_ms: request.expires_at_ms, }); let configure_failure = self @@ -1187,6 +1191,7 @@ async fn provider_refresh_cli_run_functions_wire_requests() { credential_key: "MS_GRAPH_ACCESS_TOKEN", strategy: "oauth2_client_credentials", material: &["tenant_id=tenant".to_string()], + secret_material_env: &[], secret_material_keys: &["client_secret".to_string()], credential_expires_at_ms: Some(1_767_225_600_000), }, @@ -1216,6 +1221,8 @@ async fn provider_refresh_cli_run_functions_wire_requests() { ProviderRefreshRequestLog::Configure { provider_name: "my-graph".to_string(), credential_key: "MS_GRAPH_ACCESS_TOKEN".to_string(), + material: HashMap::from([("tenant_id".to_string(), "tenant".to_string())]), + secret_material_keys: vec!["client_secret".to_string()], expires_at_ms: Some(1_767_225_600_000), }, ProviderRefreshRequestLog::Status { @@ -1234,6 +1241,91 @@ async fn provider_refresh_cli_run_functions_wire_requests() { ); } +#[tokio::test] +async fn provider_refresh_configure_reads_secret_material_from_env_off_argv() { + let ts = run_server().await; + + run::provider_create( + &ts.endpoint, + "gc-bridge", + "outlook", + false, + &["GOOGLE_CHAT_ACCESS_TOKEN=pending".to_string()], + false, + &[], + &ts.tls, + ) + .await + .expect("provider create"); + + // The env value overrides the `--material` duplicate and is auto-marked secret. + let guard = EnvVarGuard::set(&[("OPENSHELL_ITEST_SME_PRIVATE_KEY", "pem-from-env")]); + run::provider_refresh_config( + &ts.endpoint, + run::ProviderRefreshConfigInput { + name: "gc-bridge", + credential_key: "GOOGLE_CHAT_ACCESS_TOKEN", + strategy: "google_service_account_jwt", + material: &[ + "client_email=bot@p.iam.gserviceaccount.com".to_string(), + "private_key=argv-value-must-be-overridden".to_string(), + ], + secret_material_env: &["private_key=OPENSHELL_ITEST_SME_PRIVATE_KEY".to_string()], + secret_material_keys: &[], + credential_expires_at_ms: None, + }, + &ts.tls, + ) + .await + .expect("provider refresh configure"); + drop(guard); + + let requests = ts.state.refresh_requests.lock().await.clone(); + assert_eq!( + requests, + vec![ProviderRefreshRequestLog::Configure { + provider_name: "gc-bridge".to_string(), + credential_key: "GOOGLE_CHAT_ACCESS_TOKEN".to_string(), + material: HashMap::from([ + ( + "client_email".to_string(), + "bot@p.iam.gserviceaccount.com".to_string() + ), + ("private_key".to_string(), "pem-from-env".to_string()), + ]), + secret_material_keys: vec!["private_key".to_string()], + expires_at_ms: None, + }] + ); +} + +#[tokio::test] +async fn provider_refresh_configure_fails_closed_when_secret_material_env_is_unset() { + let ts = run_server().await; + + let err = run::provider_refresh_config( + &ts.endpoint, + run::ProviderRefreshConfigInput { + name: "gc-bridge", + credential_key: "GOOGLE_CHAT_ACCESS_TOKEN", + strategy: "google_service_account_jwt", + material: &[], + secret_material_env: &["private_key=OPENSHELL_ITEST_SME_DEFINITELY_UNSET".to_string()], + secret_material_keys: &[], + credential_expires_at_ms: None, + }, + &ts.tls, + ) + .await + .expect_err("unset env should fail before any request is sent"); + + assert!(err.to_string().contains( + "requires local env var 'OPENSHELL_ITEST_SME_DEFINITELY_UNSET' to be set to a non-empty value" + )); + // Fails closed on the client side: nothing reached the gateway. + assert!(ts.state.refresh_requests.lock().await.is_empty()); +} + #[tokio::test] async fn provider_create_allows_empty_credentials_for_gateway_refresh_profiles() { let ts = run_server().await; @@ -2197,14 +2289,15 @@ async fn provider_create_from_gcloud_adc_happy_path() { 2, "expected configure + rotate refresh requests" ); - assert_eq!( - requests[0], + assert!(matches!( + &requests[0], ProviderRefreshRequestLog::Configure { - provider_name: "my-vertex".to_string(), - credential_key: "GOOGLE_VERTEX_AI_TOKEN".to_string(), + provider_name, + credential_key, expires_at_ms: None, - } - ); + .. + } if provider_name == "my-vertex" && credential_key == "GOOGLE_VERTEX_AI_TOKEN" + )); assert_eq!( requests[1], ProviderRefreshRequestLog::Rotate { @@ -2577,14 +2670,15 @@ async fn provider_create_from_gcloud_adc_with_config_keys() { 2, "exactly one configure call and one rotate call expected" ); - assert_eq!( - refresh_requests[0], + assert!(matches!( + &refresh_requests[0], ProviderRefreshRequestLog::Configure { - provider_name: "vertex-with-config".to_string(), - credential_key: "GOOGLE_VERTEX_AI_TOKEN".to_string(), + provider_name, + credential_key, expires_at_ms: None, - } - ); + .. + } if provider_name == "vertex-with-config" && credential_key == "GOOGLE_VERTEX_AI_TOKEN" + )); assert_eq!( refresh_requests[1], ProviderRefreshRequestLog::Rotate { diff --git a/docs/sandboxes/manage-providers.mdx b/docs/sandboxes/manage-providers.mdx index 647a196a5b..1324859ed7 100644 --- a/docs/sandboxes/manage-providers.mdx +++ b/docs/sandboxes/manage-providers.mdx @@ -147,11 +147,17 @@ openshell provider refresh configure my-graph \ --strategy oauth2-client-credentials \ --material tenant_id="$TENANT_ID" \ --material client_id="$CLIENT_ID" \ - --material client_secret="$CLIENT_SECRET" \ - --secret-material-key client_secret \ + --secret-material-env client_secret=CLIENT_SECRET \ --credential-expires-at 1767225600000 ``` +Pass secret material with `--secret-material-env KEY[=ENVVAR]` (`ENVVAR` +defaults to `KEY`): the CLI reads the value from its own environment, so the +secret never appears in the host process table the way an expanded +`--material KEY="$VALUE"` argument would, and `KEY` is automatically marked +secret. Keep non-secret material on `--material`; `--secret-material-key KEY` +still marks a key supplied through `--material` as secret. + Check refresh status: ```shell From d426bb84d6ad77cfae9a84c84682e4aaa96ea30e Mon Sep 17 00:00:00 2001 From: Hung Le Date: Wed, 8 Jul 2026 17:44:18 +0530 Subject: [PATCH 2/2] feat(cli): reject duplicate secret material keys Signed-off-by: Hung Le --- crates/openshell-cli/src/run.rs | 28 +++++++++++++- .../tests/provider_commands_integration.rs | 37 ++++++++++++++++--- 2 files changed, 59 insertions(+), 6 deletions(-) diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index 67eeb9d7ad..0182e1b668 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -4037,6 +4037,11 @@ pub fn parse_secret_material_env_pairs(items: &[String]) -> Result