From 70ce16ff237c141528cc272ef5eab0e1f6f2ac62 Mon Sep 17 00:00:00 2001 From: Patrick Gaskin Date: Thu, 14 Nov 2019 11:25:20 -0500 Subject: [PATCH] Fix handling of SRV records with no target (indicated by ".") According to the RFC, the way to indicate that a SRV has no target is to set the target to ".". Some providers do not handle this, or the API returns "" instead of ".". This situation is now tested in the integration tests and all providers (that support this) have been fixed. * Cloudflare: Fix decoding empty SRV target (fixes #561) SRV records with empty (".") targets are now returned as false by the API, which breaks Unmarshaling it into a string. * Use custom type for Cloudflare SRV target Rewrote the SRV target decoding to use a custom type for (un)marshaling, as Cloudflare returns false for null targets, but it requires a single period for giving it one. The target code has also been made more flexible to future API changes with additional normalization. This has been tested with record creation, deletion, and update and works as of 2019-11-05. * DigitalOcean: Fix target FQDN for null targets Without this, dnscontrol thinks an update is needed (.. != .) even when the SRV target is correct. * DNSimple: Fix parsing of null SRV target DNSimple only returns two fields when the target is null. * NameDotCom: Add note about not supporting null SRV targets, skip test * DNSimple: Do not append a . unless we have all three parts Signed-off-by: Amelia Aronsohn * Regenerated provider matrix --- docs/_includes/matrix.html | 8 +- integrationTest/integration_test.go | 5 ++ models/t_srv.go | 8 +- providers/cloudflare/cloudflareProvider.go | 79 ++++++++++++++----- providers/cloudflare/rest.go | 5 +- .../digitalocean/digitaloceanProvider.go | 2 + providers/dnsimple/dnsimpleProvider.go | 6 +- providers/namedotcom/namedotcomProvider.go | 2 +- providers/namedotcom/records.go | 3 + 9 files changed, 90 insertions(+), 28 deletions(-) diff --git a/docs/_includes/matrix.html b/docs/_includes/matrix.html index 87c6833d1..fffa8f9f9 100644 --- a/docs/_includes/matrix.html +++ b/docs/_includes/matrix.html @@ -449,8 +449,8 @@ - - + + @@ -621,8 +621,8 @@ - - + + diff --git a/integrationTest/integration_test.go b/integrationTest/integration_test.go index 654adb40c..5e3bf341e 100644 --- a/integrationTest/integration_test.go +++ b/integrationTest/integration_test.go @@ -459,6 +459,11 @@ func makeTests(t *testing.T) []*TestCase { tc("Change Weight", srv("_sip._tcp", 52, 62, 7, "foo.com."), srv("_sip._tcp", 15, 65, 75, "foo4.com.")), tc("Change Port", srv("_sip._tcp", 52, 62, 72, "foo.com."), srv("_sip._tcp", 15, 65, 75, "foo4.com.")), ) + if *providerToRun == "NAMEDOTCOM" { + t.Log("Skipping SRV Null Target test because provider does not support them") + } else { + tests = append(tests, tc("Null Target", srv("_sip._tcp", 52, 62, 72, "foo.com."), srv("_sip._tcp", 15, 65, 75, "."))) + } } // SSHFP diff --git a/models/t_srv.go b/models/t_srv.go index c6000d050..d08d2506b 100644 --- a/models/t_srv.go +++ b/models/t_srv.go @@ -48,10 +48,14 @@ func (rc *RecordConfig) SetTargetSRVStrings(priority, weight, port, target strin // field as the SRV priority. func (rc *RecordConfig) SetTargetSRVPriorityString(priority uint16, s string) error { part := strings.Fields(s) - if len(part) != 3 { + switch len(part) { + case 3: + return rc.setTargetSRVIntAndStrings(priority, part[0], part[1], part[2]) + case 2: + return rc.setTargetSRVIntAndStrings(priority, part[0], part[1], ".") + default: return errors.Errorf("SRV value does not contain 3 fields: (%#v)", s) } - return rc.setTargetSRVIntAndStrings(priority, part[0], part[1], part[2]) } // SetTargetSRVString is like SetTargetSRV but accepts one big string to be parsed. diff --git a/providers/cloudflare/cloudflareProvider.go b/providers/cloudflare/cloudflareProvider.go index ba375997e..4c546fd91 100644 --- a/providers/cloudflare/cloudflareProvider.go +++ b/providers/cloudflare/cloudflareProvider.go @@ -422,23 +422,66 @@ func newCloudflare(m map[string]string, metadata json.RawMessage) (providers.DNS // Used on the "existing" records. type cfRecData struct { - Name string `json:"name"` - Target string `json:"target"` - Service string `json:"service"` // SRV - Proto string `json:"proto"` // SRV - Priority uint16 `json:"priority"` // SRV - Weight uint16 `json:"weight"` // SRV - Port uint16 `json:"port"` // SRV - Tag string `json:"tag"` // CAA - Flags uint8 `json:"flags"` // CAA - Value string `json:"value"` // CAA - Usage uint8 `json:"usage"` // TLSA - Selector uint8 `json:"selector"` // TLSA - Matching_Type uint8 `json:"matching_type"` // TLSA - Certificate string `json:"certificate"` // TLSA - Algorithm uint8 `json:"algorithm"` // SSHFP - Hash_Type uint8 `json:"type"` // SSHFP - Fingerprint string `json:"fingerprint"` // SSHFP + Name string `json:"name"` + Target cfTarget `json:"target"` + Service string `json:"service"` // SRV + Proto string `json:"proto"` // SRV + Priority uint16 `json:"priority"` // SRV + Weight uint16 `json:"weight"` // SRV + Port uint16 `json:"port"` // SRV + Tag string `json:"tag"` // CAA + Flags uint8 `json:"flags"` // CAA + Value string `json:"value"` // CAA + Usage uint8 `json:"usage"` // TLSA + Selector uint8 `json:"selector"` // TLSA + Matching_Type uint8 `json:"matching_type"` // TLSA + Certificate string `json:"certificate"` // TLSA + Algorithm uint8 `json:"algorithm"` // SSHFP + Hash_Type uint8 `json:"type"` // SSHFP + Fingerprint string `json:"fingerprint"` // SSHFP +} + +// cfTarget is a SRV target. A null target is represented by an empty string, but +// a dot is so acceptable. +type cfTarget string + +// UnmarshalJSON decodes a SRV target from the Cloudflare API. A null target is +// represented by a false boolean or a dot. Domain names are FQDNs without a +// trailing period (as of 2019-11-05). +func (c *cfTarget) UnmarshalJSON(data []byte) error { + var obj interface{} + if err := json.Unmarshal(data, &obj); err != nil { + return err + } + switch v := obj.(type) { + case string: + *c = cfTarget(v) + case bool: + if v { + panic("unknown value for cfTarget bool: true") + } + *c = "" // the "." is already added by nativeToRecord + } + return nil +} + +// MarshalJSON encodes cfTarget for the Cloudflare API. Null targets are +// represented by a single period. +func (c cfTarget) MarshalJSON() ([]byte, error) { + var obj string + switch c { + case "", ".": + obj = "." + default: + obj = string(c) + } + return json.Marshal(obj) +} + +// DNSControlString returns cfTarget normalized to be a FQDN. Null targets are +// represented by a single period. +func (c cfTarget) FQDN() string { + return strings.TrimRight(string(c), ".") + "." } type cfRecord struct { @@ -493,7 +536,7 @@ func (c *cfRecord) nativeToRecord(domain string) *models.RecordConfig { case "SRV": data := *c.Data if err := rc.SetTargetSRV(data.Priority, data.Weight, data.Port, - dnsutil.AddOrigin(data.Target+".", domain)); err != nil { + dnsutil.AddOrigin(data.Target.FQDN(), domain)); err != nil { panic(errors.Wrap(err, "unparsable SRV record received from cloudflare")) } default: // "A", "AAAA", "ANAME", "CAA", "CNAME", "NS", "PTR", "TXT" diff --git a/providers/cloudflare/rest.go b/providers/cloudflare/rest.go index 5850c8c48..40322f83a 100644 --- a/providers/cloudflare/rest.go +++ b/providers/cloudflare/rest.go @@ -131,15 +131,16 @@ func (c *CloudflareApi) createZone(domainName string) (string, error) { func cfSrvData(rec *models.RecordConfig) *cfRecData { serverParts := strings.Split(rec.GetLabelFQDN(), ".") - return &cfRecData{ + c := &cfRecData{ Service: serverParts[0], Proto: serverParts[1], Name: strings.Join(serverParts[2:], "."), Port: rec.SrvPort, Priority: rec.SrvPriority, Weight: rec.SrvWeight, - Target: rec.GetTargetField(), } + c.Target = cfTarget(rec.GetTargetField()) + return c } func cfCaaData(rec *models.RecordConfig) *cfRecData { diff --git a/providers/digitalocean/digitaloceanProvider.go b/providers/digitalocean/digitaloceanProvider.go index 094ba3e0c..1e60790e5 100644 --- a/providers/digitalocean/digitaloceanProvider.go +++ b/providers/digitalocean/digitaloceanProvider.go @@ -199,6 +199,8 @@ func toRc(dc *models.DomainConfig, r *godo.DomainRecord) *models.RecordConfig { // DO returns "@" on read even if fqdn was written. if target == "@" { target = dc.Name + } else if target == "." { + target = "" // don't append another dot to null records } target = dnsutil.AddOrigin(target+".", dc.Name) // FIXME(tlim): The AddOrigin should be a no-op. diff --git a/providers/dnsimple/dnsimpleProvider.go b/providers/dnsimple/dnsimpleProvider.go index b1dd7a321..4a2adf1d7 100644 --- a/providers/dnsimple/dnsimpleProvider.go +++ b/providers/dnsimple/dnsimpleProvider.go @@ -71,7 +71,7 @@ func (c *DnsimpleApi) GetDomainCorrections(dc *models.DomainConfig) ([]*models.C if r.Name == "" { r.Name = "@" } - if r.Type == "CNAME" || r.Type == "MX" || r.Type == "ALIAS" || r.Type == "SRV" { + if r.Type == "CNAME" || r.Type == "MX" || r.Type == "ALIAS" { r.Content += "." } // dnsimple adds these odd txt records that mirror the alias records. @@ -93,6 +93,10 @@ func (c *DnsimpleApi) GetDomainCorrections(dc *models.DomainConfig) ([]*models.C panic(errors.Wrap(err, "unparsable record received from dnsimple")) } case "SRV": + parts := strings.Fields(r.Content) + if len(parts) == 3 { + r.Content += "." + } if err := rec.SetTargetSRVPriorityString(uint16(r.Priority), r.Content); err != nil { panic(errors.Wrap(err, "unparsable record received from dnsimple")) } diff --git a/providers/namedotcom/namedotcomProvider.go b/providers/namedotcom/namedotcomProvider.go index 97db7520a..ea0cd8915 100644 --- a/providers/namedotcom/namedotcomProvider.go +++ b/providers/namedotcom/namedotcomProvider.go @@ -22,7 +22,7 @@ type NameCom struct { var features = providers.DocumentationNotes{ providers.CanUseAlias: providers.Can(), providers.CanUsePTR: providers.Cannot("PTR records are not supported (See Link)", "https://www.name.com/support/articles/205188508-Reverse-DNS-records"), - providers.CanUseSRV: providers.Can(), + providers.CanUseSRV: providers.Can("SRV records with empty targets are not supported"), providers.CanUseTXTMulti: providers.Cannot(), providers.DocCreateDomains: providers.Cannot("New domains require registration"), providers.DocDualHost: providers.Cannot("Apex NS records not editable"), diff --git a/providers/namedotcom/records.go b/providers/namedotcom/records.go index 22cca72b0..e92fc143b 100644 --- a/providers/namedotcom/records.go +++ b/providers/namedotcom/records.go @@ -157,6 +157,9 @@ func (n *NameCom) createRecord(rc *models.RecordConfig, domain string) error { case "TXT": record.Answer = encodeTxt(rc.TxtStrings) case "SRV": + if rc.GetTargetField() == "." { + return errors.New("SRV records with empty targets are not supported (as of 2019-11-05, the API returns 'Parameter Value Error - Invalid Srv Format')") + } record.Answer = fmt.Sprintf("%d %d %v", rc.SrvWeight, rc.SrvPort, rc.GetTargetField()) record.Priority = uint32(rc.SrvPriority) default: