Merge lp:~dpb/charms/precise/apache2/avoid-regen-cert into lp:charms/apache2

[David Britton]

Don't regen a self-signed cert unless we need to:

- pulled out cert generation code
- added test cases for it
- made regeneration dependent on necessity (hostname/ip changed, servername changed, cert is missing/invalid)
- Included small fix for lp:1302645 to install python-yaml

[Tim Van Steenburgh (tvansteenburgh)]

Hi David,

I'd like to review this submission but your branch doesn't merge cleanly with lp:charms/apache2. Would you mind pushing an update?

Thanks!
Tim

[David Britton (dpb)]

On Mon, Jun 16, 2014 at 04:02:42PM -0000, Tim Van Steenburgh wrote:
> Hi David,
>
> I'd like to review this submission but your branch doesn't merge
> cleanly with lp:charms/apache2. Would you mind pushing an update?
>

Hi Tim --

I resolved that conflict, it's ready for a re-review. (the merge
conflict was trivial, tests all still pass).

--
David Britton <email address hidden>

[lp:~dpb/charms/precise/apache2/avoid-regen-cert]

57. By David Britton on 2014-06-18

merge up

[Tim Van Steenburgh (tvansteenburgh)]

Thanks David!

+1 LGTM. And you get +1000 bonus points for adding unit tests. :D

A member of ~charmers will be along after me for a follow-up review and to push this change to the charm store.

[Jorge Niedbalski (niedbalski)]

Hello David,

Thanks for your submission. I understand your motivation, but i am unsure if we should force to keep the same certificate without having an configuration option to force the certificate re-generation.

My suggestions for your changes:

- Implement a configuration directive ( ssl_cert_regenerate ) to allow users to force the self-signed certificate generation. Which would be the first check to do.

