Repository navigation
TRANSIP: Write TXT records unquoted (v5 regression of #2708) - #4824
Conversation
blackshadev
left a comment
There was a problem hiding this comment.
Lgtm. Due to limited time I am not able to test it. But the code looks good to me.
|
Hi @blackshadev, good to cross paths again here. You mentioned you didn't have time to test this, so let me help out by sharing the results of the testing I ran, including against a live TransIP zone. Unit tests ( The golden regeneration touches only TXT Live TransIP zone. Before this change, a After a DKIM and Microsoft/Google verification TXT records healed the same way across ~16 zones. Idempotency. Re-running Happy to provide more detail if useful. |
| func recordToNative(config *models.RecordConfig) (domain.DNSEntry, error) { | ||
| // TransIP stores the TXT "content" field verbatim, so it must be the raw, | ||
| // unquoted value. GetRDATA().String() would emit the RFC presentation form | ||
| // (enclosed in double-quotes), which TransIP would then serve as literal | ||
| // data, breaking SPF/DKIM/DMARC. Use the unquoted target instead. | ||
| content := config.GetRDATA().String() | ||
| if config.Type == "TXT" { | ||
| content = config.GetTargetTXTJoined() | ||
| } | ||
| return domain.DNSEntry{ | ||
| Name: config.Name, | ||
| Expire: int(config.TTL), | ||
| Type: config.Type, | ||
| Content: config.GetRDATA().String(), | ||
| Content: content, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
- Rename "config" to "rc".
- Use a var/switch instead of if/then.
- Less verbose comments.
| func recordToNative(config *models.RecordConfig) (domain.DNSEntry, error) { | |
| // TransIP stores the TXT "content" field verbatim, so it must be the raw, | |
| // unquoted value. GetRDATA().String() would emit the RFC presentation form | |
| // (enclosed in double-quotes), which TransIP would then serve as literal | |
| // data, breaking SPF/DKIM/DMARC. Use the unquoted target instead. | |
| content := config.GetRDATA().String() | |
| if config.Type == "TXT" { | |
| content = config.GetTargetTXTJoined() | |
| } | |
| return domain.DNSEntry{ | |
| Name: config.Name, | |
| Expire: int(config.TTL), | |
| Type: config.Type, | |
| Content: config.GetRDATA().String(), | |
| Content: content, | |
| }, nil | |
| } | |
| func recordToNative(rc *models.RecordConfig) (domain.DNSEntry, error) { | |
| var content string | |
| switch rc.TypeNum { | |
| case dnsv2.TypeTXT: | |
| // TransIP stores the TXT "content" field verbatim. | |
| content = rc.GetTargetTXTJoined() | |
| default: | |
| content = rc.GetRDATA().String() | |
| } | |
| return domain.DNSEntry{ | |
| Name: rc.Name, | |
| Expire: int(rc.TTL), | |
| Type: rc.Type, | |
| Content: content, | |
| }, nil | |
| } |
| @@ -10,6 +10,7 @@ import ( | |||
|
|
|||
| "github.com/DNSControl/dnscontrol/v5/models" | |||
There was a problem hiding this comment.
| "github.com/DNSControl/dnscontrol/v5/models" | |
| dnsv2 "codeberg.org/miekg/dns" | |
| "github.com/DNSControl/dnscontrol/v5/models" |
| // TransIP returns TXT content unquoted (see recordToNative), so parse it as | ||
| // raw data. TxtDontParse only affects TXT; other types are unchanged. | ||
| return dc.NewRecordConfigParse(dc.LabelFromShort(entry.Name), uint32(entry.Expire), entry.Type, entry.Content, nrc.Flags{TxtDontParse: true}) |
There was a problem hiding this comment.
- Style: newline before nrc.Flags{}. (I need to add this to the style guide)
| // TransIP returns TXT content unquoted (see recordToNative), so parse it as | |
| // raw data. TxtDontParse only affects TXT; other types are unchanged. | |
| return dc.NewRecordConfigParse(dc.LabelFromShort(entry.Name), uint32(entry.Expire), entry.Type, entry.Content, nrc.Flags{TxtDontParse: true}) | |
| // TransIP returns TXT content unquoted (see recordToNative), so parse it as raw data. | |
| return dc.NewRecordConfigParse(dc.LabelFromShort(entry.Name), uint32(entry.Expire), entry.Type, entry.Content, | |
| nrc.Flags{TxtDontParse: true}) | |
| } |
|
This is a common bug. Sadly I can't find an automated way to test it. The best I can do is this: https://docs.dnscontrol.org/developer-info/testing-txt-records That said, the code looks good. I just have some style suggestions. |
The v5 RDATA refactor set the TransIP DNSEntry Content to config.GetRDATA().String(), which for TXT emits the RFC presentation form (enclosed in double-quotes). TransIP stores that verbatim, so the quotes end up as literal record data and break SPF/DKIM/DMARC. Write the raw, unquoted value with GetTargetTXTJoined() and read it back with the TxtDontParse flag, mirroring the inwx provider.
42b90a4 to
80850c5
Compare
|
Thanks for the review, @TomOnTime. I've applied all three suggestions:
Squashed into the existing commit and pushed. |
|
Perfect! |
What this fixes
Since v5.0.0 the TransIP provider writes TXT record content in RFC presentation format (enclosed in double-quotes). TransIP stores that content verbatim, so the quote characters become literal data in DNS. Every TXT record managed via TransIP (SPF, DKIM, DMARC, verification tokens) is served with a leading/trailing
", which breaks anything that expects a bare value. For example, an SPF validator reportsNo SPF record found/should include .... This is a regression of #2708 ("TRANSIP: Fix TXT quoting").Root cause, in
providers/transip/transipProvider.gorecordToNative():config.GetRDATA().String()returns the quoted presentation form for TXT. #2708 had set this field to the unquoted form; the v5 RDATA refactor reintroduced the quoting.The fix
Mirror the inwx provider (another provider that stores raw TXT):
GetTargetTXTJoined()for TXT.TxtDontParseflag so the content is parsed as raw data. The flag only affects TXT; other record types are unchanged.Reproduction
The stored value is
"v=spf1 include:_spf.protonmail.ch include:spf.flowmailer.net -all", including the surrounding"characters.dnscontrol preview/pushprint the normalized model (v=spf1 ...), so the diff looks correct; onlydigreveals the literal quotes, and a re-run reports no drift, so the zone stays silently broken.Testing
TestNativeToRecordUsesV3RecordConfig(the TXT case now reflects the raw/unquoted contract) and addedTestRecordToNativeTXTUnquoted(write-side plus round-trip) andTestNativeToRecordHealsLegacyQuotedTXT(a legacy quoted value is read back with quotes intact so a push rewrites it unquoted).recordToNativegolden file with-update; the only change is TXTcontentfields losing their enclosing quotes.go test ./providers/transip/...andgo build ./...pass.previewshows no quoting drift.