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

Add support for AWS_SESSION_TOKEN and AWS_SECURITY_TOKEN #283

Merged
merged 2 commits into from
Mar 31, 2017

Conversation

bxm156
Copy link
Contributor

@bxm156 bxm156 commented Mar 16, 2017

Add support for reading the AWS_SESSION_TOKEN and AWS_SECURITY_TOKEN from the environment.

Verified this works as expected in AWS Lambda.

@bxm156
Copy link
Contributor Author

bxm156 commented Mar 16, 2017

A few questions:

  1. Should this be behind a setting ?
  2. Should we care if the there is a AWS_SESSION_TOKEN but no AWS_ACCESS/SECRET keys in the env?

@codecov-io
Copy link

codecov-io commented Mar 16, 2017

Codecov Report

Merging #283 into master will increase coverage by 0.2%.
The diff coverage is 91.66%.

@@            Coverage Diff            @@
##           master     #283     +/-   ##
=========================================
+ Coverage   72.28%   72.48%   +0.2%     
=========================================
  Files          10       10             
  Lines        1371     1381     +10     
=========================================
+ Hits          991     1001     +10     
  Misses        380      380
Impacted Files Coverage Δ
storages/backends/s3boto.py 86.36% <91.66%> (+0.22%) ⬆️
storages/backends/s3boto3.py 84.95% <91.66%> (+0.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 043b91b...9e1f832. Read the comment docs.

@AusIV
Copy link

AusIV commented Mar 22, 2017

I just encountered the same issue, and was glad to see someone had already done the legwork. I've also verified that this solves my problem on Lambda.

@narfman0
Copy link

I've verified this in my project as well, thanks Bryan! Should fix #282 too.

@jschneier
Copy link
Owner

Looks good!

@jschneier jschneier merged commit a9c4a3a into jschneier:master Mar 31, 2017
jschneier added a commit that referenced this pull request Mar 31, 2017
jschneier added a commit that referenced this pull request Mar 31, 2017
@jschneier
Copy link
Owner

@bxm156 re: your questions. The docs are overdue for an overhaul. Authentication should be given its own section. I think relying on just the environment variables is likely better in this case but I need to read the aws docs again just to make sure.

@jschneier
Copy link
Owner

Oof. This only sets .security_token if it exists. I need to switch to live tests because you always try to reference the parameter even if it isn't getting set.

@jschneier
Copy link
Owner

Crappy fix in 2529729

@bxm156
Copy link
Contributor Author

bxm156 commented Apr 4, 2017

Ah, sorry about that, I was planning on writing some tests for this but never got around to it :(

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.

5 participants