On 1.8.2016 10:19, Jan Cholasta wrote:
Hi,

the attached patches fix <https://fedorahosted.org/freeipa/ticket/6098>
and <https://fedorahosted.org/freeipa/ticket/6150>.

Self-NACK, proper patches attached.

Honza

--
Jan Cholasta
From 70ef1075bc361d836c1e836114a5367c3c5879d6 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Mon, 1 Aug 2016 09:53:39 +0200
Subject: [PATCH] cert: speed up cert-find

Use issuer+serial rather than raw DER blob to identify certificates in
cert-find's intermediate result.

Restructure the code to make it (hopefully) easier to follow.

https://fedorahosted.org/freeipa/ticket/6098
---
 ipaserver/plugins/cert.py | 394 +++++++++++++++++++++++++---------------------
 1 file changed, 215 insertions(+), 179 deletions(-)

diff --git a/ipaserver/plugins/cert.py b/ipaserver/plugins/cert.py
index 06041d3..1367448 100644
--- a/ipaserver/plugins/cert.py
+++ b/ipaserver/plugins/cert.py
@@ -21,6 +21,7 @@
 
 import base64
 import binascii
+import collections
 import datetime
 import os
 
@@ -295,18 +296,24 @@ class BaseCertObject(Object):
         ),
     )
 
-    def _parse(self, obj):
-        cert = x509.load_certificate(obj['certificate'])
-        obj['subject'] = DN(unicode(cert.subject))
-        obj['issuer'] = DN(unicode(cert.issuer))
-        obj['valid_not_before'] = unicode(cert.valid_not_before_str)
-        obj['valid_not_after'] = unicode(cert.valid_not_after_str)
-        obj['md5_fingerprint'] = unicode(
-            nss.data_to_hex(nss.md5_digest(cert.der_data), 64)[0])
-        obj['sha1_fingerprint'] = unicode(
-            nss.data_to_hex(nss.sha1_digest(cert.der_data), 64)[0])
-        obj['serial_number'] = cert.serial_number
-        obj['serial_number_hex'] = u'0x%X' % cert.serial_number
+    def _parse(self, obj, full=True):
+        cert = obj.get('certificate')
+        if cert is not None:
+            cert = x509.load_certificate(cert)
+            obj['subject'] = DN(unicode(cert.subject))
+            obj['issuer'] = DN(unicode(cert.issuer))
+            obj['serial_number'] = cert.serial_number
+            if full:
+                obj['valid_not_before'] = unicode(cert.valid_not_before_str)
+                obj['valid_not_after'] = unicode(cert.valid_not_after_str)
+                obj['md5_fingerprint'] = unicode(
+                    nss.data_to_hex(nss.md5_digest(cert.der_data), 64)[0])
+                obj['sha1_fingerprint'] = unicode(
+                    nss.data_to_hex(nss.sha1_digest(cert.der_data), 64)[0])
+
+        serial_number = obj.get('serial_number')
+        if serial_number is not None:
+            obj['serial_number_hex'] = u'0x%X' % serial_number
 
 
 class BaseCertMethod(Method):
@@ -691,10 +698,14 @@ class cert(BaseCertObject):
             yield self.api.Object[name]
 
     def _fill_owners(self, obj):
+        dns = obj.pop('owner', None)
+        if dns is None:
+            return
+
         for owner in self._owners():
             container_dn = DN(owner.container_dn, self.api.env.basedn)
             name = 'owner_' + owner.name
-            for dn in obj['owner']:
+            for dn in dns:
                 if dn.endswith(container_dn, 1):
                     value = owner.get_primary_key_from_dn(dn)
                     obj.setdefault(name, []).append(value)
@@ -984,36 +995,171 @@ class cert_find(Search, CertMethod):
                 label=owner.object_name,
             )
 
