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

Why we don't use certifi root certificates by default? #1491

Open
mangin opened this issue Apr 23, 2024 · 7 comments
Open

Why we don't use certifi root certificates by default? #1491

mangin opened this issue Apr 23, 2024 · 7 comments
Labels
enhancement M-T: A feature request for new functionality question M-T: User needs support to use the project
Milestone

Comments

@mangin
Copy link

mangin commented Apr 23, 2024

Hey there,

Today my colleauge step into the problem with ceritificate that time to time appears with other people who use your client.
You can read about this problem more here:
https://stackoverflow.com/questions/59808346/python-3-slack-client-ssl-sslcertverificationerror

As I see the problem that people have some problem with root certificates on their machine.
Python always had a problem with root certificatates and to solve this problem people started to use the project certifi.

For example you can find usage of this project in the kinda popular library requests
https://github.com/psf/requests/blob/main/src/requests/utils.py#L63
https://github.com/psf/requests/blob/main/src/requests/adapters.py#L294

Do you have any reason to don't use library certifi as requests library uses?

I think it would simplify of using your library everywhere.

@WilliamBergamin WilliamBergamin added enhancement M-T: A feature request for new functionality question M-T: User needs support to use the project and removed untriaged labels Apr 23, 2024
@WilliamBergamin
Copy link
Contributor

Hi @mangin thanks for writing in 💯

This is awesome feedback, @seratch let me know if I'm wrong, I think the reason this project does not use the certifi library is because it provides a way for users to set ssl as the certifi library without adding any dependencies to the project

From my testing the following works well

import os
import ssl

from slack_bolt import App
from slack_sdk import WebClient

import certifi

ssl_context = ssl.create_default_context(cafile=certifi.where())

app = App(client=WebClient(token=os.environ.get("SLACK_BOT_TOKEN"), ssl=ssl_context))

We could default the ssl field to include the certifi library but this would add a dependency to the project without really providing anything a user can't already configure

@mangin
Copy link
Author

mangin commented Apr 23, 2024

Thank you for your answer!

1. About adding a new dependency.
I think that this dependency it's not so big problem for your project.
It would solve so many issues of your customers and would decrease amount of issues that people create when use your library.

I saw a problem with Python and certificates many many years ago and it looks like that it would never solve and maybe it's a good approach just include this dependency to your project as requests library does (and i really believe many many others. If you would take a look on your optional dependency aiohttp it includes certifi too as a dependency). So maybe it's a good idea to just add it because with probability 100% your clients would install it without you. It's kinda a default dependency that goes with any transport library. And i think too few people now use urlib directly. The most of them prefer to use something a little bit better requests or aiohttp.

2. About your example
Yes, now i create WebClient like you show and it's a really hard to figure out that it what you need to do to solve a problem on the systems that doesn't include slack's root certificate.

It works well and from my point of view it would be fantastic if the your SDK creates SSL context with using certifi if the user didn't provide alternative ssl context.

3. Compromise approach
you can add certifi to optional dependency and start to use it as optional dependency by importing with try catch

try:
 from ceritifi import where
     DEFAULT_SSL_LOCATION = where()
except Exception:
     DEDAULT_SSL_LOCATION = NONE
     
 def some_where_in_the_place_wher_you_create_ssl_context():
    if ssl is None:
       if DEDAULT_SSL_LOCATION:
          ssl = ssl.create_default_context(cafile=DEFAULT_SSL_LOCATION)
       else:
         ssl = ssl.create_default_context()

@seratch seratch added this to the 3.x milestone Apr 23, 2024
@seratch
Copy link
Member

seratch commented Apr 23, 2024

Thank you so much for the inputs. We don't plan to add the change immediately, but if we observe many 👍 added to your suggestion, we may consider some changes in the future. Thanks again for taking the time to share this!

@mangin
Copy link
Author

mangin commented Apr 24, 2024

if you would like i can create a PR with changes that reflects my suggestion. It shouldn't be a big work.

And i would looking forward for 👍 from community )

@eddyg
Copy link
Contributor

eddyg commented Apr 24, 2024

I'm not a fan of adding certifi as a requirement.

It's not that certifi is used by the requests library, it is extracted from the requests library.

See, for example, this blog post. If a particular system needs it, then adding it (as shown by @WilliamBergamin above) is trivial.

@mangin
Copy link
Author

mangin commented Apr 24, 2024

@eddyg

history

Thank you for your addition info about the story of certifi. I didn't know that it used to take a part of it.

post

I read the post and this post just try to push one idea why cerifi is not good. And sometimes it goes so far. For example when they say that everyone can create a CA and try to push the reader to the idea that Mozila would add this bad CA immediately to certifi. Moreover some problems already are not reproduced that the author mentioned.

how to fix

Yes, @WilliamBergamin copied the answer from the stackoverflow that i mentioned in my first comment.
So it works and it's not hard to set up.

The problem that I don't think that all users of python knows how to set up it correctly. And how to go from problem with stacktrace CERTIFICATE_VERIFY_FAILED to the solution.

Adding or not adding.

As i mentioned before ALL you users are using requests or aiohttp. It's a really hard to find anyone who use urllib from the box. It means that all other requests are verified by cerifi's root cert.

And from my point of view that your library makes the things differently adds more problem than solves.

@eddyg
Copy link
Contributor

eddyg commented Apr 24, 2024

I see the certifi package as a bit of a "kludge" for systems that are old/unmaintained and aren't receiving updates to the system-supplied CA bundle, or are using versions of Python that don't properly use the system-supplied CA bundle.

Adding certifi as a dependency and overriding the SSL context with certifi.where() would override the system-supplied CA bundle. This has the potential to easily become out-of-date if the certifi package is not regularly updated by the user. In contrast, the system-supplied CA bundle gets updated via vendor-supplied auto-update mechanisms.

Just my 2¢!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality question M-T: User needs support to use the project
Projects
None yet
Development

No branches or pull requests

4 participants