beef up random seed logic, add boot loader entropy privisioning, improve docs about it #13137

poettering · 2019-07-22T16:26:41Z

Fixes for #9428 and #4271

lgtm-com · 2019-07-22T17:30:26Z

This pull request introduces 1 alert when merging c31ec80 into 670fb0b - view on LGTM.com

new alerts:

1 for Missing header guard

mrc0mmand · 2019-07-24T07:48:54Z

CentOS 7 no likey:

[802/2682] Compiling C object 'systemd-random-seed@exe/src_random-seed_random-seed.c.o'.
FAILED: systemd-random-seed@exe/src_random-seed_random-seed.c.o 
cc -Isystemd-random-seed@exe -I. -I.. -Isrc/basic -I../src/basic -Isrc/boot -I../src/boot -Isrc/shared -I../src/shared -Isrc/systemd -I../src/systemd -Isrc/journal -I../src/journal -Isrc/journal-remote -I../src/journal-remote -Isrc/nspawn -I../src/nspawn -Isrc/resolve -I../src/resolve -Isrc/timesync -I../src/timesync -I../src/time-wait-sync -Isrc/login -I../src/login -Isrc/udev -I../src/udev -Isrc/libudev -I../src/libudev -Isrc/core -I../src/core -Isrc/shutdown -I../src/shutdown -I../src/libsystemd/sd-bus -I../src/libsystemd/sd-device -I../src/libsystemd/sd-event -I../src/libsystemd/sd-hwdb -I../src/libsystemd/sd-id128 -I../src/libsystemd/sd-netlink -I../src/libsystemd/sd-network -I../src/libsystemd/sd-resolve -Isrc/libsystemd-network -I../src/libsystemd-network -I../ -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Werror -std=gnu99 -g -Wextra -Werror=undef -Wlogical-op -Wmissing-include-dirs -Wold-style-definition -Wpointer-arith -Winit-self -Wfloat-equal -Wsuggest-attribute=noreturn -Werror=missing-prototypes -Werror=implicit-function-declaration -Werror=missing-declarations -Werror=return-type -Werror=format=2 -Wstrict-prototypes -Wredundant-decls -Wmissing-noreturn -Wshadow -Wendif-labels -Wstrict-aliasing=2 -Wwrite-strings -Werror=overflow -Wnested-externs -Wno-unused-parameter -Wno-missing-field-initializers -Wno-unused-result -Wno-error=nonnull -Wno-maybe-uninitialized -ffast-math -fno-common -fdiagnostics-show-option -fno-strict-aliasing -fvisibility=hidden -fstack-protector -fstack-protector-strong --param=ssp-buffer-size=4 -Werror=shadow -include config.h -g -O1 -fno-omit-frame-pointer -ftrapv -MD -MQ 'systemd-random-seed@exe/src_random-seed_random-seed.c.o' -MF 'systemd-random-seed@exe/src_random-seed_random-seed.c.o.d' -o 'systemd-random-seed@exe/src_random-seed_random-seed.c.o' -c ../src/random-seed/random-seed.c
../src/random-seed/random-seed.c: In function ‘run’:
../src/random-seed/random-seed.c:270:17: error: implicit declaration of function ‘getrandom’ [-Werror=implicit-function-declaration]
                 k = getrandom(buf, buf_size, GRND_NONBLOCK);
                 ^
../src/random-seed/random-seed.c:270:17: error: nested extern declaration of ‘getrandom’ [-Werror=nested-externs]
cc1: all warnings being treated as errors

Arch in KVM timed out during reboot though, will give it a second spin if it's not just a fluke.

poettering · 2019-07-24T08:35:08Z

ah, indeed! thanks for the hint, force pushed a new version which adds the missing prototype definition to that .c file

mrc0mmand · 2019-07-24T11:51:39Z

What's currently failing:

Ubuntu CI fails around installing /bin/bootctl:

D: install any Execs from the service files
I: Attempting to install /bin/bash
I: Attempting to install /bin/bootctl
Makefile:4: recipe for target 'setup' failed
make: Leaving directory '/tmp/autopkgtest.QqpWOh/build.ht8/systemd/test/TEST-32-OOMPOLICY'

Not sure what's going on there, since in some tests it passes, in most, however, it does not.

As for CentOS CI (Arch in KVM), my first attempt in reproducing it locally went nowhere (it just works), yet the random seed initialization seemed to take a little bit more time than usual. I'll try to debug it with the same image we use in CentOS CI, if that's the case. Right now I still have no idea why it takes it more than 5 minutes to reboot (will try to temporarily bump the timeout a bit).

mrc0mmand · 2019-07-24T12:06:17Z

I guess it would be worth pinging @evverx & @ddstreet since the Ubuntu fail looks similarly to something we dealt with a while ago in #12861 (comment) and the patch we used to fix it seems to affect the currently failing code (3cdb93d & 71a0de3)

evverx · 2019-07-24T12:26:12Z

I think @rootbindir@ should be replaced with @rootlibexecdir@ in systemd-boot-system-token.service.in to get past that particular point.

mrc0mmand · 2019-07-24T13:39:28Z

Also, this issue forced me, again, into trying to get the output from serial console into a file (in the Vagrant runs), and I finally got a working PoC. Will try to deploy it ASAP, so we can debug this further.

poettering · 2019-07-24T13:55:13Z

hmm, we installed bootctl to rootlibexecdir? i.e. to /usr/lib? that doesn't look right?

evverx · 2019-07-24T14:00:19Z

Looks like install_rpath : rootlibexecdir misled me. Anyway, to judge from the logs bootctl is installed to /usr/bin on Ubuntu CI while the unit file points to /bin apparently.

mrc0mmand · 2019-07-24T14:07:18Z

@poettering looks like bumping the boot timeout from 5 to 15 minutes helped, as the Arch in KVM is now executing the last test. However, I'd like to debug this further, so if the merge could wait (e.g. to tomorrow), it would be much appreciated. I've prepared a patch for the serial console issue in systemd/systemd-centos-ci#153 and would like to use it to look at what's happening during the failed boots.

poettering · 2019-07-24T14:39:51Z

(btw, the most recent pushed version now uses @bindir@ when referencing the bootctl binary)

poettering · 2019-07-24T14:40:59Z

@mrc0mmand yeah, no hurries, @keszybz suggested to delay the merge of this after v243 (or at least the pre for it), so it will stay open at least until tomorrow

evverx · 2019-07-24T14:57:42Z

Looking at the logs I noticed gcc complained about "incompatible pointer types" on i386

[583/1821] cc -c ../src/boot/efi/linux.c -o src/boot/efi/linux.c.o -Wall -Wextra -std=gnu90 -nostdinc -ggdb -O0 -fpic -fshort-wchar -ffreestanding -fno-strict-aliasing -fno-stack-protector -Wsign-compare -Wno-missing-field-initializers -isystem /usr/include/efi -isystem /usr/include/efi/ia32 -include src/boot/efi/efi_config.h -include version.h
In file included from /usr/include/efi/efi.h:41:0,
                 from ../src/boot/efi/linux.c:3:
../src/boot/efi/linux.c: In function â<80><98>linux_execâ<80><99>:
../src/boot/efi/linux.c:48:60: warning: passing argument 4 of â<80><98>BS->AllocatePagesâ<80><99> from incompatible pointer type [-Wincompatible-pointer-types]
                                 EFI_SIZE_TO_PAGES(0x4000), (UINTN *) &boot_params);
                                                            ^
../src/boot/efi/linux.c:48:60: note: expected â<80><98>EFI_PHYSICAL_ADDRESS * {aka long long unsigned int *}â<80><99> but argument is of type â<80><98>UINTN * {aka unsigned int *}â<80><99>

