Skip to content

Refuse AF_PACKET - #1457

Open
Darren Hoehna (dhoehna) wants to merge 13 commits into
mainfrom
user/dahoehna/lxc-afpacket-seccomp
Open

Darren Hoehna (dhoehna) wants to merge 13 commits into
mainfrom
user/dahoehna/lxc-afpacket-seccomp

Conversation

@dhoehna

@dhoehna Darren Hoehna (dhoehna) commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

📖 Description

A program holding CAP_NET_RAW can build its own packets and hand them straight to the kernel, underneath the layer the sandbox's networking rules filter at. The rules report the traffic blocked and it leaves anyway. Dropping the capability stops that, but it also takes ICMP down, and MXC promises ICMP on every backend.

I have now found a way to deny a program the sockets that reach below the rules while keeping ICMP. SecComp.

A firewalled LXC workload is now refused four ways of getting one of those sockets:

  • AF_PACKET, which is where this started.
  • AF_XDP, which reaches the driver.
  • The legacy SOCK_PACKET form, where the kernel rewrites the family to a packet socket after the filter has already read it.
  • io_uring, which creates sockets without going through the socket syscall at all, so a filter on that syscall never sees them. A firewalled workload gets ENOSYS from the ring instead.

ping, TCP and UDP still work, and the workload still holds CAP_NET_RAW.

What this does not cover is written down in the backend documentation: a descriptor opened before the workload starts, and a socket family a future kernel adds.

I made two changes

  1. Refactor src/tools/lxc/src. The code is now way easier to read and understand. The flow is also easier to see.
  2. A firewalled workload is handed a filter the kernel enforces for the life of the process, so the refusals above happen before the socket is ever created.

🔗 References

https://task.ms/64150389

🔍 Validation

Added automatic tests, including one that runs the probe through the real firewalled attach path instead of calling the filter directly. It checks both directions: the routes above come back refused, and ICMP and ordinary sockets still come back working. Each denial test first checks that the kernel would have allowed the socket with no filter installed, so a test cannot pass just because the machine running it happens to lack a capability.

I ran the LXC suite and, because this code sits under both, the bubble wrap suite too. Both pass.

✅ Checklist

📋 Issue Type

  • [ x] Bug fix
  • Feature
  • Task

GitHub Actions runs the PR validation build automatically. The ADO pipeline
(MXC-PR-Build) is the Azure version of the PR pipeline, kept in parity with the GitHub
Actions build; it runs on merge to main, and Microsoft reviewers with write access can trigger it
on a PR with /azp run. See pull request builds.

If the dependency-feed-check check fails on a new dependency, the crate must be added to
the feed before the PR can pass. See pull request builds
for the steps.

Microsoft Reviewers: Open in CodeFlow

CAP_NET_RAW stays, because an explicit protocol: "icmp" allow needs a raw
socket, and it also opens AF_PACKET, which reaches the interface below the
hook the chains hang on. A syscall entering under numbering the filter was
not built for kills the process rather than passing unchecked.

AB#64150389

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9df5d37d-b3a2-487c-95df-ded398109b1d
@dhoehna
Darren Hoehna (dhoehna) requested a review from a team as a code owner October 9, 2026 18:03
Copilot AI balanced review requested due to automatic review settings October 9, 2026 18:03
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The filter remains bypassable through io_uring, Windows Hyperlight builds fail, and the CLI refactor introduces diagnostic regressions.

4 open findings
What changed in this PR

Adds LXC network confinement intended to block AF_PACKET while retaining CAP_NET_RAW for ICMP, alongside an executor CLI refactor.

Changes:

  • Adds a seccomp socket filter and capability assertions.
  • Refactors LXC CLI argument and mode handling.
  • Updates ICMP tests and LXC limitations documentation.
