Third time's a charm?...

Implement SSLInfo() on linux
Implement part of x509_certificate on linux
Implement ReconnectIgnoringLastError on linux
Hoist test case for bad cert down into url_request_unittest

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

[dank, 2008-12-04 17:58:13]

(resending)

We'll need to install certs and make tlslite present on try/build servers for
this...

[Nicolas Sylvain, 2008-12-04 18:15:50]

I added jay to the list of reviewers, because he wrote the initial code.

[Nicolas Sylvain, 2008-12-04 18:26:57]

I'll let wtc review the linux/ssl stuff, but the cert part looks good.

tlslite is only required on linux right?

we'll install the cert of the rest of the build machines

http://codereview.chromium.org/12930/diff/1/8
File chrome/browser/ssl_uitest.cc (right):

http://codereview.chromium.org/12930/diff/1/8#newcode22
Line 22: SSLUITest(): util_(L"chrome/test/data") {
Should it be in net/ ?

[jcampan, 2008-12-04 19:05:05]

Just some style nits.

http://codereview.chromium.org/12930/diff/1/8
File chrome/browser/ssl_uitest.cc (right):

http://codereview.chromium.org/12930/diff/1/8#newcode21
Line 21: 
Remove blank line

http://codereview.chromium.org/12930/diff/1/8#newcode49
Line 49: util_.GetDocRoot(), util_.GetOKCertPath().ToWStringHack());
Should be 4 space indented or aligned with params on previous line.

http://codereview.chromium.org/12930/diff/1/8#newcode54
Line 54: util_.GetDocRoot(), util_.GetExpiredCertPath().ToWStringHack());
Should be 4 space indented or aligned with params on previous line.

