CNI file permissions based on CIS Benchmarks #8991

missa-wndrvr · 2024-07-08T19:15:33Z

CNI-related CIS Benchmarks include:

1.1.9 Ensure that the Container Network Interface file
permissions are set to 600 or more restrictive.

Tests:

Running "make test" in 'cni-plugin' directory successfully
Delpoying Calico-node and Calico-Kube-controllers pods successfully
Ensure /etc/cni/net.d/10-calico.conflist file permissions is set to 600
Ensure Calico plugin is listed under '/opt/cni/bin/'

Related issues/PRs

#7461

Todos

[ x] Tests
Documentation
Release note

Release Note

Adjust CNI config file permissions to satisfy CIS benchmark expectations.

Reminder for the reviewer

Make sure that this PR has the correct labels and milestone set.

Every PR needs one docs-* label.

docs-pr-required: This change requires a change to the documentation that has not been completed yet.
docs-completed: This change has all necessary documentation completed.
docs-not-required: This change has no user-facing impact and requires no docs.

Every PR needs one release-note-* label.

release-note-required: This PR has user-facing changes. Most PRs should have this label.
release-note-not-required: This PR has no user-facing changes.

Other optional labels:

cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.
needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.

CLAassistant · 2024-07-08T19:15:38Z

All committers have signed the CLA.

mazdakn · 2024-07-16T16:47:30Z

/sem-approve

caseydavenport · 2024-07-22T16:52:53Z

Looks like a handful of tests failing in the CNI package with errors like these:

------------------------------36:09
• Failure [0.960 seconds]36:09
CNI installation tests36:09
/go/src/github.com/projectcalico/calico/cni-plugin/pkg/install/install_test.go:15836:09
  should use CNI_NETWORK_CONFIG_FILE over CNI_NETWORK_CONFIG [It]36:09
  /go/src/github.com/projectcalico/calico/cni-plugin/pkg/install/install_test.go:32136:09
 36:09
  failed to read file /tmp/cni-install-ut-4151388086/net.d/10-calico.conflist36:09
  Unexpected error:36:09
      <*fs.PathError | 0xc00059e2d0>: 36:09
      open /tmp/cni-install-ut-4151388086/net.d/10-calico.conflist: permission denied36:09
      {36:09
          Op: "open",36:09
          Path: "/tmp/cni-install-ut-4151388086/net.d/10-calico.conflist",36:09
          Err: <syscall.Errno>0xd,36:09
      }36:09
  occurred36:09
--36:13
••36:13
------------------------------36:13
• Failure [0.828 seconds]36:13
CNI installation tests36:13
/go/src/github.com/projectcalico/calico/cni-plugin/pkg/install/install_test.go:15836:13
  copying /calico-secrets36:13
  /go/src/github.com/projectcalico/calico/cni-plugin/pkg/install/install_test.go:36236:13
    Should copy a non-hidden file [It]36:13
    /go/src/github.com/projectcalico/calico/cni-plugin/pkg/install/install_test.go:37736:13
 36:13
    Unexpected error:36:13
        <*fs.PathError | 0xc000614a80>: 36:13
        open /tmp/cni-install-ut-2865909681/net.d/10-calico.conflist: permission denied36:13
        {36:13
            Op: "open",36:13
            Path: "/tmp/cni-install-ut-2865909681/net.d/10-calico.conflist",36:13
            Err: <syscall.Errno>0xd,

steven-webster · 2024-07-24T22:20:49Z

Just a small FYI missa-wndrvr will be away until Aug. 6 2024 but will address any comments upon his return.

missa-wndrvr · 2024-08-09T16:30:10Z

I can see the 5 test cases related to reading "10-calico.conflist" run by the function "runCniContainer" failing. Could I get some feedback or assistance on this?

Is there a way to give permissions to that function to be able to read/write as the owner of the file since it's a temp file created for testing purposes?

Also, for my own knowledge, why does it not have the necessary permissions to read the file since it was created in the test run? Does this unit test failure signify that it might affect other people's deployments or is it only related to how the test case was written?

caseydavenport · 2024-08-19T17:54:19Z

@missa-wndrvr sorry for the delay.

My understanding is that the tests are running the calico-cni container, which writes a 10-calico.conflist file with the new permissions.

It then attempts to open that file to check its contents are expected, but it unable to do so because the test container isn't running with the right user / group to read the file.

Suspect that this is probably mostly a test harness artifact, although in theory in real clusters other agents on the node (i.e., container runtimes) do need to read this file so will need to have permissions to do so.

missa-wndrvr · 2024-08-20T18:14:57Z

Thanks for the reply @caseydavenport. I was able to deploy the Calico pods with the permission change on my system which was successful in which any resource that needed access to the file was able to get it (if they had the necessary permissions)

How else would you suggest I test this to ensure it doesn't affect other agents in real clusters, so that we can verify that this might just be a test harness conflict?

caseydavenport · 2024-08-23T18:57:00Z

How else would you suggest I test this to ensure it doesn't affect other agents in real clusters

I don't think we can guarantee this, so will have to live with the risk (or keep the permissions as they are). To mitigate this, the thing to do is probably to look at the most popular container runtimes and kuberentes cluster installers to see how the runtimes are configured and if they will by default have permissions to access the CNI config file. I am not too worried about this aspect.

We do need to get the tests passing, though. For that we probably need to adjust the group that the tests are running under such that the test code can read the generated configuration file.

missa-wndrvr · 2024-09-09T18:22:11Z

Please re-review

caseydavenport · 2024-09-10T15:31:58Z

/sem-approve

caseydavenport · 2024-09-11T16:47:13Z

cni-plugin/pkg/install/install_test.go

@@ -268,7 +268,7 @@ PuB/TL+u2y+iQUyXxLy3
 		})

 		It("Should parse and output a templated config", func() {
-			err := runCniContainer(tempDir, true)
+			err := runCniContainer(tempDir, true, "-u", "0")


We're still seeing the same permissions error it seems.

I'm not sure that setting -u 0 is right.

The problem is that the install test runs as a binary on the host:

calico/cni-plugin/Makefile

Lines 247 to 257 in f543e75

# We pre-build the test binary so that we can run it outside a container and allow it

# to interact with docker.

pkg/install/install.test: pkg/install/*.go

$(DOCKER_RUN) $(CALICO_BUILD) sh -c '$(GIT_CONFIG_SSH) \

cd /go/src/$(PACKAGE_NAME) && \

go test ./pkg/install -c --tags install_test -o ./pkg/install/install.test'

.PHONY: test-install-cni

## Test the install

test-install-cni: run-k8s-apiserver image pkg/install/install.test

cd pkg/install && CONTAINER_NAME=$(CNI_PLUGIN_IMAGE):latest-$(ARCH) CERTS_PATH=$(CERTS_PATH) ./install.test

So, expectFileContents isn't being run as a privileged user (which is desirable). We may need to consider other changes for this.

Perhaps we need to make the permissions used by install.go configurable, so the test code can set it to something more permissive (but defaults to 600 for production clusters).

caseydavenport · 2024-09-20T17:07:48Z

Thanks @missa-wndrvr !

kashifest · 2024-11-22T08:26:38Z

@missa-wndrvr @caseydavenport can we take this PR in? We are also waiting for this patch

caseydavenport · 2024-11-22T16:07:26Z

/sem-approve

caseydavenport · 2024-11-22T16:25:53Z

Looks like the PR is failing code formatting checks - running make fix-all should clean it up.

@@ -306,8 +306,8 @@ func isValidJSON(s string) error {02:28
 02:28
 // Convert permission string to fileMode for testing purposes02:28
 func stringToFileMode(permString string) (os.FileMode, error) {02:28
-        perm, err := strconv.ParseUint(permString, 8, 32)02:28
-        return os.FileMode(perm), err02:28
+	perm, err := strconv.ParseUint(permString, 8, 32)02:28
+	return os.FileMode(perm), err02:28
 }02:28
 02:28
 func writeCNIConfig(c config) {02:28
@@ -390,9 +390,9 @@ func writeCNIConfig(c config) {02:28
 	permString := getEnv("TEST_FILE_PERMISSION", "0600")02:28
 	logrus.Infof("CNI config file permission is set to %s\n", permString)02:28
 	perm, err := stringToFileMode(permString)02:28
-        if err != nil {02:28
-                logrus.Fatal(err)02:28
-        }02:28
+	if err != nil {02:28
+		logrus.Fatal(err)02:28
+	}02:28
 02:28
 	name := getEnv("CNI_CONF_NAME", "10-calico.conflist")02:28
 	path := winutils.GetHostPath(fmt.Sprintf("/host/etc/cni/net.d/%s", name))

kashifest · 2024-11-25T07:35:19Z

Thanks @caseydavenport for the quick response. @missa-wndrvr would you please push the change or if you need help I can do it as well?

missa-wndrvr · 2024-11-26T16:56:50Z

Hello @kashifest, sorry I got caught up with other things. Please go ahead, and thank you!

marvin-tigera · 2024-11-26T18:14:16Z

Removing "merge-when-ready" label due to new commits

caseydavenport · 2024-11-26T18:14:28Z

/sem-approve

caseydavenport · 2024-11-26T18:14:34Z

Pushed a formatting fix.

kashifest · 2024-11-27T08:45:13Z

@caseydavenport the PR now shows that is has 214 commits ?

caseydavenport · 2024-11-27T17:59:09Z

@kashifest oh shoot. I must have messed up the merge! I'll fix it 😆

CNI-related CIS Benchmarks include: 1.1.9 Ensure that the Container Network Interface file permissions are set to 600 or more restrictive. Tests: - Running "make test" in cni-plugin directory successfully - Delpoying Calico-node and Calico-Kube-controllers pods successfully - Ensure /etc/cni/net.d/10-calico.conflist file permissions is set to 600

Testing if running the failed tasks as root would give permission to read the CNI config file.

Allow the configuration of the CNI config file's permission mode. This will satisfy the CNI file permissions set by CIS Benchmarks. Also, allowing the unit test code to read the config file with no restriction.

marvin-tigera · 2024-11-27T18:01:04Z

Removing "merge-when-ready" label due to new commits

caseydavenport · 2024-11-27T18:01:17Z

/sem-approve

kashifest · 2024-11-28T11:36:41Z

thanks a lot @caseydavenport for the quick response.

* CNI file permissions based on CIS Benchmarks CNI-related CIS Benchmarks include: 1.1.9 Ensure that the Container Network Interface file permissions are set to 600 or more restrictive. Tests: - Running "make test" in cni-plugin directory successfully - Delpoying Calico-node and Calico-Kube-controllers pods successfully - Ensure /etc/cni/net.d/10-calico.conflist file permissions is set to 600 * CNI file permissions based on CIS Benchmarks Testing if running the failed tasks as root would give permission to read the CNI config file. * Configurable CNI config file permission Allow the configuration of the CNI config file's permission mode. This will satisfy the CNI file permissions set by CIS Benchmarks. Also, allowing the unit test code to read the config file with no restriction. * Fix formatting --------- Co-authored-by: Casey Davenport <[email protected]>

missa-wndrvr requested a review from a team as a code owner July 8, 2024 19:15

marvin-tigera added this to the Calico v3.29.0 milestone Jul 8, 2024

marvin-tigera added release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels Jul 8, 2024

caseydavenport reviewed Sep 11, 2024

View reviewed changes

danudey modified the milestones: Calico v3.29.0, Calico v3.30.0 Sep 18, 2024

missa-wndrvr requested a review from caseydavenport September 20, 2024 16:17

caseydavenport approved these changes Sep 20, 2024

View reviewed changes

caseydavenport added merge-when-ready squash-commits labels Sep 20, 2024

marvin-tigera removed merge-when-ready squash-commits labels Nov 26, 2024

caseydavenport added merge-when-ready squash-commits labels Nov 26, 2024

missa-wndrvr and others added 4 commits November 27, 2024 10:00

CNI file permissions based on CIS Benchmarks

f59987b

Testing if running the failed tasks as root would give permission to read the CNI config file.

Configurable CNI config file permission

318b171

Allow the configuration of the CNI config file's permission mode. This will satisfy the CNI file permissions set by CIS Benchmarks. Also, allowing the unit test code to read the config file with no restriction.

Fix formatting

bbd2919

caseydavenport force-pushed the calico-config-file-permission branch from 7deac09 to bbd2919 Compare November 27, 2024 18:00

marvin-tigera removed merge-when-ready squash-commits labels Nov 27, 2024

caseydavenport added merge-when-ready squash-commits labels Nov 27, 2024

marvin-tigera merged commit 29533a3 into projectcalico:master Nov 27, 2024
7 checks passed

kashifest mentioned this pull request Dec 19, 2024

CNI File permission on existing files #9619

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CNI file permissions based on CIS Benchmarks #8991

CNI file permissions based on CIS Benchmarks #8991

missa-wndrvr commented Jul 8, 2024 •

edited

Loading

CLAassistant commented Jul 8, 2024 •

edited

Loading

mazdakn commented Jul 16, 2024

caseydavenport commented Jul 22, 2024

steven-webster commented Jul 24, 2024

missa-wndrvr commented Aug 9, 2024

caseydavenport commented Aug 19, 2024

missa-wndrvr commented Aug 20, 2024

caseydavenport commented Aug 23, 2024

missa-wndrvr commented Sep 9, 2024

caseydavenport commented Sep 10, 2024

caseydavenport Sep 11, 2024

caseydavenport commented Sep 20, 2024

kashifest commented Nov 22, 2024

caseydavenport commented Nov 22, 2024

caseydavenport commented Nov 22, 2024

kashifest commented Nov 25, 2024

missa-wndrvr commented Nov 26, 2024

marvin-tigera commented Nov 26, 2024

caseydavenport commented Nov 26, 2024

caseydavenport commented Nov 26, 2024

kashifest commented Nov 27, 2024

caseydavenport commented Nov 27, 2024

marvin-tigera commented Nov 27, 2024

caseydavenport commented Nov 27, 2024

kashifest commented Nov 28, 2024

	# We pre-build the test binary so that we can run it outside a container and allow it
	# to interact with docker.
	pkg/install/install.test: pkg/install/*.go
	$(DOCKER_RUN) $(CALICO_BUILD) sh -c '$(GIT_CONFIG_SSH) \
	cd /go/src/$(PACKAGE_NAME) && \
	go test ./pkg/install -c --tags install_test -o ./pkg/install/install.test'

	.PHONY: test-install-cni
	## Test the install
	test-install-cni: run-k8s-apiserver image pkg/install/install.test
	cd pkg/install && CONTAINER_NAME=$(CNI_PLUGIN_IMAGE):latest-$(ARCH) CERTS_PATH=$(CERTS_PATH) ./install.test

CNI file permissions based on CIS Benchmarks #8991

CNI file permissions based on CIS Benchmarks #8991

Conversation

missa-wndrvr commented Jul 8, 2024 • edited Loading

Related issues/PRs

Todos

Release Note

Reminder for the reviewer

CLAassistant commented Jul 8, 2024 • edited Loading

mazdakn commented Jul 16, 2024

caseydavenport commented Jul 22, 2024

steven-webster commented Jul 24, 2024

missa-wndrvr commented Aug 9, 2024

caseydavenport commented Aug 19, 2024

missa-wndrvr commented Aug 20, 2024

caseydavenport commented Aug 23, 2024

missa-wndrvr commented Sep 9, 2024

caseydavenport commented Sep 10, 2024

caseydavenport Sep 11, 2024

Choose a reason for hiding this comment

caseydavenport commented Sep 20, 2024

kashifest commented Nov 22, 2024

caseydavenport commented Nov 22, 2024

caseydavenport commented Nov 22, 2024

kashifest commented Nov 25, 2024

missa-wndrvr commented Nov 26, 2024

marvin-tigera commented Nov 26, 2024

caseydavenport commented Nov 26, 2024

caseydavenport commented Nov 26, 2024

kashifest commented Nov 27, 2024

caseydavenport commented Nov 27, 2024

marvin-tigera commented Nov 27, 2024

caseydavenport commented Nov 27, 2024

kashifest commented Nov 28, 2024

missa-wndrvr commented Jul 8, 2024 •

edited

Loading

CLAassistant commented Jul 8, 2024 •

edited

Loading