-
Notifications
You must be signed in to change notification settings - Fork 128
Add block_network to Sandboxes #1488
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 issues found across 15 files
Prompt for AI agents (all 4 issues)
Understand the root cause of the following 4 issues and fix them.
<file name="pkg/types/backend.go">
<violation number="1" location="pkg/types/backend.go:463">
`BlockNetwork` flag is never propagated to protobuf ContainerRequest, so requests that enable it lose the setting and still allow networking.</violation>
</file>
<file name="pkg/worker/network.go">
<violation number="1" location="pkg/worker/network.go:515">
The DROP rule needs to allow established or related traffic; as written it blocks responses for exposed ports, breaking inbound connectivity when block_network is enabled.</violation>
<violation number="2" location="pkg/worker/network.go:524">
Rule violated: **Prevent Redundant Code Duplication**
The IPv6 prefix/address calculation here duplicates the earlier block in configureContainerNetwork (lines ~437-446). Please extract this logic into a shared helper or reuse the previously computed value to comply with the Prevent Redundant Code Duplication guideline.</violation>
<violation number="3" location="pkg/worker/network.go:528">
This IPv6 DROP rule also needs to exempt established/related traffic; currently it drops reply packets for exposed ports, so inbound IPv6 connections break under block_network.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed changes from recent commits (found 2 issues).
2 issues found across 3 files
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="pkg/worker/network.go">
<violation number="1" location="pkg/worker/network.go:515">
Allowing conntrack RELATED traffic lets BlockNetwork containers initiate outbound connections (e.g. FTP active mode) and bypass the egress block.</violation>
<violation number="2" location="pkg/worker/network.go:528">
IPv6 BlockNetwork rule also allows RELATED traffic, letting exposed services initiate outbound connections and defeating the block.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed changes from recent commits (found 2 issues).
2 issues found across 1 file
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="pkg/worker/network.go">
<violation number="1" location="pkg/worker/network.go:515">
Dropping RELATED packets here prevents the container from emitting ICMP/TCP control traffic tied to inbound connections (e.g., PMTU, RST), which will break legitimate clients. Please keep RELATED permitted alongside ESTABLISHED.</violation>
<violation number="2" location="pkg/worker/network.go:528">
We also need to permit conntrack RELATED traffic for IPv6; otherwise ICMPv6 control replies (vital for PMTU/ND) are dropped, breaking inbound connections. Please restore RELATED in the allowed state list.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed changes from recent commits (found 1 issue).
1 issue found across 1 file
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="pkg/worker/network.go">
<violation number="1" location="pkg/worker/network.go:236">
Please handle the error returned by netns.Set(hostNS); if the switch back to the host namespace fails we should return that error rather than continuing in the wrong namespace.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 1 file
|
I'm unsure if I should allow RELATED packets. Blocking them breaks ICMP/control messages tied to established connections but it reduces the isolation block_network provides |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 1 file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed changes from recent commits (found 1 issue).
1 issue found across 3 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="pkg/types/types.proto">
<violation number="1" location="pkg/types/types.proto:93">
Changing the existing `block_network` field number from 26 to 28 breaks protobuf compatibility; existing clients will interpret wire data incorrectly. Please keep `block_network` at its original tag and assign new numbers to the added fields.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 1 file
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="pkg/worker/network.go">
<violation number="1" location="pkg/worker/network.go:542">
Rule violated: **Prevent Redundant Code Duplication**
These three added lines duplicate the existing logic in pkg/worker/network.go:787 that builds the truncated container ID, veth host name, and comment for iptables. Please extract this into a shared helper to avoid violating the Prevent Redundant Code Duplication guideline.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| return err | ||
| } | ||
|
|
||
| truncatedContainerId := containerId[len(containerId)-5:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rule violated: Prevent Redundant Code Duplication
These three added lines duplicate the existing logic in pkg/worker/network.go:787 that builds the truncated container ID, veth host name, and comment for iptables. Please extract this into a shared helper to avoid violating the Prevent Redundant Code Duplication guideline.
Prompt for AI agents
Address the following comment on pkg/worker/network.go at line 542:
<comment>These three added lines duplicate the existing logic in pkg/worker/network.go:787 that builds the truncated container ID, veth host name, and comment for iptables. Please extract this into a shared helper to avoid violating the Prevent Redundant Code Duplication guideline.</comment>
<file context>
@@ -539,10 +539,14 @@ func (m *ContainerNetworkManager) setupBlockNetwork(containerId string, request
return err
}
+ truncatedContainerId := containerId[len(containerId)-5:]
+ vethHost := fmt.Sprintf("%s%s", containerVethHostPrefix, truncatedContainerId)
+ comment := fmt.Sprintf("%s:%s", vethHost, containerId)
</file context>
Things to consider
|
Setting block_network to True blocks all outbound network traffic from a Sandbox excluding from exposed ports
Summary by cubic
Added a block_network option to Sandboxes/Pods to block all outbound network traffic while still allowing inbound connections to exposed ports. The flag is wired through the API, SDK, and worker to enforce at runtime.
Written for commit 1469875. Summary will update automatically on new commits.