From 65777155be52bca65d4a6d3b2f135d9d3ea79d7c Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Fri, 30 Oct 2015 19:12:33 -0700 Subject: [PATCH] Remove CNAME/DNAME logic Fixes https://github.com/letsencrypt/boulder/issues/1048 --- core/dns.go | 49 --------------------------------- core/dns_test.go | 30 -------------------- core/interfaces.go | 2 -- mocks/mocks.go | 47 ------------------------------- va/validation-authority.go | 47 ++++++------------------------- va/validation-authority_test.go | 38 ++++++++----------------- 6 files changed, 21 insertions(+), 192 deletions(-) diff --git a/core/dns.go b/core/dns.go index d1552f8df..5107bbc80 100644 --- a/core/dns.go +++ b/core/dns.go @@ -225,55 +225,6 @@ func (dnsResolver *DNSResolverImpl) LookupHost(hostname string) ([]net.IP, time. return addrs, rtt, nil } -// LookupCNAME returns the target name if a CNAME record exists for -// the given domain name. If the CNAME does not exist (NXDOMAIN, -// NXRRSET, or a successful response with no CNAME records), it -// returns the empty string and a nil error. -func (dnsResolver *DNSResolverImpl) LookupCNAME(hostname string) (string, time.Duration, error) { - r, rtt, err := dnsResolver.ExchangeOne(hostname, dns.TypeCNAME) - if err != nil { - return "", 0, err - } - if r.Rcode == dns.RcodeNXRrset || r.Rcode == dns.RcodeNameError { - return "", rtt, nil - } - if r.Rcode != dns.RcodeSuccess { - err = fmt.Errorf("DNS failure: %d-%s for CNAME query", r.Rcode, dns.RcodeToString[r.Rcode]) - return "", rtt, err - } - - for _, answer := range r.Answer { - if cname, ok := answer.(*dns.CNAME); ok { - return cname.Target, rtt, nil - } - } - - return "", rtt, nil -} - -// LookupDNAME is LookupCNAME, but for DNAME. -func (dnsResolver *DNSResolverImpl) LookupDNAME(hostname string) (string, time.Duration, error) { - r, rtt, err := dnsResolver.ExchangeOne(hostname, dns.TypeDNAME) - if err != nil { - return "", 0, err - } - if r.Rcode == dns.RcodeNXRrset || r.Rcode == dns.RcodeNameError { - return "", rtt, nil - } - if r.Rcode != dns.RcodeSuccess { - err = fmt.Errorf("DNS failure: %d-%s for DNAME query", r.Rcode, dns.RcodeToString[r.Rcode]) - return "", rtt, err - } - - for _, answer := range r.Answer { - if cname, ok := answer.(*dns.DNAME); ok { - return cname.Target, rtt, nil - } - } - - return "", rtt, nil -} - // LookupCAA sends a DNS query to find all CAA records associated with // the provided hostname. If the response code from the resolver is // SERVFAIL an empty slice of CAA records is returned. diff --git a/core/dns_test.go b/core/dns_test.go index e082d98f6..559fe506a 100644 --- a/core/dns_test.go +++ b/core/dns_test.go @@ -176,9 +176,6 @@ func TestDNSLookupsNoServer(t *testing.T) { _, _, err = obj.LookupHost("letsencrypt.org") test.AssertError(t, err, "No servers") - _, _, err = obj.LookupCNAME("letsencrypt.org") - test.AssertError(t, err, "No servers") - _, _, err = obj.LookupCAA("letsencrypt.org") test.AssertError(t, err, "No servers") } @@ -190,9 +187,6 @@ func TestDNSServFail(t *testing.T) { _, _, err := obj.LookupTXT(bad) test.AssertError(t, err, "LookupTXT didn't return an error") - _, _, err = obj.LookupCNAME(bad) - test.AssertError(t, err, "LookupCNAME didn't return an error") - _, _, err = obj.LookupHost(bad) test.AssertError(t, err, "LookupHost didn't return an error") @@ -262,27 +256,3 @@ func TestDNSLookupCAA(t *testing.T) { test.AssertNotError(t, err, "CAA lookup failed") test.Assert(t, len(caas) > 0, "Should follow CNAME to find CAA") } - -func TestDNSLookupCNAME(t *testing.T) { - obj := NewDNSResolverImpl(time.Second*10, []string{dnsLoopbackAddr}) - - target, _, err := obj.LookupCNAME("cps.letsencrypt.org") - test.AssertNotError(t, err, "CNAME lookup failed") - test.AssertEquals(t, target, "") - - target, _, err = obj.LookupCNAME("cname.letsencrypt.org") - test.AssertNotError(t, err, "CNAME lookup failed") - test.AssertEquals(t, target, "cps.letsencrypt.org.") -} - -func TestDNSLookupDNAME(t *testing.T) { - obj := NewDNSResolverImpl(time.Second*10, []string{dnsLoopbackAddr}) - - target, _, err := obj.LookupDNAME("cps.letsencrypt.org") - test.AssertNotError(t, err, "DNAME lookup failed") - test.AssertEquals(t, target, "") - - target, _, err = obj.LookupDNAME("dname.letsencrypt.org") - test.AssertNotError(t, err, "DNAME lookup failed") - test.AssertEquals(t, target, "cps.letsencrypt.org.") -} diff --git a/core/interfaces.go b/core/interfaces.go index fe2e2c0a4..5e33028e2 100644 --- a/core/interfaces.go +++ b/core/interfaces.go @@ -152,8 +152,6 @@ type DNSResolver interface { ExchangeOne(string, uint16) (*dns.Msg, time.Duration, error) LookupTXT(string) ([]string, time.Duration, error) LookupHost(string) ([]net.IP, time.Duration, error) - LookupCNAME(string) (string, time.Duration, error) - LookupDNAME(string) (string, time.Duration, error) LookupCAA(string) ([]*dns.CAA, time.Duration, error) LookupMX(string) ([]string, time.Duration, error) } diff --git a/mocks/mocks.go b/mocks/mocks.go index a63341d74..db5f00cc0 100644 --- a/mocks/mocks.go +++ b/mocks/mocks.go @@ -53,53 +53,6 @@ func (mock *DNSResolver) LookupHost(hostname string) ([]net.IP, time.Duration, e return []net.IP{ip}, 0, nil } -// LookupCNAME is a mock -func (mock *DNSResolver) LookupCNAME(domain string) (string, time.Duration, error) { - switch strings.TrimRight(domain, ".") { - case "cname-absent.com": - return "absent.com.", 30, nil - case "cname-critical.com": - return "critical.com.", 30, nil - case "cname-present.com", "cname-and-dname.com": - return "cname-target.present.com.", 30, nil - case "cname2-present.com": - return "cname-present.com.", 30, nil - case "a.cname-loop.com": - return "b.cname-loop.com.", 30, nil - case "b.cname-loop.com": - return "a.cname-loop.com.", 30, nil - case "www.caa-loop.com": - // nothing wrong with CNAME, but prevents CAA algorithm from terminating - return "oops.www.caa-loop.com.", 30, nil - case "cname2servfail.com": - return "servfail.com.", 30, nil - case "cname-servfail.com": - return "", 0, fmt.Errorf("SERVFAIL") - case "cname2dname.com": - return "dname2cname.com.", 30, nil - default: - return "", 0, nil - } -} - -// LookupDNAME is a mock -func (mock *DNSResolver) LookupDNAME(domain string) (string, time.Duration, error) { - switch strings.TrimRight(domain, ".") { - case "cname-and-dname.com", "dname-present.com": - return "dname-target.present.com.", time.Minute, nil - case "a.dname-loop.com": - return "b.dname-loop.com.", time.Minute, nil - case "b.dname-loop.com": - return "a.dname-loop.com.", time.Minute, nil - case "dname2cname.com": - return "cname2-present.com.", time.Minute, nil - case "dname-servfail.com": - return "", time.Minute, fmt.Errorf("SERVFAIL") - default: - return "", 0, nil - } -} - // LookupCAA is a mock func (mock *DNSResolver) LookupCAA(domain string) ([]*dns.CAA, time.Duration, error) { var results []*dns.CAA diff --git a/va/validation-authority.go b/va/validation-authority.go index 16d0e38bb..b6d82b25a 100644 --- a/va/validation-authority.go +++ b/va/validation-authority.go @@ -743,19 +743,19 @@ func newCAASet(CAAs []*dns.CAA) *CAASet { } func (va *ValidationAuthorityImpl) getCAASet(hostname string) (*CAASet, error) { - label := strings.TrimRight(hostname, ".") - cnames := 0 + hostname = strings.TrimRight(hostname, ".") + labels := strings.Split(hostname, ".") // See RFC 6844 "Certification Authority Processing" for pseudocode. - for { - if strings.IndexRune(label, '.') == -1 { - // Reached TLD - break - } + // Essentially: check CAA records for the FDQN to be issued, and all parent + // domains. + // We depend on our resolver to snap CNAME and DNAME records. + for i := 0; i < len(labels); i++ { + name := strings.Join(labels[i:len(labels)], ".") // Break if we've reached an ICANN TLD. - if tld, err := publicsuffix.ICANNTLD(label); err != nil || tld == label { + if tld, err := publicsuffix.ICANNTLD(name); err != nil || tld == name { break } - CAAs, caaRtt, err := va.DNSResolver.LookupCAA(label) + CAAs, caaRtt, err := va.DNSResolver.LookupCAA(name) if err != nil { return nil, err } @@ -764,35 +764,6 @@ func (va *ValidationAuthorityImpl) getCAASet(hostname string) (*CAASet, error) { if len(CAAs) > 0 { return newCAASet(CAAs), nil } - cname, cnameRtt, err := va.DNSResolver.LookupCNAME(label) - if err != nil { - return nil, err - } - va.stats.TimingDuration("VA.DNS.RTT.CNAME", cnameRtt, 1.0) - va.stats.Inc("VA.DNS.Rate", 1, 1.0) - dname, dnameRtt, err := va.DNSResolver.LookupDNAME(label) - if err != nil { - return nil, err - } - va.stats.TimingDuration("VA.DNS.RTT.DNAME", dnameRtt, 1.0) - va.stats.Inc("VA.DNS.Rate", 1, 1.0) - if cname == "" && dname == "" { - // Try parent domain (note we confirmed - // earlier that label contains '.') - label = label[strings.IndexRune(label, '.')+1:] - continue - } - if cname != "" && dname != "" && cname != dname { - return nil, errors.New("both CNAME and DNAME exist for " + label) - } - if cname != "" { - label = strings.TrimRight(cname, ".") - } else { - label = strings.TrimRight(dname, ".") - } - if cnames++; cnames > maxCNAME { - return nil, ErrTooManyCNAME - } } // no CAA records found return nil, nil diff --git a/va/validation-authority_test.go b/va/validation-authority_test.go index 21a6b1a55..38b4416e9 100644 --- a/va/validation-authority_test.go +++ b/va/validation-authority_test.go @@ -1065,22 +1065,11 @@ func TestCAAChecking(t *testing.T) { // Critical CAATest{"critical.com", true, false}, CAATest{"nx.critical.com", true, false}, - CAATest{"cname-critical.com", true, false}, - CAATest{"nx.cname-critical.com", true, false}, // Good (absent) CAATest{"absent.com", false, true}, - CAATest{"cname-absent.com", false, true}, - CAATest{"nx.cname-absent.com", false, true}, - CAATest{"cname-nx.com", false, true}, CAATest{"example.co.uk", false, true}, // Good (present) CAATest{"present.com", true, true}, - CAATest{"cname-present.com", true, true}, - CAATest{"cname2-present.com", true, true}, - CAATest{"nx.cname2-present.com", true, true}, - CAATest{"dname-present.com", true, true}, - CAATest{"dname2cname.com", true, true}, - // CNAME to critical } stats, _ := statsd.NewNoopClient() @@ -1089,10 +1078,16 @@ func TestCAAChecking(t *testing.T) { va.IssuerDomain = "letsencrypt.org" for _, caaTest := range tests { present, valid, err := va.CheckCAARecords(core.AcmeIdentifier{Type: "dns", Value: caaTest.Domain}) - test.AssertNotError(t, err, caaTest.Domain) + if err != nil { + t.Errorf("CheckCAARecords error for %s: %s", caaTest.Domain, err) + } fmt.Println(caaTest.Domain, caaTest.Present == present, caaTest.Valid == valid) - test.AssertEquals(t, caaTest.Present, present) - test.AssertEquals(t, caaTest.Valid, valid) + if present != caaTest.Present { + t.Errorf("CheckCAARecords presence mismatch for %s: got %t expected %t", caaTest.Domain, present, caaTest.Present) + } + if valid != caaTest.Valid { + t.Errorf("CheckCAARecords presence mismatch for %s: got %t expected %t", caaTest.Domain, valid, caaTest.Valid) + } } present, valid, err := va.CheckCAARecords(core.AcmeIdentifier{Type: "dns", Value: "servfail.com"}) @@ -1100,18 +1095,9 @@ func TestCAAChecking(t *testing.T) { test.Assert(t, !present, "Present should be false") test.Assert(t, !valid, "Valid should be false") - for _, name := range []string{ - "www.caa-loop.com", - "a.cname-loop.com", - "a.dname-loop.com", - "cname-servfail.com", - "cname2servfail.com", - "dname-servfail.com", - "cname-and-dname.com", - "servfail.com", - } { - _, _, err = va.CheckCAARecords(core.AcmeIdentifier{Type: "dns", Value: name}) - test.AssertError(t, err, name) + _, _, err = va.CheckCAARecords(core.AcmeIdentifier{Type: "dns", Value: "servfail.com"}) + if err == nil { + t.Errorf("Should have returned error on CAA lookup, but did not: %s", "servfail.com") } }