interface: add usb device support to serial-port

[jocave]

Now allow serial-ports to be used in two ways:

1. A fixed "path: /dev/tty.." attribute defined on the slot with no attributes expected on the plug
2. A implicit slot with no attributes declared by the core snap, which may be connected to by a plug with VID & PID attributes. These will be used to create a UDev rule with a TAG that will cause snap-confine to set up cgroups such that only matching devices will be accessible.

[morphis]

We should verify here too for the case that we have a path specified that we're only allowed to connect when the plug as the path attribute with the same value specified.

To improve the user experience for this we may should add attribtes to the output of snap interfaces

[morphis]

Mostly LGMT, one comment inline.

[jocave]

Last commit makes it necessary for the path attribute to be on both plug & slot.

I also raised a bug for the comment about the snap interfaces command

[mvo5]

Needs de-conflicting

[morphis]

LGTM, @jdstrand can you have a look?

[jdstrand]

@morphis - yes, after I look at a high priority bug.

[jocave]

Please consider reviewing the PR at the same time - they are essentially the same code but for different device types:

#1696

[jdstrand]

Gustavo asked me to request that people use const string (and do a []byte(<string>) when needed) for security backend variables that hold policy. There are plenty of examples in the interfaces where this isn't happening, and they will be cleaned up.

[jdstrand]

I prefer your cleanup here too. Thanks

[jdstrand]

Can you update docs/interfaces.md for the new attributes, etc?

[jdstrand]

udevVidPidFormat is hard to read (for me). Would udevVendorIdFormat be better?

UPDATE: actually, since this regex is used for Product Id too, how about udevVendorProductIdFormat?

[jdstrand]

Ok, now that we are on the plugs side...

This comment has a grammatical error. I think it is supposed to say "Check slot has a path attribute to identify serial device".

That said, the code doesn't do that-- it is verifying if the plug side has a valid path attribute.

The PR description says: "1. A fixed "path: /dev/tty.." attribute defined on the slot with no attributes expected on the plug", but you are validating the path attribute on the plugs side.

It seems to me that the case 1 code needs to:

• remove the path check-- it is already in SanitizePlug
• if has vendor attribute, verify the pattern
• if has product attribute, verify the pattern

[jdstrand]

This is wrong based on "1. A fixed "path: /dev/tty.." attribute defined on the slot with no attributes expected on the plug". Shouldn't this be slotPath, slotOk := slot.Attrs["path"].(string) and then you add the apparmor rule?

[jdstrand]

I think the logic would be clearer if you did:

if slotOk && slotPath != "" {
    ...
        return []byte(fmt.Sprintf("%s rwk,\n", cleanedPath)), nil
} else {
    ...
        return []byte("/dev/tty* rw,\n"), nil
    ...
        return udevSnippet.Bytes(), nil
    }

[jdstrand]

