(3.2) Teach tsh login to configure docker/helm clients. #3045

r0mant · 2019-10-01T21:25:40Z

The Why

This PR extends tsh login to support login to docker registry and helm chart repository that may be optionally provided by the proxy server (in Gravity Hub use-case, see gravitational/gravity#580).

With this change, when a user performs tsh login into Gravity Hub, the local docker/helm clients on the user's machine will be configured to use the Hub (== web proxy in Teleport's terminology) as their respective registry/repository using the same Teleport-issued user certificates for authentication.

For regular Teleport clusters, literally nothing will change as tsh login will only attempt to configure docker/helm if it detects it's talking to a Hub (more on how that works below).

The How

The server's PingResponse has been extended with additional "features" field. This is how tsh knows whether it needs to configure docker/helm as Gravity Hub will populate it with features it supports.
To configure docker client user certificates are symlinked to /etc/docker/certs.d/<proxy> directory. The tricky part here is that this directory is not configurable and requires root permissions, so tsh will check if it's running as root and if it's not, it will relaunch a subcommand as root (which will prompt user for password in case they don't have passwordless sudo setup) to create these symlinks. This should only be done once because symlinks are not removed on logout but will point to non-existent certificates after tsh logout.
To configure helm client tsh executes helm repo add and helm repo update commands, again configuring the same certificates for client authentication. Gravity vendors helm as a library so it could do this on its own but tsh has to call the commands.

fspmarshall

LGTM. Might be nice to add some package-level docs to lib/client/configurator since the package name is a bit ambiguous.

r0mant · 2019-10-08T23:15:49Z

@klizhentas @russjones Ping :)

russjones · 2019-10-11T21:27:49Z

lib/client/configurator/helm.go

