From 475cdba65bcd29871b840395576572c0d8399a6b Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Wed, 29 Jun 2022 15:44:33 -0700 Subject: [PATCH] Emit metrics for tempurl & formpost digest usage Without this, it's really hard for operators to feel comfortable removing support for now-deprecated digests. This patch adds new statsd metrics in the form of: formpost.digests. tempurl.digest. Something like: formpost.digessts.sha1 tempurl.digests.sha512 Change-Id: I203607a0576582330241172d05bf8fd223bbbb9d --- swift/common/middleware/formpost.py | 11 ++++++----- swift/common/middleware/tempurl.py | 6 ++++-- test/unit/common/middleware/test_formpost.py | 20 +++++++++++++------- test/unit/common/middleware/test_tempurl.py | 8 ++++++++ 4 files changed, 31 insertions(+), 14 deletions(-) diff --git a/swift/common/middleware/formpost.py b/swift/common/middleware/formpost.py index b1df258256..57ba41d5c2 100644 --- a/swift/common/middleware/formpost.py +++ b/swift/common/middleware/formpost.py @@ -206,11 +206,12 @@ class FormPost(object): :param conf: The configuration dict for the middleware. """ - def __init__(self, app, conf): + def __init__(self, app, conf, logger=None): #: The next WSGI application/filter in the paste.deploy pipeline. self.app = app #: The filter configuration dict. self.conf = conf + self.logger = logger or get_logger(conf, log_route='formpost') # Defaulting to SUPPORTED_DIGESTS just so we don't completely # deprecate sha1 yet. We'll change this to DEFAULT_ALLOWED_DIGESTS # later. @@ -413,13 +414,12 @@ class FormPost(object): has_valid_sig = False signature = attributes.get('signature', '') try: - hash_algorithm, signature = extract_digest_and_algorithm(signature) + hash_name, signature = extract_digest_and_algorithm(signature) except ValueError: raise FormUnauthorized('invalid signature') - if hash_algorithm not in self.allowed_digests: + if hash_name not in self.allowed_digests: raise FormUnauthorized('invalid signature') - if six.PY2: - hash_algorithm = getattr(hashlib, hash_algorithm) + hash_algorithm = getattr(hashlib, hash_name) if six.PY2 else hash_name for key in keys: # Encode key like in swift.common.utls.get_hmac. @@ -430,6 +430,7 @@ class FormPost(object): has_valid_sig = True if not has_valid_sig: raise FormUnauthorized('invalid signature') + self.logger.increment('formpost.digests.%s' % hash_name) substatus = [None] subheaders = [None] diff --git a/swift/common/middleware/tempurl.py b/swift/common/middleware/tempurl.py index ab155f0398..aef3e6de2c 100644 --- a/swift/common/middleware/tempurl.py +++ b/swift/common/middleware/tempurl.py @@ -423,11 +423,12 @@ class TempURL(object): :param conf: The configuration dict for the middleware. """ - def __init__(self, app, conf): + def __init__(self, app, conf, logger=None): #: The next WSGI application/filter in the paste.deploy pipeline. self.app = app #: The filter configuration dict. self.conf = conf + self.logger = logger or get_logger(conf, log_route='tempurl') self.allowed_digests = conf.get( 'allowed_digests', DEFAULT_ALLOWED_DIGESTS.split()) @@ -560,6 +561,7 @@ class TempURL(object): break if not is_valid_hmac: return self._invalid(env, start_response) + self.logger.increment('tempurl.digests.%s' % hash_algorithm) # disallowed headers prevent accidentally allowing upload of a pointer # to data that the PUT tempurl would not otherwise allow access for. # It should be safe to provide a GET tempurl for data that an @@ -867,4 +869,4 @@ def filter_factory(global_conf, **local_conf): register_sensitive_param('temp_url_sig') - return lambda app: TempURL(app, conf) + return lambda app: TempURL(app, conf, logger) diff --git a/test/unit/common/middleware/test_formpost.py b/test/unit/common/middleware/test_formpost.py index caa7487d6b..455997d226 100644 --- a/test/unit/common/middleware/test_formpost.py +++ b/test/unit/common/middleware/test_formpost.py @@ -134,6 +134,7 @@ class TestFormPost(unittest.TestCase): self.app = FakeApp() self.auth = tempauth.filter_factory({})(self.app) self.formpost = formpost.filter_factory({})(self.auth) + self.logger = self.formpost.logger = debug_logger() def _make_request(self, path, tempurl_keys=(), **kwargs): req = Request.blank(path, **kwargs) @@ -1489,10 +1490,8 @@ class TestFormPost(unittest.TestCase): self._fake_cache_env('AUTH_test', [key])) env['swift.infocache'][get_cache_key( 'AUTH_test', 'container')] = {'meta': {}} - self.app = FakeApp(iter([('201 Created', {}, b''), - ('201 Created', {}, b'')])) - self.auth = tempauth.filter_factory({})(self.app) - self.formpost = formpost.filter_factory({})(self.auth) + self.auth.app = app = FakeApp(iter([('201 Created', {}, b''), + ('201 Created', {}, b'')])) status = [None] headers = [None] exc_info = [None] @@ -1514,14 +1513,21 @@ class TestFormPost(unittest.TestCase): self.assertIsNone(location) self.assertIsNone(exc_info) self.assertTrue(b'201 Created' in body) - self.assertEqual(len(self.app.requests), 2) - self.assertEqual(self.app.requests[0].body, b'Test File\nOne\n') - self.assertEqual(self.app.requests[1].body, b'Test\nFile\nTwo\n') + self.assertEqual(len(app.requests), 2) + self.assertEqual(app.requests[0].body, b'Test File\nOne\n') + self.assertEqual(app.requests[1].body, b'Test\nFile\nTwo\n') for digest in ('sha1', 'sha256', 'sha512'): do_test(digest, True) do_test(digest, False) + # NB: one increment per *upload*, not client request + self.assertEqual(self.logger.get_increment_counts(), { + 'formpost.digests.sha1': 4, + 'formpost.digests.sha256': 4, + 'formpost.digests.sha512': 4, + }) + def test_prefixed_and_not_prefixed_sigs_unsupported(self): def do_test(digest, prefixed): key = b'abc' diff --git a/test/unit/common/middleware/test_tempurl.py b/test/unit/common/middleware/test_tempurl.py index 1d61e99e13..4c6a31bc1b 100644 --- a/test/unit/common/middleware/test_tempurl.py +++ b/test/unit/common/middleware/test_tempurl.py @@ -78,6 +78,7 @@ class TestTempURL(unittest.TestCase): self.app = FakeApp() self.auth = tempauth.filter_factory({'reseller_prefix': ''})(self.app) self.tempurl = tempurl.filter_factory({})(self.auth) + self.logger = self.tempurl.logger = debug_logger() def _make_request(self, path, environ=None, keys=(), container_keys=None, **kwargs): @@ -162,6 +163,7 @@ class TestTempURL(unittest.TestCase): tempurl1 = tempurl.filter_factory({ 'allowed_digests': 'sha1'})(self.auth) + tempurl1.logger = self.logger sig = hmac.new(key, hmac_body, hashlib.sha1).hexdigest() self.assert_valid_sig(expires, path, [key], sig, tempurl=tempurl1) @@ -176,6 +178,12 @@ class TestTempURL(unittest.TestCase): key, hmac_body, hashlib.sha512).digest()) self.assert_valid_sig(expires, path, [key], b'sha512:' + sig) + self.assertEqual(self.logger.get_increment_counts(), { + 'tempurl.digests.sha1': 1, + 'tempurl.digests.sha256': 2, + 'tempurl.digests.sha512': 1 + }) + def test_get_valid_key2(self): method = 'GET' expires = int(time() + 86400)