Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: jobtracker, security
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      Added web-authorization for job-history pages. This is an incompatible change - it changes the JobHistory format by adding job-acls to job-history files and JobHistory currently does not have the support to read older versions of history files.

      Description

      MAPREDUCE-1455 introduces authorization for most of the Map/Reduce jsp pages and servlets, but left history pages. This JIRA will make sure that authorization checks are made while accessing job-history pages also.

      1. MAPREDUCE-1493-20100222.1.txt
        51 kB
        Vinod Kumar Vavilapalli
      2. MAPREDUCE-1493-20100225.2.txt
        59 kB
        Vinod Kumar Vavilapalli
      3. MAPREDUCE-1493-20100226.1.txt
        59 kB
        Vinod Kumar Vavilapalli
      4. MAPREDUCE-1493-20100227.2-ydist.txt
        50 kB
        Ravi Gummadi
      5. MAPREDUCE-1493-20100227.3-ydist.txt
        51 kB
        Vinod Kumar Vavilapalli
      6. MAPREDUCE-1493-20100301.1.txt
        59 kB
        Vinod Kumar Vavilapalli
      7. MAPREDUCE-1493-20100304.1.txt
        60 kB
        Vinod Kumar Vavilapalli
      8. MAPREDUCE-1493-20100304.txt
        60 kB
        Vinod Kumar Vavilapalli

        Issue Links

          Activity

          Hide
          Vinod Kumar Vavilapalli added a comment -

          History pages are served by the JobTracker directly off the job-history files on the DFS. To facilitate the authorization of these pages, we will need the job ACLs that are introduced by MAPREDUCE-1307 and used subsequently in MAPREDUCE-1455. Along with JobHistory files, job configuration files are also stored on DFS, so we can directly read the ACLs from conf files and use them for the sake of authorization.

          Show
          Vinod Kumar Vavilapalli added a comment - History pages are served by the JobTracker directly off the job-history files on the DFS. To facilitate the authorization of these pages, we will need the job ACLs that are introduced by MAPREDUCE-1307 and used subsequently in MAPREDUCE-1455 . Along with JobHistory files, job configuration files are also stored on DFS, so we can directly read the ACLs from conf files and use them for the sake of authorization.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          For getting the ACLs for retired jobs, we actually have two options:

          • Log the ACLs also into job-history itself during job-submission and retrieve the same for access control.
            • This avoids reading job-conf file for every request of a job specific page.
            • Needs a change to JobHistory submisison event.
          • Read the job-conf file on history also to get the acls.
            • Introduces extra round trip to DFS
            • No change to JobHistory.

          I think the former is better performance wise. Thoughts?

          Show
          Vinod Kumar Vavilapalli added a comment - For getting the ACLs for retired jobs, we actually have two options: Log the ACLs also into job-history itself during job-submission and retrieve the same for access control. This avoids reading job-conf file for every request of a job specific page. Needs a change to JobHistory submisison event. Read the job-conf file on history also to get the acls. Introduces extra round trip to DFS No change to JobHistory. I think the former is better performance wise. Thoughts?
          Hide
          Devaraj Das added a comment -

          I think we anyway should log the job ACLs in the history file for the job. So in that sense, the first option seems to make more sense to me.

          Show
          Devaraj Das added a comment - I think we anyway should log the job ACLs in the history file for the job. So in that sense, the first option seems to make more sense to me.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Regarding putting the ACLs in JobHistory I've checked with Jothi Padmanabhan to see how easy/difficult it is to put it in there. From what I understand from that conversation, it is non-trivial to do it and will involve changes in our JobHistory APIs and possibly in other places like Vaidya, Rumen which consume JobHistory. Further, he was of the opinion that ACLs are simply a configuration and may not exactly fit into the history file, though we agreed the line is really blurred as to what should go into the history file and what should not.

          Given that, I am leaning towards going with the conf file itself where the ACLs are already present.

          Show
          Vinod Kumar Vavilapalli added a comment - Regarding putting the ACLs in JobHistory I've checked with Jothi Padmanabhan to see how easy/difficult it is to put it in there. From what I understand from that conversation, it is non-trivial to do it and will involve changes in our JobHistory APIs and possibly in other places like Vaidya, Rumen which consume JobHistory. Further, he was of the opinion that ACLs are simply a configuration and may not exactly fit into the history file, though we agreed the line is really blurred as to what should go into the history file and what should not. Given that, I am leaning towards going with the conf file itself where the ACLs are already present.
          Hide
          Devaraj Das added a comment -

          I think it makes more sense to do only the JobHistory lookup for the ACL (there should be a standard place in the history file where the job information is logged), and avoid looking up the jobconf..

          Show
          Devaraj Das added a comment - I think it makes more sense to do only the JobHistory lookup for the ACL (there should be a standard place in the history file where the job information is logged), and avoid looking up the jobconf..
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Attaching patch for authorization of JobHistory web-pages. As per Ddas, the acls are put in the history file itself. The patch is to be applied on top of MAPREDUCE-1455 patch, and hence is not very fine, but reviewable.

          Show
          Vinod Kumar Vavilapalli added a comment - Attaching patch for authorization of JobHistory web-pages. As per Ddas, the acls are put in the history file itself. The patch is to be applied on top of MAPREDUCE-1455 patch, and hence is not very fine, but reviewable.
          Hide
          Ravi Gummadi added a comment -

          Didn't review the whole patch. Patch looks good functionally, when I tested this patch on web UI. Some comments:

          (1) Though it doesn't seem to be a security hole, taskstatshistory.jsp can build taskid from attemptid instead of taking both as parameters.
          (2) Similar to jobhistory.jsp, jobtracker.jsp's Retired Jobs need to take only logFile as parameter and need not take jobId as parameter.
          (3) All jsps modified in MAPREDUCE-1455 have parameter names as "tipid" and "taskid" to refer to tip and attempt. But in history related jsps, name "taskid" is used to refer to tip sometimes and attempt in some other places. We could follow the same names as all of jsps of MAPREDUCE-1455 are following. For eg, links of task logs and task counters in taskdetailshistory.jsp.
          (4) jobdetailshistory.jsp can display the job ACLs similar to jobdetails.jsp.
          (5) Irrespective of this patch, search on the jobhistory page seem to be taking only till underscore(excluding underscore) in the username. For eg, if I search for user name "ravi_tmp", it gives No Jobs even though I launched jobs as ravi_tmp. It gives job
          s of ravi_tmp if I give username as ravi. Is this a known issue ?

          Show
          Ravi Gummadi added a comment - Didn't review the whole patch. Patch looks good functionally, when I tested this patch on web UI. Some comments: (1) Though it doesn't seem to be a security hole, taskstatshistory.jsp can build taskid from attemptid instead of taking both as parameters. (2) Similar to jobhistory.jsp, jobtracker.jsp's Retired Jobs need to take only logFile as parameter and need not take jobId as parameter. (3) All jsps modified in MAPREDUCE-1455 have parameter names as "tipid" and "taskid" to refer to tip and attempt. But in history related jsps, name "taskid" is used to refer to tip sometimes and attempt in some other places. We could follow the same names as all of jsps of MAPREDUCE-1455 are following. For eg, links of task logs and task counters in taskdetailshistory.jsp. (4) jobdetailshistory.jsp can display the job ACLs similar to jobdetails.jsp. (5) Irrespective of this patch, search on the jobhistory page seem to be taking only till underscore(excluding underscore) in the username. For eg, if I search for user name "ravi_tmp", it gives No Jobs even though I launched jobs as ravi_tmp. It gives job s of ravi_tmp if I give username as ravi. Is this a known issue ?
          Hide
          Ravi Gummadi added a comment -

          Some more comments:

          (6) JobACLs.java In comment, change "specify" to "specified". Also it says 'superuser/supergroup of the "jobTracker"' — superuser/supergroup will not be specific to JT after 1455 and it is cluster level.
          (7) Error messages displayed when authorization fails are not having the job-view-ACLs configured for the job. Please add the message obtained from Acces sControlException in the error message.
          (8) In TestWebUIAuthorization.java, the name of existing method testWebUIAuthorization() is changed to TestWebUIAuthorization(). May be you did it for your testing sothat the method won't run. Please change the name back. A cooment refers to "jobdetails.jsp" — should instead be "jobdetailshistory.jsp". props.setProperty(JSPUtil.PRIVATE_ACTIONS_KEY, "true"); is not needed for testing history related stuff.
          (9) Don't we want to support viewing of older history files ? With older history files(where job ACLs are not there), JSPUtil.getJobInfo() gets NPE because JobSubmittedEvent.getJobACLs() gets NPE. Should we allow viewing of older history files assuming that view access exists for those jobs for all users ?
          (10) taskstatshistory.jsp, taskdetailshistory.jsp and TaskLogServlet can be validated in the testcase.
          (11) In rumen, in Job20LineHistoryEventEmitter.java, from the parsed line of history file, jobACLs are not read but empty ACLs are written to. I guess we need to build ACLs from the parsedLine of history.
          (12) In all history related jsps, unnecessary import of "org.apache.hadoop.security.UserGroupInformation" can be removed. Also unnecesary line "String user = request.getRemoteUser();" can be removed.
          (13) Very minor: Indentation of line "JSPUtil.checkAccess...." in jobdetailshistory.jsp
          (14) We wanted to remove the variable "conf"(which is doing just new Configuration()) from JSPUtil.java in this patch. Can you do it here ? We can get the CACHE_SIZE using jobTracker.conf.get(JT_JOBHISTORY_CACHE_SIZE) in getJobInfo() ?
          (15) Am not sure if passing of rumen tests is good enough for this patch. Do we need any additional testing ?

          Show
          Ravi Gummadi added a comment - Some more comments: (6) JobACLs.java In comment, change "specify" to "specified". Also it says 'superuser/supergroup of the "jobTracker"' — superuser/supergroup will not be specific to JT after 1455 and it is cluster level. (7) Error messages displayed when authorization fails are not having the job-view-ACLs configured for the job. Please add the message obtained from Acces sControlException in the error message. (8) In TestWebUIAuthorization.java, the name of existing method testWebUIAuthorization() is changed to TestWebUIAuthorization(). May be you did it for your testing sothat the method won't run. Please change the name back. A cooment refers to "jobdetails.jsp" — should instead be "jobdetailshistory.jsp". props.setProperty(JSPUtil.PRIVATE_ACTIONS_KEY, "true"); is not needed for testing history related stuff. (9) Don't we want to support viewing of older history files ? With older history files(where job ACLs are not there), JSPUtil.getJobInfo() gets NPE because JobSubmittedEvent.getJobACLs() gets NPE. Should we allow viewing of older history files assuming that view access exists for those jobs for all users ? (10) taskstatshistory.jsp, taskdetailshistory.jsp and TaskLogServlet can be validated in the testcase. (11) In rumen, in Job20LineHistoryEventEmitter.java, from the parsed line of history file, jobACLs are not read but empty ACLs are written to. I guess we need to build ACLs from the parsedLine of history. (12) In all history related jsps, unnecessary import of "org.apache.hadoop.security.UserGroupInformation" can be removed. Also unnecesary line "String user = request.getRemoteUser();" can be removed. (13) Very minor: Indentation of line "JSPUtil.checkAccess...." in jobdetailshistory.jsp (14) We wanted to remove the variable "conf"(which is doing just new Configuration()) from JSPUtil.java in this patch. Can you do it here ? We can get the CACHE_SIZE using jobTracker.conf.get(JT_JOBHISTORY_CACHE_SIZE) in getJobInfo() ? (15) Am not sure if passing of rumen tests is good enough for this patch. Do we need any additional testing ?
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Updated patch addressing the above comments except:

          (9) Don't we want to support viewing of older history files ? With older history files(where job ACLs are not there), JSPUtil.getJobInfo() gets NPE because JobSubmittedEvent.getJobACLs() gets NPE. Should we allow viewing of older history files assuming that view access exists for those jobs for all users ?

          JobHistory currently DOES NOT have the support to read older versions of history files. So this cannot be done in this issue and hence makes this an incompatible change.

          (11) In rumen, in Job20LineHistoryEventEmitter.java, from the parsed line of history file, jobACLs are not read but empty ACLs are written to. I guess we need to build ACLs from the parsedLine of history.

          There are no ACLs in the 20 format of JobHistory, so we don't need to do anything here. Changes percolated into this class as I've broken the compatibility of JobSubmissionEvent class constructor. Not very sure if we need backward-compatibility of this class as it may be internal only.

          (15) Am not sure if passing of rumen tests is good enough for this patch. Do we need any additional testing ?

          It's on Rumen how to use the logged ACLs. 'Cause this patch is not adding any new code for Rumen w.r.t JobACLs, we don't need more tests in this regard.

          Show
          Vinod Kumar Vavilapalli added a comment - Updated patch addressing the above comments except: (9) Don't we want to support viewing of older history files ? With older history files(where job ACLs are not there), JSPUtil.getJobInfo() gets NPE because JobSubmittedEvent.getJobACLs() gets NPE. Should we allow viewing of older history files assuming that view access exists for those jobs for all users ? JobHistory currently DOES NOT have the support to read older versions of history files. So this cannot be done in this issue and hence makes this an incompatible change. (11) In rumen, in Job20LineHistoryEventEmitter.java, from the parsed line of history file, jobACLs are not read but empty ACLs are written to. I guess we need to build ACLs from the parsedLine of history. There are no ACLs in the 20 format of JobHistory, so we don't need to do anything here. Changes percolated into this class as I've broken the compatibility of JobSubmissionEvent class constructor. Not very sure if we need backward-compatibility of this class as it may be internal only. (15) Am not sure if passing of rumen tests is good enough for this patch. Do we need any additional testing ? It's on Rumen how to use the logged ACLs. 'Cause this patch is not adding any new code for Rumen w.r.t JobACLs, we don't need more tests in this regard.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Updated patch sync'ing with the latest one at MAPREDUCE-1455.

          Show
          Vinod Kumar Vavilapalli added a comment - Updated patch sync'ing with the latest one at MAPREDUCE-1455 .
          Hide
          Ravi Gummadi added a comment -

          Patch looks good.
          +1

          Show
          Ravi Gummadi added a comment - Patch looks good. +1
          Hide
          Ravi Gummadi added a comment -

          Hudson can apply this patch only after MAPREDUCE-1455 gets committed. So cancelling submission of this patch to Hudson.

          Show
          Ravi Gummadi added a comment - Hudson can apply this patch only after MAPREDUCE-1455 gets committed. So cancelling submission of this patch to Hudson.
          Hide
          Ravi Gummadi added a comment -

          Attaching patch for earlier version on behalf of vinod. Not for commit here.

          Show
          Ravi Gummadi added a comment - Attaching patch for earlier version on behalf of vinod. Not for commit here.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Updated patch for previous versions. Not for commit here.

          Show
          Vinod Kumar Vavilapalli added a comment - Updated patch for previous versions. Not for commit here.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Update sync'ing with the latest patch at MAPREDUCE-1455.

          Show
          Vinod Kumar Vavilapalli added a comment - Update sync'ing with the latest patch at MAPREDUCE-1455 .
          Hide
          Ravi Gummadi added a comment -

          Latest patch looks fine.
          +1

          Show
          Ravi Gummadi added a comment - Latest patch looks fine. +1
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12437473/MAPREDUCE-1493-20100301.1.txt
          against trunk revision 916823.

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

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

          -1 patch. The patch command could not apply the patch.

          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/494/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/12437473/MAPREDUCE-1493-20100301.1.txt against trunk revision 916823. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/494/console This message is automatically generated.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          MAPREDUCE-1455 is in. Running this patch through Hudson.

          Show
          Vinod Kumar Vavilapalli added a comment - MAPREDUCE-1455 is in. Running this patch through Hudson.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Hudson's MAPREDUCE queue has gone bonkers again.

          In the meanwhile, MAPREDUCE-1454 broke this patch. Attaching updated patch.

          Show
          Vinod Kumar Vavilapalli added a comment - Hudson's MAPREDUCE queue has gone bonkers again. In the meanwhile, MAPREDUCE-1454 broke this patch. Attaching updated patch.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12437853/MAPREDUCE-1493-20100304.txt
          against trunk revision 919277.

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

          +1 tests included. The patch appears to include 9 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 passed 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/502/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/502/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/502/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/502/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/12437853/MAPREDUCE-1493-20100304.txt against trunk revision 919277. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 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 passed 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/502/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/502/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/502/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/502/console This message is automatically generated.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Updated patch fixing a bug in TaskLogServlet.

          Show
          Vinod Kumar Vavilapalli added a comment - Updated patch fixing a bug in TaskLogServlet.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12438161/MAPREDUCE-1493-20100304.1.txt
          against trunk revision 919645.

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

          +1 tests included. The patch appears to include 9 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 passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/505/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/505/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/505/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/505/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/12438161/MAPREDUCE-1493-20100304.1.txt against trunk revision 919645. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 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 passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/505/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/505/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/505/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/505/console This message is automatically generated.
          Hide
          Ravi Gummadi added a comment -

          Patch looks good.
          +1

          Show
          Ravi Gummadi added a comment - Patch looks good. +1
          Hide
          Vinod Kumar Vavilapalli added a comment -

          I've just committed this.

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

          Integrated in Hadoop-Mapreduce-trunk-Commit #267 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/267/)
          . Authorization for job-history pages. Contributed by Vinod Kumar Vavilapalli.

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

            People

            • Assignee:
              Vinod Kumar Vavilapalli
              Reporter:
              Vinod Kumar Vavilapalli
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development