Merge pull request #5859 from cli/cmbrose/fix-auto-key-path

Use `codespaces.auto` instead for the automatic ssh keys

pkg/cmd/codespace/ssh.go

@@ -24,6 +24,12 @@ import (
 	"github.com/spf13/cobra"
 )
 
+// In 2.13.0 these commands started automatically generating key pairs named 'codespaces' and 'codespaces.pub'
+// which could collide with suggested the ssh config also named 'codespaces'. We now use 'codespaces.auto'
+// and 'codespaces.auto.pub' in order to avoid that collision.
+const automaticPrivateKeyNameOld = "codespaces"
+const automaticPrivateKeyName = "codespaces.auto"
+
 type sshOptions struct {
 	codespace  string
 	profile    string
@@ -44,6 +50,9 @@ func newSSHCmd(app *App) *cobra.Command {
 		Long: heredoc.Doc(`
 			The 'ssh' command is used to SSH into a codespace. In its simplest form, you can
 			run 'gh cs ssh', select a codespace interactively, and connect.
+			
+			By default, the 'ssh' command will create a public/private ssh key pair to  
+			authenticate with the codespace inside the ~/.ssh directory.
 
 			The 'ssh' command also supports deeper integration with OpenSSH using a
 			'--config' option that generates per-codespace ssh configuration in OpenSSH
@@ -124,9 +133,9 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e
 	sshContext := ssh.Context{}
 	startSSHOptions := liveshare.StartSSHServerOptions{}
 
-	if shouldGenerateSSHKeys(args, opts) && sshContext.HasKeygen() {
-		keyPair, err := sshContext.GenerateSSHKey("codespaces", "")
-		if err != nil && !errors.Is(err, ssh.ErrKeyAlreadyExists) {
+	if shouldUseAutomaticSSHKeys(args, opts) {
+		keyPair, err := setupAutomaticSSHKeys(sshContext)
+		if err != nil {
 			return fmt.Errorf("failed to generate ssh keys: %w", err)
 		}
 
@@ -207,7 +216,7 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e
 	}
 }
 
-func shouldGenerateSSHKeys(args []string, opts sshOptions) bool {
+func shouldUseAutomaticSSHKeys(args []string, opts sshOptions) bool {
 	if opts.profile != "" {
 		// The profile may specify the identity file so cautiously don't override anything with that option
 		return false
@@ -228,6 +237,67 @@ func shouldGenerateSSHKeys(args []string, opts sshOptions) bool {
 	return true
 }
 
+func setupAutomaticSSHKeys(sshContext ssh.Context) (*ssh.KeyPair, error) {
+	keyPair := checkAndUpdateOldKeyPair(sshContext)
+	if keyPair != nil {
+		return keyPair, nil
+	}
+
+	keyPair, err := sshContext.GenerateSSHKey(automaticPrivateKeyName, "")
+	if err != nil && !errors.Is(err, ssh.ErrKeyAlreadyExists) {
+		return nil, err
+	}
+
+	return keyPair, nil
+}
+
+// checkAndUpdateOldKeyPair handles backward compatibility with the old keypair names.
+// If the old public and private keys both exist they are renamed to the new name.
+// The return value is non-nil only if the rename happens.
+func checkAndUpdateOldKeyPair(sshContext ssh.Context) *ssh.KeyPair {
+	publicKeys, err := sshContext.LocalPublicKeys()
+	if err != nil {
+		return nil
+	}
+
+	for _, publicKey := range publicKeys {
+		if filepath.Base(publicKey) != automaticPrivateKeyNameOld+".pub" {
+			continue
+		}
+
+		privateKey := strings.TrimSuffix(publicKey, ".pub")
+		_, err := os.Stat(privateKey)
+		if err != nil {
+			continue
+		}
+
+		// Both old public and private keys exist, rename them to the new name
+
+		sshDir := filepath.Dir(publicKey)
+
+		publicKeyNew := filepath.Join(sshDir, automaticPrivateKeyName+".pub")
+		err = os.Rename(publicKey, publicKeyNew)
+		if err != nil {
+			return nil
+		}
+
+		privateKeyNew := filepath.Join(sshDir, automaticPrivateKeyName)
+		err = os.Rename(privateKey, privateKeyNew)
+		if err != nil {
+			return nil
+		}
+
+		keyPair := &ssh.KeyPair{
+			PublicKeyPath:  publicKeyNew,
+			PrivateKeyPath: privateKeyNew,
+		}
+
+		return keyPair
+	}
+
+	return nil
+}
+
 func (a *App) printOpenSSHConfig(ctx context.Context, opts sshOptions) (err error) {
 	ctx, cancel := context.WithCancel(ctx)
 	defer cancel()
@@ -381,6 +451,9 @@ func newCpCmd(app *App) *cobra.Command {
 			be evaluated on the remote machine, subject to expansion of tildes, braces, globs,
 			environment variables, and backticks. For security, do not use this flag with arguments
 			provided by untrusted users; see <https://lwn.net/Articles/835962/> for discussion.
+			
+			By default, the 'cp' command will create a public/private ssh key pair to authenticate with 
+			the codespace inside the ~/.ssh directory.
 		`, "`"),
 		Example: heredoc.Doc(`
 			$ gh codespace cp -e README.md 'remote:/workspaces/$RepositoryName/'

pkg/cmd/codespace/ssh_test.go

@@ -2,10 +2,15 @@ package codespace
 
 import (
 	"context"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/cli/cli/v2/internal/codespaces/api"
 	"github.com/cli/cli/v2/pkg/iostreams"
+	"github.com/cli/cli/v2/pkg/ssh"
 )
 
 func TestPendingOperationDisallowsSSH(t *testing.T) {
@@ -20,6 +25,105 @@ func TestPendingOperationDisallowsSSH(t *testing.T) {
 	}
 }
 
+func TestAutomaticSSHKeyPairs(t *testing.T) {
+	tests := []struct {
+		// These files exist when calling setupAutomaticSSHKeys
+		existingFiles []string
+		// These files should exist after setupAutomaticSSHKeys finishes
+		wantFinalFiles []string
+	}{
+		// Basic case: no existing keys, they should be created
+		{
+			nil,
+			[]string{automaticPrivateKeyName, automaticPrivateKeyName + ".pub"},
+		},
+		// Basic case: keys already exist
+		{
+			[]string{automaticPrivateKeyName, automaticPrivateKeyName + ".pub"},
+			[]string{automaticPrivateKeyName, automaticPrivateKeyName + ".pub"},
+		},
+		// Backward compatibility: both old keys exist, they should be renamed
+		{
+			[]string{automaticPrivateKeyNameOld, automaticPrivateKeyNameOld + ".pub"},
+			[]string{automaticPrivateKeyName, automaticPrivateKeyName + ".pub"},
+		},
+		// Backward compatibility: old private key exists but not the public key, the new keys should be created
+		{
+			[]string{automaticPrivateKeyNameOld},
+			[]string{automaticPrivateKeyNameOld, automaticPrivateKeyName, automaticPrivateKeyName + ".pub"},
+		},
+		// Backward compatibility: old public key exists but not the private key, the new keys should be created
+		{
+			[]string{automaticPrivateKeyNameOld + ".pub"},
+			[]string{automaticPrivateKeyNameOld + ".pub", automaticPrivateKeyName, automaticPrivateKeyName + ".pub"},
+		},
+		// Backward compatibility (edge case): files exist which contains old key name as a substring, the new keys should be created
+		{
+			[]string{"foo" + automaticPrivateKeyNameOld + ".pub", "foo" + automaticPrivateKeyNameOld},
+			[]string{"foo" + automaticPrivateKeyNameOld + ".pub", "foo" + automaticPrivateKeyNameOld, automaticPrivateKeyName, automaticPrivateKeyName + ".pub"},
+		},
+	}
+
+	for _, tt := range tests {
+		dir := t.TempDir()
+
+		sshContext := ssh.Context{
+			ConfigDir: dir,
+		}
+
+		for _, file := range tt.existingFiles {
+			f, err := os.Create(filepath.Join(dir, file))
+			if err != nil {
+				t.Errorf("Failed to setup test files: %v", err)
+			}
+			// If the file isn't closed here windows will have errors about file already in use
+			f.Close()
+		}
+
+		keyPair, err := setupAutomaticSSHKeys(sshContext)
+		if err != nil {
+			t.Errorf("Unexpected error from setupAutomaticSSHKeys: %v", err)
+		}
+		if keyPair == nil {
+			t.Fatal("Unexpected nil KeyPair from setupAutomaticSSHKeys")
+		}
+		if !strings.HasSuffix(keyPair.PrivateKeyPath, automaticPrivateKeyName) {
+			t.Errorf("Expected private key path %v, got %v", automaticPrivateKeyName, keyPair.PrivateKeyPath)
+		}
+		if !strings.HasSuffix(keyPair.PublicKeyPath, automaticPrivateKeyName+".pub") {
+			t.Errorf("Expected public key path %v, got %v", automaticPrivateKeyName+".pub", keyPair.PublicKeyPath)
+		}
+
+		// Check that all the expected files are present
+		for _, file := range tt.wantFinalFiles {
+			if _, err := os.Stat(filepath.Join(dir, file)); err != nil {
+				t.Errorf("Want file %q to exist after setupAutomaticSSHKeys but it doesn't", file)
+			}
+		}
+
+		// Check that no unexpected files are present
+		allExistingFiles, err := ioutil.ReadDir(dir)
+		if err != nil {
+			t.Errorf("Failed to list files in test directory: %v", err)
+		}
+		for _, file := range allExistingFiles {
+			filename := file.Name()
+			isWantedFile := false
+			for _, wantedFile := range tt.wantFinalFiles {
+				if filename == wantedFile {
+					isWantedFile = true
+					break
+				}
+			}
+
+			if !isWantedFile {
+				t.Errorf("Unexpected file %q exists after setupAutomaticSSHKeys", filename)
+			}
+		}
+	}
+
+}
+
 func testingSSHApp() *App {
 	user := &api.User{Login: "monalisa"}
 	disabledCodespace := &api.Codespace{