UX improvements for tbot by timothyb89 · Pull Request #10833 · gravitational/teleport

timothyb89 · 2022-03-04T01:14:59Z

A last batch of UX tweaks for 9.0:

rename --renew-interval to --renewal-interval
add --oneshot mode to fetch one set of certs and exit (client side only, no server enforcement yet)
add tbot version (fixes tbot version returns an error #10782)
add unix signal handling: graceful exit on SIGINT, reload on SIGHUP/SIGUSR1
make auth server an optional config option and check it only when needed (i.e. tbot start)
add cut workaround to allow SSH to work without DNS (fixes Add support for cut to ProxyCommand #10813)
fix product name in CLI help (fixes Update tbot name in CLI #10926)
add --format=json to tctl bots add (fixes Add support for --format=json flag to tctl bots add #10783)
check SSH version and exclude the RSA deprecation workaround for older SSH clients (fixes Machine ID ssh_config generation for older versions of OpenSSH #10781)

timothyb89 · 2022-03-04T01:18:27Z

I know there's a few other renames being discussed in #10030

I do like the -lifetime suffix and can still make that change tctl/tbot if we'd like.

For context, here's the tbot start syntax as of this PR:

$ ./tbot start --help
usage: tbot start [<flags>]

Starts the renewal bot, writing certificates to the data dir at a set interval.

Flags:
  -d, --debug             Verbose logging to stdout
  -c, --config            tbot.yaml path
  -a, --auth-server       Specify the Teleport auth server host
      --token             A bot join token, if attempting to onboard a new bot; used on first connect.
      --ca-pin            A repeatable auth server CA hash to pin; used on first connect.
      --data-dir          Directory to store internal bot data.
      --destination-dir   Directory to write generated certificates
      --certificate-ttl   TTL of generated certificates
      --renewal-interval  Interval at which certificates are renewed; must be less than the certificate TTL.
      --join-method       Method to use to join the cluster.
      --oneshot           If set, quit after the first renewal.

tbot init:

usage: tbot init --bot-user=BOT-USER [<flags>]

Initialize a certificate destination directory for writes from a separate bot user.

Flags:
  -d, --debug            Verbose logging to stdout
  -c, --config           tbot.yaml path
      --destination-dir  If NOT using a config file, specify the destination directory.
      --init-dir         If using a config file and multiple destinations are configured, specify which to initialize
      --clean            If set, remove unexpected files and directories from the destination
      --bot-user         Name of the bot Unix user which should have write access to the destination.

Aliases:

tctl bots add:

usage: tctl bots add --roles=ROLES [<flags>] <name>

Add a new certificate renewal bot to the cluster.

Flags:
  -d, --debug        Enable verbose logging to stderr
  -c, --config       Path to a configuration file [/etc/teleport.yaml]. Can also be set via the TELEPORT_CONFIG_FILE environment variable.
      --auth-server  Attempts to connect to specific auth/proxy address(es) instead of local auth [127.0.0.1:3025]
  -i, --identity     Path to an identity file. Must be provided to make remote connections to auth. An identity file can be exported with 'tctl auth sign'
      --insecure     When specifying a proxy address in --auth-server, do not verify its TLS certificate. Danger: any data you send can be intercepted or modified by an attacker.
      --roles        Roles the bot is able to assume.
      --ttl          TTL for the bot join token.
      --token        Name of an existing token to use.

Args:
  <name>  A name to uniquely identify this bot in the cluster.

Aliases:

benarent · 2022-03-08T05:03:09Z

@timothyb89 When trying the above, I'm unable to use --token when using tctl bots add

tctl bots add --roles=access --token=blah terminator
ERROR: unknown long flag '--token'

timothyb89 · 2022-03-08T06:14:03Z

@timothyb89 When trying the above, I'm unable to use --token when using tctl bots add
tctl bots add --roles=access --token=blah terminator
ERROR: unknown long flag '--token'

Hmm, that definitely should work. My initial guess is that you might have an outdated tctl, but I'll dig into this more tomorrow.

zmb3 · 2022-03-09T17:59:21Z

 const exampleConfigFile = `
 auth_server: auth.example.com
-renew_interval: 5m
+renewal_interval: 5m


Are there any docs that need to be updated for this rename?

Looks like the docs PR (#10775) doesn't refer to this config parameter at all, luckily.

zmb3 · 2022-03-09T18:02:46Z


 func onStart(botConfig *config.BotConfig) error {
+	if botConfig.AuthServer == "" {
+		return trace.BadParameter("An auth server must be set via --auth-server or configuration")


Do we support tunneling through proxy? If so, I recommend we change the wording to mention that it can be an auth or proxy server.

Good point, will reword.

zmb3 · 2022-03-09T18:03:04Z

@@ -158,9 +173,12 @@ func onStart(botConfig *config.BotConfig) error {
 	var authClient auth.ClientI

 	// TODO: graceful shutdown via signal; see #7066


Can you remove this TODO

codingllama · 2022-03-10T14:23:25Z

89 commits and 2.7k+ added lines is a tough review. Any chance we could split this into reasonably-sized parts?

timothyb89 · 2022-03-10T17:20:17Z

89 commits and 2.7k+ added lines is a tough review. Any chance we could split this into reasonably-sized parts?

Hrm, GitHub got confused over the merge base change, it's supposed to be just +300 or so. I'll try to fix it.

A last batch of UX tweaks for 9.0: - rename --renew-interval -> renewal-interval - add `--oneshot` mode to fetch one set of certs and exit (client side only, no server enforcement yet) - add `tbot version` - add unix signal handling: graceful exit on SIGINT, reload on SIGHUP/SIGUSR1 - make auth server an optional config option and check it only when needed (i.e. `tbot start`)

…orkaround

Co-authored-by: Zac Bergquist <zmb3@users.noreply.github.com>

timothyb89 · 2022-03-10T17:35:38Z

Alright, apologies for confusion, the diff should now be back under control. Hopefully +230/-50 is a little more sane 🙂

codingllama · 2022-03-10T17:42:50Z

+func parseSSHVersion(versionString string) (*semver.Version, error) {
+	versionTokens := strings.Split(versionString, " ")
+	if len(versionTokens) == 0 {
+		return nil, trace.Errorf("invalid version string: %s", versionString)


nit: prefer BadParameter to Errorf?

Suggested change

return nil, trace.Errorf("invalid version string: %s", versionString)

return nil, trace.BadParameter("invalid version string: %s", versionString)

Same for others.

codingllama · 2022-03-10T17:46:01Z

 		return trace.Wrap(err)
 	}

+	// Default to including the RSA deprecation workaround.


What is the RSA workaround? Do we have it explained somewhere?

I'll add an explainer comment to the IncludeRSAWorkaround docstring:

IncludeRSAWorkaround controls whether the RSA deprecation workaround is included in the generated configuration. Newer versions of OpenSSH deprecate RSA certificates and, due to a bug in golang's ssh package, Teleport wrongly advertises its unaffected certificates as a now-deprecated certificate type. The workaround includes a config override to re-enable RSA certs for just Teleport hosts, however it is only supported on OpenSSH 8.5 and later.

codingllama · 2022-03-10T17:51:52Z

@@ -144,6 +155,10 @@ func onWatch(botConfig *config.BotConfig) error {
 }

 func onStart(botConfig *config.BotConfig) error {


Not for this PR, but we should consider refactoring the bulk of this logic outside of package main and test it.

Yeah, there's a lot of cleanup and refactoring needed here. Hoping to prioritize that sooner rather than later 🙂

codingllama · 2022-03-10T17:55:46Z

Alright, apologies for confusion, the diff should now be back under control. Hopefully +230/-50 is a little more sane 🙂

Much better :)

Co-authored-by: Alan Parra <alan.parra@goteleport.com>

* UX improvements for tbot A last batch of UX tweaks for 9.0: - rename --renew-interval -> renewal-interval - add `--oneshot` mode to fetch one set of certs and exit (client side only, no server enforcement yet) - add `tbot version` - add unix signal handling: graceful exit on SIGINT, reload on SIGHUP/SIGUSR1 - make auth server an optional config option and check it only when needed (i.e. `tbot start`) * Remove `--auth-server` flag from `tbot init` example * Add `cut` workaround to allow connecting to nodes without DNS * Update product name in tbot CLI help * Add `--format=json` support to `tctl bots add` * Detect OpenSSH version and conditionally remove the RSA deprecation workaround * Fix failing unit test after rename * Update tool/tbot/config/configtemplate_ssh.go Co-authored-by: Zac Bergquist <zmb3@users.noreply.github.com> * Address review feedback * Apply suggestions from code review Co-authored-by: Alan Parra <alan.parra@goteleport.com> * Document IncludeRSAWorkaround and address review comments Co-authored-by: Zac Bergquist <zmb3@users.noreply.github.com> Co-authored-by: Alan Parra <alan.parra@goteleport.com>

* UX improvements for tbot A last batch of UX tweaks for 9.0: - rename --renew-interval -> renewal-interval - add `--oneshot` mode to fetch one set of certs and exit (client side only, no server enforcement yet) - add `tbot version` - add unix signal handling: graceful exit on SIGINT, reload on SIGHUP/SIGUSR1 - make auth server an optional config option and check it only when needed (i.e. `tbot start`) * Remove `--auth-server` flag from `tbot init` example * Add `cut` workaround to allow connecting to nodes without DNS * Update product name in tbot CLI help * Add `--format=json` support to `tctl bots add` * Detect OpenSSH version and conditionally remove the RSA deprecation workaround * Fix failing unit test after rename * Update tool/tbot/config/configtemplate_ssh.go Co-authored-by: Zac Bergquist <zmb3@users.noreply.github.com> * Address review feedback * Apply suggestions from code review Co-authored-by: Alan Parra <alan.parra@goteleport.com> * Document IncludeRSAWorkaround and address review comments Co-authored-by: Zac Bergquist <zmb3@users.noreply.github.com> Co-authored-by: Alan Parra <alan.parra@goteleport.com> Co-authored-by: Zac Bergquist <zmb3@users.noreply.github.com> Co-authored-by: Alan Parra <alan.parra@goteleport.com>

timothyb89 added machine-id merge-for-v9 labels Mar 4, 2022

timothyb89 force-pushed the timothyb89/tbot-ux branch 3 times, most recently from 2e2f690 to 5f8a8db Compare March 8, 2022 02:43

timothyb89 force-pushed the timothyb89/tbot-ux branch from 5f8a8db to fccb456 Compare March 8, 2022 23:31

timothyb89 mentioned this pull request Mar 8, 2022

Add support for --format=json flag to tctl bots add #10783

Closed

timothyb89 force-pushed the timothyb89/tbot-ux branch from 9c7e9a0 to a635966 Compare March 9, 2022 01:16

timothyb89 mentioned this pull request Mar 9, 2022

Machine ID ssh_config generation for older versions of OpenSSH #10781

Closed

zmb3 reviewed Mar 9, 2022

View reviewed changes

timothyb89 marked this pull request as ready for review March 9, 2022 18:20

github-actions Bot added the tctl tctl - Teleport admin tool label Mar 9, 2022

github-actions Bot requested review from codingllama and jakule March 9, 2022 18:20

r0mant mentioned this pull request Mar 9, 2022

Teleport 9.0 Release Plan #10447

Closed

38 tasks

timothyb89 force-pushed the timothyb89/tbot-ux branch 2 times, most recently from fa50d20 to 0e8c901 Compare March 10, 2022 02:19

Base automatically changed from timothyb89/tbot-init to master March 10, 2022 06:09

timothyb89 added 7 commits March 10, 2022 10:31

Remove --auth-server flag from tbot init example

dd15532

Add cut workaround to allow connecting to nodes without DNS

758d8de

Update product name in tbot CLI help

15bd716

Add --format=json support to tctl bots add

31cd6f3

Detect OpenSSH version and conditionally remove the RSA deprecation w…

59c7a3d

…orkaround

Fix failing unit test after rename

b57ab7a

timothyb89 and others added 2 commits March 10, 2022 10:31

Update tool/tbot/config/configtemplate_ssh.go

ef73b4a

Co-authored-by: Zac Bergquist <zmb3@users.noreply.github.com>

Address review feedback

82d1dee

timothyb89 force-pushed the timothyb89/tbot-ux branch from 0e8c901 to 82d1dee Compare March 10, 2022 17:32

timothyb89 added the backport-required label Mar 10, 2022

codingllama approved these changes Mar 10, 2022

View reviewed changes

timothyb89 and others added 2 commits March 10, 2022 11:03

Apply suggestions from code review

6be0619

Co-authored-by: Alan Parra <alan.parra@goteleport.com>

Document IncludeRSAWorkaround and address review comments

ce01786

jakule approved these changes Mar 10, 2022

View reviewed changes

timothyb89 enabled auto-merge (squash) March 10, 2022 19:57

Merge branch 'master' into timothyb89/tbot-ux

ccff239

zmb3 approved these changes Mar 10, 2022

View reviewed changes

timothyb89 merged commit bea5f7f into master Mar 10, 2022

timothyb89 deleted the timothyb89/tbot-ux branch March 10, 2022 20:41

This was referenced Mar 10, 2022

[v9] UX improvements for tbot (#10833) #11046

Merged

Certificate Bot Refresh and TTLs #10030

Closed

timothyb89 mentioned this pull request Mar 11, 2022

Bad configuration option: pubkeyacceptedalgorithms #10939

Closed

webvictim mentioned this pull request Apr 19, 2022

opened in error #12068

Closed

webvictim mentioned this pull request Jun 8, 2022

Opened in error #13285

Closed

		@@ -158,9 +173,12 @@ func onStart(botConfig *config.BotConfig) error {
		var authClient auth.ClientI

		// TODO: graceful shutdown via signal; see #7066

	return nil, trace.Errorf("invalid version string: %s", versionString)
	return nil, trace.BadParameter("invalid version string: %s", versionString)

		@@ -144,6 +155,10 @@ func onWatch(botConfig *config.BotConfig) error {
		}

		func onStart(botConfig *config.BotConfig) error {

Conversation

timothyb89 commented Mar 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timothyb89 commented Mar 4, 2022

Uh oh!

benarent commented Mar 8, 2022

Uh oh!

timothyb89 commented Mar 8, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codingllama commented Mar 10, 2022

Uh oh!

timothyb89 commented Mar 10, 2022

Uh oh!

timothyb89 commented Mar 10, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codingllama commented Mar 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

timothyb89 commented Mar 4, 2022 •

edited

Loading