fix: clean up through the provider that published the record

provider/cleanup_test.go

@@ -0,0 +1,185 @@
+package provider
+
+import (
+	"context"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"sync"
+	"testing"
+
+	sdk "github.com/nginxui/plugin-sdk-go"
+	"github.com/nginxui/plugin-sdk-go/protocol"
+)
+
+// fakeISPConfig answers the ISPConfig remote JSON API calls lego makes and
+// records which record IDs were deleted.
+type fakeISPConfig struct {
+	mu      sync.Mutex
+	added   int
+	deleted []string
+}
+
+func (f *fakeISPConfig) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	body, _ := io.ReadAll(r.Body)
+	w.Header().Set("Content-Type", "application/json")
+
+	f.mu.Lock()
+	defer f.mu.Unlock()
+
+	switch r.URL.RawQuery {
+	case "login":
+		_, _ = io.WriteString(w, `{"code":"ok","message":"","response":"session"}`)
+	case "dns_zone_get_id":
+		_, _ = io.WriteString(w, `{"code":"ok","message":"","response":123}`)
+	case "dns_zone_get":
+		_, _ = io.WriteString(w, `{"code":"ok","message":"","response":{"id":"456","sys_userid":"789"}}`)
+	case "client_get_id":
+		_, _ = io.WriteString(w, `{"code":"ok","message":"","response":321}`)
+	case "dns_txt_add":
+		f.added++
+		_, _ = io.WriteString(w, `{"code":"ok","message":"","response":"record-1"}`)
+	case "dns_txt_delete":
+		id := "unknown"
+		if strings.Contains(string(body), `"record-1"`) {
+			id = "record-1"
+		}
+		f.deleted = append(f.deleted, id)
+		_, _ = io.WriteString(w, `{"code":"ok","message":"","response":1}`)
+	default:
+		http.Error(w, "unexpected call "+r.URL.RawQuery, http.StatusBadRequest)
+	}
+}
+
+func ispconfigRequest(serverURL string) sdk.DNS01Request {
+	return sdk.DNS01Request{
+		Provider: "ispconfig",
+		Config: map[string]string{
+			"ISPCONFIG_SERVER_URL": serverURL,
+			"ISPCONFIG_USERNAME":   "user",
+			"ISPCONFIG_PASSWORD":   "secret",
+		},
+		// Keeps lego from resolving the challenge record's CNAME over the
+		// network.
+		Options: map[string]any{protocol.DNS01OptionDisableCNAME: true},
+		Domain:  "example.com",
+		Token:   "token",
+		KeyAuth: "token.key",
+	}
+}
+
+// lego providers such as cloudflare and ispconfig keep the vendor's record ID
+// in memory during Present. CleanUp must reach that same provider, or the
+// record cannot be found and stays behind.
+func TestCleanUpRemovesTheRecordPresentPublished(t *testing.T) {
+	vendor := &fakeISPConfig{}
+	server := httptest.NewServer(vendor)
+	defer server.Close()
+
+	h := New()
+	req := ispconfigRequest(server.URL)
+
+	if err := h.Present(context.Background(), req); err != nil {
+		t.Fatalf("Present: %v", err)
+	}
+	if err := h.CleanUp(context.Background(), req); err != nil {
+		t.Fatalf("CleanUp: %v", err)
+	}
+
+	vendor.mu.Lock()
+	defer vendor.mu.Unlock()
+	if vendor.added != 1 || len(vendor.deleted) != 1 || vendor.deleted[0] != "record-1" {
+		t.Fatalf("added %d, deleted %v, want the one published record deleted", vendor.added, vendor.deleted)
+	}
+}
+
+// Every record is cleaned up through its own provider: two challenges of one
+// certificate, presented before either is cleaned up, both get removed.
+func TestCleanUpHandlesInterleavedChallenges(t *testing.T) {
+	vendor := &fakeISPConfig{}
+	server := httptest.NewServer(vendor)
+	defer server.Close()
+
+	h := New()
+	wildcard := ispconfigRequest(server.URL)
+	apex := ispconfigRequest(server.URL)
+	apex.Token, apex.KeyAuth = "token-2", "token-2.key"
+
+	for _, req := range []sdk.DNS01Request{wildcard, apex} {
+		if err := h.Present(context.Background(), req); err != nil {
+			t.Fatalf("Present %s: %v", req.Token, err)
+		}
+	}
+	for _, req := range []sdk.DNS01Request{wildcard, apex} {
+		if err := h.CleanUp(context.Background(), req); err != nil {
+			t.Fatalf("CleanUp %s: %v", req.Token, err)
+		}
+	}
+
+	if got := len(h.presented); got != 0 {
+		t.Fatalf("%d providers kept after every cleanup, want 0", got)
+	}
+}
+
+// A cleanup with other credentials, or one that arrives after the plugin lost
+// the provider, falls back to a fresh provider exactly as before and reports
+// the provider's own error.
+func TestCleanUpWithoutAPresentedProviderFallsBack(t *testing.T) {
+	vendor := &fakeISPConfig{}
+	server := httptest.NewServer(vendor)
+	defer server.Close()
+
+	h := New()
+	req := ispconfigRequest(server.URL)
+	if err := h.Present(context.Background(), req); err != nil {
+		t.Fatalf("Present: %v", err)
+	}
+
+	other := ispconfigRequest(server.URL)
+	other.Config["ISPCONFIG_PASSWORD"] = "another"
+	err := h.CleanUp(context.Background(), other)
+	if err == nil || !strings.Contains(err.Error(), "unknown record ID") {
+		t.Fatalf("CleanUp with other credentials = %v, want the fresh provider's unknown record ID error", err)
+	}
+
+	// The provider built from the right credentials is still there.
+	if err := h.CleanUp(context.Background(), req); err != nil {
+		t.Fatalf("CleanUp: %v", err)
+	}
+}
+
+// A dry run never touches the vendor and never keeps a provider.
+func TestDryRunKeepsNoProvider(t *testing.T) {
+	vendor := &fakeISPConfig{}
+	server := httptest.NewServer(vendor)
+	defer server.Close()
+
+	h := New()
+	req := ispconfigRequest(server.URL)
+	req.DryRun = true
+	if err := h.Present(context.Background(), req); err != nil {
+		t.Fatalf("Present dry run: %v", err)
+	}
+	if len(h.presented) != 0 || vendor.added != 0 {
+		t.Fatalf("dry run kept %d providers and added %d records, want none", len(h.presented), vendor.added)
+	}
+}
+
+// An entry whose cleanup never came expires instead of leaking.
+func TestPresentedProvidersExpire(t *testing.T) {
+	h := New()
+	key := newPresentedKey(ispconfigRequest("http://127.0.0.1:1"))
+	h.keepPresented(key, nil)
+
+	entry := h.presented[key]
+	entry.expires = entry.expires.Add(-2 * presentedTTL)
+	h.presented[key] = entry
+
+	if p := h.takePresented(key); p != nil {
+		t.Fatal("an expired provider was handed out")
+	}
+	if len(h.presented) != 0 {
+		t.Fatal("the expired entry was not forgotten")
+	}
+}

provider/provider.go

@@ -6,8 +6,12 @@ package provider
 
 import (
 	"context"
+	"crypto/sha256"
+	"encoding/hex"
 	"errors"
 	"fmt"
+	"maps"
+	"slices"
 	"strconv"
 	"strings"
 	"sync"
@@ -46,11 +50,36 @@ type Settings struct {
 	DefaultPropagationTimeout time.Duration
 }
 
+// presentedTTL bounds how long the provider that published a record is kept
+// for the cleanup of that record. lego cleans up right after the validation,
+// well within this, so the bound only stops a missed cleanup from leaking.
+const presentedTTL = 6 * time.Hour
+
 // Handler implements the dns01 capability on top of lego.
 type Handler struct {
 	mu         sync.RWMutex
 	settings   Settings
 	configured bool
+
+	// presented keeps the provider of every record Present published until
+	// CleanUp removes it. Many lego providers remember the vendor's record ID
+	// in memory during Present and cannot delete the record without it.
+	presentedMu sync.Mutex
+	presented   map[presentedKey]presentedEntry
+}
+
+// presentedKey names one published challenge record.
+type presentedKey struct {
+	provider    string
+	fingerprint string
+	domain      string
+	token       string
+	keyAuth     string
+}
+
+type presentedEntry struct {
+	provider challenge.Provider
+	expires  time.Time
 }
 
 // New builds a handler with lego's defaults.
@@ -193,9 +222,21 @@ func (h *Handler) solve(ctx context.Context, req sdk.DNS01Request, present bool)
 		return sdk.Internal(err.Error())
 	}
 
