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

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)

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()]

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:

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
 

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.
 

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))

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

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)

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."""