-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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] Add SSL Certificate Context And Setting For Async/Sync HTTP Requests #6976
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few concerns with this solution.
- We are modifying environment variables from within the application and the logic to so in
get_certificates
andrestore_certs
is complex to follow. - We are introduction a state inside
Env()
. I assume that's why we have some lines withEnv()
to refresh the state (?). This class should be stateless and readonly for consistency and predictability. - The implementation requires us to constantly get and restore certificates, which is not obvious when implementing new requests.
I suggest using a single session that picks configurations and is shared across the app as proposed here #6939.
OK, try it out now, @montezdesousa. I found a way to hack the session into the Posthog handler. The implementation now goes the other way where
Using any of the utility HTTP request functions will read the system_settings.json and take precedence over any environment variable. Binding session objects to the
Any unclosed async session object will be closed on exit. |
This reverts commit 714df64.
Why?:
The Requests library accepts an environment variable,
REQUESTS_CA_BUNDLE
, and AIOHTTP does not. This creates inconsistencies between synchronous and async HTTP requests. Furthermore, defining this variable limits the Requests library to that particular file. If you want to use a self-signed certificate for only some things, it is not straightforward.This solution combines the specified certificate with the
certifi
defaults.What?:
system_settings.json
to accept extras.http
as a nested dictionary.make_request
amake_request
amake_requests
uvicorn.run
by storing them as a dictionary to:system_settings.python_settings.uvicorn
Impact:
REQUESTS_CA_BUNDLE
, ports directly to the async requests.system_settings.json
instead of environment variables.cafile
is an equivalent toREQUESTS_CA_BUNDLE
, when pointing to a file."verify_ssl": false
, SSL certificate verification is disabled within all OpenBB functions."python_settings["uvicorn"]"
dictionary are passed directly touvicorn.run
when launching the API as:python -m openbb_core.api.rest_api
openbb-api
These items, in
system_settings.json
, will take precedence over environment variables:Note: Keyword arguments added to the command line from
openbb-api
take precedence over thesystem_settings.json
file.Testing Done:
.crt
file as shown above.requests
andopenbb_core.provider.utils.helpers.make_request
requests.get
should fail whilemake_request
succeeds.null
and addREQUESTS_CA_BUNDLE='/full/path/to/certificate/localhost.crt'
to the.env
file.requests.get
andmake_request
should succeed.requests.get("https://google.com")
will fail,make_request
should succeed.With the environment variable defined, and not
system_settings.json
, the same A/B can be applied toyfinance.download()
vs. `obb.equity.price.historical(provider="yfinance")This fails because yFinance is only verifying against the self-signed certificate for
localhost
.openbb_yfinance.utils.helpers.yf_download
applies the environment configuration for the duration of the request.