Merge branch 'master' into quic-latest

* master:
  Try to avoid mixing curl headers and body for disjoing-wait-for-cache test
  Move TestClientAction to SNIConfig class
  Add mechanism to enforce SNI policy
  x-remap ignoring age in gold file
  Adjust consume logic in data frame read
  Skipping log_retention.test.py because it is flaky in CI
  Fix code to avoid HostDBContinuation use after free
  Fix crash when H2 client does not set End-of-data bit
  Signal VC_EVENT_READ_COMPLETE when ATS received END_STREAM flag
  if transaction status nonzero, bypass the slice plugin (#6417)
  Turn on debug for the bash script test_logstats_summary
  Fix port selection for ssl ipv6
  SSLNetVConnection, fixed/removed assert when running debug build
  traffic_dump post_process.py

doc/admin-guide/files/records.config.en.rst

@@ -1844,6 +1844,24 @@ Security
    ``2`` Similar to 0, except return a 416 error code and no response body.
    ===== ======================================================================
 
+.. ts:cv:: CONFIG proxy.config.http.host_sni_policy INT 2
+
+   This option controls how host header and SNI name mismatches are handled.  Mismatches
+   may result in SNI-based policies defined in :file:`sni.yaml` being avoided.  For example, ``foo.com``
+   may be the fqdn value in :file:`sni.yaml` which defines that client certificates are required.
+   The user could specify ``bar.com`` as the SNI to avoid the policy requiring the client certificate
+   but specify ``foo.com`` as the HTTP host header to still access the same object.
+
+   Therefore, if a host header would have triggered a SNI policy, it is possible that the user is
+   trying to bypass a SNI policy if the host header and SNI values do not match.
+
+   If this setting is 0, no checking is performed.  If this setting is 1 or 2, the host header and SNI values
+   are compared if the host header value would have triggered a SNI policy.  If there is a mismatch and the value
+   is 1, a warning is generated but the transaction is allowed to proceed.  If the value is 2 and there is a
+   mismatch, a warning is generated and a status 403 is returned.
+
+   You can override this global setting on a per domain basis in the :file:`sni.yaml` file using the :ref:`host_sni_policy attribute<override-host-sni-policy>` action.
+
 Cache Control
 =============
 

doc/admin-guide/files/sni.yaml.en.rst

@@ -45,6 +45,7 @@ the user needs to enter the fqdn in the configuration with a ``*.`` followed by
 .. _override-verify-origin-server:
 .. _override-verify-server-policy:
 .. _override-verify-server-properties:
+.. _override-host-sni-policy:
 
 ========================= ==============================================================================
 Key                       Meaning
@@ -73,6 +74,11 @@ verify_client             One of the values :code:`NONE`, :code:`MODERATE`, or :
 
                           By default this is :ts:cv:`proxy.config.ssl.client.certification_level`.
 
+host_sni_policy           One of the values :code:`DISABLED`, :code:`PERMISSIVE`, or :code:`ENFORCED`.
+
+                          If not specified, the value of :ts:cv:`proxy.config.http.host_sni_policy` is used.  This controls
+                          how policy impacting mismatches between host header and SNI values are dealt with.
+
 valid_tls_versions_in     This specifies the list of TLS protocols that will be offered to user agents during
                           the TLS negotiation.  This replaces the global settings in :ts:cv:`proxy.config.ssl.TLSv1`,
                           :ts:cv:`proxy.config.ssl.TLSv1_1`, :ts:cv:`proxy.config.ssl.TLSv1_2`,
@@ -175,13 +181,24 @@ Disable HTTP/2 for ``no-http2.example.com``.
    - fqdn: no-http2.example.com
      http2: off
 
-Require client certificate verification for ``example.com`` and any server name ending with ``.yahoo.com``. Therefore, client request for a server name ending with yahoo.com (e.g., def.yahoo.com, abc.yahoo.com etc.) will cause |TS| require and verify the client certificate. By contrast, |TS| will allow a client certificate to be provided for ``example.com`` and if it is, |TS| will require the certificate to be valid.
+Require client certificate verification for ``foo.com`` and any server name ending with ``.yahoo.com``. Therefore, client
+request for a server name ending with yahoo.com (e.g., def.yahoo.com, abc.yahoo.com etc.) will cause |TS| require and verify
+the client certificate.
+
+For ``foo.com``, if the user agent sets the host header to foo.com but the SNI to some other value, |TS| will warn about the
+mismatch but continue to process the request.  Mismatches for the other domains will cause |TS| to warn and return 403.
+
+|TS| will allow a client certificate to be provided for ``example.com`` and if it is, |TS| will require the
+certificate to be valid.
 
 .. code-block:: yaml
 
    sni:
    - fqdn: example.com
      verify_client: MODERATE
+   - fqdn: 'foo.com'
+     verify_client: STRICT
+     host_sni_policy: PERMISSIVE
    - fqdn: '*.yahoo.com'
      verify_client: STRICT
 

iocore/hostdb/HostDB.cc

@@ -1178,8 +1178,9 @@ HostDBContinuation::dnsEvent(int event, HostEnt *e)
   if (event == EVENT_INTERVAL) {
     if (!action.continuation) {
       // give up on insert, it has been too long
+      // remove_trigger_pending_dns will notify and clean up all requests
+      // including this one.
       remove_trigger_pending_dns();
-      hostdb_cont_free(this);
       return EVENT_DONE;
     }
     MUTEX_TRY_LOCK(lock, action.mutex, thread);
@@ -1440,18 +1441,18 @@ HostDBContinuation::dnsEvent(int event, HostEnt *e)
         }
       }
       if (need_to_reschedule) {
-        remove_trigger_pending_dns();
         SET_HANDLER((HostDBContHandler)&HostDBContinuation::probeEvent);
-        thread->schedule_in(this, HOST_DB_RETRY_PERIOD);
+        // remove_trigger_pending_dns should kick off the current hostDB too
+        // No need to explicitly reschedule
+        remove_trigger_pending_dns();
         return EVENT_CONT;
       }
     }
     // wake up everyone else who is waiting
     remove_trigger_pending_dns();
 
