Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

open_local_machine & open_current_user cannot be specified as read-only #61

Open
crusty-dave opened this issue Dec 17, 2019 · 7 comments

Comments

@crusty-dave
Copy link

dwFlags should include CERT_STORE_READONLY_FLAG.

If the intent for those API is to allow write access, then there should be new APIs that allow at least a bool, if not additional flags.

Unfortunately, I couldn't easily override the fn because CertStore constructor is private.

@steffengy
Copy link
Owner

We can't change the open_local_machine without breaking backwards-compatibility.
Also CertStore itself provides add_cert, which wouldn't make sense on a CertStore with CERT_STORE_READONLY_FLAG.

We could deprecate them and provide something like std::fs::OpenOptions?
For curiositys sake, do you have a specific usecase for having to pass CERT_STORE_READONLY_FLAG ?

@crusty-dave
Copy link
Author

Yes, to avoid elevating privileges on Windows. Providing direct access to the API might be a good option to allow more of the parameters to be specified. Or if you made the CertStore constructor public, then it would be easier to override for special cases.

Thanks,
-Dave

@steffengy
Copy link
Owner

steffengy commented Dec 18, 2019

Providing direct access to the API also is suboptimal, if it should ever change.
We can't make the constructor public, since that would prevent further winapi upgrades
(since winapi would be a part of the public API surface).
Similarily to #58 we can provide a way to get a CertStore from a *mut c_void.

@crusty-dave
Copy link
Author

crusty-dave commented Dec 18, 2019

If you made the following:

pub fn open(LocalMachine | CurrentUser: SystemStore, which: &str, read_only: bool) -> io::Result<CertStore>

That would provide the flexibility that I seek.

@steffengy
Copy link
Owner

steffengy commented Dec 18, 2019

Until the next person needs a different flag.
That's why a builder pattern works better for these cases, since it's extendable.
Maybe something like CertStore.open().local_machine().read_only(true).open("which")? taking some inspiration from std::fs::OpenOptions.

@crusty-dave
Copy link
Author

crusty-dave commented Dec 18, 2019

That works for me.

Sorry, I said CurrentSystem, I meant LocalMachine obviously. I updated my prior comment.

@steffengy
Copy link
Owner

I'll think about in the next few days and probably would like to

  1. Add unsafe methods that allow conversion from/to *mut c_void, as considered previously in Provide access to the underlying Windows handles #58.
  2. Add a more configurable builderlike way to open CertStores, as described above.

@sfackler pinging, in case you have any reservations/preferences on these matters

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants