Copilot commented on code in PR #13202:
URL: https://github.com/apache/trafficserver/pull/13202#discussion_r3352278545
##########
src/iocore/net/SSLNetVConnection.cc:
##########
@@ -123,11 +123,26 @@ SSLNetVConnection::_make_ssl_connection(SSL_CTX *ctx)
SSL_set_bio(ssl, bio, bio);
} else {
- this->initialize_handshake_buffers();
- BIO *rbio = BIO_new(BIO_s_mem());
- BIO *wbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
- BIO_set_mem_eof_return(wbio, -1);
- SSL_set_bio(ssl, rbio, wbio);
+ if (this->transparentPassThrough || this->allowPlain) {
+ // Blind-tunnel possible: keep handshake buffers to capture/replay the
CLIENT_HELLO.
+ this->initialize_handshake_buffers();
+ BIO *rbio = BIO_new(BIO_s_mem());
+ BIO *wbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
+ BIO_set_mem_eof_return(wbio, -1);
+ SSL_set_bio(ssl, rbio, wbio);
+ } else {
+ // No blind-tunnel: direct socket BIO, skip handshake buffers. On
OpenSSL a
+ // write-side buffer BIO coalesces TLS records into fewer write()
syscalls;
+ // BoringSSL has no filter BIOs, so it uses the plain socket BIO.
+ BIO *rbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
+ BIO *wbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
+#if !defined(OPENSSL_IS_BORINGSSL)
+ BIO *buf_wbio = BIO_new(BIO_f_buffer());
+ BIO_set_write_buffer_size(buf_wbio, 65536);
+ wbio = BIO_push(buf_wbio, wbio);
+#endif
Review Comment:
BIO_new(BIO_f_buffer()) / BIO_push() results are not checked for failure. If
allocation or push fails, buf_wbio may be nullptr (crash on
BIO_set_write_buffer_size) or BIO_push may return nullptr, leaving wbio invalid.
##########
src/iocore/net/SSLNetVConnection.cc:
##########
@@ -123,11 +123,26 @@ SSLNetVConnection::_make_ssl_connection(SSL_CTX *ctx)
SSL_set_bio(ssl, bio, bio);
} else {
- this->initialize_handshake_buffers();
- BIO *rbio = BIO_new(BIO_s_mem());
- BIO *wbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
- BIO_set_mem_eof_return(wbio, -1);
- SSL_set_bio(ssl, rbio, wbio);
+ if (this->transparentPassThrough || this->allowPlain) {
+ // Blind-tunnel possible: keep handshake buffers to capture/replay the
CLIENT_HELLO.
+ this->initialize_handshake_buffers();
+ BIO *rbio = BIO_new(BIO_s_mem());
+ BIO *wbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
+ BIO_set_mem_eof_return(wbio, -1);
+ SSL_set_bio(ssl, rbio, wbio);
+ } else {
+ // No blind-tunnel: direct socket BIO, skip handshake buffers. On
OpenSSL a
+ // write-side buffer BIO coalesces TLS records into fewer write()
syscalls;
+ // BoringSSL has no filter BIOs, so it uses the plain socket BIO.
+ BIO *rbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
+ BIO *wbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
+#if !defined(OPENSSL_IS_BORINGSSL)
+ BIO *buf_wbio = BIO_new(BIO_f_buffer());
+ BIO_set_write_buffer_size(buf_wbio, 65536);
+ wbio = BIO_push(buf_wbio, wbio);
+#endif
+ SSL_set_bio(ssl, rbio, wbio);
+ }
Review Comment:
A buffered filter BIO is pushed onto the SSL wbio before the TLS handshake
runs, but the handshake path (_ssl_accept / _sslStartHandShake) never calls
BIO_flush. Because BIO_f_buffer can retain handshake records until flush /
buffer-full, SSL_accept() may return WANT_READ while the ServerHello/etc are
still stuck in the buffer, potentially stalling the handshake.
##########
src/iocore/net/SSLNetVConnection.cc:
##########
@@ -123,11 +123,26 @@ SSLNetVConnection::_make_ssl_connection(SSL_CTX *ctx)
SSL_set_bio(ssl, bio, bio);
} else {
- this->initialize_handshake_buffers();
- BIO *rbio = BIO_new(BIO_s_mem());
- BIO *wbio = BIO_new_socket(this->get_socket(), BIO_NOCLOSE);
- BIO_set_mem_eof_return(wbio, -1);
- SSL_set_bio(ssl, rbio, wbio);
+ if (this->transparentPassThrough || this->allowPlain) {
+ // Blind-tunnel possible: keep handshake buffers to capture/replay the
CLIENT_HELLO.
+ this->initialize_handshake_buffers();
Review Comment:
This change skips handshake buffers unless transparent passthrough /
allow-plain is enabled, but SSLNetVConnection::read_raw_data() (used during
CLIENT_HELLO buffering) is also where PROXY protocol is parsed and the
allowlist is enforced. With handshake buffers disabled, the PROXY header will
be consumed by OpenSSL as TLS bytes and proxy protocol will no longer work on
normal TLS ports.
##########
src/iocore/net/SSLNetVConnection.cc:
##########
@@ -796,6 +811,14 @@ SSLNetVConnection::load_buffer_and_write(int64_t towrite,
MIOBufferAccessor &buf
Metrics::Counter::increment(net_rsb.calls_to_write);
} while (num_really_written == try_to_write && total_written < towrite);
+ // Flush the buffer BIO once here, not per-record, so a response coalesces
into
+ // one write(). No-op on a plain socket BIO.
+ if (total_written > 0) {
+ if (BIO *wbio = SSL_get_wbio(ssl); wbio != nullptr) {
+ (void)BIO_flush(wbio);
+ }
+ }
Review Comment:
BIO_flush() return value is ignored. With a write-side buffer BIO, flush can
fail / retry (e.g. EAGAIN) leaving encrypted bytes pending in the BIO. If the
application buffer becomes empty, UnixNetVConnection::write_to_net() will
disable further write events, so the pending BIO output may never be flushed.
--
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]
