feat(sdk): add strict Dockerfile sandbox launch - #16
Conversation
4412924 to
445cecf
Compare
tianyuzhou95
left a comment
There was a problem hiding this comment.
Thanks for putting together the RFC and the reference implementation. The overall direction is useful, and the submitted unit tests, Ruff, and mypy checks pass locally. However, the current implementation has several correctness and security issues that need to be addressed before merge.
Blocking findings
-
SDK-side
ADD URLcreates an SSRF boundary violation._dockerfile_runner.pylines 234-244 downloads remote URLs withurllibin the SDK process and then uploads the response into the sandbox. An agent-supplied Dockerfile can therefore read loopback, link-local, cloud metadata, or private-network endpoints reachable from the SDK host and retrieve the response from inside the sandbox. Restricting redirect schemes does not address the host-level SSRF. The fetch should happen inside the sandbox, or remoteADDmust be an explicit trusted-input-only opt-in with an enforceable network policy. -
The build context bypasses
.dockerignoreand does not implement consistent Docker context semantics._dockerfile_runner.pylines 246-275 passes a local directory directly tocopy_from_local, soCOPY . ...can upload files such as credentials,.env, or.giteven when they are excluded by.dockerignore.DockerContext.walk()is not used, and non-local contexts cannot correctly support directories or wildcards. Please build a filtered context manifest, apply.dockerignore, expand source patterns, and use the same behavior for local and remote contexts. -
Valid Dockerfile syntax is accepted as directly launchable but executed incorrectly.
_dockerfile.pylines 433-487 parses JSON-formCOPY/ADDwithshlex, soCOPY ["a b", "/dest/"]becomes source"[a b,"and destination"/dest/]". Exec-formRUNis also treated as a shell string, and flags such as--chmodand--linkare silently discarded. In addition,_dockerfile_runner.pylines 129-139 resets relativeWORKDIRvalues instead of resolving them against the previous directory and ignoresARG, even though build arguments affectFROM, later instructions, andRUN. Unsupported syntax must either be implemented or rejected bycheck_direct_launch; it must not return success and then run with different semantics. -
The evaluator does not inherit the base image configuration. It initializes build state as empty environment,
/, and root, and resolvesCMD/ENTRYPOINTonly from instructions in the current Dockerfile. A Dockerfile containing onlyFROM nginx, for example, is reported as launchable but does not start the inherited nginx command; inheritedUSER,WORKDIR,CMD, andENTRYPOINTare likewise lost. This needs OCI image-config plumbing or a narrower explicitly documented contract. Without it, the feature cannot claim that the resulting sandbox behaves like the Dockerfile. -
The documented warning policy is silent in the main API.
sandbox.pylines 292-303 discards theDockerfileApplyResult, so warnings for ignored instructions never reach callers ofSandbox(context=...). Unknown instructions also remain warnings understrict=True. Please reject unsupported behavior by default, or emit/expose the warnings and make strict mode consistently reject every ignored instruction. -
CMD/ENTRYPOINTresolution and readiness do not match the stated contract._dockerfile.pylines 154-167 drops a shell-formCMDwhen paired with an exec-formENTRYPOINT, whereas Docker appends/bin/sh -c .... The launcher also infers shell form from whether a single argument contains a space, which breaks valid one-element exec-form commands. Finally, the readiness check only confirms that the sandbox is alive before starting the background command, and the returned process handle is discarded, so an immediately failing application can still produce a successful constructor. Please preserve the parsed command form explicitly and either define an application-start check or describe this as sandbox readiness rather than application readiness.
I recommend revising the RFC around a strict, trusted Dockerfile subset and making unsupported constructs fail closed. The PR also needs to be rebased onto the current backend-neutral SDK architecture before these changes can be integrated.
Add a backend-neutral direct-launch path for a deliberately strict Dockerfile subset. The FROM image supplies only the sandbox root filesystem; explicitly declared RUN, COPY, ADD, ENV, WORKDIR, USER, CMD, and ENTRYPOINT behavior is applied through the public sandbox facades without BuildKit, a Docker daemon, or a registry push. Validate Dockerfiles and build contexts before remote side effects. Unsupported syntax fails closed, remote ADD URLs are rejected, and a filtered manifest applies .dockerignore consistently to local and custom contexts. Local paths use no-follow directory-relative opens, all COPY and ADD inputs are materialized before sandbox operations, tar members are restricted to safe regular files and directories, and ownership changes are limited to outputs of the current instruction. Expose the background CMD or ENTRYPOINT CommandHandle while defining constructor success as sandbox readiness and successful dispatch rather than application health. Integrate cleanup with the backend-neutral BackendSession lifecycle and document the rootfs-only and no-snapshot contract. Signed-off-by: Peng-YM <Peng-YM@users.noreply.github.com>
445cecf to
4f2a35e
Compare
|
Thanks for the detailed review. I addressed each blocking finding in commit 1. SDK-side
|
tianyuzhou95
left a comment
There was a problem hiding this comment.
Thanks for the thorough update. The six findings from my previous review are
materially addressed by this revision, and the narrower fail-closed contract is
much clearer. I reran the new Dockerfile/context tests and reviewed the updated
RFC against the implementation. The following correctness issues still block
the direct-launch path.
Blocking findings
-
The explicit root working directory is delegated to the backend default.
_dockerfile_runner.pylines 253-257
passescwd=Nonewhenever the accumulated DockerfileWORKDIRis/, and
the startup path repeats this at lines 741-746.Nonemeans "use the backend
default", not/; the actor backend creates a temporary default directory
and resolves a missing cwd to it. Consequently,FROM ubuntufollowed by
RUN pwdruns under/tmp/sandbox_*, and a Dockerfile withoutWORKDIR
starts itsCMDthere as well, despite the RFC defining/as the baseline.
Please pass the explicit/through for RUN, startup, tar, and ownership
commands instead of converting it toNone. -
COPYdrops source file metadata, including the executable bit.
_materialize()
recreates every selected source withopen(..., "wb"), but neither the
context abstraction nor the copy plan records its mode. In a minimal local
reproduction, a0755entrypoint.shwas staged as0664; no later chmod
restores it. Thus the supported pattern
COPY entrypoint.sh /usr/local/bin/entrypointcommonly produces a
non-executable startup file.
Docker's supported COPY semantics preserve file metadata and permissions.
The context/manifest needs to carry and apply the relevant metadata, or the
advertised COPY contract must fail closed for semantics it cannot preserve. -
Advertised directory COPY cannot represent empty directories.
DockerContext.walk()
enumerates files only, and the manifest derives directories only from those
file paths. For a context containing an emptyempty/directory,
check_direct_launch()reportsCOPY empty/ /srv/empty/as launchable, but
walk()is empty and_validate_copy_plan()fails with "sources select no
files". Since literal directories are explicitly in the strict subset, the
manifest must represent directory entries and the runner must create selected
empty directories (including empty directories nested under non-empty trees). -
A
USERbefore local tarADDmakes the supported ADD fail.
_extract_tar()
creates the destination through the root-owned filesystem facade and then
wrapstar xfwith the accumulated non-rootUSER. For example,USER app
followed byADD app.tar /opt/app/attempts to extract asappinto the
newly root-owned/opt/app, so it normally fails with permission denied.
COPY does not switch to this user either. COPY/ADD should run with builder
ownership semantics, with the validated--chownapplied afterward, rather
than applying runtime USER to tar extraction. -
Valid
USER user:groupand numeric USER forms are silently changed.
wrap_user()
unconditionally discards the group.USER app:stafftherefore runs with
app's default group instead ofstaff.USER 1000:1001is also reported
as directly launchable, but becomesrunuser -u 1000;runusertreats that
as a user name and fails when no passwd entry named1000exists, whereas a
numeric Docker USER does not require such an entry. Please preserve these
forms, or reject the unsupported forms during strict parsing rather than
changing their meaning at execution time.
I ran the 117 directly affected unit tests on Python 3.10, 3.11, and 3.12; all
pass. The focused reproductions above cover cases not present in that suite.
Preserve the explicit root working directory and represent Docker build context entries with file type and permission metadata so COPY retains executable modes and empty directories. Run archive extraction with builder ownership and reject USER forms that cannot be represented faithfully by the current command facade. Add focused regressions for directory targets, modes, ownership, strict USER validation, and live-example coverage for the corrected behavior. Signed-off-by: Peng-YM <Peng-YM@users.noreply.github.com>
|
Thanks for the focused follow-up. I addressed all five findings in a separate commit, 1. Explicit root working directoryThe Dockerfile runner now passes the accumulated absolute Regression coverage asserts that every runner command receives 2. COPY file metadata and executable modeThe public context protocol now exposes a frozen
The unit suite covers 3. Empty directories in directory COPYThe context manifest now represents directory entries directly instead of inferring them only from files. Local contexts enumerate all directories deterministically, including top-level and nested empty directories; custom contexts must also provide every directory ancestor explicitly. Missing ancestors, duplicate paths, file-as-ancestor conflicts, symlinks, and special files fail closed. Literal directories,
4. USER before local tar ADDCOPY and ADD now always use builder/root ownership semantics and no longer receive the accumulated runtime The previous test that expected 5. Group-qualified and numeric USER formsThe direct-launch subset now explicitly supports named users only, such as The README, RFC, public API documentation, maintenance contract, and example prechecks now describe and exercise this narrower fail-closed rule. Validation
The RFC issue and PR description have also been updated to reflect the corrected public contract and current test evidence. Could you please take another look when convenient? |
tianyuzhou95
left a comment
There was a problem hiding this comment.
Thanks for the focused follow-up. I verified that the five findings from the
previous review are addressed in cc53726: root cwd is now explicit, COPY
modes and empty directories are represented, tar ADD uses builder ownership,
and unsupported USER forms fail closed. Two runtime correctness issues and one
quality-gate issue remain.
Blocking findings
-
Wildcard directory sources retain an extra directory level.
_select_wildcard()
prefixes every child of a matched directory with
posixpath.basename(directory). For a context containing
sub/dir1/dir2/foo,COPY sub/* /subdest/therefore writes
/subdest/dir1/dir2/foo. Docker/BuildKit treats the matcheddir1as a
directory source and copies its contents, so the expected target is
/subdest/dir2/foo; this exact case is covered by BuildKit's upstream
testCopyWildcards.
The strict subset advertises wildcard directory support, so the matched
directory root needs destination-marker semantics rather than being added
to each relative target. -
Unreadable directories are silently copied as empty directories.
LocalDockerContext.walk()
callsos.walk()without anonerrorcallback. Python consequently ignores
a permission error while descending into a directory but still records the
directory entry seen from its parent. In a minimal reproduction, a mode
000directory containingrequired.txtwas emitted as one empty directory
entry;COPY blocked /appthen succeeded with onlymkdir /appand silently
droppedrequired.txt. This violates the RFC's fail-closed context contract.
Please make traversal errors raiseDockerContextErrorand add a regression
covering an unreadable selected directory. -
The claimed 208-test gate depends on the caller's umask.
test_local_and_memory_contexts_match
creates local files and directories using ambient modes, while
MemoryDockerContexthard-codes0644/0755. With the commonumask 0002,
local entries are0664/0775, so the complete unit gate consistently runs
208 tests with one failure on Python 3.10, 3.11, and 3.12. The same gate
passes after forcingumask 0022. Please assign explicit modes in this test
somake sdk-checkis hermetic.
Ruff and mypy pass locally. The updated head currently has no GitHub check runs.
Match Docker source wildcards one path segment at a time and copy the contents of matched directories without retaining their root name. Fail closed when a local context traversal cannot descend into an entry, and keep context parity tests independent of the caller's umask. Add regressions for wildcard directory targets, filepath-style double-star matching, traversal failures, destination ownership, and the maintained live example. Signed-off-by: Peng-YM <Peng-YM@users.noreply.github.com>
|
Thanks for the latest verification. I addressed all three findings in a new, separate commit, 1. Wildcard directory sourcesWildcard-matched directory roots now use destination-marker semantics. Their contents are merged into the destination without retaining the matched root basename, while directly matched files continue to use their basename. For the reported case: COPY sub/* /subdest/with The implementation also handles mixed file/directory matches, multiple matched directories, empty matched directories, root destinations, and target collisions. Multiple directory markers may share one destination, but colliding non-marker content fails before any sandbox operation. New destination directories are included in exact During independent verification, I also found and corrected a related source-pattern issue: Docker uses Go 2. Unreadable context directories
Regression tests inject
3. Umask-independent quality gateThe local/custom context parity fixture now explicitly sets context files to The full quality gate was run from a subshell with Additional validation
The RFC issue and PR description have been updated with the wildcard matching, traversal failure, 211-test, umask, and live-example evidence. Could you please take another look when convenient? |
tianyuzhou95
left a comment
There was a problem hiding this comment.
Thanks for the update. The three findings from the previous review are addressed in this revision: wildcard-matched directory roots now have the expected destination-marker behavior, local traversal failures fail closed, and the context parity test is independent of the caller's umask. Two Dockerfile-semantics issues still need to be resolved before merge.
| pattern_segments = pattern.split("/") | ||
| path_segments = path.split("/") | ||
| return len(pattern_segments) == len(path_segments) and all( | ||
| fnmatch.fnmatchcase(path_segment, pattern_segment) |
There was a problem hiding this comment.
Blocking: fnmatch.fnmatchcase() does not implement the Go filepath.Match semantics promised by the RFC and used by Dockerfile source patterns. In particular, Python uses ! for character-class negation while Go uses ^: with a.txt and b.txt, [!a].txt selects b.txt here but a.txt under Go, and [^a].txt has the opposite behavior. Python also treats a malformed pattern such as file[.txt as a literal match, whereas filepath.Match returns ErrBadPattern. This can silently copy the wrong context files instead of failing closed. Please use a Go-compatible matcher with malformed-pattern validation, or reject character-class syntax if it is outside the supported subset.
There was a problem hiding this comment.
Resolved in 5736dc1.
I replaced fnmatch.fnmatchcase() with a precompiled matcher for the strict no-escape subset of Go filepath.Match. Only leading ^ negates a class, ! remains a literal member, ranges and Unicode codepoints follow Go behavior, and */?/classes stay within one path segment. Every pattern is compiled before context entries are inspected, so malformed classes fail even for an empty context and before any file read or sandbox operation.
The tests cover [!a], [^a], ranges, Unicode ?, single-segment **, empty/unclosed classes, and malformed ranges. An independent verifier also compared 1,368 pattern/name cases against Go filepath.Match with no mismatch. Backslash escapes remain explicitly outside the documented strict subset.
| or selection.has_directories | ||
| or ins.dest.endswith("/") | ||
| ) | ||
| must_use_directory = selection.has_directories or len(selection.entries) > 1 |
There was a problem hiding this comment.
Blocking: this guard cannot reject wildcard expansions containing directories, because selection.has_directories makes both must_use_directory and directory_target true; the later not directory_target condition is therefore false. For a context containing one/a and two/b, COPY * /target currently succeeds and writes /target/a and /target/b. Docker requires the destination to end in / whenever a wildcard expands to multiple sources, just as it does for multiple explicit sources. Please retain/count the top-level wildcard matches and reject a multi-source expansion without a trailing slash, while preserving the valid single-directory case.
There was a problem hiding this comment.
Resolved in 5736dc1.
_ContextSelection now records the number of top-level wildcard sources after .dockerignore filtering: each matched directory root counts once regardless of descendants, and each directly matched file counts once. The runner rejects an expansion count greater than one when the destination does not end in /, before materialization or any sandbox operation.
Regression coverage includes two directories, two files, and a mixed directory/file expansion, with no-trailing-slash rejection and trailing-slash success. A single directory or single file without a trailing slash remains valid, and marker, mode, collision, and --chown behavior remains covered.
Match Dockerfile source patterns with Go filepath-compatible character classes and reject malformed patterns before context files are read. Track top-level wildcard expansions so multi-source copies require a directory destination. Add regressions for class negation, malformed ranges, Unicode matching, ignored sources, and wildcard expansion counts. Signed-off-by: Peng-YM <Peng-YM@users.noreply.github.com>
|
Thanks for the follow-up. Both blocking inline comments are addressed in a new, separate commit, Go-compatible source patternsThe SDK no longer delegates Dockerfile source matching to Python
The new unit coverage includes Multi-source wildcard destinations
When one wildcard expands to multiple top-level sources, the runner now requires the destination to end in
Validation
I also replied directly to both inline discussions. Could you please take another look when convenient? |
What changed
This PR adds a backend-neutral, strict Dockerfile direct-launch path to the
Python SDK.
Sandbox(context=...)uses the existing publicSandbox,Commands, andFilesystemfacades: it parses one Dockerfile, creates asandbox from
FROM, applies the explicit supported instructions, and exposesthe optional background startup command as
sandbox.startup_command.The branch has been rebased onto the backend-neutral upstream
mainarchitecture. The implementation keeps backend-native conversions behind the
existing boundary rather than coupling Dockerfile launch to either backend.
The public surface includes
DockerContext,DockerContextEntry,LocalDockerContext,parse_dockerfile,check_direct_launch,apply_dockerfile, typed parse/checkand apply results, and constructor options
context,auto_start_cmd, andbuild_run_timeout. The SDK README and maintainedsdk/python/examples/dockerfile_launch.pynow document and exercise thecurrent contract.
Why
Small single-stage Dockerfiles are a common way to describe a development
environment. Direct launch supports a narrow, auditable subset without
requiring BuildKit, a Docker daemon, or a registry push. It is intentionally
not a Docker build replacement: every launch reruns supported
RUN,COPY,and
ADDinstructions and creates no cache or snapshot.Supported contract
FROMis deliberately rootfs-only. The base image supplies the sandbox rootfilesystem, while its OCI
ENV,USER,WORKDIR,CMD, andENTRYPOINTconfiguration is not inherited. The Dockerfile must state required runtime
settings explicitly.
The supported subset is exactly one literal
FROMwith optionalASalias;shell-form
RUN; shell-form localCOPY/ADDsources for files, directories,., and wildcards; literal--chown; literal local tar extraction; literalENV; absoluteWORKDIR; namedUSERvalues such asapporroot;EXPOSEmetadata; and exec- or shell-formCMD/ENTRYPOINTwith normalizedOCI-style argv combination. Group-qualified and numeric UID/GID
USERformsare rejected because the current command facade cannot preserve them faithfully.
Multi-stage input,
COPY --from,ARG, remoteADDURLs, JSONCOPY/ADD,exec-form
RUN, relativeWORKDIR, build-time variable expansion, unsupportedflags, and unsupported instructions are rejected. Execution paths fail closed:
Sandbox(context=...)parses strictly,check_direct_launch()returnsFalsewith reason codes, and
apply_dockerfile()rejects parsed unsupported items.Non-strict parsing is diagnostics only.
After instructions complete, the SDK polls sandbox readiness and dispatches the
resolved startup argv in the background. A non-
NoneCommandHandleconfirmsdispatch, not process longevity or application health. Callers can
wait()orkill()the handle and own their application-specific health check. Dockerfileswithout a startup command, or launches with
auto_start_cmd=False, exposeNone.Security and correctness
The context implementation validates the complete structured manifest and
materializes all file inputs before any sandbox operation.
DockerContextEntryrepresents files, directories, permission modes, and empty directories for both
local and custom contexts. COPY restores child entry modes non-recursively,
applies root
.dockerignore, filters reserved Dockerfile metadata, rejectslocal symbolic links and context traversal errors, and validates paths and
collisions. Dockerfile source wildcards use the strict no-escape subset of Go
filepath.Match:^negates character classes,!remains literal, malformedclasses fail closed before file reads, and wildcard expansions with multiple
top-level sources require a destination ending in
/. A matched directorycontributes its contents without retaining that matched root name. Local tar
extraction
accepts only regular files and directories with safe paths and always executes
with builder/root ownership, independently of the accumulated
USER.--chownis literal, quoted, and limited to outputs created by the currentinstruction. Remote
ADDis rejected by design, avoiding network retrieval andhost-side SSRF. Every runner command also receives the explicit Dockerfile
working directory, including the baseline
/, rather than a backend default.For unsupported Dockerfiles or build-once reuse, callers externally pre-build
an image, use
Sandbox(image=...), and explicitly launch the desired command.The SDK does not auto-start image configuration on that path.
Testing
The current unit gate contains 214 tests and passes with Ruff and mypy,
including under
umask 0002. It covers strict parser behavior, Go-compatiblecharacter classes, malformed pattern rejection, wildcard expansion counts,
startup argv resolution and cleanup, context and
.dockerignorefiltering, pre-upload manifest materialization, collisions,path validation, local tar traversal, ownership validation, and remote
ADDrejection. The maintained example adds independent sections for filtered
COPY ., BuildKit-compatible wildcard directory COPY, file and directory modes,empty-directory copies, explicit root cwd, finite
CMDandENTRYPOINT+CMDstartup handles,COPY --chown, builder/root localtar
ADDafterUSER, and no-sandbox fail-closed prechecks.Real-sandbox follow-up
The maintained five-section example passed against a real standalone
runscdeployment through the backend-neutral facade using
openyuanrong-sdk:COPY .honored.dockerignore; the ignored secret was absent,executable mode
0755was preserved, top-level, nested, and literal emptydirectories were created with the expected modes, and
COPY wild/*produced/srv/wild/dir2/foowithout retaining the matcheddir1root;RUN/ENV/WORKDIR/USERstate reached a finite CMD, and itsstartup_command.wait()completed successfully;RUNand exec-formENTRYPOINT+CMDused explicit root cwd/; thestartup argv merged and produced the expected marker;
COPY --chownproduced the expected owner; local tarADDafterUSER appstill extracted with builder/root semantics and produced the expected tree;
ADD, group-qualifiedUSER, and numericUSERwere rejected duringpre-check without creating a sandbox.
Additional live regressions placed pre-existing root-owned files in both COPY
and tar destinations. Those files remained
root:root; only files anddirectories created by the current instruction became
myuser:myuser.The same code is backend-neutral and has unit coverage for both backend
sessions. The default
openyuanrong-sandboxbackend could not be runtime-testedwith the current public standalone image: it lacks the backend's
/api/sandbox/v1route and returns HTTP 404 before sandbox creation.Licensing
The parser dependency is
dockerfile-parse(BSD-3-Clause); context matchinguses
pathspec(MIT). The value-level parsing approach was informed by the E2BPython SDK (MIT). No Docker engine, BuildKit, or registry component is added.