Machine ID: Switch to using new tbot ssh-proxy-command by default.
#41482
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
strideynet
commented
May 14, 2024
| @@ -62,17 +63,27 @@ type ProxySSHConfig struct { | |||
| // ProxySSH creates a local ssh proxy, dialing a node and transferring data through | |||
| // stdin and stdout, to be used as an OpenSSH/PuTTY proxy command. | |||
| func ProxySSH(ctx context.Context, proxyConfig ProxySSHConfig) error { | |||
There was a problem hiding this comment.
@rosstimothy - I've made some minor changes here, perhaps worth your attention...
| @@ -21,6 +21,6 @@ Host *.tele.aperture.labs tele.blackmesa.gov | |||
| # Flags for all tele.aperture.labs hosts except the proxy | |||
| Host *.tele.aperture.labs !tele.blackmesa.gov | |||
| Port 3022 | |||
| ProxyCommand "/path/to/tbot" proxy --destination-dir=/test/dir --proxy-server=tele.blackmesa.gov:443 ssh --cluster=tele.aperture.labs %r@%h:%p | |||
| ProxyCommand "/path/to/tbot" ssh-proxy-command --destination-dir=/test/dir --proxy-server=tele.blackmesa.gov:443 --cluster=tele.aperture.labs --tls-routing=true --connection-upgrade=false --resume=true --user=%r --host=%h --port=%p | |||
There was a problem hiding this comment.
@rosstimothy / @espadolini - double check that tI'm not missing anything here.
There was a problem hiding this comment.
Looks about right to me. Whats the story for proxy templates? Does that require humans to manually tweak the config?
There was a problem hiding this comment.
Yeah right now the answer is "Manually modify the file" - I wanted to keep this PR a manageable size. What I'll do in another PR is allow them to be configured directly within the tbot.yaml and then they will be written to proxytemplates.yaml in the output destination.
|
I'm going to manually test this against:
|
rosstimothy
approved these changes
May 14, 2024
| @@ -21,6 +21,6 @@ Host *.tele.aperture.labs tele.blackmesa.gov | |||
| # Flags for all tele.aperture.labs hosts except the proxy | |||
| Host *.tele.aperture.labs !tele.blackmesa.gov | |||
| Port 3022 | |||
| ProxyCommand "/path/to/tbot" proxy --destination-dir=/test/dir --proxy-server=tele.blackmesa.gov:443 ssh --cluster=tele.aperture.labs %r@%h:%p | |||
| ProxyCommand "/path/to/tbot" ssh-proxy-command --destination-dir=/test/dir --proxy-server=tele.blackmesa.gov:443 --cluster=tele.aperture.labs --tls-routing=true --connection-upgrade=false --resume=true --user=%r --host=%h --port=%p | |||
There was a problem hiding this comment.
Looks about right to me. Whats the story for proxy templates? Does that require humans to manually tweak the config?
Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>
espadolini
reviewed
May 14, 2024
timothyb89
reviewed
May 15, 2024
Co-authored-by: Edoardo Spadolini <edoardo.spadolini@goteleport.com>
Co-authored-by: Edoardo Spadolini <edoardo.spadolini@goteleport.com>
…avitational/teleport into strideynet/switch-to-new-ssh-proxying
espadolini
approved these changes
May 15, 2024
|
Test against my local cluster (with a TLS terminating L7LB and TLS routing) And using the |
rosstimothy
reviewed
May 15, 2024
Comment on lines
+41
to
+46
| s = `'` + strings.ReplaceAll(s, `'`, `'"'"'`) + `'` | ||
| // escape any percent signs which could trigger the percent expansion | ||
| // for ProxyCommand. | ||
| s = strings.ReplaceAll(s, `%`, `%%`) | ||
| // escape any newlines which could impact the parsing of ssh_config | ||
| s = strings.ReplaceAll(s, "\n", `'"\n"'`) |
There was a problem hiding this comment.
should we use safetext here?
There was a problem hiding this comment.
We couldn't find a sensible way to then go back and replace literal newlines with their escaped form with a shsprintf.DefaultWhatever - which we need because we're trying to build a string that, when fed through openssh's config parsing, percent replacing and execution through a shell, results in the process we want being executed with the arguments that we want.
|
Tested against a Cloud tenant (TLS routing on, but no need for connection upgrade) |
|
@strideynet See the table below for backport results.
|
strideynet
added a commit
that referenced
this pull request
May 17, 2024
…41482) * Add warning to previous command * Hash out basics of new tbot proxying command use * Avoid passing full botConfig into `tbot ssh-proxy-command` * wip * simplify impl * Set up test suite for ssh config generation * Add support for checking if ALPNConnUpgrade is required * Cache the alpn upgrade result * Americanize Spellings * Update lib/tbot/config/template_ssh_client.go Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com> * Add CHANGELOG.md entry for breaking change * Update tool/tbot/proxy.go Co-authored-by: Edoardo Spadolini <edoardo.spadolini@goteleport.com> * Update lib/tbot/tbot.go Co-authored-by: Edoardo Spadolini <edoardo.spadolini@goteleport.com> * Switch to `--no` variants rather than `={bool}` * Use SingleFlight on alpn proxy upgrade cache * Improve singleflight * Add rudimentary shell quoting * Uniformly pad the templating * Rename shellQuote -> proxyCommandQuote * Fix imports --------- Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com> Co-authored-by: Edoardo Spadolini <edoardo.spadolini@goteleport.com>
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 17, 2024
…41482) (#41694) * Add warning to previous command * Hash out basics of new tbot proxying command use * Avoid passing full botConfig into `tbot ssh-proxy-command` * wip * simplify impl * Set up test suite for ssh config generation * Add support for checking if ALPNConnUpgrade is required * Cache the alpn upgrade result * Americanize Spellings * Update lib/tbot/config/template_ssh_client.go * Add CHANGELOG.md entry for breaking change * Update tool/tbot/proxy.go * Update lib/tbot/tbot.go * Switch to `--no` variants rather than `={bool}` * Use SingleFlight on alpn proxy upgrade cache * Improve singleflight * Add rudimentary shell quoting * Uniformly pad the templating * Rename shellQuote -> proxyCommandQuote * Fix imports --------- Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com> Co-authored-by: Edoardo Spadolini <edoardo.spadolini@goteleport.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #28910
Switches the generated
ssh_configto use the newssh-proxy-command. Plans the deprecation of the old ssh proxy command to occur in v17. The new behavior is significantly more performant and less resource intense, and reduces the dependency ontsh.The old behavior can be temporarily selected by setting
TBOT_SSH_CONFIG_PROXY_COMMAND_MODE=legacy- this will be removed in v17.In addition, I've made some minor changes to the new command to avoid it depending so explicitly on the complete BotConfig.
I will backport this change to v15, but flip the behaviour so that this mode is opted-in to rather than the default. This will allow select users to switch to the new behavior without waiting for v16.
Associated with #41463
changelog: Switches the default SSH proxying mode in Machine ID to the newer, more performant, version. If you use Machine ID and OpenSSH, you may need to adjust your configuration, see https://goteleport.com/docs/machine-id/reference/v16-upgrade-guide/ for more information.