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 AWS4 support in the S3_RGW_OUTSCALE provider #736

Closed
wants to merge 1 commit into
base: trunk
from

Conversation

Projects
None yet
3 participants
@jmunhoz
Contributor

jmunhoz commented Apr 4, 2016

This patch adds AWS4 auth protocol support in the S3_RGW_OUTSCALE
provider. It is needed to use AWS4 with the upcoming version of Ceph RGW
(Jewel)

Ceph Jewel will ship with AWS2 and AWS4 enabled by default.

In the case of regions and signature binding, Ceph does not enforce any
signature version per region. Every region supports AWS2 and AWS4. Ceph
detects the signature version per request in order to authenticate
properly.

More information on the Ceph's AWS4 implementation:

http://docs.ceph.com/docs/master/release-notes/#v10-1-0-jewel-release-candidate
http://blogs.igalia.com/jmunhoz/blog/2016/03/01/aws-signature-version-4-goes-upstream-in-ceph.html

@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Apr 5, 2016

Contributor

Hey @jmunhoz I remember the AWS4 auth algorithm works with GET and POST, but I'm not so sure about PUT. Can you please check that you can upload files using these changes

Contributor

tonybaloney commented Apr 5, 2016

Hey @jmunhoz I remember the AWS4 auth algorithm works with GET and POST, but I'm not so sure about PUT. Can you please check that you can upload files using these changes

@jmunhoz

This comment has been minimized.

Show comment
Hide comment
@jmunhoz

jmunhoz Apr 6, 2016

Contributor

