veyron/services/mgmt/node/impl: Addressing Ankur's comments on go/vcl/6576.
Change-Id: Ide81f39d36f4c9f327553f9be92dd3c1491631a3
diff --git a/services/mgmt/node/impl/app_invoker.go b/services/mgmt/node/impl/app_invoker.go
index 55882ae..47104a5 100644
--- a/services/mgmt/node/impl/app_invoker.go
+++ b/services/mgmt/node/impl/app_invoker.go
@@ -378,8 +378,8 @@
return installationDir, nil
}
-// setupIdentity sets up the instance's principal, with the right blessings.
-func setupIdentity(instanceDir, versionDir string, call ipc.ServerContext, info *instanceInfo) error {
+// setupPrincipal sets up the instance's principal, with the right blessings.
+func setupPrincipal(instanceDir, versionDir string, call ipc.ServerContext, info *instanceInfo) error {
credentialsDir := filepath.Join(instanceDir, "credentials")
// TODO(caprita): The app's system user id needs access to this dir.
// Use the suidhelper to chown it.
@@ -400,8 +400,12 @@
nmPrincipal := call.LocalPrincipal()
// Take the blessings conferred upon us by the Start-er, extend them
// with the app title.
+ grantedBlessings := call.Blessings()
+ if grantedBlessings == nil {
+ return errInvalidBlessing
+ }
// TODO(caprita): Revisit UnconstrainedUse.
- appBlessings, err := nmPrincipal.Bless(p.PublicKey(), call.Blessings(), envelope.Title, security.UnconstrainedUse())
+ appBlessings, err := nmPrincipal.Bless(p.PublicKey(), grantedBlessings, envelope.Title, security.UnconstrainedUse())
if err != nil {
vlog.Errorf("Bless() failed: %v", err)
return errOperationFailed
@@ -422,6 +426,17 @@
}
// In addition, we give the app separate blessings for the purpose of
// communicating with the node manager.
+ //
+ // NOTE(caprita/ataly): Giving the app an unconstrained blessing from
+ // the node manager's default presents the app with a blessing that's
+ // potentially more powerful than what is strictly needed to allow
+ // communication between node manager and app (which could be more
+ // narrowly accomplished by using a custom-purpose self-signed blessing
+ // that the node manger produces solely to talk to the app).
+ //
+ // TODO(caprita): Figure out if there is any feature value in providing
+ // the app with a node manager-derived blessing (e.g., may the app need
+ // to prove it's running on the node?).
nmBlessings, err := nmPrincipal.Bless(p.PublicKey(), nmPrincipal.BlessingStore().Default(), "callback", security.UnconstrainedUse())
// Put the names of the node manager's default blessings as patterns for
// the child, so that the child uses the right blessing when talking
@@ -480,7 +495,7 @@
return instanceDir, instanceID, errOperationFailed
}
instanceInfo := new(instanceInfo)
- if err := setupIdentity(instanceDir, versionDir, call, instanceInfo); err != nil {
+ if err := setupPrincipal(instanceDir, versionDir, call, instanceInfo); err != nil {
return instanceDir, instanceID, err
}
if err := saveInstanceInfo(instanceDir, instanceInfo); err != nil {
diff --git a/services/mgmt/node/impl/impl_test.go b/services/mgmt/node/impl/impl_test.go
index 9b1f878..7c8c79f 100644
--- a/services/mgmt/node/impl/impl_test.go
+++ b/services/mgmt/node/impl/impl_test.go
@@ -565,6 +565,12 @@
// Install the app.
appID := installApp(t)
+
+ // Start requires the caller to grant a blessing for the app instance.
+ if _, err := startAppImpl(t, appID, ""); err == nil || !verror.Is(err, verror.BadArg) {
+ t.Fatalf("Start(%v) expected to fail with %v, got %v instead", appID, verror.BadArg, err)
+ }
+
// Start an instance of the app.
instance1ID := startApp(t, appID)
diff --git a/services/mgmt/node/impl/util_test.go b/services/mgmt/node/impl/util_test.go
index b9b9ad3..4e8a866 100644
--- a/services/mgmt/node/impl/util_test.go
+++ b/services/mgmt/node/impl/util_test.go
@@ -240,8 +240,12 @@
return g.p.Bless(other.PublicKey(), g.p.BlessingStore().Default(), g.extension, security.UnconstrainedUse())
}
-func startAppImpl(t *testing.T, appID string, opt []veyron2.Runtime) (string, error) {
- if instanceIDs, err := appStub(appID).Start(ort(opt).NewContext(), &granter{p: ort(opt).Principal(), extension: "forapp"}); err != nil {
+func startAppImpl(t *testing.T, appID, grant string, opt ...veyron2.Runtime) (string, error) {
+ var opts []ipc.CallOpt
+ if grant != "" {
+ opts = append(opts, &granter{p: ort(opt).Principal(), extension: grant})
+ }
+ if instanceIDs, err := appStub(appID).Start(ort(opt).NewContext(), opts...); err != nil {
return "", err
} else {
if want, got := 1, len(instanceIDs); want != got {
@@ -252,7 +256,7 @@
}
func startApp(t *testing.T, appID string, opt ...veyron2.Runtime) string {
- instanceID, err := startAppImpl(t, appID, opt)
+ instanceID, err := startAppImpl(t, appID, "forapp", opt...)
if err != nil {
t.Fatalf("%s: Start(%v) failed: %v", loc(1), appID, err)
}
@@ -260,7 +264,7 @@
}
func startAppExpectError(t *testing.T, appID string, expectedError verror.ID, opt ...veyron2.Runtime) {
- if _, err := startAppImpl(t, appID, opt); err == nil || !verror.Is(err, expectedError) {
+ if _, err := startAppImpl(t, appID, "forapp", opt...); err == nil || !verror.Is(err, expectedError) {
t.Fatalf("%s: Start(%v) expected to fail with %v, got %v instead", loc(1), appID, expectedError, err)
}
}
