Add X509CertificateBytes which uses CRYPTO_BUFFER instead of macOS-native certificate types.

Add X509CertificateBytes which uses CRYPTO_BUFFER instead of macOS-native certificate types.

(Other platforms will come in later CLs.)

BUG=671420
Review-Url: https://codereview.chromium.org/2746103003
Cr-Commit-Position: refs/heads/master@{#463507}
Committed: https://chromium.googlesource.com/chromium/src/+/4cede8d39db10321b053c0d9776cf6b23f290310

[mattm, 2017-03-15 01:35:11]

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

[commit-bot: I haz the power, 2017-03-15 01:35:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-03-15 01:45:59]

Description was changed from

==========
WIP

BUG=
==========

to

==========
Add X509CertificateBytes which uses CRYPTO_BUFFER instead of OS-native
certificate types (only used on Mac in this CL.)

BUG=671420
==========

[commit-bot: I haz the power, 2017-03-15 01:47:58]

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

[commit-bot: I haz the power, 2017-03-15 01:47:59]

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...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[mattm, 2017-03-15 19:53:59]

Description was changed from

==========
Add X509CertificateBytes which uses CRYPTO_BUFFER instead of OS-native
certificate types (only used on Mac in this CL.)

BUG=671420
==========

to

==========
Add X509CertificateBytes which uses CRYPTO_BUFFER instead of macOS-native
certificate types.

(Other platforms will come it later CLs.)

BUG=671420
==========

[mattm, 2017-03-15 21:51:27]

Patchset #7 (id:120001) has been deleted

[mattm, 2017-03-15 21:51:32]

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

[commit-bot: I haz the power, 2017-03-15 21:52: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-03-15 22:07:39]

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

[commit-bot: I haz the power, 2017-03-15 22:07:40]

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

[mattm, 2017-03-15 22:28:56]

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

[commit-bot: I haz the power, 2017-03-15 22:29:32]

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-03-15 22:40:47]

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

[commit-bot: I haz the power, 2017-03-15 22:40:48]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mattm, 2017-03-18 00:26:51]

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

[commit-bot: I haz the power, 2017-03-18 00:27:57]

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-03-18 00:40:21]

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

[commit-bot: I haz the power, 2017-03-18 00:40:22]

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

[mattm, 2017-03-18 01:03:40]

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

[commit-bot: I haz the power, 2017-03-18 01:04:27]

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-03-18 02:46:31]

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

[commit-bot: I haz the power, 2017-03-18 02:46:32]

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_...)

[mattm, 2017-03-28 03:14:26]

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

[commit-bot: I haz the power, 2017-03-28 03:15:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-03-28 03:15:18]

Description was changed from

==========
Add X509CertificateBytes which uses CRYPTO_BUFFER instead of macOS-native
certificate types.

(Other platforms will come it later CLs.)

BUG=671420
==========

to

==========
Add X509CertificateBytes which uses CRYPTO_BUFFER instead of macOS-native
certificate types.

(Other platforms will come in later CLs.)

BUG=671420
==========

[commit-bot: I haz the power, 2017-03-28 03:43:43]

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

[commit-bot: I haz the power, 2017-03-28 03:43:44]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mattm, 2017-03-29 01:07:55]

Patchset #14 (id:280001) has been deleted

[mattm, 2017-03-29 01:08:38]

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

[commit-bot: I haz the power, 2017-03-29 01:09:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-03-29 01:19:10]

mattm@chromium.org changed reviewers:
+ eroman@chromium.org

[mattm, 2017-03-29 01:19:11]

https://codereview.chromium.org/2746103003/diff/300001/build/config/crypto.gni
File build/config/crypto.gni (right):

https://codereview.chromium.org/2746103003/diff/300001/build/config/crypto.gn...
build/config/crypto.gni:29: use_byte_certs = is_mac
My thought is to land the CL with this set to false, and then do the flag flip
in a separate CL. That way if anything needs to be reverted it can just revert
the flag flip CL. Then sometime later the old x509_certificate_mac and related
code could be deleted.

The other option would just be to rip out x509_certificate_mac and other old
code right away, and just deal with a big revert if it happens to be necessary.

Any opinions?

