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

Better support for permissions-restricted (IAM) environments on AWS #223

Closed
wants to merge 10 commits into from

Conversation

@coderanger
Copy link
Contributor

coderanger commented Jan 16, 2014

This addresses two issues I filed (LIBCLOUD-497, LIBCLOUD-498).

It adds:

  • ACL override support for S3 via extra={'acl': '...'}
  • Support for a token= keyword argument on S3 driver (and can be easily added to other AWS drivers) to include the AWS Security Token param/header, which is required when using IAM role-provider credentials or other temporary AS credentials.
  • Makes get_container on S3 no longer call list_containers and otherwise work correctly in an environment with highly restricted permissions. This is a (minorly) backwards incompatible change as `get_container will no longer load the creation time of the bucket. This is not of huge importance, but should be mentioned in the release notes.
@Kami
Copy link
Member

Kami commented Jan 16, 2014

Thanks!

I'm going to add more comments, but for testing that the token is present you should add a test case to the MockHttp class which actually receives the requests and returns the mock responses.

For example, you could set MockHttp.type inside test_token method and then add a corresponding method to the MockHttp class and verify that token is present in the headers.

E.g.

 def _TOKEN(self, method, url, body, headers):
    self.assertEqual(headers['x-amz-security-token'], ...)
   ...
@@ -55,7 +55,7 @@ class Object(object):
"""

def __init__(self, name, size, hash, extra, meta_data, container,
driver):
driver, acl=None):

This comment has been minimized.

@Kami

Kami Jan 16, 2014 Member

Since not all of the providers support ACLs, I'd rather put this inside the extra dictionary.

@@ -134,3 +134,9 @@ class GoogleStorageDriver(S3StorageDriver):
namespace = NAMESPACE
supports_chunked_encoding = False
supports_s3_multipart_upload = False

# Security tokens are not actually a feature of Google Storage
def _ex_connection_class_kwargs(self):

This comment has been minimized.

@Kami

Kami Jan 16, 2014 Member

Yeah, previously the S3 driver didn't contain any Amazon S3-only functionality so it worked fine with Google Storage and others S3 compatible providers, but going forward, we should refactor the standard S3 functionality into a separate class (we do a similar thing for the EC2 driver).

This means we should have a class hierarchy similar to this one:

  • BaseS3StorageDriver - Base class which implements standard S3 functionality and is never instantiated directly. S3StorageDriver and other S3-compatible drivers should inherit from it.
  • S3StorageDriver - This class should inherit from BaseS3StorageDriver and implement Amazon S3 specific functionality.
  • GoogleStorageDriver - This class should inherit from BaseS3StorageDriver.

We should also do a similar thing for the connection classes.

This comment has been minimized.

@coderanger

coderanger Jan 16, 2014 Author Contributor

Do you want me to do that split as part of this patch or leave that for another day?

This comment has been minimized.

@Kami

Kami Jan 16, 2014 Member

I'd prefer to do this as part of this patch.

If you are busy, you can tackle other comments and I can do the class refactoring once other comments are addressed.

@@ -106,10 +106,16 @@ def parse_error(self):


class SignedAWSConnection(ConnectionUserAndKey):
def __init__(self, *args, **kwargs):

This comment has been minimized.

@Kami

Kami Jan 16, 2014 Member

I would prefer to explicitly list all the arguments here in other places where you have used *args and **kwargs.

Using *args and **kwargs makes programmatic introspection very painful and means you need to rely on docstring for arguments which aren't explicitly declared in the method signature.

@coderanger
Copy link
Contributor Author

coderanger commented Jan 16, 2014

The headers passed in there don't seem to contain any auth-related parameters.

(Pdb) headers
{'Host': 's3.amazonaws.com', 'Accept-Encoding': 'gzip,deflate', 'User-Agent': 'libcloud/0.14.0-beta3 (Amazon S3 (standard)) '}
@Kami
Copy link
Member

Kami commented Jan 16, 2014

@coderanger Oh, right, I totally missed this out. Rest of the values get sent as part of query parameters and not via headers.

This means the code needs to be updated to send X-Amz-Security-Token value via headers and not via query parameters. We also need to test this and make sure it works if you send Signature, AWSAccessKeyId, etc. via query parameter and X-Amz-Security-Token via headers.

Edit: While we are at it, it would also be good to check it they might support sending this value via query parameters as well (I quickly glanced over the documentation and I could see this value being sent via headers).

@coderanger
Copy link
Contributor Author

coderanger commented Jan 16, 2014

Still need to double check that Amazon actually works with this style of sending the token, will do that in a moment.

coderanger added 4 commits Jan 16, 2014
…ers under the hood.

This will behave correctly with restricted IAM permissions.
coderanger added a commit to coderanger/depot that referenced this pull request Jan 17, 2014
def add_default_params(self, params):
# Even though we are adding it to the headers, we need it here too
# so that the token is added to the signature.
if self.token:

This comment has been minimized.

@Kami

Kami Jan 17, 2014 Member

Hmm, are you sure that the token also needs to be taken into account when calculating the signature?

From the documentation (http://docs.aws.amazon.com/STS/latest/UsingSTS/using-temp-creds.html):

"Include the IAM session token that is part of the temporary security credentials. You include the session token as an authorization header to the request—for example, as the X-Amz-Security-Token header. (The session token is not part of the information that's used to create the signature.)"

This comment has been minimized.

@coderanger

coderanger Jan 17, 2014 Author Contributor

Yes, not because it has special meaning but because you need to sign either all params or all x-amz headers (depending on the type of request).

This comment has been minimized.

@Kami

Kami Jan 17, 2014 Member

I see.

Did you test the whole patch with a live installation yet?

In any case, the patch looks good to me. I'll go ahead and test it and if everything looks good, I'll go ahead and merge it.

This comment has been minimized.

@coderanger

coderanger Jan 17, 2014 Author Contributor

Yep, tested via depot with both normal and temporary credentials.

raise ContainerDoesNotExistError(value=None, driver=self,
container_name=container_name)
except InvalidCredsError:
# This just means the user doesn't have IAM permissions to do a

This comment has been minimized.

@Kami

Kami Jan 17, 2014 Member

I assume that S3 still returns 401 if you don't have the necessary permissions and the container doesn't exist?

If that is indeed the case, I wonder if just throwing here might be better / more correct.

This comment has been minimized.

@coderanger

coderanger Jan 17, 2014 Author Contributor

No, it returns a 404 no matter what your permissions are. You only get 401 if it exists and you don't have perms.

This comment has been minimized.

@Kami

Kami Jan 17, 2014 Member

Alright, in this case we should be all set.

@Kami
Copy link
Member

Kami commented Jan 17, 2014

Alright, I've squashed the commits and merged changes into trunk - thanks!

Tomorrow, I also plan to update documentation with some examples of how to use this new functionality.

@asfgit asfgit closed this in 701de69 Jan 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.