amadan

carlos / platform Public

edge: route53 zone lookup — suffix-walk exact matches, model DNSName start-point in fake

ListHostedZonesByName's DNSName is not a filter, it's the start point of a
listing sorted by zone name with labels reversed. A parent zone (shorter,
sorts earlier) was silently absent from a listing started at the child
fqdn, so the old suffix-match-over-the-listing approach always reported "no
hosted zone" and the wildcard could never issue. hostedZoneID now walks
fqdn's parent suffixes longest-first, probing ListHostedZonesByName per
candidate and accepting only an exact name match — first hit is the most
specific by construction. The fake now models the real start-point/sort
contract instead of ignoring DNSName, which is what hid the bug; a new
fixture covers the immediate-parent-has-its-own-zone case.
Paul Campbell c38835ec1a17463783732ece3a5f241516062a74 parent 0f0c24d
2 files changed, +101 −31
  • internal/edge/route53dns.go +39 −23
  • internal/edge/route53dns_test.go +62 −8
diff --git a/internal/edge/route53dns.go b/internal/edge/route53dns.go
index 45d8d0776..1a735c230 100644
--- a/internal/edge/route53dns.go
+++ b/internal/edge/route53dns.go
@@ -8,9 +8,10 @@ package edge
//
// Same seam as DNSimple (dnsProvider): UPSERT the challenge TXT, wait for
// the change to go INSYNC, and hand back a closure that removes it. The
-// zone owning the record fqdn is discovered via ListHostedZonesByName's
-// suffix match, so per-account parents (which have no zone of their own)
-// resolve to the registered domain's zone.
+// zone owning the record fqdn is discovered by walking fqdn's parent
+// suffixes and probing ListHostedZonesByName per candidate (see
+// hostedZoneID for why), so per-account parents (which have no zone of
+// their own) resolve to the registered domain's zone.
import (
"context"
@@ -105,8 +106,26 @@ func (r *route53DNS) present(ctx context.Context, fqdn, value string) (func(cont
}
// hostedZoneID resolves the hosted zone authoritative for fqdn, caching the
-// lookup: the most specific returned zone whose name equals fqdn or is a
-// parent suffix of it. Ids come back as "/hostedzone/Z123"; we store the
+// lookup.
+//
+// ListHostedZonesByName's DNSName is NOT a filter — it is the start point of
+// a listing sorted by zone name with labels reversed (so "com.example" sorts
+// near other ".example.com" zones). A parent zone of fqdn, being shorter,
+// sorts BEFORE fqdn's own reversed key and would therefore be entirely
+// absent from a listing started at fqdn — silently, since the call still
+// succeeds with zero relevant results. Suffix-matching over that listing (as
+// an earlier version of this function did) never sees the parent and always
+// reports "no hosted zone".
+//
+// So instead we walk fqdn's parent suffixes longest-first (most specific
+// first) and issue one ListHostedZonesByName probe per candidate, using the
+// candidate itself as DNSName. A zone equal to the candidate has a reversed
+// key identical to the start point, so it sorts at the head of that
+// specific listing — meaning pagination is not load-bearing here, we only
+// ever need to look at the zones present in the first page. We accept only
+// a returned zone whose Name is an EXACT match for the candidate; the first
+// hit wins, which is automatically the most specific zone since candidates
+// are tried longest-first. Ids come back as "/hostedzone/Z123"; we store the
// bare id.
func (r *route53DNS) hostedZoneID(ctx context.Context, fqdn string) (string, error) {
r.mu.Lock()
@@ -116,27 +135,24 @@ func (r *route53DNS) hostedZoneID(ctx context.Context, fqdn string) (string, err
}
r.mu.Unlock()
- want := dnsFQDN(fqdn)
- out, err := r.client.ListHostedZonesByName(ctx, &route53.ListHostedZonesByNameInput{DNSName: aws.String(want)})
- if err != nil {
- return "", fmt.Errorf("route53: list hosted zones: %w", err)
- }
- var bestName, bestID string
- for _, hz := range out.HostedZones {
- zn := strings.ToLower(aws.ToString(hz.Name))
- if zn == want || strings.HasSuffix(want, "."+zn) {
- if len(zn) > len(bestName) {
- bestName, bestID = zn, strings.TrimPrefix(aws.ToString(hz.Id), "/hostedzone/")
+ labels := strings.Split(strings.ToLower(strings.TrimSuffix(fqdn, ".")), ".")
+ for i := 1; i <= len(labels)-2; i++ { // a zone has at least two labels
+ cand := dnsFQDN(strings.Join(labels[i:], "."))
+ out, err := r.client.ListHostedZonesByName(ctx, &route53.ListHostedZonesByNameInput{DNSName: aws.String(cand)})
+ if err != nil {
+ return "", fmt.Errorf("route53: list hosted zones: %w", err)
+ }
+ for _, hz := range out.HostedZones {
+ if strings.ToLower(aws.ToString(hz.Name)) == cand {
+ id := strings.TrimPrefix(aws.ToString(hz.Id), "/hostedzone/")
+ r.mu.Lock()
+ r.zoneIDs[fqdn] = id
+ r.mu.Unlock()
+ return id, nil
}
}
}
- if bestID == "" {
- return "", fmt.Errorf("route53: no hosted zone for %s", fqdn)
- }
- r.mu.Lock()
- r.zoneIDs[fqdn] = bestID
- r.mu.Unlock()
- return bestID, nil
+ return "", fmt.Errorf("route53: no hosted zone for %s", fqdn)
}
// waitInSync polls GetChange until the change is INSYNC or the timeout
diff --git a/internal/edge/route53dns_test.go b/internal/edge/route53dns_test.go
index 2d20d5763..5e42ff920 100644
--- a/internal/edge/route53dns_test.go
+++ b/internal/edge/route53dns_test.go
@@ -2,6 +2,7 @@ package edge
import (
"context"
+ "sort"
"strings"
"testing"
@@ -16,10 +17,37 @@ type fakeRoute53 struct {
changes []string // "ACTION|name|value"
}
+// route53SortKey mirrors the real ListHostedZonesByName ordering: zone names
+// are compared with their dot-separated labels reversed (so "tito.io."
+// becomes "io.tito"), which groups zones under shared parents together.
+func route53SortKey(name string) string {
+ labels := strings.Split(strings.ToLower(strings.TrimSuffix(name, ".")), ".")
+ rev := make([]string, len(labels))
+ for i, l := range labels {
+ rev[len(labels)-1-i] = l
+ }
+ return strings.Join(rev, ".")
+}
+
+// ListHostedZonesByName models the real API's contract: DNSName is not a
+// filter, it is the start point of a listing sorted by route53SortKey. Only
+// zones whose sort key is >= the start key are returned — a real, and
+// initially unnoticed, source of the Critical bug this fake now exists to
+// catch: a parent zone of the query fqdn sorts BEFORE that fqdn's own key
+// and is silently absent from a listing started at the fqdn itself.
func (f *fakeRoute53) ListHostedZonesByName(_ context.Context, in *route53.ListHostedZonesByNameInput, _ ...func(*route53.Options)) (*route53.ListHostedZonesByNameOutput, error) {
- out := &route53.ListHostedZonesByNameOutput{}
+ start := route53SortKey(aws.ToString(in.DNSName))
+ type entry struct{ name, id, key string }
+ var entries []entry
for name, id := range f.zones {
- out.HostedZones = append(out.HostedZones, r53types.HostedZone{Name: aws.String(name), Id: aws.String("/hostedzone/" + id)})
+ entries = append(entries, entry{name: name, id: id, key: route53SortKey(name)})
+ }
+ sort.Slice(entries, func(i, j int) bool { return entries[i].key < entries[j].key })
+ out := &route53.ListHostedZonesByNameOutput{}
+ for _, e := range entries {
+ if e.key >= start {
+ out.HostedZones = append(out.HostedZones, r53types.HostedZone{Name: aws.String(e.name), Id: aws.String("/hostedzone/" + e.id)})
+ }
}
return out, nil
}
@@ -35,12 +63,22 @@ func (f *fakeRoute53) GetChange(_ context.Context, _ *route53.GetChangeInput, _
return &route53.GetChangeOutput{ChangeInfo: &r53types.ChangeInfo{Status: r53types.ChangeStatusInsync}}, nil
}
-// The fqdn seam: the backend must find the most specific zone that is a
-// parent of the record fqdn — tito.io owns _acme-challenge.go.tito.io when
-// go.tito.io has no zone of its own, and the value must be quoted (Route53
-// stores TXT values as quoted character-strings).
+// The fqdn seam: the backend must find the zone that is a parent of the
+// record fqdn — tito.io owns _acme-challenge.go.tito.io when go.tito.io has
+// no zone of its own, and the value must be quoted (Route53 stores TXT
+// values as quoted character-strings). "example.com." is an unrelated
+// second zone in the account, present to prove the fake's DNSName
+// start-point filtering (and the real code's exact-match walk) don't get
+// tripped up by an unrelated zone sharing the account.
+//
+// This fixture is also the regression case for the Critical bug: tito.io.'s
+// route53SortKey ("io.tito") sorts BEFORE _acme-challenge.go.tito.io.'s own
+// key ("io.tito.go._acme-challenge"), so the old implementation — which
+// called ListHostedZonesByName with the full fqdn as DNSName and then
+// suffix-matched the results — never saw tito.io. in the listing and always
+// returned "no hosted zone" here.
func TestRoute53PresentUpsertsAndCleansUp(t *testing.T) {
- fake := &fakeRoute53{zones: map[string]string{"tito.io.": "Z1", "io.": "Z0"}}
+ fake := &fakeRoute53{zones: map[string]string{"tito.io.": "Z1", "example.com.": "Z9"}}
r := &route53DNS{client: fake, zoneIDs: map[string]string{}}
cleanup, err := r.present(context.Background(), "_acme-challenge.go.tito.io", "txtvalue")
@@ -57,7 +95,23 @@ func TestRoute53PresentUpsertsAndCleansUp(t *testing.T) {
t.Fatalf("changes after cleanup = %v", fake.changes)
}
if got := r.zoneIDs["_acme-challenge.go.tito.io"]; got != "Z1" {
- t.Fatalf("cached zone id = %q, want Z1 (most specific parent zone)", got)
+ t.Fatalf("cached zone id = %q, want Z1 (parent zone tito.io.)", got)
+ }
+}
+
+// TestRoute53PrefersMostSpecificZone covers the case where the challenge
+// fqdn's immediate parent (go.tito.io.) has its own hosted zone alongside
+// the grandparent (tito.io.) — the more specific zone must win, because the
+// candidate walk tries go.tito.io. before tito.io. and returns on first hit.
+func TestRoute53PrefersMostSpecificZone(t *testing.T) {
+ fake := &fakeRoute53{zones: map[string]string{"tito.io.": "Z1", "go.tito.io.": "Z2"}}
+ r := &route53DNS{client: fake, zoneIDs: map[string]string{}}
+
+ if _, err := r.present(context.Background(), "_acme-challenge.go.tito.io", "txtvalue"); err != nil {
+ t.Fatalf("present: %v", err)
+ }
+ if got := r.zoneIDs["_acme-challenge.go.tito.io"]; got != "Z2" {
+ t.Fatalf("cached zone id = %q, want Z2 (most specific parent zone go.tito.io.)", got)
}
}