SDAP: check that connection is open before BIND is perfomed

[Pavel Reichl]

Hello,

please see attached patches and read commit messages for more details.

As I do not have a Tivoli server handy, customer was so nice that he 
verified that patches are fixing the problem for him.

Thanks!

[Jakub Hrozek]

On Thu, Sep 18, 2014 at 10:22:31AM +0200, Pavel Reichl wrote:
> Hello,
>
> please see attached patches and read commit messages for more details.
>
> As I do not have a Tivoli server handy, customer was so nice that he
> verified that patches are fixing the problem for him.
>
> Thanks!

I admit I only uran some sanity tests against a regular LDAP server.
> From fe330d3670df147cc03e433a12dfe243f81a2b17 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Tue, 16 Sep 2014 09:42:06 +0100
> Subject: [PATCH 1/2] SDAP: move deciding of tls usage into new function
>
> Separate code for deciding tls usage from sdap_cli_connect_send() to new
> function decide_tls_usage().
>
> src/providers/ldap/sdap_async_connection.c | 57 +++++++++++++++++++-----------
>  1 file changed, 37 insertions(+), 20 deletions(-)
>
> diff --git a/src/providers/ldap/sdap_async_connection.c b/src/providers/ldap/sdap_async_connection.c
> index a1f78c025e03495a2ac4bcc7c5b05ac3a48ddf4f..8f8d67a9b43bb82c2f0e776f36af886ff86c65f8 100644
> --- a/src/providers/ldap/sdap_async_connection.c
> +++ b/src/providers/ldap/sdap_async_connection.c
> @@ -1412,6 +1412,37 @@ static void sdap_cli_auth_step(struct tevent_req *req);
>  static void sdap_cli_auth_done(struct tevent_req *subreq);
>  static void sdap_cli_rootdse_auth_done(struct tevent_req *subreq);
>
> +static errno_t
> +decide_tls_usage(enum connect_tls force_tls, struct dp_option *basic,
>             const char *uri, bool *_use_tls)

