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.

Github user twilmes commented on the pull request:

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

Looks good.

VOTE: +1

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

Github user asfgit closed the pull request at:

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

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`?

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?

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

`.

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

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)`.

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!