Full_Name: Rob Holland Version: 2.2.26 OS: Gentoo Linux URL: http://bugzilla.padl.com/show_bug.cgi?id=210 Submission from: (NULL) (80.3.128.9) Please see the bug mentioned in the URL which provides a patch to enable clients to use start_tls on referrals via the rebind_proc callback. This is needed in master+slaves infrastructures which require start_tls to be used. This wasn't previously possible as the "are we already using tls?" sanity check in ldap_start_tls_s didn't check the default connection, merely the first connection.
Please note that 3rd party contributions, such as this, cannot be accepted. You should contact the author of this patch and request that he submit his patch for consideration directly to the OpenLDAP Project. -- Kurt At 04:16 PM 6/21/2005, tigger@gentoo.org wrote: >Full_Name: Rob Holland >Version: 2.2.26 >OS: Gentoo Linux >URL: http://bugzilla.padl.com/show_bug.cgi?id=210 >Submission from: (NULL) (80.3.128.9) > > >Please see the bug mentioned in the URL which provides a patch to enable clients >to use start_tls on referrals via the rebind_proc callback. > >This is needed in master+slaves infrastructures which require start_tls to be >used. > >This wasn't previously possible as the "are we already using tls?" sanity check >in ldap_start_tls_s didn't check the default connection, merely the first >connection.
Kurt, I am the author of the patch. Regards, Rob -- rob holland - [ tigger@gentoo.org ] - Gentoo Audit Team [ 5251 4FAC D684 8845 5604 E44F D65C 392F D91B 4729 ]
Please note that without this patch clients are unable to do start_tls on referrals. This leads to security problems when combined with pam_ldap, for example, where in a master+slave setup pam_ldap rebinds to the master following a referral and sends credentials in plaintext even if pam_ldap was configured to do start_tls. If the infrastructure is not set to force tls server-side this might even go unnoticed by the admins, who assume that the ldap.conf setting "ssl start_tls" is being honoured. Regards, Rob -- rob holland - [ tigger@gentoo.org ] - Gentoo Audit Team [ 5251 4FAC D684 8845 5604 E44F D65C 392F D91B 4729 ]
Hi, any news on this? Is it planned to integrate this patch into CVS? To me it look reasonable. -- Ralf
> Hi, > > any news on this? Is it planned to integrate this patch into CVS? To me it > look reasonable. I'd rather say it looks obscure. I'm thinking about something slightly different for back-ldap/meta; in fact, I believe this should go into the ldap_rebind_proc that's supplied by the client and not in the client library itself. In fact, starting TLS on a connection to a different DSA as a consequence of chasing a referral may result in error cases which require client's intervention. So, the fix should go in pam_ldap rather than in libldap. All we should do is provide, in some doc, an example ldap_rebind_proc that retries the original bind, optionally starting TLS if required. Comments? p. -- Pierangelo Masarati mailto:pierangelo.masarati@sys-net.it SysNet - via Dossi,8 27100 Pavia Tel: +390382573859 Fax: +390382476497
rhafer@suse.de wrote: > Hi, > > any news on this? Is it planned to integrate this patch into CVS? To me it > look reasonable. > > I was looking at the patch and wondering why we don't just test ld->ld_defconn here; it seems the test on ld->ld_sb is unnecessary. Didn't have time to investigate further though. Anyway, at present the code isn't clear to me. If someone else is comfortable with it, they should speak up. -- -- Howard Chu Chief Architect, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc OpenLDAP Core Team http://www.openldap.org/project/
> >> Hi, >> >> any news on this? Is it planned to integrate this patch into CVS? To me >> it >> look reasonable. > > I'd rather say it looks obscure. I realize (maybe because of its obscureness?) that I overlooked the issue. Looking closer at both patches, it appears they're just doing what I suggested. I'm not 100% sure about the OpenLDAP side of the fix, though. p. -- Pierangelo Masarati mailto:pierangelo.masarati@sys-net.it SysNet - via Dossi,8 27100 Pavia Tel: +390382573859 Fax: +390382476497
> rhafer@suse.de wrote: >> Hi, >> >> any news on this? Is it planned to integrate this patch into CVS? To me >> it >> look reasonable. >> >> > I was looking at the patch and wondering why we don't just test > ld->ld_defconn here; it seems the test on ld->ld_sb is unnecessary. > Didn't have time to investigate further though. Anyway, at present the > code isn't clear to me. If someone else is comfortable with it, they > should speak up. if one calls ldap_init[ialize](), ld_sb is created, but ld_defconn is not; if one calls ldap_open(), they both get created, and ld_defconn gets ld_sb as Sockbuf. Since one can call ldap_start_tls[_s]() right after ldap_init[ialize](), the test on ld_sb seems reasonable. I don't quite see if and why the Sockbuf in ld_defconn can differ from ld_sb in the first place, so this fix seems useless. Also, I note this control is missing from ldap_start_tls(), the asynchronous version of ldap_start_tls_s(). p. -- Pierangelo Masarati mailto:pierangelo.masarati@sys-net.it SysNet - via Dossi,8 27100 Pavia Tel: +390382573859 Fax: +390382476497
> Also, I note this control is > missing from ldap_start_tls(), the asynchronous version of > ldap_start_tls_s(). I've found the answer: the check is in ldap_install_tls(), which must be called __after__ receiving successful response from the asyncronous call. So, if the check is modified, it must be modified in both places. -- Pierangelo Masarati mailto:pierangelo.masarati@sys-net.it SysNet - via Dossi,8 27100 Pavia Tel: +390382573859 Fax: +390382476497
On Thursday 11 August 2005 09:59, ando@sys-net.it wrote: > > Hi, > > > > any news on this? Is it planned to integrate this patch into CVS? To me > > it look reasonable. > > I'd rather say it looks obscure. > > I'm thinking about something slightly different for back-ldap/meta; in > fact, I believe this should go into the ldap_rebind_proc that's supplied > by the client and not in the client library itself. > In fact, starting TLS > on a connection to a different DSA as a consequence of chasing a referral > may result in error cases which require client's intervention. So, the > fix should go in pam_ldap rather than in libldap. All we should do is > provide, in some doc, an example ldap_rebind_proc that retries the > original bind, optionally starting TLS if required. > > Comments? Hmm, this doesn't work at the moment. In this special case the ldap_rebind_proc of pam_ldap was fixed to start TLS on the referral when pam_ldap is configured to use StartTLS. This doesn't work with the current libldap. It errors out with LDAP_LOCAL_ERROR in ldap_start_tls_s, that's what this patch is supposed to fix. -- Ralf
> Hmm, this doesn't work at the moment. In this special case the > ldap_rebind_proc of pam_ldap was fixed to start TLS on the referral when > pam_ldap is configured to use StartTLS. This doesn't work with the current > libldap. It errors out with LDAP_LOCAL_ERROR in ldap_start_tls_s, that's > what > this patch is supposed to fix. As far as I can tell, start tls is already propagated by libldap on rebind while chasing referrals: if you follow the path, from request.c:ldap_chase_v3referrals() request.c:ldap_send_server_request() request.c:ldap_new_connection() with a newly allocated Sockbuf open.c:ldap_int_open_connection() starts TLS if ( ld->ld_options.ldo_tls_mode == LDAP_OPT_X_TLS_HARD || strcmp( srv->lud_scheme, "ldaps" ) == 0 ) Then request.c:ldap_new_connection() calls ldap_rebind_proc(). So, ldap_rebind_proc() should start TLS only if the above test is negative. Since I don't expect that to be a reliable means to determine if one should start TLS, I think a better fix would be to expose something equivalent to ldap_pvt_tls_inplace(), something like int ldap_tls_inplace( LDAP *ld ) { Sockbuf *sb = NULL; ldap_get_option( lc->lc_ld, LDAP_OPT_SOCKBUF, (void *)&sb ); return ldap_pvt_tls_inplace( sb ); } so that a possible ldap_rebind_proc() would be int rebind_proc( LDAP *ld, const char *url, ber_tag_t request, ber_int_t msgid, void *params ) { if ( !ldap_tls_inplace( ld ) ) { int rc; rc = ldap_start_tls_s( ld, NULL, NULL ); if ( rc != LDAP_SUCCESS ) { return rc; } } return ldap_simple_bind_s( ld, params->dn, params->cred ); } If my analysis is correct, the LDAP library is already doing what expected, and both the rebind proc of the client and the proposed fix are incorrect. My suggestion would just ease writing a correct client. Comments? p. -- Pierangelo Masarati mailto:pierangelo.masarati@sys-net.it SysNet - via Dossi,8 27100 Pavia Tel: +390382573859 Fax: +390382476497
On Thursday 11 August 2005 11:44, ando@sys-net.it wrote: [..] > As far as I can tell, start tls is already propagated by libldap on rebind > while chasing referrals: if you follow the path, from > > request.c:ldap_chase_v3referrals() > request.c:ldap_send_server_request() > request.c:ldap_new_connection() with a newly allocated Sockbuf > > open.c:ldap_int_open_connection() starts TLS > if ( ld->ld_options.ldo_tls_mode == LDAP_OPT_X_TLS_HARD || > strcmp( srv->lud_scheme, "ldaps" ) == 0 ) > > Then > > request.c:ldap_new_connection() calls ldap_rebind_proc(). > > So, ldap_rebind_proc() should start TLS only if the above test is > negative. Since I don't expect that to be a reliable means to determine > if one should start TLS, I think a better fix would be to expose something > equivalent to ldap_pvt_tls_inplace(), something like > > > int > ldap_tls_inplace( LDAP *ld ) > { > Sockbuf *sb = NULL; > > ldap_get_option( lc->lc_ld, LDAP_OPT_SOCKBUF, (void *)&sb ); > > return ldap_pvt_tls_inplace( sb ); > } > > so that a possible ldap_rebind_proc() would be > > int > rebind_proc( LDAP *ld, const char *url, ber_tag_t request, > ber_int_t msgid, void *params ) > { > if ( !ldap_tls_inplace( ld ) ) { > int rc; > > rc = ldap_start_tls_s( ld, NULL, NULL ); But this would again return LDAP_LOCAL_ERROR since ldap_pvt_tls_inplace() is true for ld->ld_sb. If I see it correctly ld->ld_sb still points to the original connection (not to the new connection of the referral). Please correct me if I am wrong here. > if ( rc != LDAP_SUCCESS ) { > return rc; > } > } > > return ldap_simple_bind_s( ld, params->dn, params->cred ); > } -- Ralf
> But this would again return LDAP_LOCAL_ERROR since ldap_pvt_tls_inplace() > is > true for ld->ld_sb. If I see it correctly ld->ld_sb still points to the > original connection (not to the new connection of the referral). Please > correct me if I am wrong here. That's correct. After re-checking the code, it appears that the option LDAP_OPT_X_TLS_HARD is not working as intended. I need to investigate it a bit further. However, the patch looks essentially correct, so I'm going to apply it with some reworking. I think it's reasonable that tls refers to the ld_defconn, so I've modified ldap_tls_inplace() to refer to the Sockbuf of ld_defconn, and used that test inside libldap tls code as well. The test can be performed from outside instead of delegating to the library. p. -- Pierangelo Masarati mailto:pierangelo.masarati@sys-net.it SysNet - via Dossi,8 27100 Pavia Tel: +390382573859 Fax: +390382476497
changed notes changed state Open to Test moved from Incoming to Software Bugs
changed state Test to Release
changed notes changed state Release to Closed
moved from Software Bugs to Archive.Software Bugs
patch applied to HEAD in re23