Skip to content

feat(firewall): open XNet port to all nodes in the network#10263

Open
pierugo-dfinity wants to merge 2 commits into
masterfrom
pierugo/firewall/open-xnet-to-all-nodes
Open

feat(firewall): open XNet port to all nodes in the network#10263
pierugo-dfinity wants to merge 2 commits into
masterfrom
pierugo/firewall/open-xnet-to-all-nodes

Conversation

@pierugo-dfinity
Copy link
Copy Markdown
Contributor

@pierugo-dfinity pierugo-dfinity commented May 20, 2026

In an effort to allow XNet connections between cloud engines and other subnets, this PR opens port 2497 (the XNet port) to all nodes in the network.

This is done by splitting the current [tcp/udp]_ports_for_node_whitelist config field into two trusted_[tcp/udp]_ports_for_node_whitelist and untrusted_[tcp/udp]_ports_for_node_whitelist. The trusted ports are open only to "whitelisted" nodes (i.e., non-type4 nodes for non-type4 nodes, and all nodes for type4 nodes, as dictated by the is_whitelisted function in firewall.rs), while the untrusted ports are always open to all nodes.

@pierugo-dfinity pierugo-dfinity added the CI_ALL_BAZEL_TARGETS Runs all bazel targets label May 20, 2026
@github-actions github-actions Bot added the feat label May 20, 2026
@pierugo-dfinity pierugo-dfinity marked this pull request as ready for review May 20, 2026 12:01
@pierugo-dfinity pierugo-dfinity requested review from a team as code owners May 20, 2026 12:01
Comment on lines +324 to +327
trusted_tcp_ports_for_node_whitelist: [22, 4100, 8080],
trusted_udp_ports_for_node_whitelist: [4100],
untrusted_tcp_ports_for_node_whitelist: [2497],
untrusted_udp_ports_for_node_whitelist: [],
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.

I'm not sure I understand what trusted/untrusted means in this context. Maybe something like this?

Suggested change
trusted_tcp_ports_for_node_whitelist: [22, 4100, 8080],
trusted_udp_ports_for_node_whitelist: [4100],
untrusted_tcp_ports_for_node_whitelist: [2497],
untrusted_udp_ports_for_node_whitelist: [],
whitelisted_node_tcp_ports: [22, 4100, 8080],
whitelisted_node_udp_ports: [4100],
all_node_tcp_ports: [2497],
all_node_udp_ports: [],

I think that would also be closer to the existing naming in the orchestrator

Copy link
Copy Markdown
Contributor Author

@pierugo-dfinity pierugo-dfinity May 20, 2026

Choose a reason for hiding this comment

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

Right, renamed here. I kept the "whitelist" at the end in contrast to the next field ports_for_http_adapter_blacklist

@pierugo-dfinity pierugo-dfinity force-pushed the pierugo/firewall/open-xnet-to-all-nodes branch from d9cd7d0 to 712017d Compare May 20, 2026 15:11
Copy link
Copy Markdown
Contributor

@Bownairo Bownairo left a comment

Choose a reason for hiding this comment

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

Approving node changes

// Insert the whitelisting rules at the top of the list (highest priority)
tcp_rules.insert(0, tcp_node_whitelisting_rule);
udp_rules.insert(0, udp_node_whitelisting_rule);
// Insert the ic-http-adapter rule at the top of the list (highest priority)
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.

The comment here suggests that the order matters. Should we preserve it, and also add one above in get_node_whitelisting_rules?

Comment on lines +324 to +327
whitelisted_nodes_tcp_ports_whitelist: [22, 4100, 8080],
whitelisted_nodes_udp_ports_whitelist: [4100],
all_nodes_tcp_ports_whitelist: [2497],
all_nodes_udp_ports_whitelist: [],
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.

Is there any system test we could adapt to test this?

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants