From 7b8b3f73e7330e1dc7ab723a5765076af46934b0 Mon Sep 17 00:00:00 2001 From: Devin Carr Date: Thu, 9 Mar 2023 15:23:11 -0800 Subject: [PATCH] TUN-7259: Add warning for missing ingress rules Providing no ingress rules in the configuration file or via the CLI will now provide a warning and return 502 for all incoming HTTP requests. --- CHANGES.md | 4 + cmd/cloudflared/tunnel/configuration.go | 16 +--- component-tests/config.py | 3 +- component-tests/conftest.py | 13 ++- component-tests/test_quicktunnels.py | 3 +- component-tests/test_tunnel.py | 43 +++++++++ ingress/config.go | 2 +- ingress/config_test.go | 2 +- ingress/ingress.go | 72 ++++++++++++--- ingress/ingress_test.go | 117 +++++++++++++++++++++++- ingress/origin_proxy.go | 3 + ingress/origin_service.go | 13 +++ proxy/proxy_test.go | 3 +- 13 files changed, 255 insertions(+), 39 deletions(-) create mode 100644 component-tests/test_tunnel.py diff --git a/CHANGES.md b/CHANGES.md index e5ea9ddf..e34f8615 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,3 +1,7 @@ +## 2023.3.1 +### Breaking Change +- Running a tunnel without ingress rules defined in configuration file nor from the CLI flags will no longer provide a default ingress rule to localhost:8080 and instead will return HTTP response code 502 for all incoming HTTP requests. + ## 2023.2.2 ### Notices - Legacy tunnels were officially deprecated on December 1, 2022. Starting with this version, cloudflared no longer supports connecting legacy tunnels. diff --git a/cmd/cloudflared/tunnel/configuration.go b/cmd/cloudflared/tunnel/configuration.go index 7f1488ab..7fa86273 100644 --- a/cmd/cloudflared/tunnel/configuration.go +++ b/cmd/cloudflared/tunnel/configuration.go @@ -227,22 +227,10 @@ func prepareTunnelConfig( Arch: info.OSArch(), } cfg := config.GetConfiguration() - ingressRules, err := ingress.ParseIngress(cfg) - if err != nil && err != ingress.ErrNoIngressRules { + ingressRules, err := ingress.ParseIngressFromConfigAndCLI(cfg, c, log) + if err != nil { return nil, nil, err } - if c.IsSet("url") || c.IsSet(ingress.HelloWorldFlag) || c.IsSet(config.BastionFlag) { - // Ingress rules cannot be provided with --url, --hello-world or --bastion flag - if !ingressRules.IsEmpty() { - return nil, nil, ingress.ErrURLIncompatibleWithIngress - } - // Only for quick or adhoc tunnels will we attempt to parse: - // --url, --hello-world, --bastion, or --unix-socket flag for a tunnel ingress rule - ingressRules, err = ingress.NewSingleOrigin(c, false) - if err != nil { - return nil, nil, err - } - } protocolSelector, err := connection.NewProtocolSelector(transportProtocol, namedTunnel.Credentials.AccountTag, c.IsSet(TunnelTokenFlag), c.Bool("post-quantum"), edgediscovery.ProtocolPercentage, connection.ResolveTTL, log) if err != nil { diff --git a/component-tests/config.py b/component-tests/config.py index fc48623f..7d811641 100644 --- a/component-tests/config.py +++ b/component-tests/config.py @@ -30,6 +30,7 @@ class NamedTunnelBaseConfig(BaseConfig): tunnel: str = None credentials_file: str = None ingress: list = None + hostname: str = None def __post_init__(self): if self.tunnel is None: @@ -63,7 +64,7 @@ class NamedTunnelConfig(NamedTunnelBaseConfig): self.merge_config(additional_config)) def get_url(self): - return "https://" + self.ingress[0]['hostname'] + return "https://" + self.hostname def base_config(self): config = self.full_config.copy() diff --git a/component-tests/conftest.py b/component-tests/conftest.py index 3e6a7603..942c9b91 100644 --- a/component-tests/conftest.py +++ b/component-tests/conftest.py @@ -26,7 +26,7 @@ def component_tests_config(): config = yaml.safe_load(stream) LOGGER.info(f"component tests base config {config}") - def _component_tests_config(additional_config={}, cfd_mode=CfdModes.NAMED, run_proxy_dns=True): + def _component_tests_config(additional_config={}, cfd_mode=CfdModes.NAMED, run_proxy_dns=True, provide_ingress=True): if run_proxy_dns: # Regression test for TUN-4177, running with proxy-dns should not prevent tunnels from running. # So we run all tests with it. @@ -36,12 +36,21 @@ def component_tests_config(): additional_config.pop("proxy-dns", None) additional_config.pop("proxy-dns-port", None) + # Allows the ingress rules to be omitted from the provided config + ingress = [] + if provide_ingress: + ingress = config['ingress'] + + # Provide the hostname to allow routing to the tunnel even if the ingress rule isn't defined in the config + hostname = config['ingress'][0]['hostname'] + if cfd_mode is CfdModes.NAMED: return NamedTunnelConfig(additional_config=additional_config, cloudflared_binary=config['cloudflared_binary'], tunnel=config['tunnel'], credentials_file=config['credentials_file'], - ingress=config['ingress']) + ingress=ingress, + hostname=hostname) elif cfd_mode is CfdModes.PROXY_DNS: return ProxyDnsConfig(cloudflared_binary=config['cloudflared_binary']) elif cfd_mode is CfdModes.QUICK: diff --git a/component-tests/test_quicktunnels.py b/component-tests/test_quicktunnels.py index f6d32259..cb58cde4 100644 --- a/component-tests/test_quicktunnels.py +++ b/component-tests/test_quicktunnels.py @@ -1,10 +1,9 @@ #!/usr/bin/env python -import requests from conftest import CfdModes from constants import METRICS_PORT from util import LOGGER, start_cloudflared, wait_tunnel_ready, get_quicktunnel_url, send_requests -class TestCLI: +class TestQuickTunnels: def test_quick_tunnel(self, tmp_path, component_tests_config): config = component_tests_config(cfd_mode=CfdModes.QUICK, run_proxy_dns=False) LOGGER.debug(config) diff --git a/component-tests/test_tunnel.py b/component-tests/test_tunnel.py new file mode 100644 index 00000000..9a193aec --- /dev/null +++ b/component-tests/test_tunnel.py @@ -0,0 +1,43 @@ +#!/usr/bin/env python +import requests +from conftest import CfdModes +from constants import METRICS_PORT, MAX_RETRIES, BACKOFF_SECS +from retrying import retry +from util import LOGGER, start_cloudflared, wait_tunnel_ready, send_requests + +class TestTunnel: + '''Test tunnels with no ingress rules from config.yaml but ingress rules from CLI only''' + + def test_tunnel_hello_world(self, tmp_path, component_tests_config): + config = component_tests_config(cfd_mode=CfdModes.NAMED, run_proxy_dns=False, provide_ingress=False) + LOGGER.debug(config) + with start_cloudflared(tmp_path, config, cfd_args=["run", "--hello-world"], new_process=True): + wait_tunnel_ready(tunnel_url=config.get_url(), + require_min_connections=4) + + def test_tunnel_url(self, tmp_path, component_tests_config): + config = component_tests_config(cfd_mode=CfdModes.NAMED, run_proxy_dns=False, provide_ingress=False) + LOGGER.debug(config) + with start_cloudflared(tmp_path, config, cfd_args=["run", "--url", f"http://localhost:{METRICS_PORT}/"], new_process=True): + wait_tunnel_ready(require_min_connections=4) + send_requests(config.get_url()+"/ready", 3, True) + + def test_tunnel_no_ingress(self, tmp_path, component_tests_config): + ''' + Running a tunnel with no ingress rules provided from either config.yaml or CLI will still work but return 502 + for all incoming requests. + ''' + config = component_tests_config(cfd_mode=CfdModes.NAMED, run_proxy_dns=False, provide_ingress=False) + LOGGER.debug(config) + with start_cloudflared(tmp_path, config, cfd_args=["run"], new_process=True): + wait_tunnel_ready(require_min_connections=4) + resp = send_request(config.get_url()+"/") + assert resp.status_code == 502, "Expected cloudflared to return 502 for all requests with no ingress defined" + resp = send_request(config.get_url()+"/test") + assert resp.status_code == 502, "Expected cloudflared to return 502 for all requests with no ingress defined" + + +@retry(stop_max_attempt_number=MAX_RETRIES, wait_fixed=BACKOFF_SECS * 1000) +def send_request(url): + with requests.Session() as s: + return s.get(url, timeout=BACKOFF_SECS) diff --git a/ingress/config.go b/ingress/config.go index 212180e6..25f7993a 100644 --- a/ingress/config.go +++ b/ingress/config.go @@ -113,7 +113,7 @@ func (rc *RemoteConfig) UnmarshalJSON(b []byte) error { return nil } -func originRequestFromSingeRule(c *cli.Context) OriginRequestConfig { +func originRequestFromSingleRule(c *cli.Context) OriginRequestConfig { var connectTimeout = defaultHTTPConnectTimeout var tlsTimeout = defaultTLSTimeout var tcpKeepAlive = defaultTCPKeepAlive diff --git a/ingress/config_test.go b/ingress/config_test.go index 2b9f8a3b..249cf2d5 100644 --- a/ingress/config_test.go +++ b/ingress/config_test.go @@ -411,7 +411,7 @@ func TestDefaultConfigFromCLI(t *testing.T) { KeepAliveTimeout: defaultKeepAliveTimeout, ProxyAddress: defaultProxyAddress, } - actual := originRequestFromSingeRule(c) + actual := originRequestFromSingleRule(c) require.Equal(t, expected, actual) } diff --git a/ingress/ingress.go b/ingress/ingress.go index ccee3978..2b20fc48 100644 --- a/ingress/ingress.go +++ b/ingress/ingress.go @@ -20,6 +20,7 @@ import ( var ( ErrNoIngressRules = errors.New("The config file doesn't contain any ingress rules") + ErrNoIngressRulesCLI = errors.New("No ingress rules were defined in provided config (if any) nor from the cli, cloudflared will return 502 for all incoming HTTP requests") errLastRuleNotCatchAll = errors.New("The last ingress rule must match all URLs (i.e. it should not have a hostname or path filter)") errBadWildcard = errors.New("Hostname patterns can have at most one wildcard character (\"*\") and it can only be used for subdomains, e.g. \"*.example.com\"") errHostnameContainsPort = errors.New("Hostname cannot contain a port") @@ -70,16 +71,52 @@ type Ingress struct { Defaults OriginRequestConfig `json:"originRequest"` } -// NewSingleOrigin constructs an Ingress set with only one rule, constructed from -// CLI parameters for quick tunnels like --url or --no-chunked-encoding. -func NewSingleOrigin(c *cli.Context, allowURLFromArgs bool) (Ingress, error) { +// ParseIngress parses ingress rules, but does not send HTTP requests to the origins. +func ParseIngress(conf *config.Configuration) (Ingress, error) { + if len(conf.Ingress) == 0 { + return Ingress{}, ErrNoIngressRules + } + return validateIngress(conf.Ingress, originRequestFromConfig(conf.OriginRequest)) +} + +// ParseIngressFromConfigAndCLI will parse the configuration rules from config files for ingress +// rules and then attempt to parse CLI for ingress rules. +// Will always return at least one valid ingress rule. If none are provided by the user, the default +// will be to return 502 status code for all incoming requests. +func ParseIngressFromConfigAndCLI(conf *config.Configuration, c *cli.Context, log *zerolog.Logger) (Ingress, error) { + // Attempt to parse ingress rules from configuration + ingressRules, err := ParseIngress(conf) + if err == nil && !ingressRules.IsEmpty() { + return ingressRules, nil + } + if err != ErrNoIngressRules { + return Ingress{}, err + } + // Attempt to parse ingress rules from CLI: + // --url or --unix-socket flag for a tunnel HTTP ingress + // --hello-world for a basic HTTP ingress self-served + // --bastion for ssh bastion service + ingressRules, err = parseCLIIngress(c, false) + if errors.Is(err, ErrNoIngressRulesCLI) { + log.Warn().Msgf(ErrNoIngressRulesCLI.Error()) + return newDefaultOrigin(c, log), nil + } + if err != nil { + return Ingress{}, err + } + return ingressRules, nil +} + +// parseCLIIngress constructs an Ingress set with only one rule constructed from +// CLI parameters: --url, --hello-world, --bastion, or --unix-socket +func parseCLIIngress(c *cli.Context, allowURLFromArgs bool) (Ingress, error) { service, err := parseSingleOriginService(c, allowURLFromArgs) if err != nil { return Ingress{}, err } // Construct an Ingress with the single rule. - defaults := originRequestFromSingeRule(c) + defaults := originRequestFromSingleRule(c) ing := Ingress{ Rules: []Rule{ { @@ -92,6 +129,22 @@ func NewSingleOrigin(c *cli.Context, allowURLFromArgs bool) (Ingress, error) { return ing, err } +// newDefaultOrigin always returns a 502 response code to help indicate that there are no ingress +// rules setup, but the tunnel is reachable. +func newDefaultOrigin(c *cli.Context, log *zerolog.Logger) Ingress { + noRulesService := newDefaultStatusCode(log) + defaults := originRequestFromSingleRule(c) + ingress := Ingress{ + Rules: []Rule{ + { + Service: &noRulesService, + }, + }, + Defaults: defaults, + } + return ingress +} + // WarpRoutingService starts a tcp stream between the origin and requests from // warp clients. type WarpRoutingService struct { @@ -137,8 +190,7 @@ func parseSingleOriginService(c *cli.Context, allowURLFromArgs bool) (OriginServ } return &unixSocketPath{path: path, scheme: "http"}, nil } - u, err := url.Parse("http://localhost:8080") - return &httpService{url: u}, err + return nil, ErrNoIngressRulesCLI } // IsEmpty checks if there are any ingress rules. @@ -335,14 +387,6 @@ func (e errRuleShouldNotBeCatchAll) Error() string { "will never be triggered.", e.index+1, e.hostname) } -// ParseIngress parses ingress rules, but does not send HTTP requests to the origins. -func ParseIngress(conf *config.Configuration) (Ingress, error) { - if len(conf.Ingress) == 0 { - return Ingress{}, ErrNoIngressRules - } - return validateIngress(conf.Ingress, originRequestFromConfig(conf.OriginRequest)) -} - func isHTTPService(url *url.URL) bool { return url.Scheme == "http" || url.Scheme == "https" || url.Scheme == "ws" || url.Scheme == "wss" } diff --git a/ingress/ingress_test.go b/ingress/ingress_test.go index dd54c1c0..affbbf97 100644 --- a/ingress/ingress_test.go +++ b/ingress/ingress_test.go @@ -43,7 +43,7 @@ ingress: require.Equal(t, "https", s.scheme) } -func Test_parseIngress(t *testing.T) { +func TestParseIngress(t *testing.T) { localhost8000 := MustParseURL(t, "https://localhost:8000") localhost8001 := MustParseURL(t, "https://localhost:8001") fourOhFour := newStatusCode(404) @@ -517,7 +517,7 @@ func TestSingleOriginSetsConfig(t *testing.T) { allowURLFromArgs := false require.NoError(t, err) - ingress, err := NewSingleOrigin(cliCtx, allowURLFromArgs) + ingress, err := parseCLIIngress(cliCtx, allowURLFromArgs) require.NoError(t, err) assert.Equal(t, config.CustomDuration{Duration: time.Second}, ingress.Rules[0].Config.ConnectTimeout) @@ -537,6 +537,119 @@ func TestSingleOriginSetsConfig(t *testing.T) { assert.Equal(t, socksProxy, ingress.Rules[0].Config.ProxyType) } +func TestSingleOriginServices(t *testing.T) { + host := "://localhost:8080" + httpURL := urlMustParse("http" + host) + tcpURL := urlMustParse("tcp" + host) + unix := "unix://service" + newCli := func(params ...string) *cli.Context { + flagSet := flag.NewFlagSet(t.Name(), flag.PanicOnError) + flagSet.Bool("hello-world", false, "") + flagSet.Bool("bastion", false, "") + flagSet.String("url", "", "") + flagSet.String("unix-socket", "", "") + cliCtx := cli.NewContext(cli.NewApp(), flagSet, nil) + for i := 0; i < len(params); i += 2 { + cliCtx.Set(params[i], params[i+1]) + } + + return cliCtx + } + + tests := []struct { + name string + cli *cli.Context + expectedService OriginService + err error + }{ + { + name: "Valid hello-world", + cli: newCli("hello-world", "true"), + expectedService: &helloWorld{}, + }, + { + name: "Valid bastion", + cli: newCli("bastion", "true"), + expectedService: newBastionService(), + }, + { + name: "Valid http url", + cli: newCli("url", httpURL.String()), + expectedService: &httpService{url: httpURL}, + }, + { + name: "Valid tcp url", + cli: newCli("url", tcpURL.String()), + expectedService: newTCPOverWSService(tcpURL), + }, + { + name: "Valid unix-socket", + cli: newCli("unix-socket", unix), + expectedService: &unixSocketPath{path: unix, scheme: "http"}, + }, + { + name: "No origins defined", + cli: newCli(), + err: ErrNoIngressRulesCLI, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ingress, err := parseCLIIngress(test.cli, false) + require.Equal(t, err, test.err) + if test.err != nil { + return + } + require.Equal(t, 1, len(ingress.Rules)) + rule := ingress.Rules[0] + require.Equal(t, test.expectedService, rule.Service) + }) + } +} + +func urlMustParse(s string) *url.URL { + u, err := url.Parse(s) + if err != nil { + panic(err) + } + return u +} + +func TestSingleOriginServices_URL(t *testing.T) { + host := "://localhost:8080" + newCli := func(param string, value string) *cli.Context { + flagSet := flag.NewFlagSet(t.Name(), flag.PanicOnError) + flagSet.String("url", "", "") + cliCtx := cli.NewContext(cli.NewApp(), flagSet, nil) + cliCtx.Set(param, value) + return cliCtx + } + + httpTests := []string{"http", "https"} + for _, test := range httpTests { + t.Run(test, func(t *testing.T) { + url := urlMustParse(test + host) + ingress, err := parseCLIIngress(newCli("url", url.String()), false) + require.NoError(t, err) + require.Equal(t, 1, len(ingress.Rules)) + rule := ingress.Rules[0] + require.Equal(t, &httpService{url: url}, rule.Service) + }) + } + + tcpTests := []string{"ssh", "rdp", "smb", "tcp"} + for _, test := range tcpTests { + t.Run(test, func(t *testing.T) { + url := urlMustParse(test + host) + ingress, err := parseCLIIngress(newCli("url", url.String()), false) + require.NoError(t, err) + require.Equal(t, 1, len(ingress.Rules)) + rule := ingress.Rules[0] + require.Equal(t, newTCPOverWSService(url), rule.Service) + }) + } +} + func TestFindMatchingRule(t *testing.T) { ingress := Ingress{ Rules: []Rule{ diff --git a/ingress/origin_proxy.go b/ingress/origin_proxy.go index 83bcd4fc..232ef2f6 100644 --- a/ingress/origin_proxy.go +++ b/ingress/origin_proxy.go @@ -44,6 +44,9 @@ func (o *httpService) RoundTrip(req *http.Request) (*http.Response, error) { } func (o *statusCode) RoundTrip(_ *http.Request) (*http.Response, error) { + if o.defaultResp { + o.log.Warn().Msgf(ErrNoIngressRulesCLI.Error()) + } resp := &http.Response{ StatusCode: o.code, Status: fmt.Sprintf("%d %s", o.code, http.StatusText(o.code)), diff --git a/ingress/origin_service.go b/ingress/origin_service.go index 7fc8201e..ea99a80e 100644 --- a/ingress/origin_service.go +++ b/ingress/origin_service.go @@ -247,13 +247,26 @@ func (o helloWorld) MarshalJSON() ([]byte, error) { // Typical use-case is "user wants the catch-all rule to just respond 404". type statusCode struct { code int + + // Set only when the user has not defined any ingress rules + defaultResp bool + log *zerolog.Logger } func newStatusCode(status int) statusCode { return statusCode{code: status} } +// default status code (502) that is returned for requests to cloudflared that don't have any ingress rules setup +func newDefaultStatusCode(log *zerolog.Logger) statusCode { + return statusCode{code: 502, defaultResp: true, log: log} +} + func (o *statusCode) String() string { + // returning a different service name can help with identifying users via config that don't setup ingress rules + if o.defaultResp { + return "default_http_status:502" + } return fmt.Sprintf("http_status:%d", o.code) } diff --git a/proxy/proxy_test.go b/proxy/proxy_test.go index 81e351e5..b3b1e30b 100644 --- a/proxy/proxy_test.go +++ b/proxy/proxy_test.go @@ -149,8 +149,7 @@ func TestProxySingleOrigin(t *testing.T) { err := cliCtx.Set("hello-world", "true") require.NoError(t, err) - allowURLFromArgs := false - ingressRule, err := ingress.NewSingleOrigin(cliCtx, allowURLFromArgs) + ingressRule, err := ingress.ParseIngressFromConfigAndCLI(&config.Configuration{}, cliCtx, &log) require.NoError(t, err) require.NoError(t, ingressRule.StartOrigins(&log, ctx.Done()))