[commit-bot: I haz the power, 2017-03-29 03:11:10]

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

[commit-bot: I haz the power, 2017-03-29 03:11:11]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)

[eroman, 2017-03-29 23:06:55]

https://codereview.chromium.org/2746103003/diff/300001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/2746103003/diff/300001/build/config/BUILD.gn#...
build/config/BUILD.gn:75: if (use_byte_certs) {
Not sure if the OWNERS will be open to this, given the warning comments above. I
guess the alternative could be to express USE_BYTE_CERTS in terms of
platform-defines.

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
File net/cert/x509_certificate.cc (right):

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate.cc:192: #endif  // !defined(USE_NSS_CERTS)
nit: Update comments?

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
File net/cert/x509_certificate_bytes.cc (right):

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate_bytes.cc:44: // true, or return false if the if was
not a valid DER Sequence.
if the if --> if the TLV

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate_bytes.cc:73: !NormalizeName(issuer_value,
&normalized_cert_issuer))
Curly brace for consistency with above? Or conversely remove curlies above.

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate_bytes.cc:92: return
name_attribute.ValueAsStringUnsafe(out);
Should this case explicitly check value_tag as well?

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate_bytes.cc:220: if
(!ParseTbsCertificate(tbs_certificate_tlv, {}, &tbs, nullptr)) {
same comment for single-line ifs

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate_bytes.cc:267: if (IsCertIssuerMatch(cert_handle_,
normalized_issuers))
[optional] I am not a fan of this function, but I think I understand why you
wrote it this way. I think it may be more readable as a
GetNormalizedCertIssuer(), but then you would need to duplicate the find().

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate_bytes.cc:297: if
(!ParseCertificate(der::Input(CRYPTO_BUFFER_data(cert_handle),
You could replace this with asn1::ExtractSPKIFromDERCert() I believe.

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate_bytes.cc:330: *size_bits = EVP_PKEY_bits(pkey.get());
EVP_PKEY_bits() returns an int -- do you think it can ever be negative?
The paranoid crazy person in me suggests a saturated cast to 0 if it does.

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate_bytes.cc:424: SHA256_Final(sha256.data, &sha256_ctx);
Should we be checking the return values from these BoringSSL functions? Or are
the only failures for allocation?

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate_bytes.cc:434: if
(!ParseCertificate(der::Input(CRYPTO_BUFFER_data(cert_handle),
Having seen this pattern a few times, I wonder if we should offer an all-in one
helper. Maybe a version of ParseTbsCertificate() that internally calls
ParseCertificate, and the out parameters can optionally be passed as nullptr for
uninteresting fields? Probably best left to a cleanup, but worth thinking about.

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate_bytes.cc:456: if (normalized_subject !=
normalized_issuer)
Interestingly, I just glanced at the _mac.cc version and it doesn't compare
normalized names (unless if those fields are already normalized internally)?

This version here looks good though.

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate_bytes.cc:466: return
VerifySignedData(*signature_algorithm, tbs_certificate_tlv,
I wonder is this even necessary? I see some implementations do this. I am not
sure, but does openssl actually do this internally? (I seem to recall it doesn't
check signatures on self-signed).

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate_bytes.cc:479: return CreateOSCertHandleFromBytes(data,
length);
paranoid crazy person: is length always non-negative? (as this does an implicit
sign cast).

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_util_ios.h
File net/cert/x509_util_ios.h (right):

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_util_ios...
net/cert/x509_util_ios.h:20: base::ScopedCFTypeRef<SecCertificateRef> NET_EXPORT
should net_export be before the return type?

https://codereview.chromium.org/2746103003/diff/300001/net/ssl/client_cert_st...
File net/ssl/client_cert_store_mac.cc (right):

https://codereview.chromium.org/2746103003/diff/300001/net/ssl/client_cert_st...
net/ssl/client_cert_store_mac.cc:294: if (!SupportsSSLClientAuth(cert_handle))
Can you explain this line? Was this a check that would being done inernally by
CreateFromHandle() and previously manifesting as a null |cert| ?

[mattm, 2017-03-30 04:32:14]

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

[commit-bot: I haz the power, 2017-03-30 04:32:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-03-30 04:38:10]

https://codereview.chromium.org/2746103003/diff/300001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/2746103003/diff/300001/build/config/BUILD.gn#...
build/config/BUILD.gn:75: if (use_byte_certs) {
On 2017/03/29 23:06:54, eroman wrote:
> Not sure if the OWNERS will be open to this, given the warning comments above.
I
> guess the alternative could be to express USE_BYTE_CERTS in terms of
> platform-defines.

Switched to a buildflag defined in net/BUILD.gn.

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
File net/cert/x509_certificate.cc (right):

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate.cc:192: #endif  // !defined(USE_NSS_CERTS)
On 2017/03/29 23:06:54, eroman wrote:
> nit: Update comments?

Done.

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
File net/cert/x509_certificate_bytes.cc (right):

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate_bytes.cc:44: // true, or return false if the if was
not a valid DER Sequence.
On 2017/03/29 23:06:55, eroman wrote:
> if the if --> if the TLV

Done.

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate_bytes.cc:73: !NormalizeName(issuer_value,
&normalized_cert_issuer))
On 2017/03/29 23:06:54, eroman wrote:
> Curly brace for consistency with above? Or conversely remove curlies above.

oops, I've switched to what is (I think) the common style of: no braces on
single line condition + single line body, and braces on multi-line condition +
single line body.

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate_bytes.cc:92: return
name_attribute.ValueAsStringUnsafe(out);
On 2017/03/29 23:06:54, eroman wrote:
> Should this case explicitly check value_tag as well?

I was gonna say ValueAsStringUnsafe checks value_tag itself, but I guess it's
kinda fragile to depend on that.

Maybe a better approach is just to move this into X509NameAttribute, especially
since some places are using ValueAsStringUnsafe unsafely :/

I'll make a separate CL to do that.

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate_bytes.cc:220: if
(!ParseTbsCertificate(tbs_certificate_tlv, {}, &tbs, nullptr)) {
On 2017/03/29 23:06:55, eroman wrote:
> same comment for single-line ifs

Done.

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate_bytes.cc:267: if (IsCertIssuerMatch(cert_handle_,
normalized_issuers))
On 2017/03/29 23:06:55, eroman wrote:
> [optional] I am not a fan of this function, but I think I understand why you
> wrote it this way. I think it may be more readable as a
> GetNormalizedCertIssuer(), but then you would need to duplicate the find().

Hmm, okay. done.

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate_bytes.cc:297: if
(!ParseCertificate(der::Input(CRYPTO_BUFFER_data(cert_handle),
On 2017/03/29 23:06:54, eroman wrote:
> You could replace this with asn1::ExtractSPKIFromDERCert() I believe.

Done.

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate_bytes.cc:330: *size_bits = EVP_PKEY_bits(pkey.get());
On 2017/03/29 23:06:54, eroman wrote:
> EVP_PKEY_bits() returns an int -- do you think it can ever be negative?
> The paranoid crazy person in me suggests a saturated cast to 0 if it does.

From some code spelunking it seem the impls return 0 on errors, but from a
safety perspective I suppose that a saturated_cast couldn't hurt.

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate_bytes.cc:424: SHA256_Final(sha256.data, &sha256_ctx);
On 2017/03/29 23:06:54, eroman wrote:
> Should we be checking the return values from these BoringSSL functions? Or are
> the only failures for allocation?

It seems like no existing code checks the results of these. The comments for
Init and Update say they always return 1 and Final only returns 0 on programmer
error. shrug.

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate_bytes.cc:434: if
(!ParseCertificate(der::Input(CRYPTO_BUFFER_data(cert_handle),
On 2017/03/29 23:06:55, eroman wrote:
> Having seen this pattern a few times, I wonder if we should offer an all-in
one
> helper. Maybe a version of ParseTbsCertificate() that internally calls
> ParseCertificate, and the out parameters can optionally be passed as nullptr
for
> uninteresting fields? Probably best left to a cleanup, but worth thinking
about.

Acknowledged.

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate_bytes.cc:456: if (normalized_subject !=
normalized_issuer)
On 2017/03/29 23:06:54, eroman wrote:
> Interestingly, I just glanced at the _mac.cc version and it doesn't compare
> normalized names (unless if those fields are already normalized internally)?
> 
> This version here looks good though.

Apparently:
CSSMOID_X509V1IssuerName and CSSMOID_X509V1SubjectName = normalized.
CSSMOID_X509V1IssuerNameStd and CSSMOID_X509V1SubjectNameStd = raw.

So yeah, the existing code was doing non-normalized comparison (same for the
IsIssuedBy method). (I believe ios and openssl impl of x509certificate also do
un-normalized comparisons.)

NSS impl of X509Certificate at does seem to do normalized comparisons (though I
think NSS does a simpler form of normalization). 

Not sure about win.

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate_bytes.cc:466: return
VerifySignedData(*signature_algorithm, tbs_certificate_tlv,
On 2017/03/29 23:06:54, eroman wrote:
> I wonder is this even necessary? I see some implementations do this. I am not
> sure, but does openssl actually do this internally? (I seem to recall it
doesn't
> check signatures on self-signed).

Yeah, based on how it's used (and the unittest as well) this method does need to
check the signature, not just the names. I'm not sure I understood the question
about openssl doing this internally.

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_certific...
net/cert/x509_certificate_bytes.cc:479: return CreateOSCertHandleFromBytes(data,
length);
On 2017/03/29 23:06:54, eroman wrote:
> paranoid crazy person: is length always non-negative? (as this does an
implicit
> sign cast).

Looks like the pickle ReadData code will return false if the length was negative
(or too long), so I think that should be safe.

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_util_ios.h
File net/cert/x509_util_ios.h (right):

https://codereview.chromium.org/2746103003/diff/300001/net/cert/x509_util_ios...
net/cert/x509_util_ios.h:20: base::ScopedCFTypeRef<SecCertificateRef> NET_EXPORT
On 2017/03/29 23:06:55, eroman wrote:
> should net_export be before the return type?

hmm, I guess so.  Done.

https://codereview.chromium.org/2746103003/diff/300001/net/ssl/client_cert_st...
File net/ssl/client_cert_store_mac.cc (right):

https://codereview.chromium.org/2746103003/diff/300001/net/ssl/client_cert_st...
net/ssl/client_cert_store_mac.cc:294: if (!SupportsSSLClientAuth(cert_handle))
On 2017/03/29 23:06:55, eroman wrote:
> Can you explain this line? Was this a check that would being done inernally by
> CreateFromHandle() and previously manifesting as a null |cert| ?

This is moved from GetClientCertsImpl (the previous block in the diff) to
ClientCertStoreMac::GetClientCerts, because it requires a SecCertificateRef.
GetClientCertsImpl now only has a X509Certificate, so it would need to create a
new SecCertificate if we left the SupportsSSLClientAuth check in there (or would
need to re-write SupportsSSLClientAuth to not depend on OS calls).

Moving it here should be safe, since GetClientCertsImpl is only called by this
function or by unittests, and the unittests, at least currently, only pass in
client certs anyway and are just testing the issuer filtering.

[commit-bot: I haz the power, 2017-03-30 06:11:11]

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

[commit-bot: I haz the power, 2017-03-30 06:11:12]

Dry run: This issue passed the CQ dry run.

[eroman, 2017-04-03 19:52:03]

lgtm

https://codereview.chromium.org/2746103003/diff/340001/net/cert/x509_util_mac.h
File net/cert/x509_util_mac.h (right):

https://codereview.chromium.org/2746103003/diff/340001/net/cert/x509_util_mac...
net/cert/x509_util_mac.h:25: // Test that a given |cert_handle| is actually a
valid X.509 certificate, and
nit: Test -> Tests

https://codereview.chromium.org/2746103003/diff/340001/net/cert/x509_util_mac...
net/cert/x509_util_mac.h:26: // return true if it is.
nit: return -> returns

[mattm, 2017-04-03 23:35:03]

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

[commit-bot: I haz the power, 2017-04-03 23:35:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-04-03 23:36:16]

+thakis for chrome/browser/ui/

https://codereview.chromium.org/2746103003/diff/340001/net/cert/x509_util_mac.h
File net/cert/x509_util_mac.h (right):

https://codereview.chromium.org/2746103003/diff/340001/net/cert/x509_util_mac...
net/cert/x509_util_mac.h:25: // Test that a given |cert_handle| is actually a
valid X.509 certificate, and
On 2017/04/03 19:52:03, eroman wrote:
> nit: Test -> Tests

Done.

https://codereview.chromium.org/2746103003/diff/340001/net/cert/x509_util_mac...
net/cert/x509_util_mac.h:26: // return true if it is.
On 2017/04/03 19:52:03, eroman wrote:
> nit: return -> returns

Done.

[commit-bot: I haz the power, 2017-04-04 01:14:51]

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

[commit-bot: I haz the power, 2017-04-04 01:14:52]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-04-04 18:39:04]

mattm@chromium.org changed reviewers:
+ thakis@chromium.org

[mattm, 2017-04-04 18:39:05]

+thakis for chrome/browser/ui/

(forgot to actually add to reviewer list last message)

[mattm, 2017-04-04 23:34:21]

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

[commit-bot: I haz the power, 2017-04-04 23:35:06]

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-05 00:26:41]

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

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

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

[mattm, 2017-04-07 04:49:08]

thakis: ping

[Nico, 2017-04-07 13:08:17]

C/b/u lgtm, sorry for missing this. One optional bikeshed below.

https://codereview.chromium.org/2746103003/diff/380001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm (right):

https://codereview.chromium.org/2746103003/diff/380001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm:212:
net::x509_util::CreateSecCertificateFromX509Certificate(
That's a very long function name. Two "x509", one "util", two "Certificate".

[mattm, 2017-04-07 22:18:51]

https://codereview.chromium.org/2746103003/diff/380001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm (right):

https://codereview.chromium.org/2746103003/diff/380001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm:212:
net::x509_util::CreateSecCertificateFromX509Certificate(
On 2017/04/07 13:08:17, Nico wrote:
> That's a very long function name. Two "x509", one "util", two "Certificate".

Yeah, I guess that is. On the plus side it's pretty unambiguous.
(X509Certificate is a class name, so the X509 does have reason to be repeated).
I guess it could just be named CreateSecCertificate and overload on the argument
type, though the style guide seems to advocate for this style rather than
overloading.

[mattm, 2017-04-07 22:38:44]

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

[commit-bot: I haz the power, 2017-04-07 22:39:55]

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-07 23:51:59]

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

[commit-bot: I haz the power, 2017-04-07 23:52:00]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-04-11 01:03:44]

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

[commit-bot: I haz the power, 2017-04-11 01:04:45]

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-11 02:21:12]

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

[commit-bot: I haz the power, 2017-04-11 02:21:13]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-04-11 02:37:38]

The CQ bit was checked by mattm@chromium.org

[mattm, 2017-04-11 02:37:38]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2746103003/#ps420001
(title: "rebase")

[commit-bot: I haz the power, 2017-04-11 02:38:30]

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-11 02:55:06]

CQ is committing da patch.

Bot data: {"patchset_id": 420001, "attempt_start_ts": 1491878258065220,
"parent_rev": "4cd1f352db04e0325fa1929034af54359f80ebff", "commit_rev":
"4cede8d39db10321b053c0d9776cf6b23f290310"}

[commit-bot: I haz the power, 2017-04-11 02:55:58]

Description was changed from

==========
Add X509CertificateBytes which uses CRYPTO_BUFFER instead of macOS-native
certificate types.

(Other platforms will come in later CLs.)

BUG=671420
==========

to

==========
Add X509CertificateBytes which uses CRYPTO_BUFFER instead of macOS-native
certificate types.

(Other platforms will come in later CLs.)

BUG=671420

Review-Url: https://codereview.chromium.org/2746103003
Cr-Commit-Position: refs/heads/master@{#463507}
Committed:
https://chromium.googlesource.com/chromium/src/+/4cede8d39db10321b053c0d9776c...
==========

[commit-bot: I haz the power, 2017-04-11 02:55:59]

Committed patchset #20 (id:420001) as
https://chromium.googlesource.com/chromium/src/+/4cede8d39db10321b053c0d9776c...