From cd7b75c4b239b6a046566e53f91747175f471077 Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Thu, 23 Jun 2022 00:34:15 +0000 Subject: [PATCH] Add back validatation that the scheme matches the wire protocol This adds back in the scheme and wire protocol check (see #8465) along with a configuration to be able to disable the check if the verification is not desired. --- doc/admin-guide/files/records.config.en.rst | 23 +++++++ mgmt/RecordsConfig.cc | 2 + proxy/http/HttpConfig.cc | 2 + proxy/http/HttpConfig.h | 3 +- proxy/http/HttpSM.cc | 14 ++++ .../forward_proxy/forward_proxy.replay.yaml | 3 - .../forward_proxy/forward_proxy.test.py | 68 ++++++++++++++----- 7 files changed, 95 insertions(+), 20 deletions(-) diff --git a/doc/admin-guide/files/records.config.en.rst b/doc/admin-guide/files/records.config.en.rst index ed7d3b97d13..c3dccaca7a0 100644 --- a/doc/admin-guide/files/records.config.en.rst +++ b/doc/admin-guide/files/records.config.en.rst @@ -3983,6 +3983,29 @@ Client-Related Configuration if sni_policy = ``verify_with_name_source``, the sni will be the host header value and the name to check in the server certificate will be the remap header value. +.. ts:cv:: CONFIG proxy.config.ssl.client.scheme_proto_mismatch_policy INT 2 + :overridable: + + This option controls how |TS| behaves when the client side connection + protocol and the client request's scheme do not match. For example, if + enforcement is enabled by setting this value to ``2`` and the client + connection is a cleartext HTTP connection but the scheme of the URL is + ``https://``, then |TS| will emit a warning and return an immediate 400 HTTP + response without proxying the request to the origin. + + The default value is ``2``, meaning that |TS| will enforce that the protocol + matches the scheme. + + ===== ====================================================================== + Value Description + ===== ====================================================================== + ``0`` Disable verification that the protocol and scheme match. + ``1`` Check that the protocol and scheme match, but only emit a warning if + they do not. + ``2`` Check that the protocol and scheme match and, if they do not, emit a + warning and return an immediate HTTP 400 response. + ===== ====================================================================== + .. ts:cv:: CONFIG proxy.config.ssl.client.TLSv1 INT 0 Enables (``1``) or disables (``0``) TLSv1.0 in the ATS client context. If not specified, enabled by default diff --git a/mgmt/RecordsConfig.cc b/mgmt/RecordsConfig.cc index 05c71811ca7..8cb8825e7c1 100644 --- a/mgmt/RecordsConfig.cc +++ b/mgmt/RecordsConfig.cc @@ -1180,6 +1180,8 @@ static const RecordElement RecordsConfig[] = , {RECT_CONFIG, "proxy.config.ssl.client.sni_policy", RECD_STRING, "host", RECU_RESTART_TS, RR_NULL, RECC_NULL, nullptr, RECA_NULL} , + {RECT_CONFIG, "proxy.config.ssl.client.scheme_proto_mismatch_policy", RECD_INT, "2", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL} + , {RECT_CONFIG, "proxy.config.ssl.origin_session_cache", RECD_INT, "1", RECU_RESTART_TS, RR_NULL, RECC_NULL, nullptr, RECA_NULL} , {RECT_CONFIG, "proxy.config.ssl.origin_session_cache.size", RECD_INT, "10240", RECU_RESTART_TS, RR_NULL, RECC_NULL, nullptr, RECA_NULL} diff --git a/proxy/http/HttpConfig.cc b/proxy/http/HttpConfig.cc index 7fe46cef783..5b97d15aaac 100644 --- a/proxy/http/HttpConfig.cc +++ b/proxy/http/HttpConfig.cc @@ -1415,6 +1415,7 @@ HttpConfig::startup() HttpEstablishStaticConfigByte(c.http_host_sni_policy, "proxy.config.http.host_sni_policy"); HttpEstablishStaticConfigStringAlloc(c.oride.ssl_client_sni_policy, "proxy.config.ssl.client.sni_policy"); + HttpEstablishStaticConfigByte(c.scheme_proto_mismatch_policy, "proxy.config.ssl.client.scheme_proto_mismatch_policy"); OutboundConnTrack::config_init(&c.global_outbound_conntrack, &c.oride.outbound_conntrack); @@ -1706,6 +1707,7 @@ HttpConfig::reconfigure() params->redirect_actions_string = ats_strdup(m_master.redirect_actions_string); params->redirect_actions_map = parse_redirect_actions(params->redirect_actions_string, params->redirect_actions_self_action); params->http_host_sni_policy = m_master.http_host_sni_policy; + params->scheme_proto_mismatch_policy = m_master.scheme_proto_mismatch_policy; params->oride.ssl_client_sni_policy = ats_strdup(m_master.oride.ssl_client_sni_policy); diff --git a/proxy/http/HttpConfig.h b/proxy/http/HttpConfig.h index 9f8d3138b87..469b1c72051 100644 --- a/proxy/http/HttpConfig.h +++ b/proxy/http/HttpConfig.h @@ -859,7 +859,8 @@ struct HttpConfigParams : public ConfigInfo { MgmtInt http_request_line_max_size = 65535; MgmtInt http_hdr_field_max_size = 131070; - MgmtByte http_host_sni_policy = 0; + MgmtByte http_host_sni_policy = 0; + MgmtByte scheme_proto_mismatch_policy = 2; // noncopyable ///////////////////////////////////// diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc index baaabc4b5b3..7ffbf4cc606 100644 --- a/proxy/http/HttpSM.cc +++ b/proxy/http/HttpSM.cc @@ -853,6 +853,20 @@ HttpSM::state_read_client_request_header(int event, void *data) break; } + if (!is_internal && t_state.http_config_param->scheme_proto_mismatch_policy != 0) { + auto scheme = t_state.hdr_info.client_request.url_get()->scheme_get_wksidx(); + if ((client_connection_is_ssl && (scheme == URL_WKSIDX_HTTP || scheme == URL_WKSIDX_WS)) || + (!client_connection_is_ssl && (scheme == URL_WKSIDX_HTTPS || scheme == URL_WKSIDX_WSS))) { + Warning("scheme [%s] vs. protocol [%s] mismatch", hdrtoken_index_to_wks(scheme), + client_connection_is_ssl ? "tls" : "plaintext"); + if (t_state.http_config_param->scheme_proto_mismatch_policy == 2) { + t_state.http_return_code = HTTP_STATUS_BAD_REQUEST; + call_transact_and_set_next_state(HttpTransact::BadRequest); + break; + } + } + } + if (_from_early_data) { // Only allow early data for safe methods defined in RFC7231 Section 4.2.1. // https://tools.ietf.org/html/rfc7231#section-4.2.1 diff --git a/tests/gold_tests/forward_proxy/forward_proxy.replay.yaml b/tests/gold_tests/forward_proxy/forward_proxy.replay.yaml index d744fa38a2d..926f5c8add8 100644 --- a/tests/gold_tests/forward_proxy/forward_proxy.replay.yaml +++ b/tests/gold_tests/forward_proxy/forward_proxy.replay.yaml @@ -19,6 +19,3 @@ sessions: headers: fields: - [ Content-Length, 8 ] - - proxy-response: - status: 200 diff --git a/tests/gold_tests/forward_proxy/forward_proxy.test.py b/tests/gold_tests/forward_proxy/forward_proxy.test.py index 2cf63e2b726..d631b874c77 100644 --- a/tests/gold_tests/forward_proxy/forward_proxy.test.py +++ b/tests/gold_tests/forward_proxy/forward_proxy.test.py @@ -1,5 +1,5 @@ -''' -''' +""" +""" # Licensed to the Apache Software Foundation (ASF) under one # or more contributor license agreements. See the NOTICE file # distributed with this work for additional information @@ -16,25 +16,49 @@ # See the License for the specific language governing permissions and # limitations under the License. -import sys +from typing import Union Test.Summary = 'Verify ATS can function as a forward proxy' Test.ContinueOnFail = True class ForwardProxyTest: - def __init__(self): + _scheme_proto_mismatch_policy: Union[int, None] + _ts_counter: int = 0 + _server_counter: int = 0 + + def __init__(self, verify_scheme_matches_protocol: Union[int, None]): + """Construct a ForwardProxyTest object. + + :param verify_scheme_matches_protocol: The value with which to + configure Traffic Server's + proxy.config.ssl.client.scheme_proto_mismatch_policy. A value of None + means that no value will be explicitly set in the records.config. + :type verify_scheme_matches_protocol: int or None + """ + self._scheme_proto_mismatch_policy = verify_scheme_matches_protocol self.setupOriginServer() self.setupTS() def setupOriginServer(self): - self.server = Test.MakeVerifierServerProcess("server", "forward_proxy.replay.yaml") - self.server.Streams.All = Testers.ContainsExpression( - 'Received an HTTP/1 request with key 1', - 'Verify that the server received the request.') + """Configure the Proxy Verifier server.""" + proc_name = f"server{ForwardProxyTest._server_counter}" + self.server = Test.MakeVerifierServerProcess(proc_name, "forward_proxy.replay.yaml") + ForwardProxyTest._server_counter += 1 + if self._scheme_proto_mismatch_policy in (2, None): + self.server.Streams.All = Testers.ExcludesExpression( + 'Received an HTTP/1 request with key 1', + 'Verify that the server did not receive the request.') + else: + self.server.Streams.All = Testers.ContainsExpression( + 'Received an HTTP/1 request with key 1', + 'Verify that the server received the request.') def setupTS(self): - self.ts = Test.MakeATSProcess("ts", enable_tls=True, enable_cache=False) + """Configure the Traffic Server process.""" + proc_name = f"ts{ForwardProxyTest._ts_counter}" + self.ts = Test.MakeATSProcess(proc_name, enable_tls=True, enable_cache=False) + ForwardProxyTest._ts_counter += 1 self.ts.addDefaultSSLFiles() self.ts.Disk.ssl_multicert_config.AddLine("dest_ip=* ssl_cert_name=server.pem ssl_key_name=server.key") self.ts.Disk.remap_config.AddLine( @@ -50,10 +74,13 @@ def setupTS(self): 'proxy.config.diags.debug.tags': "http", }) + if self._scheme_proto_mismatch_policy is not None: + self.ts.Disk.records_config.update({ + 'proxy.config.ssl.client.scheme_proto_mismatch_policy': self._scheme_proto_mismatch_policy, + }) + def addProxyHttpsToHttpCase(self): - """ - Test ATS as an HTTPS forward proxy behind an HTTP server. - """ + """Test ATS as an HTTPS forward proxy behind an HTTP server.""" tr = Test.AddTestRun() tr.Processes.Default.StartBefore(self.server) tr.Processes.Default.StartBefore(self.ts) @@ -65,12 +92,21 @@ def addProxyHttpsToHttpCase(self): tr.StillRunningAfter = self.server tr.StillRunningAfter = self.ts - tr.Processes.Default.Streams.All = Testers.ContainsExpression( - '< HTTP/1.1 200 OK', - 'Verify that curl received a 200 OK response.') + if self._scheme_proto_mismatch_policy in (2, None): + tr.Processes.Default.Streams.All = Testers.ContainsExpression( + '< HTTP/1.1 400 Invalid HTTP Request', + 'Verify that the request was rejected.') + else: + tr.Processes.Default.Streams.All = Testers.ContainsExpression( + '< HTTP/1.1 200 OK', + 'Verify that curl received a 200 OK response.') def run(self): + """Configure the TestRun instances for this set of tests.""" self.addProxyHttpsToHttpCase() -ForwardProxyTest().run() +ForwardProxyTest(verify_scheme_matches_protocol=None).run() +ForwardProxyTest(verify_scheme_matches_protocol=0).run() +ForwardProxyTest(verify_scheme_matches_protocol=1).run() +ForwardProxyTest(verify_scheme_matches_protocol=2).run()