Re: [HACKERS] LDAP where DN does not include UID attribute
On Sat, Dec 12, 2009 at 02:41, Robert Haas robertmh...@gmail.com wrote: On Sun, Dec 6, 2009 at 11:29 PM, Greg Smith g...@2ndquadrant.com wrote: Magnus Hagander wrote: On Sun, Nov 29, 2009 at 13:05, Magnus Hagander mag...@hagander.net wrote: I'll be happy to work on this to get it ready for commit, or do you want to do the updates? Here's a patch with my work so far. Major points missing is the sanitizing of the username and the docs. It looks like you did an initial review here already. Since we haven't heard from Robert in a while and you seem interested in the patch, I just updated this one to list you as the committer and marked it ready for committer. You can commit it or bounce it from there, but it's obvious none of our other reviewers are going to be able to do anything with it. I think we should mark this returned with feedback, since it sounds like there are still open items. I plan to clean up those open item smyself since there has been no further feedback from the OP. Hopefully in time before the CF ends. So please leave it for a few days more :-) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LDAP where DN does not include UID attribute
On Sat, Dec 12, 2009 at 12:24, Magnus Hagander mag...@hagander.net wrote: On Sat, Dec 12, 2009 at 02:41, Robert Haas robertmh...@gmail.com wrote: On Sun, Dec 6, 2009 at 11:29 PM, Greg Smith g...@2ndquadrant.com wrote: Magnus Hagander wrote: On Sun, Nov 29, 2009 at 13:05, Magnus Hagander mag...@hagander.net wrote: I'll be happy to work on this to get it ready for commit, or do you want to do the updates? Here's a patch with my work so far. Major points missing is the sanitizing of the username and the docs. It looks like you did an initial review here already. Since we haven't heard from Robert in a while and you seem interested in the patch, I just updated this one to list you as the committer and marked it ready for committer. You can commit it or bounce it from there, but it's obvious none of our other reviewers are going to be able to do anything with it. I think we should mark this returned with feedback, since it sounds like there are still open items. I plan to clean up those open item smyself since there has been no further feedback from the OP. Hopefully in time before the CF ends. So please leave it for a few days more :-) I have applied the attached, rather heavily reworked, patch. It also includes the required documentation. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ ldap.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LDAP where DN does not include UID attribute
On Sun, Dec 6, 2009 at 11:29 PM, Greg Smith g...@2ndquadrant.com wrote: Magnus Hagander wrote: On Sun, Nov 29, 2009 at 13:05, Magnus Hagander mag...@hagander.net wrote: I'll be happy to work on this to get it ready for commit, or do you want to do the updates? Here's a patch with my work so far. Major points missing is the sanitizing of the username and the docs. It looks like you did an initial review here already. Since we haven't heard from Robert in a while and you seem interested in the patch, I just updated this one to list you as the committer and marked it ready for committer. You can commit it or bounce it from there, but it's obvious none of our other reviewers are going to be able to do anything with it. I think we should mark this returned with feedback, since it sounds like there are still open items. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LDAP where DN does not include UID attribute
Magnus Hagander wrote: On Sun, Nov 29, 2009 at 13:05, Magnus Hagander mag...@hagander.net wrote: I'll be happy to work on this to get it ready for commit, or do you want to do the updates? Here's a patch with my work so far. Major points missing is the sanitizing of the username and the docs. It looks like you did an initial review here already. Since we haven't heard from Robert in a while and you seem interested in the patch, I just updated this one to list you as the committer and marked it ready for committer. You can commit it or bounce it from there, but it's obvious none of our other reviewers are going to be able to do anything with it. -- Greg Smith2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support g...@2ndquadrant.com www.2ndQuadrant.com
Re: [HACKERS] LDAP where DN does not include UID attribute
On Fri, Sep 18, 2009 at 02:24, Robert Fleming flemi...@gmail.com wrote: On Thu, Sep 17, 2009 at 11:15 AM, Magnus Hagander mag...@hagander.net wrote: On Thu, Sep 17, 2009 at 18:02, Robert Fleming flemi...@gmail.com wrote: Following a discussion on the pgsql-admin list http://archives.postgresql.org/pgsql-admin/2009-09/msg00075.php, I have created a patch to (optionally) allow PostgreSQL to do a LDAP search to determine the user's DN (as is done in Apache, MediaWiki, Bugzilla, et al.) instead of building the DN from a prefix and suffix. This is necessary for schemas where the login attribute is not in the DN, such as is described here http://www.ldapman.org/articles/intro_to_ldap.html#individual (look for name-based). This patch is against PostgreSQL 8.4.0 from Debian Lenny-backports. If this would be a welcome addition, I can port it forward to the latest from postgresql.org. Thanks in advance for your feedback. This sounds like a very useful feature, and one that I can then remove from my personal TODO list without having to do much work :-) A couple of comments: First of all, please read up on the PostgreSQL coding style, or at least look at the code around yours. This doesn't look anything like our standards. Sorry, the 8.1 manual said all contributions go through pgindent so I didn't spend too much time on that. I see that the 8.4 manual clarifies that pgindent won't get run till release time. In any case, I've re-formatted the patch using the Emacs settings from the 8.1 manual (why are they gone from the 8.4 manual)? It seems like most of the rest of the Coding Conventions have to do with error reporting. Do please let me know if there's something else I can do. Second, this appears to require an anonymous bind to the directory, which is something we should not encourage people to enable on their LDAP servers. I think we need to also take parameters with a DN and a password to bind with in order to do the search, and then re-bind as the user when found. The new patch implements the initial bind using new configuration parameters ldapbinddn and ldapbindpasswd. I've also added a ldapsearchattribute just to be complete. I've finally had the time to look at this patch some more, for the current commitfest - sorry about the delay. Other than minor style changes (make the indentation be the same as the code around it), I noticed one fairly large issue. The way that LDAP is re-initialized both breaks Windows (in the error reporting part) and not as obvious but even more importantly, it breaks TLS. If you enable TLS for LDAP it will only be used for the search, not for the actual password check - which really removes the point of it :-) I assume we need the second ldap_init() because we want to do a new ldap_simple_bind()? In that case, we realliy need to re-initialize the whole connection, including TLS. I also notice that it's hardcoded to retrieve the uid attribute, while the one being searched for can be configured. ISTM it's better to set both of these to the hba-ldapsearchattribute value - so we won't fail if uid does not exist. Also, as I'm sure you're aware there are no documentation updates included. I'll be happy to work on this to get it ready for commit, or do you want to do the updates? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LDAP where DN does not include UID attribute
On Sun, Nov 29, 2009 at 13:05, Magnus Hagander mag...@hagander.net wrote: On Fri, Sep 18, 2009 at 02:24, Robert Fleming flemi...@gmail.com wrote: On Thu, Sep 17, 2009 at 11:15 AM, Magnus Hagander mag...@hagander.net wrote: On Thu, Sep 17, 2009 at 18:02, Robert Fleming flemi...@gmail.com wrote: Following a discussion on the pgsql-admin list http://archives.postgresql.org/pgsql-admin/2009-09/msg00075.php, I have created a patch to (optionally) allow PostgreSQL to do a LDAP search to determine the user's DN (as is done in Apache, MediaWiki, Bugzilla, et al.) instead of building the DN from a prefix and suffix. This is necessary for schemas where the login attribute is not in the DN, such as is described here http://www.ldapman.org/articles/intro_to_ldap.html#individual (look for name-based). This patch is against PostgreSQL 8.4.0 from Debian Lenny-backports. If this would be a welcome addition, I can port it forward to the latest from postgresql.org. Thanks in advance for your feedback. This sounds like a very useful feature, and one that I can then remove from my personal TODO list without having to do much work :-) A couple of comments: First of all, please read up on the PostgreSQL coding style, or at least look at the code around yours. This doesn't look anything like our standards. Sorry, the 8.1 manual said all contributions go through pgindent so I didn't spend too much time on that. I see that the 8.4 manual clarifies that pgindent won't get run till release time. In any case, I've re-formatted the patch using the Emacs settings from the 8.1 manual (why are they gone from the 8.4 manual)? It seems like most of the rest of the Coding Conventions have to do with error reporting. Do please let me know if there's something else I can do. Second, this appears to require an anonymous bind to the directory, which is something we should not encourage people to enable on their LDAP servers. I think we need to also take parameters with a DN and a password to bind with in order to do the search, and then re-bind as the user when found. The new patch implements the initial bind using new configuration parameters ldapbinddn and ldapbindpasswd. I've also added a ldapsearchattribute just to be complete. I've finally had the time to look at this patch some more, for the current commitfest - sorry about the delay. Other than minor style changes (make the indentation be the same as the code around it), I noticed one fairly large issue. The way that LDAP is re-initialized both breaks Windows (in the error reporting part) and not as obvious but even more importantly, it breaks TLS. If you enable TLS for LDAP it will only be used for the search, not for the actual password check - which really removes the point of it :-) I assume we need the second ldap_init() because we want to do a new ldap_simple_bind()? In that case, we realliy need to re-initialize the whole connection, including TLS. I also notice that it's hardcoded to retrieve the uid attribute, while the one being searched for can be configured. ISTM it's better to set both of these to the hba-ldapsearchattribute value - so we won't fail if uid does not exist. Also, as I'm sure you're aware there are no documentation updates included. I'll be happy to work on this to get it ready for commit, or do you want to do the updates? Here's a patch with my work so far. Major points missing is the sanitizing of the username and the docs. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ ldap_search.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] LDAP where DN does not include UID attribute
Following a discussion on the pgsql-admin list
http://archives.postgresql.org/pgsql-admin/2009-09/msg00075.php, I have
created a patch to (optionally) allow PostgreSQL to do a LDAP search to
determine the user's DN (as is done in Apache, MediaWiki, Bugzilla, et al.)
instead of building the DN from a prefix and suffix.
This is necessary for schemas where the login attribute is not in the DN,
such as is described here
http://www.ldapman.org/articles/intro_to_ldap.html#individual (look for
name-based). This patch is against PostgreSQL 8.4.0 from Debian
Lenny-backports. If this would be a welcome addition, I can port it forward
to the latest from postgresql.org.
Thanks in advance for your feedback.
Robert
diff -ru postgresql-8.4-8.4.0-original/src/backend/libpq/auth.c postgresql-8.4-8.4.0/src/backend/libpq/auth.c
--- postgresql-8.4-8.4.0-original/src/backend/libpq/auth.c 2009-06-25 04:30:08.0 -0700
+++ postgresql-8.4-8.4.0/src/backend/libpq/auth.c 2009-09-16 22:33:46.0 -0700
@@ -2150,10 +2150,75 @@
}
}
- snprintf(fulluser, sizeof(fulluser), %s%s%s,
- port-hba-ldapprefix ? port-hba-ldapprefix : ,
- port-user_name,
- port-hba-ldapsuffix ? port-hba-ldapsuffix : );
+ if (port-hba-ldapbasedn)
+ {
+ char filter[NAMEDATALEN + 10]; // FIXME: maybe there's a preferred way to pick this size?
+ LDAPMessage* search_message;
+ char* attributes[2];
+ LDAPMessage* entry;
+ char* dn;
+
+ /* it seems that you need to search for at least one attribute, else /all/ attributes are returned */
+ attributes[0] = uid;
+ attributes[1] = NULL;
+
+ snprintf(filter, sizeof(filter), (uid=%s), port-user_name);
+ filter[sizeof(filter) - 1] = '\0';
+
+ r = ldap_search_s(ldap,
+ port-hba-ldapbasedn,
+ LDAP_SCOPE_SUBTREE,
+ filter,
+ attributes,
+ 0,
+ search_message);
+
+ if (r != LDAP_SUCCESS)
+ {
+ ereport(LOG,
+ (errmsg(LDAP search failed for user \%s\ on server \%s\: error code %d,
+fulluser, port-hba-ldapserver, r)));
+ return STATUS_ERROR;
+ }
+
+ if (ldap_count_entries(ldap, search_message) != 1)
+ {
+ if (ldap_count_entries(ldap, search_message) == 0)
+ ereport(LOG,
+ (errmsg(LDAP search failed for user \%s\ on server \%s\: no such user,
+ fulluser, port-hba-ldapserver)));
+ else
+ ereport(LOG,
+ (errmsg(LDAP search failed for user \%s\ on server \%s\: user is not unique (%d matches),
+ fulluser, port-hba-ldapserver, ldap_count_entries(ldap, search_message;
+
+ ldap_msgfree(search_message);
+ return STATUS_ERROR;
+ }
+
+ entry = ldap_first_entry(ldap, search_message);
+ dn = ldap_get_dn(ldap, entry);
+ if (dn == NULL)
+ {
+ int error;
+ r = ldap_get_option(ldap, LDAP_OPT_RESULT_CODE, error);
+ ereport(LOG,
+ (errmsg(LDAP search failed for user \%s\ on server \%s\: %s,
+fulluser, port-hba-ldapserver, ldap_err2string(error;
+ ldap_msgfree(search_message);
+ return STATUS_ERROR;
+ }
+ strncpy(fulluser, dn, sizeof(fulluser));
+
+ ldap_memfree(dn);
+ ldap_msgfree(search_message);
+ }
+ else
+ snprintf(fulluser, sizeof(fulluser), %s%s%s,
+ port-hba-ldapprefix ? port-hba-ldapprefix : ,
+ port-user_name,
+ port-hba-ldapsuffix ? port-hba-ldapsuffix : );
+
fulluser[sizeof(fulluser) - 1] = '\0';
r = ldap_simple_bind_s(ldap, fulluser, passwd);
diff -ru postgresql-8.4-8.4.0-original/src/backend/libpq/hba.c postgresql-8.4-8.4.0/src/backend/libpq/hba.c
--- postgresql-8.4-8.4.0-original/src/backend/libpq/hba.c 2009-06-24 06:39:42.0 -0700
+++ postgresql-8.4-8.4.0/src/backend/libpq/hba.c 2009-09-16 22:19:59.0 -0700
@@ -1032,6 +1032,11 @@
return false;
}
}
+ else if (strcmp(token, ldapbasedn) == 0)
+ {
+REQUIRE_AUTH_OPTION(uaLDAP, ldapbasedn, ldap);
+parsedline-ldapbasedn = pstrdup(c);
+ }
else if (strcmp(token, ldapprefix) == 0)
{
REQUIRE_AUTH_OPTION(uaLDAP, ldapprefix, ldap);
diff -ru postgresql-8.4-8.4.0-original/src/include/libpq/hba.h postgresql-8.4-8.4.0/src/include/libpq/hba.h
--- postgresql-8.4-8.4.0-original/src/include/libpq/hba.h 2009-06-11 07:49:11.0 -0700
+++ postgresql-8.4-8.4.0/src/include/libpq/hba.h 2009-09-16 22:20:07.0 -0700
@@ -53,6 +53,7 @@
bool ldaptls;
char *ldapserver;
int ldapport;
+ char *ldapbasedn;
char *ldapprefix;
char *ldapsuffix;
bool clientcert;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LDAP where DN does not include UID attribute
On Thu, Sep 17, 2009 at 18:02, Robert Fleming flemi...@gmail.com wrote: Following a discussion on the pgsql-admin list http://archives.postgresql.org/pgsql-admin/2009-09/msg00075.php, I have created a patch to (optionally) allow PostgreSQL to do a LDAP search to determine the user's DN (as is done in Apache, MediaWiki, Bugzilla, et al.) instead of building the DN from a prefix and suffix. This is necessary for schemas where the login attribute is not in the DN, such as is described here http://www.ldapman.org/articles/intro_to_ldap.html#individual (look for name-based). This patch is against PostgreSQL 8.4.0 from Debian Lenny-backports. If this would be a welcome addition, I can port it forward to the latest from postgresql.org. Thanks in advance for your feedback. This sounds like a very useful feature, and one that I can then remove from my personal TODO list without having to do much work :-) A couple of comments: First of all, please read up on the PostgreSQL coding style, or at least look at the code around yours. This doesn't look anything like our standards. Second, this appears to require an anonymous bind to the directory, which is something we should not encourage people to enable on their LDAP servers. I think we need to also take parameters with a DN and a password to bind with in order to do the search, and then re-bind as the user when found. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LDAP where DN does not include UID attribute
On Thu, Sep 17, 2009 at 11:15 AM, Magnus Hagander mag...@hagander.netwrote:
On Thu, Sep 17, 2009 at 18:02, Robert Fleming flemi...@gmail.com wrote:
Following a discussion on the pgsql-admin list
http://archives.postgresql.org/pgsql-admin/2009-09/msg00075.php, I
have
created a patch to (optionally) allow PostgreSQL to do a LDAP search to
determine the user's DN (as is done in Apache, MediaWiki, Bugzilla, et
al.)
instead of building the DN from a prefix and suffix.
This is necessary for schemas where the login attribute is not in the DN,
such as is described here
http://www.ldapman.org/articles/intro_to_ldap.html#individual (look
for
name-based). This patch is against PostgreSQL 8.4.0 from Debian
Lenny-backports. If this would be a welcome addition, I can port it
forward
to the latest from postgresql.org.
Thanks in advance for your feedback.
This sounds like a very useful feature, and one that I can then remove
from my personal TODO list without having to do much work :-)
A couple of comments:
First of all, please read up on the PostgreSQL coding style, or at
least look at the code around yours. This doesn't look anything like
our standards.
Sorry, the 8.1 manual said all contributions go through pgindent so I didn't
spend too much time on that. I see that the 8.4 manual clarifies that
pgindent won't get run till release time. In any case, I've re-formatted
the patch using the Emacs settings from the 8.1 manual (why are they gone
from the 8.4 manual)? It seems like most of the rest of the Coding
Conventions have to do with error reporting. Do please let me know if
there's something else I can do.
Second, this appears to require an anonymous bind to the directory,
which is something we should not encourage people to enable on their
LDAP servers. I think we need to also take parameters with a DN and a
password to bind with in order to do the search, and then re-bind as
the user when found.
The new patch implements the initial bind using new configuration parameters
ldapbinddn and ldapbindpasswd. I've also added a ldapsearchattribute
just to be complete.
Robert
diff -ru postgresql-8.4-8.4.0-original/src/backend/libpq/auth.c postgresql-8.4-8.4.0/src/backend/libpq/auth.c
--- postgresql-8.4-8.4.0-original/src/backend/libpq/auth.c 2009-06-25 04:30:08.0 -0700
+++ postgresql-8.4-8.4.0/src/backend/libpq/auth.c 2009-09-17 18:12:57.0 -0700
@@ -2150,10 +2150,110 @@
}
}
- snprintf(fulluser, sizeof(fulluser), %s%s%s,
- port-hba-ldapprefix ? port-hba-ldapprefix : ,
- port-user_name,
- port-hba-ldapsuffix ? port-hba-ldapsuffix : );
+ if (port-hba-ldapbasedn)
+ {
+ char filter[NAMEDATALEN + 10]; // FIXME: maybe there's a preferred way to pick this size?
+ LDAPMessage* search_message;
+ char* attributes[2];
+ LDAPMessage* entry;
+ char* dn;
+
+ /* bind for searching */
+ r = ldap_simple_bind_s(ldap,
+ port-hba-ldapbinddn ? port-hba-ldapbinddn : ,
+ port-hba-ldapbindpasswd ? port-hba-ldapbindpasswd : );
+ if (r != LDAP_SUCCESS)
+ {
+ ereport(LOG,
+ (errmsg(LDAP initial bind failed for ldapbinddn \%s\ on server \%s\: error code %d,
+ port-hba-ldapbinddn, port-hba-ldapserver, r)));
+ return STATUS_ERROR;
+ }
+
+ /* fetch just one attribute, else /all/ attributes are returned */
+ attributes[0] = uid;
+ attributes[1] = NULL;
+
+ snprintf(filter, sizeof(filter), (%s=%s),
+ port-hba-ldapsearchattribute ? port-hba-ldapsearchattribute : uid,
+ port-user_name); /* FIXME: sanitize user_name? */
+ filter[sizeof(filter) - 1] = '\0';
+
+ r = ldap_search_s(ldap,
+ port-hba-ldapbasedn,
+ LDAP_SCOPE_SUBTREE,
+ filter,
+ attributes,
+ 0,
+ search_message);
+
+ if (r != LDAP_SUCCESS)
+ {
+ ereport(LOG,
+ (errmsg(LDAP search failed for filter \%s\ on server \%s\: error code %d,
+ filter, port-hba-ldapserver, r)));
+ return STATUS_ERROR;
+ }
+
+ if (ldap_count_entries(ldap, search_message) != 1)
+ {
+ if (ldap_count_entries(ldap, search_message) == 0)
+ ereport(LOG,
+ (errmsg(LDAP search failed for filter \%s\ on server \%s\: no such user,
+ filter, port-hba-ldapserver)));
+ else
+ ereport(LOG,
+ (errmsg(LDAP search failed for filter \%s\ on server \%s\: user is not unique (%d matches),
+ filter, port-hba-ldapserver, ldap_count_entries(ldap, search_message;
+
+ ldap_msgfree(search_message);
+ return STATUS_ERROR;
+ }
+
+ entry = ldap_first_entry(ldap, search_message);
+ dn = ldap_get_dn(ldap, entry);
+ if (dn == NULL)
+ {
+ int error;
+ (void)ldap_get_option(ldap, LDAP_OPT_RESULT_CODE, error);
+ ereport(LOG,
+ (errmsg(LDAP get_dn() for the first entry matching \%s\ on server \%s\: %s,
+ filter, port-hba-ldapserver, ldap_err2string(error;
+ ldap_msgfree(search_message);
+ return
Re: [HACKERS] LDAP where DN does not include UID attribute
On Thu, Sep 17, 2009 at 6:24 PM, Robert Fleming flemi...@gmail.com wrote: The new patch ... s/printf/ereport(LOG,/
