fix: enable to use secrets with special characters #961

mpatlasov · 2025-06-25T01:07:19Z

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.

/kind bug

Fixes #507, #358, #573

Release note:

none

k8s-ci-robot · 2025-06-25T01:07:24Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mpatlasov
Once this PR has been reviewed and has the lgtm label, please assign msau42 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2025-06-25T01:07:29Z

Hi @mpatlasov. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

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.

andyzhangx

/ok-to-test

andyzhangx · 2025-06-28T03:19:12Z

pkg/smb/nodeserver.go

@@ -231,7 +231,7 @@ 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)}
+			sensitiveMountOptions = []string{fmt.Sprintf("%s=%s", usernameField, username), fmt.Sprintf("%s=%s", passwordField, password)}


there could be problem when there is mount process concurrently, is it possible to detect whether there is special chars in secrets first, if yes, then mount with cred file? I think that would be safer since it won't break anything, thanks.

andyzhangx · 2025-06-28T03:21:03Z

btw, sanity tests all failed with this PR: https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/kubernetes-csi_csi-driver-smb/961/pull-csi-driver-smb-sanity/1938798037342097408

k8s-ci-robot · 2025-06-28T04:08:58Z

@mpatlasov: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-csi-driver-smb-sanity	`a700858`	link	true	`/test pull-csi-driver-smb-sanity`
pull-csi-driver-smb-e2e	`a700858`	link	false	`/test pull-csi-driver-smb-e2e`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

coveralls · 2025-07-02T07:05:09Z

Pull Request Test Coverage Report for Build 15887613607

Details

10 of 14 (71.43%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.1%) to 78.356%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/smb/smb_common_linux.go	9	13	69.23%

Totals
Change from base Build 15728679263:	-0.1%
Covered Lines:	1115
Relevant Lines:	1423

💛 - Coveralls

johanot · 2025-07-14T08:11:17Z

pkg/smb/smb_common_linux.go

 	"os"

 	mount "k8s.io/mount-utils"
 )

 func Mount(m *mount.SafeFormatAndMount, source, target, fsType string, options, sensitiveMountOptions []string, _ string) error {
+	if len(sensitiveMountOptions) != 0 {
+		file, err := os.CreateTemp("/tmp/", "*.smb.credentials")


I would prefer this file on a ramfs so it wouldn't touch the disk at all. But... at the very minimum, the temp file must have restricted permissions, since /tmp is world-readable on most systems. This needs to be safe in the rare cases that the driver is not run in a container and/or with multiple processes sharing /tmp.

@johanot , the temp file already has restricted permissions (rw for the user only) because that's how os.CreateTemp() works. See its official docs here: https://pkg.go.dev/os#CreateTemp .

I intentionally used "/tmp" instead of ramfs, because the location of ramfs mount may vary depending on environment (at least, I don't know any path that would exist everywhere). But generally I fully agree -- not touching the disk would be better. And "defer os.Remove" will wipe it anyway.

the temp file already has restricted permissions

Oh.. Never mind then :)

And "defer os.Remove" will wipe it anyway.

I guess a rare case exists where the process could get SIGKILL'ed before the deferred function is executed, but.. yeah.

because the location of ramfs mount may vary depending on environment

I think I would create a new ramfs for this purpose alone and choose the location myself, e.g. mount at a level below /tmp. However it's easier said than done, iirc, because you'd have to pull in mount-utils only for this pre-mount purpose. If the maintainers here are good with a standard temp file, then I rest my case.

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. release-note-none Denotes a PR that doesn't merit a release note. labels Jun 25, 2025

k8s-ci-robot requested review from andyzhangx and ZeroMagic June 25, 2025 01:07

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 25, 2025

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 25, 2025

fix: enable to use secrets with special characters

a700858

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.

mpatlasov force-pushed the Use-credentials-mount-option branch from a118c24 to a700858 Compare June 25, 2025 21:30

andyzhangx reviewed Jun 28, 2025

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 28, 2025

andyzhangx reviewed Jun 28, 2025

View reviewed changes

johanot suggested changes Jul 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: enable to use secrets with special characters #961

fix: enable to use secrets with special characters #961

mpatlasov commented Jun 25, 2025

Uh oh!

k8s-ci-robot commented Jun 25, 2025

Uh oh!

k8s-ci-robot commented Jun 25, 2025

Uh oh!

andyzhangx left a comment

Uh oh!

andyzhangx Jun 28, 2025

Uh oh!

andyzhangx commented Jun 28, 2025

Uh oh!

k8s-ci-robot commented Jun 28, 2025

Uh oh!

coveralls commented Jul 2, 2025

Uh oh!

johanot Jul 14, 2025

Uh oh!

mpatlasov Jul 14, 2025

Uh oh!

johanot Jul 15, 2025

Uh oh!

Uh oh!

fix: enable to use secrets with special characters #961

Are you sure you want to change the base?

fix: enable to use secrets with special characters #961

Conversation

mpatlasov commented Jun 25, 2025

Uh oh!

k8s-ci-robot commented Jun 25, 2025

Uh oh!

k8s-ci-robot commented Jun 25, 2025

Uh oh!

andyzhangx left a comment

Choose a reason for hiding this comment

Uh oh!

andyzhangx Jun 28, 2025

Choose a reason for hiding this comment

Uh oh!

andyzhangx commented Jun 28, 2025

Uh oh!

k8s-ci-robot commented Jun 28, 2025

Uh oh!

coveralls commented Jul 2, 2025

Pull Request Test Coverage Report for Build 15887613607

Details

💛 - Coveralls

Uh oh!

johanot Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

mpatlasov Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

johanot Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!