interfaces: add random interface #3045

Merged
merged 17 commits into from May 8, 2017

Conversation

Projects
None yet
5 participants
Contributor

femdom commented Mar 17, 2017

Allow access to the /dev/hwrng device (used in the Raspberry Pi).

femdom added some commits Mar 17, 2017

We can probably name this as simply "random", and include in this interface any random-number generation infrastructure available in the system.

Otherwise, just a few notes:

interfaces/builtin/basedeclaration.go
+ allow-installation:
+ slot-snap-type:
+ - core
+ deny-auto-connection: true
@niemeyer

niemeyer Mar 17, 2017

Contributor

Why? This looks like something we should hand off by default.

interfaces/builtin/hw_random.go
+# Description: Allow access to the hardware random number generator device - /dev/hwrng
+
+/dev/hwrng rw,
+/devices/virtual/misc/hw_random rw,
@niemeyer

niemeyer Mar 17, 2017

Contributor

Missing a /sys prefix.

@niemeyer niemeyer changed the title from Adds hw-random interface to interfaces: add random interface Mar 17, 2017

femdom added some commits Mar 17, 2017

Contributor

femdom commented Mar 17, 2017

@niemeyer , I fixed the code after your comments.

For the some reason tests fail because of timeout.

@niemeyer niemeyer requested a review from jdstrand Mar 20, 2017

Contributor

niemeyer commented Mar 20, 2017

@femdom Thanks. We had some issues last week with test machine contention (too many tests, not enough machines), which resulted in them waiting too much for a machine to be available. Situation is normalized now and I've restarted the test.

@jdstrand can you please have a look at this one when you have a moment?

Contributor

niemeyer commented Mar 20, 2017

Travis tests passing.

stolowski added some commits Mar 27, 2017

Contributor

stolowski commented Mar 27, 2017

@femdom FYI, I've updated this PR for the latest API changes.

Contributor

femdom commented Mar 27, 2017

Thanks, @stolowski! Checks passed!

Several open questions in addition to requested changes.

@niemeyer - you should weigh in on 'hardware-random' vs 'hardware-random-control' and 'hardware-random-observe'.

interfaces/builtin/basedeclaration.go
+ allow-installation:
+ slot-snap-type:
+ - core
+ allow-auto-connection: true
@jdstrand

jdstrand Mar 27, 2017

Contributor

I think this should be renamed to hardware-random or even hardware-random-control since random implies /dev/random and /dev/urandom and we already allow access to those by default.

I also think this should be: deny-auto-connection: true since direct access to /dev/hwrng is not normally needed and typically only used by applications such as rng-tools to mix /dev/hwrng into kernel entropy. More importantly, a hardware-specific kernel driver for /dev/hwrng could be implemented in any number of ways including with weak (or non-existent) entropy (the kernel does not perform any fitness tests). Since this interface allows changing the hwrng via sysfs, it is possible to switch out a stronger rng for a weaker one and this is something that should not be auto-connected.

Perhaps this could be broken out into two interfaces:

  • hardware-random-observe - read access to relevant files from this PR, manually connected
  • hardware-random-control - all of hardware-random-observe plugs write access to /sys/class/misc/hw_random/rng_current for changing the hwrng, manually connected

I would argue that hardware-random-observe would still be a manual connection because the access is not typically needed, the device may not be provided by the kernel and there is no guarantee that the fitness of the device is good (ie, the gadget developer or device owner should decide whether a snap should use /dev/hwrng).

@niemeyer

niemeyer Apr 26, 2017

Contributor

All recommendations, including naming, feel sound. 👍

@zyga

zyga May 8, 2017

Contributor

I'm doing this change now.

interfaces/builtin/random.go
+)
+
+const randomConnectedPlugAppArmor = `
+# Description: Allow access to the hardware random number generator device - /dev/hwrng
@jdstrand

jdstrand Mar 27, 2017

Contributor

Please change this to:

# Description: allow direct access to the hardware random number generator device. Usually,
# the default access to /dev/random is sufficient, but this allows applications such as rng-tools
# to use /dev/hwrng directly or change the hwrng via sysfs. For details, see
# https://www.kernel.org/doc/Documentation/hw_random.txt
@zyga

zyga May 8, 2017

Contributor

Done

interfaces/builtin/random.go
+const randomConnectedPlugAppArmor = `
+# Description: Allow access to the hardware random number generator device - /dev/hwrng
+
+/dev/hwrng rw,
@jdstrand

jdstrand Mar 27, 2017

Contributor

This should just be 'r'.

@zyga

zyga May 8, 2017

Contributor

Done though have a look, I'm not sure if the control-vs-observe split applies here

interfaces/builtin/random.go
+# Description: Allow access to the hardware random number generator device - /dev/hwrng
+
+/dev/hwrng rw,
+/sys/devices/virtual/misc/hw_random rw,
@jdstrand

