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

Issue 4646001: Implement LoadTemporaryRoot for Windows (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 5 months ago by Ryan Sleevi
Modified:
2 years, 11 months ago
CC:
chromium-reviews_chromium.org, 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) Lint 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 ? errors 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 ? errors 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 ? errors 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 7 errors 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 1 errors 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 1 errors 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 1 errors 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 2 errors 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 2 errors 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 0 errors 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 ? errors 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 ? errors 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 ? errors 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 0 errors 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 ? errors 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 ? errors 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 1 errors 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 ? errors 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 ? errors 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 ? errors Download
Trybot results:
Commit:

Messages

Total messages: 28
Ryan Sleevi
@wtc: Here's the CL I mentioned earlier this week. Everything is green here on my ...
3 years, 5 months ago #1
Ryan Sleevi
I should add, as a "Known Issue", that because the temporary roots singleton is used ...
3 years, 5 months ago #2
Paweł Hajdan Jr.
Drive-by with minor test comment. No need to wait for another review by me. By ...
3 years, 5 months ago #3
joth (inactive)
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 ...
3 years, 5 months ago #4
bulach
thanks ryan! I'll look in more detail shortly, just a quick note about the gyp: ...
3 years, 5 months ago #5
bulach
wtc surely needs to review the actual impl, I know nothing about mac :) however, ...
3 years, 5 months ago #6
wtc
Somehow I missed this CL. Sorry! I will review it today.
3 years, 5 months ago #7
wtc
rsleevi: thanks a lot for writing this CL. Please see my comments below. Overall comments: ...
3 years, 5 months ago #8
Ryan Sleevi
Thanks wtc and bulach, I've uploaded a new patchset that should have all the changes ...
3 years, 5 months ago #9
joth (inactive)
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): ...
3 years, 5 months ago #10
joth (inactive)
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 = ...
3 years, 5 months ago #11
bulach
thanks, ryan! a fresh look at it gave me new insight, sorry I should've thought ...
3 years, 5 months ago #12
wtc
rsleevi: I haven't reviewed Patch Set 7 yet. Let me first send you some comments ...
3 years, 5 months ago #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, ...
3 years, 5 months ago #14
Ryan Sleevi
wtc, bulach: Thanks for the feedback. I've integrated what you asked for in patchset 8, ...
3 years, 5 months ago #15
joth (inactive)
You patch on my patch LGTM, thanks.
3 years, 5 months ago #16
bulach
thanks ryan! the code looks much simpler now, but I still have one suggestion to ...
3 years, 5 months ago #17
wtc
rsleevi: I don't have time to review new Patch Sets. So I'll be counting on ...
3 years, 5 months ago #18
Ryan Sleevi
Bulach: I've added HasInstance() and renamed as requested where appropriate. Wan-Teh: Based on your concerns ...
3 years, 5 months ago #19
joth (inactive)
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 ...
3 years, 5 months ago #20
bulach
LGTM it'd be nice to double check the win implementation, and some suggestions to further ...
3 years, 5 months ago #21
wtc
LGTM. I'm glad you discovered a good solution for Windows. We should not check in ...
3 years, 5 months ago #22
Ryan Sleevi
wtc: Just to make sure I understand your feedback - you're ok with the current ...
3 years, 4 months ago #23
joth (inactive)
Thanks for the detailed follow up. My comments are now niceties, please don't let another ...
3 years, 4 months ago #24
bulach
thanks ryan! clarified my idea below, but I'll leave your good judgement to decide whether ...
3 years, 4 months ago #25
wtc
On 2010/12/03 03:28:06, Ryan Sleevi wrote: > wtc: Just to make sure I understand your ...
3 years, 4 months ago #26
wtc
rsleevi: I had a typo in my previous comment. I meant: I have a slight ...
3 years, 4 months ago #27
Ryan Sleevi
3 years, 4 months ago #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 1280:2d3e6564b7b6