From 7a3ae39025a1307a096594e10e53841c12e32184 Mon Sep 17 00:00:00 2001 From: Tyler Smalley Date: Thu, 19 Oct 2023 11:03:06 -0700 Subject: [PATCH] cmd/tailscale/cli: [serve/funnel] support omitting scheme for TCP (#9856) The `serve` command for TCP has always required the scheme of the target to be specified. However, when it's omitted the error message reported is misleading ``` error: failed to apply TCP serve: invalid TCP target "localhost:5900": missing port in address ``` Since we know the target is TCP, we shouldn't require it to be specified. This aligns with the changes for HTTP proxies in https://github.com/tailscale/tailscale/issues/8489 closes #9855 Signed-off-by: Tyler Smalley --- cmd/tailscale/cli/serve_v2.go | 43 +++++++------------- cmd/tailscale/cli/serve_v2_test.go | 63 ++++++++++++++++++++---------- 2 files changed, 57 insertions(+), 49 deletions(-) diff --git a/cmd/tailscale/cli/serve_v2.go b/cmd/tailscale/cli/serve_v2.go index 5bc996819..04156d20e 100644 --- a/cmd/tailscale/cli/serve_v2.go +++ b/cmd/tailscale/cli/serve_v2.go @@ -16,6 +16,7 @@ import ( "os/signal" "path" "path/filepath" + "slices" "sort" "strconv" "strings" @@ -502,7 +503,7 @@ func (e *serveEnv) applyWebServe(sc *ipn.ServeConfig, dnsName string, srvPort ui } h.Path = target default: - t, err := expandProxyTargetDev(target) + t, err := expandProxyTargetDev(target, []string{"http", "https", "https+insecure"}, "http") if err != nil { return err } @@ -552,34 +553,22 @@ func (e *serveEnv) applyTCPServe(sc *ipn.ServeConfig, dnsName string, srcType se return fmt.Errorf("invalid TCP target %q", target) } - dstURL, err := url.Parse(target) + targetURL, err := expandProxyTargetDev(target, []string{"tcp"}, "tcp") if err != nil { - return fmt.Errorf("invalid TCP target %q: %v", target, err) + return fmt.Errorf("unable to expand target: %v", err) } - host, dstPortStr, err := net.SplitHostPort(dstURL.Host) + + dstURL, err := url.Parse(targetURL) if err != nil { return fmt.Errorf("invalid TCP target %q: %v", target, err) } - switch host { - case "localhost", "127.0.0.1": - // ok - default: - return fmt.Errorf("invalid TCP target %q, must be one of localhost or 127.0.0.1", target) - } - - if p, err := strconv.ParseUint(dstPortStr, 10, 16); p == 0 || err != nil { - return fmt.Errorf("invalid port %q", dstPortStr) - } - - fwdAddr := "127.0.0.1:" + dstPortStr - // TODO: needs to account for multiple configs from foreground mode if sc.IsServingWeb(srcPort) { return fmt.Errorf("cannot serve TCP; already serving web on %d", srcPort) } - mak.Set(&sc.TCP, srcPort, &ipn.TCPPortHandler{TCPForward: fwdAddr}) + mak.Set(&sc.TCP, srcPort, &ipn.TCPPortHandler{TCPForward: dstURL.Host}) if terminateTLS { sc.TCP[srcPort].TerminateTLS = dnsName @@ -739,24 +728,22 @@ func (e *serveEnv) removeTCPServe(sc *ipn.ServeConfig, src uint16) error { // examples: // - 3000 // - localhost:3000 +// - tcp://localhost:3000 // - http://localhost:3000 // - https://localhost:3000 // - https-insecure://localhost:3000 // - https-insecure://localhost:3000/foo -func expandProxyTargetDev(target string) (string, error) { - var ( - scheme = "http" - host = "127.0.0.1" - ) +func expandProxyTargetDev(target string, supportedSchemes []string, defaultScheme string) (string, error) { + var host = "127.0.0.1" // support target being a port number if port, err := strconv.ParseUint(target, 10, 16); err == nil { - return fmt.Sprintf("%s://%s:%d", scheme, host, port), nil + return fmt.Sprintf("%s://%s:%d", defaultScheme, host, port), nil } // prepend scheme if not present if !strings.Contains(target, "://") { - target = scheme + "://" + target + target = defaultScheme + "://" + target } // make sure we can parse the target @@ -766,10 +753,8 @@ func expandProxyTargetDev(target string) (string, error) { } // ensure a supported scheme - switch u.Scheme { - case "http", "https", "https+insecure": - default: - return "", errors.New("must be a URL starting with http://, https://, or https+insecure://") + if !slices.Contains(supportedSchemes, u.Scheme) { + return "", fmt.Errorf("must be a URL starting with one of the supported schemes: %v", supportedSchemes) } // validate the port diff --git a/cmd/tailscale/cli/serve_v2_test.go b/cmd/tailscale/cli/serve_v2_test.go index 25f1e6223..4ebf40be1 100644 --- a/cmd/tailscale/cli/serve_v2_test.go +++ b/cmd/tailscale/cli/serve_v2_test.go @@ -367,10 +367,6 @@ func TestServeDevConfigMutations(t *testing.T) { // // tcp add(step{reset: true}) - add(step{ // must include scheme for tcp - command: cmd("serve --tls-terminated-tcp=443 --bg localhost:5432"), - wantErr: exactErr(errHelp, "errHelp"), - }) add(step{ // !somehost, must be localhost or 127.0.0.1 command: cmd("serve --tls-terminated-tcp=443 --bg tcp://somehost:5432"), wantErr: exactErr(errHelp, "errHelp"), @@ -383,6 +379,18 @@ func TestServeDevConfigMutations(t *testing.T) { command: cmd("serve --tls-terminated-tcp=443 --bg tcp://somehost:65536"), wantErr: exactErr(errHelp, "errHelp"), }) + add(step{ // support shorthand + command: cmd("serve --tls-terminated-tcp=443 --bg 5432"), + want: &ipn.ServeConfig{ + TCP: map[uint16]*ipn.TCPPortHandler{ + 443: { + TCPForward: "127.0.0.1:5432", + TerminateTLS: "foo.test.ts.net", + }, + }, + }, + }) + add(step{reset: true}) add(step{ command: cmd("serve --tls-terminated-tcp=443 --bg tcp://localhost:5432"), want: &ipn.ServeConfig{ @@ -956,28 +964,43 @@ func TestSrcTypeFromFlags(t *testing.T) { func TestExpandProxyTargetDev(t *testing.T) { tests := []struct { - input string - expected string - wantErr bool + name string + input string + defaultScheme string + supportedSchemes []string + expected string + wantErr bool }{ - {input: "8080", expected: "http://127.0.0.1:8080"}, - {input: "localhost:8080", expected: "http://127.0.0.1:8080"}, - {input: "http://localhost:8080", expected: "http://127.0.0.1:8080"}, - {input: "http://127.0.0.1:8080", expected: "http://127.0.0.1:8080"}, - {input: "http://127.0.0.1:8080/foo", expected: "http://127.0.0.1:8080/foo"}, - {input: "https://localhost:8080", expected: "https://127.0.0.1:8080"}, - {input: "https+insecure://localhost:8080", expected: "https+insecure://127.0.0.1:8080"}, + {name: "port-only", input: "8080", expected: "http://127.0.0.1:8080"}, + {name: "hostname+port", input: "localhost:8080", expected: "http://127.0.0.1:8080"}, + {name: "convert-localhost", input: "http://localhost:8080", expected: "http://127.0.0.1:8080"}, + {name: "no-change", input: "http://127.0.0.1:8080", expected: "http://127.0.0.1:8080"}, + {name: "include-path", input: "http://127.0.0.1:8080/foo", expected: "http://127.0.0.1:8080/foo"}, + {name: "https-scheme", input: "https://localhost:8080", expected: "https://127.0.0.1:8080"}, + {name: "https+insecure-scheme", input: "https+insecure://localhost:8080", expected: "https+insecure://127.0.0.1:8080"}, + {name: "change-default-scheme", input: "localhost:8080", defaultScheme: "https", expected: "https://127.0.0.1:8080"}, + {name: "change-supported-schemes", input: "localhost:8080", defaultScheme: "tcp", supportedSchemes: []string{"tcp"}, expected: "tcp://127.0.0.1:8080"}, // errors - {input: "localhost:9999999", wantErr: true}, - {input: "ftp://localhost:8080", expected: "", wantErr: true}, - {input: "https://tailscale.com:8080", expected: "", wantErr: true}, - {input: "", expected: "", wantErr: true}, + {name: "invalid-port", input: "localhost:9999999", wantErr: true}, + {name: "unsupported-scheme", input: "ftp://localhost:8080", expected: "", wantErr: true}, + {name: "not-localhost", input: "https://tailscale.com:8080", expected: "", wantErr: true}, + {name: "empty-input", input: "", expected: "", wantErr: true}, } for _, tt := range tests { - t.Run(tt.input, func(t *testing.T) { - actual, err := expandProxyTargetDev(tt.input) + defaultScheme := "http" + supportedSchemes := []string{"http", "https", "https+insecure"} + + if tt.supportedSchemes != nil { + supportedSchemes = tt.supportedSchemes + } + if tt.defaultScheme != "" { + defaultScheme = tt.defaultScheme + } + + t.Run(tt.name, func(t *testing.T) { + actual, err := expandProxyTargetDev(tt.input, supportedSchemes, defaultScheme) if tt.wantErr == true && err == nil { t.Errorf("Expected an error but got none")