From 1309d09d35eb831355647beb1530115b70b410bb Mon Sep 17 00:00:00 2001 From: Jacky Date: Thu, 12 Nov 2026 20:05:35 +0800 Subject: [PATCH] fix: clean up through the provider that published the record Every dns01.present and dns01.cleanup call built a new lego provider. The providers that keep the vendor's record ID in memory during Present, such as cloudflare and ispconfig, then failed the cleanup with "unknown record ID" and left the _acme-challenge record behind. The handler now keeps the provider of each published record, keyed by the provider code, a fingerprint of the credentials and options, the domain, the token and the key authorization, and CleanUp takes it back. A cleanup that finds none, for example after the plugin restarted, builds a fresh provider as before. Entries expire after six hours so a missed cleanup cannot leak them. Refs 0xJacky/nginx-ui#2113, 0xJacky/nginx-ui#2115 Co-Authored-By: Claude Opus 5.5 --- provider/cleanup_test.go | 185 +++++++++++++++++++++++++++++++++++++++ provider/provider.go | 102 ++++++++++++++++++++- 2 files changed, 284 insertions(+), 3 deletions(-) create mode 100644 provider/cleanup_test.go diff --git a/provider/cleanup_test.go b/provider/cleanup_test.go new file mode 100644 index 0000000..d0d4775 --- /dev/null +++ b/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") + } +} diff --git a/provider/provider.go b/provider/provider.go index 377198b..c7ea98a 100644 --- a/provider/provider.go +++ b/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)