328791 - LDAP C SDK command line tools need to support IPv6 scoped addresses

Status: RESOLVED FIXED

(Directory Graveyard :: LDAP C SDK, defect)

Description

[nkinder]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.12) Gecko/20060130 Red Hat/1.0.7-1.4.3 Firefox/1.0.7
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.12) Gecko/20060130 Red Hat/1.0.7-1.4.3 Firefox/1.0.7

The current trunk of the LDAP C SDK does not handle scoped IPv6 addresses properly.

This is due to the fact that we are not using the proper NSPR functions that set the scope_id field of the PRNetAddr structure.  The scope_id field needs to contain the local interface to use when connecting to a link-local address.  We should be using the PR_GetAddrInfoByName() function since it will fill the scope_id in for us.

Reproducible: Always

Steps to Reproduce:

Comment 1

[nkinder]

Attachment: Initial fix

This allows the command-line tools to connect via a link-local IPv6 address.  The host/ip needs to be in extended scoped address format (ex - ./ldapsearch -h "[fe80::210:c6ff:fede:56c6%eth0]" -p 389 ...) for it to work.

Comment 2

[Mark Smith [:mcs]]

Thanks for the patch!

I was not aware of scoped IPv6 addresses.  Does this patch (and NSPR) implemented the scheme defined in RFC 4007?

Is there a separate bug open about the fact that PR_StringToNetAddr() does not support scoped addresses?  I need to look and see what PR_GetAddrInfoByName() does if you pass it an address instead of a host name (some people pass IP addresses to  ldap_init() and other LDAP functions to avoid DNS lookups being done inside libldap).

Do you know if your changes work on platforms where IPv6 was not supported?

Comment 3

[nkinder]

Mark:  This patch is using the scheme defined in RFC 4007.  It essentially using getaddrinfo() underneath the covers.

I have tested passing an address into PR_GetAddrInfoByName(), which is actually the only way we can get scoped addresses to work currently.  It's not a legal format to say "<hostname>%<if>".  The underlying system calls also do not seem to support using extended scoped addresses in /etc/hosts.  In order to support connecting to an LDAP server via an IPv6 link-local address using a hostname, we would need to add an additional command-line argument to specify the interface to use.  This is the approach taken with the "ping6" utility.  I'm not sure if it's worth adding yet another command-line argument to the tools for this though.

PR_GetAddrInfoByName() is supposed to be protocol independent, so it should work on machines where IPv6 is not available, but I have not tested that myself.  We should also make sure that these changes work on systems that do not support getaddrinfo().  NSPR is supposed to deal with this for us, but it'd be good to test it for peace of mind.

Comment 4

[nkinder]

Noriko is working on enhancing the PR_StringToNetAddr() and PR_NetAddrToString() functions to use getaddrinfo() and getnameinfo() on platforms where those functions are supported.  That work will make my fix in this bug unneccessary.

Comment 5

[Noriko Hosoi]

I'd like to make this bug depend on 34843: Use getaddrinfo/getnameinfo
But it looks I don't have the permission to do it...

Comment 6

[Wan-Teh Chang]

Noriko, I just gave you to permissions to do that. Please try again.

Comment 7

[Noriko Hosoi]

Thank you, Wan-Teh!  I'm re-trying...

Comment 8

[Noriko Hosoi]

Attachment: cvs diff directory/c-sdk/ldap/libraries/libprldap/{ldappr-dns.c,ldappr-io.c}

To make LDAP C SDK command line tools support IPv6 scoped addresses on Windows, NSPR fix for 34843 is not sufficient, but we need to call appropriate NSPR APIs as Nathan originally suggested (see 228439 LDAP broken if IPv6-Stack is installed).

The problem on Windows is when IPv6 is installed and the target address is IPv4, the current prldap_connect converts the IPv4 address to IPv4 mapped IPv6 address, which is not supported on Windows.  Instead, we should use PR_GetAddrInfoByName to get all the available addresses (including IPv4 address and IPv4 compatible IPv6 address) and enumerate the returned PRAddrInfo examining each address in the linked list.

Basically, the diffs are almost identical to the ones that Nathan proposed.  I made one small change: eliminated the call PR_StringToNetAddr which is called when PR_GetAddrInfoByName fails.  PR_StringToNetAddr is going to be updated to call PR_GetAddrInfoByName internally.  Thus, if PR_GetAddrInfoByName fails, PR_StringToNetAddr also fails and it cannot be a fallback for PR_GetAddrInfoByName.

I tested the code on Windows 2003 as well as Windows 2K + IPv6 technology preview.  When IPv6 is installed, both address types are supported.  If IPv6 is uninstalled, binding the server with the IPv6 address fails.

>ldapsearch -h [FE80::208:74FF:FE38:FCD5%4] -p <port> -b "dc=example,dc=com" "(uid=test1)" dn
dn: uid=test1, ou=People, dc=example,dc=com

>ldapsearch -h 10.10.1.8 (or <hostname>) -p <port> -b "dc=example,dc=com" "(uid=test1)" dn
dn: uid=test1, ou=People, dc=example,dc=com

Thank you!

Comment 9

[Rich Megginson]

Ok.  Looks good.

Comment 10

[Mark Smith [:mcs]]

Comment on attachment 237187 [details] [diff] [review]
cvs diff directory/c-sdk/ldap/libraries/libprldap/{ldappr-dns.c,ldappr-io.c}

OK (I am not an expert on NSPR's IPv6-related functions; I have forgotten much of what I used to know).

Do these changes mean that libprldap requires a newer version of NSPR to work correctly?  What will this break?

Comment 11

[Noriko Hosoi]

(In reply to comment #10)
> (From update of attachment 237187 [details] [diff] [review] [edit])
> OK (I am not an expert on NSPR's IPv6-related functions; I have forgotten much
> of what I used to know).

Thanks for the review, Mark.  But, so sorry.  I have to ask you one more time.  Wan-Teh nicely reviewed the diffs and found some bugs in them.  I fixed and tested them.  I'm posting the new diff in the next "Comment".

> Do these changes mean that libprldap requires a newer version of NSPR to work
> correctly?  What will this break?

It should work on majority of the platforms with the reasonably new NSPR such as NSPR 4.6.2. (I haven't tested with the older version.)  There could be some  platforms on which a string address is not handled correctly unless NSPR have the fix for 34843 although I have not seen the case yet.  Please note that when a host name is passed by the user, there should be no problem.

Comment 12

[Noriko Hosoi]

Attachment: cvs diff ldappr-dns.c ldappr-io.c

Changes from the proposal in Comment #8
ldappr-dns.c:
  . cleaned up the code by moving PR_FreeAddrInfo and eliminating the goto label
  . resurrected PR_StrintToNetAddr in case the string address was not handled
  correctly by PR_GetAddrInfoByName.
ldappr-io.c:
  . resurrected PR_StrintToNetAddr in case the string address was not handled
  correctly by PR_GetAddrInfoByName.
  . fixed a bug in the condition to stop the enumeration on NetAddrInfo.

Comment 13

[Mark Smith [:mcs]]

Comment on attachment 237394 [details] [diff] [review]
cvs diff ldappr-dns.c ldappr-io.c


>Index: directory/c-sdk/ldap/libraries/libprldap/ldappr-io.c
> ...
>+	    } while ( rc < 0 );
>+	    PR_FreeAddrInfo( infop );
>+	} else if ( PR_SUCCESS == PR_StringToNetAddr( host, &addr )) {
>+	    rc = prldap_try_one_address( prsockp, &addr, timeout, options );
> 	}

In the above code, I think you need to set the port inside addr.  Looks OK to me otherwise.

Comment 14

[Noriko Hosoi]

Attachment: cvs diff ldappr-dns.c ldappr-io.c (final)

Thank you for the review and comments, Mark!
I resurrected the code to set the port in addr.
+        } else if ( PR_SUCCESS == PR_StringToNetAddr( host, &addr )) {
+            if ( PR_SUCCESS != PR_SetNetAddr( PR_IpAddrNull, /* don't touch IP
addr. */
+                PR_NetAddrFamily( &addr ), (PRUint16)port, &addr )) {
+                return( -1 );
             }
+            rc = prldap_try_one_address( prsockp, &addr, timeout, options );
         }

Checked in into sun_merge_branch_20060523
=========================================
[328791] LDAP C SDK command line tools need to support IPv6 scoped addresses
. instead of PR_GetIPNodeByName, use PR_GetAddrInfoByName for the better
  support for the both IPv4 and IPv6.
. eliminated calling PR_ConvertIPv4AddrToIPv6 since IPv4 mapped IPv6 address
  is not supported on Windows. [228439]
CVS: ----------------------------------------------------------------------
CVS: Modified Files:
CVS:  Tag: sun_merge_branch_20060523
CVS:    ldappr-dns.c ldappr-io.c
CVS: ----------------------------------------------------------------------
Checking in ldappr-dns.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libprldap/ldappr-dns.c,v  <--  ldappr-dns.c
new revision: 5.2.8.1; previous revision: 5.2
done
Checking in ldappr-io.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libprldap/ldappr-io.c,v  <--  ldappr-io.c
new revision: 5.5.8.1; previous revision: 5.5
done

Comment 15

[David :Bienvenu]

so do we need to check this into the trunk of the ldap c sdk, so that TB trunk and 2.0 will pick up this fix?

Comment 16

[Mark Banner (:standard8)]

(In reply to comment #15)
> so do we need to check this into the trunk of the ldap c sdk, so that TB trunk
> and 2.0 will pick up this fix?

David, note also http://wiki.mozilla.org/LDAPRoadmap looks like the plan is to merge the sun branch to the trunk at some stage.

Checking this into trunk ldap c sdk won't pick it up for TB trunk & 2.0.

For 2.0 I wonder if we could port the patch to MOZILLA_1_8_BRANCH. Note we never got round to updating to csdk 5.1.7 on the branch, only on the trunk. I would certianly like to get it in for Thunderbird 2.0/SeaMonkey 1.1 if possible.

For trunk, we could either port to ldapcsdk_5_17_client_branch or pick up the latest trunk from a fresh release (once merged).

Comment 17

[Rich Megginson]

We're currently testing the sun merge branch (sun_merge_branch_20060523).  Once it passes, we are going to merge this into the ldapcsdk trunk and call it version 6.0.  The version change is due to the new functionality (sasl, ipv6, other) and also because it will be binary incompatible with v5.x.  We should be doing the merge in a week or two, assuming we don't find any major issues during testing.

Comment 18

[David :Bienvenu]

I think we need to port that change to our 2.0 branch - is the ldap directory on the branch open or closed to checkins? I.e., can I do the checkin?

I didn't see a timeframe in the roadmap for this work. It would be nice to pick up the LDAP SASL work as well, if it's stable enough for TB 2.0

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

It's open.  In addition, I believe mcs recently changed the default (i.e. non-client-branch) LDAP partition to be open as well.

Comment 20

[Rich Megginson]

SASL is probably not going to work for tbird/seamonkey.  We have an explicit dependency on cyrus-sasl, and we don't support sasl at all on Windows.  There is another bug for SASL/GSSAPI LDAP support in tbird/seamonkey - bug 308118.  The difference is that for the mozilla client, in order to work cross platform, the nsIAuthModule must be used instead of cyrus-sasl.

Comment 21

[Noriko Hosoi]

Attachment: cvs diffs (libprldap)

I have to update the code one more time.  (note: this would affect the sun merge task [352519])
Files:
  mozilla/directory/c-sdk/ldap/libraries/libprldap/
       ldappr-int.h
       ldappr-io.c
       ldappr-dns.c

Changes:
1) ldappr-int.h, ldappr-io.c: Wan-Teh gave us a suggestion, that is instead of calling a function PR_SetNetAddr, we should use a macro PR_NetAddrInetPort.  Unfortunately, building libprldap failed on Windows if we use the macro itself.   The plan is a new macro which sets the port to PRNetAddr will be introduced in NSPR (newer than v4.6.2), in the meantime and to keep the backward compatibility with the current NSPR, we define PRLDAP_SET_PORT in libprldap and use it internally.
2) ldappr-io.c: [352519] taught me there is a leak (thanks!)  I think we don't need the second ldap_memfree (outside of for loop).  What do you think?
    + ldap_memfree( host );
3) ldappr-dns.c: I'd like to back off the previous change.  It turned out the change does not make the function IPv6 enabled since it still calls PR_GetHostByAddr.  Plus prldap_gethostbyname (sister function of prldap_gethostbyaddr) has higher chance to be called, but it's not IPv6 enabled.  Actually, when prldap_init (or more precisely prldap_install_io_functions) is called in an application, prldap_connect is set and called (prldap_gethostbyname is skipped), which works fine in the IPv4 and IPv6 environment with the fix made in this bug.  The function prldap_gethostbyname is called only when prldap_install_io_functions is NOT called and prldap_install_dns_functions is called.  It fails sifunctionsnce it's using the deprecated function PR_GetIPNodeByName.  We could try to replace these deprecated functions with the newer NSPR functions with the better IPv6 support.  But there is another interface issue, which is the return value from prldap_gethostbyname and prldap_gethostbyaddr: LDAPHostEnt.  It's not straightforward to generate the type from PRNetAddr.  Also, looking at the system library, gethostbyname and gethostbyaddr are not IPv6 friendly, anyway, and getaddinfo and getnameinfo are recommended to use in the new code.  The final input is "struct ldap_dns_fns" is defined in a header file named ldap-to-be-deprecated.h!!  Summarizing these facts, the right way to go is leaving the functions prldap_gethostbyname and prldap_gethostbyaddr as is.  If necessary, introduce the interface:
        LDAP_DNSFN_GETADDRINFO        *lddnsfn_getaddrinfo;
        LDAP_DNSFN_GETNAMEINFO       *lddnsfn_getnameinfo;
