vault: cache the transport certificate on client #476

HonzaCholasta · 2017-02-17T11:22:28Z

https://fedorahosted.org/freeipa/ticket/6652

tiran

Good work!

I left a couple of comments and I'm going to update the issue with to action items.

tiran · 2017-02-17T11:30:26Z

ipaclient/plugins/vault.py

+                raise
+        with tempfile.NamedTemporaryFile(dir=TRANSPORT_CERT_CACHE_PATH,
+                                         delete=False) as f:
+            f.write(data)


It's safer to close or at least flush the file.

Feel free to ignore the problem for now and replace the function later with David's helper function from PR #467.

tiran · 2017-02-17T12:04:08Z

ipaclient/plugins/vault.py

+                set_cached_transport_cert(api.env.domain, transport_cert)
+            else:
+                transport_cert_der = (
+                    transport_cert.public_bytes(serialization.Encoding.DER))


Please move this functionality to a plugin or some other place. I like to fetch and cache the KRA transport cert every time Custodia is started. The API should have an optional argument to force a refresh.

This is not what was requested in the ticket...

Anyway, what's the point? Is there a design or PoC for Custodia that I can take a look at?

It was obvious to me that the certificate is retrieved and cached in the main process at start-up. It's faster and avoids so many issues with concurrent reads and writes in concurrent processes. In fact it was so obvious to me that I did not consider any other approach when I wrote the ticket.

Added in-memory caching in the last update.

An in-memory cache does not work for Custodia because it is a forking webserver. It has to be an API call that ignores any caches (file) and downloads the KRA transport cert from remote server.

tiran · 2017-02-17T12:05:16Z

ipaclient/plugins/vault.py

+            if e.errno != errno.ENOENT:
+                raise
+    except Exception as e:
+        logger.warning("Failed to load %s: %s", filename, e)


Please use exc_info=True everywhere in order to get a full traceback. It makes debugging much more pleasant.

tiran · 2017-02-17T12:07:00Z

ipaclient/plugins/vault.py

+            except (errors.InternalError,
+                    errors.ExecutionError,
+                    errors.GenericError):
+                del_cached_transport_cert(api.env.domain)


This removes the cached key in any case of error. That's OK for a PoC. For the final version we should be more clever and only delete the certificate when it is not recognized by the KRA. I'll figure out the exact error message.

KRA subsystem doesn't report invalid KRA transport cert correctly, https://fedorahosted.org/pki/ticket/2597

The cache is deleted only in case of an unexpected error, which is why there is a list of them, not in case of any error.

The KRA bug should definitely be fixed, but we should still cache the cert properly even for older servers.

tiran

Good work! exc_info() can lead to memory-leak like reference cycles. See inline comment

tiran · 2017-02-22T12:10:11Z

ipaclient/plugins/vault.py

+        except (errors.InternalError,
+                errors.ExecutionError,
+                errors.GenericError):
+            raise _TransportCertInvalid(sys.exc_info())


sys.exc_info() is problematic. It returns a stack frame object, which usually creates a reference cycle that keeps all local objects alive. If any object has __del__ method, then Python 2 is unable to break the reference cycle. It's rather bad for long running processes because it is a kind of memory leak.

Here are two reasonable well written blog postings that I just found

http://engineering.hearsaysocial.com/2013/06/16/circular-references-in-python/

https://cosmicpercolator.com/2016/01/13/exception-leaks-in-python-2-and-3/

Proposal:

class _TransportCertInvalid(Exception): def __init__(self, exception): self.inner = exception

and

except (errors.InternalError, errors.ExecutionError, errors.GenericError) as e: root_logger.debug("some message", exc_info=True) raise _TransportCertInvalid(e)

later:

six.reraise(type(e.inner), e.inner, None)

There is no local variable which would hold the reference to the sys.exc_info() result or the _TransportCertInvalid exception. Is raising the exception enough to create a reference to it?

tiran · 2017-02-22T12:12:58Z

ipaclient/plugins/vault.py

+        """
+        domain = self.api.env.domain
+        dirname = TRANSPORT_CERT_CACHE_PATH
+        basename = DNSName(domain).ToASCII() + '.pem'


What do you think about DNSName(domain).ToASCII() + '-kra-transport.pem' to make the purpose of the cert clear?

Seems a bit redundant to me, since the full path is ~/.cache/ipa/kra-transport-certs/$DOMAIN.pem.

Scratch my proposal. I didn't notice that you are using an extra directory just for KRA transport certs. Sorry.

tiran · 2017-02-22T12:15:16Z

ipaclient/plugins/vault.py

+            except _TransportCertInvalid:
+                _transport_cert_cache.pop(domain, None)
+                try:
+                    os.remove(filename)


Minor nit pick: os.unlink. Unix developers don't remove files. We unlink names from inodes. :)

tiran · 2017-03-02T14:08:38Z

I didn't see your comment regarding the in-memory cache because github hid the section after your most recent push.

The in-memory cache doesn't work for Custodia because Custodia is a forking webserver. Requests are handled in one-shot client processes. I must be able to forcefully download the certificate in the main process, before it starts listening on incoming requests.

HonzaCholasta · 2017-03-06T10:45:37Z

Calling vaultconfig_show now refreshes the cache, you can call it to download and cache the certificate in the main process.

MartinBasti · 2017-03-09T13:03:14Z

needs rebase

Cache the KRA transport certificate on disk (in ~/.cache/ipa) as well as in memory. https://fedorahosted.org/freeipa/ticket/6652

MartinBasti · 2017-03-13T11:05:57Z

Works for me

MartinBasti · 2017-03-13T15:02:42Z

master:

98bb539 vault: cache the transport certificate on client

tiran requested changes Feb 17, 2017

View reviewed changes

tiran requested changes Feb 22, 2017

View reviewed changes

vault: cache the transport certificate on client

8bee187

Cache the KRA transport certificate on disk (in ~/.cache/ipa) as well as in memory. https://fedorahosted.org/freeipa/ticket/6652

MartinBasti added ack Pull Request approved, can be merged pushed Pull Request has already been pushed labels Mar 13, 2017

MartinBasti closed this Mar 13, 2017

vault: cache the transport certificate on client #476

vault: cache the transport certificate on client #476

HonzaCholasta commented Feb 17, 2017 •

edited by MartinBasti

tiran left a comment

tiran Feb 17, 2017

tiran Feb 17, 2017

HonzaCholasta Feb 21, 2017

HonzaCholasta Feb 21, 2017

tiran Feb 21, 2017

HonzaCholasta Feb 22, 2017

tiran Mar 2, 2017 •

edited

tiran Feb 17, 2017

HonzaCholasta Feb 21, 2017

tiran Feb 17, 2017

tiran Feb 17, 2017

HonzaCholasta Feb 21, 2017

tiran left a comment

tiran Feb 22, 2017

HonzaCholasta Feb 23, 2017

tiran Feb 22, 2017

HonzaCholasta Feb 23, 2017

tiran Feb 23, 2017

tiran Feb 22, 2017

HonzaCholasta Feb 23, 2017

tiran commented Mar 2, 2017

HonzaCholasta commented Mar 6, 2017

MartinBasti commented Mar 9, 2017

MartinBasti commented Mar 13, 2017

MartinBasti commented Mar 13, 2017

vault: cache the transport certificate on client #476

vault: cache the transport certificate on client #476

Conversation

HonzaCholasta commented Feb 17, 2017 • edited by MartinBasti

tiran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiran Mar 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiran commented Mar 2, 2017

HonzaCholasta commented Mar 6, 2017

MartinBasti commented Mar 9, 2017

MartinBasti commented Mar 13, 2017

MartinBasti commented Mar 13, 2017

HonzaCholasta commented Feb 17, 2017 •

edited by MartinBasti

tiran Mar 2, 2017 •

edited