Skip to content

Commit cdff7d1

Browse files
un-defun-def
authored
[runner] Fix ~ expansion in files (#2865)
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: #2864
1 parent 325bf8b commit cdff7d1

File tree

2 files changed

+75
-78
lines changed

2 files changed

+75
-78
lines changed

‎runner/internal/executor/executor.go‎

Lines changed: 18 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -133,16 +133,17 @@ func (ex *RunExecutor) Run(ctx context.Context) (err error) {
133133
ctx = log.WithLogger(ctx, log.NewEntry(logger, int(log.DefaultEntry.Logger.Level))) // todo loglevel
134134
log.Info(ctx, "Run job", "log_level", log.GetLogger(ctx).Logger.Level.String())
135135

136-
if ex.jobSpec.User != nil {
137-
if err := fillUser(ex.jobSpec.User); err != nil {
138-
ex.SetJobStateWithTerminationReason(
139-
ctx,
140-
types.JobStateFailed,
141-
types.TerminationReasonExecutorError,
142-
fmt.Sprintf("Failed to fill in the job user fields (%s)", err),
143-
)
144-
return gerrors.Wrap(err)
145-
}
136+
if ex.jobSpec.User == nil {
137+
ex.jobSpec.User = &schemas.User{Uid: &ex.currentUid}
138+
}
139+
if err := fillUser(ex.jobSpec.User); err != nil {
140+
ex.SetJobStateWithTerminationReason(
141+
ctx,
142+
types.JobStateFailed,
143+
types.TerminationReasonExecutorError,
144+
fmt.Sprintf("Failed to fill in the job user fields (%s)", err),
145+
)
146+
return gerrors.Wrap(err)
146147
}
147148

148149
if err := ex.setupFiles(ctx); err != nil {
@@ -331,37 +332,28 @@ func (ex *RunExecutor) execJob(ctx context.Context, jobLogFile io.Writer) error
331332
cmd.Dir = workingDir
332333
}
333334

335+
// User must be already set
334336
user := ex.jobSpec.User
335-
if user != nil {
337+
// Strictly speaking, we need CAP_SETUID and CAP_GUID (for Cmd.Start()->
338+
// Cmd.SysProcAttr.Credential) and CAP_CHOWN (for startCommand()->os.Chown()),
339+
// but for the sake of simplicity we instead check if we are root or not
340+
if ex.currentUid == 0 {
336341
log.Trace(
337342
ctx, "Using credentials",
338343
"uid", *user.Uid, "gid", *user.Gid, "groups", user.GroupIds,
339344
"username", user.GetUsername(), "groupname", user.GetGroupname(),
340345
"home", user.HomeDir,
341346
)
342-
log.Trace(ctx, "Current user", "uid", ex.currentUid)
343-
344-
// 1. Ideally, We should check uid, gid, and supplementary groups mismatches,
345-
// but, for the sake of simplicity, we only check uid. Unprivileged runner
346-
// should not receive job requests where user credentials do not match the
347-
// current user's ones in the first place (it should be handled by the server)
348-
// 2. Strictly speaking, we need CAP_SETUID and CAP_GUID (for Cmd.Start()->
349-
// Cmd.SysProcAttr.Credential) and CAP_CHOWN (for startCommand()->os.Chown()),
350-
// but for the sake of simplicity we instead check if we are root or not
351-
if *user.Uid != ex.currentUid && ex.currentUid != 0 {
352-
return gerrors.Newf("cannot start job as %d, current uid is %d", *user.Uid, ex.currentUid)
353-
}
354-
355347
if cmd.SysProcAttr == nil {
356348
cmd.SysProcAttr = &syscall.SysProcAttr{}
357349
}
358-
// It's safe to setuid(2)/setgid(2)/setgroups(2) as unprivileged user if we use
359-
// user's own credentials (basically, it's noop)
360350
cmd.SysProcAttr.Credential = &syscall.Credential{
361351
Uid: *user.Uid,
362352
Gid: *user.Gid,
363353
Groups: user.GroupIds,
364354
}
355+
} else {
356+
log.Info(ctx, "Current user is not root, cannot set process credentials", "uid", ex.currentUid)
365357
}
366358

367359
envMap := NewEnvMap(ParseEnvList(os.Environ()), jobEnvs, ex.secrets)

‎runner/internal/executor/files.go‎

Lines changed: 57 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -39,67 +39,72 @@ func (ex *RunExecutor) setupFiles(ctx context.Context) error {
3939
homeDir := ex.workingDir
4040
uid := -1
4141
gid := -1
42-
if ex.jobSpec.User != nil {
43-
if ex.jobSpec.User.HomeDir != "" {
44-
homeDir = ex.jobSpec.User.HomeDir
45-
}
46-
if ex.jobSpec.User.Uid != nil {
47-
uid = int(*ex.jobSpec.User.Uid)
48-
}
49-
if ex.jobSpec.User.Gid != nil {
50-
gid = int(*ex.jobSpec.User.Gid)
51-
}
42+
// User must be already set
43+
if ex.jobSpec.User.HomeDir != "" {
44+
homeDir = ex.jobSpec.User.HomeDir
45+
}
46+
if ex.jobSpec.User.Uid != nil {
47+
uid = int(*ex.jobSpec.User.Uid)
48+
}
49+
if ex.jobSpec.User.Gid != nil {
50+
gid = int(*ex.jobSpec.User.Gid)
5251
}
5352

5453
for _, fa := range ex.jobSpec.FileArchives {
55-
log.Trace(ctx, "Extracting file archive", "id", fa.Id, "path", fa.Path)
56-
57-
p := path.Clean(fa.Path)
58-
// `~username[/path/to]` is not supported
59-
if p == "~" {
60-
p = homeDir
61-
} else if rest, found := strings.CutPrefix(p, "~/"); found {
62-
p = path.Join(homeDir, rest)
63-
} else if !path.IsAbs(p) {
64-
p = path.Join(ex.workingDir, p)
65-
}
66-
dir, root := path.Split(p)
67-
if err := mkdirAll(ctx, dir, uid, gid); err != nil {
54+
archivePath := path.Join(ex.archiveDir, fa.Id)
55+
if err := extractFileArchive(ctx, archivePath, fa.Path, ex.workingDir, uid, gid, homeDir); err != nil {
6856
return gerrors.Wrap(err)
6957
}
58+
}
7059

71-
if err := os.RemoveAll(p); err != nil {
72-
log.Warning(ctx, "Failed to remove", "path", p, "err", err)
73-
}
60+
if err := os.RemoveAll(ex.archiveDir); err != nil {
61+
log.Warning(ctx, "Failed to remove file archives dir", "path", ex.archiveDir, "err", err)
62+
}
7463

75-
archivePath := path.Join(ex.archiveDir, fa.Id)
76-
archive, err := os.Open(archivePath)
77-
if err != nil {
78-
return gerrors.Wrap(err)
79-
}
80-
defer func() {
81-
_ = archive.Close()
82-
if err := os.Remove(archivePath); err != nil {
83-
log.Warning(ctx, "Failed to remove archive", "path", archivePath, "err", err)
84-
}
85-
}()
64+
return nil
65+
}
8666

87-
var paths []string
88-
repl := fmt.Sprintf("%s$2", root)
89-
renameAndRemember := func(s string) string {
90-
s = renameRegex.ReplaceAllString(s, repl)
91-
paths = append(paths, s)
92-
return s
93-
}
94-
if err := extract.Tar(ctx, archive, dir, renameAndRemember); err != nil {
95-
return gerrors.Wrap(err)
96-
}
67+
func extractFileArchive(ctx context.Context, archivePath string, targetPath string, targetRoot string, uid int, gid int, homeDir string) error {
68+
log.Trace(ctx, "Extracting file archive", "archive", archivePath, "target", targetPath)
9769

98-
if uid != -1 || gid != -1 {
99-
for _, p := range paths {
100-
if err := os.Chown(path.Join(dir, p), uid, gid); err != nil {
101-
log.Warning(ctx, "Failed to chown", "path", p, "err", err)
102-
}
70+
targetPath = path.Clean(targetPath)
71+
// `~username[/path/to]` is not supported
72+
if targetPath == "~" {
73+
targetPath = homeDir
74+
} else if rest, found := strings.CutPrefix(targetPath, "~/"); found {
75+
targetPath = path.Join(homeDir, rest)
76+
} else if !path.IsAbs(targetPath) {
77+
targetPath = path.Join(targetRoot, targetPath)
78+
}
79+
dir, root := path.Split(targetPath)
80+
if err := mkdirAll(ctx, dir, uid, gid); err != nil {
81+
return gerrors.Wrap(err)
82+
}
83+
if err := os.RemoveAll(targetPath); err != nil {
84+
log.Warning(ctx, "Failed to remove", "path", targetPath, "err", err)
85+
}
86+
87+
archive, err := os.Open(archivePath)
88+
if err != nil {
89+
return gerrors.Wrap(err)
90+
}
91+
defer archive.Close()
92+
93+
var paths []string
94+
repl := fmt.Sprintf("%s$2", root)
95+
renameAndRemember := func(s string) string {
96+
s = renameRegex.ReplaceAllString(s, repl)
97+
paths = append(paths, s)
98+
return s
99+
}
100+
if err := extract.Tar(ctx, archive, dir, renameAndRemember); err != nil {
101+
return gerrors.Wrap(err)
102+
}
103+
104+
if uid != -1 || gid != -1 {
105+
for _, p := range paths {
106+
if err := os.Chown(path.Join(dir, p), uid, gid); err != nil {
107+
log.Warning(ctx, "Failed to chown", "path", p, "err", err)
103108
}
104109
}
105110
}

0 commit comments

Comments
 (0)