On Mon, Aug 29, 2016 at 06:39:58PM +0200, Martin Babinsky wrote:
> On 08/23/2016 08:40 AM, Fraser Tweedale wrote:
> > Hi folks,
> > 
> > Please review attached patch which fixes
> > https://fedorahosted.org/freeipa/ticket/6019.
> > 
> > Thanks,
> > Fraser
> > 
> > 
> > 
> Hi Fraser,
> 
> I have couple of comments:
> 
Thanks for your review, Martin.  Updated patch attached.  Comments
inline.

> 1.)
> -            for entry in lwcas:
> -                self.server_track_lightweight_ca(entry)
> +            try:
> +                from ipaserver.install import cainstance
> + cainstance.add_lightweight_ca_tracking_requests(self.log, lwcas)
> +            except Exception as e:
> +                self.log.exception(
> +                    "Failed to add lightweight CA tracking requests")
> 
> You are importing a server-side module in a basically client-side command
> which I don't like very much. Isn't there a possibility to use shared
> client-server module for this?
> 
It's ugly.  It is an effect of my desire to keep LWCA tracking code
where IMO it belongs: in cainstance module.

If you know a nicer way to conditionally get at contents of
cainstance module I'm happy to do it differently.  Otherwise I don't
think it's a showstopper.

> 2.)
> +    def __add_lightweight_ca_tracking_requests(self):
> +        server_id = installutils.realm_to_serverid(api.env.realm)
> +        dogtag_uri = 'ldapi://%%2fvar%%2frun%%2fslapd-%s.socket' %
> server_id
> +        conn = ldap2.ldap2(api, ldap_uri=dogtag_uri)
> +        is_already_connected = conn.isconnected()
> 
> Why use these connection setup shenanigans when you can use either
> api.Backend.ldap2 (IIRC this runs in server context so LDAPI should be a
> given) or even the service's own 'admin_conn' member.
> 
I changed it to use admin_conn.

> +
> +        if not is_already_connected:
> +            try:
> +                conn.connect(autobind=True)
> +            except errors.PublicError as e:
> +                self.log.error(
> +                    "Cannot connect to LDAP to add "
> +                        "lightweight CA tracking requests: %s",
> +                    e
> +                )
> PEP8 error here, the second line of the message is misformatted.
>
Thanks, fixed.

> +                return
> +
> +        try:
> +            lwcas = conn.get_entries(
> +                base_dn=ipautil.realm_to_suffix(api.env.realm),
> +                filter='(objectclass=ipaca)',
> +                attrs_list=['cn', 'ipacaid'],
> +            )
> I would rather use the result of api.Command.ca_find to fetch sub-CAs. Also,
> ipautil.realm_to_suffix is superseded by api.env.basedn to fetch search
> base.
> 
Updated to use api.env.basedn.  I hit problems using ca_find and
connecting the ldap2 backend so I'm sticking with admin_conn, which
is working.

> +            add_lightweight_ca_tracking_requests(self.log, lwcas)
> +        except errors.NotFound:
> +            pass  # shouldn't happen, but don't fail if it does
> I would add at least some debug message here.
> 
OK.

> +        finally:
> +            if not is_already_connected:
> +                conn.disconnect()
> +
> 
> 
> -- 
> Martin^3 Babinsky
From 71ca530fd14cfb33833bcba6533310ec101da1b8 Mon Sep 17 00:00:00 2001
From: Fraser Tweedale <ftwee...@redhat.com>
Date: Tue, 23 Aug 2016 16:14:30 +1000
Subject: [PATCH] Track lightweight CAs on replica installation

Add Certmonger tracking requests for lightweight CAs on replica
installation.  As part of this change, extract most of the
lightweight CA tracking code out of ipa-certupdate and into
cainstance.

Fixes: https://fedorahosted.org/freeipa/ticket/6019
---
 ipaclient/ipa_certupdate.py     | 53 +++++++--------------------------
 ipalib/constants.py             |  2 ++
 ipaserver/install/cainstance.py | 66 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+), 43 deletions(-)

