dns: don't respond with FORMERR if message has ID 0 (#35129)

Commit Message: dns: don't respond with FORMERR if message has ID 0 Additional Description: 0 is a valid ID according to RFC 1035 Risk Level: Low Testing: `./ci/run_envoy_docker.sh './ci/do_ci.sh dev'` Docs Changes: N/A Release Notes: dns: don't respond with FORMERR if message has ID 0 Platform Specific Features: N/A Fixes #35097 Signed-off-by: Mike Beaumont <[email protected]>
envoyproxy · Jul 22, 2024 · b8ae3ab · b8ae3ab
1 parent ec2ab0c
commit b8ae3ab
Show file tree

Hide file tree

Showing 4 changed files with 7 additions and 13 deletions.
diff --git a/changelogs/current.yaml b/changelogs/current.yaml
@@ -8,6 +8,9 @@ minor_behavior_changes:
 
 bug_fixes:
 # *Changes expected to improve the state of the world and are unlikely to have negative effects*
+- area: dns
+  change: |
+    The DNS filter no longer returns FORMERR if a message has an ID of 0.
 
 removed_config_or_runtime:
 # *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`

diff --git a/source/extensions/filters/udp/dns_filter/dns_parser.cc b/source/extensions/filters/udp/dns_filter/dns_parser.cc
@@ -217,10 +217,6 @@ bool DnsMessageParser::parseDnsObject(DnsQueryContextPtr& context,
   }
 
   context->id_ = static_cast<uint16_t>(context->header_.id);
-  if (context->id_ == 0) {
-    ENVOY_LOG(debug, "No ID in DNS query");
-    return false;
-  }
 
   // Almost always, we will have only one query here. Per the RFC, QDCOUNT is usually 1
   context->queries_.reserve(context->header_.questions);

diff --git a/test/extensions/filters/udp/dns_filter/dns_filter_test.cc b/test/extensions/filters/udp/dns_filter/dns_filter_test.cc
@@ -1863,12 +1863,11 @@ TEST_F(DnsFilterTest, NotImplementedQueryTest) {
   EXPECT_EQ(0, config_->stats().downstream_rx_invalid_queries_.value());
 }
 
-TEST_F(DnsFilterTest, NoTransactionIdTest) {
+TEST_F(DnsFilterTest, ZeroTransactionIdTest) {
   InSequence s;
 
   setup(forward_query_off_config);
-  // This buffer has an invalid Transaction ID. We should return an error
-  // to the client
+  // This buffer has a Transaction ID of zero. This is not an error.
   constexpr char dns_request[] = {
       0x00, 0x00,                               // Transaction ID
       0x01, 0x20,                               // Flags
@@ -1888,8 +1887,8 @@ TEST_F(DnsFilterTest, NoTransactionIdTest) {
   sendQueryFromClient("10.0.0.1:1000", query);
 
   response_ctx_ = ResponseValidator::createResponseContext(udp_response_, counters_);
-  EXPECT_FALSE(response_ctx_->parse_status_);
-  EXPECT_EQ(DNS_RESPONSE_CODE_FORMAT_ERROR, response_ctx_->getQueryResponseCode());
+  EXPECT_TRUE(response_ctx_->parse_status_);
+  EXPECT_EQ(DNS_RESPONSE_CODE_NOT_IMPLEMENTED, response_ctx_->getQueryResponseCode());
 }
 
 TEST_F(DnsFilterTest, InvalidShortBufferTest) {

diff --git a/test/extensions/filters/udp/dns_filter/dns_filter_test_utils.cc b/test/extensions/filters/udp/dns_filter/dns_filter_test_utils.cc
@@ -182,10 +182,6 @@ bool DnsResponseValidator::validateDnsResponseObject(DnsQueryContextPtr& context
   }
 
   context->id_ = static_cast<uint16_t>(context->header_.id);
-  if (context->id_ == 0) {
-    ENVOY_LOG(debug, "No ID in DNS Response");
-    return false;
-  }
 
   // Almost always, we will have only one query here. Per the RFC, QDCOUNT is usually 1
   context->queries_.reserve(context->header_.questions);