Change default privkey permissions while preserving group permissions (#6480)

Fixes #1473.

writes privkey.pem to 0600 by default for new lineages
on renewals where a new privkey is generated, preserves group mode and gid
Things this PR does not do:

we talked about forcing 0600 on privkeys when a Certbot upgrade is detected. Instead, this PR only creates new lineages with the more restrictive permission to prevent renewal breakages.
this doesn't solve many of the problems mentioned in #1473 that are not directly related to the title issue!

* safe_open on archive keyfiles

* keep group from current lineage

* clean up integration test

* safe_open can follow symlinks

* fix tests on windows, maybe

* Address Brad's comments

* Revert changes to safe_open
* Test chown is called when saving new key
* Reorder chown operation

* Changelog and documentation

* Fix documentation style

CHANGELOG.md

@@ -12,7 +12,9 @@ Certbot adheres to [Semantic Versioning](http://semver.org/).
 
 ### Changed
 
-*
+* Private key permissioning changes: Renewal preserves existing group mode
+  & gid of previous private key material. Private keys for new
+  lineages (i.e. new certs, not renewed) default to 0o600.
 
 ### Fixed
 

certbot/storage.py

@@ -29,6 +29,7 @@ logger = logging.getLogger(__name__)
 ALL_FOUR = ("cert", "privkey", "chain", "fullchain")
 README = "README"
 CURRENT_VERSION = util.get_strict_version(certbot.__version__)
+BASE_PRIVKEY_MODE = 0o600
 
 
 def renewal_conf_files(config):
@@ -1050,13 +1051,14 @@ class RenewableCert(object):
         # Put the data into the appropriate files on disk
         target = dict([(kind, os.path.join(live_dir, kind + ".pem"))
                        for kind in ALL_FOUR])
+        archive_target = dict([(kind, os.path.join(archive, kind + "1.pem"))
+                               for kind in ALL_FOUR])
         for kind in ALL_FOUR:
-            os.symlink(os.path.join(_relpath_from_file(archive, target[kind]), kind + "1.pem"),
+            os.symlink(_relpath_from_file(archive_target[kind], target[kind]), target[kind])
-                       target[kind])
         with open(target["cert"], "wb") as f:
             logger.debug("Writing certificate to %s.", target["cert"])
             f.write(cert)
-        with open(target["privkey"], "wb") as f:
+        with util.safe_open(archive_target["privkey"], "wb", chmod=BASE_PRIVKEY_MODE) 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
@@ -1120,14 +1122,15 @@ class RenewableCert(object):
               os.path.join(self.archive_dir, "{0}{1}.pem".format(kind, target_version)))
              for kind in ALL_FOUR])
 
+        old_privkey = os.path.join(
+            self.archive_dir, "privkey{0}.pem".format(prior_version))
+
         # Distinguish the cases where the privkey has changed and where it
         # has not changed (in the latter case, making an appropriate symlink
         # to an earlier privkey version)
         if new_privkey is None:
             # The behavior below keeps the prior key by creating a new
             # symlink to the old key or the target of the old key symlink.
-            old_privkey = os.path.join(
-                self.archive_dir, "privkey{0}.pem".format(prior_version))
             if os.path.islink(old_privkey):
                 old_privkey = os.readlink(old_privkey)
             else:
@@ -1135,9 +1138,16 @@ 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"], "wb") as f:
+            with util.safe_open(target["privkey"], "wb", chmod=BASE_PRIVKEY_MODE) as f:
                 logger.debug("Writing new private key to %s.", target["privkey"])
                 f.write(new_privkey)
+            # If the previous privkey in this lineage has an existing gid or group mode > 0,
+            # let's keep those.
+            group_mode = stat.S_IMODE(os.stat(old_privkey).st_mode) & \
+                (stat.S_IRGRP | stat.S_IWGRP | stat.S_IXGRP)
+            mode = BASE_PRIVKEY_MODE | group_mode
+            os.chown(target["privkey"], -1, os.stat(old_privkey).st_gid)
+            os.chmod(target["privkey"], mode)
 
         # Save everything else
         with open(target["cert"], "wb") as f:

certbot/tests/storage_test.py

@@ -13,6 +13,7 @@ import six
 
 import certbot
 from certbot import cli
+from certbot import compat
 from certbot import errors
 from certbot.storage import ALL_FOUR
 
@@ -91,6 +92,8 @@ class BaseRenewableCertTest(test_util.ConfigTestCase):
                    link)
         with open(link, "wb") as f:
             f.write(kind.encode('ascii') if value is None else value)
+        if kind == "privkey":
+            os.chmod(link, 0o600)
 
     def _write_out_ex_kinds(self):
         for kind in ALL_FOUR:
@@ -543,6 +546,47 @@ class RenewableCertTests(BaseRenewableCertTest):
         self.assertFalse(os.path.islink(self.test_rc.version("privkey", 10)))
         self.assertFalse(os.path.exists(temp_config_file))
 
