From 1190a1b41d436de4dab7a622d78217baba44a9ef Mon Sep 17 00:00:00 2001

From: Christian Heimes <cheimes@redhat.com>

Date: Fri, 17 Mar 2017 10:44:38 +0100

Subject: [PATCH] Simplify KRA transport cert cache

In-memory cache causes problem in forking servers. A file based cache is

good enough. It's easier to understand and avoids performance regression

and synchronization issues when cert becomes out-of-date.

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

Signed-off-by: Christian Heimes <cheimes@redhat.com>

Reviewed-By: Jan Cholasta <jcholast@redhat.com>

---

 ipaclient/plugins/vault.py | 103 ++++++++++++++++++++++++---------------------

 1 file changed, 55 insertions(+), 48 deletions(-)

diff --git a/ipaclient/plugins/vault.py b/ipaclient/plugins/vault.py

index d677ec0287d6b37cfd63820a919c0726d3a4ae9f..3fb4900d9cf90e6902c40e1c3d8cfdafec2e28b8 100644

--- a/ipaclient/plugins/vault.py

+++ b/ipaclient/plugins/vault.py

@@ -20,7 +20,6 @@

 from __future__ import print_function

 import base64

-import collections

 import errno

 import getpass

 import io

@@ -558,74 +557,79 @@ class vault_mod(Local):

         return response

-class _TransportCertCache(collections.MutableMapping):

+class _TransportCertCache(object):

     def __init__(self):

         self._dirname = os.path.join(

-                USER_CACHE_PATH, 'ipa', 'kra-transport-certs')

-        self._transport_certs = {}

+                USER_CACHE_PATH, 'ipa', 'kra-transport-certs'

+        )

     def _get_filename(self, domain):

         basename = DNSName(domain).ToASCII() + '.pem'

         return os.path.join(self._dirname, basename)

-    def __getitem__(self, domain):

-        try:

-            transport_cert = self._transport_certs[domain]

-        except KeyError:

-            transport_cert = None

+    def load_cert(self, domain):

+        """Load cert from cache

-            filename = self._get_filename(domain)

+        :param domain: IPA domain

+        :return: cryptography.x509.Certificate or None

+        """

+        filename = self._get_filename(domain)

+        try:

             try:

-                try:

-                    transport_cert = x509.load_certificate_from_file(filename)

-                except EnvironmentError as e:

-                    if e.errno != errno.ENOENT:

-                        raise

-            except Exception:

-                logger.warning("Failed to load %s: %s", filename,

-                               exc_info=True)

-

-            if transport_cert is None:

-                raise KeyError(domain)

-

-            self._transport_certs[domain] = transport_cert

+                return x509.load_certificate_from_file(filename)

+            except EnvironmentError as e:

+                if e.errno != errno.ENOENT:

+                    raise

+        except Exception:

+            logger.warning("Failed to load %s", filename, exc_info=True)

-        return transport_cert

+    def store_cert(self, domain, transport_cert):

+        """Store a new cert or override existing cert

-    def __setitem__(self, domain, transport_cert):

+        :param domain: IPA domain

+        :param transport_cert: cryptography.x509.Certificate

+        :return: True if cert was stored successfully

+        """

         filename = self._get_filename(domain)

-        transport_cert_der = (

-            transport_cert.public_bytes(serialization.Encoding.DER))

+        pem = transport_cert.public_bytes(serialization.Encoding.PEM)

         try:

             try:

                 os.makedirs(self._dirname)

             except EnvironmentError as e:

                 if e.errno != errno.EEXIST:

                     raise

-            fd, tmpfilename = tempfile.mkstemp(dir=self._dirname)

-            os.close(fd)

-            x509.write_certificate(transport_cert_der, tmpfilename)

-            os.rename(tmpfilename, filename)

+            with tempfile.NamedTemporaryFile(dir=self._dirname, delete=False,

+                                             mode='wb') as f:

+                try:

+                    f.write(pem)

+                    f.flush()

+                    os.fdatasync(f.fileno())

+                    f.close()

+                    os.rename(f.name, filename)

+                except Exception:

+                    os.unlink(f.name)

+                    raise

         except Exception:

             logger.warning("Failed to save %s", filename, exc_info=True)

+            return False

+        else:

+            return True

-        self._transport_certs[domain] = transport_cert

+    def remove_cert(self, domain):

+        """Remove a cert from cache, ignores errors

-    def __delitem__(self, domain):

+        :param domain: IPA domain

+        :return: True if cert was found and removed

+        """

         filename = self._get_filename(domain)

         try:

             os.unlink(filename)

         except EnvironmentError as e:

             if e.errno != errno.ENOENT:

                 logger.warning("Failed to remove %s", filename, exc_info=True)

-

-        del self._transport_certs[domain]

-

-    def __len__(self):

-        return len(self._transport_certs)

-

-    def __iter__(self):

-        return iter(self._transport_certs)

+            return False

+        else:

+            return True

 _transport_cert_cache = _TransportCertCache()

@@ -646,7 +650,10 @@ class vaultconfig_show(MethodOverride):

         # cache transport certificate

         transport_cert = x509.load_certificate(

                 response['result']['transport_cert'], x509.DER)

-        _transport_cert_cache[self.api.env.domain] = transport_cert

+

+        _transport_cert_cache.store_cert(

+            self.api.env.domain, transport_cert

+        )

         if file:

             with open(file, 'w') as f:

@@ -680,7 +687,7 @@ class ModVaultData(Local):

         except (errors.InternalError,

                 errors.ExecutionError,

                 errors.GenericError):

-            _transport_cert_cache.pop(self.api.env.domain, None)

+            _transport_cert_cache.remove_cert(self.api.env.domain)

             if raise_unexpected:

                 raise

@@ -691,17 +698,17 @@ class ModVaultData(Local):

         domain = self.api.env.domain

         # try call with cached transport certificate

-        transport_cert = _transport_cert_cache.get(domain)

+        transport_cert = _transport_cert_cache.load_cert(domain)

         if transport_cert is not None:

             result = self._do_internal(algo, transport_cert, False,

                                        *args, **options)

             if result is not None:

                 return result

-        # retrieve and cache transport certificate

-        self.api.Command.vaultconfig_show()

-        transport_cert = _transport_cert_cache[domain]

-

+        # retrieve transport certificate (cached by vaultconfig_show)

+        response = self.api.Command.vaultconfig_show()

+        transport_cert = x509.load_certificate(

+            response['result']['transport_cert'], x509.DER)

         # call with the retrieved transport certificate

         return self._do_internal(algo, transport_cert, True,

                                  *args, **options)

-- 

2.12.1