URL: https://github.com/freeipa/freeipa/pull/616
Author: tiran
 Title: #616: Simplify KRA transport cert cache
Action: opened

PR body:
"""
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.

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

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/616/head:pr616
git checkout pr616
From 0f0a9457ba6f2e871dedf3a1cb9491c916693c39 Mon Sep 17 00:00:00 2001
From: Christian Heimes <chei...@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.

Signed-off-by: Christian Heimes <chei...@redhat.com>
---
 ipaclient/plugins/vault.py | 106 +++++++++++++++++++++++++--------------------
 1 file changed, 59 insertions(+), 47 deletions(-)

diff --git a/ipaclient/plugins/vault.py b/ipaclient/plugins/vault.py
index d677ec0..ca6d6da 100644
--- a/ipaclient/plugins/vault.py
+++ b/ipaclient/plugins/vault.py
@@ -558,74 +558,79 @@ def forward(self, *args, **options):
         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
+        except:
+            return True
 
 
 _transport_cert_cache = _TransportCertCache()
@@ -646,7 +651,10 @@ def forward(self, *args, **options):
         # 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 +688,7 @@ def _do_internal(self, algo, transport_cert, raise_unexpected,
         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 +699,21 @@ def internal(self, algo, *args, **options):
         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
+        response = self.api.Command.vaultconfig_show()
+        transport_cert = x509.load_certificate(
+            response['result']['transport_cert'], x509.DER)
+        # cache it again
+        _transport_cert_cache.store_cert(
+            self.api.env.domain, transport_cert
+        )
         # call with the retrieved transport certificate
         return self._do_internal(algo, transport_cert, True,
                                  *args, **options)
-- 
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