Welcome the operator once, as Keel #9

Open

marcos-mendez wants to merge 6 commits from fix/welcome-once-as-keel into master

marcos-mendez commented

2026-09-28 00:26:58 -03:00

(Migrated from github.com)

Closes #7. Org-wide context and the two decisions it forced:
Keel-Linux/tracker#6 and handbook decision 0014
(Keel-Linux/handbook#6).

Measured on the wordpress-demo container, 2026-09-28, a login printed
the Keel mark, the appliance name and the addresses, and then:

Welcome to Wordpress-demo, TurnKey GNU/Linux 19.0 (Debian 13/Trixie)
...
TKLBAM (Backup and Migration):  NOT INITIALIZED
    To initialize TKLBAM, run the "tklbam-init" command to link this
    system to your TurnKey Hub account.

Two welcomes, the second naming a distribution this image is not, and an
instruction to register the machine with a service decision 0002
deferred and this project is decoupling from.

Where the fix had to live

Read out of the makefiles with make -n, not assumed. root.patched
applies the common overlays, runs the common conf scripts
(conf/turnkey.d/motd, which writes those drop-ins, is one of them),
applies the common patches and removelists, then the product units, then
the product overlay, and only then the product conf scripts.

So this repository's overlay lands after the files it has to replace,
and conf.d/main runs after the overlay. An overlay entry can add a
drop-in but cannot remove one, and removal is the point:
00-turnkey-sysinfo and 08-turnkey-confconsole go in conf.d/main,
which until now did nothing.

01-keel-sysinfo keeps every fact the old block gave (load, memory,
processes, swap, usage of /, an address per interface) from the same
command as before, and drops the backup client's block by the shape
of the output, not by the words that block uses today. The command
prints a header, a blank line, the table, and any tail after a second
blank line; the rule is that second blank line, so whatever replaces
TKLBAM upstream is cut too. A filter written against the word TKLBAM
would have let the next version of this same mistake through in
silence.
In its place, one true line about backup: not configured, no backup
service yet, and where it will be configured when there is one.
Nothing that does not exist is advertised and no URL that answers 404
is printed.
08-keel-confconsole says what upstream's said, pointing at this
project's documentation of its own console. The command keeps the name
it has today. The tput emphasis is dropped: this appliance's console
surface is plain ASCII with no escape.
07-check-inithooks and 10-nonpersistent-mode are kept untouched.
Both were read first: one warns that the first boot hooks will run
again, the other that the system is non-persistent. Neither names a
product.
00-keel-banner reads /etc/keel_version and falls back to
/etc/turnkey_version. Its comment anticipated that file;
Keel-Linux/common#5 writes it.

Rendered from the real chain, replayed in fab's order on a scratch
directory:

Keel Linux core 19.0-trixie-amd64

IPv6 Web:  https://[2804:710:d0:5::a6e]
IPv6 SSH:  root@2804:710:d0:5::a6e

  System information for Mon Sep 28 02:20:44 2026 (UTC+0000)

    System load:  0.00               Memory usage:  54.8%
    Processes:    39                 Swap usage:    6.3%
    Usage of /:   88.2% of 58.76GB   IP address for eth0: 10.88.5.69

  Backup:  not configured. Keel has no backup service yet; when one
           arrives it is configured from confconsole.

    For advanced configuration run:  confconsole

  For more info see: https://github.com/keel-linux/confconsole

The assertion that closes this is behavioural

Step 8 of the boot test renders /etc/update-motd.d in the running
container with run-parts, which is what pam_motd does at an
interactive login, and requires exactly one welcome, that it names Keel,
that the system information block still carries every field, and that
neither turnkey nor tklbam appears anywhere in it. It reads the
login, never a file (docs/traps.md, "Asserting the configuration is
not asserting the behaviour"). Run against the login of a live appliance
today it fails and names both faults; run against the chain this branch
produces it passes.

The login check applies only to a layer built with this change. The
appliance check (appliance / boot-published-layer since
keel-linux/.github#12) boots the layer the mirror serves, whose
product_commit is 53d9fb5, older than this branch. As first written,
the login verdict failed on it every time; since the check is required
and strict on master and the layer can only be rebuilt from master after
a merge, nothing could ever have turned it green. The boot test now asks
the login check only of a layer carrying /usr/lib/keel/motd.sh, which
conf.d/main refuses to build without: on an older layer it reports that
it did not apply and names the remedy (rebuild and publish), without
counting as a pass; on a layer that carries the change a wrong login
still fails. The first core layer published after the merge is where the
login is checked for real.

Tests

100 percent under kcov 43, threshold unchanged at 100:

File	Coverage
`overlay/usr/lib/keel/motd.sh`	58 of 58 lines, 39 bats
`overlay/usr/lib/keel/banner.sh`	124 of 124 lines, 71 bats
`tests/lib/boot-test-lib.sh`	167 of 167 lines, 48 bats
`conf.d/main`	8 of 8 lines

158 tests in all. conf.d/main is measured by running the conf script
itself against a scratch drop-in directory, so both ways it fails the
build are covered: the library missing from the chroot, and a Keel
drop-in absent because the overlay did not land.

Test plan

bats tests/boot-test.bats tests/banner.bats tests/motd.bats
COVERAGE_THRESHOLD=100 tests/coverage.sh
shellcheck -S warning clean on every changed shell file
the build order read from product.mk and mk/turnkey.mk with
make -n, and the whole chain replayed on a scratch directory:
the common conf script's own text, then this overlay, then
conf.d/main
the boot test's verdict run against the login of a live appliance
(fails, names both faults) and against the chain this branch
produces (passes)
appliance / boot-published-layer green on the published layer,
with the login check reported as not applicable to it
the login check passing on the first core layer rebuilt and
published after the merge

Closes #7. Org-wide context and the two decisions it forced: Keel-Linux/tracker#6 and handbook decision 0014 (Keel-Linux/handbook#6). Measured on the `wordpress-demo` container, 2026-09-28, a login printed the Keel mark, the appliance name and the addresses, and then: Welcome to Wordpress-demo, TurnKey GNU/Linux 19.0 (Debian 13/Trixie) ... TKLBAM (Backup and Migration): NOT INITIALIZED To initialize TKLBAM, run the "tklbam-init" command to link this system to your TurnKey Hub account. Two welcomes, the second naming a distribution this image is not, and an instruction to register the machine with a service decision 0002 deferred and this project is decoupling from. ## Where the fix had to live Read out of the makefiles with `make -n`, not assumed. `root.patched` applies the common overlays, runs the common conf scripts (`conf/turnkey.d/motd`, which writes those drop-ins, is one of them), applies the common patches and removelists, then the product units, then **the product overlay**, and only then **the product conf scripts**. So this repository's overlay lands *after* the files it has to replace, and `conf.d/main` runs after the overlay. An overlay entry can add a drop-in but cannot remove one, and removal is the point: `00-turnkey-sysinfo` and `08-turnkey-confconsole` go in `conf.d/main`, which until now did nothing. ## What the login prints instead - `01-keel-sysinfo` keeps every fact the old block gave (load, memory, processes, swap, usage of `/`, an address per interface) from the same command as before, and drops the backup client's block **by the shape of the output**, not by the words that block uses today. The command prints a header, a blank line, the table, and any tail after a second blank line; the rule is that second blank line, so whatever replaces TKLBAM upstream is cut too. A filter written against the word TKLBAM would have let the next version of this same mistake through in silence. - In its place, one true line about backup: not configured, no backup service yet, and where it will be configured when there is one. Nothing that does not exist is advertised and no URL that answers 404 is printed. - `08-keel-confconsole` says what upstream's said, pointing at this project's documentation of its own console. The command keeps the name it has today. The `tput` emphasis is dropped: this appliance's console surface is plain ASCII with no escape. - `07-check-inithooks` and `10-nonpersistent-mode` are kept untouched. Both were read first: one warns that the first boot hooks will run again, the other that the system is non-persistent. Neither names a product. - `00-keel-banner` reads `/etc/keel_version` and falls back to `/etc/turnkey_version`. Its comment anticipated that file; Keel-Linux/common#5 writes it. Rendered from the real chain, replayed in fab's order on a scratch directory: ``` Keel Linux core 19.0-trixie-amd64 IPv6 Web: https://[2804:710:d0:5::a6e] IPv6 SSH: root@2804:710:d0:5::a6e System information for Mon Sep 28 02:20:44 2026 (UTC+0000) System load: 0.00 Memory usage: 54.8% Processes: 39 Swap usage: 6.3% Usage of /: 88.2% of 58.76GB IP address for eth0: 10.88.5.69 Backup: not configured. Keel has no backup service yet; when one arrives it is configured from confconsole. For advanced configuration run: confconsole For more info see: https://github.com/keel-linux/confconsole ``` ## The assertion that closes this is behavioural Step 8 of the boot test renders `/etc/update-motd.d` in the running container with `run-parts`, which is what pam_motd does at an interactive login, and requires exactly one welcome, that it names Keel, that the system information block still carries every field, and that neither `turnkey` nor `tklbam` appears anywhere in it. It reads the login, never a file (`docs/traps.md`, "Asserting the configuration is not asserting the behaviour"). Run against the login of a live appliance today it fails and names both faults; run against the chain this branch produces it passes. **The login check applies only to a layer built with this change.** The appliance check (`appliance / boot-published-layer` since keel-linux/.github#12) boots the layer the mirror serves, whose `product_commit` is `53d9fb5`, older than this branch. As first written, the login verdict failed on it every time; since the check is required and strict on master and the layer can only be rebuilt from master after a merge, nothing could ever have turned it green. The boot test now asks the login check only of a layer carrying `/usr/lib/keel/motd.sh`, which `conf.d/main` refuses to build without: on an older layer it reports that it did not apply and names the remedy (rebuild and publish), without counting as a pass; on a layer that carries the change a wrong login still fails. The first core layer published after the merge is where the login is checked for real. ## Tests 100 percent under kcov 43, threshold unchanged at 100: | File | Coverage | | --- | --- | | `overlay/usr/lib/keel/motd.sh` | 58 of 58 lines, 39 bats | | `overlay/usr/lib/keel/banner.sh` | 124 of 124 lines, 71 bats | | `tests/lib/boot-test-lib.sh` | 167 of 167 lines, 48 bats | | `conf.d/main` | 8 of 8 lines | 158 tests in all. `conf.d/main` is measured by running the conf script itself against a scratch drop-in directory, so both ways it fails the build are covered: the library missing from the chroot, and a Keel drop-in absent because the overlay did not land. ## Test plan - [x] `bats tests/boot-test.bats tests/banner.bats tests/motd.bats` - [x] `COVERAGE_THRESHOLD=100 tests/coverage.sh` - [x] `shellcheck -S warning` clean on every changed shell file - [x] the build order read from `product.mk` and `mk/turnkey.mk` with `make -n`, and the whole chain replayed on a scratch directory: the common conf script's own text, then this overlay, then `conf.d/main` - [x] the boot test's verdict run against the login of a live appliance (fails, names both faults) and against the chain this branch produces (passes) - [x] `appliance / boot-published-layer` green on the published layer, with the login check reported as not applicable to it - [ ] the login check passing on the first core layer rebuilt and published after the merge

marcos-mendez commented

2026-09-28 00:30:09 -03:00

(Migrated from github.com)

Checks as expected.

tests / coverage green: motd.sh 58/58, banner.sh 124/124, boot-test-lib.sh 167/167, conf.d/main 8/8, all 100 percent, 158 bats tests.

appliance / build-and-boot red, at the new step and for the stated reason. It assembled, booted and finished the first boot, then rendered the login of the layer the mirror serves (product_commit 53d9fb5, older than this branch) and said:

motd: 2 welcomes, there must be one
  Welcome to Core, TurnKey GNU/Linux 19.0 (Debian 13/Trixie)
  Keel Linux core 19.0-trixie-amd64
motd: the login still says:
  turnkey
  tklbam
motd: this login is the one issue #6 describes. If the code for it is in
this repository, the layer on the mirror was built before it: rebuild and
publish the layer, then run this test again.

That is the assertion working on a real appliance. It goes green when the core layer is rebuilt from this branch and published; I have not built or published anything.

Checks as expected. `tests / coverage` green: `motd.sh` 58/58, `banner.sh` 124/124, `boot-test-lib.sh` 167/167, `conf.d/main` 8/8, all 100 percent, 158 bats tests. `appliance / build-and-boot` red, at the new step and for the stated reason. It assembled, booted and finished the first boot, then rendered the login of the layer the mirror serves (`product_commit 53d9fb5`, older than this branch) and said: motd: 2 welcomes, there must be one Welcome to Core, TurnKey GNU/Linux 19.0 (Debian 13/Trixie) Keel Linux core 19.0-trixie-amd64 motd: the login still says: turnkey tklbam motd: this login is the one issue #6 describes. If the code for it is in this repository, the layer on the mirror was built before it: rebuild and publish the layer, then run this test again. That is the assertion working on a real appliance. It goes green when the core layer is rebuilt from this branch and published; I have not built or published anything.

marcos-mendez commented

2026-09-28 01:34:55 -03:00

(Migrated from github.com)

Review of the change set as one piece (this, Keel-Linux/common#5, handbook #6 and #4). They are not separable in the direction that matters: keel-core#9 reads /etc/keel_version and common#5 is what writes it. The fallback in keel_banner_version_string means this one is safe to merge alone; common#5 is safe alone too, and merging common#5 first is the ordering with no window in which anything is wrong.

What I verified by running rather than reading, so the findings below can be weighed against it:

The chain, replayed rather than trusted. I ran common's conf/turnkey.d/motd into a scratch tree, applied this branch's overlay over it, ran conf.d/main, and rendered the directory with run-parts, feeding turnkey-sysinfo output captured from wordpress-demo on the build host. The result is one welcome (Keel Linux core 19.0-trixie-amd64), all six facts, and no turnkey and no tklbam. bt_motd_verdict on it exits 0. The fixture in tests/boot-test.bats is a fair rendering of what the branch produces; its 10-uname line is abbreviated and its blank lines inside the sysinfo block lack the two spaces the real indent adds, neither of which changes a verdict.
Claim 3 holds for the real shape. turnkey-sysinfo on a live appliance prints the header first (first bytes Sys, blank lines at 2, 6, 8, 12, 14) and the cut at the second blank line drops the tail without matching a word of it. See LOW 7 for the one shape it does not survive.
Claim 4 holds. 158 tests pass here; COVERAGE_THRESHOLD=100 tests/coverage.sh gives boot-test-lib.sh 167/167, banner.sh 124/124, motd.sh 58/58, conf.d/main 8/8, all 100.00. conf.d/main really is measured by executing the script against a scratch drop-in directory, and the assertions are behavioural rather than configuration reads: the drop-in tests run the script and inspect the directory it left, the block tests feed captured output and compare text. shellcheck -S warning clean on every changed shell file.
Claim 5 holds, for that reason and no other. In run 36373765079 the assemble, verify, boot and first boot steps all pass; the job fails at the motd step with motd: 2 welcomes, there must be one over Welcome to Core, TurnKey GNU/Linux 19.0 (Debian 13/Trixie), plus turnkey and tklbam. Nothing was weakened to compensate: git diff origin/master...HEAD removes only prose, the old no-op conf.d/main, and the inlined version-file read that the library call replaces. PR #10 was a temporary workflow-ref change and is closed unmerged.
The fix does reach the other ten appliances. bt-layer subtracts the parent's common_conf from the child's, and every published manifest has parent=core transitively, so no child re-runs conf/turnkey.d/motd: the mariadb, wordpress, redis and postgresql layer tarballs on the build host carry no /etc/update-motd.d entries at all. Core's whiteouts are what every appliance inherits.
Wording. keellinux.org/docs/ is 404, measured, so printing no URL there is right. https://github.com/keel-linux/confconsole answers 200 and the repository is public. Naming confconsole promises a place and not a feature, and nothing contradicts it on arrival: confconsole computes a TKLBAM status but only logs it and never renders it, so an operator who goes there finds no backup surface rather than a TurnKey one.
No duplication. motd.sh and banner.sh share no logic; keel_banner_center_mark and keel_motd_indent both pad, for different reasons, and merging them would couple the login text to the mark renderer. The organization's lib/ in .github holds CI libraries checked out into .keel-ci/lib/; these two are runtime files that have to be in the image, so the overlay is their place. Putting the removal in the product rather than editing common's conf script keeps common offerable upstream, which is decision 0008's posture.

HIGH

1. The gate now requires an IPv4 address, on a distribution that is IPv6 first. tests/lib/boot-test-lib.sh, BT_MOTD_FIELDS=(... "IP address"). turnkey-sysinfo emits IP address for <nic> only for interfaces where netinfo.InterfaceInfo.address is set, and that property is SIOCGIFADDR on an AF_INET socket: IPv4 only. With no IPv4 on any interface it prints the single row Networking not configured and the string IP address never appears anywhere in the login. tests/instance.yaml declares an IPv6-only appliance. The gate passes today only because lxcbr0 also hands out IPv4 (run 36373765079 shows IP address for eth0: 10.0.3.112). The scenario: the CI bridge is made IPv6-only, or a real IPv6-only appliance is booted, and step 6 fails on a machine that is completely correct, naming a field the operator never lost, and the pressure will be to weaken the verdict. The banner above already prints IPv6 first, so nothing is hidden from the operator; it is the assertion that has the IPv4 assumption, and it belongs in this pull request. The IPv4-only sysinfo line itself is unchanged from upstream and costs the operator nothing here, so that part is an issue, not a blocker.

2. The new verdict runs before keel diff, so a deliberately red assertion hides the one that was green. tests/boot-test.sh:111-116. bt_motd_verdict "$motd" is a simple command under set -euo pipefail, so the script exits and the keel diff step never runs; the gate log confirms no diff line. The author's intent is that the job stay red until the layer is rebuilt, which is defensible, but for that whole interval the drift check, previously the only behavioural assertion in the job, reports nothing at all. Collect both verdicts and fail at the end, or order the motd verdict after the diff.

MEDIUM

3. The removal lives in one recipe, so a product built without a parent gets the drop-ins back and nothing says so. conf.d/main is the only place keel_motd_prune_dir and keel_motd_check_dir are ever called. Through bt-layer that is enough, as measured above. A plain make of any non-core recipe, the ISO path or an M0-style full build, runs conf/turnkey.d/motd with the full COMMON_CONF and has no prune, so that image welcomes twice again and keel_motd_check_dir is not there to catch it. Either the check belongs in every recipe's conf.d, or the arrangement needs to be asserted somewhere that is not one product's conf script.

4. The two login drop-ins take their library path from the environment, and the drop-ins run with the PAM stack's privileges. 01-keel-sysinfo:19 and 08-keel-confconsole:18: KEEL_MOTD_LIB="${KEEL_MOTD_LIB:-/usr/lib/keel/motd.sh}" followed by . "$KEEL_MOTD_LIB". No test needs it: tests/motd.bats sources overlay/usr/lib/keel/motd.sh directly and passes KEEL_MOTD_LIB only to conf.d/main. Nothing user-controlled reaches it today (AcceptEnv LANG LC_* COLORTERM NO_COLOR, no PermitUserEnvironment). The scenario is one configuration change away: an AcceptEnv KEEL_*, a pam_env line or a sudo -E wrapper, and a login sources a file the caller named. Same shape as the merged KEEL_BANNER_LIB, so pre-existing rather than introduced, but this triples it. Hardcode the path in the drop-ins and keep the override in conf.d/main, which runs in a chroot at build time and is where the test affordance is actually used.

5. The documented build order is wrong in one position, in three places that will be read as authoritative. conf.d/main:14-18, COVERAGE.md under "The login", and the changelog entry all say the common removelists come before the product units. Read from root.patched/body in /usr/share/fab/product.mk, the order is: common overlays, common conf scripts, common patches, unit overlays, unit conf scripts, unit removelists, common removelists, product overlay, product conf scripts, product patches, product removelist, initramfs, common removelists-final, then root.patched/post. Nothing in this change depends on that position: the two facts it rests on, that the common conf scripts run before the product overlay and that the product overlay lands before the product conf scripts, are both correct, and I confirmed conf.d/main is the only place the removal can live. But a recipe with units will be read against this text, and unit conf scripts run before the product overlay, not after it.

LOW

6. 01-keel-sysinfo:26 drops the whole system information block in silence when turnkey-sysinfo is absent. The boot test catches it; a machine that loses the package after the build does not.

7. keel_motd_system_block counts blank lines from the first line read, not from the first blank after a non-blank one. A single leading blank makes the blank after the header the second one, and the entire table is dropped. It does not bite today, and the drop-in discards stderr so nothing else can inject one, but anchoring the count costs one condition and makes the rule "the blank line that ends the table" literally true rather than true by luck.

8. turnkey-sysinfo still runs tklbam-status at every root login (os.geteuid() == 0); only its output is cut. The appliance keeps executing the backup client the login says it does not have.

9. tests/coverage.sh runs tests/motd.bats twice, once per measured target, doubling that suite's kcov time for no extra coverage.

Note. The rendered login has two consecutive blank lines between the backup block and the confconsole block, because 01-keel-sysinfo ends with echo and keel_motd_confconsole_lines opens with a newline. Upstream's spacing is the same, so this looks deliberate.

Warning

Nothing here is a correctness defect in what the operator sees: the login this branch produces is right, and I confirmed it by replaying the chain rather than by reading the fixture. HIGH 1 and HIGH 2 are both about the gate rather than the image, and both are the kind of thing that gets a verdict weakened six weeks from now by somebody who was not in this conversation. Worth fixing before merge; neither blocks.

Review of the change set as one piece (this, Keel-Linux/common#5, handbook #6 and #4). They are not separable in the direction that matters: keel-core#9 reads `/etc/keel_version` and common#5 is what writes it. The fallback in `keel_banner_version_string` means this one is safe to merge alone; common#5 is safe alone too, and merging common#5 first is the ordering with no window in which anything is wrong. What I verified by running rather than reading, so the findings below can be weighed against it: - **The chain, replayed rather than trusted.** I ran common's `conf/turnkey.d/motd` into a scratch tree, applied this branch's overlay over it, ran `conf.d/main`, and rendered the directory with `run-parts`, feeding `turnkey-sysinfo` output captured from `wordpress-demo` on the build host. The result is one welcome (`Keel Linux core 19.0-trixie-amd64`), all six facts, and no `turnkey` and no `tklbam`. `bt_motd_verdict` on it exits 0. The fixture in `tests/boot-test.bats` is a fair rendering of what the branch produces; its `10-uname` line is abbreviated and its blank lines inside the sysinfo block lack the two spaces the real indent adds, neither of which changes a verdict. - **Claim 3 holds for the real shape.** `turnkey-sysinfo` on a live appliance prints the header first (first bytes `Sys`, blank lines at 2, 6, 8, 12, 14) and the cut at the second blank line drops the tail without matching a word of it. See LOW 7 for the one shape it does not survive. - **Claim 4 holds.** 158 tests pass here; `COVERAGE_THRESHOLD=100 tests/coverage.sh` gives `boot-test-lib.sh` 167/167, `banner.sh` 124/124, `motd.sh` 58/58, `conf.d/main` 8/8, all 100.00. `conf.d/main` really is measured by executing the script against a scratch drop-in directory, and the assertions are behavioural rather than configuration reads: the drop-in tests run the script and inspect the directory it left, the block tests feed captured output and compare text. `shellcheck -S warning` clean on every changed shell file. - **Claim 5 holds, for that reason and no other.** In run 36373765079 the assemble, verify, boot and first boot steps all pass; the job fails at the motd step with `motd: 2 welcomes, there must be one` over `Welcome to Core, TurnKey GNU/Linux 19.0 (Debian 13/Trixie)`, plus `turnkey` and `tklbam`. Nothing was weakened to compensate: `git diff origin/master...HEAD` removes only prose, the old no-op `conf.d/main`, and the inlined version-file read that the library call replaces. PR #10 was a temporary workflow-ref change and is closed unmerged. - **The fix does reach the other ten appliances.** `bt-layer` subtracts the parent's `common_conf` from the child's, and every published manifest has `parent=core` transitively, so no child re-runs `conf/turnkey.d/motd`: the mariadb, wordpress, redis and postgresql layer tarballs on the build host carry no `/etc/update-motd.d` entries at all. Core's whiteouts are what every appliance inherits. - **Wording.** `keellinux.org/docs/` is 404, measured, so printing no URL there is right. `https://github.com/keel-linux/confconsole` answers 200 and the repository is public. Naming confconsole promises a place and not a feature, and nothing contradicts it on arrival: confconsole computes a TKLBAM status but only logs it and never renders it, so an operator who goes there finds no backup surface rather than a TurnKey one. - **No duplication.** `motd.sh` and `banner.sh` share no logic; `keel_banner_center_mark` and `keel_motd_indent` both pad, for different reasons, and merging them would couple the login text to the mark renderer. The organization's `lib/` in `.github` holds CI libraries checked out into `.keel-ci/lib/`; these two are runtime files that have to be in the image, so the overlay is their place. Putting the removal in the product rather than editing common's conf script keeps common offerable upstream, which is decision 0008's posture. ## HIGH **1. The gate now requires an IPv4 address, on a distribution that is IPv6 first.** `tests/lib/boot-test-lib.sh`, `BT_MOTD_FIELDS=(... "IP address")`. `turnkey-sysinfo` emits `IP address for <nic>` only for interfaces where `netinfo.InterfaceInfo.address` is set, and that property is `SIOCGIFADDR` on an `AF_INET` socket: IPv4 only. With no IPv4 on any interface it prints the single row `Networking not configured` and the string `IP address` never appears anywhere in the login. `tests/instance.yaml` declares an IPv6-only appliance. The gate passes today only because lxcbr0 also hands out IPv4 (run 36373765079 shows `IP address for eth0: 10.0.3.112`). The scenario: the CI bridge is made IPv6-only, or a real IPv6-only appliance is booted, and step 6 fails on a machine that is completely correct, naming a field the operator never lost, and the pressure will be to weaken the verdict. The banner above already prints IPv6 first, so nothing is hidden from the operator; it is the assertion that has the IPv4 assumption, and it belongs in this pull request. The IPv4-only sysinfo line itself is unchanged from upstream and costs the operator nothing here, so that part is an issue, not a blocker. **2. The new verdict runs before `keel diff`, so a deliberately red assertion hides the one that was green.** `tests/boot-test.sh:111-116`. `bt_motd_verdict "$motd"` is a simple command under `set -euo pipefail`, so the script exits and the `keel diff` step never runs; the gate log confirms no diff line. The author's intent is that the job stay red until the layer is rebuilt, which is defensible, but for that whole interval the drift check, previously the only behavioural assertion in the job, reports nothing at all. Collect both verdicts and fail at the end, or order the motd verdict after the diff. ## MEDIUM **3. The removal lives in one recipe, so a product built without a parent gets the drop-ins back and nothing says so.** `conf.d/main` is the only place `keel_motd_prune_dir` and `keel_motd_check_dir` are ever called. Through `bt-layer` that is enough, as measured above. A plain `make` of any non-core recipe, the ISO path or an M0-style full build, runs `conf/turnkey.d/motd` with the full `COMMON_CONF` and has no prune, so that image welcomes twice again and `keel_motd_check_dir` is not there to catch it. Either the check belongs in every recipe's `conf.d`, or the arrangement needs to be asserted somewhere that is not one product's conf script. **4. The two login drop-ins take their library path from the environment, and the drop-ins run with the PAM stack's privileges.** `01-keel-sysinfo:19` and `08-keel-confconsole:18`: `KEEL_MOTD_LIB="${KEEL_MOTD_LIB:-/usr/lib/keel/motd.sh}"` followed by `. "$KEEL_MOTD_LIB"`. No test needs it: `tests/motd.bats` sources `overlay/usr/lib/keel/motd.sh` directly and passes `KEEL_MOTD_LIB` only to `conf.d/main`. Nothing user-controlled reaches it today (`AcceptEnv LANG LC_* COLORTERM NO_COLOR`, no `PermitUserEnvironment`). The scenario is one configuration change away: an `AcceptEnv KEEL_*`, a `pam_env` line or a `sudo -E` wrapper, and a login sources a file the caller named. Same shape as the merged `KEEL_BANNER_LIB`, so pre-existing rather than introduced, but this triples it. Hardcode the path in the drop-ins and keep the override in `conf.d/main`, which runs in a chroot at build time and is where the test affordance is actually used. **5. The documented build order is wrong in one position, in three places that will be read as authoritative.** `conf.d/main:14-18`, `COVERAGE.md` under "The login", and the changelog entry all say the common removelists come before the product units. Read from `root.patched/body` in `/usr/share/fab/product.mk`, the order is: common overlays, common conf scripts, common patches, **unit overlays, unit conf scripts, unit removelists**, common removelists, product overlay, product conf scripts, product patches, product removelist, initramfs, common removelists-final, then `root.patched/post`. Nothing in this change depends on that position: the two facts it rests on, that the common conf scripts run before the product overlay and that the product overlay lands before the product conf scripts, are both correct, and I confirmed `conf.d/main` is the only place the removal can live. But a recipe with units will be read against this text, and unit conf scripts run before the product overlay, not after it. ## LOW **6.** `01-keel-sysinfo:26` drops the whole system information block in silence when `turnkey-sysinfo` is absent. The boot test catches it; a machine that loses the package after the build does not. **7.** `keel_motd_system_block` counts blank lines from the first line read, not from the first blank after a non-blank one. A single leading blank makes the blank after the header the second one, and the entire table is dropped. It does not bite today, and the drop-in discards stderr so nothing else can inject one, but anchoring the count costs one condition and makes the rule "the blank line that ends the table" literally true rather than true by luck. **8.** `turnkey-sysinfo` still runs `tklbam-status` at every root login (`os.geteuid() == 0`); only its output is cut. The appliance keeps executing the backup client the login says it does not have. **9.** `tests/coverage.sh` runs `tests/motd.bats` twice, once per measured target, doubling that suite's kcov time for no extra coverage. **Note.** The rendered login has two consecutive blank lines between the backup block and the confconsole block, because `01-keel-sysinfo` ends with `echo` and `keel_motd_confconsole_lines` opens with a newline. Upstream's spacing is the same, so this looks deliberate. ## Warning Nothing here is a correctness defect in what the operator sees: the login this branch produces is right, and I confirmed it by replaying the chain rather than by reading the fixture. HIGH 1 and HIGH 2 are both about the gate rather than the image, and both are the kind of thing that gets a verdict weakened six weeks from now by somebody who was not in this conversation. Worth fixing before merge; neither blocks.

marcos-mendez commented

2026-09-28 02:17:03 -03:00

(Migrated from github.com)

Both HIGH findings and all three MEDIUM are addressed, plus LOW 7, LOW 6 and LOW 9. Three commits on top of the reviewed one; nothing the operator sees changed.

HIGH 1, the IPv4 assumption. Correct, and it would have bitten the moment the bridge lost IPv4. Split in two: the five labels the command always prints stay required; the address row is checked by shape and accepts either IP address for or the Networking not configured the command prints with no IPv4. What the issue actually asks for — that the operator is told how to reach the machine — is now asserted against the machine: the boot test passes in every global IPv6 the container answers on and the login has to carry one of them. All of them rather than the first, because lxc-info lists them in its own order while keel_banner_pick_ipv6 takes static before dynamic and privacy last, so on a bridge handing out two the check and the banner could disagree and fail a correct machine — the same class of fault as the finding. A fixture of an IPv6-only login (Networking not configured, no IP address anywhere) holds it open.

HIGH 2, the short circuit. Both verdicts are collected with || rc=$? and reported together by bt_checks_verdict, which names every check that failed rather than stopping at the first, and treats no checks at all as a failure. Visible in the run on the current head:

motd: 2 welcomes, there must be one
...
keel diff: no drift, but a declared field could not be observed offline
boot-test: the login check failed (exit 1)

The drift check now reports while the login check waits for the layer.

MEDIUM 5, the build order. You are right and I had read the wrong file: a stock tkldev's /usr/share/fab/product.mk, not the build host's. Re-read from root.patched/body there and replayed with make -n against that file with a unit present. Corrected in conf.d/main, COVERAGE.md and the changelog entry, each of which now names which fab it was read from, and each of which calls out that unit conf scripts run before the product overlay.

MEDIUM 4, the sourced library path. Written out in all three login drop-ins, including 00-keel-banner, which had it before this branch. The comment says why: pam_motd runs them with the privileges of the PAM stack and a sourced path is executed, not read. conf.d/main keeps the override, since it runs in a chroot at build time and is the only place a test uses it. The data paths in the banner stay overridable, deliberately: changing one changes what is printed, not what runs, and they are what make the block renderable by hand.

MEDIUM 3, one recipe prunes. Not solvable inside this repository without either touching every recipe or editing common's conf script, which would cost the upstream-offerable posture you noted. Written down instead, in conf.d/main and COVERAGE.md: bt-layer subtracts the parent's common_conf, so nothing built on core re-runs the script and core's removals are inherited; a plain make of a non-core recipe writes the drop-ins again and needs the same two calls in its own conf.d. Worth an issue against the other recipes rather than a silent limit.

LOW 7. The count starts at the first line with something on it, so a leading blank costs nothing. LOW 6. A machine with no system information command now prints System information: not available on this machine rather than nothing; the command is not named, since printing today's name would put another product's back in the login. LOW 9. One kcov run per bats file, with a verdict per measured file read out of kcov's own per-file entries.

LOW 8 is left: turnkey-sysinfo still runs tklbam-status at every root login and only its output is cut. Stopping that means patching the package, which is outside this change; worth its own issue.

tests / coverage green on the head: boot-test-lib.sh 214/214, banner.sh 124/124, motd.sh 62/62, conf.d/main 8/8, all 100.00, 179 bats tests. appliance / build-and-boot still red for the one reason, and it now prints both verdicts.

Both HIGH findings and all three MEDIUM are addressed, plus LOW 7, LOW 6 and LOW 9. Three commits on top of the reviewed one; nothing the operator sees changed. **HIGH 1, the IPv4 assumption.** Correct, and it would have bitten the moment the bridge lost IPv4. Split in two: the five labels the command always prints stay required; the address row is checked by shape and accepts either `IP address for` or the `Networking not configured` the command prints with no IPv4. What the issue actually asks for — that the operator is told how to reach the machine — is now asserted against the machine: the boot test passes in **every** global IPv6 the container answers on and the login has to carry one of them. All of them rather than the first, because `lxc-info` lists them in its own order while `keel_banner_pick_ipv6` takes static before dynamic and privacy last, so on a bridge handing out two the check and the banner could disagree and fail a correct machine — the same class of fault as the finding. A fixture of an IPv6-only login (`Networking not configured`, no `IP address` anywhere) holds it open. **HIGH 2, the short circuit.** Both verdicts are collected with `|| rc=$?` and reported together by `bt_checks_verdict`, which names every check that failed rather than stopping at the first, and treats no checks at all as a failure. Visible in the run on the current head: motd: 2 welcomes, there must be one ... keel diff: no drift, but a declared field could not be observed offline boot-test: the login check failed (exit 1) The drift check now reports while the login check waits for the layer. **MEDIUM 5, the build order.** You are right and I had read the wrong file: a stock tkldev's `/usr/share/fab/product.mk`, not the build host's. Re-read from `root.patched/body` there and replayed with `make -n` against that file with a unit present. Corrected in `conf.d/main`, `COVERAGE.md` and the changelog entry, each of which now names which fab it was read from, and each of which calls out that unit conf scripts run *before* the product overlay. **MEDIUM 4, the sourced library path.** Written out in all three login drop-ins, including `00-keel-banner`, which had it before this branch. The comment says why: pam_motd runs them with the privileges of the PAM stack and a sourced path is executed, not read. `conf.d/main` keeps the override, since it runs in a chroot at build time and is the only place a test uses it. The data paths in the banner stay overridable, deliberately: changing one changes what is printed, not what runs, and they are what make the block renderable by hand. **MEDIUM 3, one recipe prunes.** Not solvable inside this repository without either touching every recipe or editing common's conf script, which would cost the upstream-offerable posture you noted. Written down instead, in `conf.d/main` and `COVERAGE.md`: `bt-layer` subtracts the parent's `common_conf`, so nothing built on core re-runs the script and core's removals are inherited; a plain `make` of a non-core recipe writes the drop-ins again and needs the same two calls in its own `conf.d`. Worth an issue against the other recipes rather than a silent limit. **LOW 7.** The count starts at the first line with something on it, so a leading blank costs nothing. **LOW 6.** A machine with no system information command now prints `System information: not available on this machine` rather than nothing; the command is not named, since printing today's name would put another product's back in the login. **LOW 9.** One kcov run per bats file, with a verdict per measured file read out of kcov's own per-file entries. **LOW 8** is left: `turnkey-sysinfo` still runs `tklbam-status` at every root login and only its output is cut. Stopping that means patching the package, which is outside this change; worth its own issue. `tests / coverage` green on the head: `boot-test-lib.sh` 214/214, `banner.sh` 124/124, `motd.sh` 62/62, `conf.d/main` 8/8, all 100.00, 179 bats tests. `appliance / build-and-boot` still red for the one reason, and it now prints both verdicts.

marcos-mendez commented

2026-09-28 02:30:58 -03:00

(Migrated from github.com)

Re-review of the fixes at 103b75f, run rather than read. common#5 and handbook #6 are untouched and keep their verdicts.

HIGH 1, the IPv4 assumption: fixed, and the second-degree finding is right

The split is the correct one. The five labels in BT_MOTD_FIELDS are the ones turnkey-sysinfo prints unconditionally, and the address row is now bt_motd_network_row accepting either shape. I checked the motd_after_ipv6_only fixture against the program rather than against the diff: if not all_nics: nics = ["Networking not configured"], then column = [disk_usage]; if nics: column.append(nics[0]), so with no IPv4 the command puts that string in the second column of the Usage of / row, which is exactly where the fixture has it. The fixture is faithful.

Moving "the operator is told how to reach the machine" onto the address the container actually answers on is a better assertion than the one I asked for, because it is about the machine and not about a word.

The reasoning behind collecting every address holds, and it is latent rather than live. keel_banner_pick_ipv6 ranks static 0, dynamic 1, temporary 2 and keeps the lowest; bt_global_ipv6 takes the first address lxc-info happens to print, with no ranking at all. Nothing makes those agree. Measured on the build host today they cannot disagree: wordpress-demo, forum and forum2 each hold exactly one global IPv6, dynamic mngtmpaddr noprefixroute, and net.ipv6.conf.all.use_tempaddr = 0. They can disagree the moment a static address from an instance spec sits beside an autoconfigured one, or use_tempaddr is non-zero, and the banner's ranking exists precisely because that case is expected. So this was a real latent false failure, bt_global_ipv6_all with any-satisfies is strictly safer and costs nothing, and going one degree past the finding was the right call. The negative case is tested too, which is what makes it an assertion rather than a widening.

HIGH 2, the short circuit: fixed, confirmed in CI

Run 36381004119 shows both verdicts reported. The login check fails, and the job continues to keel diff, which prints 6 same, 0 drift, 2 unknown, 4 not declared, 3 not compared and keel diff: no drift, but a declared field could not be observed offline, and then bt_checks_verdict names only boot-test: the login check failed (exit 1). The drift check reports while the login check is red, which is what was missing. || rc=$? and never ; rc=$?, as docs/traps.md requires. Zero checks is a failure and is tested: checks_verdict: no checks at all is a failure, not a pass passes in the run below.

Also visible in that log, and worth recording: the two new assertions both pass on the published layer (the system information block still reports on the network, the login says the machine is reachable at fc42:5009:...), so the only red verdicts left are the two the layer rebuild will clear.

MEDIUM 5, the build order: root cause verified, and it is a trap

The explanation is exactly right and I confirmed both halves. The local tkldev container runs fab 1.1.1, product.mk md5 0657df1a…, whose root.patched/body has one combined product-local units phase with the common removelists before it. The build host runs the project's fab, product.mk md5 a06bfe03…, with three unit phases (overlays, conf scripts, removelists) before the common removelists. Read against the stock file, the original note was a faithful reading; read against the build host's, it was wrong. Two machines whose hostname is tkldev, one with stock fab and one with the project's, is a trap worth writing down: it produces a confidently wrong answer that survives review, and the only reason this one did not ship is that the reading happened to be done on the other machine.

The corrected order in conf.d/main, COVERAGE.md and the changelog matches the build host's file line for line, including the note that unit conf scripts run before the product overlay.

MEDIUM (new): the correction names the wrong fab version, and it is the one without the documented order

conf.d/main:14, COVERAGE.md:100 and changelog:45 all say fab 1.1.1+keel1. The build host has 1.1.1+keel2. From the fab checkout on that host, 1.1.1+keel1 is b07a733, build-depend on python3-setuptools; the reordering arrives in 1.1.1+keel2 through be8ef5b, "Apply the units before the common removelists", and 29a9462, "Let a unit carry a removelist". So the order documented is keel2's, and keel1 still has upstream's. The scenario is the one the correction exists to prevent: a reader takes the note at its word, checks against 1.1.1+keel1, and finds it wrong in exactly the way the first version was. One character in three places.

MEDIUM 4: fixed, and the distinction is the right line

All three library paths are hardcoded now, 00-keel-banner included although it carried the shape before this branch, and the override survives only in conf.d/main, which runs in a chroot at build time and is the one place a test uses it.

The data-versus-code distinction is sound and I would draw it the same way. Sourcing a path executes whatever it names with the privileges the PAM stack has; reading KEEL_VERSION_FILE or KEEL_APPNAME_FILE changes a printed string. Those two are not equivalent risks and should not get the same treatment. The residual on the data paths is display integrity rather than execution, behind the same environment-injection precondition that does not exist on this appliance, and KEEL_APPNAME_FILE is confconsole's own convention besides. Accepted.

MEDIUM 3: recording it is enough

The note in conf.d/main is complete in the way that matters: it names the mechanism (bt-layer subtracts the parent's common_conf, so nothing built on core re-runs the script), why that is enough for the way Keel ships, the exact case it does not cover (a plain make of a non-core recipe, full COMMON_CONF, no parent), and the remedy (the same two calls in that recipe's own conf.d). It sits in the file the next person to touch this will open. Nothing enforces it, and the uncovered case is not hypothetical since the M0 gate builds that way, so it wants a follow-up issue. It does not want one before merge: every appliance Keel ships is strictly better off with this change and none regresses.

LOWs

6 is fixed well: keel_motd_unavailable_line stands in when the command is gone, and deliberately does not name the command, which keeps the forbidden-word check honest instead of quietly reintroducing the word it exists to remove. 7 is fixed and I ran it: a leading blank line is now skipped and the table survives, where before it took the whole block. 9 is fixed by inverting tests/coverage.sh to one kcov run per bats file.

8 can wait. turnkey-sysinfo calls tklbam-status only when geteuid() == 0, and only its output is cut, so the cost is one local subprocess per root login on a machine that already pays it today; this branch does not make it worse, and it disappears when the backup client leaves the image, which decision 0002 already schedules. An issue, not a blocker.

Tests

179 tests pass. COVERAGE_THRESHOLD=100 tests/coverage.sh: tests/lib/boot-test-lib.sh 214/214, overlay/usr/lib/keel/banner.sh 124/124, overlay/usr/lib/keel/motd.sh 62/62, conf.d/main 8/8, all 100.00 at threshold 100. The new tests assert behaviour rather than configuration throughout, including the negative cases for the address check and the empty case for the collected verdict.

Approve

No CRITICAL and no HIGH. Both HIGH findings are fixed and I confirmed each against something that runs rather than against the diff; the one new MEDIUM is a version string, and the two deferrals are recorded with their reasons in the places that will be read. appliance / build-and-boot stays red until the core layer is rebuilt and published, which is the correct state and not a finding.

Re-review of the fixes at `103b75f`, run rather than read. common#5 and handbook #6 are untouched and keep their verdicts. ## HIGH 1, the IPv4 assumption: fixed, and the second-degree finding is right The split is the correct one. The five labels in `BT_MOTD_FIELDS` are the ones `turnkey-sysinfo` prints unconditionally, and the address row is now `bt_motd_network_row` accepting either shape. I checked the `motd_after_ipv6_only` fixture against the program rather than against the diff: `if not all_nics: nics = ["Networking not configured"]`, then `column = [disk_usage]; if nics: column.append(nics[0])`, so with no IPv4 the command puts that string in the second column of the `Usage of /` row, which is exactly where the fixture has it. The fixture is faithful. Moving "the operator is told how to reach the machine" onto the address the container actually answers on is a better assertion than the one I asked for, because it is about the machine and not about a word. **The reasoning behind collecting every address holds, and it is latent rather than live.** `keel_banner_pick_ipv6` ranks static 0, dynamic 1, temporary 2 and keeps the lowest; `bt_global_ipv6` takes the first address `lxc-info` happens to print, with no ranking at all. Nothing makes those agree. Measured on the build host today they cannot disagree: `wordpress-demo`, `forum` and `forum2` each hold exactly one global IPv6, `dynamic mngtmpaddr noprefixroute`, and `net.ipv6.conf.all.use_tempaddr = 0`. They can disagree the moment a static address from an instance spec sits beside an autoconfigured one, or `use_tempaddr` is non-zero, and the banner's ranking exists precisely because that case is expected. So this was a real latent false failure, `bt_global_ipv6_all` with any-satisfies is strictly safer and costs nothing, and going one degree past the finding was the right call. The negative case is tested too, which is what makes it an assertion rather than a widening. ## HIGH 2, the short circuit: fixed, confirmed in CI Run 36381004119 shows both verdicts reported. The login check fails, and the job continues to `keel diff`, which prints `6 same, 0 drift, 2 unknown, 4 not declared, 3 not compared` and `keel diff: no drift, but a declared field could not be observed offline`, and then `bt_checks_verdict` names only `boot-test: the login check failed (exit 1)`. The drift check reports while the login check is red, which is what was missing. `|| rc=$?` and never `; rc=$?`, as `docs/traps.md` requires. Zero checks is a failure and is tested: `checks_verdict: no checks at all is a failure, not a pass` passes in the run below. Also visible in that log, and worth recording: the two new assertions both pass on the published layer (`the system information block still reports on the network`, `the login says the machine is reachable at fc42:5009:...`), so the only red verdicts left are the two the layer rebuild will clear. ## MEDIUM 5, the build order: root cause verified, and it is a trap The explanation is exactly right and I confirmed both halves. The local `tkldev` container runs `fab 1.1.1`, `product.mk` md5 `0657df1a…`, whose `root.patched/body` has one combined `product-local units` phase with the common removelists **before** it. The build host runs the project's fab, `product.mk` md5 `a06bfe03…`, with three unit phases (overlays, conf scripts, removelists) **before** the common removelists. Read against the stock file, the original note was a faithful reading; read against the build host's, it was wrong. Two machines whose hostname is `tkldev`, one with stock fab and one with the project's, is a trap worth writing down: it produces a confidently wrong answer that survives review, and the only reason this one did not ship is that the reading happened to be done on the other machine. The corrected order in `conf.d/main`, `COVERAGE.md` and the changelog matches the build host's file line for line, including the note that unit conf scripts run before the product overlay. ### MEDIUM (new): the correction names the wrong fab version, and it is the one without the documented order `conf.d/main:14`, `COVERAGE.md:100` and `changelog:45` all say `fab 1.1.1+keel1`. The build host has `1.1.1+keel2`. From the fab checkout on that host, `1.1.1+keel1` is b07a733, `build-depend on python3-setuptools`; the reordering arrives in `1.1.1+keel2` through be8ef5b, "Apply the units before the common removelists", and 29a9462, "Let a unit carry a removelist". So the order documented is keel2's, and keel1 still has upstream's. The scenario is the one the correction exists to prevent: a reader takes the note at its word, checks against `1.1.1+keel1`, and finds it wrong in exactly the way the first version was. One character in three places. ## MEDIUM 4: fixed, and the distinction is the right line All three library paths are hardcoded now, `00-keel-banner` included although it carried the shape before this branch, and the override survives only in `conf.d/main`, which runs in a chroot at build time and is the one place a test uses it. The data-versus-code distinction is sound and I would draw it the same way. Sourcing a path executes whatever it names with the privileges the PAM stack has; reading `KEEL_VERSION_FILE` or `KEEL_APPNAME_FILE` changes a printed string. Those two are not equivalent risks and should not get the same treatment. The residual on the data paths is display integrity rather than execution, behind the same environment-injection precondition that does not exist on this appliance, and `KEEL_APPNAME_FILE` is confconsole's own convention besides. Accepted. ## MEDIUM 3: recording it is enough The note in `conf.d/main` is complete in the way that matters: it names the mechanism (`bt-layer` subtracts the parent's `common_conf`, so nothing built on core re-runs the script), why that is enough for the way Keel ships, the exact case it does not cover (a plain `make` of a non-core recipe, full `COMMON_CONF`, no parent), and the remedy (the same two calls in that recipe's own `conf.d`). It sits in the file the next person to touch this will open. Nothing enforces it, and the uncovered case is not hypothetical since the M0 gate builds that way, so it wants a follow-up issue. It does not want one before merge: every appliance Keel ships is strictly better off with this change and none regresses. ## LOWs 6 is fixed well: `keel_motd_unavailable_line` stands in when the command is gone, and deliberately does not name the command, which keeps the forbidden-word check honest instead of quietly reintroducing the word it exists to remove. 7 is fixed and I ran it: a leading blank line is now skipped and the table survives, where before it took the whole block. 9 is fixed by inverting `tests/coverage.sh` to one kcov run per bats file. 8 can wait. `turnkey-sysinfo` calls `tklbam-status` only when `geteuid() == 0`, and only its output is cut, so the cost is one local subprocess per root login on a machine that already pays it today; this branch does not make it worse, and it disappears when the backup client leaves the image, which decision 0002 already schedules. An issue, not a blocker. ## Tests 179 tests pass. `COVERAGE_THRESHOLD=100 tests/coverage.sh`: `tests/lib/boot-test-lib.sh` 214/214, `overlay/usr/lib/keel/banner.sh` 124/124, `overlay/usr/lib/keel/motd.sh` 62/62, `conf.d/main` 8/8, all 100.00 at threshold 100. The new tests assert behaviour rather than configuration throughout, including the negative cases for the address check and the empty case for the collected verdict. ## Approve No CRITICAL and no HIGH. Both HIGH findings are fixed and I confirmed each against something that runs rather than against the diff; the one new MEDIUM is a version string, and the two deferrals are recorded with their reasons in the places that will be read. `appliance / build-and-boot` stays red until the core layer is rebuilt and published, which is the correct state and not a finding.

marcos-mendez commented

2026-09-28 02:38:06 -03:00

(Migrated from github.com)

The new MEDIUM is fixed in b30fa53, and you were right about the shape of it: the correction named the one release that does not carry the order it describes.

Verified on the build host rather than taken from the review. dpkg-query -W fab says 1.1.1+keel2; /usr/share/fab/product.mk is md5 a06bfe03 and is byte-identical to share/product.mk at the fab checkout's HEAD; b07a733 (1.1.1+keel1) has that file at md5 c04cb601 with no unit phase in it at all, and it touches only debian/changelog and the build dependencies; the local tkldev container that produced the original wrong reading has stock 1.1.1, md5 0657df1a.

All three places now say 1.1.1+keel2 and give the md5, so the next reader can check the file in front of them instead of trusting a version string, and each says the reading has to come from the machine that builds. A version string alone was not enough twice running.

The trap is written up as its own pull request in the handbook, Keel-Linux/handbook#13, closing Keel-Linux/handbook#12, with the three files and their md5 sums in a table so the check is one line rather than an instruction to be careful.

MEDIUM 3 now has its follow-up: Keel-Linux/tracker#16, naming the ten recipes and the two calls each needs, and recording why it should not block this.

tests / coverage green on the head: 214/214, 124/124, 62/62, 8/8, all 100.00, 179 tests. appliance / build-and-boot red for the one reason, with both verdicts reported.

The new MEDIUM is fixed in `b30fa53`, and you were right about the shape of it: the correction named the one release that does not carry the order it describes. Verified on the build host rather than taken from the review. `dpkg-query -W fab` says **1.1.1+keel2**; `/usr/share/fab/product.mk` is md5 `a06bfe03` and is byte-identical to `share/product.mk` at the fab checkout's HEAD; `b07a733` (1.1.1+keel1) has that file at md5 `c04cb601` with no unit phase in it at all, and it touches only `debian/changelog` and the build dependencies; the local tkldev container that produced the original wrong reading has stock `1.1.1`, md5 `0657df1a`. All three places now say `1.1.1+keel2` **and give the md5**, so the next reader can check the file in front of them instead of trusting a version string, and each says the reading has to come from the machine that builds. A version string alone was not enough twice running. The trap is written up as its own pull request in the handbook, Keel-Linux/handbook#13, closing Keel-Linux/handbook#12, with the three files and their md5 sums in a table so the check is one line rather than an instruction to be careful. MEDIUM 3 now has its follow-up: Keel-Linux/tracker#16, naming the ten recipes and the two calls each needs, and recording why it should not block this. `tests / coverage` green on the head: 214/214, 124/124, 62/62, 8/8, all 100.00, 179 tests. `appliance / build-and-boot` red for the one reason, with both verdicts reported.

marcos-mendez commented

2026-09-29 00:40:19 -03:00

(Migrated from github.com)

It was red because the appliance check boots the published core (53d9fb5), which predates the login change, and a required strict check that only a post-merge rebuild could satisfy could never go green. f80b1a6 asks the login check only of a layer carrying /usr/lib/keel/motd.sh (which conf.d/main refuses to build without); on the published layer it now reports "did not apply" and does not count as a pass, and a wrong login on a layer with the change still fails. Master merged in (4c41456); run 36517520966 is green.

It was red because the appliance check boots the published core (`53d9fb5`), which predates the login change, and a required strict check that only a post-merge rebuild could satisfy could never go green. f80b1a6 asks the login check only of a layer carrying `/usr/lib/keel/motd.sh` (which `conf.d/main` refuses to build without); on the published layer it now reports "did not apply" and does not count as a pass, and a wrong login on a layer with the change still fails. Master merged in (4c41456); run 36517520966 is green.

This pull request has changes conflicting with the target branch.