feature: isolation among all netavark bridge #703

yassi-github · 2023-05-29T14:57:43Z

Currently, it appears that isolation is only enabled when both bridges are in isolate mode.
This means that communication with networks that do not have isolate enabled is still possible.

What about using isolate=strict to deny communication with networks that do not have isolate enabled?
Specifically, add a new chain of iptables and add a DROP rule to the chain for networks that do not have isolate enabled.

Here is a table showing the connectivity between networks before and after this change:

before:

|send\recv  |nonisol1   |nonisol1   |isol1      |isol2      |
|-----------|-----------|-----------|-----------|-----------|
|nonisol1   |     o     |     o     |     o     |     o     |
|nonisol2   |     o     |     o     |     o     |     o     |
|isol1      |     o     |     o     |     o     |     x     |
|isol2      |     o     |     o     |     x     |     o     |

after:

|send\recv  |nonisol1   |nonisol1   |isol1      |isol2      |strictisol1|strictisol2|
|-----------|-----------|-----------|-----------|-----------|-----------|-----------|
|nonisol1   |     o     |     o     |     o     |     o     |     x     |     x     |
|nonisol2   |     o     |     o     |     o     |     o     |     x     |     x     |
|isol1      |     o     |     o     |     o     |     x     |     x     |     x     |
|isol2      |     o     |     o     |     x     |     o     |     x     |     x     |
|strictisol1|     x     |     x     |     x     |     x     |     o     |     x     |
|strictisol2|     x     |     x     |     x     |     x     |     x     |     o     |

yassi-github · 2023-05-30T17:16:00Z

@Luap99 @flouthoc
Is there anything else left to do for the review?
I'm new to OSS so sorry if I'm doing something wrong.

Luap99 · 2023-05-30T17:29:18Z

@yassi-github No everything is good, I will try to take a look this week. I am a bit busy right now.

yassi-github · 2023-05-30T17:44:26Z

@Luap99 OK, Thank you for your time. 😃

Luap99

Overall I like the idea of this new option. Please add tests for it, see the existing isolate test: https://github.com/containers/netavark/blob/main/test/100-bridge-iptables.bats#L632

I haven't verified if the iptables rules work correctly yet but the code looks mostly good, just some small comments.

Luap99 · 2023-06-01T14:19:09Z

src/network/internal_types.rs

@@ -73,3 +73,11 @@ pub struct IPAMAddresses {
    pub net_addresses: Vec<types::NetAddress>,
    pub nameservers: Vec<IpAddr>,
 }
+
+// IsolateOption is used to select isolate option value
+#[derive(Clone, Debug, PartialEq)]


Considering that this is a very simple type you can add the Copy trait there as well.
Then you can just remove the all the references for IsolateOption and pass it by value, that should simplify the code a bit.

Luap99 · 2023-06-01T14:22:17Z

src/network/bridge.rs

-            parse_option(&self.info.network.options, OPTION_ISOLATE)?.unwrap_or(false);
+        // let isolate: String =
+        //     parse_option(&self.info.network.options, OPTION_ISOLATE)?.unwrap_or(ISOLATE_OPTION_FALSE.to_string());
+        let isolate: IsolateOption = get_isolate_option(&self.info.network.options);


We should definitely keep using parse_option() here, we only do not want to error for the teardown code path. In validate we should error out here.

Luap99 · 2023-06-01T14:26:24Z

src/network/bridge.rs

+        Err(e) => {
+            // just log we still try to do as much as possible for cleanup
+            error!("failed to parse {} option: {}", OPTION_ISOLATE, e);
+            ISOLATE_OPTION_FALSE.to_string()


you can just return IsolateOption::Never here

Luap99 · 2023-06-01T14:31:02Z

And thanks for your contribution of course.

baude · 2023-06-07T13:24:20Z

/approve
LGTM

Luap99 · 2023-06-08T12:27:22Z

@mheon PTAL, can this be done with firewalld? I would like to avoid adding firewall driver specific options that we cannot support in other backends. Once we enable other backends we have to implement there as well. The problem is end users have no idea about what driver will be used and I really do not want critical security options only work with one implementation.

Luap99

Also could you squash the commits please.

Luap99 · 2023-06-08T12:29:32Z

test/100-bridge-iptables.bats

+    expected_rc=1 run_in_container_netns 1 ping -w 1 -c 1 10.89.2.2
+    expected_rc=1 run_in_container_netns 1 ping -w 1 -c 1 10.89.3.2
+    expected_rc=1 run_in_container_netns 2 ping -w 1 -c 1 10.89.0.2
+    expected_rc=1 run_in_container_netns 2 ping -w 1 -c 1 10.89.1.2
+    expected_rc=1 run_in_container_netns 2 ping -w 1 -c 1 10.89.3.2
+    expected_rc=1 run_in_container_netns 3 ping -w 1 -c 1 10.89.0.2
+    expected_rc=1 run_in_container_netns 3 ping -w 1 -c 1 10.89.1.2
+    expected_rc=1 run_in_container_netns 3 ping -w 1 -c 1 10.89.2.2


Please add comments here what checks what, i.e from network x to y. If one thing here fails it is a nightmare to debug.

Luap99 · 2023-06-08T12:29:42Z

test/100-bridge-iptables.bats

+    expected_rc=1 run_in_container_netns 1 ping -w 1 -c 1 fd92::2
+    expected_rc=1 run_in_container_netns 1 ping -w 1 -c 1 fd93::2
+    expected_rc=1 run_in_container_netns 2 ping -w 1 -c 1 fd90::2
+    expected_rc=1 run_in_container_netns 2 ping -w 1 -c 1 fd99::2
+    expected_rc=1 run_in_container_netns 2 ping -w 1 -c 1 fd93::2
+    expected_rc=1 run_in_container_netns 3 ping -w 1 -c 1 fd90::2
+    expected_rc=1 run_in_container_netns 3 ping -w 1 -c 1 fd99::2
+    expected_rc=1 run_in_container_netns 3 ping -w 1 -c 1 fd92::2


mheon · 2023-06-08T12:55:14Z

@Luap99 Not with the present implementation. Firewalld keeps everything in the same zone right now. Theoretically we could change this to have a 1:1 network:firewalld zone relationship which would allow us to do this, but within the same zone everything is treated as trusted - so we can't block traffic.

Luap99 · 2023-06-08T14:16:12Z

@mheon What is the state of firewalld implementation then? I guess this is already a problem with the current isolate=true/false behavior? I think this needs to be supported by firewalld if we ever want to enable firewalld. But given how long we have been pushing this I have doubts that this is going to happen soon.

In any case I think this PR is good and solves a real problem and the firewalld future is something for us maintainers to figure out and should not block this PR from merging.

mheon · 2023-06-08T14:20:20Z

You are correct, firewalld does not support isolation at the moment.

Signed-off-by: yassi-github <[email protected]>

Luap99

LGTM, thanks
/lgtm

In order to create networks with podman network create --opt isolate=strict we need to patch c/common to accept that new value.
Do you want to make a PR there to enable it? If not I can do it.

ref: https://github.com/containers/common/blob/20def0054c6e1841e1b33fe4ecc45c0ab3c29e6f/libnetwork/netavark/config.go#L189-L195

openshift-ci · 2023-06-16T13:26:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude, Luap99, yassi-github

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Luap99,baude]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

