Implements RFD-0022 - OpenSSH-compatible Agent Forwarding #6525

tcsc · 2021-04-21T02:42:24Z

Prior to this change, tsh will only ever forward the internal key
agent managed by tsh to a remote machine.

This change allows a user to specify that tsh should forward either
the tsh-internal keystore, or the system key agent at $SSH_AUTH_SOCK.

This change also brings the -A command-line option into line with
OpenSSH.

For more info refer to RFD-0022.

See-Also: #1571

russjones · 2021-04-27T03:24:24Z

@nklaassen @fspmarshall Can you review?

nklaassen · 2021-04-27T22:37:53Z

Does the command line option need to be updated, or is this coming in another PR? Seems like it is still a bool, and probably needs an update to the help string as well https://github.com/gravitational/teleport/blob/trent/key-agent/tool/tsh/tsh.go#L327

I'm not sure how difficult this could be, but would it be feasible to test that the correct key agent is actually forwarded when this option is set?

I missed the RFD review, but to me the options "yes" and "local" seem a bit unnecessarily confusing. Maybe something like "system" and "tsh", with "yes" as an alias to "system" could be more clear? Just spitballing here, not sure if this is reasonable

tcsc · 2021-04-30T04:51:40Z

@nklaassen - the command line semantics are expected to change. From the RFD:

The effect of this change is that the tsh ssh option -A will automatically acquire
semantics in line with the OpenSSH client, while the backwards compatible behaviour
can be activated with something like:
$ tsh ssh -o "ForwardAgent local" [email protected]

We could potentially add something like -A means ForwardAgent yes and -A=local means ForwardAgent local, but given there is already a way to activate legacy behaviour I didn't think it worthwhile. Will look at the help string, though.

Re: the option value names, we're kinds stuck with "yes" and "no" as they are compatible with the OpenSSH ssh client options. Aliasing "yes" to "system" might be a useful affordance, but I'd be tempted to get feedback from the wild. I can take or leave the name "local", so if we want to change that I have no objections.

nklaassen · 2021-04-30T05:24:53Z

@tcsc ahh I was just confused on that point, I thought that tsh ssh -A local was meant to work, realizing now that wouldn't line up with OpenSSH

As for the option value names, I'm just finding "yes" vs "local" unintuitive - both options could map to either one in my mind.

nklaassen

Code changes look good to me

Prior to this change, `tsh` will only ever forward the internal key agent managed by `tsh` to a remote machine. This change allows a user to specify that `tsh` should forward either the `tsh`-internal keystore, or the system key agent at `$SSH_AUTH_SOCK`. This change also brings the `-A` command-line option into line with OpenSSH. For more info refer to RFD-0022. See-Also: #1571

awly

Just to confirm: you tested this by logging into a remote server and running ssh-add -L?

lib/client/api.go

tool/tsh/options.go

tcsc · 2021-05-05T03:05:35Z

Just to confirm: you tested this by logging into a remote server and running ssh-add -L?

$ ./build/tsh login --proxy 3.25.51.119 --user beaker --insecure
Enter password for Teleport user beaker:
WARNING: You are using insecure connection to SSH proxy https://3.25.51.119:3080
> Profile URL:        https://3.25.51.119:3080
  Logged in as:       beaker
  Cluster:            aws-standalone
  Roles:              admin
  Logins:             rowlf
  Kubernetes:         disabled
  Valid until:        2021-05-06 00:56:48 +1000 AEST [valid for 12h0m0s]
  Extensions:         permit-agent-forwarding, permit-port-forwarding, permit-pty

$ ./build/tsh ssh rowlf@auth-node
rowlf@ip-172-31-5-54:~$ ssh-add -l
Could not open a connection to your authentication agent.
rowlf@ip-172-31-5-54:~$ exit

$ ./build/tsh ssh -A rowlf@auth-node
rowlf@ip-172-31-5-54:~$ ssh-add -l
256 SHA256:dR1daFKmKLEDK055QNGbFi6zur2SSl+NOMnxzP4uBwQ [email protected] (ED25519)
2048 SHA256:5FPfhJMZH7x1yfzK9G3IZFqGkopy600C1AOR9sDH0A8 teleport:kermit (RSA-CERT)
2048 SHA256:5FPfhJMZH7x1yfzK9G3IZFqGkopy600C1AOR9sDH0A8 teleport:kermit (RSA)
2048 SHA256:AEML8fwmFdUazCzj+o1wEKBGIEaR33ei1zG4h5eZFU4 teleport:beaker (RSA-CERT)
2048 SHA256:AEML8fwmFdUazCzj+o1wEKBGIEaR33ei1zG4h5eZFU4 teleport:beaker (RSA)
rowlf@ip-172-31-5-54:~$ exit

$ ./build/tsh ssh -o "ForwardAgent local" rowlf@auth-node
rowlf@ip-172-31-5-54:~$ ssh-add -l
2048 SHA256:AEML8fwmFdUazCzj+o1wEKBGIEaR33ei1zG4h5eZFU4 teleport:beaker (RSA-CERT)
2048 SHA256:AEML8fwmFdUazCzj+o1wEKBGIEaR33ei1zG4h5eZFU4 teleport:beaker (RSA)
rowlf@ip-172-31-5-54:~$ exit

