Skip to content

Commit d426bb8

Browse files
committed
feat(cli): reject duplicate secret material keys
Signed-off-by: Hung Le <hple@nvidia.com>
1 parent cc785be commit d426bb8

2 files changed

Lines changed: 59 additions & 6 deletions

File tree

crates/openshell-cli/src/run.rs

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4037,6 +4037,11 @@ pub fn parse_secret_material_env_pairs(items: &[String]) -> Result<HashMap<Strin
40374037
));
40384038
}
40394039

4040+
if map.contains_key(key) {
4041+
return Err(miette::miette!(
4042+
"--secret-material-env key '{key}' supplied more than once"
4043+
));
4044+
}
40404045
map.insert(key.to_string(), value);
40414046
}
40424047

@@ -5331,8 +5336,14 @@ pub async fn provider_refresh_config(
53315336
let strategy = provider_refresh_strategy(input.strategy)?;
53325337
let mut material = parse_key_value_pairs(input.material, "--material")?;
53335338
let mut secret_material_keys = input.secret_material_keys.to_vec();
5334-
// Env-resolved secrets override `--material` duplicates and are auto-marked secret.
5339+
// Env-resolved secrets are auto-marked secret; duplicate keys are an
5340+
// error rather than a precedence order.
53355341
for (key, value) in parse_secret_material_env_pairs(input.secret_material_env)? {
5342+
if material.contains_key(&key) {
5343+
return Err(miette!(
5344+
"duplicate material key '{key}': supplied via both --material and --secret-material-env"
5345+
));
5346+
}
53365347
if !secret_material_keys.contains(&key) {
53375348
secret_material_keys.push(key.clone());
53385349
}
@@ -8091,6 +8102,21 @@ mod tests {
80918102
assert!(err.to_string().contains("key cannot be empty"));
80928103
}
80938104

8105+
#[test]
8106+
fn parse_secret_material_env_pairs_rejects_duplicate_keys() {
8107+
let _guard = EnvVarGuard::set("NAV_PARSE_SME_DUP", "value");
8108+
8109+
let err = parse_secret_material_env_pairs(&[
8110+
"private_key=NAV_PARSE_SME_DUP".to_string(),
8111+
"private_key=NAV_PARSE_SME_DUP".to_string(),
8112+
])
8113+
.expect_err("duplicate key should error");
8114+
assert!(
8115+
err.to_string()
8116+
.contains("key 'private_key' supplied more than once")
8117+
);
8118+
}
8119+
80948120
#[test]
80958121
fn parse_credential_expiry_pairs_accepts_epoch_millis_and_rfc3339() {
80968122
let parsed = parse_credential_expiry_pairs(&[

crates/openshell-cli/tests/provider_commands_integration.rs

Lines changed: 32 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1258,18 +1258,15 @@ async fn provider_refresh_configure_reads_secret_material_from_env_off_argv() {
12581258
.await
12591259
.expect("provider create");
12601260

1261-
// The env value overrides the `--material` duplicate and is auto-marked secret.
1261+
// The env value reaches the request and is auto-marked secret.
12621262
let guard = EnvVarGuard::set(&[("OPENSHELL_ITEST_SME_PRIVATE_KEY", "pem-from-env")]);
12631263
run::provider_refresh_config(
12641264
&ts.endpoint,
12651265
run::ProviderRefreshConfigInput {
12661266
name: "gc-bridge",
12671267
credential_key: "GOOGLE_CHAT_ACCESS_TOKEN",
12681268
strategy: "google_service_account_jwt",
1269-
material: &[
1270-
"client_email=bot@p.iam.gserviceaccount.com".to_string(),
1271-
"private_key=argv-value-must-be-overridden".to_string(),
1272-
],
1269+
material: &["client_email=bot@p.iam.gserviceaccount.com".to_string()],
12731270
secret_material_env: &["private_key=OPENSHELL_ITEST_SME_PRIVATE_KEY".to_string()],
12741271
secret_material_keys: &[],
12751272
credential_expires_at_ms: None,
@@ -1299,6 +1296,36 @@ async fn provider_refresh_configure_reads_secret_material_from_env_off_argv() {
12991296
);
13001297
}
13011298

1299+
#[tokio::test]
1300+
async fn provider_refresh_configure_rejects_key_supplied_via_both_material_and_env() {
1301+
let ts = run_server().await;
1302+
1303+
let guard = EnvVarGuard::set(&[("OPENSHELL_ITEST_SME_DUP_KEY", "pem-from-env")]);
1304+
let err = run::provider_refresh_config(
1305+
&ts.endpoint,
1306+
run::ProviderRefreshConfigInput {
1307+
name: "gc-bridge",
1308+
credential_key: "GOOGLE_CHAT_ACCESS_TOKEN",
1309+
strategy: "google_service_account_jwt",
1310+
material: &["private_key=argv-value".to_string()],
1311+
secret_material_env: &["private_key=OPENSHELL_ITEST_SME_DUP_KEY".to_string()],
1312+
secret_material_keys: &[],
1313+
credential_expires_at_ms: None,
1314+
},
1315+
&ts.tls,
1316+
)
1317+
.await
1318+
.expect_err("duplicate key across --material and --secret-material-env should fail");
1319+
drop(guard);
1320+
1321+
assert!(
1322+
err.to_string()
1323+
.contains("duplicate material key 'private_key'")
1324+
);
1325+
// Rejected client-side: nothing reached the gateway.
1326+
assert!(ts.state.refresh_requests.lock().await.is_empty());
1327+
}
1328+
13021329
#[tokio::test]
13031330
async fn provider_refresh_configure_fails_closed_when_secret_material_env_is_unset() {
13041331
let ts = run_server().await;

0 commit comments

Comments
 (0)