Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.8.0
    • Fix Version/s: 1.7.3, 1.8.1, 2.0.0
    • Component/s: proxy
    • Labels:
      None

      Description

      The system permission list between core and Thrift proxy is inconsistent. The proxy lacks the support for some of the newly added system permissions, including:
      System.CREATE_NAMESPACE
      System.DROP_NAMESPACE
      System.ALTER_NAMESPACE
      System.OBTAIN_DELEGATION_TOKEN

      Currently, when connecting through Thrift proxy, we can't grant, check or revoke the above 4 System permissions. When a proxy client sends permissions (i.e., System.CREATE_NAMESPACE), it will receive AccumuloException wrapping around java.lang.NullPointerException:

      Traceback (most recent call last):
        File "Client.py", line 32, in <module>
          client.grantSystemPermission(login, username, CREATE_NAMESPACE_PERM)
        File "***AccumuloProxy.py", line 2980, in grantSystemPermission
          self.recv_grantSystemPermission()
        File "***AccumuloProxy.py", line 3006, in recv_grantSystemPermission
          raise result.ouch1
      accumulo.ttypes.AccumuloException: AccumuloException(msg='java.lang.NullPointerException')
      

      The bug is in the Thrift proxy file

      accumulo/proxy/src/main/thrift/proxy/thrift
      enum SystemPermission {
        GRANT = 0,
        CREATE_TABLE = 1,
        DROP_TABLE = 2,
        ALTER_TABLE = 3,
        CREATE_USER = 4,
        DROP_USER = 5,
        ALTER_USER = 6,
        SYSTEM = 7,
      }
      

      The SystemPermission enum clearly misses Permission #8--#11 defined in Accumulo core:

      accumulo/core/.../SystemPermission.java
      public enum SystemPermission {
        /*
         * One may add new permissions, but new permissions must use new numbers. Current numbers in use must not be changed.
         */
        GRANT((byte) 0),
        CREATE_TABLE((byte) 1),
        DROP_TABLE((byte) 2),
        ALTER_TABLE((byte) 3),
        CREATE_USER((byte) 4),
        DROP_USER((byte) 5),
        ALTER_USER((byte) 6),
        SYSTEM((byte) 7),
        CREATE_NAMESPACE((byte) 8),
        DROP_NAMESPACE((byte) 9),
        ALTER_NAMESPACE((byte) 10),
        OBTAIN_DELEGATION_TOKEN((byte) 11);
      }
      

      The fix should be straightforward---just add the corresponding permissions into the Thrift proxy file.

      Let me know if you need any more info, or want a patch for this.

      Thanks!

        Issue Links

          Activity

          Hide
          elserj Josh Elser added a comment -

          Thanks for letting us know about this issue, Yudong Wu!

          A patch would be great, ideally you could add some new tests to SimpleProxyBase.java which would outline this lacking properties too

          Let us know if you need any help.

          Show
          elserj Josh Elser added a comment - Thanks for letting us know about this issue, Yudong Wu ! A patch would be great, ideally you could add some new tests to SimpleProxyBase.java which would outline this lacking properties too Let us know if you need any help.
          Hide
          ctubbsii Christopher Tubbs added a comment -

          Fixing this in older branches will violate semver, but I think it's probably acceptable in this case. To fix the bug and make this part of the proxy functional again, we need to add to its public API these additional enums.

          We should also add a unit test for the proxy to check for new enums without the corresponding proxy enums. That way, this won't happen again.

          Show
          ctubbsii Christopher Tubbs added a comment - Fixing this in older branches will violate semver, but I think it's probably acceptable in this case. To fix the bug and make this part of the proxy functional again, we need to add to its public API these additional enums. We should also add a unit test for the proxy to check for new enums without the corresponding proxy enums. That way, this won't happen again.
          Hide
          elserj Josh Elser added a comment -

          we need to add to its public API these additional enums

          I didn't think the thrift proxy was covered by the public API definition... Either way, fixing negligence is good.

          Show
          elserj Josh Elser added a comment - we need to add to its public API these additional enums I didn't think the thrift proxy was covered by the public API definition... Either way, fixing negligence is good.
          Hide
          cutylewiwi Yudong Wu added a comment -

          BTW what are public APIs need to fix?

          Show
          cutylewiwi Yudong Wu added a comment - BTW what are public APIs need to fix?
          Hide
          elserj Josh Elser added a comment -

          what are public APIs need to fix?

          He's referring to the SystemPermission enum that you outlined in the description here.

          The "Public API" in terms of semantic versioning is defined by https://git-wip-us.apache.org/repos/asf?p=accumulo.git;a=blob;f=README.md;hb=refs/heads/master#l70

          This is the normal Java client API, and, to my understanding, does not include the Thrift proxy API.

          Show
          elserj Josh Elser added a comment - what are public APIs need to fix? He's referring to the SystemPermission enum that you outlined in the description here. The "Public API" in terms of semantic versioning is defined by https://git-wip-us.apache.org/repos/asf?p=accumulo.git;a=blob;f=README.md;hb=refs/heads/master#l70 This is the normal Java client API, and, to my understanding, does not include the Thrift proxy API.
          Hide
          cutylewiwi Yudong Wu added a comment - - edited

          Oh, I see.

          Yep those API does not relate to the enum defined in Thrift proxy files I mentioned before. That API should refer to permissions related information in package org.apache.accumulo.core.security, where no permission absence happened.

          Show
          cutylewiwi Yudong Wu added a comment - - edited Oh, I see. Yep those API does not relate to the enum defined in Thrift proxy files I mentioned before. That API should refer to permissions related information in package org.apache.accumulo.core.security , where no permission absence happened.
          Hide
          cutylewiwi Yudong Wu added a comment -

          patch against trunk

          Show
          cutylewiwi Yudong Wu added a comment - patch against trunk
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 pylint 0m 0s Pylint was not available.
          0 rubocop 0m 0s rubocop was not available.
          0 ruby-lint 0m 0s Ruby-lint was not available.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
          +1 mvninstall 0m 54s master passed
          +1 compile 0m 18s master passed
          +1 checkstyle 0m 6s master passed
          +1 mvneclipse 0m 16s master passed
          +1 findbugs 1m 8s master passed
          +1 javadoc 0m 22s master passed
          +1 mvninstall 0m 24s the patch passed
          +1 compile 0m 18s the patch passed
          +1 cc 0m 18s the patch passed
          +1 javac 0m 18s the patch passed
          +1 checkstyle 0m 6s the patch passed
          +1 mvneclipse 0m 13s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 19s the patch passed
          +1 javadoc 0m 21s the patch passed
          +1 unit 12m 47s root in the patch passed.
          +1 asflicense 0m 5s The patch does not generate ASF License warnings.
          18m 51s



          Subsystem Report/Notes
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12839670/ACCUMULO-4519.0000.patch
          JIRA Issue ACCUMULO-4519
          Optional Tests asflicense javac javadoc unit cc compile findbugs checkstyle pylint rubocop ruby_lint
          uname Linux asf917.gq1.ygridcore.net 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /home/jenkins/jenkins-slave/workspace/PreCommit-ACCUMULO-Build/test_framework/yetus-0.3.0/lib/precommit/personality/accumulo.sh
          git revision master / 2551c2a
          Default Java 1.8.0_102
          findbugs v3.0.1
          Test Results https://builds.apache.org/job/PreCommit-ACCUMULO-Build/52/testReport/
          modules C: proxy U: proxy
          Console output https://builds.apache.org/job/PreCommit-ACCUMULO-Build/52/console
          Powered by Apache Yetus 0.3.0 http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pylint 0m 0s Pylint was not available. 0 rubocop 0m 0s rubocop was not available. 0 ruby-lint 0m 0s Ruby-lint was not available. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 0m 54s master passed +1 compile 0m 18s master passed +1 checkstyle 0m 6s master passed +1 mvneclipse 0m 16s master passed +1 findbugs 1m 8s master passed +1 javadoc 0m 22s master passed +1 mvninstall 0m 24s the patch passed +1 compile 0m 18s the patch passed +1 cc 0m 18s the patch passed +1 javac 0m 18s the patch passed +1 checkstyle 0m 6s the patch passed +1 mvneclipse 0m 13s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 19s the patch passed +1 javadoc 0m 21s the patch passed +1 unit 12m 47s root in the patch passed. +1 asflicense 0m 5s The patch does not generate ASF License warnings. 18m 51s Subsystem Report/Notes JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12839670/ACCUMULO-4519.0000.patch JIRA Issue ACCUMULO-4519 Optional Tests asflicense javac javadoc unit cc compile findbugs checkstyle pylint rubocop ruby_lint uname Linux asf917.gq1.ygridcore.net 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /home/jenkins/jenkins-slave/workspace/PreCommit-ACCUMULO-Build/test_framework/yetus-0.3.0/lib/precommit/personality/accumulo.sh git revision master / 2551c2a Default Java 1.8.0_102 findbugs v3.0.1 Test Results https://builds.apache.org/job/PreCommit-ACCUMULO-Build/52/testReport/ modules C: proxy U: proxy Console output https://builds.apache.org/job/PreCommit-ACCUMULO-Build/52/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          elserj Josh Elser added a comment -

          Thanks for the patch, Yudong Wu. It looks good to me.

          Might you be able to add a test to SimpleProxyBase.java that verifies that the Namespace permissions work as expected? I would skip the delegation token one – that would be trickier to test.

          Show
          elserj Josh Elser added a comment - Thanks for the patch, Yudong Wu . It looks good to me. Might you be able to add a test to SimpleProxyBase.java that verifies that the Namespace permissions work as expected? I would skip the delegation token one – that would be trickier to test.
          Hide
          cutylewiwi Yudong Wu added a comment -

          Sure, I am glad to help.

          I am not pretty sure about what is the typically routine for such tests. Is it fine that the testing routine will be granting a namespace permission (i. e., System.CREATE_NAMESPACE) to a user, then trying to perform the action controlled by the permission (i.e., trying to create a namespace), following by revoking that permission and trying to perform the action again?

          Show
          cutylewiwi Yudong Wu added a comment - Sure, I am glad to help. I am not pretty sure about what is the typically routine for such tests. Is it fine that the testing routine will be granting a namespace permission (i. e., System.CREATE_NAMESPACE) to a user, then trying to perform the action controlled by the permission (i.e., trying to create a namespace), following by revoking that permission and trying to perform the action again?
          Hide
          elserj Josh Elser added a comment -

          Is it fine that the testing routine will be granting a namespace permission (i. e., System.CREATE_NAMESPACE) to a user, then trying to perform the action controlled by the permission (i.e., trying to create a namespace), following by revoking that permission and trying to perform the action again?

          That would be perfect!

          Show
          elserj Josh Elser added a comment - Is it fine that the testing routine will be granting a namespace permission (i. e., System.CREATE_NAMESPACE) to a user, then trying to perform the action controlled by the permission (i.e., trying to create a namespace), following by revoking that permission and trying to perform the action again? That would be perfect!
          Hide
          mjwall Michael Wall added a comment -

          Josh Elser Christopher Tubbs is this ready? Planning to push to 1.8.2 but wanted to ping you in case this is ready.

          Show
          mjwall Michael Wall added a comment - Josh Elser Christopher Tubbs is this ready? Planning to push to 1.8.2 but wanted to ping you in case this is ready.
          Hide
          elserj Josh Elser added a comment -

          IIRC, we still don't have a test to prevent regressions.

          Yudong Wu offered to write a test, but I don't think that happened. Might you have time to do that before our next release, Yudong Wu (within the next 7 days or so)?

          Show
          elserj Josh Elser added a comment - IIRC, we still don't have a test to prevent regressions. Yudong Wu offered to write a test, but I don't think that happened. Might you have time to do that before our next release, Yudong Wu (within the next 7 days or so)?
          Hide
          mjwall Michael Wall added a comment -

          If that happens, please move back to 1.8.1.

          Show
          mjwall Michael Wall added a comment - If that happens, please move back to 1.8.1.
          Hide
          ctubbsii Christopher Tubbs added a comment -

          This limits the usability of the proxy significantly. I think we should apply it in 1.8.1. We can always add a regression test later, in 1.8.2.

          Show
          ctubbsii Christopher Tubbs added a comment - This limits the usability of the proxy significantly. I think we should apply it in 1.8.1. We can always add a regression test later, in 1.8.2.
          Hide
          mjwall Michael Wall added a comment -

          This patch going in shortly. Christopher Tubbs is merging up and regenerating the thrift files (Thanks Christopher). Yudong Wu may we add you to the list of contributors, http://accumulo.apache.org/people/#contributors?

          Show
          mjwall Michael Wall added a comment - This patch going in shortly. Christopher Tubbs is merging up and regenerating the thrift files (Thanks Christopher). Yudong Wu may we add you to the list of contributors, http://accumulo.apache.org/people/#contributors?
          Hide
          ctubbsii Christopher Tubbs added a comment -

          Applied patch with generated thrift in a separate commit.

          Show
          ctubbsii Christopher Tubbs added a comment - Applied patch with generated thrift in a separate commit.

            People

            • Assignee:
              cutylewiwi Yudong Wu
              Reporter:
              cutylewiwi Yudong Wu
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - Not Specified
                Not Specified
                Remaining:
                Remaining Estimate - 0h
                0h
                Logged:
                Time Spent - 1h 20m
                1h 20m

                  Development