AWS auth v4 #16

mrwsr · 2016-12-29T00:33:51Z

@exarkun Here it is! I believe it is the best evidence in favor of TLS client certificates the world has ever seen.

The new module, _aws_v4_auth, should have 100% test coverage.

In addition to unit tests, its coverage includes a subset of the cases in AWS's Auth V4 test suite. The test suite is rather large so these tests are skipped unless you set the AWS4_TEST_SUITE_PATH environment variable to the unzipped archive. Is there a better way to communicate such a path to trial?

I've also modified the Travis configuration to pull down that archive. We probably don't want to do that because connection issues will make the tests flaky. I'm open to ideas for improvement.

It may be the case that these tests are not very helpful, too; where the canonical request claims it wants a full URI, it actually wants just the path and query string.

I'll fix the merge conflicts later. I didn't want to wait any longer to push this PR.

x-amz-content-sha256 replaces Content-MD5; all header values are converted to strings; unused canonicalization code disappears. Note that providing the full URL for the S3 endpoint will fail to authenticate; the canonical request must instead only include the path. This contravenes AWS' documentation!

exarkun

Thanks!

This looks really good overall. I've made various inline comments but they're all pretty localized issues (and some are opinion/style/optional) that don't affect the big-picture design or implementation at all, as far as I can tell.

I'll be very happy to see this merged with just a little more work.

exarkun · 2016-12-29T13:18:22Z

txaws/s3/client.py

-        headers = {"Content-Length": content_length,
-                   "Date": self.date}
+            content_length = str(len(self.data))
+        headers = {"Content-Length": content_length}


I noticed the int/str length problem in the s3-testing branch yesterday, too. @mithrandi suggested that txAWS may not need to set Content-Length at all. Reading https://github.com/twisted/twisted/blob/trunk/src/twisted/web/_newclient.py#L687-L695 I think he's right. So perhaps this Content-Length code can just go.

... unless there are authv4/signature consequences deriving from the Content-Length header... hopefully not?

Nope! The only headers you have to sign when using V4 auth with S3 are host and whatever the date header is (x-amz-date here). I'll definitely get rid of this!

exarkun · 2016-12-29T13:23:01Z

txaws/s3/client.py

        if not self.endpoint or not self.endpoint.host:
            self.endpoint = AWSServiceEndpoint(S3_ENDPOINT)
        self.endpoint.set_method(self.action)

+    def _utcnow(self):
+        return datetime.datetime.utcnow()


If utcnow (or datetime.datetime) were an __init__ parameter then the tests (and anyone else with a novel use) could just pass in their desired implementation instead of needing to monkey-patch private parts of the implementation.

It might be worth considering whether this should be an IReactorTime instead - since Twisted provides us with twisted.internet.task.Clock for easy monkeying with time.

IReactorTime operates in terms of seconds since the epoch which is a bit more primitive than datetime so I can see the appeal of using datetime instead. Maybe datetime.utcfromtimestamp mitigates that, not sure. I'll leave it up to you.

Or, it seems like what the code really needs is to know the time at the point submit is called. So another solution would be to add an optional argument to Query.submit that gives the time and let the caller worry about how to derive that value. It moves the responsibility from the Query-instantiator to the submit-caller which I'm not sure is the best thing ... but in all the txAWS code I've read, those two things happen right next to each other so it may not actually matter.

+1 for dependency injection!

exarkun · 2016-12-29T13:25:15Z

txaws/s3/client.py

@@ -575,11 +577,13 @@ def __init__(self, bucket=None, object_name=None, data="",
        self.content_type = content_type
        self.metadata = metadata
        self.amz_headers = amz_headers
-        self.date = datetimeToString()


Possibly this incompatible change could break some application code. Maybe date could be preserved as a property computed from the _utctime hook?

OTOH I'm not actually sure what txAWS's compatibility policies are. I see there are some deprecation warnings elsewhere in the codebase. That suggests some attempts at backwards compatibility are being made, I guess.

I copied an existing deprecation pattern. One thing to note about my approach is that date is now useless and inaccurate. This means it's not really a deprecation since it's already wrong to interact with it! However, it's hard to set the time in the initializer when it's passed in, after instantiation, to the submit method. I do think that's the correct design. I'm hoping that developers who find their code broken turn on warnings, and if not, that the clear indication of deprecation leads them on the right path.

exarkun · 2016-12-29T13:48:34Z

txaws/s3/tests/test_client.py

-            headers.get("Content-MD5"), "1B2M2Y8AsgTpgAmY7PhCfg==")
-        self.assertTrue(len(headers.get("Date")) > 25)
+            headers.get("x-amz-content-sha256"),
+            "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855")


I guess this probably isn't one of the Amazon-provided test vectors, right? It's just a random vector that happened to be in the txAWS test suite already.

I hesitate when it comes to tests that include a direct comparison against a cryptographic random blob. Failures are the worst kind of opaque. Not only does the fact that two random hashes aren't equal not tell the maintainer much but the cryptographic properties mean that the goal of the system is for that kind of mismatch to carry no additional information.

It's a difficult case because there really is a cryptographic hash here and we want it to have the right value. That probably means the experience of debugging it when it fails can only be made so good and no better. My best suggestion is that it would be better to make an assertion like:

self.assertEquals( headers.get("x-amz-content-sha256"), sha256(b"").hexdigest(), )

(At least, I think that's the right function and argument for this particular case.) The failure is still two opaque hashes but now the test encodes the knowledge about exactly how the hash should have been computed, giving the maintainer a hint about where to look for a problem and perhaps how to fix it.

At least, I think that's the right function and argument for this particular case.

It is! I was going for consistency with the existing test, and was also trying, to a lesser extent, to avoid doing calculation in the tests. I'm definitely happy to replace it with this, though if avoiding the calculation seems like a good idea, perhaps I can write:

EMPTY_STRING_SHA256 = 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'

exarkun · 2016-12-29T14:01:36Z

.travis.yml


 install:
  - "pip install --upgrade pip setuptools wheel"
  - "pip install .[dev]"

 script:
-  - "trial txaws"
+  - "wget 'https://docs.aws.amazon.com/general/latest/gr/samples/aws4_testsuite.zip'"


I have a bias towards curl. Not sure if it is rational or not.

Perhaps more productively, if you look above a little bit you can see some caching that gets set up for pip. I think we can have travis cache this download (or even the unzipped version of it). If we also have just a little bit of retry logic for failed/interrupted downloads, I think it might actually be fairly reasonable to download it on each travis build.

But on the other hand, this isn't actually a very large archive (a txAWS git checkout is already >7MiB) . It might also be completely acceptable to just add it to the txAWS repository. The license is compatible enough so I don't think there are any problems on that front either.

exarkun · 2016-12-29T14:18:28Z

txaws/_auth_v4.py

+    Construct an AWS version 4 authorization value for use in an
+    C{Authorization} header.
+
+    @param region: The AWS region name (e.g., C{us-east-1}).


C{"us-east-1"} (with the quotes) probably

exarkun · 2016-12-29T14:20:37Z

txaws/tests/test_auth_v4.py

+                       credential_scope=scope)
+
+
+class AWS4FunctionTest(unittest.SynchronousTestCase):


txAWS naming convention seems to be "FooTestCase".

exarkun · 2016-12-29T14:22:46Z

txaws/tests/test_auth_v4.py

+        self.assertEqual(canonical, textwrap.dedent("""\
+        other-signed-header:other-signed-value
+        signed-header:signed-value
+        """))


Another idiom that avoids relying on textwrap.dedent to actually do what it's supposed to do is:

self.assertEqual( canonical, "other-signed-header:other-signed-value\n" "signed-header:signed-value\n" )

Either is fine, just mentioning it.

exarkun · 2016-12-29T14:26:43Z

txaws/tests/test_auth_v4.py

+                         "date stamp/region/service/aws4_request")
+
+
+class _CredentialTests(unittest.SynchronousTestCase):


I typically drop the leading _ when naming the tests for a private API. It's ambiguous about whether it means the class itself is private. It's typically clear what the unit under test is even without the _ and the class docstring spells it out if not.

exarkun · 2016-12-29T14:29:39Z

txaws/tests/test_auth_v4.py

+        self._test_case(self.path.child("get-vanilla-query-order-key-case"))
+
+    def test_get_vanilla_utf8_query(self):
+        self._test_case(self.path.child("get-vanilla-utf8-query"))


If you want, you could probably write a test_suite function that collects all of the cases (using FilePath.children()) and generates a TestSuite that includes tests for all of them.

But the code is also fine as-written. And I'd really like to have the authv4 support merged. So this might be a good follow-up issue. :)

Also, I wonder why you didn't choose to exercise all of the tests from the AWS suite. It looks like there's about 25 of them - why only run this subset of 9?

If you want, you could probably write a test_suite function that collects all of the cases (using FilePath.children()) and generates a TestSuite that includes tests for all of them.

I erred on the side of obvious but repetitious. There are also annoying inconsistencies in the directory layout (e.g., normalize-path and post-sts-token both contain sub-directories with more specific tests). Now that I'm reviewing the tests, though, I'm worried that it's too easy to copy an existing method and rename it, but then forget to change the path name. I'll give myself half an hour to implement a test_suite function and we'll see what ends up in a future commit :)

Also, I wonder why you didn't choose to exercise all of the tests from the AWS suite. It looks like there's about 25 of them - why only run this subset of 9?

Good question! Not all the tests pass because of irrelevant parsing issues. get-header-key-duplicate, for example, fails because my crummy "HTTP" parser doesn't handle duplicate headers. Now, you might think this is the perfect time to use twisted.web.http.HTTPChannel, but the canned HTTP requests are not actually HTTP requests. They don't contain Content-Length headers even when a body exists, so an HTTPChannel instance can never extract it. I didn't want to make the diff even bigger by either reaching into a private HTTPChannel API to grab the suspended body out of some buffer, or by writing a more robust parser that would require its own tests.

On second thought, I can use HTTPChannel to parse the headers and then manually extract the body by looking for a blank line. Will do so and report back!

It gets worse! urllib.urlencode does not implement RFC 3986's required behavior for unreserved characters, quoting, say, ~ to %7E when it should not. This causes get-unreserved to fail. I don't see a way around this other than writing a quoting implementation that does the right thing, making the PR even bigger.

The tests I did choose to run seemed most relevant to the S3 client. But now that I've got a better strategy for parsing the requests, I'll try to get more running in within a second half-hour time limit. Otherwise I'd like to put together a later PR that does so.

…suite. The doc strings say bytes when they mean bytes. Native string manipulation has been replaced in favor of byte string % formatting. This will work in Python 3.5 and later. textwrap has also been removed, because it was applied to bytes and not text. Header canonicalization can now handle multivalued headers, folds multiple lines into one, and normalizes whitespace, all per AWS's requirements. Note that multiple values for a header currently exists only for testing; nothing in txaws actually uses it. The AWS test suite integration has been removed in favor of a different approach that will appear in a later commit.

mrwsr · 2016-12-30T01:32:56Z

txaws/tests/test_aws4_testsuite.py

