Defines SSLServerSocket and implements SSLServerSocketNSS

Defines SSLServerSocket and implements SSLServerSocketNSS

Defines a SSLServerSocket interface. Implement this interface using NSS as
SSLServerSocketNSS.

This is the first version of the code. It disables several functions of NSS
like caching, session ticket, reneogotiation, etc. This is implemented to
suit the needs of Chromoting. Additional features of this socket will be added
when necessary.

BUG=None
TEST=None

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

[Alpha Left Google, 2010-12-09 22:30:26]

Not ready for review yet since I need to write unit test for this code. But
publishing this as a preview.

[Alpha Left Google, 2010-12-10 23:46:23]

This code is now ready for review.

[wtc, 2010-12-16 03:23:35]

I added agl as a reviewer.

[wtc, 2010-12-17 00:16:26]

This is impressive work!  I especially like your unit tests.

Please address my comments below, and then ask agl to do a
second round of review.

General comments:

We should try harder to share code between SSLClientSocketNSS
and SSLServerSocketNSS.  Several methods have the same code.
I wonder if we should have an SSLSocketNSS base class, and
then have SSLClientSocketNSS and SSLServerSocketNSS
derive from SSLSocketNSS+ClientSocket and
SSLSocketNSS+Socket, respectively.  I heard that multiple
inheritance has a lot of problems, so we'll need to do
this carefully.

I wonder if we should have a ServerSocket interface.
It's not necessary for your case, so it's OK to not have
it.

Please add comments to ssl_server_socket.h to note that
this is intended for peer-to-peer networking, as opposed
to a true server.

Disabling SSL session cache for SSLServerSocket seems fine
for peer-to-peer networking, if it simplifies the code.
I guess you don't want to deal with the issue of
calling SSL_ConfigServerSessionIDCache, because that
function should only be called once in a process.

http://codereview.chromium.org/5746003/diff/10001/net/base/ssl_config_service.h
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/5746003/diff/10001/net/base/ssl_config_service...
net/base/ssl_config_service.h:34: bool no_cache_enabled;  // Disable caching.
no_cache_enabled => session_cache_disabled

More important, we should avoid adding this config setting
if we can.  Can we use a small SSL session cache for
SSLServerSocket?

http://codereview.chromium.org/5746003/diff/10001/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/5746003/diff/10001/net/net.gyp#newcode611
net/net.gyp:611: 'socket/ssl_server_socket_nss.cc',
Also add: socket/ssl_server_socket.h

http://codereview.chromium.org/5746003/diff/10001/net/socket/nss_common.cc
File net/socket/nss_common.cc (right):

http://codereview.chromium.org/5746003/diff/10001/net/socket/nss_common.cc#ne...
net/socket/nss_common.cc:16: #include <sslproto.h>
Many of these headers don't need to be included now.

You should be able to remove at least
  hasht.h
  keyhi.h
  nspr.h (included by nss_common.h)
  pk11pub.h

http://codereview.chromium.org/5746003/diff/10001/net/socket/nss_common.cc#ne...
net/socket/nss_common.cc:65: // All other SSL options are set per-session by
SSLClientSocket.
"SSLClientSocket" needs to be updated.

http://codereview.chromium.org/5746003/diff/10001/net/socket/nss_common.cc#ne...
net/socket/nss_common.cc:87: 
Nit: remove blank lines here and on line 122 below.

http://codereview.chromium.org/5746003/diff/10001/net/socket/nss_common.h
File net/socket/nss_common.h (right):

http://codereview.chromium.org/5746003/diff/10001/net/socket/nss_common.h#new...
net/socket/nss_common.h:6: //  ssl_server_socket_nss.cc to share common
functions of NSS.
Nit: extra space at beginning of comment line.

Please name these two files nss_util.{h,cc} or
nss_ssl_util.{h,cc}.

It may be better to put these functions inside the
nested net::nss namespace.

http://codereview.chromium.org/5746003/diff/10001/net/socket/nss_common.h#new...
net/socket/nss_common.h:17: // Initalize NSS library.
NSS library => the NSS SSL library

http://codereview.chromium.org/5746003/diff/10001/net/socket/nss_common.h#new...
net/socket/nss_common.h:20: // Log a failing NSS funcion call.
Nit: failing => failed

