Use the CT policy enforcer for QUIC, if specified.

Use the CT policy enforcer for QUIC, if specified.

While QUIC does not support OCSP stapling nor the CT TLS extension,
and thus cannot support CT, ensure that all certificate verifications
go through the CertPolicyEnforcer, if specified.

As a result, if CT is being enforced by an embedder, this will remove
the EV status from QUIC connections. If CT is not being enforced by
an embedder, then it will behave as normal.

BUG=504865
Committed: https://crrev.com/9541f8639b38583dd0244a4be6c244c24d5f2443
Cr-Commit-Position: refs/heads/master@{#341233}

[Ryan Sleevi, 2015-07-03 15:07:29]

rsleevi@chromium.org changed reviewers:
+ rch@chromium.org

[Ryan Sleevi, 2015-07-03 15:07:30]

Ryan: Can you take a look at this?

I'm fairly unhappy that I don't have any tests for this, so I'm hoping you can
help me. It seems the Google/Chromium abstraction is getting in the way here -
the proof_test code abstracts away the proof interactions through
../test_tools/, so I can't directly influence the ProofVerifierChromium's
creation, since the CreateProofVerifierForTesting doesn't really lend itself to
that.

Can you offer suggestions on what might be acceptable for Chromium/Google as a
way to handle policy checks like this?

[Ryan Hamilton, 2015-07-06 17:36:36]

On 2015/07/03 15:07:30, Ryan Sleevi (slow through 7-15 wrote:
> Ryan: Can you take a look at this?
> 
> I'm fairly unhappy that I don't have any tests for this, so I'm hoping you can
> help me. It seems the Google/Chromium abstraction is getting in the way here -
> the proof_test code abstracts away the proof interactions through
> ../test_tools/, so I can't directly influence the ProofVerifierChromium's
> creation, since the CreateProofVerifierForTesting doesn't really lend itself
to
> that.
> 
> Can you offer suggestions on what might be acceptable for Chromium/Google as a
> way to handle policy checks like this?

Can you say more about what a test would look like? Would it make sense to
simply add a proof_verifier_chromium_test.cc? In any case, it's not the end of
the world if we have slight differences between the chromium and internal
versions of this code. So, for example, if you wanted to make
ProofVerifierForTesting take a CertPolicyEnforcer* that wouldn't be the end of
the world.

Does that help?

[Ryan Hamilton, 2015-07-06 17:36:48]

https://codereview.chromium.org/1216943003/diff/1/net/quic/crypto/proof_verif...
File net/quic/crypto/proof_verifier_chromium.cc (left):

https://codereview.chromium.org/1216943003/diff/1/net/quic/crypto/proof_verif...
net/quic/crypto/proof_verifier_chromium.cc:267:
std::string(reinterpret_cast<const char*>(fingerprint.data), 8)));
You would know better than I would, of course, but is it OK to remove this
histogram? My guess is that it's OK because since we're not checking CT we don't
*Really* know if this is EV?

[Ryan Sleevi, 2015-07-06 17:38:51]

https://codereview.chromium.org/1216943003/diff/1/net/quic/crypto/proof_verif...
File net/quic/crypto/proof_verifier_chromium.cc (left):

https://codereview.chromium.org/1216943003/diff/1/net/quic/crypto/proof_verif...
net/quic/crypto/proof_verifier_chromium.cc:267:
std::string(reinterpret_cast<const char*>(fingerprint.data), 8)));
On 2015/07/06 17:36:48, Ryan Hamilton wrote:
> You would know better than I would, of course, but is it OK to remove this
> histogram? My guess is that it's OK because since we're not checking CT we
don't
> *Really* know if this is EV?

This code should not have been histogramming here in the first place - that's
the responsibility of CertPolicyEnforcer. The histogram here was just sort of
hacked in to mimic the CPE being there, but leads to it representing two
different things depending on whether or not QUIC is being used.

[Ryan Hamilton, 2015-07-06 17:39:43]

https://codereview.chromium.org/1216943003/diff/1/net/quic/crypto/proof_verif...
File net/quic/crypto/proof_verifier_chromium.cc (left):

https://codereview.chromium.org/1216943003/diff/1/net/quic/crypto/proof_verif...
net/quic/crypto/proof_verifier_chromium.cc:267:
std::string(reinterpret_cast<const char*>(fingerprint.data), 8)));
On 2015/07/06 17:38:51, Ryan Sleevi (slow through 7-15 wrote:
> On 2015/07/06 17:36:48, Ryan Hamilton wrote:
> > You would know better than I would, of course, but is it OK to remove this
> > histogram? My guess is that it's OK because since we're not checking CT we
> don't
> > *Really* know if this is EV?
> 
> This code should not have been histogramming here in the first place - that's
> the responsibility of CertPolicyEnforcer. The histogram here was just sort of
> hacked in to mimic the CPE being there, but leads to it representing two
> different things depending on whether or not QUIC is being used.

Ah, that makes sense. Thanks!

[Ryan Sleevi, 2015-07-06 17:42:55]

On 2015/07/06 17:36:36, Ryan Hamilton wrote:
> Can you say more about what a test would look like? Would it make sense to
> simply add a proof_verifier_chromium_test.cc? In any case, it's not the end of
> the world if we have slight differences between the chromium and internal
> versions of this code. So, for example, if you wanted to make
> ProofVerifierForTesting take a CertPolicyEnforcer* that wouldn't be the end of
> the world.
> 
> Does that help?

Certainly, making PVForT take a CPE (and a CertVerifier) would be quite ideal :)

Sample tests:
Write a MockCV that rejects all certs - make sure the PVForT fails
Write a MockCV that fails the test if called, then make sure that PVChromium
doesn't call MockCV::Verify if the signature was bad
Write a MockCV that reports certificates as EV
  - If you don't supply a CPE, that EV status is preserved (the Opera case)
  - If you supply a CPE, make sure that CPE is checked for conformance.
  - If you supply a CPE that always says no, make sure the resulting
CertVerifyResult isn't EV
  - If you supply a CPE that always says yes, make sure the resulting
CertVerfiyResult is EV
Write a MockCV that reports no certificates as EV
  - If you supply a CPE, fail the test if it's called

[Ryan Hamilton, 2015-07-06 17:45:34]

On 2015/07/06 17:42:55, Ryan Sleevi (slow through 7-15 wrote:
> On 2015/07/06 17:36:36, Ryan Hamilton wrote:
> > Can you say more about what a test would look like? Would it make sense to
> > simply add a proof_verifier_chromium_test.cc? In any case, it's not the end
of
> > the world if we have slight differences between the chromium and internal
> > versions of this code. So, for example, if you wanted to make
> > ProofVerifierForTesting take a CertPolicyEnforcer* that wouldn't be the end
of
> > the world.
> > 
> > Does that help?
> 
> Certainly, making PVForT take a CPE (and a CertVerifier) would be quite ideal
:)

Go for it!

> Sample tests:
> Write a MockCV that rejects all certs - make sure the PVForT fails
> Write a MockCV that fails the test if called, then make sure that PVChromium
> doesn't call MockCV::Verify if the signature was bad
> Write a MockCV that reports certificates as EV
>   - If you don't supply a CPE, that EV status is preserved (the Opera case)
>   - If you supply a CPE, make sure that CPE is checked for conformance.
>   - If you supply a CPE that always says no, make sure the resulting
> CertVerifyResult isn't EV
>   - If you supply a CPE that always says yes, make sure the resulting
> CertVerfiyResult is EV
> Write a MockCV that reports no certificates as EV
>   - If you supply a CPE, fail the test if it's called

[Ryan Sleevi, 2015-07-28 00:52:46]

OK, all updated, with lots of tests now :)

[Ryan Hamilton, 2015-07-28 03:36:01]

lgtm

Thanks for doing this!!

https://codereview.chromium.org/1216943003/diff/40001/net/quic/crypto/proof_v...
File net/quic/crypto/proof_verifier_chromium_test.cc (right):

https://codereview.chromium.org/1216943003/diff/40001/net/quic/crypto/proof_v...
net/quic/crypto/proof_verifier_chromium_test.cc:134: 0xc4, 0x4b, 0x53, 0x54,
Can you comment on what this value is/where it came from?

[Ryan Sleevi, 2015-07-30 01:14:18]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2015-07-30 01:14:18]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org
Link to the patchset: https://codereview.chromium.org/1216943003/#ps80001
(title: "Rebased")

[commit-bot: I haz the power, 2015-07-30 01:14:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1216943003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1216943003/80001

[commit-bot: I haz the power, 2015-07-30 01:28:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-30 01:28:48]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ryan Sleevi, 2015-07-30 17:03:34]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2015-07-30 17:04:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1216943003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1216943003/80001

[commit-bot: I haz the power, 2015-07-30 18:06:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-30 18:06:38]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Ryan Sleevi, 2015-07-30 22:54:41]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2015-07-30 22:54:42]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org
Link to the patchset: https://codereview.chromium.org/1216943003/#ps120001
(title: "Rebased")

[commit-bot: I haz the power, 2015-07-30 22:54:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1216943003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1216943003/120001

[commit-bot: I haz the power, 2015-07-31 00:07:07]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-07-31 00:07:41]

Patchset 7 (id:??) landed as
https://crrev.com/9541f8639b38583dd0244a4be6c244c24d5f2443
Cr-Commit-Position: refs/heads/master@{#341233}