russjones

Bot.

klizhentas

Couple of nitpicks, but otherwise LGTM. You created a beautifully written RFD btw, especially the diagram, I enjoyed reading it.

klizhentas · 2021-05-06T23:46:26Z

lib/client/session.go

+func selectKeyAgent(tc *TeleportClient) agent.Agent {
+	switch tc.ForwardAgent {
+	case ForwardAgentYes:
+		log.Debugf("Selecting System Key Agent")


Nit: (derived from https://github.com/golang/go/wiki/CodeReviewComments#error-strings). Let's make our log lines proper english sentences, so:

Selecting system key agent.

Saw this after hitting Merge, will fix in new PR

klizhentas · 2021-05-06T23:46:47Z

tool/tsh/options.go

+
+	default:
+		log.Errorf(
+			"Invalid agent forwarding mode %q. Defaulting to %q",


Saw this after hitting Merge, will fix in new PR

Prior to this change, `tsh` will only ever forward the internal key agent managed by `tsh` to a remote machine. This change allows a user to specify that `tsh` should forward either the `tsh`-internal keystore, or the system key agent at `$SSH_AUTH_SOCK`. This change also brings the `-A` command-line option into line with OpenSSH. For more info refer to RFD-0022. See-Also: #1571

) Prior to this change, `tsh` will only ever forward the internal key agent managed by `tsh` to a remote machine. This change allows a user to specify that `tsh` should forward either the `tsh`-internal keystore, or the system key agent at `$SSH_AUTH_SOCK`. This change also brings the `-A` command-line option into line with OpenSSH. For more info refer to RFD-0022. See-Also: #1571

tcsc force-pushed the trent/key-agent branch 2 times, most recently from 5c6105b to e9797fe Compare April 27, 2021 02:27

tcsc marked this pull request as ready for review April 27, 2021 03:04

tcsc requested review from alex-kovoy, klizhentas, r0mant and russjones as code owners April 27, 2021 03:04

nklaassen self-requested a review April 27, 2021 21:58

russjones assigned awly and nklaassen Apr 28, 2021

tcsc force-pushed the trent/key-agent branch from e9797fe to d513ec4 Compare April 29, 2021 08:28

nklaassen approved these changes Apr 30, 2021

View reviewed changes

tcsc added 3 commits May 4, 2021 20:53

Post rebase fixup

0662b88

Test fix & extra logging

7888485

tcsc force-pushed the trent/key-agent branch from d513ec4 to 7888485 Compare May 4, 2021 10:54

awly approved these changes May 4, 2021

View reviewed changes

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

tool/tsh/options.go Outdated Show resolved Hide resolved

Applying suggestions

3de2e50

russjones approved these changes May 5, 2021

View reviewed changes

russjones added the ux label May 5, 2021

klizhentas approved these changes May 6, 2021

View reviewed changes

Merge branch 'master' into trent/key-agent

cc71ae3

tcsc enabled auto-merge (squash) May 7, 2021 00:04

tcsc merged commit 769b4b5 into master May 7, 2021

tcsc deleted the trent/key-agent branch May 7, 2021 00:17

tcsc mentioned this pull request May 7, 2021

Making log lines proper sentences #6772

Merged

russjones mentioned this pull request May 7, 2021

Agent forwarding handling by tsh #1571

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements RFD-0022 - OpenSSH-compatible Agent Forwarding #6525

Implements RFD-0022 - OpenSSH-compatible Agent Forwarding #6525

tcsc commented Apr 21, 2021

russjones commented Apr 27, 2021

nklaassen commented Apr 27, 2021

tcsc commented Apr 30, 2021

nklaassen commented Apr 30, 2021

nklaassen left a comment

awly left a comment

tcsc commented May 5, 2021 •

edited

Loading

russjones left a comment

klizhentas left a comment

klizhentas May 6, 2021

tcsc May 7, 2021

klizhentas May 6, 2021

tcsc May 7, 2021

Implements RFD-0022 - OpenSSH-compatible Agent Forwarding #6525

Implements RFD-0022 - OpenSSH-compatible Agent Forwarding #6525

Conversation

tcsc commented Apr 21, 2021

russjones commented Apr 27, 2021

nklaassen commented Apr 27, 2021

tcsc commented Apr 30, 2021

nklaassen commented Apr 30, 2021

nklaassen left a comment

Choose a reason for hiding this comment

awly left a comment

Choose a reason for hiding this comment

tcsc commented May 5, 2021 • edited Loading

russjones left a comment

Choose a reason for hiding this comment

klizhentas left a comment

Choose a reason for hiding this comment

klizhentas May 6, 2021

Choose a reason for hiding this comment

tcsc May 7, 2021

Choose a reason for hiding this comment

klizhentas May 6, 2021

Choose a reason for hiding this comment

tcsc May 7, 2021

Choose a reason for hiding this comment

tcsc commented May 5, 2021 •

edited

Loading