From dded10420b8a477ebd86cd2ceed9207a42c226cc Mon Sep 17 00:00:00 2001
From: Chris O'Haver <cohaver@infoblox.com>
Date: Fri, 17 Jun 2022 15:48:57 -0400
Subject: [PATCH] plugin/cache: Add option to adjust SERVFAIL response cache
 TTL (#5320)

* add servfail cache opt

Signed-off-by: Chris O'Haver <cohaver@infoblox.com>
---
 plugin/cache/README.md     |  4 ++++
 plugin/cache/cache.go      |  5 +++--
 plugin/cache/cache_test.go | 17 +++++++++++++++++
 plugin/cache/setup.go      | 17 +++++++++++++++++
 plugin/cache/setup_test.go | 37 +++++++++++++++++++++++++++++++++++++
 5 files changed, 78 insertions(+), 2 deletions(-)

diff --git a/plugin/cache/README.md b/plugin/cache/README.md
index 647909faf..9322af16c 100644
--- a/plugin/cache/README.md
+++ b/plugin/cache/README.md
@@ -38,6 +38,7 @@ cache [TTL] [ZONES...] {
     denial CAPACITY [TTL] [MINTTL]
     prefetch AMOUNT [[DURATION] [PERCENTAGE%]]
     serve_stale [DURATION] [REFRESH_MODE]
+    servfail DURATION
 }
 ~~~
 
@@ -63,6 +64,9 @@ cache [TTL] [ZONES...] {
   checking to see if the entry is available from the source. **REFRESH_MODE** defaults to `immediate`. Setting this
   value to `verify` can lead to increased latency when serving stale responses, but will prevent stale entries
   from ever being served if an updated response can be retrieved from the source.
+* `servfail` cache SERVFAIL responses for **DURATION**.  Setting **DURATION** to 0 will disable caching of SERVFAIL
+  responses.  If this option is not set, SERVFAIL responses will be cached for 5 seconds.  **DURATION** may not be
+  greater than 5 minutes.
 
 ## Capacity and Eviction
 
diff --git a/plugin/cache/cache.go b/plugin/cache/cache.go
index fb84fcec0..fc42866ac 100644
--- a/plugin/cache/cache.go
+++ b/plugin/cache/cache.go
@@ -32,6 +32,7 @@ type Cache struct {
 	pcap    int
 	pttl    time.Duration
 	minpttl time.Duration
+	failttl time.Duration // TTL for caching SERVFAIL responses
 
 	// Prefetch.
 	prefetch   int
@@ -59,6 +60,7 @@ func New() *Cache {
 		ncache:     cache.New(defaultCap),
 		nttl:       maxNTTL,
 		minnttl:    minNTTL,
+		failttl:    minNTTL,
 		prefetch:   0,
 		duration:   1 * time.Minute,
 		percentage: 10,
@@ -158,8 +160,7 @@ func (w *ResponseWriter) WriteMsg(res *dns.Msg) error {
 	if mt == response.NameError || mt == response.NoData {
 		duration = computeTTL(msgTTL, w.minnttl, w.nttl)
 	} else if mt == response.ServerError {
-		// use default ttl which is 5s
-		duration = minTTL
+		duration = w.failttl
 	} else {
 		duration = computeTTL(msgTTL, w.minpttl, w.pttl)
 	}
diff --git a/plugin/cache/cache_test.go b/plugin/cache/cache_test.go
index 7299dc073..69bea61a4 100644
--- a/plugin/cache/cache_test.go
+++ b/plugin/cache/cache_test.go
@@ -258,6 +258,23 @@ func TestCacheZeroTTL(t *testing.T) {
 	}
 }
 
+func TestCacheServfailTTL0(t *testing.T) {
+	c := New()
+	c.minpttl = minTTL
+	c.minnttl = minNTTL
+	c.failttl = 0
+	c.Next = servFailBackend(0)
+
+	req := new(dns.Msg)
+	req.SetQuestion("example.org.", dns.TypeA)
+	ctx := context.TODO()
+
+	c.ServeDNS(ctx, &test.ResponseWriter{}, req)
+	if c.ncache.Len() != 0 {
+		t.Errorf("SERVFAIL response should not have been cached")
+	}
+}
+
 func TestServeFromStaleCache(t *testing.T) {
 	c := New()
 	c.Next = ttlBackend(60)
diff --git a/plugin/cache/setup.go b/plugin/cache/setup.go
index e5258dc06..aa487105c 100644
--- a/plugin/cache/setup.go
+++ b/plugin/cache/setup.go
@@ -188,6 +188,23 @@ func cacheParse(c *caddy.Controller) (*Cache, error) {
 					}
 					ca.verifyStale = mode == "verify"
 				}
+			case "servfail":
+				args := c.RemainingArgs()
+				if len(args) != 1 {
+					return nil, c.ArgErr()
+				}
+				d, err := time.ParseDuration(args[0])
+				if err != nil {
+					return nil, err
+				}
+				if d < 0 {
+					return nil, errors.New("invalid negative ttl for servfail")
+				}
+				if d > 5*time.Minute {
+					// RFC 2308 prohibits caching SERVFAIL longer than 5 minutes
+					return nil, errors.New("caching SERVFAIL responses over 5 minutes is not permitted")
+				}
+				ca.failttl = d
 			default:
 				return nil, c.ArgErr()
 			}
diff --git a/plugin/cache/setup_test.go b/plugin/cache/setup_test.go
index 675147d1b..5e684c510 100644
--- a/plugin/cache/setup_test.go
+++ b/plugin/cache/setup_test.go
@@ -155,3 +155,40 @@ func TestServeStale(t *testing.T) {
 		}
 	}
 }
+
+func TestServfail(t *testing.T) {
+	tests := []struct {
+		input     string
+		shouldErr bool
+		failttl   time.Duration
+	}{
+		{"servfail 1s", false, 1 * time.Second},
+		{"servfail 5m", false, 5 * time.Minute},
+		{"servfail 0s", false, 0},
+		{"servfail 0", false, 0},
+		// fails
+		{"servfail", true, minNTTL},
+		{"servfail 6m", true, minNTTL},
+		{"servfail 20", true, minNTTL},
+		{"servfail -1s", true, minNTTL},
+		{"servfail aa", true, minNTTL},
+		{"servfail 1m invalid", true, minNTTL},
+	}
+	for i, test := range tests {
+		c := caddy.NewTestController("dns", fmt.Sprintf("cache {\n%s\n}", test.input))
+		ca, err := cacheParse(c)
+		if test.shouldErr && err == nil {
+			t.Errorf("Test %v: Expected error but found nil", i)
+			continue
+		} else if !test.shouldErr && err != nil {
+			t.Errorf("Test %v: Expected no error but found error: %v", i, err)
+			continue
+		}
+		if test.shouldErr && err != nil {
+			continue
+		}
+		if ca.failttl != test.failttl {
+			t.Errorf("Test %v: Expected stale %v but found: %v", i, test.failttl, ca.staleUpTo)
+		}
+	}
+}