diff --git a/.golangci.yaml b/.golangci.yaml index a3a54f7896..1aba39cf8f 100644 --- a/.golangci.yaml +++ b/.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' diff --git a/e2e/go.mod b/e2e/go.mod index 30e0ea97b1..74cbf4f329 100644 --- a/e2e/go.mod +++ b/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 diff --git a/go.mod b/go.mod index 048e14fb53..c53089865a 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 9d8d1b589b..67f3395449 100644 --- a/go.sum +++ b/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= diff --git a/internal/datastore/common/jitter.go b/internal/datastore/common/jitter.go deleted file mode 100644 index 3b9c54aa3f..0000000000 --- a/internal/datastore/common/jitter.go +++ /dev/null @@ -1,17 +0,0 @@ -package common - -import ( - "math" - "math/rand" - "time" -) - -// WithJitter adjusts an interval by a random amount within some factor of the -// original interval. Intervals near half of max int64 can return durations -// outside the jitter factor. -func WithJitter(factor float64, interval time.Duration) time.Duration { - if factor < 0.0 || factor > 1.0 { - return interval - } - return time.Duration(math.Abs((1 - factor + 2*rand.Float64()*factor) * float64(interval))) -} diff --git a/internal/datastore/crdb/pool/balancer.go b/internal/datastore/crdb/pool/balancer.go index f4f388214b..e88ce5c5de 100644 --- a/internal/datastore/crdb/pool/balancer.go +++ b/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)), } } diff --git a/internal/datastore/crdb/pool/balancer_test.go b/internal/datastore/crdb/pool/balancer_test.go index 413e1678df..5fefdb9ec6 100644 --- a/internal/datastore/crdb/pool/balancer_test.go +++ b/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 { diff --git a/internal/datastore/crdb/pool/health.go b/internal/datastore/crdb/pool/health.go index eedc90fa08..690bd01984 100644 --- a/internal/datastore/crdb/pool/health.go +++ b/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, }) diff --git a/internal/datastore/crdb/reader.go b/internal/datastore/crdb/reader.go index ef5cef2872..af7edb0554 100644 --- a/internal/datastore/crdb/reader.go +++ b/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 } diff --git a/internal/datastore/crdb/stats.go b/internal/datastore/crdb/stats.go index abebe1b1f5..954a64050b 100644 --- a/internal/datastore/crdb/stats.go +++ b/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) diff --git a/internal/datastore/postgres/postgres.go b/internal/datastore/postgres/postgres.go index 167b77d175..8313b90edb 100644 --- a/internal/datastore/postgres/postgres.go +++ b/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) diff --git a/internal/datastore/revisions/optimized.go b/internal/datastore/revisions/optimized.go index f73275bd3b..3a7b958d2f 100644 --- a/internal/datastore/revisions/optimized.go +++ b/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) } diff --git a/internal/datastore/spanner/stats.go b/internal/datastore/spanner/stats.go index 07093f03ce..0df2d25b79 100644 --- a/internal/datastore/spanner/stats.go +++ b/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) diff --git a/internal/datastore/spanner/watch.go b/internal/datastore/spanner/watch.go index 5988db5970..8a2c437da9 100644 --- a/internal/datastore/spanner/watch.go +++ b/internal/datastore/spanner/watch.go @@ -321,7 +321,6 @@ func (sd spannerDatastore) watch( } return nil }) - if err != nil { sendError(err) return diff --git a/internal/services/v1/experimental_test.go b/internal/services/v1/experimental_test.go index 5327f4dac9..7cdd7d76d4 100644 --- a/internal/services/v1/experimental_test.go +++ b/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 } } diff --git a/internal/services/v1/permissions.go b/internal/services/v1/permissions.go index 1f17f710f2..efb5250896 100644 --- a/internal/services/v1/permissions.go +++ b/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) } diff --git a/internal/telemetry/reporter.go b/internal/telemetry/reporter.go index db65c5d028..ae94a59045 100644 --- a/internal/telemetry/reporter.go +++ b/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(). diff --git a/internal/testfixtures/generator.go b/internal/testfixtures/generator.go index c48c45c0c8..a1382b762d 100644 --- a/internal/testfixtures/generator.go +++ b/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) diff --git a/pkg/middleware/requestid/requestid.go b/pkg/middleware/requestid/requestid.go index 7ca2f85633..82c5724522 100644 --- a/pkg/middleware/requestid/requestid.go +++ 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) -}