Merge lp:~hloeung/charms/trusty/apache2/ssl-security-options into lp:charms/trusty/apache2

[Haw Loeung]

Add option to override supported cipher suites, enforce server's preference instead of browser/client, and reorder by default so:

- PFS cipher suites are more preferred.
- for performance and to keep computation low on servers, prefer 128bits over 256bits.

We also want to keep RC4-SHA for IE6/XP clients and ignore all LOW cipher suites or those where there's potential weaknesses such as DSS and 3DES. I've also used generics such as EECDH, EDH, HIGH, MEDIUM, and LOW so that it is more future-proof as more cipher suites are added or moved between HIGH/MEDIUM/LOW.

Apache docs says the default is "ALL:!ADH:RC4+RSA:+HIGH:+MEDIUM:+LOW:+SSLv2:+EXP" [1]. The difference between the default are as follows:

$ diff -Naurp <(openssl ciphers -V 'ALL:!ADH:RC4+RSA:+HIGH:+MEDIUM:+LOW:+SSLv2:+EXP' | sort) <(openssl ciphers -V 'EECDH+AES128:EDH+AES128:aRSA+AES128:EECDH:EDH:HIGH:-MEDIUM:RC4-SHA:!aNULL:!NULL:!LOW:!3DES:!DSS:!EXP:!PSK:!SRP' | sort) | grep '^[+-]'
--- /dev/fd/63 2014-09-09 17:16:42.124195525 +1000
+++ /dev/fd/62 2014-09-09 17:16:42.124195525 +1000
- 0x00,0x03 - EXP-RC4-MD5 SSLv3 Kx=RSA(512) Au=RSA Enc=RC4(40) Mac=MD5 export
- 0x00,0x04 - RC4-MD5 SSLv3 Kx=RSA Au=RSA Enc=RC4(128) Mac=MD5
- 0x00,0x06 - EXP-RC2-CBC-MD5 SSLv3 Kx=RSA(512) Au=RSA Enc=RC2(40) Mac=MD5 export
- 0x00,0x08 - EXP-DES-CBC-SHA SSLv3 Kx=RSA(512) Au=RSA Enc=DES(40) Mac=SHA1 export
- 0x00,0x09 - DES-CBC-SHA SSLv3 Kx=RSA Au=RSA Enc=DES(56) Mac=SHA1
- 0x00,0x0A - DES-CBC3-SHA SSLv3 Kx=RSA Au=RSA Enc=3DES(168) Mac=SHA1
- 0x00,0x11 - EXP-EDH-DSS-DES-CBC-SHA SSLv3 Kx=DH(512) Au=DSS Enc=DES(40) Mac=SHA1 export
- 0x00,0x12 - EDH-DSS-DES-CBC-SHA SSLv3 Kx=DH Au=DSS Enc=DES(56) Mac=SHA1
- 0x00,0x13 - EDH-DSS-DES-CBC3-SHA SSLv3 Kx=DH Au=DSS Enc=3DES(168) Mac=SHA1
- 0x00,0x14 - EXP-EDH-RSA-DES-CBC-SHA SSLv3 Kx=DH(512) Au=RSA Enc=DES(40) Mac=SHA1 export
- 0x00,0x15 - EDH-RSA-DES-CBC-SHA SSLv3 Kx=DH Au=RSA Enc=DES(56) Mac=SHA1
- 0x00,0x16 - EDH-RSA-DES-CBC3-SHA SSLv3 Kx=DH Au=RSA Enc=3DES(168) Mac=SHA1
- 0x00,0x32 - DHE-DSS-AES128-SHA SSLv3 Kx=DH Au=DSS Enc=AES(128) Mac=SHA1
- 0x00,0x38 - DHE-DSS-AES256-SHA SSLv3 Kx=DH Au=DSS Enc=AES(256) Mac=SHA1
- 0x00,0x40 - DHE-DSS-AES128-SHA256 TLSv1.2 Kx=DH Au=DSS Enc=AES(128) Mac=SHA256
- 0x00,0x44 - DHE-DSS-CAMELLIA128-SHA SSLv3 Kx=DH Au=DSS Enc=Camellia(128) Mac=SHA1
- 0x00,0x6A - DHE-DSS-AES256-SHA256 TLSv1.2 Kx=DH Au=DSS Enc=AES(256) Mac=SHA256
- 0x00,0x87 - DHE-DSS-CAMELLIA256-SHA SSLv3 Kx=DH Au=DSS Enc=Camellia(256) Mac=SHA1
- 0x00,0x8A - PSK-RC4-SHA SSLv3 Kx=PSK Au=PSK Enc=RC4(128) Mac=SHA1
- 0x00,0x8B - PSK-3DES-EDE-CBC-SHA SSLv3 Kx=PSK Au=PSK Enc=3DES(168) Mac=SHA1
- 0x00,0x8C - PSK-AES128-CBC-SHA SSLv3 Kx=PSK Au=PSK Enc=AES(128) Mac=SHA1
- 0x00,0x8D - PSK-AES256-CBC-SHA SSLv3 Kx=PSK Au=PSK Enc=AES(256) Mac=SHA1
- 0x00,0x96 - SEED-SHA SSLv3 Kx=RSA Au=RSA Enc=SEED(128) Mac=SHA1
- 0x00,0x99 - DHE-DSS-SEED-SHA SSLv3 Kx=DH Au=DSS Enc=SEED(128) Mac=SHA1
- 0x00,0x9A - DHE-RSA-SEED-SHA SSLv3 Kx=DH Au=RSA Enc=SEED(128) Mac=SHA1
- 0x00,0xA2 - DHE-DSS-AES128-GCM-SHA256 TLSv1.2 Kx=DH Au=DSS Enc=AESGCM(128) Mac=AEAD
- 0x00,0xA3 - DHE-DSS-AES256-GCM-SHA384 TLSv1.2 Kx=DH Au=DSS Enc=AESGCM(256) Mac=AEAD
- 0xC0,0x02 - ECDH-ECDSA-RC4-SHA SSLv3 Kx=ECDH/ECDSA Au=ECDH Enc=RC4(128) Mac=SHA1
- 0xC0,0x03 - ECDH-ECDSA-DES-CBC3-SHA SSLv3 Kx=ECDH/ECDSA Au=ECDH Enc=3DES(168) Mac=SHA1
- 0xC0,0x07 - ECDHE-ECDSA-RC4-SHA SSLv3 Kx=ECDH Au=ECDSA Enc=RC4(128) Mac=SHA1
- 0xC0,0x08 - ECDHE-ECDSA-DES-CBC3-SHA SSLv3 Kx=ECDH Au=ECDSA Enc=3DES(168) Mac=SHA1
- 0xC0,0x0C - ECDH-RSA-RC4-SHA SSLv3 Kx=ECDH/RSA Au=ECDH Enc=RC4(128) Mac=SHA1
- 0xC0,0x0D - ECDH-RSA-DES-CBC3-SHA SSLv3 Kx=ECDH/RSA Au=ECDH Enc=3DES(168) Mac=SHA1
- 0xC0,0x11 - ECDHE-RSA-RC4-SHA SSLv3 Kx=ECDH Au=RSA Enc=RC4(128) Mac=SHA1
- 0xC0,0x12 - ECDHE-RSA-DES-CBC3-SHA SSLv3 Kx=ECDH Au=RSA Enc=3DES(168) Mac=SHA1
- 0xC0,0x16 - AECDH-RC4-SHA SSLv3 Kx=ECDH Au=None Enc=RC4(128) Mac=SHA1
- 0xC0,0x17 - AECDH-DES-CBC3-SHA SSLv3 Kx=ECDH Au=None Enc=3DES(168) Mac=SHA1
- 0xC0,0x18 - AECDH-AES128-SHA SSLv3 Kx=ECDH Au=None Enc=AES(128) Mac=SHA1
- 0xC0,0x19 - AECDH-AES256-SHA SSLv3 Kx=ECDH Au=None Enc=AES(256) Mac=SHA1
- 0xC0,0x1A - SRP-3DES-EDE-CBC-SHA SSLv3 Kx=SRP Au=SRP Enc=3DES(168) Mac=SHA1
- 0xC0,0x1B - SRP-RSA-3DES-EDE-CBC-SHA SSLv3 Kx=SRP Au=RSA Enc=3DES(168) Mac=SHA1
- 0xC0,0x1C - SRP-DSS-3DES-EDE-CBC-SHA SSLv3 Kx=SRP Au=DSS Enc=3DES(168) Mac=SHA1
- 0xC0,0x1D - SRP-AES-128-CBC-SHA SSLv3 Kx=SRP Au=SRP Enc=AES(128) Mac=SHA1
- 0xC0,0x1E - SRP-RSA-AES-128-CBC-SHA SSLv3 Kx=SRP Au=RSA Enc=AES(128) Mac=SHA1
- 0xC0,0x1F - SRP-DSS-AES-128-CBC-SHA SSLv3 Kx=SRP Au=DSS Enc=AES(128) Mac=SHA1
- 0xC0,0x20 - SRP-AES-256-CBC-SHA SSLv3 Kx=SRP Au=SRP Enc=AES(256) Mac=SHA1
- 0xC0,0x21 - SRP-RSA-AES-256-CBC-SHA SSLv3 Kx=SRP Au=RSA Enc=AES(256) Mac=SHA1
- 0xC0,0x22 - SRP-DSS-AES-256-CBC-SHA SSLv3 Kx=SRP Au=DSS Enc=AES(256) Mac=SHA1

[1]http://httpd.apache.org/docs/2.2/mod/mod_ssl.html#sslciphersuite

[Review Queue (review-queue)]

The results (PASS) are in and available here: http://reports.vapour.ws/charm-tests/charm-bundle-test-967-results

review: Approve (cbt)

[Charles Butler (lazypower)]

Greetings Haw,

I've had a chance to review your merge proposal and this looks like a very welcome addition to the Apache2 charm. I'm a fan of using Ephemeral (temp.key) Diffie-Hellman key exchange (no cert) after hearing about it on the Security Now podcast. I see this would expose that configuration to me in the charm and I think that's wonderful.

I've deployed the requested patch and had no issues, however i'd like to see an addition to the README calling out the new feature, as well as example deployment options for users that are interested in this behavior.

Once you've gotten the requested documentation added I see no reason this won't be merged.

Thanks for the great contribution!

review: Needs Fixing

[Haw Loeung (hloeung)]

Charles, first of all, thanks for reviewing my changes.

Unfortunately, it seems that there's a slight misunderstanding here. My changes allows overriding the default apache settings for SSL sites and overrides them with more recommended settings.

You can see the default settings in /etc/apache2/mods-available/ssl.conf.

[Review Queue (review-queue)]

This items has failed automated testing! Results available here http://reports.vapour.ws/charm-tests/charm-bundle-test-1191-results

review: Needs Fixing (automated testing)

[lp:~hloeung/charms/trusty/apache2/ssl-security-options]

58. By Haw Loeung on 2014-10-13

[trivial] Merged changes from upstream.

59. By Haw Loeung on 2014-10-15

Disable SSLv3 to mitigate POODLE.

60. By Haw Loeung on 2014-10-16

Refine cipher suites.

[Haw Loeung (hloeung)]

Very similar to Mozilla's "Intermediate" settings but with 128 preferred over 256. Also we're not specifying specific cipher suites and use "HIGH" to support any new ciphers OpenSSL adds.

Diff between Mozilla's settings and ours:

$ diff -Naurp <(openssl ciphers -V 'ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:DHE-DSS-AES128-GCM-SHA256:kEDH+AESGCM:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES256-SHA384:ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES256-SHA:ECDHE-ECDSA-AES256-SHA:DHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA:DHE-DSS-AES128-SHA256:DHE-RSA-AES256-SHA256:DHE-DSS-AES256-SHA:DHE-RSA-AES256-SHA:AES128-GCM-SHA256:AES256-GCM-SHA384:AES128:AES256:AES:!aNULL:!eNULL:!EXPORT:!DES:!RC4:!MD5:!PSK' | grep -Ev '(DSS|SRP)') <(openssl ciphers -V 'EECDH+AESGCM+AES128:EDH+AESGCM+AES128:EECDH+AES128:EDH+AES128:ECDH+AESGCM+AES128:aRSA+AESGCM+AES128:ECDH+AES128:DH+AES128:aRSA+AES128:EECDH+AESGCM:EDH+AESGCM:EECDH:EDH:ECDH+AESGCM:aRSA+AESGCM:ECDH:DH:aRSA:HIGH:!MEDIUM:!aNULL:!NULL:!LOW:!3DES:!DSS:!EXP:!PSK:!SRP')
--- /dev/fd/63 2014-10-17 00:19:25.160785634 +1100
+++ /dev/fd/62 2014-10-17 00:19:25.160785634 +1100
@@ -1,36 +1,40 @@
           0xC0,0x2F - ECDHE-RSA-AES128-GCM-SHA256 TLSv1.2 Kx=ECDH Au=RSA Enc=AESGCM(128) Mac=AEAD
           0xC0,0x2B - ECDHE-ECDSA-AES128-GCM-SHA256 TLSv1.2 Kx=ECDH Au=ECDSA Enc=AESGCM(128) Mac=AEAD
- 0xC0,0x30 - ECDHE-RSA-AES256-GCM-SHA384 TLSv1.2 Kx=ECDH Au=RSA Enc=AESGCM(256) Mac=AEAD
- 0xC0,0x2C - ECDHE-ECDSA-AES256-GCM-SHA384 TLSv1.2 Kx=ECDH Au=ECDSA Enc=AESGCM(256) Mac=AEAD
           0x00,0x9E - DHE-RSA-AES128-GCM-SHA256 TLSv1.2 Kx=DH Au=RSA Enc=AESGCM(128) Mac=AEAD
- 0x00,0x9F - DHE-RSA-AES256-GCM-SHA384 TLSv1.2 Kx=DH Au=RSA Enc=AESGCM(256) Mac=AEAD
           0xC0,0x27 - ECDHE-RSA-AES128-SHA256 TLSv1.2 Kx=ECDH Au=RSA Enc=AES(128) Mac=SHA256
           0xC0,0x23 - ECDHE-ECDSA-AES128-SHA256 TLSv1.2 Kx=ECDH Au=ECDSA Enc=AES(128) Mac=SHA256
           0xC0,0x13 - ECDHE-RSA-AES128-SHA SSLv3 Kx=ECDH Au=RSA Enc=AES(128) Mac=SHA1
           0xC0,0x09 - ECDHE-ECDSA-AES128-SHA SSLv3 Kx=ECDH Au=ECDSA Enc=AES(128) Mac=SHA1
- 0xC0,0x28 - ECDHE-RSA-AES256-SHA384 TLSv1.2 Kx=ECDH Au=RSA Enc=AES(256) Mac=SHA384
- 0xC0,0x24 - ECDHE-ECDSA-AES256-SHA384 TLSv1.2 Kx=ECDH Au=ECDSA Enc=AES(256) Mac=SHA384
- 0xC0,0x14 - ECDHE-RSA-AES256-SHA SSLv3 Kx=ECDH Au=RSA Enc=AES(256) Mac=SHA1
- 0xC0,0x0A - ECDHE-ECDSA-AES256-SHA SSLv3 Kx=ECDH Au=ECDSA Enc=AES(256) Mac=SHA1
           0x00,0x67 - DHE-RSA-AES128-SHA256 TLSv1.2 Kx=DH Au=RSA Enc=AES(128) Mac=SHA256
           0x00,0x33 - DHE-RSA-AES128-SHA SSLv3 Kx=DH Au=RSA Enc=AES(128) Mac=SHA1
- 0x00,0x6B - DHE-RSA-AES256-SHA256 TLSv1.2 Kx=DH Au=RSA Enc=AES(256) Mac=SHA256
- 0x00,0x39 - DHE-RSA-AES256-SHA SSLv3 Kx=DH Au=RSA Enc=AES(256) Mac=SHA1
- 0x00,0x9C - AES128-GCM-SHA256 TLSv1.2 Kx=RSA Au=RSA Enc=AESGCM(128) Mac=AEAD
- ...

[Haw Loeung (hloeung)]

https://wiki.mozilla.org/Security/Server_Side_TLS#Recommended_configurations

[Adam Israel (aisrael)]

Hi Haw,

I've had a chance to review this merge proposal. As Charles mentioned, this will be a very nice addition to the apache2 charm. I'd also like to see some documentation added to the README to describe this new functionality, as well as unit tests against it.

There's one minor bug that could also use fixing. The Makefile needs to have `python-jinja2` added to the list of packages installed, or `make build` will fail in a clean environment.

I expect there'll be no problem promoting these changes to the store, once the above are addressed.

Thanks again for all of your work!

review: Needs Fixing

[lp:~hloeung/charms/trusty/apache2/ssl-security-options]

61. By Haw Loeung on 2014-11-20

Merged changes from upstream.

62. By Haw Loeung on 2014-11-20

Document new ssl_protocol, ssl_honor_cipher_order, and ssl_cipher_suite configuration options.

63. By Haw Loeung on 2014-11-20

Ignore virtualenv created by make build/test.

[Adam Israel (aisrael)]

Hi Haw,

Thanks for your work addressing the requested changes! I had the opportunity to retest this merge proposal today. The updates to the README and Makefile look great. I did run into one issue running the tests, unfortunately.

Both test_is_selfsigned_cert_stale_private_address_changed and test_is_selfsigned_cert_stale_public_address_changed failed, running under the local environment:

======================================================================
FAIL: tests.test_cert.CertTests.test_is_selfsigned_cert_stale_private_address_changed
----------------------------------------------------------------------
_StringException: Traceback (most recent call last):
  File "/home/stone/charms/trusty/apache2/.venv/local/lib/python2.7/site-packages/mock.py", line 1201, in patched
    return func(*args, **keywargs)
  File "/home/stone/charms/trusty/apache2/hooks/tests/test_cert.py", line 95, in test_is_selfsigned_cert_stale_private_address_changed
    hooks.is_selfsigned_cert_stale(config, cert_file, key_file))
  File "/home/stone/charms/trusty/apache2/.venv/local/lib/python2.7/site-packages/unittest2/case.py", line 678, in assertTrue
    raise self.failureException(msg)
AssertionError: False is not true

======================================================================
FAIL: tests.test_cert.CertTests.test_is_selfsigned_cert_stale_public_address_changed
----------------------------------------------------------------------
_StringException: Traceback (most recent call last):
  File "/home/stone/charms/trusty/apache2/.venv/local/lib/python2.7/site-packages/mock.py", line 1201, in patched
    return func(*args, **keywargs)
  File "/home/stone/charms/trusty/apache2/hooks/tests/test_cert.py", line 82, in test_is_selfsigned_cert_stale_public_address_changed
    hooks.is_selfsigned_cert_stale(config, cert_file, key_file))
  File "/home/stone/charms/trusty/apache2/.venv/local/lib/python2.7/site-packages/unittest2/case.py", line 678, in assertTrue
    raise self.failureException(msg)
AssertionError: False is not true

Thanks again for all your hard work on this! I'm looking forward to seeing these changes promulgated.

review: Needs Fixing

[Adam Israel (aisrael)]

Following up based on the precise version of this MP, the failing test is being caused upstream and should not hold up this merge proposal. You have my +1.

I'll file a bug against lp:charms/apache2, unless there's already one open.

review: Approve

