QUIC Crypto - return the reasons for reject message. Reject reason

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 c8114ff110b27e094027b079890fe27b542056db..f20441f402fc3c3e5a84176ae13869685e6343eb 100644
--- a/net/quic/crypto/quic_crypto_server_config.cc
+++ b/net/quic/crypto/quic_crypto_server_config.cc
@@ -33,6 +33,7 @@
 #include "net/quic/crypto/strike_register.h"
 #include "net/quic/crypto/strike_register_client.h"
 #include "net/quic/quic_clock.h"
+#include "net/quic/quic_flags.h"
 #include "net/quic/quic_protocol.h"
 #include "net/quic/quic_socket_address_coder.h"
 #include "net/quic/quic_utils.h"
@@ -81,6 +82,9 @@ struct ClientHelloInfo {
   StringPiece client_nonce;
   StringPiece server_nonce;
   StringPiece user_agent_id;
+
+  // Errors from EvaluateClientHello.
+  vector<HandshakeFailureReason> reject_reasons;

[wtc, 2014/06/19 00:13:24]

Since reject_reasons is passed to SetVector(), we need to ensure that
sizeof(HandshakeFailureReason) is the same on all platforms.

I recommend declare |reject_reasons| as a vector<QuicTag> (or QuicTagVector?).
QuicTag has a fixed width and it is the type assumed on the client side.

This may require static_cast<QuicTag> when adding a HandshakeFailureReason value
to this vector.

[ramant (doing other things), 2014/06/19 01:57:35]

Done.

[avd, 2014/06/19 02:27:54]

QuicTag has a very specific meaning, and this is not it.  I think you should be
doing static_cast<uint32> if anything.

[ramant (doing other things), 2014/06/19 17:25:48]

Changed it vector<uint32> and added static_cast<uint32>. A compromise.

Done.

 };
 
 struct ValidateClientHelloResultCallback::Result {
@@ -146,6 +150,11 @@ class VerifyNonceIsValidAndUniqueCallback
   virtual void RunImpl(bool nonce_is_valid_and_unique) OVERRIDE {
     DVLOG(1) << "Using client nonce, unique: " << nonce_is_valid_and_unique;
     result_->info.unique = nonce_is_valid_and_unique;
+    // TODO(rtenneti): Implement capturing of error from strike register.
+    // Temporarily treat them as CLIENT_NONCE_UNKNOWN_FAILURE.
+    if (!nonce_is_valid_and_unique) {
+      result_->info.reject_reasons.push_back(CLIENT_NONCE_UNKNOWN_FAILURE);
+    }
     done_cb_->Run(result_);
   }
 
@@ -888,28 +897,57 @@ void QuicCryptoServerConfig::EvaluateClientHello(
 
   client_hello.GetStringPiece(kUAID, &info->user_agent_id);
 
-  StringPiece srct;
-  if (requested_config.get() != NULL &&
-      client_hello.GetStringPiece(kSourceAddressTokenTag, &srct) &&
-      ValidateSourceAddressToken(*requested_config,
-                                 srct,
-                                 info->client_ip,
-                                 info->now)) {
-    info->valid_source_address_token = true;
-  } else {
-    // No server config with the requested ID, or no valid source address token.
+  if (!requested_config.get()) {
+    StringPiece requested_scid;
+    client_hello.GetStringPiece(kSCID, &requested_scid);
+    if (requested_scid.empty()) {
+      info->reject_reasons.push_back(SERVER_CONFIG_INCHOATE_HELLO_FAILURE);
+    } else {
+      info->reject_reasons.push_back(SERVER_CONFIG_UNKNOWN_CONFIG_FAILURE);
+    }

[wtc, 2014/06/19 00:13:24]

Nit: require lines 902-907 as follows:

  if (client_hello.GetStringPiece(kSCID, &requested_scid)) {
    info->reject_reasons.push_back(SERVER_CONFIG_UNKNOWN_CONFIG_FAILURE);
  } else {
    info->reject_reasons.push_back(SERVER_CONFIG_INCHOATE_HELLO_FAILURE);
  }

[ramant (doing other things), 2014/06/19 01:57:34]

Done.

+    // No server config with the requested ID.

[wtc, 2014/06/19 00:13:24]

It is possible to keep going in this case, although the
ValidateSourceAddressToken test, which requirea a non-null requested_config,
must be skipped.

[ramant (doing other things), 2014/06/19 01:57:34]

avd suggested we do an early return. Will submit a separate CL with that change
so that we can discuss in it. Hope you don't mind.

     helper.ValidationComplete(QUIC_NO_ERROR, "");
     return;
   }
 
+  HandshakeFailureReason source_address_token_error;
+  StringPiece srct;
+  DCHECK(requested_config.get());

[wtc, 2014/06/19 00:13:24]

This DCHECK's condition is ensured by the test on line 900 and the return on
line 910. So I would omit this DCHECK. Up to you.

[ramant (doing other things), 2014/06/19 01:57:35]

Done.

+  if (client_hello.GetStringPiece(kSourceAddressTokenTag, &srct)) {
+    source_address_token_error =
+        ValidateSourceAddressToken(*requested_config,
+                                   srct,
+                                   info->client_ip,
+                                   info->now);
+    info->valid_source_address_token =
+        (source_address_token_error == HANDSHAKE_OK);
+  } else {
+    source_address_token_error = SOURCE_ADDRESS_TOKEN_INVALID_FAILURE;
+  }
+
+  bool found_error = false;
+  if (source_address_token_error != HANDSHAKE_OK) {
+    info->reject_reasons.push_back(source_address_token_error);
+    // No valid source address token.
+    if (FLAGS_use_early_return_when_verifying_chlo) {
+      helper.ValidationComplete(QUIC_NO_ERROR, "");
+      return;
+    }
+    found_error = true;
+  }
+
   if (client_hello.GetStringPiece(kNONC, &info->client_nonce) &&
       info->client_nonce.size() == kNonceSize) {
     info->client_nonce_well_formed = true;
   } else {
+    info->reject_reasons.push_back(CLIENT_NONCE_INVALID_FAILURE);
     // Invalid client nonce.
     DVLOG(1) << "Invalid client nonce.";
-    helper.ValidationComplete(QUIC_NO_ERROR, "");
-    return;
+    if (FLAGS_use_early_return_when_verifying_chlo) {
+      helper.ValidationComplete(QUIC_NO_ERROR, "");
+      return;
+    }
+    found_error = true;
   }
 
   if (!replay_protection_) {
@@ -922,8 +960,23 @@ void QuicCryptoServerConfig::EvaluateClientHello(
   client_hello.GetStringPiece(kServerNonceTag, &info->server_nonce);
   if (!info->server_nonce.empty()) {
     // If the server nonce is present, use it to establish uniqueness.
-    info->unique = ValidateServerNonce(info->server_nonce, info->now);
+    HandshakeFailureReason server_nonce_error =
+        ValidateServerNonce(info->server_nonce, info->now);
+    if (server_nonce_error == HANDSHAKE_OK) {
+      info->unique = true;
+    } else {
+      info->reject_reasons.push_back(server_nonce_error);
+      info->unique = false;
+    }
     DVLOG(1) << "Using server nonce, unique: " << info->unique;
+    if (FLAGS_use_early_return_when_verifying_chlo) {
+      helper.ValidationComplete(QUIC_NO_ERROR, "");
+      return;
+    }
+    found_error = true;

[wtc, 2014/06/19 00:13:24]

IMPORTANT: replace lines 972-976 with:

    helper.ValidationComplete(QUIC_NO_ERROR, "");
    return;

Your new code ends up with the same outcome but is confusing because it sets
|found_error| to true.

[ramant (doing other things), 2014/06/19 01:57:34]

Done.

+  }
+
+  if (found_error) {

[wtc, 2014/06/19 00:13:24]

Please add a comment to explain why you do not keep going at this point. The
reason is not obvious.

[ramant (doing other things), 2014/06/19 01:57:34]

Done.

     helper.ValidationComplete(QUIC_NO_ERROR, "");
     return;
   }
@@ -971,6 +1024,12 @@ void QuicCryptoServerConfig::BuildRejection(
     out->SetStringPiece(kServerNonceTag, NewServerNonce(rand, info.now));
   }
 
+  if (FLAGS_send_quic_crypto_reject_reason) {
+    // Send client the reject reason for debugging purposes.
+    DCHECK_LT(0u, info.reject_reasons.size());
+    out->SetVector(kRejectReason, info.reject_reasons);
+  }
+
   // The client may have requested a certificate chain.
   const QuicTag* their_proof_demands;
   size_t num_their_proof_demands;
@@ -1288,7 +1347,7 @@ string QuicCryptoServerConfig::NewSourceAddressToken(const Config& config,
       rand, source_address_token.SerializeAsString());
 }
 
-bool QuicCryptoServerConfig::ValidateSourceAddressToken(
+HandshakeFailureReason QuicCryptoServerConfig::ValidateSourceAddressToken(
     const Config& config,
     StringPiece token,
     const IPEndPoint& ip,
@@ -1296,13 +1355,13 @@ bool QuicCryptoServerConfig::ValidateSourceAddressToken(
   string storage;
   StringPiece plaintext;
   if (!config.source_address_token_boxer->Unbox(token, &storage, &plaintext)) {
-    return false;
+    return SOURCE_ADDRESS_TOKEN_DECRYPTION_FAILURE;
   }
 
   SourceAddressToken source_address_token;
   if (!source_address_token.ParseFromArray(plaintext.data(),
                                            plaintext.size())) {
-    return false;
+    return SOURCE_ADDRESS_TOKEN_PARSE_FAILURE;
   }
 
   IPAddressNumber ip_address = ip.address();
@@ -1311,7 +1370,7 @@ bool QuicCryptoServerConfig::ValidateSourceAddressToken(
   }
   if (source_address_token.ip() != IPAddressToPackedString(ip_address)) {
     // It's for a different IP address.
-    return false;
+    return SOURCE_ADDRESS_TOKEN_DIFFERENT_IP_ADDRESS_FAILURE;
   }
 
   const QuicWallTime timestamp(
@@ -1320,15 +1379,15 @@ bool QuicCryptoServerConfig::ValidateSourceAddressToken(
 
   if (now.IsBefore(timestamp) &&
       delta.ToSeconds() > source_address_token_future_secs_) {
-    return false;
+    return SOURCE_ADDRESS_TOKEN_CLOCK_SKEW_FAILURE;
   }
 
   if (now.IsAfter(timestamp) &&
       delta.ToSeconds() > source_address_token_lifetime_secs_) {
-    return false;
+    return SOURCE_ADDRESS_TOKEN_EXPIRED_FAILURE;
   }
 
-  return true;
+  return HANDSHAKE_OK;
 }
 
 // kServerNoncePlaintextSize is the number of bytes in an unencrypted server
@@ -1354,12 +1413,13 @@ string QuicCryptoServerConfig::NewServerNonce(QuicRandom* rand,
       StringPiece(reinterpret_cast<char*>(server_nonce), sizeof(server_nonce)));
 }
 
-bool QuicCryptoServerConfig::ValidateServerNonce(StringPiece token,
-                                                 QuicWallTime now) const {
+HandshakeFailureReason QuicCryptoServerConfig::ValidateServerNonce(
+    StringPiece token,
+    QuicWallTime now) const {
   string storage;
   StringPiece plaintext;
   if (!server_nonce_boxer_.Unbox(token, &storage, &plaintext)) {
-    return false;
+    return SERVER_NONCE_DECRYPTION_FAILURE;
   }
 
   // plaintext contains:
@@ -1369,7 +1429,7 @@ bool QuicCryptoServerConfig::ValidateServerNonce(StringPiece token,
   if (plaintext.size() != kServerNoncePlaintextSize) {
     // This should never happen because the value decrypted correctly.
     LOG(DFATAL) << "Seemingly valid server nonce had incorrect length.";
-    return false;
+    return SERVER_NONCE_INVALID_FAILURE;
   }
 
   uint8 server_nonce[32];
@@ -1394,7 +1454,7 @@ bool QuicCryptoServerConfig::ValidateServerNonce(StringPiece token,
         server_nonce, static_cast<uint32>(now.ToUNIXSeconds()));
   }
 
-  return is_unique;
+  return is_unique ? HANDSHAKE_OK : SERVER_NONCE_NOT_UNIQUE_FAILURE;
 }
 
 QuicCryptoServerConfig::Config::Config()