This patch attempts to make the uninstaller less scary. We were throwing a lot of odd errors about the client uninstall failing and certmonger failures. The problem was the leap of faith I was taking when untracking certs. I'm a lot more careful about how I do it.

I've moved certmonger.py to ipapython as it is now shared between the client and server.


rob
>From 822c744761751ac9b410c79672aa2a0b562c531f Mon Sep 17 00:00:00 2001
From: Rob Crittenden <rcrit...@redhat.com>
Date: Tue, 31 Aug 2010 17:21:25 -0400
Subject: [PATCH] Fix certmonger errors when doing a client or server uninstall.

This started with the client uninstaller returning a 1 when not installed.
There was no way to tell whether the uninstall failed or the client
simply wasn't installed which caused no end of grief with the installer.

This led to a lot of certmonger failures too, either trying to stop
tracking a non-existent cert or not handling an existing tracked
certificate.

I moved the certmonger code out of the installer and put it into the
client/server shared ipapython lib. It now tries a lot harder and smarter
to untrack a certificate.

ticket 142
---
 install/tools/ipa-server-install          |    5 +-
 ipa-client/ipa-install/ipa-client-install |   19 ++-
 ipa-client/man/ipa-client-install.1       |    2 +
 ipapython/certmonger.py                   |  248 +++++++++++++++++++++++++++++
 ipaserver/install/certmonger.py           |  152 ------------------
 ipaserver/install/certs.py                |   27 ++--
 ipaserver/install/dsinstance.py           |    4 +-
 7 files changed, 280 insertions(+), 177 deletions(-)
 create mode 100644 ipapython/certmonger.py
 delete mode 100644 ipaserver/install/certmonger.py

diff --git a/install/tools/ipa-server-install b/install/tools/ipa-server-install
index 89bb83e..900f17a 100755
--- a/install/tools/ipa-server-install
+++ b/install/tools/ipa-server-install
@@ -381,11 +381,12 @@ def uninstall(dm_password=None):
         api.Backend.ldap2.connect(bind_dn="cn=Directory Manager", bind_pw=dm_password)
 
     try:
-        run(["/usr/sbin/ipa-client-install", "--on-master", "--unattended", "--uninstall"])
+        (stdout, stderr, rc) = run(["/usr/sbin/ipa-client-install", "--on-master", "--unattended", "--uninstall"], raiseonerr=False)
+        if rc != 2:
+            raise RuntimeException(stdout)
     except Exception, e:
         print "Uninstall of client side components failed!"
         print "ipa-client-install returned: " + str(e)
-        pass
 
     ntpinstance.NTPInstance(fstore).uninstall()
     if cainstance.CADSInstance().is_configured():
diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install
index ebf7dd0..c87bf01 100755
--- a/ipa-client/ipa-install/ipa-client-install
+++ b/ipa-client/ipa-install/ipa-client-install
@@ -34,6 +34,7 @@ try:
     from ipapython.ipautil import run, user_input, CalledProcessError, file_exists
     from ipapython import sysrestore
     from ipapython import version
+    from ipapython import certmonger
     import SSSDConfig
     from ConfigParser import RawConfigParser
 except ImportError:
@@ -174,7 +175,7 @@ def uninstall(options):
 
     if not fstore.has_files() and not options.force:
         print "IPA client is not configured on this system."
-        return 1
+        return 2
 
     # Remove our host cert and CA cert
     if nickname_exists("IPA CA"):
@@ -183,19 +184,25 @@ def uninstall(options):
         except Exception, e:
             print "Failed to remove IPA CA from /etc/pki/nssdb: %s" % str(e)
     if nickname_exists("Server-Cert"):
+        # Always start certmonger. We can't untrack something if it isn't
+        # running
+        try:
+            service('certmonger', 'start')
+        except:
+            pass
+        try:
+            certmonger.stop_tracking('/etc/pki/nssdb', nickname='Server-Cert')
+        except (CalledProcessError, RuntimeError), e:
+            logging.error("certmonger failed to stop tracking certificate: %s" % str(e))
         try:
             run(["/usr/bin/certutil", "-D", "-d", "/etc/pki/nssdb", "-n", "Server-Cert"])
         except Exception, e:
             print "Failed to remove Server-Cert from /etc/pki/nssdb: %s" % str(e)
