Include cert status in invalid certificate reports

Include cert status in invalid certificate reports

This CL sends |SSLInfo::cert_status| along with the rest of the data in
invalid certificate reports.

BUG=462713, 461588
Committed: https://crrev.com/1d306d54916abfd8596049dc00a268d01ca9f200
Cr-Commit-Position: refs/heads/master@{#329197}

[estark, 2015-05-05 23:58:34]

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

[estark, 2015-05-05 23:58:35]

Ryan, could you please take a look?

Context:
Sending the cert validation result is probably the most important part of
https://codereview.chromium.org/1076273002/ and on its own doesn't run into any
of the issues with mixing interstitial knowledge into //c/b/net. So I'm
submitting this change as a separate CL to have a good chance of getting it in
for M44. I'm working on the refactoring we discussed in 1076273002 in a series
of separate CLs.

[Ryan Sleevi, 2015-05-06 00:00:33]

https://codereview.chromium.org/1117173005/diff/1/chrome/browser/net/cert_log...
File chrome/browser/net/cert_logger.proto (right):

https://codereview.chromium.org/1117173005/diff/1/chrome/browser/net/cert_log...
chrome/browser/net/cert_logger.proto:44: optional uint32 cert_status = 6;
The //net flags are API compatible, but are not defined as a stable API (that
is, we can change the status flags, so long as we handle the disk cache issues
that arise)

While it sounds duplicative, I'd suggest you define your own enum here, then map
the CertStatusFlags to it.

[estark, 2015-05-06 00:50:20]

https://codereview.chromium.org/1117173005/diff/1/chrome/browser/net/cert_log...
File chrome/browser/net/cert_logger.proto (right):

https://codereview.chromium.org/1117173005/diff/1/chrome/browser/net/cert_log...
chrome/browser/net/cert_logger.proto:44: optional uint32 cert_status = 6;
On 2015/05/06 00:00:33, Ryan Sleevi wrote:
> The //net flags are API compatible, but are not defined as a stable API (that
> is, we can change the status flags, so long as we handle the disk cache issues
> that arise)
> 
> While it sounds duplicative, I'd suggest you define your own enum here, then
map
> the CertStatusFlags to it.

Done. (Now it's reporting the most serious error, like
|MapCertStatusToNetError|.)

[Ryan Sleevi, 2015-05-06 01:03:43]

That works, or you could report all the errors.

My main goal was for you not to rely on net::'s value's being long-term stable
and suitable for external (e.g. outside of //chrome) storage. However you
accomplish that (repeated ints, a single int-mask) are fine by me, as long as
your storage layer owns its persistence layer, and it translates rather than
overlays.

https://codereview.chromium.org/1117173005/diff/40001/chrome/browser/net/cert...
File chrome/browser/net/cert_logger.proto (right):

https://codereview.chromium.org/1117173005/diff/40001/chrome/browser/net/cert...
chrome/browser/net/cert_logger.proto:59: // The most serious error encountered
(if any) when validating this
newline?

https://codereview.chromium.org/1117173005/diff/40001/chrome/browser/net/cert...
chrome/browser/net/cert_logger.proto:60: // certificate chain.
I think it's fine for your proto to report all of the errors. 

For example
repeated CertError cert_error = 6;

(or something. I don't know. I don't proto well)

[Ryan Sleevi, 2015-05-06 01:05:13]

Oh, I meant to add LGTM, since my comment was more about 'alternatives'

Just double check with someone on the proto changes who has proto readability
(or at least knows proto patterns and idioms)

[estark, 2015-05-06 22:23:20]

Thanks Ryan.

https://codereview.chromium.org/1117173005/diff/40001/chrome/browser/net/cert...
File chrome/browser/net/cert_logger.proto (right):

https://codereview.chromium.org/1117173005/diff/40001/chrome/browser/net/cert...
chrome/browser/net/cert_logger.proto:59: // The most serious error encountered
(if any) when validating this
On 2015/05/06 01:03:43, Ryan Sleevi wrote:
> newline?

Done.

https://codereview.chromium.org/1117173005/diff/40001/chrome/browser/net/cert...
chrome/browser/net/cert_logger.proto:60: // certificate chain.
On 2015/05/06 01:03:43, Ryan Sleevi wrote:
> I think it's fine for your proto to report all of the errors. 
> 
> For example
> repeated CertError cert_error = 6;
> 
> (or something. I don't know. I don't proto well)

Oh, ok, done. I'll try to find someone with proto self-confidence to take a
look.

[estark, 2015-05-06 22:24:36]

Actually, felt@ and meacer@, do either of you have opinions about the .proto
changes in this CL?

[meacer, 2015-05-06 22:45:53]

meacer@chromium.org changed reviewers:
+ meacer@chromium.org

[meacer, 2015-05-06 22:45:53]

The proto looks good to me with a small comment.

https://codereview.chromium.org/1117173005/diff/100001/chrome/browser/net/cer...
File chrome/browser/net/certificate_error_reporter.cc (right):

https://codereview.chromium.org/1117173005/diff/100001/chrome/browser/net/cer...
chrome/browser/net/certificate_error_reporter.cc:119: }
Should there be a DCHECK or CertLoggerRequest::ERR_OTHER or something similar
here as a sanity check?

