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

rng-tools: 6.7 -> 6.8 #73007

Merged
merged 1 commit into from Nov 12, 2019
Merged

rng-tools: 6.7 -> 6.8 #73007

merged 1 commit into from Nov 12, 2019

Conversation

c0bw3b
Copy link
Contributor

@c0bw3b c0bw3b commented Nov 7, 2019

Motivation for this change

Update
+ enable tests
+ enable jitterentropy by default
+ add c0bw3b to maintainers

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @JohnAZoidberg

@c0bw3b
Copy link
Contributor Author

c0bw3b commented Nov 7, 2019

Changelog:

Bug fix update containing the following fixes:

  • Fixed texrels on on rdrand_asm.S for pic compilation
  • allow use of libargp if libc lacks argp parsing
  • explicitly link against -lcrypto, fixing build in pkcs11 entropy source
  • replace pthread_yield with posix compliant sched_yield
  • bias rngd to use faster sources of entropy when available, falling back to slower sources when needed
  • Fix a shutdown delay resulting from a thread exit race
  • Fix a few minor compilation warnings
  • Fix make distcheck make target
  • Minor typo fixes/cleanups

@c0bw3b
Copy link
Contributor Author

c0bw3b commented Nov 7, 2019

@GrahamcOfBorg eval

@JohnAZoidberg
Copy link
Member

Thanks! I looked at the individual patches and noticed that argp-standalone can now be used (nhorman/rng-tools@ddecdb5). It's needed when using musl because that doesn't include argp.

argp-standalone can just be added to buildInputs. The configure script only links it, if the libc doesn't provide it. I checked that the closure doesn't include it, when building with glibc.

I don't know, however, how to build just this package with musl to verify that that works.
Do you?

nix-build -A pkgsCross.musl64.rng-tools and nix-build -A pkgsMusl.rng-tools seem to build all dependencies with musl, too.

@c0bw3b
Copy link
Contributor Author

c0bw3b commented Nov 8, 2019

I don't know, however, how to build just this package with musl to verify that that works.
Do you?

Not exactly. I would have tried build pkgsMusl.rng-tools like you did.
But @dtzWill could help us here. :)

@c0bw3b
Copy link
Contributor Author

c0bw3b commented Nov 8, 2019

I added argp-standalone. It was also mentioned in the changelog in fact.
EDIT: oh only available on x64 -> will fix

I also removed the file copy in postPatch because it doesn't seem to serve any purpose. (?) The file is not installed.

@kmcopper
Copy link
Contributor

kmcopper commented Nov 11, 2019

Would it be possible to change enableFeature withJitterEntropy to withFeature as well as enable it by default? This is the default in two other distributions, archlinux and fedora, and shouldn't harm entropy at all, only help it. Otherwise looks good. I tested this on my own branch (c0bw3b/nixpkgs@pkg/rngtools...kmcopper:rng-tools) and it works great. Additionally this will allow rngd to improve entropy on sandybridge systems.

Closure size (with my branch):
rng-tools-6.7 45981904 -> rng-tools-6.8 46038872
Tested, compiled, and ran on NixOS in a VM

Entropy Improvement cat /dev/urandom|rngtest:
with: input channel speed: (min=1.693; avg=16.490; max=18.626)Gibits/s
w/o: input channel speed: (min=829.282; avg=16685.221; max=19073.486)Mibits/s

@c0bw3b
Copy link
Contributor Author

c0bw3b commented Nov 12, 2019

Since this v6.8 release rngd will prioritize faster hardware sources (hwrng, TPM, RDRAND) over software sources like jitterentropy, which may still be used as additional source if available. Plus the jitterentropy lib has shown compliance with NIST SP800-90B as an example benchmark.
For me the concern about its quality is gone by now and I'm +1 on enabling it by default along other sources. The additional closure size is minimal (see further down).
I'll update this PR soon.


Would it be possible to change enableFeature withJitterEntropy to withFeature

Upstream configure flag to add jitter source is --enable-jitterentropy and not --with-jitterentropy like other features (--with-pkcs11 for example).
So it has to be enableFeature here.

and shouldn't harm entropy at all, only help it.

Entropy is more complicated than that. You can actually harm it with too many sources gathering too many data points from not-so-unpredictable events. See Dan J. Bernstein on this topic:
http://blog.cr.yp.to/20140205-entropy.html
Plus another good read on the matter of randomness and entropy in general : https://www.2uo.de/myths-about-urandom/

Additionally this will allow rngd to improve entropy on sandybridge systems.

Sandy Bridge has RDRAND I believe? Plus it would help rngd only if you don't have any other sources available. On my test system with RDRAND and TPM sources available, the average "input channel speed" remains the same with or without jitter source.


Regarding closure size (nix path-info -sSh) :

# standard build without jitterentropy:
/nix/store/c00nxwcn3x3x1n87hidbgqgvln2xbwq8-rng-tools-6.8	  80.0K	  43.9M

# rng-tools build WITH jitterentropy:
/nix/store/xb347wd2bvqn1fh7p6db2b75lfdfm5i3-rng-tools-6.8	  97.3K	  43.9M

+ run tests
+ enable jitterentropy by default
+ add c0bw3b to maintainers
Copy link
Contributor

@kmcopper kmcopper left a comment

Choose a reason for hiding this comment

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

My testing was done on a 2600k. Looks good to me.
RDRAND was added in Ivybridge (Sandybridge tock).

@c0bw3b
Copy link
Contributor Author

c0bw3b commented Nov 12, 2019

@GrahamcOfBorg build rng-tools

@c0bw3b c0bw3b merged commit 810abeb into NixOS:master Nov 12, 2019
@c0bw3b c0bw3b deleted the pkg/rngtools branch November 12, 2019 23:05
@Mic92 Mic92 mentioned this pull request Nov 13, 2019
10 tasks
kmcopper pushed a commit to kmcopper/nixpkgs that referenced this pull request Nov 18, 2019
+ run tests
+ enable jitterentropy by default
+ add c0bw3b to maintainers

(cherry picked from commit 810abeb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants