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

sasl authentication type error due to Json format

Details

    Description

      The documentation states :

      The password should be an encoded sequence of UTF-8 bytes

      Thus the SaslAuthenticationHandler expects to receive a byte[] type var.

      However, using gremlin-server with GraphSonMessageSerializer, if I send the payload with the sasl argument (say \x00stephen\x00password) in response to a gremlin-server 407 authentication challenge, I will get the following error:

      java.lang.ClassCastException: java.lang.String cannot be cast to [B
      	at org.apache.tinkerpop.gremlin.server.handler.SaslAuthenticationHandler.channelRead(SaslAuthenticationHandler.java:74)
      

      This seems "normal" in that Json does not support any binary dataType and the sasl argument will automatically be converted to String.

      I quickly tested a correction locally by changing this line to :

      final String saslString = (String) requestMessage.getArgs().get(Tokens.ARGS_SASL);
      final byte[] saslResponse = saslString.getBytes(Charset.forName("UTF-8"));

      This is clearly a breaking change, but it solved the Json issue.

      If you have any ideas on the way you want to go with this (or If I'm totally doing something wrong) let me know. I could probably make a PR for this.

      Attachments

        Activity

          This was a nice pull request. I added a bonus test for type embedded graphson and that worked too. Thanks for working it through.

          spmallette Stephen Mallette added a comment - This was a nice pull request. I added a bonus test for type embedded graphson and that worked too. Thanks for working it through.
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

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

          Github user PommeVerte commented on the pull request:

          https://github.com/apache/incubator-tinkerpop/pull/98#issuecomment-144334190

          It's worth going through the issue as I've made an arbitrary decision on the JSON input type and this needs to be reviewed.

          githubbot ASF GitHub Bot added a comment - Github user PommeVerte commented on the pull request: https://github.com/apache/incubator-tinkerpop/pull/98#issuecomment-144334190 It's worth going through the issue as I've made an arbitrary decision on the JSON input type and this needs to be reviewed.
          githubbot ASF GitHub Bot added a comment -

          GitHub user PommeVerte opened a pull request:

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

          Made correction to fix TINKERPOP3-855.

          This fixes the issue with Json submitted sasl arguments not being in `byte[]` format.
          It is not BC breaking. Tests seem to pass though I wouldn't mind confirmation here.

          I also added changes to the documentation to reflect the new functionality.

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

          $ git pull https://github.com/PommeVerte/incubator-tinkerpop TINKERPOP3-855-json-auth

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

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


          commit 39cb42ddde538a33bd7b3b3a0b27428aae2a2276
          Author: Dylan Millikin <dylan.millikin@brightzone.fr>
          Date: 2015-09-30T08:52:56Z

          Made correction to fix TINKERPOP3-855. Added Test and changed documentation.


          githubbot ASF GitHub Bot added a comment - GitHub user PommeVerte opened a pull request: https://github.com/apache/incubator-tinkerpop/pull/98 Made correction to fix TINKERPOP3-855 . This fixes the issue with Json submitted sasl arguments not being in `byte[]` format. It is not BC breaking. Tests seem to pass though I wouldn't mind confirmation here. I also added changes to the documentation to reflect the new functionality. You can merge this pull request into a Git repository by running: $ git pull https://github.com/PommeVerte/incubator-tinkerpop TINKERPOP3-855 -json-auth Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-tinkerpop/pull/98.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 #98 commit 39cb42ddde538a33bd7b3b3a0b27428aae2a2276 Author: Dylan Millikin <dylan.millikin@brightzone.fr> Date: 2015-09-30T08:52:56Z Made correction to fix TINKERPOP3-855 . Added Test and changed documentation.
          dmill Dylan Millikin added a comment - - edited

          Ok I'm done with this and I've got a PR ready. It will however need tweaks because :

          Your test does indeed fail (with or without my changes). The gist of the issue here with your test is that jackson will convert the byte[] to Base64 and it fails the test in two ways:

          • Without the fix for this issue it will throw the same error (can't cast String to [B)
          • With the fix another error occurs because the Base64 string doesn't follow the NUL separated specs. (thus failing to populate user and password)

          Seeing as this feature doesn't currently work anyways, I think we have full reign on how to implement it. Converting byte[] to a Base64 string isn't unpleasant in itself and drivers should be able to implement this.

          My suggestion would be to change the documentation and the feature to work in these two scenarios:

          • ARGS_SASL is of type byte[] and we treat it normally (ensures the feature is BC)
          • ARGS_SASL is of type String and it must be a Base64 encoded string of a byte[]

          This however implies that the data be passed differently depending on the serializer being used... Which in itself isn't ideal.
          The only other option I can think of to homogenize the feature would be to remove the expectancy on a byte[] typed variable, but that would be a BC breaking change so alas no.

          This is an easy implementation so I'm going ahead with this change, if you have any counter indications or we decide not to go with it I can easily revert back.

          dmill Dylan Millikin added a comment - - edited Ok I'm done with this and I've got a PR ready. It will however need tweaks because : Your test does indeed fail (with or without my changes). The gist of the issue here with your test is that jackson will convert the byte[] to Base64 and it fails the test in two ways: Without the fix for this issue it will throw the same error ( can't cast String to [B ) With the fix another error occurs because the Base64 string doesn't follow the NUL separated specs. (thus failing to populate user and password) Seeing as this feature doesn't currently work anyways, I think we have full reign on how to implement it. Converting byte[] to a Base64 string isn't unpleasant in itself and drivers should be able to implement this. My suggestion would be to change the documentation and the feature to work in these two scenarios: ARGS_SASL is of type byte[] and we treat it normally (ensures the feature is BC) ARGS_SASL is of type String and it must be a Base64 encoded string of a byte[] This however implies that the data be passed differently depending on the serializer being used... Which in itself isn't ideal. The only other option I can think of to homogenize the feature would be to remove the expectancy on a byte[] typed variable, but that would be a BC breaking change so alas no. This is an easy implementation so I'm going ahead with this change, if you have any counter indications or we decide not to go with it I can easily revert back.
          dmill Dylan Millikin added a comment -

          Ok I'll give this a go today or tomorrow. It should make 3.0.2

          dmill Dylan Millikin added a comment - Ok I'll give this a go today or tomorrow. It should make 3.0.2
          spmallette Stephen Mallette added a comment - - edited

          I have a feeling that this change won't work for gremlin-driver for plain application/json but maybe I'm wrong. Try this test with it by adding it to GremlinServerAuthIntegrateTest:

          @Test
          public void shouldAuthenticateWithPlainTextOverJSONSerialization() throws Exception {
              final Cluster cluster = Cluster.build().serializer(Serializers.GRAPHSON).credentials("stephen", "password").create();
              final Client client = cluster.connect();
          
              try {
                  assertEquals(2, client.submit("1+1").all().get().get(0).getInt());
                  assertEquals(3, client.submit("1+2").all().get().get(0).getInt());
                  assertEquals(4, client.submit("1+3").all().get().get(0).getInt());
              } finally {
                  cluster.close();
              }
          }
          
          spmallette Stephen Mallette added a comment - - edited I have a feeling that this change won't work for gremlin-driver for plain application/json but maybe I'm wrong. Try this test with it by adding it to GremlinServerAuthIntegrateTest : @Test public void shouldAuthenticateWithPlainTextOverJSONSerialization() throws Exception { final Cluster cluster = Cluster.build().serializer(Serializers.GRAPHSON).credentials( "stephen" , "password" ).create(); final Client client = cluster.connect(); try { assertEquals(2, client.submit( "1+1" ).all().get().get(0).getInt()); assertEquals(3, client.submit( "1+2" ).all().get().get(0).getInt()); assertEquals(4, client.submit( "1+3" ).all().get().get(0).getInt()); } finally { cluster.close(); } }

          Thanks for digging into this one. I needed to dig in myself to make sure I understood what was going on and your description on this issue helped make things easier. You have about half of the solution - the other half is in how to do this without breaking stuff that's working (e.g. gryo and GraphSON with embedded types). Here's how I think you should formulate your pull request:

          You suggested replacing this line with your code:

          https://github.com/apache/incubator-tinkerpop/blob/ad27fce579a182de3ebf886fdbd85d5960852bdd/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/SaslAuthenticationHandler.java#L76

          To go a step further, I think you should test the type of ARGS_SASL and determine if it is a String or byte[] and then use your code or cast to byte[] accordingly. If it is neither of those options you should send back an error message (use the UNAUTHORIZED message as an example in the SaslAuthenticationHandler. I think you should use ResponseCode.REQUEST_ERROR_MALFORMED_REQUEST.

          This work should be based on the tp30 branch that way we get this fix for 3.0.2 which we are preparing for release 10/19.

          Does that make sense? Can you submit a pull request in the next few days based on that information?

          spmallette Stephen Mallette added a comment - Thanks for digging into this one. I needed to dig in myself to make sure I understood what was going on and your description on this issue helped make things easier. You have about half of the solution - the other half is in how to do this without breaking stuff that's working (e.g. gryo and GraphSON with embedded types). Here's how I think you should formulate your pull request: You suggested replacing this line with your code: https://github.com/apache/incubator-tinkerpop/blob/ad27fce579a182de3ebf886fdbd85d5960852bdd/gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/SaslAuthenticationHandler.java#L76 To go a step further, I think you should test the type of ARGS_SASL and determine if it is a String or byte[] and then use your code or cast to byte[] accordingly. If it is neither of those options you should send back an error message (use the UNAUTHORIZED message as an example in the SaslAuthenticationHandler . I think you should use ResponseCode.REQUEST_ERROR_MALFORMED_REQUEST . This work should be based on the tp30 branch that way we get this fix for 3.0.2 which we are preparing for release 10/19. Does that make sense? Can you submit a pull request in the next few days based on that information?

          People

            spmallette Stephen Mallette
            dmill Dylan Millikin
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: