diff --git a/.github/workflows/sandbox-isolation.yml b/.github/workflows/sandbox-isolation.yml index e29c0d4e5..10d54c328 100644 --- a/.github/workflows/sandbox-isolation.yml +++ b/.github/workflows/sandbox-isolation.yml @@ -224,8 +224,8 @@ jobs: executed=$(grep -oE 'executed="[0-9]+"' "$trx" | head -1 | grep -oE '[0-9]+') passed=$(grep -oE 'passed="[0-9]+"' "$trx" | head -1 | grep -oE '[0-9]+') echo "executed=${executed:-0} passed=${passed:-0}" - if [ "${executed:-0}" -lt 66 ]; then - echo "::error::Expected >=66 sandbox isolation tests to run (bwrap/prlimit confinement + cap-drop + cgroup-namespace re-root + egress-allowlist filter + cgroup resource cap + durable-launch cgroup wiring + argv/envp per-string kernel ceiling + a prompt past it riding stdin + a read-only workspace mount + a network-off run sealed to its broker + a read-only reviewer reading its diff with the real CLIs), but only ${executed:-0} did — the Category=Sandbox filter matched too few (trait regression?). If a case was deliberately removed, lower this number in the same PR." + if [ "${executed:-0}" -lt 67 ]; then + echo "::error::Expected >=67 sandbox isolation tests to run (bwrap/prlimit confinement + cap-drop + cgroup-namespace re-root + egress-allowlist filter + cgroup resource cap + durable-launch cgroup wiring + argv/envp per-string kernel ceiling + a prompt past it riding stdin + a read-only workspace mount + a network-off run sealed to its broker + a read-only reviewer reading its diff with the real CLIs), but only ${executed:-0} did — the Category=Sandbox filter matched too few (trait regression?). If a case was deliberately removed, lower this number in the same PR." exit 1 fi @@ -258,12 +258,12 @@ jobs: print(f'All {len(arms)} reviewer E2E arms ran and passed.') # The sealed-egress E2E returns early on a host that cannot seal, which reads as Passed; require each arm's marker. - for arm in ('durable', 'non-durable', 'ipv6', 'restart-reissue', 'policy-route-discard', 'setup-failure'): + for arm in ('durable', 'non-durable', 'ipv6', 'restart-reissue', 'policy-route-discard-dst', 'policy-route-discard-l4', 'setup-failure'): assert f'[sealed-egress-e2e] ran {arm}' in text, f'sealed-egress E2E arm "{arm}" did not run — this lane is root with bwrap, ip and nft, so it must seal' - for method, rows in (('A_network_off_brokered_run_reaches_its_broker_and_nothing_else', 2), ('A_sealed_namespace_drops_the_gateway_over_ipv6_link_local_too', 1), ('A_30_still_held_by_a_run_that_outlived_its_worker_is_not_handed_to_the_next_run', 1), ('A_host_whose_policy_rule_discards_the_run_s_replies_fails_the_setup_and_leaks_nothing', 1), ('A_sealed_setup_that_fails_on_this_host_refuses_the_launch_typed_and_leaks_nothing', 1)): + for method, rows in (('A_network_off_brokered_run_reaches_its_broker_and_nothing_else', 2), ('A_sealed_namespace_drops_the_gateway_over_ipv6_link_local_too', 1), ('A_30_still_held_by_a_run_that_outlived_its_worker_is_not_handed_to_the_next_run', 1), ('A_host_whose_policy_rule_discards_the_run_s_replies_fails_the_setup_and_leaks_nothing', 2), ('A_sealed_setup_that_fails_on_this_host_refuses_the_launch_typed_and_leaks_nothing', 1)): cases = [r for r in results if 'SealedEgressE2ETests.' + method in r.get('testName', '')] assert len(cases) == rows and all(r.get('outcome') == 'Passed' for r in cases), f'{method}: all {rows} case(s) must pass' - print('All 6 sealed-egress arms ran and passed.') + print('All 7 sealed-egress arms ran and passed.') print('All 10 batch/stream kernel cases passed.') PY diff --git a/backend/src/CodeSpace.Core/Services/Agents/Sandbox/Isolation/FilteredEgressPlan.cs b/backend/src/CodeSpace.Core/Services/Agents/Sandbox/Isolation/FilteredEgressPlan.cs index 9930a5b76..f57da4af0 100644 --- a/backend/src/CodeSpace.Core/Services/Agents/Sandbox/Isolation/FilteredEgressPlan.cs +++ b/backend/src/CodeSpace.Core/Services/Agents/Sandbox/Isolation/FilteredEgressPlan.cs @@ -1,3 +1,5 @@ +using System.Globalization; + namespace CodeSpace.Core.Services.Agents.Sandbox.Isolation; /// @@ -35,14 +37,16 @@ public sealed record FilteredEgressPlan public required IReadOnlyList> SetupCommands { get; init; } /// - /// The argv that asks the kernel how the host reaches the namespace's end — the lookup every reply the host itself - /// sends there makes, the broker's included. Run after : the /30 was chosen from the - /// routes the host lists (), but only the kernel's own lookup accounts for a policy - /// rule, or the null route in a table one consults before main, that would discard those replies after a - /// clean setup. It is an output lookup, so it does not see an allowlist run's NAT'd replies, which are routed on - /// input: a rule keyed on the uplink (iif) can still divert those. + /// The argv that asks the kernel how the host reaches the namespace's end. Run after : + /// the /30 was chosen from the routes the host lists (), but only the kernel's own + /// lookup accounts for a policy rule, or the null route in a table one consults before main, that would + /// discard the run's replies after a clean setup. A sealed plan names what its broker's replies carry — TCP from the + /// broker port — so a rule keyed on the protocol or the source port is seen too. It cannot name the rest: each reply + /// goes to the agent's own ephemeral port and may carry a mark, so a rule keyed on the destination port or a mark + /// still gets past it. An allowlist plan has no one port to name, and its NAT'd replies are routed on input, so a + /// rule keyed on those selectors or on the uplink (iif) can still divert them. /// - public IReadOnlyList RouteCheckArgv => new[] { "ip", "route", "get", NsIp, "from", HostIp }; + public required IReadOnlyList RouteCheckArgv { get; init; } /// Why 's answer does not take the namespace's traffic through its own host veth, or null when it does. internal string? RouteCheckFailure(int exit, string output) @@ -56,8 +60,9 @@ public sealed record FilteredEgressPlan /// /// The device ip route get answered with — the word after its one dev — or null for anything else. - /// Read from the text answer, not -j: route get learned JSON only in iproute2 5.0, three releases - /// after the route listing the allocator reads, and on those releases it prints text under -j too. + /// Read from the text answer, not -j: route get learned JSON only in iproute2 5.0, while the route + /// listing the allocator reads already parses on some 4.x builds (Debian 10's 4.20), where route get prints + /// text under -j too. /// private static string? RoutedDevice(string output) { @@ -113,6 +118,7 @@ public static FilteredEgressPlan Build(string runId, IReadOnlyList allow NsAddrCidr = $"{nsIp}/30", HostIp = hostIp, NsIp = nsIp, + RouteCheckArgv = RouteCheck(nsIp, hostIp), NsSubnetCidr = subnetCidr, SetupCommands = setup, NftRuleset = nftRuleset, @@ -146,6 +152,7 @@ public static FilteredEgressPlan BuildSealed(string runId, int brokerPort, Egres NsAddrCidr = $"{subnet.NsIp}/30", HostIp = subnet.HostIp, NsIp = subnet.NsIp, + RouteCheckArgv = RouteCheck(subnet.NsIp, subnet.HostIp, "ipproto", "6", "sport", brokerPort.ToString(CultureInfo.InvariantCulture)), NsSubnetCidr = subnet.Cidr, SetupCommands = NamespaceSetup(ns, vethHost, vethNs, subnet), NftRuleset = BuildSealedNftRuleset(ns, vethHost, subnet.HostIp, brokerPort), @@ -167,6 +174,9 @@ public static FilteredEgressPlan BuildSealed(string runId, int brokerPort, Egres new[] { "ip", "netns", "exec", ns, "ip", "link", "set", "lo", "up" }, }; + /// The route lookup to the namespace's end from the gateway, narrowed by . The protocol is named by number: the name tcp needs /etc/protocols, which minimal images lack. + private static IReadOnlyList RouteCheck(string nsIp, string hostIp, params string[] selectors) => ["ip", "route", "get", nsIp, "from", hostIp, .. selectors]; + /// The per-run netns / nft-table name — derived PURELY from , so a reaper / teardown reconstructs it with no setup-time state. public static string NamespaceFor(string runId) => $"cs-egr-{Slug(runId)}"; diff --git a/backend/tests/CodeSpace.SandboxTests/SealedEgressE2ETests.cs b/backend/tests/CodeSpace.SandboxTests/SealedEgressE2ETests.cs index 4d101a31b..5723eb75c 100644 --- a/backend/tests/CodeSpace.SandboxTests/SealedEgressE2ETests.cs +++ b/backend/tests/CodeSpace.SandboxTests/SealedEgressE2ETests.cs @@ -153,8 +153,10 @@ public async Task A_30_still_held_by_a_run_that_outlived_its_worker_is_not_hande finally { await FilteredEgressNetns.TeardownAsync(survivor, CancellationToken.None); } } - [Fact] - public async Task A_host_whose_policy_rule_discards_the_run_s_replies_fails_the_setup_and_leaks_nothing() + [Theory] + [InlineData(false)] + [InlineData(true)] // a rule that only TCP from the broker port meets — the lookup the broker's replies make, and so the one the check must ask + public async Task A_host_whose_policy_rule_discards_the_run_s_replies_fails_the_setup_and_leaks_nothing(bool keyedOnTheBrokerReply) { if (!Seals()) return; @@ -165,7 +167,7 @@ public async Task A_host_whose_policy_rule_discards_the_run_s_replies_fails_the_ var third = RandomNumberGenerator.GetInt32(0, 64) * 4; var lease = new EgressSubnetAllocator.Lease { Cidr = $"192.0.2.{third}/30", HostIp = $"192.0.2.{third + 1}", NsIp = $"192.0.2.{third + 2}" }; var table = RandomNumberGenerator.GetInt32(10_000, 1_000_000).ToString(CultureInfo.InvariantCulture); - var rule = new[] { "pref", "100", "to", lease.Cidr, "lookup", table }; + string[] rule = keyedOnTheBrokerReply ? ["pref", "100", "to", lease.Cidr, "ipproto", "6", "sport", "9", "lookup", table] : ["pref", "100", "to", lease.Cidr, "lookup", table]; var runId = Guid.NewGuid().ToString("N"); var plan = FilteredEgressPlan.BuildSealed(runId, brokerPort: 9, lease); @@ -189,7 +191,7 @@ public async Task A_host_whose_policy_rule_discards_the_run_s_replies_fails_the_ (await NetnsExistsAsync(plan.Namespace)).ShouldBeFalse("a setup that failed its route check tears its namespace down"); (await RunHostExitAsync(["ip", "link", "show", plan.VethHost])).ShouldNotBe(0, "and the host end of its veth, with the address on it"); - output.WriteLine($"{RanMarker} policy-route-discard {refused.SetupError}"); + output.WriteLine($"{RanMarker} policy-route-discard-{(keyedOnTheBrokerReply ? "l4" : "dst")} {refused.SetupError}"); } finally { @@ -226,6 +228,7 @@ public async Task A_sealed_setup_that_fails_on_this_host_refuses_the_launch_type ((CodeSpace.Messages.Failures.IFailure)refusal).Code.ShouldBe(CodeSpace.Messages.Failures.FailureCodes.SandboxSealedEgressUnavailable); refusal.Cause.ShouldContain("ip link add", customMessage: $"the refusal must name the setup step that failed: {refusal.Cause}"); + refusal.Message.ShouldContain("fix what that step names", customMessage: $"a host that can seal is told to fix the failed step, not to grant privileges and wait for a re-probe: {refusal.Message}"); (await NetnsExistsAsync(names.Namespace)).ShouldBeFalse("a failed setup must tear down the namespace it had already created"); output.WriteLine($"{RanMarker} setup-failure cause={refusal.Cause}"); diff --git a/backend/tests/CodeSpace.UnitTests/Workflows/FilteredEgressPlanTests.cs b/backend/tests/CodeSpace.UnitTests/Workflows/FilteredEgressPlanTests.cs index 87bc9f260..1c1d4d8f1 100644 --- a/backend/tests/CodeSpace.UnitTests/Workflows/FilteredEgressPlanTests.cs +++ b/backend/tests/CodeSpace.UnitTests/Workflows/FilteredEgressPlanTests.cs @@ -131,7 +131,8 @@ public void The_route_check_asks_the_kernel_how_the_host_reaches_the_namespace_e { var plan = FilteredEgressPlan.BuildSealed("run-ffff6666", 43121, Subnet); - plan.RouteCheckArgv.ShouldBe(new[] { "ip", "route", "get", "10.5.7.18", "from", "10.5.7.17" }, "the lookup a reply from the broker makes: to the namespace's end, from the gateway — as text, which every iproute2 prints"); + plan.RouteCheckArgv.ShouldBe(new[] { "ip", "route", "get", "10.5.7.18", "from", "10.5.7.17", "ipproto", "6", "sport", "43121" }, "what a reply from the broker carries — to the namespace's end, from the gateway, TCP from its port — so a rule keyed on the protocol or the source port is seen too; as text, which every iproute2 prints"); + FilteredEgressPlan.Build("run-ffff6666", new[] { "1.1.1.1" }, Subnet).RouteCheckArgv.ShouldBe(new[] { "ip", "route", "get", "10.5.7.18", "from", "10.5.7.17" }, "an allowlist run has no one port its replies come from"); } [Theory] diff --git a/backend/tests/CodeSpace.UnitTests/Workflows/LocalProcessRunnerEnvScrubTests.cs b/backend/tests/CodeSpace.UnitTests/Workflows/LocalProcessRunnerEnvScrubTests.cs index 52cb8a864..a51d22989 100644 --- a/backend/tests/CodeSpace.UnitTests/Workflows/LocalProcessRunnerEnvScrubTests.cs +++ b/backend/tests/CodeSpace.UnitTests/Workflows/LocalProcessRunnerEnvScrubTests.cs @@ -357,14 +357,32 @@ public void A_brokered_child_with_no_proxy_keeps_its_environment(string? workerO resolved.Environment.Keys.ShouldBe(new[] { "ANTHROPIC_BASE_URL" }, customMessage: "only the broker host is substituted when the child has no proxy to exempt it from"); } - [Fact] - public void A_proxy_the_worker_passes_through_the_scrub_is_one_the_child_is_exempted_from() + [Theory] + [InlineData(null, "10.63.12.1")] + [InlineData(".corp.internal,gitlab.corp", ".corp.internal,gitlab.corp,10.63.12.1")] // the worker's own exemptions survive into the list, or the agent's git and pip to internal hosts go through the proxy + public void A_proxy_the_worker_passes_through_the_scrub_is_one_the_child_is_exempted_from(string? workerNoProxy, string expected) { var environment = new Dictionary { ["ANTHROPIC_BASE_URL"] = $"http://{SandboxSpec.ModelBrokerHostToken}:41234/r0uteId" }; + var worker = new Dictionary { ["https_proxy"] = "http://proxy.corp:3128" }; + if (workerNoProxy is not null) worker["NO_PROXY"] = workerNoProxy; + + var resolved = WithWorkerProxyEnvironment(worker, () => LocalProcessRunner.ResolveModelBrokerHost(EnvSpec() with { Environment = environment }, "10.63.12.1")); + + resolved.Environment["NO_PROXY"].ShouldBe(expected, "the worker's https_proxy survives the scrub and reaches the child, so the broker must be exempted from it"); + resolved.Environment["no_proxy"].ShouldBe(expected); + } + + [Fact] + public void A_proxy_the_task_hands_its_child_is_one_the_child_is_exempted_from() + { + // The task's own variables reach the child unfiltered, ALL_PROXY included — which Codex and curl honour — so the + // gate must count them whether or not the scrub would keep the worker's copy of the name. + var environment = new Dictionary { ["ANTHROPIC_BASE_URL"] = $"http://{SandboxSpec.ModelBrokerHostToken}:41234/r0uteId", ["ALL_PROXY"] = "socks5h://proxy.corp:1080" }; - var resolved = WithWorkerProxyEnvironment(new Dictionary { ["https_proxy"] = "http://proxy.corp:3128" }, () => LocalProcessRunner.ResolveModelBrokerHost(EnvSpec() with { Environment = environment }, "10.63.12.1")); + var resolved = WithWorkerProxyEnvironment(new Dictionary(), () => LocalProcessRunner.ResolveModelBrokerHost(EnvSpec() with { Environment = environment }, "10.63.12.1")); - resolved.Environment["NO_PROXY"].ShouldBe("10.63.12.1", "the worker's https_proxy survives the scrub and reaches the child, so the broker must be exempted from it"); + resolved.Environment["NO_PROXY"].ShouldBe("10.63.12.1", "a brokered call sent to the task's proxy fails like a provider outage"); + resolved.Environment["no_proxy"].ShouldBe("10.63.12.1"); } /// Run with every proxy variable of this process cleared but : the worker's own values are the fallback, and this host's must not leak in.