366614 - Convert NSS to be able to use IPv6

Status: ASSIGNED

(NSS :: Tools, enhancement, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

Everywhere in NSS (cmds and libs) where it creates a new TCP socket, it calls
    s = PR_NewTCPSocket();
but this has a problem.  It always creates an IPv4 socket, never an IPv6 socket.
So, NSS cmds and libs CANNOT connect to an IPv6 address, even if they are able
to resolve a hostname to an IPv6 address, or accept IPv6 addresses on the 
command line.  (See bug 161610).  This must be fixed before we can do ANY 
testing of NSS on IPv6 systems/networks.

The solution involves replacing ALL the PR_NewTCPSocket() calls with calls to
PR_OpenTCPSocket(addr.raw.family), as is done in the patch for tstclnt, 
attached to bug 324305.  

Not sure if this should target 3.11.5 or 3.12.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

We need to fix this because people are using our tools code as a template,
and then they wonder why they can't use IPv6.

The solution also involves eliminating all uses of PR_GetHostByName and its
kin, and replacing them with PR_GetAddrInfoByName and its kin.

Comment 2

[Julien Pierre]

This is mostly a tools problem. There are only 2 places on the trunk under lib that call PR_NewTCPSocket : one is in the OCSP client, the other in libpkix - and that's used for outgoing connections.

Except for OCSP, NSS can already use IPv6, and indeed products already use it, since most of them create NSPR sockets and then import them to SSL with SSL_ImportFD, which gives them the ability to create an IPV6 NSPR socket upfront.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

I have opened a separate bug about the lack of IPv6 capability in 
selfserv and strsclnt, and have attached patches to it.  So this bug
is reduced in scope to "everything else" :)

Comment 4

[Biswatosh Chakraborty]

assigning the bug to myself...

Comment 5

[Biswatosh Chakraborty]

Attachment: nss/cmd utilities patch, v1, for review

This covers these files in nss/cmd, namely:
(1)SSLsample/client.c
(2)SSLsample/server.c
(3)vfyserv/vfyserv.c
(4)ssltap/ssltap.c
Bugs have been separately opened for selfserv and strsclnt. See Comment #3.
Hence, this bug's scope is the files mentioned above(1-4) plus two
files in nss/lib, namely 
(5)certhigh/ocsp.c and 
(6)libpkix/pkix_pl_nss/module/pkix_pl_socket.c.

I will be giving the patches for the nss/lib files mentioned above(5 and 6)
but right now, I am requesting a review of the nss/cmd files(1-4) only.

Thanks,

Comment 6

[Biswatosh Chakraborty]

Attachment: This covers patches for nss/cmd utilities only.

This covers these files in nss/cmd, namely:
(1)SSLsample/client.c
(2)SSLsample/server.c
(3)vfyserv/vfyserv.c
(4)ssltap/ssltap.c
Bugs have been separately opened for selfserv and strsclnt. See Comment #3.
Hence, this bug's scope is the files mentioned above(1-4) plus two
files in nss/lib, namely 
(5)certhigh/ocsp.c and 
(6)libpkix/pkix_pl_nss/module/pkix_pl_socket.c.

I will be giving the patches for the nss/lib files mentioned above(5 and 6)
but right now, I am requesting a review of the nss/cmd files(1-4) only.

Thanks,

Comment 7

[Biswatosh Chakraborty]

Comment on attachment 277701 [details] [diff] [review]
This covers patches for nss/cmd utilities only.

I pressed the back button and it resubmitted the same patch again. Pls ignore this duplicate patch. Sorry for the confusion.

Comment 8

[Biswatosh Chakraborty]

Attachment: nss/lib patch, v1, For review

This covers nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_socket.c and
nss/lib/certhigh/ocsp.c and nss/lib/libpkix/include/pkix_errorstrings.h.

To test libpkix changes, I tested this using "pkixutil test_socket"
command and to test changes in ocsp.c, I ran ocsp test using all.sh.

