veyron/services/mgmt/node/impl: Resume detecta changed associations.
The node manager uses the setuid helper to run application instances with
an associated system name. Resuming an application under a different system
name may result in application failure when a resumed application discovers
that it can no longer access its previously created filesystem state. This
change detects this case and refuses to Resume applications under this
circumstance.
Change-Id: Ic0f317455460456c8f3faf57386fcfb932a36513
diff --git a/services/mgmt/node/impl/app_invoker.go b/services/mgmt/node/impl/app_invoker.go
index 1171507..e26b2bb 100644
--- a/services/mgmt/node/impl/app_invoker.go
+++ b/services/mgmt/node/impl/app_invoker.go
@@ -26,6 +26,7 @@
// info - app manager name and process id for the instance (if running)
// version - symbolic link to installation version for the instance
// <status> - one of the values for instanceState enum
+// systemname - the system name used to execute this instance
// instance-<id b>
// ...
// installation-<id 2>
@@ -404,7 +405,12 @@
// 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(helperStat os.FileInfo, identityNames []string, uat BlessingSystemAssociationStore) (systemName string, err error) {
+func systemAccountForHelper(helperPath string, identityNames []string, uat BlessingSystemAssociationStore) (systemName string, err error) {
+ helperStat, err := os.Stat(helperPath)
+ if err != nil {
+ vlog.Errorf("Stat(%v) failed: %v. helper is required.", helperPath, err)
+ return "", errOperationFailed
+ }
haveHelper := isSetuid(helperStat)
systemName, present := uat.SystemAccountForBlessings(identityNames)
@@ -434,7 +440,7 @@
// TODO(rjkroege): Turning on the setuid feature of the suidhelper
// requires an installer with root permissions to install it in <config.Root>/helper
-func genCmd(instanceDir string, helperPath string, uat BlessingSystemAssociationStore, identityNames []string) (*exec.Cmd, error) {
+func genCmd(instanceDir, helperPath, systemName string) (*exec.Cmd, error) {
versionLink := filepath.Join(instanceDir, "version")
versionDir, err := filepath.EvalSymlinks(versionLink)
if err != nil {
@@ -451,19 +457,8 @@
return nil, errOperationFailed
}
- helperStat, err := os.Stat(helperPath)
- if err != nil {
- vlog.Errorf("Stat(%v) failed: %v. helper is required.", helperPath, err)
- return nil, errOperationFailed
- }
cmd := exec.Command(helperPath)
-
- cmd.Args = append(cmd.Args, "--username")
- uname, err := systemAccountForHelper(helperStat, identityNames, uat)
- if err != nil {
- return nil, err
- }
- cmd.Args = append(cmd.Args, uname)
+ cmd.Args = append(cmd.Args, "--username", systemName)
// TODO(caprita): Also pass in configuration info like NAMESPACE_ROOT to
// the app (to point to the device mounttable).
@@ -473,8 +468,7 @@
return nil, err
}
cmd.Dir = rootDir
- cmd.Args = append(cmd.Args, "--workspace")
- cmd.Args = append(cmd.Args, rootDir)
+ cmd.Args = append(cmd.Args, "--workspace", rootDir)
logDir := filepath.Join(instanceDir, "logs")
if err := mkdir(logDir); err != nil {
@@ -485,18 +479,14 @@
if cmd.Stdout, err = openWriteFile(stdoutLog); err != nil {
return nil, err
}
- cmd.Args = append(cmd.Args, "--stdoutlog")
- cmd.Args = append(cmd.Args, stdoutLog)
+ cmd.Args = append(cmd.Args, "--stdoutlog", stdoutLog)
stderrLog := filepath.Join(logDir, fmt.Sprintf("STDERR-%d", timestamp))
if cmd.Stderr, err = openWriteFile(stderrLog); err != nil {
return nil, err
}
- cmd.Args = append(cmd.Args, "--stderrlog")
- cmd.Args = append(cmd.Args, stderrLog)
-
- cmd.Args = append(cmd.Args, "--run")
- cmd.Args = append(cmd.Args, binPath)
+ cmd.Args = append(cmd.Args, "--stderrlog", stderrLog)
+ cmd.Args = append(cmd.Args, "--run", binPath)
cmd.Args = append(cmd.Args, "--")
// Set up args and env.
@@ -548,11 +538,12 @@
return nil
}
-func (i *appInvoker) run(instanceDir string, blessings []string) error {
+func (i *appInvoker) run(instanceDir, systemName, helper string) error {
if err := transitionInstance(instanceDir, suspended, starting); err != nil {
return err
}
- cmd, err := genCmd(instanceDir, filepath.Join(i.config.Root, "helper"), i.uat, blessings)
+
+ cmd, err := genCmd(instanceDir, helper, systemName)
if err == nil {
err = i.startCmd(instanceDir, cmd)
}
@@ -565,13 +556,27 @@
func (i *appInvoker) Start(call ipc.ServerContext) ([]string, error) {
instanceDir, instanceID, err := i.newInstance()
- if err == nil {
- err = i.run(instanceDir, call.RemoteBlessings().ForContext(call))
- }
if err != nil {
cleanupDir(instanceDir)
return nil, err
}
+
+ helper := filepath.Join(i.config.Root, "helper")
+ systemName, err := systemAccountForHelper(helper, call.RemoteBlessings().ForContext(call), i.uat)
+ if err != nil {
+ cleanupDir(instanceDir)
+ return nil, err
+ }
+
+ if err := saveSystemNameForInstance(instanceDir, systemName); err != nil {
+ cleanupDir(instanceDir)
+ return nil, err
+ }
+
+ if err = i.run(instanceDir, systemName, helper); err != nil {
+ cleanupDir(instanceDir)
+ return nil, err
+ }
return []string{instanceID}, nil
}
@@ -589,13 +594,27 @@
return instanceDir, nil
}
-// TODO(rjkroege): Only the original invoking identity may resume an application.
func (i *appInvoker) Resume(call ipc.ServerContext) error {
instanceDir, err := i.instanceDir()
if err != nil {
return err
}
- return i.run(instanceDir, call.RemoteBlessings().ForContext(call))
+
+ helper := filepath.Join(i.config.Root, "helper")
+ systemName, err := systemAccountForHelper(helper, call.RemoteBlessings().ForContext(call), i.uat)
+ if err != nil {
+ return err
+ }
+
+ startSystemName, err := readSystemNameForInstance(instanceDir)
+ if err != nil {
+ return err
+ }
+
+ if startSystemName != systemName {
+ return verror.NoAccessf("Not allowed to resume an application under a different system name.")
+ }
+ return i.run(instanceDir, systemName, helper)
}
func stopAppRemotely(appVON string) error {
diff --git a/services/mgmt/node/impl/args_darwin_test.go b/services/mgmt/node/impl/args_darwin_test.go
index bf4b58d..e42e887 100644
--- a/services/mgmt/node/impl/args_darwin_test.go
+++ b/services/mgmt/node/impl/args_darwin_test.go
@@ -1,5 +1,6 @@
package impl_test
const (
- testUserName = "_uucp"
+ testUserName = "_uucp"
+ anotherTestUserName = "_lp"
)
diff --git a/services/mgmt/node/impl/args_linux_test.go b/services/mgmt/node/impl/args_linux_test.go
index 07c557f..4b1e0f8 100644
--- a/services/mgmt/node/impl/args_linux_test.go
+++ b/services/mgmt/node/impl/args_linux_test.go
@@ -1,5 +1,6 @@
package impl_test
const (
- testUserName = "uucp"
+ testUserName = "uucp"
+ anotherTestUserName = "lp"
)
diff --git a/services/mgmt/node/impl/associate_instance_test.go b/services/mgmt/node/impl/associate_instance_test.go
new file mode 100644
index 0000000..317c881
--- /dev/null
+++ b/services/mgmt/node/impl/associate_instance_test.go
@@ -0,0 +1,28 @@
+package impl
+
+import (
+ "io/ioutil"
+ "os"
+ "testing"
+)
+
+func TestSystemNameState(t *testing.T) {
+ dir, err := ioutil.TempDir("", "instance")
+ if err != nil {
+ t.Fatalf("Failed to create temp dir: %v", err)
+ }
+ defer os.RemoveAll(dir)
+
+ expected := "vanadium-user"
+ if err := saveSystemNameForInstance(dir, expected); err != nil {
+ t.Fatalf("saveSystemNameForInstance(%v, %v) failed: %v", dir, expected, err)
+ }
+
+ got, err := readSystemNameForInstance(dir)
+ if err != nil {
+ t.Fatalf("readSystemNameForInstance(%v) failed: ", err)
+ }
+ if got != expected {
+ t.Fatalf("got %v, expected %v", got, expected)
+ }
+}
diff --git a/services/mgmt/node/impl/association_instance.go b/services/mgmt/node/impl/association_instance.go
new file mode 100644
index 0000000..02ae3a7
--- /dev/null
+++ b/services/mgmt/node/impl/association_instance.go
@@ -0,0 +1,30 @@
+package impl
+
+// Code to manage the persistence of which systemName is associated with
+// a given application instance.
+
+import (
+ "io/ioutil"
+ "path/filepath"
+
+ "veyron.io/veyron/veyron2/vlog"
+)
+
+func saveSystemNameForInstance(dir, systemName string) error {
+ snp := filepath.Join(dir, "systemname")
+ if err := ioutil.WriteFile(snp, []byte(systemName), 0600); err != nil {
+ vlog.Errorf("WriteFile(%v, %v) failed: %v", snp, systemName, err)
+ return errOperationFailed
+ }
+ return nil
+}
+
+func readSystemNameForInstance(dir string) (string, error) {
+ snp := filepath.Join(dir, "systemname")
+ name, err := ioutil.ReadFile(snp)
+ if err != nil {
+ vlog.Errorf("ReadFile(%v) failed: %v", snp, err)
+ return "", errOperationFailed
+ }
+ return string(name), nil
+}
diff --git a/services/mgmt/node/impl/impl_test.go b/services/mgmt/node/impl/impl_test.go
index 9fcee59..baaf81a 100644
--- a/services/mgmt/node/impl/impl_test.go
+++ b/services/mgmt/node/impl/impl_test.go
@@ -1285,7 +1285,11 @@
t.Fatalf("AssociateAccount failed %v", err)
}
// Add Start to the ACL list for root/other.
- newACL := security.ACL{In: map[security.BlessingPattern]security.LabelSet{"root/other/...": security.AllLabels}}
+ newACL, _, err := nodeStub.GetACL(selfRT.NewContext())
+ if err != nil {
+ t.Fatalf("GetACL failed %v", err)
+ }
+ newACL.In["root/other/..."] = security.AllLabels
if err = nodeStub.SetACL(selfRT.NewContext(), newACL, ""); err != nil {
t.Fatalf("SetACL failed %v", err)
}
@@ -1293,5 +1297,28 @@
vlog.VI(2).Infof("other attempting to run an app with access. Should succeed.")
instance2ID := startApp(t, appID, otherRT)
verifyHelperArgs(t, <-pingCh, testUserName) // Wait until the app pings us that it's ready.
+ suspendApp(t, appID, instance2ID, otherRT)
+
+ vlog.VI(2).Infof("Verify that Resume with the same systemName works.")
+ resumeApp(t, appID, instance2ID, otherRT)
+ verifyHelperArgs(t, <-pingCh, testUserName) // Wait until the app pings us that it's ready.
+ suspendApp(t, appID, instance2ID, otherRT)
+
+ // Change the associated system name.
+ if err = nodeStub.AssociateAccount(selfRT.NewContext(), []string{"root/other"}, anotherTestUserName); err != nil {
+ t.Fatalf("AssociateAccount failed %v", err)
+ }
+
+ vlog.VI(2).Infof("Show that Resume with a different systemName fails.")
+ resumeAppExpectError(t, appID, instance2ID, verror.NoAccess, otherRT)
+
+ // Clean up.
stopApp(t, appID, instance2ID, otherRT)
+
+ vlog.VI(2).Infof("Show that Start with different systemName works.")
+ instance3ID := startApp(t, appID, otherRT)
+ verifyHelperArgs(t, <-pingCh, anotherTestUserName) // Wait until the app pings us that it's ready.
+
+ // Clean up.
+ stopApp(t, appID, instance3ID, otherRT)
}
diff --git a/services/mgmt/node/impl/util_test.go b/services/mgmt/node/impl/util_test.go
index fd543dd..8c93dba 100644
--- a/services/mgmt/node/impl/util_test.go
+++ b/services/mgmt/node/impl/util_test.go
@@ -252,6 +252,12 @@
}
}
+func resumeAppExpectError(t *testing.T, appID, instanceID string, expectedError verror.ID, opt ...veyron2.Runtime) {
+ if err := appStub(t, appID, instanceID).Resume(ort(opt).NewContext()); err == nil || !verror.Is(err, expectedError) {
+ t.Fatalf("Resume(%v/%v) expected to fail with %v, got %v instead", appID, instanceID, expectedError, err)
+ }
+}
+
func updateApp(t *testing.T, appID string, opt ...veyron2.Runtime) {
if err := appStub(t, appID).Update(ort(opt).NewContext()); err != nil {
t.Fatalf("Update(%v) failed: %v", appID, err)
