-
Notifications
You must be signed in to change notification settings - Fork 268
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix security errors in lint steps #2061
Changes from 10 commits
99ae458
0c6bad2
6b53f31
9d6db3b
8da4081
efd1869
74043c7
9bf5f3d
5731e26
31d4af3
c8150ef
54cba16
b1fc26d
9b9b291
62afc3c
25422a8
5fd005a
7fbcc18
d8e30c6
015bd0e
0ac9768
8ad8c4e
a358e68
4b1ab86
c781a10
d4d3071
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
module github.com/authzed/spicedb/e2e | ||
|
||
go 1.22.4 | ||
go 1.22.7 | ||
|
||
require ( | ||
github.com/authzed/authzed-go v0.15.0 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,8 @@ import ( | |
|
||
"golang.org/x/exp/maps" | ||
|
||
"github.com/ccoveille/go-safecast" | ||
|
||
log "github.com/authzed/spicedb/internal/logging" | ||
"github.com/authzed/spicedb/pkg/datastore" | ||
core "github.com/authzed/spicedb/pkg/proto/core/v1" | ||
|
@@ -118,7 +120,11 @@ func (ch *Changes[R, K]) adjustByteSize(item sized, delta int) error { | |
return spiceerrors.MustBugf("byte size underflow") | ||
} | ||
|
||
if ch.currentByteSize > int64(ch.maxByteSize) { | ||
// We checked for underflow above, so the current byte size | ||
// should fit in a uint64 | ||
currentByteSize, _ := safecast.ToUint64(ch.currentByteSize) | ||
|
||
if currentByteSize > ch.maxByteSize { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I was going through these, I tended to go one of four ways:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should return the error as a MustBugf even if we know it can be safe; it is "extra" code but it means if anything changes we're guaranteed to catch the error vs having a silent bug appear |
||
return datastore.NewMaximumChangesSizeExceededError(ch.maxByteSize) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,7 +68,7 @@ func (gc *fakeGC) DeleteBeforeTx(_ context.Context, rev datastore.Revision) (Del | |
|
||
revInt := rev.(revisions.TransactionIDRevision).TransactionID() | ||
|
||
return gc.deleter.DeleteBeforeTx(int64(revInt)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seemed like a type mismatch, so I propagated it through. I'd welcome a check of that assumption. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, seems so |
||
return gc.deleter.DeleteBeforeTx(revInt) | ||
} | ||
|
||
func (gc *fakeGC) HasGCRun() bool { | ||
|
@@ -101,22 +101,22 @@ func (gc *fakeGC) GetMetrics() gcMetrics { | |
|
||
// Allows specifying different deletion behaviors for tests | ||
type gcDeleter interface { | ||
DeleteBeforeTx(revision int64) (DeletionCounts, error) | ||
DeleteBeforeTx(revision uint64) (DeletionCounts, error) | ||
} | ||
|
||
// Always error trying to perform a delete | ||
type alwaysErrorDeleter struct{} | ||
|
||
func (alwaysErrorDeleter) DeleteBeforeTx(_ int64) (DeletionCounts, error) { | ||
func (alwaysErrorDeleter) DeleteBeforeTx(_ uint64) (DeletionCounts, error) { | ||
return DeletionCounts{}, fmt.Errorf("delete error") | ||
} | ||
|
||
// Only error on specific revisions | ||
type revisionErrorDeleter struct { | ||
errorOnRevisions []int64 | ||
errorOnRevisions []uint64 | ||
} | ||
|
||
func (d revisionErrorDeleter) DeleteBeforeTx(revision int64) (DeletionCounts, error) { | ||
func (d revisionErrorDeleter) DeleteBeforeTx(revision uint64) (DeletionCounts, error) { | ||
if slices.Contains(d.errorOnRevisions, revision) { | ||
return DeletionCounts{}, fmt.Errorf("delete error") | ||
} | ||
|
@@ -178,7 +178,7 @@ func TestGCFailureBackoffReset(t *testing.T) { | |
// Error on revisions 1 - 5, giving the exponential | ||
// backoff enough time to fail the test if the interval | ||
// is not reset properly. | ||
errorOnRevisions: []int64{1, 2, 3, 4, 5}, | ||
errorOnRevisions: []uint64{1, 2, 3, 4, 5}, | ||
}) | ||
|
||
ctx, cancel := context.WithCancel(context.Background()) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,11 +3,13 @@ package pool | |
import ( | ||
"context" | ||
"hash/maphash" | ||
"math" | ||
"math/rand" | ||
"slices" | ||
"strconv" | ||
"time" | ||
|
||
"github.com/ccoveille/go-safecast" | ||
"github.com/jackc/pgx/v5" | ||
"github.com/jackc/pgx/v5/pgxpool" | ||
"github.com/prometheus/client_golang/prometheus" | ||
|
@@ -84,7 +86,15 @@ type nodeConnectionBalancer[P balancePoolConn[C], C balanceConn] struct { | |
// newNodeConnectionBalancer is generic over underlying connection types for | ||
// testing purposes. Callers should use the exported NewNodeConnectionBalancer. | ||
func newNodeConnectionBalancer[P balancePoolConn[C], C balanceConn](pool balanceablePool[P, C], healthTracker *NodeHealthTracker, interval time.Duration) *nodeConnectionBalancer[P, C] { | ||
seed := int64(new(maphash.Hash).Sum64()) | ||
seed := int64(0) | ||
for seed == 0 { | ||
// Sum64 returns a uint64, and safecast will return 0 if it's not castable, | ||
// which will happen about half the time (?). We just keep running it until | ||
// we get a seed that fits in the box. | ||
// Subtracting math.MaxInt64 should mean that we retain the entire range of | ||
// possible values. | ||
seed, _ = safecast.ToInt64(new(maphash.Hash).Sum64() - math.MaxInt64) | ||
} | ||
Comment on lines
+90
to
+97
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This felt a little wonky but seemed to capture the desired effect. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should just make the seed a uint64 then? @ecordell? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
interpreting the uint64 bits as int64 bits doesn't really matter for this application. the requirements for this use of rand are super minimal, we really just want "don't do the same exact thing on every replica 100% of the time". can we just divide it by 2 and then cast? or is there a way to mark this specific "unsafe" cast as "I don't care about wraparound at all"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we can |
||
return &nodeConnectionBalancer[P, C]{ | ||
ticker: time.NewTicker(interval), | ||
sem: semaphore.NewWeighted(1), | ||
|
@@ -147,7 +157,9 @@ func (p *nodeConnectionBalancer[P, C]) mustPruneConnections(ctx context.Context) | |
} | ||
} | ||
|
||
nodeCount := uint32(p.healthTracker.HealthyNodeCount()) | ||
// It's highly unlikely that we'll ever have an overflow in | ||
// this context, so we cast directly. | ||
nodeCount, _ := safecast.ToUint32(p.healthTracker.HealthyNodeCount()) | ||
if nodeCount == 0 { | ||
nodeCount = 1 | ||
} | ||
|
@@ -203,7 +215,7 @@ func (p *nodeConnectionBalancer[P, C]) mustPruneConnections(ctx context.Context) | |
// it's possible for the difference in connections between nodes to differ by up to | ||
// the number of nodes. | ||
if p.healthTracker.HealthyNodeCount() == 0 || | ||
uint32(i) < p.pool.MaxConns()%uint32(p.healthTracker.HealthyNodeCount()) { | ||
i < int(p.pool.MaxConns())%p.healthTracker.HealthyNodeCount() { | ||
perNodeMax++ | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ import ( | |
"fmt" | ||
"time" | ||
|
||
"github.com/ccoveille/go-safecast" | ||
"github.com/exaring/otelpgx" | ||
"github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/retry" | ||
zerologadapter "github.com/jackc/pgx-zerolog" | ||
|
@@ -300,15 +301,23 @@ type PoolOptions struct { | |
} | ||
|
||
// ConfigurePgx applies PoolOptions to a pgx connection pool confiugration. | ||
func (opts PoolOptions) ConfigurePgx(pgxConfig *pgxpool.Config) { | ||
func (opts PoolOptions) ConfigurePgx(pgxConfig *pgxpool.Config) error { | ||
if opts.MaxOpenConns != nil { | ||
pgxConfig.MaxConns = int32(*opts.MaxOpenConns) | ||
maxConns, err := safecast.ToInt32(*opts.MaxOpenConns) | ||
if err != nil { | ||
return err | ||
} | ||
pgxConfig.MaxConns = maxConns | ||
Comment on lines
+306
to
+310
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would require a user to put a very specific value in, but we might as well catch it. |
||
} | ||
|
||
// Default to keeping the pool maxed out at all times. | ||
pgxConfig.MinConns = pgxConfig.MaxConns | ||
if opts.MinOpenConns != nil { | ||
pgxConfig.MinConns = int32(*opts.MinOpenConns) | ||
minConns, err := safecast.ToInt32(*opts.MinOpenConns) | ||
if err != nil { | ||
return err | ||
} | ||
pgxConfig.MinConns = minConns | ||
} | ||
|
||
if pgxConfig.MaxConns > 0 && pgxConfig.MinConns > 0 && pgxConfig.MaxConns < pgxConfig.MinConns { | ||
|
@@ -335,6 +344,7 @@ func (opts PoolOptions) ConfigurePgx(pgxConfig *pgxpool.Config) { | |
|
||
ConfigurePGXLogger(pgxConfig.ConnConfig) | ||
ConfigureOTELTracer(pgxConfig.ConnConfig) | ||
return nil | ||
} | ||
|
||
type QuerierFuncs struct { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return the error even if we think it cannot be hit; its safer than ignoring it