Windows local machine certificates #52
Conversation
src/windows.rs
Outdated
| @@ -24,5 +24,13 @@ pub fn load_native_certs() -> Result<Vec<Certificate>, Error> { | |||
| } | |||
| } | |||
|
|
|||
| if let Ok(current_machine_store) = schannel::cert_store::CertStore::open_local_machine("ROOT") { | |||
There was a problem hiding this comment.
Why are we not propagating any error from the CertStore::open_local_machine() call?
It would also be nice to avoid duplicating the core loop. Perhaps we can construct a Vec for CertStore values and have an outer loop iterating over it?
|
(If you rebase on current main, CI should behave better now that #50 is merged.) |
TimLuq
force-pushed
the
fix-22-add-windows-local-machine
branch
from
ec2992c
to
1ec5b35
Compare
TimLuq
force-pushed
the
fix-22-add-windows-local-machine
branch
from
1ec5b35
to
2d0d3e0
Compare
|
@djc thanks for being super quick on this one. I redid the whole thing. unfortunately there were some problems with it and the issue 22 specifically referenced group policies which I hadn't explicitly accounted for. The new one unfortunately had to be much lower level. One of the questions you asked on the previous implementation will shine a light on some of the problems with it so I'll answer it though the code is no longer in effect.
This was for backwards compatibility. If the call would've failed it would behave exactly the same way as before the change. However it would not only seldom fail, but almost always, since schannel opened the local machine certificate store read-write which wouldn't be allowed for many users. The new implementation takes a similar approach by silently skipping failed stores if at least one succeeds (though they shouldn't fail as often since it opens the stores as readonly). As a bonus this should make a lot less allocations - if it was the only thing I wouldn't have said 150 extra lines would have been worth it. But I'll take the win if the opportunity arise. |
|
Hmm, I would recommend you contribute these changes to the schannel crate. I'm sorry, but I don't feel comfortable reviewing this much unsafe code. |
|
I echo the sentiment that we probably don't have the review capacity/expertise to look at this diff. I'm going to close this PR with the suggestion you pursue the change with the schannel crate. |
I've been encountering problems where a Windows user has not had a root certificate in the user store but it exists in the local machine store.
The manual workaround for this is for each user having this issue to use the root certificate in some other way which activates the certificate for that specific user. (E.g. by navigating a browser to a domain with the same CA.) It would be preferrable for this to just work out of the box, though.
Fixes #22.