yassi-github · 2023-06-16T15:38:10Z

LGTM, thanks /lgtm

Glad I could contribute!

In order to create networks with podman network create --opt isolate=strict we need to patch c/common to accept that new value. Do you want to make a PR there to enable it? If not I can do it.

ref: https://github.com/containers/common/blob/20def0054c6e1841e1b33fe4ecc45c0ab3c29e6f/libnetwork/netavark/config.go#L189-L195

I'd like to try it.

EDIT: created PR to c/common! (containers/common#1513)

yassi-github force-pushed the strict-isolation branch 2 times, most recently from 48de7d3 to 51b24d3 Compare May 29, 2023 16:05

Luap99 reviewed Jun 1, 2023

View reviewed changes

openshift-ci bot added the approved label Jun 7, 2023

Luap99 reviewed Jun 8, 2023

View reviewed changes

firewall: add NETAVARK_ISOLATION_3 chain for strict isolation

d31c157

Signed-off-by: yassi-github <[email protected]>

yassi-github force-pushed the strict-isolation branch from 9828560 to d31c157 Compare June 9, 2023 08:19

Luap99 mentioned this pull request Jun 15, 2023

Detect firewalld by default #722

Open

Luap99 approved these changes Jun 16, 2023

View reviewed changes

openshift-ci bot assigned Luap99 Jun 16, 2023

openshift-ci bot added the lgtm label Jun 16, 2023

openshift-merge-robot merged commit 36feb44 into containers:main Jun 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: isolation among all netavark bridge #703

feature: isolation among all netavark bridge #703

yassi-github commented May 29, 2023

yassi-github commented May 30, 2023

Luap99 commented May 30, 2023

yassi-github commented May 30, 2023

Luap99 left a comment

Luap99 Jun 1, 2023

Luap99 Jun 1, 2023

Luap99 Jun 1, 2023

Luap99 commented Jun 1, 2023

baude commented Jun 7, 2023

Luap99 commented Jun 8, 2023

Luap99 left a comment

Luap99 Jun 8, 2023

Luap99 Jun 8, 2023

mheon commented Jun 8, 2023

Luap99 commented Jun 8, 2023

mheon commented Jun 8, 2023

Luap99 left a comment

openshift-ci bot commented Jun 16, 2023

yassi-github commented Jun 16, 2023 •

edited

Loading

feature: isolation among all netavark bridge #703

feature: isolation among all netavark bridge #703

Conversation

yassi-github commented May 29, 2023

yassi-github commented May 30, 2023

Luap99 commented May 30, 2023

yassi-github commented May 30, 2023

Luap99 left a comment

Choose a reason for hiding this comment

Luap99 Jun 1, 2023

Choose a reason for hiding this comment

Luap99 Jun 1, 2023

Choose a reason for hiding this comment

Luap99 Jun 1, 2023

Choose a reason for hiding this comment

Luap99 commented Jun 1, 2023

baude commented Jun 7, 2023

Luap99 commented Jun 8, 2023

Luap99 left a comment

Choose a reason for hiding this comment

Luap99 Jun 8, 2023

Choose a reason for hiding this comment

Luap99 Jun 8, 2023

Choose a reason for hiding this comment

mheon commented Jun 8, 2023

Luap99 commented Jun 8, 2023

mheon commented Jun 8, 2023

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 16, 2023

yassi-github commented Jun 16, 2023 • edited Loading

yassi-github commented Jun 16, 2023 •

edited

Loading