diff --git a/pkg/cmd/auth/login/login_test.go b/pkg/cmd/auth/login/login_test.go index f6fbcabd5..c3cde03cf 100644 --- a/pkg/cmd/auth/login/login_test.go +++ b/pkg/cmd/auth/login/login_test.go @@ -147,8 +147,7 @@ func Test_NewCmdLogin(t *testing.T) { t.Run(tt.name, func(t *testing.T) { io, stdin, _, _ := iostreams.Test() f := &cmdutil.Factory{ - IOStreams: io, - Executable: func() string { return "/path/to/gh" }, + IOStreams: io, } io.SetStdoutTTY(true) diff --git a/pkg/cmd/auth/refresh/refresh_test.go b/pkg/cmd/auth/refresh/refresh_test.go index dbdae26af..c8dbebbe5 100644 --- a/pkg/cmd/auth/refresh/refresh_test.go +++ b/pkg/cmd/auth/refresh/refresh_test.go @@ -91,8 +91,7 @@ func Test_NewCmdRefresh(t *testing.T) { t.Run(tt.name, func(t *testing.T) { io, _, _, _ := iostreams.Test() f := &cmdutil.Factory{ - IOStreams: io, - Executable: func() string { return "/path/to/gh" }, + IOStreams: io, } io.SetStdinTTY(tt.tty) io.SetStdoutTTY(tt.tty) diff --git a/pkg/cmd/codespace/common.go b/pkg/cmd/codespace/common.go index 0f6f9cf4e..1107ae6a5 100644 --- a/pkg/cmd/codespace/common.go +++ b/pkg/cmd/codespace/common.go @@ -21,19 +21,25 @@ import ( "golang.org/x/term" ) -type App struct { - io *iostreams.IOStreams - apiClient apiClient - errLogger *log.Logger +type executable interface { + Executable() string } -func NewApp(io *iostreams.IOStreams, apiClient apiClient) *App { +type App struct { + io *iostreams.IOStreams + apiClient apiClient + errLogger *log.Logger + executable executable +} + +func NewApp(io *iostreams.IOStreams, exe executable, apiClient apiClient) *App { errLogger := log.New(io.ErrOut, "", 0) return &App{ - io: io, - apiClient: apiClient, - errLogger: errLogger, + io: io, + apiClient: apiClient, + errLogger: errLogger, + executable: exe, } } @@ -209,8 +215,13 @@ func ask(qs []*survey.Question, response interface{}) error { // checkAuthorizedKeys reports an error if the user has not registered any SSH keys; // see https://github.com/cli/cli/v2/issues/166#issuecomment-921769703. // The check is not required for security but it improves the error message. -func checkAuthorizedKeys(ctx context.Context, client apiClient, user string) error { - keys, err := client.AuthorizedKeys(ctx, user) +func checkAuthorizedKeys(ctx context.Context, client apiClient) error { + user, err := client.GetUser(ctx) + if err != nil { + return fmt.Errorf("error getting user: %w", err) + } + + keys, err := client.AuthorizedKeys(ctx, user.Login) if err != nil { return fmt.Errorf("failed to read GitHub-authorized SSH keys for %s: %w", user, err) } diff --git a/pkg/cmd/codespace/delete_test.go b/pkg/cmd/codespace/delete_test.go index 58090c809..89318b5e3 100644 --- a/pkg/cmd/codespace/delete_test.go +++ b/pkg/cmd/codespace/delete_test.go @@ -190,7 +190,7 @@ func TestDelete(t *testing.T) { io, _, stdout, stderr := iostreams.Test() io.SetStdinTTY(true) io.SetStdoutTTY(true) - app := NewApp(io, apiMock) + app := NewApp(io, nil, apiMock) err := app.Delete(context.Background(), opts) if (err != nil) != tt.wantErr { t.Errorf("delete() error = %v, wantErr %v", err, tt.wantErr) diff --git a/pkg/cmd/codespace/logs.go b/pkg/cmd/codespace/logs.go index 9fc6010b4..d0a0c233b 100644 --- a/pkg/cmd/codespace/logs.go +++ b/pkg/cmd/codespace/logs.go @@ -41,14 +41,9 @@ func (a *App) Logs(ctx context.Context, codespaceName string, follow bool) (err return fmt.Errorf("get or choose codespace: %w", err) } - user, err := a.apiClient.GetUser(ctx) - if err != nil { - return fmt.Errorf("getting user: %w", err) - } - authkeys := make(chan error, 1) go func() { - authkeys <- checkAuthorizedKeys(ctx, a.apiClient, user.Login) + authkeys <- checkAuthorizedKeys(ctx, a.apiClient) }() session, err := codespaces.ConnectToLiveshare(ctx, a, noopLogger(), a.apiClient, codespace) diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 840386d5c..726f2152f 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -4,16 +4,21 @@ package codespace import ( "context" + "errors" "fmt" + "io" "io/ioutil" "log" "net" "os" "path/filepath" "strings" + "sync" + "text/template" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/codespaces" + "github.com/cli/cli/v2/internal/codespaces/api" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/liveshare" "github.com/spf13/cobra" @@ -25,6 +30,8 @@ type sshOptions struct { serverPort int debug bool debugFile string + stdio bool + config bool scpArgs []string // scp arguments, for 'cs cp' (nil for 'cs ssh') } @@ -34,8 +41,57 @@ func newSSHCmd(app *App) *cobra.Command { sshCmd := &cobra.Command{ Use: "ssh [...] [-- ...] []", Short: "SSH into a codespace", + 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. + + The 'ssh' command also supports deeper integration with OpenSSH using a + '--config' option that generates per-codespace ssh configuration in OpenSSH + format. Including this configuration in your ~/.ssh/config improves the user + experience of tools that integrate with OpenSSH, such as bash/zsh completion of + ssh hostnames, remote path completion for scp/rsync/sshfs, git ssh remotes, and + so on. + + Once that is set up (see the second example below), you can ssh to codespaces as + if they were ordinary remote hosts (using 'ssh', not 'gh cs ssh'). + `), + Example: heredoc.Doc(` + $ gh codespace ssh + + $ gh codespace ssh --config > ~/.ssh/codespaces + $ echo 'include ~/.ssh/codespaces' >> ~/.ssh/config' + `), + PreRunE: func(c *cobra.Command, args []string) error { + if opts.stdio { + if opts.codespace == "" { + return errors.New("`--stdio` requires explicit `--codespace`") + } + if opts.config { + return errors.New("cannot use `--stdio` with `--config`") + } + if opts.serverPort != 0 { + return errors.New("cannot use `--stdio` with `--server-port`") + } + if opts.profile != "" { + return errors.New("cannot use `--stdio` with `--profile`") + } + } + if opts.config { + if opts.profile != "" { + return errors.New("cannot use `--config` with `--profile`") + } + if opts.serverPort != 0 { + return errors.New("cannot use `--config` with `--server-port`") + } + } + return nil + }, RunE: func(cmd *cobra.Command, args []string) error { - return app.SSH(cmd.Context(), args, opts) + if opts.config { + return app.printOpenSSHConfig(cmd.Context(), opts) + } else { + return app.SSH(cmd.Context(), args, opts) + } }, DisableFlagsInUseLine: true, } @@ -45,6 +101,11 @@ func newSSHCmd(app *App) *cobra.Command { sshCmd.Flags().StringVarP(&opts.codespace, "codespace", "c", "", "Name of the codespace") sshCmd.Flags().BoolVarP(&opts.debug, "debug", "d", false, "Log debug data to a file") sshCmd.Flags().StringVarP(&opts.debugFile, "debug-file", "", "", "Path of the file log to") + sshCmd.Flags().BoolVarP(&opts.config, "config", "", false, "Write OpenSSH configuration to stdout") + sshCmd.Flags().BoolVar(&opts.stdio, "stdio", false, "Proxy sshd connection to stdio") + if err := sshCmd.Flags().MarkHidden("stdio"); err != nil { + fmt.Fprintf(os.Stderr, "%v\n", err) + } return sshCmd } @@ -55,23 +116,18 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e ctx, cancel := context.WithCancel(ctx) defer cancel() + // While connecting, ensure in the background that the user has keys installed. + // That lets us report a more useful error message if they don't. + authkeys := make(chan error, 1) + go func() { + authkeys <- checkAuthorizedKeys(ctx, a.apiClient) + }() + codespace, err := getOrChooseCodespace(ctx, a.apiClient, opts.codespace) if err != nil { return fmt.Errorf("get or choose codespace: %w", err) } - // TODO(josebalius): We can fetch the user in parallel to everything else - // we should convert this call and others to happen async - user, err := a.apiClient.GetUser(ctx) - if err != nil { - return fmt.Errorf("error getting user: %w", err) - } - - authkeys := make(chan error, 1) - go func() { - authkeys <- checkAuthorizedKeys(ctx, a.apiClient, user.Login) - }() - liveshareLogger := noopLogger() if opts.debug { debugLogger, err := newFileLogger(opts.debugFile) @@ -86,14 +142,13 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e session, err := codespaces.ConnectToLiveshare(ctx, a, liveshareLogger, a.apiClient, codespace) if err != nil { + if authErr := <-authkeys; authErr != nil { + return authErr + } return fmt.Errorf("error connecting to codespace: %w", err) } defer safeClose(session, &err) - if err := <-authkeys; err != nil { - return err - } - a.StartProgressIndicatorWithLabel("Fetching SSH Details") remoteSSHServerPort, sshUser, err := session.StartSSHServer(ctx) a.StopProgressIndicator() @@ -101,6 +156,13 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e return fmt.Errorf("error getting ssh server details: %w", err) } + if opts.stdio { + fwd := liveshare.NewPortForwarder(session, "sshd", remoteSSHServerPort, true) + stdio := newReadWriteCloser(os.Stdin, os.Stdout) + err := fwd.Forward(ctx, stdio) // always non-nil + return fmt.Errorf("tunnel closed: %w", err) + } + localSSHServerPort := opts.serverPort usingCustomPort := localSSHServerPort != 0 // suppress log of command line in Shell @@ -147,6 +209,130 @@ func (a *App) SSH(ctx context.Context, sshArgs []string, opts sshOptions) (err e } } +func (a *App) printOpenSSHConfig(ctx context.Context, opts sshOptions) error { + ctx, cancel := context.WithCancel(ctx) + defer cancel() + + var err error + var csList []*api.Codespace + if opts.codespace == "" { + a.StartProgressIndicatorWithLabel("Fetching codespaces") + csList, err = a.apiClient.ListCodespaces(ctx, -1) + a.StopProgressIndicator() + } else { + var codespace *api.Codespace + codespace, err = getOrChooseCodespace(ctx, a.apiClient, opts.codespace) + csList = []*api.Codespace{codespace} + } + if err != nil { + return fmt.Errorf("error getting codespace info: %w", err) + } + + type sshResult struct { + codespace *api.Codespace + user string // on success, the remote ssh username; else nil + err error + } + + sshUsers := make(chan sshResult, len(csList)) + var wg sync.WaitGroup + var status error + for _, cs := range csList { + if cs.State != "Available" && opts.codespace == "" { + fmt.Fprintf(os.Stderr, "skipping unavailable codespace %s: %s\n", cs.Name, cs.State) + status = cmdutil.SilentError + continue + } + + cs := cs + wg.Add(1) + go func() { + result := sshResult{} + defer wg.Done() + + session, err := codespaces.ConnectToLiveshare(ctx, a, noopLogger(), a.apiClient, cs) + if err != nil { + result.err = fmt.Errorf("error connecting to codespace: %w", err) + } else { + defer session.Close() + + _, result.user, err = session.StartSSHServer(ctx) + if err != nil { + result.err = fmt.Errorf("error getting ssh server details: %w", err) + } else { + result.codespace = cs + } + } + + sshUsers <- result + }() + } + + go func() { + wg.Wait() + close(sshUsers) + }() + + // While the above fetches are running, ensure that the user has keys installed. + // That lets us report a more useful error message if they don't. + if err = checkAuthorizedKeys(ctx, a.apiClient); err != nil { + return err + } + + t, err := template.New("ssh_config").Parse(heredoc.Doc(` + Host cs.{{.Name}}.{{.EscapedRef}} + User {{.SSHUser}} + ProxyCommand {{.GHExec}} cs ssh -c {{.Name}} --stdio + UserKnownHostsFile=/dev/null + StrictHostKeyChecking no + LogLevel quiet + ControlMaster auto + + `)) + if err != nil { + return fmt.Errorf("error formatting template: %w", err) + } + + ghExec := a.executable.Executable() + for result := range sshUsers { + if result.err != nil { + fmt.Fprintf(os.Stderr, "%v\n", result.err) + status = cmdutil.SilentError + continue + } + + // codespaceSSHConfig contains values needed to write an OpenSSH host + // configuration for a single codespace. For example: + // + // Host {{Name}}.{{EscapedRef} + // User {{SSHUser} + // ProxyCommand {{GHExec}} cs ssh -c {{Name}} --stdio + // + // EscapedRef is included in the name to help distinguish between codespaces + // when tab-completing ssh hostnames. '/' characters in EscapedRef are + // flattened to '-' to prevent problems with tab completion or when the + // hostname appears in ControlMaster socket paths. + type codespaceSSHConfig struct { + Name string // the codespace name, passed to `ssh -c` + EscapedRef string // the currently checked-out branch + SSHUser string // the remote ssh username + GHExec string // path used for invoking the current `gh` binary + } + + conf := codespaceSSHConfig{ + Name: result.codespace.Name, + EscapedRef: strings.ReplaceAll(result.codespace.GitStatus.Ref, "/", "-"), + SSHUser: result.user, + GHExec: ghExec, + } + if err := t.Execute(a.io.Out, conf); err != nil { + return err + } + } + + return status +} + type cpOptions struct { sshOptions recursive bool // -r @@ -277,3 +463,21 @@ func (fl *fileLogger) Name() string { func (fl *fileLogger) Close() error { return fl.f.Close() } + +type combinedReadWriteCloser struct { + io.ReadCloser + io.WriteCloser +} + +func newReadWriteCloser(reader io.ReadCloser, writer io.WriteCloser) io.ReadWriteCloser { + return &combinedReadWriteCloser{reader, writer} +} + +func (crwc *combinedReadWriteCloser) Close() error { + werr := crwc.WriteCloser.Close() + rerr := crwc.ReadCloser.Close() + if werr != nil { + return werr + } + return rerr +} diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index 08d93c2be..110dfb4e9 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -5,7 +5,6 @@ import ( "fmt" "net/http" "os" - "path/filepath" "time" "github.com/cli/cli/v2/api" @@ -19,17 +18,10 @@ import ( ) func New(appVersion string) *cmdutil.Factory { - var exe string f := &cmdutil.Factory{ - Config: configFunc(), // No factory dependencies - Branch: branchFunc(), // No factory dependencies - Executable: func() string { - if exe != "" { - return exe - } - exe = executable("gh") - return exe - }, + Config: configFunc(), // No factory dependencies + Branch: branchFunc(), // No factory dependencies + ExecutableName: "gh", } f.IOStreams = ioStreams(f) // Depends on Config @@ -121,52 +113,6 @@ func browserLauncher(f *cmdutil.Factory) string { return os.Getenv("BROWSER") } -// Finds the location of the executable for the current process as it's found in PATH, respecting symlinks. -// If the process couldn't determine its location, return fallbackName. If the executable wasn't found in -// PATH, return the absolute location to the program. -// -// The idea is that the result of this function is callable in the future and refers to the same -// installation of gh, even across upgrades. This is needed primarily for Homebrew, which installs software -// under a location such as `/usr/local/Cellar/gh/1.13.1/bin/gh` and symlinks it from `/usr/local/bin/gh`. -// When the version is upgraded, Homebrew will often delete older versions, but keep the symlink. Because of -// this, we want to refer to the `gh` binary as `/usr/local/bin/gh` and not as its internal Homebrew -// location. -// -// None of this would be needed if we could just refer to GitHub CLI as `gh`, i.e. without using an absolute -// path. However, for some reason Homebrew does not include `/usr/local/bin` in PATH when it invokes git -// commands to update its taps. If `gh` (no path) is being used as git credential helper, as set up by `gh -// auth login`, running `brew update` will print out authentication errors as git is unable to locate -// Homebrew-installed `gh`. -func executable(fallbackName string) string { - exe, err := os.Executable() - if err != nil { - return fallbackName - } - - base := filepath.Base(exe) - path := os.Getenv("PATH") - for _, dir := range filepath.SplitList(path) { - p, err := filepath.Abs(filepath.Join(dir, base)) - if err != nil { - continue - } - f, err := os.Stat(p) - if err != nil { - continue - } - - if p == exe { - return p - } else if f.Mode()&os.ModeSymlink != 0 { - if t, err := os.Readlink(p); err == nil && t == exe { - return p - } - } - } - - return exe -} - func configFunc() func() (config.Config, error) { var cachedConfig config.Config var configError error diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index d393d1a7f..cff797e93 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -135,6 +135,7 @@ func newCodespaceCmd(f *cmdutil.Factory) *cobra.Command { vscsURL := os.Getenv("INTERNAL_VSCS_TARGET_URL") app := codespaceCmd.NewApp( f.IOStreams, + f, codespacesAPI.New( serverURL, apiURL, diff --git a/pkg/cmdutil/factory.go b/pkg/cmdutil/factory.go index 83d4a638f..73cee489a 100644 --- a/pkg/cmdutil/factory.go +++ b/pkg/cmdutil/factory.go @@ -2,6 +2,9 @@ package cmdutil import ( "net/http" + "os" + "path/filepath" + "strings" "github.com/cli/cli/v2/context" "github.com/cli/cli/v2/internal/config" @@ -25,7 +28,59 @@ type Factory struct { Branch func() (string, error) ExtensionManager extensions.ExtensionManager - - // Executable is the path to the currently invoked gh binary - Executable func() string + ExecutableName string +} + +// Executable is the path to the currently invoked binary +func (f *Factory) Executable() string { + if !strings.ContainsRune(f.ExecutableName, os.PathSeparator) { + f.ExecutableName = executable(f.ExecutableName) + } + return f.ExecutableName +} + +// Finds the location of the executable for the current process as it's found in PATH, respecting symlinks. +// If the process couldn't determine its location, return fallbackName. If the executable wasn't found in +// PATH, return the absolute location to the program. +// +// The idea is that the result of this function is callable in the future and refers to the same +// installation of gh, even across upgrades. This is needed primarily for Homebrew, which installs software +// under a location such as `/usr/local/Cellar/gh/1.13.1/bin/gh` and symlinks it from `/usr/local/bin/gh`. +// When the version is upgraded, Homebrew will often delete older versions, but keep the symlink. Because of +// this, we want to refer to the `gh` binary as `/usr/local/bin/gh` and not as its internal Homebrew +// location. +// +// None of this would be needed if we could just refer to GitHub CLI as `gh`, i.e. without using an absolute +// path. However, for some reason Homebrew does not include `/usr/local/bin` in PATH when it invokes git +// commands to update its taps. If `gh` (no path) is being used as git credential helper, as set up by `gh +// auth login`, running `brew update` will print out authentication errors as git is unable to locate +// Homebrew-installed `gh`. +func executable(fallbackName string) string { + exe, err := os.Executable() + if err != nil { + return fallbackName + } + + base := filepath.Base(exe) + path := os.Getenv("PATH") + for _, dir := range filepath.SplitList(path) { + p, err := filepath.Abs(filepath.Join(dir, base)) + if err != nil { + continue + } + f, err := os.Stat(p) + if err != nil { + continue + } + + if p == exe { + return p + } else if f.Mode()&os.ModeSymlink != 0 { + if t, err := os.Readlink(p); err == nil && t == exe { + return p + } + } + } + + return exe }