Added SSLHostInfo. Storing of server host info to our standard disk cache.

Added SSLHostInfo. Storing of server info to our standard disk cache.

Implemented  DiskCacheBasedSSLHostInfo which fetches information about an SSL host from our standard disk cache. Since the information is defined to be non-sensitive, it's ok for us to keep it on disk.

SSLHostInfo persists/restores server's certificates.  Certificates is a vector of DER encoded X.509 certificates, as the server returned them and in the same order.

These changes are the frame work. This code is not enabled to store any data to disk cache.

R=wtc@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246254

[wtc, 2014-01-15 19:08:58]

Review comments on patch set 3:

Please make the changes I suggested and upload a new patch set for future
reference. Thanks.

https://codereview.chromium.org/135373002/diff/1030001/chrome/browser/chrome_...
File chrome/browser/chrome_net_benchmarking_message_filter.cc (right):

https://codereview.chromium.org/135373002/diff/1030001/chrome/browser/chrome_...
chrome/browser/chrome_net_benchmarking_message_filter.cc:41: callback_(

I guess it is not necessary to use ALLOW_THIS_IN_INITIALIZER_LIST any more?

https://codereview.chromium.org/135373002/diff/1030001/chrome/browser/chrome_...
chrome/browser/chrome_net_benchmarking_message_filter.cc:63: // Doom all entries
except those with snapstart information.

Change "snapstart information" to "sslhostinfo".

https://codereview.chromium.org/135373002/diff/1030001/chrome/browser/chrome_...
chrome/browser/chrome_net_benchmarking_message_filter.cc:136: } else {

Remove this "else" because it is after a return statement.

https://codereview.chromium.org/135373002/diff/1030001/chrome/browser/io_thre...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/135373002/diff/1030001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:1037: system_params.ssl_host_info_factory = NULL;

This change is not necessary because the HttpNetworkSession::Params constructor
initializes the ssl_host_info_factory member to NULL.

https://codereview.chromium.org/135373002/diff/1030001/net/base/net_log_event...
File net/base/net_log_event_type_list.h (left):

https://codereview.chromium.org/135373002/diff/1030001/net/base/net_log_event...
net/base/net_log_event_type_list.h:508: // we merged the verification with the
SSLHostInfo. (Note: now obsolete.)

Please undo this change. The SSL_VERIFICATION_MERGED event is still obsolete,
even when we recurrect SSLHostInfo because we won't be predicting server
certificate.

https://codereview.chromium.org/135373002/diff/1030001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/135373002/diff/1030001/net/http/http_cache.cc...
net/http/http_cache.cc:22: #include "base/memory/scoped_ptr.h"

Remove this. This is already included by net/http/http_cache.h.

https://codereview.chromium.org/135373002/diff/1030001/net/http/http_cache.cc...
net/http/http_cache.cc:301: network_layer_.reset(new HttpNetworkLayer(session));

Please change this back to using the initializer list. Pass params.cert_verifier
to the SSLHostInfoFactoryAdaptor constructor. Also change the
ssl_host_info_factory_ and network_layer_ back to const.

https://codereview.chromium.org/135373002/diff/1030001/net/http/http_proxy_cl...
File net/http/http_proxy_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/135373002/diff/1030001/net/http/http_proxy_cl...
net/http/http_proxy_client_socket_pool_unittest.cc:81: NULL /*
cert_transparency_verifier */,

Reverse these two arguments, to match the argument order used in other classes
where ssl_host_info_factory is listed between cert_transparency_verifier and
ssl_session_cache_shard.

https://codereview.chromium.org/135373002/diff/1030001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_unittest.cc (right):

https://codereview.chromium.org/135373002/diff/1030001/net/http/http_stream_f...
net/http/http_stream_factory_impl_unittest.cc:433: NULL /*
cert_transparency_verifier */,

Reverse these two arguments to match the argument order used in other classes.

https://codereview.chromium.org/135373002/diff/1030001/net/http/http_transact...
File net/http/http_transaction.h (right):

https://codereview.chromium.org/135373002/diff/1030001/net/http/http_transact...
net/http/http_transaction.h:139: // TODO(agl): remove this.

Remove "It's used to implement Snap Start." and "TODO(agl): remove this.".

https://codereview.chromium.org/135373002/diff/1030001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/135373002/diff/1030001/net/socket/ssl_client_...
net/socket/ssl_client_socket_nss.cc:111: #include "net/ssl/ssl_config_service.h"

I assume it is necessary to add this header? It's not in the original revert CL.

https://codereview.chromium.org/135373002/diff/1030001/net/socket/ssl_client_...
net/socket/ssl_client_socket_nss.cc:458: bool predicted_cert_chain_correct;

List this right after server_cert.

https://codereview.chromium.org/135373002/diff/1030001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_nss.h (right):

https://codereview.chromium.org/135373002/diff/1030001/net/socket/ssl_client_...
net/socket/ssl_client_socket_nss.h:45: class SSLHostInfo;

This forward declaration is not necessary because it is in ssl_client_socket.h.

https://codereview.chromium.org/135373002/diff/1030001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_pool.cc (right):

https://codereview.chromium.org/135373002/diff/1030001/net/socket/ssl_client_...
net/socket/ssl_client_socket_pool.cc:128: NULL, /* TODO(rtenneti): Fix
SSLHostInfoFactory */

I think a ssl_host_info_factory member should be added to
SSLClientSocketContext.

[ramant (doing other things), 2014-01-18 00:21:55]

PTAL. Thanks very much for thorough code review.

https://codereview.chromium.org/135373002/diff/1030001/chrome/browser/chrome_...
File chrome/browser/chrome_net_benchmarking_message_filter.cc (right):

https://codereview.chromium.org/135373002/diff/1030001/chrome/browser/chrome_...
chrome/browser/chrome_net_benchmarking_message_filter.cc:41: callback_(
On 2014/01/15 19:08:59, wtc wrote:
> 
> I guess it is not necessary to use ALLOW_THIS_IN_INITIALIZER_LIST any more?

We get compiler error if we use ALLOW_THIS_IN_INITIALIZER_LIST.

https://codereview.chromium.org/135373002/diff/1030001/chrome/browser/chrome_...
chrome/browser/chrome_net_benchmarking_message_filter.cc:63: // Doom all entries
except those with snapstart information.
On 2014/01/15 19:08:59, wtc wrote:
> 
> Change "snapstart information" to "sslhostinfo".

Done.

https://codereview.chromium.org/135373002/diff/1030001/chrome/browser/chrome_...
chrome/browser/chrome_net_benchmarking_message_filter.cc:136: } else {
On 2014/01/15 19:08:59, wtc wrote:
> 
> Remove this "else" because it is after a return statement.

Done.

https://codereview.chromium.org/135373002/diff/1030001/chrome/browser/io_thre...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/135373002/diff/1030001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:1037: system_params.ssl_host_info_factory = NULL;
On 2014/01/15 19:08:59, wtc wrote:
> 
> This change is not necessary because the HttpNetworkSession::Params
constructor
> initializes the ssl_host_info_factory member to NULL.

Done.

https://codereview.chromium.org/135373002/diff/1030001/net/base/net_log_event...
File net/base/net_log_event_type_list.h (left):

https://codereview.chromium.org/135373002/diff/1030001/net/base/net_log_event...
net/base/net_log_event_type_list.h:508: // we merged the verification with the
SSLHostInfo. (Note: now obsolete.)
On 2014/01/15 19:08:59, wtc wrote:
> 
> Please undo this change. The SSL_VERIFICATION_MERGED event is still obsolete,
> even when we recurrect SSLHostInfo because we won't be predicting server
> certificate.

Done.

https://codereview.chromium.org/135373002/diff/1030001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/135373002/diff/1030001/net/http/http_cache.cc...
net/http/http_cache.cc:22: #include "base/memory/scoped_ptr.h"
On 2014/01/15 19:08:59, wtc wrote:
> 
> Remove this. This is already included by net/http/http_cache.h.

Done.

https://codereview.chromium.org/135373002/diff/1030001/net/http/http_cache.cc...
net/http/http_cache.cc:301: network_layer_.reset(new HttpNetworkLayer(session));
On 2014/01/15 19:08:59, wtc wrote:
> 
> Please change this back to using the initializer list. Pass
params.cert_verifier
> to the SSLHostInfoFactoryAdaptor constructor. Also change the
> ssl_host_info_factory_ and network_layer_ back to const.

Done.

https://codereview.chromium.org/135373002/diff/1030001/net/http/http_proxy_cl...
File net/http/http_proxy_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/135373002/diff/1030001/net/http/http_proxy_cl...
net/http/http_proxy_client_socket_pool_unittest.cc:81: NULL /*
cert_transparency_verifier */,
On 2014/01/15 19:08:59, wtc wrote:
> 
> Reverse these two arguments, to match the argument order used in other classes
> where ssl_host_info_factory is listed between cert_transparency_verifier and
> ssl_session_cache_shard.

Done.

https://codereview.chromium.org/135373002/diff/1030001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_unittest.cc (right):

https://codereview.chromium.org/135373002/diff/1030001/net/http/http_stream_f...
net/http/http_stream_factory_impl_unittest.cc:433: NULL /*
cert_transparency_verifier */,
On 2014/01/15 19:08:59, wtc wrote:
> 
> Reverse these two arguments to match the argument order used in other classes.

Done.

https://codereview.chromium.org/135373002/diff/1030001/net/http/http_transact...
File net/http/http_transaction.h (right):

https://codereview.chromium.org/135373002/diff/1030001/net/http/http_transact...
net/http/http_transaction.h:139: // TODO(agl): remove this.
On 2014/01/15 19:08:59, wtc wrote:
> 
> Remove "It's used to implement Snap Start." and "TODO(agl): remove this.".

Done.

https://codereview.chromium.org/135373002/diff/1030001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/135373002/diff/1030001/net/socket/ssl_client_...
net/socket/ssl_client_socket_nss.cc:111: #include "net/ssl/ssl_config_service.h"
On 2014/01/15 19:08:59, wtc wrote:
> 
> I assume it is necessary to add this header? It's not in the original revert
CL.

Done.

https://codereview.chromium.org/135373002/diff/1030001/net/socket/ssl_client_...
net/socket/ssl_client_socket_nss.cc:458: bool predicted_cert_chain_correct;
On 2014/01/15 19:08:59, wtc wrote:
> 
> List this right after server_cert.

Done.

https://codereview.chromium.org/135373002/diff/1030001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_nss.h (right):

https://codereview.chromium.org/135373002/diff/1030001/net/socket/ssl_client_...
net/socket/ssl_client_socket_nss.h:45: class SSLHostInfo;
On 2014/01/15 19:08:59, wtc wrote:
> 
> This forward declaration is not necessary because it is in
ssl_client_socket.h.

Done.

https://codereview.chromium.org/135373002/diff/1030001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_pool.cc (right):

https://codereview.chromium.org/135373002/diff/1030001/net/socket/ssl_client_...
net/socket/ssl_client_socket_pool.cc:128: NULL, /* TODO(rtenneti): Fix
SSLHostInfoFactory */
On 2014/01/15 19:08:59, wtc wrote:
> 
> I think a ssl_host_info_factory member should be added to
> SSLClientSocketContext.

It is already in
https://codereview.chromium.org/135373002/diff/1030001/net/socket/ssl_client_...

Am I wrong? thanks.

[ramant (doing other things), 2014-01-18 00:28:46]

https://codereview.chromium.org/135373002/diff/1030001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_pool.cc (right):

https://codereview.chromium.org/135373002/diff/1030001/net/socket/ssl_client_...
net/socket/ssl_client_socket_pool.cc:128: NULL, /* TODO(rtenneti): Fix
SSLHostInfoFactory */
On 2014/01/18 00:21:56, ramant wrote:
> On 2014/01/15 19:08:59, wtc wrote:
> > 
> > I think a ssl_host_info_factory member should be added to
> > SSLClientSocketContext.
> 
> It is already in
>
https://codereview.chromium.org/135373002/diff/1030001/net/socket/ssl_client_...
> 
> Am I wrong? thanks.
> 

Passed  context.ssl_host_info_factory and removed the TODO.

[wtc, 2014-01-18 01:02:00]

Review comments on patch set 5:

Just three nits. Everything else looks good. Thanks!

https://codereview.chromium.org/135373002/diff/1600002/chrome/browser/chrome_...
File chrome/browser/chrome_net_benchmarking_message_filter.cc (right):

https://codereview.chromium.org/135373002/diff/1600002/chrome/browser/chrome_...
chrome/browser/chrome_net_benchmarking_message_filter.cc:63: // Doom all entries
except those with sslhostinfo information.

Nit: delete "information".

https://codereview.chromium.org/135373002/diff/1600002/net/dns/address_sorter...
File net/dns/address_sorter_posix_unittest.cc (right):

https://codereview.chromium.org/135373002/diff/1600002/net/dns/address_sorter...
net/dns/address_sorter_posix_unittest.cc:12: #include
"net/cert/x509_certificate.h"

Is it necessary to add this header?

https://codereview.chromium.org/135373002/diff/1600002/net/http/http_cache.h
File net/http/http_cache.h (right):

https://codereview.chromium.org/135373002/diff/1600002/net/http/http_cache.h#...
net/http/http_cache.h:388: // TODO(rtenneti): network_layer_ should be const.

Remove the TODO comments on lines 385 and 388.

[ramant (doing other things), 2014-01-18 01:28:53]

PTAL

https://codereview.chromium.org/135373002/diff/1600002/chrome/browser/chrome_...
File chrome/browser/chrome_net_benchmarking_message_filter.cc (right):

https://codereview.chromium.org/135373002/diff/1600002/chrome/browser/chrome_...
chrome/browser/chrome_net_benchmarking_message_filter.cc:63: // Doom all entries
except those with sslhostinfo information.
On 2014/01/18 01:02:01, wtc wrote:
> 
> Nit: delete "information".

Done.

https://codereview.chromium.org/135373002/diff/1600002/net/dns/address_sorter...
File net/dns/address_sorter_posix_unittest.cc (right):

https://codereview.chromium.org/135373002/diff/1600002/net/dns/address_sorter...
net/dns/address_sorter_posix_unittest.cc:12: #include
"net/cert/x509_certificate.h"
On 2014/01/18 01:02:01, wtc wrote:
> 
> Is it necessary to add this header?

ssl_host_info.h did a forward declaration of X509Certificate and we get compile
error because ref_ptr doesn't know about X509Certificate.

http://build.chromium.org/p/tryserver.chromium/builders/linux_redux/builds/13...

...
../../base/memory/ref_counted.h:310:7: error: invalid use of incomplete type
'struct net::X509Certificate'
../../net/cert/cert_verifier.h:19:7: error: forward declaration of 'struct
net::X509Certificate'
...

https://codereview.chromium.org/135373002/diff/1600002/net/http/http_cache.h
File net/http/http_cache.h (right):

https://codereview.chromium.org/135373002/diff/1600002/net/http/http_cache.h#...
net/http/http_cache.h:388: // TODO(rtenneti): network_layer_ should be const.
On 2014/01/18 01:02:01, wtc wrote:
> 
> Remove the TODO comments on lines 385 and 388.

Done.

[wtc, 2014-01-18 01:43:33]

Patch set 6 is good. Thanks!

[ramant (doing other things), 2014-01-18 04:16:31]

Hi Want-Teh,
  Deleted SSLHostInfo related changes from SSLClientSocket and all non-net
directories. PTAL.

thanks
raman

[wtc, 2014-01-21 23:07:19]

Patch set 7 LGTM.

Please update the CL's description to make sure it describes what was actually
resurrected in the end.

I will also test this patch. Thanks!

https://codereview.chromium.org/135373002/diff/1480002/net/http/http_stream_f...
File net/http/http_stream_factory_impl_unittest.cc (right):

https://codereview.chromium.org/135373002/diff/1480002/net/http/http_stream_f...
net/http/http_stream_factory_impl_unittest.cc:439: NULL /* net_log */),

Nit: you can use C++ style comments. They will need to be after the commas
though.

https://codereview.chromium.org/135373002/diff/1480002/net/socket/client_sock...
File net/socket/client_socket_pool_manager_impl.h (right):

https://codereview.chromium.org/135373002/diff/1480002/net/socket/client_sock...
net/socket/client_socket_pool_manager_impl.h:68: SSLHostInfoFactory*
ssl_host_info_factory,

We should not need the changes to this file and to
client_socket_pool_manager_impl.cc. Please remove these two files from the CL.

[wtc, 2014-01-21 23:23:33]

Raman: I've tested patch set 7. I suggest one more change.

https://codereview.chromium.org/135373002/diff/1480002/net/http/http_network_...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/135373002/diff/1480002/net/http/http_network_...
net/http/http_network_session.cc:46: params.ssl_host_info_factory,

Remove this line, then you don't need to change ClientSocketPoolManagerImpl.

[ramant (doing other things), 2014-01-22 02:28:11]

Hi Wan-Teh,
  Would appreciate your comments about the CL description.

thanks
raman

https://codereview.chromium.org/135373002/diff/1480002/net/http/http_network_...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/135373002/diff/1480002/net/http/http_network_...
net/http/http_network_session.cc:46: params.ssl_host_info_factory,
On 2014/01/21 23:23:34, wtc wrote:
> 
> Remove this line, then you don't need to change ClientSocketPoolManagerImpl.

Done.

https://codereview.chromium.org/135373002/diff/1480002/net/http/http_stream_f...
File net/http/http_stream_factory_impl_unittest.cc (right):

https://codereview.chromium.org/135373002/diff/1480002/net/http/http_stream_f...
net/http/http_stream_factory_impl_unittest.cc:439: NULL /* net_log */),
On 2014/01/21 23:07:20, wtc wrote:
> 
> Nit: you can use C++ style comments. They will need to be after the commas
> though.

