-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Move expires timestamp in SignatureV4 to be generated before startTimestamp #1325
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.
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
Move expires timestamp in SignatureV4 to be generated before startTimestamp #1325
Conversation
…mestamp for edge case of second roll overs. As described in issue aws#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.
…nt expiration timestamp formats so it only uses public API.
Codecov Report
@@ Coverage Diff @@
## master #1325 +/- ##
============================================
+ Coverage 91.87% 91.87% +<.01%
- Complexity 2465 2466 +1
============================================
Files 142 142
Lines 7518 7523 +5
============================================
+ Hits 6907 6912 +5
Misses 611 611
Continue to review full report at Codecov.
|
tests/Signature/SignatureV4Test.php
Outdated
); | ||
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please break this into lines less than 80 characters in length.
tests/Signature/SignatureV4Test.php
Outdated
); | ||
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please break this into lines less than 80 characters in length.
@imshashank All set just updated those lines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will end up requiring a deeper change to convertExpires
or convertToTimestamp
to handle a start time to guarantee this is a fix.
{ | ||
"type": "bugfix", | ||
"category": "Signature", | ||
"description": "Fixed edgecase in expiration duration check on signature when seconds roll between implicit startime and relative end time.." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocker: double . at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All set, removed.
src/Signature/SignatureV4.php
Outdated
@@ -82,6 +82,7 @@ public function presign( | |||
$expires, | |||
array $options = [] | |||
) { | |||
$expiresTimestamp = $this->convertToTimestamp($expires); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this still susceptible to being incorrect? The only way to guarantee the strtotime
case for convertExpires()
will be in coordination with the $startTimestamp
is to pass in a $now
.
Consider the case:
1:23:45.9999 - $expiresTimestamp
generated from '+1 week'
1:23:46.0005 - $startTimestamp
generated via time()
1:23:46:0100 - convertExpires()
is called, generating 604799
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'd technically get a different duration however it won't throw an error. Assuming that server times are synced up noone could request it in historical time (requiring the extra second of duration). The goal of this PR is to fix the opposite case where the second rolls and you fall past the 1 week allow duration and it throws an error, even though the initial request is valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does fix the issue of throwing an exception on the calculation, but not the correctness of the relative time cases. For instance, the tests for the AuthTokenGenerator
detect an explicit expires length. Actually, it looks like these are already failing very infrequently.
We could take this PR as is and supply another fix for relative times, though it'd be preferable to fix both at the same time. The change for this would be using convertToTimestamp
in convertExpires
for the non-strtotime
cases and passing the $startTimestamp
to strtotime
's second param. strtotime
will properly ignore being given a $now
parameter when not using a relative time. This would be in lieu of doing a convertToTimestamp
before sending in to convertExpires
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kstich Not sure I follow what the fix for both scenarios would be. I think part of the issue is that if the SDK allows any relative timestamp to be passed there will always be scenarios when the duration for the expires length could be off. The reason for that is that the thread could be paused/roll over a second at any given point, so unless the caller of presign includes a "now" parameter (options perhaps?) there is always the possibility of a slight skew.
We could add that as an option to pass, if passed any strtotime in the convertToTimestamp will use the "now" as the second parameter for the time it is based on.
This would solve any specific duration requests from possibly being off by a tiny amount. We could maintain BC by making it an optional parameter, so if it isn't passed it will just use the current time that the createSign is invoked, which majority of the time would be the same time.
Let me know if that makes sense. I think the change to the existing PR would be fairly minimal. I'd update convertToTimestamp to take a second parameter, which would be passed to the strtotime conversion in it. Then in the options array it'd check for the "now" parameter, if non-existent it'd take current time. What do you think the option should be? "relativeTimeBase" could be more descriptive than now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daum Thanks for working through this with us. I think we're both on the same page with regards to the problem and that the solution being worked towards both fixes the issues at hand and maintains our contracts.
For a relative expires, which will be the only used case for an optional $relativeTimeBase
in convertToTimestamp
, we'll already have $startTimestamp
and the intent of the contract is the relative expires is based on that. The default/non-existent value should just be null, as that will have strtotime
use its own default if we're not overriding. This should probably also warrant a new test on the relative time handling of SignatureV4 of the '+6 days' the suite is set up for; you'll have to pass in the 'start_time' based on the 'aws_time' as an option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sounds good, will try to update the PR with the additional test + other changes either later today or tomorrow hopefully.
@kstich @imshashank Just wanted to check in on this to see if you think it will be integrated soon or if there are other questions. |
… timestamps on a presign request. Defaults to current time.
src/Signature/SignatureV4.php
Outdated
@@ -82,7 +82,9 @@ public function presign( | |||
$expires, | |||
array $options = [] | |||
) { | |||
$startTimestamp = isset($options['start_time']) ? $this->convertToTimestamp($options['start_time']) : time(); | |||
$relativeTimeBase = isset($options['relativeTimeBase']) ? $options['relativeTimeBase'] : time(); | |||
$expiresTimestamp = $this->convertToTimestamp($expires,$relativeTimeBase); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making these updates @daum!
It seems like I was unclear previously when talking about this section, apologies. I don't think there needs to be a separate relativeTimeBase
option, but instead use the $startTimestamp
we already have for this line passed to $relativeTimeBase
of convertToTimestamp
when generating $expiresTimestamp
. I can't think of any cases where someone would want 'relativeTimeBase'
to not be the same as 'start_time'
, let me know if I'm missing something.
And a small formatting update: please add a space after the ,
in this function call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just updated to use the startTimestamp. Let me know if you are ok with that ternary in the strtotime function for the convertTimestamp, wasn't sure if you'd prefer that or a nested if/else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ternary located here isn't actually needed. strtotime
will use its own default ($now = time()
) on being passed null and will correctly ignore the $now
when passed a non-relative $time
string.
$time = time();
$date = date(DATE_ATOM, $time);
echo strtotime($date); // 1501704830
echo strtotime($date, null); // 1501704830
echo strtotime($date, $time); // 1501704830
echo strtotime('+15 minutes', $time); // 1501705730
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kstich I think the segfault on the builds is coming from the precise to trusty upgrade, I can't replicate locally. It also appears this is a change Travis made recently: https://blog.travis-ci.com/2017-07-11-trusty-as-default-linux-is-coming . There were no segfaults before the upgrade, I can add the precise variable into the travis config if you want. Let me know how you want to proceed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kstich Actually that's a fun PHP gotcha, here is what happens if I send it a null when using a relative timestamp (see last test case):
$time = time();
$date = date(DATE_ATOM, $time);
echo strtotime($date)."\n"; // 1501704830
echo strtotime($date, null)."\n"; // 1501704830
echo strtotime($date, $time)."\n"; // 1501704830
echo strtotime('+15 minutes', $time)."\n"; // 1501705730
echo strtotime('+15 minutes', null)."\n"; // 900
The second parameter has to default to time() or a valid int otherwise it starts at 0 which is what you see in my last example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's lovely, this should stay a ternary then. Sorry for the misdirection!
#1339 will temporarily take care of the Travis issue and we'll rerun these jobs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok - will wait for you guys to merge that, then will merge those updates into this branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1339 has been merged in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All set!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor formatting fixes. Besides that LGTM
src/Signature/SignatureV4.php
Outdated
@@ -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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/Signature/SignatureV4.php
Outdated
{ | ||
if ($dateValue instanceof \DateTime) { | ||
$timestamp = $dateValue->getTimestamp(); | ||
} elseif (!is_numeric($dateValue)) { | ||
$timestamp = strtotime($dateValue); | ||
$timestamp = strtotime($dateValue, $relativeTimeBase === null ? time() : $relativeTimeBase ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks just updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks again for this work @daum! |
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.