From d7d169f2bf77fae6dff9851de2934b14e5611ba6 Mon Sep 17 00:00:00 2001 From: Dmitry Meyer Date: Tue, 1 Jul 2025 15:14:56 +0000 Subject: [PATCH 1/2] [runner] Fix `~` expansion in `files` Use the runner process user (`root` expected) as the job user if the `user` directive is not specified in either the image or the dstack configuration. Previously, the repo dir (`/workflow`) was used as a fallback for the home dir if the user is not specified. Fixes: https://github.com/dstackai/dstack/issues/2864 --- runner/internal/executor/executor.go | 27 ++++--- runner/internal/executor/files.go | 106 ++++++++++++++------------- 2 files changed, 69 insertions(+), 64 deletions(-) diff --git a/runner/internal/executor/executor.go b/runner/internal/executor/executor.go index c720eb02e9..fd2491501e 100644 --- a/runner/internal/executor/executor.go +++ b/runner/internal/executor/executor.go @@ -133,16 +133,17 @@ func (ex *RunExecutor) Run(ctx context.Context) (err error) { ctx = log.WithLogger(ctx, log.NewEntry(logger, int(log.DefaultEntry.Logger.Level))) // todo loglevel log.Info(ctx, "Run job", "log_level", log.GetLogger(ctx).Logger.Level.String()) - if ex.jobSpec.User != nil { - if err := fillUser(ex.jobSpec.User); err != nil { - ex.SetJobStateWithTerminationReason( - ctx, - types.JobStateFailed, - types.TerminationReasonExecutorError, - fmt.Sprintf("Failed to fill in the job user fields (%s)", err), - ) - return gerrors.Wrap(err) - } + if ex.jobSpec.User == nil { + ex.jobSpec.User = &schemas.User{Uid: &ex.currentUid} + } + if err := fillUser(ex.jobSpec.User); err != nil { + ex.SetJobStateWithTerminationReason( + ctx, + types.JobStateFailed, + types.TerminationReasonExecutorError, + fmt.Sprintf("Failed to fill in the job user fields (%s)", err), + ) + return gerrors.Wrap(err) } if err := ex.setupFiles(ctx); err != nil { @@ -331,15 +332,15 @@ func (ex *RunExecutor) execJob(ctx context.Context, jobLogFile io.Writer) error cmd.Dir = workingDir } + // User must be already set user := ex.jobSpec.User - if user != nil { + if ex.currentUid == 0 { log.Trace( ctx, "Using credentials", "uid", *user.Uid, "gid", *user.Gid, "groups", user.GroupIds, "username", user.GetUsername(), "groupname", user.GetGroupname(), "home", user.HomeDir, ) - log.Trace(ctx, "Current user", "uid", ex.currentUid) // 1. Ideally, We should check uid, gid, and supplementary groups mismatches, // but, for the sake of simplicity, we only check uid. Unprivileged runner @@ -362,6 +363,8 @@ func (ex *RunExecutor) execJob(ctx context.Context, jobLogFile io.Writer) error Gid: *user.Gid, Groups: user.GroupIds, } + } else { + log.Info(ctx, "Current user is not root, cannot set process credentials", "uid", ex.currentUid) } envMap := NewEnvMap(ParseEnvList(os.Environ()), jobEnvs, ex.secrets) diff --git a/runner/internal/executor/files.go b/runner/internal/executor/files.go index 89de206338..9cff8a28ca 100644 --- a/runner/internal/executor/files.go +++ b/runner/internal/executor/files.go @@ -39,67 +39,69 @@ func (ex *RunExecutor) setupFiles(ctx context.Context) error { homeDir := ex.workingDir uid := -1 gid := -1 - if ex.jobSpec.User != nil { - if ex.jobSpec.User.HomeDir != "" { - homeDir = ex.jobSpec.User.HomeDir - } - if ex.jobSpec.User.Uid != nil { - uid = int(*ex.jobSpec.User.Uid) - } - if ex.jobSpec.User.Gid != nil { - gid = int(*ex.jobSpec.User.Gid) - } + // User must be already set + if ex.jobSpec.User.HomeDir != "" { + homeDir = ex.jobSpec.User.HomeDir + } + if ex.jobSpec.User.Uid != nil { + uid = int(*ex.jobSpec.User.Uid) + } + if ex.jobSpec.User.Gid != nil { + gid = int(*ex.jobSpec.User.Gid) } for _, fa := range ex.run.RunSpec.FileArchives { - log.Trace(ctx, "Extracting file archive", "id", fa.Id, "path", fa.Path) - - p := path.Clean(fa.Path) - // `~username[/path/to]` is not supported - if p == "~" { - p = homeDir - } else if rest, found := strings.CutPrefix(p, "~/"); found { - p = path.Join(homeDir, rest) - } else if !path.IsAbs(p) { - p = path.Join(ex.workingDir, p) - } - dir, root := path.Split(p) - if err := mkdirAll(ctx, dir, uid, gid); err != nil { + archivePath := path.Join(ex.archiveDir, fa.Id) + if err := extractFileArchive(ctx, archivePath, fa.Path, ex.workingDir, uid, gid, homeDir); err != nil { return gerrors.Wrap(err) } + } - if err := os.RemoveAll(p); err != nil { - log.Warning(ctx, "Failed to remove", "path", p, "err", err) - } + if err := os.RemoveAll(ex.archiveDir); err != nil { + log.Warning(ctx, "Failed to remove file archives dir", "path", ex.archiveDir, "err", err) + } - archivePath := path.Join(ex.archiveDir, fa.Id) - archive, err := os.Open(archivePath) - if err != nil { - return gerrors.Wrap(err) - } - defer func() { - _ = archive.Close() - if err := os.Remove(archivePath); err != nil { - log.Warning(ctx, "Failed to remove archive", "path", archivePath, "err", err) - } - }() + return nil +} - var paths []string - repl := fmt.Sprintf("%s$2", root) - renameAndRemember := func(s string) string { - s = renameRegex.ReplaceAllString(s, repl) - paths = append(paths, s) - return s - } - if err := extract.Tar(ctx, archive, dir, renameAndRemember); err != nil { - return gerrors.Wrap(err) - } +func extractFileArchive(ctx context.Context, archivePath string, targetPath string, targetRoot string, uid int, gid int, homeDir string) error { + log.Trace(ctx, "Extracting file archive", "archive", archivePath, "target", targetPath) - if uid != -1 || gid != -1 { - for _, p := range paths { - if err := os.Chown(path.Join(dir, p), uid, gid); err != nil { - log.Warning(ctx, "Failed to chown", "path", p, "err", err) - } + targetPath = path.Clean(targetPath) + // `~username[/path/to]` is not supported + if targetPath == "~" { + targetPath = homeDir + } else if rest, found := strings.CutPrefix(targetPath, "~/"); found { + targetPath = path.Join(homeDir, rest) + } else if !path.IsAbs(targetPath) { + targetPath = path.Join(targetRoot, targetPath) + } + dir, root := path.Split(targetPath) + if err := mkdirAll(ctx, dir, uid, gid); err != nil { + return gerrors.Wrap(err) + } + + archive, err := os.Open(archivePath) + if err != nil { + return gerrors.Wrap(err) + } + defer archive.Close() + + var paths []string + repl := fmt.Sprintf("%s$2", root) + renameAndRemember := func(s string) string { + s = renameRegex.ReplaceAllString(s, repl) + paths = append(paths, s) + return s + } + if err := extract.Tar(ctx, archive, dir, renameAndRemember); err != nil { + return gerrors.Wrap(err) + } + + if uid != -1 || gid != -1 { + for _, p := range paths { + if err := os.Chown(path.Join(dir, p), uid, gid); err != nil { + log.Warning(ctx, "Failed to chown", "path", p, "err", err) } } } From cd24255cef0f574051f90e5a32bfc1b8056f4d50 Mon Sep 17 00:00:00 2001 From: Dmitry Meyer Date: Wed, 2 Jul 2025 08:35:57 +0000 Subject: [PATCH 2/2] Address PR comments * Remove `path` if exists before unpacking * Remove redundant checks and no longer relevant comments --- runner/internal/executor/executor.go | 17 +++-------------- runner/internal/executor/files.go | 3 +++ 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/runner/internal/executor/executor.go b/runner/internal/executor/executor.go index fd2491501e..e14ce540db 100644 --- a/runner/internal/executor/executor.go +++ b/runner/internal/executor/executor.go @@ -334,6 +334,9 @@ func (ex *RunExecutor) execJob(ctx context.Context, jobLogFile io.Writer) error // User must be already set user := ex.jobSpec.User + // Strictly speaking, we need CAP_SETUID and CAP_GUID (for Cmd.Start()-> + // Cmd.SysProcAttr.Credential) and CAP_CHOWN (for startCommand()->os.Chown()), + // but for the sake of simplicity we instead check if we are root or not if ex.currentUid == 0 { log.Trace( ctx, "Using credentials", @@ -341,23 +344,9 @@ func (ex *RunExecutor) execJob(ctx context.Context, jobLogFile io.Writer) error "username", user.GetUsername(), "groupname", user.GetGroupname(), "home", user.HomeDir, ) - - // 1. Ideally, We should check uid, gid, and supplementary groups mismatches, - // but, for the sake of simplicity, we only check uid. Unprivileged runner - // should not receive job requests where user credentials do not match the - // current user's ones in the first place (it should be handled by the server) - // 2. Strictly speaking, we need CAP_SETUID and CAP_GUID (for Cmd.Start()-> - // Cmd.SysProcAttr.Credential) and CAP_CHOWN (for startCommand()->os.Chown()), - // but for the sake of simplicity we instead check if we are root or not - if *user.Uid != ex.currentUid && ex.currentUid != 0 { - return gerrors.Newf("cannot start job as %d, current uid is %d", *user.Uid, ex.currentUid) - } - if cmd.SysProcAttr == nil { cmd.SysProcAttr = &syscall.SysProcAttr{} } - // It's safe to setuid(2)/setgid(2)/setgroups(2) as unprivileged user if we use - // user's own credentials (basically, it's noop) cmd.SysProcAttr.Credential = &syscall.Credential{ Uid: *user.Uid, Gid: *user.Gid, diff --git a/runner/internal/executor/files.go b/runner/internal/executor/files.go index 9cff8a28ca..c14dff0b44 100644 --- a/runner/internal/executor/files.go +++ b/runner/internal/executor/files.go @@ -80,6 +80,9 @@ func extractFileArchive(ctx context.Context, archivePath string, targetPath stri if err := mkdirAll(ctx, dir, uid, gid); err != nil { return gerrors.Wrap(err) } + if err := os.RemoveAll(targetPath); err != nil { + log.Warning(ctx, "Failed to remove", "path", targetPath, "err", err) + } archive, err := os.Open(archivePath) if err != nil {