Fix writing pem files with Python3 (#3757)

* Standardize arguments name for mode and chmod in the util API

* Handle OpenSSL pem as bytes objects only for Python3 compatibility

* Handle OpenSSL pem as bytes objects only (remaining bits)

* Manipulate bytes objects only when testing PEM-related functions

* Fix argument order when calling util.unique_file

certbot-nginx/certbot_nginx/configurator.py

@@ -405,7 +405,8 @@ class NginxConfigurator(common.Plugin):
         cert = acme_crypto_util.gen_ss_cert(key, domains=[socket.gethostname()])
         cert_pem = OpenSSL.crypto.dump_certificate(
             OpenSSL.crypto.FILETYPE_PEM, cert)
-        cert_file, cert_path = util.unique_file(os.path.join(tmp_dir, "cert.pem"))
+        cert_file, cert_path = util.unique_file(
+            os.path.join(tmp_dir, "cert.pem"), mode="wb")
         with cert_file:
             cert_file.write(cert_pem)
         return cert_path, le_key.file

certbot/client.py

@@ -322,7 +322,7 @@ class Client(object):
                 self.config.strict_permissions)
 
         cert_pem = OpenSSL.crypto.dump_certificate(
-            OpenSSL.crypto.FILETYPE_PEM, certr.body.wrapped).decode('ascii')
+            OpenSSL.crypto.FILETYPE_PEM, certr.body.wrapped)
 
         cert_file, abs_cert_path = _open_pem_file('cert_path', cert_path)
 
