Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(313)

Issue 4646001: Implement LoadTemporaryRoot for Windows (Closed)

Created:
10 years, 1 month ago by Ryan Sleevi
Modified:
9 years, 7 months ago
CC:
chromium-reviews, pam+watch_chromium.org, cbentzel+watch_chromium.org, darin-cc_chromium.org, Paweł Hajdan Jr., brettw-cc_chromium.org, Timur Iskhodzhanov, Alexander Potapenko, stuartmorgan+watch_chromium.org
Visibility:
Public.

Description

Add support for temporarily trusting a certificate for the duration of unit tests on Windows, rather than requiring the machine to be pre-configured out-of-band. Given the lack of a Microsoft-provided high-level API to supply application-level trusts to the verification routines, this implements a workaround that intercepts attempts to open the trusted system root store and injects the test certificates directly. This allows the unit tests to work without requiring that the Test CA be added to the machine's Trusted Certificates store. While doing so, clean up the interface to adding/removing trusted test certificates, so as to support more than one trusted certificate if necessary. BUG=8470 TEST=To follow Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=69351

Patch Set 1 #

Patch Set 2 : Fix DCHECK_GT() usage on Win #

Patch Set 3 : Slight Mac optimization/forgot a conditional #

Total comments: 2

Patch Set 4 : Ensure temporary root certs are searched (and preferred) when building chains on Win #

Total comments: 2

Patch Set 5 : Feedback from phajdan.jr and bulach #

Total comments: 36

Patch Set 6 : Rebase to trunk #

Patch Set 7 : bulach and wtc feedback #

Total comments: 25

Patch Set 8 : more feedback #

Patch Set 9 : Rebase to trunk with OpenSSL fixes from joth #

Total comments: 1

Patch Set 10 : New Win method & unittests #

Total comments: 29

Patch Set 11 : Feedback #

Patch Set 12 : Rebase #

Patch Set 13 : Split tests & new certs into new CL #

Patch Set 14 : Fix comment typo, remove using operator<<, and update OpenSSL to trunk #

Patch Set 15 : Rebase to trunk #

Patch Set 16 : Widen suppresions #

Unified diffs Side-by-side diffs Delta from patch set Stats (+794 lines, -332 lines) Patch
M base/openssl_util.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +3 lines, -1 line 0 comments Download
D net/base/cert_test_util.h View 1 2 3 4 5 6 7 8 9 2 chunks +13 lines, -6 lines 0 comments Download
M net/base/cert_test_util.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +20 lines, -136 lines 0 comments Download
A net/base/test_root_certs.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +103 lines, -0 lines 0 comments Download
A net/base/test_root_certs.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +59 lines, -0 lines 0 comments Download
A net/base/test_root_certs_mac.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +135 lines, -0 lines 0 comments Download
A net/base/test_root_certs_nss.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +119 lines, -0 lines 0 comments Download
A net/base/test_root_certs_openssl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +51 lines, -0 lines 0 comments Download
A net/base/test_root_certs_win.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +206 lines, -0 lines 0 comments Download
M net/base/x509_certificate.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +7 lines, -0 lines 0 comments Download
M net/base/x509_certificate_mac.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +3 lines, -67 lines 0 comments Download
M net/base/x509_certificate_openssl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +16 lines, -7 lines 0 comments Download
M net/base/x509_certificate_unittest.cc View 1 2 3 4 5 6 7 8 9 6 chunks +5 lines, -34 lines 0 comments Download
M net/base/x509_certificate_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 6 chunks +32 lines, -5 lines 0 comments Download
M net/net.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 5 chunks +10 lines, -1 line 0 comments Download
M net/test/test_server.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +0 lines, -12 lines 0 comments Download
M net/test/test_server.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 5 chunks +5 lines, -31 lines 0 comments Download
M net/test/test_server_posix.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -4 lines 0 comments Download
M net/test/test_server_win.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -28 lines 0 comments Download
M tools/valgrind/memcheck/suppressions.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +7 lines, -0 lines 0 comments Download

Messages

Total messages: 28 (0 generated)
Ryan Sleevi
@wtc: Here's the CL I mentioned earlier this week. Everything is green here on my ...
10 years, 1 month ago (2010-11-07 11:08:48 UTC) #1
Ryan Sleevi
I should add, as a "Known Issue", that because the temporary roots singleton is used ...
10 years, 1 month ago (2010-11-07 11:18:21 UTC) #2
Paweł Hajdan Jr.
Drive-by with minor test comment. No need to wait for another review by me. By ...
10 years, 1 month ago (2010-11-08 10:42:31 UTC) #3
joth
just a comment on this one question http://codereview.chromium.org/4646001/diff/4001/5012 File net/net.gyp (right): http://codereview.chromium.org/4646001/diff/4001/5012#newcode762 net/net.gyp:762: ], On ...
10 years, 1 month ago (2010-11-08 15:22:25 UTC) #4
bulach
thanks ryan! I'll look in more detail shortly, just a quick note about the gyp: ...
10 years, 1 month ago (2010-11-08 18:16:39 UTC) #5
bulach
wtc surely needs to review the actual impl, I know nothing about mac :) however, ...
10 years, 1 month ago (2010-11-09 16:21:09 UTC) #6
wtc
Somehow I missed this CL. Sorry! I will review it today.
10 years, 1 month ago (2010-11-16 20:26:29 UTC) #7
wtc
rsleevi: thanks a lot for writing this CL. Please see my comments below. Overall comments: ...
10 years, 1 month ago (2010-11-16 23:24:01 UTC) #8
Ryan Sleevi
Thanks wtc and bulach, I've uploaded a new patchset that should have all the changes ...
10 years, 1 month ago (2010-11-17 09:37:43 UTC) #9
joth
Quick heads up now that I landed the openssl_utils refactor. Cheers! http://codereview.chromium.org/4646001/diff/19001/net/base/temporary_root_certs_openssl.cc File net/base/temporary_root_certs_openssl.cc (right): ...
10 years, 1 month ago (2010-11-17 10:37:01 UTC) #10
joth
correction to my previous comment. http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs_openssl.cc File net/base/test_root_certs_openssl.cc (right): http://codereview.chromium.org/4646001/diff/56001/net/base/test_root_certs_openssl.cc#newcode26 net/base/test_root_certs_openssl.cc:26: } while ((error_code = ...
10 years, 1 month ago (2010-11-17 16:29:20 UTC) #11
bulach
thanks, ryan! a fresh look at it gave me new insight, sorry I should've thought ...
10 years, 1 month ago (2010-11-17 17:17:30 UTC) #12
wtc
rsleevi: I haven't reviewed Patch Set 7 yet. Let me first send you some comments ...
10 years, 1 month ago (2010-11-17 19:44:39 UTC) #13
wtc
LGTM. http://codereview.chromium.org/4646001/diff/56001/net/base/openssl_util.h File net/base/openssl_util.h (right): http://codereview.chromium.org/4646001/diff/56001/net/base/openssl_util.h#newcode51 net/base/openssl_util.h:51: // any modifications that TestRootCerts may have done, ...
10 years, 1 month ago (2010-11-18 02:12:47 UTC) #14
Ryan Sleevi
wtc, bulach: Thanks for the feedback. I've integrated what you asked for in patchset 8, ...
10 years, 1 month ago (2010-11-18 05:31:57 UTC) #15
joth
You patch on my patch LGTM, thanks.
10 years, 1 month ago (2010-11-18 10:27:19 UTC) #16
bulach
thanks ryan! the code looks much simpler now, but I still have one suggestion to ...
10 years, 1 month ago (2010-11-18 12:42:11 UTC) #17
wtc
rsleevi: I don't have time to review new Patch Sets. So I'll be counting on ...
10 years, 1 month ago (2010-11-19 01:29:44 UTC) #18
Ryan Sleevi
Bulach: I've added HasInstance() and renamed as requested where appropriate. Wan-Teh: Based on your concerns ...
10 years, 1 month ago (2010-11-21 22:15:30 UTC) #19
joth
It LGTM but needs wtc's input on the win32 api shimming design. http://codereview.chromium.org/4646001/diff/82002/net/base/test_root_certs.cc File net/base/test_root_certs.cc ...
10 years, 1 month ago (2010-11-22 13:05:40 UTC) #20
bulach
LGTM it'd be nice to double check the win implementation, and some suggestions to further ...
10 years, 1 month ago (2010-11-22 14:36:50 UTC) #21
wtc
LGTM. I'm glad you discovered a good solution for Windows. We should not check in ...
10 years, 1 month ago (2010-11-23 00:30:11 UTC) #22
Ryan Sleevi
wtc: Just to make sure I understand your feedback - you're ok with the current ...
10 years ago (2010-12-03 03:28:06 UTC) #23
joth
Thanks for the detailed follow up. My comments are now niceties, please don't let another ...
10 years ago (2010-12-03 10:40:59 UTC) #24
bulach
thanks ryan! clarified my idea below, but I'll leave your good judgement to decide whether ...
10 years ago (2010-12-03 10:58:26 UTC) #25
wtc
On 2010/12/03 03:28:06, Ryan Sleevi wrote: > wtc: Just to make sure I understand your ...
10 years ago (2010-12-03 19:20:57 UTC) #26
wtc
rsleevi: I had a typo in my previous comment. I meant: I have a slight ...
10 years ago (2010-12-03 19:22:16 UTC) #27
Ryan Sleevi
10 years ago (2010-12-06 08:29:22 UTC) #28
Thanks all for the reviews, and thanks joth&bulach for the clarification. I'll
look to address those concerns in a separate CL, to keep this one sane.

For those interested, I've split off the unit tests and new certificates into a
new CL, http://codereview.chromium.org/5535006/ , to make it easier to review.
I'm holding off committing this CL until the other is LGTMed, just to ensure
that both changes land close enough together to tickle out any (hidden) bugs in
this implementation, even though both are passing the try bots fine.

Powered by Google App Engine
This is Rietveld 408576698