bryancall commented on code in PR #13711:
URL: https://github.com/apache/trafficserver/pull/13711#discussion_r4076587447
##########
src/iocore/net/P_SSLConfig.h:
##########
@@ -73,6 +73,7 @@ struct SSLConfigParams : public ConfigInfo {
int verify_depth;
int ssl_origin_session_cache{0};
int ssl_origin_session_cache_size{0};
+ int ssl_origin_session_max_size{0};
Review Comment:
**Fixing**, in the next push — same change as the duplicate of this thread
above. Storing it unsigned and clamping at the boundary, which composes with
@bneradt's P2 finding about the environment-variable path bypassing `RECC_INT`.
##########
tests/gold_tests/tls/tls_origin_session_reuse_large.test.py:
##########
@@ -0,0 +1,145 @@
+'''
+'''
+# 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 = '''
+Origin TLS sessions must still be cached when the serialized session is large.
+
+The origin session cache refuses to store a session whose i2d_SSL_SESSION form
+exceeds SSL_MAX_ORIG_SESSION_SIZE. A serialized session carries the peer
+certificate and the session ticket, so an origin with a large certificate -- or
+any mutual-TLS origin, whose ticket encodes the client certificate -- produces
a
Review Comment:
**Fixing**, in the next push. Fair — the summary still describes a fixed
`SSL_MAX_ORIG_SESSION_SIZE` that this PR removes. It will describe the default
ceiling versus a configured one.
##########
src/iocore/net/SSLConfig.cc:
##########
@@ -463,9 +464,12 @@ SSLConfigParams::initialize(ConfigContext ctx)
// SSL session cache configurations
ssl_origin_session_cache =
RecGetRecordInt("proxy.config.ssl.origin_session_cache.enabled").value_or(0);
ssl_origin_session_cache_size =
RecGetRecordInt("proxy.config.ssl.origin_session_cache.size").value_or(0);
+ ssl_origin_session_max_size =
+
RecGetRecordInt("proxy.config.ssl.origin_session_cache.max_session_size").value_or(SSL_DEFAULT_MAX_ORIG_SESSION_SIZE);
Review Comment:
**Fixing**, in the next push — and thank you, this is the real one. The
upper bound is not cosmetic here: `insert_session()` checks the size
immediately before `SSLSessionDup()`, whose portable path `alloca()`s a buffer
of exactly that length, so a value that escapes `RECC_INT` defeats the only
thing keeping that allocation on the stack. `-1` becoming `SIZE_MAX` is the bad
case. I will clamp at the assignment in `SSLConfig.cc` rather than trusting the
record layer, and log when a configured value is clamped.
##########
doc/admin-guide/files/records.yaml.en.rst:
##########
@@ -4675,6 +4675,29 @@ SSL Termination
Setting a value less than or equal to ``0`` effectively disables
SSL session cache for the origin server.
+.. ts:cv:: CONFIG proxy.config.ssl.origin_session_cache.max_session_size INT
8192
+
+ The largest origin session |TS| will place in the origin session cache,
measured
+ as the size in bytes of its ASN.1 form. A session over this size is not
cached,
+ and every connection to that origin performs a full handshake.
+
+ A serialized session carries the origin's certificate and the session ticket
the
+ origin issued, so the size is set by the origin, not by |TS|. Two common
cases
+ run large: an origin with a big certificate, and any mutual-TLS origin,
because a
+ stateless ticket has to encode the client certificate |TS| presented in
order to
+ resume the authenticated session. The default accommodates both.
+
+ The accepted range is 4096 to 65536. The floor is the size this was fixed
at before
+ it became configurable, so no setting can cache less than |TS| always did;
disable the
+ cache with :ts:cv:`proxy.config.ssl.origin_session_cache.enabled` instead.
The ceiling
+ keeps the serialization buffer in ``SSLSessionDup()`` within the thread
stack.
+
+ Raise this if :ts:stat:`proxy.process.ssl.ssl_origin_session_cache_hit`
stays at
+ zero while :ts:stat:`proxy.process.ssl.ssl_origin_session_cache_miss`
climbs; the
+ ``ssl.origin_session_cache`` debug tag reports each session refused for its
size.
+ Note that the cache holds up to
:ts:cv:`proxy.config.ssl.origin_session_cache.size`
+ entries, so this value bounds the memory that cache can occupy.
Review Comment:
**Fixing**, in the next push. Agreed the sentence overclaims — what is
bounded is the serialized ASN.1 size accepted for insertion and the duplication
buffer sized from it, not the in-memory footprint of the cached `SSL_SESSION`
objects. Rewording to say that, and to treat the memory implication as
approximate.
##########
src/iocore/net/SSLConfig.cc:
##########
@@ -70,6 +70,7 @@ char *SSLConfigParams::ssl_ocsp_user_agent
= nullptr;
int SSLConfigParams::ssl_handshake_timeout_in = 0;
int SSLConfigParams::origin_session_cache = 1;
size_t SSLConfigParams::origin_session_cache_size = 10240;
+size_t SSLConfigParams::origin_session_max_size =
SSL_DEFAULT_MAX_ORIG_SESSION_SIZE;
Review Comment:
**Fixing**, in the next push. Reasonable — the default belongs with the
config that owns it. Moving it to a `static constexpr size_t` on
`SSLConfigParams`, which also removes the `SSLSessionCache.h` include
dependency from `SSLConfig.cc`.
##########
tests/gold_tests/tls/tls_origin_session_reuse_large.test.py:
##########
@@ -0,0 +1,143 @@
+# 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 = '''
+Origin TLS sessions must still be cached when the serialized session is large.
+
+The origin session cache refuses to store a session whose i2d_SSL_SESSION form
+exceeds SSL_MAX_ORIG_SESSION_SIZE. A serialized session carries the peer
+certificate and the session ticket, so an origin with a large certificate -- or
+any mutual-TLS origin, whose ticket encodes the client certificate -- produces
a
+session that trips that limit and is silently dropped, costing a full handshake
+on every connection.
+'''
+
+# ts_origin presents a deliberately large certificate (120 SANs, 4328 bytes
DER),
+# so the session ts_proxy caches for it does not fit in a small fixed buffer.
+ts_origin = Test.MakeATSProcess("ts_origin", enable_tls=True)
+ts_proxy = Test.MakeATSProcess("ts_proxy", enable_tls=True)
+server = Test.MakeOriginServer("server")
+
+request_header = {"headers": "GET / HTTP/1.1\r\nHost:
www.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": "large session test"
+}
+server.addResponse("sessionlog.json", request_header, response_header)
+
+ts_origin.addSSLfile("ssl/server-large.pem")
+ts_origin.addSSLfile("ssl/server-large.key")
+ts_proxy.addSSLfile("ssl/server.pem")
+ts_proxy.addSSLfile("ssl/server.key")
+
Review Comment:
**Fixing**, in the next push. Will restructure it on
`tests/gold_tests/ats_probe/ats_probe.test.py` — thanks for the pointer to a
current example.
##########
src/iocore/net/SSLSessionCache.h:
##########
@@ -40,9 +40,16 @@
* surprised and this turns out to be too small, there is an assertion gaurd to
Review Comment:
**Fixing**, in the next push. Pre-existing typo in the comment above the
constant I touched, but it is one word and I am already in that block.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
