openstack
/
octavia
Code Issues Proposed changes

Validate certificate content at API level 

Starting from Rocky, certificates are loaded still at API level when
converting objects to provider data models. The act of loading the
certificate provides validation as to its content. For example, it
checks if a value in the Common Name field is set. When the content is
passed in on create and update actions via reference, it is checked at
API level. If it's invalid, it fails right there and an error is
returned to the user.

Although, certificate content is not checked at API level in Queens.
Should an invalid certificate be passed in, the API accepts but it will
later fail at provisioning -- the listener and loadbalancer go into
ERROR. The problem starts when the health manager runs the periodic
update health check. It calculates the expected number of listeners and
sees the listener in ERROR. In an attempt to heal it, an amphora
failover is triggered. As it runs the failover, it tries, again, to load
up the invalid certificate. Amphora failover goes on in a loop.

This patch is a Queens-only patch.

Task: 34261
Story: 2005925

Change-Id: Ib0fb3db3b2d56345eeeda2014c15e35281bcfec8
This commit is contained in:
Carlos Goncalves 2019-06-24 20:56:50 +02:00
parent d9c459a868
commit 26d8fde69f
5 changed files with 51 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
octavia/api/v2/controllers/listener.py
View File

@ -30,6 +30,7 @@ from octavia.common import constants
from octavia.common import data_models
from octavia.common import exceptions
from octavia.common import stats
from octavia.common.tls_utils import cert_parser
from octavia.db import api as db_api
from octavia.db import prepare as db_prepare
@ -139,7 +140,8 @@ class ListenersController(base.BaseController):
bad_refs = []
for ref in tls_refs:
try:
self.cert_manager.get_cert(context, ref, check_only=True)
cert_parser.load_certificate_data(
self.cert_manager, ref, context)
except Exception:
bad_refs.append(ref)

19
octavia/common/tls_utils/cert_parser.py
View File

