crypto uses rand.Reader directly.

[jbenet]

We use rand.Reader directly in the crypto package, which makes it depend on the global seed. We should be using a specific, configurable rand.Source, like crypto/tls:
http://golang.org/src/pkg/crypto/tls/key_agreement.go#L214

// crypto/tls
ka.privateKey, x, y, err = elliptic.GenerateKey(ka.curve, config.rand())

[whyrusleeping]

👍

[chriscool]

In http://golang.org/src/pkg/crypto/tls/common.go there is:

   208  type Config struct {
   209      // Rand provides the source of entropy for nonces and RSA blinding.
   210      // If Rand is nil, TLS uses the cryptographic random reader in package
   211      // crypto/rand.
   212      // The Reader must be safe for use by multiple goroutines.
   213      Rand io.Reader
...

and then:

   324  func (c *Config) rand() io.Reader {
   325      r := c.Rand
   326      if r == nil {
   327          return rand.Reader
   328      }
   329      return r
   330  }

So by default it will use rand.Reader.

We could do the same but I am not sure we should add the Rand io.Reader to our Config struct in config/config.go. Our config stuct looks like:

// Config is used to load IPFS config files.
type Config struct {
    Identity  Identity         // local node's peer identity
    Datastore Datastore        // local node's storage
    Addresses Addresses        // local node's addresses
    Mounts    Mounts           // local node's mount points
    Version   Version          // local node's version management
    Bootstrap []*BootstrapPeer // local nodes's bootstrap peers
    Tour      Tour             // local node's tour position
}

[jbenet]

We could do the same

Yeah, sounds good to me. Should be configurable, but default to rand.Reader

our Config struct in config/config.go

That's the config var matching the config file, so yeah, let's not put it there. If we want a single random Reader per node, we could add it directly to core/core.IpfsNode.

In any case, the random Reader should at some point get to spipe.SecurePipe (all the spipe code is a bit of a mess right now). It's kind of clunky to pass random Reader from IpfsNode -> IpfsNetwork -> Swarm -> NewSecureConn -> SecurePipe, but that would be the order it would have to go. The alternative is to use a static variable somewhere -- which is gross, but may be simpler.

[rht]

I don't think the purpose of having a Rand field in crypto/tls is so that people can use different source for different instances in their code.
I could be wrong, but the point is it allows one to use a different random source (that is not available through crypto/rand) for the entire program.

The default rand.Reader already suffices as a CSPRNG[1].
Any userspace approach (math/rand) with seeds from crypto/rand is still less secure than using crypto/rand directly with its single global seed[2].
The only reason why this config.rand() needs to be implemented is if accessing the default global source is a major performance bottleneck[2].

[1] http://www.2uo.de/myths-about-urandom/
[2] https://news.ycombinator.com/item?id=7610273.

[jbenet]

thanks @rht -- that makes sense. probably still good to do. another reason is to mock it out to run tests faster

[rht]

/dev/urandom (rand.Reader's default for linux, os x, freebsd) can be used right away, at the expense of lower entropy and still be more random than math/rand.

[whyrusleeping]

@rht but for tests, we dont care about randomness, we just want it to be 'random enough' and fast so our tests can not take forever

[rht]

rand.Reader is non-blocking unless the test reaches its concurrent limit.

From https://travis-ci.org/ipfs/go-ipfs/jobs/65047346, make $TEST_SUITE took 298.93s, 283.137s of which is from github.com/ipfs/go-ipfs/exchange/bitswap.
Can't further tell if rand.Reader is a major bottleneck here, but I will try swapping with math/rand and check the perf increase later.

For the sharness test, I replaced https://github.com/ipfs/go-ipfs/blob/master/test/sharness/lib/test-lib.sh#L250
with sleep 0.1.
I discovered a ~1 min speedup from
make 24.52s user 6.43s system 15% cpu 3:25.66 total to make 24.71s user 6.20s system 22% cpu 2:19.29 total.

[jbenet]

hardware differs. for my macbook air, math/rand is much faster for me than crypto/rand.Reader

[jbenet]

For the sharness test, I replaced https://github.com/ipfs/go-ipfs/blob/master/test/sharness/lib/test-lib.sh#L250 with sleep 0.1. I discovered a ~1 min speedup from

ohh wow, yeah. submit a PR?

[rht]

The changes are within standard deviation #1326, so it's not much apparently. The PR only makes the checks more granular.

[whyrusleeping]

closing, if further discussion is warranted open a new issue.