OCPBUGS-57090: UPSTREAM: 961: fix: enable to use secrets with special characters

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 != "" {

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,

pkg/smb/smb_common_linux.go

@@ -20,12 +20,40 @@ 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
+		}
+		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
+			}
+		}
+
+		sensitiveMountOptions = []string{fmt.Sprintf("credentials=%s", file.Name())}
+	}
 	return m.MountSensitive(source, target, fsType, options, sensitiveMountOptions)
 }
 

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))
+	}
+}