From 19b540081149cf312acbb9c52ad35dba079992f4 Mon Sep 17 00:00:00 2001 From: Caleb Brose <5447118+cmbrose@users.noreply.github.com> Date: Wed, 29 Jun 2022 10:46:44 -0500 Subject: [PATCH] Handle back compat --- pkg/cmd/codespace/ssh.go | 76 +++++++++++++++++++++++---- pkg/cmd/codespace/ssh_test.go | 98 +++++++++++++++++++++++++++++++++++ 2 files changed, 164 insertions(+), 10 deletions(-) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index c869f536e..3072d251b 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -24,6 +24,10 @@ 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 { @@ -48,9 +52,7 @@ func newSSHCmd(app *App) *cobra.Command { 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 (if they haven't already been created) inside the - ~/.ssh directory. Any private keys for which the public key has been uploaded to - your GitHub profile may also be used instead, if desired. + 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 @@ -131,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(automaticPrivateKeyName, "") - 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) } @@ -214,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 @@ -235,6 +237,62 @@ 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 +} + +func checkAndUpdateOldKeyPair(sshContext ssh.Context) *ssh.KeyPair { + publicKeys, err := sshContext.LocalPublicKeys() + if err != nil { + return nil + } + + for _, publicKey := range publicKeys { + if !strings.HasSuffix(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 + + privateKeyNew := strings.Replace(privateKey, automaticPrivateKeyNameOld, automaticPrivateKeyName, -1) + err = os.Rename(privateKey, privateKeyNew) + if err != nil { + return nil + } + + publicKeyNew := strings.Replace(publicKey, automaticPrivateKeyNameOld, automaticPrivateKeyName, -1) + err = os.Rename(publicKey, publicKeyNew) + 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() @@ -390,9 +448,7 @@ func newCpCmd(app *App) *cobra.Command { provided by untrusted users; see for discussion. By default, the 'cp' command will create a public/private ssh key pair to authenticate with - the codespace (if they haven't already been created) inside the ~/.ssh directory. Any private - keys for which the public key has been uploaded to your GitHub profile may also be used - instead, if desired. + the codespace inside the ~/.ssh directory. `, "`"), Example: heredoc.Doc(` $ gh codespace cp -e README.md 'remote:/workspaces/$RepositoryName/' diff --git a/pkg/cmd/codespace/ssh_test.go b/pkg/cmd/codespace/ssh_test.go index 63a8961f3..753772c7d 100644 --- a/pkg/cmd/codespace/ssh_test.go +++ b/pkg/cmd/codespace/ssh_test.go @@ -2,10 +2,14 @@ package codespace import ( "context" + "io/ioutil" + "os" + "path/filepath" "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 +24,100 @@ func TestPendingOperationDisallowsSSH(t *testing.T) { } } +func TestAutomaticSSHKeyPairs(t *testing.T) { + dir := t.TempDir() + + sshContext := ssh.Context{ + ConfigDir: dir, + } + + 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"}, + }, + } + + for _, tt := range tests { + err := os.RemoveAll(dir) + if err != nil { + t.Errorf("Failed to clean test directory: %v", err) + } + os.MkdirAll(dir, 0711) + if err != nil { + t.Errorf("Failed to set up test directory: %v", err) + } + + for _, file := range tt.existingFiles { + if _, err := os.Create(filepath.Join(dir, file)); err != nil { + t.Errorf("Failed to setup test files: %v", err) + } + } + + keyPair, err := setupAutomaticSSHKeys(sshContext) + if err != nil { + t.Errorf("Unexpected error from setupAutomaticSSHKeys: %v", err) + } + if keyPair == nil { + t.Error("Unexpected nil KeyPair from setupAutomaticSSHKeys") + } + + // 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{