device identity #614
device identity #614
Conversation
|
Can you drop commits adding gui, instead of adding+removing gui? the PR is quite big already... |
piotrbartman
force-pushed
the
qdev
branch
4 times, most recently
from
3d3a72a
to
65f14db
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #614 +/- ##
==========================================
+ Coverage 69.32% 69.39% +0.07%
==========================================
Files 58 58
Lines 11993 12388 +395
==========================================
+ Hits 8314 8597 +283
- Misses 3679 3791 +112
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
qubes/ext/utils.py
Outdated
| def confirm_device_attachment(device, frontends) -> str: | ||
| try: | ||
| # pylint: disable=consider-using-with | ||
| proc = subprocess.Popen( | ||
| ["attach-confirm", device.backend_domain.name, | ||
| device.port_id, device.description, | ||
| *[f.name for f in frontends.keys()]], | ||
| stdout=subprocess.PIPE, stderr=subprocess.PIPE | ||
| ) | ||
| (target_name, _) = proc.communicate() | ||
| return target_name.decode() | ||
| except Exception as exc: | ||
| print("attach-confirm", exc, file=sys.stderr) | ||
| return "" |
There was a problem hiding this comment.
There are a few problems with this function:
- It must be async, since it may block for some time, and it's not acceptable to block the whole qubesd for this time. In fact, attach-confirm probably won't work this way at all if it tries to talk to qubesd, since it's blocked on waiting for attach-confirm...
- subprocess.Popen -> asyncio variant
- The tool name is IMO too generic for a tool in a common /usr/bin/
- The tool belongs to desktop-linux-manager repo, which looks like a layering violation - dom0 code should also work without any of the GUI frontends installed in dom0.
- Extension of the above: this also will need adjustment to the GUI domain threat model: verify the response is one of allowed ones (on the
frontendslist?)
There was a problem hiding this comment.
I see the code of attach-confirm already is async. Maybe simply put its code here, instead of calling external program? It means you will need to keep the params dict format in sync, but changes there needs to be done in compatible way anyway (due to the GUI domain case, where both ends may be updated independently). Plus, you won't need to make external get_system_info() call, as by running inside qubesd you already have all the info here.
There was a problem hiding this comment.
- The tool belongs to desktop-linux-manager repo, which looks like a layering violation - dom0 code should also work without any of the GUI frontends installed in dom0.
I agree that this to some level introduces a dependency of the lower layer on the upper one, and that's why I decided to implement it as an independent program. If we can think of another way to confirm device attaching, it should be easy to implement. Currently, however, the GUI is required to implement ask-to-attach, and if the package is not available, such an assignment will simply be ignored. So the implementation is now: for ask-to-attach, ask the user for confirmation if possible, otherwise ignore the assignment.
There was a problem hiding this comment.
For sys-gui case, since this is socket protocol already, it's very easy to pipe it to a qrexec service. As said above, I'd prefer the interface between those two packages be "send this to a socket, read response in this format", instead of "call this executable". It will allow adding qrexec support without needing to still have the GUI-related package installed in dom0.
And yes, for now the approach "deny if confirmation not available" is fine.
There was a problem hiding this comment.
This is a fuller review of this part. I haven't done more testing since last time.
And also, it got few conflicts in the meantime...
There needs to be a documentation somewhere about API changes (compared to R4.2). It should include the following:
- API for device class extensions (like the one for mic or usb)
- Extension controlling Admin API access to devices (extension that handles
admin-permission:...events) - Usage of the client part (that's more relevant for the core-admin-client part)
- Changes to qrexec methods (in case somebody was using Admin API directly, or has alternative client implementation)
It doesn't need to be very detailed, more like a checklist of changes to look into when updating 3rd-party code interacting with devices.
qubes/device_protocol.py
Outdated
| f"when expected port: {expected.port_id}.") | ||
| properties.pop('port_id', None) | ||
|
|
||
| if expected.devclass == 'peripheral': |
There was a problem hiding this comment.
What is the use case for this special value? When denying attaching any device?
qubes/ext/block.py
Outdated
| self.notify_auto_attached(vm, device, options) | ||
| device = assignment.device | ||
| if assignment.mode.value == "ask-to-attach": | ||
| if vm.name != confirm_device_attachment(device, {vm: assignment}): |
There was a problem hiding this comment.
as noted before, this needs to be async
qubes/ext/utils.py
Outdated
| if len(frontends) > 1: | ||
| # unique | ||
| device = tuple(frontends.values())[0].device | ||
| target_name = confirm_device_attachment(device, frontends) |
qubes/api/admin.py
Outdated
There was a problem hiding this comment.
There should be also Admin API methods for reading/writing deny list. If nothing more fancy, then at least similar to qrexec policy ones that:
- Verify syntax when writing
- Allow race-free edits (read returns a "token" being hash of the file, and write gets the token to verify if nobody changed it in the meantime).
If you prefer to add it in a separate PR, convert this note to an issue.
we might need to ask to attach rest
|
As for the API changes, looks good. Should it be new file in Just minor thing:
|
| while not socked_call.done(): | ||
| await asyncio.sleep(0.1) |
There was a problem hiding this comment.
Why this loop? the await socked_call below should be enough, no?
There was a problem hiding this comment.
If we don't wait for the task to complete, using await in this case results in tasks being destroyed (including resolve_conflicts_and_attach) so attachment is skipped. I'm not entirely sure what causes this.
There was a problem hiding this comment.
Weird, I'll try to find out what is going on.
There was a problem hiding this comment.
I've tried and it works for me just fine without this part. I've tried by assigning block device with ask, and then connecting and disconnecting it - I got the prompt and when accepted the device got attached.
BTW, attaching device could use some log entry, especially when automatic or with ask (so you have some trail to which qube it got attached).
There was a problem hiding this comment.
Ok, I got once "Task was destroyed but it is pending". Skipping create task (ask_response = await call_socket_service(...))helped.
There was a problem hiding this comment.
in my case skipping creating task always ended up with Task was destroyed but it is pending
| "default_target": front_names[0] if number_of_targets == 1 else "", | ||
| "icons": { | ||
| ( | ||
| dom.name if dom.klass != "DispVM" else f"@dispvm:{dom.name}" |
There was a problem hiding this comment.
shouldn't that be getattr(dom, "template_for_dispvms", False) check instead? DispVM class is already running one, not a template to create new one from.
There was a problem hiding this comment.
Yes, it's for running DispVMs to indicate in the vm list which vms are disposible
doc/qubes-devices.rst
Outdated
| If a connected device has multiple assignments to different `frontend_domain` | ||
| instances, the user will be asked to choose which domain connect the device to. | ||
| If no GUI client is available, the device will not be connected to any domain. | ||
| beacon how to get mev |
There was a problem hiding this comment.
Cat on keyboard pressed "paste"? ;)
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2024111214-4.3&flavor=pull-requests Test run included the following:
New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2024091704-4.3&flavor=update
Failed tests63 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/112766#dependencies 201 fixed
Unstable tests
|
|
First start of sys-usb fails: Retrying sys-usb start works. This one is weird, but I think it's related to some caching, maybe on libvirt side? See the USB controllers were disconnected from dom0 just above, yet "usb_usb3" was still listed there. I think the cleanest way is to catch this exception in nodedev list
taken before starting sys-usb |
|
Looking at the traceback a bit closer, here:
it tries to get device for a specific assignment (so, it knows which device it wants). Yet it results in
... listing all of them. This sounds quite bad performance-wise (quadratic number of calls to whatever device backend it has) |
This is not entirely true. The property: |
|
Ok, so remaining issue is about the unit tests, for example: And there are also 2 minor pylint compains |
|
Please note that from our previous diagnosis, mypy complains with your fix |
|
Note to self: check what happens when starting VM with PCI device that got removed |
implements: QubesOS/qubes-issues/issues/9325