From f5e422eb7858f789066a20b7718a316ea951659f Mon Sep 17 00:00:00 2001
From: Tim Burke <tim.burke@gmail.com>
Date: Mon, 12 Feb 2024 22:06:12 -0800
Subject: [PATCH] s3api: Improve checksum-mismatch detection
Previously, we relied on raising a swob.HTTPException from within a
wrapped wsgi.input and just hoping that it either
- comes uncaught all the way back through the pipeline, or at least
- gets translated back to a 422 response.
This was brittle, however; if any middleware between s3api and the
proxy took s3api's approach of turning unexpected responses into 500s,
for example, it all breaks down.
Take a cue from eventlet's Timeout and create a new s3api-specific
BaseException (which does *not* inherit from Exception) to improve
our ability to cut through all the layers.
Change-Id: I9924ff3b8d7d246631fe61b916823e028e2c01f2
---
swift/common/middleware/s3api/s3request.py | 32 +++++++++++--------
test/unit/common/middleware/s3api/test_obj.py | 12 +++++++
.../common/middleware/s3api/test_s3request.py | 19 ++++++-----
3 files changed, 39 insertions(+), 24 deletions(-)
diff --git a/swift/common/middleware/s3api/s3request.py b/swift/common/middleware/s3api/s3request.py
index 9050745a39..a2f11d900d 100644
--- a/swift/common/middleware/s3api/s3request.py
+++ b/swift/common/middleware/s3api/s3request.py
@@ -120,6 +120,17 @@ def _header_acl_property(resource):
doc='Get and set the %s acl property' % resource)
+class S3InputSHA256Mismatch(BaseException):
+ """
+ Client provided a X-Amz-Content-SHA256, but it doesn't match the data.
+
+ Inherit from BaseException (rather than Exception) so it cuts from the
+ proxy-server app (which will presumably be the one reading the input)
+ through all the layers of the pipeline back to us. It should never escape
+ the s3api middleware.
+ """
+
+
class HashingInput(object):
"""
wsgi.input wrapper to verify the hash of the input as it's read.
@@ -141,7 +152,7 @@ class HashingInput(object):
self._hasher.hexdigest() != self._expected):
self.close()
# Since we don't return the last chunk, the PUT never completes
- raise swob.HTTPUnprocessableEntity(
+ raise S3InputSHA256Mismatch(
'The X-Amz-Content-SHA56 you specified did not match '
'what we received.')
return chunk
@@ -910,13 +921,8 @@ class S3Request(swob.Request):
# Limit the read similar to how SLO handles manifests
try:
body = self.body_file.read(max_length)
- except swob.HTTPException as err:
- if err.status_int == HTTP_UNPROCESSABLE_ENTITY:
- # Special case for HashingInput check
- raise BadDigest(
- 'The X-Amz-Content-SHA56 you specified did not '
- 'match what we received.')
- raise
+ except S3InputSHA256Mismatch as err:
+ raise BadDigest(err.args[0])
else:
# No (or zero) Content-Length provided, and not chunked transfer;
# no body. Assume zero-length, and enforce a required body below.
@@ -1410,13 +1416,11 @@ class S3Request(swob.Request):
try:
sw_resp = sw_req.get_response(app)
- except swob.HTTPException as err:
- # Maybe a 422 from HashingInput? Put something in
- # s3api.backend_path - hopefully by now any modifications to the
- # path (e.g. tenant to account translation) will have been made by
- # auth middleware
+ except S3InputSHA256Mismatch as err:
+ # hopefully by now any modifications to the path (e.g. tenant to
+ # account translation) will have been made by auth middleware
self.environ['s3api.backend_path'] = sw_req.environ['PATH_INFO']
- sw_resp = err
+ raise BadDigest(err.args[0])
else:
# reuse account
_, self.account, _ = split_path(sw_resp.environ['PATH_INFO'],
diff --git a/test/unit/common/middleware/s3api/test_obj.py b/test/unit/common/middleware/s3api/test_obj.py
index 617322d4c4..e186e9c416 100644
--- a/test/unit/common/middleware/s3api/test_obj.py
+++ b/test/unit/common/middleware/s3api/test_obj.py
@@ -770,6 +770,18 @@ class BaseS3ApiObj(object):
req.environ.get('swift.backend_path'))
def test_object_PUT_v4_bad_hash(self):
+ orig_app = self.s3api.app
+
+ def error_catching_app(env, start_response):
+ try:
+ return orig_app(env, start_response)
+ except Exception:
+ self.logger.exception('uh oh')
+ start_response('599 Uh Oh', [])
+ return [b'']
+
+ self.s3api.app = error_catching_app
+
req = Request.blank(
'/bucket/object',
environ={'REQUEST_METHOD': 'PUT'},
diff --git a/test/unit/common/middleware/s3api/test_s3request.py b/test/unit/common/middleware/s3api/test_s3request.py
index 16c359d66c..344e4d7ce3 100644
--- a/test/unit/common/middleware/s3api/test_s3request.py
+++ b/test/unit/common/middleware/s3api/test_s3request.py
@@ -29,7 +29,8 @@ from swift.common.middleware.s3api.subresource import ACL, User, Owner, \
Grant, encode_acl
from test.unit.common.middleware.s3api.test_s3api import S3ApiTestCase
from swift.common.middleware.s3api.s3request import S3Request, \
- S3AclRequest, SigV4Request, SIGV4_X_AMZ_DATE_FORMAT, HashingInput
+ S3AclRequest, SigV4Request, SIGV4_X_AMZ_DATE_FORMAT, HashingInput, \
+ S3InputSHA256Mismatch
from swift.common.middleware.s3api.s3response import InvalidArgument, \
NoSuchBucket, InternalError, ServiceUnavailable, \
AccessDenied, SignatureDoesNotMatch, RequestTimeTooSkewed, BadDigest, \
@@ -1405,9 +1406,11 @@ class TestHashingInput(S3ApiTestCase):
self.assertEqual(b'1234', wrapped.read(4))
self.assertEqual(b'56', wrapped.read(2))
# even though the hash matches, there was more data than we expected
- with self.assertRaises(swob.HTTPException) as raised:
+ with self.assertRaises(S3InputSHA256Mismatch) as raised:
wrapped.read(3)
- self.assertEqual(raised.exception.status, '422 Unprocessable Entity')
+ self.assertIsInstance(raised.exception, BaseException)
+ # won't get caught by most things in a pipeline
+ self.assertNotIsInstance(raised.exception, Exception)
# the error causes us to close the input
self.assertTrue(wrapped._input.closed)
@@ -1419,9 +1422,8 @@ class TestHashingInput(S3ApiTestCase):
self.assertEqual(b'1234', wrapped.read(4))
self.assertEqual(b'56', wrapped.read(2))
# even though the hash matches, there was more data than we expected
- with self.assertRaises(swob.HTTPException) as raised:
+ with self.assertRaises(S3InputSHA256Mismatch):
wrapped.read(4)
- self.assertEqual(raised.exception.status, '422 Unprocessable Entity')
self.assertTrue(wrapped._input.closed)
def test_bad_hash(self):
@@ -1431,17 +1433,14 @@ class TestHashingInput(S3ApiTestCase):
md5(raw, usedforsecurity=False).hexdigest())
self.assertEqual(b'1234', wrapped.read(4))
self.assertEqual(b'5678', wrapped.read(4))
- with self.assertRaises(swob.HTTPException) as raised:
+ with self.assertRaises(S3InputSHA256Mismatch):
wrapped.read(4)
- self.assertEqual(raised.exception.status, '422 Unprocessable Entity')
self.assertTrue(wrapped._input.closed)
def test_empty_bad_hash(self):
wrapped = HashingInput(BytesIO(b''), 0, hashlib.sha256, 'nope')
- with self.assertRaises(swob.HTTPException) as raised:
+ with self.assertRaises(S3InputSHA256Mismatch):
wrapped.read(3)
- self.assertEqual(raised.exception.status, '422 Unprocessable Entity')
- # the error causes us to close the input
self.assertTrue(wrapped._input.closed)