Repository: certbot/certbot
Pull request: Use UTF-8 encoding for nginx plugin
Based on and supersedes #6725, which was originally based on #5341.

Fixes #5337.
Closes #6725.

## Pull Request Checklist

- [x] If the change being made is to a [distributed component](https://certbot.eff.org/docs/contributing.html#code-components-and-layout), edit the `master` section of `certbot/CHANGELOG.md` to include a description of the change being made.
- [x] Include your name in `AUTHORS.md` if you like.

Problem statement:
Nginx plugin crashes when non-ascii configuration file read
## My operating system is (include version):

Arch Linux ARM 4.9.69-1-ARCH

## I installed Certbot with (certbot-auto, OS package manager, pip, etc):

OS package manager (`pacman -S certbot-nginx`)

## I ran this command and it produced this output:

I ran this command:

`sudo certbot --nginx`

and it produced this:

```
Saving debug log to /var/log/letsencrypt/letsencrypt.log
An unexpected error occurred:
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc2 in position 10453: ordinal not in range(128)
Please see the logfiles in /var/log/letsencrypt for more details.
```

## Certbot's behavior differed from what I expected because:

I found that the error comes from `/etc/nginx/mime.types` which was included in the default nginx package. If there is any non-ascii encoded file in the nginx configuration path, the same error occurs.

## Here is a Certbot log showing the issue:

<details>
<summary>Click to expand</summary>

```
2017-12-18 20:21:34,224:DEBUG:certbot.main:certbot version: 0.20.0
2017-12-18 20:21:34,235:DEBUG:certbot.main:Arguments: ['--nginx']
2017-12-18 20:21:34,243:DEBUG:certbot.main:Discovered plugins: PluginsRegistry(PluginEntryPoint#manual,PluginEntryPoint#nginx,PluginEntryPoint#null,PluginEntryPoint#standalone,PluginEntryPoint#webroot)
2017-12-18 20:21:34,417:DEBUG:certbot.log:Root logging level set at 20
2017-12-18 20:21:34,427:INFO:certbot.log:Saving debug log to /var/log/letsencrypt/letsencrypt.log
2017-12-18 20:21:34,434:DEBUG:certbot.plugins.selection:Requested authenticator nginx and installer nginx
2017-12-18 20:21:34,742:DEBUG:certbot.log:Exiting abnormally:
Traceback (most recent call last):
  File "/usr/bin/certbot", line 11, in <module>
    load_entry_point('certbot==0.20.0', 'console_scripts', 'certbot')()
  File "/usr/lib/python3.6/site-packages/certbot/main.py", line 861, in main
    return config.func(config, plugins)
  File "/usr/lib/python3.6/site-packages/certbot/main.py", line 685, in run
    installer, authenticator = plug_sel.choose_configurator_plugins(config, plugins, "run")
  File "/usr/lib/python3.6/site-packages/certbot/plugins/selection.py", line 189, in choose_configurator_plugins
    authenticator = installer = pick_configurator(config, req_inst, plugins)
  File "/usr/lib/python3.6/site-packages/certbot/plugins/selection.py", line 25, in pick_configurator
    (interfaces.IAuthenticator, interfaces.IInstaller))
  File "/usr/lib/python3.6/site-packages/certbot/plugins/selection.py", line 77, in pick_plugin
    verified.prepare()
  File "/usr/lib/python3.6/site-packages/certbot/plugins/disco.py", line 248, in prepare
    return [plugin_ep.prepare() for plugin_ep in six.itervalues(self._plugins)]
  File "/usr/lib/python3.6/site-packages/certbot/plugins/disco.py", line 248, in <listcomp>
    return [plugin_ep.prepare() for plugin_ep in six.itervalues(self._plugins)]
  File "/usr/lib/python3.6/site-packages/certbot/plugins/disco.py", line 130, in prepare
    self._initialized.prepare()
  File "/usr/lib/python3.6/site-packages/certbot_nginx/configurator.py", line 156, in prepare
    self.parser = parser.NginxParser(self.conf('server-root'))
  File "/usr/lib/python3.6/site-packages/certbot_nginx/parser.py", line 35, in __init__
    self.load()
  File "/usr/lib/python3.6/site-packages/certbot_nginx/parser.py", line 42, in load
    self._parse_recursively(self.config_root)
  File "/usr/lib/python3.6/site-packages/certbot_nginx/parser.py", line 63, in _parse_recursively
    self._parse_recursively(subentry[1])
  File "/usr/lib/python3.6/site-packages/certbot_nginx/parser.py", line 53, in _parse_recursively
    trees = self._parse_files(filepath)
  File "/usr/lib/python3.6/site-packages/certbot_nginx/parser.py", line 203, in _parse_files
    parsed = nginxparser.load(_file)
  File "/usr/lib/python3.6/site-packages/certbot_nginx/nginxparser.py", line 122, in load
    return loads(_file.read())
  File "/usr/lib/python3.6/encodings/ascii.py", line 26, in decode
    return codecs.ascii_decode(input, self.errors)[0]
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc2 in position 10453: ordinal not in range(128)
2017-12-18 20:21:34,759:ERROR:certbot.log:An unexpected error occurred:
```

</details>

Hints from issue discussion:
And this is how I fixed the issue: https://github.com/g6123/certbot/commit/e74505968ba24a364a5be70ef86bbe20620d17db#diff-b2dab7e6fe39d3a1bf642d561d045633.
This looks to be essentially a duplicate of #5236
The parent issue for all of these UTF-8 problems is #4516
I think we'd be happy to take a PR of the changes you made, but should also prioritize making progress on total UTF-8 compliance
Okay, I went through the relavant issues you linked. The whole UTF-8 compatibility issues seem more complicated than I thought, so I'd like to make a new PR just for this little change first, and then look into the other codebase when I'm afford to.
I opened the PR #5341. Thank you for the comment, @SwartzCr :)
@ohemorange ... any progress on this? I found the issue is to do with when it reads "/etc/nginx/mime.types" ... as it does this without the correct encoding.  (line 205 of parser.py) ... the only solution I can think of is that if it throws opening as the default encoding then to retry using utf8. Dunno what your thoughts are?

I've ended up doing a real "bodge":
```python
            try:
                encoding = "ascii"
                if item == "/etc/nginx/mime.types":
                    encoding = "utf8"
                with open(item, encoding=encoding) as _file:
```
@diversemix working on a larger overhaul to fix these problems in the codebase. keep an eye out!
What is the progress to this issue, if i may ask?
Follow along with our progress here: https://github.com/certbot/certbot/projects/2
I'm currently having this same issue, and it's reproducible in the Python interpreter: ![](https://outlaw-techno.psychobitch.club/i/5mn5.png)

e: I tried @diversemix's solution and certbot issued a cert successfully.
Hi! The reason is that if you have some non ascii letters in nginx config (even in comments!!!) it will not work.

Commit message: Add simple comments

Patch to review:
diff --git a/AUTHORS.md b/AUTHORS.md
index 80a24d3be94..8653382b8ed 100644
--- a/AUTHORS.md
+++ b/AUTHORS.md
@@ -268,3 +268,4 @@ Authors
 * [YourDaddyIsHere](https://github.com/YourDaddyIsHere)
 * [Zach Shepherd](https://github.com/zjs)
 * [陈三](https://github.com/chenxsan)
+* [Yuseong Cho](https://github.com/g6123)
diff --git a/certbot-nginx/certbot_nginx/_internal/parser.py b/certbot-nginx/certbot_nginx/_internal/parser.py
index f71d7c018fb..0c1151826c1 100644
--- a/certbot-nginx/certbot_nginx/_internal/parser.py
+++ b/certbot-nginx/certbot_nginx/_internal/parser.py
@@ -2,6 +2,7 @@
 import copy
 import functools
 import glob
+import io
 import logging
 import re
 
@@ -205,12 +206,16 @@ def _parse_files(self, filepath, override=False):
             if item in self.parsed and not override:
                 continue
             try:
-                with open(item) as _file:
+                with io.open(item, "r", encoding="utf-8") as _file:
                     parsed = nginxparser.load(_file)
                     self.parsed[item] = parsed
                     trees.append(parsed)
             except IOError:
                 logger.warning("Could not open file: %s", item)
+            except UnicodeDecodeError:
+                logger.warning("Could not read file: %s due to invalid "
+                               "character. Only UTF-8 encoding is "
+                               "supported.", item)
             except pyparsing.ParseException as err:
                 logger.debug("Could not parse file: %s due to %s", item, err)
         return trees
@@ -414,10 +419,13 @@ def duplicate_vhost(self, vhost_template, remove_singleton_listen_params=False,
 def _parse_ssl_options(ssl_options):
     if ssl_options is not None:
         try:
-            with open(ssl_options) as _file:
+            with io.open(ssl_options, "r", encoding="utf-8") as _file:
                 return nginxparser.load(_file)
         except IOError:
             logger.warning("Missing NGINX TLS options file: %s", ssl_options)
+        except UnicodeDecodeError:
+            logger.warning("Could not read file: %s due to invalid character. "
+                           "Only UTF-8 encoding is supported.", ssl_options)
         except pyparsing.ParseBaseException as err:
             logger.debug("Could not parse file: %s due to %s", ssl_options, err)
     return []
diff --git a/certbot-nginx/tests/parser_test.py b/certbot-nginx/tests/parser_test.py
index f3a5665c541..1f9e3c9962f 100644
--- a/certbot-nginx/tests/parser_test.py
+++ b/certbot-nginx/tests/parser_test.py
@@ -482,7 +482,45 @@ def test_duplicate_vhost_remove_ipv6only(self):
                 called = True
         self.assertTrue(called)
 
+    def test_valid_unicode_characters(self):
+        nparser = parser.NginxParser(self.config_path)
+        path = nparser.abs_path('valid_unicode_comments.conf')
+        parsed = nparser._parse_files(path)  # pylint: disable=protected-access
+        self.assertEqual(['server'], parsed[0][2][0])
+        self.assertEqual(['listen', '80'], parsed[0][2][1][3])
+
+    def test_invalid_unicode_characters(self):
+        with self.assertLogs() as log:
+            nparser = parser.NginxParser(self.config_path)
+            path = nparser.abs_path('invalid_unicode_comments.conf')
+            parsed = nparser._parse_files(path)  # pylint: disable=protected-access
+
+        self.assertEqual([], parsed)
+        self.assertTrue([
+            True
+            for output in log.output
+            if ('invalid character' in output) and ('UTF-8' in output)
+        ])
 
+    def test_valid_unicode_characters_in_ssl_options(self):
+        nparser = parser.NginxParser(self.config_path)
+        path = nparser.abs_path('valid_unicode_comments.conf')
+        parsed = parser._parse_ssl_options(path)  # pylint: disable=protected-access
+        self.assertEqual(['server'], parsed[2][0])
+        self.assertEqual(['listen', '80'], parsed[2][1][3])
+
+    def test_invalid_unicode_characters_in_ssl_options(self):
+        with self.assertLogs() as log:
+            nparser = parser.NginxParser(self.config_path)
+            path = nparser.abs_path('invalid_unicode_comments.conf')
+            parsed = parser._parse_ssl_options(path)  # pylint: disable=protected-access
+
+        self.assertEqual([], parsed)
+        self.assertTrue([
+            True
+            for output in log.output
+            if ('invalid character' in output) and ('UTF-8' in output)
+        ])
 
 if __name__ == "__main__":
     unittest.main()  # pragma: no cover
diff --git a/certbot-nginx/tests/test_log_util.py b/certbot-nginx/tests/test_log_util.py
new file mode 100644
index 00000000000..7aebf21511a
--- /dev/null
+++ b/certbot-nginx/tests/test_log_util.py
@@ -0,0 +1,125 @@
+"""Backport for `TestCase.assertLogs()`.
+
+Most of the idea and code are from CPython implementation.
+https://github.com/python/cpython/blob/b76518d43fb82ed9e5d27025d18c90a23d525c90/Lib/unittest/case.py
+"""
+import logging
+import collections
+
+__all__ = ['AssertLogsMixin']
+
+LoggingWatcher = collections.namedtuple('LoggingWatcher', ['records', 'output'])
+
+
+class CapturingHandler(logging.Handler):
+    """
+    A logging handler capturing all (raw and formatted) logging output.
+    """
+
+    def __init__(self):
+        super(CapturingHandler, self).__init__()
+        self.watcher = LoggingWatcher([], [])
+
+    def flush(self):
+        pass
+
+    def emit(self, record):
+        self.watcher.records.append(record)
+        self.watcher.output.append(self.format(record))
+
+
+
+class AssertLogsContext(object):
+    """
+    A context manager used to implement `TestCase.assertLogs()`.
+    """
+
+    LOGGING_FORMAT = '%(levelname)s:%(name)s:%(message)s'
+
+    def __init__(self, test_case, logger_name, level):
+        self.test_case = test_case
+
+        self.logger_name = logger_name
+        self.logger_states = None
+        self.logger = None
+
+        if level:
+            # pylint: disable=protected-access,no-member
+            try:
+                # In Python 3.x
+                name_to_level = logging._nameToLevel  # type: ignore
+            except AttributeError:
+                # In Python 2.7
+                name_to_level = logging._levelNames  # type: ignore
+
+            self.level = name_to_level.get(level, level)
+        else:
+            self.level = logging.INFO
+
+        self.watcher = None
+
+    def _save_logger_states(self):
+        self.logger_states = (self.logger.handlers[:], self.logger.level, self.logger.propagate)
+
+    def _restore_logger_states(self):
+        self.logger.handlers, self.logger.level, self.logger.propagate = self.logger_states
+
+    def __enter__(self):
+        if isinstance(self.logger_name, logging.Logger):
+            self.logger = self.logger_name
+        else:
+            self.logger = logging.getLogger(self.logger_name)
+
+        formatter = logging.Formatter(self.LOGGING_FORMAT)
+
+        handler = CapturingHandler()
+        handler.setFormatter(formatter)
+
+        self._save_logger_states()
+        self.logger.handlers = [handler]
+        self.logger.setLevel(self.level)
+        self.logger.propagate = False
+
+        self.watcher = handler.watcher
+        return handler.watcher
+
+    def __exit__(self, exc_type, exc_value, tb):
+        self._restore_logger_states()
+
+        if exc_type is not None:
+            # let unexpected exceptions pass through
+            return
+
+        if not self.watcher.records:
+            self._raiseFailure(
+                "no logs of level {} or higher triggered on {}"
+                .format(logging.getLevelName(self.level), self.logger.name))
+
+    def _raiseFailure(self, message):
+        message = self.test_case._formatMessage(None, message)  # pylint: disable=protected-access
+        raise self.test_case.failureException(message)
+
+
+class AssertLogsMixin(object):
+    """
+    A mixin that implements `TestCase.assertLogs()`.
+    """
+
+    def assertLogs(self, logger=None, level=None):
+        """Fail unless a log message of level *level* or higher is emitted
+        on *logger_name* or its children.  If omitted, *level* defaults to
+        INFO and *logger* defaults to the root logger.
+        This method must be used as a context manager, and will yield
+        a recording object with two attributes: `output` and `records`.
+        At the end of the context manager, the `output` attribute will
+        be a list of the matching formatted log messages and the
+        `records` attribute will be a list of the corresponding LogRecord
+        objects.
+        Example::
+            with self.assertLogs('foo', level='INFO') as cm:
+                logging.getLogger('foo').info('first message')
+                logging.getLogger('foo.bar').error('second message')
+            self.assertEqual(cm.output, ['INFO:foo:first message',
+                                         'ERROR:foo.bar:second message'])
+        """
+        return AssertLogsContext(self, logger, level)
diff --git a/certbot-nginx/tests/test_util.py b/certbot-nginx/tests/test_util.py
index 8dfd18637b3..4c9da84bdea 100644
--- a/certbot-nginx/tests/test_util.py
+++ b/certbot-nginx/tests/test_util.py
@@ -14,9 +14,10 @@
 from certbot.tests import util as test_util
 from certbot_nginx._internal import configurator
 from certbot_nginx._internal import nginxparser
+import test_log_util
 
 
-class NginxTest(test_util.ConfigTestCase):
+class NginxTest(test_log_util.AssertLogsMixin, test_util.ConfigTestCase):
 
     def setUp(self):
         super(NginxTest, self).setUp()
diff --git a/certbot-nginx/tests/testdata/etc_nginx/invalid_unicode_comments.conf b/certbot-nginx/tests/testdata/etc_nginx/invalid_unicode_comments.conf
new file mode 100644
index 00000000000..596044cc9b7
--- /dev/null
+++ b/certbot-nginx/tests/testdata/etc_nginx/invalid_unicode_comments.conf
@@ -0,0 +1,7 @@
+# This configuration file is saved with EUC-KR (a.k.a. cp949) encoding,
+# including some Korean alphabets.
+
+server {
+    # �ȳ��ϼ���. 80�� ��Ʈ���� ��û�� ��ٸ���.
+    listen	80;
+}
diff --git a/certbot-nginx/tests/testdata/etc_nginx/valid_unicode_comments.conf b/certbot-nginx/tests/testdata/etc_nginx/valid_unicode_comments.conf
new file mode 100644
index 00000000000..89c978b2e37
--- /dev/null
+++ b/certbot-nginx/tests/testdata/etc_nginx/valid_unicode_comments.conf
@@ -0,0 +1,9 @@
+# This configuration file is saved with valid UTF-8 encoding,
+# including some CJK alphabets.
+
+server {
+    # 안녕하세요. 80번 포트에서 요청을 기다린다.
+    # こんにちは。８０番ポートからリクエストを待つ。
+    # 你好。等待端口80上的请求。
+    listen	80;
+}
diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md
index 126b07eecba..837f48db2b4 100644
--- a/certbot/CHANGELOG.md
+++ b/certbot/CHANGELOG.md
@@ -17,7 +17,7 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 
 ### Fixed
 
-*
+* Fix nginx plguin crash when non-ASCII configuration file is being read
 
 More details about these changes can be found on our GitHub repo.
 


Code review comments:
diff_hunk: @@ -268,3 +268,4 @@ Authors
 * [YourDaddyIsHere](https://github.com/YourDaddyIsHere)
 * [Zach Shepherd](https://github.com/zjs)
 * [陈三](https://github.com/chenxsan)
+* [Yuseong Cho](https://github.com/g6123)
path: AUTHORS.md
text: @user1:
It looks like this should be alphabetical order (so, before Zach Shepherd).

@author:
I see. Fixed in 2ae090529e66a059e0e077d233f427583caa7c6c.

diff_hunk: @@ -482,7 +482,45 @@ def test_duplicate_vhost_remove_ipv6only(self):
                 called = True
         self.assertTrue(called)
 
+    def test_valid_unicode_characters(self):
+        nparser = parser.NginxParser(self.config_path)
+        path = nparser.abs_path('valid_unicode_comments.conf')
+        parsed = nparser._parse_files(path)  # pylint: disable=protected-access
+        self.assertEqual(['server'], parsed[0][2][0])
+        self.assertEqual(['listen', '80'], parsed[0][2][1][3])
+
+    def test_invalid_unicode_characters(self):
+        with self.assertLogs() as log:
+            nparser = parser.NginxParser(self.config_path)
+            path = nparser.abs_path('invalid_unicode_comments.conf')
+            parsed = nparser._parse_files(path)  # pylint: disable=protected-access
+
+        self.assertEqual([], parsed)
+        self.assertTrue([
path: certbot-nginx/tests/parser_test.py
text: @user1:
I don't think this use of `assertTrue` is valid:

First, the thing you're asserting is `True` is a list of booleans, which is not the right type to use here.

Second, if there are multiple items in `log.output` (I don't remember whether that can happen), you will get a `True` for each one that is right but no list entry for each one that is wrong.

I would say that you could do something like

`assertTrue(all(('invalid character' in output) and ('UTF-8' in output) for output in log.output))`

if you expect multiple items in `log.output` (or `any()` instead of `all()` if only one of them needs to have the intended output).

@author:
Oh this is definitely my mistake. I should've apply `any()` as you suggested. Fixed in a48907920885671ef5a57bc1cda556e90c5e0e20.

diff_hunk: @@ -17,7 +17,7 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 
 ### Fixed
 
-*
+* Fix nginx plguin crash when non-ASCII configuration file is being read
path: certbot/CHANGELOG.md
text: @user1:
should be "plugin"

@author:
🙈 Fixed in 2ae090529e66a059e0e077d233f427583caa7c6c.