From ac2e2df8d19f0622d20b2ec58730ab23c096824a Mon Sep 17 00:00:00 2001 From: Susan Hinrichs Date: Fri, 31 Mar 2023 17:32:21 +0000 Subject: [PATCH 1/5] Add allow-plain server ports attribute --- doc/admin-guide/files/records.yaml.en.rst | 7 ++ include/records/I_RecHttp.h | 3 + iocore/net/P_SSLNetVConnection.h | 15 +++ iocore/net/P_SSLNextProtocolAccept.h | 3 +- iocore/net/SSLNetVConnection.cc | 92 ++++++++++++++++-- iocore/net/SSLNextProtocolAccept.cc | 59 +++++++----- proxy/ProtocolProbeSessionAccept.cc | 10 +- proxy/http/HttpProxyServerMain.cc | 2 +- src/records/RecHttp.cc | 18 ++++ tests/gold_tests/tls/allow-plain.test.py | 94 +++++++++++++++++++ .../tls/replay/allow-plain.replay.yaml | 59 ++++++++++++ 11 files changed, 330 insertions(+), 32 deletions(-) create mode 100644 tests/gold_tests/tls/allow-plain.test.py create mode 100644 tests/gold_tests/tls/replay/allow-plain.replay.yaml diff --git a/doc/admin-guide/files/records.yaml.en.rst b/doc/admin-guide/files/records.yaml.en.rst index d48129f5e4c..060a65193a4 100644 --- a/doc/admin-guide/files/records.yaml.en.rst +++ b/doc/admin-guide/files/records.yaml.en.rst @@ -697,6 +697,7 @@ HTTP Engine tr-out Outbound transparent. tr-pass Pass through enabled. mptcp Multipath TCP. + allow-plain Allow failback to non-TLS for TLS ports =========== =============== ======================================== *number* @@ -721,6 +722,8 @@ ssl Not compatible with: ``blind`` and ``quic``. + ``allow-plain`` allows a failback to non SSL for such ports. + quic Require QUIC termination for inbound connections. SSL :ref:`must be configured ` for this option to provide a functional server port. **THIS IS EXPERIMENTAL SUPPORT AND NOT READY FOR PRODUCTION USE.** @@ -779,6 +782,10 @@ mptcp Requires custom Linux kernel available at https://multipath-tcp.org. +allow-plain + For TLS ports, will fall back to non-TLS processing if the TLS handshake fails. Incompatible with + quic ports. + .. topic:: Example Listen on port 80 on any address for IPv4 and IPv6.:: diff --git a/include/records/I_RecHttp.h b/include/records/I_RecHttp.h index b61133046f3..5662a34ec11 100644 --- a/include/records/I_RecHttp.h +++ b/include/records/I_RecHttp.h @@ -271,6 +271,8 @@ struct HttpProxyPort { bool m_outbound_transparent_p = false; // True if transparent pass-through is enabled on this port. bool m_transparent_passthrough = false; + // True if allow-plain is enabled on this port. + bool m_allow_plain = false; /// True if MPTCP is enabled on this port. bool m_mptcp = false; /// Local address for inbound connections (listen address). @@ -420,6 +422,7 @@ struct HttpProxyPort { static const char *const OPT_TRANSPARENT_OUTBOUND; ///< Outbound transparent. static const char *const OPT_TRANSPARENT_FULL; ///< Full transparency. static const char *const OPT_TRANSPARENT_PASSTHROUGH; ///< Pass-through non-HTTP. + static const char *const OPT_ALLOW_PLAIN; ///< Backup to plain HTTP. static const char *const OPT_SSL; ///< SSL (experimental) static const char *const OPT_QUIC; ///< QUIC (experimental) static const char *const OPT_PROXY_PROTO; ///< Proxy Protocol diff --git a/iocore/net/P_SSLNetVConnection.h b/iocore/net/P_SSLNetVConnection.h index 048accbac82..99b3b50370a 100644 --- a/iocore/net/P_SSLNetVConnection.h +++ b/iocore/net/P_SSLNetVConnection.h @@ -181,6 +181,18 @@ class SSLNetVConnection : public UnixNetVConnection, transparentPassThrough = val; } + bool + getAllowPlain() const + { + return allowPlain; + } + + void + setAllowPlain(bool val) + { + allowPlain = val; + } + // Copy up here so we overload but don't override using super::reenable; @@ -438,6 +450,7 @@ class SSLNetVConnection : public UnixNetVConnection, int handShakeBioStored = 0; bool transparentPassThrough = false; + bool allowPlain = false; int sent_cert = 0; @@ -484,6 +497,8 @@ class SSLNetVConnection : public UnixNetVConnection, void _make_ssl_connection(SSL_CTX *ctx); void _bindSSLObject(); void _unbindSSLObject(); + UnixNetVConnection *_migrateFromSSL(); + void _propagateHandShakeBuffer(UnixNetVConnection *target, EThread *t); int _ssl_read_from_net(EThread *lthread, int64_t &ret); ssl_error_t _ssl_read_buffer(void *buf, int64_t nbytes, int64_t &nread); diff --git a/iocore/net/P_SSLNextProtocolAccept.h b/iocore/net/P_SSLNextProtocolAccept.h index 18070cd6bc8..512960441d5 100644 --- a/iocore/net/P_SSLNextProtocolAccept.h +++ b/iocore/net/P_SSLNextProtocolAccept.h @@ -33,7 +33,7 @@ class SSLNextProtocolAccept : public SessionAccept { public: - SSLNextProtocolAccept(Continuation *, bool); + SSLNextProtocolAccept(Continuation *, bool, bool); ~SSLNextProtocolAccept() override; bool accept(NetVConnection *, MIOBuffer *, IOBufferReader *) override; @@ -60,6 +60,7 @@ class SSLNextProtocolAccept : public SessionAccept SSLNextProtocolSet protoset; SessionProtocolSet protoenabled; bool transparent_passthrough; + bool allow_plain; friend struct SSLNextProtocolTrampoline; }; diff --git a/iocore/net/SSLNetVConnection.cc b/iocore/net/SSLNetVConnection.cc index 7f0f74eaf06..1bbf4e2249b 100644 --- a/iocore/net/SSLNetVConnection.cc +++ b/iocore/net/SSLNetVConnection.cc @@ -1360,13 +1360,23 @@ SSLNetVConnection::sslServerHandShakeEvent(int &err) err = errno; SSLVCDebug(this, "SSL handshake error: %s (%d), errno=%d", SSLErrorName(ssl_error), ssl_error, err); - // start a blind tunnel if tr-pass is set and data does not look like ClientHello char *buf = handShakeBuffer ? handShakeBuffer->buf() : nullptr; - if (getTransparentPassThrough() && buf && *buf != SSL_OP_HANDSHAKE) { - SSLVCDebug(this, "Data does not look like SSL handshake, starting blind tunnel"); - this->attributes = HttpProxyPort::TRANSPORT_BLIND_TUNNEL; - sslHandshakeStatus = SSLHandshakeStatus::SSL_HANDSHAKE_ONGOING; - return EVENT_CONT; + if (buf && *buf != SSL_OP_HANDSHAKE) { + if (getAllowPlain()) { + SSLVCDebug(this, "Try plain"); + // If this doesn't look like a ClientHello, convert this connection to a UnixNetVC and send the + // packet for Http Processing + this->_migrateFromSSL(); + return EVENT_CONT; + } else if (getTransparentPassThrough() && buf && *buf != SSL_OP_HANDSHAKE) { + // start a blind tunnel if tr-pass is set and data does not look like ClientHello + SSLVCDebug(this, "Data does not look like SSL handshake, starting blind tunnel"); + this->attributes = HttpProxyPort::TRANSPORT_BLIND_TUNNEL; + sslHandshakeStatus = SSL_HANDSHAKE_ONGOING; + return EVENT_CONT; + } else { + SSLVCDebug(this, "Give up"); + } } } @@ -2141,6 +2151,76 @@ SSLNetVConnection::_getNetProcessor() return &sslNetProcessor; } +void +SSLNetVConnection::_propagateHandShakeBuffer(UnixNetVConnection *target, EThread *t) +{ + Debug("ssl", "allow-plain, handshake buffer ready to read=%" PRId64, this->handShakeHolder->read_avail()); + // Take ownership of the handShake buffer + this->sslHandshakeStatus = SSL_HANDSHAKE_DONE; + NetState *s = &target->read; + s->vio.buffer.writer_for(this->handShakeBuffer); + s->vio.set_reader(this->handShakeHolder); + this->handShakeHolder = nullptr; + this->handShakeBuffer = nullptr; + s->vio.vc_server = target; + s->vio.cont = this->read.vio.cont; + s->vio.mutex = this->read.vio.cont->mutex; + // Passing along the buffer, don't keep a reading holding early in the buffer + this->handShakeReader->dealloc(); + this->handShakeReader = nullptr; + + // Kick things again, so the data that was copied into the + // vio.read buffer gets processed + target->readSignalDone(VC_EVENT_READ_COMPLETE, get_NetHandler(t)); +} + +/* + * Replaces the current SSLNetVConnection with a UnixNetVConnection + * Propagates any data in the SSL handShakeBuffer to be processed + * by the UnixNetVConnection logic + */ +UnixNetVConnection * +SSLNetVConnection::_migrateFromSSL() +{ + EThread *t = this_ethread(); + NetHandler *client_nh = get_NetHandler(t); + ink_assert(client_nh); + + Connection hold_con; + hold_con.move(this->con); + + // We will leave the SSL object with the original SSLNetVC to be + // cleaned up. Only moving the socket and handShakeBuffer + // So no need to call _prepareMigration + + // Do_io_close will signal the VC to be freed on the original thread + // Since we moved the con context, the fd will not be closed + // Go ahead and remove the fd from the original thread's epoll structure, so it is not + // processed on two threads simultaneously + this->ep.stop(); + + // Create new VC: + UnixNetVConnection *newvc = static_cast(unix_netProcessor.allocate_vc(t)); + ink_assert(newvc != nullptr); + if (newvc != nullptr && newvc->populate(hold_con, this->read.vio.cont, nullptr) != EVENT_DONE) { + newvc->do_io_close(); + Debug("ssl", "Failed to populate unixvc for allow-plain"); + newvc = nullptr; + } + if (newvc != nullptr) { + newvc->attributes = HttpProxyPort::TRANSPORT_DEFAULT; + newvc->set_is_transparent(this->is_transparent); + newvc->set_context(get_context()); + newvc->options = this->options; + Debug("ssl", "Move to unixvc for allow-plain"); + this->_propagateHandShakeBuffer(newvc, t); + } + + // Do not mark this closed until the end so it does not get freed by the other thread too soon + this->do_io_close(); + return newvc; +} + ssl_curve_id SSLNetVConnection::_get_tls_curve() const { diff --git a/iocore/net/SSLNextProtocolAccept.cc b/iocore/net/SSLNextProtocolAccept.cc index fcec559086f..192e77975cb 100644 --- a/iocore/net/SSLNextProtocolAccept.cc +++ b/iocore/net/SSLNextProtocolAccept.cc @@ -77,14 +77,20 @@ struct SSLNextProtocolTrampoline : public Continuation { vio = static_cast(edata); netvc = dynamic_cast(vio->vc_server); - ink_assert(netvc != nullptr); switch (event) { case VC_EVENT_EOS: case VC_EVENT_ERROR: case VC_EVENT_ACTIVE_TIMEOUT: case VC_EVENT_INACTIVITY_TIMEOUT: - netvc->do_io_close(); + if (netvc != nullptr) { + netvc->do_io_close(); + } else { // Try making a unix netvc + UnixNetVConnection *vc = dynamic_cast(vio->vc_server); + if (vc != nullptr) { + vc->do_io_close(); + } + } delete this; return EVENT_ERROR; case VC_EVENT_READ_COMPLETE: @@ -93,25 +99,32 @@ struct SSLNextProtocolTrampoline : public Continuation { return EVENT_ERROR; } - // Cancel the action, so later timeouts and errors don't try to - // send the event to the Accept object. After this point, the accept - // object does not care. - netvc->set_action(nullptr); - - Continuation *endpoint_cont = netvc->endpoint(); - if (!endpoint_cont) { - // Route to the default endpoint - endpoint_cont = npnParent->endpoint; - } - - if (endpoint_cont) { - // disable read io, send events to endpoint - netvc->do_io_read(endpoint_cont, 0, nullptr); - - send_plugin_event(endpoint_cont, NET_EVENT_ACCEPT, netvc); + // This wasn't really a TLS connection + // Trying to process it as a TCP connection + if (netvc == nullptr) { + UnixNetVConnection *plain_netvc = dynamic_cast(vio->vc_server); + send_plugin_event(npnParent->endpoint, NET_EVENT_ACCEPT, plain_netvc); } else { - // No handler, what should we do? Best to just kill the VC while we can. - netvc->do_io_close(); + // Cancel the action, so later timeouts and errors don't try to + // send the event to the Accept object. After this point, the accept + // object does not care. + netvc->set_action(nullptr); + + Continuation *endpoint_cont = netvc->endpoint(); + if (!endpoint_cont) { + // Route to the default endpoint + endpoint_cont = npnParent->endpoint; + } + + if (endpoint_cont) { + // disable read io, send events to endpoint + netvc->do_io_read(endpoint_cont, 0, nullptr); + + send_plugin_event(endpoint_cont, NET_EVENT_ACCEPT, netvc); + } else { + // No handler, what should we do? Best to just kill the VC while we can. + netvc->do_io_close(); + } } delete this; @@ -132,6 +145,7 @@ SSLNextProtocolAccept::mainEvent(int event, void *edata) ink_release_assert(netvc != nullptr); netvc->setTransparentPassThrough(transparent_passthrough); + netvc->setAllowPlain(allow_plain); // Register our protocol set with the VC and kick off a zero-length read to // force the SSLNetVConnection to complete the SSL handshake. Don't tell @@ -167,11 +181,12 @@ SSLNextProtocolAccept::enableProtocols(const SessionProtocolSet &protos) this->protoenabled = protos; } -SSLNextProtocolAccept::SSLNextProtocolAccept(Continuation *ep, bool transparent_passthrough) +SSLNextProtocolAccept::SSLNextProtocolAccept(Continuation *ep, bool transparent_passthrough, bool allow_plain) : SessionAccept(nullptr), buffer(new_empty_MIOBuffer(SSLConfigParams::ssl_misc_max_iobuffer_size_index)), endpoint(ep), - transparent_passthrough(transparent_passthrough) + transparent_passthrough(transparent_passthrough), + allow_plain(allow_plain) { SET_HANDLER(&SSLNextProtocolAccept::mainEvent); } diff --git a/proxy/ProtocolProbeSessionAccept.cc b/proxy/ProtocolProbeSessionAccept.cc index f9351b62f37..327ae7e26d3 100644 --- a/proxy/ProtocolProbeSessionAccept.cc +++ b/proxy/ProtocolProbeSessionAccept.cc @@ -170,8 +170,14 @@ ProtocolProbeSessionAccept::mainEvent(int event, void *data) ink_assert(data); VIO *vio; - NetVConnection *netvc = static_cast(data); - ProtocolProbeTrampoline *probe = new ProtocolProbeTrampoline(this, netvc->mutex, nullptr, nullptr); + NetVConnection *netvc = static_cast(data); + ProtocolProbeTrampoline *probe; + UnixNetVConnection *unix_netvc = dynamic_cast(netvc); + if (unix_netvc != nullptr && unix_netvc->read.vio.get_writer() != nullptr) { + probe = new ProtocolProbeTrampoline(this, netvc->mutex, unix_netvc->read.vio.get_writer(), unix_netvc->read.vio.get_reader()); + } else { + probe = new ProtocolProbeTrampoline(this, netvc->mutex, nullptr, nullptr); + } // The connection has completed, set the accept inactivity timeout here to watch over the difference between the // connection set up and the first transaction.. diff --git a/proxy/http/HttpProxyServerMain.cc b/proxy/http/HttpProxyServerMain.cc index 21ba1d9806e..e85ff1233c5 100644 --- a/proxy/http/HttpProxyServerMain.cc +++ b/proxy/http/HttpProxyServerMain.cc @@ -206,7 +206,7 @@ MakeHttpProxyAcceptor(HttpProxyAcceptor &acceptor, HttpProxyPort &port, unsigned ProtocolSessionCreateMap.insert({TS_ALPN_PROTOCOL_INDEX_HTTP_2_0, create_h2_server_session}); if (port.isSSL()) { - SSLNextProtocolAccept *ssl = new SSLNextProtocolAccept(probe, port.m_transparent_passthrough); + SSLNextProtocolAccept *ssl = new SSLNextProtocolAccept(probe, port.m_transparent_passthrough, port.m_allow_plain); // ALPN selects the first server-offered protocol, // so make sure that we offer the newest protocol first. diff --git a/src/records/RecHttp.cc b/src/records/RecHttp.cc index 4a95a44a084..55d376dc6a4 100644 --- a/src/records/RecHttp.cc +++ b/src/records/RecHttp.cc @@ -191,6 +191,7 @@ const char *const HttpProxyPort::OPT_TRANSPARENT_INBOUND = "tr-in"; const char *const HttpProxyPort::OPT_TRANSPARENT_OUTBOUND = "tr-out"; const char *const HttpProxyPort::OPT_TRANSPARENT_FULL = "tr-full"; const char *const HttpProxyPort::OPT_TRANSPARENT_PASSTHROUGH = "tr-pass"; +const char *const HttpProxyPort::OPT_ALLOW_PLAIN = "allow-plain"; const char *const HttpProxyPort::OPT_SSL = "ssl"; const char *const HttpProxyPort::OPT_PROXY_PROTO = "pp"; const char *const HttpProxyPort::OPT_PLUGIN = "plugin"; @@ -450,6 +451,8 @@ HttpProxyPort::processOptions(const char *opts) #else Warning("Transparent pass-through requested [%s] in port descriptor '%s' but TPROXY was not configured.", item, opts); #endif + } else if (0 == strcasecmp(OPT_ALLOW_PLAIN, item)) { + m_allow_plain = true; } else if (0 == strcasecmp(OPT_MPTCP, item)) { if (mptcp_supported()) { m_mptcp = true; @@ -500,6 +503,17 @@ HttpProxyPort::processOptions(const char *opts) m_transparent_passthrough = false; } + // Make sure QUIC is not enabled with incompatible options + if (this->isQUIC()) { + if (this->m_allow_plain) { + Warning("allow_plain incompatible with QUIC"); + zret = false; + } else if (this->m_inbound_transparent_p || this->m_outbound_transparent_p) { + Warning("transparent mode not supported with QUIC"); + zret = false; + } + } + // Set the default session protocols. if (!sp_set_p) { if (this->isSSL()) { @@ -655,6 +669,10 @@ HttpProxyPort::print(char *out, size_t n) zret += snprintf(out + zret, n - zret, ":%s", OPT_TRANSPARENT_PASSTHROUGH); } + if (m_allow_plain) { + zret += snprintf(out + zret, n - zret, ":%s", OPT_ALLOW_PLAIN); + } + /* Don't print the IP resolution preferences if the port is outbound * transparent (which means the preference order is forced) or if * the order is the same as the default. diff --git a/tests/gold_tests/tls/allow-plain.test.py b/tests/gold_tests/tls/allow-plain.test.py new file mode 100644 index 00000000000..1ef4d128750 --- /dev/null +++ b/tests/gold_tests/tls/allow-plain.test.py @@ -0,0 +1,94 @@ +''' +Test the allow-plain attribute of the ssl port +Clients sending non-tls request to ssl port should get passed to +non-tls processing in ATS if the allow-plain attibute is present +''' +# 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. + +import os + +Test.Summary = ''' +Test allow-plain attributed +''' + +Test.ContinueOnFail = True + +# Define default ATS +ts = Test.MakeATSProcess("ts", enable_tls=True) + +# ---- +# Setup Origin Server +# ---- +replay_file = "replay/allow-plain.replay.yaml" +server = Test.MakeVerifierServerProcess("server", replay_file) + +ts.addDefaultSSLFiles() + +ts.Disk.records_config.update({ + 'proxy.config.http.server_ports': '{0}:ssl:allow-plain'.format(ts.Variables.ssl_port), + 'proxy.config.ssl.server.cert.path': '{0}'.format(ts.Variables.SSLDir), + 'proxy.config.ssl.server.private_key.path': '{0}'.format(ts.Variables.SSLDir), + 'proxy.config.diags.debug.enabled': 0, + 'proxy.config.diags.debug.tags': 'ssl|http', +}) + +ts.Disk.remap_config.AddLines([ + 'map / http://127.0.0.1:{0}'.format(server.Variables.http_port), + 'map /post http://127.0.0.1:{0}/post'.format(server.Variables.http_port) +]) + +ts.Disk.ssl_multicert_config.AddLine( + 'dest_ip=* ssl_cert_name=server.pem ssl_key_name=server.key' +) + +big_post_body = "0123456789" * 50000 +big_post_body_file = open(os.path.join(Test.RunDirectory, "big_post_body"), "w") +big_post_body_file.write(big_post_body) +big_post_body_file.close() + +# TLS curl should work of course +tr = Test.AddTestRun() +# Wait for the micro server +tr.Processes.Default.StartBefore(server, ready=When.PortOpen(server.Variables.http_port)) +# Delay on readiness of our ssl ports +tr.Processes.Default.StartBefore(Test.Processes.ts) + +tr.Processes.Default.Command = 'curl -o /dev/null -k --verbose -H "uuid: get" --ipv4 --http1.1 --resolve www.example.com:{}:127.0.0.1 https://www.example.com:{}/'.format( + ts.Variables.ssl_port, ts.Variables.ssl_port) +tr.Processes.Default.ReturnCode = 0 +tr.StillRunningAfter = server +tr.StillRunningAfter = ts +tr.Processes.Default.Streams.all = Testers.ContainsExpression("TLS", "Should negiotiate TLS") + +# non-TLS curl should also work to the same port +tr2 = Test.AddTestRun() +tr2.Processes.Default.Command = 'curl --verbose --ipv4 --http1.1 -H "uuid: get" --resolve www.example.com:{}:127.0.0.1 http://www.example.com:{}'.format( + ts.Variables.ssl_port, ts.Variables.ssl_port) +tr2.Processes.Default.ReturnCode = 0 +tr2.StillRunningAfter = server +tr2.StillRunningAfter = ts +tr2.Processes.Default.Streams.all = Testers.ExcludesExpression("TLS", "Should not negiotiate TLS") + +# Make sure a post > 32K works. Early version forgot to free a reader which caused a stall once the initial buffer filled +# Seems like we needed to make a second resquest to trigger the issue +tr3 = Test.AddTestRun() +tr3.Processes.Default.Command = 'curl --verbose -d @big_post_body -H "uuid: post" --ipv4 --http1.1 --resolve www.example.com:{}:127.0.0.1 http://www.example.com:{}/post http://www.example.com:{}/post'.format( + ts.Variables.ssl_port, ts.Variables.ssl_port, ts.Variables.ssl_port) +tr3.Processes.Default.ReturnCode = 0 +tr3.StillRunningAfter = server +tr3.StillRunningAfter = ts +tr3.Processes.Default.Streams.all = Testers.ExcludesExpression("TLS", "Should not negiotiate TLS") diff --git a/tests/gold_tests/tls/replay/allow-plain.replay.yaml b/tests/gold_tests/tls/replay/allow-plain.replay.yaml new file mode 100644 index 00000000000..4d0f50add9b --- /dev/null +++ b/tests/gold_tests/tls/replay/allow-plain.replay.yaml @@ -0,0 +1,59 @@ +# 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. + +# +# This replay file assumes that caching is enabled and +# proxy.config.http.cache.ignore_server_no_cache is set to 1(meaning the +# cache-control directives in responses to bypass the cache is ignored) +meta: + version: "1.0" + +sessions: + - transactions: + # The client is actually curl + - client-request: + method: "POST" + version: "1.1" + headers: + fields: + - [uuid, post] + - [Expect, 100-continue] + + proxy-request: + method: "POST" + + server-response: + status: 200 + reason: OK + headers: + fields: + - [Content-Length, 4] + - client-request: + method: "GET" + version: "1.1" + headers: + fields: + - [uuid, get] + + proxy-request: + method: "GET" + + server-response: + status: 200 + reason: OK + headers: + fields: + - [Content-Length, 4] From cca5a99ded229a87d69927c71839d920b8f876c7 Mon Sep 17 00:00:00 2001 From: Susan Hinrichs Date: Mon, 11 Sep 2023 19:35:44 +0000 Subject: [PATCH 2/5] Fix use after free issue --- iocore/net/P_UnixNetVConnection.h | 3 +++ iocore/net/SSLNetVConnection.cc | 18 +++++++++++++++--- iocore/net/UnixNetVConnection.cc | 21 +++++++++++++-------- 3 files changed, 31 insertions(+), 11 deletions(-) diff --git a/iocore/net/P_UnixNetVConnection.h b/iocore/net/P_UnixNetVConnection.h index f453d014944..63d7b3f6bb4 100644 --- a/iocore/net/P_UnixNetVConnection.h +++ b/iocore/net/P_UnixNetVConnection.h @@ -115,6 +115,9 @@ class UnixNetVConnection : public NetVConnection, public NetEvent void get_local_sa(); + // decrement recursion and see if vc should be freed + bool test_inline_close(); + // these are not part of the pure virtual interface. They were // added to reduce the amount of duplicate code in classes inherited // from NetVConnection (SSL). diff --git a/iocore/net/SSLNetVConnection.cc b/iocore/net/SSLNetVConnection.cc index 1bbf4e2249b..96db7ff514a 100644 --- a/iocore/net/SSLNetVConnection.cc +++ b/iocore/net/SSLNetVConnection.cc @@ -603,11 +603,20 @@ SSLNetVConnection::net_read_io(NetHandler *nh, EThread *lthread) if (!getSSLHandShakeComplete()) { int err = 0; + // May get into logic that will clean up the current VC + // Increment the recursion to delay do_io_close cleaup. + this->recursion++; + if (get_context() == NET_VCONNECTION_OUT) { ret = sslStartHandShake(SSL_EVENT_CLIENT, err); } else { ret = sslStartHandShake(SSL_EVENT_SERVER, err); } + if (ret == EVENT_RESTART) { + // VC migrated into a new object + // Just give up and go home. Events should trigger on the new vc + return; + } // If we have flipped to blind tunnel, don't read ahead if (this->handShakeReader) { if (this->attributes == HttpProxyPort::TRANSPORT_BLIND_TUNNEL) { @@ -642,6 +651,7 @@ SSLNetVConnection::net_read_io(NetHandler *nh, EThread *lthread) this->readSignalDone(VC_EVENT_READ_COMPLETE, nh); } } + test_inline_close(); return; // Leave if we are tunneling } } @@ -658,6 +668,7 @@ SSLNetVConnection::net_read_io(NetHandler *nh, EThread *lthread) read.triggered = 0; nh->read_ready_list.remove(this); readSignalError(nh, ETIMEDOUT); + test_inline_close(); return; } } @@ -693,6 +704,7 @@ SSLNetVConnection::net_read_io(NetHandler *nh, EThread *lthread) } else { readReschedule(nh); } + test_inline_close(); return; } @@ -1367,12 +1379,12 @@ SSLNetVConnection::sslServerHandShakeEvent(int &err) // If this doesn't look like a ClientHello, convert this connection to a UnixNetVC and send the // packet for Http Processing this->_migrateFromSSL(); - return EVENT_CONT; + return EVENT_RESTART; } else if (getTransparentPassThrough() && buf && *buf != SSL_OP_HANDSHAKE) { // start a blind tunnel if tr-pass is set and data does not look like ClientHello SSLVCDebug(this, "Data does not look like SSL handshake, starting blind tunnel"); this->attributes = HttpProxyPort::TRANSPORT_BLIND_TUNNEL; - sslHandshakeStatus = SSL_HANDSHAKE_ONGOING; + sslHandshakeStatus = SSLHandshakeStatus::SSL_HANDSHAKE_ONGOING; return EVENT_CONT; } else { SSLVCDebug(this, "Give up"); @@ -2156,7 +2168,7 @@ SSLNetVConnection::_propagateHandShakeBuffer(UnixNetVConnection *target, EThread { Debug("ssl", "allow-plain, handshake buffer ready to read=%" PRId64, this->handShakeHolder->read_avail()); // Take ownership of the handShake buffer - this->sslHandshakeStatus = SSL_HANDSHAKE_DONE; + this->sslHandshakeStatus = SSLHandshakeStatus::SSL_HANDSHAKE_DONE; NetState *s = &target->read; s->vio.buffer.writer_for(this->handShakeBuffer); s->vio.set_reader(this->handShakeHolder); diff --git a/iocore/net/UnixNetVConnection.cc b/iocore/net/UnixNetVConnection.cc index ee7fe8a93f2..03bc6e90f46 100644 --- a/iocore/net/UnixNetVConnection.cc +++ b/iocore/net/UnixNetVConnection.cc @@ -98,10 +98,7 @@ read_signal_and_update(int event, UnixNetVConnection *vc) break; } } - if (!--vc->recursion && vc->closed) { - /* BZ 31932 */ - ink_assert(vc->thread == this_ethread()); - vc->nh->free_netevent(vc); + if (vc->test_inline_close()) { return EVENT_DONE; } else { return EVENT_CONT; @@ -132,10 +129,7 @@ write_signal_and_update(int event, UnixNetVConnection *vc) break; } } - if (!--vc->recursion && vc->closed) { - /* BZ 31932 */ - ink_assert(vc->thread == this_ethread()); - vc->nh->free_netevent(vc); + if (vc->test_inline_close()) { return EVENT_DONE; } else { return EVENT_CONT; @@ -1495,3 +1489,14 @@ UnixNetVConnection::set_tcp_congestion_control(int side) return -1; #endif } + +bool +UnixNetVConnection::test_inline_close() +{ + if (!--this->recursion && this->closed) { + ink_assert(this->thread == this_ethread()); + this->nh->free_netevent(this); + return true; + } + return false; +} From 467112f1aa89e2db31a7faefbf1da93bb1d6b549 Mon Sep 17 00:00:00 2001 From: Susan Hinrichs Date: Fri, 15 Sep 2023 16:46:36 +0000 Subject: [PATCH 3/5] Back out the recursion tracking memory solution --- iocore/net/P_UnixNetVConnection.h | 3 --- iocore/net/SSLNetVConnection.cc | 7 ------- iocore/net/UnixNetVConnection.cc | 21 ++++++++------------- 3 files changed, 8 insertions(+), 23 deletions(-) diff --git a/iocore/net/P_UnixNetVConnection.h b/iocore/net/P_UnixNetVConnection.h index 63d7b3f6bb4..f453d014944 100644 --- a/iocore/net/P_UnixNetVConnection.h +++ b/iocore/net/P_UnixNetVConnection.h @@ -115,9 +115,6 @@ class UnixNetVConnection : public NetVConnection, public NetEvent void get_local_sa(); - // decrement recursion and see if vc should be freed - bool test_inline_close(); - // these are not part of the pure virtual interface. They were // added to reduce the amount of duplicate code in classes inherited // from NetVConnection (SSL). diff --git a/iocore/net/SSLNetVConnection.cc b/iocore/net/SSLNetVConnection.cc index 96db7ff514a..010b060cce0 100644 --- a/iocore/net/SSLNetVConnection.cc +++ b/iocore/net/SSLNetVConnection.cc @@ -603,10 +603,6 @@ SSLNetVConnection::net_read_io(NetHandler *nh, EThread *lthread) if (!getSSLHandShakeComplete()) { int err = 0; - // May get into logic that will clean up the current VC - // Increment the recursion to delay do_io_close cleaup. - this->recursion++; - if (get_context() == NET_VCONNECTION_OUT) { ret = sslStartHandShake(SSL_EVENT_CLIENT, err); } else { @@ -651,7 +647,6 @@ SSLNetVConnection::net_read_io(NetHandler *nh, EThread *lthread) this->readSignalDone(VC_EVENT_READ_COMPLETE, nh); } } - test_inline_close(); return; // Leave if we are tunneling } } @@ -668,7 +663,6 @@ SSLNetVConnection::net_read_io(NetHandler *nh, EThread *lthread) read.triggered = 0; nh->read_ready_list.remove(this); readSignalError(nh, ETIMEDOUT); - test_inline_close(); return; } } @@ -704,7 +698,6 @@ SSLNetVConnection::net_read_io(NetHandler *nh, EThread *lthread) } else { readReschedule(nh); } - test_inline_close(); return; } diff --git a/iocore/net/UnixNetVConnection.cc b/iocore/net/UnixNetVConnection.cc index 03bc6e90f46..ee7fe8a93f2 100644 --- a/iocore/net/UnixNetVConnection.cc +++ b/iocore/net/UnixNetVConnection.cc @@ -98,7 +98,10 @@ read_signal_and_update(int event, UnixNetVConnection *vc) break; } } - if (vc->test_inline_close()) { + if (!--vc->recursion && vc->closed) { + /* BZ 31932 */ + ink_assert(vc->thread == this_ethread()); + vc->nh->free_netevent(vc); return EVENT_DONE; } else { return EVENT_CONT; @@ -129,7 +132,10 @@ write_signal_and_update(int event, UnixNetVConnection *vc) break; } } - if (vc->test_inline_close()) { + if (!--vc->recursion && vc->closed) { + /* BZ 31932 */ + ink_assert(vc->thread == this_ethread()); + vc->nh->free_netevent(vc); return EVENT_DONE; } else { return EVENT_CONT; @@ -1489,14 +1495,3 @@ UnixNetVConnection::set_tcp_congestion_control(int side) return -1; #endif } - -bool -UnixNetVConnection::test_inline_close() -{ - if (!--this->recursion && this->closed) { - ink_assert(this->thread == this_ethread()); - this->nh->free_netevent(this); - return true; - } - return false; -} From 3de3260bba2822aaddd17189d50f82481d9fcd55 Mon Sep 17 00:00:00 2001 From: Susan Hinrichs Date: Fri, 15 Sep 2023 21:43:56 +0000 Subject: [PATCH 4/5] Fix conflicting contants --- iocore/net/SSLNetVConnection.cc | 9 ++++++--- tests/gold_tests/tls/allow-plain.test.py | 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/iocore/net/SSLNetVConnection.cc b/iocore/net/SSLNetVConnection.cc index 010b060cce0..bc1b0a32513 100644 --- a/iocore/net/SSLNetVConnection.cc +++ b/iocore/net/SSLNetVConnection.cc @@ -71,6 +71,7 @@ using namespace std::literals; #define SSL_WRITE_WOULD_BLOCK 10 #define SSL_WAIT_FOR_HOOK 11 #define SSL_WAIT_FOR_ASYNC 12 +#define SSL_RESTART 13 ClassAllocator sslNetVCAllocator("sslNetVCAllocator"); @@ -608,9 +609,10 @@ SSLNetVConnection::net_read_io(NetHandler *nh, EThread *lthread) } else { ret = sslStartHandShake(SSL_EVENT_SERVER, err); } - if (ret == EVENT_RESTART) { + if (ret == SSL_RESTART) { // VC migrated into a new object // Just give up and go home. Events should trigger on the new vc + Dbg(dbg_ctl_ssl, "Restart for allow plain"); return; } // If we have flipped to blind tunnel, don't read ahead @@ -1367,13 +1369,14 @@ SSLNetVConnection::sslServerHandShakeEvent(int &err) char *buf = handShakeBuffer ? handShakeBuffer->buf() : nullptr; if (buf && *buf != SSL_OP_HANDSHAKE) { + SSLVCDebug(this, "SSL hanshake error with bad HS buffer"); if (getAllowPlain()) { SSLVCDebug(this, "Try plain"); // If this doesn't look like a ClientHello, convert this connection to a UnixNetVC and send the // packet for Http Processing this->_migrateFromSSL(); - return EVENT_RESTART; - } else if (getTransparentPassThrough() && buf && *buf != SSL_OP_HANDSHAKE) { + return SSL_RESTART; + } else if (getTransparentPassThrough()) { // start a blind tunnel if tr-pass is set and data does not look like ClientHello SSLVCDebug(this, "Data does not look like SSL handshake, starting blind tunnel"); this->attributes = HttpProxyPort::TRANSPORT_BLIND_TUNNEL; diff --git a/tests/gold_tests/tls/allow-plain.test.py b/tests/gold_tests/tls/allow-plain.test.py index 1ef4d128750..5273438fd18 100644 --- a/tests/gold_tests/tls/allow-plain.test.py +++ b/tests/gold_tests/tls/allow-plain.test.py @@ -25,7 +25,7 @@ Test allow-plain attributed ''' -Test.ContinueOnFail = True +Test.ContinueOnFail = False # Define default ATS ts = Test.MakeATSProcess("ts", enable_tls=True) From 1be584aa52f849fa473671d857a64f4033ef6da3 Mon Sep 17 00:00:00 2001 From: Susan Hinrichs Date: Mon, 18 Sep 2023 17:19:34 +0000 Subject: [PATCH 5/5] Address Maskit's comments --- iocore/net/SSLNextProtocolAccept.cc | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/iocore/net/SSLNextProtocolAccept.cc b/iocore/net/SSLNextProtocolAccept.cc index 192e77975cb..624dedecd99 100644 --- a/iocore/net/SSLNextProtocolAccept.cc +++ b/iocore/net/SSLNextProtocolAccept.cc @@ -75,21 +75,15 @@ struct SSLNextProtocolTrampoline : public Continuation { SSLNetVConnection *netvc; - vio = static_cast(edata); - netvc = dynamic_cast(vio->vc_server); + vio = static_cast(edata); switch (event) { case VC_EVENT_EOS: case VC_EVENT_ERROR: case VC_EVENT_ACTIVE_TIMEOUT: case VC_EVENT_INACTIVITY_TIMEOUT: - if (netvc != nullptr) { - netvc->do_io_close(); - } else { // Try making a unix netvc - UnixNetVConnection *vc = dynamic_cast(vio->vc_server); - if (vc != nullptr) { - vc->do_io_close(); - } + if (vio->vc_server != nullptr) { + vio->vc_server->do_io_close(); } delete this; return EVENT_ERROR; @@ -101,9 +95,9 @@ struct SSLNextProtocolTrampoline : public Continuation { // This wasn't really a TLS connection // Trying to process it as a TCP connection + netvc = dynamic_cast(vio->vc_server); if (netvc == nullptr) { - UnixNetVConnection *plain_netvc = dynamic_cast(vio->vc_server); - send_plugin_event(npnParent->endpoint, NET_EVENT_ACCEPT, plain_netvc); + send_plugin_event(npnParent->endpoint, NET_EVENT_ACCEPT, vio->vc_server); } else { // Cancel the action, so later timeouts and errors don't try to // send the event to the Accept object. After this point, the accept