diff --git a/CLAUDE.md b/CLAUDE.md index abc507c..4cff13c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -31,8 +31,7 @@ Edit the `SETTINGS` block at the top of `openvpncertupdate.py` before first run. | `--revoke CN` | Revoke cert and regenerate CRL | | `--gen-crl` | Regenerate and copy CRL only | | `--list` | List recently-expired/soon-to-expire CNs (per `DAYS_PAST`/`DAYS_AHEAD`) with email; read-only, no CA passphrase needed | -| `--list-all` | List all CNs with email; read-only, no CA passphrase needed | -| | Both grow a trailing `CERT` column marking `MISSING` CNs — see `CertInfo.has_cert_file`. The column is omitted entirely when every CN has its `.crt` | +| `--list-all` | List all CNs with email; read-only, no CA passphrase needed. Both `--list`/`--list-all` grow a trailing `CERT` column marking `MISSING` CNs — see `CertInfo.has_cert_file`. The column is omitted entirely when every CN has its `.crt` | | `--email EMAIL` | Recipient address | | `--days N` | Certificate lifetime in days for `--create`/`--reissue`; overrides `CERT_DAYS` for that run | | `--send-email` | Force email delivery | @@ -69,7 +68,7 @@ python3 -m pytest tests/test_pki.py::test_sorted_ascending -v # single test ## Re-issue workflow -0. `has_issued_cert()` gates steps 1–2: if `/issued/.crt` is absent, both are **skipped** with a warning and the workflow goes straight to step 3. EasyRSA reads the serial out of the `.crt` itself, so `revoke-issued` can only fail on such a CN — and there is nothing to add to the CRL either. This happens when an `index.txt` is carried over from an older EasyRSA install without the `issued/` files: the index still lists V-status certs whose `.crt` never came along. `--revoke` / the TUI `r` key deliberately do *not* skip — an explicit revoke request should fail loudly rather than silently no-op. Such CNs are flagged before the user picks one: `_load_current_certs()` sets `CertInfo.has_cert_file` (one stat per CN, after the dedup), rendered as `(no cert)` before the email in the TUI list and as a `MISSING` cell in the `CERT` column of `--list`/`--list-all` +0. `has_issued_cert()` gates steps 1–2: if `/issued/.crt` is absent, both are **skipped** with a warning and the workflow goes straight to step 3. EasyRSA reads the serial out of the `.crt` itself, so `revoke-issued` can only fail on such a CN — and there is nothing to add to the CRL either. This happens when an `index.txt` is carried over from an older EasyRSA install without the `issued/` files: the index still lists V-status certs whose `.crt` never came along. `--revoke` / the TUI `r` key deliberately do *not* skip — an explicit revoke request should fail loudly rather than silently no-op. Such CNs are flagged before the user picks one: `_load_current_certs()` sets `CertInfo.has_cert_file` (one stat per CN, after the dedup), rendered as `(no cert)` before the email in the TUI list and as a `MISSING` cell in the `CERT` column of `--list`/`--list-all`. Two guards bound the skip so it can't silently do the wrong thing: (a) on the CLI, the skip only fires for a CN that `_load_current_certs()` actually knows about — a typo'd/nonexistent `--reissue CN` is *not* a migration gap and is rejected with `error: unknown CN ...` before anything is built (the TUI can't hit this: renewal always opens on an existing row, so the CN is never freeform there); (b) whichever entry point takes the skip, it first checks for leftover `pki/reqs/.req` / `pki/private/.key` — normally `revoke-issued` archives both into `pki/revoked/`, but skipping it leaves them in place, and EasyRSA's `build-client-full` aborts outright rather than overwrite them. Either leftover fails the workflow fast with the exact path(s), before the CA passphrase prompt — this tool never moves or deletes a private key itself 1. `revoke-issued ` — archives old key + CSR to `pki/revoked/` 2. CRL regenerated and copied to `CRL_DEST_PATH` immediately after the revoke succeeds — the old cert is already revoked at this point, so the published CRL would otherwise be stale until a separate manual regen. Not fatal: a failure here is reported but the workflow continues to step 3 (a new cert is more urgent than a fresh CRL, and "Regenerate CRL" / `--gen-crl` remain available to retry) 3. `build-client-full --passout=pass:` — generates new key + cert diff --git a/openvpncertupdate.py b/openvpncertupdate.py index 1266f30..62307cf 100644 --- a/openvpncertupdate.py +++ b/openvpncertupdate.py @@ -341,6 +341,21 @@ def has_issued_cert(pki_dir: str, cn: str) -> bool: return os.path.isfile(issued_cert_path(pki_dir, cn)) +def _leftover_build_paths(pki_dir: str, cn: str) -> List[str]: + """Which of pki/reqs/.req and pki/private/.key still exist. + + `build-client-full` aborts outright if either is present — EasyRSA + refuses to overwrite them. Normally `revoke-issued` clears both by + archiving them into pki/revoked/, but that step is skipped when there is + no issued cert to revoke (see has_issued_cert()), so a re-issue that + takes the skip path must check for these leftovers itself before calling + build-client-full, rather than let the confusing EasyRSA abort surface + after the CA passphrase prompt. + """ + candidates = [f"{pki_dir}/reqs/{cn}.req", f"{pki_dir}/private/{cn}.key"] + return [p for p in candidates if os.path.isfile(p)] + + def revoke_issued( easyrsa_dir: str, pki_dir: str, cn: str, ca_passphrase: str ) -> None: @@ -1254,6 +1269,22 @@ class CursesApp: revoked = False if is_renewal and not has_issued_cert(EASYRSA_PKI_DIR, final_cn): + # No unknown-CN check here (unlike CliRunner._issue): final_cn is + # cn_readonly in this form, always the CN the row was opened + # for, which is only ever populated from _load_current_certs() + # (see show_main_screen()/RENEW_SELECTED) — a typo'd CN can't + # reach this branch through the TUI. + leftovers = _leftover_build_paths(EASYRSA_PKI_DIR, final_cn) + if leftovers: + self._error( + stdscr, + f"Cannot skip revoke for {final_cn} — build-client-full " + f"would abort:\n" + "\n".join(leftovers) + "\n\n" + f"revoke-issued normally archives these; since there is " + f"no issued cert to revoke, move or remove them manually " + f"(this tool won't touch a private key), then retry.", + ) + return True # index.txt lists the cert but the .crt is gone, so there is # nothing EasyRSA can revoke. Issue the replacement anyway. warning = ( @@ -1526,6 +1557,34 @@ class CliRunner: revoked = False if is_renewal and not has_issued_cert(EASYRSA_PKI_DIR, cn): + # A CN with no issued .crt only takes the skip path (below) if + # index.txt still knows it — that's the migration-gap case this + # branch exists for. Anything else (a typo'd/nonexistent CN) must + # not fall through to issuing a certificate nobody asked for. + try: + known_cns = {c.cn for c in _load_current_certs(EASYRSA_PKI_DIR)} + except FileNotFoundError as exc: + print(f"error: cannot read PKI index.txt: {exc}", file=sys.stderr) + sys.exit(1) + if cn not in known_cns: + print( + f"error: unknown CN {cn!r} — no entry in index.txt and no " + f"issued certificate; nothing to reissue.", + file=sys.stderr, + ) + sys.exit(1) + leftovers = _leftover_build_paths(EASYRSA_PKI_DIR, cn) + if leftovers: + print( + f"error: cannot skip revoke for {cn} — " + f"build-client-full would abort: " + " and ".join(leftovers) + + " already exist. revoke-issued normally archives these; " + "since there is no issued cert to revoke, move or remove " + "them manually (this tool won't touch a private key), " + "then retry.", + file=sys.stderr, + ) + sys.exit(1) # index.txt lists the cert but the .crt is gone, so there is # nothing EasyRSA can revoke. Issue the replacement anyway. print( diff --git a/tests/test_app_reissue.py b/tests/test_app_reissue.py index f9d4351..4daf7f9 100644 --- a/tests/test_app_reissue.py +++ b/tests/test_app_reissue.py @@ -100,6 +100,43 @@ def test_tui_seeds_days_field_from_cert_days(monkeypatch): assert m.show_cert_form.call_args.kwargs["days"] == "90" +def test_tui_reissue_blocks_skip_when_leftover_req_exists(monkeypatch, tmp_path): + # revoke-issued normally archives pki/reqs/.req and + # pki/private/.key into pki/revoked/; when it's skipped (no issued + # .crt to revoke) those leftovers make build-client-full abort. Catch it + # with an error dialog instead of letting the confusing EasyRSA error + # surface after the CA passphrase prompt. + (tmp_path / "reqs").mkdir() + req_path = tmp_path / "reqs" / "y.kuts.req" + req_path.write_text("leftover request") + monkeypatch.setattr("openvpncertupdate.EASYRSA_PKI_DIR", str(tmp_path)) + mocks = _patch_workflow(monkeypatch, cert_file_present=False) + app = CursesApp() + shown = [] + monkeypatch.setattr(CursesApp, "_error", + lambda self, stdscr, text: shown.append(text)) + result = app._process_cert(_stdscr(), "y.kuts", "", is_renewal=True) + assert result is True + assert shown and str(req_path) in shown[0] + mocks["build_client_full"].assert_not_called() + + +def test_tui_reissue_blocks_skip_when_leftover_key_exists(monkeypatch, tmp_path): + (tmp_path / "private").mkdir() + key_path = tmp_path / "private" / "y.kuts.key" + key_path.write_text("leftover key") + monkeypatch.setattr("openvpncertupdate.EASYRSA_PKI_DIR", str(tmp_path)) + mocks = _patch_workflow(monkeypatch, cert_file_present=False) + app = CursesApp() + shown = [] + monkeypatch.setattr(CursesApp, "_error", + lambda self, stdscr, text: shown.append(text)) + result = app._process_cert(_stdscr(), "y.kuts", "", is_renewal=True) + assert result is True + assert shown and str(key_path) in shown[0] + mocks["build_client_full"].assert_not_called() + + def test_tui_forwards_form_days_to_build_client_full(monkeypatch): mocks = _patch_workflow(monkeypatch, cert_file_present=True) monkeypatch.setattr( diff --git a/tests/test_cli.py b/tests/test_cli.py index 5714fe2..0049aee 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -114,10 +114,23 @@ def test_create_does_not_touch_crl(monkeypatch, capsys): mocks["copy_crl"].assert_not_called() -def test_reissue_skips_revoke_when_cert_file_missing(monkeypatch, capsys): +def _write_index(pki_dir, cns): + """Write an index.txt with one far-future V-status line per CN, so + _load_current_certs()/has_issued_cert()'s "is this CN known?" check has + something real to read. Mirrors make_pki() in test_pki.py.""" + lines = "".join( + f"V\t350101000000Z\t\t01\tunknown\t/CN={cn}/emailAddress={cn}@example.com\n" + for cn in cns + ) + (pki_dir / "index.txt").write_text(lines) + + +def test_reissue_skips_revoke_when_cert_file_missing(monkeypatch, capsys, tmp_path): # An index.txt copied from an older EasyRSA install lists V-status certs # whose .crt was never carried over. EasyRSA reads the serial out of the # .crt, so revoke-issued can only fail — issue the replacement instead. + _write_index(tmp_path, ["y.kuts"]) + monkeypatch.setattr("openvpncertupdate.EASYRSA_PKI_DIR", str(tmp_path)) mocks = _patch_issue(monkeypatch) monkeypatch.setattr("openvpncertupdate.has_issued_cert", MagicMock(return_value=False)) CliRunner().reissue("y.kuts", "y.kuts@example.com") @@ -128,7 +141,9 @@ def test_reissue_skips_revoke_when_cert_file_missing(monkeypatch, capsys): assert mocks["build_client_full"].call_args.args[2] == "y.kuts" -def test_reissue_warns_when_revoke_skipped(monkeypatch, capsys): +def test_reissue_warns_when_revoke_skipped(monkeypatch, capsys, tmp_path): + _write_index(tmp_path, ["y.kuts"]) + monkeypatch.setattr("openvpncertupdate.EASYRSA_PKI_DIR", str(tmp_path)) mocks = _patch_issue(monkeypatch) monkeypatch.setattr("openvpncertupdate.has_issued_cert", MagicMock(return_value=False)) CliRunner().reissue("y.kuts", "y.kuts@example.com") @@ -137,10 +152,12 @@ def test_reissue_warns_when_revoke_skipped(monkeypatch, capsys): assert "issued/y.kuts.crt" in err -def test_reissue_skipped_revoke_build_failure_does_not_claim_revocation(monkeypatch, capsys): +def test_reissue_skipped_revoke_build_failure_does_not_claim_revocation(monkeypatch, capsys, tmp_path): # Nothing was revoked, so the "has been revoked but no new cert" warning # would be a lie here. import openvpncertupdate + _write_index(tmp_path, ["y.kuts"]) + monkeypatch.setattr("openvpncertupdate.EASYRSA_PKI_DIR", str(tmp_path)) _patch_issue(monkeypatch) monkeypatch.setattr("openvpncertupdate.has_issued_cert", MagicMock(return_value=False)) monkeypatch.setattr("openvpncertupdate.build_client_full", @@ -150,12 +167,89 @@ def test_reissue_skipped_revoke_build_failure_does_not_claim_revocation(monkeypa assert "has been revoked" not in capsys.readouterr().err -def _reissue_against_real_pki(monkeypatch, tmp_path, cert_files): +def test_reissue_unknown_cn_exits_without_issuing(monkeypatch, capsys, tmp_path): + # A typo'd/nonexistent CN must not fall into the "migration gap" skip + # path: before this fix it warned, skipped the revoke, and issued (and + # could email) a brand-new certificate for a CN nobody asked to renew. + _write_index(tmp_path, ["someone.else"]) + monkeypatch.setattr("openvpncertupdate.EASYRSA_PKI_DIR", str(tmp_path)) + mocks = _patch_issue(monkeypatch) + monkeypatch.setattr("openvpncertupdate.has_issued_cert", MagicMock(return_value=False)) + with pytest.raises(SystemExit) as exc_info: + CliRunner().reissue("totally-made-up-cn", "ghost@example.com") + assert exc_info.value.code == 1 + err = capsys.readouterr().err + assert "unknown" in err.lower() + assert "totally-made-up-cn" in err + mocks["build_client_full"].assert_not_called() + mocks["build_ovpn"].assert_not_called() + mocks["create_note"].assert_not_called() + mocks["send_email"].assert_not_called() + + +def test_reissue_leftover_req_blocks_skip(monkeypatch, capsys, tmp_path): + # revoke-issued normally archives pki/reqs/.req and + # pki/private/.key into pki/revoked/; when it's skipped (no issued + # .crt to revoke) those leftovers make build-client-full abort. Catch it + # before the CA passphrase prompt with an actionable message instead. + _write_index(tmp_path, ["y.kuts"]) + (tmp_path / "reqs").mkdir() + req_path = tmp_path / "reqs" / "y.kuts.req" + req_path.write_text("leftover request") + monkeypatch.setattr("openvpncertupdate.EASYRSA_PKI_DIR", str(tmp_path)) + mocks = _patch_issue(monkeypatch) + monkeypatch.setattr("openvpncertupdate.has_issued_cert", MagicMock(return_value=False)) + with pytest.raises(SystemExit) as exc_info: + CliRunner().reissue("y.kuts", "y.kuts@example.com") + assert exc_info.value.code == 1 + err = capsys.readouterr().err + assert str(req_path) in err + mocks["build_client_full"].assert_not_called() + + +def test_reissue_leftover_key_blocks_skip(monkeypatch, capsys, tmp_path): + _write_index(tmp_path, ["y.kuts"]) + (tmp_path / "private").mkdir() + key_path = tmp_path / "private" / "y.kuts.key" + key_path.write_text("leftover key") + monkeypatch.setattr("openvpncertupdate.EASYRSA_PKI_DIR", str(tmp_path)) + mocks = _patch_issue(monkeypatch) + monkeypatch.setattr("openvpncertupdate.has_issued_cert", MagicMock(return_value=False)) + with pytest.raises(SystemExit) as exc_info: + CliRunner().reissue("y.kuts", "y.kuts@example.com") + assert exc_info.value.code == 1 + err = capsys.readouterr().err + assert str(key_path) in err + mocks["build_client_full"].assert_not_called() + + +def test_reissue_missing_index_txt_exits_cleanly(monkeypatch, capsys, tmp_path): + # Covers only the _load_current_certs() call added inside _issue()'s new + # unknown-CN check: with an --email supplied, reissue()'s own + # get_email(EASYRSA_PKI_DIR, cn) fallback lookup is short-circuited + # (`final_email = email_addr or get_email(...)`) and never runs, so this + # does not exercise (or claim to fix) get_email()'s own bare open() on a + # missing index.txt when no --email is given — that gap predates this + # branch and is not one of the findings in scope here. + pki = tmp_path / "no-such-pki" + monkeypatch.setattr("openvpncertupdate.EASYRSA_PKI_DIR", str(pki)) + mocks = _patch_issue(monkeypatch) + monkeypatch.setattr("openvpncertupdate.has_issued_cert", MagicMock(return_value=False)) + with pytest.raises(SystemExit) as exc_info: + CliRunner().reissue("y.kuts", "y.kuts@example.com") + assert exc_info.value.code == 1 + assert "index.txt" in capsys.readouterr().err + mocks["build_client_full"].assert_not_called() + + +def _reissue_against_real_pki(monkeypatch, tmp_path, cert_files, index_cns=("y.kuts",)): """Run --reissue with the real has_issued_cert against a temp PKI layout.""" issued = tmp_path / "issued" issued.mkdir() for name in cert_files: (issued / name).write_text("-----BEGIN CERTIFICATE-----\n") + if index_cns: + _write_index(tmp_path, index_cns) mocks = _patch_issue(monkeypatch) monkeypatch.setattr("openvpncertupdate.has_issued_cert", real_has_issued_cert) monkeypatch.setattr("openvpncertupdate.EASYRSA_PKI_DIR", str(tmp_path)) @@ -692,4 +786,11 @@ def test_main_rejects_bad_days_flag(monkeypatch, capsys): monkeypatch.setattr("openvpncertupdate.resolve_ca_passphrase", MagicMock(return_value="")) with pytest.raises(SystemExit): main() - assert "--days" in capsys.readouterr().err + err = capsys.readouterr().err + # Must come from resolve_cert_days()'s own message, not just from + # argparse's "unrecognized arguments: --days 0" (which also contains the + # substring "--days" and would pass even if the --days flag were removed + # entirely — see resolve_cert_days()). + assert "--days" in err + assert "must be a positive number of days" in err + assert "EasyRSA rejects 0" in err diff --git a/tests/test_dialogs.py b/tests/test_dialogs.py index 1bc8c58..1e9de3f 100644 --- a/tests/test_dialogs.py +++ b/tests/test_dialogs.py @@ -134,6 +134,11 @@ def test_show_cert_form_confirm(): assert result.email == "bob@example.com" assert result.days == "90" assert len(result.password) > 0 + # Pins the field count itself: a partial reversion that drops "days" + # from _FORM_FIELDS while CertFormResult/_submit() keep it would still + # round-trip days="90" above (it's passed straight through, untraversed), + # but would only consume 4 Enters, not 5. + assert win.getch.call_count == 5 def test_show_cert_form_cancel_button():