From 5129fb08413a168e3a61f56f69939860e00ddd65 Mon Sep 17 00:00:00 2001
From: Otto Moerbeek <otto.moerbeek@open-xchange.com>
Date: Mon, 10 Jun 2024 15:53:23 +0200
Subject: [PATCH] rec: make max CNAME chain length handled settable, previously
 fixed at 10

---
 pdns/recursordist/rec-main.cc         |  1 +
 pdns/recursordist/settings/table.py   | 12 ++++++++++++
 pdns/recursordist/syncres.cc          |  2 +-
 pdns/recursordist/test-syncres_cc.cc  |  1 +
 pdns/recursordist/test-syncres_cc1.cc | 12 ++++++++++++
 5 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/pdns/recursordist/rec-main.cc b/pdns/recursordist/rec-main.cc
index 2b2d1705b3..4dcabb35ef 100644
--- a/pdns/recursordist/rec-main.cc
+++ b/pdns/recursordist/rec-main.cc
@@ -1750,6 +1750,7 @@ static int initSyncRes(Logr::log_t log)
   SyncRes::s_event_trace_enabled = ::arg().asNum("event-trace-enabled");
   SyncRes::s_save_parent_ns_set = ::arg().mustDo("save-parent-ns-set");
   SyncRes::s_max_busy_dot_probes = ::arg().asNum("max-busy-dot-probes");
+  SyncRes::s_max_CNAMES_followed = ::arg().asNum("max-cnames-followed");
   {
     uint64_t sse = ::arg().asNum("serve-stale-extensions");
     if (sse > std::numeric_limits<uint16_t>::max()) {
diff --git a/pdns/recursordist/settings/table.py b/pdns/recursordist/settings/table.py
index e16200c7d1..5a072bc6d1 100644
--- a/pdns/recursordist/settings/table.py
+++ b/pdns/recursordist/settings/table.py
@@ -1525,6 +1525,18 @@ This is used to avoid cycles resolving names.
  ''',
         'versionchanged': ('5.1.0', 'The default used to be 60, with an extra allowance if qname minimization was enabled. Having better algorithms allows for a lower default limit.'),
     },
+    {
+        'name' : 'max_cnames_followed',
+        'section' : 'recursor',
+        'type' : LType.Uint64,
+        'default' : '10',
+        'help' : 'Maximum number CNAME records followed',
+        'doc' : '''
+Maximum length of a CNAME chain. If a CNAME chain exceeds this length, a ``ServFail`` answer will be returned.
+Previously, this limit was fixed at 10.
+ ''',
+    'versionadded': '5.1.0'
+    },    
     {
         'name' : 'max_ns_address_qperq',
         'section' : 'outgoing',
diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc
index e54c88ed51..3a19b0bfcd 100644
--- a/pdns/recursordist/syncres.cc
+++ b/pdns/recursordist/syncres.cc
@@ -474,7 +474,7 @@ bool SyncRes::s_dot_to_port_853;
 int SyncRes::s_event_trace_enabled;
 bool SyncRes::s_save_parent_ns_set;
 unsigned int SyncRes::s_max_busy_dot_probes;
-unsigned int SyncRes::s_max_CNAMES_followed = 10;
+unsigned int SyncRes::s_max_CNAMES_followed;
 bool SyncRes::s_addExtendedResolutionDNSErrors;
 
 // NOLINTNEXTLINE(cppcoreguidelines-macro-usage)
diff --git a/pdns/recursordist/test-syncres_cc.cc b/pdns/recursordist/test-syncres_cc.cc
index d0b5892e16..7f253647d1 100644
--- a/pdns/recursordist/test-syncres_cc.cc
+++ b/pdns/recursordist/test-syncres_cc.cc
@@ -186,6 +186,7 @@ void initSR(bool debug)
   SyncRes::s_locked_ttlperc = 0;
   SyncRes::s_minimize_one_label = 4;
   SyncRes::s_max_minimize_count = 10;
+  SyncRes::s_max_CNAMES_followed = 10;
 
   SyncRes::clearNSSpeeds();
   BOOST_CHECK_EQUAL(SyncRes::getNSSpeedsSize(), 0U);
diff --git a/pdns/recursordist/test-syncres_cc1.cc b/pdns/recursordist/test-syncres_cc1.cc
index a844564523..86278fbe64 100644
--- a/pdns/recursordist/test-syncres_cc1.cc
+++ b/pdns/recursordist/test-syncres_cc1.cc
@@ -1771,6 +1771,18 @@ BOOST_AUTO_TEST_CASE(test_cname_length)
   BOOST_CHECK_EQUAL(res, RCode::ServFail);
   BOOST_CHECK_EQUAL(ret.size(), length);
   BOOST_CHECK_EQUAL(length, SyncRes::s_max_CNAMES_followed + 1);
+
+  // Currently a CNAME bounds check originating from the record cache causes an ImmediateServFail
+  // exception. This is different from the non-cached case, tested above. There a ServFail is
+  // returned with a partial CNAME chain. This should be fixed one way or another. For details, see
+  // how the result of syncres.cc:scanForCNAMELoop() is handled in the two cases.
+  ret.clear();
+  length = 0;
+  BOOST_CHECK_EXCEPTION(sr->beginResolve(target, QType(QType::A), QClass::IN, ret),
+                        ImmediateServFailException,
+                        [&](const ImmediateServFailException& isfe) {
+                          return isfe.reason == "max number of CNAMEs exceeded";
+                        });
 }
 
 BOOST_AUTO_TEST_CASE(test_cname_target_servfail)
-- 
2.47.2