Merge lp:~tal-it-innovation/charms/precise/rabbitmq-server/ssl-support into lp:charms/rabbitmq-server

[Thomas Leonard]

This branch adds support for enabling Rabbit's SSL support. Instructions are in the README.

[Clint Byrum (clint-fewbar)]

Hi Thomas. This is a great change. Needs a few fixups:

+exec 3> rabbitmq.config.new

This is a predictable temp file. While the dir is not normally world-writable, its best not to tempt fate with this type of code. If an admin or a later release of Ubuntu were to add group-write perms to the dir, then an attacker could use this predictability to overwrite files as root.

new_config=`mktemp /etc/rabbitmq/.rabbitmq.config.XXXXXX`
exec 3> $new_config

and then later of course you need to do your rename using $new_config.

Also I think there's something missing here (though its not a blocker to merge the change) which is to add an amqp-ssl relation or some kind of indicator that SSL is available to the existing "rabbitmq" interface relation.

review: Needs Fixing

[Clint Byrum (clint-fewbar)]

Oh I forgot to mention, after fixing these things, please change the status of the merge proposal back to 'Needs Review'.

[Thomas Leonard (tal-it-innovation)]

I don't think that will allow untrusted rabbit admins. If you grant some group (e.g. rabbitadm) write access to the config directory:

cd /etc/rabbitmq
sudo chgrp rabbitadm .
sudo chmod g+sw
[ and change juju umask to make new files group writable ]

then members of the group can cause a file called .rabbitmq.config.XXXXXX to be written to the root-owned directory of their choice with:

ln -s /some/path.d /etc/rabbitmq/rabbitmq.config

They can then edit that file as they please.

("run-parts" ignores files whose names contain dots, which limits the problem a bit though)

Auditing all Juju charms to make sure they are safe when modifying configuration directories that are under the control of attackers seems infeasibly-hard. I don't know what the right fix is, though. If there were a rabbitadm group already then we could run the juju hooks as that user, which would solve the problem. The FreeBSD *at system calls (openat, renameat, etc) don't follow symlinks to locations above the given FD directory (after calling cap_enter(2)) which would be handy for limiting changes to be within the /etc/rabbitmq directory, but Ubuntu doesn't support that yet.

(hopefully https://bugs.launchpad.net/juju/+bug/728905 will clear things up a bit though)

Anyway, I'll make your suggested change. Thanks,

[Clint Byrum (clint-fewbar)]

Indeed having the .new file in /etc/rabbitmq helps a lot. Perhaps I am being a bit over-zealous with requiring using mktemp in a root-owned dir. However, I'd like to think if somebody copies your charm bits, they will be less likely to expose themselves. The official charms are as much examples for others to follow as they are whole solutions to be used.

Also we don't necessarily need to audit every charm, we just need to catch problems like these on review.

[Clint Byrum (clint-fewbar)]

Alright, w/ mktemp looks good.

Now.. about putting the SSL key in config settings. I was thinking about this and it may not be the best practice given Juju's current design. This data will be in ZooKeeper, readable by many hosts and transmitted in clear text inside the environment's network (we use SSH between the client and ZK, but agents just speak to ZK directly). It would probably be better to allow and document manual copying of the SSL key using juju scp if the user wants higher security. Given juju's current security bug list[1] though, I don't think this is a blocker. +1

[1] https://bugs.launchpad.net/juju/+bugs?field.tag=security

review: Approve

=== added file 'README'
--- README	1970-01-01 00:00:00 +0000
+++ README	2012-09-06 13:22:19 +0000
@@ -0,0 +1,17 @@
+Configuring SSL
+---------------
+Generate an unencrypted RSA private key for the servers and a certificate:
+
+  openssl genrsa -out rabbit-server-privkey.pem 2048
+
+Get an X.509 certificate. This can be self-signed, for example:
+
+  openssl req -batch -new -x509 -key rabbit-server-privkey.pem -out rabbit-server-cert.pem -days 10000
+
+Deploy the service:
+
+  juju deploy rabbitmq-server rabbit
+
+Enable SSL, passing in the key and certificate as configuration settings:
+
+  juju set rabbit ssl_enabled=True ssl_key="`cat rabbit-server-privkey.pem`" ssl_cert="`cat rabbit-server-cert.pem`"

=== added file 'config.yaml'
--- config.yaml	1970-01-01 00:00:00 +0000
+++ config.yaml	2012-09-06 13:22:19 +0000
@@ -0,0 +1,15 @@
+options:
+  ssl_enabled:
+    type: boolean
+    default: False
+    description: enable SSL
+  ssl_port:
+    type: int
+    default: 5673
+    description: SSL port
+  ssl_key:
+    type: string
+    description: private unencrypted key in PEM format (starts "-----BEGIN RSA PRIVATE KEY-----")
+  ssl_cert:
+    type: string
+    description: X.509 certificate in PEM format (starts "-----BEGIN CERTIFICATE-----")

=== added file 'hooks/config-changed'
--- hooks/config-changed	1970-01-01 00:00:00 +0000
+++ hooks/config-changed	2012-09-06 13:22:19 +0000
@@ -0,0 +1,59 @@
+#!/bin/bash
+set -eu
+
+juju-log "rabbitmq-server: Firing config hook"
+
+ssl_enabled=`config-get ssl_enabled`
+
+cd /etc/rabbitmq
+
+new_config=`mktemp /etc/rabbitmq/.rabbitmq.config.XXXXXX`
+chgrp rabbitmq "$new_config"
+chmod g+r "$new_config"
+exec 3> "$new_config"
+
+cat >&3 <<EOF
+[
+    {rabbit, [
+EOF
+
+ssl_key_file=/etc/rabbitmq/rabbit-server-privkey.pem
+ssl_cert_file=/etc/rabbitmq/rabbit-server-cert.pem
+
+if [ "$ssl_enabled" == "True" ]; then
+	umask 027
+	config-get ssl_key > "$ssl_key_file"
+	config-get ssl_cert > "$ssl_cert_file"
+	chgrp rabbitmq "$ssl_key_file" "$ssl_cert_file"
+	if [ ! -s "$ssl_key_file" ]; then
+		juju-log "ssl_key not set - can't configure SSL"
+		exit 0
+	fi
+	if [ ! -s "$ssl_cert_file" ]; then
+		juju-log "ssl_cert not set - can't configure SSL"
+		exit 0
+	fi
+	cat >&3 <<EOF
+	{ssl_listeners, [`config-get ssl_port`]},
+	{ssl_options, [
+		{certfile,"$ssl_cert_file"},
+		{keyfile,"$ssl_key_file"}
+	]},
+EOF
+fi
+
+cat >&3 <<EOF
+        {tcp_listeners, [5672]}
+    ]}
+].
+EOF
+
+exec 3>&-
+
+if [ -f rabbitmq.config ]; then
+	mv rabbitmq.config{,.bak}
+fi
+
+mv "$new_config" rabbitmq.config
+
+/etc/init.d/rabbitmq-server restart

=== modified file 'revision'
--- revision	2012-04-14 05:42:17 +0000
+++ revision	2012-09-06 13:22:19 +0000
@@ -1,1 +1,1 @@
-31
+34