Use truststore directly not pip_system_certs#18490
Conversation
|
@jmdaweb could you test if this PR works with tele NVDA? |
Yes, this is still working. I used this PR to test with 2025.1.2.
By the way, does restarting NVDA refresh environment variables? |
|
which environment variables do you mean @hwf1324 |
|
thank you for testing as well - we will get this into a beta4 |
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the deprecated pip_system_certs workaround with direct use of truststore for system certificate handling, and cleans up related temporary patches in remote client modules.
- Switch to
truststore.inject_into_ssl()incore.py - Remove legacy
pip_system_certsmonkey-patch blocks intransport.pyandserver.py - Update
pyproject.tomlto depend ontruststoreand droppip_system_certs
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| source/core.py | Use truststore.inject_into_ssl() instead of pip_system_certs |
| source/_remoteClient/transport.py | Remove unittest.mock patch that worked around pip_system_certs |
| source/_remoteClient/server.py | Remove similar mock patch for server SSL socket |
| pyproject.toml | Replace pip_system_certs with truststore==0.10.1 |
Comments suppressed due to low confidence (3)
source/core.py:677
- Add a unit or integration test to verify that HTTP requests made after
truststore.inject_into_ssl()actually use the Windows root certificates, guarding against regressions in certificate handling.
# Use Windows root certificates for requests rather than certifi.
pyproject.toml:33
- [nitpick] Update the project README or developer documentation to note that
pip_system_certshas been replaced bytruststorefor system certificate management.
"truststore==0.10.1",
source/core.py:678
- Consider wrapping
truststore.inject_into_ssl()in a try/except block to log failures and avoid crashing the application on startup if certificate injection fails.
truststore.inject_into_ssl()
System environment variables. It should be because when restarting, a new NVDA instance is started from the current NVDA process, so it inherits the environment variables of the current instance. |
|
I think generally yes restarting NVDA keeps the CLI parameters you started it with |
### Reverts PR Reverts #18354 Reverts #18490 Reverts #18402 ### Issues fixed <!-- Issues that will be closed by reverting, i.e. the issues introduced via the PR getting reverted --> Fixes #18519 ### Issues reopened <!-- Issues that will be re-opened by reverting, i.e. the issues that were fixed via the PR getting reverted --> Reopens #15905 ### Reason for revert The current implementation breaks NVDA remote and breaks add-on APIs ### Can this PR be reimplemented? If so, what is required for the next attempt - use truststore.SSLContext directly rather than source code wide injections. It is [unclear](https://truststore.readthedocs.io/en/latest/#using-truststore-with-requests) how to do that with requests. [this SO answer suggestions an option](https://stackoverflow.com/a/78265028). - perform the changes in an API breaking release, encourage authors to use `inject_into_ssl, extract_from_ssl` where required
…8528) Fixes #15905 Reimplements #18354, #18402, #18490 ### Summary of the issue: When trying to download an add-on from the add-on store in certain environments such as corporates, the download fails due to the machine not trusting the root certificate of the add-on source. This can be caused by two issues: * the requests library using certifi rather than the system root certificates * the system root certificates not trusting the add-on download source When performing an update check, we update windows root certificates if our certificate is invalid or out of date. ### Description of user facing changes: NVDA should more reliably trust request endpoints using windows root certificates. e.g. accessing the add-on store in a corporate environment. ### Description of developer facing changes: requests and similar libraries now has truststore injected into it ### Description of development approach: This pull request improves how NVDA handles SSL/TLS certificate verification, particularly for HTTPS requests and add-on downloads. It ensures that the application uses the Windows root certificate store instead of the Python default (certifi), provides a fallback mechanism to update root certificates when encountering untrusted certificates, and enhances user interaction when certificate errors occur during add-on downloads. Additionally, shared networking logic has been refactored and centralized for maintainability. **Certificate Verification and Networking Improvements:** * Added the `truststore` dependency and injected it at startup so that all `requests` HTTP calls use the Windows root certificate store instead of certifi, improving compatibility in corporate and managed environments. (`pyproject.toml`, `source/core.py`) [[1]](diffhunk://#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711R32-R33) [[2]](diffhunk://#diff-dbe862167add4fbe8e502757f792615f714c7603621c03d0c60c08a6a613ad68R34) [[3]](diffhunk://#diff-dbe862167add4fbe8e502757f792615f714c7603621c03d0c60c08a6a613ad68R680-R683) * Introduced a new `utils/networking.py` module that centralizes logic for fetching URLs, handling certificate verification errors, and updating Windows root certificates. This includes helper functions like `_fetchUrlAndUpdateRootCertificates`, `_getCertificate`, and `_updateWindowsRootCertificates`. (`source/utils/networking.py`) **Add-on Store and Update Check Enhancements:** * Updated add-on store and update check logic to use the new `_fetchUrlAndUpdateRootCertificates` function, ensuring robust handling of certificate verification failures and automatic root certificate updates when necessary. (`source/addonStore/dataManager.py`, `source/updateCheck.py`) [[1]](diffhunk://#diff-9fd9bbb610bac3157554c0876fba6ea3475383ea9de36c2101ba1127cf5189e5R30) [[2]](diffhunk://#diff-9fd9bbb610bac3157554c0876fba6ea3475383ea9de36c2101ba1127cf5189e5L127-R128) [[3]](diffhunk://#diff-9fd9bbb610bac3157554c0876fba6ea3475383ea9de36c2101ba1127cf5189e5L138-R143) [[4]](diffhunk://#diff-69aa1f3d46c308cf71c96483fd196eb49d6348727ab1953f834b1974ff031a43R68-R76) [[5]](diffhunk://#diff-69aa1f3d46c308cf71c96483fd196eb49d6348727ab1953f834b1974ff031a43L241-R245) * Improved the add-on download process to prompt the user when encountering an untrusted certificate, displaying the SHA256 fingerprint and allowing the user to trust and install the root certificate if appropriate. (`source/addonStore/network.py`) [[1]](diffhunk://#diff-097e46da59cc87eb0100bd63db7cfe32c7095ec66cf4be02d5dc84f28682ac7eL239-R297) [[2]](diffhunk://#diff-097e46da59cc87eb0100bd63db7cfe32c7095ec66cf4be02d5dc84f28682ac7eR314-R315) **Codebase Cleanup and Refactoring:** * Removed duplicate and now-unnecessary certificate update logic from `updateCheck.py` and centralized all certificate handling in `utils/networking.py`. (`source/updateCheck.py`) * Updated variable naming and improved code clarity in the add-on download logic. (`source/addonStore/network.py`) [[1]](diffhunk://#diff-097e46da59cc87eb0100bd63db7cfe32c7095ec66cf4be02d5dc84f28682ac7eL239-R297) [[2]](diffhunk://#diff-097e46da59cc87eb0100bd63db7cfe32c7095ec66cf4be02d5dc84f28682ac7eR314-R315) [[3]](diffhunk://#diff-097e46da59cc87eb0100bd63db7cfe32c7095ec66cf4be02d5dc84f28682ac7eL268-R324) [[4]](diffhunk://#diff-097e46da59cc87eb0100bd63db7cfe32c7095ec66cf4be02d5dc84f28682ac7eL278-R334) [[5]](diffhunk://#diff-097e46da59cc87eb0100bd63db7cfe32c7095ec66cf4be02d5dc84f28682ac7eL290-R346) ### Testing strategy: - [x] Get user testing from #15905 - [x] Smoke test using add-on store - [x] Smoke test update check ### Known issues with pull request: None --------- Co-authored-by: Sascha Cowley <16543535+SaschaCowley@users.noreply.github.com>
Link to issue number:
Fixes #18460
Fixup of #18354
Summary of the issue:
There is a fragile workaround introduced in #18402 (comment).
This is also a soft API breakage for add-ons, forcing teleNVDA to use the workaround
This is likely a bug fixed in truststore sethmlarson/truststore#149
We are using pip_system_certs to patch in an old version of truststore. this is no longer being actively maintained and apparently truststore has the same patching feature now.
Description of user facing changes:
None
Description of developer facing changes:
Fix workaround which was required for add-ons such as tele NVDA
Description of development approach:
Testing strategy:
Ensure #18460 is not reintroduced.
Make sure NVDA can still use update check and the add-on store in corporate network environments.
Known issues with pull request:
Code Review Checklist:
@coderabbitai summary