Closed Bug 428009 Opened 16 years ago Closed 16 years ago

hook up ssltunnel to mochitest

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ted, Assigned: mayhemer)

References

Details

(Whiteboard: [fixed in 1.9.1b1])

Attachments

(2 files, 8 obsolete files)

bug 426867 introduced a ssltunnel binary that can be used to tunnel SSL traffic to an arbitrary httpd. We should hook this up so it's available in Mochitest. (Waldo, hope you don't mind owning this!)
Right now the server just responds to any and all requests that come its way, completely disregarding the target host/port and exposing bogus information to handlers.  This patch would make it possible to write handlers that can discriminate based on the host/scheme/port, which I believe is a necessary part of really getting this working.  This passes the smoketest of a few telnet sessions, and Mochitest starts up, runs, and seems to do okay with the postMessage tests that work on a bunch of different domains, but it's heavily undertested, inaccurately documented, and bizarrely abstracted right now.  Also, it's duplicating the list of servers in use (fixable, but not fixed yet).

These problems need to be solved before this can go any further, but with it posted here at least it's backed up off my system.  The ssltunnel-related work should be a good bit simpler and quicker than the server hacking, I expect.
This seems to work fine (haven't run the entire set of Mochitests, but isolated portions involving cross-domain stuff work fine), but it lacks tests targeted at the new functionality that ensure we return 400s and set host/scheme/etc. appropriately given the right inputs.
Attachment #316821 - Attachment is obsolete: true
Attached patch Test, 1 (obsolete) — Splinter Review
This is mochitest that test Jeff's patch. All passes.
Attachment #320173 - Flags: review?(jwalden+bmo)
Comment on attachment 320173 [details] [diff] [review]
Test, 1

Actually, I plan to write xpcshell tests for this, just as I've done for past server modifications.  At a crude level Mochitest tests the changes in the patch, but it doesn't test any of the more interesting things that are changed -- dynamic addition and removal of locations from the identity of a server, determination of host and port from an incoming request, calculation of the scheme/host/port as exposed to request handlers, etc.  The ability to write custom request handlers and to dynamically interact with the server is just as important as the "static" ability of servers to respond to requests correctly.  I find it's much easier to find my mistakes due to future changes when I've rigorously tested what might break, and this just isn't enough to do it.
Attachment #320173 - Flags: review?(jwalden+bmo)
Oops, wasn't aware of that. Do you want to write the tests your self or should I do that? Currently I am testing ssltunnel program and have problems with it - the data relay code seems not to be reliable. I will change it and possibly preserve support for HTTP proxy-like behavior (CONNECT + 200 CONNECTED support); that should probably be done in a new bug.
I plan to write them myself.

I've been thinking about the SSL behavior and reading up on exactly how SSL and SSL proxy behavior works, and I haven't seen an obvious way to support this behavior of starting unencrypted and then switching to encrypted after the CONNECT/200 happens or if one isn't received.  One thing that might be reasonably simple would be to write an NSPR layer to check for "CONNECT example.com:443 HTTP/1.1\r\nHost: example.com:443\r\n\r\n" and respond with a 200 OK if that's received and otherwise (or after that) to just transparently pass through data after that.

Also, I noted the read timeout of 250ms in HandleConnection while going through the code; given that the HTTP server currently generates content all-at-once, I wouldn't be surprised if we exceeded that time on particularly large responses.  That value might need to be bumped up if we ever run into problems with it, but hopefully I can just fix bug 396226 before we hit that problem.
(In reply to comment #6)
> I plan to write them myself.
> 
> I've been thinking about the SSL behavior and reading up on exactly how SSL and
> SSL proxy behavior works, and I haven't seen an obvious way to support this
> behavior of starting unencrypted and then switching to encrypted after the
> CONNECT/200 happens or if one isn't received.  One thing that might be
> reasonably simple would be to write an NSPR layer to check for "CONNECT
> example.com:443 HTTP/1.1\r\nHost: example.com:443\r\n\r\n" and respond with a
> 200 OK if that's received and otherwise (or after that) to just transparently
> pass through data after that.
> 

There is very simple way to do it - the listen socket may only be a TCP socket and SSL server socket might be imported/overlapped later, after the HTTP proxy behavior is handled. I have rich experience with it and actually, I am just working on it.

> Also, I noted the read timeout of 250ms in HandleConnection while going through
> the code; given that the HTTP server currently generates content all-at-once, I
> wouldn't be surprised if we exceeded that time on particularly large responses.
>  That value might need to be bumped up if we ever run into problems with it,
> but hopefully I can just fix bug 396226 before we hit that problem.
> 

The current code has IMHO gaps. I am unable to load even the root page using it. I rewrote the code that retransmits data to be a generic relay using non-blocking sockets and PR_Poll and it is getting better. Still working on it.
Attachment #320173 - Attachment is obsolete: true
this patch:
- fixes ssltunnel relay problems
- turns ssltunnel to a very simple HTTP SSL tunnel proxy
- automatically generates a temporary ca and issues a temporary ssl server certificate for https:// hosts from the server locations file
- adds the temp ca to the profile cert db as a trusted issuer
- runs configured ssltunnel proxy with the server cert
- adds new option to the server list (nocertificate) to allow connection to that site but only with added exception for that host/domain
- fixes process kill on windows
- adds option to easily pass a string to stdin of a child process (needed for certutil to generate the ca)
- adds build of certutil to security/manager/Makefile.in when tests are enabled
Attachment #320777 - Flags: review?(sayrer)
Attachment #320777 - Flags: review?(nelson)
Attachment #320777 - Flags: review?(jwalden+bmo)
I really don't want ssltunnel to be an HTTP proxy, I would like it to continue to be a protocol-agnostic tunnel.
Also, I think we should probably just have a pre-built cert db checked in, instead of generating them from scratch every time.
(In reply to comment #9)
> I really don't want ssltunnel to be an HTTP proxy, I would like it to continue
> to be a protocol-agnostic tunnel.
> 

Ok, however, we need this. Then to have sslproxy.cpp including http proxy changes would be ok?

(In reply to comment #10)
> Also, I think we should probably just have a pre-built cert db checked in,
> instead of generating them from scratch every time.
> 

I also have that tough, there is no argument from my site not to do that this way and what you suggest is probably better for testing. Just, is the format portable between platforms? Isn't it advantage to have a dynamic list of hosts for which we can easily change the cert?
Is there a reason we need an HTTP proxy? I don't see that spelled out here. I was imagining that just forwarding the HTTP requests to httpd.js would work fine. (Forwarding them to apache worked fine in my testing.)

> Isn't it advantage to have a dynamic list of hosts
for which we can easily change the cert?

Changing the host list will require checking in a change, so I don't think it's a big deal to have to check in a new cert if you need one. In addition, I imagine that we'll want to test a variety of broken or weird cert cases, to test our security UI, so we'll probably want to generate these manually. In fact, I believe we'll probably need only a small number of working certs in the db, probably to test DV and EV certs, and then many more invalid or broken certs.
(In reply to comment #12)
> Is there a reason we need an HTTP proxy? I don't see that spelled out here. I
> was imagining that just forwarding the HTTP requests to httpd.js would work
> fine. (Forwarding them to apache worked fine in my testing.)
> 

Mochitest (actually automation.py.in) defines the FindProxyForURL function to allow multiple to virtually proxy to several hosts in the list. When an https host is required to connect from the mochitest profile environment then proxing to the ssltunnel as it was wasn't sufficient. Firefox (and any browser) sends through unencrypted connection the CONNECT method request and expects 200 Connected response. After that it start ssl connection negotiation with the target server. This is what my patch allows and here I explain why we need it.

> > Isn't it advantage to have a dynamic list of hosts
> for which we can easily change the cert?
> 
> Changing the host list will require checking in a change, so I don't think it's
> a big deal to have to check in a new cert if you need one. In addition, I
> imagine that we'll want to test a variety of broken or weird cert cases, to
> test our security UI, so we'll probably want to generate these manually. In
> fact, I believe we'll probably need only a small number of working certs in the
> db, probably to test DV and EV certs, and then many more invalid or broken
> certs.
> 

I agree. However, I would like to here opinions from other people about this. Maybe this is interesting feature or it could be enhanced to support what you suggest.
(In reply to comment #12)
> Changing the host list will require checking in a change, so I don't think it's
> a big deal to have to check in a new cert if you need one. In addition, I
> imagine that we'll want to test a variety of broken or weird cert cases, to
> test our security UI, so we'll probably want to generate these manually. In
> fact, I believe we'll probably need only a small number of working certs in the
> db, probably to test DV and EV certs, and then many more invalid or broken
> certs.

Assuming that we generate our own CA and trust it in the profile, generating "valid" DV certs is easy, but generating an EV cert is slightly harder.  Above and beyond the CA cert being included in the cert DB, the CA's policy OID and associated info needs to be added to the myTrustedEVInfos[] array here:

http://mxr.mozilla.org/mozilla/source/security/manager/ssl/src/nsIdentityChecking.cpp#83

Presumably we'll want to wrap this in #ifdef ENABLE_TESTS or something similar, since we don't want end user builds trusting our testing CA.  The policy OID is just a numeric string identifier like "1.3.6.1.4.1.14370.1.6."  In the real world, it identifies the specific policy statement under which a given cert was issued (EV, DV, Code signing, etc.) but for our purposes, obviously, it just needs to be some string that we associate with our "EV" certs.

I guess while I'm thinking about it, we'll at least want:

 - A valid "DV" host (maybe a couple?)
 - A valid "EV" host
 - A host using a self-signed cert
 - A host using an expired-but-otherwise-valid cert
 - A couple of obvious mismatch scenarios, e.g.
   - Invalid use of parent cert (sub1.example.com using example.com cert)
   - Invalid use of child cert (example.com using sub1.example.com cert)
   - Use of unrelated cert (example.com using example.org cert)

There are a lot of other SSL states, that's nowhere near exhaustive, but it would be great coverage for a lot of our security UI code as a beginning, it allows us to construct other things, like mixed-content tests, and I'm really not trying to be exhaustive out the gate - just useful.

Thanks again for your work here Ted, Waldo, Honza.  I don't want to add another cook to a clearly well-staffed kitchen getting the framework built, but I look forward to writing tests once it's live.
(In reply to comment #14)
> but generating an EV cert is slightly harder.  Above
> and beyond the CA cert being included in the cert DB, the CA's policy OID and
> associated info needs to be added to the myTrustedEVInfos[] array here:
> 
> http://mxr.mozilla.org/mozilla/source/security/manager/ssl/src/nsIdentityChecking.cpp#83
> 
> Presumably we'll want to wrap this in #ifdef ENABLE_TESTS or something similar,
> since we don't want end user builds trusting our testing CA.  

In debug builds there is possible to add a file that contains testing manual EV certs. It could be possibly enhanced to work also in test-enabled builds.
(In reply to comment #13)
> Mochitest (actually automation.py.in) defines the FindProxyForURL function to
> allow multiple to virtually proxy to several hosts in the list. When an https
> host is required to connect from the mochitest profile environment then proxing
> to the ssltunnel as it was wasn't sufficient. Firefox (and any browser) sends
> through unencrypted connection the CONNECT method request and expects 200
> Connected response. After that it start ssl connection negotiation with the
> target server. This is what my patch allows and here I explain why we need it.

Ah, gotcha. Sorry, I didn't get that initially. I think it would be better to have a little script or something act as a proxy instead then, even though that means another layer. Maybe we could double route this through httpd.js?
Comment on attachment 320777 [details] [diff] [review]
Integration of ssltunnel to mochitest

Sorry, I cannot review this patch.
Attachment #320777 - Flags: review?(nelson)
(In reply to comment #16)
> Maybe we could double route this through httpd.js?
> 

I don't think its necessary to do it that complicated way when the work were already done. But if you have any idea tell me please. And just a note, the changes of the code that relays data is needed, w/o it ssltunnel doesn't work at all.
Comment on attachment 320777 [details] [diff] [review]
Integration of ssltunnel to mochitest

I skimmed a bit since we're still hashing things out, definitely expect more comments on the next iteration.

(In reply to comment #8)
> - adds option to easily pass a string to stdin of a child process (needed for
> certutil to generate the ca)

Your tempfile gunk isn't necessary (try StringIO, I think).  I'm all but certain the stdin you pass to Popen just needs to quack like a file, not actually be one.


> - adds build of certutil to security/manager/Makefile.in when tests are
>   enabled

I'd rather we not depended on ENABLE_TESTS, frankly; tests should not be optional code.  I'd love if certutils were built unconditionally to make fooling with certs easier for anyone doing a build, but that might not fly with NSS people.  Maybe hack up a cert-generation program that links against NSS to do this?  It's suck every way.


(In reply to comment #11)
> (In reply to comment #9)
> > I really don't want ssltunnel to be an HTTP proxy, I would like it to
> > continue to be a protocol-agnostic tunnel.
> 
> Ok, however, we need this. Then to have sslproxy.cpp including http proxy
> changes would be ok?

Maybe an --http flag to indicate it's proxying HTTP to expect the CONNECT line and headers?


> (In reply to comment #10)
> > Also, I think we should probably just have a pre-built cert db checked in,
> > instead of generating them from scratch every time.

(In reply to comment #12)
> > Isn't it advantage to have a dynamic list of hosts for which we can
> > easily change the cert?
> 
> Changing the host list will require checking in a change, so I don't think
> it's a big deal to have to check in a new cert if you need one.

Checking in is easy -- it's generating the certs that's hard (I spent several hours trying and failing to do it, and that's ignoring the time spent getting certutils built).  Forcing would-be-testwriters through this pain is not cool.  I think we should generate at compile time and then install at runtime (note this *not* like the patch, which does it all at runtime).

I don't know how often we'll want to add new locations at which tests will run; we've added a fair number since I initially checked in support for it.  I very much don't want to give up the ease of use we currently have with respect to adding new locations.


> In addition, I imagine that we'll want to test a variety of broken or weird
> cert cases, to test our security UI, so we'll probably want to generate
> these manually. In fact, I believe we'll probably need only a small number
> of working certs in the db, probably to test DV and EV certs, and then many
> more invalid or broken certs.

Perhaps we generate the certs to a directory in the build tree and then copy over all the certs from a testing/mochitest/ssltunnel/certs directory or something like that.  I don't fully understand everything we need, to be honest, and I'd prefer leaving that to someone else in a much smaller future change.


(In reply to comment #14)
> (In reply to comment #12)
> Assuming that we generate our own CA and trust it in the profile,
> generating "valid" DV certs is easy, but generating an EV cert is slightly
> harder.  Above and beyond the CA cert being included in the cert DB, the
> CA's policy OID and associated info needs to be added to the
> myTrustedEVInfos[] array here:
> 
> http://mxr.mozilla.org/mozilla/source/security/manager/ssl/src/nsIdentityChecking.cpp#83
> 
> Presumably we'll want to wrap this in #ifdef ENABLE_TESTS or something
> similar, since we don't want end user builds trusting our testing CA.  The
> policy OID is just a numeric string identifier like "1.3.6.1.4.1.14370.1.6."
> In the real world, it identifies the specific policy statement under which
> a given cert was issued (EV, DV, Code signing, etc.) but for our purposes,
> obviously, it just needs to be some string that we associate with our "EV"
> certs.

Perhaps a preference to enable/disable use of this at startup, stored in a static boolean location, to control this behavior?  (There's basically no overhead beyond a single preference-get to it.)  I'm opposed to a solution which relies on build flags to work (either ENABLE_TESTS or DEBUG, since I think we want tests to be runnable using any arbitrary binary -- not to mention that tests are important and shouldn't be optional for symbolic reasons if nothing else, but I digress).


> I guess while I'm thinking about it, we'll at least want:

These want to be followup bugs; we want enough to hit the run-from-https use case and not much more than that on a first pass (the certs directory idea would hopefully address the anticipatory changes you'd need to do these things without too much difficulty).


(In reply to comment #16)
> (In reply to comment #13)
> I think it would be better to have a little script or something act as a
> proxy instead then, even though that means another layer. Maybe we could
> double route this through httpd.js?

That doesn't work because httpd.js serves one response at a time.  You need bug 396226 fixed to really make this work correctly, and it's not trivial to make that work; one of these days I'll get it working, but don't count on it any time soon.  Given my schedule, there's a decent it won't even happen this year.  :-\
Attachment #320777 - Flags: review?(jwalden+bmo) → review-
Honza: given all of that, I would support some sort of command-line flag to enable http proxy mode on ssltunnel, since you already have that code written, and it sounds like the easiest path to success here.

Also, since I forgot to say so earlier, thanks for jumping in here and cleaning up ssltunnel!
So, I would like to make the testing environment as easy of use as possible. My opinion of further works follows:

 1. I won't do automatic generation of certificates at runtime because it seems to be too much bounding and inflexible. (Fortunately it was not the most time consuming think to do ;))

 2. Many of you agreed that checking in a pre-filled certification database is the way (Kai has ensured me it is portable); it allows others writing tests add new certificates among existing ones and reference them by nickname (described bellow) or use just the current set if it is suitable for the test.

 3. Point 2 means that there is no need to build certutil and any other stuff; however, it is useful to have it and is obvious tool for creation of new certificates. I will leave it as part of this patch.

 4. I have already implemented and tested cmdline option '--httpproxy' of ssltunnel; both behavior models are preserved

 5. if that option is engaged I want to allow selection of the certificate not only by a port but also by a host to connect. the reason is that I want to allow different tests use port 443 (which is in some cases internally represented as '-1' - the default value for HTTPS) but with different certificate each. there is no way to distinguish only by port in such a case.

 6. test writers therefor must besides a new cert also add a new host to the server-locations.txt file and use new option (as currently 'privileged' is) called e.g. 'cert:<certnickname>' that bonds the new virtual host name with the new certificate. this way allows two or more hosts use the same cert and, different certs might be available on a single port.

Please let me know your opinions, I am about to start working on it ASAP. The first test written will be for bug 413909.
(In reply to comment #21)
> So, I would like to make the testing environment as easy of use as possible. My
> opinion of further works follows:
> 
>  1. I won't do automatic generation of certificates at runtime because it seems
> to be too much bounding and inflexible. (Fortunately it was not the most time
> consuming think to do ;))
> 
>  2. Many of you agreed that checking in a pre-filled certification database is
> the way (Kai has ensured me it is portable); it allows others writing tests add
> new certificates among existing ones and reference them by nickname (described
> bellow) or use just the current set if it is suitable for the test.

Right, I think the majority of tests will either use existing certs, or the test-author will know enough about certs to know the kind of certs they want. (kai or johnath, for example)

>  3. Point 2 means that there is no need to build certutil and any other stuff;
> however, it is useful to have it and is obvious tool for creation of new
> certificates. I will leave it as part of this patch.

I think this is a good idea, as long as we make sure that it's not getting packaged. (see patch on bug 430052) I had a hard time getting a working certutil for testing of ssltunnel.

>  4. I have already implemented and tested cmdline option '--httpproxy' of
> ssltunnel; both behavior models are preserved

Excellent, thanks!

>  5. if that option is engaged I want to allow selection of the certificate not
> only by a port but also by a host to connect. the reason is that I want to
> allow different tests use port 443 (which is in some cases internally
> represented as '-1' - the default value for HTTPS) but with different
> certificate each. there is no way to distinguish only by port in such a case.
> 
>  6. test writers therefor must besides a new cert also add a new host to the
> server-locations.txt file and use new option (as currently 'privileged' is)
> called e.g. 'cert:<certnickname>' that bonds the new virtual host name with the
> new certificate. this way allows two or more hosts use the same cert and,
> different certs might be available on a single port.

This sounds reasonable. I wonder if it doesn't make sense to support some sort of configuration file, instead of just trying to cram this all onto the commandline? The existing commandline is pretty confusing already, if we try to offer multiple options it might be too much. Even a very simple ini-like format would probably work fine.

[localhost]
cert=localhost-cert
port=443

or something similar? I don't want to make anything that's a pain in the ass to implement, but I'd like to make it somewhat intuitive for people to add new entries in the future.

> Please let me know your opinions, I am about to start working on it ASAP. The
first test written will be for bug 413909.

Awesome! Can't wait to get this hooked up and get tests using it.
(In reply to comment #22)
> (In reply to comment #21)
> I wonder if it doesn't make sense to support some sort
> of configuration file, instead of just trying to cram this all onto the
> commandline? The existing commandline is pretty confusing already, if we try to
> offer multiple options it might be too much. Even a very simple ini-like format
> would probably work fine.
> 
> [localhost]
> cert=localhost-cert
> port=443
> 
> or something similar? I don't want to make anything that's a pain in the ass to
> implement, but I'd like to make it somewhat intuitive for people to add new
> entries in the future.
> 

Thanks for this feedback! This is good idea. We might move all options including htts proxy switch to a config file. For simplicity of use and implementation we could use format like this:

httpproxy:on
certdbpath:/usr/x/ssltunnel/certdb/
www.mypage.com:443:test_cert1
:5443:certforport5443  # certificate for my tests

When ssltunnel runs w/o any parameter it could show example file like this. It would take just a path as a single parameter to locate the config file file. This file will be generated by automation.py instead of the list of parameters.
(In reply to comment #21)
>  2. Many of you agreed that checking in a pre-filled certification database
> is the way (Kai has ensured me it is portable); it allows others writing
> tests add new certificates among existing ones and reference them by
> nickname (described bellow) or use just the current set if it is suitable
> for the test.

If you're going to build certutil, I don't see why the vast majority of the certs shouldn't be generated at compile time during make libs or something.  There will be exceptions, but we should make the common case as simple as possible.  Nobody is going to want to have to fool with the certutil commandline if they can help it, and you already basically got the code for it working once already, so it shouldn't be huge overhead to make it work again, I'd think.
(In reply to comment #24)
> If you're going to build certutil, I don't see why the vast majority of the
> certs shouldn't be generated at compile time during make libs or something. 
> There will be exceptions, but we should make the common case as simple as
> possible.  Nobody is going to want to have to fool with the certutil
> commandline if they can help it, and you already basically got the code for it
> working once already, so it shouldn't be huge overhead to make it work again,
> I'd think.
> 

Yes, but when someone wants to add a new certificate that for example could no be generated with certutil from some reason or it is coming from outside - a third party certificate - then it should be added to the database in the repository and should remain there.

Other option is to check-in certificates in .crt or .p12 files and import them during build time.

I created a script that generates the default database but it is not run during compile time.
As I said, dump the exceptional cases into testing/mochitest/ssltunnel/certs and copy them from there into the DB, but don't penalize the simple cases with onerous work every time a new cert or schemehostport must be added.
To clarify: you want to have testing/mochitest/ssltunnel/certs that contains 
1) the default certificates = ssl server cert and key + temp CA cert 
2) any custom cert from testers

When running the mochitest, a new database will be created and all certs and p12 modules will be imported to it? Nickname would then be based on the filename. It would process files with .crt extension (just pure certificates) as CAs i.e. import them to the profile cert database and .p12 files as server certificates with key that will be imported into the ssltunnel certdb. Would you agree on that?

I personally don't see any advantage in this approach because certutil and p12util already use cert database to do their work. So, doing operation on a temporary database, extracting the cert (an additional operation for the tester) and coping it into a directory where from it is imported again to another database seems to me more complicated then to simply work on that database directly.
And one more think: to generate certificates using certutil during build time requires a random file or random input from a user (the former is difficult to do on all platforms and the letter is unacceptable).
I believe what Waldo is suggesting is:

testing/mochitest/ssltunnel/certs contains:
1) the CA cert
2) exceptional certs for specific cases (domain name mismatch etc)
these could be in the form of a certdb, I don't think it matters.

then, at compile time, certs for all other cases would be generated from the hostname list and inserted into the certdb, so that adding a new SSL host with a valid cert would just entail adding it to the list.

(In reply to comment #29)
> I believe what Waldo is suggesting is:
> 
> testing/mochitest/ssltunnel/certs contains:
> 1) the CA cert
> 2) exceptional certs for specific cases (domain name mismatch etc)
> these could be in the form of a certdb, I don't think it matters.
> 
> then, at compile time, certs for all other cases would be generated from the
> hostname list and inserted into the certdb, so that adding a new SSL host with
> a valid cert would just entail adding it to the list.

In case it might otherwise get forgotten, I'll mention explicitly the issue of the CA private key.  Signing new certs at some point in the future will require the private key for the CA, which means either:

 a) Storing the private key somewhere, along with any passkey used
 b) Regenerating the CA cert, all "normal" certs, and the new certs, any time an additional cert is required for some test down the road

Obviously, a) is less work.  The only downside is that it means any one can pull our tree, use that CA cert to sign new certs, and deceive instances of Firefox configured for testing.  I don't think that's a problem, as long as we are *positive* that trust for the test ca is not in a code path that anyone will hit by accident, or that we will ever accidentally include in a released build.
To summarize the goals:

1. we want to allow people with minimal effort just add new hosts to the list and have automatically a certificate (+ key) for them

2. we want to allow testers writing bug tests add new custom certificates (+ keys) and use it with new hosts (customcert:<nickname> option in server-locations.txt to bond)

To allow usage of custom certs (from paragraph 2) we should trust its issuer; this can be made by two ways: 
  a) we can have a fixed CA+key that is used to sign certs from paragraph 1 and can be used to sign custom certs 
  b) we have a run-time CA+key (as the patch does) and each custom certificate  have to have its own CA which is added as trusted along with the run-time CA; this forces creators of the custom certs to also create its own CAs

Personally, I can imagine tests where also a custom CAs are needed. Then option a) and b) must both be supported.

So, as Johnathan says, let's have a fixed CA+key stored in p12 module somewhere, and let's have a directory ssltunnel/certs where content follows:

*.server - p12 modules of server cert + key to be present in the ssltunnel db, nickname is the filename
*.ca - binary certs of CAs that have to be added as trusted to the database in the mochitest profile; this will also include the binary cert of our fixed CA

Content of the directory will be walked by automation.py and both databases will be created. It will also walk the list of hosts and for all https:// that has not any of 'nocert' either 'customcert:<nickname>' automatically creates a server certificate (as the patch does) and will use the fixed CA+key to sign it.

It seems to me a bit complicated and potentially confusing. What are your opinions?

PS: The fixed CA will obviously never be used as trusted outside the mochitest environment, so there is IMHO no security risk.
And one more cosmetic note: shouldn't we move ssltunnel and all certificates/keys to build/pgo? because automation.py residing in that dir takes care about run ssltunnel and prepare certs/environment for it to allow its usage in the profile.
(In reply to comment #29)
> I believe what Waldo is suggesting is:

Yeah, this is what I was suggesting.

> these could be in the form of a certdb, I don't think it matters.

No, not overly much, although I think keeping each in a separate file makes it easier to analyze the certs individually.


(In reply to comment #30)
> I don't think that's a problem, as long as we are *positive* that trust for
> the test ca is not in a code path that anyone will hit by accident

If we add a single preference "security.enable_unsafe_testing_cert_authority" which must be present with the exact value true to toggle the behavior and isn't in all.js at all, and this check occurs once at startup time, I think that's perfectly fine.


(In reply to comment #31)
> 1. we want to allow people with minimal effort just add new hosts to the list
> and have automatically a certificate (+ key) for them

Yeah, basically.  Is there a reason we couldn't just use a single cert with lots of dNSNames for this?  I've been assuming all the non-special ones would just use a single cert, and the special ones would specify a cert by nickname.  I'm more a fan of certificate=nickname for the entry in server-locations.txt than the syntaxes you've suggested, but that's just nitpicking.

> It seems to me a bit complicated and potentially confusing.

I'm more than willing here to take the copout answer that security's hard and that there's no truly simple solution here.  ;-)  Hopefully the patch will spur better ideas (and if not, something's better than nothing).

(In reply to comment #32)
> shouldn't we move ssltunnel and all certificates/keys to build/pgo?

Perhaps.  I hear Mercurial makes moving files easy, which would be nice if you end up doing this.  :-)
(In reply to comment #33)
> (In reply to comment #30)
> > I don't think that's a problem, as long as we are *positive* that trust for
> > the test ca is not in a code path that anyone will hit by accident
> 
> If we add a single preference "security.enable_unsafe_testing_cert_authority"
> which must be present with the exact value true to toggle the behavior and
> isn't in all.js at all, and this check occurs once at startup time, I think
> that's perfectly fine.
> 

I don't think we need any option for it. The CA will be added as trusted only into the testing profile cert db. This is already implemented in the next version of the patch. Also take into account there will be need to add custom CAs. I don't know how the "security.enable_unsafe_testing_cert_authority" should recognize such "testing" CAs and filter them off.

> 
> (In reply to comment #31)
> > 1. we want to allow people with minimal effort just add new hosts to the list
> > and have automatically a certificate (+ key) for them
> 
> Yeah, basically.  Is there a reason we couldn't just use a single cert with
> lots of dNSNames for this?  I've been assuming all the non-special ones would
> just use a single cert, and the special ones would specify a cert by nickname. 

That is exactly what I have now (and actually it is also doing the current patch).

> I'm more a fan of certificate=nickname for the entry in server-locations.txt
> than the syntaxes you've suggested, but that's just nitpicking.
> 

Yes, I am doing this (almost have it - just struggling with parsing of server-locations :)) *.server files (p12 modules) in ssltunnel/certs dir will be all added into the ssltunnel database with nickname that is stored in the p12 module and will be referenced with cert=nickname option in server-locations.

> > It seems to me a bit complicated and potentially confusing.
> 
> I'm more than willing here to take the copout answer that security's hard and
> that there's no truly simple solution here.  ;-)  Hopefully the patch will spur
> better ideas (and if not, something's better than nothing).
> 

Agree ;)
(In reply to comment #34)
> I don't think we need any option for it. The CA will be added as trusted only
> into the testing profile cert db. This is already implemented in the next
> version of the patch. Also take into account there will be need to add custom
> CAs. I don't know how the "security.enable_unsafe_testing_cert_authority"
> should recognize such "testing" CAs and filter them off.

I'm probably just mixing problems together; this would be needed for the EV testing that I want punted to a followup bug, and my brain is just turning to mush.  :-)
(In reply to comment #35)
> (In reply to comment #34)
> > I don't think we need any option for it. The CA will be added as trusted only
> > into the testing profile cert db. This is already implemented in the next
> > version of the patch. Also take into account there will be need to add custom
> > CAs. I don't know how the "security.enable_unsafe_testing_cert_authority"
> > should recognize such "testing" CAs and filter them off.
> 
> I'm probably just mixing problems together; this would be needed for the EV
> testing that I want punted to a followup bug, and my brain is just turning to
> mush.  :-)

You and me both - yeah, the pref is only needed for the EV trust, since that's not done in the cert DB.  And that can definitely be punted to another bug.  :)
Okay, this is ready to go.  Mostly it's just test changes since the last patch, but there's some functionality change to deal with the somewhat convoluted way I make primary location designation and the automatically-generated http://127.0.0.1:PORT location interact.  All the server xpcshell tests and all Mochitests pass with this patch applied; most of the other xpcshell tests pass with it applied, but I don't know whether all of them do because of bug 434234 and a couple other tests that assert and die when I run them.

Not entirely sure who's best to review this; I'd say biesi for all the server changes if I could, except that he tends to have a full schedule.  Instead let's try biesi for the head_utils.js changes alone (want to make sure I'm not accidentally introducing any race conditions or misuse of APIs for socket creation/reading/writing), Ted for the automation and build config-y changes, and sayrer for the server changes.  Or something like that.  I really don't know someone perfect for reviewing the server changes, since they're moderately complicated to dissect.

(I think we can agree this is a bit more testing than a simple Mochitest would have been, eh?  :-) )
Attachment #319999 - Attachment is obsolete: true
Attachment #321439 - Flags: review?(ted.mielczarek)
Attachment #321439 - Flags: review?(sayrer)
Attachment #321439 - Flags: review?(cbiesinger)
Could you please add a change to the patches, that makes sure that certutil will not get packaged? (probably equivalent to attachment 316806 [details] [diff] [review])
I have ready a patch addressing all comments here, including move of ssltunnel and new certs directory to build/pgo. But I have a problem - ssltunnel binary is being build too soon (before even nspr), all include headers are missing. Do you know how to delay the build? I would like to preserve this location however, if there were no way to do it, I have to return back to testing/mochitest :(
There's the gross hack of $(MAKE) -C $topsrcdir/build/pgo ssltunnel.  That said, I don't think it's really necessary that ssltunnel itself live in build/pgo -- certs and other stuff, sure, but ssltunnel itself works just fine where it is.
- ssltunnel is left at the same place: testing/mochitest/ssltunnel
- ssltunnel changed to be configured by file
- ssltunnel allows custom certificates for different hosts while listening on just a single port in httpproxy mode
- created build/pgo/certs for managing certificates, contains pre-generated CA for test certificates, it is checked-in to be persistent and be used for simple signing of new custom server certificates
- this CA can be manually re-generated via py script in _profile/pgo
- the server cert (for ssltunnel) is generated at compile-time (of testing/mochitest **) and added with all other custom build/pgo/certs/*.server files to prepared database in _profile/pgo/certs
- the list of custom CAs (including the default one) from build/pgo/certs/*.ca is added at run-time to the profile (mochitest) database
- custom server certs reference is driven from server-locations.txt using cert=<nickname> option
- left TemporaryFile for input redirect (StreamIO didn't work)

I should create a wiki page how to use this beast. Could you navigate me where it can be added? Probably new chapter in docs/Mochitest?

**) I struggled with building binaries or running scripts from libs: target in build/pgo/Makefile.in. No success. It is too soon - script might not yet exist, binaries are dependent on non yet existing headers.
Attachment #320777 - Attachment is obsolete: true
Attachment #320777 - Flags: review?(sayrer)
(In reply to comment #41)
> - left TemporaryFile for input redirect (StreamIO didn't work)

Do you really mean StreamIO, or did you actually attempt to use StringIO?
I attempted to use it:

  File "D:\mozilla-HEAD\mozilla\_obj-browser-debug\_profile\pgo\automation.py", line 80, in __init__
    stdin = inputfile)
  File "c:\mozilla-build\python25\lib\subprocess.py", line 586, in __init__
    errread, errwrite) = self._get_handles(stdin, stdout, stderr)
  File "c:\mozilla-build\python25\lib\subprocess.py", line 680, in _get_handles
    p2cread = msvcrt.get_osfhandle(stdin.fileno())
AttributeError: StringIO instance has no attribute 'fileno'
Perhaps use .communicate() instead on the Popen object, then?  Since we spawn processes from new threads, as I recall, it shouldn't be a problem that the function waits for EOF from the process.
I was trying to do it as well for windows. It doesn't work. I checked it ones again doing exactly this:

     command = os.path.abspath(command)
     if IS_WIN32:
+      import tempfile
       import subprocess
+      
       cmd = [command]
       cmd.extend(args)
       p = subprocess.Popen(cmd, env = env,
                            stdout = subprocess.PIPE,
                            stderr = subprocess.STDOUT)
       self._out = p.stdout
+      self._indata = inputdata
     else:
       import popen2
       cmd = []
       if env != None:
         for (k, v) in env.iteritems():
           cmd.append(k + "='" + v + "' ")
           
       cmd.append("'" + command + "'")
       cmd.extend(map(lambda x: "'" + x + "'", args))
       cmd = " ".join(cmd)
       p = popen2.Popen4(cmd)
       self._out = p.fromchild
 
+      if inputdata != None:
+        p.tochild.write(inputdata)
+        p.tochild.flush()
+
     self._process = p
     self.pid = p.pid
     
     self._thread = threading.Thread(target = lambda: self._run())
     self._thread.start()
 
   def _run(self):
     "Continues execution of this process on a separate thread."
     p = self._process
     out = self._out
 
     if IS_WIN32:
       running = lambda: p.poll() is None
+      if self._indata:
+        log.info("addin data: " + self._indata)
+        p.communicate(self._indata)
     else:
       running = lambda: p.poll() == -1
 
Attachment #322087 - Flags: review?(ted.mielczarek)
Attachment #322087 - Flags: review?(kengert)
Attachment #322087 - Flags: review?(jwalden+bmo)
Comment on attachment 322087 [details] [diff] [review]
Integration of ssltunnel to mochitest, 2

This patch lists the contents of binary file build/pgo/certs/pgoca.p12 with garbage characters.

I think you must tell cvs that you are adding a binary file.

Instead of "cvs add file" use "cvs add -kb file".
You should probably be using hg by now and working with mozilla-central; git-style diffs will properly record binary files and changes to them, as I understand it.
Depends on: 435558
Comment on attachment 321439 [details] [diff] [review]
HTTP server host-checking, with tests

+        var av = 0;
+        try
+        {
+          var av = bis.available();


remove the var the second time :)

r=biesi on the head_utils.js changes
Attachment #321439 - Flags: review?(cbiesinger) → review+
Attachment #321439 - Attachment is obsolete: true
Attachment #322393 - Flags: review?(ted.mielczarek)
Attachment #322393 - Flags: review?(sayrer)
Attachment #321439 - Flags: review?(ted.mielczarek)
Attachment #321439 - Flags: review?(sayrer)
Comment on attachment 322087 [details] [diff] [review]
Integration of ssltunnel to mochitest, 2

>diff -u10dpN build/pgo/Makefile.in build/pgo/Makefile.in 

>+genpgocert.py: genpgocert.py.in
>+	$(PYTHON) $(topsrcdir)/config/Preprocessor.py \
>+	$(AUTOMATION_PPARGS) $(DEFINES) $(ACDEFINES) $^ > $@
>+

Hm, we're starting to get up there in the number of preprocessed files here.  Maybe worth doing a wildcard target or something here, eventually -- up to you on this.


>diff -u10dpN build/pgo/automation.py.in build/pgo/automation.py.in 

>+  def __init__(self, command, args, env, inputdata = None):
>+      import tempfile

>+      if inputdata != None:

Just |if inputdata:|, please.

I'm still unconvinced you have to use a tempfile here, but I don't have time to suggest a better method that doesn't require rejiggering _run.


>   def kill(self):
>     "Kills this process."
>     try:
>-      if not IS_WIN32: # XXX
>+      if not IS_WIN32:
>         os.kill(self._process.pid, signal.SIGKILL)
>+      else:
>+        import subprocess
>+        pid = "%i" % self.pid
>+        subprocess.Popen(["taskkill", "/F", "/PID", pid])

You need to call .wait() on this last to have the same semantics as os.kill, I think.



>+def readLocations(locationsPath = "server-locations.txt"):
>   """
>   Reads the locations at which the Mochitest HTTP server is available from
>   server-locations.txt.
>   """
> 
>-  locationFile = codecs.open("server-locations.txt", "r", "UTF-8")
>+  locationFile = codecs.open(locationsPath, "r", "UTF-8")

XXX

>-                      r"(?P<options>\w+(?:,\w+)*)"
>+                      r"(?P<options>(?:\w+|\w+=\w+)(?:,(?:\w+|\w+=\w+))*)"

Just make this |(?P<options>\S+)|, if we're extending with key-value pairs I'd rather go overboard just to accommodate =, I think.


>@@ -315,43 +337,106 @@ function FindProxyForURL(url, host)
>   if (isHttp)
>     return 'PROXY localhost:8888';
>-  return 'DIRECT';
>+  else
>+    return 'PROXY localhost:4443';

Else after return is pointless; just do the return directly, as before but with the new value.



>+def fillCertificateDB(profileDir):
>+
>+  def runUtil(util, args, inputdata = None):
>+    proc = Process(util, args, None, inputdata)
>+    return proc.wait();

Don't bother with the intermediate, just return Process(...).wait() (and get rid of the semicolon).


>diff -u10dpN build/pgo/genpgocert.py.in build/pgo/genpgocert.py.in 

>+#expand DIST_BIN = "./" + __XPC_BIN_PATH__
>+#expand BIN_SUFFIX = __BIN_SUFFIX__
>+#expand CERTS_DIR = __CERTS_DIR__
>+#expand CERTS_SRC_DIR = __CERTS_SRC_DIR__
>+
>+import sys
>+import os
>+import re
>+import automation

Imports go first, and please alphabetize them.


>+dbFiles = ["cert8.db", "key3.db", "secmod.db"]
>+def dbPaths(dir):

Need a newline between these.


>+sys.exit(1)
>\ No newline at end of file

Add a newline, please.

I didn't read all that much of this file, to be honest; I don't know the commandline arguments, so I'll defer to others with more NSS chops.


>diff -u10dpN build/pgo/server-locations.txt build/pgo/server-locations.txt 

> # Unrecognized options are ignored.  Recognized options are "primary" and
>-# "privileged".  "primary" denotes a location which is the canonical location of
>+# "privileged", "nocert", "cert=some_cert_nickname".  

"primary", "privileged", "nocert", and "cert=nickname".

>+# "nocert" makes sense only for https:// hosts and means there is not
>+# any certificate automatically generated for this host.

"means no certificate is generated for this host"


>+# "cert=nickname" tells the pgo server to use a particular certificate
>+# for this host. The certificate is referenced by its nickname that must
>+# not contain any spaces. The certificate + key files (PKCS12 modules)

"nickname, which must not contain spaces."


>+# for custom certification are loaded from build/pgo/ssltunnel/certs

"from the"


>+# directory. When new certificate is added to this dir pgo/ssltunnel
>+# must be builded then.

"When a new certificate is added to this directory, build/pgo/ssltunnel must be rebuilt."


>diff -u10dpN testing/mochitest/server.js testing/mochitest/server.js 

>-               "(\\w+(?:,\\w+)*)" +
>+               "((?:\\w+|\\w+=\\w+)(?:,(?:\\w+|\\w+=\\w+))*)" +

Same here re \S+.


>diff -u10dpN testing/mochitest/ssltunnel/ssltunnel.cpp testing/mochitest/ssltunnel/ssltunnel.cpp 

>+    struct relay
>+    {
>+      char *buffer, *bufferhead, *buffertail, *bufferend;
>+      relay() { bufferhead = buffertail = buffer = new char[BUF_SIZE]; bufferend = buffer + BUF_SIZE; }
>+      ~relay() { delete [] buffer; }
>+
>+      bool empty() { return bufferhead == buffertail; }
>+      PRInt32 free() { return bufferend - buffertail; }
>+      PRInt32 present() { return buffertail - bufferhead; }
>+      void compact() { if (buffertail == bufferhead) {buffertail = bufferhead = buffer;} }

Put statements on separate lines here, and don't put any on the same line as the method name.

>+        PRInt32 s2 = s == 1 ? 0 : 1;
>+        PRInt16 out_flags = sockets[s].out_flags, &in_flags = sockets[s].in_flags, &in_flags2 = sockets[s2].in_flags;

Don't mix up references and values here; use separate lines.


I didn't read the NSS-y and ssltunnel.cpp stuff very closely but it looks more or less fine.  If you could, please wait so I can land my patch (hopefully to be reviewed soon) before you land yours -- I'm not going to be around this week, but I should be back by Saturday or so, and hopefully I can land a mess of patches at that time.  Stupid closed tree...
Attachment #322087 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 322393 [details] [diff] [review]
With that change, plus some default-port tests

diff --git a/build/pgo/Makefile.in b/build/pgo/Makefile.in

I think we ought to move automation.py, and related things (except profileserver.py) up one level to build/. Should be easy to do in hg.

diff --git a/build/pgo/automation.py.in b/build/pgo/automation.py.in

+  # Perhaps more detail than necessary, but it's the easiest way to make sure
+  # we get exactly the format we want.
+  lineRe = re.compile(r"^(?P<scheme>[a-z][-a-z0-9+.]*)"

Could you make a note that this format is documented in server-locations.txt?

+  # Grant God-power to all the privileged servers on which tests run.

This is awesome, much better than before!

diff --git a/testing/mochitest/server.js b/testing/mochitest/server.js
+function processLocations(server)

It's a little sadmaking that you wrote the same server-locations.txt processing code in both Python and JavaScript, but passing that data between the two is probably a pain in the ass.

r=me
Attachment #322393 - Flags: review?(ted.mielczarek) → review+
(In reply to comment #50)
> >+def readLocations(locationsPath = "server-locations.txt"):
> >   """
> >   Reads the locations at which the Mochitest HTTP server is available from
> >   server-locations.txt.
> >   """
> > 
> >-  locationFile = codecs.open("server-locations.txt", "r", "UTF-8")
> >+  locationFile = codecs.open(locationsPath, "r", "UTF-8")
> 
> XXX

Hrm, epic fail me.  I don't think it's a good idea for the argument to be optional here; it should be passed in explicitly in each case.
(In reply to comment #51)
> I think we ought to move automation.py, and related things (except
> profileserver.py) up one level to build/. Should be easy to do in hg.

Yeah; I'll do this in a followup change if I hopefully have time this week -- I'd prefer to leave such changes to not conflate the changes in history.  It's gonna be a race to see whether I actually can end up doing that, tho -- if I don't have it done and committed by next Thursday, it probably won't happen.


> It's a little sadmaking that you wrote the same server-locations.txt processing
> code in both Python and JavaScript, but passing that data between the two is
> probably a pain in the ass.

Yeah, this was probably the simplest way to address the problem.  I'm not too worried about it, since this won't change much anyway, and any differences that might manifest are unlikely to have deleterious side effects anyway (unless it's a complete failure on one side or the other).
Jeff, I already have a patch addressing your comments, but I will wait with submit and further reviews until you land your patch. There is already a new test for bug 435558 based on features of this bug.
Comment on attachment 322393 [details] [diff] [review]
With that change, plus some default-port tests

I'm not wild about the custom syntax thing for the servers, seems like CSV would be fine. But this is r+ either way. 

The wiki page should link into MXR and not duplicate the list of domains, so that comment should go. 

Neat stuff.
Attachment #322393 - Flags: review?(sayrer) → review+
I landed the server patch with the requested changes, and the MDC page now links to the file in the tree.  The rest of the work's good to go any time; do note I won't be able to look over anything else unless a delay of several weeks and potentially longer is acceptable.
Comment on attachment 322087 [details] [diff] [review]
Integration of ssltunnel to mochitest, 2

clearing review requests on old patch
Attachment #322087 - Flags: review?(ted.mielczarek)
Attachment #322087 - Flags: review?(kaie)
Kai, please take a look at changes of security building.
Attachment #322087 - Attachment is obsolete: true
Attachment #325400 - Flags: review?(kaie)
Comment on attachment 325400 [details] [diff] [review]
Integration of ssltunnel to mochitest, final

r=kaie on the security/manager changes
Attachment #325400 - Flags: review?(kaie) → review+
Attached patch Fix for leaktest.py (obsolete) — Splinter Review
This adds definitions for automation.py.in also to build/Makefile.in (overlooked by me previously).
Blocks: 443017
We need new versions of the patch.
I've been working with Honza by email, and we still have failures on Windows.

Right now we struggle with the fact that we get an interactive prompt for the master password database.

I think this is because of the hardcoded database filenames like "key3.db" and "cert8.db" in the patch. We should not do that. I'm running environment variables that will use newer file versions key4.db and cert9.db.

I think we should use wildcards, at least something like key*.db and cert*.db
I've changed the patch to include the new filenames key4.db and cert9.db, I propose we get this landed first, and change it to wildcards in an incremental patch.
I tried to land again, but it failed again.
Failure to find an NSS .so

=== Output ===
python leaktest.py -- -register
 in dir /builds/moz2_slave/mozilla-central-linux-debug/build/obj-firefox/_leaktest (timeout 1200 secs)
 watching logfiles {}
 argv: ['python', 'leaktest.py', '--', '-register']
 environment:
  CVS_RSH=ssh
  DISPLAY=:0
  G_BROKEN_FILENAMES=1
  HISTSIZE=1000
  HOME=/home/cltbld
  HOSTNAME=moz2-linux-slave05.build.mozilla.org
  INPUTRC=/etc/inputrc
  JAVA_HOME=/builds/jdk
  LANG=en_US.UTF-8
  LD_LIBRARY_PATH=obj-firefox/dist/bin
  LESSOPEN=|/usr/bin/lesspipe.sh %s
  LOGNAME=cltbld
  LS_COLORS=no=00:fi=00:di=01;34:ln=01;36:pi=40;33:so=01;35:bd=40;33;01:cd=40;33;01:or=01;05;37;41:mi=01;05;37;41:ex=01;32:*.cmd=01;32:*.exe=01;32:*.com=01;32:*.btm=01;32:*.bat=01;32:*.sh=01;32:*.csh=01;32:*.tar=01;31:*.tgz=01;31:*.arj=01;31:*.taz=01;31:*.lzh=01;31:*.zip=01;31:*.z=01;31:*.Z=01;31:*.gz=01;31:*.bz2=01;31:*.bz=01;31:*.tz=01;31:*.rpm=01;31:*.cpio=01;31:*.jpg=01;35:*.gif=01;35:*.bmp=01;35:*.xbm=01;35:*.xpm=01;35:*.png=01;35:*.tif=01;35:
  MAIL=/var/spool/mail/cltbld
  MOZ_CRASHREPORTER_NO_REPORT=1
  MOZ_OBJDIR=obj-firefox
  PATH=/opt/local/bin:/tools/buildbot/bin:/tools/twisted/bin:/tools/twisted-core/bin:/tools/python/bin:/tools/python/bin:/usr/kerberos/bin:/usr/local/bin:/bin:/usr/bin:/home/cltbld/bin
  PWD=/home/cltbld
  PYTHONHOME=/tools/python
  PYTHONPATH=/tools/buildbotcustom:/tools/buildbot/lib/python2.5/site-packages:/tools/twisted/lib/python2.5/site-packages:/tools/twisted-core/lib/python2.5/site-packages:/tools/zope-interface/lib/python2.5/site-packages/
  SHELL=/bin/bash
  SHLVL=1
  SSH_ASKPASS=/usr/libexec/openssh/gnome-ssh-askpass
  SSH_CLIENT=10.2.72.11 58291 22
  SSH_CONNECTION=10.2.72.11 58291 10.2.71.176 22
  SSH_TTY=/dev/pts/0
  TBOX_CLIENT_CVS_DIR=/builds/tinderbox/mozilla/tools
  TERM=xterm-color
  USER=cltbld
  XPCOM_DEBUG_BREAK=stack-and-abort
  _=/tools/buildbot/bin/buildbot
 closing stdin
 using PTY: True
/builds/moz2_slave/mozilla-central-linux-debug/build/obj-firefox/dist/bin/certutil: error while loading shared libraries: libnssutil3.so: cannot open shared object file: No such file or directory
ERROR FAIL Certificate integration
program finished with exit code 0
LD_LIBRARY_PATH=obj-firefox/dist/bin

That should be an absolute path, not a relative path
Only one Linux machine had failed, others succeeded.

I don't understand why we ended up with a relative path, the code attempts to produce an absolute path.

The makefile calls the genpgocert.py script with a path:
./Makefile.in:  $(PYTHON) $(DEPTH)/_profile/pgo/genpgocert.py --gen-server

This means a path should be available in argv[0].

The python script uses:
SCRIPT_DIR = os.path.abspath(os.path.realpath(os.path.dirname(sys.argv[0])))
myEnvironment["LD_LIBRARY_PATH"] = os.path.join(SCRIPT_DIR, automation.DIST_BIN)

Why is this not sufficient to produce an absolute path on machine "Linux mozilla-central leak test build"?
(In reply to comment #65)
> Only one Linux machine had failed, others succeeded.

Looks like the green build cycle really had failed, too:

/tools/python/bin/python ../../_profile/pgo/genpgocert.py --gen-server
/bin/sh: /builds/moz2_slave/mozilla-central-linux/build/obj-firefox/dist/bin/certutil: No such file or directory
ERROR FAIL: SSL Server Certificate generation


In addition, a Windows machine turned orange:
WINNT 5.2 mozilla-central qm-win2k3-03 dep unit test

The certutil execution worked, instead this machine complained about leaks
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1218865945.1218869280.13189.gz&fulltext=1
Blocks: 413909
Blocks: 452401
Ready to land.
Assignee: jwalden+bmo → honzab
Attachment #325400 - Attachment is obsolete: true
Attachment #325528 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #337039 - Flags: review+
Landed (again):
http://hg.mozilla.org/mozilla-central/rev/70dd0e9f14c2
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
This didn't fix _all_ build problems it seems, as now SeaMonkey and Thunderbird builds are broken:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1220630798.1220632051.30279.gz#err0
certutil.exe could not be found. It is probably because of the mozilla sub tree inside of mozilla-comm checkout. I will do with it something quickly.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed a bustage fix:
http://hg.mozilla.org/mozilla-central/rev/4ffc02f54b7f
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Depends on: 453909
+++ b/testing/mochitest/Makefile.in
+libs::
+	$(PYTHON) $(DEPTH)/_profile/pgo/genpgocert.py --gen-server

This completely breaks:

1) Cross-compile builds -- genpgocert.py would try to execute a foreign-platform binary

2) scratchbox semi-native-compile-builds -- the foreign-platform nss binaries are executed under emulation and either take way too long or break

--disable-tests might get around this (not sure, haven't tried yet), but that means that it's not possible to build tests for executing on a remote target.  There really needs to be a way to disable this that doesn't involve disabling all tests.
Can we wind up building a host version of the NSS utils, or is that too complicated? This is going to be tough to selectively disable, as we're going to start having mochitests that rely on it.
(In reply to comment #73)
> Can we wind up building a host version of the NSS utils, or is that too
> complicated? This is going to be tough to selectively disable, as we're going
> to start having mochitests that rely on it.

In theory we could, but our build isn't at all set up to build a separate host version.  Since we're just generating certs, can we fall back to using openssl to generate if we're cross compiling?  I think we can require that a host openssl be installed and available (the binary) if cross-compiling.
Can the openssl tools generate the certs in the format NSS needs?
Alternately, we could just provide a way to point to a pre-compiled host binary for certutil. Would that work for you?
We should avoid a situation where we have to maintain two independent sets of scripts for the SSL tests, one using NSS, another one using something else. If you can't use NSS (yet), then let's fix that.

Can the build scripts distinguish between native and cross-compile build?

If it's a cross-compile, we could require that NSS tools are installed on the host system and use those.
Several Linux flavours already install the NSS tools, and they should be straightforward to build.
(In reply to comment #75)
> Can the openssl tools generate the certs in the format NSS needs?

No. certutil and pk12util (when importing) are working with NSS certificate database that is not compatible with openssl certificate storage. This database format is needed for ssltunnel because ssltunnel is built upon NSS code, naturally.

(In reply to comment #76)
> Alternately, we could just provide a way to point to a pre-compiled host binary
> for certutil. Would that work for you?

It is IMHO much better solution then trying to bridge or hack other way using openssl for cert generation.


Other way is to do the cert generation completely at run-time and not at compile-time as original version of the patch did. I am not quit sure why exactly we decided to do it that way. Only 'problem' I see is that the server certificate will be re-generated every time the mochitest is to be run.
Depends on: 454881
Blocks: 455168
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.
Component: Testing → Mochitest
Product: Core → Testing
QA Contact: testing → mochitest
Version: Trunk → unspecified
Blocks: 471129
Whiteboard: [fixed in 1.9.1b1]
Version: unspecified → Trunk
Depends on: 472706
Depends on: 480077
You need to log in before you can comment on or make changes to this bug.