Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3495005:
Prototype OpenSSL client socket implementation. (Closed)
DescriptionFirst step at OpenSSL client socket implementation.
This is early in-progress implementation, no cert handling supported. So only available under a build-time flag. (GYP_DEFINES="'use_openssl=1'")
Adds a new build dependency for system OpenSSL libraries, and a new USE_OPENSSL define. Eventually this will disable USE_NSS but for now the two coexist.
BUG=none
TEST=build with use_openssl=1. Goto some https:// pages.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=60936
Patch Set 1 #Patch Set 2 : comments update #
Total comments: 14
Patch Set 3 : agl comments #Patch Set 4 : error handling improvements #
Total comments: 28
Patch Set 5 : bulach comments #Patch Set 6 : fix checkdeps #
Total comments: 10
Patch Set 7 : agl comments no. 2 #
Total comments: 21
Messages
Total messages: 12 (0 generated)
Just a quick review. The use of memory BIOs leads me to suspect that the entire history of the connection will be buffered by them, but maybe you're clearing them somewhere that I'm missing. http://codereview.chromium.org/3495005/diff/2001/3004 File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/3495005/diff/2001/3004#newcode30 net/socket/ssl_client_socket_openssl.cc:30: if (!SSL_library_init()) SSL_library_init returns 1, no matter what, so the condition is superfluous, although harmless. http://codereview.chromium.org/3495005/diff/2001/3004#newcode33 net/socket/ssl_client_socket_openssl.cc:33: SSL_METHOD* pMethod = SSLv23_client_method(); IMPORTANT: SSLv23_client_method uses SSLv2 handshakes, which we cannot allow. I think you'll need two different methods. A TLSv1_client_method should be used most of the time (and you can develop with that). In the end you'll also need an SSLv3_client_method in order to handle a small fraction of servers, but the higher layers will tell you when that's the case (by clearing ssl_config_.tls1_enabled). SSLv2 should never be enabled or used for anything. http://codereview.chromium.org/3495005/diff/2001/3004#newcode94 net/socket/ssl_client_socket_openssl.cc:94: delete buffer_send_callback_; scoped_ptrs... http://codereview.chromium.org/3495005/diff/2001/3004#newcode111 net/socket/ssl_client_socket_openssl.cc:111: ssl_info->security_bits = 1024; This is the security level, not the size of the multiplicative group. (i.e. pick 128 as a random example.) http://codereview.chromium.org/3495005/diff/2001/3004#newcode131 net/socket/ssl_client_socket_openssl.cc:131: return kNextProtoUnsupported; You need to call proto->clear() http://codereview.chromium.org/3495005/diff/2001/3004#newcode160 net/socket/ssl_client_socket_openssl.cc:160: return -1; // Anything but OK signals error the comment is correct, but the errors mean something. -1 is ERR_IO_PENDING for example, so you should use a name. See net/base/net_error_list.h http://codereview.chromium.org/3495005/diff/2001/3004#newcode165 net/socket/ssl_client_socket_openssl.cc:165: return -1; ditto
Thanks for the comments Adam, I've fixed them all plus made a first cut at sorting out error handling and factoring out duplicated code. Regarding the BIOs, I couldn't quite work out how I would flush buffer contents in the previous design, but I'm now following the canonical example -- creating a joined BIO pair rather than configuring SSL to read and write from independent BIO -- so hopefully this will address your concern. Certainly, I can't find any evidence it is buffering the stream indefinitely. Hopefully this is approaching the point I can make an initial check in (Given it is under a build flag). Getting an initial patch landed will help allow me & Marcus to work on the remaining parts of it in parallel. Cheers Joth http://codereview.chromium.org/3495005/diff/2001/3004 File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/3495005/diff/2001/3004#newcode30 net/socket/ssl_client_socket_openssl.cc:30: if (!SSL_library_init()) On 2010/09/24 18:48:02, agl wrote: > SSL_library_init returns 1, no matter what, so the condition is superfluous, > although harmless. Done. http://codereview.chromium.org/3495005/diff/2001/3004#newcode33 net/socket/ssl_client_socket_openssl.cc:33: SSL_METHOD* pMethod = SSLv23_client_method(); On 2010/09/24 18:48:02, agl wrote: > IMPORTANT: SSLv23_client_method uses SSLv2 handshakes, which we cannot allow. > > I think you'll need two different methods. A TLSv1_client_method should be used > most of the time (and you can develop with that). In the end you'll also need an > SSLv3_client_method in order to handle a small fraction of servers, but the > higher layers will tell you when that's the case (by clearing > ssl_config_.tls1_enabled). > > SSLv2 should never be enabled or used for anything. Thanks for making me aware of this. Presumably this means we can't use v2 for negotiation either? (from the docs, I guess this is no good for us?) The list of protocols available can later be limited using the SSL_OP_NO_SSLv2, SSL_OP_NO_SSLv3, SSL_OP_NO_TLSv1 options of the SSL_CTX_set_options() or SSL_set_options() functions. Using these options it is possible to choose e.g. SSLv23_server_method() and be able to negotiate with all possible clients, but to only allow newer protocols like SSLv3 or TLSv1. http://codereview.chromium.org/3495005/diff/2001/3004#newcode94 net/socket/ssl_client_socket_openssl.cc:94: delete buffer_send_callback_; On 2010/09/24 18:48:02, agl wrote: > scoped_ptrs... Done. (removed ptrs) http://codereview.chromium.org/3495005/diff/2001/3004#newcode111 net/socket/ssl_client_socket_openssl.cc:111: ssl_info->security_bits = 1024; On 2010/09/24 18:48:02, agl wrote: > This is the security level, not the size of the multiplicative group. (i.e. pick > 128 as a random example.) Done. http://codereview.chromium.org/3495005/diff/2001/3004#newcode131 net/socket/ssl_client_socket_openssl.cc:131: return kNextProtoUnsupported; On 2010/09/24 18:48:02, agl wrote: > You need to call proto->clear() Done. http://codereview.chromium.org/3495005/diff/2001/3004#newcode160 net/socket/ssl_client_socket_openssl.cc:160: return -1; // Anything but OK signals error On 2010/09/24 18:48:02, agl wrote: > the comment is correct, but the errors mean something. -1 is ERR_IO_PENDING for > example, so you should use a name. See net/base/net_error_list.h Done. http://codereview.chromium.org/3495005/diff/2001/3004#newcode165 net/socket/ssl_client_socket_openssl.cc:165: return -1; On 2010/09/24 18:48:02, agl wrote: > ditto Done.
On 27 September 2010 17:34, <joth@chromium.org> wrote: > Reviewers: agl, > > Message: > Thanks for the comments Adam, I've fixed them all plus made a first cut at > sorting out error handling and factoring out duplicated code. > > Regarding the BIOs, I couldn't quite work out how I would flush buffer > contents > in the previous design, but I'm now following the canonical example -- > creating > a joined BIO pair rather than configuring SSL to read and write from > independent > BIO -- so hopefully this will address your concern. Certainly, I can't find > any > evidence it is buffering the stream indefinitely. > > Hopefully this is approaching the point I can make an initial check in > (Given it > is under a build flag). Getting an initial patch landed will help allow me > & > Marcus to work on the remaining parts of it in parallel. > > Cheers > Joth > > > > > http://codereview.chromium.org/3495005/diff/2001/3004 > File net/socket/ssl_client_socket_openssl.cc (right): > > http://codereview.chromium.org/3495005/diff/2001/3004#newcode30 > net/socket/ssl_client_socket_openssl.cc:30: if (!SSL_library_init()) > On 2010/09/24 18:48:02, agl wrote: > >> SSL_library_init returns 1, no matter what, so the condition is >> > superfluous, > >> although harmless. >> > > Done. > > > http://codereview.chromium.org/3495005/diff/2001/3004#newcode33 > net/socket/ssl_client_socket_openssl.cc:33: SSL_METHOD* pMethod = > SSLv23_client_method(); > On 2010/09/24 18:48:02, agl wrote: > >> IMPORTANT: SSLv23_client_method uses SSLv2 handshakes, which we cannot >> > allow. > > I think you'll need two different methods. A TLSv1_client_method >> > should be used > >> most of the time (and you can develop with that). In the end you'll >> > also need an > >> SSLv3_client_method in order to handle a small fraction of servers, >> > but the > >> higher layers will tell you when that's the case (by clearing >> ssl_config_.tls1_enabled). >> > > SSLv2 should never be enabled or used for anything. >> > > Thanks for making me aware of this. Presumably this means we can't use > v2 for negotiation either? > (from the docs, I guess this is no good for us?) > The list of protocols available can later be limited using the > SSL_OP_NO_SSLv2, SSL_OP_NO_SSLv3, SSL_OP_NO_TLSv1 options of the > SSL_CTX_set_options() or SSL_set_options() functions. Using these > options it is possible to choose e.g. SSLv23_server_method() and be able > to negotiate with all possible clients, but to only allow newer > protocols like SSLv3 or TLSv1. > > ... just to clarify this question, can I do it like the flip server does? http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/net/tools/fli... > http://codereview.chromium.org/3495005/diff/2001/3004#newcode94 > net/socket/ssl_client_socket_openssl.cc:94: delete > buffer_send_callback_; > On 2010/09/24 18:48:02, agl wrote: > >> scoped_ptrs... >> > > Done. > (removed ptrs) > > > http://codereview.chromium.org/3495005/diff/2001/3004#newcode111 > net/socket/ssl_client_socket_openssl.cc:111: ssl_info->security_bits = > 1024; > On 2010/09/24 18:48:02, agl wrote: > >> This is the security level, not the size of the multiplicative group. >> > (i.e. pick > >> 128 as a random example.) >> > > Done. > > > http://codereview.chromium.org/3495005/diff/2001/3004#newcode131 > net/socket/ssl_client_socket_openssl.cc:131: return > kNextProtoUnsupported; > On 2010/09/24 18:48:02, agl wrote: > >> You need to call proto->clear() >> > > Done. > > > http://codereview.chromium.org/3495005/diff/2001/3004#newcode160 > net/socket/ssl_client_socket_openssl.cc:160: return -1; // Anything but > OK signals error > On 2010/09/24 18:48:02, agl wrote: > >> the comment is correct, but the errors mean something. -1 is >> > ERR_IO_PENDING for > >> example, so you should use a name. See net/base/net_error_list.h >> > > Done. > > > http://codereview.chromium.org/3495005/diff/2001/3004#newcode165 > net/socket/ssl_client_socket_openssl.cc:165: return -1; > On 2010/09/24 18:48:02, agl wrote: > >> ditto >> > > Done. > > Description: > > Prototype OpenSSL client socket implementation. > > Very prototype code, no cert handling supported. > > Adds a new build dependency for system OpenSSL libraries, and a new > USE_OPENSSL > define. Eventually this will disable USE_NSS but for now the two coexist. > > BUG=none > TEST=build with use_openssl=1. Goto some https:// pages. > > Please review this at http://codereview.chromium.org/3495005/show > > Affected files: > M build/linux/system.gyp > M net/net.gyp > M net/socket/client_socket_factory.cc > A net/socket/ssl_client_socket_openssl.h > > A net/socket/ssl_client_socket_openssl.cc > > >
On Mon, Sep 27, 2010 at 12:34 PM, <joth@chromium.org> wrote: > Message: > Thanks for the comments Adam, I've fixed them all plus made a first cut at > sorting out error handling and factoring out duplicated code. (Backed up. Hopefully will manage this review today.) AGL
I can't review (yet) the actual implementation, agl is definitely the expert here. some nits / suggestions anyway: http://codereview.chromium.org/3495005/diff/9001/10002 File net/net.gyp (right): http://codereview.chromium.org/3495005/diff/9001/10002#newcode543 net/net.gyp:543: 'socket/ssl_client_socket_openssl.cc', hmm, this failed on the mac bot, but I guess you can remove both without adding a factory? ERROR in /b/slave/mac/build/src/net/socket/ssl_client_socket_openssl.h Illegal include: "openssl/ssl.h" Because of no rule applying http://codereview.chromium.org/3495005/diff/9001/10004 File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/3495005/diff/9001/10004#newcode20 net/socket/ssl_client_socket_openssl.cc:20: #define GotoState(s) do { LOG(INFO) << (void *)this << " " << __FUNCTION__ << \ "Goto" feels like it'd do more things to change the state, perhaps just SetState instead? also, you may consider a private method SetState(s, tracked_objects::Location) so that callers could just use the "FROM_HERE" macro and avoid this one? http://codereview.chromium.org/3495005/diff/9001/10004#newcode32 net/socket/ssl_client_socket_openssl.cc:32: << ERR_error_string(error_num, buf); ERR_error_string_n(error_num, buf, sizeof(buf)) http://codereview.chromium.org/3495005/diff/9001/10004#newcode57 net/socket/ssl_client_socket_openssl.cc:57: buffer_recv_callback_(this, &SSLClientSocketOpenSSL::BufferRecvComplete), ALLOW_THIS_IN_INITIALIZER_LIST http://codereview.chromium.org/3495005/diff/9001/10004#newcode82 net/socket/ssl_client_socket_openssl.cc:82: if(g_ctx) { nit: s/if(/if (/ http://codereview.chromium.org/3495005/diff/9001/10004#newcode91 net/socket/ssl_client_socket_openssl.cc:91: SSL_METHOD* pMethod = TLSv1_client_method(); s/pMethod/method/ http://codereview.chromium.org/3495005/diff/9001/10004#newcode146 net/socket/ssl_client_socket_openssl.cc:146: (((int)TLS1_CK_RSA_EXPORT1024_WITH_DES_CBC_SHA) & s/(int)/static_cast/ http://codereview.chromium.org/3495005/diff/9001/10004#newcode194 net/socket/ssl_client_socket_openssl.cc:194: if(!InitOpenSSL()) { s/if(/if (/ http://codereview.chromium.org/3495005/diff/9001/10004#newcode200 net/socket/ssl_client_socket_openssl.cc:200: if(!Init()) { s/if(/if (/ http://codereview.chromium.org/3495005/diff/9001/10004#newcode224 net/socket/ssl_client_socket_openssl.cc:224: //verifier_.reset(); ? http://codereview.chromium.org/3495005/diff/9001/10004#newcode250 net/socket/ssl_client_socket_openssl.cc:250: // rv = DoVerifyCert(rv); I think the preferred way is to have #if 0? http://codereview.chromium.org/3495005/diff/9001/10005 File net/socket/ssl_client_socket_openssl.h (right): http://codereview.chromium.org/3495005/diff/9001/10005#newcode33 net/socket/ssl_client_socket_openssl.h:33: const SSLConfig& ssl_config); nit: align http://codereview.chromium.org/3495005/diff/9001/10005#newcode81 net/socket/ssl_client_socket_openssl.h:81: int BufferRecv(void); nit: remove s/void// http://codereview.chromium.org/3495005/diff/9001/10005#newcode108 net/socket/ssl_client_socket_openssl.h:108: std::vector<scoped_refptr<X509Certificate> > client_certs_; base/ScopedVector?
Thanks! all points addressed or responded to... http://codereview.chromium.org/3495005/diff/9001/10002 File net/net.gyp (right): http://codereview.chromium.org/3495005/diff/9001/10002#newcode543 net/net.gyp:543: 'socket/ssl_client_socket_openssl.cc', On 2010/09/27 17:23:16, bulach wrote: > hmm, this failed on the mac bot, but I guess you can remove both without adding > a factory? > > ERROR in /b/slave/mac/build/src/net/socket/ssl_client_socket_openssl.h > Illegal include: "openssl/ssl.h" > Because of no rule applying > Yeah, it's a checkdeps failure, I fixed that in the code now (was using #include "" rather than <> style) http://codereview.chromium.org/3495005/diff/9001/10004 File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/3495005/diff/9001/10004#newcode20 net/socket/ssl_client_socket_openssl.cc:20: #define GotoState(s) do { LOG(INFO) << (void *)this << " " << __FUNCTION__ << \ On 2010/09/27 17:23:16, bulach wrote: > "Goto" feels like it'd do more things to change the state, perhaps just SetState > instead? > > also, you may consider a private method > SetState(s, tracked_objects::Location) so that callers could just use the > "FROM_HERE" macro and avoid this one? Agree, but I'm tempted to leave as is for now to avoid hiding the commonality with the NSS version. I'll add a big comment up top. http://codereview.chromium.org/3495005/diff/9001/10004#newcode32 net/socket/ssl_client_socket_openssl.cc:32: << ERR_error_string(error_num, buf); On 2010/09/27 17:23:16, bulach wrote: > ERR_error_string_n(error_num, buf, sizeof(buf)) Done. http://codereview.chromium.org/3495005/diff/9001/10004#newcode57 net/socket/ssl_client_socket_openssl.cc:57: buffer_recv_callback_(this, &SSLClientSocketOpenSSL::BufferRecvComplete), On 2010/09/27 17:23:16, bulach wrote: > ALLOW_THIS_IN_INITIALIZER_LIST Done. http://codereview.chromium.org/3495005/diff/9001/10004#newcode82 net/socket/ssl_client_socket_openssl.cc:82: if(g_ctx) { On 2010/09/27 17:23:16, bulach wrote: > nit: > s/if(/if (/ Done. http://codereview.chromium.org/3495005/diff/9001/10004#newcode91 net/socket/ssl_client_socket_openssl.cc:91: SSL_METHOD* pMethod = TLSv1_client_method(); On 2010/09/27 17:23:16, bulach wrote: > s/pMethod/method/ Done. (removed it) http://codereview.chromium.org/3495005/diff/9001/10004#newcode146 net/socket/ssl_client_socket_openssl.cc:146: (((int)TLS1_CK_RSA_EXPORT1024_WITH_DES_CBC_SHA) & On 2010/09/27 17:23:16, bulach wrote: > s/(int)/static_cast/ Done. (removed) http://codereview.chromium.org/3495005/diff/9001/10004#newcode194 net/socket/ssl_client_socket_openssl.cc:194: if(!InitOpenSSL()) { On 2010/09/27 17:23:16, bulach wrote: > s/if(/if (/ Done. http://codereview.chromium.org/3495005/diff/9001/10004#newcode200 net/socket/ssl_client_socket_openssl.cc:200: if(!Init()) { On 2010/09/27 17:23:16, bulach wrote: > s/if(/if (/ Done. http://codereview.chromium.org/3495005/diff/9001/10004#newcode224 net/socket/ssl_client_socket_openssl.cc:224: //verifier_.reset(); On 2010/09/27 17:23:16, bulach wrote: > ? cleaned this up. http://codereview.chromium.org/3495005/diff/9001/10004#newcode250 net/socket/ssl_client_socket_openssl.cc:250: // rv = DoVerifyCert(rv); On 2010/09/27 17:23:16, bulach wrote: > I think the preferred way is to have #if 0? Done. http://codereview.chromium.org/3495005/diff/9001/10005 File net/socket/ssl_client_socket_openssl.h (right): http://codereview.chromium.org/3495005/diff/9001/10005#newcode33 net/socket/ssl_client_socket_openssl.h:33: const SSLConfig& ssl_config); On 2010/09/27 17:23:16, bulach wrote: > nit: align Done. http://codereview.chromium.org/3495005/diff/9001/10005#newcode81 net/socket/ssl_client_socket_openssl.h:81: int BufferRecv(void); On 2010/09/27 17:23:16, bulach wrote: > nit: remove s/void// Done. http://codereview.chromium.org/3495005/diff/9001/10005#newcode108 net/socket/ssl_client_socket_openssl.h:108: std::vector<scoped_refptr<X509Certificate> > client_certs_; On 2010/09/27 17:23:16, bulach wrote: > base/ScopedVector? doesn't work for refptr. (conversely, ScopedVector only exists because vector<scoped_ptr> doesn't work due to no copy c'tor)
Obviously incomplete, but LGTM so far. http://codereview.chromium.org/3495005/diff/21001/22002 File build/linux/system.gyp (right): http://codereview.chromium.org/3495005/diff/21001/22002#newcode339 build/linux/system.gyp:339: '-lcrypto -lssl', Why use pkgconfig for --cflags but not for --libs? http://codereview.chromium.org/3495005/diff/21001/22005 File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/3495005/diff/21001/22005#newcode28 net/socket/ssl_client_socket_openssl.cc:28: void LogSslError() { LogSSLError (Later. I see that it's used pessimistically later on in this code. In which case I might call it MaybeLogSSLError.) http://codereview.chromium.org/3495005/diff/21001/22005#newcode203 net/socket/ssl_client_socket_openssl.cc:203: // Set SLL to client mode. Handshake happens in the loop below. typo: SSL http://codereview.chromium.org/3495005/diff/21001/22005#newcode525 net/socket/ssl_client_socket_openssl.cc:525: if (rv == ERR_IO_PENDING) If you're going to { } around the else, you should also include braces around the if. http://codereview.chromium.org/3495005/diff/21001/22006 File net/socket/ssl_client_socket_openssl.h (right): http://codereview.chromium.org/3495005/diff/21001/22006#newcode9 net/socket/ssl_client_socket_openssl.h:9: #include <openssl/ssl.h> We really don't want to pull OpenSSL's headers in here. Hopefully you can forward declare them.
Thanks, all comments addressed. Just to check, was that an "LGTM" to go ahead and land? As mentioned, implementing the remaining bits will be much cleaner process once this initial patch is landed (and I'm reluctant to pull the bunch of new files to implement certs into this CL) http://codereview.chromium.org/3495005/diff/21001/22002 File build/linux/system.gyp (right): http://codereview.chromium.org/3495005/diff/21001/22002#newcode339 build/linux/system.gyp:339: '-lcrypto -lssl', On 2010/09/27 22:00:46, agl wrote: > Why use pkgconfig for --cflags but not for --libs? Done. (I got so distracted working out why "pkg-config --cflags openssl" yields an empty line, I clearly forgot to complete the job in hand!) http://codereview.chromium.org/3495005/diff/21001/22005 File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/3495005/diff/21001/22005#newcode28 net/socket/ssl_client_socket_openssl.cc:28: void LogSslError() { On 2010/09/27 22:00:46, agl wrote: > LogSSLError > > (Later. I see that it's used pessimistically later on in this code. In which > case I might call it MaybeLogSSLError.) Done. http://codereview.chromium.org/3495005/diff/21001/22005#newcode203 net/socket/ssl_client_socket_openssl.cc:203: // Set SLL to client mode. Handshake happens in the loop below. On 2010/09/27 22:00:46, agl wrote: > typo: SSL Done. http://codereview.chromium.org/3495005/diff/21001/22005#newcode525 net/socket/ssl_client_socket_openssl.cc:525: if (rv == ERR_IO_PENDING) On 2010/09/27 22:00:46, agl wrote: > If you're going to { } around the else, you should also include braces around > the if. Done. http://codereview.chromium.org/3495005/diff/21001/22006 File net/socket/ssl_client_socket_openssl.h (right): http://codereview.chromium.org/3495005/diff/21001/22006#newcode9 net/socket/ssl_client_socket_openssl.h:9: #include <openssl/ssl.h> On 2010/09/27 22:00:46, agl wrote: > We really don't want to pull OpenSSL's headers in here. Hopefully you can > forward declare them. Yes. I left it as this is the style used in the NSS version, but it is indeed easy enough to avoid. Done.
LGTM On Sep 28, 2010 9:05 AM, <joth@chromium.org> wrote: > Thanks, all comments addressed. Just to check, was that an "LGTM" to go > ahead > and land? > As mentioned, implementing the remaining bits will be much cleaner process > once > this initial patch is landed (and I'm reluctant to pull the bunch of new > files > to implement certs into this CL) > > > http://codereview.chromium.org/3495005/diff/21001/22002 > File build/linux/system.gyp (right): > > http://codereview.chromium.org/3495005/diff/21001/22002#newcode339 > build/linux/system.gyp:339: '-lcrypto -lssl', > On 2010/09/27 22:00:46, agl wrote: >> Why use pkgconfig for --cflags but not for --libs? > > Done. (I got so distracted working out why "pkg-config --cflags openssl" > yields an empty line, I clearly forgot to complete the job in hand!) > > http://codereview.chromium.org/3495005/diff/21001/22005 > File net/socket/ssl_client_socket_openssl.cc (right): > > http://codereview.chromium.org/3495005/diff/21001/22005#newcode28 > net/socket/ssl_client_socket_openssl.cc:28: void LogSslError() { > On 2010/09/27 22:00:46, agl wrote: >> LogSSLError > >> (Later. I see that it's used pessimistically later on in this code. In > which >> case I might call it MaybeLogSSLError.) > > Done. > > http://codereview.chromium.org/3495005/diff/21001/22005#newcode203 > net/socket/ssl_client_socket_openssl.cc:203: // Set SLL to client mode. > Handshake happens in the loop below. > On 2010/09/27 22:00:46, agl wrote: >> typo: SSL > > Done. > > http://codereview.chromium.org/3495005/diff/21001/22005#newcode525 > net/socket/ssl_client_socket_openssl.cc:525: if (rv == ERR_IO_PENDING) > On 2010/09/27 22:00:46, agl wrote: >> If you're going to { } around the else, you should also include braces > around >> the if. > > Done. > > http://codereview.chromium.org/3495005/diff/21001/22006 > File net/socket/ssl_client_socket_openssl.h (right): > > http://codereview.chromium.org/3495005/diff/21001/22006#newcode9 > net/socket/ssl_client_socket_openssl.h:9: #include <openssl/ssl.h> > On 2010/09/27 22:00:46, agl wrote: >> We really don't want to pull OpenSSL's headers in here. Hopefully you > can >> forward declare them. > > Yes. I left it as this is the style used in the NSS version, but it is > indeed easy enough to avoid. Done. > > http://codereview.chromium.org/3495005/show
LGTM. I have some suggested changes below. (Note especially the comment marked with "BUG".) You can fix them in a separate CL if you don't want this code review to drag on for too long. I didn't review ssl_client_socket_openssl.cc carefully. I mainly checked coding style and whatever minor issues I happened to notice as I skimmed through the file. http://codereview.chromium.org/3495005/diff/27001/28002 File build/linux/system.gyp (right): http://codereview.chromium.org/3495005/diff/27001/28002#newcode331 build/linux/system.gyp:331: 'USE_OPENSSL', You should document that eventually USE_OPENSSL and USE_NSS will be mutually exclusive (only one of the macros should be defined). During the transitional period, a use_openssl=1 build still needs to define USE_NSS, so it is necessary to test the USE_OPENSSL macro before testing USE_NSS. (net/socket/client_socket_factory.cc is an example.) http://codereview.chromium.org/3495005/diff/27001/28005 File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/3495005/diff/27001/28005#newcode22 net/socket/ssl_client_socket_openssl.cc:22: // Enable this to see logging for state machine state transitions. Nit: this comment says "Enable this", but the ifdef is already #if 1. It would be better to reverse the order like this: #if 0 #define GotoState(s) do { LOG(INFO) << (void *)this << " " << __FUNCTION__ << \ ... #else #define GotoState(s) next_handshake_state_ = s #endif http://codereview.chromium.org/3495005/diff/27001/28005#newcode50 net/socket/ssl_client_socket_openssl.cc:50: return err < 0 ? ERR_SSL_PROTOCOL_ERROR : err; BUG: This error-mapping function should never return an OpenSSL error unmapped. It does that when err >= 0. http://codereview.chromium.org/3495005/diff/27001/28005#newcode56 net/socket/ssl_client_socket_openssl.cc:56: SSL_CTX* SSLClientSocketOpenSSL::g_ctx = NULL; Nit: we usually add a comment // static before static member initializers. http://codereview.chromium.org/3495005/diff/27001/28005#newcode86 net/socket/ssl_client_socket_openssl.cc:86: if (g_ctx) { Nit: in the net module we usually omit curly braces {} around a simple one-line "if" body. Please go through the file to fix other occurrences. http://codereview.chromium.org/3495005/diff/27001/28005#newcode142 net/socket/ssl_client_socket_openssl.cc:142: ssl_info->cert = X509Certificate::CreateFromBytes( There is a better way to construct a fake certificate. Please see how the Gears and Chrome Frame code does that. (Search for X509Certificate::Create or "new X509Certificate" or "new net::X509Certificate" to find it...) http://codereview.chromium.org/3495005/diff/27001/28005#newcode149 net/socket/ssl_client_socket_openssl.cc:149: ((TLS1_CK_RSA_EXPORT1024_WITH_DES_CBC_SHA) & Nit: this fake cipher suite is inconsistent with the fake security_bits of 128 above. (DES is 56 bits.) http://codereview.chromium.org/3495005/diff/27001/28005#newcode193 net/socket/ssl_client_socket_openssl.cc:193: // and call the callback with more info Nit: my C++ readability review told me to use proper punctuation in source code comments, so I'm passing this on to you. So please add a period (.) to the end of sentences. I won't repeat this nit-picking... http://codereview.chromium.org/3495005/diff/27001/28005#newcode219 net/socket/ssl_client_socket_openssl.cc:219: // (Optional) Tell loadlog we are starting and store a ref I have no idea what this comment means ... http://codereview.chromium.org/3495005/diff/27001/28006 File net/socket/ssl_client_socket_openssl.h (right): http://codereview.chromium.org/3495005/diff/27001/28006#newcode63 net/socket/ssl_client_socket_openssl.h:63: bool InitOpenSSL(); Please add a TODO comment to note that InitOpenSSL() is not thread-safe. More than one thread may create and use SSLClientSocket objects. Our standard solution is to add an EnsureInitOpenSSL function that uses a Singleton internally. Look at EnsureInitNSS and EnsureInitWinsock as examples.
Thanks! Addressed almost all comments, not sure on how to improve the bogus cert creation. Cheers Joth http://codereview.chromium.org/3495005/diff/27001/28002 File build/linux/system.gyp (right): http://codereview.chromium.org/3495005/diff/27001/28002#newcode331 build/linux/system.gyp:331: 'USE_OPENSSL', On 2010/09/28 18:08:35, wtc wrote: > You should document that eventually USE_OPENSSL and USE_NSS > will be mutually exclusive (only one of the macros should be > defined). During the transitional period, a use_openssl=1 > build still needs to define USE_NSS, so it is necessary to > test the USE_OPENSSL macro before testing USE_NSS. > (net/socket/client_socket_factory.cc is an example.) Done. http://codereview.chromium.org/3495005/diff/27001/28005 File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/3495005/diff/27001/28005#newcode22 net/socket/ssl_client_socket_openssl.cc:22: // Enable this to see logging for state machine state transitions. On 2010/09/28 18:08:35, wtc wrote: > Nit: this comment says "Enable this", but the ifdef is already > #if 1. > > It would be better to reverse the order like this: > > #if 0 > #define GotoState(s) do { LOG(INFO) << (void *)this << " " << __FUNCTION__ << \ > ... > #else > #define GotoState(s) next_handshake_state_ = s > #endif Done. (It was left as was for consistency with NSS) http://codereview.chromium.org/3495005/diff/27001/28005#newcode50 net/socket/ssl_client_socket_openssl.cc:50: return err < 0 ? ERR_SSL_PROTOCOL_ERROR : err; On 2010/09/28 18:08:35, wtc wrote: > BUG: > This error-mapping function should never return an > OpenSSL error unmapped. It does that when err >= 0. Done. http://codereview.chromium.org/3495005/diff/27001/28005#newcode56 net/socket/ssl_client_socket_openssl.cc:56: SSL_CTX* SSLClientSocketOpenSSL::g_ctx = NULL; On 2010/09/28 18:08:35, wtc wrote: > Nit: we usually add a comment > // static > before static member initializers. Done. http://codereview.chromium.org/3495005/diff/27001/28005#newcode86 net/socket/ssl_client_socket_openssl.cc:86: if (g_ctx) { On 2010/09/28 18:08:35, wtc wrote: > Nit: in the net module we usually omit curly braces {} > around a simple one-line "if" body. Please go through the > file to fix other occurrences. Done. http://codereview.chromium.org/3495005/diff/27001/28005#newcode142 net/socket/ssl_client_socket_openssl.cc:142: ssl_info->cert = X509Certificate::CreateFromBytes( On 2010/09/28 18:08:35, wtc wrote: > There is a better way to construct a fake certificate. > Please see how the Gears and Chrome Frame code does that. > (Search for X509Certificate::Create or > "new X509Certificate" or "new net::X509Certificate" to > find it...) Hmmm couldn't find any reference to X509 in gears or chrome_frame, just in test code, which didn't cover the needs for here. It's already covered by a TODO to delete this anyway, but if you have a link for the better hack I'll gladly update it. http://codereview.chromium.org/3495005/diff/27001/28005#newcode149 net/socket/ssl_client_socket_openssl.cc:149: ((TLS1_CK_RSA_EXPORT1024_WITH_DES_CBC_SHA) & On 2010/09/28 18:08:35, wtc wrote: > Nit: this fake cipher suite is inconsistent with the fake > security_bits of 128 above. (DES is 56 bits.) Done. (although, I kind of like that this bogus info was broken!) http://codereview.chromium.org/3495005/diff/27001/28005#newcode193 net/socket/ssl_client_socket_openssl.cc:193: // and call the callback with more info On 2010/09/28 18:08:35, wtc wrote: > Nit: my C++ readability review told me to use > proper punctuation in source code comments, so I'm passing > this on to you. So please add a period (.) to the end of > sentences. I won't repeat this nit-picking... Removed obsolete comment. http://codereview.chromium.org/3495005/diff/27001/28005#newcode219 net/socket/ssl_client_socket_openssl.cc:219: // (Optional) Tell loadlog we are starting and store a ref On 2010/09/28 18:08:35, wtc wrote: > I have no idea what this comment means ... Removed obsolete comment. http://codereview.chromium.org/3495005/diff/27001/28006 File net/socket/ssl_client_socket_openssl.h (right): http://codereview.chromium.org/3495005/diff/27001/28006#newcode63 net/socket/ssl_client_socket_openssl.h:63: bool InitOpenSSL(); On 2010/09/28 18:08:35, wtc wrote: > Please add a TODO comment to note that InitOpenSSL() is > not thread-safe. More than one thread may create and use > SSLClientSocket objects. > > Our standard solution is to add an EnsureInitOpenSSL function > that uses a Singleton internally. Look at EnsureInitNSS > and EnsureInitWinsock as examples. Done.
http://codereview.chromium.org/3495005/diff/27001/28005 File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/3495005/diff/27001/28005#newcode142 net/socket/ssl_client_socket_openssl.cc:142: ssl_info->cert = X509Certificate::CreateFromBytes( On 2010/09/29 11:58:54, joth wrote: > > Hmmm couldn't find any reference to X509 in gears or chrome_frame, just in test > code, which didn't cover the needs for here. > It's already covered by a TODO to delete this anyway, but if you have a link for > the better hack I'll gladly update it. I should have said more about how to find those files because their file names don't say "gears" or "chrome_frame". Sorry about that. Search for "Chrome Internal" in the chromium source tree. You will find: gears: src\chrome\common\net\url_request_intercept_job.cc chrome frame: src\chrome\browser\automation\url_request_automation_job.cc |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