The only change I did in ocsp.c was in ocsp_ConnectToHost() in 
certhigh/ocsp.c
I forced ocsp test to call ocsp_ConnectToHost().
I did this by commenting one line in ocsp_GetEncodedOCSPResponseFromRequest()
function in the same file. I commented 
" registeredHttpClient = SEC_GetRegisteredHttpClient();"
and now registeredHttpClient being NULL, ocsp_ConnectToHost() was being
called.

I had to change some code in test_socket.c to see that it understands
IPv6 addresses or not. 

This patch thus covers nss/lib.

Comment 9

[Biswatosh Chakraborty]

Attachment: libpkix test command patch, v1, for review

This covers test_socket.c only, present in nss/cmd/libpkix/pkix_pl/module
I added a function to detect whether a string is an IP address or a hostname.
This is certainly not a full-proof function. It expects correct format
and can be used for what test_socket() is being used, that is for unit testing.

I am going to attach some sample runs in a while.

Comment 10

[Biswatosh Chakraborty]

Attachment: Sample outputs with various inputs to "pkixutil test_socket" command

Comment 11

[Biswatosh Chakraborty]

Attachment: libPKIX test command sample output

Comment 12

[Biswatosh Chakraborty]

Comment on attachment 279589 [details]
Sample outputs with various inputs to "pkixutil test_socket" command

This is not openable through a browser. Hence, 
reattaching the same output as a plain-text format.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 279590 [details]
libPKIX test command sample output

Julien, I have never seen the output of this program before. Have you?  
Can you tell me if this is the expected output?

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 277700 [details] [diff] [review]
nss/cmd utilities patch, v1, for review

Part of this patch applies to selfserv.  r- for that part, because there is another more comprehensive patch for that in bug 388117.  The rest of this patch is not yet reviewed.

Comment 15

[Julien Pierre]

Nelson,

Yes, I have seen it, but I don't remember the expected output. I think it would be best to compare before and after the patch.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 277700 [details] [diff] [review]
nss/cmd utilities patch, v1, for review

1. This patch adds a large body of code to each program, and twice in
some programs.  It must not do that.  Instead, it must add a new 
function into cmd/lib/somenewfile.c and call it in all the right places.

SECStatus
SECU_StringToNetAddr(const char * hostname, PRUint16 port, PRNetAddr *addr)

The new function should never exit, but only return SECFailure on error.
It may continue to print error messages to stderr. It must not leak.

2. ssltap must use the same address family for the server socket that it
uses for the client socket.  That is, the address family for the socket
that it connects to the remote server must also be used as the address
family for the listen socket it creates.  

3. in server.c, 

Get rid of this:

>+const PRIPv6Addr _pr_in6addr_any =	{{{ 0, 0, 0, 0,
>+					    0, 0, 0, 0,
>+					    0, 0, 0, 0,
>+				            0, 0, 0, 0 
>+                                         }}};
>+

Combine the following two if-the-elses into one,
and test for enableIPv6, or enableIPv6 != PR_FALSE,
but do not test for enableIPv6 == PR_TRUE.

> 	/* Create a new socket. */
>-	listenSocket = PR_NewTCPSocket();
>+	if (enableIPv6 == PR_TRUE)
>+	    fprintf(stderr,"\nserver with IPv6\n");
>+	else
>+	    fprintf(stderr,"\nserver with IPv4\n");
>+
>+	if (enableIPv6 == PR_TRUE)
>+	    listenSocket = PR_OpenTCPSocket(PR_AF_INET6);
>+	else
>+	    listenSocket = PR_OpenTCPSocket(PR_AF_INET);
>+

Replace all of the following code with a single unconditional
call to PR_SetNetAddr()

> 	/* Configure the network connection. */
>-	addr.inet.family = PR_AF_INET;
>-	addr.inet.ip	 = PR_INADDR_ANY;
>-	addr.inet.port	 = PR_htons(port);
>+	if (enableIPv6 == PR_FALSE) {
>+	    addr.inet.family     = PR_AF_INET;
>+	    addr.inet.ip	 = PR_INADDR_ANY;
>+	    addr.inet.port	 = PR_htons(port);
>+	} else {
>+	    addr.ipv6.family = PR_AF_INET6;
>+	    addr.ipv6.ip     = _pr_in6addr_any;
>+	    addr.ipv6.port   = PR_htons(port);
>+	}

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 279588 [details] [diff] [review]
libpkix test command patch, v1, for review

