Cleanup of OpenSSL/NSS implementation of ProofVerfifier release.

Implemented comments from wtc in CL https://chromiumcodereview.appspot.com/17385010/#ps170001 (Patch Set 12).

R=wtc@chromium.org

Review URL: https://chromiumcodereview.appspot.com/18033005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@210095 0039d316-1c4b-4281-b951-d872f2087c98

12 files changed, 45 insertions, 49 deletions

diff --git a/net/data/ssl/certificates/README b/net/data/ssl/certificates/README
index 84e65e9..3fea6a9 100644
--- a/net/data/ssl/certificates/README
+++ b/net/data/ssl/certificates/README
@@ -217,5 +217,5 @@ unit tests.
 - quic_intermediate.crt
 - quic_test_ecc.example.com.crt
 - quic_test.example.com.crt
-- quic_proof_verify.crt
+- quic_root.crt
      These certificates are used by the ProofVerifier's unit tests of QUIC.
diff --git a/net/data/ssl/certificates/quic_proof_verify.crt b/net/data/ssl/certificates/quic_root.crt
index 55502e6..55502e6 100644
--- a/net/data/ssl/certificates/quic_proof_verify.crt
+++ b/net/data/ssl/certificates/quic_root.crt
diff --git a/net/quic/crypto/crypto_handshake.cc b/net/quic/crypto/crypto_handshake.cc
index 66a83de..abac98a 100644
--- a/net/quic/crypto/crypto_handshake.cc
+++ b/net/quic/crypto/crypto_handshake.cc
@@ -430,8 +430,7 @@ QuicErrorCode QuicCryptoClientConfig::CachedState::SetServerConfig(
 
   if (!matches_existing) {
     server_config_ = server_config.as_string();
-    server_config_valid_ = false;
-    ++generation_counter_;
+    SetProofInvalid();
     scfg_.reset(new_scfg_storage.release());
   }
   return QUIC_NO_ERROR;
@@ -440,8 +439,7 @@ QuicErrorCode QuicCryptoClientConfig::CachedState::SetServerConfig(
 void QuicCryptoClientConfig::CachedState::InvalidateServerConfig() {
   server_config_.clear();
   scfg_.reset();
-  server_config_valid_ = false;
-  ++generation_counter_;
+  SetProofInvalid();
 }
 
 void QuicCryptoClientConfig::CachedState::SetProof(const vector<string>& certs,
@@ -463,8 +461,7 @@ void QuicCryptoClientConfig::CachedState::SetProof(const vector<string>& certs,
   }
 
   // If the proof has changed then it needs to be revalidated.
-  server_config_valid_ = false;
-  ++generation_counter_;
+  SetProofInvalid();
   certs_ = certs;
   server_config_sig_ = signature.as_string();
 }
@@ -473,6 +470,11 @@ void QuicCryptoClientConfig::CachedState::SetProofValid() {
   server_config_valid_ = true;
 }
 
+void QuicCryptoClientConfig::CachedState::SetProofInvalid() {
+  server_config_valid_ = false;
+  ++generation_counter_;
+}
+
 const string& QuicCryptoClientConfig::CachedState::server_config() const {
   return server_config_;
 }
diff --git a/net/quic/crypto/crypto_handshake.h b/net/quic/crypto/crypto_handshake.h
index e702bb6..89e707b 100644
--- a/net/quic/crypto/crypto_handshake.h
+++ b/net/quic/crypto/crypto_handshake.h
@@ -268,6 +268,11 @@ class NET_EXPORT_PRIVATE QuicCryptoClientConfig : public QuicCryptoConfig {
     // (Note: this does not check the chain or signature.)
     void SetProofValid();
 
+    // If the server config or the proof has changed then it needs to be
+    // revalidated. Helper function to keep server_config_valid_ and
+    // generation_counter_ in sync.
+    void SetProofInvalid();
+
     const std::string& server_config() const;
     const std::string& source_address_token() const;
     const std::vector<std::string>& certs() const;
@@ -287,9 +292,10 @@ class NET_EXPORT_PRIVATE QuicCryptoClientConfig : public QuicCryptoConfig {
     bool server_config_valid_;          // True if |server_config_| is correctly
                                         // signed and |certs_| has been
                                         // validated.
-    uint64 generation_counter_;         // Generation counter associated with
-                                        // the |server_config_|, |certs_| and
-                                        // |server_config_sig_| combination.
+    // Generation counter associated with the |server_config_|, |certs_| and
+    // |server_config_sig_| combination. It is incremented whenever we set
+    // server_config_valid_ to false.
+    uint64 generation_counter_;
 
     // scfg contains the cached, parsed value of |server_config|.
     mutable scoped_ptr<CryptoHandshakeMessage> scfg_;
diff --git a/net/quic/crypto/proof_test.cc b/net/quic/crypto/proof_test.cc
index b30c9aa..7b38546 100644
--- a/net/quic/crypto/proof_test.cc
+++ b/net/quic/crypto/proof_test.cc
@@ -13,10 +13,6 @@
 #include "net/test/cert_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-#if defined(OS_WIN)
-#include "base/win/windows_version.h"
-#endif
-
 using std::string;
 using std::vector;
 
@@ -264,7 +260,9 @@ TEST(Proof, VerifyRSAKnownAnswerTest) {
 
 // A known answer test that allows us to test ProofVerifier without a working
 // ProofSource.
-// TODO(rtenneti): Enable VerifyECDSAKnownAnswerTest on win_rel and XP.
+// TODO(rtenneti): Enable VerifyECDSAKnownAnswerTest on Windows. Disabled this
+// test because X509Certificate::GetPublicKeyInfo is not returning the correct
+// type for ECDSA certificates.
 #if defined(OS_WIN)
 #define MAYBE_VerifyECDSAKnownAnswerTest DISABLED_VerifyECDSAKnownAnswerTest
 #else
diff --git a/net/quic/crypto/proof_verifier_chromium.cc b/net/quic/crypto/proof_verifier_chromium.cc
index 7a764b6..0c0b71f 100644
--- a/net/quic/crypto/proof_verifier_chromium.cc
+++ b/net/quic/crypto/proof_verifier_chromium.cc
@@ -40,10 +40,6 @@ ProofVerifierChromium::ProofVerifierChromium(CertVerifier* cert_verifier,
 
 ProofVerifierChromium::~ProofVerifierChromium() {
   verifier_.reset();
-
-  // Reset object state.
-  callback_.Reset();
-  cert_verify_result_.Reset();
 }
 
 int ProofVerifierChromium::VerifyProof(const string& hostname,
@@ -75,8 +71,6 @@ int ProofVerifierChromium::VerifyProof(const string& hostname,
   }
   cert_ = X509Certificate::CreateFromDERCertChain(cert_pieces);
   if (!cert_.get()) {
-    cert_verify_result_.Reset();
-    cert_verify_result_.cert_status = CERT_STATUS_INVALID;
     *error_details = "Failed to create certificate chain";
     DLOG(WARNING) << *error_details;
     return ERR_FAILED;
@@ -149,7 +143,7 @@ int ProofVerifierChromium::DoVerifyCertComplete(int result) {
 
   if (result <= ERR_FAILED) {
     *error_details_ = StringPrintf("Failed to verify certificate chain: %s",
-                                  ErrorToString(result));
+                                   ErrorToString(result));
     DLOG(WARNING) << *error_details_;
     result = ERR_FAILED;
   }
diff --git a/net/quic/crypto/proof_verifier_chromium.h b/net/quic/crypto/proof_verifier_chromium.h
index 2466507..f27cc41 100644
--- a/net/quic/crypto/proof_verifier_chromium.h
+++ b/net/quic/crypto/proof_verifier_chromium.h
@@ -20,18 +20,15 @@
 
 namespace net {
 
-class BoundNetLog;
 class CertVerifier;
-class CertVerifyResult;
 class SingleRequestCertVerifier;
-class X509Certificate;
 
 // ProofVerifierChromium implements the QUIC ProofVerifier interface.
 // TODO(rtenneti): Add support for multiple requests for one ProofVerifier.
 class NET_EXPORT_PRIVATE ProofVerifierChromium : public ProofVerifier {
  public:
-  explicit ProofVerifierChromium(CertVerifier* cert_verifier,
-                                 const BoundNetLog& net_log);
+  ProofVerifierChromium(CertVerifier* cert_verifier,
+                        const BoundNetLog& net_log);
   virtual ~ProofVerifierChromium();
 
   // ProofVerifier interface
@@ -74,9 +71,6 @@ class NET_EXPORT_PRIVATE ProofVerifierChromium : public ProofVerifier {
   // X509Certificate from a chain of DER encoded certificates.
   scoped_refptr<X509Certificate> cert_;
 
-  // |generation_counter| passed to VerifyProof call.
-  uint64 generation_counter_;
-
   State next_state_;
 
   BoundNetLog net_log_;
diff --git a/net/quic/quic_crypto_client_stream.cc b/net/quic/quic_crypto_client_stream.cc
index f0ec9130..7b6eb7a 100644
--- a/net/quic/quic_crypto_client_stream.cc
+++ b/net/quic/quic_crypto_client_stream.cc
@@ -71,7 +71,8 @@ void QuicCryptoClientStream::DoHandshakeLoop(
     next_state_ = STATE_IDLE;
     switch (state) {
       case STATE_SEND_CHLO: {
-        // Send the subsequent client hello in plaintext.
+        DCHECK_EQ(OK, result);
+        // Send the client hello in plaintext.
         session()->connection()->SetDefaultEncryptionLevel(ENCRYPTION_NONE);
         if (num_client_hellos_ > kMaxClientHellos) {
           CloseConnection(QUIC_CRYPTO_TOO_MANY_REJECTS);
@@ -153,7 +154,7 @@ void QuicCryptoClientStream::DoHandshakeLoop(
             cached->SetProofValid();
           } else if (!cached->signature().empty()) {
             next_state_ = STATE_VERIFY_PROOF;
-            continue;
+            break;
           }
         }
         next_state_ = STATE_SEND_CHLO;
@@ -161,7 +162,7 @@ void QuicCryptoClientStream::DoHandshakeLoop(
       case STATE_VERIFY_PROOF: {
         ProofVerifier* verifier = session()->proof_verifier();
         DCHECK(verifier);
-        next_state_ = STATE_VERIFY_PROOF_COMPLETED;
+        next_state_ = STATE_VERIFY_PROOF_COMPLETE;
         generation_counter_ = cached->generation_counter();
         result = verifier->VerifyProof(
             server_hostname_,
@@ -177,24 +178,21 @@ void QuicCryptoClientStream::DoHandshakeLoop(
         }
         break;
       }
-      case STATE_VERIFY_PROOF_COMPLETED: {
+      case STATE_VERIFY_PROOF_COMPLETE:
         if (result != OK) {
             CloseConnectionWithDetails(
                 QUIC_PROOF_INVALID, "Proof invalid: " + error_details_);
             return;
         }
-        ProofVerifier* verifier = session()->proof_verifier();
-        DCHECK(verifier);
         // Check if generation_counter has changed between STATE_VERIFY_PROOF
-        // and STATE_VERIFY_PROOF_COMPLETED state changes.
+        // and STATE_VERIFY_PROOF_COMPLETE state changes.
         if (generation_counter_ != cached->generation_counter()) {
           next_state_ = STATE_VERIFY_PROOF;
-          continue;
+        } else {
+          cached->SetProofValid();
+          next_state_ = STATE_SEND_CHLO;
         }
-        cached->SetProofValid();
-        next_state_ = STATE_SEND_CHLO;
         break;
-      }
       case STATE_RECV_SHLO: {
         // We sent a CHLO that we expected to be accepted and now we're hoping
         // for a SHLO from the server to confirm that.
@@ -265,7 +263,7 @@ void QuicCryptoClientStream::DoHandshakeLoop(
 }
 
 void QuicCryptoClientStream::OnVerifyProofComplete(int result) {
-  DCHECK_EQ(STATE_VERIFY_PROOF_COMPLETED, next_state_);
+  DCHECK_EQ(STATE_VERIFY_PROOF_COMPLETE, next_state_);
   DVLOG(1) << "VerifyProof completed: " << result;
   DoHandshakeLoop(NULL, result);
 }
diff --git a/net/quic/quic_crypto_client_stream.h b/net/quic/quic_crypto_client_stream.h
index 86633da..7234050 100644
--- a/net/quic/quic_crypto_client_stream.h
+++ b/net/quic/quic_crypto_client_stream.h
@@ -48,14 +48,19 @@ class NET_EXPORT_PRIVATE QuicCryptoClientStream : public QuicCryptoStream {
     STATE_SEND_CHLO,
     STATE_RECV_REJ,
     STATE_VERIFY_PROOF,
-    STATE_VERIFY_PROOF_COMPLETED,
+    STATE_VERIFY_PROOF_COMPLETE,
     STATE_RECV_SHLO,
   };
 
   // DoHandshakeLoop performs a step of the handshake state machine. Note that
-  // |in| is NULL for the first call.
+  // |in| is NULL for the first call. OnVerifyProofComplete passes the |result|
+  // it has received from VerifyProof call (from all other places |result| is
+  // set to OK).
   void DoHandshakeLoop(const CryptoHandshakeMessage* in, int result);
 
+  // OnVerifyProofComplete is passed as the callback method to VerifyProof.
+  // ProofVerifier calls this method with the result of proof verification when
+  // verification is performed asynchronously.
   void OnVerifyProofComplete(int result);
 
   base::WeakPtrFactory<QuicCryptoClientStream> weak_factory_;
diff --git a/net/quic/quic_session.cc b/net/quic/quic_session.cc
index b6c3f10..10cad83 100644
--- a/net/quic/quic_session.cc
+++ b/net/quic/quic_session.cc
@@ -264,7 +264,7 @@ ProofVerifier* QuicSession::proof_verifier() const {
   return proof_verifier_.get();
 }
 
-void QuicSession::SetProofVerifier(ProofVerifier* verifier) {
+void QuicSession::set_proof_verifier(ProofVerifier* verifier) {
   proof_verifier_.reset(verifier);
 }
 
diff --git a/net/quic/quic_session.h b/net/quic/quic_session.h
index bd3f604..f70ebb2 100644
--- a/net/quic/quic_session.h
+++ b/net/quic/quic_session.h
@@ -107,11 +107,11 @@ class NET_EXPORT_PRIVATE QuicSession : public QuicConnectionVisitorInterface {
 
   virtual ProofVerifier* proof_verifier() const;
 
-  // SetProofVerifier takes ownership of a |ProofVerifier| that clients are
+  // set_proof_verifier takes ownership of a |ProofVerifier| that clients are
   // free to use in order to verify certificate chains from servers. If a
   // ProofVerifier is set then the client will request a certificate chain from
   // the server.
-  virtual void SetProofVerifier(ProofVerifier* verifier);
+  virtual void set_proof_verifier(ProofVerifier* verifier);
 
   // Returns mutable config for this session. Returned config is owned
   // by QuicSession.
diff --git a/net/quic/test_tools/crypto_test_utils_chromium.cc b/net/quic/test_tools/crypto_test_utils_chromium.cc
index 5a3212ae..8faf3a8 100644
--- a/net/quic/test_tools/crypto_test_utils_chromium.cc
+++ b/net/quic/test_tools/crypto_test_utils_chromium.cc
@@ -4,7 +4,6 @@
 
 #include "net/quic/test_tools/crypto_test_utils.h"
 
-#include "base/files/file_path.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "net/base/test_data_directory.h"
@@ -39,7 +38,7 @@ class TestProofVerifierChromium : public ProofVerifierChromium {
 // static
 ProofVerifier* CryptoTestUtils::ProofVerifierForTesting() {
   TestProofVerifierChromium* proof_verifier = new TestProofVerifierChromium(
-      CertVerifier::CreateDefault(), "quic_proof_verify.crt");
+      CertVerifier::CreateDefault(), "quic_root.crt");
   return proof_verifier;
 }