From 9e08b7477da09d6ccb717716fa2c5874579a72a1 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Mon, 20 Sep 2021 13:40:45 -0400 Subject: [PATCH 1/6] 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 2/6] 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 3/6] 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 4/6] 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 5/6] 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 6/6] 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