diff options
author | Adrien Ferrand <adferrand@users.noreply.github.com> | 2020-04-27 19:38:30 +0300 |
---|---|---|
committer | GitHub <noreply@github.com> | 2020-04-27 19:38:30 +0300 |
commit | 9cbb13ef041cb16c668f6644d132fcd738488798 (patch) | |
tree | a8cf0db634b8ca7cb645d8e7b6e0770edf900925 | |
parent | 01dc981a0923ccd5eb3267a737360dc2f1906602 (diff) |
Run hooks with Powershell on Windows (#7800)
Fixes #7713.
As discussed in #7713, providing a Powershell script as hook for Certbot is not working currently. This is because hooks are run in a `cmd` environment, that recognizes only `.bat` files as valid scripts that can be run from their bare name on command line.
On the other hand, the Powershell both `.bat` and `.ps1` scripts as valid scripts.
This PR makes hooks command be executed by Powershell, instead of `cmd` as `Popen` does by default when `shell=true` is used. It also modifies the tests to handle this new environment, in particular in term of encoding (UTF-16-LE is the default one in Powershell).
* Run hooks in powershell on Windows
* Fix hook test
* Fallback to unittest.mock
* In fact, shell_cmd as a list of str could not work. Declare only str as acceptable input for shell_cmd.
* Added changelog
-rwxr-xr-x | certbot-ci/certbot_integration_tests/assets/hook.py | 4 | ||||
-rw-r--r-- | certbot-ci/certbot_integration_tests/certbot_tests/assertions.py | 4 | ||||
-rw-r--r-- | certbot-ci/certbot_integration_tests/utils/misc.py | 7 | ||||
-rw-r--r-- | certbot/CHANGELOG.md | 2 | ||||
-rw-r--r-- | certbot/certbot/_internal/hooks.py | 31 | ||||
-rw-r--r-- | certbot/certbot/_internal/plugins/manual.py | 3 | ||||
-rw-r--r-- | certbot/certbot/compat/misc.py | 41 | ||||
-rw-r--r-- | certbot/tests/compat/misc_test.py | 48 | ||||
-rw-r--r-- | certbot/tests/hook_test.py | 44 |
9 files changed, 107 insertions, 77 deletions
diff --git a/certbot-ci/certbot_integration_tests/assets/hook.py b/certbot-ci/certbot_integration_tests/assets/hook.py index 39aa72ac5..483892a93 100755 --- a/certbot-ci/certbot_integration_tests/assets/hook.py +++ b/certbot-ci/certbot_integration_tests/assets/hook.py @@ -1,4 +1,5 @@ #!/usr/bin/env python +from __future__ import print_function import os import sys @@ -7,5 +8,4 @@ if hook_script_type == 'deploy' and ('RENEWED_DOMAINS' not in os.environ or 'REN sys.stderr.write('Environment variables not properly set!\n') sys.exit(1) -with open(sys.argv[2], 'a') as file_h: - file_h.write(hook_script_type + '\n') +print(hook_script_type) diff --git a/certbot-ci/certbot_integration_tests/certbot_tests/assertions.py b/certbot-ci/certbot_integration_tests/certbot_tests/assertions.py index 1b5914d1a..53df6b890 100644 --- a/certbot-ci/certbot_integration_tests/certbot_tests/assertions.py +++ b/certbot-ci/certbot_integration_tests/certbot_tests/assertions.py @@ -1,4 +1,5 @@ """This module contains advanced assertions for the certbot integration tests.""" +import io import os try: @@ -21,7 +22,8 @@ def assert_hook_execution(probe_path, probe_content): :param probe_path: path to the file that received the hook output :param probe_content: content expected when the hook is executed """ - with open(probe_path, 'r') as file: + encoding = 'utf-8' if POSIX_MODE else 'utf-16' + with io.open(probe_path, 'rt', encoding=encoding) as file: data = file.read() lines = [line.strip() for line in data.splitlines()] diff --git a/certbot-ci/certbot_integration_tests/utils/misc.py b/certbot-ci/certbot_integration_tests/utils/misc.py index fbb965034..4b2774827 100644 --- a/certbot-ci/certbot_integration_tests/utils/misc.py +++ b/certbot-ci/certbot_integration_tests/utils/misc.py @@ -140,13 +140,12 @@ def generate_test_file_hooks(config_dir, hook_probe): entrypoint_script = '''\ #!/usr/bin/env bash set -e -"{0}" "{1}" "{2}" "{3}" +"{0}" "{1}" "{2}" >> "{3}" '''.format(sys.executable, hook_path, entrypoint_script_path, hook_probe) else: - entrypoint_script_path = os.path.join(hook_dir, 'entrypoint.bat') + entrypoint_script_path = os.path.join(hook_dir, 'entrypoint.ps1') entrypoint_script = '''\ -@echo off -"{0}" "{1}" "{2}" "{3}" +& "{0}" "{1}" "{2}" >> "{3}" '''.format(sys.executable, hook_path, entrypoint_script_path, hook_probe) with open(entrypoint_script_path, 'w') as file_h: diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 9eb7cd9e8..6b5a1055c 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -15,6 +15,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). * Added TLS-ALPN-01 challenge support in the `acme` library. Support of this challenge in the Certbot client is planned to be added in a future release. * Added minimal proxy support for OCSP verification. +* On Windows, hooks are now executed in a Powershell shell instead of a CMD shell, + allowing both `*.ps1` and `*.bat` as valid scripts for Certbot. ### Changed diff --git a/certbot/certbot/_internal/hooks.py b/certbot/certbot/_internal/hooks.py index 589c59e89..f26c5c76b 100644 --- a/certbot/certbot/_internal/hooks.py +++ b/certbot/certbot/_internal/hooks.py @@ -2,14 +2,13 @@ from __future__ import print_function import logging -from subprocess import PIPE -from subprocess import Popen from acme.magic_typing import List from acme.magic_typing import Set from certbot import errors from certbot import util from certbot.compat import filesystem +from certbot.compat import misc from certbot.compat import os from certbot.plugins import util as plug_util @@ -229,36 +228,10 @@ def _run_hook(cmd_name, shell_cmd): :type shell_cmd: `list` of `str` or `str` :returns: stderr if there was any""" - err, _ = execute(cmd_name, shell_cmd) + err, _ = misc.execute_command(cmd_name, shell_cmd) return err -def execute(cmd_name, shell_cmd): - """Run a command. - - :param str cmd_name: the user facing name of the hook being run - :param shell_cmd: shell command to execute - :type shell_cmd: `list` of `str` or `str` - - :returns: `tuple` (`str` stderr, `str` stdout)""" - logger.info("Running %s command: %s", cmd_name, shell_cmd) - - # universal_newlines causes Popen.communicate() - # to return str objects instead of bytes in Python 3 - cmd = Popen(shell_cmd, shell=True, stdout=PIPE, - stderr=PIPE, universal_newlines=True) - out, err = cmd.communicate() - base_cmd = os.path.basename(shell_cmd.split(None, 1)[0]) - if out: - logger.info('Output from %s command %s:\n%s', cmd_name, base_cmd, out) - if cmd.returncode != 0: - logger.error('%s command "%s" returned error code %d', - cmd_name, shell_cmd, cmd.returncode) - if err: - logger.error('Error output from %s command %s:\n%s', cmd_name, base_cmd, err) - return err, out - - def list_hooks(dir_path): """List paths to all hooks found in dir_path in sorted order. diff --git a/certbot/certbot/_internal/plugins/manual.py b/certbot/certbot/_internal/plugins/manual.py index b46622796..430059445 100644 --- a/certbot/certbot/_internal/plugins/manual.py +++ b/certbot/certbot/_internal/plugins/manual.py @@ -9,6 +9,7 @@ from certbot import errors from certbot import interfaces from certbot import reverter from certbot._internal import hooks +from certbot.compat import misc from certbot.compat import os from certbot.plugins import common @@ -186,4 +187,4 @@ permitted by DNS standards.) self.reverter.recovery_routine() def _execute_hook(self, hook_name): - return hooks.execute(self.option_name(hook_name), self.conf(hook_name)) + return misc.execute_command(self.option_name(hook_name), self.conf(hook_name)) diff --git a/certbot/certbot/compat/misc.py b/certbot/certbot/compat/misc.py index 956c56370..49a0814f4 100644 --- a/certbot/certbot/compat/misc.py +++ b/certbot/certbot/compat/misc.py @@ -4,12 +4,16 @@ particular category. """ from __future__ import absolute_import +import logging import select +import subprocess import sys from certbot import errors from certbot.compat import os +from acme.magic_typing import Tuple + try: from win32com.shell import shell as shellwin32 POSIX_MODE = False @@ -17,6 +21,7 @@ except ImportError: # pragma: no cover POSIX_MODE = True +logger = logging.getLogger(__name__) # For Linux: define OS specific standard binary directories STANDARD_BINARY_DIRS = ["/usr/sbin", "/usr/local/bin", "/usr/local/sbin"] if POSIX_MODE else [] @@ -109,3 +114,39 @@ def underscores_for_unsupported_characters_in_path(path): # Windows specific drive, tail = os.path.splitdrive(path) return drive + tail.replace(':', '_') + + +def execute_command(cmd_name, shell_cmd): + # type: (str, str) -> Tuple[str, str] + """ + Run a command: + - on Linux command will be run by the standard shell selected with Popen(shell=True) + - on Windows command will be run in a Powershell shell + + :param str cmd_name: the user facing name of the hook being run + :param str shell_cmd: shell command to execute + + :returns: `tuple` (`str` stderr, `str` stdout) + """ + logger.info("Running %s command: %s", cmd_name, shell_cmd) + + if POSIX_MODE: + cmd = subprocess.Popen(shell_cmd, shell=True, stdout=subprocess.PIPE, + stderr=subprocess.PIPE, universal_newlines=True) + else: + line = ['powershell.exe', '-Command', shell_cmd] + cmd = subprocess.Popen(line, stdout=subprocess.PIPE, stderr=subprocess.PIPE, + universal_newlines=True) + + # universal_newlines causes Popen.communicate() + # to return str objects instead of bytes in Python 3 + out, err = cmd.communicate() + base_cmd = os.path.basename(shell_cmd.split(None, 1)[0]) + if out: + logger.info('Output from %s command %s:\n%s', cmd_name, base_cmd, out) + if cmd.returncode != 0: + logger.error('%s command "%s" returned error code %d', + cmd_name, shell_cmd, cmd.returncode) + if err: + logger.error('Error output from %s command %s:\n%s', cmd_name, base_cmd, err) + return err, out diff --git a/certbot/tests/compat/misc_test.py b/certbot/tests/compat/misc_test.py new file mode 100644 index 000000000..642f395ba --- /dev/null +++ b/certbot/tests/compat/misc_test.py @@ -0,0 +1,48 @@ +"""Tests for certbot.compat.misc""" +try: + import mock +except ImportError: # pragma: no cover + from unittest import mock # type: ignore +import unittest + +from certbot.compat import os + + +class ExecuteTest(unittest.TestCase): + """Tests for certbot.compat.misc.execute_command.""" + + @classmethod + def _call(cls, *args, **kwargs): + from certbot.compat.misc import execute_command + return execute_command(*args, **kwargs) + + def test_it(self): + for returncode in range(0, 2): + for stdout in ("", "Hello World!",): + for stderr in ("", "Goodbye Cruel World!"): + self._test_common(returncode, stdout, stderr) + + def _test_common(self, returncode, stdout, stderr): + given_command = "foo" + given_name = "foo-hook" + with mock.patch("certbot.compat.misc.subprocess.Popen") as mock_popen: + mock_popen.return_value.communicate.return_value = (stdout, stderr) + mock_popen.return_value.returncode = returncode + with mock.patch("certbot.compat.misc.logger") as mock_logger: + self.assertEqual(self._call(given_name, given_command), (stderr, stdout)) + + executed_command = mock_popen.call_args[1].get( + "args", mock_popen.call_args[0][0]) + if os.name == 'nt': + expected_command = ['powershell.exe', '-Command', given_command] + else: + expected_command = given_command + self.assertEqual(executed_command, expected_command) + + mock_logger.info.assert_any_call("Running %s command: %s", + given_name, given_command) + if stdout: + mock_logger.info.assert_any_call(mock.ANY, mock.ANY, + mock.ANY, stdout) + if stderr or returncode: + self.assertTrue(mock_logger.error.called) diff --git a/certbot/tests/hook_test.py b/certbot/tests/hook_test.py index 32081f9d0..b8fd02461 100644 --- a/certbot/tests/hook_test.py +++ b/certbot/tests/hook_test.py @@ -72,13 +72,13 @@ class HookTest(test_util.ConfigTestCase): @classmethod def _call_with_mock_execute(cls, *args, **kwargs): - """Calls self._call after mocking out certbot._internal.hooks.execute. + """Calls self._call after mocking out certbot.compat.misc.execute_command. The mock execute object is returned rather than the return value of self._call. """ - with mock.patch("certbot._internal.hooks.execute") as mock_execute: + with mock.patch("certbot.compat.misc.execute_command") as mock_execute: mock_execute.return_value = ("", "") cls._call(*args, **kwargs) return mock_execute @@ -292,7 +292,7 @@ class RenewalHookTest(HookTest): # pylint: disable=abstract-method def _call_with_mock_execute(self, *args, **kwargs): - """Calls self._call after mocking out certbot._internal.hooks.execute. + """Calls self._call after mocking out certbot.compat.misc.execute_command. The mock execute object is returned rather than the return value of self._call. The mock execute object asserts that environment @@ -313,7 +313,7 @@ class RenewalHookTest(HookTest): self.assertEqual(os.environ["RENEWED_LINEAGE"], lineage) return ("", "") - with mock.patch("certbot._internal.hooks.execute") as mock_execute: + with mock.patch("certbot.compat.misc.execute_command") as mock_execute: mock_execute.side_effect = execute_side_effect self._call(*args, **kwargs) return mock_execute @@ -418,42 +418,6 @@ class RenewHookTest(RenewalHookTest): mock_execute.assert_called_with("deploy-hook", self.config.renew_hook) -class ExecuteTest(unittest.TestCase): - """Tests for certbot._internal.hooks.execute.""" - - @classmethod - def _call(cls, *args, **kwargs): - from certbot._internal.hooks import execute - return execute(*args, **kwargs) - - def test_it(self): - for returncode in range(0, 2): - for stdout in ("", "Hello World!",): - for stderr in ("", "Goodbye Cruel World!"): - self._test_common(returncode, stdout, stderr) - - def _test_common(self, returncode, stdout, stderr): - given_command = "foo" - given_name = "foo-hook" - with mock.patch("certbot._internal.hooks.Popen") as mock_popen: - mock_popen.return_value.communicate.return_value = (stdout, stderr) - mock_popen.return_value.returncode = returncode - with mock.patch("certbot._internal.hooks.logger") as mock_logger: - self.assertEqual(self._call(given_name, given_command), (stderr, stdout)) - - executed_command = mock_popen.call_args[1].get( - "args", mock_popen.call_args[0][0]) - self.assertEqual(executed_command, given_command) - - mock_logger.info.assert_any_call("Running %s command: %s", - given_name, given_command) - if stdout: - mock_logger.info.assert_any_call(mock.ANY, mock.ANY, - mock.ANY, stdout) - if stderr or returncode: - self.assertTrue(mock_logger.error.called) - - class ListHooksTest(test_util.TempDirTestCase): """Tests for certbot._internal.hooks.list_hooks.""" |