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

RFC for AWS SignatureVersion 4 for EC2 #444

Closed
wants to merge 8 commits into
base: trunk
from

Conversation

Projects
None yet
3 participants
@gertjanol
Contributor

gertjanol commented Feb 2, 2015

As shortly discussed with @Kami in #407, this patch adds support for Signature Version 4 to the SignedAWSConnection class. v4 will be used when a Driver-class selects EC2V4Connection as its connectionCls. In this patch this is only enabled for the the new Frankfurt region, represented by a new EC2EUCentralNodeDriver class.
Haven't created any tests yet, since this is just the PoC. If this has any chance of getting merged in, I'll do the tests.

I would like some feedback and thoughts on this work.
Some questions:

  • Is this the way to go, with the Connection-classes specifying a version, and the Driver classes specifying the Connection class to use?
  • I've chosen the pre_connect_hook to do the work, because we need both the headers and the parameters to calculate the signature. Hope that's ok?
  • I’ve tested all regions for EC2 and all seem to work fine with the v4 authentication. I haven’t tested the only other part that uses the SignedAWSConnection, the ELBConnection, but I’m guessing that that also supports V4. It would reduce complexity if we could drop support for V2 in the SignedAWSConnection altogether.
  • The credential scope, as explained here needs the service we are requesting. This should be just ‘ec2’ for compute service. I found no existing way to retrieve this, so I introduced a new class variable in the EC2Connection. I don’t like this, so maybe there is a better way?
  • This patch now only works for GET-requests, but the EC2-driver doesn't use anything else. I think S3 uses other methods, but seems to have its own authentication routines. Why is that? Should I take other methods into consideration here?
  • Performance of calculating this signature sucks :(. Haven't done any measuring, but v2 seems way faster.
@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Feb 2, 2015

Member

Great, thanks - I will look asap.

Member

Kami commented Feb 2, 2015

Great, thanks - I will look asap.

Show outdated Hide outdated libcloud/common/aws.py
# For self.method == GET
request_params = '&'.join(["%s=%s" % (urlquote(k, safe=''), urlquote(v, safe='-_~'))
for k, v in sorted(params.items())])
payload_hash = hashlib.sha256('').hexdigest()

This comment has been minimized.

@Kami

Kami Feb 2, 2015

Member

Hm, is this correct? It looks like it's just calculating SHA256 hash of an empty string.

@Kami

Kami Feb 2, 2015

Member

Hm, is this correct? It looks like it's just calculating SHA256 hash of an empty string.

This comment has been minimized.

@gertjanol

gertjanol Feb 2, 2015

Contributor

Yeah, one of the reasons this patch only works for GET-requests. This should be a hash of the payload of the request, which for GET requests is empty. See http://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html, step 6.

@gertjanol

gertjanol Feb 2, 2015

Contributor

Yeah, one of the reasons this patch only works for GET-requests. This should be a hash of the payload of the request, which for GET requests is empty. See http://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html, step 6.

@gertjanol

This comment has been minimized.

Show comment
Hide comment
@gertjanol

gertjanol Feb 11, 2015

Contributor

@Kami what do you think of this? I'm happy to improve this work, but would like some feedback first.

Contributor

gertjanol commented Feb 11, 2015

@Kami what do you think of this? I'm happy to improve this work, but would like some feedback first.

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Feb 11, 2015

Member

@gertjanol So far it looks fine, but I didn't have time to test it end to end yet.

Member

Kami commented Feb 11, 2015

@gertjanol So far it looks fine, but I didn't have time to test it end to end yet.

return params
def pre_connect_hook(self, params, headers):
now = datetime.utcnow()

This comment has been minimized.

@allardhoeve

allardhoeve Mar 6, 2015

Contributor

datetime.utcnow returns a naive datetime. You should use datetime.now(tzinfo=libcloud.utils.uso8601.UTC).

https://github.com/apache/libcloud/blob/trunk/libcloud/utils/iso8601.py#L65

@allardhoeve

allardhoeve Mar 6, 2015

Contributor

datetime.utcnow returns a naive datetime. You should use datetime.now(tzinfo=libcloud.utils.uso8601.UTC).

https://github.com/apache/libcloud/blob/trunk/libcloud/utils/iso8601.py#L65

This comment has been minimized.

@allardhoeve

allardhoeve Mar 6, 2015

Contributor

But as this currently works, I deduce that AWS is not that strict about this header 😉

@allardhoeve

allardhoeve Mar 6, 2015

Contributor

But as this currently works, I deduce that AWS is not that strict about this header 😉

def _get_authorization_v4_header(self, params, headers, dt):
assert self.method == 'GET', 'AWS Signature V4 not implemented for ' \
'other methods than GET'

This comment has been minimized.

@allardhoeve

allardhoeve Mar 6, 2015

Contributor

Please document what a future committer would have to change to get this working. I assume writing code to serialize the POST data?

@allardhoeve

allardhoeve Mar 6, 2015

Contributor

Please document what a future committer would have to change to get this working. I assume writing code to serialize the POST data?

@@ -178,6 +180,97 @@ def _get_aws_auth_param(self, params, secret_key, path='/'):
return b64_hmac.decode('utf-8')
class V4SignedAWSConnection(AWSTokenConnection):

This comment has been minimized.

@allardhoeve

allardhoeve Mar 6, 2015

Contributor

We write a docstring and link to the AWS reference code you used to implement this for (future) reference.

@allardhoeve

allardhoeve Mar 6, 2015

Contributor

We write a docstring and link to the AWS reference code you used to implement this for (future) reference.

@allardhoeve

This comment has been minimized.

Show comment
Hide comment
@allardhoeve

allardhoeve Mar 6, 2015

Contributor

I'm loving it, even though we're only field-tested for EC2. But, that's exactly why there is only the EC2Connection class and no others. 👍

@Kami?

Contributor

allardhoeve commented Mar 6, 2015

I'm loving it, even though we're only field-tested for EC2. But, that's exactly why there is only the EC2Connection class and no others. 👍

@Kami?

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Mar 6, 2015

Member

@allardhoeve You tested it with the new Frankfurt region?

Member

Kami commented Mar 6, 2015

@allardhoeve You tested it with the new Frankfurt region?

@gertjanol

This comment has been minimized.

Show comment
Hide comment
@gertjanol

gertjanol Mar 6, 2015

Contributor

@Kami we're using this right now for both Frankfurt and Ireland region (@allardhoeve and me == co-workers :))

Contributor

gertjanol commented Mar 6, 2015

@Kami we're using this right now for both Frankfurt and Ireland region (@allardhoeve and me == co-workers :))

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Mar 6, 2015

Member

Cool - I will also look into it (and test it) myself and try to get it merged ASAP.

Member

Kami commented Mar 6, 2015

Cool - I will also look into it (and test it) myself and try to get it merged ASAP.

@@ -5545,6 +5586,15 @@ class EC2EUNodeDriver(EC2NodeDriver):
_region = 'eu-west-1'
class EC2EUCentralNodeDriver(EC2NodeDriver):

This comment has been minimized.

@Kami

Kami Mar 6, 2015

Member

I just noticed an issue while testing this change - this code will only use signature v4 if you are using old and deprecated class based approach. It won't use it if you use the region argument.

EC2 = get_driver(Provider.EC2)
driver = EC2('...', '...', region='eu-central-1')
 -------- begin 140518062231056 request ----------
curl -i -X GET -H 'Host: ec2.eu-central-1.amazonaws.com' -H 'X-LC-Request-ID: 140518062231056' -H 'Accept-Encoding: gzip,deflate' -H 'User-Agent: libcloud/0.17.1-dev (Amazon EC2) ' --compress 'https://ec2.eu-central-1.amazonaws.com:443/?SignatureVersion=2&AWS...'

I will work on a fix.

@Kami

Kami Mar 6, 2015

Member

I just noticed an issue while testing this change - this code will only use signature v4 if you are using old and deprecated class based approach. It won't use it if you use the region argument.

EC2 = get_driver(Provider.EC2)
driver = EC2('...', '...', region='eu-central-1')
 -------- begin 140518062231056 request ----------
curl -i -X GET -H 'Host: ec2.eu-central-1.amazonaws.com' -H 'X-LC-Request-ID: 140518062231056' -H 'Accept-Encoding: gzip,deflate' -H 'User-Agent: libcloud/0.17.1-dev (Amazon EC2) ' --compress 'https://ec2.eu-central-1.amazonaws.com:443/?SignatureVersion=2&AWS...'

I will work on a fix.

This comment has been minimized.

@Kami

Kami Mar 6, 2015

Member

I will probably do something along those lines:

  1. Modify SignedAWSConnection class to take signature_version argument in the constructor and default it to 2.
  2. Modify _ex_connection_class_kwargs method to pass the correct version to the connection class constructor in case the region is eu-central-1
  3. Remove the new `EC2EUCentralNodeDriver``class since the class based approach has been deprecated quite a while ago now (see https://libcloud.readthedocs.org/en/latest/upgrade_notes.html#amazon-ec2-compute-driver-changes).

@gertjanol Will change nr. 3 affect you? Do you use a class per region approach?

@Kami

Kami Mar 6, 2015

Member

I will probably do something along those lines:

  1. Modify SignedAWSConnection class to take signature_version argument in the constructor and default it to 2.
  2. Modify _ex_connection_class_kwargs method to pass the correct version to the connection class constructor in case the region is eu-central-1
  3. Remove the new `EC2EUCentralNodeDriver``class since the class based approach has been deprecated quite a while ago now (see https://libcloud.readthedocs.org/en/latest/upgrade_notes.html#amazon-ec2-compute-driver-changes).

@gertjanol Will change nr. 3 affect you? Do you use a class per region approach?

@Kami Kami referenced this pull request Mar 7, 2015

Merged

Support for AWS signature algorithm v4 (continuation) #481

4 of 4 tasks complete

@asfgit asfgit closed this in 882fdf8 Mar 28, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment