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

use configured algorithms for dynamic SSH host certs #46329

Merged
merged 3 commits into from
Sep 11, 2024

Conversation

nklaassen
Copy link
Contributor

@nklaassen nklaassen commented Sep 6, 2024

This PR updates the SSH host keys/certs generated in lib/reversetunnel/cache.go to use the new configurable key algorithms described in RFD 136.

To support this change, we need to stop writing a static list of supported HostKeyAlgorithms that excludes Ed25519 and ECDSA host certs into our generated OpenSSH config files. The defaults in recent OpenSSH versions are pretty good - we were actually making them unsafe by always including [email protected].

We have been specifically including HostKeyAlgorithms [email protected] because OpenSSH disabled it by default before Go added server-side support for the newer SHA-2 based hashes. It seems safe to remove the entire HostKeyAlgorithms line in tsh v17:

  • with tsh v17 the oldest proxy you should be dialing is also v17, which definitely supports the new RSA SHA-2 algorithms
  • All OpenSSH clients should be able to dial to a Teleport proxy without any HostKeyAlgorithms line:
    • newer OpenSSH clients can use a newer algorithm (RSA with SHA-2, or Ed25519, or ECDSA)
    • older OpenSSH clients can continue to use [email protected] by default if the cluster uses the legacy algorithm suite
  • very old OpenSSH clients may not be able to connect to a Teleport SSH server using a newer signature algorithm suite. These would have to be quite old, Ed25519 and ECDSA support have been around longer than RSA with SHA-2 support. Users on these ancient versions will need to update OpenSSH or use the legacy signature algorithm suite in their cluster.

changelog: Removed HostKeyAlgorithms from generated OpenSSH config files

@github-actions github-actions bot added machine-id size/sm tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Sep 6, 2024
@nklaassen
Copy link
Contributor Author

@jakule do I need to keep the HostKeyAlgorithms line around for some third party lib? If so could I add to the defaults with + instead of overwriting them? #16912 (comment)

Comment on lines -323 to -325
// Deprecated: this block will be removed in v17. It exists so users can
// revert to the old behavior if necessary.
// TODO(strideynet) DELETE IN 17.0.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@strideynet confirming, am I good to delete this for 17?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup - all good to delete :)

@gravitational gravitational deleted a comment from github-actions bot Sep 6, 2024
@nklaassen nklaassen force-pushed the nklaassen/openssh-host-algs branch 2 times, most recently from 2e879e0 to c3d894b Compare September 6, 2024 05:01
@jakule
Copy link
Contributor

jakule commented Sep 6, 2024

@jakule do I need to keep the HostKeyAlgorithms line around for some third party lib? If so could I add to the defaults with + instead of overwriting them? #16912 (comment)

This line was needed to make Teleport in Proxy recording more work nicely with OpenSSH 7.4 (default in CentOS 7). I'm not sure if we care about the Proxy recording mode anymore since we replace it with agentless. Since then also Go crypto added a few patches to be compatible with older buggy OpenSSH.

The bottom line is, I'm not sure. I'd be very happy to remove it, but we can still break some compatibility.

@nklaassen
Copy link
Contributor Author

@jakule do I need to keep the HostKeyAlgorithms line around for some third party lib? If so could I add to the defaults with + instead of overwriting them? #16912 (comment)

This line was needed to make Teleport in Proxy recording more work nicely with OpenSSH 7.4 (default in CentOS 7). I'm not sure if we care about the Proxy recording mode anymore since we replace it with agentless. Since then also Go crypto added a few patches to be compatible with older buggy OpenSSH.

The bottom line is, I'm not sure. I'd be very happy to remove it, but we can still break some compatibility.

I think agentless is probably equivalent to proxy recording mode here since it's still terminating the incoming SSH connection at the proxy. But yeah with those crypto/ssh patches to support the buggy SSH clients I think it will work now without the HostKeyAlgorithms line. Hopefully people are using a recent proxy with those patches if they are using tsh v17 (and i think we enforce that now). I'll try to find some time to test this out with an openssh7.4 client to confirm

@nklaassen nklaassen force-pushed the nklaassen/openssh-host-algs branch from c3d894b to d814bc0 Compare September 9, 2024 17:29
Comment on lines -1056 to -1061
// auth and proxy benefit from precomputing keys since they can experience spikes in key
// generation due to web session creation and recorded session creation respectively.
// for all other agents precomputing keys consumes excess resources.
if cfg.Auth.Enabled || cfg.Proxy.Enabled {
native.PrecomputeKeys()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have switched to lazily starting the RSA key precomputation where we actually generate keys for web sessions or proxy-recorded sesssions/agentless sessions. It will now only start the first time we generate an RSA key, which will be never for the newer algorithm suites and should be acceptable even when using the legacy suite. Importantly, most tests will now never start the RSA key precomputation which will be a huge win for saving CPU time.

Copy link
Contributor

Choose a reason for hiding this comment

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

This specific check was added to prevent agents from precomputing keys(#14021). Do the changes made here limit lazy precomputing of keys to auth and proxy as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, because now we exclusively start the key precomputation closer to where the keys are actually needed, in newWebSession or generateHostCert, which will only be called on auth or proxy

@nklaassen
Copy link
Contributor Author

@rosstimothy can I get an ExcludeFlake for TestTSHConfigConnectWithOpenSSHClient which is a ~16s test. It's getting slightly faster with the combination of my currently open PRs eliminating RSA but it's still slow for a few reasons:

  • calling out to OpenSSH is slow (for some reason it takes a while to exit after the command completes)
  • can't parallelize the subtests because they create a cluster with different session recording and networking configs, and each test needs to login and have tsh called via openssh pick up the right TELEPORT_HOME

@rosstimothy
Copy link
Contributor

/excludeflake TestTSHConfigConnectWithOpenSSHClient

@nklaassen nklaassen enabled auto-merge September 11, 2024 19:11
@nklaassen nklaassen added this pull request to the merge queue Sep 11, 2024
Merged via the queue into master with commit c15825c Sep 11, 2024
42 checks passed
@nklaassen nklaassen deleted the nklaassen/openssh-host-algs branch September 11, 2024 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
machine-id size/sm tsh tsh - Teleport's command line tool for logging into nodes running Teleport.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants