Remove FileSystemWatcher dependency from Unix X509Store#7655
Remove FileSystemWatcher dependency from Unix X509Store#7655bartonjs merged 2 commits intodotnet:masterfrom
Conversation
|
(@iamjasonp, you apparently just independently discovered the new store permissions problem, so... FYI, here's the fix PR) |
|
FYI: to deal with cases where we only want tests to run sometimes, we've been using ConditionalFact. See: dotnet/wcf#978, dotnet/buildtools#619 Consider using [ConditionalFact] on the tests you know will modify the machine if you're concerned about modifying machine state |
|
@iamjasonp [ConditionalFact] is "in some detectable condition, do not run this test". The condition I want for non-ephemeral tests is "this isn't being called from build.cmd/build.sh", aka [OuterLoop]. |
bartonjs
force-pushed
the
unix_x509store_nofsw
branch
from
5cee898 to
15c4a82
Compare
|
Test Outerloop Ubuntu14.04 Debug please |
|
@bartonjs, have you done any measurements to see how this impacts https performance with HttpClient when we're unable to use OpenSSL's cert verification, e.g. when the server cert callback is used? |
|
@stephentoub No, I haven't done any perf assessments. One call to Certificates per object is "less memory, and faster" (by a list and list copy). Multiple calls is "less memory, and filesystem v RAM slower". Since CurlHandler doesn't open any X509Store objects itself (it uses X509Chain), it has one call per X509Store object, so it should be "less memory, and faster". |
Maybe I'm just confused about the change? If we can't use the OpenSSL verification, we need to open and enumerate the root store, right? Which means whereas previously we would just access a collection in memory, now we need to enumerate the file system? So now we're enumerating the file system on every https connection, no? |
Both root stores (user and machine), but yes.
X509Chain opens a new X509Store instance for each call to Build, so it was reading the filesystem into a List, registering a FSW, then copying the List out, then throwing it all away. Now it just reads the filesystem into an X509Certificate2Collection (aka List), without the other stuff. The machine Root store is different, that one does get held as a static collection, so it's one filesystem read per process.
Yes, but we also were reading the filesystem on every https connection before this change. |
If the FSW was always getting thrown away, what was its purpose? I thought the purpose of it was to avoid needing to enumerate the file system for the machine store each time... was that not happening?
Sure, for the user store, but now we're also always doing it for the machine store, no? Or are you saying the FSW mechanism was completely broken and we were always enumerating both the user and machine stores? |
|
LM\Root and LM\CA use CollectionBackedStoreProvider, which didn't change here. The CU* stores used DirectoryBackedStoreProvider, which is changing. The cache+FSW allowed for a long-lived instance to do multiple reads of the Certificates property without having to go back to the filesystem, and that worked (to whatever extent of reliability FSW works). No part of that state was shared across instances representing the same store. So any X509Store in a using statement was paying costs with no dividends. |
|
Ah. Ok, I see now. Thanks for the explanation. |
You're welcome. Thanks for caring enough to make the inquiries. |
bartonjs
force-pushed
the
unix_x509store_nofsw
branch
from
15c4a82 to
a775727
Compare
|
Test Innerloop OSX Release Build and Test please |
|
(Previous run failed due to the machine going offline: http://dotnet-ci.cloudapp.net/job/dotnet_corefx/job/osx_release_prtest/3740/) |
|
I didn't dig into the code but the dependency removal looks good to me. Thanks for cleaning this up @bartonjs |
| @@ -84,11 +80,6 @@ internal DirectoryBasedStoreProvider(string storeName, OpenFlags openFlags) | |||
|
|
|||
| public void Dispose() | |||
There was a problem hiding this comment.
Does IStorePal still need to be IDisposable? I've not looked to see if other implementations have code in it.
There was a problem hiding this comment.
The Windows version is holding native resources, yeah.
|
LGTM |
Users who create their first X509Store on build 23910 or later will get an exception that the directory has the wrong permissions. No test existed at the time, and manual testing failed to identify the distinction between "continued use" and "initial creation". This changes the permissions check to ensure that the user permissions are correct, and ignores the group and other permissions. The files are explicitly tested for rw?--?--?, and set to rw------- on mismatch, so there is no major concern with the directory being overly readable. It also adjusts the exception message to match the adjusted requirement, and adds a test. This test will still suffer from the x509stores directory already existing on established machines, since otherwise that runs the risk of breaking the user's my and root stores if test cleanup goes awry.
With this change the Certificates collection will just read from the filesystem on every call. Since multiple calls are believed to be infrequent, this just removes some memory and complexity trying to make the second call faster. A large number of X509Store tests were also added to verify not just this change's impact (namely, independant objects of the same store name should see each other's changes immediately both before and after the code change) but many of the machine-state impact altogether. When directories get created (first write), cleaned (never), files reused (when appropriate), etc.
bartonjs
force-pushed
the
unix_x509store_nofsw
branch
from
a775727 to
7d3bcf5
Compare
|
@mmitche hudson.remoting.ChannelClosedException: channel is already closed (http://dotnet-ci.cloudapp.net/job/dotnet_corefx/job/osx_release_prtest/3784/console) |
|
Since this passed other times, and passed on the Linux configurations, and the mac queue is still long, I'm just going to go ahead and merge. |
Remove FileSystemWatcher dependency from Unix X509Store Commit migrated from dotnet/corefx@9c0a7a9
7 participants
The original impetus for change was to remove the FileSystemWatcher dependency for the Unix implementation of X509Store. Since changes were being made, some tests were finally added to OuterLoop (since it involves making state changes on the filesystem). This uncovered an issue that creating new stores fails due to the directory permissions check being overly restrictive (in a way that we explicitly concluded it wasn't doing at the time of the change).
So, with the two commits:
So, this reacts to #6797, fixes #6618, and fixes #3374.
cc: @stephentoub @weshaggard @atsushikan