inejge / ldap3

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

Allow getting the peer certificate as DER #90

Closed dasJ closed 2 years ago

dasJ commented 2 years ago

There is only the DER format available because both libraries only support this format and returning a Vec allows us to have a library-independant way of returning the data.

inejge commented 2 years ago

Thanks for the PR (and sorry about the delay in responding). The motivation behind the proposed functionality is sound, but there are two issues which make the implementation inadequate:

  1. The method is defined on LdapConnAsync (as it must be), but that struct is only briefly available in the async workflow, before being handed over to drive!(), and not available at all in the sync case.
  2. Consequently, the method can't be made available to the sync users, which is not acceptable, since the sync/async methods must have strict parity unless dictated by the low-level nature of I/O.

The only way I see to have this kind of method implemented would involve adding another MPSC channel to LdapConnAsync, having Ldap/LdapConn fire a request containing a oneshot sender to LdapConnAsync's select! loop, and retrieving the result via the corresponding receiver.

Conni2461 commented 2 years ago

Hey @inejge,

thanks for your answer. I am a college of dasJ, who writes a little bit more rust and had a little bit more down time, so i took over. I implemented your proposed solution using channels. I still need to test it against our slapd server but i plan to do that in the next couple of days.

Conni2461 commented 2 years ago

Review would be appreciated, i will test it later against our production slapd.

inejge commented 2 years ago

Thanks, LGTM aside from the timeout issue I've highlighted. When you address that, please squash the commits and force-push the result, which I'm going to merge. I'll probably make a couple of further small stylistic changes, for which I won't bother with additional back-and-forth.

inejge commented 2 years ago

Merged. Heads up, I renemed get_peer_certificate_der to get_peer_certificate, I like it better that way.

Conni2461 commented 2 years ago

Thanks :) also thanks for working on this whole project, its been really useful :D