@@ -595,10 +595,10 @@ def _open_pem_file(cli_arg_path, pem_path):
 
     """
     if cli.set_by_cli(cli_arg_path):
-        return util.safe_open(pem_path, chmod=0o644),\
+        return util.safe_open(pem_path, chmod=0o644, mode="wb"),\
             os.path.abspath(pem_path)
     else:
-        uniq = util.unique_file(pem_path, 0o644)
+        uniq = util.unique_file(pem_path, 0o644, "wb")
         return uniq[0], os.path.abspath(uniq[1])
 
 def _save_chain(chain_pem, chain_file):

certbot/crypto_util.py

@@ -53,7 +53,8 @@ def init_save_key(key_size, key_dir, keyname="key-certbot.pem"):
     # Save file
     util.make_or_verify_dir(key_dir, 0o700, os.geteuid(),
                             config.strict_permissions)
-    key_f, key_path = util.unique_file(os.path.join(key_dir, keyname), 0o600)
+    key_f, key_path = util.unique_file(
+        os.path.join(key_dir, keyname), 0o600, "wb")
     with key_f:
         key_f.write(key_pem)
 
@@ -85,7 +86,7 @@ def init_save_csr(privkey, names, path, csrname="csr-certbot.pem"):
     util.make_or_verify_dir(path, 0o755, os.geteuid(),
                                config.strict_permissions)
     csr_f, csr_filename = util.unique_file(
-        os.path.join(path, csrname), 0o644)
+        os.path.join(path, csrname), 0o644, "wb")
     csr_f.write(csr_pem)
     csr_f.close()
 
@@ -351,11 +352,11 @@ def dump_pyopenssl_chain(chain, filetype=OpenSSL.crypto.FILETYPE_PEM):
         if isinstance(cert, jose.ComparableX509):
             # pylint: disable=protected-access
             cert = cert.wrapped
-        return OpenSSL.crypto.dump_certificate(filetype, cert).decode('ascii')
+        return OpenSSL.crypto.dump_certificate(filetype, cert)
 
     # assumes that OpenSSL.crypto.dump_certificate includes ending
     # newline character
-    return "".join(_dump_cert(cert) for cert in chain)
+    return b"".join(_dump_cert(cert) for cert in chain)
 
 
 def notBefore(cert_path):

certbot/storage.py

@@ -817,17 +817,17 @@ class RenewableCert(object):  # pylint: disable=too-many-instance-attributes
         for kind in ALL_FOUR:
             os.symlink(os.path.join(archive, kind + "1.pem"),
                        target[kind])
-        with open(target["cert"], "w") as f:
+        with open(target["cert"], "wb") as f:
             logger.debug("Writing certificate to %s.", target["cert"])
             f.write(cert)
-        with open(target["privkey"], "w") as f:
+        with open(target["privkey"], "wb") as f:
             logger.debug("Writing private key to %s.", target["privkey"])
             f.write(privkey)
             # XXX: Let's make sure to get the file permissions right here
-        with open(target["chain"], "w") as f:
+        with open(target["chain"], "wb") as f:
             logger.debug("Writing chain to %s.", target["chain"])
             f.write(chain)
-        with open(target["fullchain"], "w") as f:
+        with open(target["fullchain"], "wb") as f:
             # assumes that OpenSSL.crypto.dump_certificate includes
             # ending newline character
             logger.debug("Writing full chain to %s.", target["fullchain"])

certbot/tests/crypto_util_test.py

@@ -40,9 +40,9 @@ class InitSaveKeyTest(unittest.TestCase):
 
     @mock.patch('certbot.crypto_util.make_key')
     def test_success(self, mock_make):
-        mock_make.return_value = 'key_pem'
+        mock_make.return_value = b'key_pem'
         key = self._call(1024, self.key_dir)
-        self.assertEqual(key.pem, 'key_pem')
+        self.assertEqual(key.pem, b'key_pem')
         self.assertTrue('key-certbot.pem' in key.file)
 
     @mock.patch('certbot.crypto_util.make_key')
@@ -67,13 +67,13 @@ class InitSaveCSRTest(unittest.TestCase):
     def test_it(self, unused_mock_verify, mock_csr):
         from certbot.crypto_util import init_save_csr
 
-        mock_csr.return_value = ('csr_pem', 'csr_der')
+        mock_csr.return_value = (b'csr_pem', b'csr_der')
 
         csr = init_save_csr(
             mock.Mock(pem='dummy_key'), 'example.com', self.csr_dir,
             'csr-certbot.pem')
 
-        self.assertEqual(csr.data, 'csr_der')
+        self.assertEqual(csr.data, b'csr_der')
         self.assertTrue('csr-certbot.pem' in csr.file)
 
 

certbot/tests/storage_test.py

@@ -573,18 +573,18 @@ class RenewableCertTests(BaseRenewableCertTest):
 
         from certbot import storage
         result = storage.RenewableCert.new_lineage(
-            "the-lineage.com", "cert", "privkey", "chain", self.cli_config)
+            "the-lineage.com", b"cert", b"privkey", b"chain", self.cli_config)
         # This consistency check tests most relevant properties about the
         # newly created cert lineage.
         # pylint: disable=protected-access
         self.assertTrue(result._consistent())
         self.assertTrue(os.path.exists(os.path.join(
             self.cli_config.renewal_configs_dir, "the-lineage.com.conf")))
-        with open(result.fullchain) as f:
+        with open(result.fullchain, "rb") as f:
-            self.assertEqual(f.read(), "cert" + "chain")
+            self.assertEqual(f.read(), b"cert" + b"chain")
         # Let's do it again and make sure it makes a different lineage
         result = storage.RenewableCert.new_lineage(
-            "the-lineage.com", "cert2", "privkey2", "chain2", self.cli_config)
+            "the-lineage.com", b"cert2", b"privkey2", b"chain2", self.cli_config)
         self.assertTrue(os.path.exists(os.path.join(
             self.cli_config.renewal_configs_dir, "the-lineage.com-0001.conf")))
         # Now trigger the detection of already existing files
@@ -592,15 +592,15 @@ class RenewableCertTests(BaseRenewableCertTest):
             self.cli_config.live_dir, "the-lineage.com-0002"))
         self.assertRaises(errors.CertStorageError,
                           storage.RenewableCert.new_lineage, "the-lineage.com",
-                          "cert3", "privkey3", "chain3", self.cli_config)
+                          b"cert3", b"privkey3", b"chain3", self.cli_config)
         os.mkdir(os.path.join(self.cli_config.default_archive_dir, "other-example.com"))
         self.assertRaises(errors.CertStorageError,
                           storage.RenewableCert.new_lineage,
-                          "other-example.com", "cert4",
+                          "other-example.com", b"cert4",
-                          "privkey4", "chain4", self.cli_config)
+                          b"privkey4", b"chain4", self.cli_config)
         # Make sure it can accept renewal parameters
         result = storage.RenewableCert.new_lineage(
-            "the-lineage.com", "cert2", "privkey2", "chain2", self.cli_config)
+            "the-lineage.com", b"cert2", b"privkey2", b"chain2", self.cli_config)
         # TODO: Conceivably we could test that the renewal parameters actually
         #       got saved
 
@@ -617,7 +617,7 @@ class RenewableCertTests(BaseRenewableCertTest):
         shutil.rmtree(self.cli_config.live_dir)
 
         storage.RenewableCert.new_lineage(
-            "the-lineage.com", "cert2", "privkey2", "chain2", self.cli_config)
+            "the-lineage.com", b"cert2", b"privkey2", b"chain2", self.cli_config)
         self.assertTrue(os.path.exists(
             os.path.join(
                 self.cli_config.renewal_configs_dir, "the-lineage.com.conf")))

certbot/util.py

@@ -151,11 +151,11 @@ def safe_open(path, mode="w", chmod=None, buffering=None):
         mode, *fdopen_args)
 
 
-def _unique_file(path, filename_pat, count, mode):
+def _unique_file(path, filename_pat, count, chmod, mode):
     while True:
         current_path = os.path.join(path, filename_pat(count))
         try:
-            return safe_open(current_path, chmod=mode),\
+            return safe_open(current_path, chmod=chmod, mode=mode),\
                 os.path.abspath(current_path)
         except OSError as err:
             # "File exists," is okay, try a different name.
@@ -164,11 +164,12 @@ def _unique_file(path, filename_pat, count, mode):
         count += 1
 
 
-def unique_file(path, mode=0o777):
+def unique_file(path, chmod=0o777, mode="w"):
     """Safely finds a unique file.
 
     :param str path: path/filename.ext
