middleware/secondary: multiple fixes (#745)

Fix transferring the zone from a master and the matching of notifies
to source and dst IP addresses.

Add `upstream` keyword as well, because it is needed for the same
reasons as in the *file* middlware.
Add some dire warning about upstream in the readme of both middlewares.

Out of band testing, hidden by net build tag was added. Integration
testing still needs to be setup.

5 files changed, 26 insertions, 11 deletions

diff --git a/middleware/file/README.md b/middleware/file/README.md
index 283986945..30391ed47 100644
--- a/middleware/file/README.md
+++ b/middleware/file/README.md
@@ -38,7 +38,8 @@ file DBFILE [ZONES... ] {
 * `no_reload` by default CoreDNS will reload a zone from disk whenever it detects a change to the
   file. This option disables that behavior.
 * `upstream` defines upstream resolvers to be used resolve external names found (think CNAMEs)
-  pointing to external names.
+  pointing to external names. This is only really useful when CoreDNS is configured as a proxy, for
+  normal authoritative serving you don't need *or* want to use this.
 
 ## Examples
 
diff --git a/middleware/file/notify.go b/middleware/file/notify.go
index 90dfdc521..e8f518d25 100644
--- a/middleware/file/notify.go
+++ b/middleware/file/notify.go
@@ -3,8 +3,8 @@ package file
 import (
 	"fmt"
 	"log"
+	"net"
 
-	"github.com/coredns/coredns/middleware"
 	"github.com/coredns/coredns/middleware/pkg/rcode"
 	"github.com/coredns/coredns/request"
 
@@ -21,8 +21,13 @@ func (z *Zone) isNotify(state request.Request) bool {
 	if len(z.TransferFrom) == 0 {
 		return false
 	}
-	remote := middleware.Addr(state.IP()).Normalize()
-	for _, from := range z.TransferFrom {
+	// If remote IP matches we accept.
+	remote := state.IP()
+	for _, f := range z.TransferFrom {
+		from, _, err := net.SplitHostPort(f)
+		if err != nil {
+			continue
+		}
 		if from == remote {
 			return true
 		}
diff --git a/middleware/file/secondary.go b/middleware/file/secondary.go
index 70ec217f7..e371600e9 100644
--- a/middleware/file/secondary.go
+++ b/middleware/file/secondary.go
@@ -26,19 +26,19 @@ Transfer:
 		t := new(dns.Transfer)
 		c, err := t.In(m, tr)
 		if err != nil {
-			log.Printf("[ERROR] Failed to setup transfer `%s' with `%s': %v", z.origin, tr, err)
+			log.Printf("[ERROR] Failed to setup transfer `%s' with `%q': %v", z.origin, tr, err)
 			Err = err
 			continue Transfer
 		}
 		for env := range c {
 			if env.Error != nil {
-				log.Printf("[ERROR] Failed to parse transfer `%s': %v", z.origin, env.Error)
+				log.Printf("[ERROR] Failed to transfer `%s' from %q: %v", z.origin, tr, env.Error)
 				Err = env.Error
 				continue Transfer
 			}
 			for _, rr := range env.RR {
 				if err := z1.Insert(rr); err != nil {
-					log.Printf("[ERROR] Failed to parse transfer `%s': %v", z.origin, err)
+					log.Printf("[ERROR] Failed to parse transfer `%s' from: %q: %v", z.origin, tr, err)
 					Err = err
 					continue Transfer
 				}
@@ -48,7 +48,6 @@ Transfer:
 		break
 	}
 	if Err != nil {
-		log.Printf("[ERROR] Failed to transfer %s: %s", z.origin, Err)
 		return Err
 	}
 
diff --git a/middleware/file/xfr.go b/middleware/file/xfr.go
index 4d7f07a48..54f7b71f8 100644
--- a/middleware/file/xfr.go
+++ b/middleware/file/xfr.go
@@ -57,6 +57,6 @@ func (x Xfr) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) (in
 }
 
 // Name implements the middleware.Hander interface.
-func (x Xfr) Name() string { return "xfr" } // Or should we return "file" here?
+func (x Xfr) Name() string { return "xfr" }
 
 const transferLength = 1000 // Start a new envelop after message reaches this size in bytes. Intentionally small to test multi envelope parsing.
diff --git a/middleware/file/zone.go b/middleware/file/zone.go
index 7592798f0..a216f8af8 100644
--- a/middleware/file/zone.go
+++ b/middleware/file/zone.go
@@ -2,6 +2,7 @@ package file
 
 import (
 	"fmt"
+	"net"
 	"path"
 	"strings"
 	"sync"
@@ -55,12 +56,12 @@ func NewZone(name, file string) *Zone {
 	return z
 }
 
-// Copy copies a zone *without* copying the zone's content. It is not a deep copy.
 func (z *Zone) Copy() *Zone {
 	z1 := NewZone(z.origin, z.file)
 	z1.TransferTo = z.TransferTo
 	z1.TransferFrom = z.TransferFrom
 	z1.Expired = z.Expired
+
 	z1.Apex = z.Apex
 	return z1
 }
@@ -113,11 +114,20 @@ func (z *Zone) Insert(r dns.RR) error {
 func (z *Zone) Delete(r dns.RR) { z.Tree.Delete(r) }
 
 // TransferAllowed checks if incoming request for transferring the zone is allowed according to the ACLs.
-func (z *Zone) TransferAllowed(req request.Request) bool {
+func (z *Zone) TransferAllowed(state request.Request) bool {
 	for _, t := range z.TransferTo {
 		if t == "*" {
 			return true
 		}
+		// If remote IP matches we accept.
+		remote := state.IP()
+		to, _, err := net.SplitHostPort(t)
+		if err != nil {
+			continue
+		}
+		if to == remote {
+			return true
+		}
 	}
 	// TODO(miek): future matching against IP/CIDR notations
 	return false