Cleanup: Remove server name stuff from NetVConnection (#11745)

* Cleanup: Remove server name stuff from NetVConnection

* Fix a bug

* Maybe it's nullptr vs empty string

* Another try

* Add another empty string check

include/iocore/net/NetVConnection.h

@@ -163,15 +163,6 @@ class NetVConnection : public VConnection, public PluginUserArgs<TS_USER_ARGS_VC
   */
   void do_io_shutdown(ShutdownHowTo_t howto) override = 0;
 
-  /**
-    Return the server name that is appropriate for the network VC type
-  */
-  virtual const char *
-  get_server_name() const
-  {
-    return nullptr;
-  }
-
   ////////////////////////////////////////////////////////////
   // Set the timeouts associated with this connection.      //
   // active_timeout is for the total elapsed time of        //
@@ -327,22 +318,6 @@ class NetVConnection : public VConnection, public PluginUserArgs<TS_USER_ARGS_VC
     return netvc_context;
   }
 
-  /**
-   * Returns true if the network protocol
-   * supports a client provided SNI value
-   */
-  virtual bool
-  support_sni() const
-  {
-    return false;
-  }
-
-  virtual const char *
-  get_sni_servername() const
-  {
-    return nullptr;
-  }
-
   virtual bool
   peer_provided_cert() const
   {

include/iocore/net/TLSSNISupport.h

@@ -61,6 +61,11 @@ class TLSSNISupport
   void on_client_hello(ClientHello &client_hello);
   void on_servername(SSL *ssl, int *al, void *arg);
 
+  /**
+   * Get the server name in SNI
+   *
+   * @return Either a pointer to the (null-terminated) name fetched from the TLS object or the empty string.
+   */
   const char *get_sni_server_name() const;
   bool        would_have_actions_for(const char *servername, IpEndpoint remote, int &enforcement_policy);
 

src/api/InkAPI.cc

@@ -7928,17 +7928,17 @@ TSVConnFdGet(TSVConn vconnp)
 const char *
 TSVConnSslSniGet(TSVConn sslp, int *length)
 {
-  char const     *server_name = nullptr;
-  NetVConnection *vc          = reinterpret_cast<NetVConnection *>(sslp);
-
-  if (vc == nullptr) {
+  if (sslp == nullptr) {
     return nullptr;
   }
 
-  server_name = vc->get_server_name();
-
-  if (length) {
-    *length = server_name ? strlen(server_name) : 0;
+  char const     *server_name = nullptr;
+  NetVConnection *vc          = reinterpret_cast<NetVConnection *>(sslp);
+  if (auto snis = vc->get_service<TLSSNISupport>(); snis) {
+    server_name = snis->get_sni_server_name();
+    if (length) {
+      *length = server_name ? strlen(server_name) : 0;
+    }
   }
 
   return server_name;

src/iocore/net/CMakeLists.txt

@@ -118,14 +118,8 @@ endif()
 
 if(BUILD_TESTING)
   add_executable(
-    test_net
-    libinknet_stub.cc
-    NetVCTest.cc
-    unit_tests/test_ProxyProtocol.cc
-    unit_tests/test_SSLSNIConfig.cc
-    unit_tests/test_YamlSNIConfig.cc
-    unit_tests/unit_test_main.cc
-    unit_tests/test_Net.cc
+    test_net libinknet_stub.cc NetVCTest.cc unit_tests/test_ProxyProtocol.cc unit_tests/test_SSLSNIConfig.cc
+             unit_tests/test_YamlSNIConfig.cc unit_tests/unit_test_main.cc
   )
   target_link_libraries(test_net PRIVATE ts::inknet catch2::catch2)
   set(LIBINKNET_UNIT_TEST_DIR "${CMAKE_SOURCE_DIR}/src/iocore/net/unit_tests")

src/iocore/net/P_QUICNetVConnection.h

@@ -110,8 +110,6 @@ class QUICNetVConnection : public UnixNetVConnection,
   // NetVConnection
   int         populate_protocol(std::string_view *results, int n) const override;
   const char *protocol_contains(std::string_view tag) const override;
-  const char *get_server_name() const override;
-  bool        support_sni() const override;
 
   // QUICConnection
   QUICStreamManager *stream_manager() override;

src/iocore/net/P_SSLNetVConnection.h

@@ -317,20 +317,6 @@ class SSLNetVConnection : public UnixNetVConnection,
 
   std::shared_ptr<SSL_SESSION> client_sess = nullptr;
 
-  // The serverName is either a pointer to the (null-terminated) name fetched from the
-  // SSL object or the empty string.
-  const char *
-  get_server_name() const override
-  {
-    return get_sni_server_name() ? get_sni_server_name() : "";
-  }
-
-  bool
-  support_sni() const override
-  {
-    return true;
-  }
-
   /// Set by asynchronous hooks to request a specific operation.
   SslVConnOp hookOpRequested = SSL_HOOK_OP_DEFAULT;
 
@@ -356,12 +342,6 @@ class SSLNetVConnection : public UnixNetVConnection,
     verify_cert = ctx;
   }
 
-  const char *
-  get_sni_servername() const override
-  {
-    return SSL_get_servername(this->ssl, TLSEXT_NAMETYPE_host_name);
-  }
-
   bool
   peer_provided_cert() const override
   {

src/iocore/net/P_UnixNetVConnection.h

@@ -60,12 +60,6 @@ class UnixNetVConnection : public NetVConnection, public NetEvent
 
   bool get_data(int id, void *data) override;
 
-  const char *
-  get_server_name() const override
-  {
-    return nullptr;
-  }
-
   void do_io_close(int lerrno = -1) override;
   void do_io_shutdown(ShutdownHowTo_t howto) override;
 

src/iocore/net/QUICNetVConnection.cc

@@ -755,18 +755,6 @@ QUICNetVConnection::protocol_contains(std::string_view prefix) const
   return retval;
 }
 
-const char *
-QUICNetVConnection::get_server_name() const
-{
-  return get_sni_server_name();
-}
-
-bool
-QUICNetVConnection::support_sni() const
-{
-  return true;
-}
-
 QUICConnection *
 QUICNetVConnection::get_quic_connection()
 {

src/proxy/ProxySession.cc

@@ -335,7 +335,11 @@ ProxySession::reenable(VIO *vio)
 bool
 ProxySession::support_sni() const
 {
-  return _vc ? _vc->support_sni() : false;
+  if (this->_vc) {
+    return this->_vc->get_service<TLSSNISupport>() != nullptr;
+  } else {
+    return false;
+  }
 }
 
 PoolableSession *

src/proxy/http/HttpSM.cc

@@ -501,7 +501,11 @@ HttpSM::setup_blind_tunnel_port()
           t_state.hdr_info.client_request.url_get()->port_set(netvc->get_local_port());
         }
       } else {
-        t_state.hdr_info.client_request.url_get()->host_set(netvc->get_server_name(), strlen(netvc->get_server_name()));
+        const char *server_name = "";
+        if (auto *snis = netvc->get_service<TLSSNISupport>(); snis) {
+          server_name = snis->get_sni_server_name();
+        }
+        t_state.hdr_info.client_request.url_get()->host_set(server_name, strlen(server_name));
         t_state.hdr_info.client_request.url_get()->port_set(netvc->get_local_port());
       }
     }
@@ -1375,7 +1379,11 @@ plugins required to work with sni_routing.
             t_state.hdr_info.client_request.url_get()->port_set(netvc->get_local_port());
           }
         } else {
-          t_state.hdr_info.client_request.url_get()->host_set(netvc->get_server_name(), strlen(netvc->get_server_name()));
+          const char *server_name = "";
+          if (auto *snis = netvc->get_service<TLSSNISupport>(); snis) {
+            server_name = snis->get_sni_server_name();
+          }
+          t_state.hdr_info.client_request.url_get()->host_set(server_name, strlen(server_name));
           t_state.hdr_info.client_request.url_get()->port_set(netvc->get_local_port());
         }
       }
