interfaces: add a joystick interface

[kyrofa]

This PR resolves LP: #1675871 by adding a joystick interface that simply allows AppArmor access to /dev/input/js[0-9]*.

Note that the framebuffer interface was heavily used for reference here, minus the udev component which seems to be in the midst of being removed. I wrote this component, but commented it out with a TODO so we don't forget to fix it as part of LP: #1675738.

Note also that this PR is conservative: this interface is not automatically connected in its present state. I don't see an immediately obvious reason this interface shouldn't be automatically connected, but I'll let the reviewers decide.

[mvo5]

You probably want snap.TypeOS here

[kyrofa]

Indeed I do, thank you. Done.

[jdstrand]

The interface looks fine to me. I would actually prefer that the udev code be present with addSnippet() commented out with a TODO that this needs to be fixed as a reminder that we need to do this. The framebuffer udev tagging is only temporarily being removed (and I asked them to comment out the code instead of removing in the other PR).

Please also update https://bugs.launchpad.net/snapd/+bug/1675738 to add the joystick interface to the list for phase 2.

[jdstrand]

I think your instincts for the base decalaration are correct. On the one hand, this is giving precisely what the interface advertises, but it is a device driver which adds attack surface to the kernel. I also suspect that we are going to need more extensive udev and/or /dev/input access in the future which definitely means we won't want to auto-connect. We can always reconsider in the future.

[kyrofa]

Always easier to open it up later than it is to lock it down 👍 .

[zyga]

Shall we make this js[0-9]+, unless /dev/js is a customary name as well

[kyrofa]

Indeed, the first device is js0. I didn't realize + was supported.

[jdstrand]

AARE doesn't support '+'. See 'Globbing' in man apparmor.d. If you want to support /dev/input/js on its own, then please use:

/dev/input/js{,[0-9]*} rw,

if you want only /dev/input/jsN, then the current rule is correct.

[kyrofa]

js is always followed by a number, so leaving as-is.

[jdstrand]

Looking a thttps://github.com/torvalds/linux/blob/master/Documentation/admin-guide/devices.txt, it will always have a number, so the current rule is correct.

In reading that, please also add this for futureproof-ness:

/run/udev/data/c13:{[0-9],[12][0-9],3[01]} r,

[kyrofa]

Sure thing-- done.

[zyga]

@jdstrand those numbers there are totally magic to me. I think we could use a comment that says what that is (or a link to some kernel document)h

[kyrofa]

I would actually prefer that the udev code be present with addSnippet() commented out with a TODO that this needs to be fixed as a reminder that we need to do this.

No problem, done.

Please also update https://bugs.launchpad.net/snapd/+bug/1675738 to add the joystick interface to the list for phase 2.

Also done.

[zyga]

LGTM

[kyrofa]

2017/03/30 19:47:09 Found /tmp/autopkgtest.hlqd2F/build.B6x/snapd/spread.yaml.
2017/03/30 19:47:53 Project content is packed for delivery (58.49MB).
2017/03/30 19:47:53 Sequence of jobs produced with -seed=1490903273
2017/03/30 19:47:53 Allocating autopkgtest:ubuntu-16.04-amd64...
2017/03/30 19:47:53 Waiting for autopkgtest:ubuntu-16.04-amd64 to make SSH available at localhost:22...
2017/03/30 19:47:53 Allocated autopkgtest:ubuntu-16.04-amd64.
2017/03/30 19:47:53 If killed, discard servers with: spread -reuse-pid=8030 -discard
2017/03/30 19:47:53 Connecting to autopkgtest:ubuntu-16.04-amd64...
2017/03/30 19:48:53 Discarding autopkgtest:ubuntu-16.04-amd64, cannot connect: cannot connect to autopkgtest:ubuntu-16.04-amd64: ssh: must specify HostKeyCallback
2017/03/30 19:48:54 Successful tasks: 0
2017/03/30 19:48:54 Aborted tasks: 147

Do the autopkgtests just need to run again, or is there a larger problem?

[mvo5]

One question about the possibility of simplifying the interface. Otherwise looks very nice, thanks for doing this work!

[mvo5]

Silly(?) question - but given that it appears this is only having an apparmor snippet and is otherwise a very simple interface - would it make sense to just use the commonInterface abstraction/helper here? Similar to e.g. camera.go. This way joystick.go could probably be written as:

const joystickConnectedPlugAppArmor = `
 /dev/input/js[0-9]* rw, 
 /run/udev/data/c13:{[0-9],[12][0-9],3[01]} r, 
`
func NewJoystickInterface() interfaces.Interface {
	return &commonInterface{
		name: "joystick",
		connectedPlugAppArmor: joystickConnectedPlugAppArmor,
		reservedForOS:         true,
	}
}

I put a better diff here http://paste.ubuntu.com/24305321/

[jdstrand]

@mvo5 because the CE team is committed to add the udev backend to all the interfaces that reference /dev but don't use it. Having it written this way for this new interface will help to avoid adding extra work to that effort.

[kyrofa]

@jdstrand specifically instructed me to use framebuffer for reference instead of camera. I'm not 100% sure why, but suspect it has something to do with LP: #1675738. I'm sure he can clarify.

[kyrofa]

Heh, sorry, we were typing at the same time 😄 .

[jdstrand]

Actually, we can do the same with /dev/input/js which I thought of over the weekend. To combine this with @zyga's request, please adjust this to be:

# Per https://github.com/torvalds/linux/blob/master/Documentation/admin-guide/devices.txt
# only js0-js31 is valid so limit the /dev and udev entries to those devices.
/dev/input/js{[0-9],[12][0-9],3[01]} rw,
/run/udev/data/c13:{[0-9],[12][0-9],3[01]} r,

[kyrofa]

Good idea-- done.