Skip to content

Commit ae50421

Browse files
josephschorrjosephschorr
authored
Merge pull request from GHSA-jg7w-cxjv-98c2
Redact any passwords found in datastore config errors
2 parents 5bcbd87 + 8ea7cfa commit ae50421

File tree

6 files changed

+101
-41
lines changed

6 files changed

+101
-41
lines changed

‎internal/datastore/common/errors.go

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,15 @@ package common
22

33
import (
44
"fmt"
5+
"regexp"
6+
"strings"
57

68
"google.golang.org/grpc/codes"
79
"google.golang.org/grpc/status"
810

911
v1 "github.com/authzed/authzed-go/proto/authzed/api/v1"
1012

13+
log "github.com/authzed/spicedb/internal/logging"
1114
core "github.com/authzed/spicedb/pkg/proto/core/v1"
1215
"github.com/authzed/spicedb/pkg/spiceerrors"
1316
"github.com/authzed/spicedb/pkg/tuple"
@@ -92,3 +95,20 @@ func NewCreateRelationshipExistsError(relationship *core.RelationTuple) error {
9295
relationship,
9396
}
9497
}
98+
99+
var (
100+
portMatchRegex = regexp.MustCompile("invalid port \\\"(.+)\\\" after host")
101+
parseMatchRegex = regexp.MustCompile("parse \\\"(.+)\\\":")
102+
)
103+
104+
// RedactAndLogSensitiveConnString elides the given error, logging it only at trace
105+
// level (after being redacted).
106+
func RedactAndLogSensitiveConnString(baseErr string, err error, pgURL string) error {
107+
// See: https://github.com/jackc/pgx/issues/1271
108+
filtered := err.Error()
109+
filtered = strings.ReplaceAll(filtered, pgURL, "(redacted)")
110+
filtered = portMatchRegex.ReplaceAllString(filtered, "(redacted)")
111+
filtered = parseMatchRegex.ReplaceAllString(filtered, "(redacted)")
112+
log.Trace().Msg(baseErr + ": " + filtered)
113+
return fmt.Errorf("%s. To view details of this error (that may contain sensitive information), please run with --log-level=trace", baseErr)
114+
}

‎internal/datastore/crdb/crdb.go

Lines changed: 12 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ const (
6464
colCaveatContextName = "caveat_name"
6565
colCaveatContext = "caveat_context"
6666

67-
errUnableToInstantiate = "unable to instantiate datastore: %w"
67+
errUnableToInstantiate = "unable to instantiate datastore"
6868
errRevision = "unable to find revision: %w"
6969

7070
querySelectNow = "SELECT cluster_logical_timestamp()"
@@ -76,18 +76,18 @@ const (
7676
func newCRDBDatastore(url string, options ...Option) (datastore.Datastore, error) {
7777
config, err := generateConfig(options)
7878
if err != nil {
79-
return nil, fmt.Errorf(errUnableToInstantiate, err)
79+
return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, url)
8080
}
8181

8282
readPoolConfig, err := pgxpool.ParseConfig(url)
8383
if err != nil {
84-
return nil, fmt.Errorf(errUnableToInstantiate, err)
84+
return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, url)
8585
}
8686
config.readPoolOpts.ConfigurePgx(readPoolConfig)
8787

8888
writePoolConfig, err := pgxpool.ParseConfig(url)
8989
if err != nil {
90-
return nil, fmt.Errorf(errUnableToInstantiate, err)
90+
return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, url)
9191
}
9292
config.writePoolOpts.ConfigurePgx(writePoolConfig)
9393

@@ -96,7 +96,7 @@ func newCRDBDatastore(url string, options ...Option) (datastore.Datastore, error
9696

9797
healthChecker, err := pool.NewNodeHealthChecker(url)
9898
if err != nil {
99-
return nil, fmt.Errorf(errUnableToInstantiate, err)
99+
return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, url)
100100
}
101101

