external authorization: set the SNI value from server name if it isn'…

…t available on the connection/socket (#34100)

Signed-off-by: Marc Barry <[email protected]>

changelogs/current.yaml

@@ -91,6 +91,11 @@ bug_fixes:
   change: |
     Fix BalsaParser resetting state too early, guarded by default-true
     ``envoy.reloadable_features.http1_balsa_delay_reset``.
+- area: ext_authz
+  change: |
+    Set the SNI value from the requested server name if it isn't available on the connection/socket. This applies when
+    ``include_tls_session`` is true. The requested server name is set on a connection when filters such as the TLS
+    inspector are used.
 
 removed_config_or_runtime:
 # *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`

source/extensions/filters/common/ext_authz/check_request_utils.cc

@@ -223,10 +223,14 @@ void CheckRequestUtils::setAttrContextRequest(
 
 void CheckRequestUtils::setTLSSession(
     envoy::service::auth::v3::AttributeContext::TLSSession& session,
-    const Ssl::ConnectionInfoConstSharedPtr ssl_info) {
-  if (!ssl_info->sni().empty()) {
+    const Envoy::Network::Connection& connection) {
+  const Ssl::ConnectionInfoConstSharedPtr ssl_info = connection.ssl();
+  if (ssl_info != nullptr && !ssl_info->sni().empty()) {
     const std::string server_name(ssl_info->sni());
     session.set_sni(server_name);
+  } else if (!connection.requestedServerName().empty()) {
+    const std::string server_name(connection.requestedServerName());
+    session.set_sni(server_name);
   }
 }
 
@@ -248,6 +252,7 @@ void CheckRequestUtils::createHttpCheck(
   // *cb->connection(), callbacks->streamInfo() and callbacks->decodingBuffer() are not qualified as
   // const.
   auto* cb = const_cast<Envoy::Http::StreamDecoderFilterCallbacks*>(callbacks);
+
   setAttrContextPeer(*attrs->mutable_source(), *cb->connection(), service, false,
                      include_peer_certificate);
   setAttrContextPeer(*attrs->mutable_destination(), *cb->connection(), EMPTY_STRING, true,
@@ -256,8 +261,8 @@ void CheckRequestUtils::createHttpCheck(
                         cb->decodingBuffer(), headers, max_request_bytes, pack_as_bytes,
                         encode_raw_headers, allowed_headers_matcher, disallowed_headers_matcher);
 
-  if (include_tls_session && cb->connection()->ssl() != nullptr) {
-    setTLSSession(*attrs->mutable_tls_session(), cb->connection()->ssl());
+  if (include_tls_session) {
+    setTLSSession(*attrs->mutable_tls_session(), *cb->connection());
   }
   (*attrs->mutable_destination()->mutable_labels()) = destination_labels;
   // Fill in the context extensions and metadata context.
@@ -280,8 +285,9 @@ void CheckRequestUtils::createTcpCheck(
                      include_peer_certificate);
   setAttrContextPeer(*attrs->mutable_destination(), cb->connection(), server_name, true,
                      include_peer_certificate);
-  if (include_tls_session && cb->connection().ssl() != nullptr) {
-    setTLSSession(*attrs->mutable_tls_session(), cb->connection().ssl());
+
+  if (include_tls_session) {
+    setTLSSession(*attrs->mutable_tls_session(), cb->connection());
   }
   (*attrs->mutable_destination()->mutable_labels()) = destination_labels;
 }

source/extensions/filters/common/ext_authz/check_request_utils.h

@@ -143,7 +143,7 @@ class CheckRequestUtils {
       bool encode_raw_headers, const MatcherSharedPtr& allowed_headers_matcher,
       const MatcherSharedPtr& disallowed_headers_matcher);
   static void setTLSSession(envoy::service::auth::v3::AttributeContext::TLSSession& session,
-                            const Ssl::ConnectionInfoConstSharedPtr ssl_info);
+                            const Envoy::Network::Connection& connection);
   static std::string getHeaderStr(const Envoy::Http::HeaderEntry* entry);
   static Envoy::Http::HeaderMap::Iterate fillHttpHeaders(const Envoy::Http::HeaderEntry&, void*);
 };

test/extensions/filters/common/ext_authz/check_request_utils_test.cc

@@ -98,7 +98,11 @@ class CheckRequestUtilsTest : public testing::Test {
 
     EXPECT_EQ(want_tls_session != nullptr, request.attributes().has_tls_session());
     if (want_tls_session != nullptr) {
-      EXPECT_EQ(want_tls_session->sni(), request.attributes().tls_session().sni());
+      if (!want_tls_session->sni().empty()) {
+        EXPECT_EQ(want_tls_session->sni(), request.attributes().tls_session().sni());
+      } else {
+        EXPECT_EQ(requested_server_name_, request.attributes().tls_session().sni());
+      }
     }
   }
 
@@ -223,10 +227,10 @@ TEST_F(CheckRequestUtilsTest, TcpPeerCertificate) {
 // Verify that createTcpCheck populates the tls session details correctly.
 TEST_F(CheckRequestUtilsTest, TcpTlsSession) {
   envoy::service::auth::v3::CheckRequest request;
-  EXPECT_CALL(net_callbacks_, connection()).Times(5).WillRepeatedly(ReturnRef(connection_));
+  EXPECT_CALL(net_callbacks_, connection()).Times(4).WillRepeatedly(ReturnRef(connection_));
   connection_.stream_info_.downstream_connection_info_provider_->setRemoteAddress(addr_);
   connection_.stream_info_.downstream_connection_info_provider_->setLocalAddress(addr_);
-  EXPECT_CALL(Const(connection_), ssl()).Times(4).WillRepeatedly(Return(ssl_));
+  EXPECT_CALL(Const(connection_), ssl()).Times(3).WillRepeatedly(Return(ssl_));
   EXPECT_CALL(*ssl_, uriSanPeerCertificate()).WillOnce(Return(std::vector<std::string>{"source"}));
   EXPECT_CALL(*ssl_, uriSanLocalCertificate())
       .WillOnce(Return(std::vector<std::string>{"destination"}));
@@ -240,6 +244,29 @@ TEST_F(CheckRequestUtilsTest, TcpTlsSession) {
   EXPECT_EQ(want_tls_session.sni(), request.attributes().tls_session().sni());
 }
 
+// Verify that createTcpCheck populates the tls session details correctly from the connection when
+// TLS session information isn't present.
+TEST_F(CheckRequestUtilsTest, TcpTlsSessionNoSessionSni) {
+  envoy::service::auth::v3::CheckRequest request;
+  EXPECT_CALL(net_callbacks_, connection()).Times(4).WillRepeatedly(ReturnRef(connection_));
+  connection_.stream_info_.downstream_connection_info_provider_->setRemoteAddress(addr_);
+  connection_.stream_info_.downstream_connection_info_provider_->setLocalAddress(addr_);
+  EXPECT_CALL(connection_, requestedServerName())
+      .Times(3)
+      .WillRepeatedly(Return(requested_server_name_));
+  EXPECT_CALL(Const(connection_), ssl()).Times(3).WillRepeatedly(Return(ssl_));
+  EXPECT_CALL(*ssl_, uriSanPeerCertificate()).WillOnce(Return(std::vector<std::string>{"source"}));
+  EXPECT_CALL(*ssl_, uriSanLocalCertificate())
+      .WillOnce(Return(std::vector<std::string>{"destination"}));
+  envoy::service::auth::v3::AttributeContext_TLSSession want_tls_session;
+  EXPECT_CALL(*ssl_, sni()).WillOnce(ReturnRef(want_tls_session.sni()));
+
+  CheckRequestUtils::createTcpCheck(&net_callbacks_, request, false, true,
+                                    Protobuf::Map<std::string, std::string>());
+  EXPECT_TRUE(request.attributes().has_tls_session());
+  EXPECT_EQ(requested_server_name_, request.attributes().tls_session().sni());
+}
+
 // Verify that createHttpCheck's dependencies are invoked when it's called.
 // Verify that check request object has no request data.
 // Verify that a client supplied EnvoyAuthPartialBody will not affect the
@@ -691,11 +718,11 @@ TEST_F(CheckRequestUtilsTest, CheckAttrContextPeerCertificate) {
 // Verify that the SNI is populated correctly.
 TEST_F(CheckRequestUtilsTest, CheckAttrContextPeerTLSSession) {
   EXPECT_CALL(callbacks_, connection())
-      .Times(4)
+      .Times(3)
       .WillRepeatedly(Return(OptRef<const Network::Connection>{connection_}));
   connection_.stream_info_.downstream_connection_info_provider_->setRemoteAddress(addr_);
   connection_.stream_info_.downstream_connection_info_provider_->setLocalAddress(addr_);
-  EXPECT_CALL(Const(connection_), ssl()).Times(4).WillRepeatedly(Return(ssl_));
+  EXPECT_CALL(Const(connection_), ssl()).Times(3).WillRepeatedly(Return(ssl_));
   EXPECT_CALL(callbacks_, streamId()).WillOnce(Return(0));
   EXPECT_CALL(callbacks_, decodingBuffer()).WillOnce(Return(buffer_.get()));
   EXPECT_CALL(callbacks_, streamInfo()).WillOnce(ReturnRef(req_info_));
@@ -715,11 +742,14 @@ TEST_F(CheckRequestUtilsTest, CheckAttrContextPeerTLSSession) {
 // Verify that the SNI is populated correctly.
 TEST_F(CheckRequestUtilsTest, CheckAttrContextPeerTLSSessionWithoutSNI) {
   EXPECT_CALL(callbacks_, connection())
-      .Times(4)
+      .Times(3)
       .WillRepeatedly(Return(OptRef<const Network::Connection>{connection_}));
   connection_.stream_info_.downstream_connection_info_provider_->setRemoteAddress(addr_);
   connection_.stream_info_.downstream_connection_info_provider_->setLocalAddress(addr_);
-  EXPECT_CALL(Const(connection_), ssl()).Times(4).WillRepeatedly(Return(ssl_));
+  EXPECT_CALL(Const(connection_), ssl()).Times(3).WillRepeatedly(Return(ssl_));
+  EXPECT_CALL(connection_, requestedServerName())
+      .Times(2)
+      .WillRepeatedly(Return(requested_server_name_));
   EXPECT_CALL(callbacks_, streamId()).WillOnce(Return(0));
   EXPECT_CALL(callbacks_, decodingBuffer()).WillOnce(Return(buffer_.get()));
   EXPECT_CALL(callbacks_, streamInfo()).WillOnce(ReturnRef(req_info_));