Apply review comments

Signed-off-by: Kateryna Nezdolii <kateryna.nezdolii@gmail.com>
nezdolik · Apr 10, 2024 · 0a99292 · 0a99292
1 parent 3188514
commit 0a99292
Show file tree

Hide file tree

Showing 5 changed files with 98 additions and 24 deletions.
diff --git a/api/envoy/extensions/filters/listener/proxy_protocol/v3/proxy_protocol.proto b/api/envoy/extensions/filters/listener/proxy_protocol/v3/proxy_protocol.proto
@@ -77,7 +77,7 @@ message ProxyProtocol {
 message TlvsMetadata {
   // Typed metadata for :ref:`Proxy protocol filter <envoy_v3_api_msg_extensions.filters.listener.proxy_protocol.v3.ProxyProtocol>`, that represents map of TLVs.
   // Each entry in the map consists of a key (TLV type) and value (TLV value in bytes).
-  // When runtime flag ``envoy.reloadable_features.use_typed_metadata_in_proxy_protol_listener`` is enabled,
+  // When runtime flag ``envoy.reloadable_features.use_typed_metadata_in_proxy_protocol_listener`` is enabled,
   // :ref:`Proxy protocol filter <envoy_v3_api_msg_extensions.filters.listener.proxy_protocol.v3.ProxyProtocol>`
   // will set typed metadata instead of regular metadata. By default filter will use typed metadata.
   map<string, google.protobuf.BytesValue> typed_metadata = 1;

diff --git a/changelogs/current.yaml b/changelogs/current.yaml
@@ -15,7 +15,7 @@ behavior_changes:
   change: |
     Use typed metadata by default in proxy protocol listener. Typed metadata can be consumed as
     :ref:`TlvsMetadata type <envoy_v3_api_msg_extensions.filters.listener.proxy_protocol.v3.TlvsMetadata>`.
-    This change temporarily disabled by setting the runtime flag ``envoy.reloadable_features.use_typed_metadata_in_proxy_protol_listener``
+    This change temporarily disabled by setting the runtime flag ``envoy.reloadable_features.use_typed_metadata_in_proxy_protocol_listener``
     to ``false``.
 - area: http2
   change: |

diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc
@@ -101,7 +101,7 @@ RUNTIME_GUARD(envoy_reloadable_features_upstream_allow_connect_with_2xx);
 RUNTIME_GUARD(envoy_reloadable_features_upstream_wait_for_response_headers_before_disabling_read);
 RUNTIME_GUARD(envoy_reloadable_features_use_cluster_cache_for_alt_protocols_filter);
 RUNTIME_GUARD(envoy_reloadable_features_use_http3_header_normalisation);
-RUNTIME_GUARD(envoy_reloadable_features_use_typed_metadata_in_proxy_protol_listener);
+RUNTIME_GUARD(envoy_reloadable_features_use_typed_metadata_in_proxy_protocol_listener);
 RUNTIME_GUARD(envoy_reloadable_features_validate_connect);
 RUNTIME_GUARD(envoy_reloadable_features_validate_grpc_header_before_log_grpc_status);
 RUNTIME_GUARD(envoy_reloadable_features_validate_upstream_headers);

diff --git a/source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc b/source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc
@@ -442,7 +442,7 @@ bool Filter::parseTlvs(const uint8_t* buf, size_t len) {
     auto key_value_pair = config_->isTlvTypeNeeded(tlv_type);
     if (nullptr != key_value_pair) {
       if (Runtime::runtimeFeatureEnabled(
-              "envoy.reloadable_features.use_typed_metadata_in_proxy_protol_listener")) {
+              "envoy.reloadable_features.use_typed_metadata_in_proxy_protocol_listener")) {
         auto& typed_filter_metadata = (*cb_->dynamicMetadata().mutable_typed_filter_metadata());
         std::string metadata_key = key_value_pair->metadata_namespace().empty()
                                        ? "envoy.filters.listener.proxy_protocol"
@@ -451,16 +451,24 @@ bool Filter::parseTlvs(const uint8_t* buf, size_t len) {
         Protobuf::BytesValue tlv_byte_value;
         tlv_byte_value.set_value(tlv_value.data(), tlv_value.size());
         envoy::extensions::filters::listener::proxy_protocol::v3::TlvsMetadata tlvs_metadata;
+        auto status = absl::OkStatus();
         if (typed_proxy_filter_metadata != typed_filter_metadata.end()) {
-          MessageUtil::unpackTo(typed_proxy_filter_metadata->second, tlvs_metadata);
+          status = MessageUtil::unpackToNoThrow(typed_proxy_filter_metadata->second, tlvs_metadata);
+        }
+        if (!status.ok()) {
+          ENVOY_LOG(warn, "proxy_protocol: Failed to unpack typed metadata for TLV type ",
+                    tlv_type);
+        } else {
+          tlvs_metadata.mutable_typed_metadata()->insert({key_value_pair->key(), tlv_byte_value});
+          ProtobufWkt::Any typed_metadata;
+          typed_metadata.PackFrom(tlvs_metadata);
+          cb_->setDynamicTypedMetadata(metadata_key, typed_metadata);
         }
-        tlvs_metadata.mutable_typed_metadata()->insert({key_value_pair->key(), tlv_byte_value});
-        ProtobufWkt::Any typed_metadata;
-        typed_metadata.PackFrom(tlvs_metadata);
-        cb_->setDynamicTypedMetadata(metadata_key, typed_metadata);
       } else {
         ProtobufWkt::Value metadata_value;
-        metadata_value.set_string_value(tlv_value.data(), tlv_value.size());
+        // Sanitize any non utf8 characters.
+        auto sanitised_tlv_value = MessageUtil::sanitizeUtf8String(tlv_value);
+        metadata_value.set_string_value(sanitised_tlv_value.data(), sanitised_tlv_value.size());
 
         std::string metadata_key = key_value_pair->metadata_namespace().empty()
                                        ? "envoy.filters.listener.proxy_protocol"

diff --git a/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc b/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc
@@ -820,7 +820,7 @@ TEST_P(ProxyProtocolTest, V1TooLongWithAllowNoProxyProtocol) {
 TEST_P(ProxyProtocolTest, V2ParseExtensions) {
   TestScopedRuntime scoped_runtime;
   scoped_runtime.mergeValues({
-      {"envoy.reloadable_features.use_typed_metadata_in_proxy_protol_listener", "false"},
+      {"envoy.reloadable_features.use_typed_metadata_in_proxy_protocol_listener", "false"},
   });
   // A well-formed ipv4/tcp with a pair of TLV extensions is accepted
   constexpr uint8_t buffer[] = {0x0d, 0x0a, 0x0d, 0x0a, 0x00, 0x0d, 0x0a, 0x51, 0x55, 0x49,
@@ -1432,7 +1432,7 @@ const std::string ProxyProtocol = "envoy.filters.listener.proxy_protocol";
 TEST_P(ProxyProtocolTest, V2ParseExtensionsLargeThanInitMaxReadBytes) {
   TestScopedRuntime scoped_runtime;
   scoped_runtime.mergeValues({
-      {"envoy.reloadable_features.use_typed_metadata_in_proxy_protol_listener", "false"},
+      {"envoy.reloadable_features.use_typed_metadata_in_proxy_protocol_listener", "false"},
   });
   // A well-formed ipv4/tcp with a pair of TLV extensions is accepted
   constexpr uint8_t buffer[] = {0x0d, 0x0a, 0x0d, 0x0a, 0x00, 0x0d, 0x0a, 0x51, 0x55, 0x49,
@@ -1477,7 +1477,7 @@ TEST_P(ProxyProtocolTest, V2ParseExtensionsLargeThanInitMaxReadBytes) {
 TEST_P(ProxyProtocolTest, V2ExtractTlvOfInterest) {
   TestScopedRuntime scoped_runtime;
   scoped_runtime.mergeValues({
-      {"envoy.reloadable_features.use_typed_metadata_in_proxy_protol_listener", "false"},
+      {"envoy.reloadable_features.use_typed_metadata_in_proxy_protocol_listener", "false"},
   });
   // A well-formed ipv4/tcp with a pair of TLV extensions is accepted
   constexpr uint8_t buffer[] = {0x0d, 0x0a, 0x0d, 0x0a, 0x00, 0x0d, 0x0a, 0x51, 0x55, 0x49,
@@ -1520,7 +1520,7 @@ TEST_P(ProxyProtocolTest, V2ExtractTlvOfInterest) {
 TEST_P(ProxyProtocolTest, V2ExtractTlvOfInterestAndEmitWithSpecifiedMetadataNamespace) {
   TestScopedRuntime scoped_runtime;
   scoped_runtime.mergeValues({
-      {"envoy.reloadable_features.use_typed_metadata_in_proxy_protol_listener", "false"},
+      {"envoy.reloadable_features.use_typed_metadata_in_proxy_protocol_listener", "false"},
   });
   // A well-formed ipv4/tcp with a pair of TLV extensions is accepted
   constexpr uint8_t buffer[] = {0x0d, 0x0a, 0x0d, 0x0a, 0x00, 0x0d, 0x0a, 0x51, 0x55, 0x49,
@@ -1564,7 +1564,7 @@ TEST_P(ProxyProtocolTest, V2ExtractTlvOfInterestAndEmitWithSpecifiedMetadataName
 TEST_P(ProxyProtocolTest, V2ExtractMultipleTlvsOfInterest) {
   TestScopedRuntime scoped_runtime;
   scoped_runtime.mergeValues({
-      {"envoy.reloadable_features.use_typed_metadata_in_proxy_protol_listener", "false"},
+      {"envoy.reloadable_features.use_typed_metadata_in_proxy_protocol_listener", "false"},
   });
   // A well-formed ipv4/tcp with a pair of TLV extensions is accepted
   constexpr uint8_t buffer[] = {0x0d, 0x0a, 0x0d, 0x0a, 0x00, 0x0d, 0x0a, 0x51, 0x55, 0x49,
@@ -1624,6 +1624,74 @@ TEST_P(ProxyProtocolTest, V2ExtractMultipleTlvsOfInterest) {
   disconnect();
 }
 
+TEST_P(ProxyProtocolTest, V2ExtractMultipleTlvsOfInterestAndSanitiseNonUtf8) {
+  TestScopedRuntime scoped_runtime;
+  scoped_runtime.mergeValues({
+      {"envoy.reloadable_features.use_typed_metadata_in_proxy_protocol_listener", "false"},
+  });
+  // A well-formed ipv4/tcp with a pair of TLV extensions is accepted.
+  constexpr uint8_t buffer[] = {0x0d, 0x0a, 0x0d, 0x0a, 0x00, 0x0d, 0x0a, 0x51, 0x55, 0x49,
+                                0x54, 0x0a, 0x21, 0x11, 0x00, 0x39, 0x01, 0x02, 0x03, 0x04,
+                                0x00, 0x01, 0x01, 0x02, 0x03, 0x05, 0x00, 0x02};
+  // A TLV of type 0x00 with size of 4 (1 byte is value).
+  constexpr uint8_t tlv1[] = {0x00, 0x00, 0x01, 0xff};
+  // A TLV of type 0x02 with size of 10 bytes (7 bytes are value). Second and last bytes in the
+  // value are non utf8 characters.
+  constexpr uint8_t tlv_type_authority[] = {0x02, 0x00, 0x07, 0x66, 0xfe,
+                                            0x6f, 0x2e, 0x63, 0x6f, 0xc1};
+  // A TLV of type 0x0f with size of 6 bytes (3 bytes are value).
+  constexpr uint8_t tlv3[] = {0x0f, 0x00, 0x03, 0xf0, 0x00, 0x0f};
+  // A TLV of type 0xea with size of 25 bytes (22 bytes are value). 7th and 21st bytes are non utf8
+  // characters.
+  constexpr uint8_t tlv_vpc_id[] = {0xea, 0x00, 0x16, 0x01, 0x76, 0x70, 0x63, 0x2d, 0x30,
+                                    0xc0, 0x35, 0x74, 0x65, 0x73, 0x74, 0x32, 0x66, 0x61,
+                                    0x36, 0x63, 0x36, 0x33, 0x68, 0xf9, 0x37};
+  constexpr uint8_t data[] = {'D', 'A', 'T', 'A'};
+
+  envoy::extensions::filters::listener::proxy_protocol::v3::ProxyProtocol proto_config;
+  auto rule_type_authority = proto_config.add_rules();
+  rule_type_authority->set_tlv_type(0x02);
+  rule_type_authority->mutable_on_tlv_present()->set_key("PP2 type authority");
+
+  auto rule_vpc_id = proto_config.add_rules();
+  rule_vpc_id->set_tlv_type(0xea);
+  rule_vpc_id->mutable_on_tlv_present()->set_key("PP2 vpc id");
+
+  connect(true, &proto_config);
+  write(buffer, sizeof(buffer));
+  dispatcher_->run(Event::Dispatcher::RunType::NonBlock);
+
+  write(tlv1, sizeof(tlv1));
+  write(tlv_type_authority, sizeof(tlv_type_authority));
+  write(tlv3, sizeof(tlv3));
+  write(tlv_vpc_id, sizeof(tlv_vpc_id));
+  write(data, sizeof(data));
+  expectData("DATA");
+
+  EXPECT_EQ(1, server_connection_->streamInfo().dynamicMetadata().filter_metadata_size());
+
+  auto metadata = server_connection_->streamInfo().dynamicMetadata().filter_metadata();
+  EXPECT_EQ(1, metadata.size());
+  EXPECT_EQ(1, metadata.count(ProxyProtocol));
+
+  auto fields = metadata.at(ProxyProtocol).fields();
+  EXPECT_EQ(2, fields.size());
+  EXPECT_EQ(1, fields.count("PP2 type authority"));
+  EXPECT_EQ(1, fields.count("PP2 vpc id"));
+
+  const char replacement = 0x21;
+  auto value_type_authority = fields.at("PP2 type authority").string_value();
+  // Non utf8 characters have been replaced with `0x21` (`!` character).
+  ASSERT_THAT(value_type_authority,
+              ElementsAre(0x66, replacement, 0x6f, 0x2e, 0x63, 0x6f, replacement));
+
+  auto value_vpc_id = fields.at("PP2 vpc id").string_value();
+  ASSERT_THAT(value_vpc_id,
+              ElementsAre(0x01, 0x76, 0x70, 0x63, 0x2d, 0x30, replacement, 0x35, 0x74, 0x65, 0x73,
+                          0x74, 0x32, 0x66, 0x61, 0x36, 0x63, 0x36, 0x33, 0x68, replacement, 0x37));
+  disconnect();
+}
+
 TEST_P(ProxyProtocolTest, V2ExtractMultipleTlvsOfInterestAndEmitTypedMetadata) {
   // A well-formed ipv4/tcp with a pair of TLV extensions is accepted
   constexpr uint8_t buffer[] = {0x0d, 0x0a, 0x0d, 0x0a, 0x00, 0x0d, 0x0a, 0x51, 0x55, 0x49,
@@ -1662,15 +1730,14 @@ TEST_P(ProxyProtocolTest, V2ExtractMultipleTlvsOfInterestAndEmitTypedMetadata) {
   write(data, sizeof(data));
   expectData("DATA");
 
-  // By default (when runtime flag `use_typed_metadata_in_proxy_protol_listener` is set to `true`),
-  // only typed metadata should be populated.
+  // By default (when runtime flag `use_typed_metadata_in_proxy_protocol_listener` is set to
+  // `true`), only typed metadata should be populated.
   EXPECT_EQ(0, server_connection_->streamInfo().dynamicMetadata().filter_metadata_size());
 
   auto typed_metadata = server_connection_->streamInfo().dynamicMetadata().typed_filter_metadata();
   EXPECT_EQ(1, typed_metadata.size());
   EXPECT_EQ(1, typed_metadata.count(ProxyProtocol));
-  envoy::extensions::filters::listener::proxy_protocol::v3::ProxyProtocol::TlvsMetadata
-      tlvs_metadata;
+  envoy::extensions::filters::listener::proxy_protocol::v3::TlvsMetadata tlvs_metadata;
   MessageUtil::unpackTo(typed_metadata[ProxyProtocol], tlvs_metadata);
   EXPECT_EQ(2, tlvs_metadata.typed_metadata().size());
 
@@ -1725,16 +1792,15 @@ TEST_P(ProxyProtocolTest, V2ExtractMultipleTlvsOfInterestWithNonUtf8CharsAndEmit
   write(data, sizeof(data));
   expectData("DATA");
 
-  // By default (when runtime flag `use_typed_metadata_in_proxy_protol_listener` is set to `true`),
-  // only typed metadata should be populated.
+  // By default (when runtime flag `use_typed_metadata_in_proxy_protocol_listener` is set to
+  // `true`), only typed metadata should be populated.
   EXPECT_EQ(0, server_connection_->streamInfo().dynamicMetadata().filter_metadata_size());
 
   auto typed_metadata = server_connection_->streamInfo().dynamicMetadata().typed_filter_metadata();
   EXPECT_EQ(1, typed_metadata.size());
   EXPECT_EQ(1, typed_metadata.count(ProxyProtocol));
 
-  envoy::extensions::filters::listener::proxy_protocol::v3::ProxyProtocol::TlvsMetadata
-      tlvs_metadata;
+  envoy::extensions::filters::listener::proxy_protocol::v3::TlvsMetadata tlvs_metadata;
   MessageUtil::unpackTo(typed_metadata[ProxyProtocol], tlvs_metadata);
   EXPECT_EQ(2, tlvs_metadata.typed_metadata().size());
 
@@ -1752,7 +1818,7 @@ TEST_P(ProxyProtocolTest, V2ExtractMultipleTlvsOfInterestWithNonUtf8CharsAndEmit
 TEST_P(ProxyProtocolTest, V2WillNotOverwriteTLV) {
   TestScopedRuntime scoped_runtime;
   scoped_runtime.mergeValues({
-      {"envoy.reloadable_features.use_typed_metadata_in_proxy_protol_listener", "false"},
+      {"envoy.reloadable_features.use_typed_metadata_in_proxy_protocol_listener", "false"},
   });
   // A well-formed ipv4/tcp with a pair of TLV extensions is accepted
   constexpr uint8_t buffer[] = {0x0d, 0x0a, 0x0d, 0x0a, 0x00, 0x0d, 0x0a, 0x51, 0x55, 0x49,