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.

        Activity

        Hide
        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?

        Show
        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?
        Hide
        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();
            }
        }
        
        Show
        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(); } }
        Hide
        dmill Dylan Millikin added a comment -

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

        Show
        dmill Dylan Millikin added a comment - Ok I'll give this a go today or tomorrow. It should make 3.0.2
        Hide
        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.

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


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

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

        Github user asfgit closed the pull request at:

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

        Show
        githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/incubator-tinkerpop/pull/98
        Hide
        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.

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

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development