OpenSSL/NSS implementation of ProofVerfifier.

OpenSSL/NSS implementation of ProofVerfifier.

Changes to make ProofVerifier asynchronous. Each QuicSession's ProofVerifier is used to verify the signature and cert chain.

Implemented generation counter in QuicCryptoClientConfig's CachedState in case certs change when we are verifying the Proof.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209946

[wtc, 2013-06-19 19:44:36]

Review comments on patch set 2:

Thanks for writing the CL.

https://codereview.chromium.org/17385010/diff/2001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/17385010/diff/2001/net/net.gyp#newcode757
net/net.gyp:757: 'quic/crypto/proof_verifier_openssl.cc',

proof_verifier_openssl.h should also be added to the file list.
Without it, the code will still compile fine, but the header
may not show up in the file list of an IDE.

https://codereview.chromium.org/17385010/diff/2001/net/quic/crypto/proof_veri...
File net/quic/crypto/proof_verifier_openssl.cc (right):

https://codereview.chromium.org/17385010/diff/2001/net/quic/crypto/proof_veri...
net/quic/crypto/proof_verifier_openssl.cc:38: store_ = X509_STORE_new();

We need to initialize OpenSSL. I suggest we add a crypto::EnsureOpenSSLInit()
call here in the ProofVerifierOpenSSL constructor.

https://codereview.chromium.org/17385010/diff/2001/net/quic/test_tools/crypto...
File net/quic/test_tools/crypto_test_utils.cc (right):

https://codereview.chromium.org/17385010/diff/2001/net/quic/test_tools/crypto...
net/quic/test_tools/crypto_test_utils.cc:7: #include "base/files/file_path.h"

This change is not necessary.

[ramant (doing other things), 2013-06-19 19:58:20]

Many many thanks Wan-Teh. It is really awesome/great to see these tests working.

https://chromiumcodereview.appspot.com/17385010/diff/2001/net/net.gyp
File net/net.gyp (right):

https://chromiumcodereview.appspot.com/17385010/diff/2001/net/net.gyp#newcode757
net/net.gyp:757: 'quic/crypto/proof_verifier_openssl.cc',
On 2013/06/19 19:44:36, wtc wrote:
> 
> proof_verifier_openssl.h should also be added to the file list.
> Without it, the code will still compile fine, but the header
> may not show up in the file list of an IDE.

Done.

https://chromiumcodereview.appspot.com/17385010/diff/2001/net/quic/crypto/pro...
File net/quic/crypto/proof_verifier_openssl.cc (right):

https://chromiumcodereview.appspot.com/17385010/diff/2001/net/quic/crypto/pro...
net/quic/crypto/proof_verifier_openssl.cc:38: store_ = X509_STORE_new();
On 2013/06/19 19:44:36, wtc wrote:
> 
> We need to initialize OpenSSL. I suggest we add a crypto::EnsureOpenSSLInit()
> call here in the ProofVerifierOpenSSL constructor.

Done.

https://chromiumcodereview.appspot.com/17385010/diff/2001/net/quic/test_tools...
File net/quic/test_tools/crypto_test_utils.cc (right):

https://chromiumcodereview.appspot.com/17385010/diff/2001/net/quic/test_tools...
net/quic/test_tools/crypto_test_utils.cc:7: #include "base/files/file_path.h"
On 2013/06/19 19:44:36, wtc wrote:
> 
> This change is not necessary.

Done.

[wtc, 2013-06-19 23:12:17]

Patch set 3 LGTM, with the understanding that this is just an
experiment, not for checkin.

[wtc, 2013-06-24 22:36:56]

Review comments on patch set 6:

Please update the CL's description to describe the new code.

https://codereview.chromium.org/17385010/diff/53001/net/data/ssl/certificates...
File net/data/ssl/certificates/proof_verify.crt (right):

https://codereview.chromium.org/17385010/diff/53001/net/data/ssl/certificates...
net/data/ssl/certificates/proof_verify.crt:1: Certificate:

Please add a comment to net/data/ssl/certificates/README to
describe what this certificate file is for.

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_tes...
File net/quic/crypto/proof_test.cc (right):

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_tes...
net/quic/crypto/proof_test.cc:282: }

Please use the following code for this new unit test. You need to
add the certificate files test_ecc.example.com.crt and
intermediate.crt to net/data/ssl/certificates.

static string PEMCertFileToDER(const string& file_name) {
  base::FilePath certs_dir = GetTestCertsDirectory();
  scoped_refptr<X509Certificate> cert =
      ImportCertFromFile(certs_dir, file_name);
  CHECK_NE(static_cast<X509Certificate*>(NULL), cert);

  string der_bytes;
  CHECK(X509Certificate::GetDEREncoded(cert->os_cert_handle(), &der_bytes));
  return der_bytes;
}

// A known answer test that allows us to test ProofVerifier without a working
// ProofSource.
TEST(Proof, VerifyECDSAKnownAnswerTest) {
  // These sample signatures were generated by running the Proof.Verify test
  // (modified to use ECDSA for signing proofs) and dumping the bytes of the
  // |signature| output of ProofSource::GetProof().
  static const unsigned char signature_data_0[] = {
    0x30, 0x45, 0x02, 0x20, 0x15, 0xb7, 0x9f, 0xe3, 0xd9, 0x7a,
    0x3c, 0x3b, 0x18, 0xb0, 0xdb, 0x60, 0x23, 0x56, 0xa0, 0x06,
    0x4e, 0x70, 0xa3, 0xf7, 0x4b, 0xe5, 0x0d, 0x69, 0xf0, 0x35,
    0x8c, 0xae, 0xb5, 0x54, 0x32, 0xe9, 0x02, 0x21, 0x00, 0xf7,
    0xe3, 0x06, 0x99, 0x16, 0x56, 0x7e, 0xab, 0x33, 0x53, 0x0d,
    0xde, 0xbe, 0xef, 0x6d, 0xb0, 0xc7, 0xa6, 0x63, 0xaf, 0x8d,
    0xab, 0x34, 0xa9, 0xc0, 0x63, 0x88, 0x47, 0x17, 0x4c, 0x4c,
    0x04,
  };
  static const unsigned char signature_data_1[] = {
    0x30, 0x44, 0x02, 0x20, 0x69, 0x60, 0x55, 0xbb, 0x11, 0x93,
    0x6a, 0xdc, 0x9b, 0x61, 0x2c, 0x60, 0x19, 0xbc, 0x15, 0x55,
    0xcf, 0xf2, 0x8e, 0x2e, 0x27, 0x0b, 0x69, 0xef, 0x33, 0x25,
    0x1e, 0x5d, 0x8c, 0x00, 0x11, 0xef, 0x02, 0x20, 0x0c, 0x26,
    0xfe, 0x0b, 0x06, 0x8f, 0xe8, 0xe2, 0x02, 0x63, 0xe5, 0x43,
    0x0d, 0xc9, 0x80, 0x4d, 0xe9, 0x6f, 0x6e, 0x18, 0xdb, 0xb0,
    0x04, 0x2a, 0x45, 0x37, 0x1a, 0x60, 0x0e, 0xc6, 0xc4, 0x8f,
  };
  static const unsigned char signature_data_2[] = {
    0x30, 0x45, 0x02, 0x21, 0x00, 0xd5, 0x43, 0x36, 0x60, 0x50,
    0xce, 0xe0, 0x00, 0x51, 0x02, 0x84, 0x95, 0x51, 0x47, 0xaf,
    0xe4, 0xf9, 0xe1, 0x23, 0xae, 0x21, 0xb4, 0x98, 0xd1, 0xa3,
    0x5f, 0x3b, 0xf3, 0x6a, 0x65, 0x44, 0x6b, 0x02, 0x20, 0x30,
    0x7e, 0xb4, 0xea, 0xf0, 0xda, 0xdb, 0xbd, 0x38, 0xb9, 0x7a,
    0x5d, 0x12, 0x04, 0x0e, 0xc2, 0xf0, 0xb1, 0x0e, 0x25, 0xf8,
    0x0a, 0x27, 0xa3, 0x16, 0x94, 0xac, 0x1e, 0xb8, 0x6e, 0x00,
    0x05,
  };

  TestCompletionCallback callback;
  scoped_ptr<ProofVerifier> verifier(
      CryptoTestUtils::ProofVerifierForTesting());

  const string server_config = "server config bytes";
  const string hostname = "test.example.com";

  vector<string> certs(2);
  certs[0] = PEMCertFileToDER("test_ecc.example.com.crt");
  certs[1] = PEMCertFileToDER("intermediate.crt");

  // Signatures are nondeterministic, so we test multiple signatures on the
  ...
  <<<Use your code here.>>>
}

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
File net/quic/crypto/proof_verifier.h (right):

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier.h:26: // the problem and returns ERR_FAILED.

"returns ERR_FAILED" should replace the "returns false" on the previous line.

We also need to add comments to document the |callback| argument and the
async behavior (the function may return ERR_IO_PENDING, in which case the
|callback| will be run with the final OK/ERR_FAILED result when the proof is
verified).

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier.h:37: const CompletionCallback& callback,

The |callback| argument seems to be the last argument by convention.
Could you check some other methods in net that take a |callback| argument?
Thanks.

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
File net/quic/crypto/proof_verifier_chromium.cc (right):

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.cc:53: // We call VerifySignature first
to avoid copying of server_config data.

server_config => server_config and signature.

Remove "data".

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.cc:66: // TODO(rtenneti): do we need to
save the original |error|?

I suggest saving the original |error| in string form in |*error_details|.
Something like:

    *error_details = "Failed to verify certificate chain: error " +
                     ErrorToString(error);
    error = ERR_FAILED;

But, this whole thing (lines 64-68) should be moved to DoVerifyCertComplete().

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.cc:79: // Convert certs to X509.

X509 => X509Certificate

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.cc:88: return ERR_FAILED;

ERR_FAILED => ERR_CERT_INVALID

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.cc:123:
base::ResetAndReturn(&callback_).Run(rv >= OK ? OK : ERR_FAILED);

We should pass just |rv| to Run(). The error code mapping should be done
in DoVerifyCertComplete().

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.cc:130: // TODO(rtenneti): Are the
following flags correct??

Let's start with the following:
  int flags = CertVerifier::VERIFY_REV_CHECKING_ENABLED |
              CertVerifier::VERIFY_CERT_IO_ENABLED;

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
File net/quic/crypto/proof_verifier_chromium.h (right):

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.h:30: typedef struct CERTCertificateStr
CERTCertificate;

Delete these two lines. They are NSS-specific.

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.h:46: // TODO(rtenneti): Do we need
completed_cert_verification??

The completed_cert_verification() method doesn't seem necessary.

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.h:61: void OnVerifyCertIOComplete(int
result);

These two functions can be named simply DoLoop(int result)
and OnIOComplete(int result).

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.h:69: 

Delete one blank line.

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.h:83: CertVerifyResult
server_cert_verify_result_;

Remove "server_" from the member's name.

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.h:86: scoped_refptr<X509Certificate>
server_cert_;

It may be sufficient to just name this member |cert_|.

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.h:88: State next_cert_state_;

This member can be named simply next_state_.

https://codereview.chromium.org/17385010/diff/53001/net/quic/quic_crypto_clie...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/17385010/diff/53001/net/quic/quic_crypto_clie...
net/quic/quic_crypto_client_stream.cc:150: // asynchronous.

Delete this TODO comment because it's done.

https://codereview.chromium.org/17385010/diff/53001/net/quic/quic_crypto_clie...
net/quic/quic_crypto_client_stream.cc:158: &error_details);

Ideally we should set next_state_ to a state that handles
the completion of verifier->VerifyProof(). That is the
convention of the state machines in net.

https://codereview.chromium.org/17385010/diff/53001/net/quic/quic_crypto_clie...
net/quic/quic_crypto_client_stream.cc:162: } else {

Nit: omit the "else" after a break statement.

https://codereview.chromium.org/17385010/diff/53001/net/quic/quic_crypto_clie...
net/quic/quic_crypto_client_stream.cc:175: next_state_ = STATE_SEND_CHLO;

It is possible to use additional states to avoid duplicating this code
(lines 172-175) in two states (this state and the
STATE_PROOF_VERIFICATION_COMPLETED
below). The tricky part is how to deal with the cached->SetProofValid() call.

https://codereview.chromium.org/17385010/diff/53001/net/quic/quic_crypto_clie...
net/quic/quic_crypto_client_stream.cc:254:
CloseConnectionWithDetails(QUIC_PROOF_INVALID, "Proof invalid:");

The error_details string is incomplete: "Proof invalid:". It is missing
the actual error details.

I think this code (lines 253-256) should be moved to the code that handles
the STATE_PROOF_VERIFICATION_COMPLETED. This requires passing |result| to
DoHandshakeLoop(). Is it easy to do that?

https://codereview.chromium.org/17385010/diff/53001/net/quic/test_tools/crypt...
File net/quic/test_tools/crypto_test_utils_chromium.cc (right):

https://codereview.chromium.org/17385010/diff/53001/net/quic/test_tools/crypt...
net/quic/test_tools/crypto_test_utils_chromium.cc:31: void
InstallRootCertificate() {

This method should just be inlined in the constructor. This doesn't need
to be a public method.

https://codereview.chromium.org/17385010/diff/53001/net/quic/test_tools/crypt...
net/quic/test_tools/crypto_test_utils_chromium.cc:34: "proof_verify.crt");

The root certificate file name should ideally be passed in as an
argument.

https://codereview.chromium.org/17385010/diff/53001/net/quic/test_tools/crypt...
net/quic/test_tools/crypto_test_utils_chromium.cc:40:
scoped_refptr<X509Certificate> root_cert_;

Remove the root_cert_ member. The scoped_root_ member will acquire a
reference to the root cert. See the |cert_| member of the ScopedTestRoot
class in net/cert/test_root_certs.h.

[ramant (doing other things), 2013-06-28 19:16:56]

Hi Wan-Teh,
  Made all the changes you have suggested. Could you please take another look?

thanks
raman

https://codereview.chromium.org/17385010/diff/53001/net/data/ssl/certificates...
File net/data/ssl/certificates/proof_verify.crt (right):

https://codereview.chromium.org/17385010/diff/53001/net/data/ssl/certificates...
net/data/ssl/certificates/proof_verify.crt:1: Certificate:
On 2013/06/24 22:36:56, wtc wrote:
> 
> Please add a comment to net/data/ssl/certificates/README to
> describe what this certificate file is for.

Done.

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_tes...
File net/quic/crypto/proof_test.cc (right):

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_tes...
net/quic/crypto/proof_test.cc:282: }
On 2013/06/24 22:36:56, wtc wrote:
> 
> Please use the following code for this new unit test. You need to
> add the certificate files test_ecc.example.com.crt and
> intermediate.crt to net/data/ssl/certificates.
> 
> static string PEMCertFileToDER(const string& file_name) {
>   base::FilePath certs_dir = GetTestCertsDirectory();
>   scoped_refptr<X509Certificate> cert =
>       ImportCertFromFile(certs_dir, file_name);
>   CHECK_NE(static_cast<X509Certificate*>(NULL), cert);
> 
>   string der_bytes;
>   CHECK(X509Certificate::GetDEREncoded(cert->os_cert_handle(), &der_bytes));
>   return der_bytes;
> }
> 
> // A known answer test that allows us to test ProofVerifier without a working
> // ProofSource.
> TEST(Proof, VerifyECDSAKnownAnswerTest) {
>   // These sample signatures were generated by running the Proof.Verify test
>   // (modified to use ECDSA for signing proofs) and dumping the bytes of the
>   // |signature| output of ProofSource::GetProof().
>   static const unsigned char signature_data_0[] = {
>     0x30, 0x45, 0x02, 0x20, 0x15, 0xb7, 0x9f, 0xe3, 0xd9, 0x7a,
>     0x3c, 0x3b, 0x18, 0xb0, 0xdb, 0x60, 0x23, 0x56, 0xa0, 0x06,
>     0x4e, 0x70, 0xa3, 0xf7, 0x4b, 0xe5, 0x0d, 0x69, 0xf0, 0x35,
>     0x8c, 0xae, 0xb5, 0x54, 0x32, 0xe9, 0x02, 0x21, 0x00, 0xf7,
>     0xe3, 0x06, 0x99, 0x16, 0x56, 0x7e, 0xab, 0x33, 0x53, 0x0d,
>     0xde, 0xbe, 0xef, 0x6d, 0xb0, 0xc7, 0xa6, 0x63, 0xaf, 0x8d,
>     0xab, 0x34, 0xa9, 0xc0, 0x63, 0x88, 0x47, 0x17, 0x4c, 0x4c,
>     0x04,
>   };
>   static const unsigned char signature_data_1[] = {
>     0x30, 0x44, 0x02, 0x20, 0x69, 0x60, 0x55, 0xbb, 0x11, 0x93,
>     0x6a, 0xdc, 0x9b, 0x61, 0x2c, 0x60, 0x19, 0xbc, 0x15, 0x55,
>     0xcf, 0xf2, 0x8e, 0x2e, 0x27, 0x0b, 0x69, 0xef, 0x33, 0x25,
>     0x1e, 0x5d, 0x8c, 0x00, 0x11, 0xef, 0x02, 0x20, 0x0c, 0x26,
>     0xfe, 0x0b, 0x06, 0x8f, 0xe8, 0xe2, 0x02, 0x63, 0xe5, 0x43,
>     0x0d, 0xc9, 0x80, 0x4d, 0xe9, 0x6f, 0x6e, 0x18, 0xdb, 0xb0,
>     0x04, 0x2a, 0x45, 0x37, 0x1a, 0x60, 0x0e, 0xc6, 0xc4, 0x8f,
>   };
>   static const unsigned char signature_data_2[] = {
>     0x30, 0x45, 0x02, 0x21, 0x00, 0xd5, 0x43, 0x36, 0x60, 0x50,
>     0xce, 0xe0, 0x00, 0x51, 0x02, 0x84, 0x95, 0x51, 0x47, 0xaf,
>     0xe4, 0xf9, 0xe1, 0x23, 0xae, 0x21, 0xb4, 0x98, 0xd1, 0xa3,
>     0x5f, 0x3b, 0xf3, 0x6a, 0x65, 0x44, 0x6b, 0x02, 0x20, 0x30,
>     0x7e, 0xb4, 0xea, 0xf0, 0xda, 0xdb, 0xbd, 0x38, 0xb9, 0x7a,
>     0x5d, 0x12, 0x04, 0x0e, 0xc2, 0xf0, 0xb1, 0x0e, 0x25, 0xf8,
>     0x0a, 0x27, 0xa3, 0x16, 0x94, 0xac, 0x1e, 0xb8, 0x6e, 0x00,
>     0x05,
>   };
> 
>   TestCompletionCallback callback;
>   scoped_ptr<ProofVerifier> verifier(
>       CryptoTestUtils::ProofVerifierForTesting());
> 
>   const string server_config = "server config bytes";
>   const string hostname = "test.example.com";
> 
>   vector<string> certs(2);
>   certs[0] = PEMCertFileToDER("test_ecc.example.com.crt");
>   certs[1] = PEMCertFileToDER("intermediate.crt");
> 
>   // Signatures are nondeterministic, so we test multiple signatures on the
>   ...
>   <<<Use your code here.>>>
> }

