inejge / ldap3

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

"Broken pipe" instead of "TLS authentication error" #17

Closed golddranks closed 7 years ago

golddranks commented 7 years ago

When I replace the certificate with a wrong one, I'd expect to get a "TLS authentication error" or something similar. Instead, I get just "broken pipe", and not on the attempt to connect to the LDAP server, but on the first attempt to communicate with it.

I'm doing something like this:

    let cert = Certificate::from_der(LDAP_CERT)?;

    let mut tls_connector = TlsConnector::builder()?;
    tls_connector.add_root_certificate(cert)?;
    let tls_connector = tls_connector.build()?;

    let conn = LdapConnBuilder::<LdapConn>::new()
        .with_tls_connector(tls_connector)
        .connect(&*LDAP_URL)?;

    let res = conn.simple_bind(&bind_dm, &login_info.password)?;

It doesn't explicitly fail when connecting, but on the simple_bind. The error looks like this, debug-printed:

Error { repr: Custom(Custom { kind: BrokenPipe, error: StringError("broken pipe") }) }

I just spent hours of trying to pinpoint where this bug originates from, but without much success – there's layers upon layers of Tokio abstractions :(

golddranks commented 7 years ago

As I understand the gist of the code after reading it, it connects to the LDAP server using TcpClient from tokio_proto. The connect method returns a future that stands for a "service object" (here ClientService<TcpStream, TlsClient<LdapProto>>, where ClientService is from tokio_proto and TlsClient is tokio_tls::proto::Client, which for some reason doesn't show up in the docs of tokio_tls.). The "service object" represents the LDAP server. Then one can interact with the service object.

If I get the purpose of the TlsClient correctly, it's there to "wrap" another protocol, here, "LdapProto", so that the protocol doesn't need to care about the transport and if it's encrypted or not.

It seems, from the API that the thing should fail already when connecting the ClientService<TcpStream, TlsClient<LdapProto>>, because the TLS handshake is done when the TCP stream is started, not for every "interaction" with the service.

But on the other hand, from glance, ClientService seems to be designed in a way that fails on connect if the "transport", or here, TcpStream, fails. That is, it might be that TlsClient has no say about the matter on the connection stage – so it might be a wrong abstraction! @carllerche and @alexcrichton , sorry, I know that you're super busy, but has this viewpoint taken into account in the design of tokio_tls::proto::Client? Or is the code in this crate ab/misusing it?

golddranks commented 7 years ago

Oh, wait, is the purpose of ClientService to spawn multiple clients that all connect to the LDAP server, and share the transport?

golddranks commented 7 years ago

Btw. I'm trying to desperately solve this bug, since I'm unable to connect to the LDAP server from a specific host, but the unhelpful "broken pipe" error message masks the real reason why the connection is failing. I figured that the same error also manifests if the certificate is the wrong one on a host that is able to connect, so looking into it might solve the problem.

inejge commented 7 years ago

See this recent issue. It's possible that the error is swallowed in tokio-proto, and all you get back when trying to use the connection is "broken pipe". Use the settings in the environment to activate detailed tracing.

golddranks commented 7 years ago

It seems to be exactly the same error as in #14 – which I already knew because I deliberately used a wrong cert :P It's good that the TODO comment in the Tokio code reveals that they are aware of that bug. I wonder if there is an open issue of BindClient code eating errors – I wouldn't find any after a quick search.

Another thing that bothers me – not maybe a bug, but an architectural choice – is that the TLS connection error is noticed only when the connection is attempted to be used. I'd prefer it to fail early if it were to fail.

At least I can now use log traces to debug the real issue. Thanks!

inejge commented 7 years ago

As this looks like a situation which could occur frequently, I'll make a note in the documentation. I believe that trying to fix this on the Tokio side should wait until the new structure of the stack emerges.

golddranks commented 7 years ago

Btw. my problem, in the end, was that some time ago, I started using the new, OpenSSL 1.1 version in Linux builds. It is much stricter about the CA certificates being used, and it didn't accept it (because of a mistaken X509v3 Basic Constraints: CA:FALSE). This didn't show up in the macOS environment which is my primary development environment, because the native-tls crate chooses entirely different library for Mac.

(Sorry for the offtopic :D)

I think that even if the broken pipe error isn't this crate's fault, it would be helpful to instruct the users to enable logging to catch the real error as tokio_proto logs it, since I'd imagine the TLS connection failing on setup is not exactly a rare thing when developing and debugging.

inejge commented 7 years ago

Documented in the README (d770a8311985aa6f979c8226db9ff256b61220ca). Since it's immediately visible in the repository, I'll close this. (I know, crates.io now renders READMEs and won't pick it up automatically, but I expect that most users still land on the GH page.)

golddranks commented 7 years ago

Thanks!