Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1518613002:
Support for server session cache. (Closed)
DescriptionSupport for server session cache.
Allows SSL server sockets created through a SSLServerSocketContext
to share a single session cache.
OpenSSL only.
BUG=568650
Committed: https://crrev.com/eb9e3bc7bfc291ad600943b24304946328bcf4f0
Cr-Commit-Position: refs/heads/master@{#379751}
Patch Set 1 #Patch Set 2 : Rebased #Patch Set 3 : Rebase #Patch Set 4 : Rebase only #
Total comments: 38
Patch Set 5 : Addresses Reviewer's Comments #Patch Set 6 : Addresses Reviewer's Comments (ignore patch 5) #Patch Set 7 : Cleaned up some unnecessary change #Patch Set 8 : Rebased #Patch Set 9 : Updated remoting, fixed ssl_server_socket_nss compile errors #Patch Set 10 : Updated remoting #Patch Set 11 : Rebased #Patch Set 12 : Some cleanup #Patch Set 13 : Rebased #Patch Set 14 : Rebased to head #
Total comments: 39
Patch Set 15 : Addresses Reviewer's Comments #Patch Set 16 : Added a missing period. #
Total comments: 2
Patch Set 17 : Fixed nit #
Total comments: 7
Patch Set 18 : Remoting - move context to P2PStreamSocketAdapter #
Total comments: 2
Patch Set 19 : Remoting: Moved server_context_ above socket_ #Patch Set 20 : Remoting: Fixed initialization ordering of socket_ and server_context_ #Messages
Total messages: 40 (13 generated)
Description was changed from ========== Support for server session cache. Allows SSL server sockets created through a SSLServerSocketContext to share a single session cache. ========== to ========== Support for server session cache. Allows SSL server sockets created through a SSLServerSocketContext to share a single session cache. BUG=568650 ==========
Description was changed from ========== Support for server session cache. Allows SSL server sockets created through a SSLServerSocketContext to share a single session cache. BUG=568650 ========== to ========== Support for server session cache. Allows SSL server sockets created through a SSLServerSocketContext to share a single session cache. OpenSSL only. BUG=568650 ==========
ryanchung@chromium.org changed reviewers: + davidben@chromium.org, dougsteed@chromium.org, rsleevi@chromium.org
Ping. Thanks.
David: Could you make sure you're happy with the high-level design, and then I'll do the code deep dive? I don't have as much paged in mentally about where you'd like the client socket context objects to go, so I just wanted to sanity check the approach first with you.
Sorry, I got the dependency arrows in Rietveld flipped and looked at this one before the client auth one. Will go back to looking at the client auth one next week. In the meantime, here's some comments on this one. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... File net/socket/ssl_server_socket.h (right): https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket.h:34: class SSLServerSocketContext { svaldez has been calling the corresponding client API 'SSLClientContext' which is a little shorter. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket.h:36: // Creates an SSL server socket over an already-connected transport socket. Add: The caller must ensure the returned socket does not outlive the server context. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket.h:39: // of CreateSSLServerSocket will delete |socket| and return NULL. This is already expressed in the function signature. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket.h:39: // of CreateSSLServerSocket will delete |socket| and return NULL. What do you mean stubbed versions? Nothing every checks that the return value is nullptr and it doesn't really make sense for this to return nullptr. The interface shouldn't be documented to do that. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket.h:64: NET_EXPORT scoped_ptr<SSLServerSocketContext> CreateSSLServerSocketContext( Ditto re SSLServerSocketContext vs SSLServerContext. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_nss.cc (right): https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_nss.cc:3: // found in the LICENSE file. [This implementation completely violates the interface, but we should resolve that separately by just removing this file. crbug.com/580686.] https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_nss.cc:82: crypto::RSAPrivateKey* key, I don't think this compiles. It doesn't match the header. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_nss.cc:92: scoped_refptr<X509Certificate> certificate, Nit: Unnecessary bouncing on reference counts. X509Certificate*? https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_nss.h (right): https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_nss.h:47: class SSLServerSocketNSS : public SSLServerSocket { I don't think this type actually needs to be defined in the header file anymore. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_openssl.cc (right): https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:82: : ssl_ctx_(SSL_CTX_new(SSLv23_server_method())), Nit: It's the same thing, but this ought to be TLS_method(). https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:89: ssl_ctx_.get(), (unsigned char*)&session_ctx_id, sizeof(session_ctx_id)); unsigned char -> uint8_t. Also C++-style casts. But since any old garbage would do, you can avoid the cast by just making session_ctx_id a uint8_t. (It's in my queue to see if we can get rid of this as it's a little weird.) https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:98: CHECK(key_); This probably should be the first line. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:733: // Set certificate and private key. Just about everything from hear onwards should be configured on the SSL_CTX, not each SSL individually. It'll be more efficient and you won't need to pass as many things to the socket. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:775: options.ConfigureFlag(SSL_OP_NO_TICKET, true); Why turn off tickets? I can't think of any reason why you'd want to do that. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_openssl.h (right): https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.h:54: class SSLServerSocketOpenSSL : public SSLServerSocket { Ditto that this needn't be in the header file now. Also you could avoid the friend bit by making this an inner class. (Forward-declare it in SSLServerSocketContextOpenSSL and then define it in the C++ file.) https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_unittest.cc (right): https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:76: class FakeDataChannel : public base::RefCountedThreadSafe<FakeDataChannel> { Reference-counted mutable classes are usually an indication or confused ownership. Why is this ref-counted? https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:394: void Initialize(bool reinitialize_server_context = false) { Style: no default arguments. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:569: ASSERT_TRUE(server_ret == OK || server_ret == ERR_IO_PENDING); I wouldn't bother with assertions like this one. The GetResult assertion will cover everything. https://codereview.chromium.org/1518613002/diff/60001/net/test/embedded_test_... File net/test/embedded_test_server/embedded_test_server.cc (right): https://codereview.chromium.org/1518613002/diff/60001/net/test/embedded_test_... net/test/embedded_test_server/embedded_test_server.cc:298: return context->CreateSSLServerSocket(std::move(connection)); The socket should not outlive the context, nor should you create a separate context for each connection. There should be one SSLServerSocketContext for the EmbeddedTestServer so ETS can do tests with a session cache.
Thanks. I'll fix these first but will probably need to rebase on to the other change as that one progresses. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... File net/socket/ssl_server_socket.h (right): https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket.h:34: class SSLServerSocketContext { On 2016/01/22 23:57:48, davidben (slow) wrote: > svaldez has been calling the corresponding client API 'SSLClientContext' which > is a little shorter. Done. Will call it SSLServerContext. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket.h:36: // Creates an SSL server socket over an already-connected transport socket. On 2016/01/22 23:57:47, davidben (slow) wrote: > Add: The caller must ensure the returned socket does not outlive the server > context. Done. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket.h:39: // of CreateSSLServerSocket will delete |socket| and return NULL. On 2016/01/22 23:57:47, davidben (slow) wrote: > What do you mean stubbed versions? Nothing every checks that the return value is > nullptr and it doesn't really make sense for this to return nullptr. The > interface shouldn't be documented to do that. Whoops, looks like an outdated comment I accidentally moved from the original CreateSSLServerSocket. Will remove. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket.h:39: // of CreateSSLServerSocket will delete |socket| and return NULL. On 2016/01/22 23:57:48, davidben (slow) wrote: > This is already expressed in the function signature. Done. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket.h:64: NET_EXPORT scoped_ptr<SSLServerSocketContext> CreateSSLServerSocketContext( On 2016/01/22 23:57:48, davidben (slow) wrote: > Ditto re SSLServerSocketContext vs SSLServerContext. Done. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_nss.cc (right): https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_nss.cc:3: // found in the LICENSE file. On 2016/01/22 23:57:48, davidben (slow) wrote: > [This implementation completely violates the interface, but we should resolve > that separately by just removing this file. crbug.com/580686.] Ok. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_nss.cc:82: crypto::RSAPrivateKey* key, On 2016/01/22 23:57:48, davidben (slow) wrote: > I don't think this compiles. It doesn't match the header. Done. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_nss.cc:92: scoped_refptr<X509Certificate> certificate, On 2016/01/22 23:57:48, davidben (slow) wrote: > Nit: Unnecessary bouncing on reference counts. X509Certificate*? Done. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_nss.h (right): https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_nss.h:47: class SSLServerSocketNSS : public SSLServerSocket { On 2016/01/22 23:57:48, davidben (slow) wrote: > I don't think this type actually needs to be defined in the header file anymore. Done. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_openssl.cc (right): https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:82: : ssl_ctx_(SSL_CTX_new(SSLv23_server_method())), On 2016/01/22 23:57:48, davidben (slow) wrote: > Nit: It's the same thing, but this ought to be TLS_method(). Done. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:89: ssl_ctx_.get(), (unsigned char*)&session_ctx_id, sizeof(session_ctx_id)); On 2016/01/22 23:57:48, davidben (slow) wrote: > unsigned char -> uint8_t. Also C++-style casts. But since any old garbage would > do, you can avoid the cast by just making session_ctx_id a uint8_t. > > (It's in my queue to see if we can get rid of this as it's a little weird.) Done. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:98: CHECK(key_); On 2016/01/22 23:57:48, davidben (slow) wrote: > This probably should be the first line. Done. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:733: // Set certificate and private key. On 2016/01/22 23:57:48, davidben (slow) wrote: > Just about everything from hear onwards should be configured on the SSL_CTX, not > each SSL individually. It'll be more efficient and you won't need to pass as > many things to the socket. Done. I moved the configuration steps to the constructor of the SSLServerContext. Or should they be in a separate Init for the context? Having them in the constructor, I changed all the "return ERR_UNEXPECTED;" to CHECK()s. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.cc:775: options.ConfigureFlag(SSL_OP_NO_TICKET, true); On 2016/01/22 23:57:48, davidben (slow) wrote: > Why turn off tickets? I can't think of any reason why you'd want to do that. Done. Should not be turned off. Line removed. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_openssl.h (right): https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_openssl.h:54: class SSLServerSocketOpenSSL : public SSLServerSocket { On 2016/01/22 23:57:48, davidben (slow) wrote: > Ditto that this needn't be in the header file now. Also you could avoid the > friend bit by making this an inner class. (Forward-declare it in > SSLServerSocketContextOpenSSL and then define it in the C++ file.) Done. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... File net/socket/ssl_server_socket_unittest.cc (right): https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:76: class FakeDataChannel : public base::RefCountedThreadSafe<FakeDataChannel> { On 2016/01/22 23:57:48, davidben (slow) wrote: > Reference-counted mutable classes are usually an indication or confused > ownership. Why is this ref-counted? Done. Changed it back. These were used by both the server and client sockets while the unittest class was the owner. I originally changed it to ref-count so the unittest class didn't have to own it and so I can create as many of these sockets as I needed. Since I only needed 2 of each sockets, I'll just have the unittest make 4 FakeDataChannels. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:394: void Initialize(bool reinitialize_server_context = false) { On 2016/01/22 23:57:48, davidben (slow) wrote: > Style: no default arguments. Done. https://codereview.chromium.org/1518613002/diff/60001/net/socket/ssl_server_s... net/socket/ssl_server_socket_unittest.cc:569: ASSERT_TRUE(server_ret == OK || server_ret == ERR_IO_PENDING); On 2016/01/22 23:57:48, davidben (slow) wrote: > I wouldn't bother with assertions like this one. The GetResult assertion will > cover everything. Done. https://codereview.chromium.org/1518613002/diff/60001/net/test/embedded_test_... File net/test/embedded_test_server/embedded_test_server.cc (right): https://codereview.chromium.org/1518613002/diff/60001/net/test/embedded_test_... net/test/embedded_test_server/embedded_test_server.cc:298: return context->CreateSSLServerSocket(std::move(connection)); On 2016/01/22 23:57:48, davidben (slow) wrote: > The socket should not outlive the context, nor should you create a separate > context for each connection. There should be one SSLServerSocketContext for the > EmbeddedTestServer so ETS can do tests with a session cache. I moved the context creation to InitializeAndListene(). Could you please help me check if I'm putting the context initialization in the right place? I'm not too familiar with the code here. Thanks!
This change has been rebased after client certs landed. It is ready for review. Thanks.
This basically looks good. Mostly comments on the tests. Having all the CHECKs is kind of annoying, but we don't really want new non-test consumers of this API, so if this works for Cast, this is probably fine? https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... File net/socket/ssl_server_socket_openssl.cc (right): https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:172: SSLServerConfig ssl_server_config_; I'm not sure this is actually used. https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:175: scoped_refptr<X509Certificate> cert_; Ditto. https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:178: scoped_ptr<crypto::RSAPrivateKey> key_; Ditto. https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:896: CHECK(SSL_CTX_use_certificate(ssl_ctx_.get(), cert_->os_cert_handle()) == 1); Nit: The == 1 is a remnant of OpenSSL returning -1, 0, 1 for everything. This can just be CHECK(SSL_CTX_use_certificate(...)); https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:909: CHECK(SSL_CTX_use_certificate(ssl_ctx_.get(), x509.get()) == 1); Nit: ditto https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:913: CHECK(SSL_CTX_use_PrivateKey(ssl_ctx_.get(), key_->key()) == 1); Nit: ditto https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:980: crypto::EnsureOpenSSLInit(); This needs to happen before the SSL_CTX_new call. (So move it to the constructor and then move the SSL_CTX_new() outside the initializer list.) https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:983: std::move(socket), cert_.get(), *key_, ssl_server_config_, ssl)); I think this comment is moot because we can just get rid of those fields (above), but rather than make a copy of everything, you can have SSLServerSocketOpenSSL contain a weak pointer to the SSLServerContextOpenSSL. Then SSLServerSocketOpenSSL is either a private inner class or a friend of SSLServerContextOpenSSL. (But it seems we don't need to bother.) https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... File net/socket/ssl_server_socket_unittest.cc (right): https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:355: initialized_ = true; Ugh, globals. :-( Having you rewrite every test would be silly, but perhaps this kind of a split would be tidier. What do you think? 1. Move lines 347-350 into the constructor. Save server_cert_ and server_private_key_ since unlike everything else, those are actually logically constants. 2. Move this block of code into the constructor. 3. Make channel_1_ and channel_2_ be scoped_ptr so you can drop them and create new ones. 4. InitializeContext will: a) Reset sockets and channels because you're about to destroy the old server context. b) Reset server_contet_ 5. InitializeSockets will: a) Reset any existing sockets and channels. b) Create new FakeDataChannel objects and new sockets. 6. Optional: Maybe Initialize -> Create since you call it multiple times? Dunno. Basically this is to merge InitialCacheSockets into InitializeSockets and get rid of this awkward thing where InitializeContext is even more stateful than before. https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:368: socket_factory_->ClearSSLSessionCache(); [Chatted out-of-band, but I think we can remove this line.] https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:457: FakeDataChannel channel_2b; Style: Should end with underscores. https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:515: // This tests makes sure the session cache is working (I suspect this test or the following one will blow up on NSS. You'll probably need to move them under the #ifdef.) https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:535: EXPECT_EQ(CERT_STATUS_AUTHORITY_INVALID, ssl_info.cert_status); (You probably don't need to bother with checking the cert status. Ditto below) https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:536: EXPECT_EQ(ssl_info.handshake_type, SSLInfo::HANDSHAKE_FULL); Maybe check both the client and server SSLInfo? Ditto below. https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:538: // Make sure the second connection is cached Nit: period at end https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:675: // Create the connection again Nit: period at end https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:763: // Below, check that the cache didn't store the result of a failed handshake Nit: period at end. https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:829: // Below, check that the cache didn't store the result of a failed handshake Nit: period at end. https://codereview.chromium.org/1518613002/diff/260001/net/test/embedded_test... File net/test/embedded_test_server/embedded_test_server.cc (left): https://codereview.chromium.org/1518613002/diff/260001/net/test/embedded_test... net/test/embedded_test_server/embedded_test_server.cc:284: CHECK(base::ReadFileToString(key_path, &key_string)); [ Oh hey, we won't have to read the file on every connection now! :-) ] https://codereview.chromium.org/1518613002/diff/260001/remoting/protocol/ssl_... File remoting/protocol/ssl_hmac_channel_authenticator.cc (right): https://codereview.chromium.org/1518613002/diff/260001/remoting/protocol/ssl_... remoting/protocol/ssl_hmac_channel_authenticator.cc:230: void SslHmacChannelAuthenticator::SecureAndAuthenticate( It looks like this only gets called once[*] for each SslHmacChannelAuthenticator object, which means you don't actually need to separate out InitializeSSLServerContext from creating the SSLServerSocket. Might be a bit less churn. [*] https://code.google.com/p/chromium/codesearch#chromium/src/remoting/protocol/... Also it saves all kinds of state and would probably explode if you called it multiple times. :-)
> Having all the CHECKs is kind of annoying, but we don't really want new non-test consumers of this API, so if this works for Cast, this is probably fine? Maybe an alternative is to create an Init method for SSLServerContext? https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... File net/socket/ssl_server_socket_openssl.cc (right): https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:172: SSLServerConfig ssl_server_config_; On 2016/02/24 21:01:25, davidben wrote: > I'm not sure this is actually used. Done. https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:175: scoped_refptr<X509Certificate> cert_; On 2016/02/24 21:01:26, davidben wrote: > Ditto. Done. https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:178: scoped_ptr<crypto::RSAPrivateKey> key_; On 2016/02/24 21:01:26, davidben wrote: > Ditto. Done. https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:896: CHECK(SSL_CTX_use_certificate(ssl_ctx_.get(), cert_->os_cert_handle()) == 1); On 2016/02/24 21:01:26, davidben wrote: > Nit: The == 1 is a remnant of OpenSSL returning -1, 0, 1 for everything. This > can just be CHECK(SSL_CTX_use_certificate(...)); Done. https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:909: CHECK(SSL_CTX_use_certificate(ssl_ctx_.get(), x509.get()) == 1); On 2016/02/24 21:01:25, davidben wrote: > Nit: ditto Done. https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:913: CHECK(SSL_CTX_use_PrivateKey(ssl_ctx_.get(), key_->key()) == 1); On 2016/02/24 21:01:26, davidben wrote: > Nit: ditto Done. https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:980: crypto::EnsureOpenSSLInit(); On 2016/02/24 21:01:25, davidben wrote: > This needs to happen before the SSL_CTX_new call. (So move it to the constructor > and then move the SSL_CTX_new() outside the initializer list.) Done. Or alternately, can we just call EnsureOpenSSLInit() inside CreateSSLServerContext()? https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_openssl.cc:983: std::move(socket), cert_.get(), *key_, ssl_server_config_, ssl)); On 2016/02/24 21:01:25, davidben wrote: > I think this comment is moot because we can just get rid of those fields > (above), but rather than make a copy of everything, you can have > SSLServerSocketOpenSSL contain a weak pointer to the SSLServerContextOpenSSL. > Then SSLServerSocketOpenSSL is either a private inner class or a friend of > SSLServerContextOpenSSL. (But it seems we don't need to bother.) Thanks! https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... File net/socket/ssl_server_socket_unittest.cc (right): https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:355: initialized_ = true; On 2016/02/24 21:01:26, davidben wrote: > Ugh, globals. :-( Having you rewrite every test would be silly, but perhaps this > kind of a split would be tidier. What do you think? > > 1. Move lines 347-350 into the constructor. Save server_cert_ and > server_private_key_ since unlike everything else, those are actually logically > constants. > > 2. Move this block of code into the constructor. > > 3. Make channel_1_ and channel_2_ be scoped_ptr so you can drop them and create > new ones. > > 4. InitializeContext will: > a) Reset sockets and channels because you're about to destroy the old server > context. > b) Reset server_contet_ > > 5. InitializeSockets will: > a) Reset any existing sockets and channels. > b) Create new FakeDataChannel objects and new sockets. > > 6. Optional: Maybe Initialize -> Create since you call it multiple times? Dunno. > > Basically this is to merge InitialCacheSockets into InitializeSockets and get > rid of this awkward thing where InitializeContext is even more stateful than > before. Done. Yes, this will make it a lot cleaner! Thanks. https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:368: socket_factory_->ClearSSLSessionCache(); On 2016/02/24 21:01:26, davidben wrote: > [Chatted out-of-band, but I think we can remove this line.] Done. https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:457: FakeDataChannel channel_2b; On 2016/02/24 21:01:26, davidben wrote: > Style: Should end with underscores. Done. https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:515: // This tests makes sure the session cache is working On 2016/02/24 21:01:26, davidben wrote: > (I suspect this test or the following one will blow up on NSS. You'll probably > need to move them under the #ifdef.) Done. https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:535: EXPECT_EQ(CERT_STATUS_AUTHORITY_INVALID, ssl_info.cert_status); On 2016/02/24 21:01:26, davidben wrote: > (You probably don't need to bother with checking the cert status. Ditto below) Done. https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:536: EXPECT_EQ(ssl_info.handshake_type, SSLInfo::HANDSHAKE_FULL); On 2016/02/24 21:01:26, davidben wrote: > Maybe check both the client and server SSLInfo? Ditto below. Done. https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:538: // Make sure the second connection is cached On 2016/02/24 21:01:26, davidben wrote: > Nit: period at end Done. https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:675: // Create the connection again On 2016/02/24 21:01:26, davidben wrote: > Nit: period at end Done. https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:763: // Below, check that the cache didn't store the result of a failed handshake On 2016/02/24 21:01:26, davidben wrote: > Nit: period at end. Done. https://codereview.chromium.org/1518613002/diff/260001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:829: // Below, check that the cache didn't store the result of a failed handshake On 2016/02/24 21:01:26, davidben wrote: > Nit: period at end. Done. https://codereview.chromium.org/1518613002/diff/260001/remoting/protocol/ssl_... File remoting/protocol/ssl_hmac_channel_authenticator.cc (right): https://codereview.chromium.org/1518613002/diff/260001/remoting/protocol/ssl_... remoting/protocol/ssl_hmac_channel_authenticator.cc:230: void SslHmacChannelAuthenticator::SecureAndAuthenticate( On 2016/02/24 21:01:26, davidben wrote: > It looks like this only gets called once[*] for each SslHmacChannelAuthenticator > object, which means you don't actually need to separate out > InitializeSSLServerContext from creating the SSLServerSocket. Might be a bit > less churn. > > [*] > https://code.google.com/p/chromium/codesearch#chromium/src/remoting/protocol/... > > Also it saves all kinds of state and would probably explode if you called it > multiple times. :-) Done.
lgtm! https://codereview.chromium.org/1518613002/diff/300001/net/socket/ssl_server_... File net/socket/ssl_server_socket_unittest.cc (right): https://codereview.chromium.org/1518613002/diff/300001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:507: // NSS ports don't support client certificates. Nit: Perhaps "don't support client certificates and have a global session cache". (Hrm. I should probably see about disabling the session cache in the NSS side altogether. Although we're *finally* moving forward on getting iOS off of NSS anyway. Meh.)
On 2016/02/25 00:46:40, ryanchung wrote: > > Having all the CHECKs is kind of annoying, but we don't really want new > non-test > consumers of this API, so if this works for Cast, this is probably fine? > > Maybe an alternative is to create an Init method for SSLServerContext? I'm fine with any of these I think. (Init method, create function being able to fail, or just CHECKs as you have it right now.)
Thanks! https://codereview.chromium.org/1518613002/diff/300001/net/socket/ssl_server_... File net/socket/ssl_server_socket_unittest.cc (right): https://codereview.chromium.org/1518613002/diff/300001/net/socket/ssl_server_... net/socket/ssl_server_socket_unittest.cc:507: // NSS ports don't support client certificates. On 2016/03/04 18:48:00, davidben wrote: > Nit: Perhaps "don't support client certificates and have a global session > cache". > > (Hrm. I should probably see about disabling the session cache in the NSS side > altogether. Although we're *finally* moving forward on getting iOS off of NSS > anyway. Meh.) Done.
ryanchung@chromium.org changed reviewers: + sergeyu@chromium.org
+sergeyu Could you please review remoting? Thank you!
https://codereview.chromium.org/1518613002/diff/320001/remoting/protocol/ssl_... File remoting/protocol/ssl_hmac_channel_authenticator.cc (right): https://codereview.chromium.org/1518613002/diff/320001/remoting/protocol/ssl_... remoting/protocol/ssl_hmac_channel_authenticator.cc:231: server_context_ = net::CreateSSLServerContext( Does server_context_ need to be a class member instead of just a local variable here?
https://codereview.chromium.org/1518613002/diff/320001/remoting/protocol/ssl_... File remoting/protocol/ssl_hmac_channel_authenticator.cc (right): https://codereview.chromium.org/1518613002/diff/320001/remoting/protocol/ssl_... remoting/protocol/ssl_hmac_channel_authenticator.cc:231: server_context_ = net::CreateSSLServerContext( On 2016/03/04 20:37:25, Sergey Ulanov wrote: > Does server_context_ need to be a class member instead of just a local variable > here? The socket shouldn't outlive the context. (It's not going to hold on to any state that you didn't already hold on to before this change. You all don't really use session caching, which is why it's a little degenerate on your end.)
https://codereview.chromium.org/1518613002/diff/320001/remoting/protocol/ssl_... File remoting/protocol/ssl_hmac_channel_authenticator.cc (right): https://codereview.chromium.org/1518613002/diff/320001/remoting/protocol/ssl_... remoting/protocol/ssl_hmac_channel_authenticator.cc:231: server_context_ = net::CreateSSLServerContext( On 2016/03/04 20:56:55, davidben wrote: > On 2016/03/04 20:37:25, Sergey Ulanov wrote: > > Does server_context_ need to be a class member instead of just a local > variable > > here? > > The socket shouldn't outlive the context. (It's not going to hold on to any > state that you didn't already hold on to before this change. You all don't > really use session caching, which is why it's a little degenerate on your end.) So then this code will not work correctly. SslHmacChannelAuthenticator doesn't outlive the socket it creates. It gives away ownership of the socket in CheckDone() and is deleted shortly after that. Maybe make SSLServerContext ref-counted so it's possible to share it between multiple sockets?
https://codereview.chromium.org/1518613002/diff/320001/remoting/protocol/ssl_... File remoting/protocol/ssl_hmac_channel_authenticator.cc (right): https://codereview.chromium.org/1518613002/diff/320001/remoting/protocol/ssl_... remoting/protocol/ssl_hmac_channel_authenticator.cc:231: server_context_ = net::CreateSSLServerContext( On 2016/03/04 21:35:31, Sergey Ulanov wrote: > On 2016/03/04 20:56:55, davidben wrote: > > On 2016/03/04 20:37:25, Sergey Ulanov wrote: > > > Does server_context_ need to be a class member instead of just a local > > variable > > > here? > > > > The socket shouldn't outlive the context. (It's not going to hold on to any > > state that you didn't already hold on to before this change. You all don't > > really use session caching, which is why it's a little degenerate on your > end.) > > So then this code will not work correctly. SslHmacChannelAuthenticator doesn't > outlive the socket it creates. It gives away ownership of the socket in > CheckDone() and is deleted shortly after that. > > Maybe make SSLServerContext ref-counted so it's possible to share it between > multiple sockets? Oh, that's annoying. Reference-counting is viral because it makes the object have lifetime infinity, which means lifetime contracts around non-owning pointers can't happen. And there is a raw pointer in the SSLServerConfig (ClientCertVerifier), so we need to have defined lifetimes. I guess, for consumers that don't actually want session caching and want to be able to carry server sockets standalone, we could make a StreamSocket implementation which contains scoped_ptr<SSLServerContext> context_; scoped_ptr<SSLServerSocket> socket_; and then forwards all methods to socket_. Then you can pass it around standalone.
https://codereview.chromium.org/1518613002/diff/320001/remoting/protocol/ssl_... File remoting/protocol/ssl_hmac_channel_authenticator.cc (right): https://codereview.chromium.org/1518613002/diff/320001/remoting/protocol/ssl_... remoting/protocol/ssl_hmac_channel_authenticator.cc:231: server_context_ = net::CreateSSLServerContext( On 2016/03/04 21:45:53, davidben wrote: > On 2016/03/04 21:35:31, Sergey Ulanov wrote: > > On 2016/03/04 20:56:55, davidben wrote: > > > On 2016/03/04 20:37:25, Sergey Ulanov wrote: > > > > Does server_context_ need to be a class member instead of just a local > > > variable > > > > here? > > > > > > The socket shouldn't outlive the context. (It's not going to hold on to any > > > state that you didn't already hold on to before this change. You all don't > > > really use session caching, which is why it's a little degenerate on your > > end.) > > > > So then this code will not work correctly. SslHmacChannelAuthenticator doesn't > > outlive the socket it creates. It gives away ownership of the socket in > > CheckDone() and is deleted shortly after that. > > > > Maybe make SSLServerContext ref-counted so it's possible to share it between > > multiple sockets? Oh, I probably should have elaborated here: you can already share it between multiple sockets. That's the point. So, yeah, if you had a context struct that SslHmacChannelAuthenticators shared, that would work too. But only if you were actually okay with session resumption happening (which I suspect you aren't?). So I think this is correct.
https://codereview.chromium.org/1518613002/diff/320001/remoting/protocol/ssl_... File remoting/protocol/ssl_hmac_channel_authenticator.cc (right): https://codereview.chromium.org/1518613002/diff/320001/remoting/protocol/ssl_... remoting/protocol/ssl_hmac_channel_authenticator.cc:231: server_context_ = net::CreateSSLServerContext( On 2016/03/04 21:45:53, davidben wrote: > On 2016/03/04 21:35:31, Sergey Ulanov wrote: > > On 2016/03/04 20:56:55, davidben wrote: > > > On 2016/03/04 20:37:25, Sergey Ulanov wrote: > > > > Does server_context_ need to be a class member instead of just a local > > > variable > > > > here? > > > > > > The socket shouldn't outlive the context. (It's not going to hold on to any > > > state that you didn't already hold on to before this change. You all don't > > > really use session caching, which is why it's a little degenerate on your > > end.) > > > > So then this code will not work correctly. SslHmacChannelAuthenticator doesn't > > outlive the socket it creates. It gives away ownership of the socket in > > CheckDone() and is deleted shortly after that. > > > > Maybe make SSLServerContext ref-counted so it's possible to share it between > > multiple sockets? > > Oh, that's annoying. > > Reference-counting is viral because it makes the object have lifetime infinity, > which means lifetime contracts around non-owning pointers can't happen. And > there is a raw pointer in the SSLServerConfig (ClientCertVerifier), so we need > to have defined lifetimes. > > I guess, for consumers that don't actually want session caching and want to be > able to carry server sockets standalone, we could make a StreamSocket > implementation which contains > > scoped_ptr<SSLServerContext> context_; > scoped_ptr<SSLServerSocket> socket_; > > and then forwards all methods to socket_. Then you can pass it around > standalone. We have P2PStreamSocketAdapter that takes ownership of the socket. I think you can add server_context_ there as well. For the client sockets it will be null.
https://codereview.chromium.org/1518613002/diff/320001/remoting/protocol/ssl_... File remoting/protocol/ssl_hmac_channel_authenticator.cc (right): https://codereview.chromium.org/1518613002/diff/320001/remoting/protocol/ssl_... remoting/protocol/ssl_hmac_channel_authenticator.cc:231: server_context_ = net::CreateSSLServerContext( On 2016/03/04 22:34:07, Sergey Ulanov wrote: > On 2016/03/04 21:45:53, davidben wrote: > > On 2016/03/04 21:35:31, Sergey Ulanov wrote: > > > On 2016/03/04 20:56:55, davidben wrote: > > > > On 2016/03/04 20:37:25, Sergey Ulanov wrote: > > > > > Does server_context_ need to be a class member instead of just a local > > > > variable > > > > > here? > > > > > > > > The socket shouldn't outlive the context. (It's not going to hold on to > any > > > > state that you didn't already hold on to before this change. You all don't > > > > really use session caching, which is why it's a little degenerate on your > > > end.) > > > > > > So then this code will not work correctly. SslHmacChannelAuthenticator > doesn't > > > outlive the socket it creates. It gives away ownership of the socket in > > > CheckDone() and is deleted shortly after that. > > > > > > Maybe make SSLServerContext ref-counted so it's possible to share it between > > > multiple sockets? > > > > Oh, that's annoying. > > > > Reference-counting is viral because it makes the object have lifetime > infinity, > > which means lifetime contracts around non-owning pointers can't happen. And > > there is a raw pointer in the SSLServerConfig (ClientCertVerifier), so we need > > to have defined lifetimes. > > > > I guess, for consumers that don't actually want session caching and want to be > > able to carry server sockets standalone, we could make a StreamSocket > > implementation which contains > > > > scoped_ptr<SSLServerContext> context_; > > scoped_ptr<SSLServerSocket> socket_; > > > > and then forwards all methods to socket_. Then you can pass it around > > standalone. > > We have P2PStreamSocketAdapter that takes ownership of the socket. I think you > can add server_context_ there as well. For the client sockets it will be null. Done. I've added server_context_ to P2PStreamSocketAdapter. Thanks!
lgtm, but please see my comment https://codereview.chromium.org/1518613002/diff/340001/remoting/protocol/ssl_... File remoting/protocol/ssl_hmac_channel_authenticator.cc (right): https://codereview.chromium.org/1518613002/diff/340001/remoting/protocol/ssl_... remoting/protocol/ssl_hmac_channel_authenticator.cc:172: scoped_ptr<net::SSLServerContext> server_context_; move this above socket_. Otherwise it will be destroyed before socket_ and I don't think you want that.
https://codereview.chromium.org/1518613002/diff/340001/remoting/protocol/ssl_... File remoting/protocol/ssl_hmac_channel_authenticator.cc (right): https://codereview.chromium.org/1518613002/diff/340001/remoting/protocol/ssl_... remoting/protocol/ssl_hmac_channel_authenticator.cc:172: scoped_ptr<net::SSLServerContext> server_context_; On 2016/03/07 22:30:24, Sergey Ulanov wrote: > move this above socket_. Otherwise it will be destroyed before socket_ and I > don't think you want that. Done. Thanks!
The CQ bit was checked by ryanchung@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org, sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/1518613002/#ps360001 (title: "Remoting: Moved server_context_ above socket_")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1518613002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1518613002/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by ryanchung@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1518613002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1518613002/360001
The CQ bit was unchecked by ryanchung@chromium.org
The CQ bit was checked by ryanchung@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org, davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1518613002/#ps380001 (title: "Remoting: Fixed initialization ordering of socket_ and server_context_")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1518613002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1518613002/380001
Message was sent while issue was closed.
Description was changed from ========== Support for server session cache. Allows SSL server sockets created through a SSLServerSocketContext to share a single session cache. OpenSSL only. BUG=568650 ========== to ========== Support for server session cache. Allows SSL server sockets created through a SSLServerSocketContext to share a single session cache. OpenSSL only. BUG=568650 ==========
Message was sent while issue was closed.
Committed patchset #20 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== Support for server session cache. Allows SSL server sockets created through a SSLServerSocketContext to share a single session cache. OpenSSL only. BUG=568650 ========== to ========== Support for server session cache. Allows SSL server sockets created through a SSLServerSocketContext to share a single session cache. OpenSSL only. BUG=568650 Committed: https://crrev.com/eb9e3bc7bfc291ad600943b24304946328bcf4f0 Cr-Commit-Position: refs/heads/master@{#379751} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/eb9e3bc7bfc291ad600943b24304946328bcf4f0 Cr-Commit-Position: refs/heads/master@{#379751} |
