From ed03229e83cb380f65e441e5364e32e7c5766eae Mon Sep 17 00:00:00 2001
From: Ted Poole <tpoole@redhat.com>
Date: Wed, 22 May 2024 14:29:45 +0100
Subject: [PATCH] tls_inspector: Fix invalid ALPN extension in test

This commit stops generateClientHelloFromJA3Fingerprint() generating client
hellos containing an invalid ALPN extension. It also updates relevant
tls_inspector_test functions to check the ALPN value, if expected.

When the generateClientHelloFromJA3Fingerprint() function was asked to include
an ALPN extension (16) in the generated client hello, it was generating a
default empty extension with the correct id (16) but a zero length. While this
is technically a valid extension, it is not a valid ALPN extension, which must
include a list of the client's preferred protocol(s).

This was causing test failures in the envoy-openssl repo because OpenSSL
responds to the malformed ALPN extension by sending a TLS alert 50 (Decode
Error) which causes many of the tls_inspector_test functions to fail.

Signed-off-by: Ted Poole <tpoole@redhat.com>
---
 .../listener/tls_inspector/tls_inspector_test.cc | 12 ++++++++----
 .../listener/tls_inspector/tls_utility.cc        | 16 ++++++++++++++++
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc b/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc
index af7cd5492e93..d50ae1449595 100644
--- a/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc
+++ b/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc
@@ -94,7 +94,7 @@ class TlsInspectorTest : public testing::TestWithParam<std::tuple<uint16_t, uint
   }
 
   void testJA3(const std::string& fingerprint, bool expect_server_name = true,
-               const std::string& hash = {});
+               const std::string& hash = {}, bool expect_alpn = true);
 
   NiceMock<Api::MockOsSysCalls> os_sys_calls_;
   TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls_{&os_sys_calls_};
@@ -311,7 +311,7 @@ TEST_P(TlsInspectorTest, ConnectionFingerprint) {
 }
 
 void TlsInspectorTest::testJA3(const std::string& fingerprint, bool expect_server_name,
-                               const std::string& hash) {
+                               const std::string& hash, bool expect_alpn) {
   envoy::extensions::filters::listener::tls_inspector::v3::TlsInspector proto_config;
   proto_config.mutable_enable_ja3_fingerprinting()->set_value(true);
   cfg_ = std::make_shared<Config>(*store_.rootScope(), proto_config);
@@ -328,7 +328,11 @@ void TlsInspectorTest::testJA3(const std::string& fingerprint, bool expect_serve
   if (expect_server_name) {
     EXPECT_CALL(socket_, setRequestedServerName(absl::string_view("www.envoyproxy.io")));
   }
-  EXPECT_CALL(socket_, setRequestedApplicationProtocols(_)).Times(0);
+  if (expect_alpn) {
+    EXPECT_CALL(socket_, setRequestedApplicationProtocols(testing::Contains("HTTP/1.1")));
+  } else {
+    EXPECT_CALL(socket_, setRequestedApplicationProtocols(_)).Times(0);
+  }
   // EXPECT_CALL(cb_, continueFilterChain(true));
   EXPECT_CALL(socket_, setDetectedTransportProtocol(absl::string_view("tls")));
   EXPECT_CALL(socket_, detectedTransportProtocol()).Times(::testing::AnyNumber());
@@ -387,7 +391,7 @@ TEST_P(TlsInspectorTest, ConnectionJA3HashNoEllipticCurvesOrPointFormats) {
 TEST_P(TlsInspectorTest, ConnectionJA3HashTls10NoExtensions) {
   testJA3("769,49162-49157-49161-49156-49159-49154-49160-49155-49172-49167-49171-49166-49169-49164-"
           "49170-49165-57-51-53-47-5-4-10,,,",
-          false);
+          false, "", false);
 }
 
 // Test that the filter sets the correct `JA3` hash with TLS1.1.
diff --git a/test/extensions/filters/listener/tls_inspector/tls_utility.cc b/test/extensions/filters/listener/tls_inspector/tls_utility.cc
index 8a353de4bb4f..c143fd3c2e9a 100644
--- a/test/extensions/filters/listener/tls_inspector/tls_utility.cc
+++ b/test/extensions/filters/listener/tls_inspector/tls_utility.cc
@@ -117,6 +117,18 @@ std::vector<uint8_t> generateClientHelloFromJA3Fingerprint(const std::string& ja
                                                // algorithm
                                                0x04, 0x03};
 
+  // ALPN extension
+  const uint16_t alpn_id = 0x10;
+  std::vector<uint8_t> alpn_extension = {(alpn_id & 0xff00) >> 8, alpn_id & 0xff,
+                                         // length
+                                         0x00, 0x0b,
+                                         // list length
+                                         0x00, 0x09,
+                                         // protocol length
+                                         0x08,
+                                         // protocol name
+                                         'H', 'T', 'T', 'P', '/', '1', '.', '1'};
+
   // extensions
   values = absl::StrSplit(fingerprint[2], '-', absl::SkipEmpty());
   std::vector<uint8_t> extensions;
@@ -141,6 +153,10 @@ std::vector<uint8_t> generateClientHelloFromJA3Fingerprint(const std::string& ja
                         std::end(signature_algorithms));
       break;
     }
+    case alpn_id: {
+      extensions.insert(std::end(extensions), std::begin(alpn_extension), std::end(alpn_extension));
+      break;
+    }
     default: {
       uint16_t extension_id = std::stoi(v, nullptr);
       extensions.push_back((extension_id & 0xff00) >> 8);