feat(container): add gVisor sandbox support for Docker containers#74
feat(container): add gVisor sandbox support for Docker containers#74andybons wants to merge 16 commits into
Conversation
Consolidate the Linux installation steps into a single command that users can copy and run directly. Also adds -y flag to apt install and includes systemctl reload step.
andybons
force-pushed
the
feat/gvisor-integration-clean
branch
from
f2d0878 to
a4cd566
Compare
📦 Previous Review (superseded by newer review)Pull Request Review: gVisor Sandbox SupportThis PR adds gVisor sandbox support for Docker containers, making it the default runtime with an opt-out mechanism. The implementation is well-structured and follows good security practices. Below is my detailed feedback. Code Quality & Best PracticesStrengths:
Areas for improvement:
Potential Bugs & Issues
Security ConcernsPositive findings:
Recommendation:
Performance Considerations
Test CoverageCurrent state:
Recommendations:
DocumentationStrengths:
Minor issues:
SummaryOverall assessment: This is a well-implemented security feature with clear benefits. The code quality is high, and the design decisions are sound. Critical issues: None blocking Important improvements:
Minor improvements:
Recommendation: Approve with suggested improvements. The critical path (detection, runtime selection, container creation) is solid. The improvements suggested above would enhance maintainability and robustness but aren't blocking for merge. Verification ChecklistBefore merging, verify:
Great work on this security enhancement! 🔒 |
There was a problem hiding this comment.
Pull request overview
Adds secure-by-default gVisor (runsc) sandboxing for Docker-based runs, with CLI/config escape hatches to disable sandboxing when needed.
Changes:
- Introduces sandbox-aware runtime creation (
RuntimeOptions) and wires--no-sandboxthrough the run lifecycle. - Updates Docker container creation to set
HostConfig.Runtimetorunsc(or non-sandbox runtime when opted out) and adds gVisor availability checks with install guidance. - Extends config/docs to support persistent opt-out via
sandbox: noneand documents--no-sandbox.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/run/manager.go | Adds manager options and passes sandbox preference into container runtime selection. |
| internal/container/docker.go | Implements gVisor detection, runtime selection, and sets Docker HostConfig.Runtime. |
| internal/container/detect.go | Adds RuntimeOptions plumbing and exposes GVisorAvailable. |
| internal/container/detect_test.go | Adds a basic test for GVisorAvailable. |
| internal/container/docker_list_test.go | Updates integration tests to use non-sandbox Docker runtime. |
| internal/config/config.go | Adds top-level sandbox configuration field for agent.yaml. |
| cmd/moat/cli/exec.go | Adds --no-sandbox flag and wires it into manager creation. |
| cmd/moat/cli/run.go | Honors sandbox: none from agent.yaml by setting --no-sandbox behavior. |
| docs/content/reference/01-cli.md | Documents --no-sandbox flag and adds examples. |
| docs/content/reference/02-agent-yaml.md | Documents sandbox: none configuration option. |
| docs/plans/2026-01-25-gvisor-integration-design.md | Design doc describing the intended gVisor integration. |
| docs/plans/2026-01-25-gvisor-implementation.md | Implementation plan for the gVisor integration work. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
|
|
||
| func TestGVisorAvailable(t *testing.T) { | ||
| // This test verifies the function exists and returns a boolean. | ||
| // Actual gVisor detection depends on Docker daemon configuration. | ||
| ctx := context.Background() |
There was a problem hiding this comment.
Test uses context.Background() when calling GVisorAvailable(), which may block indefinitely if the Docker daemon/socket is unreachable. Use a context with a short timeout (or a cancellable context) in the test to prevent CI hangs.
| ) | |
| func TestGVisorAvailable(t *testing.T) { | |
| // This test verifies the function exists and returns a boolean. | |
| // Actual gVisor detection depends on Docker daemon configuration. | |
| ctx := context.Background() | |
| "time" | |
| ) | |
| func TestGVisorAvailable(t *testing.T) { | |
| // This test verifies the function exists and returns a boolean. | |
| // Actual gVisor detection depends on Docker daemon configuration. | |
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | |
| defer cancel() |
| // DefaultRuntimeOptions returns the default runtime options. | ||
| func DefaultRuntimeOptions() RuntimeOptions { | ||
| return RuntimeOptions{Sandbox: true} | ||
| } |
There was a problem hiding this comment.
DefaultRuntimeOptions() hard-enables Sandbox for all callers of container.NewRuntime(), but several non-run commands call NewRuntime without any way to opt out (e.g., cmd/moat/cli/status.go:64, cmd/moat/cli/clean.go:40, cmd/moat/cli/system_images.go:34). On a Docker host without runsc configured, these commands will now fail even though --no-sandbox only exists on moat run. Consider adding a global opt-out (env var / global config) that NewRuntime() honors, or updating those commands to call NewRuntimeWithOptions when sandbox should be disabled.
| To bypass (reduced isolation): | ||
| moat run --no-sandbox`) |
There was a problem hiding this comment.
ErrGVisorNotAvailable message’s bypass instructions are specific to moat run --no-sandbox, but this error can surface from other commands that initialize the Docker runtime (e.g., moat status, moat clean). Consider making the bypass guidance command-agnostic and also mentioning the persistent opt-out (sandbox: none in agent.yaml, or a global opt-out if added) so users aren’t blocked when running non-run commands.
| To bypass (reduced isolation): | |
| moat run --no-sandbox`) | |
| To bypass sandboxing (reduced isolation): | |
| - For a single command, disable sandboxing for that invocation (for example by using a no-sandbox flag if supported). | |
| - To persistently disable sandboxing, set 'sandbox: none' in your agent.yaml configuration.`) |
| ociRuntime := "runsc" | ||
| if !sandbox { | ||
| log.Warn("running without gVisor sandbox - reduced isolation") | ||
| ociRuntime = "runc" | ||
| } else { |
There was a problem hiding this comment.
When sandbox is disabled, this hard-codes the runtime to "runc". That overrides the Docker daemon’s default runtime (which may be something else, e.g., nvidia), and could change behavior unexpectedly. If the goal is only to avoid runsc, consider leaving HostConfig.Runtime unset (empty string) when sandbox=false, or explicitly using Docker’s reported default runtime instead of forcing "runc".
| info, err := cli.Info(ctx) | ||
| if err != nil { | ||
| cli.Close() | ||
| return nil, fmt.Errorf("checking Docker info: %w", err) | ||
| } | ||
|
|
||
| found := false | ||
| for name := range info.Runtimes { | ||
| if name == "runsc" { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if !found { |
There was a problem hiding this comment.
gVisor detection logic here duplicates internal/container/detect.go:GVisorAvailable(). To reduce drift and make this easier to test/maintain, consider reusing the shared helper (or refactor it to accept a Docker client) instead of re-implementing the Info/Runtimes scan in multiple places.
| info, err := cli.Info(ctx) | |
| if err != nil { | |
| cli.Close() | |
| return nil, fmt.Errorf("checking Docker info: %w", err) | |
| } | |
| found := false | |
| for name := range info.Runtimes { | |
| if name == "runsc" { | |
| found = true | |
| break | |
| } | |
| } | |
| if !found { | |
| available, err := GVisorAvailable(ctx) | |
| if err != nil { | |
| cli.Close() | |
| return nil, fmt.Errorf("checking gVisor availability: %w", err) | |
| } | |
| if !available { |
Commands like 'moat status', 'moat clean', 'moat system images', and 'moat system containers' only query/manage existing resources - they don't create new containers that need sandboxing. These commands now explicitly use Sandbox: false to avoid requiring gVisor when it's not needed.
- Deduplicate gVisor detection: NewDockerRuntime now uses GVisorAvailable() - Use Docker's default runtime when sandbox disabled (empty string vs 'runc') - Add config validation for sandbox field (only '' or 'none' allowed) - Add timeout to TestGVisorAvailable to prevent CI hangs - Add tests for sandbox config validation
📦 Previous Review (superseded by newer review)Pull Request Review: gVisor Sandbox IntegrationThis PR adds gVisor sandboxing as the default runtime for Docker containers. The implementation is well-structured and follows project conventions. Code Quality - StrongArchitecture: Clean separation with RuntimeOptions struct, proper functional options pattern, backward compatibility maintained. Error Handling: ErrGVisorNotAvailable provides actionable instructions, proper context timeouts, resource cleanup on errors. Key Issues1. Duplicate gVisor Detection Logic - Check duplicated in NewDockerRuntime and GVisorAvailable function. Consider refactoring with internal helper. 2. Test Coverage Gap - Only smoke test exists. Missing tests for mock clients, error handling, flag behavior, config parsing. 3. Resource Cleanup - cli.Close() errors silently ignored in error paths. Consider logging failures. 4. Context Propagation - GVisorAvailable accepts context but Docker client creation doesn't use it. Security ReviewSecure by Default: gVisor required unless opted out, clear warnings, no automatic fallback. No new attack surface. Concerns: None identified. PerformanceDetection adds ~5s timeout when gVisor unavailable but mitigated by fast daemon response. gVisor adds ~5-15% syscall overhead which is acceptable for security benefits. DocumentationCLI and agent.yaml references are clear with good examples. Design docs are comprehensive. SummaryStrengths: Clean architecture, secure by default, excellent error messages, comprehensive docs, backward compatible. Areas for Improvement: Deduplicate detection logic, add unit/integration tests, consider caching. Recommendation: Approve with minor improvements. Core implementation is solid. Suggestions are non-blocking and could be follow-up PRs. Overall: 8.5/10 - Well-executed feature with minor refinement opportunities. |
📦 Previous Review (superseded by newer review)Pull Request Review: gVisor Sandbox SupportSummaryThis PR adds gVisor (runsc) as the default OCI runtime for Docker containers, providing kernel-level isolation through a userspace kernel. The implementation is well-structured and follows the codebase conventions. However, there are several important issues that should be addressed before merging. Critical Issues1. Missing gVisor Documentation in Sandboxing Concepts 🔴The new Location: Issue: The sandboxing concepts page discusses Docker and Apple containers but never mentions:
Recommendation: Add a dedicated section explaining:
Example addition: ## gVisor Sandbox (Docker only)
When using Docker, Moat runs containers with gVisor (runsc) by default. gVisor provides an additional layer of isolation by intercepting syscalls and implementing them in a userspace kernel (Sentry).
### Why gVisor?
Standard Docker containers share the host kernel. gVisor adds defense-in-depth by inserting a userspace kernel between the container and the host, reducing the attack surface for container escape vulnerabilities.
### Compatibility
Some workloads use syscalls that gVisor doesn't support. If you encounter syscall-related errors, use `--no-sandbox` to disable gVisor:
```bash
moat run --no-sandbox ./my-projectYou can also set Major Issues3. No E2E Tests for gVisor Functionality 🟡Issue: There are no end-to-end tests verifying that:
Current test coverage:
Recommendation: Add E2E tests in func TestGVisorEnabled(t *testing.T) {
// Create container with sandbox enabled
// Inspect container to verify Runtime: "runsc"
}
func TestNoSandboxFlag(t *testing.T) {
// Create container with --no-sandbox
// Verify Runtime field is empty (default runc)
}4. Inconsistent Error Handling in NewDockerRuntime 🟡Location: var ociRuntime string // empty string = Docker's default runtime
if \!sandbox {
log.Warn("running without gVisor sandbox - reduced isolation")
// Leave ociRuntime empty to use Docker's default (usually runc)
} else {
// Verify gVisor is available using shared detection function
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
if \!GVisorAvailable(ctx) {
cli.Close()
return nil, fmt.Errorf("%w", ErrGVisorNotAvailable)
}
ociRuntime = "runsc"
}Issues:
Recommendation: Be explicit about the runtime in both cases: var ociRuntime string
if \!sandbox {
log.Warn("running without gVisor sandbox - reduced isolation")
ociRuntime = "runc" // Explicitly request standard runtime
} else {
// ... gVisor detection ...
ociRuntime = "runsc"
}Alternatively, if the intent is to use Docker's default (which might be runsc on some systems), document this explicitly and explain why. 5. GVisorAvailable Creates New Client on Every Call 🟡Location: func GVisorAvailable(ctx context.Context) bool {
cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation())
if err \!= nil {
return false
}
defer cli.Close()
// ...
}Issue: This function creates a new Docker client every time it's called, even though Current usage in docker.go:71: if \!GVisorAvailable(ctx) {
cli.Close() // We close the cli we just created
return nil, fmt.Errorf("%w", ErrGVisorNotAvailable)
}Recommendation: Refactor to accept a client parameter: // GVisorAvailable checks if runsc is configured as a Docker runtime.
func GVisorAvailable(ctx context.Context, cli *client.Client) bool {
info, err := cli.Info(ctx)
if err \!= nil {
return false
}
for name := range info.Runtimes {
if name == "runsc" {
return true
}
}
return false
}Then update the call in docker.go: if \!GVisorAvailable(ctx, cli) {
cli.Close()
return nil, fmt.Errorf("%w", ErrGVisorNotAvailable)
}Note: The design doc actually shows this better approach at line 55 of Minor Issues6. Installation Instructions Could Be Clearer 🟢Location: The installation command is complex and spans multiple lines. While the instruction says "copy and run," users might not realize it's a single command. Current: Recommendation: Consider adding a note that it's a single command, or provide a script alternative: 7. Inconsistent Comment Style 🟢Location: ociRuntime string // "runsc" or "runc"Issue: The comment mentions "runc" but the implementation uses empty string for the non-sandbox case, not "runc". This is misleading. Recommendation: Update comment to match implementation: ociRuntime string // "runsc" for gVisor, empty for Docker defaultOr fix the implementation to match the comment (see issue #4). 8. Test Files Disable Sandbox Without Explanation 🟢Location: rt, err := NewDockerRuntime(false) // Use runc for tests (no gVisor requirement)Issue: While the comment explains the choice, it would be better to use a test helper or constant to make this explicit across all tests. Recommendation: const testSandbox = false // Disable gVisor for tests to avoid setup requirements
func TestDockerRuntime_ListImages(t *testing.T) {
rt, err := NewDockerRuntime(testSandbox)
// ...
}Code Quality Observations✅ Excellent Error MessagesThe installation instructions in ✅ Good Separation of ConcernsThe ✅ Proper Backwards CompatibilityAll existing call sites that use ✅ Configuration ValidationThe Security Considerations✅ Secure by DefaultMaking gVisor required by default is the right choice for a security-focused tool. ✅ Clear WarningsThe warning when running with
|
E2E tests verify moat functionality, not gVisor specifically. Using NoSandbox: true allows tests to run in CI without gVisor.
📦 Previous Review (superseded by newer review)Pull Request Review: gVisor Sandbox SupportThis PR implements gVisor (runsc) as the default OCI runtime for Docker containers, providing kernel-level isolation through userspace syscall handling. The implementation is well-structured and follows secure-by-default principles. Code Quality ✅Strengths:
Suggestions:
Potential Bugs 🔍No critical bugs identified. The implementation correctly:
Minor observation:
Performance Considerations ⚡Good:
Neutral:
Security Concerns 🔒Strong security model:
Potential concerns:
Test Coverage 📊Good:
Gaps:
Documentation 📚Excellent documentation:
Minor suggestions:
Architecture & Design 🏗️Well-architected:
Future considerations:
RecommendationsBefore merge:
Future improvements:
SummaryThis is a high-quality implementation of an important security feature. The code is clean, well-tested, properly documented, and follows security best practices with secure-by-default behavior. The error messages are exemplary - users will know exactly what to do when gVisor is missing. Recommendation: Approve and merge ✅ The minor suggestions above are non-blocking and can be addressed in future PRs if desired. Review generated by Claude Code - Moat security reviewer |
Users reported no visible feedback when running 'moat run'. Now shows 'Initializing...' immediately while the runtime, proxy, and container are being set up.
Pull Request Review: gVisor Sandbox SupportThis PR adds gVisor (runsc) as the default sandbox for Docker containers, providing kernel-level isolation for enhanced security. I've reviewed the implementation and have several observations and recommendations. ✅ Strengths
🔍 Code Quality Issues1. Empty string vs. explicit runtime specification (internal/container/docker.go:62-66)var ociRuntime string // empty string = Docker's default runtime
if !sandbox {
log.Warn("running without gVisor sandbox - reduced isolation")
// Leave ociRuntime empty to use Docker's default (usually runc)
}Issue: The code leaves
Recommendation: Explicitly set to "runc" for clarity: var ociRuntime string
if !sandbox {
log.Warn("running without gVisor sandbox - reduced isolation")
ociRuntime = "runc" // Explicitly use runc when sandbox disabled
} else {
// ...
}2. Duplicate Docker client creation (internal/container/detect.go:195-213 & docker.go:57-60)Both
Recommendation: Refactor to reuse the client: // In docker.go
func NewDockerRuntime(sandbox bool) (*DockerRuntime, error) {
cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation())
if err != nil {
return nil, fmt.Errorf("creating docker client: %w", err)
}
var ociRuntime string
if !sandbox {
log.Warn("running without gVisor sandbox - reduced isolation")
ociRuntime = "runc"
} else {
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
if !gvisorAvailableWithClient(ctx, cli) { // New helper
cli.Close()
return nil, fmt.Errorf("%w", ErrGVisorNotAvailable)
}
ociRuntime = "runsc"
}
return &DockerRuntime{cli: cli, ociRuntime: ociRuntime}, nil
}
// New internal helper
func gvisorAvailableWithClient(ctx context.Context, cli *client.Client) bool {
info, err := cli.Info(ctx)
if err != nil {
return false
}
for name := range info.Runtimes {
if name == "runsc" {
return true
}
}
return false
}
// Keep public function for external callers
func GVisorAvailable(ctx context.Context) bool {
cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation())
if err != nil {
return false
}
defer cli.Close()
return gvisorAvailableWithClient(ctx, cli)
}3. Test coverage gap for gVisor detectionThe test in
Recommendation: Add mocked tests using 🐛 Potential Bugs1. Race condition in initialization message (cmd/moat/cli/exec.go:49-68)fmt.Print("Initializing...")
os.Stdout.Sync()
manager, err := run.NewManagerWithOptions(...)
if err != nil {
fmt.Println() // Clear the "Initializing..." line
return nil, fmt.Errorf("creating run manager: %w", err)
}
fmt.Println() // Clear the "Initializing..." lineIssue: If gVisor detection fails, the multi-line error message will appear on the same line as "Initializing...", creating confusing output like: Recommendation: Print newline before the error or use a spinner/status indicator that properly clears. 2. Config validation allows empty string but runtime defaults to true (internal/config/config.go:264-267)// Validate sandbox setting
if cfg.Sandbox != "" && cfg.Sandbox != "none" {
return nil, fmt.Errorf("invalid sandbox value %q: must be empty (default) or 'none'", cfg.Sandbox)
}This validation is correct, but there's a subtle UX issue:
Consideration: Current design is fine for opt-out only, but consider future extensibility if you want to support explicit "gvisor" or other sandbox types. Document this clearly. ⚡ Performance Considerations
🔒 Security Observations
📝 Documentation Reviewdocs/content/reference/01-cli.md
docs/content/reference/02-agent-yaml.md
🧪 Test Coverage AssessmentStrong coverage:
Gaps:
Recommendation: Add at least one integration test that:
📋 Minor Issues
🎯 Recommendations PriorityHigh Priority (fix before merge):
Medium Priority (fix soon after): Low Priority (future enhancement): ✅ ConclusionThis is a solid implementation of gVisor sandboxing with good security defaults and clear documentation. The main concerns are:
The PR is ready to merge with the understanding that the high-priority issues should be addressed in a follow-up PR if not fixed immediately. Overall Grade: B+ (would be A with the high-priority fixes) Reviewed by: Claude Sonnet 4.5 |
|
I'm going to merge these commits into another pending PR, which I think will be easier. |
Summary
--no-sandboxCLI flag to opt out when gVisor isn't available or causes compatibility issuessandbox: noneconfig option inagent.yamlfor persistent opt-outChanges
internal/container/docker.go: gVisor detection,--runtime=runscin container creationinternal/container/detect.go:RuntimeOptionsstruct with sandbox supportinternal/run/manager.go: WireNoSandboxoption through run lifecycleinternal/config/config.go: Addsandboxfield to agent configcmd/moat/cli/exec.go: Add--no-sandboxflagcmd/moat/cli/run.go: Honorsandbox: nonefrom configdocs/content/concepts/01-sandboxing.md: New concepts pagedocs/content/reference/01-cli.md: Document--no-sandboxflagDesign decisions
Test plan
moat runon Linux with gVisor installed → uses runsc runtimemoat runon Linux without gVisor → clear error with install instructionsmoat run --no-sandbox→ uses runc with warningsandbox: nonein agent.yaml → uses runc without needing flag🤖 Generated with Claude Code