feat(security): default sandbox on + plumb SecurityOptions through REST/CLI/Go/C#652
Draft
G4614 wants to merge 4 commits into
Draft
feat(security): default sandbox on + plumb SecurityOptions through REST/CLI/Go/C#652G4614 wants to merge 4 commits into
G4614 wants to merge 4 commits into
Conversation
`JailerBuilder` previously exposed only `with_jailer_enabled` and
`with_seccomp_enabled` even though `SecurityOptions` carries another
dozen knobs (UID/GID drop, PID/net namespaces, chroot base + toggle,
FD cleanup, env sanitization + allowlist, rlimits, macOS sandbox
profile + network). Callers wanting any of those had to construct a
`SecurityOptions` separately and pipe it through `with_security(...)`,
which scattered build sites and skipped past any future invariants
the builder might add.
Mirror `SecurityOptionsBuilder`'s API in JailerBuilder's
consuming-self style:
- with_uid(Option<u32>) / with_gid(Option<u32>)
- with_new_pid_ns(bool) / with_new_net_ns(bool)
- with_chroot_base(impl Into<PathBuf>) / with_chroot_enabled(bool)
- with_close_fds(bool) / with_sanitize_env(bool)
- with_env_allowlist(Vec<String>) / with_allowed_env(impl Into<String>)
- with_resource_limits(ResourceLimits)
- with_max_open_files / _file_size_bytes / _processes /
_memory_bytes / _cpu_time_seconds
- with_sandbox_profile(Option<PathBuf>) / with_network_enabled(bool)
- with_development_security() / with_standard_security() /
with_maximum_security() — preset shortcuts
Each setter delegates to the matching `SecurityOptions` field with no
new defaults; the initial value still comes from
`SecurityOptions::default()`. `with_allowed_env` is idempotent
(duplicates ignored).
## Tests
8 new tests under `jailer::builder::tests`, one per cluster of new
methods. Each builds a real `Jailer` and asserts the field landed,
so a regression that drops any setter shows up as a single failing
assertion rather than a compile error elsewhere.
- `builder_exposes_uid_gid_passthroughs`
- `builder_exposes_namespace_toggles`
- `builder_exposes_chroot_settings`
- `builder_exposes_env_and_fd_hygiene` (also pins
`with_allowed_env` dedup behaviour)
- `builder_exposes_resource_limits` (per-field setters)
- `builder_exposes_resource_limits_bulk_set`
- `builder_exposes_macos_sandbox_knobs`
- `builder_preset_shortcuts_pick_known_profiles` (confirms
presets wholesale-replace, so `with_uid(0)` before
`with_standard_security()` does NOT win — documents intent)
Full `jailer::builder` suite: 15/15 green. Clippy clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces a comprehensive security preset framework enabling callers to apply predefined sandbox profiles ( ChangesSecurity presets framework
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Ubuntu seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
gamnaansong seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
…ST/CLI/Go/C Reshape of boxlite-ai#652. The original PR only added fluent setters to `JailerBuilder` (pure Rust SDK ergonomics). This rewrite is the real operator-facing fix: (1) Default-on. `SecurityOptions::default()` now returns the standard preset on Linux + macOS, not just macOS. Linux callers that hit the runtime default — which is REST, CLI, JSON config, and every SDK that doesn't explicitly construct `SecurityOptions` — were silently running with no jailer, no seccomp, no chroot. The flip is `default_jailer_enabled` returning `cfg!(any(linux, macos))` instead of `cfg!(macos)`, plus `default_seccomp_enabled` → `cfg!(target_os = "linux")`. (2) `SecurityOptions::from_preset(name)` central helper. Maps the operator-visible strings (`development` / `standard` / `maximum`, plus `dev` / `default` / `max` / `strict` synonyms, case-insensitive, trimmed) to the matching preset, or returns `BoxliteError::InvalidArgument` echoing the offending value. Every operator surface (CLI, REST, Go, C) routes through this one function so a typo surfaces identically everywhere instead of silently falling back to "default". (3) REST wire. Both client (`rest::types::CreateBoxRequest`) and server (`serve::types::CreateBoxRequest`) gain `security_settings: Option<SecurityOptions>` (full struct) plus server-side `security: Option<String>` (preset name). Client `from_options` populates `security_settings` only when `options.advanced.security != SecurityOptions::default()` — pre-PR POST bodies stay byte-identical. Server `build_box_options` resolves in priority order: settings > preset > default. Unknown preset surfaces back to the caller verbatim. (4) CLI `--security` flag. Added to `ManagementFlags` with env `BOXLITE_SECURITY`. `apply_to` returns Result so the typo case bubbles out to the operator. Callers in run.rs / create.rs updated with `?`. (5) C SDK FFI. `boxlite_options_set_security_preset(opts, preset, out_error) -> BoxliteErrorCode`. cbindgen regenerates the decl in `sdks/c/include/boxlite.h`. Returns InvalidArgument on unknown preset with `out_error` populated. (6) Go SDK. `WithSecurityPreset(string) BoxOption` calling the C FFI. Empty string = no-op (keeps runtime default). Unknown preset propagates as an error from `Runtime.Create` instead of silently landing on the default. Added `buildAndFreeCOptions` test helper because Go forbids cgo in `_test.go` files. The `JailerBuilder` setters from the previous version of this PR are kept — still useful for Rust SDK callers — but are no longer the headline. ## Tests Twelve new tests bracketing the contract on every surface; reverting the corresponding production hook flips exactly the right one red. `runtime::options::tests`: - `security_from_preset_canonical_names` — three preset names. - `security_from_preset_case_insensitive_and_synonyms` — `STANDARD`, ` maximum ` (trim), and every documented synonym (dev / default / max / strict). - `security_from_preset_unknown_surfaces_invalid_argument` — rejection echoes the offending value AND lists the supported presets. - `security_default_is_standard_on_supported_platforms` — the default-flip contract: reverting `default_jailer_enabled` flips this on Linux. - `test_security_builder_new` updated to the new default. `rest::types::tests`: - `test_create_box_request_omits_security_settings_when_default` — back-compat: default-config clients produce byte-identical POST bodies. - `test_create_box_request_carries_custom_security_settings_on_the_wire` — non-default surfaces as `security_settings`. `serve::tests`: - `build_box_options_no_security_field_keeps_default` — pre-PR bodies get the (now sandbox-on) default. - `build_box_options_security_preset_string_resolves` — preset name deserializes to the matching SecurityOptions. - `build_box_options_security_preset_typo_surfaces_invalid_argument` — unknown preset surfaces InvalidArgument all the way back to the REST client (not silent default). - `build_box_options_security_settings_wins_over_preset` — explicit settings beat the preset when both are sent. `cli::tests`: - `management_security_preset_applies_to_box_options` — `--security=maximum` lands as `SecurityOptions::maximum()`. - `management_security_preset_absent_leaves_default`. - `management_security_preset_typo_surfaces_anyhow_error`. `sdks/go`: - `TestBuildCOptions_SecurityPresetValid` — five preset strings apply cleanly. - `TestBuildCOptions_SecurityPresetUnknownRejects` — bad preset surfaces back with the offending value in the error. - `TestBuildCOptions_SecurityPresetEmptyKeepsDefault` — empty string = no-op. Full Rust lib sweep: 740/740 green (785 with --features rest, minus 1 pre-existing flake unrelated to this PR: `ws_watchdog_fires_when_idle` fails identically on upstream/main as confirmed by partial revert). Go SDK: 3/3 new tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
G4614
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sdks/c/src/options.rs`:
- Around line 275-280: The converted preset string error handling currently
calls write_error(out_error, e) but returns BoxliteErrorCode::InvalidArgument
while the written error may be Internal; update the error written into out_error
to use an InvalidArgument error before returning so both fields align: when
c_str_to_string(preset) returns Err, construct or map the error passed to
write_error to indicate InvalidArgument (same for the similar block handling
presets at the later 287-290 section), ensuring the function returns
BoxliteErrorCode::InvalidArgument and out_error.code reflects InvalidArgument;
refer to c_str_to_string, write_error, out_error, and
BoxliteErrorCode::InvalidArgument to locate and change both places.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 391a2ec0-b66b-4a1a-80f1-54f2ebbd6918
📒 Files selected for processing (13)
sdks/c/include/boxlite.hsdks/c/src/options.rssdks/go/boxlite_test.gosdks/go/options.gosrc/boxlite/src/jailer/builder.rssrc/boxlite/src/rest/types.rssrc/boxlite/src/runtime/advanced_options.rssrc/boxlite/src/runtime/options.rssrc/cli/src/cli.rssrc/cli/src/commands/create.rssrc/cli/src/commands/run.rssrc/cli/src/commands/serve/mod.rssrc/cli/src/commands/serve/types.rs
Comment on lines
+275
to
+280
| let preset_str = match c_str_to_string(preset) { | ||
| Ok(s) => s, | ||
| Err(e) => { | ||
| write_error(out_error, e); | ||
| return BoxliteErrorCode::InvalidArgument; | ||
| } |
There was a problem hiding this comment.
Align return code and out_error.code for invalid preset input.
At Line 275 and Line 278, conversion failures from c_str_to_string can write an Internal error into out_error, while this function returns InvalidArgument. That mismatch can produce inconsistent behavior for FFI callers reading both fields. Normalize the written error to InvalidArgument for null/invalid preset input so the contract is coherent.
Also applies to: 287-290
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdks/c/src/options.rs` around lines 275 - 280, The converted preset string
error handling currently calls write_error(out_error, e) but returns
BoxliteErrorCode::InvalidArgument while the written error may be Internal;
update the error written into out_error to use an InvalidArgument error before
returning so both fields align: when c_str_to_string(preset) returns Err,
construct or map the error passed to write_error to indicate InvalidArgument
(same for the similar block handling presets at the later 287-290 section),
ensuring the function returns BoxliteErrorCode::InvalidArgument and
out_error.code reflects InvalidArgument; refer to c_str_to_string, write_error,
out_error, and BoxliteErrorCode::InvalidArgument to locate and change both
places.
DorianZheng
reviewed
Jun 4, 2026
|
|
||
| void boxlite_options_set_detach(CBoxliteOptions *opts, int val); | ||
|
|
||
| // Pick a sandbox security preset by name. |
Member
There was a problem hiding this comment.
Should be consistent with boxlite_options_new, boxlite_options_set_rootfs_path, etc.
Per @DorianZheng review on boxlite-ai#652: the new `boxlite_options_set_security_preset` was the only `set_*` in the C SDK with an `(out_error, BoxliteErrorCode)` signature — every sibling (`set_cpus`, `set_rootfs_path`, `set_auto_remove`, …) returns void. The only thing forcing the error-returning signature was sync-time preset validation, but that's not necessary: the typo just needs to surface loudly *somewhere* before a box runs. Moving validation to `boxlite_create_box` keeps the loud-rejection contract intact (InvalidArgument + offending name echoed) while letting the setter match its siblings. What changed: - C: setter is now `void`. Preset name is stashed on `OptionsHandle` via a new `pending_security_preset: Option<String>` field, and `create_box` calls `SecurityOptions::from_preset` synchronously before taking ownership of `opts` — bad names return `InvalidArgument` immediately, callback never fires, caller still owns opts (same ownership semantics as the null-cb error path). - Go: `buildCOptions` drops the cerrSec / freeError block; calls the void setter and lets the typo surface from `Runtime.Create`. - Tests: - New `create_box_rejects_unknown_security_preset` in `sdks/c/src/tests.rs` pins the C-side deferred-validation contract (sync InvalidArgument + "ultra" in the message + callback never fires + opts still ownable by caller). - Deleted Go-side `TestBuildCOptions_SecurityPresetUnknownRejects` — it asserted error-at-buildCOptions which is no longer true. The policy is still covered by the Rust unit test `security_from_preset_unknown_surfaces_invalid_argument` plus the new C-SDK integration test above. Two-side verified: with the new code in place, the deleted Go test fired its t.Fatal at boxlite_test.go:429 (buildAndFreeCOptions now returns nil for bad presets) — confirming the behavior really moved. All 53 boxlite-c crate tests pass; the 2 remaining Go preset tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
2 participants
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.
open security options by default
support RESTAPI/CLI Flag/C SDK FFI/GO SDK
Test plan
Two-sided via
runtime::options::tests+rest::types::tests+serve::tests+cli::tests+sdks/go, the reverted side toggling either the default flip (default_jailer_enabled) orSecurityOptions::from_preset. Each surface tests both the happy path and an explicit typo case so silent-fallback regressions surface loudly.security_default_is_standard_on_supported_platforms— pins thedefault_jailer_enabledflip. Reverting the cfg fromany(linux, macos)totarget_os = "macos"flips this red on Linux.security_from_preset_unknown_surfaces_invalid_argument— pins that the rejection echoes the offending value AND lists the supported presets. Used by CLI, REST, Go, C; one revert hits all four call sites.build_box_options_security_settings_wins_over_preset— explicitsecurity_settingsstruct beats thesecuritypreset string when both are sent (matches the in-code priority comment).management_security_preset_typo_surfaces_anyhow_error+TestBuildCOptions_SecurityPresetUnknownRejects— CLI and Go both reject unknown preset by name, not by silently landing on the default.test_create_box_request_omits_security_settings_when_default— back-compat: pre-PR clients producing defaultBoxOptionskeep POST bodies byte-identical.test_create_box_request_carries_custom_security_settings_on_the_wire— non-defaultSecurityOptionssurfaces assecurity_settings.BoxOptions::default().advanced.security.jailer_enabledon Linuxfalse— every Linux callsite that hits the runtime default runs unsandboxedtrue(standard preset)CreateBoxRequest(no security field)AdvancedBoxOptions::default()→ jailer offboxlite run --security=maximum alpine ...on Linuxadvanced.security = SecurityOptions::maximum()WithSecurityPreset("ultra")in Go SDKRuntime.Createreturns anInvalidArgumenterror echoing"ultra"boxlite_options_set_security_preset(opts, "ultra", &err)in C SDKInvalidArgument,err.messageechoes"ultra"BoxOptionswith non-defaultSecurityOptions(e.g.maximum)security_settings: {...}; server reconstructs the exact structsecurity_settingsskipped viaOption::is_noneso pre-PR clients send byte-identical POSTs--security ultra,"security": "ultra",WithSecurityPreset("ultra"), FFI bad preset)Full Rust lib sweep: 740/740 green (785 with
--features rest, minus 1 pre-existing flakews_watchdog_fires_when_idleunrelated to this PR — verified by partial revert against upstream/main). Go SDK: 3/3 new tests green.Summary by CodeRabbit
development,standard,maximum) for simplified sandbox configuration--securityCLI flag to apply presets