start: Simpler and safer mount on start

The --mount-string argument defaults to `/Users` on darwin, and
homedir.Homedir() on other platforms (e.g. $HOME on unix).

This is wrong in many ways:

- `/Users` is not HOME on darwin (the right path is `/Users/$USER`).
  Using the default mount we cannot access anything inside the guest in
  the user home directory.  We can access the special `/Users/Shared`
  directory, but this should not be a default mount.

- Mounting the user home directory inside the guest in read-write mode
  is a horrible default. This exposes the users private keys in .ssh/ to
  the guest, any sensitive files in the user home directory, and allows
  the guest to change any file on the host.

- Using the `--mount` option mount the default mount directory silently.
  This is unexpected, surprising, and not documented in the minikube
  handbook[1].

Example access to user private key from the guest with the default
mount:

    $ minikube start --mount

    $ minikube ssh cat /minikube-host/.ssh/id_ed25519
    -----BEGIN OPENSSH PRIVATE KEY-----
    ...
    -----END OPENSSH PRIVATE KEY-----

Fixed by removing the default mount directory and changing mount logic
to check for non-empty mount-string instead of the mount flag.

The mount flag is kept for backward compatibility, but its value is
ignored. In the next release we want to use this flag for supporting
multiple mounts.

Example usage before:

    minikube start --mount --mount-string ~/models:/mnt/models

Example usage after:

    minikube start --mount-string ~/models:/mnt/models

Breaking changes:

User depending the default mount will have to replace the command:

    minikube start --mount

With:

    minikube start  --mount-string $HOME:/minikube-host

[1] https://minikube.sigs.k8s.io/docs/handbook/mount/

Author: nirs

cmd/minikube/cmd/start.go