http://codereview.chromium.org/12930/diff/1/4
File net/base/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/12930/diff/1/4#newcode27
Line 27: static SECStatus
Methods declaration should be on 1 line, including the {
Also the name should start with a capital.

http://codereview.chromium.org/12930/diff/1/4#newcode30
Line 30: PRErrorCode err = PR_GetError();
Indent should be 2 spaces.

http://codereview.chromium.org/12930/diff/1/4#newcode183
Line 183: int SSLClientSocketNSS::ReconnectIgnoringLastError(CompletionCallback*
callback) {
More than 80 chars

http://codereview.chromium.org/12930/diff/1/4#newcode210
Line 210: CompletionCallback* callback) {
Should be aligned with the params on previous line.

http://codereview.chromium.org/12930/diff/1/4#newcode229
Line 229: CompletionCallback* callback) {
Should be aligned with the params on previous line.

http://codereview.chromium.org/12930/diff/1/6
File net/base/ssl_test_util.cc (right):

http://codereview.chromium.org/12930/diff/1/6#newcode14
Line 14: 
Remove extra blank line.

http://codereview.chromium.org/12930/diff/1/6#newcode82
Line 82: SSLTestUtil::SSLTestUtil(const std::wstring docRoot) :
docRoot_(docRoot){
docRoot -> doc_root

http://codereview.chromium.org/12930/diff/1/6#newcode82
Line 82: SSLTestUtil::SSLTestUtil(const std::wstring docRoot) :
docRoot_(docRoot){
Missing space between ) and {

http://codereview.chromium.org/12930/diff/1/6#newcode93
Line 93: NOTREACHED();
These 2 lines could be simplified as DCHECK(cert);

http://codereview.chromium.org/12930/diff/1/6#newcode136
Line 136: X509_ASN_ENCODING | PKCS_7_ASN_ENCODING,
Params misaligned.

http://codereview.chromium.org/12930/diff/1/6#newcode147
Line 147: "certificate to your trusted roots for this test to work. For more "
Should be 4 space indented.

http://codereview.chromium.org/12930/diff/1/5
File net/base/ssl_test_util.h (right):

http://codereview.chromium.org/12930/diff/1/5#newcode27
Line 27: // Where test data is kept in source tree
Nit: all comments should end with a full stop.

http://codereview.chromium.org/12930/diff/1/5#newcode43
Line 43: bool CheckCATrusted();
Could probably be static.

http://codereview.chromium.org/12930/diff/1/5#newcode47
Line 47: std::wstring docRoot_;
Should be doc_root_

http://codereview.chromium.org/12930/diff/1/5#newcode52
Line 52: #endif
DISALLOW_COPY...

http://codereview.chromium.org/12930/diff/1/5#newcode55
Line 55: #endif
// NET_BASE_SSL_TEST_UTIL_H_

[wtc, 2008-12-04 22:03:33]

The Note in your CL description says base/nss_init.cc, which
should be net/base/ssl_test_util.cc.  One solution is to
add that file to both net_unittests.vcproj and the vcproj
for chrome ui tests.  This is a little ugly because it
violates modularity of the net module.

http://codereview.chromium.org/12930/diff/1/8
File chrome/browser/ssl_uitest.cc (right):

http://codereview.chromium.org/12930/diff/1/8#newcode43
Line 43: TestServer PlainServer() {
Nit: It's not easy to tell from the method names that
PlainServer returns an HTTP test server and GoodServer
and BadServer return HTTPS test servers.  Perhaps the
latter two should be renamed GoodHTTPSServer and
BadHTTPSServer.

http://codereview.chromium.org/12930/diff/1/8#newcode49
Line 49: util_.GetDocRoot(), util_.GetOKCertPath().ToWStringHack());
On 2008/12/04 19:05:05, jcampan wrote:
> Should be 4 space indented or aligned with params on previous line.

Dan, could you run cpplint.py on this CL and in the future?
It should be able to catch style nits like this.

http://codereview.chromium.org/12930/diff/1/8#newcode364
Line 364: HTTPSTestServer https_server(kLocalHost, util_.kOKHTTPSPort,
Just to make sure I understand it: this is not a GoodServer
because the first argument to the constructor is kLocalHost
as opposed to kHostName.  Correct?

http://codereview.chromium.org/12930/diff/1/4
File net/base/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/12930/diff/1/4#newcode28
Line 28: ownBadCertHandler(void * arg, PRFileDesc * socket)
As jcampan pointed out, please reformat this piece of NSS
code according to Google's C++ style guide.

http://codereview.chromium.org/12930/diff/1/6
File net/base/ssl_test_util.cc (right):

http://codereview.chromium.org/12930/diff/1/6#newcode147
Line 147: "certificate to your trusted roots for this test to work. For more "
On 2008/12/04 19:05:05, jcampan wrote:
> Should be 4 space indented.

Alternatively, you can align with the opening quote on the
preceding line.

http://codereview.chromium.org/12930/diff/1/6#newcode150
Line 150: return false;
Incorrect indentation.

http://codereview.chromium.org/12930/diff/1/5
File net/base/ssl_test_util.h (right):

http://codereview.chromium.org/12930/diff/1/5#newcode17
Line 17: SSLTestUtil(const std::wstring docRoot);
The parameter should be a const reference:
  SSLTestUtil(const std::wstring& doc_root);

http://codereview.chromium.org/12930/diff/1/15
File net/data/ssl/certificates/expired_cert.pem (right):

http://codereview.chromium.org/12930/diff/1/15#newcode1
Line 1: -----BEGIN CERTIFICATE-----
I like short directory name "certs".  If it's too much
work changing it now, don't worry about it.

http://codereview.chromium.org/12930/diff/1/7
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/12930/diff/1/7#newcode126
Line 126: HTTPSRequestTest() : util_(L"chrome/test/data") {};
The path "chrome/test/data" needs to be updated to
"net/data".

http://codereview.chromium.org/12930/diff/1/7#newcode139
Line 139: HTTPSTestServer https_server(util_.kHostName, util_.kOKHTTPSPort,
Isn't this GoodServer()?  Does this file not have access to
GoodServer()?

[dank, 2008-12-05 03:27:53]

This builds on Linux, but can't be tried on the trybots;
I'll build it for real early tomorrow and try to submit
it as soon as I get an LGTM, and be ready to back it
out quickly if one of the build server fails (which 
seems likely, given the need for tlslite and certs).

[wtc, 2008-12-05 22:35:29]

LGTM.  Just a few nits below.  Please go ahead
and check it in after fixing them.

http://codereview.chromium.org/12930/diff/406/280
File chrome/browser/ssl_uitest.cc (right):

http://codereview.chromium.org/12930/diff/406/280#newcode59
Line 59: DISALLOW_COPY_AND_ASSIGN(SSLUITest);
You need to put DISALLOW_COPY_AND_ASSIGN in the private
section, so please add "private:" before this line.

http://codereview.chromium.org/12930/diff/406/280#newcode365
Line 365: scoped_ptr<HTTPSTestServer> https_server(
You don't need to use scoped_ptr and new for this one.
I guess you're going for consistency with the other
unit tests.

http://codereview.chromium.org/12930/diff/406/272
File net/base/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/12930/diff/406/272#newcode27
Line 27: static SECStatus ownBadCertHandler(void* arg, PRFileDesc* socket) {
I guess we also need to capitalize the function name:
OwnBadCertHandler.  Sorry I missed this before.

http://codereview.chromium.org/12930/diff/406/272#newcode131
Line 131: :
It's better to place the colon (:) on the next line:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Initializer_Lists

http://codereview.chromium.org/12930/diff/406/272#newcode301
Line 301: * Do network I/O between the given buffer and the given socket.
Nit: use C++ style comment // for this block comment.