From c556c3c38ffc78060512f0083f72e38097b488fb 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 | 21 ++++ .../forward_proxy/forward_proxy.test.py | 112 ++++++++++++++++++ 7 files changed, 176 insertions(+), 1 deletion(-) create mode 100644 tests/gold_tests/forward_proxy/forward_proxy.replay.yaml create mode 100644 tests/gold_tests/forward_proxy/forward_proxy.test.py diff --git a/doc/admin-guide/files/records.config.en.rst b/doc/admin-guide/files/records.config.en.rst index 56aba60f10e..c9e0c7e2e13 100644 --- a/doc/admin-guide/files/records.config.en.rst +++ b/doc/admin-guide/files/records.config.en.rst @@ -3743,6 +3743,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 7f62fdce521..143b1448c2f 100644 --- a/mgmt/RecordsConfig.cc +++ b/mgmt/RecordsConfig.cc @@ -1138,6 +1138,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.session_cache", RECD_INT, "2", RECU_RESTART_TS, RR_NULL, RECC_NULL, nullptr, RECA_NULL} , {RECT_CONFIG, "proxy.config.ssl.session_cache.size", RECD_INT, "102400", RECU_RESTART_TS, RR_NULL, RECC_NULL, nullptr, RECA_NULL} diff --git a/proxy/http/HttpConfig.cc b/proxy/http/HttpConfig.cc index 439541ac452..ee311af3b7b 100644 --- a/proxy/http/HttpConfig.cc +++ b/proxy/http/HttpConfig.cc @@ -1363,6 +1363,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.outbound_conntrack, &c.oride.outbound_conntrack); @@ -1645,6 +1646,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 11ebef6d06c..14e3f3bc61c 100644 --- a/proxy/http/HttpConfig.h +++ b/proxy/http/HttpConfig.h @@ -830,7 +830,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 e67ceb5c35f..de0d2339229 100644 --- a/proxy/http/HttpSM.cc +++ b/proxy/http/HttpSM.cc @@ -863,6 +863,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 new file mode 100644 index 00000000000..926f5c8add8 --- /dev/null +++ b/tests/gold_tests/forward_proxy/forward_proxy.replay.yaml @@ -0,0 +1,21 @@ +meta: + version: "1.0" + +sessions: +- transactions: + - all: + client-request: + method: "GET" + url: "/a/path" + version: "1.1" + headers: + fields: + - [ Host, example.com ] + - [ uuid, 1 ] + + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, 8 ] diff --git a/tests/gold_tests/forward_proxy/forward_proxy.test.py b/tests/gold_tests/forward_proxy/forward_proxy.test.py new file mode 100644 index 00000000000..d631b874c77 --- /dev/null +++ b/tests/gold_tests/forward_proxy/forward_proxy.test.py @@ -0,0 +1,112 @@ +""" +""" +# 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 +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from typing import Union + +Test.Summary = 'Verify ATS can function as a forward proxy' +Test.ContinueOnFail = True + + +class ForwardProxyTest: + _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): + """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): + """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( + f"map / http://127.0.0.1:{self.server.Variables.http_port}/") + + self.ts.Disk.records_config.update({ + 'proxy.config.ssl.server.cert.path': self.ts.Variables.SSLDir, + 'proxy.config.ssl.server.private_key.path': self.ts.Variables.SSLDir, + 'proxy.config.ssl.client.verify.server.policy': 'PERMISSIVE', + 'proxy.config.ssl.keylog_file': '/tmp/keylog.txt', + + 'proxy.config.diags.debug.enabled': 1, + '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.""" + tr = Test.AddTestRun() + tr.Processes.Default.StartBefore(self.server) + tr.Processes.Default.StartBefore(self.ts) + tr.Processes.Default.Command = ( + f'curl --proxy-insecure -v -H "uuid: 1" ' + f'--proxy "https://127.0.0.1:{self.ts.Variables.ssl_port}/" ' + f'http://example.com/') + tr.Processes.Default.ReturnCode = 0 + tr.StillRunningAfter = self.server + tr.StillRunningAfter = self.ts + + 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(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()