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

Feature cloudfront sign #735

Merged
merged 11 commits into from
Dec 8, 2015
Merged

Conversation

rayluo
Copy link
Contributor

@rayluo rayluo commented Dec 4, 2015

Provide the signer class. It will be used in CLI soon.

Also note that there is no (hard) dependency of rsa in this implementation.

setup.py and requirements.txt are also updated to include new dependency rsa.
@rayluo rayluo added the pr/needs-review This PR needs a review from a member of the team. label Dec 4, 2015
def datetime2timestamp(dt):
"""Calculate the timestamp based on the given datetime instance.

Naive datetime input will be treated as UTC time rather than local time.
Copy link
Member

Choose a reason for hiding this comment

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

Curious to see what others think about this. A utility function further down in this module does the same thing, but that was specifically for back-compat to fix a regression. Not sure if it makes sense to carry that forward.

We'd still have to support the no timezone == UTC in the CLI, but we don't necessarily have to have that throughout the botocore utility methods. I could go either way.

:type rsa_signer: callable
:param rsa_signer: An RSA signer.
Its only input parameter will be the message to be signed,
and its output will be the signed content as a binary string.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also may be good to say that it requires SHA-1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. See the next line in code.

@kyleknap
Copy link
Contributor

kyleknap commented Dec 7, 2015

Looks good. Just had some small comments and questions that would be good to discuss.

@jamesls jamesls added incorporating-feedback and removed pr/needs-review This PR needs a review from a member of the team. labels Dec 7, 2015
@rayluo rayluo added pr/needs-review This PR needs a review from a member of the team. and removed incorporating-feedback labels Dec 8, 2015
# Note:
# 1. Order in canned policy is significant. Special care has been taken
# to ensure the output will match the order defined by document.
# There is also a test case in this commit to ensure that order.
Copy link
Member

Choose a reason for hiding this comment

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

Remove reference to "this commit". That's very easy to get out of date as changes are made.

@jamesls
Copy link
Member

jamesls commented Dec 8, 2015

:shipit:

@mtdowling
Copy link
Contributor

🚢

One possibility for a future addition: did you consider adding a helper method to create an rsa_signer for the user? For example, maybe it could be a function that accepts that path to a private key and returns a function that could be given to rsa_signer (basically capturing your example in the doc comment).

@kyleknap
Copy link
Contributor

kyleknap commented Dec 8, 2015

Thanks 🚢

rayluo added a commit that referenced this pull request Dec 8, 2015
@rayluo rayluo merged commit 729057f into boto:develop Dec 8, 2015
@rayluo rayluo added enhancement This issue requests an improvement to a current feature. cloudfront labels Dec 8, 2015

First you create a cloudfront signer based on a normalized RSA signer::

import rsa
Copy link

Choose a reason for hiding this comment

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

Your example should use cryptography instead of rsa.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cloudfront enhancement This issue requests an improvement to a current feature. pr/needs-review This PR needs a review from a member of the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants