Skip to content

Use truststore directly not pip_system_certs#18490

Merged
SaschaCowley merged 4 commits into
betafrom
useTrustStoreDirectly
Jul 18, 2025
Merged

Use truststore directly not pip_system_certs#18490
SaschaCowley merged 4 commits into
betafrom
useTrustStoreDirectly

Conversation

@seanbudd

@seanbudd seanbudd commented Jul 17, 2025

Copy link
Copy Markdown
Member

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:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

@coderabbitai summary

@seanbudd

Copy link
Copy Markdown
Member Author

@hwf1324 could you please test if this build still fixes #15905, i.e. this build still uses system certs not certifi, you can use the add-on store and update check.
I'm hoping using the latest version of truststore like this still works and fixes #18460

@seanbudd seanbudd added this to the 2025.2 milestone Jul 17, 2025
@seanbudd

Copy link
Copy Markdown
Member Author

@jmdaweb could you test if this PR works with tele NVDA?

@hwf1324

hwf1324 commented Jul 17, 2025

Copy link
Copy Markdown
Contributor

could you please test if this build still fixes #15905, i.e. this build still uses system certs not certifi, you can use the add-on store and update check. I'm hoping using the latest version of truststore like this still works and fixes #18460

Yes, this is still working. I used this PR to test with 2025.1.2.

  • Failed to download add-on for 2025.1.2
  • This PR successfully downloads the add-on

By the way, does restarting NVDA refresh environment variables?

@seanbudd

Copy link
Copy Markdown
Member Author

which environment variables do you mean @hwf1324

@seanbudd seanbudd marked this pull request as ready for review July 17, 2025 08:08
Copilot AI review requested due to automatic review settings July 17, 2025 08:08
@seanbudd seanbudd requested a review from a team as a code owner July 17, 2025 08:08
@seanbudd seanbudd requested a review from SaschaCowley July 17, 2025 08:08
@seanbudd

Copy link
Copy Markdown
Member Author

thank you for testing as well - we will get this into a beta4

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() in core.py
  • Remove legacy pip_system_certs monkey-patch blocks in transport.py and server.py
  • Update pyproject.toml to depend on truststore and drop pip_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_certs has been replaced by truststore for 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()

@hwf1324

hwf1324 commented Jul 17, 2025

Copy link
Copy Markdown
Contributor

which environment variables do you mean @hwf1324

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.

@seanbudd

Copy link
Copy Markdown
Member Author

I think generally yes restarting NVDA keeps the CLI parameters you started it with

@SaschaCowley SaschaCowley merged commit fcf7182 into beta Jul 18, 2025
18 of 19 checks passed
@SaschaCowley SaschaCowley deleted the useTrustStoreDirectly branch July 18, 2025 03:38
seanbudd added a commit that referenced this pull request Jul 22, 2025
seanbudd added a commit that referenced this pull request Jul 22, 2025
### 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
SaschaCowley added a commit that referenced this pull request Oct 10, 2025
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants