Improve support for requesting client certs in tlslite

Improve support for requesting client certs in tlslite

Currently, tlslite only supports the caller passing in a list of CAs pre-encoded for the TLS CertificateRequest message. This CL improves that, by providing a means of extracting the DER-encoded subject name from an X509 certificate, supplying a list of such names to tlslite's server routines, and having tlslite encode the list of CAs as part of the CertificateRequest.

BUG=47656, 47658
TEST=net_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=56982

[Ryan Sleevi, 2010-08-14 22:24:45]

Python is not a primary or secondary programming language for me, so given that,
I've done my best in following
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html but fully
acknowledge that I may have misinterpreted/misunderstood things - particularly
on the python syntax for documentation.

Pawel, I've tagged you because I'm aware of your pending
http://codereview.chromium.org/3080029/show . I've specifically avoided exposing
these options via test_server.cc/.h for that reason. My concern with that patch,
and how it intersects with this patch, is that it appears you've eschewed
offering as many options, instead using type (eg: TYPE_HTTPS_CLIENT_AUTH).

With this patch, the types of tests that will be run will involve altering the
arguments passed to testserver.py for each test, so I don't know if it would be
appropriate to balloon Type, which would be unfortunate, expose constructor
parameters, which may also be unfortunate, or to expose mutator methods, which
your patch appears to avoid.

To make it a bit more obvious, here's the parameters that would be passed to
testserver.py, for the following chains:
Client certificate chain: C->A->B
Server certificate chain: S->A->B
Unrelated certificate: X

Command line parameters:
--https=S --ssl-client-auth
--https=S --ssl-client-auth --ssl-client-ca=A
--https=S --ssl-client-auth --ssl-client-ca=B
--https=S --ssl-client-auth --ssl-client-ca=A --ssl-client-ca=B
--https=S --ssl-client-auth --ssl-client-ca=B --ssl-client-ca=A
--https=S --ssl-client-auth --ssl-client-ca=X
--https=[SA] --ssl-client-auth
--https=[SA] --ssl-client-auth --ssl-client-ca=A
--https=[SA] --ssl-client-auth --ssl-client-ca=B
--https=[SA] --ssl-client-auth --ssl-client-ca=A --ssl-client-ca=B
--https=[SA] --ssl-client-auth --ssl-client-ca=B --ssl-client-ca=A
--https=[SA] --ssl-client-auth --ssl-client-ca=X
--https=[SAB] --ssl-client-auth
--https=[SAB] --ssl-client-auth --ssl-client-ca=A
--https=[SAB] --ssl-client-auth --ssl-client-ca=B
--https=[SAB] --ssl-client-auth --ssl-client-ca=A --ssl-client-ca=B
--https=[SAB] --ssl-client-auth --ssl-client-ca=B --ssl-client-ca=A
--https=[SAB] --ssl-client-auth --ssl-client-ca=X

You can see that the parameters changing here are what file is being passed to
https (to specify a single certificate or chain, which is not yet patched in to
testserver.py) and to pass multiple instances of the accept client CAs.

[Paweł Hajdan Jr., 2010-08-16 18:09:33]

Removing myself from reviewers. Looks fine at a glance.

Please let me know if you need/want to modify test_server and/or its API.

[wtc, 2010-08-17 23:31:10]

rsleevi: thanks for the CL.  I have one suggestion below.

davidben: could you review this CL?  You looked at the client auth code in
TLS Lite recently, and I'm swamped by code reviews.  Thanks.

http://codereview.chromium.org/3177015/diff/1/3
File third_party/tlslite/README.chromium (right):

http://codereview.chromium.org/3177015/diff/1/3#newcode6
third_party/tlslite/README.chromium:6: - close_notify.patch:
tlslite/TLSRecordLayer.py was changed to force the
Can you take the opportunity to create a "patches" directory and move the
patches there?

