bridge: Add macspoofchk support #639

EdDev · 2021-06-24T13:42:48Z

The new macspoofchk field is added to the bridge plugin to support
anti-mac-spoofing.
When the parameter is enabled, traffic is limited to the mac addresses
of the container interface (the veth peer that is placed in the
container ns).

The implementation is using nftables and should only be used on nodes
that support it.

Notes:

~~This change is under e2e and functional testing, confirming it works as expected beyond the existing integration tests.~~
~~The go-nft used in this PR is not a tagged version. Post testing and documentation a tag will be added and the dependency updated.~~

EdDev · 2021-06-28T12:28:36Z

Verification: The change has been tested on kubevirt and confirmed to function properly (nftables config added, mac correctly filtered and configuration removed from node after pod termination).

dcbw · 2021-06-30T15:57:43Z

@EdDev can you rebase on top of your container-veth static MAC now that it's merged?

EdDev · 2021-06-30T16:27:07Z

@EdDev can you rebase on top of your container-veth static MAC now that it's merged?

Done

qinqon

First pass very basic questions more or less

pkg/link/spoofcheck.go

EdDev · 2021-07-20T14:11:46Z

change: Answered review comment about using the go-nft library rule lookup.

qinqon · 2021-07-21T06:47:39Z

@EdDev can you split the PR into two commits one with deps bump and the other with the changes ?

EdDev · 2021-07-21T07:18:20Z

@EdDev can you split the PR into two commits one with deps bump and the other with the changes ?

I am unsure if it is acceptable by the maintainers here, so I prefer to wait with that for their input.
The added value in splitting like that is ease of review, but it comes with the cost that the commit is not self contained.

AlonaKaplan

Partial review, I didn't review the tests yet.

pkg/link/spoofcheck.go

plugins/main/bridge/bridge.go

AlonaKaplan

Couldn't find any e2e tests for the change. Did I miss something?

plugins/main/bridge/bridge_test.go

EdDev

Couldn't find any e2e tests for the change. Did I miss something?

AFAIK there are no functional tests which check that the configuration on the system is actually affecting the traffic. The tests here are just checking that the configuration is reaching the system/kernel.

Nevertheless, I have checked this as stated here.
Adding e2e tests to kubevirt should cover this.

plugins/main/bridge/bridge_test.go

EdDev · 2021-07-26T20:37:29Z

Change: answered a review comment.

qinqon · 2021-07-27T06:45:11Z

/lgtm

AlonaKaplan · 2021-07-27T07:20:24Z

/lgtm

As an intermediate and temporary step, consume the changes posted to the CNI bridge [1]. Once the PR is merged into the original repo, this change will be reverted. In case the PR will not be merged into the original repo, we will proceed with a more formal resolution. [1] containernetworking/plugins#639 Signed-off-by: Edward Haas <edwardh@redhat.com>

* components: Consume CNI Bridge with macspoofchk support As an intermediate and temporary step, consume the changes posted to the CNI bridge [1]. Once the PR is merged into the original repo, this change will be reverted. In case the PR will not be merged into the original repo, we will proceed with a more formal resolution. [1] containernetworking/plugins#639 Signed-off-by: Edward Haas <edwardh@redhat.com> * components: Build linux-bridge CNI from new sources This patch builds a new image from the newly updated components.yaml. Signed-off-by: Petr Horáček <phoracek@redhat.com> Co-authored-by: Petr Horáček <phoracek@redhat.com>

EdDev · 2021-08-25T06:33:59Z

change: Rebase

pkg/link/spoofcheck.go

squeed · 2021-09-01T19:40:31Z

This looks good to me. I have a minor bit of bikeshedding that, honestly, can be ignored.

Can you also file a PR updating the bridge documentation: https://github.com/containernetworking/cni.dev/blob/main/content/plugins/current/main/bridge.md

Thanks!

plugins/meta/portmap/portmap.go

mccv1r0 · 2021-09-08T15:56:45Z

This should also support MAC to IPv6 addresses as well

plugins/main/bridge/bridge.go

squeed · 2021-09-08T16:14:27Z

hey @EdDev, just to make explicit, what security is this providing? In other words, what is enforced?

Port A can only source traffic from mac A
Port A can only receive traffic to mac A
Mac A can only source traffic from IPs A...
Mac A can only receive traffic to IPs A...

Separately, should we add an explicit rule for the gateway?

EdDev · 2021-09-13T11:03:59Z

Change: Bumped go-nft to v0.2.0 to overcome the SELinux issue detected on some platforms.

The new macspoofchk field is added to the bridge plugin to support anti-mac-spoofing. When the parameter is enabled, traffic is limited to the mac addresses of the container interface (the veth peer that is placed in the container ns). Any traffic that exits the pod is checked against the source mac address that is expected. If the mac address is different, the frames are dropped. The implementation is using nftables and should only be used on nodes that support it. Signed-off-by: Edward Haas <edwardh@redhat.com>

EdDev · 2021-09-14T09:59:35Z

hey @EdDev, just to make explicit, what security is this providing? In other words, what is enforced?
* Port A can only source traffic from mac A

* Port A can only receive traffic to mac A

* Mac A can only source traffic from IPs A...

* Mac A can only receive traffic to IPs A...

I tried to add this to the commit message and the PR description.
mac-spoof is bout L2 only, it filters traffic based on the "approved" source mac address on pod egress traffic.

Therefore, any traffic that exists the pod (on the specific interface), is checked against the source mac address.
It is not looking at higher layers (e.g. IP & TCP protocols).
Furthermore, ingress traffic is not examined, so multicast, broadcast and potential DoS attacks are not checked or handled.

Separately, should we add an explicit rule for the gateway?

The rule is on the veth peer at the host side. It is supposed to handle any L2 traffic from that veth interface, so I do not think we need a special handling if the destination mac address is the gateway.
Do you see a special issue here?

EdDev · 2021-09-14T10:02:30Z

This should also support MAC to IPv6 addresses as well

mac-spook-check is not handing L3 spoofing (e.g. IP), so I think we are good.

Ref: containernetworking/plugins#639 Signed-off-by: Edward Haas <edwardh@redhat.com>

EdDev · 2021-09-14T10:30:16Z

This looks good to me. I have a minor bit of bikeshedding that, honestly, can be ignored.

Can you also file a PR updating the bridge documentation: https://github.com/containernetworking/cni.dev/blob/main/content/plugins/current/main/bridge.md

Thanks!

Done

matthewdupre

Code looks sensible but I haven't gone through the tests

dcbw · 2021-09-29T15:59:11Z

@EdDev looks good in general, could you resolve the PR comments that you feel have been addressed?

EdDev · 2021-09-30T07:38:16Z

@EdDev looks good in general, could you resolve the PR comments that you feel have been addressed?

@dcbw I think I covered all comments. Went over the "opened" ones and marked them as resolved.

mccv1r0 · 2021-10-06T15:27:16Z

I think this is ready to go
/lgtm

As containernetworking/plugins#639 has been merged to the plugins repo, revert back to track it instead of the temporary repo which included the macspoofchk feature. Signed-off-by: Edward Haas <edwardh@redhat.com>

* components: Track the formal CNI plugins repo As containernetworking/plugins#639 has been merged to the plugins repo, revert back to track it instead of the temporary repo which included the macspoofchk feature. Signed-off-by: Edward Haas <edwardh@redhat.com> * components: Rebuild linux bridge image Signed-off-by: Petr Horáček <phoracek@redhat.com> Co-authored-by: Petr Horáček <phoracek@redhat.com>

EdDev force-pushed the bridge-macspoofchk branch 5 times, most recently from b9a0c7e to 000a8b4 Compare June 28, 2021 12:11

EdDev changed the title ~~[Under Testing] bridge: Add macspoofchk support~~ bridge: Add macspoofchk support Jun 28, 2021

EdDev force-pushed the bridge-macspoofchk branch 2 times, most recently from 684e240 to fbb5622 Compare June 29, 2021 19:06

EdDev force-pushed the bridge-macspoofchk branch from fbb5622 to 0cd73fe Compare June 30, 2021 16:23

qinqon suggested changes Jul 20, 2021

View reviewed changes

pkg/link/spoofcheck.go Show resolved Hide resolved

pkg/link/spoofcheck.go Show resolved Hide resolved

pkg/link/spoofcheck.go Outdated Show resolved Hide resolved

pkg/link/spoofcheck.go Show resolved Hide resolved

EdDev force-pushed the bridge-macspoofchk branch from 0cd73fe to f6bfdc0 Compare July 20, 2021 14:10

AlonaKaplan reviewed Jul 25, 2021

View reviewed changes

pkg/link/spoofcheck.go Show resolved Hide resolved

pkg/link/spoofcheck.go Show resolved Hide resolved

pkg/link/spoofcheck.go Show resolved Hide resolved

pkg/link/spoofcheck.go Show resolved Hide resolved

plugins/main/bridge/bridge.go Show resolved Hide resolved

AlonaKaplan reviewed Jul 26, 2021

View reviewed changes

plugins/main/bridge/bridge_test.go Show resolved Hide resolved

EdDev commented Jul 26, 2021

View reviewed changes

plugins/main/bridge/bridge_test.go Show resolved Hide resolved

