Synchronise 2025.1 with upstream

octavia/common/jinja/haproxy/combined_listeners/jinja_cfg.py

@@ -18,13 +18,17 @@
 
 import jinja2
 from octavia_lib.common import constants as lib_consts
+from oslo_log import log as logging
 from oslo_utils import versionutils
 
 from octavia.common.config import cfg
 from octavia.common import constants
 from octavia.common import utils as octavia_utils
+from octavia.common import validate
 from octavia.db import models
 
+LOG = logging.getLogger(__name__)
+
 PROTOCOL_MAP = {
     constants.PROTOCOL_TCP: 'tcp',
     constants.PROTOCOL_HTTP: 'http',
@@ -362,7 +366,13 @@ def _transform_listener(self, listener: models.Listener, tls_certs,
                                  constants.PROTOCOL_PROMETHEUS):
             tls_enabled = True
             if listener.tls_ciphers is not None:
-                ret_value['tls_ciphers'] = listener.tls_ciphers
+                if not validate.VALID_CIPHER_RE.match(listener.tls_ciphers):
+                    LOG.warning("Listener %s has invalid tls_ciphers in DB, "
+                                "falling back to default", listener.id)
+                    ret_value['tls_ciphers'] = (
+                        cfg.CONF.api_settings.default_listener_ciphers)
+                else:
+                    ret_value['tls_ciphers'] = listener.tls_ciphers
             if listener.tls_versions is not None:
                 ret_value['tls_versions'] = listener.tls_versions
             if listener.alpn_protocols is not None:
@@ -447,7 +457,13 @@ def _transform_pool(self, pool, feature_compatibility,
             ret_value['client_cert'] = pool_tls_certs.get('client_cert')
         if pool.tls_enabled is True:
             if pool.tls_ciphers is not None:
-                ret_value['tls_ciphers'] = pool.tls_ciphers
+                if not validate.VALID_CIPHER_RE.match(pool.tls_ciphers):
+                    LOG.warning("Pool %s has invalid tls_ciphers in DB, "
+                                "falling back to default", pool.id)
+                    ret_value['tls_ciphers'] = (
+                        cfg.CONF.api_settings.default_pool_ciphers)
+                else:
+                    ret_value['tls_ciphers'] = pool.tls_ciphers
             if pool.tls_versions is not None:
                 ret_value['tls_versions'] = pool.tls_versions
             if (pool.alpn_protocols is not None and
@@ -516,6 +532,14 @@ def _transform_health_monitor(self, monitor, feature_compatibility):
             'domain_name': monitor.domain_name,
         }
 
+    @staticmethod
+    def _sanitize_url(value, field_name, obj_id):
+        if value and validate.INVALID_URL_CHARS_RE.search(value):
+            LOG.warning("L7policy %s has invalid %s in DB, clearing",
+                        obj_id, field_name)
+            return None
+        return value
+
     def _transform_l7policy(self, l7policy, feature_compatibility,
                             listener_tls_enabled, tls_certs=None):
         """Transforms an L7 policy into an object that will
@@ -525,8 +549,10 @@ def _transform_l7policy(self, l7policy, feature_compatibility,
         ret_value = {
             'id': l7policy.id,
             'action': l7policy.action,
-            'redirect_url': l7policy.redirect_url,
-            'redirect_prefix': l7policy.redirect_prefix,
+            'redirect_url': self._sanitize_url(
+                l7policy.redirect_url, 'redirect_url', l7policy.id),
+            'redirect_prefix': self._sanitize_url(
+                l7policy.redirect_prefix, 'redirect_prefix', l7policy.id),
             'enabled': l7policy.enabled
         }
         if (l7policy.redirect_pool and

octavia/common/jinja/haproxy/combined_listeners/templates/base.j2

@@ -29,10 +29,10 @@ global
     {% if ssl_cache is defined %}
     tune.ssl.cachesize {{ ssl_cache }}
     {% endif %}
-    {%- if cpu_count is defined and cpu_count > 1 %}
+    {% if cpu_count is defined and cpu_count > 1 %}
     nbthread {{ cpu_count - 1 }}
     cpu-map auto:1/1-{{ cpu_count - 1 }} 1-{{ cpu_count - 1 }}
-    {%- endif %}
+    {% endif %}
     {% set found_ns = namespace(found=false) %}
     {% for listener in loadbalancer.listeners if listener.enabled %}
         {% for pool in listener.pools if pool.enabled %}

octavia/common/validate.py

@@ -41,8 +41,18 @@
 _ListenerPUT = 'octavia.api.v2.types.listener.ListenerPUT'
 
 
+# Reject control characters and spaces in URLs before passing to rfc3986.
+# The rfc3986 library encodes these (e.g. \n -> %0A) before validating, so
+# they pass structural validation, but Octavia stores the raw input which
+# ends up in HAProxy config files — allowing config injection via newlines.
+INVALID_URL_CHARS_RE = re.compile(r'[\x00-\x20\x7f-\x9f]')
+
+
 def url(url, require_scheme=True):
     """Raises an error if the url doesn't look like a URL."""
+    if INVALID_URL_CHARS_RE.search(url):
+        raise exceptions.InvalidURL(url=url)
+
     validator = validators.Validator()
     if require_scheme:
         validator.allow_schemes('http', 'https')
@@ -450,7 +460,20 @@ def ip_not_reserved(ip_address):
                                        option='member address')
 
 
+VALID_CIPHER_RE = re.compile(r'^[A-Za-z0-9\-+!:@=_.]+\Z')
+
+
+def check_cipher_string(cipherstring):
+    """Validate that a cipher string contains only safe characters."""
+    if not VALID_CIPHER_RE.match(cipherstring):
+        raise exceptions.ValidationException(detail=_(
+            'Invalid characters in cipher string. '
+            'Only alphanumeric, -, +, !, :, @, =, _, and . '
+            'are allowed.'))
+
+
 def check_cipher_prohibit_list(cipherstring):
+    check_cipher_string(cipherstring)
     ciphers = cipherstring.split(':')
     prohibit_list = CONF.api_settings.tls_cipher_prohibit_list.split(':')
     rejected = []

octavia/tests/unit/common/jinja/haproxy/combined_listeners/test_jinja_cfg.py

@@ -918,6 +918,37 @@ def test_render_template_ping_monitor_http_insecure_fork(self):
         self.assertEqual(sample_configs_combined.sample_base_expected_config(
             backend=be, global_opts=go), rendered_obj)
 
+    def test_render_template_ping_monitor_http_with_cpu_count(self):
+        be = ("backend sample_pool_id_1:sample_listener_id_1\n"
+              "    mode http\n"
+              "    balance roundrobin\n"
+              "    cookie SRV insert indirect nocache\n"
+              "    timeout check 31s\n"
+              "    option external-check\n"
+              "    external-check command /var/lib/octavia/ping-wrapper.sh\n"
+              f"    fullconn {constants.HAPROXY_DEFAULT_MAXCONN}\n"
+              "    option allbackups\n"
+              "    timeout connect 5000\n"
+              "    timeout server 50000\n"
+              "    server sample_member_id_1 10.0.0.99:82 "
+              "weight 13 check inter 30s fall 3 rise 2 "
+              "cookie sample_member_id_1\n"
+              "    server sample_member_id_2 10.0.0.98:82 "
+              "weight 13 check inter 30s fall 3 rise 2 "
+              "cookie sample_member_id_2\n\n")
+        go = ("    maxconn 50000\n"
+              "    nbthread 6\n"
+              "    cpu-map auto:1/1-6 1-6\n"
+              "    external-check\n\n")
+        rendered_obj = self.jinja_cfg.render_loadbalancer_obj(
+            sample_configs_combined.sample_amphora_tuple(),
+            [sample_configs_combined.sample_listener_tuple(
+                proto='HTTP', monitor_proto='PING')],
+            amp_details={"cpu_count": 7,
+                         "active_tuned_profiles": "virtual-guest amphora"})
+        self.assertEqual(sample_configs_combined.sample_base_expected_config(
+            backend=be, global_opts=go), rendered_obj)
+
     def test_render_template_no_monitor_https(self):
         fe = ("frontend sample_listener_id_1\n"
               f"    maxconn {constants.HAPROXY_DEFAULT_MAXCONN}\n"
@@ -1739,6 +1770,30 @@ def test_transform_l7policy_disabled_rule(self):
         ret = self.jinja_cfg._transform_l7policy(in_l7policy, {}, False)
         self.assertEqual(sample_configs_combined.RET_L7POLICY_6, ret)
 
+    def test_transform_l7policy_redirect_url_injection(self):
+        in_l7policy = sample_configs_combined.sample_l7policy_tuple(
+            'sample_l7policy_id_2', sample_policy=2)
+        # Replace redirect_url with a tainted value
+        in_l7policy = in_l7policy._replace(
+            redirect_url='https://example.com/\ncheck')
+        ret = self.jinja_cfg._transform_l7policy(in_l7policy, {}, False)
+        self.assertIsNone(ret['redirect_url'])
+
+    def test_transform_l7policy_redirect_prefix_injection(self):
+        in_l7policy = sample_configs_combined.sample_l7policy_tuple(
+            'sample_l7policy_id_2', sample_policy=2)
+        in_l7policy = in_l7policy._replace(
+            redirect_prefix='https://example.com/\ncheck',
+            redirect_url=None)
+        ret = self.jinja_cfg._transform_l7policy(in_l7policy, {}, False)
+        self.assertIsNone(ret['redirect_prefix'])
+
+    def test_transform_l7policy_redirect_url_valid(self):
+        in_l7policy = sample_configs_combined.sample_l7policy_tuple(
+            'sample_l7policy_id_2', sample_policy=2)
+        ret = self.jinja_cfg._transform_l7policy(in_l7policy, {}, False)
+        self.assertEqual('http://www.example.com', ret['redirect_url'])
+
     def test_escape_haproxy_config_string(self):
         self.assertEqual(self.jinja_cfg._escape_haproxy_config_string(
             'string_with_none'), 'string_with_none')
@@ -1792,7 +1847,8 @@ def test_render_template_amp_details(self):
                     "    option http-keep-alive\n\n\n")
         global_opts = ("    maxconn 50000\n"
                        "    nbthread 6\n"
-                       "    cpu-map auto:1/1-6 1-6\n")
+                       "    cpu-map auto:1/1-6 1-6\n"
+                       "\n")
         self.assertEqual(
             sample_configs_combined.sample_base_expected_config(
                 defaults=defaults, logging="\n", global_opts=global_opts),
@@ -2086,3 +2142,40 @@ def test_build_config(self, mock_render_loadbalancer_obj):
             mock_amp, mock_listeners, tls_certs=mock_tls_certs,
             socket_path=mock_socket_path, amp_details=None,
             feature_compatibility=expected_fc)
