diff --git a/certbot/storage.py b/certbot/storage.py index c90a95a73..eb17e1d38 100644 --- a/certbot/storage.py +++ b/certbot/storage.py @@ -1141,11 +1141,11 @@ class RenewableCert(object): 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 + # Preserve gid and (mode & 074) from previous privkey in this lineage. + old_mode = stat.S_IMODE(os.stat(old_privkey).st_mode) & \ + (stat.S_IRGRP | stat.S_IWGRP | stat.S_IXGRP | \ + stat.S_IROTH) + mode = BASE_PRIVKEY_MODE | old_mode os.chown(target["privkey"], -1, os.stat(old_privkey).st_gid) os.chmod(target["privkey"], mode) diff --git a/certbot/tests/storage_test.py b/certbot/tests/storage_test.py index 5fd7746ed..d75f4f595 100644 --- a/certbot/tests/storage_test.py +++ b/certbot/tests/storage_test.py @@ -562,12 +562,12 @@ class RenewableCertTests(BaseRenewableCertTest): 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 + # If new key, permissions should be kept as 644 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)) + os.stat(self.test_rc.version("privkey", 3)).st_mode, 0o644)) # If permissions reverted, next renewal will also revert permissions of new key - os.chmod(self.test_rc.version("privkey", 3), 0o404) + os.chmod(self.test_rc.version("privkey", 3), 0o400) 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)) diff --git a/tests/certbot-boulder-integration.sh b/tests/certbot-boulder-integration.sh index 709daa217..87e3cc319 100755 --- a/tests/certbot-boulder-integration.sh +++ b/tests/certbot-boulder-integration.sh @@ -280,13 +280,20 @@ 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" +CheckPermissions() { +# Args: +# Checks mode of two files match under + masked_mode() { echo $((0`stat -c %a $1` & 0$2)); } + if [ `masked_mode $1 $3` -ne `masked_mode $2 $3` ] ; then + echo "With $3 mask, expected mode `masked_mode $1 $3`, got `masked_mode $2 $3` on file $2" exit 1 fi +} + +CheckGID() { +# Args: +# Checks group owner of two files match + group_owner() { echo `stat -c %G $1`; } if [ `group_owner $1` != `group_owner $2` ] ; then echo "Expected group owner `group_owner $1`, got `group_owner $2` on file $2" exit 1 @@ -294,9 +301,11 @@ CheckGroup() { } CheckOthersPermission() { +# Args: +# Tests file's other/world permission against expected mode other_permission=$((0`stat -c %a $1` & 07)) - if [ $other_permission -ne 0 ] ; then - echo "Expected file $1 not to be accessible by others" + if [ $other_permission -ne $2 ] ; then + echo "Expected file $1 to have others mode $2, got $other_permission instead" exit 1 fi } @@ -316,9 +325,10 @@ 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" +CheckOthersPermission "${root}/conf/archive/le.wtf/privkey1.pem" 0 +CheckOthersPermission "${root}/conf/archive/le.wtf/privkey2.pem" 0 +CheckPermissions "${root}/conf/archive/le.wtf/privkey1.pem" "${root}/conf/archive/le.wtf/privkey2.pem" 074 +CheckGID "${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 @@ -337,8 +347,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" +CheckGID "${root}/conf/archive/le.wtf/privkey2.pem" "${root}/conf/archive/le.wtf/privkey3.pem" +CheckPermissions "${root}/conf/archive/le.wtf/privkey2.pem" "${root}/conf/archive/le.wtf/privkey3.pem" 074 +CheckOthersPermission "${root}/conf/archive/le.wtf/privkey3.pem" 04 if [ -s "$HOOK_DIRS_TEST" ]; then echo "Directory hooks were executed with --no-directory-hooks!" >&2