1286702 - Use BROKER_USE_SSL on Heroku to force TLS for Celery RabbitMQ connections

Status: RESOLVED FIXED

(Tree Management :: Treeherder: Infrastructure, defect, P1)

Description

[Ed Morley [:emorley]]

On Heroku, the value of CLOUDAMQP_URL is set automatically by the CloudAMQP addon we're using, to a value of form:

amqp://USERNAME:PASSWORD@INSTANCE_NAME.rmq.cloudamqp.com/VHOST

However since this is using neither of:
* scheme `amqps://`
* URL param `?ssl=true`

Then I'm pretty sure this isn't using TLS, although the Celery/Kombu docs are pretty dire. Even the CloudAMQP Heroku guide doesn't mention this at all, and its example using Pika doesn't enable TLS (only clear after reading the Pika source - what is it with these crappy docs around TLS in this day and age - the Python Elasticsearch library docs were just as bad).

We should:
1) Pester CloudAMQP to set CLOUDAMQP_URL to something starting with `amqps://`, so we can just use that (and any changes such as password rotation stays in sync)
2) Set an amqps:// URL manually ourselves (but we then have to keep it in sync)
3) Set the BROKER_USE_SSL option for celery, so SSL setting is independent of the URL. Though we'll then need to disable this in Vagrant, since the rabbitmq instance doesn't have a cert.

Though it's not clear if celery/kombu have support for amqps://, so we may have to resort to #3 regardless. I've filed this upstream issue for improving the docs on their side:
https://github.com/celery/kombu/issues/610

See also:
http://kombu.readthedocs.io/en/latest/userguide/connections.html#connection-options
http://docs.celeryproject.org/en/latest/configuration.html#broker-use-ssl
https://devcenter.heroku.com/articles/cloudamqp#use-with-python

Comment 1

[Ed Morley [:emorley]]

s/We should:/We should either:/

Comment 2

[Ed Morley [:emorley]]

http://kombu.readthedocs.io/en/latest/userguide/connections.html#connection-urls
https://www.cloudamqp.com/docs/faq.html#TLS_SSL_support

Comment 3

[Ed Morley [:emorley]]

I've emailed CloudAMQP about adding an option to their control panel, that would make the CLOUDAMQP_URL use 'amqps://'. I'll follow up on this (plus Celery support of amqps:// in bug 1288369.

However for now, let's go with #3, since we'd probably want to do this regardless of the other fixes, if only for defense in depth.

Comment 4

[Ed Morley [:emorley]]

Attachment: Enable TLS for Celery RabbitMQ connections that support it

Comment 5

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/50818d965edd9f59a3b35e7fe44bf1948aa8ceba
Bug 1286702 - Enable TLS for Celery RabbitMQ connections that support it

Celery uses Kombu to connect to the RabbitMQ instance, which defaults to
not enabling SSL, unless the URL scheme is `amqps://` or the query
string contains `ssl=1` / `ssl=true`.

On Heroku we're using CloudAMQP, who don't use either string in their
automatically defined `CLOUDAMQP_URL` environment variable, so we must
set the Celery preference `BROKER_USE_SSL` to ensure TLS is still used:
http://docs.celeryproject.org/en/latest/configuration.html#broker-use-ssl

I've contacted CloudAMQP to encourage them to use `amqps://` in their
URLs, however even if they do switch, using `BROKER_USE_SSL` is a
sensible defence-in-depth measure we should take regardless.

TLS support isn't set up for the rabbitmq servers on SCL3, Travis or in
the Vagrant environment, so `BROKER_USE_SSL` must not be set there. In
the future we may decide it's worth the effort to use self-signed
certificates to add support for TLS to Travis/Vagrant too.

Comment 6

[GitHub Autolander Bot]

Attachment: [treeherder] mozilla:fix-tls-support-check > mozilla:master

Comment 7

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/fc95375ac388bfff1d8f0dd4eaa6017edce3a2b0
Bug 1286702 - Fix hostname extraction in server_supports_tls()

urlparse's `netloc` attribute (which I'd copied from the `SITE_HOSTNAME`
usage elsewhere in settings.py) includes the port number as well as the
hostname, and so was causing the SCL3 hostname check to not match,
meaning TLS was enabled for celery on SCL3 when the rabbitmq instance
there doesn't support it.

https://github.com/mozilla/treeherder/commit/617de68a44fee3496217fd925dbc241590ed9102
Bug 1286702 - Make SITE_HOSTNAME use `hostname` not `netloc`

Since whilst most of the time there will be no port specified (and so
they'll be equivalent), it's really just the hostname we want, so let's
be clear about it.

Comment 8

[Ed Morley [:emorley]]

I've rotated the credentials on the prototype and stage.

However doing so caused some errors in the CloudAMQP logs until the nodes were restarted. Not a problem for this time, but clearly a bug in their password rotation that doesn't restart all services correctly.

Have emailed CloudAMQP support reporting the bug.

Comment 9

[Ed Morley [:emorley]]

s/nodes/CloudAMQP server instances/ (not the dynos)