middleware/cache: don't cache expired RRSIGs (#641)

Check message for expired sig and don't cache those.

Aside: This hack of caching entire messages is probably something we
should stop doing at some point in the future and do this on a per RRset
basis.

Fixes #367 #635

middleware/cache/cache.go

@@ -63,7 +63,7 @@ type ResponseWriter struct {
 // WriteMsg implements the dns.ResponseWriter interface.
 func (c *ResponseWriter) WriteMsg(res *dns.Msg) error {
 	do := false
-	mt, opt := response.Typify(res)
+	mt, opt := response.Typify(res, time.Now().UTC())
 	if opt != nil {
 		do = opt.Do()
 	}

middleware/cache/cache_test.go

@@ -21,6 +21,7 @@ type cacheTestCase struct {
 	Authoritative      bool
 	RecursionAvailable bool
 	Truncated          bool
+	shouldCache        bool
 }
 
 var cacheTestCases = []cacheTestCase{
@@ -40,6 +41,7 @@ var cacheTestCases = []cacheTestCase{
 				test.MX("miek.nl.	3601	IN	MX	10 aspmx2.googlemail.com."),
 			},
 		},
+		shouldCache: true,
 	},
 	{
 		RecursionAvailable: true, AuthenticatedData: true, Authoritative: true,
@@ -57,6 +59,7 @@ var cacheTestCases = []cacheTestCase{
 				test.MX("mIEK.nL.	3601	IN	MX	10 aspmx2.googlemail.com."),
 			},
 		},
+		shouldCache: true,
 	},
 	{
 		Truncated: true,
@@ -65,6 +68,7 @@ var cacheTestCases = []cacheTestCase{
 			Answer: []dns.RR{test.MX("miek.nl.	1800	IN	MX	1 aspmx.l.google.com.")},
 		},
 		in:          test.Case{},
+		shouldCache: false,
 	},
 	{
 		RecursionAvailable: true, Authoritative: true,
@@ -82,6 +86,51 @@ var cacheTestCases = []cacheTestCase{
 				test.SOA("example.org. 3600 IN	SOA	sns.dns.icann.org. noc.dns.icann.org. 2016082540 7200 3600 1209600 3600"),
 			},
 		},
+		shouldCache: true,
+	},
+	{
+		RecursionAvailable: true, Authoritative: true,
+		Case: test.Case{
+			Qname: "miek.nl.", Qtype: dns.TypeMX,
+			Do: true,
+			Answer: []dns.RR{
+				test.MX("miek.nl.	3600	IN	MX	1 aspmx.l.google.com."),
+				test.MX("miek.nl.	3600	IN	MX	10 aspmx2.googlemail.com."),
+				test.RRSIG("miek.nl.	3600	IN	RRSIG	MX 8 2 1800 20160521031301 20160421031301 12051 miek.nl. lAaEzB5teQLLKyDenatmyhca7blLRg9DoGNrhe3NReBZN5C5/pMQk8Jc u25hv2fW23/SLm5IC2zaDpp2Fzgm6Jf7e90/yLcwQPuE7JjS55WMF+HE LEh7Z6AEb+Iq4BWmNhUz6gPxD4d9eRMs7EAzk13o1NYi5/JhfL6IlaYy qkc="),
+			},
+		},
+		in: test.Case{
+			Qname: "miek.nl.", Qtype: dns.TypeMX,
+			Do: true,
+			Answer: []dns.RR{
+				test.MX("miek.nl.	3600	IN	MX	1 aspmx.l.google.com."),
+				test.MX("miek.nl.	3600	IN	MX	10 aspmx2.googlemail.com."),
+				test.RRSIG("miek.nl.	1800	IN	RRSIG	MX 8 2 1800 20160521031301 20160421031301 12051 miek.nl. lAaEzB5teQLLKyDenatmyhca7blLRg9DoGNrhe3NReBZN5C5/pMQk8Jc u25hv2fW23/SLm5IC2zaDpp2Fzgm6Jf7e90/yLcwQPuE7JjS55WMF+HE LEh7Z6AEb+Iq4BWmNhUz6gPxD4d9eRMs7EAzk13o1NYi5/JhfL6IlaYy qkc="),
+			},
+		},
+		shouldCache: false,
+	},
+	{
+		RecursionAvailable: true, Authoritative: true,
+		Case: test.Case{
+			Qname: "example.org.", Qtype: dns.TypeMX,
+			Do: true,
+			Answer: []dns.RR{
+				test.MX("example.org.	3600	IN	MX	1 aspmx.l.google.com."),
+				test.MX("example.org.	3600	IN	MX	10 aspmx2.googlemail.com."),
+				test.RRSIG("example.org.	3600	IN	RRSIG	MX 8 2 1800 20170521031301 20170421031301 12051 miek.nl. lAaEzB5teQLLKyDenatmyhca7blLRg9DoGNrhe3NReBZN5C5/pMQk8Jc u25hv2fW23/SLm5IC2zaDpp2Fzgm6Jf7e90/yLcwQPuE7JjS55WMF+HE LEh7Z6AEb+Iq4BWmNhUz6gPxD4d9eRMs7EAzk13o1NYi5/JhfL6IlaYy qkc="),
+			},
+		},
+		in: test.Case{
+			Qname: "example.org.", Qtype: dns.TypeMX,
+			Do: true,
+			Answer: []dns.RR{
+				test.MX("example.org.	3600	IN	MX	1 aspmx.l.google.com."),
+				test.MX("example.org.	3600	IN	MX	10 aspmx2.googlemail.com."),
+				test.RRSIG("example.org.	1800	IN	RRSIG	MX 8 2 1800 20170521031301 20170421031301 12051 miek.nl. lAaEzB5teQLLKyDenatmyhca7blLRg9DoGNrhe3NReBZN5C5/pMQk8Jc u25hv2fW23/SLm5IC2zaDpp2Fzgm6Jf7e90/yLcwQPuE7JjS55WMF+HE LEh7Z6AEb+Iq4BWmNhUz6gPxD4d9eRMs7EAzk13o1NYi5/JhfL6IlaYy qkc="),
+			},
+		},
+		shouldCache: true,
 	},
 }
 
