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.

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

Refresh timestamp for each signing event for SigV4 #162

Merged
merged 7 commits into from
Oct 30, 2013

Conversation

garnaat
Copy link
Member

@garnaat garnaat commented Oct 19, 2013

We are creating a timestamp in the SigV4Auth class but this causes problems if you hold onto an endpoint for more than five minutes or so. The timestamp expires and the signature is rejected. We need to create a new timestamp each time add_auth is called. But we also need a way to pass in a timestamp to use for the unit tests. This fixes #149.

@toastdriven
Copy link
Contributor

LGTM.

@jamesls
Copy link
Member

jamesls commented Oct 21, 2013

If we pass in the datetime.utcnow function into the init, then we wouldn't have divergent code paths in the tests vs actual use. Or we could just mock.patch the utcnow() call.

jamesls and others added 7 commits October 21, 2013 22:20
If a service has not metadata dict that contains
regions/endpoint/protocol info, we should still be able to
construct endpoints by passing endpoint_url.  I've also
updated the code to not require region_name unless it's a sigv4
service (which is the only reason we need to require a region name
if we're given an explicit endpoint_url).

With this commit, we can consume the unprocessed model as it
exists in services/*.json.
* regions-not-required:
  Add test that verifies a region is required with no endpoint_url
  Specifying endpoint_url should not require metadata dict
…oblems if you hold onto an endpoint for more than five minutes or so. The timestamp expires and the signature is rejected. We need to create a new timestamp each time add_auth is called. But we also need a way to pass in a timestamp to use for the unit tests. This fixes boto#149.
@garnaat
Copy link
Member Author

garnaat commented Oct 23, 2013

I changed it to mock datetime.datetime.now() using the approach Ned Batchelder described. That all seems to work fine. Still need to create a timestamp in the constructor just for the unit tests.

I also just noticed that if I set a region in my config file, I get three test failures in botocore but that happens in develop branch, too. Looking at how to fix that but its not really relevant to this PR.

@jamesls
Copy link
Member

jamesls commented Oct 25, 2013

:shipit: Looks good.

In the future we might make everything except add_auth an internal method, because the timestamp in the __init__ is needed for more than just the unit tests. For example I can't create a sigv4 class and call the public scope() method unless a timestamp has been created. Really there's no reason for code to be calling anything other than add_auth so we could make everything else internal eventually, though we'd have to find a different way to test the sigv4 components.

@garnaat garnaat merged commit d5df4ad into boto:develop Oct 30, 2013
@garnaat garnaat deleted the fix-sigv4-timestamp branch October 30, 2013 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SigV4Auth should not use cached timestamps for signing requests.
3 participants