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

Consistent use of base 64 encoded bytes for SASL negotiation

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Done
    • Affects Version/s: 3.2.3
    • Fix Version/s: 3.2.4
    • Component/s: driver, server
    • Labels:
      None

      Description

      Gremlin Server currently uses a mix of base 64 encoded bytes and byte arrays for SASL negotiation. This can cause problems for certain serializers (like toString serialization with gryo) as the byte array won't be respected. In an effort to easily support virtually any serializer a switch to using just base 64 string is probably best.

      This can be done in such a way as to be backward compatible. The base64 SASL value will be returned in the response message status attributes map in a key called "sasl". The original byte array will continue to be returned in the response message result. Eventually, we could phase out the byte array in the result - perhaps with 3.3.0.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

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

          Github user spmallette commented on the issue:

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

          @vtslab i just pushed a fixup commit - i had a sorta dumb mistake on this pr. i knew that having your work would uncover this - it's good that you had #534 ready. I merged your changes locally onto this branch and the tests now pass without any additional changes on your end.

          I'm going to get this merged to tp32/master now. At that point you can rebase (hopefully the last time) and I can put some focus on full review of your PR.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/533 @vtslab i just pushed a fixup commit - i had a sorta dumb mistake on this pr. i knew that having your work would uncover this - it's good that you had #534 ready. I merged your changes locally onto this branch and the tests now pass without any additional changes on your end. I'm going to get this merged to tp32/master now. At that point you can rebase (hopefully the last time) and I can put some focus on full review of your PR.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user vtslab commented on the issue:

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

          I agree with your explanation that a byte array returned from gremlin server as the result of a query does not crash gremlin driver (and I also checked it manually). Sorry for the confusion introduced.

          Show
          githubbot ASF GitHub Bot added a comment - Github user vtslab commented on the issue: https://github.com/apache/tinkerpop/pull/533 I agree with your explanation that a byte array returned from gremlin server as the result of a query does not crash gremlin driver (and I also checked it manually). Sorry for the confusion introduced.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user vtslab commented on the issue:

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

          Just undoing [this commit](https://github.com/apache/tinkerpop/pull/534/commits/62648242c6576b020d2dd2933b89b9d69e87fed0) in TINKERPOP-1566 and merging in TINKERPOP-1600 does not work for me, see below. I did not dig in yet, maybe you recognize what is happening. Other tests without serializeResultToString configured fail the same way. Btw, in my testing branch I renamed GremlinServerAuthKrb5IntegrateTest to GremlinServerAuthKrb5Test for faster testing.

          Tests run: 8, Failures: 0, Errors: 3, Skipped: 0, Time elapsed: 34.711 sec <<< FAILURE! - in org.apache.tinkerpop.gremlin.server.GremlinServerAuthKrb5Test
          shouldAuthenticateWithSerializeResultToString(org.apache.tinkerpop.gremlin.server.GremlinServerAuthKrb5Test) Time elapsed: 5.611 sec <<< ERROR!
          java.util.concurrent.ExecutionException: java.lang.IllegalArgumentException: Illegal base64 character 5b
          at java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:357)
          at java.util.concurrent.CompletableFuture.get(CompletableFuture.java:1895)
          at org.apache.tinkerpop.gremlin.server.GremlinServerAuthKrb5Test.shouldAuthenticateWithSerializeResultToString(GremlinServerAuthKrb5Test.java:220)
          Caused by: java.lang.IllegalArgumentException: Illegal base64 character 5b
          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.driver.Handler$GremlinSaslAuthenticationHandler.channelRead0(Handler.java:119)
          at org.apache.tinkerpop.gremlin.driver.Handler$GremlinSaslAuthenticationHandler.channelRead0(Handler.java:67)
          at io.netty.channel.SimpleChannelInboundHandler.channelRead(SimpleChannelInboundHandler.java:105)

          Show
          githubbot ASF GitHub Bot added a comment - Github user vtslab commented on the issue: https://github.com/apache/tinkerpop/pull/533 Just undoing [this commit] ( https://github.com/apache/tinkerpop/pull/534/commits/62648242c6576b020d2dd2933b89b9d69e87fed0 ) in TINKERPOP-1566 and merging in TINKERPOP-1600 does not work for me, see below. I did not dig in yet, maybe you recognize what is happening. Other tests without serializeResultToString configured fail the same way. Btw, in my testing branch I renamed GremlinServerAuthKrb5IntegrateTest to GremlinServerAuthKrb5Test for faster testing. Tests run: 8, Failures: 0, Errors: 3, Skipped: 0, Time elapsed: 34.711 sec <<< FAILURE! - in org.apache.tinkerpop.gremlin.server.GremlinServerAuthKrb5Test shouldAuthenticateWithSerializeResultToString(org.apache.tinkerpop.gremlin.server.GremlinServerAuthKrb5Test) Time elapsed: 5.611 sec <<< ERROR! java.util.concurrent.ExecutionException: java.lang.IllegalArgumentException: Illegal base64 character 5b at java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:357) at java.util.concurrent.CompletableFuture.get(CompletableFuture.java:1895) at org.apache.tinkerpop.gremlin.server.GremlinServerAuthKrb5Test.shouldAuthenticateWithSerializeResultToString(GremlinServerAuthKrb5Test.java:220) Caused by: java.lang.IllegalArgumentException: Illegal base64 character 5b 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.driver.Handler$GremlinSaslAuthenticationHandler.channelRead0(Handler.java:119) at org.apache.tinkerpop.gremlin.driver.Handler$GremlinSaslAuthenticationHandler.channelRead0(Handler.java:67) at io.netty.channel.SimpleChannelInboundHandler.channelRead(SimpleChannelInboundHandler.java:105)
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

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

          VOTE +1

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

          Github user davebshow commented on the issue:

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

          All of my driver tests succeed against this branch. They use `'saslMechanism': 'PLAIN'`, so they don't expect any `sasl` value to be returned.

          VOTE +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user davebshow commented on the issue: https://github.com/apache/tinkerpop/pull/533 All of my driver tests succeed against this branch. They use `'saslMechanism': 'PLAIN'`, so they don't expect any `sasl` value to be returned. VOTE +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

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

          The driver shouldn't crash by sending a byte array in the results, because it is only the SASL authentication process that was expecting `byte[]`. When it would find a `String` that's when we'd get the failure as it tried to cast a `byte[]` from that. A `byte[]` as a normal result from a script evaluation should not have any problems. Please let me know if I'm missing something.

          `serializeResultToString` is used by the Gremlin Console. It presents a cheap way for the console to interact with the server by making the console more of a dummy terminal that just prints the results from the server without having to get a giant body of fully serialized results back. It also allows for you to work with classes that aren't necessarily serializable as they will simply be toString'd on the way back to the console.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/533 The driver shouldn't crash by sending a byte array in the results, because it is only the SASL authentication process that was expecting `byte[]`. When it would find a `String` that's when we'd get the failure as it tried to cast a `byte[]` from that. A `byte[]` as a normal result from a script evaluation should not have any problems. Please let me know if I'm missing something. `serializeResultToString` is used by the Gremlin Console. It presents a cheap way for the console to interact with the server by making the console more of a dummy terminal that just prints the results from the server without having to get a giant body of fully serialized results back. It also allows for you to work with classes that aren't necessarily serializable as they will simply be toString'd on the way back to the console.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user vtslab commented on the issue:

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

          While this solves the issue for byte[] returned from Sasl, I can still crash the driver by adding a byte[] as a vertex property and ask for the result:
          gremlin> g.V(1).property('test1', 'test1' as byte[])
          ==>v[1]
          gremlin> g.V(1).values('test1')
          ==>[116, 101, 115, 116, 49]
          gremlin> g.V(1).values('test1').next().getClass()
          ==>class [B
          OK, this is a pathological example, why configure serializeResultToString if you want a byte[] returned... BTW, what is this serializeResultToString meant for anyway?

          Show
          githubbot ASF GitHub Bot added a comment - Github user vtslab commented on the issue: https://github.com/apache/tinkerpop/pull/533 While this solves the issue for byte[] returned from Sasl, I can still crash the driver by adding a byte[] as a vertex property and ask for the result: gremlin> g.V(1).property('test1', 'test1' as byte[]) ==>v [1] gremlin> g.V(1).values('test1') ==> [116, 101, 115, 116, 49] gremlin> g.V(1).values('test1').next().getClass() ==>class [B OK, this is a pathological example, why configure serializeResultToString if you want a byte[] returned... BTW, what is this serializeResultToString meant for anyway?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

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

          ah - cool. i think that i kinda needed your changes so that i could better test this PR. perhaps I can get mine merged in first, then you can rebase yours with these changes.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/533 ah - cool. i think that i kinda needed your changes so that i could better test this PR. perhaps I can get mine merged in first, then you can rebase yours with these changes.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user vtslab commented on the issue:

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

          This attacks the same issue as I had in PR TINKERPOP-1566 Kerberos:
          https://github.com/apache/tinkerpop/pull/534/commits/62648242c6576b020d2dd2933b89b9d69e87fed0
          I am fine with your solution in TINKERPOP-1600.

          Show
          githubbot ASF GitHub Bot added a comment - Github user vtslab commented on the issue: https://github.com/apache/tinkerpop/pull/533 This attacks the same issue as I had in PR TINKERPOP-1566 Kerberos: https://github.com/apache/tinkerpop/pull/534/commits/62648242c6576b020d2dd2933b89b9d69e87fed0 I am fine with your solution in TINKERPOP-1600 .
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user spmallette opened a pull request:

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

          TINKERPOP-1600 Added base64 encoded string to sasl challenge

          This is a small change but I was hoping for some tests by driver providers before merging as this change messes with the authentication scheme a bit. It should be a backward compatible change, but I just wanted to be sure I didn't break anything before merging this.

          VOTE +1

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

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

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

          https://github.com/apache/tinkerpop/pull/533.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 #533


          commit 0fcb306efa05c21b9c11f25f5dddb658159a4540
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2017-01-16T20:22:07Z

          TINKERPOP-1600 Added base64 encoded string to sasl challenge


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user spmallette opened a pull request: https://github.com/apache/tinkerpop/pull/533 TINKERPOP-1600 Added base64 encoded string to sasl challenge This is a small change but I was hoping for some tests by driver providers before merging as this change messes with the authentication scheme a bit. It should be a backward compatible change, but I just wanted to be sure I didn't break anything before merging this. VOTE +1 You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/tinkerpop TINKERPOP-1600 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/533.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 #533 commit 0fcb306efa05c21b9c11f25f5dddb658159a4540 Author: Stephen Mallette <spmva@genoprime.com> Date: 2017-01-16T20:22:07Z TINKERPOP-1600 Added base64 encoded string to sasl challenge

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development