veyron/services/mgmt/suidhelper: prep for go1.4
Update suidhelper for correct operation on go1.4 and make better use
of Go's syscall interface to reduce likelyhood of flake bugs.
Change-Id: I3f9083a0de5cf020e52f2844f2a28927e15c73ac
diff --git a/services/mgmt/device/impl/app_service.go b/services/mgmt/device/impl/app_service.go
index 940df40..1538c98 100644
--- a/services/mgmt/device/impl/app_service.go
+++ b/services/mgmt/device/impl/app_service.go
@@ -119,7 +119,6 @@
"io/ioutil"
"os"
"os/exec"
- "os/user"
"path"
"path/filepath"
"reflect"
@@ -677,56 +676,6 @@
return instanceDir, instanceID, nil
}
-// isSetuid is defined like this so we can override its
-// implementation for tests.
-var isSetuid = func(fileStat os.FileInfo) bool {
- vlog.VI(2).Infof("running the original isSetuid")
- return fileStat.Mode()&os.ModeSetuid == os.ModeSetuid
-}
-
-// systemAccountForHelper returns the uname that the helper uses to invoke the
-// application. If the helper exists and is setuid, the device manager
-// requires that there is a uname associated with the Veyron
-// identity that requested starting an application.
-// TODO(rjkroege): This function assumes a desktop installation target
-// and is probably not a good fit in other contexts. Revisit the design
-// as appropriate. This function also internalizes a decision as to when
-// it is possible to start an application that needs to be made explicit.
-func systemAccountForHelper(ctx ipc.ServerContext, helperPath string, uat BlessingSystemAssociationStore) (systemName string, err error) {
- identityNames := ctx.RemoteBlessings().ForContext(ctx)
- helperStat, err := os.Stat(helperPath)
- if err != nil {
- vlog.Errorf("Stat(%v) failed: %v. helper is required.", helperPath, err)
- return "", verror2.Make(ErrOperationFailed, ctx.Context())
- }
- haveHelper := isSetuid(helperStat)
- systemName, present := uat.SystemAccountForBlessings(identityNames)
-
- switch {
- case haveHelper && present:
- return systemName, nil
- case haveHelper && !present:
- // The helper is owned by the device manager and installed as
- // setuid root. Therefore, the device manager must never run an
- // app as itself to prevent an app trivially granting itself
- // root permissions. There must be an associated uname for the
- // account in this case.
- return "", verror2.Make(verror2.NoAccess, ctx.Context(), "use of setuid helper requires an associated uname.")
- case !haveHelper:
- // When the helper is not setuid, the helper can't change the
- // app's uid so just run the app as the device manager's uname
- // whether or not there is an association.
- vlog.VI(1).Infof("helper not setuid. Device manager will invoke app with its own userid")
- user, err := user.Current()
- if err != nil {
- vlog.Errorf("user.Current() failed: %v", err)
- return "", verror2.Make(ErrOperationFailed, ctx.Context())
- }
- return user.Username, nil
- }
- return "", verror2.Make(ErrOperationFailed, ctx.Context())
-}
-
func genCmd(instanceDir, helperPath, systemName string, nsRoots []string) (*exec.Cmd, error) {
versionLink := filepath.Join(instanceDir, "version")
versionDir, err := filepath.EvalSymlinks(versionLink)
@@ -745,7 +694,15 @@
}
cmd := exec.Command(helperPath)
- cmd.Args = append(cmd.Args, "--username", systemName)
+
+ switch yes, err := suidHelper.suidhelperEnabled(systemName, helperPath); {
+ case err != nil:
+ return nil, err
+ case yes:
+ cmd.Args = append(cmd.Args, "--username", systemName)
+ case !yes:
+ cmd.Args = append(cmd.Args, "--username", systemName, "--dryrun")
+ }
var nsRootEnvs []string
for i, r := range nsRoots {
@@ -908,12 +865,7 @@
return nil, err
}
- systemName, err := systemAccountForHelper(call, helper, i.uat)
- if err != nil {
- cleanupDir(instanceDir, helper)
- return nil, err
- }
-
+ systemName := suidHelper.usernameForPrincipal(call, i.uat)
if err := saveSystemNameForInstance(instanceDir, systemName); err != nil {
cleanupDir(instanceDir, helper)
return nil, err
@@ -956,11 +908,7 @@
return err
}
- systemName, err := systemAccountForHelper(call, i.config.Helper, i.uat)
- if err != nil {
- return err
- }
-
+ systemName := suidHelper.usernameForPrincipal(call, i.uat)
startSystemName, err := readSystemNameForInstance(instanceDir)
if err != nil {
return err
diff --git a/services/mgmt/device/impl/helper_manager.go b/services/mgmt/device/impl/helper_manager.go
new file mode 100644
index 0000000..9e935d2
--- /dev/null
+++ b/services/mgmt/device/impl/helper_manager.go
@@ -0,0 +1,69 @@
+package impl
+
+import (
+ "os"
+ "os/user"
+
+ "v.io/core/veyron2/ipc"
+ "v.io/core/veyron2/verror2"
+ "v.io/core/veyron2/vlog"
+)
+
+type suidHelperState string
+
+var suidHelper suidHelperState
+
+func init() {
+ u, err := user.Current()
+ if err != nil {
+ vlog.Panicf("devicemanager has no current user: %v", err)
+ }
+ suidHelper = suidHelperState(u.Username)
+}
+
+// isSetuid is defined like this so we can override its
+// implementation for tests.
+var isSetuid = func(fileStat os.FileInfo) bool {
+ vlog.VI(2).Infof("running the original isSetuid")
+ return fileStat.Mode()&os.ModeSetuid == os.ModeSetuid
+}
+
+// unameRequiresSuidhelper determines if the the suidhelper must exist
+// and be setuid to run an application as system user un. If false, the
+// setuidhelper must be invoked with the --dryrun flag to skip making
+// system calls that will fail or provide apps with a trivial
+// priviledge escalation.
+func (dn suidHelperState) suidhelperEnabled(un, helperPath string) (bool, error) {
+ helperStat, err := os.Stat(helperPath)
+ if err != nil {
+ vlog.Errorf("Stat(%v) failed: %v. helper is required.", helperPath, err)
+ return false, verror2.Make(ErrOperationFailed, nil)
+ }
+ haveHelper := isSetuid(helperStat)
+
+ switch {
+ case haveHelper && string(dn) != un:
+ return true, nil
+ case haveHelper && string(dn) == un:
+ return false, verror2.Make(verror2.NoAccess, nil)
+ default:
+ return false, nil
+ }
+ // Keep the compiler happy.
+ return false, nil
+}
+
+// usernameForVanadiumPrincipal returns the system name that the
+// devicemanager will use to invoke apps.
+// TODO(rjkroege): This code assumes a desktop target and will need
+// to be reconsidered for embedded contexts.
+func (i suidHelperState) usernameForPrincipal(ctx ipc.ServerContext, uat BlessingSystemAssociationStore) string {
+ identityNames := ctx.RemoteBlessings().ForContext(ctx)
+ systemName, present := uat.SystemAccountForBlessings(identityNames)
+
+ if present {
+ return systemName
+ } else {
+ return string(i)
+ }
+}
diff --git a/services/mgmt/suidhelper/impl/args.go b/services/mgmt/suidhelper/impl/args.go
index 67003a3..185bae8 100644
--- a/services/mgmt/suidhelper/impl/args.go
+++ b/services/mgmt/suidhelper/impl/args.go
@@ -36,7 +36,7 @@
var (
flagUsername, flagWorkspace, flagLogDir, flagRun *string
flagMinimumUid *int64
- flagRemove *bool
+ flagRemove, flagDryrun *bool
)
func init() {
@@ -53,6 +53,7 @@
flagRun = sflag.Run
flagMinimumUid = sflag.MinimumUid
flagRemove = sflag.Remove
+ flagDryrun = sflag.Dryrun
}
// ParseArguments populates the WorkParameter object from the provided args
@@ -88,6 +89,8 @@
return fmt.Errorf("suidhelper does not permit uids less than %d", *flagMinimumUid)
}
+ wp.dryrun = *flagDryrun
+
// Preserve the arguments for examination by the test harness if executed
// in the course of a test.
if os.Getenv("VEYRON_SUIDHELPER_TEST") != "" {
diff --git a/services/mgmt/suidhelper/impl/args_test.go b/services/mgmt/suidhelper/impl/args_test.go
index 9358e9f..9341fc7 100644
--- a/services/mgmt/suidhelper/impl/args_test.go
+++ b/services/mgmt/suidhelper/impl/args_test.go
@@ -85,6 +85,24 @@
fmt.Errorf("suidhelper does not permit uids less than 501"),
WorkParameters{},
},
+
+ {
+ []string{"setuidhelper", "--minuid", "1", "--username", testUserName, "--workspace", "/hello",
+ "--logdir", "/logging", "--run", "/bin/veyron", "--dryrun", "--", "one", "two"},
+ []string{"A=B"},
+ nil,
+ WorkParameters{
+ uid: testUid,
+ gid: testGid,
+ workspace: "/hello",
+ logDir: "/logging",
+ argv0: "/bin/veyron",
+ argv: []string{"/bin/veyron", "one", "two"},
+ envv: []string{"A=B"},
+ dryrun: true,
+ remove: false,
+ },
+ },
}
for _, c := range cases {
diff --git a/services/mgmt/suidhelper/impl/flag/flag.go b/services/mgmt/suidhelper/impl/flag/flag.go
index f8b53e1..273f56b 100644
--- a/services/mgmt/suidhelper/impl/flag/flag.go
+++ b/services/mgmt/suidhelper/impl/flag/flag.go
@@ -15,7 +15,7 @@
var (
Username, Workspace, LogDir, Run *string
MinimumUid *int64
- Remove *bool
+ Remove, Dryrun *bool
)
func init() {
@@ -29,6 +29,7 @@
Run = fs.String("run", "", "Path to the application to exec.")
MinimumUid = fs.Int64("minuid", uidThreshold, "UIDs cannot be less than this number.")
Remove = fs.Bool("rm", false, "Remove the file trees given as command-line arguments.")
+ Dryrun = fs.Bool("dryrun", false, "Elides root-requiring systemcalls.")
}
const uidThreshold = 501
diff --git a/services/mgmt/suidhelper/impl/system.go b/services/mgmt/suidhelper/impl/system.go
index 60a7b92..8a93d91 100644
--- a/services/mgmt/suidhelper/impl/system.go
+++ b/services/mgmt/suidhelper/impl/system.go
@@ -34,22 +34,37 @@
}
func (hw *WorkParameters) Exec() error {
+ attr := new(syscall.ProcAttr)
+
+ if dir, err := os.Getwd(); err != nil {
+ log.Printf("error Getwd(): %v\n", err)
+ return fmt.Errorf("os.Getwd failed: %v", err)
+ attr.Dir = dir
+ }
+ attr.Env = hw.envv
+
if hw.dryrun {
log.Printf("[dryrun] syscall.Setgid(%d)", hw.gid)
log.Printf("[dryrun] syscall.Setuid(%d)", hw.uid)
} else {
- // NOTE(caprita): Commenting this out since it's broken with go
- // 1.4, to make the integration test pass. go/vcl/8240 will fix
- // it properly.
-
- // if err := syscall.Setgid(hw.gid); err != nil {
- // return fmt.Errorf("syscall.Setgid(%d) failed: %v", hw.gid, err)
- // }
- // if err := syscall.Setuid(hw.uid); err != nil {
- // return fmt.Errorf("syscall.Setuid(%d) failed: %v", hw.uid, err)
- // }
+ attr.Sys = new(syscall.SysProcAttr)
+ attr.Sys.Credential = new(syscall.Credential)
+ attr.Sys.Credential.Gid = uint32(hw.gid)
+ attr.Sys.Credential.Uid = uint32(hw.uid)
}
- return syscall.Exec(hw.argv0, hw.argv, hw.envv)
+
+ _, _, err := syscall.StartProcess(hw.argv0, hw.argv, attr)
+ if err != nil {
+ if !hw.dryrun {
+ log.Printf("StartProcess failed: attr: %#v, attr.Sys: %#v, attr.Sys.Cred: %#v error: %v\n", attr, attr.Sys, attr.Sys.Credential, err)
+ } else {
+ log.Printf("StartProcess failed: %v", err)
+ }
+ return fmt.Errorf("syscall.StartProcess(%s) failed: %v", hw.argv0, err)
+ }
+ // TODO(rjkroege): Return the pid to the node manager.
+ os.Exit(0)
+ return nil // Not reached.
}
func (hw *WorkParameters) Remove() error {