Any particular reason why this function doesn't return bool right away?
Please note returning int && bool* is not wrong, I'm just curious.
> +{
> bool use_tls = true;
> switch (force_tls) {
> case CON_TLS_DFL:
>    use_tls = dp_opt_get_bool(basic, SDAP_ID_TLS);
>    break;
> case CON_TLS_ON:
>    use_tls = true;
>    break;
> case CON_TLS_OFF:
>    use_tls = false;
>    break;
> default:
>    return EINVAL;
>    break;
> }
> if (use_tls && sdap_is_secure_uri(uri)) {
>    DEBUG(SSSDBG_TRACE_INTERNAL,
>          "[%s] is a secure channel. No need to run START_TLS\n", uri);
>    use_tls = false;
> }
> *_use_tls = use_tls;
> return EOK;
>
> +}
>
> struct tevent_req *sdap_cli_connect_send(TALLOC_CTX *memctx,
>                                           struct tevent_context *ev,
>                                           struct sdap_options *opts,
> @@ -1475,21 +1506,14 @@ static void sdap_cli_resolve_done(struct tevent_req *subreq)
>      struct sdap_cli_connect_state *state = tevent_req_data(req,
>                                               struct sdap_cli_connect_state);
>      int ret;
> bool use_tls = true;
> bool use_tls;
> switch (state->force_tls) {
> case CON_TLS_DFL:
>    use_tls = dp_opt_get_bool(state->opts->basic, SDAP_ID_TLS);
>    break;
> case CON_TLS_ON:
>    use_tls = true;
>    break;
> case CON_TLS_OFF:
>    use_tls = false;
>    break;
> default:
> ret = decide_tls_usage(state->force_tls, state->opts->basic,
>                       state->service->uri, &use_tls);
> if (ret != EOK) {

I think we can move the decide_tls_usage() after the
be_resolve_server_recv unless you see a reason otherwise. This would at
least give us the possibility to free subreq in case
be_resolve_server_recv() errors out. I'm not sure why we have the
use_tls assignment first in master, then be_resolve_server_recv() and
then we act on use_tls. I suspect we just changed the code gradually
over time...
>      tevent_req_error(req, EINVAL);
>    break;
>    return;
>
> }
>
> ret = be_resolve_server_recv(subreq, &state->srv);
>
> @@ -1502,13 +1526,6 @@ static void sdap_cli_resolve_done(struct tevent_req *subreq)
>          return;
>      }
> if (use_tls && sdap_is_secure_uri(state->service->uri)) {
>    DEBUG(SSSDBG_TRACE_INTERNAL,
>          "[%s] is a secure channel. No need to run START_TLS\n",
>              state->service->uri);
>    use_tls = false;
> }
> subreq = sdap_connect_send(state, state->ev, state->opts,
>                               state->service->uri,
>                               state->service->sockaddr,
>
> -- 
> 1.9.3
> From b8419bbf7ad5a2beeb6650711c83a63fa5458bc7 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Mon, 15 Sep 2014 14:13:21 +0100

Only some real nitpicks here...
> Subject: [PATCH 2/2] SDAP: check that connection is open before BIND
>
> Tivoli server does not return an empty response when being asked for the
> rootDSE data but an error. In this case the rootDSE lookup in SSSD will
> terminate the connection to the server and return a error. But since
> errors except timeouts are ignored SSSD will try to continue with the
> bind, but since the connection is already terminated this will fail as
> well. And this will terminate the whole operation.
>
> Make sure the connection is open before performing BIND operation.

I would prefer to avoid using all-capitals BIND in the subject and
commit message. It just suggests BIND the nameserver to me..
> Resolves:
> https://fedorahosted.org/sssd/ticket/2435
>
> src/providers/ldap/sdap_async_connection.c | 87 ++++++++++++++++++++++++++++++
>  1 file changed, 87 insertions(+)
>
> diff --git a/src/providers/ldap/sdap_async_connection.c b/src/providers/ldap/sdap_async_connection.c
> index 8f8d67a9b43bb82c2f0e776f36af886ff86c65f8..9951e8b62e49eae310893a3f0844cf4ee2027e83 100644
> --- a/src/providers/ldap/sdap_async_connection.c
> +++ b/src/providers/ldap/sdap_async_connection.c
> @@ -1398,6 +1398,7 @@ struct sdap_cli_connect_state {
>  enum connect_tls force_tls;
>  bool do_auth;
> bool use_tls;

I wonder if it was more consistent to use state->use_tls instead of a
local variable in sdap_cli_resolve_done() as well.
> };
>
> static int sdap_cli_resolve_next(struct tevent_req *req);
> @@ -1410,6 +1411,8 @@ static void sdap_cli_kinit_step(struct tevent_req *req);
>  static void sdap_cli_kinit_done(struct tevent_req *subreq);
>  static void sdap_cli_auth_step(struct tevent_req *req);
>  static void sdap_cli_auth_done(struct tevent_req *subreq);
> +static errno_t sdap_cli_auth_reconnect(struct tevent_req *subreq);
> +static void sdap_cli_auth_reconnect_done(struct tevent_req *subreq);
>  static void sdap_cli_rootdse_auth_done(struct tevent_req *subreq);
>
> static errno_t
> @@ -1781,6 +1784,16 @@ static void sdap_cli_auth_step(struct tevent_req *req)
>      struct sss_auth_token *authtok;
>      errno_t ret;
> /* It's possible that connection was terminated by server (e.g. #2435),
>   to overcome this try to connect again. */
> if (state->sh == NULL || !state->sh->connected) {

Can you put a DEBUG message here?
>    ret = sdap_cli_auth_reconnect(req);
>    if (ret != EOK) {

And maybe also here..
>        tevent_req_error(req, ret);
>    }
>    return;
> }
> /* Set the LDAP expiration time
>     * If SASL has already set it, use the sooner of the two
>     */
>
> @@ -1842,6 +1855,80 @@ static void sdap_cli_auth_step(struct tevent_req *req)
>      tevent_req_set_callback(subreq, sdap_cli_auth_done, req);
>  }
>
> +static errno_t sdap_cli_auth_reconnect(struct tevent_req *req)
> +{
> struct sdap_cli_connect_state *state;
> struct tevent_req *subreq;
> errno_t ret;
> state = tevent_req_data(req, struct sdap_cli_connect_state);
> ret = decide_tls_usage(state->force_tls, state->opts->basic,
>                       state->service->uri, &state->use_tls);
> if (ret != EOK) {
>    goto done;
> }
> subreq = sdap_connect_send(state, state->ev, state->opts,
>                           state->service->uri,
>                           state->service->sockaddr,
>                           state->use_tls);
> if (subreq == NULL) {
>    ret = ENOMEM;
>    goto done;
> }
> tevent_req_set_callback(subreq, sdap_cli_auth_reconnect_done, req);
> ret = EOK;
>
> +done:
> return ret;
>
> +}
>
> +static void sdap_cli_auth_reconnect_done(struct tevent_req *subreq)
> +{
> struct sdap_cli_connect_state *state;
> struct tevent_req *req;
> errno_t ret;
> req = tevent_req_callback_data(subreq, struct tevent_req);
> state = tevent_req_data(req, struct sdap_cli_connect_state);
> talloc_zfree(state->sh);

Please put a newline here. Normally freeing state->sh must be done with
caution as it terminates an LDAP connection, but here I guess it's OK
since we're trying to establish a new connection..
> ret = sdap_connect_recv(subreq, state, &state->sh);
> talloc_zfree(subreq);
> if (ret != EOK) {
>    goto done;
> }
> /* if TLS was used, the sdap handle is already marked as connected */
> if (!state->use_tls) {
>    /* we need to mark handle as connected to allow anonymous bind */
>    ret = sdap_set_connected(state->sh, state->ev);
>    if (ret != EOK) {
>        DEBUG(SSSDBG_CRIT_FAILURE, "sdap_set_connected() failed.\n");
>        goto done;
>    }
> }
> /* End request if reconnecting failed to avoid endless loop */
> if (state->sh == NULL || !state->sh->connected) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "Failed to reconnect.\n");
>    ret = EIO;
>    goto done;
> } else {

We don't need the else statement here. It's more readable to do:

if (failure) {
        fail();
    }
    success();

Rather than:
    if (failure) {
        fail();
    } else {
        success();
    }

as you can still keep your eyes on the outermost indendation level only.
>    sdap_cli_auth_step(req);
> }
> ret = EOK;
>
> +done:
> if (ret != EOK) {
>    tevent_req_error(req, ret);
> }
>
> +}
>
> static void sdap_cli_auth_done(struct tevent_req *subreq)
>  {
>      struct tevent_req *req = tevent_req_callback_data(subreq,
> -- 
> 1.9.3

Thank you, these we mostly nitpicks, so I guess we can push the patches
soon!

[Pavel Reichl]

On 09/23/2014 11:01 PM, Jakub Hrozek wrote:
> On Thu, Sep 18, 2014 at 10:22:31AM +0200, Pavel Reichl wrote:
> > Hello,
> >
> > please see attached patches and read commit messages for more details.
> >
> > As I do not have a Tivoli server handy, customer was so nice that he
> > verified that patches are fixing the problem for him.
> >
> > Thanks!
>
> I admit I only uran some sanity tests against a regular LDAP server.
> > From fe330d3670df147cc03e433a12dfe243f81a2b17 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Tue, 16 Sep 2014 09:42:06 +0100
> > Subject: [PATCH 1/2] SDAP: move deciding of tls usage into new function
> >
> > Separate code for deciding tls usage from sdap_cli_connect_send() to new
> > function decide_tls_usage().
> >
> > src/providers/ldap/sdap_async_connection.c | 57 +++++++++++++++++++-----------
> >   1 file changed, 37 insertions(+), 20 deletions(-)
> >
> > diff --git a/src/providers/ldap/sdap_async_connection.c b/src/providers/ldap/sdap_async_connection.c
> > index a1f78c025e03495a2ac4bcc7c5b05ac3a48ddf4f..8f8d67a9b43bb82c2f0e776f36af886ff86c65f8 100644
> > --- a/src/providers/ldap/sdap_async_connection.c
> > +++ b/src/providers/ldap/sdap_async_connection.c
> > @@ -1412,6 +1412,37 @@ static void sdap_cli_auth_step(struct tevent_req *req);
> >   static void sdap_cli_auth_done(struct tevent_req *subreq);
> >   static void sdap_cli_rootdse_auth_done(struct tevent_req *subreq);
> >
> > +static errno_t
> > +decide_tls_usage(enum connect_tls force_tls, struct dp_option *basic,
> >             const char *uri, bool *_use_tls)
>
> Any particular reason why this function doesn't return bool right away?
> Please note returning int && bool* is not wrong, I'm just curious.
> > +{
> > bool use_tls = true;
> > switch (force_tls) {
> > case CON_TLS_DFL:
> >    use_tls = dp_opt_get_bool(basic, SDAP_ID_TLS);
> >    break;
> > case CON_TLS_ON:
> >    use_tls = true;
> >    break;
> > case CON_TLS_OFF:
> >    use_tls = false;
> >    break;
> > default:
> >    return EINVAL;

Because of this, I don't know which value should I store into the bool 
variable in this case. I find it easier this way and more consistent 
with the original code I refactored.
> >    break;
> > }
> > if (use_tls && sdap_is_secure_uri(uri)) {
> >    DEBUG(SSSDBG_TRACE_INTERNAL,
> >          "[%s] is a secure channel. No need to run START_TLS\n", uri);
> >    use_tls = false;
> > }
> > *_use_tls = use_tls;
> > return EOK;
> >
> > +}
> > struct tevent_req *sdap_cli_connect_send(TALLOC_CTX *memctx,
> >                                          struct tevent_context *ev,
> >                                          struct sdap_options *opts,
> >
> > @@ -1475,21 +1506,14 @@ static void sdap_cli_resolve_done(struct tevent_req *subreq)
> >       struct sdap_cli_connect_state *state = tevent_req_data(req,
> >                                                struct sdap_cli_connect_state);
> >       int ret;
> > bool use_tls = true;
> > bool use_tls;
> > switch (state->force_tls) {
> > case CON_TLS_DFL:
> >    use_tls = dp_opt_get_bool(state->opts->basic, SDAP_ID_TLS);
> >    break;
> > case CON_TLS_ON:
> >    use_tls = true;
> >    break;
> > case CON_TLS_OFF:
> >    use_tls = false;
> >    break;
> > default:
> > ret = decide_tls_usage(state->force_tls, state->opts->basic,
> >                       state->service->uri, &use_tls);
> > if (ret != EOK) {
>
> I think we can move the decide_tls_usage() after the
> be_resolve_server_recv unless you see a reason otherwise. This would at
> least give us the possibility to free subreq in case
> be_resolve_server_recv() errors out. I'm not sure why we have the
> use_tls assignment first in master, then be_resolve_server_recv() and
> then we act on use_tls. I suspect we just changed the code gradually
> over time...

OK, fixed.
> >       tevent_req_error(req, EINVAL);
> >    break;
> >    return;
> > }
> >
> > ret = be_resolve_server_recv(subreq, &state->srv);
> >
> > @@ -1502,13 +1526,6 @@ static void sdap_cli_resolve_done(struct tevent_req *subreq)
> >           return;
> >       }
> > if (use_tls && sdap_is_secure_uri(state->service->uri)) {
> >    DEBUG(SSSDBG_TRACE_INTERNAL,
> >          "[%s] is a secure channel. No need to run START_TLS\n",
> >              state->service->uri);
> >    use_tls = false;
> > }
> > subreq = sdap_connect_send(state, state->ev, state->opts,
> >                            state->service->uri,
> >                            state->service->sockaddr,
> >
> > -- 
> > 1.9.3
> >
> > From b8419bbf7ad5a2beeb6650711c83a63fa5458bc7 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Mon, 15 Sep 2014 14:13:21 +0100
>
> Only some real nitpicks here...
> > Subject: [PATCH 2/2] SDAP: check that connection is open before BIND
> >
> > Tivoli server does not return an empty response when being asked for the
> > rootDSE data but an error. In this case the rootDSE lookup in SSSD will
> > terminate the connection to the server and return a error. But since
> > errors except timeouts are ignored SSSD will try to continue with the
> > bind, but since the connection is already terminated this will fail as
> > well. And this will terminate the whole operation.
> >
> > Make sure the connection is open before performing BIND operation.
>
> I would prefer to avoid using all-capitals BIND in the subject and
> commit message. It just suggests BIND the nameserver to me..

OK, done.
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2435
> >
> > src/providers/ldap/sdap_async_connection.c | 87 ++++++++++++++++++++++++++++++
> >   1 file changed, 87 insertions(+)
> >
> > diff --git a/src/providers/ldap/sdap_async_connection.c b/src/providers/ldap/sdap_async_connection.c
> > index 8f8d67a9b43bb82c2f0e776f36af886ff86c65f8..9951e8b62e49eae310893a3f0844cf4ee2027e83 100644
> > --- a/src/providers/ldap/sdap_async_connection.c
> > +++ b/src/providers/ldap/sdap_async_connection.c
> > @@ -1398,6 +1398,7 @@ struct sdap_cli_connect_state {
> >   enum connect_tls force_tls;
> >   bool do_auth;
> > bool use_tls;
>
> I wonder if it was more consistent to use state->use_tls instead of a
> local variable in sdap_cli_resolve_done() as well.

OK, fixed.
> > };
> >
> > static int sdap_cli_resolve_next(struct tevent_req *req);
> > @@ -1410,6 +1411,8 @@ static void sdap_cli_kinit_step(struct tevent_req *req);
> >   static void sdap_cli_kinit_done(struct tevent_req *subreq);
> >   static void sdap_cli_auth_step(struct tevent_req *req);
> >   static void sdap_cli_auth_done(struct tevent_req *subreq);
> > +static errno_t sdap_cli_auth_reconnect(struct tevent_req *subreq);
> > +static void sdap_cli_auth_reconnect_done(struct tevent_req *subreq);
> >   static void sdap_cli_rootdse_auth_done(struct tevent_req *subreq);
> >
> > static errno_t
> > @@ -1781,6 +1784,16 @@ static void sdap_cli_auth_step(struct tevent_req *req)
> >       struct sss_auth_token *authtok;
> >       errno_t ret;
> > /* It's possible that connection was terminated by server (e.g. #2435),
> >   to overcome this try to connect again. */
> > if (state->sh == NULL || !state->sh->connected) {
>
> Can you put a DEBUG message here?

Sure, done.
> >    ret = sdap_cli_auth_reconnect(req);
> >    if (ret != EOK) {
>
> And maybe also here..

Sure, done.
> >        tevent_req_error(req, ret);
> >    }
> >    return;
> > }
> > /* Set the LDAP expiration time
> >  * If SASL has already set it, use the sooner of the two
> >  */
> >
> > @@ -1842,6 +1855,80 @@ static void sdap_cli_auth_step(struct tevent_req *req)
> >       tevent_req_set_callback(subreq, sdap_cli_auth_done, req);
> >   }
> >
> > +static errno_t sdap_cli_auth_reconnect(struct tevent_req *req)
> > +{
> > struct sdap_cli_connect_state *state;
> > struct tevent_req *subreq;
> > errno_t ret;
> > state = tevent_req_data(req, struct sdap_cli_connect_state);
> > ret = decide_tls_usage(state->force_tls, state->opts->basic,
> >                       state->service->uri, &state->use_tls);
> > if (ret != EOK) {
> >    goto done;
> > }
> > subreq = sdap_connect_send(state, state->ev, state->opts,
> >                           state->service->uri,
> >                           state->service->sockaddr,
> >                           state->use_tls);
> > if (subreq == NULL) {
> >    ret = ENOMEM;
> >    goto done;
> > }
> > tevent_req_set_callback(subreq, sdap_cli_auth_reconnect_done, req);
> > ret = EOK;
> >
> > +done:
> > return ret;
> >
> > +}
> >
> > +static void sdap_cli_auth_reconnect_done(struct tevent_req *subreq)
> > +{
> > struct sdap_cli_connect_state *state;
> > struct tevent_req *req;
> > errno_t ret;
> > req = tevent_req_callback_data(subreq, struct tevent_req);
> > state = tevent_req_data(req, struct sdap_cli_connect_state);
> > talloc_zfree(state->sh);
>
> Please put a newline here. Normally freeing state->sh must be done with
> caution as it terminates an LDAP connection, but here I guess it's OK
> since we're trying to establish a new connection..

OK, done.
> > ret = sdap_connect_recv(subreq, state, &state->sh);
> > talloc_zfree(subreq);
> > if (ret != EOK) {
> >    goto done;
> > }
> > /* if TLS was used, the sdap handle is already marked as connected */
> > if (!state->use_tls) {
> >    /* we need to mark handle as connected to allow anonymous bind */
> >    ret = sdap_set_connected(state->sh, state->ev);
> >    if (ret != EOK) {
> >        DEBUG(SSSDBG_CRIT_FAILURE, "sdap_set_connected() failed.\n");
> >        goto done;
> >    }
> > }
> > /* End request if reconnecting failed to avoid endless loop */
> > if (state->sh == NULL || !state->sh->connected) {
> >    DEBUG(SSSDBG_CRIT_FAILURE, "Failed to reconnect.\n");
> >    ret = EIO;
> >    goto done;
> > } else {
>
> We don't need the else statement here. It's more readable to do:
>  if (failure) {
>      fail();
>  }
>  success();
>
> Rather than:
>      if (failure) {
>          fail();
>      } else {
>          success();
>      }
>
> as you can still keep your eyes on the outermost indendation level only.

OK, done.
> >    sdap_cli_auth_step(req);
> > }
> > ret = EOK;
> >
> > +done:
> > if (ret != EOK) {
> >    tevent_req_error(req, ret);
> > }
> >
> > +}
> > static void sdap_cli_auth_done(struct tevent_req *subreq)
> > {
> >     struct tevent_req *req = tevent_req_callback_data(subreq,
> >
> > -- 
> > 1.9.3
>
> Thank you, these we mostly nitpicks, so I guess we can push the patches
> soon!
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

I'm sorry about the delay in review.

The patches were confirmed to fix the problem by the reporter. I did
some sanity testing of TLS connection with ldap_id_use_start_tls
parameter and I didn't see any regressions.

ACK

[Jakub Hrozek]

On Sun, Oct 12, 2014 at 06:25:55PM +0200, Jakub Hrozek wrote:
> I'm sorry about the delay in review.
>
> The patches were confirmed to fix the problem by the reporter. I did
> some sanity testing of TLS connection with ldap_id_use_start_tls
> parameter and I didn't see any regressions.
>
> ACK

* master:
 * 28929b67fed17df3b7622bb48ed3700f944d91d0
 * daa3dd779fbaec36142d112f8a47c7b9f23177c1