BUG: SPF flattening removes redirect= modifiers that an "all" mechanism ignores

commands/types/dnscontrol.d.ts

@@ -3465,6 +3465,7 @@ declare function SOA(name: string, ns: string, mbox: string, refresh: number, re
  * * `txtMaxSize` The maximum size for each TXT record. Values over 255 will result in [multiple strings][multi-string]. General recommendation is to [not go higher than 450][record-size] so that DNS responses will still fit in a UDP packet. (Optional. Default: `"255"`)
  * * `parts:` The individual parts of the SPF settings.
  * * `flatten:` Which includes should be inlined. For safety purposes the flattening is done on an opt-in basis. If `"*"` is listed, all includes will be flattened... this might create more problems than is solves due to length limitations.
+ * * `keepIgnoredRedirects:` Inlining an include can move its `redirect=` modifier into a record that has an `all` mechanism, where [RFC 7208 Section 6.1](https://tools.ietf.org/html/rfc7208#section-6.1) requires it to be ignored. Such modifiers are removed. Set this to `true` to keep them. (Optional. Default: `false`)
  *
  * [multi-string]: https://tools.ietf.org/html/rfc4408#section-3.1.3 [record-size]: https://tools.ietf.org/html/rfc4408#section-3.1.4
  *
@@ -3590,7 +3591,7 @@ declare function SOA(name: string, ns: string, mbox: string, refresh: number, re
  *
  * @see https://docs.dnscontrol.org/language-reference/domain-modifiers/spf_builder
  */
-declare function SPF_BUILDER(opts: { label?: string; overflow?: string; overhead1?: string; raw?: string; ttl?: Duration; txtMaxSize?: number; parts: string[]; flatten?: string[] }): DomainModifier;
+declare function SPF_BUILDER(opts: { label?: string; overflow?: string; overhead1?: string; raw?: string; ttl?: Duration; txtMaxSize?: number; parts: string[]; flatten?: string[]; keepIgnoredRedirects?: boolean }): DomainModifier;
 
 /**
  * `SRV` adds a [Service locator record](https://www.rfc-editor.org/rfc/rfc2782) to a domain. The name should be the relative label for the record.

documentation/language-reference/domain-modifiers/SPF_BUILDER.md

@@ -9,6 +9,7 @@ parameters:
   - txtMaxSize
   - parts
   - flatten
+  - keepIgnoredRedirects
 parameters_object: true
 parameter_types:
   label: string?
@@ -19,6 +20,7 @@ parameter_types:
   txtMaxSize: number?
   parts: string[]
   flatten: string[]?
+  keepIgnoredRedirects: boolean?
 ---
 
 DNSControl can optimize the SPF settings on a domain by flattening (inlining) includes and removing duplicates. DNSControl also makes it easier to document your SPF configuration.
@@ -128,6 +130,7 @@ The parameters are:
 * `txtMaxSize` The maximum size for each TXT record. Values over 255 will result in [multiple strings][multi-string]. General recommendation is to [not go higher than 450][record-size] so that DNS responses will still fit in a UDP packet. (Optional. Default: `"255"`)
 * `parts:` The individual parts of the SPF settings.
 * `flatten:` Which includes should be inlined. For safety purposes the flattening is done on an opt-in basis. If `"*"` is listed, all includes will be flattened... this might create more problems than is solves due to length limitations.
+* `keepIgnoredRedirects:` Inlining an include can move its `redirect=` modifier into a record that has an `all` mechanism, where [RFC 7208 Section 6.1](https://tools.ietf.org/html/rfc7208#section-6.1) requires it to be ignored. Such modifiers are removed. Set this to `true` to keep them. (Optional. Default: `false`)
 
 [multi-string]: https://tools.ietf.org/html/rfc4408#section-3.1.3 [record-size]: https://tools.ietf.org/html/rfc4408#section-3.1.4
 

pkg/js/helpers.js

@@ -1247,6 +1247,7 @@ function LOC_builder_push(value, dms) {
 // flatten: A list of domains to be flattened.
 // overhead1: Amout of "buffer room" to reserve on the first item in the spf chain.
 // txtMaxSize: The maximum size for each TXT string. Values over 255 will result in multiple strings (default: '255')
+// keepIgnoredRedirects: Keep redirect= modifiers that flattening moved into a record with an "all" mechanism, which RFC 7208 requires to be ignored. (default: false)
 
 function SPF_BUILDER(value) {
     if (!value.parts || value.parts.length < 2) {
@@ -1290,6 +1291,10 @@ function SPF_BUILDER(value) {
         p.txtMaxSize = value.txtMaxSize;
     }
 
+    if (value.keepIgnoredRedirects) {
+        p.keepIgnoredRedirects = 'true';
+    }
+
     // Generate a TXT record with the metaparameters.
     if (value.ttl) {
         r.push(TXT(value.label, rawspf, p, TTL(value.ttl)));

pkg/normalize/flatten.go

@@ -47,7 +47,11 @@ func flattenSPFs(cfg *models.DNSConfig) []error {
 				}
 			}
 			if flatten, ok := txt.Metadata["flatten"]; ok && strings.HasPrefix(txtTarget, "v=spf1") {
-				rec = rec.Flatten(flatten)
+				var opts []spflib.FlattenOption
+				if txt.Metadata["keepIgnoredRedirects"] == "true" {
+					opts = append(opts, spflib.KeepIgnoredRedirects())
+				}
+				rec = rec.Flatten(flatten, opts...)
 				err = txt.SetTargetTXT(rec.TXT())
 				if err != nil {
 					errs = append(errs, err)

pkg/spflib/flatten.go

@@ -107,8 +107,31 @@ func (s *SPFRecord) split(thisfqdn string, pattern string, nextIdx int, m map[st
 	newRec.split(nextFQDN, pattern, nextIdx+1, m, 0, txtMaxSize)
 }
 
+// FlattenOption alters the behavior of Flatten.
+type FlattenOption func(*flattenOptions)
+
+type flattenOptions struct {
+	keepIgnoredRedirects bool
+}
+
+// KeepIgnoredRedirects makes Flatten retain redirect= modifiers that are
+// ignored because the record contains an "all" mechanism.
+func KeepIgnoredRedirects() FlattenOption {
+	return func(o *flattenOptions) {
+		o.keepIgnoredRedirects = true
+	}
+}
+
 // Flatten optimizes s.
-func (s *SPFRecord) Flatten(spec string) *SPFRecord {
+func (s *SPFRecord) Flatten(spec string, opts ...FlattenOption) *SPFRecord {
+	var o flattenOptions
+	for _, opt := range opts {
+		opt(&o)
+	}
+	return s.flatten(spec, o)
+}
+
+func (s *SPFRecord) flatten(spec string, opts flattenOptions) *SPFRecord {
 	newRec := &SPFRecord{}
 	for _, p := range s.Parts {
 		if p.IncludeRecord == nil {
@@ -119,7 +142,7 @@ func (s *SPFRecord) Flatten(spec string) *SPFRecord {
 			newRec.Parts = append(newRec.Parts, p)
 		} else {
 			// flatten child recursively
-			flattenedChild := p.IncludeRecord.Flatten(spec)
+			flattenedChild := p.IncludeRecord.flatten(spec, opts)
 			// include their parts (skipping final all term)
 			parts := flattenedChild.Parts
 			if n := len(parts); n > 0 && isAllMechanism(parts[n-1].Text) {
@@ -128,17 +151,34 @@ func (s *SPFRecord) Flatten(spec string) *SPFRecord {
 			newRec.Parts = append(newRec.Parts, parts...)
 		}
 	}
+	if !opts.keepIgnoredRedirects {
+		newRec.dropIgnoredRedirects()
+	}
 	return newRec
 }
 
-func isAllMechanism(text string) bool {
-	if text == "" {
-		return false
+// dropIgnoredRedirects removes the redirect= modifiers of s. RFC 7208 Section
+// 6.1 requires them to be ignored when the record has an "all" mechanism.
+func (s *SPFRecord) dropIgnoredRedirects() {
+	if !slices.ContainsFunc(s.Parts, func(p *SPFPart) bool { return isAllMechanism(p.Text) }) {
+		return
 	}
-	if qualifiers[text[0]] {
-		text = text[1:]
+	s.Parts = slices.DeleteFunc(s.Parts, func(p *SPFPart) bool {
+		return strings.HasPrefix(trimQualifier(p.Text), "redirect=")
+	})
+}
+
+// trimQualifier removes the leading qualifier of a term, as Parse does before
+// deciding what the term is.
+func trimQualifier(text string) string {
+	if text != "" && qualifiers[text[0]] {
+		return text[1:]
 	}
-	return strings.EqualFold(text, "all")
+	return text
+}
+
+func isAllMechanism(text string) bool {
+	return strings.EqualFold(trimQualifier(text), "all")
 }
 
 func matchesFlatSpec(spec, fqdn string) bool {

pkg/spflib/flatten_test.go

@@ -127,6 +127,121 @@ func TestFlattenTrailingAll(t *testing.T) {
 	}
 }
 
+func TestFlattenIgnoredRedirect(t *testing.T) {
+	tests := []struct {
+		description string
+		dnsres      fakeResolver
+		input       string
+		spec        string
+		opts        []FlattenOption
+		want        string
+	}{
+		{
+			description: "redirect of a flattened include is dropped when an all mechanism ignores it",
+			dnsres: fakeResolver{
+				"child.example.net": "v=spf1 ip4:1.2.3.4 redirect=other.example.org",
+				"other.example.org": "v=spf1 ip4:9.9.9.9 -all",
+			},
+			input: "v=spf1 include:child.example.net -all",
+			spec:  "child.example.net",
+			want:  "v=spf1 ip4:1.2.3.4 -all",
+		},
+		{
+			description: "redirect is kept when no all mechanism ignores it",
+			dnsres: fakeResolver{
+				"child.example.net": "v=spf1 ip4:1.2.3.4 redirect=other.example.org",
+				"other.example.org": "v=spf1 ip4:9.9.9.9 -all",
+			},
+			input: "v=spf1 include:child.example.net",
+			spec:  "child.example.net",
+			want:  "v=spf1 ip4:1.2.3.4 redirect=other.example.org",
+		},
+		{
+			description: "KeepIgnoredRedirects retains a redirect an all mechanism ignores",
+			dnsres: fakeResolver{
+				"child.example.net": "v=spf1 ip4:1.2.3.4 redirect=other.example.org",
+				"other.example.org": "v=spf1 ip4:9.9.9.9 -all",
+			},
+			input: "v=spf1 include:child.example.net -all",
+			spec:  "child.example.net",
+			opts:  []FlattenOption{KeepIgnoredRedirects()},
+			want:  "v=spf1 ip4:1.2.3.4 redirect=other.example.org -all",
+		},
+		{
+			description: "qualified redirect is dropped when an all mechanism ignores it",
+			dnsres: fakeResolver{
+				"child.example.net": "v=spf1 ip4:1.2.3.4 ~redirect=other.example.org",
+				"other.example.org": "v=spf1 ip4:9.9.9.9 -all",
+			},
+			input: "v=spf1 include:child.example.net -all",
+			spec:  "child.example.net",
+			want:  "v=spf1 ip4:1.2.3.4 -all",
+		},
+		{
+			description: "qualified redirect is kept when no all mechanism ignores it",
+			dnsres: fakeResolver{
+				"child.example.net": "v=spf1 ip4:1.2.3.4 ~redirect=other.example.org",
+				"other.example.org": "v=spf1 ip4:9.9.9.9 -all",
+			},
+			input: "v=spf1 include:child.example.net",
+			spec:  "child.example.net",
+			want:  "v=spf1 ip4:1.2.3.4 ~redirect=other.example.org",
+		},
+		{
+			description: "redirect ignored inside a nested include does not become live in the parent",
+			dnsres: fakeResolver{
+				"a.example.net":     "v=spf1 include:b.example.net ~all",
+				"b.example.net":     "v=spf1 ip4:1.2.3.4 redirect=other.example.org",
+				"other.example.org": "v=spf1 ip4:9.9.9.9 -all",
+			},
+			input: "v=spf1 include:a.example.net",
+			spec:  "a.example.net,b.example.net",
+			want:  "v=spf1 ip4:1.2.3.4",
+		},
+		{
+			description: "redirect that matches the flatten spec is still inlined",
+			dnsres: fakeResolver{
+				"child.example.net": "v=spf1 ip4:1.2.3.4 redirect=other.example.org",
+				"other.example.org": "v=spf1 ip4:9.9.9.9 -all",
+			},
+			input: "v=spf1 include:child.example.net -all",
+			spec:  "*",
+			want:  "v=spf1 ip4:1.2.3.4 ip4:9.9.9.9 -all",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.description, func(t *testing.T) {
+			rec, err := Parse(test.input, test.dnsres)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if got := rec.Flatten(test.spec, test.opts...).TXT(); got != test.want {
+				t.Errorf("got %s want %s", got, test.want)
+			}
+		})
+	}
+}
+
+func TestFlattenOutputParses(t *testing.T) {
+	for _, qualifier := range []string{"", "+", "~", "-", "?"} {
+		t.Run(qualifier+"redirect", func(t *testing.T) {
+			dnsres := fakeResolver{
+				"child.example.net": "v=spf1 ip4:1.2.3.4 " + qualifier + "redirect=other.example.org",
+				"other.example.org": "v=spf1 ip4:9.9.9.9 -all",
+			}
+			rec, err := Parse("v=spf1 include:child.example.net -all", dnsres)
+			if err != nil {
+				t.Fatal(err)
+			}
+			got := rec.Flatten("child.example.net").TXT()
+			if _, err := Parse(got, dnsres); err != nil {
+				t.Errorf("Parse(%q) returned %v", got, err)
+			}
+		})
+	}
+}
+
 // each test is array of strings.
 // first item is unsplit input
 // next is @ spf record