Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tsh config helper to generate OpenSSH client configuration #7437

Merged
merged 17 commits into from
Jul 22, 2021

Conversation

timothyb89
Copy link
Contributor

@timothyb89 timothyb89 commented Jun 29, 2021

This adds a new subcommand, tsh config, to generate OpenSSH client configuration snippets that allow users to connect directly to nodes using the standard ssh client.

To support this change, tsh's known_hosts file has been modified to match the format required by OpenSSH when verifying hosts against certificates. Old-style known_hosts entries will be automatically replaced and pruned when the end user first logs in with an updated tsh. Small changes were additionally made to the keystore and key agent to pass the proxy host into AddKnownHostKeys and to support wildcard hostnames in known_hosts entries.

As an example, on my machine, the generated configuration from tsh config looks like this:

#
# Begin generated Teleport configuration for proxy.foo.example.com:3080 from `tsh config`
#

# Common flags for all foo.example.com hosts
Host *.foo.example.com proxy.foo.example.com
    UserKnownHostsFile "/Users/tim/.tsh/known_hosts"

# Flags for all foo.example.com hosts except the proxy
Host *.foo.example.com !proxy.foo.example.com
    Port 3022
    ProxyCommand ssh -p 3023 proxy.foo.example.com -s proxy:$(echo %h | cut -d '.' -f 1):%p

# Common flags for all leaf.example.com hosts
Host *.leaf.example.com proxy.foo.example.com
    UserKnownHostsFile "/Users/tim/.tsh/known_hosts"

# Flags for all leaf.example.com hosts except the proxy
Host *.leaf.example.com !proxy.foo.example.com
    Port 3022
    ProxyCommand ssh -p 3023 proxy.foo.example.com -s proxy:$(echo %h | cut -d '.' -f 1):%[email protected]

# End generated Teleport configuration

... and the revised ~/.tsh/known_hosts format looks like this:

@cert-authority proxy.foo.example.com,foo.example.com,*.foo.example.com ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCt6aILUzSic4xq/BmrLsIZNs2wfzNbVE0Qlft2dUo4Sb9mwODVhvYLDMfgI1nWsDCTcwWHbu1o0j6GxLhnyigEHJpavpBbn4YqutTQ1xp35dWMi76PSUu7bNCAx3IZz8E1JhELHzFUSFAY5CuyuIy+cxKsD+CdN0xFUNtjqozX6BCp8bcY+WQCNLDf8H1m8RST/FeTaBmPtBzxF6yei7ehczEcBTC1czbprQ7piiBI6CGNeE9zKdKYHDtbQyRGs8Cy56jCF0fgEdXO0p7DqJIcr0/IjmdW6OQku1MKC8v3VxusXCVxKuJ9T5TeiRPglyW6APy4iKZYAO40Wy2WONcf type=host
@cert-authority proxy.foo.example.com,leaf.example.com,*.leaf.example.com ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDWUE468X1slCu5xGDIZInkVdLRIi1FU6x6221+ecr9h1KVBP0flzSssRBS7Qfa47uJ+cJaak3/luwnUmIgtQX4ovU4r5MJuZhaAreYMF6A73vDMehhOUH3KsNkEvMwOlOREJYt2YM8RZ6eSLgyo/HDlX8BHfl4uKDd9qOqvKSNab5VDstEHuoslEEj8YG2v8FaBMKUjudheSzmzRvdeSKVy9FjHA5yZslXzcMS0S0QACL4BleWwzpp/zZ++xGiTbtdb50FKhrc7REQQHmec/O/N31mLyzsUnJjeBcipphX/iYVnnzgvY78N7RfBnebNmt4To1KCul1HVK9jgsqcrUf type=host

Fixes #3734

@timothyb89
Copy link
Contributor Author

