plugin/file: reject SOA owners that do not match the zone (#8555)

Signed-off-by: houyuwushang <liuluoqianqiu@outlook.com>

plugin/file/README.md

@@ -22,6 +22,12 @@ file DBFILE [ZONES...]
 * **ZONES** zones it should be authoritative for. If empty, the zones from the configuration block
   are used.
 
+The SOA record's owner name must match the zone being loaded. Names without a final dot are
+relative to the current origin; for example, `test` in a zone loaded as `test.` becomes
+`test.test.`, not `test.`. Use `@` (when the current origin matches the zone) or the zone's
+absolute name for the SOA owner. A mismatched SOA causes loading to fail; an invalid reload
+leaves the last successfully loaded zone in service.
+
 If you want to round-robin A and AAAA responses look at the *loadbalance* plugin.
 
 ~~~
@@ -72,17 +78,28 @@ www     IN A     127.0.0.1
 ~~~
 
 
-Or use a single zone file for multiple zones:
+Or use a single zone file for multiple zones, with relative owner names and no fixed `$ORIGIN`:
 
 ~~~ corefile
 . {
-    file example.org.signed example.org example.net
+    file db.shared example.org example.net
     transfer example.org example.net {
         to * 10.240.1.1
     }
 }
 ~~~
 
+For example, `db.shared` can contain:
+
+~~~
+@   3600 IN SOA sns.dns.icann.org. noc.dns.icann.org. 2017042745 7200 3600 1209600 3600
+@   3600 IN NS a.iana-servers.net.
+www 3600 IN A 127.0.0.1
+~~~
+
+Each configured zone is used as the initial origin when parsing this file, so `@` is its apex.
+Signed zones require signatures for the actual owner names and must be signed separately.
+
 Note that if you have a configuration like the following you may run into a problem of the origin
 not being correctly recognized:
 
@@ -93,9 +110,9 @@ not being correctly recognized:
 ~~~
 
 We omit the origin for the file `db.example.org`, so this references the zone in the server block,
-which, in this case, is the root zone. Any contents of `db.example.org` will then read with that
-origin set; this may or may not do what you want.
-It's better to be explicit here and specify the correct origin. This can be done in two ways:
+which, in this case, is the root zone. A file with an SOA for `example.org.` will be rejected
+because it does not match the configured root zone, even if the file sets `$ORIGIN example.org.`.
+Specify the correct zone in one of two ways:
 
 ~~~ corefile
 . {

plugin/file/dname_test.go

@@ -142,7 +142,7 @@ var dnameDnssecTestCases = []test.Case{
 }
 
 func TestLookupDNAMEDNSSEC(t *testing.T) {
-	zone, err := Parse(strings.NewReader(dbExampleDNAMESigned), testzone, "stdin", 0)
+	zone, err := Parse(strings.NewReader(dbExampleDNAMESigned), "example.org.", "stdin", 0)
 	if err != nil {
 		t.Fatalf("Expect no error when reading zone, got %q", err)
 	}

plugin/file/file.go

@@ -192,8 +192,11 @@ func Parse(f io.Reader, origin, fileName string, serial int64) (*Zone, error) {
 
 	seenSOA := false
 	for rr, ok := zp.Next(); ok; rr, ok = zp.Next() {
-		if !seenSOA {
-			if s, ok := rr.(*dns.SOA); ok {
+		if s, ok := rr.(*dns.SOA); ok {
+			if dns.CanonicalName(canonicalEscape(s.Hdr.Name)) != dns.CanonicalName(canonicalEscape(z.origin)) {
+				return nil, fmt.Errorf("file %q has SOA owner %s that does not match origin %s", fileName, s.Hdr.Name, z.origin)
+			}
+			if !seenSOA {
 				seenSOA = true
 
 				// -1 is valid serial is we failed to load the file on startup.

plugin/file/file_test.go

@@ -1,6 +1,7 @@
 package file
 
 import (
+	"fmt"
 	"strings"
 	"testing"
 )
@@ -21,6 +22,74 @@ func TestParseNoSOA(t *testing.T) {
 	}
 }
 
+func TestParseSOAOrigin(t *testing.T) {
+	tests := []struct {
+		name, origin, prefix, owner string
+		wantErr                     bool
+	}{
+		{name: "at", origin: "example.org.", owner: "@"},
+		{name: "absolute", origin: "example.org.", owner: "example.org."},
+		{name: "mixed case", origin: "example.org.", owner: "ExAmPlE.OrG."},
+		{name: "mixed case origin", origin: "ExAmPlE.OrG.", owner: "example.org."},
+		{name: "origin without final dot", origin: "example.org", owner: "@"},
+		{name: "relative to parent", origin: "example.org.", prefix: "$ORIGIN org.\n", owner: "example"},
+		{name: "decimal escape", origin: "example.org.", owner: `\101xample.org.`},
+		{name: "escaped dot", origin: `has\.dot.example.`, owner: `has\046dot.example.`},
+		{name: "root", origin: ".", owner: "@"},
+		{name: "relative owner", origin: "test", owner: "test", wantErr: true},
+		{name: "child", origin: "example.org.", owner: "child.example.org.", wantErr: true},
+		{name: "parent", origin: "example.org.", owner: "org.", wantErr: true},
+		{name: "unrelated", origin: "example.org.", owner: "example.net.", wantErr: true},
+		{name: "different ORIGIN", origin: "example.org.", prefix: "$ORIGIN example.net.\n", owner: "@", wantErr: true},
+		{name: "not root", origin: ".", owner: "example.org.", wantErr: true},
+	}
+	for _, tc := range tests {
+		for _, serial := range []int64{-1, 2, 3} {
+			t.Run(fmt.Sprintf("%s/serial=%d", tc.name, serial), func(t *testing.T) {
+				zone := tc.prefix + tc.owner + " 500 IN SOA ns.example. hostmaster.example. 3 3600 600 86400 300\n"
+				z, err := Parse(strings.NewReader(zone), tc.origin, "db.test", serial)
+				if tc.wantErr {
+					if err == nil {
+						t.Fatalf("accepted SOA owner %q for origin %q", z.SOA.Hdr.Name, tc.origin)
+					}
+					if !strings.Contains(err.Error(), "SOA owner") || !strings.Contains(err.Error(), "db.test") {
+						t.Fatalf("expected SOA owner error with file name, got %v", err)
+					}
+					if z != nil {
+						t.Fatal("invalid zone must not be returned")
+					}
+					return
+				}
+				if serial == 3 {
+					if _, ok := err.(*serialErr); !ok {
+						t.Fatalf("expected unchanged serial error, got %v", err)
+					}
+					return
+				}
+				if err != nil {
+					t.Fatal(err)
+				}
+				if z.SOA == nil {
+					t.Fatal("missing apex SOA")
+				}
+			})
+		}
+	}
+}
+
+func TestParseLaterSOAOrigin(t *testing.T) {
+	zone := `@ 500 IN SOA ns.example. hostmaster.example. 3 3600 600 86400 300
+child 500 IN SOA ns.example. hostmaster.example. 4 3600 600 86400 300
+`
+	z, err := Parse(strings.NewReader(zone), "example.org.", "db.test", -1)
+	if err == nil {
+		t.Fatalf("replaced apex SOA with %q", z.SOA.Hdr.Name)
+	}
+	if !strings.Contains(err.Error(), "SOA owner") {
+		t.Fatalf("expected SOA owner error, got %v", err)
+	}
+}
+
 const dbNoSOA = `
 $TTL         1M
 $ORIGIN      example.org.

plugin/file/reload_test.go

@@ -3,8 +3,10 @@ package file
 import (
 	"context"
 	"os"
+	"path/filepath"
 	"strings"
 	"testing"
+	"testing/synctest"
 	"time"
 
 	"github.com/coredns/coredns/plugin/test"
@@ -80,6 +82,52 @@ func TestZoneReloadSOAChange(t *testing.T) {
 	}
 }
 
+func TestZoneReloadSOAOrigin(t *testing.T) {
+	synctest.Test(t, func(t *testing.T) {
+		fileName := filepath.Join(t.TempDir(), "db.example.org")
+		z, err := Parse(strings.NewReader(dbRelative), "example.org.", fileName, -1)
+		if err != nil {
+			t.Fatal(err)
+		}
+		beforeApex, beforeTree := z.snapshot()
+		updated := strings.Replace(dbRelative, " 3 3600", " 4 3600", 1)
+		updated = strings.Replace(updated, "192.0.2.1", "192.0.2.2", 1)
+		invalid := strings.Replace(updated, "@ 500 IN SOA", "child 500 IN SOA", 1)
+		if err := os.WriteFile(fileName, []byte(invalid), 0644); err != nil {
+			t.Fatal(err)
+		}
+		z.ReloadInterval = time.Second
+		if err := z.Reload(nil); err != nil {
+			t.Fatal(err)
+		}
+		defer z.OnShutdown()
+
+		time.Sleep(2 * time.Second)
+		synctest.Wait()
+		apex, tree := z.snapshot()
+		if apex.SOA != beforeApex.SOA || tree != beforeTree {
+			t.Fatal("invalid reload replaced the last valid zone")
+		}
+
+		if err := os.WriteFile(fileName, []byte(updated), 0644); err != nil {
+			t.Fatal(err)
+		}
+		time.Sleep(time.Second)
+		synctest.Wait()
+		apex, tree = z.snapshot()
+		if apex.SOA.Hdr.Name != "example.org." || apex.SOA.Serial != 4 || tree == beforeTree {
+			t.Fatalf("corrected zone was not reloaded: %v", apex.SOA)
+		}
+		r := new(dns.Msg)
+		r.SetQuestion("foo.example.org.", dns.TypeA)
+		state := request.Request{W: &test.ResponseWriter{}, Req: r}
+		answer, _, _, result := z.Lookup(context.Background(), state, state.Name())
+		if result != Success || len(answer) != 1 || answer[0].String() != "foo.example.org.\t500\tIN\tA\t192.0.2.2" {
+			t.Fatalf("expected updated A record, got result %v, answer %v", result, answer)
+		}
+	})
+}
+
 func TestZoneReloadByMtime(t *testing.T) {
 	// Test 1: Basic mtime trigger - file modification should trigger reload
 	t.Run("BasicMtimeTrigger", func(t *testing.T) {

plugin/file/setup_test.go

@@ -1,14 +1,27 @@
 package file
 
 import (
+	"context"
+	"fmt"
+	"os"
+	"path/filepath"
+	"strings"
 	"testing"
 	"time"
 
 	"github.com/coredns/caddy"
+	"github.com/coredns/coredns/plugin/pkg/dnstest"
 	"github.com/coredns/coredns/plugin/pkg/fall"
 	"github.com/coredns/coredns/plugin/test"
+
+	"github.com/miekg/dns"
 )
 
+const dbRelative = `@ 500 IN SOA ns.example. hostmaster.example. 3 3600 600 86400 300
+@ 500 IN NS ns.example.
+foo 500 IN A 192.0.2.1
+`
+
 func TestFileParse(t *testing.T) {
 	zoneFileName1, rm, err := test.TempFile(".", dbMiekNL)
 	if err != nil {
@@ -22,6 +35,12 @@ func TestFileParse(t *testing.T) {
 	}
 	defer rm()
 
+	zoneFileName3, rm, err := test.TempFile(".", dbRelative)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer rm()
+
 	tests := []struct {
 		inputFileRules      string
 		shouldErr           bool
@@ -41,13 +60,13 @@ func TestFileParse(t *testing.T) {
 			fall.Zero,
 		},
 		{
-			`file ` + zoneFileName2 + ` 10.0.0.0/8`,
+			`file ` + zoneFileName3 + ` 10.0.0.0/8`,
 			false,
 			Zones{Names: []string{"10.in-addr.arpa."}},
 			fall.Zero,
 		},
 		{
-			`file ` + zoneFileName2 + ` example.org. {
+			`file ` + zoneFileName3 + ` example.org. {
 					fallthrough
 				}`,
 			false,
@@ -55,7 +74,7 @@ func TestFileParse(t *testing.T) {
 			fall.Root,
 		},
 		{
-			`file ` + zoneFileName2 + ` example.org. {
+			`file ` + zoneFileName3 + ` example.org. {
 					fallthrough www.example.org
 				}`,
 			false,
@@ -78,7 +97,7 @@ func TestFileParse(t *testing.T) {
 			fall.Zero,
 		},
 		{
-			`file ` + zoneFileName1 + ` example.net. {
+			`file ` + zoneFileName3 + ` example.net. {
 				no_reload
 			}`,
 			true,
@@ -86,7 +105,7 @@ func TestFileParse(t *testing.T) {
 			fall.Zero,
 		},
 		{
-			`file ` + zoneFileName1 + ` example.net. {
+			`file ` + zoneFileName3 + ` example.net. {
 				no_rebloat
 			}`,
 			true,
@@ -120,7 +139,7 @@ func TestFileParse(t *testing.T) {
 }
 
 func TestParseReload(t *testing.T) {
-	name, rm, err := test.TempFile(".", dbMiekNL)
+	name, rm, err := test.TempFile(".", dbRelative)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -144,9 +163,87 @@ func TestParseReload(t *testing.T) {
 
 	for i, test := range tests {
 		c := caddy.NewTestController("dns", test.input)
-		z, _, _ := fileParse(c)
+		z, _, err := fileParse(c)
+		if err != nil {
+			t.Fatal(err)
+		}
 		if x := z.Z["example.org."].ReloadInterval; x != test.reload {
 			t.Errorf("Test %d expected reload to be %s, but got %s", i, test.reload, x)
 		}
 	}
 }
+
+func TestFileParseSOAOrigin(t *testing.T) {
+	for _, owner := range []string{"test", "@", "test."} {
+		for _, explicit := range []bool{false, true} {
+			t.Run(fmt.Sprintf("%s/explicit=%t", owner, explicit), func(t *testing.T) {
+				contents := fmt.Sprintf(`%s 500 IN SOA ns1.outside.com. root.test 3 604800 86400 2419200 604800
+%s 500 IN NS ns1.outside.com.
+foo 500 IN A 1.1.1.1
+`, owner, owner)
+				fileName := filepath.Join(t.TempDir(), "db.test")
+				if err := os.WriteFile(fileName, []byte(contents), 0644); err != nil {
+					t.Fatal(err)
+				}
+				corefile := fmt.Sprintf("file %q", filepath.ToSlash(fileName))
+				if explicit {
+					corefile += " test"
+				}
+				c := caddy.NewTestController("dns", corefile)
+				c.ServerBlockKeys = []string{"test:53"}
+				zones, _, err := fileParse(c)
+				if owner == "test" {
+					if err == nil || !strings.Contains(err.Error(), "SOA owner test.test. that does not match origin test.") {
+						t.Fatalf("expected mismatched SOA error, got %v", err)
+					}
+					return
+				}
+				if err != nil {
+					t.Fatal(err)
+				}
+				f := File{Zones: zones}
+				for _, tc := range []test.Case{
+					{
+						Qname: "foo.test.", Qtype: dns.TypeA, Authoritative: true,
+						Answer: []dns.RR{test.A("foo.test. 500 IN A 1.1.1.1")},
+						Ns:     []dns.RR{test.NS("test. 500 IN NS ns1.outside.com.")},
+					},
+					{
+						Qname: "bar.test.", Qtype: dns.TypeA, Rcode: dns.RcodeNameError, Authoritative: true,
+						Ns: []dns.RR{test.SOA("test. 500 IN SOA ns1.outside.com. root.test.test. 3 604800 86400 2419200 604800")},
+					},
+				} {
+					rec := dnstest.NewRecorder(&test.ResponseWriter{})
+					if _, err := f.ServeDNS(context.Background(), rec, tc.Msg()); err != nil {
+						t.Fatal(err)
+					}
+					if err := test.SortAndCheck(rec.Msg, tc); err != nil {
+						t.Error(err)
+					}
+				}
+			})
+		}
+	}
+}
+
+func TestFileParseRelativeZones(t *testing.T) {
+	fileName, rm, err := test.TempFile(".", dbRelative)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer rm()
+	c := caddy.NewTestController("dns", "file "+fileName+" example.org example.net")
+	zones, _, err := fileParse(c)
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, origin := range []string{"example.org.", "example.net."} {
+		z := zones.Z[origin]
+		if z == nil || z.SOA == nil || z.SOA.Hdr.Name != origin {
+			t.Fatalf("missing SOA at %s", origin)
+		}
+		if _, ok := z.Search("foo." + origin); !ok {
+			t.Errorf("missing relative A record in %s", origin)
+		}
+	}
+}