Hadoop Common
  1. Hadoop Common
  2. HADOOP-8726

The Secrets in Credentials are not available to MR tasks

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.23.3, 2.0.0-alpha, 3.0.0
    • Fix Version/s: 0.23.3, 2.0.2-alpha
    • Component/s: security
    • Labels:
      None

      Description

      Though secrets are passed in Credentials, the secrets are not available to the MR tasks.

      This issue exists with security on/off.

      This is related to the change in HADOOP-8225

      1. HADOOP-8726.patch
        2 kB
        Benoy Antony
      2. HADOOP-8726.patch
        11 kB
        Benoy Antony
      3. HADOOP-8726.patch
        13 kB
        Daryn Sharp
      4. HADOOP-8726.patch
        13 kB
        Daryn Sharp

        Issue Links

          Activity

          Hide
          Benoy Antony added a comment -

          Attaching an initial patch to unblock MAPREDUCE-4554

          Show
          Benoy Antony added a comment - Attaching an initial patch to unblock MAPREDUCE-4554
          Hide
          Daryn Sharp added a comment -

          This doesn't work. Tokens can be added directly to the UGI and this patch won't return them. A new UGI wrapping the Subject also won't "see" its tokens. The Credentials needs to be constructed from the Subject.

          I think the correct approach is for UGI#getCredentials to also extract the secrets from the Subject. While working on HADOOP-8225 I noticed the secrets aren't propagate, but I didn't address the issue since I wasn't sure what impact it might have.

          Show
          Daryn Sharp added a comment - This doesn't work. Tokens can be added directly to the UGI and this patch won't return them. A new UGI wrapping the Subject also won't "see" its tokens. The Credentials needs to be constructed from the Subject . I think the correct approach is for UGI#getCredentials to also extract the secrets from the Subject . While working on HADOOP-8225 I noticed the secrets aren't propagate, but I didn't address the issue since I wasn't sure what impact it might have.
          Hide
          Benoy Antony added a comment -

          I agree, Daryn.
          I can handle secrets similar to tokens using the NamedToken. But I need to differentiate between token and secret.
          I can add a enum in the NamedToken to indicate if its token or secret. Is that fine ?

          Show
          Benoy Antony added a comment - I agree, Daryn. I can handle secrets similar to tokens using the NamedToken. But I need to differentiate between token and secret. I can add a enum in the NamedToken to indicate if its token or secret. Is that fine ?
          Hide
          Benoy Antony added a comment -

          Another approach will be to delegate token and secret management to Credentials itself and keep the entire Credentials object in Subject as PrivateCredentials.

          This will avoid UGI class from handling the internals of Credentials. Any drawbacks ?

          Show
          Benoy Antony added a comment - Another approach will be to delegate token and secret management to Credentials itself and keep the entire Credentials object in Subject as PrivateCredentials. This will avoid UGI class from handling the internals of Credentials. Any drawbacks ?
          Hide
          Benoy Antony added a comment -

          Attaching a new patch.

          TOkens and Secrets are maintained by Credentials.
          Credentials object is stored in Subject.privateCredentials.

          Benefits of this approach incluse

          1) The UGI class is independent of internal of Credentials class.

          2) Tokens and Secrets are handled and the handling is delegated to Credentials class.

          3) Relatively better performance as the UGI#getCredentials () and UGI#addToken functions do iterate the full collection

          Show
          Benoy Antony added a comment - Attaching a new patch. TOkens and Secrets are maintained by Credentials. Credentials object is stored in Subject.privateCredentials. Benefits of this approach incluse 1) The UGI class is independent of internal of Credentials class. 2) Tokens and Secrets are handled and the handling is delegated to Credentials class. 3) Relatively better performance as the UGI#getCredentials () and UGI#addToken functions do iterate the full collection
          Hide
          Daryn Sharp added a comment -

          On cursory inspection, it looks good, but the patch doesn't apply to trunk. Please update and I'll review.

          Show
          Daryn Sharp added a comment - On cursory inspection, it looks good, but the patch doesn't apply to trunk. Please update and I'll review.
          Hide
          Daryn Sharp added a comment -

          Comments:

          1. UGI.getLoginUser should call UGI.addCredentials instead of duplicating the logic of that method.
          2. getCredentials should return a copy of the Credentials. See more below.
          3. A few nits on the style and consistency. Should add a space around one of the "!=", and use a cuddled "if".

          Regarding getCredentials: The existing getTokens is returning an immutable list prevent the caller from directly tampering with the UGI's tokens. getCredentials must also return a copy to avoid leaking tokens into the wrong UGI. Ex. Let's say I want to get the tokens from one UGI, add more tokens, and then use those with a new UGI. With the change in this patch, the original UGI will now also contain the new tokens only intended for the new/other UGI. You'll probably need to add a private getCredentialsInternal that fetches from the Subject, and the public getCredentials returns new Credentials(getCredentialsInternal()).

          Show
          Daryn Sharp added a comment - Comments: UGI.getLoginUser should call UGI.addCredentials instead of duplicating the logic of that method. getCredentials should return a copy of the Credentials . See more below. A few nits on the style and consistency. Should add a space around one of the "!=", and use a cuddled "if". Regarding getCredentials : The existing getTokens is returning an immutable list prevent the caller from directly tampering with the UGI's tokens. getCredentials must also return a copy to avoid leaking tokens into the wrong UGI. Ex. Let's say I want to get the tokens from one UGI, add more tokens, and then use those with a new UGI. With the change in this patch, the original UGI will now also contain the new tokens only intended for the new/other UGI. You'll probably need to add a private getCredentialsInternal that fetches from the Subject , and the public getCredentials returns new Credentials(getCredentialsInternal()) .
          Hide
          Daryn Sharp added a comment -

          I'll take a stab at a patch since I think we really need this for 23. I'll write some test, but it would be great if you can test with specific use case.

          Show
          Daryn Sharp added a comment - I'll take a stab at a patch since I think we really need this for 23. I'll write some test, but it would be great if you can test with specific use case.
          Hide
          Benoy Antony added a comment -

          Thanks for the review, Daryn. I'll take care of your recommendations.
          I have a question while going through the changes in HADOOP-8225 and HADOOP-8276. Would it make sense to change UGI#addToken and UGI#getTokens back to their original state ?
          This will cleanly separate the Credentials and Tokens stored on the UGI while achieving the original intent of loading the tokens as part of getLoginUser() call.

          I'll post a patch soon.

          Show
          Benoy Antony added a comment - Thanks for the review, Daryn. I'll take care of your recommendations. I have a question while going through the changes in HADOOP-8225 and HADOOP-8276 . Would it make sense to change UGI#addToken and UGI#getTokens back to their original state ? This will cleanly separate the Credentials and Tokens stored on the UGI while achieving the original intent of loading the tokens as part of getLoginUser() call. I'll post a patch soon.
          Hide
          Daryn Sharp added a comment -

          The overall goal is to freely convert between UGI and Credentials. Per my earlier comment, I was going to upload a patch when the test results come back.

          Show
          Daryn Sharp added a comment - The overall goal is to freely convert between UGI and Credentials . Per my earlier comment, I was going to upload a patch when the test results come back.
          Hide
          Daryn Sharp added a comment -

          A simpler impl of Benoy's approach of using a Credentials within the UGI's Subject to help ensure the secrets propagate. Also ensures the returned creds are not the ones internal to Subject.

          Show
          Daryn Sharp added a comment - A simpler impl of Benoy's approach of using a Credentials within the UGI 's Subject to help ensure the secrets propagate. Also ensures the returned creds are not the ones internal to Subject .
          Hide
          Benoy Antony added a comment -

          Looks good to me. One comment:
          UGI#getTokens can be
          return Collections.unmodifiableCollection(
          getCredentialsInternal().getAllTokens());

          Show
          Benoy Antony added a comment - Looks good to me. One comment: UGI#getTokens can be return Collections.unmodifiableCollection( getCredentialsInternal().getAllTokens());
          Hide
          Benoy Antony added a comment -

          Ran test to make sure that this patch unblocks MAPREDUCE-4554.

          Show
          Benoy Antony added a comment - Ran test to make sure that this patch unblocks MAPREDUCE-4554 .
          Hide
          Daryn Sharp added a comment -

          Add Benoy's suggestion to return immutable collection.

          Show
          Daryn Sharp added a comment - Add Benoy's suggestion to return immutable collection.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12542928/HADOOP-8726.patch
          against trunk revision .

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

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

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

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

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

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

          +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core.

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1380//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1380//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/12542928/HADOOP-8726.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/1380//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/1380//console This message is automatically generated.
          Hide
          Benoy Antony added a comment -

          Reviewed the latest patch. Looks good
          +1

          Show
          Benoy Antony added a comment - Reviewed the latest patch. Looks good +1
          Hide
          Robert Joseph Evans added a comment -

          I am a +1 on this too Thanks Daryn, I'll check this in.

          Show
          Robert Joseph Evans added a comment - I am a +1 on this too Thanks Daryn, I'll check this in.
          Hide
          Robert Joseph Evans added a comment -

          Thanks Daryn and Benoy.

          I checked this into trunk, branch-2, and branch-0.23.

          Show
          Robert Joseph Evans added a comment - Thanks Daryn and Benoy. I checked this into trunk, branch-2, and branch-0.23.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-0.23-Build #360 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Build/360/)
          svn merge -c 1379100. FIXES: HADOOP-8726. The Secrets in Credentials are not available to MR tasks (daryn and Benoy Antony via bobby) (Revision 1379109)

          Result = UNSTABLE
          bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1379109
          Files :

          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java
          • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java
          • /hadoop/common/branches/branch-0.23/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapred/YarnChild.java
          • /hadoop/common/branches/branch-0.23/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/MRAppMaster.java
          • /hadoop/common/branches/branch-0.23/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/TestJob.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-0.23-Build #360 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Build/360/ ) svn merge -c 1379100. FIXES: HADOOP-8726 . The Secrets in Credentials are not available to MR tasks (daryn and Benoy Antony via bobby) (Revision 1379109) Result = UNSTABLE bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1379109 Files : /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java /hadoop/common/branches/branch-0.23/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapred/YarnChild.java /hadoop/common/branches/branch-0.23/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/MRAppMaster.java /hadoop/common/branches/branch-0.23/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/TestJob.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1151 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1151/)
          HADOOP-8726. The Secrets in Credentials are not available to MR tasks (daryn and Benoy Antony via bobby) (Revision 1379100)

          Result = FAILURE
          bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1379100
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapred/YarnChild.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/MRAppMaster.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/TestJob.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1151 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1151/ ) HADOOP-8726 . The Secrets in Credentials are not available to MR tasks (daryn and Benoy Antony via bobby) (Revision 1379100) Result = FAILURE bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1379100 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapred/YarnChild.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/MRAppMaster.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/TestJob.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1182 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1182/)
          HADOOP-8726. The Secrets in Credentials are not available to MR tasks (daryn and Benoy Antony via bobby) (Revision 1379100)

          Result = SUCCESS
          bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1379100
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapred/YarnChild.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/MRAppMaster.java
          • /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/TestJob.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1182 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1182/ ) HADOOP-8726 . The Secrets in Credentials are not available to MR tasks (daryn and Benoy Antony via bobby) (Revision 1379100) Result = SUCCESS bobby : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1379100 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/Credentials.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestCredentials.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestUserGroupInformation.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapred/YarnChild.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-app/src/main/java/org/apache/hadoop/mapreduce/v2/app/MRAppMaster.java /hadoop/common/trunk/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/test/java/org/apache/hadoop/mapreduce/TestJob.java
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Benoy Antony and Daryn Sharp, I wish this had a MR specific ticket. Some of us don't necessarily follow all of common tickets. Please do so in future. Tx.

          Show
          Vinod Kumar Vavilapalli added a comment - Benoy Antony and Daryn Sharp , I wish this had a MR specific ticket. Some of us don't necessarily follow all of common tickets. Please do so in future. Tx.

            People

            • Assignee:
              Daryn Sharp
              Reporter:
              Benoy Antony
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development