Thank you for using this instead of /dev/** rw,. There is a bug in snap-confine that I need to fix wrt no matched tags and these finer-grained apparmor rules help limit the impact of such bugs.

[jdstrand]

If we are going to support vendor, product or vendor+product (which I thought we would), then this needs to be reworked.

[morphis]

We shouldn't. We can add support for this easily later without breaking anything but for now I don't see any need to assign access to all devices from the same vendor or product type.

[jdstrand]

Ok, excepting the questions regarding attributes, whether product id and vendor id can be specified separately, I want to discuss this PR direction to make sure we all (eg, you, @zyga, @niemeyer and myself) all agree with the approach.

As per the PR description:
"Now allow serial-ports to be used in two ways:

1. A fixed "path: /dev/tty.." attribute defined on the slot with no attributes expected on the plug
2. A implicit slot with no attributes declared by the core snap, which may be connected to by a plug with VID & PID attributes. These will be used to create a UDev rule with a TAG that will cause snap-confine to set up cgroups such that only matching devices will be accessible."

I'll try to summarize plans AIUI:

'1' is intended to support future work where snapd from the core snap detects serial ports and exposes them as implicit slots. This detection would include detecting unpluggable serial ports like /dev/ttyS0 on boot as well as processing udev notifications of hotplug/hotunplug events that match the tty and usb subsystems. This future work would expose :<some serial port name> (ie, a named (TBD) implicit slot) for snaps to then be able to plugs: [ <some serial port name> ]. The snap author does not have to specify the device path in the yaml and the user does not have to express the device path with 'snap connect'.

'2' is intended to support the need today for assigning a particular device with a specific vendor and product id to a snap. The snap author does specify the vendor and product id but does not need to express the device path in the yaml and the user does not have to express the device path (or vendor and product id) with 'snap connect'.

Neither '1' nor '2' addresses how gadget snaps are supposed to weigh in.

I'm trying to understand where we are going to make sure that we implement this interface in a manner that is consistent with the final design. I suspect that might be difficult since this is not all fleshed out yet but let me propose a straw man that I think unblocks the immediate requirement (what prompted this PR) that is consistent with the future.

= Straw man =
At a high level:

• we plan for the core snap to perform auto-detection of unpluggable and hotpluggable serial ports using dynamic, but meaningful low-level interface names (eg, serial0, serial1 and usbserial0). The auto-detection is done via udev queries of subsystems (eg, tty, usb)
• when possible, the core snap provides aliases for the low-level interface names based on vendor and product ids (eg, serial-abcd-0123 -> usbserial0)
• the gadget snap provides additional aliases for the core snap's low-level names and aliases (eg, 'left-serial' -> serial0, 'right-serial' -> serial1, 'back-serial' -> serial-abcd-0123 (eg, hard-wired usb device), 'foo-addon-serial' -> serial-edff-4567 (an optional serial port that can be hotplugged into the device)

Assuming a gadget with /dev/ttyS0, /dev/ttyS1, hard-wired /dev/ttyUSB0 (vendor=abcd, product=0123) and hotpluggable /dev/ttyUSB1 (vendor=edff, product=4567) that is unplugged, we have implicit dynamic slots from core that on boot (but before the gadget aliases are applied) that are the equivalent of:

slots:
  serial0:
    interface: serial-port
    path: /dev/ttyS0
  serial1:
    interface: serial-port
    path: /dev/ttyS1
  usbserial0:
    interface: serial-port
    vendor: abcd
    product: 0123
    aliases:
    - serial-abcd-0123

Such that:

$ snap interfaces
Slot              Plug
:serial0          -
:serial1          -
:usbserial0       -
:serial-abcd-0123 -

Then the gadget snap applies its aliases and the merged core+gadget implicit dynamic slots are the equivalent of:

slots:
  serial0:
    interface: serial-port
    path: /dev/ttyS0
    aliases:
    - left-serial
  serial1:
    interface: serial-port
    path: /dev/ttyS1
    aliases:
    - right-serial
  usbserial0:
    interface: serial-port
    vendor: abcd
    product: 0123
    aliases:
    - serial-abcd-0123
    - back-serial

Such that:

$ snap interfaces
Slot              Plug
:serial0          -
:serial1          -
:usbserial0       -
:serial-abcd-0123 -
:left-serial      -
:right-serial     -
:back-serial      -

Then at some later point, the user plugs in the addon usb serial port and the new merged dynamic slot equivalent yaml is (omitting snap interfaces output):

slots:
  serial0:
    interface: serial-port
    path: /dev/ttyS0
    aliases:
    - left-serial
  serial1:
    interface: serial-port
    path: /dev/ttyS1
    aliases:
    - right-serial
  usbserial0:
    interface: serial-port
    vendor: abcd
    product: 0123
    aliases:
    - serial-abcd-0123
    - back-serial
  usbserial1:
    interface: serial-port
    vendor: edff
    product: 4567
    aliases:
    - serial-edff-4567
    - foo-addon-serial

Finally, apps can plugs like so:

plugs:
- serial0          # request the first serial device
- right-serial     # requests serial port by gadget alias
- serial-abcd-0123 # requests a serial device by vendor and product id
- foo-addon-serial # requests hotplugged serial port by gadget alias

Now, understanding that dynamic detection is not here yet, how can we meet the requirements of '2' while being forward thinking? Rather than add vendor and product attributes, simply plugs: [ serial-abcd-0123 ] and snapd will parse that.

Several issues with the above:

• perhaps we want don't want to alias 'serial-abcd-0123' but instead want to add interface attributes for the plugs side that maps to the slot side (that's easy enough to adjust the above for)
• snap interfaces output is poor
• how to assign all serial devices (plugs: [ serial* ]?)
• working through assigning unplugged devices with unknown vendor and product (if that should be supported)

@zyga and @niemeyer - I hope you don't mind the long comment. I tried to tie together various pieces based on my understanding. If you have already worked all this out before, please show me the specification so I might understand how this PR fits in, otherwise, this comment can be a springboard for discussion. :)

[morphis]

@jdstrand We had a a very long discussion on this already and agreed that this is for now the way forward and wouldn't break a future design where snapd would expose dynamic slots. In detail what we want to allow for now and agreed on:

• Add an implicit slot :serial to the core snap without any attributes and it doesn't put any snippets in place
• A plug has to specify either path || (vendor_id && product_id) [we can easily allow vendor_id || product_id only but I don't see any reason for this yet]
• On plug connection we put the udev rule for device tagging in place which will put the device node into the device cgroup of the snap application
• On plug connection a AppArmor rule allows access to the single device node path either specified directly via the path attribute or indirect by vendor_id && product_id

This can easily stay for future design changes as we rely on the implicit :serial slot of the core snap which I would suppose to stay even if we add dynamic device detection and slot creation inside snapd. Once we add that we can limit the use of the :serial slot on the store review process side of things to move people over to the new dynamic slots over time.

If all agree I would like to offload the discussion for the whole dynamic slot thing and finish the implementation to archive the requirements listed above. We have a very tight timeline and I would us to get the first version described above done. As outlined there shouldn't be any problems with a future extension of the interface to handle dynamic slots.

[jocave]

Made the policy strings constants and fixed the comment with errors in it.

@jdstrand - Quite a few of your comments are based on the premise the path attribute should only be slot side. That is how I initially developed the interface, but after the first review from @morphis, I modified the code to require a matching path attribute on both plug and slot for connection to be made.

I'm happy to wait for the thoughts of the others tagged in the last review and can changed if required. However, I think that the implementation as is meets what we set out to achieve for the next iteration of this interface given forthcoming milestones.

[morphis]

@jocave Right that was my idea there, as that way we could also specify a slot for now on the gadget snap rather than using the :serial slot on the core snap too. But this should still allow connecting to a slot on the core snap which does not have any attribute defined.

[niemeyer]

@jdstrand We had a a very long discussion on this already and agreed that this is for now the way forward and wouldn't break a future design where snapd would expose dynamic slots. In detail what we want to allow for now and agreed on:

For the record, I was part of the initial conversation a couple of months back, when we still had hardcoded paths, but wasn't part of those follow up discussions and agreements. A lot here is completely new to snapd and to me, and some of those ideas are problematic as we discussed online last week.

@jdstrand - Quite a few of your comments are based on the premise the path attribute should only be slot side. That is how I initially developed the interface, but after the first review from @morphis, I modified the code to require a matching path attribute on both plug and slot for connection to be made.

How can we possibly have a path on the plug side? We'd be asking people developing snaps that require a serial port to know where the serial port will live on the target device!

The idea of hardcoding vendor-id and product-id on a serial port is also twisting my mind a little bit. There are many providers of generic serial port chips. Using such a hardcoded id pair while purporting it to be hot-pluggable seems counter-intuitive. It means although one may hot-plug, it needs to be that one serial port chip or it won't work.

Finally, as an implementation detail, the trick of counting the number of attributes to see if a specific attribute is present or not is an approach that breaks forwards compatibility badly. Imagine what happens with old devices when we extend the declaration with one more attribute. That said, this is just a hint for future cases. I think we want to review those descriptions anyway.

We need a call to figure those details out.

[niemeyer]

We need a new approach here. I'll close this and other PRs that are using the same methodology. Let's please discuss the right approach and recreate a clean PR when ready for review.