+    @test_util.broken_on_windows
+    @mock.patch("certbot.storage.relevant_values")
+    def test_save_successor_maintains_group_mode(self, mock_rv):
+        # Mock relevant_values() to claim that all values are relevant here
+        # (to avoid instantiating parser)
+        mock_rv.side_effect = lambda x: x
+        for kind in ALL_FOUR:
+            self._write_out_kind(kind, 1)
+        self.test_rc.update_all_links_to(1)
+        self.assertTrue(compat.compare_file_modes(
+            os.stat(self.test_rc.version("privkey", 1)).st_mode, 0o600))
+        os.chmod(self.test_rc.version("privkey", 1), 0o444)
+        # If no new key, permissions should be the same (we didn't write any keys)
+        self.test_rc.save_successor(1, b"newcert", None, b"new chain", self.config)
+        self.assertTrue(compat.compare_file_modes(
+            os.stat(self.test_rc.version("privkey", 2)).st_mode, 0o444))
+        # If new key, permissions should be rest to 600 + preserved group
+        self.test_rc.save_successor(2, b"newcert", b"new_privkey", b"new chain", self.config)
+        self.assertTrue(compat.compare_file_modes(
+            os.stat(self.test_rc.version("privkey", 3)).st_mode, 0o640))
+        # If permissions reverted, next renewal will also revert permissions of new key
+        os.chmod(self.test_rc.version("privkey", 3), 0o404)
+        self.test_rc.save_successor(3, b"newcert", b"new_privkey", b"new chain", self.config)
+        self.assertTrue(compat.compare_file_modes(
+            os.stat(self.test_rc.version("privkey", 4)).st_mode, 0o600))
+
+    @test_util.broken_on_windows
+    @mock.patch("certbot.storage.relevant_values")
+    @mock.patch("certbot.storage.os.chown")
+    def test_save_successor_maintains_gid(self, mock_chown, mock_rv):
+        # Mock relevant_values() to claim that all values are relevant here
+        # (to avoid instantiating parser)
+        mock_rv.side_effect = lambda x: x
+        for kind in ALL_FOUR:
+            self._write_out_kind(kind, 1)
+        self.test_rc.update_all_links_to(1)
+        self.test_rc.save_successor(1, b"newcert", None, b"new chain", self.config)
+        self.assertFalse(mock_chown.called)
+        self.test_rc.save_successor(2, b"newcert", b"new_privkey", b"new chain", self.config)
+        self.assertTrue(mock_chown.called)
+
     def _test_relevant_values_common(self, values):
         defaults = dict((option, cli.flag_default(option))
                         for option in ("authenticator", "installer",
@@ -629,6 +673,7 @@ class RenewableCertTests(BaseRenewableCertTest):
             self.config.live_dir, "README")))
         self.assertTrue(os.path.exists(os.path.join(
             self.config.live_dir, "the-lineage.com", "README")))
+        self.assertTrue(compat.compare_file_modes(os.stat(result.key_path).st_mode, 0o600))
         with open(result.fullchain, "rb") as f:
             self.assertEqual(f.read(), b"cert" + b"chain")
         # Let's do it again and make sure it makes a different lineage

docs/using.rst

@@ -717,6 +717,10 @@ The following files are available:
      put it into a safe, however - your server still needs to access
      this file in order for SSL/TLS to work.
 
+  .. note:: As of Certbot version 0.29.0, private keys for new certificate
+     default to ``0600``. Any changes to the group mode or group owner (gid)
+     of this file will be preserved on renewals.
+
   This is what Apache needs for `SSLCertificateKeyFile
   <https://httpd.apache.org/docs/2.4/mod/mod_ssl.html#sslcertificatekeyfile>`_,
   and Nginx for `ssl_certificate_key

tests/certbot-boulder-integration.sh

@@ -280,7 +280,29 @@ CheckCertCount() {
     fi
 }
 
+CheckGroup() {
+    group_mode() { echo $((0`stat -c %a $1` & 070)); }
+    group_owner() { echo `stat -c %G $1`; }
+    if [ `group_mode $1` -ne `group_mode $2` ] ; then
+        echo "Expected group permission `group_mode $1`, got `group_mode $2` on file $2"
+        exit 1
+    fi
+    if [ `group_owner $1` != `group_owner $2` ] ; then
+        echo "Expected group owner `group_owner $1`, got `group_owner $2` on file $2"
+        exit 1
+    fi
+}
+
+CheckOthersPermission() {
+    other_permission=$((0`stat -c %a $1` & 07))
+    if [ $other_permission -ne 0 ] ; then
+        echo "Expected file $1 not to be accessible by others"
+        exit 1
+    fi
+}
+
 CheckCertCount "le.wtf" 1
+
 # This won't renew (because it's not time yet)
 common_no_force_renew renew
 CheckCertCount "le.wtf" 1
@@ -294,6 +316,11 @@ rm -rf "$renewal_hooks_root"
 common renew --cert-name le.wtf --authenticator manual
 CheckCertCount "le.wtf" 2
 
+CheckOthersPermission "${root}/conf/archive/le.wtf/privkey1.pem"
+CheckOthersPermission "${root}/conf/archive/le.wtf/privkey2.pem"
+CheckGroup "${root}/conf/archive/le.wtf/privkey1.pem" "${root}/conf/archive/le.wtf/privkey2.pem"
+chmod 0444 "${root}/conf/archive/le.wtf/privkey2.pem"
+
 # test renewal with no executables in hook directories
 for hook_dir in $renewal_hooks_dirs; do
     touch "$hook_dir/file"
@@ -310,6 +337,9 @@ CreateDirHooks
 sed -i "4arenew_before_expiry = 4 years" "$root/conf/renewal/le.wtf.conf"
 common_no_force_renew renew --rsa-key-size 2048 --no-directory-hooks
 CheckCertCount "le.wtf" 3
+CheckGroup "${root}/conf/archive/le.wtf/privkey2.pem" "${root}/conf/archive/le.wtf/privkey3.pem"
+CheckOthersPermission "${root}/conf/archive/le.wtf/privkey3.pem"
+
 if [ -s "$HOOK_DIRS_TEST" ]; then
     echo "Directory hooks were executed with --no-directory-hooks!" >&2
     exit 1