GUI-related feature flags not validated

[DemiMarie]

How to file a helpful issue

Qubes OS release

R4.1

Brief summary

Various GUI-related features are not validated by qubesd. Once QubesOS/qubes-core-admin-client#221 lands, this will cause qvm-start-daemon to produce an invalid GUI daemon configuration file, preventing the GUI daemon from starting.

Steps to reproduce

Have QubesOS/qubes-core-admin-client#221 and set gui-subwindows feature to something invalid.

Expected behavior

Feature set call is rejected by qubesd. Attempts to set unknown gui-* features should likely be rejected unconditionally, as these could acquire a meaning in the future.

Actual behavior

Feature set call is not rejected by qubesd.

[GWeck]

Even documented gui-* features are handled inconsistently:

• While setting allow_utf8_titles via the Qube Manager to one of the values allowed, disallowed or use system default behaves as expected and sets the value of the corresponding feature gui-allow-utf8-titles accordingly, the value of the corresponding global feature gui-default-allow-utf8-titles, as shown in the qvm-features command, does not change when the global property is changed via the Qube Manager. The value used is that of the Qube Manager and not the value shown in the qvm-features command.

• It is unclear how to set global defaults from the CLI: The qvm-feautures command requires a VMname parameter, which makes no sense for a global feature.

• It is possible to create meaningless gui-* features, like allow_utf8_titles which cannot be removed; the --unset command just clears them but does not remove them.

All very confusing.

[marmarek]

Documentation says "To change a given GUI option globally, set the gui-default-{option} feature on the GuiVM for that qube." - if your GuiVM is dom0 (the default) then gui-default-* needs to be set on dom0.

[GWeck]

This makes it clear: gui-default-allow-utf8-titles should have been set not for <VMname>, but for dom0, or possibly sys-gui or sys-gui-gpu. But then this would apply to all qubes using that GuiVM, not just that qube, as stated in the documentation of qvm-features. I checked, and it works as expected, and I updated the documentation accordingly (see pull request #207 Thank you for this clarification!

The confusion about this behavior could perhaps be reduced by

• rejecting any attempt to set a gui-default-* option on any non-GuiVM with an appropriate error message;

• allowing to completely remove meaningless features via the qvm-features --unset command.

[marmarek]

• the --unset command just clears them but does not remove them.

Can you clarify? What exact command you tried and what was the result?

[GWeck]

I repeated the tests now, and qvm-features --unset now removed the meaningless parameters, like it should. Probably the earlier tests brought something into an inconsistent state causing the command to fail. So I suppose that there is nothing to do in this respect. Sorry for the inconvenience!

Still it would be nice if any attempt to set a gui-default-* option for a non-GuiVM would cause some meaningfull warning.

[alimirjamali]

Various GUI-related features are not validated by qubesd.

This appears to be easy to solve. The current GUI_DAEMON_OPTIONS is a list of tuples (feature, type). And type could be changed to lambda validation functions to be used in retrieve_gui_daemon_options function. I will work on it.

[alimirjamali]

User-end validation (within qvm-start-daemon) is done. It skips invalid values and logs an error messages. Also sends a visual notification via notify-send.

Now the same logic could be applied to qubesd to forbid users from setting invalid values for recognized features and setting unknown gui-* and default-gui-* features (but is it really necessary with the above patch?)

There are some concerns. Specific libraries are required for validating (clipboard) key sequences and valid window_background_color values. Using them within qubes-core-admin-client should be OK (hopefully). Depending qubesd on them is something else.

[alimirjamali]

The PR for this issue (including unittests) is done. All CI/CD checks are green

Review priority: low

Notes: Color and keyboard sequence verification is done via direct connection to X Display via python-xlib library. Invalid features will be ignored and user will receive a warning notification (picture below). If the validation is moved from GUIVM to qubesd to validate features at setting time, dom0 should always have access to a display server. And I wonder if valid colors/keys are the same between different versions of XServer and XWayland. Overall this is a major decision for senior developers.

[marmarek]

Since the change is has noticeable regression potential, I'm not going to backport it to R4.2, at least not yet.