+_UNRESERVED_SKIP = "urllib.quote quotes unreserved characters"
+_NOT_YET_SUPPORTED_SKIP = "Not yet supported."
+
+_SKIPS = {


@exarkun These 12 tests are skipped while the remaining 19 pass. Later pull requests can whittle this down until it's entirely gone.

mrwsr · 2016-12-30T01:33:30Z

txaws/tests/test_aws4_testsuite.py

+
+    if not methods:
+        empty_tests = test_class()
+        empty_tests.skip = "Missing AWS test suite directory: {}".format(


I can convert this to % formatting!

Twisted's Agent sets this, so we don't need to.

Previously the signature assumed all requests were GETs, leading to signature failures on PUTs, etc. Fix this by including the method.

mrwsr · 2016-12-30T06:35:43Z

@exarkun I think I addressed most, if not all, of your (excellent) review comments. I also fixed a bug that broke at least put_object. Please review again and let me know what else needs to be done.

Thanks!

mrwsr · 2016-12-30T06:38:05Z

Also, the S3 client will no longer authenticate correctly when given a body producer. I'm not sure if that ever worked, though.

exarkun · 2016-12-30T13:01:04Z

If there wasn't a test for it, it's hard to argue it ever worked. :) Offhand I'm not sure which S3 operations require a request body. I'll probably double check that LeastAuthority is depending on that behavior - most likely not until Monday, though (when I'll re-review and hopefully merge!). Thanks again!

mrwsr · 2016-12-30T19:39:11Z

@exarkun Body producers can work so long as they define a length. They won't work right now because there's no way to calculate their SHA 256 hash for inclusion in the x-amz-content-sha256 header.

It turns out that txAWS currently uses a body producer to upload immediate data, wrapping it in a FileBodyProducer. (txAWS ships with its own FileBodyProducer presumably because it predates an era where one shipped with Twisted. We're in such an era now, though, so we should definitely delete the one in txAWS!).

So it's certainly the case that some body producers work. There are two reasons why:

the data was available during the get_headers call, so its SHA-256 hash could be computed and stashed in the x-amz-content-sha256 header. This will not be true for any user-provided body producer.
FileBodyProducers define a length attribute, which means t.w._newclient.Request.writeTo (called by Agent) won't use chunked encoding.

Chunked encoding with auth v4 is a nightmare because each chunk has to be signed via extensions, a feature that basically nothing implements. Twisted certainly doesn't and it would take significant thought to work out how to even communicate this downward.

The good news is that AWS makes chunked encoding basically useless because you have to specify the total length in the Content-Length header! (see note.) If that's available, then Twisted won't use chunked encoding, and if it isn't, you can't do chunked encoding. A very useful Catch-22.

We do need to work out a way for users to communicate the SHA-256 of length-ified body producers. I hope Least Authority's use of the S3 client means this can wait til a later PR. :)

exarkun · 2016-12-31T13:57:51Z

Argh, I hadn't fully appreciated how much of a mess the handling of body producers was here.

Fortunately it looks like LeastAuthority (by way of Tahoe-LAFS) doesn't use body_producer anywhere in its S3 code.

So I think we won't have to worry about those issues for this PR. Someone (maybe me) should file an issue for cleaning up the awful get_page interface and perhaps another for adding some safety features around body_producer (rejecting one without a length outright seems like it might be a good idea, for example).

exarkun

There's a .DS_Store that should be removed, I think. Other than that and the minor inline comments, this looks good now.

I wish I had my S3 integration testing branch merged already but I'll merge master into that as soon as this is merged into master and see how things go. :) Given the passing AWS tests here and the overall level of better-ness it seems clear to me that (modulo trivial remaining comments) this should be merged.

Thanks so much for the contribution!

exarkun · 2016-12-31T13:20:18Z

txaws/_auth_v4.py

+
+    @param instant: A naive UTC L{datetime.datetime} (as returned from
+        L{datetime.datetime.utcnow})
+    @type instant: L{dateimte.datetime}


typo, dateimte.

exarkun · 2016-12-31T13:22:27Z

txaws/_auth_v4.py

+    @type instant: L{dateimte.datetime}
+
+    @return: The formatted date and time.
+    @rtype: L{str}


... bytes? Though strftime returns the native string type... which is exactly what str is. I forget if there's an unambiguous solution here. :/ Maybe something like L{str} (native string).

Note that these are straight from the test suite zip archive! They might need to be removed every time the suite is updated.

mrwsr added 5 commits December 28, 2016 16:02

Auth V4 support code

91980e5

Resolve merge conflict

e8af54e

Replace spurious sha256 utility function with hashlib.

cbd65ca

New and also improved documentation and naming

ae4b5b5

exarkun requested changes Dec 29, 2016

View reviewed changes

mrwsr added 2 commits December 29, 2016 17:25

Include the AWS auth v4 test suite in txaws

0c8e879

mrwsr commented Dec 30, 2016

View reviewed changes

mrwsr added 8 commits December 29, 2016 21:30

Bring back the date attribute, but deprecate it.

7f3a4b9

All attrs classes are frozen; whitespace

dc446f4

Quotes around string constants in docstring.

199f746

Remove content-length header.

f837811

Twisted's Agent sets this, so we don't need to.

Replace opaque blobs with explicit hashing.

c517022

Consistently rename tests to ...TestCase

66c0ffe

Clean up docstrings and imports

a62c12b

Include method in signature.

9b2025b

Previously the signature assumed all requests were GETs, leading to signature failures on PUTs, etc. Fix this by including the method.

Punctation.

8c3edc7

exarkun approved these changes Dec 31, 2016

View reviewed changes

mrwsr added 2 commits December 31, 2016 12:33

Fix typos & clarify string types.

8a232e8

Remove .DS_Store files from AWS test suite.

1e70b5e

Note that these are straight from the test suite zip archive! They might need to be removed every time the suite is updated.

markrwilliams merged commit 8b7973a into twisted:master Jan 2, 2017

exarkun mentioned this pull request Jan 4, 2017

Implement Auth v4 #8

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AWS auth v4 #16

AWS auth v4 #16

mrwsr commented Dec 29, 2016

exarkun left a comment

exarkun Dec 29, 2016

exarkun Dec 29, 2016

mrwsr Dec 29, 2016

exarkun Dec 29, 2016

exarkun Dec 29, 2016

markrwilliams Dec 29, 2016

exarkun Dec 29, 2016

mrwsr Dec 30, 2016 •

edited

Loading

exarkun Dec 29, 2016

mrwsr Dec 29, 2016

exarkun Dec 29, 2016

exarkun Dec 29, 2016

exarkun Dec 29, 2016

exarkun Dec 29, 2016

exarkun Dec 29, 2016

exarkun Dec 29, 2016

exarkun Dec 29, 2016

mrwsr Dec 29, 2016

mrwsr Dec 30, 2016

mrwsr Dec 30, 2016

mrwsr commented Dec 30, 2016

mrwsr commented Dec 30, 2016

exarkun commented Dec 30, 2016

mrwsr commented Dec 30, 2016

exarkun commented Dec 31, 2016

exarkun left a comment

exarkun Dec 31, 2016

exarkun Dec 31, 2016

		credential_scope=scope)


		class AWS4FunctionTest(unittest.SynchronousTestCase):

		"date stamp/region/service/aws4_request")


		class _CredentialTests(unittest.SynchronousTestCase):

AWS auth v4 #16

AWS auth v4 #16

Conversation

mrwsr commented Dec 29, 2016

exarkun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrwsr Dec 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrwsr commented Dec 30, 2016

mrwsr commented Dec 30, 2016

exarkun commented Dec 30, 2016

mrwsr commented Dec 30, 2016

exarkun commented Dec 31, 2016

exarkun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrwsr Dec 30, 2016 •

edited

Loading