-    // all done
+    // all done, or at least scheduled to be all done
     //
-    hostdb_cont_free(this);
     return EVENT_DONE;
   }
 }

iocore/net/I_NetVConnection.h

@@ -456,6 +456,15 @@ class NetVConnection : public AnnotatedVConnection
   */
   virtual Action *send_OOB(Continuation *cont, char *buf, int len);
 
+  /**
+    Return the server name that is appropriate for the network VC type
+  */
+  virtual const char *
+  get_server_name() const
+  {
+    return nullptr;
+  }
+
   /**
     Cancels a scheduled send_OOB. Part of the message could have
     been sent already. Not callbacks to the cont are made after
@@ -628,6 +637,16 @@ class NetVConnection : public AnnotatedVConnection
     return netvc_context;
   }
 
+  /**
+   * Returns true if the network protocol
+   * supports a client provided SNI value
+   */
+  virtual bool
+  support_sni() const
+  {
+    return false;
+  }
+
   /** Structure holding user options. */
   NetVCOptions options;
 

iocore/net/P_SNIActionPerformer.h

@@ -39,6 +39,18 @@ class ActionItem
 {
 public:
   virtual int SNIAction(Continuation *cont) const = 0;
+
+  /**
+    This method tests whether this action would have been triggered by a
+    particuarly SNI value and IP address combination.  This is run after the
+    TLS exchange finished to see if the client used an SNI name different from
+    the host name to avoid SNI-based policy
+  */
+  virtual bool
+  TestClientSNIAction(const char *servername, const IpEndpoint &ep, int &policy) const
+  {
+    return false;
+  }
   virtual ~ActionItem(){};
 };
 
@@ -102,6 +114,36 @@ class VerifyClient : public ActionItem
     setClientCertLevel(ssl_vc->ssl, this->mode);
     return SSL_TLSEXT_ERR_OK;
   }
+  bool
+  TestClientSNIAction(const char *servername, const IpEndpoint &ep, int &policy) const override
+  {
+    // This action is triggered by a SNI if it was set
+    return true;
+  }
+};
+
+class HostSniPolicy : public ActionItem
+{
+  uint8_t policy;
+
+public:
+  HostSniPolicy(const char *param) : policy(atoi(param)) {}
+  HostSniPolicy(uint8_t param) : policy(param) {}
+  ~HostSniPolicy() override {}
+  int
+  SNIAction(Continuation *cont) const override
+  {
+    // On action this doesn't do anything
+    return SSL_TLSEXT_ERR_OK;
+  }
+  bool
+  TestClientSNIAction(const char *servername, const IpEndpoint &ep, int &in_policy) const override
+  {
+    // Update the policy when testing
+    in_policy = this->policy;
+    // But this action didn't really trigger during the action phase
+    return false;
+  }
 };
 
 class TLSValidProtocols : public ActionItem
