From 01d07f86cd525c0f324074ee026faf6d7f179839 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 2 Aug 2023 13:15:43 +0200 Subject: [PATCH] Set permissions for files and directories created in PGDATA (#2781) Postgres supports two types of permissions: 1. owner only 2. group readable By default the first one is used because it provides better security. But, sometimes people want to run a backup tool with the user that is different from postgres. In this case the second option becomes very useful. Unfortunately it didn't work correctly because Patroni was creating files with owner access only permissions. This PR changes the behavior and permissions on files and directories that are created by Patroni will be calculated based on permissions of PGDATA. I.e., they will get group readable access when it is necessary. Close #1899 Close #1901 --- patroni/config.py | 3 ++ patroni/file_perm.py | 95 ++++++++++++++++++++++++++++++++++++ patroni/postgresql/config.py | 46 ++++++++++++++--- patroni/postgresql/slots.py | 12 +++-- tests/test_config.py | 1 + tests/test_file_perm.py | 33 +++++++++++++ tests/test_ha.py | 1 + tests/test_postgresql.py | 5 +- 8 files changed, 184 insertions(+), 12 deletions(-) create mode 100644 patroni/file_perm.py create mode 100644 tests/test_file_perm.py diff --git a/patroni/config.py b/patroni/config.py index facf16fd..d2770517 100644 --- a/patroni/config.py +++ b/patroni/config.py @@ -13,6 +13,7 @@ from . import PATRONI_ENV_PREFIX from .collections import CaseInsensitiveDict from .dcs import ClusterConfig, Cluster from .exceptions import ConfigParseError +from .file_perm import pg_perm from .postgresql.config import ConfigHandler from .utils import deep_compare, parse_bool, parse_int, patch_config @@ -275,11 +276,13 @@ class Config(object): if self._cache_needs_saving: tmpfile = fd = None try: + pg_perm.set_permissions_from_data_directory(self._data_dir) (fd, tmpfile) = tempfile.mkstemp(prefix=self.__CACHE_FILENAME, dir=self._data_dir) with os.fdopen(fd, 'w') as f: fd = None json.dump(self.dynamic_configuration, f) tmpfile = shutil.move(tmpfile, self._cache_file) + os.chmod(self._cache_file, pg_perm.file_create_mode) self._cache_needs_saving = False except Exception: logger.exception('Exception when saving file: %s', self._cache_file) diff --git a/patroni/file_perm.py b/patroni/file_perm.py new file mode 100644 index 00000000..ed9c4e67 --- /dev/null +++ b/patroni/file_perm.py @@ -0,0 +1,95 @@ +"""Helper object that helps with figuring out file and directory permissions based on permissions of PGDATA. + +:var logger: logger of this module. +:var pg_perm: instance of the :class:`__FilePermissions` object. +""" +import logging +import os +import stat + +logger = logging.getLogger(__name__) + + +class __FilePermissions: + """Helper class for managing permissions of directories and files under PGDATA. + + Execute :meth:`set_permissions_from_data_directory` to figure out which permissions should be used for files and + directories under PGDATA based on permissions of PGDATA root directory. + """ + + # Mode mask for data directory permissions that only allows the owner to + # read/write directories and files -- mask 077. + __PG_MODE_MASK_OWNER = stat.S_IRWXG | stat.S_IRWXO + + # Mode mask for data directory permissions that also allows group read/execute -- mask 027. + __PG_MODE_MASK_GROUP = stat.S_IWGRP | stat.S_IRWXO + + # Default mode for creating directories -- mode 700. + __PG_DIR_MODE_OWNER = stat.S_IRWXU + + # Mode for creating directories that allows group read/execute -- mode 750. + __PG_DIR_MODE_GROUP = stat.S_IRWXU | stat.S_IRGRP | stat.S_IXGRP + + # Default mode for creating files -- mode 600. + __PG_FILE_MODE_OWNER = stat.S_IRUSR | stat.S_IWUSR + + # Mode for creating files that allows group read -- mode 640. + __PG_FILE_MODE_GROUP = stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP + + def __init__(self) -> None: + """Create a :class:`__FilePermissions` object and set default permissions.""" + self.__set_owner_permissions() + self.__set_umask() + + def __set_umask(self) -> None: + """Set umask value based on calculations. + + .. note:: + Should only be called once either :meth:`__set_owner_permissions` + or :meth:`__set_group_permissions` has been executed. + """ + try: + os.umask(self.__pg_mode_mask) + except Exception as e: + logger.error('Can not set umask to %03o: %r', self.__pg_mode_mask, e) + + def __set_owner_permissions(self) -> None: + """Make directories/files accessible only by the owner.""" + self.__pg_dir_create_mode = self.__PG_DIR_MODE_OWNER + self.__pg_file_create_mode = self.__PG_FILE_MODE_OWNER + self.__pg_mode_mask = self.__PG_MODE_MASK_OWNER + + def __set_group_permissions(self) -> None: + """Make directories/files accessible by the owner and readable by group.""" + self.__pg_dir_create_mode = self.__PG_DIR_MODE_GROUP + self.__pg_file_create_mode = self.__PG_FILE_MODE_GROUP + self.__pg_mode_mask = self.__PG_MODE_MASK_GROUP + + def set_permissions_from_data_directory(self, data_dir: str) -> None: + """Set new permissions based on provided *data_dir*. + + :param data_dir: reference to PGDATA to calculate permissions from. + """ + try: + st = os.stat(data_dir) + if (st.st_mode & self.__PG_DIR_MODE_GROUP) == self.__PG_DIR_MODE_GROUP: + self.__set_group_permissions() + else: + self.__set_owner_permissions() + except Exception as e: + logger.error('Can not check permissions on %s: %r', data_dir, e) + else: + self.__set_umask() + + @property + def dir_create_mode(self) -> int: + """Directory permissions.""" + return self.__pg_dir_create_mode + + @property + def file_create_mode(self) -> int: + """File permissions.""" + return self.__pg_file_create_mode + + +pg_perm = __FilePermissions() diff --git a/patroni/postgresql/config.py b/patroni/postgresql/config.py index 3a61a31d..6d1fe384 100644 --- a/patroni/postgresql/config.py +++ b/patroni/postgresql/config.py @@ -6,14 +6,16 @@ import socket import stat import time +from contextlib import contextmanager from urllib.parse import urlparse, parse_qsl, unquote from types import TracebackType -from typing import Any, Collection, Dict, List, Optional, Union, Tuple, Type, TYPE_CHECKING +from typing import Any, Collection, Dict, Iterator, List, Optional, Union, Tuple, Type, TYPE_CHECKING from .validator import recovery_parameters, transform_postgresql_parameter_value, transform_recovery_parameter_value from ..collections import CaseInsensitiveDict, CaseInsensitiveSet from ..dcs import Leader, Member, RemoteMember, slot_name_from_member_name from ..exceptions import PatroniFatalException +from ..file_perm import pg_perm from ..utils import compare_values, parse_bool, parse_int, split_host_port, uri, validate_directory, is_subpath from ..validator import IntValidator, EnumValidator @@ -367,6 +369,30 @@ class ConfigHandler(object): configuration.append('pg_ident.conf') return configuration + def set_file_permissions(self, filename: str) -> None: + """Set permissions of file *filename* according to the expected permissions if it resides under PGDATA. + + .. note:: + Do nothing if the file is not under PGDATA. + + :param filename: path to a file which permissions might need to be adjusted. + """ + if is_subpath(self._postgresql.data_dir, filename): + pg_perm.set_permissions_from_data_directory(self._postgresql.data_dir) + os.chmod(filename, pg_perm.file_create_mode) + + @contextmanager + def config_writer(self, filename: str) -> Iterator[ConfigWriter]: + """Create :class:`ConfigWriter` object and set permissions on a *filename*. + + :param filename: path to a config file. + + :yields: :class:`ConfigWriter` object. + """ + with ConfigWriter(filename) as writer: + yield writer + self.set_file_permissions(filename) + def save_configuration_files(self, check_custom_bootstrap: bool = False) -> bool: """ copy postgresql.conf to postgresql.conf.backup to be able to retrieve configuration files @@ -380,6 +406,7 @@ class ConfigHandler(object): backup_file = os.path.join(self._postgresql.data_dir, f + '.backup') if os.path.isfile(config_file): shutil.copy(config_file, backup_file) + self.set_file_permissions(backup_file) except IOError: logger.exception('unable to create backup copies of configuration files') return True @@ -393,9 +420,11 @@ class ConfigHandler(object): if not os.path.isfile(config_file): if os.path.isfile(backup_file): shutil.copy(backup_file, config_file) + self.set_file_permissions(config_file) # Previously we didn't backup pg_ident.conf, if file is missing just create empty elif f == 'pg_ident.conf': open(config_file, 'w').close() + self.set_file_permissions(config_file) except IOError: logger.exception('unable to restore configuration files from backup') @@ -409,7 +438,7 @@ class ConfigHandler(object): if self._postgresql.enforce_hot_standby_feedback: configuration['hot_standby_feedback'] = 'on' - with ConfigWriter(self._postgresql_conf) as f: + with self.config_writer(self._postgresql_conf) as f: include = self._config.get('custom_conf') or self._postgresql_base_conf_name f.writeline("include '{0}'\n".format(ConfigWriter.escape(include))) for name, value in sorted((configuration).items()): @@ -439,6 +468,7 @@ class ConfigHandler(object): if not self.hba_file and not self._config.get('pg_hba'): with open(self._pg_hba_conf, 'a') as f: f.write('\n{}\n'.format('\n'.join(config))) + self.set_file_permissions(self._pg_hba_conf) return True def replace_pg_hba(self) -> Optional[bool]: @@ -458,14 +488,14 @@ class ConfigHandler(object): self.local_replication_address['host'], self.local_replication_address['port'], 0, socket.SOCK_STREAM, socket.IPPROTO_TCP)}) - with ConfigWriter(self._pg_hba_conf) as f: + with self.config_writer(self._pg_hba_conf) as f: for address, t in addresses.items(): f.writeline(( '{0}\treplication\t{1}\t{3}\ttrust\n' '{0}\tall\t{2}\t{3}\ttrust' ).format(t, self.replication['username'], self._superuser.get('username') or 'all', address)) elif not self.hba_file and self._config.get('pg_hba'): - with ConfigWriter(self._pg_hba_conf) as f: + with self.config_writer(self._pg_hba_conf) as f: f.writelines(self._config['pg_hba']) return True @@ -478,7 +508,7 @@ class ConfigHandler(object): """ if not self.ident_file and self._config.get('pg_ident'): - with ConfigWriter(self._pg_ident_conf) as f: + with self.config_writer(self._pg_ident_conf) as f: f.writelines(self._config['pg_ident']) return True @@ -800,9 +830,11 @@ class ConfigHandler(object): if self._postgresql.major_version >= 120000: if parse_bool(recovery_params.pop('standby_mode', None)): open(self._standby_signal, 'w').close() + self.set_file_permissions(self._standby_signal) else: self._remove_file_if_exists(self._standby_signal) open(self._recovery_signal, 'w').close() + self.set_file_permissions(self._recovery_signal) def restart_required(name: str) -> bool: if self._postgresql.major_version >= 140000: @@ -813,8 +845,7 @@ class ConfigHandler(object): self._current_recovery_params = CaseInsensitiveDict({n: [v, restart_required(n), self._postgresql_conf] for n, v in recovery_params.items()}) else: - with ConfigWriter(self._recovery_conf) as f: - os.chmod(self._recovery_conf, stat.S_IWRITE | stat.S_IREAD) + with self.config_writer(self._recovery_conf) as f: self._write_recovery_params(f, recovery_params) def remove_recovery_conf(self) -> None: @@ -843,6 +874,7 @@ class ConfigHandler(object): if overwrite: try: with open(self._auto_conf, 'w') as f: + self.set_file_permissions(self._auto_conf) for raw_line in lines: f.write(raw_line) except Exception: diff --git a/patroni/postgresql/slots.py b/patroni/postgresql/slots.py index 0d2b1ed8..82b83002 100644 --- a/patroni/postgresql/slots.py +++ b/patroni/postgresql/slots.py @@ -10,6 +10,7 @@ from typing import Any, Dict, Iterator, List, Optional, Union, Tuple, TYPE_CHECK from .connection import get_connection_cursor from .misc import format_lsn, fsync_dir from ..dcs import Cluster, Leader +from ..file_perm import pg_perm from ..psycopg import OperationalError if TYPE_CHECKING: # pragma: no cover @@ -471,23 +472,28 @@ class SlotsHandler(object): logger.error("Failed to copy logical slots from the %s via postgresql connection: %r", leader.name, e) if copy_slots and self._postgresql.stop(): + pg_perm.set_permissions_from_data_directory(self._postgresql.data_dir) for name, value in copy_slots.items(): - slot_dir = os.path.join(self._postgresql.slots_handler.pg_replslot_dir, name) + slot_dir = os.path.join(self.pg_replslot_dir, name) slot_tmp_dir = slot_dir + '.tmp' if os.path.exists(slot_tmp_dir): shutil.rmtree(slot_tmp_dir) os.makedirs(slot_tmp_dir) + os.chmod(slot_tmp_dir, pg_perm.dir_create_mode) fsync_dir(slot_tmp_dir) - with open(os.path.join(slot_tmp_dir, 'state'), 'wb') as f: + slot_filename = os.path.join(slot_tmp_dir, 'state') + with open(slot_filename, 'wb') as f: + os.chmod(slot_filename, pg_perm.file_create_mode) f.write(value['data']) f.flush() os.fsync(f.fileno()) if os.path.exists(slot_dir): shutil.rmtree(slot_dir) os.rename(slot_tmp_dir, slot_dir) + os.chmod(slot_dir, pg_perm.dir_create_mode) fsync_dir(slot_dir) self._logical_slots_processing_queue[name] = None - fsync_dir(self._postgresql.slots_handler.pg_replslot_dir) + fsync_dir(self.pg_replslot_dir) self._postgresql.start() def schedule(self, value: Optional[bool] = None) -> None: diff --git a/tests/test_config.py b/tests/test_config.py index 3f7e4049..cf798d00 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -85,6 +85,7 @@ class TestConfig(unittest.TestCase): @patch('os.path.exists', Mock(return_value=True)) @patch('os.remove', Mock(side_effect=IOError)) @patch('os.close', Mock(side_effect=IOError)) + @patch('os.chmod', Mock()) @patch('shutil.move', Mock(return_value=None)) @patch('json.dump', Mock()) def test_save_cache(self): diff --git a/tests/test_file_perm.py b/tests/test_file_perm.py new file mode 100644 index 00000000..e85b3c65 --- /dev/null +++ b/tests/test_file_perm.py @@ -0,0 +1,33 @@ +import unittest +import stat + +from mock import Mock, patch + +from patroni.file_perm import pg_perm + + +class TestFilePermissions(unittest.TestCase): + + @patch('os.stat') + @patch('os.umask') + @patch('patroni.file_perm.logger.error') + def test_set_umask(self, mock_logger, mock_umask, mock_stat): + mock_umask.side_effect = Exception + mock_stat.return_value.st_mode = stat.S_IRWXU | stat.S_IRGRP | stat.S_IXGRP + pg_perm.set_permissions_from_data_directory('test') + + # umask is called with PG_MODE_MASK_GROUP + self.assertEqual(mock_umask.call_args[0][0], stat.S_IWGRP | stat.S_IRWXO) + self.assertEqual(mock_logger.call_args[0][0], 'Can not set umask to %03o: %r') + + mock_umask.reset_mock() + mock_stat.return_value.st_mode = stat.S_IRWXU + pg_perm.set_permissions_from_data_directory('test') + # umask is called with PG_MODE_MASK_OWNER (permissions changed from group to owner) + self.assertEqual(mock_umask.call_args[0][0], stat.S_IRWXG | stat.S_IRWXO) + + @patch('os.stat', Mock(side_effect=FileNotFoundError)) + @patch('patroni.file_perm.logger.error') + def test_set_permissions_from_data_directory(self, mock_logger): + pg_perm.set_permissions_from_data_directory('test') + self.assertEqual(mock_logger.call_args[0][0], 'Can not check permissions on %s: %r') diff --git a/tests/test_ha.py b/tests/test_ha.py index ae6be3e2..d3a75ed3 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -1421,6 +1421,7 @@ class TestHa(PostgresInit): @patch('os.open', Mock()) @patch('os.fsync', Mock()) @patch('os.close', Mock()) + @patch('os.chmod', Mock()) @patch('os.rename', Mock()) @patch('patroni.postgresql.Postgresql.is_starting', Mock(return_value=False)) @patch('builtins.open', mock_open()) diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 036e225b..b584f2b2 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -523,8 +523,9 @@ class TestPostgresql(BaseTestPostgresql): def test_save_configuration_files(self): self.p.config.save_configuration_files() - @patch('os.path.isfile', Mock(side_effect=[False, True])) - @patch('shutil.copy', Mock(side_effect=IOError)) + @patch('os.path.isfile', Mock(side_effect=[False, True, False, True])) + @patch('shutil.copy', Mock(side_effect=[None, IOError])) + @patch('os.chmod', Mock()) def test_restore_configuration_files(self): self.p.config.restore_configuration_files()