-        try:
-            run(["/usr/bin/ipa-getcert", "stop-tracking", "-d", "/etc/pki/nssdb", "-n", "Server-Cert"])
-        except Exception, e:
-            print "Failed to stop tracking Server-Cert in certmonger: %s" % str(e)
 
     try:
         service('certmonger', 'stop')
     except:
-        print "Failed to stop the certmonger daemon"
+        pass
 
     try:
         chkconfig('certmonger', 'off')
diff --git a/ipa-client/man/ipa-client-install.1 b/ipa-client/man/ipa-client-install.1
index 4a1fcb5..4cdc921 100644
--- a/ipa-client/man/ipa-client-install.1
+++ b/ipa-client/man/ipa-client-install.1
@@ -81,3 +81,5 @@ Remove the IPA client software and restore the configuration to the pre-IPA stat
 0 if the installation was successful
 
 1 if an error occurred
+
+2 if uninstalling and the client is not configured
diff --git a/ipapython/certmonger.py b/ipapython/certmonger.py
new file mode 100644
index 0000000..667a70d
--- /dev/null
+++ b/ipapython/certmonger.py
@@ -0,0 +1,248 @@
+# Authors: Rob Crittenden <rcrit...@redhat.com>
+#
+# Copyright (C) 2010  Red Hat
+# see file 'COPYING' for use and warranty information
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation; version 2 only
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+#
+
+# Some certmonger functions, mostly around updating the request file.
+# This is used so we can add tracking to the Apache and 389-ds
+# server certificates created during the IPA server installation.
+
+import os
+import re
+import time
+from ipapython import ipautil
+
+REQUEST_DIR='/var/lib/certmonger/requests/'
+
+def find_request_value(filename, directive):
+    """
+    Return a value from a certmonger request file for the requested directive
+
+    It tries to do this a number of times because sometimes there is a delay
+    when ipa-getcert returns and the file is fully updated, particularly
+    when doing a request. Genrerating a CSR is fast but not instantaneous.
+    """
+    tries = 1
+    value = None
+    found = False
+    while value is None and tries <= 5:
+        tries=tries + 1
+        time.sleep(1)
+        fp = open(filename, 'r')
+        lines = fp.readlines()
+        fp.close()
+
+        for line in lines:
+            if found:
+                # A value can span multiple lines. If it does then it has a
+                # leading space.
+                if not line.startswith(' '):
+                    # We hit the next directive, return now
+                    return value
+                else:
+                    value = value + line[1:]
+            else:
+                if line.startswith(directive + '='):
+                    found = True
+                    value = line[len(directive)+1:]
+
+    return value
+
+def get_request_value(request_id, directive):
+    """
+    There is no guarantee that the request_id will match the filename
+    in the certmonger requests directory, so open each one to find the
+    request_id.
+    """
+    fileList=os.listdir(REQUEST_DIR)
+    for file in fileList:
+        value = find_request_value('%s/%s' % (REQUEST_DIR, file), 'id')
+        if value is not None and value.rstrip() == request_id:
+            return find_request_value('%s/%s' % (REQUEST_DIR, file), directive)
+
+    return None
+
+def get_request_id(criteria):
+    """
+    If you don't know the certmonger request_id then try to find it by looking
+    through all the request files. An alternative would be to parse the
+    ipa-getcert list output but this seems cleaner.
+
+    criteria is a tuple of key/value pairs to search for. The more specific
+    the better. An error is raised if multiple request_ids are returned for
+    the same criteria.
+
+    None is returned if none of the criteria match.
+    """
+    assert type(criteria) is tuple
+
+    reqid=None
+    fileList=os.listdir(REQUEST_DIR)
+    for file in fileList:
+        match = True
+        for (key, value) in criteria:
+            rv = find_request_value('%s/%s' % (REQUEST_DIR, file), key)
+            if rv is None or rv.rstrip() != value:
+                match = False
+                break
+        if match and reqid is not None:
+            raise RuntimeError('multiple certmonger requests match the criteria')
+        if match:
+            reqid = find_request_value('%s/%s' % (REQUEST_DIR, file), 'id').rstrip()
+
+    return reqid
+
+def add_request_value(request_id, directive, value):
+    """
+    Add a new directive to a certmonger request file.
+
+    The certmonger service MUST be stopped in order for this to work.
+    """
+    fileList=os.listdir(REQUEST_DIR)
+    for file in fileList:
+        id = find_request_value('%s/%s' % (REQUEST_DIR, file), 'id')
+        if id is not None and id.rstrip() == request_id:
+            current_value = find_request_value('%s/%s' % (REQUEST_DIR, file), directive)
+            if not current_value:
+                fp = open('%s/%s' % (REQUEST_DIR, file), 'a')
+                fp.write('%s=%s\n' % (directive, value))
+                fp.close()
+
+    return
+
+def add_principal(request_id, principal):
+    """
+    In order for a certmonger request to be renwable it needs a principal.
+
+    When an existing certificate is added via start-tracking it won't have
+    a principal.
+    """
+    return add_request_value(request_id, 'template_principal', principal)
+
+def add_subject(request_id, subject):
+    """
+    In order for a certmonger request to be renwable it needs the subject
+    set in the request file.
+
+    When an existing certificate is added via start-tracking it won't have
+    a subject_template set.
+    """
+    return add_request_value(request_id, 'template_subject', subject)
+
+def request_cert(nssdb, nickname, subject, principal, passwd_fname=None):
+    """
+    Execute certmonger to request a server certificate
+    """
+    args = ['/usr/bin/ipa-getcert',
+            'request',
+            '-d', nssdb,
+            '-n', nickname,
+            '-N', subject,
+            '-K', principal,
+    ]
+    if passwd_fname:
+        args.append('-p')
+        args.append(passwd_fname)
+    (stdout, stderr, returncode) = ipautil.run(args)
+    # FIXME: should be some error handling around this
+    m = re.match('New signing request "(\d+)" added', stdout)
+    request_id = m.group(1)
+    return request_id
+
+def cert_exists(nickname, secdir):
+    """
+    See if a nickname exists in an NSS database.
+
+    Returns True/False
+
+    This isn't very sophisticated in that it doesn't differentiate between
+    a database that doesn't exist and a nickname that doesn't exist within
+    the database.
+    """
+    args = ["/usr/bin/certutil", "-L",
+           "-d", secdir,
+           "-n", nickname
+          ]
+    (stdout, stderr, rc) = ipautil.run(args, raiseonerr=False)
+    if rc == 0:
+        return True
+    else:
+        return False
+
+def start_tracking(nickname, secdir, password_file=None):
+    """
+    Tell certmonger to track the given certificate nickname in NSS
+    database in secdir protected by optional password file password_file.
+
+    Returns the stdout, stderr and returncode from running ipa-getcert
+
+    This assumes that certmonger is already running.
+    """
+    if not cert_exists(nickname, secdir):
+        raise RuntimeError('Nickname "%s" doesn\'t exist in NSS database "%s"' % (nickname, secdir))
+    args = ["/usr/bin/ipa-getcert", "start-tracking",
+            "-d", secdir,
+            "-n", nickname]
+    if password_file:
+        args.append("-p")
+        args.append(password_file)
+
+    (stdout, stderr, returncode) = ipautil.run(args)
+
+    return (stdout, stderr, returncode)
+
+def stop_tracking(secdir, request_id=None, nickname=None):
+    """
+    Stop tracking the current request using either the request_id or nickname.
+
+    This assumes that the certmonger service is running.
+    """
+    if request_id is None and nickname is None:
+        raise RuntimeError('Both request_id and nickname are missing.')
+    if nickname:
+        # Using the nickname find the certmonger request_id
+        criteria = (('cert_storage_location','%s' % secdir),('cert_nickname', '%s' % nickname))
+        try:
+            request_id = get_request_id(criteria)
+            if request_id is None:
+                return ('', '', 0)
+        except RuntimeError:
+            # This means that multiple requests matched, skip it for now
+            # Fall back to trying to stop tracking using nickname
+            pass
+
+    args = ['/usr/bin/ipa-getcert',
+            'stop-tracking',
+    ]
+    if request_id:
+        args.append('-i')
+        args.append(request_id)
+    else:
+        args.append('-n')
+        args.append(nickname)
+        args.append('-d')
+        args.append(secdir)
+
+    (stdout, stderr, returncode) = ipautil.run(args)
+
+    return (stdout, stderr, returncode)
+
+if __name__ == '__main__':
+    request_id = request_cert("/etc/httpd/alias", "Test", "cn=tiger.example.com,O=IPA", "HTTP/tiger.example....@example.com")
+    csr = get_request_value(request_id, 'csr')
+    print csr
+    stop_tracking(request_id)
diff --git a/ipaserver/install/certmonger.py b/ipaserver/install/certmonger.py
deleted file mode 100644
index bb56c2a..0000000
--- a/ipaserver/install/certmonger.py
+++ /dev/null
@@ -1,152 +0,0 @@
-# Authors: Rob Crittenden <rcrit...@redhat.com>
-#
-# Copyright (C) 2010  Red Hat
-# see file 'COPYING' for use and warranty information
-#
-# This program is free software; you can redistribute it and/or
-# modify it under the terms of the GNU General Public License as
-# published by the Free Software Foundation; version 2 only
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program; if not, write to the Free Software
-# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
-#
-
-# Some certmonger functions, mostly around updating the request file.
-# This is used so we can add tracking to the Apache and 389-ds
-# server certificates created during the IPA server installation.
-
-import os
-import re
-import time
-from ipapython import ipautil
-
-REQUEST_DIR='/var/lib/certmonger/requests/'
-
-def find_request_value(filename, directive):
-    """
-    Return a value from a certmonger request file for the requested directive
-
-    It tries to do this a number of times because sometimes there is a delay
-    when ipa-getcert returns and the file is fully updated, particularly
-    when doing a request. Genrerating a CSR is fast but not instantaneous.
-    """
-    tries = 1
-    value = None
-    found = False
-    while value is None and tries <= 5:
-        tries=tries + 1
-        time.sleep(1)
-        fp = open(filename, 'r')
-        lines = fp.readlines()
-        fp.close()
-
-        for line in lines:
-            if found:
-                # A value can span multiple lines. If it does then it has a
-                # leading space.
-                if not line.startswith(' '):
-                    # We hit the next directive, return now
-                    return value
-                else:
-                    value = value + line[1:]
-            else:
-                if line.startswith(directive + '='):
-                    found = True
-                    value = line[len(directive)+1:]
-
-    return value
-
-def get_request_value(request_id, directive):
-    """
-    There is no guarantee that the request_id will match the filename
-    in the certmonger requests directory, so open each one to find the
-    request_id.
-    """
-    fileList=os.listdir(REQUEST_DIR)
-    for file in fileList:
-        value = find_request_value('%s/%s' % (REQUEST_DIR, file), 'id')
-        if value is not None and value.rstrip() == request_id:
-            return find_request_value('%s/%s' % (REQUEST_DIR, file), directive)
-
-    return None
-
-def add_request_value(request_id, directive, value):
-    """
-    Add a new directive to a certmonger request file.
-
-    The certmonger service MUST be stopped in order for this to work.
-    """
-    fileList=os.listdir(REQUEST_DIR)
-    for file in fileList:
-        id = find_request_value('%s/%s' % (REQUEST_DIR, file), 'id')
-        if id is not None and id.rstrip() == request_id:
-            current_value = find_request_value('%s/%s' % (REQUEST_DIR, file), directive)
-            if not current_value:
-                fp = open('%s/%s' % (REQUEST_DIR, file), 'a')
-                fp.write('%s=%s\n' % (directive, value))
-                fp.close()
-
-    return
-
-def add_principal(request_id, principal):
-    """
-    In order for a certmonger request to be renwable it needs a principal.
-
-    When an existing certificate is added via start-tracking it won't have
-    a principal.
-    """
-    return add_request_value(request_id, 'template_principal', principal)
-
-def add_subject(request_id, subject):
-    """
-    In order for a certmonger request to be renwable it needs the subject
-    set in the request file.
-
-    When an existing certificate is added via start-tracking it won't have
-    a subject_template set.
-    """
-    return add_request_value(request_id, 'template_subject', subject)
-
-def request_cert(nssdb, nickname, subject, principal, passwd_fname=None):
-    """
-    Execute certmonger to request a server certificate
-    """
-    args = ['/usr/bin/ipa-getcert',
-            'request',
-            '-d', nssdb,
-            '-n', nickname,
-            '-N', subject,
-            '-K', principal,
-    ]
-    if passwd_fname:
-        args.append('-p')
-        args.append(passwd_fname)
-    (stdout, stderr, returncode) = ipautil.run(args)
-    # FIXME: should be some error handling around this
-    m = re.match('New signing request "(\d+)" added', stdout)
-    request_id = m.group(1)
-    return request_id
-
-def stop_tracking(request_id):
-    """
-    Stop tracking the current request.
-
-    This assumes that the certmonger service is running.
-    """
-    args = ['/usr/bin/ipa-getcert',
-            'stop-tracking',
-            '-i', request_id
-    ]
-    (stdout, stderr, returncode) = ipautil.run(args)
-
-if __name__ == '__main__':
-    request_id = request_cert("/etc/httpd/alias", "Test", "cn=tiger.example.com,O=IPA", "HTTP/tiger.example....@example.com")
-    csr = get_request_value(request_id, 'csr')
-    print csr
-    stop_tracking(request_id)
diff --git a/ipaserver/install/certs.py b/ipaserver/install/certs.py
index 7f246d1..c8e1d17 100644
--- a/ipaserver/install/certs.py
+++ b/ipaserver/install/certs.py
@@ -32,10 +32,10 @@ from ipapython import nsslib
 from ipapython import dogtag
 from ipapython import sysrestore
 from ipapython import ipautil
