Repository: kubernetes-csi/csi-driver-smb

Issue:
Seems certain characters in smb credentials break authentication.

**What happened**:

It seems mounting of the volume fails when using `$` in base64 encoded password in kubernetes secret.

Mount fails with the following errors:

dmesg
```
[10671.918745] CIFS: Attempting to mount //some/mount/
[10671.934408] Status code returned 0xc000006d STATUS_LOGON_FAILURE
[10671.934415] CIFS VFS: \\some.mount Send error in SessSetup = -13
[10671.934639] CIFS VFS: cifs_mount failed w/return code = -13
```

syslog (kubelet)
```
7-2bl92" (UID: "ac426042-e91d-4999-805e-fd43d7a4c518") : rpc error: code = Internal desc = volume(NewsDropMedia) mount "//some/mount/" on "/var/lib/kubelet/plugins/kubernetes.io/csi/pv/pv-smb/globalmount" failed with mount failed: exit status 32
Jan 13 10:24:22 k8s02usr-sbx-alpha05 kubelet[684]: Mounting command: mount
Jan 13 10:24:22 k8s02usr-sbx-alpha05 kubelet[684]: Mounting arguments: -t cifs -o dir_mode=0777,file_mode=0777,noperm,mfsymlinks,cache=strict,noserverino,vers=3.0,domain=some.domain,<masked> //some/mount/ /var/lib/kubelet/plugins/kubernetes.io/csi/pv/pv-smb/globalmount
Jan 13 10:24:22 k8s02usr-sbx-alpha05 kubelet[684]: Output: mount error(13): Permission denied
Jan 13 10:24:22 k8s02usr-sbx-alpha05 kubelet[684]: Refer to the mount.cifs(8) manual page (e.g. man mount.cifs) and kernel log messages (dmesg)
```

**What you expected to happen**:

Correct mounting of  volume

**How to reproduce it**:

Use `$` character in password.

**Anything else we need to know?**:

Upon removing $ from the password both in AD as well as the kubernetes secret the share mounted as intended.

Physical Volume:
```
apiVersion: v1
kind: PersistentVolume
metadata:
  name: pv-smb
spec:
  capacity:
    storage: 100Gi
  accessModes:
    - ReadWriteMany
  persistentVolumeReclaimPolicy: Retain
  mountOptions:
    - dir_mode=0777
    - file_mode=0777
    - noperm
    - mfsymlinks
    - cache=strict
    - noserverino  # required to prevent data corruption
    - vers=3.0
  csi:
    driver: smb.csi.k8s.io
    readOnly: false
    volumeHandle: somemountid
    volumeAttributes:
      source: "//some/mount/"
    nodeStageSecretRef:
      name: smbcreds
      namespace: smb-app
```

Secret:
```
apiVersion: v1
data:
  domain: b64-domain
  password: b64-password
  username: b64-username
kind: Secret
metadata:
  name: smbcreds
  namespace: smb-app
type: Opaque
```

**Environment**:
- CSI Driver version: 1.9.0
- Kubernetes version (use `kubectl version`): v1.23.8-gke.1900
- OS (e.g. from /etc/os-release): Ubuntu 20.04.5 LTS (Focal Fossa)
- Kernel (e.g. `uname -a`): Linux 5.4.0-1054-gkeop
- Install tools: `curl -skSL https://raw.githubusercontent.com/kubernetes-csi/csi-driver-smb/v1.9.0/deploy/install-driver.sh | bash -s v1.9.0 --`
- Others:


Discussion:
@andyzhangx: do you know whether whether quoting work, e.g.  `"password"`? this is a common cifs issue: https://serverfault.com/questions/309429/mount-cifs-credentials-file-has-special-character@sigurdtheone: >  do you know whether whether quoting work, e.g. "password"? this is a common cifs issue: https://serverfault.com/questions/309429/mount-cifs-credentials-file-has-special-character

Not as such, the credentials in this case are supplied via a secret in Kubernetes which is base 64 encoded, I was not sure what effect encoding them in the secret with quotes would have and wether they'd be enterpreted literally.@serbanvatavu: Also , and * chars are not supported. Does someone knows a workaround for this :) ? Thanks@k8s-triage-robot: The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:
- After 90d of inactivity, `lifecycle/stale` is applied
- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied
- After 30d of inactivity since `lifecycle/rotten` was applied, the issue is closed

You can:
- Mark this issue as fresh with `/remove-lifecycle stale`
- Close this issue with `/close`
- Offer to help out with [Issue Triage][1]

Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community).

/lifecycle stale

[1]: https://www.kubernetes.dev/docs/guide/issue-triage/@johanot: https://github.com/kubernetes/mount-utils/issues/15

csi-driver-smb should really use a credentials file instead of verbatim cmdline mount options, imho. From `man mount.cifs`:
![image](https://github.com/kubernetes-csi/csi-driver-smb/assets/998763/5d7252f3-82ab-4bbb-ad10-addddc83766e)

>  the credentials in this case are supplied via a secret in Kubernetes which is base 64 encoded

Credentials file is not being used currently, the password is grabbed from the k8s secrets api and then passed as "sensitiveOptions" to kubernetes/mount-utils , which in turn passes credentials directly on the mount-cmdline.
@sureshgoli25: We are also facing same problem with special characters in password field of secret. Is there a way to escape the special characters. We tried to wrap password in single quotes but didn't worked out. Also,  tried to escape with \ but no luck.

We need urgent help.@bribroder: I need to create a large number of storageclasses, each using a different username and a password which I can't change. For this case, using the credentials file is really the only good solution

This issue has been reported previously a few times, really worth a fix:

#507
#358
#248@johanot: I've been looking at a fix, but ran out of time. Ideally you would want to setup a ramfs for the credentials file, since - even though it will only exist very briefly - you really don't want the credentials file to ever touch a disk.@k8s-triage-robot: The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:
- After 90d of inactivity, `lifecycle/stale` is applied
- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied
- After 30d of inactivity since `lifecycle/rotten` was applied, the issue is closed

You can:
- Mark this issue as fresh with `/remove-lifecycle rotten`
- Close this issue with `/close`
- Offer to help out with [Issue Triage][1]

Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community).

