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

[WIP] Add new portal for USB device access #559

Closed
wants to merge 1 commit into from

Conversation

refi64
Copy link
Contributor

@refi64 refi64 commented Jan 25, 2021

Related: flatpak/flatpak#4083

Core design notes

  • This is very closely inspired by Android's UsbManager API.
  • Dynamic permissions are used to control opening a device for RW access.
  • Individual objects weren't represented as devices, because from some basic experiments it basically made everything more complicated for very little gain.
  • Apps with --device=all don't need dynamic permissions, because they already have full access to everything.
    • Snaps are presumed to never have full access, because from what I can tell, they don't give enough information to the portal to determine this (also I have no idea how any of that works there).

Remaining questions / issues

  • The main issue is that this hasn't yet been fully integrated with Chromium, so there are likely e.g. properties it needs that aren't currently exposed.
  • Read vs read/write is currently not reported to the user. I'm not really sure how to handle this properly: opening game controllers in RO vs RW is mostly insignificant, since all the latter is used for afaik is primarily effects like rumble. However, on a USB stick? Completely different story.
  • Should there be an API to check if an app already has permission to a device? This would be a bit different stylistically than the other portals, however...
  • Should we even require dynamic permissions for joysticks? Afaik no browsers really guard the web gamepad APIs by permission checks, simply because the scope of damage is pretty limited...
  • Android Q+ guards accessing the device serial behind having dynamic permissions, since they're pretty unique and could in theory be used for fingerprinting.
  • Android P+ guards opening camera / video devices behind the corresponding separate permissions. (Part of the reason I didn't really play with this much is because I, err, don't have any USB camera / video devices.)
  • Related to the above, I've mostly just tested this with a Stadia controller and a USB DAC.

I have a test script that will be cleaned up and posted at some point. This could in theory also be auto tested using something like https://github.com/martinpitt/umockdev but setting all of that up is probably also a bit out of scope!

@refi64
Copy link
Contributor Author

refi64 commented Jan 25, 2021

(CI failures seem to just be that libudev is missing, will update tomorrow)

@hadess
Copy link
Contributor

hadess commented Jan 25, 2021

Quick drive-by comments:

  • use libgudev, not libudev, it has niceties that make it more usable in GObject projects
  • the "devices" section from the Flatpak manifest needs to have a specific USB section, not to be bunched up with other types of devices
  • what devices this gives access to looks weird to me. I don't think it should access anything outside /dev/bus/usb/. I don't think it should give access to USB serial ports, or USB HID device nodes, or USB anything else. We should have separate APIs for that, otherwise it just muddies things (eg. it makes no sense to support USB HID device nodes and not Bluetooth HID device nodes, ditto for serial ports, etc.)
  • the "read" vs. "read/write" modes aren't useful. I don't even know if it's enforceable at the kernel level.
  • the properties from the EnumerateDevices API should just be the udev properties, stuff like has_joystick really isn't extensible
  • I don't think we should have a separate "RequestPermission" API, the permission can be requested by the portal, if needed, when you open the device
  • I don't think that anything but "raw" device API should be considered, so I don't think it makes sense to make joysticks special.

To see if this API is viable, I'd really like to see an attempt at porting a libusb application to this. The API should be usable in single-threaded mode, so we could probably have a "Flatpak" backend where enumeration, hotplug, and opening device would go through the portal automatically.

We're still missing revoke support in the kernel, as I mentionedi in the original issue, so that the portal can't yank access to an application that's opened a device, which is a potential security issue, and we don't have a way to request access to USB devices for which we don't already have access. For example, accessing an Arduino in programming mode still would require running a bunch of commands as root/install udev rules.

@refi64
Copy link
Contributor Author

refi64 commented Jan 26, 2021

use libgudev, not libudev, it has niceties that make it more usable in GObject projects

I had looked into it, but at a glance for this usage, it mostly provided stuff like ootb autoptr support and slightly cleaner attribute parsing, which is nice but would really shave off at most 10 lines or so and didn't really seem worth adding a dependency for.

That being said, apparently a lot of stuff already requires gudev:

$ rpm --test -e libgudev 2>&1 | rg 'needed by' | wc -l
21

