USB portal (cont.) #1354

hfiguiere · 2024-04-30T02:32:36Z

This supercede #1238

Close #1238

hfiguiere · 2024-08-15T02:33:44Z

After a painful rebasing.

hfiguiere · 2024-09-14T03:06:35Z

Feature is linked to flatpak/flatpak#5620

I think this is ready for review.

swick

Checked the bits outside of usb.c so far.

src/xdp-app-info-flatpak.c

src/xdp-utils.h

swick · 2024-09-17T10:59:51Z

src/xdp-app-info-flatpak.c

+
+      enumerable_devices = g_key_file_get_string_list (app_info_flatpak->flatpak_info,
+                                                       "USB Devices",
+                                                       "allowed-devices",


enumerable-devices/hidden-devices

Might want to use defines for those constants.

fixed the literal.

src/xdg-desktop-portal.c

src/xdp-utils.c

swick

Bunch of comments on the interface definition. Mostly about documentation but I'm a bit worried about what device removal means and how we should deal with udev properties.

data/org.freedesktop.portal.Usb.xml

swick · 2024-09-17T12:21:46Z

data/org.freedesktop.portal.Usb.xml

+
+        * ``properties`` (``a{sv}``)
+
+          A list of udev properties that this device has. These properties


I'm really worried about this. It essentially makes udev a stable API even though it really is not.

swick · 2024-09-17T12:25:16Z

data/org.freedesktop.portal.Usb.xml

+        This method can only be called once, and only after calling
+        org.freedesktop.portal.Usb.AcquireDevices().


Should mention the #org.freedesktop.portal.Request::Response signal

I think the response signal should carry an id that should be passed to FinishAcquireDevices. That way there is no problem with calling AcquireDevices multiple times before calling FinishAcquireDevices.

swick · 2024-09-17T12:26:38Z

data/org.freedesktop.portal.Usb.xml

+
+        There are no supported keys in the @options vardict.
+     -->
+    <method name="ReleaseDevices">


Should probably say something about the relation to the session and connection. I assume closing the session or connection implicitly also releases all devices?

Eh, there is no session involved with acquiring and releasing devices, only a connection. The question still remains: what happens when the connection is closed. Same as release? Nothing?

when the dbus connection is closed all the devices are removed (ie released)

I think my suggestion on FinishAcquireDevices would resolve this thread.

swick · 2024-09-17T12:28:15Z

data/org.freedesktop.portal.Usb.xml

+
+        * ``action`` (``s``)
+
+          Type of event that occurred. One of "add", "change", or "remove".


What happens when one has acquired a device and it got removed? Is the fd still usable? Only sometimes?

We don't have a mechanism for revoking the fd so a real device unplug will result in a remove with the fd becoming unusable but when the user wants to revoke a device, we can't do anything with the existing fd on the client and so it won't be.

I think my suggestion on FinishAcquireDevices would resolve this thread as well.

swick · 2024-09-17T12:29:00Z

data/org.freedesktop.portal.Usb.xml

+        @devices: Array of device identifiers
+        @options: Vardict with optional further information
+
+        Releases previously acquired devices.


What does it mean to release a device? Is the fd still usable?

It will close the file descriptor when we release the "owned device" object. I haven't checked how libusb, for example, reacts to this.

Well, it closes the fd on the portal side which is good because we need to do that to not leak things, but it doesn't close the fd on the client side. In fact, you cannot close the fd on the client side from the portal side. The only thing we can do is revoke file descriptors but that currently only works for evdev and hidraw and not for usb. So as long as we don't get kernel support for that, clients will be able to continue to use the fd that they acquired even if the connection is closed. We should however say something in the interface description here which will allow us to revoke those USB devices if the connection gets closed, or if the user wants to revoke access manually, when we get USB revoke from the kernel.

swick

Looked through the usb.c file now. Mostly nickpicks but also a few actual issues we have to work out.

src/usb.c

swick · 2024-09-18T14:15:43Z

src/usb.c

+      g_dbus_method_invocation_return_error (invocation,
+                                             XDG_DESKTOP_PORTAL_ERROR,
+                                             XDG_DESKTOP_PORTAL_ERROR_FAILED,
+                                             "Cannot call AcquireDevices() with an unfinished "


Why not? The interface description doesn't mention this restriction at all.

The changes I proposed on AcquireDevices would resolve this thread.

src/usb.c

swick · 2024-09-18T14:25:15Z

data/org.freedesktop.portal.Usb.xml

+        This method can only be called once, and only after calling
+        org.freedesktop.portal.Usb.AcquireDevices().


I think the response signal should carry an id that should be passed to FinishAcquireDevices. That way there is no problem with calling AcquireDevices multiple times before calling FinishAcquireDevices.

swick · 2024-09-18T14:35:32Z

src/usb.c

+      owned_device = g_hash_table_lookup (sender_info->owned_devices, access_data->device_id);
+      if (!owned_device)
+        {
+          fd = open (device_file, access_data->writable ? O_RDWR : O_RDONLY);


Why would opening the file fail? Is udev already telling us that we should be able to open the device node (i.e. we have permission to do it) or is do we discover about bad permissions just here?

many reason. It's IO nothing is guaranteed. The device could have disappear, could be unitialized. If it can go wrong it will go wrong.

Right, but that means we'll show USB devices to the portal clients, letting users go through the UI to approve access, just to then eventually notice that we actually can't make the device available. Would be much nicer if we didn't advertise those devices in the first place.

there is not a lot we can do. We can call access on the device is_gudev_device_suitable() and return FALSE if the call fail. That will check for existance and read permission. At that leve we don't know if we want the write permissions.

And we already check in handle_acquire_devices() as well.

Still a lot of race condition can occur it acquiring the device.

Yes, obviously there can always be cases where opening will fail because this is inherently racy, but why would we want to advertise devices which we cannot open because we have no read permission? The code right now figures out that it won't be able to open it, advertises it anyway and then fails when a client tries to acquire it.

we no longer do. the change just hadn't been pushed yet.

Maybe I'm blind but I can't seem to find it? Are you sure you pushed the latest version now?

https://github.com/flatpak/xdg-desktop-portal/pull/1354/files#diff-4ce7d34e66e5ca9b52f8e5666f0d4efe11e48278f34a7cdba6df390c14012a27R163

Which mean if it can't access it it won't list it. So open won't be attempted. In case of race condition, ie the device is no longer accessible between the list and this, then this will error normally.

Ah, I missed that one. Perfect!

src/usb.c

hfiguiere · 2024-09-21T01:05:30Z

(I haven't addressed everything yet)

src/xdp-app-info-host.c

swick · 2024-10-11T16:07:40Z

data/org.freedesktop.portal.Usb.xml

+
+          Device ID of the parent device.
+
+        * ``readable`` (``b``)


What use is a device which can not be read from? AcquireDevices also doesn't have a readable only a writable. Pretty sure this should just be removed.

Until very recently it would have listed the devices even if it couldn't access them.

See #1354 (comment)

It is devices that can be enumerated after all.

data/org.freedesktop.portal.Usb.xml

swick · 2024-10-15T17:30:41Z

Pushed a branch with a bunch of cleanups and changes I'd like to see https://github.com/swick/xdg-desktop-portal/commits/wip/usb-portal-fixes1/

But in general this LGTM. The biggest issue is the lack of tests IMO. Some basic python tests using umockdev would be great but I'd also like to see this merged rather sooner than later. Do you have time to do that work?

swick · 2024-10-16T23:55:47Z

I started working on python tests for this portal here: https://github.com/swick/xdg-desktop-portal/commits/wip/usb-portal-tests/

It's based on the PR which tests the location portal with the python tests because it already has a bunch of improvements.

Found a few bugs in the impl already and the branch contains fixes.

It's using umockdev to create the testbed and it succeeds in hiding the host devices but no success so far with making devices visible from the test itself.

swick · 2024-10-17T12:50:01Z

Some prepwork for the testing is up in a PR: #1477

Also update the branch https://github.com/swick/xdg-desktop-portal/commits/wip/usb-portal-tests/ which now has passing tests with some mocked devices. It could use some more tests and it still contains fixes that this PR doesn't have.

The USB portal is the middleman between sandboxed apps, and the devices connected and available to the host system. This is the first version of the portal. Device filtering ================ Sandboxed apps must declare which USB devices they support ahead of time. This information is read by the XDG Desktop Portal and used to determine which USB devices will be exposed to requesting apps. On Flatpak, these enumerable and hidden devices are set by the "--usb" and "--nousb" arguments against "flatpak build-finish" and "flatpak run". Neither "--devices=all" nor "--device=usb" do influence the portal. Hidding a device always take precedence over making them enumerable, even when a blanket permission ("--usb=all") is set. Individual devices are assigned a unique identifier by the portal, which is used for all further interactions. This unique identifier is completely random and independent of the device. Permission checks are in place to not allow apps to try and guess device ids without having permission to access then. Permissions =========== There are 2 dynamic permissions managed by the USB portal in the permission store: 1. Blanket USB permission: per-app permission to use any methods of the USB portal. Without this permission, apps must not be able to do anything - enumerate, monitor, or acquire - with the USB portal. [1] 2. Specific device permission: per-app permission to acquire a specific USB device, down to the serial number. Enumerating devices =================== There are 2 ways for apps to learn about devices: - Apps can call the EnumerateDevices() method, which gives a snapshot of the current devices to the app. - Apps can create a device monitoring session with CreateSession() which sends the list of available devices on creation, and also notifies the app about connected and disconnected devices. Only devices that the app is allowed to see are reported in both cases. The udev properties exposed by device enumeration is limited to a well known subset of properties. [2] Device acquisition & release ============================ Once an app has determined which devices it wants to access, the app can call the AcquireDevices() method. This method may prompt a dialog for the user to allow or deny the app from accessing specific devices. If permission is granted, XDG Desktop Portal tries to open the device file on the behalf of the requesting app, and pass down the file descriptor to that file. [3] --- [1] Exceptionally, apps can release previously acquired devices, even when this permission is disabled. This is so because we don't yet have kernel-sided USB revoking. With USB revoking in place, it would be possible to hard-cut app access right when the app permission changes. [2] This patch uses a hardcoded list. There is no mechanism for apps to influence which other udev properties are fetched. This approach is open to suggestions - it may be necessary to expose more information more liberally through the portal. [3] This is clearly not ideal. The ideal approach is to go through logind's TakeDevice() method. However, that will add significant complexity to the portal, since this logind method can only be called by the session controller (i.e. the only executable capable of calling TakeControl() in the session - usually the compositor). This can and probably should be implemented in a subsequent round of improvements to the USB portal. Co-Authored By: Georges Basile Stavracas Neto <[email protected]> Co-Authored-By: Ryan Gonzalez <[email protected]> Signed-off-by: Hubert Figuière <[email protected]>

Signed-off-by: Hubert Figuière <[email protected]>

hfiguiere force-pushed the usb-portal branch 2 times, most recently from cb66d92 to f28ffdf Compare May 6, 2024 02:29

hfiguiere force-pushed the usb-portal branch 3 times, most recently from ce2b214 to 4907506 Compare May 20, 2024 00:51

hfiguiere force-pushed the usb-portal branch from bf64980 to 393f863 Compare June 22, 2024 01:43

hfiguiere force-pushed the usb-portal branch from 393f863 to 74f9508 Compare August 15, 2024 02:33

hfiguiere self-assigned this Sep 5, 2024

hfiguiere force-pushed the usb-portal branch from 74f9508 to 761f0a9 Compare September 14, 2024 03:05

hfiguiere marked this pull request as ready for review September 14, 2024 03:05

hfiguiere force-pushed the usb-portal branch 2 times, most recently from 69843ce to 4dcfc94 Compare September 14, 2024 18:35

swick reviewed Sep 17, 2024

View reviewed changes

swick reviewed Sep 18, 2024

View reviewed changes

hfiguiere force-pushed the usb-portal branch 2 times, most recently from a258338 to b40ccee Compare September 21, 2024 00:12

hfiguiere force-pushed the usb-portal branch 7 times, most recently from bdaa78e to 27f10e2 Compare September 26, 2024 03:40

hfiguiere force-pushed the usb-portal branch 3 times, most recently from a8ce8e9 to a3123a1 Compare October 9, 2024 01:47

swick mentioned this pull request Oct 9, 2024

USB portal #1238

Closed

swick reviewed Oct 11, 2024

View reviewed changes

src/xdp-app-info-host.c Outdated Show resolved Hide resolved

swick reviewed Oct 11, 2024

View reviewed changes

data/org.freedesktop.portal.Usb.xml Outdated Show resolved Hide resolved

swick reviewed Oct 11, 2024

View reviewed changes

data/org.freedesktop.portal.Usb.xml Outdated Show resolved Hide resolved

swick reviewed Oct 11, 2024

View reviewed changes

data/org.freedesktop.portal.Usb.xml Outdated Show resolved Hide resolved

hfiguiere force-pushed the usb-portal branch 3 times, most recently from 5dfe11a to 3cb7179 Compare October 13, 2024 03:11

hfiguiere force-pushed the usb-portal branch 2 times, most recently from adf2665 to 75e2525 Compare October 16, 2024 03:07

hfiguiere force-pushed the usb-portal branch from 75e2525 to 33e07bc Compare October 18, 2024 00:04

hfiguiere added 2 commits October 19, 2024 20:01

usb: If not sandboxed, assume all devices

f8039a0

Signed-off-by: Hubert Figuière <[email protected]>

hfiguiere force-pushed the usb-portal branch from 33e07bc to f8039a0 Compare October 20, 2024 00:02


		* ``properties`` (``a{sv}``)

		A list of udev properties that this device has. These properties

		This method can only be called once, and only after calling
		org.freedesktop.portal.Usb.AcquireDevices().


		* ``action`` (``s``)

		Type of event that occurred. One of "add", "change", or "remove".

USB portal (cont.) #1354

Are you sure you want to change the base?

USB portal (cont.) #1354

Conversation

hfiguiere commented Apr 30, 2024 • edited Loading

hfiguiere commented Aug 15, 2024

hfiguiere commented Sep 14, 2024

swick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swick 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

swick left a comment • edited Loading

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

hfiguiere commented Sep 21, 2024

Choose a reason for hiding this comment

hfiguiere Oct 13, 2024 • edited Loading

Choose a reason for hiding this comment

swick commented Oct 15, 2024

swick commented Oct 16, 2024

swick commented Oct 17, 2024

hfiguiere commented Apr 30, 2024 •

edited

Loading

swick left a comment •

edited

Loading

hfiguiere Oct 13, 2024 •

edited

Loading