I'm not sure whether it has anything to do with this PR though. @ddstreet would it be possible to pass --werror to meson as we do everywhere else as far as I know?

poettering · 2019-07-24T15:15:33Z

@evverx yeah, looks like a bug, not introduced by this PR. please open an issue of its own?

ddstreet · 2019-07-24T19:17:04Z

I guess it would be worth pinging @evverx & @ddstreet since the Ubuntu fail looks similarly to something we dealt with a while ago in #12861 (comment) and the patch we used to fix it seems to affect the currently failing code (3cdb93d & 71a0de3)

I may not have all the context, but specifically re: systemd-bless-boot, that's installed into /lib/systemd/ for Ubuntu (and Debian), not /usr/lib/systemd.

would it be possible to pass --werror to meson as we do everywhere else as far as I know?

I think so, for the Ubuntu CI runs, let me see if I can work out how to add that.

mrc0mmand · 2019-07-24T22:04:58Z

After the next force push the Arch in KVM job should provide logs from the serial console, which should make this (hopefully) debug-able. I also reverted the temporary reboot timeout bump, so it's back at 5 minutes, let's see if it's going to show us something useful.

evverx · 2019-07-25T00:05:23Z

I think so, for the Ubuntu CI runs, let me see if I can work out how to add that.

Thank you. Apparently to judge from src/boot/efi/meson.build that part of systemd lives in its own universe where --werror passed to meson doesn't help much but it can be fixed later on the systemd side.

…py pool

That way we can reuse it elsewhere.

…doesn't work There's no reason why writing should work if reading and writing doesn't. Let's simplify this hence. /dev/urandom is generally an r/w device, and everything else would be a serious system misconfiguration.

This makes two major changes to the way systemd-random-seed operates: 1. We now optionally credit entropy if this is configured (via an env var). Previously we never would do that, with this change we still don't by default, but it's possible to enable this if people acknowledge that they shouldn't replicate an image with a contained random seed to multiple systems. Note that in this patch crediting entropy is a boolean thing (unlike in previous attempts such as systemd#1062), where only a relative amount of bits was credited. The simpler scheme implemented here should be OK though as the random seeds saved to disk are now written only with data from the kernel's entropy pool retrieved after the pool is fully initialized. Specifically: 2. This makes systemd-random-seed.service a synchronization point for kernel entropy pool initialization. It was already used like this, for example by systemd-cryptsetup-generator's /dev/urandom passphrase handling, with this change it explicitly operates like that (at least systems which provide getrandom(), where we can support this). This means services that rely on an initialized random pool should now place After=systemd-random-seed.service and everything should be fine. Note that with this change sysinit.target (and thus early boot) is NOT systematically delayed until the entropy pool is initialized, i.e. regular services need to add explicit ordering deps on this service if they require an initialized random pool. Fixes: systemd#4271 Replaces: systemd#10621 systemd#4513

…terface

Fixes: systemd#9428

poettering · 2019-07-25T16:48:39Z

Force pushed a new version. Addresses all issues @keszybz found, except for the xattr thing, see above for the rationale.

I also made one more change in light of @mrc0mmand's trouble: i dropped the ordering between systemd-random-seed.service and sysinit.target. This means we are not going to delay boot anymore until the pool is initialized (since this apparently caused major delays on those VMs). This means not only early-boot need to order themselves after the service if they require an initialized pool, but regular services too. i.e. it's still nice to have the sycnrhonization pool, but it's only one if people actually use it. This should remove the huge delay on @mrc0mmand's CI machines.

(Apparently those CI machines have RDRAND but no TRUST_CPU kerne config option set?)

poettering · 2019-07-25T16:54:53Z

thanks a ton for the thorough review, @keszybz, of course!

mrc0mmand · 2019-07-25T18:35:32Z

