Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7054010:
Update SSLServerSocket to provide the net::StreamSocket interface. (Closed)
|
Created:
9 years, 7 months ago by Wez Modified:
9 years, 6 months ago Reviewers:
willchan no longer on Chromium, Sergey Ulanov, bulach, wtc, Mike Belshe, commit-bot: I haz the power CC:
chromium-reviews, jamiewalch+watch_chromium.org, hclam+watch_chromium.org, cbentzel+watch_chromium.org, simonmorris+watch_chromium.org, wez+watch_chromium.org, dmaclach+watch_chromium.org, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, darin-cc_chromium.org, ajwong+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionThis change is part of a cleanup of the remoting component's session initialisation.
Have SSLServerSocket implement the StreamSocket interface, since SSL is stream-oriented. Calling code can then make use of methods not available on the more general Socket interface, such as IsConnected and Disconnect.
BUG=
TEST=Unit tests.
Patch Set 1 : Split up CL; this one just covers the StreamSocket change. #Patch Set 2 : Fix the SSLServerSocketOpenSSL stub for these changes. #
Total comments: 6
Patch Set 3 : Pick nits. #Patch Set 4 : Remove const qualifier on RSAPrivateKey parameter. #
Total comments: 2
Patch Set 5 : Fix function signature for OpenSSL build. #
Total comments: 11
Patch Set 6 : Reinstate SSLServerSocket type, and address comments. #
Total comments: 15
Patch Set 7 : Switch back to SSLServerSocket from StreamSocket at call-sites. #
Total comments: 4
Patch Set 8 : Add a StartHandshake method, and make Connect NOT_IMPLEMENTED. #
Total comments: 8
Patch Set 9 : Fix net_unittests and rename things consistently to "handshake". #
Total comments: 12
Patch Set 10 : Address comments. #Patch Set 11 : Fix missed Connect call to Handshake in unit test. #
Messages
Total messages: 31 (0 generated)
This just a heads-up; no need to review anything (yet). It's an extension of CL 7040021 which additionally changes the SSLServerSocket to be a full StreamSocket (currently only for NSS), and fixes the connection initialization logic in JingleSession.
Alpha: Any thoughts on why SSLServerSocket shouldn't be a StreamSocket as in this CL? (CL also gets rid of SocketWrapper; please ignore that for now!)
I think it should be a StreamSocket. StreamSocket was called ClientSocket at the time SSLServerSocket was implemented - and I think that's why nobody even considered implementing ClientSocket interface in SSLServerSocket :(. Thanks for fixing this, Wez. Is this CL ready for review? On Tue, May 31, 2011 at 9:52 AM, <wez@chromium.org> wrote: > Alpha: Any thoughts on why SSLServerSocket shouldn't be a StreamSocket as > in > this CL? > > (CL also gets rid of SocketWrapper; please ignore that for now!) > > > http://codereview.chromium.org/7054010/ >
On 2011/05/31 17:01:07, sergeyu wrote: > I think it should be a StreamSocket. StreamSocket was called ClientSocket at > the time SSLServerSocket was implemented - and I think that's why nobody > even considered implementing ClientSocket interface in SSLServerSocket :(. > Thanks for fixing this, Wez. Is this CL ready for review? The SSLServerSocket->StreamSocket are ready. The changes to remove SocketWrapper need more work, so I'll pull them out into a separate CL.
On 2011/05/31 16:52:32, Wez wrote: > Alpha: Any thoughts on why SSLServerSocket shouldn't be a StreamSocket as in > this CL? > > (CL also gets rid of SocketWrapper; please ignore that for now!) Yeah like Sergey said there wasn't a StreamSocket defined before and this file hasn't been changed since the very first patch.
sergeyu: I've stripped this CL down just to the SSLServerSocket->StreamSocket change - PTAL. bulach: I've stripped out the stub implementation of SSLServerSocketOpenSSL, since it's just there so the tree builds with use_openssl=1, and nothing will ever get to calling the stub in that case, I don't think?
LGTM. Just some nits. http://codereview.chromium.org/7054010/diff/13001/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_nss.cc (right): http://codereview.chromium.org/7054010/diff/13001/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.cc:223: nit: remove this empty line. http://codereview.chromium.org/7054010/diff/13001/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_nss.h (right): http://codereview.chromium.org/7054010/diff/13001/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.h:81: void DoAcceptCallback(int result); rename this to DoConnectCallback() http://codereview.chromium.org/7054010/diff/13001/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.h:103: CompletionCallback* user_accept_callback_; user_connect_callback_?
Nits picked. Will wait to commit til @bulach has a chance to sanity-check removal of OpenSSL-based stub. http://codereview.chromium.org/7054010/diff/13001/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_nss.cc (right): http://codereview.chromium.org/7054010/diff/13001/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.cc:223: On 2011/05/31 22:19:49, sergeyu wrote: > nit: remove this empty line. Done. http://codereview.chromium.org/7054010/diff/13001/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_nss.h (right): http://codereview.chromium.org/7054010/diff/13001/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.h:81: void DoAcceptCallback(int result); On 2011/05/31 22:19:49, sergeyu wrote: > rename this to DoConnectCallback() Done. http://codereview.chromium.org/7054010/diff/13001/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.h:103: CompletionCallback* user_accept_callback_; On 2011/05/31 22:19:49, sergeyu wrote: > user_connect_callback_? Done.
+wtc to approve changes in net/
LGTM thanks again! just one nit on the OpenSSL stub to get it linking: http://codereview.chromium.org/7054010/diff/15001/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_openssl.cc (right): http://codereview.chromium.org/7054010/diff/15001/net/socket/ssl_server_socke... net/socket/ssl_server_socket_openssl.cc:13: StreamSocket* CreateSSLServerSocket(Socket* socket, nit: param1 is StreamSocket*
OK, just waiting on @wtc now. http://codereview.chromium.org/7054010/diff/15001/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_openssl.cc (right): http://codereview.chromium.org/7054010/diff/15001/net/socket/ssl_server_socke... net/socket/ssl_server_socket_openssl.cc:13: StreamSocket* CreateSSLServerSocket(Socket* socket, On 2011/06/01 10:57:08, bulach wrote: > nit: param1 is StreamSocket* Ooops - thanks for spotting that.
I will review this CL tomorrow. (I took this afternoon off.)
I added willchan and mbelshe as reviewers. willchan reviewed sergeyu's previous CLs that added ServerSocket and StreamSocket (http://codereview.chromium.org/6820057 and http://codereview.chromium.org/6930014), so he should review this CL and advise on the class hierarchy. My main comment is the first comment below regarding the lack of a SSLServerSocket type; all the other comments are just nits. Fixing the nits and adding a SSLServerSocket type would make me happy, but I'd also like to hear willchan's opinion. http://codereview.chromium.org/7054010/diff/15002/net/socket/ssl_server_socket.h File net/socket/ssl_server_socket.h (left): http://codereview.chromium.org/7054010/diff/15002/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:39: }; It is strange that a header file named ssl_server_socket.h doesn't define the SSLServerSocket class. I would define SSLServerSocket as a typedef or a dummy derived class of StreamSocket. It is also strange for a server socket to use a Connect method, but I know why you do this -- the ServerSocket interface is designed for transport layer and doesn't match how an SSL server socket is created. Perhaps the ServerSocket interface, which has the Listen and Accept methods of a conventional server socket in the BSD socket API, should be renamed TransportServerSocket? Another solution is to have a single SSLSocket or SSLStreamSocket class, with an attribute to indicate whether it should handshake as a server or as a client. http://codereview.chromium.org/7054010/diff/15002/net/socket/ssl_server_socket.h File net/socket/ssl_server_socket.h (right): http://codereview.chromium.org/7054010/diff/15002/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:26: // The SSL StreamSocket takes ownership of |socket|. You should document that this happens even if the function returns NULL. This is what the OpenSSL stub function does. Alternatively, change the OpenSSL stub function to NOT delete 'socket'. http://codereview.chromium.org/7054010/diff/15002/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:31: // The caller starts the SSL connection acceptance protocol by calling Connect Nit: SSL connection acceptance protocol => SSL server handshake http://codereview.chromium.org/7054010/diff/15002/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_nss.h (right): http://codereview.chromium.org/7054010/diff/15002/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.h:44: // StreamSocket calls passed straight to underlying stream. Nit: add "socket" after "stream". Alternatively, you can call it "transport socket". http://codereview.chromium.org/7054010/diff/15002/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.h:55: virtual bool SetSendBufferSize(int32 size); These probably should also be forwarded to the underlying stream socket. http://codereview.chromium.org/7054010/diff/15002/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.h:121: // Socket for sending and receiving data. Nit: Socket => StreamSocket
With regards to the ServerSocket vs StreamSocket naming/function issue, I'm inclined to agree. With the current naming scheme this functionality makes most sense rolled in to SSLClientSocket, with that re-named to SSLStreamSocket. (An SSLServerSocket in that world sits on top of a transport ServerSocket and creates a new server-side SSLStreamSocket instance for each incoming connection.) http://codereview.chromium.org/7054010/diff/15002/net/socket/ssl_server_socket.h File net/socket/ssl_server_socket.h (right): http://codereview.chromium.org/7054010/diff/15002/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:26: // The SSL StreamSocket takes ownership of |socket|. On 2011/06/02 19:48:24, wtc wrote: > You should document that this happens even if the function > returns NULL. This is what the OpenSSL stub function does. > > Alternatively, change the OpenSSL stub function to NOT > delete 'socket'. Done. http://codereview.chromium.org/7054010/diff/15002/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:31: // The caller starts the SSL connection acceptance protocol by calling Connect On 2011/06/02 19:48:24, wtc wrote: > Nit: SSL connection acceptance protocol => SSL server handshake Done. http://codereview.chromium.org/7054010/diff/15002/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_nss.h (right): http://codereview.chromium.org/7054010/diff/15002/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.h:44: // StreamSocket calls passed straight to underlying stream. On 2011/06/02 19:48:24, wtc wrote: > Nit: add "socket" after "stream". Alternatively, you can > call it "transport socket". Done. http://codereview.chromium.org/7054010/diff/15002/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.h:55: virtual bool SetSendBufferSize(int32 size); On 2011/06/02 19:48:24, wtc wrote: > These probably should also be forwarded to the underlying > stream socket. Done. http://codereview.chromium.org/7054010/diff/15002/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.h:121: // Socket for sending and receiving data. On 2011/06/02 19:48:24, wtc wrote: > Nit: Socket => StreamSocket Done.
LGTM. You can check this in before willchan reviews it. Nit: I would shorten the "certificate" arguments to "cert" throughout this CL. Up to you. http://codereview.chromium.org/7054010/diff/18002/net/socket/ssl_server_socket.h File net/socket/ssl_server_socket.h (right): http://codereview.chromium.org/7054010/diff/18002/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:25: // Creates an SSL server-side StreamSocket over analready-connected transport Typo: analready => an already http://codereview.chromium.org/7054010/diff/18002/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_nss.cc (right): http://codereview.chromium.org/7054010/diff/18002/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.cc:79: ssl_config_.tls1_enabled = true; Can you find out why we hardcode the values of these three SSL config settings? I wonder if this is what hclam's TODO comment in the .h file refers to. http://codereview.chromium.org/7054010/diff/18002/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_nss.h (right): http://codereview.chromium.org/7054010/diff/18002/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.h:24: class SSLServerSocketNSS : public StreamSocket { Change this back to SSLServerSocket. http://codereview.chromium.org/7054010/diff/18002/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.h:48: // StreamSocket calls passed straight to the underlying StreamSocket. I think this comment can be deleted, or be moved to the .cc file. http://codereview.chromium.org/7054010/diff/18002/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_unittest.cc (right): http://codereview.chromium.org/7054010/diff/18002/net/socket/ssl_server_socke... net/socket/ssl_server_socket_unittest.cc:272: scoped_ptr<net::StreamSocket> server_socket_; Change this back to SSLServerSocket. http://codereview.chromium.org/7054010/diff/18002/remoting/protocol/jingle_se... File remoting/protocol/jingle_session.cc (right): http://codereview.chromium.org/7054010/diff/18002/remoting/protocol/jingle_se... remoting/protocol/jingle_session.cc:403: net::StreamSocket* socket = net::CreateSSLServerSocket( Change this back to SSLServerSocket.
I'm fine with the overall goal of making SSLServerSocket inherit from StreamSocket. I don't think Connect() makes sense though. I'd be fine with an ERR_UNEXPECTED implementation for Connect() right now. No one should be calling Connect() on SSLServerSocket via the StreamSocket interface. You have the concrete type when you create the server socket, so go ahead and call a non-virtual method. http://codereview.chromium.org/7054010/diff/18002/remoting/protocol/jingle_se... File remoting/protocol/jingle_session.cc (right): http://codereview.chromium.org/7054010/diff/18002/remoting/protocol/jingle_se... remoting/protocol/jingle_session.cc:407: int ret = socket->Connect(&ssl_connect_callback_); I don't believe we should make this change. Use Accept() instead. We may want to consider removing Connect() from the StreamSocket interface and adding a new interface that inherits from StreamSocket that provides the Connect() method. I need to think further. But I'm pretty sure there's no need to call Connect() here. Calling Accept() or whatever you want to call it other than Connect() seems fine to me.
http://codereview.chromium.org/7054010/diff/18002/net/socket/ssl_server_socket.h File net/socket/ssl_server_socket.h (right): http://codereview.chromium.org/7054010/diff/18002/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:25: // Creates an SSL server-side StreamSocket over analready-connected transport On 2011/06/02 23:12:58, wtc wrote: > Typo: analready => an already Done. http://codereview.chromium.org/7054010/diff/18002/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_nss.cc (right): http://codereview.chromium.org/7054010/diff/18002/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.cc:79: ssl_config_.tls1_enabled = true; On 2011/06/02 23:12:58, wtc wrote: > Can you find out why we hardcode the values of these three > SSL config settings? I wonder if this is what hclam's > TODO comment in the .h file refers to. According to hclam, these settings work while other combinations may not; certainly the rest of the code seems to ignore the first two entirely, so I think the intent is that |ssl_config_| reflect the settings that are actually in effect. Am checking this with hclam, though. http://codereview.chromium.org/7054010/diff/18002/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_nss.h (right): http://codereview.chromium.org/7054010/diff/18002/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.h:24: class SSLServerSocketNSS : public StreamSocket { On 2011/06/02 23:12:58, wtc wrote: > Change this back to SSLServerSocket. Done. http://codereview.chromium.org/7054010/diff/18002/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.h:48: // StreamSocket calls passed straight to the underlying StreamSocket. On 2011/06/02 23:12:58, wtc wrote: > I think this comment can be deleted, or be moved to the > .cc file. Done. http://codereview.chromium.org/7054010/diff/18002/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_unittest.cc (right): http://codereview.chromium.org/7054010/diff/18002/net/socket/ssl_server_socke... net/socket/ssl_server_socket_unittest.cc:272: scoped_ptr<net::StreamSocket> server_socket_; On 2011/06/02 23:12:58, wtc wrote: > Change this back to SSLServerSocket. Done. http://codereview.chromium.org/7054010/diff/18002/remoting/protocol/jingle_se... File remoting/protocol/jingle_session.cc (right): http://codereview.chromium.org/7054010/diff/18002/remoting/protocol/jingle_se... remoting/protocol/jingle_session.cc:403: net::StreamSocket* socket = net::CreateSSLServerSocket( On 2011/06/02 23:12:58, wtc wrote: > Change this back to SSLServerSocket. Done. http://codereview.chromium.org/7054010/diff/18002/remoting/protocol/jingle_se... remoting/protocol/jingle_session.cc:407: int ret = socket->Connect(&ssl_connect_callback_); On 2011/06/03 10:23:44, willchan wrote: > I don't believe we should make this change. Use Accept() instead. We may want to > consider removing Connect() from the StreamSocket interface and adding a new > interface that inherits from StreamSocket that provides the Connect() method. I > need to think further. But I'm pretty sure there's no need to call Connect() > here. Calling Accept() or whatever you want to call it other than Connect() > seems fine to me. I chose to re-use Connect() for three reasons: 1. There is no semantic difference between SSLClientSocket::Connect() and what I've called SSLServerSocket::Connect(); both cause the SSL handshake to be performed over the already-connected underlying StreamSocket(*). Giving the two the same name allows more common code between the client and server sides of an application that uses the two. 2. Conceptually the Connect() call makes a StreamSocket "connected" - in the case of SSL that means performing the handshake, which is exactly what this call does. 3. accept() is an operation on a listening socket in the BSD sockets world, and that is the model that net::Socket and net::ServerSocket seem to follow. The only reason I can see for the call previously having been called Accept() is that OpenSSL uses that terminology in places. (*) That said, the semantics of SSLClientSocket::Connect() don't quite match those described for StreamSocket::Connect(), so the equivalence may go away if that gets fixed. The SSLServerSocket class name is confusing, since it implies that this is a net::ServerSocket. Similarly, the Connect() semantics of both SSLClientSocket and SSLServerSocket aren't what a net::StreamSocket caller would expect; splitting the current StreamSocket::Connect() into client-side Connect() and client/server-common Handshake() calls could address that. I'm more than happy to help improve the net::Socket class hierarchy and interfaces to be more consistent, but I don't think this CL is the place to do it. :)
http://codereview.chromium.org/7054010/diff/18002/remoting/protocol/jingle_se... File remoting/protocol/jingle_session.cc (right): http://codereview.chromium.org/7054010/diff/18002/remoting/protocol/jingle_se... remoting/protocol/jingle_session.cc:407: int ret = socket->Connect(&ssl_connect_callback_); On 2011/06/03 17:47:07, Wez wrote: > > (*) That said, the semantics of SSLClientSocket::Connect() don't quite match > those described for StreamSocket::Connect(), so the equivalence may go away if > that gets fixed. > > The SSLServerSocket class name is confusing, since it implies that this is a > net::ServerSocket. Similarly, the Connect() semantics of both SSLClientSocket > and SSLServerSocket aren't what a net::StreamSocket caller would expect; > splitting the current StreamSocket::Connect() into client-side Connect() and > client/server-common Handshake() calls could address that. I agree with willchan's comment and wez's comment here. The difficulty here is what to call the "connection setup" method of a server-side layered socket. Perhaps reserving Connect() and Accept() for transport sockets and adding a Handshake() method, which takes no destination address argument and operates in either client or server mode, for layered sockets is a good solution.
A few comments. http://codereview.chromium.org/7054010/diff/26001/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_nss.cc (right): http://codereview.chromium.org/7054010/diff/26001/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.cc:95: int SSLServerSocketNSS::Connect(CompletionCallback* callback) { I guess you didn't like accept() because its not a BSD-style accept. But connect() is even worse, because it's not a BSD-style connect either (this is a server socket) How about "StartServerHandshake()"? This is more clear. http://codereview.chromium.org/7054010/diff/26001/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_nss.h (right): http://codereview.chromium.org/7054010/diff/26001/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.h:120: scoped_ptr<StreamSocket> transport_socket_; Is this a SSLServerSocket or a StreamSocket? I realize they are typedef'd equivalently.
Some more responses to comments. I've also chatted to willchan@ a little about this, and agreed that a general cleanup of this class hierarchy is desirable. http://codereview.chromium.org/7054010/diff/26001/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_nss.cc (right): http://codereview.chromium.org/7054010/diff/26001/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.cc:95: int SSLServerSocketNSS::Connect(CompletionCallback* callback) { On 2011/06/03 18:34:59, Mike Belshe wrote: > I guess you didn't like accept() because its not a BSD-style accept. Yes, and the BSD sockets model seems to be what net::Socket, StreamSocket & ServerSocket follow. > But > connect() is even worse, because it's not a BSD-style connect either (this is a > server socket) No, it isn't; it's an SSL server-side connection socket. A ServerSocket is what BSD would call a "listening socket", i.e. a point-of-presence to which clients can connect. SSLServerSocket is (confusingly) an SSL server-side layered over any arbitrary underlying StreamSocket. Connect() is correct here in that the work done by the call is the same as that done by SSLClientSocket::Connect(), which is to take the SSL StreamSocket to a usable ("connected") state. Unfortunately SSLClientSocket::Connect() differs in semantics from StreamSocket::Connect(), but I think that's something to deal with in a separate CL. > How about "StartServerHandshake()"? This is more clear. Having a separate call specific to this class just means that calling code needs separate code paths for SSLClientSocket versus SSLServerSocket, despite the fact that it's functionally equivalent to SSLClientSocket's Connect. http://codereview.chromium.org/7054010/diff/26001/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_nss.h (right): http://codereview.chromium.org/7054010/diff/26001/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.h:120: scoped_ptr<StreamSocket> transport_socket_; On 2011/06/03 18:34:59, Mike Belshe wrote: > Is this a SSLServerSocket or a StreamSocket? I realize they are typedef'd > equivalently. This is the underlying StreamSocket, over which SSL is being layered to create an SSL server-side StreamSocket (aka SSLServerSocket).
I've upload two new Patch Sets: Patch Set 8, as requested, defines an SSLServerSocket interface with a StartHandshake method, and makes the Socket-inherited Connect call a not-implemented stub. Patch Set 9 just an FYI, illustrating the (minor) simplification to the calling code in JingleSession made possible by having the SSL handshake process initiated via the same call to both SSL client and server sockets.
I see it in patchset 8, but the new interface is gone in patchset 9, what's the deal? On 2011/06/07 22:29:28, Wez wrote: > I've upload two new Patch Sets: > > Patch Set 8, as requested, defines an SSLServerSocket interface with a > StartHandshake method, and makes the Socket-inherited Connect call a > not-implemented stub. > > Patch Set 9 just an FYI, illustrating the (minor) simplification to the calling > code in JingleSession made possible by having the SSL handshake process > initiated via the same call to both SSL client and server sockets.
On 2011/06/09 14:26:45, willchan wrote: > I see it in patchset 8, but the new interface is gone in patchset 9, what's the > deal? Yeah, forget that patch set for now, I think.
Wez: I am confused by the state of this CL. I don't see the Patch Set 9 that you mentioned. Patch Set 8 seems incomplete. See my comments below. http://codereview.chromium.org/7054010/diff/31001/net/socket/ssl_server_socket.h File net/socket/ssl_server_socket.h (right): http://codereview.chromium.org/7054010/diff/31001/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:28: // when the process completes, or fails. If Disconnect is called on Nit: change "process" to "handshake" to be more specific. Remove ", or fails" because a handshake can complete with a failure. http://codereview.chromium.org/7054010/diff/31001/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:31: virtual int StartHandshake(CompletionCallback* callback) = 0; Just call this method "Handshake" unless "handshake" is not a verb. http://codereview.chromium.org/7054010/diff/31001/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_nss.h (right): http://codereview.chromium.org/7054010/diff/31001/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.h:82: void DoConnectCallback(int result); DoConnectCallback and user_connect_callback_ should be renamed DohandshakeCallback and user_handshake_callback_. http://codereview.chromium.org/7054010/diff/31001/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_unittest.cc (right): http://codereview.chromium.org/7054010/diff/31001/net/socket/ssl_server_socke... net/socket/ssl_server_socket_unittest.cc:296: int server_ret = server_socket_->Connect(&accept_callback); These Connect calls should be changed to StartHandshake calls. Did you compile this code?
http://codereview.chromium.org/7054010/diff/31001/net/socket/ssl_server_socket.h File net/socket/ssl_server_socket.h (right): http://codereview.chromium.org/7054010/diff/31001/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:28: // when the process completes, or fails. If Disconnect is called on On 2011/06/10 22:17:35, wtc wrote: > Nit: change "process" to "handshake" to be more specific. > Remove ", or fails" because a handshake can complete with > a failure. Reworded this anyway. http://codereview.chromium.org/7054010/diff/31001/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:31: virtual int StartHandshake(CompletionCallback* callback) = 0; On 2011/06/10 22:17:35, wtc wrote: > Just call this method "Handshake" unless "handshake" is not > a verb. Done. http://codereview.chromium.org/7054010/diff/31001/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_nss.h (right): http://codereview.chromium.org/7054010/diff/31001/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.h:82: void DoConnectCallback(int result); On 2011/06/10 22:17:35, wtc wrote: > DoConnectCallback and user_connect_callback_ should be > renamed DohandshakeCallback and user_handshake_callback_. Done. http://codereview.chromium.org/7054010/diff/31001/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_unittest.cc (right): http://codereview.chromium.org/7054010/diff/31001/net/socket/ssl_server_socke... net/socket/ssl_server_socket_unittest.cc:296: int server_ret = server_socket_->Connect(&accept_callback); On 2011/06/10 22:17:35, wtc wrote: > These Connect calls should be changed to StartHandshake calls. > Did you compile this code? Yes. SSLServerSocket has a Connect method inherited from StreamSocket. The problem is that I must not have re-run net_unittests after the re-name. :(
Wez: just wanted to doublecheck -- do you want us to review Patch Set 9? In a previous comment, you said (without uploading Patch Set 9 at that time): I've upload two new Patch Sets: Patch Set 8, as requested, defines an SSLServerSocket interface with a StartHandshake method, and makes the Socket-inherited Connect call a not-implemented stub. Patch Set 9 just an FYI, illustrating the (minor) simplification to the calling code in JingleSession made possible by having the SSL handshake process initiated via the same call to both SSL client and server sockets. So I don't know if the newly uploaded Patch Set 9 is still just an FYI.
Please ignore the previous comments about a Patchset 9, and yes, please look at the Patchset 9 you see listed now, which is the latest state of what we discussed (renaming the start-handshake API to Handshake(), etc). Sorry for the confusion! Wez On 14 June 2011 10:26, <wtc@chromium.org> wrote: > Wez: just wanted to doublecheck -- do you want us to review > Patch Set 9? > > In a previous comment, you said (without uploading Patch Set > 9 at that time): > > > I've upload two new Patch Sets: > > Patch Set 8, as requested, defines an SSLServerSocket > interface with a StartHandshake method, and makes the > Socket-inherited Connect call a not-implemented stub. > > Patch Set 9 just an FYI, illustrating the (minor) > simplification to the calling code in JingleSession made > possible by having the SSL handshake process initiated > via the same call to both SSL client and server sockets. > > So I don't know if the newly uploaded Patch Set 9 is still > just an FYI. > > > http://codereview.chromium.**org/7054010/<http://codereview.chromium.org/7054... >
I took a quick look and it all LGTM. wtc: please doublecheck that everything is fine with you. On 2011/06/15 03:47:59, Wez wrote: > Please ignore the previous comments about a Patchset 9, and yes, please look > at the Patchset 9 you see listed now, which is the latest state of what we > discussed (renaming the start-handshake API to Handshake(), etc). > > Sorry for the confusion! > > Wez > > > > On 14 June 2011 10:26, <mailto:wtc@chromium.org> wrote: > > > Wez: just wanted to doublecheck -- do you want us to review > > Patch Set 9? > > > > In a previous comment, you said (without uploading Patch Set > > 9 at that time): > > > > > > I've upload two new Patch Sets: > > > > Patch Set 8, as requested, defines an SSLServerSocket > > interface with a StartHandshake method, and makes the > > Socket-inherited Connect call a not-implemented stub. > > > > Patch Set 9 just an FYI, illustrating the (minor) > > simplification to the calling code in JingleSession made > > possible by having the SSL handshake process initiated > > via the same call to both SSL client and server sockets. > > > > So I don't know if the newly uploaded Patch Set 9 is still > > just an FYI. > > > > > > > http://codereview.chromium.**org/7054010/%3Chttp://codereview.chromium.org/70...> > >
LGTM. Please fix as many of the problems I listed below as you can, and check this in. It is easier to review a new CL for the delta. http://codereview.chromium.org/7054010/diff/32009/net/socket/ssl_server_socket.h File net/socket/ssl_server_socket.h (right): http://codereview.chromium.org/7054010/diff/32009/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:23: class SSLServerSocket : public net::StreamSocket { Remove net:: because this is in the net namespace. http://codereview.chromium.org/7054010/diff/32009/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:30: // dropped in the same way as for other StreamSocket calls. Nit: you still distinguish between completion and failure, but failure is also a form of completion -- we call CompletionCallback when the operation completes either successfully or with an error. http://codereview.chromium.org/7054010/diff/32009/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:34: // Creates an SSL server-side StreamSocket over an already-connected transport I would just say "an SSL server socket" here. http://codereview.chromium.org/7054010/diff/32009/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:44: // The caller starts the SSL server handshake by calling StartHandshake on the Typo: StartHandshake => Handshake http://codereview.chromium.org/7054010/diff/32009/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_nss.cc (right): http://codereview.chromium.org/7054010/diff/32009/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.cc:96: net_log_.BeginEvent(NetLog::TYPE_SSL_ACCEPT, NULL); NetLog::TYPE_SSL_ACCEPT probably should be renamed NetLog::TYPE_SSL_HANDSHAKE or NetLog::TYPE_SSL_SERVER_HANDSHAKE. http://codereview.chromium.org/7054010/diff/32009/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_unittest.cc (right): http://codereview.chromium.org/7054010/diff/32009/net/socket/ssl_server_socke... net/socket/ssl_server_socket_unittest.cc:287: // This test executes Connect() on SSLClientSocket and Handshake() on Important: SSLClientSocket should also be changed to use the Handshake() method for SSL handshake, and the Connect() method should also fail with ERR_NOT_IMPLEMENTED.
Have addressed most of wtc's comments, will follow up with the Connect->Handshake change for SSLClientSocket in a separate CL. Thanks for everyone's patience with this one... http://codereview.chromium.org/7054010/diff/32009/net/socket/ssl_server_socket.h File net/socket/ssl_server_socket.h (right): http://codereview.chromium.org/7054010/diff/32009/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:23: class SSLServerSocket : public net::StreamSocket { On 2011/06/16 21:35:07, wtc wrote: > Remove net:: because this is in the net namespace. Done. http://codereview.chromium.org/7054010/diff/32009/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:30: // dropped in the same way as for other StreamSocket calls. On 2011/06/16 21:35:07, wtc wrote: > Nit: you still distinguish between completion and failure, > but failure is also a form of completion -- we call > CompletionCallback when the operation completes either > successfully or with an error. Done. http://codereview.chromium.org/7054010/diff/32009/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:34: // Creates an SSL server-side StreamSocket over an already-connected transport On 2011/06/16 21:35:07, wtc wrote: > I would just say "an SSL server socket" here. Done. http://codereview.chromium.org/7054010/diff/32009/net/socket/ssl_server_socke... net/socket/ssl_server_socket.h:44: // The caller starts the SSL server handshake by calling StartHandshake on the On 2011/06/16 21:35:07, wtc wrote: > Typo: StartHandshake => Handshake Done. http://codereview.chromium.org/7054010/diff/32009/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_nss.cc (right): http://codereview.chromium.org/7054010/diff/32009/net/socket/ssl_server_socke... net/socket/ssl_server_socket_nss.cc:96: net_log_.BeginEvent(NetLog::TYPE_SSL_ACCEPT, NULL); On 2011/06/16 21:35:07, wtc wrote: > NetLog::TYPE_SSL_ACCEPT probably should be renamed > NetLog::TYPE_SSL_HANDSHAKE or NetLog::TYPE_SSL_SERVER_HANDSHAKE. Done. I've renamed to SSL_SERVER_HANDSHAKE, and updated the comment. I've also updated the comment on SSL_CONNECT, and will re-name that in the follow-up CL. http://codereview.chromium.org/7054010/diff/32009/net/socket/ssl_server_socke... File net/socket/ssl_server_socket_unittest.cc (right): http://codereview.chromium.org/7054010/diff/32009/net/socket/ssl_server_socke... net/socket/ssl_server_socket_unittest.cc:287: // This test executes Connect() on SSLClientSocket and Handshake() on On 2011/06/16 21:35:07, wtc wrote: > Important: SSLClientSocket should also be changed to use > the Handshake() method for SSL handshake, and the Connect() > method should also fail with ERR_NOT_IMPLEMENTED. Deferred to a follow-up CL.
Change committed as 89535 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