so it's probably more prevalent than I initially realized. This probably wouldn't be too hard to change out.

the "devices" section from the Flatpak manifest needs to have a specific USB section, not to be bunched up with other types of devices

I'm not really sure what this is referring to here, is it in the Flatpak PR? I haven't updated any of the docs there yet.

what devices this gives access to looks weird to me. I don't think it should access anything outside /dev/bus/usb/.
I don't think it should give access to USB serial ports, or USB HID device nodes, or USB anything else.

All the mentioned device nodes are in /dev/bus/usb, though (the code already determines if it's a USB device by checking ID_BUS). Did you mean it shouldn't access anything where the subsystem isn't USB?

We should have separate APIs for that, otherwise it just muddies things (eg. it makes no sense to support USB HID device nodes and not Bluetooth HID device nodes, ditto for serial ports, etc.)

I don't think trying to exclude certain subsystems would help much, though. The only real way (from what I can tell) to identity USB devices outside of the bus is the subsystem like I mentioned above. However, there are already some miscellaneous devices attached to this subsystem, this is what I find in /dev/bus/usb without actually having anything plugged in:

  • Two root hubs.
  • My front facing camera.
  • My bluetooth controller.

None of the udev attributes available on it would indicate what types of devices these are; I only found out by reading the vendor strings. Thus, regardless, we're going to end up with devices that have alternative APIs also exposed over USB without a clear way of blocking them.

It's also worth noting that Android intentionally leaves HID devices available through UsbManager. If an application wants to communicate with the device in a more low level way that's USB-specific, that functionality is still available. For instance, the Stadia app uses UsbManager to communicate with the controller and set its current charging acceptance state. This is a strictly USB-only operation, so it makes sense that it's through a USB portal vs an HID portal, but it's still being performed on an HID device.

the "read" vs. "read/write" modes aren't useful. I don't even know if it's enforceable at the kernel level.

I will admit, this is more just so that it's easier to open the device nodes and not related to permissions. Since they may be ro, we can't just unconditionally open them as rw when the application calls OpenDevice, and we can't just always open them as ro since the application may need to write to it (i.e. to send force feedback data). By making the application request ro vs rw, we can easily fail out early on if write access was in fact needed, while not having to worry about it if the application only needs read access.

I'm open to ideas on how to clean it up!

the properties from the EnumerateDevices API should just be the udev properties, stuff like has_joystick really isn't extensible

My personal concern with using udev properties is that they're a bit of a wild west (over 130 ID_* attributes are referenced in my installed set of udev rules), so we risk accidentally revealing some information that could be used to identify more than we want without the app having permission first.

Stuff like has_joystick is definitely a bit weird, but it's mostly just there since afaik there isn't really a way of distinguishing between different types of USB input devices without the weird ID_INPUT_JOYSTICK property.

I don't think we should have a separate "RequestPermission" API, the permission can be requested by the portal, if needed, when you open the device

One reason for this distinction is partly what I mentioned above:

Android Q+ guards accessing the device serial behind having dynamic permissions, since they're pretty unique and could in theory be used for fingerprinting.

If this indeed ends up being guarded behind having permissions, the application would need to call OpenDevice if it needs the serial, which would be a bit bizarre. Now, we could have OpenDevice simply always show a permission dialog in addition to RequestPermission, but depending on how USB management requests get handled (see below), we'd end up with other calls that all also can show permission dialogs in addition to RequestPermission, which seemed a bit odd API-wise?

Now that I think about it, though, it's a bit odd that requesting permission to read/write to a device would be required to request access to the serial. Maybe RequestPermission could have a set of different permission options like open vs info (the ability to request potentially fingerprint-able information from the device)? Although, I'm not sure the resulting dialogs would really be able to contain actionable information for the user...

To see if this API is viable, I'd really like to see an attempt at porting a libusb application to this. The API should be usable in single-threaded mode, so we could probably have a "Flatpak" backend where enumeration, hotplug, and opening device would go through the portal automatically.

This is definitely also in the plans, along with the demos I mentioned before!

We're still missing revoke support in the kernel, as I mentioned in the original issue, so that the portal can't yank access to an application that's opened a device, which is a potential security issue

This is probably terrible, but...could this, in theory, be handled by holding onto the fd in the portal, and calling shutdown on it if permission is revoked? This would have the issue that the portal dying would result in losing control over the fd, however.

and we don't have a way to request access to USB devices for which we don't already have access. For example, accessing an Arduino in programming mode still would require running a bunch of commands as root/install udev rules.

This is something I actually forgot to mention above. It's definitely a bit of an issue; I guess in theory, we could of course just use polkit to have some system service hand access to the device, but the portal of course doesn't have any communication set up for that right now.

This also comes back to the other thing that I completely forgot to mention previously, which is that the usbdevfs ioctls aren't supported yet, and those are definitely something that's needed for full-fledged device support (e.g. the HID use case I mentioned above). Some of them needed some careful treading, which is why I wanted to work on and propose the core permissions system first.

(Side note: this is certainly a lot more written out than I anticipated, I should probably have gone into some of these design ideas in the initial post first.)

@hadess
Copy link
Contributor

hadess commented Jan 29, 2021

the "devices" section from the Flatpak manifest needs to have a specific USB section, not to be bunched up with other types of devices

I'm not really sure what this is referring to here, is it in the Flatpak PR? I haven't updated any of the docs there yet.

I meant here. I was under the impression that it just added more items that would ultimately end up in the devices= line in the Flatpak manifest. Looks like it doesn't, so I'm good :)

what devices this gives access to looks weird to me. I don't think it should access anything outside /dev/bus/usb/.
I don't think it should give access to USB serial ports, or USB HID device nodes, or USB anything else.

All the mentioned device nodes are in /dev/bus/usb, though (the code already determines if it's a USB device by checking ID_BUS). Did you mean it shouldn't access anything where the subsystem isn't USB?

No. I meant that it should be allowed to accessing the device node for the special driver for the device, just the raw USB device.

Eg. I don't think that it should be able to access the dev input node for a USB mouse or keyboard, or the v4l device node for a webcam, etc.

P: /devices/pci0000:00/0000:00:14.0/usb1/1-1/1-1.6/1-1.6:1.0/0003:0596:0524.0004/input/input29/event3
N: input/event3
L: 0
S: input/by-path/pci-0000:00:14.0-usb-0:1.6:1.0-event-mouse
S: input/by-id/usb-3M_3M_MicroTouch_USB_controller-event-mouse
E: DEVPATH=/devices/pci0000:00/0000:00:14.0/usb1/1-1/1-1.6/1-1.6:1.0/0003:0596:0524.0004/input/input29/event3
E: SUBSYSTEM=input
E: DEVNAME=/dev/input/event3
E: MAJOR=13
E: MINOR=67
E: USEC_INITIALIZED=3648947
E: ID_INPUT=1
E: ID_INPUT_MOUSE=1
E: ID_VENDOR=3M
E: ID_VENDOR_ID=0596
E: ID_MODEL=3M_MicroTouch_USB_controller
E: ID_MODEL_ID=0524
E: ID_REVISION=0106
E: ID_SERIAL=3M_3M_MicroTouch_USB_controller
E: ID_TYPE=hid
E: ID_BUS=usb
E: ID_USB_INTERFACES=:030000:
E: ID_USB_INTERFACE_NUM=00
E: ID_USB_DRIVER=usbhid
E: ID_PATH=pci-0000:00:14.0-usb-0:1.6:1.0
E: ID_PATH_TAG=pci-0000_00_14_0-usb-0_1_6_1_0
E: LIBINPUT_DEVICE_GROUP=3/596/524:usb-0000:00:14.0-1
E: DEVLINKS=/dev/input/by-path/pci-0000:00:14.0-usb-0:1.6:1.0-event-mouse /dev/input/by-id/usb-3M_3M_MicroTouch_USB_controller-event-mouse

Which is why I would restrict exporting to devices under /dev/bus/usb

We should have separate APIs for that, otherwise it just muddies things (eg. it makes no sense to support USB HID device nodes and not Bluetooth HID device nodes, ditto for serial ports, etc.)

I don't think trying to exclude certain subsystems would help much, though. The only real way (from what I can tell) to identity USB devices outside of the bus is the subsystem like I mentioned above. However, there are already some miscellaneous devices attached to this subsystem, this is what I find in /dev/bus/usb without actually having anything plugged in:

* Two root hubs.

* My front facing camera.

* My bluetooth controller.

None of the udev attributes available on it would indicate what types of devices these are; I only found out by reading the vendor strings. Thus, regardless, we're going to end up with devices that have alternative APIs also exposed over USB without a clear way of blocking them.

It's also worth noting that Android intentionally leaves HID devices available through UsbManager. If an application wants to communicate with the device in a more low level way that's USB-specific, that functionality is still available. For instance, the Stadia app uses UsbManager to communicate with the controller and set its current charging acceptance state. This is a strictly USB-only operation, so it makes sense that it's through a USB portal vs an HID portal, but it's still being performed on an HID device.

I really think that should be something separate. There's a separate discussion about HID devices in this same repo. Note that in the Stadia controller's case, I'm pretty sure it talks to /dev/hiddev* or /dev/hidraw* devices, not directly to the raw USB device.

I think this particular use case would be better served through:
#536

the "read" vs. "read/write" modes aren't useful. I don't even know if it's enforceable at the kernel level.

I will admit, this is more just so that it's easier to open the device nodes and not related to permissions. Since they may be ro, we can't just unconditionally open them as rw when the application calls OpenDevice, and we can't just always open them as ro since the application may need to write to it (i.e. to send force feedback data). By making the application request ro vs rw, we can easily fail out early on if write access was in fact needed, while not having to worry about it if the application only needs read access.

This comes back to the original problem. The API seems to have been designed to access any device node that's associated with a USB device. I think it should only be a proxy for libusb/usbdevfs access to devices, and for nothing else. If we want other devices, we can always extend it. But restricting it to that API means that you can't actually open devices read-only.

the properties from the EnumerateDevices API should just be the udev properties, stuff like has_joystick really isn't extensible

My personal concern with using udev properties is that they're a bit of a wild west (over 130 ID_* attributes are referenced in my installed set of udev rules), so we risk accidentally revealing some information that could be used to identify more than we want without the app having permission first.

I certainly think that blocklists and allowlists of, well, blocked and allowed properties would be better than using single item properties. Use an a{sv} and list the more common properties in the docs. The ID_* attributes are all udev API, and they're not going to be changed that easily.

The serial of the device will also be accessible to the app once it opens the device, and could be shown/used in some way in the portal permission dialogue. I could very well imagine a Flatpak announcing in its manifest that it supports all the devices from Apple in a certain PID range, and leaving it to the end-user to select the device to open in a portal dialogue. Something closer to a "File open" dialogue where you filter a type of file, rather than getting access to a full file list ahead of time.

Stuff like has_joystick is definitely a bit weird, but it's mostly just there since afaik there isn't really a way of distinguishing between different types of USB input devices without the weird ID_INPUT_JOYSTICK property.

See above, I don't think that this should be here.

I don't think we should have a separate "RequestPermission" API, the permission can be requested by the portal, if needed, when you open the device

One reason for this distinction is partly what I mentioned above:

Android Q+ guards accessing the device serial behind having dynamic permissions, since they're pretty unique and could in theory be used for fingerprinting.

I meant that there shouldn't be a separate RequestPermission API. For each of "enumerating devices" and "opening devices", I think that the portal should be the one asking the user for permission (through a shell dialogue, for GNOME at least).

If this indeed ends up being guarded behind having permissions, the application would need to call OpenDevice if it needs the serial, which would be a bit bizarre. Now, we could have OpenDevice simply always show a permission dialog in addition to RequestPermission, but depending on how USB management requests get handled (see below), we'd end up with other calls that all also can show permission dialogs in addition to RequestPermission, which seemed a bit odd API-wise?

Now that I think about it, though, it's a bit odd that requesting permission to read/write to a device would be required to request access to the serial. Maybe RequestPermission could have a set of different permission options like open vs info (the ability to request potentially fingerprint-able information from the device)? Although, I'm not sure the resulting dialogs would really be able to contain actionable information for the user...

To see if this API is viable, I'd really like to see an attempt at porting a libusb application to this. The API should be usable in single-threaded mode, so we could probably have a "Flatpak" backend where enumeration, hotplug, and opening device would go through the portal automatically.

This is definitely also in the plans, along with the demos I mentioned before!

We're still missing revoke support in the kernel, as I mentioned in the original issue, so that the portal can't yank access to an application that's opened a device, which is a potential security issue

This is probably terrible, but...could this, in theory, be handled by holding onto the fd in the portal, and calling shutdown on it if permission is revoked? This would have the issue that the portal dying would result in losing control over the fd, however.

I'm pretty certain that won't work just as it doesn't work for input devices, which would be why there's an evdev revoke API. Happy to be proven wrong.

and we don't have a way to request access to USB devices for which we don't already have access. For example, accessing an Arduino in programming mode still would require running a bunch of commands as root/install udev rules.

This is something I actually forgot to mention above. It's definitely a bit of an issue; I guess in theory, we could of course just use polkit to have some system service hand access to the device, but the portal of course doesn't have any communication set up for that right now.

My problem here is that we need to figure out how this could work in a way that doesn't make a popup show up to escape the sandbox, and another popup show up to access the device node. I think we need to figure this out before implementing this feature. Or only show user-accessible devices (TAGS=:seat:) in the portal to start with.

This also comes back to the other thing that I completely forgot to mention previously, which is that the usbdevfs ioctls aren't supported yet, and those are definitely something that's needed for full-fledged device support (e.g. the HID use case I mentioned above). Some of them needed some careful treading, which is why I wanted to work on and propose the core permissions system first.

Those ioctl() aren't supported by what?

(Side note: this is certainly a lot more written out than I anticipated, I should probably have gone into some of these design ideas in the initial post first.)

I'm guessing you read #227 (comment) ?

In short, I think it would be good to:

  • restrict to /dev/bus/usb (HID devices served by separate HID portal, webcams supported by Pipewire, sound cards through Pipewire/PulseAudio, etc.)
  • remove separate RequestPermission() call
  • use an allowlist of udev properties in an a{sv} for now

Does that all make sense?

I'd be happy to help out with implementing a backend for libusb and integration tests (using umockdev).

(PS: I'm scared of that regex)

@soredake
Copy link

soredake commented Mar 2, 2021

Any progress on this?

@refi64
Copy link
Contributor Author

refi64 commented Mar 29, 2021

@hadess Hey sorry for the long radio silence here, there's been a lot going on. I'm almost done with some changes to fix most of the issues raised, though there were a few design issues that came up that are a bit interesting:

  • Without a separate RequestPermission() call, there's no way for an application to access properties guarded behind permission like the serial #, unless they also open the device. Would it be worth keeping a separate RequestPermission but still let OpenDevice itself request permission?
  • In retrospect I entirely agree that OpenDevice should only bother opening the device nodes under /dev/bus/usb, though it's worth noting that HID devices still appear here.
  • While setting up the allow list, I realized that udev doesn't actually expose all of the info needed for proper device access in the properties. In particular, the parent device is accessible via a separate libudev call, and the currently active USB configuration seems to only be accessible through sysattr. My current solution is to return a D-Bus dictionary with parent as a top-level property, as well as sysattr and udev properties that each contain dicts with some restricted subset of the corresponding information (i.e. sysattr would contain a few essentially sysattr values, udev would contain the allowed udev properties). Would this work?
  • Related to the above, this new structure also means the filtering system would become significantly messier. I'm tempted to simply remove it, since in retrospect, having this sort of functionality isn't that useful; there aren't enough new devices being changed at once to justify the implementation overhead I'd imagine.

EDIT: yes I accidentally hit Shift+Enter before I finished, this may have appeared incomplete in any email notifications. Sorry about that!

@hadess
Copy link
Contributor

hadess commented Mar 31, 2021

  • Without a separate RequestPermission() call, there's no way for an application to access properties guarded behind permission like the serial #, unless they also open the device. Would it be worth keeping a separate RequestPermission but still let OpenDevice itself request permission?

But seeing the serial number would be something of interest when enumerating devices. So do we need a separate method to request permission to access things like the name, or serial number when it's information that seems pretty vital when enumerating.

My idea as far as permissions were concerned was to avoid explicit permission requests, and roll those into API calls that were going to get made.

  1. static permissions with allowlist/blocklist of devices in the Flatpak manifest (VID/PID or class-based lists, I would expect)
  2. portal level permission when enumerating USB devices
  3. portal level permission when opening/accessing a USB device

This gives plenty of opportunity to tighten or loosen the permissions without changes to the API.

Were your concerns about leaking the serial number (and other metadata) about fingerprinting/information leakage, or were there other concerns?

  • In retrospect I entirely agree that OpenDevice should only bother opening the device nodes under /dev/bus/usb, though it's worth noting that HID devices still appear here.

USB HID devices, yes :)

  • While setting up the allow list, I realized that udev doesn't actually expose all of the info needed for proper device access in the properties. In particular, the parent device is accessible via a separate libudev call, and the currently active USB configuration seems to only be accessible through sysattr. My current solution is to return a D-Bus dictionary with parent as a top-level property, as well as sysattr and udev properties that each contain dicts with some restricted subset of the corresponding information (i.e. sysattr would contain a few essentially sysattr values, udev would contain the allowed udev properties). Would this work?

I think this would need to be checked on a case-by-case basis. For sysattr data (currently active USB configuration), we could export that through a udev property directly if it's useful. I have commit rights for systemd, and exporting a sysattr isn't too complicated. What would the parent be used for?

  • Related to the above, this new structure also means the filtering system would become significantly messier. I'm tempted to simply remove it, since in retrospect, having this sort of functionality isn't that useful; there aren't enough new devices being changed at once to justify the implementation overhead I'd imagine.

Yes, I think that it might be a little bit overkill, if combined with the 3 above permission entry points.

@refi64
Copy link
Contributor Author

refi64 commented Apr 1, 2021

Were your concerns about leaking the serial number (and other metadata) about fingerprinting/information leakage, or were there other concerns?

Pretty much this. AFAIK some devices can have fully unique serials, meaning that you could basically pretty easily fingerprint devices, even trying to link them together based on any sometimes-plugged-in shared hardware, whereas just the name would be far more limited in that goal.

I guess we could maybe add a separate method just for getting restricted properties? Then, future calls to get properties would include the restricted ones since permission was already granted.

I think this would need to be checked on a case-by-case basis. For sysattr data (currently active USB configuration), we could export that through a udev property directly if it's useful. I have commit rights for systemd, and exporting a sysattr isn't too complicated.

From what I can tell, the primary candidate I found was bConfigurationValue, which reports the currently selected device configuration. I can double check again, though.

However, wouldn't this mean that any systems with an older udev wouldn't get the new properties, unless we can assign the properties in custom udev rules by reading the sysattr file?

What would the parent be used for?

I'm admittedly not 100% clear on this, but from what I can tell it's primarily used since different USB interfaces are exposed as different nodes, so code sometimes has to walk them up to the closest matching parent in order to find the one it wants to manipulate. In theory, we could abstract over this in the portal...but I'm hesitant to go that far past what udev itself provides.

@hadess
Copy link
Contributor

hadess commented May 21, 2021

Sorry about not getting back to you earlier, I really thought I did (I even remember thinking about the answers).

Were your concerns about leaking the serial number (and other metadata) about fingerprinting/information leakage, or were there other concerns?

Pretty much this. AFAIK some devices can have fully unique serials, meaning that you could basically pretty easily fingerprint devices, even trying to link them together based on any sometimes-plugged-in shared hardware, whereas just the name would be far more limited in that goal.

I guess we could maybe add a separate method just for getting restricted properties? Then, future calls to get properties would include the restricted ones since permission was already granted.

I don't think that we need to protect those properties by using additional calls. The application can only list the set of devices it declared interest in in the Flatpak manifest. Then the application needs to ask for enumeration. Maybe in that part of the API the application could say which fields it's interested in, with the default being to pass the names, and serial if there are duplicate names. Eventually, we could have host-side lockdown settings for whether that data should be passed at all (similarly to the privacy settings in Android forks that allow further lockdown compared to the stock OS).

The serial and other metadata could also be provided as one of the return values to opening the device if that information is only useful once the device is opened.

I think this would need to be checked on a case-by-case basis. For sysattr data (currently active USB configuration), we could export that through a udev property directly if it's useful. I have commit rights for systemd, and exporting a sysattr isn't too complicated.

From what I can tell, the primary candidate I found was bConfigurationValue, which reports the currently selected device configuration. I can double check again, though.

However, wouldn't this mean that any systems with an older udev wouldn't get the new properties, unless we can assign the properties in custom udev rules by reading the sysattr file?

I can't think of a single case where a newer xdg-desktop-portal would be deployed and the rest of the host couldn't be updated, or the support in question backported. I should be able to work on that soon.

What would the parent be used for?

I'm admittedly not 100% clear on this, but from what I can tell it's primarily used since different USB interfaces are exposed as different nodes, so code sometimes has to walk them up to the closest matching parent in order to find the one it wants to manipulate. In theory, we could abstract over this in the portal...but I'm hesitant to go that far past what udev itself provides.

Could we avoid adding API/metadata until we have a user for it?

I think that it would be a very good idea to look at what it would take to port a couple of applications/utilities to using this portal API, at least one that would use a yet to be written libusb shim, and one that can be modified to fully take advantage of a UI-based workflow.

Maybe something like solaar, the Logitech wireless dongle management app, would be a good candidate. It's written in Python, and has both a command-line and UI interface.

The most important isn't to get the API right first time, but not to dig ourselves into an API hole where we couldn't get out. So minimal but extensible API, and API that's close to the existing usage patterns.

Talking of, it might be a good time to start looking at the UI design of the USB portal. Do you have prior art available for, say, Android, iOS, macOS, and maybe Windows UWP?

@Erick555
Copy link

Erick555 commented May 21, 2021

I can't think of a single case where a newer xdg-desktop-portal would be deployed and the rest of the host couldn't be updated, or the support in question backported. I should be able to work on that soon.

Flatpak PPA does xdg-desktop-portal updates on ubuntu including LTS.

@hadess
Copy link
Contributor

hadess commented May 21, 2021

Flatpak PPA does xdg-desktop-portal updates on ubuntu including LTS.

That probably isn't sufficient to support all the new features exported through portals. For example, I can't imagine the Flatpak screen sharing and other newer features working too well if there's no (new enough) pipewire on the host, anymore than if the user uses a desktop which doesn't have the right features. New portals requiring host updates isn't much to ask for.

@ssokolow

This comment has been minimized.

@ssokolow

This comment has been minimized.

@hadess

This comment has been minimized.

@refi64
Copy link
Contributor Author

refi64 commented May 21, 2021

Sorry about not getting back to you earlier, I really thought I did (I even remember thinking about the answers).

No worries, I barely had time to work on this before (and have made far too many replies "in mind only" regardless!).

I don't think that we need to protect those properties by using additional calls. The application can only list the set of devices it declared interest in in the Flatpak manifest. Then the application needs to ask for enumeration.

Maybe in that part of the API the application could say which fields it's interested in

So to be clear:

  • The API could have some option dictating what udev properties to return.
  • If it's undefined, only properties that require no permissions will be returned.
  • If defined and containing a property that's privileged, then the user will be asked to grant device access. If they reject, should the privileged properties be omitted, or the entire call fail?

with the default being to pass the names, and serial if there are duplicate names.

This could be troublesome if a duplicate device is added afterwards though, right? For instance:

  • I attach a device named a, let's call it a1.
  • a1 has no other devices with the same name, so the name is passed directly to the application.
  • I attach another device named a, a2.
  • a2's name now is duplicated, so a2's serial is passed.
  • a1 is re-attached. Now, the device itself is the same, but the serial is now used instead of the name.

To be clear, if all an application needs is "a roughly unique name for each attached USB device", the portal generates random IDs to attach to each device on add. This would serve most purposes, but afaik the purpose of the serial leans more towards being unique across runtime adds & removes.

In addition, as I was typing this, there's one other issue: not having a way of getting the serial itself directly means we couldn't support WebUSB in Chromium or Firefox, since it requires the ability to get a device serial.

I can't think of a single case where a newer xdg-desktop-portal would be deployed and the rest of the host couldn't be updated, or the support in question backported. I should be able to work on that soon.

Hmm, I think in most cases thus far though, an entire portal ends up missing, right? It seems weird to make the device portal just "not work" if a specific udev property that is desired is not available, even if no app actually needs it. On the other hand, if the portal appears to work, but a particular property is just silently missing, it could result in some bizarre behavior.

As I was thinking about this, I realized we could technically read the value ourselves and "synthesize" a udev property if it's not available on the host version. However, I'm not sure if udev change events would get emitted if said synthesized property changes... I guess technically the portal could also just ship a udev rule to set it? (Not sure if rules can actually read sysattr values and track changes though.)

Could we avoid adding API/metadata until we have a user for it?

There is a user for it: that example was pulled from Chromium. I just haven't stepped through the code far back enough to get the full reasoning as to why (but I can if needed).

Talking of, it might be a good time to start looking at the UI design of the USB portal. Do you have prior art available for, say, Android, iOS, macOS, and maybe Windows UWP?

As-is, the current dialog was loosely based (but reworded to match the other portal dialogs) on Android, which looks like this (admittedly the first screenshot I found on the web):

image

In essence, it just asks the user, showing them the app and device name.

The offer to open the application on connecting the USB device was not added, since it would open up a sizable rabbit hole of implementation details.

@serebit
Copy link

serebit commented Jul 25, 2021

I'm waiting on this for one of my projects. If I can help out in any way, let me know.

@rmader
Copy link
Contributor

rmader commented Sep 7, 2021

Sorry for the noise, just wanted to add that I'd also love to see progress here in order to lock down Firefox and Chromium/Electron based apps. I'm currently working on making WebRTC use the xdp-camera portal, but as browsers also support Yubikeys, gaming controllers etc. we're stuck on device=all until this lands, IIUC.

If you're busy with other things, I wonder if it would make sense to have a device=usboption in flatpak, exposing /dev/bus/usb - as a simple option that at least allows some more restriction. WDYT?

Edit: opened flatpak/flatpak#4405

Copy link
Member

@GeorgesStavracas GeorgesStavracas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some random drive-by comments.

</para></listitem>
</varlistentry>
<varlistentry>
<term>has_joystick s</term>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a boolean?

<varlistentry>
<term>has_joystick s</term>
<listitem><para>
Whether this device has / is a joystick.If not present, then it should be
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
Whether this device has / is a joystick.If not present, then it should be
Whether this device has / is a joystick. If not present, then it should be

<term>writable b</term>
<listitem><para>
Whether the device can be opened for writing with
org.freedesktop.portal.Usb.OpenDevice().If not present, then it should be
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
org.freedesktop.portal.Usb.OpenDevice().If not present, then it should be
org.freedesktop.portal.Usb.OpenDevice(). If not present, then it should be

</para></listitem>
</varlistentry>
<varlistentry>
<term>devnode s</term>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps device_node? (Probably not a good idea though, since devnode matches udev terminology)

filter = get_filter_from_options (&options_dict);

permissions = app_usb_permissions_for_app_info (app_info);
usb_enumerate_all_to_variant (usb, &builder, permissions, filter);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please clarify how USB devices are enumerated the first time an app is run? It looks to me that this code would run into a chicken-and-egg problem, where apps can't enumerate devices because they don't have permissions stored, and they don't have permissions because they can't enumerate devices.

@hadess
Copy link
Contributor

hadess commented Apr 21, 2022

We're still missing revoke support in the kernel, as I mentionedi in the original issue, so that the portal can't yank access to an application that's opened a device, which is a potential security issue, and we don't have a way to request access to USB devices for which we don't already have access. For example, accessing an Arduino in programming mode still would require running a bunch of commands as root/install udev rules.

I've started working on the USB revoke support, motivated by whot's work on HID revoke support: systemd/systemd#23140

@GeorgesStavracas
Copy link
Member

GeorgesStavracas commented Dec 8, 2023

I took the liberty to use these patches as the starting point of a major rework, and this has now moved to #1238

(I'll keep coming back to this PR because @hadess' comments are very much valuable, but I'll close it so we track the work in a single place.)

@ssokolow

This comment was marked as outdated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Triaged
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

8 participants