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

Add ability to disable Remote #17825

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add ability to disable Remote #17825

wants to merge 6 commits into from

Conversation

SaschaCowley
Copy link
Member

@SaschaCowley SaschaCowley commented Mar 14, 2025

Link to issue number:

Closes #17791
Blocked by #17853

Summary of the issue:

In some cases, it may be desireable to disable NVDA's Remote Access functionality entirely. Before this PR, this was not possible.

Description of user facing changes

A checkbox has been added in Remote settings to enable/disable Remote. When remote is disabled, the entire feature is unavailable.
As users cannot access the Remote settings panel in secure mode, sysadmins can prevent users from using this feature in secure environments by ensuring the feature is disabled, and forcing secure mode.
Remote is disabled by default.

Description of development approach

Added a boolean config item, config.conf["remote"]["enabled"], which sets whether or not Remote is enabled. When initialising NVDA, check this item is True before calling remoteClient.initialize.

Added a new function, remoteRunning, to remoteClient. When terminating NVDA, if remoteClient.remoteRunning returns True, call remoteClient.terminate().

Added a checkbox to RemoteSettingsPanel to enable/disable remote. When saving settings, if the value of `config.conf["remote"]["enabled"] has changed, initialize or terminate Remote, as appropriate.

Updated remoteClient.client.RemoteClient.terminate to terminate its menu, if it has one.

Added a gui.blockAction.Context member, REMOTE_ACCESS_DISABLED, to block performing gestures when Remote is disabled. Decorated Remote's gestures with this block action.

Remaining questions:

  1. Secure desktops
    • We want remote to be disabled by default
    • We want remote to work on secure desktops, if the user has reached the secure desktop while a remote session was active
    • If we disable remote by default, that means it will be disabled on secure desktops, so this won't work
    • Even in organisations where they want Remote to be available, or don't force secure mode, users probably can't save their config to secure desktops
    • You cannot initiate a Remote connection on secure screens, only continue an already running one, so an option could be to default to enabling Remote on secure screens
    • If that is the case, how do we force it to disabled on secure screens?

Testing strategy:

Started and quit NVDA with Remote enabled and disabled.

Started NVDA with Remote enabled, disabled it through settings, and attempted to use Remote.

Started NVDA with Remote disabled, enabled Remote, and established a connection.

Known issues with pull request:

None known.

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

@CyrilleB79
Copy link
Collaborator

Hi. I am a quite concerned by this approach.

If correctly set up by an IT admin, the secure mode should be associated with a right restriction on the user's config folder, i.e. do not allow to write this folder. This use case is very restrictive and am not sure that it feets the needs of so many people. Indeed for a user, the possibility to modify their settings and save them all along there use of NVDA is very important.

Though, I expect that more IT admins would like to disable remote connection possibility completely, but let the users change their configuration.

Wouldn't it be possible to:

  • make the disable remote access a registry key config; no need to make it a per-user setting IMO, a system-wide setting is OK.
  • ask the IT admin to forbid writing to addons or scratchpad subfolder, and maybe other others such as addonStore, addonsState.pickle. So that a user cannot install NVDA remote add-on.

Maybe this is too broad for 2025.1 but IMO, the possibility to disable remote access offered in this PR will only be used by a very little subset of people that would need it, due to associated restrictions.

@CyrilleB79
Copy link
Collaborator

Also, has anybody really tested to restrict write rights on nvda config folder?
I have just given a try and the NVDA instance being started freezes while loading the configuration.
I am not at all an expert on right management, so maybe I did things wrong; just asking if it has been tested.

@SaschaCowley, in addition to the tests you did, you should probably test a real use case, i.e. with NVDA settings folder restrictions, so that the user cannot enable Remote access manually writing in their config file.

@codeofdusk
Copy link
Contributor

I strongly agree that a registry or similar system-wide flag is the right approach here, and that the feature should be enabled by default.

@LeonarddeR
Copy link
Collaborator

I tend to agree as well here.

@valiant8086
Copy link

presumably, a portable copy will be able to honor this? Also, I'm unsure whether the registry can be locked down by sysadmins. If both answers are yes, this sounds like a good approach. Otherwise I can learn what the registry key is and change it. We need to know whether registry can be locked though. Does that require a deep freeze and or is there a legit situation where the sysadmin won't want to or cannot lock it down that much but still does want to be able to disable NVDA remote.

@nvdaes
Copy link
Collaborator

nvdaes commented Mar 15, 2025

Cyrille wrote:

for a user, the possibility to modify their settings and save them all along there use of NVDA is very important.

I think that this is applicable to the secure mode in general. Perhaps in this mode should be possible to save certain settings not dangerous, like settings available for profiles, not global like remote, general, etc.
These settings can be changed in most cases, for example the punctuation level, though they aren't saved. If possible, I would like to have the ability to disable the remote access via the gui since it's much more easier, considering that many admins may not have a great knowledge about screen readers, even about the gui, and options made via the registry may be even less known.
If there is an issue when the configuration folder is restricted, this should be fixed for secure mode in general, imo.

@CyrilleB79
Copy link
Collaborator

@valiant8086, the registry key in HKLM are only modifiable by people with admin rights. Thus, a user with no admin right cannot modify it. That's how forcing secure mode is already implemented in NVDA.

@nvdaes that's true that my remark also applies to secure mode in general. The reg key setting is already the way to force secure mode in NVDA. Maybe we can imagine a GUI to set admin params such as start in secure mode or disable remote; this GUI would just control the corresponding reg keys. But this can be handled in a subsequent work IMO.

@SaschaCowley
Copy link
Member Author

@CyrilleB79 we are well aware of the limitations of Secure Mode, and have an issue open to address them by adding a "Corporate Mode" (see #16599). Unfortunately, any setting that we add to prevent Remote can be bypassed from the Python Console, so the only way to effectively fully disable Remote is to disable Remote and run NVDA in Secure Mode.

@SaschaCowley
Copy link
Member Author

Also, has anybody really tested to restrict write rights on nvda config folder? I have just given a try and the NVDA instance being started freezes while loading the configuration. I am not at all an expert on right management, so maybe I did things wrong; just asking if it has been tested.

I can't reproduce that with NVDA alpha-35670,a71589de . Steps:

  1. Open %AppData%, and select but do not open nvda`.
  2. Alt+Enter to open properties.
  3. Check "Read-only (Only applies to files in folder)".
  4. Quit NVDA, launch Narrator, and save the changes.
  5. Respond yes to the prompt.
  6. Quit Narrator and start NVDA.

