Add checkbox for reporting invalid TLS/SSL cert chains

chrome/browser/net/chrome_fraudulent_certificate_reporter.cc

Index: chrome/browser/net/chrome_fraudulent_certificate_reporter.cc
diff --git a/chrome/browser/net/chrome_fraudulent_certificate_reporter.cc b/chrome/browser/net/chrome_fraudulent_certificate_reporter.cc
index d5584938a7fe8e07f83636351ca752d4a03eda1a..cbaca61e5f7184c05591f33e71a56989e3f29394 100644
--- a/chrome/browser/net/chrome_fraudulent_certificate_reporter.cc
+++ b/chrome/browser/net/chrome_fraudulent_certificate_reporter.cc
@@ -27,10 +27,13 @@ namespace chrome_browser_net {
 static const char kFraudulentCertificateUploadEndpoint[] =
     "http://clients3.google.com/log_cert_error";
 
+static const char kInvalidCertificateChainUploadEndpoint[] = "";
+
 ChromeFraudulentCertificateReporter::ChromeFraudulentCertificateReporter(
     net::URLRequestContext* request_context)
     : request_context_(request_context),
-      upload_url_(kFraudulentCertificateUploadEndpoint) {
+      pinning_violation_upload_url_(kFraudulentCertificateUploadEndpoint),
+      invalid_chain_upload_url_(kInvalidCertificateChainUploadEndpoint) {
 }
 
 ChromeFraudulentCertificateReporter::~ChromeFraudulentCertificateReporter() {
@@ -61,15 +64,16 @@ static std::string BuildReport(const std::string& hostname,
 
 scoped_ptr<net::URLRequest>
 ChromeFraudulentCertificateReporter::CreateURLRequest(
-    net::URLRequestContext* context) {
+    net::URLRequestContext* context,
+    const GURL& upload_url) {
   scoped_ptr<net::URLRequest> request =
-      context->CreateRequest(upload_url_, net::DEFAULT_PRIORITY, this, NULL);
+      context->CreateRequest(upload_url, net::DEFAULT_PRIORITY, this, NULL);
   request->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
                         net::LOAD_DO_NOT_SAVE_COOKIES);
   return request.Pass();
 }
 
-void ChromeFraudulentCertificateReporter::SendReport(
+void ChromeFraudulentCertificateReporter::SendPinningViolationReport(
     const std::string& hostname,
     const net::SSLInfo& ssl_info) {
   // We do silent/automatic reporting ONLY for Google properties. For other
@@ -80,7 +84,8 @@ void ChromeFraudulentCertificateReporter::SendReport(
 
   std::string report = BuildReport(hostname, ssl_info);
 
-  scoped_ptr<net::URLRequest> url_request = CreateURLRequest(request_context_);
+  scoped_ptr<net::URLRequest> url_request =
+      CreateURLRequest(request_context_, pinning_violation_upload_url_);
   url_request->set_method("POST");
 
   scoped_ptr<net::UploadElementReader> reader(
@@ -98,6 +103,15 @@ void ChromeFraudulentCertificateReporter::SendReport(
   raw_url_request->Start();
 }
 
+void ChromeFraudulentCertificateReporter::SendInvalidChainReport(
+    const std::string& hostname,
+    const net::SSLInfo& ssl_info) {

[felt, 2015/02/18 04:16:29]

Have you thought about having a single method for both, that differs based on a
bool? That might avoid repeating code down the line, and it would be nice for
the cert pinning reporter to get some/all of the same improvements that you'll
be adding.

[felt, 2015/02/18 04:16:29]

Should this double check here that the user is opted in to the reporting
program? (Even though it doesn't actually do anything now.) (Or if you want to
delay that to another CL, add a TODO here for that?)

[estark, 2015/02/18 05:22:44]

Good idea, I added a TODO. Not immediately sure how to actually do this -- I
think we'll need access to the Profile, which I don't think we have here. Maybe
I can pass a Profile in to the ChromeFraudulentCertificateReporter constructor
and use it here to check the SBER preference... unless there is a much simpler
way that I'm missing?

[estark, 2015/02/18 05:22:44]

Yeah, I did think about that; my reasoning was that I could factor out common
code into a helper method, and that
`reporter->SendPinningViolationReport(hostname, ssl_info)` is a little more
readable than `reporter->SendReport(hostname, ssl_info, true)` (i.e. so that the
reader doesn't have to always look up that boolean parameter and figure out what
true/false means), and that having different method names makes it easier to
expand to other types of reports later (e.g. we might later want to add
SendHPKPReport). Whaddya think?

[felt, 2015/02/19 00:32:32]

Well, you could create an enum so that the param is clear (e.g. HPKP,
GOOGLE_PIN, EXTENDED_REPORTING) to avoid the boolean parameter.

Another thing to think about: with this current structure, we will send two
reports for each google.com pinning violation. One via SendInvalidChainReport,
and one via SendPinningViolationReport from its separate call site. You should
decide whether that's desirable or not in the future. If you end up condensing
them to a single call site that decides which one to send it to, then having two
separate methods might not make sense.

[felt, 2015/02/19 00:32:32]

Yes, I think that passing in a Profile to the constructor would be the best way
to do this. TODO works for now.

[estark, 2015/02/19 02:32:08]

Oh I like the enum idea! Did that. Wasn't sure whether to make it the first
argument or the last... eventually made it the first arg because it looked a
little cleaner to me but no super strong opinion, and definitely willing to be
convinced otherwise.

My initial reaction to the google.com pinning thing is that it makes sense to
get two separate reports so that we can easily answer questions such as "what %
of SSL errors are pinning violations?", but I'll have to think about that a bit.
(Maybe we already have data to answer that question?)

[felt, 2015/02/20 19:20:34]

Might make sense. I can see it go either way. We can perhaps use UMA to answer
that specific question.

+  // TODO(estark): Temporarily, since there is no upload endpoint, just log
+  // the information.
+  LOG(ERROR) << "SSL report for " << hostname << ":\n"
+             << BuildReport(hostname, ssl_info) << "\n\n";
+}
+
 void ChromeFraudulentCertificateReporter::RequestComplete(
     net::URLRequest* request) {
   std::set<net::URLRequest*>::iterator i = inflight_requests_.find(request);