diff --git a/cmd/commands/ssl_acme_add.go b/cmd/commands/ssl_acme_add.go index 0e4cfb41d..441c691f7 100644 --- a/cmd/commands/ssl_acme_add.go +++ b/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))) diff --git a/cmd/commands/ssl_add.go b/cmd/commands/ssl_add.go index 0a04efc79..49717420b 100644 --- a/cmd/commands/ssl_add.go +++ b/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 { diff --git a/cmd/commands/ssl_conf_restore.go b/cmd/commands/ssl_conf_restore.go new file mode 100644 index 000000000..3732ae6b8 --- /dev/null +++ b/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-.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 +} diff --git a/cmd/commands/ssl_conf_restore_test.go b/cmd/commands/ssl_conf_restore_test.go new file mode 100644 index 000000000..1c5ed5625 --- /dev/null +++ b/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) + } +} diff --git a/internal/ssl/acme/install.go b/internal/ssl/acme/install.go index c30a9d432..7b07da194 100644 --- a/internal/ssl/acme/install.go +++ b/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: diff --git a/internal/ssl/acme/install_test.go b/internal/ssl/acme/install_test.go index 44ff7578c..fd6cd95c5 100644 --- a/internal/ssl/acme/install_test.go +++ b/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) + } +}