Add a Clone method to ProofVerifyDetails to allow for the proof verify

Add a Clone method to ProofVerifyDetails to allow for the proof verify
details to be copied when initiailizing a CachedState from a canonical
entry.

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

[wtc, 2014-07-29 01:48:24]

Patch set 2 LGTM. Note the bugs.

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

https://codereview.chromium.org/422623004/diff/20001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.cc:36:
other->cert_verify_result.CopyFrom(cert_verify_result);

I wonder why we don't just do

  other->cert_verify_result = cert_verify_result;

since CertVerifyResult doesn't have DISALLOW_COPY_AND_ASSIGN.

https://codereview.chromium.org/422623004/diff/20001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.cc:37: return NULL;

BUG: return other;

You owe me a lunch :-)

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

https://codereview.chromium.org/422623004/diff/20001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.h:30: virtual ProofVerifyDetails*
Clone() const OVERRIDE;

Should we add a "ProofVerifyDetails interface" comment?

https://codereview.chromium.org/422623004/diff/20001/net/quic/crypto/quic_cry...
File net/quic/crypto/quic_crypto_client_config_test.cc (right):

https://codereview.chromium.org/422623004/diff/20001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config_test.cc:19: class
TestProofVerifyDetails : public ProofVerifyDetails {

Nit: we probably should nest this inside an unnamed namespace.

https://codereview.chromium.org/422623004/diff/20001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config_test.cc:22: virtual
ProofVerifyDetails* Clone() const OVERRIDE {

Nit: should we add a "ProofVerifyDetails interface" comment?

https://codereview.chromium.org/422623004/diff/20001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config_test.cc:23: return NULL;

BUG: this should return new TestProofVerifyDetails.

[Ryan Hamilton, 2014-07-29 17:04:50]

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

https://codereview.chromium.org/422623004/diff/20001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.cc:36:
other->cert_verify_result.CopyFrom(cert_verify_result);
On 2014/07/29 01:48:24, wtc wrote:
> 
> I wonder why we don't just do
> 
>   other->cert_verify_result = cert_verify_result;
> 
> since CertVerifyResult doesn't have DISALLOW_COPY_AND_ASSIGN.

Done. I wonder why it has a CopyFrom method?

https://codereview.chromium.org/422623004/diff/20001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.cc:37: return NULL;
On 2014/07/29 01:48:24, wtc wrote:
> 
> BUG: return other;
> 
> You owe me a lunch :-)

Done. (I had not yet verified the fix :>)

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

https://codereview.chromium.org/422623004/diff/20001/net/quic/crypto/proof_ve...
net/quic/crypto/proof_verifier_chromium.h:30: virtual ProofVerifyDetails*
Clone() const OVERRIDE;
On 2014/07/29 01:48:24, wtc wrote:
> 
> Should we add a "ProofVerifyDetails interface" comment?

Done.

https://codereview.chromium.org/422623004/diff/20001/net/quic/crypto/quic_cry...
File net/quic/crypto/quic_crypto_client_config_test.cc (right):

https://codereview.chromium.org/422623004/diff/20001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config_test.cc:19: class
TestProofVerifyDetails : public ProofVerifyDetails {
On 2014/07/29 01:48:24, wtc wrote:
> 
> Nit: we probably should nest this inside an unnamed namespace.

Done.

https://codereview.chromium.org/422623004/diff/20001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config_test.cc:22: virtual
ProofVerifyDetails* Clone() const OVERRIDE {
On 2014/07/29 01:48:24, wtc wrote:
> 
> Nit: should we add a "ProofVerifyDetails interface" comment?

Done.

https://codereview.chromium.org/422623004/diff/20001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config_test.cc:23: return NULL;
On 2014/07/29 01:48:24, wtc wrote:
> 
> BUG: this should return new TestProofVerifyDetails.

Done.

[Ryan Hamilton, 2014-07-29 17:05:15]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-07-29 17:06:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rch@chromium.org/422623004/40001

[Ryan Hamilton, 2014-07-29 17:36:41]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-07-29 17:37:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rch@chromium.org/422623004/60001

[Ryan Hamilton, 2014-07-29 18:23:16]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-07-29 18:25:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rch@chromium.org/422623004/80001

[wtc, 2014-07-29 19:26:11]

Patch set 5 LGTM.

[ramant (doing other things), 2014-07-29 21:40:04]

lgtm

https://codereview.chromium.org/422623004/diff/80001/net/quic/crypto/quic_cry...
File net/quic/crypto/quic_crypto_client_config.cc (right):

https://codereview.chromium.org/422623004/diff/80001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config.cc:289: }
nit: drive by. Should we consider adding a DCHECK if proof_verify_details_.get()
is NULL then server_config_valid_ is false? thanks.

[commit-bot: I haz the power, 2014-07-29 21:42:17]

Change committed as 286305

[Ryan Hamilton, 2014-07-29 21:59:19]

https://codereview.chromium.org/422623004/diff/80001/net/quic/crypto/quic_cry...
File net/quic/crypto/quic_crypto_client_config.cc (right):

https://codereview.chromium.org/422623004/diff/80001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config.cc:289: }
On 2014/07/29 21:40:04, ramant wrote:
> nit: drive by. Should we consider adding a DCHECK if
proof_verify_details_.get()
> is NULL then server_config_valid_ is false? thanks.

It's worth a shot.  Though there are 2 caveats:
1) The internal proof verifier does not generate proof verify details ever, so
we'd need to implement that. wtc is taking a look at this
2) We don't have proof verify details for insecure QUIC connections. Does the
CachedState know if it's secure? Oh, I guess we could key off of certs being
non-empty?