=== modified file '.bzrignore'
--- .bzrignore	2013-10-10 22:47:57 +0000
+++ .bzrignore	2014-11-20 00:25:17 +0000
@@ -8,3 +8,4 @@
 *.key
 lib/*
 build/charm-helpers
+.venv/

=== modified file 'README.md'
--- README.md	2014-06-20 21:27:04 +0000
+++ README.md	2014-11-20 00:25:17 +0000
@@ -261,6 +261,11 @@
 according to `ssl_certlocation` and `ssl_keylocation` as listed
 above.
 
+`ssl_protocol`, `ssl_honor_cipher_order`, and `ssl_cipher_suite` can
+be used to override SSL/TLS version and the cipher suites supported.
+These default to what Canonical IS recommends and is using. Before
+making any changes, please see the Mozilla Security/Server Side TLS.
+
 ## `{enable,disable}_modules`
 
 Space separated list of modules to be enabled or disabled. If a module to

=== modified file 'config.yaml'
--- config.yaml	2014-01-30 09:11:53 +0000
+++ config.yaml	2014-11-20 00:25:17 +0000
@@ -151,6 +151,18 @@
         base64 encoded chain certificates file.  If ssl_cert is
         specified as SELFSIGNED, this will be ignored.
     default: ''
+  ssl_protocol:
+    type: string
+    description: SSL Protocols to enable.
+    default: "ALL -SSLv2 -SSLv3"
+  ssl_honor_cipher_order:
+    type: string
+    description: Enable server cipher suite preference.
+    default: "On"
+  ssl_cipher_suite:
+    type: string
+    description: List of server cipher suites.
+    default: "EECDH+AESGCM+AES128:EDH+AESGCM+AES128:EECDH+AES128:EDH+AES128:ECDH+AESGCM+AES128:aRSA+AESGCM+AES128:ECDH+AES128:DH+AES128:aRSA+AES128:EECDH+AESGCM:EDH+AESGCM:EECDH:EDH:ECDH+AESGCM:aRSA+AESGCM:ECDH:DH:aRSA:HIGH:!MEDIUM:!aNULL:!NULL:!LOW:!3DES:!DSS:!EXP:!PSK:!SRP"
   server_tokens:
     type: string
     description: Security setting. Set to one of  Full  OS  Minimal  Minor  Major  Prod

=== modified file 'data/security.template'
--- data/security.template	2014-05-27 14:44:13 +0000
+++ data/security.template	2014-11-20 00:25:17 +0000
@@ -50,3 +50,9 @@
 # Set to one of:  On | Off | extended
 #
 TraceEnable {{ trace_enabled }}
+
+<IfModule mod_ssl.c>
+	SSLProtocol {{ ssl_protocol }}
+	SSLHonorCipherOrder {{ ssl_honor_cipher_order }}
+	SSLCipherSuite {{ ssl_cipher_suite }}
+</IfModule>

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2014-10-29 00:10:14 +0000
+++ hooks/hooks.py	2014-11-20 00:25:17 +0000
@@ -23,6 +23,7 @@
     unit_get
 )
 from charmhelpers.contrib.charmsupport import nrpe
+from charmhelpers.fetch import apt_update
 
 ###############################################################################
 # Global variables
@@ -287,6 +288,7 @@
 def install_hook():
     if not os.path.exists(default_apache2_service_config_dir):
         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")
@@ -470,6 +472,9 @@
         'server_tokens': config_data['server_tokens'],
         'server_signature': config_data['server_signature'],
         'trace_enabled': config_data['trace_enabled'],
+        'ssl_protocol': config_data['ssl_protocol'],
+        'ssl_honor_cipher_order': config_data['ssl_honor_cipher_order'],
+        'ssl_cipher_suite': config_data['ssl_cipher_suite'],
         'is_apache24': is_apache24(),
     }
     template = \

=== modified file 'hooks/tests/test_balancer_hook.py'
--- hooks/tests/test_balancer_hook.py	2014-06-20 21:27:04 +0000
+++ hooks/tests/test_balancer_hook.py	2014-11-20 00:25:17 +0000
@@ -502,7 +502,9 @@
     @patch('os.mkdir')
     @patch('hooks.apt_get_install')
     @patch('hooks.log', MagicMock())
-    def test_installs_hook(self, apt_get_install, mkdir, exists, config_get):
+    @patch('hooks.apt_update')
+    def test_installs_hook(
+            self, apt_update, apt_get_install, mkdir, exists, config_get):
         exists.return_value = self.not_a_dir
         config_get.return_value = None
         apt_get_install.return_value = 'some result'
@@ -526,8 +528,9 @@
     @patch('os.mkdir')
     @patch('hooks.apt_get_install')
     @patch('hooks.log', MagicMock())
+    @patch('hooks.apt_update')
     def test_install_hook_installs_extra_packages(
-            self, apt_get_install, mkdir, exists, config_get):
+            self, apt_update, apt_get_install, mkdir, exists, config_get):
         exists.return_value = self.dir_exists
         config_get.return_value = "extra"
         apt_get_install.return_value = 'some result'
@@ -549,8 +552,9 @@
     @patch('os.mkdir')
     @patch('hooks.apt_get_install')
     @patch('hooks.log', MagicMock())
+    @patch('hooks.apt_update')
     def test_doesnt_create_dir_to_install_hooks_if_not_needed(
-            self, apt_get_install, mkdir, exists, config_get):
+            self, apt_update, apt_get_install, mkdir, exists, config_get):
         exists.return_value = self.dir_exists
         config_get.return_value = None
         apt_get_install.return_value = 'some result'

=== 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-11-20 00:25:17 +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))