Skip NATS firewall creation when cgroupv2 enabled

[ramonskie]

this will add the following nftables rules
when cgroupv2 are enabled on a linux system

socket cgroupv2 level 2 "system.slice/bosh-agent.service" ip daddr 1.2.3.4 tcp dport 1234 log prefix "Matched cgroup bosh-agent nats rule: " accept
meta skuid 0 ip daddr 1.2.3.4tcp dport 1234 log prefix "Matched skuid director nats rule: " accept
ip daddr 1.2.3.4 tcp dport 1234 log prefix "dropped nats rule: " drop

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: ramonskie / name: Ramon Makkelie (95239be, e6eb872, 6270d93, adfe22b, 65156af)
• ✅ login: jpalermo / name: Joseph Palermo (6553c7a, 6448a75)

[ramonskie]

currenly the test will not work as we don't test it on a stemcell.
as this test assumes it has a bosh-agent and systemd available
so the test should be improved. any suggestions?

[aramprice]

For testing, if we are relying on the CFF Concourse instances, we may need to wait for a cgroups v2 stemcell to become available and for Concourse to be able to consume it for the cgroup part. Unsure about systemd and whether that's present in a Concourse container.

[jpalermo]

We talked in the FIWG meeting about having the tests conditional on the cgroups currently enabled. So for now, in the bosh-agent pipelines, the cgroupsv2 code path wouldn't actually get tested. But as soon as we start shipping Noble and add a job for the pipeline, the cgroupsv2 tests would be active when running against Noble.

[rkoster]

@ramonskie did you have a chance to take a look at ^

[rkoster]

Once CI is green again @ramonskie is gonna look at this (as discussed during working group meeting).

[jpalermo]

Hey @ramonskie and @rkoster, some of the new tests are still failing because they're trying to call iptables during the SetupNatsFirewall(test.mbus) call in the tests.

Since the unit tests are running on jammy, they're not using the nftables branch.

We'd probably need to either delete that test, or inject enough dependencies in the unit tests to prevent them from actually doing anything.

But larger question, can we just delete all this code?

The firewall rules are designed to prevent workloads from talking to NATs because the NATs creds used to be accessible from the IaaS metadata endpoints. But we've switched to using temporary NATs creds, so that's no longer an attack vector.

This does provide an additional layer of security, but it's a LOT of complexity to maintain for a "second fence".

We should discuss at the FIWG meeting tomorrow.

[beyhan]

The decision was made to not implement this logic in Nobel because it doesn't bring more value from security perspective.

[jpalermo]

Updated the PR to skip firewall creation when using cgroups v2. The commits adding nftables support are still there in case we want them as a reference in the future.

[jpalermo]

@beyhan, I added you as a reviewer because you said you were going to check if you all had any security concerns with this.

[jpalermo]

@a-hassanin , just assigning you to check if you have any security concerns about this.

[aramprice]

The following patch should resolve the linter error:

diff --git a/platform/net/firewall_provider_linux.go b/platform/net/firewall_provider_linux.go
index 306d8938..9298d665 100644
--- a/platform/net/firewall_provider_linux.go
+++ b/platform/net/firewall_provider_linux.go
@@ -6,17 +6,15 @@ package net
 import (
 	"errors"
 	"fmt"
-	bosherr "github.com/cloudfoundry/bosh-utils/errors"
 	"net"
 	gonetURL "net/url"
 	"os"
 	"strings"
-	// NOTE: "cgroups is only intended to be used/compiled on linux based system"
-	// see: https://github.com/containerd/cgroups/issues/19
-	"github.com/containerd/cgroups"
-	"github.com/opencontainers/runtime-spec/specs-go"
 
+	bosherr "github.com/cloudfoundry/bosh-utils/errors"
+	"github.com/containerd/cgroups" // NOTE: linux only; see: https://github.com/containerd/cgroups/issues/19
 	"github.com/coreos/go-iptables/iptables"
+	"github.com/opencontainers/runtime-spec/specs-go"
 )
 
 const (

[rkoster]

@beyhan any news on the security front?

[beyhan]

I'm waiting for feedback here.

[a-hassanin]

Mentioning the feature PR cloudfoundry/bosh#2417 as it is relevant for the decision here.

[a-hassanin]

It looks Ok from our side we can merge it. We will need to revisit this from SAP side when we want to use Noble bec. as I saw we need a PoC to make sure it will not pose a security risk on AliCloud. But this we will plan later.