Hi all, I've got a few open questions I'd especially like some feedback on here:

  1. This mildly changes the semantics of tsh's keystore. Previously, tsh only wrote a single hostname, clusterName, into each known_hosts entry. With this change, entries need to have at least *.clusterName and proxyHost in their hosts list, and with just those two both OpenSSH and tsh ssh appear to work properly. However:

    • It breaks the semantics of the GetKnownHostKeys - if I insert example.com, I could only retrieve it with *.example.com (or the proxy host, though occasionally the proxyHost and clusterName are identical)
    • Because of this, 2 unit tests break. They could be fixed by retrieving *.example.com instead.
    • I'm unclear if tsh ever depends on bare clusterName retrieval.

    Given this, I've opted to additionally include the plain clusterName in each known_hosts entry. Is this the right thing to do?

  2. As we're now inserting wildcard hosts, GetKnownHostKeys could only retrieve a literal *.example.com and would not match, say, foo.example.com. I have modified fsLocalNonSessionKeyStore.GetKnownHostKeys() to additionally match simple wildcard hostnames, however I don't believe this is used anywhere. While it does resolve a soundness issue, it does increase attack surface. Is this a change we want to keep?

  3. Should we stick with tsh config ssh or instead use @r0mant's suggestion in Provide an easy option to setup OpenSSH Config. #3734 (comment) to use just tsh config?

lib/client/known_hosts_migrate.go Outdated Show resolved Hide resolved
tool/tsh/config.go Outdated Show resolved Hide resolved
tool/tsh/tsh.go Outdated Show resolved Hide resolved
Copy link
Contributor

@webvictim webvictim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of general implementation questions:

  1. How does this work with IP addresses? A lot of customers/PoCs still use ssh [email protected] - will this handle IP ranges OK?
  2. What happens if proxy_service.public_addr isn't set for a given cluster?

@timothyb89
Copy link
Contributor Author

  1. How does this work with IP addresses? A lot of customers/PoCs still use ssh [email protected] - will this handle IP ranges OK?

They won't work as-is, since both the config generation and known_hosts depend on having some predictable way of addressing nodes for a particular cluster. The existing OpenSSH client guide notes a similar limitation however as far as I can tell the node host certificates have their IPs listed as principals as well.

It could probably be made to work if we listed out every individual node IP in both the generated config and in ~/.tsh/known_hosts, but I'm not sure how I feel about doing that at this stage.

Maybe this is a good candidate to implement in a future pass, alongside Match exec host matching? I'll also note that recent versions of OpenSSH support a KnownHostsCommand directive which could generate a proper known hosts entry on the fly if needed.

  1. What happens if proxy_service.public_addr isn't set for a given cluster?

It still generates the same output on my test cluster, I assume the client falls back to the address as written by the user if the public address is unset?

This adds a new subcommand, `tsh config ssh`, to generate OpenSSH
client configuration snippets that allow users to connect directly to
nodes using the standard `ssh` client.

To support this change, tsh's `known_hosts` file has been modified to
match the format required by OpenSSH when verifying hosts against
certificates. Old-style `known_hosts` entries will be automatically
replaced and pruned when the end user first logs in with an updated
`tsh`. Small changes were additionally made to the keystore and key
agent to pass the proxy host into `AddKnownHostKeys` and to support
wildcard hostnames in `known_hosts` entries.
This changes the config helper to use just `tsh config` per
suggestion from @r0mant.
@timothyb89 timothyb89 force-pushed the timothyb89/ssh-config-helper branch from 18e01e3 to 4a02b15 Compare July 8, 2021 22:49
@timothyb89 timothyb89 changed the title Add tsh config ssh helper to generate OpenSSH client configuration Add tsh config helper to generate OpenSSH client configuration Jul 12, 2021
@russjones
Copy link
Contributor

@r0mant @xacrimon @jane Can you review?

