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

OSSL_STORE: Windows: use system certificates storage as default store #24218

Closed
wants to merge 1 commit into from

Conversation

yjh-styx
Copy link

@yjh-styx yjh-styx commented Apr 20, 2024

don't change X509_get_default_cert_dir - it used as default directory also (by_dir.c:91)

crypto/x509/by_store.c Outdated Show resolved Hide resolved
@t8m
Copy link
Member

t8m commented Apr 22, 2024

Please do not add merge commits. Instead if there are conflicts, please rebase and force-push.

@hlandau
Copy link
Member

hlandau commented Apr 29, 2024

I don't think this is adequate. See #21190 for the history. This was already done and had to be reverted.

@t8m t8m added resolved: wont fix The issue has been confirmed but won't be fixed triaged: feature The issue/pr requests/adds a feature labels Apr 29, 2024
@openssl-machine
Copy link
Collaborator

This PR is waiting for the creator to make requested changes but it has not been updated for 30 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section).

@yjh-styx yjh-styx requested a review from shahsb May 30, 2024 10:14
@nhorman
Copy link
Contributor

nhorman commented May 30, 2024

This PR is waiting for you to make requested changes @yjh-styx . I will close this as inactive if there is no activity in another 30 days.

@yjh-styx
Copy link
Author

Could you clarify please what changes have been requested from my side?

@nhorman
Copy link
Contributor

nhorman commented May 30, 2024

@yjh-styx see comment here

@yjh-styx
Copy link
Author

Sorry, but I don't understand exactly what changes are required of me.
The link you provide points to a discussion about the reasons for "not entering" a similar (as intended) PR in version 3.2.
If you wanted to say that this PR also requires some changes in the documentation, then I won't be of any help here (my English is not enough for this). If there were any other changes in mind, please clarify which ones specifically.

@t8m
Copy link
Member

t8m commented May 31, 2024

We basically dispute the possibility to make the winstore a default store on Windows until the problems discussed here: #24170 (comment) are resolved.

@yjh-styx
Copy link
Author

If a link to this discussion had been given as a reason for refusing this PR, I would have understood. But I don't understand what changes are required from me based on it.

Before the advent of the winstore mechanism, almost everyone using openssl to work with certificates in Windows wrote cycles for fetching certificates from regisytry and filling ossl-store with a simple filter like:
(pCtx->dwCertEncodingType & PKCS_7_ASN_ENCODING) == PKCS_7_ASN_ENCODING
and it worked quietly on a bunch of machines for years.
Now (in version 3) your team has a winstore mechanism and I made a patch
using it (without asking the question "how and why you implemented it").

The discussion you link to may be the basis for demands for changes to
winstore, but as it follows:
"This PR is waiting for you to make requested changes"
?https://github.com/openssl/openssl/pull/24218#issuecomment-2139402884
What changes exactly are expected from me?

@t8m
Copy link
Member

t8m commented Jun 3, 2024

I am closing the PR as I do not expect you will be implementing the missing bits in winstore.

@t8m t8m closed this Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolved: wont fix The issue has been confirmed but won't be fixed triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants