Implement a whitelist for code-signing certificates.

Implement a whitelist for code-signing certificates.

With this change, only downloads that match a whitelisted URL or certificate will be exempt from the download protection pingback.  The certificate whitelist format allows matching on the certificate issuer along with the CN, O, or OU attributes of the certificate.

BUG=102540
TEST=DownloadProtectionServiceTest


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

[noelutz, 2011-12-01 06:46:13]

lgtm

really nicely done!

thanks,
noé.

http://codereview.chromium.org/8762007/diff/2001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

http://codereview.chromium.org/8762007/diff/2001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/download_protection_service.cc:629: if
(chain.element_size() < 2) {
add a comment for this case?

[mattm, 2011-12-01 22:27:19]

http://codereview.chromium.org/8762007/diff/2001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

http://codereview.chromium.org/8762007/diff/2001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/download_protection_service.cc:781: // 
[/CN=common_name][/O=organization][/OU=organizational unit]
Was a bit worried until I realized this is supposed to be a continuation of the
first line rather than two separate paths.  I'd put it all on one line and
ignore the line length warning, or come up with some way to shorten it (maybe
use abbreviations and define them on separate lines, or something)

http://codereview.chromium.org/8762007/diff/2001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/download_protection_service.cc:781: // 
[/CN=common_name][/O=organization][/OU=organizational unit]
I would have expected CN to be listed last, but I suppose it doesn't really
matter that much.  You can ignore this suggestion.

http://codereview.chromium.org/8762007/diff/2001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/download_protection_service.cc:785: // always be in
the order shown above.
Mention that at least one attribute will always be present.

[Brian Ryner, 2011-12-01 22:35:12]

Please take another look.

http://codereview.chromium.org/8762007/diff/2001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

http://codereview.chromium.org/8762007/diff/2001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/download_protection_service.cc:629: if
(chain.element_size() < 2) {
On 2011/12/01 06:46:13, noelutz wrote:
> add a comment for this case?

Done.

http://codereview.chromium.org/8762007/diff/2001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/download_protection_service.cc:781: // 
[/CN=common_name][/O=organization][/OU=organizational unit]
On 2011/12/01 22:27:19, mattm wrote:
> Was a bit worried until I realized this is supposed to be a continuation of
the
> first line rather than two separate paths.  I'd put it all on one line and
> ignore the line length warning, or come up with some way to shorten it (maybe
> use abbreviations and define them on separate lines, or something)

Shortened the names a bit and moved everything onto one line.

http://codereview.chromium.org/8762007/diff/2001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/download_protection_service.cc:781: // 
[/CN=common_name][/O=organization][/OU=organizational unit]
On 2011/12/01 22:27:19, mattm wrote:
> I would have expected CN to be listed last, but I suppose it doesn't really
> matter that much.  You can ignore this suggestion.

Yeah, as long as we are consistent on the server side this should be ok.

http://codereview.chromium.org/8762007/diff/2001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/download_protection_service.cc:785: // always be in
the order shown above.
On 2011/12/01 22:27:19, mattm wrote:
> Mention that at least one attribute will always be present.

Done.

[mattm, 2011-12-01 22:45:46]

lgtm

[commit-bot: I haz the power, 2011-12-01 22:49:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryner@chromium.org/8762007/6001

[commit-bot: I haz the power, 2011-12-02 02:23:42]

Change committed as 112604

[Ryan Sleevi, 2011-12-02 03:03:05]

http://codereview.chromium.org/8762007/diff/6001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/download_protection_service_unittest.cc
(right):

http://codereview.chromium.org/8762007/diff/6001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/download_protection_service_unittest.cc:744:
crypto::RSAPrivateKey::Create(1024));
Just a drive-by: Over on the net/ side, we've found this to be expensive when
running under Valgrind.

It seems to me that just using some well-known certificates from testdata_path_
would be a better/less expensive way.

Under Valgrind, it's not at all uncommon to see timeouts > 60 seconds for these
tests - even at relatively small keysizes.

[Brian Ryner, 2011-12-02 03:30:53]

Thanks Ryan.  I'll send out a follow-up that switches this to use pre-generated
certificates.

On 2011/12/02 03:03:05, Ryan Sleevi wrote:
>
http://codereview.chromium.org/8762007/diff/6001/chrome/browser/safe_browsing...
> File chrome/browser/safe_browsing/download_protection_service_unittest.cc
> (right):
> 
>
http://codereview.chromium.org/8762007/diff/6001/chrome/browser/safe_browsing...
> chrome/browser/safe_browsing/download_protection_service_unittest.cc:744:
> crypto::RSAPrivateKey::Create(1024));
> Just a drive-by: Over on the net/ side, we've found this to be expensive when
> running under Valgrind.
> 
> It seems to me that just using some well-known certificates from
testdata_path_
> would be a better/less expensive way.
> 
> Under Valgrind, it's not at all uncommon to see timeouts > 60 seconds for
these
> tests - even at relatively small keysizes.