Add QWT support #181

omeg · 2025-02-10T16:24:31Z

marmarek

This isn't a full review yet, but I think I captured main point here already.

marmarek · 2025-02-18T15:25:32Z

rpc/qubes.WinSign.CreateKey

+set -efo pipefail
+
+# shellcheck source=SCRIPTDIR/qubes.WinSign.common
+. /etc/qubes-rpc/qubes.WinSign.common


Better not hardcode the path here. Especially, it's convenient to install the RPC files in /usr/local/etc/qubes-rpc in a specific AppVM.
You can use $(dirname "$0") to find the directory.

Similar comment to all the other services too.

marmarek · 2025-02-18T15:26:15Z

tools/windows/edit-iso.sh

+
+echo "[*] Copying the final iso from '${DISPVM}' to '${OUTPUT}'..."
+
+shell_call "${DISPVM}" "cat ~/win-build.iso" | cat > "${OUTPUT}"


Why extra cat?

Suggested change

shell_call "${DISPVM}" "cat ~/win-build.iso" | cat > "${OUTPUT}"

shell_call "${DISPVM}" "cat ~/win-build.iso" > "${OUTPUT}"

marmarek · 2025-02-18T15:28:46Z

qubesbuilder/plugins/build_windows/__init__.py

+                    key_name=sign_key_name,
+                )
+
+            dvm.kill()


If starting dvm fails, this will fail too (UnboundLocalError: cannot access local variable 'dvm' where it is not associated with a value). Either move starting dvm before the try part, or initialize the variable early (with None?) and check for it here.
But also, it looks like the dvm start can be moved much later? Currently it's at the start of the build, but it's used only after the build - so, if you move it later, you can save starting it in case of build failure.

marmarek · 2025-02-18T15:29:06Z

rpc/policy/51-qubesbuilder-windows.policy

+qubes.WinSign.CreateKey  +Qubes__Windows__Tools  work-qubesos  vault-sign  allow
+qubes.WinSign.DeleteKey  +Qubes__Windows__Tools  work-qubesos  vault-sign  allow
+qubes.WinSign.GetCert    +Qubes__Windows__Tools  work-qubesos  vault-sign  allow
+qubes.WinSign.Sign       +Qubes__Windows__Tools  work-qubesos  vault-sign  allow


Missing policy for the timestamp service

marmarek · 2025-02-18T15:30:11Z

rpc/policy/51-qubesbuilder-windows.policy

+admin.vm.property.Get            *  work-qubesos  win-build     allow  target=dom0
+admin.vm.device.block.Attach     *  work-qubesos  win-build     allow  target=dom0
+
+qubes.WinSign.QueryKey   +Qubes__Windows__Tools  work-qubesos  vault-sign  allow


Generally for builder-specific services we use qubesbuilder. prefix.

marmarek · 2025-02-18T15:50:57Z

qubesbuilder/executors/windows.py

+    def run(
+        self,
+        cmd: List[str],
+        copy_in: List[Tuple[Path, PurePath]] = None,
+        copy_out: List[Tuple[PurePath, Path]] = None,
+    ):


To support building in DispVM, this should be able to create and start new DispVM first and kill it at the end.
It does mean fresh DispVM for each component - is that a problem? In other words - are there some hidden assumption about reusing the same VM (like, depending on some files being present from previous build)?

For development builds it probably make sense to retain option to reuse the same VM over and over, so the DispVM case can be under some config option.

I think it can be a DispVM all the time, for development I usually just do local builds without the builder.

marmarek · 2025-02-18T15:58:00Z

qubesbuilder/executors/windows.py

+from qubesadmin import Qubes
+from qubesadmin.devices import DeviceAssignment, UnknownDevice
+from qubesadmin.exc import DeviceAlreadyAttached, QubesException
+from qubesadmin.utils import encode_for_vmexec
+from qubesadmin.vm import DispVM, QubesVM


It should be possible to use qubes-builderv2 outside of Qubes VM too. It doesn't need to support building Windows code there, but it shouldn't crash on ImportError. I think there need to be try / except ImportError somewhere, that will catch it nicely. I have an idea where - see separate comment.

marmarek · 2025-02-18T15:58:51Z

qubesbuilder/config.py

@@ -31,6 +31,7 @@
 from qubesbuilder.executors.container import ContainerExecutor
 from qubesbuilder.executors.local import LocalExecutor
 from qubesbuilder.executors.qubes import LinuxQubesExecutor
+from qubesbuilder.executors.windows import WindowsExecutor


You can catch ImportError here, and set some variable like windows_executor_available = False

marmarek · 2025-02-18T15:59:18Z

