URL: https://github.com/SSSD/sssd/pull/700 Author: jhrozek Title: #700: LDAP: Only authenticate the auth connection if we need to look up user information Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/700/head:pr700 git checkout pr700
From f95292ef7f487fb5f9c388f9abaa90a2f3c0e846 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Thu, 22 Nov 2018 12:51:14 +0100 Subject: [PATCH 1/3] LDAP: minor refactoring in auth_send() to conform to our coding style Related: https://pagure.io/SSSD/sssd/issue/3451 A tevent _send() function should only return NULL on ENOMEM, otherwise it should mark the request as failed but return the req pointer. This was not much of an issue, before, but the next patch will add another function call to the auth_send call which would make error handling awkward. --- src/providers/ldap/ldap_auth.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/providers/ldap/ldap_auth.c b/src/providers/ldap/ldap_auth.c index d40bc9414..c409353d9 100644 --- a/src/providers/ldap/ldap_auth.c +++ b/src/providers/ldap/ldap_auth.c @@ -636,6 +636,7 @@ static struct tevent_req *auth_send(TALLOC_CTX *memctx, { struct tevent_req *req; struct auth_state *state; + errno_t ret; req = tevent_req_create(memctx, &state, struct auth_state); if (!req) return NULL; @@ -645,11 +646,11 @@ static struct tevent_req *auth_send(TALLOC_CTX *memctx, if (sss_authtok_get_type(authtok) == SSS_AUTHTOK_TYPE_SC_PIN || sss_authtok_get_type(authtok) == SSS_AUTHTOK_TYPE_SC_KEYPAD) { /* Tell frontend that we do not support Smartcard authentication */ - tevent_req_error(req, ERR_SC_AUTH_NOT_SUPPORTED); + ret = ERR_SC_AUTH_NOT_SUPPORTED; } else { - tevent_req_error(req, ERR_AUTH_FAILED); + ret = ERR_AUTH_FAILED; } - return tevent_req_post(req, ev); + goto fail; } state->ev = ev; @@ -663,13 +664,17 @@ static struct tevent_req *auth_send(TALLOC_CTX *memctx, state->sdap_service = ctx->service; } - if (!auth_connect_send(req)) goto fail; + if (auth_connect_send(req) == NULL) { + ret = ENOMEM; + goto fail; + } return req; fail: - talloc_zfree(req); - return NULL; + tevent_req_error(req, ret); + tevent_req_post(req, ev); + return req; } static struct tevent_req *auth_connect_send(struct tevent_req *req) From 9c7065286fdf8b7020949ac15757ae67ee25315e Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@redhat.com> Date: Thu, 22 Nov 2018 12:17:51 +0100 Subject: [PATCH 2/3] LDAP: Only authenticate the auth connection if we need to look up user information Related: https://pagure.io/SSSD/sssd/issue/3451 Commit add72860c7a7a2c418f4d8b6790b5caeaf7dfb7b initially addressed #3451 by using the full sdap_cli_connect() request during LDAP authentication. This was a good idea as it addressed the case where the authentication connection must also look up some user information (typically with id_provider=proxy where you don't know the DN to bind as during authentication), but this approach also broke the use-case of id_provider=ldap and auth_provider=ldap with ldap_sasl_auth=gssapi. This is because (for reason I don't know) AD doesn't like if you use both GSSAPI and startTLS on the same connection. But the code would force TLS during the authentication as a general measure to not transmit passwords in the clear, but then, the connection would also see that ldap_sasl_auth=gssapi is set and also bind with GSSAPI. This patch checks if the user DN is already known and if yes, then doesn't authenticate the connection as the connection will then only be used for the user simple bind. --- src/providers/ldap/ldap_auth.c | 53 +++++++++++++++++++++++++++------- 1 file changed, 42 insertions(+), 11 deletions(-) diff --git a/src/providers/ldap/ldap_auth.c b/src/providers/ldap/ldap_auth.c index c409353d9..b4d045a65 100644 --- a/src/providers/ldap/ldap_auth.c +++ b/src/providers/ldap/ldap_auth.c @@ -664,6 +664,18 @@ static struct tevent_req *auth_send(TALLOC_CTX *memctx, state->sdap_service = ctx->service; } + ret = get_user_dn(state, state->ctx->be->domain, + state->ctx->opts, state->username, &state->dn, + &state->pw_expire_type, &state->pw_expire_data); + if (ret == EAGAIN) { + DEBUG(SSSDBG_TRACE_FUNC, + "Need to look up the DN of %s later\n", state->username); + } else if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, + "Cannot get user DN [%d]: %s\n", ret, sss_strerror(ret)); + goto fail; + } + if (auth_connect_send(req) == NULL) { ret = ENOMEM; goto fail; @@ -683,6 +695,8 @@ static struct tevent_req *auth_connect_send(struct tevent_req *req) struct auth_state *state = tevent_req_data(req, struct auth_state); bool use_tls; + bool skip_conn_auth = false; + const char *sasl_mech; /* Check for undocumented debugging feature to disable TLS * for authentication. This should never be used in production @@ -695,10 +709,33 @@ static struct tevent_req *auth_connect_send(struct tevent_req *req) "for debugging purposes only."); } + if (state->dn != NULL) { + /* In case the user's DN is known, the connection will only be used + * to bind as the user to perform the authentication. In that case, + * we don't need to authenticate the connection, because we're not + * looking up any information using the connection. This might be + * needed e.g. in case both ID and AUTH providers are set to LDAP + * and the server is AD, because otherwise the connection would + * both do a startTLS and later bind using GSSAPI which doesn't work + * well with AD. + */ + skip_conn_auth = true; + } + + if (skip_conn_auth == false) { + sasl_mech = dp_opt_get_string(state->ctx->opts->basic, + SDAP_SASL_MECH); + if (sasl_mech && strcasecmp(sasl_mech, "GSSAPI") == 0) { + /* Don't force TLS on if we're told to use GSSAPI */ + use_tls = false; + } + } + subreq = sdap_cli_connect_send(state, state->ev, state->ctx->opts, state->ctx->be, state->sdap_service, false, - use_tls ? CON_TLS_ON : CON_TLS_OFF, false); + use_tls ? CON_TLS_ON : CON_TLS_OFF, + skip_conn_auth); if (subreq == NULL) { tevent_req_error(req, ENOMEM); @@ -739,15 +776,7 @@ static void auth_connect_done(struct tevent_req *subreq) return; } - ret = get_user_dn(state, state->ctx->be->domain, - state->ctx->opts, state->username, &state->dn, - &state->pw_expire_type, &state->pw_expire_data); - if (ret == EOK) { - /* All required user data was pre-cached during an identity lookup. - * We can proceed with the bind */ - auth_do_bind(req); - return; - } else if (ret == EAGAIN) { + if (state->dn == NULL) { /* The cached user entry was missing the bind DN. Need to look * it up based on user name in order to perform the bind */ subreq = get_user_dn_send(req, state->ev, state->ctx->be->domain, @@ -760,7 +789,9 @@ static void auth_connect_done(struct tevent_req *subreq) return; } - tevent_req_error(req, ret); + /* All required user data was pre-cached during an identity lookup. + * We can proceed with the bind */ + auth_do_bind(req); return; } From bf29887d467e96ce5f3326eaeafd3202586c9fc0 Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Mon, 26 Nov 2018 12:38:40 +0100 Subject: [PATCH 3/3] LDAP: Log the encryption used during LDAP authentication --- src/providers/ldap/ldap_auth.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/providers/ldap/ldap_auth.c b/src/providers/ldap/ldap_auth.c index b4d045a65..4666dbfbb 100644 --- a/src/providers/ldap/ldap_auth.c +++ b/src/providers/ldap/ldap_auth.c @@ -747,6 +747,31 @@ static struct tevent_req *auth_connect_send(struct tevent_req *req) return subreq; } +static void check_encryption(LDAP *ldap) +{ + ber_len_t sasl_ssf = 0; + int tls_inplace = 0; + int ret; + + ret = ldap_get_option(ldap, LDAP_OPT_X_SASL_SSF, &sasl_ssf); + if (ret != LDAP_SUCCESS) { + DEBUG(SSSDBG_TRACE_LIBS, "ldap_get_option failed to get sasl ssf, " + "assuming SASL is not used.\n"); + } + + tls_inplace = ldap_tls_inplace(ldap); + + DEBUG(SSSDBG_TRACE_ALL, + "Encryption used: SASL SSF [%lu] tls_inplace [%s].\n", sasl_ssf, + tls_inplace == 1 ? "TLS inplace" : "TLS NOT inplace"); + + if (sasl_ssf <= 1 && tls_inplace != 1) { + DEBUG(SSSDBG_CRIT_FAILURE, + "No encryption detected on LDAP connection.\n"); + sss_log(SSS_LOG_CRIT, "No encryption detected on LDAP connection.\n"); + } +} + static void auth_connect_done(struct tevent_req *subreq) { struct tevent_req *req = tevent_req_callback_data(subreq, @@ -776,6 +801,8 @@ static void auth_connect_done(struct tevent_req *subreq) return; } + check_encryption(state->sh->ldap); + if (state->dn == NULL) { /* The cached user entry was missing the bind DN. Need to look * it up based on user name in order to perform the bind */
_______________________________________________ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org