Skip to content

feat(sandbox): add --no-login-shell to skip shell startup files on exec - #2852

Open
letv1nnn wants to merge 5 commits into
NVIDIA:mainfrom
letv1nnn:feat-sandbox-exec-no-login-shell
Open

feat(sandbox): add --no-login-shell to skip shell startup files on exec#2852
letv1nnn wants to merge 5 commits into
NVIDIA:mainfrom
letv1nnn:feat-sandbox-exec-no-login-shell

Conversation

@letv1nnn

@letv1nnn letv1nnn commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Summary

sandbox exec always ran commands through a login shell (bash -lc), so sandbox-user startup files (.profile, .bash_profile, .bashrc) were sourced before the requested command — letting them alter its output, create files, or block it, which made exec unsuitable as a trusted execution boundary for automation and managed checks. This adds an opt-out (--no-login-shell / no_login_shell) that runs the command without sourcing those files, while the login-shell default is preserved for existing callers.

Related Issue

Closes #2668

Changes

  • Add no_login_shell field to ExecSandboxRequest in proto/openshell.proto (default false preserves current login-shell behavior).
  • Gateway (openshell-server): read the field and signal it to the supervisor over an SSH OPENSHELL_NO_LOGIN_SHELL env request, sent before exec on both the non-interactive and interactive russh paths.
  • Supervisor (openshell-supervisor-process): store the flag from env_request, thread it through start_shell to the PTY and pipe spawn sites, and select bash -c instead of bash -lc when set.
  • CLI (openshell-cli): add --no-login-shell flag to sandbox exec, wired through to both the streaming and interactive request builders.
  • SDKs: expose the option in the Rust (ExecOptions.no_login_shell), Go (ExecOptions.NoLoginShell), Python (no_login_shell kwarg), and TypeScript (noLoginShell) exec APIs. - Docs: document the flag and tradeoff in docs/sandboxes/manage-sandboxes.mdx; note the login-shell default and SSH env carrier in architecture/sandbox.md.

Reproduction

Podman-backed sandbox with a profile marker seeded in ~/.bash_profile (echo PROFILE-RAN):

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)
  • Architecture docs updated (if applicable)

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

copy-pr-bot Bot commented Aug 20, 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.

…o-login-shell

Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
@letv1nnn
letv1nnn marked this pull request as ready for review August 21, 2026 11:05
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

An operator who upgrades the gateway while an existing sandbox keeps its older supervisor can request non-login execution, but that supervisor accepts and ignores the new signal, so user-controlled startup files still run without any error.

Action required: add supervisor capability negotiation and fail closed for no_login_shell when the connected supervisor does not advertise support, with coverage for unary and interactive exec.

Blocking findings:

  • GATOR-7d79a247-01: the opt-out silently falls back to a login shell with a pre-change supervisor.

Carried findings:

  • None
Gator metadata
  • Validation: Project-valid through accepted issue #2668.
  • Docs: Fern sandbox documentation and architecture documentation are updated.
  • Checks: Current head has passing DCO and status-publication checks; required workflows have not yet been dispatched.
  • E2E: Required for the sandbox/gateway/supervisor behavior change after review feedback is resolved.
  • Head SHA: 7d79a247134fd446e9146759b92a6f423b6b7b45
  • Base SHA: 20d2e867e0e25b24d383a78dd362ba5647ef12c8
  • Merge base SHA: 20d2e867e0e25b24d383a78dd362ba5647ef12c8
  • Patch ID: 24c2ca391d463bf4a7214e11fb9274058913da31
  • Gator payload: 7
  • Review mode: initial
  • Previous reviewed SHA: none
  • Review budget exhausted: no
  • Maintainer decision required: no
  • Next state: gator:in-review

Comment thread crates/openshell-server/src/grpc/sandbox.rs
@johntmyers johntmyers added the gator:in-review Gator is reviewing or awaiting PR review feedback label Aug 24, 2026
@letv1nnn

letv1nnn commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

hey @johntmyers, summarizing what we've got.

I don't think pre-0.1.0 fully covers us here, since it's less a versioning thing than a timing one. The supervisor is baked into each sandbox and never updates, while the gateway upgrades separately, so a sandbox created just before a gateway upgrade keeps its old supervisor. That can show up in a normal deploy, not only across releases.

When it does, the old supervisor quietly ignores no_login_shell, runs bash -lc, sources the user's profile, and the caller still gets a clean success. A security opt-out that silently no-ops on exec seems worth avoiding, so my lean is to handle it now, but you know the roadmap better, so if you'd rather punt it I'm fine with that too.