@SaschaCowley, in addition to the tests you did, you should probably test a real use case, i.e. with NVDA settings folder restrictions, so that the user cannot enable Remote access manually writing in their config file.

In order for this setting not to be bypassed, secure mode must be enabled, and %AppData%\nvda must be read only. Without both of those in place, the user will be able to bypass the setting through the UI, by editing the config by hand, or via the Python console. This is the standard recommendation for securing NVDA.

@CyrilleB79
Copy link
Collaborator

I guess that anybody can check/uncheck the read-only checkbox, can't they?

On my side, I have tested through the "Security" tab in the Properties dialog, what would probably be more similar to what an admin would do. But I should do further tests with two distinct accounts (admin and non admin) to be really representative of the real-life situation. I'll let you know when I have time to do so.

@SaschaCowley
Copy link
Member Author

I guess that anybody can check/uncheck the read-only checkbox, can't they?

Presumably, yes.

On my side, I have tested through the "Security" tab in the Properties dialog, what would probably be more similar to what an admin would do. But I should do further tests with two distinct accounts (admin and non admin) to be really representative of the real-life situation. I'll let you know when I have time to do so.

I don't think this should make a difference. At the end of the day, they're both just preventing writing to the directory, but I'll try that route here and report back.

@SaschaCowley
Copy link
Member Author

