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

[FEATURE] Optionally disable default certifi certificates #877

Open
nokados opened this issue Dec 23, 2024 · 4 comments
Open

[FEATURE] Optionally disable default certifi certificates #877

nokados opened this issue Dec 23, 2024 · 4 comments
Labels
enhancement New feature or request

Comments

@nokados
Copy link

nokados commented Dec 23, 2024

Is your feature request related to a problem?

I dislike certifi because it makes managing self-signed certificates difficult. I would prefer to use the system certificate store instead.

What solution would you like?

I believe the most straightforward solution would be to use ca_certs=None if it is explicitly set. Here's what I mean: change this

            # Convert all sentinel values to their actual default
            # values if not using an SSLContext.
            if verify_certs is VERIFY_CERTS_DEFAULT:
                verify_certs = True
            if ssl_show_warn is SSL_SHOW_WARN_DEFAULT:
                ssl_show_warn = True

            ca_certs = self.default_ca_certs() if ca_certs is None else ca_certs

to this

```python
            # Convert all sentinel values to their actual default
            # values if not using an SSLContext.
            if verify_certs is VERIFY_CERTS_DEFAULT:
                verify_certs = True
            if ssl_show_warn is SSL_SHOW_WARN_DEFAULT:
                ssl_show_warn = True

            ca_certs = self.default_ca_certs() if ca_certs is CA_CERTS_DEFAULT else ca_certs

What alternatives have you considered?

I can't think of any other viable solutions.

@nokados nokados added enhancement New feature or request untriaged Need triage labels Dec 23, 2024
@dblock
Copy link
Member

dblock commented Dec 23, 2024

Maybe we could add ca_certs as an input parameter, introduce ca_certs = AUTO and default to it?

Either way appreciate a PR!

@dblock dblock removed the untriaged Need triage label Dec 23, 2024
@nokados
Copy link
Author

nokados commented Dec 24, 2024

I think I can make some time this week

@nokados
Copy link
Author

nokados commented Dec 24, 2024

Maybe we could add ca_certs as an input parameter, introduce ca_certs = AUTO and default to it?

Yes, your option is probably better, as it is more explicit, but it deviates from the pattern of automatically determining default values used for verify_certs and ssl_show_warn. On the other hand, these are boolean variables, while we are changing str | Path | certifi | system_default. Alternatively, we could define constants like 'system' and 'certifi' to make it even clearer which paths will be checked. Please select the most preferable option. Also, are there any other suggestions for the Pull Request?

@dblock
Copy link
Member

dblock commented Dec 24, 2024

Thanks @nokados. My biggest concern is backwards compatibility, we cannot change the way existing parameters and their values work in the current interface without a major version bump, and a lesser concern future adaptability so we can keep changing it. We can always add of course. Give all these options a try and keep that in mind?

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

No branches or pull requests

2 participants