Guard --reissue against unknown CNs and leftover build files
Code review findings on the "skip revoke when .crt is missing" migration path: - CliRunner._issue() took the skip path whenever has_issued_cert() was False, which is also true for a typo'd/nonexistent CN — it would warn, skip the revoke, and go on to build, package, and email a brand-new certificate for a CN nobody asked to renew. The skip now only fires when the CN has a current index.txt entry (via _load_current_certs()); an unknown CN prints an error and exits 1 with nothing built. The TUI's _process_cert() doesn't need the same guard — renewal there always opens on an existing row (cn_readonly pins the CN), so a typo'd CN can't reach the branch. - Skipping the revoke leaves pki/reqs/<CN>.req and pki/private/<CN>.key in place (normally revoke-issued archives both), which makes EasyRSA's build-client-full abort. Both CliRunner._issue() and CursesApp._process_cert() now check for those leftovers before building and fail fast with the exact paths, rather than surfacing EasyRSA's confusing error after the CA passphrase prompt. Neither path touches the files itself. Also: strengthened two under-specified tests (test_main_rejects_bad_days_flag now checks the resolver's message text, not just "--days", which also appears in argparse's unrelated error; test_show_cert_form_confirm now pins the Enter-keypress count so a partial "days" field reversion is caught), and folded a malformed CLAUDE.md table row into its neighbor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Vlad Doloman
@@ -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 `<PKI_DIR>/issued/<CN>.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 `<PKI_DIR>/issued/<CN>.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/<CN>.req` / `pki/private/<CN>.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 <CN>` — 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 <CN> --passout=pass:<pw>` — generates new key + cert
|
||||
|
||||
@@ -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/<CN>.req and pki/private/<CN>.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(
|
||||
|
||||
@@ -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/<CN>.req and
|
||||
# pki/private/<CN>.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(
|
||||
|
||||
@@ -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/<CN>.req and
|
||||
# pki/private/<CN>.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
|
||||
|
||||
@@ -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():
|
||||
|
||||
Reference in New Issue
Block a user