[PATCH] Remove unnecessary unbind in LDAP search+bind mode

Started by Anatoly Zaretskyalmost 3 years ago4 messages
#1Anatoly Zaretsky
anatoly.zaretsky@gmail.com
1 attachment(s)

Hi!

Comments in src/backend/libpq/auth.c [1]https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/libpq/auth.c;h=bc0cf26b122a1b28c20fe037ec851c0e99b1ffb6;hb=HEAD#l2603 say:
(after successfully finding the final DN to check the user-supplied
password against)
/* Unbind and disconnect from the LDAP server */
and later
/*
* Need to re-initialize the LDAP connection, so that we can bind to
* it with a different username.
*/

But the protocol actually permits multiple subsequent authentications
("binds" in LDAP parlance) over a single connection [2]https://www.rfc-editor.org/rfc/rfc4511#section-4.2.1.
Moreover, inspection of the code revision history of mod_authnz_ldap,
pam_ldap, Bugzilla, and MediaWiki LDAP authentication plugin, shows that
they've been doing this bind-after-search over the same LDAP connection for
~20 years without any evidence of interoperability troubles.

(mod_authnz_ldap and pam_ldap are listed in the PostgreSQL documentation as
examples of other software implementing this scheme. Bugzilla and MediaWiki
are the original patch author's motivating examples [3]/messages/by-id/4c0112730909141334n201cadf3x2e288528a97883ca@mail.gmail.com)

Also it might be interesting to consider this note from the current
revision of the protocol RFC [4]https://www.rfc-editor.org/rfc/rfc4511#section-4.3 -- Best regards, Anatoly Zaretsky:
"The Unbind operation is not the antithesis of the Bind operation as the
name implies. The naming of these operations are historical. The Unbind
operation should be thought of as the "quit" operation."

So, it seems like the whole connection re-initialization thing was just a
confusion caused by this very unfortunate "historical" naming, and can be
safely removed, thus saving quite a few network round-trips, especially for
the case of ldaps/starttls.

[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/libpq/auth.c;h=bc0cf26b122a1b28c20fe037ec851c0e99b1ffb6;hb=HEAD#l2603
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/libpq/auth.c;h=bc0cf26b122a1b28c20fe037ec851c0e99b1ffb6;hb=HEAD#l2603
[2]: https://www.rfc-editor.org/rfc/rfc4511#section-4.2.1
[3]: /messages/by-id/4c0112730909141334n201cadf3x2e288528a97883ca@mail.gmail.com
/messages/by-id/4c0112730909141334n201cadf3x2e288528a97883ca@mail.gmail.com
[4]: https://www.rfc-editor.org/rfc/rfc4511#section-4.3 -- Best regards, Anatoly Zaretsky
--
Best regards,
Anatoly Zaretsky

Attachments:

v1-0001-Remove-unnecessary-unbind-in-LDAP-search-bind-mod.patchapplication/octet-stream; name=v1-0001-Remove-unnecessary-unbind-in-LDAP-search-bind-mod.patchDownload
From da725c7308fc1a98c2a2849e6dd6d4cd99eb0921 Mon Sep 17 00:00:00 2001
From: Anatoly Zaretsky <anatoly.zaretsky@gmail.com>
Date: Tue, 21 Mar 2023 05:03:16 +0200
Subject: [PATCH v1] Remove unnecessary unbind in LDAP search+bind mode

---
 doc/src/sgml/client-auth.sgml |  6 +++---
 src/backend/libpq/auth.c      | 25 -------------------------
 2 files changed, 3 insertions(+), 28 deletions(-)

diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index b9d73deced..3e86b8eb65 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -1775,13 +1775,13 @@ omicron         bryanh                  guest1
     do an exact match of the attribute specified in
     <replaceable>ldapsearchattribute</replaceable>.
     Once the user has been found in
-    this search, the server disconnects and re-binds to the directory as
+    this search, the server re-binds to the directory as
     this user, using the password specified by the client, to verify that the
     login is correct. This mode is the same as that used by LDAP authentication
     schemes in other software, such as Apache <literal>mod_authnz_ldap</literal> and <literal>pam_ldap</literal>.
     This method allows for significantly more flexibility
     in where the user objects are located in the directory, but will cause
-    two separate connections to the LDAP server to be made.
+    two additional requests to the LDAP server to be made.
    </para>
 
    <para>
@@ -2008,7 +2008,7 @@ host ... ldap ldapserver=ldap.example.net ldapbasedn="dc=example, dc=net" ldapse
     the LDAP server, perform a search for <literal>(uid=someuser)</literal>
     under the specified base DN.  If an entry is found, it will then attempt to
     bind using that found information and the password supplied by the client.
-    If that second connection succeeds, the database access is granted.
+    If that second bind succeeds, the database access is granted.
    </para>
 
    <para>
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index bc0cf26b12..a949258717 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -2599,31 +2599,6 @@ CheckLDAPAuth(Port *port)
 		pfree(filter);
 		ldap_memfree(dn);
 		ldap_msgfree(search_message);
-
-		/* Unbind and disconnect from the LDAP server */
-		r = ldap_unbind_s(ldap);
-		if (r != LDAP_SUCCESS)
-		{
-			ereport(LOG,
-					(errmsg("could not unbind after searching for user \"%s\" on server \"%s\"",
-							fulluser, server_name)));
-			pfree(passwd);
-			pfree(fulluser);
-			return STATUS_ERROR;
-		}
-
-		/*
-		 * Need to re-initialize the LDAP connection, so that we can bind to
-		 * it with a different username.
-		 */
-		if (InitializeLDAPConnection(port, &ldap) == STATUS_ERROR)
-		{
-			pfree(passwd);
-			pfree(fulluser);
-
-			/* Error message already sent */
-			return STATUS_ERROR;
-		}
 	}
 	else
 		fulluser = psprintf("%s%s%s",

base-commit: 8fba928fd78856712f69d96852f8061e77390fda
-- 
2.37.1 (Apple Git-137.1)

#2Peter Eisentraut
peter@eisentraut.org
In reply to: Anatoly Zaretsky (#1)
Re: [PATCH] Remove unnecessary unbind in LDAP search+bind mode

On 23.03.23 02:45, Anatoly Zaretsky wrote:

Comments in src/backend/libpq/auth.c [1] say:
(after successfully finding the final DN to check the user-supplied
password against)
/* Unbind and disconnect from the LDAP server */
and later
/*
* Need to re-initialize the LDAP connection, so that we can bind to
* it with a different username.
*/

But the protocol actually permits multiple subsequent authentications
("binds" in LDAP parlance) over a single connection [2].
Moreover, inspection of the code revision history of mod_authnz_ldap,
pam_ldap, Bugzilla, and MediaWiki LDAP authentication plugin, shows that
they've been doing this bind-after-search over the same LDAP connection
for ~20 years without any evidence of interoperability troubles.

So, it seems like the whole connection re-initialization thing was just
a confusion caused by this very unfortunate "historical" naming, and can
be safely removed, thus saving quite a few network round-trips,
especially for the case of ldaps/starttls.

Your reasoning and your patch look correct to me.

#3Peter Eisentraut
peter@eisentraut.org
In reply to: Peter Eisentraut (#2)
Re: [PATCH] Remove unnecessary unbind in LDAP search+bind mode

On 03.07.23 11:53, Peter Eisentraut wrote:

On 23.03.23 02:45, Anatoly Zaretsky wrote:

Comments in src/backend/libpq/auth.c [1] say:
(after successfully finding the final DN to check the user-supplied
password against)
/* Unbind and disconnect from the LDAP server */
and later
/*
* Need to re-initialize the LDAP connection, so that we can bind to
* it with a different username.
*/

But the protocol actually permits multiple subsequent authentications
("binds" in LDAP parlance) over a single connection [2].
Moreover, inspection of the code revision history of mod_authnz_ldap,
pam_ldap, Bugzilla, and MediaWiki LDAP authentication plugin, shows
that they've been doing this bind-after-search over the same LDAP
connection for ~20 years without any evidence of interoperability
troubles.

So, it seems like the whole connection re-initialization thing was
just a confusion caused by this very unfortunate "historical"
naming, and can be safely removed, thus saving quite a few
network round-trips, especially for the case of ldaps/starttls.

Your reasoning and your patch look correct to me.

committed

#4Anatoly Zaretsky
anatoly.zaretsky@gmail.com
In reply to: Peter Eisentraut (#3)
Re: [PATCH] Remove unnecessary unbind in LDAP search+bind mode

On Sun, Jul 9, 2023 at 9:57 AM Peter Eisentraut <peter@eisentraut.org>
wrote:

committed

Thanks!

--
Best regards,
Anatoly Zaretsky