http://codereview.chromium.org/5746003/diff/10001/net/socket/nss_common.h#new...
net/socket/nss_common.h:29: int MapNSPRError(PRErrorCode err);
It looks nicer to name this function MapNSSError, to match
MapErrorToNSS above.  Please update the comment at line 28
accordingly.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:119: #if 0
IMPORTANT: please undo this change.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socket.h
File net/socket/ssl_server_socket.h (right):

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket.h:1: // Copyright (c) 2006-2009 The Chromium
Authors. All rights reserved.
Nit: 2006-2009 => 2010

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket.h:22: // authentication and encryption on top of
it.
Nit: remove "authentication and encryption".

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket.h:25: SSLServerSocket() {}
Delete the constructor, and the four 'Socket' interface
methods on lines 38-44.  I would also delete the
DISALLOW_COPY_AND_ASSIGN on line 47.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket.h:31: // This method takes a callback. Return true
value of this method is the
Nit: Return true value => Return value

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket.h:50: // Creates a SSL server socket using a
already connected socket. A signed
Nit: a SSL => an SSL
     a already => an already

Nit: remove "signed".  (A certificate is always signed.)

We probably should add a ServerSocketFactory class,
and make CreateSSLServerSocket a method of the factory.
This allows mocking.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket.h:53: // This created server socket will take
ownership of |socket|. However |key|
Since we can't copy a base::RSAPrivateKey easily, it would be
better to have this object take ownership of |key|, or
require that |key| remains valid for the lifetime of this
object.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket.h:56: Socket* socket,
scoped_refptr<X509Certificate> certificate,
Just use X509Certificate* certificate.  No need to pass
scoped_refptr.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
File net/socket/ssl_server_socket_nss.cc (right):

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.cc:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
If ssl_server_socket_nss.cc is heavily based on
ssl_client_socket_nss.cc, please use "svn cp" to add
this file, so I can see the diffs more easily.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.cc:398: // OnSendComplete may need to call
DoPayloadRead while the renegotiation
You should omit the renegotiation-related code, which is
client-side specific.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
File net/socket/ssl_server_socket_nss.h (right):

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.h:29: class X509Certificate;
I believe most of the header inclusions and forward
declarations can be removed because
"net/socket/ssl_server_socket.h" already takes care of them.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.h:33: // This object takes ownership of the
parameters given in the constructor.
This is not true for |key|; this object copies |key|.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.h:37: SSLServerSocketNSS(Socket* socket,
The constructor should take a "const SSLConfig& ssl_config"
parameter.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.h:76: static SECStatus OwnAuthCertHandler(void*
arg,
OwnAuthCertHandler is needed only if your server will verify
client certificates.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.h:80: static SECStatus
PlatformClientAuthHandler(
Remove PlatformClientAuthHandler and ClientAuthHandler.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.h:126: scoped_ptr<Socket> socket_;
Please name this member transport_ or transport_socket_.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.h:131: // Private key used by the server for
encryption.
Nit: delete "for encryption".  (It's used for key exchange.)

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.h:134: enum State {
If there is only one state, I wonder if we still need a state
machine...

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
File net/socket/ssl_server_socket_unittest.cc (right):

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:30: #include
"net/socket/ssl_server_socket.h"
Nit: list "net/socket/ssl_server_socket.h" first.
This is suggested by an example in the Style Guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:135: return false;
This probably should return true.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:171: // Establish connections between
two sockets.
Nit: connections => channels

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:194: EXPECT_EQ(kTestDataSize,
callback.WaitForResult());
BUG: these two statements should be reversed.  You need to
first wait for Read completion and then do the memcmp.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:207: net::ClientSocket*
fake_server_socket = new FakeClientSocket(&channel_2_,
It is confusing to see fake_server_socket is a ClientSocket.
I hope it's possible to clean this up.  Perhaps define
both FakeSocket and FakeClientSocket?

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:234:
net::ClientSocketFactory::GetDefaultFactory()->CreateSSLClientSocket(
You should replace net::ClientSocketFactory::GetDefaultFactory()
by socket_factory_, otherwise the socket_factory_ member
is not being used.  Or you can just remove the socket_factory_
member.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:249: TEST_F(SSLServerSocketTest,
Initialize) {
What's the purpose of this test?  To check that Initialize()
doesn't crash and has no valgrind errors?

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:253: TEST_F(SSLServerSocketTest,
Connect) {
Nit: it may be better to name this test Handshake since
it tests not only Connect but also Accept.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:265: if (client_ret ==
net::ERR_IO_PENDING) {
Nit: in the 'net' module we usually omit curly braces around
one-liner "if" bodies.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:315: // Read then write.
Since you reuse the read buffer for the "Read then write"
part of this unit test, it'd be better to have a second
test data, so that you can be sure the second Read call
actually reads in new data.

[Alpha Left Google, 2010-12-17 01:02:13]

That's for thorough review. Here's my thoughts on your high level comments. I'll
address your code comments later.

On 2010/12/17 00:16:26, wtc wrote:
> This is impressive work!  I especially like your unit tests.
> 
> Please address my comments below, and then ask agl to do a
> second round of review.
> 
> General comments:
> 
> We should try harder to share code between SSLClientSocketNSS
> and SSLServerSocketNSS.  Several methods have the same code.
> I wonder if we should have an SSLSocketNSS base class, and
> then have SSLClientSocketNSS and SSLServerSocketNSS
> derive from SSLSocketNSS+ClientSocket and
> SSLSocketNSS+Socket, respectively.  I heard that multiple
> inheritance has a lot of problems, so we'll need to do
> this carefully.
> 

I do like the idea of sharing code between SSLClientSocketNSS and
SSLServerSocketNSS. I agree that we should do this a later CL. Instead of doing
this with inheritance, I suggest we define a SSLSocketNSS which can operates in
server mode and client mode, the number of different branches should be small.
and then server socket and client socket would use it as composition.

> I wonder if we should have a ServerSocket interface.
> It's not necessary for your case, so it's OK to not have
> it.

I'm not trying to define a generic server socket interface so I'd skip this.

> Please add comments to ssl_server_socket.h to note that
> this is intended for peer-to-peer networking, as opposed
> to a true server.

OK. Will do.

> 
> Disabling SSL session cache for SSLServerSocket seems fine
> for peer-to-peer networking, if it simplifies the code.
> I guess you don't want to deal with the issue of
> calling SSL_ConfigServerSessionIDCache, because that
> function should only be called once in a process.

That's right. Would like to start with simple code first before we complete the
features / optimize handshaking latency.

[Alpha Left Google, 2010-12-17 08:30:43]

http://codereview.chromium.org/5746003/diff/10001/net/base/ssl_config_service.h
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/5746003/diff/10001/net/base/ssl_config_service...
net/base/ssl_config_service.h:34: bool no_cache_enabled;  // Disable caching.
On 2010/12/17 00:16:26, wtc wrote:
> no_cache_enabled => session_cache_disabled
> 
> More important, we should avoid adding this config setting
> if we can.  Can we use a small SSL session cache for
> SSLServerSocket?

I'd like to simply the implementations in the first version. What about adding a
TODO and file a bug to complete this feature?

http://codereview.chromium.org/5746003/diff/10001/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/5746003/diff/10001/net/net.gyp#newcode611
net/net.gyp:611: 'socket/ssl_server_socket_nss.cc',
On 2010/12/17 00:16:26, wtc wrote:
> Also add: socket/ssl_server_socket.h

Done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/nss_common.cc
File net/socket/nss_common.cc (right):

http://codereview.chromium.org/5746003/diff/10001/net/socket/nss_common.cc#ne...
net/socket/nss_common.cc:16: #include <sslproto.h>
On 2010/12/17 00:16:26, wtc wrote:
> Many of these headers don't need to be included now.
> 
> You should be able to remove at least
>   hasht.h
>   keyhi.h
>   nspr.h (included by nss_common.h)
>   pk11pub.h

Done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/nss_common.cc#ne...
net/socket/nss_common.cc:65: // All other SSL options are set per-session by
SSLClientSocket.
On 2010/12/17 00:16:26, wtc wrote:
> "SSLClientSocket" needs to be updated.

Done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/nss_common.cc#ne...
net/socket/nss_common.cc:87: 
On 2010/12/17 00:16:26, wtc wrote:
> Nit: remove blank lines here and on line 122 below.

Done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/nss_common.h
File net/socket/nss_common.h (right):

http://codereview.chromium.org/5746003/diff/10001/net/socket/nss_common.h#new...
net/socket/nss_common.h:6: //  ssl_server_socket_nss.cc to share common
functions of NSS.
On 2010/12/17 00:16:26, wtc wrote:
> Nit: extra space at beginning of comment line.
> 
> Please name these two files nss_util.{h,cc} or
> nss_ssl_util.{h,cc}.
> 
> It may be better to put these functions inside the
> nested net::nss namespace.

Done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/nss_common.h#new...
net/socket/nss_common.h:17: // Initalize NSS library.
On 2010/12/17 00:16:26, wtc wrote:
> NSS library => the NSS SSL library

Done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/nss_common.h#new...
net/socket/nss_common.h:20: // Log a failing NSS funcion call.
On 2010/12/17 00:16:26, wtc wrote:
> Nit: failing => failed

Done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/nss_common.h#new...
net/socket/nss_common.h:29: int MapNSPRError(PRErrorCode err);
On 2010/12/17 00:16:26, wtc wrote:
> It looks nicer to name this function MapNSSError, to match
> MapErrorToNSS above.  Please update the comment at line 28
> accordingly.

Done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:119: #if 0
On 2010/12/17 00:16:26, wtc wrote:
> IMPORTANT: please undo this change.

Uhh.... Careless.. This is done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socket.h
File net/socket/ssl_server_socket.h (right):

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket.h:1: // Copyright (c) 2006-2009 The Chromium
Authors. All rights reserved.
On 2010/12/17 00:16:26, wtc wrote:
> Nit: 2006-2009 => 2010

Done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket.h:22: // authentication and encryption on top of
it.
On 2010/12/17 00:16:26, wtc wrote:
> Nit: remove "authentication and encryption".

Done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket.h:31: // This method takes a callback. Return true
value of this method is the
On 2010/12/17 00:16:26, wtc wrote:
> Nit: Return true value => Return value

Done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket.h:50: // Creates a SSL server socket using a
already connected socket. A signed
On 2010/12/17 00:16:26, wtc wrote:
> Nit: a SSL => an SSL
>      a already => an already
> 
> Nit: remove "signed".  (A certificate is always signed.)
> 
> We probably should add a ServerSocketFactory class,
> and make CreateSSLServerSocket a method of the factory.
> This allows mocking.

Fixed nits. Added a TODO to define ServerSocketFactory.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket.h:53: // This created server socket will take
ownership of |socket|. However |key|
On 2010/12/17 00:16:26, wtc wrote:
> Since we can't copy a base::RSAPrivateKey easily, it would be
> better to have this object take ownership of |key|, or
> require that |key| remains valid for the lifetime of this
> object.

In the case of chromoting this is not desirable. Between server and client there
are three SSL connections. So either we define RSAPrivateKey::Clone() or we
assume this constructor will copy the private key.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket.h:56: Socket* socket,
scoped_refptr<X509Certificate> certificate,
On 2010/12/17 00:16:26, wtc wrote:
> Just use X509Certificate* certificate.  No need to pass
> scoped_refptr.

Done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
File net/socket/ssl_server_socket_nss.cc (right):

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.cc:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
On 2010/12/17 00:16:26, wtc wrote:
> If ssl_server_socket_nss.cc is heavily based on
> ssl_client_socket_nss.cc, please use "svn cp" to add
> this file, so I can see the diffs more easily.

In fact I can't do this.. since git doesn't have an equivalent command for svn
cp.. :(

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.cc:398: // OnSendComplete may need to call
DoPayloadRead while the renegotiation
On 2010/12/17 00:16:26, wtc wrote:
> You should omit the renegotiation-related code, which is
> client-side specific.

Done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
File net/socket/ssl_server_socket_nss.h (right):

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.h:29: class X509Certificate;
On 2010/12/17 00:16:26, wtc wrote:
> I believe most of the header inclusions and forward
> declarations can be removed because
> "net/socket/ssl_server_socket.h" already takes care of them.

Done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.h:33: // This object takes ownership of the
parameters given in the constructor.
On 2010/12/17 00:16:26, wtc wrote:
> This is not true for |key|; this object copies |key|.

Done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.h:37: SSLServerSocketNSS(Socket* socket,
On 2010/12/17 00:16:26, wtc wrote:
> The constructor should take a "const SSLConfig& ssl_config"
> parameter.

Done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.h:76: static SECStatus OwnAuthCertHandler(void*
arg,
On 2010/12/17 00:16:26, wtc wrote:
> OwnAuthCertHandler is needed only if your server will verify
> client certificates.

Leaving this as a TODO in .cc because in the future we do want to verify client
certificate.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.h:80: static SECStatus
PlatformClientAuthHandler(
On 2010/12/17 00:16:26, wtc wrote:
> Remove PlatformClientAuthHandler and ClientAuthHandler.

Done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.h:126: scoped_ptr<Socket> socket_;
On 2010/12/17 00:16:26, wtc wrote:
> Please name this member transport_ or transport_socket_.

Done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.h:131: // Private key used by the server for
encryption.
On 2010/12/17 00:16:26, wtc wrote:
> Nit: delete "for encryption".  (It's used for key exchange.)

Done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.h:134: enum State {
On 2010/12/17 00:16:26, wtc wrote:
> If there is only one state, I wonder if we still need a state
> machine...

Trying to keep this similar to the client code, makes it easier to share code
between them.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
File net/socket/ssl_server_socket_unittest.cc (right):

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:30: #include
"net/socket/ssl_server_socket.h"
On 2010/12/17 00:16:26, wtc wrote:
> Nit: list "net/socket/ssl_server_socket.h" first.
> This is suggested by an example in the Style Guide:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

Done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:135: return false;
On 2010/12/17 00:16:26, wtc wrote:
> This probably should return true.

Done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:171: // Establish connections between
two sockets.
On 2010/12/17 00:16:26, wtc wrote:
> Nit: connections => channels

Done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:194: EXPECT_EQ(kTestDataSize,
callback.WaitForResult());
On 2010/12/17 00:16:26, wtc wrote:
> BUG: these two statements should be reversed.  You need to
> first wait for Read completion and then do the memcmp.

nice catch, done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:207: net::ClientSocket*
fake_server_socket = new FakeClientSocket(&channel_2_,
On 2010/12/17 00:16:26, wtc wrote:
> It is confusing to see fake_server_socket is a ClientSocket.
> I hope it's possible to clean this up.  Perhaps define
> both FakeSocket and FakeClientSocket?

Renamed FakeClientSocket to FakeSocket. And instead of net::ClientSocket*, have
the variables be type FakeSocket*.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:234:
net::ClientSocketFactory::GetDefaultFactory()->CreateSSLClientSocket(
On 2010/12/17 00:16:26, wtc wrote:
> You should replace net::ClientSocketFactory::GetDefaultFactory()
> by socket_factory_, otherwise the socket_factory_ member
> is not being used.  Or you can just remove the socket_factory_
> member.

Done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:249: TEST_F(SSLServerSocketTest,
Initialize) {
On 2010/12/17 00:16:26, wtc wrote:
> What's the purpose of this test?  To check that Initialize()
> doesn't crash and has no valgrind errors?

To make sure setting up the sockets is successful. And to have minimal scope for
valgrind. I'll document that.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:253: TEST_F(SSLServerSocketTest,
Connect) {
On 2010/12/17 00:16:26, wtc wrote:
> Nit: it may be better to name this test Handshake since
> it tests not only Connect but also Accept.

Done.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:265: if (client_ret ==
net::ERR_IO_PENDING) {
On 2010/12/17 00:16:26, wtc wrote:
> Nit: in the 'net' module we usually omit curly braces around
> one-liner "if" bodies.

Yup, I tend to omit {} for a single line, but this is the exception where the
content is a macro.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:315: // Read then write.
On 2010/12/17 00:16:26, wtc wrote:
> Since you reuse the read buffer for the "Read then write"
> part of this unit test, it'd be better to have a second
> test data, so that you can be sure the second Read call
> actually reads in new data.

Done.

[wtc, 2010-12-17 16:38:32]

hclam: thank you for making my suggested changes quickly.
Let me first respond to your comments below, and then I'll
review the new Patch Set.

http://codereview.chromium.org/5746003/diff/10001/net/base/ssl_config_service.h
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/5746003/diff/10001/net/base/ssl_config_service...
net/base/ssl_config_service.h:34: bool no_cache_enabled;  // Disable caching.
Adding a TODO for enabling server session cache is fine.

But you should not need to disable client session cache.
Why do you need to do that in SSLServerSocketTest?

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socket.h
File net/socket/ssl_server_socket.h (right):

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket.h:53: // This created server socket will take
ownership of |socket|. However |key|
I see.  Perhaps the proper solution is to make RSAPrivateKey
ref-counted?  If you agree, please add a TODO comment.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
File net/socket/ssl_server_socket_unittest.cc (right):

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:265: if (client_ret ==
net::ERR_IO_PENDING) {
On 2010/12/17 08:30:43, Alpha wrote:
>
> Yup, I tend to omit {} for a single line, but this is the exception where the
> content is a macro.

Someone else also told me about this exception.  Does
this exception comes from the Style Guide?

[wtc, 2010-12-17 18:08:04]

LGTM.  Please address the comments below before you check
this in, especially the comments marked with "IMPORTANT",
"API DESIGN", or "BUG".

My main concern is why you need to set the SSL_NO_CACHE
option on SSLClientSocketNSS in SSLServerSocketTest.

Please file a bug and add a TODO about sharing more code
between SSLClientSocketNSS and SSLServerSocketNSS.

agl: It would be nice if you could skim through this CL,
but I've had it covered.

http://codereview.chromium.org/5746003/diff/25001/net/base/ssl_config_service.h
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/5746003/diff/25001/net/base/ssl_config_service...
net/base/ssl_config_service.h:38: bool session_cache_disabled;  // Disable
caching.
Please change the comment to say
  Don't use the session cache.
because "disabling cache" may be misunderstood as
disabling the session cache globally.  Perhaps this
member should be named session_resume_disabled.

http://codereview.chromium.org/5746003/diff/25001/net/socket/nss_ssl_util.h
File net/socket/nss_ssl_util.h (right):

http://codereview.chromium.org/5746003/diff/25001/net/socket/nss_ssl_util.h#n...
net/socket/nss_ssl_util.h:14: #include <nss.h>
I believe this file only needs to include <nspr.h>.
In fact, just <prerror.h> (for PRErrorCode) is enough.

http://codereview.chromium.org/5746003/diff/25001/net/socket/nss_ssl_util.h#n...
net/socket/nss_ssl_util.h:35: int MapHandshakeError(PRErrorCode err);
Nit: since this function is in the top-level net namespace,
let's rename this function MapNSSHandshakeError to make clear
it's NSS-specific.  Please also say "Map NSS handshake error..."
in the comment.

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socket.h
File net/socket/ssl_server_socket.h (right):

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket.h:25: // SSLClientSocket and is not intended to be
used as a standaline SSL server.
Remove "in pair with SSLClientSocket", because the other side
can use any SSL client implementation.

Typo: standaline => standalone.

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
File net/socket/ssl_server_socket_nss.cc (right):

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.cc:46: // default object.
API DESIGN: it's better to change CreateSSLServerSocket()
to take a "const SSLConfig& ssl_config" parameter, and pass
that to the SSLServerSocketNSS constructor.

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.cc:51: Socket* socket,
Nit: socket => transport_socket

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.cc:52: const SSLConfig& ssl_config,
Nit: list ssl_config after cert and key.

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.cc:395: void
SSLServerSocketNSS::OnSendComplete(int result) {
IMPORTANT: I believe OnSendComplete can be written just like
OnRecvComplete now.  The remaining code is essentially
DoWriteLoop + DoWriteCallback.

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
File net/socket/ssl_server_socket_nss.h (right):

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.h:33: SSLServerSocketNSS(Socket* socket,
Nit: socket => transport_socket

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
File net/socket/ssl_server_socket_unittest.cc (right):

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:216: net::X509Certificate* cert =
BUG: you should use a scoped_refptr<net::X509Certificate>
here.

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:247: scoped_ptr<net::CertVerifier>
cert_verifier_;
You can use
  net::CertVerifier cert_verifier_;
here, and pass
  &cert_verifier_
to socket_factory_->CreateSSLClientSocket() at
line 237 above.

[agl, 2010-12-17 18:30:50]

LGTM

http://codereview.chromium.org/5746003/diff/25001/net/base/ssl_config_service.h
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/5746003/diff/25001/net/base/ssl_config_service...
net/base/ssl_config_service.h:35: // TODO(hclam): This option is used to
simplify SSLServerSocketNSS
s/simplify/simplify the/

http://codereview.chromium.org/5746003/diff/25001/net/base/ssl_config_service...
net/base/ssl_config_service.h:36: // implementation and should be removed when
session cache is implemented.
s/cache/caching/

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socket.h
File net/socket/ssl_server_socket.h (right):

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket.h:24: // This class is designed to work in
peer-to-peer connection in pair with
s/in/in a/

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket.h:25: // SSLClientSocket and is not intended to be
used as a standaline SSL server.
s/standaline/standalone/

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket.h:30: // Performs SSL server handshaking on an
existing socket. The socket given
"Performs an SSL server handshake on the existing socket. The given socket must
have already been connected."

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket.h:33: // This method takes a callback. Return true
value of this method is the
"Accept either returns ERR_IO_PENDING, in which case the given callback will be
called in the future with the real result, or it completes synchronously,
returning the result immediately."

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
File net/socket/ssl_server_socket_nss.cc (right):

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.cc:45: // SSLConfig is currently not used in
SSLServerSocketNSS so pass in the
s/so/so we/

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
File net/socket/ssl_server_socket_nss.h (right):

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.h:30: // The following parameter are copied in
the constructor.
s/parameter/parameters/

[Alpha Left Google, 2010-12-17 20:09:57]

wtc: Answering your questions first.

http://codereview.chromium.org/5746003/diff/10001/net/base/ssl_config_service.h
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/5746003/diff/10001/net/base/ssl_config_service...
net/base/ssl_config_service.h:34: bool no_cache_enabled;  // Disable caching.
On 2010/12/17 16:38:32, wtc wrote:
> Adding a TODO for enabling server session cache is fine.
> 
> But you should not need to disable client session cache.
> Why do you need to do that in SSLServerSocketTest?

SSLClientSocketNSS uses session key between tests. Since server implementation
doesn't support session key, tests after the one will fail since it fails
handshaking.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socket.h
File net/socket/ssl_server_socket.h (right):

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket.h:53: // This created server socket will take
ownership of |socket|. However |key|
On 2010/12/17 16:38:32, wtc wrote:
> I see.  Perhaps the proper solution is to make RSAPrivateKey
> ref-counted?  If you agree, please add a TODO comment.

Added TODO to RSAPrivateKey.

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
File net/socket/ssl_server_socket_unittest.cc (right):

http://codereview.chromium.org/5746003/diff/10001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:265: if (client_ret ==
net::ERR_IO_PENDING) {
On 2010/12/17 16:38:32, wtc wrote:
> On 2010/12/17 08:30:43, Alpha wrote:
> >
> > Yup, I tend to omit {} for a single line, but this is the exception where
the
> > content is a macro.
> 
> Someone else also told me about this exception.  Does
> this exception comes from the Style Guide?

Don't remember if this is in the Style Guide. But it is dangerous to have
if(...) DCHECK(); and the best is to enclose all marcos by {}.

http://codereview.chromium.org/5746003/diff/25001/net/base/ssl_config_service.h
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/5746003/diff/25001/net/base/ssl_config_service...
net/base/ssl_config_service.h:38: bool session_cache_disabled;  // Disable
caching.
On 2010/12/17 18:08:04, wtc wrote:
> Please change the comment to say
>   Don't use the session cache.
> because "disabling cache" may be misunderstood as
> disabling the session cache globally.  Perhaps this
> member should be named session_resume_disabled.

Done.

http://codereview.chromium.org/5746003/diff/25001/net/base/ssl_config_service...
net/base/ssl_config_service.h:38: bool session_cache_disabled;  // Disable
caching.
On 2010/12/17 18:08:04, wtc wrote:
> Please change the comment to say
>   Don't use the session cache.
> because "disabling cache" may be misunderstood as
> disabling the session cache globally.  Perhaps this
> member should be named session_resume_disabled.

Done.

http://codereview.chromium.org/5746003/diff/25001/net/socket/nss_ssl_util.h
File net/socket/nss_ssl_util.h (right):

http://codereview.chromium.org/5746003/diff/25001/net/socket/nss_ssl_util.h#n...
net/socket/nss_ssl_util.h:35: int MapHandshakeError(PRErrorCode err);
On 2010/12/17 18:08:04, wtc wrote:
> Nit: since this function is in the top-level net namespace,
> let's rename this function MapNSSHandshakeError to make clear
> it's NSS-specific.  Please also say "Map NSS handshake error..."
> in the comment.

Done.

[Alpha Left Google, 2010-12-17 20:31:12]

Address comments by wtc and agl.

http://codereview.chromium.org/5746003/diff/25001/net/socket/nss_ssl_util.h
File net/socket/nss_ssl_util.h (right):

http://codereview.chromium.org/5746003/diff/25001/net/socket/nss_ssl_util.h#n...
net/socket/nss_ssl_util.h:14: #include <nss.h>
On 2010/12/17 18:08:04, wtc wrote:
> I believe this file only needs to include <nspr.h>.
> In fact, just <prerror.h> (for PRErrorCode) is enough.

Done.

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socket.h
File net/socket/ssl_server_socket.h (right):

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket.h:25: // SSLClientSocket and is not intended to be
used as a standaline SSL server.
On 2010/12/17 18:08:04, wtc wrote:
> Remove "in pair with SSLClientSocket", because the other side
> can use any SSL client implementation.
> 
> Typo: standaline => standalone.

Done.

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket.h:25: // SSLClientSocket and is not intended to be
used as a standaline SSL server.
On 2010/12/17 18:08:04, wtc wrote:
> Remove "in pair with SSLClientSocket", because the other side
> can use any SSL client implementation.
> 
> Typo: standaline => standalone.

Done.

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket.h:30: // Performs SSL server handshaking on an
existing socket. The socket given
On 2010/12/17 18:30:50, agl wrote:
> "Performs an SSL server handshake on the existing socket. The given socket
must
> have already been connected."

Done.

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket.h:33: // This method takes a callback. Return true
value of this method is the
On 2010/12/17 18:30:50, agl wrote:
> "Accept either returns ERR_IO_PENDING, in which case the given callback will
be
> called in the future with the real result, or it completes synchronously,
> returning the result immediately."

Done.

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
File net/socket/ssl_server_socket_nss.cc (right):

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.cc:46: // default object.
On 2010/12/17 18:08:04, wtc wrote:
> API DESIGN: it's better to change CreateSSLServerSocket()
> to take a "const SSLConfig& ssl_config" parameter, and pass
> that to the SSLServerSocketNSS constructor.

Done.

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.cc:51: Socket* socket,
On 2010/12/17 18:08:04, wtc wrote:
> Nit: socket => transport_socket

Done.

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.cc:52: const SSLConfig& ssl_config,
On 2010/12/17 18:08:04, wtc wrote:
> Nit: list ssl_config after cert and key.

Done.

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.cc:52: const SSLConfig& ssl_config,
On 2010/12/17 18:08:04, wtc wrote:
> Nit: list ssl_config after cert and key.

Done.

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.cc:395: void
SSLServerSocketNSS::OnSendComplete(int result) {
On 2010/12/17 18:08:04, wtc wrote:
> IMPORTANT: I believe OnSendComplete can be written just like
> OnRecvComplete now.  The remaining code is essentially
> DoWriteLoop + DoWriteCallback.

Done.

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
File net/socket/ssl_server_socket_nss.h (right):

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.h:30: // The following parameter are copied in
the constructor.
On 2010/12/17 18:30:50, agl wrote:
> s/parameter/parameters/

Done.

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_nss.h:33: SSLServerSocketNSS(Socket* socket,
On 2010/12/17 18:08:04, wtc wrote:
> Nit: socket => transport_socket

Done.

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
File net/socket/ssl_server_socket_unittest.cc (right):

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:216: net::X509Certificate* cert =
On 2010/12/17 18:08:04, wtc wrote:
> BUG: you should use a scoped_refptr<net::X509Certificate>
> here.

Done.

http://codereview.chromium.org/5746003/diff/25001/net/socket/ssl_server_socke...
net/socket/ssl_server_socket_unittest.cc:247: scoped_ptr<net::CertVerifier>
cert_verifier_;
On 2010/12/17 18:08:04, wtc wrote:
> You can use
>   net::CertVerifier cert_verifier_;
> here, and pass
>   &cert_verifier_
> to socket_factory_->CreateSSLClientSocket() at
> line 237 above.

Done.

[agl, 2010-12-17 20:41:18]

Still LGTM