Handle duplicate node names & support UUID based proxying. #3340

fspmarshall · 2020-02-11T18:35:05Z

This PR consists of two parts. First, attempting to ssh into an ambiguous target results in an error. Second, nodes can now be unambiguously connected to via UUID. Ex:

$ tsh ssh alice@example.com
error: ambiguous host could match multiple nodes
  
Node  Name  Node ID                              Address        Labels
----------- ------------------------------------ -------------- ---------
example.com ae79f375-633e-4a81-941e-83faf4a3c966 127.0.0.1:3022 foo=bar
example.com 17a980a9-ce32-4c95-a5c4-c4b4717851e9 127.0.0.1:4022 spam=eggs            
  
Hint: try addressing the node by unique id (ex: tctl ssh -U user@node-id)

$ tsh ssh -U alice@17a980a9-ce32-4c95-a5c4-c4b4717851e9
...

In order to implement the above, a new format for the proxy subsystem was added:

ssh -o "ForwardAgent yes" \
    -o "ProxyCommand ssh -o 'ForwardAgent yes' -p 3023 %r@example.com -s proxy:conn\(uuid=%h\)" \
    alice@17a980a9-ce32-4c95-a5c4-c4b4717851e9

Fixes #2396

tool/tsh/tsh.go

lib/client/api.go

lib/web/terminal.go

lib/client/client.go

russjones · 2020-02-11T21:38:36Z

