From 6cb950cd9ed328fe3dbac5a566c0eceb5c7edcf8 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 14 Oct 2026 13:08:55 +0000 Subject: [PATCH] dns: add ACME challenge tracker and propagation check Exact appends and deletes; values go 10m after last use or on Close. Limits per node, one lock for provider writes. Waits for every authoritative NS and holds a new value until the CA cache has expired. Updates #2137 --- flakehashes.json | 2 +- go.mod | 2 +- hscontrol/dns/challenge.go | 302 ++++++++++++++++++++++ hscontrol/dns/challenge_test.go | 404 ++++++++++++++++++++++++++++++ hscontrol/dns/dnstest/provider.go | 150 +++++++++++ hscontrol/dns/propagation.go | 180 +++++++++++++ hscontrol/dns/propagation_test.go | 154 ++++++++++++ 7 files changed, 1192 insertions(+), 2 deletions(-) create mode 100644 hscontrol/dns/challenge.go create mode 100644 hscontrol/dns/challenge_test.go create mode 100644 hscontrol/dns/dnstest/provider.go create mode 100644 hscontrol/dns/propagation.go create mode 100644 hscontrol/dns/propagation_test.go diff --git a/flakehashes.json b/flakehashes.json index a09a7fc76..35438cd06 100644 --- a/flakehashes.json +++ b/flakehashes.json @@ -1,6 +1,6 @@ { "vendor": { - "goModSum": "sha256-oSB3v9cn+i6QxfOQGzsjCFjVgSOTrpp9mjR7mJmzQYs=", + "goModSum": "sha256-X7Yx0jOestuniDSZ6Aup4MpL6ys5IdF0AOu9rS4Bzyo=", "sri": "sha256-KpiRQbHBAPp2BB+NodeSih2/WIlg52dfoOpYxbrVmBI=" } } diff --git a/go.mod b/go.mod index 4dc87cc9b..80b7d50c9 100644 --- a/go.mod +++ b/go.mod @@ -29,6 +29,7 @@ require ( github.com/libdns/libdns v1.1.1 github.com/libdns/powerdns v0.1.4 github.com/libdns/rfc2136 v1.0.1 + github.com/miekg/dns v1.1.73 github.com/oapi-codegen/runtime v1.7.0 github.com/oauth2-proxy/mockoidc v0.0.0-20240214162133-caebfff84d25 github.com/ory/dockertest/v3 v3.12.0 @@ -178,7 +179,6 @@ require ( github.com/mattn/go-runewidth v0.0.31 // indirect github.com/mdlayher/netlink v1.11.2 // indirect github.com/mdlayher/socket v0.7.0 // indirect - github.com/miekg/dns v1.1.73 // indirect github.com/mitchellh/go-ps v1.0.0 // indirect github.com/mittwald/go-powerdns v0.6.6 // indirect github.com/moby/docker-image-spec v1.3.1 // indirect diff --git a/hscontrol/dns/challenge.go b/hscontrol/dns/challenge.go new file mode 100644 index 000000000..720ff2a0d --- /dev/null +++ b/hscontrol/dns/challenge.go @@ -0,0 +1,302 @@ +package dns + +import ( + "context" + "errors" + "fmt" + "strings" + "sync" + "time" + + "github.com/libdns/libdns" + "github.com/rs/zerolog/log" +) + +var ( + ErrTooManyChallenges = errors.New("too many pending ACME challenges for this node") + ErrChallengeProvider = errors.New("DNS provider failed") + ErrEmptyChallenge = errors.New("empty ACME challenge value") + ErrChallengesClosed = errors.New("ACME challenges are shutting down") +) + +const ( + // ChallengeTTL is the TTL of every challenge record headscale writes. + ChallengeTTL = time.Minute + // challengeLifetime outlives both authorizations of a wildcard order + // and the CA's validation retries. A retry of the same order resends + // the same value, which restarts the clock. + challengeLifetime = 10 * time.Minute + // maxChallengesPerNode covers a wildcard order (two values) racing a + // plain order. It also bounds how many records one node can make + // headscale write. The limit is per node, not per name: a node renames + // itself by changing its hostname, and every new name would come with + // fresh slots. + maxChallengesPerNode = 4 + providerTimeout = 30 * time.Second + // caLookupDelay is how long after Present returns the CA may still + // make its first lookup of the value. + caLookupDelay = 5 * time.Second +) + +// Challenges publishes ACME DNS-01 TXT values through a [Provider] and +// removes them again. Clients never ask for removal, so every value is +// deleted [challengeLifetime], plus the propagation timeout, after it was +// last published, or on [Challenges.Close]. Nothing is persisted: a crash, +// or a failed delete, leaves a few random TXT values behind. +type Challenges struct { + provider Provider + zone string + lifetime time.Duration + wait func(ctx context.Context, fqdn, value string) error + + // mu guards the fields below and is held across provider calls. + // Providers that read-modify-write a record set lose a value when two + // changes to one name overlap, and the changes can come from different + // nodes: one renames away while its values are live, another takes the + // name. Issuance is rare, so one lock for every name is enough. + mu sync.Mutex + values map[challengeKey]*challengeValue + // waiting counts each node's Present calls waiting for propagation, so + // duplicate requests cannot pile up DNS queries. + waiting map[uint64]int + // closed refuses new values once Close has run. Noise connections + // outlive the HTTP server's shutdown, so Present still gets called. + closed bool +} + +type challengeKey struct{ fqdn, value string } + +type challengeValue struct { + // owner is the node the value counts against. + owner uint64 + // record is what the provider returned from AppendRecords; deleting + // exactly it matters for providers that match on TTL or record ID. + record libdns.Record + timer *time.Timer + // expires moves on every use instead of resetting timer: a timer that + // already fired, blocked on the lock behind that use, must not delete + // the value. + expires time.Time + // shownAt is when Present returned, or will return, for this value. + shownAt time.Time +} + +// NewChallenges returns a [Challenges] writing into zone. Present waits up +// to propagationTimeout for the zone's nameservers, found through +// resolvers, to serve a value. +func NewChallenges( + provider Provider, + zone string, + resolvers []string, + propagationTimeout time.Duration, +) *Challenges { + return &Challenges{ + provider: provider, + zone: fqdn(zone), + // A value must outlive its own wait and hold, or it is deleted + // while Present still waits on it, and its hold with it. + lifetime: challengeLifetime + propagationTimeout, + wait: newTXTChecker(resolvers, propagationTimeout).wait, + values: make(map[challengeKey]*challengeValue), + waiting: make(map[uint64]int), + } +} + +// Present publishes value as a TXT record at name for the node owner and +// returns once the zone's nameservers serve it. Values are added next to +// existing ones, never replacing them, and publishing a value again is a +// no-op. +func (c *Challenges) Present(ctx context.Context, owner uint64, name, value string) error { + if value == "" { + return ErrEmptyChallenge + } + + k := challengeKey{fqdn: fqdn(name), value: value} + + err := c.append(ctx, owner, k) + if err != nil { + return err + } + + err = c.wait(ctx, k.fqdn, value) + if err == nil { + select { + case <-ctx.Done(): + err = ctx.Err() + case <-time.After(time.Until(c.show(k))): + } + } + + c.mu.Lock() + defer c.mu.Unlock() + + c.waiting[owner]-- + if c.waiting[owner] == 0 { + delete(c.waiting, owner) + } + + // A failed wait keeps the value until its lifetime ends: deleting it + // at once would let a client that cancels requests in a loop make + // headscale call the provider without bound. A success restarts the + // lifetime, which is the CA's window to validate. + if v, ok := c.values[k]; ok && err == nil { + v.expires = time.Now().Add(c.lifetime) + } + + return err +} + +// append publishes k for owner unless it already is, and counts the caller +// as waiting. +func (c *Challenges) append(ctx context.Context, owner uint64, k challengeKey) error { + c.mu.Lock() + defer c.mu.Unlock() + + if c.closed { + return ErrChallengesClosed + } + + if c.waiting[owner] >= maxChallengesPerNode { + return ErrTooManyChallenges + } + + // A node that resends a value another node published, on a name it has + // taken over, keeps that value alive, so the value moves to its count. + v, ok := c.values[k] + if (!ok || v.owner != owner) && c.count(owner) >= maxChallengesPerNode { + return ErrTooManyChallenges + } + + if ok { + v.owner = owner + v.expires = time.Now().Add(c.lifetime) + c.waiting[owner]++ + + return nil + } + + record := libdns.TXT{ + Name: libdns.RelativeName(k.fqdn, c.zone), + TTL: ChallengeTTL, + Text: k.value, + } + + // The call must finish even if the client goes away, or a record + // could be created that nothing tracks and deletes. + ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), providerTimeout) + defer cancel() + + got, err := c.provider.AppendRecords(ctx, c.zone, []libdns.Record{record}) + if err != nil { + return fmt.Errorf("%w: %w", ErrChallengeProvider, err) + } + + stored := libdns.Record(record) + if len(got) == 1 && got[0].RR().Type == "TXT" && got[0].RR().Data == k.value { + stored = got[0] + } + + c.values[k] = &challengeValue{ + owner: owner, + record: stored, + expires: time.Now().Add(c.lifetime), + timer: time.AfterFunc(c.lifetime, func() { c.expire(k) }), //nolint:contextcheck // outlives the request + } + c.waiting[owner]++ + + return nil +} + +// count returns how many values owner has published; c.mu must be held. +func (c *Challenges) count(owner uint64) int { + n := 0 + + for _, v := range c.values { + if v.owner == owner { + n++ + } + } + + return n +} + +// show reserves when Present may return for k. The client asks the CA to +// validate at once, and the CA's resolvers cache the record set they find +// for its TTL. A resolver that cached this name for an earlier value, such +// as the first of a wildcard order or one left by the node that had the +// name before, would not see a new one, so a new value is held back until +// that cache has expired. The hold counts from every other value on the name, +// so a resend cannot cut its own hold short. Another name, such as the +// node's name before a rename, is cached on its own. +func (c *Challenges) show(k challengeKey) time.Time { + c.mu.Lock() + defer c.mu.Unlock() + + at := time.Now() + + for other, v := range c.values { + expired := v.shownAt.Add(ChallengeTTL + caLookupDelay) + if other.fqdn == k.fqdn && other != k && expired.After(at) { + at = expired + } + } + + if v, ok := c.values[k]; ok { + v.shownAt = at + } + + return at +} + +// expire removes k once it has gone unused for the lifetime. +func (c *Challenges) expire(k challengeKey) { + c.mu.Lock() + defer c.mu.Unlock() + + v, ok := c.values[k] + if !ok { + return + } + + if d := time.Until(v.expires); d > 0 { + v.timer.Reset(d) + + return + } + + c.remove(k) +} + +// remove deletes one tracked value from the provider; c.mu must be held. +// It never deletes by name alone: providers treat an empty value as +// "every record here", which would take out the operator's records and +// the other value of a wildcard order. +func (c *Challenges) remove(k challengeKey) { + v := c.values[k] + v.timer.Stop() + delete(c.values, k) + + ctx, cancel := context.WithTimeout(context.Background(), providerTimeout) + defer cancel() + + _, err := c.provider.DeleteRecords(ctx, c.zone, []libdns.Record{v.record}) + if err != nil { + log.Warn().Err(err).Str("fqdn", k.fqdn).Msg("removing ACME challenge record") + } +} + +// Close deletes every pending value and refuses new ones. +func (c *Challenges) Close() { + c.mu.Lock() + defer c.mu.Unlock() + + c.closed = true + + for k := range c.values { + c.remove(k) + } +} + +func fqdn(name string) string { + return strings.ToLower(strings.TrimSuffix(name, ".")) + "." +} diff --git a/hscontrol/dns/challenge_test.go b/hscontrol/dns/challenge_test.go new file mode 100644 index 000000000..d7555c57d --- /dev/null +++ b/hscontrol/dns/challenge_test.go @@ -0,0 +1,404 @@ +package dns + +import ( + "context" + "errors" + "sync/atomic" + "testing" + "testing/synctest" + "time" + + "github.com/juanfont/headscale/hscontrol/dns/dnstest" + "github.com/libdns/libdns" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + testZone = "ts.example.com." + testName = "_acme-challenge.node.ts.example.com." + // renamedName is testName's node after it changed its hostname. + renamedName = "_acme-challenge.renamed.ts.example.com." + testOwner = 1 + // operatorValue is a record headscale did not create and must keep. + operatorValue = "operator-owned" +) + +var errAPIDown = errors.New("api down") + +func newTestChallenges(p *dnstest.Provider) *Challenges { + return NewChallenges(p, "TS.example.com", nil, 0) +} + +func TestChallengesAppendNotReplace(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + p := &dnstest.Provider{} + p.Add(testName, operatorValue) + c := newTestChallenges(p) + ctx := context.Background() + + // A wildcard order sends two values for one name, one after the other. + require.NoError(t, c.Present(ctx, testOwner, testName, "value-1")) + require.NoError(t, c.Present(ctx, testOwner, "_acme-challenge.NODE.ts.example.com", "value-2")) + assert.ElementsMatch(t, []string{operatorValue, "value-1", "value-2"}, p.Values(testName)) + + // A retry of the same order resends the same value. + require.NoError(t, c.Present(ctx, testOwner, testName, "value-1")) + assert.Len(t, p.Calls(), 2, "resent value must not be appended again") + + for _, call := range p.Calls() { + assert.Equal(t, testZone, call.Zone) + } + + time.Sleep(challengeLifetime) //nolint:forbidigo // synctest fake clock + synctest.Wait() + + assert.Equal(t, []string{operatorValue}, p.Values(testName)) + }) +} + +// TestChallengesResendExtendsLifetime also pins that an expiry timer that +// fired just before a resend, and then waited for the lock, leaves the +// refreshed value alone. +func TestChallengesResendExtendsLifetime(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + p := &dnstest.Provider{} + c := newTestChallenges(p) + ctx := context.Background() + + require.NoError(t, c.Present(ctx, testOwner, testName, "value")) + time.Sleep(challengeLifetime - time.Minute) //nolint:forbidigo // synctest fake clock + require.NoError(t, c.Present(ctx, testOwner, testName, "value")) + + c.expire(challengeKey{fqdn: testName, value: "value"}) + time.Sleep(2 * time.Minute) //nolint:forbidigo // synctest fake clock + synctest.Wait() + assert.Equal(t, []string{"value"}, p.Values(testName)) + + time.Sleep(challengeLifetime) //nolint:forbidigo // synctest fake clock + synctest.Wait() + assert.Empty(t, p.Values(testName)) + }) +} + +func TestChallengesLimitPerNode(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + p := &dnstest.Provider{} + c := newTestChallenges(p) + ctx := context.Background() + + for i := range maxChallengesPerNode { + require.NoError(t, c.Present(ctx, testOwner, testName, "value-"+string(rune('a'+i)))) + } + + err := c.Present(ctx, testOwner, testName, "one-too-many") + require.ErrorIs(t, err, ErrTooManyChallenges) + + // Renaming gives the node a new name, not new slots. + err = c.Present(ctx, testOwner, renamedName, "value") + require.ErrorIs(t, err, ErrTooManyChallenges) + assert.Len(t, p.Calls(), maxChallengesPerNode) + + // Other nodes have their own limit. + require.NoError(t, c.Present(ctx, testOwner+1, "_acme-challenge.other.ts.example.com", "value")) + + // A node that took the name over and resends a value there takes + // over its slot. + require.NoError(t, c.Present(ctx, testOwner+1, testName, "value-a")) + require.NoError(t, c.Present(ctx, testOwner, renamedName, "value")) + + c.Close() + }) +} + +// TestChallengesHoldsNewValueForCACache pins that a second value on a name +// is answered only once the CA's cache of the first has expired: the CA +// validates the second value of a wildcard order seconds after the first. +func TestChallengesHoldsNewValueForCACache(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + c := newTestChallenges(&dnstest.Provider{}) + ctx := context.Background() + start := time.Now() + + require.NoError(t, c.Present(ctx, testOwner, testName, "value-1")) + assert.Equal(t, start, time.Now(), "first value is not held") + + require.NoError(t, c.Present(ctx, testOwner, renamedName, "value-2")) + assert.Equal(t, start, time.Now(), "other names, such as after a rename, are not held") + + require.NoError(t, c.Present(ctx, testOwner, testName, "value-2")) + assert.Equal(t, start.Add(ChallengeTTL+caLookupDelay), time.Now()) + + shown := time.Now() + + require.NoError(t, c.Present(ctx, testOwner, testName, "value-2")) + assert.Equal(t, shown, time.Now(), "a resent value is not held") + + require.NoError(t, c.Present(ctx, testOwner+1, testName, "value-3")) + assert.Equal(t, shown.Add(ChallengeTTL+caLookupDelay), time.Now(), "a node that took the name over is held too") + + ctx, cancel := context.WithCancel(ctx) + cancel() + require.ErrorIs(t, c.Present(ctx, testOwner, testName, "value-4"), context.Canceled) + + c.Close() + }) +} + +// TestChallengesResendKeepsHold pins that resending a value while its hold +// is pending does not cut the hold short. +func TestChallengesResendKeepsHold(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + c := newTestChallenges(&dnstest.Provider{}) + ctx := context.Background() + start := time.Now() + + require.NoError(t, c.Present(ctx, testOwner, testName, "value-1")) + + go func() { _ = c.Present(ctx, testOwner, testName, "value-2") }() + + time.Sleep(time.Second) //nolint:forbidigo // synctest fake clock + require.NoError(t, c.Present(ctx, testOwner, testName, "value-2")) + assert.Equal(t, start.Add(ChallengeTTL+caLookupDelay), time.Now()) + + c.Close() + }) +} + +// TestChallengesFailedWaitKeepsValue pins that a client that went away +// still gets its value appended and tracked, and that a failed or +// cancelled wait leaves the value for its lifetime: deleting at once would +// let a client that cancels in a loop drive unbounded provider calls. +func TestChallengesFailedWaitKeepsValue(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + p := &dnstest.Provider{} + c := newTestChallenges(p) + c.wait = func(ctx context.Context, _, _ string) error { return ctx.Err() } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + require.ErrorIs(t, c.Present(ctx, testOwner, testName, "value"), context.Canceled) + assert.Equal(t, []string{"value"}, p.Values(testName), "append must finish for a gone client") + + // A retry of the same value is not appended again. + require.NoError(t, c.Present(context.Background(), testOwner, testName, "value")) + assert.Len(t, p.Calls(), 1) + + time.Sleep(challengeLifetime) //nolint:forbidigo // synctest fake clock + synctest.Wait() + assert.Empty(t, p.Values(testName)) + }) +} + +// TestChallengesLimitsWaiters pins that duplicate requests for one name +// cannot pile up propagation waits. +func TestChallengesLimitsWaiters(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + p := &dnstest.Provider{} + c := newTestChallenges(p) + + release := make(chan struct{}) + c.wait = func(context.Context, string, string) error { + <-release + + return nil + } + + for range maxChallengesPerNode { + go func() { _ = c.Present(context.Background(), testOwner, testName, "value") }() + } + + synctest.Wait() + require.ErrorIs(t, c.Present(context.Background(), testOwner, testName, "value"), ErrTooManyChallenges) + + close(release) + synctest.Wait() + + c.wait = func(context.Context, string, string) error { return nil } + require.NoError(t, c.Present(context.Background(), testOwner, testName, "value")) + c.Close() + }) +} + +func TestChallengesProviderError(t *testing.T) { + p := &dnstest.Provider{} + p.SetError(errAPIDown) + c := newTestChallenges(p) + + err := c.Present(context.Background(), testOwner, testName, "value") + require.ErrorIs(t, err, ErrChallengeProvider) + + p.SetError(nil) + c.Close() + assert.Len(t, p.Calls(), 1, "failed append must not be tracked for deletion") +} + +func TestChallengesClose(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + p := &dnstest.Provider{} + p.Add(testName, operatorValue) + c := newTestChallenges(p) + ctx := context.Background() + + require.NoError(t, c.Present(ctx, testOwner, testName, "value-1")) + require.NoError(t, c.Present(ctx, testOwner, testName, "value-2")) + c.Close() + + assert.Equal(t, []string{operatorValue}, p.Values(testName)) + + for _, call := range p.Calls() { + assert.NotEmpty(t, call.Value, "a delete without value wipes the record set") + } + }) +} + +// appendGate holds AppendRecords until release is closed. +type appendGate struct { + *dnstest.Provider + + entered, release chan struct{} +} + +func (p appendGate) AppendRecords( + ctx context.Context, + zone string, + recs []libdns.Record, +) ([]libdns.Record, error) { + close(p.entered) + <-p.release + + return p.Provider.AppendRecords(ctx, zone, recs) +} + +// TestChallengesCloseRefusesNewValues pins that Close removes an append +// already under way and refuses later ones: Noise connections outlive +// the HTTP server's shutdown, so Present can still be called. +func TestChallengesCloseRefusesNewValues(t *testing.T) { + p := appendGate{Provider: &dnstest.Provider{}, entered: make(chan struct{}), release: make(chan struct{})} + c := NewChallenges(p, testZone, nil, 0) + + presented := make(chan error) + + go func() { presented <- c.Present(context.Background(), testOwner, testName, "in-flight") }() + + <-p.entered + + closed := make(chan struct{}) + + go func() { + c.Close() + close(closed) + }() + + close(p.release) + <-closed + require.NoError(t, <-presented) + assert.Empty(t, p.Values(testName), "append in flight during Close must be removed") + + err := c.Present(context.Background(), testOwner+1, testName, "late") + require.ErrorIs(t, err, ErrChallengesClosed) + assert.Len(t, p.Calls(), 2, "only the in-flight append and its delete") +} + +func TestChallengesEmptyValue(t *testing.T) { + p := &dnstest.Provider{} + c := newTestChallenges(p) + + require.ErrorIs(t, c.Present(context.Background(), testOwner, testName, ""), ErrEmptyChallenge) + assert.Empty(t, p.Calls()) +} + +// serialProvider records whether two provider calls overlapped. Providers +// that read a record set and, one API round trip later, write back what +// they computed from it, as PowerDNS does, lose one of two overlapping +// changes to a name. +type serialProvider struct { + *dnstest.Provider + + busy, overlapped atomic.Bool + // entered gets a value as each call starts. + entered chan struct{} +} + +func (p *serialProvider) call() func() { + if !p.busy.CompareAndSwap(false, true) { + p.overlapped.Store(true) + } + + p.entered <- struct{}{} + + time.Sleep(20 * time.Millisecond) //nolint:forbidigo // the API round trip + + return func() { p.busy.Store(false) } +} + +func (p *serialProvider) AppendRecords( + ctx context.Context, + zone string, + recs []libdns.Record, +) ([]libdns.Record, error) { + defer p.call()() + + return p.Provider.AppendRecords(ctx, zone, recs) +} + +func (p *serialProvider) DeleteRecords( + ctx context.Context, + zone string, + recs []libdns.Record, +) ([]libdns.Record, error) { + defer p.call()() + + return p.Provider.DeleteRecords(ctx, zone, recs) +} + +// TestChallengesNameTakenOver pins that a node that took over a name +// cannot interleave its provider writes with the removal of a value the +// node that had the name left there, whichever starts first. It runs on +// the real clock: synctest cannot advance time while a goroutine waits on +// a mutex. +func TestChallengesNameTakenOver(t *testing.T) { + p := &serialProvider{Provider: &dnstest.Provider{}, entered: make(chan struct{}, 8)} + c := NewChallenges(p, testZone, nil, 0) + ctx := context.Background() + + for i, name := range []string{testName, renamedName} { + require.NoError(t, c.Present(ctx, testOwner, name, "old")) + <-p.entered + + // testOwner renamed away, and "old" expires while the node that + // took the name publishes there. It was shown long ago, so nothing + // is held. + old := challengeKey{fqdn: name, value: "old"} + + c.mu.Lock() + c.values[old].expires = time.Time{} + c.values[old].shownAt = time.Time{} + c.mu.Unlock() + + first := func() { c.expire(old) } + second := func() { assert.NoError(t, c.Present(ctx, testOwner+1, name, "new")) } + + if i == 1 { + first, second = second, first + } + + done := make(chan struct{}) + + go func() { + first() + close(done) + }() + + <-p.entered + second() + <-done + <-p.entered + + assert.Equal(t, []string{"new"}, p.Values(name)) + } + + assert.False(t, p.overlapped.Load(), "provider calls overlapped") + c.Close() +} diff --git a/hscontrol/dns/dnstest/provider.go b/hscontrol/dns/dnstest/provider.go new file mode 100644 index 000000000..7ab24dc1a --- /dev/null +++ b/hscontrol/dns/dnstest/provider.go @@ -0,0 +1,150 @@ +// Package dnstest provides an in-memory DNS provider for tests. +package dnstest + +import ( + "context" + "slices" + "sync" + + "github.com/libdns/libdns" +) + +// Call is one provider call, one entry per record. +type Call struct { + Method string + Zone string + Name string + Value string +} + +// Provider keeps TXT records in memory and records every call. The zero +// value is ready to use and safe for concurrent use. +type Provider struct { + mu sync.Mutex + records map[string][]string + calls []Call + err error +} + +// SetError makes every following call fail with err; nil clears it. +func (p *Provider) SetError(err error) { + p.mu.Lock() + defer p.mu.Unlock() + + p.err = err +} + +// AppendRecords fails for a done context, as a provider calling an API +// would, so tests catch a caller that lets a request's end abandon a +// write. +func (p *Provider) AppendRecords( + ctx context.Context, + zone string, + recs []libdns.Record, +) ([]libdns.Record, error) { + err := ctx.Err() + if err != nil { + return nil, err + } + + p.mu.Lock() + defer p.mu.Unlock() + + out := make([]libdns.Record, 0, len(recs)) + + for _, rec := range recs { + rr := rec.RR() + name := libdns.AbsoluteName(rr.Name, zone) + p.calls = append(p.calls, Call{Method: "append", Zone: zone, Name: name, Value: rr.Data}) + + if p.err != nil { + return out, p.err + } + + if p.records == nil { + p.records = make(map[string][]string) + } + + p.records[name] = append(p.records[name], rr.Data) + out = append(out, libdns.TXT{Name: rr.Name, TTL: rr.TTL, Text: rr.Data, ProviderData: name}) + } + + return out, nil +} + +// DeleteRecords follows the libdns contract: an empty value deletes every +// value at the name. Tests that keep an unrelated value at the same name +// therefore catch a caller that would wipe a whole record set. Like +// AppendRecords, it fails for a done context. +func (p *Provider) DeleteRecords( + ctx context.Context, + zone string, + recs []libdns.Record, +) ([]libdns.Record, error) { + err := ctx.Err() + if err != nil { + return nil, err + } + + p.mu.Lock() + defer p.mu.Unlock() + + var out []libdns.Record + + for _, rec := range recs { + rr := rec.RR() + name := libdns.AbsoluteName(rr.Name, zone) + p.calls = append(p.calls, Call{Method: "delete", Zone: zone, Name: name, Value: rr.Data}) + + if p.err != nil { + return out, p.err + } + + if rr.Data == "" { + delete(p.records, name) + + out = append(out, rec) + + continue + } + + i := slices.Index(p.records[name], rr.Data) + if i < 0 { + continue + } + + p.records[name] = slices.Delete(p.records[name], i, i+1) + + out = append(out, rec) + } + + return out, nil +} + +// Add stores a value directly, as if the operator had created it. +func (p *Provider) Add(name, value string) { + p.mu.Lock() + defer p.mu.Unlock() + + if p.records == nil { + p.records = make(map[string][]string) + } + + p.records[name] = append(p.records[name], value) +} + +// Values returns the TXT values stored at the fully qualified name. +func (p *Provider) Values(name string) []string { + p.mu.Lock() + defer p.mu.Unlock() + + return slices.Clone(p.records[name]) +} + +// Calls returns every call made so far. +func (p *Provider) Calls() []Call { + p.mu.Lock() + defer p.mu.Unlock() + + return slices.Clone(p.calls) +} diff --git a/hscontrol/dns/propagation.go b/hscontrol/dns/propagation.go new file mode 100644 index 000000000..d779f9a47 --- /dev/null +++ b/hscontrol/dns/propagation.go @@ -0,0 +1,180 @@ +package dns + +import ( + "context" + "errors" + "fmt" + "net" + "slices" + "strings" + "sync" + "sync/atomic" + "time" + + "github.com/rs/zerolog/log" +) + +var ErrNotPropagated = errors.New("TXT record not visible on all authoritative nameservers") + +const propagationInterval = 2 * time.Second + +// txtChecker waits until every authoritative nameserver of a zone serves +// a TXT value. The client asks the CA to validate as soon as set-dns +// returns, so answering early fails the challenge and costs the node a +// failed authorization at the CA. +type txtChecker struct { + resolvers []string + timeout time.Duration + interval time.Duration + // port the authoritative nameservers are queried on. + port string +} + +func newTXTChecker(resolvers []string, timeout time.Duration) txtChecker { + return txtChecker{ + resolvers: resolvers, + timeout: timeout, + interval: propagationInterval, + port: "53", + } +} + +// wait returns nil once each nameserver of the zone holding fqdn answers +// it with value. +// A timeout of zero disables the check. +func (c txtChecker) wait(ctx context.Context, fqdn, value string) error { + if c.timeout <= 0 { + return nil + } + + start := time.Now() + + ctx, cancel := context.WithTimeout(ctx, c.timeout) + defer cancel() + + recursive := resolverAt(c.resolvers...) + + nameservers, err := nameserversOf(ctx, recursive, fqdn) + if err != nil { + return fmt.Errorf("looking up nameservers of %s: %w", fqdn, c.via(err)) + } + + // A nameserver counts as done when any of its addresses has the value: + // the host may lack a route to one address family. + pending := make(map[string][]string, len(nameservers)) + for _, ns := range nameservers { + addrs, err := recursive.LookupHost(ctx, ns.Host) + if err != nil { + return fmt.Errorf("resolving nameserver %s: %w", ns.Host, c.via(err)) + } + + pending[ns.Host] = addrs + } + + for { + // Every address is asked at once, so a silent one delays a round by + // at most one query. + var ( + mu sync.Mutex + wg sync.WaitGroup + found = make(map[string]string) + ) + + for host, addrs := range pending { + for _, addr := range addrs { + wg.Go(func() { + // Bound each query: the resolver's own timeout and retries + // come from /etc/resolv.conf and make one silent address + // cost tens of seconds. + qctx, cancel := context.WithTimeout(ctx, c.interval) + defer cancel() + + txts, _ := resolverAt(net.JoinHostPort(addr, c.port)).LookupTXT(qctx, fqdn) + if slices.Contains(txts, value) { + mu.Lock() + found[host] = addr + mu.Unlock() + } + }) + } + } + + wg.Wait() + + for host, addr := range found { + log.Debug(). + Str("fqdn", fqdn). + Str("nameserver", host). + Str("addr", addr). + Dur("elapsed", time.Since(start)). + Msg("ACME challenge visible on nameserver") + delete(pending, host) + } + + if len(pending) == 0 { + return nil + } + + select { + case <-ctx.Done(): + hosts := make([]string, 0, len(pending)) + for host := range pending { + hosts = append(hosts, host) + } + + slices.Sort(hosts) + + return fmt.Errorf("%w: %s waiting for %s", ErrNotPropagated, fqdn, strings.Join(hosts, ", ")) + case <-time.After(c.interval): + } + } +} + +// nameserversOf returns the nameservers of the closest enclosing name of +// name that has any, which is the zone holding it. +func nameserversOf(ctx context.Context, r *net.Resolver, name string) ([]*net.NS, error) { + for { + nameservers, err := r.LookupNS(ctx, name) + + _, parent, _ := strings.Cut(name, ".") + if !isNotFound(err) || parent == "" { + return nameservers, err + } + + name = parent + } +} + +func isNotFound(err error) bool { + dnsErr, ok := errors.AsType[*net.DNSError](err) + + return ok && dnsErr.IsNotFound +} + +// via names the resolvers a lookup went to: Go reports the server from +// /etc/resolv.conf, which the custom dialer never contacts. +func (c txtChecker) via(err error) error { + if dnsErr, ok := errors.AsType[*net.DNSError](err); ok { + dnsErr.Server = strings.Join(c.resolvers, ", ") + } + + return err +} + +// resolverAt returns a resolver that sends queries to addrs instead of the +// servers in /etc/resolv.conf. Each dial takes the next address, so the +// resolver's retries move on to another server when one does not answer. +func resolverAt(addrs ...string) *net.Resolver { + var next atomic.Uint32 + + return &net.Resolver{ + PreferGo: true, + Dial: func(ctx context.Context, network, _ string) (net.Conn, error) { + var d net.Dialer + + addr := addrs[int(next.Add(1)-1)%len(addrs)] + + return d.DialContext(ctx, network, addr) + }, + } +} diff --git a/hscontrol/dns/propagation_test.go b/hscontrol/dns/propagation_test.go new file mode 100644 index 000000000..a921f8179 --- /dev/null +++ b/hscontrol/dns/propagation_test.go @@ -0,0 +1,154 @@ +package dns + +import ( + "context" + "net" + "runtime" + "sync" + "sync/atomic" + "testing" + "time" + + mdns "github.com/miekg/dns" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakeNameserver answers authoritatively for testZone: ns1 and ns2 are +// the zone's nameservers, and TXT answers come from txt. +type fakeNameserver struct { + mu sync.Mutex + txt map[string][]string + // hideFor makes the first n TXT queries miss, like a nameserver that + // has not caught up yet. + hideFor atomic.Int32 +} + +func (f *fakeNameserver) set(values ...string) { + f.mu.Lock() + defer f.mu.Unlock() + + f.txt[testName] = values +} + +func (f *fakeNameserver) ServeDNS(w mdns.ResponseWriter, r *mdns.Msg) { + m := new(mdns.Msg) + m.SetReply(r) + m.Authoritative = true + + q := r.Question[0] + hdr := mdns.RR_Header{Name: q.Name, Class: mdns.ClassINET, Ttl: 60, Rrtype: q.Qtype} + + switch q.Qtype { + case mdns.TypeNS: + m.Answer = []mdns.RR{ + &mdns.NS{Hdr: hdr, Ns: "ns1." + testZone}, + &mdns.NS{Hdr: hdr, Ns: "ns2." + testZone}, + } + case mdns.TypeA: + switch q.Name { + case "ns1." + testZone: + m.Answer = []mdns.RR{&mdns.A{Hdr: hdr, A: net.IPv4(127, 0, 0, 1)}} + case "ns2." + testZone: + m.Answer = []mdns.RR{&mdns.A{Hdr: hdr, A: net.IPv4(127, 0, 0, 2)}} + } + case mdns.TypeTXT: + if f.hideFor.Add(-1) >= 0 { + break + } + + f.mu.Lock() + for _, v := range f.txt[q.Name] { + m.Answer = append(m.Answer, &mdns.TXT{Hdr: hdr, Txt: []string{v}}) + } + f.mu.Unlock() + } + + if len(m.Answer) == 0 { + m.Ns = []mdns.RR{&mdns.SOA{ + Hdr: mdns.RR_Header{Name: testZone, Rrtype: mdns.TypeSOA, Class: mdns.ClassINET, Ttl: 60}, + Ns: "ns1." + testZone, + Mbox: "hostmaster." + testZone, + Serial: 1, + Refresh: 60, Retry: 60, Expire: 60, Minttl: 60, + }} + } + + _ = w.WriteMsg(m) +} + +// startNameservers runs ns1 on 127.0.0.1 and ns2 on 127.0.0.2, both on the +// same port, and returns a checker using ns1 as its recursive resolver. +func startNameservers(t *testing.T) (txtChecker, *fakeNameserver, *fakeNameserver) { + t.Helper() + + if runtime.GOOS != "linux" { + t.Skip("needs 127.0.0.2, which only Linux routes by default") + } + + ns1 := &fakeNameserver{txt: map[string][]string{}} + ns2 := &fakeNameserver{txt: map[string][]string{}} + + var lc net.ListenConfig + + pc1, err := lc.ListenPacket(t.Context(), "udp", "127.0.0.1:0") + require.NoError(t, err) + + _, port, err := net.SplitHostPort(pc1.LocalAddr().String()) + require.NoError(t, err) + + pc2, err := lc.ListenPacket(t.Context(), "udp", net.JoinHostPort("127.0.0.2", port)) + require.NoError(t, err) + + for pc, h := range map[net.PacketConn]mdns.Handler{pc1: ns1, pc2: ns2} { + srv := &mdns.Server{PacketConn: pc, Handler: h} + + go func() { _ = srv.ActivateAndServe() }() + + t.Cleanup(func() { _ = srv.Shutdown() }) + } + + c := newTXTChecker([]string{net.JoinHostPort("127.0.0.1", port)}, 5*time.Second) + c.interval = 10 * time.Millisecond + c.port = port + + return c, ns1, ns2 +} + +func TestTXTCheckerAllNameservers(t *testing.T) { + c, ns1, ns2 := startNameservers(t) + ctx := context.Background() + + ns1.set("other", "value") + ns2.set("value") + require.NoError(t, c.wait(ctx, testName, "value")) + + // ns2 never gets the value: waiting for ns1 alone would let the CA + // hit ns2 and fail the challenge. + ns1.set("value-2") + + c.timeout = 200 * time.Millisecond + + err := c.wait(ctx, testName, "value-2") + require.ErrorIs(t, err, ErrNotPropagated) + require.ErrorContains(t, err, "ns2."+testZone) + assert.NotContains(t, err.Error(), "ns1.") +} + +func TestTXTCheckerWaitsForLaggingNameserver(t *testing.T) { + c, ns1, ns2 := startNameservers(t) + + ns1.set("value") + ns2.set("value") + ns2.hideFor.Store(3) + + require.NoError(t, c.wait(context.Background(), testName, "value")) + assert.Negative(t, ns2.hideFor.Load(), "ns2 was asked again after it missed") +} + +func TestTXTCheckerErrorNamesResolvers(t *testing.T) { + c := newTXTChecker([]string{"1.1.1.1:53", "8.8.8.8:53"}, time.Second) + err := c.via(&net.DNSError{Err: "no such host", Name: "ts.example.com.", Server: "127.0.0.53:53"}) + + assert.EqualError(t, err, "lookup ts.example.com. on 1.1.1.1:53, 8.8.8.8:53: no such host") +}