MAINT: Update TXT docs, suggest not using TxtNoLen255 (#1548)

* suggest not using TxtNoLen255

* Rename functions

* wip!

* fixing!

models/quotes.go

@@ -1,8 +1,10 @@
 package models
 
 import (
-	"encoding/csv"
+	"fmt"
 	"strings"
+
+	"github.com/miekg/dns"
 )
 
 // IsQuoted returns true if the string starts and ends with a double quote.
@@ -32,13 +34,14 @@ func StripQuotes(s string) string {
 // `foo`  -> []string{"foo"}
 // `"foo"` -> []string{"foo"}
 // `"foo" "bar"` -> []string{"foo", "bar"}
-// NOTE: it is assumed there is exactly one space between the quotes.
+// `"f"oo" "bar"` -> []string{`f"oo`, "bar"}
+// NOTE: It is assumed there is exactly one space between the quotes.
+// NOTE: This doesn't handle escaped quotes.
+// NOTE: You probably want to use ParseQuotedFields() for RFC 1035-compliant quoting.
 func ParseQuotedTxt(s string) []string {
 	if !IsQuoted(s) {
 		return []string{s}
 	}
-
-	// TODO(tlim): Consider using r, err := ParseQuotedFields(s)
 	return strings.Split(StripQuotes(s), `" "`)
 }
 
@@ -48,13 +51,12 @@ func ParseQuotedFields(s string) ([]string, error) {
 	// Parse according to RFC1035 zonefile specifications.
 	// "foo"  -> one string: `foo``
 	// "foo" "bar"  -> two strings: `foo` and `bar`
-	// Quotes are escaped with \"
 
-	// Implementation note:
+	// The dns package doesn't expose the quote parser. Therefore we create a TXT record and extract the strings.
-	// Fields are space-separated but a field might be quoted.  This is,
+	rr, err := dns.NewRR("example.com. IN TXT " + s)
-	// essentially, a CSV where spaces are the field separator (not
+	if err != nil {
-	// commas). Therefore, we use the CSV parser. See https://stackoverflow.com/a/47489846/71978
+		return nil, fmt.Errorf("could not parse %q TXT: %w", s, err)
-	r := csv.NewReader(strings.NewReader(s))
+	}
-	r.Comma = ' ' // space
+
-	return r.Read()
+	return rr.(*dns.TXT).Txt, nil
 }

models/quotes_test.go

@@ -50,23 +50,20 @@ func TestStripQuotes(t *testing.T) {
 	}
 }
 
-func TestSetTxtParse(t *testing.T) {
+func TestParseQuotedTxt(t *testing.T) {
 	tests := []struct {
 		d1 string
-		e1 string
 		e2 []string
 	}{
-		{`foo`, `foo`, []string{`foo`}},
+		{`foo`, []string{`foo`}},
-		{`"foo"`, `foo`, []string{`foo`}},
+		{`"foo"`, []string{`foo`}},
-		{`"foo bar"`, `foo bar`, []string{`foo bar`}},
+		{`"foo bar"`, []string{`foo bar`}},
-		{`foo bar`, `foo bar`, []string{`foo bar`}},
+		{`foo bar`, []string{`foo bar`}},
-		{`"aaa" "bbb"`, `aaa`, []string{`aaa`, `bbb`}},
+		{`"aaa" "bbb"`, []string{`aaa`, `bbb`}},
+		{`"a"a" "bbb"`, []string{`a"a`, `bbb`}},
 	}
 	for i, test := range tests {
 		ls := ParseQuotedTxt(test.d1)
-		if ls[0] != test.e1 {
-			t.Errorf("%v: expected Target=(%v) got (%v)", i, test.e1, ls[0])
-		}
 		if len(ls) != len(test.e2) {
 			t.Errorf("%v: expected TxtStrings=(%v) got (%v)", i, test.e2, ls)
 		}
@@ -86,6 +83,7 @@ func TestParseQuotedFields(t *testing.T) {
 		{`1 2 3`, []string{`1`, `2`, `3`}},
 		{`1 "2" 3`, []string{`1`, `2`, `3`}},
 		{`1 2 "three 3"`, []string{`1`, `2`, `three 3`}},
+		{`1 2 "qu\"te" "4"`, []string{`1`, `2`, `qu\"te`, `4`}},
 		{`0 issue "letsencrypt.org; validationmethods=dns-01; accounturi=https://acme-v02.api.letsencrypt.org/acme/acct/1234"`, []string{`0`, `issue`, `letsencrypt.org; validationmethods=dns-01; accounturi=https://acme-v02.api.letsencrypt.org/acme/acct/1234`}},
 	}
 	for i, test := range tests {

models/t_txt.go

@@ -1,39 +1,113 @@
 package models
 
-import "strings"
+import (
+	"fmt"
+	"strings"
+)
 
 /*
-Sadly many providers handle TXT records in strange and non-compliant ways.
+Sadly many providers handle TXT records in strange and non-compliant
+ways.  DNSControl has to handle all of them.  Over the years we've
+tried many things.  This explain the current state of the code.
 
-The variations we've seen:
+What are some of these variations?
 
-* a TXT record is a list of strings, each 255-octets or fewer.
+* The RFCs say that a TXT record is a series of strings, each 255-octets
-* a TXT record is a list of strings, all but the last will be 255 octets in length.
+  or fewer.  Yet, most provider APIs only support a single string which
-* a TXT record is a string less than 256 octets.
+  is split into 255-octetl chunks behind the scenes.  Some only support
-* a TXT record is any length, but we'll split it into 255-octet chunks behind the scenes.
+  a single string that is 255-octets or less.
 
-DNSControl stores the string as the user specified, then lets the
+* The RFCs don't say much about the content of the strings.  Some
-provider work out how to handle the given input. There are two
+  providers accept any octet, some only accept ASCII-printable chars,
-opportunties to work with the data:
+  some get confused by TXT records that include backticks, quotes, or
+  whitespace at the end of the string.
 
-1. The provider's AuditRecords() function is called to permit
+DNSControl has tried many different ways to handle all these
-the provider to return an error if it won't be able to handle the
+variations over the years. This is what we found works best:
-contents. For example, it might detect that the string contains a char
-the provider doesn't support (for example, a backtick). This auditing
-is done without any communication to the provider's API. This allows
-such errors to be detected at the "dnscontrol check" stage.  2. When
-performing corrections (GetDomainCorrections()), the provider can slice
-and dice the user's input however they want.
 
-* If the user input is a list of strings:
+Principle 1. Store the string as the user input it.
-  * The strings are stored in RecordConfig.TxtStrings
-* If the user input is a single string:
-  * The strings are stored in RecordConfig.TxtStrings[0]
 
-In both cases, the .Target stores a string that can be used in error
+DNSControl stores the string as the user specified in dnsconfig.js.
-messages and other UI messages. This string should not be used by the
+The user can specify a string of any length, or many individual
-provider in any other way because it has been modified from the user's
+strings of any length.
-original input.
+
+No matter how the user presented the data in dnsconfig.js, the data is
+stored as a list of strings (RecordConfig.TxtStrings []string).  If
+they input 1 string, the list has one element. If the user input many
+individual strings, the list is copied into .TxtStrings.
+
+When we store the data in .TxtStrings there is no length checking. The data is not manipulated.
+
+Principle 2. When downloading zone records, receive the data as appropriate.
+
+When the API returns a TXT record, the provider's code must properly
+store it in the .TxtStrings field of RecordConfig.
+
+We've found most APIs return TXT strings in one of three ways:
+
+  * The API returns a single string: use RecordConfig.SetTargetTXT().
+  * The API returns multiple strings: use RecordConfig.SetTargetTXTs().
+	* (THIS IS RARE) The API returns a single string that must be parsed
+		into multiple strings: The provider is responsible for the
+		parsing.  However, usually the format is "quoted like in RFC 1035"
+		which is vague, but we've implemented it as
+		RecordConfig.SetTargetTXTfromRFC1035Quoted().
+
+If the format is something else, please write the parser as a separate
+function and write unit tests based on actual data received from the
+API.
+
+Principle 3. When sending TXT records to the API, send what the API expects.
+
+The provider's code must decide how to take the list of strings in
+.TxtStrings and present them to the API.
+
+Most providers fall into one of these categories:
+
+	* If the API expects one long string, the provider code joins all
+	  the smaller strings and sends one big string.  Use the helper
+	  function RecordConfig.GetTargetTXTJoined()
+  * If the API expects many strings of any size, the provider code
+	  sends the individual strings. Those strings are accessed as
+	  the array RecordConfig.TxtStrings
+	* (THIS IS RARE) If the API expects multiple strings to be sent as
+	  one long string, quoted RFC 1025-style, call
+	  RecordConfig.GetTargetRFC1035Quoted() and send that string.
+
+Note: If the API expects many strings, each 255-octets or smaller, the
+provider code must split the longer strings into smaller strings.  The
+helper function txtutil.SplitSingleLongTxt(dc.Records) will iterate
+over all TXT records and split out any strings longer than 255 octets.
+Call this once in GetDomainCorrections().  (Yes, this violates
+Principle 1, but we decided it is best to do it once, than provide a
+getter that would re-split the strings on every call.)
+
+Principle 4. Providers can communicate back to DNSControl strings they can't handle.
+
+As mentioned before, some APIs reject TXT records for various reasons:
+Illegal chars, whitespace at the end, etc.  We can't make a flag for
+every variation.  Instead we call the provider's AuditRecords()
+function and it reports if there are any records that it can't
+process.
+
+We've provided many helper functions to make this easier.  Look at any
+of the providers/.../auditrecord.go` files for examples.
+
+The integration tests call AuditRecords() to skip any tests that we
+know will fail.  If one of the integration tests is failing, it is
+often better to update AuditRecords() than to try to figure out why,
+for example, the provider doesn't support backticks in strings.  Don't
+spend a lot of effort trying to fix situations that are rare or will
+not appear in real-world situations.
+
+Companies do update their APIs occasionally. You might want to try
+eliminating the checks one at a time to see if the API has improved.
+Don't feel obligated to do this more than once a year.
+
+Conclusion:
+
+When we follow these 4 principles, and stick with the helper functions
+provided, we're able to handle all the variations.
 
 */
 
@@ -78,20 +152,45 @@ func (rc *RecordConfig) GetTargetTXTJoined() string {
 	return strings.Join(rc.TxtStrings, "")
 }
 
-// SetTargetTXTString is like SetTargetTXT but accepts one big string,
+// SetTargetTXTString is like SetTargetTXTs but accepts one big string,
-// which must be parsed into one or more strings based on how it is quoted.
+// which is parsed into individual strings.
 // Ex: foo             << 1 string
 //     foo bar         << 1 string
 //     "foo bar"       << 1 string
 //     "foo" "bar"     << 2 strings
+//     "f"oo" "bar"    << 2 strings, one has a quote in it
+//
+// BUG: This function doesn't handle escaped quotes ("like \" this").
+//
 // FIXME(tlim): This function is badly named. It obscures the fact
-// that the string is parsed for quotes and should only be used for returns TXTMulti.
+// that the string is parsed for quotes and stores a list of strings.
-// Deprecated: Use SetTargetTXTfromRFC1035Quoted instead.
+//
+// Deprecated: This function has a confusing name. Most providers API
+// return a single string, in which case you should use
+// SetTargetTXT(). If your provider returns multiple strings, use
+// SetTargetTXTs().  If your provider returns a single string that
+// must be parsed to extract the individual strings, use
+// SetTargetTXTfromRFC1035Quoted().  Sadly we have not figured out
+// an integration test that will fail if you chose the wrong function.
+// As a result, we recommend trying SetTargetTXT() before you try
+// SetTargetTXTfromRFC1035Quoted().
 func (rc *RecordConfig) SetTargetTXTString(s string) error {
 	return rc.SetTargetTXTs(ParseQuotedTxt(s))
 }
 
+// SetTargetTXTfromRFC1035Quoted parses a series of quoted strings
+// and sets .TxtStrings based on the result.
+// Note: Most APIs do notThis is rarely used. Try using SetTargetTXT() first.
+// Ex: "foo"        << 1 string
+//     "foo bar"    << 1 string
+//     "foo" "bar"  << 2 strings
+//     foo          << error. No quotes! Did you intend to use SetTargetTXT?
 func (rc *RecordConfig) SetTargetTXTfromRFC1035Quoted(s string) error {
+	if s != "" && s[0] != '"' {
+		// If you get this error, it is likely that you should use
+		// SetTargetTXT() instead of SetTargetTXTfromRFC1035Quoted().
+		return fmt.Errorf("non-quoted string used with SetTargetTXTfromRFC1035Quoted: (%s)", s)
+	}
 	many, err := ParseQuotedFields(s)
 	if err != nil {
 		return err

pkg/recordaudit/txt.go

@@ -57,8 +57,9 @@ func TxtNoDoubleQuotes(records []*models.RecordConfig) error {
 	return nil
 }
 
-// TxtNoLen255 audits TXT records for strings exactly 255 octets long.
+// TxtNoStringsExactlyLen255 audits TXT records for strings exactly 255 octets long.
-func TxtNoLen255(records []*models.RecordConfig) error {
+// This is rare; you probably want to use TxtNoLongStrings() instead.
+func TxtNoStringsExactlyLen255(records []*models.RecordConfig) error {
 	for _, rc := range records {
 
 		if rc.HasFormatIdenticalToTXT() { // TXT and similar:
@@ -73,8 +74,8 @@ func TxtNoLen255(records []*models.RecordConfig) error {
 	return nil
 }
 
-// TxtNoLongStrings audits TXT records for strings that are >255 octets.
+// TxtNoStringsLen256orLonger audits TXT records for strings that are >255 octets.
-func TxtNoLongStrings(records []*models.RecordConfig) error {
+func TxtNoStringsLen256orLonger(records []*models.RecordConfig) error {
 	for _, rc := range records {
 
 		if rc.HasFormatIdenticalToTXT() { // TXT and similar:

providers/cscglobal/auditrecords.go

@@ -20,7 +20,7 @@ func AuditRecords(records []*models.RecordConfig) error {
 		return err
 	} // Needed as of 2022-06-10
 
-	if err := recordaudit.TxtNoLen255(records); err != nil {
+	if err := recordaudit.TxtNoStringsLen256orLonger(records); err != nil {
 		return err
 	} // Needed as of 2022-06-10
 

providers/inwx/auditrecords.go

@@ -14,7 +14,7 @@ func AuditRecords(records []*models.RecordConfig) error {
 	}
 	// Still needed as of 2021-03-01
 
-	if err := recordaudit.TxtNoLen255(records); err != nil {
+	if err := recordaudit.TxtNoStringsExactlyLen255(records); err != nil {
 		return err
 	}
 	// Still needed as of 2021-03-01

providers/msdns/auditrecords.go

@@ -14,7 +14,7 @@ func AuditRecords(records []*models.RecordConfig) error {
 	}
 	// Still needed as of 2021-03-01
 
-	if err := recordaudit.TxtNoLongStrings(records); err != nil {
+	if err := recordaudit.TxtNoStringsLen256orLonger(records); err != nil {
 		return err
 	}
 
@@ -38,10 +38,5 @@ func AuditRecords(records []*models.RecordConfig) error {
 	}
 	// Still needed as of 2021-03-01
 
-	if err := recordaudit.TxtNoLen255(records); err != nil {
-		return err
-	}
-	// Still needed as of 2021-03-01
-
 	return nil
 }