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

Conversation

@c0bw3b
Copy link
Contributor

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

c0bw3b commented Nov 7, 2019

@JohnAZoidberg

This comment has been minimized.

Copy link
Member

JohnAZoidberg commented Nov 8, 2019

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

This comment has been minimized.

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 c0bw3b force-pushed the c0bw3b:pkg/rngtools branch from b4c9d8d to df8894d Nov 8, 2019
@c0bw3b

This comment has been minimized.

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.

@c0bw3b c0bw3b force-pushed the c0bw3b:pkg/rngtools branch from df8894d to 67ab304 Nov 8, 2019
@kmcopper

This comment has been minimized.

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

This comment has been minimized.

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
@c0bw3b c0bw3b force-pushed the c0bw3b:pkg/rngtools branch from 67ab304 to c5996d4 Nov 12, 2019
Copy link
Contributor

kmcopper left a comment

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

@c0bw3b

This comment has been minimized.

Copy link
Contributor Author

c0bw3b commented Nov 12, 2019

@GrahamcOfBorg build rng-tools

@c0bw3b c0bw3b merged commit 810abeb into NixOS:master Nov 12, 2019
16 checks passed
16 checks passed
rng-tools on x86_64-darwin No attempt
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
rng-tools on aarch64-linux Success
Details
rng-tools on x86_64-linux Success
Details
@c0bw3b c0bw3b deleted the c0bw3b:pkg/rngtools branch Nov 12, 2019
kmcopper added a commit to kmcopper/nixpkgs that referenced this pull request Nov 13, 2019
+ run tests
+ enable jitterentropy by default
+ add c0bw3b to maintainers
@Mic92 Mic92 mentioned this pull request Nov 13, 2019
4 of 10 tasks complete
kmcopper added a commit to kmcopper/nixpkgs that referenced this pull request Nov 13, 2019
+ run tests
+ enable jitterentropy by default
+ add c0bw3b to maintainers

(cherry picked from commit 810abeb)
kmcopper added 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
3 participants
You can’t perform that action at this time.