Done.

https://codereview.chromium.org/135373002/diff/1480002/net/socket/client_sock...
File net/socket/client_socket_pool_manager_impl.h (right):

https://codereview.chromium.org/135373002/diff/1480002/net/socket/client_sock...
net/socket/client_socket_pool_manager_impl.h:68: SSLHostInfoFactory*
ssl_host_info_factory,
On 2014/01/21 23:07:20, wtc wrote:
> 
> We should not need the changes to this file and to
> client_socket_pool_manager_impl.cc. Please remove these two files from the CL.

Done.

[commit-bot: I haz the power, 2014-01-22 02:48:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/135373002/2580001

[commit-bot: I haz the power, 2014-01-22 09:23:29]

Change committed as 246254

[rvargas (doing something else), 2014-01-23 01:18:32]

https://codereview.chromium.org/135373002/diff/2580001/net/http/http_cache_tr...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/135373002/diff/2580001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:35: #include
"net/http/disk_cache_based_ssl_host_info.h"
seems not needed.

https://codereview.chromium.org/135373002/diff/2580001/net/http/http_transact...
File net/http/http_transaction.h (right):

https://codereview.chromium.org/135373002/diff/2580001/net/http/http_transact...
net/http/http_transaction.h:139: virtual void SetSSLHostInfo(SSLHostInfo*) {};
It seems better to maintain the pattern of the rest of the class and don't
provide a default implementation.

[ramant (doing other things), 2014-01-23 18:50:03]

Made the changes in the following CL:

https://codereview.chromium.org/143073006/

thanks
raman

https://codereview.chromium.org/135373002/diff/2580001/net/http/http_cache_tr...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/135373002/diff/2580001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:35: #include
"net/http/disk_cache_based_ssl_host_info.h"
On 2014/01/23 01:18:32, rvargas wrote:
> seems not needed.

Done.

https://codereview.chromium.org/135373002/diff/2580001/net/http/http_transact...
File net/http/http_transaction.h (right):

https://codereview.chromium.org/135373002/diff/2580001/net/http/http_transact...
net/http/http_transaction.h:139: virtual void SetSSLHostInfo(SSLHostInfo*) {};
On 2014/01/23 01:18:32, rvargas wrote:
> It seems better to maintain the pattern of the rest of the class and don't
> provide a default implementation.

Done.