Full_Name: Singam Sudhir Reddy Version: master branch OS: fedora URL: ftp://ftp.openldap.org/incoming/sudhirsingam-180505.patch Submission from: (NULL) (61.1.232.154) The attached file is derived from OpenLDAP Software. All of the modifications to OpenLDAP Software represented in the following patch(es) were developed by NOKIA. NOKIA has not assigned rights and/or interest in this work to any party. I, SINGAM SUDHIR REDDY authorized by NOKIA, my employer, to release this work under the following terms. NOKIA hereby place the following modifications to OpenLDAP Software (and only these modifications) into the public domain. Hence, these modifications may be freely used and/or redistributed for any purpose with or without attribution and/or other notice. **** Description: Currently when using the openldap client and try to connect to LDAP server using LDAP URL, client automatically binds to an IP address returned by kernel. For example, in the below usage, client automatically binds to an IP address returned by kernel. ldapsearch -H ldap://10.63.57.239:389 D "uid=admin, ou=administrators, ou=topologymanagement, o=netscaperoot" -x -w admin -b "uid=baha, ou=people, ou=accounts, ou=region-911080, ou=regions, ou=netact, dc=noklab, dc=net, dc=localdomain" But if we want to route the traffic on a specific interface/IP address, currently there is no provision. And the idea or enhancement is to introduce such provision by giving source bind IP address in the URL in the following format. ldap://TARGET-IP-ADDRESS@SOURCE-BIND-IP-ADDRESS:PORT For example, ldapsearch -H ldap://10.63.57.239@10.37.220.9:389 D "uid=admin, ou=administrators, ou=topologymanagement, o=netscaperoot" -x -w admin -b "uid=baha, ou=people, ou=accounts, ou=region-911080, ou=regions, ou=netact, dc=noklab, dc=net, dc=localdomain" Note this feature is backward compatible, that is, it is optional to provide source bind IP address in the URL. This feature also supports IPV6 addresses.
Adding a source IP to an URI feels wrong to it. I have not read RFC dealing with URI, however having a quick look [1] seems to indicate that using the at sign in this way is non-standard. Regardless of the syntax, I don't think a Uniform Resource Identifier is the right place to add source IP information. An LDAP URI typically refer to a (usually remote) LDAP server or servers. It's all about the destination. A source IP is machine specific. I think a separate option would make more sense. Any specific reason for wanting to add it in the URI? I am not an OpenLDAP developer/contributor, this is just my opinion. [1] https://en.wikipedia.org/wiki/Uniform_Resource_Identifier On Sun, 2018-05-06 at 06:15 +0000, sudhir.singam@nokia.com wrote: > Full_Name: Singam Sudhir Reddy > Version: master branch > OS: fedora > URL: ftp://ftp.openldap.org/incoming/sudhirsingam-180505.patch > Submission from: (NULL) (61.1.232.154) > > > The attached file is derived from OpenLDAP Software. All of the modifications > to > OpenLDAP Software represented in the following patch(es) were developed by > NOKIA. NOKIA has not assigned rights and/or interest in this work to any > party. > I, SINGAM SUDHIR REDDY authorized by NOKIA, my employer, to release this work > under the following terms. > > NOKIA hereby place the following modifications to OpenLDAP Software (and only > these modifications) into the public domain. Hence, these modifications may be > freely used and/or redistributed for any purpose with or without attribution > and/or other notice. > > **** > > Description: > > Currently when using the openldap client and try to connect to LDAP server > using > LDAP URL, client automatically binds to an IP address returned by kernel. > > For example, in the below usage, client automatically binds to an IP address > returned by kernel. > > ldapsearch -H ldap://10.63.57.239:389 D "uid=admin, ou=administrators, > ou=topologymanagement, o=netscaperoot" -x -w admin -b "uid=baha, ou=people, > ou=accounts, ou=region-911080, ou=regions, ou=netact, dc=noklab, dc=net, > dc=localdomain" > > But if we want to route the traffic on a specific interface/IP address, > currently there is no provision. And the idea or enhancement is to introduce > such provision by giving source bind IP address in the URL in the following > format. > > ldap://TARGET-IP-ADDRESS@SOURCE-BIND-IP-ADDRESS:PORT > > For example, > > ldapsearch -H ldap://10.63.57.239@10.37.220.9:389 D "uid=admin, > ou=administrators, ou=topologymanagement, o=netscaperoot" -x -w admin -b > "uid=baha, ou=people, ou=accounts, ou=region-911080, ou=regions, ou=netact, > dc=noklab, dc=net, dc=localdomain" > > Note this feature is backward compatible, that is, it is optional to provide > source bind IP address in the URL. > > This feature also supports IPV6 addresses. > >
On Sun, May 06, 2018 at 01:50:23PM +0000, arekkusu@r42.ch wrote: >Adding a source IP to an URI feels wrong to it. > >I have not read RFC dealing with URI, however having a quick look [1] seems to >indicate that using the at sign in this way is non-standard. I agree. @ in URIs is already defined as separating credentials (or just username) from the host. I don't recall whether OpenLDAP supports that usage but in any case we shouldn't re-define it. I believe ITS#8654 is about the same feature? That one implemented this by copying a Microsoft option, LDAP_OPT_SOCKET_BIND_ADDRESSES. I would think that's probably a better approach. Maybe you could pick up where the author of that one left off? He disappeared after posting his patch for review... thanks Ryan
ryan@openldap.org wrote: > On Sun, May 06, 2018 at 01:50:23PM +0000, arekkusu@r42.ch wrote: >> Adding a source IP to an URI feels wrong to it. >> >> I have not read RFC dealing with URI, however having a quick look [1] seems to >> indicate that using the at sign in this way is non-standard. > > I agree. @ in URIs is already defined as separating credentials (or just > username) from the host. I don't recall whether OpenLDAP supports that > usage but in any case we shouldn't re-define it. Agreed. URI syntax is pretty thoroughly specified in multiple RFCs, nobody can just arbitrarily decide to change it. And the point of a URI is that it is valid for a destination no matter who/where the source is. This patch completely breaks the function and intent of URIs. Closing this ITS. > I believe ITS#8654 is about the same feature? That one implemented this > by copying a Microsoft option, LDAP_OPT_SOCKET_BIND_ADDRESSES. I would > think that's probably a better approach. Maybe you could pick up where > the author of that one left off? He disappeared after posting his patch > for review... > > thanks > Ryan > > > > -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
hyc@symas.com wrote: > ryan@openldap.org wrote: >> On Sun, May 06, 2018 at 01:50:23PM +0000, arekkusu@r42.ch wrote: >>> Adding a source IP to an URI feels wrong to it. >>> >>> I have not read RFC dealing with URI, however having a quick look [1] seems to >>> indicate that using the at sign in this way is non-standard. >> >> I agree. @ in URIs is already defined as separating credentials (or just >> username) from the host. I don't recall whether OpenLDAP supports that >> usage but in any case we shouldn't re-define it. > > Agreed. URI syntax is pretty thoroughly specified in multiple RFCs, nobody can > just arbitrarily decide to change it. And the point of a URI is that it is > valid for a destination no matter who/where the source is. > > This patch completely breaks the function and intent of URIs. IMO having the capability to specify the source IP is very useful in multi-homed host setups with strict network security. But of course one should not invent new URL syntax or abuse existing definitions. RFC 4516 indeed specifies LDAP URL extensions and this should be used. This also has the advantage that e.g. python-ldap's LDAP URL parser can also be used for that. Ideally one could write a very short I-D for such an extension. Ciao, Michael.
Hi, From NOKIA, we are picking up this patch -> to add new ldap option, to be able to set specific IPv4/IPv6 bind address. We will come back with design details soon. Regards, Sudhir Singam DELIVERING BEST-IN-CLASS PLATFORM is our vision -----Original Message----- From: Ryan Tandy <ryan@openldap.org> Sent: Sunday, May 06, 2018 10:25 PM To: openldap-its@OpenLDAP.org; Singam, Sudhir (Nokia - IN/Bangalore) <sudhir.singam@nokia.com> Cc: arekkusu@r42.ch Subject: Re: (ITS#8847) New LDAP URL syntax to support binding to specific IP address at client side On Sun, May 06, 2018 at 01:50:23PM +0000, arekkusu@r42.ch wrote: >Adding a source IP to an URI feels wrong to it. > >I have not read RFC dealing with URI, however having a quick look [1] seems to >indicate that using the at sign in this way is non-standard. I agree. @ in URIs is already defined as separating credentials (or just username) from the host. I don't recall whether OpenLDAP supports that usage but in any case we shouldn't re-define it. I believe ITS#8654 is about the same feature? That one implemented this by copying a Microsoft option, LDAP_OPT_SOCKET_BIND_ADDRESSES. I would think that's probably a better approach. Maybe you could pick up where the author of that one left off? He disappeared after posting his patch for review... thanks Ryan
changed notes
has patch;IPR OK See also ITS#8654, ITS#8930
Hi, I have prepared and uploaded the following patch, according to earlier discussed design (in e-mail with "openldap-devel@openldap.org" mailing list) Patch URL: ftp://ftp.openldap.org/incoming/Ramakant-sharma-181128.patch Also copying here the discussed design " Requirement: User shall be able to set IPv4/IPv6 socket bind address to be able to route the LDAP traffic via desired network interface. Based on the target IP address type, matching IP address will be picked for explicit binding at client side. Work items: 1. LDAP option to set the IPv4/IPv6 socket bind addresses. /Format: space separated list of IP addresses/ New configuration option LDAP_OPT_SOCKET_BIND_ADDRESSES (0x5013) is introduced (in ldap.h) to be used via ldap_set_option. For example, char* p = "10.24.56.34 2001:0db8:85a3:0000:0000:8a2e:0370:7334"; ldap_set_option(NULL, LDAP_OPT_SOCKET_BIND_ADDRESSES, p); Bind addresses can also be provided in ldap.conf file via the option "SOCKET_BIND_ADDRESSES" Valid examples: SOCKET_BIND_ADDRESSES 10.24.56.45 2001:0db8:85a3:0000:0000:8a2e:0370:7334 SOCKET_BIND_ADDRESSES 10.24.56.45 SOCKET_BIND_ADDRESSES 2001:0db8:85a3:0000:0000:8a2e:0370:7334 SOCKET_BIND_ADDRESSES 2001:0db8:85a3:0000:0000:8a2e:0370:7334 10.24.56.45 Invalid examples: SOCKET_BIND_ADDRESSES 2001:0db8:85a3:0000:0000:8a2e:0370:7334 2001:0db8:85a3:0000:0000:8a2e:0370:7335 SOCKET_BIND_ADDRESSES 10.24.56.45 10.24.56.47 Note : *Option set to LDAP handle will override the global option. *Setting the option multiple times will override the previous values but does not append. 2. Parsing & validations *Space separated IP addresses will be parsed & validated. *Basic syntax validation will be done for IPv4 or IPv6 addresses, if any error, setting of the option will fail and LDAP client will use the default IP address or previously successfully validated IP addresses provided by set option. *If multiple IPv4 or multiple IPv6 address is set, validation will fail. "ldapoptions" structure in ldap-int.h is modified to add new variable to hold given IPv4 and IPv6 address. char** ldo_local_IP_addresses New functions /ldap_parse_and_validate_sourceip (), get_IPv4_from_lst () and get_IPv6_from_lst() are introduced in newly added file bind_ip.c.c to parse, validate and store the IP address to " ldo_local_IP_addresses" variable and to fetch respective IP. If ldap_parse_and_validate_sourceip () fails to parse & validate the addresses successfully then previously set IP address will not be overwritten. If there were no previous address then default kernel address will be used during connection. 3. Using Bind IP addresses during connection *After the connection socket is created (ldap_int_socket) and before it is connected (ldap_pvt_connect). *Check if the target address family type, If it is AF_INET, IPv4 bind *If the list is empty means there were no addresses provided from user, then default kernel provided address will be used for binding the interface. *If the list is not empty and not able to bind to provided IPv4 address, connection will fail> *if the list is not empty and it just contains IPV6 address then default kernel provided IPv4 address will be used for binding the interface. *If it is AF_INET6, IPv6 bind address will be used from the list. *If the list is not empty and not able to bind to provided IPv6 addresses, connection will fail. *if the list is not empty and it just contains IPV4 address then default kernel provided IPv6 address will be used for binding the interface. *If the list is empty then LDAP client will continue to use the kernel provided IPv6 address. " Kindly Review the patch and provide your valuable inputs on how to proceed further. BR, Ramakant Sharma Senior R&D Engineer Nokia Networks - Bangalore
Thanks for the updated patch. diff --git a/include/ldap.h b/include/ldap.h index c58c576d3..78e07d49f 100644 --- a/include/ldap.h +++ b/include/ldap.h @@ -138,6 +138,7 @@ LDAP_BEGIN_DECL #define LDAP_OPT_CONNECT_ASYNC 0x5010 /* create connections asynchronously */ #define LDAP_OPT_CONNECT_CB 0x5011 /* connection callbacks */ #define LDAP_OPT_SESSION_REFCNT 0x5012 /* session reference count */ +#define LDAP_OPT_SOCKET_BIND_ADDRESSES 0x5013 /* user configured bind IPs */ /* OpenLDAP TLS options */ #define LDAP_OPT_X_TLS 0x6000 You've got strange whitespace here. Please re-read the CONTRIBUTING docs; we use 4-column tab stops. diff --git a/libraries/libldap/bind_ip.c b/libraries/libldap/bind_ip.c new file mode 100644 index 000000000..56eddda43 --- /dev/null +++ b/libraries/libldap/bind_ip.c @@ -0,0 +1,102 @@ +/* LIBLDAP bind_ip.c*/ +/* $OpenLDAP$ */ +/* This work is part of OpenLDAP Software <http://www.openldap.org/>. + * I see no reason why these functions are exposed in their own source file or exported from libldap. parse_and_validate_sourceip() could just be a static function inside options.c get_IPvX_from_lst() can just be statics in os-ip.c. In particular, you've defined these as global but they don't have an ldap_ prefix, which is just wrong. The work of parse_and_validate is wasted since you call inet_pton both for validating and for using the result later. You should simply save the IPv4 and IPv6 binary addresses when the validation succeeds. There should be no get_IPvX_from_lst() functions at all. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Hi Howard, Big thanks for your valuable inputs. >> You've got strange whitespace here. Please re-read the CONTRIBUTING docs; we use >>4-column tab stops. Yes, I must fix it in next patch. >> I see no reason why these functions are exposed in their own source file or >>exported from libldap. parse_and_validate_sourceip() could just be a static function inside >>options.c >> get_IPvX_from_lst() can just be statics in os-ip.c. In particular, you've >>defined these as global but they don't have an ldap_ prefix, which is just wrong. Okay. I will remove newly created file (bind_ip.c) and will keep parse_and_validate_sourceip() and other getters as per your suggestion. >> The work of parse_and_validate is wasted since you call inet_pton both for >>validating and for using the result later. You should simply save the IPv4 and IPv6 binary >>addresses when the validation succeeds. There should be no get_IPvX_from_lst() functions >>at all. I am bit confused regarding this point. The intent of parse_and_validate_sourceip() is to set only valid internet address to "ldo_local_IP_addresses" so that we don't overwrite "ldo_local_IP_addresses" in case user provides some invalid addresses either from ldap.conf or from application. Now during connect, the need is to get either IPv4 or IPv6 internet address based on target LDAP server's address family type. So, once it is known that target LDAP server is using IPv4 or IPv6. Code will fetch the related configured bind IP from "ldo_local_IP_addresses". Since at this step we are not sure at which index the related bind IP is present so, I used inet_pton call to get the matching internet bind IP (internet address) from the list (intent was not to use binary address) If the concern is to avoid inet_pton call at this stage then, I can use "." to search IPv4 and ":" to search IPv6 address in respective getters (because we already know that at this stage the fetched IP will always be valid) Kindly let me know your opinion. BR, Ramakant Sharma Senior R&D Engineer Nokia Networks, Bangalore
Sharma, Ramakant 2. (Nokia - IN/Bangalore) wrote: >>> The work of parse_and_validate is wasted since you call inet_pton both for >>>validating and for using the result later. You should simply save the IPv4 and IPv6 binary >>>addresses when the validation succeeds. There should be no get_IPvX_from_lst() functions >>>at all. > > I am bit confused regarding this point. > > The intent of parse_and_validate_sourceip() is to set only valid internet address to "ldo_local_IP_addresses" so that we dont overwrite > ldo_local_IP_addresses in case user provides some invalid addresses either from ldap.conf or from application. Sure, the validation step is required. The result of validation is either a struct in_addr or a struct in6_addr. You should simply store them directly in the ldapoptions struct. Maybe add a flag to tell that each one has been set. > Now during connect, the need is to get either IPv4 or IPv6 internet address based on target LDAP server's address family type. So, once it is known that target > LDAP server is using IPv4 or IPv6. Code will fetch the related configured bind IP from "ldo_local_IP_addresses". Yes. All that should be required here is to fetch the struct in_addr or in6_addr value accordingly. > > Since at this step we are not sure at which index the related bind IP is present so, I used inet_pton call to get the matching internet bind IP (internet > address) from the list (intent was not to use binary address) This is stupid and wasted work. > If the concern is to avoid inet_pton call at this stage then, I can use "." to search IPv4 and ":" to search IPv6 address in respective getters (because we > already know that at this stage the fetched IP will always be valid) This is stupid and wasted work. > Kindly let me know your opinion. > > > > BR, > > Ramakant Sharma > > Senior R&D Engineer > > Nokia Networks, Bangalore > > > -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Hi Howard, I have modified the code as per the above mentioned comments. All comments are incorporated and uploaded the latest patch. Kindly find the URL: ftp://ftp.openldap.org/incoming/ramakant-sharma-190121.patch Please provide your valuable inputs to take it forward. thanks Ryan
Hi Howard, I have uploaded the new patch which addresses the previous comments. Kindly check and provide your valuable comments . ftp://ftp.openldap.org/incoming/ramakant-sharma-190121.patch BR, Ramakant Sharma -----Original Message----- From: Howard Chu <hyc@symas.com> Sent: Tuesday, December 11, 2018 5:43 PM To: Sharma, Ramakant 2. (Nokia - IN/Bangalore) <ramakant.2.sharma@nokia.com>; openldap-its@OpenLDAP.org Cc: Singam, Sudhir (Nokia - IN/Bangalore) <sudhir.singam@nokia.com> Subject: Re: (ITS#8847) New LDAP URL syntax to support binding to specific IP address at client side Sharma, Ramakant 2. (Nokia - IN/Bangalore) wrote: >>> The work of parse_and_validate is wasted since you call inet_pton >>>both for validating and for using the result later. You should simply >>>save the IPv4 and IPv6 binary addresses when the validation succeeds. >>>There should be no get_IPvX_from_lst() functions at all. > > I am bit confused regarding this point. > > The intent of parse_and_validate_sourceip() is to set only valid > internet address to "ldo_local_IP_addresses" so that we don't overwrite "ldo_local_IP_addresses" in case user provides some invalid addresses either from ldap.conf or from application. Sure, the validation step is required. The result of validation is either a struct in_addr or a struct in6_addr. You should simply store them directly in the ldapoptions struct. Maybe add a flag to tell that each one has been set. > Now during connect, the need is to get either IPv4 or IPv6 internet > address based on target LDAP server's address family type. So, once it is known that target LDAP server is using IPv4 or IPv6. Code will fetch the related configured bind IP from "ldo_local_IP_addresses". Yes. All that should be required here is to fetch the struct in_addr or in6_addr value accordingly. > > Since at this step we are not sure at which index the related bind IP > is present so, I used inet_pton call to get the matching internet bind > IP (internet > address) from the list (intent was not to use binary address) This is stupid and wasted work. > If the concern is to avoid inet_pton call at this stage then, I can > use "." to search IPv4 and ":" to search IPv6 address in respective > getters (because we already know that at this stage the fetched IP > will always be valid) This is stupid and wasted work. > Kindly let me know your opinion. > > > > BR, > > Ramakant Sharma > > Senior R&D Engineer > > Nokia Networks, Bangalore > > > -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Hi Howard, Please provide your valuable inputs for the below patch. Also let us know how to proceed further. BR, Ramakant Sharma -----Original Message----- From: Sharma, Ramakant 2. (Nokia - IN/Bangalore) Sent: Monday, January 28, 2019 10:10 AM To: Howard Chu <hyc@symas.com>; openldap-its@OpenLDAP.org Cc: Singam, Sudhir (Nokia - IN/Bangalore) <sudhir.singam@nokia.com> Subject: RE: (ITS#8847) New LDAP URL syntax to support binding to specific IP address at client side Hi Howard, I have uploaded the new patch which addresses the previous comments. Kindly check and provide your valuable comments . ftp://ftp.openldap.org/incoming/ramakant-sharma-190121.patch BR, Ramakant Sharma -----Original Message----- From: Howard Chu <hyc@symas.com> Sent: Tuesday, December 11, 2018 5:43 PM To: Sharma, Ramakant 2. (Nokia - IN/Bangalore) <ramakant.2.sharma@nokia.com>; openldap-its@OpenLDAP.org Cc: Singam, Sudhir (Nokia - IN/Bangalore) <sudhir.singam@nokia.com> Subject: Re: (ITS#8847) New LDAP URL syntax to support binding to specific IP address at client side Sharma, Ramakant 2. (Nokia - IN/Bangalore) wrote: >>> The work of parse_and_validate is wasted since you call inet_pton >>>both for validating and for using the result later. You should simply >>>save the IPv4 and IPv6 binary addresses when the validation succeeds. >>>There should be no get_IPvX_from_lst() functions at all. > > I am bit confused regarding this point. > > The intent of parse_and_validate_sourceip() is to set only valid > internet address to "ldo_local_IP_addresses" so that we don't overwrite "ldo_local_IP_addresses" in case user provides some invalid addresses either from ldap.conf or from application. Sure, the validation step is required. The result of validation is either a struct in_addr or a struct in6_addr. You should simply store them directly in the ldapoptions struct. Maybe add a flag to tell that each one has been set. > Now during connect, the need is to get either IPv4 or IPv6 internet > address based on target LDAP server's address family type. So, once it is known that target LDAP server is using IPv4 or IPv6. Code will fetch the related configured bind IP from "ldo_local_IP_addresses". Yes. All that should be required here is to fetch the struct in_addr or in6_addr value accordingly. > > Since at this step we are not sure at which index the related bind IP > is present so, I used inet_pton call to get the matching internet bind > IP (internet > address) from the list (intent was not to use binary address) This is stupid and wasted work. > If the concern is to avoid inet_pton call at this stage then, I can > use "." to search IPv4 and ":" to search IPv6 address in respective > getters (because we already know that at this stage the fetched IP > will always be valid) This is stupid and wasted work. > Kindly let me know your opinion. > > > > BR, > > Ramakant Sharma > > Senior R&D Engineer > > Nokia Networks, Bangalore > > > -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Sharma, Ramakant 2. (Nokia - IN/Bangalore) wrote: > Hi Howard, > > Please provide your valuable inputs for the below patch. It is a little strange that ldap_set_option() takes a string but ldap_get_option() returns a binary structure. get/set should be totally symmetric. It would be simplest to just save a copy of the string given to set and return the string in get. In libldap/os-ip.c, you should not be using ldap_get_option() to retrieve the values. You're operating inside libldap, you can access the structure directly. In particular, it's a waste to use ldap_get_option here and create a dynamically allocated copy of the address struct. Look at how the existing code works. Notice that it just uses ld->ld_options directly. This is rule #1 in software development - when extending existing code, your new code should do things the same way existing code does. Corollaries to this rule are: don't patch code without reading it first, and don't patch code without studying its complete context. > > Also let us know how to proceed further. > > BR, > Ramakant Sharma > > -----Original Message----- > From: Sharma, Ramakant 2. (Nokia - IN/Bangalore) > Sent: Monday, January 28, 2019 10:10 AM > To: Howard Chu <hyc@symas.com>; openldap-its@OpenLDAP.org > Cc: Singam, Sudhir (Nokia - IN/Bangalore) <sudhir.singam@nokia.com> > Subject: RE: (ITS#8847) New LDAP URL syntax to support binding to specific IP address at client side > > Hi Howard, > > I have uploaded the new patch which addresses the previous comments. > > Kindly check and provide your valuable comments . > > ftp://ftp.openldap.org/incoming/ramakant-sharma-190121.patch > > BR, > Ramakant Sharma > -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Hi Howard, Thanks for valuable comments. I have accepted and incorporated both the comments and uploaded the new patch. ftp://ftp.openldap.org/incoming/Ramakant-Sharma-190221.patch Please check and do the needful. BR, Ramakant Sharma -----Original Message----- From: Howard Chu <hyc@symas.com> Sent: Wednesday, February 13, 2019 3:40 PM To: Sharma, Ramakant 2. (Nokia - IN/Bangalore) <ramakant.2.sharma@nokia.com>; openldap-its@OpenLDAP.org Cc: Singam, Sudhir (Nokia - IN/Bangalore) <sudhir.singam@nokia.com> Subject: Re: (ITS#8847) New LDAP URL syntax to support binding to specific IP address at client side Sharma, Ramakant 2. (Nokia - IN/Bangalore) wrote: > Hi Howard, > > Please provide your valuable inputs for the below patch. It is a little strange that ldap_set_option() takes a string but ldap_get_option() returns a binary structure. get/set should be totally symmetric. It would be simplest to just save a copy of the string given to set and return the string in get. In libldap/os-ip.c, you should not be using ldap_get_option() to retrieve the values. You're operating inside libldap, you can access the structure directly. In particular, it's a waste to use ldap_get_option here and create a dynamically allocated copy of the address struct. Look at how the existing code works. Notice that it just uses ld->ld_options directly. This is rule #1 in software development - when extending existing code, your new code should do things the same way existing code does. Corollaries to this rule are: don't patch code without reading it first, and don't patch code without studying its complete context. > > Also let us know how to proceed further. > > BR, > Ramakant Sharma > > -----Original Message----- > From: Sharma, Ramakant 2. (Nokia - IN/Bangalore) > Sent: Monday, January 28, 2019 10:10 AM > To: Howard Chu <hyc@symas.com>; openldap-its@OpenLDAP.org > Cc: Singam, Sudhir (Nokia - IN/Bangalore) <sudhir.singam@nokia.com> > Subject: RE: (ITS#8847) New LDAP URL syntax to support binding to > specific IP address at client side > > Hi Howard, > > I have uploaded the new patch which addresses the previous comments. > > Kindly check and provide your valuable comments . > > ftp://ftp.openldap.org/incoming/ramakant-sharma-190121.patch > > BR, > Ramakant Sharma > -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Hi Howard, Thanks for valuable comments. I have accepted and incorporated both the comments and uploaded the new patch. ftp://ftp.openldap.org/incoming/Ramakant-Sharma-190221.patch Please check and do the needful. BR, Ramakant Sharma
Hi Howard, Please share your valuable inputs for the patch. Also, let us know how to proceed further. BR, Ramakant Sharma -----Original Message----- From: Sharma, Ramakant 2. (Nokia - IN/Bangalore) Sent: Thursday, February 21, 2019 4:55 PM To: 'Howard Chu' <hyc@symas.com>; openldap-its@OpenLDAP.org Cc: Singam, Sudhir (Nokia - IN/Bangalore) <sudhir.singam@nokia.com> Subject: RE: (ITS#8847) New LDAP URL syntax to support binding to specific IP address at client side Hi Howard, Thanks for valuable comments. I have accepted and incorporated both the comments and uploaded the new patch. ftp://ftp.openldap.org/incoming/Ramakant-Sharma-190221.patch Please check and do the needful. BR, Ramakant Sharma -----Original Message----- From: Howard Chu <hyc@symas.com> Sent: Wednesday, February 13, 2019 3:40 PM To: Sharma, Ramakant 2. (Nokia - IN/Bangalore) <ramakant.2.sharma@nokia.com>; openldap-its@OpenLDAP.org Cc: Singam, Sudhir (Nokia - IN/Bangalore) <sudhir.singam@nokia.com> Subject: Re: (ITS#8847) New LDAP URL syntax to support binding to specific IP address at client side Sharma, Ramakant 2. (Nokia - IN/Bangalore) wrote: > Hi Howard, > > Please provide your valuable inputs for the below patch. It is a little strange that ldap_set_option() takes a string but ldap_get_option() returns a binary structure. get/set should be totally symmetric. It would be simplest to just save a copy of the string given to set and return the string in get. In libldap/os-ip.c, you should not be using ldap_get_option() to retrieve the values. You're operating inside libldap, you can access the structure directly. In particular, it's a waste to use ldap_get_option here and create a dynamically allocated copy of the address struct. Look at how the existing code works. Notice that it just uses ld->ld_options directly. This is rule #1 in software development - when extending existing code, your new code should do things the same way existing code does. Corollaries to this rule are: don't patch code without reading it first, and don't patch code without studying its complete context. > > Also let us know how to proceed further. > > BR, > Ramakant Sharma > > -----Original Message----- > From: Sharma, Ramakant 2. (Nokia - IN/Bangalore) > Sent: Monday, January 28, 2019 10:10 AM > To: Howard Chu <hyc@symas.com>; openldap-its@OpenLDAP.org > Cc: Singam, Sudhir (Nokia - IN/Bangalore) <sudhir.singam@nokia.com> > Subject: RE: (ITS#8847) New LDAP URL syntax to support binding to > specific IP address at client side > > Hi Howard, > > I have uploaded the new patch which addresses the previous comments. > > Kindly check and provide your valuable comments . > > ftp://ftp.openldap.org/incoming/ramakant-sharma-190121.patch > > BR, > Ramakant Sharma > -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Hi Howard, Please provide your valuable inputs for the patch in trailing mail. BR, Ramakant Sharma -----Original Message----- From: Sharma, Ramakant 2. (Nokia - IN/Bangalore) Sent: Monday, March 4, 2019 9:47 AM To: 'Howard Chu' <hyc@symas.com>; 'openldap-its@OpenLDAP.org' <openldap-its@OpenLDAP.org> Cc: Singam, Sudhir (Nokia - IN/Bangalore) <sudhir.singam@nokia.com> Subject: RE: (ITS#8847) New LDAP URL syntax to support binding to specific IP address at client side Hi Howard, Please share your valuable inputs for the patch. Also, let us know how to proceed further. BR, Ramakant Sharma -----Original Message----- From: Sharma, Ramakant 2. (Nokia - IN/Bangalore) Sent: Thursday, February 21, 2019 4:55 PM To: 'Howard Chu' <hyc@symas.com>; openldap-its@OpenLDAP.org Cc: Singam, Sudhir (Nokia - IN/Bangalore) <sudhir.singam@nokia.com> Subject: RE: (ITS#8847) New LDAP URL syntax to support binding to specific IP address at client side Hi Howard, Thanks for valuable comments. I have accepted and incorporated both the comments and uploaded the new patch. ftp://ftp.openldap.org/incoming/Ramakant-Sharma-190221.patch Please check and do the needful. BR, Ramakant Sharma -----Original Message----- From: Howard Chu <hyc@symas.com> Sent: Wednesday, February 13, 2019 3:40 PM To: Sharma, Ramakant 2. (Nokia - IN/Bangalore) <ramakant.2.sharma@nokia.com>; openldap-its@OpenLDAP.org Cc: Singam, Sudhir (Nokia - IN/Bangalore) <sudhir.singam@nokia.com> Subject: Re: (ITS#8847) New LDAP URL syntax to support binding to specific IP address at client side Sharma, Ramakant 2. (Nokia - IN/Bangalore) wrote: > Hi Howard, > > Please provide your valuable inputs for the below patch. It is a little strange that ldap_set_option() takes a string but ldap_get_option() returns a binary structure. get/set should be totally symmetric. It would be simplest to just save a copy of the string given to set and return the string in get. In libldap/os-ip.c, you should not be using ldap_get_option() to retrieve the values. You're operating inside libldap, you can access the structure directly. In particular, it's a waste to use ldap_get_option here and create a dynamically allocated copy of the address struct. Look at how the existing code works. Notice that it just uses ld->ld_options directly. This is rule #1 in software development - when extending existing code, your new code should do things the same way existing code does. Corollaries to this rule are: don't patch code without reading it first, and don't patch code without studying its complete context. > > Also let us know how to proceed further. > > BR, > Ramakant Sharma > > -----Original Message----- > From: Sharma, Ramakant 2. (Nokia - IN/Bangalore) > Sent: Monday, January 28, 2019 10:10 AM > To: Howard Chu <hyc@symas.com>; openldap-its@OpenLDAP.org > Cc: Singam, Sudhir (Nokia - IN/Bangalore) <sudhir.singam@nokia.com> > Subject: RE: (ITS#8847) New LDAP URL syntax to support binding to > specific IP address at client side > > Hi Howard, > > I have uploaded the new patch which addresses the previous comments. > > Kindly check and provide your valuable comments . > > ftp://ftp.openldap.org/incoming/ramakant-sharma-190121.patch > > BR, > Ramakant Sharma > -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Hi Howard/Team, Please share your inputs on latest patch. Also, let us know how to proceed further. BR, Ramakant Sharma
Hi Howard/Team Kindly provide your valuable inputs for the below patch. ftp://ftp.openldap.org/incoming/Ramakant-Sharma-190221.patch Also, let us know how to proceed further. BR, Ramakant Sharma
Hi Howard/Team Please provide your valuable comments for the patch. Waiting for your kind response. BR, Ramakant Sharma
Hi Howard/Team, We are keenly waiting for the feedback on updated patch. Kindly provide your valuable inputs and suggest how to take it forward. BR, Ramakant Sharma
Created attachment 679 [details] sudhirsingam-2018-05-05.patch
Created attachment 680 [details] Ramakant-Sharma-190221.patch
*** Issue 8654 has been marked as a duplicate of this issue. ***
https://git.openldap.org/openldap/openldap/-/merge_requests/16
*** Issue 8893 has been marked as a duplicate of this issue. ***
Hello, I have been reviewing and testing this patch and I think that there are a number of issues, some less severe and some more, that should still be addressed. In general the patch does not seem well adapted to the surrounding code. For example things have been added at random positions in lists that previously were sorted, and the whitespace style (and code style generally) are quite different from the existing code. Also, the new code does not seem to respect the configure option (and #ifdefs etc) for disabling IPv6 support. doc/man/man3/ldap_get_option.3: - LDAP_OPT_SOCKET_BIND_ADDRESSES added at the wrong place doc/man/man5/ldap.conf.5: - SOCKET_BIND_ADDRESSES added at the wrong place - typo (seperated -> separated) libraries/libldap/ldap-int.h: - /* pull in netinet/in */ is a useless comment - fails to compile under MinGW (there is no netinet/in.h header) -> I could be wrong but 'struct in_addr' feels rather low-level for this file? but I'm not sure what a better design would look like... - should not include IPv6 bits if IPv6 disabled - LDAP_LDO_NULLARG has not been updated (gcc generates a warning) - if ITS#6567 is finished before this one, MAX_LDAP_ADDR_LEN will probably need an update ("GSSAPI_ALLOW_REMOTE_PRINCIPAL" is longer than "SOCKET_BIND_ADDRESSES" is longer than "TLS_CIPHER_SUITE") libraries/libldap/options.c: - in ldap_set_option: other options reset to default when invalue == NULL, it would be nice if this would do the same - ldap_validate_and_fill_sourceip feels a bit weird again, there are no other similar functions in this file... maybe os-ip.c or util-int.c? - in the existing code, inet_pton is only used if LDAP_PF_INET6; should probably follow that pattern (there is also HAVE_INET_NTOP...) libraries/libldap/os-ip.c: - possibly the new code should be in ldap_int_prepare_socket()? not sure... - address family mismatch (only one bind address specified and socket uses the other family) ignored; should we try to catch it? -> MS implementation returns LDAP_SERVER_DOWN when this happens
Created attachment 803 [details] client_address_binding_2021-03-05
Hi, I would like to take over this task. @Ryan Tandy thank you for your great feedback, I have tried to work everything into the new patch. -whitespace style -sorted lists and typos -IPv6 ifdefs -useless comments -add initial NULLARG for new struct -ldap_set_option reset to default if invalue==NULL -ldap_validate_and_fill_sourceip moved to new position -inet_pton is only used if LDAP_PF_INET6 like in existing code -os-ip.c i didn't changed the position of the code. For me it was cleaner if we set address (local or remote) in one place Im not sure about the MinGW Compile Fix - part Is there an instruction how the mingw environment should be set up to retest it?
(In reply to HoweverAT from comment #33) You will need to provide an IPR statement as per https://www.openldap.org/devel/contributing.html#notice for this patch to be looked at.
Created attachment 804 [details] Client Address Binding Option A new try. I also updated the patch against latest. The attached patch file is derived from OpenLDAP Software. All of the modifications to OpenLDAP Software represented in the following patch(es) were developed by Lukas Wimmer laeufer4321@gmx.at. I have not assigned rights and/or interest in this work to any party. I, Lukas Wimmer, hereby place the following modifications to OpenLDAP Software (and only these modifications) into the public domain. Hence, these modifications may be freely used and/or redistributed for any purpose with or without attribution and/or other notice.
Since I've just been tinkering with debug output, this came to mind. With this patch, it would be a good idea to add the locally bound IP:port to the debug output somewhere, e.g. in ldap_dump_connection. Should probably use the recently added lutil_sockaddrstr, and that function likely should be moved into ldap_pvt_ instead of lutil.
(In reply to Howard Chu from comment #36) > Since I've just been tinkering with debug output, this came to mind. With > this patch, it would be a good idea to add the locally bound IP:port to the > debug output somewhere, e.g. in ldap_dump_connection. Should probably use > the recently added lutil_sockaddrstr, and that function likely should be > moved into ldap_pvt_ instead of lutil. Hello, thank you for your input. I tried to add to ldap_dump_connection( see the "from" part, which is always printed out independet of ) e.g. Output: ** ld 0x55974c710680 Connections: * host: localhost port: 389 (default) * from: IP=127.0.0.1:33676 refcnt: 2 status: Connected last used: Tue Mar 9 12:01:18 2021 ** ld 0x55974c710680 Outstanding Requests: * msgid 1, origid 1, status InProgress outstanding referrals 0, parent count 0 ld 0x55974c710680 request count 1 (abandoned 0) ** ld 0x55974c710680 Response Queue: Empty ld 0x55974c710680 response count 0 I tried to add to ldap_dump_connection the "from" part which is always output regardless of the new SOCKET_BIND_ADDRESSES option If this ok, i could provide a patch for it tomorrow :)
(In reply to HoweverAT from comment #37) > (In reply to Howard Chu from comment #36) > > Since I've just been tinkering with debug output, this came to mind. With > > this patch, it would be a good idea to add the locally bound IP:port to the > > debug output somewhere, e.g. in ldap_dump_connection. Should probably use > > the recently added lutil_sockaddrstr, and that function likely should be > > moved into ldap_pvt_ instead of lutil. > > Hello, > > thank you for your input. > > I tried to add to ldap_dump_connection( see the "from" part, which is always > printed out independet of ) > > e.g. Output: > ** ld 0x55974c710680 Connections: > * host: localhost port: 389 (default) > * from: IP=127.0.0.1:33676 > refcnt: 2 status: Connected > last used: Tue Mar 9 12:01:18 2021 > > > ** ld 0x55974c710680 Outstanding Requests: > * msgid 1, origid 1, status InProgress > outstanding referrals 0, parent count 0 > ld 0x55974c710680 request count 1 (abandoned 0) > ** ld 0x55974c710680 Response Queue: > Empty > ld 0x55974c710680 response count 0 > > I tried to add to ldap_dump_connection the "from" part which is always > output regardless of the new SOCKET_BIND_ADDRESSES option > > If this ok, i could provide a patch for it tomorrow :) Yes, this is what I had in mind, thanks.
Created attachment 807 [details] Add SOCKET_BIND_ADDRESSES Option New patch against latest master Changed: - Debug Improvements (Add client address in ldap_dump_connection, also print binded address in DEBUG_TRACE if used) - Bugfix Thank you for your feedback in advance Lukas The attached patch file is derived from OpenLDAP Software. All of the modifications to OpenLDAP Software represented in the following patch(es) were developed by Lukas Wimmer laeufer4321@gmx.at. I have not assigned rights and/or interest in this work to any party. I, Lukas Wimmer, hereby place the following modifications to OpenLDAP Software (and only these modifications) into the public domain. Hence, these modifications may be freely used and/or redistributed for any purpose with or without attribution and/or other notice.
(In reply to HoweverAT from comment #39) > Created attachment 807 [details] > Add SOCKET_BIND_ADDRESSES Option > > New patch against latest master > > Changed: > - Debug Improvements (Add client address in ldap_dump_connection, also print > binded address in DEBUG_TRACE if used) > - Bugfix I would suggest submitting a merge request via your existing account at https://git.openldap.org for review to ease the review process. Regards, Quanah
Commits: • 8ebd0650 by HoweverAT at 2021-03-25T17:37:26+00:00 ITS#8847 Print local address in connection dump • 9d594a11 by HoweverAT at 2021-03-25T18:47:11+00:00 ITS#8847 Add SOCKET_BIND_ADDRESSES Option
Commits: • 829263c4 by Howard Chu at 2021-03-26T13:45:26+00:00 ITS#8847 move lutil_sockaddrstr() to ldap_pvt_sockaddrstr() Commits: • f2ddf89e by Howard Chu at 2021-03-26T14:12:47+00:00 ITS#8847 more fallout from ldap_pvt_sockaddrstr move
Commits: • bc0d62db by Howard Chu at 2021-03-27T10:38:59+00:00 Revert "ITS#8847 more fallout from ldap_pvt_sockaddrstr move" This reverts commit f2ddf89e3cbe2ba65728cfc7b4c022d72192f442. Move Sockaddr def to ac/socket.h instead.
Commits: • 7df2a0f3 by Ondřej Kuzník at 2021-04-15T15:16:19+01:00 ITS#8847 Allocate a large enough buffer