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
base: main
Are you sure you want to change the base?
Conversation
29b2cfa
to
21c1bf2
Compare
Codecov ReportAttention:
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ef50609
to
d00e1a6
Compare
self.enforce(not self.arg) | ||
if untrusted_payload not in (b"", b"uuid"): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
0f5ef5a
to
86bfa69
Compare
86bfa69
to
8e1b63a
Compare
There was a problem hiding this 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
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. |
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 |
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. |
Simpler solution:
|
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. |
8e1b63a
to
83816eb
Compare
83816eb
to
3dd1045
Compare
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
3dd1045
to
8315e54
Compare
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 theuuid
key of each VM's JSON object.Fixes: QubesOS/qubes-issues#8862