fix(auth): install the Docker credential shim into a writable directory
What does this MR do and why?
glab auth configure-docker fails for every non-root user when glab comes
from a system package:
ERROR
Writing docker-credential-glab shim: open /usr/bin/.docker-credential-glab4522999119351626827: permission denied.dockercredhelper.Install pinned the docker-credential-glab shim to the
directory holding the glab binary, which the .deb and .rpm packages put in
the root-owned /usr/bin. The odd path in the error is
renameio's pending file,
staged in the target directory before the atomic rename, not a path glab chose.
This is not Debian-specific. It breaks any install that lands glab somewhere
the invoking user cannot write: the .deb and .rpm packages, a
sudo-installed binary in /usr/local/bin, and a read-only Nix store.
Co-locating the shim was never a requirement. The shim is a two-line script
that re-resolves glab from PATH at run time:
#!/bin/sh -eu
glab auth docker-helper "$@"So the one hard constraint is a directory on PATH, because that is how
Docker resolves a credential helper. The old code satisfied only half of that
("on PATH") and treated the two as one requirement.
Install now tries, in order:
- glab's own directory, which is on
PATHby construction and keeps an existing writable install idempotent rather than orphaning its shim behind a second copy. ~/.local/bin, created if needed, whether or not it is onPATH. It belongs to this user, where/usr/local/binand/opt/homebrew/binare machine-wide and commonly group-writable. When it is offPATHit is the one candidate Docker cannot resolve as it stands, so both commands warn with the directory to add.- Any other writable
PATHentry, inPATHorder. Reached only when the home directory cannot be resolved.
The second and third ranks were the other way round until viktomas pointed out
what the 0700 shim mode does on a shared host: a copy in a group-writable
/usr/local/bin is executable only by whoever installed it, so a second user's
Docker finds it on PATH and fails with permission denied instead of falling
through, and their own configure-docker then replaces it with a copy the
first user cannot run. A warning the user has to act on beats two users
breaking each other.
Writability is settled by attempting the write, not by inspecting mode bits.
That avoids TOCTOU and the cases the bits get wrong: ACLs, read-only mounts,
and running as root. An error meaning something other than "this directory will
not do" (a full disk, an I/O error) fails immediately rather than being retried
down the whole of PATH, where it would bury the real cause under a list of
directories.
Install returns an Installation{Path, OnPath} instead of a path so the
last-resort case is reportable by the caller.
Two related items fixed here
- A stale comment in
artifactregistry/login. The--dockerpreflight claimedsupportedOSandLocatewere "everythingInstallcan reject before its first write". That was already untrue for this bug: both preflights pass on a.debinstall,runexchanges a live token, and then the write fails. The comment now says what is and is not preflighted, and why attempting the write invalidatewould be worse (it would leave a shim behind on a login whose exchange then fails). configure-dockerhelp text now says where the script is installed.
How the commits are arranged
Two commits, red then green, since the failure is the substance of the report:
-
57cdbafe5adds the tests. On that commit all five fail with the reported error, at both thedockercredhelperand theglab auth configure-dockerlayer:--- FAIL: TestInstall_FallsBackToLocalBinOnPath --- FAIL: TestInstall_FallsBackToAWritablePathEntry --- FAIL: TestInstall_CreatesLocalBinWhenNothingOnPathIsWritable --- FAIL: TestInstall_NoWritableDirectoryAnywhere --- FAIL: TestConfigureDocker_SucceedsWithAPackagedGlab writing docker-credential-glab shim: open /.../001/.docker-credential-glab6372986206671340153: permission denied -
a33ab7b6bis the fix, and turns them green.
caaf1677e is the review round: see the resolved threads for the shared-host
ordering above, the raw-PATH dedup bug (/usr/bin and /usr/bin/ produced
two candidates for one directory), and the help text.
Test coverage, and the CI-as-root trap
The first two commits gated every new test on chmod 0o555, which root
ignores, and tests:unit runs in the golang image as root. So all five
skipped in CI: 94.1% locally against 84.3% in the pipeline, with the gap being
candidates/unusable, the new logic. Caught by viktomas.
candidates, unusable, writeShim and PathWarning are now tested
directly, which needs no unwritable directory: the first is pure, and
writeShim's create path uses the blocker-file trick already in
internal/oauth2's unwritableDir, where a regular file standing in for a
parent directory makes the write fail with ENOTDIR that root cannot bypass.
Those four are at 100% in the root-immune subset, which brings the package to
90.3% under CI conditions and 94.6% locally.
Still skipped as root: the chmod end-to-end Install tests, and the two
command-layer warning tests. Exercising those needs the first candidate's
write to fail, which needs glab's own directory to be unwritable, and the
ENOTDIR trick cannot do that because glab has to live in a real directory.
Why this was never caught
Every existing test points PATH at a t.TempDir(), which is always writable.
There was no coverage for an unwritable directory.
Verification
lefthook run pre-push passes: build, go-lint (0 issues), check-generated,
markdownlint, vale, lychee. Full make test is green at 5226 tests.
Also checked end to end with the real built binary, copied into a chmod 555
directory to stand in for a packaged /usr/bin, with env -i for a clean
environment:
~/.local/binonPATH: the shim lands there at mode0700,credHelpersis written for all three gitlab.com domains, and no warning is printed.~/.local/binnot onPATH: the directory is created, the shim lands there, the command exits 0, and it warnsdocker-credential-glab was installed to .../.local/bin, which is not on your PATH.~/.local/binoffPATHwith a writable shared directory onPATH: the shared directory is left untouched and the shim still goes to~/.local/binwith the warning. This is the P2 ordering, confirmed against the real binary rather than only in tests.
Not tested against a real docker pull, which needs a live registry
credential.
Closes #8541 (closed)