I have used the latest code from you for the above test. Hope that is ok.

Done.

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
File net/quic/crypto/proof_verifier.h (right):

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier.h:26: // the problem and returns ERR_FAILED.
On 2013/06/24 22:36:56, wtc wrote:
> 
> "returns ERR_FAILED" should replace the "returns false" on the previous line.
> 
> We also need to add comments to document the |callback| argument and the
> async behavior (the function may return ERR_IO_PENDING, in which case the
> |callback| will be run with the final OK/ERR_FAILED result when the proof is
> verified).

Done.

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier.h:37: const CompletionCallback& callback,
On 2013/06/24 22:36:56, wtc wrote:
> 
> The |callback| argument seems to be the last argument by convention.
> Could you check some other methods in net that take a |callback| argument?
> Thanks.

./socket/client_socket_pool_manager.h:106, socet_test_util.h and others don't
seem to follow that convention (they have callback argument in the middle).
Should I change it? thanks.

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
File net/quic/crypto/proof_verifier_chromium.cc (right):

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.cc:53: // We call VerifySignature first
to avoid copying of server_config data.
On 2013/06/24 22:36:56, wtc wrote:
> 
> server_config => server_config and signature.
> 
> Remove "data".

Done.

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.cc:66: // TODO(rtenneti): do we need to
save the original |error|?
On 2013/06/24 22:36:56, wtc wrote:
> 
> I suggest saving the original |error| in string form in |*error_details|.
> Something like:
> 
>     *error_details = "Failed to verify certificate chain: error " +
>                      ErrorToString(error);
>     error = ERR_FAILED;
> 
> But, this whole thing (lines 64-68) should be moved to DoVerifyCertComplete().

Done.

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.cc:79: // Convert certs to X509.
On 2013/06/24 22:36:56, wtc wrote:
> 
> X509 => X509Certificate

Done.

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.cc:88: return ERR_FAILED;
On 2013/06/24 22:36:56, wtc wrote:
> 
> ERR_FAILED => ERR_CERT_INVALID

Done.

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.cc:123:
base::ResetAndReturn(&callback_).Run(rv >= OK ? OK : ERR_FAILED);
On 2013/06/24 22:36:56, wtc wrote:
> 
> We should pass just |rv| to Run(). The error code mapping should be done
> in DoVerifyCertComplete().

Done.

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.cc:130: // TODO(rtenneti): Are the
following flags correct??
On 2013/06/24 22:36:56, wtc wrote:
> 
> Let's start with the following:
>   int flags = CertVerifier::VERIFY_REV_CHECKING_ENABLED |
>               CertVerifier::VERIFY_CERT_IO_ENABLED;

Done.

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
File net/quic/crypto/proof_verifier_chromium.h (right):

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.h:30: typedef struct CERTCertificateStr
CERTCertificate;
On 2013/06/24 22:36:56, wtc wrote:
> 
> Delete these two lines. They are NSS-specific.

Done.

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.h:46: // TODO(rtenneti): Do we need
completed_cert_verification??
On 2013/06/24 22:36:56, wtc wrote:
> 
> The completed_cert_verification() method doesn't seem necessary.

Done.

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.h:61: void OnVerifyCertIOComplete(int
result);
On 2013/06/24 22:36:56, wtc wrote:
> 
> These two functions can be named simply DoLoop(int result)
> and OnIOComplete(int result).

Done.

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.h:69: 
On 2013/06/24 22:36:56, wtc wrote:
> 
> Delete one blank line.

Done.

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.h:83: CertVerifyResult
server_cert_verify_result_;
On 2013/06/24 22:36:56, wtc wrote:
> 
> Remove "server_" from the member's name.

Done.

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.h:86: scoped_refptr<X509Certificate>
server_cert_;
On 2013/06/24 22:36:56, wtc wrote:
> 
> It may be sufficient to just name this member |cert_|.

Done.

https://codereview.chromium.org/17385010/diff/53001/net/quic/crypto/proof_ver...
net/quic/crypto/proof_verifier_chromium.h:88: State next_cert_state_;
On 2013/06/24 22:36:56, wtc wrote:
> 
> This member can be named simply next_state_.

Done.

https://codereview.chromium.org/17385010/diff/53001/net/quic/quic_crypto_clie...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/17385010/diff/53001/net/quic/quic_crypto_clie...
net/quic/quic_crypto_client_stream.cc:150: // asynchronous.
On 2013/06/24 22:36:56, wtc wrote:
> 
> Delete this TODO comment because it's done.

Done.

https://codereview.chromium.org/17385010/diff/53001/net/quic/quic_crypto_clie...
net/quic/quic_crypto_client_stream.cc:158: &error_details);
On 2013/06/24 22:36:56, wtc wrote:
> 
> Ideally we should set next_state_ to a state that handles
> the completion of verifier->VerifyProof(). That is the
> convention of the state machines in net.

Done.

