Merge pull request from GHSA-jg7w-cxjv-98c2

Redact any passwords found in datastore config errors

internal/datastore/common/errors.go

@@ -2,12 +2,15 @@ package common
 
 import (
 	"fmt"
+	"regexp"
+	"strings"
 
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 
 	v1 "github.com/authzed/authzed-go/proto/authzed/api/v1"
 
+	log "github.com/authzed/spicedb/internal/logging"
 	core "github.com/authzed/spicedb/pkg/proto/core/v1"
 	"github.com/authzed/spicedb/pkg/spiceerrors"
 	"github.com/authzed/spicedb/pkg/tuple"
@@ -92,3 +95,20 @@ func NewCreateRelationshipExistsError(relationship *core.RelationTuple) error {
 		relationship,
 	}
 }
+
+var (
+	portMatchRegex  = regexp.MustCompile("invalid port \\\"(.+)\\\" after host")
+	parseMatchRegex = regexp.MustCompile("parse \\\"(.+)\\\":")
+)
+
+// RedactAndLogSensitiveConnString elides the given error, logging it only at trace
+// level (after being redacted).
+func RedactAndLogSensitiveConnString(baseErr string, err error, pgURL string) error {
+	// See: https://github.com/jackc/pgx/issues/1271
+	filtered := err.Error()
+	filtered = strings.ReplaceAll(filtered, pgURL, "(redacted)")
+	filtered = portMatchRegex.ReplaceAllString(filtered, "(redacted)")
+	filtered = parseMatchRegex.ReplaceAllString(filtered, "(redacted)")
+	log.Trace().Msg(baseErr + ": " + filtered)
+	return fmt.Errorf("%s. To view details of this error (that may contain sensitive information), please run with --log-level=trace", baseErr)
+}

internal/datastore/crdb/crdb.go

@@ -64,7 +64,7 @@ const (
 	colCaveatContextName = "caveat_name"
 	colCaveatContext     = "caveat_context"
 
-	errUnableToInstantiate = "unable to instantiate datastore: %w"
+	errUnableToInstantiate = "unable to instantiate datastore"
 	errRevision            = "unable to find revision: %w"
 
 	querySelectNow      = "SELECT cluster_logical_timestamp()"
@@ -76,18 +76,18 @@ const (
 func newCRDBDatastore(url string, options ...Option) (datastore.Datastore, error) {
 	config, err := generateConfig(options)
 	if err != nil {
-		return nil, fmt.Errorf(errUnableToInstantiate, err)
+		return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, url)
 	}
 
 	readPoolConfig, err := pgxpool.ParseConfig(url)
 	if err != nil {
-		return nil, fmt.Errorf(errUnableToInstantiate, err)
+		return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, url)
 	}
 	config.readPoolOpts.ConfigurePgx(readPoolConfig)
 
 	writePoolConfig, err := pgxpool.ParseConfig(url)
 	if err != nil {
-		return nil, fmt.Errorf(errUnableToInstantiate, err)
+		return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, url)
 	}
 	config.writePoolOpts.ConfigurePgx(writePoolConfig)
 
@@ -96,7 +96,7 @@ func newCRDBDatastore(url string, options ...Option) (datastore.Datastore, error
 
 	healthChecker, err := pool.NewNodeHealthChecker(url)
 	if err != nil {
-		return nil, fmt.Errorf(errUnableToInstantiate, err)
+		return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, url)
 	}
 
 	// 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
 	initPoolConfig.MinConns = 1
 	initPool, err := pool.NewRetryPool(initCtx, "init", initPoolConfig, healthChecker, config.maxRetries, config.connectRate)
 	if err != nil {
-		return nil, fmt.Errorf(errUnableToInstantiate, err)
+		return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, url)
 	}
 	defer initPool.Close()
 
 	var version crdbVersion
 	if err := queryServerVersion(initCtx, initPool, &version); err != nil {
-		return nil, fmt.Errorf(errUnableToInstantiate, err)
+		return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, url)
 	}
 
 	changefeedQuery := queryChangefeed
