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
certbot · Nov 29, 2018 · 7d0ac47 · 7d0ac47
1 parent 7527a6c
commit 7d0ac47
Show file tree

Hide file tree

Showing 5 changed files with 98 additions and 7 deletions.
diff --git a/CHANGELOG.md b/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
 

diff --git a/certbot/storage.py b/certbot/storage.py
@@ -29,6 +29,7 @@
 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 @@ def new_lineage(cls, lineagename, cert, privkey, chain, cli_config):
  # 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"),
- target[kind])
+ os.symlink(_relpath_from_file(archive_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,24 +1122,32 @@ def save_successor(self, prior_version, new_cert,
  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:
  old_privkey = "privkey{0}.pem".format(prior_version)
  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:

diff --git a/certbot/tests/storage_test.py b/certbot/tests/storage_test.py
@@ -13,6 +13,7 @@
 
 import certbot
 from certbot import cli
+from certbot import compat
 from certbot import errors
 from certbot.storage import ALL_FOUR
 
@@ -91,6 +92,8 @@ def _write_out_kind(self, kind, ver, value=None):
  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 @@ def test_save_successor(self, mock_rv):
  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 @@ def test_new_lineage(self, mock_rv):
  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

diff --git a/docs/using.rst b/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

diff --git a/tests/certbot-boulder-integration.sh b/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