On my side, I have tested through the "Security" tab in the Properties dialog, what would probably be more similar to what an admin would do. But I should do further tests with two distinct accounts (admin and non admin) to be really representative of the real-life situation. I'll let you know when I have time to do so.

I don't think this should make a difference. At the end of the day, they're both just preventing writing to the directory, but I'll try that route here and report back.

I was able to replicate your finding with NVDA freezing if "Disable write" was checked for my account in the security properties on %AppData%\nvda, but only when launching NVDA normally. With secure mode enabled (simulated by running nvda --secure), NVDA launched fine.

@gerald-hartig
Copy link
Collaborator

As always, thanks for all the great feedback on the PR. After reviewing the discussion, I want to take a second to clarify NV Access's position on this implementation.

The primary goal of this PR is to address the security needs in #17791, allowing organisations to disable Remote Access. There has been some discussions on implementation consistency and on having a registry-based approach. I'd like to share our reasoning for proceeding with the current implementation:

  1. We use machine-level registry keys primarily to prevent users from changing critical security settings. While we could implement Remote disabling this way, it would mean all users (including non-corporate users) would need administrator permissions to enable/disable Remote, which seems unnecessarily restrictive for personal use.
  2. The current approach allows for different security configurations on shared machines. Some user accounts could have Remote disabled while others have it enabled. This supports orgs with varying security requirements across departments or roles.
  3. Setting Remote disabled by default is consistent with a security-first approach and how things were before integration into core. Orgs / users who want Remote can easily enable it, while those in security-sensitive environments get protection by default.
  4. As Sascha noted, true security enforcement already requires combining secure mode with read-only configuration folders. This implementation aligns with that existing pattern rather than introducing a new one.
  5. We're planning more comprehensive enterprise controls through "Corporate Mode" Corporate mode for NVDA #16599, which likely may include moving away from registry keys toward group policy mechanisms. The current implementation provides an immediate solution without creating technical debt ahead of these planned improvements.

I understand this may not fully satisfy those advocating for a registry-based approach, but we believe this implementation balances immediate security needs with our longer-term architectural plans. Orgs that require strict enforcement can still achieve it through the combination of this setting with secure mode and appropriate folder permissions. As always, happy to continue the discussion if you feel that we have not considered a particular use case / user need.

@codeofdusk
Copy link
Contributor

As Sascha noted, true security enforcement already requires combining secure mode with read-only configuration folders.

Which means, to disable remote, we're now in a situation where the user can't persist things like TTS settings, which is extremely not ideal.

@SaschaCowley
Copy link
Member Author

@codeofdusk as an intermediate measure, Remote can effectively be disabled by configuring the firewall to block requests from NVDA, optionally except to endpoints like api.nvaccess.org if NVDA's update check is still desired (though it likely isn't, given users in such scenarios likely don't have the appropriate permissions to install same).

@@ -347,6 +347,7 @@

# Remote Settings
[remote]
enabled = boolean(default=False)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can see why remote is disabled by default, but for former add-on users, may be we can fix the profile upgrade in such a way that it will set it to enabled when:

  • remote.ini exists and is therefore imported into NVDA's config
  • The add-on is installed

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, that's the plan :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@seanbudd any idea how we can check if Remote is installed and enabled in the profile upgrade step? I tried

	from addonHandler import getAvailableAddons
	profile["remote"]["enabled"] = any(a.name == 'remote' and a.isEnabled for a in getAvailableAddons(refresh=True))

but this fails because localisation hasn't yet been initialised. I know I could just check for addons\remote\ in the user config directory, and presumably look at addonsState.pickle to see if it's enabled, but I'd rather not reinvent the wheel here

@SaschaCowley SaschaCowley marked this pull request as ready for review March 25, 2025 04:04
@SaschaCowley SaschaCowley requested a review from a team as a code owner March 25, 2025 04:04
@SaschaCowley SaschaCowley requested a review from seanbudd March 25, 2025 04:04
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.

Add ability to disable Remote entirely
7 participants