Merge pull request #46963 from corhere/libn/resolver-pack-servfail

libnetwork: write ServFail if DNS reply msg is bad

libnetwork/resolver.go

@@ -378,9 +378,18 @@ func (r *Resolver) serveDNS(w dns.ResponseWriter, query *dns.Msg) {
 
 	reply := func(msg *dns.Msg) {
 		if err = w.WriteMsg(msg); err != nil {
-			r.log(ctx).WithError(err).Errorf("[resolver] failed to write response")
+			r.log(ctx).WithError(err).Error("[resolver] failed to write response")
 			span.RecordError(err)
 			span.SetStatus(codes.Error, "WriteMsg failed")
+			// Make a best-effort attempt to send a failure response to the
+			// client so it doesn't have to wait for a timeout if the failure
+			// has to do with the content of msg rather than the connection.
+			if msg.Rcode != dns.RcodeServerFailure {
+				if err := w.WriteMsg(new(dns.Msg).SetRcode(query, dns.RcodeServerFailure)); err != nil {
+					r.log(ctx).WithError(err).Error("[resolver] writing ServFail response also failed")
+					span.RecordError(err)
+				}
+			}
 		}
 	}
 

libnetwork/resolver_test.go

@@ -38,6 +38,10 @@ type tstwriter struct {
 }
 
 func (w *tstwriter) WriteMsg(m *dns.Msg) (err error) {
+	// Assert that the message is serializable.
+	if _, err := m.Pack(); err != nil {
+		return err
+	}
 	w.msg = m
 	return nil
 }
@@ -82,7 +86,7 @@ func checkDNSAnswersCount(t *testing.T, m *dns.Msg, expected int) {
 func checkDNSResponseCode(t *testing.T, m *dns.Msg, expected int) {
 	t.Helper()
 	if m.MsgHdr.Rcode != expected {
-		t.Fatalf("Expected DNS response code: %d. Found: %d", expected, m.MsgHdr.Rcode)
+		t.Fatalf("Expected DNS response code: %d (%s). Found: %d (%s)", expected, dns.RcodeToString[expected], m.MsgHdr.Rcode, dns.RcodeToString[m.MsgHdr.Rcode])
 	}
 }
 
@@ -355,3 +359,26 @@ func TestProxyNXDOMAIN(t *testing.T) {
 	assert.Assert(t, is.Len(resp.Ns, 1))
 	assert.Equal(t, resp.Ns[0].String(), mockSOA.String())
 }
+
+type ptrDNSBackend struct {
+	noopDNSBackend
+	zone map[string]string
+}
+
+func (b *ptrDNSBackend) ResolveIP(_ context.Context, name string) string {
+	return b.zone[name]
+}
+
+// Regression test for https://github.com/moby/moby/issues/46928
+func TestInvalidReverseDNS(t *testing.T) {
+	rsv := NewResolver("", false, &ptrDNSBackend{zone: map[string]string{"4.3.2.1": "sixtyfourcharslong9012345678901234567890123456789012345678901234"}})
+	rsv.logger = testLogger(t)
+
+	w := &tstwriter{}
+	q := new(dns.Msg).SetQuestion("4.3.2.1.in-addr.arpa.", dns.TypePTR)
+	rsv.serveDNS(w, q)
+	resp := w.GetResponse()
+	checkNonNullResponse(t, resp)
+	t.Log("Response: ", resp.String())
+	checkDNSResponseCode(t, resp, dns.RcodeServerFailure)
+}

libnetwork/resolver_unix_test.go

@@ -61,7 +61,7 @@ func TestDNSIPQuery(t *testing.T) {
 
 	// test name1's IP is resolved correctly with the default A type query
 	// Also make sure DNS lookups are case insensitive
-	names := []string{"name1", "NaMe1"}
+	names := []string{"name1.", "NaMe1."}
 	for _, name := range names {
 		q := new(dns.Msg)
 		q.SetQuestion(name, dns.TypeA)
@@ -84,7 +84,7 @@ func TestDNSIPQuery(t *testing.T) {
 
 	// test MX query with name1 results in Success response with 0 answer records
 	q := new(dns.Msg)
-	q.SetQuestion("name1", dns.TypeMX)
+	q.SetQuestion("name1.", dns.TypeMX)
 	r.serveDNS(w, q)
 	resp := w.GetResponse()
 	checkNonNullResponse(t, resp)
@@ -96,7 +96,7 @@ func TestDNSIPQuery(t *testing.T) {
 	// test MX query with non existent name results in ServFail response with 0 answer records
 	// since this is a unit test env, we disable proxying DNS above which results in ServFail rather than NXDOMAIN
 	q = new(dns.Msg)
-	q.SetQuestion("nonexistent", dns.TypeMX)
+	q.SetQuestion("nonexistent.", dns.TypeMX)
 	r.serveDNS(w, q)
 	resp = w.GetResponse()
 	checkNonNullResponse(t, resp)