456001 - Need automated testing for SSL client auth

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Kai Engert (:KaiE:)]

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).

Comment 1

[Tony Chung [:tchung]]

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.

Comment 2

[Marcia Knous [:marcia]]

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.

Comment 3

[Honza Bambas (:mayhemer)]

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.

Comment 4

[Kai Engert (:KaiE:)]

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.)

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: mochitest ssl client auth capability, v1

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).

Comment 6

[Bjarne (:bjarne)]

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

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: mochitest ssl client auth capability, v1.1

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

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: mochitest ssl client auth capability, v2

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.

Comment 9

[Honza Bambas (:mayhemer)]

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

Comment 10

[Bjarne (:bjarne)]

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.

Comment 11

[Honza Bambas (:mayhemer)]

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?

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: mochitest ssl client auth capability, v2.1

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

Comment 13

[Honza Bambas (:mayhemer)]

Attachment: mochitest ssl client auth capability, v2.2

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).

Comment 14

[(not currently active) Ted Mielczarek]

(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 15

[Honza Bambas (:mayhemer)]

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 16

[Jeff Walden [:Waldo]]

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...

Comment 17

[Honza Bambas (:mayhemer)]

Attachment: mochitest ssl client auth capability, v2.3

Ready to land patch.

Comment 18

[Honza Bambas (:mayhemer)]

Attachment: new ssltunnel comment

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 19

[Jeff Walden [:Waldo]]

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".

Comment 20

[Honza Bambas (:mayhemer)]

Attachment: new ssltunnel help text, v2

Hope my english is better in this patch.

Comment 21

[Jeff Walden [:Waldo]]

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 23

[timeless]

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"

Comment 24

[Honza Bambas (:mayhemer)]

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

Comment 25

[Bjarne (:bjarne)]

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.)

Comment 26

[Honza Bambas (:mayhemer)]

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 27

[Bjarne (:bjarne)]

(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).

Comment 28

[Honza Bambas (:mayhemer)]

Attachment: v2.4 [Checkin: Comment 30]

- 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 29

[Jeff Walden [:Waldo]]

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 30

[Honza Bambas (:mayhemer)]

Comment on attachment 356957 [details] [diff] [review]
v2.4
[Checkin: Comment 30]

Re-landed as http://hg.mozilla.org/mozilla-central/rev/90d1568a80b5.

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 32

[Honza Bambas (:mayhemer)]

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.

Comment 33

[Dave Townsend [:mossop]]

(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

Comment 34

[Honza Bambas (:mayhemer)]

Attachment: v2.4 for 1.9.1 branch

Comment 35

[Honza Bambas (:mayhemer)]

Attachment: v2.4 + certs for 1.9.1 branch [Checkin: Comment 36]

I forgot to re-add the client certs.

Comment 36

[Honza Bambas (:mayhemer)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b9cb0aebada0

Comment 37

[Honza Bambas (:mayhemer)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b9cb0aebada0

Comment 38

[juan becerra [:juanb]]

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.

Comment 39

[Honza Bambas (:mayhemer)]

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

Comment 40

[juan becerra [:juanb]]

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.

Comment 41

[Kai Engert (:KaiE:)]

reassign bug owner.
mass-update-kaie-20120918