x/swift3
Code Issues Proposed changes

Don't try to read a body if the client didn't send one

Browse Source 
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
This commit is contained in:
Tim Burke
2016-06-17 10:25:14 -07:00
parent c416369d38
commit cc74dfef53
10 changed files with 178 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
swift3/acl_handlers.py
View File

@@ -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)

12
swift3/controllers/acl.py
View File

@@ -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,11 +103,17 @@ 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:
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:

9
swift3/controllers/multi_delete.py
View File

@@ -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')

3
swift3/controllers/multi_upload.py
View File

@@ -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)

10
swift3/request.py
View File

@@ -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()
if te or self.message_length():
# Limit the read similar to how SLO handles manifests
body = self.body_file.read(max_length)
if check_md5:
self.check_md5(body)
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

23
swift3/test/unit/helpers.py
View File

@@ -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)

36
swift3/test/unit/test_acl.py
View File

@@ -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('<MissingHeaderName>x-amz-acl</MissingHeaderName>', 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'},

15
swift3/test/unit/test_bucket.py
View File

@@ -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')

35
swift3/test/unit/test_multi_delete.py
View File

@@ -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()

37
swift3/test/unit/test_multi_upload.py
View File

@@ -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):