docs/pages/server-access/guides/openssh.mdx Outdated Show resolved Hide resolved
lib/client/keystore.go Outdated Show resolved Hide resolved
lib/client/keystore.go Outdated Show resolved Hide resolved
lib/client/keystore.go Outdated Show resolved Hide resolved
lib/client/keystore.go Outdated Show resolved Hide resolved
// exists. If not, pass it through.
for _, entry := range oldEntries {
if canPruneOldHostsEntry(entry, newEntries) {
log.Debugf("Pruning old known_hosts entry for %s.", entry.hosts[0])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check hosts length?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old entries have their length checked in isOldStyleHostsEntry() and are guaranteed to have exactly 1 host entry. Additionally, crypto/ssh's ParseKnownHosts rejects lines without any hosts so the list should never be empty to begin with.

If this is still problematic for code style reasons I'm happy to add more checks, though.

lib/client/known_hosts_migrate.go Outdated Show resolved Hide resolved
Host *.{{ .clusterName }} !{{ .proxyHost }}
Port 3022
{{- if .leaf }}
ProxyCommand ssh -p {{ .proxyPort }} {{ .proxyHost }} -s proxy:$(echo %h | cut -d '.' -f 1):%p@{{ .clusterName }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can $(echo %h | cut -d '.' -f 1) be done by Go code instead and pasted here as a template variable?

Same below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible but would require another subcommand of some sort to be built into tsh. Per advice from @russjones we've decided to defer for a bit as the upcoming tsh connect functionality could supersede any text manipulation we do.

tool/tsh/config.go Outdated Show resolved Hide resolved
tool/tsh/config.go Outdated Show resolved Hide resolved
@timothyb89 timothyb89 force-pushed the timothyb89/ssh-config-helper branch from 3045327 to 8edfa87 Compare July 16, 2021 17:49
Copy link
Contributor

@xacrimon xacrimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise on the implementation

lib/client/known_hosts_migrate.go Outdated Show resolved Hide resolved
lib/client/keystore.go Show resolved Hide resolved
Copy link
Contributor

@russjones russjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bot.

@timothyb89 timothyb89 enabled auto-merge (squash) July 22, 2021 19:40
@timothyb89 timothyb89 merged commit 9a9562b into master Jul 22, 2021
@timothyb89 timothyb89 deleted the timothyb89/ssh-config-helper branch July 22, 2021 20:40
timothyb89 added a commit that referenced this pull request Jul 22, 2021
* Add `tsh config ssh` helper to generate OpenSSH client configuration

This adds a new subcommand, `tsh config ssh`, to generate OpenSSH
client configuration snippets that allow users to connect directly to
nodes using the standard `ssh` client.

To support this change, tsh's `known_hosts` file has been modified to
match the format required by OpenSSH when verifying hosts against
certificates. Old-style `known_hosts` entries will be automatically
replaced and pruned when the end user first logs in with an updated
`tsh`. Small changes were additionally made to the keystore and key
agent to pass the proxy host into `AddKnownHostKeys` and to support
wildcard hostnames in `known_hosts` entries.

* Fix broken link to Trusted Clusters documentation

* Use text/template for SSH config generation; wrap all errors.

* Rename config helper from `config ssh` to just `config`

This changes the config helper to use just `tsh config` per
suggestion from @r0mant.

* Fix known_hosts_migrate_test after rebase

* First pass at review feedback

* Update docs/pages/server-access/guides/openssh.mdx

Co-authored-by: Roman Tkachenko <[email protected]>

* Ensure top-level hostnames never match wildcard patterns

* Add additional host count check to `canPruneOldHostsEntry`.

* Replace excess call to `isOldStyleHostsEntry` with documented invariant

* Trim trailing dots on absolute hostnames in `matchesWildcard`

Co-authored-by: Roman Tkachenko <[email protected]>
timothyb89 added a commit that referenced this pull request Jul 23, 2021
…) (#7651)

* Add `tsh config ssh` helper to generate OpenSSH client configuration

This adds a new subcommand, `tsh config ssh`, to generate OpenSSH
client configuration snippets that allow users to connect directly to
nodes using the standard `ssh` client.

To support this change, tsh's `known_hosts` file has been modified to
match the format required by OpenSSH when verifying hosts against
certificates. Old-style `known_hosts` entries will be automatically
replaced and pruned when the end user first logs in with an updated
`tsh`. Small changes were additionally made to the keystore and key
agent to pass the proxy host into `AddKnownHostKeys` and to support
wildcard hostnames in `known_hosts` entries.

* Fix broken link to Trusted Clusters documentation

* Use text/template for SSH config generation; wrap all errors.

* Rename config helper from `config ssh` to just `config`

This changes the config helper to use just `tsh config` per
suggestion from @r0mant.

* Fix known_hosts_migrate_test after rebase

* First pass at review feedback

* Update docs/pages/server-access/guides/openssh.mdx

Co-authored-by: Roman Tkachenko <[email protected]>

* Ensure top-level hostnames never match wildcard patterns

* Add additional host count check to `canPruneOldHostsEntry`.

* Replace excess call to `isOldStyleHostsEntry` with documented invariant

* Trim trailing dots on absolute hostnames in `matchesWildcard`

Co-authored-by: Roman Tkachenko <[email protected]>

Co-authored-by: Roman Tkachenko <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide an easy option to setup OpenSSH Config.
6 participants