Add `tbot proxy` and `tbot db` wrapper commands #12687

timothyb89 · 2022-05-17T01:15:10Z

This adds new wrapper commands that leverage tsh for proxy and database access.

It also adds a new tshwrap helper package which contains utilities for locating the tsh executable, checking its version, and loading all necessary data (certificates, destinations, etc) that will need to be passed to tsh for wrapped commands to function.

Usage examples:

SSH: ssh -F tbot-user/ssh_config user@host.example.com
(should now work both with and without the legacy SSH proxy port enabled)
Database connect: tbot db --destination-dir=./tbot-user --proxy=proxy.example.com connect db-name
Database proxy: tbot proxy --destination-dir=./tbot-user --proxy=proxy.example.com:3080 db db-name
Database proxy with authenticated tunnel: tbot proxy --destination-dir=./tbot-user --proxy=proxy.example.com:3080 --tunnel db-name

Fixes #11445

This enhances support for identity files in tsh, which previously only worked for regular SSH access. The largest blocker for support is that tsh uses profiles for all non-SSH resource access, and profiles have a direct mapping to some on-disk resources. This patch works around this in a few ways: * Virtual profiles: When an identity file is specified with `-i`, we use it to create an in-memory virtual profile using the cert as the root identity _and_ for every `RouteToDatabase` (and in the future, app) field contained in the cert. * Virtual profile paths: Certain profile operations require paths to valid certificates and other resources on disk, which may not exist inside the identity file. As the driving use-case for this change is integration with Machine ID, we can "cheat" and pass the correct paths to tsh via environment variables. A cooperating wrapper in `tbot` will execute `tsh` with appropriate flags and environment variables, which override tsh's usual certifiate paths. This allows commands like `tsh db connect ...` to work as expected. * Key stores: previously we used a `noLocalKeyStore{}` with which all lookups fail. This patch replaces it with an in-memory keystore if a client key is available. * Profile status: lastly, we add a new `StatusCurrentWithIdentity()` function to load virtual profiles where supported. Some commands are not supported in this PR (like `tsh app ...`), but others don't make sense to support (like cert reissuing). We might consider merging everything into the traditional `StatusCurrent()` when adding app support. App access is still broken and will be addressed in a later change. Partially fixes #11770

timothyb89 · 2022-05-17T01:15:48Z

Outstanding TODOs:

Merge Extend support for identity files in tsh #12686
Use tbot proxy ssh in generated ssh_config

This adds new wrapper commands that leverage tsh for proxy and database access. It also adds a new `tshwrap` helper package which contains utilities for locating the tsh executable, checking its version, and loading all necessary data (certificates, destinations, etc) that will need to be passed to tsh for wrapped commands to function.

Additionally, log a warning when environment variable paths aren't found.

…nded-identityfiles

Co-authored-by: Krzysztof Skrzętnicki <krzysztof.skrzetnicki@goteleport.com>

…nded-identityfiles

…tbot-proxy-wrappers

…xy-wrappers

…into timothyb89/tbot-proxy-wrappers

strideynet

This looks solid. There's a test failure for TestDefaultTemplateRendering which may be relevant though.

It might be easier to rebase this on top of #12855 when thats been merged rather than vice versa.

strideynet · 2022-05-25T07:46:41Z

tool/tbot/config/configtemplate_ssh_client.go