@@ -4318,7 +4326,7 @@ HttpSM::check_sni_host()
     // In a SNI/Host mismatch where the Host would have triggered SNI policy, mark the transaction
     // to be considered for rejection after the remap phase passes.  Gives the opportunity to conf_remap
     // override the policy to be rejected in the end_remap logic
-    const char *sni_value    = netvc->get_server_name();
+    const char *sni_value    = snis->get_sni_server_name();
     const char *action_value = host_sni_policy == 2 ? "terminate" : "continue";
     if (!sni_value || sni_value[0] == '\0') { // No SNI
       Warning("No SNI for TLS request with hostname %.*s action=%s", host_len, host_name, action_value);
@@ -5132,9 +5140,11 @@ HttpSM::get_outbound_sni() const
   swoc::TextView zret;
   swoc::TextView policy{t_state.txn_conf->ssl_client_sni_policy, swoc::TextView::npos};
 
+  TLSSNISupport *snis = nullptr;
   if (_ua.get_txn()) {
     if (auto *netvc = _ua.get_txn()->get_netvc(); netvc) {
-      if (auto *snis = netvc->get_service<TLSSNISupport>(); snis && snis->hints_from_sni.outbound_sni_policy.has_value()) {
+      snis = netvc->get_service<TLSSNISupport>();
+      if (snis && snis->hints_from_sni.outbound_sni_policy.has_value()) {
         policy.assign(snis->hints_from_sni.outbound_sni_policy->data(), swoc::TextView::npos);
       }
     }
@@ -5146,7 +5156,12 @@ HttpSM::get_outbound_sni() const
     char const *ptr = t_state.hdr_info.server_request.host_get(&len);
     zret.assign(ptr, len);
   } else if (_ua.get_txn() && policy == "server_name"_tv) {
-    zret.assign(_ua.get_txn()->get_netvc()->get_server_name(), swoc::TextView::npos);
+    const char *server_name = snis->get_sni_server_name();
+    if (server_name[0] == '\0') {
+      zret.assign(nullptr, swoc::TextView::npos);
+    } else {
+      zret.assign(snis->get_sni_server_name(), swoc::TextView::npos);
+    }
   } else if (policy.front() == '@') { // guaranteed non-empty from previous clause
     zret = policy.remove_prefix(1);
   } else {

src/proxy/http/HttpSessionManager.cc

@@ -34,6 +34,7 @@
 #include "proxy/ProxySession.h"
 #include "proxy/http/HttpSM.h"
 #include "proxy/http/HttpDebugNames.h"
+#include "iocore/net/TLSSNISupport.h"
 #include <iterator>
 
 namespace
@@ -91,14 +92,18 @@ ServerSessionPool::validate_host_sni(HttpSM *sm, NetVConnection *netvc)
     // by fetching the hostname from the server request.  So the connection should only
     // be reused if the hostname in the new request is the same as the host name in the
     // original request
-    const char *session_sni = netvc->get_sni_servername();
-    if (session_sni) {
-      // TS-4468: If the connection matches, make sure the SNI server
-      // name (if present) matches the request hostname
-      int         len      = 0;
-      const char *req_host = sm->t_state.hdr_info.server_request.host_get(&len);
-      retval               = strncasecmp(session_sni, req_host, len) == 0;
-      Dbg(dbg_ctl_http_ss, "validate_host_sni host=%*.s, sni=%s", len, req_host, session_sni);
+    if (auto snis = netvc->get_service<TLSSNISupport>(); snis) {
+      const char *session_sni = snis->get_sni_server_name();
+      if (session_sni && session_sni[0] != '\0') {
+        // TS-4468: If the connection matches, make sure the SNI server
+        // name (if present) matches the request hostname
+        int         len      = 0;
+        const char *req_host = sm->t_state.hdr_info.server_request.host_get(&len);
+        retval               = strncasecmp(session_sni, req_host, len) == 0;
+        Dbg(dbg_ctl_http_ss, "validate_host_sni host=%*.s, sni=%s", len, req_host, session_sni);
+      }
+    } else {
+      retval = false;
     }
   }
   return retval;
@@ -112,14 +117,18 @@ ServerSessionPool::validate_sni(HttpSM *sm, NetVConnection *netvc)
   // a new connection.
   //
   if (sm->t_state.scheme == URL_WKSIDX_HTTPS) {
-    const char      *session_sni  = netvc->get_sni_servername();
-    std::string_view proposed_sni = sm->get_outbound_sni();
-    Dbg(dbg_ctl_http_ss, "validate_sni proposed_sni=%.*s, sni=%s", static_cast<int>(proposed_sni.length()), proposed_sni.data(),
-        session_sni);
-    if (!session_sni || proposed_sni.length() == 0) {
-      retval = session_sni == nullptr && proposed_sni.length() == 0;
+    if (auto snis = netvc->get_service<TLSSNISupport>(); snis) {
+      const char      *session_sni  = snis->get_sni_server_name();
+      std::string_view proposed_sni = sm->get_outbound_sni();
+      Dbg(dbg_ctl_http_ss, "validate_sni proposed_sni=%.*s, sni=%s", static_cast<int>(proposed_sni.length()), proposed_sni.data(),
+          session_sni);
+      if (!session_sni || session_sni[0] == '\0' || proposed_sni.length() == 0) {
+        retval = session_sni == nullptr && proposed_sni.length() == 0;
+      } else {
+        retval = proposed_sni.compare(session_sni) == 0;
+      }
     } else {
-      retval = proposed_sni.compare(session_sni) == 0;
+      retval = false;
     }
   }
   return retval;

src/proxy/private/SSLProxySession.cc

@@ -23,14 +23,15 @@
 
 #include "SSLProxySession.h"
 #include "iocore/net/NetVConnection.h"
+#include "iocore/net/TLSSNISupport.h"
 
 class TLSSNISupport;
 
 void
 SSLProxySession::init(NetVConnection const &new_vc)
 {
-  if (new_vc.get_service<TLSSNISupport>() != nullptr) {
-    if (char const *name = new_vc.get_server_name()) {
+  if (auto *snis = new_vc.get_service<TLSSNISupport>(); snis != nullptr) {
+    if (char const *name = snis->get_sni_server_name()) {
       _client_sni_server_name.assign(name);
     }
   }