This change introduces a way to tie source address token keys to specific QUIC server configs

net/quic/crypto/quic_crypto_server_config.cc

Index: net/quic/crypto/quic_crypto_server_config.cc
diff --git a/net/quic/crypto/quic_crypto_server_config.cc b/net/quic/crypto/quic_crypto_server_config.cc
index a6c099c875131b0a81c7f391571a451af46aa453..bfcbeb4d7274639fb61e44750f370d617e13192f 100644
--- a/net/quic/crypto/quic_crypto_server_config.cc
+++ b/net/quic/crypto/quic_crypto_server_config.cc
@@ -46,6 +46,19 @@ using std::vector;
 
 namespace net {
 
+namespace {
+
+string ComputeSourceAddressTokenKey(StringPiece source_address_token_secret) {

[wtc, 2014/04/07 18:38:24]

Nit: "Derive" is the usual verb for this operation.

[ramant (doing other things), 2014/04/21 22:39:29]

Done.

+  crypto::HKDF hkdf(source_address_token_secret,
+                    StringPiece() /* no salt */,
+                    "QUIC source address token key",
+                    CryptoSecretBoxer::GetKeySize(),
+                    0 /* no fixed IV needed */);
+  return hkdf.server_write_key().as_string();
+}
+
+}  // namespace
+
 // ClientHelloInfo contains information about a client hello message that is
 // only kept for as long as it's being processed.
 struct ClientHelloInfo {
@@ -183,11 +196,8 @@ QuicCryptoServerConfig::QuicCryptoServerConfig(
       source_address_token_lifetime_secs_(86400),
       server_nonce_strike_register_max_entries_(1 << 10),
       server_nonce_strike_register_window_secs_(120) {
-  crypto::HKDF hkdf(source_address_token_secret, StringPiece() /* no salt */,
-                    "QUIC source address token key",
-                    CryptoSecretBoxer::GetKeySize(),
-                    0 /* no fixed IV needed */);
-  source_address_token_boxer_.SetKey(hkdf.server_write_key());
+  default_source_address_token_boxer_.SetKey(
+      ComputeSourceAddressTokenKey(source_address_token_secret));
 
   // Generate a random key and orbit for server nonces.
   rand->RandBytes(server_nonce_orbit_, sizeof(server_nonce_orbit_));
@@ -386,6 +396,7 @@ bool QuicCryptoServerConfig::SetConfigs(
              parsed_configs.begin();
          i != parsed_configs.end(); ++i) {
       scoped_refptr<Config> config = *i;
+
       ConfigMap::iterator it = configs_.find(config->id);
       if (it != configs_.end()) {
         VLOG(1)
@@ -440,7 +451,11 @@ void QuicCryptoServerConfig::ValidateClientHello(
       new ValidateClientHelloResultCallback::Result(
           client_hello, client_ip, now);
 
+  StringPiece requested_scid;
+  client_hello.GetStringPiece(kSCID, &requested_scid);
+
   uint8 primary_orbit[kOrbitSize];
+  scoped_refptr<Config> requested_config;
   {
     base::AutoLock locked(configs_lock_);
 
@@ -457,10 +472,12 @@ void QuicCryptoServerConfig::ValidateClientHello(
 
       memcpy(primary_orbit, primary_config_->orbit, sizeof(primary_orbit));
     }
+
+    requested_config = GetConfigWithScid(requested_scid);
   }
 
   if (result->error_code == QUIC_NO_ERROR) {
-    EvaluateClientHello(primary_orbit, result, done_cb);
+    EvaluateClientHello(primary_orbit, requested_config, result, done_cb);
   } else {
     done_cb->Run(result);
   }
@@ -529,15 +546,7 @@ QuicErrorCode QuicCryptoServerConfig::ProcessClientHello(
 
     primary_config = primary_config_;
 
-    if (!requested_scid.empty()) {
-      ConfigMap::const_iterator it = configs_.find(requested_scid.as_string());
-      if (it != configs_.end()) {
-        // We'll use the config that the client requested in order to do
-        // key-agreement. Otherwise we'll give it a copy of |primary_config_|
-        // to use.
-        requested_config = it->second;
-      }
-    }
+    requested_config = GetConfigWithScid(requested_scid);
   }
 
   if (validate_chlo_result.error_code != QUIC_NO_ERROR) {
@@ -718,7 +727,10 @@ QuicErrorCode QuicCryptoServerConfig::ProcessClientHello(
   }
   out->SetVector(kVER, supported_version_tags);
   out->SetStringPiece(kSourceAddressTokenTag,
-                      NewSourceAddressToken(client_address, rand, info.now));
+                      NewSourceAddressToken(
+                          requested_config.get(),
+                          client_address, rand,
+                          info.now));
   QuicSocketAddressCoder address_coder(client_address);
   out->SetStringPiece(kCADR, address_coder.Encode());
   out->SetStringPiece(kPUBS, forward_secure_public_value);
@@ -729,6 +741,25 @@ QuicErrorCode QuicCryptoServerConfig::ProcessClientHello(
   return QUIC_NO_ERROR;
 }
 
+scoped_refptr<QuicCryptoServerConfig::Config>
+QuicCryptoServerConfig::GetConfigWithScid(StringPiece requested_scid) const {
+  // In Chromium, we will dead lock if the lock is held by the current thread.
+  // Chromium doesn't have AssertReaderHeld API call.
+  // configs_lock_.AssertReaderHeld();
+
+  if (!requested_scid.empty()) {
+    ConfigMap::const_iterator it = configs_.find(requested_scid.as_string());
+    if (it != configs_.end()) {
+      // We'll use the config that the client requested in order to do
+      // key-agreement. Otherwise we'll give it a copy of |primary_config_|
+      // to use.

[wtc, 2014/04/07 18:38:24]

The comment "Otherwise we'll give it a copy of |primary_config_| to use.", or
perhaps this whole paragraph, should be moved back to the call site at line 549
(or a little earlier, at line 547). The reference to |primary_config_| only
makes sense there.

[ramant (doing other things), 2014/04/21 22:39:29]

Please take a look at the new comments.

Done.

+      return scoped_refptr<Config>(it->second);
+    }
+  }
+
+  return scoped_refptr<Config>();
+}
+
 // ConfigPrimaryTimeLessThan is a comparator that implements "less than" for
 // Config's based on their primary_time.
 // static
@@ -823,7 +854,9 @@ void QuicCryptoServerConfig::SelectNewPrimaryConfig(
   DVLOG(1) << "New primary config.  orbit: "
            << base::HexEncode(
                reinterpret_cast<const char*>(primary_config_->orbit),
-               kOrbitSize);
+               kOrbitSize)
+           << " scid: " << base::HexEncode(primary_config_->id.data(),
+                                           primary_config_->id.size());
   next_config_promotion_time_ = QuicWallTime::Zero();
   if (primary_config_changed_cb_.get() != NULL) {
     primary_config_changed_cb_->Run(primary_config_->id);
@@ -832,6 +865,7 @@ void QuicCryptoServerConfig::SelectNewPrimaryConfig(
 
 void QuicCryptoServerConfig::EvaluateClientHello(
     const uint8* primary_orbit,
+    scoped_refptr<Config> requested_config,
     ValidateClientHelloResultCallback::Result* client_hello_state,
     ValidateClientHelloResultCallback* done_cb) const {
   ValidateClientHelloHelper helper(client_hello_state, done_cb);
@@ -854,8 +888,12 @@ void QuicCryptoServerConfig::EvaluateClientHello(
   }
 
   StringPiece srct;
-  if (client_hello.GetStringPiece(kSourceAddressTokenTag, &srct) &&
-      ValidateSourceAddressToken(srct, info->client_ip, info->now)) {
+  if (requested_config.get() != NULL &&

[wtc, 2014/04/07 18:38:24]

Why do we want to allow requested_config to be NULL? It may be better for the
caller to ensure that requested_config is not NULL.

[ramant (doing other things), 2014/04/21 22:39:29]

Will make this change in internal source code.

+      client_hello.GetStringPiece(kSourceAddressTokenTag, &srct) &&
+      ValidateSourceAddressToken(requested_config.get(),
+                                 srct,
+                                 info->client_ip,
+                                 info->now)) {
     info->valid_source_address_token = true;
   } else {
     // No valid source address token.

[wtc, 2014/04/07 18:38:24]

With the new code, this comment is no longer accurate. Now it should say
something like "No server config with the requested ID, or no valid source
address token".

[ramant (doing other things), 2014/04/21 22:39:29]

Done.

@@ -923,7 +961,11 @@ void QuicCryptoServerConfig::BuildRejection(
   out->set_tag(kREJ);
   out->SetStringPiece(kSCFG, config->serialized);
   out->SetStringPiece(kSourceAddressTokenTag,
-                      NewSourceAddressToken(info.client_ip, rand, info.now));
+                      NewSourceAddressToken(
+                          config,
+                          info.client_ip,
+                          rand,
+                          info.now));
   if (replay_protection_) {
     out->SetStringPiece(kServerNonceTag, NewServerNonce(rand, info.now));
   }
@@ -1013,6 +1055,18 @@ QuicCryptoServerConfig::ParseConfigProtobuf(
   scoped_refptr<Config> config(new Config);
   config->serialized = protobuf->config();
 
+  if (!protobuf->has_source_address_token_secret_override()) {
+    // Use the default boxer.
+    config->source_address_token_boxer = &default_source_address_token_boxer_;
+  } else {
+    // Create override boxer instance.
+    CryptoSecretBoxer* boxer = new CryptoSecretBoxer;
+    boxer->SetKey(ComputeSourceAddressTokenKey(
+        protobuf->source_address_token_secret_override()));
+    config->source_address_token_boxer_storage.reset(boxer);
+    config->source_address_token_boxer = boxer;
+  }
+
   if (protobuf->has_primary_time()) {
     config->primary_time =
         QuicWallTime::FromUNIXSeconds(protobuf->primary_time());
@@ -1044,7 +1098,7 @@ QuicCryptoServerConfig::ParseConfigProtobuf(
 
   StringPiece orbit;
   if (!msg->GetStringPiece(kORBT, &orbit)) {
-    LOG(WARNING) << "Server config message is missing OBIT";
+    LOG(WARNING) << "Server config message is missing ORBT";
     return NULL;
   }
 
@@ -1218,6 +1272,7 @@ void QuicCryptoServerConfig::AcquirePrimaryConfigChangedCb(
 }
 
 string QuicCryptoServerConfig::NewSourceAddressToken(
+    const QuicCryptoServerConfig::Config* config,
     const IPEndPoint& ip,
     QuicRandom* rand,
     QuicWallTime now) const {
@@ -1225,17 +1280,18 @@ string QuicCryptoServerConfig::NewSourceAddressToken(
   source_address_token.set_ip(IPAddressToPackedString(ip.address()));
   source_address_token.set_timestamp(now.ToUNIXSeconds());
 
-  return source_address_token_boxer_.Box(
+  return config->source_address_token_boxer->Box(
       rand, source_address_token.SerializeAsString());
 }
 
 bool QuicCryptoServerConfig::ValidateSourceAddressToken(
+    const QuicCryptoServerConfig::Config* config,
     StringPiece token,
     const IPEndPoint& ip,
     QuicWallTime now) const {
   string storage;
   StringPiece plaintext;
-  if (!source_address_token_boxer_.Unbox(token, &storage, &plaintext)) {
+  if (!config->source_address_token_boxer->Unbox(token, &storage, &plaintext)) {
     return false;
   }
 
@@ -1337,7 +1393,8 @@ QuicCryptoServerConfig::Config::Config()
     : channel_id_enabled(false),
       is_primary(false),
       primary_time(QuicWallTime::Zero()),
-      priority(0) {}
+      priority(0),
+      source_address_token_boxer(NULL) {}
 
 QuicCryptoServerConfig::Config::~Config() { STLDeleteElements(&key_exchanges); }