Repository: zalando/patroni
Pull request #2722 (2023-06-23 15:45:21)

REST API would not reload SSL certificate upon receiving an SIGHUP
Patroni used to have basically 3 ways of loading a new certificate file in the REST API server:

* Restarting the Patroni agent
* Changing the value of `restapi.certfile` and issuing an SIGHUP
* Changing the actual certificate pointed by `restapi.certfile` and issuing an SIGHUP

The 3rd method relies on checking differences between serial numbers of the configured certificate before and after an SIGHUP.

Through [#2648](https://github.com/zalando/patroni/pull/2648) we applied a change to method `get_certificate_serial_number` that introduced a bug: REST API's certificate will not be reloaded upon an SIGHUP if the value of `restapi.certfile` remains unchanged, but with a different content in the actual certificate file.

With the code of 3.0.3 we are attempting to use the output of `load_verify_locations` to get the serial number of the certificate. However, that method only loads the information, and we would need to fetch the serial number from entries returned by `get_ca_certs` instead.

However, even with `load_verify_locations` + `get_ca_certs`, there would still be cases where the serial number would not be returned as `get_ca_certs` would return an empty list: certificates created with `CA:FALSE` -- refer to [CPython _ssl](https://github.com/python/cpython/blob/c283a0cff5603540f06d9017e484b3602cc62e7c/Modules/_ssl.c#L4618C14-L4619).

With that in mind, through this PR we are reverting to using `ssl._ssl._test_decode_cert` to get serial number from certificates. In the future we might come up with a more elegant solution, if any, but for now we will stick with the working approach that uses private functions.

References: PAT-130.

coveralls: ## Pull Request Test Coverage Report for [Build 5358135122](https://coveralls.io/builds/60926177)

* **4** of **4**   **(100.0%)**  changed or added relevant lines in **1** file are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage remained the same at **99.858%**

---



|  Totals | [![Coverage Status](https://coveralls.io/builds/60926177/badge)](https://coveralls.io/builds/60926177) |
| :-- | --: |
| Change from base [Build 5343483570](https://coveralls.io/builds/60891915): |  0.0% |
| Covered Lines: | 12642 |
| Relevant Lines: | 12660 |

---
##### 💛  - [Coveralls](https://coveralls.io)


Patch:
diff --git a/docs/releases.rst b/docs/releases.rst
index b9b0dfa6..6adda2e1 100644
--- a/docs/releases.rst
+++ b/docs/releases.rst
@@ -16,7 +16,7 @@ Version 3.0.3
 
   Validator rules are loaded from YAML files located in ``patroni/postgresql/available_parameters/`` directory. Files are ordered in alphabetical order and applied one after another. It makes possible to have custom validators for non-standard Postgres distributions.
 
-- Added ``restapi.request_queue_size`` option (Andrey Zhidenkov)
+- Added ``restapi.request_queue_size`` option (Andrey Zhidenkov, Aleksei Sukhov)
 
   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.
 
diff --git a/patroni/api.py b/patroni/api.py
index f461811b..7be2a049 100644
--- a/patroni/api.py
+++ b/patroni/api.py
@@ -1541,11 +1541,11 @@ class RestApiServer(ThreadingMixIn, HTTPServer, Thread):
         if self.__ssl_options.get('certfile'):
             import ssl
             try:
-                ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
-                crts = ctx.load_verify_locations(self.__ssl_options['certfile'])
-                if crts:
-                    return crts[0].get('serialNumber')
-            except Exception as e:
+                crt: Dict[str, Any] = ssl._ssl._test_decode_cert(self.__ssl_options['certfile'])  # pyright: ignore
+                if TYPE_CHECKING:  # pragma: no cover
+                    assert isinstance(crt, dict)
+                return crt.get('serialNumber')
+            except ssl.SSLError as e:
                 logger.error('Failed to get serial number from certificate %s: %r', self.__ssl_options['certfile'], e)
 
     def reload_local_certificate(self) -> Optional[bool]:


Test patch:
diff --git a/tests/test_api.py b/tests/test_api.py
index 166d3eb1..25342dca 100644
--- a/tests/test_api.py
+++ b/tests/test_api.py
@@ -180,7 +180,6 @@ class MockRestApiServer(RestApiServer):
 
 @patch('ssl.SSLContext.load_cert_chain', Mock())
 @patch('ssl.SSLContext.wrap_socket', Mock(return_value=0))
-@patch('ssl.SSLContext.load_verify_locations', Mock(return_value=[Mock()]))
 @patch.object(HTTPServer, '__init__', Mock())
 class TestRestApiHandler(unittest.TestCase):
 
@@ -589,7 +588,6 @@ class TestRestApiServer(unittest.TestCase):
     @patch('ssl.SSLContext.load_cert_chain', Mock())
     @patch('ssl.SSLContext.set_ciphers', Mock())
     @patch('ssl.SSLContext.wrap_socket', Mock(return_value=0))
-    @patch('ssl.SSLContext.load_verify_locations', Mock(return_value=[Mock()]))
     @patch.object(HTTPServer, '__init__', Mock())
     def setUp(self):
         self.srv = MockRestApiServer(Mock(), '', {'listen': '*:8008', 'certfile': 'a', 'verify_client': 'required',
@@ -652,10 +650,9 @@ class TestRestApiServer(unittest.TestCase):
         mock_get_request.return_value = (self.__create_socket(), ('127.0.0.1', 55555))
         self.srv._handle_request_noblock()
 
-    @patch('ssl.SSLContext.load_verify_locations', Mock(return_value=[Mock()]))
+    @patch('ssl._ssl._test_decode_cert', Mock())
     def test_reload_local_certificate(self):
         self.assertTrue(self.srv.reload_local_certificate())
 
-    @patch('ssl.SSLContext.load_verify_locations', Mock(side_effect=Exception))
     def test_get_certificate_serial_number(self):
         self.assertIsNone(self.srv.get_certificate_serial_number())