From 5f85225428d0183dee7af35678555ef854a96023 Mon Sep 17 00:00:00 2001 From: Victor Farazdagi Date: Thu, 25 Jun 2020 17:48:03 +0300 Subject: [PATCH 01/17] adds cryptorand analyzer --- BUILD.bazel | 1 + nogo_config.json | 20 +++-- tools/analyzers/cryptorand/BUILD.bazel | 28 +++++++ tools/analyzers/cryptorand/analyzer.go | 82 +++++++++++++++++++ tools/analyzers/cryptorand/analyzer_test.go | 11 +++ .../analyzers/cryptorand/testdata/BUILD.bazel | 11 +++ .../cryptorand/testdata/custom_import.go | 23 ++++++ .../analyzers/cryptorand/testdata/rand_new.go | 21 +++++ 8 files changed, 192 insertions(+), 5 deletions(-) create mode 100644 tools/analyzers/cryptorand/BUILD.bazel create mode 100644 tools/analyzers/cryptorand/analyzer.go create mode 100644 tools/analyzers/cryptorand/analyzer_test.go create mode 100644 tools/analyzers/cryptorand/testdata/BUILD.bazel create mode 100644 tools/analyzers/cryptorand/testdata/custom_import.go create mode 100644 tools/analyzers/cryptorand/testdata/rand_new.go diff --git a/BUILD.bazel b/BUILD.bazel index 1f9712ebf9e9..da095f7bac2b 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -105,6 +105,7 @@ nogo( "@org_golang_x_tools//go/analysis/passes/asmdecl:go_tool_library", "//tools/analyzers/maligned:go_tool_library", "//tools/analyzers/roughtime:go_tool_library", + "//tools/analyzers/cryptorand:go_tool_library", "//tools/analyzers/errcheck:go_tool_library", "//tools/analyzers/featureconfig:go_tool_library", ] + select({ diff --git a/nogo_config.json b/nogo_config.json index e421293f1a66..061778761262 100644 --- a/nogo_config.json +++ b/nogo_config.json @@ -34,7 +34,7 @@ "exclude_files": { "external/.*": "Third party code" } - }, + }, "copylocks": { "exclude_files": { "external/.*": "Third party code" @@ -44,7 +44,7 @@ "exclude_files": { "external/.*": "Third party code" } - }, + }, "cgocall": { "exclude_files": { "external/.*": "Third party code" @@ -70,8 +70,8 @@ }, "roughtime": { "only_files": { - "beacon-chain/.*": "", - "shared/.*": "", + "beacon-chain/.*": "", + "shared/.*": "", "validator/.*": "" }, "exclude_files": { @@ -96,7 +96,17 @@ }, "featureconfig": { "only_files": { - ".*_test\\.go": "Only tests" + ".*_test\\.go": "Only tests" + } + }, + "cryptorand": { + "only_files": { + "beacon-chain/.*": "", + "shared/.*": "", + "validator/.*": "" + }, + "exclude_files": { + ".*/.*_test\\.go": "Tests are OK to use weak crypto" } } } diff --git a/tools/analyzers/cryptorand/BUILD.bazel b/tools/analyzers/cryptorand/BUILD.bazel new file mode 100644 index 000000000000..d10a1dd6cfd4 --- /dev/null +++ b/tools/analyzers/cryptorand/BUILD.bazel @@ -0,0 +1,28 @@ +load("@prysm//tools/go:def.bzl", "go_library") +load("@io_bazel_rules_go//go:def.bzl", "go_tool_library") + +go_library( + name = "go_default_library", + srcs = ["analyzer.go"], + importpath = "github.com/prysmaticlabs/prysm/tools/analyzers/cryptorand", + visibility = ["//visibility:public"], + deps = [ + "@org_golang_x_tools//go/analysis:go_default_library", + "@org_golang_x_tools//go/analysis/passes/inspect:go_default_library", + "@org_golang_x_tools//go/ast/inspector:go_default_library", + ], +) + +go_tool_library( + name = "go_tool_library", + srcs = ["analyzer.go"], + importpath = "github.com/prysmaticlabs/prysm/tools/analyzers/cryptorand", + visibility = ["//visibility:public"], + deps = [ + "@org_golang_x_tools//go/analysis:go_tool_library", + "@org_golang_x_tools//go/analysis/passes/inspect:go_tool_library", + "@org_golang_x_tools//go/ast/inspector:go_tool_library", + ], +) + +# gazelle:exclude analyzer_test.go diff --git a/tools/analyzers/cryptorand/analyzer.go b/tools/analyzers/cryptorand/analyzer.go new file mode 100644 index 000000000000..e176f3c755b4 --- /dev/null +++ b/tools/analyzers/cryptorand/analyzer.go @@ -0,0 +1,82 @@ +package cryptorand + +import ( + "errors" + "fmt" + "go/ast" + "strings" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" +) + +// Doc explaining the tool. +const Doc = "Tool to enforce the use of stronger crypto: crypto/rand instead of math/rand" + +var errWeakCrypto = errors.New("weak cryptography, use crypto/rand, or add an exclusion to //:nogo.json") + +// Analyzer runs static analysis. +var Analyzer = &analysis.Analyzer{ + Name: "cryptorand", + Doc: Doc, + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, +} + +func run(pass *analysis.Pass) (interface{}, error) { + inspect, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + if !ok { + return nil, errors.New("analyzer is not type *inspector.Inspector") + } + + nodeFilter := []ast.Node{ + (*ast.File)(nil), + (*ast.ImportSpec)(nil), + (*ast.CallExpr)(nil), + } + + aliases := make(map[string]string) + disallowedFns := []string{"NewSource", "New", "Seed", "Int63", "Uint32", "Uint64", "Int31", "Int", + "Int63n", "Int31n", "Intn", "Float64", "Float32", "Perm", "Shuffle", "Read"} + + inspect.Preorder(nodeFilter, func(node ast.Node) { + switch stmt := node.(type) { + case *ast.File: + // Reset aliases (per file). + aliases = make(map[string]string) + case *ast.ImportSpec: + // Collect aliases to rand packages. + pkg := stmt.Path.Value + if strings.Contains(pkg, "/rand") && !strings.Contains(pkg, "crypto/rand") { + if stmt.Name != nil { + aliases[stmt.Name.Name] = stmt.Path.Value + } else { + aliases["rand"] = stmt.Path.Value + } + } + case *ast.CallExpr: + // Check if any of disallowed functions have been used. + for pkg, path := range aliases { + for _, fn := range disallowedFns { + if isPkgDot(stmt.Fun, pkg, fn) { + pass.Reportf(node.Pos(), fmt.Sprintf( + "%s: %s.%s() (from %s)", errWeakCrypto.Error(), pkg, fn, path)) + } + } + } + } + }) + + return nil, nil +} + +func isPkgDot(expr ast.Expr, pkg, name string) bool { + sel, ok := expr.(*ast.SelectorExpr) + return ok && isIdent(sel.X, pkg) && isIdent(sel.Sel, name) +} + +func isIdent(expr ast.Expr, ident string) bool { + id, ok := expr.(*ast.Ident) + return ok && id.Name == ident +} diff --git a/tools/analyzers/cryptorand/analyzer_test.go b/tools/analyzers/cryptorand/analyzer_test.go new file mode 100644 index 000000000000..545adf8f66f8 --- /dev/null +++ b/tools/analyzers/cryptorand/analyzer_test.go @@ -0,0 +1,11 @@ +package cryptorand + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +func TestAnalyzer(t *testing.T) { + analysistest.Run(t, analysistest.TestData(), Analyzer) +} diff --git a/tools/analyzers/cryptorand/testdata/BUILD.bazel b/tools/analyzers/cryptorand/testdata/BUILD.bazel new file mode 100644 index 000000000000..fc00232e9535 --- /dev/null +++ b/tools/analyzers/cryptorand/testdata/BUILD.bazel @@ -0,0 +1,11 @@ +load("@prysm//tools/go:def.bzl", "go_library") + +go_library( + name = "go_default_library", + srcs = [ + "rand_new.go", + "weak_crypto_custom_import.go", + ], + importpath = "github.com/prysmaticlabs/prysm/tools/analyzers/cryptorand/testdata", + visibility = ["//visibility:public"], +) diff --git a/tools/analyzers/cryptorand/testdata/custom_import.go b/tools/analyzers/cryptorand/testdata/custom_import.go new file mode 100644 index 000000000000..bb788afdd7b7 --- /dev/null +++ b/tools/analyzers/cryptorand/testdata/custom_import.go @@ -0,0 +1,23 @@ +package testdata + +import ( + mathRand "math/rand" + xxx "math/rand" + "time" +) + +func UseRandNewCustomImport() { + randGenerator := mathRand.New(mathRand.NewSource(time.Now().UnixNano())) + start := uint64(randGenerator.Intn(32)) + _ = start + + randGenerator = mathRand.New(mathRand.NewSource(time.Now().UnixNano())) +} + +func UseWithoutSeeCustomImportd() { + assignedIndex := mathRand.Intn(128) + _ = assignedIndex + xxx.Shuffle(10, func(i, j int) { + + }) +} diff --git a/tools/analyzers/cryptorand/testdata/rand_new.go b/tools/analyzers/cryptorand/testdata/rand_new.go new file mode 100644 index 000000000000..17b333426497 --- /dev/null +++ b/tools/analyzers/cryptorand/testdata/rand_new.go @@ -0,0 +1,21 @@ +package testdata + +import ( + "math/rand" + mathRand "math/rand" + "time" +) + +func UseRandNew() { + randGenerator := mathRand.New(rand.NewSource(time.Now().UnixNano())) + start := uint64(randGenerator.Intn(32)) + _ = start + + + randGenerator = rand.New(rand.NewSource(time.Now().UnixNano())) +} + +func UseWithoutSeed() { + assignedIndex := rand.Intn(int(128)) + _ = assignedIndex +} From 81b16e620f94e93fa5fd5952700431b60cdb7493 Mon Sep 17 00:00:00 2001 From: Victor Farazdagi Date: Thu, 25 Jun 2020 17:52:35 +0300 Subject: [PATCH 02/17] better naming --- tools/analyzers/cryptorand/testdata/custom_import.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/analyzers/cryptorand/testdata/custom_import.go b/tools/analyzers/cryptorand/testdata/custom_import.go index bb788afdd7b7..5991244ef66a 100644 --- a/tools/analyzers/cryptorand/testdata/custom_import.go +++ b/tools/analyzers/cryptorand/testdata/custom_import.go @@ -1,8 +1,8 @@ package testdata import ( + foobar "math/rand" mathRand "math/rand" - xxx "math/rand" "time" ) @@ -17,7 +17,7 @@ func UseRandNewCustomImport() { func UseWithoutSeeCustomImportd() { assignedIndex := mathRand.Intn(128) _ = assignedIndex - xxx.Shuffle(10, func(i, j int) { + foobar.Shuffle(10, func(i, j int) { }) } From e54419354921375cda3d50907e1eadc8a347fc22 Mon Sep 17 00:00:00 2001 From: Victor Farazdagi Date: Thu, 25 Jun 2020 17:57:25 +0300 Subject: [PATCH 03/17] rely on suffix --- tools/analyzers/cryptorand/analyzer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/analyzers/cryptorand/analyzer.go b/tools/analyzers/cryptorand/analyzer.go index e176f3c755b4..0a74638aa14d 100644 --- a/tools/analyzers/cryptorand/analyzer.go +++ b/tools/analyzers/cryptorand/analyzer.go @@ -48,7 +48,7 @@ func run(pass *analysis.Pass) (interface{}, error) { case *ast.ImportSpec: // Collect aliases to rand packages. pkg := stmt.Path.Value - if strings.Contains(pkg, "/rand") && !strings.Contains(pkg, "crypto/rand") { + if strings.HasSuffix(pkg, "/rand\"") && !strings.Contains(pkg, "crypto/rand") { if stmt.Name != nil { aliases[stmt.Name.Name] = stmt.Path.Value } else { From 3ad95e34ae79196c12c4945d520770d438531a1b Mon Sep 17 00:00:00 2001 From: Victor Farazdagi Date: Thu, 25 Jun 2020 17:58:49 +0300 Subject: [PATCH 04/17] sync/pending_* use crypto/rand --- beacon-chain/sync/pending_attestations_queue.go | 9 +++++++-- beacon-chain/sync/pending_blocks_queue.go | 9 +++++++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/beacon-chain/sync/pending_attestations_queue.go b/beacon-chain/sync/pending_attestations_queue.go index 725cf0741929..d803b1c9c50c 100644 --- a/beacon-chain/sync/pending_attestations_queue.go +++ b/beacon-chain/sync/pending_attestations_queue.go @@ -2,7 +2,9 @@ package sync import ( "context" + "crypto/rand" "encoding/hex" + "math/big" "sync" pubsub "github.com/libp2p/go-libp2p-pubsub" @@ -18,7 +20,6 @@ import ( "github.com/prysmaticlabs/prysm/shared/traceutil" "github.com/sirupsen/logrus" "go.opencensus.io/trace" - "golang.org/x/exp/rand" ) // This defines how often a node cleans up and processes pending attestations in the queue. @@ -130,7 +131,11 @@ func (s *Service) processPendingAtts(ctx context.Context) error { log.Debug("No peer IDs available to request missing block from for pending attestation") return nil } - pid := pids[rand.Int()%len(pids)] + randInt, err := rand.Int(rand.Reader, big.NewInt(int64(len(pids)))) + if err != nil { + return err + } + pid := pids[randInt.Int64()%int64(len(pids))] targetSlot := helpers.SlotToEpoch(attestations[0].Message.Aggregate.Data.Target.Epoch) for _, p := range pids { cs, err := s.p2p.Peers().ChainState(p) diff --git a/beacon-chain/sync/pending_blocks_queue.go b/beacon-chain/sync/pending_blocks_queue.go index 9e498ea29bd7..d1205c513d9c 100644 --- a/beacon-chain/sync/pending_blocks_queue.go +++ b/beacon-chain/sync/pending_blocks_queue.go @@ -2,7 +2,9 @@ package sync import ( "context" + "crypto/rand" "encoding/hex" + "math/big" "sort" "sync" @@ -16,7 +18,6 @@ import ( "github.com/prysmaticlabs/prysm/shared/traceutil" "github.com/sirupsen/logrus" "go.opencensus.io/trace" - "golang.org/x/exp/rand" ) var processPendingBlocksPeriod = slotutil.DivideSlotBy(3 /* times per slot */) @@ -80,7 +81,11 @@ func (s *Service) processPendingBlocks(ctx context.Context) error { // Start with a random peer to query, but choose the first peer in our unsorted list that claims to // have a head slot newer than the block slot we are requesting. - pid := pids[rand.Int()%len(pids)] + randInt, err := rand.Int(rand.Reader, big.NewInt(int64(len(pids)))) + if err != nil { + return err + } + pid := pids[randInt.Int64()%int64(len(pids))] for _, p := range pids { cs, err := s.p2p.Peers().ChainState(p) if err != nil { From 6554094b0b0c19a71c5ec46de60637a95ee6651c Mon Sep 17 00:00:00 2001 From: Victor Farazdagi Date: Thu, 25 Jun 2020 22:18:14 +0300 Subject: [PATCH 05/17] define shared/rand --- shared/rand/rand.go | 54 ++++++++++++++++++++++++++ tools/analyzers/cryptorand/analyzer.go | 4 +- 2 files changed, 56 insertions(+), 2 deletions(-) create mode 100644 shared/rand/rand.go diff --git a/shared/rand/rand.go b/shared/rand/rand.go new file mode 100644 index 000000000000..25a89ce65e3d --- /dev/null +++ b/shared/rand/rand.go @@ -0,0 +1,54 @@ +package rand + +import ( + "crypto/rand" + "encoding/binary" + mrand "math/rand" +) + +type source struct{} + +var _ = mrand.Source64(&source{}) + +// Seed does nothing when crypto/rand is used as source. +func (s *source) Seed(seed int64) {} + +// Int63 returns uniformly-distributed random (as in CSPRNG) int64 value within [0, 1<<63) range. +// Panics if random generator reader cannot return data. +func (s *source) Int63() int64 { + return int64(s.Uint64() & ^uint64(1<<63)) +} + +// Uint64 returns uniformly-distributed random (as in CSPRNG) uint64 value within [0, 1<<64) range. +// Panics if random generator reader cannot return data. +func (s *source) Uint64() (val uint64) { + if err := binary.Read(rand.Reader, binary.BigEndian, &val); err != nil { + panic(err) + } + return +} + +// Rand is alias for underlying random generator. +type Rand = mrand.Rand + +// New returns a new generator that uses random values from crypto/rand as a source (cryptographically +// secure random number generator). +// Panics if crypto/rand input cannot be read. +// Use it for everything where crypto secure non-deterministic randomness is required. Performance +// takes a hit, so use sparingly. +func NewRandomGenerator() *Rand { + return mrand.New(&source{}) +} + +// NewPseudoRandomGenerator returns a random generator which is only seeded with crypto/rand, but +// is deterministic otherwise (given seed, produces given results, deterministically). +// Panics if crypto/rand input cannot be read. +// Use this method for performance, where deterministic pseudo-random behaviour is enough. Otherwise, +// rely on NewRandomGenerator(). +func NewPseudoRandomGenerator() *Rand { + var seed int64 + if err := binary.Read(rand.Reader, binary.BigEndian, seed); err != nil { + panic(err) + } + return mrand.New(mrand.NewSource(seed)) +} diff --git a/tools/analyzers/cryptorand/analyzer.go b/tools/analyzers/cryptorand/analyzer.go index 0a74638aa14d..5c6552e047c3 100644 --- a/tools/analyzers/cryptorand/analyzer.go +++ b/tools/analyzers/cryptorand/analyzer.go @@ -14,7 +14,7 @@ import ( // Doc explaining the tool. const Doc = "Tool to enforce the use of stronger crypto: crypto/rand instead of math/rand" -var errWeakCrypto = errors.New("weak cryptography, use crypto/rand, or add an exclusion to //:nogo.json") +var errWeakCrypto = errors.New("crypto-secure RNGs are required, use CSPRNG or PRNG defined in github.com/prysmaticlabs/prysm/shared/rand") // Analyzer runs static analysis. var Analyzer = &analysis.Analyzer{ @@ -48,7 +48,7 @@ func run(pass *analysis.Pass) (interface{}, error) { case *ast.ImportSpec: // Collect aliases to rand packages. pkg := stmt.Path.Value - if strings.HasSuffix(pkg, "/rand\"") && !strings.Contains(pkg, "crypto/rand") { + if strings.HasSuffix(pkg, "/rand\"") && !strings.Contains(pkg, "/prysm/shared/rand") { if stmt.Name != nil { aliases[stmt.Name.Name] = stmt.Path.Value } else { From ac8d228f16e87966ab204e695cd98464e6da64bc Mon Sep 17 00:00:00 2001 From: Victor Farazdagi Date: Thu, 25 Jun 2020 22:32:49 +0300 Subject: [PATCH 06/17] updates fetcher --- beacon-chain/sync/BUILD.bazel | 1 - beacon-chain/sync/initial-sync/BUILD.bazel | 1 + .../sync/initial-sync/blocks_fetcher.go | 26 +++++-------------- shared/rand/BUILD.bazel | 8 ++++++ .../analyzers/cryptorand/testdata/BUILD.bazel | 2 +- 5 files changed, 16 insertions(+), 22 deletions(-) create mode 100644 shared/rand/BUILD.bazel diff --git a/beacon-chain/sync/BUILD.bazel b/beacon-chain/sync/BUILD.bazel index 359721c17612..44d7bb99fc38 100644 --- a/beacon-chain/sync/BUILD.bazel +++ b/beacon-chain/sync/BUILD.bazel @@ -88,7 +88,6 @@ go_library( "@com_github_prysmaticlabs_ethereumapis//eth/v1alpha1:go_default_library", "@com_github_sirupsen_logrus//:go_default_library", "@io_opencensus_go//trace:go_default_library", - "@org_golang_x_exp//rand:go_default_library", ], ) diff --git a/beacon-chain/sync/initial-sync/BUILD.bazel b/beacon-chain/sync/initial-sync/BUILD.bazel index 391aee7c0029..8ed8dfb84b0a 100644 --- a/beacon-chain/sync/initial-sync/BUILD.bazel +++ b/beacon-chain/sync/initial-sync/BUILD.bazel @@ -30,6 +30,7 @@ go_library( "//shared/bytesutil:go_default_library", "//shared/mathutil:go_default_library", "//shared/params:go_default_library", + "//shared/rand:go_default_library", "//shared/roughtime:go_default_library", "@com_github_kevinms_leakybucket_go//:go_default_library", "@com_github_libp2p_go_libp2p_core//peer:go_default_library", diff --git a/beacon-chain/sync/initial-sync/blocks_fetcher.go b/beacon-chain/sync/initial-sync/blocks_fetcher.go index ff060961bae2..519b69f080f0 100644 --- a/beacon-chain/sync/initial-sync/blocks_fetcher.go +++ b/beacon-chain/sync/initial-sync/blocks_fetcher.go @@ -2,12 +2,9 @@ package initialsync import ( "context" - "crypto/rand" "fmt" "io" "math" - "math/big" - mathRand "math/rand" "sort" "sync" "time" @@ -24,6 +21,7 @@ import ( p2ppb "github.com/prysmaticlabs/prysm/proto/beacon/p2p/v1" "github.com/prysmaticlabs/prysm/shared/mathutil" "github.com/prysmaticlabs/prysm/shared/params" + "github.com/prysmaticlabs/prysm/shared/rand" "github.com/prysmaticlabs/prysm/shared/roughtime" "github.com/sirupsen/logrus" "go.opencensus.io/trace" @@ -64,6 +62,7 @@ type blocksFetcher struct { sync.Mutex ctx context.Context cancel context.CancelFunc + rand *rand.Rand headFetcher blockchain.HeadFetcher p2p p2p.P2P blocksPerSecond uint64 @@ -108,6 +107,7 @@ func newBlocksFetcher(ctx context.Context, cfg *blocksFetcherConfig) *blocksFetc return &blocksFetcher{ ctx: ctx, cancel: cancel, + rand: rand.NewRandomGenerator(), headFetcher: cfg.headFetcher, p2p: cfg.p2p, blocksPerSecond: uint64(blocksPerSecond), @@ -387,11 +387,7 @@ func (f *blocksFetcher) selectFailOverPeer(excludedPID peer.ID, peers []peer.ID) return "", errNoPeersAvailable } - randInt, err := rand.Int(rand.Reader, big.NewInt(int64(len(peers)))) - if err != nil { - return "", err - } - return peers[randInt.Int64()], nil + return peers[f.rand.Int()%len(peers)], nil } // waitForMinimumPeers spins and waits up until enough peers are available. @@ -425,12 +421,7 @@ func (f *blocksFetcher) filterPeers(peers []peer.ID, peersPercentage float64) ([ // Shuffle peers to prevent a bad peer from // stalling sync with invalid blocks. - randSource, err := rand.Int(rand.Reader, big.NewInt(roughtime.Now().Unix())) - if err != nil { - return nil, errors.Wrap(err, "could not generate random int") - } - randGenerator := mathRand.New(mathRand.NewSource(randSource.Int64())) - randGenerator.Shuffle(len(peers), func(i, j int) { + f.rand.Shuffle(len(peers), func(i, j int) { peers[i], peers[j] = peers[j], peers[i] }) @@ -478,11 +469,6 @@ func (f *blocksFetcher) nonSkippedSlotAfter(ctx context.Context, slot uint64) (u return 0, errNoPeersAvailable } - randSource, err := rand.Int(rand.Reader, big.NewInt(roughtime.Now().Unix())) - if err != nil { - return 0, errors.Wrap(err, "could not generate random int") - } - randGenerator := mathRand.New(mathRand.NewSource(randSource.Int64())) slotsPerEpoch := params.BeaconConfig().SlotsPerEpoch pidInd := 0 @@ -525,7 +511,7 @@ func (f *blocksFetcher) nonSkippedSlotAfter(ctx context.Context, slot uint64) (u slot = slot + nonSkippedSlotsFullSearchEpochs*slotsPerEpoch upperBoundSlot := helpers.StartSlot(epoch + 1) for ind := slot + 1; ind < upperBoundSlot; ind += (slotsPerEpoch * slotsPerEpoch) / 2 { - start := ind + uint64(randGenerator.Intn(int(slotsPerEpoch))) + start := ind + uint64(f.rand.Intn(int(slotsPerEpoch))) nextSlot, err := fetch(peers[pidInd%len(peers)], start, slotsPerEpoch/2, slotsPerEpoch) if err != nil { return 0, err diff --git a/shared/rand/BUILD.bazel b/shared/rand/BUILD.bazel new file mode 100644 index 000000000000..09cfcbfffd45 --- /dev/null +++ b/shared/rand/BUILD.bazel @@ -0,0 +1,8 @@ +load("@prysm//tools/go:def.bzl", "go_library") + +go_library( + name = "go_default_library", + srcs = ["rand.go"], + importpath = "github.com/prysmaticlabs/prysm/shared/rand", + visibility = ["//visibility:public"], +) diff --git a/tools/analyzers/cryptorand/testdata/BUILD.bazel b/tools/analyzers/cryptorand/testdata/BUILD.bazel index fc00232e9535..49b7794a3fa5 100644 --- a/tools/analyzers/cryptorand/testdata/BUILD.bazel +++ b/tools/analyzers/cryptorand/testdata/BUILD.bazel @@ -3,8 +3,8 @@ load("@prysm//tools/go:def.bzl", "go_library") go_library( name = "go_default_library", srcs = [ + "custom_import.go", "rand_new.go", - "weak_crypto_custom_import.go", ], importpath = "github.com/prysmaticlabs/prysm/tools/analyzers/cryptorand/testdata", visibility = ["//visibility:public"], From 0195a5ae7daf5829764f102628d0f0654834d4dc Mon Sep 17 00:00:00 2001 From: Victor Farazdagi Date: Fri, 26 Jun 2020 10:26:49 +0300 Subject: [PATCH 07/17] fixes rand issue in sync package --- beacon-chain/sync/BUILD.bazel | 1 + beacon-chain/sync/pending_attestations_queue.go | 10 +++------- beacon-chain/sync/pending_blocks_queue.go | 10 +++------- 3 files changed, 7 insertions(+), 14 deletions(-) diff --git a/beacon-chain/sync/BUILD.bazel b/beacon-chain/sync/BUILD.bazel index 44d7bb99fc38..9055bbd19af0 100644 --- a/beacon-chain/sync/BUILD.bazel +++ b/beacon-chain/sync/BUILD.bazel @@ -70,6 +70,7 @@ go_library( "//shared/messagehandler:go_default_library", "//shared/p2putils:go_default_library", "//shared/params:go_default_library", + "//shared/rand:go_default_library", "//shared/roughtime:go_default_library", "//shared/runutil:go_default_library", "//shared/sliceutil:go_default_library", diff --git a/beacon-chain/sync/pending_attestations_queue.go b/beacon-chain/sync/pending_attestations_queue.go index d803b1c9c50c..4c40ab9a672d 100644 --- a/beacon-chain/sync/pending_attestations_queue.go +++ b/beacon-chain/sync/pending_attestations_queue.go @@ -2,9 +2,7 @@ package sync import ( "context" - "crypto/rand" "encoding/hex" - "math/big" "sync" pubsub "github.com/libp2p/go-libp2p-pubsub" @@ -15,6 +13,7 @@ import ( "github.com/prysmaticlabs/prysm/shared/bytesutil" "github.com/prysmaticlabs/prysm/shared/featureconfig" "github.com/prysmaticlabs/prysm/shared/params" + "github.com/prysmaticlabs/prysm/shared/rand" "github.com/prysmaticlabs/prysm/shared/runutil" "github.com/prysmaticlabs/prysm/shared/slotutil" "github.com/prysmaticlabs/prysm/shared/traceutil" @@ -61,6 +60,7 @@ func (s *Service) processPendingAtts(ctx context.Context) error { } s.pendingAttsLock.RUnlock() + randGen := rand.NewRandomGenerator() for _, bRoot := range roots { s.pendingAttsLock.RLock() attestations := s.blkRootToPendingAtts[bRoot] @@ -131,11 +131,7 @@ func (s *Service) processPendingAtts(ctx context.Context) error { log.Debug("No peer IDs available to request missing block from for pending attestation") return nil } - randInt, err := rand.Int(rand.Reader, big.NewInt(int64(len(pids)))) - if err != nil { - return err - } - pid := pids[randInt.Int64()%int64(len(pids))] + pid := pids[randGen.Int()%len(pids)] targetSlot := helpers.SlotToEpoch(attestations[0].Message.Aggregate.Data.Target.Epoch) for _, p := range pids { cs, err := s.p2p.Peers().ChainState(p) diff --git a/beacon-chain/sync/pending_blocks_queue.go b/beacon-chain/sync/pending_blocks_queue.go index d1205c513d9c..42a0c6e7d1c8 100644 --- a/beacon-chain/sync/pending_blocks_queue.go +++ b/beacon-chain/sync/pending_blocks_queue.go @@ -2,9 +2,7 @@ package sync import ( "context" - "crypto/rand" "encoding/hex" - "math/big" "sort" "sync" @@ -13,6 +11,7 @@ import ( "github.com/prysmaticlabs/prysm/beacon-chain/core/helpers" "github.com/prysmaticlabs/prysm/beacon-chain/state/stateutil" "github.com/prysmaticlabs/prysm/shared/bytesutil" + "github.com/prysmaticlabs/prysm/shared/rand" "github.com/prysmaticlabs/prysm/shared/runutil" "github.com/prysmaticlabs/prysm/shared/slotutil" "github.com/prysmaticlabs/prysm/shared/traceutil" @@ -52,6 +51,7 @@ func (s *Service) processPendingBlocks(ctx context.Context) error { trace.Int64Attribute("numPeers", int64(len(pids))), ) + randGen := rand.NewRandomGenerator() for _, slot := range slots { ctx, span := trace.StartSpan(ctx, "processPendingBlocks.InnerLoop") span.AddAttributes(trace.Int64Attribute("slot", int64(slot))) @@ -81,11 +81,7 @@ func (s *Service) processPendingBlocks(ctx context.Context) error { // Start with a random peer to query, but choose the first peer in our unsorted list that claims to // have a head slot newer than the block slot we are requesting. - randInt, err := rand.Int(rand.Reader, big.NewInt(int64(len(pids)))) - if err != nil { - return err - } - pid := pids[randInt.Int64()%int64(len(pids))] + pid := pids[randGen.Int()%len(pids)] for _, p := range pids { cs, err := s.p2p.Peers().ChainState(p) if err != nil { From 4e23537a0355e58a0a61b7991b6650e0b17a0a4c Mon Sep 17 00:00:00 2001 From: Victor Farazdagi Date: Fri, 26 Jun 2020 10:27:03 +0300 Subject: [PATCH 08/17] gofmt --- tools/analyzers/cryptorand/testdata/rand_new.go | 1 - 1 file changed, 1 deletion(-) diff --git a/tools/analyzers/cryptorand/testdata/rand_new.go b/tools/analyzers/cryptorand/testdata/rand_new.go index 17b333426497..aa82713f7f24 100644 --- a/tools/analyzers/cryptorand/testdata/rand_new.go +++ b/tools/analyzers/cryptorand/testdata/rand_new.go @@ -11,7 +11,6 @@ func UseRandNew() { start := uint64(randGenerator.Intn(32)) _ = start - randGenerator = rand.New(rand.NewSource(time.Now().UnixNano())) } From f4cb6c78240bf538c5ce3b606e5e4e0924e7a369 Mon Sep 17 00:00:00 2001 From: Victor Farazdagi Date: Fri, 26 Jun 2020 10:27:41 +0300 Subject: [PATCH 09/17] shared/rand: more docs + add exclusion nogo_config.json --- nogo_config.json | 3 ++- shared/rand/rand.go | 32 ++++++++++++++++++++++++++++++-- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/nogo_config.json b/nogo_config.json index 061778761262..d31880dd5e49 100644 --- a/nogo_config.json +++ b/nogo_config.json @@ -106,7 +106,8 @@ "validator/.*": "" }, "exclude_files": { - ".*/.*_test\\.go": "Tests are OK to use weak crypto" + ".*/.*_test\\.go": "Tests are OK to use weak crypto", + "shared/rand/rand\\.go": "Abstracts CSPRNGs for common use" } } } diff --git a/shared/rand/rand.go b/shared/rand/rand.go index 25a89ce65e3d..b11597988169 100644 --- a/shared/rand/rand.go +++ b/shared/rand/rand.go @@ -1,3 +1,31 @@ +/* +Package rand defines methods of obtaining cryptographically secure random number generators. + +One is expected to use randomness from this package only, without introducing any other packages. +This limits the scope of code that needs to be hardened. + +There are two modes, one for deterministic and another non-deterministic randomness: +1. If deterministic pseudo-random generator is enough, use: + + import "github.com/prysmaticlabs/prysm/shared/rand" + randGen := rand.NewDeterministicRandomGenerator() + randGen.Intn(32) // or any other func defined in math.rand API + + In this mode, only seed is generated using cryptographically secure source (crypto/rand). So, + once seed is obtained, and generator is seeded, all the rest usage is deterministic, and fast. + This method is still better than using unix time for source of randomness - since time is not a + good source of seed randomness, when you have many concurrent servers using it. + +2. For cryptographically secure non-deterministic mode (CSPRNG), use: + + import "github.com/prysmaticlabs/prysm/shared/rand" + randGen := rand.NewRandomGenerator() + randGen.Intn(32) // or any other func defined in math.rand API + + Again, any of the functions from `math/rand` can be used, however, they all use custom source + of randomness (crypto/rand), on every step. This makes randomness non-deterministic. However, + you take a performance hit -- as it is an order of magnitude slower. +*/ package rand import ( @@ -40,12 +68,12 @@ func NewRandomGenerator() *Rand { return mrand.New(&source{}) } -// NewPseudoRandomGenerator returns a random generator which is only seeded with crypto/rand, but +// NewDeterministicRandomGenerator returns a random generator which is only seeded with crypto/rand, but // is deterministic otherwise (given seed, produces given results, deterministically). // Panics if crypto/rand input cannot be read. // Use this method for performance, where deterministic pseudo-random behaviour is enough. Otherwise, // rely on NewRandomGenerator(). -func NewPseudoRandomGenerator() *Rand { +func NewDeterministicRandomGenerator() *Rand { var seed int64 if err := binary.Read(rand.Reader, binary.BigEndian, seed); err != nil { panic(err) From 4ba4616f7c1c2559af63105bb26a201f2b8cb551 Mon Sep 17 00:00:00 2001 From: Victor Farazdagi Date: Fri, 26 Jun 2020 10:50:17 +0300 Subject: [PATCH 10/17] updates validator/assignments --- beacon-chain/rpc/validator/BUILD.bazel | 1 + beacon-chain/rpc/validator/assignments.go | 35 +++++++---------------- 2 files changed, 12 insertions(+), 24 deletions(-) diff --git a/beacon-chain/rpc/validator/BUILD.bazel b/beacon-chain/rpc/validator/BUILD.bazel index 8dc37d795453..f8e0ee03aed8 100644 --- a/beacon-chain/rpc/validator/BUILD.bazel +++ b/beacon-chain/rpc/validator/BUILD.bazel @@ -43,6 +43,7 @@ go_library( "//shared/featureconfig:go_default_library", "//shared/hashutil:go_default_library", "//shared/params:go_default_library", + "//shared/rand:go_default_library", "//shared/roughtime:go_default_library", "//shared/slotutil:go_default_library", "//shared/traceutil:go_default_library", diff --git a/beacon-chain/rpc/validator/assignments.go b/beacon-chain/rpc/validator/assignments.go index 4d62756ac63c..8cd0fe52351c 100644 --- a/beacon-chain/rpc/validator/assignments.go +++ b/beacon-chain/rpc/validator/assignments.go @@ -2,11 +2,8 @@ package validator import ( "context" - "crypto/rand" - "math/big" "time" - "github.com/pkg/errors" ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1" "github.com/prysmaticlabs/prysm/beacon-chain/cache" "github.com/prysmaticlabs/prysm/beacon-chain/core/feed" @@ -15,6 +12,7 @@ import ( "github.com/prysmaticlabs/prysm/beacon-chain/core/state" "github.com/prysmaticlabs/prysm/shared/bytesutil" "github.com/prysmaticlabs/prysm/shared/params" + "github.com/prysmaticlabs/prysm/shared/rand" "github.com/prysmaticlabs/prysm/shared/roughtime" "github.com/prysmaticlabs/prysm/shared/slotutil" "google.golang.org/grpc/codes" @@ -178,12 +176,8 @@ func (vs *Server) duties(ctx context.Context, req *ethpb.DutiesRequest) (*ethpb. validatorAssignments = append(validatorAssignments, assignment) nextValidatorAssignments = append(nextValidatorAssignments, nextAssignment) // Assign relevant validator to subnet. - if err := assignValidatorToSubnet(pubKey, assignment.Status); err != nil { - return nil, errors.Wrap(err, "could not assign validator to subnet") - } - if err := assignValidatorToSubnet(pubKey, nextAssignment.Status); err != nil { - return nil, errors.Wrap(err, "could not assign validator to subnet") - } + assignValidatorToSubnet(pubKey, assignment.Status) + assignValidatorToSubnet(pubKey, nextAssignment.Status) } return ðpb.DutiesResponse{ @@ -195,33 +189,26 @@ func (vs *Server) duties(ctx context.Context, req *ethpb.DutiesRequest) (*ethpb. // assignValidatorToSubnet checks the status and pubkey of a particular validator // to discern whether persistent subnets need to be registered for them. -func assignValidatorToSubnet(pubkey []byte, status ethpb.ValidatorStatus) error { +func assignValidatorToSubnet(pubkey []byte, status ethpb.ValidatorStatus) { if status != ethpb.ValidatorStatus_ACTIVE && status != ethpb.ValidatorStatus_EXITING { - return nil + return } _, ok, expTime := cache.SubnetIDs.GetPersistentSubnets(pubkey) if ok && expTime.After(roughtime.Now()) { - return nil + return } epochDuration := time.Duration(params.BeaconConfig().SlotsPerEpoch * params.BeaconConfig().SecondsPerSlot) assignedIdxs := []uint64{} + randGen := rand.NewRandomGenerator() for i := uint64(0); i < params.BeaconNetworkConfig().RandomSubnetsPerValidator; i++ { - randInt, err := rand.Int(rand.Reader, big.NewInt(int64(params.BeaconNetworkConfig().AttestationSubnetCount))) - if err != nil { - return errors.Wrap(err, "could not get random subnet index") - } - assignedIdxs = append(assignedIdxs, randInt.Uint64()) + assignedIdx := randGen.Intn(int(params.BeaconNetworkConfig().AttestationSubnetCount)) + assignedIdxs = append(assignedIdxs, uint64(assignedIdx)) } - randInt, err := rand.Int(rand.Reader, big.NewInt(int64(params.BeaconNetworkConfig().EpochsPerRandomSubnetSubscription))) - if err != nil { - return errors.Wrap(err, "could not get random subnet duration") - } - assignedDuration := randInt.Int64() - assignedDuration += int64(params.BeaconNetworkConfig().EpochsPerRandomSubnetSubscription) + assignedDuration := randGen.Intn(int(params.BeaconNetworkConfig().EpochsPerRandomSubnetSubscription)) + assignedDuration += int(params.BeaconNetworkConfig().EpochsPerRandomSubnetSubscription) totalDuration := epochDuration * time.Duration(assignedDuration) cache.SubnetIDs.AddPersistentCommittee(pubkey, assignedIdxs, totalDuration*time.Second) - return nil } From f840c96c93be5748d6d4d24cc174a16a67b44926 Mon Sep 17 00:00:00 2001 From: Victor Farazdagi Date: Fri, 26 Jun 2020 10:50:42 +0300 Subject: [PATCH 11/17] updates comment --- shared/rand/rand.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/shared/rand/rand.go b/shared/rand/rand.go index b11597988169..25bf6299d9b3 100644 --- a/shared/rand/rand.go +++ b/shared/rand/rand.go @@ -14,7 +14,8 @@ There are two modes, one for deterministic and another non-deterministic randomn In this mode, only seed is generated using cryptographically secure source (crypto/rand). So, once seed is obtained, and generator is seeded, all the rest usage is deterministic, and fast. This method is still better than using unix time for source of randomness - since time is not a - good source of seed randomness, when you have many concurrent servers using it. + good source of seed randomness, when you have many concurrent servers using it (and can coincide + in start times). 2. For cryptographically secure non-deterministic mode (CSPRNG), use: From de290fa8e44ebc6755c05dcc074887a5a5733951 Mon Sep 17 00:00:00 2001 From: Victor Farazdagi Date: Fri, 26 Jun 2020 11:10:50 +0300 Subject: [PATCH 12/17] fixes remaning cases --- beacon-chain/rpc/service.go | 3 --- beacon-chain/rpc/validator/assignments_test.go | 4 +--- beacon-chain/rpc/validator/proposer.go | 17 ++++------------- 3 files changed, 5 insertions(+), 19 deletions(-) diff --git a/beacon-chain/rpc/service.go b/beacon-chain/rpc/service.go index b5ec263aee61..8b9df15c392c 100644 --- a/beacon-chain/rpc/service.go +++ b/beacon-chain/rpc/service.go @@ -5,9 +5,7 @@ package rpc import ( "context" "fmt" - "math/rand" "net" - "os" middleware "github.com/grpc-ecosystem/go-grpc-middleware" recovery "github.com/grpc-ecosystem/go-grpc-middleware/recovery" @@ -50,7 +48,6 @@ var log logrus.FieldLogger func init() { log = logrus.WithField("prefix", "rpc") - rand.Seed(int64(os.Getpid())) } // Service defining an RPC server for a beacon node. diff --git a/beacon-chain/rpc/validator/assignments_test.go b/beacon-chain/rpc/validator/assignments_test.go index 611fe9b1af6c..59d9f38e76a1 100644 --- a/beacon-chain/rpc/validator/assignments_test.go +++ b/beacon-chain/rpc/validator/assignments_test.go @@ -467,9 +467,7 @@ func TestStreamDuties_OK_ChainReorg(t *testing.T) { func TestAssignValidatorToSubnet(t *testing.T) { k := pubKey(3) - if err := assignValidatorToSubnet(k, ethpb.ValidatorStatus_ACTIVE); err != nil { - t.Fatal(err) - } + assignValidatorToSubnet(k, ethpb.ValidatorStatus_ACTIVE) coms, ok, exp := cache.SubnetIDs.GetPersistentSubnets(k) if !ok { t.Fatal("No cache entry found for validator") diff --git a/beacon-chain/rpc/validator/proposer.go b/beacon-chain/rpc/validator/proposer.go index 39e478a1a782..162ed4bcbf83 100644 --- a/beacon-chain/rpc/validator/proposer.go +++ b/beacon-chain/rpc/validator/proposer.go @@ -2,7 +2,6 @@ package validator import ( "context" - "crypto/rand" "fmt" "math/big" "time" @@ -23,6 +22,7 @@ import ( "github.com/prysmaticlabs/prysm/shared/bytesutil" "github.com/prysmaticlabs/prysm/shared/hashutil" "github.com/prysmaticlabs/prysm/shared/params" + "github.com/prysmaticlabs/prysm/shared/rand" "github.com/prysmaticlabs/prysm/shared/trieutil" "go.opencensus.io/trace" "google.golang.org/grpc/codes" @@ -223,18 +223,9 @@ func (vs *Server) randomETH1DataVote(ctx context.Context) (*ethpb.Eth1Data, erro } // set random roots and block hashes to prevent a majority from being // built if the eth1 node is offline - randomDepBytes := make([]byte, 32) - randomBlkBytes := make([]byte, 32) - _, err = rand.Read(randomDepBytes) - if err != nil { - return nil, err - } - _, err = rand.Read(randomBlkBytes) - if err != nil { - return nil, err - } - depRoot := hashutil.Hash(randomDepBytes) - blockHash := hashutil.Hash(randomBlkBytes) + randGen := rand.NewRandomGenerator() + depRoot := hashutil.Hash(bytesutil.Bytes32(randGen.Uint64())) + blockHash := hashutil.Hash(bytesutil.Bytes32(randGen.Uint64())) return ðpb.Eth1Data{ DepositRoot: depRoot[:], DepositCount: headState.Eth1DepositIndex(), From 47bf4d9374e4978a531bea96a96503ae6c3845f0 Mon Sep 17 00:00:00 2001 From: Victor Farazdagi Date: Fri, 26 Jun 2020 11:13:48 +0300 Subject: [PATCH 13/17] re-arranges comments --- shared/rand/rand.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/shared/rand/rand.go b/shared/rand/rand.go index 25bf6299d9b3..f560e1b15889 100644 --- a/shared/rand/rand.go +++ b/shared/rand/rand.go @@ -60,8 +60,8 @@ func (s *source) Uint64() (val uint64) { // Rand is alias for underlying random generator. type Rand = mrand.Rand -// New returns a new generator that uses random values from crypto/rand as a source (cryptographically -// secure random number generator). +// NewRandomGenerator returns a new generator that uses random values from crypto/rand as a source +// (cryptographically secure random number generator). // Panics if crypto/rand input cannot be read. // Use it for everything where crypto secure non-deterministic randomness is required. Performance // takes a hit, so use sparingly. @@ -69,11 +69,11 @@ func NewRandomGenerator() *Rand { return mrand.New(&source{}) } -// NewDeterministicRandomGenerator returns a random generator which is only seeded with crypto/rand, but -// is deterministic otherwise (given seed, produces given results, deterministically). +// NewDeterministicRandomGenerator returns a random generator which is only seeded with crypto/rand, +// but is deterministic otherwise (given seed, produces given results, deterministically). // Panics if crypto/rand input cannot be read. -// Use this method for performance, where deterministic pseudo-random behaviour is enough. Otherwise, -// rely on NewRandomGenerator(). +// Use this method for performance, where deterministic pseudo-random behaviour is enough. +// Otherwise, rely on NewRandomGenerator(). func NewDeterministicRandomGenerator() *Rand { var seed int64 if err := binary.Read(rand.Reader, binary.BigEndian, seed); err != nil { From 7e51b05f0af0d3636a0f31dc783a6b961b8bedd8 Mon Sep 17 00:00:00 2001 From: Victor Farazdagi Date: Fri, 26 Jun 2020 11:50:23 +0300 Subject: [PATCH 14/17] fixes tests --- beacon-chain/db/testing/BUILD.bazel | 1 + beacon-chain/db/testing/setup_db.go | 8 ++------ nogo_config.json | 3 ++- shared/rand/rand.go | 7 ++----- shared/testutil/BUILD.bazel | 1 + shared/testutil/block.go | 12 +++++++----- shared/testutil/helpers.go | 4 ++-- validator/db/BUILD.bazel | 1 + validator/db/setup_db.go | 9 +++------ 9 files changed, 21 insertions(+), 25 deletions(-) diff --git a/beacon-chain/db/testing/BUILD.bazel b/beacon-chain/db/testing/BUILD.bazel index c5d90826b86e..7ecee6779800 100644 --- a/beacon-chain/db/testing/BUILD.bazel +++ b/beacon-chain/db/testing/BUILD.bazel @@ -10,6 +10,7 @@ go_library( "//beacon-chain/cache:go_default_library", "//beacon-chain/db:go_default_library", "//beacon-chain/db/kv:go_default_library", + "//shared/rand:go_default_library", "//shared/testutil:go_default_library", ], ) diff --git a/beacon-chain/db/testing/setup_db.go b/beacon-chain/db/testing/setup_db.go index 5377252c210c..a9a7cf40423f 100644 --- a/beacon-chain/db/testing/setup_db.go +++ b/beacon-chain/db/testing/setup_db.go @@ -3,9 +3,7 @@ package testing import ( - "crypto/rand" "fmt" - "math/big" "os" "path" "testing" @@ -13,15 +11,13 @@ import ( "github.com/prysmaticlabs/prysm/beacon-chain/cache" "github.com/prysmaticlabs/prysm/beacon-chain/db" "github.com/prysmaticlabs/prysm/beacon-chain/db/kv" + "github.com/prysmaticlabs/prysm/shared/rand" "github.com/prysmaticlabs/prysm/shared/testutil" ) // SetupDB instantiates and returns database backed by key value store. func SetupDB(t testing.TB) (db.Database, *cache.StateSummaryCache) { - randPath, err := rand.Int(rand.Reader, big.NewInt(1000000)) - if err != nil { - t.Fatalf("could not generate random file path: %v", err) - } + randPath := rand.NewDeterministicRandomGenerator().Int() p := path.Join(testutil.TempDir(), fmt.Sprintf("/%d", randPath)) if err := os.RemoveAll(p); err != nil { t.Fatalf("failed to remove directory: %v", err) diff --git a/nogo_config.json b/nogo_config.json index d31880dd5e49..0693639b3916 100644 --- a/nogo_config.json +++ b/nogo_config.json @@ -107,7 +107,8 @@ }, "exclude_files": { ".*/.*_test\\.go": "Tests are OK to use weak crypto", - "shared/rand/rand\\.go": "Abstracts CSPRNGs for common use" + "shared/rand/rand\\.go": "Abstracts CSPRNGs for common use", + "shared/aggregation/testing/bitlistutils.go": "Test-only package" } } } diff --git a/shared/rand/rand.go b/shared/rand/rand.go index f560e1b15889..4206422609d1 100644 --- a/shared/rand/rand.go +++ b/shared/rand/rand.go @@ -75,9 +75,6 @@ func NewRandomGenerator() *Rand { // Use this method for performance, where deterministic pseudo-random behaviour is enough. // Otherwise, rely on NewRandomGenerator(). func NewDeterministicRandomGenerator() *Rand { - var seed int64 - if err := binary.Read(rand.Reader, binary.BigEndian, seed); err != nil { - panic(err) - } - return mrand.New(mrand.NewSource(seed)) + randGen := NewRandomGenerator() + return mrand.New(mrand.NewSource(randGen.Int63())) } diff --git a/shared/testutil/BUILD.bazel b/shared/testutil/BUILD.bazel index 7933e35a492b..999f57b7ff14 100644 --- a/shared/testutil/BUILD.bazel +++ b/shared/testutil/BUILD.bazel @@ -27,6 +27,7 @@ go_library( "//shared/hashutil:go_default_library", "//shared/interop:go_default_library", "//shared/params:go_default_library", + "//shared/rand:go_default_library", "//shared/trieutil:go_default_library", "@com_github_ghodss_yaml//:go_default_library", "@com_github_gogo_protobuf//proto:go_default_library", diff --git a/shared/testutil/block.go b/shared/testutil/block.go index ec1787dc9915..59ab45026c6e 100644 --- a/shared/testutil/block.go +++ b/shared/testutil/block.go @@ -5,7 +5,6 @@ import ( "fmt" "log" "math" - "math/rand" "github.com/pkg/errors" ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1" @@ -17,6 +16,7 @@ import ( "github.com/prysmaticlabs/prysm/shared/bls" "github.com/prysmaticlabs/prysm/shared/bytesutil" "github.com/prysmaticlabs/prysm/shared/params" + "github.com/prysmaticlabs/prysm/shared/rand" ) // BlockGenConfig is used to define the requested conditions @@ -316,13 +316,14 @@ func generateAttesterSlashings( numSlashings uint64, ) ([]*ethpb.AttesterSlashing, error) { attesterSlashings := make([]*ethpb.AttesterSlashing, numSlashings) + randGen := rand.NewDeterministicRandomGenerator() for i := uint64(0); i < numSlashings; i++ { - committeeIndex := rand.Uint64() % params.BeaconConfig().MaxCommitteesPerSlot + committeeIndex := randGen.Uint64() % params.BeaconConfig().MaxCommitteesPerSlot committee, err := helpers.BeaconCommitteeFromState(bState, bState.Slot(), committeeIndex) if err != nil { return nil, err } - randIndex := rand.Uint64() % uint64(len(committee)) + randIndex := randGen.Uint64() % uint64(len(committee)) valIndex := committee[randIndex] slashing, err := GenerateAttesterSlashingForValidator(bState, privs[valIndex], valIndex) if err != nil { @@ -379,8 +380,9 @@ func GenerateAttestations(bState *stateTrie.BeaconState, privs []bls.SecretKey, } } if randomRoot { + randGen := rand.NewDeterministicRandomGenerator() b := make([]byte, 32) - _, err := rand.Read(b) + _, err := randGen.Read(b) if err != nil { return nil, err } @@ -527,5 +529,5 @@ func randValIndex(bState *stateTrie.BeaconState) (uint64, error) { if err != nil { return 0, err } - return rand.Uint64() % activeCount, nil + return rand.NewRandomGenerator().Uint64() % activeCount, nil } diff --git a/shared/testutil/helpers.go b/shared/testutil/helpers.go index 0d8777cb5a47..e4f387a47162 100644 --- a/shared/testutil/helpers.go +++ b/shared/testutil/helpers.go @@ -3,7 +3,6 @@ package testutil import ( "context" "encoding/binary" - "math/rand" "testing" "github.com/pkg/errors" @@ -13,6 +12,7 @@ import ( stateTrie "github.com/prysmaticlabs/prysm/beacon-chain/state" "github.com/prysmaticlabs/prysm/shared/bls" "github.com/prysmaticlabs/prysm/shared/params" + "github.com/prysmaticlabs/prysm/shared/rand" ) // RandaoReveal returns a signature of the requested epoch using the beacon proposer private key. @@ -76,7 +76,7 @@ func BlockSignature( // Random32Bytes generates a random 32 byte slice. func Random32Bytes(t *testing.T) []byte { b := make([]byte, 32) - _, err := rand.Read(b) + _, err := rand.NewDeterministicRandomGenerator().Read(b) if err != nil { t.Fatal(err) } diff --git a/validator/db/BUILD.bazel b/validator/db/BUILD.bazel index 85b5b3a95bfb..3ecf4df7cc02 100644 --- a/validator/db/BUILD.bazel +++ b/validator/db/BUILD.bazel @@ -16,6 +16,7 @@ go_library( deps = [ "//proto/slashing:go_default_library", "//shared/params:go_default_library", + "//shared/rand:go_default_library", "//validator/db/iface:go_default_library", "@com_github_gogo_protobuf//proto:go_default_library", "@com_github_pkg_errors//:go_default_library", diff --git a/validator/db/setup_db.go b/validator/db/setup_db.go index 8bc075ec5e76..4301dc02df7f 100644 --- a/validator/db/setup_db.go +++ b/validator/db/setup_db.go @@ -1,20 +1,17 @@ package db import ( - "crypto/rand" "fmt" - "math/big" "os" "path/filepath" "testing" + + "github.com/prysmaticlabs/prysm/shared/rand" ) // SetupDB instantiates and returns a DB instance for the validator client. func SetupDB(t testing.TB, pubkeys [][48]byte) *Store { - randPath, err := rand.Int(rand.Reader, big.NewInt(1000000)) - if err != nil { - t.Fatalf("Could not generate random file path: %v", err) - } + randPath := rand.NewDeterministicRandomGenerator().Int() p := filepath.Join(TempDir(), fmt.Sprintf("/%d", randPath)) if err := os.RemoveAll(p); err != nil { t.Fatalf("Failed to remove directory: %v", err) From 21d605aca3ad7a4afe76092e454486e0dc79abf0 Mon Sep 17 00:00:00 2001 From: Victor Farazdagi Date: Fri, 26 Jun 2020 11:55:32 +0300 Subject: [PATCH 15/17] renames in shared/rand API --- beacon-chain/db/testing/setup_db.go | 2 +- beacon-chain/rpc/validator/assignments.go | 2 +- beacon-chain/rpc/validator/proposer.go | 2 +- beacon-chain/sync/initial-sync/blocks_fetcher.go | 2 +- beacon-chain/sync/pending_attestations_queue.go | 2 +- beacon-chain/sync/pending_blocks_queue.go | 2 +- shared/rand/rand.go | 16 ++++++++-------- shared/testutil/block.go | 6 +++--- shared/testutil/helpers.go | 2 +- validator/db/setup_db.go | 2 +- 10 files changed, 19 insertions(+), 19 deletions(-) diff --git a/beacon-chain/db/testing/setup_db.go b/beacon-chain/db/testing/setup_db.go index a9a7cf40423f..15b181dbff73 100644 --- a/beacon-chain/db/testing/setup_db.go +++ b/beacon-chain/db/testing/setup_db.go @@ -17,7 +17,7 @@ import ( // SetupDB instantiates and returns database backed by key value store. func SetupDB(t testing.TB) (db.Database, *cache.StateSummaryCache) { - randPath := rand.NewDeterministicRandomGenerator().Int() + randPath := rand.NewDeterministicGenerator().Int() p := path.Join(testutil.TempDir(), fmt.Sprintf("/%d", randPath)) if err := os.RemoveAll(p); err != nil { t.Fatalf("failed to remove directory: %v", err) diff --git a/beacon-chain/rpc/validator/assignments.go b/beacon-chain/rpc/validator/assignments.go index 8cd0fe52351c..7b3a2cd2b684 100644 --- a/beacon-chain/rpc/validator/assignments.go +++ b/beacon-chain/rpc/validator/assignments.go @@ -200,7 +200,7 @@ func assignValidatorToSubnet(pubkey []byte, status ethpb.ValidatorStatus) { } epochDuration := time.Duration(params.BeaconConfig().SlotsPerEpoch * params.BeaconConfig().SecondsPerSlot) assignedIdxs := []uint64{} - randGen := rand.NewRandomGenerator() + randGen := rand.NewGenerator() for i := uint64(0); i < params.BeaconNetworkConfig().RandomSubnetsPerValidator; i++ { assignedIdx := randGen.Intn(int(params.BeaconNetworkConfig().AttestationSubnetCount)) assignedIdxs = append(assignedIdxs, uint64(assignedIdx)) diff --git a/beacon-chain/rpc/validator/proposer.go b/beacon-chain/rpc/validator/proposer.go index 162ed4bcbf83..abbd81f4364b 100644 --- a/beacon-chain/rpc/validator/proposer.go +++ b/beacon-chain/rpc/validator/proposer.go @@ -223,7 +223,7 @@ func (vs *Server) randomETH1DataVote(ctx context.Context) (*ethpb.Eth1Data, erro } // set random roots and block hashes to prevent a majority from being // built if the eth1 node is offline - randGen := rand.NewRandomGenerator() + randGen := rand.NewGenerator() depRoot := hashutil.Hash(bytesutil.Bytes32(randGen.Uint64())) blockHash := hashutil.Hash(bytesutil.Bytes32(randGen.Uint64())) return ðpb.Eth1Data{ diff --git a/beacon-chain/sync/initial-sync/blocks_fetcher.go b/beacon-chain/sync/initial-sync/blocks_fetcher.go index 519b69f080f0..9130b2d33302 100644 --- a/beacon-chain/sync/initial-sync/blocks_fetcher.go +++ b/beacon-chain/sync/initial-sync/blocks_fetcher.go @@ -107,7 +107,7 @@ func newBlocksFetcher(ctx context.Context, cfg *blocksFetcherConfig) *blocksFetc return &blocksFetcher{ ctx: ctx, cancel: cancel, - rand: rand.NewRandomGenerator(), + rand: rand.NewGenerator(), headFetcher: cfg.headFetcher, p2p: cfg.p2p, blocksPerSecond: uint64(blocksPerSecond), diff --git a/beacon-chain/sync/pending_attestations_queue.go b/beacon-chain/sync/pending_attestations_queue.go index 4c40ab9a672d..1a6808a79015 100644 --- a/beacon-chain/sync/pending_attestations_queue.go +++ b/beacon-chain/sync/pending_attestations_queue.go @@ -60,7 +60,7 @@ func (s *Service) processPendingAtts(ctx context.Context) error { } s.pendingAttsLock.RUnlock() - randGen := rand.NewRandomGenerator() + randGen := rand.NewGenerator() for _, bRoot := range roots { s.pendingAttsLock.RLock() attestations := s.blkRootToPendingAtts[bRoot] diff --git a/beacon-chain/sync/pending_blocks_queue.go b/beacon-chain/sync/pending_blocks_queue.go index 42a0c6e7d1c8..b88e8f3e6f8b 100644 --- a/beacon-chain/sync/pending_blocks_queue.go +++ b/beacon-chain/sync/pending_blocks_queue.go @@ -51,7 +51,7 @@ func (s *Service) processPendingBlocks(ctx context.Context) error { trace.Int64Attribute("numPeers", int64(len(pids))), ) - randGen := rand.NewRandomGenerator() + randGen := rand.NewGenerator() for _, slot := range slots { ctx, span := trace.StartSpan(ctx, "processPendingBlocks.InnerLoop") span.AddAttributes(trace.Int64Attribute("slot", int64(slot))) diff --git a/shared/rand/rand.go b/shared/rand/rand.go index 4206422609d1..d46b54e0ce42 100644 --- a/shared/rand/rand.go +++ b/shared/rand/rand.go @@ -8,7 +8,7 @@ There are two modes, one for deterministic and another non-deterministic randomn 1. If deterministic pseudo-random generator is enough, use: import "github.com/prysmaticlabs/prysm/shared/rand" - randGen := rand.NewDeterministicRandomGenerator() + randGen := rand.NewDeterministicGenerator() randGen.Intn(32) // or any other func defined in math.rand API In this mode, only seed is generated using cryptographically secure source (crypto/rand). So, @@ -20,7 +20,7 @@ There are two modes, one for deterministic and another non-deterministic randomn 2. For cryptographically secure non-deterministic mode (CSPRNG), use: import "github.com/prysmaticlabs/prysm/shared/rand" - randGen := rand.NewRandomGenerator() + randGen := rand.NewGenerator() randGen.Intn(32) // or any other func defined in math.rand API Again, any of the functions from `math/rand` can be used, however, they all use custom source @@ -60,21 +60,21 @@ func (s *source) Uint64() (val uint64) { // Rand is alias for underlying random generator. type Rand = mrand.Rand -// NewRandomGenerator returns a new generator that uses random values from crypto/rand as a source +// NewGenerator returns a new generator that uses random values from crypto/rand as a source // (cryptographically secure random number generator). // Panics if crypto/rand input cannot be read. // Use it for everything where crypto secure non-deterministic randomness is required. Performance // takes a hit, so use sparingly. -func NewRandomGenerator() *Rand { +func NewGenerator() *Rand { return mrand.New(&source{}) } -// NewDeterministicRandomGenerator returns a random generator which is only seeded with crypto/rand, +// NewDeterministicGenerator returns a random generator which is only seeded with crypto/rand, // but is deterministic otherwise (given seed, produces given results, deterministically). // Panics if crypto/rand input cannot be read. // Use this method for performance, where deterministic pseudo-random behaviour is enough. -// Otherwise, rely on NewRandomGenerator(). -func NewDeterministicRandomGenerator() *Rand { - randGen := NewRandomGenerator() +// Otherwise, rely on NewGenerator(). +func NewDeterministicGenerator() *Rand { + randGen := NewGenerator() return mrand.New(mrand.NewSource(randGen.Int63())) } diff --git a/shared/testutil/block.go b/shared/testutil/block.go index 59ab45026c6e..583fea3b0b87 100644 --- a/shared/testutil/block.go +++ b/shared/testutil/block.go @@ -316,7 +316,7 @@ func generateAttesterSlashings( numSlashings uint64, ) ([]*ethpb.AttesterSlashing, error) { attesterSlashings := make([]*ethpb.AttesterSlashing, numSlashings) - randGen := rand.NewDeterministicRandomGenerator() + randGen := rand.NewDeterministicGenerator() for i := uint64(0); i < numSlashings; i++ { committeeIndex := randGen.Uint64() % params.BeaconConfig().MaxCommitteesPerSlot committee, err := helpers.BeaconCommitteeFromState(bState, bState.Slot(), committeeIndex) @@ -380,7 +380,7 @@ func GenerateAttestations(bState *stateTrie.BeaconState, privs []bls.SecretKey, } } if randomRoot { - randGen := rand.NewDeterministicRandomGenerator() + randGen := rand.NewDeterministicGenerator() b := make([]byte, 32) _, err := randGen.Read(b) if err != nil { @@ -529,5 +529,5 @@ func randValIndex(bState *stateTrie.BeaconState) (uint64, error) { if err != nil { return 0, err } - return rand.NewRandomGenerator().Uint64() % activeCount, nil + return rand.NewGenerator().Uint64() % activeCount, nil } diff --git a/shared/testutil/helpers.go b/shared/testutil/helpers.go index e4f387a47162..afc675ad68d1 100644 --- a/shared/testutil/helpers.go +++ b/shared/testutil/helpers.go @@ -76,7 +76,7 @@ func BlockSignature( // Random32Bytes generates a random 32 byte slice. func Random32Bytes(t *testing.T) []byte { b := make([]byte, 32) - _, err := rand.NewDeterministicRandomGenerator().Read(b) + _, err := rand.NewDeterministicGenerator().Read(b) if err != nil { t.Fatal(err) } diff --git a/validator/db/setup_db.go b/validator/db/setup_db.go index 4301dc02df7f..4548e3b277c8 100644 --- a/validator/db/setup_db.go +++ b/validator/db/setup_db.go @@ -11,7 +11,7 @@ import ( // SetupDB instantiates and returns a DB instance for the validator client. func SetupDB(t testing.TB, pubkeys [][48]byte) *Store { - randPath := rand.NewDeterministicRandomGenerator().Int() + randPath := rand.NewDeterministicGenerator().Int() p := filepath.Join(TempDir(), fmt.Sprintf("/%d", randPath)) if err := os.RemoveAll(p); err != nil { t.Fatalf("Failed to remove directory: %v", err) From c522fa5a93f4efe39d9f9c5c87d2e961eb51946d Mon Sep 17 00:00:00 2001 From: Victor Farazdagi Date: Fri, 26 Jun 2020 12:32:31 +0300 Subject: [PATCH 16/17] adds simple no-panic test --- shared/rand/rand.go | 6 +++--- shared/rand/rand_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 3 deletions(-) create mode 100644 shared/rand/rand_test.go diff --git a/shared/rand/rand.go b/shared/rand/rand.go index d46b54e0ce42..4a2bca5ffcb5 100644 --- a/shared/rand/rand.go +++ b/shared/rand/rand.go @@ -12,10 +12,10 @@ There are two modes, one for deterministic and another non-deterministic randomn randGen.Intn(32) // or any other func defined in math.rand API In this mode, only seed is generated using cryptographically secure source (crypto/rand). So, - once seed is obtained, and generator is seeded, all the rest usage is deterministic, and fast. + once seed is obtained, and generator is seeded, the next generations are deterministic, thus fast. This method is still better than using unix time for source of randomness - since time is not a - good source of seed randomness, when you have many concurrent servers using it (and can coincide - in start times). + good source of seed randomness, when you have many concurrent servers using it (and they have + coinciding random generators' start times). 2. For cryptographically secure non-deterministic mode (CSPRNG), use: diff --git a/shared/rand/rand_test.go b/shared/rand/rand_test.go new file mode 100644 index 000000000000..b027b1155534 --- /dev/null +++ b/shared/rand/rand_test.go @@ -0,0 +1,24 @@ +package rand + +import ( + "math/rand" + "testing" +) + +func TestNewGenerator(t *testing.T) { + // Make sure that generation works, no panics. + randGen := NewGenerator() + _ = randGen.Int63() + _ = randGen.Uint64() + _ = randGen.Intn(32) + var _ = rand.Source64(randGen) +} + +func TestNewDeterministicGenerator(t *testing.T) { + // Make sure that generation works, no panics. + randGen := NewDeterministicGenerator() + _ = randGen.Int63() + _ = randGen.Uint64() + _ = randGen.Intn(32) + var _ = rand.Source64(randGen) +} From e1e9b76b495f2a99ac1a8acdbfcd5783ca1cfea9 Mon Sep 17 00:00:00 2001 From: Victor Farazdagi Date: Fri, 26 Jun 2020 12:54:40 +0300 Subject: [PATCH 17/17] gazelle --- shared/rand/BUILD.bazel | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/shared/rand/BUILD.bazel b/shared/rand/BUILD.bazel index 09cfcbfffd45..5c8281403904 100644 --- a/shared/rand/BUILD.bazel +++ b/shared/rand/BUILD.bazel @@ -1,3 +1,4 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_test") load("@prysm//tools/go:def.bzl", "go_library") go_library( @@ -6,3 +7,9 @@ go_library( importpath = "github.com/prysmaticlabs/prysm/shared/rand", visibility = ["//visibility:public"], ) + +go_test( + name = "go_default_test", + srcs = ["rand_test.go"], + embed = [":go_default_library"], +)