lib/srv/regular/proxy.go

 	for i := range servers {
+		// If nodeId is supplied, we must unambiguously match


I'm thinking this logic where we loop over servers with all the breaks and continues is getting too unwieldy due to all the special cases we now have. What do you think about breaking it up? So something like:

var servers services.Server if t.nodeID != "" { servers = getServersByUUID(...) } else { servers = getServers(...) } // If multiple servers matched, return an error that the client can use to present a "server-is-ambiguous" error. if len(servers) > 1 { return trace.NotFound("err-server-is-ambiguous") }

lib/utils/utils.go

russjones · 2020-02-11T22:23:35Z

lib/utils/utils.go

+	return fmt.Sprintf("%s:%s", p.Host, port)
+}
+
+func (p *ProxyParams) Encode() string {


The more I think about it, I don't think we should try to support connecting via UUID and host:port to our current proxy subsystem.

The current system is designed around connecting via host:port which is 99% of the use-cases and is going to really be the only one that works well with OpenSSH (where URL encoding probably won't be supported) and not something we can drop support for.

All the special casing to support UUID seems to indicate we're going down the wrong path.

What do you think about this: we leave the current proxy subsystem as-is and we add another subsystem proxy-uuid. When tsh connects via UUID it just requests the proxy-uuid subsystem. Parsing the old subsystem won't change at all and parsing the UUID subsystem should be much simpler.

What do you think? Would that help simplify the codebase?

I'm honestly not sure... there are pros and cons to both.

In the short term, keeping the old subsystem separate is probably the easier choice. Definitely reduces the chances of accidentally introducing a regression.

Long-term, separate subsystems feels like more maintenance and more places for tests to accidentally miss an edge case. In terms of openssh, this new format seems just as compatible. Either way, the user is manually encoding the subsystem call (see original comment).

@klizhentas any thoughts on this?

russjones

Left some largely minor comments, overall looks good, let's discuss tomorrow.

russjones · 2020-02-19T00:23:23Z

lib/client/api.go

+	uuid string
+}
+
+func (t targetNode) String() string {


// String returns either the UUID or host:port for the target host. UUID // is given priority if provided.

russjones · 2020-02-19T00:25:09Z

lib/client/api.go

+			if err != nil {
+				return nil, trace.Wrap(err)
+			}
+			// TODO(fspmarshall): Transition to using UUID here once


Can you add a similar comment to what we have for the web proxy.

// DELETE IN: 5.0 // // This fallback will be unnecessary once all proxies support lookup // by UUID.

russjones · 2020-02-19T00:28:18Z

lib/client/api.go

 		}
 	}
 	if len(nodes) == 0 {
+		if tc.HostUUID != "" {


// Client is attempting to connect via UUID.

russjones · 2020-02-19T00:29:10Z

lib/client/api.go

+	if err != nil {
+		return trace.Wrap(err)
+	}
+	// TODO(fspmarshall): Upgrade this to use node uuid instead once


Can you add a similar comment to what we have for the web proxy.

// DELETE IN: 5.0 // // This fallback will be unnecessary once all proxies support lookup // by UUID.

russjones · 2020-02-19T00:32:40Z

lib/client/api.go

@@ -1411,13 +1457,24 @@ func (tc *TeleportClient) ListNodes(ctx context.Context) ([]services.Server, err
 	return proxyClient.FindServersByLabels(ctx, tc.Namespace, tc.Labels)
 }

+// ListAllNodes is the same as ListNodes except that it ignores labels.
+func (tc *TeleportClient) ListAllNodes(ctx context.Context) ([]services.Server, error) {


Is this being used? Can't see it being called by anyone?

russjones · 2020-02-19T00:48:49Z

lib/utils/utils.go

@@ -250,6 +250,197 @@ func SplitHostPort(hostname string) (string, string, error) {
 	return host, port, nil
 }

+type ProxyParams struct {


Struct and all members need comments.

russjones · 2020-02-19T00:55:58Z

lib/utils/utils.go

+	return net.JoinHostPort(host, port)
+}
+
+func (p *ProxyParams) Encode() string {


// Encode takes the parameters given and creates the proxy subsystem string. // For connections using host:port that means it returns something like // "server:3022@default@example.com". For connections using UUID, it creates // something like "conn(uuid=00000000-0000-0000-0000-000000000000&...)".

russjones · 2020-02-19T00:57:03Z

lib/utils/utils.go

+	return EncodeConnParams(cp)
+}
+
+func (p *ProxyParams) legacyEncode() string {


// legacyEncode transforms the given parameters into the proxy format of // "host:port@namespace@clusterName".

russjones · 2020-02-19T00:59:09Z

lib/utils/utils.go

+	paramNodeID    = "uuid"
+)
+
+func ParseProxyParams(request string) (ProxyParams, error) {


// ParseProxyParams parses the proxy subsystem request string. Supports // standard as well as URL encoded proxy subsystem request schemes.

russjones · 2020-02-19T01:00:09Z

lib/utils/utils.go

+	return params, nil
+}
+
+func parseLegacyProxyParams(request string) (ProxyParams, error) {


// parseLegacyProxyParams will parse the classic proxy subsystem request.

benarent

Just a comment about tctl vs tsh, and comment about how much work to expand this to SCP

benarent · 2020-02-19T17:08:11Z

tool/tsh/tsh.go

+			}
+			fmt.Fprintf(os.Stderr, "error: ambiguous host could match multiple nodes\n\n")
+			showNodes(nodes, true)
+			fmt.Fprintf(os.Stderr, "Hint: try addressing the node by unique id (ex: tctl ssh -U user@node-id)\n")


We need to change this from (ex: tctl ssh -U user@node-id)\n") to (ex: tsh ssh -U user@node-id)\n")

benarent · 2020-02-19T17:12:32Z

( also note to self to add this to CLI docs after )

fspmarshall · 2020-02-20T23:00:37Z

Feedback Discussion

I've gathered a few rounds of feedback via various channels. I think its time to summarize the various questions/concerns/comments I've come across, my current thinking on them, and a proposal for where to go from here.

In no particular order...

Why not just start rejecting duplicate hostnames?

Because users may (and do) have multiple nodes that share the same hostname, rejecting duplicate hostnames would break existing deployments. Furthermore, rejecting duplicate hostnames poses too much of an issue when dealing with things like replication, Blue/Green deploys, rotations, etc... There are perfectly legitimate reasons to have multiple nodes with the same hostname.

Does UUID based routing need to be explicit? Why can't we make <user>@<uuid> "just work"?

If we take the approach of simply checking if the hostname parameter matches a node's unique id, we don't actually succeed in eliminating ambiguity. Take this perfectly legal example:

forrest@fspm:~/teleport$ tctl nodes ls
Nodename UUID                                 Address        Labels    
-------- ------------------------------------ -------------- --------- 
fspm     ae79f375-633e-4a81-941e-83faf4a3c966 127.0.0.1:3022           
fspm     fspm                                 127.0.0.1:4022

The proposed subsystem syntax is kinda ugly and requires escaping when invoked in a shell.

Agreed. If we go this route, a syntax which doesn't require escapes (at least in the trivial case) would be preferable. One simple change is to move from conn(<params>) to conn[<params>], though this doesn't change the fact that multiple keys introduces an ampersand which still needs escaping (e.g. conn[uuid=hello\&port=123]). Another option is to support a "pretty" format that is distinguished by subsystem prefix instead of key-value pairs (I'll cover this later).

Subsystem based proxying should be deprecated now that we support -J/JumpHost.

I'm of the opinion that the -J/JumpHost support isn't sufficient to deprecate subsystem based proxying. While using -J is definitely cleaner in the simple single-cluster case, I think the workflow starts is too limited once trusted clusters are involved.

Because clusters need to be specified by certificate when using JumpHost, the user is forced into making a difficult choice. Either never operate against multiple clusters simultaneously (very limiting when using ssh as part of your automation), or manually manage multiple teleport identity files (cumbersome, and potentially risky).

Requiring certificate-level parameters to resolve ambiguities also increases the likelihood of human error. Say Alice is working on node example.com in cluster testing. Bob asks Alice to assist him with something on node example.com in cluster prod. Alice runs tsh login prod, gives Bob a hand, and goes to lunch. Alice comes back, forgets to run tsh login testing, and proceeds to have a very bad day. There is a real benefit in having cluster be an explicit parameter: it forces users to acknowledge ambiguities and makes it easy for users to permanently resolve ambiguities.

The root issue here stems from the fact that JumpHost doesn't have great support for side-band information.

Why not add a new subsystem for this instead of overhauling the existing proxy: subsystem?

Seems reasonable. There are pros and cons to both approaches, but adding a new subsystem definitely cuts down on churn.

This does open up the possibility of side-stepping the issue of encoding by simply giving up on the idea of supporting key-value pairs. A dedicated subsystem for uuid based routing could be as simple as uuid:<uuid>[@<cluster>]. Not sure I like this though; I'd prefer new subsystem formats to support key-value pairs (even if they also support a more concise version for use in ssh config files).

Why not pass parameters via ENV variable instead of in the subsystem request string?

This is technically possible, but cumbersome to use with openssh. Version 7.8 of openssh introduced the SetEnv directive which improves the experience, but this version still isn't in wide circulation.

Revised Proposal

Leave the existing proxy: subsystem unchanged, with no support for uuid based routing or key-value style parameters.

Create a new subsystem named uuid (or maybe proxy-uuid if that seems too ambiguous) which accepts two alternative encodings, A "pretty" uuid@cluster style encoding, and a "verbose" conn[<params>] style encoding:

# pretty
ProxyCommand ssh -p 3023 %r@some-proxy -s uuid:some-uuid@some-cluster

# verbose
ProxyCommand ssh -p 3023 %r@some-proxy -s uuid:conn[uuid=some-uuid\&cluster=some-cluster]

The verbose encoding will be used by tsh, while the pretty encoding will be the recommended encoding when invoking the uuid subsystem via openssh.

For all future subsystems moving forward, ensure that they accept a key-value pair encoding.

klizhentas · 2020-02-20T23:10:26Z

@benarent @fspmarshall @russjones still seems like a solution that works, but at a very high cost of adding extra subsystem, deprecating the old subsystem and not supporting -J. I would like us to keep looking for a better solution.

klizhentas · 2020-02-20T23:15:37Z

Requiring certificate-level parameters to resolve ambiguities also increases the likelihood of human error. Say Alice is working on node example.com in cluster testing. Bob asks Alice to assist him with something on node example.com in cluster prod. Alice runs tsh login prod, gives Bob a hand, and goes to lunch. Alice comes back, forgets to run tsh login testing, and proceeds to have a very bad day. There is a real benefit in having cluster be an explicit parameter: it forces users to acknowledge ambiguities and makes it easy for users to permanently resolve ambiguities.

@fspmarshall this argument is also valid for any tsh login <cluster> scenario, so it does not really change the UX.

Continue to brainstorming, so still I can't understand why the following can't work:

$ tsh ssh alice@example.com
error: ambiguous host could match multiple nodes
  
Node  Name  Node ID                              Address        Labels
----------- ------------------------------------ -------------- ---------
example.com ae79f375-633e-4a81-941e-83faf4a3c966 127.0.0.1:3022 foo=bar
example.com 17a980a9-ce32-4c95-a5c4-c4b4717851e9 127.0.0.1:4022 spam=eggs            
  
Hint: try addressing the node by unique id (ex: tctl ssh user@node-id)

$ tsh ssh  alice@17a980a9-ce32-4c95-a5c4-c4b4717851e9
...

webvictim · 2020-02-21T00:11:12Z

@fspmarshall this argument is also valid for any tsh login scenario, so it does not really change the UX.

Yes, and this has been on the “we should fix this” list for months. Several people requested that we improve the tsh workflow when using multiple clusters: #3089

russjones · 2020-03-05T18:08:33Z

Closing this in favor of #3377.

fspmarshall added 4 commits February 10, 2020 21:19

update deps

b4f1404

Duplicate node handling prototype

3556823

Duplicate node handling prototype 2

c37e918

improve msg

bb1107d

fspmarshall requested review from russjones and benarent February 11, 2020 18:35

benarent reviewed Feb 11, 2020

View reviewed changes

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

tool/tsh/tsh.go Show resolved Hide resolved

UUID based proxy connection params (WIP)

dcd84d9

fspmarshall force-pushed the fspmarshall/handle-duplicate-node-names branch from 5aa69eb to dcd84d9 Compare February 11, 2020 21:52

russjones reviewed Feb 11, 2020

View reviewed changes

fspmarshall added 3 commits February 12, 2020 13:16

improve comments & constants

f3dbd25

use HostPort

278030b

initial cleanup of proxyToHost

6cc2300

fspmarshall force-pushed the fspmarshall/handle-duplicate-node-names branch from 84777c8 to 6cc2300 Compare February 13, 2020 00:29

russjones approved these changes Feb 19, 2020

View reviewed changes

benarent requested changes Feb 19, 2020

View reviewed changes

fspmarshall mentioned this pull request Feb 24, 2020

Handle duplicate node names & support UUID based proxying (Version 2) #3377

Merged

russjones closed this Mar 5, 2020

zmb3 deleted the fspmarshall/handle-duplicate-node-names branch August 4, 2022 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle duplicate node names & support UUID based proxying. #3340

Handle duplicate node names & support UUID based proxying. #3340

fspmarshall commented Feb 11, 2020 •

edited

Loading

russjones Feb 11, 2020

russjones Feb 11, 2020 •

edited

Loading

fspmarshall Feb 12, 2020 •

edited

Loading

russjones left a comment

russjones Feb 19, 2020

russjones Feb 19, 2020

russjones Feb 19, 2020 •

edited

Loading

russjones Feb 19, 2020

russjones Feb 19, 2020

russjones Feb 19, 2020

russjones Feb 19, 2020

russjones Feb 19, 2020

russjones Feb 19, 2020

russjones Feb 19, 2020

benarent left a comment

benarent Feb 19, 2020

benarent commented Feb 19, 2020 •

edited

Loading

fspmarshall commented Feb 20, 2020

klizhentas commented Feb 20, 2020

klizhentas commented Feb 20, 2020

webvictim commented Feb 21, 2020

russjones commented Mar 5, 2020

		for i := range servers {
		// If nodeId is supplied, we must unambiguously match

Handle duplicate node names & support UUID based proxying. #3340

Handle duplicate node names & support UUID based proxying. #3340

Conversation

fspmarshall commented Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

russjones Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

fspmarshall Feb 12, 2020 • edited Loading

Choose a reason for hiding this comment

russjones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russjones Feb 19, 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

benarent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benarent commented Feb 19, 2020 • edited Loading

fspmarshall commented Feb 20, 2020

Feedback Discussion

Revised Proposal

klizhentas commented Feb 20, 2020

klizhentas commented Feb 20, 2020

webvictim commented Feb 21, 2020

russjones commented Mar 5, 2020

fspmarshall commented Feb 11, 2020 •

edited

Loading

russjones Feb 11, 2020 •

edited

Loading

fspmarshall Feb 12, 2020 •

edited

Loading

russjones Feb 19, 2020 •

edited

Loading

benarent commented Feb 19, 2020 •

edited

Loading