Extend support for identity files in tsh #12686

timothyb89 · 2022-05-17T00:50:00Z

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 modify StatusCurrent() to accept an identity file path and load virtual profiles when set.

This particular change is primarily targets database support, but lays the groundwork for app and Kubernetes support. We'll work on support for them in future PRs.

Partially fixes #11770

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

Tener · 2022-05-17T05:23:36Z

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.

Can we use tsh daemon instead? This would have the benefit of sharing that capability with Teleport Connect, and we wouldn't have to invent some ad-hoc protocol, just use gRPC which is already in place.

Tener

A couple of high-level comments. In general the concept of "virtual profiles" looks complex enough to benefit from RFD - we are adding a lot of complexity here which will be costly in the long run.

Tener · 2022-05-17T05:35:15Z

tool/tsh/db.go

+
+	// Note: we don't use StatusCurrentWithIdentity here as virtual identity
+	// profiles are read-only.
+	profile, err := client.StatusCurrentWithIdentity(cf.HomePath, cf.Proxy, cf.IdentityFileIn)


Would it make sense to separate the concept of read-only profile from identity-based profile? Right now both are expressed with catch-all "virtual" designation, but it feels like a leaky abstraction.

It might, though I'm not sure what that use-case would be. As far as I know the only place read-only profiles make sense is identity-based profiles: there's no other way to get a certificate that has database/app/k8s/etc fields, and tsh only writes to request extra permissions for a particular database/app/etc.

Either you go the usual way through tsh (which is inherently read/write), or you use tctl auth sign ... / Machine ID in which case you bring an identity file with whichever additional fields you'd like. A read-only tsh profile wouldn't be able to request e.g. database certs in a very useful way.

Tener · 2022-05-17T05:36:53Z

tool/tsh/db.go

