search

Antti / rust-amqp

AMQP client in pure rust. Corresponds to rabbitmq spec.
MIT License
247 stars 45 forks source link

amqp: add optional TLS support #13

Closed lucab closed 9 years ago

lucab commented 9 years ago

This commit adds optional TLS support, using OpenSSL. It is enabled by default and pull in an additional dependency on the openssl crate. However, if not needed this can be disabled with cargo --no-default-features. It has been successfully tested against a third-party RabbitMQ (3.5.3) instance.

As I was not sure if the lack of TLS support, explicitly mentioned in the README, was a design decision, I implemented this part as an optional feature/dependency, enabled by default. This may also allow you to swap in a different TLS library later. Let me know if this is ok for you, or if you prefer to directly wire it.

Antti commented 9 years ago

The lack of TLS is not a design decision, it's just lack of time/was not needed at the moment.

This PR looks very good so far.

Antti commented 9 years ago

We should document in the Readme, that you can disable TLS support with cargo --no-default-features

lucab commented 9 years ago

The lack of TLS is not a design decision, it's just lack of time/was not needed at the moment.

I see. Do you prefer to keep this as an optional feature or should I just remove all the #cfg[tls] stuff?

Antti commented 9 years ago

Let's keep the cfg for now until we'll be certain on the TLS library. I prefer the native rust one instead of openssl bindings.

lucab commented 9 years ago

Amended: added doc and changed boolean assignment style.

I prefer the native rust one instead of openssl bindings.

Are you referring to a specific/existing one, or are you talking about a future / not-yet-there TLS library?

Antti commented 9 years ago

I haven't tried existing ones yet, but they seem promising.