Details

    • Hadoop Flags:
      Reviewed
    • Release Note:
      Hide
      Adds job-level authorization to servlets(other than history related servlets) for accessing job related info. Deprecates mapreduce.jobtracker.permissions.supergroup and adds the config mapreduce.cluster.permissions.supergroup at cluster level sothat it will be used by TaskTracker also. Authorization checks are done if authentication is succeeded and mapreduce.cluster.job-authorization-enabled is set to true.
      Show
      Adds job-level authorization to servlets(other than history related servlets) for accessing job related info. Deprecates mapreduce.jobtracker.permissions.supergroup and adds the config mapreduce.cluster.permissions.supergroup at cluster level sothat it will be used by TaskTracker also. Authorization checks are done if authentication is succeeded and mapreduce.cluster.job-authorization-enabled is set to true.

      Description

      This jira is about building the authorization for servlets (on top of MAPREDUCE-1307). That is, the JobTracker/TaskTracker runs authorization checks on web requests based on the configured job permissions. For e.g., if the job permission is 600, then no one except the authenticated user can look at the job details via the browser. The authenticated user in the servlet can be obtained using the HttpServletRequest method.

      1. 1455.v4.patch
        110 kB
        Ravi Gummadi
      2. 1455.v4.2.patch
        110 kB
        Ravi Gummadi
      3. 1455.v4.1.patch
        108 kB
        Ravi Gummadi
      4. 1455.v3.patch
        104 kB
        Ravi Gummadi
      5. 1455.v2.patch
        81 kB
        Ravi Gummadi
      6. 1455.v1.patch
        76 kB
        Ravi Gummadi
      7. 1455.patch
        69 kB
        Ravi Gummadi
      8. 1455.20S.2.patch
        106 kB
        Ravi Gummadi
      9. 1455.20S.2.fix1.patch
        11 kB
        Ravi Gummadi
      10. 1455.20S.2.fix.patch
        0.7 kB
        Ravi Gummadi

        Issue Links

          Activity

          Hide
          Ravi Gummadi added a comment -

          Patch fixing testcase for earlier version of hadoop. Not for commit here.

          Show
          Ravi Gummadi added a comment - Patch fixing testcase for earlier version of hadoop. Not for commit here.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #248 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/248/)

          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #248 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/248/ )
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #253 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/253/)
          . Authorization for servlets. Contributed by Ravi Gummadi.

          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #253 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/253/ ) . Authorization for servlets. Contributed by Ravi Gummadi.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          I've just committed this. Thanks Ravi!

          Show
          Vinod Kumar Vavilapalli added a comment - I've just committed this. Thanks Ravi!
          Hide
          Vinod Kumar Vavilapalli added a comment -

          TestSimulatorStressJobSubmission and TestSimulatorTaskTracker passed on my machine. Seems like some Hudson problem w.r.t pulling core jars. TestMiniMRLocalFS failure is because of MAPREDUCE-1520. I am going to check this in.

          Show
          Vinod Kumar Vavilapalli added a comment - TestSimulatorStressJobSubmission and TestSimulatorTaskTracker passed on my machine. Seems like some Hudson problem w.r.t pulling core jars. TestMiniMRLocalFS failure is because of MAPREDUCE-1520 . I am going to check this in.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12437469/1455.v4.2.patch
          against trunk revision 916823.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 11 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          -1 javac. The patch appears to cause tar ant target to fail.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          -1 contrib tests. The patch failed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/8/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/8/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/8/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/8/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12437469/1455.v4.2.patch against trunk revision 916823. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The patch appears to cause tar ant target to fail. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/8/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/8/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/8/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/8/console This message is automatically generated.
          Hide
          Ravi Gummadi added a comment -

          Unit tests passed on my local machine(except the known failures of MAPREDUCE-1421 and MAPREDUCE-1520).
          ant test-patch gave:

          [exec] +1 overall.
          [exec]
          [exec] +1 @author. The patch does not contain any @author tags.
          [exec]
          [exec] +1 tests included. The patch appears to include 11 new or modified tests.
          [exec]
          [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
          [exec]
          [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
          [exec]
          [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
          [exec]
          [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.

          Show
          Ravi Gummadi added a comment - Unit tests passed on my local machine(except the known failures of MAPREDUCE-1421 and MAPREDUCE-1520 ). ant test-patch gave: [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 11 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          +1 for the latest patch.

          Show
          Vinod Kumar Vavilapalli added a comment - +1 for the latest patch.
          Hide
          Ravi Gummadi added a comment -

          Attaching patch for trunk.

          Please review and provide your comments.

          Show
          Ravi Gummadi added a comment - Attaching patch for trunk. Please review and provide your comments.
          Hide
          Ravi Gummadi added a comment -

          Attaching fix for testcase TestNodeRefresh on top of patch of earlier version of hadoop. Not for commit here.

          Show
          Ravi Gummadi added a comment - Attaching fix for testcase TestNodeRefresh on top of patch of earlier version of hadoop. Not for commit here.
          Hide
          Ravi Gummadi added a comment -

          Attaching patch for Y! 20 branch. Not for commit here.

          Show
          Ravi Gummadi added a comment - Attaching patch for Y! 20 branch. Not for commit here.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          The latest one looks good. +1 for the patch.

          Letting Hudson hammer it.

          Show
          Vinod Kumar Vavilapalli added a comment - The latest one looks good. +1 for the patch. Letting Hudson hammer it.
          Hide
          Ravi Gummadi added a comment -

          Attaching new patch by removing some unused imports from jsps and removing ^M characters from testcase.

          Show
          Ravi Gummadi added a comment - Attaching new patch by removing some unused imports from jsps and removing ^M characters from testcase.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Patch looks good. The test TestWebUIAuthorization has ^M characters. Can you reupload the patch?

          Show
          Vinod Kumar Vavilapalli added a comment - Patch looks good. The test TestWebUIAuthorization has ^M characters. Can you reupload the patch?
          Hide
          Ravi Gummadi added a comment -

          Attaching new patch after some refactoring of code and adding/updating some javadoc.

          Show
          Ravi Gummadi added a comment - Attaching new patch after some refactoring of code and adding/updating some javadoc.
          Hide
          Ravi Gummadi added a comment -

          The failed 2 tests TestFileArgs(MAPREDUCE-1375) and TestMiniMRLocalFS(MAPREDUCE-1520) are known failures in current trunk.

          Show
          Ravi Gummadi added a comment - The failed 2 tests TestFileArgs( MAPREDUCE-1375 ) and TestMiniMRLocalFS( MAPREDUCE-1520 ) are known failures in current trunk.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12436917/1455.v3.patch
          against trunk revision 915223.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 11 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          -1 contrib tests. The patch failed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/486/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/486/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/486/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/486/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12436917/1455.v3.patch against trunk revision 915223. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/486/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/486/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/486/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/486/console This message is automatically generated.
          Hide
          Ravi Gummadi added a comment -

          Incorporated Vinod's offline comments regarding refactoring of code and added more testcases in TestWebUIAuthorization.java

          Attaching new patch for trunk.

          Show
          Ravi Gummadi added a comment - Incorporated Vinod's offline comments regarding refactoring of code and added more testcases in TestWebUIAuthorization.java Attaching new patch for trunk.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12436568/1455.v2.patch
          against trunk revision 912471.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 11 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 1 new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          -1 contrib tests. The patch failed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/474/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/474/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/474/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/474/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12436568/1455.v2.patch against trunk revision 912471. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 1 new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/474/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/474/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/474/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/474/console This message is automatically generated.
          Hide
          Ravi Gummadi added a comment -

          Allowing it to go through Hudson...

          Show
          Ravi Gummadi added a comment - Allowing it to go through Hudson...
          Hide
          Ravi Gummadi added a comment -

          Incorporated review comments and uploading new patch.

          Show
          Ravi Gummadi added a comment - Incorporated review comments and uploading new patch.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          > What about the configuration webinterface.private.actions?
          >>> We need to think of this more and decide.
          Let's open a new issue.

          > The variable 'conf' should actually be removed
          >>> So would it be better to handle this in MAPREDUCE-1493 as that is using getJobInfo() and in turn this "conf" variable ?
          +1

          > Make it something like JSPUtil.checkAccessAndDoOperation(JobOperation).
          >>> Hmm. This may make the method checkAccessAndDoOperation() complex
          OK. We'll leave it as is.

          Show
          Vinod Kumar Vavilapalli added a comment - > What about the configuration webinterface.private.actions? >>> We need to think of this more and decide. Let's open a new issue. > The variable 'conf' should actually be removed >>> So would it be better to handle this in MAPREDUCE-1493 as that is using getJobInfo() and in turn this "conf" variable ? +1 > Make it something like JSPUtil.checkAccessAndDoOperation(JobOperation). >>> Hmm. This may make the method checkAccessAndDoOperation() complex OK. We'll leave it as is.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12436335/1455.v1.patch
          against trunk revision 912471.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 11 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 3 new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          -1 contrib tests. The patch failed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/334/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/334/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/334/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/334/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12436335/1455.v1.patch against trunk revision 912471. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 3 new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/334/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/334/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/334/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/334/console This message is automatically generated.
          Hide
          Ravi Gummadi added a comment -

          >> What about the configuration webinterface.private.actions? It was originally added as part of HADOOP-1484 'cause authentication/authorization were missing back then. Now that we have them in place, it doesn't look like we really need it anymore. I'm fine fixing this in another issue.

          If we remove this config now, the default behaviour changes — in the sense that any user can do modify operations on any job(by default — i.e. if user doesn't enable authorization). This doesn't look good. We need to think of this more and decide.

          >> The variable 'conf' should actually be removed, instead of just putting a warning comment about its usage. We should fix the usage of this conf object in this patch itself, which I find is only at one place in JSPUtil.

          Hmm. This would need changes like JSPUtil.getJobInfo() taking conf as another parameter ---- in turn leads to code changes in all history related JSPs, which are not touched by this patch. So would it be better to handle this in MAPREDUCE-1493 as that is using getJobInfo() and in turn this "conf" variable ?

          >> Can we move the UGI.doAs() checks also from all the JSPs into JSPUtil.checkAccessAndGetJob()?...

          OK

          >> Given above, we can even overload JSPUtil.checkAccessAndGetJob(), (add a new JobOperation enum?) and make it something like JSPUtil.checkAccessAndDoOperation(JobOperation). That will make things much much simpler, I think.

          Hmm. This may make the method checkAccessAndDoOperation() complex as it needs to return JobInProgress object in one case and doesn't have to return anything in other cases(like killJob, setJobPriority, killTask, failTask). Also it needs to take different parameters based on the operation it is going to do. What do you say ?

          Show
          Ravi Gummadi added a comment - >> What about the configuration webinterface.private.actions? It was originally added as part of HADOOP-1484 'cause authentication/authorization were missing back then. Now that we have them in place, it doesn't look like we really need it anymore. I'm fine fixing this in another issue. If we remove this config now, the default behaviour changes — in the sense that any user can do modify operations on any job(by default — i.e. if user doesn't enable authorization). This doesn't look good. We need to think of this more and decide. >> The variable 'conf' should actually be removed, instead of just putting a warning comment about its usage. We should fix the usage of this conf object in this patch itself, which I find is only at one place in JSPUtil. Hmm. This would need changes like JSPUtil.getJobInfo() taking conf as another parameter ---- in turn leads to code changes in all history related JSPs, which are not touched by this patch. So would it be better to handle this in MAPREDUCE-1493 as that is using getJobInfo() and in turn this "conf" variable ? >> Can we move the UGI.doAs() checks also from all the JSPs into JSPUtil.checkAccessAndGetJob()?... OK >> Given above, we can even overload JSPUtil.checkAccessAndGetJob(), (add a new JobOperation enum?) and make it something like JSPUtil.checkAccessAndDoOperation(JobOperation). That will make things much much simpler, I think. Hmm. This may make the method checkAccessAndDoOperation() complex as it needs to return JobInProgress object in one case and doesn't have to return anything in other cases(like killJob, setJobPriority, killTask, failTask). Also it needs to take different parameters based on the operation it is going to do. What do you say ?
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Submitting to Hudson anyways to see if tests are passing or not..

          Show
          Vinod Kumar Vavilapalli added a comment - Submitting to Hudson anyways to see if tests are passing or not..
          Hide
          Vinod Kumar Vavilapalli added a comment -

          The patch looks good functionally. Some code-level comments:

          JSPUtil.java

          • What about the configuration webinterface.private.actions? It was originally added as part of HADOOP-1484 'cause authentication/authorization were missing back then. Now that we have them in place, it doesn't look like we really need it anymore. I'm fine fixing this in another issue.
          • The variable 'conf' should actually be removed, instead of just putting a warning comment about its usage. We should fix the usage of this conf object in this patch itself, which I find is only at one place in JSPUtil.
          • Can we move the UGI.doAs() checks also from all the JSPs into JSPUtil.checkAccessAndGetJob()? Right now, the authorization code for checking view-job-acl is sprinkled and is inconsistent across various JSPs. For e.g., unnecessary AccessControlException handling is being done in TaskGraphServlet. All this will go away if we simply encapsulate all the authorization code inside JSPUtil.checkAccessAndGetJob() and let it return JIP on success or if authorization is disabled and null if authorization fails. Once we do this, all the authorizaton code will be in JSPUtil and once this call returns, we can simply then return back from JSPs.
          • Given above, we can even overload JSPUtil.checkAccessAndGetJob(), (add a new JobOperation enum?) and make it something like JSPUtil.checkAccessAndDoOperation(JobOperation). That will make things much much simpler, I think.

          JobACLsManager.java

          • Very minor: " Access Control List configured for this job : " should instead be "Access control list ... " (letter-case)

          JobTracker.java

          • Can we add the call to refreshUserToGroupsMappings(conf) in JobTracker.offerService() in another JIRA issue? 'Coz I a can think of test-cases for this and adding them here would stretch this issue.

          TaskLog.java

          • Rename taskLogAclFile to jobACLsFile?
          • TaskLogsServlet is also be now passed through servlet filters.Should we add a testcase for web-authentication for this servlet. Split the test into two? One for authentication and one for authorization? Should we enhance the tests in general w.r.t authentication. For e.g. we now are not verifying authentication failures from the filters.

          TaskLogServlet.java

          • We should replace the method checkAccessForTaskLogs() with calls to JobACLsManager now. Perhaps after refactoring around code in JobACLsManager a bit. TaskTracker should also instantiate a JobACLsManager object similar to JT.

          TaskRunner.java

          • Rename writeTaskLogAcls() to writeJobACLs()?
          • New test in TestTrackerLocaliztion to verify proper writing of the acls file on the local disk? Also verify the read back?
          Show
          Vinod Kumar Vavilapalli added a comment - The patch looks good functionally. Some code-level comments: JSPUtil.java What about the configuration webinterface.private.actions ? It was originally added as part of HADOOP-1484 'cause authentication/authorization were missing back then. Now that we have them in place, it doesn't look like we really need it anymore. I'm fine fixing this in another issue. The variable 'conf' should actually be removed, instead of just putting a warning comment about its usage. We should fix the usage of this conf object in this patch itself, which I find is only at one place in JSPUtil. Can we move the UGI.doAs() checks also from all the JSPs into JSPUtil.checkAccessAndGetJob() ? Right now, the authorization code for checking view-job-acl is sprinkled and is inconsistent across various JSPs. For e.g., unnecessary AccessControlException handling is being done in TaskGraphServlet. All this will go away if we simply encapsulate all the authorization code inside JSPUtil.checkAccessAndGetJob() and let it return JIP on success or if authorization is disabled and null if authorization fails. Once we do this, all the authorizaton code will be in JSPUtil and once this call returns, we can simply then return back from JSPs. Given above, we can even overload JSPUtil.checkAccessAndGetJob() , (add a new JobOperation enum?) and make it something like JSPUtil.checkAccessAndDoOperation(JobOperation) . That will make things much much simpler, I think. JobACLsManager.java Very minor: " Access Control List configured for this job : " should instead be "Access control list ... " (letter-case) JobTracker.java Can we add the call to refreshUserToGroupsMappings(conf) in JobTracker.offerService() in another JIRA issue? 'Coz I a can think of test-cases for this and adding them here would stretch this issue. TaskLog.java Rename taskLogAclFile to jobACLsFile? TaskLogsServlet is also be now passed through servlet filters.Should we add a testcase for web-authentication for this servlet. Split the test into two? One for authentication and one for authorization? Should we enhance the tests in general w.r.t authentication. For e.g. we now are not verifying authentication failures from the filters. TaskLogServlet.java We should replace the method checkAccessForTaskLogs() with calls to JobACLsManager now. Perhaps after refactoring around code in JobACLsManager a bit. TaskTracker should also instantiate a JobACLsManager object similar to JT. TaskRunner.java Rename writeTaskLogAcls() to writeJobACLs() ? New test in TestTrackerLocaliztion to verify proper writing of the acls file on the local disk? Also verify the read back?
          Hide
          Ravi Gummadi added a comment -

          Attaching new patch for MAPREDUCE-1455 on top of latest patch of MAPREDUCE-1307.

          Other changes in this patch compared to earlier patch include

          (1) jobdetails.jsp shows job-ACLs configured for the job.
          (2) Made error page display the job-acls for the particular job.
          (3) Fixed an existing security issue in all the modified jsps sothat they build (a) tipid and jobid from taskid(i.e. attemptid) instead of taking all of them as parameters, if taskid is input to jsp (b) jobid from tipid instead of taking jobid also as another parameter, if tipid is input to jsp. Without this fix, people can modify only jobid in the URL and specify tipid and taskid of other jobs and can access those tasks' pages.
          (4) refactored code to avoid code duplication related to handling AccessControlException.

          Please review and provide your comments.

          Show
          Ravi Gummadi added a comment - Attaching new patch for MAPREDUCE-1455 on top of latest patch of MAPREDUCE-1307 . Other changes in this patch compared to earlier patch include (1) jobdetails.jsp shows job-ACLs configured for the job. (2) Made error page display the job-acls for the particular job. (3) Fixed an existing security issue in all the modified jsps sothat they build (a) tipid and jobid from taskid(i.e. attemptid) instead of taking all of them as parameters, if taskid is input to jsp (b) jobid from tipid instead of taking jobid also as another parameter, if tipid is input to jsp. Without this fix, people can modify only jobid in the URL and specify tipid and taskid of other jobs and can access those tasks' pages. (4) refactored code to avoid code duplication related to handling AccessControlException. Please review and provide your comments.
          Hide
          Ravi Gummadi added a comment -

          Attaching patch that adds authorization to servlets and jsps as mentioned in earlier comments.

          This patch makes the config property mapreduce.jobtracker.permissions.supergroup cluster level. New config is mapreduce.cluster.permissions.supergroup. All members of this supergroup can view and modify the job.

          This patch needs to be applied on top of MAPREDUCE-1307.

          Please review and provide your comments.

          Show
          Ravi Gummadi added a comment - Attaching patch that adds authorization to servlets and jsps as mentioned in earlier comments. This patch makes the config property mapreduce.jobtracker.permissions.supergroup cluster level. New config is mapreduce.cluster.permissions.supergroup. All members of this supergroup can view and modify the job. This patch needs to be applied on top of MAPREDUCE-1307 . Please review and provide your comments.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Does it make sense to have the info in the log.info file that is already there in the task logs directory?

          This isn't much different from writing a separate file in attempt directory except in the number of files per attempt. In both the approaches, there is a duplication of ACLs information across various tasks of the same job.

          MAPREDUCE-927 is planning to change the logs directory structure from userlogs/$attemptid to userlogs/$jobid/$attemptid. Once that is done, we can simply write one ACL file per job in the userlogs/$jobid directory. This will avoid the duplication of information and can be done. Unless we are really very concerned about adding a new file per job after MAPREDUCE-927.

          Show
          Vinod Kumar Vavilapalli added a comment - Does it make sense to have the info in the log.info file that is already there in the task logs directory? This isn't much different from writing a separate file in attempt directory except in the number of files per attempt. In both the approaches, there is a duplication of ACLs information across various tasks of the same job. MAPREDUCE-927 is planning to change the logs directory structure from userlogs/$attemptid to userlogs/$jobid/$attemptid . Once that is done, we can simply write one ACL file per job in the userlogs/$jobid directory. This will avoid the duplication of information and can be done. Unless we are really very concerned about adding a new file per job after MAPREDUCE-927 .
          Hide
          Devaraj Das added a comment -

          3) As tasktracker doesn't have the job ACLs, when any one tries to access task logs of a job, I propose we store the job ACLs in a file say job-acls.xml) when task log files are created by taskTracker. And tasktracker will read this job-acls.xml when somebody tries to access task logs using web UI and does the authorization. I guess job-acls.xml can contain only the 2 config properties mapreduce.job.user.name and mapreduce.job.acl-view-job.

          Does it make sense to have the info in the log.info file that is already there in the task logs directory?

          Show
          Devaraj Das added a comment - 3) As tasktracker doesn't have the job ACLs, when any one tries to access task logs of a job, I propose we store the job ACLs in a file say job-acls.xml) when task log files are created by taskTracker. And tasktracker will read this job-acls.xml when somebody tries to access task logs using web UI and does the authorization. I guess job-acls.xml can contain only the 2 config properties mapreduce.job.user.name and mapreduce.job.acl-view-job. Does it make sense to have the info in the log.info file that is already there in the task logs directory?
          Hide
          Vinod Kumar Vavilapalli added a comment -

          One more thing is /logs, /static, /stack, /conf, /logLevel etc. are not going through authorization as part of this JIRA.

          Filed HADOOP-6568 for the same.

          Show
          Vinod Kumar Vavilapalli added a comment - One more thing is /logs, /static, /stack, /conf, /logLevel etc. are not going through authorization as part of this JIRA. Filed HADOOP-6568 for the same.
          Hide
          Ravi Gummadi added a comment -

          One more thing is /logs, /static, /stack, /conf, /logLevel etc. are not going through authorization as part of this JIRA. It needs changes in common and will be addressed in a separate JIRA.

          Show
          Ravi Gummadi added a comment - One more thing is /logs, /static, /stack, /conf, /logLevel etc. are not going through authorization as part of this JIRA. It needs changes in common and will be addressed in a separate JIRA.
          Hide
          Ravi Gummadi added a comment -

          (1) As history files don't have job ACLs stored along with them, accessing history related web pages will not be protected as part of this JIRA. That can be done as an improvement to this JIRA later.

          (2) This JIRA focuses on authorization of users against viewing/modifying jobs only. So no authorization for web pages that have info about queues, machines.

          (3) As tasktracker doesn't have the job ACLs, when any one tries to access task logs of a job, I propose we store the job ACLs in a file say job-acls.xml) when task log files are created by taskTracker. And tasktracker will read this job-acls.xml when somebody tries to access task logs using web UI and does the authorization. I guess job-acls.xml can contain only the 2 config properties mapreduce.job.user.name and mapreduce.job.acl-view-job.

          (4) Similar to the supergroup existing in jobtracker now, we would need supergroup(same config property) to be set on taskTracker also. This is to allow members of supergroup to access task logs. I will deprecate the earlier jobtracker config property and add one at cluster level.

          Thoughts ?

          Show
          Ravi Gummadi added a comment - (1) As history files don't have job ACLs stored along with them, accessing history related web pages will not be protected as part of this JIRA. That can be done as an improvement to this JIRA later. (2) This JIRA focuses on authorization of users against viewing/modifying jobs only. So no authorization for web pages that have info about queues, machines. (3) As tasktracker doesn't have the job ACLs, when any one tries to access task logs of a job, I propose we store the job ACLs in a file say job-acls.xml) when task log files are created by taskTracker. And tasktracker will read this job-acls.xml when somebody tries to access task logs using web UI and does the authorization. I guess job-acls.xml can contain only the 2 config properties mapreduce.job.user.name and mapreduce.job.acl-view-job. (4) Similar to the supergroup existing in jobtracker now, we would need supergroup(same config property) to be set on taskTracker also. This is to allow members of supergroup to access task logs. I will deprecate the earlier jobtracker config property and add one at cluster level. Thoughts ?
          Hide
          Devaraj Das added a comment -

          +1 to adding the doAs in the servlets

          Show
          Devaraj Das added a comment - +1 to adding the doAs in the servlets
          Hide
          Ravi Gummadi added a comment -

          We will get authenticated user using HttpServletRequest.getRemoteUser().
          I am proposing to run the methods that access the job as the user(using UserGroupInformation.doAs()) from JSPs and servlets sothat methods of JobTracker can just do authorization(by checking the UserGroupInformation.getCurrentUser()).
          This avoids many changes in MAPREDUCE-1307 and also avoids adding new methods that take UGI as parameter in jobtracker.

          Thoughts ?

          Show
          Ravi Gummadi added a comment - We will get authenticated user using HttpServletRequest.getRemoteUser(). I am proposing to run the methods that access the job as the user(using UserGroupInformation.doAs()) from JSPs and servlets sothat methods of JobTracker can just do authorization(by checking the UserGroupInformation.getCurrentUser()). This avoids many changes in MAPREDUCE-1307 and also avoids adding new methods that take UGI as parameter in jobtracker. Thoughts ?

            People

            • Assignee:
              Ravi Gummadi
              Reporter:
              Devaraj Das
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development