Support restriction the TLS cipher selection in test_server.py

Add support to test_server.py to restrict the SSL/TLS bulk encryption algorithms via the command-line argument --ssl-alg. 

BUG=58831
TEST=Run test_server.py as an HTTPS server with --ssl-alg=rc4. Connect via openssl s_client -connect 127.0.0.1:1337 -cipher DEFAULT:\!RC4. Observe a connection failure. Connect with openssl s_client -connect 127.0.0.1:1337, observe that a  ciphersuite that uses RC4 is negotiated.

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

[Ryan Sleevi, 2010-10-15 07:31:43]

Pawel:

   While this change to test_server.py itself is quite minor, I'd like to also
expose this setting via test_server.cc so that it can be used in unit tests. My
intent was to do it as part of this CL, although I'll happily spin up another if
you'd prefer to just keep this side localized to the Python changes.

   Before I go much further, I wanted to confirm that your sentiments as
conveyed by davidben on http://codereview.chromium.org/3177015/show are still
the same - that you'd like to see a set of options for test server
initialization, and then have those passed into a constructor, so that they
don't change during the server's operation.

   If so, is something like TestServerSSLParams (as proposed by davidben)
acceptable? Any other thoughts or guidance you can provide?

[Paweł Hajdan Jr., 2010-10-15 07:49:47]

That's right, setting up TestServer at the time of construction is strongly
preferred (I'm not sure about particular implementations of that).

Most tests probably don't use SSL, so please try to also make it easy and simple
to use for tests that never touch SSL.

Also, maybe it would be worth to merge TYPE_HTTPS* things to this new SSL
configuration thing.

Could you do all of the changes, including net/test/test_server changes in this
CL? This way we'll be able to see the entire result.

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

http://codereview.chromium.org/3812007/diff/1/2#newcode1250
net/tools/testserver/testserver.py:1250: option_parser.add_option('',
'--ssl-alg', action='append',
I'd prefer a non-abbreviated --ssl-algorithms. Also applies to variable names,
i.e. ssl_alg -> ssl_algorithms.

[wtc, 2010-10-15 18:36:07]

rsleevi: thanks for the patch.

Please ask me to review all SSL-related changes to test_server.py
because phajdan.jr doesn't work on SSL.

I just have a nit on option and parameter naming below.

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

http://codereview.chromium.org/3812007/diff/1/2#newcode67
net/tools/testserver/testserver.py:67: ssl_client_auth, ssl_client_cas,
ssl_algorithms):
The parameter name 'ssl_algorithms' is too vague.  It should
'ssl_ciphers' or 'ssl_bulk_ciphers'.

The 'ssl_ciphers' name comes from tlslite, which this is
ultimately passed to:
http://trevp.net/tlslite/docs/public/tlslite.HandshakeSettings.HandshakeSetti...

The 'ssl_bulk_ciphers' name comes from the TLS 1.0 spec:
    enum { null, rc4, rc2, des, 3des, des40 } BulkCipherAlgorithm;
which is only an approximation.

[wtc, 2010-10-15 18:38:20]

Just to clarify: what I meant is that phajdan.jr should
review all changes to test_server.py.  If the changes are
related to SSL, please have me as an additional reviewer.

[Ryan Sleevi, 2010-10-15 19:24:04]

On 2010/10/15 18:38:20, wtc wrote:
> Just to clarify: what I meant is that phajdan.jr should
> review all changes to test_server.py.  If the changes are
> related to SSL, please have me as an additional reviewer.

Will do. I wasn't planning on committing with-out your signoff + review. I just
wanted to be able to "show my work" / "show the problem" to phajdan.jr to make
sure the design solution was right, before proceeding too far with the
implementation.

[Ryan Sleevi, 2010-10-16 08:57:14]

PTAL - I've uploaded a new changeset with the changes to C++ TestServer. 

There are several things that I'm not thrilled about here, but in looking
through existing code, I didn't see any better solutions.

http://codereview.chromium.org/3812007/diff/7001/8005
File net/test/test_server.h (right):

http://codereview.chromium.org/3812007/diff/7001/8005#newcode52
net/test/test_server.h:52: };
I don't like that the horizontal cost of this is so great, but I didn't have any
bright ideas here.

net::TestServer::HTTPSOptions(net::TestServer::HTTPSOptions::CERTIFICATE_OK),
but I wasn't sure how much abbreviation flexibility there was here.

net::TestServer::HTTPSOptions(net::TestServer::HTTPSOptions::kCertOK) is
slightly better, but not by much (not to mention it violates the Style Guide
rule on naming - follow the pattern that is there)

net::TestServer::TLSOptions(net::TestServer::TLSOptions::kCertOK) gets closer,
but I wasn't sure if the "TLSOptions" muddied the water too much versus
"HTTPSOptions"

http://codereview.chromium.org/3812007/diff/7001/8007
File net/test/test_server_win.cc (right):

http://codereview.chromium.org/3812007/diff/7001/8007#newcode76
net/test/test_server_win.cc:76: bool AppendHTTPSFlags(const
TestServer::HTTPSOptions& options,
@phajdan.jr: This seems like it would be easier if base::CommandLine was being
used, at least, judging from the interface. Is there a reason that the two
implementations can't use it to build up the command to run (with Linux
prefixing in the call to python)? I'm not familiar enough with the code base to
know if there are limitations preventing that.

Just with the number of added flags, I'm concerned with two implementations that
are "almost" identical - copy pasta and all - getting out of sync.

[Paweł Hajdan Jr., 2010-10-16 09:17:36]

Some rather minor comments from me. I think this is the right direction for the
public interface of TestServer.

http://codereview.chromium.org/3812007/diff/7001/8004
File net/test/test_server.cc (right):

http://codereview.chromium.org/3812007/diff/7001/8004#newcode116
net/test/test_server.cc:116: : server_certificate(cert),
request_client_certificate(false),
nit: Follow the style guide for initializer lists (each parameter on its own
line).

http://codereview.chromium.org/3812007/diff/7001/8004#newcode135
net/test/test_server.cc:135: : https_options_(HTTPSOptions::CERTIFICATE_OK),
type_(type) {
This is weird. Can we make HTTPSOptions have a default ctor?

http://codereview.chromium.org/3812007/diff/7001/8005
File net/test/test_server.h (right):

http://codereview.chromium.org/3812007/diff/7001/8005#newcode73
net/test/test_server.h:73: ~HTTPSOptions();
nit: Is this needed?

http://codereview.chromium.org/3812007/diff/7001/8005#newcode95
net/test/test_server.h:95: FilePath GetCertificateFile() const;
nit: This should be before the member variables.

http://codereview.chromium.org/3812007/diff/7001/8005#newcode99
net/test/test_server.h:99: TestServer(Type type,
In this case we're going to assume TYPE_HTTPS, so let's remove Type from the
ctor's signature for simplicity. I'd then move HTTPSOptions to be the first
parameter, so that it's more similar to the ctor above.

http://codereview.chromium.org/3812007/diff/7001/8007
File net/test/test_server_win.cc (right):

http://codereview.chromium.org/3812007/diff/7001/8007#newcode20
net/test/test_server_win.cc:20: using net::TestServer;
nit: Add empty line above.

http://codereview.chromium.org/3812007/diff/7001/8007#newcode76
net/test/test_server_win.cc:76: bool AppendHTTPSFlags(const
TestServer::HTTPSOptions& options,
You're right. I think it's only a historical thing, and unification is a great
idea.

[Ryan Sleevi, 2010-10-16 11:24:17]

Thanks for the feedback @phajdan.jr. I've moved the python launching to use
CommandLine, rather than building the arguments, which has made things much
simpler and (I think) easier to understand/maintain.

I did have one question about your feedback below.

http://codereview.chromium.org/3812007/diff/7001/8004
File net/test/test_server.cc (right):

http://codereview.chromium.org/3812007/diff/7001/8004#newcode116
net/test/test_server.cc:116: : server_certificate(cert),
request_client_certificate(false),
On 2010/10/16 09:17:36, Paweł Hajdan Jr. wrote:
> nit: Follow the style guide for initializer lists (each parameter on its own
> line).

Ah, I misunderstood what the style guide was stating. I thought it was only
specifying that you should indent when wrapping multiple lines, not that every
item should be on it's own. Fixed.

http://codereview.chromium.org/3812007/diff/7001/8004#newcode135
net/test/test_server.cc:135: : https_options_(HTTPSOptions::CERTIFICATE_OK),
type_(type) {
On 2010/10/16 09:17:36, Paweł Hajdan Jr. wrote:
> This is weird. Can we make HTTPSOptions have a default ctor?

Right, this was one of the things that bothered me. It stems from the
SSLBrowserTests, which has TestServer as a member variable of the class. Because
TestServer is non-copyable, it has to be initialized in the initializer list,
which means it has to be possible to pass the certificate to use in the
initializer list.

The way I saw it, there were only a few options:
1) Change SSLUITest to use scoped_ptr<>s for the servers, rather than as
members. Every test in the class would need to be updated, but it simplify
things in the long term.
2) Provide two constructors - a default ctor and an explicit ctor
3) Move ServerCertificate enum to TestServer, and add it as a parameter for the
second ctor - TestServer::TestServer(ServerCertificate, document_root,
HTTPSOptions)
4) This approach

1 I liked, but it seems like the pattern of having a member test server without
having to declare it as a pointer type is both valid and useful. Why require a
pointer for HTTPS, but not (technically) for HTTP/FTP? Of course, this problem
still exists if you want to touch anything else - such as client authentication
or ciphers...

2 felt weird because I felt it wouldn't exactly be clear why there were two
constructors, other than to simply provide a default value, which seemed to be
frowned upon by the style guide. 

3 Felt like it muddied the waters even more than 2 - HTTPSOptions that only
cover /some/ HTTPSOptions.

That was the rationale behind the choice. The other aspect is that an HTTPS
server must always have a server certificate - it's the one option that is
mandatory. 


To make sure I understand - are you suggesting drop the explicit ctor entirely,
and just use server_certificate to set it - or were you suggesting add a default
ctor (#2)?

http://codereview.chromium.org/3812007/diff/7001/8005
File net/test/test_server.h (right):

http://codereview.chromium.org/3812007/diff/7001/8005#newcode73
net/test/test_server.h:73: ~HTTPSOptions();
On 2010/10/16 09:17:36, Paweł Hajdan Jr. wrote:
> nit: Is this needed?

Mainly to keep the FBTF happy - @erg has been moving them into the .cc whenever
a .cc exists. The dtor is non-trivial due to the |client_authorities|

[Paweł Hajdan Jr., 2010-10-16 11:34:09]

LGTM. Nice CommandLine cleanup!

[wtc, 2010-10-25 20:31:22]

rsleevi: I glanced through Patch Set 4.  (It's longer than
I had time to review.  Sorry!).  It looks correct.  Thanks.

The nested namespaces result in very long types and enum
constants, unfortunately.  I have some comments on minor
issues below.  Perhaps only the last comment is important.

http://codereview.chromium.org/3812007/diff/37001/17005
File net/test/test_server.cc (right):

http://codereview.chromium.org/3812007/diff/37001/17005#newcode45
net/test/test_server.cc:45: int GetHttpsPortBase(const TestServer::HTTPSOptions&
options) {
Nit: this function should be named GetHTTPSPortBase.

http://codereview.chromium.org/3812007/diff/37001/17006
File net/test/test_server.h (right):

http://codereview.chromium.org/3812007/diff/37001/17006#newcode50
net/test/test_server.h:50: CERTIFICATE_OK,
Nit: these can be shortened to
  CERT_OK,
  CERT_MISMATCHED_NAME,
  CERT_EXPIRED,

http://codereview.chromium.org/3812007/diff/37001/17006#newcode57
net/test/test_server.h:57: enum BulkCipherAlgorithm {
Nit: you can shorten this type name to "BulkCipher".

http://codereview.chromium.org/3812007/diff/37001/17006#newcode73
net/test/test_server.h:73: explicit HTTPSOptions(ServerCertificate cert);
Nit: it would be convenient to have a default HTTPSOptions
constructor that initializes server_certificate to
CERTIFICATE_OK.

http://codereview.chromium.org/3812007/diff/37001/17009
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/3812007/diff/37001/17009#newcode1253
net/tools/testserver/testserver.py:1253: 'values are "aes256", "aes128", "3des",
"rc4". If '
You should explain how to specify multiple ciphers.
Do you use multiple --ssl-bulk-cipher options, or a single
--ssl-bulk-cipher option with a comma-separated value?

[Ryan Sleevi, 2010-10-26 02:58:41]

@wtc: Thanks, I've incorporated all of your suggestions. I wasn't sure if the
"It looks correct" was an LGTM or just an off-hand remark, so I'll wait to hear
further from you before committing.

[wtc, 2010-10-26 22:29:44]

rsleevi: I'm sorry I wasn't clear.

I consider phajdan.jr as the primary reviewer of this CL.
Since he said LGTM, you can check this in.

I cannot say "LGTM" because I didn't review the entire CL.
But the parts I reviewed (quickly) look correct.

Please commit this CL.  Thanks!

http://codereview.chromium.org/3812007/diff/51001/52003
File net/socket/ssl_client_socket_unittest.cc (right):

http://codereview.chromium.org/3812007/diff/51001/52003#newcode71
net/socket/ssl_client_socket_unittest.cc:71: NULL /* ssl_host_info */));
Nit: I would delete the /* ssl_host_info */ comments.

http://codereview.chromium.org/3812007/diff/51001/52008
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/3812007/diff/51001/52008#newcode1250
net/tools/testserver/testserver.py:1250: 'specified file. This option may appear
multiple '
Hmm... is this behavior implied by "action='append'"?
I'm sorry if I made you document the obvious.