mirror of
https://github.com/agent-substrate/substrate.git
synced 2026-11-11 10:23:17 +08:00
Fixes #202 atelet's gRPC server runs on an unauthenticated hostPort (`manifests/ate-install/atelet.yaml`), so its `Run`/`Restore`/`Checkpoint` RPCs must treat every request field as untrusted. Several of those fields are turned into host filesystem paths and then deleted, created, written, or executed, none of them validated first. A caller that can reach the port can smuggle a path separator or `..` through them and make atelet operate outside the intended actor tree. ### Unvalidated inputs (each an independent escape route) - **`actor_template_namespace` / `name` / `id`** are concatenated by `ateompath.ActorPath` into the per-actor dir that `resetActorDirs` runs `os.RemoveAll` / `os.MkdirAll` on. - **`target_ateom_uid`** builds the netns path (`/run/netns/ateom:<uid>`) and the ateom control socket (`.../ateoms/<uid>/ateom.sock`). - **`spec.containers[].name`** becomes an OCI bundle path component (`os.RemoveAll` / `MkdirAll` / `config.json` write). - **`runsc.sha256_hash`** builds `static-files/runsc-<hash>`. The cache-hit `os.Stat` early return hands that path to ateom to **execute** before the existing hex check runs, so an escaped path could execute an arbitrary on-disk binary as the sandbox runtime. ### Where the validators live The field rules are exported from **`internal/resources`** (`ValidateActorRef`, `ValidateAteomUID`, `ValidateContainerNames`, `ValidateRunscHash`, `ValidateSnapshotURIPrefix`), alongside the existing `ValidateActorID`. Every one of these values originates upstream in user-supplied API/CRD data (the runsc hash and snapshot location come from the `ActorTemplate` spec), so the API server and controller can apply the same rules at admission time instead of failing at actor-activation time. `ValidateContainerNames` takes names rather than the atelet proto so `resources` stays proto-free. Unifying `ValidateActorID` with `ValidateActorRef` is deferred: it changes ate-api-server's client-facing error responses. The per-RPC glue stays in `cmd/atelet`: each of `Run`/`Checkpoint`/`Restore` calls its own typed validator (`validateRunRequest` etc.) as the first statement at the boundary, before any path is constructed. ### What is validated - namespace and actor ID as DNS-1123 labels, template as a DNS-1123 subdomain, matching how these are named; additionally the combined `<ns>:<tmpl>:<id>` path component is capped at 255 bytes, since individually valid DNS names can join to 381 bytes, over common filesystems' filename limit; - `target_ateom_uid` as a DNS-1123 label (pod UIDs are UUIDs, which qualify); - each container name as a DNS-1123 label, additionally rejecting the reserved name `pause` (collides with the sandbox-infra bundle and races its concurrent writer) and duplicate names (same bundle path, mutual corruption); - `runsc.sha256_hash` as exactly 64 hex chars, checked in `fetchRunsc` before the path is built and before the cache-hit early return; - `snapshot_uri_prefix` (Checkpoint/Restore) as a parseable URI naming a bucket, with query/fragment/userinfo rejected: object names are appended by concatenation and the storage layer re-parses using only host and path, so those components would silently redirect the upload/download to a different object. The scheme is deliberately not restricted; that is storage-backend policy. Validation failures return `codes.InvalidArgument` with the reason. Previously the interceptor surfaced them as `codes.Internal` with the message masked as "internal server error". DNS-1123 is a deliberate fit. These values already *are* Kubernetes names, so every legitimate value passes, while the charset forbids `/`, `.` (for labels), and `..`, which is exactly what enables traversal. For a cooperating caller the change is behavior-neutral: inputs newly rejected previously failed later or misbehaved silently. ### Testing - Table tests for each validator in `internal/resources`, targeting the exported functions (valid values, separators, `..`, empty, case, length, the label-vs-subdomain distinction, the combined-length cap, the `pause`/duplicate rules, and the URI component rejections). - Per-request validator tests in `cmd/atelet` pin which rules each RPC enforces, one table per validator. - `TestRPCBoundariesReject` exercises `Run`/`Checkpoint`/`Restore` directly and asserts a traversal value is rejected with `InvalidArgument`, which guards against future removal or reordering of the validation call. - `TestFetchRunscRejectsBadHash` plants a real cache file at the path an invalid hash resolves to and asserts `fetchRunsc` still errors. It passes with the current code and fails if validation is moved after the `os.Stat` early return, so it proves the ordering rather than just that validation runs. - [x] Tests pass - [x] No documentation changes needed