Uploaded image for project: 'TinkerPop'
  1. TinkerPop
  2. TINKERPOP-1657

Provide abstraction to easily allow different HttpAuth schemes

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Implemented
    • Affects Version/s: None
    • Fix Version/s: 3.2.5
    • Component/s: server
    • Labels:
      None

      Description

      The current HttpChannelizer allows for extension through an Authenticator class supplied through the authorization settings. There isn't, however, an extension point for an authentication handler. Currently the choice is between the `AllowAllAuthenticator` or the `HttpBasicAuthenticationHandler`. One would need to create a new channelizer where the HttpChannelizer would suffice. Creating an abstract class that can be extended would make it easier to extend Authentication for things like token authentication schemes.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          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 <krlohnes@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.


          Show
          githubbot ASF GitHub Bot added a comment - 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 <krlohnes@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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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?

          Show
          githubbot ASF GitHub Bot added a comment - 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?
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          Show
          githubbot ASF GitHub Bot added a comment - 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`.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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`

          Show
          githubbot ASF GitHub Bot added a comment - 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`
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          Show
          githubbot ASF GitHub Bot added a comment - 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...
          Hide
          githubbot ASF GitHub Bot added a comment -

          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

          Show
          githubbot ASF GitHub Bot added a comment - 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          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

          Show
          githubbot ASF GitHub Bot added a comment - 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          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

          Show
          githubbot ASF GitHub Bot added a comment - 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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}

            .
            ```

          Show
          githubbot ASF GitHub Bot added a comment - 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} . ```
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          Show
          githubbot ASF GitHub Bot added a comment - 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] ------------------------------------------------------------------------ ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          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

          Show
          githubbot ASF GitHub Bot added a comment - 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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.

          Show
          githubbot ASF GitHub Bot added a comment - 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user krlohnes commented on the issue:

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

          Sorry about that. Fixed.

          Show
          githubbot ASF GitHub Bot added a comment - Github user krlohnes commented on the issue: https://github.com/apache/tinkerpop/pull/583 Sorry about that. Fixed.
          Hide
          githubbot ASF GitHub Bot added a comment -

          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

          Show
          githubbot ASF GitHub Bot added a comment - 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          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

          Show
          githubbot ASF GitHub Bot added a comment - 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the issue:

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

          VOTE: +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/583 VOTE: +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

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

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development