+// sshConfigUnsupportedWarning is used to ensure we don't spam log messages if
+// using non-filesystem backends.
+var sshConfigUnsupportedWarning sync.Once
+
 func (c *TemplateSSHClient) Render(ctx context.Context, authClient auth.ClientI, currentIdentity *identity.Identity, destination *DestinationConfig) error {


It would be super nice to include a unit-test for this function using the golden helper, so we can easily see what changes to the logic result in what changes to the generated file in PRs though. More than willing to provide some advice on this, or I will do it post this PR being merged.

I've gone ahead and done this in #12898 . It should be simple enough to pull that commit into this branch, or approve and merge that PR and then rebase onto it.

You'll need to run the following to regenerate the golden file for your changes in this PR:

GOLDEN_UPDATE=1 go test -run ^TestTemplateSSHClient_Render$ github.com/gravitational/teleport/tool/tbot/config

I am a big fan of making the result of these changes obvious during review. Any objections if we try to get this in after v9.3?

Ah, yep, I've fixed the failing test, making ssh_config optional broke it.

I do like the golden approach but let's aim to include that post-v9.3.

tool/tbot/tshwrap/wrap.go

timothyb89 · 2022-05-25T17:46:58Z

It might be easier to rebase this on top of #12855 when thats been merged rather than vice versa.

A caveat here is that we are looking to squeeze this into v9.3 (end of this week). I have mixed feelings about also squeezing in and backporting a refactoring patch, especially with the timeline constraints. What do you think?

tool/tbot/main.go

tool/tbot/proxy.go

strideynet · 2022-05-25T19:32:32Z

It might be easier to rebase this on top of #12855 when thats been merged rather than vice versa.

A caveat here is that we are looking to squeeze this into v9.3 (end of this week). I have mixed feelings about also squeezing in and backporting a refactoring patch, especially with the timeline constraints. What do you think?

If that's the case, just get that into 9.3 :)

This fixes a failing unit test by making the description for `ssh_config` match its behavior in practice. This necessitated passing the destination to all templates, unfortunately.

codingllama

Apologies for the delay.

tool/tbot/config/configtemplate_ssh_client.go

codingllama · 2022-05-26T21:13:28Z

tool/tbot/config/configtemplate.go

@@ -88,7 +88,7 @@ type Template interface {
 	// statically as this must be callable without any auth clients (or any
 	// secrets) for use with `tbot init`. If an arbitrary number of files must
 	// be generated, they should be placed in a subdirectory.
-	Describe() []FileDescription
+	Describe(destination destination.Destination) []FileDescription


Note for some other time: "destination.Destination" stutters, "destination destination.Destination" triply so. When you get some spare cycles (if ever), consider renaming package and/or type to a combination that doesn't stutters and takes better advantage of both package and type name.

https://github.com/golang/go/wiki/CodeReviewComments#package-names

It's come up before and I agree that it's ugly, but this practically needs to live in its own package to avoid import cycles. Since it's the only thing that will ever live in that package, there's few names that would make sense other than, well, destination. Always open to suggestions if you've got a more creative solution to offer 🙂

Having a quick look the only implementor I found was DestinationDirectory. The destination package holds an interface and nothing else, which is a bit of a design smell for Go.

I would suggest the following:

Rename package destination to package destinations. The main reason is that we can have variables named destination without overshadowing the package name (similar to strings or bytes).

Move DestinationDirectory to destinations.Directory. Names fit nicely and the package now exports something concrete. (IIUC, this won't cause import cycles.)

Ditch the interface, or at the very least ditch it from the destinations package. If you do need an interface, spring up one where it's used, preferably with only 1 or 2 methods that the code cares about.

We don't usually need "preemptive" interfaces in Go. Code against the concrete type if you can; if you need an interface, spit it out where it's needed, and only after it has a reason to be there. (The api/ package is the one place where we may want to do differently, but for internal code this should work just fine.)

tool/tbot/tshwrap/wrap_test.go

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

codingllama

Just a few ramblings / minor suggestions. Thanks for the fixes.

codingllama · 2022-05-27T14:02:21Z

tool/tbot/main.go

+	// Remaining args are stored directly to a []string rather than written to
+	// a shared ref like most other kingpin args, so we'll need to manually
+	// move them to the remaining args field.
+	if len(*dbRemaining) > 0 {


Kingpin is a bit of a strange choice, cobra is much more common nowadays (I think it leads to better code too, no need for that "what's the command" switch). I assume Teleport predates cobra, hence kingpin.

Just idle musings, no actions expected here.

tool/tbot/db.go

tool/tbot/tshwrap/wrap.go

codingllama · 2022-05-27T14:07:39Z

tool/tbot/tshwrap/wrap.go

+const (
+	// TSHVarName is the name of the environment variable that can override the
+	// tsh path that would otherwise be located on the $PATH.
+	TSHVarName = "TSH"


That's fair. Maybe tshwrap.EnvVarName would cut it, but it's fine as-is.

tool/tbot/config/configtemplate_cockroach.go

tool/tbot/tshwrap/wrap.go

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

* Extend support for identity files in tsh This enhances support for identity files in tsh, which previously only worked for regular SSH access. The largest blocker for support is that tsh uses profiles for all non-SSH resource access, and profiles have a direct mapping to some on-disk resources. This patch works around this in a few ways: * Virtual profiles: When an identity file is specified with `-i`, we use it to create an in-memory virtual profile using the cert as the root identity _and_ for every `RouteToDatabase` (and in the future, app) field contained in the cert. * Virtual profile paths: Certain profile operations require paths to valid certificates and other resources on disk, which may not exist inside the identity file. As the driving use-case for this change is integration with Machine ID, we can "cheat" and pass the correct paths to tsh via environment variables. A cooperating wrapper in `tbot` will execute `tsh` with appropriate flags and environment variables, which override tsh's usual certifiate paths. This allows commands like `tsh db connect ...` to work as expected. * Key stores: previously we used a `noLocalKeyStore{}` with which all lookups fail. This patch replaces it with an in-memory keystore if a client key is available. * Profile status: lastly, we add a new `StatusCurrentWithIdentity()` function to load virtual profiles where supported. Some commands are not supported in this PR (like `tsh app ...`), but others don't make sense to support (like cert reissuing). We might consider merging everything into the traditional `StatusCurrent()` when adding app support. App access is still broken and will be addressed in a later change. Partially fixes #11770 * Fix failing lint * Add `tbot proxy` and `tbot db` wrapper commands This adds new wrapper commands that leverage tsh for proxy and database access. It also adds a new `tshwrap` helper package which contains utilities for locating the tsh executable, checking its version, and loading all necessary data (certificates, destinations, etc) that will need to be passed to tsh for wrapped commands to function. * Fix failing unit test due to incorrect default IsVirtual profile flag * Combine `StatusCurrentWithIdentity()` into `StatusCurrent()` Additionally, log a warning when environment variable paths aren't found. * Fix virtual profile flag always being true * Update lib/client/api.go Co-authored-by: Krzysztof Skrzętnicki <krzysztof.skrzetnicki@goteleport.com> * Address review feedback * Use `tbot proxy` in generated `ssh_config` * Add tests for mockable parts of our tsh integration * Fix lints * Clarify docstrings in CLIConf * Tweak comment for clarity; fix typo in `onProxyCommand` * Add missing copyright header * Fix failing unit test and pass destination to `Describe()` This fixes a failing unit test by making the description for `ssh_config` match its behavior in practice. This necessitated passing the destination to all templates, unfortunately. * Add a few extra tests * Apply suggestions from code review Co-authored-by: Alan Parra <alan.parra@goteleport.com> * Address another batch of review comments * Comment tweaks * Refactor tshwrap to remove the Runner interface. * Apply suggestions from code review Co-authored-by: Alan Parra <alan.parra@goteleport.com> * Address review comments Co-authored-by: Krzysztof Skrzętnicki <krzysztof.skrzetnicki@goteleport.com> Co-authored-by: Alan Parra <alan.parra@goteleport.com>

* Extend support for identity files in tsh This enhances support for identity files in tsh, which previously only worked for regular SSH access. The largest blocker for support is that tsh uses profiles for all non-SSH resource access, and profiles have a direct mapping to some on-disk resources. This patch works around this in a few ways: * Virtual profiles: When an identity file is specified with `-i`, we use it to create an in-memory virtual profile using the cert as the root identity _and_ for every `RouteToDatabase` (and in the future, app) field contained in the cert. * Virtual profile paths: Certain profile operations require paths to valid certificates and other resources on disk, which may not exist inside the identity file. As the driving use-case for this change is integration with Machine ID, we can "cheat" and pass the correct paths to tsh via environment variables. A cooperating wrapper in `tbot` will execute `tsh` with appropriate flags and environment variables, which override tsh's usual certifiate paths. This allows commands like `tsh db connect ...` to work as expected. * Key stores: previously we used a `noLocalKeyStore{}` with which all lookups fail. This patch replaces it with an in-memory keystore if a client key is available. * Profile status: lastly, we add a new `StatusCurrentWithIdentity()` function to load virtual profiles where supported. Some commands are not supported in this PR (like `tsh app ...`), but others don't make sense to support (like cert reissuing). We might consider merging everything into the traditional `StatusCurrent()` when adding app support. App access is still broken and will be addressed in a later change. Partially fixes #11770 * Fix failing lint * Add `tbot proxy` and `tbot db` wrapper commands This adds new wrapper commands that leverage tsh for proxy and database access. It also adds a new `tshwrap` helper package which contains utilities for locating the tsh executable, checking its version, and loading all necessary data (certificates, destinations, etc) that will need to be passed to tsh for wrapped commands to function. * Fix failing unit test due to incorrect default IsVirtual profile flag * Combine `StatusCurrentWithIdentity()` into `StatusCurrent()` Additionally, log a warning when environment variable paths aren't found. * Fix virtual profile flag always being true * Update lib/client/api.go Co-authored-by: Krzysztof Skrzętnicki <krzysztof.skrzetnicki@goteleport.com> * Address review feedback * Use `tbot proxy` in generated `ssh_config` * Add tests for mockable parts of our tsh integration * Fix lints * Clarify docstrings in CLIConf * Tweak comment for clarity; fix typo in `onProxyCommand` * Add missing copyright header * Fix failing unit test and pass destination to `Describe()` This fixes a failing unit test by making the description for `ssh_config` match its behavior in practice. This necessitated passing the destination to all templates, unfortunately. * Add a few extra tests * Apply suggestions from code review Co-authored-by: Alan Parra <alan.parra@goteleport.com> * Address another batch of review comments * Comment tweaks * Refactor tshwrap to remove the Runner interface. * Apply suggestions from code review Co-authored-by: Alan Parra <alan.parra@goteleport.com> * Address review comments Co-authored-by: Krzysztof Skrzętnicki <krzysztof.skrzetnicki@goteleport.com> Co-authored-by: Alan Parra <alan.parra@goteleport.com> Co-authored-by: Krzysztof Skrzętnicki <krzysztof.skrzetnicki@goteleport.com> Co-authored-by: Alan Parra <alan.parra@goteleport.com>

timothyb89 added 2 commits May 16, 2022 19:18

Fix failing lint

8163f73

timothyb89 force-pushed the timothyb89/tbot-proxy-wrappers branch from 325def1 to be0153b Compare May 17, 2022 01:19

timothyb89 and others added 5 commits May 16, 2022 19:31

Fix failing unit test due to incorrect default IsVirtual profile flag

bae4479

Combine StatusCurrentWithIdentity() into StatusCurrent()

74a61e3

Additionally, log a warning when environment variable paths aren't found.

Fix virtual profile flag always being true

a8ceb70

Merge remote-tracking branch 'origin/master' into timothyb89/tsh-expa…

3b0ed1e

…nded-identityfiles

Update lib/client/api.go

8080cd5

Co-authored-by: Krzysztof Skrzętnicki <krzysztof.skrzetnicki@goteleport.com>

timothyb89 mentioned this pull request May 24, 2022

Extend support for identity files in tsh #12686

Merged

timothyb89 added 3 commits May 24, 2022 15:16

Address review feedback

c8e1108

Merge remote-tracking branch 'origin/master' into timothyb89/tsh-expa…

c76b4a3

…nded-identityfiles

Merge branch 'timothyb89/tsh-expanded-identityfiles' into timothyb89/…

0d4f12a

…tbot-proxy-wrappers

Base automatically changed from timothyb89/tsh-expanded-identityfiles to master May 24, 2022 21:58

timothyb89 added 5 commits May 24, 2022 17:16

Use tbot proxy in generated ssh_config

7efc778

Merge branch 'master' into timothyb89/tbot-proxy-wrappers

5e29813

Add tests for mockable parts of our tsh integration

e2be3c9

Merge remote-tracking branch 'origin/master' into timothyb89/tbot-pro…

5dbc1e2

…xy-wrappers

Merge remote-tracking branch 'origin/timothyb89/tbot-proxy-wrappers' …

d04d8bd

…into timothyb89/tbot-proxy-wrappers

timothyb89 marked this pull request as ready for review May 25, 2022 00:22

github-actions bot requested review from codingllama and ibeckermayer May 25, 2022 00:23

timothyb89 requested a review from strideynet May 25, 2022 00:28

timothyb89 added the backport-required label May 25, 2022

timothyb89 added 2 commits May 24, 2022 18:31

Fix lints

54cf48b

Clarify docstrings in CLIConf

ac29966

strideynet approved these changes May 25, 2022

View reviewed changes

strideynet reviewed May 25, 2022

View reviewed changes

tool/tbot/tshwrap/wrap.go Show resolved Hide resolved

tool/tbot/tshwrap/wrap.go Show resolved Hide resolved

tool/tbot/tshwrap/wrap.go Show resolved Hide resolved

strideynet mentioned this pull request May 25, 2022

Demonstrate usage of golden for tbot template generation tests. #12898

Merged

ibeckermayer reviewed May 25, 2022

View reviewed changes

tool/tbot/main.go Outdated Show resolved Hide resolved

tool/tbot/proxy.go Outdated Show resolved Hide resolved

timothyb89 added 4 commits May 25, 2022 14:17

Tweak comment for clarity; fix typo in onProxyCommand

085b50c

Add missing copyright header

d16ed03

Fix failing unit test and pass destination to Describe()

bc5aa4d

This fixes a failing unit test by making the description for `ssh_config` match its behavior in practice. This necessitated passing the destination to all templates, unfortunately.

Add a few extra tests

8743115

timothyb89 requested a review from ibeckermayer May 26, 2022 16:27

codingllama reviewed May 26, 2022

View reviewed changes

timothyb89 and others added 4 commits May 26, 2022 17:05

Apply suggestions from code review

bb6e458

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

Address another batch of review comments

afb18a2

Comment tweaks

a9dd70b

Refactor tshwrap to remove the Runner interface.

da9dede

codingllama approved these changes May 27, 2022

View reviewed changes

timothyb89 and others added 2 commits May 27, 2022 10:44

Apply suggestions from code review

a75aa23

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

Address review comments

0225c97

timothyb89 enabled auto-merge (squash) May 27, 2022 17:24

Merge branch 'master' into timothyb89/tbot-proxy-wrappers

ef91fb3

timothyb89 merged commit 8f36b9c into master May 27, 2022

timothyb89 mentioned this pull request May 27, 2022

[v9] Add tbot proxy and tbot db wrapper commands (#12687) #12990

Merged

webvictim mentioned this pull request Jun 8, 2022

Opened in error #13285

Closed

zmb3 deleted the timothyb89/tbot-proxy-wrappers branch April 26, 2023 21:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `tbot proxy` and `tbot db` wrapper commands #12687

Add `tbot proxy` and `tbot db` wrapper commands #12687

timothyb89 commented May 17, 2022 •

edited

Loading

timothyb89 commented May 17, 2022 •

edited

Loading

strideynet left a comment

strideynet May 25, 2022 •

edited

Loading

strideynet May 25, 2022 •

edited

Loading

timothyb89 May 25, 2022

timothyb89 May 25, 2022

timothyb89 commented May 25, 2022

strideynet commented May 25, 2022

codingllama left a comment

codingllama May 26, 2022

timothyb89 May 27, 2022

codingllama May 27, 2022

codingllama left a comment

codingllama May 27, 2022

codingllama May 27, 2022

Add tbot proxy and tbot db wrapper commands #12687

Add tbot proxy and tbot db wrapper commands #12687

Conversation

timothyb89 commented May 17, 2022 • edited Loading

timothyb89 commented May 17, 2022 • edited Loading

strideynet left a comment

Choose a reason for hiding this comment

strideynet May 25, 2022 • edited Loading

Choose a reason for hiding this comment

strideynet May 25, 2022 • edited Loading

Choose a reason for hiding this comment

timothyb89 May 25, 2022

Choose a reason for hiding this comment

timothyb89 May 25, 2022

Choose a reason for hiding this comment

timothyb89 commented May 25, 2022

strideynet commented May 25, 2022

codingllama left a comment

Choose a reason for hiding this comment

codingllama May 26, 2022

Choose a reason for hiding this comment

timothyb89 May 27, 2022

Choose a reason for hiding this comment

codingllama May 27, 2022

Choose a reason for hiding this comment

codingllama left a comment

Choose a reason for hiding this comment

codingllama May 27, 2022

Choose a reason for hiding this comment

codingllama May 27, 2022

Choose a reason for hiding this comment

Add `tbot proxy` and `tbot db` wrapper commands #12687

Add `tbot proxy` and `tbot db` wrapper commands #12687

timothyb89 commented May 17, 2022 •

edited

Loading

timothyb89 commented May 17, 2022 •

edited

Loading

strideynet May 25, 2022 •

edited

Loading

strideynet May 25, 2022 •

edited

Loading