Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move expires timestamp in SignatureV4 to be generated before startTimestamp #1325

Merged
merged 9 commits into from Aug 4, 2017
Merged

Move expires timestamp in SignatureV4 to be generated before startTimestamp #1325

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
bbc6908
Moved expires timestamp in SignatureV4 to be generated before startTi…
daum Jul 6, 2017
e98a955
Added changelog document
daum Jul 6, 2017
6e7ac7c
Updated Signature Presign tests to not use reflection to test differe…
daum Jul 7, 2017
30d6c8b
Updated line spacing on test to be under 80 characters
daum Jul 11, 2017
12cc59e
Removed extra period in sentence in change doc.
daum Jul 13, 2017
880c3e1
Added relativeTimeBase option to specify a base time for all relative…
daum Aug 3, 2017
de105ed
Removed relativeTimeBase option and instead uses startTime as base.
daum Aug 3, 2017
6cd12a2
Merge branch 'master' into bugfix-signature-expiration-second-break
daum Aug 3, 2017
851c29f
Formatting update for line breaks
daum Aug 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changes/nextrelease/signatureExpiratinoEdgeCaseFix.json
View file
Open in desktop
@@ -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."
}
]
14 changes: 8 additions & 6 deletions src/Signature/SignatureV4.php
View file
Open in desktop
Expand Up @@ -82,7 +82,9 @@ public function presign(
$expires,
array $options = []
) {
$startTimestamp = isset($options['start_time']) ? $this->convertToTimestamp($options['start_time']) : time();

$startTimestamp = isset($options['start_time']) ? $this->convertToTimestamp($options['start_time'], null) : time();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please break this into multiple lines. We try to keep all line length to be of less than 80 characters in the PHP SDK.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to my other comment - how would you like the ternary broken up. If you would prefer I could also switch it to a if/else which would have a shorter line length.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example of what our 3 line ternary looks like can be found here.

$expiresTimestamp = $this->convertToTimestamp($expires, $startTimestamp);

$parsed = $this->createPresignedRequest($request, $credentials);
$payload = $this->getPresignedPayload($request);
Expand All @@ -94,7 +96,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(
Expand Down Expand Up @@ -285,22 +287,22 @@ private function getCanonicalizedQuery(array $query)
return substr($qs, 0, -1);
}

private function convertToTimestamp($dateValue)
private function convertToTimestamp($dateValue, $relativeTimeBase = null)
{
if ($dateValue instanceof \DateTime) {
$timestamp = $dateValue->getTimestamp();
} elseif (!is_numeric($dateValue)) {
$timestamp = strtotime($dateValue);
$timestamp = strtotime($dateValue, $relativeTimeBase === null ? time() : $relativeTimeBase );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please break this into multiple lines. We try to keep all line length to be of less than 80 characters in the PHP SDK.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you want to break the ternary statements? I personally think they are much more readable on one line like this but understand the 80 character comment, was just following other examples in the code like https://github.com/daum/aws-sdk-php/blob/6cd12a2744b9da9813faf1f7d6bf3f52d336c67a/src/Signature/SignatureV4.php#L39

Can you just give me an example of how you'd like the ternary formatted for the multiple lines and I'll get that fixed up asap.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this case, the ternary can be one line but the arguments would be separated.

strtotime(
    $dateValue,
    $relativeTimeBase === null ? time() : $relativeTimeBase
);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks just updated

} else {
$timestamp = $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) {
Expand Down
12 changes: 12 additions & 0 deletions tests/Signature/S3SignatureV4Test.php
View file
Open in desktop
Expand Up @@ -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',
['start_time' => $_SERVER['aws_time']]
)->getUri();
$this->assertContains('X-Amz-Expires=518400', $url);
}

public function testAddsSecurityTokenIfPresent()
{
list($request, $credentials, $signature) = $this->getFixtures();
Expand Down
43 changes: 22 additions & 21 deletions tests/Signature/SignatureV4Test.php
View file
Open in desktop
Expand Up @@ -95,43 +95,44 @@ 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()
Expand Down