Then there are two possibilities to solve the problem.

  1. Introduce SupervisorHello capability negotiation (what gator flagged). Supervisor advertises a set of supported capabilities (e.g. ["no_login_shell", ...]), gateway retains it with the live session, and rejects no_login_shell with FailedPrecondition when the capability is absent.
    • Not lightweight, since it adds a handshake/protocol surface.
    • But it's the right shape if more gateway <-> sandbox SSH features are coming. Each future skew-sensitive flag just adds a capability string and reuses the same negotiation and the same FailedPrecondition path. Pay the cost once.
  2. SSH identification banner version gate. The supervisor already exchanges an SSH identification string (SSH-2.0-<software>) at connection start, before we send the exec. Supervisor stamps its OpenShell version there; gateway parses the remote id and gates no_login_shell on it, rejecting with FailedPrecondition when the version is too old.
    • No new protocol message, no proto field, no extra round-trip, reuses the banner every SSH connection already sends.
    • Narrower: it carries a version, not a capability list, so every future feature needs its own "added in version ≥ X" mapping on the gateway side. I checked russh 0.62 and both sides are supported, the supervisor can set server::Config.server_id and the client exposes Session::remote_sshid(). One wrinkle: remote_sshid() lives on the client Session, not the Handle, so the gateway has to capture it in a Handler callback and thread it out to the exec path, a bit of glue, not a free getter.

Worth noting: this isn't exec-only. There are already two of these signals riding SSH env requests, no_login_shell (exec) and main_read_only (main-attach), so the skew spans more than one pipeline. That's why I'd lean toward a session-scoped capability set that every pipeline gates against, rather than something exec-specific.

My lean is that if we expect the gateway↔sandbox SSH surface to keep growing (more flags like this), the first option is worth building now, it turns "every new feature re-solves skew" into "every new feature adds one string." If this stays a one-off, then the second option is the cheaper fail-closed fix. Either way the outcome is the same fail-closed behavior: reject rather than silently fall back to the login shell.

So my suggestion: land a minimal fail-closed gate here so this PR doesn't ship the silent fallback, and split the general session-scoped handshake into its own issue.

Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

Author Follow-Up Nudge

This PR has been in gator:in-review for more than 48 business hours with unresolved review feedback.

@letv1nnn, please respond to the review comments or push an update. The pending action is to implement a fail-closed compatibility gate for no_login_shell on older supervisors, with unary and interactive exec coverage, or obtain an explicit maintainer waiver. If this is no longer planned, please say so and a maintainer can close it out.

@letv1nnn

letv1nnn commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the nudge, this is still on, just pushed the fix.

Went with the fail-closed gate (option 2 from my earlier comment). The supervisor now stamps its version into the SSH banner (SSH-2.0-OpenShell_<version>), and the gateway reads that on kex_done. If a sandbox's supervisor predates the feature, --no-login-shell now fails with FailedPrecondition rather than silently dropping back to the login shell. Both exec paths are covered, unary and interactive.

Filed the broader fail-closed capability negotiation (incl. main_read_only and the other SSH pipelines) as #2949; this PR ships the minimal per-feature banner gate for no_login_shell.

@letv1nnn
letv1nnn requested a review from johntmyers August 27, 2026 11:39
@johntmyers johntmyers added the test:e2e Requires end-to-end coverage label Aug 27, 2026
@github-actions

Copy link
Copy Markdown

Label test:e2e applied, but pull-request/2852 does not exist yet. A maintainer needs to comment /ok to test 6fa8778bb206cec519f63fd4a6cf144b37738a00 to mirror this PR. Once the mirror exists, re-apply the label or re-run Branch E2E Checks from the Actions tab.

@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

Re-check After Author Update

Thanks @letv1nnn. I checked your current-head SSH banner gate against the older-supervisor fallback you described: both unary and interactive exec now reject --no-login-shell with FailedPrecondition unless the connected supervisor advertises the new OpenShell banner. The prior compatibility finding is resolved, and I found no new blocking issues in the author-only delta.

Blocking findings:

  • No blocking findings remain

Carried findings:

  • GATOR-7d79a247-01: resolved by the current-head fail-closed banner gate and dual-path coverage
Gator metadata
  • Validation: Project-valid through accepted issue #2668.
  • Docs: Fern sandbox documentation and architecture documentation are updated.
  • Checks: DCO and status publication pass; required branch, Helm, and E2E workflows are not yet dispatched for the current head.
  • E2E: test:e2e applied; E2E Label Help is running and its instructions are pending.
  • Head SHA: 6fa8778bb206cec519f63fd4a6cf144b37738a00
  • Base SHA: 20d2e867e0e25b24d383a78dd362ba5647ef12c8
  • Merge base SHA: 20d2e867e0e25b24d383a78dd362ba5647ef12c8
  • Patch ID: ccf68c26397501a27c81465b03c96c858e410e25
  • Gator payload: 7
  • Review mode: follow_up
  • Previous reviewed SHA: 7d79a247134fd446e9146759b92a6f423b6b7b45
  • Review budget exhausted: no
  • Maintainer decision required: no
  • Next state: gator:in-review

@johntmyers

Copy link
Copy Markdown
Collaborator

/ok to test 6fa8778

@johntmyers johntmyers added gator:watch-pipeline Gator is monitoring PR CI/CD status gator:approval-needed Gator completed review; maintainer approval needed and removed gator:in-review Gator is reviewing or awaiting PR review feedback gator:watch-pipeline Gator is monitoring PR CI/CD status labels Aug 27, 2026
@letv1nnn
letv1nnn requested a review from johntmyers August 27, 2026 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gator:approval-needed Gator completed review; maintainer approval needed test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sandbox exec always runs commands through a login shell, so sandbox-user startup files run before the requested command

2 participants