Chromium Code Reviews
Help | Chromium Project | Sign in
(8)

Issue 4646001: Implement LoadTemporaryRoot for Windows (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years ago by Ryan Sleevi (out til 12-8)
Modified:
5 years, 6 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
Trybot results:
Commit queue not available (can’t edit this change).

Messages

Total messages: 28 (0 generated)
Ryan Sleevi (out til 12-8)
@wtc: Here's the CL I mentioned earlier this week. Everything is green here on my ...
6 years ago (2010-11-07 11:08:48 UTC) #1
Ryan Sleevi (out til 12-8)
I should add, as a "Known Issue", that because the temporary roots singleton is used ...
6 years 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 ...
6 years 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 ...
6 years 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: ...
6 years ago (2010-11-08 18:16:39 UTC) #5
bulach
wtc surely needs to review the actual impl, I know nothing about mac :) however, ...
6 years ago (2010-11-09 16:21:09 UTC) #6
wtc
Somehow I missed this CL. Sorry! I will review it today.
6 years 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: ...
6 years ago (2010-11-16 23:24:01 UTC) #8
Ryan Sleevi (out til 12-8)
Thanks wtc and bulach, I've uploaded a new patchset that should have all the changes ...
6 years 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): ...
6 years 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 = ...
6 years 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 ...
6 years 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 ...
6 years 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, ...
6 years ago (2010-11-18 02:12:47 UTC) #14
Ryan Sleevi (out til 12-8)
wtc, bulach: Thanks for the feedback. I've integrated what you asked for in patchset 8, ...
6 years ago (2010-11-18 05:31:57 UTC) #15
joth
You patch on my patch LGTM, thanks.
6 years 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 ...
6 years 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 ...
6 years ago (2010-11-19 01:29:44 UTC) #18
Ryan Sleevi (out til 12-8)
Bulach: I've added HasInstance() and renamed as requested where appropriate. Wan-Teh: Based on your concerns ...
6 years 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 ...
6 years 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 ...
6 years 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 ...
6 years ago (2010-11-23 00:30:11 UTC) #22
Ryan Sleevi (out til 12-8)
wtc: Just to make sure I understand your feedback - you're ok with the current ...
6 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 ...
6 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 ...
6 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 ...
6 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 ...
6 years ago (2010-12-03 19:22:16 UTC) #27
Ryan Sleevi (out til 12-8)
5 years, 12 months 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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f8e48bd