majek / puka

Puka - the opinionated RabbitMQ client
https://github.com/majek/puka
Other
182 stars 34 forks source link

amqps: scheme (AMQP over SSL) #43

Closed istalker2 closed 10 years ago

majek commented 11 years ago

Good job! Of course, adding an SSL adds a question of client-supplied certificates and checking if the server one is valid.

I'm not sure if I'm up to merging this yet, but still - great job and thanks for the contribution. I may merge it in the future. (and it definitely will be useful for some Puka users!)

istalker2 commented 11 years ago

This is similar to how SSL support implemented in pika. It is no problem passing additional SSL options that would allow using client certificates etc. The reason i haven't done it is because AFAIK amqp:// URL is the only way to initialize connection and there is no way to pass SSL options in URL. I could make explicit Connection constructor that would allow passing optional SSL parameters if that would remove your doubts about merging changes I made

majek commented 11 years ago

Here it is: http://www.rabbitmq.com/uri-spec.html

It doesn't specify certificates, so I guess your proposal is enough. Can we make sure that by default the certificate is checked against the available certificate authorities (CA's)?

istalker2 commented 11 years ago

According to http://docs.python.org/2/library/ssl.html#ssl.wrap_socket
The parameter cert_reqs specifies whether a certificate is required from the other side of the connection, and whether it will be validated if provided. It must be one of the three values CERT_NONE (certificates ignored), CERT_OPTIONAL (not required, but validated if provided), or CERT_REQUIRED (required and validated). If the value of this parameter is not CERT_NONE, then the ca_certs parameter must point to a file of CA certificates.

So using certificate validation as a default would require supplying of CA certificates file which would make amqps:// scheme useless as CA certificates files cannot be passed as a part of URL

I believe it is better to stick with CERT_NONE as it is also the default for pika and other AMQP libraries and it it better to have consistency with them

istalker2 commented 11 years ago

I you don't mind I'm going to add ability to pass additional (optional) SSL parameters alongside amqps:// URL but use Python's SSL defaults by default and then resubmit my pull request.

majek commented 11 years ago

Yeah, I'd opt for CERT_REQUIRED as default, and some easy option maybe like "amqps://blah/?no-check-certificate=true" as a fallback. Or, additional parameters to the constructor if you prefer that. What I do care about - by default let's check the certificate against localy known CA's.

istalker2 commented 11 years ago

I believe I've my last addresses all your suggestions. Is is good enough now to be merged into something like 0.0.7?