qubesbuilder/config.py

@@ -575,6 +576,8 @@ def get_executor(options):
            executor = LocalExecutor(**executor_options)  # type: ignore
        elif executor_type == "qubes":
            executor = LinuxQubesExecutor(**executor_options)  # type: ignore
+        elif executor_type == "windows":
+            executor = WindowsExecutor(**executor_options)  # type: ignore


And here throw an exception if windows_executor_available is false.

marmarek · 2025-02-18T16:05:31Z

qubesbuilder/plugins/build_windows/__init__.py

+from qubesbuilder.config import Config
+from qubesbuilder.distribution import QubesDistribution
+from qubesbuilder.executors import ExecutorError
+from qubesbuilder.executors.windows import WindowsExecutor


And similarly this (and the qubesadmin imports above) can be problematic. It wants either some wrapping here (and raise an exception early in run if qubesadmin is not available), or changing PluginManager so that plugin import error is not fatal. IMO the first option is better if wouldn't clutter the code too much.

FWIW several CI tests that are unrelated to QWT fail due to importing qubesadmin. That could be a good check if it works as expected - tests not related to QWT should pass even if qubesadmin cannot be imported.

I removed the qubesadmin dependency.

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

marmarek · 2025-02-26T17:57:09Z

It has conflicts now... @fepitre in the meantime added most of the cross-distribution dependencies that will be needed for building iso+rpm (remaining part is in #185, but that shouldn't conflict anymore, I hope).

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

Add option to not ask user to enable network discovery Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

…age options Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

codecov · 2025-02-27T14:46:32Z

Codecov Report

Attention: Patch coverage is 34.53510% with 345 lines in your changes missing coverage. Please review.

Project coverage is 74.79%. Comparing base (1987baa) to head (4916714).
Report is 54 commits behind head on main.

Files with missing lines	Patch %	Lines
qubesbuilder/plugins/build_windows/__init__.py	21.93%	153 Missing ⚠️
qubesbuilder/executors/windows.py	22.60%	89 Missing ⚠️
qubesbuilder/executors/qubes.py	44.31%	49 Missing ⚠️
qubesbuilder/plugins/source_windows/__init__.py	42.10%	22 Missing ⚠️
qubesbuilder/executors/qrexec.py	70.68%	17 Missing ⚠️
qubesbuilder/plugins/__init__.py	46.15%	7 Missing ⚠️
qubesbuilder/config.py	33.33%	4 Missing ⚠️
qubesbuilder/distribution.py	60.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #181      +/-   ##
==========================================
- Coverage   78.74%   74.79%   -3.95%     
==========================================
  Files          47       51       +4     
  Lines        5388     5865     +477     
==========================================
+ Hits         4243     4387     +144     
- Misses       1145     1478     +333

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

marmarek · 2025-02-27T14:49:57Z

Can you squash the fixup commits (just do git rebase -i --autosquash)

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

…utor Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

tlaurion · 2025-02-28T20:45:30Z

Amazing work @omeg ! Can't wait to test testing packages. Also really interesting piece of work in terms of complex qubes-builderv2 for a learner. Thanks!

marmarek · 2025-03-04T11:13:39Z

I reviewed this as of 5f3210a, and generally looks good, I'll re-test it again and if all good should be good for merging. There are a couple of TODO comments - are planning to handle them now, or leave for later (I'm fine with either)?

omeg · 2025-03-04T11:17:57Z

I reviewed this as of 5f3210a, and generally looks good, I'll re-test it again and if all good should be good for merging. There are a couple of TODO comments - are planning to handle them now, or leave for later (I'm fine with either)?

I think we can merge, the remaining TODOs are minor improvements that don't impact functionality.

marmarek

With this version, using window-ssh executor I needed to attach EWDK myself. I think it was done automatically before. Since the lifetime of this VM is outside of the builder control (right?) this step needs to be documented. In theory it's one time thing (persistent attach), but the loop device needs to be re-created after each work-qubesos VM restart (or added to rc.local...). But if it can be automated, even better.

marmarek · 2025-03-05T02:18:47Z

tools/windows/edit-iso.sh

+SELF=$(qubesdb-read /name)
+read -r -a result <<< "$(qrexec_call "${SELF}" admin.vm.property.Get+default_dispvm)"
+dispvm_template="${result[2]}"  # default=False type=vm vm-name
+DISPVM=$(qrexec_call "$dispvm_template" admin.vm.CreateDisposable)


Suggested change

DISPVM=$(qrexec_call "$dispvm_template" admin.vm.CreateDisposable)

DISPVM=$(qrexec_call "dom0" admin.vm.CreateDisposable)

