Merge pull request #21125 from jakecorrenti/single-ssh-key

machine: Use a single ssh key for all machines

cmd/podman/machine/rm.go

@@ -41,9 +41,6 @@ func init() {
 	formatFlagName := "force"
 	flags.BoolVarP(&destroyOptions.Force, formatFlagName, "f", false, "Stop and do not prompt before rming")
 
-	keysFlagName := "save-keys"
-	flags.BoolVar(&destroyOptions.SaveKeys, keysFlagName, false, "Do not delete SSH keys")
-
 	ignitionFlagName := "save-ignition"
 	flags.BoolVar(&destroyOptions.SaveIgnition, ignitionFlagName, false, "Do not delete ignition file")
 

docs/source/locale/ja/LC_MESSAGES/markdown.po

@@ -18478,10 +18478,6 @@ msgstr ""
 msgid "Do not delete the VM image."
 msgstr ""
 
-#: ../../source/markdown/podman-machine-rm.1.md:43
-msgid "**--save-keys**"
-msgstr ""
-
 #: ../../source/markdown/podman-machine-rm.1.md:45
 msgid ""
 "Do not delete the SSH keys for the VM.  The system connection is always "

docs/source/markdown/podman-machine-rm.1.md

@@ -10,7 +10,7 @@ podman\-machine\-rm - Remove a virtual machine
 
 Remove a virtual machine and its related files.  What is actually deleted
 depends on the virtual machine type.  For all virtual machines, the generated
-SSH keys and the podman system connection are deleted.  The ignition files
+podman system connections are deleted.  The ignition files
 generated for that VM are also removed as is its image file on the filesystem.
 
 Users get a display of what is deleted and are required to confirm unless the option `--force`
@@ -39,11 +39,6 @@ Do not delete the generated ignition file.
 
 Do not delete the VM image.
 
-#### **--save-keys**
-
-Do not delete the SSH keys for the VM.  The system connection is always
-deleted.
-
 ## EXAMPLES
 
 Remove a VM named "test1":
@@ -53,8 +48,6 @@ $ podman machine rm test1
 
 The following files will be deleted:
 
-/home/user/.ssh/test1
-/home/user/.ssh/test1.pub
 /home/user/.config/containers/podman/machine/qemu/test1.ign
 /home/user/.local/share/containers/podman/machine/qemu/test1_fedora-coreos-33.20210315.1.0-qemu.x86_64.qcow2
 /home/user/.config/containers/podman/machine/qemu/test1.json

pkg/machine/applehv/machine.go

@@ -22,9 +22,9 @@ import (
 	"github.com/containers/podman/v4/pkg/machine"
 	"github.com/containers/podman/v4/pkg/machine/applehv/vfkit"
 	"github.com/containers/podman/v4/pkg/machine/define"
+	"github.com/containers/podman/v4/pkg/machine/ignition"
 	"github.com/containers/podman/v4/pkg/machine/sockets"
 	"github.com/containers/podman/v4/pkg/machine/vmconfigs"
-    "github.com/containers/podman/v4/pkg/machine/ignition"
 	"github.com/containers/podman/v4/pkg/strongunits"
 	"github.com/containers/podman/v4/pkg/systemd/parser"
 	"github.com/containers/podman/v4/pkg/util"
@@ -202,7 +202,7 @@ func (m *MacMachine) Init(opts machine.InitOptions) (bool, error) {
 		return false, err
 	}
 
-	m.IdentityPath = util.GetIdentityPath(m.Name)
+	m.IdentityPath = util.GetIdentityPath(define.DefaultIdentityName)
 	m.Rootful = opts.Rootful
 	m.RemoteUsername = opts.Username
 
@@ -241,11 +241,10 @@ func (m *MacMachine) Init(opts machine.InitOptions) (bool, error) {
 	}
 
 	if len(opts.IgnitionPath) < 1 {
-		key, err = machine.CreateSSHKeys(m.IdentityPath)
+		key, err = machine.GetSSHKeys(m.IdentityPath)
 		if err != nil {
 			return false, err
 		}
-		callbackFuncs.Add(m.removeSSHKeys)
 	}
 
 	builder := ignition.NewIgnitionBuilder(ignition.DynamicIgnition{
@@ -260,7 +259,8 @@ func (m *MacMachine) Init(opts machine.InitOptions) (bool, error) {
 	})
 
 	if len(opts.IgnitionPath) > 0 {
-		return false, builder.BuildWithIgnitionFile(opts.IgnitionPath)
+		err = builder.BuildWithIgnitionFile(opts.IgnitionPath)
+		return false, err
 	}
 
 	if err := builder.GenerateIgnitionConfig(); err != nil {
@@ -293,13 +293,6 @@ func createReadyUnitFile() (string, error) {
 	return readyUnit.ToString()
 }
 
-func (m *MacMachine) removeSSHKeys() error {
-	if err := os.Remove(fmt.Sprintf("%s.pub", m.IdentityPath)); err != nil {
-		logrus.Error(err)
-	}
-	return os.Remove(m.IdentityPath)
-}
-
 func (m *MacMachine) removeSystemConnections() error {
 	return machine.RemoveConnections(m.Name, fmt.Sprintf("%s-root", m.Name))
 }
@@ -344,9 +337,6 @@ func (m *MacMachine) Inspect() (*machine.InspectInfo, error) {
 // collectFilesToDestroy retrieves the files that will be destroyed by `Remove`
 func (m *MacMachine) collectFilesToDestroy(opts machine.RemoveOptions) []string {
 	files := []string{}
-	if !opts.SaveKeys {
-		files = append(files, m.IdentityPath, m.IdentityPath+".pub")
-	}
 	if !opts.SaveIgnition {
 		files = append(files, m.IgnitionFile.GetPath())
 	}

pkg/machine/config.go

@@ -113,7 +113,6 @@ type StopOptions struct{}
 
 type RemoveOptions struct {
 	Force        bool
-	SaveKeys     bool
 	SaveImage    bool
 	SaveIgnition bool
 }

pkg/machine/define/config.go

@@ -1,3 +1,4 @@
 package define
 
 const UserCertsTargetPath = "/etc/containers/certs.d"
+const DefaultIdentityName = "machine"

pkg/machine/e2e/config_rm_test.go

@@ -5,13 +5,11 @@ type rmMachine struct {
 	  -f, --force           Stop and do not prompt before rming
 	      --save-ignition   Do not delete ignition file
 	      --save-image      Do not delete the image file
-	      --save-keys       Do not delete SSH keys
 
 	*/
 	force        bool
 	saveIgnition bool
 	saveImage    bool
-	saveKeys     bool
 
 	cmd []string
 }
@@ -27,9 +25,6 @@ func (i *rmMachine) buildCmd(m *machineTestBuilder) []string {
 	if i.saveImage {
 		cmd = append(cmd, "--save-image")
 	}
-	if i.saveKeys {
-		cmd = append(cmd, "--save-keys")
-	}
 	cmd = append(cmd, m.name)
 	i.cmd = cmd
 	return cmd
@@ -49,8 +44,3 @@ func (i *rmMachine) withSaveImage() *rmMachine {
 	i.saveImage = true
 	return i
 }
-
-func (i *rmMachine) withSaveKeys() *rmMachine {
-	i.saveKeys = true
-	return i
-}

pkg/machine/e2e/init_test.go

@@ -303,7 +303,7 @@ var _ = Describe("podman machine init", func() {
 		img := inspectSession.outputToString()
 
 		rm := rmMachine{}
-		removeSession, err := mb.setCmd(rm.withForce().withSaveKeys()).run()
+		removeSession, err := mb.setCmd(rm.withForce()).run()
 		Expect(err).ToNot(HaveOccurred())
 		Expect(removeSession).To(Exit(0))
 
@@ -313,20 +313,21 @@ var _ = Describe("podman machine init", func() {
 		Expect(err).ToNot(HaveOccurred())
 		Expect(ec).To(Equal(125))
 
-		// Clashing keys - init fails
-		i = new(initMachine)
-		session, err = mb.setName(name).setCmd(i.withImagePath(mb.imagePath)).run()
-		Expect(err).ToNot(HaveOccurred())
-		Expect(session).To(Exit(125))
-
-		// ensure files created by init are cleaned up on init failure
-		_, err = os.Stat(img)
-		Expect(err).To(HaveOccurred())
-		_, err = os.Stat(cfgpth)
-		Expect(err).To(HaveOccurred())
-
 		// WSL does not use ignition
 		if testProvider.VMType() != define.WSLVirt {
+			// Bad ignition path - init fails
+			i = new(initMachine)
+			i.ignitionPath = "/bad/path"
+			session, err = mb.setName(name).setCmd(i.withImagePath(mb.imagePath)).run()
+			Expect(err).ToNot(HaveOccurred())
+			Expect(session).To(Exit(125))
+
+			// ensure files created by init are cleaned up on init failure
+			_, err = os.Stat(img)
+			Expect(err).To(HaveOccurred())
+			_, err = os.Stat(cfgpth)
+			Expect(err).To(HaveOccurred())
+
 			_, err = os.Stat(ign)
 			Expect(err).To(HaveOccurred())
 		}

pkg/machine/e2e/rm_test.go

@@ -3,6 +3,7 @@ package e2e_test
 import (
 	"fmt"
 	"os"
+	"path/filepath"
 
 	"github.com/containers/podman/v4/pkg/machine/define"
 	. "github.com/onsi/ginkgo/v2"
@@ -80,7 +81,7 @@ var _ = Describe("podman machine rm", func() {
 		Expect(ec).To(Equal(125))
 	})
 
-	It("machine rm --save-keys, --save-ignition, --save-image", func() {
+	It("machine rm --save-ignition --save-image", func() {
 		i := new(initMachine)
 		session, err := mb.setCmd(i.withImagePath(mb.imagePath)).run()
 		Expect(err).ToNot(HaveOccurred())
@@ -104,7 +105,7 @@ var _ = Describe("podman machine rm", func() {
 		img := inspectSession.outputToString()
 
 		rm := rmMachine{}
-		removeSession, err := mb.setCmd(rm.withForce().withSaveIgnition().withSaveImage().withSaveKeys()).run()
+		removeSession, err := mb.setCmd(rm.withForce().withSaveIgnition().withSaveImage()).run()
 		Expect(err).ToNot(HaveOccurred())
 		Expect(removeSession).To(Exit(0))
 
@@ -127,4 +128,71 @@ var _ = Describe("podman machine rm", func() {
 		_, err = os.Stat(img)
 		Expect(err).ToNot(HaveOccurred())
 	})
+
+	It("Remove machine sharing ssh key with another machine", func() {
+		expectedIdentityPathSuffix := filepath.Join(".local", "share", "containers", "podman", "machine", define.DefaultIdentityName)
+
+		fooName := "foo"
+		foo := new(initMachine)
+		session, err := mb.setName(fooName).setCmd(foo.withImagePath(mb.imagePath)).run()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(session).To(Exit(0))
+
+		barName := "bar"
+		bar := new(initMachine)
+		session, err = mb.setName(barName).setCmd(bar.withImagePath(mb.imagePath).withNow()).run()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(session).To(Exit(0))
+
+		inspectFoo := new(inspectMachine)
+		inspectFoo = inspectFoo.withFormat("{{.SSHConfig.IdentityPath}}")
+		inspectSession, err := mb.setName(fooName).setCmd(inspectFoo).run()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(inspectSession).To(Exit(0))
+		Expect(inspectSession.outputToString()).To(ContainSubstring(expectedIdentityPathSuffix))
+		fooIdentityPath := inspectSession.outputToString()
+
+		inspectBar := new(inspectMachine)
+		inspectBar = inspectBar.withFormat("{{.SSHConfig.IdentityPath}}")
+		inspectSession, err = mb.setName(barName).setCmd(inspectBar).run()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(inspectSession).To(Exit(0))
+		Expect(inspectSession.outputToString()).To(Equal(fooIdentityPath))
+
+		rmFoo := new(rmMachine)
+		stop, err := mb.setName(fooName).setCmd(rmFoo.withForce()).run()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(stop).To(Exit(0))
+
+		// removal of foo should not affect the ability to ssh into the bar machine
+		sshBar := new(sshMachine)
+		sshSession, err := mb.setName(barName).setCmd(sshBar.withSSHCommand([]string{"echo", "foo"})).run()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(sshSession).To(Exit(0))
+	})
+
+	It("Removing all machines doesn't delete ssh keys", func() {
+		fooName := "foo"
+		foo := new(initMachine)
+		session, err := mb.setName(fooName).setCmd(foo.withImagePath(mb.imagePath)).run()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(session).To(Exit(0))
+
+		inspectFoo := new(inspectMachine)
+		inspectFoo = inspectFoo.withFormat("{{.SSHConfig.IdentityPath}}")
+		inspectSession, err := mb.setName(fooName).setCmd(inspectFoo).run()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(inspectSession).To(Exit(0))
+		fooIdentityPath := inspectSession.outputToString()
+
+		rmFoo := new(rmMachine)
+		stop, err := mb.setName(fooName).setCmd(rmFoo.withForce()).run()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(stop).To(Exit(0))
+
+		_, err = os.Stat(fooIdentityPath)
+		Expect(err).ToNot(HaveOccurred())
+		_, err = os.Stat(fooIdentityPath + ".pub")
+		Expect(err).ToNot(HaveOccurred())
+	})
 })

pkg/machine/hyperv/machine.go

@@ -21,8 +21,8 @@ import (
 	"github.com/containers/podman/v4/pkg/machine"
 	"github.com/containers/podman/v4/pkg/machine/define"
 	"github.com/containers/podman/v4/pkg/machine/hyperv/vsock"
-	"github.com/containers/podman/v4/pkg/machine/vmconfigs"
 	"github.com/containers/podman/v4/pkg/machine/ignition"
+	"github.com/containers/podman/v4/pkg/machine/vmconfigs"
 	"github.com/containers/podman/v4/pkg/strongunits"
 	"github.com/containers/podman/v4/pkg/systemd/parser"
 	"github.com/containers/podman/v4/pkg/util"
@@ -178,7 +178,7 @@ func (m *HyperVMachine) Init(opts machine.InitOptions) (bool, error) {
 		return nil
 	})
 
-	m.IdentityPath = util.GetIdentityPath(m.Name)
+	m.IdentityPath = util.GetIdentityPath(define.DefaultIdentityName)
 
 	if m.UID == 0 {
 		m.UID = 1000
@@ -205,11 +205,10 @@ func (m *HyperVMachine) Init(opts machine.InitOptions) (bool, error) {
 	callbackFuncs.Add(m.removeSystemConnections)
 
 	if len(opts.IgnitionPath) < 1 {
-		key, err = machine.CreateSSHKeys(m.IdentityPath)
+		key, err = machine.GetSSHKeys(m.IdentityPath)
 		if err != nil {
 			return false, err
 		}
-		callbackFuncs.Add(m.removeSSHKeys)
 	}
 
 	m.ResourceConfig = vmconfigs.ResourceConfig{
@@ -233,7 +232,8 @@ func (m *HyperVMachine) Init(opts machine.InitOptions) (bool, error) {
 	// If the user provides an ignition file, we need to
 	// copy it into the conf dir
 	if len(opts.IgnitionPath) > 0 {
-		return false, builder.BuildWithIgnitionFile(opts.IgnitionPath)
+		err = builder.BuildWithIgnitionFile(opts.IgnitionPath)
+		return false, err
 	}
 	callbackFuncs.Add(m.IgnitionFile.Delete)
 
@@ -319,13 +319,6 @@ func (m *HyperVMachine) unregisterMachine() error {
 	return vm.Remove("")
 }
 
-func (m *HyperVMachine) removeSSHKeys() error {
-	if err := os.Remove(fmt.Sprintf("%s.pub", m.IdentityPath)); err != nil {
-		logrus.Error(err)
-	}
-	return os.Remove(m.IdentityPath)
-}
-
 func (m *HyperVMachine) removeSystemConnections() error {
 	return machine.RemoveConnections(m.Name, fmt.Sprintf("%s-root", m.Name))
 }
@@ -378,10 +371,6 @@ func (m *HyperVMachine) Inspect() (*machine.InspectInfo, error) {
 // collectFilesToDestroy retrieves the files that will be destroyed by `Remove`
 func (m *HyperVMachine) collectFilesToDestroy(opts machine.RemoveOptions, diskPath *string) []string {
 	files := []string{}
-
-	if !opts.SaveKeys {
-		files = append(files, m.IdentityPath, m.IdentityPath+".pub")
-	}
 	if !opts.SaveIgnition {
 		files = append(files, m.IgnitionFile.GetPath())
 	}

pkg/machine/keys.go

@@ -36,6 +36,20 @@ func CreateSSHKeys(writeLocation string) (string, error) {
 	return strings.TrimSuffix(string(b), "\n"), nil
 }
 
+// GetSSHKeys checks to see if there is a ssh key at the provided location.
+// If not, we create the priv and pub keys. The ssh key is then returned.
+func GetSSHKeys(identityPath string) (string, error) {
+	if _, err := os.Stat(identityPath); err == nil {
+		b, err := os.ReadFile(identityPath + ".pub")
+		if err != nil {
+			return "", err
+		}
+		return strings.TrimSuffix(string(b), "\n"), nil
+	}
+
+	return CreateSSHKeys(identityPath)
+}
+
 func CreateSSHKeysPrefix(identityPath string, passThru bool, skipExisting bool, prefix ...string) (string, error) {
 	_, e := os.Stat(identityPath)
 	if !skipExisting || errors.Is(e, os.ErrNotExist) {