Bigtop
  1. Bigtop
  2. BIGTOP-1326

iTest Shell does fully work under root user only

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 0.8.0
    • Fix Version/s: 1.1.0
    • Component/s: documentation, tests
    • Labels:
      None

      Description

      When you specify different effective user for a shell object, eg:

      Shell shHDFS = new Shell("/bin/bash", "hdfs")
      

      It will try to run the following command to do it:

      sudo -u hdfs PATH=$PATH /bin/bash
      

      For reference, this is defined in bigtop-test-framework/src/main/groovy/org/apache/bigtop/itest/shell/Shell.groovy on by:

      def proc = user ? "sudo -u $user PATH=${System.getenv('PATH')} $shell".execute() : "$shell".execute()
      

      This means that without further sudo configuration, such code would only work if you are running it under root (edited for clarity).

      To run it under user bigtop who is single member of group bigtop, you need
      to add the following sudo configuration file into /etc/sudoers.d directory:

      Defaults    env_keep += "PATH"
      Defaults    exempt_group = bigtop
      
      bigtop  ALL= (root) NOPASSWD: /bin/bash
      bigtop  ALL= (hdfs) NOPASSWD: /bin/bash
      

      The first two lines allows bigtop user to pass PATH env variable to process
      executed via sudo.

      Since sudo (and for quite good reasons) redefines enviroment variables of the
      process it executes (see env_reset sudoers option) and morevoer sets PATH to
      predefined safe value (see secure_path sudoers option), first two lines are
      needed:

      • first line allows passing PATH in general, but it doesn't have any effect
        alone alone because of secure_path which always redefines it anyway
      • second line allows users of group bigtop to pass or redefine PATH enviroment
        variable, but it doesn't work without the first line

      In addition, when you do this sudo configuration, you don't need to set PATH
      explicitelly as done in previous example, but the PATH is passed by default
      (but only for members of bigtop group), so this is enough:

      sudo -u hdfs /bin/bash
      

      On the other hand we can't remove explicit passing of PATH variable, because
      it would break it for the root user.

      So I propose the following:

      • discuss this if workflow is ok to be declared as official way how to use
        iTest shell object under non root users
      • then I will attach a patch for documention this in some README file
        (any ideas where it would be best to add it?)
      • does it make sense to run most of the bigtop tests under non root user until there is a good reason to use another one? I think that running tests under root user by default is not optimal because of the complexity of hadoop environment.
      1. BIGTOP-1326.1.patch
        10 kB
        Martin Bukatovic

        Issue Links

          Activity

          Martin Bukatovic created issue -
          Hide
          Konstantin Boudnik added a comment - - edited

          This means that such code would only work if you are running it under root.

          Actually, it isn't completely true - it will also work under any user with permissions to run commands without password check. Which you of course has explicitely stated later in the description.

          Show
          Konstantin Boudnik added a comment - - edited This means that such code would only work if you are running it under root. Actually, it isn't completely true - it will also work under any user with permissions to run commands without password check. Which you of course has explicitely stated later in the description.
          Martin Bukatovic made changes -
          Field Original Value New Value
          Description When you specify different effective user for a shell object, eg:

          {noformat}
          Shell shHDFS = new Shell("/bin/bash", "hdfs")
          {noformat}

          It will try to run the following command to do it:

          {noformat}
          sudo -u hdfs PATH=$PATH /bin/bash
          {noformat}

          For reference, this is defined in {{bigtop-test-framework/src/main/groovy/org/apache/bigtop/itest/shell/Shell.groovy}} on by:

          {noformat}
          def proc = user ? "sudo -u $user PATH=${System.getenv('PATH')} $shell".execute() : "$shell".execute()
          {noformat}

          This means that such code would only work if you are running it under root.

          To run it under user {{bigtop}} who is single member of group bigtop, you need
          to add the following sudo configuration file into {{/etc/sudoers.d}} directory:

          {noformat}
          Defaults env_keep += "PATH"
          Defaults exempt_group = bigtop

          bigtop ALL= (root) NOPASSWD: /bin/bash
          bigtop ALL= (hdfs) NOPASSWD: /bin/bash
          {noformat}

          The first two lines allows bigtop user to pass PATH env variable to process
          executed via sudo.

          Since sudo (and for quite good reasons) redefines enviroment variables of the
          process it executes (see {{env_reset}} sudoers option) and morevoer sets PATH to
          predefined safe value (see {{secure_path}} sudoers option), first two lines are
          needed:

           * first line allows passing PATH in general, but it doesn't have any effect
             alone alone because of {{secure_path}} which always redefines it anyway
           * second line allows users of group bigtop to pass or redefine PATH enviroment
             variable, but it doesn't work without the first line

          In addition, when you do this sudo configuration, you don't need to set PATH
          explicitelly as done in previous example, but the PATH is passed by default
          (but only for members of bigtop group), so this is enough:

          {noformat}
          sudo -u hdfs /bin/bash
          {noformat}

          On the other hand we can't remove explicit passing of PATH variable, because
          it would break it for the root user.

          So I propose the following:

           * discuss this if workflow is ok to be declared as official way how to use
             iTest shell object under non root users
           * then I will attach a patch for documention this in some README file
             (any ideas where it would be best to add it?)
           * does it make sense to run most of the bigtop tests under non root user until there is a good reason to use another one? I think that running tests under root user by default is not optimal because of the complexity of hadoop environment.
          When you specify different effective user for a shell object, eg:

          {noformat}
          Shell shHDFS = new Shell("/bin/bash", "hdfs")
          {noformat}

          It will try to run the following command to do it:

          {noformat}
          sudo -u hdfs PATH=$PATH /bin/bash
          {noformat}

          For reference, this is defined in {{bigtop-test-framework/src/main/groovy/org/apache/bigtop/itest/shell/Shell.groovy}} on by:

          {noformat}
          def proc = user ? "sudo -u $user PATH=${System.getenv('PATH')} $shell".execute() : "$shell".execute()
          {noformat}

          This means that without further sudo configuration, such code would only work if you are running it under root (edited for clarity).

          To run it under user {{bigtop}} who is single member of group bigtop, you need
          to add the following sudo configuration file into {{/etc/sudoers.d}} directory:

          {noformat}
          Defaults env_keep += "PATH"
          Defaults exempt_group = bigtop

          bigtop ALL= (root) NOPASSWD: /bin/bash
          bigtop ALL= (hdfs) NOPASSWD: /bin/bash
          {noformat}

          The first two lines allows bigtop user to pass PATH env variable to process
          executed via sudo.

          Since sudo (and for quite good reasons) redefines enviroment variables of the
          process it executes (see {{env_reset}} sudoers option) and morevoer sets PATH to
          predefined safe value (see {{secure_path}} sudoers option), first two lines are
          needed:

           * first line allows passing PATH in general, but it doesn't have any effect
             alone alone because of {{secure_path}} which always redefines it anyway
           * second line allows users of group bigtop to pass or redefine PATH enviroment
             variable, but it doesn't work without the first line

          In addition, when you do this sudo configuration, you don't need to set PATH
          explicitelly as done in previous example, but the PATH is passed by default
          (but only for members of bigtop group), so this is enough:

          {noformat}
          sudo -u hdfs /bin/bash
          {noformat}

          On the other hand we can't remove explicit passing of PATH variable, because
          it would break it for the root user.

          So I propose the following:

           * discuss this if workflow is ok to be declared as official way how to use
             iTest shell object under non root users
           * then I will attach a patch for documention this in some README file
             (any ideas where it would be best to add it?)
           * does it make sense to run most of the bigtop tests under non root user until there is a good reason to use another one? I think that running tests under root user by default is not optimal because of the complexity of hadoop environment.
          Hide
          Martin Bukatovic added a comment -

          Correct, I edited the line a bit to point this out directly there. But my main point was that even user with sudo permissions to run commands without password check wouldn't be able to use it because it will fail on explicit path export, see what will happen when I remove first two lines (Defaults env_keep += "PATH", Defaults exempt_group = bigtop) from bigtop sudo config file (as listed above):

          [bigtop@testserver ~]$ sudo -u hdfs PATH=$PATH /bin/bash
          sudo: sorry, you are not allowed to set the following environment variables: PATH
          [bigtop@testserver ~]$ sudo -u hdfs /bin/bash
          bash-4.1$ 
          
          Show
          Martin Bukatovic added a comment - Correct, I edited the line a bit to point this out directly there. But my main point was that even user with sudo permissions to run commands without password check wouldn't be able to use it because it will fail on explicit path export, see what will happen when I remove first two lines ( Defaults env_keep += "PATH" , Defaults exempt_group = bigtop ) from bigtop sudo config file (as listed above): [bigtop@testserver ~]$ sudo -u hdfs PATH=$PATH /bin/bash sudo: sorry, you are not allowed to set the following environment variables: PATH [bigtop@testserver ~]$ sudo -u hdfs /bin/bash bash-4.1$
          Hide
          Konstantin Boudnik added a comment -

          I see - you're right, of course. I agree with your points and about bad air of running tests under a root object. What do you envision as a possible solution here?

          Show
          Konstantin Boudnik added a comment - I see - you're right, of course. I agree with your points and about bad air of running tests under a root object. What do you envision as a possible solution here?
          Hide
          Martin Bukatovic added a comment -

          I think that the best option would be not just document what I have found about non-root user, but also to agree on common test environment for bigtop tests and declare that it's needed to:

          • create user bigtop who will be in bigtop group (and hadoop group maybe?)
          • configure sudo by copying config file listed above into /etc/sudoers.d/20_bigtop
          • run add bigtop tests under bigtop user by default, unless there is a need to use different one (eg. some TestCLI test cases requires privileges of hdfs root user, so this one needs to be run under hdfs user instead of bigtop)

          But I'm not sure how much additional changes of bigtop tests would be needed as I haven't tried to configure my hadoop cluster to run and analyse them all in detail.

          Show
          Martin Bukatovic added a comment - I think that the best option would be not just document what I have found about non-root user, but also to agree on common test environment for bigtop tests and declare that it's needed to: create user bigtop who will be in bigtop group (and hadoop group maybe?) configure sudo by copying config file listed above into /etc/sudoers.d/20_bigtop run add bigtop tests under bigtop user by default, unless there is a need to use different one (eg. some TestCLI test cases requires privileges of hdfs root user, so this one needs to be run under hdfs user instead of bigtop) But I'm not sure how much additional changes of bigtop tests would be needed as I haven't tried to configure my hadoop cluster to run and analyse them all in detail.
          Hide
          Konstantin Boudnik added a comment -

          We do users provisioning with Puppet and, I believe, bigtop user is one of them. We totally can do sudoers change if needed as part of that provisioning - no problem with that. I agree though that documenting this requirement is very important for those who don't use our recipes for their cluster provisioning.

          We are trying to continue moving forward with Bigtop book - sources are under src/site/docbookx/. Do you think you can take a stab at the documenting these requirements there? Thanks!

          Show
          Konstantin Boudnik added a comment - We do users provisioning with Puppet and, I believe, bigtop user is one of them. We totally can do sudoers change if needed as part of that provisioning - no problem with that. I agree though that documenting this requirement is very important for those who don't use our recipes for their cluster provisioning. We are trying to continue moving forward with Bigtop book - sources are under src/site/docbookx/ . Do you think you can take a stab at the documenting these requirements there? Thanks!
          Konstantin Boudnik made changes -
          Link This issue is required by BIGTOP-1307 [ BIGTOP-1307 ]
          Hide
          Martin Bukatovic added a comment -

          I'm attaching initial documentation patch BIGTOP-1326.1.patch: so far just the
          short description of this sudo configuration.

          After looking around a bit, I think there are also other places worth updating,
          such as:

          Show
          Martin Bukatovic added a comment - I'm attaching initial documentation patch BIGTOP-1326 .1.patch: so far just the short description of this sudo configuration. After looking around a bit, I think there are also other places worth updating, such as: `bigtop/README.md` (main readme file) - proposed changes included in BIGTOP-1326 .1.patch https://cwiki.apache.org/confluence/display/BIGTOP/Running+integration+and+system+tests I would add there the sudoer config file in similar way to what I have proposed for the main README file `bigtop/bigtop-test-framework/README` there are some setup notes already, but I would need to better understand how BIGTOP-1192 is related into the setup (eg. BIGTOP_SMOKES_USER variable)
          Martin Bukatovic made changes -
          Attachment BIGTOP-1326.1.patch [ 12650781 ]
          Hide
          Roman Shaposhnik added a comment -

          Konstantin Boudnik any updates on this re: 0.8.0?

          Show
          Roman Shaposhnik added a comment - Konstantin Boudnik any updates on this re: 0.8.0?
          Konstantin Boudnik made changes -
          Assignee Martin Bukatovic [ mbukatov ]
          Hide
          Konstantin Boudnik added a comment -

          Martin Bukatovic, I have looked over the patch again and it seems ok. Could you give it one more run to make sure that it is current and reflects everything before I commit?

          Show
          Konstantin Boudnik added a comment - Martin Bukatovic , I have looked over the patch again and it seems ok. Could you give it one more run to make sure that it is current and reflects everything before I commit?
          Hide
          Martin Bukatovic added a comment -

          Konstantin Boudnik I'm currently using it in my enviroment (based on bigtop state from Oct) without any problems. But I will try to recheck based on current plain bigtop enviroment next week and let you know. Btw attached patch also tries to describe itself in the userguide (it's just a first try), but I'm not sure how this should be improved considering the guide seems to be currently empty.

          Show
          Martin Bukatovic added a comment - Konstantin Boudnik I'm currently using it in my enviroment (based on bigtop state from Oct) without any problems. But I will try to recheck based on current plain bigtop enviroment next week and let you know. Btw attached patch also tries to describe itself in the userguide (it's just a first try), but I'm not sure how this should be improved considering the guide seems to be currently empty.
          Konstantin Boudnik made changes -
          Component/s documentation [ 12319676 ]
          Hide
          Konstantin Boudnik added a comment -

          Thanks Martin Bukatovic - appreciate your help!
          I think it is great to have a new chapter in the userguide book - we need to start adding the content there and yours is a very important step!

          Show
          Konstantin Boudnik added a comment - Thanks Martin Bukatovic - appreciate your help! I think it is great to have a new chapter in the userguide book - we need to start adding the content there and yours is a very important step!
          Hide
          Konstantin Boudnik added a comment -

          So Martin Bukatovic - shall we commit this?

          Show
          Konstantin Boudnik added a comment - So Martin Bukatovic - shall we commit this?
          Konstantin Boudnik made changes -
          Fix Version/s 0.9.0 [ 12326836 ]
          Hide
          Martin Bukatovic added a comment -

          Konstantin Boudnik I'm sorry but I still haven't setup clean new cluster with Bigtop 8.0 for me to polish this README and documentation patch. Would you mind to wait a little more?

          Show
          Martin Bukatovic added a comment - Konstantin Boudnik I'm sorry but I still haven't setup clean new cluster with Bigtop 8.0 for me to polish this README and documentation patch. Would you mind to wait a little more?
          Hide
          Konstantin Boudnik added a comment -

          Absolutely. I just wanted to make sure that I am not dropping this Thanks

          Show
          Konstantin Boudnik added a comment - Absolutely. I just wanted to make sure that I am not dropping this Thanks
          Hide
          Roman Shaposhnik added a comment -

          I guess I'll add my ping to Cos's one Martin Bukatovic

          Show
          Roman Shaposhnik added a comment - I guess I'll add my ping to Cos's one Martin Bukatovic
          Hide
          Konstantin Boudnik added a comment -

          Sorta related: we just figured out that simply running tests under user hdfs won't fly either, as there are some negative hdfs tests that are expected to fail, but would pass under hdfs user.

          Show
          Konstantin Boudnik added a comment - Sorta related: we just figured out that simply running tests under user hdfs won't fly either, as there are some negative hdfs tests that are expected to fail, but would pass under hdfs user.
          Konstantin Boudnik made changes -
          Fix Version/s 1.0.0 [ 12326837 ]
          Fix Version/s 0.9.0 [ 12326836 ]
          Hide
          Konstantin Boudnik added a comment -

          Can we get it validated for 1.0? If not - let's move it to 1.1.0

          Show
          Konstantin Boudnik added a comment - Can we get it validated for 1.0? If not - let's move it to 1.1.0
          Hide
          Konstantin Boudnik added a comment -

          Any input? Martin Bukatovic could you please take a look? Thanks!

          Show
          Konstantin Boudnik added a comment - Any input? Martin Bukatovic could you please take a look? Thanks!
          Hide
          Konstantin Boudnik added a comment -

          Martin Bukatovic - could you please confirm that this stands so we can close it in 1.0?

          Show
          Konstantin Boudnik added a comment - Martin Bukatovic - could you please confirm that this stands so we can close it in 1.0?
          Konstantin Boudnik made changes -
          Fix Version/s 1.1.0 [ 12329714 ]
          Fix Version/s 1.0.0 [ 12326837 ]
          Hide
          Konstantin Boudnik added a comment -

          This is doc issue, after all and doesn't seem to be blocking the release - moving out for now.

          Show
          Konstantin Boudnik added a comment - This is doc issue, after all and doesn't seem to be blocking the release - moving out for now.

            People

            • Assignee:
              Martin Bukatovic
              Reporter:
              Martin Bukatovic
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:

                Development