I also made one more change in light of @mrc0mmand's trouble: i dropped the ordering between systemd-random-seed.service and sysinit.target. This means we are not going to delay boot anymore until the pool is initialized (since this apparently caused major delays on those VMs). This means not only early-boot need to order themselves after the service if they require an initialized pool, but regular services too. i.e. it's still nice to have the sycnrhonization pool, but it's only one if people actually use it. This should remove the huge delay on @mrc0mmand's CI machines.

Thanks! The random pool init still takes ~25 seconds, but if it stays this way, I have no problem with that:

[*     ] A start job is running for Load/Save Random Seed (7s / 10min)
^M[**    ] A start job is running for Load/Save Random Seed (8s / 10min)
^M[***   ] A start job is running for Load/Save Random Seed (8s / 10min)
^M[ ***  ] A start job is running for Load/Save Random Seed (9s / 10min)
^M[  *** ] A start job is running for Load/Save Random Seed (10s / 10min)
^M[   ***] A start job is running for Load/Save Random Seed (10s / 10min)
^M[    **] A start job is running for Load/Save Random Seed (11s / 10min)
^M[     *] A start job is running for Load/Save Random Seed (11s / 10min)
^M[    **] A start job is running for Load/Save Random Seed (12s / 10min)
^M[   ***] A start job is running for Load/Save Random Seed (12s / 10min)
^M[  *** ] A start job is running for Load/Save Random Seed (13s / 10min)
^M[ ***  ] A start job is running for Load/Save Random Seed (14s / 10min)
^M[***   ] A start job is running for Load/Save Random Seed (14s / 10min)
^M[**    ] A start job is running for Load/Save Random Seed (15s / 10min)
^M[*     ] A start job is running for Load/Save Random Seed (15s / 10min)
^M[**    ] A start job is running for Load/Save Random Seed (16s / 10min)
^M[***   ] A start job is running for Load/Save Random Seed (17s / 10min)
^M[ ***  ] A start job is running for Load/Save Random Seed (17s / 10min)
^M[  *** ] A start job is running for Load/Save Random Seed (18s / 10min)
^M[   ***] A start job is running for Load/Save Random Seed (18s / 10min)
^M[    **] A start job is running for Load/Save Random Seed (19s / 10min)
^M[     *] A start job is running for Load/Save Random Seed (20s / 10min)
^M[    **] A start job is running for Load/Save Random Seed (20s / 10min)
^M[   ***] A start job is running for Load/Save Random Seed (21s / 10min)
^M[  *** ] A start job is running for Load/Save Random Seed (21s / 10min)
^M[ ***  ] A start job is running for Load/Save Random Seed (22s / 10min)
^M[***   ] A start job is running for Load/Save Random Seed (22s / 10min)
^M[**    ] A start job is running for Load/Save Random Seed (23s / 10min)
^M[*     ] A start job is running for Load/Save Random Seed (24s / 10min)
^M[**    ] A start job is running for Load/Save Random Seed (24s / 10min)
^M[***   ] A start job is running for Load/Save Random Seed (25s / 10min)
^M[ ***  ] A start job is running for Load/Save Random Seed (25s / 10min)
^M[  *** ] A start job is running for Load/Save Random Seed (26s / 10min)
^M[  OK  ] Started Load/Save Random Seed.
[  OK  ] Reached target System Initialization.

(Apparently those CI machines have RDRAND but no TRUST_CPU kerne config option set?)

The Arch machines are QEMU VMs (with KVM) and they don't seem to have RDRAND according to their CPU flags.

As for the TRUST_CPU kernel option, it's indeed not set:

# zgrep TRUST_CPU /proc/config.gz
# CONFIG_RANDOM_TRUST_CPU is not set

mrc0mmand · 2019-07-25T18:58:16Z

@poettering out of curiosity I tried to pass through the host's /dev/random into the VM in systemd/systemd-centos-ci#154, and the random init pool initialization time is back to "normal":

         Starting Load/Save Random Seed...
         Starting Create System Users...