EdDev force-pushed the bridge-macspoofchk branch from f6bfdc0 to bf36e4c Compare July 26, 2021 20:32

AlonaKaplan approved these changes Jul 27, 2021

View reviewed changes

qinqon approved these changes Jul 27, 2021

View reviewed changes

EdDev mentioned this pull request Jul 27, 2021

components: Consume CNI Bridge with macspoofchk support kubevirt/cluster-network-addons-operator#945

Merged

EdDev force-pushed the bridge-macspoofchk branch from bf36e4c to cbec9bf Compare August 25, 2021 06:14

squeed reviewed Sep 1, 2021

View reviewed changes

pkg/link/spoofcheck.go Outdated Show resolved Hide resolved

matthewdupre reviewed Sep 8, 2021

View reviewed changes

plugins/meta/portmap/portmap.go Outdated Show resolved Hide resolved

matthewdupre reviewed Sep 8, 2021

View reviewed changes

plugins/main/bridge/bridge.go Show resolved Hide resolved

EdDev force-pushed the bridge-macspoofchk branch 2 times, most recently from f2ada05 to aadcf9a Compare September 13, 2021 10:50

EdDev force-pushed the bridge-macspoofchk branch from aadcf9a to 081ed44 Compare September 14, 2021 09:50

EdDev added a commit to EdDev/cni.dev that referenced this pull request Sep 14, 2021

bridge plugin: Document the macspoofchk field argument

07a42a7

Ref: containernetworking/plugins#639 Signed-off-by: Edward Haas <edwardh@redhat.com>

EdDev mentioned this pull request Sep 14, 2021

bridge plugin: Document the macspoofchk field argument containernetworking/cni.dev#90

Merged

matthewdupre approved these changes Sep 29, 2021

View reviewed changes

mccv1r0 approved these changes Oct 6, 2021

View reviewed changes

matthewdupre merged commit f1f128e into containernetworking:master Oct 6, 2021

EdDev deleted the bridge-macspoofchk branch October 10, 2021 06:48

EdDev mentioned this pull request Oct 12, 2021

components: Track the formal CNI plugins repo kubevirt/cluster-network-addons-operator#1052

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bridge: Add macspoofchk support #639

bridge: Add macspoofchk support #639

EdDev commented Jun 24, 2021 •

edited

Loading

EdDev commented Jun 28, 2021

dcbw commented Jun 30, 2021

EdDev commented Jun 30, 2021

qinqon left a comment

EdDev commented Jul 20, 2021 •

edited

Loading

qinqon commented Jul 21, 2021

EdDev commented Jul 21, 2021

AlonaKaplan left a comment

AlonaKaplan left a comment

EdDev left a comment

EdDev commented Jul 26, 2021

qinqon commented Jul 27, 2021

AlonaKaplan commented Jul 27, 2021

EdDev commented Aug 25, 2021

squeed commented Sep 1, 2021

mccv1r0 commented Sep 8, 2021

squeed commented Sep 8, 2021

EdDev commented Sep 13, 2021

EdDev commented Sep 14, 2021

EdDev commented Sep 14, 2021

EdDev commented Sep 14, 2021

matthewdupre left a comment

dcbw commented Sep 29, 2021

EdDev commented Sep 30, 2021

mccv1r0 commented Oct 6, 2021

bridge: Add macspoofchk support #639

bridge: Add macspoofchk support #639

Conversation

EdDev commented Jun 24, 2021 • edited Loading

EdDev commented Jun 28, 2021

dcbw commented Jun 30, 2021

EdDev commented Jun 30, 2021

qinqon left a comment

Choose a reason for hiding this comment

EdDev commented Jul 20, 2021 • edited Loading

qinqon commented Jul 21, 2021

EdDev commented Jul 21, 2021

AlonaKaplan left a comment

Choose a reason for hiding this comment

AlonaKaplan left a comment

Choose a reason for hiding this comment

EdDev left a comment

Choose a reason for hiding this comment

EdDev commented Jul 26, 2021

qinqon commented Jul 27, 2021

AlonaKaplan commented Jul 27, 2021

EdDev commented Aug 25, 2021

squeed commented Sep 1, 2021

mccv1r0 commented Sep 8, 2021

squeed commented Sep 8, 2021

EdDev commented Sep 13, 2021

EdDev commented Sep 14, 2021

EdDev commented Sep 14, 2021

EdDev commented Sep 14, 2021

matthewdupre left a comment

Choose a reason for hiding this comment

dcbw commented Sep 29, 2021

EdDev commented Sep 30, 2021

mccv1r0 commented Oct 6, 2021

EdDev commented Jun 24, 2021 •

edited

Loading

EdDev commented Jul 20, 2021 •

edited

Loading