This patch adds a large body of code to NSS libraries in several
places.  It must not do that.  Instead, create one new function, e.g. 

SECStatus
SEC_StringToNetAddr(const char * hostname, PRUint16 port, PRNetAddr *addr)

that does it in one place.  Add it to an NSS internal private header file.
It must not exit and it must not do any printfs.  It must set error codes,
and return appropriate SECStatus.  Call it where needed.

Don't construct PRNetAddr's by hand.  Instead, call PR_SetNetAddr

Get rid of this new function, and call SEC_StringToNetAddr().
It handles name vs address detection.

>+/*This is not a full proof function. It expects a correct format*/
>+int ip_or_hostname(char *hostid) {
>+    int ctr = 0;

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 279582 [details] [diff] [review]
nss/lib patch, v1, For review

1.  Don't use the macro PKIX_PL_NSSCALLRV in any new code.  
If you need to change a line that uses that macro, get rid of the macro
and just make the function call instead.

2. There is WAY too much duplicated code in this new patch.  
Generally, you should not duplicate code but rather use subroutine 
functions.  In this case, you should at least be calling SEC_StringToNetAddr
but you can probably eliminate more duplicate code than that.

3. Eliminate all the duplication of code between 
pkix_pl_Socket_CreateByName and pkix_pl_Socket_CreateByHostAndPort.
Make pkix_pl_Socket_CreateByName parse its input and then call 
pkix_pl_Socket_CreateByHostAndPort to do the work.

4. Change pkix_pl_Socket_CreateByName to separate the host name from the 
port number with a comma, not with a colon.  

5. Change any test scripts and programs to use comma instead of colon.

Comment 19

[Biswatosh Chakraborty]

Attachment: PatchV2. For review. Implements Nelson's suggestions.

Comment 20

[Biswatosh Chakraborty]

The patch (id=281291) covers mainly the following source files
1)nss/cmd/SSLsample/client.c
2)nss/cmd/SSLsample/server.c
3)nss/cmd/ssltap/ssltap.c
4)nss/cmd/vfyserv/vfyserv.c
5)nss/cmd/libpkix/pkix_pl/module/test_socket.c
6)nss/lib/certhigh/ocsp.c
7)nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_socket.c

And, in addition, it changes(slightly) :
8)nss/cmd/lib/manifest.mn
9)nss/lib/libpkix/include/pkix_errorstrings.h
10)nss/tests/iopr/ocsp_iopr.sh
11)nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_socket.h

Plus, I added a new file secnet.c in nss/cmd/lib and it's 
corresponding header file secnet.h in the same place.
secnet.c contains only one function, namely 
SECU_StringToNetAddr(const char * hostname, PRUint16 port, PRNetAddr *addr).
This was suggested in Comment #16.


Salient points:

(a)I have not used the multithreaded approach indicated in Comment #14. 

My patch does not include selfserv. Plus, since the approach in Bug 388117 
is still being tested for selfserv, I did not implement this. 
However, if Comment #14 means that I should use the same multithreaded technique 
in some source files of my patch(I guess SSLsample/server.c could be one candidate), then those source files in the patch need not be reviewed.
But, could someone suggest which files in my patch are good candidates for
such an experimentation?

(b)Created a function in nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_socket.c
SECStatus pkix_pl_StringToNetAddr(const char* hostname_in, PRUint16 port_in,
                                  PRNetAddr *na_addr_out)
This converts hostname into NetAddr and functions in libpkix can use it.

(c) However, I chose not to call this kind of function (which converts
hostname to netaddr) in certhigh/ocsp.c and instead wrote the whole code
of the function there. The reason being, I tried to do what the old code was doing in  ocsp_ConnectToHost(). There, in the old code, the while loop
was trying to Enumerate the address and Connect to the Host, which can't
be replicated if I call a StringToNetAddr() function directly. 

(d) In the same ocsp.c, I have changed  ocsp_ParseURL(). Earlier, it was
able to parse a URL with IPv4 address only and where the port was separated
by a colon(:). Now, this modified fuucntion can handle the usual IPv4 URLs,
separated by a colon(:) or a comma and can also pasre a IPv6 URL where the port
is separated by either a comma or a colon BUT the URL in IPV6 is being
assumed to conform to  RFC 3986. That is, an IPv6 URL would be enclosed
between square brackets([,]).

(e)Nelson suggested me to change the test scripts so that this time
the separator would be comma instead of a colon.(See Comment #18, 5th point).
That is being done in this patch and thus a ssltap command should now run like
ssltap www.netscape.com,80 or for pkix, "pkixutil testsocket 127.0.0.1,80".

Thus, I changed ocsp_iopr() in /nss/tests/iopr/ocsp_iopr.sh to let the
separator be a comma. With this, ocsp.sh passed fully.
I am not certain if there are other files to be changed this way.
My patch includes all those that I could find. I shall be thankful if
somebody could point to some of the missed files in nss.

(f)Now, pkix_pl_Socket_CreateByName() in nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_socket.c, parse's its input
and then call pkix_pl_Socket_CreateByHostAndPort to do the work.
Hence, the former function has been hugely reduced. This was suggested 
in Comment #18, point 3.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

I have not yet reviewed the latest patch.  These comments are based on
comment 20 alone.

Please ignore my mistaken remark about selfserv in comment 14.

My suggestion in comment 18, fifth point, was to change any libpkix test
scripts that were using the name:port syntax.  This is a change for libpkix
only.  I did not intend that you change the syntax of ssltap, or any scripts 
that use ssltap.  The syntax of ssltap must be preserved.  

Is there any difference between pkix_pl_StringToNetAddr and SEC_StringToNetAddr 
other than the spelling of the function names?  If so, we don't need pkix_pl_StringToNetAddr.  We don't need two functions identical except for 
their names.  The name SEC_StringToNetAddr is good enough to be used 
everywhere. 

Adding a new .c source file in nss/cmd/lib for the implementation of 
SEC_StringToNetAddr is good, but the declaration of that function needs to 
be in secutil.h, not in a new separate .h file.  

I will review the actual patch next.  

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 281291 [details] [diff] [review]
PatchV2. For review. Implements Nelson's suggestions.

1. Eliminate secnet.h, and move secnet.c into nss/lib/util.
I changed my mind about one thing.  Make SEC_StringToNetAddr be public.
Don't have a copy in nss/cmd/lib, and use the copy in libutil in the 
commands, too.  I don't remember why I thought there should be two 
copies, one in nss/cmd/lib and one in nss/lib/something, but now I'm
satisfied that we want just one copy in nss/lib and use that copy in 
commands too. 

2. In SSLsample client.c and server.c, and in vfyserv, get rid of ALL 
code in between #if 0 and the following #endif.

3. instead of passing a boolean named enableIPv6 and having code like
   enableIPv6?PR_AF_INET6:PR_AF_INET
pass the value PR_AF_INET6 or PR_AF_INET directly.  Call it "af".
Use the same type as for the argument to PR_OpenTCPSocket().

4. Use spaces after commas and before and after colons and question marks, 
e.g. don't code like this:
   enableIPv6?PR_AF_INET6:PR_AF_INET,port,&addr);
do code like this:
   enableIPv6 ? PR_AF_INET6 : PR_AF_INET, port, &addr);
That comment applies to all source files in this patch.

5. For the argument string for PL_CreateOptState, keep the option 
characters in ASCII colating sequence.  Put the 6 after the 3.

6. secnet.c doesn't seem to follow NSS coding style very well.
The function type should be on its own line.
First '{' in function in column 1.
First line of code indented 4 columns
Indentation = 4

7. code of the form 
    if (expression1) { 
        action1;
        return;
    } else ...

is bad coding practice.  The thing to avoid is "return; else" 
There's no need for the else after the return, and no need for the 
code following the return to be indented the extra level.  
There are two occurrences of return;else in this function. Fix both.