Doing the call to dom0 makes it use default_dispvm, so you can save one call. And admin.vm.property.Get in the policy.

marmarek · 2025-03-05T02:18:58Z

rpc/qubesbuilder.WinSign.GetCert

+set -efo pipefail
+
+# shellcheck source=SCRIPTDIR/qubesbuilder.WinSign.common
+. "$(dirname "$0")/qubes.WinSign.common"


Suggested change

. "$(dirname "$0")/qubes.WinSign.common"

. "$(dirname "$0")/qubesbuilder.WinSign.common"

marmarek · 2025-03-05T02:29:54Z

rpc/policy/51-qubesbuilder-windows.policy

+admin.vm.device.block.Available  *  work-qubesos  work-qubesos  allow  target=dom0
+
+admin.vm.Start                   *  work-qubesos  win-build     allow  target=dom0
+admin.vm.CurrentState            *  work-qubesos  win-build     allow  target=dom0


This appears to be unused.

marmarek · 2025-03-05T02:46:06Z

rpc/policy/51-qubesbuilder-windows.policy

Missing policy for qubesbuilder.WinFileCopyIn and qubesbuilder.WinFileCopyOut

marmarek · 2025-03-05T02:51:26Z

qubesbuilder/executors/qubes.py

+            what="copy-in",
+            vm=self.dispvm,
+            service=f"{self.copy_in_service}+{encoded_dst_path}",
+            args=["/usr/lib/qubes/qfile-agent", str(src)],


I'd suggest for windows adding --ignore-symlinks, or ignore them at the receiver side. At least for me it choked on them (I did have some in one of the repos).

you added the parameter, but it isn't actually used in the command...

Oops, fixed.

marmarek · 2025-03-05T03:01:48Z

qubesbuilder/executors/qrexec.py

+    qrexec_call(
+        log=log,
+        what="remove vm",
+        vm=vm,
+        service="admin.vm.Remove",
+        ignore_errors=True,
+    )


Kill on a DispVM implicitly remove it already. In practice this one is harmless, but results in a warning, as the target VM doesn't exist at this point anymore.

The dispvm is created in a stopped state (to attach EWDK) so Kill doesn't do anything if there's some error during this phase. I guess I can only call Remove if Kill fails...

I see. Yes, doing Remove if Kill fails with QubesVMNotStartedError (or maybe simply any error) is a good idea.

Improved dispvm removal.

omeg · 2025-03-05T11:30:43Z

With this version, using window-ssh executor I needed to attach EWDK myself. I think it was done automatically before. Since the lifetime of this VM is outside of the builder control (right?) this step needs to be documented. In theory it's one time thing (persistent attach), but the loop device needs to be re-created after each work-qubesos VM restart (or added to rc.local...). But if it can be automated, even better.

I followed @fepitre suggestion that in the SSH case the Windows worker could be any (even non-Qubes) Windows machine. Maybe I should add a config option to auto attach EWDK in the common (Qubes VM) case?

omeg · 2025-03-05T11:32:10Z

Or even better, try to attach EWDK if the current ewdk path option is specified.

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

omeg · 2025-03-05T18:15:03Z

I've added ssh-vm parameter in the config that, if set, will cause the SSH executor to auto-start the vm and auto-attach EWDK to it. If the parameter is not present, it's assumed all configuration was done manually.

marmarek · 2025-03-07T19:01:40Z

tools/windows/generate-iso.sh

+"
+}
+
+if ! OPTS=$(getopt -o hi:o: --long help,iso,output: -n "$0" -- "$@"); then


Suggested change

if ! OPTS=$(getopt -o hi:o: --long help,iso,output: -n "$0" -- "$@"); then

if ! OPTS=$(getopt -o hi:o: --long help,iso:,output: -n "$0" -- "$@"); then

marmarek · 2025-03-07T19:02:59Z

tools/windows/edit-iso.sh

+SCRIPT_DIR=$(readlink -f "${SCRIPT_DIR}")
+
+echo "[*] Setting up a loop device for the ISO..."
+LODEV=$(losetup -f)


and also this sometimes needs sudo (if there are no free devices and one needs to be created)

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

Also support auto-starting a SSH qube and attaching EWDK to it. Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

marmarek requested changes Feb 18, 2025

View reviewed changes

Add support for Windows executor and build

12d3f67

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

omeg added 15 commits February 26, 2025 18:57

windows: add support for code signing

9bdf7c6

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: improve unattended OS installation

b964409

Add option to not ask user to enable network discovery Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

Fix typos in README

b091107

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: fix shellcheck warnings in scripts

212940e

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: fix download script call

