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 <preichl(a)redhat.com>
> 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 <preichl(a)redhat.com>
> 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
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel