net: add DNS certificate revocation experiment.

net: add DNS revocation experiment.

This change adds an experiment to test the feasibility of DNS based revocation
checks. For each URL loaded from a Comodo issued HTTPS site, we kick of a DNS
resolution of a TXT record at wibble.comodoca.com. Since the record has a TTL
of 20 minutes and the DnsRRResolver has a local cache, we won't actually send
requests more than three times an hour.

Privacy-wise, this leaks strictly less information than OCSP/CRL checks would
have done.

I'll revert this so that it doesn't get picked up into the M19 branch. We might
get enough data from a canary build to draw conclusions.


BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=127994

[Ryan Sleevi, 2012-03-19 18:58:14]

+cc szym, whose been working on the new DNS resolver, to see if there is
anything he cares to weigh in on.

Just some nits (what a surprise...):

https://chromiumcodereview.appspot.com/9733005/diff/1/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://chromiumcodereview.appspot.com/9733005/diff/1/net/url_request/url_req...
net/url_request/url_request_http_job.cc:81: reinterpret_cast<const
char*>(i->data), sizeof(i->data)), &base64_hash);
nit: Use arraysize() rather than sizeof() - functionally equivalent, but
arraysize() will complain more if SHA1Fingerprint changes, so it's easier to
catch.

https://chromiumcodereview.appspot.com/9733005/diff/1/net/url_request/url_req...
net/url_request/url_request_http_job.cc:761: // TODO(agl): remove once
experiment is complete, by July 2012.
Can this be implemented as a FieldTrial (base/metrics/FieldTrial.h), if
necessarily plumbed through the SSLConfigService as a static variable (as we
previously did DNS RR resolver trials and currently do the cached info
extension)?

The benefit here is that you can set a firm expiration in the event someone
falls off the update bandwagon.

https://chromiumcodereview.appspot.com/9733005/diff/1/net/url_request/url_req...
net/url_request/url_request_http_job.cc:773: RRResponse*
response(new(RRResponse));
drive by: 
RRResponse* response = new RRResponse;

Alternatively, make it clear you're not doing placement new with:
RRResponse* response(new RRResponse);

However, this syntax is less common for naked pointers - it's more commonly used
for scoped_ptr<> construction. Naked pointers historically seem to fit "Foo foo*
= new Foo"

[willchan no longer on Chromium, 2012-03-19 19:11:01]

https://chromiumcodereview.appspot.com/9733005/diff/1/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://chromiumcodereview.appspot.com/9733005/diff/1/net/url_request/url_req...
net/url_request/url_request_http_job.cc:762: if (result == OK &&
transaction_->GetResponseInfo() != NULL) {
Can we move all this code into ChromeNetworkDelegate::OnResponseStarted()? I
know it's at a later point, so we lose some data points, but I suspect you don't
care. The reason to do it there is to keep net/ clean and also prevent this
behavior leaking to a bunch of our other consumers. This code is actually used
quite widely in contexts that you wouldn't necessarily expect.
ChromeNetworkDelegate makes it so that it's only used for normal Chrome
browsing.

That way we don't need to create a DnsRRResolver for each URLRequestContext
either, we can just stash it into ChromeNetworkDelegate.

https://chromiumcodereview.appspot.com/9733005/diff/1/net/url_request/url_req...
net/url_request/url_request_http_job.cc:769: // The Comodo DNS record has a 20
minute TTL, so we won't actually
What happens if Comodo screws up their TTL? Do we have throttling at some other
level? If not, then I'm not OK with this behavior moving beyond canary.

https://chromiumcodereview.appspot.com/9733005/diff/1/net/url_request/url_req...
net/url_request/url_request_http_job.cc:777: response, 0 /* priority */,
how about using base::Owned(response) so that on shutdown we don't leak?
valgrind shutdown leaks are annoying.

[agl, 2012-03-19 20:16:48]

https://chromiumcodereview.appspot.com/9733005/diff/1/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://chromiumcodereview.appspot.com/9733005/diff/1/net/url_request/url_req...
net/url_request/url_request_http_job.cc:81: reinterpret_cast<const
char*>(i->data), sizeof(i->data)), &base64_hash);
On 2012/03/19 18:58:15, Ryan Sleevi wrote:
> nit: Use arraysize() rather than sizeof() - functionally equivalent, but
> arraysize() will complain more if SHA1Fingerprint changes, so it's easier to
> catch.

Done.

https://chromiumcodereview.appspot.com/9733005/diff/1/net/url_request/url_req...
net/url_request/url_request_http_job.cc:761: // TODO(agl): remove once
experiment is complete, by July 2012.
On 2012/03/19 18:58:15, Ryan Sleevi wrote:
> Can this be implemented as a FieldTrial

Yes, I like the idea of a date-based cutoff if all else fails. Have gated it on
a FieldTrial.

https://chromiumcodereview.appspot.com/9733005/diff/1/net/url_request/url_req...
net/url_request/url_request_http_job.cc:762: if (result == OK &&
transaction_->GetResponseInfo() != NULL) {
On 2012/03/19 19:11:01, willchan wrote:
> Can we move all this code into ChromeNetworkDelegate::OnResponseStarted()?

Done.

https://chromiumcodereview.appspot.com/9733005/diff/1/net/url_request/url_req...
net/url_request/url_request_http_job.cc:769: // The Comodo DNS record has a 20
minute TTL, so we won't actually
On 2012/03/19 19:11:01, willchan wrote:
> What happens if Comodo screws up their TTL? Do we have throttling at some
other
> level?

Have added a rate limit of once every ten minutes.

https://chromiumcodereview.appspot.com/9733005/diff/1/net/url_request/url_req...
net/url_request/url_request_http_job.cc:773: RRResponse*
response(new(RRResponse));
On 2012/03/19 18:58:15, Ryan Sleevi wrote:
> drive by: 
> RRResponse* response = new RRResponse;

Yep, that's better. Thanks.

https://chromiumcodereview.appspot.com/9733005/diff/1/net/url_request/url_req...
net/url_request/url_request_http_job.cc:777: response, 0 /* priority */,
On 2012/03/19 19:11:01, willchan wrote:
> how about using base::Owned(response)?

Done.

[willchan no longer on Chromium, 2012-03-19 21:24:33]

lgtm

https://chromiumcodereview.appspot.com/9733005/diff/4003/chrome/browser/net/c...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://chromiumcodereview.appspot.com/9733005/diff/4003/chrome/browser/net/c...
chrome/browser/net/chrome_network_delegate.cc:132: i =
public_key_hashes.begin(); i != public_key_hashes.end(); i++) {
++i

https://chromiumcodereview.appspot.com/9733005/diff/4003/chrome/browser/net/c...
chrome/browser/net/chrome_network_delegate.cc:296: base::TimeTicks
start_time(base::TimeTicks::Now());
How about reusing |now|?

https://chromiumcodereview.appspot.com/9733005/diff/4003/chrome/browser/net/c...
File chrome/browser/net/chrome_network_delegate.h (right):

https://chromiumcodereview.appspot.com/9733005/diff/4003/chrome/browser/net/c...
chrome/browser/net/chrome_network_delegate.h:105: static bool
comodo_dns_experiment_enabled_;
chrome-network-stack folks discussed and decided to use g_global_variable naming
in .cc files rather than class private static variables. Can you move this to
the .cc file and rename it?

[agl, 2012-03-21 16:16:39]

https://chromiumcodereview.appspot.com/9733005/diff/4003/chrome/browser/net/c...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://chromiumcodereview.appspot.com/9733005/diff/4003/chrome/browser/net/c...
chrome/browser/net/chrome_network_delegate.cc:132: i =
public_key_hashes.begin(); i != public_key_hashes.end(); i++) {
On 2012/03/19 21:24:33, willchan wrote:
> ++i

Done.

https://chromiumcodereview.appspot.com/9733005/diff/4003/chrome/browser/net/c...
chrome/browser/net/chrome_network_delegate.cc:296: base::TimeTicks
start_time(base::TimeTicks::Now());
On 2012/03/19 21:24:33, willchan wrote:
> How about reusing |now|?

Done.

https://chromiumcodereview.appspot.com/9733005/diff/4003/chrome/browser/net/c...
File chrome/browser/net/chrome_network_delegate.h (right):

https://chromiumcodereview.appspot.com/9733005/diff/4003/chrome/browser/net/c...
chrome/browser/net/chrome_network_delegate.h:105: static bool
comodo_dns_experiment_enabled_;
On 2012/03/19 21:24:33, willchan wrote:
> chrome-network-stack folks discussed and decided to use g_global_variable
naming
> in .cc files rather than class private static variables. Can you move this to
> the .cc file and rename it?

Done.

[commit-bot: I haz the power, 2012-03-21 16:16:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/9733005/12001

[commit-bot: I haz the power, 2012-03-21 17:42:07]

Change committed as 127994