Details

    Description

      Errors with HTTP REST basic auth. I was able to reproduce this on tp30 and master. I have a pull request coming.

      1. Built the latest tp30 branch. Copied gremlin-server-secure.yaml to gremlin-server-secure-rest.yaml and updated the channelizer to org.apache.tinkerpop.gremlin.server.channel.HttpChannelizer.

      Ran this command.

      $ curl -k -X POST -v -H 'Content-Type: application/json' -u stephen:password https://127.0.0.1:8182 -d '{"gremlin":"100-3"}'
      * Rebuilt URL to: https://127.0.0.1:8182/
      *   Trying 127.0.0.1...
      * Connected to 127.0.0.1 (127.0.0.1) port 8182 (#0)
      * TLS 1.2 connection using TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA
      * Server certificate: example.com
      * Server auth using Basic with user 'stephen'
      > POST / HTTP/1.1
      > Host: 127.0.0.1:8182
      > Authorization: Basic c3RlcGhlbjpwYXNzd29yZA==
      > User-Agent: curl/7.43.0
      > Accept: */*
      > Content-Type: application/json
      > Content-Length: 19
      >
      * upload completely sent off: 19 out of 19 bytes
      

      Got this output on the server.

      [ERROR] HttpGremlinEndpointHandler - Error processing HTTP Request
      java.lang.IllegalArgumentException: Illegal base64 character 20
          at java.util.Base64$Decoder.decode0(Base64.java:714)
          at java.util.Base64$Decoder.decode(Base64.java:526)
          at java.util.Base64$Decoder.decode(Base64.java:549)
          at org.apache.tinkerpop.gremlin.server.handler.HttpBasicAuthenticationHandler.channelRead(HttpBasicAuthenticationHandler.java:64)
          at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:308)
          at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:294)
          at io.netty.handler.codec.MessageToMessageDecoder.channelRead(MessageToMessageDecoder.java:103)
          at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:308)
          at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:294)
          at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:244)
          at io.netty.channel.CombinedChannelDuplexHandler.channelRead(CombinedChannelDuplexHandler.java:147)
          at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:308)
          at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:294)
          at io.netty.handler.ssl.SslHandler.unwrap(SslHandler.java:1069)
          at io.netty.handler.ssl.SslHandler.decode(SslHandler.java:944)
          at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:327)
          at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:230)
          at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:308)
          at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:294)
          at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:846)
          at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:131)
          at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:511)
          at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:468)
          at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:382)
          at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:354)
          at io.netty.util.concurrent.SingleThreadEventExecutor$2.run(SingleThreadEventExecutor.java:111)
          at java.lang.Thread.run(Thread.java:745)
      

      The fix is to chop off "Basic " from the Authorization header.

      2. After that worked, I ran into this error when trying to run consecutive curls.

      $ curl -k -X POST -v -H 'Content-Type: application/json' -u stephen:password https://127.0.0.1:8182 -d '{"gremlin":"100-3"}'
      * Rebuilt URL to: https://127.0.0.1:8182/
      *   Trying 127.0.0.1...
      * Connected to 127.0.0.1 (127.0.0.1) port 8182 (#0)
      * Server aborted the SSL handshake
      * Closing connection 0
      curl: (35) Server aborted the SSL handshake
      

      Got this output on the server.

      io.netty.channel.ChannelPipelineException: org.apache.tinkerpop.gremlin.server.handler.HttpBasicAuthenticationHandler is not a @Sharable handler, so can't be added or removed multiple times.
          at io.netty.channel.DefaultChannelPipeline.checkMultiplicity(DefaultChannelPipeline.java:464)
          at io.netty.channel.DefaultChannelPipeline.addLast0(DefaultChannelPipeline.java:136)
          at io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:129)
          at io.netty.channel.DefaultChannelPipeline.addLast(DefaultChannelPipeline.java:120)
          at org.apache.tinkerpop.gremlin.server.channel.HttpChannelizer.configure(HttpChannelizer.java:71)
          at org.apache.tinkerpop.gremlin.server.AbstractChannelizer.initChannel(AbstractChannelizer.java:134)
          at org.apache.tinkerpop.gremlin.server.AbstractChannelizer.initChannel(AbstractChannelizer.java:63)
          at io.netty.channel.ChannelInitializer.channelRegistered(ChannelInitializer.java:69)
          at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRegistered(AbstractChannelHandlerContext.java:133)
          at io.netty.channel.AbstractChannelHandlerContext.fireChannelRegistered(AbstractChannelHandlerContext.java:119)
          at io.netty.channel.DefaultChannelPipeline.fireChannelRegistered(DefaultChannelPipeline.java:733)
          at io.netty.channel.AbstractChannel$AbstractUnsafe.register0(AbstractChannel.java:450)
          at io.netty.channel.AbstractChannel$AbstractUnsafe.access$100(AbstractChannel.java:378)
          at io.netty.channel.AbstractChannel$AbstractUnsafe$1.run(AbstractChannel.java:424)
          at io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:357)
          at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:357)
          at io.netty.util.concurrent.SingleThreadEventExecutor$2.run(SingleThreadEventExecutor.java:111)
          at java.lang.Thread.run(Thread.java:745)
      

      I fixed this by moving the initialization of the authentication handler in HttpChannelizer from init() to configure(). It doesn't seem like a safe to assumption that the Authenticator interface implementation can be shared.

      3. After that worked, the other thing I noticed is that if authorization fails, the curl doesn't close.

      $ curl -k -X POST -v -H 'Content-Type: application/json' -u stephen:bogus https://127.0.0.1:8182 -d '{"gremlin":"100-3"}'
      * Rebuilt URL to: https://127.0.0.1:8182/
      *   Trying 127.0.0.1...
      * Connected to 127.0.0.1 (127.0.0.1) port 8182 (#0)
      * TLS 1.2 connection using TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA
      * Server certificate: example.com
      * Server auth using Basic with user 'stephen'
      > POST / HTTP/1.1
      > Host: 127.0.0.1:8182
      > Authorization: Basic c3RlcGhlbjpwYXNz
      > User-Agent: curl/7.43.0
      > Accept: */*
      > Content-Type: application/json
      > Content-Length: 19
      >
      * upload completely sent off: 19 out of 19 bytes
      < HTTP/1.1 401 Unauthorized
      * no chunk, no close, no size. Assume close to signal end
      <
      

      I fixed this by adding a listener ChannelFutureListener.CLOSE during the write unautorized response.

      Attachments

        Activity

          The REST interface probably needs some work, so I'm glad you dug into it. It was added as a bit of an afterthought to websockets so if you have other ways of making it better or more efficient, the let's discuss.

          Glad you did this work against the tp30 branch as it would be nice to see this fix for 3.0.2.

          spmallette Stephen Mallette added a comment - The REST interface probably needs some work, so I'm glad you dug into it. It was added as a bit of an afterthought to websockets so if you have other ways of making it better or more efficient, the let's discuss. Glad you did this work against the tp30 branch as it would be nice to see this fix for 3.0.2.
          dmill Dylan Millikin added a comment -

          To add to this I've been meaning to patch the Authentication handler to properly catch exceptions generated by incorrectly formatted base64 strings, as it doesn't currently terminate cleanly and just hangs, maybe it would be nice to have that here as well.. (we could make a single PR for all this).

          dmill Dylan Millikin added a comment - To add to this I've been meaning to patch the Authentication handler to properly catch exceptions generated by incorrectly formatted base64 strings, as it doesn't currently terminate cleanly and just hangs, maybe it would be nice to have that here as well.. (we could make a single PR for all this).
          pluradj Jason Plurad added a comment -

          Sounds fine to me, Dylan. You can have a look at what I have so far here https://github.com/pluradj/incubator-tinkerpop/tree/tp30-http-basic-auth and let me know what else to add.

          pluradj Jason Plurad added a comment - Sounds fine to me, Dylan. You can have a look at what I have so far here https://github.com/pluradj/incubator-tinkerpop/tree/tp30-http-basic-auth and let me know what else to add.
          dmill Dylan Millikin added a comment -

          Essentially it would be about catching IllegalArgumentException on Base64.Decoder.decode() and closing connections in the following two spots :

          https://github.com/pluradj/incubator-tinkerpop/blob/tp30-http-basic-auth/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpBasicAuthenticationHandler.java#L65
          and
          https://github.com/apache/incubator-tinkerpop/blob/tp30/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/SaslAuthenticationHandler.java#L82 (though this code seems to be more recent than your branch)

          For the SaslAuthenticationHandler you would probably want to throw a REQUEST_ERROR_MALFORMED_REQUEST error. I'm not so sure about the HttpBasicAuthenticationHandler though. Maybe Stephen can clear that up.

          dmill Dylan Millikin added a comment - Essentially it would be about catching IllegalArgumentException on Base64.Decoder.decode() and closing connections in the following two spots : https://github.com/pluradj/incubator-tinkerpop/blob/tp30-http-basic-auth/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/HttpBasicAuthenticationHandler.java#L65 and https://github.com/apache/incubator-tinkerpop/blob/tp30/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/SaslAuthenticationHandler.java#L82 (though this code seems to be more recent than your branch) For the SaslAuthenticationHandler you would probably want to throw a REQUEST_ERROR_MALFORMED_REQUEST error. I'm not so sure about the HttpBasicAuthenticationHandler though. Maybe Stephen can clear that up.

          Maybe these should be two separate bodies of work/PRs? thoughts?

          > For the SaslAuthenticationHandler you would probably want to throw a REQUEST_ERROR_MALFORMED_REQUEST error. I'm not so sure about the HttpBasicAuthenticationHandler though.

          I guess folks would argue about what to do here in any respect given that this is a security function. I'm fine with "malformed request" for both though one could also argue for a simple "unauthorized". I don't feel strongly about either direction.

          spmallette Stephen Mallette added a comment - Maybe these should be two separate bodies of work/PRs? thoughts? > For the SaslAuthenticationHandler you would probably want to throw a REQUEST_ERROR_MALFORMED_REQUEST error. I'm not so sure about the HttpBasicAuthenticationHandler though. I guess folks would argue about what to do here in any respect given that this is a security function. I'm fine with "malformed request" for both though one could also argue for a simple "unauthorized". I don't feel strongly about either direction.
          dmill Dylan Millikin added a comment -

          Maybe these should be two separate bodies of work/PRs? thoughts?

          I'm fine either way. There were a couple of other typos and such that I also wanted to address so I'm fine with doing all that in another "cleanup" PR.

          dmill Dylan Millikin added a comment - Maybe these should be two separate bodies of work/PRs? thoughts? I'm fine either way. There were a couple of other typos and such that I also wanted to address so I'm fine with doing all that in another "cleanup" PR.
          pluradj Jason Plurad added a comment -

          Separate is fine with me as long as the response is consistent from REST and WebSockets.

          spmallette do you have any suggestions on how I can start incorporating some unit tests for this PR? I think the code I have is fine, but just trying to be better about getting unit tests in place.

          pluradj Jason Plurad added a comment - Separate is fine with me as long as the response is consistent from REST and WebSockets. spmallette do you have any suggestions on how I can start incorporating some unit tests for this PR? I think the code I have is fine, but just trying to be better about getting unit tests in place.

          If you guys have a better feel for how this is coming together and there's lots of overlap, then by all means, put it together. If you folks were on a path that made sense, then don't let me disrupt it.

          pluradj do you mean actual "unit" tests or "integration" tests? and are you aware of the difference in the TinkerPop context?

          spmallette Stephen Mallette added a comment - If you guys have a better feel for how this is coming together and there's lots of overlap, then by all means, put it together. If you folks were on a path that made sense, then don't let me disrupt it. pluradj do you mean actual "unit" tests or "integration" tests? and are you aware of the difference in the TinkerPop context?
          pluradj Jason Plurad added a comment -

          No, not aware of the difference in TinkerPop context. I'm willing to add whatever tests are needed.

          pluradj Jason Plurad added a comment - No, not aware of the difference in TinkerPop context. I'm willing to add whatever tests are needed.

          A "unit test" refers to something that typically runs quickly and most often tests just a single class/method. We expand that to include the "process" suite of tests which technically test GraphTraversal, but that's an exception rather than a rule. "Unit tests" will run when you mvn clean install by default.

          An "Integration test" is typically something that takes extended time to run and most often tests functionality in a much wider sense. Most of the Gremlin Server tests that validate operations are of this type. Why? because we want to test Gremlin Server in an end-to-end fashion so these tests will start/stop Gremlin Server in between test executions. You can alter the settings on the fly and really make sure that stuff works top-down. For example for REST, we have this:

          https://github.com/apache/incubator-tinkerpop/blob/master/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerHttpIntegrateTest.java

          You would likely add your tests there. You could also break it up if you like - I recently did that for websockets and made auth tests something separate. Note that these tests do not run by default. You have to explicitly turn them:

          mvn verify -DskipIntegrationTests=false
          

          Hope that makes sense.

          spmallette Stephen Mallette added a comment - A "unit test" refers to something that typically runs quickly and most often tests just a single class/method. We expand that to include the "process" suite of tests which technically test GraphTraversal , but that's an exception rather than a rule. "Unit tests" will run when you mvn clean install by default. An "Integration test" is typically something that takes extended time to run and most often tests functionality in a much wider sense. Most of the Gremlin Server tests that validate operations are of this type. Why? because we want to test Gremlin Server in an end-to-end fashion so these tests will start/stop Gremlin Server in between test executions. You can alter the settings on the fly and really make sure that stuff works top-down. For example for REST, we have this: https://github.com/apache/incubator-tinkerpop/blob/master/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerHttpIntegrateTest.java You would likely add your tests there. You could also break it up if you like - I recently did that for websockets and made auth tests something separate. Note that these tests do not run by default. You have to explicitly turn them: mvn verify -DskipIntegrationTests= false Hope that makes sense.

          A "unit test" refers to something that typically runs quickly and most often tests just a single class/method. We expand that to include the "process" suite of tests which technically test GraphTraversal, but that's an exception rather than a rule. "Unit tests" will run when you mvn clean install by default.

          An "Integration test" is typically something that takes extended time to run and most often tests functionality in a much wider sense. Most of the Gremlin Server tests that validate operations are of this type. Why? because we want to test Gremlin Server in an end-to-end fashion so these tests will start/stop Gremlin Server in between test executions. You can alter the settings on the fly and really make sure that stuff works top-down. For example for REST, we have this:

          https://github.com/apache/incubator-tinkerpop/blob/master/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerHttpIntegrateTest.java

          You would likely add your tests there. You could also break it up if you like - I recently did that for websockets and made auth tests something separate. Note that these tests do not run by default. You have to explicitly turn them:

          mvn verify -DskipIntegrationTests=false
          

          Hope that makes sense.

          spmallette Stephen Mallette added a comment - A "unit test" refers to something that typically runs quickly and most often tests just a single class/method. We expand that to include the "process" suite of tests which technically test GraphTraversal , but that's an exception rather than a rule. "Unit tests" will run when you mvn clean install by default. An "Integration test" is typically something that takes extended time to run and most often tests functionality in a much wider sense. Most of the Gremlin Server tests that validate operations are of this type. Why? because we want to test Gremlin Server in an end-to-end fashion so these tests will start/stop Gremlin Server in between test executions. You can alter the settings on the fly and really make sure that stuff works top-down. For example for REST, we have this: https://github.com/apache/incubator-tinkerpop/blob/master/gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerHttpIntegrateTest.java You would likely add your tests there. You could also break it up if you like - I recently did that for websockets and made auth tests something separate. Note that these tests do not run by default. You have to explicitly turn them: mvn verify -DskipIntegrationTests= false Hope that makes sense.
          pluradj Jason Plurad added a comment - - edited

          dmill I handled the IllegalArgumentException in HttpBasicAuthenticationHandler. I went with UNAUTHORIZED because it gives more attention to the HTTP Authorization header being the cause of the error. REQUEST_ERROR_MALFORMED_REQUEST also seemed more specific to the OP handling.

          pluradj Jason Plurad added a comment - - edited dmill I handled the IllegalArgumentException in HttpBasicAuthenticationHandler. I went with UNAUTHORIZED because it gives more attention to the HTTP Authorization header being the cause of the error. REQUEST_ERROR_MALFORMED_REQUEST also seemed more specific to the OP handling.

          People

            pluradj Jason Plurad
            pluradj Jason Plurad
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: