Raised by the maintainer reviewing PR #100, where runtimeDir() had grown a private openat2(RESOLVE_NO_SYMLINKS) helper plus an ownership check — in the maintainer's words, "a partial implementation of *os.Root". That PR adopts os.Root inside that one function. This ticket is the part that PR deliberately does not do.
The real idea: let the type carry the guarantee
The maintainer's point was not only "call os.Root instead of hand-rolling the walk". It is that a verified directory and an unverified path should not be the same type.
Today, after all the verification work:
func runtimeDir() (string, error)
It opens the base, checks ownership and mode, refuses a symlink at each name it owns, opens the directory as an *os.Root, takes its lock — and then returns a string and closes the handle. Every caller starts again from a path:
dir, err := runtimeDir()
sock := filepath.Join(dir, "ssh-agent.sock") // identity.go
sock := filepath.Join(dir, "podman.sock") // container.go
At the return statement the guarantee is gone. What comes back is indistinguishable, to the compiler and to a reader, from any other string. The checks are real but they are checks about a moment, and nothing carries them forward.
Two shapes worth weighing:
func runtimeDir() (*os.Root, error)
type RuntimeDir struct{ root *os.Root }
func OpenRuntimeDir() (*RuntimeDir, error)The second is the stronger of the two and is what the maintainer proposed. *os.Root is already a real improvement — the handle stays open, so a caller cannot re-walk a path that has changed underneath it — but any *os.Root satisfies any function that wants one, so nothing stops the engine's root being passed where the runtime directory was meant. A named type makes "this is snug's runtime directory, already verified, already locked" a thing the compiler knows, and gives the invariants somewhere to live as methods rather than as comments at each call site:
- the lock descriptor's lifetime belongs to the value that owns the directory, instead of a package-level map keyed by path;
- "what may be created in here" becomes a small set of methods rather than a
filepath.Join at each caller;
- a function that needs the runtime directory says so in its signature.
The honest limit is the socket, and it is the reason this needs measuring rather than assuming. bind(2) has no *at variant, so the last step of both call sites is net.Listen("unix", <path string>) and a path string has to exist somewhere. The candidate answer is for the type to hand out /proc/self/fd/<fd>/<name>, so the bind resolves through the already-verified descriptor rather than re-walking the name — with the side benefit that it is short, which matters against sun_path's 108-byte limit. That is a hypothesis, not a measurement. Nobody has run it here. Whoever takes this must verify by execution that such a bind works, that the resulting socket is reachable by the sandbox through its bind mount, and what the peer sees as the socket's path — and must keep the ownership/mode check regardless, because a descriptor proves what a directory is, not who may write into it.
The background idea, unchanged
os.Root (Go 1.24, extended since; this repo builds with Go 1.26) is a directory handle whose methods cannot reference anything outside it: Open, OpenFile, Create, Mkdir, MkdirAll, Remove, RemoveAll, Stat, Lstat, Readlink, Rename, Link, Symlink, Chmod, Chown, ReadFile, WriteFile, OpenRoot and FS. On Linux it uses openat2 where available and falls back to a checked component-by-component walk where it is not — so a caller gets the guarantee without having to refuse to run on an older kernel, which a raw openat2 call has to do.
For a project whose whole subject is which paths are reachable, "the handle cannot name anything outside this directory" is a better primitive than a path string plus discipline.
Why a ticket rather than a sweep
1. os.Root follows symlinks that stay inside the root. That is its documented contract, and it is weaker than RESOLVE_NO_SYMLINKS at any component snug creates itself: an in-root symlink can still redirect one. PR #100's answer is to keep an Lstat refusal at exactly the two names it owns and to say so at the site. Each candidate site needs the same question asked, not the same answer assumed.
2. It cannot cover everything. See the socket above. os.Root narrows the window; it does not remove it.
3. It is a signature change across call sites. The type-level version touches every caller by definition, which is exactly why it is not being bolted onto a PR that is already carrying a rename.
Candidate sites, unaudited
Grep for the shapes rather than trusting this list: os.MkdirAll, os.RemoveAll, filepath.Join immediately followed by an open or a stat, and any place a path is validated and then used again by name. internal/engine (the per-engine socket directories and reap.go), the staging of generated files, and the identity/credential staging are the obvious places to start.
runtimeDir itself is the first site and the one this ticket is written around. Note that PR #100 also moves cmd/snug into internal/cli, so the paths in this list will have moved by the time anyone works on it.
Explicitly out of scope: internal/policy. It is pure by rule — no globals, no filesystem, no exec — and that is what lets the security-critical tests run in CI with no privileges. Nothing here changes that.
What would make this worth doing
Not aesthetics. The measure is whether a converted site can stop carrying a hand-written guard, or whether a site that has no guard today gets one for free. For the type-level change specifically, the measure is whether a wrong call becomes a compile error rather than a review comment — runLock's package-level map keyed by path, and the filepath.Join at each of the two socket call sites, are the two places to point at when judging that.
A conversion that leaves the same checks in place plus a new type is churn, and this repo has a rule against prose and code that drift; it should have one against that too.
Do it site by site, each with its own golden or test diff, never as one sweeping change.
refs #100
Raised by the maintainer reviewing PR #100, where
runtimeDir()had grown a privateopenat2(RESOLVE_NO_SYMLINKS)helper plus an ownership check — in the maintainer's words, "a partial implementation of*os.Root". That PR adoptsos.Rootinside that one function. This ticket is the part that PR deliberately does not do.The real idea: let the type carry the guarantee
The maintainer's point was not only "call
os.Rootinstead of hand-rolling the walk". It is that a verified directory and an unverified path should not be the same type.Today, after all the verification work:
It opens the base, checks ownership and mode, refuses a symlink at each name it owns, opens the directory as an
*os.Root, takes its lock — and then returns a string and closes the handle. Every caller starts again from a path:At the return statement the guarantee is gone. What comes back is indistinguishable, to the compiler and to a reader, from any other string. The checks are real but they are checks about a moment, and nothing carries them forward.
Two shapes worth weighing:
The second is the stronger of the two and is what the maintainer proposed.
*os.Rootis already a real improvement — the handle stays open, so a caller cannot re-walk a path that has changed underneath it — but any*os.Rootsatisfies any function that wants one, so nothing stops the engine's root being passed where the runtime directory was meant. A named type makes "this is snug's runtime directory, already verified, already locked" a thing the compiler knows, and gives the invariants somewhere to live as methods rather than as comments at each call site:filepath.Joinat each caller;The honest limit is the socket, and it is the reason this needs measuring rather than assuming.
bind(2)has no*atvariant, so the last step of both call sites isnet.Listen("unix", <path string>)and a path string has to exist somewhere. The candidate answer is for the type to hand out/proc/self/fd/<fd>/<name>, so the bind resolves through the already-verified descriptor rather than re-walking the name — with the side benefit that it is short, which matters againstsun_path's 108-byte limit. That is a hypothesis, not a measurement. Nobody has run it here. Whoever takes this must verify by execution that such a bind works, that the resulting socket is reachable by the sandbox through its bind mount, and what the peer sees as the socket's path — and must keep the ownership/mode check regardless, because a descriptor proves what a directory is, not who may write into it.The background idea, unchanged
os.Root(Go 1.24, extended since; this repo builds with Go 1.26) is a directory handle whose methods cannot reference anything outside it:Open,OpenFile,Create,Mkdir,MkdirAll,Remove,RemoveAll,Stat,Lstat,Readlink,Rename,Link,Symlink,Chmod,Chown,ReadFile,WriteFile,OpenRootandFS. On Linux it usesopenat2where available and falls back to a checked component-by-component walk where it is not — so a caller gets the guarantee without having to refuse to run on an older kernel, which a rawopenat2call has to do.For a project whose whole subject is which paths are reachable, "the handle cannot name anything outside this directory" is a better primitive than a path string plus discipline.
Why a ticket rather than a sweep
1.
os.Rootfollows symlinks that stay inside the root. That is its documented contract, and it is weaker thanRESOLVE_NO_SYMLINKSat any component snug creates itself: an in-root symlink can still redirect one. PR #100's answer is to keep anLstatrefusal at exactly the two names it owns and to say so at the site. Each candidate site needs the same question asked, not the same answer assumed.2. It cannot cover everything. See the socket above.
os.Rootnarrows the window; it does not remove it.3. It is a signature change across call sites. The type-level version touches every caller by definition, which is exactly why it is not being bolted onto a PR that is already carrying a rename.
Candidate sites, unaudited
Grep for the shapes rather than trusting this list:
os.MkdirAll,os.RemoveAll,filepath.Joinimmediately followed by an open or a stat, and any place a path is validated and then used again by name.internal/engine(the per-engine socket directories andreap.go), the staging of generated files, and the identity/credential staging are the obvious places to start.runtimeDiritself is the first site and the one this ticket is written around. Note that PR #100 also movescmd/snugintointernal/cli, so the paths in this list will have moved by the time anyone works on it.Explicitly out of scope:
internal/policy. It is pure by rule — no globals, no filesystem, noexec— and that is what lets the security-critical tests run in CI with no privileges. Nothing here changes that.What would make this worth doing
Not aesthetics. The measure is whether a converted site can stop carrying a hand-written guard, or whether a site that has no guard today gets one for free. For the type-level change specifically, the measure is whether a wrong call becomes a compile error rather than a review comment —
runLock's package-level map keyed by path, and thefilepath.Joinat each of the two socket call sites, are the two places to point at when judging that.A conversion that leaves the same checks in place plus a new type is churn, and this repo has a rule against prose and code that drift; it should have one against that too.
Do it site by site, each with its own golden or test diff, never as one sweeping change.
refs #100