-		return trace.Wrap(err)
+	// Identity files contain themselves act as the database credentials (if
+	// any), so don't bother fetching new certs.
+	if profile.IsVirtual {


Is there a way to restructure the code so that we don't increase the complexity massively by having each and every operation support "virtual profiles" in explicit way?

I don't much like the verbosity either, but ultimately certain codepaths (like this one) do things that will fail when given an identity file. Arguably when using an identity file I don't think any additional certs should get written to disk, and moreover RetryWithRelogin() and IssueUserCertsWithMFA() will fail on Machine ID identity files as they're explicitly non-renewable.

If we (per your comment below) made RetryWithRelogin and friends identity-aware (i.e. effectively a no-op), we might be able to skip more of these checks. It's hard for me to predict if that'd be significantly simpler, though.

(and wow, that comment is unreadable, whoops...)

With the latest change I've merged everything into the one codepath (StatusCurrent()) and we have a total of 3 IsVirtual checks for core + database access.

I know app (and probably k8s) access will need some of their own when we circle back to support them properly, but I don't think this is especially onerous - worst case if some specific handling is missing it fails due to some identity-related error exactly as it would already today.

Overall, though, I think the alternative where we modify RetryWithRelogin to support identity files ends up quite a bit more complicated and I'd rather direct that effort to a proper refactoring where hacks like this aren't needed at all.

Tener · 2022-05-17T05:37:53Z

tool/tsh/db.go

+		log.Info("Note: already logged in due to an identity file (`-i ...`); will only update database config files.")
+	} else {
+		var key *client.Key
+		if err = client.RetryWithRelogin(cf.Context, tc, func() error {


Should we perhaps add virtual (read-only?) profile handling to RetryWithLogin to have uniform support across the various ops?

I'd like to give this a deeper look. My gut feeling is that it'd be similarly verbose: everything that issues certs would need to see the identity file, skip reissuing certs, and then store to memory rather than disk. We'd still have issues with profiles and especially the paths that get passed to e.g. database CLIs that are expected to be on disk.

I've looked a bit more, and I don't think we really save anything here: RetryWithRelogin() (and more specifically, client.Login()) just manages the core client identity which we already configure with the identity file at startup.

Database / Kubernetes / App access all use profile certs and never try to use the core identity even if it does actually have the proper permissions. Unfortunately the only shared codepath for these is the profiles system, which is what necessitated this hacky solution to begin with.

Ideally I'd like to see us invest in a larger refactoring to clean all of this up but frankly I don't even know what I'd want it to look like, tsh has a lot of functionality to juggle. In the mean time hopefully we can enable at least the Machine ID use-case.

Tener · 2022-05-17T05:40:12Z

tool/tsh/db.go

@@ -275,7 +292,7 @@ func onDatabaseConfig(cf *CLIConf) error {
 	if err != nil {
 		return trace.Wrap(err)
 	}
-	profile, err := client.StatusCurrent(cf.HomePath, cf.Proxy)
+	profile, err := client.StatusCurrentWithIdentity(cf.HomePath, cf.Proxy, cf.IdentityFileIn)


Seeing how many instances of StatusCurrent had to be updated I wonder if we can have a helper method taking in CLIConf instead? This way we can ensure consistency across the board and reduce the risk of someone using the wrong method (without cf.IdentityFileIn)

We could also merge identity file support into StatusCurrent() itself and have just the one function. It would mean adding a profile.IsVirtual check afterward in the few places that can't support virtual profiles (mainly anything that reissues certs).

I figured it'd be safer to keep the old codepath where (currently) unsupported, but it's an easy change to make.

I've gone ahead and merged all the functionality into StatusCurrent(). We need to pass along the identity path but it's easily available everywhere, so I think this should be modestly nicer overall.

timothyb89 · 2022-05-17T16:37:44Z

Can we use tsh daemon instead? This would have the benefit of sharing that capability with Teleport Connect, and we wouldn't have to invent some ad-hoc protocol, just use gRPC which is already in place.

To be honest I wasn't aware of that new functionality - it's definitely worth a look. I fear it'll have similar limitations for our needs on the Machine ID front, though. We specifically need the proxying functionality and without changes none of these tsh codepaths will simply trust an external identity without trying to do problematic things (like fetching new certs or reading/writing files).

Tener · 2022-05-17T19:10:27Z

Can we use tsh daemon instead? This would have the benefit of sharing that capability with Teleport Connect, and we wouldn't have to invent some ad-hoc protocol, just use gRPC which is already in place.

To be honest I wasn't aware of that new functionality - it's definitely worth a look. I fear it'll have similar limitations for our needs on the Machine ID front, though. We specifically need the proxying functionality and without changes none of these tsh codepaths will simply trust an external identity without trying to do problematic things (like fetching new certs or reading/writing files).

Yes, I think the changes will be needed, I just figured the wrapper looks similar to tsh daemon functionality, so perhaps worth sharing the code paths. Also, it may be that Teleport Connect will need use the same functionality either directly (with user-provided identities) or behind the scenes (as some sort of background mechanism).

timothyb89 · 2022-05-18T16:43:42Z

Yes, I think the changes will be needed, I just figured the wrapper looks similar to tsh daemon functionality, so perhaps worth sharing the code paths. Also, it may be that Teleport Connect will need use the same functionality either directly (with user-provided identities) or behind the scenes (as some sort of background mechanism).

I've given the daemon functionality a quick glance and my takeaway so far is: I definitely like the gRPC interface and it could be a big help for Machine ID's needs in the future, but doesn't really provide any of the functionality we want at the moment (mainly tsh proxy, tsh db, tsh ssh). The latter already works and that proxy and db are broken is considered a bug (#11770).

That said, since our main supported interface to tsh will be through CLI wrappers in tbot (tbot proxy, etc) I think it would be feasible to migrate to tsh daemon if/when it supports the functionality we need for Machine ID.

Timeline-wise we're in a bit of a awkward spot with access capabilities in Machine ID: we don't support Teleport's new out-of-the-box configuration since everything's now behind TLS routing by default, which requires a local proxy. While we can issue certs for databases they're unusable currently, and even SSH needs awkward workarounds. So, there's a desire to get some functionality in in the near term and I'm not opposed to replacing it with something better in the long/medium-term.

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

…nded-identityfiles

lib/client/api.go

zmb3 · 2022-05-24T07:50:00Z

lib/client/api.go

+type VirtualPathParams []string
+
+// VirtualPathCAParams returns parameters for selecting CA certificates.
+func VirtualPathCAParams(caType types.CertAuthType) VirtualPathParams {


If this needs to be kept in sync with the CA types I would put a comment in both locations mentioning that any updates need to be reflected on the other side.

Oddly tsh only ever uses the HostCA. I do kind of wonder if this is a problem for new clusters since I think the database CA is meant to be used to access databases in 9.0+.

In any case the only thing that would need to be updated is on Machine ID's end if/when we ever needed to pass multiple CAs. Right now we're just using the fallback case (just TSH_VIRTUAL_PATH_CA, with no CA type) in #12687 since we don't care to disambiguate (at least for now).

zmb3 · 2022-05-24T07:52:47Z

lib/client/api.go

+// and returns a *ProfileStatus which can be used to print the status of the
+// profile.
+func ReadProfileStatus(profileDir string, profileName string) (*ProfileStatus, error) {
+	var err error


Can you remove this?

Assuming you mean the err, yup, will do.

zmb3 · 2022-05-24T07:53:45Z

lib/client/api.go

@@ -1192,7 +1408,36 @@ func NewClient(c *Config) (tc *TeleportClient, err error) {
 		// if the client was passed an agent in the configuration and skip local auth, use
 		// the passed in agent.
 		if c.Agent != nil {
-			tc.localAgent = &LocalKeyAgent{Agent: c.Agent, keyStore: noLocalKeyStore{}, siteName: tc.SiteName}
+			webProxyHost, _ := tc.WebProxyHostPort()


What's the _ that you're ignoring here?

That's the port (int). That said we also have a WebProxyHost() helper that does exactly this, so I'll switch to it.

Co-authored-by: Krzysztof Skrzętnicki <[email protected]>

…nded-identityfiles

* 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 * 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 <[email protected]> * Address review feedback Co-authored-by: Krzysztof Skrzętnicki <[email protected]>

* 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 * 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 <[email protected]> * Address review feedback Co-authored-by: Krzysztof Skrzętnicki <[email protected]> Co-authored-by: Krzysztof Skrzętnicki <[email protected]>

timothyb89 added the backport-required label May 17, 2022

github-actions bot added the tsh tsh - Teleport's command line tool for logging into nodes running Teleport. label May 17, 2022

github-actions bot requested review from Tener and zmb3 May 17, 2022 00:50

timothyb89 mentioned this pull request May 17, 2022

Add tbot proxy and tbot db wrapper commands #12687

Merged

Fix failing lint

8163f73

Tener reviewed May 17, 2022

View reviewed changes

timothyb89 added 3 commits May 23, 2022 15:21

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

Tener approved these changes May 24, 2022

View reviewed changes

lib/client/api.go Outdated Show resolved Hide resolved

zmb3 approved these changes May 24, 2022

View reviewed changes

timothyb89 and others added 3 commits May 24, 2022 14:33

Update lib/client/api.go

8080cd5

Co-authored-by: Krzysztof Skrzętnicki <[email protected]>

Address review feedback

c8e1108

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

c76b4a3

…nded-identityfiles

timothyb89 enabled auto-merge (squash) May 24, 2022 21:25

timothyb89 merged commit d873ea4 into master May 24, 2022

timothyb89 deleted the timothyb89/tsh-expanded-identityfiles branch May 24, 2022 21:58

timothyb89 mentioned this pull request May 25, 2022

[v9] Extend support for identity files in tsh (#12686) #12922

Merged

webvictim mentioned this pull request Jun 8, 2022

Opened in error #13285

Closed

Joerger mentioned this pull request Feb 23, 2024

Support non-ssh access through tsh with an identity file #38592

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend support for identity files in tsh #12686

Extend support for identity files in tsh #12686

timothyb89 commented May 17, 2022 •

edited

Loading

Tener commented May 17, 2022

Tener left a comment

Tener May 17, 2022

timothyb89 May 17, 2022 •

edited

Loading

Tener May 17, 2022

timothyb89 May 17, 2022

timothyb89 May 24, 2022

Tener May 17, 2022

timothyb89 May 17, 2022

timothyb89 May 24, 2022

Tener May 17, 2022

timothyb89 May 17, 2022

timothyb89 May 23, 2022

timothyb89 commented May 17, 2022

Tener commented May 17, 2022

timothyb89 commented May 18, 2022

zmb3 May 24, 2022

timothyb89 May 24, 2022 •

edited

Loading

zmb3 May 24, 2022

timothyb89 May 24, 2022

zmb3 May 24, 2022

timothyb89 May 24, 2022

Extend support for identity files in tsh #12686

Extend support for identity files in tsh #12686

Conversation

timothyb89 commented May 17, 2022 • edited Loading

Tener commented May 17, 2022

Tener left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothyb89 May 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothyb89 commented May 17, 2022

Tener commented May 17, 2022

timothyb89 commented May 18, 2022

Choose a reason for hiding this comment

timothyb89 May 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothyb89 commented May 17, 2022 •

edited

Loading

timothyb89 May 17, 2022 •

edited

Loading

timothyb89 May 24, 2022 •

edited

Loading