From be1e3bbfc13aff9a583108376f245b81cc3666fb Mon Sep 17 00:00:00 2001

From: Rob Crittenden <rcritten@redhat.com>

Date: Thu, 9 Sep 2021 15:26:55 -0400

Subject: [PATCH] Don't store entries with a usercertificate in the LDAP cache

usercertificate often has a subclass and both the plain and

subclassed (binary) values are queried. I'm concerned that

they are used more or less interchangably in places so not

caching these entries is the safest path forward for now until

we can dedicate the time to find all usages, determine their

safety and/or perhaps handle this gracefully within the cache

now.

What we see in this bug is that usercertificate;binary holds the

first certificate value but a user-mod is done with

setattr usercertificate=<new_cert>. Since there is no

usercertificate value (remember, it's usercertificate;binary)

a replace is done and 389-ds wipes the existing value as we've

asked it to.

I'm not comfortable with simply treating them the same because

in LDAP they are not.

https://pagure.io/freeipa/issue/8986

Signed-off-by: Rob Crittenden <rcritten@redhat.com>

Reviewed-By: Francois Cami <fcami@redhat.com>

Reviewed-By: Fraser Tweedale <ftweedal@redhat.com>

---

 ipapython/ipaldap.py | 14 +++++++++++---

 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py

index f94b784d6..ced8f1bd6 100644

--- a/ipapython/ipaldap.py

+++ b/ipapython/ipaldap.py

@@ -1821,9 +1821,17 @@ class LDAPCache(LDAPClient):

                         entry=None, exception=None):

         # idnsname - caching prevents delete when mod value to None

         # cospriority - in a Class of Service object, uncacheable

-        # TODO - usercertificate was banned at one point and I don't remember

-        #        why...

-        BANNED_ATTRS = {'idnsname', 'cospriority'}

+        # usercertificate* - caching subtypes is tricky, trade less

+        #                    complexity for performance

+        #

+        # TODO: teach the cache about subtypes

+

+        BANNED_ATTRS = {

+            'idnsname',

+            'cospriority',

+            'usercertificate',

+            'usercertificate;binary'

+        }

         if not self._enable_cache:

             return

-- 

2.31.1

From 86588640137562b2016fdb0f91142d00bc38e54a Mon Sep 17 00:00:00 2001

From: Rob Crittenden <rcritten@redhat.com>

Date: Fri, 10 Sep 2021 09:01:48 -0400

Subject: [PATCH] ipatests: Test that a user can be issued multiple

 certificates

Prevent regressions in the LDAP cache layer that caused newly

issued certificates to overwrite existing ones.

https://pagure.io/freeipa/issue/8986

Signed-off-by: Rob Crittenden <rcritten@redhat.com>

Reviewed-By: Francois Cami <fcami@redhat.com>

Reviewed-By: Fraser Tweedale <ftweedal@redhat.com>

---

 ipatests/test_integration/test_cert.py | 29 ++++++++++++++++++++++++++

 1 file changed, 29 insertions(+)

diff --git a/ipatests/test_integration/test_cert.py b/ipatests/test_integration/test_cert.py

index 7d51b76ee..b4e85eadc 100644

--- a/ipatests/test_integration/test_cert.py

+++ b/ipatests/test_integration/test_cert.py

@@ -16,6 +16,7 @@ import string

 import time

 from ipaplatform.paths import paths

+from ipapython.dn import DN

 from cryptography import x509

 from cryptography.x509.oid import ExtensionOID

 from cryptography.hazmat.backends import default_backend

@@ -183,6 +184,34 @@ class TestInstallMasterClient(IntegrationTest):

         )

         assert "profile: caServerCert" in result.stdout_text

+    def test_multiple_user_certificates(self):

+        """Test that a user may be issued multiple certificates"""

+        ldap = self.master.ldap_connect()

+

+        user = 'user1'

+

+        tasks.kinit_admin(self.master)

+        tasks.user_add(self.master, user)

+

+        for id in (0,1):

+            csr_file = f'{id}.csr'

+            key_file = f'{id}.key'

+            cert_file = f'{id}.crt'

+            openssl_cmd = [

+                'openssl', 'req', '-newkey', 'rsa:2048', '-keyout', key_file,

+                '-nodes', '-out', csr_file, '-subj', '/CN=' + user]

+            self.master.run_command(openssl_cmd)

+

+            cmd_args = ['ipa', 'cert-request', '--principal', user,

+                        '--certificate-out', cert_file, csr_file]

+            self.master.run_command(cmd_args)

+

+        # easier to count by pulling the LDAP entry

+        entry = ldap.get_entry(DN(('uid', user), ('cn', 'users'),

+                               ('cn', 'accounts'), self.master.domain.basedn))

+

+        assert len(entry.get('usercertificate')) == 2

+

     @pytest.fixture

     def test_subca_certs(self):

         """

-- 

2.31.1