/lifecycle rotten

[1]: https://www.kubernetes.dev/docs/guide/issue-triage/@bribroder: /remove-lifecycle rotten@k8s-triage-robot: The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:
- After 90d of inactivity, `lifecycle/stale` is applied
- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied
- After 30d of inactivity since `lifecycle/rotten` was applied, the issue is closed

You can:
- Mark this issue as fresh with `/remove-lifecycle stale`
- Close this issue with `/close`
- Offer to help out with [Issue Triage][1]

Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community).

/lifecycle stale

[1]: https://www.kubernetes.dev/docs/guide/issue-triage/@k8s-triage-robot: The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:
- After 90d of inactivity, `lifecycle/stale` is applied
- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied
- After 30d of inactivity since `lifecycle/rotten` was applied, the issue is closed

You can:
- Mark this issue as fresh with `/remove-lifecycle rotten`
- Close this issue with `/close`
- Offer to help out with [Issue Triage][1]

Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community).

/lifecycle rotten

[1]: https://www.kubernetes.dev/docs/guide/issue-triage/@mg-christian-esser: /remove-lifecycle rotten@k8s-triage-robot: The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:
- After 90d of inactivity, `lifecycle/stale` is applied
- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied
- After 30d of inactivity since `lifecycle/rotten` was applied, the issue is closed

You can:
- Mark this issue as fresh with `/remove-lifecycle stale`
- Close this issue with `/close`
- Offer to help out with [Issue Triage][1]

Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community).

/lifecycle stale

[1]: https://www.kubernetes.dev/docs/guide/issue-triage/@mg-christian-esser: /remove-lifecycle stale@k8s-triage-robot: The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:
- After 90d of inactivity, `lifecycle/stale` is applied
- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied
- After 30d of inactivity since `lifecycle/rotten` was applied, the issue is closed

You can:
- Mark this issue as fresh with `/remove-lifecycle stale`
- Close this issue with `/close`
- Offer to help out with [Issue Triage][1]

Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community).

/lifecycle stale

[1]: https://www.kubernetes.dev/docs/guide/issue-triage/@mg-christian-esser: /remove-lifecycle stale@andyzhangx: create a credential file under csi driver, and then set os env is not a trivial fix, not sure whether upstream mount-utils project would take such fix: https://github.com/kubernetes/mount-utils/blob/954afbfb05e3c9338ec8253854e90cf0eadbfa0b/mount_linux.go#L260-L262

what about adding a new field(`base64password`) in secert? then this driver would use base64 decoding for password, that should work

```
# echo 'pa*&$' | base64
cGEqJiQK
# kubectl create secret generic smbcreds-test --from-literal username=USERNAME --from-literal base64password="cGEqJiQK"
```@johanot: I don't understand how #906 fixes the actual issue? The password is still un-encoded when passed to mount-utils as mount arg? Hence, weird characters _still_ end up on the mount cmdline and might fail.@andyzhangx: correct, that won't solve the problem, reopen now.@k8s-triage-robot: The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:
- After 90d of inactivity, `lifecycle/stale` is applied
- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied
- After 30d of inactivity since `lifecycle/rotten` was applied, the issue is closed

You can:
- Mark this issue as fresh with `/remove-lifecycle stale`
- Close this issue with `/close`
- Offer to help out with [Issue Triage][1]

Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community).

/lifecycle stale

[1]: https://www.kubernetes.dev/docs/guide/issue-triage/@mg-christian-esser: /remove-lifecycle stale@esgn: Had a `"` in a password, could not mount, had to change user password. Does it also open a way to inject code in mount command ? @k8s-triage-robot: The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:
- After 90d of inactivity, `lifecycle/stale` is applied
- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied
- After 30d of inactivity since `lifecycle/rotten` was applied, the issue is closed

You can:
- Mark this issue as fresh with `/remove-lifecycle stale`
- Close this issue with `/close`
- Offer to help out with [Issue Triage][1]

Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community).

/lifecycle stale

[1]: https://www.kubernetes.dev/docs/guide/issue-triage/@mg-christian-esser: /remove-lifecycle stale@andyzhangx: fixed by https://github.com/kubernetes-csi/csi-driver-smb/pull/961

Solution patch:
diff --git a/pkg/smb/nodeserver.go b/pkg/smb/nodeserver.go
index 0528003ea..104def37e 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 c6b28fe39..03bc9d031 100644
--- a/pkg/smb/smb_common_linux.go
+++ b/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)
 }
 



Test patch:
diff --git a/pkg/smb/nodeserver_test.go b/pkg/smb/nodeserver_test.go
index 4b87fe1a9..b7d674d43 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_test.go b/pkg/smb/smb_common_linux_test.go
new file mode 100644
index 000000000..ebb6bab99
--- /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))
+	}
+}