Repository navigation
keel-wp and keel-wordpress-update, with the turnkey names kept as symlinks - #6
Conversation
…mlinks The two commands an operator types on this appliance are written by this overlay, so the name they carry is this project's choice and nobody else's. The Keel name becomes the real command and the turnkey-* name is kept as a symlink to it, which is the general policy of decision 0015 of the handbook (tracker#12). Both names keep working: somebody who pasted a command out of TurnKey's documentation, or who wrote a script against turnkey-wp last year, must not silently lose it. The links are committed into the overlay rather than made in conf.d/main. fab-apply-overlay copies an overlay with "cp -TdR" and -d keeps a link a link; this recipe applies its overlay twice, once through COMMON_OVERLAYS and once as the product-local ROOT_OVERLAY, and a second copy over an existing link still leaves a link. tests/wrappers.bats asserts both by making that copy with the same command the build uses and then running the copied turnkey-wp, because a link that resolves is not a link that works. What either command does is unchanged. keel-wp runs wp-cli through runuser as www-data, never as root, with an explicit --path, and owns /var/www/.wp-cli. keel-wordpress-update refuses a caller who is not root, updates core, verifies WordPress's own per file checksums and puts the ownership boundary back; its refusal now names whichever of its two names was typed. The paths each script hands to another program are overridable from the environment, as keel-wp's WP_DIR, WP_USR and WP_CACHE already were, with the hard coded values as the defaults, so a booted appliance behaves identically and a test can drive the pair against a scratch tree. That is what makes them measurable: both were at no coverage at all and are now at 100 percent of their lines, 9/9 and 18/18, with every branch driven.
A caller left on the old name still works through the symlink, which is exactly why a call site is easy to miss: the build would have passed either way. So they were found by grep rather than by the gate. conf.d/main verifies WordPress's per file checksums with keel-wp during the build, and its comment about who uses wp-cli afterwards says keel-wp too. README.rst documents the supervised core update as keel-wordpress-update in both places it names it. tests/v19.sh, the release acceptance flow, drives keel-wp through siteurl, post create, get and delete, core version, check-update and verify-checksums, and runs the updater as keel-wordpress-update. tests/v19.sh also asks the compatibility names for the same answers on a real appliance: turnkey-wp has to report the same core version and the same siteurl keel-wp just did, and the updater's root guard has to hold under turnkey-wordpress-update as well. Reading the link would have proved neither. firstboot.d/40wordpress is not a call site: it calls wp-cli directly with --allow-root, not through either wrapper.
Without a changelog entry the newest installable version stays the old one and the change never reaches a machine, which is the trap docs/traps.md of the handbook records and what the "package / changelog" gate exists to catch. COVERAGE.md records what was measured rather than what was hoped for: eight shell files now, 99.12 percent (562/567) over 217 bats tests, none failing, with keel-wp at 100 percent (9/9) and keel-wordpress-update at 100 percent (18/18). The threshold stays 97, the lowest measured file, which is still 40wordpress.
ReviewI re-ran the suite and the coverage measurement, read fab's own source for the overlay step, mutated the new tests to see which of them are load-bearing, and read the booted Verified
MEDIUM 1 — the one safeguard flagged for review is the one the suite cannot fail on
The scenario: someone later "tidies" the variable back to
export WP_USR=keel-test-web-user WP_USER=keel-test-web-user
MEDIUM 2 — a destructive root-run script now takes its target path from an unvalidated environment variable
This is the half of the refactor that deserves the scrutiny it was flagged for. Lines 25-27 are To be clear about what this is not: it is not a privilege escalation. I checked for a constrained root path and there is none — no sudoers rule, no cron entry, no unit, no confconsole plugin calls this, and It is still a footgun the hard-coded version did not have, and
Related, minor: MEDIUM 3 — the compatibility assertion on a real appliance passes if both sides fail
test "$(turnkey-wp option get siteurl)" = "$(keel-wp option get siteurl)"
The script already knows the literal it wants; it asserted it at line 22. Comparing against it instead costs nothing and cannot pass on a double failure: test "$(turnkey-wp option get siteurl)" = https://localhostLine 64 ( LOW 1 —
|
…ller Review of #6 found that the one safeguard the description put up for judgement was the one the suite could not fail on, and that the refactor which made the script testable had opened a footgun on a script that runs as root. The web user. tests/wrappers.bats took the expected value from id -un, and under bats USER is already id -un, so ${USER:-www-data} and ${WP_USER:-www-data} expanded to the same string and no assertion could tell them apart: the reviewer reintroduced USER="${USER:-www-data}" throughout and all 21 tests stayed green. The web user in the tests is now the sentinel keel-test-web-user, which no account can be called, and a test runs the updater with USER=root and requires wp-content to still go to the sentinel. chown, install and runuser are stubs that only log, so the value never needed to name a real account. The target. WPROOT, WP_USER and WP_CLI were readable from the environment under those names, so WP_CLI=/bin/true WPROOT=/ keel-wordpress-update would chown and chmod the whole filesystem and strip setuid from sudo, su and passwd. It needs root to trigger and there is no constrained root path to this script, but the hard coded version could not express it at all, and WPROOT is not an obscure name: conf.d/main of this repository uses it for this same path. The three are now KEEL_TEST_WPROOT, KEEL_TEST_WP_USER and KEEL_TEST_WP_CLI, the prefix this suite already uses for exactly this purpose, and a test asserts that the unprefixed names change nothing. Belt and braces on a destructive root script: the updater refuses a target that is not a WordPress before it rewrites anything, by the same wp-includes/version.php that conf.d/main asserts after unpacking the release. keel-wp gave its WP_CLI back and calls /usr/local/bin/wp literally again. It never needed the knob, because runuser is stubbed and the command string is never executed, and the test asserts the shipped path instead. All three mutations are caught now: USER="${USER:-www-data}" fails three tests, un-namespacing WPROOT fails eight, and dropping the WordPress guard fails one. Also in this file, from the same review: it is -L that would flatten the compatibility link, not the absence of -d. -P is already the default under -R and -d only adds --preserve=links, which is about hard links, so the comment claiming -d was load-bearing described a property the test did not check -- dropping -d from the test's own cp failed nothing. There is now a test that checks the real one: a plain cp -TR still yields a working turnkey-wp, and cp -TLR turns it into a second regular file.
… failed
tests/v19.sh compared two command substitutions inside test:
test "$(turnkey-wp option get siteurl)" = "$(keel-wp option get siteurl)"
set -euo pipefail is on, but a command substitution in test's arguments does
not trip errexit. With wp-cli broken, the path wrong or Apache down, both
sides are empty and this is test "" = "", which passes. That is docs/traps.md
of the handbook, "asserting the configuration is not asserting the
behaviour", in the one check offered as proof that the old name still answers
on a real machine.
It now compares against the literal the script already asserted at line 22,
which cannot pass on a double failure. $before, the yardstick for the two
core version comparisons, is asserted non-empty where it is taken, for the
same reason.
The claim that -d is what keeps a symlink a symlink was wrong in COVERAGE.md and in the changelog as well as in the test. -R copies a symlink as a symlink unless -L is given; -P is already the default and -d only adds --preserve=links, which is about hard links. Measured with coreutils 9.7 and now asserted by a test rather than only written down. The conclusion is unchanged: the build's copy step is safe, but for a different reason than the one given. The changelog entry also records the KEEL_TEST_ prefixed overrides and why the prefix is not decoration, the new refusal of a target that is not a WordPress, that keel-wp gained no knob at all, and that both links are relative because an absolute one does not resolve inside fab-chroot. Numbers re-measured: 221 bats tests, none failing, 99.12 percent (564/569) over the eight measured files, keel-wp 8/8 and keel-wordpress-update 21/21. .github/workflows/tests.yml still said tests/coverage.sh measures six shell files; it has measured eight since the previous commit on this branch. The threshold below it is unchanged and still correct.
Re-reviewI checked each fix by running it, not by reading the diff. Every claim in the update holds, with one numeric correction. The suite is 25 tests in The three MEDIUM findings, re-testedMEDIUM 1, the Three tests, as claimed — the numbers are 22, 23 and 24, not the 20, 23 and 24 in the description. Substance unaffected; worth correcting only because the description is where the next person will look. Test 24 is the right shape: it sets MEDIUM 2, the footgun — both fixes taken, both load-bearing, namespacing complete in the script the finding named. Un-namespacing Namespacing is complete in One thing I want to name rather than leave implicit, because it is the only un-namespaced override left anywhere in these two files. MEDIUM 3, the vacuous v19 assertion — fixed, and the line I had excluded was fixed too. The
|
1 participant
Closes #7. Org-wide context, cross-reference only: Keel-Linux/tracker#12. The policy it implements is handbook decision 0015 (Keel-Linux/handbook#3).
Merge order: Keel-Linux/handbook#3 first. This branch cites decision 0015 in
changelog,COVERAGE.md,tests/wrappers.batsand both script headers, and 0015 is not on the handbook'smainyet.The two commands an operator types on this appliance are written by this overlay, so the name they carry is this project's choice. The Keel name becomes the real command and the
turnkey-*name is kept as a symlink to it. Both keep working: somebody who pasted a command out of TurnKey's documentation, or who wrote a script againstturnkey-wplast year, must not silently lose it.overlay/usr/local/bin/keel-wp, withturnkey-wpa symlink to itoverlay/usr/local/sbin/keel-wordpress-update, withturnkey-wordpress-updatea symlink to itBoth links are relative. An absolute link does not resolve inside
fab-chroot, soconf.d/mainwould stop finding the command at build time; making either absolute fails three tests here.What does not change
keel-wpstill runs wp-cli throughrunuseraswww-data, never as root, with an explicit--path, and still owns/var/www/.wp-cli.keel-wordpress-updatestill refuses a caller who is not root, updates core, verifies WordPress's own per file checksums and puts the ownership boundary back. Its refusal now names whichever of its two names was typed, so an operator who ranturnkey-wordpress-updateis not told about a command they did not run.keel-wpgains no new override: itsWP_DIR,WP_USRandWP_CACHEare the three it already shipped with, and it calls/usr/local/bin/wpliterally as before.keel-wordpress-updatereads its WordPress root, web user and wp-cli path fromKEEL_TEST_WPROOT,KEEL_TEST_WP_USERandKEEL_TEST_WP_CLIwhen set, with the previously hard-coded values as defaults, so a booted appliance behaves identically. The prefix is deliberate — see the review response below. It also now refuses a target that is not a WordPress, by the samewp-includes/version.phpthatconf.d/mainasserts after unpacking.Responding to the review
USERsafeguard was untestable, since under bats$USERequals$(id -un)and the suite used that as the expected valuekeel-test-web-user, which no account can be called;MEis gone. A test runs the updater withUSER=rootand requireswp-contentto still go to the sentinel. The reviewer's mutation,USER="${USER:-www-data}"throughout, now fails three tests.WP_CLI=/bin/true WPROOT=/ keel-wordpress-updatewalks the whole filesystem as rootKEEL_TEST_*, the prefix this suite already uses; and the script refuses a target with nowp-includes/version.phpbefore it rewrites anything. Un-namespacingWPROOTnow fails eight tests; dropping the guard fails one.tests/v19.sh:65compared two command substitutions, passing astest "" = ""$before, the yardstick for the twocore versioncomparisons, is asserted non-empty where it is taken.-dis not what preserves the symlink, and the test could not telltests/wrappers.bats,COVERAGE.mdand thechangelog.-Rcopies a symlink as a symlink unless-Lis given;-Pis already the default and-donly adds--preserve=links, which is hard links. A new test asserts the real property: a plaincp -TRstill yields a workingturnkey-wp, andcp -TLRturns it into a second regular file..github/workflows/tests.yml:20says eight shell files.first=loop is gone.keel-wp:15also gainedWP_CLIrunuseris a stub and the command string is never executed. The test asserts the literal/usr/local/bin/wpinstead.KEEL_TEST_prefixed and read only by the tests;keel-wp's two remain the shipped operator-facing ones. The distinction is now meaningful rather than accidental.I did not remove the second
cp -TdR, which the reviewer flagged as redundant but worth keeping.How the symlink is made, and why that way
Committed into the overlay, not created by
ln -sinconf.d/main.fab-apply-overlayiscp -TdR OVERLAY DEST(cmd_apply_overlayin fab). Measured here with coreutils 9.7:So the property is the absence of
-L.tests/wrappers.batsasserts all three rows, copies the overlay with the build's own command twice (once forCOMMON_OVERLAYS, once for the product-localROOT_OVERLAY) and then runs the copied command.Call sites
A caller left on the old name still works through the symlink, which is exactly why one is easy to miss: the build would pass either way. They were found by
grep -rn, not by the gate.conf.d/mainkeel-wp core verify-checksums --version=...during the build, and the comment above itREADME.rsttests/v19.shturnkey-wpcall, and the updater invocationoverlay/usr/lib/inithooks/firstboot.d/40wordpressis named in #7 as a call site, and it is not one: it calls wp-cli directly with--allow-root, never through either wrapper. Nothing there needed changing.Tests
The 21 original tests were written before the code and watched go red. The four added in response to the review were written the same way, and each was checked against the mutation it exists to catch.
tests/v19.shkeeps everything it drove before — siteurl, post create/get/delete,core version,check-update,verify-checksumsand the non-root updater guard — now under the Keel name, and additionally asks the compatibility names for the same answers on a booted appliance, each against a literal.99.12 percent (564/569) over the eight measured shell files, up from 99.07 percent (535/540) over six on master. Both wrappers were at no coverage at all and are at 100 percent with every branch driven. Threshold stays 97, still set by
40wordpress.Test plan
bats tests/— 221 tests, 0 failuresCOVERAGE_THRESHOLD=97 tests/coverage.sh— exit 0, numbers aboveshellcheckclean on both scripts andtests/coverage.shUSERmutation, un-namespacingWPROOT, and dropping the WordPress guard each fail tests nowappliance / build-and-boot— see belowtests/v19.shon a rebuilt appliance, where the compatibility names are proved on a real machineappliance / build-and-bootfailed on the previous push for a reason outside this branch: the job boots the layer the mirror already serves, which does not carry this change, and every appliance assertion passed. It failed onapt-get update, where the runner resolvesarchive.keellinux.orgto its own loopback (127.0.1.1) and is served a self-signed certificate. This branch touches neithertests/boot-test.shnortests/lib/boot-test-lib.sh.