@ -334,6 +334,12 @@ def build_pem(tls_container):
return b'\n'.join(pem) + b'\n'
def load_certificate_data(cert_mngr, cert_ref, context):
"""Load TLS certificate data."""
return _map_cert_tls_container(
cert_mngr.get_cert(context, cert_ref, check_only=True))
def load_certificates_data(cert_mngr, listener, context=None):
"""Load TLS certificate data from the listener.
@ -345,16 +351,13 @@ def load_certificates_data(cert_mngr, listener, context=None):
context = oslo_context.RequestContext(project_id=listener.project_id)
if listener.tls_certificate_id:
tls_cert = _map_cert_tls_container(
cert_mngr.get_cert(context,
listener.tls_certificate_id,
check_only=True))
tls_cert = load_certificate_data(
cert_mngr, listener.tls_certificate_id, context)
if listener.sni_containers:
for sni_cont in listener.sni_containers:
cert_container = _map_cert_tls_container(
cert_mngr.get_cert(context,
sni_cont.tls_container_id,
check_only=True))
cert_container = load_certificate_data(
cert_mngr, sni_cont.tls_container_id, context)
sni_certs.append(cert_container)
return {'tls_cert': tls_cert, 'sni_certs': sni_certs}

40
octavia/tests/functional/api/v2/test_listener.py
View File

@ -437,7 +437,8 @@ class TestListener(base.BaseAPITest):
listener_path = self.listener_path
self.get(listener_path.format(listener_id='SEAN-CONNERY'), status=404)
def test_create(self, **optionals):
@mock.patch('octavia.common.tls_utils.cert_parser.load_certificate_data')
def test_create(self, mock_load_cert, **optionals):
sni1 = uuidutils.generate_uuid()
sni2 = uuidutils.generate_uuid()
lb_listener = {'name': 'listener1', 'default_pool_id': None,
@ -480,7 +481,8 @@ class TestListener(base.BaseAPITest):
self.create_listener(constants.PROTOCOL_HTTP, 80, self.lb_id,
status=409)
def test_create_bad_tls_ref(self):
@mock.patch('octavia.common.tls_utils.cert_parser.load_certificate_data')
def test_create_bad_tls_ref(self, mock_load_cert):
sni1 = uuidutils.generate_uuid()
sni2 = uuidutils.generate_uuid()
tls_ref = uuidutils.generate_uuid()
@ -492,7 +494,7 @@ class TestListener(base.BaseAPITest):
'loadbalancer_id': self.lb_id}
body = self._build_body(lb_listener)
self.cert_manager_mock().get_cert.side_effect = [
mock_load_cert.side_effect = [
Exception("bad cert"), None, Exception("bad_cert")]
response = self.post(self.LISTENERS_PATH, body, status=400).json
self.assertIn(sni1, response['faultstring'])
@ -597,7 +599,8 @@ class TestListener(base.BaseAPITest):
listener_prov_status=constants.ERROR,
listener_op_status=constants.OFFLINE)
def test_create_authorized(self, **optionals):
@mock.patch('octavia.common.tls_utils.cert_parser.load_certificate_data')
def test_create_authorized(self, mock_load_cert, **optionals):
sni1 = uuidutils.generate_uuid()
sni2 = uuidutils.generate_uuid()
lb_listener = {'name': 'listener1', 'default_pool_id': None,
@ -723,7 +726,8 @@ class TestListener(base.BaseAPITest):
listener_id=api_listener.get('id'),
listener_prov_status=constants.ERROR)
def test_update(self):
@mock.patch('octavia.common.tls_utils.cert_parser.load_certificate_data')
def test_update(self, mock_load_cert):
tls_uuid = uuidutils.generate_uuid()
listener = self.create_listener(
constants.PROTOCOL_TCP, 80, self.lb_id,
@ -774,7 +778,8 @@ class TestListener(base.BaseAPITest):
self.assert_final_listener_statuses(self.lb_id,
listener['listener']['id'])
def test_update_unset_defaults(self):
@mock.patch('octavia.common.tls_utils.cert_parser.load_certificate_data')
def test_update_unset_defaults(self, mock_load_cert):
tls_uuid = uuidutils.generate_uuid()
listener = self.create_listener(
constants.PROTOCOL_TERMINATED_HTTPS, 80, self.lb_id,
@ -796,7 +801,8 @@ class TestListener(base.BaseAPITest):
self.assertEqual([], api_listener['sni_container_refs'])
self.assertNotEqual(listener, api_listener)
def test_update_authorized(self):
@mock.patch('octavia.common.tls_utils.cert_parser.load_certificate_data')
def test_update_authorized(self, mock_load_cert):
tls_uuid = uuidutils.generate_uuid()
listener = self.create_listener(
constants.PROTOCOL_TCP, 80, self.lb_id,
@ -850,7 +856,8 @@ class TestListener(base.BaseAPITest):
self.assert_final_listener_statuses(self.lb_id,
api_listener['id'])
def test_update_not_authorized(self):
@mock.patch('octavia.common.tls_utils.cert_parser.load_certificate_data')
def test_update_not_authorized(self, mock_load_cert):
tls_uuid = uuidutils.generate_uuid()
listener = self.create_listener(
constants.PROTOCOL_TCP, 80, self.lb_id,
@ -1037,7 +1044,8 @@ class TestListener(base.BaseAPITest):
listener_id=listener['listener'].get('id'))
self.put(listener_path, {}, status=400)
def test_update_bad_tls_ref(self):
@mock.patch('octavia.common.tls_utils.cert_parser.load_certificate_data')
def test_update_bad_tls_ref(self, mock_load_cert):
sni1 = uuidutils.generate_uuid()
sni2 = uuidutils.generate_uuid()
tls_ref = uuidutils.generate_uuid()
@ -1060,7 +1068,7 @@ class TestListener(base.BaseAPITest):
body = self._build_body(lb_listener_put)
listener_path = self.LISTENER_PATH.format(
listener_id=api_listener['id'])
self.cert_manager_mock().get_cert.side_effect = [
mock_load_cert.side_effect = [
Exception("bad cert"), None, Exception("bad cert")]
response = self.put(listener_path, body, status=400).json
self.assertIn(tls_ref2, response['faultstring'])
@ -1155,7 +1163,8 @@ class TestListener(base.BaseAPITest):
listener_id=api_listener['id'])
self.delete(listener_path, status=204)
def test_create_with_tls_termination_data(self):
@mock.patch('octavia.common.tls_utils.cert_parser.load_certificate_data')
def test_create_with_tls_termination_data(self, mock_load_cert):
cert_id = uuidutils.generate_uuid()
listener = self.create_listener(constants.PROTOCOL_TERMINATED_HTTPS,
80, self.lb_id,
@ -1165,7 +1174,8 @@ class TestListener(base.BaseAPITest):
get_listener = self.get(listener_path).json['listener']
self.assertEqual(cert_id, get_listener['default_tls_container_ref'])
def test_update_with_tls_termination_data(self):
@mock.patch('octavia.common.tls_utils.cert_parser.load_certificate_data')
def test_update_with_tls_termination_data(self, mock_load_cert):
cert_id = uuidutils.generate_uuid()
listener = self.create_listener(constants.PROTOCOL_TERMINATED_HTTPS,
80, self.lb_id)
@ -1192,7 +1202,8 @@ class TestListener(base.BaseAPITest):
.format(constants.PROTOCOL_TERMINATED_HTTPS),
listener.get('faultstring'))
def test_create_with_sni_data(self):
@mock.patch('octavia.common.tls_utils.cert_parser.load_certificate_data')
def test_create_with_sni_data(self, mock_load_cert):
sni_id1 = uuidutils.generate_uuid()
sni_id2 = uuidutils.generate_uuid()
listener = self.create_listener(constants.PROTOCOL_HTTP, 80,
@ -1204,7 +1215,8 @@ class TestListener(base.BaseAPITest):
self.assertItemsEqual([sni_id1, sni_id2],
get_listener['sni_container_refs'])
def test_update_with_sni_data(self):
@mock.patch('octavia.common.tls_utils.cert_parser.load_certificate_data')
def test_update_with_sni_data(self, mock_load_cert):
sni_id1 = uuidutils.generate_uuid()
sni_id2 = uuidutils.generate_uuid()
listener = self.create_listener(constants.PROTOCOL_HTTP, 80,

9
octavia/tests/functional/api/v2/test_load_balancer.py
View File

@ -2209,7 +2209,8 @@ class TestLoadBalancerGraph(base.BaseAPITest):
api_lb = response.json.get(self.root_tag)
self._assert_graphs_equal(expected_lb, api_lb)
def test_with_one_listener_sni_containers(self):
@mock.patch('octavia.common.tls_utils.cert_parser.load_certificate_data')
def test_with_one_listener_sni_containers(self, mock_load_cert):
create_sni_containers, expected_sni_containers = (
self._get_sni_container_bodies())
create_listener, expected_listener = self._get_listener_bodies(
@ -2415,7 +2416,8 @@ class TestLoadBalancerGraph(base.BaseAPITest):
body = self._build_body(create_lb)
return body, expected_lb
def test_with_one_of_everything(self):
@mock.patch('octavia.common.tls_utils.cert_parser.load_certificate_data')
def test_with_one_of_everything(self, mock_load_cert):
body, expected_lb = self._test_with_one_of_everything_helper()
response = self.post(self.LBS_PATH, body)
api_lb = response.json.get(self.root_tag)
@ -2497,7 +2499,8 @@ class TestLoadBalancerGraph(base.BaseAPITest):
self.start_quota_mock(data_models.HealthMonitor)
self.post(self.LBS_PATH, body, status=403)
def test_create_over_quota_sanity_check(self):
@mock.patch('octavia.common.tls_utils.cert_parser.load_certificate_data')
def test_create_over_quota_sanity_check(self, mock_load_cert):
# This one should create, as we don't check quotas on L7Policies
body, _ = self._test_with_one_of_everything_helper()
self.start_quota_mock(data_models.L7Policy)

5
releasenotes/notes/fix-certificate-validation-d65df8ff16e7f985.yaml Normal file
View File

@ -0,0 +1,5 @@
---
fixes:
- |
Fixed an issue where invalid certificates would trigger an amphora failover
loop. Certificates are now validated at API level.