102102
// The initPool is a 1-connection pool that is only used for setup tasks.
@@ -106,13 +106,13 @@ func newCRDBDatastore(url string, options ...Option) (datastore.Datastore, error
106106
initPoolConfig.MinConns = 1
107107
initPool, err := pool.NewRetryPool(initCtx, "init", initPoolConfig, healthChecker, config.maxRetries, config.connectRate)
108108
if err != nil {
109-
return nil, fmt.Errorf(errUnableToInstantiate, err)
109+
return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, url)
110110
}
111111
defer initPool.Close()
112112

113113
var version crdbVersion
114114
if err := queryServerVersion(initCtx, initPool, &version); err != nil {
115-
return nil, fmt.Errorf(errUnableToInstantiate, err)
115+
return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, url)
116116
}
117117

118118
changefeedQuery := queryChangefeed
@@ -140,10 +140,7 @@ func newCRDBDatastore(url string, options ...Option) (datastore.Datastore, error
140140
switch config.overlapStrategy {
141141
case overlapStrategyStatic:
142142
if len(config.overlapKey) == 0 {
143-
return nil, fmt.Errorf(
144-
errUnableToInstantiate,
145-
fmt.Errorf("static tx overlap strategy specified without an overlap key"),
146-
)
143+
return nil, fmt.Errorf("static tx overlap strategy specified without an overlap key")
147144
}
148145
keyer = appendStaticKey(config.overlapKey)
149146
case overlapStrategyPrefix:
@@ -183,12 +180,12 @@ func newCRDBDatastore(url string, options ...Option) (datastore.Datastore, error
183180
ds.writePool, err = pool.NewRetryPool(ds.ctx, "write", writePoolConfig, healthChecker, config.maxRetries, config.connectRate)
184181
if err != nil {
185182
ds.cancel()
186-
return nil, fmt.Errorf(errUnableToInstantiate, err)
183+
return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, url)
187184
}
188185
ds.readPool, err = pool.NewRetryPool(ds.ctx, "read", readPoolConfig, healthChecker, config.maxRetries, config.connectRate)
189186
if err != nil {
190187
ds.cancel()
191-
return nil, fmt.Errorf(errUnableToInstantiate, err)
188+
return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, url)
192189
}
193190

194191
if config.enablePrometheusStats {
@@ -197,15 +194,15 @@ func newCRDBDatastore(url string, options ...Option) (datastore.Datastore, error
197194
"pool_usage": "write",
198195
})); err != nil {
199196
ds.cancel()
200-
return nil, fmt.Errorf(errUnableToInstantiate, err)
197+
return nil, err
201198
}
202199

203200
if err := prometheus.Register(pgxpoolprometheus.NewCollector(ds.readPool, map[string]string{
204201
"db_name": "spicedb",
205202
"pool_usage": "read",
206203
})); err != nil {
207204
ds.cancel()
208-
return nil, fmt.Errorf(errUnableToInstantiate, err)
205+
return nil, err
209206
}
210207
}
211208

‎internal/datastore/mysql/datastore.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -111,16 +111,16 @@ func newMySQLDatastore(uri string, options ...Option) (*Datastore, error) {
111111

112112
parsedURI, err := mysql.ParseDSN(uri)
113113
if err != nil {
114-
return nil, fmt.Errorf("NewMySQLDatastore: could not parse connection URI `%s`: %w", uri, err)
114+
return nil, common.RedactAndLogSensitiveConnString("NewMySQLDatastore: could not parse connection URI", err, uri)
115115
}
116116

117117
if !parsedURI.ParseTime {
118-
return nil, fmt.Errorf("NewMySQLDatastore: connection URI for MySQL datastore must include `parseTime=true` as a query parameter. See https://spicedb.dev/d/parse-time-mysql for more details. Found: `%s`", uri)
118+
return nil, common.RedactAndLogSensitiveConnString("NewMySQLDatastore: connection URI for MySQL datastore must include `parseTime=true` as a query parameter. See https://spicedb.dev/d/parse-time-mysql for more details.", err, uri)
119119
}
120120

121121
connector, err := mysql.MySQLDriver{}.OpenConnector(uri)
122122
if err != nil {
123-
return nil, fmt.Errorf("NewMySQLDatastore: failed to create connector: %w", err)
123+
return nil, common.RedactAndLogSensitiveConnString("NewMySQLDatastore: failed to create connector", err, uri)
124124
}
125125

126126
if config.lockWaitTimeoutSeconds != nil {
@@ -129,15 +129,15 @@ func newMySQLDatastore(uri string, options ...Option) (*Datastore, error) {
129129
"innodb_lock_wait_timeout": strconv.FormatUint(uint64(*config.lockWaitTimeoutSeconds), 10),
130130
})
131131
if err != nil {
132-
return nil, fmt.Errorf("NewMySQLDatastore: failed to add session variables to connector: %w", err)
132+
return nil, common.RedactAndLogSensitiveConnString("NewMySQLDatastore: failed to add session variables to connector", err, uri)
133133
}
134134
}
135135

