[Freeipa-devel] [freeipa PR#616][synchronized] Simplify KRA transport cert cache

2017-03-17 Thread tiran 
   URL: https://github.com/freeipa/freeipa/pull/616
Author: tiran
 Title: #616: Simplify KRA transport cert cache
Action: synchronized

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 3aa2cf9ead8af29bfd7d64176e000ad2a4e7cbcd Mon Sep 17 00:00:00 2001
From: Christian Heimes 
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 
---
 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 d677ec0..3fb4900 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 @@ 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:

[Freeipa-devel] [freeipa PR#616][synchronized] Simplify KRA transport cert cache

2017-03-17 Thread tiran 
   URL: https://github.com/freeipa/freeipa/pull/616
Author: tiran
 Title: #616: Simplify KRA transport cert cache
Action: synchronized

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 44d2c83342a2e87a35674d6cac831fc6122d9ff8 Mon Sep 17 00:00:00 2001
From: Christian Heimes 
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 
---
 ipaclient/plugins/vault.py | 102 -
 1 file changed, 55 insertions(+), 47 deletions(-)

diff --git a/ipaclient/plugins/vault.py b/ipaclient/plugins/vault.py
index d677ec0..b7045b7 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