Restrict QUIC session pool when channel ID is present.

net/quic/quic_client_session_test.cc

Index: net/quic/quic_client_session_test.cc
diff --git a/net/quic/quic_client_session_test.cc b/net/quic/quic_client_session_test.cc
index 264fbfa52708d69cc545cc15640f6acd24c5a28e..74c45d24fdb58768f15133dd197e23f58db476a6 100644
--- a/net/quic/quic_client_session_test.cc
+++ b/net/quic/quic_client_session_test.cc
@@ -6,11 +6,15 @@
 
 #include <vector>
 
+#include "base/files/file_path.h"
 #include "base/rand_util.h"
 #include "net/base/capturing_net_log.h"
 #include "net/base/test_completion_callback.h"
+#include "net/base/test_data_directory.h"
+#include "net/cert/cert_verify_result.h"
 #include "net/quic/crypto/aes_128_gcm_12_encrypter.h"
 #include "net/quic/crypto/crypto_protocol.h"
+#include "net/quic/crypto/proof_verifier_chromium.h"
 #include "net/quic/crypto/quic_decrypter.h"
 #include "net/quic/crypto/quic_encrypter.h"
 #include "net/quic/crypto/quic_server_info.h"
@@ -20,6 +24,7 @@
 #include "net/quic/test_tools/quic_test_utils.h"
 #include "net/quic/test_tools/simple_quic_framer.h"
 #include "net/socket/socket_test_util.h"
+#include "net/test/cert_test_util.h"
 #include "net/udp/datagram_client_socket.h"
 
 using testing::_;
@@ -61,13 +66,29 @@ class TestPacketWriter : public QuicDefaultPacketWriter {
   QuicPacketHeader header_;
 };
 
+class FakeChannelIDKey : public ChannelIDKey {
+ public:
+  // Sign signs |signed_data| using the ChannelID private key and puts the
+  // signature into |out_signature|. It returns true on success.

[wtc, 2014/07/01 23:00:14]

Replace this comment with something like
  // ChannelIDKey implementation:

[Ryan Hamilton, 2014/07/01 23:26:19]

Done.

+  virtual bool Sign(base::StringPiece signed_data,
+                    std::string* out_signature) const OVERRIDE {
+    return true;

[wtc, 2014/07/01 23:00:15]

I think it's more realistic to set *out_signature to some string. (I know this
method can even return false because it's not invoked by the test.)

[Ryan Hamilton, 2014/07/01 23:26:20]

Done.

+  }
+
+  // SerializeKey returns the serialized ChannelID public key.

[wtc, 2014/07/01 23:00:15]

Delete this comment.

[Ryan Hamilton, 2014/07/01 23:26:19]

Done.

+  virtual std::string SerializeKey() const OVERRIDE {
+    return "";
+  }
+};
+
 class QuicClientSessionTest : public ::testing::TestWithParam<QuicVersion> {
  protected:
   QuicClientSessionTest()
       : writer_(new TestPacketWriter(GetParam())),
         connection_(
             new PacketSavingConnection(false, SupportedVersions(GetParam()))),
-        session_(connection_, GetSocket().Pass(), writer_.Pass(), NULL, NULL,
+        session_(host_port_pair_, connection_, GetSocket().Pass(),
+                 writer_.Pass(), NULL, NULL,
                  make_scoped_ptr((QuicServerInfo*)NULL),
                  QuicServerId(kServerHostname, kServerPort, false,
                               PRIVACY_MODE_DISABLED),
@@ -98,6 +119,7 @@ class QuicClientSessionTest : public ::testing::TestWithParam<QuicVersion> {
   }
 
   scoped_ptr<QuicDefaultPacketWriter> writer_;
+  const HostPortPair host_port_pair_;

[wtc, 2014/07/01 23:00:14]

1. IMPORTANT: we never set this member, so it is an empty string!

2. Please rename this member "server_host_port_", if it refers to the server.

[Ryan Hamilton, 2014/07/01 23:26:20]

Ok, fixed this. Turns out I hadn't actually run the channel ID test. It failed
because this was not set. Now that it's fixed, it passed. Nice catch!

   PacketSavingConnection* connection_;
   CapturingNetLog net_log_;
   MockClientSocketFactory socket_factory_;
@@ -166,6 +188,54 @@ TEST_P(QuicClientSessionTest, GoAwayReceived) {
   EXPECT_EQ(NULL, session_.CreateOutgoingDataStream());
 }
 
+TEST_P(QuicClientSessionTest, CanPool) {
+  // Load a cert that is valid for:
+  //   www.example.org
+  //   mail.example.org
+  //   www.example.com
+  base::FilePath certs_dir = GetTestCertsDirectory();
+
+  CertVerifyResult result;
+  ProofVerifyDetailsChromium details;
+  details.cert_verify_result.verified_cert =
+      ImportCertFromFile(certs_dir, "spdy_pooling.pem");
+  ASSERT_NE(static_cast<X509Certificate*>(NULL),
+            details.cert_verify_result.verified_cert);

[wtc, 2014/07/01 23:00:14]

Nit: why don't we just do

  ASSERT_TRUE(details.cert_verify_result.verified_cert);

If you make this change, make the same change to the next unit test.

[Ryan Hamilton, 2014/07/01 23:26:20]

Done.

+
+  session_.OnProofVerifyDetailsAvailable(details);
+  CompleteCryptoHandshake();
+
+
+  EXPECT_TRUE(session_.CanPool("www.example.org"));
+  EXPECT_TRUE(session_.CanPool("mail.example.org"));
+  EXPECT_TRUE(session_.CanPool("mail.example.com"));
+  EXPECT_FALSE(session_.CanPool("mail.google.com"));
+}
+
+TEST_P(QuicClientSessionTest, ConnectionPooledWithTlsChannelId) {
+  // Load a cert that is valid for:
+  //   www.example.org
+  //   mail.example.org
+  //   www.example.com
+  base::FilePath certs_dir = GetTestCertsDirectory();
+
+  CertVerifyResult result;
+  ProofVerifyDetailsChromium details;
+  details.cert_verify_result.verified_cert =
+      ImportCertFromFile(certs_dir, "spdy_pooling.pem");
+  ASSERT_NE(static_cast<X509Certificate*>(NULL),
+            details.cert_verify_result.verified_cert);
+
+  session_.OnProofVerifyDetailsAvailable(details);
+  QuicClientSessionPeer::SetChannelIDKey(&session_, new FakeChannelIDKey);
+  CompleteCryptoHandshake();

[wtc, 2014/07/01 23:00:14]

It seems safer to call QuicClientSessionPeer::SetChannelIDKey after
CompleteCryptoHandshake.

[Ryan Hamilton, 2014/07/01 23:26:19]

Done.

+
+  EXPECT_TRUE(session_.CanPool("www.example.org"));
+  EXPECT_TRUE(session_.CanPool("mail.example.org"));
+  EXPECT_FALSE(session_.CanPool("mail.example.com"));
+  EXPECT_FALSE(session_.CanPool("mail.google.com"));
+}
+
 }  // namespace
 }  // namespace test
 }  // namespace net