136136
var db *sql.DB
137137
if config.enablePrometheusStats {
138138
connector, err = instrumentConnector(connector)
139139
if err != nil {
140-
return nil, fmt.Errorf("NewMySQLDatastore: unable to instrument connector: %w", err)
140+
return nil, common.RedactAndLogSensitiveConnString("NewMySQLDatastore: unable to instrument connector", err, uri)
141141
}
142142

143143
db = sql.OpenDB(connector)

‎internal/datastore/postgres/postgres.go

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ const (
5656
colCaveatContextName = "caveat_name"
5757
colCaveatContext = "caveat_context"
5858

59-
errUnableToInstantiate = "unable to instantiate datastore: %w"
59+
errUnableToInstantiate = "unable to instantiate datastore"
6060

6161
// The parameters to this format string are:
6262
// 1: the created_xid or deleted_xid column name
@@ -125,19 +125,19 @@ func newPostgresDatastore(
125125
) (datastore.Datastore, error) {
126126
config, err := generateConfig(options)
127127
if err != nil {
128-
return nil, fmt.Errorf(errUnableToInstantiate, err)
128+
return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, pgURL)
129129
}
130130

131131
// Parse the DB URI into configuration.
132132
parsedConfig, err := pgxpool.ParseConfig(pgURL)
133133
if err != nil {
134-
return nil, fmt.Errorf(errUnableToInstantiate, err)
134+
return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, pgURL)
135135
}
136136

137137
// Setup the default custom plan setting, if applicable.
138138
pgConfig, err := defaultCustomPlan(parsedConfig)
139139
if err != nil {
140-
return nil, fmt.Errorf(errUnableToInstantiate, err)
140+
return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, pgURL)
141141
}
142142

143143
// Setup the config for each of the read and write pools.
@@ -168,20 +168,20 @@ func newPostgresDatastore(
168168

169169
readPool, err := pgxpool.NewWithConfig(initializationContext, readPoolConfig)
170170
if err != nil {
171-
return nil, fmt.Errorf(errUnableToInstantiate, err)
171+
return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, pgURL)
172172
}
173173

174174
writePool, err := pgxpool.NewWithConfig(initializationContext, writePoolConfig)
175175
if err != nil {
176-
return nil, fmt.Errorf(errUnableToInstantiate, err)
176+
return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, pgURL)
177177
}
178178

179179
// Verify that the server supports commit timestamps
180180
var trackTSOn string
181181
if err := readPool.
182182
QueryRow(initializationContext, "SHOW track_commit_timestamp;").
183183
Scan(&trackTSOn); err != nil {
184-
return nil, fmt.Errorf(errUnableToInstantiate, err)
184+
return nil, err
185185
}
186186

187187
watchEnabled := trackTSOn == "on"
@@ -194,16 +194,16 @@ func newPostgresDatastore(
194194
"db_name": "spicedb",
195195
"pool_usage": "read",
196196
})); err != nil {
197-
return nil, fmt.Errorf(errUnableToInstantiate, err)
197+
return nil, err
198198
}
199199
if err := prometheus.Register(pgxpoolprometheus.NewCollector(writePool, map[string]string{
200200
"db_name": "spicedb",
201201
"pool_usage": "write",
202202
})); err != nil {
203-
return nil, fmt.Errorf(errUnableToInstantiate, err)
203+
return nil, err
204204
}
205205
if err := common.RegisterGCMetrics(); err != nil {
206-
return nil, fmt.Errorf(errUnableToInstantiate, err)
206+
return nil, err
207207
}
208208
}
209209

‎internal/datastore/spanner/spanner.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ func init() {
3939
const (
4040
Engine = "spanner"
4141

42-
errUnableToInstantiate = "unable to instantiate spanner client: %w"
42+
errUnableToInstantiate = "unable to instantiate spanner client"
4343

4444
errRevision = "unable to load revision: %w"
4545

@@ -82,7 +82,7 @@ type spannerDatastore struct {
8282
func NewSpannerDatastore(database string, opts ...Option) (datastore.Datastore, error) {
8383
config, err := generateConfig(opts)
8484
if err != nil {
85-
return nil, fmt.Errorf(errUnableToInstantiate, err)
85+
return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, database)
8686
}
8787

8888
if len(config.emulatorHost) > 0 {
@@ -128,7 +128,7 @@ func NewSpannerDatastore(database string, opts ...Option) (datastore.Datastore,
128128
),
129129
)
130130
if err != nil {
131-
return nil, fmt.Errorf(errUnableToInstantiate, err)
131+
return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, database)
132132
}
133133

134134
maxRevisionStaleness := time.Duration(float64(config.revisionQuantization.Nanoseconds())*

‎pkg/datastore/errors_test.go

Lines changed: 51 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,59 @@
1-
package datastore
1+
package datastore_test
22

33
import (
44
"fmt"
55
"testing"
66

7-
"github.com/authzed/spicedb/internal/logging"
7+
"github.com/stretchr/testify/require"
8+
9+
"github.com/authzed/spicedb/internal/datastore/crdb"
10+
"github.com/authzed/spicedb/internal/datastore/mysql"
11+
"github.com/authzed/spicedb/internal/datastore/postgres"
12+
"github.com/authzed/spicedb/internal/datastore/spanner"
13+
"github.com/authzed/spicedb/pkg/datastore"
814
)
915

10-
func TestError(_ *testing.T) {
11-
logging.Info().Err(ErrNamespaceNotFound{
12-
error: fmt.Errorf("test"),
13-
namespaceName: "test/test",
14-
},
15-
).Msg("test")
16+
func createEngine(engineID string, uri string) error {
17+
switch engineID {
18+
case "postgres":
19+
_, err := postgres.NewPostgresDatastore(uri)
20+
return err
21+
22+
case "mysql":
23+
_, err := mysql.NewMySQLDatastore(uri)
24+
return err
25+
26+
case "spanner":
27+
_, err := spanner.NewSpannerDatastore(uri)
28+
return err
29+
30+
case "cockroachdb":
31+
_, err := crdb.NewCRDBDatastore(uri)
32+
return err
33+
34+
default:
35+
panic(fmt.Sprintf("missing create implementation for engine %s", engineID))
36+
}
37+
}
38+
39+
func TestDatastoreURIErrors(t *testing.T) {
40+
tcs := map[string]string{
41+
"some-wrong-uri": "wrong",
42+
"postgres://foo:bar:baz@someurl": "bar",
43+
"postgres://spicedb:somepassword": "somepassword",
44+
"postgres://spicedb:somepassword#@foo": "somepassword",
45+
"username=foo password=somepassword dsn=whatever": "somepassword",
46+
}
47+
48+
for _, engineID := range datastore.Engines {
49+
t.Run(engineID, func(t *testing.T) {
50+
for tc, check := range tcs {
51+
t.Run(tc, func(t *testing.T) {
52+
err := createEngine(engineID, tc)
53+
require.Error(t, err)
54+
require.NotContains(t, err.Error(), check)
55+
})
56+
}
57+
})
58+
}
1659
}

0 commit comments

Comments
 (0)