[estark, 2015-05-06 23:12:34]

Thanks Mustafa!

https://codereview.chromium.org/1117173005/diff/100001/chrome/browser/net/cer...
File chrome/browser/net/certificate_error_reporter.cc (right):

https://codereview.chromium.org/1117173005/diff/100001/chrome/browser/net/cer...
chrome/browser/net/certificate_error_reporter.cc:119: }
On 2015/05/06 22:45:53, Mustafa Emre Acer wrote:
> Should there be a DCHECK or CertLoggerRequest::ERR_OTHER or something similar
> here as a sanity check?

Hmm. I think ERR_OTHER would be unnecessary, because the absence of cert errors
would be equivalent to ERR_OTHER.

As for a DCHECK, I don't think that would work out of the box right now. This
class is used by ChromeFraudulentCertificateReporter to send pinning violation
reports on Google properties:
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...

It looks like the cert status flag for a pinning violation actually gets set
after the report is sent (down on line 922 of that file). So in the case of
ChromeFraudulentCertificateReporter reports, we actually won't have a
cert_status flag to report.

We could maybe rearrange that code to send the report after the cert status flag
is set, but it seems like an unnecessary guarantee to have to enforce. (Why
shouldn't callers be able to send certificate reports without cert status flags
set if they want to?)

What do you think?

[meacer, 2015-05-07 00:00:12]

Proto LGTM (but I'm no proto expert either)

https://codereview.chromium.org/1117173005/diff/100001/chrome/browser/net/cer...
File chrome/browser/net/certificate_error_reporter.cc (right):

https://codereview.chromium.org/1117173005/diff/100001/chrome/browser/net/cer...
chrome/browser/net/certificate_error_reporter.cc:119: }
On 2015/05/06 23:12:34, estark wrote:
> On 2015/05/06 22:45:53, Mustafa Emre Acer wrote:
> > Should there be a DCHECK or CertLoggerRequest::ERR_OTHER or something
similar
> > here as a sanity check?
> 
> Hmm. I think ERR_OTHER would be unnecessary, because the absence of cert
errors
> would be equivalent to ERR_OTHER.
> 
> As for a DCHECK, I don't think that would work out of the box right now. This
> class is used by ChromeFraudulentCertificateReporter to send pinning violation
> reports on Google properties:
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...
> 
> It looks like the cert status flag for a pinning violation actually gets set
> after the report is sent (down on line 922 of that file). So in the case of
> ChromeFraudulentCertificateReporter reports, we actually won't have a
> cert_status flag to report.
> 
> We could maybe rearrange that code to send the report after the cert status
flag
> is set, but it seems like an unnecessary guarantee to have to enforce. (Why
> shouldn't callers be able to send certificate reports without cert status
flags
> set if they want to?)
> 
> What do you think?

Ah, I thought that was a guarantee that you wanted to provide. If not, then this
looks good as it is. Thanks for the details!

[estark, 2015-05-08 04:59:27]

just fyi in case anyone is wondering, I'm waiting on the corresponding
server-side changes before landing this. (and the server-side CL will also get
us another pair of eyes on the proto changes)

[estark, 2015-05-11 17:15:25]

Confirmed with noelutz that adding new protobuf fields is backwards-compatible
and checked that we get a 200 response from the existing server with this
change, so going to commit this now.

[estark, 2015-05-11 17:15:29]

The CQ bit was checked by estark@chromium.org

[estark, 2015-05-11 17:15:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
rsleevi@chromium.org, meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/1117173005/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2015-05-11 17:16:10]

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

[commit-bot: I haz the power, 2015-05-11 18:39:06]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-05-11 18:40:59]

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