CNI file permissions based on CIS Benchmarks

[missa-wndrvr]

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

https://github.com/projectcalico/calico/issues/7461

Todos

• [ x] Tests
• [ ] Documentation
• [x] 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]

All committers have signed the CLA.

[mazdakn]

/sem-approve

[caseydavenport]

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]

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

[missa-wndrvr]

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]

@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]

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]

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]

Please re-review

[caseydavenport]

/sem-approve

[caseydavenport]

Thanks @missa-wndrvr !

[kashifest]

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

[caseydavenport]

/sem-approve

[caseydavenport]

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]

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]

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

[marvin-tigera]

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

[caseydavenport]

/sem-approve

[caseydavenport]

Pushed a formatting fix.

[kashifest]

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

[caseydavenport]

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

[marvin-tigera]

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

[caseydavenport]

/sem-approve

[kashifest]

thanks a lot @caseydavenport for the quick response.

[missa-wndrvr]

Hello @caseydavenport, I noticed that this change was initially marked for v3.30.0 release. Now I see that it is tagged for v3.31.0. Could you please let me know why, and if it still can be part of v3.30.0?

[caseydavenport]

@missa-wndrvr from what I can tell, it's still within the v3.30 milestone. This should be released in Calico v3.30

[missa-wndrvr]

Thanks @caseydavenport for the lightning quick reply! I was unsure because I can see the commit under the tag: v3.31.0.dev, but it is in the v3.30.0 milestone. Thanks for clarifying!