amadan

carlos / platform Public

Merge branch 'wildcard-zone-check' into main

Merge wildcard-zone-check: check the DNS-01 zone before placing a wildcard order (#237)
claude 58be381129188e4cc52e79d8fc56985596f6d78c parent c7e595bparent 0ce4c8e
5 files changed, +190 −5
  • internal/edge/dns.go +24 −1
  • internal/edge/dns_test.go +22 −0
  • internal/edge/route53dns.go +7 −1
  • internal/edge/wildcard.go +29 −3
  • internal/edge/wildcard_zonecheck_test.go +108 −0
diff --git a/internal/edge/dns.go b/internal/edge/dns.go
index 89c152ea6..20c8427f3 100644
--- a/internal/edge/dns.go
+++ b/internal/edge/dns.go
@@ -36,6 +36,23 @@ type dnsProvider interface {
present(ctx context.Context, fqdn, value string) (cleanup func(context.Context) error, err error)
}
+// errNoZone is a provider's answer that none of the zones it can write
+// holds a name. It is a configuration fact, not a transient one: the zone is
+// in another account or another provider, and asking again in an hour will
+// get the same answer.
+var errNoZone = errors.New("no zone for this name in the configured DNS provider")
+
+// zoneChecker is implemented by a provider that can say, writing nothing,
+// whether present could publish a record at fqdn. The wildcard manager asks
+// before it places an ACME order, so a parent the provider cannot answer for
+// costs a zone lookup rather than an order the CA will never see completed.
+//
+// Optional on purpose: a provider without it (a test fake) behaves as
+// before, and present's own error remains the backstop.
+type zoneChecker interface {
+ checkZone(ctx context.Context, fqdn string) error
+}
+
// dnsProviderFromEnv selects the DNS-01 backend:
//
// - unset / CARLOS_DNS_PROVIDER=dnsimple → DNSimple; needs
@@ -128,7 +145,13 @@ func (d *dnsimpleDNS) zoneFor(ctx context.Context, fqdn string) (zone, name stri
return cand, strings.Join(labels[:i], "."), nil
}
}
- return "", "", fmt.Errorf("dnsimple: no zone found for %s", fqdn)
+ return "", "", fmt.Errorf("dnsimple: no zone found for %s: %w", fqdn, errNoZone)
+}
+
+// checkZone satisfies zoneChecker with the same suffix walk present uses.
+func (d *dnsimpleDNS) checkZone(ctx context.Context, fqdn string) error {
+ _, _, err := d.zoneFor(ctx, fqdn)
+ return err
}
func (d *dnsimpleDNS) zoneExists(ctx context.Context, zone string) (bool, error) {
diff --git a/internal/edge/dns_test.go b/internal/edge/dns_test.go
index 2cc0e3dc3..1e49d6c1f 100644
--- a/internal/edge/dns_test.go
+++ b/internal/edge/dns_test.go
@@ -3,6 +3,7 @@ package edge
import (
"context"
"encoding/json"
+ "errors"
"fmt"
"net/http"
"net/http/httptest"
@@ -176,3 +177,24 @@ func TestDNSimplePresentNoZone(t *testing.T) {
t.Fatal("present with no owning zone: want error, got nil")
}
}
+
+// checkZone answers from the same suffix walk present uses, writes nothing,
+// and reports a missing zone as errNoZone.
+func TestDNSimpleCheckZone(t *testing.T) {
+ fake := &fakeDNSimple{zones: map[string]bool{"letsjelly.dev": true}}
+ srv := httptest.NewServer(fake.handler(t))
+ defer srv.Close()
+ t.Setenv("CARLOS_DNSIMPLE_BASE", srv.URL)
+
+ d := newDNSimpleDNS("tok")
+ if err := d.checkZone(context.Background(), "_acme-challenge.letsjelly.dev"); err != nil {
+ t.Fatalf("held zone: %v", err)
+ }
+ err := d.checkZone(context.Background(), "_acme-challenge.bad.carlos.exciting.io")
+ if !errors.Is(err, errNoZone) {
+ t.Fatalf("missing zone: err = %v, want errNoZone", err)
+ }
+ if len(fake.created) != 0 || len(fake.deleted) != 0 {
+ t.Fatalf("checkZone wrote records: created %v, deleted %v", fake.created, fake.deleted)
+ }
+}
diff --git a/internal/edge/route53dns.go b/internal/edge/route53dns.go
index 1a735c230..66f9324e2 100644
--- a/internal/edge/route53dns.go
+++ b/internal/edge/route53dns.go
@@ -152,7 +152,13 @@ func (r *route53DNS) hostedZoneID(ctx context.Context, fqdn string) (string, err
}
}
}
- return "", fmt.Errorf("route53: no hosted zone for %s", fqdn)
+ return "", fmt.Errorf("route53: no hosted zone for %s: %w", fqdn, errNoZone)
+}
+
+// checkZone satisfies zoneChecker with the lookup present uses.
+func (r *route53DNS) checkZone(ctx context.Context, fqdn string) error {
+ _, err := r.hostedZoneID(ctx, fqdn)
+ return err
}
// waitInSync polls GetChange until the change is INSYNC or the timeout
diff --git a/internal/edge/wildcard.go b/internal/edge/wildcard.go
index 343de8e0f..1c5ae8dff 100644
--- a/internal/edge/wildcard.go
+++ b/internal/edge/wildcard.go
@@ -522,10 +522,19 @@ func (w *wildcardManager) pass() time.Duration {
err := w.obtain(ctx, p)
cancel()
if err != nil {
- log.Printf("edge: wildcard *.%s: %v — per-host issuance keeps covering; retrying in 1h", p, err)
- delay = time.Hour
+ // A missing zone is configuration, not a blip: asking hourly would
+ // only repeat the line. It is still re-checked, twice a day, so a
+ // zone added to the provider later is picked up without a restart.
+ retry := time.Hour
+ if errors.Is(err, errNoZone) {
+ retry = noZoneRetry
+ }
+ log.Printf("edge: wildcard *.%s: %v — per-host issuance keeps covering; retrying in %s", p, err, retry)
+ if retry < delay {
+ delay = retry
+ }
w.mu.Lock()
- w.nextAttempt[p] = time.Now().Add(time.Hour)
+ w.nextAttempt[p] = time.Now().Add(retry)
w.mu.Unlock()
continue
}
@@ -577,9 +586,26 @@ func (w *wildcardManager) logStandDown(parents int) {
log.Printf("edge: wildcard: %d parent(s) need issuance but this box is not the deployment's certificate issuer — serving what the issuer publishes", parents)
}
+// noZoneRetry is how long a parent whose challenge zone the DNS provider
+// does not hold waits before it is looked at again (see pass).
+const noZoneRetry = 12 * time.Hour
+
// obtain runs one full ACME DNS-01 issuance for *.parent and swaps the
// result in.
func (w *wildcardManager) obtain(ctx context.Context, parent string) error {
+ // Before the CA hears anything: can the provider publish the challenge
+ // at all? A parent whose zone lives elsewhere (per-account platform
+ // parents on a deployment whose apps domain is not in the provider's
+ // account, the case that found this) would otherwise place an order
+ // every hour, fail at present, and deactivate it again — no harm to the
+ // rate limits, since the authorization is deactivated and never
+ // validated, but a pointless round trip to the CA and a journal line per
+ // parent per hour. The lookup is the one present would make first.
+ if zc, ok := w.dns.(zoneChecker); ok {
+ if err := zc.checkZone(ctx, w.challengeFQDN(parent)); err != nil {
+ return fmt.Errorf("cannot publish the DNS-01 challenge: %w", err)
+ }
+ }
acctKey, err := w.accountKey(ctx)
if err != nil {
return fmt.Errorf("account key: %v", err)
diff --git a/internal/edge/wildcard_zonecheck_test.go b/internal/edge/wildcard_zonecheck_test.go
new file mode 100644
index 000000000..685db6769
--- /dev/null
+++ b/internal/edge/wildcard_zonecheck_test.go
@@ -0,0 +1,108 @@
+package edge
+
+import (
+ "context"
+ "errors"
+ "fmt"
+ "net/http"
+ "net/http/httptest"
+ "sync/atomic"
+ "testing"
+ "time"
+
+ "golang.org/x/crypto/acme/autocert"
+)
+
+// zoneCheckingDNS is fakeDNS plus a zone check that answers from a fixed
+// set of challenge names it can publish.
+type zoneCheckingDNS struct {
+ fakeDNS
+ writable map[string]bool
+ checked []string
+}
+
+func (z *zoneCheckingDNS) checkZone(_ context.Context, fqdn string) error {
+ z.checked = append(z.checked, fqdn)
+ if z.writable[fqdn] {
+ return nil
+ }
+ return fmt.Errorf("fake: no zone found for %s: %w", fqdn, errNoZone)
+}
+
+// countingCA counts every request a manager makes to its ACME directory.
+func countingCA(t *testing.T) (*httptest.Server, *atomic.Int32) {
+ t.Helper()
+ var n atomic.Int32
+ srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ n.Add(1)
+ http.NotFound(w, r) // a 4xx: the ACME client retries 5xx, and this must fail fast
+ }))
+ t.Cleanup(srv.Close)
+ return srv, &n
+}
+
+// A parent whose challenge zone the provider does not hold never reaches the
+// CA: no account, no order, and the error says why (errNoZone).
+func TestObtainChecksTheZoneBeforeOrdering(t *testing.T) {
+ ca, requests := countingCA(t)
+ dns := &zoneCheckingDNS{}
+ w := newWildcardManager(nil, "carlos.exciting.io", func() []string { return nil },
+ nil, "", autocert.DirCache(t.TempDir()), dns)
+ w.directory = ca.URL
+ err := w.obtain(context.Background(), "bad.carlos.exciting.io")
+ if !errors.Is(err, errNoZone) {
+ t.Fatalf("obtain = %v, want an errNoZone", err)
+ }
+ if got := requests.Load(); got != 0 {
+ t.Fatalf("the CA saw %d request(s) for a parent with no zone, want 0", got)
+ }
+ if len(dns.checked) != 1 || dns.checked[0] != "_acme-challenge.bad.carlos.exciting.io" {
+ t.Fatalf("checked %v, want the parent's own challenge name", dns.checked)
+ }
+ if len(dns.presented) != 0 {
+ t.Fatalf("presented %v, want nothing", dns.presented)
+ }
+}
+
+// A writable zone passes the check and the order goes ahead (here, to a CA
+// that refuses — what matters is that it was asked).
+func TestObtainOrdersWhenTheZoneIsThere(t *testing.T) {
+ ca, requests := countingCA(t)
+ dns := &zoneCheckingDNS{writable: map[string]bool{"_acme-challenge.letsjelly.dev": true}}
+ w := newWildcardManager([]string{"letsjelly.dev"}, "", func() []string { return nil },
+ nil, "", autocert.DirCache(t.TempDir()), dns)
+ w.directory = ca.URL
+ ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+ defer cancel()
+ if err := w.obtain(ctx, "letsjelly.dev"); err == nil || errors.Is(err, errNoZone) {
+ t.Fatalf("obtain = %v, want the CA's refusal", err)
+ }
+ if requests.Load() == 0 {
+ t.Fatal("the CA was never asked for a parent whose zone is writable")
+ }
+}
+
+// A no-zone parent backs off for noZoneRetry rather than the hour a
+// transient failure gets, and the pass asks to run again no sooner.
+func TestPassBacksOffANoZoneParentLonger(t *testing.T) {
+ ca, requests := countingCA(t)
+ w := newWildcardManager(nil, "carlos.exciting.io", func() []string { return nil },
+ nil, "", autocert.DirCache(t.TempDir()), &zoneCheckingDNS{})
+ w.directory = ca.URL
+ w.mu.Lock()
+ w.parents = []string{"bad.carlos.exciting.io"}
+ w.mu.Unlock()
+ start := time.Now()
+ if delay := w.pass(); delay != noZoneRetry {
+ t.Fatalf("pass delay = %s, want %s", delay, noZoneRetry)
+ }
+ w.mu.Lock()
+ next := w.nextAttempt["bad.carlos.exciting.io"]
+ w.mu.Unlock()
+ if next.Before(start.Add(noZoneRetry - time.Minute)) {
+ t.Fatalf("nextAttempt = %s, want about %s from now", next.Sub(start), noZoneRetry)
+ }
+ if requests.Load() != 0 {
+ t.Fatal("the pass reached the CA for a parent with no zone")
+ }
+}