Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Implemented
    • Affects Version/s: 3.0.2-incubating
    • Fix Version/s: 3.1.1-incubating
    • Component/s: groovy
    • Labels:
      None

      Description

      The sandboxing abstractions are not so good a set of building blocks as I'd once thought. Helper methods aren't in the right places and more flexibilty is required in managing methods/variables than just simple filters. Need to develop more concrete actions on this still.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user spmallette opened a pull request:

          https://github.com/apache/incubator-tinkerpop/pull/179

          TINKERPOP-891 Refactored the sandboxing abstractions for Gremlin Server

          https://issues.apache.org/jira/browse/TINKERPOP-891

          Deprecated the `SandboxExtension` and replaced it with the `AbstractSandboxExtension`. Took existing implementations and had them extend from the new `AbstractSandboxExtension`. Added a new "useful" implementation called `FileSandboxExtension` which gets its white list configuration from a file. Updated appropriate docs and wrote a number of tests for the new `FileSandboxExtension`.

          Both unit and integration tests pass. Also tested manually by configuring Gremlin Server to use the different sandboxes.

          VOTE: +1

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/apache/incubator-tinkerpop TINKERPOP-891

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/incubator-tinkerpop/pull/179.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #179


          commit a15b646ade8a106495dbbe29ba803218445bb20e
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2015-12-11T19:56:06Z

          Refactored the sandboxing abstractions for Gremlin Server.

          Deprecated the SandboxExtension and replaced it with the AbstractSandboxExtension. Took existing implementations and had them extend from the new AbstractSandboxExtension. Added a new "useful" implementation called FileSandboxExtension which gets its white list configuration from a file. Updated appropriate docs and wrote a number of tests for the new FileSandboxExtension.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user spmallette opened a pull request: https://github.com/apache/incubator-tinkerpop/pull/179 TINKERPOP-891 Refactored the sandboxing abstractions for Gremlin Server https://issues.apache.org/jira/browse/TINKERPOP-891 Deprecated the `SandboxExtension` and replaced it with the `AbstractSandboxExtension`. Took existing implementations and had them extend from the new `AbstractSandboxExtension`. Added a new "useful" implementation called `FileSandboxExtension` which gets its white list configuration from a file. Updated appropriate docs and wrote a number of tests for the new `FileSandboxExtension`. Both unit and integration tests pass. Also tested manually by configuring Gremlin Server to use the different sandboxes. VOTE: +1 You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/incubator-tinkerpop TINKERPOP-891 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-tinkerpop/pull/179.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #179 commit a15b646ade8a106495dbbe29ba803218445bb20e Author: Stephen Mallette <spmva@genoprime.com> Date: 2015-12-11T19:56:06Z Refactored the sandboxing abstractions for Gremlin Server. Deprecated the SandboxExtension and replaced it with the AbstractSandboxExtension. Took existing implementations and had them extend from the new AbstractSandboxExtension. Added a new "useful" implementation called FileSandboxExtension which gets its white list configuration from a file. Updated appropriate docs and wrote a number of tests for the new FileSandboxExtension.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user twilmes commented on the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/179#issuecomment-164474080

          Looks good.

          VOTE: +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user twilmes commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/179#issuecomment-164474080 Looks good. VOTE: +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/179#issuecomment-164529984

          • `mvn clean install`: worked
          • integration tests: worked

          VOTE: +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/179#issuecomment-164529984 `mvn clean install`: worked integration tests: worked VOTE: +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/incubator-tinkerpop/pull/179

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/incubator-tinkerpop/pull/179
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dpitera commented on the issue:

          https://github.com/apache/tinkerpop/pull/179

          @spmallette I am curious why this Pull Request got rid of the `methodBlackList`? I ask because I want to extend the `FileSandboxExtension` here to support blacklisting-- would you prefer I did the extension at that level, created another `FileSandboxWithBlacklistingExtension`, or did the extension at the `AbstractSandboxExtension`?

          Show
          githubbot ASF GitHub Bot added a comment - Github user dpitera commented on the issue: https://github.com/apache/tinkerpop/pull/179 @spmallette I am curious why this Pull Request got rid of the `methodBlackList`? I ask because I want to extend the `FileSandboxExtension` here to support blacklisting-- would you prefer I did the extension at that level, created another `FileSandboxWithBlacklistingExtension`, or did the extension at the `AbstractSandboxExtension`?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/179

          I don't see a reference to `methodBlackList` in this PR, but if we were to just reduce the question to why do we have whitelisting and no blacklisting then I think I could probably answer that. I'd rather not support blacklisting in TinkerPop, as it just seems to lead people into thinking they have a secure solution when they soon learn that they'd forgotten yet another harmful entry to blacklist.

          I think that the whitelist works really well in TinkerPop, because the base list of classes required to execute Gremlin is small (and really that's all we care about from TinkerPop's perspective). Whitelisting tends to work best in cases like this as it assumes everything is bad except for this small, easy to maintain list. Since whitelisting fits this situation so well, a blacklist feels a bit useless - extra code without purpose which we try to avoid.

          Obviously, we do have the `SimpleSandboxExtension` which does some basic blacklisting but it's mostly for demonstration and for basic protection from the worst of the worst `System.exit()`.

          Does that make sense?

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/179 I don't see a reference to `methodBlackList` in this PR, but if we were to just reduce the question to why do we have whitelisting and no blacklisting then I think I could probably answer that. I'd rather not support blacklisting in TinkerPop, as it just seems to lead people into thinking they have a secure solution when they soon learn that they'd forgotten yet another harmful entry to blacklist. I think that the whitelist works really well in TinkerPop, because the base list of classes required to execute Gremlin is small (and really that's all we care about from TinkerPop's perspective). Whitelisting tends to work best in cases like this as it assumes everything is bad except for this small, easy to maintain list. Since whitelisting fits this situation so well, a blacklist feels a bit useless - extra code without purpose which we try to avoid. Obviously, we do have the `SimpleSandboxExtension` which does some basic blacklisting but it's mostly for demonstration and for basic protection from the worst of the worst `System.exit()`. Does that make sense?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dpitera commented on the issue:

          https://github.com/apache/tinkerpop/pull/179

          > Whitelisting tends to work best in cases like this as it assumes everything is bad except for this small, easy to maintain list.

          Agreed. Which is what leads me to find myself in a situation where even things like `"^java\\.lang
          .String"` must be whitelisted to be called.

          However....["^java\\.lang\\.String#getBoolean\\("](http://docs.oracle.com/javase/6/docs/api/java/lang/Boolean.html#getBoolean(java.lang.String)) must be blacklisted because it leaks implementations details about the underlying System.

          I believe this is a prime example for situations where the best filter is something like:
          `!methodBlackList.any

          { descriptor =~ it } && methodWhiteList.any { descriptor =~ it }

          `.

          Would you agree?

          > I don't see a reference to methodBlackList in this PR

          This is because the methodBlackList of which I speak is part of the deprecated sadnbox extension classes

          Show
          githubbot ASF GitHub Bot added a comment - Github user dpitera commented on the issue: https://github.com/apache/tinkerpop/pull/179 > Whitelisting tends to work best in cases like this as it assumes everything is bad except for this small, easy to maintain list. Agreed. Which is what leads me to find myself in a situation where even things like `"^java\\.lang .String"` must be whitelisted to be called. However.... ["^java\\.lang\\.String#getBoolean\\("] ( http://docs.oracle.com/javase/6/docs/api/java/lang/Boolean.html#getBoolean(java.lang.String )) must be blacklisted because it leaks implementations details about the underlying System. I believe this is a prime example for situations where the best filter is something like: `!methodBlackList.any { descriptor =~ it } && methodWhiteList.any { descriptor =~ it } `. Would you agree? > I don't see a reference to methodBlackList in this PR This is because the methodBlackList of which I speak is part of the deprecated sadnbox extension classes
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/179

          That's a reasonable example. Could someone not just "blacklist" by whitelisting though? I just mean that the whitelisting system is all regex based. You could add negation to the whitelist and get rid of your troublesome method:

          ```text
          java\.lang\.Boolean#(?!getBoolean(String)).*
          ```

          That allows everything else on `Boolean` except `getBoolean(String)`.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/179 That's a reasonable example. Could someone not just "blacklist" by whitelisting though? I just mean that the whitelisting system is all regex based. You could add negation to the whitelist and get rid of your troublesome method: ```text java\.lang\.Boolean#(?!getBoolean(String)).* ``` That allows everything else on `Boolean` except `getBoolean(String)`.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dpitera commented on the issue:

          https://github.com/apache/tinkerpop/pull/179

          I will have to investigate what you've just claimed, but if this is true, then yes you are right... I'll get back to you when I've confirmed. Thanks for the suggestion!

          Show
          githubbot ASF GitHub Bot added a comment - Github user dpitera commented on the issue: https://github.com/apache/tinkerpop/pull/179 I will have to investigate what you've just claimed, but if this is true, then yes you are right... I'll get back to you when I've confirmed. Thanks for the suggestion!

            People

            • Assignee:
              spmallette stephen mallette
              Reporter:
              spmallette stephen mallette
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development