ipatests: refactor test for login using cifs alias principal

The test had two problems:
* if it was failing,  samba services were not started and all other
tests also failed
* Utility for copying keys obscured fatal problems i.e. if file does not
exist or can not be parsed.

Fixed by moving the check to separate test and raising exceptions in
KerberosKeyCopier on any unexpected problem.

Reviewed-By: Alexander Bokovoy <abokovoy@redhat.com>

ipatests/pytest_ipa/integration/tasks.py

@@ -2044,10 +2044,7 @@ class KerberosKeyCopier:
             princ = self.host_princ_template.format(master=self.host.hostname,
                                                     realm=self.realm)
         result = self.host.run_command(
-            [paths.KLIST, "-eK", "-k", keytab],
-            log_stdout=False, raiseonerr=False)
-        if result.returncode != 0:
-            return None
+            [paths.KLIST, "-eK", "-k", keytab], log_stdout=False)
 
         keys_to_sync = []
         for l in result.stdout_text.splitlines():
@@ -2074,11 +2071,25 @@ class KerberosKeyCopier:
                     kvno=keyentry.kvno, etype=keyentry.etype,
                     key=keyentry.key[2:])
 
-        result = self.host.run_command(
-            [paths.KTUTIL], stdin_text=stdin,
-            raiseonerr=False, log_stdout=False)
+        def get_keytab_mtime():
+            """Get keytab file mtime.
 
-        return result.returncode == 0
+            Returns mtime with sub-second precision as a string with format
+            "2020-08-25 14:35:05.980503425 +0200" or None if file does not
+            exist.
+            """
+            if self.host.transport.file_exists(keytab):
+                return self.host.run_command(
+                    ['stat', '-c', '%y', keytab]).stdout_text.strip()
+            return None
+
+        mtime_before = get_keytab_mtime()
+
+        self.host.run_command([paths.KTUTIL], stdin_text=stdin,
+                              log_stdout=False)
+        if mtime_before == get_keytab_mtime():
+            raise Exception('{} did not update keytab file "{}"'.format(
+                paths.KTUTIL, keytab))
 
     def copy_keys(self, origin, destination, principal=None, replacement=None):
         def sync_keys(origkeys, destkeys):
@@ -2093,23 +2104,23 @@ class KerberosKeyCopier:
                             destkey.etype == origkey.etype]):
                         if any([destkey.key != origkey.key,
                                 destkey.kvno != origkey.kvno]):
-                            copied = self.copy_key(destination, origkey)
+                            self.copy_key(destination, origkey)
+                            copied = True
                             break
                         uptodate = True
                 if not (copied or uptodate):
-                    copied = self.copy_key(destination, origkey)
-            return copied or uptodate
+                    self.copy_key(destination, origkey)
 
         if not self.host.transport.file_exists(origin):
-            return False
+            raise ValueError('File "{}" does not exist'.format(origin))
         origkeys = self.extract_key_refs(origin, princ=principal)
         if self.host.transport.file_exists(destination):
             destkeys = self.extract_key_refs(destination)
             if any([origkeys is None, destkeys is None]):
-                logger.warning('Either %s or %s are missing or unreadable',
-                               origin, destination)
-                return False
-            return sync_keys(origkeys, destkeys)
+                raise Exception(
+                    'Either {} or {} are missing or unreadable'.format(
+                        origin, destination))
+            sync_keys(origkeys, destkeys)
         else:
             for origkey in origkeys:
                 if origkey.principal in replacement:
@@ -2117,9 +2128,7 @@ class KerberosKeyCopier:
                         [origkey.kvno, replacement.get(origkey.principal),
                          origkey.etype, origkey.key])
                     origkey = newkey
-                if not self.copy_key(destination, origkey):
-                    return False
-            return True
+                self.copy_key(destination, origkey)
 
 
 class FileBackup:

ipatests/test_integration/test_smb.py

@@ -229,28 +229,6 @@ class TestSMB(IntegrationTest):
         self.smbclient.run_command(['umount', mountpoint], raiseonerr=False)
         self.smbclient.run_command(['rmdir', mountpoint], raiseonerr=False)
 
-    def smb_cifs_principal_alias_check(self):
-        netbiosname = self.smbserver.hostname.split('.')[0].upper() + '$'
-        copier = tasks.KerberosKeyCopier(self.smbserver)
-
-        principal = 'cifs/{hostname}@{realm}'.format(
-            hostname=self.smbserver.hostname, realm=copier.realm)
-        alias = '{netbiosname}@{realm}'.format(
-            netbiosname=netbiosname, realm=copier.realm)
-        replacement = {principal: alias}
-
-        result = self.smbserver.run_command(['mktemp'])
-        # klist/ktutil will fail with 0-sized file
-        # so we just use the temporary file as a prefix
-        tmpname = result.stdout_text.strip() + '.keytab'
-
-        copier.copy_keys('/etc/samba/samba.keytab',
-                         tmpname, principal=principal, replacement=replacement)
-        self.smbserver.run_command(['kinit', '-kt', tmpname, netbiosname],
-                                   raiseonerr=True)
-        self.smbserver.run_command(['rm', '-f', tmpname])
-        self.smbserver.run_command(['rm', '-f', tmpname[:-7]])
-
     def test_samba_uninstallation_without_installation(self):
         res = self.smbserver.run_command(
             ['ipa-client-samba', '--uninstall', '-U'])
@@ -264,8 +242,6 @@ class TestSMB(IntegrationTest):
             result = self.smbserver.run_command(
                 ['systemctl', 'status', service], raiseonerr=False)
             assert result.returncode == 3
-        # Validate that we can authenticate with the service alias principal
-        self.smb_cifs_principal_alias_check()
         self.smbserver.run_command([
             'systemctl', 'enable', '--now', 'smb', 'winbind'
         ])
@@ -279,6 +255,30 @@ class TestSMB(IntegrationTest):
         # started - this way we prevent other tests from failing
         self.smb_installation_check(samba_install_result)
 
+    def test_authentication_with_smb_cifs_principal_alias(self):
+        """Test that we can auth as NetBIOS alias
+
+        cifs/... principal on SMB server side has NetBIOS name of the SMB
+        server as its alias. Test that we can actually initialize credentials
+        using this alias. We don't need to use it anywhere in Samba, just
+        verify that alias works.
+        Test for  https://pagure.io/freeipa/issue/8291"""
+        netbiosname = self.smbserver.hostname.split('.')[0].upper() + '$'
+        copier = tasks.KerberosKeyCopier(self.smbserver)
+
+        principal = 'cifs/{hostname}@{realm}'.format(
+            hostname=self.smbserver.hostname, realm=copier.realm)
+        alias = '{netbiosname}@{realm}'.format(
+            netbiosname=netbiosname, realm=copier.realm)
+        replacement = {principal: alias}
+        tmpname = tasks.create_temp_file(self.smbserver, create_file=False)
+        try:
+            copier.copy_keys(paths.SAMBA_KEYTAB, tmpname, principal=principal,
+                             replacement=replacement)
+            self.smbserver.run_command(['kinit', '-kt', tmpname, netbiosname])
+        finally:
+            self.smbserver.run_command(['rm', '-f', tmpname])
+
     def test_samba_service_listed(self):
         """Check samba service is listed.