Repository navigation
d/s merge, fix security in d/s #343
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,14 +26,14 @@ import ( | |
| "os" | ||
| "os/signal" | ||
| "os/user" | ||
| "sync" | ||
| "syscall" | ||
| "time" | ||
|
|
||
| "golang.org/x/net/netutil" | ||
| utilwait "k8s.io/apimachinery/pkg/util/wait" | ||
|
|
||
| "gopkg.in/k8snetworkplumbingwg/multus-cni.v4/pkg/cmdutils" | ||
| "gopkg.in/k8snetworkplumbingwg/multus-cni.v4/pkg/logging" | ||
| "gopkg.in/k8snetworkplumbingwg/multus-cni.v4/pkg/multus" | ||
| srv "gopkg.in/k8snetworkplumbingwg/multus-cni.v4/pkg/server" | ||
|
|
@@ -209,34 +209,44 @@ func startMultusDaemon(ctx context.Context, daemonConfig *srv.ControllerNetConf, | |
| } | ||
|
|
||
| func cniServerConfig(configFilePath string) (*srv.ControllerNetConf, error) { | ||
| configPath, err := cmdutils.NewRootedFile(configFilePath) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("illegal path in server config path %s: %w", configFilePath, err) | ||
| } | ||
| defer configPath.Close() | ||
|
|
||
| configFileContents, err := configPath.Root.ReadFile(configPath.FileName) | ||
|
Contributor
There was a problem hiding this comment. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "Declared Go versions:"
for file in go.mod go.work .go-version .tool-versions; do
if [[ -f "$file" ]]; then
echo "--- $file"
rg -n '(^go |^toolchain |go1\.[0-9]+|GO_VERSION|golang:)' "$file" || true
fi
done
echo "Go versions in build and CI files:"
fd -HI -t f -g 'Dockerfile*' -g 'Containerfile*' -g '*.yml' -g '*.yaml' . \
| xargs -r rg -n '(golang:|go1\.[0-9]+|GO_VERSION)' || true
echo "os.Root API call sites:"
rg -n --glob '*.go' '\bos\.OpenRoot\b|\*os\.Root\b|\.Root\.ReadFile\b' pkg cmdRepository: openshift/multus-cni Length of output: 1081 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "--- go.mod"
sed -n '1,35p' go.mod
echo "--- build image files"
for file in images/Dockerfile images/Dockerfile.debug images/Dockerfile.thick; do
if [[ -f "$file" ]]; then
echo "### $file"
cat -n "$file"
fi
done
echo "--- tracked build and CI references"
git ls-files | rg '(^|/)(Dockerfile[^/]*|Containerfile[^/]*|.*\.(yml|yaml|mk|sh)$|Makefile[^/]*)$' \
| xargs -r rg -n 'go1\.25|go1\.26|golang:|docker build|podman build|GO_VERSION|GOTOOLCHAIN|go build|make' || trueRepository: openshift/multus-cni Length of output: 5689 🌐 Web query:
💡 Result: In Go, automatic toolchain switching is a mechanism that allows the 'go' command to automatically detect and use a newer version of the Go toolchain than the one currently running, if a project's configuration requires it [1][2]. This behavior is primarily controlled by the 'GOTOOLCHAIN' environment variable and the directives in 'go.mod' or 'go.work' files [1][3]. GOTOOLCHAIN Configuration The 'GOTOOLCHAIN' environment variable determines how the 'go' command handles toolchain selection [1]. It can be set globally (e.g., via 'go env -w GOTOOLCHAIN=...') or per-command [1][3]. Common settings include: - auto (default): Allows the 'go' command to automatically download and switch to a newer toolchain if the 'go.mod' or 'go.work' file requires a version newer than the current one [1][2]. - local: Disables automatic downloads; the 'go' command will only use the installed (local) toolchain [2]. If the local toolchain is older than the required version, the command will fail [1][2]. - [version]+auto: Begins with a specific default toolchain version but still permits automatic upgrades to newer versions if required [1][4]. 'go.mod' Directives The 'go.mod' file supports two directives that influence this selection [3][5]: - go [version]: Specifies the minimum Go language version required for the module [5][6]. - toolchain [version]: Explicitly suggests a specific Go toolchain version to use [5][6]. This takes effect when the project is the main module and the current toolchain is older than the suggested one [5]. - toolchain default: A special value that disables automatic toolchain switching for the module, forcing the use of the default toolchain [5][7]. Starting in Go 1.25, the distribution includes fewer prebuilt tool binaries, and tools not invoked directly by build or test operations are managed via 'go tool' as needed [8]. In later versions like Go 1.26, internal package improvements—such as 'NewSwitcher' in 'cmd/go/internal/toolchain'—further refined the logic for switching to newer toolchains to resolve version requirements [9][10]. To check the current configuration or modify it, you can use: go env GOTOOLCHAIN go env -w GOTOOLCHAIN=auto Users should generally prefer 'go get go@version' or 'go get toolchain@version' to update these directives, rather than manual editing [11]. Citations:
Use Go 1.26 for the build images. The 📍 Affects 3 files
🤖 Prompt for AI Agents |
||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read server config %s: %w", configPath.Path(), err) | ||
| } | ||
| return srv.LoadDaemonNetConf(configFileContents) | ||
| } | ||
|
|
||
| func copyUserProvidedConfig(multusConfigPath string, cniConfigDir string) error { | ||
| srcPath, err := cmdutils.NewRootedFile(multusConfigPath) | ||
| if err != nil { | ||
| return fmt.Errorf("illegal path in multusConfigPath %s: %w", multusConfigPath, err) | ||
| } | ||
| defer srcPath.Close() | ||
|
|
||
| srcFile, err := srcPath.Root.Open(srcPath.FileName) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to open (READ only) file %s: %w", srcPath.Path(), err) | ||
| } | ||
| defer srcFile.Close() | ||
|
|
||
| dstPath, err := cmdutils.NewRootedFileInDir(cniConfigDir, srcPath.FileName) | ||
| if err != nil { | ||
| return fmt.Errorf("illegal destination path in cniConfigDir %s: %w", cniConfigDir, err) | ||
| } | ||
| defer dstPath.Close() | ||
|
|
||
| dstFile, err := dstPath.Root.Create(dstPath.FileName) | ||
| if err != nil { | ||
| return fmt.Errorf("creating copying file %s: %w", dstPath.Path(), err) | ||
| } | ||
| defer dstFile.Close() | ||
|
Comment on lines
+256
to
+260
Contributor
There was a problem hiding this comment. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n -C 2 'defer .*\.Close\(\)' \
cmd/multus-daemon/main.go \
cmd/kubeconfig_generator/main.goRepository: openshift/multus-cni Length of output: 2801 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- cmd/multus-daemon/main.go ---'
sed -n '205,285p' cmd/multus-daemon/main.go
printf '%s\n' '--- cmd/kubeconfig_generator/main.go ---'
sed -n '55,105p' cmd/kubeconfig_generator/main.go
printf '%s\n' '--- cmdutils rooted-path definitions and close contract ---'
rg -n -C 4 'type Rooted|func .*NewRooted|OpenRoot|func .*Close|os\.Root' --glob '*.go' .Repository: openshift/multus-cni Length of output: 9348 Handle deferred If 📍 Affects 2 files
🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
| nBytes, err := io.Copy(dstFile, srcFile) | ||
| if err != nil { | ||
| return fmt.Errorf("error copying file: %w", err) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,144 @@ | ||
| // Copyright (c) 2026 Multus Authors | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package main | ||
|
|
||
| import ( | ||
| "os" | ||
| "path/filepath" | ||
| "testing" | ||
|
|
||
| srv "gopkg.in/k8snetworkplumbingwg/multus-cni.v4/pkg/server" | ||
| ) | ||
|
|
||
| func TestCNIServerConfigReadsValidatedAbsolutePath(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| configPath := filepath.Join(tmpDir, "daemon-config.json") | ||
| configBytes := []byte(`{"logFile":"/etc/cni/net.d/multus.d/daemon-config.json","socketDir":"/run/multus/"}`) | ||
| if err := os.WriteFile(configPath, configBytes, 0600); err != nil { | ||
| t.Fatalf("failed to write test daemon config: %v", err) | ||
| } | ||
|
|
||
| config, err := cniServerConfig(configPath) | ||
| if err != nil { | ||
| t.Fatalf("expected daemon config to load: %v", err) | ||
| } | ||
| if config.SocketDir != "/run/multus/" { | ||
| t.Fatalf("expected socketDir to load, got %q", config.SocketDir) | ||
| } | ||
| if string(config.ConfigFileContents) != string(configBytes) { | ||
| t.Fatalf("expected config contents to be preserved") | ||
| } | ||
| } | ||
|
|
||
| func TestCNIServerConfigRejectsUnsafePaths(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| unsafePaths := []string{ | ||
| "", | ||
| "daemon-config.json", | ||
| tmpDir + string(os.PathSeparator) + ".." + string(os.PathSeparator) + "daemon-config.json", | ||
| } | ||
|
|
||
| for _, unsafePath := range unsafePaths { | ||
| if _, err := cniServerConfig(unsafePath); err == nil { | ||
| t.Fatalf("expected cniServerConfig to reject %q", unsafePath) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestCopyUserProvidedConfigUsesValidatedRoots(t *testing.T) { | ||
| srcDir := t.TempDir() | ||
| destDir := t.TempDir() | ||
| srcPath := filepath.Join(srcDir, "multus.conf") | ||
| if err := os.WriteFile(srcPath, []byte("multus-config"), 0644); err != nil { | ||
| t.Fatalf("failed to write source config: %v", err) | ||
| } | ||
|
|
||
| if err := copyUserProvidedConfig(srcPath, destDir); err != nil { | ||
| t.Fatalf("expected copyUserProvidedConfig to copy config: %v", err) | ||
| } | ||
|
|
||
| copiedConfig, err := os.ReadFile(filepath.Join(destDir, "multus.conf")) | ||
| if err != nil { | ||
| t.Fatalf("failed to read copied config: %v", err) | ||
| } | ||
| if string(copiedConfig) != "multus-config" { | ||
| t.Fatalf("unexpected copied config: %q", copiedConfig) | ||
| } | ||
| } | ||
|
|
||
| func TestCopyUserProvidedConfigRejectsUnsafePaths(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| validSrcPath := filepath.Join(tmpDir, "multus.conf") | ||
| if err := os.WriteFile(validSrcPath, []byte("multus-config"), 0644); err != nil { | ||
| t.Fatalf("failed to write source config: %v", err) | ||
| } | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| multusConfigPath string | ||
| cniConfigDir string | ||
| }{ | ||
| { | ||
| name: "empty source", | ||
| multusConfigPath: "", | ||
| cniConfigDir: tmpDir, | ||
| }, | ||
| { | ||
| name: "relative source", | ||
| multusConfigPath: "multus.conf", | ||
| cniConfigDir: tmpDir, | ||
| }, | ||
| { | ||
| name: "parent source", | ||
| multusConfigPath: tmpDir + string(os.PathSeparator) + ".." + string(os.PathSeparator) + "multus.conf", | ||
| cniConfigDir: tmpDir, | ||
| }, | ||
| { | ||
| name: "relative destination", | ||
| multusConfigPath: validSrcPath, | ||
| cniConfigDir: "relative", | ||
| }, | ||
| { | ||
| name: "parent destination", | ||
| multusConfigPath: validSrcPath, | ||
| cniConfigDir: tmpDir + string(os.PathSeparator) + "..", | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| if err := copyUserProvidedConfig(tc.multusConfigPath, tc.cniConfigDir); err == nil { | ||
| t.Fatalf("expected copyUserProvidedConfig to reject source %q and destination %q", tc.multusConfigPath, tc.cniConfigDir) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestCNIServerConfigUsesDefaultSocketDir(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| configPath := filepath.Join(tmpDir, "daemon-config.json") | ||
| configBytes := []byte(`{"logFile":"/etc/cni/net.d/multus.d/daemon-config.json"}`) | ||
| if err := os.WriteFile(configPath, configBytes, 0600); err != nil { | ||
| t.Fatalf("failed to write test daemon config: %v", err) | ||
| } | ||
|
|
||
| config, err := cniServerConfig(configPath) | ||
| if err != nil { | ||
| t.Fatalf("expected daemon config to load: %v", err) | ||
| } | ||
| if config.SocketDir != srv.DefaultMultusRunDir { | ||
| t.Fatalf("expected default socketDir %q, got %q", srv.DefaultMultusRunDir, config.SocketDir) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change upstream?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes that is upstream as well, wonder why we did that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
That change was made by us in upstream commit:
8ac7d5c53 Remove uncatchable SIGKILL from kubeconfig generatorIt removed
syscall.SIGKILLfromsignal.NotifybecauseSIGKILLcannot be caught by the process, so cleanup code can never run forkill -9.It was a follow-up to the static analysis/CodeRabbit finding on the upstream branch, then it landed in upstream #1524 and came into downstream through the merge. It was not manually added downstream.