From 06db2966122cc3bf0291169b3cd2a2e93a522445 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Tue, 25 Jul 2023 08:48:18 +0200 Subject: [PATCH] Fixes in `patroni.request` (#2768) 1. Take client certificates only from the `ctl` section. Motivation: sometimes there are server-only certificates that can't be used as client certificates. As a result neither Patroni not patronictl work correctly even if `--insecure` option is used. 2. Document that if `restapi.verify_client` is set to `required` then client certificates **must** be provided in the `ctl` section. 3. Add support for `ctl.authentication` and prefer to use it over `restapi.authentication`. 4. Silence annoying InsecureRequestWarning when `patronictl -k` is used, so that behavior becomes is similar to `curl -k`. --- docs/ENVIRONMENT.rst | 19 ++++++-- docs/yaml_configuration.rst | 16 +++++-- features/environment.py | 4 +- patroni/config.py | 14 +++--- patroni/ctl.py | 3 +- patroni/request.py | 76 +++++++++++++++++++----------- tests/test_ctl.py | 4 +- typings/urllib3/__init__.pyi | 3 +- typings/urllib3/connectionpool.pyi | 2 + 9 files changed, 94 insertions(+), 47 deletions(-) create mode 100644 typings/urllib3/connectionpool.pyi diff --git a/docs/ENVIRONMENT.rst b/docs/ENVIRONMENT.rst index 91816184..6fbfe4e0 100644 --- a/docs/ENVIRONMENT.rst +++ b/docs/ENVIRONMENT.rst @@ -196,10 +196,19 @@ REST API - **PATRONI\_RESTAPI\_HTTPS\_EXTRA\_HEADERS**: (optional) HTTPS headers let the REST API server pass additional information with an HTTP response when TLS is enabled. This will also pass additional information set in ``http_extra_headers``. - **PATRONI\_RESTAPI\_REQUEST\_QUEUE\_SIZE**: (optional): Sets request queue size for TCP socket used by Patroni REST API. Once the queue is full, further requests get a "Connection denied" error. The default value is 5. +.. warning:: + + - The ``PATRONI_RESTAPI_CONNECT_ADDRESS`` must be accessible from all nodes of a given Patroni cluster. Internally Patroni is using it during the leader race to find nodes with minimal replication lag. + - If you enabled client certificates validation (``PATRONI_RESTAPI_VERIFY_CLIENT`` is set to ``required``), you also **must** provide **valid client certificates** in the ``PATRONI_CTL_CERTFILE``, ``PATRONI_CTL_KEYFILE``, ``PATRONI_CTL_KEYFILE_PASSWORD``. If not provided, Patroni will not work correctly. + + CTL --- -- **PATRONICTL\_CONFIG\_FILE**: location of the configuration file. -- **PATRONI\_CTL\_INSECURE**: Allow connections to REST API without verifying SSL certs. -- **PATRONI\_CTL\_CACERT**: Specifies the file with the CA_BUNDLE file or directory with certificates of trusted CAs to use while verifying REST API SSL certs. If not provided patronictl will use the value provided for REST API "cafile" parameter. -- **PATRONI\_CTL\_CERTFILE**: Specifies the file with the client certificate in the PEM format. If not provided patronictl will use the value provided for REST API "certfile" parameter. -- **PATRONI\_CTL\_KEYFILE**: Specifies the file with the client secret key in the PEM format. If not provided patronictl will use the value provided for REST API "keyfile" parameter. +- **PATRONICTL\_CONFIG\_FILE**: (optional) location of the configuration file. +- **PATRONI\_CTL\_USERNAME**: (optional) Basic-auth username for accessing protected REST API endpoints. If not provided patronictl will use the value provided for REST API "username" parameter. +- **PATRONI\_CTL\_PASSWORD**: (optional) Basic-auth password for accessing protected REST API endpoints. If not provided patronictl will use the value provided for REST API "password" parameter. +- **PATRONI\_CTL\_INSECURE**: (optional) Allow connections to REST API without verifying SSL certs. +- **PATRONI\_CTL\_CACERT**: (optional) Specifies the file with the CA_BUNDLE file or directory with certificates of trusted CAs to use while verifying REST API SSL certs. If not provided patronictl will use the value provided for REST API "cafile" parameter. +- **PATRONI\_CTL\_CERTFILE**: (optional) Specifies the file with the client certificate in the PEM format. +- **PATRONI\_CTL\_KEYFILE**: (optional) Specifies the file with the client secret key in the PEM format. +- **PATRONI\_CTL\_KEYFILE\_PASSWORD**: (optional) Specifies a password for decrypting the client keyfile. diff --git a/docs/yaml_configuration.rst b/docs/yaml_configuration.rst index f58c9b0a..fbb2de68 100644 --- a/docs/yaml_configuration.rst +++ b/docs/yaml_configuration.rst @@ -335,17 +335,27 @@ Here is an example of both **http_extra_headers** and **https_extra_headers**: https_extra_headers: 'Strict-Transport-Security': 'max-age=31536000; includeSubDomains' +.. warning:: + + - The ``restapi.connect_address`` must be accessible from all nodes of a given Patroni cluster. Internally Patroni is using it during the leader race to find nodes with minimal replication lag. + - If you enabled client certificates validation (``restapi.verify_client`` is set to ``required``), you also **must** provide **valid client certificates** in the ``ctl.certfile``, ``ctl.keyfile``, ``ctl.keyfile_password``. If not provided, Patroni will not work correctly. + + .. _patronictl_settings: CTL --- - **ctl**: (optional) + - **authentication**: + + - **username**: Basic-auth username for accessing protected REST API endpoints. If not provided patronictl will use the value provided for REST API "username" parameter. + - **password**: Basic-auth password for accessing protected REST API endpoints. If not provided patronictl will use the value provided for REST API "password" parameter. - **insecure**: Allow connections to REST API without verifying SSL certs. - **cacert**: Specifies the file with the CA_BUNDLE file or directory with certificates of trusted CAs to use while verifying REST API SSL certs. If not provided patronictl will use the value provided for REST API "cafile" parameter. - - **certfile**: Specifies the file with the client certificate in the PEM format. If not provided patronictl will use the value provided for REST API "certfile" parameter. - - **keyfile**: Specifies the file with the client secret key in the PEM format. If not provided patronictl will use the value provided for REST API "keyfile" parameter. - - **keyfile\_password**: Specifies a password for decrypting the keyfile. If not provided patronictl will use the value provided for REST API "keyfile\_password" parameter. + - **certfile**: Specifies the file with the client certificate in the PEM format. + - **keyfile**: Specifies the file with the client secret key in the PEM format. + - **keyfile\_password**: Specifies a password for decrypting the client keyfile. Watchdog -------- diff --git a/features/environment.py b/features/environment.py index 5906becd..ec503bb5 100644 --- a/features/environment.py +++ b/features/environment.py @@ -1082,7 +1082,9 @@ def before_all(context): 'PATRONI_RESTAPI_CERTFILE': context.certfile, 'PATRONI_RESTAPI_KEYFILE': context.keyfile, 'PATRONI_RESTAPI_VERIFY_CLIENT': 'required', - 'PATRONI_CTL_INSECURE': 'on'}) + 'PATRONI_CTL_INSECURE': 'on', + 'PATRONI_CTL_CERTFILE': context.certfile, + 'PATRONI_CTL_KEYFILE': context.keyfile}) ctl.update({'cacert': context.certfile, 'certfile': context.certfile, 'keyfile': context.keyfile}) context.request_executor = PatroniRequest({'ctl': ctl}, True) context.dcs_ctl = context.pctl.known_dcs[context.pctl.dcs](context) diff --git a/patroni/config.py b/patroni/config.py index aa71d821..365abc24 100644 --- a/patroni/config.py +++ b/patroni/config.py @@ -452,9 +452,10 @@ class Config(object): ret[param] = value return ret - restapi_auth = _get_auth('restapi') - if restapi_auth: - ret['restapi']['authentication'] = restapi_auth + for section in ('ctl', 'restapi'): + auth = _get_auth(section) + if auth: + ret[section]['authentication'] = auth authentication = {} for user_type in ('replication', 'superuser', 'rewind'): @@ -531,9 +532,10 @@ class Config(object): elif name not in config or name in ['watchdog']: config[name] = deepcopy(value) if value else {} - # restapi server expects to get restapi.auth = 'username:password' - if 'restapi' in config and 'authentication' in config['restapi']: - config['restapi']['auth'] = '{username}:{password}'.format(**config['restapi']['authentication']) + # restapi server expects to get restapi.auth = 'username:password' and similarly for `ctl` + for section in ('ctl', 'restapi'): + if section in config and 'authentication' in config[section]: + config[section]['auth'] = '{username}:{password}'.format(**config[section]['authentication']) # special treatment for old config diff --git a/patroni/ctl.py b/patroni/ctl.py index 55a054c1..bb6b4b1c 100644 --- a/patroni/ctl.py +++ b/patroni/ctl.py @@ -254,7 +254,6 @@ arg_cluster_name = click.argument('cluster_name', required=False, option_default_citus_group = click.option('--group', required=False, type=int, help='Citus group', default=lambda: click.get_current_context().obj.get('citus', {}).get('group')) option_citus_group = click.option('--group', required=False, type=int, help='Citus group') -option_insecure = click.option('-k', '--insecure', is_flag=True, help='Allow connections to SSL sites without certs') role_choice = click.Choice(['leader', 'primary', 'standby-leader', 'replica', 'standby', 'any', 'master']) @@ -262,7 +261,7 @@ role_choice = click.Choice(['leader', 'primary', 'standby-leader', 'replica', 's @click.option('--config-file', '-c', help='Configuration file', envvar='PATRONICTL_CONFIG_FILE', default=CONFIG_FILE_PATH) @click.option('--dcs-url', '--dcs', '-d', 'dcs_url', help='The DCS connect url', envvar='DCS_URL') -@option_insecure +@click.option('-k', '--insecure', is_flag=True, help='Allow connections to SSL sites without certs') @click.pass_context def ctl(ctx: click.Context, config_file: str, dcs_url: Optional[str], insecure: bool) -> None: """Entry point of ``patronictl`` utility. diff --git a/patroni/request.py b/patroni/request.py index d063310d..0421bf86 100644 --- a/patroni/request.py +++ b/patroni/request.py @@ -11,6 +11,19 @@ from .dcs import Member from .utils import USER_AGENT +class HTTPSConnectionPool(urllib3.HTTPSConnectionPool): + + def _validate_conn(self, *args: Any, **kwargs: Any) -> None: + """Override parent method to silence warnings about requests without certificate verification enabled.""" + + +class PatroniPoolManager(urllib3.PoolManager): + + def __init__(self, *args: Any, **kwargs: Any) -> None: + super(PatroniPoolManager, self).__init__(*args, **kwargs) + self.pool_classes_by_scheme = {'http': urllib3.HTTPConnectionPool, 'https': HTTPSConnectionPool} + + class PatroniRequest(object): """Wrapper for performing requests to Patroni's REST API. @@ -28,22 +41,30 @@ class PatroniRequest(object): * If none of the above applies, then it falls back to ``False``. """ self._insecure = insecure - self._pool = urllib3.PoolManager(num_pools=10, maxsize=10) + self._pool = PatroniPoolManager(num_pools=10, maxsize=10) self.reload_config(config) @staticmethod - def _get_cfg_value(config: Union[Config, Dict[str, Any]], name: str) -> Union[Any, None]: - """Get value of *name* setting in *config*. - - .. note:: - *name* key will be searched only under ``ctl`` and ``restapi`` sections, in that order. + def _get_ctl_value(config: Union[Config, Dict[str, Any]], name: str, default: Any = None) -> Optional[Any]: + """Get value of *name* setting from the ``ctl`` section of the *config*. :param config: Patroni YAML configuration. :param name: name of the setting value to be retrieved. - :returns: value of ``ctl -> *name*`` or ``restapi -> *name*``, if either is present, ``None`` otherwise. + :returns: value of ``ctl -> *name*`` if present, ``None`` otherwise. """ - return config.get('ctl', {}).get(name) or config.get('restapi', {}).get(name) + return config.get('ctl', {}).get(name, default) + + @staticmethod + def _get_restapi_value(config: Union[Config, Dict[str, Any]], name: str) -> Optional[Any]: + """Get value of *name* setting from the ``restapi`` section of the *config*. + + :param config: Patroni YAML configuration. + :param name: name of the setting value to be retrieved. + + :returns: value of ``restapi -> *name*`` if present, ``None`` otherwise. + """ + return config.get('restapi', {}).get(name) def _apply_pool_param(self, param: str, value: Any) -> None: """Configure *param* as *value* in the request manager. @@ -65,12 +86,11 @@ class PatroniRequest(object): * ``cert``: gets translated to ``certfile`` * ``key``: gets translated to ``keyfile`` - Will attempt to fetch the requested key first from ``ctl`` section, and fall back to ``restapi`` section - if the former is missing. + Will attempt to fetch the requested key first from ``ctl`` section. - :returns: value of ``ctl -> *name*file`` or ``restapi -> *name*file`` if either is present, ``None`` otherwise. + :returns: value of ``ctl -> *name*file`` if present, ``None`` otherwise. """ - value = self._get_cfg_value(config, name + 'file') + value = self._get_ctl_value(config, name + 'file') self._apply_pool_param(name + '_file', value) return value @@ -79,37 +99,39 @@ class PatroniRequest(object): Configure these HTTP headers for requests: - * ``authorization``: based on Patroni' REST API authentication config; + * ``authorization``: based on Patroni' CTL or REST API authentication config; * ``user-agent``: based on `patroni.utils.USER_AGENT`. Also configure SSL related settings for requests: * ``ca_certs`` is configured if ``ctl -> cacert`` or ``restapi -> cafile`` is available; - * ``cert``, ``key`` and ``key_password`` are configured if ``ctl -> certile`` or ``restapi -> certfile`` is - available. + * ``cert``, ``key`` and ``key_password`` are configured if ``ctl -> certfile`` is available. :param config: Patroni YAML configuration. """ - # ``restapi -> auth`` is equivalent to ``restapi -> authentication -> username`` + ``:`` + - # ``restapi -> authentication -> password`` - self._pool.headers = urllib3.make_headers(basic_auth=self._get_cfg_value(config, 'auth'), user_agent=USER_AGENT) + # ``ctl -> auth`` is equivalent to ``ctl -> authentication -> username`` + ``:`` + + # ``ctl -> authentication -> password``. And the same for ``restapi -> auth`` + basic_auth = self._get_ctl_value(config, 'auth') or self._get_restapi_value(config, 'auth') + self._pool.headers = urllib3.make_headers(basic_auth=basic_auth, user_agent=USER_AGENT) + self._pool.connection_pool_kw['cert_reqs'] = 'CERT_REQUIRED' + + insecure = self._insecure if isinstance(self._insecure, bool)\ + else self._get_ctl_value(config, 'insecure', False) - insecure = self._insecure if isinstance(self._insecure, bool) else config.get('ctl', {}).get('insecure', False) if self._apply_ssl_file_param(config, 'cert'): - # With client certificate the cert_reqs must be set to CERT_REQUIRED even if insecure option is used - self._pool.connection_pool_kw['cert_reqs'] = 'CERT_REQUIRED' - # The assert_hostname = False helps to silence warnings - self._pool.connection_pool_kw['assert_hostname'] = False if insecure else None + if insecure: # The assert_hostname = False helps to silence warnings + self._pool.connection_pool_kw['assert_hostname'] = False self._apply_ssl_file_param(config, 'key') - - password = self._get_cfg_value(config, 'keyfile_password') + password = self._get_ctl_value(config, 'keyfile_password') self._apply_pool_param('key_password', password) else: - self._pool.connection_pool_kw['cert_reqs'] = 'CERT_NONE' if insecure else 'CERT_REQUIRED' + if insecure: # Disable server certificate validation if requested + self._pool.connection_pool_kw['cert_reqs'] = 'CERT_NONE' + self._pool.connection_pool_kw.pop('assert_hostname', None) self._pool.connection_pool_kw.pop('key_file', None) - cacert = config.get('ctl', {}).get('cacert') or config.get('restapi', {}).get('cafile') + cacert = self._get_ctl_value(config, 'cacert') or self._get_restapi_value(config, 'cafile') self._apply_pool_param('ca_certs', cacert) def request(self, method: str, url: str, body: Optional[Any] = None, diff --git a/tests/test_ctl.py b/tests/test_ctl.py index f7d3eec5..b30cecc1 100644 --- a/tests/test_ctl.py +++ b/tests/test_ctl.py @@ -22,7 +22,7 @@ from .test_ha import get_cluster_initialized_without_leader, get_cluster_initial @patch('patroni.ctl.load_config', Mock(return_value={ - 'scope': 'alpha', 'restapi': {'listen': '::', 'certfile': 'a'}, + 'scope': 'alpha', 'restapi': {'listen': '::', 'certfile': 'a'}, 'ctl': {'certfile': 'a'}, 'etcd': {'host': 'localhost:2379'}, 'citus': {'database': 'citus', 'group': 0}, 'postgresql': {'data_dir': '.', 'pgpass': './pgpass', 'parameters': {}, 'retry_timeout': 5}})) class TestCtl(unittest.TestCase): @@ -451,7 +451,7 @@ class TestCtl(unittest.TestCase): mock_get_dcs.return_value.get_cluster = get_cluster_initialized_with_leader for role in self.TEST_ROLES: - result = self.runner.invoke(ctl, ['flush', 'dummy', 'restart', '-r', role], input='y') + result = self.runner.invoke(ctl, ['-k', 'flush', 'dummy', 'restart', '-r', role], input='y') assert 'No scheduled restart' in result.output result = self.runner.invoke(ctl, ['flush', 'dummy', 'restart', '--force']) diff --git a/typings/urllib3/__init__.pyi b/typings/urllib3/__init__.pyi index ff573518..61b5b66c 100644 --- a/typings/urllib3/__init__.pyi +++ b/typings/urllib3/__init__.pyi @@ -1,6 +1,7 @@ +from .connectionpool import HTTPConnectionPool, HTTPSConnectionPool from .poolmanager import PoolManager from .response import HTTPResponse from .util.request import make_headers from .util.timeout import Timeout -__all__ = ['HTTPResponse', 'PoolManager', 'Timeout', 'make_headers'] +__all__ = ['HTTPResponse', 'HTTPConnectionPool', 'HTTPSConnectionPool', 'PoolManager', 'Timeout', 'make_headers'] diff --git a/typings/urllib3/connectionpool.pyi b/typings/urllib3/connectionpool.pyi new file mode 100644 index 00000000..b2e8d3db --- /dev/null +++ b/typings/urllib3/connectionpool.pyi @@ -0,0 +1,2 @@ +class HTTPConnectionPool: ... +class HTTPSConnectionPool(HTTPConnectionPool): ...