vc: qemu: Add config option to choose entropy source. #781

jcvenegas · 2018-09-24T19:32:55Z

Today we use /dev/random as entropy source.
This is a blocking entropy source, when the
host is getting a low amount of entropy the
Kata VM startup takes longer because block
trying to get entropy from the source.

This change allow to change the entropy source.
For example: urandom
Fixes: #702

jcvenegas · 2018-09-24T19:34:05Z

/test

katacontainersbot · 2018-09-24T19:49:52Z

PSS Measurement:
Qemu: 169896 KB
Proxy: 4164 KB
Shim: 8984 KB

Memory inside container:
Total Memory: 2043448 KB
Free Memory: 2006588 KB

sboeuf · 2018-09-24T19:58:40Z

virtcontainers/qemu_arch_base.go

@@ -525,7 +525,8 @@ func (q *qemuArchBase) appendVFIODevice(devices []govmmQemu.Device, vfioDev conf
 func (q *qemuArchBase) appendRNGDevice(devices []govmmQemu.Device, rngDev config.RNGDev) []govmmQemu.Device {
 	devices = append(devices,
 		govmmQemu.RngDevice{
-			ID: rngDev.ID,
+			ID:       rngDev.ID,
+			Filename: "/dev/urandom",


@jcvenegas looks good, but don't you think we should make this configurable instead of hardcoding in govmm ?
We should have the opportunity to select /dev/urandom from Kata code, WDYT?

Agree, updating.

codecov · 2018-09-24T20:51:11Z

Codecov Report

Merging #781 into master will decrease coverage by <.01%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master     #781      +/-   ##
==========================================
- Coverage   65.32%   65.32%   -0.01%     
==========================================
  Files          87       87              
  Lines       10611    10619       +8     
==========================================
+ Hits         6932     6937       +5     
- Misses       2982     2985       +3     
  Partials      697      697

laijs · 2018-09-25T03:22:26Z

this change makes me a little nervous. security is important in this project. I would prefer /dev/random as the DEFAULT source.

WeiZhang555 · 2018-09-25T03:58:49Z

The code looks good, but I have a question. Do we need a configuration option? I mean, in which situation should we choose /dev/random against /dev/urandom ? Will be /dev/urandom always a prefered option?

sboeuf · 2018-09-25T05:12:33Z

/test

sboeuf · 2018-09-25T05:14:22Z

@laijs @WeiZhang555 I have asked the question about /dev/urandom on the kata-dev mailing list. Please give more details about the potential issue related to using /dev/urandom

katacontainersbot · 2018-09-25T05:26:02Z

PSS Measurement:
Qemu: 165442 KB
Proxy: 4153 KB
Shim: 8792 KB

Memory inside container:
Total Memory: 2043448 KB
Free Memory: 2006580 KB

jodh-intel · 2018-09-25T08:36:31Z

Would it be worth atleast logging the value of /proc/sys/kernel/random/entropy_avail on container create as a debug aid?

I suspect we can't reliably determine how much entropy we'd actually need. But if we could, if the system entropy falls below that threshold, we could make that a hard error.

jodh-intel · 2018-09-25T10:09:46Z

cli/kata-env.go

@@ -319,6 +320,7 @@ func getHypervisorInfo(config oci.RuntimeConfig) HypervisorInfo {
 		Msize9p:           config.HypervisorConfig.Msize9p,
 		UseVSock:          config.HypervisorConfig.UseVSock,
 		MemorySlots:       config.HypervisorConfig.MemSlots,
+		EntropySource:     config.HypervisorConfig.EntropySource,


should this be displayed in kata-env?

should we do any validation on this before handing to the hypervisor? (aka does it exist, is it a char device?)
Looking at govmm, it also doesn't check this but that may well be by design since presumably qemu will :)
https://github.com/intel/govmm/blob/master/qemu/qemu.go#L1035..L1042

/cc @markdryan

@jodh-intel qemu complains about it, OCI runtime create failed: qemu-system-x86_64: -object rng-random,id=rng0,filename=/tmp/r: Could not open '/tmp/r': No such file or directory: unknown

jcvenegas · 2018-09-25T14:17:52Z

/retest

katacontainersbot · 2018-09-25T16:24:26Z

PSS Measurement:
Qemu: 169678 KB
Proxy: 3994 KB
Shim: 8882 KB

Memory inside container:
Total Memory: 2043448 KB
Free Memory: 2006448 KB

sboeuf · 2018-09-25T22:27:07Z

/retest

sboeuf

Based on the several inputs we got on the kata-dev mailing list, it is fine to pursue with this approach and I am all for merging this :)

egernst · 2018-09-25T22:28:13Z

Makefile

@@ -136,6 +136,8 @@ DEFMEMSLOTS := 10
 DEFBRIDGES := 1
 #Default network model
 DEFNETWORKMODEL := macvtap
+#Default entropy source
+DEFENTROPYSOURCE := /dev/random


should the default be /dev/urandom?

sboeuf · 2018-09-25T22:30:14Z

Makefile

@@ -136,6 +136,8 @@ DEFMEMSLOTS := 10
 DEFBRIDGES := 1
 #Default network model
 DEFNETWORKMODEL := macvtap
+#Default entropy source
+DEFENTROPYSOURCE := /dev/random


Oh sorry, I'd like urandom to be the default here, and we should add some comments related to /dev/random, explaining this can be used but with some limitations.

katacontainersbot · 2018-09-25T22:42:15Z

PSS Measurement:
Qemu: 169098 KB
Proxy: 4253 KB
Shim: 8722 KB

Memory inside container:
Total Memory: 2043448 KB
Free Memory: 2006332 KB

This adds a config option to choose the VM entropy source. Fixes: kata-containers#702 Signed-off-by: Jose Carlos Venegas Munoz <jose.carlos.venegas.munoz@intel.com>

jcvenegas · 2018-09-25T22:56:18Z

/test

katacontainersbot · 2018-09-25T23:17:50Z

PSS Measurement:
Qemu: 171426 KB
Proxy: 4073 KB
Shim: 8834 KB

Memory inside container:
Total Memory: 2043448 KB
Free Memory: 2006712 KB

bergwolf · 2018-09-26T01:41:56Z

Per Ted's recommendation on the mailing list, we are proceeding the patch with just seeding virtio-rng with /dev/urandom[1]. Let's remove the wip label and merge it since it has already got enough ACKs.

[1] http://lists.katacontainers.io/pipermail/kata-dev/2018-September/000439.html

jcvenegas requested a review from sboeuf September 24, 2018 19:33

sboeuf reviewed Sep 24, 2018

View reviewed changes

jcvenegas added the do-not-merge label Sep 24, 2018

jcvenegas force-pushed the urandom-source branch 2 times, most recently from 6e7cb17 to 3380bf2 Compare September 25, 2018 00:01

bergwolf approved these changes Sep 25, 2018

View reviewed changes

bergwolf mentioned this pull request Sep 25, 2018

Kata fails to start #702

Closed

jodh-intel reviewed Sep 25, 2018

View reviewed changes

jcvenegas force-pushed the urandom-source branch from 3380bf2 to ba2df10 Compare September 25, 2018 14:16

jcvenegas changed the title ~~vc: qemu: Use urandom as entropy source.~~ vc: qemu: Add config option to choose entropy source. Sep 25, 2018

jcvenegas force-pushed the urandom-source branch 3 times, most recently from fcc028b to fb6fc8a Compare September 25, 2018 21:13

sboeuf approved these changes Sep 25, 2018

View reviewed changes

egernst reviewed Sep 25, 2018

View reviewed changes

sboeuf reviewed Sep 25, 2018

View reviewed changes

jcvenegas force-pushed the urandom-source branch from fb6fc8a to 23747f2 Compare September 25, 2018 22:35

vc: qemu: Add option to change entropy source

41619e4

This adds a config option to choose the VM entropy source. Fixes: kata-containers#702 Signed-off-by: Jose Carlos Venegas Munoz <jose.carlos.venegas.munoz@intel.com>

jcvenegas force-pushed the urandom-source branch from 23747f2 to 41619e4 Compare September 25, 2018 22:55

egernst approved these changes Sep 25, 2018

View reviewed changes

bergwolf removed the do-not-merge label Sep 26, 2018

bergwolf merged commit 304ec7e into kata-containers:master Sep 26, 2018

grahamwhaley mentioned this pull request Sep 28, 2018

metrics: fast footprint kata-containers/tests#766

Merged

jcvenegas deleted the urandom-source branch January 18, 2019 17:16

jcvenegas restored the urandom-source branch September 30, 2019 18:48

jcvenegas deleted the urandom-source branch January 23, 2020 19:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vc: qemu: Add config option to choose entropy source. #781

vc: qemu: Add config option to choose entropy source. #781

jcvenegas commented Sep 24, 2018 •

edited

jcvenegas commented Sep 24, 2018

katacontainersbot commented Sep 24, 2018

sboeuf Sep 24, 2018

jcvenegas Sep 24, 2018

codecov bot commented Sep 24, 2018 •

edited

laijs commented Sep 25, 2018

WeiZhang555 commented Sep 25, 2018

sboeuf commented Sep 25, 2018

sboeuf commented Sep 25, 2018

katacontainersbot commented Sep 25, 2018

jodh-intel commented Sep 25, 2018

jodh-intel Sep 25, 2018

jcvenegas Sep 25, 2018

jcvenegas commented Sep 25, 2018

katacontainersbot commented Sep 25, 2018

sboeuf commented Sep 25, 2018

sboeuf left a comment

egernst Sep 25, 2018

sboeuf Sep 25, 2018

katacontainersbot commented Sep 25, 2018

jcvenegas commented Sep 25, 2018

katacontainersbot commented Sep 25, 2018

bergwolf commented Sep 26, 2018

vc: qemu: Add config option to choose entropy source. #781

vc: qemu: Add config option to choose entropy source. #781

Conversation

jcvenegas commented Sep 24, 2018 • edited

jcvenegas commented Sep 24, 2018

katacontainersbot commented Sep 24, 2018

sboeuf Sep 24, 2018

Choose a reason for hiding this comment

jcvenegas Sep 24, 2018

Choose a reason for hiding this comment

codecov bot commented Sep 24, 2018 • edited

Codecov Report

laijs commented Sep 25, 2018

WeiZhang555 commented Sep 25, 2018

sboeuf commented Sep 25, 2018

sboeuf commented Sep 25, 2018

katacontainersbot commented Sep 25, 2018

jodh-intel commented Sep 25, 2018

jodh-intel Sep 25, 2018

Choose a reason for hiding this comment

jcvenegas Sep 25, 2018

Choose a reason for hiding this comment

jcvenegas commented Sep 25, 2018

katacontainersbot commented Sep 25, 2018

sboeuf commented Sep 25, 2018

sboeuf left a comment

Choose a reason for hiding this comment

egernst Sep 25, 2018

Choose a reason for hiding this comment

sboeuf Sep 25, 2018

Choose a reason for hiding this comment

katacontainersbot commented Sep 25, 2018

jcvenegas commented Sep 25, 2018

katacontainersbot commented Sep 25, 2018

bergwolf commented Sep 26, 2018

jcvenegas commented Sep 24, 2018 •

edited

codecov bot commented Sep 24, 2018 •

edited