Merge "services/device/internal: Allow re-run'ing of apps to work"
diff --git a/services/device/device/doc.go b/services/device/device/doc.go
index cd1ba14..53c9a17 100644
--- a/services/device/device/doc.go
+++ b/services/device/device/doc.go
@@ -41,6 +41,9 @@
-alsologtostderr=true
log to standard error as well as files
+ -chown=false
+ Change owner of files and directories given as command-line arguments to the
+ user specified by this flag
-dryrun=false
Elides root-requiring systemcalls.
-kill=false
diff --git a/services/device/deviced/doc.go b/services/device/deviced/doc.go
index 567ed5c..3468792 100644
--- a/services/device/deviced/doc.go
+++ b/services/device/deviced/doc.go
@@ -43,6 +43,9 @@
-alsologtostderr=true
log to standard error as well as files
+ -chown=false
+ Change owner of files and directories given as command-line arguments to the
+ user specified by this flag
-dryrun=false
Elides root-requiring systemcalls.
-kill=false
diff --git a/services/device/internal/impl/app_service.go b/services/device/internal/impl/app_service.go
index 3f2b49d..066b667 100644
--- a/services/device/internal/impl/app_service.go
+++ b/services/device/internal/impl/app_service.go
@@ -766,6 +766,7 @@
saArgs.workspace = rootDir
logDir := filepath.Join(instanceDir, "logs")
+ suidHelper.chownTree(suidHelper.getCurrentUser(), instanceDir, os.Stdout, os.Stdin)
if err := mkdirPerm(logDir, 0755); err != nil {
return nil, err
}
diff --git a/services/device/internal/impl/helper_manager.go b/services/device/internal/impl/helper_manager.go
index f4369eb..280db2b 100644
--- a/services/device/internal/impl/helper_manager.go
+++ b/services/device/internal/impl/helper_manager.go
@@ -40,6 +40,10 @@
}
}
+func (s suidHelperState) getCurrentUser() string {
+ return s.dmUser
+}
+
// terminatePid sends a SIGKILL to the target pid
func (s suidHelperState) terminatePid(pid int, stdout, stderr io.Writer) error {
if err := s.internalModalOp(stdout, stderr, "--kill", strconv.Itoa(pid)); err != nil {
@@ -56,6 +60,16 @@
return nil
}
+// chown files or directories
+func (s suidHelperState) chownTree(username string, dirOrFile string, stdout, stderr io.Writer) error {
+ args := []string{"--chown", "--username", username, dirOrFile}
+
+ if err := s.internalModalOp(stdout, stderr, args...); err != nil {
+ return fmt.Errorf("devicemanager's invocation of suidhelper chown %v failed: %v", dirOrFile, err)
+ }
+ return nil
+}
+
type suidAppCmdArgs struct {
// args to helper
targetUser, progname, workspace, logdir, binpath string
@@ -116,6 +130,7 @@
if err := cmd.Run(); err != nil {
vlog.Errorf("failed calling helper with args (%v):%v", arg, err)
+ return err
}
return nil
}
diff --git a/services/device/internal/suid/args.go b/services/device/internal/suid/args.go
index 10ccfac..1473654 100644
--- a/services/device/internal/suid/args.go
+++ b/services/device/internal/suid/args.go
@@ -8,6 +8,7 @@
"bytes"
"encoding/json"
"flag"
+ "fmt"
"os"
"os/user"
"strconv"
@@ -38,6 +39,7 @@
envv []string
dryrun bool
remove bool
+ chown bool
kill bool
killPids []int
}
@@ -54,7 +56,7 @@
var (
flagUsername, flagWorkspace, flagLogDir, flagRun, flagProgName *string
flagMinimumUid *int64
- flagRemove, flagKill, flagDryrun *bool
+ flagRemove, flagKill, flagChown, flagDryrun *bool
)
func init() {
@@ -71,6 +73,7 @@
flagMinimumUid = fs.Int64("minuid", uidThreshold, "UIDs cannot be less than this number.")
flagRemove = fs.Bool("rm", false, "Remove the file trees given as command-line arguments.")
flagKill = fs.Bool("kill", false, "Kill process ids given as command-line arguments.")
+ flagChown = fs.Bool("chown", false, "Change owner of files and directories given as command-line arguments to the user specified by this flag")
flagDryrun = fs.Bool("dryrun", false, "Elides root-requiring systemcalls.")
}
@@ -84,29 +87,62 @@
return nenv
}
+// checkFlagCombinations makes sure that a valid combination of flags has been
+// specified for rm/kill/chown
+//
+// --rm and --kill are modal. Complain if any other flag is set along with one of
+// those. --chown allows specification of --username, --dryrun, and --minuid,
+// but nothing else
+func checkFlagCombinations(fs *flag.FlagSet) error {
+ if !(*flagRemove || *flagKill || *flagChown) {
+ return nil
+ }
+
+ // Count flags that are set. The device manager test always sets --minuid=1
+ // and --test.run=TestSuidHelper so when in a test, tolerate those.
+ flagsToIgnore := map[string]string{}
+ if os.Getenv("V23_SUIDHELPER_TEST") != "" {
+ flagsToIgnore["minuid"] = "1"
+ flagsToIgnore["test.run"] = "TestSuidHelper"
+ }
+ if *flagChown {
+ // Allow all values of --username, --dryrun, and --minuid
+ flagsToIgnore["username"] = "*"
+ flagsToIgnore["dryrun"] = "*"
+ flagsToIgnore["minuid"] = "*"
+ }
+
+ counter := 0
+ fs.Visit(func(f *flag.Flag) {
+ if flagsToIgnore[f.Name] != f.Value.String() && flagsToIgnore[f.Name] != "*" {
+ counter++
+ }
+ })
+
+ if counter > 1 {
+ return verror.New(errInvalidFlags, nil, counter, "--rm and --kill cannot be used with any other flag. --chown can only be used with --username, --dryrun, and --minuid")
+ }
+ return nil
+}
+
+// warnMissingSuidPrivs makes it a little easier to debug when suid privs are required but
+// are not present. It's not a comprehensive check -- e.g. we may be running as user
+// <username> and suppress the warning, but still fail to chown a file owned by some other user.
+func warnMissingSuidPrivs(uid int) {
+ osUid, osEuid := os.Getuid(), os.Geteuid()
+ if osUid == 0 || osEuid == 0 || osUid == uid || osEuid == uid {
+ return
+ }
+
+ fmt.Fprintln(os.Stderr, "uid is ", os.Getuid(), ", effective uid is ", os.Geteuid())
+ fmt.Fprintln(os.Stderr, "WARNING: suidhelper is not root. Is your filesystem mounted with nosuid?")
+}
+
// ParseArguments populates the WorkParameter object from the provided args
// and env strings.
func (wp *WorkParameters) ProcessArguments(fs *flag.FlagSet, env []string) error {
- // --rm and --kill are modal. Complain if any other flag is set along with one of those.
- if *flagRemove || *flagKill {
- // Count flags that are set. The device manager test always sets --minuid=1
- // and --test.run=TestSuidHelper so when in a test, tolerate those
- flagsToIgnore := map[string]string{}
- if os.Getenv("V23_SUIDHELPER_TEST") != "" {
- flagsToIgnore["minuid"] = "1"
- flagsToIgnore["test.run"] = "TestSuidHelper"
- }
-
- counter := 0
- fs.Visit(func(f *flag.Flag) {
- if flagsToIgnore[f.Name] != f.Value.String() {
- counter++
- }
- })
-
- if counter > 1 {
- return verror.New(errInvalidFlags, nil, counter, "--rm and --kill cannot be used with any other flag")
- }
+ if err := checkFlagCombinations(fs); err != nil {
+ return err
}
if *flagRemove {
@@ -146,15 +182,25 @@
if err != nil {
return verror.New(errInvalidGID, nil, usr.Gid)
}
+ warnMissingSuidPrivs(int(uid))
// Uids less than 501 can be special so we forbid running as them.
if uid < *flagMinimumUid {
return verror.New(errUIDTooLow, nil,
uid, *flagMinimumUid)
}
+ wp.uid = int(uid)
+ wp.gid = int(gid)
wp.dryrun = *flagDryrun
+ // At this point, all flags allowed by --chown have been processed
+ if *flagChown {
+ wp.chown = true
+ wp.argv = fs.Args()
+ return nil
+ }
+
// Preserve the arguments for examination by the test harness if executed
// in the course of a test.
if os.Getenv("V23_SUIDHELPER_TEST") != "" {
@@ -171,8 +217,6 @@
wp.dryrun = true
}
- wp.uid = int(uid)
- wp.gid = int(gid)
wp.workspace = *flagWorkspace
wp.argv0 = *flagRun
wp.logDir = *flagLogDir
diff --git a/services/device/internal/suid/args_test.go b/services/device/internal/suid/args_test.go
index 092c1ab..491a171 100644
--- a/services/device/internal/suid/args_test.go
+++ b/services/device/internal/suid/args_test.go
@@ -41,6 +41,7 @@
envv: []string{"A=B"},
dryrun: false,
remove: false,
+ chown: false,
kill: false,
killPids: nil,
},
@@ -61,6 +62,7 @@
envv: []string{"A=B"},
dryrun: false,
remove: false,
+ chown: false,
kill: false,
killPids: nil,
},
@@ -87,6 +89,27 @@
envv: nil,
dryrun: false,
remove: true,
+ chown: false,
+ kill: false,
+ killPids: nil,
+ },
+ },
+
+ {
+ []string{"setuidhelper", "--chown", "--username", testUserName, "--dryrun", "--minuid", "1", "/tmp/foo", "/tmp/bar"},
+ []string{"A=B"},
+ "",
+ WorkParameters{
+ uid: testUid,
+ gid: testGid,
+ workspace: "",
+ logDir: "",
+ argv0: "",
+ argv: []string{"/tmp/foo", "/tmp/bar"},
+ envv: nil,
+ dryrun: true,
+ remove: false,
+ chown: true,
kill: false,
killPids: nil,
},
@@ -106,6 +129,7 @@
envv: nil,
dryrun: false,
remove: false,
+ chown: false,
kill: true,
killPids: []int{235, 451},
},
@@ -125,6 +149,7 @@
envv: nil,
dryrun: false,
remove: false,
+ chown: false,
kill: true,
killPids: nil,
},
@@ -145,6 +170,7 @@
envv: []string{"A=B"},
dryrun: true,
remove: false,
+ chown: false,
kill: false,
killPids: nil,
},
diff --git a/services/device/internal/suid/system.go b/services/device/internal/suid/system.go
index 6aad84b..285c4e3 100644
--- a/services/device/internal/suid/system.go
+++ b/services/device/internal/suid/system.go
@@ -39,8 +39,15 @@
return os.Chown(path, hw.uid, hw.gid)
}
- for _, p := range []string{hw.workspace, hw.logDir} {
+ chownPaths := hw.argv
+ if !hw.chown {
+ // Chown was invoked as part of regular suid execution, rather than directly
+ // via --chown. In that case, we chown the workspace and log directory
// TODO(rjkroege): Ensure that the device manager can read log entries.
+ chownPaths = []string{hw.workspace, hw.logDir}
+ }
+
+ for _, p := range chownPaths {
if err := filepath.Walk(p, chown); err != nil {
return verror.New(errChownFailed, nil, p, hw.uid, hw.gid, err)
}