+
+    def test_transform_listener_tls_ciphers_injection_fallback(self):
+        tainted = 'ECDHE-RSA-AES128-GCM-SHA256\ncheck'
+        in_listener = sample_configs_combined.sample_listener_tuple(
+            proto='TERMINATED_HTTPS', tls=True,
+            tls_ciphers=tainted)
+        ret = self.jinja_cfg._transform_listener(
+            in_listener, None, {}, in_listener.load_balancer)
+        # Should fall back to default, not use the tainted value
+        self.assertEqual(
+            cfg.CONF.api_settings.default_listener_ciphers,
+            ret['tls_ciphers'])
+
+    def test_transform_listener_tls_ciphers_valid(self):
+        valid = 'ECDHE-RSA-AES128-GCM-SHA256:AES256-SHA'
+        in_listener = sample_configs_combined.sample_listener_tuple(
+            proto='TERMINATED_HTTPS', tls=True,
+            tls_ciphers=valid)
+        ret = self.jinja_cfg._transform_listener(
+            in_listener, None, {}, in_listener.load_balancer)
+        self.assertEqual(valid, ret['tls_ciphers'])
+
+    def test_transform_pool_tls_ciphers_injection_fallback(self):
+        tainted = 'AES128-SHA\ncheck'
+        in_pool = sample_configs_combined.sample_pool_tuple(
+            tls_enabled=True, tls_ciphers=tainted)
+        ret = self.jinja_cfg._transform_pool(in_pool, {}, False)
+        self.assertEqual(
+            cfg.CONF.api_settings.default_pool_ciphers,
+            ret['tls_ciphers'])
+
+    def test_transform_pool_tls_ciphers_valid(self):
+        valid = 'ECDHE-RSA-AES128-GCM-SHA256:AES256-SHA'
+        in_pool = sample_configs_combined.sample_pool_tuple(
+            tls_enabled=True, tls_ciphers=valid)
+        ret = self.jinja_cfg._transform_pool(in_pool, {}, False)
+        self.assertEqual(valid, ret['tls_ciphers'])

