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

Merged
merged 1 commit into from
Oct 10, 2024

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: Patch coverage is 75.55556% with 11 lines in your changes missing coverage. Please review.

Project coverage is 69.36%. Comparing base (488752c) to head (8039442).
Report is 69 commits behind head on main.

Files with missing lines Patch % Lines
qubes/vm/qubesvm.py 27.27% 8 Missing ⚠️
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   69.35%   69.36%   +0.01%     
==========================================
  Files          58       58              
  Lines       11919    11949      +30     
==========================================
+ Hits         8266     8289      +23     
- Misses       3653     3660       +7     
Flag Coverage Δ
unittests 69.36% <75.55%> (+0.01%) ⬆️

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).

@DemiMarie DemiMarie force-pushed the vm-create-get-uuid branch 2 times, most recently from 3dd1045 to 8315e54 Compare April 19, 2024 22:04
@marmarek
Copy link
Member

This got some conflicts too, and is needed for QubesOS/qubes-core-qrexec#135

@marmarek
Copy link
Member

Test fails. And also pylint complains about too long lines (others look as false positives)

@marmarek marmarek merged commit e6d1b10 into QubesOS:main Oct 10, 2024
4 checks passed
@DemiMarie DemiMarie deleted the vm-create-get-uuid branch October 15, 2024 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support UUIDs in the Admin API
3 participants