Convert Windows to use X509CertificateBytes.

Convert Windows to use X509CertificateBytes.

BUG=671420
Review-Url: https://codereview.chromium.org/2913253003
Cr-Commit-Position: refs/heads/master@{#480686}
Committed: https://chromium.googlesource.com/chromium/src/+/2fe429a914dccf9bc3b8901c8a0d93f445f3a3b6

[mattm, 2017-06-01 03:09:02]

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

[commit-bot: I haz the power, 2017-06-01 03:09:16]

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-06-01 05:27:49]

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

[commit-bot: I haz the power, 2017-06-01 05:27:49]

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

[mattm, 2017-06-02 23:21:49]

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

[commit-bot: I haz the power, 2017-06-02 23:22:20]

Dry run: CQ is trying da patch.

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

[mattm, 2017-06-02 23:23:51]

Description was changed from

==========
*WIP* Convert Windows to use X509CertificateBytes.

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

to

==========
Convert Windows to use X509CertificateBytes.

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

[commit-bot: I haz the power, 2017-06-03 01:17:38]

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

[commit-bot: I haz the power, 2017-06-03 01:17:39]

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

[mattm, 2017-06-03 01:23:57]

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

[commit-bot: I haz the power, 2017-06-03 01:24:28]

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-06-03 04:18:39]

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

[commit-bot: I haz the power, 2017-06-03 04:18:40]

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

[mattm, 2017-06-05 21:34:50]

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

[commit-bot: I haz the power, 2017-06-05 21:35: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-06-06 02:28:43]

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

[commit-bot: I haz the power, 2017-06-06 02:28:44]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-06-08 21:51:06]

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

[commit-bot: I haz the power, 2017-06-08 21:51:38]

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-06-08 22:14:35]

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

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

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

[mattm, 2017-06-08 22:36:22]

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

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

Dry run: CQ is trying da patch.

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

[mattm, 2017-06-08 22:47:35]

Patchset #6 (id:100001) has been deleted

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

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

[commit-bot: I haz the power, 2017-06-09 00:35:24]

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-06-09 00:41:15]

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

[commit-bot: I haz the power, 2017-06-09 00:41:52]

Dry run: CQ is trying da patch.

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

[mattm, 2017-06-09 00:41:59]

Patchset #6 (id:120001) has been deleted

[commit-bot: I haz the power, 2017-06-09 02:39:08]

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

[commit-bot: I haz the power, 2017-06-09 02:39:09]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-06-13 19:41:46]

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

[davidben, 2017-06-15 23:55:18]

lgtm

https://codereview.chromium.org/2913253003/diff/140001/chrome/browser/ui/cryp...
File chrome/browser/ui/cryptuiapi_shim.h (right):

https://codereview.chromium.org/2913253003/diff/140001/chrome/browser/ui/cryp...
chrome/browser/ui/cryptuiapi_shim.h:11: // which include cryptuiapi should
instead include this header.
Blegh. I wonder how crazy it'd be to add a #define to BoringSSL which suppresses
everything relating to crypto/x509 from the headers, now that we've gotten
pretty far in unshipping. I guess we'd need to be fully there.

I was going to say I can break the ssl.h => x509.h #include link, but the
problem is that'd rely on forward decls and the forward decls still collide.

https://codereview.chromium.org/2913253003/diff/140001/net/cert/x509_util_win.cc
File net/cert/x509_util_win.cc (right):

https://codereview.chromium.org/2913253003/diff/140001/net/cert/x509_util_win...
net/cert/x509_util_win.cc:21: crypto::CAPIDestroyerWithFlags<HCERTSTORE,
CertCloseStore, 0>>
Nit: using

https://codereview.chromium.org/2913253003/diff/140001/net/cert/x509_util_win...
net/cert/x509_util_win.cc:76: reinterpret_cast<const
BYTE*>(CRYPTO_BUFFER_data(cert->os_cert_handle())),
Optional: It's probably safe to assume this cast isn't needed?
CRYPTO_BUFFER_data returns a const uint8_t *, and BoringSSL requires that
uint8_t and unsigned char are the same. Microsoft documents that BYTE is
unsigned char.

