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 support for configuration overrides via GPOs in Windows #11223

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

egglessness
Copy link
Contributor

@egglessness egglessness commented Sep 1, 2024

Fixes: #2189

KeePassXC does not provide a way to enforce specific configurations in a centralized way. Since settings are stored only in INI files, there is no possibility to use Group Policy Objects (GPOs), which mainly rely on modifications in the Windows registry to apply enforced settings.

With this patch, it is possible to override settings using the Windows registry.

By default, user settings are still loaded from the INI files, but these settings will be overridden by any other setting defined in:

HKEY_LOCAL_MACHINE\SOFTWARE\Policies\KeePassXC

These settings follow the same hierarchy of the INI files. Each section (e.g., GUI, Security, etc.) corresponds to a registry key, and each setting is defined as a DWORD (32-bit) value.

Screenshots

For example, we can now create a policy to enforce the automatic database lock after 120 seconds of idle:

image

In the Application Settings pane, the user sees the setting as disabled and a tooltip explaining that that configuration is enforced by the organization:

image

Discussion

EDIT: now the user can see a tooltip on each managed setting.

Details

As for now, the patch is pretty minimal. When accessing the settings pane, the user gets the resulting set of configurations after the override. Any attempt to change one of the managed setting will be silently ignored: after having clicked OK and re-opened the settings pane, the setting will be reverted back at the enforced value.

Ideally, it would be better to show some visual cue to the user about the enforced settings, for instance:

  • showing a MessageWidget that says something like "KeePassXC configuration is being managed by your organization"
  • setting each QWidget corresponding to a managed setting as disabled

While showing the message would be fairly easy to implement, disabling the widgets would be much cumbersome since we would need to call the setEnabled() manually for each widget depending on the value of config()->isManaged(KEY).

That's why before even implementing it, I would like some advice from you.
Would that be acceptable, or we can think of something better?

Testing strategy

Manual

Type of change

  • ✅ New feature (change that adds functionality)
  • ✅ Refactor (significant modification to existing code)

src/core/Config.cpp Outdated Show resolved Hide resolved
@droidmonkey
Copy link
Member

Nice this is awesome, been wanting to do this for years. Here are some ideas on the visual cues:

  1. ApplicationSettingsWidget needs to be redone to strongly bind the underlying config value with each checkbox/spinbox/etc
  2. Once this binding is setup then you can get replace all the "setChecked" and "isChecked" with a loop. That loop can then check if the config setting is managed and if so it would disable it and set a tooltip "This setting is managed by your organization"

You can do similar with the spinboxes and such just not in a loop

I am personally in favor of only allowing certain settings to be managed (ie, those that actually impact security).

@egglessness
Copy link
Contributor Author

Hi @droidmonkey, thanks for your feedback!

  1. ApplicationSettingsWidget needs to be redone to strongly bind the underlying config value with each checkbox/spinbox/etc

How would you implement the strong binding? I saw that maybe we could use QProperty, but it seems limited only to Qt 6, so we would need to upgrade. Is there any other option?

I am personally in favor of only allowing certain settings to be managed (ie, those that actually impact security).

Do you have concerns over some specific settings that should not be managed?

In my experience, we found useful to enforce some non-security-related settings to better fit KeePassXC in our environment.
For instance, we enforced the option to start minimized, so that we can put KeePassXC on autorun and users would find it already opened, but minimized, ready to be used - this forested the actual adoption by quite a bit!
On top of that, we also disabled the automatic update check, since we prefer to distribute new versions using our software distribution flow, so that users get the updated version transparently.

@droidmonkey
Copy link
Member

droidmonkey commented Sep 2, 2024

For the "strong bind" I would just use either a QList<QPair<Config::ConfigKey, QCheckbox>> or QMap<Config::ConfigKey, QCheckbox>. You build the list or map in the settings widget constructor.

We can leave out the complexity of excluding certain settings.

@egglessness
Copy link
Contributor Author

egglessness commented Sep 2, 2024

For the "strong bind" I would just use either a QList<QPair<Config::ConfigKey, QCheckbox>> or QMap<Config::ConfigKey, QCheckbox>.

Oh, that's why the loop!

Yep, this was a far easier approach. I refactored the code using a QMap to bind each Config::ConfigKey with a QWidget.

@droidmonkey
Copy link
Member

Wow great job! I'll give this a test but the code looks beautiful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: Windows pr: new feature Pull request that adds a new feature security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: allow enforced defaults from the Windows registry
3 participants