@tonybaloney I would expect S3RGWOutscaleConnectionAWS4 (https://github.com/jmunhoz/libcloud/blob/s3-rgw-outscale-aws4/libcloud/storage/drivers/s3.py#L1021) having the same behaviour as S3APNE2Connection (https://github.com/jmunhoz/libcloud/blob/s3-rgw-outscale-aws4/libcloud/storage/drivers/s3.py#L987). I ran a smoke test listing objects (GET) and the testing suite. They were ok.

As suggested I will check the PUT case explicitly ASAP. Thanks!

Contributor

jmunhoz commented Apr 6, 2016

@tonybaloney I would expect S3RGWOutscaleConnectionAWS4 (https://github.com/jmunhoz/libcloud/blob/s3-rgw-outscale-aws4/libcloud/storage/drivers/s3.py#L1021) having the same behaviour as S3APNE2Connection (https://github.com/jmunhoz/libcloud/blob/s3-rgw-outscale-aws4/libcloud/storage/drivers/s3.py#L987). I ran a smoke test listing objects (GET) and the testing suite. They were ok.

As suggested I will check the PUT case explicitly ASAP. Thanks!

@jmunhoz

This comment has been minimized.

Show comment
Hide comment
@jmunhoz

jmunhoz May 1, 2016

Contributor

@tonybaloney I reviewed the PUT case. The Libcloud code required one minor fix to handle the different behaviour related to ETags in the Amazon S3 and Ceph RGW implementations. Ceph RGW is not shipping ETag headers in this case so the code needs to handle the case where ETags don't exist.

BTW, Ceph RGW was not handling the case for unsigned payloads in the PUT method so I pushed this support in Ceph and it is upstream now with Jewel 10.2.0

The PUT method is working now. Tests are passing. The header dumps for Amazon S3 and Ceph RGW using the new patch follow...

Amazon S3
---------
PUT /my-container-12345678901234/my-name-12345678901234 HTTP/1.1
X-AMZ-Content-SHA256: UNSIGNED-PAYLOAD
Content-Length: 12
Accept-Encoding: gzip,deflate
X-AMZ-Date: 20160501T164719Z
x-amz-storage-class: STANDARD
Content-Type: text/plain
Host: s3.eu-central-1.amazonaws.com
Authorization: AWS4-HMAC-SHA256 Credential=12345678901234567890/20160502/eu-central-1/s3/aws4_request, SignedHeaders=accept-encoding;content-length;content-type;host;user-agent;x-amz-content-sha256;x-amz-date;x-amz-storage-class, Signature=3200c1e45af4ef5acde510877d1b78541337b7c90f420012d694a04a13cebcd5
User-Agent: libcloud/1.0.0-rc2 (OUTSCALE Ceph RGW S3 (eu-central-1)) 
Hello test!
HTTP/1.1 200 OK
x-amz-id-2: FiEe9rH1Rmgb7+palW18yvo6cVNt1TE90UbLwC3syxRiu0DNuDWMwzGSZMQT0ao7srkGyqOmQi4=
x-amz-request-id: CC7007FFB4A71C44
Date: Sun, 01 May 2016 16:47:20 GMT
ETag: "8ac91dd17dd64e95271601b773859aaa"
Content-Length: 0
Server: AmazonS3
Ceph RGW
---------
PUT /my-container-12345678901234/my-name-12345678901234 HTTP/1.1
X-AMZ-Content-SHA256: UNSIGNED-PAYLOAD
Content-Length: 12
Accept-Encoding: gzip,deflate
X-AMZ-Date: 20160501T185440Z
x-amz-storage-class: STANDARD
Content-Type: text/plain
Host: osu.eu-west-1.outscale.com
Authorization: AWS4-HMAC-SHA256 Credential=12345678901234567890/20160502/eu-west-1/s3/aws4_request, SignedHeaders=accept-encoding;content-length;content-type;host;user-agent;x-amz-content-sha256;x-amz-date;x-amz-storage-class, Signature=fb7503d5014ce581723d9695a1395bb668a3a5fcee713663de3d7a316d9b4b27
User-Agent: libcloud/1.0.0-rc2 (OUTSCALE Ceph RGW S3 (eu-west-1)) 
Hello test!
HTTP/1.1 200 OK
ETag: "8ac91dd17dd64e95271601b773859aaa"
Content-Length: 0
Accept-Ranges: bytes
x-amz-request-id: tx000000000000000000012-00572650f0-1017-default
Date: Sun, 01 May 2016 18:54:40 GMT
Contributor

jmunhoz commented May 1, 2016

@tonybaloney I reviewed the PUT case. The Libcloud code required one minor fix to handle the different behaviour related to ETags in the Amazon S3 and Ceph RGW implementations. Ceph RGW is not shipping ETag headers in this case so the code needs to handle the case where ETags don't exist.

BTW, Ceph RGW was not handling the case for unsigned payloads in the PUT method so I pushed this support in Ceph and it is upstream now with Jewel 10.2.0

The PUT method is working now. Tests are passing. The header dumps for Amazon S3 and Ceph RGW using the new patch follow...

Amazon S3
---------
PUT /my-container-12345678901234/my-name-12345678901234 HTTP/1.1
X-AMZ-Content-SHA256: UNSIGNED-PAYLOAD
Content-Length: 12
Accept-Encoding: gzip,deflate
X-AMZ-Date: 20160501T164719Z
x-amz-storage-class: STANDARD
Content-Type: text/plain
Host: s3.eu-central-1.amazonaws.com
Authorization: AWS4-HMAC-SHA256 Credential=12345678901234567890/20160502/eu-central-1/s3/aws4_request, SignedHeaders=accept-encoding;content-length;content-type;host;user-agent;x-amz-content-sha256;x-amz-date;x-amz-storage-class, Signature=3200c1e45af4ef5acde510877d1b78541337b7c90f420012d694a04a13cebcd5
User-Agent: libcloud/1.0.0-rc2 (OUTSCALE Ceph RGW S3 (eu-central-1)) 
Hello test!
HTTP/1.1 200 OK
x-amz-id-2: FiEe9rH1Rmgb7+palW18yvo6cVNt1TE90UbLwC3syxRiu0DNuDWMwzGSZMQT0ao7srkGyqOmQi4=
x-amz-request-id: CC7007FFB4A71C44
Date: Sun, 01 May 2016 16:47:20 GMT
ETag: "8ac91dd17dd64e95271601b773859aaa"
Content-Length: 0
Server: AmazonS3
Ceph RGW
---------
PUT /my-container-12345678901234/my-name-12345678901234 HTTP/1.1
X-AMZ-Content-SHA256: UNSIGNED-PAYLOAD
Content-Length: 12
Accept-Encoding: gzip,deflate
X-AMZ-Date: 20160501T185440Z
x-amz-storage-class: STANDARD
Content-Type: text/plain
Host: osu.eu-west-1.outscale.com
Authorization: AWS4-HMAC-SHA256 Credential=12345678901234567890/20160502/eu-west-1/s3/aws4_request, SignedHeaders=accept-encoding;content-length;content-type;host;user-agent;x-amz-content-sha256;x-amz-date;x-amz-storage-class, Signature=fb7503d5014ce581723d9695a1395bb668a3a5fcee713663de3d7a316d9b4b27
User-Agent: libcloud/1.0.0-rc2 (OUTSCALE Ceph RGW S3 (eu-west-1)) 
Hello test!
HTTP/1.1 200 OK
ETag: "8ac91dd17dd64e95271601b773859aaa"
Content-Length: 0
Accept-Ranges: bytes
x-amz-request-id: tx000000000000000000012-00572650f0-1017-default
Date: Sun, 01 May 2016 18:54:40 GMT
@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney May 2, 2016

Contributor

Great work. LGTM 👍

Contributor

tonybaloney commented May 2, 2016

Great work. LGTM 👍

Show outdated Hide outdated libcloud/storage/drivers/s3.py
def _get_s3_rgw_outscale_conn_by_sign_version(signature_version):
# aws4 connection support
class S3RGWOutscaleConnectionAWS4(SignedAWSConnection, BaseS3Connection):

This comment has been minimized.

@Kami

Kami May 2, 2016

Member

I would prefer to make those classes top-level and define them outside of this function, but that's mostly a style thing.

@Kami

Kami May 2, 2016

Member

I would prefer to make those classes top-level and define them outside of this function, but that's mostly a style thing.

This comment has been minimized.

@jmunhoz

jmunhoz May 2, 2016

Contributor

ok

@jmunhoz

jmunhoz May 2, 2016

Contributor

ok

Show outdated Hide outdated libcloud/storage/drivers/s3.py
@@ -1029,7 +1056,9 @@ def __init__(self, key, secret=None, secure=True, host=None, port=None,
self.name = 'OUTSCALE Ceph RGW S3 (%s)' % (region)
self.ex_location_name = region
self.region_name = region
self.connectionCls = S3RGWOutscaleConnection
signature_version = kwargs.get('signature_version', 'aws2')
self.connectionCls =\

This comment has been minimized.

@Kami

Kami May 2, 2016

Member

Since connectionCls is a class-level attribute (and not an instance one), I believe this won't work correctly if multiple instances of this class are instantiated and each one uses a different signature version.

For example:

cls1 = S3RGWOutscaleStorageDriver(..., signature_version='aws2')
cls2 = S3RGWOutscaleStorageDriver(..., signature_version='aws4')

The version of the class which was instantiated last wins so connectionCls of cls1 would also be set to S3RGWOutscaleConnectionAWS4 after the second class is instantiated.

Another option, which I believe should work correctly, is to utilize ex_connection_class_kwargs method and pass correct value for the version argument to the connectionCls depending on the requested signature version.

@Kami

Kami May 2, 2016

Member

Since connectionCls is a class-level attribute (and not an instance one), I believe this won't work correctly if multiple instances of this class are instantiated and each one uses a different signature version.

For example:

cls1 = S3RGWOutscaleStorageDriver(..., signature_version='aws2')
cls2 = S3RGWOutscaleStorageDriver(..., signature_version='aws4')

The version of the class which was instantiated last wins so connectionCls of cls1 would also be set to S3RGWOutscaleConnectionAWS4 after the second class is instantiated.

Another option, which I believe should work correctly, is to utilize ex_connection_class_kwargs method and pass correct value for the version argument to the connectionCls depending on the requested signature version.

This comment has been minimized.

@Kami

Kami May 2, 2016

Member

Here is an example of how we handle region connection class argument in the same manner - https://github.com/apache/libcloud/blob/trunk/libcloud/compute/drivers/rackspace.py#L142

@Kami

Kami May 2, 2016

Member

Here is an example of how we handle region connection class argument in the same manner - https://github.com/apache/libcloud/blob/trunk/libcloud/compute/drivers/rackspace.py#L142

This comment has been minimized.

@jmunhoz

jmunhoz May 2, 2016

Contributor

I see, thanks!

@jmunhoz

jmunhoz May 2, 2016

Contributor

I see, thanks!

@jmunhoz

This comment has been minimized.

Show comment
Hide comment
@jmunhoz

jmunhoz May 2, 2016

Contributor

@Kami I added the suggested improvements. After some tests I finished adding the dispatching code between the connection classes as part of the _ex_connection_class_kwargs method. It is working with the case you mentioned.

Contributor

jmunhoz commented May 2, 2016

@Kami I added the suggested improvements. After some tests I finished adding the dispatching code between the connection classes as part of the _ex_connection_class_kwargs method. It is working with the case you mentioned.

super(S3RGWOutscaleStorageDriver, self).__init__(key, secret,
secure, host, port,
api_version, region,
**kwargs)
def _ex_connection_class_kwargs(self):

This comment has been minimized.

@Kami

Kami May 10, 2016

Member

With this approach, the same issue exists as before - you need to do something along the lines of:

def _ex_connection_class_kwargs(self):
    kwargs = {}
    kwargs['signature_version'] = self.signature_version
    return kwargs

And you also need to modify connection class to take signature_version argument (or similar) and use this argument to correctly set host attribute on the connection class inside the constructor.

I'm also fine with merging this approach for now, as long as we add some test cases for it (a test case where you instantiate two drivers with different signature versions and verify that the connectionCls and connectionCls.host is set correctly) so it's easy to refactor and fix in the near future.

@Kami

Kami May 10, 2016

Member

With this approach, the same issue exists as before - you need to do something along the lines of:

def _ex_connection_class_kwargs(self):
    kwargs = {}
    kwargs['signature_version'] = self.signature_version
    return kwargs

And you also need to modify connection class to take signature_version argument (or similar) and use this argument to correctly set host attribute on the connection class inside the constructor.

I'm also fine with merging this approach for now, as long as we add some test cases for it (a test case where you instantiate two drivers with different signature versions and verify that the connectionCls and connectionCls.host is set correctly) so it's easy to refactor and fix in the near future.

This comment has been minimized.

@jmunhoz

jmunhoz May 12, 2016

Contributor

@Kami done. Added the modifications. I tested with the shared snippet and it looks okay now.

@jmunhoz

jmunhoz May 12, 2016

Contributor

@Kami done. Added the modifications. I tested with the shared snippet and it looks okay now.

This comment has been minimized.

@Kami

Kami May 13, 2016

Member

@jmunhoz Thanks for the update and sorry to bug you again, but this still won't work correctly.

You are still manipulating connectionCls inside the driver constructor. You should get rid of this code and as mentioned above, update connection class constructor to take signature_version argument and correctly set host inside the connection class constructor.

This _ex_connection_class_kwargs method doesn't actually do anything at this point, it doesn't throw because connection class declares **kwargs. So in short, connection class constructor accepts this argument, but doesn't do anything with it.

In any case, I can go ahead, merge this into master as is and add a (failing) test case so this can be fixed in the future.

@Kami

Kami May 13, 2016

Member

@jmunhoz Thanks for the update and sorry to bug you again, but this still won't work correctly.

You are still manipulating connectionCls inside the driver constructor. You should get rid of this code and as mentioned above, update connection class constructor to take signature_version argument and correctly set host inside the connection class constructor.

This _ex_connection_class_kwargs method doesn't actually do anything at this point, it doesn't throw because connection class declares **kwargs. So in short, connection class constructor accepts this argument, but doesn't do anything with it.

In any case, I can go ahead, merge this into master as is and add a (failing) test case so this can be fixed in the future.

This comment has been minimized.

@jmunhoz

jmunhoz May 13, 2016

Contributor

@Kami Thanks for the review. It is very appreciated.

Yes, please. Feel free to merge. We can continue improving the code over the upstream version. I guess my next patch would build on a similar bug so they could be fixed together.

I guess the example code I used at #736 (comment) is not catching the issue properly although it looks working ok? I am manipulating the connectionCls inside the driver constructor cause it looks the only place to choose between the two different connection classes without a further modification.

@jmunhoz

jmunhoz May 13, 2016

Contributor

@Kami Thanks for the review. It is very appreciated.

Yes, please. Feel free to merge. We can continue improving the code over the upstream version. I guess my next patch would build on a similar bug so they could be fixed together.

I guess the example code I used at #736 (comment) is not catching the issue properly although it looks working ok? I am manipulating the connectionCls inside the driver constructor cause it looks the only place to choose between the two different connection classes without a further modification.

Add AWS4 support in the S3_RGW_OUTSCALE provider
This patch adds AWS4 auth protocol support in the S3_RGW_OUTSCALE
provider. It is needed to use AWS4 with Ceph RGW Jewel.

Ceph Jewel ships with AWS2 and AWS4 enabled by default.

In the case of regions and signature binding, Ceph does not enforce any
signature version per region. Every region supports AWS2 and AWS4. Ceph
detects the signature version per request in order to authenticate
properly.

More information on the Ceph's AWS4 implementation:

http://docs.ceph.com/docs/master/release-notes/#v10-1-0-jewel-release-candidate
http://blogs.igalia.com/jmunhoz/blog/2016/03/01/aws-signature-version-4-goes-upstream-in-ceph.html

Signed-off-by: Javier M. Mellid <jmunhoz@igalia.com>
@jmunhoz

This comment has been minimized.

Show comment
Hide comment
@jmunhoz

jmunhoz May 11, 2016

Contributor

@Kami updated!

I tested the 'last connectionCls wins' issue with the following code:

$ cat ex.py  
#!/usr/bin/env python
from libcloud.storage.types import Provider, ContainerDoesNotExistError
from libcloud.storage.providers import get_driver
import libcloud
container_name = 'container-example'
object_name = 'object-example'
cls = get_driver(Provider.S3_RGW_OUTSCALE)
driver1 = cls('id', 'secret', region='eu-west-1', secure=False, signature_version='4')
driver2 = cls('id', 'secret', region='us-east-2', secure=False, signature_version='2')
print "driver1.connectionCls = %s" % (driver1.connectionCls)
print "driver1.connectionCls.host = %s" %( driver1.connectionCls.host)
print "driver2.connectionCls = %s" % (driver2.connectionCls)
print "driver2.connectionCls.host = %s" %( driver2.connectionCls.host)
$ ./ex.py 
driver1.connectionCls = <class 'libcloud.storage.drivers.s3.S3RGWOutscaleConnectionAWS4'>
driver1.connectionCls.host = osu.eu-west-1.outscale.com
driver2.connectionCls = <class 'libcloud.storage.drivers.s3.S3RGWOutscaleConnectionAWS2'>
driver2.connectionCls.host = osu.us-east-2.outscale.com
Contributor

jmunhoz commented May 11, 2016

@Kami updated!

I tested the 'last connectionCls wins' issue with the following code:

$ cat ex.py  
#!/usr/bin/env python
from libcloud.storage.types import Provider, ContainerDoesNotExistError
from libcloud.storage.providers import get_driver
import libcloud
container_name = 'container-example'
object_name = 'object-example'
cls = get_driver(Provider.S3_RGW_OUTSCALE)
driver1 = cls('id', 'secret', region='eu-west-1', secure=False, signature_version='4')
driver2 = cls('id', 'secret', region='us-east-2', secure=False, signature_version='2')
print "driver1.connectionCls = %s" % (driver1.connectionCls)
print "driver1.connectionCls.host = %s" %( driver1.connectionCls.host)
print "driver2.connectionCls = %s" % (driver2.connectionCls)
print "driver2.connectionCls.host = %s" %( driver2.connectionCls.host)
$ ./ex.py 
driver1.connectionCls = <class 'libcloud.storage.drivers.s3.S3RGWOutscaleConnectionAWS4'>
driver1.connectionCls.host = osu.eu-west-1.outscale.com
driver2.connectionCls = <class 'libcloud.storage.drivers.s3.S3RGWOutscaleConnectionAWS2'>
driver2.connectionCls.host = osu.us-east-2.outscale.com
@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami May 13, 2016

Member

Made a small change (09638ed) and merged it into trunk - thanks!

P.S. I forgot to close the PR in the commit message, can you please close this PR?

Member

Kami commented May 13, 2016

Made a small change (09638ed) and merged it into trunk - thanks!

P.S. I forgot to close the PR in the commit message, can you please close this PR?

@jmunhoz

This comment has been minimized.

Show comment
Hide comment
@jmunhoz

jmunhoz May 13, 2016

Contributor

Thanks! closing this PR

Contributor

jmunhoz commented May 13, 2016

Thanks! closing this PR

@jmunhoz jmunhoz closed this May 13, 2016

tonybaloney added a commit to DimensionDataCBUSydney/libcloud that referenced this pull request May 25, 2016

Following fixes were done: Only MX and SRV records have pririoty. Con…
…tent-type for data has to be json. data param of connection request must be string, changed that. Closes #775   Closes #736   Closes #736
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment