From 9fe30f21c987bdccf80ef5f6d645fdc59b393bdb Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: Jun 16 2023 19:09:52 +0000 Subject: Revert "Use the OpenSSL certificate parser in cert-find" This reverts commit 191880bc9f77c3e8a3cecc82e6eea33ab5ad03e4. The problem isn't with python-cryptography, it is with the IPACertificate class which does way more work on a certificate than is necessary in cert-find. Related: https://pagure.io/freeipa/issue/9331 Reviewed-By: Florence Blanc-Renaud --- diff --git a/freeipa.spec.in b/freeipa.spec.in index f3380b4..2b18963 100755 --- a/freeipa.spec.in +++ b/freeipa.spec.in @@ -390,7 +390,6 @@ BuildRequires: python3-pylint BuildRequires: python3-pytest-multihost BuildRequires: python3-pytest-sourceorder BuildRequires: python3-qrcode-core >= 5.0.0 -BuildRequires: python3-pyOpenSSL BuildRequires: python3-samba BuildRequires: python3-six BuildRequires: python3-sss @@ -862,7 +861,6 @@ Requires: python3-netifaces >= 0.10.4 Requires: python3-pyasn1 >= 0.3.2-2 Requires: python3-pyasn1-modules >= 0.3.2-2 Requires: python3-pyusb -Requires: python3-pyOpenSSL Requires: python3-qrcode-core >= 5.0.0 Requires: python3-requests Requires: python3-six diff --git a/ipaserver/plugins/cert.py b/ipaserver/plugins/cert.py index cec3d93..88c6b62 100644 --- a/ipaserver/plugins/cert.py +++ b/ipaserver/plugins/cert.py @@ -30,7 +30,6 @@ import cryptography.x509 from cryptography.hazmat.primitives import hashes, serialization from dns import resolver, reversename import six -import sys from ipalib import Command, Str, Int, Flag, StrEnum from ipalib import api @@ -1623,19 +1622,7 @@ class cert_find(Search, CertMethod): ) def _get_cert_key(self, cert): - # for cert-find with a certificate value - if isinstance(cert, x509.IPACertificate): - return (DN(cert.issuer), cert.serial_number) - - issuer = [] - for oid, value in cert.get_issuer().get_components(): - issuer.append( - '{}={}'.format(oid.decode('utf-8'), value.decode('utf-8')) - ) - issuer = ','.join(issuer) - # Use this to flip from OpenSSL reverse to X500 ordering - issuer = DN(issuer).x500_text() - return (DN(issuer), cert.get_serial_number()) + return (DN(cert.issuer), cert.serial_number) def _cert_search(self, pkey_only, **options): result = collections.OrderedDict() @@ -1755,11 +1742,6 @@ class cert_find(Search, CertMethod): return result, False, complete def _ldap_search(self, all, pkey_only, no_members, **options): - # defer import of the OpenSSL module to not affect the requests - # module which will use pyopenssl if this is available. - if sys.modules.get('OpenSSL.SSL', False) is None: - del sys.modules["OpenSSL.SSL"] - import OpenSSL.crypto ldap = self.api.Backend.ldap2 filters = [] @@ -1818,14 +1800,12 @@ class cert_find(Search, CertMethod): ca_enabled = getattr(context, 'ca_enabled') for entry in entries: for attr in ('usercertificate', 'usercertificate;binary'): - for der in entry.raw.get(attr, []): - cert = OpenSSL.crypto.load_certificate( - OpenSSL.crypto.FILETYPE_ASN1, der) + for cert in entry.get(attr, []): cert_key = self._get_cert_key(cert) try: obj = result[cert_key] except KeyError: - obj = {'serial_number': cert.get_serial_number()} + obj = {'serial_number': cert.serial_number} if not pkey_only and (all or not ca_enabled): # Retrieving certificate details is now deferred # until after all certificates are collected. From 3b1dbcdba2994bf57908f530913998e9ab888e4c Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: Jun 16 2023 19:09:52 +0000 Subject: Revert "cert_find: fix call with --all" This reverts commit 1f30cc65276a532e7288217f216b72a2b0628c8f. The problem isn't with python-cryptography, it is with the IPACertificate class which does way more work on a certificate than is necessary in cert-find. Related: https://pagure.io/freeipa/issue/9331 Reviewed-By: Florence Blanc-Renaud --- diff --git a/ipaserver/plugins/cert.py b/ipaserver/plugins/cert.py index 88c6b62..ba37525 100644 --- a/ipaserver/plugins/cert.py +++ b/ipaserver/plugins/cert.py @@ -1812,7 +1812,6 @@ class cert_find(Search, CertMethod): # For the case of CA-less we need to keep # the certificate because getting it again later # would require unnecessary LDAP searches. - cert = cert.to_cryptography() obj['certificate'] = ( base64.b64encode( cert.public_bytes(x509.Encoding.DER)) From d00fd3398c32beb2c3e72f4878c87f9d2c0e833d Mon Sep 17 00:00:00 2001 From: Rob Crittenden Date: Jun 16 2023 19:09:52 +0000 Subject: Use the python-cryptography parser directly in cert-find cert-find is a rather complex beast because it not only looks for certificates in the optional CA but within the IPA LDAP database as well. It has a process to deduplicate the certificates since any PKI issued certificates will also be associated with an IPA record. In order to obtain the data to deduplicate the certificates the cert from LDAP must be parser for issuer and serial number. ipaldap has automation to determine the datatype of an attribute and will use the ipalib.x509 IPACertificate class to decode a certificate automatically if you access entry['usercertificate']. The downside is that this is comparatively slow. Here is the parse time in microseconds: cryptography 0.0081 OpenSSL.crypto 0.2271 ipalib.x509 2.6814 Since only issuer and subject are required there is no need to make the expensive IPACertificate call. The IPACertificate parsing time is fine if you're parsing one certificate but if the LDAP search returns a lot of certificates, say in the thousands, then those microseconds add up quickly. In testing it took ~17 seconds to parse 5k certificates (excluding transmission overhead, etc). cert-find when there are a lot of certificates has been historically slow. It isn't related to the CA which returns large sets (well, 5k anyway) in a second or two. It was the LDAP comparision adding tens of seconds to the runtime. When searching with the default sizelimit of 100 the time is ~10s without this patch. With it the time is 1.5s. CLI times from before and after searching for all certs: original: ------------------------------- Number of entries returned 5038 ------------------------------- real 0m15.507s user 0m0.828s sys 0m0.241s using cryptography: real 0m4.037s user 0m0.816s sys 0m0.193s Fixes: https://pagure.io/freeipa/issue/9331 Signed-off-by: Rob Crittenden Reviewed-By: Florence Blanc-Renaud --- diff --git a/ipaserver/plugins/cert.py b/ipaserver/plugins/cert.py index ba37525..619be83 100644 --- a/ipaserver/plugins/cert.py +++ b/ipaserver/plugins/cert.py @@ -1800,7 +1800,8 @@ class cert_find(Search, CertMethod): ca_enabled = getattr(context, 'ca_enabled') for entry in entries: for attr in ('usercertificate', 'usercertificate;binary'): - for cert in entry.get(attr, []): + for der in entry.raw.get(attr, []): + cert = cryptography.x509.load_der_x509_certificate(der) cert_key = self._get_cert_key(cert) try: obj = result[cert_key] diff --git a/ipatests/test_xmlrpc/test_cert_plugin.py b/ipatests/test_xmlrpc/test_cert_plugin.py index 433cebc..583c67f 100644 --- a/ipatests/test_xmlrpc/test_cert_plugin.py +++ b/ipatests/test_xmlrpc/test_cert_plugin.py @@ -254,6 +254,16 @@ class test_cert(BaseCert): result = _emails_are_valid(email_addrs, []) assert not result + def test_00012_cert_find_all(self): + """ + Test that cert-find --all returns successfully. + + We don't know how many we'll get but there should be at least 10 + by default. + """ + res = api.Command['cert_find'](all=True) + assert 'count' in res and res['count'] >= 10 + def test_99999_cleanup(self): """ Clean up cert test data @@ -283,7 +293,7 @@ class test_cert_find(XMLRPC_test): short = api.env.host.split('.', maxsplit=1)[0] - def test_0001_find_all(self): + def test_0001_find_all_certs(self): """ Search for all certificates.