diff --git a/hub/CHANGELOG.md b/hub/CHANGELOG.md index 21f13f84..1e1cd7db 100644 --- a/hub/CHANGELOG.md +++ b/hub/CHANGELOG.md @@ -1,5 +1,6 @@ ## Unreleased (2026-10-06 night) — a reinstall's new backup key no longer overwrites the old one in the escrow (R-366) +- hub (R-31): a second Save of a customer's off-site settings while the first is still provisioning is refused at once (409, „already running — wait about a minute, then reload; do not save again") instead of racing it: both used to see no sub-account and create one (for a dedicated box, a second bill). Per customer, in memory; another customer is never blocked. Tests `TestProvision_R31_*` (red-proved: `documentation/audits/night-burndown-2026-10-06/hub/R-31-red.txt`). The async save with a status card stays open. - hub: the current escrow row is kept (retained, operator-only) when the whole-guest backup key's fingerprint changes, not only when the restic password changes. A reinstall mints a new PBS key while R-241 keeps the restic password, so the old rule overwrote the only copy of the old key and every pre-reinstall whole-guest archive became unopenable. An empty fingerprint on either side is unknown, not a change. Tests `TestSaveHostEscrow_R366_*` (red-proved: `documentation/audits/night-burndown-2026-10-06/r366/`). ## v0.140.0 — a failed operator mail is sent again; a Docker set is approved only after the engine showed it reports a memory kill (decision 157) (2026-10-06) diff --git a/hub/internal/offsite/offsite.go b/hub/internal/offsite/offsite.go index 0e078707..da010f32 100644 --- a/hub/internal/offsite/offsite.go +++ b/hub/internal/offsite/offsite.go @@ -12,10 +12,12 @@ import ( "context" "crypto/rand" "encoding/json" + "errors" "fmt" "log" "math/big" "strings" + "sync" "time" "gitea.dooplex.hu/admin/felhom-hub/internal/hetznerapi" @@ -29,8 +31,8 @@ const sftpPort = 23 // the password or the SSH key. type Descriptor struct { Enabled bool `json:"enabled"` - Type string `json:"type,omitempty"` // "shared" | "dedicated" - Host string `json:"host,omitempty"` // .your-storagebox.de (dedicated) / -subN… (shared) + Type string `json:"type,omitempty"` // "shared" | "dedicated" + Host string `json:"host,omitempty"` // .your-storagebox.de (dedicated) / -subN… (shared) User string `json:"user,omitempty"` Port int `json:"port,omitempty"` // 23 RepoPath string `json:"repo_path,omitempty"` // /home/ @@ -66,6 +68,35 @@ type Provisioner struct { // creation by seconds-to-a-minute, so the first scan typically fails with "no such host"). nil → the // default ~60s ladder. Tests inject zeros. The total must fit inside applyOffsite's 3-min detached ctx. ScanBackoff []time.Duration + + // inflight (R-31) holds the customers whose provisioning is running RIGHT NOW. + // A second Save while the first still runs used to race it: both listed no sub-account, both created + // one (a second dedicated box is a second bill). The second call now fails at once with + // ErrProvisionInProgress. Pinned by TestProvision_R31_ConcurrentSaveRefused. + inflightMu sync.Mutex + inflight map[string]bool +} + +// ErrProvisionInProgress (R-31): another provisioning for the same customer is still running. +var ErrProvisionInProgress = errors.New("offsite: provisioning for this customer is already running — wait about a minute, then reload the page; do not save again") + +// begin claims customerID for one provisioning step; the returned func releases it. +func (p *Provisioner) begin(customerID string) (func(), error) { + p.inflightMu.Lock() + defer p.inflightMu.Unlock() + if p.inflight == nil { + p.inflight = map[string]bool{} + } + if p.inflight[customerID] { + p.logf("[offsite] refused a second provisioning for %s while one is running (R-31)", customerID) + return nil, ErrProvisionInProgress + } + p.inflight[customerID] = true + return func() { + p.inflightMu.Lock() + delete(p.inflight, customerID) + p.inflightMu.Unlock() + }, nil } // defaultScanBackoff: 5 retries, ~60s total — sized to the observed DNS propagation lag. @@ -78,8 +109,10 @@ func (p *Provisioner) logf(f string, a ...any) { } // customerLabel is the idempotency/teardown key. -func customerLabel(customerID string) map[string]string { return map[string]string{"felhom-customer": customerID} } -func customerSelector(customerID string) string { return "felhom-customer=" + customerID } +func customerLabel(customerID string) map[string]string { + return map[string]string{"felhom-customer": customerID} +} +func customerSelector(customerID string) string { return "felhom-customer=" + customerID } // repoPath is the controller-facing RepoPath — each account is chrooted, /home is writable (SPIKE). const repoPath = "/home/felhom-repo" @@ -92,8 +125,12 @@ func (p *Provisioner) ProvisionOffsite(ctx context.Context, customerID string, i if !in.Enabled { return &Descriptor{Enabled: false}, nil } + release, err := p.begin(customerID) + if err != nil { + return nil, err + } + defer release() var d *Descriptor - var err error switch in.Type { case "shared": d, err = p.provisionShared(ctx, customerID, in) diff --git a/hub/internal/offsite/r31_concurrent_save_test.go b/hub/internal/offsite/r31_concurrent_save_test.go new file mode 100644 index 00000000..7e699a89 --- /dev/null +++ b/hub/internal/offsite/r31_concurrent_save_test.go @@ -0,0 +1,81 @@ +package offsite + +import ( + "context" + "errors" + "sync" + "testing" + + "gitea.dooplex.hu/admin/felhom-hub/internal/hetznerapi" +) + +// gatedAPI blocks the FIRST ListSubaccounts after it has listed, so a second Save can arrive in the +// window between "no sub-account yet" and "create" — the race R-31 names. +type gatedAPI struct { + *hetznerapi.Fake + once sync.Once + listed chan struct{} + release chan struct{} +} + +func (g *gatedAPI) ListSubaccounts(ctx context.Context, boxID int64, sel string) ([]hetznerapi.Subaccount, error) { + out, err := g.Fake.ListSubaccounts(ctx, boxID, sel) + first := false + g.once.Do(func() { first = true }) + if first { + close(g.listed) + <-g.release + } + return out, err +} + +// R-31 — a second Save while the first provisioning still runs must not create a second sub-account +// (for a dedicated box: a second bill). It is refused at once with ErrProvisionInProgress. +// +// COMPANION RED-PROOF (observed): remove the begin()/release() claim from ProvisionOffsite → this fails +// with "the second Save must be refused while the first runs; got err=". Restored. +func TestProvision_R31_ConcurrentSaveRefused(t *testing.T) { + p, fake, _ := newTestProvisioner(t) + g := &gatedAPI{Fake: fake, listed: make(chan struct{}), release: make(chan struct{})} + p.API = g + + firstErr := make(chan error, 1) + go func() { + _, err := p.ProvisionOffsite(context.Background(), "cust-r31", Input{Enabled: true, Type: "shared", QuotaGB: 10}) + firstErr <- err + }() + <-g.listed // the first Save has seen "no sub-account" and is about to create one + + _, err := p.ProvisionOffsite(context.Background(), "cust-r31", Input{Enabled: true, Type: "shared", QuotaGB: 10}) + close(g.release) + if e := <-firstErr; e != nil { + t.Fatalf("the first Save must succeed: %v", e) + } + if !errors.Is(err, ErrProvisionInProgress) { + t.Fatalf("the second Save must be refused while the first runs; got err=%v", err) + } + if fake.CreatedSubaccounts != 1 { + t.Fatalf("exactly one sub-account must exist, got %d", fake.CreatedSubaccounts) + } + + // After the first finished, a Save is idempotent again (no refusal, no second create). + if _, err := p.ProvisionOffsite(context.Background(), "cust-r31", Input{Enabled: true, Type: "shared", QuotaGB: 10}); err != nil { + t.Fatalf("a Save after the first finished must pass: %v", err) + } + if fake.CreatedSubaccounts != 1 { + t.Fatalf("still exactly one sub-account, got %d", fake.CreatedSubaccounts) + } +} + +// Another customer is never blocked by one customer's running provisioning. +func TestProvision_R31_OtherCustomerNotBlocked(t *testing.T) { + p, _, _ := newTestProvisioner(t) + release, err := p.begin("cust-x") + if err != nil { + t.Fatal(err) + } + defer release() + if _, err := p.ProvisionOffsite(context.Background(), "cust-y", Input{Enabled: true, Type: "shared", QuotaGB: 10}); err != nil { + t.Fatalf("another customer must not be blocked: %v", err) + } +} diff --git a/hub/internal/web/configs.go b/hub/internal/web/configs.go index 3238af11..62a2f063 100644 --- a/hub/internal/web/configs.go +++ b/hub/internal/web/configs.go @@ -779,7 +779,12 @@ func (s *Server) handleConfigCreate(w http.ResponseWriter, r *http.Request) { } // Offsite provisioning (fail-closed): a provisioning error must NOT save a half-enabled config. - if err := s.applyOffsite(r.Context(), r, cfg); err != nil { + if err := s.applyOffsite(r.Context(), r, cfg); errors.Is(err, offsite.ErrProvisionInProgress) { + // R-31: a second Save while the first still provisions — nothing saved, nothing created. + s.logger.Printf("[INFO] offsite provision for %s refused: one is already running (R-31)", customerID) + http.Error(w, err.Error(), http.StatusConflict) + return + } else if err != nil { s.logger.Printf("[ERROR] offsite provision for %s: %v", customerID, err) http.Error(w, "Offsite provisioning failed: "+err.Error(), http.StatusBadGateway) return @@ -859,7 +864,12 @@ func (s *Server) handleConfigUpdate(w http.ResponseWriter, r *http.Request, cust cfg.ConfigJSON = buildConfigJSON(r) - if err := s.applyOffsite(r.Context(), r, cfg); err != nil { + if err := s.applyOffsite(r.Context(), r, cfg); errors.Is(err, offsite.ErrProvisionInProgress) { + // R-31: a second Save while the first still provisions — nothing saved, nothing created. + s.logger.Printf("[INFO] offsite provision for %s refused: one is already running (R-31)", customerID) + http.Error(w, err.Error(), http.StatusConflict) + return + } else if err != nil { s.logger.Printf("[ERROR] offsite provision for %s: %v", customerID, err) http.Error(w, "Offsite provisioning failed: "+err.Error(), http.StatusBadGateway) return