useruid: portable shell rewrite (Alpine + Debian)#34
Merged
Conversation
Replaces the usermod/groupmod/getent dependency chain with a script that mutates /etc/passwd and /etc/group directly via awk + sed, chowns \$HOME, and works on BusyBox/Alpine the same as on shadow-utils Debian. Drops the previous \"Alpine: skip with warning\" guard since the script no longer cares which toolchain shipped in the base image. Limitations: doesn't touch /etc/shadow or /etc/gshadow. Those key by username, not UID, so password-based login still works — only su(1) semantics on hardened images would be affected, and no real devcontainer flow depends on that. Integration coverage: - Parametrized test (debian + alpine) builds a base image with vscode at a fixed UID, runs Up against a workspace owned by the test runner UID, and asserts id -u vscode inside the container == host UID. Linux-only (macOS Docker Desktop remaps ownership). - Negative path: updateRemoteUserUID=false leaves UID at the image default. Closes #29 §2 (Alpine portability) and §4 (integration coverage). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughReplaces inline Dockerfile UID edits with a standalone uid-fix.sh (awk/sed-based) run during image build, gates reconciliation to Linux hosts, updates Dockerfile generation to COPY and execute the script, and adds unit and Linux-only integration tests for Debian and Alpine and a disabled path. ChangesUID Reconciliation Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
useruid.go (1)
39-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip UID reconciliation on Darwin too.
The new integration coverage explicitly treats Linux as the only host where the workspace owner UID matches what the container bind mount sees. Leaving
darwinenabled here can still rebuild the image to the macOS host UID/GID even though Docker Desktop remaps ownership differently, soupdateRemoteUserUIDbecomes incorrect instead of a harmless no-op.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@useruid.go` around lines 39 - 40, The OS check currently allows Darwin to proceed and can trigger updateRemoteUserUID incorrectly; change the runtime.GOOS guard in useruid.go so only Linux proceeds (i.e., replace the condition that checks != "linux" && != "darwin" with a single check for != "linux") so the function returns early on all non-Linux hosts and avoids rebuilding images or calling updateRemoteUserUID on Darwin/macOS; verify the code paths around finalImage and updateRemoteUserUID still behave as no-ops when the guard returns early.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/integration/uid_reconcile_test.go`:
- Around line 188-199: The test currently asserts on res.Stdout without
verifying the command succeeded; update the negative-path block after eng.Exec
(the call that runs "id -u vscode" with devcontainer.ExecOptions and returns
res) to first check res.ExitCode (and err) and fail fast with a clear t.Fatalf
including res.ExitCode and res.Stderr when the exit code is non-zero, then trim
and compare res.Stdout to strconv.Itoa(imageUID) only after confirming success —
mirror the positive-path check pattern used elsewhere so failures report
stderr/exit status.
In `@useruid.go`:
- Around line 173-199: The current script uses interpolated sed regexes and
literal comparisons that mis-handle usernames like "1000" or "vscode:users" and
will break on names with regex metacharacters; normalize and validate
_REMOTE_USER (reject or canonicalize forms containing ':' or all-digits) and
replace the sed-based rewrites with field-aware awk edits: use awk to locate and
print/modify the matching /etc/passwd row (matching $1==user) and the /etc/group
row (matching $3==gid or $1==old_group) to change only the UID/GID fields, or if
you must use sed, properly escape _REMOTE_USER and OLD_GROUP_NAME before
interpolation; update the places referencing _REMOTE_USER, CUR_UID, CUR_GID,
OLD_GROUP_NAME and the two sed invocations to use the safe awk-based
replacements and add tests for numeric and "user:group" inputs.
---
Outside diff comments:
In `@useruid.go`:
- Around line 39-40: The OS check currently allows Darwin to proceed and can
trigger updateRemoteUserUID incorrectly; change the runtime.GOOS guard in
useruid.go so only Linux proceeds (i.e., replace the condition that checks !=
"linux" && != "darwin" with a single check for != "linux") so the function
returns early on all non-Linux hosts and avoids rebuilding images or calling
updateRemoteUserUID on Darwin/macOS; verify the code paths around finalImage and
updateRemoteUserUID still behave as no-ops when the guard returns early.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc2c3390-bcd2-4d55-99b9-36cb34df365a
📒 Files selected for processing (3)
test/integration/uid_reconcile_test.gouseruid.gouseruid_test.go
- Skip UID reconciliation on Darwin: macOS Docker Desktop remaps bind-mount ownership in the VM, so reconciling to the host UID is wrong, not a no-op. Linux-only matches the integration test guard. - Replace interpolated sed regexes with field-aware awk rewrites for /etc/passwd and /etc/group. Avoids silent skips and wrong-row rewrites when remoteUser contains regex metacharacters or unusual forms (e.g. "foo.bar"). rewrite_file preserves inode/owner/mode. - Negative-path integration test now asserts ExitCode == 0 before comparing stdout, so a failed `id -u vscode` no longer disguises itself as a UID mismatch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
useruid.go (1)
212-221: 💤 Low valueConsider defensive check for pipeline failures.
POSIX
set -edoes not catch failures in non-last pipeline commands. Ifawkwere to fail or produce empty output,rewrite_filewould proceed and could truncate/etc/passwdor/etc/group.The risk is low in practice (these awk programs are simple and awk is reliable), and
set -o pipefailisn't portable to BusyBox ash. A defensive approach would validate the temp file isn't empty before overwriting:🛡️ Optional: add empty-file guard in rewrite_file
rewrite_file() { # $1 = path. Reads stdin, writes back to $1 in place. tmp=$1.dc-uid-tmp cat > "$tmp" + if [ ! -s "$tmp" ]; then + echo "updateRemoteUserUID: rewrite produced empty output for $1; aborting" >&2 + rm "$tmp" + exit 1 + fi cat "$tmp" > "$1" rm "$tmp" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@useruid.go` around lines 212 - 221, The awk pipelines that rewrite /etc/group and /etc/passwd can produce empty output on failure and then feed rewrite_file, so add a defensive check: after each awk pipeline (the two blocks that call rewrite_file) verify the generated temporary file is non-empty and valid before letting rewrite_file replace the target; alternatively harden the rewrite_file function to abort if its input file is empty or below a minimal size/line count and return a non-zero exit code so the script fails instead of truncating /etc/passwd or /etc/group. Reference the awk blocks operating on /etc/group and /etc/passwd and the rewrite_file helper when applying this guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@useruid.go`:
- Around line 212-221: The awk pipelines that rewrite /etc/group and /etc/passwd
can produce empty output on failure and then feed rewrite_file, so add a
defensive check: after each awk pipeline (the two blocks that call rewrite_file)
verify the generated temporary file is non-empty and valid before letting
rewrite_file replace the target; alternatively harden the rewrite_file function
to abort if its input file is empty or below a minimal size/line count and
return a non-zero exit code so the script fails instead of truncating
/etc/passwd or /etc/group. Reference the awk blocks operating on /etc/group and
/etc/passwd and the rewrite_file helper when applying this guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d19d17d-b6c5-4f6f-bcf7-d33081674a04
📒 Files selected for processing (3)
test/integration/uid_reconcile_test.gouseruid.gouseruid_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/integration/uid_reconcile_test.go
- useruid_test.go
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.
Summary
Drops the `usermod`/`groupmod`/`getent` dependency chain in favor of a portable script that mutates `/etc/passwd` and `/etc/group` directly via `awk` + `sed`. Works on BusyBox/Alpine the same as on shadow-utils Debian, so the Alpine "skip with warning" guard from #30 goes away.
Closes #29 §2 (Alpine portability) and §4 (integration coverage).
Approach
The reconciliation step is now: write a self-contained shell script (`uidReconcileScript`) into the build context, COPY it, and `/bin/sh` it. This keeps the Dockerfile minimal and the script readable — no escaping nightmares from embedding multi-line shell into a Dockerfile RUN.
Script logic:
Limitations
Test plan
Unit (`useruid_test.go`):
Integration (`test/integration/uid_reconcile_test.go`, Linux-only):
`TestUpdateRemoteUserUID_ReconcilesUIDOnDebianAndAlpine` — parametrized over both distros. Builds a base image with vscode at UID 12345, runs Up against a workspace owned by the test runner's UID (different by construction in CI), and asserts `id -u vscode` inside the container matches the host UID. Skips on macOS (Docker Desktop ownership remap) and when runner UID happens to match the baked UID.
`TestUpdateRemoteUserUID_FalseLeavesUIDUnchanged` — negative path: `updateRemoteUserUID: false` keeps the image-default UID.
`go test ./...`
`go vet ./...` (incl. `-tags=integration`)
`gofmt` clean
`go build -tags=integration ./test/integration/...`
CI cost
Adds two integration tests building a small derivative each — ~30s total for the matrix entries (Alpine pull is cached after the first run). Acceptable given we're now actually exercising the feature on the path consumers use.
Summary by CodeRabbit
Tests
Refactor