File Description
tests/​scripts/​run_lxc_network_ga_egress_test.sh Removes the host ICMP preflight.
tests/​configs/​lxc_network_ga_egress_icmp_allowed.json Exposes ping diagnostics.
src/​tools/​lxc/​src/​main.rs Delegates CLI operations to the new argument module.
src/​tools/​lxc/​src/​linux_executor_arguments.rs Encapsulates CLI parsing and operations.
src/​mxc-sdk/​tests/​wxc_e2e_tests_e2e_lxc_network_capability.rs Checks retained capabilities and seccomp mode.
src/​mxc-sdk/​src/​backends/​lxc/​common/​lxc_runner.rs Updates confinement test naming.
src/​mxc-sdk/​src/​backends/​lxc/​common/​lxc_bindings.rs Adds the seccomp AF_PACKET filter.
docs/​backends/​lxc/​lxc-backend.md Removes the raw-socket limitation.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/mxc-sdk/src/backends/lxc/common/lxc_bindings.rs Outdated
Comment thread src/mxc-sdk/tests/wxc_e2e_tests_e2e_lxc_network_capability.rs
Comment thread src/tools/lxc/src/main.rs
Comment on lines +86 to +88
pub fn should_display_avalible_backends(&self) -> bool {
self.available_backends
}

@MGudgin Gudge (MGudgin) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review summary

Requesting changes on the current PR head c6a95211a. The Windows x64 Hyperlight feature build fails (inline), and the security-sensitive filter has no behavioral test of the socket it is meant to refuse. Please address both before merge. The backend's broader packet-socket guarantee also needs either complete enforcement or an explicit limitation.

Scope and clean checks: The eight-file, +430/-288 local diff exactly matches gh pr diff 1457. The PR is behind the current base (a0b66e63c3); the reviewed three-dot range uses merge-base 7cd00d1ee6. The existing CAP_NET_ADMIN bounding-set drop is still present, and the E2E test still checks all three capability masks while retaining CAP_NET_RAW. Its test count remains one before and after the PR. The performance pass found no material new hot-path cost: the filter is fixed-size and installed once. These checks do not establish effective packet filtering.

Already raised on this PR (not duplicated inline)

  • High — behavioral packet-socket test missing (introduced_by_change): the E2E assertion reads Seccomp: 2, which any inherited filter can provide; it never attempts socket(AF_PACKET, ...). Existing discussion r4233182128 covers this. Require EPERM for the denied socket and positive ICMP/ordinary-socket controls; ideally prove the test fails with this filter removed.
  • Medium — optional --log-file failure aborts all modes (introduced_by_change): main.rs:29-37 now exits instead of warning, even for --available-backends. Existing discussion r4233182199 covers this.
  • Medium — io_uring socket path is not covered (claim_mismatch): the added filter examines SYS_socket only, while Linux io_socket calls __sys_socket_file without a userspace socket(2) call. Existing discussion r4233182073 identifies this route. It predates this PR and is not, by itself, a new exploit attributed to the change or an independent merge condition; whether it is usable depends on host policy. It does contradict the PR's claim to deny packet-capable sockets.

Claim gaps and additional regression coverage

  • Medium — legacy packet socket (claim_mismatch, PR description): the claim that this denies RAW/PACKAGE sockets is wider than the AF_PACKET-family check. Linux __sock_create translates PF_INET + SOCK_PACKET into PF_PACKET after seccomp sees the original family; CAP_NET_RAW remains available. Reject the legacy family/type pair or retain the limitation and test it.
  • Medium — AF_XDP (claim_mismatch, PR description): a kernel with usable AF_XDP can create AF_XDP / SOCK_RAW with CAP_NET_RAW and transmit link-layer frames; this family is allowed by the added filter. Actual bind/transmit availability depends on the host and other policy, so verify it in the supported environment before claiming this route is closed. Restrict it if the guarantee is meant to include all link-layer transmission.
  • Medium — BPF table regression seam (introduced_by_change, lxc_bindings.rs:223-232): positional branch offsets determine allow/deny/kill. This is part of the behavioral-test blocker above rather than a separate blocking issue. A pure builder or isolated child-process test for native/foreign ABI, x32, AF_PACKET, legacy packet type, and ordinary sockets would make changes to that table reviewable.
  • Medium — CLI contract tests (introduced_by_change, linux_executor_arguments.rs:94-115, main.rs:19-69): the refactor changes log failures, no-config errors and delete output with no targeted CLI tests. Add subprocess checks for exit code, stdout/stderr, mode precedence and invalid configuration. The separate parser logger and output regressions are also noted inline.
  • Low — ICMP harness diagnostic (introduced_by_change, tests/scripts/run_lxc_network_ga_egress_test.sh:283-289): the deleted peer ping preflight previously distinguished an unreachable/echo-silent peer from firewall denial. The allowed ICMP test still fails, but now reports a less precise cause; MXC_PING_MISSING is emitted without a harness check. Reinstate a reliable peer check or explicitly fail on that marker.

