Properly strip headers for request signing

Previously, when clients sent non-printable characters in headers, we
might reply with InvalidDigest or some other flavor of 400 while AWS
would have sent a 403 SignatureDoesNotMatch in a similar situation.

See, for example, the removed known failures for ceph/s3-tests.

Additionally, factor out canonical request from string-to-sign for
SigV4Mixin. This simplifies the diagnosing of certain unit test failures.

Change-Id: I703d3db134b8e2202d271eca018b920fbedd08e7

swift3/request.py

@@ -73,6 +73,19 @@ SIGV2_TIMESTAMP_FORMAT = '%Y-%m-%dT%H:%M:%S'
 SIGV4_X_AMZ_DATE_FORMAT = '%Y%m%dT%H%M%SZ'
 
 
+def _header_strip(value):
+    # S3 seems to strip *all* control characters
+    if value is None:
+        return None
+    stripped = _header_strip.re.sub('', value)
+    if value and not stripped:
+        # If there's nothing left after stripping,
+        # behave as though it wasn't provided
+        return None
+    return stripped
+_header_strip.re = re.compile('^[\x00-\x20]*|[\x00-\x20]*$')
+
+
 def _header_acl_property(resource):
     """
     Set and retrieve the acl in self.headers
@@ -236,7 +249,7 @@ class SigV4Mixin(object):
         :return : dict of headers to sign, the keys are all lower case
         """
         headers_lower_dict = dict(
-            (k.lower().strip(), ' '.join((v or '').strip().split()))
+            (k.lower().strip(), ' '.join(_header_strip(v or '').split()))
             for (k, v) in six.iteritems(self.headers))
 
         if 'host' in headers_lower_dict and re.match(
@@ -268,13 +281,7 @@ class SigV4Mixin(object):
         """
         return self.environ.get('RAW_PATH_INFO', self.path)
 
-    def _string_to_sign(self):
+    def _canonical_request(self):
-        """
-        Create 'StringToSign' value in Amazon terminology for v4.
-        """
-        scope = (self.timestamp.amz_date_format.split('T')[0] +
-                 '/' + CONF.location + '/s3/aws4_request')
-
         # prepare 'canonical_request'
         # Example requests are like following:
         #
@@ -323,12 +330,19 @@ class SigV4Mixin(object):
         else:
             hashed_payload = self.headers['X-Amz-Content-SHA256']
         cr.append(hashed_payload)
-        canonical_request = '\n'.join(cr)
+        return '\n'.join(cr).encode('utf-8')
+
+    def _string_to_sign(self):
+        """
+        Create 'StringToSign' value in Amazon terminology for v4.
+        """
+        scope = (self.timestamp.amz_date_format.split('T')[0] +
+                 '/' + CONF.location + '/s3/aws4_request')
 
         return ('AWS4-HMAC-SHA256' + '\n'
                 + self.timestamp.amz_date_format + '\n'
                 + scope + '\n'
-                + sha256(canonical_request.encode('utf-8')).hexdigest())
+                + sha256(self._canonical_request()).hexdigest())
 
 
 def get_request_class(env):
@@ -571,8 +585,8 @@ class Request(swob.Request):
 
         self._validate_dates()
 
-        if 'Content-MD5' in self.headers:
+        value = _header_strip(self.headers.get('Content-MD5'))
-            value = self.headers['Content-MD5']
+        if value is not None:
             if not re.match('^[A-Za-z0-9+/]+={0,2}$', value):
                 # Non-base64-alphabet characters in value.
                 raise InvalidDigest(content_md5=value)
@@ -725,9 +739,9 @@ class Request(swob.Request):
         """
         amz_headers = {}
 
-        buf = "%s\n%s\n%s\n" % (self.method,
+        buf = [self.method,
-                                self.headers.get('Content-MD5', ''),
+               _header_strip(self.headers.get('Content-MD5')) or '',
-                                self.headers.get('Content-Type') or '')
+               _header_strip(self.headers.get('Content-Type')) or '']
 
         for amz_header in sorted((key.lower() for key in self.headers
                                   if key.lower().startswith('x-amz-'))):
@@ -735,32 +749,33 @@ class Request(swob.Request):
 
         if self._is_header_auth:
             if 'x-amz-date' in amz_headers:
-                buf += "\n"
+                buf.append('')
             elif 'Date' in self.headers:
-                buf += "%s\n" % self.headers['Date']
+                buf.append(self.headers['Date'])
         elif self._is_query_auth:
-            buf += "%s\n" % self.params['Expires']
+            buf.append(self.params['Expires'])
         else:
             # Should have already raised NotS3Request in _parse_auth_info,
             # but as a sanity check...
             raise AccessDenied()
 
         for k in sorted(key.lower() for key in amz_headers):
-            buf += "%s:%s\n" % (k, amz_headers[k])
+            buf.append("%s:%s" % (k, amz_headers[k]))
 
         path = self._canonical_uri()
         if self.query_string:
             path += '?' + self.query_string
+        params = []
         if '?' in path:
             path, args = path.split('?', 1)
-            params = []
             for key, value in sorted(self.params.items()):
                 if key in ALLOWED_SUB_RESOURCES:
                     params.append('%s=%s' % (key, value) if value else key)
-            if params:
+        if params:
-                return '%s%s?%s' % (buf, path, '&'.join(params))
+            buf.append('%s?%s' % (path, '&'.join(params)))
-
+        else:
-        return buf + path
+            buf.append(path)
+        return '\n'.join(buf)
 
     @property
     def controller_name(self):

swift3/test/functional/conf/ceph-known-failures-keystone.yaml

@@ -3,13 +3,9 @@ ceph_s3:
   <nose.suite.ContextSuite context=test_routing_generator>:setup: {status: KNOWN}
   s3tests.functional.test_headers.test_bucket_create_bad_authorization_invalid_aws2: {status: KNOWN}
   s3tests.functional.test_headers.test_bucket_create_bad_authorization_none: {status: KNOWN}
-  s3tests.functional.test_headers.test_bucket_create_bad_contentlength_empty: {status: KNOWN}
   s3tests.functional.test_headers.test_object_create_bad_authorization_invalid_aws2: {status: KNOWN}
   s3tests.functional.test_headers.test_object_create_bad_authorization_none: {status: KNOWN}
-  s3tests.functional.test_headers.test_object_create_bad_contentlength_empty: {status: KNOWN}
-  s3tests.functional.test_headers.test_object_create_bad_contenttype_unreadable: {status: KNOWN}
   s3tests.functional.test_headers.test_object_create_bad_date_after_end_aws2: {status: KNOWN}
-  s3tests.functional.test_headers.test_object_create_bad_md5_unreadable: {status: KNOWN}
   s3tests.functional.test_s3.test_100_continue: {status: KNOWN}
   s3tests.functional.test_s3.test_abort_multipart_upload: {status: KNOWN}
   s3tests.functional.test_s3.test_abort_multipart_upload_not_found: {status: KNOWN}

swift3/test/functional/conf/ceph-known-failures-tempauth.yaml

@@ -3,13 +3,9 @@ ceph_s3:
   <nose.suite.ContextSuite context=test_routing_generator>:setup: {status: KNOWN}
   s3tests.functional.test_headers.test_bucket_create_bad_authorization_invalid_aws2: {status: KNOWN}
   s3tests.functional.test_headers.test_bucket_create_bad_authorization_none: {status: KNOWN}
-  s3tests.functional.test_headers.test_bucket_create_bad_contentlength_empty: {status: KNOWN}
   s3tests.functional.test_headers.test_object_create_bad_authorization_invalid_aws2: {status: KNOWN}
   s3tests.functional.test_headers.test_object_create_bad_authorization_none: {status: KNOWN}
-  s3tests.functional.test_headers.test_object_create_bad_contentlength_empty: {status: KNOWN}
-  s3tests.functional.test_headers.test_object_create_bad_contenttype_unreadable: {status: KNOWN}
   s3tests.functional.test_headers.test_object_create_bad_date_after_end_aws2: {status: KNOWN}
-  s3tests.functional.test_headers.test_object_create_bad_md5_unreadable: {status: KNOWN}
   s3tests.functional.test_s3.test_100_continue: {status: KNOWN}
   s3tests.functional.test_s3.test_atomic_conditional_write_1mb: {status: KNOWN}
   s3tests.functional.test_s3.test_atomic_dual_conditional_write_1mb: {status: KNOWN}

swift3/test/unit/test_middleware.py

@@ -163,6 +163,19 @@ class TestSwift3Middleware(Swift3TestCase):
         self.assertEqual(str1, str2)
         self.assertEqual(str2, str3)
 
+        # Note that boto does not do proper stripping (as of 2.42.0).
+        # These were determined by examining the StringToSignBytes element of
+        # resulting SignatureDoesNotMatch errors from AWS.
+        str1 = canonical_string('/', {'Content-Type': 'text/plain',
+                                      'Content-MD5': '##'})
+        str2 = canonical_string('/', {'Content-Type': '\x01\x02text/plain',
+                                      'Content-MD5': '\x1f ##'})
+        str3 = canonical_string('/', {'Content-Type': 'text/plain \x10',
+                                      'Content-MD5': '##\x18'})
+
+        self.assertEqual(str1, str2)
+        self.assertEqual(str2, str3)
+
     def test_signed_urls_expired(self):
         expire = '1000000000'
         req = Request.blank('/bucket/object?Signature=X&Expires=%s&'
@@ -652,7 +665,7 @@ class TestSwift3Middleware(Swift3TestCase):
         test(auth_str, 'AccessDenied', 'Access Denied.')
 
     def test_canonical_string_v4(self):
-        def canonical_string(path, environ):
+        def _get_req(path, environ):
             if '?' in path:
                 path, query_string = path.split('?', 1)
             else:
@@ -663,17 +676,28 @@ class TestSwift3Middleware(Swift3TestCase):
                 'PATH_INFO': path,
                 'QUERY_STRING': query_string,
                 'HTTP_DATE': 'Mon, 09 Sep 2011 23:36:00 GMT',
-                'HTTP_X_AMZ_CONTENT_SHA256': (
+                'HTTP_X_AMZ_CONTENT_SHA256':
                     'e3b0c44298fc1c149afbf4c8996fb924'
-                    '27ae41e4649b934ca495991b7852b855')
+                    '27ae41e4649b934ca495991b7852b855',
+                'HTTP_AUTHORIZATION':
+                    'AWS4-HMAC-SHA256 '
+                    'Credential=X:Y/dt/reg/host/blah, '
+                    'SignedHeaders=content-md5;content-type;date, '
+                    'Signature=x',
             }
             env.update(environ)
             with patch('swift3.request.Request._validate_headers'):
                 req = SigV4Request(env)
-            return req._string_to_sign()
+            return req
+
+        def string_to_sign(path, environ):
+            return _get_req(path, environ)._string_to_sign()
+
+        def canonical_string(path, environ):
+            return _get_req(path, environ)._canonical_request()
 
         def verify(hash_val, path, environ):
-            s = canonical_string(path, environ)
+            s = string_to_sign(path, environ)
             s = s.split('\n')[3]
             self.assertEqual(hash_val, s)
 
@@ -787,6 +811,19 @@ class TestSwift3Middleware(Swift3TestCase):
                '1b14f04345cbfe6e739236c76dd48f74',
                '/', env)
 
+        # Note that boto does not do proper stripping (as of 2.42.0).
+        # These were determined by examining the StringToSignBytes element of
+        # resulting SignatureDoesNotMatch errors from AWS.
+        str1 = canonical_string('/', {'CONTENT_TYPE': 'text/plain',
+                                      'HTTP_CONTENT_MD5': '##'})
+        str2 = canonical_string('/', {'CONTENT_TYPE': '\x01\x02text/plain',
+                                      'HTTP_CONTENT_MD5': '\x1f ##'})
+        str3 = canonical_string('/', {'CONTENT_TYPE': 'text/plain \x10',
+                                      'HTTP_CONTENT_MD5': '##\x18'})
+
+        self.assertEqual(str1, str2)
+        self.assertEqual(str2, str3)
+
     def test_mixture_param_v4(self):
         # now we have an Authorization header
         headers = {