[  OK  ] Started Load/Save Random Seed.

so I guess we should make that change permanent, as the QEMU's entropy pool is not enough?

poettering · 2019-07-26T05:41:38Z

please pass through /dev/urandom though, but yes, always do that.

mrc0mmand · 2019-07-26T07:21:59Z

please pass through /dev/urandom though, but yes, always do that.

FWIK /dev/urandom is handled automatically, but for /dev/random additional shenanigans are required. I'll extend the CentOS CI PR to the other Arch VMs, so it works everywhere as expected.

mrc0mmand · 2019-07-26T10:33:27Z

In the latest CentOS CI KVM run the boot time is back to "normal", so there's nothing blocking this PR from the CentOS CI point of view.

keszybz

I didn't do a full re-review, just the changed parts. There's still some wording changes, but I think we can just as well do that in a later PR.

keszybz · 2019-07-26T10:34:16Z

src/boot/bootctl.c

+
+        tmp = mfree(tmp);
+
+        log_info("Successfully written random seed file %s with %zu bytes.", path, sz);


That's grammatically incorrect. Maybe "Random seed file %s successfully written with %zu bytes".

keszybz · 2019-07-26T10:40:27Z

man/bootctl.xml

-        <filename>/boot/</filename>, and <filename>/boot/efi</filename> are checked in turn.  It is recommended to mount
-        the ESP to <filename>/efi/</filename>, if possible.</para></listitem>
+        <filename>/boot/</filename>, and <filename>/boot/efi/</filename> are checked in turn.  It is
+        recommended to mount the ESP to <filename>/efi/</filename>, if possible.</para></listitem>


I wonder if the time has come to relax this, and accept that everybody mounts it on /boot/efi...

floppym

Note that with this change sysinit.target (and thus early boot) is NOT systematically delayed until the entropy pool is initialized

@poettering systemd-random-seed.service still has Before=sysinit.target. Is this a mistake?

poettering added documentation pid1 util-lib sd-boot/sd-stub/bootctl random-seed labels Jul 22, 2019

poettering force-pushed the efi-random branch 4 times, most recently from 2742dbd to 8a19372 Compare July 24, 2019 07:03

poettering force-pushed the efi-random branch from 8a19372 to 588f308 Compare July 24, 2019 08:31

poettering mentioned this pull request Jul 24, 2019

RDRAND on AMD CPUs does not work #11810

Closed

poettering force-pushed the efi-random branch from 588f308 to 2156ec5 Compare July 24, 2019 14:27

evverx mentioned this pull request Jul 24, 2019

gcc seems to be complaining about "incompatible pointer types" in boot/efi/linux.c on i386 #13174

Closed

poettering added 15 commits July 25, 2019 18:16

core: take random seed from boot loader and credit it to kernel entro…

c18ecf0

…py pool

random-seed: move pool size determination to random-util.[ch]

3e155eb

That way we can reuse it elsewhere.

bootctl: add new verb for initializing a random seed in the ESP

e44c322

bootctl: show random seed state

d6e9a34

units: automatically initialize the system token if that makes sense

d985064

random-seed: reduce scope of variable

15d961b

docs: document new random seed EFI vars as part of the boot loader in…

c7bb4df

…terface

man: document the systemd-random-seed rework

39867bb

bootctl: add is-installed verb

a2aa605

Fixes: systemd#9428

man: extend on the --print-boot-path description a bit

7fb0c61

docs: add longer document about systemd and random number seeds

93f5910

TODO: remove apparently fixed issue from TODO

341fd87

update TODO

312dc15

poettering force-pushed the efi-random branch from 357dd43 to 312dc15 Compare July 25, 2019 16:45

keszybz approved these changes Jul 26, 2019

View reviewed changes

keszybz merged commit 47685d9 into systemd:master Jul 26, 2019

