Audit uses of SmallRng (#757)

While testing another program, I was reminded that `SmallRng` can have
surprising behavior when cloned: since a SmallRng's internal state is
stored in a field, each clone ends up in the same state such that each
clone produces the same sequence of values.

Furthermore, the use of `SmallRng::from_entropy` uses `getrandom`
internally, which can be quite slow, especially compared to the
alternative `SmallRng::from_rng(&mut thread_rng())`.

It doesn't appear that any of our uses of `SmallRng` are particularly
problematic -- i.e. the services that hold a `SmallRng` are not cloned,
so we shouldn't be in a place where we always use a single value.
However, in the spirit of being defensive to this scenario, this change
avoid storing a `SmallRng` in Layer and NewService implemetnations so
that each produced service obtains a unique `SmallRng`. Furthermore, all
uses of `SmallRng::from_entropy()` are converted to use the faster
thread-local PRNG equivalent.

Author: olix0r

Cargo.lock

@@ -838,7 +838,6 @@ dependencies = [
  "pin-project",
  "procinfo",
  "prost-types",
- "rand 0.7.2",
  "regex 1.3.9",
  "tokio",
  "tokio-timer",

linkerd/app/core/Cargo.toml

@@ -59,7 +59,6 @@ linkerd2-stack = { path = "../../stack" }
 linkerd2-stack-metrics = { path = "../../stack/metrics" }
 linkerd2-stack-tracing = { path = "../../stack/tracing" }
 linkerd2-trace-context = { path = "../../trace-context" }
-rand = { version = "0.7", features = ["small_rng"] }
 regex = "1.0.0"
 tokio = { version = "0.2.22", features = ["macros", "sync", "parking_lot"]}
 tokio-timer = "0.2"

linkerd/exp-backoff/src/lib.rs

@@ -1,7 +1,7 @@
 #![deny(warnings, rust_2018_idioms)]
 
 use pin_project::pin_project;
-use rand::{rngs::SmallRng, SeedableRng};
+use rand::{rngs::SmallRng, thread_rng, SeedableRng};
 use std::fmt;
 use std::future::Future;
 use std::ops::Mul;
@@ -44,7 +44,7 @@ impl ExponentialBackoff {
     pub fn stream(&self) -> ExponentialBackoffStream {
         ExponentialBackoffStream {
             backoff: *self,
-            rng: SmallRng::from_entropy(),
+            rng: SmallRng::from_rng(&mut thread_rng()).expect("RNG must be valid"),
             iterations: 0,
             delay: None,
         }

linkerd/proxy/http/src/balance.rs

@@ -2,7 +2,7 @@ use crate::Error;
 use http;
 use hyper::body::HttpBody;
 pub use hyper_balance::{PendingUntilFirstData, PendingUntilFirstDataBody};
-use rand::{rngs::SmallRng, SeedableRng};
+use rand::thread_rng;
 use std::{hash::Hash, marker::PhantomData, time::Duration};
 use tower::discover::Discover;
 pub use tower::{
@@ -16,7 +16,6 @@ pub use tower::{
 pub struct Layer<A, B> {
     decay: Duration,
     default_rtt: Duration,
-    rng: SmallRng,
     _marker: PhantomData<fn(A) -> B>,
 }
 
@@ -26,7 +25,6 @@ pub fn layer<A, B>(default_rtt: Duration, decay: Duration) -> Layer<A, B> {
     Layer {
         decay,
         default_rtt,
-        rng: SmallRng::from_entropy(),
         _marker: PhantomData,
     }
 }
@@ -36,7 +34,6 @@ impl<A, B> Clone for Layer<A, B> {
         Self {
             decay: self.decay,
             default_rtt: self.default_rtt,
-            rng: self.rng.clone(),
             _marker: PhantomData,
         }
     }
@@ -58,6 +55,6 @@ where
     fn layer(&self, discover: D) -> Self::Service {
         let instrument = PendingUntilFirstData::default();
         let loaded = PeakEwmaDiscover::new(discover, self.default_rtt, self.decay, instrument);
-        Balance::from_rng(loaded, self.rng.clone()).expect("RNG must be valid")
+        Balance::from_rng(loaded, &mut thread_rng()).expect("RNG must be valid")
     }
 }

linkerd/proxy/tap/Cargo.toml

@@ -19,7 +19,7 @@ linkerd2-proxy-api = { git = "https://github.com/linkerd/linkerd2-proxy-api", ta
 linkerd2-proxy-http = { path = "../http" }
 linkerd2-proxy-transport = { path = "../transport" }
 linkerd2-stack = { path = "../../stack" }
-rand = { version = "0.7", features = ["small_rng"] }
+rand = { version = "0.7" }
 tokio = { version = "0.2", features = ["time"]}
 tower = {version = "0.3", default-features = false }
 tonic = { version = "0.3", default-features = false }

linkerd/proxy/tcp/Cargo.toml

@@ -11,7 +11,7 @@ futures = { version = "0.3", features = ["compat"] }
 linkerd2-duplex = { path = "../../duplex" }
 linkerd2-error = { path = "../../error" }
 linkerd2-stack = { path = "../../stack" }
-rand = { version = "0.7", features = ["small_rng"] }
+rand = { version = "0.7" }
 tokio = { version = "0.2" }
 tower = { version = "0.3", default-features = false, features = ["balance", "load", "discover"] }
 pin-project = "0.4"

linkerd/proxy/tcp/src/balance.rs

@@ -1,6 +1,6 @@
 use linkerd2_error::Error;
 use linkerd2_stack::layer;
-use rand::{rngs::SmallRng, SeedableRng};
+use rand::thread_rng;
 use std::{hash::Hash, time::Duration};
 pub use tower::{
     balance::p2c::Balance,
@@ -20,10 +20,9 @@ where
     D::Service: tower::Service<T>,
     <D::Service as tower::Service<T>>::Error: Into<Error>,
 {
-    let rng = SmallRng::from_entropy();
     layer::mk(move |discover| {
         let loaded =
             PeakEwmaDiscover::new(discover, default_rtt, decay, CompleteOnResponse::default());
-        Balance::from_rng(loaded, rng.clone()).expect("RNG must be valid")
+        Balance::from_rng(loaded, &mut thread_rng()).expect("RNG must be valid")
     })
 }

linkerd/service-profiles/src/split.rs

@@ -5,7 +5,7 @@ use linkerd2_addr::Addr;
 use linkerd2_error::Error;
 use linkerd2_stack::{layer, NewService};
 use rand::distributions::{Distribution, WeightedIndex};
-use rand::{rngs::SmallRng, SeedableRng};
+use rand::{rngs::SmallRng, thread_rng, SeedableRng};
 use std::{
     marker::PhantomData,
     pin::Pin,
@@ -17,18 +17,15 @@ use tracing::{debug, trace};
 pub fn layer<N, S, Req>() -> impl layer::Layer<N, Service = NewSplit<N, S, Req>> + Clone {
     // This RNG doesn't need to be cryptographically secure. Small and fast is
     // preferable.
-    let rng = SmallRng::from_entropy();
     layer::mk(move |inner| NewSplit {
         inner,
-        rng: rng.clone(),
         _service: PhantomData,
     })
 }
 
 #[derive(Debug)]
 pub struct NewSplit<N, S, Req> {
     inner: N,
-    rng: SmallRng,
     _service: PhantomData<fn(Req) -> S>,
 }
 
@@ -57,7 +54,6 @@ impl<N: Clone, S, Req> Clone for NewSplit<N, S, Req> {
     fn clone(&self) -> Self {
         Self {
             inner: self.inner.clone(),
-            rng: self.rng.clone(),
             _service: self._service,
         }
     }
@@ -115,7 +111,7 @@ where
                     services,
                     addrs,
                     distribution: WeightedIndex::new(weights).unwrap(),
-                    rng: self.rng.clone(),
+                    rng: SmallRng::from_rng(&mut thread_rng()).expect("RNG must initialize"),
                 }
             }
         };

linkerd/trace-context/Cargo.toml

@@ -13,7 +13,7 @@ hex = "0.3.2"
 http = "0.2"
 linkerd2-error = { path = "../error" }
 linkerd2-stack = { path = "../stack" }
-rand = { version = "0.7", features = ["small_rng"] }
+rand = { version = "0.7" }
 tower = { version = "0.3", default-features = false, features = ["util"] }
 tracing = "0.1.2"
 tokio = {version = "0.2", features = ["sync"]}

linkerd/trace-context/src/propagation.rs

@@ -2,7 +2,7 @@ use crate::{Flags, Id, InsufficientBytes};
 use bytes::Bytes;
 use http::header::HeaderValue;
 use linkerd2_error::Error;
-use rand::{rngs::SmallRng, SeedableRng};
+use rand::thread_rng;
 use std::convert::TryInto;
 use std::fmt;
 use tracing::{trace, warn};
@@ -148,7 +148,7 @@ fn parse_grpc_trace_context_field(
 }
 
 fn increment_grpc_span_id<B>(request: &mut http::Request<B>, context: &TraceContext) -> Id {
-    let span_id = Id::new_span_id(&mut SmallRng::from_entropy());
+    let span_id = Id::new_span_id(&mut thread_rng());
 
     trace!(message = "incremented span id", %span_id);
 
@@ -195,7 +195,7 @@ fn unpack_http_trace_context<B>(request: &http::Request<B>) -> Option<TraceConte
 }
 
 fn increment_http_span_id<B>(request: &mut http::Request<B>) -> Id {
-    let span_id = Id::new_span_id(&mut SmallRng::from_entropy());
+    let span_id = Id::new_span_id(&mut thread_rng());
 
     trace!("incremented span id: {}", span_id);