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