jdstrand Mar 27, 2017

Contributor

This is a directory, not a file. This rule should be:

/sys/devices/virtual/misc/ r,
/sys/devices/virtual/misc/hw_random/rng_{available,current} r,
# Allow changing the hwrng
/sys/devices/virtual/misc/hw_random/rng_current w,

The above assumes you actually need write access to /sys/devices/virtual/misc/hw_random/rng_current. Do you?

You should also add this rule:

/run/udev/data/c10:183 r,
@zyga

zyga May 8, 2017

Contributor

Done

Contributor

femdom commented Mar 28, 2017

Understood, thanks.

Contributor

jdstrand commented Mar 28, 2017

@femdom - just to be clear, please don't make any changes just yet until these open questions are answered.

Contributor

femdom commented Apr 25, 2017

Just updating the ticket. Let me know if I can fix addressed issues.

Contributor

niemeyer commented Apr 26, 2017

@femdom This is good to move on following @jdstrand's advices.

zyga added some commits May 8, 2017

snap,interfaces: split random into hardwar-random-{observe,control}
This patch splits the "random" interface into a hardware-random-observe
and -control interface pair. In addition the plugs no longer
auto-connect and there are some extra permission for the -control plug.
There are several smaller changes as requested by jdstrand.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Contributor

zyga commented May 8, 2017

I just updated this according to @jdstrand specification. Let me know if there's anything more needed, I can quickly iterate and hopefully land this interface.

zyga added some commits May 8, 2017

interfaces/builtin: add extra checks for hardware-random-*
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
interfaces/builtin: tweak description for hardware-random-observe
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Approving since my feedback is all nitpicks, but please update before committing.

BTW - thanks for the alphabetizing! :)

+# Description: allow direct access to the hardware random number generator device. Usually,
+# the default access to /dev/random is sufficient, but this allows applications such as rng-tools
+# to use /dev/hwrng directly or change the hwrng via sysfs. For details, see
+# https://www.kernel.org/doc/Documentation/hw_random.txt
@jdstrand

jdstrand May 8, 2017

Contributor

Nit: can you wrap these to 80 chars?

@zyga

zyga May 8, 2017

Contributor

Done

+/sys/devices/virtual/misc/hw_random/rng_{available,current} r,
+# Allow changing the hwrng
+/sys/devices/virtual/misc/hw_random/rng_current w,
+/run/udev/data/c10:183 r,
@jdstrand

jdstrand May 8, 2017

Contributor

Can you move this to below /dev/hwrng? It corresponds to that device and this access isn't needed for changing the hwrng. I also like to have blcnks lines before comments in policy, for readability. Eg:

 +/dev/hwrng rw,
 +/run/udev/data/c10:183 r,
 +/sys/devices/virtual/misc/ r,
 +/sys/devices/virtual/misc/hw_random/rng_{available,current} r,
 +
 +# Allow changing the hwrng
 +/sys/devices/virtual/misc/hw_random/rng_current w,
@zyga

zyga May 8, 2017

Contributor

Done

+}
+
+func (iface *HardwareRandomControlInterface) AutoConnect(*interfaces.Plug, *interfaces.Slot) bool {
+ return true
@jdstrand

jdstrand May 8, 2017

Contributor

Can you add above this 'return' (for consistency with other interfaces):

// Allow what is allowed in the declarations
@zyga

zyga May 8, 2017

Contributor

Done

+/dev/hwrng r,
+/sys/devices/virtual/misc/ r,
+/sys/devices/virtual/misc/hw_random/rng_{available,current} r,
+/run/udev/data/c10:183 r,
@jdstrand

jdstrand May 8, 2017

Contributor

Can you move this under /dev/hwrng, like above?

@zyga

zyga May 8, 2017

Contributor

Done :-)

+}
+
+func (iface *HardwareRandomObserveInterface) AutoConnect(*interfaces.Plug, *interfaces.Slot) bool {
+ return true
@jdstrand

jdstrand May 8, 2017

Contributor

and add above this 'return':

// Allow what is allowed in the declarations
@zyga

zyga May 8, 2017

Contributor

Also done :-)

zyga added some commits May 8, 2017

interfaces/builtin: wrap description to 80 columns
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
interfaces/builtin: reorder apparmor rules for association
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
interfaces/builtin: add newlines for visual spacing
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
interfaces/builtin: add comments for consistency
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

@zyga zyga merged commit a98da33 into snapcore:master May 8, 2017

5 of 7 checks passed

xenial-amd64 autopkgtest running
Details
xenial-i386 autopkgtest running
Details
artful-amd64 autopkgtest finished (success)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
xenial-ppc64el autopkgtest finished (success)
Details
yakkety-amd64 autopkgtest finished (success)
Details
zesty-amd64 autopkgtest finished (success)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment