Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make fallback to /dev/random optional when getrandom(2) is available #127

Closed
wants to merge 3 commits into from
Closed

Make fallback to /dev/random optional when getrandom(2) is available #127

wants to merge 3 commits into from

Conversation

nbraud
Copy link

@nbraud nbraud commented Jan 5, 2020

The fallback to a character file is now gated on the file-fallback
feature (enabled by default); affects Linux/Android, macOS, and Solaris/Illumos.

Disabling this feature can be advantageous for reduced code size.

Moreover, some users may prefer to guarantee using getrandom(2) on Linux
rather than have to reason about Linux's blocking behaviour for random(4) and
urandom(4).

The fallback to a character file is now gated on the `file-fallback`
feature (enabled by default); affects Linux/Android, macOS, and Solaris/Illumos.

Disabling this feature when targetting environments where getrandom(2) is
guaranteed to be present can be advantageous for reduced code size and ease of
audit/reasoning.

(Linux's random(4)/urandom(4) has silly blocking behaviour which makes it
difficult to implement sensible behaviour, and removing that code can be easier
than auditing it.)
@dhardy dhardy requested a review from josephlr January 6, 2020 10:07
@josephlr
Copy link
Member

josephlr commented Jan 7, 2020

So I'm skeptical if we actually need this additional complexity in getrandom. Here's why:

  • Linux has the most complex file fallback code, but even then, I'm not sure the code size savings are worthwhile.
    • To make a small binary, I took this very small example program, and I did the following (see https://github.com/johnthagen/min-sized-rust for motivation):
      • Compile with --release
      • Use today's nightly (makes a smaller binary than stable)
      • Use a release profile of:
        [profile.release]
        lto = true
        codegen-units = 1
        panic = 'abort'
      • I didn't enable opt-level = 'z' (see below as to why)
      • Use the build-std Cargo feature, running cargo like: cargo build -Z build-std=panic_abort,std --release --target=x86_64-unknown-linux-gnu.
      • strip the binary
    • This produced binaries with the following sizes:
      1. Current master: 182496 bytes
      2. Removing all file fallback on Linux: 174288 bytes
    • This savings is 8208 bytes, but that code won't be "hot" on a system supporting getrandom(2), so you won't actually gain that much. Also, enabling opt-level = 'z' (to make the smallest possible binary) causes the final size difference to only be 12 bytes (129248 bytes vs 129232 bytes).
    • Regardless, is seems like the code size savings are small in the best case, and are dwarfed by other minor configuration changes.
  • Other than code size, what's the motivation for this change? If you're auditing the code of your dependencies, you still have to read the fallback code and trust it. This is because Cargo features are additive, so any dependency enabling the default set of features will enable the fallback code.
  • You mention that on Linux, users would "have to reason about Linux's blocking behavior". I'm not quite sure what you mean here. The implementation reads from /dev/urandom once /dev/random is ready via internal file descriptors. This is all hidden to users of the library, they just call getrandom::getrandom() and get the expected behavior.
  • We should probably focus on just removing the fallback behavior once Rust stops supporting the specific versions of the relevant OS (see below for why this might take awhile).
  • We don't get to remove the use_file code, so we make the overall codebase more complex instead of simpler.
  • What should our advice be to users about when to enable this feature?

Some comments on the CL itself:

  • This should be part of 0.2 (see Tracking issue for 0.2.0 #98) as it isn't a bug fix. Please open the PR against the 0.2 branch if we decide to continue.
  • Make sure the CI is passing (it's failing on some targets right now).
  • For Linux, we don't need the HAS_GETRANDOM stuff if the fallback isn't enabled. We can just call the syscall and get ENOSYS back. That's fine.
  • Cargo can't support target specific features (see build-dependencies and dependencies should not have features unified rust-lang/cargo#4866). This change has no way to say "have the fallback turned on for Linux but not for macOS". So this would have to be 3 separate features, for Linux, Solaris, and macOS.
  • There are other fallback mechanisms for getting random data in in this codebase, which not included in this PR. Why are the file-based fallbacks worse than the other fallback mechanisms?
  • More generally, this change groups together three different fallbacks that are actually quite different. Specifically:
    • Linux/Android: is the getrandom(2) syscall present?
      • The newest kernel not supporting the syscall is 3.16.
      • This means the widely used Debian 8 and RHEL 7 systems don't have the syscall
        • Debian 8 has Long Term Support until 30 June 2020.
        • RedHat hasn't even annonced an "End of Extended Life-cycle Support" date for RHEL 7, but it will probobly be around 2028, see their support table.
        • But most other Linux distros have a newer kernel
      • Also Android 5 and earlier don't have the syscall
    • Solaris/Illumos: is the getrandom() function present?
    • macOS: is the getentropy() function present?
      • Added in OSX 10.12, IOS 10.0
      • Any older versions (without the function) are not supported by Apple anymore.
      • However, Rust officially supports 10.7 and up for macOS targets, so we have to keep the fallback in.

@nbraud What are your thoughts? Have I missed something?

if #[cfg(feature = "file-fallback")] {
use_file::getrandom_inner(dest)
} else {
Err(error::UNSUPPORTED)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a macOS specific error constant (with a good description)

if #[cfg(feature = "file-fallback")] {
use_file::getrandom_inner(dest)
} else {
Err(error::UNSUPPORTED)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a Solaris specific error constant (with a good description)

@newpavlov
Copy link
Member

newpavlov commented Jan 7, 2020

While I am somewhat sympathetic to the idea of removing /dev/urandom fallback (see #5), in the end I think adding the feature does not really worth the associated troubles. The code is already complex enough, making it even more complex with feature gates should have a really good motivation, which I don't think is present here.

I would be happy to remove the fallback completely, but as @josephlr already wrote, we probably will have to wait until ~2030 to do that.

@dhardy
Copy link
Member

dhardy commented Jan 7, 2020

Moreover, some users may prefer to guarantee using getrandom(2) on Linux
rather than have to reason about Linux's blocking behaviour for random(4) and
urandom(4).

Either way, we always use getrandom if it is available, so this is a strange argument.

Yes, I think we can reject this PR; @josephlr gives a very good rationale for why.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants