search

inejge / ldap3

A pure-Rust LDAP library using the Tokio stack
Apache License 2.0
220 stars 38 forks source link

Make the dependency to native-tls optional #6

Closed fknorr closed 7 years ago

fknorr commented 7 years ago

This pull request adds a feature tls (enabled by default) which gates the connect_ssl() functions. By building without this feature, one can avoid dependency to native TLS libraries caused by native-tls which may not be statically linkable (or simply unnecessary if TLS support is not required).

inejge commented 7 years ago

Thank you for the PR! It seems perfectly correct technically; however, I'd like to better understand the use cases for LDAP sans TLS. IMO any LDAP connection that leaves the local host should be encrypted, and even for a minimal, local-only library I'd actually prefer Unix domain sockets over regular TCP, because of SO_PEERCRED.

I know that using OpenSSL with musl and static linking (if that's the principal motive behind this PR) is a hassle, but there are at least two approaches that appear workable: Docker images and cross-compiling. I admit that I haven't tried either of them, and I understand that they might not work for you, which is why I'm interested in the details.

fknorr commented 7 years ago

Just as you guessed, the use case is connecting to a local server. Whether I use Unix domain sockets or unencrypted TCP, I do not need TLS support.

I'm actually deploying binaries to a system where I cannot guarantee that libssl and libcrypto are available, so I have to manually deploy these binaries as well without needing them. I don't need a fully-static link, libc and the like are acceptable. So this is not a deal-breaker at all, but rather an inconvenience and a contribution to the enormous amounts of dependencies I cannot avoid in my application.

I've just spotted a small mistake in my changes: Line src/conn.rs:395 also needs to be prefixed with #[cfg(feature = "tls")] as to avoid running into an unimplemented!() in line 423. Sorry about that.

inejge commented 7 years ago

Line src/conn.rs:395 also needs to be prefixed with #[cfg(feature = "tls")]

Could you please push a commit with this change? I'm inclined to accept the PR, but I still need to think it over, including the best way to document it etc. (Maybe I'll combine it with compiling out Unix domain sockets to get truly minimal connectivity.) Since this is an enhancement, I wouldn't publish it in 0.4.4; it would have to wait for 0.5, but meanwhile it could be used from git master.

fknorr commented 7 years ago

I pushed a commit which fixes this issue.

inejge commented 7 years ago

@fknorr Merged, thanks again! I'll probably re-arrange the features a bit when I find the time.

fknorr commented 7 years ago

It may even be possible to switch TLS implementations (e.g. between native-tls and rustls, which does not have external dependencies) by using different features, but I don't have any details on that.