-    def execute(self, criteria=None, all=False, raw=False, pkey_only=False,
-                no_members=True, timelimit=None, sizelimit=None, **options):
-        ca_options = {'cacn',
-                      'revocation_reason',
-                      'issuer',
-                      'subject',
-                      'min_serial_number', 'max_serial_number',
-                      'exactly',
-                      'validnotafter_from', 'validnotafter_to',
-                      'validnotbefore_from', 'validnotbefore_to',
-                      'issuedon_from', 'issuedon_to',
-                      'revokedon_from', 'revokedon_to'}
-        ldap_options = {prefix + owner.name
-                        for owner in self.obj._owners()
-                        for prefix in ('', 'no_')}
-        has_ca_options = (
-            any(name in options for name in ca_options - {'exactly'}) or
-            options['exactly'])
-        has_ldap_options = any(name in options for name in ldap_options)
-        has_cert_option = 'certificate' in options
+    def _get_cert_key(self, cert):
+        nss_cert = x509.load_certificate(cert, x509.DER)
+
+        return (DN(unicode(nss_cert.issuer)), nss_cert.serial_number)
+
+    def _get_cert_obj(self, cert, all, raw, pkey_only):
+        obj = {'certificate': unicode(base64.b64encode(cert))}
+
+        full = not pkey_only and all
+        if not raw:
+            self.obj._parse(obj, full)
+        if not full:
+            del obj['certificate']
+
+        return obj
+
+    def _cert_search(self, all, raw, pkey_only, **options):
+        result = collections.OrderedDict()
+
+        try:
+            cert = options['certificate']
+        except KeyError:
+            return result, False, False
+
+        key = self._get_cert_key(cert)
+
+        result[key] = self._get_cert_obj(cert, all, raw, pkey_only)
+
+        return result, False, True
+
+    def _ca_search(self, all, raw, pkey_only, sizelimit, exactly, **options):
+        ra_options = {}
+        for name in ('revocation_reason',
+                     'issuer',
+                     'subject',
+                     'min_serial_number', 'max_serial_number',
+                     'validnotafter_from', 'validnotafter_to',
+                     'validnotbefore_from', 'validnotbefore_to',
+                     'issuedon_from', 'issuedon_to',
+                     'revokedon_from', 'revokedon_to'):
+            try:
+                value = options[name]
+            except KeyError:
+                continue
+            if isinstance(value, datetime.datetime):
+                value = value.strftime(PKIDATE_FORMAT)
+            elif isinstance(value, DN):
+                value = unicode(value)
+            ra_options[name] = value
+        if sizelimit:
+            ra_options['sizelimit'] = sizelimit
+        if exactly:
+            ra_options['exactly'] = True
+
+        result = collections.OrderedDict()
+        complete = bool(ra_options)
 
         try:
             ca_enabled_check()
         except errors.NotFound:
-            if has_ca_options:
+            if ra_options:
                 raise
-            ca_enabled = False
+            return result, False, complete
+
+        ra = self.api.Backend.ra
+        for ra_obj in ra.find(ra_options):
+            issuer = DN(ra_obj['issuer'])
+            serial_number = ra_obj['serial_number']
+
+            if pkey_only:
+                obj = {'serial_number': serial_number}
+            else:
+                obj = ra_obj
+                obj['issuer'] = issuer
+                obj['subject'] = DN(ra_obj['subject'])
+                del obj['serial_number_hex']
+
+                if all:
+                    ra_obj = ra.get_certificate(str(serial_number))
+                    obj['certificate'] = (
+                        ra_obj['certificate'].replace('\r\n', ''))
+                    if not raw:
+                        self.obj._parse(obj)
+
+            result[issuer, serial_number] = obj
+
+        return result, False, complete
+
+    def _ldap_search(self, all, raw, pkey_only, no_members, timelimit,
+                     sizelimit, **options):
+        ldap = self.api.Backend.ldap2
+
+        filters = []
+        for owner in self.obj._owners():
+            for prefix, rule in (('', ldap.MATCH_ALL),
+                                 ('no_', ldap.MATCH_NONE)):
+                try:
+                    value = options[prefix + owner.name]
+                except KeyError:
+                    continue
+
+                filter = ldap.make_filter_from_attr(
+                    'objectclass',
+                    owner.object_class,
+                    ldap.MATCH_ALL)
+                if filter not in filters:
+                    filters.append(filter)
+
+                filter = ldap.make_filter_from_attr(
+                    owner.primary_key.name,
+                    value,
+                    rule)
+                filters.append(filter)
+
+        cert = options.get('certificate')
+        if cert is not None:
+            filter = ldap.make_filter_from_attr('usercertificate', value)
+            filters.append(filter)
+
+        result = collections.OrderedDict()
+        complete = bool(filters)
+
+        if cert is None:
+            filter = '(usercertificate=*)'
+            filters.append(filter)
+
+        filter = ldap.combine_filters(filters, ldap.MATCH_ALL)
+        try:
+            entries, truncated = ldap.find_entries(
+                base_dn=self.api.env.basedn,
+                filter=filter,
+                attrs_list=['usercertificate'],
+                time_limit=timelimit,
+                size_limit=sizelimit,
+            )
+        except errors.EmptyResult:
+            entries = []
+            truncated = False
         else:
-            ca_enabled = True
+            truncated = bool(truncated)
+
+        for entry in entries:
+            for attr in ('usercertificate', 'usercertificate;binary'):
+                for cert in entry.get(attr, []):
+                    key = self._get_cert_key(cert)
+
+                    try:
+                        obj = result[key]
+                    except KeyError:
+                        obj = self._get_cert_obj(cert, all, raw, pkey_only)
+                        result[key] = obj
 
+                    if not pkey_only and (all or not no_members):
+                        owners = obj.setdefault('owner', [])
+                        if entry.dn not in owners:
+                            owners.append(entry.dn)
+
+        if not raw:
+            for obj in six.itervalues(result):
+                self.obj._fill_owners(obj)
+
+        return result, truncated, complete
+
+    def execute(self, criteria=None, all=False, raw=False, pkey_only=False,
+                no_members=True, timelimit=None, sizelimit=None, **options):
         if 'cacn' in options:
             ca_obj = api.Command.ca_show(options['cacn'])['result']
             ca_sdn = unicode(ca_obj['ipacasubjectdn'][0])
@@ -1028,153 +1174,43 @@ class cert_find(Search, CertMethod):
         if criteria is not None:
             return dict(result=[], count=0, truncated=False)
 
-        obj_seq = []
-        obj_dict = {}
+        result = collections.OrderedDict()
         truncated = False
-
-        if has_cert_option:
-            cert = options['certificate']
-            obj = {'certificate': unicode(base64.b64encode(cert))}
-            obj_seq.append(obj)
-            obj_dict[cert] = obj
-
-        if ca_enabled:
-            ra_options = {}
-            for name, value in options.items():
-                if name not in ca_options:
-                    continue
-                if isinstance(value, datetime.datetime):
-                    value = value.strftime(PKIDATE_FORMAT)
-                elif isinstance(value, DN):
-                    value = unicode(value)
-                ra_options[name] = value
-            if sizelimit is not None:
-                if sizelimit != 0:
-                    ra_options['sizelimit'] = sizelimit
-                sizelimit = 0
-                has_ca_options = True
-
-            for ra_obj in self.Backend.ra.find(ra_options):
-                obj = {}
-                if ((not pkey_only and all) or
-                        not no_members or
-                        not has_ca_options or
-                        has_ldap_options or
-                        has_cert_option):
-                    ra_obj.update(
-                        self.Backend.ra.get_certificate(
-                            str(ra_obj['serial_number'])))
-                    cert = base64.b64decode(ra_obj['certificate'])
-                    try:
-                        obj = obj_dict[cert]
-                    except KeyError:
-                        if has_cert_option:
-                            continue
-                        obj = {}
-                        obj_seq.append(obj)
-                        obj_dict[cert] = obj
+        complete = False
+
+        for sub_search in (self._cert_search,
+                           self._ca_search,
+                           self._ldap_search):
+            sub_result, sub_truncated, sub_complete = sub_search(
+                all=all,
+                raw=raw,
+                pkey_only=pkey_only,
+                no_members=no_members,
+                timelimit=timelimit,
+                sizelimit=sizelimit,
+                **options)
+
+            if sub_complete:
+                sizelimit = None
+
+                for key in tuple(six.iterkeys(result)):
+                    if key not in sub_result:
+                        del result[key]
+
+            for key, sub_obj in six.iteritems(sub_result):
+                try:
+                    obj = result[key]
+                except KeyError:
+                    if complete:
+                        continue
+                    result[key] = sub_obj
                 else:
-                    obj_seq.append(obj)
-                obj.update(ra_obj)
-
-        if ((not pkey_only and all) or
-                not no_members or
-                not has_ca_options or
-                has_ldap_options or
-                has_cert_option):
-            ldap = self.api.Backend.ldap2
+                    obj.update(sub_obj)
 
-            filters = []
-            if 'certificate' in options:
-                cert_filter = ldap.make_filter_from_attr(
-                    'usercertificate', options['certificate'])
-            else:
-                cert_filter = '(usercertificate=*)'
-            filters.append(cert_filter)
-            for owner in self.obj._owners():
-                oc_filter = ldap.make_filter_from_attr(
-                    'objectclass', owner.object_class, ldap.MATCH_ALL)
-                for prefix, rule in (('', ldap.MATCH_ALL),
-                                     ('no_', ldap.MATCH_NONE)):
-                    value = options.get(prefix + owner.name)
-                    if value is None:
-                        continue
-                    pkey_filter = ldap.make_filter_from_attr(
-                        owner.primary_key.name, value, rule)
-                    filters.append(oc_filter)
-                    filters.append(pkey_filter)
-            filter = ldap.combine_filters(filters, ldap.MATCH_ALL)
+            truncated = truncated or sub_truncated
+            complete = complete or sub_complete
 
-            try:
-                entries, truncated = ldap.find_entries(
-                    base_dn=self.api.env.basedn,
-                    filter=filter,
-                    attrs_list=['usercertificate'],
-                    time_limit=timelimit,
-                    size_limit=sizelimit,
-                )
-            except errors.EmptyResult:
-                entries, truncated = [], False
-            for entry in entries:
-                seen = set()
-                for attr in ('usercertificate', 'usercertificate;binary'):
-                    for cert in entry.get(attr, []):
-                        if cert in seen:
-                            continue
-                        seen.add(cert)
-                        try:
-                            obj = obj_dict[cert]
-                        except KeyError:
-                            if has_ca_options or has_cert_option:
-                                continue
-                            obj = {
-                                'certificate': unicode(base64.b64encode(cert))}
-                            obj_seq.append(obj)
-                            obj_dict[cert] = obj
-                        obj.setdefault('owner', []).append(entry.dn)
-
-        result = []
-        for obj in obj_seq:
-            if has_ldap_options and 'owner' not in obj:
-                continue
-            if not pkey_only:
-                if not raw:
-                    if 'certificate' in obj:
-                        obj['certificate'] = (
-                            obj['certificate'].replace('\r\n', ''))
-                        self.obj._parse(obj)
-                        if not all:
-                            del obj['certificate']
-                            del obj['valid_not_before']
-                            del obj['valid_not_after']
-                            del obj['md5_fingerprint']
-                            del obj['sha1_fingerprint']
-                    if 'subject' in obj:
-                        obj['subject'] = DN(obj['subject'])
-                    if 'issuer' in obj:
-                        obj['issuer'] = DN(obj['issuer'])
-                    if 'status' in obj:
-                        obj['revoked'] = (
-                            obj['status'] in (u'REVOKED', u'REVOKED_EXPIRED'))
-                    if 'owner' in obj:
-                        if all or not no_members:
-                            self.obj._fill_owners(obj)
-                        del obj['owner']
-                else:
-                    if 'certificate' in obj:
-                        if not all:
-                            del obj['certificate']
-                    if 'owner' in obj:
-                        if not all and no_members:
-                            del obj['owner']
-            else:
-                if 'serial_number' in obj:
-                    serial_number = obj['serial_number']
-                    obj.clear()
-                    obj['serial_number'] = serial_number
-                else:
-                    obj.clear()
-            result.append(obj)
+        result = list(six.itervalues(result))
 
         ret = dict(
             result=result
-- 
2.7.4

From 99a3c0e7308a5b522a6ec315b44f088dd176a799 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Mon, 1 Aug 2016 09:55:58 +0200
Subject: [PATCH] cert: do not crash on invalid data in cert-find

https://fedorahosted.org/freeipa/ticket/6150
---
 ipaserver/plugins/cert.py | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/ipaserver/plugins/cert.py b/ipaserver/plugins/cert.py
index 1367448..eba39a0 100644
--- a/ipaserver/plugins/cert.py
+++ b/ipaserver/plugins/cert.py
@@ -32,7 +32,7 @@ import six
 
 from ipalib import Command, Str, Int, Flag
 from ipalib import api
-from ipalib import errors
+from ipalib import errors, messages
 from ipalib import pkcs10
 from ipalib import x509
 from ipalib import ngettext
@@ -996,7 +996,15 @@ class cert_find(Search, CertMethod):
             )
 
     def _get_cert_key(self, cert):
-        nss_cert = x509.load_certificate(cert, x509.DER)
+        try:
+            nss_cert = x509.load_certificate(cert, x509.DER)
+        except NSPRError as e:
+            message = messages.SearchResultTruncated(
+                reason=_("failed to load certificate: %s") % e,
+            )
+            self.add_message(message)
+
+            raise ValueError("failed to load certificate")
 
         return (DN(unicode(nss_cert.issuer)), nss_cert.serial_number)
 
@@ -1019,7 +1027,10 @@ class cert_find(Search, CertMethod):
         except KeyError:
             return result, False, False
 
-        key = self._get_cert_key(cert)
+        try:
+            key = self._get_cert_key(cert)
+        except ValueError:
+            return result, True, True
 
         result[key] = self._get_cert_obj(cert, all, raw, pkey_only)
 
@@ -1134,12 +1145,21 @@ class cert_find(Search, CertMethod):
             entries = []
             truncated = False
         else:
+            try:
+                ldap.handle_truncated_result(truncated)
+            except errors.LimitsExceeded as e:
+                self.add_message(messages.SearchResultTruncated(reason=e))
+
             truncated = bool(truncated)
 
         for entry in entries:
             for attr in ('usercertificate', 'usercertificate;binary'):
                 for cert in entry.get(attr, []):
-                    key = self._get_cert_key(cert)
+                    try:
+                        key = self._get_cert_key(cert)
+                    except ValueError:
+                        truncated = True
+                        continue
 
                     try:
                         obj = result[key]
-- 
2.7.4

-- 
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