RFC: allow crediting the seed file for some entropy by Villemoes · Pull Request #10621 · systemd/systemd

Villemoes · 2018-11-02T10:25:15Z

This provides the administrator with a knob so that the seed file can be credited with providing some amount of entropy. My primary motivation is for use in embedded products [1], but there are also reports (e.g. https://lore.kernel.org/lkml/20181030001807.7wailpm37mlinsli@breakpoint.cc/) suggesting that it might be useful for desktops.

My idea is that the administrator tweaks the random-seed.service file to add the -c option, but I'm not really sure what the most convenient interface is. For our use case, just the first patch would be sufficient, since we could then just patch in the appropriate number with a sed script during the BSP build.

This probably will need some documentation update, but I'll wait with that until getting some feedback and knowing whether this has any chance of being accepted.

[1] In the embedded products we support, we often see (and get complaints) that full system initialization takes a very long time. Part of that is due to the lack of good entropy sources, especially during the early phase. Most, but not all, modern hardware have random generators built in, but legacy hardware still needs to be maintained.

poettering · 2018-11-03T13:28:12Z

Also see #4271, #4513.

poettering · 2018-11-03T13:29:58Z

So, for the previously discussed reasons (see those linked github issues/prs) I very much not convinced we should default to crediting the entropy. You patch makes this opt-in though, and that definitely should be OK.

I think it would be nicer to do this keyed off an env var though, they are easier to add with a unit file drop-in. Maybe even in addition to the cmdline parameter...

poettering · 2018-11-03T13:37:47Z

src/random-seed/random-seed.c

        _cleanup_close_ int seed_fd = -1, random_fd = -1;
        bool read_seed_file, write_seed_file;
-        _cleanup_free_ void* buf = NULL;
+        unsigned entropy_count = 0;


hmm, this is weird, why "unsigned"? the kernel structure actually expects an "int" here (which appears kinda sloppy to me, but anyway...). And philosophically it appears to me this should be a size_t, as ideally if we pass fully random data in then the entropy_count should equal the buffer size, no?

Dunno. I started with a size_t, then I think I had some reason to change it, though I don't recall why. The kernel caps it at, I think, 4096 anyway (or some other similar smallish number), so it doesn't matter too much.

still, we generally care about type issues in our codebase. If the kernel APIs are a bit sloppy with types then this doesn't mean we should be sloppy too. Please pick the most appropriate type always.

poettering · 2018-11-03T13:38:24Z

src/random-seed/random-seed.c


-        buf = malloc(buf_size);
-        if (!buf) {
+        info = malloc(sizeof(*info) + buf_size);


should be:

info = malloc(offsetof(struct rand_pool_info, buf) + buf_size);

So, in this case offsetof() and sizeof() are the same, so I won't bikeshed too much over this. But FTR, the reason I prefer the sizeof() idiom is that it ensures one allocates a full struct, even if buf_size is small. Consider something like struct { long a; char b; char c[]; }. Allocating using offsetof is error-prone if the allocation is hit with a memset(..., sizeof()), or one uses struct assignment. The fully correct thing is to do the offsetof thing, then max() with sizeof(*info), but that's so much of a mouthful that it requires a helper macro.

Well, the way I understood C sizeof() and offsetof(…) will actually result in the very same results in cases like this. however, one makes very clear what is going on here I think for the reader. hence, this is really about readability, nothing else...

not addresed yet

poettering · 2018-11-03T13:40:05Z

src/random-seed/random-seed.c

+        else {
+                log_error("Unknown verb '%s'.", argv[optind]);
+                return -EINVAL;
+        }


(i appreciate the getoptification, but in an ideal world we'd go even further and use verbs.[ch] for dispatching the verbs here... but i guess this is unrelated to this PR)

Yeah, and if the interface ends up being an env var, this goes away, so I won't spend too much time polishing this.

poettering · 2018-11-03T14:05:28Z

Hmm, so I find it strange to make the numeric entropy count configurable like this, given that it needs to match the properties of the actual random seed data currently on disk. i.e. instead of making the numeric value an admin-facing option possibly not matching the size and quality of the random seed read from disk (which is an automatic system-maintained data element) it should be a boolean I guess and the actual entropy count should be derived from the stored seed. Or what am I missing?

If we assume perfect entropy of the data read from disk we could simply derive the entropy count to credit from the size of the data: just multiply it by 8 to convert bytes to bits (the unit of entropy_count is unfortunately not even documented even though it appears to be bits, grrrrr). Now, I am not sure how the Linux RNG subsystem in this regard really works, and these things are all not documented, but if we read data during shutdown from /dev/urandom, I presume the entropy is actually much lower than the actual byte size we are reading? if so, then we should probably read the current actual entropy from the kernel and store it alongside the seed file we are writing, so that when we come back on next boot we don't request to credit more entropy than the seed actually contains... There appears to be an ioctl (RNDGETENTCNT) that allows us to retrieve the current amount of entropy, so maybe it is as easy as calling that and storing that somewhere on close to the random seed file (maybe as an xattr, so that it is inherently part of the file, and removed/recreated always alongside with it, and can't get out of sync). Of course, ideally we'd atomically read the seed data and the entropy value from the kernel in one go, so that things aren't racy, but I figure we are OK ignoring this, we have to work with what we got,...

That said, I am not sure I grok this all properly even. The man page has example shell scripts explaining what to do during shutdown/reboot. And they of course do not credit the entropy at all. If this is something that should be done these days, then great, but it would be very welcome if the man page would actually say so...

Villemoes · 2018-11-03T22:20:11Z

So, for the previously discussed reasons (see those linked github issues/prs) I very much not convinced we should default to crediting the entropy. You patch makes this opt-in though, and that definitely should be OK.

Fully agree, this should not be done by default, and requires explicit opt-in.

I think it would be nicer to do this keyed off an env var though, they are easier to add with a unit file drop-in. Maybe even in addition to the cmdline parameter...

Yes, as I wrote, I wasn't sure about the best interface. An env var would certainly also work, and would avoid having to do the full getoptization (which then also somewhat compels one to implement -h etc.). Even from the shell (should anyone call systemd-random-seed manually), there's not much difference between 'ENTROPY_COUNT=123 systemd-random-seed load' and 'systemd-random-seed -c 123 load'. Is there some naming scheme/namespace to follow for the env var, or would ENTROPY_COUNT be fine?

Villemoes · 2018-11-03T22:44:31Z

Hmm, so I find it strange to make the numeric entropy count configurable like this, given that it needs to match the properties of the actual random seed data currently on disk. i.e. instead of making the numeric value an admin-facing option possibly not matching the size and quality of the random seed read from disk (which is an automatic system-maintained data element) it should be a boolean I guess and the actual entropy count should be derived from the stored seed. Or what am I missing?

In an ideal world, you're right. The problem is, one never (not even the kernel) knows the actual amount of entropy. So it will always boil down to a question of not how much entropy is there, but how much does the admin trust the file to provide/contain. And for that reason, it does make sense to make it an admin knob - essentially, he gets to do the tradeoff between boot time and security.

If we assume perfect entropy of the data read from disk we could simply derive the entropy count to credit from the size of the data: just multiply it by 8 to convert bytes to bits (the unit of entropy_count is unfortunately not even documented even though it appears to be bits, grrrrr).

Yup, I went to the one true place for documentation on this (i.e., the kernel source code).

Now, I am not sure how the Linux RNG subsystem in this regard really works, and these things are all not documented, but if we read data during shutdown from /dev/urandom, I presume the entropy is actually much lower than the actual byte size we are reading? if so, then we should probably read the current actual entropy from the kernel and store it alongside the seed file we are writing, so that when we come back on next boot we don't request to credit more entropy than the seed actually contains...

I don't think that will work. Just because you read 16 bytes out of /dev/urandom doesn't decrement /proc/sys/kernel/random/entropy_avail by 128. Again, this is really just a judgment call for the admin to make.

There appears to be an ioctl (RNDGETENTCNT) that allows us to retrieve the current amount of entropy, so maybe it is as easy as calling that and storing that somewhere on close to the random seed file (maybe as an xattr, so that it is inherently part of the file, and removed/recreated always alongside with it, and can't get out of sync). Of course, ideally we'd atomically read the seed data and the entropy value from the kernel in one go, so that things aren't racy, but I figure we are OK ignoring this, we have to work with what we got,...

It might make sense to sample /proc/sys/kernel/random/entropy_avail when we save the new seed file [obviously, on the initial load, sample before load] just to get a ballpark estimate of how good the randomness one reads at that point in time is, but using it as some exact figure is wrong. Not sure what to do with that quality assesment, though. As for storage, please just add another 4 bytes or whatnot to the file, instead of relying on the file system having xattrs.

That said, I am not sure I grok this all properly even. The man page has example shell scripts explaining what to do during shutdown/reboot. And they of course do not credit the entropy at all. If this is something that should be done these days, then great, but it would be very welcome if the man page would actually say so...

So, this is not "something that should be done these days period". Please see the recent thread on (among other lists) lkml, "Bug#912087: openssh-server: Slow startup after the upgrade to 7.9p1", in particular Ted Ts'o (the kernel random maintainer) in https://lore.kernel.org/lkml/20181030205136.GB6236@thunk.org/ :

In any case, if Debian wants to ship a program which reads a seed file
and uses it to initialize the random pull assuming that it's
trustworthy via the RNDADDENTROPY ioctl, that's not an insane thing to
do. My recommendation would be to make it be configurable, however,
just as whether we trust RDRAND should be trusted (in isolation) to
initialize the CRNG.

And that is precisely what I'm trying to do here :)

sebastianas · 2018-11-04T17:00:40Z

So, for the previously discussed reasons (see those linked github issues/prs) I very much not convinced we should default to crediting the entropy. You patch makes this opt-in though, and that definitely should be OK.

I think it would be nicer to do this keyed off an env var though, they are easier to add with a unit file drop-in. Maybe even in addition to the cmdline parameter...

Please don't do this even optionally. This shouldn't be an issue (after talking with Ted) on "recent" machines. KVM hosts can add a hw-rnd device. Everything embedded that lacks good entropy will turn this on without thinking…

poettering · 2018-11-05T13:19:10Z

Yes, as I wrote, I wasn't sure about the best interface. An env var would certainly also work, and would avoid having to do the full getoptization (which then also somewhat compels one to implement -h etc.).

There's nothing wrong with getoptization... Given that you already did that work for that, please just keep it in, even if it's not strictly necessary now. I mean, a --help text is always useful.

Even from the shell (should anyone call systemd-random-seed manually), there's not much difference between 'ENTROPY_COUNT=123 systemd-random-seed load' and 'systemd-random-seed -c 123 load'. Is there some naming scheme/namespace to follow for the env var, or would ENTROPY_COUNT be fine?

Please prefix the env var with SYSTEMD_. Also, document it in docs/ENVIRONMENT.md which is where we document most env vars.

poettering · 2018-11-05T13:20:50Z

In an ideal world, you're right. The problem is, one never (not even the kernel) knows the actual amount of entropy. So it will always boil down to a question of not how much entropy is there, but how much does the admin trust the file to provide/contain. And for that reason, it does make sense to make it an admin knob - essentially, he gets to do the tradeoff between boot time and security.

Well, a file that contains a single byte should never be allowed to credit more than 8bit of entropy...

I am pretty sure the very least we should do is use the file size as upper ceiling for the bits to credit.

poettering · 2018-11-05T13:22:38Z

I don't think that will work. Just because you read 16 bytes out of /dev/urandom doesn't decrement /proc/sys/kernel/random/entropy_avail by 128. Again, this is really just a judgment call for the admin to make.

But I am pretty sure the entropy the kernel supports should at least be considered an upper ceiling for what we credit

poettering · 2018-11-05T13:26:03Z

As for storage, please just add another 4 bytes or whatnot to the file, instead of relying on the file system having xattrs.

I do not see how we'd do that without breaking compat. We might end up reading files written out from older versions/other tools which are not aware of the entropy counter, and we'd then credit a random amount of entropy...

All writable, persistent file systems that might be suitable for /var I know do support xattrs just fine. Moreover, we should fail gracefully in case we can't set the xattr, and when we read the random seed and see no xattr set we should not credit any entropy. This basically means we are fine with any kind of /var, and if you really use an fs there that can't do xattr, then all you lose is the entropy crediting, which shouldn't be much of a loss...

poettering · 2018-11-05T13:28:20Z

And that is precisely what I'm trying to do here :)

well, those comments don't really answer the questions I raised above. I'd prefer if we'd have clearer information on all this, i.e. whether to care about the entropy when reading, or whether that doesn't matter.

Villemoes · 2018-11-05T20:29:34Z

In an ideal world, you're right. The problem is, one never (not even the kernel) knows the actual amount of entropy. So it will always boil down to a question of not how much entropy is there, but how much does the admin trust the file to provide/contain. And for that reason, it does make sense to make it an admin knob - essentially, he gets to do the tradeoff between boot time and security.

Well, a file that contains a single byte should never be allowed to credit more than 8bit of entropy...

I am pretty sure the very least we should do is use the file size as upper ceiling for the bits to credit.

Yes, which I'm already doing... actually not using the file size, but the number of bytes we read from the file, in the unlikely case they're different.

keszybz · 2018-11-08T08:59:52Z

What about defining the variable (SYSTEMD_ENTROPY_VALUE=?) to be a float, between 0 and 1 inclusive, that would specify how much trust to put in the bytes in the file. So SYSTEMD_ENTROPY_VALUE=1 would mean that every byte in the file counts as 8 bits of entropy, SYSTEMD_ENTROPY_VALUE=0.5 would mean that every byte is 4 bits, etc.

poettering · 2018-11-08T09:04:02Z

here's an idea regarding how to store the amount of entropy the kernel had during shutdown on disk in a backwards compatible way: we could simply size the resulting seed file according to how much entropy the kernel reported: if the kernel only has a few bits, we'd only write out a short seed file out, and if it has many we generate a longer seed file. When booting up we'd credit the file size. All subject to some upper bounds, and subject to the fraction multiplier, as @keszybz suggested.

poettering · 2018-11-08T09:05:34Z

btw, I figure we should use getrandom() with GRND_NONBLOCK for acquiring the seed to write out during shutdown. With that we should see EAGAIN when there's no entropy around and could then remove the seed file.

keszybz · 2018-11-08T09:14:10Z

we could simply size the resulting seed file according to how much entropy the kernel reported: if the kernel only has a few bits, we'd only write out a short seed file out, and if it has many we generate a longer seed file.

If the kernel has low-quality random bytes, the randomness will be spread evenly over the bytes we get. So if we think what we got is 50% good, and we just took the first half, then we'd actually store half of the entropy we got. So I don't think this is a good idea.

Villemoes · 2018-11-08T09:38:42Z

What about defining the variable (SYSTEMD_ENTROPY_VALUE=?) to be a float, between 0 and 1 inclusive, that would specify how much trust to put in the bytes in the file. So SYSTEMD_ENTROPY_VALUE=1 would mean that every byte in the file counts as 8 bits of entropy, SYSTEMD_ENTROPY_VALUE=0.5 would mean that every byte is 4 bits, etc.

I thought of something similar: If we're going to store an estimate of how good the randomness we got when creating the seed file was, it doesn't make sense to have the admin tell some absolute number to use for the entropy amount. Instead, let's just multiply the estimated entropy by the float (I'd actually rather do it as a percentage, to avoid floating point when not strictly necessary).

As for getting that estimate, I'd do a RNDGETENTCNT before and after reading the bytes from /dev/urandom, and use the minimum of the two. For the 'random-seed save', that's the number I'd put it. For 'random-seed load', we need to take into account that we just manually adjusted the entropy estimate, so I'd deduct the info.entropy_count we passed in - the kernel uses a sub-additive formula (see the "Credit: we have to account for the possibility of overwriting already present entropy. " comment in the kernel source), so this should ensure we deduct at least what we were credited for providing. Of course, this "heuristic" fails when the board is shut down shortly after boot.

poettering · 2018-11-08T09:50:21Z

regarding the GRND_NONBLOCK thing I suggested above. Would love to see #10676 reviewed and merged first (reviews always welcome, @Villemoes hint hint ;-)). After that we can just issue genuine_random_bytes(… RANDOM_DONT_DRAIN). The benefit over calling getrandom() directly is that it will do an automatic fall back to /dev/urandom internally already if getrandom() is not available on the local kernel.

Villemoes · 2018-11-08T09:55:13Z

So, for the previously discussed reasons (see those linked github issues/prs) I very much not convinced we should default to crediting the entropy. You patch makes this opt-in though, and that definitely should be OK.
I think it would be nicer to do this keyed off an env var though, they are easier to add with a unit file drop-in. Maybe even in addition to the cmdline parameter...

Please don't do this even optionally. This shouldn't be an issue (after talking with Ted) on "recent" machines. KVM hosts can add a hw-rnd device.

I don't understand the "recent machines" argument. Lots of hardware doesn't get replaced every other year, for a variety of practical, economical, technical and/or environmental reasons. We're regularly servicing devices deployed 5,10,15 years ago that are expected to be in use for another similar timespan.

Everything embedded that lacks good entropy will turn this on without thinking…

I agree that this provides admins with rope. But I don't agree that all admins and embedded developers will necessarily use that as an opportunity to hang themselves.

poettering · 2018-11-08T10:07:18Z

so here's one more problem with all this: we have kind of a feedback loop here during early boot. So far, at boot, we read the random seed of disk, push it into /dev/urandom, then pull out a new random seed and write that to disk, so that we know for sure that on next boot we'll have a different seed in place we push in. this is great as long as we don#t actually credit the entropy for it. But as soon as we do that, then this falls apart because the stuff we pull out again will likely result from just the entropy we pushed in and this means the entropy in the kernel is gone again... Or what am I missing there?

I have the suspicion that the least we have to do is use blocking getrandom() when reading back the seed after having written it and remove the seed file in between to mark it invalidated. i.e.:

at start:

read seed off disk
delete seed on disk
write seed to to /dev/urandom (if crediting the entropy is enabled we do so at this point)
acquire a new seed (if crediting the entropy is enabled in blocking getrandom() mode, otherwise in /dev/urandom read mode, i.e. without blocking and with possibly returning data from a half initialized pool)
write new seed to disk

at stop:

acquire a new seed (if crediting the entropy is enabled in non-blocking getrandom() mode — which might result in EAGAIN —, otherwise in /dev/urandom read mode, i.e. non-blocking mode possibly returning data from a half initialized pool)
if we got EAGAIN leave the seed as it is, otherwise replace it

poettering · 2018-11-08T10:08:33Z

Instead, let's just multiply the estimated entropy by the float (I'd actually rather do it as a percentage, to avoid floating point when not strictly necessary).

I prefer a percentage too I guess, and we already have a nice parser for that in parse_percent()

poettering · 2018-11-08T10:09:58Z

I agree that this provides admins with rope. But I don't agree that all admins and embedded developers will necessarily use that as an opportunity to hang themselves.

well, if we hide this in some env var i think doing this is fine. We generally use env vars for hiding options that should not be considered primary API but just some expert/debugging tool.

Villemoes · 2018-11-08T10:33:38Z

here's an idea regarding how to store the amount of entropy the kernel had during shutdown on disk in a backwards compatible way: we could simply size the resulting seed file according to how much entropy the kernel reported: if the kernel only has a few bits, we'd only write out a short seed file out, and if it has many we generate a longer seed file. When booting up we'd credit the file size. All subject to some upper bounds, and subject to the fraction multiplier, as @keszybz suggested.

No, please don't do it like that. I retract my objection to using xattr. Just because the kernel's estimate of the amount of entropy is 8 bits, the internal state of the pool can still be in 2^4096 different states (some more likely than others, perhaps, but still unknown to any would-be attacker), so we should create a file that has at least 4096 bits. If we just created a one-byte file, regardless of whether we're crediting the entropy on next boot or not, we're throwing away a lot of actual entropy, making it easier to guess the internal state of the pool on next boot (i.e., if everything else in the boot process is completely predictable, it could only be in 256 different states).

Villemoes · 2018-11-08T10:59:35Z

so here's one more problem with all this: we have kind of a feedback loop here during early boot. So far, at boot, we read the random seed of disk, push it into /dev/urandom, then pull out a new random seed and write that to disk, so that we know for sure that on next boot we'll have a different seed in place we push in. this is great as long as we don#t actually credit the entropy for it. But as soon as we do that, then this falls apart because the stuff we pull out again will likely result from just the entropy we pushed in and this means the entropy in the kernel is gone again... Or what am I missing there?

So I hinted at this problem in my other comment, but it probably crossed yours. As I wrote, we can easily account for that by deducting the entropy count we added. The only problem is if shutdown happens shortly after boot, where the pool would still more or less only contain the stuff we added during boot, but we wouldn't correct for that. (OTOH, I'm not really sure that's a problem, or that we should even do the deducting in the load+save case - the entropy estimate is a measure of how hard it is to guess the contents of the internal state, and it doesn't really matter if the contributions came from a lava lamp or the admin saying "these bytes are $x random" or some combination).

I don't think there's much point in using getrandom() if we're reading the entropy estimate with an ioctl() anyway. getrandom() only differs from direct reads from /dev/random or /dev/urandom in that one can do a potentially-blocking-at-boot read from /dev/urandom, but as soon as the entropy estimate hits 256 bits once (which it's likely to do if we credit some entropy), getrandom() becomes equivalent to reading from /dev/urandom.

This reworks random-seed to use the RNDADDENTROPY ioctl. With an entropy_count of 0, this is exactly equivalent to writing the bytes to /dev/urandom, so this patch in itself does not change behaviour. However, in subsequent patches we will store an estimate of the amount of entropy of the random data in an xattr associated to the file, and teach systemd-random-seed to take an option --entropy-credit <ratio> (and/or environment variable SYSTEMD_ENTROPY_CREDIT) to indicate how much of that entropy should be credited to the the kernel's random pool.

This adds parse_argv() and help() boilerplate in preparation for teaching the command the --entropy-credit option.

Use the ioctl RNDGETENTCNT to get an estimate of how good the random data we read from the kernel is, and store it in an xattr in the random-seed file.

Villemoes · 2019-01-09T07:56:38Z

So I tried doing the xattr thing, this is roughly how it would look. Documentation omitted until there's consensus on the approach.

poettering · 2019-01-17T23:43:21Z

src/random-seed/random-seed.c

+                entropy_credit = parse_permille(e);
+                if (entropy_credit < 0) {
+                        log_warning("Invalid value '%s' of 'SYSTEMD_ENTROPY_CREDIT'. Ignoring.", e);
+                        entropy_credit = 0;


please assign the result of parse_permille() to a temporary variable first, and when it's not a failure then assign it to arg_entropy_credit. That way, should eventually the default for arg_entropy_credit change we only have to change it at the top of the file, and not here again.

poettering · 2019-01-17T23:43:27Z

src/random-seed/random-seed.c

        ACTION_LOAD,
        ACTION_SAVE,
 } arg_action = ACTION_NONE;
+static int entropy_credit;


please prefix with "arg_", we generally do that for all variables directly mapping to cmdline args.

also, to make this explicit, could you explicitly initialize it to zero (yes, i know, that's not strictly necessary, but I think it makes clearer for static variables what their default is)

please also rename this in a way it's clear this is permille, i.e. arg_entropy_credit_permille or so.

poettering · 2019-01-17T23:44:34Z

src/random-seed/random-seed.c

+        if (e) {
+                entropy_credit = parse_permille(e);
+                if (entropy_credit < 0) {
+                        log_warning("Invalid value '%s' of 'SYSTEMD_ENTROPY_CREDIT'. Ignoring.", e);


parse_permille() returns an error. Please pass that to log_warning_errno(), in case structured logging is use.

poettering · 2019-01-17T23:45:15Z

src/random-seed/random-seed.c

+                                log_warning("Ignoring invalid argument to --entropy-credit=: %s.",
+                                            optarg);
+                                entropy_credit = 0;
+                        }


as above, please parse into a temporary variable r first, then assign to arg_entropy_credit only on success. And please pass the error to log_warning_errno()

poettering · 2019-01-17T23:46:12Z

src/random-seed/random-seed.c

                        entropy_count = 0;
                        entropy_count = MIN(entropy_count, 8*k);
+                        entropy_count *= entropy_credit;
+                        entropy_count /= 1000;


why do this in four lines? that's needlessly verbose, no?

poettering · 2019-01-17T23:56:53Z

src/random-seed/random-seed.c

+                return 0;
+        buf[sizeof(buf) - 1] = '\0';
+        if (safe_atoi(buf, &entropy_count) < 0)
+                return 0;


as before, debug log, and pass error code

poettering · 2019-01-17T23:57:00Z

src/random-seed/random-seed.c

+                return 0;
+        /* Make sure we never end up crediting the same bits twice. */
+        if (fremovexattr(fd, XATTR_NAME) < 0)
+                return 0;


similar here

poettering · 2019-01-17T23:57:38Z

src/random-seed/random-seed.c

+        if (fremovexattr(fd, XATTR_NAME) < 0)
+                return 0;
+
+        return entropy_count;


i think some validity checks would be appropriate here, for example, filter out negative entropy and so on

maybe add some paranoia here: fsync the file and the dir it is in after dropping the xattr, to make sure the entropy is never credited again ever. i.e. call fsync() on the fd first, followed by fsync_directory_of_file().

poettering · 2019-01-17T23:58:52Z

src/random-seed/random-seed.c

+                        e1 = 0;
                k = loop_read(random_fd, info->buf, buf_size, false);
+                if (ioctl(random_fd, RNDGETENTCNT, &e2) < 0)
+                        e2 = 0;


my understanding today is that the random pool never depletes once it is seeded, hence reading this twice is probably not necessary

also, I figure some safety check should be added: never credit more than k*8 bits, in the unlikely event the seed we store is smaller than the pool size in the kernel

poettering · 2019-01-17T23:59:11Z

src/random-seed/random-seed.c

                r = loop_write(seed_fd, info->buf, (size_t) k, false);
                if (r < 0)
                        return log_error_errno(r, "Failed to write new random seed file: %m");
+                write_entropy_xattr(seed_fd, MIN(e1, e2));


cast to (void) here, to tell analyzers we knowingly ignore errors

what i don't like here though: you have an xattr here you called "entropy_count" and a cmdline arg/env var called "entropy_credit". To me it's not clear at all what the difference is or the unit. The latter takes a permille and the former stores an absolute number of bits. I think that's quite confusing, and they should carry different names, and explicit names. Maybe call the xattr "entropy_credit_bits". And internally call the cmdline arg arg_entropy_share_permille (or maybe "ratio" instead of "share"? or "fraction"? or "quota"), but I figure for command line args it's not customary to contain units in the name, hence maybe drop it there, i.e. simply use --entropy-share=, but take permille.

poettering · 2019-01-18T00:18:12Z

btw, might make sense to add a kernel cmdline var for this too. might be helpful for people who find themselves with a frozen boot because entropy is missing, but who have no control on env vars or cmdline args...

hooking that up is easy, just use proc_cmdline_get_key(), it's a ready-made function for this purpose.

and env var and cmdline should override the kernel cmdline

poettering · 2019-03-06T09:47:59Z

BTW, is there still interest in this? Given how far we come, would love to see this finished for v242?

Villemoes · 2019-03-06T10:54:00Z

Sorry, $dayjob has been and still is filled with concrete customer tasks for a few months. In the meantime, I think we've concluded that fixing this systemd service won't actually be enough, since it runs too late (e.g., after systemd itself has started using /dev/[u]random for generating a machine-id etc.). Instead, since we anyway have our own init that does a few things before execing into systemd, we'll probably end up hooking something like this into that, and ensure that the board bootstrap procedure provides each board with a seed-file.

IOW, I'm unlikely to spend more time on this in the near future. Of course, anyone else is welcome to pick it up. If nobody volunteers in, say, the next week, it's probably best to close the PR for now (though I'll leave that up to Lennart et al).

jktjkt · 2019-03-07T10:43:16Z

I also hit this issue (for me, it was systemd-resolved getting stuck for 30 seconds). I tried this patch, but it required additional changes, such as manually specifying the trusted entropy ratio.

Similar to the patch author, we also ended up moving this entropy pool seeding into pre-init. Here's a simple C source which reads from stdin and seeds the kernel's RNG.

poettering · 2019-03-14T15:06:44Z

OK, let's close this then. If anyone wants to pick this up, please do, and post as a new PR

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 early boot 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 systematically delayed until the entropy pool is initialized. I'd argue that's a good thing, and is what we want, if this turns out to be problematic IRL we might want to drop the Before=sysinit.target line from the systemd-random-seed.service, which would drop the requirement that the entropy pool is initialized before regular services are started, and would mean that regular services using /dev/urandom would have to be individually ordered against systemd-random-seed.service. Fixes: systemd#4271 Replaces systemd#10621 systemd#4513

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 early boot 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 systematically delayed until the entropy pool is initialized. I'd argue that's a good thing, and is what we want, if this turns out to be problematic IRL we might want to drop the Before=sysinit.target line from the systemd-random-seed.service, which would drop the requirement that the entropy pool is initialized before regular services are started, and would mean that regular services using /dev/urandom would have to be individually ordered against systemd-random-seed.service. Fixes: systemd#4271 Replaces: systemd#10621 systemd#4513

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

yuwata added the random-seed label Nov 3, 2018

poettering reviewed Nov 3, 2018

View reviewed changes

poettering mentioned this pull request Nov 3, 2018

Use ioctl RNDADDENTROPY for writing back random seed #4513

Closed

poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Nov 5, 2018

Rasmus Villemoes added 2 commits January 9, 2019 08:36

random-seed: getopt'ify it

3ca12fe

This adds parse_argv() and help() boilerplate in preparation for teaching the command the --entropy-credit option.

Rasmus Villemoes added 2 commits January 9, 2019 08:39

random-seed: implement --entropy-credit option

42d1383

random-seed: save estimate of quality of random data in an xattr

bc49142

Use the ioctl RNDGETENTCNT to get an estimate of how good the random data we read from the kernel is, and store it in an xattr in the random-seed file.

Villemoes force-pushed the random-credit branch from 426de6f to bc49142 Compare January 9, 2019 07:41

yuwata removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Jan 12, 2019

poettering requested changes Jan 18, 2019

View reviewed changes

poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Mar 4, 2019

poettering closed this Mar 14, 2019

jktjkt mentioned this pull request Jun 11, 2020

systemd.volatile without initrd #16107

Closed

Uh oh!

Conversation

Villemoes commented Nov 2, 2018

Uh oh!

poettering commented Nov 3, 2018

Uh oh!

poettering commented Nov 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poettering commented Nov 3, 2018

Uh oh!

Villemoes commented Nov 3, 2018

Uh oh!

Villemoes commented Nov 3, 2018

Uh oh!

sebastianas commented Nov 4, 2018

Uh oh!

poettering commented Nov 5, 2018

Uh oh!

poettering commented Nov 5, 2018

Uh oh!

poettering commented Nov 5, 2018

Uh oh!

poettering commented Nov 5, 2018

Uh oh!

poettering commented Nov 5, 2018

Uh oh!

Villemoes commented Nov 5, 2018

Uh oh!

keszybz commented Nov 8, 2018

Uh oh!

poettering commented Nov 8, 2018

Uh oh!

poettering commented Nov 8, 2018

Uh oh!

keszybz commented Nov 8, 2018

Uh oh!

Villemoes commented Nov 8, 2018

Uh oh!

poettering commented Nov 8, 2018

Uh oh!

Villemoes commented Nov 8, 2018

Uh oh!

poettering commented Nov 8, 2018

Uh oh!

poettering commented Nov 8, 2018

Uh oh!

poettering commented Nov 8, 2018

Uh oh!

Villemoes commented Nov 8, 2018

Uh oh!

Villemoes commented Nov 8, 2018

Uh oh!

Villemoes commented Jan 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!