From cc74dfef5368a0293badd6fac7b27781f0b014b2 Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Fri, 17 Jun 2016 10:25:14 -0700 Subject: [PATCH] Don't try to read a body if the client didn't send one From RFC 7230 [1]: > The presence of a message body in a request is signaled by a > Content-Length or Transfer-Encoding header field. Thus, if a client sent neither a Content-Length nor a Transfer-Encoding header, we should assume there is no body and never read from wsgi.input. In such a case, return an empty string unless the caller has specified that a body is required. This revealed a whole bunch of edge cases around our XML parsing: * Attempting to PUT bucket ACLs while not actually supplying an ACL should return a MissingSecurityHeader error. * Attempting to perform a multi-delete without a body should return a MissingRequestBody error. * Attempting to complete a multi-part upload without a body should return an InvalidRequest error. [1] https://tools.ietf.org/html/rfc7230#section-3.3 Change-Id: I47ad946cff64a3da16a759a4364e6fe29b000e11 Closes-Bug: 1593870 --- swift3/acl_handlers.py | 3 +-- swift3/controllers/acl.py | 26 +++++++++++-------- swift3/controllers/multi_delete.py | 9 +++++-- swift3/controllers/multi_upload.py | 3 +++ swift3/request.py | 14 +++++----- swift3/test/unit/helpers.py | 23 +++++++++++++++++ swift3/test/unit/test_acl.py | 36 ++++++++++++++++++++++++++ swift3/test/unit/test_bucket.py | 15 +++++++++++ swift3/test/unit/test_multi_delete.py | 35 +++++++++++++++++++++++-- swift3/test/unit/test_multi_upload.py | 37 ++++++++++++++++++++++++++- 10 files changed, 178 insertions(+), 23 deletions(-) diff --git a/swift3/acl_handlers.py b/swift3/acl_handlers.py index c9fcaea5..14dbb9f7 100644 --- a/swift3/acl_handlers.py +++ b/swift3/acl_handlers.py @@ -61,8 +61,7 @@ def get_acl(headers, body, bucket_owner, object_owner=None): if acl is None: # Get acl from request body if possible. if not body: - msg = 'Your request was missing a required header' - raise MissingSecurityHeader(msg, missing_header_name='x-amz-acl') + raise MissingSecurityHeader(missing_header_name='x-amz-acl') try: elem = fromstring(body, ACL.root_tag) acl = ACL.from_elem(elem) diff --git a/swift3/controllers/acl.py b/swift3/controllers/acl.py index ccc09462..7627a6ec 100644 --- a/swift3/controllers/acl.py +++ b/swift3/controllers/acl.py @@ -20,7 +20,7 @@ from swift.common.utils import public from swift3.exception import ACLError from swift3.controllers.base import Controller from swift3.response import HTTPOk, S3NotImplemented, MalformedACLError, \ - UnexpectedContent + UnexpectedContent, MissingSecurityHeader from swift3.etree import Element, SubElement, tostring from swift3.acl_utils import swift_acl_translate, XMLNS_XSI @@ -103,18 +103,24 @@ class AclController(Controller): else: # Handle Bucket ACL xml = req.xml(MAX_ACL_BODY_SIZE) - if 'HTTP_X_AMZ_ACL' in req.environ and xml: + if all(['HTTP_X_AMZ_ACL' in req.environ, xml]): # S3 doesn't allow to give ACL with both ACL header and body. raise UnexpectedContent() - elif xml and 'HTTP_X_AMZ_ACL' not in req.environ: - # We very likely have an XML-based ACL request. - try: - translated_acl = swift_acl_translate(xml, xml=True) - except ACLError: - raise MalformedACLError() + elif not any(['HTTP_X_AMZ_ACL' in req.environ, xml]): + # Both canned ACL header and xml body are missing + raise MissingSecurityHeader(missing_header_name='x-amz-acl') + else: + # correct ACL exists in the request + if xml: + # We very likely have an XML-based ACL request. + # let's try to translate to the request header + try: + translated_acl = swift_acl_translate(xml, xml=True) + except ACLError: + raise MalformedACLError() - for header, acl in translated_acl: - req.headers[header] = acl + for header, acl in translated_acl: + req.headers[header] = acl resp = req.get_response(self.app, 'POST') resp.status = HTTP_OK diff --git a/swift3/controllers/multi_delete.py b/swift3/controllers/multi_delete.py index b1b7f090..8d26432b 100644 --- a/swift3/controllers/multi_delete.py +++ b/swift3/controllers/multi_delete.py @@ -21,7 +21,8 @@ from swift3.controllers.base import Controller, bucket_operation from swift3.etree import Element, SubElement, fromstring, tostring, \ XMLSyntaxError, DocumentInvalid from swift3.response import HTTPOk, S3NotImplemented, NoSuchKey, \ - ErrorResponse, MalformedXML, UserKeyMustBeSpecified, AccessDenied + ErrorResponse, MalformedXML, UserKeyMustBeSpecified, AccessDenied, \ + MissingRequestBodyError from swift3.cfg import CONF from swift3.utils import LOGGER @@ -64,7 +65,11 @@ class MultiObjectDeleteController(Controller): yield key, version try: - xml = req.xml(MAX_MULTI_DELETE_BODY_SIZE, check_md5=True) + xml = req.xml(MAX_MULTI_DELETE_BODY_SIZE) + if not xml: + raise MissingRequestBodyError() + + req.check_md5(xml) elem = fromstring(xml, 'Delete') quiet = elem.find('./Quiet') diff --git a/swift3/controllers/multi_upload.py b/swift3/controllers/multi_upload.py index be30c86c..da1ce28d 100644 --- a/swift3/controllers/multi_upload.py +++ b/swift3/controllers/multi_upload.py @@ -533,6 +533,9 @@ class UploadController(Controller): previous_number = 0 try: xml = req.xml(MAX_COMPLETE_UPLOAD_BODY_SIZE) + if not xml: + raise InvalidRequest(msg='You must specify at least one part') + complete_elem = fromstring(xml, 'CompleteMultipartUpload') for part_elem in complete_elem.iterchildren('Part'): part_number = int(part_elem.find('./PartNumber').text) diff --git a/swift3/request.py b/swift3/request.py index 9248949f..c970eb6c 100644 --- a/swift3/request.py +++ b/swift3/request.py @@ -682,7 +682,7 @@ class Request(swob.Request): """ raise AttributeError("No attribute 'body'") - def xml(self, max_length, check_md5=False): + def xml(self, max_length): """ Similar to swob.Request.body, but it checks the content length before creating a body string. @@ -697,11 +697,13 @@ class Request(swob.Request): if self.message_length() > max_length: raise MalformedXML() - # Limit the read similar to how SLO handles manifests - body = self.body_file.read(max_length) - - if check_md5: - self.check_md5(body) + if te or self.message_length(): + # Limit the read similar to how SLO handles manifests + body = self.body_file.read(max_length) + else: + # No (or zero) Content-Length provided, and not chunked transfer; + # no body. Assume zero-length, and enforce a required body below. + return None return body diff --git a/swift3/test/unit/helpers.py b/swift3/test/unit/helpers.py index 154dc1a3..3e152cb3 100644 --- a/swift3/test/unit/helpers.py +++ b/swift3/test/unit/helpers.py @@ -153,3 +153,26 @@ class FakeSwift(object): def clear_calls(self): del self._calls[:] + + +class UnreadableInput(object): + # Some clients will send neither a Content-Length nor a Transfer-Encoding + # header, which will cause (some versions of?) eventlet to bomb out on + # reads. This class helps us simulate that behavior. + def __init__(self, test_case): + self.calls = 0 + self.test_case = test_case + + def read(self, *a, **kw): + self.calls += 1 + # Calling wsgi.input.read with neither a Content-Length nor + # a Transfer-Encoding header will raise TypeError (See + # https://bugs.launchpad.net/swift3/+bug/1593870 in detail) + # This unreadable class emulates the behavior + raise TypeError + + def __enter__(self): + return self + + def __exit__(self, *args): + self.test_case.assertEqual(0, self.calls) diff --git a/swift3/test/unit/test_acl.py b/swift3/test/unit/test_acl.py index 65047c84..9a7eb009 100644 --- a/swift3/test/unit/test_acl.py +++ b/swift3/test/unit/test_acl.py @@ -15,10 +15,12 @@ import unittest from cStringIO import StringIO +from hashlib import md5 from swift.common.swob import Request, HTTPAccepted from swift3.test.unit import Swift3TestCase +from swift3.test.unit.helpers import UnreadableInput from swift3.etree import fromstring, tostring, Element, SubElement, XMLNS_XSI from swift3.test.unit.test_s3_acl import s3acl import mock @@ -125,6 +127,40 @@ class TestSwift3Acl(Swift3TestCase): self.assertEqual(self._get_error_code(body), 'UnexpectedContent') + def _test_put_no_body(self, use_content_length=False, + use_transfer_encoding=False, string_to_md5=''): + content_md5 = md5(string_to_md5).digest().encode('base64').strip() + with UnreadableInput(self) as fake_input: + req = Request.blank( + '/bucket?acl', + environ={ + 'REQUEST_METHOD': 'PUT', + 'wsgi.input': fake_input}, + headers={ + 'Authorization': 'AWS test:tester:hmac', + 'Date': self.get_date_header(), + 'Content-MD5': content_md5}, + body='') + if not use_content_length: + req.environ.pop('CONTENT_LENGTH') + if use_transfer_encoding: + req.environ['HTTP_TRANSFER_ENCODING'] = 'chunked' + status, headers, body = self.call_swift3(req) + self.assertEqual(status, '400 Bad Request') + self.assertEqual(self._get_error_code(body), 'MissingSecurityHeader') + self.assertEqual(self._get_error_message(body), + 'Your request was missing a required header.') + self.assertIn('x-amz-acl', body) + + @s3acl + def test_bucket_fails_with_neither_acl_header_nor_xml_PUT(self): + self._test_put_no_body() + self._test_put_no_body(string_to_md5='test') + self._test_put_no_body(use_content_length=True) + self._test_put_no_body(use_content_length=True, string_to_md5='test') + self._test_put_no_body(use_transfer_encoding=True) + self._test_put_no_body(use_transfer_encoding=True, string_to_md5='zz') + def test_object_acl_GET(self): req = Request.blank('/bucket/object?acl', environ={'REQUEST_METHOD': 'GET'}, diff --git a/swift3/test/unit/test_bucket.py b/swift3/test/unit/test_bucket.py index 04658ced..aff7d415 100644 --- a/swift3/test/unit/test_bucket.py +++ b/swift3/test/unit/test_bucket.py @@ -21,6 +21,7 @@ from swift.common.swob import Request from swift.common.utils import json from swift3.test.unit import Swift3TestCase +from swift3.test.unit.helpers import UnreadableInput from swift3.etree import Element, SubElement, fromstring, tostring from swift3.test.unit.test_s3_acl import s3acl from swift3.subresource import Owner, encode_acl, ACLPublicRead @@ -501,6 +502,7 @@ class TestSwift3Bucket(Swift3TestCase): headers={'Authorization': 'AWS test:tester:hmac', 'Date': self.get_date_header()}) status, headers, body = self.call_swift3(req) + self.assertEqual(body, '') self.assertEqual(status.split()[0], '200') self.assertEqual(headers['Location'], '/bucket') @@ -512,6 +514,19 @@ class TestSwift3Bucket(Swift3TestCase): 'Date': self.get_date_header(), 'Transfer-Encoding': 'chunked'}) status, headers, body = self.call_swift3(req) + self.assertEqual(body, '') + self.assertEqual(status.split()[0], '200') + self.assertEqual(headers['Location'], '/bucket') + + with UnreadableInput(self) as fake_input: + req = Request.blank( + '/bucket', + environ={'REQUEST_METHOD': 'PUT', + 'wsgi.input': fake_input}, + headers={'Authorization': 'AWS test:tester:hmac', + 'Date': self.get_date_header()}) + status, headers, body = self.call_swift3(req) + self.assertEqual(body, '') self.assertEqual(status.split()[0], '200') self.assertEqual(headers['Location'], '/bucket') diff --git a/swift3/test/unit/test_multi_delete.py b/swift3/test/unit/test_multi_delete.py index abfa6a7d..b17c9cb2 100644 --- a/swift3/test/unit/test_multi_delete.py +++ b/swift3/test/unit/test_multi_delete.py @@ -22,6 +22,7 @@ from swift.common import swob from swift.common.swob import Request from swift3.test.unit import Swift3TestCase +from swift3.test.unit.helpers import UnreadableInput from swift3.etree import fromstring, tostring, Element, SubElement from swift3.cfg import CONF from swift3.test.unit.test_s3_acl import s3acl @@ -77,11 +78,10 @@ class TestSwift3MultiDelete(Swift3TestCase): req = Request.blank('/bucket?delete', environ={'REQUEST_METHOD': 'POST'}, headers={'Authorization': 'AWS test:tester:hmac', + 'Content-Type': 'multipart/form-data', 'Date': self.get_date_header(), 'Content-MD5': content_md5}, body=body) - req.date = datetime.now() - req.content_type = 'text/plain' status, headers, body = self.call_swift3(req) self.assertEqual(status.split()[0], '200') @@ -249,5 +249,36 @@ class TestSwift3MultiDelete(Swift3TestCase): elem = fromstring(body) self.assertEqual(len(elem.findall('Deleted')), len(self.keys)) + def _test_no_body(self, use_content_length=False, + use_transfer_encoding=False, string_to_md5=''): + content_md5 = md5(string_to_md5).digest().encode('base64').strip() + with UnreadableInput(self) as fake_input: + req = Request.blank( + '/bucket?delete', + environ={ + 'REQUEST_METHOD': 'POST', + 'wsgi.input': fake_input}, + headers={ + 'Authorization': 'AWS test:tester:hmac', + 'Date': self.get_date_header(), + 'Content-MD5': content_md5}, + body='') + if not use_content_length: + req.environ.pop('CONTENT_LENGTH') + if use_transfer_encoding: + req.environ['HTTP_TRANSFER_ENCODING'] = 'chunked' + status, headers, body = self.call_swift3(req) + self.assertEqual(status, '400 Bad Request') + self.assertEqual(self._get_error_code(body), 'MissingRequestBodyError') + + @s3acl + def test_object_multi_DELETE_empty_body(self): + self._test_no_body() + self._test_no_body(string_to_md5='test') + self._test_no_body(use_content_length=True) + self._test_no_body(use_content_length=True, string_to_md5='test') + self._test_no_body(use_transfer_encoding=True) + self._test_no_body(use_transfer_encoding=True, string_to_md5='test') + if __name__ == '__main__': unittest.main() diff --git a/swift3/test/unit/test_multi_upload.py b/swift3/test/unit/test_multi_upload.py index 9b1312f5..c05ca578 100644 --- a/swift3/test/unit/test_multi_upload.py +++ b/swift3/test/unit/test_multi_upload.py @@ -14,10 +14,11 @@ # limitations under the License. import base64 +from hashlib import md5 +from mock import patch import os import time import unittest -from mock import patch from urllib import quote from swift.common import swob @@ -25,6 +26,7 @@ from swift.common.swob import Request from swift.common.utils import json from swift3.test.unit import Swift3TestCase +from swift3.test.unit.helpers import UnreadableInput from swift3.etree import fromstring, tostring from swift3.subresource import Owner, Grant, User, ACL, encode_acl, \ decode_acl, ACLPublicRead @@ -1600,6 +1602,39 @@ class TestSwift3MultiUpload(Swift3TestCase): self.assertEqual('bytes=0-9', put_headers['Range']) self.assertEqual('/src_bucket/src_obj', put_headers['X-Copy-From']) + def _test_no_body(self, use_content_length=False, + use_transfer_encoding=False, string_to_md5=''): + content_md5 = md5(string_to_md5).digest().encode('base64').strip() + with UnreadableInput(self) as fake_input: + req = Request.blank( + '/bucket/object?uploadId=X', + environ={ + 'REQUEST_METHOD': 'POST', + 'wsgi.input': fake_input}, + headers={ + 'Authorization': 'AWS test:tester:hmac', + 'Date': self.get_date_header(), + 'Content-MD5': content_md5}, + body='') + if not use_content_length: + req.environ.pop('CONTENT_LENGTH') + if use_transfer_encoding: + req.environ['HTTP_TRANSFER_ENCODING'] = 'chunked' + status, headers, body = self.call_swift3(req) + self.assertEqual(status, '400 Bad Request') + self.assertEqual(self._get_error_code(body), 'InvalidRequest') + self.assertEqual(self._get_error_message(body), + 'You must specify at least one part') + + @s3acl + def test_object_multi_upload_empty_body(self): + self._test_no_body() + self._test_no_body(string_to_md5='test') + self._test_no_body(use_content_length=True) + self._test_no_body(use_content_length=True, string_to_md5='test') + self._test_no_body(use_transfer_encoding=True) + self._test_no_body(use_transfer_encoding=True, string_to_md5='test') + class TestSwift3MultiUploadNonUTC(TestSwift3MultiUpload): def setUp(self):