Add --ignore-certificate-errors-spki-list switch and UMA histogram.

Add --ignore-certificate-errors-spki-list switch and UMA histogram.

The switch takes a comma-separated list of base64-encoded, SHA-256
subject public key hashes. If any of the certs presented by the
server match a hash from the list, certificate verification is
skipped, in order to ignore certificate errors.

Presence of the flag is counted in a UMA histogram.

This should allow us to remove the more blunt
--ignore-certificate-errors switch eventually.

R=rsleevi@chromium.org
CC=agl@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2753123002
Cr-Commit-Position: refs/heads/master@{#474021}
Committed: https://chromium.googlesource.com/chromium/src/+/8402d3c5bc13e018fa75eba650ed881755e0223b

[martinkr, 2017-03-16 22:49:41]

I finally got around to adding this flag that we chatted about a while ago. 

I'm a bit unsure about which public keys to match against. We could restrict it
to only the leaf, but matching the entire chain seems reasonable as well (in
case leaf certificates under a common CA are generated on-the-fly e.g.). Also,
if NSS is able to validate the cert and generate a chain, should we include
that, too?

[Ryan Sleevi, 2017-03-16 23:19:14]

note x509_certificate_nss is only used on Linux/ChromeOS, so it'd have to be
updated for all platforms.

However, a few suggestions that might bypass this altogether :)

https://codereview.chromium.org/2753123002/diff/1/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/2753123002/diff/1/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:1032:
params->ignore_certificate_error_spki_list.insert(spki);
Should we validate this is parsable/correct? I'm just curious what we expect if
someone gives a bad string (like "foo") - is that surfaced as an error or just
silently accepted (and it would never match)?

https://codereview.chromium.org/2753123002/diff/1/content/public/common/conte...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/2753123002/diff/1/content/public/common/conte...
content/public/common/content_switches.cc:586: // A set of certificates by
public key hash for which to ignore
I think this may either be missing a word (certificates _identified_ by public
key hash?) or may just be better with fewer words (A set of public key hashes
...)

https://codereview.chromium.org/2753123002/diff/1/content/public/common/conte...
content/public/common/content_switches.cc:588: // (leaf or intermediate) matches
a public key from this list, certificate
Grammar: an SSL certificates mismatches plurality

But I'm wondering if this might be simpler as

// If the certificate chain presented by the server does not validate,
// and one or more certificates have public key hashes that match a
// key from this list, the error is ignored.

?

https://codereview.chromium.org/2753123002/diff/1/content/public/common/conte...
content/public/common/content_switches.cc:593: // length).
// The switch value is comprised of a comma-separated list of
// RFC 7469 pin-directives (e.g. pin-sha256=abcdef)

Alternatively, you could say:

// The switch value must a be a comma-separated list of Base64-encoded
// SHA-256 SPKI Fingerprints (RFC 7469, Section 2.4).

https://codereview.chromium.org/2753123002/diff/1/content/public/common/conte...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/2753123002/diff/1/content/public/common/conte...
content/public/common/content_switches.h:183: CONTENT_EXPORT extern const char
kIgnoreGpuBlacklist[];
unrelated?

https://codereview.chromium.org/2753123002/diff/1/net/cert/x509_certificate.h
File net/cert/x509_certificate.h (right):

https://codereview.chromium.org/2753123002/diff/1/net/cert/x509_certificate.h...
net/cert/x509_certificate.h:371: static SHA256HashValue
CalculatePublicKeyHashSHA256(OSCertHandle cert_handle);
We've been trying to avoid adding new public static methods to X509Certificate,
so it doesn't become a catch-all, and to keep the API from over-complicating.

I think it's probably appropriate that in the place you need to check, you can
just use the pattern we've been using

base::StringPiece spki;
if (!asn1::ExtractSPKIFromDERCert([cert der], &spki)) {
  // unparsable cert
}
hash = crypto::SHA256HashString(spki);


I don't think there'd be a big win for wrapping into a utility method on the
base class.

https://codereview.chromium.org/2753123002/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/2753123002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:1535: // in case validation was
successful (i.e. ssl_config_.public_key_hashes)?
I'm not sure why we would - but did I miss something?

https://codereview.chromium.org/2753123002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:1547: const std::string spki =
hash.ToString().substr(7);  // Strip 'sha256/'.
This ends up forcing multiple string allocation/copies. Any reason that
ignore_certificate_error_spki_list shouldn't just be a std::set<HashValue> ? Or
better yet, base::flat_set<> since this list should always be small?

Further, if you went with |hashes| as a base::flat_set<>, then you could do

auto first1 = session_params().ignore_[...].begin();
auto last1 = session_params().ignore_[...].end();
auto first2 = hashes.begin();
auto last2 = hashes.end();
while (first1 != last1 && first2 != last2) {
  if (*first1 < *first2) ++first1;
  elseif (*first2 < *first1) ++first2;
  else return true;
}
return false;


That will tell you if any element in [first2, last2) is in [first1, last1) with
a single linear scan of both.

(This is effectively std::set_intersection, except returning if at least one
item is found)

[martinkr, 2017-03-28 23:16:03]

Apologies for the delayed response, PTAL.

https://codereview.chromium.org/2753123002/diff/1/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/2753123002/diff/1/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:1032:
params->ignore_certificate_error_spki_list.insert(spki);
On 2017/03/16 23:19:13, Ryan Sleevi wrote:
> Should we validate this is parsable/correct? I'm just curious what we expect
if
> someone gives a bad string (like "foo") - is that surfaced as an error or just
> silently accepted (and it would never match)?

I don't feel strongly about it either way, but making the params field a set of
SHA256HashValue as you suggested below makes sense and resolves this.

https://codereview.chromium.org/2753123002/diff/1/content/public/common/conte...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/2753123002/diff/1/content/public/common/conte...
content/public/common/content_switches.cc:586: // A set of certificates by
public key hash for which to ignore
On 2017/03/16 at 23:19:13, Ryan Sleevi wrote:
> I think this may either be missing a word (certificates _identified_ by public
key hash?) or may just be better with fewer words (A set of public key hashes
...)

Done.

https://codereview.chromium.org/2753123002/diff/1/content/public/common/conte...
content/public/common/content_switches.cc:588: // (leaf or intermediate) matches
a public key from this list, certificate
On 2017/03/16 at 23:19:13, Ryan Sleevi wrote:
> Grammar: an SSL certificates mismatches plurality
> 
> But I'm wondering if this might be simpler as
> 
> // If the certificate chain presented by the server does not validate,
> // and one or more certificates have public key hashes that match a
> // key from this list, the error is ignored.
> 
> ?

Done.

https://codereview.chromium.org/2753123002/diff/1/content/public/common/conte...
content/public/common/content_switches.cc:593: // length).
On 2017/03/16 at 23:19:13, Ryan Sleevi wrote:
> // The switch value is comprised of a comma-separated list of
> // RFC 7469 pin-directives (e.g. pin-sha256=abcdef)
> 
> Alternatively, you could say:
> 
> // The switch value must a be a comma-separated list of Base64-encoded
> // SHA-256 SPKI Fingerprints (RFC 7469, Section 2.4).

Done.

https://codereview.chromium.org/2753123002/diff/1/content/public/common/conte...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/2753123002/diff/1/content/public/common/conte...
content/public/common/content_switches.h:183: CONTENT_EXPORT extern const char
kIgnoreGpuBlacklist[];
On 2017/03/16 23:19:13, Ryan Sleevi wrote:
> unrelated?

Screwed up a rebase, sorry.

https://codereview.chromium.org/2753123002/diff/1/net/cert/x509_certificate.h
File net/cert/x509_certificate.h (right):

https://codereview.chromium.org/2753123002/diff/1/net/cert/x509_certificate.h...
net/cert/x509_certificate.h:371: static SHA256HashValue
CalculatePublicKeyHashSHA256(OSCertHandle cert_handle);
On 2017/03/16 at 23:19:13, Ryan Sleevi wrote:
> We've been trying to avoid adding new public static methods to
X509Certificate, so it doesn't become a catch-all, and to keep the API from
over-complicating.
> 
> I think it's probably appropriate that in the place you need to check, you can
just use the pattern we've been using
> 
> base::StringPiece spki;
> if (!asn1::ExtractSPKIFromDERCert([cert der], &spki)) {
>   // unparsable cert
> }
> hash = crypto::SHA256HashString(spki);
> 
> 
> I don't think there'd be a big win for wrapping into a utility method on the
base class.

Done.

https://codereview.chromium.org/2753123002/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/2753123002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:1535: // in case validation was
successful (i.e. ssl_config_.public_key_hashes)?
On 2017/03/16 at 23:19:14, Ryan Sleevi wrote:
> I'm not sure why we would - but did I miss something?

I guess it might be possible that the cert validates ok but fails hostname
matching, with some intermediate or root in the chain not sent by the server
(and hence not in GetIntermediateCertificates()), and the user wants to ignore
errors based on that CA? Not sure how realistic that is though.

https://codereview.chromium.org/2753123002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:1547: const std::string spki =
hash.ToString().substr(7);  // Strip 'sha256/'.
On 2017/03/16 at 23:19:14, Ryan Sleevi wrote:
> This ends up forcing multiple string allocation/copies. Any reason that
ignore_certificate_error_spki_list shouldn't just be a std::set<HashValue> ? Or
better yet, base::flat_set<> since this list should always be small?
> 
> Further, if you went with |hashes| as a base::flat_set<>, then you could do
> 
> auto first1 = session_params().ignore_[...].begin();
> auto last1 = session_params().ignore_[...].end();
> auto first2 = hashes.begin();
> auto last2 = hashes.end();
> while (first1 != last1 && first2 != last2) {
>   if (*first1 < *first2) ++first1;
>   elseif (*first2 < *first1) ++first2;
>   else return true;
> }
> return false;
> 
> 
> That will tell you if any element in [first2, last2) is in [first1, last1)
with a single linear scan of both.
> 
> (This is effectively std::set_intersection, except returning if at least one
item is found)

I just assumed these sets to be tiny in all cases, I guess, but you're right.

Done.

[Ryan Sleevi, 2017-03-30 15:12:09]

A much larger design comment - soliciting for feedback before you do anything,
to get your take on it.

https://codereview.chromium.org/2753123002/diff/20001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/2753123002/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1043: }
So a thought - if we didn't want to plumb the SPKI logic all the way down, and
force it into the override, we could simply create a wrapping CertVerifier
around 578-585 which would call in to the normal CertVerifier, and then either
bypass calling the OS verifier (for those certs in the bypass list) or mask off
the errors.

That is, I'm wondering whether something like

class IgnoreErrorsCertVerifier : public CertVerifier {
 public:
  IgnoreErrorsCertVerifier(scoped_ptr<CertVerifier> next_verifier,
                           HashValueVector certs);

  ...
};

Makes more sense?

The main difference with that is that by masking off the error at the
CertVerifier level, we potentially allow it to store entries to the disk cache
or to use H/2 pooling, which I don't think the current approach would allow.

The other thing is whether we should force user-data-dir to be present (e.g. see
https://cs.chromium.org/chromium/src/chrome/browser/chrome_content_browser_cl...
or
https://cs.chromium.org/chromium/src/chrome/common/secure_origin_whitelist.cc...)

[martinkr, 2017-03-30 17:33:38]

https://codereview.chromium.org/2753123002/diff/20001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/2753123002/diff/20001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1043: }
On 2017/03/30 15:12:09, Ryan Sleevi wrote:
> So a thought - if we didn't want to plumb the SPKI logic all the way down, and
> force it into the override, we could simply create a wrapping CertVerifier
> around 578-585 which would call in to the normal CertVerifier, and then either
> bypass calling the OS verifier (for those certs in the bypass list) or mask
off
> the errors.
> 
> That is, I'm wondering whether something like
> 
> class IgnoreErrorsCertVerifier : public CertVerifier {
>  public:
>   IgnoreErrorsCertVerifier(scoped_ptr<CertVerifier> next_verifier,
>                            HashValueVector certs);
> 
>   ...
> };
> 
> Makes more sense?
>
> The main difference with that is that by masking off the error at the
> CertVerifier level, we potentially allow it to store entries to the disk cache
> or to use H/2 pooling, which I don't think the current approach would allow.

Yeah, I think it would also be easier to understand than the current override
logic (at least for me who's not terribly familiar with this code).
 
> The other thing is whether we should force user-data-dir to be present (e.g.
see
>
https://cs.chromium.org/chromium/src/chrome/browser/chrome_content_browser_cl...
> or
>
https://cs.chromium.org/chromium/src/chrome/common/secure_origin_whitelist.cc...)

Good question. I lean towards yes because it doesn't seem to complicate things
when this switch is used in test automation but probably provides a meaningful
security benefit for human users. OTOH, there is nothing preventing you from
setting user-data-dir to  ~/.config/google-chrome/Default, is there? So if we're
worried about users whitelisting some SPKI because stackoverflow told them so
after they couldn't connect to a site, then this is probably not an additional
deterrent against that.

[Ryan Sleevi, 2017-03-30 18:12:27]

On 2017/03/30 17:33:38, martinkr wrote:
> Good question. I lean towards yes because it doesn't seem to complicate things
> when this switch is used in test automation but probably provides a meaningful
> security benefit for human users. OTOH, there is nothing preventing you from
> setting user-data-dir to  ~/.config/google-chrome/Default, is there? So if
we're
> worried about users whitelisting some SPKI because stackoverflow told them so
> after they couldn't connect to a site, then this is probably not an additional
> deterrent against that.

Yeah, that's been the general sentiment. It's somewhat defense in depth, but
Stack Overflow is Stack overflow.

Do you think you have sufficient knowledge to try the CertVerifier approach? I
think it should work, but feel free to ping on gchat if you run into any issues
so that you're not stalled.

[martinkr, 2017-03-30 18:13:14]

On 2017/03/30 18:12:27, Ryan Sleevi wrote:
> On 2017/03/30 17:33:38, martinkr wrote:
> > Good question. I lean towards yes because it doesn't seem to complicate
things
> > when this switch is used in test automation but probably provides a
meaningful
> > security benefit for human users. OTOH, there is nothing preventing you from
> > setting user-data-dir to  ~/.config/google-chrome/Default, is there? So if
> we're
> > worried about users whitelisting some SPKI because stackoverflow told them
so
> > after they couldn't connect to a site, then this is probably not an
additional
> > deterrent against that.
> 
> Yeah, that's been the general sentiment. It's somewhat defense in depth, but
> Stack Overflow is Stack overflow.
> 
> Do you think you have sufficient knowledge to try the CertVerifier approach? I
> think it should work, but feel free to ping on gchat if you run into any
issues
> so that you're not stalled.

Yup, sounds good, thanks!

[martinkr, 2017-04-06 22:31:07]

PTAL

[Ryan Sleevi, 2017-04-07 16:08:05]

Thanks for continuing to work on this! Some thoughts below, but feel free to
reach out if it's easier to discuss :)

(Note: Sometimes, the answer is just trying to make sure the design is
understood and documented on the code review, should anyone have to do source
archaeology in the future and wonder why )

https://codereview.chromium.org/2753123002/diff/60001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/2753123002/diff/60001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:585: if
(command_line.HasSwitch(switches::kIgnoreCertificateErrorsSPKIList)) {
I wasn't sure, but I thought you were thinking --user-data-dir was a
sufficient/appropriate hurdle (even if it could be set to the user's data dir)

https://codereview.chromium.org/2753123002/diff/60001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:592:
net::IgnoreErrorsCertVerifier::MakeWhitelist(spki_list));
This removes the command-line validation aspect, right? Since the CertVerifier
will be created w/o issue?

https://codereview.chromium.org/2753123002/diff/60001/content/public/common/c...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/2753123002/diff/60001/content/public/common/c...
content/public/common/content_switches.h:182: CONTENT_EXPORT extern const char
kIgnoreCertificateErrorsSPKIList[];
I think this belongs in chrome/common/chrome_switches, since only the Chrome
initializer sets it, right?

https://codereview.chromium.org/2753123002/diff/60001/net/cert/ignore_errors_...
File net/cert/ignore_errors_cert_verifier.cc (right):

https://codereview.chromium.org/2753123002/diff/60001/net/cert/ignore_errors_...
net/cert/ignore_errors_cert_verifier.cc:83: callback);
I'm not sure - why do you chain to the verifier if the error will be ignored?

Are you trying to have the verify_result populated? The NetLog?

https://codereview.chromium.org/2753123002/diff/60001/net/cert/ignore_errors_...
File net/cert/ignore_errors_cert_verifier.h (right):

https://codereview.chromium.org/2753123002/diff/60001/net/cert/ignore_errors_...
net/cert/ignore_errors_cert_verifier.h:6: #define
NET_CERT_IGNORE_ERRORS_CERT_VERIFIER_H_
Seems like this could be moved to chrome/browser/ssl , right?

Is there a reason for putting it in //net ?

https://codereview.chromium.org/2753123002/diff/60001/net/cert/ignore_errors_...
File net/cert/ignore_errors_cert_verifier_unittest.cc (right):

https://codereview.chromium.org/2753123002/diff/60001/net/cert/ignore_errors_...
net/cert/ignore_errors_cert_verifier_unittest.cc:82: auto test_cert =
GetNonWhitelistedTestCert();
We try to forbid auto here when it can hide memory management subtleties (in
particular, smart pointers). For example, at this call site, it's unclear the
type that test_cert will produce.

https://google.github.io/styleguide/cppguide.html#auto describes it as
noisy/obvious/unimportant, but these types do aid clarity.

The discussion linked in https://chromium-cpp.appspot.com/ is useful (and in
particular,
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/5-Bt3BJzAo0/EWcU2...
)

https://codereview.chromium.org/2753123002/diff/60001/net/cert/mock_cert_veri...
File net/cert/mock_cert_verifier.cc (right):

https://codereview.chromium.org/2753123002/diff/60001/net/cert/mock_cert_veri...
net/cert/mock_cert_verifier.cc:60: goto exit;
We don't use goto in Chromium code

[martinkr, 2017-04-07 21:40:57]

https://codereview.chromium.org/2753123002/diff/60001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/2753123002/diff/60001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:585: if
(command_line.HasSwitch(switches::kIgnoreCertificateErrorsSPKIList)) {
On 2017/04/07 16:08:05, Ryan Sleevi wrote:
> I wasn't sure, but I thought you were thinking --user-data-dir was a
> sufficient/appropriate hurdle (even if it could be set to the user's data dir)

Yup, forgot about this. Done (and updated the flag description).

https://codereview.chromium.org/2753123002/diff/60001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:592:
net::IgnoreErrorsCertVerifier::MakeWhitelist(spki_list));
On 2017/04/07 16:08:05, Ryan Sleevi wrote:
> This removes the command-line validation aspect, right? Since the CertVerifier
> will be created w/o issue?

MakeWhitelist has the same "validation" as before, in that it logs an error if
any of the fingerprints aren't valid.

https://codereview.chromium.org/2753123002/diff/60001/content/public/common/c...
File content/public/common/content_switches.h (right):

https://codereview.chromium.org/2753123002/diff/60001/content/public/common/c...
content/public/common/content_switches.h:182: CONTENT_EXPORT extern const char
kIgnoreCertificateErrorsSPKIList[];
On 2017/04/07 16:08:05, Ryan Sleevi wrote:
> I think this belongs in chrome/common/chrome_switches, since only the Chrome
> initializer sets it, right?

Probably. :) I was just trying to colocate it with kIgnoreCertificateErrors to
be honest. I don't think I fully understand the difference.

https://codereview.chromium.org/2753123002/diff/60001/net/cert/ignore_errors_...
File net/cert/ignore_errors_cert_verifier.cc (right):

https://codereview.chromium.org/2753123002/diff/60001/net/cert/ignore_errors_...
net/cert/ignore_errors_cert_verifier.cc:83: callback);
On 2017/04/07 16:08:05, Ryan Sleevi wrote:
> I'm not sure - why do you chain to the verifier if the error will be ignored?
> 
> Are you trying to have the verify_result populated? The NetLog?

IIUC, at least CertVerifyResult.verified_cert needs to be set. So we have the
choice between filling it here ourselves (presumably with just the unverified
chain) or having verifier_ do it (and possibly finding a different chain than
the one presented).

I couldn't really think of a reason not to do the latter, and AFAIU that's the
same behavior you'd get with --ignore-certificate-errors, so I opted for that.

https://codereview.chromium.org/2753123002/diff/60001/net/cert/ignore_errors_...
File net/cert/ignore_errors_cert_verifier.h (right):

https://codereview.chromium.org/2753123002/diff/60001/net/cert/ignore_errors_...
net/cert/ignore_errors_cert_verifier.h:6: #define
NET_CERT_IGNORE_ERRORS_CERT_VERIFIER_H_
On 2017/04/07 16:08:05, Ryan Sleevi wrote:
> Seems like this could be moved to chrome/browser/ssl , right?
> 
> Is there a reason for putting it in //net ?

Ah yes, I wasn't super familiar with the code organization, but this makes
sense.

Done.

https://codereview.chromium.org/2753123002/diff/60001/net/cert/ignore_errors_...
File net/cert/ignore_errors_cert_verifier_unittest.cc (right):

https://codereview.chromium.org/2753123002/diff/60001/net/cert/ignore_errors_...
net/cert/ignore_errors_cert_verifier_unittest.cc:82: auto test_cert =
GetNonWhitelistedTestCert();
On 2017/04/07 16:08:05, Ryan Sleevi wrote:
> We try to forbid auto here when it can hide memory management subtleties (in
> particular, smart pointers). For example, at this call site, it's unclear the
> type that test_cert will produce.
> 
> https://google.github.io/styleguide/cppguide.html#auto describes it as
> noisy/obvious/unimportant, but these types do aid clarity.
> 
> The discussion linked in https://chromium-cpp.appspot.com/ is useful (and in
> particular,
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/5-Bt3BJzAo0/EWcU2...
> )

Yep, agreed. Done.

https://codereview.chromium.org/2753123002/diff/60001/net/cert/mock_cert_veri...
File net/cert/mock_cert_verifier.cc (right):

https://codereview.chromium.org/2753123002/diff/60001/net/cert/mock_cert_veri...
net/cert/mock_cert_verifier.cc:60: goto exit;
On 2017/04/07 16:08:05, Ryan Sleevi wrote:
> We don't use goto in Chromium code

Done.

[Ryan Sleevi, 2017-04-12 21:53:18]

https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/igno...
File chrome/browser/ssl/ignore_errors_cert_verifier.cc (right):

https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/igno...
chrome/browser/ssl/ignore_errors_cert_verifier.cc:30: HashValue hv;
style: https://google.github.io/styleguide/cppguide.html#General_Naming_Rules

a name like "hash" or "hash_value" is probably more appropriate here.

https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/igno...
chrome/browser/ssl/ignore_errors_cert_verifier.cc:76: auto wl =
whitelist_.begin();
style: https://google.github.io/styleguide/cppguide.html#General_Naming_Rules

whitelist_begin/whitelist_end, fingerprints_begin/fingerprints_end ?

https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/igno...
chrome/browser/ssl/ignore_errors_cert_verifier.cc:94: [](CompletionCallback
callback, int result) { callback.Run(net::OK); },
So in thinking more about this, the problem is that it potentially violates the
contract between the net::Error returned and the CertStatus in the
CertVerifyResult

if (ignore_errors) {
  verify_result->Reset();
  verify_result->verified_cert = cert;
  verify_result->public_key_hashes.assign(spki_fingerprints.begin(),
spki_fingerprints.end());
  return net::OK;
}

return verifier_->Verify(params, crl_set, verify_result, callback, out_req,
net_log);

https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/igno...
File chrome/browser/ssl/ignore_errors_cert_verifier.h (right):

https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/igno...
chrome/browser/ssl/ignore_errors_cert_verifier.h:18: class NetLogWithSource;
For 16/17/18, you don't need to forward declare, since they're part of the
net::CertVerifier interface contract.

https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/igno...
File chrome/browser/ssl/ignore_errors_cert_verifier_unittest.cc (right):

https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/igno...
chrome/browser/ssl/ignore_errors_cert_verifier_unittest.cc:44:
"MtnqgdSwAIgEjse7SpxnmyKoo/RTiL9CDIWwFnz4nas=",
It'd be better if this was dynamically computed. x509_verify_results.chain.pem
is an auto-generated file, so any time it's regenerated, this would have to be
updated (if baked in). Having it dynamically computed doesn't seem to undermine
the system under test, but I'm curious your thoughts.

https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/igno...
chrome/browser/ssl/ignore_errors_cert_verifier_unittest.cc:67: CHECK(test_cert);
This is a unit test, so we shouldn't CHECK() for any of these (which brings down
the whole framework), but using the unit test macros (ASSERT_ and friends). This
does require using void returns (since ASSERT_ is just a quick return) along
with
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...

https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/igno...
chrome/browser/ssl/ignore_errors_cert_verifier_unittest.cc:81: CHECK_EQ(3U,
certs.size());
ditto re: CHECK

https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/igno...
chrome/browser/ssl/ignore_errors_cert_verifier_unittest.cc:88: CHECK_EQ(2U,
cert_chain->GetIntermediateCertificates().size());
ditto re: CHECK

https://codereview.chromium.org/2753123002/diff/80001/net/cert/mock_cert_veri...
File net/cert/mock_cert_verifier.cc (right):

https://codereview.chromium.org/2753123002/diff/80001/net/cert/mock_cert_veri...
net/cert/mock_cert_verifier.cc:69: callback.Run(result);
BUG: This isn't consistent with how Chrome's async callbacks work. You'd
actually need to PostTask to guarantee the async-ness.

In order to allow async, you also have to populate out_req so you can allow the
request to be cancelled (meaning you couldn't PostTask to callback, you'd need
to PostTask to run an internal helper that would then run the callback)

class InternalRequest : public CertVerifier::Request {
 public:
  explicit InternalRequest(const CompletionCallback& callback,
                           int result)
      : callback_(callback), result_(result), weak_factory_(this) {}

  void Start() {
    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
base::Bind(&InternalRequest::RunCallback, weak_factory_.GetWeakPtr()));
  }

  void RunCallback() {
    base::ResetAndReturn(callback_).Run(result_);
  }

 private:
  CompletionCallback callback_;
  int result_;
  base::WeakPtrFactor<InternalRequest> weak_factory_;
};


And then here, you'd do:

std::unique_ptr<InternalRequest> req(base::MakeUnique<InternalRequest>(callback,
result));
req->Start();
*out_req = req;
return ERR_IO_PENDING;



For context, the goal is to start an async task that will run the callback on
the _next_ message loop turn (very important, //net avoids re-entrant callbacks
like this would introduce), but also observe the cancellation semantics that
guarantees that |callback| won't run if |out_req| is deleted (this is
accomplished by the WeakPtrFactory, since the callback will no-op if the WeakPtr
is invalidated)

[martinkr, 2017-04-24 23:54:26]

https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/igno...
File chrome/browser/ssl/ignore_errors_cert_verifier.cc (right):

https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/igno...
chrome/browser/ssl/ignore_errors_cert_verifier.cc:30: HashValue hv;
On 2017/04/12 21:53:17, Ryan Sleevi wrote:
> style: https://google.github.io/styleguide/cppguide.html#General_Naming_Rules
> 
> a name like "hash" or "hash_value" is probably more appropriate here.

Done.

https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/igno...
chrome/browser/ssl/ignore_errors_cert_verifier.cc:76: auto wl =
whitelist_.begin();
On 2017/04/12 21:53:17, Ryan Sleevi wrote:
> style: https://google.github.io/styleguide/cppguide.html#General_Naming_Rules
> 
> whitelist_begin/whitelist_end, fingerprints_begin/fingerprints_end ?

Done.

https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/igno...
chrome/browser/ssl/ignore_errors_cert_verifier.cc:94: [](CompletionCallback
callback, int result) { callback.Run(net::OK); },
On 2017/04/12 21:53:17, Ryan Sleevi wrote:
> So in thinking more about this, the problem is that it potentially violates
the
> contract between the net::Error returned and the CertStatus in the
> CertVerifyResult
> 
> if (ignore_errors) {
>   verify_result->Reset();
>   verify_result->verified_cert = cert;
>   verify_result->public_key_hashes.assign(spki_fingerprints.begin(),
> spki_fingerprints.end());
>   return net::OK;
> }
> 
> return verifier_->Verify(params, crl_set, verify_result, callback, out_req,
> net_log);

Done.

https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/igno...
File chrome/browser/ssl/ignore_errors_cert_verifier.h (right):

https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/igno...
chrome/browser/ssl/ignore_errors_cert_verifier.h:18: class NetLogWithSource;
On 2017/04/12 21:53:17, Ryan Sleevi wrote:
> For 16/17/18, you don't need to forward declare, since they're part of the
> net::CertVerifier interface contract.

Done.

https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/igno...
File chrome/browser/ssl/ignore_errors_cert_verifier_unittest.cc (right):

https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/igno...
chrome/browser/ssl/ignore_errors_cert_verifier_unittest.cc:44:
"MtnqgdSwAIgEjse7SpxnmyKoo/RTiL9CDIWwFnz4nas=",
On 2017/04/12 21:53:17, Ryan Sleevi wrote:
> It'd be better if this was dynamically computed. x509_verify_results.chain.pem
> is an auto-generated file, so any time it's regenerated, this would have to be
> updated (if baked in). Having it dynamically computed doesn't seem to
undermine
> the system under test, but I'm curious your thoughts.

IMHO it seems a little silly, since I essentially duplicate the SUT into the
test to compute the expected value. But to be totally honest, I originally just
printed the string and copied the test, so it's not too different I guess.
¯\_(ツ)_/¯

Done.

https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/igno...
chrome/browser/ssl/ignore_errors_cert_verifier_unittest.cc:67: CHECK(test_cert);
On 2017/04/12 21:53:17, Ryan Sleevi wrote:
> This is a unit test, so we shouldn't CHECK() for any of these (which brings
down
> the whole framework), but using the unit test macros (ASSERT_ and friends).
This
> does require using void returns (since ASSERT_ is just a quick return) along
> with
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...

Done.

https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/igno...
chrome/browser/ssl/ignore_errors_cert_verifier_unittest.cc:81: CHECK_EQ(3U,
certs.size());
On 2017/04/12 21:53:17, Ryan Sleevi wrote:
> ditto re: CHECK

Done.

https://codereview.chromium.org/2753123002/diff/80001/chrome/browser/ssl/igno...
chrome/browser/ssl/ignore_errors_cert_verifier_unittest.cc:88: CHECK_EQ(2U,
cert_chain->GetIntermediateCertificates().size());
On 2017/04/12 21:53:17, Ryan Sleevi wrote:
> ditto re: CHECK

Done.

https://codereview.chromium.org/2753123002/diff/80001/net/cert/mock_cert_veri...
File net/cert/mock_cert_verifier.cc (right):

https://codereview.chromium.org/2753123002/diff/80001/net/cert/mock_cert_veri...
net/cert/mock_cert_verifier.cc:69: callback.Run(result);
On 2017/04/12 21:53:17, Ryan Sleevi wrote:
> BUG: This isn't consistent with how Chrome's async callbacks work. You'd
> actually need to PostTask to guarantee the async-ness.
> 
> In order to allow async, you also have to populate out_req so you can allow
the
> request to be cancelled (meaning you couldn't PostTask to callback, you'd need
> to PostTask to run an internal helper that would then run the callback)
> 
> class InternalRequest : public CertVerifier::Request {
>  public:
>   explicit InternalRequest(const CompletionCallback& callback,
>                            int result)
>       : callback_(callback), result_(result), weak_factory_(this) {}
> 
>   void Start() {
>     base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
> base::Bind(&InternalRequest::RunCallback, weak_factory_.GetWeakPtr()));
>   }
> 
>   void RunCallback() {
>     base::ResetAndReturn(callback_).Run(result_);
>   }
> 
>  private:
>   CompletionCallback callback_;
>   int result_;
>   base::WeakPtrFactor<InternalRequest> weak_factory_;
> };
> 
> 
> And then here, you'd do:
> 
> std::unique_ptr<InternalRequest>
req(base::MakeUnique<InternalRequest>(callback,
> result));
> req->Start();
> *out_req = req;
> return ERR_IO_PENDING;
> 
> 
> 
> For context, the goal is to start an async task that will run the callback on
> the _next_ message loop turn (very important, //net avoids re-entrant
callbacks
> like this would introduce), but also observe the cancellation semantics that
> guarantees that |callback| won't run if |out_req| is deleted (this is
> accomplished by the WeakPtrFactory, since the callback will no-op if the
WeakPtr
> is invalidated)

Ah crap, I meant to look up how to properly do this asynchronously and forgot.
:o)

Since the new code is bypassing the wrapped verifier, rather than wrapping the
user-provided callback, I think we can simply drop this and the associated
tests, do you agree?

[Ryan Sleevi, 2017-04-25 18:05:45]

Description was changed from

==========
Add --ignore-certificate-errors-spki-list switch and UMA histogram.

The switch takes a comma-separated list of base64-encoded, SHA-256
subject public key hashes. If any of the certs presented by the server
match a hash from the list, certificate errors are ignored.

Presence of the flag is counted in a UMA histogram.

This should allow us to remove the more blunt
--ignore-certificate-errors switch eventually.

R=rsleevi@chromium.org
CC=agl@chromium.org
BUG=
==========

to

==========
Add --ignore-certificate-errors-spki-list switch and UMA histogram.

The switch takes a comma-separated list of base64-encoded, SHA-256
subject public key hashes. If any of the certs presented by the server match a
hash from the list, certificate verification is skipped, in order to ignore
certificate errors.

Presence of the flag is counted in a UMA histogram.

This should allow us to remove the more blunt
--ignore-certificate-errors switch eventually.

R=rsleevi@chromium.org
CC=agl@chromium.org
BUG=
==========

[Ryan Sleevi, 2017-04-25 18:07:10]

lgtm

https://codereview.chromium.org/2753123002/diff/80001/net/cert/mock_cert_veri...
File net/cert/mock_cert_verifier.cc (right):

https://codereview.chromium.org/2753123002/diff/80001/net/cert/mock_cert_veri...
net/cert/mock_cert_verifier.cc:69: callback.Run(result);
On 2017/04/24 23:54:26, martinkr wrote:
> On 2017/04/12 21:53:17, Ryan Sleevi wrote:
> > BUG: This isn't consistent with how Chrome's async callbacks work. You'd
> > actually need to PostTask to guarantee the async-ness.
> > 
> > In order to allow async, you also have to populate out_req so you can allow
> the
> > request to be cancelled (meaning you couldn't PostTask to callback, you'd
need
> > to PostTask to run an internal helper that would then run the callback)
> > 
> > class InternalRequest : public CertVerifier::Request {
> >  public:
> >   explicit InternalRequest(const CompletionCallback& callback,
> >                            int result)
> >       : callback_(callback), result_(result), weak_factory_(this) {}
> > 
> >   void Start() {
> >     base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
> > base::Bind(&InternalRequest::RunCallback, weak_factory_.GetWeakPtr()));
> >   }
> > 
> >   void RunCallback() {
> >     base::ResetAndReturn(callback_).Run(result_);
> >   }
> > 
> >  private:
> >   CompletionCallback callback_;
> >   int result_;
> >   base::WeakPtrFactor<InternalRequest> weak_factory_;
> > };
> > 
> > 
> > And then here, you'd do:
> > 
> > std::unique_ptr<InternalRequest>
> req(base::MakeUnique<InternalRequest>(callback,
> > result));
> > req->Start();
> > *out_req = req;
> > return ERR_IO_PENDING;
> > 
> > 
> > 
> > For context, the goal is to start an async task that will run the callback
on
> > the _next_ message loop turn (very important, //net avoids re-entrant
> callbacks
> > like this would introduce), but also observe the cancellation semantics that
> > guarantees that |callback| won't run if |out_req| is deleted (this is
> > accomplished by the WeakPtrFactory, since the callback will no-op if the
> WeakPtr
> > is invalidated)
> 
> Ah crap, I meant to look up how to properly do this asynchronously and forgot.
> :o)
> 
> Since the new code is bypassing the wrapped verifier, rather than wrapping the
> user-provided callback, I think we can simply drop this and the associated
> tests, do you agree?

Yup! SG :)

https://codereview.chromium.org/2753123002/diff/100001/chrome/browser/ssl/ign...
File chrome/browser/ssl/ignore_errors_cert_verifier.cc (right):

https://codereview.chromium.org/2753123002/diff/100001/chrome/browser/ssl/ign...
chrome/browser/ssl/ignore_errors_cert_verifier.cc:102: [](const SHA256HashValue
v) { return HashValue(v); });
this should be "const SHA256HashValue& v", right?

https://codereview.chromium.org/2753123002/diff/100001/chrome/browser/ssl/ign...
File chrome/browser/ssl/ignore_errors_cert_verifier.h (right):

https://codereview.chromium.org/2753123002/diff/100001/chrome/browser/ssl/ign...
chrome/browser/ssl/ignore_errors_cert_verifier.h:43: // result is returned.
I think this needs updating with the new behaviour?

[martinkr, 2017-04-25 19:48:37]

https://codereview.chromium.org/2753123002/diff/100001/chrome/browser/ssl/ign...
File chrome/browser/ssl/ignore_errors_cert_verifier.cc (right):

https://codereview.chromium.org/2753123002/diff/100001/chrome/browser/ssl/ign...
chrome/browser/ssl/ignore_errors_cert_verifier.cc:102: [](const SHA256HashValue
v) { return HashValue(v); });
On 2017/04/25 18:07:09, Ryan Sleevi wrote:
> this should be "const SHA256HashValue& v", right?

Done.

https://codereview.chromium.org/2753123002/diff/100001/chrome/browser/ssl/ign...
File chrome/browser/ssl/ignore_errors_cert_verifier.h (right):

https://codereview.chromium.org/2753123002/diff/100001/chrome/browser/ssl/ign...
chrome/browser/ssl/ignore_errors_cert_verifier.h:43: // result is returned.
On 2017/04/25 18:07:09, Ryan Sleevi wrote:
> I think this needs updating with the new behaviour?

Done.

[Ryan Sleevi, 2017-04-25 19:48:59]

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

[Ryan Sleevi, 2017-04-25 19:49:39]

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

[Ryan Sleevi, 2017-04-25 19:49:40]

Adding a metrics reviewer :) Hopefully CL description has sufficient context for
"why" :)

[commit-bot: I haz the power, 2017-04-25 19:49:51]

Dry run: CQ is trying da patch.

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

[Ryan Sleevi, 2017-04-25 19:50:02]

Description was changed from

==========
Add --ignore-certificate-errors-spki-list switch and UMA histogram.

The switch takes a comma-separated list of base64-encoded, SHA-256
subject public key hashes. If any of the certs presented by the server match a
hash from the list, certificate verification is skipped, in order to ignore
certificate errors.

Presence of the flag is counted in a UMA histogram.

This should allow us to remove the more blunt
--ignore-certificate-errors switch eventually.

R=rsleevi@chromium.org
CC=agl@chromium.org
BUG=
==========

to

==========
Add --ignore-certificate-errors-spki-list switch and UMA histogram.

The switch takes a comma-separated list of base64-encoded, SHA-256
subject public key hashes. If any of the certs presented by the
server match a hash from the list, certificate verification is
skipped, in order to ignore certificate errors.

Presence of the flag is counted in a UMA histogram.

This should allow us to remove the more blunt
--ignore-certificate-errors switch eventually.

R=rsleevi@chromium.org
CC=agl@chromium.org
BUG=
==========

[martinkr, 2017-04-25 19:50:16]

On 2017/04/25 19:49:40, Ryan Sleevi wrote:
> Adding a metrics reviewer :) Hopefully CL description has sufficient context
for
> "why" :)

Ah you beat me to it, thanks.

[commit-bot: I haz the power, 2017-04-25 20:27:18]

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

[commit-bot: I haz the power, 2017-04-25 20:27:19]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Ilya Sherman, 2017-04-26 00:14:33]

Metrics LGTM % comments:

https://codereview.chromium.org/2753123002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2753123002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:31683: +    enum="BooleanEnabled">
This enum has roughly opposite labels to what's being recorded, IMO.  I'd
recommend (just in this file) using an enum with more specific labels, like
"Validation enabled" and "Validation disabled for some keys".

https://codereview.chromium.org/2753123002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:31687: +   
--ignore-certificate-errors-spki-list flag.
Please document when this is recorded.

[martinkr, 2017-04-26 00:57:04]

https://codereview.chromium.org/2753123002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2753123002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:31683: +    enum="BooleanEnabled">
On 2017/04/26 00:14:33, Ilya Sherman wrote:
> This enum has roughly opposite labels to what's being recorded, IMO.  I'd
> recommend (just in this file) using an enum with more specific labels, like
> "Validation enabled" and "Validation disabled for some keys".

How about adding "Present" to emphasize that this is recording flag presence and
changing the enum accordingly?

https://codereview.chromium.org/2753123002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:31687: +   
--ignore-certificate-errors-spki-list flag.
On 2017/04/26 00:14:33, Ilya Sherman wrote:
> Please document when this is recorded.

Done.

[martinkr, 2017-04-26 00:57:25]

The CQ bit was checked by martinkr@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 00:58:05]

Dry run: 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, 2017-04-26 01:46:07]

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

[commit-bot: I haz the power, 2017-04-26 01:46:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ilya Sherman, 2017-04-26 07:14:43]

