426867 - (https-server) ssl proxy for mochitest

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[(not currently active) Ted Mielczarek]

Attachment: ssltunnel.cpp

I poked through the NSS docs and samples and wrote an SSL proxy. It works sorta like stunnel. The code isn't quite done yet, and I don't have a real makefile for it yet, but I'll attach what I have done so far.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: crappy standalone makefile with hardcoded paths

This is what I'm using to build this right now. It's pretty crappy, but it should actually be easier in the moz build system.

Comment 2

[(not currently active) Ted Mielczarek]

So, right now this is hardcoded to forward all incoming connections to 192.168.1.10:80, because I happen to have a webserver there. I just need to write some commandline handling to allow specifying extra info. You'll need to have an NSS cert db setup for this, it expects to find a cert nicknamed "localhost" to use. You can pass a path to the NSS dbs on the commandline. I set up a CA cert and a server cert like so:

Create db:
certutil -N -d .\db
Create self-signed CA cert:
certutil.exe -d .\db -S -k rsa -n fakeca -t C,C,C -s "CN=fakeca.example.com,O=Fake CA,C=US" -x -v 120 -w -1 -1 -2
Create a regular cert:
certutil.exe -d .\db -S -k rsa -n localhost -t p,p,p -s "CN=localhost,O=Your local host,C=US" -c fakeca -v 120 -w -1 -1

I downloaded certutil.exe from the NSS utils package here: https://ftp.mozilla.org/pub/mozilla.org/security/nss/releases/NSS_3_11_4_RTM/msvc6.0/WINNT5.0_OPT.OBJ/

I'll attach a zip of my cert db in a minute so you don't have to screw around with it as much.

Comment 3

[(not currently active) Ted Mielczarek]

Attachment: cert db dir (zipped)

Comment 4

[(not currently active) Ted Mielczarek]

Attachment: ssltunnel.cpp

Ok, with updated cmdline parsing. Now requires args of the form:
ssltunnel <NSS db path> <remote ip> <remote port> (<cert name> <port>)+

It will proxy traffic from each port specified in a cert name, port pair to the remote ip:remote port, using the cert from the NSS cert db with the nickname "cert name" for that port.

I think this is code complete, it just needs a real makefile now.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 313446 [details]
ssltunnel.cpp

Kai, if you have a few minutes, could you take a look at this and see if I'm doing anything stupid with the NSS API? My code appears to work, but I just fumbled my way through the docs/examples.

Comment 6

[(not currently active) Ted Mielczarek]

Attachment: ssltunnel patch

This just gets it building. Haven't tested on anything but Windows, I'll throw it at the try server. We'll need some logic to start it up in runtests.{pl.py} (can we ditch the perl copy yet?), get a test CA cert into the mochitest profile, and make the PAC setup forward some domains appropriately. We'll also need a NSS db for the server process full of test server certs. I know very little about actual certs, so hopefully someone else can point out what we could use for testing there.

Comment 7

[(not currently active) Ted Mielczarek]

Attachment: ssltunnel patch (with some mac/linux fixes)

A few fixes to make gcc happy, this now builds and works on all three Tier 1 platforms.

Comment 8

[Kai Engert [:KaiE:]]

