feat(uninstall): remove what this tool put on the host, not one file#14
Merged
Conversation
Groundwork only; uninstall itself is unchanged. Both gaps were found by mapping what a complete teardown must touch, and both are defects on their own terms. schedule can now FIND its units. sudoers and sshdconf each have an Orphans() sweep; schedule had none, because Cancel is name-keyed — it derives every path from a username you must already know, and the registry that knows usernames is the thing that goes missing. So a unit whose row was lost was unreachable: nothing could name it, so nothing could cancel it. That has teeth because a unit IS an ExecStart on the installed binary: one nobody can name still fires, and once the binary is gone it fires forever and fails forever, leaving the account it should have deleted alive with its grants. The sweep globs v1's prefix too, and that is the point rather than a courtesy. v1's units carry no "-v2-" infix, so the v2 glob walks straight past them, and v1's install path was byte-identical to v2's — a v1 timer on an upgraded host invokes THIS binary, with an argv this code parses perfectly. Its accounts strand exactly like v2's. config gains StateDir and the v1 names so they can be found rather than guessed at. sudoers.Remove can now report failure, and every caller listens. It returned nothing, so the removal of a file that hands out passwordless root the instant its username exists could fail without a word. revoke strips that grant FIRST, deliberately, so an account surviving a refused revoke cannot survive holding root — and the error saying it had survived was discarded. cleanup-expired was worse: it printed "removed an orphaned sudo grant" whichever way the removal went, which is the worst available lie about that particular file. Note Go will not point at these call sites: a function returning an error can be called as a statement, so changing the signature compiled clean and every existing caller kept ignoring it. They were found by grep, not the compiler. The test for the surviving grant makes os.Remove fail AS ROOT — a read-only directory does not, root bypasses it, and the test would have skipped in CI forever while asserting nothing. It also asserts the distinctive phrase rather than "sudo": revoke's absent-account branch says "cleaning up registry/sudoers/sshd" on that very path, so the loose substring passed with the reporting deleted. The mutation caught both. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
uninstall deleted /usr/local/sbin/linux-temp-admin and left everything else exactly where it was: the accounts, their sudo grants, their sshd exceptions, their auto-delete tasks, the registry, the language preference, and v1's leftovers. The menu entry had to be called "卸载稳定命令" — uninstall the stable command — because the opaque object was the only honest part of the label. It now says 卸载, because it now means it. It takes an inventory, shows it, asks once, then tears down in an order where every step leaves the host no worse than it found it. --force was a trap. The auto-delete unit hardcodes an ExecStart on the install path, and --force skipped the "registered users still exist" refusal — so it deleted the binary out from under every timer on the box, which then fired against a missing path forever. The accounts never expired and a NOPASSWD-sudo account was left with nothing coming for it. --force keeps only its original meaning now. The invariant is that the binary is never removed while a managed account survives: leaving a sudo-capable account while deleting the only thing that manages it is worse than not uninstalling. Survival is decided by asking the system, not by revoke's exit code — revoke answers 0 for a deletion, for cleaning up an account that was already gone, AND for a declined confirmation, so its rc cannot carry that weight. The inventory unions witnesses because the registry is the weakest one: every way it goes wrong drops accounts silently rather than announcing them, and an inventory that under-reports is how a teardown strands what it never saw. A witness that cannot be READ refuses the whole thing. The GECOS marker is deliberately not a witness — it is the one signal an account can write to itself, so trusting it would let `usermod -c` enlist a real administrator's account and home directory into a teardown. v1's users.tsv is read before it is deleted. It is not litter; it is v1's account registry, and v1's install path was byte-identical to v2's, so a v1 timer invokes this binary. Deleting that registry while globbing only the "-v2-" prefix would have reproduced this very release's footgun for v1 accounts and shredded the only record naming them. The audit log survives by default. It records who opened and closed root-capable accounts; erasing it on the way out is what covering tracks looks like. The teardown's own record is written before --purge-audit runs, or the purge would recreate the file and mean "leave exactly one line". Refusing to run from the account it would delete is an interlock for the honest operator, not a security boundary: `sudo su -` drops SUDO_USER, the only identity signal this tool has. The code says that rather than implying a guarantee it cannot keep. StateDir and AuditLogDir are App fields, not constants read in place, for the same reason InstallPath already was: these are removed RECURSIVELY, and CI runs the integration suite as root on every push. A test reading the constants would delete the real /var/lib/linux-temp-admin — on the runner and on the developer's box. A test pins that they stay injected. Every guard here was mutation-tested: each was removed, the test confirmed to fail, the guard restored. An existing test (TestUninstallRefusesOnRegistryReadError) caught a real regression on the way — the first draft warned where the old code refused, which is exactly the blind-inventory hazard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A six-lens review (43 agents, three refuters per finding) surfaced six real defects. The highest was a strand the redesign was specifically meant to prevent. Witness-only accounts are now actually removed. The teardown reused revoke with --yes but not --force, and revoke REFUSES an unregistered account before its cleanup branch runs. Every witness the union inventory was built for — the sudoers glob, the sshd glob, the unit globs, v1's registry — names accounts that are by definition NOT in the v2 registry, so revoke turned away exactly the accounts the inventory worked hardest to find. Because such an account (when already gone) is not a survivor by user.Exists, the teardown removed the binary and printed "their grants are gone" while a NOPASSWD:ALL drop-in that re-arms full root on username reuse sat untouched on disk. It now passes --force --confirm-force <name>; revoke's protected-target and UID-proof gates ignore --force, so a real non-managed account is still refused and the survivor check still catches it. v1's units are removed, not just found. Cancel derived the unit path from the v2 prefix only, so a v1 timer the inventory listed was disabled and deleted under a name that does not exist — leaving the real v1 .timer/.service enabled against the now-removed binary, reproducing this release's own footgun for v1. Cancel now disables and removes every unit prefix it recognises. The v1 registry witness got the error channel every other witness has. It collapsed "present but unreadable" into "absent" and never checked sc.Err(), so the one file the code calls the only record of a sudo-less v1 account could vanish from the inventory without tripping the fatal-inventory gate. Now a present-but-unreadable v1 registry refuses the uninstall. The GECOS test could not fail. It asserted the marker is not a witness while creating no marked account, so it would have passed if the marker BECAME a witness — the vacuous-assertion shape that has bitten this work repeatedly. It now creates a real account carrying the managed GECOS and nothing else, and a mutation that trusts the marker fails it. Two documentation fixes the review caught: binaryBlocker said "needs --force" even when --force was passed (it now reports no blocker under force), and the CHANGELOG claimed revoke/cleanup-expired were "untouched" in the same entry that describes changing them (reworded to: their CONTRACT is unchanged, with the one internal change named). Every new guard mutation-tested. Full unit + vet (both tags) + race + root integration + non-root-as-nobody pass, and the witness-only cleanup was confirmed on a real pty. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
Adversarial review (43 agents, 3 refuters/finding) found six real defects — fixed in e2c9ee9The highest was a strand the redesign exists to prevent, and the review caught it because I'd built the union inventory and then reused HIGH — witness-only accounts were refused, not removedThe teardown called MEDIUM (completeness critic) — v1 units found but never removed
HIGH — v1 registry witness had no error channelIt collapsed present-but-unreadable into absent and never checked MEDIUM — the GECOS test could not failIt asserted the marker is not a witness while creating no marked account, so it would have passed if the marker became one. This is the vacuous-assertion shape that's bitten this work repeatedly. It now creates a real account carrying the managed GECOS and nothing else, and a mutation that trusts the marker fails it. LOW ×2 (docs)
Every new guard mutation-tested. Full unit + Refuted (not changed): 7 findings, incl. a claim that the v2.5.1 CHANGELOG history was "erased" (v2.5.1 was never tagged/released — it folds into this release) and a re-report of the witness-only issue against already-fixed code. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This started from one observation — 「卸载稳定命令」这个命名很奇怪 — and the name turned out to be the honest part.
uninstalldeleted one file and left everything else exactly where it was. The label needed that opaque object ("the stable command") precisely because "uninstall" alone would have been a lie.The menu entry is now just 「卸载」, because it now means it.
What it does
Inventory → one confirmation → teardown in an order where every step leaves the host no worse than it found it.
Accounts go through the ordinary
revoke— same path, same protections, same audit trail. Nothing reimplements deletion.uninstall --forcewas a trapThe auto-delete unit hardcodes
ExecStart=/usr/local/sbin/linux-temp-admin revoke --user X --yes.--forceskipped the "registered users still exist" refusal, so it deleted the binary out from under every timer on the box. Those timers then fired against a missing path forever: the accounts never expired, and a NOPASSWD-sudo account was left with nothing coming for it.--forceno longer bypasses accounts; it keeps only its original meaning (remove a target that isn't a safe root-owned regular file).The invariant
The binary is never removed while a managed account survives. Leaving a sudo-capable account while deleting the only thing that manages it is worse than not uninstalling. If any account can't be removed, the binary and the state directory stay (that row is the only record of what it was), the account is named, rc=1.
Survival is decided by asking the system (
user.Exists), not byrevoke's exit code — which returns 0 for a deletion, for cleaning up an already-gone account, and for a confirmation the operator declined.The inventory unions witnesses; the registry is the weakest
Every way a file goes wrong makes accounts vanish from it rather than announce themselves, and an inventory that under-reports is exactly how a teardown strands what it never saw. So an account named by its sudo grant, sshd exception, auto-delete unit, or v1's registry is torn down too. A witness that cannot be read refuses the whole uninstall.
The GECOS marker is deliberately not a witness — a security property, not an omission. It's the one signal an account can write to itself, so
usermod -c 'linux-temp-admin temporary admin' realadminwould otherwise enlist a real administrator's account and their home directory into a teardown.v1 is read before it is deleted
/var/lib/linux-temp-admin/users.tsvisn't litter — it's v1's account registry. v1's units carry no-v2-infix (so the v2 glob walks past them) and v1's install path was byte-identical to v2's, so a v1 timer on an upgraded host invokes this binary. Deleting that registry while globbing only the v2 prefix would have reproduced this release's own footgun for v1 accounts and shredded the only record naming them.schedulegained theOrphans/UnitUserssweepsudoersandsshdconfalways had.Also here
--purge-auditremoves it, and the teardown's own record is written before the purge, so "purge" can't mean "leave exactly one line".sudo su -dropsSUDO_USER, the only identity signal this tool has). The code says so rather than implying a guarantee it can't keep.sudoers.Removereports failure and every caller listens. Note Go won't point at those call sites — a function returning an error can be called as a statement, so the signature change compiled clean with every caller still ignoring it. Found by grep, not the compiler.StateDir/AuditLogDirare App fields, not constants read in place — these are removed recursively and CI runs the integration suite as root on every push. A test reading the constants would delete the real/var/lib/linux-temp-admin, on the runner and on the developer's box. A test pins that they stay injected.Verification
Every guard mutation-tested — removed, test confirmed to fail, restored:
TestUninstallKeepsTheBinaryWhenAnAccountSurvivesTestUninstallRefusesFromTheAccountItWouldDeleteTestUninstallRefusesWhenTheInventoryIsBlind--yesdeletes accounts without--remove-usersTestUninstallWithAccountsRefusesNonInteractivelyWithoutTheFlagTestUninstallKeepsTheAuditLogUnlessAskedTwiceTestInventoryUnionsEveryWitnessAn existing test caught a real regression on the way: the first draft warned where the old code refused on an unreadable registry — the exact blind-inventory hazard.
Verified on a real pty. Full unit +
go vet(both tags) +-race+ root integration + non-root-as-nobodyall pass.Minor, not patch:
uninstalldoes substantially more,--forceand the menu's fifth entry change meaning,--remove-users/--purge-auditare new.invite,revoke,status,cleanup-expiredare untouched.🤖 Generated with Claude Code