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

Proposed by Haw Loeung on 2014-09-09
Status: Merged
Merged at revision: 60
Proposed branch: lp:~hloeung/charms/precise/apache2/ssl-security-options
Merge into: lp:charms/apache2
Diff against target: 75 lines (+27/-0)
5 files modified
.bzrignore (+1/-0)
README.md (+5/-0)
config.yaml (+12/-0)
data/security.template (+6/-0)
hooks/hooks.py (+3/-0)
To merge this branch: bzr merge lp:~hloeung/charms/precise/apache2/ssl-security-options
Reviewer Review Type Date Requested Status
Adam Israel (community) Approve on 2014-12-11
Review Queue (community) automated testing Needs Fixing on 2014-10-09
Charles Butler (community) 2014-09-09 Needs Fixing on 2014-09-26
Review via email: mp+233877@code.launchpad.net

Description of the change

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

To post a comment you must log in.
Review Queue (review-queue) wrote :

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

review: Approve (cbt)
Charles Butler (lazypower) wrote :

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
Review Queue (review-queue) wrote :

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

review: Needs Fixing (automated testing)
Review Queue (review-queue) wrote :

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

review: Needs Fixing (automated testing)
Review Queue (review-queue) wrote :

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

review: Needs Fixing (automated testing)
60. By Haw Loeung on 2014-10-13

[trivial] Merged changes from upstream.

61. By Haw Loeung on 2014-10-15

Disable SSLv3 to mitigate POODLE.

62. By Haw Loeung on 2014-10-16

Refine cipher suites.

Haw Loeung (hloeung) wrote :
Download full text (6.3 KiB)

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

Read more...

Adam Israel (aisrael) wrote :

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
63. By Haw Loeung on 2014-11-20

Document new ssl_protocol, ssl_honor_cipher_order, and ssl_cipher_suite configuration options.

64. By Haw Loeung on 2014-11-20

Ignore virtualenv created by make build/test.

Adam Israel (aisrael) wrote :

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/precise/apache2/.venv/local/lib/python2.7/site-packages/mock.py", line 1201, in patched
    return func(*args, **keywargs)
  File "/home/stone/charms/precise/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/precise/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/precise/apache2/.venv/local/lib/python2.7/site-packages/mock.py", line 1201, in patched
    return func(*args, **keywargs)
  File "/home/stone/charms/precise/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/precise/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
Haw Loeung (hloeung) wrote :

Hi Adam,

Thanks for taking the time to review this but it seems those tests fail in the upstream charm itself, see https://pastebin.canonical.com/122119/ and unrelated to my changes.

I ran a quick bzr blame and seems to be broken since r57?

Thanks,

Haw

Adam Israel (aisrael) wrote :

Hi Haw,

You're absolutely correct. 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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file '.bzrignore'
2--- .bzrignore 2013-10-10 22:47:57 +0000
3+++ .bzrignore 2014-11-20 00:34:46 +0000
4@@ -8,3 +8,4 @@
5 *.key
6 lib/*
7 build/charm-helpers
8+.venv/
9
10=== modified file 'README.md'
11--- README.md 2014-06-18 15:25:07 +0000
12+++ README.md 2014-11-20 00:34:46 +0000
13@@ -261,6 +261,11 @@
14 according to `ssl_certlocation` and `ssl_keylocation` as listed
15 above.
16
17+`ssl_protocol`, `ssl_honor_cipher_order`, and `ssl_cipher_suite` can
18+be used to override SSL/TLS version and the cipher suites supported.
19+These default to what Canonical IS recommends and is using. Before
20+making any changes, please see the Mozilla Security/Server Side TLS.
21+
22 ## `{enable,disable}_modules`
23
24 Space separated list of modules to be enabled or disabled. If a module to
25
26=== modified file 'config.yaml'
27--- config.yaml 2014-01-30 09:11:53 +0000
28+++ config.yaml 2014-11-20 00:34:46 +0000
29@@ -151,6 +151,18 @@
30 base64 encoded chain certificates file. If ssl_cert is
31 specified as SELFSIGNED, this will be ignored.
32 default: ''
33+ ssl_protocol:
34+ type: string
35+ description: SSL Protocols to enable.
36+ default: "ALL -SSLv2 -SSLv3"
37+ ssl_honor_cipher_order:
38+ type: string
39+ description: Enable server cipher suite preference.
40+ default: "On"
41+ ssl_cipher_suite:
42+ type: string
43+ description: List of server cipher suites.
44+ 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"
45 server_tokens:
46 type: string
47 description: Security setting. Set to one of Full OS Minimal Minor Major Prod
48
49=== modified file 'data/security.template'
50--- data/security.template 2014-05-27 14:44:13 +0000
51+++ data/security.template 2014-11-20 00:34:46 +0000
52@@ -50,3 +50,9 @@
53 # Set to one of: On | Off | extended
54 #
55 TraceEnable {{ trace_enabled }}
56+
57+<IfModule mod_ssl.c>
58+ SSLProtocol {{ ssl_protocol }}
59+ SSLHonorCipherOrder {{ ssl_honor_cipher_order }}
60+ SSLCipherSuite {{ ssl_cipher_suite }}
61+</IfModule>
62
63=== modified file 'hooks/hooks.py'
64--- hooks/hooks.py 2014-09-10 17:34:41 +0000
65+++ hooks/hooks.py 2014-11-20 00:34:46 +0000
66@@ -472,6 +472,9 @@
67 'server_tokens': config_data['server_tokens'],
68 'server_signature': config_data['server_signature'],
69 'trace_enabled': config_data['trace_enabled'],
70+ 'ssl_protocol': config_data['ssl_protocol'],
71+ 'ssl_honor_cipher_order': config_data['ssl_honor_cipher_order'],
72+ 'ssl_cipher_suite': config_data['ssl_cipher_suite'],
73 'is_apache24': is_apache24(),
74 }
75 template = \

Subscribers

People subscribed via source and target branches

to all changes: