fix: gh cs logs to use automatic ssh key pair
#10783
Conversation
| @@ -0,0 +1,123 @@ | |||
| package sshkey | |||
There was a problem hiding this comment.
This package includes the code that was previously only used by gh cs ssh and gh cs cp to detect the right key to use and to generate keys in case the user didn't have any.
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { |
There was a problem hiding this comment.
I've modified the tests so that they used subtests with names to make it easier to detect errors.
pkg/cmd/codespace/logs.go
Outdated
| args := []string{fmt.Sprintf("%s /workspaces/.codespaces/.persistedshare/creation.log", cmdType)} | ||
|
|
||
| keys := sshkey.NewConfig(ssh.Context{}) | ||
| keyPair, args, err := keys.FindKey(ctx, args, "") |
There was a problem hiding this comment.
This is going to always select the automatic key, since there is never a -i flag added. That usually works, but we've gotten complaints in the past that users who got flagged for having the automatic key on their work machines.
Should we add an optional parameter for people to workaround that? Or support gh cs logs -- <ssh-args> like we do with ssh and cp?
Alternatively, I think @dmgardiner25 has added a gRPC call to get the content of a file that we could use here instead of ssh cat. That wouldn't support --follow, but anyways the command is totally busted so I think it might be OK to just remove that option?
There was a problem hiding this comment.
Makes sense! Let me work on that feedback and I'll let you know.
There was a problem hiding this comment.
I was thinking about this more overnight: something to consider actually is that this is only going to export the devcontainer creation log - which is not going to be super helpful for your goal of exporting all the codespace logs for debugging. We should probably switch this to using the full ExportLogs grpc call rather than just retrieving this one file
There was a problem hiding this comment.
I had not seen this comment, but after the refactor I've done it should be relatively easy to add a new option to the logs command that does just this.
79b2efd to
5f64401
Compare
This commit changes the codespaces `logs` command to use the same connection infrastructure as the `ssh` and `cp` commands. By adopting the shared architecture, the logs command now: - Uses the same SSH key handling and connection management as other codespace commands, as described in cli#8208. These changes bring several architectural benefits: - Better encapsulation: Connection details are now hidden behind a clean interface that all commands can use. This also simplifies use of concurrency in the commands as those details are now encapsulated too on the right `grpctunnel` package - Improved testability: The tunnel abstraction allows for isolated testing of connection behavior - Component reusability: New commands can be created through composition with the tunnel interface The shared connection architecture makes it simple to maintain consistent behavior across all codespace commands that need remote access, while allowing each command to focus on its specific functionality rather than connection handling details. As an extra, in order to reduce the cognitive load on the `ssh --config` command, this also extracts a package that handles printing the configuration.
| type rwio struct { | ||
| io.ReadCloser | ||
| io.WriteCloser | ||
| } |
There was a problem hiding this comment.
This struct is extracted from the ssh command. It is used for the gh cs ssh --stdio option.
| // 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 Config struct { |
There was a problem hiding this comment.
This was extracted from the ssh command and is used for gh cs ssh --config command. It was previously declared inline on the command for each codespace.
| tunnel, args, err := a.connect(ctx, opts.selector, args, opts.profile) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer tunnel.Close() | ||
|
|
||
| hasRemote := false | ||
| for _, arg := range args { | ||
| if rest := strings.TrimPrefix(arg, "remote:"); rest != arg { | ||
| hasRemote = true | ||
| // scp treats each filename argument as a shell expression, | ||
| // subjecting it to expansion of environment variables, braces, | ||
| // tilde, backticks, globs and so on. Because these present a | ||
| // security risk (see https://lwn.net/Articles/835962/), we | ||
| // disable them by shell-escaping the argument unless the user | ||
| // provided the -e flag. | ||
| if !opts.expand { | ||
| arg = `remote:'` + strings.Replace(rest, `'`, `'\''`, -1) + `'` | ||
| } | ||
|
|
||
| } else if !filepath.IsAbs(arg) { | ||
| // scp treats a colon in the first path segment as a host identifier. | ||
| // Escape it by prepending "./". | ||
| // TODO(adonovan): test on Windows, including with a c:\\foo path. | ||
| const sep = string(os.PathSeparator) | ||
| first := strings.Split(filepath.ToSlash(arg), sep)[0] | ||
| if strings.Contains(first, ":") { | ||
| arg = "." + sep + arg | ||
| } | ||
| } | ||
| opts.scpArgs = append(opts.scpArgs, arg) | ||
| } | ||
| if !hasRemote { | ||
| return cmdutil.FlagErrorf("at least one argument must have a 'remote:' prefix") | ||
| // The cs cp command doesn't have an option to select the server port, we use | ||
| // 0 here so that it pick an unused port ransomly. | ||
| if err := tunnel.Forward(ctx, 0); err != nil { | ||
| return fmt.Errorf("error forwarding tunnel: %w", err) | ||
| } | ||
| return a.SSH(ctx, nil, opts.sshOptions) | ||
|
|
||
| return tunnel.Run(opts.profile, func(destination string, port int) error { | ||
| return codespaces.Copy(ctx, args, port, destination) | ||
| }) |
There was a problem hiding this comment.
As the code for connections is encapsulated now, gh cs cp doesn't need to depend on gh cs ssh, which was unnecessarily coupling both commands.
5f64401 to
0f4e47d
Compare
|
👋 Hey @cmbrose, just going through the PR backlog and figured I'd bump this one. |
Ref: #8208
TL;DR
This PR modifies the
gh cs logscommand so that it uses the automaticallygenerated keypair in case not other key has been detected. It also refactors
the key detection and generation a bit to make it easy to share between
commands.
What is this doing and why?
This commit changes the codespaces
logscommand to use the sameconnection infrastructure as the
sshandcpcommands. By adoptingthe shared architecture, the logs command now:
codespace commands, as described in All
gh cscommands that use SSH should respect/generate thecodespaces.autoSSH key #8208.These changes bring several architectural benefits:
interface that all commands can use. This also simplifies use of
concurrency in the commands as those details are now encapsulated too
on the right
grpctunnelpackagetesting of connection behavior
with the tunnel interface
The shared connection architecture makes it simple to maintain
consistent behavior across all codespace commands that need remote
access, while allowing each command to focus on its specific
functionality rather than connection handling details.
As an extra, in order to reduce the cognitive load on the
ssh --configcommand, this also extracts a package that handles printing the
configuration.
How have I tested this?
I've created a new codespace, disabled my own keys and tested that I was not able to see the logs, but I was able to access through SSH though.
Using the compiled binary including this code I'm able to access through SSH, and also to see the logs.