From 9922248a896c9ac836055b7e4fe421f796f835b8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?C=C3=A9dric=20Jeanneret?= <cjeanner@redhat.com>
Date: Wed, 13 Apr 2022 13:52:53 +0200
Subject: [PATCH] Save the HAProxy state outside of its systemd unit

By default, SELinux prevents HAProxy context (haproxy_t) to execute
shell context (shell_exec_t) for security reasons.

This prevents HAProxy to actually reload properly, since SELinux will
deny its call to a shell to save its state to a file.

In order to avoid opening a potential security hole in the load-balancer
image, the best way is to generate the state file before the actual
reload.

There are more details about the SELinux denials in the associated Red
Hat Bugzilla.

 Conflicts:
	octavia/amphorae/backends/utils/haproxy_query.py

 Fixed in this backport:
   - A compatibility issue with Python 2.7
     (open() didn't have an encoding kwarg)
   - save_state() tried to use Exception().output, which in general
     causes an AttributeError

Resolves: rhbz#2073491
Change-Id: I6b9a5e1e3bafe77ad9f9506b8c0995d8c2a00081
(cherry picked from commit 21d74c373b92f8711ff91766c2e7c8bcb811bdda)
(cherry picked from commit 0a062cd66416c6020d9718d87988727bb8a78627)
(cherry picked from commit edcd6931fcc83dffc4d4a37c91b8db044ff3fdc4)
(cherry picked from commit 97ac37bc6e6572ce7b368c974b928ce5e35700d1)
(cherry picked from commit d4f2de8ba88a09a1139cf69be14acde06e765470)
(cherry picked from commit 86672909d435ddb1941e5b4fb0ef92d806c4802d)
---
 .../backends/agent/api_server/loadbalancer.py | 12 ++++++++
 .../api_server/templates/systemd.conf.j2      |  1 -
 .../amphorae/backends/utils/haproxy_query.py  | 27 +++++++++++++++--
 .../backend/agent/api_server/test_server.py   |  6 ++--
 .../agent/api_server/test_loadbalancer.py     |  5 ++--
 .../backends/utils/test_haproxy_query.py      | 29 +++++++++++++++++++
 ...-healthcheck-selinux-e3b7d360c8503527.yaml |  6 ++++
 7 files changed, 79 insertions(+), 7 deletions(-)
 create mode 100644 releasenotes/notes/ping-healthcheck-selinux-e3b7d360c8503527.yaml

diff --git a/octavia/amphorae/backends/agent/api_server/loadbalancer.py b/octavia/amphorae/backends/agent/api_server/loadbalancer.py
index b67248a258..d40e827fd7 100644
--- a/octavia/amphorae/backends/agent/api_server/loadbalancer.py
+++ b/octavia/amphorae/backends/agent/api_server/loadbalancer.py
@@ -31,6 +31,7 @@ from werkzeug import exceptions
 from octavia.amphorae.backends.agent.api_server import haproxy_compatibility
 from octavia.amphorae.backends.agent.api_server import osutils
 from octavia.amphorae.backends.agent.api_server import util
+from octavia.amphorae.backends.utils import haproxy_query
 from octavia.common import constants as consts
 from octavia.common import utils as octavia_utils
 
@@ -248,6 +249,17 @@ class Loadbalancer(object):
         if action == consts.AMP_ACTION_RELOAD:
             if consts.OFFLINE == self._check_haproxy_status(lb_id):
                 action = consts.AMP_ACTION_START
+            else:
+                # We first have to save the state when we reload
+                haproxy_state_file = util.state_file_path(lb_id)
+                stat_sock_file = util.haproxy_sock_path(lb_id)
+
+                lb_query = haproxy_query.HAProxyQuery(stat_sock_file)
+                if not lb_query.save_state(haproxy_state_file):
+                    # We accept to reload haproxy even if the state_file is
+                    # not generated, but we probably want to know about that
+                    # failure!
+                    LOG.warning('Failed to save haproxy-%s state!', lb_id)
 
         cmd = ("/usr/sbin/service haproxy-{lb_id} {action}".format(
             lb_id=lb_id, action=action))
diff --git a/octavia/amphorae/backends/agent/api_server/templates/systemd.conf.j2 b/octavia/amphorae/backends/agent/api_server/templates/systemd.conf.j2
index 3ccc69489f..545486e3a7 100644
--- a/octavia/amphorae/backends/agent/api_server/templates/systemd.conf.j2
+++ b/octavia/amphorae/backends/agent/api_server/templates/systemd.conf.j2
@@ -13,7 +13,6 @@ Environment="CONFIG={{ haproxy_cfg }}" "USERCONFIG={{ haproxy_user_group_cfg }}"
 
 ExecStartPre={{ haproxy_cmd }} -f $CONFIG -f $USERCONFIG -c -q -L {{ peer_name }}
 
-ExecReload=/bin/sh -c "echo 'show servers state' | socat stdio unix-connect:{{ haproxy_socket }} > {{ haproxy_state_file }}"
 ExecReload={{ haproxy_cmd }} -c -f $CONFIG -f $USERCONFIG -L {{ peer_name }}
 ExecReload=/bin/kill -USR2 $MAINPID
 
diff --git a/octavia/amphorae/backends/utils/haproxy_query.py b/octavia/amphorae/backends/utils/haproxy_query.py
index c9366cc903..1d62d6a8b3 100644
--- a/octavia/amphorae/backends/utils/haproxy_query.py
+++ b/octavia/amphorae/backends/utils/haproxy_query.py
@@ -15,11 +15,14 @@
 import csv
 import socket
 
+from oslo_log import log as logging
 import six
 
 from octavia.common import constants as consts
 from octavia.i18n import _
 
+LOG = logging.getLogger(__name__)
+
 
 class HAProxyQuery(object):
     """Class used for querying the HAProxy statistics socket.
@@ -30,9 +33,9 @@ class HAProxyQuery(object):
     """
 
     def __init__(self, stats_socket):
-        """stats_socket
+        """Initialize the class
 
-            Path to the HAProxy statistics socket file.
+        :param stats_socket: Path to the HAProxy statistics socket file.
         """
 
         self.socket = stats_socket
@@ -134,3 +137,23 @@ class HAProxyQuery(object):
                 final_results[line['pxname']]['members'][line['svname']] = (
                     line['status'])
         return final_results
+
+    def save_state(self, state_file_path):
+        """Save haproxy connection state to a file.
+
+        :param state_file_path: Absolute path to the state file
+
+        :returns: bool (True if success, False otherwise)
+        """
+
+        try:
+            result = self._query('show servers state')
+            # No need for binary mode, the _query converts bytes to ascii.
+            with open(state_file_path, 'w') as fh:
+                fh.write(result)
+            return True
+        except Exception as e:
+            # Catch any exception - may be socket issue, or write permission
+            # issue as well.
+            LOG.warning("Unable to save state: %r", e)
+            return False
diff --git a/octavia/tests/functional/amphorae/backend/agent/api_server/test_server.py b/octavia/tests/functional/amphorae/backend/agent/api_server/test_server.py
index 9e03612879..d58ecb87e3 100644
--- a/octavia/tests/functional/amphorae/backend/agent/api_server/test_server.py
+++ b/octavia/tests/functional/amphorae/backend/agent/api_server/test_server.py
@@ -352,8 +352,10 @@ class TestServerTestCase(base.TestCase):
     @mock.patch('octavia.amphorae.backends.agent.api_server.loadbalancer.'
                 'Loadbalancer._check_haproxy_status')
     @mock.patch('subprocess.check_output')
-    def _test_reload(self, distro, mock_subprocess, mock_haproxy_status,
-                     mock_vrrp, mock_exists, mock_listdir):
+    @mock.patch('octavia.amphorae.backends.utils.haproxy_query.HAProxyQuery')
+    def _test_reload(self, distro, mock_haproxy_query, mock_subprocess,
+                     mock_haproxy_status, mock_vrrp, mock_exists,
+                     mock_listdir):
 
         self.assertIn(distro, [consts.UBUNTU, consts.CENTOS])
 
diff --git a/octavia/tests/unit/amphorae/backends/agent/api_server/test_loadbalancer.py b/octavia/tests/unit/amphorae/backends/agent/api_server/test_loadbalancer.py
index 5de0f23516..663a59609b 100644
--- a/octavia/tests/unit/amphorae/backends/agent/api_server/test_loadbalancer.py
+++ b/octavia/tests/unit/amphorae/backends/agent/api_server/test_loadbalancer.py
@@ -65,8 +65,9 @@ class ListenerTestCase(base.TestCase):
     @mock.patch('octavia.amphorae.backends.agent.api_server.loadbalancer.'
                 'Loadbalancer._check_lb_exists')
     @mock.patch('subprocess.check_output')
-    def test_start_stop_lb(self, mock_check_output, mock_lb_exists,
-                           mock_path_exists, mock_vrrp_update,
+    @mock.patch('octavia.amphorae.backends.utils.haproxy_query.HAProxyQuery')
+    def test_start_stop_lb(self, mock_haproxy_query, mock_check_output,
+                           mock_lb_exists, mock_path_exists, mock_vrrp_update,
                            mock_check_status):
         listener_id = uuidutils.generate_uuid()
 
diff --git a/octavia/tests/unit/amphorae/backends/utils/test_haproxy_query.py b/octavia/tests/unit/amphorae/backends/utils/test_haproxy_query.py
index 1e58876147..ced4f3b100 100644
--- a/octavia/tests/unit/amphorae/backends/utils/test_haproxy_query.py
+++ b/octavia/tests/unit/amphorae/backends/utils/test_haproxy_query.py
@@ -145,3 +145,32 @@ class QueryTestCase(base.TestCase):
              'description': '', 'Release_date': '2014/07/25'},
             self.q.show_info()
         )
+
+    def test_save_state(self):
+        filename = 'state_file'
+
+        query_mock = mock.Mock()
+        query_mock.return_value = 'DATA'
+
+        self.q._query = query_mock
+        builtin_open = '__builtin__.open' if six.PY2 else 'builtins.open'
+
+        with mock.patch(builtin_open) as mock_open:
+            mock_fh = mock.MagicMock()
+            mock_open().__enter__.return_value = mock_fh
+
+            self.q.save_state(filename)
+
+            mock_fh.write.assert_called_once_with('DATA')
+
+    def test_save_state_error(self):
+        """save_state() should swallow exceptions"""
+        filename = 'state_file'
+
+        query_mock = mock.Mock(side_effect=OSError())
+        self.q._query = query_mock
+
+        try:
+            self.q.save_state(filename)
+        except Exception as ex:
+            self.fail("save_state() raised %r unexpectedly!" % ex)
diff --git a/releasenotes/notes/ping-healthcheck-selinux-e3b7d360c8503527.yaml b/releasenotes/notes/ping-healthcheck-selinux-e3b7d360c8503527.yaml
new file mode 100644
index 0000000000..70043c4348
--- /dev/null
+++ b/releasenotes/notes/ping-healthcheck-selinux-e3b7d360c8503527.yaml
@@ -0,0 +1,6 @@
+---
+issues:
+  - |
+    When using a distribution with a recent SELinux release such as CentOS 8
+    Stream, PING health-monitor does not work as shell_exec_t calls are denied
+    by SELinux.