From 8dd4fc79dc840e09afff303eb7b210adb7306168 Mon Sep 17 00:00:00 2001 From: saad-ali Date: Fri, 7 Feb 2020 17:43:22 -0800 Subject: [PATCH] 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 | 11 ++- mount.go | 80 ++++++++++++++++++++-- mount_linux.go | 113 ++++++++++++++++++++++--------- mount_linux_test.go | 77 +++++++++++++++++++++ mount_test.go | 124 ++++++++++++++++++++++++++++++++++ mount_unsupported.go | 7 +- mount_windows.go | 24 +++++-- safe_format_and_mount_test.go | 50 ++++++++++++-- 8 files changed, 433 insertions(+), 53 deletions(-) diff --git a/fake_mounter.go b/fake_mounter.go index d3a82f415c2..8e690bbfc28 100644 --- a/fake_mounter.go +++ b/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 diff --git a/mount.go b/mount.go index 46d77327fbf..112d7ebc178 100644 --- a/mount.go +++ b/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 = "" ) // 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,, +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 +} diff --git a/mount_linux.go b/mount_linux.go index 4dc696d69b0..2d24af913e4 100644 --- a/mount_linux.go +++ b/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{ diff --git a/mount_linux_test.go b/mount_linux_test.go index 47b6e31a04c..dc7d4fd882f 100644 --- a/mount_linux_test.go +++ b/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) +} diff --git a/mount_test.go b/mount_test.go index 6b9d5d5fc4b..041a28dae00 100644 --- a/mount_test.go +++ b/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) + } + } +} diff --git a/mount_unsupported.go b/mount_unsupported.go index d13ec5c34d5..985edbe3d56 100644 --- a/mount_unsupported.go +++ b/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) } diff --git a/mount_windows.go b/mount_windows.go index 9371ff43551..e9fad2b47fa 100644 --- a/mount_windows.go +++ b/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) diff --git a/safe_format_and_mount_test.go b/safe_format_and_mount_test.go index a0869293656..782d3da67e7 100644 --- a/safe_format_and_mount_test.go +++ b/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) + } + } + } } } }