fix(ssl): ssl add leaves no rejected nginx conf; ACME first conversion rolls back; prepare errors are E471 (D-0412)

cmd/commands/ssl_acme_add.go

@@ -132,14 +132,27 @@ func installFailed(err error) error { return fmt.Errorf("%w%.0w", err, acme.ErrI
 // the target), reloads nginx and checks the fingerprint it serves.
 func writeAndVerifyAdd(ctx context.Context, r *acmeRun, l acme.Lineage, cert []byte, reload acme.NginxReloader) error {
 	hosts, _ := ssl.ServedHosts(r.res.NginxDir)
+	undoConf, undone := func() error { return nil }, "" // nothing was written when a served conf already uses the target
 	if !strings.Contains(hosts[l.Domains[0]], "/"+l.Name+"/") {
 		upstream, _ := r.cmd.Flags().GetString("upstream")
+		snap, err := snapshotCustomDomainConf(r.workdir, r.cfg.Nginx.FrontedBy, l.Domains[0])
+		if err != nil {
+			return e151(installFailed(acmeRefuse("the certificate is installed; fix the conf directory and re-run", "reading nginx conf: %v", err)))
+		}
+		undoConf, undone = snap.restore, snap.outcome()
 		if err := writeCustomDomainConfServed(r.workdir, r.cfg.Nginx.FrontedBy, l.Domains[0], upstream); err != nil {
 			return e151(installFailed(acmeRefuse("the certificate is installed; fix the conf directory and re-run", "writing nginx conf: %v", err)))
 		}
 	}
 	if err := reload.Reload(ctx); err != nil {
-		return e151(installFailed(acmeRefuse("fix the nginx error above; the certificate is installed", "%v", err)))
+		// nginx rejected the new conf: take it out so the next restart still starts.
+		note := "the certificate is installed"
+		if rerr := undoConf(); rerr != nil {
+			note += "; restoring the conf failed (" + rerr.Error() + "), fix it by hand"
+		} else {
+			note += "; " + undone
+		}
+		return e151(installFailed(acmeRefuse("fix the nginx error above; "+note, "%v", err)))
 	}
 	if err := r.verifier(l, cert)(ctx); err != nil {
 		return e151(installFailed(acmeRefuse("check that nginx serves the domain on its HTTPS port", "served certificate check failed: %v", err)))

cmd/commands/ssl_add.go

@@ -126,14 +126,22 @@ func runSSLAdd(cmd *cobra.Command, args []string) error {
 		return fmt.Errorf("installing certificate for %s: %w", domain, err)
 	}
 
-	// Write the nginx server block for this custom domain.
+	// Write the nginx server block for this custom domain. Keep what was there:
+	// a conf nginx rejects must not stay behind to break the next restart.
+	snap, err := snapshotCustomDomainConf(workdir, frontedBy, domain)
+	if err != nil {
+		return fmt.Errorf("reading nginx conf: %w", err)
+	}
 	if err := writeCustomDomainConfServed(workdir, frontedBy, domain, upstream); err != nil {
 		return fmt.Errorf("writing nginx conf: %w", err)
 	}
 
 	// Validate nginx config before reloading.
 	if testErr := nginxCompose(cmd.Context(), servedRoot, "-t"); testErr != nil {
-		return fmt.Errorf("nginx config test failed: %w", testErr)
+		if rerr := snap.restore(); rerr != nil {
+			return fmt.Errorf("nginx config test failed: %w (restoring %s also failed: %v)", testErr, snap.path, rerr)
+		}
+		return fmt.Errorf("nginx config test failed, %s: %w", snap.outcome(), testErr)
 	}
 
 	if err := nginxCompose(cmd.Context(), servedRoot, "-s", "reload"); err != nil {

cmd/commands/ssl_conf_restore.go

@@ -0,0 +1,58 @@
+package commands
+
+// Purpose: undo a custom-domain conf that nginx rejected.
+// Inputs: the served conf directory and the domain `trust ssl add` writes for.
+// Outputs: a snapshot taken before the write, and its restore.
+// Constraints: a conf that did not exist is removed; one that did gets its bytes back.
+
+import (
+	"errors"
+	"fmt"
+	"io/fs"
+	"os"
+	"path/filepath"
+)
+
+// confSnapshot is the state of one custom-domain conf before `ssl add` wrote it.
+type confSnapshot struct {
+	path    string
+	data    []byte
+	existed bool
+}
+
+// snapshotCustomDomainConf records custom-<domain-safe>.conf in the served conf dir.
+func snapshotCustomDomainConf(workdir, frontedBy, domain string) (confSnapshot, error) {
+	confDir, err := servedConfDir(workdir, frontedBy)
+	if err != nil {
+		return confSnapshot{}, err
+	}
+	s := confSnapshot{path: filepath.Join(confDir, fmt.Sprintf("custom-%s.conf", domainToFilesafe(domain)))}
+	data, err := os.ReadFile(s.path) //nolint:gosec // path built from the served conf dir
+	switch {
+	case err == nil:
+		s.data, s.existed = data, true
+	case !errors.Is(err, fs.ErrNotExist):
+		return confSnapshot{}, err
+	}
+	return s, nil
+}
+
+// outcome names what restore does, for the error text shown to the operator.
+func (s confSnapshot) outcome() string {
+	if s.existed {
+		return "the previous conf was restored"
+	}
+	return "the new conf was removed"
+}
+
+// restore puts the conf back as snapshotted, so the next nginx test or restart
+// does not trip over the rejected one.
+func (s confSnapshot) restore() error {
+	if !s.existed {
+		if err := os.Remove(s.path); err != nil && !errors.Is(err, fs.ErrNotExist) {
+			return err
+		}
+		return nil
+	}
+	return os.WriteFile(s.path, s.data, 0o644) //nolint:gosec // nginx reads it; same mode the writer uses
+}

cmd/commands/ssl_conf_restore_test.go

@@ -0,0 +1,77 @@
+package commands
+
+// ssl_conf_restore_test.go: a custom-domain conf nginx rejects is taken out
+// again (D-0412), on the snapshot helper and through `trust ssl add --acme`.
+
+import (
+	"context"
+	"errors"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+func TestConfSnapshotRestore(t *testing.T) {
+	for _, tc := range []struct {
+		name   string
+		before *string
+	}{
+		{"new conf is removed", nil},
+		{"existing conf gets its bytes back", ptr("# hand edited\n")},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			work := t.TempDir()
+			confDir := filepath.Join(work, "nginx", "conf.d")
+			if err := os.MkdirAll(confDir, 0o750); err != nil {
+				t.Fatal(err)
+			}
+			path := filepath.Join(confDir, "custom-app-example-org.conf")
+			if tc.before != nil {
+				if err := os.WriteFile(path, []byte(*tc.before), 0o644); err != nil {
+					t.Fatal(err)
+				}
+			}
+			snap, err := snapshotCustomDomainConf(work, "", "app.example.org")
+			if err != nil {
+				t.Fatal(err)
+			}
+			if err := writeCustomDomainConfServed(work, "", "app.example.org", ""); err != nil {
+				t.Fatal(err)
+			}
+			if err := snap.restore(); err != nil {
+				t.Fatal(err)
+			}
+			if want := map[bool]string{false: "removed", true: "restored"}[snap.existed]; !strings.Contains(snap.outcome(), want) {
+				t.Errorf("outcome = %q, want it to say %q", snap.outcome(), want)
+			}
+			got, rerr := os.ReadFile(path)
+			switch {
+			case tc.before == nil && !errors.Is(rerr, os.ErrNotExist):
+				t.Errorf("conf still there after restore: %q %v", got, rerr)
+			case tc.before != nil && string(got) != *tc.before:
+				t.Errorf("conf = %q, want %q", got, *tc.before)
+			}
+		})
+	}
+}
+
+func TestSSLAddACMENginxRejectsConf(t *testing.T) {
+	f, _ := addFix(t, nil)
+	conf := filepath.Join(f.served, "nginx", "conf.d", "custom-app-example-org.conf")
+	// nginx -t fails only once the new conf is on disk, as it would for a bad --upstream.
+	acmeD.exec = func(_ context.Context, _ string, cmd []string) (string, string, error) {
+		f.execs = append(f.execs, cmd)
+		if _, err := os.Stat(conf); err == nil && cmd[len(cmd)-1] == "-t" {
+			return "", "nginx: [emerg] host not found in upstream", errors.New("exit status 1")
+		}
+		return "", "", nil
+	}
+	_, err := f.runAdd("app.example.org", "--acme", "--agree-tos", "--upstream=bad_host:3000")
+	if err == nil || !strings.Contains(err.Error(), "the new conf was removed") {
+		t.Fatalf("want a refusal naming the removed conf, got %v", err)
+	}
+	if _, serr := os.Stat(conf); !errors.Is(serr, os.ErrNotExist) {
+		t.Errorf("rejected conf left behind: %v", serr)
+	}
+}

internal/ssl/acme/install.go

@@ -148,7 +148,8 @@ func Install(ctx context.Context, r InstallReq) (map[string]int, error) {
 	for _, t := range r.Targets {
 		s, err := prepare(r, t)
 		if err != nil {
-			return nil, err
+			// Tag it so the commands answer E471 (issued, not installed), not a bare refusal.
+			return nil, fmt.Errorf("%w%.0w", err, ErrInstall)
 		}
 		swaps = append(swaps, s)
 	}
@@ -203,6 +204,11 @@ func prepare(r InstallReq, target string) (swap, error) {
 			return swap{}, fmt.Errorf("converting %s: %w", target, err)
 		}
 		if err := point(link, genName(base, prev)); err != nil {
+			// Put the directory back: a half-converted target would serve nothing until Repair runs.
+			_ = os.Remove(link + ".tmp-link")
+			if rerr := os.Rename(filepath.Join(parent, genName(base, prev)), link); rerr != nil {
+				return swap{}, errors.Join(err, fmt.Errorf("restoring %s: %w", target, rerr))
+			}
 			return swap{}, err
 		}
 	case err == nil && prev < 0:

internal/ssl/acme/install_test.go

@@ -445,3 +445,31 @@ func TestACMENonASCIIName(t *testing.T) {
 		t.Fatalf("want a punycode remediation, got %v", err)
 	}
 }
+
+// TestACMEFirstConversionFailure: when the symlink switch of the first
+// conversion fails, the real directory goes back in place (D-0412) and the
+// error is tagged as an install failure (E471).
+func TestACMEFirstConversionFailure(t *testing.T) {
+	posixOnly(t)
+	ssl, certA := fixture(t)
+	certB, keyB := testPair(t, []string{"api.example.org"}, time.Now().Add(90*24*time.Hour))
+	// A non-empty directory where point() stages its link makes the symlink fail.
+	if err := os.MkdirAll(filepath.Join(ssl, tgt+".tmp-link", "x"), 0o700); err != nil {
+		t.Fatal(err)
+	}
+	rel := &fakeReloader{}
+	_, err := Install(context.Background(), InstallReq{SSLDir: ssl, Targets: []string{tgt}, Cert: certB, Key: keyB, Reloader: rel})
+	if err == nil {
+		t.Fatal("install succeeded with a blocked link")
+	}
+	if !errors.Is(err, ErrInstall) || CodeFor(err) != CodeInstall {
+		t.Errorf("error not tagged as an install failure: %v (code %q)", err, CodeFor(err))
+	}
+	fi, lerr := os.Lstat(filepath.Join(ssl, tgt))
+	if lerr != nil || !fi.IsDir() {
+		t.Fatalf("target is not the original directory after the failure: %v %v", fi, lerr)
+	}
+	if string(served(t, ssl, tgt)) != string(certA) || rel.calls != 0 {
+		t.Errorf("previous pair not intact (reloads=%d)", rel.calls)
+	}
+}