Use binary flag when writing cert and key files (#4462)

* Use binary flag when writing cert and key files

Add binary flag to mode argument when opening files for writing key and
certificate files.
On Python 3 the data buffers use for writing are bytes objects not
strings, and the write fails accordingly.
As far as I understand, it the "b" flag will not hurt things in Python 2
either.

* Update the tests for RenewableCert::save_successor

Update the tests for RenewableCert::save_successor after changing
three  parameters to be called with bytes objects instead of strings.

Also, update the doc comment of the function.

certbot/storage.py

@@ -1047,10 +1047,10 @@ class RenewableCert(object):
             is regarded as a successor (used to choose a privkey, if the
             key has not changed, but otherwise this information is not
             permanently recorded anywhere)
-        :param str new_cert: the new certificate, in PEM format
+        :param bytes new_cert: the new certificate, in PEM format
-        :param str new_privkey: the new private key, in PEM format,
+        :param bytes new_privkey: the new private key, in PEM format,
             or ``None``, if the private key has not changed
-        :param str new_chain: the new chain, in PEM format
+        :param bytes new_chain: the new chain, in PEM format
         :param .NamespaceConfig cli_config: parsed command line
             arguments
 
@@ -1086,18 +1086,18 @@ class RenewableCert(object):
             logger.debug("Writing symlink to old private key, %s.", old_privkey)
             os.symlink(old_privkey, target["privkey"])
         else:
-            with open(target["privkey"], "w") as f:
+            with open(target["privkey"], "wb") as f:
                 logger.debug("Writing new private key to %s.", target["privkey"])
                 f.write(new_privkey)
 
         # Save everything else
-        with open(target["cert"], "w") as f:
+        with open(target["cert"], "wb") as f:
             logger.debug("Writing certificate to %s.", target["cert"])
             f.write(new_cert)
-        with open(target["chain"], "w") as f:
+        with open(target["chain"], "wb") as f:
             logger.debug("Writing chain to %s.", target["chain"])
             f.write(new_chain)
-        with open(target["fullchain"], "w") as f:
+        with open(target["fullchain"], "wb") as f:
             logger.debug("Writing full chain to %s.", target["fullchain"])
             f.write(new_cert + new_chain)
 

certbot/tests/storage_test.py

@@ -489,8 +489,8 @@ class RenewableCertTests(BaseRenewableCertTest):
                 self._write_out_kind(kind, ver)
         self.test_rc.update_all_links_to(3)
         self.assertEqual(
-            6, self.test_rc.save_successor(3, "new cert", None,
+            6, self.test_rc.save_successor(3, b'new cert', None,
-                                           "new chain", self.cli_config))
+                                           b'new chain', self.cli_config))
         with open(self.test_rc.version("cert", 6)) as f:
             self.assertEqual(f.read(), "new cert")
         with open(self.test_rc.version("chain", 6)) as f:
@@ -502,11 +502,11 @@ class RenewableCertTests(BaseRenewableCertTest):
         self.assertTrue(os.path.islink(self.test_rc.version("privkey", 6)))
         # Let's try two more updates
         self.assertEqual(
-            7, self.test_rc.save_successor(6, "again", None,
+            7, self.test_rc.save_successor(6, b'again', None,
-                                           "newer chain", self.cli_config))
+                                           b'newer chain', self.cli_config))
         self.assertEqual(
-            8, self.test_rc.save_successor(7, "hello", None,
+            8, self.test_rc.save_successor(7, b'hello', None,
-                                           "other chain", self.cli_config))
+                                           b'other chain', self.cli_config))
         # All of the subsequent versions should link directly to the original
         # privkey.
         for i in (6, 7, 8):
@@ -520,8 +520,8 @@ class RenewableCertTests(BaseRenewableCertTest):
         # Test updating from latest version rather than old version
         self.test_rc.update_all_links_to(8)
         self.assertEqual(
-            9, self.test_rc.save_successor(8, "last", None,
+            9, self.test_rc.save_successor(8, b'last', None,
-                                           "attempt", self.cli_config))
+                                           b'attempt', self.cli_config))
         for kind in ALL_FOUR:
             self.assertEqual(self.test_rc.available_versions(kind),
                              list(six.moves.range(1, 10)))
@@ -535,8 +535,8 @@ class RenewableCertTests(BaseRenewableCertTest):
         # Test updating when providing a new privkey.  The key should
         # be saved in a new file rather than creating a new symlink.
         self.assertEqual(
-            10, self.test_rc.save_successor(9, "with", "a",
+            10, self.test_rc.save_successor(9, b'with', b'a',
-                                            "key", self.cli_config))
+                                            b'key', self.cli_config))
         self.assertTrue(os.path.exists(self.test_rc.version("privkey", 10)))
         self.assertFalse(os.path.islink(self.test_rc.version("privkey", 10)))
         self.assertFalse(os.path.exists(temp_config_file))