and implement prldap_getaddrinfo and prldap_getnameinfo.  But for now, as long as calling prldap_install_io_functions (or prldap_init), the application is IPv6 safe.

Comment 22

[Noriko Hosoi]

Anton, let me include you in the loop.

Comment 23

[Rich Megginson]

Diff https://bugzilla.mozilla.org/attachment.cgi?id=238277 looks good.

Comment 24

[Noriko Hosoi]

Thank you, Rich, for the review and the discussion.  As you suggested,
I'd like to put the following comment to emphasize to use prldap and call 
prldap_init in the application which expects to work in the IPv6 environment.
I also think this should be doc'ed.

Index: ldappr.h
===================================================================
RCS file: /cvsroot/mozilla/directory/c-sdk/ldap/include/ldappr.h,v
retrieving revision 5.5
diff -t -w -U8 -r5.5 ldappr.h
--- ldappr.h    9 Sep 2004 18:50:03 -0000       5.5
+++ ldappr.h    13 Sep 2006 21:36:28 -0000
@@ -54,16 +54,20 @@
  *
  * Create a new LDAP session handle, but with NSPR I/O, threading, and DNS
  * functions installed.
  *
  * Pass a non-zero value for the 'shared' parameter if you plan to use
  * this LDAP * handle from more than one thread.
  *
  * Returns an LDAP session handle (or NULL if an error occurs).
+ *
+ * NOTE: If you want to use IPv6, you must use prldap creating a LDAP handle
+ * with this function prldap_init.  Prldap_init installs the appropriate
+ * set of NSPR functions and prevents calling deprecated functions accidentally.
  */
 LDAP * LDAP_CALL prldap_init( const char *defhost, int defport, int shared );

Comment 25

[Noriko Hosoi]

Attachment: cvs diffs (revised proposal)

Thanks to Wan-Teh for the comments and ideas to improve the code.
Files:
  include/ldappr.h
  libraries/libprldap/ldappr-int.h
  libraries/libprldap/ldappr-dns.c
  libraries/libprldap/ldappr-io.c

Changes:
include/ldappr.h: no change from the previous proposal
libraries/libprldap/ldappr-int.h: removed the NSPR dependency.  It may be implemented as a function, not macro in NSPR.  So, for now, we use our macro.
libraries/libprldap/ldappr-dns.c:
 1) if the memory pointed by statusp is not given, we do nothing.
 2) better initialize the space for the address with NULL.
 3) use PRLDAP_SET_PORT instead of PR_SetNetAddr.
libraries/libprldap/ldappr-io.c: set PR_AI_NOCANONNAME to the hint flag for PR_GetAddrInfoByName since we don't need to determine the canonical name.

Comment 26

[Noriko Hosoi]

Comment on attachment 238312 [details] [diff] [review]
cvs diffs (revised proposal)

I have to ask for reviewing the code once more...  Thank you so much in advance.

Comment 27

[Mark Smith [:mcs]]

Comment on attachment 238312 [details] [diff] [review]
cvs diffs (revised proposal)

Looks OK (I trust Wan-Teh on the NSPR details).

It seems like the same comment you added near prldap_init() should be added near prldap_install_io_functions().   Perhaps the comment really needs to be added near ldap_init(), because anyone who has decided to use prldap should be OK with respect to IPv6 support.  Do what you think is best.

Comment 28

[Noriko Hosoi]

Attachment: cvs diffs (ldap-standard-tmpl.h and open.c with IPv6 comments)

Thank you for the review and comments, Mark.
I added the IPv6 comments to ldap-standard-tmpl.h and open.c.
Files:
  mozilla/directory/c-sdk/ldap/include/ldap-standard-tmpl
  mozilla/directory/c-sdk/ldap/libraries/libldap/open.c,

Comment 29

[Noriko Hosoi]

Attachment: cvs commit message

Checked in into sun_merge_branch_20060523.

Comment 30

[Noriko Hosoi]

Attachment: cvs diff ldappr-int.h

Sorry, I found one more bug to fix.

Before setting port in PRNetddr, it should be processed with htons.

Comment 31

[Mark Smith [:mcs]]

Comment on attachment 238665 [details] [diff] [review]
cvs diff ldappr-int.h

OK.  There is also a PR_htons() but all it does is call htons() anyway....

Comment 32

[Rich Megginson]

Re: comment #30.  Ok.  Looks good.

Comment 33

[Noriko Hosoi]

Attachment: cvs commit message

Thank you so much for the review and comments, Mark and Rich!
I replaced htons with PR_htons (prldap does not call hton? nor ntoh? directly...)
following the comments from Mark.

Checked in into sun_merge_branch_20060523.

Comment 34

[David :Bienvenu]

Is anyone interested in porting this to the 1.8.1 branch, which we're shipping TB 2.0 off of? Or should I have a go at it? I don't have vista available to test this, unfortunately.

Comment 35

[Noriko Hosoi]

(In reply to comment #34)
> Is anyone interested in porting this to the 1.8.1 branch, which we're shipping
> TB 2.0 off of? Or should I have a go at it? I don't have vista available to
> test this, unfortunately.
> 

Shall I volunteer to port the change?  I'm not familiar with the 1.8.1 branch, though.  Could you please tell me the branch tag?  And which version(s) of NSS/NSPR the LDAP C SDK depends upon?  Also, we don't have Vista, either.  We only have Win2K, Win2003, and WinXP...

Comment 36

[David :Bienvenu]

Thx for offering to volunteer! I'd be happy to test any 1.8.1 branch patch you come up with.

MOZILLA_1_8_BRANCH is the branch - my guess is that it uses the same version of nspr/nss as the trunk.

I only mentioned Vista because I heard it required ipv6, and I just want to make sure this works on Vista. But getting ldap working ipv6 should be all we need to do.

Comment 37

[Noriko Hosoi]

Attachment: cvs diffs (MOZILLA_1_8_BRANCH)

(In reply to comment #36)
> Thx for offering to volunteer! I'd be happy to test any 1.8.1 branch patch you
> come up with.

Thank you for the input, David.

> MOZILLA_1_8_BRANCH is the branch - my guess is that it uses the same version of
> nspr/nss as the trunk.
> 
> I only mentioned Vista because I heard it required ipv6, and I just want to
> make sure this works on Vista. But getting ldap working ipv6 should be all we
> need to do.

The attached diff file is the patch for LDAP C SDK checked out with the CVS branch tag MOZILLA_1_8_BRANCH.
To apply the patch, ...
$ cvs -d $MOZCVSROOT co -r MOZILLA_1_8_BRANCH DirectorySDKSourceC
$ cd mozilla/
$ patch -p0 < "patch_file_name"
patching file directory/c-sdk/ldap/include/ldap-standard-tmpl.h
patching file directory/c-sdk/ldap/include/ldappr.h
patching file directory/c-sdk/ldap/libraries/libldap/open.c
patching file directory/c-sdk/ldap/libraries/libprldap/ldappr-dns.c
patching file directory/c-sdk/ldap/libraries/libprldap/ldappr-int.h
patching file directory/c-sdk/ldap/libraries/libprldap/ldappr-io.c

I tested it on Red Hat Enterprise Linux 4, Win2K, and Win2003.
If you could review and/or test the code, I'd greatly appreciate it.
Thanks!
--noriko

Comment 38

[David :Bienvenu]

I'm running with this patch - basic ldap seems to work fine on XP. I'm probably going to try to land this for 2.0 and try to get some testing on vista for ipv6 compatibility. Thx so much, Noriko!

Comment 39

[Scott MacGregor]

putting this on the thunderbird 2 blocking radar so we don't lose track of it.

Comment 40

[David :Bienvenu]

fixed on 1.8.1 branch - thx again, Noriko!

Comment 41

[Daniel Veditz [:dveditz]]

updating fixed keyword, this landed after the 1.8.1 release.

Comment 42

[alice nodelman [:alice] [:anode]]

The final patch appears to have been tested on various platforms - are we confident enough to consider this verified?

If so can one of those who tested the patch replace the keyword fixed1.8.1.1 with verified1.8.1.1 - or provide me with a testcase so that I can do my own verification?

Comment 43

[Noriko Hosoi]

I used the ldap c sdk commands for testing against the Fedora Directory Server 1.0.3 and newer (please take a look at the comment #8).

When IPv6 is disabled on the host
The host argument (-h) takes host name and IPv4 address (xx.xx.xx.xx).

When IPv6 is enabled, in addition to the above, IPv6 numeric address ([xx:xx:...:xx[%xxx]]) can be used to access the server.

Comment 44

[Scott MacGregor]

I wonder if this should block the next security release as well? (1.5.10?)

Without this patch, vista users will be unable to use LDAP on 1.5.x...

But I don't see a field to nominate the bug with.