feat(sdk): add NAS/OSS mount syntax sugar#1282
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Pangjiping
force-pushed
the
feat/sdk-mount-syntax-sugar
branch
2 times, most recently
from
1ca9a5c to
2d972fa
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d972fa7d9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Pangjiping
force-pushed
the
feat/sdk-mount-syntax-sugar
branch
from
2d972fa to
dfec96d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfec96ddab
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return execution; | ||
| } | ||
| const plan = buildOssfs2Plan(options); | ||
| await this.files.writeFiles([buildOssfs2ConfEntry(plan)]); |
There was a problem hiding this comment.
When version: "2.0" is used, this uploads a /tmp/opensandbox-ossfs-*.conf file and then only runs plan.command, so the config is never removed after either a successful mount or a failed command. I checked the existing Docker runtime path, and it removes both passwd and ossfs2 config files in a finally block (server/opensandbox_server/services/docker/ossfs_mixin.py lines 381-390); the new SDK READMEs also promise password/config cleanup. In sandboxes that perform repeated ossfs2 mounts, or callers that pass sensitive raw config options, this leaves stale private config files behind; preserve the mount exit code while deleting plan.confPath (and apply the same behavior to the other SDK implementations).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 73a5af9 across all five SDKs (Kotlin, Python, TypeScript, Go, C#). The ossfs2 mount command is now wrapped as ( ossfs2 mount ... ); __rc=$?; rm -f <confPath>; exit $__rc, mirroring the ossfs1 pattern and the server-side finally block in services/docker/ossfs_mixin.py:381-390. This preserves the ossfs2 exit code (so MountFailedException still surfaces the underlying failure and its stderr) while guaranteeing the /tmp/opensandbox-ossfs-*.conf file is removed on both success and failure. Added a regression assertion in each SDK: ( ossfs2 mount ... ) + __rc=$?; rm -f <confPath>; exit $__rc must be present.
There was a problem hiding this comment.
(re-post) Fixed in 73a5af9 across all five SDKs. The ossfs2 mount command now wraps as ( ossfs2 mount ... ); __rc=$?; rm -f <confPath>; exit $__rc — same pattern as ossfs1 and the server-side finally in services/docker/ossfs_mixin.py:381-390. Exit code preserved, cleanup guaranteed on both success and failure. Regression assertions added in each SDK.
Pangjiping
force-pushed
the
feat/sdk-mount-syntax-sugar
branch
from
dfec96d to
73a5af9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73a5af9332
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const core = | ||
| "ossfs --version && " + | ||
| `install -m 600 /dev/null ${quotedPasswdPath} && ` + | ||
| `printf %s ${shQuote(passwd)} > ${quotedPasswdPath} && ` + |
There was a problem hiding this comment.
When this helper is used, the full shell string is sent to execd, which logs received commands after log.SanitizeCommand (components/execd/pkg/runtime/command.go), and I checked components/execd/pkg/log/sanitize.go: the sanitizer does not match OSS_ACCESS_KEY_SECRET nor the ossfs1 bucket:AK:SK password literal. In any deployment collecting execd logs, every OSS mount can therefore write the caller's OSS secret into logs; upload the secret material as a 0600 file or pass it via non-logged command envs instead of interpolating it into the command string.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 484d79f across all five SDKs (Kotlin, Python, TypeScript, Go, C#). Credentials no longer appear in the shell command text that execd logs.
ossfs 1.x: the bucket:AK:SK[:token] password file is now uploaded via the filesystem API (mode 0600); the shell command only references it via -opasswd_file=<path>. Previously the command contained printf %s 'bucket:AK:SK' > /etc/ossfspass, which leaked AK/SK into execd logs.
ossfs 2.x: a separate env file at /tmp/opensandbox-ossfsenv-<uuid> (mode 0600) now carries OSS_ACCESS_KEY_ID=... / OSS_ACCESS_KEY_SECRET=... / (optional) OSS_SESSION_TOKEN=.... The command sources it with set -a && . <env> && set +a, so the credentials only flow through the env file. Previously the command contained export OSS_ACCESS_KEY_ID='AK' etc.
Cleanup still runs unconditionally (__rc=$?; rm -f <files>; exit $__rc) and each SDK now has a assertNoCredentialsInCommand regression check that fails if printf %s, install -m 600 /dev/null, or export OSS_ACCESS_KEY_* ever reappear in the generated command.
…Script, Go, and C# Expose `sandbox.mount(NfsMountOptions | OssfsMountOptions)` and `sandbox.umount(mountPoint)` on the sandbox facade in all five SDKs. The helpers build the appropriate `mount -t nfs` / `ossfs` / `ossfs2` shell command and run it via `commands.run`, so no server change is required. The sandbox image must have the corresponding mount binary installed, or the options.installation field can install it at mount time. Cross-SDK behavior is aligned so callers can rely on the same semantics: - Version selection: `OssfsMountOptions.version` defaults to ossfs 1.x when omitted, ossfs 2.x when set to "2.0". - `bucketDirectory` is supported in both ossfs 1.x (`bucket:/dir`) and ossfs 2.x (`--oss_bucket_prefix=<dir>/`). - STS `securityToken` appends to the ossfs 1.x password file and exports `OSS_SESSION_TOKEN` for ossfs 2.x. - Failures raise a new `MountFailedException` / `MountFailedError` that extends the SDK's existing sandbox exception hierarchy and carries the failing `Execution` for diagnostics. Security-hardened compared to the internal reference implementation: 1. The ossfs 1.x password file is created with mode 0600 atomically using `install -m 600 /dev/null /etc/ossfspass` before `printf %s <passwd>`, removing the world-readable window a naive `echo > file; chmod 600` would create. 2. The ossfs 1.x cleanup step is wrapped in `( ossfs ... ); __rc=$?; rm -f /etc/ossfspass; exit $__rc` so the password file is unlinked even when the ossfs binary itself exits non-zero. 3. The ossfs 2.x configuration file is written to `/tmp/opensandbox-ossfs-<uuid>.conf` with mode 600 rather than dropped in the current working directory. 4. All shell arguments are single-quoted with proper escaping. While pinning the ossfs 2.x conf mode to 600 the existing SDK convention was verified: `WriteEntry.mode` is serialized as a JSON number and parsed server-side via `strconv.ParseUint(strconv.Itoa(mode), 8, 32)` (see `components/execd/pkg/web/controller/utils.go`), so callers must pass the decimal literal `600` rather than an octal literal such as `0o600` (which would serialize to 384 and fail the octal parse). The Kotlin, TypeScript, Go, and C# implementations all pass `600`; Python passes `mode=600` as before. The `createIsolatedOssSession` helper from the internal Java SDK (bwrap seccomp workaround that pre-mounts OSSFS on the host side and bind-mounts it into the isolated session) is intentionally out of scope of this change and will be added separately if needed. Test results (unit only; e2e requires real NAS/OSS backends): - Kotlin: `./gradlew :sandbox:test` -> 216 pass - Python: `uv run pytest tests/` -> 302 pass - TypeScript: `pnpm run test` -> 70 pass - Go: `go test ./...` -> all pass - C#: not verified locally (no dotnet SDK available on the workstation); code is written to match the existing patterns in the C# SDK and passes static review, but `dotnet test` still needs to run in CI.
Pangjiping
force-pushed
the
feat/sdk-mount-syntax-sugar
branch
from
73a5af9 to
484d79f
Compare
| validateOssfs, | ||
| } from "../dist/internal.js"; | ||
|
|
||
| const OSSFS1_PASSWD_RE = /'\/tmp\/opensandbox-ossfspass-[0-9a-f-]{32,36}'/; |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 484d79fcf0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| def build_ossfs1_passwd_entry(plan: Ossfs1Plan) -> WriteEntry: | ||
| """Convert an :class:`Ossfs1Plan` into a :class:`WriteEntry` for upload.""" | ||
| return WriteEntry(path=plan.passwd_path, data=plan.passwd_content, mode=600) |
There was a problem hiding this comment.
The new mount path sends the OSS AK/SK through files.write_files here, but the current execd upload implementation opens the destination with os.ModePerm and only later applies the requested mode in applyUploadPermission. In sandboxes where another process/user can read /tmp while a mount is starting, mode=600 does not prevent a world-readable window for this password file, and the ossfs2 env file follows the same pattern, so the credential can be scraped before chmod; use a creation path that opens the secret file with 0600 atomically or otherwise avoid the upload endpoint for secret material.
Useful? React with 👍 / 👎.
1 participant
No description provided.