(In reply to comment #5)
> (From update of attachment 313446 [details])
> Kai, if you have a few minutes, could you take a look at this and see if I'm
> doing anything stupid with the NSS API? My code appears to work, but I just
> fumbled my way through the docs/examples.

On first sight I don't see anything stupid.
I'd rather say, if your code works, great work! :-)

I don't see any cleanup for NSS resources in your tool.
This might be ok if the intention of your tool is use it for quick tests.
However, if the intention of your tool is to be a gateway that is running for extended periods of time, the it would be good to work on the cleanup.
(Like calling CERT_DestroyCertificate etc.)

Comment 9

[Kai Engert [:KaiE:]]

Bob, Nelson, Wan-Teh, I cc'ed you FYI only, you might be interested to hear that such a testing tool is being worked on.
I was able to compile it and get it to run.
Please feel free to look at the latest patch and give feedback to Ted, as you might have more experience with writing NSS based server programs than I have.

Comment 10

[Kai Engert [:KaiE:]]

Comment on attachment 313594 [details] [diff] [review]
ssltunnel patch (with some mac/linux fixes)

Although this tools requires improvements wrt resource correctness, I think it's a good idea to have it available. I was able to compile it and run a test.

I expect that you'll have to improve that tool, though, for example, you'll probably have to forward the HTTP headers the client sent to the target.

Comment 11

[Kai Engert [:KaiE:]]

(In reply to comment #10)
> 
> I expect that you'll have to improve that tool, though, for example, you'll
> probably have to forward the HTTP headers the client sent to the target.

Sorry, what I meant is:

If you connect to a host that uses virtual hosting, the destination expects that you send an HTTP/1.1 header that indicates the desired hostname.

When I connected to https://127.0.0.1:localport that header was missing, as expected, and the target server didn't know which content to send.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 313594 [details] [diff] [review]
ssltunnel patch (with some mac/linux fixes)

Unsolicited review comments... :)

This is a good start in the right direction, but it will leak 
resources a lot and may hang due to resource exhaustion.

1. This code never destroys any objects it gets.  
Sockets, certs, keys: Never destroyed.

For example, all of the returns, after the first, in function 
StartServer return without freeing any of the resources allocated 
up to that point. 
Hint: PR_ShutdownSocket only terminates the connection but doesn't
destroy/release the socket.  Deleting "ci" doesn't either.

2. extreme timeouts

>+  PRIntervalTime connect_timeout = PR_SecondsToInterval(2000);
>+  PRIntervalTime short_timeout = PR_MillisecondsToInterval(100);

33.3 minutes is a LONG time to wait for an initiated connect to fail.
That thread will be tied up a long time.

0ne tenth of a second is a very short time to wait for the arrival 
of the first data from the remote client, and probably far too short
for SSL handshakes to the remote server to complete over real WAN connections, or with busy servers.  

3. Error detection missing in several places.  Examples:

- In main, if PR_StringToNetAddr fails, the code proceeds using 
uninitialized contents of remote_addr.  

- in HandleConnection, If the first PR_Recv call reaches that 0.1 
second timeout limit, it will return -1, and the following line 
will dutifully try to send -1 bytes of data out the other socket. 

>+    PRInt32 bytes = PR_Recv(ci->client_sock, buf, BUF_SIZE, 0, short_timeout);
>+    PR_Send(other_sock, buf, bytes, 0, short_timeout);

4. Don't shutdown or deinitialize NSS or NSPR at end (not a biggie).

Comment 13

[(not currently active) Ted Mielczarek]

Kai: mochitest uses a somewhat complicated system involving a PAC to serve
content from the same server under multiple hostnames. I'm not sure this will
be an actual limitation in practice, but we'll see what happens. Thanks for
looking over my patch!

Nelson: Thanks for the comments! I'll take another pass over it and do some cleanup. I think I switched between milliseconds and seconds there at some point, and forgot to switch the values back. (Oops!) I'll look at doing more cleanup as well. FWIW, my only intended use of this code is as a proxy to a local httpd in our test harnesses, but I can add a comment to that effect.

Comment 14

[(not currently active) Ted Mielczarek]

Attachment: updated to do cleanup etc

Comment 15

[(not currently active) Ted Mielczarek]

Comment on attachment 314068 [details] [diff] [review]
updated to do cleanup etc

Ok, this patch should fix all of Nelson's review comments. I added some stack classes to ensure we cleanup certs/keys/sockets in spite of early return, added some more error checking, and tweaked the timeouts to be more realistic.

Comment 16

[(not currently active) Ted Mielczarek]

Attachment: with cleanup and better shutdown

After some testing, I realized that I wasn't actually shutting down the threadpool if one of the server threads errored out. I added a lock/condvar and signaling so it can all shut down properly in event of an error.

I think this should be good enough for our needs.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 314079 [details] [diff] [review]
with cleanup and better shutdown

Thanks, Ted, This is another big step forward.  A few more small things.
In my prior comments, I gave examples of missing error detection but did 
not attempt to exhaustively list them all.  Here are two more.

1. In HandleConnection: this line:

>+  AutoFD other_sock(PR_NewTCPSocket());

is not followed by a NULL detection.
If PR_NewTCPSocket returns NULL, this code will crash.
This failure is detected when it occurs in StartServer, but not here.
It's more likely to occur here than in StartServer.

2. In StartServer, after this line has succeeded,

>+  AutoFD listen_socket(PR_NewTCPSocket());

we see this line.

>+  listen_socket.reset(SSL_ImportFD(NULL, listen_socket));
>+  if (!listen_socket) {

This is analogous to the classic problem: ptr = realloc(ptr, number);
If SSL_ImportFD returns NULL, then the listen_socket acquired 
above is leaked.  The AutoFD will not close the PR_FileDesc *, because 
it will have been replaced with NULL.  One way to fix this is to change 
the reset operator to not replace _fd if newfd is NULL.

Other suggestions (just suggestions):

a) in HandleConnection, after any operation on ci->client_sock fails
(returns -1), I suggest you proceed directly to call PR_Close and not
attempt to do any further operations (such as other PR_Recv, PR_Send,
or PR_Shutdown) on ci->client_sock .

b) NSS_Shutdown returns SECFailure if any application-allocated NSS
resources are still allocated when it is called.  Checking its return
value is an easy way to detect leaks of NSS objects.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 314079 [details] [diff] [review]
with cleanup and better shutdown

I have no review comments beyond those I put in the previous bug comment.  
How you use them is up to you.  I hope they were helpful.  Regards,

Comment 19

[(not currently active) Ted Mielczarek]

Attachment: even more error checking

Right, I think this handles error returns everywhere possible. It also changes the socket handling to match Nelson's suggestion. I'm checking this patch in as-is.

