Merge pull request #1757 from authzed/replace-math-rand

re-enable gosec/G404

.golangci.yaml

@@ -10,9 +10,10 @@ linters-settings:
     packages:
       - 'github.com/jmoiron/sqlx'
       - 'github.com/jackc/pgx'
-  gosec:
-    excludes:
-      - 'G404'  # Allow the usage of math/rand
+  revive:
+    rules:
+      - name: 'unused-parameter'
+        disabled: true
 linters:
   enable:
     - 'bidichk'

e2e/go.mod

@@ -7,7 +7,7 @@ toolchain go1.21.1
 require (
 	github.com/authzed/authzed-go v0.10.1
 	github.com/authzed/grpcutil v0.0.0-20240123092924-129dc0a6a6e1
-	github.com/authzed/spicedb v1.25.0
+	github.com/authzed/spicedb v1.29.1
 	github.com/brianvoe/gofakeit/v6 v6.23.2
 	github.com/ecordell/optgen v0.0.10-0.20230609182709-018141bf9698
 	github.com/jackc/pgx/v5 v5.5.2

go.mod

@@ -61,6 +61,7 @@ require (
 	github.com/prometheus/client_model v0.5.0
 	github.com/prometheus/common v0.45.0
 	github.com/rs/cors v1.10.1
+	github.com/rs/xid v1.5.0
 	github.com/rs/zerolog v1.31.0
 	github.com/samber/lo v1.39.0
 	github.com/schollz/progressbar/v3 v3.14.1

go.sum

@@ -745,6 +745,7 @@ github.com/rogpeppe/go-internal v1.11.0 h1:cWPaGQEPrBb5/AsnsZesgZZ9yb1OQ+GOISoDN
 github.com/rogpeppe/go-internal v1.11.0/go.mod h1:ddIwULY96R17DhadqLgMfk9H9tvdUzkipdSkR5nkCZA=
 github.com/rs/cors v1.10.1 h1:L0uuZVXIKlI1SShY2nhFfo44TYvDPQ1w4oFkUJNfhyo=
 github.com/rs/cors v1.10.1/go.mod h1:XyqrcTp5zjWr1wsJ8PIRZssZ8b/WMcMf71DJnit4EMU=
+github.com/rs/xid v1.5.0 h1:mKX4bl4iPYJtEIxp6CYiUuLQ/8DYMoz0PUdtGgMFRVc=
 github.com/rs/xid v1.5.0/go.mod h1:trrq9SKmegXys3aeAKXMUTdJsYXVwGY3RLcfgqegfbg=
 github.com/rs/zerolog v1.31.0 h1:FcTR3NnLWW+NnTwwhFWiJSZr4ECLpqCm6QsEnyvbV4A=
 github.com/rs/zerolog v1.31.0/go.mod h1:/7mN4D5sKwJLZQ2b/znpjC3/GQWY/xaDXUM0kKWRHss=

internal/datastore/crdb/pool/balancer.go

@@ -90,7 +90,11 @@ func newNodeConnectionBalancer[P balancePoolConn[C], C balanceConn](pool balance
 		healthTracker: healthTracker,
 		pool:          pool,
 		seed:          seed,
-		rnd:           rand.New(rand.NewSource(seed)),
+		// nolint:gosec
+		// use of non cryptographically secure random number generator is not concern here,
+		// as it's used for shuffling the nodes to balance the connections when the number of
+		// connections do not divide evenly.
+		rnd: rand.New(rand.NewSource(seed)),
 	}
 }
 

internal/datastore/crdb/pool/balancer_test.go

@@ -150,6 +150,9 @@ func TestNodeConnectionBalancerPrune(t *testing.T) {
 
 			p := newNodeConnectionBalancer[*FakePoolConn[*FakeConn], *FakeConn](pool, tracker, 1*time.Minute)
 			p.seed = 0
+			// nolint:gosec
+			// G404 use of non cryptographically secure random number generator is not concern here,
+			// as it's used for jittering the interval for health checks.
 			p.rnd = rand.New(rand.NewSource(0))
 
 			for _, n := range tt.conns {

internal/datastore/crdb/pool/health.go

@@ -59,6 +59,9 @@ func NewNodeHealthChecker(url string) (*NodeHealthTracker, error) {
 // Poll starts polling the cluster and recording the node IDs that it sees.
 func (t *NodeHealthTracker) Poll(ctx context.Context, interval time.Duration) {
 	ticker := jitterbug.New(interval, jitterbug.Uniform{
+		// nolint:gosec
+		// G404 use of non cryptographically secure random number generator is not concern here,
+		// as it's used for jittering the interval for health checks.
 		Source: rand.New(rand.NewSource(time.Now().Unix())),
 		Min:    interval,
 	})

internal/datastore/crdb/reader.go

@@ -241,7 +241,6 @@ func loadAllNamespaces(ctx context.Context, tx pgxcommon.DBFuncQuerier, fromBuil
 		}
 		return nil
 	}, sql, args...)
-
 	if err != nil {
 		return nil, err
 	}

internal/datastore/crdb/stats.go

@@ -90,6 +90,9 @@ func (cds *crdbDatastore) Statistics(ctx context.Context) (datastore.Stats, erro
 
 func updateCounter(ctx context.Context, tx pgx.Tx, change int64) (datastore.Revision, error) {
 	counterID := make([]byte, 2)
+	// nolint:gosec
+	// G404 use of non cryptographically secure random number generator is not concern here,
+	// as this is only used to randomly distributed the counters across multiple rows and reduce write row contention
 	_, err := rand.New(rng).Read(counterID)
 	if err != nil {
 		return datastore.NoRevision, fmt.Errorf("unable to select random counter: %w", err)

internal/datastore/postgres/postgres.go

@@ -358,7 +358,6 @@ func (pgd *pgDatastore) ReadWriteTx(
 
 			return fn(ctx, rwt)
 		}))
-
 		if err != nil {
 			if !config.DisableRetries && errorRetryable(err) {
 				pgxcommon.SleepOnErr(ctx, err, i)

internal/datastore/revisions/optimized.go

@@ -44,6 +44,10 @@ func (cor *CachedOptimizedRevisions) OptimizedRevision(ctx context.Context) (dat
 	// Subtract a random amount of time from now, to let barely expired candidates get selected
 	adjustedNow := localNow
 	if cor.maxRevisionStaleness > 0 {
+		// nolint:gosec
+		// G404 use of non cryptographically secure random number generator is not a security concern here,
+		// as we are using it to introduce randomness to the accepted staleness of a revision and reduce the odds of
+		// a thundering herd to the datastore
 		adjustedNow = localNow.Add(-1 * time.Duration(rand.Int63n(cor.maxRevisionStaleness.Nanoseconds())) * time.Nanosecond)
 	}
 

internal/datastore/spanner/stats.go

@@ -64,6 +64,9 @@ func updateCounter(ctx context.Context, rwt *spanner.ReadWriteTransaction, chang
 	newValue := change
 
 	counterID := make([]byte, 2)
+	// nolint:gosec
+	// G404 use of non cryptographically secure random number generator is not concern here,
+	// as this is only used to randomly distributed the counters across multiple rows and reduce write contention
 	_, err := rand.New(rng).Read(counterID)
 	if err != nil {
 		return fmt.Errorf("unable to select random counter: %w", err)

internal/datastore/spanner/watch.go

@@ -321,7 +321,6 @@ func (sd spannerDatastore) watch(
 		}
 		return nil
 	})
-
 	if err != nil {
 		sendError(err)
 		return

internal/services/v1/experimental_test.go

@@ -113,6 +113,9 @@ func constBatch(size int) func() int {
 
 func randomBatch(min, max int) func() int {
 	return func() int {
+		// nolint:gosec
+		// G404 use of non cryptographically secure random number generator is not a security concern here,
+		// as this is only used for generating fixtures in testing.
 		return rand.Intn(max-min) + min
 	}
 }

internal/services/v1/permissions.go

@@ -448,7 +448,6 @@ func (ps *permissionServer) LookupResources(req *v1.LookupResourcesRequest, resp
 			OptionalLimit:  req.OptionalLimit,
 		},
 		stream)
-
 	if err != nil {
 		return ps.rewriteError(ctx, err)
 	}

internal/telemetry/reporter.go

@@ -164,7 +164,9 @@ func RemoteReporter(
 	}
 
 	return func(ctx context.Context) error {
-		// Smear the startup delay out over 10% of the reporting interval
+		// nolint:gosec
+		// G404 use of non cryptographically secure random number generator is not a security concern here,
+		// as this is only used to smear the startup delay out over 10% of the reporting interval
 		startupDelay := time.Duration(rand.Int63n(int64(interval.Seconds()/10))) * time.Second
 
 		log.Ctx(ctx).Info().

internal/testfixtures/generator.go

@@ -22,6 +22,9 @@ func RandomObjectID(length uint8) string {
 		if i == 0 {
 			sourceLetters = FirstLetters
 		}
+		// nolint:gosec
+		// G404 use of non cryptographically secure random number generator is not a security concern here,
+		// as this is only used for generating fixtures in testing.
 		b[i] = sourceLetters[rand.Intn(len(sourceLetters))]
 	}
 	return string(b)

pkg/middleware/requestid/requestid.go

@@ -2,18 +2,18 @@ package requestid
 
 import (
 	"context"
-	"math/rand"
 
 	log "github.com/authzed/spicedb/internal/logging"
 
 	"github.com/authzed/authzed-go/pkg/responsemeta"
 	"github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors"
+	"github.com/rs/xid"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/metadata"
 )
 
-// RequestIDMetadataKey is the key in which request IDs are passed to metadata.
-const RequestIDMetadataKey = "x-request-id"
+// MetadataKey is the key in which request IDs are passed to metadata.
+const MetadataKey = "x-request-id"
 
 // Option instances control how the middleware is initialized.
 type Option func(*handleRequestID)
@@ -31,41 +31,9 @@ func GenerateIfMissing(enable bool) Option {
 // IDGenerator functions are used to generate request IDs if a new one is needed.
 type IDGenerator func() string
 
-// WithIDGenerator gives the middleware a function to use for generating requestIDs.
-//
-// default: 32 character hex string
-func WithIDGenerator(genFunc IDGenerator) Option {
-	return func(reporter *handleRequestID) {
-		reporter.requestIDGenerator = genFunc
-	}
-}
-
 // GenerateRequestID generates a new request ID.
 func GenerateRequestID() string {
-	return randSeq(32)
-}
-
-// GetOrGenerateRequestID returns the request ID found in the given context. If not, a new request ID
-// is generated and added to the returned context.
-func GetOrGenerateRequestID(ctx context.Context) (string, context.Context) {
-	var haveRequestID bool
-	md, ok := metadata.FromIncomingContext(ctx)
-	if ok {
-		var requestIDs []string
-		requestIDs, haveRequestID = md[RequestIDMetadataKey]
-		if haveRequestID {
-			return requestIDs[0], ctx
-		}
-	}
-
-	if md == nil {
-		md = metadata.New(nil)
-	}
-
-	requestID := GenerateRequestID()
-	md.Set(RequestIDMetadataKey, requestID)
-	ctx = metadata.NewIncomingContext(ctx, md)
-	return requestID, ctx
+	return xid.New().String()
 }
 
 type handleRequestID struct {
@@ -79,7 +47,7 @@ func (r *handleRequestID) ServerReporter(ctx context.Context, _ interceptors.Cal
 	md, ok := metadata.FromIncomingContext(ctx)
 	if ok {
 		var requestIDs []string
-		requestIDs, haveRequestID = md[RequestIDMetadataKey]
+		requestIDs, haveRequestID = md[MetadataKey]
 		if haveRequestID {
 			requestID = requestIDs[0]
 		}
@@ -93,12 +61,12 @@ func (r *handleRequestID) ServerReporter(ctx context.Context, _ interceptors.Cal
 			md = metadata.New(nil)
 		}
 
-		md.Set(RequestIDMetadataKey, requestID)
+		md.Set(MetadataKey, requestID)
 		ctx = metadata.NewIncomingContext(ctx, md)
 	}
 
 	if haveRequestID {
-		ctx = metadata.AppendToOutgoingContext(ctx, RequestIDMetadataKey, requestID)
+		ctx = metadata.AppendToOutgoingContext(ctx, MetadataKey, requestID)
 		err := responsemeta.SetResponseHeaderMetadata(ctx, map[responsemeta.ResponseMetadataHeaderKey]string{
 			responsemeta.RequestID: requestID,
 		})
@@ -138,13 +106,3 @@ func createReporter(opts []Option) *handleRequestID {
 
 	return reporter
 }
-
-var letters = []rune("0123456789abcdef")
-
-func randSeq(n int) string {
-	b := make([]rune, n)
-	for i := range b {
-		b[i] = letters[rand.Intn(len(letters))]
-	}
-	return string(b)
-}