The verified-directory checks get one implementation, and the two remaining sites use it (refs #233) - #239
Merged
Merged
Conversation
… (refs #233) #233 asks for two more sites to be converted to *os.Root. Writing the first one showed that doing it site by site the obvious way would mean a THIRD copy of the same checks — mkdir-that-refuses-to-reuse, an Lstat refusal at the name being created, an ownership and mode check on the opened descriptor rather than on a path — and three copies of a security check is a rule written once and applied to one of its halves, which is the failure shape this project keeps naming. So the checks move to internal/vdir, unchanged in behaviour, and internal/cli calls them. internal/engine and prepareHostTmpDir follow in their own commits, which is the point of moving them first. Two copies were ALREADY drifting, which is the argument for this rather than a comment saying "keep these in sync": only internal/cli refused a symlink at the name it creates, and only internal/cli checked the handle instead of the path. The engine's version and prepareHostTmpDir's are each missing one of the two. The error sweep follows them. TestErrorsInTheOddEnvironmentPathsNameTheFix holds a per-file floor of fmt.Errorf sites, and moving the messages dropped runtimedir.go below its floor — the useful half of which is that lowering the floor alone would have left those messages swept by NOTHING. They are what a human meets when $XDG_RUNTIME_DIR or /tmp is in a state snug refuses, which is the odd-environment path #180 wrote that sweep for, so the map now names ../vdir/vdir.go and the parser follows it out of the package. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… string (refs #233) createRunDir hand-rolled what internal/vdir now does for everyone — mkdir-that-refuses-to-reuse, O_DIRECTORY|O_NOFOLLOW, fstat on the descriptor rather than a second walk. That code was right; what #233 is about is what happened NEXT. The descriptor was closed, the path became a string, and every later operation on the most exposed directory snug creates re-derived it by name: os.MkdirAll for the two halves of the C2b split, and os.RemoveAll at teardown. /tmp is why it matters more here than for $XDG_RUNTIME_DIR, and createRunDir's own comment said so: commonly world-writable and sticky, so a same-uid process on a shared host can plant an entry at a guessable path before snug gets there. runDirs now holds the run directory's handle AND its parent's for the life of the engine. sock/ and conf/ are created through the run handle; teardown removes through the parent handle. The test for that renames the parent first, which is the only way to tell the implementations apart: os.RemoveAll on a route that no longer leads here reports SUCCESS having removed nothing, so a path-based teardown would report clean while leaving this run's engine socket and generated config on disk. Teardown also stopped swallowing its error. `_ = os.RemoveAll(e.runDir)` said nothing when it failed; a directory snug could not clean up is state that survives the user, so it is named on stderr the way sweepStaleRunDirs already names one. TWO THINGS DELIBERATELY NOT CONVERTED, because the conversion would break what they are for: - the store and the runroot use MkdirAll and must keep it. They are keyed by profiles+target rather than by pid, and a warm start IS a second run finding the first run's store — first-writer-wins is the design, and the libpod database refuses a runroot disagreeing with the one recorded in it. The sharing question those two raise (who else can reach a store keyed by a hash) is real and is not this change. - the sock/ and conf/ handles are closed after verification rather than kept. Nothing writes into them through a Root today, and a descriptor held for a reader that does not exist is the exact shape #103 found in runStateRoot. The limit is stated at runDirs rather than left to be discovered: podman is a separate process taking --root and --runroot as argv, so those paths are handed over as strings and no descriptor can go there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…refs #233) prepareHostTmpDir was the weakest of the three copies of these checks and the only one with no test at all — which is presumably why it was the weakest. It Lstat'd a path, refused a symlink, checked owner and mode on that FileInfo, and returned a string, never holding a descriptor at any point. Every answer was about a name at a moment. It now creates through a vdir handle that cannot name anything outside $TMPDIR, refuses a symlink at exactly the name it is about to open, and checks owner and mode on the opened directory itself. Reuse stays legal here — the name is derived from the target so a build cache stays warm — so it calls SecureSubdir and ignores the created flag, which is the opposite of the engine's run directory and the reason the flag exists. One behaviour change, stated rather than buried: the old check refused a mode with ANY group or other bits, vdir requires exactly 0700. A 0500 or 0000 directory passed before and is refused now. Stricter direction, same rule the runtime directory has always had, and a shared /tmp snug cannot write to is not usable as one. THE LIMIT is inherent and is written at the call site: bwrap binds this directory BY PATH, in a process forked later, and no descriptor can be handed to it. The handle establishes that the directory snug created and checked is the one it opened — not that the name still leads there when bwrap walks it. Holding the descriptor open would not close that window either, because a rename of the NAME is what the window is. THE TESTS ARE WHERE THE INTERESTING PART IS. Two of the three new symlink tests were VACUOUS when first written, both for the same reason: they planted an ABSOLUTE symlink, which os.Root refuses on its own contract (the target leaves the root), so deleting snug's Lstat guard left them green. A RELATIVE in-root symlink is the case os.Root FOLLOWS and therefore the only case that tests our guard. Corrected, and the correction is written at each test. The engine's symlink test still cannot discriminate — MustCreateSubdir's no-reuse rule refuses a planted entry before the symlink guard is reached — so it now says so and points at the two tests that do (internal/vdir's own, and the shared-tmp one). Belt and braces is worth having; claiming a test proves the brace while the belt is holding is not. internal/vdir gets its own suite: the in-root symlink refusal, the mode refusal (and that it refuses rather than repairs), the created flag both ways, MustCreateSubdir's no-reuse, and that OpenExistingSubdir brings nothing into existence just by looking. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
fixes #233 . Three commits, one site each, as the ticket asks.
Why a shared package came first
#233 lists two sites to convert. Writing the first one showed that doing it the obvious way would produce a third copy of the same checks — mkdir-that-refuses-to-reuse, an
Lstatrefusal at the name being created, an owner/mode check on the opened descriptor rather than on a path. Three copies of a security check is a rule written once and applied to one of its halves.And two of them were already drifting, which is the argument against a "keep these in sync" comment: only
internal/clirefused a symlink at the name it creates, and onlyinternal/clichecked the handle instead of the path. The engine's copy andprepareHostTmpDir's were each missing one.So
internal/vdirholds them now — behaviour unchanged,internal/clicalls them, and the two new sites follow in their own commits.The three commits
1.
internal/vdir. The move, plusMustCreateSubdirfor the caller whose name is unique per run. The error sweep (TestErrorsInTheOddEnvironmentPathsNameTheFix) follows the messages out of the package rather than having its floor lowered — those are the errors a human meets when$XDG_RUNTIME_DIRor/tmpis in a state snug refuses, which is exactly the odd-environment path #180 wrote it for.2.
internal/engine.createRunDir's checks were right; what #233 is about is what happened next — the descriptor was closed, the path became a string, andos.MkdirAllandos.RemoveAllre-derived the most exposed directory snug creates by name.runDirsnow holds the run directory's handle and its parent's for the life of the engine;sock/andconf/are created through the run handle, teardown removes through the parent handle, and teardown's error is named on stderr instead of_ =-discarded.Deliberately not converted, with reasons at the site: the store and runroot keep
MkdirAll, because they are keyed by profiles+target rather than pid and a warm start is a second run finding the first run's store — first-writer-wins is the design. Thesock//conf/handles are closed after verification rather than kept, because nothing writes into them through aRoottoday and a descriptor held for a non-existent reader is the shape #103 found inrunStateRoot.3.
prepareHostTmpDir. The weakest copy and the only one with no test at all — presumably related. Now created and verified through a handle. Reuse stays legal here (the name is target-derived so a build cache stays warm), which is the opposite of the engine's rule and the reasonSecureSubdirreports acreatedflag. One behaviour change stated rather than buried: the old check refused any group/other bits,vdirrequires exactly0700, so a0500directory that passed before is refused now.The part worth reviewing
Two of the three new symlink tests were vacuous when first written, and the same mistake made both. They planted an absolute symlink — which
os.Rootrefuses on its own contract, because the target leaves the root — so deleting snug'sLstatguard left them green. The case that matters is a relative, in-root symlink: that is the oneos.Root's contract says it will follow, and the only one our guard is standing in front of. Corrected, with the reason written at each test.The engine's symlink test still cannot discriminate, because
MustCreateSubdir's no-reuse rule refuses a planted entry before the symlink guard is reached. Rather than dress it up, it now says which rule is doing the work and points at the two tests that do discriminate.Mutation checks: deleting the
Lstatrefusal, the mode check, the no-reuse rule, or revertingprepareHostTmpDirto its path-based checks each fails a test — and each failure was verified to be for the right reason.Limits stated, not covered
--root/--runrootas argv, so those paths are handed over as strings; no descriptor can go there./tmpdirectory by path, in a process forked later. The handle establishes that the directory snug created and checked is the one it opened — not that the name still leads there when bwrap walks it. Holding the descriptor would not close that window, because a rename of the name is the window.make gategreen;make integrationrunning, reported when it lands.🤖 Generated with Claude Code