add TLS support - amqp.DialTLS

[gbolo]

Any plans on adding TLS support via amqp.DialTLS? I do see it mentioned, not sure if your still working on it. I'm very new to golang, I could attempt a PR, but I wouldn't bother if your already working on it.

[mihaitodor]

Hi @gbolo. I left that TODO in there as a reminder, but I haven't managed to get back to it yet. Please feel free to submit a PR if you wish. I don't think I'll have time to tackle it this month :(

[gbolo]

Hi @mihaitodor. Any particular strategy/preference I should follow before attempting this PR?

[mihaitodor]

@gbolo I am caught up in some other project this week, so I'll try to have a look next week in detail, because I'm not at all familiar with setting up TLS for Sensu (we don't use this feature internally). At a high level, the config plumbing is already done here, so I think it might be enough to just call amqp.DialTLS. In oder to do that, it will require a wrapper similar to this one which needs also to be added here. Then it needs to be called in some way here. Then, some of the tests will probably break, like here and here and it would be nice to have some test which makes sure that we hit amqp.DialTLS when the config contains this node. After all this is put in place, sensu-client-go will need to get the new version of this library added to Godeps (which has proven to be a difficult task, especially since you'll have to work on a fork)...

But, maybe the first step would be to get a test setup going similar to that docker-compose environment that you created for this bug. It would be really helpful to be able to have a Ruby client that already works with TLS configured and then just try to swap it with the Go client to test. If you can provide that, I can put in some time to help with the code changes.

[gbolo]

@mihaitodor Thanks for tips! I will get to working on it during free time

[gbolo]

hi @mihaitodor,

So I am testing some code modifications to your HA rabbitmq setup to add tls support. However, I can't seem to get the config to load properly. Can you provide some insight?

I am building: upfluence/sensu-client-go

then loading this config (with no env variables set) with -c which contains:

{
  "client": {
    "name": "foo",
    "address": "192.168.1.1"
  },
  "rabbitmq": [
    {
      "host": "127.0.0.1",
      "port": 5671,
      "vhost": "/",
      "user": "guest1",
      "password": "guest1",
      "ssl": {
        "cert_chain_file": "/home/gbolo/syncthing/git/github/gbolo/dockerfiles/sensu/test/tls/client_rsa-x509.pem",
        "private_key_file": "/home/gbolo/syncthing/git/github/gbolo/dockerfiles/sensu/test/tls/client_rsa-key.pem"
      }
    }
  ]
}

However, seems like the client just ignores it and uses some defaults:

./sensu-client-go -c sensu/testdata/client-tls.json 
[N 170307 13:48:35 rabbitmq.go:122] Trying to connect to URI: amqp://guest:guest@localhost:5672/%2F

Am I missing something?

[mihaitodor]

@gbolo Indeed, improving the documentation is still on my TODO list :(

Basically, you need to modify sensu-client.go and use something like this:

sensuConfig, err := sensu.NewConfigFromFlagSet(sensu.ExtractFlags())

if err != nil {
	log.Fatalf("Error reading Sensu config: %s", err)
}

haConfig, err := sensuConfig.RabbitMQHAConfig()
if err != nil {
	log.Fatalf("Error reading RabbitMQ HA config: %s", err)
}

transport := rabbitmq.NewRabbitMQHATransport(haConfig)

sensuClient := sensu.NewClient(transport, sensuConfig)

err = sensuClient.Start()

if err != nil {
	log.Fatalf("Failed to start the Sensu client: %s", err)
}

[gbolo]

Hi @mihaitodor,
Thanks for tips. I managed to get TLS working using amqp.DialConfig instead of amqp.DialTLS, since this function just calls DialConfig anyways. It's a small hack right now, and only works along side your rabbitmqHA config (which I prefer over the default). I will prepare the test environment for you play with.

[mihaitodor]

Yeah, Dial is the same (a simple wrapper for DialConfig), but I'd rather use the library functions as much as possible, since they might change in the future. Also, amqp.defaultHeartbeat is private (for reasons that escape me), so I wanted to avoid having to mirror that value in this code.

[gbolo]

Hi @mihaitodor,

So i finished adding TLS support and tested it with various configs. I changed the default dialer to use ampq.DialTLS and introduced a function tlsConfig to return the required second param for ampq.DialTLS. I also modified GetURI to detected if TLS should be used and return a amqps:// scheme. Please see this commit: df83493. I stray a bit from the official sensu client by forcing the user to provide a CA cert as well, instead of just the chain file.

Please note that I have removed all GoDeps from the code and am pointing to the current amqp go lib. I am not sure if this code meets your standards, but I kind of urgently need TLS support so that's why I forked it out like this. If you are interested in this code I could put together a PR. Also I will provide the test environment for you tomorrow (its already done, just got to commit it). Also, thank you very much for your help and the HA config PR you submitted!

[gbolo]

Hi @mihaitodor,

TLS can be tested as follows:

## start cluster
git clone https://github.com/gbolo/dockerfiles.git
cd dockerfiles/sensu
docker-compose -f docker-compose-tls.yml up -d

## view logs
docker ps
docker logs sensu_sensu-client-go_1 -f

## exposed ports on local machine
5671 - rabbitmq TLS endpoint
15672 - rabbitmq management (guest/guest)
3000 - uchiwa

## bring down cluster
docker-compose -f docker-compose-tls.yml down

[mihaitodor]

Hi @gbolo,

Thanks for sharing your updates! I will try to allocate some time to look at it in detail. Your code looks promising, but it needs some adjustments and testing in order to fit nicely into this library. Having this docker-compose environment setup helps a lot!