Verified pre-existing, not charged as introduced defects: Linux's PF_INET/SOCK_PACKET, io_uring socket creation and AF_XDP support predate this PR. They are listed above only as qualified mismatches with the PR's newly asserted security guarantee, capped at Medium. No finding is filed merely because unchanged code exists elsewhere. The signal-watchdog call also moved, but follow-up inspection found no Linux thread-spawning work before it; I am not filing a present-day signal bug on that basis.

The inline comments cover the newly introduced build failure, compat-ABI termination, seccomp-install prerequisite, parsing diagnostics and changed CLI outputs. The Windows feature-build failure was reproduced by cargo check -p lxc --features hyperlight; Linux LXC socket probes were not run on this Windows host.

}

//KVM is checked before anything is pulled.
#[cfg(target_os = "linux")]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

High (maintainability, cross-platform parity) — Windows x64 Hyperlight builds fail.

Attribution: introduced_by_change — this extracted helper now returns Result, but the setup/result expression is inside the Linux-only block. With hyperlight on Windows x64, the WHP check is the only remaining expression; its success path evaluates to (), producing E0317. cargo check -p lxc --features hyperlight reproduced this on Windows.

Fix: Keep OS-specific WHP/KVM preconditions separate, then run the shared parse_runtimes/setup code on both platforms (or provide a Windows branch returning Result).

libc::sock_filter { code: JUMP_IF_EQUAL, jt: 1, jf: 0, k: libc::AF_PACKET as u32 }, // return error if using AF_PACKET
libc::sock_filter { code: RETURN, jt: 0, jf: 0, k: libc::SECCOMP_RET_ALLOW },
libc::sock_filter { code: RETURN, jt: 0, jf: 0, k: REFUSE_WITH_EPERM },
libc::sock_filter { code: RETURN, jt: 0, jf: 0, k: libc::SECCOMP_RET_KILL_PROCESS },

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium (reliability) — compat-ABI workloads are killed on their first syscall.

Attribution: introduced_by_change — this new SECCOMP_RET_KILL_PROCESS branch handles foreign audit architectures and x32 syscall numbers. A 32-bit program under an installed firewall can now die with SIGSYS before it opens any network socket, rather than receiving a meaningful sandbox-policy error. This may be a deliberate fail-closed policy, but it is neither documented nor exercised by the new test.

Fix: Document and test the supported ABI restriction with a useful diagnostic, or add a compat-ABI policy that still refuses packet sockets.