@@ -93,7 +142,7 @@ func cacheMsg(m *dns.Msg, tc cacheTestCase) *dns.Msg {
 	m.Truncated = tc.Truncated
 	m.Answer = tc.in.Answer
 	m.Ns = tc.in.Ns
-	//	m.Extra = tc.in.Extra , not the OPT record!
+	// m.Extra = tc.in.Extra don't copy Extra, because we don't care and fake EDNS0 DO with tc.Do.
 	return m
 }
 
@@ -107,6 +156,9 @@ func newTestCache(ttl time.Duration) (*Cache, *ResponseWriter) {
 }
 
 func TestCache(t *testing.T) {
+	now, _ := time.Parse(time.UnixDate, "Fri Apr 21 10:51:21 BST 2017")
+	utc := now.UTC()
+
 	c, crr := newTestCache(maxTTL)
 
 	log.SetOutput(ioutil.Discard)
@@ -116,15 +168,18 @@ func TestCache(t *testing.T) {
 		m = cacheMsg(m, tc)
 		do := tc.in.Do
 
-		mt, _ := response.Typify(m)
+		mt, _ := response.Typify(m, utc)
 		k := key(m, mt, do)
+
 		crr.set(m, k, mt, c.pttl)
 
 		name := middleware.Name(m.Question[0].Name).Normalize()
 		qtype := m.Question[0].Qtype
+
 		i, ok, _ := c.get(name, qtype, do)
-		if ok && m.Truncated {
+
-			t.Errorf("Truncated message should not have been cached")
+		if ok != tc.shouldCache {
+			t.Errorf("cached message that should not have been cached: %s", name)
 			continue
 		}
 

middleware/dnssec/dnssec.go

@@ -41,7 +41,7 @@ func New(zones []string, keys []*DNSKEY, next middleware.Handler, cache *lru.Cac
 func (d Dnssec) Sign(state request.Request, zone string, now time.Time) *dns.Msg {
 	req := state.Req
 
-	mt, _ := response.Typify(req) // TODO(miek): need opt record here?
+	mt, _ := response.Typify(req, time.Now().UTC()) // TODO(miek): need opt record here?
 	if mt == response.Delegation {
 		return req
 	}

middleware/log/log.go

@@ -52,7 +52,8 @@ func (l Logger) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg)
 			rc = 0
 		}
 
-		class, _ := response.Classify(rrw.Msg)
+		tpe, _ := response.Typify(rrw.Msg, time.Now().UTC())
+		class := response.Classify(tpe)
 		if rule.Class == response.All || rule.Class == class {
 			rep := replacer.New(r, rrw, CommonLogEmptyValue)
 			rule.Log.Println(rep.Replace(rule.Format))

middleware/pkg/response/classify.go

@@ -1,10 +1,6 @@
 package response
 
-import (
+import "fmt"
-	"fmt"
-
-	"github.com/miekg/dns"
-)
 
 // Class holds sets of Types
 type Class int
@@ -50,14 +46,8 @@ func ClassFromString(s string) (Class, error) {
 	return All, fmt.Errorf("invalid Class: %s", s)
 }
 
-// Classify classifies a dns message: it returns its Class.
+// Classify classifies the Type t, it returns its Class.
-func Classify(m *dns.Msg) (Class, *dns.OPT) {
+func Classify(t Type) Class {
-	t, o := Typify(m)
-	return classify(t), o
-}
-
-// Does need to be exported?
-func classify(t Type) Class {
 	switch t {
 	case NoError, Delegation:
 		return Success

middleware/pkg/response/typify.go

@@ -2,11 +2,12 @@ package response
 
 import (
 	"fmt"
+	"time"
 
 	"github.com/miekg/dns"
 )
 
-// Type is the type of the message
+// Type is the type of the message.
 type Type int
 
 const (
@@ -26,54 +27,39 @@ const (
 	OtherError
 )
 
-func (t Type) String() string {
+var toString = map[Type]string{
-	switch t {
+	NoError:    "NOERROR",
-	case NoError:
+	NameError:  "NXDOMAIN",
-		return "NOERROR"
+	NoData:     "NODATA",
-	case NameError:
+	Delegation: "DELEGATION",
-		return "NXDOMAIN"
+	Meta:       "META",
-	case NoData:
+	Update:     "UPDATE",
-		return "NODATA"
+	OtherError: "OTHERERROR",
-	case Delegation:
-		return "DELEGATION"
-	case Meta:
-		return "META"
-	case Update:
-		return "UPDATE"
-	case OtherError:
-		return "OTHERERROR"
-	}
-	return ""
 }
 
+func (t Type) String() string { return toString[t] }
+
 // TypeFromString returns the type from the string s. If not type matches
 // the OtherError type and an error are returned.
 func TypeFromString(s string) (Type, error) {
-	switch s {
+	for t, str := range toString {
-	case "NOERROR":
+		if s == str {
-		return NoError, nil
+			return t, nil
-	case "NXDOMAIN":
+		}
-		return NameError, nil
-	case "NODATA":
-		return NoData, nil
-	case "DELEGATION":
-		return Delegation, nil
-	case "META":
-		return Meta, nil
-	case "UPDATE":
-		return Update, nil
-	case "OTHERERROR":
-		return OtherError, nil
 	}
 	return NoError, fmt.Errorf("invalid Type: %s", s)
 }
 
 // Typify classifies a message, it returns the Type.
-func Typify(m *dns.Msg) (Type, *dns.OPT) {
+func Typify(m *dns.Msg, t time.Time) (Type, *dns.OPT) {
 	if m == nil {
 		return OtherError, nil
 	}
 	opt := m.IsEdns0()
+	do := false
+	if opt != nil {
+		do = opt.Do()
+	}
 
 	if m.Opcode == dns.OpcodeUpdate {
 		return Update, opt
@@ -90,6 +76,13 @@ func Typify(m *dns.Msg) (Type, *dns.OPT) {
 		}
 	}
 
+	// If our message contains any expired sigs and we care about that, we should return expired
+	if do {
+		if expired := typifyExpired(m, t); expired {
+			return OtherError, opt
+		}
+	}
+
 	if len(m.Answer) > 0 && m.Rcode == dns.RcodeSuccess {
 		return NoError, opt
 	}
@@ -107,6 +100,7 @@ func Typify(m *dns.Msg) (Type, *dns.OPT) {
 	}
 
 	// Check length of different sections, and drop stuff that is just to large? TODO(miek).
+
 	if soa && m.Rcode == dns.RcodeSuccess {
 		return NoData, opt
 	}
@@ -114,7 +108,7 @@ func Typify(m *dns.Msg) (Type, *dns.OPT) {
 		return NameError, opt
 	}
 
-	if ns > 0 && ns == len(m.Ns) && m.Rcode == dns.RcodeSuccess {
+	if ns > 0 && ns > 0 && m.Rcode == dns.RcodeSuccess {
 		return Delegation, opt
 	}
 
@@ -124,3 +118,29 @@ func Typify(m *dns.Msg) (Type, *dns.OPT) {
 
 	return OtherError, opt
 }
+
+func typifyExpired(m *dns.Msg, t time.Time) bool {
+	if expired := typifyExpiredRRSIG(m.Answer, t); expired {
+		return true
+	}
+	if expired := typifyExpiredRRSIG(m.Ns, t); expired {
+		return true
+	}
+	if expired := typifyExpiredRRSIG(m.Extra, t); expired {
+		return true
+	}
+	return false
+}
+
+func typifyExpiredRRSIG(rrs []dns.RR, t time.Time) bool {
+	for _, r := range rrs {
+		if r.Header().Rrtype != dns.TypeRRSIG {
+			continue
+		}
+		ok := r.(*dns.RRSIG).ValidityPeriod(t)
+		if !ok {
+			return true
+		}
+	}
+	return false
+}

middleware/pkg/response/typify_test.go

@@ -2,6 +2,7 @@ package response
 
 import (
 	"testing"
+	"time"
 
 	"github.com/coredns/coredns/middleware/test"
 
@@ -11,17 +12,39 @@ import (
 func TestTypifyNilMsg(t *testing.T) {
 	var m *dns.Msg
 
-	ty, _ := Typify(m)
+	ty, _ := Typify(m, time.Now().UTC())
 	if ty != OtherError {
-		t.Errorf("message wrongly typified, expected OtherError, got %d", ty)
+		t.Errorf("message wrongly typified, expected OtherError, got %s", ty)
 	}
 }
 
-func TestClassifyDelegation(t *testing.T) {
+func TestTypifyDelegation(t *testing.T) {
 	m := delegationMsg()
-	mt, _ := Typify(m)
+	mt, _ := Typify(m, time.Now().UTC())
 	if mt != Delegation {
-		t.Errorf("message is wrongly classified, expected delegation, got %d", mt)
+		t.Errorf("message is wrongly typified, expected Delegation, got %s", mt)
+	}
+}
+
+func TestTypifyRRSIG(t *testing.T) {
+	now, _ := time.Parse(time.UnixDate, "Fri Apr 21 10:51:21 BST 2017")
+	utc := now.UTC()
+
+	m := delegationMsgRRSIGOK()
+	if mt, _ := Typify(m, utc); mt != Delegation {
+		t.Errorf("message is wrongly typified, expected Delegation, got %s", mt)
+	}
+
+	// Still a Delegation because EDNS0 OPT DO bool is not set, so we won't check the sigs.
+	m = delegationMsgRRSIGFail()
+	if mt, _ := Typify(m, utc); mt != Delegation {
+		t.Errorf("message is wrongly typified, expected Delegation, got %s", mt)
+	}
+
+	m = delegationMsgRRSIGFail()
+	m = addOpt(m)
+	if mt, _ := Typify(m, utc); mt != OtherError {
+		t.Errorf("message is wrongly typified, expected OtherError, got %s", mt)
 	}
 }
 
@@ -38,3 +61,24 @@ func delegationMsg() *dns.Msg {
 		},
 	}
 }
+
+func delegationMsgRRSIGOK() *dns.Msg {
+	del := delegationMsg()
+	del.Ns = append(del.Ns,
+		test.RRSIG("miek.nl.		1800	IN	RRSIG	NS 8 2 1800 20170521031301 20170421031301 12051 miek.nl. PIUu3TKX/sB/N1n1E1yWxHHIcPnc2q6Wq9InShk+5ptRqChqKdZNMLDm gCq+1bQAZ7jGvn2PbwTwE65JzES7T+hEiqR5PU23DsidvZyClbZ9l0xG JtKwgzGXLtUHxp4xv/Plq+rq/7pOG61bNCxRyS7WS7i7QcCCWT1BCcv+ wZ0="),
+	)
+	return del
+}
+
+func delegationMsgRRSIGFail() *dns.Msg {
+	del := delegationMsg()
+	del.Ns = append(del.Ns,
+		test.RRSIG("miek.nl.		1800	IN	RRSIG	NS 8 2 1800 20160521031301 20160421031301 12051 miek.nl. PIUu3TKX/sB/N1n1E1yWxHHIcPnc2q6Wq9InShk+5ptRqChqKdZNMLDm gCq+1bQAZ7jGvn2PbwTwE65JzES7T+hEiqR5PU23DsidvZyClbZ9l0xG JtKwgzGXLtUHxp4xv/Plq+rq/7pOG61bNCxRyS7WS7i7QcCCWT1BCcv+ wZ0="),
+	)
+	return del
+}
+
+func addOpt(m *dns.Msg) *dns.Msg {
+	m.Extra = append(m.Extra, test.OPT(4096, true))
+	return m
+}