From e5105ffa09f7919cf27fa9f70aecbc98e53536aa Mon Sep 17 00:00:00 2001 From: Matthew Oliver Date: Fri, 31 Mar 2023 16:48:01 +1100 Subject: [PATCH] internal_client: Remove allow_modify_pipeline option The internal client is suppose to be internal to the cluster, and as such we rely on it to not remove any headers we decide to send. However if the allow_modify_pipeline option is set the gatekeeper middleware is added to the internal client's proxy pipeline. So firstly, this patch removes the allow_modify_pipeline option from the internal client constructor. And when calling loadapp allow_modify_pipeline is always passed with a False. Further, an op could directly put the gatekeeper middleware into the internal client config. The internal client constructor will now check the pipeline and raise a ValueError if one has been placed in the pipeline. To do this, there is now a check_gatekeeper_loaded staticmethod that will walk the pipeline which called from the InternalClient.__init__ method. Enabling this walking through the pipeline, we are now stashing the wsgi pipeline in each filter so that we don't have to rely on 'app' naming conventions to iterate the pipeline. Co-Authored-By: Alistair Coles Change-Id: Idcca7ac0796935c8883de9084d612d64159d9f92 --- etc/internal-client.conf-sample | 1 + swift/common/internal_client.py | 25 ++++++++-- swift/common/wsgi.py | 8 +++- swift/container/sharder.py | 1 - test/unit/common/test_internal_client.py | 61 ++++++++++++++++++++++++ test/unit/common/test_wsgi.py | 6 +++ test/unit/container/test_sharder.py | 4 -- 7 files changed, 96 insertions(+), 10 deletions(-) diff --git a/etc/internal-client.conf-sample b/etc/internal-client.conf-sample index 7ded5fd8ad..d9ed5e24b2 100644 --- a/etc/internal-client.conf-sample +++ b/etc/internal-client.conf-sample @@ -26,6 +26,7 @@ # log_statsd_metric_prefix = [pipeline:main] +# Note: gatekeeper middleware is not allowed in the internal client pipeline pipeline = catch_errors proxy-logging cache symlink proxy-server [app:proxy-server] diff --git a/swift/common/internal_client.py b/swift/common/internal_client.py index 2c1c99cc0e..d82035c398 100644 --- a/swift/common/internal_client.py +++ b/swift/common/internal_client.py @@ -28,6 +28,7 @@ from zlib import compressobj from swift.common.exceptions import ClientException from swift.common.http import (HTTP_NOT_FOUND, HTTP_MULTIPLE_CHOICES, is_client_error, is_server_error) +from swift.common.middleware.gatekeeper import GatekeeperMiddleware from swift.common.request_helpers import USE_REPLICATION_NETWORK_HEADER from swift.common.swob import Request, bytes_to_wsgi from swift.common.utils import quote, close_if_possible, drain_and_close @@ -144,6 +145,8 @@ class InternalClient(object): :param user_agent: User agent to be sent to requests to Swift. :param request_tries: Number of tries before InternalClient.make_request() gives up. + :param use_replication_network: Force the client to use the replication + network over the cluster. :param global_conf: a dict of options to update the loaded proxy config. Options in ``global_conf`` will override those in ``conf_path`` except where the ``conf_path`` option is preceded by ``set``. @@ -151,12 +154,15 @@ class InternalClient(object): """ def __init__(self, conf_path, user_agent, request_tries, - allow_modify_pipeline=False, use_replication_network=False, - global_conf=None, app=None): + use_replication_network=False, global_conf=None, app=None, + **kwargs): if request_tries < 1: raise ValueError('request_tries must be positive') + # Internal clients don't use the gatekeeper and the pipeline remains + # static so we never allow anything to modify the proxy pipeline. self.app = app or loadapp(conf_path, global_conf=global_conf, - allow_modify_pipeline=allow_modify_pipeline,) + allow_modify_pipeline=False,) + self.check_gatekeeper_not_loaded(self.app) self.user_agent = \ self.app._pipeline_final_app.backend_user_agent = user_agent self.request_tries = request_tries @@ -167,6 +173,19 @@ class InternalClient(object): self.auto_create_account_prefix = \ self.app._pipeline_final_app.auto_create_account_prefix + @staticmethod + def check_gatekeeper_not_loaded(app): + # the Gatekeeper middleware would prevent an InternalClient passing + # X-Backend-* headers to the proxy app, so ensure it's not present + try: + for app in app._pipeline: + if isinstance(app, GatekeeperMiddleware): + raise ValueError( + "Gatekeeper middleware is not allowed in the " + "InternalClient proxy pipeline") + except AttributeError: + pass + def make_request( self, method, path, headers, acceptable_statuses, body_file=None, params=None): diff --git a/swift/common/wsgi.py b/swift/common/wsgi.py index 4fa4946dd7..7c39a89e25 100644 --- a/swift/common/wsgi.py +++ b/swift/common/wsgi.py @@ -361,10 +361,14 @@ def loadapp(conf_file, global_conf=None, allow_modify_pipeline=True): if func and allow_modify_pipeline: func(PipelineWrapper(ctx)) filters = [c.create() for c in reversed(ctx.filter_contexts)] + pipeline = [ultimate_app] + ultimate_app._pipeline = pipeline + ultimate_app._pipeline_final_app = ultimate_app app = ultimate_app - app._pipeline_final_app = ultimate_app for filter_app in filters: - app = filter_app(app) + app = filter_app(pipeline[0]) + pipeline.insert(0, app) + app._pipeline = pipeline app._pipeline_final_app = ultimate_app return app return ctx.create() diff --git a/swift/container/sharder.py b/swift/container/sharder.py index 378bad7c94..ca6ee05662 100644 --- a/swift/container/sharder.py +++ b/swift/container/sharder.py @@ -895,7 +895,6 @@ class ContainerSharder(ContainerSharderConf, ContainerReplicator): internal_client_conf_path, 'Swift Container Sharder', request_tries, - allow_modify_pipeline=False, use_replication_network=True, global_conf={'log_name': '%s-ic' % conf.get( 'log_name', self.log_route)}) diff --git a/test/unit/common/test_internal_client.py b/test/unit/common/test_internal_client.py index d26ef0e2d1..1ccbf30f08 100644 --- a/test/unit/common/test_internal_client.py +++ b/test/unit/common/test_internal_client.py @@ -30,6 +30,7 @@ from swift.common import exceptions, internal_client, request_helpers, swob, \ from swift.common.header_key_dict import HeaderKeyDict from swift.common.storage_policy import StoragePolicy from swift.common.middleware.proxy_logging import ProxyLoggingMiddleware +from swift.common.middleware.gatekeeper import GatekeeperMiddleware from test.debug_logger import debug_logger from test.unit import with_tempdir, write_fake_ring, patch_policies @@ -392,6 +393,21 @@ class TestInternalClient(unittest.TestCase): conf_path, user_agent, request_tries=0) mock_loadapp.assert_not_called() + # if we load it with the gatekeeper middleware then we also get a + # value error + gate_keeper_app = GatekeeperMiddleware(app, {}) + gate_keeper_app._pipeline_final_app = app + gate_keeper_app._pipeline = [gate_keeper_app, app] + with mock.patch.object( + internal_client, 'loadapp', return_value=gate_keeper_app) \ + as mock_loadapp, self.assertRaises(ValueError) as err: + internal_client.InternalClient( + conf_path, user_agent, request_tries) + self.assertEqual( + str(err.exception), + ('Gatekeeper middleware is not allowed in the InternalClient ' + 'proxy pipeline')) + with mock.patch.object( internal_client, 'loadapp', return_value=app) as mock_loadapp: client = internal_client.InternalClient( @@ -421,6 +437,51 @@ class TestInternalClient(unittest.TestCase): self.assertEqual(request_tries, client.request_tries) self.assertTrue(client.use_replication_network) + def test_gatekeeper_not_loaded(self): + app = FakeSwift() + pipeline = [app] + + class RandomMiddleware(object): + def __init__(self, app): + self.app = app + self._pipeline_final_app = app + self._pipeline = pipeline + self._pipeline.insert(0, self) + + # if there is no Gatekeeper middleware then it's false + # just the final app + self.assertFalse( + internal_client.InternalClient.check_gatekeeper_not_loaded(app)) + + # now with a bunch of middlewares + app_no_gatekeeper = app + for i in range(5): + app_no_gatekeeper = RandomMiddleware(app_no_gatekeeper) + self.assertFalse( + internal_client.InternalClient.check_gatekeeper_not_loaded( + app_no_gatekeeper)) + + # But if we put the gatekeeper on the end, it will be found + app_with_gatekeeper = GatekeeperMiddleware(app_no_gatekeeper, {}) + pipeline.insert(0, app_with_gatekeeper) + app_with_gatekeeper._pipeline = pipeline + with self.assertRaises(ValueError) as err: + internal_client.InternalClient.check_gatekeeper_not_loaded( + app_with_gatekeeper) + self.assertEqual(str(err.exception), + ('Gatekeeper middleware is not allowed in the ' + 'InternalClient proxy pipeline')) + + # even if we bury deep into the pipeline + for i in range(5): + app_with_gatekeeper = RandomMiddleware(app_with_gatekeeper) + with self.assertRaises(ValueError) as err: + internal_client.InternalClient.check_gatekeeper_not_loaded( + app_with_gatekeeper) + self.assertEqual(str(err.exception), + ('Gatekeeper middleware is not allowed in the ' + 'InternalClient proxy pipeline')) + def test_make_request_sets_user_agent(self): class FakeApp(FakeSwift): def __init__(self, test): diff --git a/test/unit/common/test_wsgi.py b/test/unit/common/test_wsgi.py index d43f6730b0..5cddc7164f 100644 --- a/test/unit/common/test_wsgi.py +++ b/test/unit/common/test_wsgi.py @@ -1642,6 +1642,12 @@ class TestPipelineModification(unittest.TestCase): self.assertIs(app.app.app, app._pipeline_final_app) self.assertIs(app.app.app, app.app._pipeline_final_app) self.assertIs(app.app.app, app.app.app._pipeline_final_app) + exp_pipeline = [app, app.app, app.app.app] + self.assertEqual(exp_pipeline, app._pipeline) + self.assertEqual(exp_pipeline, app.app._pipeline) + self.assertEqual(exp_pipeline, app.app.app._pipeline) + self.assertIs(app._pipeline, app.app._pipeline) + self.assertIs(app._pipeline, app.app.app._pipeline) # make sure you can turn off the pipeline modification if you want def blow_up(*_, **__): diff --git a/test/unit/container/test_sharder.py b/test/unit/container/test_sharder.py index f73f202545..c101f93cd0 100644 --- a/test/unit/container/test_sharder.py +++ b/test/unit/container/test_sharder.py @@ -203,7 +203,6 @@ class TestSharder(BaseTestSharder): 'container-sharder', sharder.logger.logger.name) mock_ic.assert_called_once_with( '/etc/swift/internal-client.conf', 'Swift Container Sharder', 3, - allow_modify_pipeline=False, use_replication_network=True, global_conf={'log_name': 'container-sharder-ic'}) @@ -218,7 +217,6 @@ class TestSharder(BaseTestSharder): sharder, mock_ic = self._do_test_init(conf, expected) mock_ic.assert_called_once_with( '/etc/swift/internal-client.conf', 'Swift Container Sharder', 3, - allow_modify_pipeline=False, use_replication_network=True, global_conf={'log_name': 'container-sharder-ic'}) @@ -280,7 +278,6 @@ class TestSharder(BaseTestSharder): sharder, mock_ic = self._do_test_init(conf, expected) mock_ic.assert_called_once_with( '/etc/swift/my-sharder-ic.conf', 'Swift Container Sharder', 2, - allow_modify_pipeline=False, use_replication_network=True, global_conf={'log_name': 'container-sharder-ic'}) self.assertEqual(self.logger.get_lines_for_level('warning'), [ @@ -418,7 +415,6 @@ class TestSharder(BaseTestSharder): mock_ic.assert_called_once_with( '/etc/swift/internal-client.conf', 'Swift Container Sharder', 3, - allow_modify_pipeline=False, global_conf={'log_name': exp_internal_client_log_name}, use_replication_network=True)