From d093cbb3a9443e9a95abfdac4001e9d40d5e1878 Mon Sep 17 00:00:00 2001 From: Carlos Goncalves Date: Thu, 6 Aug 2020 12:07:58 +0200 Subject: [PATCH] Fix listener tls_versions and tls_ciphers clearing Clearning tls_versions was throwing a TypeError ('NoneType' object is not iterable). This patch fixes that and resets tls_versions and tls_ciphers to their default values set in the API configuration file. Task: 40607 Story: 2007981 Change-Id: I4540bbb2cb5b1200b268300cb8a9f0ad1e5104a9 --- octavia/api/v2/controllers/listener.py | 5 + octavia/common/validate.py | 5 + .../tests/functional/api/v2/test_listener.py | 155 +++++++++++++++++- ...ons-ciphers-clearing-4e987ee17d6b6e79.yaml | 8 + 4 files changed, 171 insertions(+), 2 deletions(-) create mode 100644 releasenotes/notes/fix-listener-tls-versions-ciphers-clearing-4e987ee17d6b6e79.yaml diff --git a/octavia/api/v2/controllers/listener.py b/octavia/api/v2/controllers/listener.py index 53bf4f2ccd..09d16f1b71 100644 --- a/octavia/api/v2/controllers/listener.py +++ b/octavia/api/v2/controllers/listener.py @@ -538,6 +538,11 @@ class ListenersController(base.BaseController): CONF.haproxy_amphora.timeout_tcp_inspect) if listener.client_authentication is None: listener.client_authentication = constants.CLIENT_AUTH_NONE + if listener.tls_ciphers is None: + listener.tls_ciphers = CONF.api_settings.default_listener_ciphers + if listener.tls_versions is None: + listener.tls_versions = ( + CONF.api_settings.default_listener_tls_versions) @wsme_pecan.wsexpose(listener_types.ListenerRootResponse, wtypes.text, body=listener_types.ListenerRootPUT, status_code=200) diff --git a/octavia/common/validate.py b/octavia/common/validate.py index c8361f4a67..fafc8f7d01 100644 --- a/octavia/common/validate.py +++ b/octavia/common/validate.py @@ -466,6 +466,11 @@ def check_tls_version_list(versions): raise exceptions.ValidationException( detail=_('Empty TLS version list. Either specify at least one TLS ' 'version or remove this parameter to use the default.')) + + # Unset action + if versions is None: + return + invalid_versions = [v for v in versions if v not in constants.TLS_ALL_VERSIONS] if invalid_versions: diff --git a/octavia/tests/functional/api/v2/test_listener.py b/octavia/tests/functional/api/v2/test_listener.py index 1671b1f5ed..0c3e2aae17 100644 --- a/octavia/tests/functional/api/v2/test_listener.py +++ b/octavia/tests/functional/api/v2/test_listener.py @@ -17,6 +17,7 @@ import copy import random from unittest import mock +from octavia_lib.common import constants as lib_consts from oslo_config import cfg from oslo_config import fixture as oslo_fixture from oslo_utils import uuidutils @@ -1205,6 +1206,72 @@ class TestListener(base.BaseAPITest): self.create_listener(constants.PROTOCOL_TCP, 80, self.lb_id, allowed_cidrs=allowed_cidrs) + # TODO(johnsom) Fix this when there is a noop certificate manager + @mock.patch('octavia.common.tls_utils.cert_parser.load_certificates_data') + def test_create_with_tls_versions(self, mock_cert_data): + cert1 = data_models.TLSContainer(certificate='cert 1') + mock_cert_data.return_value = {'tls_cert': cert1} + cert_id = uuidutils.generate_uuid() + tls_versions = constants.TLS_VERSIONS_OWASP_SUITE_B + listener = self.create_listener(constants.PROTOCOL_TERMINATED_HTTPS, + 80, self.lb_id, + default_tls_container_ref=cert_id, + tls_versions=tls_versions) + listener_path = self.LISTENER_PATH.format( + listener_id=listener['listener']['id']) + get_listener = self.get(listener_path).json['listener'] + self.assertEqual(tls_versions, get_listener['tls_versions']) + + # TODO(johnsom) Fix this when there is a noop certificate manager + @mock.patch('octavia.common.tls_utils.cert_parser.load_certificates_data') + def test_create_with_tls_versions_negative(self, mock_cert_data): + cert1 = data_models.TLSContainer(certificate='cert 1') + mock_cert_data.return_value = {'tls_cert': cert1} + cert_id = uuidutils.generate_uuid() + req_dict = {'protocol': constants.PROTOCOL_TERMINATED_HTTPS, + 'protocol_port': 80, + 'loadbalancer_id': self.lb_id, + 'default_tls_container_ref': cert_id, + 'tls_versions': [lib_consts.TLS_VERSION_1_3, 'insecure']} + res = self.post(self.LISTENERS_PATH, self._build_body(req_dict), + status=400) + fault = res.json['faultstring'] + self.assertIn('Validation failure: Invalid TLS versions', fault) + self.assert_correct_status(lb_id=self.lb_id) + + # TODO(johnsom) Fix this when there is a noop certificate manager + @mock.patch('octavia.common.tls_utils.cert_parser.load_certificates_data') + def test_create_with_tls_ciphers(self, mock_cert_data): + cert1 = data_models.TLSContainer(certificate='cert 1') + mock_cert_data.return_value = {'tls_cert': cert1} + cert_id = uuidutils.generate_uuid() + tls_ciphers = constants.CIPHERS_OWASP_SUITE_B + listener = self.create_listener(constants.PROTOCOL_TERMINATED_HTTPS, + 80, self.lb_id, + default_tls_container_ref=cert_id, + tls_ciphers=tls_ciphers) + listener_path = self.LISTENER_PATH.format( + listener_id=listener['listener']['id']) + get_listener = self.get(listener_path).json['listener'] + self.assertEqual(tls_ciphers, get_listener['tls_ciphers']) + + # TODO(johnsom) Fix this when there is a noop certificate manager + @mock.patch('octavia.common.tls_utils.cert_parser.load_certificates_data') + def test_create_with_tls_ciphers_negative(self, mock_cert_data): + cert1 = data_models.TLSContainer(certificate='cert 1') + mock_cert_data.return_value = {'tls_cert': cert1} + cert_id = uuidutils.generate_uuid() + req_dict = {'protocol': constants.PROTOCOL_TERMINATED_HTTPS, + 'protocol_port': 80, + 'loadbalancer_id': self.lb_id, + 'default_tls_container_ref': cert_id, + 'tls_ciphers': ['cipher-insecure']} + res = self.post(self.LISTENERS_PATH, self._build_body(req_dict), + status=400) + fault = res.json['faultstring'] + self.assertIn('Invalid input for field/attribute tls_ciphers', fault) + self.assert_correct_status(lb_id=self.lb_id) + def _test_negative_create_with_headers(self, protocol): req_dict = {'name': 'listener1', 'default_pool_id': None, 'description': 'desc1', @@ -1357,6 +1424,77 @@ class TestListener(base.BaseAPITest): api_listener = response.json.get(self.root_tag) self.assertEqual('listener2', api_listener['name']) + # TODO(johnsom) Fix this when there is a noop certificate manager + @mock.patch('octavia.common.tls_utils.cert_parser.load_certificates_data') + def test_update_with_tls_versions(self, mock_cert_data): + cert_id = uuidutils.generate_uuid() + cert1 = data_models.TLSContainer(certificate='cert 1') + mock_cert_data.return_value = {'tls_cert': cert1} + tls_versions_orig = [lib_consts.TLS_VERSION_1_1] + tls_versions = [lib_consts.TLS_VERSION_1_2, lib_consts.TLS_VERSION_1_3] + listener = self.create_listener( + constants.PROTOCOL_TERMINATED_HTTPS, 80, self.lb_id, + default_tls_container_ref=cert_id, + tls_versions=tls_versions_orig) + self.set_lb_status(self.lb_id) + listener_path = self.LISTENER_PATH.format( + listener_id=listener['listener']['id']) + get_listener = self.get(listener_path).json['listener'] + self.assertEqual(tls_versions_orig, + get_listener.get('tls_versions')) + self.put(listener_path, + self._build_body({'tls_versions': tls_versions})) + get_listener = self.get(listener_path).json['listener'] + self.assertEqual(tls_versions, get_listener.get('tls_versions')) + + # TODO(johnsom) Fix this when there is a noop certificate manager + @mock.patch('octavia.common.tls_utils.cert_parser.load_certificates_data') + def test_update_with_tls_versions_negative(self, mock_cert_data): + cert_id = uuidutils.generate_uuid() + cert1 = data_models.TLSContainer(certificate='cert 1') + mock_cert_data.return_value = {'tls_cert': cert1} + tls_versions_orig = [lib_consts.TLS_VERSION_1_1] + listener = self.create_listener( + constants.PROTOCOL_TERMINATED_HTTPS, 80, self.lb_id, + default_tls_container_ref=cert_id, + tls_versions=tls_versions_orig) + self.set_lb_status(self.lb_id) + listener_path = self.LISTENER_PATH.format( + listener_id=listener['listener']['id']) + get_listener = self.get(listener_path).json['listener'] + self.assertEqual(tls_versions_orig, get_listener.get('tls_versions')) + + req_dict = {'tls_versions': [lib_consts.TLS_VERSION_1_3, 'insecure']} + res = self.put(listener_path, self._build_body(req_dict), + status=400) + fault = res.json['faultstring'] + self.assertIn('Validation failure: Invalid TLS versions:', fault) + self.assert_correct_status(lb_id=self.lb_id) + + # TODO(johnsom) Fix this when there is a noop certificate manager + @mock.patch('octavia.common.tls_utils.cert_parser.load_certificates_data') + def test_update_with_tls_ciphers_negative(self, mock_cert_data): + cert_id = uuidutils.generate_uuid() + cert1 = data_models.TLSContainer(certificate='cert 1') + mock_cert_data.return_value = {'tls_cert': cert1} + tls_ciphers_orig = constants.CIPHERS_OWASP_SUITE_B + listener = self.create_listener( + constants.PROTOCOL_TERMINATED_HTTPS, 80, self.lb_id, + default_tls_container_ref=cert_id, + tls_ciphers=tls_ciphers_orig) + self.set_lb_status(self.lb_id) + listener_path = self.LISTENER_PATH.format( + listener_id=listener['listener']['id']) + get_listener = self.get(listener_path).json['listener'] + self.assertEqual(tls_ciphers_orig, get_listener.get('tls_ciphers')) + + req_dict = {'tls_ciphers': ['cipher-insecure']} + res = self.put(listener_path, self._build_body(req_dict), + status=400) + fault = res.json['faultstring'] + self.assertIn('Invalid input for field/attribute tls_ciphers', fault) + self.assert_correct_status(lb_id=self.lb_id) + def test_negative_update_udp_case(self): api_listener = self.create_listener(constants.PROTOCOL_UDP, 6666, self.lb_id).get(self.root_tag) @@ -1611,6 +1749,12 @@ class TestListener(base.BaseAPITest): self.conf.config(group='haproxy_amphora', timeout_member_connect=21) self.conf.config(group='haproxy_amphora', timeout_member_data=22) self.conf.config(group='haproxy_amphora', timeout_tcp_inspect=23) + self.conf.config(group='api_settings', + default_listener_tls_versions=( + constants.TLS_VERSIONS_OWASP_SUITE_B)) + self.conf.config(group='api_settings', + default_listener_ciphers=( + constants.CIPHERS_OWASP_SUITE_B)) self.cert_manager_mock().get_secret.side_effect = [ sample_certs.X509_CA_CERT, sample_certs.X509_CA_CRL, @@ -1635,7 +1779,9 @@ class TestListener(base.BaseAPITest): timeout_member_data=3, timeout_tcp_inspect=4, client_authentication=constants.CLIENT_AUTH_OPTIONAL, client_crl_container_ref=crl_tls_uuid, - client_ca_tls_container_ref=ca_tls_uuid).get(self.root_tag) + client_ca_tls_container_ref=ca_tls_uuid, + tls_versions=[lib_consts.TLS_VERSION_1_3], + tls_ciphers='TLS_AES_256_GCM_SHA384').get(self.root_tag) self.set_lb_status(self.lb_id) unset_params = { 'name': None, 'description': None, 'connection_limit': None, @@ -1644,7 +1790,8 @@ class TestListener(base.BaseAPITest): 'timeout_member_connect': None, 'timeout_member_data': None, 'timeout_tcp_inspect': None, 'client_ca_tls_container_ref': None, 'client_authentication': None, 'default_pool_id': None, - 'client_crl_container_ref': None} + 'client_crl_container_ref': None, 'tls_versions': None, + 'tls_ciphers': None} body = self._build_body(unset_params) listener_path = self.LISTENER_PATH.format( listener_id=listener['id']) @@ -1666,6 +1813,10 @@ class TestListener(base.BaseAPITest): self.assertEqual(constants.CLIENT_AUTH_NONE, api_listener['client_authentication']) self.assertIsNone(api_listener['default_pool_id']) + self.assertEqual(constants.TLS_VERSIONS_OWASP_SUITE_B, + api_listener['tls_versions']) + self.assertEqual(constants.CIPHERS_OWASP_SUITE_B, + api_listener['tls_ciphers']) @mock.patch('octavia.common.tls_utils.cert_parser.load_certificates_data') def test_update_with_bad_ca_cert(self, mock_cert_data): diff --git a/releasenotes/notes/fix-listener-tls-versions-ciphers-clearing-4e987ee17d6b6e79.yaml b/releasenotes/notes/fix-listener-tls-versions-ciphers-clearing-4e987ee17d6b6e79.yaml new file mode 100644 index 0000000000..09a975fec3 --- /dev/null +++ b/releasenotes/notes/fix-listener-tls-versions-ciphers-clearing-4e987ee17d6b6e79.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + Fixed an issue where clearing listener TLS versions resulted in a + server-side error. + - | + Fixed an issue where when clearing listener TLS versions and ciphers would + not apply the default values per defined in the API configuration settings.