From bbc6908eaa94768cd7e1c007edc05916c6a74972 Mon Sep 17 00:00:00 2001 From: Daum Date: Thu, 6 Jul 2017 19:12:46 -0400 Subject: [PATCH 1/8] Moved expires timestamp in SignatureV4 to be generated before startTimestamp for edge case of second roll overs. As described in issue #1322 an edge case of using exactly 1 week expiration when a second rolls between the startTimestamp calculation via time() and the expires timestamp can cause incorrect exceptions. This occurs when the second rolls over (ie 12:01:01.9999999 to 12:02:02.00000) between the startTime (time()) and endTime conversion from strtotime(). This causes the exception of the expiration must be less than a week to be thrown. By moving the endTime to be converted before startTime this guarantees that will never occur. --- src/Signature/SignatureV4.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Signature/SignatureV4.php b/src/Signature/SignatureV4.php index f0fd13db90..bbd59a489d 100644 --- a/src/Signature/SignatureV4.php +++ b/src/Signature/SignatureV4.php @@ -82,6 +82,7 @@ public function presign( $expires, array $options = [] ) { + $expiresTimestamp = $this->convertToTimestamp($expires); $startTimestamp = isset($options['start_time']) ? $this->convertToTimestamp($options['start_time']) : time(); $parsed = $this->createPresignedRequest($request, $credentials); @@ -94,7 +95,7 @@ public function presign( $parsed['query']['X-Amz-Credential'] = $credential; $parsed['query']['X-Amz-Date'] = gmdate('Ymd\THis\Z', $startTimestamp); $parsed['query']['X-Amz-SignedHeaders'] = 'host'; - $parsed['query']['X-Amz-Expires'] = $this->convertExpires($expires, $startTimestamp); + $parsed['query']['X-Amz-Expires'] = $this->convertExpires($expiresTimestamp, $startTimestamp); $context = $this->createContext($parsed, $payload); $stringToSign = $this->createStringToSign($httpDate, $scope, $context['creq']); $key = $this->getSigningKey( @@ -298,9 +299,9 @@ private function convertToTimestamp($dateValue) return $timestamp; } - private function convertExpires($expires, $startTimestamp) + private function convertExpires($expiresTimestamp, $startTimestamp) { - $duration = $this->convertToTimestamp($expires) - $startTimestamp; + $duration = $expiresTimestamp - $startTimestamp; // Ensure that the duration of the signature is not longer than a week if ($duration > 604800) { From e98a95551f4e390cf6f913539a6bff650e0e6552 Mon Sep 17 00:00:00 2001 From: Daum Date: Thu, 6 Jul 2017 19:22:17 -0400 Subject: [PATCH 2/8] Added changelog document --- .changes/nextrelease/signatureExpiratinoEdgeCaseFix.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changes/nextrelease/signatureExpiratinoEdgeCaseFix.json diff --git a/.changes/nextrelease/signatureExpiratinoEdgeCaseFix.json b/.changes/nextrelease/signatureExpiratinoEdgeCaseFix.json new file mode 100644 index 0000000000..c3ec13e46a --- /dev/null +++ b/.changes/nextrelease/signatureExpiratinoEdgeCaseFix.json @@ -0,0 +1,7 @@ +[ + { + "type": "bugfix", + "category": "Signature", + "description": "Fixed edgecase in expiration duration check on signature when seconds roll between implicit startime and relative end time.." + } +] From 6e7ac7c44d3650c77e374301cc17dcc86fc5932e Mon Sep 17 00:00:00 2001 From: Daum Date: Fri, 7 Jul 2017 06:05:43 -0400 Subject: [PATCH 3/8] Updated Signature Presign tests to not use reflection to test different expiration timestamp formats so it only uses public API. --- tests/Signature/SignatureV4Test.php | 35 ++++++++++++----------------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/tests/Signature/SignatureV4Test.php b/tests/Signature/SignatureV4Test.php index 18fd361b90..87c18fa265 100644 --- a/tests/Signature/SignatureV4Test.php +++ b/tests/Signature/SignatureV4Test.php @@ -95,43 +95,36 @@ private function getFixtures() $request = new Request('GET', 'http://foo.com'); $credentials = new Credentials('foo', 'bar'); $signature = new SignatureV4('service', 'region'); - $ref = new \ReflectionMethod($signature, 'convertExpires'); - $ref->setAccessible(true); - return array($request, $credentials, $signature, $ref); + return array($request, $credentials, $signature); } public function testCreatesPresignedDatesFromDateTime() { $_SERVER['override_v4_time'] = true; - list($request, $credentials, $signature, $ref) = $this->getFixtures(); - $this->assertEquals(518400, - $ref->invoke( - $signature, - new \DateTime('December 11, 2013 00:00:00 UTC'), - $_SERVER['aws_time'] - ) - ); + list($request, $credentials, $signature) = $this->getFixtures(); + $credentials = new Credentials('foo', 'bar', '123'); + $url = (string) $signature->presign($request,$credentials,new \DateTime('December 11, 2013 00:00:00 UTC'))->getUri(); + $this->assertContains('X-Amz-Expires=518400',$url); + } public function testCreatesPresignedDatesFromUnixTimestamp() { $_SERVER['override_v4_time'] = true; - list($request, $credentials, $signature, $ref) = $this->getFixtures(); - $this->assertEquals( - 518400, - $ref->invoke($signature, 1386720000, $_SERVER['aws_time']) - ); + list($request, $credentials, $signature) = $this->getFixtures(); + $credentials = new Credentials('foo', 'bar', '123'); + $url = (string) $signature->presign($request,$credentials,1386720000)->getUri(); + $this->assertContains('X-Amz-Expires=518400',$url); } public function testCreatesPresignedDateFromStrtotime() { $_SERVER['override_v4_time'] = true; - list($request, $credentials, $signature, $ref) = $this->getFixtures(); - $this->assertEquals( - 518400, - $ref->invoke($signature, 'December 11, 2013 00:00:00 UTC', $_SERVER['aws_time']) - ); + list($request, $credentials, $signature) = $this->getFixtures(); + $credentials = new Credentials('foo', 'bar', '123'); + $url = (string) $signature->presign($request,$credentials,'December 11, 2013 00:00:00 UTC')->getUri(); + $this->assertContains('X-Amz-Expires=518400',$url); } public function testAddsSecurityTokenIfPresentInPresigned() From 30d6c8bd3372df428e900a5ecdcdecaeafef6d45 Mon Sep 17 00:00:00 2001 From: Daum Date: Tue, 11 Jul 2017 03:33:27 -0400 Subject: [PATCH 4/8] Updated line spacing on test to be under 80 characters --- tests/Signature/SignatureV4Test.php | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/tests/Signature/SignatureV4Test.php b/tests/Signature/SignatureV4Test.php index 87c18fa265..2d64f3d789 100644 --- a/tests/Signature/SignatureV4Test.php +++ b/tests/Signature/SignatureV4Test.php @@ -104,7 +104,11 @@ public function testCreatesPresignedDatesFromDateTime() $_SERVER['override_v4_time'] = true; list($request, $credentials, $signature) = $this->getFixtures(); $credentials = new Credentials('foo', 'bar', '123'); - $url = (string) $signature->presign($request,$credentials,new \DateTime('December 11, 2013 00:00:00 UTC'))->getUri(); + $url = (string) $signature->presign( + $request, + $credentials, + new \DateTime('December 11, 2013 00:00:00 UTC') + )->getUri(); $this->assertContains('X-Amz-Expires=518400',$url); } @@ -123,7 +127,11 @@ public function testCreatesPresignedDateFromStrtotime() $_SERVER['override_v4_time'] = true; list($request, $credentials, $signature) = $this->getFixtures(); $credentials = new Credentials('foo', 'bar', '123'); - $url = (string) $signature->presign($request,$credentials,'December 11, 2013 00:00:00 UTC')->getUri(); + $url = (string) $signature->presign( + $request, + $credentials, + 'December 11, 2013 00:00:00 UTC' + )->getUri(); $this->assertContains('X-Amz-Expires=518400',$url); } From 12cc59ec1ec523cf6032486c29f3d55dca6ca3e1 Mon Sep 17 00:00:00 2001 From: daum Date: Thu, 13 Jul 2017 14:24:04 -0400 Subject: [PATCH 5/8] Removed extra period in sentence in change doc. --- .changes/nextrelease/signatureExpiratinoEdgeCaseFix.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changes/nextrelease/signatureExpiratinoEdgeCaseFix.json b/.changes/nextrelease/signatureExpiratinoEdgeCaseFix.json index c3ec13e46a..fff8352404 100644 --- a/.changes/nextrelease/signatureExpiratinoEdgeCaseFix.json +++ b/.changes/nextrelease/signatureExpiratinoEdgeCaseFix.json @@ -2,6 +2,6 @@ { "type": "bugfix", "category": "Signature", - "description": "Fixed edgecase in expiration duration check on signature when seconds roll between implicit startime and relative end time.." + "description": "Fixed edgecase in expiration duration check on signature when seconds roll between implicit startime and relative end time." } ] From 880c3e1f4c1e00893d5a9591983524efbefa0e8d Mon Sep 17 00:00:00 2001 From: Daum Date: Thu, 3 Aug 2017 17:13:52 -0400 Subject: [PATCH 6/8] Added relativeTimeBase option to specify a base time for all relative timestamps on a presign request. Defaults to current time. --- src/Signature/SignatureV4.php | 9 +++++---- tests/Signature/S3SignatureV4Test.php | 12 ++++++++++++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/src/Signature/SignatureV4.php b/src/Signature/SignatureV4.php index bbd59a489d..6a3220ad3d 100644 --- a/src/Signature/SignatureV4.php +++ b/src/Signature/SignatureV4.php @@ -82,8 +82,9 @@ public function presign( $expires, array $options = [] ) { - $expiresTimestamp = $this->convertToTimestamp($expires); - $startTimestamp = isset($options['start_time']) ? $this->convertToTimestamp($options['start_time']) : time(); + $relativeTimeBase = isset($options['relativeTimeBase']) ? $options['relativeTimeBase'] : time(); + $expiresTimestamp = $this->convertToTimestamp($expires,$relativeTimeBase); + $startTimestamp = isset($options['start_time']) ? $this->convertToTimestamp($options['start_time'],$relativeTimeBase) : time(); $parsed = $this->createPresignedRequest($request, $credentials); $payload = $this->getPresignedPayload($request); @@ -286,12 +287,12 @@ private function getCanonicalizedQuery(array $query) return substr($qs, 0, -1); } - private function convertToTimestamp($dateValue) + private function convertToTimestamp($dateValue, $relativeTimeBase) { if ($dateValue instanceof \DateTime) { $timestamp = $dateValue->getTimestamp(); } elseif (!is_numeric($dateValue)) { - $timestamp = strtotime($dateValue); + $timestamp = strtotime($dateValue, $relativeTimeBase); } else { $timestamp = $dateValue; } diff --git a/tests/Signature/S3SignatureV4Test.php b/tests/Signature/S3SignatureV4Test.php index 366c68eed0..a51aef2eaa 100644 --- a/tests/Signature/S3SignatureV4Test.php +++ b/tests/Signature/S3SignatureV4Test.php @@ -98,6 +98,18 @@ public function testCreatesPresignedDateFromStrtotime() $this->assertContains('X-Amz-Expires=518400', $url); } + public function testCreatesPresignedDateFromStrtotimeRelativeTimeBase() + { + list($request, $credentials, $signature) = $this->getFixtures(); + $url = (string) $signature->presign( + $request, + $credentials, + '+6 days', + ['relativeTimeBase' => $_SERVER['aws_time']] + )->getUri(); + $this->assertContains('X-Amz-Expires=518400', $url); + } + public function testAddsSecurityTokenIfPresent() { list($request, $credentials, $signature) = $this->getFixtures(); From de105ed1ba057784ba881a2f582606361897ada9 Mon Sep 17 00:00:00 2001 From: Daum Date: Thu, 3 Aug 2017 17:32:50 -0400 Subject: [PATCH 7/8] Removed relativeTimeBase option and instead uses startTime as base. --- src/Signature/SignatureV4.php | 10 +++++----- tests/Signature/S3SignatureV4Test.php | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Signature/SignatureV4.php b/src/Signature/SignatureV4.php index 6a3220ad3d..c50f23aa06 100644 --- a/src/Signature/SignatureV4.php +++ b/src/Signature/SignatureV4.php @@ -82,9 +82,9 @@ public function presign( $expires, array $options = [] ) { - $relativeTimeBase = isset($options['relativeTimeBase']) ? $options['relativeTimeBase'] : time(); - $expiresTimestamp = $this->convertToTimestamp($expires,$relativeTimeBase); - $startTimestamp = isset($options['start_time']) ? $this->convertToTimestamp($options['start_time'],$relativeTimeBase) : time(); + + $startTimestamp = isset($options['start_time']) ? $this->convertToTimestamp($options['start_time'], null) : time(); + $expiresTimestamp = $this->convertToTimestamp($expires, $startTimestamp); $parsed = $this->createPresignedRequest($request, $credentials); $payload = $this->getPresignedPayload($request); @@ -287,12 +287,12 @@ private function getCanonicalizedQuery(array $query) return substr($qs, 0, -1); } - private function convertToTimestamp($dateValue, $relativeTimeBase) + private function convertToTimestamp($dateValue, $relativeTimeBase = null) { if ($dateValue instanceof \DateTime) { $timestamp = $dateValue->getTimestamp(); } elseif (!is_numeric($dateValue)) { - $timestamp = strtotime($dateValue, $relativeTimeBase); + $timestamp = strtotime($dateValue, $relativeTimeBase === null ? time() : $relativeTimeBase ); } else { $timestamp = $dateValue; } diff --git a/tests/Signature/S3SignatureV4Test.php b/tests/Signature/S3SignatureV4Test.php index a51aef2eaa..936aefc05f 100644 --- a/tests/Signature/S3SignatureV4Test.php +++ b/tests/Signature/S3SignatureV4Test.php @@ -105,7 +105,7 @@ public function testCreatesPresignedDateFromStrtotimeRelativeTimeBase() $request, $credentials, '+6 days', - ['relativeTimeBase' => $_SERVER['aws_time']] + ['start_time' => $_SERVER['aws_time']] )->getUri(); $this->assertContains('X-Amz-Expires=518400', $url); } From 851c29ff3dd514110bcd376928fe35730e4320ad Mon Sep 17 00:00:00 2001 From: Daum Date: Thu, 3 Aug 2017 20:04:47 -0400 Subject: [PATCH 8/8] Formatting update for line breaks --- src/Signature/SignatureV4.php | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/Signature/SignatureV4.php b/src/Signature/SignatureV4.php index c50f23aa06..eb1e10a669 100644 --- a/src/Signature/SignatureV4.php +++ b/src/Signature/SignatureV4.php @@ -83,7 +83,10 @@ public function presign( array $options = [] ) { - $startTimestamp = isset($options['start_time']) ? $this->convertToTimestamp($options['start_time'], null) : time(); + $startTimestamp = isset($options['start_time']) + ? $this->convertToTimestamp($options['start_time'], null) + : time(); + $expiresTimestamp = $this->convertToTimestamp($expires, $startTimestamp); $parsed = $this->createPresignedRequest($request, $credentials); @@ -292,7 +295,9 @@ private function convertToTimestamp($dateValue, $relativeTimeBase = null) if ($dateValue instanceof \DateTime) { $timestamp = $dateValue->getTimestamp(); } elseif (!is_numeric($dateValue)) { - $timestamp = strtotime($dateValue, $relativeTimeBase === null ? time() : $relativeTimeBase ); + $timestamp = strtotime($dateValue, + $relativeTimeBase === null ? time() : $relativeTimeBase + ); } else { $timestamp = $dateValue; }