From 9a1d81e8d5b5022b71bda5e7fda56250bbb2de0c Mon Sep 17 00:00:00 2001 From: Shuvam Kumar Date: Sat, 1 Aug 2026 11:40:27 +0530 Subject: [PATCH] BUG: SPF flattening removes redirect= modifiers that an "all" mechanism ignores Flattening an include splices the child's terms into the parent. When the child's last term is a redirect= that is not itself flattened, that term is carried over verbatim, so a parent that ends in an "all" mechanism produces: v=spf1 ip4:1.2.3.4 redirect=other.example.org -all RFC 7208 Section 6.1: "Any 'redirect' modifier MUST be ignored if there is an 'all' mechanism anywhere in the record." The modifier above is therefore inert, it is published to live DNS as noise, and spflib.Parse rejects it on the way back in with "redirect=other.example.org must be last item", so DNSControl cannot re-read its own output. Flatten now drops redirect= modifiers from a record that has an "all" mechanism. The check runs at every level of the recursion, not only at the top: a redirect that is already dead inside a nested include must not come back to life when the enclosing record has no "all" of its own. Parse strips a term's qualifier before deciding what the term is, so it accepts +redirect=, ~redirect=, -redirect= and ?redirect= as well. The removal has to look past the qualifier the same way, which isAllMechanism already did; both now share trimQualifier. A redirect= in a record with no "all" is live and is left alone. Flatten takes variadic FlattenOption values so the existing callers in pkg/normalize and docs/flattener keep compiling. spflib.KeepIgnoredRedirects() turns the removal off, reachable from the DSL as the SPF_BUILDER parameter keepIgnoredRedirects (default false), following the metadata pattern already used by flatten/split/overhead1/txtMaxSize. Co-Authored-By: Claude Opus 5 --- commands/types/dnscontrol.d.ts | 3 +- .../domain-modifiers/SPF_BUILDER.md | 3 + pkg/js/helpers.js | 5 + pkg/normalize/flatten.go | 6 +- pkg/spflib/flatten.go | 56 +++++++-- pkg/spflib/flatten_test.go | 115 ++++++++++++++++++ 6 files changed, 178 insertions(+), 10 deletions(-) diff --git a/commands/types/dnscontrol.d.ts b/commands/types/dnscontrol.d.ts index a9664b2b9d..09ea7f53d3 100644 --- a/commands/types/dnscontrol.d.ts +++ b/commands/types/dnscontrol.d.ts @@ -3462,6 +3462,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 * @@ -3587,7 +3588,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. diff --git a/documentation/language-reference/domain-modifiers/SPF_BUILDER.md b/documentation/language-reference/domain-modifiers/SPF_BUILDER.md index 17ee406560..fc8420cd59 100644 --- a/documentation/language-reference/domain-modifiers/SPF_BUILDER.md +++ b/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 diff --git a/pkg/js/helpers.js b/pkg/js/helpers.js index 3d0d04ff14..d337601601 100644 --- a/pkg/js/helpers.js +++ b/pkg/js/helpers.js @@ -1796,6 +1796,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) { @@ -1839,6 +1840,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))); diff --git a/pkg/normalize/flatten.go b/pkg/normalize/flatten.go index 3727a3e0ba..ddaabc91d2 100644 --- a/pkg/normalize/flatten.go +++ b/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) diff --git a/pkg/spflib/flatten.go b/pkg/spflib/flatten.go index 4ca10a942f..1e96b8b85e 100644 --- a/pkg/spflib/flatten.go +++ b/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 { diff --git a/pkg/spflib/flatten_test.go b/pkg/spflib/flatten_test.go index da57e23353..2efd93b044 100644 --- a/pkg/spflib/flatten_test.go +++ b/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