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

Edge Case Error: Valid 1 week expiration throws error #1322

Closed
daum opened this Issue Jul 6, 2017 · 8 comments

Comments

Projects
None yet
3 participants
@daum
Contributor

daum commented Jul 6, 2017

On a few projects that we upgraded to the v4 API every once and a while we get the error: "The expiration date of a signature version 4 presigned URL must be less than one week. "

This is when pass through just "1 week" to the presign function. Looking through code it appears that in certain circumstances when you pass through no start time and try to use exactly one week it can cause an error.

https://github.com/aws/aws-sdk-php/blob/master/src/Signature/SignatureV4.php#L85 uses time() if no start time is passed through. However it isn't until later https://github.com/aws/aws-sdk-php/blob/master/src/Signature/SignatureV4.php#L97, where the expiration is checked. The problem arises if the script is on the upper bounds of a second (ie 12:01:01.999999999999999) when it makes a call to the presign function. In this case it is possible the script would not get to the expiration duration check until after it turns 12:01:02.0000000. This causes the exception to be thrown as the expiration duration check would at 12:01:02 get the result of strtotime('1 week'), which would now have an extra second, triggering the exception to be thrown in the convertExpires section. A simple proof of concept of this occurring is located in this gist: https://gist.github.com/daum/0010c9133a6ba27c373e898e2bc87c43

A somewhat simple fix could be moving the actual conversion of the timestamp of "expires" to be the line before https://github.com/aws/aws-sdk-php/blob/master/src/Signature/SignatureV4.php#L85 . This would guarantee that the expiration time is ran first, so if you do break over a second the start time would simply be 6 days, 23 hours, 59 minutes, 59 seconds from the start time, rather than what currently can happen (7 days, 0 min, 1 seconds).

Let me know what you think about this problem and that as a valid solution. I'd be happy to make a PR if it is agreed this is an edge case that should be fixed.

@imshashank

This comment has been minimized.

Show comment
Hide comment
@imshashank

imshashank Jul 6, 2017

Contributor

@daum Thanks for pointing out this issue. Please give us some time to repro this issue. In the meantime, please feel free to open a PR with the above-discussed fix.

Contributor

imshashank commented Jul 6, 2017

@daum Thanks for pointing out this issue. Please give us some time to repro this issue. In the meantime, please feel free to open a PR with the above-discussed fix.

daum added a commit to daum/aws-sdk-php that referenced this issue Jul 6, 2017

Moved expires timestamp in SignatureV4 to be generated before startTi…
…mestamp 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.
@daum

This comment has been minimized.

Show comment
Hide comment
@daum

daum Jul 6, 2017

Contributor

Sure just made a PR here: #1325 . Let me know if you want me to change or add anything. Will take a look at the failed test first thing in the morning.

Contributor

daum commented Jul 6, 2017

Sure just made a PR here: #1325 . Let me know if you want me to change or add anything. Will take a look at the failed test first thing in the morning.

@daum

This comment has been minimized.

Show comment
Hide comment
@daum

daum Jul 7, 2017

Contributor

@imshashank On the tests the reason they are failing is because they use reflection to allow direct access to the private convertExpires method (https://github.com/aws/aws-sdk-php/blob/master/tests/Signature/SignatureV4Test.php#L98). Since I've updated the method to take the converted timestamp already they are now failing.

For now what I did was update the tests to go through the entire presign function and then ensure the URL has the proper expiration duration on it. This does encapsulate a little more than the specific duration test, since if the URL changes the tests would break, however I think it is better than adding an extra convertTimestamp to the convertExpires function. Adding that extra, duplicate convertTimestamp in that method, would never do anything in production and only satisfy the test, so it isn't the best actual test. Second it would be less efficient and less readable, as it would make someone who is quickly reading the code try to think of how the $expiresTimestamp could need to be converted when it already was in the only method which calls convertExpires.

Let me know your thoughts on this and the updated tests.

Contributor

daum commented Jul 7, 2017

@imshashank On the tests the reason they are failing is because they use reflection to allow direct access to the private convertExpires method (https://github.com/aws/aws-sdk-php/blob/master/tests/Signature/SignatureV4Test.php#L98). Since I've updated the method to take the converted timestamp already they are now failing.

For now what I did was update the tests to go through the entire presign function and then ensure the URL has the proper expiration duration on it. This does encapsulate a little more than the specific duration test, since if the URL changes the tests would break, however I think it is better than adding an extra convertTimestamp to the convertExpires function. Adding that extra, duplicate convertTimestamp in that method, would never do anything in production and only satisfy the test, so it isn't the best actual test. Second it would be less efficient and less readable, as it would make someone who is quickly reading the code try to think of how the $expiresTimestamp could need to be converted when it already was in the only method which calls convertExpires.

Let me know your thoughts on this and the updated tests.

@daum

This comment has been minimized.

Show comment
Hide comment
@daum

daum Jul 27, 2017

Contributor

@imshashank Wanted to check in on this, do you think this change will be implemented soon to fix the existing edge case? Just debating on if we need to fork for the time being to account for it.

Contributor

daum commented Jul 27, 2017

@imshashank Wanted to check in on this, do you think this change will be implemented soon to fix the existing edge case? Just debating on if we need to fork for the time being to account for it.

@imshashank

This comment has been minimized.

Show comment
Hide comment
@imshashank

imshashank Jul 27, 2017

Contributor

@daum Currently working on testing on the use cases described by @kstich. Sorry, we can not provide a date of completion for this task as of yet.

Contributor

imshashank commented Jul 27, 2017

@daum Currently working on testing on the use cases described by @kstich. Sorry, we can not provide a date of completion for this task as of yet.

@daum

This comment has been minimized.

Show comment
Hide comment
@daum

daum Jul 28, 2017

Contributor

@imshashank Ok thanks! Happy to help with any test cases if you'd like.

Contributor

daum commented Jul 28, 2017

@imshashank Ok thanks! Happy to help with any test cases if you'd like.

@kstich

This comment has been minimized.

Show comment
Hide comment
@kstich

kstich Aug 4, 2017

Contributor

#1325 has been merged in and will be bundled in to the next SDK release.

Contributor

kstich commented Aug 4, 2017

#1325 has been merged in and will be bundled in to the next SDK release.

@kstich

This comment has been minimized.

Show comment
Hide comment
@kstich

kstich Aug 8, 2017

Contributor

#1325 was bundled in to version 3.32.4. Thanks again for your work @daum!

Contributor

kstich commented Aug 8, 2017

#1325 was bundled in to version 3.32.4. Thanks again for your work @daum!

@kstich kstich closed this Aug 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment