From 6fb80a1148ddbfc7e8123fd039698b92bfa63df2 Mon Sep 17 00:00:00 2001 From: Masakazu Kitajo Date: Tue, 4 Aug 2026 23:31:55 -0600 Subject: [PATCH 01/10] autest: skip tls_engine_abort test when plugin is absent (#13494) Same gap caf9c87097 fixed in tls_async_handshake.test.py: SkipUnless only registers conditions for later, it doesn't stop the script from running, so PrepareTestPlugin still ran at load time and raised a ValueError on non-OpenSSL builds where async_handshake.so is never built. tls_engine_abort.test.py was added afterward from the same template but didn't carry the guard over. Guard the call on file existence so it skips cleanly. (cherry picked from commit f00122e03a5376bfb29a141797cc8530dd82db09) --- tests/gold_tests/tls/tls_engine_abort.test.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/gold_tests/tls/tls_engine_abort.test.py b/tests/gold_tests/tls/tls_engine_abort.test.py index f2756e55ce3..06ffbfe56ad 100644 --- a/tests/gold_tests/tls/tls_engine_abort.test.py +++ b/tests/gold_tests/tls/tls_engine_abort.test.py @@ -42,7 +42,8 @@ # A wide pause window (well beyond the abort client's 0.4s handshake attempt) # so the abort reliably lands while the async job is still in flight. -Test.PrepareTestPlugin(async_handshake, ts, '-delay-ms=2000') +if os.path.isfile(async_handshake): + Test.PrepareTestPlugin(async_handshake, ts, '-delay-ms=2000') server.addResponse( "sessionlog.json", { From a403974a7664b071e3b1c2ac4f3af6ea11f4207b Mon Sep 17 00:00:00 2001 From: Masaori Koshiba Date: Wed, 5 Aug 2026 15:44:57 +0900 Subject: [PATCH 02/10] Only restore a retried parent when the retry succeeds (#13474) A parent that accepts connections but never sends a response was marked down and then restored to the pool by the very next retry probe, even when that probe also failed, so it kept receiving traffic indefinitely while healthy peers took none of the load. handle_response_from_parent() called markParentUp() on entry, before the switch on current.state, so a parent selected as a retry candidate was restored before ATS knew the retry's outcome. markParentUp() zeroes failedAt and failCount; the markParentDown() that follows a failed probe then takes its result->retry branch, which leaves new_fail_count at 0, so available is never re-cleared. The parent therefore returned to the pool with a cleared failure count after every retry_time window, no matter how long it stayed degraded. The CONNECTION_ALIVE branch already calls markParentUp() for a retry that actually succeeded, which is the correct restore point. Removing the entry call restores the behavior that predates #8546: that commit backed out the retry-limiting work and replaced a retryComplete() call -- which only decremented an in-flight retry counter and never touched availability -- with markParentUp(), silently turning "the retry finished" into "the parent is healthy". Two autests pin both outcomes of the retry path. A parent that is still silent when probed stays down; a parent that has recovered is restored. Each was negative-controlled against its own call site: reverting this change fails the first, and removing the CONNECTION_ALIVE call fails only the second. (cherry picked from commit 698cb9f0e7080a6d872192dc69e7ee93ce7e7045) --- doc/admin-guide/files/records.yaml.en.rst | 4 + src/proxy/http/HttpTransact.cc | 6 - .../parent_retry_availability.test.py | 26 +++ ...arent_retry_failure_stays_down.replay.yaml | 215 ++++++++++++++++++ .../parent_retry_success_restores.replay.yaml | 177 ++++++++++++++ 5 files changed, 422 insertions(+), 6 deletions(-) create mode 100644 tests/gold_tests/parent_proxy/parent_retry_availability.test.py create mode 100644 tests/gold_tests/parent_proxy/replays/parent_retry_failure_stays_down.replay.yaml create mode 100644 tests/gold_tests/parent_proxy/replays/parent_retry_success_restores.replay.yaml diff --git a/doc/admin-guide/files/records.yaml.en.rst b/doc/admin-guide/files/records.yaml.en.rst index 3bf27624ef2..4934c8aaadb 100644 --- a/doc/admin-guide/files/records.yaml.en.rst +++ b/doc/admin-guide/files/records.yaml.en.rst @@ -1532,6 +1532,10 @@ Parent Proxy Configuration The amount of time allowed between connection retries to a parent cache that is unavailable. + Once this time has elapsed the parent is selected again as a retry candidate. It is + restored to the pool only if that retry actually succeeds; if the retry fails, the parent + remains unavailable and a further ``retry_time`` must elapse before it is tried again. + .. ts:cv:: CONFIG proxy.config.http.parent_proxy.max_trans_retries INT 2 Limits the number of simultaneous transactions that may retry a parent once the parents diff --git a/src/proxy/http/HttpTransact.cc b/src/proxy/http/HttpTransact.cc index edf6b2c862e..c6d1b274ca0 100644 --- a/src/proxy/http/HttpTransact.cc +++ b/src/proxy/http/HttpTransact.cc @@ -3836,12 +3836,6 @@ HttpTransact::handle_response_from_parent(State *s) TxnDbg(dbg_ctl_http_trans, "(hrfp)"); HTTP_RELEASE_ASSERT(s->current.server == &s->parent_info); - // if this parent was retried from a markdown, then - // notify that the retry has completed. - if (s->parent_result.retry) { - markParentUp(s); - } - simple_or_unavailable_server_retry(s); s->parent_info.state = s->current.state; diff --git a/tests/gold_tests/parent_proxy/parent_retry_availability.test.py b/tests/gold_tests/parent_proxy/parent_retry_availability.test.py new file mode 100644 index 00000000000..51a7b1448d1 --- /dev/null +++ b/tests/gold_tests/parent_proxy/parent_retry_availability.test.py @@ -0,0 +1,26 @@ +""" +Verify the retry path restores a parent only when the retry actually succeeds. +""" +# 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. + +Test.Summary = ''' +Both outcomes of a parent retry: a parent that is still up but not serving stays +marked down, and a parent that has recovered is restored to the pool. +''' + +Test.ATSReplayTest(replay_file='replays/parent_retry_failure_stays_down.replay.yaml') +Test.ATSReplayTest(replay_file='replays/parent_retry_success_restores.replay.yaml') diff --git a/tests/gold_tests/parent_proxy/replays/parent_retry_failure_stays_down.replay.yaml b/tests/gold_tests/parent_proxy/replays/parent_retry_failure_stays_down.replay.yaml new file mode 100644 index 00000000000..d2c5fc5bd88 --- /dev/null +++ b/tests/gold_tests/parent_proxy/replays/parent_retry_failure_stays_down.replay.yaml @@ -0,0 +1,215 @@ +# 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. + +# +# A parent that accepts the connection but never sends a response ("up but not +# serving") must stay marked down when a retry probe also fails. +# +# A retried parent must only be restored once a retry actually succeeds. +# markParentUp() zeroes failedAt and failCount, and the markParentDown() that +# follows a failed probe takes its result->retry branch, which leaves +# new_fail_count at 0 and so never re-clears available. Restoring the parent +# before the probe's outcome is known therefore returns it to the pool with a +# clean counter after every retry_time window, no matter how long it stays +# degraded. That is why the restore belongs in the CONNECTION_ALIVE branch of +# HttpTransact::handle_response_from_parent and not at function entry. +# +# The verifier server plays the parent: server-response.delay (4s) exceeds the +# child's transaction_no_activity_timeout_out (2s), so ATS reads zero bytes and +# times out (INACTIVE_TIMEOUT), which enable_parent_timeout_markdowns=1 admits +# as a markdown-eligible failure. +# + +meta: + version: "1.0" + +autest: + description: 'A failed retry probe must not restore a degraded parent' + + server: + name: 'server-degraded' + # Every transaction is abandoned by ATS mid-flight (the response is still + # being held when the timeout fires), so the server may report an incomplete + # transaction cycle. The assertions live in the client statuses and the ATS + # diags log. + return_code: [0, 1] + + client: + name: 'client-degraded' + process_config: + # Each request waits out a 2s ATS timeout, and one waits a further 3s for + # retry_time. The default 5s poll timeout would abandon a transaction + # mid-flight and report no status violation at all. + other_args: '--poll-timeout 30000' + + # Resolve every hostname to loopback. The origin is never actually contacted + # (go_direct=false keeps every request on the parent), but ATS may still run + # an origin DNS lookup on the way to parent selection, and an unresolvable + # name would fail the request before selection runs. + dns: + name: 'dns-degraded' + + ats: + name: 'ts-degraded' + process_config: + enable_cache: false + + records_config: + proxy.config.diags.debug.enabled: 1 + proxy.config.diags.debug.tags: 'parent_select|http_trans' + # NOTE: no_dns_just_forward_to_parent is deliberately NOT set here. It + # routes the request through the parentExists() shortcut in + # HttpTransact::HandleRequest, which tests pRecord::available directly and + # so short-circuits to an error as soon as the parent is marked down -- + # before ParentRoundRobin::selectParent runs. The retry-window logic under + # test lives inside selectParent, so the request has to reach it. + # + # With the cache disabled every request is non-lookupable, and this + # setting (default 1) would then send each one direct to the origin + # instead of the parent -- "request not cacheable, so bypass parent". + proxy.config.http.uncacheable_requests_bypass_parent: 0 + # A read timeout only counts toward markdown when this is enabled. It is + # off by default upstream but enabled in the production config this + # reproduces. + proxy.config.http.parent_proxy.enable_parent_timeout_markdowns: 1 + # One timeout demotes the parent, keeping the test to four requests. + proxy.config.http.parent_proxy.fail_threshold: 1 + # Short enough that a single in-replay delay outlasts the down window. + proxy.config.http.parent_proxy.retry_time: 2 + # No intra-request retry looping; one attempt per request keeps the + # failure accounting one-to-one with the transactions below. + proxy.config.http.parent_proxy.total_connect_attempts: 1 + proxy.config.http.parent_proxy.per_parent_connect_attempts: 1 + # Keep the parent out of the HostStatus map so availability is governed + # purely by retry_time. + proxy.config.http.parent_proxy.self_detect: 0 + # Bound the wait on the silent parent. + proxy.config.http.transaction_no_activity_timeout_out: 2 + proxy.config.url_remap.remap_required: 0 + + # A single parent with go_direct=false: once it is unavailable there is + # nowhere else to go, so selection-time skipping is visible as a 502. + parent_config: + - 'dest_domain=. parent="127.0.0.1:{SERVER_HTTP_PORT}" go_direct=false parent_is_proxy=true' + + log_validation: + diags_log: + contains: + # The retry probe fails, so the parent must be re-marked down rather + # than left available. + - expression: 'Parent retry marked as down 127\.0\.0\.1:\d+' + description: 'The failed retry probe re-marks the parent down' + excludes: + # Note() emitted only by ParentSelectionStrategy::markParentUp. Every + # request in this replay fails, so no retry ever succeeds and the + # parent must never be restored. + - expression: 'http parent proxy 127\.0\.0\.1:\d+ restored with request' + description: 'A failed retry probe must not restore the parent' + +sessions: +- transactions: + # 1: the parent accepts but never responds. ATS times out (504) and the + # failure count reaches fail_threshold, so the parent is marked down as this + # request finishes. + - all: { headers: { fields: [[ uuid, degrade1 ]]}} + client-request: + method: GET + version: "1.1" + scheme: http + url: /degrade1 + headers: + fields: + - [ Host, example.com ] + - [ Content-Length, "0" ] + server-response: + delay: 4s + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "0" ] + proxy-response: + status: 504 + + # 2: still inside retry_time. The parent is skipped at selection and, with no + # other parent and go_direct=false, ATS returns 502 without touching the leg. + # This confirms the markdown from request 1 actually took effect. + - all: { headers: { fields: [[ uuid, downcheck ]]}} + client-request: + method: GET + version: "1.1" + scheme: http + url: /downcheck + headers: + fields: + - [ Host, example.com ] + - [ Content-Length, "0" ] + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "0" ] + proxy-response: + status: 502 + + # 3: the retry probe. The client delay pushes this request past retry_time, so + # parent selection picks the parent as a retry candidate. The probe reaches the + # still-silent parent and times out (504). The parent must not be restored: + # markParentUp belongs to the CONNECTION_ALIVE branch, which this never takes. + - all: { headers: { fields: [[ uuid, retryprobe ]]}} + client-request: + method: GET + version: "1.1" + scheme: http + url: /retryprobe + delay: 3s + headers: + fields: + - [ Host, example.com ] + - [ Content-Length, "0" ] + server-response: + delay: 4s + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "0" ] + proxy-response: + status: 504 + + # 4: the retry probe failed, so the parent must still be down. Selection skips + # it and ATS returns 502 without touching the leg. A 504 here would mean the + # failed probe had restored the parent and it was attempted again. + - all: { headers: { fields: [[ uuid, aftercheck ]]}} + client-request: + method: GET + version: "1.1" + scheme: http + url: /aftercheck + headers: + fields: + - [ Host, example.com ] + - [ Content-Length, "0" ] + server-response: + delay: 4s + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "0" ] + proxy-response: + status: 502 diff --git a/tests/gold_tests/parent_proxy/replays/parent_retry_success_restores.replay.yaml b/tests/gold_tests/parent_proxy/replays/parent_retry_success_restores.replay.yaml new file mode 100644 index 00000000000..91a8942d53e --- /dev/null +++ b/tests/gold_tests/parent_proxy/replays/parent_retry_success_restores.replay.yaml @@ -0,0 +1,177 @@ +# 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. + +# +# A parent that recovers must be restored to the pool by a successful retry. +# +# This is the positive counterpart to parent_retry_failure_stays_down.replay.yaml, +# which covers the case where the retry probe also fails. Together they pin both +# outcomes of the retry path: restore only on success, stay down on failure. +# +# The parent stops being silent partway through the replay, so the retry probe +# sent after retry_time elapses actually succeeds. markParentUp() must then run +# from the CONNECTION_ALIVE branch of handle_response_from_parent, clearing the +# failure count and returning the parent to the pool. +# + +meta: + version: "1.0" + +autest: + description: 'A successful retry restores a parent that had been marked down' + + server: + name: 'server-recovered' + # The first transaction is abandoned by ATS mid-flight (the response is + # still being held when the timeout fires), so the server may report an + # incomplete transaction cycle. + return_code: [0, 1] + + client: + name: 'client-recovered' + process_config: + # The first request waits out a 2s ATS timeout and the retry probe waits a + # further 3s for retry_time, both well past the 5s default. + other_args: '--poll-timeout 30000' + + # Resolve every hostname to loopback, as in the failing-retry scenario. + dns: + name: 'dns-recovered' + + ats: + name: 'ts-recovered' + process_config: + enable_cache: false + + records_config: + proxy.config.diags.debug.enabled: 1 + proxy.config.diags.debug.tags: 'parent_select|http_trans' + # See parent_retry_failure_stays_down.replay.yaml for why + # no_dns_just_forward_to_parent must stay unset and why this must be 0. + proxy.config.http.uncacheable_requests_bypass_parent: 0 + proxy.config.http.parent_proxy.enable_parent_timeout_markdowns: 1 + proxy.config.http.parent_proxy.fail_threshold: 1 + proxy.config.http.parent_proxy.retry_time: 2 + proxy.config.http.parent_proxy.total_connect_attempts: 1 + proxy.config.http.parent_proxy.per_parent_connect_attempts: 1 + proxy.config.http.parent_proxy.self_detect: 0 + proxy.config.http.transaction_no_activity_timeout_out: 2 + proxy.config.url_remap.remap_required: 0 + + parent_config: + - 'dest_domain=. parent="127.0.0.1:{SERVER_HTTP_PORT}" go_direct=false parent_is_proxy=true' + + log_validation: + diags_log: + contains: + # Note() from ParentSelectionStrategy::markParentUp, emitted only when + # a retry succeeds against a parent with a non-zero failure count. + - expression: 'http parent proxy 127\.0\.0\.1:\d+ restored with request' + description: 'A successful retry restores the parent' + excludes: + # The retry succeeds, so it must never be re-marked down. + - expression: 'Parent retry marked as down' + description: 'A successful retry must not re-mark the parent down' + +sessions: +- transactions: + # 1: the parent accepts but never responds. ATS times out (504) and marks it + # down, since fail_threshold is 1. + - all: { headers: { fields: [[ uuid, degrade1 ]]}} + client-request: + method: GET + version: "1.1" + scheme: http + url: /degrade1 + headers: + fields: + - [ Host, example.com ] + - [ Content-Length, "0" ] + server-response: + delay: 4s + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "0" ] + proxy-response: + status: 504 + + # 2: still inside retry_time, so the parent is skipped at selection and ATS + # returns 502 without touching the leg. + - all: { headers: { fields: [[ uuid, downcheck ]]}} + client-request: + method: GET + version: "1.1" + scheme: http + url: /downcheck + headers: + fields: + - [ Host, example.com ] + - [ Content-Length, "0" ] + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "0" ] + proxy-response: + status: 502 + + # 3: the retry probe. The client delay pushes it past retry_time so the parent + # is selected as a retry candidate, and this time the parent responds promptly + # (no server delay). The 200 proves the retry succeeded and the parent was + # restored from the CONNECTION_ALIVE branch. + - all: { headers: { fields: [[ uuid, retryprobe ]]}} + client-request: + method: GET + version: "1.1" + scheme: http + url: /retryprobe + delay: 3s + headers: + fields: + - [ Host, example.com ] + - [ Content-Length, "0" ] + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "0" ] + proxy-response: + status: 200 + + # 4: the parent is available again, so this request goes straight through + # without waiting for another retry_time window. + - all: { headers: { fields: [[ uuid, aftercheck ]]}} + client-request: + method: GET + version: "1.1" + scheme: http + url: /aftercheck + headers: + fields: + - [ Host, example.com ] + - [ Content-Length, "0" ] + server-response: + status: 200 + reason: OK + headers: + fields: + - [ Content-Length, "0" ] + proxy-response: + status: 200 From 79861fb489f11985be527272286a470bbd54ef0c Mon Sep 17 00:00:00 2001 From: JosiahWI <41302989+JosiahWI@users.noreply.github.com> Date: Wed, 5 Aug 2026 11:22:41 -0500 Subject: [PATCH 03/10] Fix bad return in `validate_hostname` (#13498) The function has a `bool` return type, but returned `-1` to indicate an error on UTF-8 conversion failure. This patch corrects the return statement to return `false`. (cherry picked from commit d70bdb703519d5ca9828803c362e3723cbac7130) --- src/tscore/X509HostnameValidator.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tscore/X509HostnameValidator.cc b/src/tscore/X509HostnameValidator.cc index 0efbcac99df..5d98b73f055 100644 --- a/src/tscore/X509HostnameValidator.cc +++ b/src/tscore/X509HostnameValidator.cc @@ -281,7 +281,7 @@ validate_hostname(X509 *x, std::string_view hostname, bool is_ip, char **peernam astrlen = ASN1_STRING_to_UTF8(&astr, str); if (astrlen < 0) { - return -1; + return false; } retval = equal(astr, astrlen, hostname_data, hostname_len); if (retval && peername) { From 2b7fb195db335311e9e4fd2eb66c05e0420ec661 Mon Sep 17 00:00:00 2001 From: Masakazu Kitajo Date: Wed, 5 Aug 2026 10:26:21 -0600 Subject: [PATCH 04/10] Resolve OpenSSL 4.0 build issues (#13476) * Resolve OpenSSL 4.0 build issues - accessors for ASN1_STRING and const-iness * Ran clang-format to fix formatting * Fix X509_NAME_get_index_by_NID const mismatch on OpenSSL 1.1.1 OpenSSL 1.1.1 declares the first argument as non-const while newer releases declare it const, so building against different versions failed depending on which signature was in effect. Add a const_cast at each call site to keep the const-qualified variables introduced by this PR buildable against both, matching the existing precedent in OCSPStapling.cc. * Drop const_cast in favor of auto for local X509_NAME variables X509_get_subject_name and X509_NAME_get_index_by_NID have their argument constness changed together across OpenSSL versions, so a local variable declared with auto tracks whatever type is correct for the OpenSSL version in use, without a cast. This applies only to the two purely local variables; ts_util.cc ssl_value_for keeps its const_cast since its parameter type is shared across multiple callers. * Fix const-cast style and X509_NAME leak flagged by Copilot review X509HostnameValidator.cc cast ASN1_STRING_get0_data return value to non-const before an ats_strndup call that only wants const char *; drop the const instead of adding it back needlessly. certifier.cc leaked the duplicated X509_NAME on the X509_NAME_add_entry_by_txt failure path, and never checked X509_NAME_dup for allocation failure. * Make Cripts X509 accessors OpenSSL-4-compatible CertBase::X509Value took hardcoded function pointer types for X509_get_subject_name, X509_get_issuer_name, X509_getm_notBefore, and X509_getm_notAfter, but those accessors change constness in different directions across OpenSSL versions, so no single hardcoded signature builds everywhere. Deduce the parameter type from the actual accessor via decltype instead. Signature::_load and _write_ip_address also read ASN1_STRING fields directly, which breaks once the struct is opaque; switch to the accessor functions used elsewhere in this codebase. * Fix lua plugin X509_NAME/ASN1_STRING OpenSSL 4 compatibility get_x509_name_string only reads through the name via X509_NAME_print_ex, so accept a const X509_NAME * to match callers that pass X509_get_subject_name/X509_get_issuer_name results directly. get_x509_signature_string read the ASN1_STRING struct fields directly, which breaks once the struct is opaque; use ASN1_STRING_get0_data/ASN1_STRING_length instead. * Fix self-signed test cert losing its CN under OpenSSL 4 make_cert_and_key mutated the X509_NAME returned by X509_get_subject_name in place, which stops compiling once that accessor can return const, and was already fragile since the returned name is only a view into the certificate internal state. Duplicate it, add the CN to the duplicate, and set it back as both subject and issuer name since this is a self-signed certificate. * Fix OpenSSL 4 X509_NAME const mismatches in example plugin and test tool client_context_dump.cc, verify_cert.cc, and ssl_client_verify_test.cc all held X509_get_subject_name/X509_NAME_get_entry/ X509_NAME_ENTRY_get_data results in hardcoded non-const locals or parameters, which stops compiling once those accessors return const. Switch to auto for the local variables and const for the debug_certificate parameter, matching the read-only usage in each case. * Drop last const_cast in txn_box ssl_value_for via a template ssl_value_for is shared by four callers, each already deducing its X509_NAME pointer type with auto from X509_get_subject_name or X509_get_issuer_name, so unlike the other three call sites fixed earlier in this series, a single hardcoded parameter type cannot track the underlying accessor across OpenSSL versions. Templating the parameter on the callers deduced type removes the cast entirely. * Alias the Cripts X509 getter decltypes for readability Bare decltype(&X509_get_subject_name) in a parameter list reads poorly at each of the four call sites; name each getter type once via using so the declarations and out-of-line definitions just say what kind of accessor they take. * Adopt RAII and decltype(func(nullptr)) patterns from PR #13482 certifier.cc freed the duplicated X509_NAME manually on every path; switch to a scoped_X509_NAME unique_ptr matching the file existing scoped_X509/scoped_EVP_PKEY/scoped_SSL_CTX aliases so no path can forget to free it. ts_util.cc templated ssl_value_for just to defer the parameter type to the caller; decltype(X509_get_subject_name(nullptr)) deduces the same pointer type directly without turning it into a template. Also drop the last C-style cast this series introduced in X509HostnameValidator.cc in favor of reinterpret_cast. --------- Co-authored-by: Jered Floyd (cherry picked from commit bdefaf97b6a00af1d6bcfe7f6ba810469fcfd69c) --- .../client_context_dump.cc | 2 +- .../plugins/c-api/verify_cert/verify_cert.cc | 2 +- include/cripts/Certs.hpp | 13 ++++++++---- plugins/certifier/certifier.cc | 21 +++++++++++++------ .../cert_reporting_tool.cc | 2 +- plugins/experimental/sslheaders/expand.cc | 8 +++---- .../txn_box/plugin/src/ts_util.cc | 4 +++- plugins/lua/ts_lua_client_cert_helpers.h | 11 ++++++---- src/api/InkAPI.cc | 6 +++--- src/cripts/Certs.cc | 16 +++++++------- src/iocore/net/OCSPStapling.cc | 8 +++---- src/iocore/net/SSLNetVConnection.cc | 2 +- src/iocore/net/SSLUtils.cc | 11 +++++----- src/iocore/net/unit_tests/test_SSLDHParams.cc | 5 ++++- src/tscore/X509HostnameValidator.cc | 17 +++++++-------- tests/tools/plugins/ssl_client_verify_test.cc | 10 ++++----- 16 files changed, 79 insertions(+), 59 deletions(-) diff --git a/example/plugins/c-api/client_context_dump/client_context_dump.cc b/example/plugins/c-api/client_context_dump/client_context_dump.cc index f8572d775ce..6dcb94919bb 100644 --- a/example/plugins/c-api/client_context_dump/client_context_dump.cc +++ b/example/plugins/c-api/client_context_dump/client_context_dump.cc @@ -65,7 +65,7 @@ dump_context(const char *ca_path, const char *ck_path) // expiration date, serial number, common name, and subject alternative names const ASN1_TIME *not_after = X509_get_notAfter(cert); const ASN1_INTEGER *serial = X509_get_serialNumber(cert); - X509_NAME *subject_name = X509_get_subject_name(cert); + auto *subject_name = X509_get_subject_name(cert); // Subject name BIO *subject_bio = BIO_new(BIO_s_mem()); diff --git a/example/plugins/c-api/verify_cert/verify_cert.cc b/example/plugins/c-api/verify_cert/verify_cert.cc index a3f553057fe..4c3c75446ee 100644 --- a/example/plugins/c-api/verify_cert/verify_cert.cc +++ b/example/plugins/c-api/verify_cert/verify_cert.cc @@ -37,7 +37,7 @@ namespace DbgCtl dbg_ctl{PLUGIN_NAME}; static void -debug_certificate(const char *msg, X509_NAME *name) +debug_certificate(const char *msg, const X509_NAME *name) { BIO *bio; diff --git a/include/cripts/Certs.hpp b/include/cripts/Certs.hpp index f19fb8f41b0..9b402a070e9 100644 --- a/include/cripts/Certs.hpp +++ b/include/cripts/Certs.hpp @@ -107,10 +107,15 @@ class CertBase } } - void _load_name(X509_NAME *(*getter)(const X509 *)) const; - void _load_integer(ASN1_INTEGER *(*getter)(X509 *)) const; - void _load_long(long (*getter)(const X509 *)) const; - void _load_time(ASN1_TIME *(*getter)(const X509 *)) const; + using NameGetter = decltype(&X509_get_subject_name); + using IntegerGetter = decltype(&X509_get_serialNumber); + using LongGetter = decltype(&X509_get_version); + using TimeGetter = decltype(&X509_get_notBefore); + + void _load_name(NameGetter getter) const; + void _load_integer(IntegerGetter getter) const; + void _load_long(LongGetter getter) const; + void _load_time(TimeGetter getter) const; CertBase *_owner = nullptr; mutable std::unique_ptr _bio{nullptr, BIO_free}; diff --git a/plugins/certifier/certifier.cc b/plugins/certifier/certifier.cc index 563a39bd40f..94a490c2fb4 100644 --- a/plugins/certifier/certifier.cc +++ b/plugins/certifier/certifier.cc @@ -88,10 +88,11 @@ template <> struct default_delete { } // namespace std /// Name aliases for unique pts to openSSL objects -using scoped_X509 = std::unique_ptr; -using scoped_X509_REQ = std::unique_ptr; -using scoped_EVP_PKEY = std::unique_ptr; -using scoped_SSL_CTX = std::unique_ptr; +using scoped_X509 = std::unique_ptr; +using scoped_X509_REQ = std::unique_ptr; +using scoped_EVP_PKEY = std::unique_ptr; +using scoped_SSL_CTX = std::unique_ptr; +using scoped_X509_NAME = std::unique_ptr; class SslLRUList { @@ -401,12 +402,20 @@ mkcrt(const std::string &commonName, int serial) X509_gmtime_adj(X509_get_notAfter(cert.get()), static_cast(3650) * 24 * 3600); // Get handle to subject name - X509_NAME *n = X509_get_subject_name(cert.get()); + scoped_X509_NAME n{X509_NAME_dup(X509_get_subject_name(cert.get())), X509_NAME_free}; + if (n == nullptr) { + TSError("[%s] %s: failed to duplicate certificate subject", PLUGIN_NAME, __func__); + return nullptr; + } // Set common name field - if (X509_NAME_add_entry_by_txt(n, "CN", MBSTRING_ASC, (unsigned char *)commonName.c_str(), -1, -1, 0) != 1) { + if (X509_NAME_add_entry_by_txt(n.get(), "CN", MBSTRING_ASC, (unsigned char *)commonName.c_str(), -1, -1, 0) != 1) { TSError("[%s] %s: failed to add certificate subject CN", PLUGIN_NAME, __func__); return nullptr; } + if (X509_set_subject_name(cert.get(), n.get()) != 1) { + TSError("[%s] %s: failed to set certificate subject", PLUGIN_NAME, __func__); + return nullptr; + } // Set Traffic Server public key if (X509_set_pubkey(cert.get(), ca_pkey_scoped.get()) == 0) { diff --git a/plugins/experimental/cert_reporting_tool/cert_reporting_tool.cc b/plugins/experimental/cert_reporting_tool/cert_reporting_tool.cc index fd1d49f1486..3613508890a 100644 --- a/plugins/experimental/cert_reporting_tool/cert_reporting_tool.cc +++ b/plugins/experimental/cert_reporting_tool/cert_reporting_tool.cc @@ -65,7 +65,7 @@ dump_context(const char *ca_path, const char *ck_path) // expiration date, serial number, common name, and subject alternative names const ASN1_TIME *not_after = X509_get_notAfter(cert); const ASN1_INTEGER *serial = X509_get_serialNumber(cert); - X509_NAME *subject_name = X509_get_subject_name(cert); + const X509_NAME *subject_name = X509_get_subject_name(cert); // Subject name BIO *subject_bio = BIO_new(BIO_s_mem()); diff --git a/plugins/experimental/sslheaders/expand.cc b/plugins/experimental/sslheaders/expand.cc index f6a8a0c7548..cbba6594240 100644 --- a/plugins/experimental/sslheaders/expand.cc +++ b/plugins/experimental/sslheaders/expand.cc @@ -49,14 +49,14 @@ x509_expand_certificate(X509 *x509, BIO *bio) static void x509_expand_subject(X509 *x509, BIO *bio) { - X509_NAME *name = X509_get_subject_name(x509); + const X509_NAME *name = X509_get_subject_name(x509); X509_NAME_print_ex(bio, name, 0 /* indent */, XN_FLAG_ONELINE); } static void x509_expand_issuer(X509 *x509, BIO *bio) { - X509_NAME *name = X509_get_issuer_name(x509); + const X509_NAME *name = X509_get_issuer_name(x509); X509_NAME_print_ex(bio, name, 0 /* indent */, XN_FLAG_ONELINE); } @@ -72,8 +72,8 @@ x509_expand_signature(X509 *x509, BIO *bio) { const ASN1_BIT_STRING *sig; X509_get0_signature(&sig, nullptr, x509); - const char *ptr = reinterpret_cast(sig->data); - const char *end = ptr + sig->length; + const char *ptr = reinterpret_cast(ASN1_STRING_get0_data(sig)); + const char *end = ptr + ASN1_STRING_length(sig); // The canonical OpenSSL way to format the signature seems to be // X509_signature_dump(). However that separates each byte with a ':', which is diff --git a/plugins/experimental/txn_box/plugin/src/ts_util.cc b/plugins/experimental/txn_box/plugin/src/ts_util.cc index a1881768956..c63de2f2fd7 100644 --- a/plugins/experimental/txn_box/plugin/src/ts_util.cc +++ b/plugins/experimental/txn_box/plugin/src/ts_util.cc @@ -1111,8 +1111,10 @@ ssl_nid(swoc::TextView const &name) namespace { + using X509_NAME_ptr = decltype(X509_get_subject_name(nullptr)); + TextView - ssl_value_for(X509_NAME *name, int nid) + ssl_value_for(X509_NAME_ptr name, int nid) { if (int loc = X509_NAME_get_index_by_NID(name, nid, -1); loc >= 0) { if (auto entry = X509_NAME_get_entry(name, loc); entry != nullptr) { diff --git a/plugins/lua/ts_lua_client_cert_helpers.h b/plugins/lua/ts_lua_client_cert_helpers.h index c7f20b50ebc..a7098ed2c0a 100644 --- a/plugins/lua/ts_lua_client_cert_helpers.h +++ b/plugins/lua/ts_lua_client_cert_helpers.h @@ -18,7 +18,7 @@ // Helper functions for certificate data extraction static std::string -get_x509_name_string(X509_NAME *name) +get_x509_name_string(const X509_NAME *name) { if (!name) { return ""; @@ -157,12 +157,15 @@ get_x509_signature_string(X509 *cert) return ""; } - for (int i = 0; i < sig->length; i++) { - if (BIO_printf(bio, "%02x", sig->data[i]) <= 0) { + const unsigned char *sig_data = ASN1_STRING_get0_data(sig); + int sig_len = ASN1_STRING_length(sig); + + for (int i = 0; i < sig_len; i++) { + if (BIO_printf(bio, "%02x", sig_data[i]) <= 0) { BIO_free(bio); return ""; } - if (i < sig->length - 1) { + if (i < sig_len - 1) { if (BIO_printf(bio, ":") <= 0) { BIO_free(bio); return ""; diff --git a/src/api/InkAPI.cc b/src/api/InkAPI.cc index 05fc77e2558..1d4d3c9d32f 100644 --- a/src/api/InkAPI.cc +++ b/src/api/InkAPI.cc @@ -8319,9 +8319,9 @@ TSSslServerCertUpdate(const char *cert_path, const char *key_path) } // Extract common name - int pos = X509_NAME_get_index_by_NID(X509_get_subject_name(cert.get()), NID_commonName, -1); - X509_NAME_ENTRY *common_name = X509_NAME_get_entry(X509_get_subject_name(cert.get()), pos); - ASN1_STRING *common_name_asn1 = X509_NAME_ENTRY_get_data(common_name); + const int pos = X509_NAME_get_index_by_NID(X509_get_subject_name(cert.get()), NID_commonName, -1); + const X509_NAME_ENTRY *common_name = X509_NAME_get_entry(X509_get_subject_name(cert.get()), pos); + const ASN1_STRING *common_name_asn1 = X509_NAME_ENTRY_get_data(common_name); char *common_name_str = reinterpret_cast(const_cast(ASN1_STRING_get0_data(common_name_asn1))); if (ASN1_STRING_length(common_name_asn1) != static_cast(strlen(common_name_str))) { // Embedded null char diff --git a/src/cripts/Certs.cc b/src/cripts/Certs.cc index 8f893c14dc2..006effcb90c 100644 --- a/src/cripts/Certs.cc +++ b/src/cripts/Certs.cc @@ -54,8 +54,8 @@ CertBase::Signature::_load() const if (!_ready && _owner->_x509) { const ASN1_BIT_STRING *sig; X509_get0_signature(&sig, nullptr, _owner->_x509); - const char *ptr = reinterpret_cast(sig->data); - const char *end = ptr + sig->length; + const char *ptr = reinterpret_cast(ASN1_STRING_get0_data(sig)); + const char *end = ptr + ASN1_STRING_length(sig); super_type::_load(); for (; ptr < end; ++ptr) { @@ -78,7 +78,7 @@ CertBase::X509Value::_update_value() const } void -CertBase::X509Value::_load_name(X509_NAME *(*getter)(const X509 *)) const +CertBase::X509Value::_load_name(NameGetter getter) const { if (!_ready && _owner->_x509) { auto *name = getter(_owner->_x509); @@ -95,7 +95,7 @@ CertBase::X509Value::_load_name(X509_NAME *(*getter)(const X509 *)) const } void -CertBase::X509Value::_load_integer(ASN1_INTEGER *(*getter)(X509 *)) const +CertBase::X509Value::_load_integer(IntegerGetter getter) const { if (!_ready && _owner->_x509) { auto *value = getter(_owner->_x509); @@ -107,7 +107,7 @@ CertBase::X509Value::_load_integer(ASN1_INTEGER *(*getter)(X509 *)) const } void -CertBase::X509Value::_load_long(long (*getter)(const X509 *)) const +CertBase::X509Value::_load_long(LongGetter getter) const { if (!_ready && _owner->_x509) { auto value = getter(_owner->_x509); @@ -119,7 +119,7 @@ CertBase::X509Value::_load_long(long (*getter)(const X509 *)) const } void -CertBase::X509Value::_load_time(ASN1_TIME *(*getter)(const X509 *)) const +CertBase::X509Value::_load_time(TimeGetter getter) const { if (!_ready && _owner->_x509) { auto *time = getter(_owner->_x509); @@ -153,8 +153,8 @@ namespace _write_ip_address(const ASN1_OCTET_STRING *ip, BIO *_bio) { char buffer[INET6_ADDRSTRLEN]; - const unsigned char *raw = ip->data; - int len = ip->length; + const unsigned char *raw = ASN1_STRING_get0_data(ip); + int len = ASN1_STRING_length(ip); if (inet_ntop(len == 4 ? AF_INET : AF_INET6, raw, buffer, sizeof(buffer))) { BIO_printf(_bio, "%s", buffer); diff --git a/src/iocore/net/OCSPStapling.cc b/src/iocore/net/OCSPStapling.cc index e1c2e4ca3b5..3e98d75d79c 100644 --- a/src/iocore/net/OCSPStapling.cc +++ b/src/iocore/net/OCSPStapling.cc @@ -489,7 +489,7 @@ TS_OCSP_cert_id_new(const EVP_MD *dgst, const X509_NAME *issuerName, const ASN1_ } /* Calculate the issuerKey hash, excluding tag and length */ - if (!EVP_Digest(issuerKey->data, issuerKey->length, md, &i, dgst, nullptr)) { + if (!EVP_Digest(ASN1_STRING_get0_data(issuerKey), ASN1_STRING_length(issuerKey), md, &i, dgst, nullptr)) { goto err; } @@ -513,9 +513,9 @@ TS_OCSP_cert_id_new(const EVP_MD *dgst, const X509_NAME *issuerName, const ASN1_ TS_OCSP_CERTID * TS_OCSP_cert_to_id(const EVP_MD *dgst, const X509 *subject, const X509 *issuer) { - const X509_NAME *iname; - const ASN1_INTEGER *serial; - ASN1_BIT_STRING *ikey; + const X509_NAME *iname; + const ASN1_INTEGER *serial; + const ASN1_BIT_STRING *ikey; if (!dgst) { dgst = EVP_sha1(); diff --git a/src/iocore/net/SSLNetVConnection.cc b/src/iocore/net/SSLNetVConnection.cc index 0423c0b37c3..f1e251bb7c2 100644 --- a/src/iocore/net/SSLNetVConnection.cc +++ b/src/iocore/net/SSLNetVConnection.cc @@ -167,7 +167,7 @@ SSLNetVConnection::_unbindSSLObject() } static void -debug_certificate_name(const char *msg, X509_NAME *name) +debug_certificate_name(const char *msg, const X509_NAME *name) { BIO *bio; diff --git a/src/iocore/net/SSLUtils.cc b/src/iocore/net/SSLUtils.cc index 22b93de583f..f528f1b3006 100644 --- a/src/iocore/net/SSLUtils.cc +++ b/src/iocore/net/SSLUtils.cc @@ -1023,7 +1023,7 @@ SSLMultiCertConfigLoader::check_server_cert_now(X509 *cert, const char *certname } /* CheckServerCertNow() */ static char * -asn1_strdup(ASN1_STRING *s) +asn1_strdup(const ASN1_STRING *s) { // Make sure we have an 8-bit encoding. ink_assert(ASN1_STRING_type(s) == V_ASN1_IA5STRING || ASN1_STRING_type(s) == V_ASN1_UTF8STRING || @@ -2347,10 +2347,9 @@ SSLMultiCertConfigLoader::load_certs_and_cross_reference_names( std::set name_set; // Grub through the names in the certs - X509_NAME *subject = nullptr; // Insert a key for the subject CN. - subject = X509_get_subject_name(cert); + auto *subject = X509_get_subject_name(cert); ats_scoped_str subj_name; if (subject) { int pos = -1; @@ -2360,9 +2359,9 @@ SSLMultiCertConfigLoader::load_certs_and_cross_reference_names( break; } - X509_NAME_ENTRY *e = X509_NAME_get_entry(subject, pos); - ASN1_STRING *cn = X509_NAME_ENTRY_get_data(e); - subj_name = asn1_strdup(cn); + const X509_NAME_ENTRY *e = X509_NAME_get_entry(subject, pos); + const ASN1_STRING *cn = X509_NAME_ENTRY_get_data(e); + subj_name = asn1_strdup(cn); Dbg(dbg_ctl_ssl_load, "subj '%s' in certificate %s %p", subj_name.get(), data.cert_names_list[i].c_str(), cert); name_set.insert(subj_name.get()); diff --git a/src/iocore/net/unit_tests/test_SSLDHParams.cc b/src/iocore/net/unit_tests/test_SSLDHParams.cc index 3e75f5fe097..8d0e44a4a0a 100644 --- a/src/iocore/net/unit_tests/test_SSLDHParams.cc +++ b/src/iocore/net/unit_tests/test_SSLDHParams.cc @@ -132,9 +132,12 @@ make_cert_and_key(EVP_CIPHER const *cipher = nullptr, char *pass = nullptr) X509_gmtime_adj(X509_getm_notAfter(x509), 60L * 60L * 24L * 365L); REQUIRE(X509_set_pubkey(x509, pkey) == 1); - X509_NAME *name = X509_get_subject_name(x509); + X509_NAME *name = X509_NAME_dup(X509_get_subject_name(x509)); + REQUIRE(name != nullptr); X509_NAME_add_entry_by_txt(name, "CN", MBSTRING_ASC, reinterpret_cast("ats-test"), -1, -1, 0); + REQUIRE(X509_set_subject_name(x509, name) == 1); REQUIRE(X509_set_issuer_name(x509, name) == 1); + X509_NAME_free(name); REQUIRE(X509_sign(x509, pkey, EVP_sha256()) > 0); BIO *cert_bio = BIO_new(BIO_s_mem()); diff --git a/src/tscore/X509HostnameValidator.cc b/src/tscore/X509HostnameValidator.cc index 5d98b73f055..36cba94714e 100644 --- a/src/tscore/X509HostnameValidator.cc +++ b/src/tscore/X509HostnameValidator.cc @@ -206,12 +206,12 @@ do_check_string(ASN1_STRING *a, int cmp_type, equal_fn equal, const unsigned cha { bool retval = false; - if (!a->data || !a->length || cmp_type != a->type) { + if (!ASN1_STRING_get0_data(a) || !ASN1_STRING_length(a) || cmp_type != ASN1_STRING_type(a)) { return false; } - retval = equal(a->data, a->length, b, blen); + retval = equal(ASN1_STRING_get0_data(a), ASN1_STRING_length(a), b, blen); if (retval && peername) { - *peername = ats_strndup((char *)a->data, a->length); + *peername = ats_strndup(reinterpret_cast(ASN1_STRING_get0_data(a)), ASN1_STRING_length(a)); } return retval; } @@ -220,7 +220,6 @@ bool validate_hostname(X509 *x, std::string_view hostname, bool is_ip, char **peername) { GENERAL_NAMES *gens = nullptr; - X509_NAME *name = nullptr; int i; int alt_type; bool retval = false; @@ -269,13 +268,13 @@ validate_hostname(X509 *x, std::string_view hostname, bool is_ip, char **peernam } } // No SAN match -- check the subject - i = -1; - name = X509_get_subject_name(x); + i = -1; + auto *name = X509_get_subject_name(x); while ((i = X509_NAME_get_index_by_NID(name, NID_commonName, i)) >= 0) { - ASN1_STRING *str; - int astrlen; - unsigned char *astr; + const ASN1_STRING *str; + int astrlen; + unsigned char *astr; str = X509_NAME_ENTRY_get_data(X509_NAME_get_entry(name, i)); // Convert to UTF-8 astrlen = ASN1_STRING_to_UTF8(&astr, str); diff --git a/tests/tools/plugins/ssl_client_verify_test.cc b/tests/tools/plugins/ssl_client_verify_test.cc index 7f3ae7c3759..a4243818e05 100644 --- a/tests/tools/plugins/ssl_client_verify_test.cc +++ b/tests/tools/plugins/ssl_client_verify_test.cc @@ -58,7 +58,7 @@ check_names(X509 *cert) bool retval = false; // Check the common name - X509_NAME *subject = X509_get_subject_name(cert); + auto *subject = X509_get_subject_name(cert); if (subject) { int pos = -1; for (; !retval;) { @@ -67,10 +67,10 @@ check_names(X509 *cert) break; } - X509_NAME_ENTRY *e = X509_NAME_get_entry(subject, pos); - ASN1_STRING *cn = X509_NAME_ENTRY_get_data(e); - char *subj_name = strndup(reinterpret_cast(ASN1_STRING_get0_data(cn)), ASN1_STRING_length(cn)); - retval = check_name(subj_name); + auto *e = X509_NAME_get_entry(subject, pos); + auto *cn = X509_NAME_ENTRY_get_data(e); + char *subj_name = strndup(reinterpret_cast(ASN1_STRING_get0_data(cn)), ASN1_STRING_length(cn)); + retval = check_name(subj_name); free(subj_name); } } From 384547e40bc39886b0ba5e8101eb71f38ed89a09 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 5 Aug 2026 11:30:02 -0500 Subject: [PATCH 05/10] build(deps): bump golang.org/x/crypto (#13479) Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.50.0 to 0.52.0. - [Commits](https://github.com/golang/crypto/compare/v0.50.0...v0.52.0) --- updated-dependencies: - dependency-name: golang.org/x/crypto dependency-version: 0.52.0 dependency-type: indirect ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit c340299eb2fed54c80b00c9fdbe8c83b6849ff32) --- tests/gold_tests/qmux/go_qmux_client/go.mod | 6 +++--- tests/gold_tests/qmux/go_qmux_client/go.sum | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/gold_tests/qmux/go_qmux_client/go.mod b/tests/gold_tests/qmux/go_qmux_client/go.mod index 531eb56c069..45b8d1a4055 100644 --- a/tests/gold_tests/qmux/go_qmux_client/go.mod +++ b/tests/gold_tests/qmux/go_qmux_client/go.mod @@ -9,7 +9,7 @@ require ( ) require ( - golang.org/x/crypto v0.50.0 // indirect - golang.org/x/net v0.53.0 // indirect - golang.org/x/sys v0.43.0 // indirect + golang.org/x/crypto v0.52.0 // indirect + golang.org/x/net v0.54.0 // indirect + golang.org/x/sys v0.45.0 // indirect ) diff --git a/tests/gold_tests/qmux/go_qmux_client/go.sum b/tests/gold_tests/qmux/go_qmux_client/go.sum index b10735cd87f..2218ca4f078 100644 --- a/tests/gold_tests/qmux/go_qmux_client/go.sum +++ b/tests/gold_tests/qmux/go_qmux_client/go.sum @@ -16,11 +16,11 @@ github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= go.uber.org/mock v0.5.2 h1:LbtPTcP8A5k9WPXj54PPPbjcI4Y6lhyOZXn+VS7wNko= go.uber.org/mock v0.5.2/go.mod h1:wLlUxC2vVTPTaE3UD51E0BGOAElKrILxhVSDYQLld5o= -golang.org/x/crypto v0.50.0 h1:zO47/JPrL6vsNkINmLoo/PH1gcxpls50DNogFvB5ZGI= -golang.org/x/crypto v0.50.0/go.mod h1:3muZ7vA7PBCE6xgPX7nkzzjiUq87kRItoJQM1Yo8S+Q= -golang.org/x/net v0.53.0 h1:d+qAbo5L0orcWAr0a9JweQpjXF19LMXJE8Ey7hwOdUA= -golang.org/x/net v0.53.0/go.mod h1:JvMuJH7rrdiCfbeHoo3fCQU24Lf5JJwT9W3sJFulfgs= -golang.org/x/sys v0.43.0 h1:Rlag2XtaFTxp19wS8MXlJwTvoh8ArU6ezoyFsMyCTNI= -golang.org/x/sys v0.43.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/crypto v0.52.0 h1:RMs7fP2rXdep0CftQlK8Uf+kibLm7qkCcradZWYz988= +golang.org/x/crypto v0.52.0/go.mod h1:1QgfPxDqh0T2M/elOJtp9RvuR95kVjir0e6/BvEmGbc= +golang.org/x/net v0.54.0 h1:2zJIZAxAHV/OHCDTCOHAYehQzLfSXuf/5SoL/Dv6w/w= +golang.org/x/net v0.54.0/go.mod h1:Sj4oj8jK6XmHpBZU/zWHw3BV3abl4Kvi+Ut7cQcY+cQ= +golang.org/x/sys v0.45.0 h1:dO4czNzziLiiXplLQgBCEpCvXQ3dnkn0SdaZSYdQ+FY= +golang.org/x/sys v0.45.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= From 2e61a7aa91837bbc6660fc7e7f6fbddefe209b75 Mon Sep 17 00:00:00 2001 From: Chris McFarlen Date: Wed, 5 Aug 2026 13:57:27 -0500 Subject: [PATCH 06/10] Fall back to the build directory when GIT_COMMON_DIR is unset (#13497) Requiring GIT_COMMON_DIR made configure fatal in trees where git cannot resolve a common directory, such as a source export without .git or a worktree mapped into a container without the paths its .git file points at. Nothing about proxy-verifier needs that directory specifically; it is only a convenient cache shared across worktrees of one clone. Introduce PV_DEST_DIR, set from GIT_COMMON_DIR when available and the build directory otherwise, and use it for the download and extraction. (cherry picked from commit b8440f029ce60575ddd9ad1676ed560699ea2ee4) --- cmake/proxy-verifier.cmake | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/cmake/proxy-verifier.cmake b/cmake/proxy-verifier.cmake index bb44998b12d..789bef4f6c8 100644 --- a/cmake/proxy-verifier.cmake +++ b/cmake/proxy-verifier.cmake @@ -15,7 +15,7 @@ # ####################### -# This will download and extract proxy-verifier to git common directory and setup variables to point to it. +# This will download and extract proxy-verifier to PV_DEST_DIR and setup variables to point to it. # # Required variables: # PROXY_VERIFIER_VERSION @@ -23,6 +23,7 @@ # # Defines variables: # +# PV_DEST_DIR Directory the archive is downloaded to and extracted in # PROXY_VERIFIER_PATH Full path to the extracted proxy verifier for the build architecture # PROXY_VERIFIER_CLIENT Full path to client-verifier # PROXY_VERIFIER_SERVER Full path to server-verifier @@ -35,22 +36,29 @@ if(NOT PROXY_VERIFIER_HASH) message(FATAL_ERROR "PROXY_VERIFIER_HASH Required") endif() -# GIT_COMMON_DIR is set by the top-level CMakeLists.txt. -if(NOT GIT_COMMON_DIR) - message(FATAL_ERROR "GIT_COMMON_DIR not set. This should be set by the top-level CMakeLists.txt") +# Prefer the git common directory (set by the top-level CMakeLists.txt) so the download is shared by +# every worktree and build directory of the same clone. It isn't always available -- a source tree +# exported without .git, or a worktree whose common directory sits outside the paths visible to the +# build, such as when the worktree alone is mapped into a container. Fall back to the build +# directory, which always exists and is writable, rather than failing the configure. +if(GIT_COMMON_DIR) + set(PV_DEST_DIR "${GIT_COMMON_DIR}") +else() + set(PV_DEST_DIR "${CMAKE_BINARY_DIR}") + message(STATUS "GIT_COMMON_DIR not set, storing proxy-verifier in the build directory instead") endif() # Convert to absolute path (handles relative .git from regular non-worktree clones). -get_filename_component(GIT_COMMON_DIR "${GIT_COMMON_DIR}" ABSOLUTE BASE_DIR "${CMAKE_SOURCE_DIR}") +get_filename_component(PV_DEST_DIR "${PV_DEST_DIR}" ABSOLUTE BASE_DIR "${CMAKE_SOURCE_DIR}") -# Download proxy-verifier to git common directory. -set(PV_ARCHIVE ${GIT_COMMON_DIR}/proxy-verifier/proxy-verifier.tar.gz) +# Download proxy-verifier to the destination directory. +set(PV_ARCHIVE ${PV_DEST_DIR}/proxy-verifier/proxy-verifier.tar.gz) file( DOWNLOAD https://ci.trafficserver.apache.org/bintray/proxy-verifier-${PROXY_VERIFIER_VERSION}.tar.gz ${PV_ARCHIVE} EXPECTED_HASH ${PROXY_VERIFIER_HASH} SHOW_PROGRESS ) -file(ARCHIVE_EXTRACT INPUT ${PV_ARCHIVE} DESTINATION ${GIT_COMMON_DIR}) +file(ARCHIVE_EXTRACT INPUT ${PV_ARCHIVE} DESTINATION ${PV_DEST_DIR}) if(CMAKE_HOST_SYSTEM_NAME STREQUAL "Linux") if(CMAKE_HOST_SYSTEM_PROCESSOR STREQUAL "x86_64" @@ -78,7 +86,7 @@ else() message(FATAL_ERROR "Host ${CMAKE_HOST_SYSTEM_NAME} doesnt support running proxy verifier") endif() -set(PROXY_VERIFIER_PATH ${GIT_COMMON_DIR}/proxy-verifier-${PROXY_VERIFIER_VERSION}/${PV_SUBDIR}) +set(PROXY_VERIFIER_PATH ${PV_DEST_DIR}/proxy-verifier-${PROXY_VERIFIER_VERSION}/${PV_SUBDIR}) set(PROXY_VERIFIER_CLIENT ${PROXY_VERIFIER_PATH}/verifier-client) set(PROXY_VERIFIER_SERVER ${PROXY_VERIFIER_PATH}/verifier-server) From 16863b5ef1023a3cd04da07449928860f07e82c7 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 5 Aug 2026 14:37:59 -0500 Subject: [PATCH 07/10] build(deps): bump golang.org/x/net (#13500) Bumps [golang.org/x/net](https://github.com/golang/net) from 0.54.0 to 0.55.0. - [Commits](https://github.com/golang/net/compare/v0.54.0...v0.55.0) --- updated-dependencies: - dependency-name: golang.org/x/net dependency-version: 0.55.0 dependency-type: indirect ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit 190a07c9a990029790e531e64a29c45dfd9f79f4) --- tests/gold_tests/qmux/go_qmux_client/go.mod | 2 +- tests/gold_tests/qmux/go_qmux_client/go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/gold_tests/qmux/go_qmux_client/go.mod b/tests/gold_tests/qmux/go_qmux_client/go.mod index 45b8d1a4055..e9161bbcdb0 100644 --- a/tests/gold_tests/qmux/go_qmux_client/go.mod +++ b/tests/gold_tests/qmux/go_qmux_client/go.mod @@ -10,6 +10,6 @@ require ( require ( golang.org/x/crypto v0.52.0 // indirect - golang.org/x/net v0.54.0 // indirect + golang.org/x/net v0.55.0 // indirect golang.org/x/sys v0.45.0 // indirect ) diff --git a/tests/gold_tests/qmux/go_qmux_client/go.sum b/tests/gold_tests/qmux/go_qmux_client/go.sum index 2218ca4f078..7527a339a24 100644 --- a/tests/gold_tests/qmux/go_qmux_client/go.sum +++ b/tests/gold_tests/qmux/go_qmux_client/go.sum @@ -18,8 +18,8 @@ go.uber.org/mock v0.5.2 h1:LbtPTcP8A5k9WPXj54PPPbjcI4Y6lhyOZXn+VS7wNko= go.uber.org/mock v0.5.2/go.mod h1:wLlUxC2vVTPTaE3UD51E0BGOAElKrILxhVSDYQLld5o= golang.org/x/crypto v0.52.0 h1:RMs7fP2rXdep0CftQlK8Uf+kibLm7qkCcradZWYz988= golang.org/x/crypto v0.52.0/go.mod h1:1QgfPxDqh0T2M/elOJtp9RvuR95kVjir0e6/BvEmGbc= -golang.org/x/net v0.54.0 h1:2zJIZAxAHV/OHCDTCOHAYehQzLfSXuf/5SoL/Dv6w/w= -golang.org/x/net v0.54.0/go.mod h1:Sj4oj8jK6XmHpBZU/zWHw3BV3abl4Kvi+Ut7cQcY+cQ= +golang.org/x/net v0.55.0 h1:bcvxaJn3e1U6InsFWt1JUq1aSjnRxLzT2rtD2KfkDF8= +golang.org/x/net v0.55.0/go.mod h1:L5U2KuzuOe1lY7Z+aWVIKK6qEeJXnXV9yzGA+WCHJww= golang.org/x/sys v0.45.0 h1:dO4czNzziLiiXplLQgBCEpCvXQ3dnkn0SdaZSYdQ+FY= golang.org/x/sys v0.45.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= From 2696089658fead3383e86615cc6e9d2e5ff77742 Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Wed, 5 Aug 2026 15:09:11 -0500 Subject: [PATCH 08/10] Harden AuTests against timing races (#13489) Two AuTests can fail depending on process scheduling and response framing. The HTTP/2 chunked clients can reach their origins before the listeners are ready, while the capped stale-response fetch can stop at a body-block boundary and time out instead of exercising its memory fallback. This patch uses a deterministic raw origin that ignores readiness probes, serves one real request, and exits. It also sizes and documents the stale-response header at the cap-plus-sentinel read boundary so memory-limit rejection is independent of body segmentation. (cherry picked from commit 80452f02a1c701f8a1f84d2f3901d51578aa2bdb) --- .../chunked_encoding_h2.test.py | 21 ++-- .../chunked_encoding_h2_server.py | 115 ++++++++++++++++++ .../chunked_encoding/delay-server.sh | 43 ------- tests/gold_tests/chunked_encoding/server2.sh | 41 ------- tests/gold_tests/chunked_encoding/server3.sh | 41 ------- .../stale_response_max_memory.replay.yaml | 5 + 6 files changed, 132 insertions(+), 134 deletions(-) create mode 100644 tests/gold_tests/chunked_encoding/chunked_encoding_h2_server.py delete mode 100755 tests/gold_tests/chunked_encoding/delay-server.sh delete mode 100755 tests/gold_tests/chunked_encoding/server2.sh delete mode 100755 tests/gold_tests/chunked_encoding/server3.sh diff --git a/tests/gold_tests/chunked_encoding/chunked_encoding_h2.test.py b/tests/gold_tests/chunked_encoding/chunked_encoding_h2.test.py index 004267c2d60..0347e6c899a 100644 --- a/tests/gold_tests/chunked_encoding/chunked_encoding_h2.test.py +++ b/tests/gold_tests/chunked_encoding/chunked_encoding_h2.test.py @@ -16,6 +16,9 @@ # See the License for the specific language governing permissions and # limitations under the License. +import os +import sys + Test.Summary = ''' Test interaction of H2 and chunked encoding ''' @@ -36,12 +39,13 @@ # add ssl materials like key, certificates for the server ts.addDefaultSSLFiles() +origin_server = os.path.join(Test.TestDirectory, "chunked_encoding_h2_server.py") delay_server = Test.Processes.Process( - "delay-server", "bash -c '" + Test.TestDirectory + "/delay-server.sh {} outserver1'".format(Test.Variables.upstream_port)) + "delay-server", f'{sys.executable} "{origin_server}" 127.0.0.1 {Test.Variables.upstream_port} outserver1 delayed-chunked') server2 = Test.Processes.Process( - "server2", "bash -c '" + Test.TestDirectory + "/server2.sh {} outserver2'".format(Test.Variables.upstream_port2)) + "server2", f'{sys.executable} "{origin_server}" 127.0.0.1 {Test.Variables.upstream_port2} outserver2 content-length') server3 = Test.Processes.Process( - "server3", "bash -c '" + Test.TestDirectory + "/server3.sh {} outserver3'".format(Test.Variables.upstream_port3)) + "server3", f'{sys.executable} "{origin_server}" 127.0.0.1 {Test.Variables.upstream_port3} outserver3 chunked') ts.Disk.records_config.update( { @@ -58,9 +62,8 @@ ts.Disk.ssl_multicert_config.AddLine('dest_ip=* ssl_cert_name=server.pem ssl_key_name=server.key') -# Using netcat as a cheap origin server in case 1 so we can insert a delay in sending back the response. -# Replaced microserver for cases 2 and 3 as well because I was getting python exceptions when running -# microserver if chunked encoding headers were specified for the request headers +# Use a raw origin server in case 1 so the final chunk can be delayed. Use it +# for cases 2 and 3 as well because microserver rejects chunked request headers. # H2 GET request # chunked response without content-length @@ -70,7 +73,7 @@ tr.Processes.Default.Command = 'nghttp -vv https://127.0.0.1:{}/delay-chunked-response'.format(ts.Variables.ssl_port) tr.Processes.Default.ReturnCode = 0 tr.Processes.Default.StartBefore(Test.Processes.ts) -tr.Processes.Default.StartBefore(delay_server) +tr.Processes.Default.StartBefore(delay_server, ready=When.PortOpen(Test.Variables.upstream_port)) tr.Processes.Default.Streams.All = Testers.ExcludesExpression("RST_STREAM", "Delayed chunk close should not cause reset") tr.Processes.Default.Streams.All += Testers.ExcludesExpression("< content-length", "Should return chunked") tr.Processes.Default.Streams.All += Testers.ContainsExpression(":status: 200", "Should get successful response") @@ -81,7 +84,7 @@ # HTTP2 POST: www.example.com Host, chunked body server2_out = Test.Disk.File("outserver2") tr = Test.AddTestRun() -tr.Processes.Default.StartBefore(server2) +tr.Processes.Default.StartBefore(server2, ready=When.PortOpen(Test.Variables.upstream_port2)) tr.MakeCurlCommand( '--http2 -k https://127.0.0.1:{}/post-full --verbose -H "Transfer-encoding: chunked" -d "Knock knock"'.format( ts.Variables.ssl_port), @@ -97,7 +100,7 @@ # HTTP2 POST: chunked post body and chunked response server3_out = Test.Disk.File("outserver3") tr = Test.AddTestRun() -tr.Processes.Default.StartBefore(server3) +tr.Processes.Default.StartBefore(server3, ready=When.PortOpen(Test.Variables.upstream_port3)) tr.MakeCurlCommand( '--http2 -k https://127.0.0.1:{}/post-chunked --verbose -H "Transfer-encoding: chunked" -d "Knock knock"'.format( ts.Variables.ssl_port), diff --git a/tests/gold_tests/chunked_encoding/chunked_encoding_h2_server.py b/tests/gold_tests/chunked_encoding/chunked_encoding_h2_server.py new file mode 100644 index 00000000000..e1b003c9b0b --- /dev/null +++ b/tests/gold_tests/chunked_encoding/chunked_encoding_h2_server.py @@ -0,0 +1,115 @@ +#!/usr/bin/env python3 +"""Serve one raw HTTP request for the chunked HTTP/2 AuTest.""" + +# 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 argparse +from pathlib import Path +import socket +import sys +import time + + +def parse_args() -> argparse.Namespace: + """Parse the command-line arguments.""" + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("address", help="Address on which to listen.") + parser.add_argument("port", type=int, help="Port on which to listen.") + parser.add_argument("output", type=Path, help="File in which to record the request.") + parser.add_argument("response", choices=("delayed-chunked", "content-length", "chunked"), help="Response to send.") + return parser.parse_args() + + +def make_listening_socket(address: str, port: int) -> socket.socket: + """Create and return a listening TCP socket.""" + listener = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + listener.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + listener.bind((address, port)) + listener.listen(1) + return listener + + +def receive_request(conn: socket.socket) -> bytes: + """Receive one HTTP request, including its declared body.""" + request = b"" + while b"\r\n\r\n" not in request: + data = conn.recv(4096) + if not data: + return request + request += data + + header, _, body = request.partition(b"\r\n\r\n") + content_length = 0 + is_chunked = False + for field in header.split(b"\r\n")[1:]: + name, separator, value = field.partition(b":") + if not separator: + continue + name = name.strip().lower() + value = value.strip().lower() + if name == b"content-length": + content_length = int(value) + elif name == b"transfer-encoding" and b"chunked" in value: + is_chunked = True + + if is_chunked: + while not (body.startswith(b"0\r\n\r\n") or b"\r\n0\r\n\r\n" in body): + data = conn.recv(4096) + if not data: + break + body += data + else: + while len(body) < content_length: + data = conn.recv(4096) + if not data: + break + body += data + + return header + b"\r\n\r\n" + body + + +def send_response(conn: socket.socket, response: str) -> None: + """Send the selected raw HTTP response.""" + if response == "delayed-chunked": + conn.sendall(b"HTTP/1.1 200\r\nTransfer-encoding: chunked\r\n\r\n") + conn.sendall(b"F\r\n123456789012345\r\n") + time.sleep(1) + conn.sendall(b"0\r\n\r\n") + elif response == "content-length": + conn.sendall(b"HTTP/1.1 200\r\nContent-length: 15\r\n\r\n123456789012345") + else: + conn.sendall(b"HTTP/1.1 200\r\nTransfer-encoding: chunked\r\n\r\nF\r\n123456789012345\r\n0\r\n\r\n") + + +def main() -> int: + """Ignore readiness probes, serve one request, and exit.""" + args = parse_args() + with make_listening_socket(args.address, args.port) as listener: + while True: + conn, _ = listener.accept() + with conn: + request = receive_request(conn) + if not request: + # When.PortOpen probes the listener without sending data. + continue + args.output.write_bytes(request) + send_response(conn, args.response) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/gold_tests/chunked_encoding/delay-server.sh b/tests/gold_tests/chunked_encoding/delay-server.sh deleted file mode 100755 index c4d4846ddc3..00000000000 --- a/tests/gold_tests/chunked_encoding/delay-server.sh +++ /dev/null @@ -1,43 +0,0 @@ -# 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. - -# A very simple cleartext server for one HTTP transaction. Does no validation of the Request message. -# Sends a fixed response message - -response () -{ - # Wait for end of Request message. - # - while (( 1 == 1 )) - do - if [[ -f $outfile ]] ; then - if tr '\r\n' '=!' < $outfile | grep '=!=!' > /dev/null - then - break; - fi - fi - sleep 1 - done - - # delay before finishing the chunk - printf "HTTP/1.1 200\r\nTransfer-encoding: chunked\r\n\r\n" - printf "F\r\n123456789012345\r\n" - sleep 1 - printf "0\r\n\r\n" - -} -outfile=$2 -response | nc -l $1 > "$outfile" diff --git a/tests/gold_tests/chunked_encoding/server2.sh b/tests/gold_tests/chunked_encoding/server2.sh deleted file mode 100755 index 2fd88f60da4..00000000000 --- a/tests/gold_tests/chunked_encoding/server2.sh +++ /dev/null @@ -1,41 +0,0 @@ -# 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. - -# A very simple cleartext server for one HTTP transaction. Does no validation of the Request message. -# Sends a fixed response message - - -response () -{ - # Wait for end of Request message. - # - while (( 1 == 1 )) - do - if [[ -f $outfile ]] ; then - if tr '\r\n' '=!' < $outfile | grep '=!=!' > /dev/null - then - break; - fi - fi - sleep 1 - done - - printf "HTTP/1.1 200\r\nContent-length: 15\r\n\r\n" - printf "123456789012345" - -} -outfile=$2 -response | nc -l $1 > "$outfile" diff --git a/tests/gold_tests/chunked_encoding/server3.sh b/tests/gold_tests/chunked_encoding/server3.sh deleted file mode 100755 index e08087faefa..00000000000 --- a/tests/gold_tests/chunked_encoding/server3.sh +++ /dev/null @@ -1,41 +0,0 @@ -# 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. - -# A very simple cleartext server for one HTTP transaction. Does no validation of the Request message. -# Sends a fixed response message - - -response () -{ - # Wait for end of Request message. - # - while (( 1 == 1 )) - do - if [[ -f $outfile ]] ; then - if tr '\r\n' '=!' < $outfile | grep '=!=!' > /dev/null - then - break; - fi - fi - sleep 1 - done - - printf "HTTP/1.1 200\r\nTransfer-encoding: chunked\r\n\r\n" - printf "F\r\n123456789012345\r\n0\r\n\r\n" - -} -outfile=$2 -response | nc -l $1 > "$outfile" diff --git a/tests/gold_tests/pluginTest/stale_response/stale_response_max_memory.replay.yaml b/tests/gold_tests/pluginTest/stale_response/stale_response_max_memory.replay.yaml index 3b199dd6285..af031c9e7b7 100644 --- a/tests/gold_tests/pluginTest/stale_response/stale_response_max_memory.replay.yaml +++ b/tests/gold_tests/pluginTest/stale_response/stale_response_max_memory.replay.yaml @@ -77,6 +77,11 @@ sessions: - [ Connection, close ] - [ Cache-Control, "max-age=1" ] - [ X-Response, oversized-origin-response ] + # The preceding fields serialize to 215 bytes. X-Padding adds 42 wire + # bytes (name, separator, 29-byte value, and CRLF), bringing the header + # to the 256-byte limit plus the one-byte overflow sentinel. This makes + # the memory rejection independent of body segmentation. + - [ X-Padding, aaaaaaaaaaaaaaaaaaaaaaaaaaaaa ] content: size: 512 From 1986d64d7e4e9fc082f70b866487fd4992d97f20 Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Wed, 5 Aug 2026 16:59:51 -0500 Subject: [PATCH 09/10] header_rewrite: reject bad run-plugin at config load (#13493) A run-plugin whose target plugin failed to load left a null instance that tripped a release assert and aborted the server on the first request. Propagate the load failure as an exception so the rule is rejected at config load time (a reload simply keeps the running config), and guard exec() so a stray bad rule can never abort the process. Co-authored-by: Leif Hedstrom (cherry picked from commit 14bcf29953a07797e4661eac0a812bfa907b5b7d) --- plugins/header_rewrite/header_rewrite.cc | 31 ++-- plugins/header_rewrite/operators.cc | 36 ++-- plugins/header_rewrite/ruleset.cc | 25 ++- plugins/header_rewrite/ruleset.h | 2 +- .../header_rewrite_bad_run_plugin.test.py | 156 ++++++++++++++++++ 5 files changed, 201 insertions(+), 49 deletions(-) create mode 100644 tests/gold_tests/pluginTest/header_rewrite/header_rewrite_bad_run_plugin.test.py diff --git a/plugins/header_rewrite/header_rewrite.cc b/plugins/header_rewrite/header_rewrite.cc index b8c4f3c6dbf..80f720a935b 100644 --- a/plugins/header_rewrite/header_rewrite.cc +++ b/plugins/header_rewrite/header_rewrite.cc @@ -202,12 +202,12 @@ validate_rule_completion(RuleSet *rule, const std::string &fname, int lineno) bool RulesConfig::parse_config(const std::string &fname, TSHttpHookID default_hook, char *from_url, char *to_url) { - std::unique_ptr rule(nullptr); - std::string filename; - int lineno = 0; - ConditionGroup *group = nullptr; - std::stack group_stack; - std::stack if_stack; + std::unique_ptr rule(nullptr); + std::string filename; + int lineno = 0; + ConditionGroup *group = nullptr; + std::stack group_stack; + std::stack> if_stack; constexpr int MAX_IF_NESTING_DEPTH = 10; @@ -366,10 +366,8 @@ RulesConfig::parse_config(const std::string &fname, TSHttpHookID default_hook, c throw std::runtime_error("maximum if nesting depth exceeded"); } - auto *op_if = new OperatorIf(); - - if_stack.push(op_if); - group = op_if->get_group(); // Set group to the new OperatorIf's group + if_stack.push(std::make_unique()); + group = if_stack.top()->get_group(); // Set group to the new OperatorIf's group Dbg(dbg_ctl, "Started nested OperatorIf, depth: %zu", if_stack.size()); } else if (p.is_endif()) { @@ -377,21 +375,20 @@ RulesConfig::parse_config(const std::string &fname, TSHttpHookID default_hook, c throw std::runtime_error("endif without matching if"); } - OperatorIf *op_if = if_stack.top(); + auto op_if = std::move(if_stack.top()); if_stack.pop(); if (!if_stack.empty()) { auto *parent_sec = if_stack.top()->cur_section(); if (parent_sec->ops.oper) { - parent_sec->ops.oper->append(op_if); + parent_sec->ops.oper->append(op_if.release()); } else { - parent_sec->ops.oper.reset(op_if); + parent_sec->ops.oper = std::move(op_if); } group = if_stack.top()->get_group(); } else { - if (!rule->add_operator(op_if)) { - delete op_if; + if (!rule->add_operator(std::move(op_if))) { throw std::runtime_error("Failed to add nested OperatorIf to RuleSet"); } group = rule->get_group(); @@ -434,10 +431,6 @@ RulesConfig::parse_config(const std::string &fname, TSHttpHookID default_hook, c // Check for unmatched if statements if (!if_stack.empty()) { TSError("[%s] %zu unmatched 'if' statement(s) without 'endif' in file: %s", PLUGIN_NAME, if_stack.size(), fname.c_str()); - while (!if_stack.empty()) { - delete if_stack.top(); - if_stack.pop(); - } return false; } diff --git a/plugins/header_rewrite/operators.cc b/plugins/header_rewrite/operators.cc index 8013990285c..5d7bbab3860 100644 --- a/plugins/header_rewrite/operators.cc +++ b/plugins/header_rewrite/operators.cc @@ -23,6 +23,7 @@ #include #include #include +#include #include "records/RecCore.h" #include "ts/ts.h" @@ -1272,8 +1273,7 @@ OperatorRunPlugin::initialize(Parser &p) auto plugin_args = p.get_value(); if (plugin_name.empty()) { - TSError("[%s] missing plugin name", PLUGIN_NAME); - return; + throw std::runtime_error("run-plugin missing plugin name"); } std::vector tokens; @@ -1284,15 +1284,10 @@ OperatorRunPlugin::initialize(Parser &p) tokens.push_back(token); } - // Create argc and argv - int argc = tokens.size() + 2; - char **argv = new char *[argc]; - - argv[0] = p.from_url(); - argv[1] = p.to_url(); + std::vector argv{p.from_url(), p.to_url()}; - for (size_t i = 0; i < tokens.size(); ++i) { - argv[i + 2] = const_cast(tokens[i].c_str()); + for (auto const &argument : tokens) { + argv.push_back(const_cast(argument.c_str())); } std::string error; @@ -1304,14 +1299,12 @@ OperatorRunPlugin::initialize(Parser &p) elevate_access = RecGetRecordInt("proxy.config.plugin.load_elevated").value_or(0); ElevateAccess access(elevate_access ? ElevateAccess::FILE_PRIVILEGE : 0); - _plugin = plugin_factory.getRemapPlugin(swoc::file::path(plugin_name), argc, const_cast(argv), error, + _plugin = plugin_factory.getRemapPlugin(swoc::file::path(plugin_name), static_cast(argv.size()), argv.data(), error, isPluginDynamicReloadEnabled()); } // done elevating access - delete[] argv; - if (!_plugin) { - TSError("[%s] Unable to load plugin '%s': %s", PLUGIN_NAME, plugin_name.c_str(), error.c_str()); + throw std::runtime_error("run-plugin unable to load plugin '" + std::string{plugin_name} + "': " + error); } } @@ -1326,7 +1319,11 @@ OperatorRunPlugin::initialize_hooks() bool OperatorRunPlugin::exec(const Resources &res) const { - TSReleaseAssert(_plugin != nullptr); + // Rejected at config load (see initialize); guard anyway so a stray bad rule can't abort the server. + if (!_plugin) { + Dbg(pi_dbg_ctl, "OperatorRunPlugin::exec skipped, plugin was not loaded"); + return true; + } if (res._rri && res.state.txnp) { _plugin->doRemap(res.state.txnp, res._rri); @@ -1654,7 +1651,7 @@ OperatorIf::new_section(Parser::CondClause clause) bool OperatorIf::add_operator(Parser &p, const char *filename, int lineno) { - Operator *op = operator_factory(p.get_op()); + std::unique_ptr op{operator_factory(p.get_op())}; if (!op) { TSError("[%s] Unknown operator: %s, file: %s, line: %d", PLUGIN_NAME, p.get_op().c_str(), filename, lineno); @@ -1667,7 +1664,6 @@ OperatorIf::add_operator(Parser &p, const char *filename, int lineno) try { op->initialize(p); } catch (std::exception const &ex) { - delete op; TSError("[%s] Failed to initialize operator: %s, file: %s, line: %d, error: %s", PLUGIN_NAME, p.get_op().c_str(), filename, lineno, ex.what()); return false; @@ -1675,10 +1671,10 @@ OperatorIf::add_operator(Parser &p, const char *filename, int lineno) // Add to current section if (_cur_section->ops.oper) { - _cur_section->ops.oper->append(op); + _cur_section->ops.oper->append(op.release()); } else { - _cur_section->ops.oper.reset(op); - _cur_section->ops.oper_mods = op->get_oper_modifiers(); + _cur_section->ops.oper = std::move(op); + _cur_section->ops.oper_mods = _cur_section->ops.oper->get_oper_modifiers(); } return true; diff --git a/plugins/header_rewrite/ruleset.cc b/plugins/header_rewrite/ruleset.cc index acbeafc72f4..eae2ff0af15 100644 --- a/plugins/header_rewrite/ruleset.cc +++ b/plugins/header_rewrite/ruleset.cc @@ -19,6 +19,7 @@ // ruleset.cc: implementation of the ruleset class // // +#include #include #include "ruleset.h" @@ -89,14 +90,20 @@ RuleSet::make_condition(Parser &p, const char *filename, int lineno) bool RuleSet::add_operator(Parser &p, const char *filename, int lineno) { - Operator *op = operator_factory(p.get_op()); + std::unique_ptr op{operator_factory(p.get_op())}; - if (nullptr != op) { + if (op) { Dbg(pi_dbg_ctl, " Adding operator: %s(%s)=\"%s\"", p.get_op().c_str(), p.get_arg().c_str(), p.get_value().c_str()); op->set_config_location(filename, lineno); - op->initialize(p); + + try { + op->initialize(p); + } catch (std::exception const &ex) { + TSError("[%s] in %s:%d: failed to initialize operator %s: %s", PLUGIN_NAME, filename, lineno, p.get_op().c_str(), ex.what()); + return false; + } + if (!op->is_hook_valid(_hook)) { - delete op; Dbg(pi_dbg_ctl, "in %s:%d: can't use this operator in hook=%s: %s(%s)", filename, lineno, TSHttpHookNameLookup(_hook), p.get_op().c_str(), p.get_arg().c_str()); TSError("[%s] in %s:%d: can't use this operator in hook=%s: %s(%s)", PLUGIN_NAME, filename, lineno, @@ -107,9 +114,9 @@ RuleSet::add_operator(Parser &p, const char *filename, int lineno) auto *cur_sec = _op_if.cur_section(); if (!cur_sec->ops.oper) { - cur_sec->ops.oper.reset(op); + cur_sec->ops.oper = std::move(op); } else { - cur_sec->ops.oper->append(op); + cur_sec->ops.oper->append(op.release()); } cur_sec->ops.oper_mods = static_cast(cur_sec->ops.oper_mods | cur_sec->ops.oper->get_oper_modifiers()); @@ -136,14 +143,14 @@ RuleSet::get_all_resource_ids() const } bool -RuleSet::add_operator(Operator *op) +RuleSet::add_operator(std::unique_ptr op) { auto *cur_sec = _op_if.cur_section(); if (!cur_sec->ops.oper) { - cur_sec->ops.oper.reset(op); + cur_sec->ops.oper = std::move(op); } else { - cur_sec->ops.oper->append(op); + cur_sec->ops.oper->append(op.release()); } // Update some ruleset state based on this new operator diff --git a/plugins/header_rewrite/ruleset.h b/plugins/header_rewrite/ruleset.h index 78680bc9d7e..0ad40f7183a 100644 --- a/plugins/header_rewrite/ruleset.h +++ b/plugins/header_rewrite/ruleset.h @@ -49,7 +49,7 @@ class RuleSet Condition *make_condition(Parser &p, const char *filename, int lineno); ResourceIDs get_all_resource_ids() const; bool add_operator(Parser &p, const char *filename, int lineno); - bool add_operator(Operator *op); + bool add_operator(std::unique_ptr op); ConditionGroup * get_group() diff --git a/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_bad_run_plugin.test.py b/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_bad_run_plugin.test.py new file mode 100644 index 00000000000..c5371f5c9e9 --- /dev/null +++ b/tests/gold_tests/pluginTest/header_rewrite/header_rewrite_bad_run_plugin.test.py @@ -0,0 +1,156 @@ +# 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. +''' +Verify header_rewrite rejects a run-plugin operator whose target plugin fails to +load. The failure must be caught at config load time, not aborted at request time. +''' + +Test.Summary = ''' +header_rewrite must reject a run-plugin whose target plugin fails to load, at +config load time, rather than aborting the server on the first request. +''' + +# Reproduce the reported crash: run-plugin against a plugin whose instance-init fails +# (conf_remap + a missing file) hands header_rewrite a null instance, which old code aborted on. +Test.SkipUnless( + Condition.PluginExists('header_rewrite.so'), + Condition.PluginExists('conf_remap.so'), +) + + +class TestBadRunPlugin: + '''Verify failed run-plugin initialization is rejected safely.''' + + ERROR_MARKER: str = 'run-plugin unable to load' + BAD_RULE_LINES: list[str] = [ + 'cond %{REMAP_PSEUDO_HOOK}', + ' run-plugin conf_remap.so no_such_conf_remap_file.yaml', + ] + NESTED_BAD_RULE_LINES: list[str] = [ + 'cond %{REMAP_PSEUDO_HOOK}', + ' if', + ' cond %{TRUE}', + ' run-plugin conf_remap.so no_such_conf_remap_file.yaml', + ' endif', + ] + + def __init__(self) -> None: + '''Configure startup and reload rejection scenarios.''' + self._configure_startup_rejection() + self._server = self._configure_origin_server() + self._ts = self._configure_traffic_server() + self._configure_baseline_request() + self._configure_bad_remap_install() + self._configure_failed_reload() + self._configure_post_reload_request() + self._ts.Disk.diags_log.Content = Testers.IncludesExpression( + self.ERROR_MARKER, 'the rejected reload should log the run-plugin failure') + + def _configure_startup_rejection(self) -> None: + '''Verify a bad top-level run-plugin fails startup cleanly.''' + ts = Test.MakeATSProcess("ts-startup", disable_log_checks=True) + ts.Disk.records_config.update({ + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'header_rewrite', + }) + ts.Disk.MakeConfigFile('bad_run_plugin.conf').AddLines(self.BAD_RULE_LINES) + ts.Disk.remap_config.AddLine( + 'map http://startup.example.com/ http://127.0.0.1/ ' + '@plugin=header_rewrite.so @pparam=bad_run_plugin.conf') + + # Invalid remap.config triggers a controlled exit rather than SIGABRT. + ts.ReturnCode = 33 + ts.Ready = 0 + ts.Disk.diags_log.Content = Testers.IncludesExpression( + self.ERROR_MARKER, 'header_rewrite must report the failed run-plugin load') + ts.Disk.traffic_out.Content = Testers.ExcludesExpression( + 'Traffic Server is fully initialized', 'ATS must not initialize with a bad run-plugin config') + + tr = Test.AddTestRun("Bad run-plugin config fails startup instead of crashing") + tr.Processes.Default.Command = 'echo verifying startup rejection' + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.StartBefore(ts) + + def _configure_origin_server(self) -> 'Process': + '''Configure the origin used to verify reload behavior.''' + server = Test.MakeOriginServer("server") + request_header = { + "headers": "GET / HTTP/1.1\r\nHost: reload.example.com\r\n\r\n", + "timestamp": "1469733493.993", + "body": "" + } + response_header = {"headers": "HTTP/1.1 200 OK\r\nConnection: close\r\n\r\n", "timestamp": "1469733493.993", "body": ""} + server.addResponse("sessionfile.log", request_header, response_header) + return server + + def _configure_traffic_server(self) -> 'Process': + '''Configure ATS with a valid initial remap table.''' + ts = Test.MakeATSProcess("ts-reload", disable_log_checks=True) + ts.Disk.records_config.update({ + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'header_rewrite', + }) + ts.Disk.MakeConfigFile('nested_bad_run_plugin.conf').AddLines(self.NESTED_BAD_RULE_LINES) + ts.Disk.remap_config.AddLine(f'map http://reload.example.com http://127.0.0.1:{self._server.Variables.Port}') + return ts + + def _configure_curl_run(self, name: str, expectation: str) -> 'TestRun': + '''Configure a request that verifies ATS still serves traffic.''' + tr = Test.AddTestRun(name) + tr.MakeCurlCommand( + f'--proxy 127.0.0.1:{self._ts.Variables.port} "http://reload.example.com" ' + '-H "Proxy-Connection: keep-alive" --verbose', + ts=self._ts) + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.Streams.stderr = Testers.IncludesExpression('200 OK', expectation) + tr.StillRunningAfter = self._ts + tr.StillRunningAfter = self._server + return tr + + def _configure_baseline_request(self) -> None: + '''Verify the valid initial configuration serves requests.''' + tr = self._configure_curl_run("Baseline request is served before reload", 'baseline request should be served') + tr.Processes.Default.StartBefore(self._server) + tr.Processes.Default.StartBefore(self._ts) + + def _configure_bad_remap_install(self) -> None: + '''Replace remap.config with one containing a bad nested run-plugin.''' + tr = Test.AddTestRun("Install a remap.config with a bad run-plugin") + remap_path = self._ts.Disk.remap_config.AbsPath + tr.Disk.File(remap_path, id="remap_bad", typename="ats:config") + tr.Disk.remap_bad.AddLine( + f'map http://reload.example.com http://127.0.0.1:{self._server.Variables.Port} ' + '@plugin=header_rewrite.so @pparam=nested_bad_run_plugin.conf') + tr.Processes.Default.Command = 'echo installed bad remap.config' + tr.Processes.Default.ReturnCode = 0 + tr.Processes.Default.Env = self._ts.Env + tr.StillRunningAfter = self._ts + tr.StillRunningAfter = self._server + + def _configure_failed_reload(self) -> None: + '''Verify the bad remap table is rejected without stopping ATS.''' + tr = Test.AddConfigReload( + self._ts, expect="fail", delay_start=2, description="Reload with bad run-plugin must be rejected, not fatal") + tr.StillRunningAfter = self._ts + tr.StillRunningAfter = self._server + + def _configure_post_reload_request(self) -> None: + '''Verify the rejected reload leaves the old configuration active.''' + self._configure_curl_run( + "Server still serves the old config after the rejected reload", 'old config should still serve after a rejected reload') + + +TestBadRunPlugin() From 25a5858ab3f02d348279e0e001c6bc050b84eff2 Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Wed, 5 Aug 2026 17:00:42 -0500 Subject: [PATCH 10/10] ocsp: use Bravo lock for cached responses (#13490) OCSP stapling serializes TLS handshake readers with refresh scans on a per-certificate mutex. Busy certificates therefore pay unnecessary lock contention on the handshake hot path. This patch uses the annotated Bravo reader-writer lock so handshake and refresh readers can proceed concurrently while cache updates remain exclusive. It keeps prefetched state immutable and allocates the OpenSSL destination outside the shared lock. The cached response is copied once after its size is revalidated. Co-authored-by: Craig Taylor (cherry picked from commit 14ce04de82fde613b74f5f8b8c21a080983831f2) --- src/iocore/net/OCSPStapling.cc | 139 +++++++++++++++++++++------------ 1 file changed, 90 insertions(+), 49 deletions(-) diff --git a/src/iocore/net/OCSPStapling.cc b/src/iocore/net/OCSPStapling.cc index 3e98d75d79c..715e68d2f90 100644 --- a/src/iocore/net/OCSPStapling.cc +++ b/src/iocore/net/OCSPStapling.cc @@ -22,6 +22,7 @@ #include "P_OCSPStapling.h" #include +#include #include #include @@ -38,6 +39,7 @@ #include "P_SSLUtils.h" #include "SSLStats.h" #include "proxy/FetchSM.h" +#include "tsutil/Bravo.h" // Macros for ASN1 and the code in TS_OCSP_* functions were borrowed from OpenSSL 3.1.0 (a92271e03a8d0dee507b6f1e7f49512568b2c7ad), // and were modified to make them compilable with BoringSSL and C++ compiler. @@ -282,18 +284,21 @@ namespace // Cached info stored in SSL_CTX ex_info struct certinfo { unsigned char idx[20] = {}; // Index in session cache SHA1 hash of certificate - TS_OCSP_CERTID *cid = nullptr; // Certificate ID for OCSP requests + TS_OCSP_CERTID *cid = nullptr; // Certificate ID for OCSP requests or nullptr if ID cannot be determined char *uri = nullptr; // Responder details char *certname = nullptr; char *user_agent = nullptr; - ink_mutex stapling_mutex; - unsigned char resp_der[MAX_STAPLING_DER] = {}; - unsigned int resp_derlen = 0; - bool is_prefetched = false; - bool is_expire = true; - time_t expire_time = 0; - - certinfo() { ink_mutex_init(&stapling_mutex); } + const bool is_prefetched; + + // OCSP response data, protected by resp_mutex. + // Readers take a shared lock; the updater takes an exclusive lock. + unsigned char resp_der[MAX_STAPLING_DER] = {}; + unsigned int resp_derlen = 0; + bool is_expire = true; + time_t expire_time = 0; + mutable ts::bravo::shared_mutex resp_mutex; + + explicit certinfo(bool is_prefetched) : is_prefetched(is_prefetched) {} ~certinfo() { if (cid) { @@ -304,7 +309,6 @@ struct certinfo { } ats_free(certname); ats_free(user_agent); - ink_mutex_destroy(&stapling_mutex); } certinfo(const certinfo &) = delete; @@ -847,12 +851,13 @@ stapling_cache_response(TS_OCSP_RESPONSE *rsp, certinfo *cinf) return false; } - ink_mutex_acquire(&cinf->stapling_mutex); - memcpy(cinf->resp_der, resp_der, resp_derlen); - cinf->resp_derlen = resp_derlen; - cinf->is_expire = false; - cinf->expire_time = time(nullptr) + SSLConfigParams::ssl_ocsp_cache_timeout; - ink_mutex_release(&cinf->stapling_mutex); + { + std::lock_guard lock(cinf->resp_mutex); + memcpy(cinf->resp_der, resp_der, resp_derlen); + cinf->resp_derlen = resp_derlen; + cinf->is_expire = false; + cinf->expire_time = time(nullptr) + SSLConfigParams::ssl_ocsp_cache_timeout; + } Dbg(dbg_ctl_ssl_ocsp, "stapling_cache_response: success to cache response"); return true; @@ -881,7 +886,7 @@ ssl_stapling_init_cert(SSL_CTX *ctx, X509 *cert, const char *certname, const cha map = new certinfo_map; map_is_new = true; } - auto cinf_ptr = std::make_unique(); + auto cinf_ptr = std::make_unique(rsp_file != nullptr); certinfo *cinf = cinf_ptr.get(); // Initialize certinfo @@ -889,8 +894,6 @@ ssl_stapling_init_cert(SSL_CTX *ctx, X509 *cert, const char *certname, const cha if (SSLConfigParams::ssl_ocsp_user_agent != nullptr) { cinf->user_agent = ats_strdup(SSLConfigParams::ssl_ocsp_user_agent); } - cinf->is_prefetched = rsp_file ? true : false; - if (cinf->is_prefetched) { Dbg(dbg_ctl_ssl_ocsp, "using OCSP prefetched response file %s", rsp_file); FILE *fp = fopen(rsp_file, "r"); @@ -1330,11 +1333,14 @@ ocsp_update() if (map) { // Walk over all certs associated with this CTX for (auto &iter : *map) { - cinf = iter.second.get(); - ink_mutex_acquire(&cinf->stapling_mutex); + cinf = iter.second.get(); current_time = time(nullptr); - if (cinf->resp_derlen == 0 || cinf->is_expire || cinf->expire_time < current_time) { - ink_mutex_release(&cinf->stapling_mutex); + bool needs_refresh; + { + ts::bravo::shared_lock lock(cinf->resp_mutex); + needs_refresh = cinf->resp_derlen == 0 || cinf->is_expire || cinf->expire_time < current_time; + } + if (needs_refresh) { if (stapling_refresh_response(cinf, &resp)) { Dbg(dbg_ctl_ssl_ocsp, "Successfully refreshed OCSP for %s certificate. url=%s", cinf->certname, cinf->uri); Metrics::Counter::increment(ssl_rsb.ocsp_refreshed_cert); @@ -1342,8 +1348,6 @@ ocsp_update() Error("Failed to refresh OCSP for %s certificate. url=%s", cinf->certname, cinf->uri); Metrics::Counter::increment(ssl_rsb.ocsp_refresh_cert_failure); } - } else { - ink_mutex_release(&cinf->stapling_mutex); } } } @@ -1419,37 +1423,74 @@ ssl_callback_ocsp_stapling(SSL *ssl, void *) return SSL_TLSEXT_ERR_NOACK; } - ink_mutex_acquire(&cinf->stapling_mutex); - time_t current_time = time(nullptr); - if ((cinf->resp_derlen == 0 || cinf->is_expire) || (cinf->expire_time < current_time && !cinf->is_prefetched)) { - ink_mutex_release(&cinf->stapling_mutex); - SiteThrottledError("ssl_callback_ocsp_stapling: failed to get certificate status for %s", cinf->certname); - return SSL_TLSEXT_ERR_NOACK; - } else { #ifdef OPENSSL_IS_BORINGSSL + int set_ok; + { + ts::bravo::shared_lock lock(cinf->resp_mutex); + + time_t current_time = time(nullptr); + if (cinf->resp_derlen == 0 || cinf->is_expire || (cinf->expire_time < current_time && !cinf->is_prefetched)) { + SiteThrottledError("ssl_callback_ocsp_stapling: failed to get certificate status for %s", cinf->certname); + return SSL_TLSEXT_ERR_NOACK; + } + // SSL_set_ocsp_response copies the response, so hand it the cached buffer directly. - int set_ok = SSL_set_ocsp_response(ssl, cinf->resp_der, cinf->resp_derlen); - ink_mutex_release(&cinf->stapling_mutex); + set_ok = SSL_set_ocsp_response(ssl, cinf->resp_der, cinf->resp_derlen); + } #else - unsigned char *p = static_cast(OPENSSL_malloc(cinf->resp_derlen)); - if (p == nullptr) { - ink_mutex_release(&cinf->stapling_mutex); - Dbg(dbg_ctl_ssl_ocsp, "ssl_callback_ocsp_stapling: failed to allocate memory for %s", cinf->certname); - return SSL_TLSEXT_ERR_NOACK; + unsigned char *p = nullptr; + unsigned int resp_capacity = 0; + unsigned int resp_derlen; + + while (true) { + unsigned int required_capacity; + bool is_response_available; + { + ts::bravo::shared_lock lock(cinf->resp_mutex); + + time_t current_time = time(nullptr); + is_response_available = + cinf->resp_derlen != 0 && !cinf->is_expire && (cinf->expire_time >= current_time || cinf->is_prefetched); + + if (is_response_available) { + resp_derlen = cinf->resp_derlen; + if (resp_derlen <= resp_capacity) { + memcpy(p, cinf->resp_der, resp_derlen); + break; + } + required_capacity = resp_derlen; + } } - memcpy(p, cinf->resp_der, cinf->resp_derlen); - ink_mutex_release(&cinf->stapling_mutex); - // Takes ownership of p and frees it on success; on failure it does not. - int set_ok = SSL_set_tlsext_status_ocsp_resp(ssl, p, cinf->resp_derlen); - if (set_ok == 0) { + + if (!is_response_available) { OPENSSL_free(p); + SiteThrottledError("ssl_callback_ocsp_stapling: failed to get certificate status for %s", cinf->certname); + return SSL_TLSEXT_ERR_NOACK; } -#endif - if (set_ok == 0) { + + unsigned char *new_p = static_cast(OPENSSL_malloc(required_capacity)); + if (new_p == nullptr) { + OPENSSL_free(p); + Dbg(dbg_ctl_ssl_ocsp, "ssl_callback_ocsp_stapling: failed to allocate memory for %s", cinf->certname); return SSL_TLSEXT_ERR_NOACK; } - Dbg(dbg_ctl_ssl_ocsp, "ssl_callback_ocsp_stapling: successfully got certificate status for %s", cinf->certname); - Dbg(dbg_ctl_ssl_ocsp, "is_prefetched:%d uri:%s", cinf->is_prefetched, cinf->uri); - return SSL_TLSEXT_ERR_OK; + OPENSSL_free(p); + p = new_p; + resp_capacity = required_capacity; } + + // Takes ownership of p and frees it on success; on failure it does not. + int set_ok = SSL_set_tlsext_status_ocsp_resp(ssl, p, resp_derlen); + if (set_ok == 0) { + OPENSSL_free(p); + } +#endif + + if (set_ok == 0) { + return SSL_TLSEXT_ERR_NOACK; + } + + Dbg(dbg_ctl_ssl_ocsp, "ssl_callback_ocsp_stapling: successfully got certificate status for %s", cinf->certname); + Dbg(dbg_ctl_ssl_ocsp, "is_prefetched:%d uri:%s", cinf->is_prefetched, cinf->uri); + return SSL_TLSEXT_ERR_OK; }