+	if err != nil {
+		return trace.Wrap(err)
+	}
+	err = runCommand("helm", "repo", "add", chartRepository,


I quickly looked at the helm docs and this looks safe. However, since you know helm better, any concern with attacker controlling config.ProxyAddress and possibly adding additional arguments here?

Interesting question. I don't see how any extra arguments could be used in a harmful way here, but one interesting implication of the attacker controlling proxy address is that they could point it to another server which could potentially contain malicious helm charts. But I think in this case tsh will also be affected, and also the attacker would need to have access to the real CA to have mTLS auth pass.

Also, the "proxy address" value is parsed as an address just a couple of lines above which I think would fail to parse if it contained something else.

russjones · 2019-10-11T21:39:02Z

lib/client/configurator/docker_linux.go

+// getSymlinks returns a map of symlinks that need to be configured in order
+// to let local Docker access registry provided by the proxy.
+func (c *dockerConfigurator) getSymlinks(config Config) map[string]string {
+	certsDir := filepath.Join(DockerCerts, config.ProxyAddress)


Since config can be attacker controlled, config.ProxyAddress can be something like ../../../some/other/dir, I would check certsDir after the join that the base directory is what you expect.

russjones · 2019-10-11T21:45:18Z

lib/client/configurator/docker_linux.go

+		return c.runAsRoot(config)
+	}
+	// Ensure /etc/docker/certs.d/<proxy> directory exists.
+	certsDir := filepath.Join(DockerCerts, config.ProxyAddress)


Check base directory similar to other issue.

klizhentas · 2019-10-15T16:38:19Z

lib/utils/utils.go

+func SafeFilepathJoin(base string, rest ...string) (string, error) {
+	path := filepath.Join(append([]string{base}, rest...)...)
+	if !strings.HasPrefix(path, base) {
+		return "", trace.BadParameter("cannot join %v and %v", base, rest)


this error is not helpful. Why it can't join and what users should do about it?

klizhentas · 2019-10-15T16:39:38Z

lib/client/configurator/docker_linux.go

+	// order to be able to find proper certificates.
+	args := []string{"configure-docker", "--profile-dir", config.ProfileDir}
+	if c.debug {
+		args = append(args, "--debug")


no need to passthough this flag everywhere, trace.IsDebug will tell you if the system is in debug mode (I think but you should check)

klizhentas · 2019-10-15T16:39:49Z

lib/client/configurator/docker_linux.go

+	if c.debug {
+		args = append(args, "--debug")
+	}
+	fmt.Printf("Will configure access to Docker registry provided by %v, "+


Who will configure it?

klizhentas · 2019-10-15T16:40:22Z

lib/client/configurator/docker_linux.go

+		args = append(args, "--debug")
+	}
+	fmt.Printf("Will configure access to Docker registry provided by %v, "+
+		"you may be prompted for password.\n", config.ProxyAddress)


You can check if the symlink exists, and then print the warning on the first setup instead of printing the noisy message all the time, that folks will learn to ignore anyways.

klizhentas · 2019-10-15T16:40:57Z

lib/client/api.go

@@ -163,6 +164,9 @@ type Config struct {
 	// InsecureSkipVerify is an option to skip HTTPS cert check
 	InsecureSkipVerify bool

+	// Debug allows to enable client debug logging.


I would see if you can use trace.IsDebug instead of passthrough of this flag.

klizhentas · 2019-10-15T16:41:40Z

lib/client/api.go

+
+	// Docker is responsible for configuring access to Docker registy that
+	// may be optionally provided by the server.
+	Docker configurator.Configurator


If you don't really care about configurators types (as they are the same interface) why not have a generic list of them

klizhentas · 2019-10-15T16:45:11Z

lib/client/configurator/configurator.go

@@ -0,0 +1,64 @@
+/*


configurator is a bit too generic, perhaps plugins is a better name? or extensions?

klizhentas · 2019-10-15T16:47:49Z

lib/client/configurator/configurator_test.go

@@ -0,0 +1,85 @@
+/*


@russjones can you take a look at this alternative design:

https://github.com/cyphar/filepath-securejoin/blob/master/join.go

I've noticed docker is using it. I wonder if we should use it instead of the home-grown one.

Looks good to me.

klizhentas · 2019-10-15T16:52:56Z

lib/client/configurator/util.go

+	return true, nil
+}
+
+// runCommandSudo executes the specified command as sudo.


I don't think we should ever hardcode as sudo, instead ask user to run as sudo or any other privileges.

@klizhentas So this runs as a part of tsh login process and thus if we don't want to invoke sudo here, I see 2 options:

Ask the user to run the entire tsh login as root which I'm pretty sure we don't wanna do.

Ask the user to run tsh docker configure themselves as a root afterwards which will sort of interrupt the login flow.

With invoking the command as a sudo here, I tried to limit the "scope" that runs as a root while still preserving decent UX because they will be auto-prompted for password if needed.

We could make this more explicit by sacrificing a bit on the UX side, for example:

User runs tsh login which fetches certs and configures helm (which doesn't need root).

Next we see if tsh has permissions to symlink to /etc/docker/certs.d.

If it does (maybe it's already running as root) - everything is cool, we do the symlinking.

If it does not (which likely would be most of the time) - we just output the message at the end asking the user to run tsh configure docker as a user that has permissions to /etc/docker/certs.d if they wish to configure registry access. This should only be needed the first time b/c for subsequent logins symlinks will already be there (unless removed).

Would that be better?

I'm concerned about tsh attempting to elevate it's own privileges without user's approval as a pattern. @knisbet @russjones what do you folks think about it?

I like @r0mant's suggestion, tsh can print a message explaining what it needs to do (create symlinks as root), why, and then ask the user to run a command as sudo to do it.

Leaves no ambiguity and the user can drop out and not perform the action if they feel it would be harmful to their system.

My 2 cents, is I think docker configure should be a separate tsh command, and if run as non-root it can then prompt indicating it should be run as root and why (or link to a KB article or something on what happens.

The reason I think this:

I suspect it may be unsafe to expect the user to have root permissions on their host on login. Some orgs may have laptops or machines where TSH is used hardened in a way that prevents sudo. I'm not sure we can predict what sort of policies may exist or that calling sudo will be available.

I'm not sure the docker login will be a commonly used feature. I suspect the average user of a gravity cluster does not need access to the images. So I don't have an issue with having a separate configuration command that is run once with matching docs.

I'm of the opinion it's a cleaner UX pattern, I think some of the miss-understandings of our product come from trying to cleverly do extra things that are non-intuitive. I didn't indicate I want to log into the docker registry, but on tsh login this other login happens automatically / behind the scenes. It would be different if this was a common pattern or a commonly understood part of the intentions.

I think this is a normal approach in other tools, gcloud compute config-ssh comes to mind as something that after login I knowingly execute to update my ssh configuration.

It only needs to be done once, so I don't see it as really interrupting the flow.

Downsides of this approach are I think the standard things the team tries to avoid, which is having additional commands in the cli, how those commands are exposed / seen on non-gravity clusters, that this is something a potential user must discover instead of being "magic", how the configuration gets removed, that a user remembers to run it on a new machine, etc.

Possible alternatives:

The docker setup could be a flag to tsh login, so it's intention based. I'm not personally a big fan of this, but it would still create a clearer intention from the user that they intend for the login to setup docker and associated requirements.

I'm not opposed to the proposal of printing in the tsh login how to configure docker at the end of the login if not already configured. My opinion though is that it probably adds noise to the login process for the average user.

@benarent I think this has to be reviewed by your team as well. My main concern is as is this PR asks tsh login to elevate it's privileges and re-run itself as root. However, alternatives outlined here should be visible to customer, so you should take a look into this. In addition to that, it would be helfpul to support a case when users don't actually want their docker / helm credentials updated. Can you please work with @r0mant to figure out the UX of this part and update this PR with your thoughts on the matter.

@klizhentas @russjones @knisbet We have spent some time today brainstorming with @benarent on this and updated the design doc with what we came up with: https://docs.google.com/document/d/1V2bI2oZQC3Y3l42_eOvwJZSrPykN6Oxm0vO4oWDiv78/edit#heading=h.ykw97ikonjby (see tsh UX part at the bottom).

The basic flow is like I described above i.e. to ask user to run the "configure" command explicitly in case they don't have permissions.

To support the use-case where they may not want to have their docker/helm configured, instead of going with the "add more flags" approach, we opted for placing these options into the role, similar to "agent/port forwarding" options it has now. This way they would be able to create a role that does not configure docker/helm by default, but still have an option to configure them manually via specific "configure" commands if they want. These options will be encoded into certs, again similar to agent/port forwarding and other stuff.

@r0mant @benarent I think role options is not a great place for what is basically user preferences. The option of logging into helm or docker registry is a client-level preference, for example same user may or may not want to login into docker on different machines.

I think a good place for user preferences is default global login preferences section in our configuration file duplicated in .tsh/profile

klizhentas · 2019-10-15T16:54:17Z

tool/tsh/tsh.go

+	// The configure-docker command sets up the local Docker daemon to use the
+	// key and certificate obtained as a result of tsh login for a registry
+	// access by symlinking them to /etc/docker/certs.d/<proxy> directory.
+	configureDocker := app.Command("configure-docker", "Configure Docker to use user certificates for registry access").Hidden()


tsh does not use dashes for subcommands so far, I would use some other notation.

Elsewhere we tend to follow a pattern of creating intermediate commands, such as tctl nodes. The tctl nodes command doesn't do anything by itself, but it it has the nice side-effect of allowing the user to run tctl nodes --help and see all the node related subcommands (e.g. tctl nodes ls) in one place. Maybe the same pattern could work here?

Yeah, sub-command was gonna be my other approach, could do that instead.

klizhentas · 2019-10-15T16:54:36Z

tool/tsh/tsh.go

+	// key and certificate obtained as a result of tsh login for a registry
+	// access by symlinking them to /etc/docker/certs.d/<proxy> directory.
+	configureDocker := app.Command("configure-docker", "Configure Docker to use user certificates for registry access").Hidden()
+	configureDocker.Flag("profile-dir", "Client profile directory, defaults to ~/.tsh.").StringVar(&cf.ProfileDir)


defaults to should use real deafault value

r0mant · 2019-10-25T18:52:18Z

@klizhentas I have addressed your comments. The behavior now is like described above (tsh will tell the user to run the command themselves if it can't create symlinks). Automatic login to docker/helm can also be disabled via a config file - it can either be provided on CLI, otherwise tsh will look for it first in ~/.tsh/tsh.yaml and then in /etc/tsh.yaml. The config may look like this:

teleport:
  docker_registry: false
  helm_repository: false

Should be ready for re-review. cc @russjones @fspmarshall @benarent

r0mant · 2019-10-30T23:58:47Z

@klizhentas @kontsevoy @benarent Ok, so I've updated according to what we discussed on the product meeting, in particular:

Rolled back tsh.yaml configuration. For now tsh always attemps to log into docker/helm when talking to a Gravity Hub that supports them.
Removed all extra "configure" commands and instead in the message ask the user to create /etc/docker/certs.d/<proxy> directory.

benarent

Looks good, I've created another note so we can track the Profile options for tsh.

benarent · 2019-10-31T00:13:35Z

lib/client/extensions/docker_linux.go

+
+If you'd like to configure your local Docker client, please create the following directory and make sure tsh has write permissions for it, for example:
+
+  mkdir -p /etc/docker/certs.d/%[1]v && chown -R %[2]v /etc/docker/certs.d/%[1]v


👍 Looks good.

benarent · 2019-10-31T00:29:22Z

Other Q: Since Gravity, has an older version of Teleport, will we include this into the latest tsh binary. If so which versions will support this feature?

Scenario is: New Ops person joins the team, and goes directly to our downloads page. Picked Teleport 4.1 binary vs having to know to use 3.2?

Add support for Docker registry and Helm repository.

217034a

r0mant requested review from klizhentas, russjones and fspmarshall October 1, 2019 21:25

fspmarshall approved these changes Oct 2, 2019

View reviewed changes

russjones approved these changes Oct 11, 2019

View reviewed changes

Address review comments.

a0a19b1

klizhentas reviewed Oct 15, 2019

View reviewed changes

klizhentas requested changes Oct 15, 2019

View reviewed changes

r0mant added 4 commits October 24, 2019 15:09

Addressing code review comments.

f97c1a4

Vendor new deps.

29a7851

Add support for tsh.yaml config.

667f6eb

Some final polish.

7521285

r0mant added 2 commits October 30, 2019 15:38

Rollback tsh config.

6ff714c

Remove extra commands.

95c26ba

benarent mentioned this pull request Oct 31, 2019

Add option to configure Gravity Docker & Helm setup via tsh profile. #3119

Closed

benarent approved these changes Oct 31, 2019

View reviewed changes

klizhentas approved these changes Nov 19, 2019

View reviewed changes

Update error message.

a572144

r0mant merged commit dfa159b into branch/3.2 Nov 20, 2019

r0mant deleted the roman/3.2/login branch November 20, 2019 02:53

r0mant mentioned this pull request Nov 20, 2019

Docker registry mTLS. gravitational/gravity#898

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(3.2) Teach tsh login to configure docker/helm clients. #3045

(3.2) Teach tsh login to configure docker/helm clients. #3045

r0mant commented Oct 1, 2019

fspmarshall left a comment

r0mant commented Oct 8, 2019

russjones Oct 11, 2019

r0mant Oct 11, 2019

russjones Oct 11, 2019

russjones Oct 11, 2019

klizhentas Oct 15, 2019

klizhentas Oct 15, 2019

klizhentas Oct 15, 2019

klizhentas Oct 15, 2019

klizhentas Oct 15, 2019

klizhentas Oct 15, 2019

klizhentas Oct 15, 2019

klizhentas Oct 15, 2019

russjones Oct 16, 2019

klizhentas Oct 15, 2019

r0mant Oct 15, 2019

klizhentas Oct 15, 2019

russjones Oct 16, 2019 •

edited

Loading

knisbet Oct 16, 2019 •

edited

Loading

klizhentas Oct 16, 2019

r0mant Oct 23, 2019

klizhentas Oct 24, 2019

klizhentas Oct 15, 2019

fspmarshall Oct 15, 2019

r0mant Oct 15, 2019

klizhentas Oct 15, 2019

r0mant commented Oct 25, 2019

r0mant commented Oct 30, 2019

benarent left a comment

benarent Oct 31, 2019

benarent commented Oct 31, 2019


		If you'd like to configure your local Docker client, please create the following directory and make sure tsh has write permissions for it, for example:

		mkdir -p /etc/docker/certs.d/%[1]v && chown -R %[2]v /etc/docker/certs.d/%[1]v

(3.2) Teach tsh login to configure docker/helm clients. #3045

(3.2) Teach tsh login to configure docker/helm clients. #3045

Conversation

r0mant commented Oct 1, 2019

The Why

The How

fspmarshall left a comment

Choose a reason for hiding this comment

r0mant commented Oct 8, 2019

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

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

russjones Oct 16, 2019 • edited Loading

Choose a reason for hiding this comment

knisbet Oct 16, 2019 • 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

r0mant commented Oct 25, 2019

r0mant commented Oct 30, 2019

benarent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benarent commented Oct 31, 2019

russjones Oct 16, 2019 •

edited

Loading

knisbet Oct 16, 2019 •

edited

Loading