diff --git a/plugins/experimental/rate_limit/limiter.h b/plugins/experimental/rate_limit/limiter.h index 6696274fafc..038c295faf1 100644 --- a/plugins/experimental/rate_limit/limiter.h +++ b/plugins/experimental/rate_limit/limiter.h @@ -327,6 +327,30 @@ template class RateLimiter return item; } + // Remove a still-queued element (e.g. a connection that closed before it was resumed). + // Returns true if it was found in the queue, so the caller can tell a queued element + // (which never reserved a slot) from one that was already resumed. + // + // Linear in the queue depth, and only reached when an element closes while queued, which + // requires the limiter to be at its limit. The depth is bounded by the configured queue size; + // note that a "queue" without a "size" leaves _max_queue at UINT32_MAX, in which case the only + // bound is proxy.config.net.connections_throttle. + bool + remove(const T &elem) + { + std::lock_guard lock(_queue_lock); + + for (auto it = _queue.begin(); it != _queue.end(); ++it) { + if (std::get<0>(*it) == elem) { + _queue.erase(it); + --_size; + return true; + } + } + + return false; + } + void incrementMetric(uint metric) { diff --git a/plugins/experimental/rate_limit/sni_limiter.cc b/plugins/experimental/rate_limit/sni_limiter.cc index 67241ad1784..36bca010045 100644 --- a/plugins/experimental/rate_limit/sni_limiter.cc +++ b/plugins/experimental/rate_limit/sni_limiter.cc @@ -171,7 +171,12 @@ sni_limit_cont(TSCont contp, TSEvent event, void *edata) if (limiter) { TSUserArgSet(vc, gVCIdx, nullptr); - limiter->free(); + // A connection that is still queued never reserved a slot, so only release one if it + // is not in the queue (either it reserved at CLIENT_HELLO or the sweep resumed it into + // a reserved slot). Dropping it from the queue also avoids a stale entry. + if (!limiter->remove(vc)) { + limiter->free(); + } limiter->selector()->release(); // Release the selector, such that it can be deleted later } TSVConnReenable(vc); diff --git a/plugins/experimental/rate_limit/sni_selector.cc b/plugins/experimental/rate_limit/sni_selector.cc index c1c2eec7ea7..5d992687aa0 100644 --- a/plugins/experimental/rate_limit/sni_selector.cc +++ b/plugins/experimental/rate_limit/sni_selector.cc @@ -219,9 +219,17 @@ sni_queue_cont(TSCont cont, TSEvent /* event ATS_UNUSED */, void * /* edata ATS_ QueueTime now = std::chrono::system_clock::now(); // Only do this once per limiter if (owner) { // Don't operate on the aliases - // Try to enable some queued VCs (if any) if there are slots available + // Try to enable some queued VCs (if any) if there are slots available. Reserving before + // dequeuing means a resumed VC owns the slot it was granted, so its VCONN_CLOSE releases + // exactly that slot. while (limiter->size() > 0 && limiter->reserve() == ReserveStatus::RESERVED) { - auto [vc, contp, start_time] = limiter->pop(); + auto [vc, contp, start_time] = limiter->pop(); + + if (nullptr == vc) { // A concurrent close emptied the queue; give the slot back + limiter->free(); + break; + } + std::chrono::milliseconds delay = std::chrono::duration_cast(now - start_time); (void)contp; // Ugly, but silences some compilers. @@ -236,11 +244,18 @@ sni_queue_cont(TSCont cont, TSEvent /* event ATS_UNUSED */, void * /* edata ATS_ while (limiter->size() > 0 && limiter->hasOldEntity(now)) { // The oldest object on the queue is too old on the queue, so "kill" it. - auto [vc, contp, start_time] = limiter->pop(); + auto [vc, contp, start_time] = limiter->pop(); + + if (nullptr == vc) { // A concurrent close emptied the queue + break; + } + std::chrono::milliseconds age = std::chrono::duration_cast(now - start_time); (void)contp; Dbg(dbg_ctl, "Queued VC is too old (%ldms), erroring out", static_cast(age.count())); + // This VC never reserved a slot; detach it (clear the arg and release the selector + // lease) so its VCONN_CLOSE does not release a slot it never held. TSUserArgSet(vc, gVCIdx, nullptr); limiter->selector()->release(); TSVConnReenableEx(vc, TS_EVENT_ERROR); diff --git a/src/iocore/net/TLSEventSupport.cc b/src/iocore/net/TLSEventSupport.cc index c1009d8c6ca..a44d5c71b6c 100644 --- a/src/iocore/net/TLSEventSupport.cc +++ b/src/iocore/net/TLSEventSupport.cc @@ -153,10 +153,20 @@ TLSEventSupport::callHooks(TSEvent eventId) Dbg(dbg_ctl_ssl, "sslHandshakeHookState=%s eventID=%d", get_ssl_handshake_hook_state_name(this->sslHandshakeHookState), eventId); // Move state if it is appropriate - if (eventId == TS_EVENT_VCONN_CLOSE) { + if (eventId == TS_EVENT_VCONN_CLOSE || eventId == TS_EVENT_VCONN_OUTBOUND_CLOSE) { // Regardless of state, if the connection is closing, then transition to // the DONE state. This will trigger us to call the appropriate cleanup // routines. + // + // A connection can close while it is parked in a handshake hook, waiting for a plugin to + // reenable it. curHook then still points into that handshake hook's list. Each hook id owns + // a separate list, so advancing curHook below would walk the handshake list rather than the + // close list: the close event is dropped when the handshake list is exhausted, or delivered + // to the wrong plugin when it is not. Restart from the head of the close list unless we are + // already iterating it. + if (this->sslHandshakeHookState != SSLHandshakeHookState::HANDSHAKE_HOOKS_DONE) { + this->curHook = nullptr; + } this->sslHandshakeHookState = SSLHandshakeHookState::HANDSHAKE_HOOKS_DONE; } else { switch (this->sslHandshakeHookState) { diff --git a/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_expiry.test.py b/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_expiry.test.py new file mode 100644 index 00000000000..8ae995bf1c4 --- /dev/null +++ b/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_expiry.test.py @@ -0,0 +1,87 @@ +''' +Regression test for the max_age expiry branch of the rate_limit SNI queue accounting. A +queued connection never reserves a slot, so when the sweep expires it the plugin must +detach it rather than release a slot it never held; otherwise the expiry underflows the +active-slot counter and the next reserve() trips a release assertion, aborting the server. +ATS must survive the expiry. +''' +# 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 = __doc__ + +Test.SkipUnless(Condition.PluginExists('rate_limit.so')) + + +class RateLimitSniExpiryTest: + """Age a queued connection out via max_age and assert the active-slot counter stays balanced.""" + + def __init__(self) -> None: + tr = Test.AddTestRun('rate_limit SNI queue max_age expiry') + self._configure_trafficserver() + self._configure_client(tr) + + def _configure_trafficserver(self) -> None: + ts = Test.MakeATSProcess('ts', enable_tls=True, enable_cache=False) + self._ts = ts + ts.addDefaultSSLFiles() + for line in ['ssl_multicert:', ' - dest_ip: "*"', ' ssl_cert_name: server.pem', ' ssl_key_name: server.key']: + ts.Disk.ssl_multicert_yaml.AddLine(line) + + # One concurrent handshake for this SNI, a one-deep queue, and a 1s max age so the + # sweep expires the queued connection. Named .config (not .yaml) so autest treats it + # as a plain config file; the plugin parses it as YAML regardless. + ts.Disk.MakeConfigFile('rate_limit.config').AddLines( + [ + 'selector:', + ' - sni: rate.limited.com', + ' limit: 1', + ' queue:', + ' size: 1', + ' max_age: 1', + ]) + ts.Disk.plugin_config.AddLine(f'rate_limit.so {ts.Variables.CONFIGDIR}/rate_limit.config') + + # Disable the freelist / ProxyAllocator so allocation behavior is not a confound; the + # abort under test is a release-assertion, and this keeps the run representative of CI. + ts.Command += ' -f -F' + + ts.Disk.records_config.update( + { + 'proxy.config.ssl.server.cert.path': ts.Variables.SSLDir, + 'proxy.config.ssl.server.private_key.path': ts.Variables.SSLDir, + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'rate_limit', + }) + + # The expiry branch is actually reached... + ts.Disk.traffic_out.Content = Testers.ContainsExpression('too old', 'a queued connection was expired') + # ...and expiring it does not underflow the active-slot counter into the release assertion. + ts.Disk.traffic_out.Content += Testers.ExcludesExpression( + '_active <= _limit|received signal', 'expiring a queued connection must not underflow and abort ATS') + + def _configure_client(self, tr: 'TestRun') -> None: + ts = self._ts + client = os.path.join(Test.TestDirectory, 'rate_limit_sni_expiry_client.sh') + tr.Processes.Default.Command = f'bash {client} 127.0.0.1 {ts.Variables.ssl_port} rate.limited.com' + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.StartBefore(ts) + tr.Processes.Default.Streams.stdout = Testers.ContainsExpression('rate_limit-expiry-done', 'the client ran to completion') + + +RateLimitSniExpiryTest() diff --git a/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_expiry_client.sh b/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_expiry_client.sh new file mode 100644 index 00000000000..10742f2c8c7 --- /dev/null +++ b/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_expiry_client.sh @@ -0,0 +1,77 @@ +#!/usr/bin/env bash +# +# 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. +# +# Exercise the max_age EXPIRY branch of the rate_limit SNI queue accounting. A queued +# connection never reserves a slot; when the sweep expires it (max_age), it must be +# detached so its close does not release a slot it never held. Otherwise the expiry is an +# unmatched decrement of the active-slot counter, and -- combined with the holder's own +# close -- the counter wraps below zero and the limiter's release assertion aborts ATS. +# +# 1. holder completes its handshake and holds the single slot (counter = 1); +# 2. one connection enqueues (slot full) and stays parked -- it is NOT disconnected, so +# only the sweep's max_age expiry removes it; +# 3. after max_age the sweep errors it out -> (unfixed) unmatched decrement -> counter 1->0; +# 4. the holder is closed; its matched decrement lands on the understated counter -> wrap; +# 5. a probe connection's reserve() observes the wrapped counter and the assertion aborts. +# +# args: host port sni +set -u +host="$1" +port="$2" +sni="$3" + +OSSL="openssl s_client -connect ${host}:${port} -servername ${sni} -quiet -no_ign_eof" + +# Run a command in the background and terminate it after a deadline. coreutils "timeout" is not +# available everywhere (notably macOS), so do it with sleep and kill. +run_for() { + deadline="$1" + shift + "$@" & + target=$! + ( + sleep "${deadline}" + kill -TERM "${target}" 2>/dev/null + ) & +} + +# 1. Holder: hold the single slot. Its stdin is a FIFO on fd 3 so we end it in step 4. +fifo_dir="$(mktemp -d "${TMPDIR:-/tmp}/rl_holder.XXXXXX")" +fifo="${fifo_dir}/fifo" +mkfifo "$fifo" +${OSSL} <"$fifo" >/dev/null 2>&1 & +exec 3<>"$fifo" +rm -rf "$fifo_dir" +sleep 3 # let the holder reserve the one slot + +# 2. One queued connection: enqueues and stays parked at the ClientHello hook (not killed), +# so the sweep's max_age expiry -- not a disconnect or a resume -- is what removes it. +${OSSL} /dev/null 2>&1 & +queued=$! +sleep 3 # > max_age (1s) + sweeps: the expiry path errors the queued connection out + +# 4. End the holder: its matched decrement lands on the (unfixed) understated counter. +exec 3>&- +sleep 2 + +# 5. Probe: its reserve() reads the counter; if it wrapped, the release assertion aborts. +run_for 2 sh -c "${OSSL} /dev/null 2>&1" +kill "${queued}" 2>/dev/null || true +sleep 1 + +echo "rate_limit-expiry-done" diff --git a/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_queue.test.py b/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_queue.test.py new file mode 100644 index 00000000000..59042752620 --- /dev/null +++ b/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_queue.test.py @@ -0,0 +1,89 @@ +''' +Regression test for a queue-accounting balance bug in the rate_limit SNI limiter: a +queued connection never reserves a slot, but its VCONN_CLOSE unconditionally releases +one, so a queued connection that closes underflows the active-slot counter and the next +reserve() trips a release assertion, aborting the server. ATS must survive the queue +churn. +''' +# 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 = __doc__ + +Test.SkipUnless(Condition.PluginExists('rate_limit.so')) + + +class RateLimitSniQueueTest: + """Churn the rate_limit SNI queue and assert the active-slot counter never underflows.""" + + def __init__(self) -> None: + tr = Test.AddTestRun('rate_limit SNI queue accounting') + self._configure_trafficserver() + self._configure_client(tr) + + def _configure_trafficserver(self) -> None: + ts = Test.MakeATSProcess('ts', enable_tls=True, enable_cache=False) + self._ts = ts + ts.addDefaultSSLFiles() + for line in ['ssl_multicert:', ' - dest_ip: "*"', ' ssl_cert_name: server.pem', ' ssl_key_name: server.key']: + ts.Disk.ssl_multicert_yaml.AddLine(line) + + # One concurrent handshake for this SNI and a queue that admits exactly one more. + # No rate and no max_age -- the sweep's resume path alone drives the scenario, with + # no rate-bucket or expiry timing to confound it. Named .config (not .yaml) so autest + # treats it as a plain config file; the plugin parses it as YAML regardless. + ts.Disk.MakeConfigFile('rate_limit.config').AddLines( + [ + 'selector:', + ' - sni: rate.limited.com', + ' limit: 1', + ' queue:', + ' size: 1', + ]) + ts.Disk.plugin_config.AddLine(f'rate_limit.so {ts.Variables.CONFIGDIR}/rate_limit.config') + + # Disable the freelist / ProxyAllocator so freed objects are really released rather + # than recycled, keeping allocation reuse from masking a stale access. + ts.Command += ' -f -F' + + ts.Disk.records_config.update( + { + 'proxy.config.ssl.server.cert.path': ts.Variables.SSLDir, + 'proxy.config.ssl.server.private_key.path': ts.Variables.SSLDir, + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'rate_limit', + }) + + # The queue path is reached... + ts.Disk.traffic_out.Content = Testers.ContainsExpression('Queueing the VC', 'a connection was queued') + # ...and the active-slot counter never underflows into the release assertion. Match + # both the specific assertion (pins the failure to this bug) and the generic abort. + ts.Disk.traffic_out.Content += Testers.ExcludesExpression( + '_active <= _limit|received signal', 'the active-slot counter must not underflow and abort ATS') + + def _configure_client(self, tr: 'TestRun') -> None: + ts = self._ts + client = os.path.join(Test.TestDirectory, 'rate_limit_sni_queue_client.sh') + tr.Processes.Default.Command = f'bash {client} 127.0.0.1 {ts.Variables.ssl_port} rate.limited.com' + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.StartBefore(ts) + tr.Processes.Default.Streams.stdout = Testers.ContainsExpression( + 'rate_limit-queue-crash-done', 'the client ran to completion') + + +RateLimitSniQueueTest() diff --git a/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_queue_client.sh b/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_queue_client.sh new file mode 100644 index 00000000000..0ce7e56f3b1 --- /dev/null +++ b/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_queue_client.sh @@ -0,0 +1,78 @@ +#!/usr/bin/env bash +# +# 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. +# +# Drive the rate_limit SNI limiter's queue-then-resume path with exactly one queued connection, +# and check that the active-slot counter stays balanced and the server survives. +# +# 1. holder completes its handshake and holds the single slot (counter = 1); +# 2. one connection enqueues because the slot is full, then closes while parked; +# 3. the sweep reserves a slot and resumes a queued connection; +# 4. the holder is closed and releases its slot; +# 5. a probe connection reserves the freed slot. +# +# Against the plugin before 508c1bea26 this aborts the server: the sweep resumed a queued +# connection without a reservation, whose close then decremented the counter unmatched until it +# wrapped and reserve() tripped TSReleaseAssert(_active <= _limit). The test asserts the counter +# never wraps and no signal is logged, so it pins that fix as well as this change. +# +# args: host port sni +set -u +host="$1" +port="$2" +sni="$3" + +OSSL="openssl s_client -connect ${host}:${port} -servername ${sni} -quiet -no_ign_eof" + +# Run a command in the background and terminate it after a deadline. coreutils "timeout" is not +# available everywhere (notably macOS), so do it with sleep and kill. +run_for() { + deadline="$1" + shift + "$@" & + target=$! + ( + sleep "${deadline}" + kill -TERM "${target}" 2>/dev/null + ) & +} + +# 1. Holder: hold the single slot. Its stdin is a FIFO kept open on fd 3, so we end the +# holder deterministically in step 4 (closing fd 3 -> EOF -> clean TLS close -> FIN). +fifo_dir="$(mktemp -d "${TMPDIR:-/tmp}/rl_holder.XXXXXX")" +fifo="${fifo_dir}/fifo" +mkfifo "$fifo" +${OSSL} <"$fifo" >/dev/null 2>&1 & +exec 3<>"$fifo" +rm -rf "$fifo_dir" +sleep 3 # let the holder reserve the one slot + +# 2. One queued connection: enqueues because the slot is full, then closes while still parked +# at the ClientHello hook. +run_for 0.3 sh -c "${OSSL} /dev/null 2>&1" +sleep 2 # >= 2 sweep periods (300ms each), so the sweep runs while the connection is queued + +# 4. End the holder, releasing its slot. +exec 3>&- # close the FIFO write end -> holder sees EOF -> clean TLS close (FIN) +sleep 2 + +# 5. Probe: reserve() must succeed against a balanced counter rather than tripping the +# TSReleaseAssert(_active <= _limit) that a wrapped counter causes. +run_for 2 sh -c "${OSSL} /dev/null 2>&1" +sleep 3 + +echo "rate_limit-queue-crash-done" diff --git a/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_reject.test.py b/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_reject.test.py new file mode 100644 index 00000000000..976d320deb1 --- /dev/null +++ b/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_reject.test.py @@ -0,0 +1,83 @@ +''' +Exercise the rate_limit SNI limiter's reject path against a TLS listener, so the +consumer-driven SSLNetVConnection teardown frees every rejected handshake VC +cleanly (no use-after-free or crash). +''' +# 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 = __doc__ + +Test.SkipUnless(Condition.PluginExists('rate_limit.so')) + + +class RateLimitSniRejectTest: + """Drive rate_limit's SNI reject path and assert ATS frees the VCs without a fault.""" + + def __init__(self) -> None: + tr = Test.AddTestRun('rate_limit SNI reject teardown') + self._configure_trafficserver() + self._configure_client(tr) + + def _configure_trafficserver(self) -> None: + ts = Test.MakeATSProcess('ts', enable_tls=True, enable_cache=False) + self._ts = ts + ts.addDefaultSSLFiles() + for line in ['ssl_multicert:', ' - dest_ip: "*"', ' ssl_cert_name: server.pem', ' ssl_key_name: server.key']: + ts.Disk.ssl_multicert_yaml.AddLine(line) + + # One concurrent handshake for this SNI and no queue, so every further concurrent + # handshake is rejected outright (TS_EVENT_ERROR) rather than queued. Named .config + # (not .yaml) so autest treats it as a plain config file; the plugin parses it as + # YAML regardless (YAML::LoadFile). + ts.Disk.MakeConfigFile('rate_limit.config').AddLines([ + 'selector:', + ' - sni: rate.limited.com', + ' limit: 1', + ]) + ts.Disk.plugin_config.AddLine(f'rate_limit.so {ts.Variables.CONFIGDIR}/rate_limit.config') + + # Disable the freelist / ProxyAllocator so a freed SSLNetVConnection is really + # free()'d rather than recycled; a stale-VC access then hits freed memory + # instead of a still-valid recycled object. + ts.Command += ' -f -F' + + ts.Disk.records_config.update( + { + 'proxy.config.ssl.server.cert.path': ts.Variables.SSLDir, + 'proxy.config.ssl.server.private_key.path': ts.Variables.SSLDir, + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'rate_limit', + }) + + # The reject disposition is reached... + ts.Disk.traffic_out.Content = Testers.ContainsExpression('Rejecting connection', 'over-limit handshakes were rejected') + # ...and ATS tears every rejected handshake VC down without a memory-safety fault. + ts.Disk.traffic_out.Content += Testers.ExcludesExpression( + 'use-after-free|attempting free|SEGV|received signal', 'ATS must survive the reject churn') + + def _configure_client(self, tr: 'TestRun') -> None: + ts = self._ts + client = os.path.join(Test.TestDirectory, 'rate_limit_sni_reject_client.sh') + tr.Processes.Default.Command = f'bash {client} 127.0.0.1 {ts.Variables.ssl_port} rate.limited.com' + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.StartBefore(ts) + tr.Processes.Default.Streams.stdout = Testers.ContainsExpression('rate_limit-reject-done', 'the client ran to completion') + + +RateLimitSniRejectTest() diff --git a/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_reject_client.sh b/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_reject_client.sh new file mode 100644 index 00000000000..8d44b314bcd --- /dev/null +++ b/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_reject_client.sh @@ -0,0 +1,60 @@ +#!/usr/bin/env bash +# +# 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. +# +# Drive the rate_limit SNI limiter (limit 1, no queue) through its reject path so the +# consumer-driven SSLNetVConnection teardown is exercised for a rejected handshake: +# holder completes the handshake and HOLDS the one slot open; +# a burst of near-simultaneous handshakes then arrives while the slot is taken and, +# with no queue configured, each is REJECTED with TS_EVENT_ERROR mid-handshake. +# ATS must free every one of these rejected handshake VCs cleanly. +# +# args: host port sni +set -u +host="$1" +port="$2" +sni="$3" + +OSSL="openssl s_client -connect ${host}:${port} -servername ${sni} -quiet -no_ign_eof" + +# Run a command in the background and terminate it after a deadline. coreutils "timeout" is not +# available everywhere (notably macOS), so do it with sleep and kill. +run_for() { + deadline="$1" + shift + "$@" & + target=$! + ( + sleep "${deadline}" + kill -TERM "${target}" 2>/dev/null + ) & +} + +# holder: complete the handshake and hold the single slot for ~5s (slow stdin keeps it open). +(sleep 5) | ${OSSL} >/dev/null 2>&1 & +sleep 2 # let the holder reserve the slot + +# Burst of near-simultaneous handshakes against the full limiter; with no queue every one +# is rejected with TS_EVENT_ERROR, so its handshake VC is torn down consumer-driven. +for _ in $(seq 5); do + run_for 2 sh -c "${OSSL} /dev/null 2>&1" +done + +# Let the burst finish and the holder release its slot cleanly. +sleep 4 + +echo "rate_limit-reject-done" diff --git a/tests/gold_tests/tls_hooks/tls_hooks_close_while_parked.test.py b/tests/gold_tests/tls_hooks/tls_hooks_close_while_parked.test.py new file mode 100644 index 00000000000..89fe0a50a3f --- /dev/null +++ b/tests/gold_tests/tls_hooks/tls_hooks_close_while_parked.test.py @@ -0,0 +1,88 @@ +''' +Verify that a plugin's VCONN_CLOSE hook runs when the connection closes while it is +parked in a TLS handshake hook. +''' +# 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 = ''' +A connection that closes while parked in a TLS handshake hook must still deliver +TS_VCONN_CLOSE_HOOK to the plugin. +''' + +Test.SkipUnless(Condition.HasOpenSSLVersion("1.1.1"),) + +ts = Test.MakeATSProcess("ts", enable_tls=True) +server = Test.MakeOriginServer("server") +server.addResponse( + "sessionlog.json", { + "headers": "GET / HTTP/1.1\r\nHost: www.example.com\r\n\r\n", + "timestamp": "1469733493.993", + "body": "" + }, { + "headers": "HTTP/1.1 200 OK\r\nConnection: close\r\n\r\n", + "timestamp": "1469733493.993", + "body": "" + }) + +ts.addDefaultSSLFiles() + +ts.Disk.records_config.update( + { + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.show_location': 0, + 'proxy.config.diags.debug.tags': 'ssl_hook_test', + # Fire the handshake timeout while the plugin still has the handshake parked (2s park). + 'proxy.config.ssl.handshake_timeout_in': 1, + 'proxy.config.ssl.server.cert.path': '{0}'.format(ts.Variables.SSLDir), + 'proxy.config.ssl.server.private_key.path': '{0}'.format(ts.Variables.SSLDir), + }) + +ts.Disk.ssl_multicert_yaml.AddLines( + """ +ssl_multicert: + - dest_ip: "*" + ssl_cert_name: server.pem + ssl_key_name: server.key +""".split("\n")) + +ts.Disk.remap_config.AddLine( + 'map https://example.com:{1} http://127.0.0.1:{0}'.format(server.Variables.Port, ts.Variables.ssl_port)) + +# The delayed client hello callback parks the handshake for 2 seconds before it reenables. +Test.PrepareTestPlugin(os.path.join(Test.Variables.AtsTestPluginsDir, 'ssl_hook_test.so'), ts, '-client_hello=1 -close=1') + +# Give up after 1 second, which is inside the 2 second park, so the connection closes while it +# is still suspended in the client hello hook. curl reports operation timed out (exit 28). +tr = Test.AddTestRun("Client disconnects while parked in the client hello hook") +tr.Processes.Default.StartBefore(server) +tr.Processes.Default.StartBefore(Test.Processes.ts) +tr.StillRunningAfter = ts +tr.StillRunningAfter = server +tr.MakeCurlCommand('-k --max-time 1 -H \'host:example.com:{0}\' https://127.0.0.1:{0}'.format(ts.Variables.ssl_port), ts=ts) +tr.Processes.Default.ReturnCode = 28 +tr.Processes.Default.TimeOut = 15 +tr.TimeOut = 15 + +# The handshake really was parked. +ts.Disk.traffic_out.Content = Testers.ContainsExpression("Client Hello callback 0", "the handshake parked in the client hello hook") + +# The close hook must still fire, with the correct event. Before the fix, callHooks() advanced +# curHook within the client hello hook list instead of the close hook list, so this never ran. +ts.Disk.traffic_out.Content += Testers.ContainsExpression( + "Close callback 0 .* - event is good", "the close hook ran for the parked connection")