Repository navigation
feat(sandbox): 接頭辞付きの credential 別名を placeholder と同じ秘密に解決する - #4
Merged
striderkein merged 4 commits intoOct 9, 2026
Merged
Conversation
The direct-TCP tests call std::env::set_var / remove_var directly. These are unsafe in edition 2024, so `cargo test -p openshell-sandbox --lib` failed to compile (19 errors in netns.rs and child_env.rs) and no sandbox test could run. Wrap each call in an unsafe block; behavior is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Port the alias resolution from NVIDIA#1286 (header and request-line scope only). A client that validates the credential's shape before sending cannot carry the canonical `openshell:resolve:env:KEY` placeholder; for example gh's `--attach` only accepts github_pat_ / ghp_ / gho_ / ghu_ tokens. Such a client can send `<prefix>OPENSHELL-RESOLVE-ENV-KEY` (e.g. `github_pat_OPENSHELL-RESOLVE-ENV-GITHUB_TOKEN`), and the proxy now resolves it to the same secret as the canonical placeholder. - resolve_placeholder falls back to the alias when there is no exact match. The alias must be the whole token: a non-empty prefix of RFC 3986 unreserved characters, the marker, and an env key running to the end. - The fail-closed scans after rewriting also reject a remaining alias marker in the header block and in the percent-decoded request line, so an unresolved alias is never forwarded upstream. The child env keeps the canonical placeholder; callers opt in to an alias per invocation (GH_TOKEN=github_pat_OPENSHELL-RESOLVE-ENV-GITHUB_TOKEN gh ...). Aliases inside Basic credentials are not resolved; git keeps using the canonical placeholder. Refs simount/NemoClaw-on-AWS#254 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
striderkein
marked this pull request as ready for review
October 9, 2026 03:08
striderkein
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
fail-closed 検査の迂回経路と、並列テストで未同期の unsafe な環境変数操作が残っています。
3 open findings
What changed in this PR
Sandbox の credential alias 解決を追加し、特定クライアントの事前 token 検証に対応する変更です。
Changes:
- provider-shaped alias の解析・秘密解決を追加
- 未解決 alias の fail-closed 検査とテストを追加
- Rust 2024 向けに環境変数操作を
unsafe化
| File | Description |
|---|---|
crates/openshell-sandbox/src/secrets.rs |
alias 解決、fail-closed 検査、テストを追加 |
crates/openshell-sandbox/src/sandbox/linux/netns.rs |
テスト内の環境変数操作を更新 |
crates/openshell-sandbox/src/child_env.rs |
テスト内の環境変数操作を更新 |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
kosaku-sim
approved these changes
Oct 9, 2026
…d request target The forward-proxy path (rewrite_forward_request) still scanned only for the canonical placeholder after rewriting, so an unknown alias in a header, or any alias in the request line, was forwarded upstream. rewrite_target_for_eval also let an alias in the target through to OPA. - rewrite_forward_request: reject a remaining placeholder or alias marker in the output, and in the raw and percent-decoded request line - rewrite_target_for_eval: reject an alias marker in the target (raw and percent-decoded). Aliases are resolved in headers only, so one in the target would never be resolved - share the marker checks (contains_credential_marker, request_line_has_credential_marker) and drop the now-unused PLACEHOLDER_PREFIX_PUBLIC Addresses review: #4 (comment) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…vars Wrapping set_var / remove_var in unsafe blocks did not meet their safety contract: tests run in parallel, and the tests in child_env.rs and sandbox/linux/netns.rs read and write the same variables. Every such test now holds a shared lock (child_env::lock_direct_tcp_env) for its whole body, including the ones that only read through build_no_proxy / proxy_env_vars. Addresses review: #4 (comment) #4 (comment) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
striderkein
merged commit Oct 9, 2026
68ac1c7
into
fix/basic-auth-credential-resolve
4 of 9 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概要
upstream NVIDIA#1286 の「接頭辞付きの別名（provider-shaped alias）の解決」を、本番のビルド元のブランチへ移植する（ヘッダとリクエスト行に限る）。proxy は
<接頭辞>OPENSHELL-RESOLVE-ENV-<KEY>（例:github_pat_OPENSHELL-RESOLVE-ENV-GITHUB_TOKEN）を、正規の placeholderopenshell:resolve:env:<KEY>と同じ秘密に差し替える。送信前に credential の形を検査するクライアントを、sandbox の中で使えるようにするための変更である。直接のきっかけは
gh pr comment --attach（gh v2.99.0 以降）で、token がgithub_pat_/ghp_/gho_/ghu_のいずれかで始まらないと、通信する前に拒否する。正規の placeholder はこの検査を通らない。関連 Issue
simount/NemoClaw-on-AWS#254（AutoDev の受け入れ試験のエビデンスを、sandbox の中から PR に添付する）。進め方は https://github.com/simount/NemoClaw-on-AWS/issues/254#issuecomment-6052307911 で合意した（fork に移植し、upstream には追従しない）。
変更内容
49ce5baa）netns.rsとchild_env.rsの direct TCP のテストが、std::env::set_var/remove_varをそのまま呼んでいる。edition 2024 ではこれらが unsafe になったため、cargo test -p openshell-sandbox --libがコンパイルできず（19 件のエラー）、このブランチでは sandbox のテストを 1 本も実行できない状態だった。各呼び出しをunsafeブロックで囲んだ。動作は変わらない78010598、crates/openshell-sandbox/src/secrets.rs）resolve_placeholderは、完全一致する placeholder が無いとき、別名として解釈して引き直す。別名と認めるのは値まるごとが次の形のときだけ: 1 文字以上の接頭辞（RFC 3986 の非予約文字A-Za-z0-9_-.~）、目印OPENSHELL-RESOLVE-ENV-、末尾まで続く環境変数名（upstream のalias_env_keyと同じ規則）GH_TOKEN=github_pat_OPENSHELL-RESOLVE-ENV-GITHUB_TOKEN gh pr comment --attach ...）テスト
pf のホスト（aarch64）の
rust:1.88-bookwormコンテナで実行した。新しいテストだけを入れた状態で
cargo test -p openshell-sandbox --lib secrets::を実行し、追加した 12 件のうち 6 件が失敗することを確認した（RED）。実装を入れた後は全件通るcargo test -p openshell-sandbox --lib: 456 件通過、失敗 0、無効 1。なおbaseline_tests::enrich_proto_baseline_paths_adds_pty_paths_for_proxy_modeは、テスト環境に/sandboxが無いと失敗する（既存のテストで環境に依存する。mkdir -p /sandboxすると通る）cargo build --release -p openshell-sandbox: 成功（4 分 18 秒）。バイナリに別名の目印とOPENSHELL_DIRECT_TCP_ENDPOINTSの両方が含まれることを確認した。まだ配布していない追加したテストの内容: 別名が正規の秘密に解決される、
token <別名>のヘッダの差し替え、接頭辞の非予約文字、未知の KEY、空の接頭辞、接頭辞の予約文字、KEY の後ろの余分な文字、解決した秘密に CRLF を含む場合、ヘッダブロックの往復、ヘッダとリクエスト行（パーセントエンコードを含む）に解決できない別名が残ったときの fail-closed、正規の placeholder が引き続き解決されることfork の CI は、ビルドの job が upstream 用の self-hosted runner（
build-amd64/build-arm64）を要求するため、fork では実行されない（queued のまま）mise run pre-commitが通る（未実行。ビルドしたホストに mise を入れていない）ユニットテストを追加・更新した
E2E テストを追加・更新した（該当する場合）
チェックリスト
🤖 Generated with Claude Code