@@ -268,7 +268,7 @@ func runStart(cmd *cobra.Command, _ []string) {
 
 	validateBuiltImageVersion(starter.Runner, ds.Name)
 
-	if existing != nil && driver.IsKIC(existing.Driver) && viper.GetBool(createMount) {
+	if existing != nil && driver.IsKIC(existing.Driver) && viper.GetString(mountString) != "" {
 		old := ""
 		if len(existing.ContainerVolumeMounts) > 0 {
 			old = existing.ContainerVolumeMounts[0]

cmd/minikube/cmd/start_flags.go

@@ -173,8 +173,8 @@ func initMinikubeFlags() {
 	startCmd.Flags().Bool(keepContext, false, "This will keep the existing kubectl context and will create a minikube context.")
 	startCmd.Flags().Bool(embedCerts, false, "if true, will embed the certs in kubeconfig.")
 	startCmd.Flags().StringP(containerRuntime, "c", constants.DefaultContainerRuntime, fmt.Sprintf("The container runtime to be used. Valid options: %s (default: auto)", strings.Join(cruntime.ValidRuntimes(), ", ")))
-	startCmd.Flags().Bool(createMount, false, "This will start the mount daemon and automatically mount files into minikube.")
-	startCmd.Flags().String(mountString, constants.DefaultMountDir+":/minikube-host", "The argument to pass the minikube mount command on start.")
+	startCmd.Flags().Bool(createMount, false, "Kept for backward compatibility, value is ignored.")
+	startCmd.Flags().String(mountString, "", "Directory to mount in the guest using format '/host-path:/guest-path'.")
 	startCmd.Flags().String(mount9PVersion, defaultMount9PVersion, mount9PVersionDescription)
 	startCmd.Flags().String(mountGID, defaultMountGID, mountGIDDescription)
 	startCmd.Flags().String(mountIPFlag, defaultMountIP, mountIPDescription)
@@ -614,7 +614,6 @@ func generateNewConfigFromFlags(cmd *cobra.Command, k8sVersion string, rtime str
 		SSHPort:                 viper.GetInt(sshSSHPort),
 		ExtraDisks:              viper.GetInt(extraDisks),
 		CertExpiration:          viper.GetDuration(certExpiration),
-		Mount:                   viper.GetBool(createMount),
 		MountString:             viper.GetString(mountString),
 		Mount9PVersion:          viper.GetString(mount9PVersion),
 		MountGID:                viper.GetString(mountGID),
@@ -655,7 +654,8 @@ func generateNewConfigFromFlags(cmd *cobra.Command, k8sVersion string, rtime str
 		AutoPauseInterval:  viper.GetDuration(autoPauseInterval),
 	}
 	cc.VerifyComponents = interpretWaitFlag(*cmd)
-	if viper.GetBool(createMount) && driver.IsKIC(drvName) {
+
+	if viper.GetString(mountString) != "" && driver.IsKIC(drvName) {
 		cc.ContainerVolumeMounts = []string{viper.GetString(mountString)}
 	}
 
@@ -867,7 +867,6 @@ func updateExistingConfigFromFlags(cmd *cobra.Command, existing *config.ClusterC
 	updateStringFromFlag(cmd, &cc.KubernetesConfig.ServiceCIDR, serviceCIDR)
 	updateBoolFromFlag(cmd, &cc.KubernetesConfig.ShouldLoadCachedImages, cacheImages)
 	updateDurationFromFlag(cmd, &cc.CertExpiration, certExpiration)
-	updateBoolFromFlag(cmd, &cc.Mount, createMount)
 	updateStringFromFlag(cmd, &cc.MountString, mountString)
 	updateStringFromFlag(cmd, &cc.Mount9PVersion, mount9PVersion)
 	updateStringFromFlag(cmd, &cc.MountGID, mountGID)

pkg/minikube/config/types.go

@@ -88,7 +88,6 @@ type ClusterConfig struct {
 	MultiNodeRequested      bool
 	ExtraDisks              int // currently only implemented for hyperkit and kvm2
 	CertExpiration          time.Duration
-	Mount                   bool
 	MountString             string
 	Mount9PVersion          string
 	MountGID                string

pkg/minikube/constants/constants_darwin.go

@@ -73,7 +73,7 @@ func configureMounts(wg *sync.WaitGroup, cc config.ClusterConfig) {
 	wg.Add(1)
 	defer wg.Done()
 
-	if !cc.Mount || driver.IsKIC(cc.Driver) {
+	if cc.MountString == "" || driver.IsKIC(cc.Driver) {
 		return
 	}
 

pkg/minikube/constants/constants_freebsd.go

@@ -32,6 +32,7 @@ const (
 	mountGID   = "0"
 	mountMSize = "6543"
 	mountUID   = "0"
+	guestPath  = "/minikube-host"
 )
 
 var mountStartPort = 46463
@@ -55,14 +56,19 @@ func TestMountStart(t *testing.T) {
 
 	// Serial tests
 	t.Run("serial", func(t *testing.T) {
+		hostPath := t.TempDir()
+		startProfileWithMount := func(ctx context.Context, t *testing.T, profile string) {
+			validateStartWithMount(ctx, t, profile, hostPath)
+		}
+
 		tests := []struct {
 			name      string
 			validator validateFunc
 			profile   string
 		}{
-			{"StartWithMountFirst", validateStartWithMount, profile1},
+			{"StartWithMountFirst", startProfileWithMount, profile1},
 			{"VerifyMountFirst", validateMount, profile1},
-			{"StartWithMountSecond", validateStartWithMount, profile2},
+			{"StartWithMountSecond", startProfileWithMount, profile2},
 			{"VerifyMountSecond", validateMount, profile2},
 			{"DeleteFirst", validateDelete, profile1},
 			{"VerifyMountPostDelete", validateMount, profile2},
@@ -87,13 +93,23 @@ func TestMountStart(t *testing.T) {
 }
 
 // validateStartWithMount starts a cluster with mount enabled
-func validateStartWithMount(ctx context.Context, t *testing.T, profile string) {
+func validateStartWithMount(ctx context.Context, t *testing.T, profile string, hostPath string) {
 	defer PostMortemLogs(t, profile)
 
 	// We have to increment this because if you have two mounts with the same port, when you kill one cluster the mount will break for the other
 	mountStartPort++
 
-	args := []string{"start", "-p", profile, "--memory=3072", "--mount", "--mount-gid", mountGID, "--mount-msize", mountMSize, "--mount-port", mountPort(), "--mount-uid", mountUID, "--no-kubernetes"}
+	args := []string{
+		"start",
+		"-p", profile,
+		"--memory=3072",
+		"--mount-string", fmt.Sprintf("%s:%s", hostPath, guestPath),
+		"--mount-gid", mountGID,
+		"--mount-msize", mountMSize,
+		"--mount-port", mountPort(),
+		"--mount-uid", mountUID,
+		"--no-kubernetes",
+	}
 	args = append(args, StartArgs()...)
 	rr, err := Run(t, exec.CommandContext(ctx, Target(), args...))
 	if err != nil {
@@ -110,7 +126,7 @@ func validateMount(ctx context.Context, t *testing.T, profile string) {
 	sshArgs := []string{"-p", profile, "ssh", "--"}
 
 	args := sshArgs
-	args = append(args, "ls", "/minikube-host")
+	args = append(args, "ls", guestPath)
 	rr, err := Run(t, exec.CommandContext(ctx, Target(), args...))
 	if err != nil {
 		t.Fatalf("mount failed: %q : %v", rr.Command(), err)