Introduce paramater for sensitive mount options.

Introduce optional sensitiveOptions parameter to allow sensitive mount
options to be passed in a separate parameter from the normal mount
options and ensures the sensitiveOptions are never logged.

fake_mounter.go

@@ -82,6 +82,15 @@ func (f *FakeMounter) GetLog() []FakeAction {
 
 // Mount records the mount event and updates the in-memory mount points for FakeMounter
 func (f *FakeMounter) Mount(source string, target string, fstype string, options []string) error {
+	return f.MountSensitive(source, target, fstype, options, nil /* sensitiveOptions */)
+}
+
+// Mount records the mount event and updates the in-memory mount points for FakeMounter
+// sensitiveOptions to be passed in a separate parameter from the normal
+// mount options and ensures the sensitiveOptions are never logged. This
+// method should be used by callers that pass sensitive material (like
+// passwords) as mount options.
+func (f *FakeMounter) MountSensitive(source string, target string, fstype string, options []string, sensitiveOptions []string) error {
 	f.mutex.Lock()
 	defer f.mutex.Unlock()
 
@@ -117,7 +126,7 @@ func (f *FakeMounter) Mount(source string, target string, fstype string, options
 	if err != nil {
 		absTarget = target
 	}
-	f.MountPoints = append(f.MountPoints, MountPoint{Device: source, Path: absTarget, Type: fstype, Opts: opts})
+	f.MountPoints = append(f.MountPoints, MountPoint{Device: source, Path: absTarget, Type: fstype, Opts: append(opts, sensitiveOptions...)})
 	klog.V(5).Infof("Fake mounter: mounted %s to %s", source, absTarget)
 	f.log = append(f.log, FakeAction{Action: FakeActionMount, Target: absTarget, Source: source, FSType: fstype})
 	return nil

mount.go

@@ -31,12 +31,21 @@ import (
 const (
 	// Default mount command if mounter path is not specified.
 	defaultMountCommand = "mount"
+	// Log message where sensitive mount options were removed
+	sensitiveOptionsRemoved = "<masked>"
 )
 
 // Interface defines the set of methods to allow for mount operations on a system.
 type Interface interface {
 	// Mount mounts source to target as fstype with given options.
+	// options MUST not contain sensitive material (like passwords).
 	Mount(source string, target string, fstype string, options []string) error
+	// MountSensitive is the same as Mount() but this method allows
+	// sensitiveOptions to be passed in a separate parameter from the normal
+	// mount options and ensures the sensitiveOptions are never logged. This
+	// method should be used by callers that pass sensitive material (like
+	// passwords) as mount options.
+	MountSensitive(source string, target string, fstype string, options []string, sensitiveOptions []string) error
 	// Unmount unmounts given target.
 	Unmount(target string) error
 	// List returns a list of all mounted filesystems.  This can be large.
@@ -72,7 +81,7 @@ type MountPoint struct {
 	Device string
 	Path   string
 	Type   string
-	Opts   []string
+	Opts   []string // Opts may contain sensitive mount options (like passwords) and MUST be treated as such (e.g. not logged).
 	Freq   int
 	Pass   int
 }
@@ -122,8 +131,18 @@ type SafeFormatAndMount struct {
 // read-only it will format it first then mount it. Otherwise, if the
 // disk is already formatted or it is being mounted as read-only, it
 // will be mounted without formatting.
+// options MUST not contain sensitive material (like passwords).
 func (mounter *SafeFormatAndMount) FormatAndMount(source string, target string, fstype string, options []string) error {
-	return mounter.formatAndMount(source, target, fstype, options)
+	return mounter.FormatAndMountSensitive(source, target, fstype, options, nil /* sensitiveOptions */)
+}
+
+// FormatAndMountSensitive is the same as FormatAndMount but this method allows
+// sensitiveOptions to be passed in a separate parameter from the normal mount
+// options and ensures the sensitiveOptions are never logged. This method should
+// be used by callers that pass sensitive material (like passwords) as mount
+// options.
+func (mounter *SafeFormatAndMount) FormatAndMountSensitive(source string, target string, fstype string, options []string, sensitiveOptions []string) error {
+	return mounter.formatAndMountSensitive(source, target, fstype, options, sensitiveOptions)
 }
 
 // getMountRefsByDev finds all references to the device provided
@@ -240,6 +259,16 @@ func IsNotMountPoint(mounter Interface, file string) (bool, error) {
 // The list equals:
 //   options - 'bind' + 'remount' (no duplicate)
 func MakeBindOpts(options []string) (bool, []string, []string) {
+	bind, bindOpts, bindRemountOpts, _ := MakeBindOptsSensitive(options, nil /* sensitiveOptions */)
+	return bind, bindOpts, bindRemountOpts
+}
+
+// MakeBindOptsSensitive is the same as MakeBindOpts but this method allows
+// sensitiveOptions to be passed in a separate parameter from the normal mount
+// options and ensures the sensitiveOptions are never logged. This method should
+// be used by callers that pass sensitive material (like passwords) as mount
+// options.
+func MakeBindOptsSensitive(options []string, sensitiveOptions []string) (bool, []string, []string, []string) {
 	// Because we have an FD opened on the subpath bind mount, the "bind" option
 	// needs to be included, otherwise the mount target will error as busy if you
 	// remount as readonly.
@@ -247,12 +276,13 @@ func MakeBindOpts(options []string) (bool, []string, []string) {
 	// As a consequence, all read only bind mounts will no longer change the underlying
 	// volume mount to be read only.
 	bindRemountOpts := []string{"bind", "remount"}
+	bindRemountSensitiveOpts := []string{}
 	bind := false
 	bindOpts := []string{"bind"}
 
 	// _netdev is a userspace mount option and does not automatically get added when
 	// bind mount is created and hence we must carry it over.
-	if checkForNetDev(options) {
+	if checkForNetDev(options, sensitiveOptions) {
 		bindOpts = append(bindOpts, "_netdev")
 	}
 
@@ -268,15 +298,32 @@ func MakeBindOpts(options []string) (bool, []string, []string) {
 		}
 	}
 
-	return bind, bindOpts, bindRemountOpts
+	for _, sensitiveOption := range sensitiveOptions {
+		switch sensitiveOption {
+		case "bind":
+			bind = true
+			break
+		case "remount":
+			break
+		default:
+			bindRemountSensitiveOpts = append(bindRemountSensitiveOpts, sensitiveOption)
+		}
+	}
+
+	return bind, bindOpts, bindRemountOpts, bindRemountSensitiveOpts
 }
 
-func checkForNetDev(options []string) bool {
+func checkForNetDev(options []string, sensitiveOptions []string) bool {
 	for _, option := range options {
 		if option == "_netdev" {
 			return true
 		}
 	}
+	for _, sensitiveOption := range sensitiveOptions {
+		if sensitiveOption == "_netdev" {
+			return true
+		}
+	}
 	return false
 }
 
@@ -298,3 +345,26 @@ func StartsWithBackstep(rel string) bool {
 	// normalize to / and check for ../
 	return rel == ".." || strings.HasPrefix(filepath.ToSlash(rel), "../")
 }
+
+// sanitizedOptionsForLogging will return a comma seperated string containing
+// options and sensitiveOptions. Each entry in sensitiveOptions will be
+// replaced with the string sensitiveOptionsRemoved
+// e.g. o1,o2,<masked>,<masked>
+func sanitizedOptionsForLogging(options []string, sensitiveOptions []string) string {
+	seperator := ""
+	if len(options) > 0 && len(sensitiveOptions) > 0 {
+		seperator = ","
+	}
+
+	sensitiveOptionsStart := ""
+	sensitiveOptionsEnd := ""
+	if len(sensitiveOptions) > 0 {
+		sensitiveOptionsStart = strings.Repeat(sensitiveOptionsRemoved+",", len(sensitiveOptions)-1)
+		sensitiveOptionsEnd = sensitiveOptionsRemoved
+	}
+
+	return strings.Join(options, ",") +
+		seperator +
+		sensitiveOptionsStart +
+		sensitiveOptionsEnd
+}

mount_linux.go

@@ -69,16 +69,25 @@ func New(mounterPath string) Interface {
 // currently come from mount(8), e.g. "ro", "remount", "bind", etc. If no more option is
 // required, call Mount with an empty string list or nil.
 func (mounter *Mounter) Mount(source string, target string, fstype string, options []string) error {
+	return mounter.MountSensitive(source, target, fstype, options, nil)
+}
+
+// MountSensitive is the same as Mount() but this method allows
+// sensitiveOptions to be passed in a separate parameter from the normal
+// mount options and ensures the sensitiveOptions are never logged. This
+// method should be used by callers that pass sensitive material (like
+// passwords) as mount options.
+func (mounter *Mounter) MountSensitive(source string, target string, fstype string, options []string, sensitiveOptions []string) error {
 	// Path to mounter binary if containerized mounter is needed. Otherwise, it is set to empty.
 	// All Linux distros are expected to be shipped with a mount utility that a support bind mounts.
 	mounterPath := ""
-	bind, bindOpts, bindRemountOpts := MakeBindOpts(options)
+	bind, bindOpts, bindRemountOpts, bindRemountOptsSensitive := MakeBindOptsSensitive(options, sensitiveOptions)
 	if bind {
-		err := mounter.doMount(mounterPath, defaultMountCommand, source, target, fstype, bindOpts)
+		err := mounter.doMount(mounterPath, defaultMountCommand, source, target, fstype, bindOpts, bindRemountOptsSensitive)
 		if err != nil {
 			return err
 		}
-		return mounter.doMount(mounterPath, defaultMountCommand, source, target, fstype, bindRemountOpts)
+		return mounter.doMount(mounterPath, defaultMountCommand, source, target, fstype, bindRemountOpts, bindRemountOptsSensitive)
 	}
 	// The list of filesystems that require containerized mounter on GCI image cluster
 	fsTypesNeedMounter := map[string]struct{}{
@@ -90,14 +99,16 @@ func (mounter *Mounter) Mount(source string, target string, fstype string, optio
 	if _, ok := fsTypesNeedMounter[fstype]; ok {
 		mounterPath = mounter.mounterPath
 	}
-	return mounter.doMount(mounterPath, defaultMountCommand, source, target, fstype, options)
+	return mounter.doMount(mounterPath, defaultMountCommand, source, target, fstype, options, sensitiveOptions)
 }
 
 // doMount runs the mount command. mounterPath is the path to mounter binary if containerized mounter is used.
-func (mounter *Mounter) doMount(mounterPath string, mountCmd string, source string, target string, fstype string, options []string) error {
-	mountArgs := MakeMountArgs(source, target, fstype, options)
+// sensitiveOptions is an extention of options except they will not be logged (because they may contain sensitive material)
+func (mounter *Mounter) doMount(mounterPath string, mountCmd string, source string, target string, fstype string, options []string, sensitiveOptions []string) error {
+	mountArgs, mountArgsLogStr := MakeMountArgsSensitive(source, target, fstype, options, sensitiveOptions)
 	if len(mounterPath) > 0 {
 		mountArgs = append([]string{mountCmd}, mountArgs...)
+		mountArgsLogStr = mountCmd + " " + mountArgsLogStr
 		mountCmd = mounterPath
 	}
 
@@ -124,21 +135,21 @@ func (mounter *Mounter) doMount(mounterPath string, mountCmd string, source stri
 		//
 		// systemd-mount is not used because it's too new for older distros
 		// (CentOS 7, Debian Jessie).
-		mountCmd, mountArgs = AddSystemdScope("systemd-run", target, mountCmd, mountArgs)
+		mountCmd, mountArgs, mountArgsLogStr = AddSystemdScopeSensitive("systemd-run", target, mountCmd, mountArgs, mountArgsLogStr)
 	} else {
 		// No systemd-run on the host (or we failed to check it), assume kubelet
 		// does not run as a systemd service.
 		// No code here, mountCmd and mountArgs are already populated.
 	}
 
-	klog.V(4).Infof("Mounting cmd (%s) with arguments (%s)", mountCmd, mountArgs)
+	// Logging with sensitive mount options removed.
+	klog.V(4).Infof("Mounting cmd (%s) with arguments (%s)", mountCmd, mountArgsLogStr)
 	command := exec.Command(mountCmd, mountArgs...)
 	output, err := command.CombinedOutput()
 	if err != nil {
-		args := strings.Join(mountArgs, " ")
-		klog.Errorf("Mount failed: %v\nMounting command: %s\nMounting arguments: %s\nOutput: %s\n", err, mountCmd, args, string(output))
+		klog.Errorf("Mount failed: %v\nMounting command: %s\nMounting arguments: %s\nOutput: %s\n", err, mountCmd, mountArgsLogStr, string(output))
 		return fmt.Errorf("mount failed: %v\nMounting command: %s\nMounting arguments: %s\nOutput: %s",
-			err, mountCmd, args, string(output))
+			err, mountCmd, mountArgsLogStr, string(output))
 	}
 	return err
 }
@@ -169,33 +180,59 @@ func detectSystemd() bool {
 }
 
 // MakeMountArgs makes the arguments to the mount(8) command.
-// Implementation is shared with NsEnterMounter
-func MakeMountArgs(source, target, fstype string, options []string) []string {
-	// Build mount command as follows:
-	//   mount [-t $fstype] [-o $options] [$source] $target
-	mountArgs := []string{}
-	if len(fstype) > 0 {
-		mountArgs = append(mountArgs, "-t", fstype)
-	}
-	if len(options) > 0 {
-		mountArgs = append(mountArgs, "-o", strings.Join(options, ","))
-	}
-	if len(source) > 0 {
-		mountArgs = append(mountArgs, source)
-	}
-	mountArgs = append(mountArgs, target)
-
+// options MUST not contain sensitive material (like passwords).
+func MakeMountArgs(source, target, fstype string, options []string) (mountArgs []string) {
+	mountArgs, _ = MakeMountArgsSensitive(source, target, fstype, options, nil /* sensitiveOptions */)
 	return mountArgs
 }
 
+// MakeMountArgsSensitive makes the arguments to the mount(8) command.
+// sensitiveOptions is an extention of options except they will not be logged (because they may contain sensitive material)
+func MakeMountArgsSensitive(source, target, fstype string, options []string, sensitiveOptions []string) (mountArgs []string, mountArgsLogStr string) {
+	// Build mount command as follows:
+	//   mount [-t $fstype] [-o $options] [$source] $target
+	mountArgs = []string{}
+	mountArgsLogStr = ""
+	if len(fstype) > 0 {
+		mountArgs = append(mountArgs, "-t", fstype)
+		mountArgsLogStr += strings.Join(mountArgs, " ")
+	}
+	if len(options) > 0 || len(sensitiveOptions) > 0 {
+		combinedOptions := []string{}
+		combinedOptions = append(combinedOptions, options...)
+		combinedOptions = append(combinedOptions, sensitiveOptions...)
+		mountArgs = append(mountArgs, "-o", strings.Join(combinedOptions, ","))
+		// exclude sensitiveOptions from log string
+		mountArgsLogStr += " -o " + sanitizedOptionsForLogging(options, sensitiveOptions)
+	}
+	if len(source) > 0 {
+		mountArgs = append(mountArgs, source)
+		mountArgsLogStr += " " + source
+	}
+	mountArgs = append(mountArgs, target)
+	mountArgsLogStr += " " + target
+
+	return mountArgs, mountArgsLogStr
+}
+
 // AddSystemdScope adds "system-run --scope" to given command line
-// implementation is shared with NsEnterMounter
+// If args contains sensitive material, use AddSystemdScopeSensitive to construct
+// a safe to log string.
 func AddSystemdScope(systemdRunPath, mountName, command string, args []string) (string, []string) {
 	descriptionArg := fmt.Sprintf("--description=Kubernetes transient mount for %s", mountName)
 	systemdRunArgs := []string{descriptionArg, "--scope", "--", command}
 	return systemdRunPath, append(systemdRunArgs, args...)
 }
 
+// AddSystemdScopeSensitive adds "system-run --scope" to given command line
+// It also accepts takes a sanitized string containing mount arguments, mountArgsLogStr,
+// and returns the string appended to the systemd command for logging.
+func AddSystemdScopeSensitive(systemdRunPath, mountName, command string, args []string, mountArgsLogStr string) (string, []string, string) {
+	descriptionArg := fmt.Sprintf("--description=Kubernetes transient mount for %s", mountName)
+	systemdRunArgs := []string{descriptionArg, "--scope", "--", command}
+	return systemdRunPath, append(systemdRunArgs, args...), strings.Join(systemdRunArgs, " ") + " " + mountArgsLogStr
+}
+
 // Unmount unmounts the target.
 func (mounter *Mounter) Unmount(target string) error {
 	klog.V(4).Infof("Unmounting %s", target)
@@ -305,7 +342,7 @@ func (mounter *SafeFormatAndMount) checkAndRepairXfsFilesystem(source string) er
 }
 
 // formatAndMount uses unix utils to format and mount the given disk
-func (mounter *SafeFormatAndMount) formatAndMount(source string, target string, fstype string, options []string) error {
+func (mounter *SafeFormatAndMount) formatAndMountSensitive(source string, target string, fstype string, options []string, sensitiveOptions []string) error {
 	readOnly := false
 	for _, option := range options {
 		if option == "ro" {
@@ -313,6 +350,15 @@ func (mounter *SafeFormatAndMount) formatAndMount(source string, target string,
 			break
 		}
 	}
+	if !readOnly {
+		// Check sensitiveOptions for ro
+		for _, option := range sensitiveOptions {
+			if option == "ro" {
+				readOnly = true
+				break
+			}
+		}
+	}
 
 	options = append(options, "defaults")
 	mountErrorValue := UnknownMountError
@@ -347,7 +393,9 @@ func (mounter *SafeFormatAndMount) formatAndMount(source string, target string,
 		klog.Infof("Disk %q appears to be unformatted, attempting to format as type: %q with options: %v", source, fstype, args)
 		output, err := mounter.Exec.Command("mkfs."+fstype, args...).CombinedOutput()
 		if err != nil {
-			detailedErr := fmt.Sprintf("format of disk %q failed: type:(%q) target:(%q) options:(%q) errcode:(%v) output:(%v) ", source, fstype, target, options, err, string(output))
+			// Do not log sensitiveOptions only options
+			sensitiveOptionsLog := sanitizedOptionsForLogging(options, sensitiveOptions)
+			detailedErr := fmt.Sprintf("format of disk %q failed: type:(%q) target:(%q) options:(%q) errcode:(%v) output:(%v) ", source, fstype, target, sensitiveOptionsLog, err, string(output))
 			klog.Error(detailedErr)
 			return NewMountError(FormatFailed, detailedErr)
 		}
@@ -378,7 +426,7 @@ func (mounter *SafeFormatAndMount) formatAndMount(source string, target string,
 
 	// Mount the disk
 	klog.V(4).Infof("Attempting to mount disk %s in %s format at %s", source, fstype, target)
-	if err := mounter.Interface.Mount(source, target, fstype, options); err != nil {
+	if err := mounter.MountSensitive(source, target, fstype, options, sensitiveOptions); err != nil {
 		return NewMountError(mountErrorValue, err.Error())
 	}
 
@@ -457,7 +505,8 @@ func parseProcMounts(content []byte) ([]MountPoint, error) {
 		}
 		fields := strings.Fields(line)
 		if len(fields) != expectedNumFieldsPerLine {
-			return nil, fmt.Errorf("wrong number of fields (expected %d, got %d): %s", expectedNumFieldsPerLine, len(fields), line)
+			// Do not log line in case it contains sensitive Mount options
+			return nil, fmt.Errorf("wrong number of fields (expected %d, got %d)", expectedNumFieldsPerLine, len(fields))
 		}
 
 		mp := MountPoint{

mount_linux_test.go

@@ -22,6 +22,7 @@ import (
 	"io/ioutil"
 	"os"
 	"reflect"
+	"strings"
 	"testing"
 )
 
@@ -437,3 +438,79 @@ func TestSearchMountPoints(t *testing.T) {
 		}
 	}
 }
+
+func TestSensitiveMountOptions(t *testing.T) {
+	// Arrange
+	testcases := []struct {
+		source           string
+		target           string
+		fstype           string
+		options          []string
+		sensitiveOptions []string
+	}{
+		{
+
+			source:           "mySrc",
+			target:           "myTarget",
+			fstype:           "myFS",
+			options:          []string{"o1", "o2"},
+			sensitiveOptions: []string{"s1", "s2"},
+		},
+		{
+
+			source:           "mySrc",
+			target:           "myTarget",
+			fstype:           "myFS",
+			options:          []string{},
+			sensitiveOptions: []string{"s1", "s2"},
+		},
+		{
+
+			source:           "mySrc",
+			target:           "myTarget",
+			fstype:           "myFS",
+			options:          []string{"o1", "o2"},
+			sensitiveOptions: []string{},
+		},
+	}
+
+	for _, v := range testcases {
+		// Act
+		mountArgs, mountArgsLogStr := MakeMountArgsSensitive(v.source, v.target, v.fstype, v.options, v.sensitiveOptions)
+
+		// Assert
+		t.Logf("\r\nmountArgs =%q\r\nmountArgsLogStr=%q", mountArgs, mountArgsLogStr)
+		for _, option := range v.options {
+			if found := contains(mountArgs, option, t); !found {
+				t.Errorf("Expected option (%q) to exist in returned mountArts (%q), but it does not", option, mountArgs)
+			}
+			if !strings.Contains(mountArgsLogStr, option) {
+				t.Errorf("Expected option (%q) to exist in returned mountArgsLogStr (%q), but it does", option, mountArgsLogStr)
+			}
+		}
+		for _, sensitiveOption := range v.sensitiveOptions {
+			if found := contains(mountArgs, sensitiveOption, t); !found {
+				t.Errorf("Expected sensitiveOption (%q) to exist in returned mountArts (%q), but it does not", sensitiveOption, mountArgs)
+			}
+			if strings.Contains(mountArgsLogStr, sensitiveOption) {
+				t.Errorf("Expected sensitiveOption (%q) to not exist in returned mountArgsLogStr (%q), but it does", sensitiveOption, mountArgsLogStr)
+			}
+		}
+	}
+}
+
+func contains(slice []string, str string, t *testing.T) bool {
+	optionsIndex := -1
+	for i, s := range slice {
+		if s == "-o" {
+			optionsIndex = i + 1
+			break
+		}
+	}
+
+	if optionsIndex < 0 || optionsIndex >= len(slice) {
+		return false
+	}
+
+	return strings.Contains(slice[optionsIndex], str)
+}

mount_test.go

@@ -18,6 +18,7 @@ package mount
 
 import (
 	"reflect"
+	"strings"
 	"testing"
 )
 
@@ -58,3 +59,126 @@ func TestMakeBindOpts(t *testing.T) {
 
 	}
 }
+
+func TestMakeBindOptsSensitive(t *testing.T) {
+	tests := []struct {
+		mountOptions                 []string
+		sensitiveMountOptions        []string
+		isBind                       bool
+		expectedBindOpts             []string
+		expectedRemountOpts          []string
+		expectedSensitiveRemountOpts []string
+	}{
+		{
+			mountOptions:                 []string{"vers=2", "ro", "_netdev"},
+			sensitiveMountOptions:        []string{"user=foo", "pass=bar"},
+			isBind:                       false,
+			expectedBindOpts:             []string{},
+			expectedRemountOpts:          []string{},
+			expectedSensitiveRemountOpts: []string{"user=foo", "pass=bar"},
+		},
+		{
+
+			mountOptions:                 []string{"vers=2", "ro", "_netdev"},
+			sensitiveMountOptions:        []string{"user=foo", "pass=bar", "bind"},
+			isBind:                       true,
+			expectedBindOpts:             []string{"bind", "_netdev"},
+			expectedRemountOpts:          []string{"bind", "remount", "vers=2", "ro", "_netdev"},
+			expectedSensitiveRemountOpts: []string{"user=foo", "pass=bar"},
+		},
+		{
+			mountOptions:                 []string{"vers=2", "remount", "ro", "_netdev"},
+			sensitiveMountOptions:        []string{"user=foo", "pass=bar"},
+			isBind:                       false,
+			expectedBindOpts:             []string{},
+			expectedRemountOpts:          []string{},
+			expectedSensitiveRemountOpts: []string{"user=foo", "pass=bar"},
+		},
+		{
+			mountOptions:                 []string{"vers=2", "ro", "_netdev"},
+			sensitiveMountOptions:        []string{"user=foo", "pass=bar", "remount"},
+			isBind:                       false,
+			expectedBindOpts:             []string{},
+			expectedRemountOpts:          []string{},
+			expectedSensitiveRemountOpts: []string{"user=foo", "pass=bar"},
+		},
+		{
+
+			mountOptions:                 []string{"vers=2", "bind", "ro", "_netdev"},
+			sensitiveMountOptions:        []string{"user=foo", "remount", "pass=bar"},
+			isBind:                       true,
+			expectedBindOpts:             []string{"bind", "_netdev"},
+			expectedRemountOpts:          []string{"bind", "remount", "vers=2", "ro", "_netdev"},
+			expectedSensitiveRemountOpts: []string{"user=foo", "pass=bar"},
+		},
+		{
+
+			mountOptions:                 []string{"vers=2", "bind", "ro", "_netdev"},
+			sensitiveMountOptions:        []string{"user=foo", "remount", "pass=bar"},
+			isBind:                       true,
+			expectedBindOpts:             []string{"bind", "_netdev"},
+			expectedRemountOpts:          []string{"bind", "remount", "vers=2", "ro", "_netdev"},
+			expectedSensitiveRemountOpts: []string{"user=foo", "pass=bar"},
+		},
+	}
+	for _, test := range tests {
+		bind, bindOpts, bindRemountOpts, bindRemountSensitiveOpts := MakeBindOptsSensitive(test.mountOptions, test.sensitiveMountOptions)
+		if bind != test.isBind {
+			t.Errorf("Expected bind to be %v but got %v", test.isBind, bind)
+		}
+		if test.isBind {
+			if !reflect.DeepEqual(test.expectedBindOpts, bindOpts) {
+				t.Errorf("Expected bind mount options to be %+v got %+v", test.expectedBindOpts, bindOpts)
+			}
+			if !reflect.DeepEqual(test.expectedRemountOpts, bindRemountOpts) {
+				t.Errorf("Expected remount options to be %+v got %+v", test.expectedRemountOpts, bindRemountOpts)
+			}
+			if !reflect.DeepEqual(test.expectedSensitiveRemountOpts, bindRemountSensitiveOpts) {
+				t.Errorf("Expected sensitive remount options to be %+v got %+v", test.expectedSensitiveRemountOpts, bindRemountSensitiveOpts)
+			}
+		}
+
+	}
+}
+
+func TestOptionsForLogging(t *testing.T) {
+	// Arrange
+	testcases := []struct {
+		options          []string
+		sensitiveOptions []string
+	}{
+		{
+			options:          []string{"o1", "o2"},
+			sensitiveOptions: []string{"s1"},
+		},
+		{
+			options:          []string{"o1", "o2"},
+			sensitiveOptions: []string{"s1", "s2"},
+		},
+		{
+			sensitiveOptions: []string{"s1", "s2"},
+		},
+		{
+			options: []string{"o1", "o2"},
+		},
+		{},
+	}
+
+	for _, v := range testcases {
+		// Act
+		maskedStr := sanitizedOptionsForLogging(v.options, v.sensitiveOptions)
+
+		// Assert
+		for _, sensitiveOption := range v.sensitiveOptions {
+			if strings.Contains(maskedStr, sensitiveOption) {
+				t.Errorf("Found sensitive log option %q in %q", sensitiveOption, maskedStr)
+			}
+		}
+
+		actualCount := strings.Count(maskedStr, sensitiveOptionsRemoved)
+		expectedCount := len(v.sensitiveOptions)
+		if actualCount != expectedCount {
+			t.Errorf("Found %v instances of %q in %q. Expected %v", actualCount, sensitiveOptionsRemoved, maskedStr, expectedCount)
+		}
+	}
+}

mount_unsupported.go

@@ -43,6 +43,11 @@ func (mounter *Mounter) Mount(source string, target string, fstype string, optio
 	return errUnsupported
 }
 
+// Mount always returns an error on unsupported platforms
+func (mounter *Mounter) MountSensitive(source string, target string, fstype string, options []string, sensitiveOptions []string) error {
+	return errUnsupported
+}
+
 // Unmount always returns an error on unsupported platforms
 func (mounter *Mounter) Unmount(target string) error {
 	return errUnsupported
@@ -63,7 +68,7 @@ func (mounter *Mounter) GetMountRefs(pathname string) ([]string, error) {
 	return nil, errUnsupported
 }
 
-func (mounter *SafeFormatAndMount) formatAndMount(source string, target string, fstype string, options []string) error {
+func (mounter *SafeFormatAndMount) formatAndMountSensitive(source string, target string, fstype string, options []string, sensitiveOptions []string) error {
 	return mounter.Interface.Mount(source, target, fstype, options)
 }
 

mount_windows.go

@@ -53,10 +53,20 @@ var getSMBMountMutex = keymutex.NewHashed(0)
 // Mount : mounts source to target with given options.
 // currently only supports cifs(smb), bind mount(for disk)
 func (mounter *Mounter) Mount(source string, target string, fstype string, options []string) error {
+	return mounter.MountSensitive(source, target, fstype, options, nil /* sensitiveOptions */)
+}
+
+// MountSensitive is the same as Mount() but this method allows
+// sensitiveOptions to be passed in a separate parameter from the normal
+// mount options and ensures the sensitiveOptions are never logged. This
+// method should be used by callers that pass sensitive material (like
+// passwords) as mount options.
+func (mounter *Mounter) MountSensitive(source string, target string, fstype string, options []string, sensitiveOptions []string) error {
 	target = NormalizeWindowsPath(target)
+	sanitizedOptionsForLogging := sanitizedOptionsForLogging(options, sensitiveOptions)
 
 	if source == "tmpfs" {
-		klog.V(3).Infof("mounting source (%q), target (%q), with options (%q)", source, target, options)
+		klog.V(3).Infof("mounting source (%q), target (%q), with options (%q)", source, target, sanitizedOptionsForLogging)
 		return os.MkdirAll(target, 0755)
 	}
 
@@ -66,23 +76,23 @@ func (mounter *Mounter) Mount(source string, target string, fstype string, optio
 	}
 
 	klog.V(4).Infof("mount options(%q) source:%q, target:%q, fstype:%q, begin to mount",
-		options, source, target, fstype)
+		sanitizedOptionsForLogging, source, target, fstype)
 	bindSource := source
 
 	// tell it's going to mount azure disk or azure file according to options
-	if bind, _, _ := MakeBindOpts(options); bind {
+	if bind, _, _, _ := MakeBindOpts(options, sensitiveOptions); bind {
 		// mount azure disk
 		bindSource = NormalizeWindowsPath(source)
 	} else {
 		if len(options) < 2 {
 			klog.Warningf("mount options(%q) command number(%d) less than 2, source:%q, target:%q, skip mounting",
-				options, len(options), source, target)
+				sanitizedOptionsForLogging, len(options), source, target)
 			return nil
 		}
 
 		// currently only cifs mount is supported
 		if strings.ToLower(fstype) != "cifs" {
-			return fmt.Errorf("only cifs mount is supported now, fstype: %q, mounting source (%q), target (%q), with options (%q)", fstype, source, target, options)
+			return fmt.Errorf("only cifs mount is supported now, fstype: %q, mounting source (%q), target (%q), with options (%q)", fstype, source, target, sanitizedOptionsForLogging)
 		}
 
 		// lock smb mount for the same source
@@ -116,7 +126,7 @@ func (mounter *Mounter) Mount(source string, target string, fstype string, optio
 // return (output, error)
 func newSMBMapping(username, password, remotepath string) (string, error) {
 	if username == "" || password == "" || remotepath == "" {
-		return "", fmt.Errorf("invalid parameter(username: %s, password: %s, remoteapth: %s)", username, password, remotepath)
+		return "", fmt.Errorf("invalid parameter(username: %s, password: %s, remoteapth: %s)", username, sensitiveOptionsRemoved, remotepath)
 	}
 
 	// use PowerShell Environment Variables to store user input string to prevent command line injection
@@ -203,7 +213,7 @@ func (mounter *Mounter) GetMountRefs(pathname string) ([]string, error) {
 	return []string{pathname}, nil
 }
 
-func (mounter *SafeFormatAndMount) formatAndMount(source string, target string, fstype string, options []string) error {
+func (mounter *SafeFormatAndMount) formatAndMountSensitive(source string, target string, fstype string, options []string, sensitiveOptions []string) error {
 	// Try to mount the disk
 	klog.V(4).Infof("Attempting to formatAndMount disk: %s %s %s", fstype, source, target)
 

safe_format_and_mount_test.go

@@ -21,6 +21,7 @@ import (
 	"io/ioutil"
 	"os"
 	"runtime"
+	"strings"
 	"testing"
 
 	"k8s.io/utils/exec"
@@ -34,6 +35,10 @@ type ErrorMounter struct {
 }
 
 func (mounter *ErrorMounter) Mount(source string, target string, fstype string, options []string) error {
+	return mounter.MountSensitive(source, target, fstype, options, nil /* sensitiveOptions */)
+}
+
+func (mounter *ErrorMounter) MountSensitive(source string, target string, fstype string, options []string, sensitiveOptions []string) error {
 	i := mounter.errIndex
 	mounter.errIndex++
 	if mounter.err != nil && mounter.err[i] != nil {
@@ -59,12 +64,13 @@ func TestSafeFormatAndMount(t *testing.T) {
 	}
 	defer os.RemoveAll(mntDir)
 	tests := []struct {
-		description  string
-		fstype       string
-		mountOptions []string
-		execScripts  []ExecArgs
-		mountErrs    []error
-		expErrorType MountErrorType
+		description           string
+		fstype                string
+		mountOptions          []string
+		sensitiveMountOptions []string
+		execScripts           []ExecArgs
+		mountErrs             []error
+		expErrorType          MountErrorType
 	}{
 		{
 			description:  "Test a read only mount of an already formatted device",
@@ -234,6 +240,17 @@ func TestSafeFormatAndMount(t *testing.T) {
 			},
 			expErrorType: HasFilesystemErrors,
 		},
+		{
+			description:           "Test that 'blkid' is called and confirms unformatted disk, format fails with sensitive options",
+			fstype:                "ext4",
+			sensitiveMountOptions: []string{"mySecret"},
+			mountErrs:             []error{fmt.Errorf("unknown filesystem type '(null)'")},
+			execScripts: []ExecArgs{
+				{"blkid", []string{"-p", "-s", "TYPE", "-s", "PTTYPE", "-o", "export", "/dev/foo"}, "", &testingexec.FakeExitError{Status: 2}},
+				{"mkfs.ext4", []string{"-F", "-m0", "/dev/foo"}, "", fmt.Errorf("formatting failed")},
+			},
+			expErrorType: FormatFailed,
+		},
 	}
 
 	for _, test := range tests {
@@ -253,7 +270,12 @@ func TestSafeFormatAndMount(t *testing.T) {
 
 		device := "/dev/foo"
 		dest := mntDir
-		err := mounter.FormatAndMount(device, dest, test.fstype, test.mountOptions)
+		var err error
+		if len(test.sensitiveMountOptions) == 0 {
+			err = mounter.FormatAndMount(device, dest, test.fstype, test.mountOptions)
+		} else {
+			err = mounter.FormatAndMountSensitive(device, dest, test.fstype, test.mountOptions, test.sensitiveMountOptions)
+		}
 		if len(test.expErrorType) == 0 {
 			if err != nil {
 				t.Errorf("test \"%s\" unexpected non-error: %v", test.description, err)
@@ -278,6 +300,20 @@ func TestSafeFormatAndMount(t *testing.T) {
 			if mntErr.Type != test.expErrorType {
 				t.Errorf("test \"%s\" unexpected error: \n          [%v]. \nExpecting err type[%v]", test.description, err, test.expErrorType)
 			}
+			if len(test.sensitiveMountOptions) == 0 {
+				if strings.Contains(mntErr.Error(), sensitiveOptionsRemoved) {
+					t.Errorf("test \"%s\" returned an error unexpectedly containing the string %q: %v", test.description, sensitiveOptionsRemoved, err)
+				}
+			} else {
+				if !strings.Contains(err.Error(), sensitiveOptionsRemoved) {
+					t.Errorf("test \"%s\" returned an error without the string %q: %v", test.description, sensitiveOptionsRemoved, err)
+				}
+				for _, sensitiveOption := range test.sensitiveMountOptions {
+					if strings.Contains(err.Error(), sensitiveOption) {
+						t.Errorf("test \"%s\" returned an error with a sensitive string (%q): %v", test.description, sensitiveOption, err)
+					}
+				}
+			}
 		}
 	}
 }