bugfix: sign content-type and x-amz-user-agent

.changes/nextrelease/signed-headers.json

@@ -0,0 +1,7 @@
+[
+  {
+    "type": "bugfix",
+    "category": "Signature",
+    "description": "Updates signed headers to include `x-amz-user-agent` and `content-type`"
+  }
+]

src/Signature/SignatureV4.php

@@ -4,7 +4,6 @@
 use Aws\Credentials\CredentialsInterface;
 use AWS\CRT\Auth\Signable;
 use AWS\CRT\Auth\SignatureType;
-use AWS\CRT\Auth\SignedBodyHeaderType;
 use AWS\CRT\Auth\Signing;
 use AWS\CRT\Auth\SigningAlgorithm;
 use AWS\CRT\Auth\SigningConfigAWS;
@@ -49,7 +48,6 @@ protected function getHeaderBlacklist()
     {
         return [
             'cache-control'         => true,
-            'content-type'          => true,
             'content-length'        => true,
             'expect'                => true,
             'max-forwards'          => true,
@@ -67,13 +65,31 @@ protected function getHeaderBlacklist()
             'from'                  => true,
             'referer'               => true,
             'user-agent'            => true,
-            'X-Amz-User-Agent'      => true,
             'x-amzn-trace-id'       => true,
             'aws-sdk-invocation-id' => true,
             'aws-sdk-retry'         => true,
         ];
     }
 
+    /**
+     * Headers that must be excluded from presigned URLs (in addition to
+     * the regular header blacklist). These headers are excluded because the
+     * consumer of the presigned URL cannot reliably reproduce them at the
+     * time the URL is used:
+     *  - content-type: caller-determined at upload time
+     *  - x-amz-user-agent: specific to the SDK that generated the URL
+     *
+     * @return array
+     */
+    protected function getPresignHeaderDenyList()
+    {
+        return [
+            'content-type'      => true,
+            'x-amz-user-agent'  => true,
+        ];
+    }
+
+
     /**
      * @param string $service Service name to use when signing
      * @param string $region  Region name to use when signing
@@ -397,15 +413,16 @@ private function convertExpires($expiresTimestamp, $startTimestamp)
 
     private function moveHeadersToQuery(array $parsedRequest)
     {
-        //x-amz-user-agent shouldn't be put in a query param
-        unset($parsedRequest['headers']['X-Amz-User-Agent']);
+        $presignDenyList = $this->getPresignHeaderDenyList();
+        $blacklist = $this->getHeaderBlacklist() + $presignDenyList;
 
         foreach ($parsedRequest['headers'] as $name => $header) {
             $lname = strtolower($name);
-            if (substr($lname, 0, 5) == 'x-amz') {
+            // Move x-amz-* headers into the query string, but skip those that
+            // must not appear in presigned URLs (e.g. x-amz-user-agent).
+            if (substr($lname, 0, 5) == 'x-amz' && !isset($presignDenyList[$lname])) {
                 $parsedRequest['query'][$name] = $header;
             }
-            $blacklist = $this->getHeaderBlacklist();
             if (isset($blacklist[$lname])
                 || $lname === strtolower(self::AMZ_CONTENT_SHA256_HEADER)
             ) {
@@ -416,6 +433,7 @@ private function moveHeadersToQuery(array $parsedRequest)
         return $parsedRequest;
     }
 
+
     private function parseRequest(RequestInterface $request)
     {
         // Clean up any previously set headers.
@@ -479,7 +497,7 @@ private function removeIllegalV4aHeaders(&$request)
             'aws-sdk-invocation-id',
             'aws-sdk-retry',
             'x-amz-region-set',
-            'transfer-encoding'
+            'transfer-encoding',
         ];
         $storedHeaders = [];
 
@@ -569,13 +587,15 @@ protected function presignWithV4a(
         ]);
 
         $this->removeIllegalV4aHeaders($request);
-        foreach ($this->getHeaderBlacklist() as $headerName => $headerValue) {
+        $denyList = $this->getHeaderBlacklist() + $this->getPresignHeaderDenyList();
+        foreach ($denyList as $headerName => $headerValue) {
             if ($request->hasHeader($headerName)) {
                 $request = $request->withoutHeader($headerName);
             }
         }
 
         $http_request = $this->CRTRequestFromGuzzleRequest($request);
+
         Signing::signRequestAws(
             Signable::fromHttpRequest($http_request),
             $signingConfig, function ($signing_result, $error_code) use (&$http_request) {

tests/Signature/SignatureV4Test.php

@@ -319,6 +319,78 @@ public function testPresignBlacklistedHeaders()
         $this->assertStringNotContainsString('Content-Type', (string)$presigned->getUri());
     }
 
+    /**
+     * Regression test for github.com/aws/aws-sdk-php/issues/3283.
+     *
+     * `content-type` should be included in SignedHeaders when present on a
+     * request that is signed (not presigned). Other AWS SDKs (botocore,
+     * Java, JS) sign `content-type` by default, so excluding it from the
+     * PHP SDK breaks cross-SDK signature verification.
+     */
+    public function testSignsContentTypeHeader()
+    {
+        $sig = new SignatureV4('foo', 'bar');
+        $creds = new Credentials('a', 'b');
+        $req = new Request('POST', 'http://foo.com', [
+            'host' => 'foo.com',
+            'Content-Type' => 'application/json',
+        ]);
+        $signed = $sig->signRequest($req, $creds);
+        $this->assertStringContainsString(
+            'content-type',
+            $signed->getHeaderLine('Authorization')
+        );
+    }
+
+    /**
+     * Regression test for github.com/aws/aws-sdk-php/issues/3274.
+     *
+     * `x-amz-user-agent` should be included in SignedHeaders when present
+     * on a request that is signed (not presigned). It is an `x-amz-*`
+     * header and AWS services require all such headers to be signed.
+     */
+    public function testSignsXAmzUserAgentHeader()
+    {
+        $sig = new SignatureV4('foo', 'bar');
+        $creds = new Credentials('a', 'b');
+        $req = new Request('POST', 'http://foo.com', [
+            'host' => 'foo.com',
+            'X-Amz-User-Agent' => 'aws-sdk-php/3.x',
+        ]);
+        $signed = $sig->signRequest($req, $creds);
+        $this->assertStringContainsString(
+            'x-amz-user-agent',
+            $signed->getHeaderLine('Authorization')
+        );
+    }
+
+    /**
+     * `content-type` and `x-amz-user-agent` must NOT be included in
+     * presigned URL SignedHeaders, since the consumer of the URL cannot
+     * be expected to reproduce these header values when invoking the URL.
+     */
+    public function testPresignExcludesContentTypeAndUserAgent()
+    {
+        $sig = new SignatureV4('foo', 'bar');
+        $creds = new Credentials('a', 'b');
+        $req = new Request('PUT', 'http://foo.com', [
+            'host' => 'foo.com',
+            'Content-Type' => 'application/json',
+            'X-Amz-User-Agent' => 'aws-sdk-php/3.x',
+            'x-amz-meta-foo' => 'bar',
+        ]);
+        $presigned = $sig->presign($req, $creds, '+5 minutes');
+        $uri = (string)$presigned->getUri();
+        $this->assertStringNotContainsString('content-type', $uri);
+        $this->assertStringNotContainsString('Content-Type', $uri);
+        $this->assertStringNotContainsString('user-agent', $uri);
+        $this->assertStringNotContainsString('User-Agent', $uri);
+        // x-amz-meta-foo is a normal x-amz-* header that should still be
+        // moved to query and signed.
+        $this->assertStringContainsString('x-amz-meta-foo', $uri);
+    }
+
+
     public function testCanonicalQuerySortingOccursAfterEncoding()
     {
         $_SERVER['aws_time'] = '20110909T233600Z';