octavia/tests/unit/common/test_validate.py

@@ -45,6 +45,31 @@ def test_validate_url_bad_schema(self):
         self.assertRaises(exceptions.InvalidURL, validate.url,
                           'ssh://www.example.com/')
 
+    def test_validate_url_newline_injection(self):
+        self.assertRaises(
+            exceptions.InvalidURL, validate.url,
+            'https://example.com/path\ncheck')
+
+    def test_validate_url_crlf_injection(self):
+        self.assertRaises(
+            exceptions.InvalidURL, validate.url,
+            'https://example.com/path\r\ncheck')
+
+    def test_validate_url_space(self):
+        self.assertRaises(
+            exceptions.InvalidURL, validate.url,
+            'https://example.com/path if check')
+
+    def test_validate_url_tab(self):
+        self.assertRaises(
+            exceptions.InvalidURL, validate.url,
+            'https://example.com/path\tcheck')
+
+    def test_validate_url_null(self):
+        self.assertRaises(
+            exceptions.InvalidURL, validate.url,
+            'https://example.com/path\x00check')
+
     def test_validate_url_path(self):
         self.assertTrue(validate.url_path('/foo'))
         self.assertTrue(validate.url_path('/foo%0Abar'))
@@ -464,6 +489,41 @@ def test_ip_not_reserved(self):
                           validate.ip_not_reserved,
                           '2001:0DB8::5')
 
