From 7572174175415a113a95c1094503bb1102257610 Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: Thu, 9 Jun 2022 13:33:55 -0400 Subject: [PATCH] Remove extraneous AJP secret from server.xml on upgrades PKI 10.10 unconditionally added an upgrade script for the AJP connector which replaced the AJP secret regardless of tomcat version. It replaced requiredSecret with secret. IPA expects the attribute by version so this could make the secrets out of date and/or have connectors with both secrets and different values. PKI commit e70373ab131aba810f318c1d917896392b49ff4b has since been reverted but there may be servers with both secrets still. On next IPA upgrade clean them up. Also allow re-writing ipa-pki-proxy.conf in case the secret changes to ensure they remain in sync. Fixes: https://pagure.io/freeipa/issue/9176 Signed-off-by: Rob Crittenden Reviewed-By: Alexander Bokovoy --- ipaserver/install/dogtaginstance.py | 12 +++++++- ipaserver/install/server/upgrade.py | 28 +++++++++++++------ .../test_secure_ajp_connector.py | 27 +++++++++++++++++- 3 files changed, 56 insertions(+), 11 deletions(-) diff --git a/ipaserver/install/dogtaginstance.py b/ipaserver/install/dogtaginstance.py index 0877204d7..c2c6b3f49 100644 --- a/ipaserver/install/dogtaginstance.py +++ b/ipaserver/install/dogtaginstance.py @@ -357,7 +357,7 @@ class DogtagInstance(service.Service): # no AJP connector means no need to update anything connectors = doc.xpath('//Connector[@protocol="AJP/1.3"]') if len(connectors) == 0: - return + return False # Whether or not we should rewrite the tomcat server.xml file with # our changes. @@ -377,6 +377,14 @@ class DogtagInstance(service.Service): # First, iterate through all adapters and see if any of them have a # secret value set. for connector in connectors: + if not self._is_newer_tomcat_version('9.0.31.0'): + # For a time pki unconditionally added "secret" to the + # connector. Remove it prior to probing for the current + # secret. + if 'secret' in connector.attrib: + del connector.attrib['secret'] + rewrite = True + if secretattr in connector.attrib or oldattr in connector.attrib: # secret is already in place # @@ -438,6 +446,8 @@ class DogtagInstance(service.Service): os.fchmod(fd.fileno(), 0o660) self.service_user.chown(fd.fileno()) + return rewrite + def http_proxy(self): """ Update the http proxy file """ template_filename = ( diff --git a/ipaserver/install/server/upgrade.py b/ipaserver/install/server/upgrade.py index c97092345..757d20dc4 100644 --- a/ipaserver/install/server/upgrade.py +++ b/ipaserver/install/server/upgrade.py @@ -172,12 +172,17 @@ def find_version(filename): else: return -1 -def upgrade_file(sub_dict, filename, template, add=False): + +def upgrade_file(sub_dict, filename, template, add=False, force=False): """ Get the version from the current and template files and update the installed configuration file if there is a new template. If add is True then create a new configuration file. + + If force is True then the version comparison is skipped. This should + be used judiciously. It does not override add nor will it affect + files that don't exist (version == -1). """ old = int(find_version(filename)) new = int(find_version(template)) @@ -199,7 +204,10 @@ def upgrade_file(sub_dict, filename, template, add=False): "overwritten. A backup of the original will be made.", filename) - if old < new or (add and old == 0): + if force: + logger.error("Forcing update of template %s", template) + + if ((old < new) or (add and old == 0)) or force: backup_file(filename, new) update_conf(sub_dict, filename, template) logger.info("Upgraded %s to version %d", filename, new) @@ -1733,19 +1741,21 @@ def upgrade_configuration(): "ipa-kdc-proxy.conf.template")) if ca.is_configured(): # Handle upgrade of AJP connector configuration - ca.secure_ajp_connector() + rewrite = ca.secure_ajp_connector() if ca.ajp_secret: sub_dict['DOGTAG_AJP_SECRET'] = "secret={}".format( ca.ajp_secret) else: sub_dict['DOGTAG_AJP_SECRET'] = '' - upgrade_file( - sub_dict, - paths.HTTPD_IPA_PKI_PROXY_CONF, - os.path.join(paths.USR_SHARE_IPA_DIR, - "ipa-pki-proxy.conf.template"), - add=True) + # force=True will ensure the secret is updated if it changes + if rewrite: + upgrade_file( + sub_dict, + paths.HTTPD_IPA_PKI_PROXY_CONF, + os.path.join(paths.USR_SHARE_IPA_DIR, + "ipa-pki-proxy.conf.template"), + add=True, force=True) else: if os.path.isfile(paths.HTTPD_IPA_PKI_PROXY_CONF): os.remove(paths.HTTPD_IPA_PKI_PROXY_CONF) diff --git a/ipatests/test_ipaserver/test_secure_ajp_connector.py b/ipatests/test_ipaserver/test_secure_ajp_connector.py index 35ef7407a..eef607b33 100644 --- a/ipatests/test_ipaserver/test_secure_ajp_connector.py +++ b/ipatests/test_ipaserver/test_secure_ajp_connector.py @@ -83,6 +83,7 @@ test_data = ( """), ('secret',), ('testing_ajp_secret',), + ('requiredSecret',), True, ), ( @@ -99,6 +100,7 @@ test_data = ( """), ('secret',), ('testing_ajp_secret',), + ('requiredSecret',), False, ), ( @@ -117,6 +119,7 @@ test_data = ( """), ('secret', 'secret'), ('testing_ajp_secret', 'testing_ajp_secret'), + ('requiredSecret', 'requiredSecret'), True, ), ( @@ -135,6 +138,7 @@ test_data = ( """), ('secret', 'secret'), ('testing_ajp_secret', 'testing_ajp_secret'), + ('requiredSecret', 'requiredSecret'), False, ), ( @@ -153,6 +157,7 @@ test_data = ( """), ('secret', 'secret'), ('RANDOM', 'RANDOM'), + ('requiredSecret', 'requiredSecret'), True, ), ( @@ -169,8 +174,27 @@ test_data = ( """), ('requiredSecret',), ('testing_ajp_secret',), + ('secret',), False, ), + ( + # Case 7: Older tomcat, both secrets are present, one s/b removed + False, + textwrap.dedent(""" + + + + + + + """), + ('requiredSecret',), + ('testing_ajp_secret',), + ('secret',), + True, + ), ) @@ -180,7 +204,7 @@ class TestAJPSecretUpgrade: @patch("ipaserver.install.dogtaginstance.lxml.etree.parse") @pytest.mark.parametrize("test_data", test_data) def test_connecter(self, mock_parse, mock_chown, mock_getpwnam, test_data): - is_newer, data, secret, expect, rewrite = test_data + is_newer, data, secret, expect, ex_secret, rewrite = test_data mock_chown.return_value = None mock_parse.return_value = mock_etree_parse(data) mock_getpwnam.return_value = mock_pkiuser_entity() @@ -203,6 +227,7 @@ class TestAJPSecretUpgrade: assert connector.attrib[secret[i]] else: assert connector.attrib[secret[i]] == expect[i] + assert connector.attrib.get(ex_secret[i]) is None i += 1 else: assert mocked_file().write.call_args is None