diff --git a/cmd/snapshot/create.go b/cmd/snapshot/create.go index b16410303..e0faf09d0 100644 --- a/cmd/snapshot/create.go +++ b/cmd/snapshot/create.go @@ -120,6 +120,7 @@ func (cmd *CreateCmd) Run(ctx context.Context, devsyConfig *config.Config, args MountPrefix: vols.MountPrefix, RunArgs: vols.RunArgs, ContainerEnv: vols.ContainerEnv, + RemoteUser: vols.RemoteUser, ContainerImageMediaType: img.MediaType, ContainerImageDigest: img.Digest, ContainerImageSize: img.Size, @@ -301,6 +302,7 @@ type pushedVolumes struct { MountPrefix string RunArgs []string ContainerEnv map[string]string + RemoteUser string } // The volumes RPC (StreamSnapshotVolumes) is served by a tunnelServer reading @@ -341,6 +343,7 @@ func (cmd *CreateCmd) pushVolumes( MountPrefix: mountPrefix, RunArgs: result.MergedConfig.RunArgs, ContainerEnv: redactedContainerEnv(result.MergedConfig.ContainerEnv), + RemoteUser: result.MergedConfig.RemoteUser, }, nil } diff --git a/cmd/snapshot/restore.go b/cmd/snapshot/restore.go index 730748e44..f633a9ab7 100644 --- a/cmd/snapshot/restore.go +++ b/cmd/snapshot/restore.go @@ -94,6 +94,7 @@ func (cmd *RestoreCmd) Run( if err != nil { return fmt.Errorf("read snapshot container env: %w", err) } + remoteUser := manifest.RemoteUser() log.Infof("restoring snapshot: ref=%s workspaceId=%s", snapshotRef, ws.ID) @@ -105,6 +106,7 @@ func (cmd *RestoreCmd) Run( DevContainerSource: ws.DevContainerSource, RunArgs: runArgs, ContainerEnv: containerEnv, + RemoteUser: remoteUser, }) } diff --git a/cmd/workspace/up/up.go b/cmd/workspace/up/up.go index 8af1211ac..53845ef6b 100644 --- a/cmd/workspace/up/up.go +++ b/cmd/workspace/up/up.go @@ -86,6 +86,9 @@ type Options struct { // the same suppressed-discovery circumstances as RunArgs. Used by // snapshot restore to replay the original devcontainer.json's containerEnv. ContainerEnv map[string]string + // RemoteUser is the remoteUser to replay under the same + // suppressed-discovery circumstances as RunArgs. Used by snapshot restore. + RemoteUser string } type HeadlessOptions struct { @@ -202,6 +205,7 @@ func buildUpCmd(g *flags.GlobalFlags, opts Options) *UpCmd { cmd.DevContainerSource = opts.DevContainerSource cmd.RunArgs = opts.RunArgs cmd.ContainerEnv = opts.ContainerEnv + cmd.RemoteUser = opts.RemoteUser if opts.Name != "" { cmd.ID = opts.Name } diff --git a/cmd/workspace/up/up_client.go b/cmd/workspace/up/up_client.go index 4f9fd4727..f382f3e6a 100644 --- a/cmd/workspace/up/up_client.go +++ b/cmd/workspace/up/up_client.go @@ -557,8 +557,9 @@ func (cmd *UpCmd) validateFromSnapshot(ctx context.Context, args []string) error } // applyFromSnapshotOverrides replays the create-time devcontainer.json -// settings the snapshot's manifest carries (runArgs, containerEnv) onto cmd, -// so the image-sourced restored container behaves like the original did. +// settings the snapshot's manifest carries (runArgs, containerEnv, +// remoteUser) onto cmd, so the image-sourced restored container behaves like +// the original did. func (cmd *UpCmd) applyFromSnapshotOverrides(manifest *snapshotpkg.Manifest) error { runArgs, err := manifest.RunArgs() if err != nil { @@ -571,6 +572,8 @@ func (cmd *UpCmd) applyFromSnapshotOverrides(manifest *snapshotpkg.Manifest) err return fmt.Errorf("read --from-snapshot container env: %w", err) } cmd.ContainerEnv = containerEnv + + cmd.RemoteUser = manifest.RemoteUser() return nil } diff --git a/e2e/tests/snapshot/snapshot.go b/e2e/tests/snapshot/snapshot.go index 98141fc8c..9a9ccacc9 100644 --- a/e2e/tests/snapshot/snapshot.go +++ b/e2e/tests/snapshot/snapshot.go @@ -21,6 +21,7 @@ const ( snapshotCmd = "snapshot" snapshotVerbCreate = "create" snapshotVerbRestore = "restore" + nonRootRemoteUser = "devsyuser" ) var _ = ginkgo.Describe("devsy snapshot", ginkgo.Label("snapshot"), func() { @@ -430,11 +431,6 @@ var _ = ginkgo.Describe("devsy snapshot", ginkgo.Label("snapshot"), func() { restoredWorkspace, err := f.FindWorkspace(ctx, restoredID) framework.ExpectNoError(err) - // The custom --label runArg only exists in this fixture's - // devcontainer.json, not in the base image or --add-host (which the - // registry fixture itself already depends on to function at all): its - // presence on the restored container proves restore replays the - // original runArgs generally, not just the one the test harness needs. containerIDs, err := dockerHelper.FindContainer(ctx, []string{ fmt.Sprintf("%s=%s", pkgconfig.DevcontainerIDLabel, restoredWorkspace.UID), "devsy-e2e-snapshot-runargs=true", @@ -445,4 +441,60 @@ var _ = ginkgo.Describe("devsy snapshot", ginkgo.Label("snapshot"), func() { "restored container should carry the original devcontainer.json's custom runArg label", ) }, ginkgo.SpecTimeout(framework.TimeoutLong())) + + ginkgo.It("restores files owned by the remote user when reusing the original id", func( + ctx context.Context, + ) { + initialDir, err := os.Getwd() + framework.ExpectNoError(err) + + tempDir, err := framework.CopyToTempDir("tests/snapshot/testdata/docker-nonroot") + framework.ExpectNoError(err) + ginkgo.DeferCleanup(framework.CleanupTempDir, initialDir, tempDir) + ginkgo.DeferCleanup(f.DevsyWorkspaceDelete, tempDir) + framework.ExpectNoError(f.DevsyUp(ctx, tempDir)) + + workspaceFolder, err := f.DevsySSH(ctx, tempDir, "pwd") + framework.ExpectNoError(err) + workspaceFolder = strings.TrimSpace(workspaceFolder) + + markerCmd := fmt.Sprintf("echo mutated > %s/marker.txt", workspaceFolder) + _, err = f.DevsySSH(ctx, tempDir, markerCmd) + framework.ExpectNoError(err) + + out, _, err := f.ExecCommandCapture(ctx, []string{ + snapshotCmd, snapshotVerbCreate, tempDir, registryFlag, registryHost + "/e2e/snapshots", + debugFlag, + }) + framework.ExpectNoError(err) + snapshotRef := strings.TrimSpace(out) + + framework.ExpectNoError(f.DevsyWorkspaceDelete(ctx, tempDir)) + + _, _, err = f.ExecCommandCapture(ctx, []string{ + snapshotCmd, snapshotVerbRestore, snapshotRef, debugFlag, + }) + framework.ExpectNoError(err) + + restoredWorkspaceFolder, err := f.DevsySSH(ctx, tempDir, "pwd") + framework.ExpectNoError(err) + restoredWorkspaceFolder = strings.TrimSpace(restoredWorkspaceFolder) + + content, err := f.DevsySSH( + ctx, tempDir, fmt.Sprintf("cat %s/marker.txt", restoredWorkspaceFolder), + ) + framework.ExpectNoError(err) + gomega.Expect(content).To(gomega.ContainSubstring("mutated")) + + // Compare the recorded owner name against the devcontainer.json's + // remoteUser rather than the SSH session's uid: the ssh session may + // resolve to root when no ssh-config entry was written + ownerCmd := fmt.Sprintf( + `test "$(stat -c %%U %s/marker.txt)" = %q && echo OWNER_OK || echo OWNER_MISMATCH`, + restoredWorkspaceFolder, nonRootRemoteUser, + ) + ownerOut, err := f.DevsySSH(ctx, tempDir, ownerCmd) + framework.ExpectNoError(err) + gomega.Expect(ownerOut).To(gomega.ContainSubstring("OWNER_OK")) + }, ginkgo.SpecTimeout(framework.TimeoutLong())) }) diff --git a/e2e/tests/snapshot/testdata/docker-nonroot/.devcontainer.json b/e2e/tests/snapshot/testdata/docker-nonroot/.devcontainer.json new file mode 100644 index 000000000..56822b418 --- /dev/null +++ b/e2e/tests/snapshot/testdata/docker-nonroot/.devcontainer.json @@ -0,0 +1,11 @@ +{ + "name": "snapshot non-root", + "build": { + "dockerfile": "Dockerfile" + }, + "remoteUser": "devsyuser", + "runArgs": ["--add-host=host.docker.internal:host-gateway"], + "containerEnv": { + "DEVSY_INSECURE_DOCKER_INTERNAL": "true" + } +} diff --git a/e2e/tests/snapshot/testdata/docker-nonroot/Dockerfile b/e2e/tests/snapshot/testdata/docker-nonroot/Dockerfile new file mode 100644 index 000000000..cd189e76a --- /dev/null +++ b/e2e/tests/snapshot/testdata/docker-nonroot/Dockerfile @@ -0,0 +1,3 @@ +FROM ghcr.io/devsy-org/test-images/base:ubuntu + +RUN useradd --create-home --shell /bin/bash devsyuser diff --git a/pkg/agent/snapshot/restore.go b/pkg/agent/snapshot/restore.go index 30cfc824b..2e2ceaa79 100644 --- a/pkg/agent/snapshot/restore.go +++ b/pkg/agent/snapshot/restore.go @@ -70,7 +70,10 @@ func RestoreVolumes( } levels := len(strings.Split(layer.MountPrefix, "/")) - if err := extract.Extract(rc, target, extract.StripLevels(levels)); err != nil { + if err := extract.Extract( + rc, target, + extract.StripLevels(levels), extract.PreserveHeaderOwnership(), + ); err != nil { return fmt.Errorf("extract snapshot volumes into %s: %w", target, err) } return nil diff --git a/pkg/copy/copy.go b/pkg/copy/copy.go index 690dc4c0e..2d3e0a463 100644 --- a/pkg/copy/copy.go +++ b/pkg/copy/copy.go @@ -1,7 +1,6 @@ package copy import ( - "errors" "fmt" "io" "io/fs" @@ -28,6 +27,45 @@ func Chown(path string, userName string) error { return os.Lchown(path, uidInt, gidInt) } +// ChownFailure is one entry a recursive chown could not reassign. +type ChownFailure struct { + Path string + Err error +} + +func (f ChownFailure) Error() string { return fmt.Sprintf("%s: %v", f.Path, f.Err) } + +func (f ChownFailure) Unwrap() error { return f.Err } + +// ChownFailures aggregates the entries ChownR could not chown. Callers +// distinguish wholesale breakage from entries a shared filesystem refuses to +// reassign via AllDenied. +type ChownFailures []ChownFailure + +func (fs ChownFailures) Error() string { + return fmt.Sprintf("%d entries could not be chowned, first: %v", len(fs), fs[0]) +} + +func (fs ChownFailures) Unwrap() []error { + errs := make([]error, len(fs)) + for i, f := range fs { + errs[i] = f + } + return errs +} + +// AllDenied reports whether every failure was refused by the filesystem +// (permission denied or read-only share) — the expected case for entries on +// virtiofs shares such as read-only .git pack files. +func (fs ChownFailures) AllDenied() bool { + for _, f := range fs { + if !deniedByFilesystem(f.Err) { + return false + } + } + return len(fs) > 0 +} + func ChownR(path string, userName string) error { if userName == "" { return nil @@ -44,28 +82,30 @@ func ChownR(path string, userName string) error { // #nosec G115 -- a resolved system uid is non-negative and fits uint32. uidU32 := uint32(uidInt) - // A single un-chownable entry (e.g. a read-only file on a virtiofs share) - // must not abort the walk and leave the rest of the tree unowned. - var errs []error + var failures ChownFailures _ = filepath.WalkDir(path, func(name string, dirEntry fs.DirEntry, err error) error { if err != nil { - errs = append(errs, err) + failures = append(failures, ChownFailure{Path: name, Err: err}) return nil } info, err := dirEntry.Info() if err != nil { + failures = append(failures, ChownFailure{Path: name, Err: err}) return nil } if IsUID(info, uidU32) { return nil } // #nosec G122 -- best-effort chown of a freshly provisioned tree we own; WalkDir yields real paths. - if err := os.Lchown(name, uidInt, gidInt); err != nil { - errs = append(errs, err) + if lerr := os.Lchown(name, uidInt, gidInt); lerr != nil { + failures = append(failures, ChownFailure{Path: name, Err: lerr}) } return nil }) - return errors.Join(errs...) + if len(failures) == 0 { + return nil + } + return failures } func MkdirAllChown(path string, perm os.FileMode, userName string) error { diff --git a/pkg/copy/copy_supported.go b/pkg/copy/copy_supported.go index 65f5a0cbb..abc942e36 100644 --- a/pkg/copy/copy_supported.go +++ b/pkg/copy/copy_supported.go @@ -3,6 +3,7 @@ package copy import ( + "errors" "fmt" "os" "syscall" @@ -13,6 +14,12 @@ func IsUID(info os.FileInfo, uid uint32) bool { return ok && stat.Uid == uid } +// deniedByFilesystem reports whether err means the filesystem refused the +// reassignment (insufficient privilege or a read-only share). +func deniedByFilesystem(err error) bool { + return errors.Is(err, os.ErrPermission) || errors.Is(err, syscall.EROFS) +} + func Lchown(info os.FileInfo, sourcePath, destPath string) error { stat, ok := info.Sys().(*syscall.Stat_t) if !ok { diff --git a/pkg/copy/copy_test.go b/pkg/copy/copy_test.go index dc0b8c4f2..7da146cef 100644 --- a/pkg/copy/copy_test.go +++ b/pkg/copy/copy_test.go @@ -3,6 +3,7 @@ package copy import ( + "errors" "os" "os/user" "path/filepath" @@ -231,3 +232,42 @@ func mustReadFile(t *testing.T, path string) []byte { } return b } + +// Chowning a file to a different owner requires privileges, so pointing +// ChownR at root as an unprivileged user exercises the denied-failure path +// deterministically. +func TestChownRDeniedFailuresAreTyped(t *testing.T) { + root := t.TempDir() + file := filepath.Join(root, "f.txt") + //nolint:gosec // G306 — test temp file + if err := os.WriteFile(file, []byte("x"), 0o600); err != nil { + t.Fatalf("write: %v", err) + } + + err := ChownR(root, "root") + var failures ChownFailures + if !errors.As(err, &failures) { + t.Fatalf("ChownR err = %v, want ChownFailures", err) + } + if !failures.AllDenied() { + t.Errorf("AllDenied() = false for %v", failures) + } + for _, f := range failures { + if !deniedByFilesystem(f.Err) { + t.Errorf("%s: unexpected cause %v", f.Path, f.Err) + } + } +} + +func TestChownRSameOwnerSucceeds(t *testing.T) { + root := t.TempDir() + file := filepath.Join(root, "f.txt") + //nolint:gosec // G306 — test temp file + if err := os.WriteFile(file, []byte("x"), 0o600); err != nil { + t.Fatalf("write: %v", err) + } + + if err := ChownR(root, currentUserName(t)); err != nil { + t.Fatalf("ChownR same owner: %v", err) + } +} diff --git a/pkg/copy/copy_unsupported.go b/pkg/copy/copy_unsupported.go index 2604b8378..76c79afdf 100644 --- a/pkg/copy/copy_unsupported.go +++ b/pkg/copy/copy_unsupported.go @@ -3,13 +3,23 @@ package copy import ( + "errors" "os" + "syscall" ) func IsUID(info os.FileInfo, uid uint32) bool { return true } +// deniedByFilesystem reports whether the platform refused the reassignment. +// IsUID short-circuits ChownR on Windows so this is rarely consulted, but a +// direct os.Lchown fails with EWINDOWS: that is "unsupported here", a +// tolerated denial, not a hard failure. +func deniedByFilesystem(err error) bool { + return errors.Is(err, syscall.EWINDOWS) +} + func Lchown(info os.FileInfo, sourcePath, destPath string) error { return nil } diff --git a/pkg/devcontainer/config.go b/pkg/devcontainer/config.go index aa333adf5..19b004b41 100644 --- a/pkg/devcontainer/config.go +++ b/pkg/devcontainer/config.go @@ -186,6 +186,9 @@ func (r *runner) rawConfigFromSource( log.Infof("ignoring project devcontainer, using image %s", spec.Image) return r.saveSynthesizedConfig(&config.DevContainerConfig{ ImageContainer: config.ImageContainer{Image: spec.Image}, + DevContainerConfigBase: config.DevContainerConfigBase{ + RemoteUser: options.RemoteUser, + }, NonComposeBase: config.NonComposeBase{ RunArgs: options.RunArgs, ContainerEnv: options.ContainerEnv, diff --git a/pkg/devcontainer/config/result.go b/pkg/devcontainer/config/result.go index 8c97beb89..d1000aec6 100644 --- a/pkg/devcontainer/config/result.go +++ b/pkg/devcontainer/config/result.go @@ -69,33 +69,48 @@ func GetContainerID(result *Result) string { return "" } -// GetRemoteUser determines the remote user using DevContainer specification priority order: -// 1. remoteUser from configuration -// 2. devsy.user label from container -// 3. User field from Docker inspect -// 4. containerUser from configuration -// -// Per DevContainer specification (https://containers.dev/implementors/json_reference/): -// "remoteUser: Overrides the user that devcontainer.json supporting services tools / runs as in the container... -// Defaults to the user the container as a whole is running as (often root).". +// GetRemoteUser resolves the user tools and the IDE run as inside the +// container, in spec priority order: remoteUser, then containerUser +// (the spec's default is "the user the container as a whole is +// running as"), then the image-derived devsy.user label and Docker-inspect +// user. Falls back to root. func GetRemoteUser(result *Result) string { if result == nil { - return "root" + return remoteUserRoot } + if user := userFromConfig(result); user != "" { + return user + } + if user := userFromContainer(result); user != "" { + return user + } + return remoteUserRoot +} - if result.MergedConfig != nil && result.MergedConfig.RemoteUser != "" { +const remoteUserRoot = "root" + +// userFromConfig returns the config-declared users: remoteUser first, then +// containerUser, which overrides the image user the container starts with. +func userFromConfig(result *Result) string { + if result.MergedConfig == nil { + return "" + } + if result.MergedConfig.RemoteUser != "" { return result.MergedConfig.RemoteUser } + return result.MergedConfig.ContainerUser +} +// userFromContainer returns the container's effective image user: first the +// devsy.user label recorded at creation, then Docker inspect's User field. +func userFromContainer(result *Result) string { if userLabel := userFromContainerLabel(result); userLabel != "" { return userLabel } - - if result.MergedConfig != nil && result.MergedConfig.ContainerUser != "" { - return result.MergedConfig.ContainerUser + if result.ContainerDetails != nil { + return result.ContainerDetails.Config.User } - - return "root" + return "" } func userFromContainerLabel(result *Result) string { diff --git a/pkg/devcontainer/config/result_test.go b/pkg/devcontainer/config/result_test.go index 602646951..f7f99b36b 100644 --- a/pkg/devcontainer/config/result_test.go +++ b/pkg/devcontainer/config/result_test.go @@ -51,3 +51,108 @@ func TestResultErr(t *testing.T) { }) } } + +func TestGetRemoteUser(t *testing.T) { + for _, tt := range getRemoteUserCases() { + t.Run(tt.name, func(t *testing.T) { + if got := GetRemoteUser(tt.result); got != tt.want { + t.Errorf("GetRemoteUser() = %q, want %q", got, tt.want) + } + }) + } +} + +type getRemoteUserCase struct { + name string + result *Result + want string +} + +func getRemoteUserCases() []getRemoteUserCase { + return append(remoteUserRootFallbackCases(), remoteUserPrecedenceCases()...) +} + +func remoteUserRootFallbackCases() []getRemoteUserCase { + return []getRemoteUserCase{ + { + name: "nil result falls back to root", + result: nil, + want: testUserRoot, + }, + { + name: "no user sources falls back to root", + result: &Result{ + MergedConfig: &MergedDevContainerConfig{}, + ContainerDetails: &ContainerDetails{ + Config: ContainerDetailsConfig{}, + }, + }, + want: testUserRoot, + }, + } +} + +func remoteUserPrecedenceCases() []getRemoteUserCase { + return []getRemoteUserCase{ + { + name: "remoteUser from config wins", + result: &Result{ + MergedConfig: &MergedDevContainerConfig{ + DevContainerConfigBase: DevContainerConfigBase{ + RemoteUser: "cfg-user", + }, + NonComposeBase: NonComposeBase{ContainerUser: testContainerUser}, + }, + ContainerDetails: &ContainerDetails{ + Config: ContainerDetailsConfig{User: testInspectUser}, + }, + }, + want: "cfg-user", + }, + { + name: "devsy.user label beats docker inspect user", + result: &Result{ + ContainerDetails: &ContainerDetails{ + Config: ContainerDetailsConfig{ + User: testInspectUser, + Labels: map[string]string{UserLabel: testLabelUser}, + }, + }, + }, + want: testLabelUser, + }, + { + name: "containerUser from config beats devsy.user label", + result: &Result{ + MergedConfig: &MergedDevContainerConfig{ + NonComposeBase: NonComposeBase{ContainerUser: testContainerUser}, + }, + ContainerDetails: &ContainerDetails{ + Config: ContainerDetailsConfig{ + User: testInspectUser, + Labels: map[string]string{UserLabel: testLabelUser}, + }, + }, + }, + want: testContainerUser, + }, + { + name: "containerUser beats docker inspect user", + result: &Result{ + MergedConfig: &MergedDevContainerConfig{ + NonComposeBase: NonComposeBase{ContainerUser: testContainerUser}, + }, + ContainerDetails: &ContainerDetails{ + Config: ContainerDetailsConfig{User: testInspectUser}, + }, + }, + want: testContainerUser, + }, + } +} + +const ( + testContainerUser = "container-user" + testInspectUser = "inspect-user" + testLabelUser = "label-user" +) diff --git a/pkg/devcontainer/setup/setup.go b/pkg/devcontainer/setup/setup.go index 3878e7044..d0fc39316 100644 --- a/pkg/devcontainer/setup/setup.go +++ b/pkg/devcontainer/setup/setup.go @@ -322,46 +322,52 @@ func linkRootHome(setupInfo *config.Result) error { } func chownWorkspace(setupInfo *config.Result, recursive bool) error { + workspaceFolder := setupInfo.SubstitutionContext.ContainerWorkspaceFolder + // Compose services aren't guaranteed a workspaceMount; absence is expected. + if _, err := os.Lstat(workspaceFolder); err != nil { + if os.IsNotExist(err) { + log.Debugf("skip chown: workspace folder does not exist: %s", workspaceFolder) + return nil + } + return fmt.Errorf("stat workspace %s: %w", workspaceFolder, err) + } + user := config.GetRemoteUser(setupInfo) - // Marker content is the workspace ID, not empty: a snapshot-restored - // container runs the ORIGINAL workspace's committed image, which already - // carries a chownWorkspace marker from when that original workspace was - // set up. That marker says nothing about whether THIS container's freshly - // restored (root-owned, just-extracted) volume content has been chowned, - // so an empty/content-agnostic marker would wrongly skip chown here and - // leave the workspace folder inaccessible to the remote user. - exists, err := markerFileExists("chownWorkspace", os.Getenv(pkgconfig.EnvWorkspaceID)) + // Scope the marker to a workspace so a restored image does not suppress + // ownership setup for a different workspace. + workspaceID := os.Getenv(pkgconfig.EnvWorkspaceID) + exists, err := markerExists("chownWorkspace", workspaceID) if err != nil { return err } else if exists { return nil } - workspaceRoot := filepath.Dir(setupInfo.SubstitutionContext.ContainerWorkspaceFolder) - + workspaceRoot := filepath.Dir(workspaceFolder) if workspaceRoot != "/" { log.Infof("chown workspace: user=%s, workspaceRoot=%s", user, workspaceRoot) - err = copy2.Chown(workspaceRoot, user) - if err != nil { - log.Warn(err) + if err := copy2.Chown(workspaceRoot, user); err != nil { + return fmt.Errorf("chown %s: %w", workspaceRoot, err) } } if recursive { - log.Infof( - "chown workspace recursively: user=%s, workspaceFolder=%s", - user, - setupInfo.SubstitutionContext.ContainerWorkspaceFolder, - ) - err = copy2.ChownR(setupInfo.SubstitutionContext.ContainerWorkspaceFolder, user) - // Best effort: some entries (e.g. read-only .git pack files on a - // virtiofs share) legitimately cannot be chowned. The remote user can - // still work in the tree, so this is not worth a warning. - if err != nil { - log.Debugf("chown workspace: some entries could not be chowned: %v", err) + log.Infof("chown workspace recursively: user=%s, workspaceFolder=%s", user, workspaceFolder) + err := copy2.ChownR(workspaceFolder, user) + var failures copy2.ChownFailures + switch { + case err == nil: + case errors.As(err, &failures) && failures.AllDenied(): + // Read-only shared files, such as virtiofs pack files, can refuse chown. + log.Warnf("chown workspace: %d entries kept their owner: %v", len(failures), err) + default: + return fmt.Errorf("chown %s recursively: %w", workspaceFolder, err) } } + if err := writeMarker("chownWorkspace", workspaceID); err != nil { + return err + } return nil } @@ -590,25 +596,47 @@ func ensureKubeConfigMaps(config *clientcmdapi.Config) *clientcmdapi.Config { return config } -func markerFileExists(markerName string, markerContent string) (bool, error) { - markerName = filepath.Join(pkgconfig.ContainerDataDir, markerName+".marker") - t, err := os.ReadFile(markerName) - if err != nil && !os.IsNotExist(err) { +// markerExists reports whether the named marker exists with the expected +// content; empty markerContent matches any existing marker. It never writes. +func markerExists(markerName string, markerContent string) (bool, error) { + // #nosec G703 -- markerName is an internal constant, never user input + path := filepath.Join(pkgconfig.ContainerDataDir, markerName+".marker") + // #nosec G304 -- path is built from internal constants, never user input + t, err := os.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + return false, nil + } return false, err - } else if err == nil && (markerContent == "" || string(t) == markerContent) { - return true, nil } + return markerContent == "" || string(t) == markerContent, nil +} - // write marker - _ = os.MkdirAll( - filepath.Dir(markerName), - 0o755, - ) // #nosec G301 -- Standard directory permissions - err = os.WriteFile(markerName, []byte(markerContent), 0o600) - if err != nil { - return false, fmt.Errorf("write marker: %w", err) +// writeMarker records that the work gated by markerExists has completed. +func writeMarker(markerName string, markerContent string) error { + // #nosec G703 -- markerName is an internal constant, never user input + path := filepath.Join(pkgconfig.ContainerDataDir, markerName+".marker") + // #nosec G301 -- Standard directory permissions + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + return fmt.Errorf("create %s: %w", filepath.Dir(path), err) + } + // #nosec G703 -- path is built from internal constants, never user input + if err := os.WriteFile(path, []byte(markerContent), 0o600); err != nil { + return fmt.Errorf("write marker: %w", err) } + return nil +} +// markerFileExists checks for the marker and writes it on miss. Work that must be +// retried on failure should bracket itself with markerExists and writeMarker instead. +func markerFileExists(markerName string, markerContent string) (bool, error) { + exists, err := markerExists(markerName, markerContent) + if err != nil || exists { + return exists, err + } + if err := writeMarker(markerName, markerContent); err != nil { + return false, fmt.Errorf("write marker: %w", err) + } return false, nil } diff --git a/pkg/devcontainer/setup/setup_test.go b/pkg/devcontainer/setup/setup_test.go index 2fd3de80f..40042dfa9 100644 --- a/pkg/devcontainer/setup/setup_test.go +++ b/pkg/devcontainer/setup/setup_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/devsy-org/devsy/pkg/agent/tunnel" + pkgconfig "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/log" "go.uber.org/zap/zapcore" @@ -190,3 +191,74 @@ func TestWriteResultFileTo_WidensStaleModeEvenWhenContentUnchanged(t *testing.T) t.Errorf("mode = %o, want 0644 even though content was already up to date", got) } } + +func TestMarkerRoundTrip(t *testing.T) { + if os.Geteuid() != 0 { + t.Skip("markers live under /var/devsy; writing them needs root") + } + t.Cleanup(func() { + _ = os.Remove(filepath.Join(pkgconfig.ContainerDataDir, "testmarker.marker")) + }) + + exists, err := markerExists("testmarker", "ws-1") + if err != nil { + t.Fatalf("markerExists on miss: %v", err) + } + if exists { + t.Fatal("markerExists = true before writeMarker, want false") + } + + if err := writeMarker("testmarker", "ws-1"); err != nil { + t.Fatalf("writeMarker: %v", err) + } + + for _, tc := range []struct { + name string + content string + want bool + }{ + {name: "matching content", content: "ws-1", want: true}, + {name: "mismatched content", content: "ws-2", want: false}, + {name: "empty content matches any", content: "", want: true}, + } { + t.Run(tc.name, func(t *testing.T) { + got, err := markerExists("testmarker", tc.content) + if err != nil { + t.Fatalf("markerExists: %v", err) + } + if got != tc.want { + t.Errorf("markerExists(%q) = %v, want %v", tc.content, got, tc.want) + } + }) + } +} + +func TestChownWorkspaceSkipsAbsentFolder(t *testing.T) { + if os.Geteuid() != 0 { + t.Skip("markers live under /var/devsy; writing them needs root") + } + t.Setenv(pkgconfig.EnvWorkspaceID, "ws-absent") + t.Cleanup(func() { + _ = os.Remove(filepath.Join(pkgconfig.ContainerDataDir, "chownWorkspace.marker")) + }) + + result := &config.Result{ + SubstitutionContext: &config.SubstitutionContext{ + ContainerWorkspaceFolder: filepath.Join(t.TempDir(), "missing"), + }, + } + + for i := range 2 { + if err := chownWorkspace(result, true); err != nil { + t.Fatalf("chownWorkspace absent folder (call %d): %v", i, err) + } + } + + exists, err := markerExists("chownWorkspace", "ws-absent") + if err != nil { + t.Fatalf("markerExists: %v", err) + } + if exists { + t.Fatal("chownWorkspace wrote the marker for a workspace it never chowned") + } +} diff --git a/pkg/extract/compress.go b/pkg/extract/compress.go index 60e9db60f..25fbe7747 100644 --- a/pkg/extract/compress.go +++ b/pkg/extract/compress.go @@ -25,13 +25,11 @@ func WriteTarExclude( return fmt.Errorf("absolute: %w", err) } - // Check if target is there stat, err := os.Stat(absolute) if err != nil { return fmt.Errorf("stat: %w", err) } - // Use compression gw := writer if compress { gwWriter := gzip.NewWriter(writer) @@ -40,11 +38,9 @@ func WriteTarExclude( gw = gwWriter } - // Create tar writer tarWriter := tar.NewWriter(gw) defer func() { _ = tarWriter.Close() }() - // When its a file we copy the file to the toplevel of the tar if !stat.IsDir() { return NewArchiver( filepath.Dir(absolute), @@ -53,8 +49,6 @@ func WriteTarExclude( ).AddToArchive(filepath.Base(absolute)) } - // When its a folder we copy the contents and not the folder itself to the - // toplevel of the tar return NewArchiver(absolute, tarWriter, excludedPaths).AddToArchive("") } @@ -88,24 +82,19 @@ func (a *Archiver) AddToArchive(relativePath string) error { return nil } - // We skip files that are suddenly not there anymore stat, err := os.Lstat(path.Join(a.basePath, relativePath)) if err != nil { - // config.Logf("[Upstream] Couldn't stat file %s: %s\n", absFilepath, err.Error()) return nil } if stat.IsDir() { - // check if excluded if a.isExcluded(path.Clean(relativePath) + "/") { return nil } - // Recursively tar folder return a.tarFolder(relativePath, stat) } - // check if excluded if a.isExcluded(path.Clean(relativePath)) { return nil } @@ -126,15 +115,11 @@ func (a *Archiver) tarFolder(target string, targetStat os.FileInfo) error { filePath := path.Join(a.basePath, target) files, err := os.ReadDir(filePath) if err != nil { - // config.Logf("[Upstream] Couldn't read dir %s: %s\n", filepath, err.Error()) return nil } if len(files) == 0 && target != "" { - // Case empty directory hdr, _ := tar.FileInfoHeader(targetStat, filePath) - hdr.Uid = 0 - hdr.Gid = 0 hdr.Mode = fillGo18FileTypeBits(int64(chmodTarEntry(os.FileMode(hdr.Mode))), targetStat) hdr.Name = target if err := a.writer.WriteHeader(hdr); err != nil { @@ -175,8 +160,6 @@ func (a *Archiver) tarFile(target string, targetStat os.FileInfo) error { return fmt.Errorf("create tar file info header: %w", err) } hdr.Name = target - hdr.Uid = 0 - hdr.Gid = 0 hdr.Mode = fillGo18FileTypeBits(int64(chmodTarEntry(os.FileMode(hdr.Mode))), targetStat) hdr.ModTime = time.Unix(targetStat.ModTime().Unix(), 0) diff --git a/pkg/extract/extract.go b/pkg/extract/extract.go index 9fbb539e6..4bd030311 100644 --- a/pkg/extract/extract.go +++ b/pkg/extract/extract.go @@ -14,11 +14,11 @@ import ( ) type Options struct { - StripLevels int - - Perm *os.FileMode - UID *int - GID *int + StripLevels int + Perm *os.FileMode + UID *int + GID *int + PreserveOwnership bool } type Option func(o *Options) @@ -29,6 +29,13 @@ func StripLevels(levels int) Option { } } +// PreserveHeaderOwnership makes Extract apply each entry's tar-header uid/gid. +func PreserveHeaderOwnership() Option { + return func(o *Options) { + o.PreserveOwnership = true + } +} + func Extract(origReader io.Reader, destFolder string, options ...Option) error { extractOptions := &Options{} for _, o := range options { @@ -161,17 +168,24 @@ func extractEntry( tarReader *tar.Reader, header *tar.Header, outFileName string, options *Options, ) error { - dirPerm := os.ModePerm - if options.Perm != nil { - dirPerm = *options.Perm + if err := os.MkdirAll(filepath.Dir(outFileName), dirMode(options)); err != nil { + return err } - if err := os.MkdirAll(filepath.Dir(outFileName), dirPerm); err != nil { + + if err := createEntry(tarReader, header, outFileName, options); err != nil { return err } + return applyOwnership(outFileName, header, options) +} + +// createEntry materializes one tar entry on disk according to its type. +func createEntry( + tarReader *tar.Reader, header *tar.Header, outFileName string, options *Options, +) error { switch header.Typeflag { case tar.TypeDir: - return os.MkdirAll(outFileName, dirPerm) + return os.MkdirAll(outFileName, dirMode(options)) case tar.TypeSymlink: return os.Symlink(header.Linkname, outFileName) case tar.TypeLink: @@ -181,6 +195,47 @@ func extractEntry( } } +// dirMode returns the directory permission mode to extract with. +func dirMode(options *Options) os.FileMode { + if options.Perm != nil { + return *options.Perm + } + return os.ModePerm +} + +// applyOwnership chowns a freshly extracted entry when the options ask for +// it, preferring explicit UID/GID overrides over the entry's header values. +func applyOwnership( + outFileName string, header *tar.Header, options *Options, +) error { + uid, gid, ok := ownershipFor(header, options) + if !ok { + return nil + } + if err := os.Lchown(outFileName, uid, gid); err != nil { + if os.Geteuid() != 0 && errors.Is(err, os.ErrPermission) { + return nil + } + return fmt.Errorf("chown %s: %w", outFileName, err) + } + return nil +} + +// ownershipFor resolves the uid/gid to apply and whether any chown is wanted. +func ownershipFor(header *tar.Header, options *Options) (int, int, bool) { + if options.UID == nil && options.GID == nil { + return header.Uid, header.Gid, options.PreserveOwnership + } + uid, gid := 0, 0 + if options.UID != nil { + uid = *options.UID + } + if options.GID != nil { + gid = *options.GID + } + return uid, gid, true +} + func extractRegularFile( tarReader *tar.Reader, header *tar.Header, diff --git a/pkg/extract/extract_test.go b/pkg/extract/extract_test.go index a88067cde..e153b9245 100644 --- a/pkg/extract/extract_test.go +++ b/pkg/extract/extract_test.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "strings" + "syscall" "testing" ) @@ -15,6 +16,8 @@ type tarEntry struct { body string linkTarget string symlink bool + uid int + gid int } func (e tarEntry) header() *tar.Header { @@ -37,6 +40,8 @@ func (e tarEntry) header() *tar.Header { Name: e.name, Size: int64(len(e.body)), Mode: 0o644, + Uid: e.uid, + Gid: e.gid, } } @@ -174,3 +179,52 @@ func TestExtract_ValidSymlinkAllowed(t *testing.T) { t.Fatalf("symlink target = %q, want %q", target, targetFileName) } } + +func TestExtract_PreserveHeaderOwnership(t *testing.T) { + t.Parallel() + buf := newTarGz(t, []tarEntry{ + {name: "dir", uid: os.Getuid(), gid: os.Getgid()}, + }) + + dest := t.TempDir() + if err := Extract(buf, dest, PreserveHeaderOwnership()); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + info, err := os.Stat(filepath.Join(dest, "dir")) + if err != nil { + t.Fatalf("stat dir: %v", err) + } + if stat, ok := info.Sys().(*syscall.Stat_t); ok { + //nolint:gosec // G115 — uid/gid are uint32 on every supported platform + if stat.Uid != uint32(os.Getuid()) || stat.Gid != uint32(os.Getgid()) { + t.Fatalf( + "dir owner = %d:%d, want %d:%d", + stat.Uid, stat.Gid, os.Getuid(), os.Getgid(), + ) + } + } +} + +func TestExtract_PreserveHeaderOwnershipUnprivilegedDegrades(t *testing.T) { + t.Parallel() + if os.Geteuid() == 0 { + t.Skip("running as root: permission errors cannot occur") + } + buf := newTarGz(t, []tarEntry{ + {name: "hello.txt", body: "world", uid: 0, gid: 0}, + }) + + dest := t.TempDir() + if err := Extract(buf, dest, PreserveHeaderOwnership()); err != nil { + t.Fatalf("unexpected error: %v", err) + } + path := filepath.Join(dest, "hello.txt") + content, err := os.ReadFile(path) //nolint:gosec // G304 — test temp file + if err != nil { + t.Fatalf("read extracted file: %v", err) + } + if string(content) != "world" { + t.Fatalf("got %q, want %q", string(content), "world") + } +} diff --git a/pkg/snapshot/manifest.go b/pkg/snapshot/manifest.go index bf7e7fcb5..ea72954d8 100644 --- a/pkg/snapshot/manifest.go +++ b/pkg/snapshot/manifest.go @@ -7,15 +7,10 @@ import ( ) const ( - VolumesMediaType = "application/vnd.devsy.snapshot.volumes.v1.tar+gzip" - ManifestMediaType = "application/vnd.oci.image.manifest.v1+json" - ManifestArtifactType = "application/vnd.devsy.snapshot.manifest.v1+json" - emptyConfigMediaType = "application/vnd.oci.empty.v1+json" - // defaultContainerImageMediaType is used only when - // BuildManifestOptions.ContainerImageMediaType is unset. Real callers - // (cmd/snapshot/create.go) always pass the media type the registry - // actually reported for the pushed image, which may be the OCI or the - // Docker v2 manifest format depending on the daemon/registry. + VolumesMediaType = "application/vnd.devsy.snapshot.volumes.v1.tar+gzip" + ManifestMediaType = "application/vnd.oci.image.manifest.v1+json" + ManifestArtifactType = "application/vnd.devsy.snapshot.manifest.v1+json" + emptyConfigMediaType = "application/vnd.oci.empty.v1+json" defaultContainerImageMediaType = "application/vnd.docker.distribution.manifest.v2+json" ) @@ -26,26 +21,10 @@ const ( AnnotationDevContainerHash = "sh.devsy.snapshot.devcontainer-hash" AnnotationSourceProvider = "sh.devsy.snapshot.source-provider" AnnotationMessage = "sh.devsy.snapshot.message" - // AnnotationMountPrefix is the create-time mount target path (leading "/" - // trimmed) that volumes archive entries are prefixed with. Restore must - // strip exactly this many path segments regardless of the restore-side - // mount target's own depth, since the two are not guaranteed to match - // (different provider defaults, different workspace-folder conventions). - AnnotationMountPrefix = "sh.devsy.snapshot.mount-prefix" - // AnnotationRunArgs is the create-time devcontainer.json's runArgs - // (JSON-encoded []string), replayed onto the restored container so - // runArgs the original devcontainer.json relied on (e.g. - // --add-host=host.docker.internal:host-gateway for a registry reachable - // only via that hostname) still apply — restore pins DevContainerSource - // to the committed image, which bypasses the project devcontainer.json - // and would otherwise silently drop them. - AnnotationRunArgs = "sh.devsy.snapshot.run-args" - // AnnotationContainerEnv is the create-time devcontainer.json's - // containerEnv (JSON-encoded map[string]string), replayed onto the - // restored container for the same reason as AnnotationRunArgs: restore - // pins DevContainerSource to the committed image, bypassing the project - // devcontainer.json and silently dropping any containerEnv it set. - AnnotationContainerEnv = "sh.devsy.snapshot.container-env" + AnnotationMountPrefix = "sh.devsy.snapshot.mount-prefix" + AnnotationRunArgs = "sh.devsy.snapshot.run-args" + AnnotationContainerEnv = "sh.devsy.snapshot.container-env" + AnnotationRemoteUser = "sh.devsy.snapshot.remote-user" ) // Descriptor mirrors the OCI content descriptor fields we need; kept minimal @@ -76,6 +55,7 @@ type BuildManifestOptions struct { MountPrefix string RunArgs []string ContainerEnv map[string]string + RemoteUser string ContainerImageMediaType string ContainerImageDigest string @@ -165,6 +145,9 @@ func addDevContainerOverrideAnnotations( } annotations[AnnotationContainerEnv] = string(raw) } + if opts.RemoteUser != "" { + annotations[AnnotationRemoteUser] = opts.RemoteUser + } return nil } @@ -182,6 +165,12 @@ func (m *Manifest) RunArgs() ([]string, error) { return args, nil } +// RemoteUser returns the create-time devcontainer.json's remoteUser, or "" +// when the snapshot carries none. +func (m *Manifest) RemoteUser() string { + return m.Annotations[AnnotationRemoteUser] +} + // ContainerEnv decodes the create-time devcontainer.json's containerEnv from // the manifest, or returns nil when the snapshot carries none. func (m *Manifest) ContainerEnv() (map[string]string, error) {