Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.0.3
    • Fix Version/s: 1.2.0
    • Component/s: mrv1
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      The class that does view-based checks, JSPUtil.JobWithViewAccessCheck, has the following internal member:

      private boolean isViewAllowed = true;

      Note that its true.

      Now, in the method that sets proper view-allowed rights, has:

      if (user != null && job != null && jt.areACLsEnabled()) {
            final UserGroupInformation ugi =
              UserGroupInformation.createRemoteUser(user);
            try {
              ugi.doAs(new PrivilegedExceptionAction<Void>() {
                public Void run() throws IOException, ServletException {
      
                  // checks job view permission
                  jt.getACLsManager().checkAccess(job, ugi,
                      Operation.VIEW_JOB_DETAILS);
                  return null;
                }
              });
            } catch (AccessControlException e) {
              String errMsg = "User " + ugi.getShortUserName() +
                  " failed to view " + jobid + "!<br><br>" + e.getMessage() +
                  "<hr><a href=\"jobtracker.jsp\">Go back to JobTracker</a><br>";
              JSPUtil.setErrorAndForward(errMsg, request, response);
              myJob.setViewAccess(false);
            } catch (InterruptedException e) {
              String errMsg = " Interrupted while trying to access " + jobid +
              "<hr><a href=\"jobtracker.jsp\">Go back to JobTracker</a><br>";
              JSPUtil.setErrorAndForward(errMsg, request, response);
              myJob.setViewAccess(false);
            }
          }
          return myJob;
      

      In the above snippet, you can notice that if user==null, which can happen if user is not http-authenticated (as its got via request.getRemoteUser()), can lead to the view being visible since the default is true and we didn't toggle the view to false for user == null case.

      Ideally the default of the view job ACL must be false, or we need an else clause that sets the view rights to false in case of a failure to find the user ID.

      1. MR-4317.patch
        6 kB
        Karthik Kambatla
      2. MR-4317.patch
        6 kB
        Karthik Kambatla

        Activity

        Hide
        Matt Foley added a comment -

        Closed upon release of Hadoop 1.2.0.

        Show
        Matt Foley added a comment - Closed upon release of Hadoop 1.2.0.
        Hide
        Alejandro Abdelnur added a comment -

        Thanks Karthik. Committed to branch-1.

        Show
        Alejandro Abdelnur added a comment - Thanks Karthik. Committed to branch-1.
        Hide
        Alejandro Abdelnur added a comment -

        +1. Thanks Karthik for following this offline with me and digging that making the changes I was suggesting would require changes in JSPs and thanks for verifying that if a null job is given to the JobWithViewAccessCheck constructor things still work as expected.

        Show
        Alejandro Abdelnur added a comment - +1. Thanks Karthik for following this offline with me and digging that making the changes I was suggesting would require changes in JSPs and thanks for verifying that if a null job is given to the JobWithViewAccessCheck constructor things still work as expected.
        Hide
        Karthik Kambatla added a comment -

        Alejandro,

        The API (Javadoc below) mentions that the job will be null, if there doesn't exist a job with that JobID. The old API also has the same functionality.

          /**
           * Validates if current user can view the job.
           * If user is not authorized to view the job, this method will modify the
           * response and forwards to an error page and returns Job with
           * viewJobAccess flag set to false.
           * @return JobWithViewAccessCheck object(contains JobInProgress object and
           *         viewJobAccess flag). Callers of this method will check the flag
           *         and decide if view should be allowed or not. Job will be null if
           *         the job with given jobid doesnot exist at the JobTracker.
           */
        
        Show
        Karthik Kambatla added a comment - Alejandro, The API (Javadoc below) mentions that the job will be null, if there doesn't exist a job with that JobID. The old API also has the same functionality. /** * Validates if current user can view the job. * If user is not authorized to view the job, this method will modify the * response and forwards to an error page and returns Job with * viewJobAccess flag set to false . * @ return JobWithViewAccessCheck object(contains JobInProgress object and * viewJobAccess flag). Callers of this method will check the flag * and decide if view should be allowed or not. Job will be null if * the job with given jobid doesnot exist at the JobTracker. */
        Hide
        Alejandro Abdelnur added a comment -

        Karthik,

        Why 'job ==null' ?

        +    if (!jt.areACLsEnabled() || job == null) {
        +      return myJob;
        +    }
        

        If job == null then myJob is also null (or even the call may fail)

        Shouldn't we check for job == null before trying to the myJob?

        Show
        Alejandro Abdelnur added a comment - Karthik, Why 'job ==null' ? + if (!jt.areACLsEnabled() || job == null ) { + return myJob; + } If job == null then myJob is also null (or even the call may fail) Shouldn't we check for job == null before trying to the myJob?
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12533419/MR-4317.patch
        against trunk revision .

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

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

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

        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2510//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/12533419/MR-4317.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 1 new or modified test files. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2510//console This message is automatically generated.
        Hide
        Karthik Kambatla added a comment -

        Updated the patch as per Alejandro's suggestions:

        1. Corrected test for unauthenticated user.
        2. Removed the check for user.equals("null").

        Sorry for not realizing this earlier.

        Thanks.

        Show
        Karthik Kambatla added a comment - Updated the patch as per Alejandro's suggestions: 1. Corrected test for unauthenticated user. 2. Removed the check for user.equals("null"). Sorry for not realizing this earlier. Thanks.
        Hide
        Karthik Kambatla added a comment -

        Makes sense. Didn't realize it. I shall modify the test to not include a userName and see if I can omit the check for user.equals("null"). Thanks for the help.

        Show
        Karthik Kambatla added a comment - Makes sense. Didn't realize it. I shall modify the test to not include a userName and see if I can omit the check for user.equals("null"). Thanks for the help.
        Hide
        Alejandro Abdelnur added a comment -

        well, IMO, that test seems wrong. if you want to have an unauthenticated user then don't include '&user.name=...'.

        Show
        Alejandro Abdelnur added a comment - well, IMO, that test seems wrong. if you want to have an unauthenticated user then don't include '&user.name=...'.
        Hide
        Karthik Kambatla added a comment -

        In the following code snippet from TestWebUIAuthorization, new URL() takes in userName. When the userName is null, the URL sees it as "null". In checkAccessAndGetJob, we read the username from this string and get "null" and not null.

        The test fails when I check only for user == null.

          static int getHttpStatusCode(String urlstring, String userName,
              String method) throws IOException {
            LOG.info("Accessing " + urlstring + " as user " + userName);
            URL url = new URL(urlstring + "&user.name=" + userName);
            HttpURLConnection connection = (HttpURLConnection)url.openConnection();
            connection.setRequestMethod(method);
            if (method.equals("POST")) {
              String encodedData = "action=kill&user.name=" + userName;      
              connection.setRequestProperty("Content-Type",
                                            "application/x-www-form-urlencoded");
              connection.setRequestProperty("Content-Length",
                                            Integer.toString(encodedData.length()));
              connection.setDoOutput(true);
        
              OutputStream os = connection.getOutputStream();
              os.write(encodedData.getBytes());
            }
            connection.connect();
        
            return connection.getResponseCode();
          }
        
        
        Show
        Karthik Kambatla added a comment - In the following code snippet from TestWebUIAuthorization, new URL() takes in userName. When the userName is null, the URL sees it as "null". In checkAccessAndGetJob, we read the username from this string and get "null" and not null. The test fails when I check only for user == null. static int getHttpStatusCode( String urlstring, String userName, String method) throws IOException { LOG.info( "Accessing " + urlstring + " as user " + userName); URL url = new URL(urlstring + "&user.name=" + userName); HttpURLConnection connection = (HttpURLConnection)url.openConnection(); connection.setRequestMethod(method); if (method.equals( "POST" )) { String encodedData = "action=kill&user.name=" + userName; connection.setRequestProperty( "Content-Type" , "application/x-www-form-urlencoded" ); connection.setRequestProperty( "Content-Length" , Integer .toString(encodedData.length())); connection.setDoOutput( true ); OutputStream os = connection.getOutputStream(); os.write(encodedData.getBytes()); } connection.connect(); return connection.getResponseCode(); }
        Hide
        Alejandro Abdelnur added a comment -

        what do you mean by the client mistakenly captures the user as "null" ?

        Show
        Alejandro Abdelnur added a comment - what do you mean by the client mistakenly captures the user as "null" ?
        Hide
        Karthik Kambatla added a comment -

        Let me explain in more detail:

        JSPUtil.checkAccessAndGetJob() takes HTTPServletRequest as input. HTTPServletRequest.getRemoteUser() returns null if the user is not authenticated. For this case, check for user == null should suffice.

        However, I have noticed while testing (TestWebUIAuthorization.validateTaskGraphServletAccess() in the patch) that when we build the HTTPServletRequest with user == null, the corresponding url captures the user as "null". For such cases, where the client mistakenly captures the user as "null", we need to check user.equals("null") as well.

        Do you think we should change the way client builds the HTTPServletRequest?

        Many thanks.

        Show
        Karthik Kambatla added a comment - Let me explain in more detail: JSPUtil.checkAccessAndGetJob() takes HTTPServletRequest as input. HTTPServletRequest.getRemoteUser() returns null if the user is not authenticated. For this case, check for user == null should suffice. However, I have noticed while testing (TestWebUIAuthorization.validateTaskGraphServletAccess() in the patch) that when we build the HTTPServletRequest with user == null, the corresponding url captures the user as "null". For such cases, where the client mistakenly captures the user as "null", we need to check user.equals("null") as well. Do you think we should change the way client builds the HTTPServletRequest? Many thanks.
        Hide
        Alejandro Abdelnur added a comment -

        but the check for user 'null' was not there, why are you introducing it? if the user is undefined it will be null, when do you expect the user to be the string 'null'. Checking for user == null should be enough, no?

        Show
        Alejandro Abdelnur added a comment - but the check for user ' null ' was not there, why are you introducing it? if the user is undefined it will be null, when do you expect the user to be the string ' null '. Checking for user == null should be enough, no?
        Hide
        Karthik Kambatla added a comment -

        Alejandro,

        The user string is read from the HTTPRequest. Both when the user == null and user == "null", the request reads it as "null". Hence, the check for both. And, I agree the user named "null" is forcibly not allowed.

        Show
        Karthik Kambatla added a comment - Alejandro, The user string is read from the HTTPRequest. Both when the user == null and user == "null", the request reads it as "null". Hence, the check for both. And, I agree the user named "null" is forcibly not allowed.
        Hide
        Alejandro Abdelnur added a comment - - edited

        Karthik, why the check if (user == null || user.equals("null")) {, checking for user == null should be enough, no? Else you a voiding the user named 'null'

        Show
        Alejandro Abdelnur added a comment - - edited Karthik, why the check if (user == null || user.equals("null")) { , checking for user == null should be enough, no? Else you a voiding the user named ' null '
        Hide
        Karthik Kambatla added a comment -

        The corresponding code in MR2 (majorly refactored) - hadoop-mapreduce-client/ AMWebServices.hasAccess(job, request) - is not as permissive. This issue doesn't pertain to MR2.

        Show
        Karthik Kambatla added a comment - The corresponding code in MR2 (majorly refactored) - hadoop-mapreduce-client/ AMWebServices.hasAccess(job, request) - is not as permissive. This issue doesn't pertain to MR2.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12533376/MR-4317.patch
        against trunk revision .

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

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

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

        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2509//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/12533376/MR-4317.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 1 new or modified test files. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2509//console This message is automatically generated.
        Hide
        Karthik Kambatla added a comment -

        Cleaned the patch up to remove unused imports. Ready.

        Show
        Karthik Kambatla added a comment - Cleaned the patch up to remove unused imports. Ready.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12531738/MR-4317.patch
        against trunk revision .

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

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

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

        Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2453//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/12531738/MR-4317.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 1 new or modified test files. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2453//console This message is automatically generated.
        Hide
        Karthik Kambatla added a comment -

        Uploading a patch that checks access of a job to user in steps:
        1. ACLs enabled/disabled
        2. Job exists or not
        3. User is valid - not null
        4. ACL check to see if the user is allowed to access

        Testing:

        Unit test - TestWebUIAuthorization

        • added a test for user == null
        Show
        Karthik Kambatla added a comment - Uploading a patch that checks access of a job to user in steps: 1. ACLs enabled/disabled 2. Job exists or not 3. User is valid - not null 4. ACL check to see if the user is allowed to access Testing: Unit test - TestWebUIAuthorization added a test for user == null
        Hide
        Karthik Kambatla added a comment -

        Added v1 patch for this - the changes are fairly small and straight-forward.

        1. I didn't see any tests checking TaskGraphServlet.
        2. Do we need to add a test to verify this behavior? If so, can someone please point me to similar existing tests.

        Show
        Karthik Kambatla added a comment - Added v1 patch for this - the changes are fairly small and straight-forward. 1. I didn't see any tests checking TaskGraphServlet. 2. Do we need to add a test to verify this behavior? If so, can someone please point me to similar existing tests.

          People

          • Assignee:
            Karthik Kambatla
            Reporter:
            Harsh J
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development