let taken = unsafe {
libc::syscall(
libc::SYS_seccomp,
libc::SECCOMP_SET_MODE_FILTER as libc::c_ulong,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium (reliability) — filter installation adds an opaque privilege prerequisite.

Attribution: introduced_by_change — SECCOMP_SET_MODE_FILTER is new here. It requires CAP_SYS_ADMIN in the caller's user namespace or an already-set no_new_privs; the code does not set the latter. A caller with enough privilege for the existing PR_CAPBSET_DROP but not this new step now fails a firewalled run. The returned bare OS error does not identify which pre_exec step failed.

Fix: Document the new prerequisite, distinguish the seccomp failure without allocating unsafely in pre_exec, and test the failing-install path. Do not set no_new_privs indiscriminately because it changes privileged-exec behavior.

(String::new(), false)
};

let mut logger = Logger::new(if self.debug {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium (correctness) — request diagnostics no longer reach --log-file.

Attribution: introduced_by_change — get_request constructs a second logger with no file sink, then passes it to load_one_shot_request. Previously parsing used the logger configured in main, so validation warnings and parse errors were recorded in the requested log. The new logger is discarded, even when the CLI configured a file sink successfully.

Fix: Pass the already-configured logger into the request parser and preserve its diagnostics through the error path.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rechecked at 7c9b02fd6: main still configures the --log-file sink on one logger, while get_request() creates a second logger and passes that one to load_one_shot_request(). The follow-up restored the missing-config message and made log-file open failures nonfatal, but it did not reconnect request-parsing diagnostics to the configured file sink.

Please pass main's logger into request parsing, preserve the existing stderr error reporting, and add a CLI regression test that checks a parse/validation diagnostic reaches --log-file. This finding remains open.

} else if let Some(ref path) = self.config_path {
(path.clone(), false)
} else {
(String::new(), false)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low (reliability) — no-argument invocation loses its actionable error.

Attribution: introduced_by_change — the old main explicitly exited with Error: No config provided. Use a positional path, --config, or --config-base64. This new fallback passes an empty string into the parser and emits a generic Request error instead.

Fix: Restore the explicit missing-config check and its usage guidance before calling load_one_shot_request.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed by 0aa18c820: get_request() again prints the actionable "No config provided" message and exits before parsing an empty path. Nothing further is needed for this specific comment; the separate parser-logger issue remains in its own thread. Thanks for fixing this.

Comment thread src/tools/lxc/src/main.rs
} else if arguments.does_user_want_to_delete_a_container() {
match arguments.delete_container() {
Ok(message) => {
eprintln!("{message}");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low (reliability) — delete-mode output moved from stdout/file log to stderr.

Attribution: introduced_by_change — the old delete path logged its result through the CLI logger and printed the buffer on stdout. This new success path prints only to stderr; the --log-file sink also misses the result. Automation reading the previous output stream sees no success message.

Fix: Preserve the old output/logging contract, or document and test an intentional CLI-breaking change.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rechecked at 7c9b02fd6: delete mode still sends both success and failure messages through eprintln! in main.rs:60-66. Before the refactor, the default buffered logger sent delete results to stdout and, when configured, its --log-file sink. The missing-config and nonfatal log-file fixes did not restore this output contract.

Please restore the previous delete output/logging behavior, or document an intentional change and add CLI subprocess tests for the output streams and exit codes (a nonexistent container can cover the failure path without provisioning LXC). This finding remains open.

@microsoft-github-policy-service microsoft-github-policy-service Bot added the Needs-Author-Feedback Waiting for additional information or action from the issue or pull-request author. label Oct 9, 2026
The setup body moved inside a target_os = "linux" block, which broke the
lxc-exec build on Windows and macOS x86_64 and left --setup-hyperlight with
nothing to do on either.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1654c073-27eb-43cf-82f3-417614ce845b
Copilot AI balanced review requested due to automatic review settings October 9, 2026 22:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The AF_PACKET restriction remains bypassable through io_uring, and the CLI refactor introduces diagnostic regressions.

4 open findings
Previously missed (2)

In code that hasn't changed since last review

Medium severity Reuse configured logger for parser diagnostics

src/​tools/​lxc/​src/​linux_executor_arguments.rs:109

This second logger is not the configured logger from main, so successful parsing no longer sends parser diagnostics to --log-file, and retained parser warnings are discarded with this local value. Accept &mut Logger in get_request and pass the existing logger through to load_one_shot_request to preserve the prior diagnostic stream.

Low severity Clarify retained CAP_NET_RAW in security comment

src/​mxc-sdk/​src/​backends/​lxc/​common/​lxc_bindings.rs:142

This security comment says both capabilities are removed, but the implementation intentionally retains CAP_NET_RAW and restricts only AF_PACKET via seccomp. Describe the actual split so future changes do not treat the retained capability as an accidental omission.

🧠 Review effort: Balanced

The container check only asserted that a filter was attached, which a
permissive filter passes identically. A paired unit test now opens a packet
socket with and without the filter: allowed unfiltered, EPERM filtered.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1654c073-27eb-43cf-82f3-417614ce845b
A missing config exited silently on an empty path, runtime parse failures lost
their Error: prefix, the signal handler installed after work had begun, and an
unreadable backend list exited 1 instead of warning and continuing.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1654c073-27eb-43cf-82f3-417614ce845b
The later cases read an unanswered echo as a firewall verdict. A peer that
never answers ICMP produces that same silence, which passes the run without
anything having been enforced.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1654c073-27eb-43cf-82f3-417614ce845b
Copilot AI balanced review requested due to automatic review settings October 9, 2026 22:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The filter remains bypassable through io_uring, introduces kernel compatibility concerns, and the CLI refactor regresses logging behavior.

4 open findings
1 resolved since last review
Previously missed (2)

In code that hasn't changed since last review

Medium severity Use configured logger for request parsing diagnostics

src/​tools/​lxc/​src/​linux_executor_arguments.rs:112

The new private logger disconnects config parsing from the logger configured in main. Consequently, successful normalization warnings are discarded in buffered mode, and neither warnings nor parse errors reach --log-file; load_one_shot_request emits both through the logger it receives. Pass main's &mut Logger into get_request instead of constructing another one.

Medium severity Delete results bypass logger and redirect success output

src/​tools/​lxc/​src/​main.rs:78

This refactor bypasses the configured logger for delete results, so --delete --log-file ... no longer records either success or failure. It also moves the success message from stdout to stderr. Write both outcomes to the logger's diagnostic sink and keep successful user output on stdout.

🧠 Review effort: Balanced

Comment on lines +240 to +244
libc::syscall(
libc::SYS_seccomp,
libc::SECCOMP_SET_MODE_FILTER as libc::c_ulong,
NO_FLAGS,
&header as *const libc::sock_fprog,
The watchdog destroys a running container on a fatal signal, and nothing
earlier in main creates one.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1654c073-27eb-43cf-82f3-417614ce845b
Copilot AI balanced review requested due to automatic review settings October 9, 2026 23:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The filter remains bypassable through io_uring, and the CLI refactor introduces diagnostic and output regressions.

4 open findings
Previously missed (4)

In code that hasn't changed since last review

Medium severity Check CapEff instead of effective UID

src/​mxc-sdk/​src/​backends/​lxc/​common/​lxc_bindings.rs:1667

EUID 0 does not imply that the process has CAP_NET_RAW (for example, a hardened container can run root with that capability dropped), and a non-root process can also be granted the capability. This makes the control fail based on the test host's capability configuration rather than filter behavior. Compare the probe result with bit 13 of /proc/self/status's CapEff mask instead.

Medium severity Use the configured logger for request parsing

src/​tools/​lxc/​src/​linux_executor_arguments.rs:112

This creates a second logger after main has configured --log-file, so parsing no longer uses the configured sink. Parse failures are absent from the requested log file, and successful normalization diagnostics buffered here are dropped with this local logger. Pass the already-configured logger into get_request instead of constructing another one.

Medium severity Preserve logger routing for deletion results

src/​tools/​lxc/​src/​main.rs:67

This branch previously routed the deletion result through logger, which both preserved the CLI's stdout behavior and wrote it to --log-file. Sending both outcomes directly to stderr silently changes the command contract and leaves the configured file sink empty.

Low severity Clarify the actual capability and socket-filtering split

src/​mxc-sdk/​src/​backends/​lxc/​common/​lxc_bindings.rs:142

The comment says both capabilities are removed, but the implementation intentionally retains CAP_NET_RAW. The parenthetical leaves the security contract contradictory; state the actual split between the capability drop and socket filtering directly.

🧠 Review effort: Balanced


/// Removing CAP_NET_RAW prevents a process from using RAW and PACKET sockets. However, ICMP is also denied.
/// LXC needs to allow ICMP and prevent programs from using sockets to
/// bypass the network guards. The solution is to give the kernel a custom filter to refuse

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the AF_PACKET socket probe. It verifies what refuse_packet_sockets() does when called directly, but the PR’s broader claim—that a firewalled workload cannot use packet-capable sockets to bypass the egress chains—needs either additional enforcement or a narrower stated scope.

Could you check these routes against the seccomp filter in refuse_packet_sockets()?

  1. Legacy packet sockets: Linux can turn socket(PF_INET, SOCK_PACKET, …) into a packet socket after the filter has inspected the PF_INET family argument. Reject that family/type combination as well, accounting for socket-type flags, and test that it returns the intended error without blocking permitted ICMP raw sockets.
  2. io_uring: Check whether a confined workload can use IORING_OP_SOCKET to create an AF_PACKET socket. That operation does not pass through the socket(2) syscall checked by this filter. If io_uring is unnecessary for these workloads, consider denying its entry path and checking the compatibility impact. If it must remain available, enforce the socket-family restriction at a layer that also covers this creation path.
  3. AF_XDP: Check whether a workload with its actual container capabilities and network device can set up an AF_XDP transmit socket and send a frame outside the IP chains. Merely opening the socket would not prove a bypass. If transmission is possible, confine that route too; if another enforced restriction prevents it, document that restriction.

Please test applicable routes through the real firewalled LXC attach path, not only by calling the filter function in a unit-test child. Include denied-socket checks and positive checks that permitted ICMP and ordinary sockets still work. If any route is intentionally outside this PR, narrow the PR description and backend documentation to state what this filter guarantees and which other restrictions the broader claim depends on.

@dhoehna Darren Hoehna (dhoehna) Oct 13, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs either additional enforcement or a narrower stated scope.

I agree. The first copilot review comment stated the same in regards to io_uring. I'm still looking into it

@microsoft-github-policy-service microsoft-github-policy-service Bot added Needs-Attention Requires attention or a decision from the MXC maintainers. and removed Needs-Author-Feedback Waiting for additional information or action from the issue or pull-request author. labels Oct 13, 2026
A firewalled workload could still reach a packet socket three ways the family check never sees: SOCK_PACKET, which the kernel rewrites to AF_PACKET after the family argument is read; AF_XDP; and a ring, which creates sockets without the socket system call.

CAP_NET_RAW stays in place.  Dropping it would take ICMP with it, which every backend promises.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fb9b6565-8ada-47a6-8b33-db655ee9a7a0
Copilot AI balanced review requested due to automatic review settings October 13, 2026 21:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The CLI refactor bypasses configured diagnostic logging during request parsing and container deletion.

4 open findings
2 resolved since last review

🧠 Review effort: Balanced

Comment on lines +108 to +112
let mut logger = Logger::new(if self.debug {
Mode::Console
} else {
Mode::Buffer
});
Comment thread src/tools/lxc/src/main.rs
} else if arguments.does_user_want_to_delete_a_container() {
match arguments.delete_container() {
Ok(message) => {
eprintln!("{message}");
The route-by-route detail is already in the backend documentation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fb9b6565-8ada-47a6-8b33-db655ee9a7a0
Copilot AI balanced review requested due to automatic review settings October 13, 2026 21:36
Both arguments are byte offsets into args[], not one derived from the other.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fb9b6565-8ada-47a6-8b33-db655ee9a7a0

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The CLI refactor introduces logging, output compatibility, probe side-effect, and capability-test regressions.

4 open findings
Previously missed (2)

In code that hasn't changed since last review

Medium severity Test confinement against required capabilities and no_new_privs

src/​mxc-sdk/​src/​backends/​lxc/​common/​lxc_bindings.rs:1586

Confinement now also installs seccomp, which requires CAP_SYS_ADMIN unless no_new_privs is set, but the test below still predicts success solely from effective UID and reports only CAP_SETPCAP. A UID-0 process can lack either capability in a restricted user namespace/container, causing a false failure and leaving the new prerequisite uncharacterized. Base the expectation on the actual capability sets or isolate the two privileged operations.

Medium severity Probe availability before initializing the log file

src/​tools/​lxc/​src/​main.rs:30

The backend probe is documented as a read-only fast path handled immediately after argument parsing (docs/development/plans/backend-support-probe-api.md:192-195). Opening the requested log first can create or append a host file for lxc-exec --available-backends --log-file ..., so this refactor breaks that contract. Move the availability branch ahead of logger/file-sink initialization.

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 13, 2026 21:42
The old wording claimed liburing falls back on ENOSYS, which was never measured.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fb9b6565-8ada-47a6-8b33-db655ee9a7a0

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The executor refactor loses parser and delete diagnostics from --log-file and changes delete output behavior.

4 open findings
Previously missed (1)

In code that hasn't changed since last review

Low severity Correct comment about retained CAP_NET_RAW and socket filtering

src/​mxc-sdk/​src/​backends/​lxc/​common/​lxc_bindings.rs:142

This comment says both capabilities are removed even though the implementation deliberately retains CAP_NET_RAW; it also implies all raw sockets bypass the chains, while the bypass being addressed is packet-capable socket creation. That contradicts the implementation and the backend documentation, so describe the drop/filter split directly.

🧠 Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 13, 2026 21:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The CLI refactor drops configured parser and deletion diagnostics from the log sink.

4 open findings
Previously missed (1)

In code that hasn't changed since last review

Low severity Correct security comment about retained CAP_NET_RAW

src/​mxc-sdk/​src/​backends/​lxc/​common/​lxc_bindings.rs:142

This security comment contradicts the implementation: only CAP_NET_ADMIN is removed, while CAP_NET_RAW is intentionally retained and constrained with seccomp. Stating that both capabilities are removed can mislead future changes to this confinement boundary.

🧠 Review effort: Balanced

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs-Attention Requires attention or a decision from the MXC maintainers.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants