Bug 454406 has tought us that we absolutely must have some kind of automated testing for the SSL client authentication feature, involving various kinds of different certs (single and multi keys).

Absolutely agree. SSL client auth testing is a very weak area in our test environment at Mozilla, and we need to do much better at this. We are working on a server, yet to be named, but it would be a proxy server we'd be hosting onsite in mountain view. And to be clear, this would be different than the mochikit automation server we have now. Sorry i dont have more information for you right now, but lets keep dialoging here and i'll add a few others i know that are interested in this project.

Kai: Do you think it would be beneficial for us to set up a meeting with the person at Red Hat that does your QA? A while back when we met in the office this was suggested but it never happened.

Why exactly is there need to have a new server? We have now ssl server support in mochitest (the main automated testing tool at Mozilla). It is easy to enhance it to support client auth and write mochitests using it.

Marcia, I'm afraid things have changed and we no longer have the QA resources to help. Honza, I think it would be good to have something for interactive testing, too. A mochitest is good to automatically check whether it's still working or now failing, but there should be a way to do manual interactive testing, which also helps for debugging. In the past I've usually used certs I generated myself and used client auth against my own kuix.de server. However, this bug specifically asks for automated testing, so Honza might be right in his assessment that mochitest might be sufficient for fixing this bug. If there is a desire to have a permanently running server for interactive testing, does Mozilla already operate one that could be used? (I found it necessary to have multiple virtual SSL web server instances, in order to test different configurations.)

Because bug 466080 requires automated testing and verification I enhanced mochitest ssl support to optionally allow client authentication. There is a new host option supported in server-locations.txt now, see example: # turn on mandatory client authentication https://test1.example.com:443 privileged,clientauth=require # turn on optional client authentication https://test2.example.com:443 privileged,clientauth=request The client cert must be signed by CA that is marked as trusted CA in the ssltunnel's cert db (I changed trust flags to "CT,," on the "pgo testing ca" CA).

There is a comma after the last item in "enum client_auth_option" on which my gcc freaks out.

Oops, visual studio ignores this, removed comma from the enum.

Fixed problem when adding new https hosts - there must be mirror among http hosts because necko doesn't send complete scheme://host:port string in Host header - the server then looks up just the http list (bound to port 80). Added default client certificate signed by the testing PGO CA, i.e. it is trusted client certificate accepted by the server.

Err, problem was not 'fixed', was workarounded.

Nice! :) We're getting there... An observation though : After applying this patch I still need to 1) import the client-cert manually and 2) add exceptions to accept the server-cert. For fully automated testing this obviously must be avoided.

You probably forget to run following commands after apply of the patch: make -C obj-dir/build/pgo make -C obj-dir/build/pgo gensrvercert make -C obj-dir/testing/mochitest Also depends on the way you imported the patch. Did you add it to your patch queue or used hg import?

Fixed pk12util call for client certs - it doesn't accept redirected input, empty password passed as a parameter.

