From 9fa71640815da2d93f7e7b48482ac64c3ca27425 Mon Sep 17 00:00:00 2001 From: Craig Peterson Date: Mon, 20 Mar 2017 14:20:02 -0600 Subject: [PATCH] Validation Refactor (#52) * Cleaning up (and enforcing) validation * Some more style fixes to validation * comment --- main.go | 13 ++- normalize/validate.go | 183 +++++++++++++++++-------------------- normalize/validate_test.go | 34 ++----- 3 files changed, 102 insertions(+), 128 deletions(-) diff --git a/main.go b/main.go index aba2c803b..a2b14ae6b 100644 --- a/main.go +++ b/main.go @@ -90,8 +90,17 @@ func main() { errs := normalize.NormalizeAndValidateConfig(dnsConfig) if len(errs) > 0 { fmt.Printf("%d Validation errors:\n", len(errs)) - for i, err := range errs { - fmt.Printf("%d: %s\n", i+1, err) + fatal := false + for _, err := range errs { + if _, ok := err.(normalize.Warning); ok { + fmt.Printf("WARNING: %s\n", err) + } else { + fatal = true + fmt.Printf("ERROR: %s\n", err) + } + } + if fatal { + log.Fatal("Exiting due to validation errors") } } diff --git a/normalize/validate.go b/normalize/validate.go index b84cfc634..1df2aae23 100644 --- a/normalize/validate.go +++ b/normalize/validate.go @@ -11,30 +11,8 @@ import ( "github.com/miekg/dns/dnsutil" ) -// Returns false if label does not validate. -func assert_no_enddot(label string) error { - if label == "@" { - return nil - } - if len(label) < 1 { - return fmt.Errorf("WARNING: null label.") - } - if label[len(label)-1] == '.' { - return fmt.Errorf("WARNING: label (%v) ends with a (.)", label) - } - return nil -} - -// Returns false if label does not validate. -func assert_no_underscores(label string) error { - if strings.ContainsRune(label, '_') { - return fmt.Errorf("WARNING: label (%v) contains an underscore", label) - } - return nil -} - // Returns false if target does not validate. -func assert_valid_ipv4(label string) error { +func checkIPv4(label string) error { if net.ParseIP(label).To4() == nil { return fmt.Errorf("WARNING: target (%v) is not an IPv4 address", label) } @@ -42,31 +20,31 @@ func assert_valid_ipv4(label string) error { } // Returns false if target does not validate. -func assert_valid_ipv6(label string) error { +func checkIPv6(label string) error { if net.ParseIP(label).To16() == nil { return fmt.Errorf("WARNING: target (%v) is not an IPv6 address", label) } return nil } -// assert_valid_cname_target returns 1 if target is not valid for cnames. -func assert_valid_target(target string) error { +// make sure target is valid reference for cnames, mx, etc. +func checkTarget(target string) error { if target == "@" { return nil } if len(target) < 1 { - return fmt.Errorf("WARNING: null target.") + return fmt.Errorf("empty target") } // If it containts a ".", it must end in a ".". if strings.ContainsRune(target, '.') && target[len(target)-1] != '.' { - return fmt.Errorf("WARNING: target (%v) must end with a (.) [Required if target is not single label]", target) + return fmt.Errorf("target (%v) must end with a (.) [Required if target is not single label]", target) } return nil } // validateRecordTypes list of valid rec.Type values. Returns true if this is a real DNS record type, false means it is a pseudo-type used internally. -func validateRecordTypes(rec *models.RecordConfig, domain_name string) error { - var valid_types = map[string]bool{ +func validateRecordTypes(rec *models.RecordConfig, domain string) error { + var validTypes = map[string]bool{ "A": true, "AAAA": true, "CNAME": true, @@ -76,84 +54,104 @@ func validateRecordTypes(rec *models.RecordConfig, domain_name string) error { "NS": true, } - if _, ok := valid_types[rec.Type]; !ok { - return fmt.Errorf("Unsupported record type (%v) domain=%v name=%v", rec.Type, domain_name, rec.Name) + if _, ok := validTypes[rec.Type]; !ok { + return fmt.Errorf("Unsupported record type (%v) domain=%v name=%v", rec.Type, domain, rec.Name) } return nil } -// validateTargets returns true if rec.Target is valid for the rec.Type. -func validateTargets(rec *models.RecordConfig, domain_name string) (errs []error) { +// underscores in names are often used erroneously. They are valid for dns records, but invalid for urls. +// here we list common records expected to have underscores. Anything else containing an underscore will print a warning. +var expectedUnderscores = []string{"_domainkey", "_dmarc"} + +func checkLabel(label string, rType string, domain string) error { + if label == "@" { + return nil + } + if len(label) < 1 { + return fmt.Errorf("empty %s label in %s", rType, domain) + } + if label[len(label)-1] == '.' { + return fmt.Errorf("label %s.%s ends with a (.)", label, domain) + } + + //underscores are warnings + if strings.ContainsRune(label, '_') { + //unless it is in our exclusion list + ok := false + for _, ex := range expectedUnderscores { + if strings.Contains(label, ex) { + ok = true + break + } + } + if !ok { + return Warning{fmt.Errorf("label %s.%s contains an underscore", label, domain)} + } + } + return nil +} + +// checkTargets returns true if rec.Target is valid for the rec.Type. +func checkTargets(rec *models.RecordConfig, domain string) (errs []error) { label := rec.Name target := rec.Target check := func(e error) { if e != nil { - errs = append(errs, e) + err := fmt.Errorf("In %s %s.%s: %s", rec.Type, rec.Name, domain, e.Error()) + if _, ok := e.(Warning); ok { + err = Warning{err} + } + errs = append(errs, err) } } - check(assert_no_enddot(label)) switch rec.Type { case "A": - check(assert_no_underscores(label)) - check(assert_valid_ipv4(target)) + check(checkIPv4(target)) case "AAAA": - check(assert_no_underscores(label)) - check(assert_valid_ipv6(target)) + check(checkIPv6(target)) case "CNAME": - // NOTE(tlim): In theory a CNAME's label should abide by the assertions - // of the record type it points to. - // i.e. a If the CNAME target points at an MX record, the label should - // be validated as if it is an MX record. - // However, that's rather difficult to do, and impossible if the CNAME - // points at a record outside of dnscontrol's control. Therefore we do - // simple hacks to guess the target type. - // Assumption: If the label contains "._domainkey", this is an TXT. - check(assert_valid_target(target)) - if !strings.Contains(label, "._domainkey") { - check(assert_no_underscores(label)) - } + check(checkTarget(target)) case "MX": - check(assert_no_underscores(label)) - check(assert_valid_target(target)) + check(checkTarget(target)) case "NS": - check(assert_no_underscores(label)) - check(assert_valid_target(target)) + check(checkTarget(target)) if label == "@" { check(fmt.Errorf("cannot create NS record for bare domain. Use NAMESERVER instead")) } case "TXT", "IMPORT_TRANSFORM": default: errs = append(errs, fmt.Errorf("Unimplemented record type (%v) domain=%v name=%v", - rec.Type, domain_name, rec.Name)) + rec.Type, domain, rec.Name)) } return } -func transform_cname(target, old_domain, new_domain string) string { - // Canonicalize. If it isn't a FQDN, add the new_domain. - result := dnsutil.AddOrigin(target, old_domain) +func transformCNAME(target, oldDomain, newDomain string) string { + // Canonicalize. If it isn't a FQDN, add the newDomain. + result := dnsutil.AddOrigin(target, oldDomain) if dns.IsFqdn(result) { result = result[:len(result)-1] } - return dnsutil.AddOrigin(result, new_domain) + "." + return dnsutil.AddOrigin(result, newDomain) + "." } // import_transform imports the records of one zone into another, modifying records along the way. -func import_transform(src_domain, dst_domain *models.DomainConfig, transforms []transform.IpConversion, ttl uint32) error { - // Read src_domain.Records, transform, and append to dst_domain.Records: +func importTransform(srcDomain, dstDomain *models.DomainConfig, transforms []transform.IpConversion, ttl uint32) error { + // Read srcDomain.Records, transform, and append to dstDomain.Records: // 1. Skip any that aren't A or CNAMEs. - // 2. Append dest_domainname to the end of the label. - // 3. For CNAMEs, append dest_domainname to the end of the target. + // 2. Append destDomainname to the end of the label. + // 3. For CNAMEs, append destDomainname to the end of the target. // 4. For As, change the target as described the transforms. - for _, rec := range src_domain.Records { - if dst_domain.HasRecordTypeName(rec.Type, rec.NameFQDN) { + for _, rec := range srcDomain.Records { + if dstDomain.HasRecordTypeName(rec.Type, rec.NameFQDN) { continue } newRec := func() *models.RecordConfig { rec2, _ := rec.Copy() rec2.Name = rec2.NameFQDN - rec2.NameFQDN = dnsutil.AddOrigin(rec2.Name, dst_domain.Name) + rec2.NameFQDN = dnsutil.AddOrigin(rec2.Name, dstDomain.Name) if ttl != 0 { rec2.TTL = ttl } @@ -168,12 +166,12 @@ func import_transform(src_domain, dst_domain *models.DomainConfig, transforms [] for _, tr := range trs { r := newRec() r.Target = tr.String() - dst_domain.Records = append(dst_domain.Records, r) + dstDomain.Records = append(dstDomain.Records, r) } case "CNAME": r := newRec() - r.Target = transform_cname(r.Target, src_domain.Name, dst_domain.Name) - dst_domain.Records = append(dst_domain.Records, r) + r.Target = transformCNAME(r.Target, srcDomain.Name, dstDomain.Name) + dstDomain.Records = append(dstDomain.Records, r) case "MX", "NS", "TXT": // Not imported. continue @@ -195,12 +193,15 @@ func deleteImportTransformRecords(domain *models.DomainConfig) { } } -func NormalizeAndValidateConfig(config *models.DNSConfig) (errs []error) { - // TODO(tlim): Before any changes are made, we should check the rules - // such as MX/CNAME/NS .Target must be a single word, "@", or FQDN. - // Validate and normalize - for _, domain := range config.Domains { +// Warning is a wrapper around error that can be used to indicate it should not +// stop execution, but is still likely a problem. +type Warning struct { + error +} +func NormalizeAndValidateConfig(config *models.DNSConfig) (errs []error) { + + for _, domain := range config.Domains { // Normalize Nameservers. for _, ns := range domain.Nameservers { ns.Name = dnsutil.AddOrigin(ns.Name, domain.Name) @@ -216,7 +217,10 @@ func NormalizeAndValidateConfig(config *models.DNSConfig) (errs []error) { if err := validateRecordTypes(rec, domain.Name); err != nil { errs = append(errs, err) } - if errs2 := validateTargets(rec, domain.Name); errs2 != nil { + if err := checkLabel(rec.Name, rec.Type, domain.Name); err != nil { + errs = append(errs, err) + } + if errs2 := checkTargets(rec, domain.Name); errs2 != nil { errs = append(errs, errs2...) } @@ -238,7 +242,7 @@ func NormalizeAndValidateConfig(config *models.DNSConfig) (errs []error) { errs = append(errs, err) continue } - err = import_transform(config.FindDomain(rec.Target), domain, table, rec.TTL) + err = importTransform(config.FindDomain(rec.Target), domain, table, rec.TTL) if err != nil { errs = append(errs, err) } @@ -256,29 +260,6 @@ func NormalizeAndValidateConfig(config *models.DNSConfig) (errs []error) { errs = append(errs, err) } } - - // Verify all labels are FQDN ending with ".": - for _, domain := range config.Domains { - for _, rec := range domain.Records { - // .Name must NOT end in "." - if rec.Name[len(rec.Name)-1] == '.' { - errs = append(errs, fmt.Errorf("Should not happen: Label ends with '.': %v %v %v %v %v", - domain.Name, rec.Name, rec.NameFQDN, rec.Type, rec.Target)) - } - // .NameFQDN must NOT end in "." - if rec.NameFQDN[len(rec.NameFQDN)-1] == '.' { - errs = append(errs, fmt.Errorf("Should not happen: FQDN ends with '.': %v %v %v %v %v", - domain.Name, rec.Name, rec.NameFQDN, rec.Type, rec.Target)) - } - // .Target MUST end in "." - if rec.Type == "CNAME" || rec.Type == "NS" || rec.Type == "MX" { - if rec.Target[len(rec.Target)-1] != '.' { - errs = append(errs, fmt.Errorf("Should not happen: Target does NOT ends with '.': %v %v %v %v %v", - domain.Name, rec.Name, rec.NameFQDN, rec.Type, rec.Target)) - } - } - } - } return errs } diff --git a/normalize/validate_test.go b/normalize/validate_test.go index 7b88c0855..baa9d89fd 100644 --- a/normalize/validate_test.go +++ b/normalize/validate_test.go @@ -6,7 +6,7 @@ import ( "github.com/StackExchange/dnscontrol/models" ) -func Test_assert_no_enddot(t *testing.T) { +func TestCheckLabel(t *testing.T) { var tests = []struct { experiment string isError bool @@ -16,10 +16,12 @@ func Test_assert_no_enddot(t *testing.T) { {"foo.bar", false}, {"foo.", true}, {"foo.bar.", true}, + {"foo_bar", true}, + {"_domainkey", false}, } for _, test := range tests { - err := assert_no_enddot(test.experiment) + err := checkLabel(test.experiment, "A", "foo.com") checkError(t, err, test.isError, test.experiment) } } @@ -33,24 +35,6 @@ func checkError(t *testing.T, err error, shouldError bool, experiment string) { } } -func Test_assert_no_underscores(t *testing.T) { - var tests = []struct { - experiment string - isError bool - }{ - {"@", false}, - {"foo", false}, - {"_foo", true}, - {"foo_", true}, - {"fo_o", true}, - } - - for _, test := range tests { - err := assert_no_underscores(test.experiment) - checkError(t, err, test.isError, test.experiment) - } -} - func Test_assert_valid_ipv4(t *testing.T) { var tests = []struct { experiment string @@ -63,7 +47,7 @@ func Test_assert_valid_ipv4(t *testing.T) { } for _, test := range tests { - err := assert_valid_ipv4(test.experiment) + err := checkIPv4(test.experiment) checkError(t, err, test.isError, test.experiment) } } @@ -81,7 +65,7 @@ func Test_assert_valid_target(t *testing.T) { } for _, test := range tests { - err := assert_valid_target(test.experiment) + err := checkTarget(test.experiment) checkError(t, err, test.isError, test.experiment) } } @@ -99,7 +83,7 @@ func Test_transform_cname(t *testing.T) { } for _, test := range tests { - actual := transform_cname(test.experiment, "old.com", "new.com") + actual := transformCNAME(test.experiment, "old.com", "new.com") if test.expected != actual { t.Errorf("%v: expected (%v) got (%v)\n", test.experiment, test.expected, actual) } @@ -109,12 +93,12 @@ func Test_transform_cname(t *testing.T) { func TestNSAtRoot(t *testing.T) { //do not allow ns records for @ rec := &models.RecordConfig{Name: "test", Type: "NS", Target: "ns1.name.com."} - errs := validateTargets(rec, "foo.com") + errs := checkTargets(rec, "foo.com") if len(errs) > 0 { t.Error("Expect no error with ns record on subdomain") } rec.Name = "@" - errs = validateTargets(rec, "foo.com") + errs = checkTargets(rec, "foo.com") if len(errs) != 1 { t.Error("Expect error with ns record on @") }