- On the function is_selfsigned_cert_stale() Please check first if the private key has changed (available on config.get('ssl_keylocation') that will allow us to determine quicker is the certificate is stale.

Please Let me know your observations.

review: Needs Fixing

[David Britton (dpb)]

On Fri, Jun 20, 2014 at 4:25 PM, Jorge Niedbalski <
<email address hidden>> wrote:

> Review: Needs Fixing
>
> Hello David,
>
> Thanks for your submission. I understand your motivation, but i am unsure
> if we should force to keep the same certificate without having an
> configuration option to force the certificate re-generation.
>
>
Hi Jorge --

Thanks for looking this over...

> My suggestions for your changes:
>
> - Implement a configuration directive ( ssl_cert_regenerate ) to allow
> users to force the self-signed certificate generation. Which would be the
> first check to do.
>
>
I'm not sure that would be a good investment here. This would be covered
in the existing charm (and in my change), by simply doing:

juju set ssl_certlocation="a_new_cert.cert"

SELFSIGNED is already mostly a testing/debugging feature. Having another
way to regenerate it with a config key seems a bit too much? What do you
think? If you still feel strongly, I think it's probably best left for an
add-on feature if someone feels the need to implement it.

> - On the function is_selfsigned_cert_stale() Please check first if the
> private key has changed (available on config.get('ssl_keylocation') that
> will allow us to determine quicker is the certificate is stale.
>

In the case of SELFSIGNED, the key is written by the charm to the
ssl_keylocation. Are you suggesting that the location of the key would
change by the admin through juju set? If so, I do check that. That case
will cause the self-signed cert to be regenerated.

Could you please explain further if I have misunderstood you?

>
>
> Diff comments:
>
> > +def is_selfsigned_cert_stale(config, cert_file, key_file):
> > + """
> > + Do we need to generate a new self-signed cert?
> > +
> > + @param config: charm data from config-get
> > + @param cert_file: destination path of generated certificate
> > + @param key_file: destination path of generated private key
> > + """
> > + # Basic Existence Checks
> > + if not os.path.exists(cert_file):
> > + return True
> > + if not os.path.exists(key_file):
> > + return True
> > +
> > + # Common Name
> > + from OpenSSL import crypto
> > + cert = crypto.load_certificate(
>
> If for whatever reason the load_certificate function fails, We should mark
> the certificate as stale, right? I think so.
>
>
If it failed -- threw an exception? returned None? -- it will actually
cause this method to error, and the hook, in turn, to error. There are a
couple of reasons this could fail, the ones I can think of seem appropriate
to allow the hook to error, instead of masking the problem, removing the
file (who knows what was there?), and generating another cert over the top
of it.

Presumably we generated the cert that we just failed to read, so I don't
think it would be helpful to continue generating certs like that.

Are you thinking of a particular use case or failure mode here? It would
help if I had something to test. I admit I may not be thinking of all the
options.

--
David Britton <email address hidden>

[Jorge Niedbalski (niedbalski)]

Thanks for your quick reply.

> I'm not sure that would be a good investment here. This would be covered
> in the existing charm (and in my change), by simply doing:
>
> juju set ssl_certlocation="a_new_cert.cert"
>
> SELFSIGNED is already mostly a testing/debugging feature. Having another
> way to regenerate it with a config key seems a bit too much? What do you
> think? If you still feel strongly, I think it's probably best left for an
> add-on feature if someone feels the need to implement it.
>

Charm's features must be made for being used in production, never for debugging/testing purposes, so i assumed by default that everything should be configurable and reversible.

Looking at the ssl_certlocation will cover the suggested case and having
something more specific would be a 'nice to have' feature. But for now, is OK to me to
use the `certlocation` directive.

>
> > - On the function is_selfsigned_cert_stale() Please check first if the
> > private key has changed (available on config.get('ssl_keylocation') that
> > will allow us to determine quicker is the certificate is stale.
> >
>
> In the case of SELFSIGNED, the key is written by the charm to the
> ssl_keylocation. Are you suggesting that the location of the key would
> change by the admin through juju set? If so, I do check that. That case
> will cause the self-signed cert to be regenerated.

Yes, that variable can be changed via `juju set`, so we must cover that
case on your re-generation constraints.

> If it failed -- threw an exception? returned None? -- it will actually
> cause this method to error, and the hook, in turn, to error. There are a
> couple of reasons this could fail, the ones I can think of seem appropriate
> to allow the hook to error, instead of masking the problem, removing the
> file (who knows what was there?), and generating another cert over the top
> of it.

I am certainly not aware of all the possible failures here, just making sure
that you know the potential errors that should be caught here. If you
think is OK to error the hook and not will not be necessary to flag the certificate as stale, i am OK with that.

[David Britton (dpb)]

On Mon, Jun 23, 2014 at 02:56:58PM -0000, Jorge Niedbalski wrote:
>
> Yes, that variable can be changed via `juju set`, so we must cover that
> case on your re-generation constraints.

I think we are good then:

    if not os.path.exists(cert_file):
        return True

That should catch the case you are talking about.

>
> I am certainly not aware of all the possible failures here, just making sure
> that you know the potential errors that should be caught here. If you
> think is OK to error the hook and not will not be necessary to flag the certificate as stale, i am OK with that.

Great -- Yes, I'm fine with that. These class of errors work best
making the charm error, IMO. I don't know why they would happen, and
it's dangerous to make assumptions about that.

Thanks!

--
David Britton <email address hidden>

[Marco Ceppi (marcoceppi)]

LGTM

review: Approve

=== modified file 'README.md'
--- README.md	2014-05-21 18:31:11 +0000
+++ README.md	2014-06-18 19:05:46 +0000
@@ -17,7 +17,8 @@
     sudo apt-get install python-software-properties
     sudo add-apt-repository ppa:chrisjohnston/flake8
     sudo apt-get update
-    sudo apt-get install flake8 python-nose python-coverage python-testtools
+    sudo apt-get install python-flake8 python-nose python-coverage \
+                         python-testtools python-pyasn1 python-pyasn1-modules
 
 To fetch additional source dependencies and run the tests:
 

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2014-06-11 17:57:46 +0000
+++ hooks/hooks.py	2014-06-18 19:05:46 +0000
@@ -50,14 +50,12 @@
 # Supporting functions
 ###############################################################################
 
-#------------------------------------------------------------------------------
-# apt_get_install( package ):  Installs a package
-#------------------------------------------------------------------------------
-def apt_get_install(packages=None):
-    if packages is None:
+def apt_get_install(package=None):
+    """Install a package."""
+    if package is None:
         return False
     cmd_line = ['apt-get', '-y', 'install', '-qq']
-    cmd_line.append(packages)
+    cmd_line.append(package)
     return subprocess.call(cmd_line)
 
 
@@ -181,6 +179,68 @@
     return True
 
 
+def gen_selfsigned_cert(config, cert_file, key_file):
+    """
+    Create a self-signed certificate.
+
+    @param config: charm data from config-get
+    @param cert_file: destination path of generated certificate
+    @param key_file: destination path of generated private key
+    """
+    os.environ['OPENSSL_CN'] = config['servername']
+    os.environ['OPENSSL_PUBLIC'] = unit_get("public-address")
+    os.environ['OPENSSL_PRIVATE'] = unit_get("private-address")
+    run(
+        ['openssl', 'req', '-new', '-x509', '-nodes', '-config',
+         os.path.join(os.environ['CHARM_DIR'], 'data', 'openssl.cnf'),
+         '-keyout', key_file, '-out', cert_file])
+
+
+def is_selfsigned_cert_stale(config, cert_file, key_file):
+    """
+    Do we need to generate a new self-signed cert?
+
+    @param config: charm data from config-get
+    @param cert_file: destination path of generated certificate
+    @param key_file: destination path of generated private key
+    """
+    # Basic Existence Checks
+    if not os.path.exists(cert_file):
+        return True
+    if not os.path.exists(key_file):
+        return True
+
+    # Common Name
+    from OpenSSL import crypto
+    cert = crypto.load_certificate(
+        crypto.FILETYPE_PEM, file(cert_file).read())
+    cn = cert.get_subject().commonName
+    if config['servername'] != cn:
+        return True
+
+    # Subject Alternate Name
+    from pyasn1.codec.der import decoder
+    from pyasn1_modules import rfc2459
+    cert_addresses = set()
+    unit_addresses = set(
+        [unit_get("public-address"), unit_get("private-address")])
+    for i in range(0, cert.get_extension_count()):
+        extension = cert.get_extension(i)
+        try:
+            names = decoder.decode(
+                extension.get_data(), asn1Spec=rfc2459.SubjectAltName())[0]
+            for name in names:
+                cert_addresses.add(str(name.getComponent()))
+        except:
+            pass
+    if cert_addresses != unit_addresses:
+        log("subjAltName: Cert (%s) != Unit (%s), assuming stale" % (
+            cert_addresses, unit_addresses))
+        return True
+
+    return False
+
+
 def _get_key_file_location(config_data):
     """Look at the config, generate the key file location."""
     key_file = None
@@ -208,9 +268,6 @@
     return chain_file
 
 
-###############################################################################
-# Hook functions
-###############################################################################
 def config_get(scope=None):
     """
     Wrapper around charm helper's config_get to replace an empty servername
@@ -229,6 +286,9 @@
         os.mkdir(default_apache2_service_config_dir, 0600)
     apt_update(fatal=True)
     apt_get_install("python-jinja2")
+    apt_get_install("python-pyasn1")
+    apt_get_install("python-pyasn1-modules")
+    apt_get_install("python-yaml")
     install_status = apt_get_install("apache2")
     if install_status == 0:
         ensure_package_status(service_affecting_packages,
@@ -519,12 +579,8 @@
     if cert_file is not None and key_file is not None:
         # ssl_cert is SELFSIGNED so generate self-signed certificate for use.
         if config_data['ssl_cert'] and config_data['ssl_cert'] == "SELFSIGNED":
-            os.environ['OPENSSL_CN'] = config_data['servername']
-            os.environ['OPENSSL_PUBLIC'] = unit_get("public-address")
-            os.environ['OPENSSL_PRIVATE'] = unit_get("private-address")
-            run(['openssl', 'req', '-new', '-x509', '-nodes', '-config',
-                 os.path.join(os.environ['CHARM_DIR'], 'data', 'openssl.cnf'),
-                 '-keyout', key_file, '-out', cert_file])
+            if is_selfsigned_cert_stale(config_data, cert_file, key_file):
+                gen_selfsigned_cert(config_data, cert_file, key_file)
 
         # Use SSL certificate and key provided either as a base64 string or
         # shipped out with the charm.

=== modified file 'hooks/tests/test_balancer_hook.py'
--- hooks/tests/test_balancer_hook.py	2014-06-11 17:18:57 +0000
+++ hooks/tests/test_balancer_hook.py	2014-06-18 19:05:46 +0000
@@ -517,6 +517,9 @@
                                  0600)
         apt_get_install.assert_has_calls([
             call('python-jinja2'),
+            call('python-pyasn1'),
+            call('python-pyasn1-modules'),
+            call('python-yaml'),
             call('apache2'),
         ])
 
@@ -537,6 +540,9 @@
         self.assertEqual(result, 'some result')
         apt_get_install.assert_has_calls([
             call('python-jinja2'),
+            call('python-pyasn1'),
+            call('python-pyasn1-modules'),
+            call('python-yaml'),
             call('apache2'),
             call('extra'),
         ])
@@ -560,6 +566,9 @@
         self.assertFalse(mkdir.called)
         apt_get_install.assert_has_calls([
             call('python-jinja2'),
+            call('python-pyasn1'),
+            call('python-pyasn1-modules'),
+            call('python-yaml'),
             call('apache2'),
         ])
 

=== added file 'hooks/tests/test_cert.py'
--- hooks/tests/test_cert.py	1970-01-01 00:00:00 +0000
+++ hooks/tests/test_cert.py	2014-06-18 19:05:46 +0000
@@ -0,0 +1,124 @@
+import os
+import shutil
+import tempfile
+
+from testtools import TestCase
+from mock import patch
+
+import hooks
+
+
+original_open = open
+
+
+def _unit_get(value):
+    return value
+
+
+def _unit_get_public_address_changed(value):
+    if value == "public-address":
+        return "changed"
+    return "foo"
+
+
+def _unit_get_private_address_changed(value):
+    if value == "private-address":
+        return "changed"
+    return "foo"
+
+
+class CertTests(TestCase):
+    def setUp(self):
+        super(CertTests, self).setUp()
+        self.tempdir = tempfile.mkdtemp()
+
+    def tearDown(self):
+        super(CertTests, self).tearDown()
+        shutil.rmtree(self.tempdir, ignore_errors=True)
+
+    def test_is_selfsigned_cert_stale_missing_key(self):
+        """Missing cert, we should regenerate."""
+        config = {"servername": "servername"}
+        cert_file = os.path.join(self.tempdir, "cert")
+        key_file = os.path.join(self.tempdir, "key")
+        with open(cert_file, "w") as f:
+            f.write("foo")
+        self.assertTrue(
+            hooks.is_selfsigned_cert_stale(config, cert_file, key_file))
+
+    def test_is_selfsigned_cert_stale_missing_cert(self):
+        """Missing cert, we should regenerate."""
+        config = {"servername": "servername"}
+        cert_file = os.path.join(self.tempdir, "cert")
+        key_file = os.path.join(self.tempdir, "key")
+        with open(key_file, "w") as f:
+            f.write("foo")
+        self.assertTrue(
+            hooks.is_selfsigned_cert_stale(config, cert_file, key_file))
+
+    @patch("hooks.log")
+    @patch("hooks.unit_get", return_value="unit")
+    def test_is_selfsigned_cert_stale_cn_changed(self, unit_get, log):
+        """With different servername, cert *should* be regenerated."""
+        config = {"servername": "servername"}
+        cert_file = os.path.join(self.tempdir, "cert")
+        key_file = os.path.join(self.tempdir, "key")
+        hooks.gen_selfsigned_cert(config, cert_file, key_file)
+        config["servername"] = "changed"
+        self.assertTrue(
+            hooks.is_selfsigned_cert_stale(config, cert_file, key_file))
+
+    @patch("hooks.log")
+    @patch("hooks.unit_get", side_effect=_unit_get)
+    def test_is_selfsigned_cert_stale_public_address_changed(
+            self, unit_get, log):
+        """With different servername, cert *should* be regenerated."""
+        config = {"servername": "servername"}
+        cert_file = os.path.join(self.tempdir, "cert")
+        key_file = os.path.join(self.tempdir, "key")
+        hooks.gen_selfsigned_cert(config, cert_file, key_file)
+        unit_get.side_effect = _unit_get_public_address_changed
+        self.assertTrue(
+            hooks.is_selfsigned_cert_stale(config, cert_file, key_file))
+
+    @patch("hooks.log")
+    @patch("hooks.unit_get", side_effect=_unit_get)
+    def test_is_selfsigned_cert_stale_private_address_changed(
+            self, unit_get, log):
+        """With different servername, cert *should* be regenerated."""
+        config = {"servername": "servername"}
+        cert_file = os.path.join(self.tempdir, "cert")
+        key_file = os.path.join(self.tempdir, "key")
+        hooks.gen_selfsigned_cert(config, cert_file, key_file)
+        unit_get.side_effect = _unit_get_private_address_changed
+        self.assertTrue(
+            hooks.is_selfsigned_cert_stale(config, cert_file, key_file))
+
+    @patch("hooks.log")
+    @patch("hooks.unit_get", return_value="unit")
+    def test_is_selfsigned_cert_stale_assert_false(self, unit_get, log):
+        """Happy path, cert exists, doesn't need to be regenerated."""
+        config = {"servername": "servername"}
+        cert_file = os.path.join(self.tempdir, "cert")
+        key_file = os.path.join(self.tempdir, "key")
+        hooks.gen_selfsigned_cert(config, cert_file, key_file)
+        self.assertFalse(
+            hooks.is_selfsigned_cert_stale(config, cert_file, key_file))
+
+    @patch("hooks.log")
+    @patch("hooks.unit_get", return_value="unit")
+    def test_gen_selfsigned_cert(self, unit_get, log):
+        """
+        Happy path, make sure we can generate a cert, invalidate,
+        and write another.
+        """
+        config = {"servername": "servername"}
+        cert_file = os.path.join(self.tempdir, "cert")
+        key_file = os.path.join(self.tempdir, "key")
+        hooks.gen_selfsigned_cert(config, cert_file, key_file)
+        config["servername"] = "changed"
+        self.assertTrue(
+            hooks.is_selfsigned_cert_stale(config, cert_file, key_file))
+        hooks.gen_selfsigned_cert(config, cert_file, key_file)
+        self.assertFalse(
+            hooks.is_selfsigned_cert_stale(config, cert_file, key_file))