Qrexec policy daemon #6

marmarta · 2019-08-09T19:38:14Z

Rewritten qrexec-daemon to use policy daemon instead of running
policy-exec separately for each call. If daemon fails, falls back
to old solution. Also contains some tests.

fixes QubesOS/qubes-issues#5125

codecov-io · 2019-08-09T22:34:18Z

Codecov Report

Merging #6 into master will increase coverage by 0.02%.
The diff coverage is 89.07%.

@@            Coverage Diff             @@
##           master       #6      +/-   ##
==========================================
+ Coverage   87.98%   88.01%   +0.02%     
==========================================
  Files          15       19       +4     
  Lines        2506     2771     +265     
==========================================
+ Hits         2205     2439     +234     
- Misses        301      332      +31

Impacted Files	Coverage Δ
qrexec/tests/qrexec_policy_daemon.py	`100% <100%> (ø)`
qrexec/__init__.py	`100% <100%> (ø)`	⬆️
qrexec/tests/cli.py	`100% <100%> (ø)`	⬆️
qrexec/tests/policy_cache.py	`100% <100%> (ø)`
qrexec/tests/policy_parser.py	`99.83% <100%> (ø)`	⬆️
qrexec/policy/parser.py	`81.31% <100%> (+0.05%)`	⬆️
qrexec/tools/qrexec_policy_exec.py	`81.9% <70.96%> (-4.77%)`	⬇️
qrexec/tools/qrexec_policy_daemon.py	`71.83% <71.83%> (ø)`
qrexec/policy/utils.py	`90.69% <90.69%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba755a8...bf57fe5. Read the comment docs.

marmarek · 2019-11-29T23:39:43Z

I've done some tests. Start situation: qrexec call into dom0 (/bin/true as a service) takes ~380ms. Observations:

Enabling this policy daemon saves ~100ms
qubes-rpc-multiplexer takes about ~130ms
Disabling policy loading on each call (caching it) saves another ~120ms

The second point should be hopefully solved by #3 (for latency critical services).
The third point could be solved by some clever caching the policy, including detection of changes. Or requiring explicit policy reload after modification, but that wouldn't be nice...

marmarek · 2019-12-03T18:04:31Z

Some issues detected during tests:

qubes-qrexec-policy-daemon service is not set to autostart (missing enable entry in a preset file)
socket is created with wrong permissions (0644 instead of 0660, especially group qubes don't have access, making it impossible to connect)
logs about allowed connections are duplicated in journalctl - once as qrexec and then as qrexec-policy-daemon
the daemon waits for qrexec-client process to terminate before serving next requests; in case of connection to dom0, this means waiting until the whole connection is terminated; this can be also an issue (untested) for calls into DisposableVM, where the daemon needs to cleanup the VM after connection is terminated

used by QubesOS/qubes-core-qrexec#6 references QubesOS/qubes-issues#5125

marmarek · 2019-12-07T12:57:29Z

Documentation/qrexec-policy-daemon.rst

+- assume_yes_for_ask=yes
+- just_evaluate=yes
+
+Newline-separated


Already mentioned at the beginning.

marmarek · 2019-12-07T12:58:09Z

Documentation/qrexec-policy-daemon.rst

+
+newline + further
+
+all not compliant responses are bad no good terrible


This should be more formal.

marmarek · 2019-12-07T12:58:53Z

qrexec/policy/utils.py

+#
+# The Qubes OS Project, http://www.qubes-os.org
+#
+# Copyright (C) 2017 Marta Marczykowska-Górecka


marmarek · 2019-12-07T13:00:54Z

qrexec/tests/qrexec_policy_daemon.py

+            functools.partial(qrexec_policy_daemon.handle_client_connection,
+                              log, Mock()),
+            path=str(tmp_path / "socket.d"))
+        # because travis is stuck in python3.5, we can't use yield


In the meantime I've updated travis to python 3.7. If you want, you can change it now. But it can stay as is too.

marmarek · 2019-12-07T13:05:16Z

qrexec/tests/qrexec_policy_daemon.py

+    @pytest.mark.asyncio
+    async def test_wrong_arg(self, mock_request, async_server, tmp_path):
+
+        data = b'domains_id=a\n' \


Please add a comment pointing at this typo, or make the wrong arg more explicit.

marmarek · 2019-12-07T13:05:29Z

qrexec/tools/qrexec_policy_daemon.py

+#
+# The Qubes OS Project, http://www.qubes-os.org
+#
+# Copyright (C) 2017 Marta Marczykowska-Górecka


marmarek · 2019-12-07T16:50:49Z

rpm_spec/qubes-qrexec-dom0.spec.in

+# and then start it back
+if systemctl is-enabled qubes-qrexec-policy-daemon.service >/dev/null 2>&1; then
+    systemctl start qubes-qrexec-policy-daemon.service
+fi


Is this %posttrans section really necessary? It is in the -vm package, because qubes-qrexec-agent.service was present before in a different package (was moved from qubes-core-agent-linux to qubes-core-qrexec). But the policy daemon wasn't here before.

marmarek · 2019-12-07T16:52:06Z

qrexec/policy/parser.py

@@ -1357,7 +1359,7 @@ class FilePolicy(AbstractFileSystemLoader, AbstractPolicy):
    ...     'qrexec.Service', '+argument', 'source-name', 'target-name',
    ...     system_info=qrexec.utils.get_system_info())
    >>> resolution = policy.evaluate(request)
-    >>> resolution.execute('process-ident')
+    >>> resolution.execute('process-ident')  # this method is asynchroneous


I'd add this for clarity:

Suggested change

>>> resolution.execute('process-ident') # this method is asynchroneous

>>> await resolution.execute('process-ident') # this method is asynchroneous

marmarek · 2019-12-07T16:53:44Z

qrexec/tools/qrexec_policy_daemon.py

+                            OPTIONAL_REQUEST_ARGUMENTS
+
+
+class DaemonResolution(AllowResolution):


DaemonAllowResolution?

Also includes rudimentary protocol documentation. Contains only pure policy daemon that's not used by anything yet. references QubesOS/qubes-issues#5125

Rewritten qrexec-daemon to use policy daemon instead of running policy-exec separately for each call. If daemon fails, falls back to old solution. fixes QubesOS/qubes-issues#5125

Log message and daemon response now are sent before the call is fully executed. Fixed daemon socket permissions being wrong. No more duplicate logs. Calls are now handled asynchroneously.

Policy parser will now be cached and updated only when changes in policy directories occured.

DemiMarie · 2019-12-07T20:00:01Z

@marmarek @marmarta is there still a reason to fall back to the old solution?

DemiMarie · 2019-12-07T20:01:41Z

systemd/qubes-qrexec-policy-daemon.service

+
+[Service]
+Type=simple
+ExecStart=/usr/bin/qrexec-policy-daemon


It would be nice if this used socket activation.

marmarek · 2019-12-07T21:12:03Z

@marmarek @marmarta is there still a reason to fall back to the old solution?

Yes, if policy daemon isn't running/can't start for any reason. Having some kind of fallback is especially useful with GUI VM (qrexec as the only interface to dom0). This is also why socket activation may not be the best idea - it would prevent such fallback from working. On the other hand, automatic restart on crash may be worth adding.

marmarek · 2020-01-22T16:35:13Z

I'll merge this PR only after #3, as conflict resolution will be much easier this way.

marmarta force-pushed the qrexec-policy-daemon branch 12 times, most recently from 31de73d to d88a0da Compare August 9, 2019 22:25

marmarta force-pushed the qrexec-policy-daemon branch 3 times, most recently from 4586fc6 to 12f048f Compare August 9, 2019 22:50

marmarek approved these changes Aug 9, 2019

View reviewed changes

marmarek mentioned this pull request Oct 11, 2019

Allow users to choose a SOCKS5 proxy instead of firewall rules. QubesOS/qubes-issues#5032

Open

marmarta added a commit to marmarta/qubes-core-admin-linux that referenced this pull request Dec 3, 2019

Added enabling of qrexec-policy-daemon.service

761b5b1

used by QubesOS/qubes-core-qrexec#6 references QubesOS/qubes-issues#5125

marmarta mentioned this pull request Dec 3, 2019

Added enabling of qrexec-policy-daemon.service QubesOS/qubes-core-admin-linux#53

Merged

marmarta force-pushed the qrexec-policy-daemon branch 8 times, most recently from a2213ec to 19e79f7 Compare December 7, 2019 01:26

marmarta force-pushed the qrexec-policy-daemon branch 3 times, most recently from 5534e24 to e35906a Compare December 7, 2019 12:17

marmarek requested changes Dec 7, 2019

View reviewed changes

marmarta added 7 commits December 7, 2019 18:22

Qrexec policy daemon and tests

703f6cd

Also includes rudimentary protocol documentation. Contains only pure policy daemon that's not used by anything yet. references QubesOS/qubes-issues#5125

Working qrexec-policy-daemon

de23776

Rewritten qrexec-daemon to use policy daemon instead of running policy-exec separately for each call. If daemon fails, falls back to old solution. fixes QubesOS/qubes-issues#5125

Fixed assorted errors and better handling for logging

ccb4bcb

Log message and daemon response now are sent before the call is fully executed. Fixed daemon socket permissions being wrong. No more duplicate logs. Calls are now handled asynchroneously.

Fixed tests and assorted bugs discovered with them

bd2c581

Added policy caching

c4c72dd

Policy parser will now be cached and updated only when changes in policy directories occured.

Added tests to PolicyCache

d6bf65d

Typos cleanup

bf57fe5

marmarta force-pushed the qrexec-policy-daemon branch from 46094a4 to bf57fe5 Compare December 7, 2019 17:29

marmarek approved these changes Dec 7, 2019

View reviewed changes

DemiMarie reviewed Dec 7, 2019

View reviewed changes

DemiMarie mentioned this pull request Feb 8, 2020

Working socket-based qrexec #3

Merged

This was referenced Feb 17, 2020

Socket based qrexec: refactor file search, add tests #24

Merged

Qrexec policy daemon: solve merge conflicts, update #25

Merged

marmarek merged commit 7c073a7 into QubesOS:master Feb 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Qrexec policy daemon #6

Qrexec policy daemon #6

marmarta commented Aug 9, 2019

codecov-io commented Aug 9, 2019 •

edited by codecov bot

Loading

marmarek commented Nov 29, 2019

marmarek commented Dec 3, 2019

marmarek Dec 7, 2019

marmarek Dec 7, 2019

marmarek Dec 7, 2019

marmarek Dec 7, 2019

marmarek Dec 7, 2019

marmarek Dec 7, 2019

marmarek Dec 7, 2019

marmarek Dec 7, 2019

marmarek Dec 7, 2019

DemiMarie commented Dec 7, 2019

DemiMarie Dec 7, 2019

marmarek commented Dec 7, 2019

marmarek commented Jan 22, 2020


		newline + further

		all not compliant responses are bad no good terrible

	>>> resolution.execute('process-ident') # this method is asynchroneous
	>>> await resolution.execute('process-ident') # this method is asynchroneous

		OPTIONAL_REQUEST_ARGUMENTS


		class DaemonResolution(AllowResolution):

Qrexec policy daemon #6

Qrexec policy daemon #6

Conversation

marmarta commented Aug 9, 2019

codecov-io commented Aug 9, 2019 • edited by codecov bot Loading

Codecov Report

marmarek commented Nov 29, 2019

marmarek commented Dec 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DemiMarie commented Dec 7, 2019

Choose a reason for hiding this comment

marmarek commented Dec 7, 2019

marmarek commented Jan 22, 2020

codecov-io commented Aug 9, 2019 •

edited by codecov bot

Loading