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

@@ -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.
+-  **PATRONICTL\_CONFIG\_FILE**: (optional) location of the configuration file.
--  **PATRONI\_CTL\_INSECURE**: Allow connections to REST API without verifying SSL certs.
+-  **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\_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\_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\_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\_INSECURE**: (optional) Allow connections to REST API without verifying SSL certs.
--  **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.
+-  **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.

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.
+   -  **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. If not provided patronictl will use the value provided for REST API "keyfile" parameter.
+   -  **keyfile**: Specifies the file with the client secret key in the PEM format.
-   -  **keyfile\_password**: Specifies a password for decrypting the keyfile. If not provided patronictl will use the value provided for REST API "keyfile\_password" parameter.
+   -  **keyfile\_password**: Specifies a password for decrypting the client keyfile.
 
 Watchdog
 --------

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)

patroni/config.py

@@ -452,9 +452,10 @@ class Config(object):
                     ret[param] = value
             return ret
 
-        restapi_auth = _get_auth('restapi')
+        for section in ('ctl', 'restapi'):
-        if restapi_auth:
+            auth = _get_auth(section)
-            ret['restapi']['authentication'] = restapi_auth
+            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'
+        # restapi server expects to get restapi.auth = 'username:password' and similarly for `ctl`
-        if 'restapi' in config and 'authentication' in config['restapi']:
+        for section in ('ctl', 'restapi'):
-            config['restapi']['auth'] = '{username}:{password}'.format(**config['restapi']['authentication'])
+            if section in config and 'authentication' in config[section]:
+                config[section]['auth'] = '{username}:{password}'.format(**config[section]['authentication'])
 
         # special treatment for old config
 

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.

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]:
+    def _get_ctl_value(config: Union[Config, Dict[str, Any]], name: str, default: Any = None) -> Optional[Any]:
-        """Get value of *name* setting in *config*.
+        """Get value of *name* setting from the ``ctl`` section of the *config*.
-
-        .. note::
-            *name* key will be searched only under ``ctl`` and ``restapi`` sections, in that order.
 
         :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
+            Will attempt to fetch the requested key first from ``ctl`` section.
-            if the former is missing.
 
-        :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
+        * ``cert``, ``key`` and ``key_password`` are configured if ``ctl -> certfile`` is available.
-            available.
 
         :param config: Patroni YAML configuration.
         """
-        # ``restapi -> auth`` is equivalent to ``restapi -> authentication -> username`` + ``:`` +
+        # ``ctl -> auth`` is equivalent to ``ctl -> authentication -> username`` + ``:`` +
-        # ``restapi -> authentication -> password``
+        # ``ctl -> authentication -> password``. And the same for ``restapi -> auth``
-        self._pool.headers = urllib3.make_headers(basic_auth=self._get_cfg_value(config, 'auth'), user_agent=USER_AGENT)
+        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)
-        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
+        insecure = self._insecure if isinstance(self._insecure, bool)\
+            else self._get_ctl_value(config, 'insecure', False)
+
+        if self._apply_ssl_file_param(config, 'cert'):
+            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_ctl_value(config, 'keyfile_password')
-            password = self._get_cfg_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,

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'])

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']

typings/urllib3/connectionpool.pyi

@@ -0,0 +1,2 @@
+class HTTPConnectionPool: ...
+class HTTPSConnectionPool(HTTPConnectionPool): ...