You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by krlohnes <gi...@git.apache.org> on 2017/03/24 15:32:41 UTC
[GitHub] tinkerpop pull request #583: TINKERPOP-1657 Provide abstraction to easily al...
GitHub user krlohnes opened a pull request:
https://github.com/apache/tinkerpop/pull/583
TINKERPOP-1657 Provide abstraction to easily allow different HttpAuth schemes
Abstracting over the http authentication allows for easy extensibility
for users/implementors to provide their own classes for http auth beyond
basic auth. The general issue is that there is a fixed overhead to
hashing passwords securely. This change allows for implementing things
like HMAC token auth and plugging them in easily to the gremlin server.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/krlohnes/tinkerpop abstraction_for_different_http_auths
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/583.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 #583
----
commit 1c7a4b18fd63f30c9da9a2b3a211c8f6fad1c596
Author: Keith Lohnes <kr...@us.ibm.com>
Date: 2017-03-20T17:37:40Z
Abstract over http auth for extensibility
Abstracting over the http authentication allows for easy extensibility
for users/implementors to provide their own classes for http auth beyond
basic auth. The general issue is that there is a fixed overhead to
hashing passwords securely. This change allows for implementing things
like HMAC token auth and plugging them in easily to the gremlin server.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #583: TINKERPOP-1657 Provide abstraction to easily al...
Posted by krlohnes <gi...@git.apache.org>.
Github user krlohnes commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/583#discussion_r108750337
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Settings.java ---
@@ -387,6 +387,18 @@ public SerializerSettings() {}
public String className = AllowAllAuthenticator.class.getName();
/**
+ * Enable audit logging of authenticated users and gremlin evaluation requests.
+ */
+ public boolean enableAuditLog = false;
+
+ /**
+ * The fully qualified class name of the {@link AuthenticationHandler} implementation.
+ * This class name will be used to load the implementation from the classpath.
+ * Defaults to null when not specified.
+ */
+ public String handlerClassName = null;
--- End diff --
That sounds good to me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #583: TINKERPOP-1657 Provide abstraction to easily allow dif...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/583
VOTE: +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #583: TINKERPOP-1657 Provide abstraction to easily al...
Posted by krlohnes <gi...@git.apache.org>.
Github user krlohnes commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/583#discussion_r109015220
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/AuthenticationHandler.java ---
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.server.handler;
+
+import io.netty.channel.ChannelFutureListener;
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelInboundHandlerAdapter;
+import io.netty.handler.codec.http.DefaultFullHttpResponse;
+import io.netty.handler.codec.http.FullHttpMessage;
+import io.netty.util.ReferenceCountUtil;
+import org.apache.tinkerpop.gremlin.server.auth.AuthenticationException;
+import org.apache.tinkerpop.gremlin.server.auth.Authenticator;
+
+import java.nio.charset.Charset;
+import java.util.Base64;
+import java.util.HashMap;
+import java.util.Map;
+
+import static io.netty.handler.codec.http.HttpResponseStatus.UNAUTHORIZED;
+import static io.netty.handler.codec.http.HttpVersion.HTTP_1_1;
+import static org.apache.tinkerpop.gremlin.groovy.plugin.dsl.credential.CredentialGraphTokens.PROPERTY_PASSWORD;
+import static org.apache.tinkerpop.gremlin.groovy.plugin.dsl.credential.CredentialGraphTokens.PROPERTY_USERNAME;
+
+/**
+ * Provides an abstraction point to allow for http auth schemes beyond basic auth.
+ */
+import io.netty.channel.ChannelInboundHandlerAdapter;
+
+public abstract class AuthenticationHandler extends ChannelInboundHandlerAdapter {
--- End diff --
Fixed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #583: TINKERPOP-1657 Provide abstraction to easily al...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/583#discussion_r108741558
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Settings.java ---
@@ -387,6 +387,18 @@ public SerializerSettings() {}
public String className = AllowAllAuthenticator.class.getName();
/**
+ * Enable audit logging of authenticated users and gremlin evaluation requests.
+ */
+ public boolean enableAuditLog = false;
+
+ /**
+ * The fully qualified class name of the {@link AuthenticationHandler} implementation.
+ * This class name will be used to load the implementation from the classpath.
+ * Defaults to null when not specified.
+ */
+ public String handlerClassName = null;
--- End diff --
I tried to stay consistent in naming for classes in the config by going with `className` where the key was representative of an object in a `Map` or `List`, but now it kinda bites us as `authentication.className` is used for the `Authenticator` when it now really should probably refer to the handler since it is now configurable.
Here's an idea:
1. Rename `handlerClassName` to `authenticationHandler`
2. Overload `className` with `authenticator` - if `authenticator` is present then ignore any value in `className`. Update all the yaml files to use `authenticator`. [Deprecate](http://tinkerpop.apache.org/docs/current/dev/developer/#_deprecation) `AuthenticationSettings.className`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #583: TINKERPOP-1657 Provide abstraction to easily al...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/583#discussion_r108742664
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/AuthenticationHandler.java ---
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.tinkerpop.gremlin.server.handler;
+
+import io.netty.channel.ChannelFutureListener;
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelInboundHandlerAdapter;
+import io.netty.handler.codec.http.DefaultFullHttpResponse;
+import io.netty.handler.codec.http.FullHttpMessage;
+import io.netty.util.ReferenceCountUtil;
+import org.apache.tinkerpop.gremlin.server.auth.AuthenticationException;
+import org.apache.tinkerpop.gremlin.server.auth.Authenticator;
+
+import java.nio.charset.Charset;
+import java.util.Base64;
+import java.util.HashMap;
+import java.util.Map;
+
+import static io.netty.handler.codec.http.HttpResponseStatus.UNAUTHORIZED;
+import static io.netty.handler.codec.http.HttpVersion.HTTP_1_1;
+import static org.apache.tinkerpop.gremlin.groovy.plugin.dsl.credential.CredentialGraphTokens.PROPERTY_PASSWORD;
+import static org.apache.tinkerpop.gremlin.groovy.plugin.dsl.credential.CredentialGraphTokens.PROPERTY_USERNAME;
+
+/**
+ * Provides an abstraction point to allow for http auth schemes beyond basic auth.
+ */
+import io.netty.channel.ChannelInboundHandlerAdapter;
+
+public abstract class AuthenticationHandler extends ChannelInboundHandlerAdapter {
--- End diff --
We tend to prefix abstract` classes with "Abstract" as a convention. Also, please move the javadoc down below the `import` statement. You might also want to clean up the list of unused imports a bit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #583: TINKERPOP-1657 Provide abstraction to easily allow dif...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/583
All tests pass with `docker/build.sh -t -n -i`
VOTE +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #583: TINKERPOP-1657 Provide abstraction to easily allow dif...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/583
Can you please search for references to `authentication.className` and replace those? I don't see any of the actual yaml files updated in the PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #583: TINKERPOP-1657 Provide abstraction to easily allow dif...
Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/583
One thing I haven't had much time to think about here is how to propagate ssl client authentication up (into an authentication handler). Right now it's at the channel level only. That means it is independent of other authentication mechanisms for better or worse. Permutations to consider:
- needClientAuth REQUIRE, no auth handler: ssl client auth required but no logging of the client
- needClientAuth REQUIRE, +auth handler: both are always required
- needClientAuth OPTIONAL, no auth handler: same as NO SECURITY!
- needClientAuth OPTIONAL, +auth handler: auth handler always required
So one option is to create ssl client auth's own Channel and Auth Handlers thus disallowing mixing. One of the limitations of the current system is what information is available to the Auth handler. So another option is to add whatever information is required to pass along the fact that ssl client auth worked then the auth handler could make its own determination as to if further authentication is required. A more complex option would be to introduce authentication chaining thus allowing multiple mechanisms and some sort of requirement scheme - required, sufficient, etc.
I don't know if this PR does help or can help with any of that. Just some food for thought...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #583: TINKERPOP-1657 Provide abstraction to easily al...
Posted by krlohnes <gi...@git.apache.org>.
Github user krlohnes commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/583#discussion_r109015202
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Settings.java ---
@@ -387,6 +387,18 @@ public SerializerSettings() {}
public String className = AllowAllAuthenticator.class.getName();
/**
+ * Enable audit logging of authenticated users and gremlin evaluation requests.
+ */
+ public boolean enableAuditLog = false;
+
+ /**
+ * The fully qualified class name of the {@link AuthenticationHandler} implementation.
+ * This class name will be used to load the implementation from the classpath.
+ * Defaults to null when not specified.
+ */
+ public String handlerClassName = null;
--- End diff --
Fixed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #583: TINKERPOP-1657 Provide abstraction to easily allow dif...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/583
Hi and thanks for contributing. In general, I like the concept of this pull request, but I have some thoughts to share to get us both on the same page. I'd thought about doing this when we first decided to get authentication in place. I opted not to do it for at least a couple reasons that I can remember:
1. The `Channelizer` abstraction might be enough of an abstraction. It's not complex by any means - the core of `HttpChannelizer` is 20 lines of easily readable code. If someone doesn't want basic auth, then just write a new `Channelizer` and done. Personally, I'm not sure I want to see us expand different kinds of HTTP auth in TinkerPop so `HttpChannelizer` with basic auth seems good enough.
2. The configuration system required further pollution of the root of the configuration file. It bugs me that we don't have `Channelizer` specific configurations - e.g. you had to add `httpAuthHandlerClassName` to `Settings` and that value has nothing to do with anything other than `HttpChannelizer`. If you were configured for web sockets it would just be ignored. Not saying that we don't have that issue at hand now (as I think we do), but I didn't want to continue to proliferate it.
Anyway, I'd had it in my mind that solving 2 would make 1 more palatable as something to do, but 2 hasn't come around yet and unless there is a neat way to do it, it will come in as a breaking change to the Gremlin Server configuration file.
So that's the basic history on this direction for auth configuration - thoughts?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #583: TINKERPOP-1657 Provide abstraction to easily al...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tinkerpop/pull/583
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #583: TINKERPOP-1657 Provide abstraction to easily allow dif...
Posted by pluradj <gi...@git.apache.org>.
Github user pluradj commented on the issue:
https://github.com/apache/tinkerpop/pull/583
Did a successful `docker/build.sh -t -i -n`. Reviewed the code, docs, and test case. Solid PR @krlohnes
VOTE: +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #583: TINKERPOP-1657 Provide abstraction to easily al...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/583#discussion_r109478243
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Settings.java ---
@@ -384,9 +384,25 @@ public SerializerSettings() {}
* used to load the implementation from the classpath. Defaults to {@link AllowAllAuthenticator} when
* not specified.
*/
+ public String authenticator = null;
+
+ /**
+ * The fully qualified class name of the {@link Authenticator} implementation. This class name will be
+ * used to load the implementation from the classpath. Defaults to {@link AllowAllAuthenticator} when
+ * not specified.
+ * Deprecated in favor of {@link authenticator}.
--- End diff --
Note that our deprecation form in javadoc typically looks like this:
```java
@deprecated As of release 3.2.5, replaced by {@link authenticator}.
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #583: TINKERPOP-1657 Provide abstraction to easily allow dif...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/583
@robertdale i'm glad you're thinking at that level wrt to security aspects of Gremlin Server. My understanding of SSL beyond what I've implemented so far is pretty weak, so if you have additional ideas there that would make things better that would be great. I can't say if this PR helps with what you described. My understanding of your comment makes me feel like it doesn't help directly and that the current way to do that would be a fresh `Channelizer` implementation, but I could be missing something.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #583: TINKERPOP-1657 Provide abstraction to easily allow dif...
Posted by krlohnes <gi...@git.apache.org>.
Github user krlohnes commented on the issue:
https://github.com/apache/tinkerpop/pull/583
@spmallette I've updated the docs to include the changes made in this PR. Let me know if I've missed anything / if you'd like me to reword anything.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #583: TINKERPOP-1657 Provide abstraction to easily allow dif...
Posted by krlohnes <gi...@git.apache.org>.
Github user krlohnes commented on the issue:
https://github.com/apache/tinkerpop/pull/583
I got the integration tests passing, I'm hoping to finish up the documentation changes today.
```
[INFO] Reactor Summary:
[INFO]
[INFO] Apache TinkerPop .................................. SUCCESS [59.115s]
[INFO] Apache TinkerPop :: Gremlin Shaded ................ SUCCESS [9.632s]
[INFO] Apache TinkerPop :: Gremlin Core .................. SUCCESS [1:13.945s]
[INFO] Apache TinkerPop :: Gremlin Test .................. SUCCESS [7.673s]
[INFO] Apache TinkerPop :: Gremlin Groovy ................ SUCCESS [2:20.814s]
[INFO] Apache TinkerPop :: Gremlin Groovy Test ........... SUCCESS [6.424s]
[INFO] Apache TinkerPop :: TinkerGraph Gremlin ........... SUCCESS [4:42.573s]
[INFO] Apache TinkerPop :: Gremlin Benchmark ............. SUCCESS [11.786s]
[INFO] Apache TinkerPop :: Gremlin Driver ................ SUCCESS [14.719s]
[INFO] Apache TinkerPop :: Neo4j Gremlin ................. SUCCESS [23:11.498s]
[INFO] Apache TinkerPop :: Gremlin Server ................ SUCCESS [16:58.561s]
[INFO] Apache TinkerPop :: Gremlin Python ................ SUCCESS [15.162s]
[INFO] Apache TinkerPop :: Hadoop Gremlin ................ SUCCESS [9:23.457s]
[INFO] Apache TinkerPop :: Spark Gremlin ................. SUCCESS [20:19.311s]
[INFO] Apache TinkerPop :: Giraph Gremlin ................ SUCCESS [2:20:57.434s]
[INFO] Apache TinkerPop :: Gremlin Console ............... SUCCESS [2:45.687s]
[INFO] Apache TinkerPop :: Gremlin Archetype ............. SUCCESS [0.115s]
[INFO] Apache TinkerPop :: Archetype - TinkerGraph ....... SUCCESS [13.418s]
[INFO] Apache TinkerPop :: Archetype - Server ............ SUCCESS [11.647s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 3:44:24.099s
[INFO] Finished at: Fri Mar 31 22:24:11 UTC 2017
[INFO] Final Memory: 107M/692M
[INFO] ------------------------------------------------------------------------
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #583: TINKERPOP-1657 Provide abstraction to easily allow dif...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/583
@krlohnes I think I'm going to retract what I said about using SASL for this case. SASL mechanisms might not suit every use case in which case it might not be the best way to go about implementing the authentication scheme you set out to have. So, I think you're basically on the right path with this PR. I'll review based on that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #583: TINKERPOP-1657 Provide abstraction to easily al...
Posted by krlohnes <gi...@git.apache.org>.
Github user krlohnes commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/583#discussion_r109015184
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Settings.java ---
@@ -387,6 +387,18 @@ public SerializerSettings() {}
public String className = AllowAllAuthenticator.class.getName();
/**
+ * Enable audit logging of authenticated users and gremlin evaluation requests.
+ */
+ public boolean enableAuditLog = false;
--- End diff --
Fixed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #583: TINKERPOP-1657 Provide abstraction to easily allow dif...
Posted by krlohnes <gi...@git.apache.org>.
Github user krlohnes commented on the issue:
https://github.com/apache/tinkerpop/pull/583
Sorry about that. Fixed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #583: TINKERPOP-1657 Provide abstraction to easily allow dif...
Posted by krlohnes <gi...@git.apache.org>.
Github user krlohnes commented on the issue:
https://github.com/apache/tinkerpop/pull/583
You make some very good points.
> The Channelizer abstraction might be enough of an abstraction.
I'd thought about that a bit, but in general, I'd rather not have to rewrite (or worse, essentially copy and paste) a piece of OS code that works almost exactly like I need it to, but simply doesn't have an extension point where there could easily be one.
>Personally, I'm not sure I want to see us expand different kinds of HTTP auth in TinkerPop
I'm not suggesting adding additional "out of the box" authorization schemes, which is why I didn't commit one. I'm really only suggesting making Tinkerpop more easily extensible so users can code their own that fits with their orgs current auth schemes/security requirements.
> If you were configured for web sockets it would just be ignored
I think this abstraction could be applied fairly easily to the `WebSocketsChannelizer`. There's already the `SaslAuthenticationHandler` there. I think that with a similar change to the ternary operator in the `SaslAuthenicationHandler.init` as in the HttpChannelizer.init, you could change the abstract `HttpAuthenticationHandler` class to an `AuthenticationHandler` and have the `SaslAuthenticationHandler` extend that. I could then change the config name from `httpAuthHandlerClassName` to `authHandlerClassName`.
> The configuration system required further pollution of the root of the configuration file
Well, it's not the _root_ of the configuration file. It's under `authentication : { ... }`.
I do agree that Channelizer config would be nice to have and may follow naturally from the idea of this PR, but I think this PR is a small, iterative change that _doesn't_ break anything.
I think with the change around the `WebSocketsChannelizer` (included in a new commit), the story around this becomes better. Let me know what you think.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #583: TINKERPOP-1657 Provide abstraction to easily al...
Posted by krlohnes <gi...@git.apache.org>.
Github user krlohnes commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/583#discussion_r108750598
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Settings.java ---
@@ -387,6 +387,18 @@ public SerializerSettings() {}
public String className = AllowAllAuthenticator.class.getName();
/**
+ * Enable audit logging of authenticated users and gremlin evaluation requests.
+ */
+ public boolean enableAuditLog = false;
--- End diff --
Ah, I'll delete that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #583: TINKERPOP-1657 Provide abstraction to easily allow dif...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/583
> I think this abstraction could be applied fairly easily to the `WebSocketsChannelizer`
You're right.
> Well, it's not the root of the configuration file. It's under `authentication : { ... }`.
sorry - i missed that and all the better that it can be generally applied to the `WebSocketChannelizer` as i agreed above. of course, given both of the above points, something doesn't feel right. Let me try to explain. It seems like we have a pluggable authentication schema already in SASL and now this pull request suggest that we make SASL pluggable. I think we just need to make the `HttpChannelizer` use SASL. Then if someone wants to do HMAC (or whatever custom security theme) they implement through SASL. Does that make sense?
> I think this PR is a small, iterative change that doesn't break anything.
agreed - this PR is well scoped as it is. i didn't mean to suggest expanding it, especially now that i understand that the change isn't at the root of the yaml.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop issue #583: TINKERPOP-1657 Provide abstraction to easily allow dif...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/583
In addition to the other individual comments I had on the code itself, please also amend this PR to handle documentation requirements:
1. Reference documentation should reflect these new configuration options and extension points.
2. Upgrade docs could use an entry in the "users" section.
3. Update CHANGELOG as needed for 3.2.x
Finally, did all tests pass with integration tests? Note that using docker can make this process easier if you don't have your environment setup to do the full build - `docker/build.sh -t -i -n`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #583: TINKERPOP-1657 Provide abstraction to easily al...
Posted by krlohnes <gi...@git.apache.org>.
Github user krlohnes commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/583#discussion_r109659946
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Settings.java ---
@@ -384,9 +384,25 @@ public SerializerSettings() {}
* used to load the implementation from the classpath. Defaults to {@link AllowAllAuthenticator} when
* not specified.
*/
+ public String authenticator = null;
+
+ /**
+ * The fully qualified class name of the {@link Authenticator} implementation. This class name will be
+ * used to load the implementation from the classpath. Defaults to {@link AllowAllAuthenticator} when
+ * not specified.
+ * Deprecated in favor of {@link authenticator}.
--- End diff --
Fixed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tinkerpop pull request #583: TINKERPOP-1657 Provide abstraction to easily al...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/583#discussion_r108735329
--- Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Settings.java ---
@@ -387,6 +387,18 @@ public SerializerSettings() {}
public String className = AllowAllAuthenticator.class.getName();
/**
+ * Enable audit logging of authenticated users and gremlin evaluation requests.
+ */
+ public boolean enableAuditLog = false;
--- End diff --
What is the purpose of this configuration option? It doesn't appear to be used in any way.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---