+    def test_check_cipher_string_valid(self):
+        # Valid cipher strings should not raise
+        validate.check_cipher_string(
+            'ECDHE-RSA-AES128-GCM-SHA256:ECDHE-RSA-AES256-GCM-SHA384')
+        validate.check_cipher_string('!aNULL:!MD5:@STRENGTH')
+        validate.check_cipher_string('TLS_AES_256_GCM_SHA384')
+
+    def test_check_cipher_string_newline_injection(self):
+        self.assertRaises(
+            exceptions.ValidationException,
+            validate.check_cipher_string,
+            'ECDHE-RSA-AES128-GCM-SHA256\ncheck')
+
+    def test_check_cipher_string_space_injection(self):
+        self.assertRaises(
+            exceptions.ValidationException,
+            validate.check_cipher_string,
+            'ECDHE-RSA-AES128-GCM-SHA256 #')
+
+    def test_check_cipher_string_trailing_newline(self):
+        self.assertRaises(
+            exceptions.ValidationException,
+            validate.check_cipher_string,
+            'foo\n')
+
+    def test_check_cipher_string_control_chars(self):
+        self.assertRaises(
+            exceptions.ValidationException,
+            validate.check_cipher_string,
+            'AES128\r\ncheck')
+        self.assertRaises(
+            exceptions.ValidationException,
+            validate.check_cipher_string,
+            'AES128\tcheck')
+
     def test_check_default_ciphers_prohibit_list_conflict(self):
         self.conf.config(group='api_settings',
                          tls_cipher_prohibit_list='PSK-AES128-CBC-SHA')

releasenotes/notes/fix-cpu-map-external-check-newline-b2162906.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    Fixed HAProxy config generation when both cpu-map (multi vCPU amphora)
+    and external-check (PING health monitor) are enabled. A missing newline
+    in the Jinja2 template caused ``external-check`` to be appended to the
+    ``cpu-map`` line, resulting in a fatal HAProxy configuration error.

releasenotes/notes/fix-redirect-url-haproxy-config-injection-b2c3d4e5f6a7b8c9.yaml

@@ -0,0 +1,12 @@
+---
+security:
+  - |
+    Fixed a HAProxy configuration injection vulnerability via the
+    ``redirect_url`` and ``redirect_prefix`` fields on L7 policies.
+    The ``rfc3986`` URL validator encodes control characters before
+    validation, so newlines and spaces passed structural checks but
+    were stored raw and written directly into the HAProxy configuration.
+    The URL validator now rejects URLs containing control characters
+    (U+0000-U+001F, U+007F-U+009F) and spaces before passing them to
+    ``rfc3986``. Existing invalid values in the database are detected
+    at config generation time and cleared.

releasenotes/notes/fix-tls-ciphers-haproxy-config-injection-a1b2c3d4e5f6a7b8.yaml

@@ -0,0 +1,11 @@
+---
+security:
+  - |
+    Fixed a HAProxy configuration injection vulnerability via the
+    ``tls_ciphers`` field on listeners and pools. The field was not validated
+    for control characters, allowing an attacker to inject arbitrary HAProxy
+    configuration directives by embedding newlines in the cipher string. The
+    API now rejects cipher strings containing characters outside the set
+    allowed by OpenSSL (alphanumeric, ``-``, ``+``, ``!``, ``:``, ``@``, ``=``,
+    ``_``, ``.``). Existing invalid values in the database are detected at
+    config generation time and replaced with the configured defaults.