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 <rcritten@redhat.com>
Reviewed-By: Alexander Bokovoy <abokovoy@redhat.com>

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 = (

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)

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("""
+            <?xml version="1.0" encoding="UTF-8"?>
+            <Server port="1234" shutdown="SHUTDOWN">
+              <Service name="Catalina">
+                <Connector port="9000" protocol="AJP/1.3" redirectPort="443"
+                 address="localhost" requiredSecret="testing_ajp_secret"
+                 secret="other_secret" />
+              </Service>
+            </Server>
+        """),
+        ('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