b6501c3

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: untabify autounattend.xml

37bbc89

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: generate random user password during worker VM setup

8390740

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: fix existing file check in get-files.sh

5bad846

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: fix missing configuration option in README

5ac9452

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: move the CONFIGURATION placeholder from executor to build st…

d8aab4e

…age options Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: automatically create loop device for EWDK

298fe55

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: edit the installation iso in a dispvm

0d05f14

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: update RPC policy files

c8b9987

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: refactor signing

10f4ac5

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: timestamp when code signing

12fe446

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

omeg force-pushed the omeg/windows branch from 3ceadd4 to a23caaf Compare February 26, 2025 19:45

omeg added 9 commits February 27, 2025 15:56

windows: support build via qrexec services

2f46e61

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: add scripts for local build

83729a5

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: remove redundant code

db70cbf

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: add note about MS Defender in the readme

eb90a85

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: remove unneeded checks of vm status to improve performance

84e4f85

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: reword 'mount' to 'attach' for EWDK loopback device

26e4737

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: create local .distfiles only if necessary

a5ccf6f

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: fix testsign switch in the build plugin

b965b0f

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: improve copy_out performance

382d11b

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

omeg added 8 commits February 27, 2025 15:57

windows: update CI dependencies

d74a23f

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

Fix CI config

d9a809e

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: fix shellcheck warnings in rpc files

977fe2b

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: fix review issues

d9984d9

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

Refactor WindowsExecutor into WindowsQubesExecutor and SSHWindowsExec…

459a5d9

…utor Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: sanitize_line() compatibility

819b714

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: remove qubesadmin dependency

279996b

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: update README

5f3210a

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

omeg force-pushed the omeg/windows branch from 40e0082 to 5f3210a Compare February 27, 2025 15:26

omeg marked this pull request as ready for review February 27, 2025 15:36

marmarek requested changes Mar 5, 2025

View reviewed changes

Properly query vm state in dispvm cleanup

4300e70

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

omeg force-pushed the omeg/windows branch from 95920d7 to 51d9a95 Compare March 7, 2025 17:47

marmarek reviewed Mar 7, 2025

View reviewed changes

omeg added 6 commits March 7, 2025 21:24

windows: ignore symlinks for copy_in

eaeb841

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: fix review issues

a830117

Also support auto-starting a SSH qube and attaching EWDK to it. Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: allow saving generated Windows ISO under a custom name

727cc86

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

Only use admin.vm.CurrentState in WindowsQubesExecutor

0f4ab3f

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: rename 'artifacts' to 'files' in build plugin artifact info

97eb2d5

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

windows: fix bash script issues

4916714

Signed-off-by: Rafał Wojdyła <omeg@invisiblethingslab.com>

omeg force-pushed the omeg/windows branch from 51d9a95 to 4916714 Compare March 7, 2025 20:25

marmarek approved these changes Mar 8, 2025

View reviewed changes

marmarek merged commit ed3319c into QubesOS:main Mar 8, 2025
2 of 4 checks passed


		echo "[*] Copying the final iso from '${DISPVM}' to '${OUTPUT}'..."

		shell_call "${DISPVM}" "cat ~/win-build.iso" \| cat > "${OUTPUT}"

	DISPVM=$(qrexec_call "$dispvm_template" admin.vm.CreateDisposable)
	DISPVM=$(qrexec_call "dom0" admin.vm.CreateDisposable)

	. "$(dirname "$0")/qubes.WinSign.common"
	. "$(dirname "$0")/qubesbuilder.WinSign.common"

	if ! OPTS=$(getopt -o hi:o: --long help,iso,output: -n "$0" -- "$@"); then
	if ! OPTS=$(getopt -o hi:o: --long help,iso:,output: -n "$0" -- "$@"); then

Add QWT support #181

Add QWT support #181

Conversation

omeg commented Feb 10, 2025 • edited by marmarek Loading

marmarek left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marmarek commented Feb 26, 2025

codecov bot commented Feb 27, 2025 • edited Loading

Codecov Report

marmarek commented Feb 27, 2025

tlaurion commented Feb 28, 2025 • edited Loading

marmarek commented Mar 4, 2025 • edited Loading

omeg commented Mar 4, 2025

marmarek left a 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

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

omeg commented Mar 5, 2025

omeg commented Mar 5, 2025

omeg commented Mar 5, 2025

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

omeg commented Feb 10, 2025 •

edited by marmarek

Loading

codecov bot commented Feb 27, 2025 •

edited

Loading

tlaurion commented Feb 28, 2025 •

edited

Loading

marmarek commented Mar 4, 2025 •

edited

Loading