(Metrics still LGTM)

[martinkr, 2017-04-26 18:47:06]

Looks like the ssl_browser_tests still fail on trybot: There seems to be an
issue around command line length on windows, any idea how to address that? And
looks like linux_chromium_chromeos is failing the added test cases, no clue why.

[martinkr, 2017-05-04 22:46:06]

The CQ bit was checked by martinkr@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 22:47:15]

Dry run: CQ is trying da patch.

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

[martinkr, 2017-05-04 22:47:58]

The CQ bit was unchecked by martinkr@google.com

[martinkr, 2017-05-05 22:07:07]

The CQ bit was checked by martinkr@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 22:08:36]

Dry run: 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, 2017-05-05 22:34:40]

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

[commit-bot: I haz the power, 2017-05-05 22:34:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[martinkr, 2017-05-05 22:42:43]

The CQ bit was checked by martinkr@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 22:43:40]

Dry run: 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, 2017-05-05 23:35:50]

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

[commit-bot: I haz the power, 2017-05-05 23:35:51]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[martinkr, 2017-05-10 00:13:49]

martinkr@google.com changed reviewers:
+ estark@chromium.org

[martinkr, 2017-05-10 00:16:02]

I added a test for --user-data-dir as suggested, although I'm not entirely sure
if this is kosher, particularly wrt test isolation.

[martinkr, 2017-05-10 00:16:12]

The CQ bit was checked by martinkr@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 00:17:20]

Dry run: CQ is trying da patch.

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

[Ilya Sherman, 2017-05-10 00:24:00]

metrics lgtm

[commit-bot: I haz the power, 2017-05-10 01:28:39]

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

[commit-bot: I haz the power, 2017-05-10 01:28:40]

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

[estark, 2017-05-11 01:07:08]

Hmm, I'm also not sure if it's okay to unset --user-data-dir in a browser test.
(Another option would be doing an io_thread_unittest instead of a browser test,
but I'm not sure that's any better.) Ryan, do you know if there's any reasonable
way to test that --ignore-cert-errors-spki-list requires --user-data-dir?

In the meantime, does the flag need to go into
chrome/browser/ui/startup/bad_flags_prompt.cc to show the "stability and
security will suffer" infobar?

[Ryan Sleevi, 2017-05-11 19:22:05]

On 2017/05/11 01:07:08, estark wrote:
> Hmm, I'm also not sure if it's okay to unset --user-data-dir in a browser
test.
> (Another option would be doing an io_thread_unittest instead of a browser
test,
> but I'm not sure that's any better.) Ryan, do you know if there's any
reasonable
> way to test that --ignore-cert-errors-spki-list requires --user-data-dir?

I'm not, and I think I share your concern that unsetting it can undermine test
hermeticism. My own risk calculus leans towards not testing this than causing
flakiness, so I'd say we can remove this aspect of the testing.

> In the meantime, does the flag need to go into
> chrome/browser/ui/startup/bad_flags_prompt.cc to show the "stability and
> security will suffer" infobar?

