Check for file permissions after the ca/cert-show is complete

The commands ca-show and cert-show provide the ability to direct
the certificate output to a file. If the requested object was
not present then this resulted in a zero-length file.

This is because the check to determine if the file was writable,
by opening it, was done prior to the operation to retrieve
the entry.

So move the check after the data retrieval.

Also convert cert-show to be more consistent with ca-show.

I considered cleaning up the empty file afterward but IMHO we
shouldn't touch the file until we're ready to write. This
costs an API roundtrip but its a small price to pay for
potentially protecting existing data.

Fixes: https://pagure.io/freeipa/issue/9562

Signed-off-by: Rob Crittenden <rcritten@redhat.com>
Reviewed-By: Florence Blanc-Renaud <flo@redhat.com>

ipaclient/plugins/ca.py

@@ -26,15 +26,16 @@ class WithCertOutArgs(MethodOverride):
         filename = None
         if 'certificate_out' in options:
             filename = options.pop('certificate_out')
+
+        result = super(WithCertOutArgs, self).forward(*keys, **options)
+
+        if filename:
             try:
                 util.check_writable_file(filename)
             except errors.FileError as e:
                 raise errors.ValidationError(name='certificate-out',
                                              error=str(e))
 
-        result = super(WithCertOutArgs, self).forward(*keys, **options)
-
-        if filename:
             # if result certificate / certificate_chain not present in result,
             # it means Dogtag did not provide it (probably due to LWCA key
             # replication lag or failure.  The server transmits a warning

ipaclient/plugins/cert.py

@@ -43,25 +43,24 @@ class CertRetrieveOverride(MethodOverride):
     )
 
     def forward(self, *args, **options):
+        filename = None
         if 'certificate_out' in options:
-            certificate_out = options.pop('certificate_out')
+            filename = options.pop('certificate_out')
-            try:
-                util.check_writable_file(certificate_out)
-            except errors.FileError as e:
-                raise errors.ValidationError(name='certificate-out',
-                                             error=str(e))
-        else:
-            certificate_out = None
 
         result = super(CertRetrieveOverride, self).forward(*args, **options)
 
-        if certificate_out is not None:
+        if filename is not None:
+            try:
+                util.check_writable_file(filename)
+            except errors.FileError as e:
+                raise errors.ValidationError(name='certificate-out',
+                                             error=str(e))
             if options.get('chain', False):
                 certs = result['result']['certificate_chain']
             else:
                 certs = [base64.b64decode(result['result']['certificate'])]
             certs = (x509.load_der_x509_certificate(cert) for cert in certs)
-            x509.write_certificate_list(certs, certificate_out)
+            x509.write_certificate_list(certs, filename)
 
         return result
 

ipatests/test_integration/test_commands.py

@@ -748,6 +748,27 @@ class TestIPACommand(IntegrationTest):
 
             self.master.run_command(['rm', '-f', filename])
 
+        # Ensure that ca/cert-show doesn't leave an empty file when
+        # the requested ca/cert does not exist.
+        commands = [
+            ['ipa', 'cert-show', '0xdeadbeef', '--certificate-out'],
+            ['ipa', 'ca-show', 'notfound', '--certificate-out'],
+        ]
+
+        for command in commands:
+            cmd = self.master.run_command(['mktemp', '--dry-run'])
+            filename = cmd.stdout_text.strip()
+
+            result = self.master.run_command(command + [filename],
+                                             raiseonerr=False)
+            assert result.returncode == 2
+
+            result = self.master.run_command(
+                ['stat', filename],
+                raiseonerr=False
+            )
+            assert result.returncode == 1
+
     def test_sssd_ifp_access_ipaapi(self):
         # check that ipaapi is allowed to access sssd-ifp for smartcard auth
         # https://pagure.io/freeipa/issue/7751