Repository navigation
fix(system): ten self-contained audit findings - #183
Merged
Merged
Conversation
`OBS_KINDS` exists so rows read back from `world_observations` can be interned into the restart dedup set. `health_check_fail`, `unit_start_limit_hit` and `volume_backend_mismatch` are all persisted by `ObservationFact::to_obs_kinds` but were missing from it, so those rows failed to intern and were dropped from the seed — defeating the very thing the seed is for. A container unhealthy across a restart had a duplicate `health_check_fail` appended with a fresh timestamp on every boot, and the same for a start-limit-hit unit and a mismatched volume backend. The list and the mapping are separate by construction, so a test now walks every `ObservationFact` variant and asserts each kind it emits can be interned — the drift is what caused this, not the three names.
`list_images_impl` filtered `repo_tags` against the exact literal `<none>:<none>`. Podman and libpod report untagged images differently across versions — a bare `<none>`, or `<none>` in either half — so those reached callers as if they were real tags and an untagged image looked tagged. The check is now about what the entry means rather than one spelling of it, and a real tag containing the word `none` is still kept.
`l[rt.signal]` says container instances that are not running are "silently skipped (no error)". Podman answers a kill against a `created` or `exited` container with 409 "container is not running", which is neither a 404 nor one of the "no such container" 500s, so it took the error path: the caller logged a warning for something the spec defines as a silent skip. Gone and not-running now both mean skipped. Other 409s stay errors.
`r[infra.nat64.detection]` counts an AAAA for `ipv4only.arpa` as evidence of NAT64 only when it is outside the canonical `192.0.0.170`/`192.0.0.171` addresses. The code treated any IPv6 result as proof, so a resolver stack handing those canonical A records back in IPv4-mapped form (`::ffff:192.0.0.170`) — which has synthesised nothing — convinced the host the network already provided NAT64, and it declined to activate its own. A genuine synthesis embeds the canonical IPv4 under a NAT64 prefix, so the embedded address matching is expected and is not what disqualifies it; being only a v4-mapped canonical address is.
The RTM_GETROUTE dump returns routes from every table, and the egress probes accepted a `/0` unicast route from any of them. docs/networking.md specifies a default route "in the main table", and `data_plane/routes.rs` already filters that way — so a host whose only default route lives in a policy-routing table reported IPv4 or IPv6 egress it does not have, which then feeds NAT64 activation and endpoint routability decisions. `effective_table` was private to the data plane and is now shared rather than copied, since both places are answering the same question, and 254 becomes `MAIN_ROUTE_TABLE` at its definition. The probes themselves need a netlink handle so they stay untested here; the helper they now depend on did not have tests and does.
…tent The stub took the last argv element containing `/` or `:` as the image. `podman_args` puts the image before the entrypoint and command arguments, so any container whose trailing arguments contain either — `["node", "server/index.js"]`, `["sh", "-c", "…"]` — had a command argument recorded as its image, and a phantom `StubImage` minted for it, corrupting stub-mode image bookkeeping. Taking the first such element instead would be no better: a volume mount like `-v /host:/ctr` comes earlier and has both characters. The image is the first positional, so the flags are skipped properly — a `-`-prefixed element carries its value after `=` or in the next element.
…lica `desired.resources` holds an entry per instance, so a deployment scaled to N built N identical groups. Each group then expanded to all N instances at lookup time, giving N² entries for one resource — and the event loop compared every copy against the same `prev_states`, so a single state change was announced once per replica. A client driven by the event feed saw the same `resource_state_changed` repeated, which for anything counting transitions is not a cosmetic difference. The dedupe is extracted so it can be tested without a reconciler and a database behind it.
The HTTP and HTTPS servers are built independently from the listener set, and nothing stopped both listing `:P`. Caddy rejects a config where two servers bind the same address, and `POST /config/` is all-or-nothing — so one such pair invalidated every route and every cert policy on the node, far past the ingress that caused it. The apply path checks before sending and fails with the ports named, so the previous good config stays in force and the error says what to change. Reported rather than quietly resolved: dropping one side would serve an ingress on a protocol its author did not ask for.
`spec_hash` hashes the podman argv, but `stop_signal` and `stop_timeout_secs` are applied as systemd unit properties and never appear there. `r[update.spec-hash]` wants the hash to capture the full configuration; changing either left the hash identical, so the reconciler saw a running instance as up to date and the new stop behaviour never reached it. They are appended only when set. A container using neither keeps the hash it already has, so this does not restart the fleet; the ones that do use them had a hash that was wrong, and get re-evaluated once.
`podman exec [--env …] <name> <argv…>` passed the container name as a bare positional after the flags. Podman stops parsing options after the container name, so the command arguments were never at risk — the audit says as much — but the name itself is only safe because name validation forbids a leading dash, which is a rule enforced somewhere else entirely. A `--` separator makes it structural rather than contingent. Defence in depth, and the argv construction is now a pure function with a test rather than a sequence of `cmd.arg` calls nothing could inspect.
Contributor
Code Coverage OverviewLanguages: TypeScript, Rust TypeScript / code-coverage/vitestThe overall line coverage in commit 4b8c168 in the Rust / code-coverage/rustThe overall line coverage in commit 4b8c168 in the Show a line coverage summary of the most impacted files.
Updated |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 Ten trivial findings from the logic-bug audit in the system layer, one commit each. This is the last of the six subsystem sweeps.
Three persisted observation kinds could not be interned.
health_check_fail,unit_start_limit_hitandvolume_backend_mismatchare written byto_obs_kindsbut were missing fromOBS_KINDS, so their rows were dropped from the restart seed — defeating the thing the seed exists for, and appending a duplicate with a fresh timestamp on every boot. A test now walks everyObservationFactvariant, since the drift is what caused this rather than the three names.Untagged images looked tagged. Only the literal
<none>:<none>was filtered; podman versions also report a bare<none>or<none>in one half.Signalling a stopped container was an error, not a skip.
l[rt.signal]says non-running instances are silently skipped, but podman's 409 "container is not running" is neither a 404 nor a "no such container" 500, so it took the error path and logged a warning for a documented no-op.NAT64 detection accepted a v4-mapped canonical address.
r[infra.nat64.detection]counts an AAAA only when it is outside192.0.0.170/.171; any IPv6 result was taken as proof, so a stack returning those canonical records in::ffff:form convinced the host the network already had NAT64 and it declined to activate its own.Egress probes accepted a default route from any table. A
/0confined to a policy-routing table is not the system's egress path.data_plane/routes.rsalready filtered on the main table, soeffective_tableis shared rather than copied, and 254 becomesMAIN_ROUTE_TABLE.The stub read the image from the wrong argv element. It took the last element containing
/or:, which is a command argument whenever one has either, minting a phantomStubImage. Taking the first is no better — a volume mount comes earlier — so it skips flags properly and takes the first positional.State-change events were emitted once per replica.
desired.resourcesholds an entry per instance, so a deployment scaled to N built N identical groups, each expanding to all N instances: N² entries, and one change announced N times.A port declared as both plaintext and TLS took down all routing. Caddy refuses a config where two servers bind one address and
POST /config/is all-or-nothing, so a single bad pair invalidated every route and cert policy on the node. The apply path refuses before sending, naming the ports, so the previous good config stays in force.spec_hashmissed the stop signal and timeout. Both are applied as systemd unit properties and never appear in the argv, so changing either left the reconciler seeing the instance as current. Appended only when set, so containers using neither keep their hash and are not restarted for this.podman execpassed the container name as a bare positional. Command arguments were never at risk — podman stops parsing options after the name — but the name is safe only because validation forbids a leading dash, which is a rule enforced elsewhere.--makes it structural.One finding from this subsystem is not here. The HTTP→HTTPS redirect targeting the node's first HTTPS port rather than the vhost's works today only because the shared
seedling_httpsserver serves every vhost on every HTTPS port — and it stops working by accident the moment L4 scopes routes to their port. Both want the same missing fact, so it is on card Y4 to be done with L4.