@@ -140,10 +140,7 @@ func newCRDBDatastore(url string, options ...Option) (datastore.Datastore, error
 	switch config.overlapStrategy {
 	case overlapStrategyStatic:
 		if len(config.overlapKey) == 0 {
-			return nil, fmt.Errorf(
-				errUnableToInstantiate,
-				fmt.Errorf("static tx overlap strategy specified without an overlap key"),
-			)
+			return nil, fmt.Errorf("static tx overlap strategy specified without an overlap key")
 		}
 		keyer = appendStaticKey(config.overlapKey)
 	case overlapStrategyPrefix:
@@ -183,12 +180,12 @@ func newCRDBDatastore(url string, options ...Option) (datastore.Datastore, error
 	ds.writePool, err = pool.NewRetryPool(ds.ctx, "write", writePoolConfig, healthChecker, config.maxRetries, config.connectRate)
 	if err != nil {
 		ds.cancel()
-		return nil, fmt.Errorf(errUnableToInstantiate, err)
+		return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, url)
 	}
 	ds.readPool, err = pool.NewRetryPool(ds.ctx, "read", readPoolConfig, healthChecker, config.maxRetries, config.connectRate)
 	if err != nil {
 		ds.cancel()
-		return nil, fmt.Errorf(errUnableToInstantiate, err)
+		return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, url)
 	}
 
 	if config.enablePrometheusStats {
@@ -197,15 +194,15 @@ func newCRDBDatastore(url string, options ...Option) (datastore.Datastore, error
 			"pool_usage": "write",
 		})); err != nil {
 			ds.cancel()
-			return nil, fmt.Errorf(errUnableToInstantiate, err)
+			return nil, err
 		}
 
 		if err := prometheus.Register(pgxpoolprometheus.NewCollector(ds.readPool, map[string]string{
 			"db_name":    "spicedb",
 			"pool_usage": "read",
 		})); err != nil {
 			ds.cancel()
-			return nil, fmt.Errorf(errUnableToInstantiate, err)
+			return nil, err
 		}
 	}
 

internal/datastore/mysql/datastore.go

@@ -111,16 +111,16 @@ func newMySQLDatastore(uri string, options ...Option) (*Datastore, error) {
 
 	parsedURI, err := mysql.ParseDSN(uri)
 	if err != nil {
-		return nil, fmt.Errorf("NewMySQLDatastore: could not parse connection URI `%s`: %w", uri, err)
+		return nil, common.RedactAndLogSensitiveConnString("NewMySQLDatastore: could not parse connection URI", err, uri)
 	}
 
 	if !parsedURI.ParseTime {
-		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)
+		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)
 	}
 
 	connector, err := mysql.MySQLDriver{}.OpenConnector(uri)
 	if err != nil {
-		return nil, fmt.Errorf("NewMySQLDatastore: failed to create connector: %w", err)
+		return nil, common.RedactAndLogSensitiveConnString("NewMySQLDatastore: failed to create connector", err, uri)
 	}
 
 	if config.lockWaitTimeoutSeconds != nil {
@@ -129,15 +129,15 @@ func newMySQLDatastore(uri string, options ...Option) (*Datastore, error) {
 			"innodb_lock_wait_timeout": strconv.FormatUint(uint64(*config.lockWaitTimeoutSeconds), 10),
 		})
 		if err != nil {
-			return nil, fmt.Errorf("NewMySQLDatastore: failed to add session variables to connector: %w", err)
+			return nil, common.RedactAndLogSensitiveConnString("NewMySQLDatastore: failed to add session variables to connector", err, uri)
 		}
 	}
 
 	var db *sql.DB
 	if config.enablePrometheusStats {
 		connector, err = instrumentConnector(connector)
 		if err != nil {
-			return nil, fmt.Errorf("NewMySQLDatastore: unable to instrument connector: %w", err)
+			return nil, common.RedactAndLogSensitiveConnString("NewMySQLDatastore: unable to instrument connector", err, uri)
 		}
 
 		db = sql.OpenDB(connector)

internal/datastore/postgres/postgres.go

@@ -56,7 +56,7 @@ const (
 	colCaveatContextName = "caveat_name"
 	colCaveatContext     = "caveat_context"
 
-	errUnableToInstantiate = "unable to instantiate datastore: %w"
+	errUnableToInstantiate = "unable to instantiate datastore"
 
 	// The parameters to this format string are:
 	// 1: the created_xid or deleted_xid column name
@@ -125,19 +125,19 @@ func newPostgresDatastore(
 ) (datastore.Datastore, error) {
 	config, err := generateConfig(options)
 	if err != nil {
-		return nil, fmt.Errorf(errUnableToInstantiate, err)
+		return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, pgURL)
 	}
 
 	// Parse the DB URI into configuration.
 	parsedConfig, err := pgxpool.ParseConfig(pgURL)
 	if err != nil {
-		return nil, fmt.Errorf(errUnableToInstantiate, err)
+		return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, pgURL)
 	}
 
 	// Setup the default custom plan setting, if applicable.
 	pgConfig, err := defaultCustomPlan(parsedConfig)
 	if err != nil {
-		return nil, fmt.Errorf(errUnableToInstantiate, err)
+		return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, pgURL)
 	}
 
 	// Setup the config for each of the read and write pools.
