From 65e33fc1ee4798bb3450f6e291bfc88404982636 Mon Sep 17 00:00:00 2001 From: Mohammed Al Sahaf Date: Sat, 5 Aug 2023 23:30:02 +0200 Subject: reverseproxy: do not parse upstream address too early if it contains replaceble parts (#5695) * reverseproxy: do not parse upstream address too early if it contains replaceble parts * remove unused method * cleanup * accommodate partially replaceable port --- modules/caddyhttp/reverseproxy/addresses.go | 67 +++++++++++++++--------- modules/caddyhttp/reverseproxy/addresses_test.go | 10 ++-- modules/caddyhttp/reverseproxy/caddyfile.go | 20 ++++--- modules/caddyhttp/reverseproxy/command.go | 8 +-- 4 files changed, 65 insertions(+), 40 deletions(-) (limited to 'modules') diff --git a/modules/caddyhttp/reverseproxy/addresses.go b/modules/caddyhttp/reverseproxy/addresses.go index 6078f11..0939497 100644 --- a/modules/caddyhttp/reverseproxy/addresses.go +++ b/modules/caddyhttp/reverseproxy/addresses.go @@ -23,11 +23,43 @@ import ( "github.com/caddyserver/caddy/v2" ) +type parsedAddr struct { + network, scheme, host, port string + valid bool +} + +func (p parsedAddr) dialAddr() string { + if !p.valid { + return "" + } + // for simplest possible config, we only need to include + // the network portion if the user specified one + if p.network != "" { + return caddy.JoinNetworkAddress(p.network, p.host, p.port) + } + + // if the host is a placeholder, then we don't want to join with an empty port, + // because that would just append an extra ':' at the end of the address. + if p.port == "" && strings.Contains(p.host, "{") { + return p.host + } + return net.JoinHostPort(p.host, p.port) +} +func (p parsedAddr) rangedPort() bool { + return strings.Contains(p.port, "-") +} +func (p parsedAddr) replaceablePort() bool { + return strings.Contains(p.port, "{") && strings.Contains(p.port, "}") +} +func (p parsedAddr) isUnix() bool { + return caddy.IsUnixNetwork(p.network) +} + // parseUpstreamDialAddress parses configuration inputs for // the dial address, including support for a scheme in front // as a shortcut for the port number, and a network type, // for example 'unix' to dial a unix socket. -func parseUpstreamDialAddress(upstreamAddr string) (string, string, error) { +func parseUpstreamDialAddress(upstreamAddr string) (parsedAddr, error) { var network, scheme, host, port string if strings.Contains(upstreamAddr, "://") { @@ -35,7 +67,7 @@ func parseUpstreamDialAddress(upstreamAddr string) (string, string, error) { // so we return a more user-friendly error message instead // to explain what to do instead if strings.Contains(upstreamAddr, "{") { - return "", "", fmt.Errorf("due to parsing difficulties, placeholders are not allowed when an upstream address contains a scheme") + return parsedAddr{}, fmt.Errorf("due to parsing difficulties, placeholders are not allowed when an upstream address contains a scheme") } toURL, err := url.Parse(upstreamAddr) @@ -46,19 +78,19 @@ func parseUpstreamDialAddress(upstreamAddr string) (string, string, error) { if strings.Contains(err.Error(), "invalid port") && strings.Contains(err.Error(), "-") { index := strings.LastIndex(upstreamAddr, ":") if index == -1 { - return "", "", fmt.Errorf("parsing upstream URL: %v", err) + return parsedAddr{}, fmt.Errorf("parsing upstream URL: %v", err) } portRange := upstreamAddr[index+1:] if strings.Count(portRange, "-") != 1 { - return "", "", fmt.Errorf("parsing upstream URL: parse \"%v\": port range invalid: %v", upstreamAddr, portRange) + return parsedAddr{}, fmt.Errorf("parsing upstream URL: parse \"%v\": port range invalid: %v", upstreamAddr, portRange) } toURL, err = url.Parse(strings.ReplaceAll(upstreamAddr, portRange, "0")) if err != nil { - return "", "", fmt.Errorf("parsing upstream URL: %v", err) + return parsedAddr{}, fmt.Errorf("parsing upstream URL: %v", err) } port = portRange } else { - return "", "", fmt.Errorf("parsing upstream URL: %v", err) + return parsedAddr{}, fmt.Errorf("parsing upstream URL: %v", err) } } if port == "" { @@ -69,18 +101,18 @@ func parseUpstreamDialAddress(upstreamAddr string) (string, string, error) { // a backend and proxying to it, so we cannot allow extra components // in backend URLs if toURL.Path != "" || toURL.RawQuery != "" || toURL.Fragment != "" { - return "", "", fmt.Errorf("for now, URLs for proxy upstreams only support scheme, host, and port components") + return parsedAddr{}, fmt.Errorf("for now, URLs for proxy upstreams only support scheme, host, and port components") } // ensure the port and scheme aren't in conflict if toURL.Scheme == "http" && port == "443" { - return "", "", fmt.Errorf("upstream address has conflicting scheme (http://) and port (:443, the HTTPS port)") + return parsedAddr{}, fmt.Errorf("upstream address has conflicting scheme (http://) and port (:443, the HTTPS port)") } if toURL.Scheme == "https" && port == "80" { - return "", "", fmt.Errorf("upstream address has conflicting scheme (https://) and port (:80, the HTTP port)") + return parsedAddr{}, fmt.Errorf("upstream address has conflicting scheme (https://) and port (:80, the HTTP port)") } if toURL.Scheme == "h2c" && port == "443" { - return "", "", fmt.Errorf("upstream address has conflicting scheme (h2c://) and port (:443, the HTTPS port)") + return parsedAddr{}, fmt.Errorf("upstream address has conflicting scheme (h2c://) and port (:443, the HTTPS port)") } // if port is missing, attempt to infer from scheme @@ -112,18 +144,5 @@ func parseUpstreamDialAddress(upstreamAddr string) (string, string, error) { network = "unix" scheme = "h2c" } - - // for simplest possible config, we only need to include - // the network portion if the user specified one - if network != "" { - return caddy.JoinNetworkAddress(network, host, port), scheme, nil - } - - // if the host is a placeholder, then we don't want to join with an empty port, - // because that would just append an extra ':' at the end of the address. - if port == "" && strings.Contains(host, "{") { - return host, scheme, nil - } - - return net.JoinHostPort(host, port), scheme, nil + return parsedAddr{network, scheme, host, port, true}, nil } diff --git a/modules/caddyhttp/reverseproxy/addresses_test.go b/modules/caddyhttp/reverseproxy/addresses_test.go index 0c7ad7b..0c51419 100644 --- a/modules/caddyhttp/reverseproxy/addresses_test.go +++ b/modules/caddyhttp/reverseproxy/addresses_test.go @@ -265,18 +265,18 @@ func TestParseUpstreamDialAddress(t *testing.T) { expectScheme: "h2c", }, } { - actualHostPort, actualScheme, err := parseUpstreamDialAddress(tc.input) + actualAddr, err := parseUpstreamDialAddress(tc.input) if tc.expectErr && err == nil { t.Errorf("Test %d: Expected error but got %v", i, err) } if !tc.expectErr && err != nil { t.Errorf("Test %d: Expected no error but got %v", i, err) } - if actualHostPort != tc.expectHostPort { - t.Errorf("Test %d: Expected host and port '%s' but got '%s'", i, tc.expectHostPort, actualHostPort) + if actualAddr.dialAddr() != tc.expectHostPort { + t.Errorf("Test %d: input %s: Expected host and port '%s' but got '%s'", i, tc.input, tc.expectHostPort, actualAddr.dialAddr()) } - if actualScheme != tc.expectScheme { - t.Errorf("Test %d: Expected scheme '%s' but got '%s'", i, tc.expectScheme, actualScheme) + if actualAddr.scheme != tc.expectScheme { + t.Errorf("Test %d: Expected scheme '%s' but got '%s'", i, tc.expectScheme, actualAddr.scheme) } } } diff --git a/modules/caddyhttp/reverseproxy/caddyfile.go b/modules/caddyhttp/reverseproxy/caddyfile.go index 1d86beb..f5eb7a5 100644 --- a/modules/caddyhttp/reverseproxy/caddyfile.go +++ b/modules/caddyhttp/reverseproxy/caddyfile.go @@ -146,7 +146,7 @@ func (h *Handler) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { // appendUpstream creates an upstream for address and adds // it to the list. appendUpstream := func(address string) error { - dialAddr, scheme, err := parseUpstreamDialAddress(address) + pa, err := parseUpstreamDialAddress(address) if err != nil { return d.WrapErr(err) } @@ -154,21 +154,27 @@ func (h *Handler) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { // the underlying JSON does not yet support different // transports (protocols or schemes) to each backend, // so we remember the last one we see and compare them - if commonScheme != "" && scheme != commonScheme { + if commonScheme != "" && pa.scheme != commonScheme { return d.Errf("for now, all proxy upstreams must use the same scheme (transport protocol); expecting '%s://' but got '%s://'", - commonScheme, scheme) + commonScheme, pa.scheme) } - commonScheme = scheme + commonScheme = pa.scheme - parsedAddr, err := caddy.ParseNetworkAddress(dialAddr) + // if the port of upstream address contains a placeholder, only wrap it with the `Upstream` struct, + // delaying actual resolution of the address until request time. + if pa.replaceablePort() { + h.Upstreams = append(h.Upstreams, &Upstream{Dial: pa.dialAddr()}) + return nil + } + parsedAddr, err := caddy.ParseNetworkAddress(pa.dialAddr()) if err != nil { return d.WrapErr(err) } - if parsedAddr.StartPort == 0 && parsedAddr.EndPort == 0 { + if pa.isUnix() || !pa.rangedPort() { // unix networks don't have ports h.Upstreams = append(h.Upstreams, &Upstream{ - Dial: dialAddr, + Dial: pa.dialAddr(), }) } else { // expand a port range into multiple upstreams diff --git a/modules/caddyhttp/reverseproxy/command.go b/modules/caddyhttp/reverseproxy/command.go index 48fabd5..8438b72 100644 --- a/modules/caddyhttp/reverseproxy/command.go +++ b/modules/caddyhttp/reverseproxy/command.go @@ -132,14 +132,14 @@ func cmdReverseProxy(fs caddycmd.Flags) (int, error) { toAddresses := make([]string, len(to)) var toScheme string for i, toLoc := range to { - addr, scheme, err := parseUpstreamDialAddress(toLoc) + addr, err := parseUpstreamDialAddress(toLoc) if err != nil { return caddy.ExitCodeFailedStartup, fmt.Errorf("invalid upstream address %s: %v", toLoc, err) } - if scheme != "" && toScheme == "" { - toScheme = scheme + if addr.scheme != "" && toScheme == "" { + toScheme = addr.scheme } - toAddresses[i] = addr + toAddresses[i] = addr.dialAddr() } // proceed to build the handler and server -- cgit v1.2.3