Skip to content

feat(cli): warn when --env values look like credentials - #2655

Open
letv1nnn wants to merge 4 commits into
NVIDIA:mainfrom
letv1nnn:warn-credential-env-vars
Open

feat(cli): warn when --env values look like credentials#2655
letv1nnn wants to merge 4 commits into
NVIDIA:mainfrom
letv1nnn:warn-credential-env-vars

Conversation

@letv1nnn

@letv1nnn letv1nnn commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Summary

openshell sandbox create now emits a non-blocking warning when an --env key looks like a credential, steering users toward providers so secrets aren't exposed to the sandboxed agent. The warning suggests a specific provider create command when the key matches a built-in provider profile, and otherwise links to the providers docs.

Related Issue

#2548

Changes

  • Add credential_env_matches in crates/openshell-cli/src/commands/common.rs: flags --env keys by name only — known provider env vars from builtin_profiles(), plus *_TOKEN, *_SECRET, *_PASSWORD, *_CREDENTIAL, *_ACCESS_KEY, *_SECRET_KEY, *_API_KEY suffixes (case-insensitive). Never inspects or prints values.
  • Add warn_credential_env_vars: prints a non-blocking stderr warning per flagged key. When built-in profiles claim the key it lists a specific openshell provider create command per match (a key can map to several, e.g. GITHUB_TOKEN → copilot and github); otherwise it links to the providers docs.
  • Add the --no-credential-warnings flag to sandbox create and wire the warning in before the CreateSandbox RPC (create only, not exec).
  • Re-export warn_credential_env_vars through run to match the existing parse_env_pairs call pattern.
  • Unit tests in common.rs covering profile match, suffix-only match, case-insensitivity, non-credential skip,
    no-value-leak, and deterministic ordering.
  • Document the warning and flag in docs/sandboxes/manage-sandboxes.mdx.

Example output

image

Testing

  • mise run pre-commit passes
  • Unit tests added/updated
  • E2E tests added/updated (if applicable)

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Publisher/user docs updated (if applicable)
  • Architecture docs updated (if applicable)

Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
@copy-pr-bot

copy-pr-bot Bot commented Aug 7, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@letv1nnn
letv1nnn marked this pull request as draft August 7, 2026 16:09
@letv1nnn
letv1nnn force-pushed the warn-credential-env-vars branch 2 times, most recently from 3d4db33 to 51926bc Compare August 7, 2026 16:32
@letv1nnn
letv1nnn marked this pull request as ready for review August 7, 2026 16:35
Comment thread crates/openshell-cli/src/commands/common.rs Outdated
Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>

@johntmyers johntmyers left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gator-agent

PR Review Status

Validation: This PR is project-valid because it implements the validated, focused CLI security-guidance feature in #2548.
Head SHA: e594d04e077b7b2f32e4b2544f93c3472fcbe802
Base SHA: 4cb77a900ebd6b789d2b68daaba4830866833b1c
Merge base SHA: d85339d621e0e96697499a9d4c8780ee9b9c1324
Patch ID: 9688fd66ee5f7411bea2ff2bf05d783b7faf450f
Gator payload: 3
Review mode: initial
Previous reviewed SHA: none

Thanks @mrunalp — I checked the suggestion to match credential words anywhere in the key against the linked issue, the PR description, the docs, and concrete environment-variable names. That broader match introduces the blocking false-positive case below.

Blocking findings:

  • GATOR-e594d04e-01: substring matching warns on unrelated keys that merely contain a credential word.

Carried findings:

  • None

Non-blocking suggestions:

  • Exercise the externally observable stderr and suppression paths so a future value leak or broken --no-credential-warnings behavior is covered; the current no_value_leak test only inspects the detector's intermediate data.

Docs: Fern user docs are updated in docs/sandboxes/manage-sandboxes.mdx, but their documented suffix behavior currently conflicts with the implementation.

Next state: gator:in-review

Comment thread crates/openshell-cli/src/commands/common.rs Outdated
@johntmyers johntmyers added the gator:in-review Gator is reviewing or awaiting PR review feedback label Aug 10, 2026
@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

Re-check After Maintainer Update

@johntmyers, I checked your August 10 reply that the goal is to scan arbitrary environment-variable names for potential credentials.

Head SHA: e594d04e077b7b2f32e4b2544f93c3472fcbe802
Base SHA: 4cb77a900ebd6b789d2b68daaba4830866833b1c
Merge base SHA: d85339d621e0e96697499a9d4c8780ee9b9c1324
Patch ID: 9688fd66ee5f7411bea2ff2bf05d783b7faf450f
Gator payload: 3

What I checked: #2548 defines the heuristic as exact built-in credential names plus *_TOKEN, *_API_KEY, *_SECRET, *_PASSWORD, *_CREDENTIAL, *_ACCESS_KEY, and *_SECRET_KEY. The PR summary and the Fern docs also describe suffix matching. The current implementation instead matches those words anywhere, so known non-secret names such as TOKENIZERS_PARALLELISM, PASSWORDLESS_LOGIN, and SECRETARY_EMAIL warn.

