Add options to skip connecting to ssh-agent from tsh #3721

awly · 2020-05-15T01:07:53Z

In particular, gpg-agent doesn't support SSH certificates, so writing to
it is potentially disruptive for the user: https://dev.gnupg.org/T1756

User has two options:

pass --no-use-local-ssh-agent for individual calls to tsh
set TELEPORT_USE_LOCAL_SSH_AGENT=false to make it permanent

Fixes #3169

tool/tsh/tsh.go

webvictim · 2020-05-15T17:09:43Z

tool/tsh/tsh.go

@@ -215,6 +213,10 @@ func Run(args []string, underTest bool) {
 	app.Flag("gops-addr", "Specify gops addr to listen on").Hidden().StringVar(&cf.GopsAddr)
 	app.Flag("skip-version-check", "Skip version checking between server and client.").BoolVar(&cf.SkipVersionCheck)
 	app.Flag("debug", "Verbose logging to stdout").Short('d').BoolVar(&cf.Debug)
+	app.Flag("ignore-local-ssh-agent", "Do not load generated SSH certificates into the local ssh-agent (specified via $SSH_AUTH_SOCK). You can also set TELEPORT_NO_LOCAL_SSH_AGENT environment variable.").
+		Envar(noLocalSSHAgentEnvVar).
+		Default(fmt.Sprint(maybeUsingGPGAgent())). // Note: the default is dynamic, based on our guess of the user environment.


So, I really like that we're trying to be smart by guessing whether gpg-agent is running and using that to decide whether to add certificates to the agent or not. This is definitely what we should do to make the experience better for people.

However, I am not much of a fan of us dynamically changing the default for a tsh flag and its help text based on an external parameter. I can see a potential situation where someone looks at tsh help output on a machine where gpg-agent is running, says "ok, the default behaviour is to ignore the ssh agent", then runs tsh assuming this to be true on a machine where gpg-agent isn't running and gets different behaviour.

I'm not 100% sure about a good solution to this though. Maybe we can just be more explicit in tsh output about ignoring SSH_AUTH_SOCK when gpg-agent is running so it's always obvious what exactly is going on, then we have tsh flags to always ignore or never ignore accordingly and don't set a default?

One note: tsh --help doesn't actually show you the defaults.

I went with this approach because I couldn't find a way to check whether a flag is set with kingpin. If we could check whether this is set, output could be very clear about us guessing.
Let me know if there's a way!

An alternative is to not do any guessing at all and let the user always set env var or the flag. This may be a niche-enough case to not require smarts in tsh.
WDYT?

I didn't realise that! If tsh --help doesn't show the defaults then I think this way is fine.

We should still (IMO) be explicit when deliberately choosing not to add keys to a gpg-agent though. Ideally I think this would be achieved by displaying a message on screen when we choose to do this. If this isn't possible with kingpin, maybe we just remove the automatic behaviour and add a specific note about gpg-agent's lack of certificate support to our docs.

I believe that most of the people experiencing the gpg-agent problem are already following the linked Github issue so disseminating information about the new flag should be easy.

Do we want a short flag for this @benarent?

I think it's Ok as a long flag and keep short flags for common workloads. IF we get a lot of feedback of people recreating tsh environments, we can come back and create a short flag.

Removed guessing since it makes everything a bit awkward.
We'll just have to mention this in the docs. @benarent I probably shouldn't add it to docs/4.2/.... I can update the docs when we have a 4.3 directory.

For 4.3 docs just create a draft PR against docs/4.3-base see #3697 for reference.

tool/tsh/tsh.go

russjones · 2020-05-16T00:55:40Z

lib/client/keyagent.go

 		noHosts:   make(map[string]bool),
 		username:  username,
 		proxyHost: proxyHost,
 	}