Nelson: thanks for all the review comments!

Comment 20

[(not currently active) Ted Mielczarek]

Ok, checked this in. I filed bug 428009 for the follow-up work of actually hooking this up to the mochitest harness.

Comment 21

[Walter Meinl]

Ted could it be that the files in testing\mochitest\ssltunnel\ were checked in in DOS-Format? OS/2 build broke with
 E:/PERL/bin/5.8.0/perl.exe E:/usr/src/mozilla/build/autoconf/make-makefile -t E:/usr/src/mozilla -d ../..  ssltunnel/Makefile
creating testing/mochitest/ssltunnel/Makefile
make.exe[5]: Entering directory `E:/mozbuild/testing/mochitest/MochiKit'
make.exe[5]: Leaving directory `E:/mozbuild/testing/mochitest/MochiKit'
make.exe[5]: Entering directory `E:/mozbuild/testing/mochitest/static'
make.exe[5]: Leaving directory `E:/mozbuild/testing/mochitest/static'
make.exe[5]: Entering directory `E:/mozbuild/testing/mochitest/tests'
E:/PERL/bin/5.8.0/perl.exe E:/usr/src/mozilla/build/autoconf/make-makefile -t E:/usr/src/mozilla -d ../../..  SimpleTest/Makefile
creating testing/mochitest/tests/SimpleTest/Makefile
E:/PERL/bin/5.8.0/perl.exe E:/usr/src/mozilla/build/autoconf/make-makefile -t E:/usr/src/mozilla -d ../../..  browser/Makefile
creating testing/mochitest/tests/browser/Makefile
make.exe[6]: Entering directory `E:/mozbuild/testing/mochitest/tests/SimpleTest'
make.exe[6]: Leaving directory `E:/mozbuild/testing/mochitest/tests/SimpleTest'
make.exe[6]: Entering directory `E:/mozbuild/testing/mochitest/tests/browser'
make.exe[6]: Leaving directory `E:/mozbuild/testing/mochitest/tests/browser'
make.exe[5]: Leaving directory `E:/mozbuild/testing/mochitest/tests'
make.exe[5]: Entering directory `E:/mozbuild/testing/mochitest/chrome'
make.exe[5]: Leaving directory `E:/mozbuild/testing/mochitest/chrome'
make.exe[5]: Entering directory `E:/mozbuild/testing/mochitest/ssltunnel'
Makefile:44: ../../..
Makefile:54: *** commands commence before first target.  Stop.
make.exe[5]: Leaving directory `E:/mozbuild/testing/mochitest/ssltunnel'
make.exe[4]: *** [export] Error 2
make.exe[4]: Leaving directory `E:/mozbuild/testing/mochitest'

Here's how the file ssltunnel.cpp starts in a freshly checked-out tree
/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
/* ***** BEGIN LICENSE BLOCK *****

 * Version: MPL 1.1/GPL 2.0/LGPL 2.1

 *

 * The contents of this file are subject to the Mozilla Public License Version

 * 1.1 (the "License"); you may not use this file except in compliance with

 * the License. You may obtain a copy of the License at

 * http://www.mozilla.org/MPL/

 *

 * Software distributed under the License is distributed on an "AS IS" basis,

Comment 22

[(not currently active) Ted Mielczarek]

Yeah, it does look like that, doesn't it? You're welcome to fix that if it's causing you a problem.

Comment 23

[Walter Meinl]

Attachment: correct line endings

Simply a white space fix as the DOS line endings break OS/2. I've no rights to check it in in case of r+ and a+.

Comment 24

[Jeff Walden [:Waldo]]

Test-only code, not shipped, yadda yadda, I just went ahead and changed the line endings.

Comment 25

[(not currently active) Ted Mielczarek]

Comment on attachment 314962 [details] [diff] [review]
correct line endings

Yeah, I was gonna say "just do it".

Comment 26

[Walter Meinl]

(In reply to comment #24)
> Test-only code, not shipped, yadda yadda, I just went ahead and changed the
> line endings.
> 

(In reply to comment #25)
> (From update of attachment 314962 [details] [diff] [review])
> Yeah, I was gonna say "just do it".
> 
Sorry, I didn't and don't want to hold you up, but as said before I myself can't correct it. Pinging Peter to correct the line endings in Makefile.in.

Comment 27

[Peter Weilbacher]

Thanks for the hint, Walter. As Jeff had only changed the linebreaks in the source file but not Makefile.in of the test I just did that to really unbreak to OS/2 build.

Comment 28

[Kai Engert [:KaiE:]]

ssltunnel fails to build in some supported environments, and therefore the build aborts. filed bug 429178

Comment 29

[Jeff Walden [:Waldo]]

Moving a bunch of Core :: Testing bugs to Testing :: Mochitest to clear out the former, which is obsolete now that we have more specialized categories for such bugs; filter on the string "MochitestMmMm" to delete all these notifications.