Copilot commented on code in PR #13711:
URL: https://github.com/apache/trafficserver/pull/13711#discussion_r4074912592
##########
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:
The new `ssl_origin_session_max_size` is stored as `int`, but it represents
a byte size and ultimately feeds a `size_t` static (`origin_session_max_size`).
For consistency and to avoid signed/unsigned friction, store it as `size_t` (or
the same integral type used by `RecGetRecordInt()`), and cast only at the
boundary if needed.
##########
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:
`SSL_DEFAULT_MAX_ORIG_SESSION_SIZE` is defined in `SSLSessionCache.h` but
used here in `SSLConfig.cc`, which creates a cross-module macro dependency.
Prefer defining the default as a `static constexpr size_t` on `SSLConfigParams`
(or in a config-focused header) to keep ownership with config and reduce
include coupling.
##########
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:
This sentence is likely too strong: the setting bounds the *serialized ASN.1
size accepted for insertion* (and any duplication/serialization buffer sized
off it), but the in-memory footprint of cached `SSL_SESSION` objects may not be
strictly bounded by this value. Consider rewording to clarify what is bounded
(e.g., serialized size / duplication buffer) and treat memory footprint as
approximate rather than guaranteed.
##########
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:
Correct spelling: change 'gaurd' to 'guard'.
--
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]
