IsoHex/edge-router

Watch

Fork 0

A removed reserved-name route keeps refusing the handshake until restart #53

New issue

Closed

opened 2026-10-26 13:31:36 +00:00 by james.oates · 1 comment

james.oates commented

2026-10-26 13:31:36 +00:00

Owner

Found while confirming the resolver's refuse-vs-fallback rule is internally consistent. Pre-existing, not from #52's fixes.

The rule, as everything else implements it

Whether a name is refused or served the self-signed fallback turns on whether a configured route host covers it — exactly, or by wildcard:

Name	Outcome
Covered by a route host, certificate usable	serve it
Covered by a route host, no usable certificate	refuse (tombstone)
Not covered by any route host	self-signed fallback

SniResolver::remove's doc states the third row from the other direction: "Forget a name entirely — it is no longer configured, so the fallback applies to it again."

Where it breaks

A reserved name (.localhost, .test, …) never gets a certificates row. reconcile_once's skip branch tombstones it in the resolver and continues before any DB write:

if !is_issuable(name) {
    let why = "not an ACME-issuable name";
    resolver.mark_not_certifiable(name, why);
    report.skipped.push((name.clone(), why.to_string()));
    continue;
}

But the only cleanup path iterates DB rows:

for cert in &existing {
    if !desired.contains(&cert.name) {
        db.delete_certificate(&cert.name).await?;
        resolver.remove(&cert.name);
        ...
    }
}

resolver.remove is called in exactly one place in the codebase, and it is this one. A name with no row is never reached, so the NotCertifiable tombstone outlives the route that created it for the life of the process.

Reproduction

Add a route with host = "grafana.localhost". The reconciler logs it skipped; the resolver holds NotCertifiable.
Remove that route from the TOML. The config watcher reloads and the name leaves the desired set.
grafana.localhost still aborts the handshake with an access-denied alert. It should now be an unconfigured name and get the fallback.
Restart — correct behaviour returns, because load_into_resolver rebuilds from DB rows and there is no row to tombstone.

Why only this branch

The issuable-name failure path is unaffected: db.record_certificate_error is an INSERT … ON CONFLICT, so even a name whose first order fails has a row, and the deletion loop cleans it up normally. The skip branch is the one path that tombstones without a row.

Impact

Low — it only affects names no public CA can issue for, which are development names by definition, and a restart clears it. Filing because it is a genuine inconsistency in the posture rule rather than a cosmetic one: the same name is refused or served the fallback depending on process history rather than current config.

Options

Have the skip branch write a row (symmetric with the failure path, and it would surface on GET /api/certificates, where a skipped name is currently invisible).
Or reconcile the resolver's own key set against desired rather than against DB rows, which fixes this class rather than this instance.

The second is the better shape — the resolver is described as the single source of truth for what ER can serve, so converging it from the desired set directly removes the dependency on a DB row existing.

Found while confirming the resolver's refuse-vs-fallback rule is internally consistent. Pre-existing, not from #52's fixes. ## The rule, as everything else implements it Whether a name is refused or served the self-signed fallback turns on **whether a configured route host covers it** — exactly, or by wildcard: | Name | Outcome | |---|---| | Covered by a route host, certificate usable | serve it | | Covered by a route host, no usable certificate | refuse (tombstone) | | Not covered by any route host | self-signed fallback | `SniResolver::remove`'s doc states the third row from the other direction: "Forget a name entirely — it is no longer configured, so the fallback applies to it again." ## Where it breaks A reserved name (`.localhost`, `.test`, …) never gets a `certificates` row. `reconcile_once`'s skip branch tombstones it in the resolver and `continue`s before any DB write: ```rust if !is_issuable(name) { let why = "not an ACME-issuable name"; resolver.mark_not_certifiable(name, why); report.skipped.push((name.clone(), why.to_string())); continue; } ``` But the only cleanup path iterates DB rows: ```rust for cert in &existing { if !desired.contains(&cert.name) { db.delete_certificate(&cert.name).await?; resolver.remove(&cert.name); ... } } ``` `resolver.remove` is called in exactly one place in the codebase, and it is this one. A name with no row is never reached, so the `NotCertifiable` tombstone outlives the route that created it for the life of the process. ## Reproduction 1. Add a route with `host = "grafana.localhost"`. The reconciler logs it skipped; the resolver holds `NotCertifiable`. 2. Remove that route from the TOML. The config watcher reloads and the name leaves the desired set. 3. `grafana.localhost` still aborts the handshake with an access-denied alert. It should now be an unconfigured name and get the fallback. 4. Restart — correct behaviour returns, because `load_into_resolver` rebuilds from DB rows and there is no row to tombstone. ## Why only this branch The issuable-name failure path is unaffected: `db.record_certificate_error` is an `INSERT … ON CONFLICT`, so even a name whose first order fails has a row, and the deletion loop cleans it up normally. The skip branch is the one path that tombstones without a row. ## Impact Low — it only affects names no public CA can issue for, which are development names by definition, and a restart clears it. Filing because it is a genuine inconsistency in the posture rule rather than a cosmetic one: the same name is refused or served the fallback depending on process history rather than current config. ## Options - Have the skip branch write a row (symmetric with the failure path, and it would surface on `GET /api/certificates`, where a skipped name is currently invisible). - Or reconcile the resolver's own key set against `desired` rather than against DB rows, which fixes this class rather than this instance. The second is the better shape — the resolver is described as the single source of truth for what ER can serve, so converging it from the desired set directly removes the dependency on a DB row existing.

james.oates referenced this issue

2026-10-26 15:38:00 +00:00

A name has no resolver entry until the serial reconcile loop reaches it, so it is served the fallback #57

james.oates added this to the v2.1 milestone

2026-10-26 17:53:32 +00:00

james.oates referenced this issue

2026-10-26 17:53:55 +00:00

The WebSocket path has no connect timeout, no idle timeout and no connection cap #62

james.oates referenced this issue from a commit

2026-10-27 11:06:18 +00:00

Converge the resolver on the desired set, not the DB rows (#53)

james.oates commented

2026-10-27 11:06:29 +00:00

Author

Owner

Fixed in d0801d3, taking your second option — the resolver's key set now converges on desired rather than on the DB rows.

Your reasoning for preferring it holds up: the resolver is documented as the single source of truth for what Edge Router can serve, so converging it from the desired set removes the dependency on a row existing, instead of writing a row for skipped names to paper over the one case that lacks one.

Worth separating from the loop above it, which that code warns is irreversible — it deletes a private key, so a transient absence costs a re-order against the weekly limit. The resolver convergence is not that: it forgets an in-memory entry, making the name unconfigured, which is what it now is. No key is touched, and any entry worth keeping belongs to a desired name. It inherits the same "desired is real" assumption as the row deletion without widening it, and if a spurious empty set ever did get through, the row loop's key deletion is already the worse outcome.

The test runs two passes — the name tombstoned while its route exists, then absent — and asserts the premise in between: db.list_certificates() is empty, so there is genuinely no row to clean up. Without that assertion it could have passed for the wrong reason. It failed on the final assertion only, before the fix.

384 tests pass, clippy --all-targets -D warnings clean, fmt clean. Invariant recorded in CLAUDE.md next to the tombstone-seeding one, since the two are easy to conflate.

No release-notes entry: the behaviour was reachable only with development names and cleared on restart, and the notes already describe the refuse-vs-fallback rule it restores.

Fixed in `d0801d3`, taking your second option — the resolver's key set now converges on `desired` rather than on the DB rows. Your reasoning for preferring it holds up: the resolver is documented as the single source of truth for what Edge Router can serve, so converging it from the desired set removes the dependency on a row existing, instead of writing a row for skipped names to paper over the one case that lacks one. Worth separating from the loop above it, which that code warns is irreversible — it deletes a private key, so a transient absence costs a re-order against the weekly limit. The resolver convergence is not that: it forgets an in-memory entry, making the name unconfigured, which is what it now is. No key is touched, and any entry worth keeping belongs to a desired name. It inherits the same "desired is real" assumption as the row deletion without widening it, and if a spurious empty set ever did get through, the row loop's key deletion is already the worse outcome. The test runs two passes — the name tombstoned while its route exists, then absent — and asserts the premise in between: `db.list_certificates()` is empty, so there is genuinely no row to clean up. Without that assertion it could have passed for the wrong reason. It failed on the final assertion only, before the fix. 384 tests pass, clippy `--all-targets -D warnings` clean, fmt clean. Invariant recorded in CLAUDE.md next to the tombstone-seeding one, since the two are easy to conflate. No release-notes entry: the behaviour was reachable only with development names and cleared on restart, and the notes already describe the refuse-vs-fallback rule it restores.

james.oates closed this issue

2026-10-27 11:06:30 +00:00

No labels

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

IsoHex/edge-router#53

No description provided.

Rows
Columns