Add ERR_CERTIFICATE_TRANSPARENCY_REQUIRED to error report cert errors

Add ERR_CERTIFICATE_TRANSPARENCY_REQUIRED to error report cert errors

I noticed that ERR_CERTIFICATE_TRANSPARENCY_REQUIRED wasn't being added
to cert reports, so this fixes that.

BUG=637970
Committed: https://crrev.com/d36933b3152a7b41c6909d0209687df020e734c9
Cr-Commit-Position: refs/heads/master@{#412117}

[felt, 2016-08-10 15:05:07]

The CQ bit was checked by felt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 15:05:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[felt, 2016-08-10 15:07:55]

Description was changed from

==========
Add ERR_CERTIFICATE_TRANSPARENCY_REQUIRED to lists of cert errors

I noticed that ERR_CERTIFICATE_TRANSPARENCY_REQUIRED wasn't being added
to cert reports, so this fixes that. This CL also adds the error to one
other place where it appears to be missing from the list of cert errors.

BUG=
==========

to

==========
Add ERR_CERTIFICATE_TRANSPARENCY_REQUIRED to lists of cert errors

[NOT READY FOR REVIEW YET]

I noticed that ERR_CERTIFICATE_TRANSPARENCY_REQUIRED wasn't being added
to cert reports, so this fixes that. This CL also adds the error to one
other place where it appears to be missing from the list of cert errors.

BUG=
==========

[felt, 2016-08-10 15:11:09]

The CQ bit was checked by felt@chromium.org to run a CQ dry run

[felt, 2016-08-10 15:11:21]

Patchset #1 (id:1) has been deleted

[felt, 2016-08-10 15:11:28]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-08-10 15:11:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[felt, 2016-08-10 15:13:53]

Description was changed from

==========
Add ERR_CERTIFICATE_TRANSPARENCY_REQUIRED to lists of cert errors

[NOT READY FOR REVIEW YET]

I noticed that ERR_CERTIFICATE_TRANSPARENCY_REQUIRED wasn't being added
to cert reports, so this fixes that. This CL also adds the error to one
other place where it appears to be missing from the list of cert errors.

BUG=
==========

to

==========
Add ERR_CERTIFICATE_TRANSPARENCY_REQUIRED to lists of cert errors

I noticed that ERR_CERTIFICATE_TRANSPARENCY_REQUIRED wasn't being added
to cert reports, so this fixes that. This CL also adds the error to one
other place where it appears to be missing from the list of cert errors.

BUG=
==========

[felt, 2016-08-10 15:13:54]

felt@chromium.org changed reviewers:
+ estark@chromium.org

[felt, 2016-08-10 15:14:29]

hey estark, what's the best way to test that reports are being sent with the
correct info / format?

[commit-bot: I haz the power, 2016-08-10 15:16:24]

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

[commit-bot: I haz the power, 2016-08-10 15:16:24]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[estark, 2016-08-10 16:43:35]

lgtm

For a unit test, you could add something to error_report_unittest.com, along the
lines of
https://cs.chromium.org/chromium/src/components/certificate_reporting/error_r...
-- i.e., create a SSLInfo, add the CT_REQUIRED cert status flag to it, create a
report from that SSLInfo, then serialize and deserialize it and check that it
has the correct cert_error in the deserialized report. Would that test what you
want to test?

https://codereview.chromium.org/2232813003/diff/40001/components/certificate_...
File components/certificate_reporting/error_report.cc (right):

https://codereview.chromium.org/2232813003/diff/40001/components/certificate_...
components/certificate_reporting/error_report.cc:41:
RENAME_CERT_STATUS(CERTIFICATE_TRANSPARENCY_REQUIRED,
You should be able to just do COPY_CERT_STATUS (not RENAME) for this one. IIRC
RENAME was for when the CertStatus flag is named something different than the
proto enum value, e.g. PINNED_KEY_MISSING => SSL_PINNKED_KEY_NOT_IN_CERT_CHAIN.

[estark, 2016-08-10 17:23:20]

On 2016/08/10 16:43:35, estark wrote:
> lgtm
> 
> For a unit test, you could add something to http://error_report_unittest.com,
along the

err, make that error_report_unittest.cc. error_report_unittest.com is definitely
not a thing.

> lines of
>
https://cs.chromium.org/chromium/src/components/certificate_reporting/error_r...
> -- i.e., create a SSLInfo, add the CT_REQUIRED cert status flag to it, create
a
> report from that SSLInfo, then serialize and deserialize it and check that it
> has the correct cert_error in the deserialized report. Would that test what
you
> want to test?
> 
>
https://codereview.chromium.org/2232813003/diff/40001/components/certificate_...
> File components/certificate_reporting/error_report.cc (right):
> 
>
https://codereview.chromium.org/2232813003/diff/40001/components/certificate_...
> components/certificate_reporting/error_report.cc:41:
> RENAME_CERT_STATUS(CERTIFICATE_TRANSPARENCY_REQUIRED,
> You should be able to just do COPY_CERT_STATUS (not RENAME) for this one. IIRC
> RENAME was for when the CertStatus flag is named something different than the
> proto enum value, e.g. PINNED_KEY_MISSING =>
SSL_PINNKED_KEY_NOT_IN_CERT_CHAIN.

[felt, 2016-08-15 21:32:43]

https://codereview.chromium.org/2232813003/diff/40001/components/certificate_...
File components/certificate_reporting/error_report.cc (right):

https://codereview.chromium.org/2232813003/diff/40001/components/certificate_...
components/certificate_reporting/error_report.cc:41:
RENAME_CERT_STATUS(CERTIFICATE_TRANSPARENCY_REQUIRED,
On 2016/08/10 16:43:35, estark wrote:
> You should be able to just do COPY_CERT_STATUS (not RENAME) for this one. IIRC
> RENAME was for when the CertStatus flag is named something different than the
> proto enum value, e.g. PINNED_KEY_MISSING =>
SSL_PINNKED_KEY_NOT_IN_CERT_CHAIN.

This one *is* named differently, but in a tricky way. It does not have the CERT_
prefix (unlike the others), and the macro appends CERT_.

[felt, 2016-08-15 22:29:37]

Description was changed from

==========
Add ERR_CERTIFICATE_TRANSPARENCY_REQUIRED to lists of cert errors

I noticed that ERR_CERTIFICATE_TRANSPARENCY_REQUIRED wasn't being added
to cert reports, so this fixes that. This CL also adds the error to one
other place where it appears to be missing from the list of cert errors.

BUG=
==========

to

==========
Add ERR_CERTIFICATE_TRANSPARENCY_REQUIRED to lists of cert errors

I noticed that ERR_CERTIFICATE_TRANSPARENCY_REQUIRED wasn't being added
to cert reports, so this fixes that. This CL also adds the error to one
other place where it appears to be missing from the list of cert errors.

BUG=637970
==========

[felt, 2016-08-15 23:14:16]

Regarding the unit tests, I've run into something fairly baffling.

Whenever I add the CT error as a cert status, the serialized report ends up
containing almost every error code in addition to the correct ones:

../../components/certificate_reporting/error_report_unittest.cc:91: Failure
Value of: deserialized_report.cert_error()
Expected: has 3 elements and there exists some permutation of elements such
that:
 - element #0 is equal to 14, and
 - element #1 is equal to 5, and
 - element #2 is equal to 1
  Actual: { 1, 3, 5, 13, 6, 7, 8, 9, 10, 11, 14 }, which has 11 elements

This does not happen for other error codes, and I can't for the life of me
figure out how this small change would trigger all of these other error codes
firing.

[felt, 2016-08-15 23:58:46]

On 2016/08/15 23:14:16, felt wrote:
> Regarding the unit tests, I've run into something fairly baffling.
> 
> Whenever I add the CT error as a cert status, the serialized report ends up
> containing almost every error code in addition to the correct ones:
> 
> ../../components/certificate_reporting/error_report_unittest.cc:91: Failure
> Value of: deserialized_report.cert_error()
> Expected: has 3 elements and there exists some permutation of elements such
> that:
>  - element #0 is equal to 14, and
>  - element #1 is equal to 5, and
>  - element #2 is equal to 1
>   Actual: { 1, 3, 5, 13, 6, 7, 8, 9, 10, 11, 14 }, which has 11 elements
> 
> This does not happen for other error codes, and I can't for the life of me
> figure out how this small change would trigger all of these other error codes
> firing.

OMG that's embarrassing. I was putting a NetError instead of a CertStatus.
Because the names look almost identical. Let's pretend this never happened.

[felt, 2016-08-15 23:59:10]

Description was changed from

==========
Add ERR_CERTIFICATE_TRANSPARENCY_REQUIRED to lists of cert errors

I noticed that ERR_CERTIFICATE_TRANSPARENCY_REQUIRED wasn't being added
to cert reports, so this fixes that. This CL also adds the error to one
other place where it appears to be missing from the list of cert errors.

BUG=637970
==========

to

==========
Add ERR_CERTIFICATE_TRANSPARENCY_REQUIRED to error report cert errors

I noticed that ERR_CERTIFICATE_TRANSPARENCY_REQUIRED wasn't being added
to cert reports, so this fixes that.

BUG=637970
==========

[felt, 2016-08-16 00:03:21]

The CQ bit was checked by felt@chromium.org

[felt, 2016-08-16 00:03:21]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org
Link to the patchset: https://codereview.chromium.org/2232813003/#ps100001
(title: "Removed WV change")

[commit-bot: I haz the power, 2016-08-16 00:03:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 00:52:15]

Description was changed from

==========
Add ERR_CERTIFICATE_TRANSPARENCY_REQUIRED to error report cert errors

I noticed that ERR_CERTIFICATE_TRANSPARENCY_REQUIRED wasn't being added
to cert reports, so this fixes that.

BUG=637970
==========

to

==========
Add ERR_CERTIFICATE_TRANSPARENCY_REQUIRED to error report cert errors

I noticed that ERR_CERTIFICATE_TRANSPARENCY_REQUIRED wasn't being added
to cert reports, so this fixes that.

BUG=637970
==========

[commit-bot: I haz the power, 2016-08-16 00:52:16]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-08-16 00:53:59]

Description was changed from

==========
Add ERR_CERTIFICATE_TRANSPARENCY_REQUIRED to error report cert errors

I noticed that ERR_CERTIFICATE_TRANSPARENCY_REQUIRED wasn't being added
to cert reports, so this fixes that.

BUG=637970
==========

to

==========
Add ERR_CERTIFICATE_TRANSPARENCY_REQUIRED to error report cert errors

I noticed that ERR_CERTIFICATE_TRANSPARENCY_REQUIRED wasn't being added
to cert reports, so this fixes that.

BUG=637970

Committed: https://crrev.com/d36933b3152a7b41c6909d0209687df020e734c9
Cr-Commit-Position: refs/heads/master@{#412117}
==========

[commit-bot: I haz the power, 2016-08-16 00:54:00]

Patchset 4 (id:??) landed as
https://crrev.com/d36933b3152a7b41c6909d0209687df020e734c9
Cr-Commit-Position: refs/heads/master@{#412117}