Disposition: the finding remains unresolved because the implementation is broader than the accepted issue and published behavior, with reachable false positives that can drive warning suppression. If broad substring matching is now intentional, an explicit maintainer waiver plus aligned PR/docs wording would resolve that contract concern; otherwise the code should restore suffix matching and add a false-positive test.

Remaining items:

  • GATOR-e594d04e-01: clarify and align the intended heuristic, implementation, docs, and false-positive coverage.

Next state: gator:in-review

@johntmyers

Copy link
Copy Markdown
Collaborator

@letv1nnn @mrunalp "anywhere in the key" could lead to some false positives. we either should accept this or tighten it up so that we're perhaps looking at word boundaries instead i.e. TOKEN_ or _TOKEN or _TOKEN_. Also the variable name suffixes is now misleading

@johntmyers

Copy link
Copy Markdown
Collaborator

/ok to test e594d04

@letv1nnn

letv1nnn commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

hey @johntmyers @mrunalp, I have the following idea: a few keywords are two words joined by _ (ACCESS_KEY, SECRET_KEY, API_KEY). If I split on _ and check each piece alone, I'd either miss those or flag anything ending in KEY.

So I'll split on _ but match contiguous groups of segments, not single ones. MY_ACCESS_KEY gets flagged, but PRIMARY_KEY / SORT_KEY don't (bare KEY isn't a keyword), and single-word ones like TOKEN still catch TOKEN_*, *_TOKEN. Still one pass per key. Sound right?

@johntmyers

Copy link
Copy Markdown
Collaborator

hey @johntmyers @mrunalp, I have the following idea: a few keywords are two words joined by _ (ACCESS_KEY, SECRET_KEY, API_KEY). If I split on _ and check each piece alone, I'd either miss those or flag anything ending in KEY.

So I'll split on _ but match contiguous groups of segments, not single ones. MY_ACCESS_KEY gets flagged, but PRIMARY_KEY / SORT_KEY don't (bare KEY isn't a keyword), and single-word ones like TOKEN still catch TOKEN_*, *_TOKEN. Still one pass per key. Sound right?

That sounds good to me.

@letv1nnn
letv1nnn requested a review from johntmyers August 10, 2026 23:19
Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
@letv1nnn
letv1nnn force-pushed the warn-credential-env-vars branch from e876779 to 9977c6d Compare August 10, 2026 23:29

@johntmyers johntmyers left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gator-agent

PR Review Status

Validation: This PR is project-valid because it implements the focused credential-warning feature tracked in #2548.
Head SHA: 9977c6d8e1714c87f4ba81077ff6c45238baa41f
Base SHA: c825b1f8efac457f3ca3c6f9e06fb068e8ce3ecc
Merge base SHA: d85339d621e0e96697499a9d4c8780ee9b9c1324
Patch ID: 5cb325a17a09099026968dabd0e0a94f4a8ce09b
Gator payload: 3
Review mode: follow_up
Previous reviewed SHA: e594d04e077b7b2f32e4b2544f93c3472fcbe802

Thanks @letv1nnn. I checked the underscore-segment approach you proposed and @johntmyers approved. The latest head limits matching to whole credential-word segments, adds positive and false-positive regression coverage, and aligns the Fern docs with that behavior.

Blocking findings:

  • No blocking findings remain.

Carried findings:

  • GATOR-e594d04e-01: resolved by the whole-segment matcher, regression tests for TOKENIZERS_PARALLELISM, PASSWORDLESS_LOGIN, and SECRETARY_EMAIL, and updated published documentation.

Docs: Fern user docs are updated in docs/sandboxes/manage-sandboxes.mdx; navigation is unchanged because no page was added or moved.

Next state: gator:watch-pipeline

@johntmyers johntmyers added gator:watch-pipeline Gator is monitoring PR CI/CD status and removed gator:in-review Gator is reviewing or awaiting PR review feedback labels Aug 10, 2026
@johntmyers

Copy link
Copy Markdown
Collaborator

/ok to test 9977c6d

@johntmyers johntmyers added gator:approval-needed Gator completed review; maintainer approval needed test:e2e Requires end-to-end coverage gator:watch-pipeline Gator is monitoring PR CI/CD status and removed gator:watch-pipeline Gator is monitoring PR CI/CD status gator:approval-needed Gator completed review; maintainer approval needed labels Aug 10, 2026
@github-actions

Copy link
Copy Markdown

Label test:e2e applied for 9977c6d. 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 johntmyers added gator:blocked Gator is blocked by process or repository gates gator:watch-pipeline Gator is monitoring PR CI/CD status and removed gator:watch-pipeline Gator is monitoring PR CI/CD status gator:blocked Gator is blocked by process or repository gates labels Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gator:blocked Gator is blocked by process or repository gates test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants