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

Support using UUIDs instead of VM names #574

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DemiMarie
Copy link
Contributor

@DemiMarie DemiMarie commented Jan 14, 2024

This supports using UUIDs (instead of VM names) in the Admin API. It also provides the UUIDs to qrexec-policy-daemon, which will use them to support the @uuid: keyword.

All admin APIs now support UUIDs as source and destination. Specific APIs changed include:

  • admin.vm.CreateDisposable: if the "uuid" argument is used, the VM UUID is returned instead of the name.
  • admin.vm.List: the UUID is included in the returned list.
  • internal.GetSystemInfo: the UUID is returned in the uuid key of each VM's JSON object.

Fixes: QubesOS/qubes-issues#8862

@DemiMarie DemiMarie force-pushed the vm-create-get-uuid branch 5 times, most recently from 29b2cfa to 21c1bf2 Compare January 14, 2024 18:33
Copy link

codecov bot commented Jan 14, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (203ee45) 68.39% compared to head (8e1b63a) 68.44%.

❗ Current head 8e1b63a differs from pull request most recent head 83816eb. Consider uploading reports for the commit 83816eb to get more accurate results

Files Patch % Lines
qubes/api/__init__.py 90.47% 2 Missing ⚠️
qubes/api/admin.py 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #574      +/-   ##
==========================================
+ Coverage   68.39%   68.44%   +0.05%     
==========================================
  Files          56       56              
  Lines       11159    11180      +21     
==========================================
+ Hits         7632     7652      +20     
- Misses       3527     3528       +1     
Flag Coverage Δ
unittests 68.44% <91.17%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@DemiMarie DemiMarie force-pushed the vm-create-get-uuid branch 2 times, most recently from ef50609 to d00e1a6 Compare January 14, 2024 20:57
qubes/api/__init__.py Outdated Show resolved Hide resolved
qubes/api/__init__.py Outdated Show resolved Hide resolved
self.enforce(not self.arg)
if untrusted_payload not in (b"", b"uuid"):
Copy link
Member

Choose a reason for hiding this comment

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

In the description you said it should be given in the argument, not the payload. So, which one is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Payload. The caller, not qrexec policy, gets to decide whether the name or UUID is returned to the caller.

qubes/tests/api_internal.py Outdated Show resolved Hide resolved
qubes/tests/api_internal.py Outdated Show resolved Hide resolved
Copy link
Member

@marmarek marmarek left a comment

Choose a reason for hiding this comment

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

looks okay now, but I'll merge it only together with other related PRs

@DemiMarie DemiMarie marked this pull request as draft February 10, 2024 20:20
@DemiMarie
Copy link
Contributor Author

looks okay now, but I'll merge it only together with other related PRs

That turned out to be a good idea, because the API isn’t sufficient for what qrexec needs. qrexec needs not only the UUID of the created VM, but also its Xen domain ID. This is needed to connect to the VM’s qrexec daemon in a race-free way. Marked as draft.

@marmarek
Copy link
Member

This is needed to connect to the VM’s qrexec daemon in a race-free way. Marked as draft.

The domain cannot change its name while running, this is non-issue. The race you consider would require a domain to be stopped, removed, re-created and started, all between qrexec-policy-daemon getting info from qubesd and calling qrexec-client (or otherwise connecting to it) which is very much unrealistic to happen. In fact, I'd strongly prefer to not return Xen ID in the system info structure, exactly because it changes on qube restart, which will make any kind of caching (if happen at some point) less effective.

What can be done at some point (not R4.2, as will likely have compatibility issues) is to create a uuid->xid symlink to qrexec socket (in addition to name->xid) and use that for connections.

@DemiMarie
Copy link
Contributor Author

This is needed to connect to the VM’s qrexec daemon in a race-free way. Marked as draft.

The domain cannot change its name while running, this is non-issue. The race you consider would require a domain to be stopped, removed, re-created and started, all between qrexec-policy-daemon getting info from qubesd and calling qrexec-client (or otherwise connecting to it) which is very much unrealistic to happen. In fact, I'd strongly prefer to not return Xen ID in the system info structure, exactly because it changes on qube restart, which will make any kind of caching (if happen at some point) less effective.

What can be done at some point (not R4.2, as will likely have compatibility issues) is to create a uuid->xid symlink to qrexec socket (in addition to name->xid) and use that for connections.

There should not be any compatibility issues. All that is needed is for qubesd to either create the symlink itself or tell qrexec-daemon to do so. That requires new command-line options, but package dependencies will guarantee that the new qrexec-daemon is installed before the new qubesd is. Existing daemons will work fine, and the new daemon can fall back to the name if the UUID symlink is missing. Yes, I do plan to test all of these scenarios.

@marmarek
Copy link
Member

Please don't. Qrexec is too critical part to risk it.

@DemiMarie
Copy link
Contributor Author

Please don't. Qrexec is too critical part to risk it.

Ack. This will still be useful for the builder, where the race window is much larger.

@DemiMarie DemiMarie marked this pull request as ready for review February 11, 2024 01:49
@DemiMarie
Copy link
Contributor Author

Please don't. Qrexec is too critical part to risk it.

Simpler solution:

  • Tell qrexec-daemon to create a symbolic link using the UUID. This information can be passed via an environment variable, which the current daemon ignores.
  • If connecting via the UUID fails, fall back to a name-based connection.
  • If the environment variable is set, qrexec-daemon uses the new admin.vm.CreateDisposable API, which gives UUID, name, and XID. The XID is used to make the connection.

@marmarek
Copy link
Member

I still don't like changes like this in a stable release. To be clear: backporting feature or an API change to a stable release should be considered exception not a rule. Based on risk-benefit analysis I do not grant exception for this case.
Using UUID-based symlink in R4.3 is fine. And at this point, I'd prefer it to be done explicitly (extra command line option with UUID?), not via a sneaky env variable. I'll add relevant package dependencies to ensure both sides are updated (on dist-upgrade).

This supports using UUIDs (instead of VM names) in the Admin API.  It
also provides the UUIDs to qrexec-policy-daemon, which will use them to
support uuid:<UUID> syntax as an alternative way of naming a VM.

All admin APIs now support UUIDs as source and destination.  Specific
APIs changed include:

- admin.vm.CreateDisposable: if the payload is "uuid" used, the VM UUID
  (with "uuid:" prefix) is returned instead of the name.
- admin.vm.List: the UUID is included in the returned list.
- internal.GetSystemInfo: the UUID is returned in the "uuid" key of each
  VM's JSON object.
- A new stubdom_uuid property contains the UUID of the stubdomain.

Fixes: QubesOS/qubes-issues#8862
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants