From b5e9739c408666673102824772d69d66c7235760 Mon Sep 17 00:00:00 2001 From: Maxim Patlasov Date: Tue, 24 Jun 2025 17:56:21 -0700 Subject: [PATCH 1/2] UPSTREAM: 961: fix: enable to use secrets with special characters If the password to SMB-server contained special characters (e.g. "foo,bar"), the mount failed. Now, when the password is passed to mount via "credentials=filename" option, then mount succeeds. --- pkg/smb/nodeserver.go | 13 ++++++++++++- pkg/smb/smb_common_linux.go | 26 ++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/pkg/smb/nodeserver.go b/pkg/smb/nodeserver.go index 0528003ea01..104def37e32 100644 --- a/pkg/smb/nodeserver.go +++ b/pkg/smb/nodeserver.go @@ -124,6 +124,13 @@ func (d *Driver) NodeUnpublishVolume(_ context.Context, req *csi.NodeUnpublishVo return &csi.NodeUnpublishVolumeResponse{}, nil } +// Returns true if the `word` contains a special character, i.e it can confuse mount command-line if passed as is: +// mount -t cifs -o username=something,password=word,... +// For now, only three such characters are known: "`, +func ContainsSpecialCharacter(word string) bool { + return strings.Contains(word, "\"") || strings.Contains(word, "`") || strings.Contains(word, ",") +} + // NodeStageVolume mount the volume to a staging path func (d *Driver) NodeStageVolume(ctx context.Context, req *csi.NodeStageVolumeRequest) (*csi.NodeStageVolumeResponse, error) { volumeID := req.GetVolumeId() @@ -231,7 +238,11 @@ func (d *Driver) NodeStageVolume(ctx context.Context, req *csi.NodeStageVolumeRe return nil, status.Error(codes.Internal, fmt.Sprintf("MkdirAll %s failed with error: %v", targetPath, err)) } if requireUsernamePwdOption && !useKerberosCache { - sensitiveMountOptions = []string{fmt.Sprintf("%s=%s,%s=%s", usernameField, username, passwordField, password)} + if ContainsSpecialCharacter(password) { + sensitiveMountOptions = []string{fmt.Sprintf("%s=%s", usernameField, username), fmt.Sprintf("%s=%s", passwordField, password)} + } else { + sensitiveMountOptions = []string{fmt.Sprintf("%s=%s,%s=%s", usernameField, username, passwordField, password)} + } } mountOptions = mountFlags if !gidPresent && volumeMountGroup != "" { diff --git a/pkg/smb/smb_common_linux.go b/pkg/smb/smb_common_linux.go index c6b28fe394d..1cedb42bfb1 100644 --- a/pkg/smb/smb_common_linux.go +++ b/pkg/smb/smb_common_linux.go @@ -20,12 +20,38 @@ limitations under the License. package smb import ( + "fmt" "os" + "strings" mount "k8s.io/mount-utils" ) +// Returns true if the `options` contains password with a special characters, and so "credentials=" needed. +// (see comments for ContainsSpecialCharacter() in pkg/smb/nodeserver.go). +// NB: implementation relies on the format: +// options := []string{fmt.Sprintf("%s=%s", usernameField, username), fmt.Sprintf("%s=%s", passwordField, password)} +func NeedsCredentialsOption(options []string) bool { + return len(options) == 2 && strings.HasPrefix(options[1], "password=") && ContainsSpecialCharacter(options[1]) +} + func Mount(m *mount.SafeFormatAndMount, source, target, fsType string, options, sensitiveMountOptions []string, _ string) error { + if NeedsCredentialsOption(sensitiveMountOptions) { + file, err := os.CreateTemp("/tmp/", "*.smb.credentials") + if err != nil { + return err + } + + for _, option := range sensitiveMountOptions { + if _, err := file.Write([]byte(fmt.Sprintf("%s\n", option))); err != nil { + return err + } + } + file.Close() + defer os.Remove(file.Name()) + + sensitiveMountOptions = []string{fmt.Sprintf("credentials=%s", file.Name())} + } return m.MountSensitive(source, target, fsType, options, sensitiveMountOptions) } From 119fe5d3cdb6e9349f845fdd24e0d4fe03345ac1 Mon Sep 17 00:00:00 2001 From: Maxim Patlasov Date: Mon, 11 Aug 2025 17:10:13 -0700 Subject: [PATCH 2/2] UPSTREAM: 961: fix: move defer up and add unit-tests for mount with "credentials=" option --- pkg/smb/nodeserver_test.go | 22 +++++++++++++ pkg/smb/smb_common_linux.go | 6 ++-- pkg/smb/smb_common_linux_test.go | 54 ++++++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 pkg/smb/smb_common_linux_test.go diff --git a/pkg/smb/nodeserver_test.go b/pkg/smb/nodeserver_test.go index 4b87fe1a987..b7d674d4335 100644 --- a/pkg/smb/nodeserver_test.go +++ b/pkg/smb/nodeserver_test.go @@ -92,6 +92,11 @@ func TestNodeStageVolume(t *testing.T) { passwordField: "test_password", domainField: "test_doamin", } + secretsSpecial := map[string]string{ + usernameField: "test_username", + passwordField: "test\"`,password", + domainField: "test_doamin", + } tests := []struct { desc string @@ -182,6 +187,23 @@ func TestNodeStageVolume(t *testing.T) { strings.Replace(testSource, "\\", "\\\\", -1), errorMountSensSource), }, }, + { + desc: "[Error] Failed SMB mount mocked by MountSensitive (password with special characters)", + req: &csi.NodeStageVolumeRequest{VolumeId: "vol_1##", StagingTargetPath: errorMountSensSource, + VolumeCapability: &stdVolCap, + VolumeContext: volContext, + Secrets: secretsSpecial}, + skipOnWindows: true, + flakyWindowsErrorMessage: fmt.Sprintf("rpc error: code = Internal desc = volume(vol_1##) mount \"%s\" on %#v failed "+ + "with NewSmbGlobalMapping(%s, %s) failed with error: rpc error: code = Unknown desc = NewSmbGlobalMapping failed.", + strings.Replace(testSource, "\\", "\\\\", -1), errorMountSensSource, testSource, errorMountSensSource), + expectedErr: testutil.TestError{ + DefaultError: status.Errorf(codes.Internal, + "volume(vol_1##) mount \"%s\" on \"%s\" failed with fake "+ + "MountSensitive: target error", + strings.Replace(testSource, "\\", "\\\\", -1), errorMountSensSource), + }, + }, { desc: "[Success] Valid request", req: &csi.NodeStageVolumeRequest{VolumeId: "vol_1##", StagingTargetPath: sourceTest, diff --git a/pkg/smb/smb_common_linux.go b/pkg/smb/smb_common_linux.go index 1cedb42bfb1..03bc9d0312c 100644 --- a/pkg/smb/smb_common_linux.go +++ b/pkg/smb/smb_common_linux.go @@ -41,14 +41,16 @@ func Mount(m *mount.SafeFormatAndMount, source, target, fsType string, options, if err != nil { return err } + defer func() { + file.Close() + os.Remove(file.Name()) + }() for _, option := range sensitiveMountOptions { if _, err := file.Write([]byte(fmt.Sprintf("%s\n", option))); err != nil { return err } } - file.Close() - defer os.Remove(file.Name()) sensitiveMountOptions = []string{fmt.Sprintf("credentials=%s", file.Name())} } diff --git a/pkg/smb/smb_common_linux_test.go b/pkg/smb/smb_common_linux_test.go new file mode 100644 index 00000000000..ebb6bab9947 --- /dev/null +++ b/pkg/smb/smb_common_linux_test.go @@ -0,0 +1,54 @@ +/* +Copyright 2020 The Kubernetes 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 smb + +import ( + "github.com/stretchr/testify/assert" + "testing" +) + +func TestNeedsCredentialsOption(t *testing.T) { + tests := []struct { + options []string + expectedResult bool + }{ + { + options: []string{"username=foo,password=bar"}, + expectedResult: false, + }, + { + options: []string{"username=foo", "password=bar"}, + expectedResult: false, + }, + { + options: []string{"username=foo", "password=b\"r"}, + expectedResult: true, + }, + { + options: []string{"username=foo", "password=b`r"}, + expectedResult: true, + }, + { + options: []string{"username=foo", "password=b,r"}, + expectedResult: true, + }, + } + + for _, test := range tests { + assert.Equal(t, test.expectedResult, NeedsCredentialsOption(test.options)) + } +}