+from ipapython import certmonger
 from ipalib import pkcs10
 from ConfigParser import RawConfigParser
 import service
-import certmonger
 from ipalib import x509
 
 from nss.error import NSPRError
@@ -441,21 +441,19 @@ class CertDB(object):
         """
         service.chkconfig_on("certmonger")
         service.start("certmonger")
-        args = ["/usr/bin/ipa-getcert", "start-tracking",
-                "-d", self.secdir,
-                "-n", nickname]
-        if password_file:
-            args.append("-p")
-            args.append(password_file)
         try:
-            (stdout, stderr, returncode) = ipautil.run(args)
-        except ipautil.CalledProcessError, e:
-            logging.error("tracking certificate failed: %s" % str(e))
+            (stdout, stderr, rc) = certmonger.start_tracking(nickname, self.secdir, password_file)
+        except (ipautil.CalledProcessError, RuntimeError), e:
+            logging.error("certmonger failed starting to track certificate: %s" % str(e))
+            return
 
         service.stop("certmonger")
         cert = self.get_cert_from_db(nickname)
         subject = str(x509.get_subject(cert))
         m = re.match('New tracking request "(\d+)" added', stdout)
+        if not m:
+            logging.error('Didn\'t get new certmonger request, got %s' % stdout)
+            raise RuntimeError('certmonger did not issue new tracking request for \'%s\' in \'%s\'. Use \'ipa-getcert list\' to list existing certificates.' % (nickname, self.secdir))
         request_id = m.group(1)
 
         certmonger.add_principal(request_id, principal)
@@ -471,13 +469,10 @@ class CertDB(object):
         # Always start certmonger. We can't untrack something if it isn't
         # running
         service.start("certmonger")
-        args = ["/usr/bin/ipa-getcert", "stop-tracking",
-                "-d", self.secdir,
-                "-n", nickname]
         try:
-            (stdout, stderr, returncode) = ipautil.run(args)
-        except ipautil.CalledProcessError, e:
-            logging.error("untracking certificate failed: %s" % str(e))
+            certmonger.stop_tracking(self.secdir, nickname=nickname)
+        except (ipautil.CalledProcessError, RuntimeError), e:
+            logging.error("certmonger failed to stop tracking certificate: %s" % str(e))
         service.stop("certmonger")
 
     def create_server_cert(self, nickname, hostname, other_certdb=None, subject=None):
diff --git a/ipaserver/install/dsinstance.py b/ipaserver/install/dsinstance.py
index 5b3bd2a..9199f9c 100644
--- a/ipaserver/install/dsinstance.py
+++ b/ipaserver/install/dsinstance.py
@@ -498,7 +498,9 @@ class DsInstance(service.Service):
 
         serverid = self.restore_state("serverid")
         if not serverid is None:
-            dirname = config_dirname(serverid)
+            # drop the trailing / off the config_dirname so the directory
+            # will match what is in certmonger
+            dirname = config_dirname(serverid)[:-1]
             dsdb = certs.CertDB(dirname)
             dsdb.untrack_server_cert("Server-Cert")
             erase_ds_instance_data(serverid)
-- 
1.7.2.1

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

Reply via email to