Repository navigation
fix sudoers setup to block privilege escalation - #85
Merged
Merged
Conversation
…s only The rules named $YBIN/y-etc-hosts-write and $YBIN/y-localhost, files in the grantee's own checkout. Whoever can edit them, the grantee and every agent running as that user, could put anything there and run it as root. That was acceptable on a laptop where the user is the admin anyway, not on a shared or hardened host where an unprivileged bot account runs clusters. y-cluster-sudoers --write now installs root-owned snapshots (0755, owner 0) of both helpers to /usr/local/libexec/ystack, records their sha256 and source path in SOURCE there (no git as root: a checkout's config can run commands), and writes rules naming only those copies, validated with visudo -cf. It refuses to snapshot from a checkout owned by a grantee unless --allow-grantee-source; the intended use is an admin running it from their own checkout at a reviewed commit. Without --write it prints the rules, what would be installed and whether --write would refuse. Callers prefer the installed copy when it is root-owned and not group/other writable, and fall back to $YBIN otherwise (a sudo password, or an old rule): y-k8s-ingress-hosts for the hosts swap, warning when the copy differs from the checkout's; y-localhost for its self-escalation. Root-owned alone is not enough, because the helpers act on input the grantee controls, so they now limit what a NOPASSWD run can do: - y-etc-hosts-write only accepts changes inside the k8s-ingress-hosts block (everything outside must equal the current file, at most one block), and entries there must map plain hostnames to 127/8, 10/8, 172.16/12 or 192.168/16. Before, a grantee could replace /etc/hosts wholesale, e.g. point any public name at an address of their choice for all users of the host. - y-localhost only adds 127.0.0.0/8 aliases to lo. It took the address from /etc/hosts, which y-etc-hosts-write let the same user write, so it could bind any address, a public one included, to loopback and capture the host's traffic to it. Hostname argument validated, exact field match instead of a grep regex. Relates to Yolean/y-cluster#11 (install help for passwordless sudo, to a path where only root can update the binary). Tested on gle01 with temp targets (Y_ETC_HOSTS) and a copy of y-localhost on a fake hosts file: block append/removal pass; edits outside the block, public or 172.32 addresses, two or unbalanced blocks, odd hostnames and non-loopback aliases are refused. --write itself not run (needs root). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
grantee_owned_sources ended in `[ "$owner" = "$u" ] && echo ...`, so when the last comparison was false (the intended case: an admin's own checkout) the function returned 1, and under set -e the GRANTEE_OWNED=$(...) assignment ended the script without a word. --write therefore installed nothing and wrote no rules, and the dry run stopped after its "would install" line. Found when the owner's run on gle01 left no /usr/local/libexec/ystack. The earlier tests used a checkout owned by the grantee, where the last comparison happens to be true. Tested now with a grantee owning nothing, and the whole --write path run as non-root against scratch paths (validation, install of both helpers, SOURCE, sudoers file 0440). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
checkit's blobs-v3/site-apply requests one bucket per site from the backend named site-userdata-blobs; on g2 that is GCS. The comment next to builds-registry-blobs said to add this entry, on the same versitygw, once a local cluster hosts sites, so checkit's per-site Buckety CRs are byte-identical here and on g2. The checkit appliance (checkit branch sprint-060-appliance) is that cluster. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The download went straight to the binary's final path and was extracted and made executable in place afterwards. A run that stopped after curl left the archive there, and every later run saw the file and executed it: "cannot execute binary file: Exec format error" until someone deleted it. A caller piping to head is enough to stop it, by SIGPIPE on the next line of download output. Parallel agents running `kubectl ... 2>&1 | head -3` on a checkout that needed kubectl 1.36.5 left both a ystack clone and a worktree with the 34 MB kubernetes-client tarball as y-kubectl-v1.36.5-bin. Now the download, the sha256 check and the unpacking all work on a temporary file next to the binary, which is renamed into place only when complete, so an interrupted run leaves no binary and a concurrent one sees either none or a finished one. The name symlink is replaced by rename as well: removing it first let a parallel run find it missing and fail (2 of 60 invocations in a 12-way test; 0 of 120 now). A regular file at the link's name is still refused, as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
k8s-ingress-hosts ends each entry with a comment naming the route it
came from ("127.0.0.1 keycloak-admin.appliance # keycloak-v3/
keycloak-admin"), and the block policy from 39a175a checked every field
after the IP as a hostname, so it refused every entry the tool writes.
A field starting with # now ends the entry, as in hosts(5); an entry
still needs at least one valid hostname before it, and a # inside a
name is still refused.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Two ways a NOPASSWD grantee could still use the root-run writer against the host, found reviewing #85 and reproduced with Y_ETC_HOSTS on scratch files (material in bots-state/code-review/prs/ystack-85/ on metalblack): - The block could give a name that root maintains outside it another address. The block's position is free, and hosts(5) lookups take the first match, so a block placed first with "127.0.0.9 db.internal" shadowed root's "10.1.1.1 db.internal" for every user of the host. A block entry for a name with an entry outside the block must now use that entry's address. A duplicate with the same address is still accepted, so a hand-made line for a cluster name keeps working. - stdin went to a temporary file next to /etc/hosts with no bound, as root, so `yes | sudo y-etc-hosts-write` could fill the root filesystem including its root reserve, before any check ran. A valid 46 MB block of comments was also accepted as /etc/hosts. Input is now limited to 1 MiB, far above what k8s-ingress-hosts writes. Verified with the PR head's probes rerun against this version: the shadowing input and the 46 MB input are refused (exit 2); route comments, same-address duplicates and a 40000-entry block are accepted; the PR's refusals (public address, edits outside the block) are unchanged. shellcheck and y-script-lint are clean. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
--write checked the owner of the two helpers and of bin/ only. On a noble VM (classic sudo 1.9.15p5) a grantee got root without a password in three ways the owner check does not see, all with a root-owned checkout: a helper group-writable by the grantee's group, bin/ group-writable (the grantee exchanged a helper with its own file between the check and the install, renameat2 RENAME_EXCHANGE, 1 in 13 runs), and a root-created symlink to a file the grantee owns. A grantee-owned directory above the checkout let it swap the whole tree during --write. Report and logs: bots-state/code-review/prs/ystack-85/vm/ on metalblack. The check now walks the helpers, this script and every directory above them (on the physical path, pwd -P), and refuses when a grantee owns one, can write one (as root: sudo -u <grantee> test -w, so group, other and ACL grants count), or one is a symlink. With nothing on the path writable by a grantee, nothing can change between the check and the copy. The script cannot vouch for itself, which the usage now says: a grantee who could write it before the admin runs it runs as root, so the admin checks with the show mode first, as root, which reports the same. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
6aef3cb refused a block entry that gives a name from outside the block another address, comparing names byte for byte. Host lookups ignore case, so "127.0.0.9 DB.INTERNAL" in the block was accepted and won over root's "10.1.1.1 db.internal": getent hosts and getaddrinfo both returned 127.0.0.9 first, and ping went there, whether the block came first or last (VM run, bots-state/code-review/prs/ystack-85/vm-fix/ 09b-case-bypass.log on metalblack). Both sides are now lowercased. Verified on scratch targets: DB.INTERNAL at the top and Db.Internal at the end refused (exit 2); "10.1.1.1 DB.internal" and "127.0.0.1 LOCALHOST" (same addresses as outside) and a route-comment block accepted. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ee9bc6a's usage told the admin to check with the show mode as root first. On the VM a grantee who could write bin/ replaced this script, and that advice ran the grantee's line as root before any report (prs/ystack-85/vm-fix/07-show-as-root.log on metalblack). The script cannot check itself, so the usage now says: show mode as yourself for what a grantee owns, and write access checked without executing the checkout (ls -ld on the path) before sudo. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Merged
LLM-review sudoers setup
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
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.
the script was a hack from a fully contained vm, now needs proper security review