I see only chromium.patch in this directory.  close_notify.patch and
python26.patch are nowhere to be found :-(

[Ryan Sleevi, 2010-08-18 00:12:00]

http://codereview.chromium.org/3177015/diff/1/3
File third_party/tlslite/README.chromium (right):

http://codereview.chromium.org/3177015/diff/1/3#newcode6
third_party/tlslite/README.chromium:6: - close_notify.patch:
tlslite/TLSRecordLayer.py was changed to force the
On 2010/08/17 23:31:10, wtc wrote:
> Can you take the opportunity to create a "patches" directory and move the
> patches there?
> 
> I see only chromium.patch in this directory.  close_notify.patch and
> python26.patch are nowhere to be found :-(

Sure, I'll do that in http://codereview.chromium.org/3115011/show, which was the
trivial dependency (updating the docs and patches with the Chromium changes)

[davidben, 2010-08-18 00:25:59]

LGTM!

We should possibly also send this upstream. Wan-Teh, I believe you said you knew
the author?

The getChildBytes thing feels somewhat sketchy, but I don't see a cleaner way
around it; this ASN1Parser object is really stateful, so it wouldn't be very
suitable as a value type. (Otherwise, my first thought would have been to add a
bytes property to ASN1Parser and hold on to the wrappers.)

I suspect the reason the refactored test server has a limited number of options
is to ease retaining one test server for all the tests, which is going to cause
problems with the session cache for SSL anyway. (All our URLRequestContexts
currently share one session cache.)

http://codereview.chromium.org/3177015/diff/1/2
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/3177015/diff/1/2#newcode1229
net/tools/testserver/testserver.py:1229: print 'specified trusted Ca file not
found: ' + ca_cert + ' exiting...'
Nit: Should that say "CA" instead of "Ca"? Perhaps this and the above message
should explicitly specify "server" and "client" in there.

http://codereview.chromium.org/3177015/diff/1/5
File third_party/tlslite/tlslite/TLSConnection.py (right):

http://codereview.chromium.org/3177015/diff/1/5#newcode1001
third_party/tlslite/tlslite/TLSConnection.py:1001: @type reqCAs: L{list} of
L{array.array} of unsigned bytes
I'm not familiar with the documentation system tlslite is using, but I get the
impression links to list won't do much if it's not provided by the package? He
doesn't link to bool above. Eh, we can also just leave this for upstream to
figure out.

http://codereview.chromium.org/3177015/diff/1/5#newcode1057
third_party/tlslite/tlslite/TLSConnection.py:1057: raise ValueError("Caller
passed reqCAs but will not reqCert")
s/will //

http://codereview.chromium.org/3177015/diff/1/8
File third_party/tlslite/tlslite/utils/ASN1Parser.py (right):

http://codereview.chromium.org/3177015/diff/1/8#newcode34
third_party/tlslite/tlslite/utils/ASN1Parser.py:34: return p.bytes[markIndex :
p.index]
You could probably implement getChild in terms of getChildBytes here.

[wtc, 2010-08-18 00:28:26]

Yes, we should submit our patches to the TLS Lite author, Trevor Perrin.
His email address is in the readme.txt file.

[davidben, 2010-08-18 01:35:44]

So, I just talked with phajdan. As I understand it, the two primary reasons for
the API changes were:

1. Avoids the mutators and such because it's odd what happens if you start the
server and then set a parameter afterwards.

2. Control the number of types of servers so that, later, when we notice that
the HTTP server is started up many times, we can limit it to one instance to
speed things up.

Since most of the things to be tested SSL-side happen during the handshake and
before the GET request happens, the SSL tests are all going to have different
configurations. So, we can't easily share the servers. Also, if we have many
flags, passing them all into the constructor is, like you said, unfortunate.

We concluded that the best approach would probably be to, since we're likely
going to need many different test parameters, add a TestServerParams (or maybe
TestServerSSLParams or something?) class that would have all the usual mutators
and then pass that along along to a /second/ constructor of the TestServer.
Pawel says he intends to make the sharing occur at a higher level, so we don't
need to worry about accidentally making our servers long-lived.

So, I guess when this lands or so, one of us can try for a CL that lets us play
with parameters in this way.

[Ryan Sleevi, 2010-08-18 01:56:02]

Thanks David, good to know what the right way is going to be, since I saw his
patch has been committed.

I'm going to hold off on writing any client auth unit tests until the changes to
chain building/validation (for x-x509-user-cert) land, since right now there is
too much platform variety as to what will happen. For example NSS can use the
server's certificates for chain build, OS X/Win can't; OS X & NSS don't do
RFC3280 name canonicalization for client certificates; Windows doesn't remember
the intermediates; etc. And of course, to make sure the calls are mockable when
necessary for the bots.

If everything still looks good, I'll forward this upstream as well once I
commit.

http://codereview.chromium.org/3177015/diff/15001/16004
File third_party/tlslite/tlslite/TLSConnection.py (right):

http://codereview.chromium.org/3177015/diff/15001/16004#newcode1001
third_party/tlslite/tlslite/TLSConnection.py:1001: @type reqCAs: list of
L{array.array} of unsigned bytes
I changed to drop the L{} for list, since the author doesn't use L{} for str,
another built-in type (or bool, as you noted)