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: opened
PR body: """ 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. """
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
URL: https://github.com/SSSD/sssd/pull/700 Title: #700: LDAP: Only authenticate the auth connection if we need to look up user information
jhrozek commented: """ I chose this approach because it only touches the ldap auth code. The other approach I was considering was to not force off the authentication, but turn the skip_auth boolean into a tri-state (do_auth, skip_auth, auth_if_possible). Then the caller from the ldap auth code would select auth_if_possible if the DN is known and do_auth if the DN must be looked up.
What auth_if_possible would do is to check if GSSAPI is the authentication method and only skip only the GSSAPI auth step. This would make it possible to use other authentication methods, whatever they might be in case the DN must be looked up.
And even another approach might be to establish authenticated connection to look up the user, then close is and authenticate the user using a second connection.
But currently the only use-case that doesn't work with the current approach is id_provider=proxy and auth_provider=ldap where the LDAP server is AD DC. In this case, you must look up the user, so the connection must be authenticated, but using TLS and GSSAPI together wouldn't work. And I'm not sure if this use-case is important enough to consider either two connections or touching the connection code. """
See the full comment at https://github.com/SSSD/sssd/pull/700#issuecomment-441025645
URL: https://github.com/SSSD/sssd/pull/700 Title: #700: LDAP: Only authenticate the auth connection if we need to look up user information
jhrozek commented: """ I chose this approach because it only touches the ldap auth code. The other approach I was considering was to not force off the authentication, but turn the skip_auth boolean into a tri-state (do_auth, skip_auth, auth_if_possible). Then the caller from the ldap auth code would select auth_if_possible if the DN is known and do_auth if the DN must be looked up.
What auth_if_possible would do is to check if GSSAPI is the authentication method and only skip only the GSSAPI auth step. This would make it possible to use other authentication methods, whatever they might be in case the DN must be looked up.
And even another approach might be to establish authenticated connection to look up the user, then close it and authenticate the user using a second connection.
But currently the only use-case that doesn't work with the current approach is id_provider=proxy and auth_provider=ldap where the LDAP server is AD DC. In this case, you must look up the user, so the connection must be authenticated, but using TLS and GSSAPI together wouldn't work, so the only way you can make this setup work is to use a bind DN and password. And I'm not sure if this use-case is important enough to consider either two connections or touching the connection code. Moreover, this combination of proxy identity, ldap authentication and GSSAPI-authenticated binds didn't work in any of the previous SSSD versions. """
See the full comment at https://github.com/SSSD/sssd/pull/700#issuecomment-441025645
URL: https://github.com/SSSD/sssd/pull/700 Title: #700: LDAP: Only authenticate the auth connection if we need to look up user information
Label: +Changes requested
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
URL: https://github.com/SSSD/sssd/pull/700 Title: #700: LDAP: Only authenticate the auth connection if we need to look up user information
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/700 Title: #700: LDAP: Only authenticate the auth connection if we need to look up user information
sumit-bose commented: """ Thank you, the patches are looking good and worked well in my tests, ACK.
I added the following call while testing the patches to better see what is going on.
``` diff --git a/src/providers/ldap/ldap_auth.c b/src/providers/ldap/ldap_auth.c index b4d045a..8b4bccf 100644 --- a/src/providers/ldap/ldap_auth.c +++ b/src/providers/ldap/ldap_auth.c @@ -747,6 +747,38 @@ 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; + void *ssl_ctx = NULL; + int ret; + + ret = ldap_get_option(ldap, LDAP_OPT_X_SASL_SSF, &sasl_ssf); + if (ret != LDAP_SUCCESS) { + DEBUG(SSSDBG_OP_FAILURE, "ldap_get_option failed to get sasl ssf, " + "ignored.\n"); + } + + /* LDAP_OPT_X_TLS_SSL_CTX returns a pointer to a struct which depends on + * the crypto library used by OpenLDAP. Since in general we do not know + * which library is used we just check if the pointer is not NULL and + * assume that in this case the TLS setup was successful. */ + ret = ldap_get_option(ldap, LDAP_OPT_X_TLS_SSL_CTX, &ssl_ctx); + if (ret != LDAP_SUCCESS) { + DEBUG(SSSDBG_OP_FAILURE, "ldap_get_option failed to get ssl ctx, " + "ignored.\n"); + } + + DEBUG(SSSDBG_TRACE_ALL, "Encryption used: SASF SSF [%lu] SSL CTX [%s].\n", + sasl_ssf, + ssl_ctx == NULL ? "No SSL CTX" : "SSL CTX present"); + + if (sasl_ssf <= 1 && ssl_ctx == NULL) { + 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 +808,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 */ ```
Maybe you think it is worth adding it. """
See the full comment at https://github.com/SSSD/sssd/pull/700#issuecomment-441576189
URL: https://github.com/SSSD/sssd/pull/700 Title: #700: LDAP: Only authenticate the auth connection if we need to look up user information
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/700 Title: #700: LDAP: Only authenticate the auth connection if we need to look up user information
sumit-bose commented: """ ah sorry, I just realized that I didn't paste the latest version of my debug call. Here it is:
``` diff --git a/src/providers/ldap/ldap_auth.c b/src/providers/ldap/ldap_auth.c index b4d045a..12c00f7 100644 --- a/src/providers/ldap/ldap_auth.c +++ b/src/providers/ldap/ldap_auth.c @@ -747,6 +747,38 @@ 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; + void *ssl_ctx = NULL; + 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"); + } + + /* LDAP_OPT_X_TLS_SSL_CTX returns a pointer to a struct which depends on + * the crypto library used by OpenLDAP. Since in general we do not know + * which library is used we just check if the pointer is not NULL and + * assume that in this case the TLS setup was successful. */ + ret = ldap_get_option(ldap, LDAP_OPT_X_TLS_SSL_CTX, &ssl_ctx); + if (ret != LDAP_SUCCESS) { + DEBUG(SSSDBG_OP_FAILURE, "ldap_get_option failed to get ssl ctx, " + "ignored.\n"); + } + + DEBUG(SSSDBG_TRACE_ALL, "Encryption used: SASF SSF [%lu] SSL CTX [%s].\n", + sasl_ssf, + ssl_ctx == NULL ? "No SSL CTX" : "SSL CTX present"); + + if (sasl_ssf <= 1 && ssl_ctx == NULL) { + 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 +808,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 */ ```
The change is in the debug level and message when trying to get LDAP_OPT_X_SASL_SSF. """
See the full comment at https://github.com/SSSD/sssd/pull/700#issuecomment-441577635
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
URL: https://github.com/SSSD/sssd/pull/700 Title: #700: LDAP: Only authenticate the auth connection if we need to look up user information
jhrozek commented: """ Thank you, this is nice. I added your patch to the PR, fixed one typo and one minor style issue. If you're OK with the fixes, I will squash the last patch into yours and push them all once CI finishes.
If we want to do additional hardening, we can even save the value of the SDAP_DISABLE_AUTH_TLS variable and unless it is set to TRUE, we can even abort the authentication if no encryption is selected.
btw during testing, I even listened to the traffic with tcpdump and then checked the pcap files to make sure the traffic is encrypted, so at least for the cases that were tested I know we are fine. But the patch is very nice to have for sure. """
See the full comment at https://github.com/SSSD/sssd/pull/700#issuecomment-441581678
URL: https://github.com/SSSD/sssd/pull/700 Title: #700: LDAP: Only authenticate the auth connection if we need to look up user information
sumit-bose commented: """ Thanks, yes, I'm fine with the changes. However, I just came across ldap_tls_inplace() which we already use and I guess it more portable. Here is my new version:
``` diff --git a/src/providers/ldap/ldap_auth.c b/src/providers/ldap/ldap_auth.c index b4d045a..4666dbf 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 */ ``` """
See the full comment at https://github.com/SSSD/sssd/pull/700#issuecomment-441596122
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
URL: https://github.com/SSSD/sssd/pull/700 Title: #700: LDAP: Only authenticate the auth connection if we need to look up user information
jhrozek commented: """ thanks, I added your newest patch version to this PR """
See the full comment at https://github.com/SSSD/sssd/pull/700#issuecomment-441610109
URL: https://github.com/SSSD/sssd/pull/700 Title: #700: LDAP: Only authenticate the auth connection if we need to look up user information
sumit-bose commented: """ thank, I'm fine with the current version, so ACK in Acepted are still valid.
About "If we want to do additional hardening, we can even save the value of the SDAP_DISABLE_AUTH_TLS variable and unless it is set to TRUE, we can even abort the authentication if no encryption is selected." I was thinking about this as well. But since the PR is mainly about fixing an authentication issues I think we do not want to risk to add another failure in a different special setup and better add this functionality in a different PR. """
See the full comment at https://github.com/SSSD/sssd/pull/700#issuecomment-441623455
URL: https://github.com/SSSD/sssd/pull/700 Title: #700: LDAP: Only authenticate the auth connection if we need to look up user information
jhrozek commented: """ * master: * 6f113c7ddeaa5c82558e10118b499d22bf7a2b14 * 57fc60c9dc77698cf824813c36eb0f90d767b315 * 09091b4b60456a989ecc8c3b6f76661a14c108ba
"""
See the full comment at https://github.com/SSSD/sssd/pull/700#issuecomment-441790753
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: closed
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
URL: https://github.com/SSSD/sssd/pull/700 Title: #700: LDAP: Only authenticate the auth connection if we need to look up user information
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/700 Title: #700: LDAP: Only authenticate the auth connection if we need to look up user information
jhrozek commented: """ * sssd-1-16: 1a7c6ab6efce3720d27def426aad49ee99eb339d 7eb18ab68762d1b1ddbcbdc32dbcbd0df183d4f1 876f1cb87d1649d0681bf6475ab589287f15babb """
See the full comment at https://github.com/SSSD/sssd/pull/700#issuecomment-441791236
URL: https://github.com/SSSD/sssd/pull/700 Title: #700: LDAP: Only authenticate the auth connection if we need to look up user information
jhrozek commented: """ btw I opened https://pagure.io/SSSD/sssd/issue/3889 to track the additional hardening. Maybe it would be a nice task for one of the new people on the team.. """
See the full comment at https://github.com/SSSD/sssd/pull/700#issuecomment-441795553
sssd-devel@lists.fedorahosted.org