On 07/18/2013 09:25 AM, Jan Cholasta wrote:
> On 17.7.2013 19:43, Ana Krivokapic wrote:
>> On 07/17/2013 06:04 PM, Jan Cholasta wrote:
>>> On 17.7.2013 17:39, Ana Krivokapic wrote:
>>>> On 07/17/2013 04:57 PM, Jan Cholasta wrote:
>>>>> Hi,
>>>>>
>>>>> On 17.7.2013 16:38, Ana Krivokapic wrote:
>>>>>> Hello,
>>>>>>
>>>>>> This patch addresses ticket https://fedorahosted.org/freeipa/ticket/3785.
>>>>>>
>>>>>
>>>>> NACK, this results in an unnecessarily ugly error message "[Errno 2] No 
>>>>> such
>>>>> file or directory: 'file'".
>>>>>
>>>>> I would suggest something like this instead:
>>>>>
>>>>> except IOError as e:
>>>>>       raise ScriptError("Failed to open %s: %s" % (ca_cert_name, 
>>>>> e.strerror))
>>>>
>>>> Fixed.
>>>
>>> Hmm, seeing how RuntimeError is used for this kind of thing in 
>>> import_pkcs12,
>>> I think it would make sense to catch the IOError right in import_pem_cert 
>>> and
>>> re-raise it as RuntimeError and then handle that RuntimeError in 
>>> check_pkcs12
>>> (sorry for misleading you into doing something else in my previous mail).
>>
>> I don't see much value in doing that - it just adds complexity. I would 
>> rather
>> leave it as it is.
>
> All the other NSSDatabase methods raise RuntimeErrors when something goes
> wrong, including I/O errors. IMO having consistent API is preferable to saving
> 2 lines of code.
>
>>>
>>>>>
>>>>> Can you please also check what happens if you pass non-existent filename 
>>>>> to
>>>>> --dirsrv_pkcs12 and --http_pkcs12?
>>>>>
>>>>> Honza
>>>>>
>>>>
>>>> I added a more specific error message to cover these cases.
>>>
>>> Can you please also add it to find_root_cert_from_pkcs12?
>>
>> Done, thanks for catching that.
>>>
>>>>
>>>> Updated patch attached.
>>>>
>>>
>>> Honza
>>>
>>
>> Updated patch is attached.
>>
>
> Honza
>

Ok, fixed.

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

From d88444351061fd8c188098dc019c6ff0220bd4f3 Mon Sep 17 00:00:00 2001
From: Ana Krivokapic <akriv...@redhat.com>
Date: Wed, 17 Jul 2013 16:30:15 +0200
Subject: [PATCH] Properly handle non-existent cert files

https://fedorahosted.org/freeipa/ticket/3785
---
 ipaserver/install/certs.py        | 13 +++++++++++--
 ipaserver/install/installutils.py |  2 +-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/ipaserver/install/certs.py b/ipaserver/install/certs.py
index 06925d53b2fa6df6d94d41d758944b6497ce2bcd..6d01d2be1e8f7ea2db35db55e7cc6d2bb092a281 100644
--- a/ipaserver/install/certs.py
+++ b/ipaserver/install/certs.py
@@ -188,6 +188,8 @@ def import_pkcs12(self, pkcs12_filename, db_password_filename,
             if e.returncode == 17:
                 raise RuntimeError("incorrect password for pkcs#12 file %s" %
                     pkcs12_filename)
+            elif e.returncode == 10:
+                raise RuntimeError("Failed to open %s" % pkcs12_filename)
             else:
                 raise RuntimeError("unknown error import pkcs#12 file %s" %
                     pkcs12_filename)
@@ -206,6 +208,8 @@ def find_root_cert_from_pkcs12(self, pkcs12_fname, passwd_fname=None):
         except ipautil.CalledProcessError, e:
             if e.returncode == 17:
                 raise RuntimeError("incorrect password for pkcs#12 file")
+            elif e.returncode == 10:
+                raise RuntimeError("Failed to open %s" % pkcs12_fname)
             else:
                 raise RuntimeError("unknown error using pkcs#12 file")
 
@@ -255,8 +259,13 @@ def import_pem_cert(self, nickname, flags, location):
 
         The file must contain exactly one certificate.
         """
-        with open(location) as fd:
-            certs = fd.read()
+        try:
+            with open(location) as fd:
+                certs = fd.read()
+        except IOError as e:
+            raise RuntimeError(
+                "Failed to open %s: %s" % (location, e.strerror)
+            )
 
         cert, st = find_cert_from_txt(certs)
         self.add_single_pem_cert(nickname, flags, cert)
diff --git a/ipaserver/install/installutils.py b/ipaserver/install/installutils.py
index a716525b3ebc20fe516613d57f19377519212a5a..d23f9b57ff3a31ab17ed126c504c8075f30de642 100644
--- a/ipaserver/install/installutils.py
+++ b/ipaserver/install/installutils.py
@@ -721,7 +721,7 @@ def check_pkcs12(pkcs12_info, ca_file, hostname):
         ca_cert_name = 'The Root CA'
         try:
             nssdb.import_pem_cert(ca_cert_name, "CT,C,C", ca_file)
-        except ValueError, e:
+        except (ValueError, RuntimeError) as e:
             raise ScriptError(str(e))
 
         # Import everything in the PKCS#12
-- 
1.8.1.4

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to