maskit commented on code in PR #13949:
URL: https://github.com/apache/trafficserver/pull/13949#discussion_r4461789852
##########
include/iocore/net/TLSSNISupport.h:
##########
@@ -106,6 +106,10 @@ class TLSSNISupport
*/
void on_client_hello(ClientHello &client_hello);
+ /** Forget the CLIENT_HELLO passed to on_client_hello(), which is only valid
during the ClientHello callback.
+ */
+ void clear_client_hello();
Review Comment:
With `_ch` cleared, `TSVConnClientHelloGet()` returns an unavailable object
outside the ClientHello hook, which is the point of the fix. But
`doc/developer-guide/api/types/TSClientHello.en.rst` still says of
`is_available()`: "As long as `TSVConnClientHelloGet` is called for a TLS
connection, the return value should be true." Could that sentence say "called
from the `TS_EVENT_SSL_CLIENT_HELLO` hook" instead?
##########
src/iocore/net/SSLNetVConnection.cc:
##########
@@ -1789,6 +1790,14 @@ SSLNetVConnection::reenable(int event)
sslHandshakeStatus = SSLHandshakeStatus::SSL_HANDSHAKE_ERROR;
}
+ // The ClientHello is only readable during the TLS library's ClientHello
callback, which
+ // the resumed handshake calls again to run the remaining ClientHello hooks.
+ if (this->get_handshake_hook_state() ==
TLSEventSupport::SSLHandshakeHookState::HANDSHAKE_HOOKS_CLIENT_HELLO_INVOKE) {
Review Comment:
The older comment below raises a concern that now applies to this branch.
OpenSSL reads `client_hello_cb` from the SSL's current `SSL_CTX` and doesn't
fall back to `session_ctx`. Only the default (`*`) context gets
`ssl_client_hello_callback` (`_set_handshake_callbacks()` in `SSLUtils.cc`).
`TSSslContextFindByName()` returns the other contexts, which don't have it.
So suppose a delayed ClientHello hook calls `SSL_set_SSL_CTX()` with one of
those contexts before reenabling. The callback isn't called again, and the
remaining ClientHello hooks are silently skipped. Before this change,
`reenable()` ran them.
On master, this pattern already loses later stages: on BoringSSL the
servername and cert hooks are skipped, and on OpenSSL the cert callback is. So
I don't think code needs to handle it. A note in the `TS_SSL_CLIENT_HELLO_HOOK`
docs saying that switching the context belongs in the cert hook would probably
be enough. What do you think?
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -256,28 +257,37 @@ ssl_client_hello_callback(const SSL_CLIENT_HELLO
*client_hello)
}
TLSSNISupport *snis = TLSSNISupport::getInstance(s);
- if (snis) {
- snis->on_client_hello(ch);
- int ret = snis->perform_sni_action(*s);
- if (ret != SSL_TLSEXT_ERR_OK) {
- return CLIENT_HELLO_ERROR;
- }
- } else {
+ if (snis == nullptr) {
// This error suggests either of these:
// 1) Call back on unsupported netvc -- Don't register callback
unnecessarily
// 2) Call back on stale netvc
Dbg(dbg_ctl_ssl_error, "ssl_client_hello_callback was called
unexpectedly");
return CLIENT_HELLO_ERROR;
}
+ snis->on_client_hello(ch);
+ ts::PostScript clear_client_hello([snis]() -> void {
snis->clear_client_hello(); });
+
+ // When a delayed ClientHello hook reenables, the TLS library calls back
again for the remaining
+ // hooks, and the SNI actions have already run.
+ if (es == nullptr || !es->resuming_client_hello_hooks()) {
+ int ret = snis->perform_sni_action(*s);
+ if (ret != SSL_TLSEXT_ERR_OK) {
+ return CLIENT_HELLO_ERROR;
+ }
+ }
+
if (es) {
if (es->finished_client_hello_hooks()) {
return CLIENT_HELLO_SUCCESS;
}
- bool reenabled = es->callHooks(TS_EVENT_SSL_CLIENT_HELLO);
- if (!reenabled) {
- return CLIENT_HELLO_RETRY;
- }
+ // Hooks that reenable from within their invocation leave the rest to run
here.
+ do {
+ bool reenabled = es->callHooks(TS_EVENT_SSL_CLIENT_HELLO);
+ if (!reenabled) {
+ return CLIENT_HELLO_RETRY;
+ }
+ } while (es->resuming_client_hello_hooks());
Review Comment:
This also fixes something the description doesn't mention. On master, with
OpenSSL, `bind_session_id_context_to_verify_scope()` never runs when any
ClientHello hook pauses. The first callback returns `CLIENT_HELLO_RETRY` before
reaching it. On re-entry, `reenable()` has already walked the hooks, so
`finished_client_hello_hooks()` is true and the early return at the top of this
function skips the bind. The fallback in `ssl_servername_callback` is compiled
out under `TS_USE_HELLO_CB`. With this change, re-entry runs `callHooks` and
reaches the bind.
BoringSSL is unaffected, because the bind sits at the tail of the
select-certificate lambda, which re-runs in full. Could the description mention
this? It bears on backporting.
##########
src/iocore/net/SSLNetVConnection.cc:
##########
@@ -1789,6 +1790,14 @@ SSLNetVConnection::reenable(int event)
sslHandshakeStatus = SSLHandshakeStatus::SSL_HANDSHAKE_ERROR;
}
+ // The ClientHello is only readable during the TLS library's ClientHello
callback, which
+ // the resumed handshake calls again to run the remaining ClientHello hooks.
+ if (this->get_handshake_hook_state() ==
TLSEventSupport::SSLHandshakeHookState::HANDSHAKE_HOOKS_CLIENT_HELLO_INVOKE) {
+ this->resume_tls_event();
+ this->readReschedule(nh);
+ return;
+ }
+
this->resume_tls_event();
// Reenabling from the handshake callback
Review Comment:
This comment now contradicts the new early return just above it. It says we
walk the rest of the hooks here because the callback might not be called again,
while the ClientHello stage now relies on exactly that call. Could it be
limited to the other stages?
##########
src/iocore/net/TLSEventSupport.cc:
##########
@@ -451,6 +451,19 @@ TLSEventSupport::finished_client_hello_hooks() const
}
}
+/** Whether a ClientHello hook has reenabled the connection with ClientHello
hooks left to run.
Review Comment:
"with ClientHello hooks left to run" isn't quite right when the last
ClientHello hook pauses. The predicate is still true there, and `callHooks`
then advances to the end of the list and moves on to SNI. The behavior is
correct; only the wording overstates. Maybe "Whether a ClientHello hook has
reenabled the connection and the ClientHello callback must continue the
ClientHello hook list"?
##########
tests/uranium_tests/tls_hooks/client_hello_reader.replay.yaml:
##########
@@ -0,0 +1,48 @@
+# 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
Review Comment:
The license header is missing its last line, `# limitations under the
License.`
--
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]
