Persist server's crypto config data to disk cache for 0-RTT

net/quic/crypto/quic_crypto_client_config.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/quic/crypto/quic_crypto_client_config.h"
 
 #include "base/stl_util.h"
+#include "net/base/completion_callback.h"
+#include "net/base/net_errors.h"
 #include "net/quic/crypto/cert_compressor.h"
 #include "net/quic/crypto/channel_id.h"
 #include "net/quic/crypto/common_cert_set.h"
 #include "net/quic/crypto/crypto_framer.h"
 #include "net/quic/crypto/crypto_utils.h"
 #include "net/quic/crypto/curve25519_key_exchange.h"
 #include "net/quic/crypto/key_exchange.h"
 #include "net/quic/crypto/p256_key_exchange.h"
 #include "net/quic/crypto/proof_verifier.h"
 #include "net/quic/crypto/quic_encrypter.h"
@@ skipping 13 matching lines @@
 
 QuicCryptoClientConfig::QuicCryptoClientConfig() {
 }
 
 QuicCryptoClientConfig::~QuicCryptoClientConfig() {
   STLDeleteValues(&cached_states_);
 }
 
 QuicCryptoClientConfig::CachedState::CachedState()
     : server_config_valid_(false),
+      need_to_persist_(false),
       generation_counter_(0) {}
 
 QuicCryptoClientConfig::CachedState::CachedState(
     scoped_ptr<QuicServerInfo> quic_server_info)
     : server_config_valid_(false),
+      need_to_persist_(false),
       generation_counter_(0),
       quic_server_info_(quic_server_info.Pass()) {}
 
 QuicCryptoClientConfig::CachedState::~CachedState() {}
 
 bool QuicCryptoClientConfig::CachedState::IsComplete(QuicWallTime now) const {
   if (server_config_.empty() || !server_config_valid_) {
     return false;
   }
 
@@ skipping 101 matching lines @@
   certs_.clear();
   server_config_sig_.clear();
 }
 
 void QuicCryptoClientConfig::CachedState::SetProofValid() {
   server_config_valid_ = true;
 }
 
 void QuicCryptoClientConfig::CachedState::SetProofInvalid() {
   server_config_valid_ = false;
+  need_to_persist_ = true;

[wtc, 2014/02/07 00:54:11]

IMPORTANT: is this a bug? It seems that we should only persist if the proof is
valid, but not if the proof is invalid.

[ramant (doing other things), 2014/02/07 20:30:51]

Added need_to_persist_ = true in SetProof and only when certs have changed.
WDYT?

   ++generation_counter_;
 }
 
 const string& QuicCryptoClientConfig::CachedState::server_config() const {
   return server_config_;
 }
 
 const string&
 QuicCryptoClientConfig::CachedState::source_address_token() const {
   return source_address_token_;
@@ skipping 13 matching lines @@
 
 uint64 QuicCryptoClientConfig::CachedState::generation_counter() const {
   return generation_counter_;
 }
 
 const ProofVerifyDetails*
 QuicCryptoClientConfig::CachedState::proof_verify_details() const {
   return proof_verify_details_.get();
 }
 
+QuicServerInfo* QuicCryptoClientConfig::CachedState::quic_server_info() const {
+  return quic_server_info_.get();
+}
+
 void QuicCryptoClientConfig::CachedState::set_source_address_token(
     StringPiece token) {
   source_address_token_ = token.as_string();
 }
 
 void QuicCryptoClientConfig::CachedState::SetProofVerifyDetails(
     ProofVerifyDetails* details) {
   proof_verify_details_.reset(details);
 }
 
 void QuicCryptoClientConfig::CachedState::InitializeFrom(
     const QuicCryptoClientConfig::CachedState& other) {
   DCHECK(server_config_.empty());
   DCHECK(!server_config_valid_);
   server_config_ = other.server_config_;
   source_address_token_ = other.source_address_token_;
   certs_ = other.certs_;
   server_config_sig_ = other.server_config_sig_;
   server_config_valid_ = other.server_config_valid_;
 }
 
+void QuicCryptoClientConfig::CachedState::LoadQuicServerInfo() {
+  if (proof_valid() || !signature().empty()) {

[wtc, 2014/02/07 00:54:11]

1. Why do you test !signature().empty()?

2. Since we are in the same class, we should use the members proof_valid_,
signature_, quic_server_info_ directly, rather than their getter methods.

Please check all the new code you wrote for this issue.

[ramant (doing other things), 2014/02/07 20:30:51]

Made the changes to access members directly.
Was worried about the race condition, that multiple requests for the same URL
were going on at the same time.

For the first request, we are waiting for the data to be read from disk cache.
The second request might have sent CHLO (without certs) and server has sent back
REJ message with certs and we are in the process of verifying proof.

Meanwhile first request has finished reading the data from the disk and trying
to update the data with what it has read.

This change says, we will use the data from disk cache only if the cache object
doesn't have data. What do you think?

+    return;
+  }
+  DCHECK(quic_server_info());
+
+  const QuicServerInfo::State& state(quic_server_info_->state());
+  if (state.certs_.empty()) {
+    return;
+  }
+
+  server_config_ = state.server_config_;
+  source_address_token_ = state.source_address_token_;
+  server_config_sig_ = state.server_config_sig_;
+  for (size_t i = 0; i < state.certs_.size(); i++) {
+    certs_.push_back(state.certs_[i]);
+  }

[wtc, 2014/02/07 00:54:11]

I think we should be able to assign a std::vector directly:

  certs_ = state.certs_;

[ramant (doing other things), 2014/02/07 20:30:51]

Done.

+  need_to_persist_ = false;
+}
+
+void QuicCryptoClientConfig::CachedState::SaveQuicServerInfo() {

[wtc, 2014/02/07 00:54:11]

IMPORTANT: these two methods show a duplication of data in CachedState and
QuicServerInfo. We should eliminate the duplication of data.

This may mean combining CachedState and QuicServerInfo into one. For example,
CachedState can implement the QuicServerInfo interface. Other solutions are
possible.

[ramant (doing other things), 2014/02/07 20:30:51]

Added a TODO for the above. Possible race condition between reading from disk
cache and another request updating the cached state.

+  if (!quic_server_info() || !proof_valid() || signature().empty() ||
+      !need_to_persist_) {

[wtc, 2014/02/07 00:54:11]

It seems that if we set need_to_persist_ appropriately, we just need to test
!need_to_persist_ here.

[ramant (doing other things), 2014/02/07 20:30:51]

Done.

+    return;
+  }
+
+  // If the QuicServerInfo hasn't managed to load from disk yet then we can't
+  // save anything.
+  if (quic_server_info_->WaitForDataReady(CompletionCallback()) != OK) {
+    return;
+  }
+
+  QuicServerInfo::State* state = quic_server_info_->mutable_state();
+
+  state->Clear();
+  state->server_config_ = server_config_;
+  state->source_address_token_ = source_address_token_;
+  state->server_config_sig_ = server_config_sig_;
+  for (size_t i = 0; i < certs_.size(); i++) {
+    state->certs_.push_back(certs_[i]);
+  }

[wtc, 2014/02/07 00:54:11]

An assignment should work:

  state->certs_ = certs_;

[ramant (doing other things), 2014/02/07 20:30:51]

Done.

+
+  quic_server_info_->Persist();
+}
+
 void QuicCryptoClientConfig::SetDefaults() {
   // Key exchange methods.
   kexs.resize(2);
   kexs[0] = kC255;
   kexs[1] = kP256;
 
   // Authenticated encryption algorithms.
   aead.resize(1);
   aead[0] = kAESG;
 }
@@ skipping 436 matching lines @@
   CachedState* canonical_cached =
       canonical_crypto_config->LookupOrCreate(canonical_server_hostname);
   if (!canonical_cached->proof_valid()) {
     return;
   }
   CachedState* cached = LookupOrCreate(server_hostname);
   cached->InitializeFrom(*canonical_cached);
 }
 
 }  // namespace net