diff --git a/ipaclient/ipa_certupdate.py b/ipaclient/ipa_certupdate.py
index 
e59047a2705eb8ccb98b5213c4c8771f55a29bc5..07eaeca38fdda92c20d127dd26b600b34ee8b61d
 100644
--- a/ipaclient/ipa_certupdate.py
+++ b/ipaclient/ipa_certupdate.py
@@ -29,10 +29,8 @@ from ipaplatform import services
 from ipaplatform.paths import paths
 from ipaplatform.tasks import tasks
 from ipalib import api, errors, x509, certstore
-from ipalib.constants import IPA_CA_CN
+from ipalib.constants import IPA_CA_NICKNAME, RENEWAL_CA_NAME
 
-IPA_CA_NICKNAME = 'caSigningCert cert-pki-ca'
-RENEWAL_CA_NAME = 'dogtag-ipa-ca-renew-agent'
 
 class CertUpdate(admintool.AdminTool):
     command_name = 'ipa-certupdate'
@@ -85,11 +83,8 @@ class CertUpdate(admintool.AdminTool):
             certs = certstore.get_ca_certs(ldap, api.env.basedn,
                                            api.env.realm, ca_enabled)
 
-            # find lightweight CAs (on renewal master only)
-            lwcas = []
-            for ca_obj in api.Command.ca_find()['result']:
-                if IPA_CA_CN not in ca_obj['cn']:
-                    lwcas.append(ca_obj)
+            # find lightweight CAs
+            lwcas = api.Command.ca_find()['result']
 
             api.Backend.rpcclient.disconnect()
         finally:
@@ -98,8 +93,13 @@ class CertUpdate(admintool.AdminTool):
         server_fstore = sysrestore.FileStore(paths.SYSRESTORE)
         if server_fstore.has_files():
             self.update_server(certs)
-            for entry in lwcas:
-                self.server_track_lightweight_ca(entry)
+            try:
+                from ipaserver.install import cainstance
+                cainstance.add_lightweight_ca_tracking_requests(
+                    self.log, lwcas)
+            except Exception as e:
+                self.log.exception(
+                    "Failed to add lightweight CA tracking requests")
 
         self.update_client(certs)
 
@@ -163,39 +163,6 @@ class CertUpdate(admintool.AdminTool):
 
         self.update_file(paths.CA_CRT, certs)
 
-    def server_track_lightweight_ca(self, entry):
-        nickname = "{} {}".format(IPA_CA_NICKNAME, entry['ipacaid'][0])
-        criteria = {
-            'cert-database': paths.PKI_TOMCAT_ALIAS_DIR,
-            'cert-nickname': nickname,
-            'ca-name': RENEWAL_CA_NAME,
-        }
-        request_id = certmonger.get_request_id(criteria)
-        if request_id is None:
-            try:
-                certmonger.dogtag_start_tracking(
-                    secdir=paths.PKI_TOMCAT_ALIAS_DIR,
-                    pin=certmonger.get_pin('internal'),
-                    pinfile=None,
-                    nickname=nickname,
-                    ca=RENEWAL_CA_NAME,
-                    pre_command='stop_pkicad',
-                    post_command='renew_ca_cert "%s"' % nickname,
-                )
-                request_id = certmonger.get_request_id(criteria)
-                certmonger.modify(request_id, profile='ipaCACertRenewal')
-                self.log.debug(
-                    'Lightweight CA renewal: '
-                    'added tracking request for "%s"', nickname)
-            except RuntimeError as e:
-                self.log.error(
-                    'Lightweight CA renewal: Certmonger failed to '
-                    'start tracking certificate: %s', e)
-        else:
-            self.log.debug(
-                'Lightweight CA renewal: '
-                'already tracking certificate "%s"', nickname)
-
     def update_file(self, filename, certs, mode=0o444):
         certs = (c[0] for c in certs if c[2] is not False)
         try:
diff --git a/ipalib/constants.py b/ipalib/constants.py
index 
9b351e260f15211330521453b3ffcd41433a04bb..04515dcd25d066d8f1ab79ae8e8b96e909a1d884
 100644
--- a/ipalib/constants.py
+++ b/ipalib/constants.py
@@ -274,3 +274,5 @@ CA_SUFFIX_NAME = 'ca'
 PKI_GSSAPI_SERVICE_NAME = 'dogtag'
 IPA_CA_CN = u'ipa'
 IPA_CA_RECORD = "ipa-ca"
+IPA_CA_NICKNAME = 'caSigningCert cert-pki-ca'
+RENEWAL_CA_NAME = 'dogtag-ipa-ca-renew-agent'
diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index 
c4b8e9ae326fb7ebda9e927cd4d0b5bad9743db4..ab006be8ffc12fe30994b66e9d95fa78443253f8
 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -1383,6 +1383,9 @@ class CAInstance(DogtagInstance):
 
         self.step("enabling CA instance", self.__enable_instance)
 
+        self.step("configuring certmonger renewal for lightweight CAs",
+                  self.__add_lightweight_ca_tracking_requests)
+
         self.start_creation(runtime=210)
 
     def setup_lightweight_ca_key_retrieval(self):
@@ -1448,6 +1451,22 @@ class CAInstance(DogtagInstance):
         os.chmod(keyfile, 0o600)
         os.chown(keyfile, pent.pw_uid, pent.pw_gid)
 
+    def __add_lightweight_ca_tracking_requests(self):
+        if not self.admin_conn:
+            self.ldap_connect()
+
+        try:
+            lwcas = self.admin_conn.get_entries(
+                base_dn=api.env.basedn,
+                filter='(objectclass=ipaca)',
+                attrs_list=['cn', 'ipacaid'],
+            )
+            add_lightweight_ca_tracking_requests(self.log, lwcas)
+        except errors.NotFound:
+            # shouldn't happen, but don't fail if it does
+            root_logger.warning(
+                "Did not find any lightweight CAs; nothing to track")
+
 
 def replica_ca_install_check(config):
     if not config.setup_ca:
@@ -2070,6 +2089,53 @@ def ensure_default_caacl():
         api.Backend.ldap2.disconnect()
 
 
+def add_lightweight_ca_tracking_requests(logger, lwcas):
+    """Add tracking requests for the given lightweight CAs.
+
+    The entries must have the 'cn' and 'ipacaid' attributes.
+
+    The IPA CA, if present, is skipped.
+
+    """
+    for entry in lwcas:
+        if ipalib.constants.IPA_CA_CN in entry['cn']:
+            continue
+
+        nickname = "{} {}".format(
+                ipalib.constants.IPA_CA_NICKNAME,
+                entry['ipacaid'][0])
+        criteria = {
+            'cert-database': paths.PKI_TOMCAT_ALIAS_DIR,
+            'cert-nickname': nickname,
+            'ca-name': ipalib.constants.RENEWAL_CA_NAME,
+        }
+        request_id = certmonger.get_request_id(criteria)
+        if request_id is None:
+            try:
+                certmonger.dogtag_start_tracking(
+                    secdir=paths.PKI_TOMCAT_ALIAS_DIR,
+                    pin=certmonger.get_pin('internal'),
+                    pinfile=None,
+                    nickname=nickname,
+                    ca=ipalib.constants.RENEWAL_CA_NAME,
+                    pre_command='stop_pkicad',
+                    post_command='renew_ca_cert "%s"' % nickname,
+                )
+                request_id = certmonger.get_request_id(criteria)
+                certmonger.modify(request_id, profile='ipaCACertRenewal')
+                logger.debug(
+                    'Lightweight CA renewal: '
+                    'added tracking request for "%s"', nickname)
+            except RuntimeError as e:
+                logger.error(
+                    'Lightweight CA renewal: Certmonger failed to '
+                    'start tracking certificate: %s', e)
+        else:
+            logger.debug(
+                'Lightweight CA renewal: '
+                'already tracking certificate "%s"', nickname)
+
+
 def update_ipa_conf():
     """
     Update IPA configuration file to ensure that RA plugins are enabled and
-- 
2.5.5

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to