feat(sandbox): file transfer, egress policy and suspend/resume on Azure - #476
Open
ItamarZand88 wants to merge 29 commits into
Open
feat(sandbox): file transfer, egress policy and suspend/resume on Azure#476ItamarZand88 wants to merge 29 commits into
ItamarZand88 wants to merge 29 commits into
Conversation
`create` sent `run --id <id>`. There is no `--id` flag on `run` — the id is positional — and without `--detach` the launcher stays attached until the control deadline kills it. Measured against a live launcher: it answers `unknown flag: --id`, exits 0, and leaves a session nothing can reach. So the call reported success and handed back an id that addressed nothing. Both env refusals go too. `--env` is accepted on `run` and on `exec`, and a sandbox inherits nothing from the container, so refusing it denied the only way to get a variable in. The test fake accepted any argv, which is how this passed review: three existing tests were green against the broken form. It now rejects unknown verbs and flags, and reverting the argv fails five tests. `fixtures/gcp-sandbox-cli-help.txt` records the launcher's real surface — eight verbs where the published reference lists six.
`.code({image})` was accepted on Azure and then dropped: the provider was
constructed with a literal `"ubuntu"` and the binding had no field to carry
anything else. Every session ran a stock image whatever the declaration said,
and nothing failed, because a sandbox on the wrong image still starts. It is
the one Azure gap with no typed error and no capability bit behind it.
The binding now carries `diskImage` and the emitter fills it from `code`. A
registry reference is refused at plan time rather than reinterpreted, matching
the AWS emitter: the create body names a public catalog image, so
`ghcr.io/org/x:tag` has nowhere to go and saying so beats substituting.
Ceilings stay the service defaults, now named rather than inline. `.limits()`
is refused on Azure at plan time, so nothing declares them and there is no
value to carry; they move into the binding when `enforcedLimits` flips.
Reverting the plumbing fails the new provider-level test — the seam that was
wrong is the one under assertion, not just the provider it feeds.
…host The scope was `management.azuredevcompute.io/.default` — the endpoint's own host, which is the natural guess and had no citation. The SDK this module's header already names as the source of its contract pins the audience for this endpoint and api-version as `dynamicsessions.io/.default`. Both audiences mint a token against our tenant, so a wrong one fails at the data plane rather than at the token endpoint, where it looks like a missing `SandboxGroup Data Owner` assignment. Which of the two the data plane accepts cannot be settled without a provisioned sandbox group: the endpoint answers 404 to an unauthenticated request in our region, so nothing short of a real group distinguishes them. The test that pinned the old value was written from the same guess rather than from the package, so it pinned the guess.
Azure was the one backend that refused `readFile`, `writeFiles` and `mkdir`,
so portable file code had to branch on the capability. The data plane has had
the verbs the whole time — `GET`/`PUT {sandbox}/files` and
`POST {sandbox}/files/mkdir` — and `write` takes `createDirs`, which is what
gives Azure the cross-backend rule that a write creates its parents.
Three things had to come first. The Azure request builder carried a `String`
body, which cannot hold a file; it now carries bytes. Every failure was one
`OperationNotSupported`, so a missing file and an unreachable data plane were
indistinguishable; they now split into a refusal, which is not retryable, and
an unknown outcome, which is retryable for a file operation and never for a
command. And a body is now truncated before it is echoed into an error, so a
32 MiB upload cannot become a 32 MiB log line.
Paths are checked before they leave the process: relative only, no `..`, no
empty components, no trailing slash, on all three operations. Whether the
server confines a path is undocumented, so the code says this is our rule and
not a guarantee. Transfers are capped at 32 MiB in both directions, the number
the agent-backed backends already enforce, with the read bounded as it arrives.
`files: true` came last, after the three methods worked. The wire tests run
against a server the test controls and each one fails against a plausible
wrong version: drop `createDirs`, move the mkdir path into the query, drop
either ceiling, or drop the path check, and a test goes red.
Azure's propagation-delay 400s arrive as `RemoteResourceConflict`, which the
client marks transient, so that variant stays out of the refusal set — a
refusal tells the caller never to retry.
…ables Two silent failures in the same create path. The data plane names the lifecycle field `state`; this client read `status`, so every response deserialized to nothing and the provider's fallback arm called that `Running`. A sandbox still being created, stopping, or being deleted all came back as ready to run commands. The mapping now covers the seven states the data plane reports and refuses one it does not recognise — every default here is a lie a caller acts on. The create body never carried `environment`, and a sandbox inherits nothing from its group, so a declared variable simply did not exist inside the session. The data plane accepts the body either way, which is why nothing failed. `create_sandbox` takes a struct now: the create body keeps gaining fields that decide what the sandbox can do, and each one added positionally is one a caller can pass in the wrong slot.
Azure's data plane takes an egress policy at create, and this backend sent none — so `deny` and a hostname list were both refused at plan time while the cloud underneath could express either. The declaration now travels in the binding and becomes the policy the sandbox is created with. `deny` is a `Deny` default under `Full` traffic inspection, plus a catch-all deny rule. Each part is load-bearing. Only `Full` blocks non-HTTP traffic — under any other mode a `Deny` default is a label on a live network. The rule is there because Microsoft documents `Partial` as evaluating only traffic a rule matches and never says `Full` differs, so a policy holding no rules at all is the one shape where "deny" could mean nothing. `allowDomains` becomes host rules over the same `Deny` default. `allow` sends no policy: the data plane is already open, and `Full` there would block the traffic `allow` promises. Then it is checked. The create response carries the policy the sandbox is actually running under, so a sandbox that came up without the one that was asked for is deleted rather than handed back — a restriction that did not take effect is worse than one nobody asked for, because the caller believes it held. The check compares the default action, the inspection mode, and every host the declaration named, not the whole object, so a normalised response does not fail every create. `egressDeny` and `domainEgressRules` flip last. Azure is the first backend to express a hostname allowlist at all: the others match CIDRs or carry a single switch. The binding's `egress` field is required, so a binding JSON without one no longer parses. Nothing is deployed, so there is nothing to migrate. What a live sandbox still has to settle: whether traffic is actually blocked. The response proves the policy is configured, never that a packet was dropped — the same evidence every other backend flips its flag on.
Three backends turned `allowDomains` into their nearest expressible thing, and only the core capability check stood between that and a rendered artifact. Render a chart or a GCP module directly and the declaration changed meaning with nothing anywhere saying so. Helm folded it into the `allow` arm and emitted `0.0.0.0/0` — every address the list existed to exclude. Its test asserted the widened policy, pinning the behaviour rather than preventing it. GCP collapsed it to `allowEgress: false`, denying everything the declaration asked to permit. Both now refuse, as AWS already did. The AWS message ends "or use a platform that supports it", which was advice to nowhere while every backend reported `domainEgressRules: false`; all four messages now name Azure, whose egress proxy matches on host pattern.
The containment check compared the effective policy inward only — every host the declaration named had to be present — which is the right test pointed the wrong way. A sandbox that came up allowing a host nobody asked for passed it, and so did one whose `rules` list allowed everything, because this client never writes that list and so did not read it either. A group-scoped policy is a documented way for an entry nobody sent to appear. Both directions now: nothing may allow a host the declaration did not name, in either list, and an advanced rule that is not a `Deny` fails the create outright. Extra denials stay harmless, so a normalised response still cannot fail a create that was honoured.
`suspendResume` was false because the two verbs were unimplemented, not
because Azure lacks them: `POST {sandbox}/stop` saves the state and
`POST {sandbox}/resume` brings it back, both returning on acceptance. That is
the contract the trait gets — a caller that needs the session stopped polls
`get`, the same rule AWS follows.
Flipping the flag drags a second thing with it. `idleSuspendSeconds` is gated
on `suspendResume`, so the declaration becomes legal on Azure the moment the
flag flips — and the create body never carried a lifecycle policy, so the
number would have been accepted and dropped. It now travels in the binding and
arrives as `lifecycle.autoSuspendPolicy`, suspending to memory, which is what
makes the resume fast enough to be worth having.
Three capabilities stay false, and each is now a recorded decision rather than
an unbuilt feature:
- `preview`: a sandbox port's auth is anonymous or Entra ID with an allowlist
of human email addresses. Neither is a credential scoped to a port for a
fixed time, and returning the anonymous URL would publish the port.
- `snapshot`: the blocker is ours. `snapshot()` returns an id and
`CreateSessionRequest` has nothing to consume one, so no backend can complete
the round trip — and nothing in the resource model owns the artifact, which
Microsoft says is never garbage collected.
- `sessionLifetime`: Azure suspends on idle and deletes after a stop, but has
no wall-clock ceiling. Accepting `maxLifetimeSeconds` would be the silent
no-op the capability set exists to prevent.
The comment those three replace claimed a per-port URL closed to anonymous
traffic and a 0.54s resume. Neither has a source, and the first is the opposite
of what the port model says.
The state mapping refused `Idle`, and `Idle` is what the SDK's stop poller waits for. The package contradicts itself — it declares `Idle` as a reason a sandbox stopped and then waits for a *state* of `Idle` — and an unrecognised state is an error here on purpose, so the one state the idle policy is most likely to produce would have failed every `get` on the sessions that policy governs. It reads as suspended, which is true under either reading. Also: `autoSuspendPolicy` mode is the SDK's own default rather than a claim about what `Disk` does, which is documented nowhere.
Six things this branch got wrong, and the review caught each one. The create body carries the caller's environment variables and a write carries file bytes, and a failure echoes the request into an error chain that is serialized into durable state. Both now go through `redact_request_body`, which every other create-with-a-secret already used. `create` owes the caller a session that can take work — the trait says a backend whose start API returns early waits here — and it was handing back one that could not. It waits now. Every failure after the sandbox exists deletes it through one path: a `?` on an unreadable state was abandoning a running sandbox that nothing could find, since Azure mints the id, has no enumeration verb and sets no auto-delete. The egress check guarded `create` alone, so a reconnect returned whatever a session was built with. Azure has no session ceiling and an idle sandbox only suspends, so one created under an older declaration outlives the change and was being handed back under the label the stack has now. `get` checks too. `policy_holds` had three holes: it compared case-sensitively where the data plane normalises, it passed any host action that was not exactly `Allow` — `Transform` and `Rewrite` reach a host by rewriting the request — and it could not see a policy field this client does not model. It is now a whitelist in both directions, and an unreadable policy fails the create. The refusal named an env var that does not exist and flattened the delete's own error through an `internal = false` boundary, publishing raw service text. It is a typed `SandboxNotAsDeclared` carrying the session id — the one thing an operator needs when the delete also failed. `catalog_disk_image` rejected a registry path but not a tag, so `ubuntu:24.04` rendered into a customer's module, planned, applied, and failed at the first session. Smaller, same review: `Stopping` is not stopped, so it no longer answers the poll `suspend` documents; `terminate` polls the client directly rather than dying on a state it cannot parse; an absolute path means "under the session root" here as everywhere else; `create` is not idempotent and no longer claims to be; an `allowDomains` naming no domain is refused at plan time; Helm refuses in the function that renders rather than one upstream; and the four egress refusals stopped pointing customers at a platform whose sandbox group nothing in this repository creates. The captured launcher fixture carries the launcher's own output, and a recipe for re-capturing it that needs nothing but the container it came from. The generated schemas and the TypeScript doc still described Azure as having no file transfer and no backend as matching hostnames. Regenerated with `pnpm -C packages/core run generate`, and the hand-written paragraph corrected.
The comment pass found eleven comments narrating the change rather than the code: a regression test explaining the bug it came from, three "used to" and "before this" asides, and a rationale stated twice — once at the orchestrating function and again at the call site. The deadline guard's reason now lives at `run_command`, and `execute_within` carries the local note plus a pointer.
`run` takes the sandbox id positionally and `--allow-egress` is one of its own flags, so an application passing `--allow-egress` as its session id was asking for the egress its binding had refused it — the one setting the binding decides rather than the caller. The doc comment three lines above says exactly that, and the argv defeated it. Reachable because create now works: the id used to sit behind a flag the launcher rejected outright. The id is checked wherever a caller supplies one — create, exec, the three file operations and terminate — rather than at the one verb that has `--allow-egress`, because every verb takes it positionally and each has its own flags.
The first fix round left three holes and opened one. `execute_shell_command` was the third body a caller controls in that file and the one that was missed: a shell command is where an app puts a token it wants the session to have, and a failure echoes the request into an error chain that reaches durable state. `settle` judged the egress policy on the create response. A create answered while the sandbox is still coming up need not carry the policy yet, and an absent policy reads as "the restriction did not take" — so every deny-declared create would have deleted the sandbox it just made. It waits for the sandbox to be running and judges what came up, which is also the read the containment check should have been making all along. `Stopping` mapped to `Running` so `suspend`'s completion poll would not answer early, and that routed a sandbox on its way down through the reconnect path as ready for work. The four states the trait publishes have no word for it, so it reads as unusable — the honest answer for everything that consumes the enum — and the wait and reconnect paths read the raw state, where the difference is the whole question. A session whose policy no longer matches the declaration was a permanent error from `get_or_create`, which owes the caller a usable session: it is now terminated and replaced, like a terminated one. `unmodelled` caught an unreadable field on the policy but not on its rules, so an exception list on a rule that otherwise reads as a plain deny still passed. The rule structs now model every field the SDK does and refuse anything past it. `SandboxCommandFailed` was fixed `internal = "false"` while wrapping cloud client errors that carry response text, and `into_external` reads only the outermost flag. It inherits, for the reason `SandboxUnreachable` already gives. An Azure session id is interpolated into the data-plane URL, where `..` resolves — reaching a sandbox group a stack-scoped identity can address but this binding was never scoped to. It is checked wherever a caller supplies one, as GCP's now is. And `run_command` re-reads the policy: the SDK hands it an arbitrary session id, so an id kept across a declaration change was the way around the check.
`get` read the egress policy before the state, so a session being deleted — which carries no policy — was reported as one running the wrong one. Under a `deny` declaration that sent a disappearing sandbox down the replace path instead of the terminated one. The test that should have caught it declared `allow`, which skips the check entirely; it declares `deny` now. `resume` is the other verb that puts code back on the network, and it did no policy read, so an id kept across a declaration change could be resumed around the check that `run_command` performs. The wait resumes through an ungated path instead, because a sandbox mid-boot has no policy to judge yet — gating both would re-break the case `settle` was restructured to fix. Reconnecting now re-judges the policy on the sandbox that came up rather than the one that was found asleep: a group-scoped policy is set somewhere this binding never writes, so the read before the wait is not the read that decides. And `discard` names the sandbox it left behind rather than attributing every failure to the egress policy — a readiness timeout reached the caller as a containment failure, pointing at a restriction that was never the finding.
…his code woke Two blockers, both found and reproduced by the review rather than read out of the diff. `get` skips the policy check for a session being deleted, because a sandbox on its way out carries no policy to judge. The two gates that stand between a kept session id and its egress then asked only whether the session exists — so a `Deleting` sandbox passed both. Azure accepts a delete rather than completing it, which is why `terminate` polls to a 404 instead of trusting the accept: the workload is still running through that window. A session built under a looser declaration, tightened since, with a delete in flight, would have run new code under the egress it was built with. Both gates now refuse a session that cannot take work, which is also the right answer for `resume`. The reconnect path judged the policy after `await_running` had already resumed the sandbox — putting whatever it was running back on the network — and then returned the error with no cleanup. Every retry repeated it, and the argument that retries converge does not hold: a stopped sandbox reporting the stale policy sends each attempt back down the same branch. It is discarded now, like every other sandbox this code creates or wakes and then refuses. And a minted id this client cannot address is no longer sent to the delete — the id it refuses to send is the id that delete would travel on.
The gate asked whether a session existed and whether it was not being deleted. Neither is the question. A sandbox still coming up carries no policy yet, so judging one reported a booting session as uncontained — and `get_or_create` acts on that by deleting it and creating another. A suspended one carries the record it stopped with, which is not what the work would run under. So the gate now brings a session up before it judges it, and every verb that starts code, wakes it, or moves the caller's own content into it goes through the same path: `run_command`, `resume`, `write_files` and `get_or_create`. `readFile` and `mkdir` stay ungated — a read returns to the caller who already holds the session, and a directory plants nothing. That also settles a question the two reconnect arms answered oppositely. A session that cannot serve is replaced, whether the reason is that it is gone, being deleted, or running a policy the declaration no longer matches. The narrow part is deliberate: a readiness timeout is not one of those reasons, because answering a slow data plane with a second sandbox makes it slower. Two smaller ones from the same review. A minted id this client will not send is still reaped unless the id is itself why the delete would be unsafe — an over-long id is one path segment and nothing else can find that sandbox, while a traversing one would send the delete into another group. And `write_files` validates its paths before it spends a round trip, not after.
A stopped sandbox carries the policy it stopped under, so it can be judged where it lies — and the gate was exempting exactly that state. The cost was concrete: a session created under `allow`, a declaration since tightened to `deny`, and a reconnect would wake it, put its workload back on the network for the length of a boot, and only then refuse it. It is judged asleep now, and again once it is up, because a group-scoped policy can change while it sleeps. Only a session still coming up is exempt, which is the one state with nothing to judge. The resume in the readiness wait was latched on the attempt rather than the outcome, so a single refusal meant nothing ever woke the sandbox again: the wait spent its whole budget watching, returned `sessionNotReady`, and that does not heal — wedging the id permanently. It retries every poll now, and never fires at all while the sandbox is still `Stopping`, which is the state the data plane refuses a resume in and the state a suspend leaves behind. The timeout carries the last refusal, because "still not running after 120s" sends a reader looking for a slow data plane when every resume was rejected. `Failed` is a state the data plane reports and this client did not know, so it became an unreadable-response error that nothing heals. It is terminal, and a session found terminal mid-wait is now replaced like one found terminal at the start — the same condition answered the same way whichever read observes it. Two verbs stopped destroying what they refuse. `run_command`, `write_files` and `resume` did not create the session and were not asked to replace it, and two revisions of a stack share a sandbox group — so reaping there turns one revision's tightened declaration into the other's outage. They refuse; `resume` puts back a session it woke and then rejected. Only `create`, which owns what it made, and `get_or_create`, which was asked for a usable session, replace. And `suspend_and_resume_reach_their_own_verbs` proves the verb is sent again: its mock answered `Running` on the first read, so the wait returned before any resume, and the assertion that named the test passed with the call never made.
A resume that found the session already running, or watched it come up on its own, stopped it on a policy mismatch — ending a command another revision of the same stack was running, since both share the sandbox group. Only the wait knows whether it issued the resume, so it reports that instead of a pre-read inferring it from the state. A failed session found on reconnect is now reaped rather than left beside its replacement, and a sleeping record with no policy at all is left for the post-wake judgement: whether the data plane reports egressPolicy for a stopped sandbox is unverified, and refusing would churn every idle-suspended session if it does not.
A create is a PUT to a collection with a service-minted id and no idempotency key, and the transport retried it three times: a response lost after the sandbox was minted made a second one, returned success, and left the first running with the caller's environment variables and no id-holder able to reap it. An exec answering late was re-sent the same way, so untrusted code could run four times. Both now take a single-attempt path; every other verb keeps its retry, which is what the new test asserts against. The rest of this change closes what the same review found. `get` and the reconnect path now share one predicate for "is there a policy here to judge", so they cannot disagree about a suspended session; `resume` judges the sleeping record before waking anything; and the fact that this call issued the resume now survives every exit from the wait, so a session woken by a wait that then failed is still put back or named. A command's own environment variables reached nothing: the exec endpoint takes no environment, so they travel as shell assignments in front of the command, with names checked because a name sits where quoting cannot reach it. Azure's catalog image rule moves to plan time beside the AWS one, so a sandbox no worker binds is still refused, and `code.image` says what Azure takes rather than offering two examples it rejects.
A refusal is the one answer that proves a session stayed asleep. A 5xx, a timeout or a dropped connection does not: the data plane can take the resume and answer nothing, and the session wakes. Counting that as "did not wake" left the put-back inert, so a session this call returned to the network under a policy the declaration forbids was abandoned there — the hole the previous commit closed, reached through the other door. A stop that answers 404 now ends the put-back quietly. The session has reached the state the stop was for, and naming it as left awake sends an operator looking for a sandbox that does not exist.
The exec endpoint takes no environment, so a command's variables travel in the shell string. In front of the wrapper they applied to it: a declared PATH could hand `setsid`, `sleep` and `kill` no-ops, and the deadline that bounds untrusted code would never fire. They go through `env` now, so they reach the command and nothing else. The wrapper also unsets the names it uses before assigning them. An inherited exported variable keeps its export attribute across re-assignment, so a session created with `nonce` set was handing the command the deadline token it uses to tell a kill from an exit. Alongside: a reconnect refuses a session it did not wake rather than deleting it, matching what resume already did — two revisions of a stack share a sandbox group, and the replacement `get_or_create` owes its caller does not require ending the other one's work. Azure's catalog image is read while planning, where a sandbox no worker binds is still seen, and `code.image` says that AWS and Azure narrow it in opposite directions. `Allow` no longer claims a private-range denial that a host-pattern matcher and a boolean switch cannot express.
Fifteen comments and doc-strings cut to the bar: the ones that had grown to five or six lines saying what three could, one that restated the name of the function under it, and one field doc whose middle sentence had no verb. Two that were not about wording. `judgeable`'s catch-all arm covers the transitional and terminal states and said nothing about why they carry nothing to judge. And the test for a command's declared variables never polled the stream it was handed, so it proved the call returned rather than that the command ran; it drains the stream and reads the exit now.
`env` reads operands as assignments until one is not, then execs that one. The separator sat after the assignments, so it became the program name and every Azure command carrying variables died with 127 before it started. Dropped, and a test now runs the generated string through a real shell instead of asserting its shape — the shape was exactly what was intended, and what was intended was wrong. A program whose own name carries `=` is refused when the call also declares variables, because `env` would take it for a variable and run the next argument in its place. A session can no longer set `PATH`, `IFS`, `LD_PRELOAD` or `LD_LIBRARY_PATH`. The wrapper that holds a command to its deadline runs inside the session and inherits them: a session `PATH` chooses which `od` draws the nonce, which is the whole basis for telling a kill from an exit. The same names per command are fine — those reach the command and nothing else. The wrapper also quotes the last two expansions it had left bare, so an inherited `IFS` cannot split a pid into words that are not children. Comments that still described assignments in front of the wrapper, and three that said a refused session is deleted, now say what the code does. Schemas regenerated for a doc reworded after the last run.
`LD_AUDIT` runs attacker code inside every process the wrapper starts — including `od`, which draws the nonce the deadline report rests on. Code in the session can then write a nonce it chose to stderr before the wrapper announces one, exit 137 itself, and be reported as killed at a deadline nothing enforced. Demonstrated end to end against a real glibc loader: a command that ran for no time at all claimed a 30-second kill. Refused as `LD_*` rather than by name. The list was two entries long because two were suggested, and the loader reads more than anyone maintaining that list would remember. A command naming no program is refused too. `env` with assignments and no operand prints the environment it was handed and exits 0, so an empty command returned the session's own variables to the caller as a command that succeeded.
Bash imports `SHELLOPTS` from the environment and applies what it lists, including tracing, even when it is called `sh`. The wrapper's own trace then occupies the first line of stderr, the announcement is not where the reader looked for it, and every command in that session comes back as one the session could not bound — the ones that succeeded included. The reader now takes the first line that is a nonce and nothing else. The trace cannot be mistaken for it: a traced line carries the shell's prefix, and bash does not take that prefix from the environment. What precedes the announcement is dropped rather than returned, because it was written before the command started and one of those lines is the trace of the announcement itself. The width is checked exactly. A single hex character on a line of its own was an announcement, which made most of the stream its own repeat. `SHELLOPTS` and `BASHOPTS` join the names a session may not set. The two answers are deliberate: one keeps a name nobody listed from breaking the report, the other keeps the ones we know about out of the session.
A session runs its command as root on a writable filesystem, so code in it can replace the `od` that draws the nonce and hand every later command a value it chose, or kill the killer — a sibling with the same uid — and outlast its deadline. Both measured against real shells. The doc claimed the nonce was unreachable and left the rest implied. So the wrapper bounds a command that is merely slow and reports honestly on one that is. It is not a boundary against a command working to escape one; the caller-side guard, which ends the session, is that. The process-group note is narrowed to match what it delivers: a child that starts a session of its own leaves the group and outlives the kill. The reader also trims a trailing carriage return. None of the transports here produce one, but a 33-byte announcement is invisible and the first line the command chose gets adopted in its place — an inversion resting on a property of today's transports rather than on anything checked.
The note in `get_or_create` said such a session is left running. It is left asleep when this call is what woke it, because `put_back` suspends what it woke — which is what `reconnect`'s own doc and the test beside it already said. Three statements of one rule, and this was the one that disagreed.
Greptile SummaryThe PR expands Azure sandbox support with file transfer, declared egress enforcement, suspend/resume lifecycle handling, and corrected data-plane request contracts.
Confidence Score: 5/5The PR appears safe to merge. The previously reported lifecycle replay failure is fixed: stop and resume now use the single-attempt request path, and indeterminate resume outcomes retain ownership for compensating suspension.
|
| Filename | Overview |
|---|---|
| crates/alien-azure-clients/src/azure/common.rs | Adds a single-attempt request executor and binary request bodies while retaining retries for safe operations. |
| crates/alien-azure-clients/src/azure/sandbox_data_plane.rs | Implements Azure sandbox file, egress, and lifecycle contracts, with stop and resume routed through the single-attempt transport. |
| crates/alien-bindings/src/providers/sandbox/azure.rs | Adds Azure session policy checks and lifecycle ownership handling, including compensating suspension after indeterminate resume outcomes. |
| crates/alien-core/src/resources/sandbox.rs | Extends sandbox capability and platform-specific image validation. |
| crates/alien-terraform/src/emitters/azure/sandbox.rs | Emits the required Azure sandbox disk-image and egress binding configuration. |
Sequence Diagram
sequenceDiagram
participant W as Worker binding
participant A as Azure sandbox provider
participant D as Azure data plane
W->>A: Use suspended session
A->>D: POST resume (single attempt)
alt Definitive refusal
D-->>A: Refusal
A-->>W: Resume failure
else Accepted or indeterminate outcome
D-->>A: Success, timeout, conflict, or 5xx
A->>A: Mark wake as owned
A->>D: Poll session state
A->>D: POST stop when returning session
end
Reviews (3): Last reviewed commit: "fix(sandbox): send a state transition on..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Stop and resume move a sandbox between states, so a repeat is refused for the state the first attempt produced. A transition that took effect and lost its response was re-sent, the re-send answered 409, and the caller was told a session it had suspended was still awake — the false `sandboxLeftAwake` report the put-back exists to make trustworthy. They join create and exec on the single-attempt path. The wait above them already re-issues a resume itself, with the state in front of it, so the transport had no business guessing. The test that pins create now pins these too; it did not before, which is why they were classified as safe to repeat.
Contributor
Author
|
The lifecycle-retry finding is already addressed on the current head (
@greptile-apps please re-run the review |
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
1 participant
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.
Summary
Adds capabilities to the Azure sandbox backend: reading and writing files inside a
session, enforcing a declared egress policy — including a hostname allowlist — and
suspending and resuming a session. Azure is the only backend that restricts egress to
hostnames, because its egress proxy matches on host pattern where the others filter by
CIDR or carry a single switch.
Step-by-step flow when a worker calls
sandbox.runCommand:describes carries the egress policy the stack declared.
variables travel through
envso they reach the command and not the wrapper. ← the heartuntrusted code again.
This PR brings the Azure binding onto the file, egress and lifecycle operations the
data plane exposes.
What was broken
Four silent failures in the existing Azure path, each found by reading the SDK the
client cites as its source: the data-plane token asked for the wrong audience, the
declared image was discarded, session state was read from a field the service does not
send (so every sandbox reported
Running), and declared environment variables neverreached the create body.
What I did
in both directions and path rules applied before anything reaches the wire.
policy back and refused a sandbox that came up without it.
back or uses a session judge it against the declaration first.
createandexecsingle-attempt. Both were being retried three times by thetransport: a lost response after a create minted a second sandbox that no id-holder
could find, and reported success.
"allow everything" or "deny everything".
Files touched
crates/alien-bindings/src/providers/sandbox/azure.rs— the backend: file ops, egressjudgement, session lifecycle, command bounding.
crates/alien-azure-clients/src/azure/sandbox_data_plane.rs— the wire contract, readfrom the published preview SDK.
crates/alien-azure-clients/src/azure/common.rs— a single-attempt send for the twoverbs a repeat performs twice.
crates/alien-core/src/resources/sandbox.rs— the capability set, and Azure'scatalog-image rule at plan time.
crates/alien-bindings/src/providers/sandbox/gcp.rs— the Cloud Run launcher takes thesession id positionally and needs
--detach; ids are checked before they reach it.Upgrading an existing Azure stack
egressanddiskImageare now required on the Azure sandbox binding with no default.A deployment whose binding was written by the older module will fail to load until
terraform applyregenerates it, so apply before or together with the binary upgrade.Defaulting them was the alternative and is worse: there is no safe default for egress —
one opens the sandbox, the other silently tightens a running deployment.
code.imagealso narrows in opposite directions per platform. AWS builds a MicroVM froman
s3://bundle; Azure names an entry in a public catalog, so a bareubuntu. Onedeclaration cannot target both, and each refuses the other's shape while planning.
How I tested
Manually: drove the generated wrapper through a real
/bin/shwith a realsetsidin
debian:stable-slim,alpineandrockylinux:9— a variable declared on a commandreaches it, a 1.5s deadline kills at 1.5s with exit 137, and a command that spawns
children takes them with it. Ran the create path against a local HTTP server answering
503then201and confirmed exactly onePUTis delivered, against four before thechange.
Unit tests: ~40 added across the sandbox provider, the data-plane client and the
emitters. Every guard was checked by reverting it and confirming its test fails.
Anything I couldn't test: no live Azure data plane. Whether it reports
egressPolicyfor a stopped sandbox is unverified, and the code is deliberately safeeither way — an absent policy on a sleeping record is treated as unknown, not as a
mismatch. Whether an ADC sandbox has any route to private ranges is also unmeasured.
Attacks considered for this diff:
session judges the running record unconditionally; the read paths skip only where
there is provably nothing to judge (
judged_sessionvsjudgeable).[A-Za-z_][A-Za-z0-9_]*and the wholeNAME=valuepair is one quoted argument.PATH,IFS,SHELLOPTS,BASHOPTSor anyLD_*, each of which changes what thewrapper resolves, splits, loads or traces.
rather than position so a traced shell cannot displace it.
chain, and the wrapping errors inherit the source's
internalflag.because two revisions of a stack share one sandbox group.
One thing did turn up and is not fixed here: the in-session deadline wrapper is an
honest report about a cooperative command, not a boundary against a hostile one. Code
running as root in a session can replace the
odthat draws the nonce, or kill thekiller — a sibling with the same uid — and outlast its deadline. Both measured. This
predates the change and defeats the existing Local backend identically; the enforced
bound is the caller-side guard, which ends the session. The comments now say so rather
than implying more.