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)
 	}
 }