+	if !noLocalSSHAgent {


if not no local agent?

I tried to keep the zero-value of client.Config usable so that not setting NoLocalSSHAgent would preserve existing behavior.
But you're right, this is clunky. Flipped the Config field and set it in MakeDefaultConfig to true.

russjones · 2020-05-16T00:56:19Z

tool/tsh/tsh.go

@@ -215,6 +213,10 @@ func Run(args []string, underTest bool) {
 	app.Flag("gops-addr", "Specify gops addr to listen on").Hidden().StringVar(&cf.GopsAddr)
 	app.Flag("skip-version-check", "Skip version checking between server and client.").BoolVar(&cf.SkipVersionCheck)
 	app.Flag("debug", "Verbose logging to stdout").Short('d').BoolVar(&cf.Debug)
+	app.Flag("ignore-local-ssh-agent", "Do not load generated SSH certificates into the local ssh-agent (specified via $SSH_AUTH_SOCK). You can also set TELEPORT_NO_LOCAL_SSH_AGENT environment variable.").
+		Envar(noLocalSSHAgentEnvVar).
+		Default(fmt.Sprint(maybeUsingGPGAgent())). // Note: the default is dynamic, based on our guess of the user environment.


Do we want a short flag for this @benarent?

webvictim

LGTM. I like this flow much better - we specify an explicit default and allow it to be overridden for use cases where we don't want the default behaviour.

benarent

+1, but please create a documentation ticket after so we make sure this goes into 4.3 docs.

The flag is used to bypass the local SSH agent even when it's running. Specifically, this helps with agents that don't support certs. The flag was added in #3721 Updates #3169

In particular, gpg-agent doesn't support SSH certificates, so writing to it is potentially disruptive for the user: https://dev.gnupg.org/T1756 User has two options: - pass `--no-use-local-ssh-agent` for individual calls to tsh - set `TELEPORT_USE_LOCAL_SSH_AGENT=false` to make it permanent Fixes #3169

awly · 2020-05-19T16:13:10Z

4.3 docs PR: #3738

The flag is used to bypass the local SSH agent even when it's running. Specifically, this helps with agents that don't support certs. The flag was added in #3721 Updates #3169

* Base fork for 4.3 docs * [docs] external email identities and Kube Users (#3628) * Base fork for 4.3 docs * [docs] external email identities and Kube Users (#3628) * Remove trailing whitespace from docs files Some editors will do this automatically on save. This causes a lot of diffs when editing the docs in such an editor. Clean them up once now and we'll try to keep it tidy going forward. * Add make rules for docs whitespace and milv docs-test-whitespace: checks for trailing whitespace in all .md files under docs/. docs-fix-whitespace: removes trailing whitespace in all .md files under docs/. docs-test-links: runs milv in all docs/ subdirectories that have milv.config.yaml. docs-test: runs whitespace and links tests, used during `make docs` * Document the new `--use-local-ssh-agent` flag for tsh The flag is used to bypass the local SSH agent even when it's running. Specifically, this helps with agents that don't support certs. The flag was added in #3721 * Remove pam_script.so docs from SSH PAM page With #3725 we now populate teleport-specific env vars in a way that's accessible to `pam_exec.so`. There's no longer any reason to install pam_script.so separately and duplicate our docs. Updates #3692 * Using the correct --insecure-no-tls flag * Run docs-fix-whitespace make rule in a busybox container * Fixes #3414 Co-authored-by: Andrew Lytvynov <andrew@gravitational.com> Co-authored-by: Gus Luxton <gus@gravitational.com> Co-authored-by: Steven Martin <steven@gravitational.com> Co-authored-by: Gus Luxton <webvictim@gmail.com>

awly requested review from fspmarshall, klizhentas, russjones and webvictim as code owners May 15, 2020 01:07

webvictim reviewed May 15, 2020

View reviewed changes

russjones reviewed May 16, 2020

View reviewed changes

awly force-pushed the andrew/no-local-ssh-agent branch 2 times, most recently from b89ac38 to 1e25b13 Compare May 18, 2020 20:29

webvictim approved these changes May 19, 2020

View reviewed changes

benarent approved these changes May 19, 2020

View reviewed changes

webvictim added tsh tsh - Teleport's command line tool for logging into nodes running Teleport. docs-required labels May 19, 2020

awly mentioned this pull request May 19, 2020

Document the new --use-local-ssh-agent flag for tsh #3738

Merged

awly force-pushed the andrew/no-local-ssh-agent branch from 1e25b13 to 5fbbb7e Compare May 19, 2020 16:13

fspmarshall approved these changes May 19, 2020

View reviewed changes

russjones approved these changes May 19, 2020

View reviewed changes

awly merged commit 133d3ea into master May 19, 2020

awly deleted the andrew/no-local-ssh-agent branch May 19, 2020 20:05

webvictim removed the docs-required label May 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add options to skip connecting to ssh-agent from tsh #3721

Add options to skip connecting to ssh-agent from tsh #3721

awly commented May 15, 2020 •

edited

Loading

webvictim May 15, 2020 •

edited

Loading

awly May 15, 2020

webvictim May 15, 2020 •

edited

Loading

russjones May 16, 2020

benarent May 18, 2020

awly May 18, 2020

benarent May 18, 2020

russjones May 16, 2020

awly May 18, 2020

russjones May 16, 2020

webvictim left a comment

benarent left a comment

awly commented May 19, 2020

Add options to skip connecting to ssh-agent from tsh #3721

Add options to skip connecting to ssh-agent from tsh #3721

Conversation

awly commented May 15, 2020 • edited Loading

webvictim May 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webvictim May 15, 2020 • 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

webvictim left a comment

Choose a reason for hiding this comment

benarent left a comment

Choose a reason for hiding this comment

awly commented May 19, 2020

awly commented May 15, 2020 •

edited

Loading

webvictim May 15, 2020 •

edited

Loading

webvictim May 15, 2020 •

edited

Loading