evverx mentioned this pull request Jul 30, 2019

systemd seems to be failing to compile on Debian Stretch #13226

Closed

cgwalters mentioned this pull request Jul 30, 2019

better entropy handling openshift/machine-config-operator#854

Closed

floppym reviewed Aug 3, 2019

View reviewed changes

mbiebl mentioned this pull request Aug 4, 2019

autopkgtest failures with 243-rc1, systemd-random-seed.service timeout during boot #13252

Closed

ddstreet mentioned this pull request Aug 13, 2019

Fix build warnings, so Ubuntu CI can pass --werror to meson #13317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

beef up random seed logic, add boot loader entropy privisioning, improve docs about it #13137

beef up random seed logic, add boot loader entropy privisioning, improve docs about it #13137

poettering commented Jul 22, 2019

lgtm-com bot commented Jul 22, 2019

mrc0mmand commented Jul 24, 2019

poettering commented Jul 24, 2019

mrc0mmand commented Jul 24, 2019 •

edited

mrc0mmand commented Jul 24, 2019 •

edited

evverx commented Jul 24, 2019

mrc0mmand commented Jul 24, 2019

poettering commented Jul 24, 2019

evverx commented Jul 24, 2019

mrc0mmand commented Jul 24, 2019

poettering commented Jul 24, 2019

poettering commented Jul 24, 2019

evverx commented Jul 24, 2019 •

edited

poettering commented Jul 24, 2019

ddstreet commented Jul 24, 2019

mrc0mmand commented Jul 24, 2019

evverx commented Jul 25, 2019

poettering commented Jul 25, 2019

poettering commented Jul 25, 2019

mrc0mmand commented Jul 25, 2019

mrc0mmand commented Jul 25, 2019 •

edited

poettering commented Jul 26, 2019

mrc0mmand commented Jul 26, 2019

mrc0mmand commented Jul 26, 2019

keszybz left a comment

keszybz Jul 26, 2019

keszybz Jul 26, 2019

floppym left a comment


		tmp = mfree(tmp);

		log_info("Successfully written random seed file %s with %zu bytes.", path, sz);

beef up random seed logic, add boot loader entropy privisioning, improve docs about it #13137

beef up random seed logic, add boot loader entropy privisioning, improve docs about it #13137

Conversation

poettering commented Jul 22, 2019

lgtm-com bot commented Jul 22, 2019

mrc0mmand commented Jul 24, 2019

poettering commented Jul 24, 2019

mrc0mmand commented Jul 24, 2019 • edited

mrc0mmand commented Jul 24, 2019 • edited

evverx commented Jul 24, 2019

mrc0mmand commented Jul 24, 2019

poettering commented Jul 24, 2019

evverx commented Jul 24, 2019

mrc0mmand commented Jul 24, 2019

poettering commented Jul 24, 2019

poettering commented Jul 24, 2019

evverx commented Jul 24, 2019 • edited

poettering commented Jul 24, 2019

ddstreet commented Jul 24, 2019

mrc0mmand commented Jul 24, 2019

evverx commented Jul 25, 2019

poettering commented Jul 25, 2019

poettering commented Jul 25, 2019

mrc0mmand commented Jul 25, 2019

mrc0mmand commented Jul 25, 2019 • edited

poettering commented Jul 26, 2019

mrc0mmand commented Jul 26, 2019

mrc0mmand commented Jul 26, 2019

keszybz left a comment

Choose a reason for hiding this comment

keszybz Jul 26, 2019

Choose a reason for hiding this comment

keszybz Jul 26, 2019

Choose a reason for hiding this comment

floppym left a comment

Choose a reason for hiding this comment

mrc0mmand commented Jul 24, 2019 •

edited

mrc0mmand commented Jul 24, 2019 •

edited

evverx commented Jul 24, 2019 •

edited

mrc0mmand commented Jul 25, 2019 •

edited