feat(sandbox): workflow-sandbox-runner agent + remote-runner config (infra-admin P3b PR8/12)#848
Merged
Conversation
…ig wiring (PR8)
Task 15: cmd/workflow-sandbox-runner — gRPC server (SandboxExecService) with
mTLS + bearer-token auth, profile clamping (permissive→standard), agent-side
secret:// resolution via secrets.Provider, streaming stdout/stderr/exit_code
chunks. Version via ldflags.
Task 16: module/sandbox_remote_runners — new sandbox.remote_runners module type
(SandboxRemoteRunnersModule) that parses named RemoteRunnerSpec list from config
and exposes a RemoteRunnerRegistry service. resolveSandboxRunner treats any
unknown exec_env as a named remote runner — looks up the registry via
app.GetService, builds sandbox/remote.RemoteRunner from the spec + per-exec
SandboxConfig. pipeline_step_sandbox_exec.go: exec_env validation deferred to
Execute time. sandbox.SandboxConfig gains a Profile field.
Registration: pipelinesteps plugin, schema coreModuleTypes + registerBuiltins,
cmd/wfctl/type_registry.go, DOCUMENTATION.md, golden editor-schemas.json.
Release: .github/workflows/release.yml build-binaries matrix gains sandbox-runner.
Security review fixes (CHANGES_REQUESTED):
- CRITICAL: secret:// runner tokens are now resolved through the module's
configured secrets_provider (new config key) before dialing — a literal
"secret://..." string is no longer sent as the Bearer header. A secret:// token
with no provider is a hard error; literal tokens pass through.
- CRITICAL: the agent refuses to start as an unauthenticated executor (no token
AND no mTLS) unless --allow-unauthenticated is passed, which logs a loud warning.
- IMPORTANT: bearer-token compare uses crypto/subtle.ConstantTimeCompare.
- IMPORTANT: SandboxRemoteRunnersModule.Init rejects reserved names
(""/local-docker/ephemeral), duplicate names, and no-auth-no-TLS runners
(unless per-spec allow_insecure: true).
- MINOR: drainStream now fails the test on unexpected stream errors; added a
non-zero exit-code (exit 7) streaming test.
- MINOR: removed the bogus Inputs port from the sandbox.remote_runners editor
schema (provider-only module); regenerated golden.
- MINOR: dropped windows/amd64 from the sandbox-runner release matrix (Linux
Docker daemon target).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a remote sandbox execution agent (workflow-sandbox-runner) and engine-side static configuration wiring so step.sandbox_exec can dispatch to named remote runners via exec_env: <runner-name>, including schema/docs/release updates.
Changes:
- Introduces
sandbox.remote_runnersmodule type that registers aRemoteRunnerRegistryfor named remote sandbox agents, with token secret resolution and TLS options. - Updates
step.sandbox_execruntime resolution to route non-local-dockerexec_envvalues to configured remote runners, and forwards an informational sandbox profile to remote agents. - Adds the
cmd/workflow-sandbox-runnergRPC agent implementation (profile clamping, env secret resolution, bearer auth interceptor) and ships it in the release workflow.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| schema/testdata/editor-schemas.golden.json | Adds editor schema entry for sandbox.remote_runners module config. |
| schema/schema.go | Registers sandbox.remote_runners as a core module type. |
| schema/module_schema.go | Adds built-in schema for sandbox.remote_runners outputs/config fields. |
| sandbox/profile.go | Records the originating profile name into SandboxConfig for forwarding. |
| sandbox/docker.go | Adds SandboxConfig.Profile + GetProfile() helper (defaults to strict). |
| plugins/pipelinesteps/plugin.go | Exposes sandbox.remote_runners module type + factory via pipelinesteps plugin. |
| module/sandbox_remote_runners.go | Implements the remote runner registry module, spec parsing, TLS construction, and RemoteRunner building. |
| module/sandbox_remote_runners_test.go | Adds unit tests for registry parsing and validation rules. |
| module/pipeline_step_sandbox_exec.go | Defers exec_env validation to runtime and wires context into runner resolution. |
| module/execenv_factory.go | Resolves named remote runners via service registry + secrets provider token resolution. |
| module/execenv_factory_test.go | Updates tests for new runtime validation and token secret resolution behavior. |
| DOCUMENTATION.md | Documents the new sandbox.remote_runners module type. |
| cmd/workflow-sandbox-runner/server.go | Implements the SandboxExec gRPC server (profile clamp, env secret resolution, streaming output, bearer auth). |
| cmd/workflow-sandbox-runner/server_test.go | Adds bufconn-based gRPC tests for clamp/secret/auth/output streaming behaviors. |
| cmd/workflow-sandbox-runner/main.go | Adds the agent entrypoint, auth gate, secrets backend selection, and TLS/bearer server setup. |
| cmd/workflow-sandbox-runner/main_test.go | Tests the unauthenticated-start refusal gate logic. |
| cmd/wfctl/type_registry.go | Adds sandbox.remote_runners to wfctl’s known module type registry. |
| .github/workflows/release.yml | Builds/releases the new workflow-sandbox-runner binary (non-Windows). |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Forward fix for 6 hardening issues on the PR8 security surface: 1. main.go buildServerOptions: refuse --tls-ca without --tls-cert AND --tls-key. Previously a CA-only invocation started with INSECURE transport while checkAuthRequirement treated --tls-ca as "auth configured" → no TLS + no token. mTLS needs the server's own certificate; fail-fast otherwise. 2. module Init: a runner with a token but no TLS (and no allow_insecure) now fails at Init (cleartext-token leak) instead of late in NewRemoteRunner at first Execute. Also validates tls.cert/tls.key both-or-neither at Init. 3. buildTLSConfig: error when exactly one of cert/key is set (both-or-neither) rather than silently dropping client authentication. 4. doc: AllowInsecure comment now reflects that it also permits no-token+no-TLS runners (not just token-over-no-TLS). 5. server.go bearer compare: compare fixed-length SHA-256 digests via subtle.ConstantTimeCompare. Raw ConstantTimeCompare is only constant-time for equal-length inputs, leaking the expected token length on a mismatch. 6. server.go Exec: validate empty command / missing image up front and return codes.InvalidArgument (caller error) instead of letting the Docker runner surface them as codes.Internal. Tests: buildServerOptions CA-without-cert/key + cert/key-mismatch + no-TLS; module token-no-TLS (+allow_insecure OK), tls cert/key both-or-neither, CA-only OK; buildTLSConfig mismatch + neither; wrong-length token Unauthenticated; empty-command + missing-image InvalidArgument. Co-Authored-By: Claude Opus 4.8 (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
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.
PR8/12 — workflow-sandbox-runner agent + static-config wiring (infra-admin Phase 3b, part 2)
Locked plan Tasks 15–16; ADR 0019. The remote executor agent + the engine-side wiring so
step.sandbox_execwithexec_env: <runner-name>dispatches to a configured remote agent. Builds on PR7 (proto + RemoteRunner client).cmd/workflow-sandbox-runner(agent — a trust boundary)Serves
SandboxExecServiceover gRPC. Per request: clamps the requested profile to{strict,standard}(forcesstandardforpermissive/unknown, logs the clamp — never honors a privileged request from the engine); resolvessecret://env refs agent-side with its OWN secrets provider (values never logged/echoed); runs the command in a local Docker sandbox; streams stdout/stderr + a terminal exit_code. Auth: bearer-tokenStreamServerInterceptor(constant-time compare) + optional mTLS. Refuses to start without--tokenor--tls-caunless--allow-unauthenticatedis explicitly passed (loud warning) — no silent unauthenticated executor.Static-config wiring
sandbox.remote_runnersmodule: parses[{name, address, token, secrets_provider, tls, allow_insecure}], validates (required address, no reserved/duplicate names, auth-or-TLS-or-explicit-insecure), and exposes a lookup registry.resolveSandboxRunnerresolvesexec_env: <name>→ aRemoteRunnerto that agent (tokensecret://refs resolved via the engine's secrets provider).ephemeralstays deferred to PR9.Review notes (resolved — security)
secret://runner tokens now resolve through the secrets provider (were sent verbatim → broken auth).--allow-unauthenticated.allow_insecure; schemaInputsfixed; windows dropped from the agent release matrix (Linux Docker daemon).Release: added
cmd/workflow-sandbox-runnerto therelease.ymlbuild-binaries matrix (v0.72.0 ships it). Verified: build +go test ./...exit 0 (151 ok); fullgolangci-lint v2.12.00 issues;--help+ unauthenticated-refusal validated.🤖 Generated with Claude Code