The one case in which return;else is OK is when the return is inside of 
an "else if" block, e.g. 
    if (expression 1) 
	action 1;
    else if (expression 2)
	action 2;
	return;
    else ...

8.  What is unistd.h needed for in secnet.c?

9. in test_socket.c, is it going to work if the client and server net 
addresses are in different address families?

10.  just as "return;else" is bad, so is "exit();else". 
So, don't do that in ssltap.

11. Appears that pkix_pl_StringToNetAddr is just SEC_StringToNetAddr 
with another name.  Get rid of it.  Call SEC_StringToNetAddr instead.
That function belongs in lib/util.

I will review certhigh/ocsp.c and module/pkix_pl_socket.c in a separate
bug comment. tomorrow.

Comment 23

[Biswatosh Chakraborty]

Attachment: Patch3. Does not contain pkix lib and pkix command

1)I am including everything in this patch all that was in the previous patch excluding pkix, that is excluding test_socket.c and pkix_pl_socket.c.

2)Implemented suggestions made in comment #22.

3)When I updated my local copy of test_socket.c, I saw that it has
been modified in 2/3 lines and it was giving error when I tested
with pkixutil test_socket hostid and port.
And, when I modified the latest copy to the older version, it
again worked. 
The error reported was "address is in use". And, waiting
for some time or even restarting the machine again did not help.

Let me the tell the exact changes made in the new version.
$ diff newversion oldversion
*********************
446a447
>         PKIX_Boolean useArenas = PKIX_FALSE;
464,465c465,474
<         PKIX_TEST_EXPECT_NO_ERROR(
<             PKIX_PL_NssContext_Create(0, PKIX_FALSE, NULL, &plContext));
---
>         useArenas = PKIX_TEST_ARENAS_ARG(argv[1]);
>
>         PKIX_TEST_EXPECT_NO_ERROR(PKIX_Initialize
>                 (PKIX_TRUE, /* nssInitNeeded */
>                 useArenas,
>                 PKIX_MAJOR_VERSION,
>                 PKIX_MINOR_VERSION,
>                 PKIX_MINOR_VERSION,
>                 &actualMinorVersion,
>                 &plContext));
*******************************************
Here, lines starting with '>' is in the older version
and lines starting with '<' is in the new version.

4)My IPV6 code based on the old code works fine but fails 
with the new code. But as soon as I change those 2/3 lines
and make it back like the old code, it again starts working.

Anyways, those lines are not related to IPV6 and so I am wondering
what should my patch for test_socket.c be like? Should I show the
difference with the old version or with the new one? Or, I wait
for the new version to be corrected and then make the patch?
I am inclined towards giving the diff with the latest one for
review. I am giving that separately.

Comment 24

[Biswatosh Chakraborty]

Attachment: cvs diff of test_socket.c.

This is based on the new version. As explained earlier in comment #23,
this fails. 

Nelson, if you think, we should wait for this getting corrected,
then I will wait and then make a fresh patch and give it for review.
Otherwise, can this be given for review?

Comment 25

[Biswatosh Chakraborty]

Attachment: libpkix

This contains just nss/lib/libpkix and mainly touches
nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_socket.c.

Nelson, you mentioned in your last point in comment #22
that you would now review ocsp.c and pkix_pl_socket.c.
However, in view of the changes you had suggested in that
comment, I have modified those two files and so, you can review
ocsp.c given in Patch3(id: 281825) and pkix_pl_socket.c
in this current attachment. Hence, you can ignore those two files in
the earlier attachmnet(id=281291).

Comment 26

[Biswatosh Chakraborty]

Please refer to Point 9, comment #22.

My finding gives me the answer "yes" to the question.

I tried making server and client addresses with all
the combinations, like:
 (1)server=IPv6,client=IPv4,
 (2)server=IPv4,client=IPv6,
 (3)server=IPv4,client=IPv4,
 (4)server=IPv6,client=IPv46
and each time it worked. Of course, not with the latest test_socket.c
(as mentioned in comment #23, point 3).

Now, how I tried?

When, I am doing "$ pkixutil test_socket 127.0.0.1,234",
the client address becomes IPv4 family and when I do a
"$ pkixutil test_socket ::ffff:127.0.0.1,234", it becomes IPv6
family. 

And, in test_socket.c, in test_socket(), in the section,
below  "subTest("Using pkix_pl_Socket_Create")", the following
function is being called:
PR_SetNetAddr(PR_IpAddrAny,clientNetAddr.raw.family,portNum,&serverNetAddr);
Here, in the 2nd argument, where I should supply the family, I tried
passing PR_AF_INET as well as PR_AF_INET6 and in all cases, "$pkixutil
 test_socket" succeded. 
Putting clientNetAddr.raw.family in the 2nd argument ensures that
the client and the server are of same families and as I saw, both being
IPv6 also works without failure.

Comment 27

[Biswatosh Chakraborty]

Attachment: Patch3. (Consolidated)...containing all changes

Nelson,

This patch is not different from the earlier patches(281825, 281835)
except for test_socket.c. This patch shows the difference between 
the old version and "oldversion+IPV6" changes for test_socket.c
281828 contained the diff of new version and "new version+IPv6".


Since, the patches 281825, 281835 and 281828 are now combined
into one, you may find it better to review this patch instead.
Hence, I am giving this one for review. So, marking earlier patches
obsolete. But, if you are already reviewing those, it would apply to
this patch as well, as they are same except test_socket.c.

Comment 28

[Biswatosh Chakraborty]

Please refer to points 3 and 4 of comment #23 and comment #24
for test_socket.c

Comment 29

[Alexei Volkov]

Biswatosh, please check the port(default is LDAP port) for availability. Also check privileges the user has that runs the test. If you still have the problem, please provide command line argument that you are using to run the test and it's output.

Comment 30

[Biswatosh Chakraborty]

Alexei,

The attachment would tell you the error. 
It is giving "Local Network address is in use" error. With port 34567(that is after running "pkixutil test_socket 127.0.0.1:34567"), netstat shows this:
tcp        0      0 localhost.localdo:34567 localhost.localdo:34044 TIME_WAIT

I am the root on my machine and thus should be having all the permissions.


I ran the command "pkixutil test_socket 127.0.0.1:34567" or even "pkixutil test_socket 127.0.0.1" or for that matter with any other port. 
This is failing with the latest test_socket.c and the moment I introduce
the line: "         PKIX_TEST_EXPECT_NO_ERROR(PKIX_Initialize
                 (PKIX_TRUE, /* nssInitNeeded */
                 useArenas,
                 PKIX_MAJOR_VERSION,
                 PKIX_MINOR_VERSION,
                 PKIX_MINOR_VERSION,
                 &actualMinorVersion,
                 &plContext));
"  in test_socket.c, it succeeds with all the tests.
The old version had this line. 
So, in nutshell, not doing PKIX_Initialize()
fails test_socket and doing that makes it pass all the tests. 
I am attaching the output in the next comment.

Comment 31

[Biswatosh Chakraborty]

Attachment: 2 outputs of "pkixutil test_socket"

The first part is with the new version of test_socket.c
and the second part is with the old version. Details are 
given in the previous comment.

Comment 32

[Julien Pierre]

3.11.7 is done. Retargeting to 3.11.8 .

Comment 33

[Julien Pierre]

Biswatosh, did you determine the reasons for the failures ? And are you going to fix this bug by friday for 3.11.8 ?

Comment 34

[Biswatosh Chakraborty]

Julien,

No, I did not investigate that. I dont think I can do that by 
this friday. Hence, making the target as 3.11.9.
Anyways, I will be contacting Alexei on whether he did anything
on that because the problem happened after a change was done
in the test_socket.c by him (I guess).

Comment 35

[Biswatosh Chakraborty]

After having reported the problem in Comment #23 and in Comment #30
sometime back, I tested it again today with the latest libpkix and 
the problem seems to be gone now. I dont know what changes in libpkix
could solve that but at least this command ($ pkixutil test_socket * )   
seems to be working now.

Comment 36

[Josh Aas]

Is this still an issue? If so, what might it mean for Gecko and Firefox, if anything?