refactor(openshell-sandbox): Split sandbox into process and network subcrates.

[rrhubenov]

Summary

This PR is an implementation of #1305.
For full context on why this is required, please read #981 and #1305 in that order.

In summary, this split has been proposed for 2 main reasons:

• As a precursor to a possible split-pod topology of the supervisor (previously sandbox)
• In general a better decomposition of the 2 main "categories" of functionality ("process" and "network")

Although this change has been mostly evaluated from the perspective of the "split pod" topology, this splitting allows for a more agile topology regardless of environment. If for any reason the "process" and "network" functionality need to work as standalone processes, this split is required.

Review suggestions:

This refactor is hard to review due to a big collection of changes that are mostly movements of files and/or code blocks. For this reason, I suggest that reviewers take the approach of reviewing the changes commit by commit. I have taken care that each commit is as small as possible, moving only one functionality (module, function, etc.) in one commit.

Bear in mind that during the implementation, one very important architectural decision is that the 2 new crates (supervisor-network and supervisor-process) do not have a dependency on one another.
Only the "orcherstrator" crate, that being the openshell-supervisor should have a dependency on both of them.
One other important point was that no behavioural changes should occur as a side-effect from this refactoring.

NOTE:
I haven't renamed the openshell-sandbox crate to openshell-supervisor since if done fully (e.g. renaming the binary as well), would result in a breaking change which I see as out of scope for this PR.

I have also tried to minimise the amount of architectural design decisions in this PR, since it is already big enough.

Related Issue

Closes #1305

Changes

Testing

• mise run pre-commit passes
• Unit tests added/updated (no update was required due to this change being only a refactor) N/A
• E2E tests added/updated (if applicable) N/A

Checklist

• Follows Conventional Commits
• Commits are signed off (DCO)
• Architecture docs updated (if applicable) N/A - review determined that docs do not go into implementation details

[derekwaynecarr]

/ok to test 1e22061

[copy-pr-bot]

/ok to test 1e22061

@derekwaynecarr, there was an error processing your request: E2

See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/

[derekwaynecarr]

/ok to test 1e22061

[copy-pr-bot]

/ok to test 1e22061

@derekwaynecarr, there was an error processing your request: E2

See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/

[TaylorMutch]

/ok to test 33e00fd

[github-actions]

Label test:e2e applied for 33e00fd. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

[drew]

This is looking good, thanks for taking this on.

I haven't renamed the openshell-sandbox crate to openshell-supervisor since if done fully (e.g. renaming the binary as well), would result in a breaking change which I see as out of scope for this PR.

Good call, we can figure this out separately.

One area I focused the review on were packages moved to openshell-core. Generally these looked good. Moving grpc client helpers, policy infra, provider components to core all make sense. Left a comment around the netns/nft_ruleset moves.

[drew]

(from codex review)

run_networking starts before run_process stores the child PID, so loading entrypoint_pid once here usually captures 0. The retry loop then keeps probing /proc/0/root and never reloads OPA with the real workload root, which regresses symlinked binary policies such as /usr/bin/python3.

Suggested fix: wait for a non-zero PID separately, then start the existing /proc//root probe loop. That keeps the old 5s retry window focused on “is proc root ready?” instead of spending it before the process has even spawned.

  let mut pid = 0;
  for attempt in 1..=20 {
      pid = resolve_pid.load(Ordering::Acquire);
      if pid != 0 {
          break;
      }
      debug!(attempt, "Entrypoint PID not available yet, retrying symlink resolution");
      tokio::time::sleep(Duration::from_millis(250)).await;
  }

  if pid == 0 {
      warn!("Entrypoint PID not available; binary symlink resolution skipped");
      return;
  }

  let probe_path = format!("/proc/{pid}/root/");
  for attempt in 1..=10 {
      tokio::time::sleep(Duration::from_millis(500)).await;
      if std::fs::metadata(&probe_path).is_ok() {
          // existing reload_from_proto_with_pid(&resolve_proto, pid) block
          return;
      }
  }

[drew]

Now that openshell-sandbox is acting as the orchestrator over the process/network leaf crates, this manifest still appears to carry a lot of leaf-only dependencies from the old monolith (regorus, russh, rcgen, tokio-rustls, ipnet, apollo-parser, openshell-router, etc.).

Can we prune the direct deps here to only what the orchestrator actually uses?

[drew]

netns/nft_ruleset probably don't belong in openshell-core. These modules are privileged Linux supervisor runtime implementation: they create namespaces, manage veths, invoke ip/nsenter/nft, install bypass rules, and emit sandbox OCSF events. That makes core own sandbox enforcement machinery, not just shared types/config.

Can we keep this out of core? The process leaf appears to only need the namespace fd for setns, so one option is for the network/orchestrator side to own the NetworkNamespace RAII handle and pass an Option<i32> fd into run_process. If more sharing is needed, a small supervisor-runtime/netns crate would preserve the no process <-> network dependency rule without expanding openshell-core into privileged runtime code.

[rrhubenov]

Absolutely, I've generally tried to keep the number of additions to openshell-core as minimal as possible, but this specific module was hard to position correctly, since it's shared by both crates and I didn't want to create a 3rd crate since it would have put even more strain on the PR.

Thank you for the solution with passing the fd! I hadn't seen that this simplification was possible. I was generally trying not to introduce API changes to existing functions, structs, etc., again to simplify the PR.

But now looking at this with a fresh pair of eyes, I would even take this a step further and argue that all network isolation and nftables logic should be moved fully to the supervisor-process crate instead. My logic being that this also would classify as "process isolation", even tough it's "networking related".
This is from the perspective that in my view, if the network (previously proxy) part would be one day deployable by itself, it would not be responsible for these things. Rather the supervisor-process would have that responsibility.
I would even apply this logic to the bypass monitor!

We can leave this as a discussion for the future, if you would prefer that.

[rrhubenov]

I think this boils down to strictly identifying/defining the set of responsibilities of both supervisor-network and supervisor-process.
In my opinion it should be:

• supervisor-network: roughly credential injection, policy enforcement, TLS interception, HTTP CONNECT proxy
• supervisor-process: roughly everything else (all process isolation techniques, bypass monitoring, ssh server)

[drew]

That split works for me 🚢

[drew]

/ok to test 8526c54

[drew]

/ok to test 7a3fdd7

[rrhubenov]

@drew Thank you for the review, I've applied changes based on your comments.

Notable new things are:

1. Denial aggregator is now part of the orchestrator (openshell-sandbox) rather than the
   openshell-supervisor-process crate.

Moving the bypass monitor to the process leaf crate caused the need for both leaf crates
to send denial events to the aggregator. This also lead to them both having to depend
on the DenialEvent type, consequently it had to be moved to the core crate.

I think having the aggregator in the orchestrator is actually a pretty natural fit.

2. Bypass monitor moved to process leaf crate as per our discussion.

Only nastier thing here is that a subset of procfs had to be copied into the process
leaf crate, since the network crate is also using it. The same logic that applied to
netns applies to procfs, so it did not fit well in openshell-core. Alternatively a
separate crate could be created for procfs, but I opted out of it since I did not want
to strain the PR. I think a future cleanup could handle this, or I can create the new
crate in this PR, if preferred.

4. There was some leftover SSH code in the network crate.

This acted as both a cleanup and a prerequisite to moving netns into the process crate.

5. netns was moved from core to process leaf crate.

This was easy to do after the previous steps, however one notable thing is that the
NetworkNamespace struct had to be created in the orchestrator and passed downwards to
the run_process func. This was due to the orchestrator having a longer lifetime than the
run_process & run_network funcs, so it was a more natural fit for it to hold the RAII
handle.

There's probably a bunch of nice followup refactors and designs that can be applied, but
I'm not sure this PR is correct for that.

6. Applied your suggested fix for the PID of the sandboxed process.

7. Nasty merge conflict had to be fixed.

Opted for a merge commit since rebasing would have taken ages and would have been error
prone. Most interesting thing there is the movement of the activity-aggregator component
and thread. It very much follows the design of the denial-aggregator (shared types in
core, thread spawned by orchestrator, transfer channels passed downstream to the leaf
crates).

[drew]

@rrhubenov thanks for addressing the comments. Do you mind creating a followup ticket just so we don't lose track of the remaining cleanups? Otherwise this looks good to me and seems like a good start towards getting the supervisor running outside the agent container.

I'll let @derekwaynecarr give the final 👍.

[drew]

/ok to test ed12505