@@ -128,6 +170,11 @@ class TLSValidProtocols : public ActionItem
     }
     return SSL_TLSEXT_ERR_OK;
   }
+  bool
+  TestClientSNIAction(const char *servername, const IpEndpoint &ep, int &policy) const override
+  {
+    return !unset;
+  }
 };
 
 class SNI_IpAllow : public ActionItem
@@ -178,4 +225,13 @@ class SNI_IpAllow : public ActionItem
       return SSL_TLSEXT_ERR_ALERT_FATAL;
     }
   }
+  bool
+  TestClientSNIAction(const char *servrername, const IpEndpoint &ep, int &policy) const override
+  {
+    bool retval = false;
+    if (ip_map.contains(ep)) {
+      retval = true;
+    }
+    return retval;
+  }
 };

iocore/net/P_SSLNetVConnection.h

@@ -401,13 +401,19 @@ class SSLNetVConnection : public UnixNetVConnection, public ALPNSupport
   // 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
+  get_server_name() const override
   {
     return _serverName.get() ? _serverName.get() : "";
   }
 
   void set_server_name(std::string_view name);
 
+  bool
+  support_sni() const override
+  {
+    return true;
+  }
+
   /// Set by asynchronous hooks to request a specific operation.
   SslVConnOp hookOpRequested = SSL_HOOK_OP_DEFAULT;
 

iocore/net/P_SSLSNI.h

@@ -122,6 +122,8 @@ struct SNIConfig {
 
   typedef ConfigProcessor::scoped_config<SNIConfig, SNIConfigParams> scoped_config;
 
+  static bool TestClientAction(const char *servername, const IpEndpoint &ep, int &enforcement_policy);
+
 private:
   static int configid;
 };

iocore/net/P_UnixNetVConnection.h

@@ -139,6 +139,12 @@ class UnixNetVConnection : public NetVConnection, public NetEvent
     return false;
   }
 
+  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;
 

iocore/net/SSLNetVConnection.cc

