Keel-Linux/keel

Fork 0

Resolve layers by channel and verify the signed pointer #34

Merged

marcos-mendez merged 8 commits from feat/channel-layout into main

2026-09-29 00:55:48 -03:00

marcos-mendez commented

2026-09-28 01:35:48 -03:00

(Migrated from github.com)

Closes #33.

The client half of the mirror layout the maintainer decided on 2026-09-28. It
lands before the publisher writes the new layout (Keel-Linux/apt#14), and
that order is not incidental: a client that could not read the flat layout,
shipped after the mirror is converted, would strand every appliance already in
the field. Both layouts are read here and the flat one is untouched; removing
it is a separate, later change.

What it reads

layers/
  sha256/<digest>                   blob, never overwritten, never deleted
  <release>/<rev>/<name>.manifest   an immutable release revision
  stable, testing                   the channel pointers, clear signed
  <name>.manifest, <name>.tar.zst   the flat layout, still served

keel pull --channel stable fetches the pointer, verifies it, refuses it when
it has expired, and resolves the revision it names. The digest chain is then
complete: the signature covers the sha256 of every manifest, and each manifest
covers the sha256 of its blob. A layer the pointer does not name is a mismatch,
because nothing signed says which manifest it should have.

--release YYYY-MM-DD --rev N names a revision directly and follows no
pointer. That is rollback, and it always works, because a blob is never
deleted.

An expired pointer is an error

Exit 19, CHANNEL_EXPIRED, never a warning. Without it a mirror that is stale,
broken or hostile holds an appliance on an old and vulnerable release simply by
not updating, and the client cannot tell that apart from "nothing new". A
pointer that goes backwards is exit 20, CHANNEL_ROLLBACK: a mirror does not
move an appliance backwards, an operator does.

Note what the expiry does and does not cover. The channel key is online, so it
protects against a stale or hostile mirror, which is a different machine
from the one holding the key. It does not protect against a compromised build
host, and nothing signed on the build host could.

What the review found, and what changed

Two of the claims below were false when this was first pushed, and both are
fixed with tests that fail without the fix.

A revoked or expired signing key verified. gpgv exits 0 for both and
still prints VALIDSIG, with Good signature from on stderr; GOODSIG is
the only line it withholds, substituting REVKEYSIG/EXPKEYSIG. A pointer
signed by a key revoked in the very keyring passed to --channel-keyring,
with --channel-signer pinned to its fingerprint, fetched the layer at exit
0. Since the channel key is the one unattended online key in this design and
revocation is the only answer to its theft, that made the answer do nothing.
GOODSIG is now required and the retirement lines refused by name. Proven
end to end against really revoked and really expired keys, made in the test
helper. --weak-digest SHA1 too, because a SHA-1 pointer got its own
GOODSIG.

--release/--rev verified nothing at all. No pointer was fetched and no
manifest digest was checked, so a mirror serving no layers/stable and a
rewritten release manifest put attacker bytes in the cache at exit 0 — on
the documented rollback path, which is used precisely when the mirror is
least trusted. Each revision now keeps the pointer that was signed for it at
layers/<release>/<rev>/revision; --release/--rev verifies it by the same
chain and requires it to name the revision it is filed under. Only the
expiry is not enforced, because an earlier revision is old on purpose, and
the output says so.

Also: an expiry now has a 30 day ceiling and a pointer signed in the
future is refused (unbounded, one signature is a permanent freeze and the
rollback refusal never fires); an expired pointer's message prints this
machine's clock and names a wrong local clock as one of the three
explanations; a flat pull says it read no pointer instead of printing what a
verified pull prints; a .hash file is bounded and dropped when it
disagrees with the manifest, so the mirror cannot choose keel verify's exit
code; and the two strings saying no trusted key is configured, which this
change makes false, are gone.

On the tests, which mattered more than the number. The suite now fails
rather than skips when no verifier is installed — with gpg and gpgv off
PATH the gate passed at 97 percent with signature.py at 15, and a gate
that passes over unrun verification code is the shape this project keeps
fixing. --channel-signer has end-to-end tests at both call sites and in
its one reader; all three mutants that previously survived now die. The
blob-sharing test names the other release instead of resolving the same one
twice, and the parent_sha256 test asserts the substring.

1010 tests, 100 percent line and branch, and CI shows signature.py at 100
percent, which is how you can tell the verifier actually ran.

Two properties of gpgv, measured rather than assumed

gpgv is the verifier because it refuses to do anything but verify: it cannot
be talked into importing a key, consulting a trust database or asking an agent.

It is not assumed to be installed. The first draft here said it is on every
appliance because apt verifies InRelease with it. That is no longer true — on
Debian 13 apt verifies with sqv and gpgv is a package of its own, which
Keel-Linux/apt's own README already records for bin/verify-repo. The package
now depends on gpgv, because the one refusal a client must never make quietly
is "I could not check this".

It writes the plain text of a document whose signature it refused.
gpgv --output produced the body of a tampered pointer and of one signed by
a key outside the keyring, in both cases beside a non zero exit. The output
is therefore read only after the exit status and a VALIDSIG line have been
accepted, and a refusal raises instead of returning text.
It reads a binary keyring only. An armored keyring, which is the form
this project publishes keys in, gives NO_PUBKEY and exit 2. An armored
keyring is accepted and dearmored in memory.

Both have a test that fails if the behaviour is ever taken on trust.

keel inspect

The channel and revision are recorded in /var/lib/keel/channel and reported
as layers.channel and layers.revision. --check-channel asks the mirror
what that channel now holds and says whether the instance is behind; it is the
only network request inspect makes, so it is off by default, and its
refusals win over INSPECT_INCOMPLETE.

Tests

1010 tests, 100 percent line and branch coverage (coverage report, gate is
95). The adversarial cases each have a test: an expired pointer, a pointer
signed by the wrong key, a pointer signed by a key that is in the keyring but
not allowed to move a channel, a pointer altered after signing, an unsigned
pointer, a pointer for another channel, a manifest whose bytes do not hash to
what the pointer signed, a manifest the pointer does not name, a blob that is
not what the manifest records, a rollback, and one blob serving two releases
and fetched once.

gnupg and gpgv were added to the workflow's apt packages: without them
those tests skip, and a skipped suite would pass the coverage gate on code
nothing ran.

Publishing side: Keel-Linux/apt#14, which should merge after this one
Decision note 0016: Keel-Linux/handbook#10

Closes #33. The client half of the mirror layout the maintainer decided on 2026-09-28. It lands **before** the publisher writes the new layout (Keel-Linux/apt#14), and that order is not incidental: a client that could not read the flat layout, shipped after the mirror is converted, would strand every appliance already in the field. Both layouts are read here and the flat one is untouched; removing it is a separate, later change. ## What it reads ``` layers/ sha256/<digest> blob, never overwritten, never deleted <release>/<rev>/<name>.manifest an immutable release revision stable, testing the channel pointers, clear signed <name>.manifest, <name>.tar.zst the flat layout, still served ``` `keel pull --channel stable` fetches the pointer, verifies it, refuses it when it has expired, and resolves the revision it names. The digest chain is then complete: the signature covers the sha256 of every manifest, and each manifest covers the sha256 of its blob. A layer the pointer does not name is a mismatch, because nothing signed says which manifest it should have. `--release YYYY-MM-DD --rev N` names a revision directly and follows no pointer. That is rollback, and it always works, because a blob is never deleted. ## An expired pointer is an error Exit 19, `CHANNEL_EXPIRED`, never a warning. Without it a mirror that is stale, broken or hostile holds an appliance on an old and vulnerable release simply by not updating, and the client cannot tell that apart from "nothing new". A pointer that goes backwards is exit 20, `CHANNEL_ROLLBACK`: a mirror does not move an appliance backwards, an operator does. Note what the expiry does and does not cover. The channel key is online, so it protects against a stale or hostile **mirror**, which is a different machine from the one holding the key. It does not protect against a compromised build host, and nothing signed on the build host could. ## What the review found, and what changed Two of the claims below were false when this was first pushed, and both are fixed with tests that fail without the fix. **A revoked or expired signing key verified.** gpgv exits 0 for both and still prints `VALIDSIG`, with `Good signature from` on stderr; `GOODSIG` is the only line it withholds, substituting `REVKEYSIG`/`EXPKEYSIG`. A pointer signed by a key revoked in the very keyring passed to `--channel-keyring`, with `--channel-signer` pinned to its fingerprint, fetched the layer at exit 0. Since the channel key is the one unattended online key in this design and revocation is the only answer to its theft, that made the answer do nothing. `GOODSIG` is now required and the retirement lines refused by name. Proven end to end against really revoked and really expired keys, made in the test helper. `--weak-digest SHA1` too, because a SHA-1 pointer got its own `GOODSIG`. **`--release/--rev` verified nothing at all.** No pointer was fetched and no manifest digest was checked, so a mirror serving no `layers/stable` and a rewritten release manifest put attacker bytes in the cache at exit 0 — on the documented rollback path, which is used precisely when the mirror is least trusted. Each revision now keeps the pointer that was signed for it at `layers/<release>/<rev>/revision`; `--release/--rev` verifies it by the same chain and requires it to name the revision it is filed under. Only the expiry is not enforced, because an earlier revision is old on purpose, and the output says so. Also: an expiry now has a **30 day ceiling** and a pointer signed in the future is refused (unbounded, one signature is a permanent freeze and the rollback refusal never fires); an expired pointer's message prints this machine's clock and names a wrong local clock as one of the three explanations; a flat pull says it read no pointer instead of printing what a verified pull prints; a `.hash` file is bounded and dropped when it disagrees with the manifest, so the mirror cannot choose `keel verify`'s exit code; and the two strings saying no trusted key is configured, which this change makes false, are gone. **On the tests, which mattered more than the number.** The suite now fails rather than skips when no verifier is installed — with `gpg` and `gpgv` off `PATH` the gate passed at 97 percent with `signature.py` at 15, and a gate that passes over unrun verification code is the shape this project keeps fixing. `--channel-signer` has end-to-end tests at both call sites and in its one reader; all three mutants that previously survived now die. The blob-sharing test names the other release instead of resolving the same one twice, and the `parent_sha256` test asserts the substring. 1010 tests, 100 percent line and branch, and CI shows `signature.py` at 100 percent, which is how you can tell the verifier actually ran. ## Two properties of gpgv, measured rather than assumed `gpgv` is the verifier because it refuses to do anything but verify: it cannot be talked into importing a key, consulting a trust database or asking an agent. **It is not assumed to be installed.** The first draft here said it is on every appliance because apt verifies `InRelease` with it. That is no longer true — on Debian 13 apt verifies with `sqv` and `gpgv` is a package of its own, which Keel-Linux/apt's own README already records for `bin/verify-repo`. The package now depends on `gpgv`, because the one refusal a client must never make quietly is "I could not check this". - **It writes the plain text of a document whose signature it refused.** `gpgv --output` produced the body of a tampered pointer and of one signed by a key outside the keyring, in both cases beside a non zero exit. The output is therefore read only after the exit status and a `VALIDSIG` line have been accepted, and a refusal raises instead of returning text. - **It reads a binary keyring only.** An armored keyring, which is the form this project publishes keys in, gives `NO_PUBKEY` and exit 2. An armored keyring is accepted and dearmored in memory. Both have a test that fails if the behaviour is ever taken on trust. ## keel inspect The channel and revision are recorded in `/var/lib/keel/channel` and reported as `layers.channel` and `layers.revision`. `--check-channel` asks the mirror what that channel now holds and says whether the instance is behind; it is the only network request `inspect` makes, so it is off by default, and its refusals win over `INSPECT_INCOMPLETE`. ## Tests 1010 tests, **100 percent line and branch coverage** (`coverage report`, gate is 95). The adversarial cases each have a test: an expired pointer, a pointer signed by the wrong key, a pointer signed by a key that is in the keyring but not allowed to move a channel, a pointer altered after signing, an unsigned pointer, a pointer for another channel, a manifest whose bytes do not hash to what the pointer signed, a manifest the pointer does not name, a blob that is not what the manifest records, a rollback, and one blob serving two releases and fetched once. `gnupg` and `gpgv` were added to the workflow's apt packages: without them those tests skip, and a skipped suite would pass the coverage gate on code nothing ran. ## Related - Publishing side: Keel-Linux/apt#14, which should merge after this one - Decision note 0016: Keel-Linux/handbook#10

marcos-mendez commented

2026-09-28 04:57:45 -03:00

(Migrated from github.com)

Reviewed as security code. Everything marked measured was run here on Debian 13.7 / gpgv 2.4.7 against this branch; everything marked read is from the diff.

CRITICAL — a revoked or expired signing key verifies

keel/layers/signature.py:141-153 treats any zero exit from gpgv as success, and :156-166 accepts any VALIDSIG line. Neither is sufficient. Measured, gpgv 2.4.7:

signature by	exit	status lines
good key	0	`GOODSIG`, `VALIDSIG`
revoked primary	0	`REVKEYSIG`, `VALIDSIG`
revoked signing subkey under a live primary	0	`REVKEYSIG`, `VALIDSIG`
expired key	0	`KEYEXPIRED`, `EXPKEYSIG`, `VALIDSIG`

stderr says Good signature from in all four.

End to end against this branch, not at the gpgv level — pointer signed by a key that is revoked in the very keyring passed to --channel-keyring, with --channel-signer pinned to that fingerprint:

$ keel pull core --source $M/layers --cache-dir $C \
    --channel stable --channel-keyring keyring-revoked.gpg \
    --channel-signer 9EDC363B5B926536424A9DC13F6006762A3A31E2 --channel-state $S
channel stable: release 2026-09-28/1, signed 2026-09-28T07:36:53Z, expires 2026-10-05T07:36:53Z
core: fetched (21 bytes)
layers: 1 resolved, 1 fetched, 0 cached, 21 bytes transferred
EXIT=0

Same result for an expired signing key (EXIT=0, layer fetched, /var/lib/keel/channel written).

Note the subkey row. VALIDSIG's last field is the primary fingerprint, and _check_signers (signature.py:169-180) intersects against both, so pinning --channel-signer <primary> matches a signature made by a revoked subkey of that primary. That is the exact shape of apt/keys/keel-archive-keyring.asc, which already carries sub D276B62C2BD16F4E ... r — a signing subkey this project has already had to revoke once.

Where it bites. The channel key is, by design, the unattended key on the build host — the most likely key in the project to be stolen. The one response to that is to revoke it and ship the revocation in keel-channel-keyring. With this code that does nothing: every appliance keeps accepting the thief's pointers. Worse, it keeps accepting them until a new keyring reaches every appliance, and preventing a new keyring from reaching appliances is exactly what the stolen key can now do — pin them to a stale revision with a long expiry (see the MEDIUM on TTL below).

Fix, one line each: require a GOODSIG line and not only VALIDSIG — gpgv emits GOODSIG only when the key is neither revoked nor expired, and replaces it with REVKEYSIG/EXPKEYSIG otherwise — or refuse outright when REVKEYSIG|EXPKEYSIG|KEYREVOKED|KEYEXPIRED appears on the status fd. The same applies to apt/lib/channel.sh:170.

Related, same call site: VERIFIER = ("gpgv",) at constants.py:79 passes no --weak-digest SHA1. A SHA-1 clearsigned pointer verifies today (measured: exit 0, GOODSIG, VALIDSIG).

HIGH — `--release/--rev` verifies no signature at all, and the output does not say so

keel/layers/pull.py:471-476. The by-release branch builds Places(release, rev) with digests=None, so Places.digest_problem (:198-201) returns None for every layer, and no pointer is fetched. The chain the whole PR is about — signature → manifest digest → blob digest — is simply absent on this path.

Measured against a hostile mirror that serves no layers/stable at all and has had 2026-09-28/1/core.manifest rewritten to point at an attacker blob:

$ keel pull core --source $M/layers --cache-dir $C --release 2026-09-28 --rev 1
release 2026-09-28/1, named on the command line
core: fetched (27 bytes)
layers: 1 resolved, 1 fetched, 0 cached, 27 bytes transferred
EXIT=0
$ cat $C/core-*.tar.zst
ATTACKER CONTROLLED ROOTFS

docs/layers.md calls this "how a rollback is asked for". Rollback is the operation an operator performs after something has gone wrong, against the mirror they have the least reason to trust, and it is the path with no verification on it. keel/layers/pull.py:176-177 is honest about it in a docstring; nothing the operator sees is. Passing --channel stable alongside --release/--rev does not help — by_release wins at :100-110 and the pointer is never fetched.

The publisher signs a release MANIFEST with the release key and now records revision inside it (Keel-Linux/apt#14). The client never fetches it. Either resolve --release/--rev through that signed MANIFEST, or at minimum make resolution_line() say the revision was not verified.

HIGH — nothing ships the default keyring, so the feature is dead on a stock appliance

constants.py:24 defaults to /usr/share/keyrings/keel-channel-keyring.gpg. debian/keel.install ships examples/instance.yaml and the firstboot hook and nothing else; no repository in the project produces that path (keel-transition ships keel-archive-keyring.gpg, apt/keys/ publishes keel-staging-keyring.asc), and debian/control gains gpgv but no keyring dependency. keel pull --channel stable on an installed appliance exits 18 on a missing file.

It fails closed, which is right. But the feature ships unusable, and the one refusal an operator will actually see looks identical to an attack.

HIGH — the coverage gate is green when no verifier is present

.github/workflows/tests.yml:20-23 reasons correctly that a skipped suite would pass the gate on code nothing ran, and then relies entirely on the apt-packages: line to prevent it. Measured: re-running the exact CI command with gpg and gpgv off PATH gives 897 passed, 74 skipped, TOTAL … 97%, and coverage report --fail-under=95 exits 0. keel/layers/signature.py is at 15% on that run — the entire verifier unrun — and pull.py at 76%.

The project already has the right guard one file away: tests/test_hook_bats.py:32-36 fails rather than skips when CI is set. tests/channel_helpers.py:44-49 has no equivalent.

HIGH — `--channel-signer` has no end-to-end test

grep -rn "channel.signer\|channel_signers" tests/ returns nothing. Mutation run: replacing the signers argument with () at keel/commands.py:383 (pull) and keel/commands.py:259 (inspect --check-channel) leaves all 971 tests passing at 100% line and branch. The flag works today (measured: exit 18, "which is not the key that may move a channel"); the net under it does not exist. It is also the clearest demonstration that 100/100 is not the property being claimed for this suite.

(Credit where due: 17 of 19 other mutants died. Removing the signer allowlist, the expiry refusal, the manifest-digest check, the whole signature step, the rollback check, the blob digest check, the channel-name binding, the no-keyring refusal, expired()'s >=, and the SIGNATURE_PRESENT wording each turned tests red. The adversarial cases assert behaviour: the two keys really are two distinct keys, the tampering is inside an intact clearsign envelope so verification and not parsing is what refuses, and the unsigned case uses a body that would otherwise validate. This is a good suite.)

MEDIUM — no ceiling on `expires_at`, in either direction

channel.py:206-220 requires only expires_at > signed_at. Measured, both accepted, exit 0:

signed now, expires_at 2126-01-01T00:00:00Z
signed_at 2076-01-01T00:00:00Z (fifty years in the future)

This is the attack on decision 0016's argument. The note says the online key "can only say which of those is current, and until when. That is all." Unbounded, "until when" is a permanent freeze primitive: one signature pins an appliance to a known-vulnerable already-staged revision indefinitely, and is_rollback never fires because the revision does not go backwards. The attacker does not even need to keep re-signing, which is the weaker version the note concedes. The publisher takes KEEL_CHANNEL_TTL_DAYS unvalidated (apt/lib/channel.sh:64,198), so nothing bounds it at either end — and "lengthening the expiry until it means nothing" is the exact failure mode the note says the whole design exists to prevent.

A client-side maximum (expires_at - signed_at <= N days, and signed_at <= now + skew) turns "forever" into "N days" and costs two comparisons.

MEDIUM — clock skew is invisible and misdiagnosed

channel.py:85-97. Clock ahead: every pointer is expired, exit 19, and the message is "the mirror is not being updated, or is holding this instance back" — it never prints now and never mentions the local clock. A fleet-wide NTP failure sends the operator to the mirror. Stalled rather than bricked, since the flat layout and --release/--rev still work, but the diagnosis is actively wrong.

Clock behind: dead pointers still look fresh, and the freeze attack the expiry exists to stop succeeds. An LXC container restored from a snapshot, or one on a host with a bad RTC, is exactly that case. Print now in staleness(), and refuse a signed_at in the future.

MEDIUM — a flat pull is indistinguishable from a verified one in the output

PullReport.resolution_line() (pull.py:155-166) returns None for the flat layout, so a pull that verified nothing prints the same layer lines and the same summary as one that checked a signature. --channel has choices=, so a typo is caught — but forgetting it entirely is not. One line saying the flat layout follows no pointer would close it.

MEDIUM — an unauthenticated `.hash` file decides `keel verify`'s exit code

pull.py:348-366 writes whatever the mirror serves at <tarball>.hash into the cache with no validation; verify.py:196-226 then reads it. Measured, four mirrors differing only in that file, byte-perfect tarball and a validly signed pointer in every case: no .hash → exit 9 and ok; truthful unsigned → exit 8; .hash lying about the sha256 → exit 7 LAYER_MISMATCH on a layer that matches its signed manifest exactly; two PGP marker strings and no signature → exit 8 "signature present".

It can only downgrade a verdict, never upgrade one — check_tarball hashes the real bytes independently — so this is not a trust hole. But a mirror picks the exit code out of {9, 8, 7} at will, and exit 7's documented meaning (exits.py:39) is a false statement about the layer.

MEDIUM — `fetch_hash_file` reads without a bound

pull.py:359-360, fob.read() over urllib with no size cap. The tarball path is careful about exactly this (:326-331 streams in READ_CHUNK and breaks past layer.size); the .hash path is not.

MEDIUM — with no `--channel-signer`, any key in the keyring may move a channel

signature.py:172-173. Documented at docs/layers.md:286, and measured: keyring holding both keys, no --channel-signer, pointer signed by the second key → accepted, exit 0. Decision 0016's whole argument for putting an online key in this role is that it is a strictly weaker role than the release key. That separation is opt-in, and nothing in this repo, in debian/, or in any hook opts in.

On `keel verify`

It does not make an unearned claim. The status word is unverified, the strings are "signature present, not verified" and "hash file present, not signed", the exit constant is SIGNATURE_UNVERIFIED, and tests/test_layers_verify.py:153 pins the wording (rewording it to "signature verified" turns two tests red). Shipping channel verification alongside it is fine.

What is not fine to leave is the reason it gives. verify.py:46-48 says "(no trusted key configured)" and hashfile.py:7-8 says "checking the signature needs a trusted key, which the project does not have yet". This commit configures a trusted key. Both sentences become false in the same change that makes them false, and an appliance can now print "no trusted key configured" seconds after verifying a signature with one. Fix the two strings here.

The deeper version is a follow-up, not a blocker, but worth naming: add_channel_options was factored out at cli.py:270-271 as "what a channel pointer is verified against, wherever one is read", and then given to pull and inspect and not to verify (cli.py:164-170). The one command whose job is to say whether what is installed can be trusted is the one command with no keyring.

LOW

signature.py:55-84 dearmor. On the question of whether a crafted armored blob can produce a trusted keyring: no, and the reason is reachability rather than the parser — the input is only ever --channel-keyring / $KEEL_CHANNEL_KEYRING, a local path, never anything fetched. Worth saying so in the docstring. Two parser edges while here: data.split(ARMOR_START)[1:] silently discards everything before the first armor header, so a keyring holding binary keys followed by an armored block loses the binary ones; and a file armored under any other header is passed through raw and yields NO_PUBKEY, the same message as a wrong key — which is the second gpgv trap recurring in the code written to avoid it.
verify.py:213 returns SIGNATURE_ABSENT ("hash file present, not signed") when the hash file exists but could not be read, asserting a signature state for bytes nobody saw. Masked today because problems is non-empty and the note is discarded; a latent false claim.
tests/test_layers_channel_pull.py:110, test_one_blob_serves_two_releases_and_is_fetched_once: both pulls resolve the same directory 2026-09-29/1, so it restates the test at line 130 and the len(blobs)==2 assertion is a property of the fixture. The property does hold (checked by pulling the other release: ['cached','cached'], 0 bytes). One-word fix: make the second pull name 2026-09-28.
tests/test_layers_channel_pull.py:294 survives deletion of the parent_sha256 check — the flat tarball fallback makes the download digest check produce the same exit 7. The check is covered by test_layers_pull.py; assert the parent_sha256 substring here.

Verified as claimed

971 tests, 100.00% line and 100.00% branch, 0 skipped — reproduced exactly (Miss=0, BrPart=0, not a blended number).
gpgv writes --output for a signature it refused — reproduced for a tampered body (exit 1, BADSIG, full plaintext written) and for an untrusted key (exit 2, NO_PUBKEY, full plaintext written). The module reads the output only after the status is accepted, and raises rather than returning text. Correct, and the right call.
gpgv reads a binary keyring only — reproduced: armored keyring gives NO_PUBKEY and exit 2, with a stderr last line identical to the wrong-key case.
gpgv is not on a Debian 13 appliance — I could not reach an appliance (ssh to the build host is not permitted from here), so this is from the archive rather than from hardware: in trixie apt Depends: sqv and not gpgv, and gpgv is Priority: optional in the trixie Packages index. keel-wordpress/COVERAGE.md:158-161 records the project measuring the same thing. The other half also holds: gpgv is in trixie main, so the dependency installs. One thing to check before merge that I could not: the same COVERAGE note records a CI runner with no IPv4 route reaching only the Keel archive and nothing of Debian's, which is where a new Debian dependency on keel would first bite.
A pointer naming no layer is refused — channel.py:223-232, exit 17, "the pointer must name at least one layer". Measured.
Exit codes, all measured: wrong signer pinned 18, tampered after signing 18, unsigned pointer 18, missing keyring 18, pointer naming another channel 17, expired 19, rollback 20, manifest digest mismatch 7. Each message names what actually failed.
refresh cannot change the revision — verified by reading apt/lib/channel.sh:227-249: release, rev and the layer lines all come out of channel_verified's output, never out of the staged tree. Only set moves a channel. Good.
The local a="$1" b="$dir/$a" shape — swept every shell file in every Keel repository and worktree with a detector validated against the known-bad shapes: zero remaining occurrences. Clean.
The make_test_key shape — apt/tests/helpers.bash:55-78 now looks both keys up by user id. Correct. Small hardening: gpg --list-keys <uid> matches substrings, so "=Keel Test Key <test@example.invalid>" is the exact-match form if a third key is ever added. No other position-selected-where-identity-matters helper found in the repositories.

Block. The CRITICAL is one added status-line check away from being closed, and the HIGH on --release/--rev is the same digest plumbing that already exists on the channel path. Everything else here is good work — the digest chain, the refusal ordering in fetch_channel, the reading of --output rather than the file, the exit-code taxonomy, and a mutation-resistant test suite — which is why the two holes are worth closing before this becomes the thing every appliance trusts.

Reviewed as security code. Everything marked **measured** was run here on Debian 13.7 / gpgv 2.4.7 against this branch; everything marked **read** is from the diff. --- ## CRITICAL — a revoked or expired signing key verifies `keel/layers/signature.py:141-153` treats any zero exit from `gpgv` as success, and `:156-166` accepts any `VALIDSIG` line. Neither is sufficient. **Measured**, gpgv 2.4.7: | signature by | exit | status lines | | --- | --- | --- | | good key | 0 | `GOODSIG`, `VALIDSIG` | | **revoked primary** | **0** | `REVKEYSIG`, `VALIDSIG` | | **revoked signing subkey** under a live primary | **0** | `REVKEYSIG`, `VALIDSIG` | | **expired key** | **0** | `KEYEXPIRED`, `EXPKEYSIG`, `VALIDSIG` | stderr says `Good signature from` in all four. End to end against this branch, not at the gpgv level — pointer signed by a key that is revoked *in the very keyring passed to `--channel-keyring`*, with `--channel-signer` pinned to that fingerprint: ``` $ keel pull core --source $M/layers --cache-dir $C \ --channel stable --channel-keyring keyring-revoked.gpg \ --channel-signer 9EDC363B5B926536424A9DC13F6006762A3A31E2 --channel-state $S channel stable: release 2026-09-28/1, signed 2026-09-28T07:36:53Z, expires 2026-10-05T07:36:53Z core: fetched (21 bytes) layers: 1 resolved, 1 fetched, 0 cached, 21 bytes transferred EXIT=0 ``` Same result for an expired signing key (`EXIT=0`, layer fetched, `/var/lib/keel/channel` written). Note the subkey row. `VALIDSIG`'s last field is the **primary** fingerprint, and `_check_signers` (`signature.py:169-180`) intersects against both, so pinning `--channel-signer <primary>` matches a signature made by a revoked subkey of that primary. That is the exact shape of `apt/keys/keel-archive-keyring.asc`, which already carries `sub D276B62C2BD16F4E ... r` — a signing subkey this project has already had to revoke once. **Where it bites.** The channel key is, by design, the unattended key on the build host — the most likely key in the project to be stolen. The one response to that is to revoke it and ship the revocation in `keel-channel-keyring`. With this code that does nothing: every appliance keeps accepting the thief's pointers. Worse, it keeps accepting them until a new keyring reaches every appliance, and preventing a new keyring from reaching appliances is exactly what the stolen key can now do — pin them to a stale revision with a long expiry (see the MEDIUM on TTL below). **Fix**, one line each: require a `GOODSIG` line and not only `VALIDSIG` — gpgv emits `GOODSIG` only when the key is neither revoked nor expired, and replaces it with `REVKEYSIG`/`EXPKEYSIG` otherwise — or refuse outright when `REVKEYSIG|EXPKEYSIG|KEYREVOKED|KEYEXPIRED` appears on the status fd. The same applies to `apt/lib/channel.sh:170`. Related, same call site: `VERIFIER = ("gpgv",)` at `constants.py:79` passes no `--weak-digest SHA1`. A SHA-1 clearsigned pointer verifies today (**measured**: exit 0, `GOODSIG`, `VALIDSIG`). --- ## HIGH — `--release/--rev` verifies no signature at all, and the output does not say so `keel/layers/pull.py:471-476`. The by-release branch builds `Places(release, rev)` with `digests=None`, so `Places.digest_problem` (`:198-201`) returns `None` for every layer, and no pointer is fetched. The chain the whole PR is about — signature → manifest digest → blob digest — is simply absent on this path. **Measured** against a hostile mirror that serves no `layers/stable` at all and has had `2026-09-28/1/core.manifest` rewritten to point at an attacker blob: ``` $ keel pull core --source $M/layers --cache-dir $C --release 2026-09-28 --rev 1 release 2026-09-28/1, named on the command line core: fetched (27 bytes) layers: 1 resolved, 1 fetched, 0 cached, 27 bytes transferred EXIT=0 $ cat $C/core-*.tar.zst ATTACKER CONTROLLED ROOTFS ``` `docs/layers.md` calls this "how a rollback is asked for". Rollback is the operation an operator performs *after* something has gone wrong, against the mirror they have the least reason to trust, and it is the path with no verification on it. `keel/layers/pull.py:176-177` is honest about it in a docstring; nothing the operator sees is. Passing `--channel stable` alongside `--release/--rev` does not help — `by_release` wins at `:100-110` and the pointer is never fetched. The publisher signs a release `MANIFEST` with the release key and now records `revision` inside it (Keel-Linux/apt#14). The client never fetches it. Either resolve `--release/--rev` through that signed MANIFEST, or at minimum make `resolution_line()` say the revision was not verified. --- ## HIGH — nothing ships the default keyring, so the feature is dead on a stock appliance `constants.py:24` defaults to `/usr/share/keyrings/keel-channel-keyring.gpg`. `debian/keel.install` ships `examples/instance.yaml` and the firstboot hook and nothing else; no repository in the project produces that path (`keel-transition` ships `keel-archive-keyring.gpg`, `apt/keys/` publishes `keel-staging-keyring.asc`), and `debian/control` gains `gpgv` but no keyring dependency. `keel pull --channel stable` on an installed appliance exits 18 on a missing file. It fails closed, which is right. But the feature ships unusable, and the one refusal an operator will actually see looks identical to an attack. --- ## HIGH — the coverage gate is green when no verifier is present `.github/workflows/tests.yml:20-23` reasons correctly that a skipped suite would pass the gate on code nothing ran, and then relies entirely on the `apt-packages:` line to prevent it. **Measured**: re-running the exact CI command with `gpg` and `gpgv` off `PATH` gives `897 passed, 74 skipped`, `TOTAL … 97%`, and `coverage report --fail-under=95` **exits 0**. `keel/layers/signature.py` is at **15%** on that run — the entire verifier unrun — and `pull.py` at 76%. The project already has the right guard one file away: `tests/test_hook_bats.py:32-36` fails rather than skips when `CI` is set. `tests/channel_helpers.py:44-49` has no equivalent. ## HIGH — `--channel-signer` has no end-to-end test `grep -rn "channel.signer\|channel_signers" tests/` returns nothing. Mutation run: replacing the signers argument with `()` at **`keel/commands.py:383`** (pull) and **`keel/commands.py:259`** (`inspect --check-channel`) leaves all 971 tests passing at 100% line and branch. The flag works today (**measured**: exit 18, "which is not the key that may move a channel"); the net under it does not exist. It is also the clearest demonstration that 100/100 is not the property being claimed for this suite. *(Credit where due: 17 of 19 other mutants died. Removing the signer allowlist, the expiry refusal, the manifest-digest check, the whole signature step, the rollback check, the blob digest check, the channel-name binding, the no-keyring refusal, `expired()`'s `>=`, and the `SIGNATURE_PRESENT` wording each turned tests red. The adversarial cases assert behaviour: the two keys really are two distinct keys, the tampering is inside an intact clearsign envelope so verification and not parsing is what refuses, and the unsigned case uses a body that would otherwise validate. This is a good suite.)* --- ## MEDIUM — no ceiling on `expires_at`, in either direction `channel.py:206-220` requires only `expires_at > signed_at`. **Measured**, both accepted, exit 0: - signed now, `expires_at 2126-01-01T00:00:00Z` - `signed_at 2076-01-01T00:00:00Z` (fifty years in the future) This is the attack on decision 0016's argument. The note says the online key "can only say which of those is current, **and until when**. That is all." Unbounded, "until when" is a permanent freeze primitive: one signature pins an appliance to a known-vulnerable *already-staged* revision indefinitely, and `is_rollback` never fires because the revision does not go backwards. The attacker does not even need to keep re-signing, which is the weaker version the note concedes. The publisher takes `KEEL_CHANNEL_TTL_DAYS` unvalidated (`apt/lib/channel.sh:64,198`), so nothing bounds it at either end — and "lengthening the expiry until it means nothing" is the exact failure mode the note says the whole design exists to prevent. A client-side maximum (`expires_at - signed_at <= N days`, and `signed_at <= now + skew`) turns "forever" into "N days" and costs two comparisons. ## MEDIUM — clock skew is invisible and misdiagnosed `channel.py:85-97`. Clock ahead: every pointer is expired, exit 19, and the message is *"the mirror is not being updated, or is holding this instance back"* — it never prints `now` and never mentions the local clock. A fleet-wide NTP failure sends the operator to the mirror. Stalled rather than bricked, since the flat layout and `--release/--rev` still work, but the diagnosis is actively wrong. Clock behind: dead pointers still look fresh, and the freeze attack the expiry exists to stop succeeds. An LXC container restored from a snapshot, or one on a host with a bad RTC, is exactly that case. Print `now` in `staleness()`, and refuse a `signed_at` in the future. ## MEDIUM — a flat pull is indistinguishable from a verified one in the output `PullReport.resolution_line()` (`pull.py:155-166`) returns `None` for the flat layout, so a pull that verified nothing prints the same layer lines and the same summary as one that checked a signature. `--channel` has `choices=`, so a typo is caught — but forgetting it entirely is not. One line saying the flat layout follows no pointer would close it. ## MEDIUM — an unauthenticated `.hash` file decides `keel verify`'s exit code `pull.py:348-366` writes whatever the mirror serves at `<tarball>.hash` into the cache with no validation; `verify.py:196-226` then reads it. **Measured**, four mirrors differing only in that file, byte-perfect tarball and a validly signed pointer in every case: no `.hash` → exit 9 and `ok`; truthful unsigned → exit 8; **`.hash` lying about the sha256 → exit 7 `LAYER_MISMATCH`** on a layer that matches its signed manifest exactly; two PGP marker strings and no signature → exit 8 "signature present". It can only downgrade a verdict, never upgrade one — `check_tarball` hashes the real bytes independently — so this is not a trust hole. But a mirror picks the exit code out of {9, 8, 7} at will, and exit 7's documented meaning (`exits.py:39`) is a false statement about the layer. ## MEDIUM — `fetch_hash_file` reads without a bound `pull.py:359-360`, `fob.read()` over `urllib` with no size cap. The tarball path is careful about exactly this (`:326-331` streams in `READ_CHUNK` and breaks past `layer.size`); the `.hash` path is not. ## MEDIUM — with no `--channel-signer`, any key in the keyring may move a channel `signature.py:172-173`. Documented at `docs/layers.md:286`, and **measured**: keyring holding both keys, no `--channel-signer`, pointer signed by the second key → accepted, exit 0. Decision 0016's whole argument for putting an online key in this role is that it is a strictly weaker role than the release key. That separation is opt-in, and nothing in this repo, in `debian/`, or in any hook opts in. --- ## On `keel verify` It does **not** make an unearned claim. The status word is `unverified`, the strings are "signature present, **not verified**" and "hash file present, not signed", the exit constant is `SIGNATURE_UNVERIFIED`, and `tests/test_layers_verify.py:153` pins the wording (rewording it to "signature verified" turns two tests red). Shipping channel verification alongside it is fine. What is not fine to leave is the *reason* it gives. `verify.py:46-48` says "(no trusted key configured)" and `hashfile.py:7-8` says "checking the signature needs a trusted key, which the project does not have yet". This commit configures a trusted key. Both sentences become false in the same change that makes them false, and an appliance can now print "no trusted key configured" seconds after verifying a signature with one. Fix the two strings here. The deeper version is a follow-up, not a blocker, but worth naming: `add_channel_options` was factored out at `cli.py:270-271` as "what a channel pointer is verified against, **wherever one is read**", and then given to `pull` and `inspect` and not to `verify` (`cli.py:164-170`). The one command whose job is to say whether what is installed can be trusted is the one command with no keyring. --- ## LOW - `signature.py:55-84` `dearmor`. On the question of whether a crafted armored blob can produce a trusted keyring: **no**, and the reason is reachability rather than the parser — the input is only ever `--channel-keyring` / `$KEEL_CHANNEL_KEYRING`, a local path, never anything fetched. Worth saying so in the docstring. Two parser edges while here: `data.split(ARMOR_START)[1:]` silently discards everything before the first armor header, so a keyring holding binary keys followed by an armored block loses the binary ones; and a file armored under any other header is passed through raw and yields `NO_PUBKEY`, the same message as a wrong key — which is the second gpgv trap recurring in the code written to avoid it. - `verify.py:213` returns `SIGNATURE_ABSENT` ("hash file present, not signed") when the hash file exists but could not be read, asserting a signature state for bytes nobody saw. Masked today because `problems` is non-empty and the note is discarded; a latent false claim. - `tests/test_layers_channel_pull.py:110`, `test_one_blob_serves_two_releases_and_is_fetched_once`: both pulls resolve the same directory `2026-09-29/1`, so it restates the test at line 130 and the `len(blobs)==2` assertion is a property of the fixture. The property does hold (checked by pulling the other release: `['cached','cached']`, 0 bytes). One-word fix: make the second pull name `2026-09-28`. - `tests/test_layers_channel_pull.py:294` survives deletion of the `parent_sha256` check — the flat tarball fallback makes the download digest check produce the same exit 7. The check is covered by `test_layers_pull.py`; assert the `parent_sha256` substring here. --- ## Verified as claimed - **971 tests, 100.00% line and 100.00% branch, 0 skipped** — reproduced exactly (`Miss=0`, `BrPart=0`, not a blended number). - **gpgv writes `--output` for a signature it refused** — reproduced for a tampered body (exit 1, `BADSIG`, full plaintext written) and for an untrusted key (exit 2, `NO_PUBKEY`, full plaintext written). The module reads the output only after the status is accepted, and raises rather than returning text. Correct, and the right call. - **gpgv reads a binary keyring only** — reproduced: armored keyring gives `NO_PUBKEY` and exit 2, with a stderr last line identical to the wrong-key case. - **gpgv is not on a Debian 13 appliance** — I could not reach an appliance (ssh to the build host is not permitted from here), so this is from the archive rather than from hardware: in trixie `apt` `Depends: sqv` and not gpgv, and `gpgv` is `Priority: optional` in the trixie `Packages` index. `keel-wordpress/COVERAGE.md:158-161` records the project measuring the same thing. The other half also holds: `gpgv` is in trixie main, so the dependency installs. One thing to check before merge that I could not: the same COVERAGE note records a CI runner with no IPv4 route reaching only the Keel archive and nothing of Debian's, which is where a new Debian dependency on `keel` would first bite. - **A pointer naming no layer is refused** — `channel.py:223-232`, exit 17, `"the pointer must name at least one layer"`. **Measured.** - Exit codes, all **measured**: wrong signer pinned 18, tampered after signing 18, unsigned pointer 18, missing keyring 18, pointer naming another channel 17, expired 19, rollback 20, manifest digest mismatch 7. Each message names what actually failed. - **`refresh` cannot change the revision** — verified by reading `apt/lib/channel.sh:227-249`: release, rev and the layer lines all come out of `channel_verified`'s output, never out of the staged tree. Only `set` moves a channel. Good. - **The `local a="$1" b="$dir/$a"` shape** — swept every shell file in every Keel repository and worktree with a detector validated against the known-bad shapes: **zero remaining occurrences**. Clean. - **The `make_test_key` shape** — `apt/tests/helpers.bash:55-78` now looks both keys up by user id. Correct. Small hardening: `gpg --list-keys <uid>` matches substrings, so `"=Keel Test Key <test@example.invalid>"` is the exact-match form if a third key is ever added. No other position-selected-where-identity-matters helper found in the repositories. --- **Block.** The CRITICAL is one added status-line check away from being closed, and the HIGH on `--release/--rev` is the same digest plumbing that already exists on the channel path. Everything else here is good work — the digest chain, the refusal ordering in `fetch_channel`, the reading of `--output` rather than the file, the exit-code taxonomy, and a mutation-resistant test suite — which is why the two holes are worth closing before this becomes the thing every appliance trusts.

marcos-mendez commented

2026-09-29 00:54:00 -03:00

(Migrated from github.com)

Verified every finding of the review as fixed on 94d5792 (mutating the GOODSIG check turns 9 tests red), and checked end to end against apt#14's publisher. 28c674b only aligns the rollback docs with it: the revision pointer is the first one signed, written once, so --release/--rev works for a revision a channel has named.

Verified every finding of the review as fixed on 94d5792 (mutating the GOODSIG check turns 9 tests red), and checked end to end against apt#14's publisher. 28c674b only aligns the rollback docs with it: the revision pointer is the first one signed, written once, so `--release/--rev` works for a revision a channel has named.