-    :param int mode: File mode
+    :param int chmod: File mode
+    :param str mode: Open mode
 
     :returns: tuple of file object and file name
 
@@ -176,15 +177,16 @@ def unique_file(path, mode=0o777):
     path, tail = os.path.split(path)
     return _unique_file(
         path, filename_pat=(lambda count: "%04d_%s" % (count, tail)),
-        count=0, mode=mode)
+        count=0, chmod=chmod, mode=mode)
 
 
-def unique_lineage_name(path, filename, mode=0o777):
+def unique_lineage_name(path, filename, chmod=0o777, mode="w"):
     """Safely finds a unique file using lineage convention.
 
     :param str path: directory path
     :param str filename: proposed filename
-    :param int mode: file mode
+    :param int chmod: file mode
+    :param str mode: open mode
 
     :returns: tuple of file object and file name (which may be modified
         from the requested one by appending digits to ensure uniqueness)
@@ -196,13 +198,13 @@ def unique_lineage_name(path, filename, mode=0o777):
     """
     preferred_path = os.path.join(path, "%s.conf" % (filename))
     try:
-        return safe_open(preferred_path, chmod=mode), preferred_path
+        return safe_open(preferred_path, chmod=chmod), preferred_path
     except OSError as err:
         if err.errno != errno.EEXIST:
             raise
     return _unique_file(
         path, filename_pat=(lambda count: "%s-%04d.conf" % (filename, count)),
-        count=1, mode=mode)
+        count=1, chmod=chmod, mode=mode)
 
 
 def safely_remove(path):