Skip to content

feat(cli): add --secret-material-env to provider refresh configure#2178

Open
hunglp6d wants to merge 2 commits into
NVIDIA:mainfrom
hunglp6d:feat/cli-secret-material-env
Open

feat(cli): add --secret-material-env to provider refresh configure#2178
hunglp6d wants to merge 2 commits into
NVIDIA:mainfrom
hunglp6d:feat/cli-secret-material-env

Conversation

@hunglp6d

@hunglp6d hunglp6d commented Jul 8, 2026

Copy link
Copy Markdown

Summary

Adds --secret-material-env KEY[=ENVVAR] to provider refresh configure so secret
refresh material (service-account private keys, OAuth client secrets) can be supplied
off-argv. Today --material KEY=VALUE is the only ingestion path, which exposes the
value in the host process table (/proc/<pid>/cmdline, ps) and to command-line audit
tooling. The new flag resolves the value from the CLI's own environment — mirroring the
existing --credential KEY ingestion — merges it into the material map, and auto-adds
the key to secret_material_keys. Duplicate keys are rejected rather than given
precedence: the same KEY supplied via both --material and the new flag, or repeated
within the flag, is an error. Missing or blank env fails closed naming the variable,
before any request is sent. Client-side only: the gRPC request, server, and refresh
strategies are unchanged.

Related Issue

Refs #2104 (implements the env-styled complementary option; --secret-material-file /
--material-stdin from the proposal remain open).

Changes

  • openshell-cli/src/main.rs: new --secret-material-env KEY[=ENVVAR] flag on
    provider refresh configure; CLI parse test extended.
  • openshell-cli/src/run.rs: parse_secret_material_env_pairs (error style mirrors the
    --credential env ingestion; never echoes values) and the merge in
    provider_refresh_config; 5 unit tests.
  • openshell-cli/tests/provider_commands_integration.rs: the mock now records
    material/secret_material_keys; new end-to-end tests (env value reaches the request
    auto-marked secret; a --material duplicate is rejected client-side before any
    request) and a fails-closed test (unset env → error names the variable, no request
    leaves the client).
  • docs/sandboxes/manage-providers.mdx: the Credential Refresh example no longer
    expands a secret onto argv; documents the new flag.

Testing

  • Unit tests added/updated
  • E2E tests added/updated (if applicable) — n/a, covered by crate integration tests
  • mise run pre-commit passes — the full alias (workspace-wide lint+check+test)
    does not fit on my dev box; ran every lane the diff touches manually:
    cargo test -p openshell-cli (351 passed, 0 failed),
    cargo fmt -p openshell-cli -- --check (clean),
    cargo clippy -p openshell-cli --all-targets (no warnings),
    markdownlint-cli2 over the repo docs globs (101 files, 0 errors).
    Deferring the untouched python/helm lanes to CI.

Also manually verified against a stock v0.0.78 gateway (release deb install; this
branch's CLI 0.0.79-dev talking to an unmodified released server):

argv exposure: old path reproduces it, new flag closes it
# old path (stock binary, --material): secret visible to any local user
$ openshell provider refresh configure sme-test ... --material private_key=SIEU-BI-MAT-123 ... &
$ cat /proc/$!/cmdline | tr '\0' ' '
openshell provider refresh configure sme-test ... --material private_key=SIEU-BI-MAT-123 ...

# new flag (this branch): argv carries only the variable NAME
$ read -rs PK_TEST; export PK_TEST
$ osnew provider refresh configure sme-test ... --secret-material-env private_key=PK_TEST &
$ cat /proc/$!/cmdline | tr '\0' ' '
.../openshell provider refresh configure sme-test ... --secret-material-env private_key=PK_TEST
✓ Configured refresh for sme-test SME_TEST_TOKEN
fails closed when the env var is unset
$ unset PK_TEST
$ osnew provider refresh configure sme-test ... --secret-material-env private_key=PK_TEST
Error:   × --secret-material-env private_key requires local env var 'PK_TEST' to be set to a non-empty value
value round-trips intact to the mint path; status never echoes material
$ openshell provider refresh rotate sme-test --credential-key SME_TEST_TOKEN
Error:   × code: 'Client specified an invalid argument', message: "google_service_account_jwt private_key must be RSA PEM"

$ openshell provider refresh status sme-test
PROVIDER   CREDENTIAL_KEY   STRATEGY                     STATUS  ...  LAST_ERROR
sme-test   SME_TEST_TOKEN   google_service_account_jwt   error   ...  google_service_account_jwt private_key must be RSA PEM

(the fake env-delivered value reached the server and was parsed at mint time — the
transport carried it verbatim; no surface echoes the material itself)

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable) — n/a (user docs updated instead)

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

All contributors have signed the DCO ✍️ ✅
Posted by the DCO Assistant Lite bot.

@hunglp6d

hunglp6d commented Jul 8, 2026

Copy link
Copy Markdown
Author

I have read the DCO document and I hereby sign the DCO.

Signed-off-by: Hung Le <hple@nvidia.com>
@johntmyers johntmyers added gator:watch-pipeline Gator is monitoring PR CI/CD status test:e2e Requires end-to-end coverage labels Jul 8, 2026
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

Label test:e2e applied for d426bb8. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

PR Review Status

Validation: This PR is project-valid because it implements a focused CLI-side improvement for the provider refresh secret-material workflow described in #2104. I checked @johntmyers's maintainer feedback on #2104 about duplicate-key rejection and off-argv secret handling, and @hunglp6d's PR implements the env-based path with duplicate rejection while leaving the file/stdin path for follow-up work.
Head SHA: d426bb84d6ad77cfae9a84c84682e4aaa96ea30e

Review findings:

  • No blocking findings remain.
  • Non-blocking reviewer suggestion: parse_secret_material_env_pairs returns a HashMap, so multiple env-sourced keys may be auto-added to secret_material_keys in nondeterministic order. This appears harmless if the server treats the list as a set, but preserving input order would make stored requests and tests more predictable.

Docs: Updated in docs/sandboxes/manage-providers.mdx; no docs/index.yml navigation change is needed because the page is already under the included sandboxes folder.

E2E: I applied test:e2e because this touches provider credential flow.

Next state: gator:watch-pipeline

@johntmyers johntmyers added gator:merge-ready and removed gator:watch-pipeline Gator is monitoring PR CI/CD status labels Jul 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gator:merge-ready test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants