Uploaded image for project: 'Hadoop Common'
  1. Hadoop Common
  2. HADOOP-13287

TestS3ACredentials#testInstantiateFromURL fails if AWS secret key contains '+'.

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.8.0, 3.0.0-alpha1
    • Component/s: fs/s3, test
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Reviewed

      Description

      HADOOP-3733 fixed accessing S3A with credentials on the command line for an AWS secret key containing a '/'. The patch added a new test suite: TestS3ACredentialsInURL. One of the tests fails if your AWS secret key contains a '+'.

      1. HADOOP-13287.001.patch
        2 kB
        Chris Nauroth
      2. HADOOP-13287.002.patch
        2 kB
        Chris Nauroth

        Issue Links

          Activity

          Hide
          cnauroth Chris Nauroth added a comment - - edited

          My test run was on branch-2.8 against an S3 bucket in US-west-2. What I saw happening was a double decoding in S3xLoginHelper#extractLoginDetails:

            public static Login extractLoginDetails(URI name) {
              try {
                String authority = name.getAuthority();
          ...
                  String password = URLDecoder.decode(login.substring(loginSplit + 1),
                      "UTF-8");
          

          According to the JavaDocs for URI#getAuthority, it performs decoding already on the output. Then we do a second explicit decoding by calling URLDecoder#decode. First, getAuthority translates "%2B" to "+". Then, URLDecoder#decode translates "+" to " ", which isn't correct for the credentials.

          However, this appear to be only a problem in the JUnit test runs. I also built a distro and tested manually with URIs that contain '+' encoded as "%2B", and that worked just fine. The reason it works fine there is because of different encoding rules applied by round-tripping through a Path before the FileSystem#get call gets triggered. With Path, the '+' gets double-encoded to "%252B", so double-decoding at the S3A layer is correct logic.

          To make this work, the test should follow the same encoding as would be used on the CLI. The attached path switches from constructing a URI to constructing a Path. I switched the exception stifling logic to catch IllegalArgumentException, becauses that's what Path throws. With this, the test passes with a secret containing a '+'. Steve Loughran or Ravi Prakash, I understand one of you might have a secret with a '/' from your work on HADOOP-3733. Would you mind testing this patch to make sure the test still passes with '/'?

          Show
          cnauroth Chris Nauroth added a comment - - edited My test run was on branch-2.8 against an S3 bucket in US-west-2. What I saw happening was a double decoding in S3xLoginHelper#extractLoginDetails : public static Login extractLoginDetails(URI name) { try { String authority = name.getAuthority(); ... String password = URLDecoder.decode(login.substring(loginSplit + 1), "UTF-8" ); According to the JavaDocs for URI#getAuthority , it performs decoding already on the output. Then we do a second explicit decoding by calling URLDecoder#decode . First, getAuthority translates "%2B" to "+". Then, URLDecoder#decode translates "+" to " ", which isn't correct for the credentials. However, this appear to be only a problem in the JUnit test runs. I also built a distro and tested manually with URIs that contain '+' encoded as "%2B", and that worked just fine. The reason it works fine there is because of different encoding rules applied by round-tripping through a Path before the FileSystem#get call gets triggered. With Path , the '+' gets double-encoded to "%252B", so double-decoding at the S3A layer is correct logic. To make this work, the test should follow the same encoding as would be used on the CLI. The attached path switches from constructing a URI to constructing a Path . I switched the exception stifling logic to catch IllegalArgumentException , becauses that's what Path throws. With this, the test passes with a secret containing a '+'. Steve Loughran or Ravi Prakash , I understand one of you might have a secret with a '/' from your work on HADOOP-3733 . Would you mind testing this patch to make sure the test still passes with '/'?
          Hide
          cnauroth Chris Nauroth added a comment -

          Reattaching correct patch.

          Show
          cnauroth Chris Nauroth added a comment - Reattaching correct patch.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 20s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          +1 mvninstall 7m 52s trunk passed
          +1 compile 0m 15s trunk passed
          +1 checkstyle 0m 13s trunk passed
          +1 mvnsite 0m 18s trunk passed
          +1 mvneclipse 0m 14s trunk passed
          +1 findbugs 0m 24s trunk passed
          +1 javadoc 0m 13s trunk passed
          +1 mvninstall 0m 12s the patch passed
          +1 compile 0m 12s the patch passed
          +1 javac 0m 12s the patch passed
          +1 checkstyle 0m 10s the patch passed
          +1 mvnsite 0m 14s the patch passed
          +1 mvneclipse 0m 9s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 0m 30s the patch passed
          +1 javadoc 0m 10s the patch passed
          +1 unit 0m 12s hadoop-aws in the patch passed.
          +1 asflicense 0m 15s The patch does not generate ASF License warnings.
          12m 31s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:e2f6409
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12811471/HADOOP-13287.001.patch
          JIRA Issue HADOOP-13287
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux ec989efeb769 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 2800695
          Default Java 1.8.0_91
          findbugs v3.0.0
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9820/testReport/
          modules C: hadoop-tools/hadoop-aws U: hadoop-tools/hadoop-aws
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9820/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 20s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 7m 52s trunk passed +1 compile 0m 15s trunk passed +1 checkstyle 0m 13s trunk passed +1 mvnsite 0m 18s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 0m 24s trunk passed +1 javadoc 0m 13s trunk passed +1 mvninstall 0m 12s the patch passed +1 compile 0m 12s the patch passed +1 javac 0m 12s the patch passed +1 checkstyle 0m 10s the patch passed +1 mvnsite 0m 14s the patch passed +1 mvneclipse 0m 9s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 30s the patch passed +1 javadoc 0m 10s the patch passed +1 unit 0m 12s hadoop-aws in the patch passed. +1 asflicense 0m 15s The patch does not generate ASF License warnings. 12m 31s Subsystem Report/Notes Docker Image:yetus/hadoop:e2f6409 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12811471/HADOOP-13287.001.patch JIRA Issue HADOOP-13287 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux ec989efeb769 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 2800695 Default Java 1.8.0_91 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9820/testReport/ modules C: hadoop-tools/hadoop-aws U: hadoop-tools/hadoop-aws Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9820/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          -1. I'm afrad

          I'm getting failures in a test, when I use a secret with a / in it

          testInstantiateFromURL(org.apache.hadoop.fs.s3a.TestS3ACredentialsInURL)  Time elapsed: 0.146 sec  <<< FAILURE!
          java.lang.AssertionError: test URI encodes the / symbol
          	at org.junit.Assert.fail(Assert.java:88)
          	at org.junit.Assert.assertTrue(Assert.java:41)
          	at org.apache.hadoop.fs.s3a.TestS3ACredentialsInURL.testInstantiateFromURL(TestS3ACredentialsInURL.java:74)
          

          Revert the patch and the test runs

          The encoded string now contains "%252F"

          Show
          stevel@apache.org Steve Loughran added a comment - -1. I'm afrad I'm getting failures in a test, when I use a secret with a / in it testInstantiateFromURL(org.apache.hadoop.fs.s3a.TestS3ACredentialsInURL) Time elapsed: 0.146 sec <<< FAILURE! java.lang.AssertionError: test URI encodes the / symbol at org.junit.Assert.fail(Assert.java:88) at org.junit.Assert.assertTrue(Assert.java:41) at org.apache.hadoop.fs.s3a.TestS3ACredentialsInURL.testInstantiateFromURL(TestS3ACredentialsInURL.java:74) Revert the patch and the test runs The encoded string now contains "%252F"
          Hide
          cnauroth Chris Nauroth added a comment - - edited

          Thanks for testing, Steve. Could you please try again with patch 002? That assertion needed to be updated to reflect the fact that we're now double-encoding the input, for symmetry with the double-decoding done in the main code. I also added a similar assertion for '+'. I retested against US-west-2 using my secret key with a '+'.

          Show
          cnauroth Chris Nauroth added a comment - - edited Thanks for testing, Steve. Could you please try again with patch 002? That assertion needed to be updated to reflect the fact that we're now double-encoding the input, for symmetry with the double-decoding done in the main code. I also added a similar assertion for '+'. I retested against US-west-2 using my secret key with a '+'.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 27s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
          +1 mvninstall 7m 11s trunk passed
          +1 compile 0m 15s trunk passed
          +1 checkstyle 0m 12s trunk passed
          +1 mvnsite 0m 18s trunk passed
          +1 mvneclipse 0m 13s trunk passed
          +1 findbugs 0m 27s trunk passed
          +1 javadoc 0m 22s trunk passed
          +1 mvninstall 0m 14s the patch passed
          +1 compile 0m 12s the patch passed
          +1 javac 0m 12s the patch passed
          +1 checkstyle 0m 10s the patch passed
          +1 mvnsite 0m 16s the patch passed
          +1 mvneclipse 0m 11s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 0m 34s the patch passed
          +1 javadoc 0m 11s the patch passed
          +1 unit 0m 13s hadoop-aws in the patch passed.
          +1 asflicense 0m 16s The patch does not generate ASF License warnings.
          12m 18s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:e2f6409
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12811731/HADOOP-13287.002.patch
          JIRA Issue HADOOP-13287
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux c50fe11da9b9 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 0319d73
          Default Java 1.8.0_91
          findbugs v3.0.0
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9830/testReport/
          modules C: hadoop-tools/hadoop-aws U: hadoop-tools/hadoop-aws
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9830/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 27s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 7m 11s trunk passed +1 compile 0m 15s trunk passed +1 checkstyle 0m 12s trunk passed +1 mvnsite 0m 18s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 0m 27s trunk passed +1 javadoc 0m 22s trunk passed +1 mvninstall 0m 14s the patch passed +1 compile 0m 12s the patch passed +1 javac 0m 12s the patch passed +1 checkstyle 0m 10s the patch passed +1 mvnsite 0m 16s the patch passed +1 mvneclipse 0m 11s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 34s the patch passed +1 javadoc 0m 11s the patch passed +1 unit 0m 13s hadoop-aws in the patch passed. +1 asflicense 0m 16s The patch does not generate ASF License warnings. 12m 18s Subsystem Report/Notes Docker Image:yetus/hadoop:e2f6409 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12811731/HADOOP-13287.002.patch JIRA Issue HADOOP-13287 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux c50fe11da9b9 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 0319d73 Default Java 1.8.0_91 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9830/testReport/ modules C: hadoop-tools/hadoop-aws U: hadoop-tools/hadoop-aws Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9830/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          +1 from me; tests passing and verified with a hadoop dist build, hadoop fs -ls URL with the URL containing a / secret.

          ravi might want to test it too

          Show
          stevel@apache.org Steve Loughran added a comment - +1 from me; tests passing and verified with a hadoop dist build, hadoop fs -ls URL with the URL containing a / secret. ravi might want to test it too
          Hide
          stevel@apache.org Steve Loughran added a comment -

          (all tests passing; S3 ireland, this time unintentionally VPNed in to the US for extra latency)

          Show
          stevel@apache.org Steve Loughran added a comment - (all tests passing; S3 ireland, this time unintentionally VPNed in to the US for extra latency)
          Hide
          cnauroth Chris Nauroth added a comment -

          Steve, thank you for the review. I committed this to trunk, branch-2 and branch-2.8. ravi, if you do notice any problems when you get around to trying the tests, please let me know.

          Show
          cnauroth Chris Nauroth added a comment - Steve, thank you for the review. I committed this to trunk, branch-2 and branch-2.8. ravi , if you do notice any problems when you get around to trying the tests, please let me know.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Hadoop-trunk-Commit #9996 (See https://builds.apache.org/job/Hadoop-trunk-Commit/9996/)
          HADOOP-13287. TestS3ACredentials#testInstantiateFromURL fails if AWS (cnauroth: rev b2c596cdda7c129951074bc53b4b9ecfedbf080a)

          • hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3ACredentialsInURL.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-trunk-Commit #9996 (See https://builds.apache.org/job/Hadoop-trunk-Commit/9996/ ) HADOOP-13287 . TestS3ACredentials#testInstantiateFromURL fails if AWS (cnauroth: rev b2c596cdda7c129951074bc53b4b9ecfedbf080a) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3ACredentialsInURL.java
          Hide
          raviprak Ravi Prakash added a comment -

          Sorry! I was trying to run the tests, but got distracted before I could figure out how. By the way my handle is raviprak

          Show
          raviprak Ravi Prakash added a comment - Sorry! I was trying to run the tests, but got distracted before I could figure out how. By the way my handle is raviprak

            People

            • Assignee:
              cnauroth Chris Nauroth
              Reporter:
              cnauroth Chris Nauroth
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development