@@ -168,20 +168,20 @@ func newPostgresDatastore(
 
 	readPool, err := pgxpool.NewWithConfig(initializationContext, readPoolConfig)
 	if err != nil {
-		return nil, fmt.Errorf(errUnableToInstantiate, err)
+		return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, pgURL)
 	}
 
 	writePool, err := pgxpool.NewWithConfig(initializationContext, writePoolConfig)
 	if err != nil {
-		return nil, fmt.Errorf(errUnableToInstantiate, err)
+		return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, pgURL)
 	}
 
 	// Verify that the server supports commit timestamps
 	var trackTSOn string
 	if err := readPool.
 		QueryRow(initializationContext, "SHOW track_commit_timestamp;").
 		Scan(&trackTSOn); err != nil {
-		return nil, fmt.Errorf(errUnableToInstantiate, err)
+		return nil, err
 	}
 
 	watchEnabled := trackTSOn == "on"
@@ -194,16 +194,16 @@ func newPostgresDatastore(
 			"db_name":    "spicedb",
 			"pool_usage": "read",
 		})); err != nil {
-			return nil, fmt.Errorf(errUnableToInstantiate, err)
+			return nil, err
 		}
 		if err := prometheus.Register(pgxpoolprometheus.NewCollector(writePool, map[string]string{
 			"db_name":    "spicedb",
 			"pool_usage": "write",
 		})); err != nil {
-			return nil, fmt.Errorf(errUnableToInstantiate, err)
+			return nil, err
 		}
 		if err := common.RegisterGCMetrics(); err != nil {
-			return nil, fmt.Errorf(errUnableToInstantiate, err)
+			return nil, err
 		}
 	}
 

internal/datastore/spanner/spanner.go

@@ -39,7 +39,7 @@ func init() {
 const (
 	Engine = "spanner"
 
-	errUnableToInstantiate = "unable to instantiate spanner client: %w"
+	errUnableToInstantiate = "unable to instantiate spanner client"
 
 	errRevision = "unable to load revision: %w"
 
@@ -82,7 +82,7 @@ type spannerDatastore struct {
 func NewSpannerDatastore(database string, opts ...Option) (datastore.Datastore, error) {
 	config, err := generateConfig(opts)
 	if err != nil {
-		return nil, fmt.Errorf(errUnableToInstantiate, err)
+		return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, database)
 	}
 
 	if len(config.emulatorHost) > 0 {
@@ -128,7 +128,7 @@ func NewSpannerDatastore(database string, opts ...Option) (datastore.Datastore,
 		),
 	)
 	if err != nil {
-		return nil, fmt.Errorf(errUnableToInstantiate, err)
+		return nil, common.RedactAndLogSensitiveConnString(errUnableToInstantiate, err, database)
 	}
 
 	maxRevisionStaleness := time.Duration(float64(config.revisionQuantization.Nanoseconds())*

pkg/datastore/errors_test.go

@@ -1,16 +1,59 @@
-package datastore
+package datastore_test
 
 import (
 	"fmt"
 	"testing"
 
-	"github.com/authzed/spicedb/internal/logging"
+	"github.com/stretchr/testify/require"
+
+	"github.com/authzed/spicedb/internal/datastore/crdb"
+	"github.com/authzed/spicedb/internal/datastore/mysql"
+	"github.com/authzed/spicedb/internal/datastore/postgres"
+	"github.com/authzed/spicedb/internal/datastore/spanner"
+	"github.com/authzed/spicedb/pkg/datastore"
 )
 
-func TestError(_ *testing.T) {
-	logging.Info().Err(ErrNamespaceNotFound{
-		error:         fmt.Errorf("test"),
-		namespaceName: "test/test",
-	},
-	).Msg("test")
+func createEngine(engineID string, uri string) error {
+	switch engineID {
+	case "postgres":
+		_, err := postgres.NewPostgresDatastore(uri)
+		return err
+
+	case "mysql":
+		_, err := mysql.NewMySQLDatastore(uri)
+		return err
+
+	case "spanner":
+		_, err := spanner.NewSpannerDatastore(uri)
+		return err
+
+	case "cockroachdb":
+		_, err := crdb.NewCRDBDatastore(uri)
+		return err
+
+	default:
+		panic(fmt.Sprintf("missing create implementation for engine %s", engineID))
+	}
+}
+
+func TestDatastoreURIErrors(t *testing.T) {
+	tcs := map[string]string{
+		"some-wrong-uri":                                  "wrong",
+		"postgres://foo:bar:baz@someurl":                  "bar",
+		"postgres://spicedb:somepassword":                 "somepassword",
+		"postgres://spicedb:somepassword#@foo":            "somepassword",
+		"username=foo password=somepassword dsn=whatever": "somepassword",
+	}
+
+	for _, engineID := range datastore.Engines {
+		t.Run(engineID, func(t *testing.T) {
+			for tc, check := range tcs {
+				t.Run(tc, func(t *testing.T) {
+					err := createEngine(engineID, tc)
+					require.Error(t, err)
+					require.NotContains(t, err.Error(), check)
+				})
+			}
+		})
+	}
 }