Ah, yes, good catch. That was the assumption (and bad on me for not catching
that it wasn't).

[martinkr, 2017-05-11 19:25:07]

On 2017/05/11 19:22:05, Ryan Sleevi wrote:
> On 2017/05/11 01:07:08, estark wrote:
> > Hmm, I'm also not sure if it's okay to unset --user-data-dir in a browser
> test.
> > (Another option would be doing an io_thread_unittest instead of a browser
> test,
> > but I'm not sure that's any better.) Ryan, do you know if there's any
> reasonable
> > way to test that --ignore-cert-errors-spki-list requires --user-data-dir?
> 
> I'm not, and I think I share your concern that unsetting it can undermine test
> hermeticism. My own risk calculus leans towards not testing this than causing
> flakiness, so I'd say we can remove this aspect of the testing.

We could test in an IOThread unittest as Emily suggested. AFAICT we would have
to extract a method just to get a testing seam, but that's probably worth it.

> 
> > In the meantime, does the flag need to go into
> > chrome/browser/ui/startup/bad_flags_prompt.cc to show the "stability and
> > security will suffer" infobar?
> 
> Ah, yes, good catch. That was the assumption (and bad on me for not catching
> that it wasn't).

Oops, yes. Adding that in…

[martinkr, 2017-05-12 01:13:02]

On 2017/05/11 19:25:07, martinkr wrote:
> On 2017/05/11 19:22:05, Ryan Sleevi wrote:
> > On 2017/05/11 01:07:08, estark wrote:
> > > Hmm, I'm also not sure if it's okay to unset --user-data-dir in a browser
> > test.
> > > (Another option would be doing an io_thread_unittest instead of a browser
> > test,
> > > but I'm not sure that's any better.) Ryan, do you know if there's any
> > reasonable
> > > way to test that --ignore-cert-errors-spki-list requires --user-data-dir?
> > 
> > I'm not, and I think I share your concern that unsetting it can undermine
test
> > hermeticism. My own risk calculus leans towards not testing this than
causing
> > flakiness, so I'd say we can remove this aspect of the testing.
> 
> We could test in an IOThread unittest as Emily suggested. AFAICT we would have
> to extract a method just to get a testing seam, but that's probably worth it.

Done (although it ended up in ignore_errors_cert_verifier_unittest, so I didn't
have to move cert-related code into iothread_unittest).

> 
> > 
> > > In the meantime, does the flag need to go into
> > > chrome/browser/ui/startup/bad_flags_prompt.cc to show the "stability and
> > > security will suffer" infobar?
> > 
> > Ah, yes, good catch. That was the assumption (and bad on me for not catching
> > that it wasn't).
> 
> Oops, yes. Adding that in…

Done.

[Ryan Sleevi, 2017-05-15 14:45:03]

lgtm

[estark, 2017-05-17 18:04:19]

lgtm with nits

https://codereview.chromium.org/2753123002/diff/220001/chrome/browser/ssl/ign...
File chrome/browser/ssl/ignore_errors_cert_verifier.h (right):

https://codereview.chromium.org/2753123002/diff/220001/chrome/browser/ssl/ign...
chrome/browser/ssl/ignore_errors_cert_verifier.h:1: // Copyright (c) 2017 The
Chromium Authors. All rights reserved.
nit: copyright header is old format, use
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
instead
(please check the other files as well)

https://codereview.chromium.org/2753123002/diff/220001/chrome/browser/ssl/ign...
File chrome/browser/ssl/ignore_errors_cert_verifier_unittest.cc (right):

https://codereview.chromium.org/2753123002/diff/220001/chrome/browser/ssl/ign...
chrome/browser/ssl/ignore_errors_cert_verifier_unittest.cc:79: // verifier_.
teeny-tiny nit: |verifier_|

https://codereview.chromium.org/2753123002/diff/220001/chrome/browser/ssl/ign...
chrome/browser/ssl/ignore_errors_cert_verifier_unittest.cc:178:
TEST_P(IgnoreCertificateErrorsSPKIListFlagTest, TestFoo) {
"TestFoo" could stand to be a tad more descriptive :P

https://codereview.chromium.org/2753123002/diff/220001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/2753123002/diff/220001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:726: // public keys from the list.
optional nit: just for consistency, include this comment above line 713

https://codereview.chromium.org/2753123002/diff/220001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:2707: // Visit a page and establish a
WebSocket connection over bad https with
Question: are you testing WebSockets because they are handled in a special way,
or to test that certificate errors on non-main-frame resources are ignored, or
both?

It might be good to have  a test that the flag works properly on subresources,
e.g. navigate to http://blah which loads an image (or script or whatever) from
https://expired.com, where the https://expired.com cert is in
--ignore-cert-errors-spki-list, and check that the image is loaded properly.

https://codereview.chromium.org/2753123002/diff/220001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:2708: //
--disable-certificate-errors-spki-list. The connection should be established
nit: s/disable/ignore? (same on line 2735)

https://codereview.chromium.org/2753123002/diff/220001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:2711:
ASSERT_TRUE(embedded_test_server()->Start());
Do you need this? Doesn't look like you use it unless I'm missing it.

[martinkr, 2017-05-18 22:34:31]

The CQ bit was checked by martinkr@google.com

[martinkr, 2017-05-18 22:34:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2753123002/#ps220001
(title: "Move test for --user-data-dir into unittest; add bad flags prompt.")

[commit-bot: I haz the power, 2017-05-18 22:36:54]

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, 2017-05-18 22:50:38]

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

[commit-bot: I haz the power, 2017-05-18 22:50:40]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[martinkr, 2017-05-18 22:55:52]

martinkr@google.com changed reviewers:
+ pkasting@chromium.org

[martinkr, 2017-05-18 22:56:46]

R: +pkasting for chrome/browser/ui/startup/bad_flags_prompt.cc

[Peter Kasting, 2017-05-19 02:03:35]

LGTM

[martinkr, 2017-05-23 18:29:52]

The CQ bit was checked by martinkr@google.com

[martinkr, 2017-05-23 18:29:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, pkasting@chromium.org, rsleevi@chromium.org,
estark@chromium.org
Link to the patchset: https://codereview.chromium.org/2753123002/#ps240001
(title: "Add --ignore-certificate-errors-spki-list switch and UMA histogram.")

[commit-bot: I haz the power, 2017-05-23 18:30:26]

CQ is trying da patch.

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

[martinkr, 2017-05-23 18:30:43]

https://codereview.chromium.org/2753123002/diff/220001/chrome/browser/ssl/ign...
File chrome/browser/ssl/ignore_errors_cert_verifier.h (right):

https://codereview.chromium.org/2753123002/diff/220001/chrome/browser/ssl/ign...
chrome/browser/ssl/ignore_errors_cert_verifier.h:1: // Copyright (c) 2017 The
Chromium Authors. All rights reserved.
On 2017/05/17 18:04:18, estark (temporarily slow) wrote:
> nit: copyright header is old format, use
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> instead
> (please check the other files as well)

Done.

https://codereview.chromium.org/2753123002/diff/220001/chrome/browser/ssl/ign...
File chrome/browser/ssl/ignore_errors_cert_verifier_unittest.cc (right):

https://codereview.chromium.org/2753123002/diff/220001/chrome/browser/ssl/ign...
chrome/browser/ssl/ignore_errors_cert_verifier_unittest.cc:79: // verifier_.
On 2017/05/17 18:04:18, estark (temporarily slow) wrote:
> teeny-tiny nit: |verifier_|

Done.

https://codereview.chromium.org/2753123002/diff/220001/chrome/browser/ssl/ign...
chrome/browser/ssl/ignore_errors_cert_verifier_unittest.cc:178:
TEST_P(IgnoreCertificateErrorsSPKIListFlagTest, TestFoo) {
On 2017/05/17 18:04:18, estark (temporarily slow) wrote:
> "TestFoo" could stand to be a tad more descriptive :P

Eh, don't you think it's pretty self explanatory? :p Done.

https://codereview.chromium.org/2753123002/diff/220001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/2753123002/diff/220001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:726: // public keys from the list.
On 2017/05/17 18:04:18, estark (temporarily slow) wrote:
> optional nit: just for consistency, include this comment above line 713

Done.

https://codereview.chromium.org/2753123002/diff/220001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:2707: // Visit a page and establish a
WebSocket connection over bad https with
On 2017/05/17 18:04:18, estark (temporarily slow) wrote:
> Question: are you testing WebSockets because they are handled in a special
way,
> or to test that certificate errors on non-main-frame resources are ignored, or
> both?
> 
> It might be good to have  a test that the flag works properly on subresources,
> e.g. navigate to http://blah which loads an image (or script or whatever) from
> https://expired.com, where the https://expired.com cert is in
> --ignore-cert-errors-spki-list, and check that the image is loaded properly.

I was actually just aping SSLUITestIgnoreCertErrors, but assumed it was to cover
both websocket specialness and non-mainframe resourcey behavior. I added the
suggested test.

https://codereview.chromium.org/2753123002/diff/220001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:2708: //
--disable-certificate-errors-spki-list. The connection should be established
On 2017/05/17 18:04:18, estark (temporarily slow) wrote:
> nit: s/disable/ignore? (same on line 2735)

Done.

https://codereview.chromium.org/2753123002/diff/220001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:2711:
ASSERT_TRUE(embedded_test_server()->Start());
On 2017/05/17 18:04:19, estark (temporarily slow) wrote:
> Do you need this? Doesn't look like you use it unless I'm missing it.

Done.

[commit-bot: I haz the power, 2017-05-23 20:10:58]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1495564192878800,
"parent_rev": "b8a0cb6ba8c4105576e6feed1fed4d132640be17", "commit_rev":
"8402d3c5bc13e018fa75eba650ed881755e0223b"}

[commit-bot: I haz the power, 2017-05-23 20:11:23]

Description was changed from

==========
Add --ignore-certificate-errors-spki-list switch and UMA histogram.

The switch takes a comma-separated list of base64-encoded, SHA-256
subject public key hashes. If any of the certs presented by the
server match a hash from the list, certificate verification is
skipped, in order to ignore certificate errors.

Presence of the flag is counted in a UMA histogram.

This should allow us to remove the more blunt
--ignore-certificate-errors switch eventually.

R=rsleevi@chromium.org
CC=agl@chromium.org
BUG=
==========

to

==========
Add --ignore-certificate-errors-spki-list switch and UMA histogram.

The switch takes a comma-separated list of base64-encoded, SHA-256
subject public key hashes. If any of the certs presented by the
server match a hash from the list, certificate verification is
skipped, in order to ignore certificate errors.

Presence of the flag is counted in a UMA histogram.

This should allow us to remove the more blunt
--ignore-certificate-errors switch eventually.

R=rsleevi@chromium.org
CC=agl@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2753123002
Cr-Commit-Position: refs/heads/master@{#474021}
Committed:
https://chromium.googlesource.com/chromium/src/+/8402d3c5bc13e018fa75eba650ed...
==========

[commit-bot: I haz the power, 2017-05-23 20:11:25]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/8402d3c5bc13e018fa75eba650ed...