From 4dbcd1fa739ffd08063b664f2218ff8ea1a14729 Mon Sep 17 00:00:00 2001 From: Supriya Premkumar Date: Wed, 7 Oct 2020 12:01:25 -0700 Subject: [PATCH] Fix auth zone hosted domain lookups. Changes: 1. When there are multiple route53 hosted top zone and delegated zones within the same account, cert-manager incorrectly uses the top level domain as auth zone for which it doesn't have perms. This DOSes AWS's IAM API. 2. This change adds the best match in determining auth zone while looking up hosted zone IDs. 3. Defines a GetBestMatch util function to perform longest domain matches. 4. Adds test cases 5. Fixes #3353 Signed-off-by: Supriya Premkumar --- pkg/issuer/acme/dns/route53/fixtures_test.go | 10 ++ pkg/issuer/acme/dns/route53/route53.go | 17 +++- pkg/issuer/acme/dns/route53/route53_test.go | 13 +++ pkg/issuer/acme/dns/util/BUILD.bazel | 10 +- pkg/issuer/acme/dns/util/dns.go | 24 +++++ pkg/issuer/acme/dns/util/dns_test.go | 98 ++++++++++++++++++++ 6 files changed, 165 insertions(+), 7 deletions(-) create mode 100644 pkg/issuer/acme/dns/util/dns_test.go diff --git a/pkg/issuer/acme/dns/route53/fixtures_test.go b/pkg/issuer/acme/dns/route53/fixtures_test.go index a8d3b5ff9..236122cda 100644 --- a/pkg/issuer/acme/dns/route53/fixtures_test.go +++ b/pkg/issuer/acme/dns/route53/fixtures_test.go @@ -30,6 +30,16 @@ var ListHostedZonesByNameResponse = ` 10 + + /hostedzone/HIJKLMN + foo.example.com. + D2224C5B-684A-DB4A-BB9A-E09E3BAFEA7A + + Test comment + false + + 10 + true example2.com diff --git a/pkg/issuer/acme/dns/route53/route53.go b/pkg/issuer/acme/dns/route53/route53.go index 9c87a30ac..3d002e521 100644 --- a/pkg/issuer/acme/dns/route53/route53.go +++ b/pkg/issuer/acme/dns/route53/route53.go @@ -245,16 +245,23 @@ func (r *DNSProvider) getHostedZoneID(fqdn string) (string, error) { return "", err } - var hostedZoneID string + zoneToID := make(map[string]string) + var hostedZones []string for _, hostedZone := range resp.HostedZones { // .Name has a trailing dot - if !*hostedZone.Config.PrivateZone && *hostedZone.Name == authZone { - hostedZoneID = *hostedZone.Id - break + if !*hostedZone.Config.PrivateZone { + zoneToID[*hostedZone.Name] = *hostedZone.Id + hostedZones = append(hostedZones, *hostedZone.Name) } } + authZone, err = util.FindBestMatch(fqdn, hostedZones...) + if err != nil { + return "", fmt.Errorf("Zone %s not found in Route 53 for domain %s", authZone, fqdn) + } - if len(hostedZoneID) == 0 { + hostedZoneID, ok := zoneToID[authZone] + + if len(hostedZoneID) == 0 || !ok { return "", fmt.Errorf("Zone %s not found in Route 53 for domain %s", authZone, fqdn) } diff --git a/pkg/issuer/acme/dns/route53/route53_test.go b/pkg/issuer/acme/dns/route53/route53_test.go index 79c844207..59b2b6774 100644 --- a/pkg/issuer/acme/dns/route53/route53_test.go +++ b/pkg/issuer/acme/dns/route53/route53_test.go @@ -105,6 +105,7 @@ func TestRoute53Present(t *testing.T) { mockResponses := MockResponseMap{ "/2013-04-01/hostedzonesbyname": MockResponse{StatusCode: 200, Body: ListHostedZonesByNameResponse}, "/2013-04-01/hostedzone/ABCDEFG/rrset/": MockResponse{StatusCode: 200, Body: ChangeResourceRecordSetsResponse}, + "/2013-04-01/hostedzone/HIJKLMN/rrset/": MockResponse{StatusCode: 200, Body: ChangeResourceRecordSetsResponse}, "/2013-04-01/change/123456": MockResponse{StatusCode: 200, Body: GetChangeResponse}, } @@ -118,6 +119,18 @@ func TestRoute53Present(t *testing.T) { err := provider.Present(domain, "_acme-challenge."+domain+".", keyAuth) assert.NoError(t, err, "Expected Present to return no error") + + subDomain := "foo.example.com" + err = provider.Present(subDomain, "_acme-challenge."+subDomain+".", keyAuth) + assert.NoError(t, err, "Expected Present to return no error") + + nonExistentSubDomain := "bar.foo.example.com" + err = provider.Present(nonExistentSubDomain, nonExistentSubDomain+".", keyAuth) + assert.NoError(t, err, "Expected Present to return no error") + + nonExistentDomain := "baz.com" + err = provider.Present(nonExistentDomain, nonExistentDomain+".", keyAuth) + assert.Error(t, err, "Expected Present to return an error") } func TestAssumeRole(t *testing.T) { diff --git a/pkg/issuer/acme/dns/util/BUILD.bazel b/pkg/issuer/acme/dns/util/BUILD.bazel index d75c2040f..872c3de50 100644 --- a/pkg/issuer/acme/dns/util/BUILD.bazel +++ b/pkg/issuer/acme/dns/util/BUILD.bazel @@ -16,10 +16,16 @@ go_library( go_test( name = "go_default_test", - srcs = ["wait_test.go"], + srcs = [ + "dns_test.go", + "wait_test.go", + ], data = glob(["testdata/**"]), embed = [":go_default_library"], - deps = ["@com_github_miekg_dns//:go_default_library"], + deps = [ + "@com_github_miekg_dns//:go_default_library", + "@com_github_stretchr_testify//assert:go_default_library", + ], ) filegroup( diff --git a/pkg/issuer/acme/dns/util/dns.go b/pkg/issuer/acme/dns/util/dns.go index fc1b18cb7..621bf2ea6 100644 --- a/pkg/issuer/acme/dns/util/dns.go +++ b/pkg/issuer/acme/dns/util/dns.go @@ -33,3 +33,27 @@ func DNS01LookupFQDN(domain string, followCNAME bool, nameservers ...string) (st return fqdn, nil } + +// FindBestMatch returns the longest match for a given domain within a list of domains +func FindBestMatch(query string, domains ...string) (string, error) { + var maxSoFar int + var longest string + + for _, domain := range domains { + if query == domain { + // Found exact match + return domain, nil + } + + maxHere := dns.CompareDomainName(query, domain) + if maxHere > maxSoFar && dns.IsSubDomain(domain, query) { + maxSoFar = maxHere + longest = domain + } + } + + if len(longest) == 0 { + return "", fmt.Errorf("query: %v has no matches", query) + } + return longest, nil +} diff --git a/pkg/issuer/acme/dns/util/dns_test.go b/pkg/issuer/acme/dns/util/dns_test.go new file mode 100644 index 000000000..d167648dd --- /dev/null +++ b/pkg/issuer/acme/dns/util/dns_test.go @@ -0,0 +1,98 @@ +// +skip_license_check + +package util + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" +) + +type input struct { + query string + domains []string +} + +type test struct { + name string + input input + want, got string +} + +var domains = []string{ + "foo.example.com", + "foo.bar.example.com", + "example.com", + "baz.com", +} + +var tests = []*test{ + { + name: "TestExactMatchTLD", + input: input{ + query: "example.com", + domains: domains, + }, + want: "example.com", + }, + { + name: "TestExactMatchSubDomain", + input: input{ + query: "foo.example.com", + domains: domains, + }, + want: "foo.example.com", + }, + { + name: "TestExactMatchSubDomainTwoLevels", + input: input{ + query: "foo.bar.example.com", + domains: domains, + }, + want: "foo.bar.example.com", + }, + { + name: "TestPartialMatchTLD", + input: input{ + query: "baz.example.com", + domains: domains, + }, + want: "example.com", + }, + { + name: "TestPartialMatchSubDomain", + input: input{ + query: "baz.foo.example.com", + domains: domains, + }, + want: "foo.example.com", + }, + { + name: "TestNoMatchReversedOrder", // Negative Test Case + input: input{ + query: "com.example.foo", + domains: domains, + }, + want: "", + }, + { + name: "TestNoMatches", // Negative Test Case + input: input{ + query: "bar.com", + domains: domains, + }, + want: "", + }, +} + +func TestLongestMatches(t *testing.T) { + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + tc.got, _ = FindBestMatch(tc.input.query, tc.input.domains...) + if tc.got != tc.want { + assert.Equal(t, tc.want, tc.got, fmt.Sprintf("Failed: TestCase: %s | Query: %s | Want: %v | Got: %v", tc.name, tc.input.query, tc.want, tc.got)) + } + }) + } +}