Hive
  1. Hive
  2. HIVE-5989

Hive metastore authorization check is not threadsafe

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 0.11.0, 0.12.0, 0.12.1
    • Fix Version/s: 0.13.0
    • Component/s: Metastore, Security
    • Labels:
      None

      Description

      Metastore-side authorization has a couple of pretty important threadsafety bugs in it:

      a) The HiveMetastoreAuthenticated instantiated by the AuthorizationPreEventListener is static. This is a premature optimization and incorrect, as it will result in Authenticator implementations that store state potentially giving an incorrect result, and this bug very much exists with the DefaultMetastoreAuthenticator.

      b) It assumes HMSHandler.getHiveConf() is itself going to be thread-safe, which it is not. HMSHandler.getConf() is the appropriate thread-safe equivalent.

      The effect of this bug is that if there are two users that are concurrently running jobs on the metastore, we might :

      a) Allow a user to do something they didn't have permission to, because the other person did. (Security hole)
      b) Disallow a user from doing something they should have permission to (More common - annoying and can cause job failures)

      1. SleepyAP.patch
        5 kB
        Sushanth Sowmyan
      2. HIVE-5989.patch
        9 kB
        Sushanth Sowmyan
      3. HIVE-5989.2.patch
        9 kB
        Sushanth Sowmyan

        Activity

        Hide
        Thejas M Nair added a comment -

        Patch committed to trunk.
        Thanks for the contribution Sushanth!

        Show
        Thejas M Nair added a comment - Patch committed to trunk. Thanks for the contribution Sushanth!
        Hide
        Sushanth Sowmyan added a comment -

        Thanks, and yup, the indentation is incorrect in those two places. I've attached a newer patch with that updated.

        Show
        Sushanth Sowmyan added a comment - Thanks, and yup, the indentation is incorrect in those two places. I've attached a newer patch with that updated.
        Hide
        Thejas M Nair added a comment -

        +1 . Just a minor indentation issue in the patch, I can add those 2 spaces before commit.

        Show
        Thejas M Nair added a comment - +1 . Just a minor indentation issue in the patch, I can add those 2 spaces before commit.
        Hide
        Sushanth Sowmyan added a comment -
        Show
        Sushanth Sowmyan added a comment - Created now : https://reviews.apache.org/r/18054/
        Hide
        Sushanth Sowmyan added a comment -

        Reviewboard is not responding currently when I attempt to create a new review request, I will try to create it later tonight.

        Show
        Sushanth Sowmyan added a comment - Reviewboard is not responding currently when I attempt to create a new review request, I will try to create it later tonight.
        Hide
        Thejas M Nair added a comment -

        Sushanth Sowmyan Can you also create a reviewboard link ?

        Show
        Thejas M Nair added a comment - Sushanth Sowmyan Can you also create a reviewboard link ?
        Hide
        Vaibhav Gumashta added a comment -

        Thanks for taking a look Sushanth Sowmyan!

        Show
        Vaibhav Gumashta added a comment - Thanks for taking a look Sushanth Sowmyan !
        Hide
        Sushanth Sowmyan added a comment -

        Much appreciated Mithun.

        And Vaibhav, I had a look at the jira you linked to - I don't think the two are related - this bug related to the metastore auth system using a non-threadlocal hiveconf to instantiate itself and test for ownership. It's not that hive has an idea of incorrect ugi, it's that the metastore auth system uses it incorrectly.

        Show
        Sushanth Sowmyan added a comment - Much appreciated Mithun. And Vaibhav, I had a look at the jira you linked to - I don't think the two are related - this bug related to the metastore auth system using a non-threadlocal hiveconf to instantiate itself and test for ownership. It's not that hive has an idea of incorrect ugi, it's that the metastore auth system uses it incorrectly.
        Show
        Vaibhav Gumashta added a comment - This might be related: HS2 creates DBs/Tables with wrong ownership when HMS setugi is true .
        Hide
        Mithun Radhakrishnan added a comment -

        As a matter of fact, I think we're hitting this right now. I'll review your patch as well.

        Show
        Mithun Radhakrishnan added a comment - As a matter of fact, I think we're hitting this right now. I'll review your patch as well.
        Hide
        Sushanth Sowmyan added a comment -

        Thejas M Nair, could I please get a review on this? I'm not certain if this is affected by any of your newer patches, but this is a pretty important bug at scale.

        Show
        Sushanth Sowmyan added a comment - Thejas M Nair , could I please get a review on this? I'm not certain if this is affected by any of your newer patches, but this is a pretty important bug at scale.
        Hide
        Hive QA added a comment -

        Overall: -1 at least one tests failed

        Here are the results of testing the latest attachment:
        https://issues.apache.org/jira/secure/attachment/12617917/HIVE-5989.patch

        ERROR: -1 due to 2 failed/errored test(s), 4761 tests executed
        Failed tests:

        org.apache.hadoop.hive.cli.TestCliDriver.testCliDriver_decimal_precision
        org.apache.hadoop.hive.cli.TestMinimrCliDriver.testCliDriver_bucket_num_reducers
        

        Test results: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/584/testReport
        Console output: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/584/console

        Messages:

        Executing org.apache.hive.ptest.execution.PrepPhase
        Executing org.apache.hive.ptest.execution.ExecutionPhase
        Executing org.apache.hive.ptest.execution.ReportingPhase
        Tests exited with: TestsFailedException: 2 tests failed
        

        This message is automatically generated.

        ATTACHMENT ID: 12617917

        Show
        Hive QA added a comment - Overall : -1 at least one tests failed Here are the results of testing the latest attachment: https://issues.apache.org/jira/secure/attachment/12617917/HIVE-5989.patch ERROR: -1 due to 2 failed/errored test(s), 4761 tests executed Failed tests: org.apache.hadoop.hive.cli.TestCliDriver.testCliDriver_decimal_precision org.apache.hadoop.hive.cli.TestMinimrCliDriver.testCliDriver_bucket_num_reducers Test results: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/584/testReport Console output: http://bigtop01.cloudera.org:8080/job/PreCommit-HIVE-Build/584/console Messages: Executing org.apache.hive.ptest.execution.PrepPhase Executing org.apache.hive.ptest.execution.ExecutionPhase Executing org.apache.hive.ptest.execution.ReportingPhase Tests exited with: TestsFailedException: 2 tests failed This message is automatically generated. ATTACHMENT ID: 12617917
        Hide
        Sushanth Sowmyan added a comment -

        Setting to patch-available so Hive-QA can pick it up to run existing tests with this patch.

        Show
        Sushanth Sowmyan added a comment - Setting to patch-available so Hive-QA can pick it up to run existing tests with this patch.
        Hide
        Sushanth Sowmyan added a comment -

        Attached patch.

        Show
        Sushanth Sowmyan added a comment - Attached patch.
        Hide
        Sushanth Sowmyan added a comment -

        SleepyAP.patch attached - Note, before using this for testing, you'll have to set hive.metastore.client.socket.timeout to an appropriately high amount (I used 60000 for 1 minute) to make sure that we don't hit a timeout on the client.

        Show
        Sushanth Sowmyan added a comment - SleepyAP.patch attached - Note, before using this for testing, you'll have to set hive.metastore.client.socket.timeout to an appropriately high amount (I used 60000 for 1 minute) to make sure that we don't hit a timeout on the client.
        Hide
        Sushanth Sowmyan added a comment -

        I'll attach the fix patch for now as this is a pretty severe bug. As for testing, it is easy to test from manual tests or with e2e tests, but difficult to test with a unit test. I'll also attach a "SleepyAuthorizationProvider" that I used to test this manually, so as to elongate the critical section to demonstrate and reproduce this error easily.

        Show
        Sushanth Sowmyan added a comment - I'll attach the fix patch for now as this is a pretty severe bug. As for testing, it is easy to test from manual tests or with e2e tests, but difficult to test with a unit test. I'll also attach a "SleepyAuthorizationProvider" that I used to test this manually, so as to elongate the critical section to demonstrate and reproduce this error easily.

          People

          • Assignee:
            Sushanth Sowmyan
            Reporter:
            Sushanth Sowmyan
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development