-	p, err := dnsproviders.NewDNSChallengeProviderByName(req.Provider)
-	if err != nil {
-		return configError(cfg, req.Config, err)
+	key := newPresentedKey(req)
+
+	// The cleanup goes through the provider that published the record, which
+	// may hold the record ID. A fresh one is the fallback when that provider is
+	// gone, for example because the plugin restarted in between.
+	var p challenge.Provider
+	if !present && !req.DryRun {
+		p = h.takePresented(key)
+	}
+	if p == nil {
+		var err error
+		p, err = dnsproviders.NewDNSChallengeProviderByName(req.Provider)
+		if err != nil {
+			return configError(cfg, req.Config, err)
+		}
 	}
 
 	if req.DryRun {
@@ -210,6 +251,7 @@ func (h *Handler) solve(ctx context.Context, req sdk.DNS01Request, present bool)
 		if err := p.Present(ctx, req.Domain, req.Token, req.KeyAuth); err != nil {
 			return sdk.Internal(err.Error())
 		}
+		h.keepPresented(key, p)
 		return nil
 	}
 
@@ -220,6 +262,60 @@ func (h *Handler) solve(ctx context.Context, req sdk.DNS01Request, present bool)
 	return nil
 }
 
+// newPresentedKey identifies the record of a request. The fingerprint covers
+// the credentials and the options, so a cleanup sent with other credentials
+// never reuses the provider built from these.
+func newPresentedKey(req sdk.DNS01Request) presentedKey {
+	hash := sha256.New()
+	for _, k := range slices.Sorted(maps.Keys(req.Config)) {
+		fmt.Fprintf(hash, "c%d:%s=%d:%s\n", len(k), k, len(req.Config[k]), req.Config[k])
+	}
+	for _, k := range slices.Sorted(maps.Keys(req.Options)) {
+		v := fmt.Sprint(req.Options[k])
+		fmt.Fprintf(hash, "o%d:%s=%d:%s\n", len(k), k, len(v), v)
+	}
+	return presentedKey{
+		provider:    req.Provider,
+		fingerprint: hex.EncodeToString(hash.Sum(nil)),
+		domain:      req.Domain,
+		token:       req.Token,
+		keyAuth:     req.KeyAuth,
+	}
+}
+
+// keepPresented remembers the provider that published a record, dropping the
+// entries whose cleanup never came.
+func (h *Handler) keepPresented(key presentedKey, p challenge.Provider) {
+	h.presentedMu.Lock()
+	defer h.presentedMu.Unlock()
+
+	now := time.Now()
+	if h.presented == nil {
+		h.presented = make(map[presentedKey]presentedEntry)
+	}
+	maps.DeleteFunc(h.presented, func(_ presentedKey, e presentedEntry) bool {
+		return now.After(e.expires)
+	})
+	h.presented[key] = presentedEntry{provider: p, expires: now.Add(presentedTTL)}
+}
+
+// takePresented returns and forgets the provider that published a record, or
+// nil when there is none.
+func (h *Handler) takePresented(key presentedKey) challenge.Provider {
+	h.presentedMu.Lock()
+	defer h.presentedMu.Unlock()
+
+	entry, ok := h.presented[key]
+	if !ok {
+		return nil
+	}
+	delete(h.presented, key)
+	if time.Now().After(entry.expires) {
+		return nil
+	}
+	return entry.provider
+}
+
 // Options reports the propagation timings lego would use for the provider.
 func (h *Handler) Options(ctx context.Context, params protocol.DNS01OptionsParams) (protocol.DNS01OptionsResult, error) {
 	cfg, ok := catalog.Get(params.Provider)