From c2f3537a322e25b8ffdfcd6ab31b9081f8695995 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 17 Sep 2021 16:26:20 +0200 Subject: [PATCH 1/8] Separate "main" package from "ghcs" package To make "ghcs" importable, this separates out the `main()` function into its own package that lives under "cmd/ghcs/main". Typically the main package would be called "cmd/ghcs", but we wanted to leave the current ghcs implementation where it is to avoid causing conflicts with current work in progress. Co-authored-by: Jose Garcia --- cmd/ghcs/code.go | 6 +----- cmd/ghcs/common.go | 2 +- cmd/ghcs/create.go | 6 +----- cmd/ghcs/delete.go | 6 +----- cmd/ghcs/list.go | 6 +----- cmd/ghcs/logs.go | 6 +----- cmd/ghcs/main/main.go | 26 ++++++++++++++++++++++++++ cmd/ghcs/ports.go | 6 +----- cmd/ghcs/{main.go => root.go} | 35 ++++++++++++++--------------------- cmd/ghcs/ssh.go | 6 +----- 10 files changed, 48 insertions(+), 57 deletions(-) create mode 100644 cmd/ghcs/main/main.go rename cmd/ghcs/{main.go => root.go} (77%) diff --git a/cmd/ghcs/code.go b/cmd/ghcs/code.go index d905c75ac..245a362be 100644 --- a/cmd/ghcs/code.go +++ b/cmd/ghcs/code.go @@ -1,4 +1,4 @@ -package main +package ghcs import ( "context" @@ -32,10 +32,6 @@ func newCodeCmd() *cobra.Command { return codeCmd } -func init() { - rootCmd.AddCommand(newCodeCmd()) -} - func code(codespaceName string, useInsiders bool) error { apiClient := api.New(os.Getenv("GITHUB_TOKEN")) ctx := context.Background() diff --git a/cmd/ghcs/common.go b/cmd/ghcs/common.go index e71e3dfe4..f15d2fef6 100644 --- a/cmd/ghcs/common.go +++ b/cmd/ghcs/common.go @@ -1,4 +1,4 @@ -package main +package ghcs // This file defines functions common to the entire ghcs command set. diff --git a/cmd/ghcs/create.go b/cmd/ghcs/create.go index 2125176fd..493a70247 100644 --- a/cmd/ghcs/create.go +++ b/cmd/ghcs/create.go @@ -1,4 +1,4 @@ -package main +package ghcs import ( "context" @@ -42,10 +42,6 @@ func newCreateCmd() *cobra.Command { return createCmd } -func init() { - rootCmd.AddCommand(newCreateCmd()) -} - func create(opts *createOptions) error { ctx := context.Background() apiClient := api.New(os.Getenv("GITHUB_TOKEN")) diff --git a/cmd/ghcs/delete.go b/cmd/ghcs/delete.go index 2c0de43ea..b75def7b6 100644 --- a/cmd/ghcs/delete.go +++ b/cmd/ghcs/delete.go @@ -1,4 +1,4 @@ -package main +package ghcs import ( "context" @@ -47,10 +47,6 @@ func newDeleteCmd() *cobra.Command { return deleteCmd } -func init() { - rootCmd.AddCommand(newDeleteCmd()) -} - func delete_(log *output.Logger, codespaceName string, force bool) error { apiClient := api.New(os.Getenv("GITHUB_TOKEN")) ctx := context.Background() diff --git a/cmd/ghcs/list.go b/cmd/ghcs/list.go index 72a25becc..09b0ea5b0 100644 --- a/cmd/ghcs/list.go +++ b/cmd/ghcs/list.go @@ -1,4 +1,4 @@ -package main +package ghcs import ( "context" @@ -31,10 +31,6 @@ func newListCmd() *cobra.Command { return listCmd } -func init() { - rootCmd.AddCommand(newListCmd()) -} - func list(opts *listOptions) error { apiClient := api.New(os.Getenv("GITHUB_TOKEN")) ctx := context.Background() diff --git a/cmd/ghcs/logs.go b/cmd/ghcs/logs.go index f65fa1109..40cac88ed 100644 --- a/cmd/ghcs/logs.go +++ b/cmd/ghcs/logs.go @@ -1,4 +1,4 @@ -package main +package ghcs import ( "context" @@ -36,10 +36,6 @@ func newLogsCmd() *cobra.Command { return logsCmd } -func init() { - rootCmd.AddCommand(newLogsCmd()) -} - func logs(ctx context.Context, log *output.Logger, codespaceName string, follow bool) error { // Ensure all child tasks (port forwarding, remote exec) terminate before return. ctx, cancel := context.WithCancel(ctx) diff --git a/cmd/ghcs/main/main.go b/cmd/ghcs/main/main.go new file mode 100644 index 000000000..01dde1270 --- /dev/null +++ b/cmd/ghcs/main/main.go @@ -0,0 +1,26 @@ +package main + +import ( + "errors" + "fmt" + "io" + "os" + + "github.com/github/ghcs/cmd/ghcs" +) + +func main() { + rootCmd := ghcs.NewRootCmd() + if err := rootCmd.Execute(); err != nil { + explainError(os.Stderr, err) + os.Exit(1) + } +} + +func explainError(w io.Writer, err error) { + if errors.Is(err, ghcs.ErrTokenMissing) { + fmt.Fprintln(w, "The GITHUB_TOKEN environment variable is required. Create a Personal Access Token at https://github.com/settings/tokens/new?scopes=repo") + fmt.Fprintln(w, "Make sure to enable SSO for your organizations after creating the token.") + return + } +} diff --git a/cmd/ghcs/ports.go b/cmd/ghcs/ports.go index 7bc53c441..052dfd37c 100644 --- a/cmd/ghcs/ports.go +++ b/cmd/ghcs/ports.go @@ -1,4 +1,4 @@ -package main +package ghcs import ( "bytes" @@ -47,10 +47,6 @@ func newPortsCmd() *cobra.Command { return portsCmd } -func init() { - rootCmd.AddCommand(newPortsCmd()) -} - func ports(codespaceName string, asJSON bool) error { apiClient := api.New(os.Getenv("GITHUB_TOKEN")) ctx := context.Background() diff --git a/cmd/ghcs/main.go b/cmd/ghcs/root.go similarity index 77% rename from cmd/ghcs/main.go rename to cmd/ghcs/root.go index 7903dad2a..6db4144a8 100644 --- a/cmd/ghcs/main.go +++ b/cmd/ghcs/root.go @@ -1,9 +1,8 @@ -package main +package ghcs import ( "errors" "fmt" - "io" "log" "os" "strconv" @@ -14,18 +13,12 @@ import ( "github.com/spf13/cobra" ) -func main() { - if err := rootCmd.Execute(); err != nil { - explainError(os.Stderr, err) - os.Exit(1) - } -} - var version = "DEV" // Replaced in the release build process (by GoReleaser or Homebrew) by the git tag version number. -var rootCmd = newRootCmd() +// GithubToken is a temporary stopgap to make the token configurable by apps that import this package +var GithubToken = os.Getenv("GITHUB_TOKEN") -func newRootCmd() *cobra.Command { +func NewRootCmd() *cobra.Command { var lightstep string root := &cobra.Command{ @@ -40,7 +33,7 @@ token to access the GitHub API with.`, PersistentPreRunE: func(cmd *cobra.Command, args []string) error { if os.Getenv("GITHUB_TOKEN") == "" { - return errTokenMissing + return ErrTokenMissing } return initLightstep(lightstep) }, @@ -48,18 +41,18 @@ token to access the GitHub API with.`, root.PersistentFlags().StringVar(&lightstep, "lightstep", "", "Lightstep tracing endpoint (service:token@host:port)") + root.AddCommand(newCodeCmd()) + root.AddCommand(newCreateCmd()) + root.AddCommand(newDeleteCmd()) + root.AddCommand(newListCmd()) + root.AddCommand(newLogsCmd()) + root.AddCommand(newPortsCmd()) + root.AddCommand(newSSHCmd()) + return root } -var errTokenMissing = errors.New("GITHUB_TOKEN is missing") - -func explainError(w io.Writer, err error) { - if errors.Is(err, errTokenMissing) { - fmt.Fprintln(w, "The GITHUB_TOKEN environment variable is required. Create a Personal Access Token at https://github.com/settings/tokens/new?scopes=repo") - fmt.Fprintln(w, "Make sure to enable SSO for your organizations after creating the token.") - return - } -} +var ErrTokenMissing = errors.New("GITHUB_TOKEN is missing") // initLightstep parses the --lightstep=service:token@host:port flag and // enables tracing if non-empty. diff --git a/cmd/ghcs/ssh.go b/cmd/ghcs/ssh.go index 6d5f2376b..527ae120f 100644 --- a/cmd/ghcs/ssh.go +++ b/cmd/ghcs/ssh.go @@ -1,4 +1,4 @@ -package main +package ghcs import ( "context" @@ -32,10 +32,6 @@ func newSSHCmd() *cobra.Command { return sshCmd } -func init() { - rootCmd.AddCommand(newSSHCmd()) -} - func ssh(ctx context.Context, sshProfile, codespaceName string, localSSHServerPort int) error { // Ensure all child tasks (e.g. port forwarding) terminate before return. ctx, cancel := context.WithCancel(ctx) From 8c0c7a8e19c5f971f434cb2b69c9e8a4dcbbccdf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 17 Sep 2021 16:29:35 +0200 Subject: [PATCH 2/8] Make GITHUB_TOKEN configurable through Go member Co-authored-by: Jose Garcia --- cmd/ghcs/code.go | 3 +-- cmd/ghcs/create.go | 2 +- cmd/ghcs/delete.go | 6 +++--- cmd/ghcs/list.go | 2 +- cmd/ghcs/logs.go | 2 +- cmd/ghcs/ports.go | 6 +++--- cmd/ghcs/ssh.go | 2 +- 7 files changed, 11 insertions(+), 12 deletions(-) diff --git a/cmd/ghcs/code.go b/cmd/ghcs/code.go index 245a362be..9f09438d5 100644 --- a/cmd/ghcs/code.go +++ b/cmd/ghcs/code.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "net/url" - "os" "github.com/github/ghcs/internal/api" "github.com/skratchdot/open-golang/open" @@ -33,7 +32,7 @@ func newCodeCmd() *cobra.Command { } func code(codespaceName string, useInsiders bool) error { - apiClient := api.New(os.Getenv("GITHUB_TOKEN")) + apiClient := api.New(GithubToken) ctx := context.Background() user, err := apiClient.GetUser(ctx) diff --git a/cmd/ghcs/create.go b/cmd/ghcs/create.go index 493a70247..45aa794e6 100644 --- a/cmd/ghcs/create.go +++ b/cmd/ghcs/create.go @@ -44,7 +44,7 @@ func newCreateCmd() *cobra.Command { func create(opts *createOptions) error { ctx := context.Background() - apiClient := api.New(os.Getenv("GITHUB_TOKEN")) + apiClient := api.New(GithubToken) locationCh := getLocation(ctx, apiClient) userCh := getUser(ctx, apiClient) log := output.NewLogger(os.Stdout, os.Stderr, false) diff --git a/cmd/ghcs/delete.go b/cmd/ghcs/delete.go index b75def7b6..34c1bc095 100644 --- a/cmd/ghcs/delete.go +++ b/cmd/ghcs/delete.go @@ -48,7 +48,7 @@ func newDeleteCmd() *cobra.Command { } func delete_(log *output.Logger, codespaceName string, force bool) error { - apiClient := api.New(os.Getenv("GITHUB_TOKEN")) + apiClient := api.New(GithubToken) ctx := context.Background() user, err := apiClient.GetUser(ctx) @@ -80,7 +80,7 @@ func delete_(log *output.Logger, codespaceName string, force bool) error { } func deleteAll(log *output.Logger, force bool) error { - apiClient := api.New(os.Getenv("GITHUB_TOKEN")) + apiClient := api.New(GithubToken) ctx := context.Background() user, err := apiClient.GetUser(ctx) @@ -119,7 +119,7 @@ func deleteAll(log *output.Logger, force bool) error { } func deleteByRepo(log *output.Logger, repo string, force bool) error { - apiClient := api.New(os.Getenv("GITHUB_TOKEN")) + apiClient := api.New(GithubToken) ctx := context.Background() user, err := apiClient.GetUser(ctx) diff --git a/cmd/ghcs/list.go b/cmd/ghcs/list.go index 09b0ea5b0..ccc150f08 100644 --- a/cmd/ghcs/list.go +++ b/cmd/ghcs/list.go @@ -32,7 +32,7 @@ func newListCmd() *cobra.Command { } func list(opts *listOptions) error { - apiClient := api.New(os.Getenv("GITHUB_TOKEN")) + apiClient := api.New(GithubToken) ctx := context.Background() user, err := apiClient.GetUser(ctx) diff --git a/cmd/ghcs/logs.go b/cmd/ghcs/logs.go index 40cac88ed..4051cc209 100644 --- a/cmd/ghcs/logs.go +++ b/cmd/ghcs/logs.go @@ -41,7 +41,7 @@ func logs(ctx context.Context, log *output.Logger, codespaceName string, follow ctx, cancel := context.WithCancel(ctx) defer cancel() - apiClient := api.New(os.Getenv("GITHUB_TOKEN")) + apiClient := api.New(GithubToken) user, err := apiClient.GetUser(ctx) if err != nil { diff --git a/cmd/ghcs/ports.go b/cmd/ghcs/ports.go index 052dfd37c..ebfd281cd 100644 --- a/cmd/ghcs/ports.go +++ b/cmd/ghcs/ports.go @@ -48,7 +48,7 @@ func newPortsCmd() *cobra.Command { } func ports(codespaceName string, asJSON bool) error { - apiClient := api.New(os.Getenv("GITHUB_TOKEN")) + apiClient := api.New(GithubToken) ctx := context.Background() log := output.NewLogger(os.Stdout, os.Stderr, asJSON) @@ -196,7 +196,7 @@ func newPortsPrivateCmd() *cobra.Command { func updatePortVisibility(log *output.Logger, codespaceName, sourcePort string, public bool) error { ctx := context.Background() - apiClient := api.New(os.Getenv("GITHUB_TOKEN")) + apiClient := api.New(GithubToken) user, err := apiClient.GetUser(ctx) if err != nil { @@ -258,7 +258,7 @@ func newPortsForwardCmd() *cobra.Command { func forwardPorts(log *output.Logger, codespaceName string, ports []string) error { ctx := context.Background() - apiClient := api.New(os.Getenv("GITHUB_TOKEN")) + apiClient := api.New(GithubToken) portPairs, err := getPortPairs(ports) if err != nil { diff --git a/cmd/ghcs/ssh.go b/cmd/ghcs/ssh.go index 527ae120f..5063e8fc9 100644 --- a/cmd/ghcs/ssh.go +++ b/cmd/ghcs/ssh.go @@ -37,7 +37,7 @@ func ssh(ctx context.Context, sshProfile, codespaceName string, localSSHServerPo ctx, cancel := context.WithCancel(ctx) defer cancel() - apiClient := api.New(os.Getenv("GITHUB_TOKEN")) + apiClient := api.New(GithubToken) log := output.NewLogger(os.Stdout, os.Stderr, false) user, err := apiClient.GetUser(ctx) From 9e08b7477da09d6ccb717716fa2c5874579a72a1 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Mon, 20 Sep 2021 13:40:45 -0400 Subject: [PATCH 3/8] delete: reject position args --- cmd/ghcs/delete.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cmd/ghcs/delete.go b/cmd/ghcs/delete.go index eb00e567f..e6c5c9f53 100644 --- a/cmd/ghcs/delete.go +++ b/cmd/ghcs/delete.go @@ -27,6 +27,9 @@ func newDeleteCmd() *cobra.Command { Use: "delete", Short: "Delete a codespace", RunE: func(cmd *cobra.Command, args []string) error { + if len(args) > 0 { + return fmt.Errorf("delete: unexpected positional arguments") + } switch { case allCodespaces && repo != "": return errors.New("both --all and --repo is not supported") From dbb80d8b1ef8bf2289c9d6614dbf95d4ad6fba0d Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Mon, 20 Sep 2021 16:01:43 -0400 Subject: [PATCH 4/8] check for authorised SSH keys --- cmd/ghcs/ssh.go | 21 +++++++++++++++++++++ internal/api/api.go | 26 ++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/cmd/ghcs/ssh.go b/cmd/ghcs/ssh.go index 6d5f2376b..3967f5512 100644 --- a/cmd/ghcs/ssh.go +++ b/cmd/ghcs/ssh.go @@ -49,6 +49,23 @@ func ssh(ctx context.Context, sshProfile, codespaceName string, localSSHServerPo return fmt.Errorf("error getting user: %w", err) } + // Check whether the user has registered any SSH keys. + // See https://github.com/github/ghcs/issues/166#issuecomment-921769703 + checkAuthKeys := func(user string) error { + keys, err := apiClient.AuthorizedKeys(ctx, user) + if err != nil { + return fmt.Errorf("failed to read GitHub-authorized SSH keys for %s: %w", user, err) + } + if len(keys) == 0 { + return fmt.Errorf("user %s has no GitHub-authorized SSH keys", user) + } + return nil // success + } + authkeys := make(chan error, 1) + go func() { + authkeys <- checkAuthKeys(user.Login) + }() + codespace, token, err := getOrChooseCodespace(ctx, apiClient, user, codespaceName) if err != nil { return fmt.Errorf("get or choose codespace: %w", err) @@ -59,6 +76,10 @@ func ssh(ctx context.Context, sshProfile, codespaceName string, localSSHServerPo return fmt.Errorf("error connecting to Live Share: %w", err) } + if err := <-authkeys; err != nil { + return err + } + log.Println("Fetching SSH Details...") remoteSSHServerPort, sshUser, err := session.StartSSHServer(ctx) if err != nil { diff --git a/internal/api/api.go b/internal/api/api.go index 1246389e8..2dd4d71b2 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -13,6 +13,7 @@ package api // - github.GetUser(github.Client) // - github.GetRepository(Client) // - github.ReadFile(Client, nwo, branch, path) // was GetCodespaceRepositoryContents +// - github.AuthorizedKeys(Client, user) // - codespaces.Create(Client, user, repo, sku, branch, location) // - codespaces.Delete(Client, user, token, name) // - codespaces.Get(Client, token, owner, name) @@ -507,6 +508,31 @@ func (a *API) GetCodespaceRepositoryContents(ctx context.Context, codespace *Cod return decoded, nil } +// AuthorizedKeys returns the public keys (in ~/.ssh/authorized_keys +// format) registered by the specified GitHub user. +func (a *API) AuthorizedKeys(ctx context.Context, user string) ([]byte, error) { + url := fmt.Sprintf("https://github.com/%s.keys", user) + req, err := http.NewRequest(http.MethodGet, url, nil) + if err != nil { + return nil, err + } + resp, err := a.do(ctx, req, "/user.keys") + if err != nil { + return nil, err + } + defer resp.Body.Close() + + b, err := ioutil.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("error reading response body: %w", err) + } + + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("server returned %s", resp.Status) + } + return b, nil +} + func (a *API) do(ctx context.Context, req *http.Request, spanName string) (*http.Response, error) { // TODO(adonovan): use NewRequestWithContext(ctx) and drop ctx parameter. span, ctx := opentracing.StartSpanFromContext(ctx, spanName) From 7f682f9c398099f30ab0824db56afc95a9edce9e Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Mon, 20 Sep 2021 16:56:57 -0400 Subject: [PATCH 5/8] Close Live Share sessions - New helper method codespaces.CloseSession to be used using defer - Upgrade to go-liveshare v0.17.0 --- cmd/ghcs/logs.go | 3 ++- cmd/ghcs/ports.go | 9 ++++++--- cmd/ghcs/ssh.go | 3 ++- internal/codespaces/codespaces.go | 8 ++++++++ internal/codespaces/states.go | 3 ++- 5 files changed, 20 insertions(+), 6 deletions(-) diff --git a/cmd/ghcs/logs.go b/cmd/ghcs/logs.go index 19528061a..4a38319e6 100644 --- a/cmd/ghcs/logs.go +++ b/cmd/ghcs/logs.go @@ -40,7 +40,7 @@ func init() { rootCmd.AddCommand(newLogsCmd()) } -func logs(ctx context.Context, log *output.Logger, codespaceName string, follow bool) error { +func logs(ctx context.Context, log *output.Logger, codespaceName string, follow bool) (err error) { // Ensure all child tasks (port forwarding, remote exec) terminate before return. ctx, cancel := context.WithCancel(ctx) defer cancel() @@ -61,6 +61,7 @@ func logs(ctx context.Context, log *output.Logger, codespaceName string, follow if err != nil { return fmt.Errorf("connecting to Live Share: %w", err) } + defer codespaces.CloseSession(session, &err) // Ensure local port is listening before client (getPostCreateOutput) connects. listen, err := net.Listen("tcp", ":0") // arbitrary port diff --git a/cmd/ghcs/ports.go b/cmd/ghcs/ports.go index 7bc53c441..45f92da7d 100644 --- a/cmd/ghcs/ports.go +++ b/cmd/ghcs/ports.go @@ -51,7 +51,7 @@ func init() { rootCmd.AddCommand(newPortsCmd()) } -func ports(codespaceName string, asJSON bool) error { +func ports(codespaceName string, asJSON bool) (err error) { apiClient := api.New(os.Getenv("GITHUB_TOKEN")) ctx := context.Background() log := output.NewLogger(os.Stdout, os.Stderr, asJSON) @@ -76,6 +76,7 @@ func ports(codespaceName string, asJSON bool) error { if err != nil { return fmt.Errorf("error connecting to Live Share: %w", err) } + defer codespaces.CloseSession(session, &err) log.Println("Loading ports...") ports, err := session.GetSharedServers(ctx) @@ -198,7 +199,7 @@ func newPortsPrivateCmd() *cobra.Command { } } -func updatePortVisibility(log *output.Logger, codespaceName, sourcePort string, public bool) error { +func updatePortVisibility(log *output.Logger, codespaceName, sourcePort string, public bool) (err error) { ctx := context.Background() apiClient := api.New(os.Getenv("GITHUB_TOKEN")) @@ -219,6 +220,7 @@ func updatePortVisibility(log *output.Logger, codespaceName, sourcePort string, if err != nil { return fmt.Errorf("error connecting to Live Share: %w", err) } + defer codespaces.CloseSession(session, &err) port, err := strconv.Atoi(sourcePort) if err != nil { @@ -260,7 +262,7 @@ func newPortsForwardCmd() *cobra.Command { } } -func forwardPorts(log *output.Logger, codespaceName string, ports []string) error { +func forwardPorts(log *output.Logger, codespaceName string, ports []string) (err error) { ctx := context.Background() apiClient := api.New(os.Getenv("GITHUB_TOKEN")) @@ -286,6 +288,7 @@ func forwardPorts(log *output.Logger, codespaceName string, ports []string) erro if err != nil { return fmt.Errorf("error connecting to Live Share: %w", err) } + defer codespaces.CloseSession(session, &err) // Run forwarding of all ports concurrently, aborting all of // them at the first failure, including cancellation of the context. diff --git a/cmd/ghcs/ssh.go b/cmd/ghcs/ssh.go index 4ece84d91..a92c99bb3 100644 --- a/cmd/ghcs/ssh.go +++ b/cmd/ghcs/ssh.go @@ -36,7 +36,7 @@ func init() { rootCmd.AddCommand(newSSHCmd()) } -func ssh(ctx context.Context, sshArgs []string, sshProfile, codespaceName string, localSSHServerPort int) error { +func ssh(ctx context.Context, sshArgs []string, sshProfile, codespaceName string, localSSHServerPort int) (err error) { // Ensure all child tasks (e.g. port forwarding) terminate before return. ctx, cancel := context.WithCancel(ctx) defer cancel() @@ -58,6 +58,7 @@ func ssh(ctx context.Context, sshArgs []string, sshProfile, codespaceName string if err != nil { return fmt.Errorf("error connecting to Live Share: %w", err) } + defer codespaces.CloseSession(session, &err) log.Println("Fetching SSH Details...") remoteSSHServerPort, sshUser, err := session.StartSSHServer(ctx) diff --git a/internal/codespaces/codespaces.go b/internal/codespaces/codespaces.go index 6235ca3a0..fe62a3d79 100644 --- a/internal/codespaces/codespaces.go +++ b/internal/codespaces/codespaces.go @@ -73,3 +73,11 @@ func ConnectToLiveshare(ctx context.Context, log logger, apiClient *api.API, use return lsclient.JoinWorkspace(ctx) } + +// CloseSession closes the Live Share session and assigns the error to the pointer if it is nil. +func CloseSession(session *liveshare.Session, err *error) { + closeErr := session.Close() + if *err == nil { + *err = closeErr + } +} diff --git a/internal/codespaces/states.go b/internal/codespaces/states.go index 408f11941..7e464d919 100644 --- a/internal/codespaces/states.go +++ b/internal/codespaces/states.go @@ -36,7 +36,7 @@ type PostCreateState struct { // PollPostCreateStates watches for state changes in a codespace, // and calls the supplied poller for each batch of state changes. // It runs until it encounters an error, including cancellation of the context. -func PollPostCreateStates(ctx context.Context, log logger, apiClient *api.API, user *api.User, codespace *api.Codespace, poller func([]PostCreateState)) error { +func PollPostCreateStates(ctx context.Context, log logger, apiClient *api.API, user *api.User, codespace *api.Codespace, poller func([]PostCreateState)) (err error) { token, err := apiClient.GetCodespaceToken(ctx, user.Login, codespace.Name) if err != nil { return fmt.Errorf("getting codespace token: %w", err) @@ -46,6 +46,7 @@ func PollPostCreateStates(ctx context.Context, log logger, apiClient *api.API, u if err != nil { return fmt.Errorf("connect to Live Share: %w", err) } + defer CloseSession(session, &err) // Ensure local port is listening before client (getPostCreateOutput) connects. listen, err := net.Listen("tcp", ":0") // arbitrary port From a83b3c08167cddb3f8edb07c25ad1de428194c79 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Tue, 21 Sep 2021 08:46:32 -0400 Subject: [PATCH 6/8] Update to go-livesare v0.18.0 - Only set err if closeErr is non-nil --- internal/codespaces/codespaces.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/codespaces/codespaces.go b/internal/codespaces/codespaces.go index fe62a3d79..ae1115905 100644 --- a/internal/codespaces/codespaces.go +++ b/internal/codespaces/codespaces.go @@ -23,6 +23,8 @@ func connectionReady(codespace *api.Codespace) bool { codespace.Environment.State == api.CodespaceEnvironmentStateAvailable } +// ConnectToLiveshare creates a Live Share client and joins the Live Share session. +// It will start the Codespace if it is not already running, it will time out after 60 seconds if fails to start. func ConnectToLiveshare(ctx context.Context, log logger, apiClient *api.API, userLogin, token string, codespace *api.Codespace) (*liveshare.Session, error) { var startedCodespace bool if codespace.Environment.State != api.CodespaceEnvironmentStateAvailable { @@ -75,9 +77,10 @@ func ConnectToLiveshare(ctx context.Context, log logger, apiClient *api.API, use } // CloseSession closes the Live Share session and assigns the error to the pointer if it is nil. +// It is meant to be called using defer with a named return argument for the error. func CloseSession(session *liveshare.Session, err *error) { closeErr := session.Close() - if *err == nil { - *err = closeErr + if *err == nil && closeErr != nil { + *err = fmt.Errorf("failed to close Live Share session: %w", closeErr) } } From d3d1ce726d5853c907775e2bafd8b0dbd163e416 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Tue, 21 Sep 2021 09:59:19 -0400 Subject: [PATCH 7/8] do logs too --- cmd/ghcs/common.go | 14 ++++++++++++++ cmd/ghcs/logs.go | 9 +++++++++ cmd/ghcs/ssh.go | 14 +------------- 3 files changed, 24 insertions(+), 13 deletions(-) diff --git a/cmd/ghcs/common.go b/cmd/ghcs/common.go index e71e3dfe4..ba23ef8b4 100644 --- a/cmd/ghcs/common.go +++ b/cmd/ghcs/common.go @@ -120,3 +120,17 @@ func ask(qs []*survey.Question, response interface{}) error { } return err } + +// checkAuthorizedKeys reports an error if the user has not registered any SSH keys; +// see https://github.com/github/ghcs/issues/166#issuecomment-921769703. +// The check is not required for security but it improves the error message. +func checkAuthorizedKeys(ctx context.Context, client *api.API, user string) error { + keys, err := client.AuthorizedKeys(ctx, user) + if err != nil { + return fmt.Errorf("failed to read GitHub-authorized SSH keys for %s: %w", user, err) + } + if len(keys) == 0 { + return fmt.Errorf("user %s has no GitHub-authorized SSH keys", user) + } + return nil // success +} diff --git a/cmd/ghcs/logs.go b/cmd/ghcs/logs.go index f65fa1109..d0f164c37 100644 --- a/cmd/ghcs/logs.go +++ b/cmd/ghcs/logs.go @@ -52,6 +52,11 @@ func logs(ctx context.Context, log *output.Logger, codespaceName string, follow return fmt.Errorf("getting user: %w", err) } + authkeys := make(chan error, 1) + go func() { + authkeys <- checkAuthorizedKeys(ctx, apiClient, user.Login) + }() + codespace, token, err := getOrChooseCodespace(ctx, apiClient, user, codespaceName) if err != nil { return fmt.Errorf("get or choose codespace: %w", err) @@ -62,6 +67,10 @@ func logs(ctx context.Context, log *output.Logger, codespaceName string, follow return fmt.Errorf("connecting to Live Share: %w", err) } + if err := <-authkeys; err != nil { + return err + } + // Ensure local port is listening before client (getPostCreateOutput) connects. listen, err := net.Listen("tcp", ":0") // arbitrary port if err != nil { diff --git a/cmd/ghcs/ssh.go b/cmd/ghcs/ssh.go index 3967f5512..23e87b33d 100644 --- a/cmd/ghcs/ssh.go +++ b/cmd/ghcs/ssh.go @@ -49,21 +49,9 @@ func ssh(ctx context.Context, sshProfile, codespaceName string, localSSHServerPo return fmt.Errorf("error getting user: %w", err) } - // Check whether the user has registered any SSH keys. - // See https://github.com/github/ghcs/issues/166#issuecomment-921769703 - checkAuthKeys := func(user string) error { - keys, err := apiClient.AuthorizedKeys(ctx, user) - if err != nil { - return fmt.Errorf("failed to read GitHub-authorized SSH keys for %s: %w", user, err) - } - if len(keys) == 0 { - return fmt.Errorf("user %s has no GitHub-authorized SSH keys", user) - } - return nil // success - } authkeys := make(chan error, 1) go func() { - authkeys <- checkAuthKeys(user.Login) + authkeys <- checkAuthorizedKeys(ctx, apiClient, user.Login) }() codespace, token, err := getOrChooseCodespace(ctx, apiClient, user, codespaceName) From e8e914c220b9ec828b4965a07a843d67bb4c3c18 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Tue, 21 Sep 2021 10:05:48 -0400 Subject: [PATCH 8/8] PR Feedback - Upgrade to go-liveshare v0.19.0 - Remove export helper method - Use local implementation --- cmd/ghcs/common.go | 7 +++++++ cmd/ghcs/logs.go | 2 +- cmd/ghcs/ports.go | 6 +++--- cmd/ghcs/ssh.go | 2 +- internal/codespaces/codespaces.go | 9 --------- internal/codespaces/states.go | 6 +++++- 6 files changed, 17 insertions(+), 15 deletions(-) diff --git a/cmd/ghcs/common.go b/cmd/ghcs/common.go index e71e3dfe4..79fda32ef 100644 --- a/cmd/ghcs/common.go +++ b/cmd/ghcs/common.go @@ -6,6 +6,7 @@ import ( "context" "errors" "fmt" + "io" "os" "sort" @@ -93,6 +94,12 @@ func getOrChooseCodespace(ctx context.Context, apiClient *api.API, user *api.Use return codespace, token, nil } +func safeClose(closer io.Closer, err *error) { + if closeErr := closer.Close(); *err == nil { + *err = closeErr + } +} + // hasTTY indicates whether the process connected to a terminal. // It is not portable to assume stdin/stdout are fds 0 and 1. var hasTTY = term.IsTerminal(int(os.Stdin.Fd())) && term.IsTerminal(int(os.Stdout.Fd())) diff --git a/cmd/ghcs/logs.go b/cmd/ghcs/logs.go index 4a38319e6..db83250c5 100644 --- a/cmd/ghcs/logs.go +++ b/cmd/ghcs/logs.go @@ -61,7 +61,7 @@ func logs(ctx context.Context, log *output.Logger, codespaceName string, follow if err != nil { return fmt.Errorf("connecting to Live Share: %w", err) } - defer codespaces.CloseSession(session, &err) + defer safeClose(session, &err) // Ensure local port is listening before client (getPostCreateOutput) connects. listen, err := net.Listen("tcp", ":0") // arbitrary port diff --git a/cmd/ghcs/ports.go b/cmd/ghcs/ports.go index 45f92da7d..f48dd1e6d 100644 --- a/cmd/ghcs/ports.go +++ b/cmd/ghcs/ports.go @@ -76,7 +76,7 @@ func ports(codespaceName string, asJSON bool) (err error) { if err != nil { return fmt.Errorf("error connecting to Live Share: %w", err) } - defer codespaces.CloseSession(session, &err) + defer safeClose(session, &err) log.Println("Loading ports...") ports, err := session.GetSharedServers(ctx) @@ -220,7 +220,7 @@ func updatePortVisibility(log *output.Logger, codespaceName, sourcePort string, if err != nil { return fmt.Errorf("error connecting to Live Share: %w", err) } - defer codespaces.CloseSession(session, &err) + defer safeClose(session, &err) port, err := strconv.Atoi(sourcePort) if err != nil { @@ -288,7 +288,7 @@ func forwardPorts(log *output.Logger, codespaceName string, ports []string) (err if err != nil { return fmt.Errorf("error connecting to Live Share: %w", err) } - defer codespaces.CloseSession(session, &err) + defer safeClose(session, &err) // Run forwarding of all ports concurrently, aborting all of // them at the first failure, including cancellation of the context. diff --git a/cmd/ghcs/ssh.go b/cmd/ghcs/ssh.go index a92c99bb3..88117c480 100644 --- a/cmd/ghcs/ssh.go +++ b/cmd/ghcs/ssh.go @@ -58,7 +58,7 @@ func ssh(ctx context.Context, sshArgs []string, sshProfile, codespaceName string if err != nil { return fmt.Errorf("error connecting to Live Share: %w", err) } - defer codespaces.CloseSession(session, &err) + defer safeClose(session, &err) log.Println("Fetching SSH Details...") remoteSSHServerPort, sshUser, err := session.StartSSHServer(ctx) diff --git a/internal/codespaces/codespaces.go b/internal/codespaces/codespaces.go index ae1115905..2933c9d8d 100644 --- a/internal/codespaces/codespaces.go +++ b/internal/codespaces/codespaces.go @@ -75,12 +75,3 @@ func ConnectToLiveshare(ctx context.Context, log logger, apiClient *api.API, use return lsclient.JoinWorkspace(ctx) } - -// CloseSession closes the Live Share session and assigns the error to the pointer if it is nil. -// It is meant to be called using defer with a named return argument for the error. -func CloseSession(session *liveshare.Session, err *error) { - closeErr := session.Close() - if *err == nil && closeErr != nil { - *err = fmt.Errorf("failed to close Live Share session: %w", closeErr) - } -} diff --git a/internal/codespaces/states.go b/internal/codespaces/states.go index 7e464d919..31105d576 100644 --- a/internal/codespaces/states.go +++ b/internal/codespaces/states.go @@ -46,7 +46,11 @@ func PollPostCreateStates(ctx context.Context, log logger, apiClient *api.API, u if err != nil { return fmt.Errorf("connect to Live Share: %w", err) } - defer CloseSession(session, &err) + defer func() { + if closeErr := session.Close(); err == nil { + err = closeErr + } + }() // Ensure local port is listening before client (getPostCreateOutput) connects. listen, err := net.Listen("tcp", ":0") // arbitrary port