From 5af6c9b26fef2489ed2643b1059921a2c2d4387b Mon Sep 17 00:00:00 2001 From: David Wilson Date: Sat, 10 Aug 2019 00:37:17 +0100 Subject: [PATCH 1/5] issue #615: use FileService for target->controll file transfers --- ansible_mitogen/connection.py | 9 +++++---- mitogen/core.py | 9 +++++++-- mitogen/service.py | 6 +++++- tests/ansible/tests/connection_test.py | 15 +++++++++++++++ 4 files changed, 32 insertions(+), 7 deletions(-) diff --git a/ansible_mitogen/connection.py b/ansible_mitogen/connection.py index c08df611..06a152b2 100644 --- a/ansible_mitogen/connection.py +++ b/ansible_mitogen/connection.py @@ -950,11 +950,12 @@ class Connection(ansible.plugins.connection.ConnectionBase): :param str out_path: Local filesystem path to write. """ - output = self.get_chain().call( - ansible_mitogen.target.read_path, - mitogen.utils.cast(in_path), + self._connect() + ansible_mitogen.target.transfer_file( + context=self.context, + in_path=in_path, + out_path=out_path ) - ansible_mitogen.target.write_path(out_path, output) def put_data(self, out_path, data, mode=None, utimes=None): """ diff --git a/mitogen/core.py b/mitogen/core.py index a14286f9..8b4f135e 100644 --- a/mitogen/core.py +++ b/mitogen/core.py @@ -395,6 +395,12 @@ else: return _partition(s, sep, s.find) or (s, '', '') +def _has_parent_authority(context_id): + return ( + (context_id == mitogen.context_id) or + (context_id in mitogen.parent_ids) + ) + def has_parent_authority(msg, _stream=None): """ Policy function for use with :class:`Receiver` and @@ -403,8 +409,7 @@ def has_parent_authority(msg, _stream=None): ` has been set to that of a parent context or the current context. """ - return (msg.auth_id == mitogen.context_id or - msg.auth_id in mitogen.parent_ids) + return _has_parent_authority(msg.auth_id) def _signals(obj, signal): diff --git a/mitogen/service.py b/mitogen/service.py index 77a1ae1b..6654fb32 100644 --- a/mitogen/service.py +++ b/mitogen/service.py @@ -1023,7 +1023,11 @@ class FileService(Service): :raises Error: Unregistered path, or Sender did not match requestee context. """ - if path not in self._paths and not self._prefix_is_authorized(path): + if ( + (path not in self._paths) and + (not self._prefix_is_authorized(path)) and + (not mitogen.core._has_parent_authority(msg.auth_id)) + ): msg.reply(mitogen.core.CallError( Error(self.unregistered_msg % (path,)) )) diff --git a/tests/ansible/tests/connection_test.py b/tests/ansible/tests/connection_test.py index e7646716..36d61f09 100644 --- a/tests/ansible/tests/connection_test.py +++ b/tests/ansible/tests/connection_test.py @@ -99,6 +99,21 @@ class OptionalIntTest(testlib.TestCase): self.assertEquals(None, self.func({1:2})) +class FetchFileTest(ConnectionMixin, testlib.TestCase): + def test_success(self): + with tempfile.NamedTemporaryFile(prefix='mitotest') as ifp, \ + tempfile.NamedTemporaryFile(prefix='mitotest') as ofp: + ifp.write('x' * (1048576 * 4)) + ifp.flush() + ifp.seek(0) + + self.conn.fetch_file(ifp.name, ofp.name) + # transfer_file() uses os.rename rather than direct data overwrite, + # so we must reopen. + with open(ofp.name, 'rb') as fp: + self.assertEquals(ifp.read(), fp.read()) + + class PutDataTest(ConnectionMixin, testlib.TestCase): def test_out_path(self): path = tempfile.mktemp(prefix='mitotest') From c464bb534620bbdef64680dda758f5f879e41b1a Mon Sep 17 00:00:00 2001 From: David Wilson Date: Sat, 10 Aug 2019 00:40:08 +0100 Subject: [PATCH 2/5] issue #615: update Changelog. --- docs/changelog.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index 26b2c316..6155eb43 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -116,6 +116,12 @@ Mitogen for Ansible module, and for any other action plug-ins that establish new connections of their own. +* `#615 `_: streaming file transfer + is implemented for the ``fetch`` and any other action that transfers files + from the target to the controller. Previously the file would be sent as a + single message, requiring the file to fit in RAM and be smaller than internal + limits on the size of a single message. + * `7ae926b3 `_: the ``lineinfile`` module began leaking writable temporary file descriptors since Ansible 2.7.0. When ``lineinfile`` was used to create or modify a script, and @@ -219,6 +225,7 @@ bug reports, testing, features and fixes in this release contributed by `Szabó Dániel Ernő `_, `Ulrich Schreiner `_, `Yuki Nishida `_, +`@alexhexabeam `_, `@DavidVentura `_, `@ghp-rr `_, `@rizzly `_, and From 9e1e1ba0159ed30028e4fd0783a5af57f9619494 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Sat, 10 Aug 2019 00:44:49 +0100 Subject: [PATCH 3/5] issue #615: Py3x fix. --- tests/ansible/tests/connection_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ansible/tests/connection_test.py b/tests/ansible/tests/connection_test.py index 36d61f09..738aed28 100644 --- a/tests/ansible/tests/connection_test.py +++ b/tests/ansible/tests/connection_test.py @@ -103,7 +103,7 @@ class FetchFileTest(ConnectionMixin, testlib.TestCase): def test_success(self): with tempfile.NamedTemporaryFile(prefix='mitotest') as ifp, \ tempfile.NamedTemporaryFile(prefix='mitotest') as ofp: - ifp.write('x' * (1048576 * 4)) + ifp.write(b'x' * (1048576 * 4)) ifp.flush() ifp.seek(0) From 588859423a499e58ff1585cae75561e1a43379e1 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Sat, 10 Aug 2019 09:52:37 +0100 Subject: [PATCH 4/5] issue #615: another Py3x fix. --- tests/ansible/tests/connection_test.py | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/tests/ansible/tests/connection_test.py b/tests/ansible/tests/connection_test.py index 738aed28..54ea3d99 100644 --- a/tests/ansible/tests/connection_test.py +++ b/tests/ansible/tests/connection_test.py @@ -101,17 +101,17 @@ class OptionalIntTest(testlib.TestCase): class FetchFileTest(ConnectionMixin, testlib.TestCase): def test_success(self): - with tempfile.NamedTemporaryFile(prefix='mitotest') as ifp, \ - tempfile.NamedTemporaryFile(prefix='mitotest') as ofp: - ifp.write(b'x' * (1048576 * 4)) - ifp.flush() - ifp.seek(0) - - self.conn.fetch_file(ifp.name, ofp.name) - # transfer_file() uses os.rename rather than direct data overwrite, - # so we must reopen. - with open(ofp.name, 'rb') as fp: - self.assertEquals(ifp.read(), fp.read()) + with tempfile.NamedTemporaryFile(prefix='mitotest') as ifp: + with tempfile.NamedTemporaryFile(prefix='mitotest') as ofp: + ifp.write(b'x' * (1048576 * 4)) + ifp.flush() + ifp.seek(0) + + self.conn.fetch_file(ifp.name, ofp.name) + # transfer_file() uses os.rename rather than direct data + # overwrite, so we must reopen. + with open(ofp.name, 'rb') as fp: + self.assertEquals(ifp.read(), fp.read()) class PutDataTest(ConnectionMixin, testlib.TestCase): From 7d4ae6cec41e0652280f3766e10c66f73ddb7835 Mon Sep 17 00:00:00 2001 From: David Wilson Date: Sat, 10 Aug 2019 10:22:08 +0100 Subject: [PATCH 5/5] issue #615: fix up FileService tests for new logic Can't perform authorization test in the same process so easily any more since it checks is_privileged --- tests/file_service_test.py | 86 +++++++++++++++++++++++++++----------- 1 file changed, 62 insertions(+), 24 deletions(-) diff --git a/tests/file_service_test.py b/tests/file_service_test.py index b9034bb1..45b621ac 100644 --- a/tests/file_service_test.py +++ b/tests/file_service_test.py @@ -22,15 +22,26 @@ class FetchTest(testlib.RouterMixin, testlib.TestCase): return recv, msg def test_unauthorized(self): + l1 = self.router.local() + service = self.klass(self.router) - recv, msg = self.replyable_msg() - service.fetch( - path='/etc/shadow', - sender=None, - msg=msg, + pool = mitogen.service.Pool( + router=self.router, + services=[service], + size=1, ) - e = self.assertRaises(mitogen.core.CallError, - lambda: recv.get().unpickle()) + try: + e = self.assertRaises(mitogen.core.CallError, + lambda: l1.call( + mitogen.service.FileService.get, + context=self.router.myself(), + path='/etc/shadow', + out_fp=None, + ) + ) + finally: + pool.stop() + expect = service.unregistered_msg % ('/etc/shadow',) self.assertTrue(expect in e.args[0]) @@ -85,30 +96,57 @@ class FetchTest(testlib.RouterMixin, testlib.TestCase): self._validate_response(recv.get().unpickle()) def test_prefix_authorized_abspath_bad(self): - recv = mitogen.core.Receiver(self.router) + l1 = self.router.local() + service = self.klass(self.router) service.register_prefix('/etc') - recv, msg = self.replyable_msg() - service.fetch( - path='/etc/foo/bar/../../../passwd', - sender=recv.to_sender(), - msg=msg, + + pool = mitogen.service.Pool( + router=self.router, + services=[service], + size=1, ) - self.assertEquals(None, recv.get().unpickle()) + path = '/etc/foo/bar/../../../passwd' + try: + e = self.assertRaises(mitogen.core.CallError, + lambda: l1.call( + mitogen.service.FileService.get, + context=self.router.myself(), + path=path, + out_fp=None, + ) + ) + finally: + pool.stop() + + expect = service.unregistered_msg % (path,) + self.assertTrue(expect in e.args[0]) + + def test_prefix_authorized_abspath_good(self): + l1 = self.router.local() - def test_prefix_authorized_abspath_bad(self): - recv = mitogen.core.Receiver(self.router) service = self.klass(self.router) service.register_prefix('/etc') - recv, msg = self.replyable_msg() - service.fetch( - path='/etc/../shadow', - sender=recv.to_sender(), - msg=msg, + path = '/etc/../shadow' + + pool = mitogen.service.Pool( + router=self.router, + services=[service], + size=1, ) - e = self.assertRaises(mitogen.core.CallError, - lambda: recv.get().unpickle()) - expect = service.unregistered_msg % ('/etc/../shadow',) + try: + e = self.assertRaises(mitogen.core.CallError, + lambda: l1.call( + mitogen.service.FileService.get, + context=self.router.myself(), + path=path, + out_fp=None + ) + ) + finally: + pool.stop() + + expect = service.unregistered_msg % (path,) self.assertTrue(expect in e.args[0])