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.

this is done in the current code.

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.

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!

actually it caused a serious regression as it didn't detect removals, and a few other things.

src/usb.c

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

(I haven't addressed everything yet)

hfiguiere · 2024-10-30T02:58:55Z

The tests pass locally. Not sure yet what's happening.

swick · 2024-10-30T12:39:46Z

I think I saw a race in the test removing a device. Maybe you can force it by adding and then removing a bunch of devices and repeating the test.

swick · 2024-10-30T12:55:59Z

src/usb.c

@@ -1252,6 +1259,7 @@ handle_acquire_devices (XdpDbusUsb            *object,
 static gboolean
 handle_finish_acquire_devices (XdpDbusUsb            *object,
                               GDBusMethodInvocation *invocation,
+                               const char            *object_path,


Do we have a guarantee that this isn't the object path of another client?

I have added a check so that if it doesn't know about the object_path / token it will return an error.

hfiguiere · 2024-11-14T03:18:46Z

I think I saw a race in the test removing a device. Maybe you can force it by adding and then removing a bunch of devices and repeating the test.

It never receives the device event for removal. Still work locally. I can even receive it twice.

swick · 2024-11-15T13:50:51Z

FWIW, with podman run --rm -ti -v .:/portal:z ghcr.io/hfiguiere/xdg-desktop-portal:20241024-3 bash I can reproduce the failure locally.

swick · 2024-11-15T15:20:07Z

It never receives the device event for removal. Still work locally. I can even receive it twice.

In the current PR the signal is connected twice so I guess that's what you're seeing.

hfiguiere · 2024-11-15T15:29:24Z

there are a few commit for debug. Yes it's connected twice... that was to answer "does it get disconnected somehow?" and the answer is "not that problem". If can reproduce locally it will be easier. These last test commits will be stripped off.

swick · 2024-11-15T16:29:39Z

I tried intercepting the mocked uevents but when I replace the socket (mv "$sock" "$sock.original" && socat -t100 -v UNIX-LISTEN:$sock,mode=777,reuseaddr,fork UNIX-CONNECT:$sock.original) I get nothing and dbusmock seems to crash the pytest harness. This is really hard to debug...

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]>

Don't assert on is_udev_device_suitable() Signed-off-by: Hubert Figuière <[email protected]>

We want to test the USB portal which requires USB queries to determine which USB devices should be enumerable and could potentially be acquired. This adds an environment variable similar to the one for the app id that the test harness can set.

This lets us control which USB devices are enumerable by setting the fixture to valid xdp USB query.

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

This is because it fails as we never get the removal event. In both podman and docker Signed-off-by: Hubert Figuière <[email protected]>

hfiguiere · 2024-11-17T19:04:58Z

The test passes as I work around skipping in CI the one that fails.

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

hfiguiere force-pushed the usb-portal branch 2 times, most recently from fdf0334 to a9c5532 Compare October 30, 2024 02:20

swick reviewed Oct 30, 2024

View reviewed changes

hfiguiere force-pushed the usb-portal branch 4 times, most recently from 787cf12 to a48dc30 Compare November 1, 2024 02:50

hfiguiere force-pushed the usb-portal branch from a48dc30 to 7c61989 Compare November 14, 2024 02:28

hfiguiere force-pushed the usb-portal branch from 7bf79b8 to c6566cf Compare November 17, 2024 15:15

hfiguiere and others added 9 commits November 17, 2024 13:15

usb: If not sandboxed, assume all devices

9b8a6a8

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

usb: Allow multiple pending devices acquisitions

7c1aba8

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

usb: Remove the sender_state for device acquisition

df70309

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

usb: Fix device insertion / removal handled

68ba73b

Don't assert on is_udev_device_suitable() Signed-off-by: Hubert Figuière <[email protected]>

tests/py: Allow setting usb_queries for AppInfoTest via fixture

7408866

This lets us control which USB devices are enumerable by setting the fixture to valid xdp USB query.

tests/usb: Test the USB portal via pytest, dbusmock and umockdev

7289c68

usb: Check the token for AcquireFinish is valid

b70b738

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

hfiguiere force-pushed the usb-portal branch 2 times, most recently from e3984d7 to bc4aced Compare November 17, 2024 18:49

test: Disable USB removal test in a container

d2e1553

This is because it fails as we never get the removal event. In both podman and docker Signed-off-by: Hubert Figuière <[email protected]>

hfiguiere force-pushed the usb-portal branch from bc4aced to d2e1553 Compare November 17, 2024 18:56


		* ``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

Choose a reason for hiding this comment

hfiguiere commented Sep 21, 2024

hfiguiere commented Oct 30, 2024

swick commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hfiguiere commented Nov 14, 2024

swick commented Nov 15, 2024 • edited Loading

swick commented Nov 15, 2024

hfiguiere commented Nov 15, 2024

swick commented Nov 15, 2024 • edited Loading

hfiguiere commented Nov 17, 2024

hfiguiere commented Apr 30, 2024 •

edited

Loading

swick left a comment •

edited

Loading

swick commented Nov 15, 2024 •

edited

Loading

swick commented Nov 15, 2024 •

edited

Loading