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/Makefile b/Makefile
index 033d1e161..23eecfd75 100644
--- a/Makefile
+++ b/Makefile
@@ -246,7 +246,7 @@ endif
 .PHONY: install-smb-provisioner
 install-smb-provisioner:
 	kubectl delete secret smbcreds --ignore-not-found -n default
-	kubectl create secret generic smbcreds --from-literal username=USERNAME --from-literal password="PASSWORD" --from-literal mountOptions="dir_mode=0777,file_mode=0777,uid=0,gid=0,mfsymlinks" -n default
+	kubectl create secret generic smbcreds --from-literal username=USERNAME --from-literal password="PASSWORD" --from-literal base64password="UEFTU1dPUkQ=" --from-literal mountOptions="dir_mode=0777,file_mode=0777,uid=0,gid=0,mfsymlinks" -n default
 ifdef TEST_WINDOWS
 	kubectl apply -f deploy/example/smb-provisioner/smb-server-lb.yaml
 else
diff --git a/pkg/smb/nodeserver.go b/pkg/smb/nodeserver.go
index 0528003ea..96f78637c 100644
--- a/pkg/smb/nodeserver.go
+++ b/pkg/smb/nodeserver.go
@@ -183,7 +183,7 @@ func (d *Driver) NodeStageVolume(ctx context.Context, req *csi.NodeStageVolumeRe
 	}
 	defer d.volumeLocks.Release(lockKey)
 
-	var username, password, domain string
+	var username, password, base64Password, domain string
 	for k, v := range secrets {
 		switch strings.ToLower(k) {
 		case usernameField:
@@ -192,9 +192,20 @@ func (d *Driver) NodeStageVolume(ctx context.Context, req *csi.NodeStageVolumeRe
 			password = strings.TrimSpace(v)
 		case domainField:
 			domain = strings.TrimSpace(v)
+		case base64PasswordField:
+			base64Password = strings.TrimSpace(v)
 		}
 	}
 
+	if base64Password != "" {
+		klog.V(2).Infof("NodeStageVolume: decoding password from base64 string")
+		decodePassword, err := base64.StdEncoding.DecodeString(base64Password)
+		if err != nil {
+			return nil, status.Error(codes.InvalidArgument, "error base64 decoding password")
+		}
+		password = string(decodePassword)
+	}
+
 	if ephemeralVol {
 		mountFlags = strings.Split(ephemeralVolMountOptions, ",")
 	}
diff --git a/pkg/smb/smb.go b/pkg/smb/smb.go
index d0f236570..1dcfe83d4 100644
--- a/pkg/smb/smb.go
+++ b/pkg/smb/smb.go
@@ -18,6 +18,7 @@ package smb
 
 import (
 	"context"
+	"encoding/base64"
 	"errors"
 	"fmt"
 	"net"
@@ -49,6 +50,7 @@ const (
 	sourceField               = "source"
 	subDirField               = "subdir"
 	domainField               = "domain"
+	base64PasswordField       = "base64password"
 	mountOptionsField         = "mountoptions"
 	secretNameField           = "secretname"
 	secretNamespaceField      = "secretnamespace"
@@ -232,6 +234,15 @@ func (d *Driver) GetUserNamePasswordFromSecret(ctx context.Context, secretName,
 	username := strings.TrimSpace(string(secret.Data[usernameField][:]))
 	password := strings.TrimSpace(string(secret.Data[passwordField][:]))
 	domain := strings.TrimSpace(string(secret.Data[domainField][:]))
+	base64Password := strings.TrimSpace(string(secret.Data[base64PasswordField][:]))
+	if base64Password != "" {
+		klog.V(2).Infof("decoding password from base64 string")
+		decodePassword, err := base64.StdEncoding.DecodeString(base64Password)
+		if err != nil {
+			return "", "", "", fmt.Errorf("could not decode password from base64 string: %v", err)
+		}
+		password = string(decodePassword)
+	}
 	return username, password, domain, nil
 }
 



Test patch:
diff --git a/pkg/smb/nodeserver_test.go b/pkg/smb/nodeserver_test.go
index 4b87fe1a9..ea914196f 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",
 	}
+	secretsWithBase64Password := map[string]string{
+		usernameField: "test_username",
+		passwordField: base64.StdEncoding.EncodeToString([]byte("test_password")),
+		domainField:   "test_doamin",
+	}
 
 	tests := []struct {
 		desc        string
@@ -230,6 +235,18 @@ func TestNodeStageVolume(t *testing.T) {
 				strings.Replace(testSource, "\\", "\\\\", -1), sourceTest, testSource, sourceTest),
 			expectedErr: testutil.TestError{},
 		},
+		{
+			desc: "[Success] Valid request with base64 encoded password",
+			req: &csi.NodeStageVolumeRequest{VolumeId: "vol_1##", StagingTargetPath: sourceTest,
+				VolumeCapability: &stdVolCap,
+				VolumeContext:    volContext,
+				Secrets:          secretsWithBase64Password},
+			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), sourceTest, testSource, sourceTest),
+			expectedErr: testutil.TestError{},
+		},
 	}
 
 	// Setup
diff --git a/pkg/smb/smb_test.go b/pkg/smb/smb_test.go
index 21761cbbd..ff01686db 100644
--- a/pkg/smb/smb_test.go
+++ b/pkg/smb/smb_test.go
@@ -17,6 +17,7 @@ limitations under the License.
 package smb
 
 import (
+	"context"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -520,6 +521,33 @@ users:
 	}
 }
 
+func TestGetUserNamePasswordFromSecret(t *testing.T) {
+	tests := []struct {
+		desc             string
+		secretName       string
+		secretNamespace  string
+		expectedUsername string
+		expectedPassword string
+		expectedDomain   string
+		expectedError    error
+	}{
+		{
+			desc:          "kubeclient is nil",
+			secretName:    "secretName",
+			expectedError: fmt.Errorf("could not username and password from secret(secretName): KubeClient is nil"),
+		},
+	}
+
+	d := NewFakeDriver()
+	for _, test := range tests {
+		username, password, domain, err := d.GetUserNamePasswordFromSecret(context.Background(), test.secretName, test.secretNamespace)
+		assert.Equal(t, test.expectedUsername, username, "test[%s]: unexpected username", test.desc)
+		assert.Equal(t, test.expectedPassword, password, "test[%s]: unexpected password", test.desc)
+		assert.Equal(t, test.expectedDomain, domain, "test[%s]: unexpected domain", test.desc)
+		assert.Equal(t, test.expectedError, err, "test[%s]: unexpected error", test.desc)
+	}
+}
+
 func createTestFile(path string) error {
 	f, err := os.Create(path)
 	if err != nil {