@@ -478,7 +478,7 @@ bool
 SSLNetVConnection::update_rbio(bool move_to_socket)
 {
   bool retval = false;
-  if (BIO_eof(SSL_get_rbio(this->ssl))) {
+  if (BIO_eof(SSL_get_rbio(this->ssl)) && this->handShakeReader != nullptr) {
     this->handShakeReader->consume(this->handShakeBioStored);
     this->handShakeBioStored = 0;
     // Load up the next block if present
@@ -601,7 +601,6 @@ SSLNetVConnection::net_read_io(NetHandler *nh, EThread *lthread)
       this->read.triggered = 0;
       readSignalError(nh, err);
     } else if (ret == SSL_HANDSHAKE_WANT_READ || ret == SSL_HANDSHAKE_WANT_ACCEPT) {
-      ink_assert(this->handShakeReader != nullptr);
       if (SSLConfigParams::ssl_handshake_timeout_in > 0) {
         double handshake_time = (static_cast<double>(Thread::get_hrtime() - sslHandshakeBeginTime) / 1000000000);
         Debug("ssl", "ssl handshake for vc %p, took %.3f seconds, configured handshake_timer: %d", this, handshake_time,
@@ -615,7 +614,7 @@ SSLNetVConnection::net_read_io(NetHandler *nh, EThread *lthread)
         }
       }
       // move over to the socket if we haven't already
-      if (this->handShakeBuffer) {
+      if (this->handShakeBuffer != nullptr) {
         read.triggered = update_rbio(true);
       } else {
         read.triggered = 0;

iocore/net/SSLSNIConfig.cc

@@ -70,6 +70,9 @@ SNIConfigParams::loadSNIConfig()
     if (item.verify_client_level != 255) {
       ai->actions.push_back(std::make_unique<VerifyClient>(item.verify_client_level));
     }
+    if (item.host_sni_policy != 255) {
+      ai->actions.push_back(std::make_unique<HostSniPolicy>(item.host_sni_policy));
+    }
     if (!item.protocol_unset) {
       ai->actions.push_back(std::make_unique<TLSValidProtocols>(item.protocol_mask));
     }
@@ -183,3 +186,22 @@ SNIConfig::release(SNIConfigParams *params)
 {
   configProcessor.release(configid, params);
 }
+
+// See if any of the client-side actions would trigger for this combination of servername and
+// client IP
+// host_sni_policy is an in/out paramter.  It starts with the global policy from the records.config
+// setting proxy.config.http.host_sni_policy and is possibly overridden if the sni policy
+// contains a host_sni_policy entry
+bool
+SNIConfig::TestClientAction(const char *servername, const IpEndpoint &ep, int &host_sni_policy)
+{
+  bool retval = false;
+  SNIConfig::scoped_config params;
+  const actionVector *actionvec = params->get(servername);
+  if (actionvec) {
+    for (auto &&item : *actionvec) {
+      retval |= item->TestClientSNIAction(servername, ep, host_sni_policy);
+    }
+  }
+  return retval;
+}

iocore/net/YamlSNIConfig.cc

@@ -104,12 +104,11 @@ std::set<std::string> valid_sni_config_keys = {TS_fqdn,
                                                TS_client_cert,
                                                TS_client_key,
                                                TS_http2,
-                                               TS_ip_allow
+                                               TS_ip_allow,
 #if TS_USE_HELLO_CB
-                                               ,
-                                               TS_valid_tls_versions_in
+                                               TS_valid_tls_versions_in,
 #endif
-};
+                                               TS_host_sni_policy};
 
 namespace YAML
 {
@@ -146,6 +145,12 @@ template <> struct convert<YamlSNIConfig::Item> {
       item.verify_client_level = static_cast<uint8_t>(level);
     }
 
+    if (node[TS_host_sni_policy]) {
+      auto value           = node[TS_host_sni_policy].as<std::string>();
+      int policy           = POLICY_DESCRIPTOR.get(value);
+      item.host_sni_policy = static_cast<uint8_t>(policy);
+    }
+
     if (node[TS_tunnel_route]) {
       item.tunnel_destination = node[TS_tunnel_route].as<std::string>();
       item.tunnel_decrypt     = false;

iocore/net/YamlSNIConfig.h

@@ -41,6 +41,7 @@ TSDECL(client_key);
 TSDECL(ip_allow);
 TSDECL(valid_tls_versions_in);
 TSDECL(http2);
+TSDECL(host_sni_policy);
 #undef TSDECL
 
 const int start = 0;
@@ -53,7 +54,8 @@ struct YamlSNIConfig {
     verify_server_policy,     // this applies to server side vc only
     verify_server_properties, // this applies to server side vc only
     client_cert,
-    h2 // this applies to client side only
+    h2,             // this applies to client side only
+    host_sni_policy // Applies to client side only
   };
   enum class Level { NONE = 0, MODERATE, STRICT };
   enum class Policy : uint8_t { DISABLED = 0, PERMISSIVE, ENFORCED, UNSET };
@@ -67,6 +69,7 @@ struct YamlSNIConfig {
     std::string fqdn;
     std::optional<bool> offer_h2; // Has no value by default, so do not initialize!
     uint8_t verify_client_level = 255;
+    uint8_t host_sni_policy     = 255;
     std::string tunnel_destination;
     bool tunnel_decrypt               = false;
     Policy verify_server_policy       = Policy::UNSET;

mgmt/RecordsConfig.cc

@@ -1460,6 +1460,14 @@ static const RecordElement RecordsConfig[] =
   // Controls for TLS ASYN_JOBS and engine loading
   {RECT_CONFIG, "proxy.config.ssl.async.handshake.enabled", RECD_INT, "0", RECU_RESTART_TS, RR_NULL, RECC_NULL, "[0-1]", RECA_NULL},
   {RECT_CONFIG, "proxy.config.ssl.engine.conf_file", RECD_STRING, nullptr, RECU_NULL, RR_NULL, RECC_NULL, nullptr, RECA_NULL},
+
+  //###########
+  //#
+  //# Control how the host/sni name mismatches are handled
+  //# 0 - no checking. 1 - log in mismatch. 2 - enforcing
+  //#
+  //###########
+  {RECT_CONFIG, "proxy.config.http.host_sni_policy", RECD_INT, "2", RECU_NULL, RR_NULL, RECC_NULL, "[0-2]", RECA_NULL},
 };
 // clang-format on