3:22 a.m.
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!
4:01 p.m.
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;
+ 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 <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..
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!
6:07 a.m.
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
11:25 a.m.
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
11:32 a.m.
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
3484
days inactive
3508
days old
sssd-devel@lists.fedorahosted.org
4 comments
2 participants
participants (2)
-
Jakub Hrozek -
Pavel Reichl