One more patch, fixing problem on windows with adding client cert. Also fixes adding the ca cert (I didn't wait for the certutil and pk12util to finish).

(In reply to comment #8) > Fixed problem when adding new https hosts - there must be mirror among http > hosts because necko doesn't send complete scheme://host:port string in Host > header - the server then looks up just the http list (bound to port 80). Can you file a new bug on this and CC Waldo? We should make the ssltunnel/httpd.js integration smarter in this case.

Comment on attachment 350531 [details] [diff] [review] mochitest ssl client auth capability, v2.2 Jeff, could you take a look at this please? It seems Kai is too busy and there is actually not a majority of secure code changes.

Comment on attachment 350531 [details] [diff] [review] mochitest ssl client auth capability, v2.2 I'm hardly an SSL expert, so I didn't review the actual mechanics of what's happening at that level; I assume they're tested and correct. >diff --git a/build/pgo/automation.py.in b/build/pgo/automation.py.in >@@ -389,47 +390,56 @@ def fillCertificateDB(profileDir): > > # Create head of the ssltunnel configuration file > sslTunnelConfigPath = os.path.join(CERTS_DIR, "ssltunnel.cfg") > sslTunnelConfig = open(sslTunnelConfigPath, "w") > > sslTunnelConfig.write("httpproxy:1\n") > sslTunnelConfig.write("certdbdir:%s\n" % CERTS_DIR) > sslTunnelConfig.write("forward:127.0.0.1:8888\n") > sslTunnelConfig.write("listen:*:4443:pgo server certificate\n") > >- # Generate automatic certificate and bond custom certificates >+ # Config automatic certificate and bond custom certificates, client authentication "Configure" instead of "Config" and "bind" rather than "bond", I think. >diff --git a/build/pgo/certs/Makefile.in b/build/pgo/certs/Makefile.in > # Extension of files must be '.ca' > _CERT_AUTHORITIES = \ > pgoca.ca \ > $(NULL) > >+_CLIENT_CERTS = \ >+ mochitest.client \ >+ $(NULL) Probably add a note similar to that above _CERT_AUTHORITIES here. >diff --git a/testing/mochitest/ssltunnel/ssltunnel.cpp b/testing/mochitest/ssltunnel/ssltunnel.cpp >+PR_CALLBACK PRIntn ClientAuthValueComparator(const void *v1, const void *v2) >+{ >+ int a = *(client_auth_option*)v1 - *(client_auth_option*)v2; static_cast<>s here. >+ if (!a) a == 0 is clearer, I think. >+ return 0; >+ if (a > 0) >+ return 1; >+ else >+ return -1; Last should be an if, like you did for a > 0 case. >+ *clientauth = *(client_auth_option*)c; static_cast<> > SSL_OptionSet(ssl_socket, SSL_SECURITY, PR_TRUE); > SSL_OptionSet(ssl_socket, SSL_HANDSHAKE_AS_CLIENT, PR_FALSE); > SSL_OptionSet(ssl_socket, SSL_HANDSHAKE_AS_SERVER, PR_TRUE); >+ switch (clientAuth) >+ { >+ case caRequire: >+ case caRequest: >+ SSL_OptionSet(ssl_socket, SSL_REQUEST_CERTIFICATE, PR_TRUE); >+ SSL_OptionSet(ssl_socket, SSL_REQUIRE_CERTIFICATE, clientAuth == caRequire); >+ case caNone: >+ break; >+ } This seems better as: if (clientAuth != caNone) { SSL_OptionSet(ssl_socket, SSL_REQUEST_CERTIFICATE, PR_TRUE); SSL_OptionSet(ssl_socket, SSL_REQUIRE_CERTIFICATE, clientAuth == caRequire); } Does SSL make any further clientAuth options likely to be implemented in the future? I don't have any idea, but above seems fine if we just have these three. >+ client_auth_option* authoption = new client_auth_option; Missing OOM check for this. >+ char *hostname_copy = new char[strlen(hostname)+strlen(hostportstring)+2]; And here. >+ PL_HashTableAdd(existingServer->host_clientauth_table, hostname_copy, authoption); And maybe here? Seems generally fine enough to not need another iteration of review from me...

Ready to land patch.

I forgot one thing: modify the ssltunnel comment to reflect the new option. I will fold the full patch with this one and land it as one patch.

Comment on attachment 355577 [details] [diff] [review] new ssltunnel comment >diff --git a/testing/mochitest/ssltunnel/ssltunnel.cpp b/testing/mochitest/ssltunnel/ssltunnel.cpp >+ " # This only works in httpproxy mode and has higher priority\n" >+ " # then the previous option.\n" "than" >+ " listen:my.host.name:443:4443:a different cert\n\n" >+ " # To make a specific host require or just request a client certificate\n" >+ " # to authenticate use following options. It can only be used\n" "This can only" >+ " # in httpproxy mode and after 'listen' option has been specified.\n" "after the" >+ " # Refer a port number of the real server too (4443 in the example).\n" >+ " clientauth:requesting-client-cert.host.com:443:4443:request\n" >+ " clientauth:requiring-client-cert.host.com:443:4443:require\n", I don't understand this; do you mean "Requires" instead of "Refer"? As far as I can see it the line specifies the hostname:port, the port to which the tunnel is opened, and what part a client certificate plays in the SSL connection process, but I'm not sure how to reconcile that with "Refer".

Hope my english is better in this patch.

Comment on attachment 355621 [details] [diff] [review] new ssltunnel help text, v2 I kind of feel like the whole of the help text isn't quite the greatest, but on the other hand, it's pretty rare for anyone to be running ssltunnel by hand anyway or modifying the current .cfg all that often, so this is fine with me.

Comment on attachment 355621 [details] [diff] [review] new ssltunnel help text, v2 + " # To make a specific host require or just request a client certificate\n" + " # to authenticate use following options. This can only be used\n" to authenticate<comma> use [the] following options..... + " # in httpproxy mode and after the 'listen' option has been specified.\n" in httpproxy mode and [only] after ... + " # You also have to specify the tunnel listen port.\n"

Landed as http://hg.mozilla.org/mozilla-central/rev/4951644cab1d Leaving this bug open, there is probably more work to do here.

Hmm... I applied this patch today and the server-cert for hosts "requireclientcert.example.com" and "requestclientcert.example.com" was not trusted by default. After adding exception in the browser it works nicely (looks like the same issue as item 2 in comment #10). (I did a full rebuild after applying the patch btw.)

Bjarne, it has been backed out because of leaks. In bug 441359 was discovered that adding new hosts with privileged option rise memory leaks over the threshold. We should just rise the threshold and reland.

(In reply to comment #26) > Bjarne, it has been backed out because of leaks. > > In bug 441359 was discovered that adding new hosts with privileged option rise > memory leaks over the threshold. We should just rise the threshold and reland. Comment #25 was just an observation / fyi. It happened when I applied your patch to my local tree in order to test some things for bug #466080, like we did last time. The two new hosts work nicely after adding exceptions for their certificates. No idea why - the patch was applied to a clean tree and I did a full recompile (make clean + make build).

- raise the leak threshold to 74500 bytes (according to WINNT logs) - address comments to help text - doesn't contain binary files (cert db and the client cert) because those were left in the repo after backout (hg log -p -r XXX | patch -Rp1 doesn't work for binary files)

Comment on attachment 356957 [details] [diff] [review] v2.4 [Checkin: Comment 30] File a followup to fix the new leak, please, and CC me? If you can figure out which test leaks in that situation that'd be good too.

Comment on attachment 356957 [details] [diff] [review] v2.4 [Checkin: Comment 30] Re-landed as http://hg.mozilla.org/mozilla-central/rev/90d1568a80b5.

Is this FIXED? It's blocking bug 466080 which is a 3.1 blocker.

The patch has landed and one part of this bug has been fixed. I left it open because the original purpose was wider. See comment 1. Feel free to land other bugs pending on this one.

(In reply to comment #29) > (From update of attachment 356957 [details] [diff] [review]) > File a followup to fix the new leak, please, and CC me? If you can figure out > which test leaks in that situation that'd be good too. That follow-up is bug 473802

I forgot to re-add the client certs.

We are setting up a certificates server to improve the black box coverage of this area. There's a vm setup for this, and in the next couple of weeks we should have it set up, so we can have a server and dummy certs to test the browser UI with.

Juan, just for curiosity, do you plan to create more wide testing environment than one that could be supported by the ssltunnel proxy? https://developer.mozilla.org/En/Modifying_Mochitest_SSL_behavior

Honza: the original plan was to set up a certificates server according to http://pki.fedoraproject.org/wiki/PKI_Main_Page and go from there.

reassign bug owner. mass-update-kaie-20120918