incubator-toree git commit: Distinguish between shutdown request and System.exit from cell

[lr...@apache.org]

Repository: incubator-toree
Updated Branches:
  refs/heads/master c96f67c9a -> 3b6eb2926


Distinguish between shutdown request and System.exit from cell

Prior to this change there were three ways the toree process
could be exited via deliberate actions.

- The user could enter dual cntrl-c (or send SIGINT) signals
  within 3 seconds of each other
- The user could issue a formal shutdown request from Jupyter
- The user could execute a cell of a notebook containing
  System.exit(n) or its equivalent

The first two use cases are considered valid and are permitted.
Use case three, however, should be prevented by design.

The issue being addressed is a conflict between the second and
third cases since both of those cases occur within the same
thread group (referred to as the restricted group).
Prior to this change, shutdown requests made via Jupyter
(vs. System.exit from within a cell) were not distinguished
from cell requests. Since the former is processed by the
ShutdownHandler while the latter is processed by the
ExecuteRequestHandler the change is to set a thread-local
residing in the KernelSecurityManager from the ShutdownHandler
immediately prior to its invocation of System.exit(0).
The KernelSecurityManager.checkExit() method then consults the
thread local to ensure this call came by way of the
ShutdownHandler and allows the exit to take place.
If the thread-local has not been set or has a value of false
and its within the restricted group, a security exception
is thrown to deny the exit attempt.

Closes #112


Project: http://git-wip-us.apache.org/repos/asf/incubator-toree/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-toree/commit/3b6eb292
Tree: http://git-wip-us.apache.org/repos/asf/incubator-toree/tree/3b6eb292
Diff: http://git-wip-us.apache.org/repos/asf/incubator-toree/diff/3b6eb292

Branch: refs/heads/master
Commit: 3b6eb29268fd0e5850d8a47f3ab33b62d506dab4
Parents: c96f67c
Author: Kevin Bates <ke...@us.ibm.com>
Authored: Sat Mar 11 08:44:22 2017 -0800
Committer: Luciano Resende <lr...@apache.org>
Committed: Tue Mar 14 09:23:57 2017 -0700

----------------------------------------------------------------------
 .../toree/security/KernelSecurityManager.scala  | 35 +++++++++++++++++---
 .../protocol/v5/handler/ShutdownHandler.scala   |  5 +++
 2 files changed, 36 insertions(+), 4 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-toree/blob/3b6eb292/kernel-api/src/main/scala/org/apache/toree/security/KernelSecurityManager.scala
----------------------------------------------------------------------
diff --git a/kernel-api/src/main/scala/org/apache/toree/security/KernelSecurityManager.scala b/kernel-api/src/main/scala/org/apache/toree/security/KernelSecurityManager.scala
index 3225261..fced5da 100644
--- a/kernel-api/src/main/scala/org/apache/toree/security/KernelSecurityManager.scala
+++ b/kernel-api/src/main/scala/org/apache/toree/security/KernelSecurityManager.scala
@@ -26,6 +26,11 @@ object KernelSecurityManager {
   val RestrictedGroupName = "restricted-" + UUID.randomUUID().toString
 
   /**
+    * ThreadLocal to indicate that an exit from a restricted group thread is permitted.
+    */
+  private val tlEnableRestrictedExit:ThreadLocal[Boolean] = new ThreadLocal[Boolean]
+
+  /**
    * Special case for this permission since the name changes with each status
    * code.
    */
@@ -60,6 +65,17 @@ object KernelSecurityManager {
    */
   private def shouldCheckPermissionSpecialCases(name: String): Boolean =
     name.startsWith(SystemExitPermissionName)
+
+  /**
+    * Sets tlEnableRestrictedExit to true if caller is in the restricted group.  This
+    * method is intended to be called exclusively from the ShutdownHandler since that's
+    * the only path by which we should permit System.exit to succeed within the notebook.
+    * Note that dual SIGINTs occur from a non-restricted thread group and are also permitted.
+    */
+  def enableRestrictedExit() {
+    val currentGroup = Thread.currentThread().getThreadGroup
+    tlEnableRestrictedExit.set(currentGroup.getName == RestrictedGroupName)
+  }
 }
 
 class KernelSecurityManager extends SecurityManager {
@@ -117,10 +133,21 @@ class KernelSecurityManager extends SecurityManager {
   override def checkExit(status: Int): Unit = {
     val currentGroup = Thread.currentThread().getThreadGroup
 
-    if (currentGroup.getName == RestrictedGroupName) {
-      // TODO: Determine why System.exit(...) is being blocked in the ShutdownHandler
-      System.out.println("Unauthorized system.exit detected!")
-      //throw new SecurityException("Not allowed to invoke System.exit!")
+    // Exit will be denied if this thread is in the restricted group AND the restricted
+    // exit thread-local has not been enabled.  This will only happen when the request
+    // came from jupyter via a (cell) execution_request indicating the cell is attempting
+    // an exit call.  Proper notebook shutdown requests will go through the ShutdownHandler
+    // where restricted exits are enabled and SIGINT (ctrl-c) requests are not performed
+    // via the restricted group and thus allowed.
+    //
+    if (currentGroup.getName == RestrictedGroupName ) {
+      val isRestrictedExitEnabled:Option[Boolean] = Some(tlEnableRestrictedExit.get())
+
+      if ( !isRestrictedExitEnabled.getOrElse(false) ) {
+        throw new SecurityException("Not allowed to invoke System.exit!")
+      }
     }
+    // Just in case this thread remains alive - force it to pass through ShutdownHandler
+    tlEnableRestrictedExit.set(false)
   }
 }

http://git-wip-us.apache.org/repos/asf/incubator-toree/blob/3b6eb292/kernel/src/main/scala/org/apache/toree/kernel/protocol/v5/handler/ShutdownHandler.scala
----------------------------------------------------------------------
diff --git a/kernel/src/main/scala/org/apache/toree/kernel/protocol/v5/handler/ShutdownHandler.scala b/kernel/src/main/scala/org/apache/toree/kernel/protocol/v5/handler/ShutdownHandler.scala
index 828f080..a31d7b3 100644
--- a/kernel/src/main/scala/org/apache/toree/kernel/protocol/v5/handler/ShutdownHandler.scala
+++ b/kernel/src/main/scala/org/apache/toree/kernel/protocol/v5/handler/ShutdownHandler.scala
@@ -21,6 +21,7 @@ import org.apache.toree.comm.{CommRegistrar, CommStorage, KernelCommWriter}
 import org.apache.toree.kernel.protocol.v5.content.{ShutdownReply, ShutdownRequest, CommOpen}
 import org.apache.toree.kernel.protocol.v5.kernel.{ActorLoader, Utilities}
 import org.apache.toree.kernel.protocol.v5._
+import org.apache.toree.security.KernelSecurityManager
 import org.apache.toree.utils.MessageLogSupport
 import play.api.data.validation.ValidationError
 import play.api.libs.json.JsPath
@@ -58,6 +59,10 @@ class ShutdownHandler(
 
     logger.debug("Attempting graceful shutdown.")
     actorLoader.load(SystemActorType.KernelMessageRelay) ! kernelResponseMessage
+
+    // Instruct security manager that exit should be allowed
+    KernelSecurityManager.enableRestrictedExit()
+
     System.exit(0)
   }