https://codereview.chromium.org/2913253003/diff/140001/net/cert/x509_util_win...
net/cert/x509_util_win.cc:109: for (size_t i = 0; i <
intermediate_ca_certs.size(); ++i) {
Nit:
for (PCCERT_CONTEXT intermediate : intermediate_ca_certs) {
  ...
}

https://codereview.chromium.org/2913253003/diff/140001/net/cert/x509_util_win...
net/cert/x509_util_win.cc:117: // the store will be freed when |primary_cert| is
freed.
This comment confuses me a little. It reads like you're leaving something
dangling, but it's in a scoper. Perhaps:

Note: |primary_cert| retains a reference to |store|, so the store will actually
be freed when |primary_cert| is freed.

https://codereview.chromium.org/2913253003/diff/140001/net/cert/x509_util_win...
net/cert/x509_util_win.cc:126: size_t sha256_size = sizeof(sha256.data);
Nit: Any reason to stash this into a variable? You could probably just write
sizeof() at line 134.

https://codereview.chromium.org/2913253003/diff/140001/net/cert/x509_util_win.h
File net/cert/x509_util_win.h (right):

https://codereview.chromium.org/2913253003/diff/140001/net/cert/x509_util_win...
net/cert/x509_util_win.h:30: ScopedPCCERT_CONTEXT;
Nit: using.

https://codereview.chromium.org/2913253003/diff/140001/net/data/parse_certifi...
File net/data/parse_certificate_unittest/v3_certificate_template.txt (right):

https://codereview.chromium.org/2913253003/diff/140001/net/data/parse_certifi...
net/data/parse_certificate_unittest/v3_certificate_template.txt:6: # are
replaced for generating various test data.
Wow, I had no idea you all had built a templater like this. That's hilarious.
:-)

https://codereview.chromium.org/2913253003/diff/140001/net/ssl/client_cert_st...
File net/ssl/client_cert_store_win.cc (right):

https://codereview.chromium.org/2913253003/diff/140001/net/ssl/client_cert_st...
net/ssl/client_cert_store_win.cc:199: intermediates);
[Ah. I see why it's easier to tie this change to USE_BYTES_CERTS.]

[mattm, 2017-06-16 21:39:32]

https://codereview.chromium.org/2913253003/diff/140001/net/cert/x509_util_win.cc
File net/cert/x509_util_win.cc (right):

https://codereview.chromium.org/2913253003/diff/140001/net/cert/x509_util_win...
net/cert/x509_util_win.cc:21: crypto::CAPIDestroyerWithFlags<HCERTSTORE,
CertCloseStore, 0>>
On 2017/06/15 23:55:18, davidben wrote:
> Nit: using

Done.

https://codereview.chromium.org/2913253003/diff/140001/net/cert/x509_util_win...
net/cert/x509_util_win.cc:76: reinterpret_cast<const
BYTE*>(CRYPTO_BUFFER_data(cert->os_cert_handle())),
On 2017/06/15 23:55:18, davidben wrote:
> Optional: It's probably safe to assume this cast isn't needed?
> CRYPTO_BUFFER_data returns a const uint8_t *, and BoringSSL requires that
> uint8_t and unsigned char are the same. Microsoft documents that BYTE is
> unsigned char.

Done.

https://codereview.chromium.org/2913253003/diff/140001/net/cert/x509_util_win...
net/cert/x509_util_win.cc:109: for (size_t i = 0; i <
intermediate_ca_certs.size(); ++i) {
On 2017/06/15 23:55:18, davidben wrote:
> Nit:
> for (PCCERT_CONTEXT intermediate : intermediate_ca_certs) {
>   ...
> }

Done.

https://codereview.chromium.org/2913253003/diff/140001/net/cert/x509_util_win...
net/cert/x509_util_win.cc:117: // the store will be freed when |primary_cert| is
freed.
On 2017/06/15 23:55:18, davidben wrote:
> This comment confuses me a little. It reads like you're leaving something
> dangling, but it's in a scoper. Perhaps:
> 
> Note: |primary_cert| retains a reference to |store|, so the store will
actually
> be freed when |primary_cert| is freed.

Done.

https://codereview.chromium.org/2913253003/diff/140001/net/cert/x509_util_win...
net/cert/x509_util_win.cc:126: size_t sha256_size = sizeof(sha256.data);
On 2017/06/15 23:55:18, davidben wrote:
> Nit: Any reason to stash this into a variable? You could probably just write
> sizeof() at line 134.

Done.

https://codereview.chromium.org/2913253003/diff/140001/net/cert/x509_util_win.h
File net/cert/x509_util_win.h (right):

https://codereview.chromium.org/2913253003/diff/140001/net/cert/x509_util_win...
net/cert/x509_util_win.h:30: ScopedPCCERT_CONTEXT;
On 2017/06/15 23:55:18, davidben wrote:
> Nit: using.

Done.

https://codereview.chromium.org/2913253003/diff/140001/net/data/parse_certifi...
File net/data/parse_certificate_unittest/v3_certificate_template.txt (right):

https://codereview.chromium.org/2913253003/diff/140001/net/data/parse_certifi...
net/data/parse_certificate_unittest/v3_certificate_template.txt:6: # are
replaced for generating various test data.
On 2017/06/15 23:55:18, davidben wrote:
> Wow, I had no idea you all had built a templater like this. That's hilarious.
> :-)

Yep, Eric came up with this. Ingenious :)