https://codereview.chromium.org/17385010/diff/53001/net/quic/quic_crypto_clie...
net/quic/quic_crypto_client_stream.cc:162: } else {
On 2013/06/24 22:36:56, wtc wrote:
> 
> Nit: omit the "else" after a break statement.

Done.

https://codereview.chromium.org/17385010/diff/53001/net/quic/quic_crypto_clie...
net/quic/quic_crypto_client_stream.cc:175: next_state_ = STATE_SEND_CHLO;
On 2013/06/24 22:36:56, wtc wrote:
> 
> It is possible to use additional states to avoid duplicating this code
> (lines 172-175) in two states (this state and the
> STATE_PROOF_VERIFICATION_COMPLETED
> below). The tricky part is how to deal with the cached->SetProofValid() call.

Done.

https://codereview.chromium.org/17385010/diff/53001/net/quic/quic_crypto_clie...
net/quic/quic_crypto_client_stream.cc:254:
CloseConnectionWithDetails(QUIC_PROOF_INVALID, "Proof invalid:");
On 2013/06/24 22:36:56, wtc wrote:
> 
> The error_details string is incomplete: "Proof invalid:". It is missing
> the actual error details.
> 
> I think this code (lines 253-256) should be moved to the code that handles
> the STATE_PROOF_VERIFICATION_COMPLETED. This requires passing |result| to
> DoHandshakeLoop(). Is it easy to do that?

Done.

https://codereview.chromium.org/17385010/diff/53001/net/quic/test_tools/crypt...
File net/quic/test_tools/crypto_test_utils_chromium.cc (right):

https://codereview.chromium.org/17385010/diff/53001/net/quic/test_tools/crypt...
net/quic/test_tools/crypto_test_utils_chromium.cc:31: void
InstallRootCertificate() {
On 2013/06/24 22:36:56, wtc wrote:
> 
> This method should just be inlined in the constructor. This doesn't need
> to be a public method.

Done.

https://codereview.chromium.org/17385010/diff/53001/net/quic/test_tools/crypt...
net/quic/test_tools/crypto_test_utils_chromium.cc:34: "proof_verify.crt");
On 2013/06/24 22:36:56, wtc wrote:
> 
> The root certificate file name should ideally be passed in as an
> argument.

Done.

https://codereview.chromium.org/17385010/diff/53001/net/quic/test_tools/crypt...
net/quic/test_tools/crypto_test_utils_chromium.cc:40:
scoped_refptr<X509Certificate> root_cert_;
On 2013/06/24 22:36:56, wtc wrote:
> 
> Remove the root_cert_ member. The scoped_root_ member will acquire a
> reference to the root cert. See the |cert_| member of the ScopedTestRoot
> class in net/cert/test_root_certs.h.

Done.

[ramant (doing other things), 2013-06-28 19:20:49]

agl@: This CL has Async Proof Verification code. Will upload another CL that has
QUIC over https changes.

[agl2, 2013-06-28 20:30:09]

https://codereview.chromium.org/17385010/diff/79024/net/quic/quic_crypto_clie...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/17385010/diff/79024/net/quic/quic_crypto_clie...
net/quic/quic_crypto_client_stream.cc:153:
base::Bind(&QuicCryptoClientStream::VerifyProofCompleted,
likewise, base::Bind can't be used in this code either, sadly.

https://codereview.chromium.org/17385010/diff/79024/net/quic/quic_crypto_clie...
net/quic/quic_crypto_client_stream.cc:171: cached->SetProofValid();
This is insecure:

In the time that a proof was being verified, another connection to the same
origin may have started and receive different certificates. Thus the proof that
this line is marking as valid may not be the proof that was passed to
VerifyProof.

verifier needs to implement a cache (I think there already is one under the
covers so that should be a no-op). I think there needs to be a
STATE_PROOF_VERIFY containing just the VerifyProof call. If VerifyProof needs to
block then the handshake state machine should return to STATE_PROOF_VERIFY and
call VerifyProof again. If the certs have changed, then it'll verify the new
ones. If they are the same then it'll hit the cache and return immediately.

https://codereview.chromium.org/17385010/diff/79024/net/quic/quic_crypto_clie...
File net/quic/quic_crypto_client_stream.h (right):

https://codereview.chromium.org/17385010/diff/79024/net/quic/quic_crypto_clie...
net/quic/quic_crypto_client_stream.h:61:
base::WeakPtrFactory<QuicCryptoClientStream> weak_factory_;
This code needs to work in google3 as well. I think concepts like WeakPtrFactory
don't exist there.

[ramant (doing other things), 2013-06-29 04:08:43]

Thanks agl2 for the comments. Fixed the state machine. PTAL. thanks very much.

https://codereview.chromium.org/17385010/diff/79024/net/quic/quic_crypto_clie...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/17385010/diff/79024/net/quic/quic_crypto_clie...
net/quic/quic_crypto_client_stream.cc:153:
base::Bind(&QuicCryptoClientStream::VerifyProofCompleted,
On 2013/06/28 20:30:09, agl2 wrote:
> likewise, base::Bind can't be used in this code either, sadly.

Will use internal API call here.

https://codereview.chromium.org/17385010/diff/79024/net/quic/quic_crypto_clie...
net/quic/quic_crypto_client_stream.cc:171: cached->SetProofValid();
On 2013/06/28 20:30:09, agl2 wrote:
> This is insecure:
> 
> In the time that a proof was being verified, another connection to the same
> origin may have started and receive different certificates. Thus the proof
that
> this line is marking as valid may not be the proof that was passed to
> VerifyProof.
> 
> verifier needs to implement a cache (I think there already is one under the
> covers so that should be a no-op). I think there needs to be a
> STATE_PROOF_VERIFY containing just the VerifyProof call. If VerifyProof needs
to
> block then the handshake state machine should return to STATE_PROOF_VERIFY and
> call VerifyProof again. If the certs have changed, then it'll verify the new
> ones. If they are the same then it'll hit the cache and return immediately.

Done.

https://codereview.chromium.org/17385010/diff/79024/net/quic/quic_crypto_clie...
File net/quic/quic_crypto_client_stream.h (right):

https://codereview.chromium.org/17385010/diff/79024/net/quic/quic_crypto_clie...
net/quic/quic_crypto_client_stream.h:61:
base::WeakPtrFactory<QuicCryptoClientStream> weak_factory_;
On 2013/06/28 20:30:09, agl2 wrote:
> This code needs to work in google3 as well. I think concepts like
WeakPtrFactory
> don't exist there.

Will not port this CL back. Will use google3 API calls if/when we port this code
back.

[agl, 2013-07-01 16:23:18]

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_te...
File net/quic/crypto/proof_test.cc (right):

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_te...
net/quic/crypto/proof_test.cc:104: // sLen = hLen
The #if 0 block should probably either be removed, or the comment should be
expanded to explain why it's being kept around.

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
File net/quic/crypto/proof_verifier.h (right):

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier.h:41: virtual std::string error_details() = 0;
This can't work, right? It's a method of the ProofVerifier, but it's a function
of the specific VerifyProof call. How would it find the right error message?

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
File net/quic/crypto/proof_verifier_chromium.h (right):

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.h:71: CertVerifyResult
cert_verify_result_;
These members suggest that you're going to have to create a new ProofVerifier
for every connection, is that the plan? I think that needs more changes than you
have made because the ProofVerifier is still attached to the
QuicCryptoClientConfig and many connections share the same
QuicCryptoClientConfig.

Rather, I think you need one ProofVerifier per process (i.e. it's a global), and
this state needs to be wrapped up in an internal object that's created for each
call to VerifyProof.

https://codereview.chromium.org/17385010/diff/102001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/17385010/diff/102001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:167: next_state_ =
STATE_PROOF_VERIFICATION_COMPLETED;
This needs to be STATE_PROOF_VERIFY I think, and that would mean that result
needs to be checked on entry to STATE_PROOF_VERIFY.

(Because the cached->SetProofValid must correspond to the cached->certs() given
to VerifyProof, and cached->certs() may have changed between STATE_PROOF_VERIFY
and STATE_PROOF_VERIFICATION_COMPLETED.)

That also means that VerifyProof must not return ERR_IO_PENDING if asked the
same question twice. If that's not possible then the cache could be implemented
here by adding a generation counter to CachedState and remembering which
generation was verified.

[wtc, 2013-07-02 00:56:38]

Review comments on patch set 9:

ramant: I reviewed everything except quic_crypto_client_stream.cc.
I will review quic_crypto_client_stream.cc tomorrow.

https://codereview.chromium.org/17385010/diff/102001/net/data/ssl/certificate...
File net/data/ssl/certificates/README (right):

https://codereview.chromium.org/17385010/diff/102001/net/data/ssl/certificate...
net/data/ssl/certificates/README:216: - intermediate.crt

Since most of these certificate files have rather generic names, I now think
we should just add them to the net/quic/crypto/testdata directory, similar to
the internal source tree.

Is it easy to do this?

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_te...
File net/quic/crypto/proof_test.cc (right):

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_te...
net/quic/crypto/proof_test.cc:104: // sLen = hLen

ramant: please feel free to just delete the #if 0 block. We can come back
to this code review to recover these sample signatures when we change the
sLen parameter for RSA-PSS signatures.

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_te...
net/quic/crypto/proof_test.cc:226: rv = callback.WaitForResult();

Nit: this can be rewritten more compactly as
  rv = callback.GetResult(rv);

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_te...
net/quic/crypto/proof_test.cc:228: ASSERT_EQ("", verifier->error_details());

Nit: add blank lines between these tests. Specifically, add a blank line
after each ASSERT_EQ("", verifier->error_details()).

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
File net/quic/crypto/proof_verifier.h (right):

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier.h:41: virtual std::string error_details() = 0;

|error_details| should still be an output argument of VerifyProof().
VerifyProof() can return an output (in addition to the 'int' result)
asynchronously. The caller needs to make sure the memory pointed to
by |error_details| remains valid until VerifyProof() completes or is
aborted.

This reminds me that the ProofVerifier destructor needs to abort a
pending VerifyProof() operation.

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
File net/quic/crypto/proof_verifier_chromium.cc (right):

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.cc:35: cert_(NULL),

This initializer is not necessary because |cert_| is a scoped_refptr,
whose default constructor is equivalent to this initializer.

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.cc:60: vector<StringPiece>
v(certs.size());

Nit: "v" seems too short. How about |pieces| or |cert_pieces|?

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.cc:68: return ERR_CERT_INVALID;

We need to return ERR_FAILED because ProofVerifier::VerifyProof
is documented to return OK or ERR_FAILED.

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.cc:91: return DoLoop(OK);

I suggest removing the VerifyChain() function and just inlining these two lines
at the only call site.

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.cc:151: // Exit DoLoop and return the
result to the caller to ProofVerify.

Typo: ProofVerify => VerifyProof

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.cc:230: DLOG(WARNING) << "VerifyFinal
success";

This should be DLOG(INFO), or should be removed.

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
File net/quic/crypto/proof_verifier_chromium.h (right):

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.h:79: BoundNetLog net_log_;

The net_log_ member probably should be initialized by an argument to the
constructor.

Take SSLClientSocketNSS for an example. Its net_log_ member is initialized by
the
NetLog of the transport_socket input to the constructor.

https://codereview.chromium.org/17385010/diff/102001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.h (right):

https://codereview.chromium.org/17385010/diff/102001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.h:51: STATE_PROOF_VERIFICATION_COMPLETED,

Nit: these two states should be named STATE_VERIFY_PROOF and
STATE_VERIFY_PROOF_COMPLETE by our convention.

https://codereview.chromium.org/17385010/diff/102001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.h:59: void VerifyProofCompleted(int result);

Nit: this method probably should be named OnVerifyProofComplete
by our convention.

https://codereview.chromium.org/17385010/diff/102001/net/quic/test_tools/cryp...
File net/quic/test_tools/crypto_test_utils_chromium.cc (right):

https://codereview.chromium.org/17385010/diff/102001/net/quic/test_tools/cryp...
net/quic/test_tools/crypto_test_utils_chromium.cc:8: #include "base/logging.h"

"base/logging.h" doesn't seem necessary...

https://codereview.chromium.org/17385010/diff/102001/net/quic/test_tools/cryp...
net/quic/test_tools/crypto_test_utils_chromium.cc:17: #include
"testing/gtest/include/gtest/gtest.h"

"testing/gtest/include/gtest/gtest.h" doesn't seem necessary.

https://codereview.chromium.org/17385010/diff/102001/net/quic/test_tools/cryp...
net/quic/test_tools/crypto_test_utils_chromium.cc:26: const std::string&
cert_file)

This constructor does not need to be marked "explicit" because it has
two arguments.

[ramant (doing other things), 2013-07-02 14:19:50]

Hi Wan-Teh and Adam,
  Made all the changes you/rch have suggested. IMO, this is not the final
version. Would appreciate if you could take another look at this CL.

thanks very much,
raman

https://codereview.chromium.org/17385010/diff/102001/net/data/ssl/certificate...
File net/data/ssl/certificates/README (right):

https://codereview.chromium.org/17385010/diff/102001/net/data/ssl/certificate...
net/data/ssl/certificates/README:216: - intermediate.crt
On 2013/07/02 00:56:38, wtc wrote:
> 
> Since most of these certificate files have rather generic names, I now think
> we should just add them to the net/quic/crypto/testdata directory, similar to
> the internal source tree.
> 
> Is it easy to do this?

Done.

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_te...
File net/quic/crypto/proof_test.cc (right):

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_te...
net/quic/crypto/proof_test.cc:104: // sLen = hLen
On 2013/07/01 16:23:18, agl wrote:
> The #if 0 block should probably either be removed, or the comment should be
> expanded to explain why it's being kept around.

Added a comment.

Done.

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_te...
net/quic/crypto/proof_test.cc:226: rv = callback.WaitForResult();
On 2013/07/02 00:56:38, wtc wrote:
> 
> Nit: this can be rewritten more compactly as
>   rv = callback.GetResult(rv);

Done.

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_te...
net/quic/crypto/proof_test.cc:228: ASSERT_EQ("", verifier->error_details());
On 2013/07/02 00:56:38, wtc wrote:
> 
> Nit: add blank lines between these tests. Specifically, add a blank line
> after each ASSERT_EQ("", verifier->error_details()).

Done.

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
File net/quic/crypto/proof_verifier.h (right):

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier.h:41: virtual std::string error_details() = 0;
On 2013/07/02 00:56:38, wtc wrote:
> 
> |error_details| should still be an output argument of VerifyProof().
> VerifyProof() can return an output (in addition to the 'int' result)
> asynchronously. The caller needs to make sure the memory pointed to
> by |error_details| remains valid until VerifyProof() completes or is
> aborted.
> 
> This reminds me that the ProofVerifier destructor needs to abort a
> pending VerifyProof() operation.

For |error_details| to be valid until ProofVerifier, stored errors in
|error_details| on ProofVerifier for both synchronous and asynchronous calls.
The other choice is to store the error details in |session|. Hope this is ok.

We call ~SingleRequestCertVerifier and it does a CancelRequest.
Done.

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
File net/quic/crypto/proof_verifier_chromium.cc (right):

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.cc:35: cert_(NULL),
On 2013/07/02 00:56:38, wtc wrote:
> 
> This initializer is not necessary because |cert_| is a scoped_refptr,
> whose default constructor is equivalent to this initializer.

Done.

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.cc:60: vector<StringPiece>
v(certs.size());
On 2013/07/02 00:56:38, wtc wrote:
> 
> Nit: "v" seems too short. How about |pieces| or |cert_pieces|?

Done.

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.cc:68: return ERR_CERT_INVALID;
On 2013/07/02 00:56:38, wtc wrote:
> 
> We need to return ERR_FAILED because ProofVerifier::VerifyProof
> is documented to return OK or ERR_FAILED.

Done.

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.cc:91: return DoLoop(OK);
On 2013/07/02 00:56:38, wtc wrote:
> 
> I suggest removing the VerifyChain() function and just inlining these two
lines
> at the only call site.

Done.

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.cc:151: // Exit DoLoop and return the
result to the caller to ProofVerify.
On 2013/07/02 00:56:38, wtc wrote:
> 
> Typo: ProofVerify => VerifyProof

Done.

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.cc:230: DLOG(WARNING) << "VerifyFinal
success";
On 2013/07/02 00:56:38, wtc wrote:
> 
> This should be DLOG(INFO), or should be removed.

Done.

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
File net/quic/crypto/proof_verifier_chromium.h (right):

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.h:71: CertVerifyResult
cert_verify_result_;
On 2013/07/01 16:23:18, agl wrote:
> These members suggest that you're going to have to create a new ProofVerifier
> for every connection, is that the plan? I think that needs more changes than
you
> have made because the ProofVerifier is still attached to the
> QuicCryptoClientConfig and many connections share the same
> QuicCryptoClientConfig.
> 
> Rather, I think you need one ProofVerifier per process (i.e. it's a global),
and
> this state needs to be wrapped up in an internal object that's created for
each
> call to VerifyProof.

We thought we would have one ProofVerifier per Session. Added a TODO to support
multiple requests for one ProofVerifier per process. Added a DCHECK that we
don't have multiple requests for one ProofVerifier.

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.h:79: BoundNetLog net_log_;
On 2013/07/02 00:56:38, wtc wrote:
> 
> The net_log_ member probably should be initialized by an argument to the
> constructor.
> 
> Take SSLClientSocketNSS for an example. Its net_log_ member is initialized by
> the
> NetLog of the transport_socket input to the constructor.

Done.

https://codereview.chromium.org/17385010/diff/102001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/17385010/diff/102001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:167: next_state_ =
STATE_PROOF_VERIFICATION_COMPLETED;
On 2013/07/01 16:23:18, agl wrote:
> This needs to be STATE_PROOF_VERIFY I think, and that would mean that result
> needs to be checked on entry to STATE_PROOF_VERIFY.
> 
> (Because the cached->SetProofValid must correspond to the cached->certs()
given
> to VerifyProof, and cached->certs() may have changed between
STATE_PROOF_VERIFY
> and STATE_PROOF_VERIFICATION_COMPLETED.)
> 
> That also means that VerifyProof must not return ERR_IO_PENDING if asked the
> same question twice. If that's not possible then the cache could be
implemented
> here by adding a generation counter to CachedState and remembering which
> generation was verified.

Implemented generation_counter.

Didn't change the next_state_. Got the error_details from ProofVerifier for
synchronous and asynchronous calls. Kept the error_details in ProofVerifier
(from ownership point of view). Another choice is to store it in session().

Done.

https://codereview.chromium.org/17385010/diff/102001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.h (right):

https://codereview.chromium.org/17385010/diff/102001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.h:51: STATE_PROOF_VERIFICATION_COMPLETED,
On 2013/07/02 00:56:38, wtc wrote:
> 
> Nit: these two states should be named STATE_VERIFY_PROOF and
> STATE_VERIFY_PROOF_COMPLETE by our convention.

Done.

https://codereview.chromium.org/17385010/diff/102001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.h:59: void VerifyProofCompleted(int result);
On 2013/07/02 00:56:38, wtc wrote:
> 
> Nit: this method probably should be named OnVerifyProofComplete
> by our convention.

Done.

https://codereview.chromium.org/17385010/diff/102001/net/quic/test_tools/cryp...
File net/quic/test_tools/crypto_test_utils_chromium.cc (right):

https://codereview.chromium.org/17385010/diff/102001/net/quic/test_tools/cryp...
net/quic/test_tools/crypto_test_utils_chromium.cc:8: #include "base/logging.h"
On 2013/07/02 00:56:38, wtc wrote:
> 
> "base/logging.h" doesn't seem necessary...

Done.

https://codereview.chromium.org/17385010/diff/102001/net/quic/test_tools/cryp...
net/quic/test_tools/crypto_test_utils_chromium.cc:17: #include
"testing/gtest/include/gtest/gtest.h"
On 2013/07/02 00:56:38, wtc wrote:
> 
> "testing/gtest/include/gtest/gtest.h" doesn't seem necessary.

Done.

https://codereview.chromium.org/17385010/diff/102001/net/quic/test_tools/cryp...
net/quic/test_tools/crypto_test_utils_chromium.cc:26: const std::string&
cert_file)
On 2013/07/02 00:56:38, wtc wrote:
> 
> This constructor does not need to be marked "explicit" because it has
> two arguments.

Done.

[agl, 2013-07-02 15:20:08]

https://codereview.chromium.org/17385010/diff/129001/net/quic/crypto/proof_ve...
File net/quic/crypto/proof_verifier_chromium.cc (right):

https://codereview.chromium.org/17385010/diff/129001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.cc:141: int flags =
CertVerifier::VERIFY_REV_CHECKING_ENABLED |
I would remove both of these.

VERIFY_REV_CHECKING_ENABLED is disabled by default in Chrome, although it's an
option in the UI. Plumbing that option in would be nice, but not critical.

Without rev checking, VERIFY_CERT_IO_ENABLED is a hangover for broken HTTPS
sites and we should try and start a new world with QUIC :)

https://codereview.chromium.org/17385010/diff/129001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/17385010/diff/129001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:153: // cached->signature is empty then we
don't check the certificates.
We can receive multiple REJ in a handshake. At the moment, if a REJ comes in
with no signature then we'll go to STATE_VERIFY_PROOF_COMPLETED and end up
calling cached->SetProofValid. i.e. sending no certificates would be accepted by
this code.

https://codereview.chromium.org/17385010/diff/129001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:160:
DCHECK_EQ(verifier->generation_counter(), 0u);
Why should the generation counter always be zero? We might end up verifying
several proofs.

https://codereview.chromium.org/17385010/diff/129001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:189:
CloseConnectionWithDetails(QUIC_PROOF_INVALID,
The generation counter changing isn't an error. We should just loop back to
STATE_VERIFY_PROOF.

https://codereview.chromium.org/17385010/diff/129001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:270: DVLOG(1) << "VerifyProof completed: "
<< result;
Is it ensured that this callback is always made on the same thread? If so, could
you comment that?

[Ryan Hamilton, 2013-07-02 16:24:50]

lgtm

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
File net/quic/crypto/proof_verifier.h (right):

https://codereview.chromium.org/17385010/diff/102001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier.h:41: virtual std::string error_details() = 0;
On 2013/07/02 14:19:50, ramant wrote:
> On 2013/07/02 00:56:38, wtc wrote:
> > 
> > |error_details| should still be an output argument of VerifyProof().
> > VerifyProof() can return an output (in addition to the 'int' result)
> > asynchronously. The caller needs to make sure the memory pointed to
> > by |error_details| remains valid until VerifyProof() completes or is
> > aborted.
> > 
> > This reminds me that the ProofVerifier destructor needs to abort a
> > pending VerifyProof() operation.
> 
> For |error_details| to be valid until ProofVerifier, stored errors in
> |error_details| on ProofVerifier for both synchronous and asynchronous calls.
> The other choice is to store the error details in |session|. Hope this is ok.
> 
> We call ~SingleRequestCertVerifier and it does a CancelRequest.
> Done.

Or, alternatively, instead of taking a completion callback it could take a
ProofVerifierDelegate* where ProofVerifierDelegate could be defined to have:

  void OnProffVerificationComplete(int rv, const std::string& error_details) =
0;

That also has the virutal of being google3 compatible.

[ramant (doing other things), 2013-07-02 19:45:51]

Could you please take another look? thanks much.

https://codereview.chromium.org/17385010/diff/129001/net/quic/crypto/proof_ve...
File net/quic/crypto/proof_verifier_chromium.cc (right):

https://codereview.chromium.org/17385010/diff/129001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.cc:141: int flags =
CertVerifier::VERIFY_REV_CHECKING_ENABLED |
On 2013/07/02 15:20:08, agl wrote:
> I would remove both of these.
> 
> VERIFY_REV_CHECKING_ENABLED is disabled by default in Chrome, although it's an
> option in the UI. Plumbing that option in would be nice, but not critical.
> 
> Without rev checking, VERIFY_CERT_IO_ENABLED is a hangover for broken HTTPS
> sites and we should try and start a new world with QUIC :)
> 

Done.

https://codereview.chromium.org/17385010/diff/129001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/17385010/diff/129001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:153: // cached->signature is empty then we
don't check the certificates.
On 2013/07/02 15:20:08, agl wrote:
> We can receive multiple REJ in a handshake. At the moment, if a REJ comes in
> with no signature then we'll go to STATE_VERIFY_PROOF_COMPLETED and end up
> calling cached->SetProofValid. i.e. sending no certificates would be accepted
by
> this code.

Reorganized the code. Could you take another look.

https://codereview.chromium.org/17385010/diff/129001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:160:
DCHECK_EQ(verifier->generation_counter(), 0u);
On 2013/07/02 15:20:08, agl wrote:
> Why should the generation counter always be zero? We might end up verifying
> several proofs.

Done.

https://codereview.chromium.org/17385010/diff/129001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:189:
CloseConnectionWithDetails(QUIC_PROOF_INVALID,
On 2013/07/02 15:20:08, agl wrote:
> The generation counter changing isn't an error. We should just loop back to
> STATE_VERIFY_PROOF.

Done.

https://codereview.chromium.org/17385010/diff/129001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:270: DVLOG(1) << "VerifyProof completed: "
<< result;
On 2013/07/02 15:20:08, agl wrote:
> Is it ensured that this callback is always made on the same thread? If so,
could
> you comment that?

Added the comment to VerifyProof.

Done.

[agl, 2013-07-02 20:41:31]

LGTM

https://codereview.chromium.org/17385010/diff/158001/net/quic/crypto/crypto_h...
File net/quic/crypto/crypto_handshake.h (right):

https://codereview.chromium.org/17385010/diff/158001/net/quic/crypto/crypto_h...
net/quic/crypto/crypto_handshake.h:356: ProofVerifier* proof_verifier() const;
It seems that removing this const isn't needed by this CL, but it's probably a
good change anyway.

https://codereview.chromium.org/17385010/diff/158001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/17385010/diff/158001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:75:
session()->connection()->SetDefaultEncryptionLevel(ENCRYPTION_NONE);
Where did this come from? When can the encryption level not already be NONE at
this point?

[ramant (doing other things), 2013-07-02 21:22:20]

Will land after wtc's review.

thanks very much Adam.

https://codereview.chromium.org/17385010/diff/158001/net/quic/crypto/crypto_h...
File net/quic/crypto/crypto_handshake.h (right):

https://codereview.chromium.org/17385010/diff/158001/net/quic/crypto/crypto_h...
net/quic/crypto/crypto_handshake.h:356: ProofVerifier* proof_verifier() const;
On 2013/07/02 20:41:32, agl wrote:
> It seems that removing this const isn't needed by this CL, but it's probably a
> good change anyway.

Chrome's linux_clang was complaining because VerifyProof was updating member
variables of ProofVerifier.

https://codereview.chromium.org/17385010/diff/158001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/17385010/diff/158001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:75:
session()->connection()->SetDefaultEncryptionLevel(ENCRYPTION_NONE);
On 2013/07/02 20:41:32, agl wrote:
> Where did this come from? When can the encryption level not already be NONE at
> this point?

In case STATE_RECV_REJ, we were calling
"session()->connection()->SetDefaultEncryptionLevel(ENCRYPTION_NONE);" at
line#162 and then setting "next_state_ = STATE_SEND_CHLO;".

rch wondered the same.

[commit-bot: I haz the power, 2013-07-02 23:12:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/17385010/170001

[wtc, 2013-07-03 00:20:26]

Patch set 12 LGTM. You can make the changes I suggested in a
follow-up CL.

I didn't read the prior reviews by agl and rch, so I apologize
if I asked about something that was already discussed.

https://codereview.chromium.org/17385010/diff/170001/net/data/ssl/certificate...
File net/data/ssl/certificates/README (right):

https://codereview.chromium.org/17385010/diff/170001/net/data/ssl/certificate...
net/data/ssl/certificates/README:219: - quic_proof_verify.crt

Nit: we should change this back to quic_root.crt. (Do this after
this CL is committed.)

https://codereview.chromium.org/17385010/diff/170001/net/quic/crypto/crypto_h...
File net/quic/crypto/crypto_handshake.cc (right):

https://codereview.chromium.org/17385010/diff/170001/net/quic/crypto/crypto_h...
net/quic/crypto/crypto_handshake.cc:467: ++generation_counter_;

It may be a good idea to create a function, say SetProofInvalid,
for these two lines (lines 466-467), so that server_config_valid_
and generation_counter_ are kept in sync.

https://codereview.chromium.org/17385010/diff/170001/net/quic/crypto/crypto_h...
File net/quic/crypto/crypto_handshake.h (right):

https://codereview.chromium.org/17385010/diff/170001/net/quic/crypto/crypto_h...
net/quic/crypto/crypto_handshake.h:292: // |server_config_sig_| combination.

Nit: it would be nice to point out that generation_counter_ is
incremented whenever we reset server_config_valid_ to false.

https://codereview.chromium.org/17385010/diff/170001/net/quic/crypto/proof_ve...
File net/quic/crypto/proof_verifier_chromium.cc (right):

https://codereview.chromium.org/17385010/diff/170001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.cc:46: cert_verify_result_.Reset();

Just wanted to point out that these two are not necessary in a destructor.
They are just defensive programming.

verifier_.reset(), however, is necessary to abort a pending CertVerifier
request.

https://codereview.chromium.org/17385010/diff/170001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.cc:79: cert_verify_result_.cert_status =
CERT_STATUS_INVALID;

Ah, I missed this earlier. Since cert_verify_result_ is for internal use,
we don't need to set cert_verify_result_.cert_status before returning
ERR_FAILED.

https://codereview.chromium.org/17385010/diff/170001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.cc:134: int flags = 0;

Just wanted to make sure you want |flags| to be 0.

https://codereview.chromium.org/17385010/diff/170001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.cc:152: ErrorToString(result));

Nit: indentation is off by one.

https://codereview.chromium.org/17385010/diff/170001/net/quic/crypto/proof_ve...
File net/quic/crypto/proof_verifier_chromium.h (right):

https://codereview.chromium.org/17385010/diff/170001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.h:27: class X509Certificate;

Nit: some of these forward declarations are not necessary because the
headers that define these types are already included. For example,
CertVerifyResult and X509Certificate, and maybe BoundNetLog.

Nit: it is possible that X509Certificate just needs a forward declaration
in this file.

https://codereview.chromium.org/17385010/diff/170001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.h:34: const BoundNetLog& net_log);

Remove "explicit" because this constructor has two arguments.

https://codereview.chromium.org/17385010/diff/170001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.h:78: uint64 generation_counter_;

I believe the generation_counter_ member is unused.

https://codereview.chromium.org/17385010/diff/170001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.cc (right):

https://codereview.chromium.org/17385010/diff/170001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:73: case STATE_SEND_CHLO: {

Nit: we may want to DCHECK that |in| is NULL and |result| is |OK|
for the STATE_SEND_CHLO state.

https://codereview.chromium.org/17385010/diff/170001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:75:
session()->connection()->SetDefaultEncryptionLevel(ENCRYPTION_NONE);

"subsequent" probably should be removed from this comment.
It made sense when this comment was in the STATE_RECV_REJ state.

https://codereview.chromium.org/17385010/diff/170001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:156: continue;

Nit: it is equivalent, but I think it looks nicer to use "break"
instead of "continue" here.

https://codereview.chromium.org/17385010/diff/170001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:187: DCHECK(verifier);

Delete these two lines. |verifier| is unused. You are getting
the generation counter from |cached| instead.

https://codereview.chromium.org/17385010/diff/170001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.cc:192: continue;

Nit: similarly, I suggest using "break" instead of "continue" here.
Or you can use if-else:

  if (generation_counter_ != cached->generation_counter()) {
    next_state_ = STATE_VERIFY_PROOF;
  } else {
    cached->SetProofValid();
    next_state_ = STATE_SEND_CHLO;
  }
  break;

https://codereview.chromium.org/17385010/diff/170001/net/quic/quic_crypto_cli...
File net/quic/quic_crypto_client_stream.h (right):

https://codereview.chromium.org/17385010/diff/170001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.h:51: STATE_VERIFY_PROOF_COMPLETED,

Nit: COMPLETED => COMPLETE

by our convention of state machines.

https://codereview.chromium.org/17385010/diff/170001/net/quic/quic_crypto_cli...
net/quic/quic_crypto_client_stream.h:56: // |in| is NULL for the first call.

Nit: document the |result| argument.

https://codereview.chromium.org/17385010/diff/170001/net/quic/quic_session.h
File net/quic/quic_session.h (right):

https://codereview.chromium.org/17385010/diff/170001/net/quic/quic_session.h#...
net/quic/quic_session.h:114: virtual void SetProofVerifier(ProofVerifier*
verifier);

Nit: the names of the getter and setter methods for ProofVerifier
have different styles.

https://codereview.chromium.org/17385010/diff/170001/net/quic/test_tools/cryp...
File net/quic/test_tools/crypto_test_utils_chromium.cc (right):

https://codereview.chromium.org/17385010/diff/170001/net/quic/test_tools/cryp...
net/quic/test_tools/crypto_test_utils_chromium.cc:7: #include
"base/files/file_path.h"

Nit: we probably don't need to include this header.

[commit-bot: I haz the power, 2013-07-03 02:10:08]

Retried try job too often on win_rel for step(s) net_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-07-03 02:10:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/17385010/201001

[commit-bot: I haz the power, 2013-07-03 03:18:31]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-03 04:10:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/17385010/213002

[ramant (doing other things), 2013-07-03 05:46:33]

Implemented changes suggested by wtc in the following CL.

https://codereview.chromium.org/18033005

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/data/ssl/cert...
File net/data/ssl/certificates/README (right):

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/data/ssl/cert...
net/data/ssl/certificates/README:219: - quic_proof_verify.crt
On 2013/07/03 00:20:26, wtc wrote:
> 
> Nit: we should change this back to quic_root.crt. (Do this after
> this CL is committed.)

Done.

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/crypto/c...
File net/quic/crypto/crypto_handshake.cc (right):

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/crypto/c...
net/quic/crypto/crypto_handshake.cc:467: ++generation_counter_;
On 2013/07/03 00:20:26, wtc wrote:
> 
> It may be a good idea to create a function, say SetProofInvalid,
> for these two lines (lines 466-467), so that server_config_valid_
> and generation_counter_ are kept in sync.

Done.

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/crypto/c...
File net/quic/crypto/crypto_handshake.h (right):

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/crypto/c...
net/quic/crypto/crypto_handshake.h:292: // |server_config_sig_| combination.
On 2013/07/03 00:20:26, wtc wrote:
> 
> Nit: it would be nice to point out that generation_counter_ is
> incremented whenever we reset server_config_valid_ to false.

Done.

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/crypto/p...
File net/quic/crypto/proof_verifier_chromium.cc (right):

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/crypto/p...
net/quic/crypto/proof_verifier_chromium.cc:46: cert_verify_result_.Reset();
On 2013/07/03 00:20:26, wtc wrote:
> 
> Just wanted to point out that these two are not necessary in a destructor.
> They are just defensive programming.
> 
> verifier_.reset(), however, is necessary to abort a pending CertVerifier
> request.

Done.

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/crypto/p...
net/quic/crypto/proof_verifier_chromium.cc:79: cert_verify_result_.cert_status =
CERT_STATUS_INVALID;
On 2013/07/03 00:20:26, wtc wrote:
> 
> Ah, I missed this earlier. Since cert_verify_result_ is for internal use,
> we don't need to set cert_verify_result_.cert_status before returning
> ERR_FAILED.

Done.

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/crypto/p...
net/quic/crypto/proof_verifier_chromium.cc:134: int flags = 0;
On 2013/07/03 00:20:26, wtc wrote:
> 
> Just wanted to make sure you want |flags| to be 0.

Did it per agl's comments.

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/crypto/p...
net/quic/crypto/proof_verifier_chromium.cc:152: ErrorToString(result));
On 2013/07/03 00:20:26, wtc wrote:
> 
> Nit: indentation is off by one.

Done.

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/crypto/p...
File net/quic/crypto/proof_verifier_chromium.h (right):

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/crypto/p...
net/quic/crypto/proof_verifier_chromium.h:27: class X509Certificate;
On 2013/07/03 00:20:26, wtc wrote:
> 
> Nit: some of these forward declarations are not necessary because the
> headers that define these types are already included. For example,
> CertVerifyResult and X509Certificate, and maybe BoundNetLog.
> 
> Nit: it is possible that X509Certificate just needs a forward declaration
> in this file.
> 

Done.

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/crypto/p...
net/quic/crypto/proof_verifier_chromium.h:34: const BoundNetLog& net_log);
On 2013/07/03 00:20:26, wtc wrote:
> 
> Remove "explicit" because this constructor has two arguments.

Done.

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/crypto/p...
net/quic/crypto/proof_verifier_chromium.h:78: uint64 generation_counter_;
On 2013/07/03 00:20:26, wtc wrote:
> 
> I believe the generation_counter_ member is unused.

Done.

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/quic_cry...
File net/quic/quic_crypto_client_stream.cc (right):

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/quic_cry...
net/quic/quic_crypto_client_stream.cc:73: case STATE_SEND_CHLO: {
On 2013/07/03 00:20:26, wtc wrote:
> 
> Nit: we may want to DCHECK that |in| is NULL and |result| is |OK|
> for the STATE_SEND_CHLO state.

Added the DCHECK for |result| is |OK|. IMO, |in| need not be NULL if the session
(or config) doesn't have a ProofVerifier and the next_state_ was STATE_RECV_REJ.
I could be wrong.

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/quic_cry...
net/quic/quic_crypto_client_stream.cc:75:
session()->connection()->SetDefaultEncryptionLevel(ENCRYPTION_NONE);
On 2013/07/03 00:20:26, wtc wrote:
> 
> "subsequent" probably should be removed from this comment.
> It made sense when this comment was in the STATE_RECV_REJ state.

Done.

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/quic_cry...
net/quic/quic_crypto_client_stream.cc:156: continue;
On 2013/07/03 00:20:26, wtc wrote:
> 
> Nit: it is equivalent, but I think it looks nicer to use "break"
> instead of "continue" here.

Done.

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/quic_cry...
net/quic/quic_crypto_client_stream.cc:187: DCHECK(verifier);
On 2013/07/03 00:20:26, wtc wrote:
> 
> Delete these two lines. |verifier| is unused. You are getting
> the generation counter from |cached| instead.

Done.

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/quic_cry...
net/quic/quic_crypto_client_stream.cc:192: continue;
On 2013/07/03 00:20:26, wtc wrote:
> 
> Nit: similarly, I suggest using "break" instead of "continue" here.
> Or you can use if-else:
> 
>   if (generation_counter_ != cached->generation_counter()) {
>     next_state_ = STATE_VERIFY_PROOF;
>   } else {
>     cached->SetProofValid();
>     next_state_ = STATE_SEND_CHLO;
>   }
>   break;

Done.

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/quic_cry...
File net/quic/quic_crypto_client_stream.h (right):

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/quic_cry...
net/quic/quic_crypto_client_stream.h:51: STATE_VERIFY_PROOF_COMPLETED,
On 2013/07/03 00:20:26, wtc wrote:
> 
> Nit: COMPLETED => COMPLETE
> 
> by our convention of state machines.

Done.

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/quic_cry...
net/quic/quic_crypto_client_stream.h:56: // |in| is NULL for the first call.
On 2013/07/03 00:20:26, wtc wrote:
> 
> Nit: document the |result| argument.

Done.

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/quic_ses...
File net/quic/quic_session.h (right):

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/quic_ses...
net/quic/quic_session.h:114: virtual void SetProofVerifier(ProofVerifier*
verifier);
On 2013/07/03 00:20:26, wtc wrote:
> 
> Nit: the names of the getter and setter methods for ProofVerifier
> have different styles.

Used the same names as it is defined in "QuicCryptoClientConfig". Changed it to
set_proof_verifier. Hope that is ok.

Done.

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/test_too...
File net/quic/test_tools/crypto_test_utils_chromium.cc (right):

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/test_too...
net/quic/test_tools/crypto_test_utils_chromium.cc:7: #include
"base/files/file_path.h"
On 2013/07/03 00:20:26, wtc wrote:
> 
> Nit: we probably don't need to include this header.

Done.

[commit-bot: I haz the power, 2013-07-03 07:45:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/17385010/228001

[commit-bot: I haz the power, 2013-07-03 10:27:47]

Change committed as 209946

[wtc, 2013-07-03 18:50:23]

Patch set 15 LGTM.

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/quic_cry...
File net/quic/quic_crypto_client_stream.cc (right):

https://chromiumcodereview.appspot.com/17385010/diff/170001/net/quic/quic_cry...
net/quic/quic_crypto_client_stream.cc:73: case STATE_SEND_CHLO: {

On 2013/07/03 05:46:34, ramant wrote:
>
> Added the DCHECK for |result| is |OK|. IMO, |in| need not be NULL if the
session
> (or config) doesn't have a ProofVerifier and the next_state_ was
STATE_RECV_REJ.
> I could be wrong.

Ah, I see. Yes, there are three ways to enter the
STATE_SEND_CHLO state. In the first way (CryptoConnect),
|in| is NULL, but in the other two ways, we enter via
some other state (STATE_RECV_REJ or STATE_VERIFY_PROOF_COMPLETE).
If we enter via STATE_RECV_REJ, |in| is not null.

https://chromiumcodereview.appspot.com/17385010/diff/228001/net/quic/crypto/p...
File net/quic/crypto/proof_test.cc (right):

https://chromiumcodereview.appspot.com/17385010/diff/228001/net/quic/crypto/p...
net/quic/crypto/proof_test.cc:18: #endif

This should be removed because it is not used in the final
version.

https://chromiumcodereview.appspot.com/17385010/diff/228001/net/quic/crypto/p...
net/quic/crypto/proof_test.cc:267: // TODO(rtenneti): Enable
VerifyECDSAKnownAnswerTest on win_rel and XP.

on win_rel and XP => on Windows

It would be nice to describe what the problem is.

[ramant (doing other things), 2013-07-03 20:15:20]

Made the changes in CL https://chromiumcodereview.appspot.com/18033005/.

thanks
raman

https://chromiumcodereview.appspot.com/17385010/diff/228001/net/quic/crypto/p...
File net/quic/crypto/proof_test.cc (right):

https://chromiumcodereview.appspot.com/17385010/diff/228001/net/quic/crypto/p...
net/quic/crypto/proof_test.cc:18: #endif
On 2013/07/03 18:50:23, wtc wrote:
> 
> This should be removed because it is not used in the final
> version.

Done.

https://chromiumcodereview.appspot.com/17385010/diff/228001/net/quic/crypto/p...
net/quic/crypto/proof_test.cc:267: // TODO(rtenneti): Enable
VerifyECDSAKnownAnswerTest on win_rel and XP.
On 2013/07/03 18:50:23, wtc wrote:
> 
> on win_rel and XP => on Windows
> 
> It would be nice to describe what the problem is.

Done.