getrandom() syscall failure warning should be a notice and worded better

The logging improvement for error handling of the getrandom() syscall in legacy/trac#24500 (moved) could be further improved:

• The log level should possibly be NOTICE rather than WARN.
• The log message should mention that tor will fall back to alternative sources of randomness.
• Maybe we want to mention header/kernel version mismatches as a specific common reason for this issue.

Yes, some of the fallbacks are somewhat dangerous, like /dev/urandom might not be seeded yet. etc,.

changed milestone to %Tor: 0.3.3.x-final in legacy/trac

added component::core tor/tor in Legacy / Trac milestone::Tor: 0.3.3.x-final in Legacy / Trac priority::medium in Legacy / Trac resolution::implemented in Legacy / Trac s8-errors in Legacy / Trac severity::normal in Legacy / Trac sponsor::8-can in Legacy / Trac status::closed in Legacy / Trac type::defect in Legacy / Trac labels

Wording and log-level patches in https://oniongit.eu/ahf/tor/merge_requests/5

These patches do not have the mechanism needed to check if /dev/urandom is safe.

Trac:
Status: new to needs_review

Do we think it's dangerous to downgrade the warning without first ensuring that we can safely use /dev/urandom even early in boot?

My opinion: I think that checking whether urandom is correctly seeded would be a good additional feature to have, but I don't think it needs to block this message downgrade. Anybody who sees the current warning is likelier to get confused than to reason correctly about urandom risks based on it -- and I think nearly everybody sees notice messages as well as warning. IMO.

Replying to nickm:

and I think nearly everybody sees notice messages as well as warning. IMO. The patch drops it all the way to INFO though, which isn't readily visible to at least Tor Browser users.

Trac:
Keywords: N/A deleted, s8-errors added
Sponsor: N/A to Sponsor8-can

So would you be okay with this patch if it were NOTICE instead?

Replying to nickm:

So would you be okay with this patch if it were NOTICE instead? Yes. Or hearing some rationale for why INFO is more appropriate than NOTICE.

My two cents.

NOTICE level should be information the operator needs or is useful to know. And I also think it is useful to provide basic status information at bootup so if we ever get a report about a misbehaving relay, we can ask for those "status" line.

With this getrandom() thing, if Tor stops because it can't use its crypto, we ought to put a warning on why even though the users would be "omgwtfbbq is that?". At least, at that point, there are possible action items that the operator can do including seeking support about that "in your face" log line.

If tor recovers from it, I would argue that it should be at NOTICE so the operator can see that it is not critical, that tor did recover but actions can still be taken to fix it.

For instance this, I think it should be at NOTICE for the above reasons. This usually happens when someones run a tor not built for their system like Stretch Debian tor package on Ubuntu 10.04. Having the notice log would allow the operator to try to fix it or simply ignore it. At INFO, I believe most of the users will just never notice it.

        log_info(LD_CRYPTO, "Can't get entropy from getrandom()."
                 " You are running a version of Tor built to support"
                 " getrandom(), but the kernel doesn't implement this"
                 " function--probably because it is too old?"
                 " Trying fallback method instead.");

In a nutshell, +1 on removing the warnings except if Tor does stop. And then +1 on NOTICE for useful logging for which the operator can notice.

I agree with dgoulet. I also agree with nickm on deferring checking of urandom seeding to another ticket.

Trac:
Status: needs_review to needs_revision

I've added a fixup commit to the PR. Remember to autosquash when pulling :-)

Trac:
Status: needs_revision to needs_review

Looks good to me, other than the Rust test failures that are presumably due to legacy/trac#25127 (moved). Hopefully those will resolve upon merge.

Trac:
Status: needs_review to merge_ready

Squashed and merged; added a changes file; fixed compilation.

Trac:
Status: merge_ready to closed
Resolution: N/A to implemented

closed

moved from legacy/trac#25120 (moved)

added Bug label and removed 1 deleted label

removed 1 deleted label