[mattm, 2017-06-16 21:39:45]

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

[commit-bot: I haz the power, 2017-06-16 21:39:55]

Dry run: CQ is trying da patch.

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

[mattm, 2017-06-16 21:41:07]

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

[mattm, 2017-06-16 21:41:08]

+pkasting for chrome/browser/ui/ owners review
(note: this is a followup CL to https://codereview.chromium.org/2898573002/)

[Peter Kasting, 2017-06-16 23:53:47]

LGTM

https://codereview.chromium.org/2913253003/diff/180001/chrome/browser/ui/cryp...
File chrome/browser/ui/cryptuiapi_shim.h (right):

https://codereview.chromium.org/2913253003/diff/180001/chrome/browser/ui/cryp...
chrome/browser/ui/cryptuiapi_shim.h:11: // which include cryptuiapi should
instead include this header.
Nit: which include -> which want to include?

[commit-bot: I haz the power, 2017-06-17 00:03:36]

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

[commit-bot: I haz the power, 2017-06-17 00:03:37]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-06-17 03:24:12]

Patchset #9 (id:200001) has been deleted

[mattm, 2017-06-17 03:32:39]

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

[commit-bot: I haz the power, 2017-06-17 03:32:50]

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-06-17 06:07:53]

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

[commit-bot: I haz the power, 2017-06-17 06:07:54]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-06-19 22:19:39]

https://codereview.chromium.org/2913253003/diff/180001/chrome/browser/ui/cryp...
File chrome/browser/ui/cryptuiapi_shim.h (right):

https://codereview.chromium.org/2913253003/diff/180001/chrome/browser/ui/cryp...
chrome/browser/ui/cryptuiapi_shim.h:11: // which include cryptuiapi should
instead include this header.
On 2017/06/16 23:53:47, Peter Kasting wrote:
> Nit: which include -> which want to include?

Done.

[mattm, 2017-06-19 22:22:32]

The CQ bit was checked by mattm@chromium.org

[mattm, 2017-06-19 22:22:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2913253003/#ps260001
(title: "rebase")

[commit-bot: I haz the power, 2017-06-19 22:23:17]

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-06-20 01:53:47]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1497910952444750,
"parent_rev": "abd9ba0ebfca0ce7a37c14593d55ebee56c4a55c", "commit_rev":
"2fe429a914dccf9bc3b8901c8a0d93f445f3a3b6"}

[commit-bot: I haz the power, 2017-06-20 01:53:57]

Description was changed from

==========
Convert Windows to use X509CertificateBytes.

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

to

==========
Convert Windows to use X509CertificateBytes.

BUG=671420

Review-Url: https://codereview.chromium.org/2913253003
Cr-Commit-Position: refs/heads/master@{#480686}
Committed:
https://chromium.googlesource.com/chromium/src/+/2fe429a914dccf9bc3b8901c8a0d...
==========

[commit-bot: I haz the power, 2017-06-20 01:53:58]

Committed patchset #11 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/2fe429a914dccf9bc3b8901c8a0d...