fix(postgresql): mask the password in DSN parse errors

pkg/clients/postgresql/postgresql.go

@@ -5,6 +5,7 @@ import (
 	"database/sql"
 	"errors"
 	"net/url"
+	"strings"
 
 	"github.com/crossplane-contrib/provider-sql/pkg/clients/xsql"
 	"github.com/lib/pq"
@@ -18,10 +19,14 @@ const (
 	// https://www.postgresql.org/docs/current/errcodes-appendix.html
 	// These are not available as part of the pq library.
 	pqInvalidCatalog = pqerror.Code("3D000")
+
+	// redactedPassword replaces the password in the DSN that error messages show.
+	redactedPassword = "xxxxx"
 )
 
 type postgresDB struct {
 	dsn      string
+	redacted string // dsn with the password masked, for error messages
 	endpoint string
 	port     string
 	sslmode  string
@@ -40,12 +45,44 @@ func New(creds map[string][]byte, database, sslmode string) xsql.DB {
 
 	return postgresDB{
 		dsn:      dsn,
+		redacted: DSN(username, redactedPassword, endpoint, port, database, sslmode),
 		endpoint: endpoint,
 		port:     port,
 		sslmode:  sslmode,
 	}
 }
 
+// redact masks the password in DSN parse errors. lib/pq reports a DSN it
+// cannot parse as a *url.Error that carries the whole DSN, and the error
+// ends up in status conditions and events. Text that wraps the *url.Error
+// is kept.
+func (c postgresDB) redact(err error) error {
+	var uerr *url.Error
+	if !errors.As(err, &uerr) {
+		return err
+	}
+	masked := *uerr
+	masked.URL = c.redacted
+	msg := err.Error()
+	if !strings.Contains(msg, uerr.Error()) {
+		// The wrapper doesn't print the *url.Error as is, so its text might
+		// still carry the password in another form.
+		return &masked
+	}
+	return &redactedError{msg: strings.Replace(msg, uerr.Error(), masked.Error(), 1), err: &masked}
+}
+
+// redactedError is an error that wrapped a DSN parse error, with the
+// password masked in its message. It unwraps to the masked *url.Error.
+type redactedError struct {
+	msg string
+	err error
+}
+
+func (e *redactedError) Error() string { return e.msg }
+
+func (e *redactedError) Unwrap() error { return e.err }
+
 // DSN returns the DSN URL
 func DSN(username, password, endpoint, port, database, sslmode string) string {
 	// Use net/url UserPassword to encode the username and password
@@ -65,12 +102,12 @@ func DSN(username, password, endpoint, port, database, sslmode string) string {
 func (c postgresDB) ExecTx(ctx context.Context, ql []xsql.Query) error {
 	d, err := sql.Open("postgres", c.dsn)
 	if err != nil {
-		return err
+		return c.redact(err)
 	}
 
 	tx, err := d.BeginTx(ctx, nil)
 	if err != nil {
-		return err
+		return c.redact(err)
 	}
 
 	// Rollback or Commit based on error state. Defer close in defer to make
@@ -97,35 +134,35 @@ func (c postgresDB) ExecTx(ctx context.Context, ql []xsql.Query) error {
 func (c postgresDB) Exec(ctx context.Context, q xsql.Query) error {
 	d, err := sql.Open("postgres", c.dsn)
 	if err != nil {
-		return err
+		return c.redact(err)
 	}
 	defer d.Close() //nolint:errcheck
 
 	_, err = d.ExecContext(ctx, q.String, q.Parameters...)
-	return err
+	return c.redact(err)
 }
 
 // Query the supplied query.
 func (c postgresDB) Query(ctx context.Context, q xsql.Query) (*sql.Rows, error) {
 	d, err := sql.Open("postgres", c.dsn)
 	if err != nil {
-		return nil, err
+		return nil, c.redact(err)
 	}
 	defer d.Close() //nolint:errcheck
 
 	rows, err := d.QueryContext(ctx, q.String, q.Parameters...)
-	return rows, err
+	return rows, c.redact(err)
 }
 
 // Scan the results of the supplied query into the supplied destination.
 func (c postgresDB) Scan(ctx context.Context, q xsql.Query, dest ...interface{}) error {
 	db, err := sql.Open("postgres", c.dsn)
 	if err != nil {
-		return err
+		return c.redact(err)
 	}
 	defer db.Close() //nolint:errcheck
 
-	return db.QueryRowContext(ctx, q.String, q.Parameters...).Scan(dest...)
+	return c.redact(db.QueryRowContext(ctx, q.String, q.Parameters...).Scan(dest...))
 }
 
 // GetConnectionDetails returns the connection details for a user of this DB
@@ -143,13 +180,13 @@ func (c postgresDB) GetConnectionDetails(username, password string) managed.Conn
 func (c postgresDB) GetServerVersion(ctx context.Context) (int, error) {
 	db, err := sql.Open("postgres", c.dsn)
 	if err != nil {
-		return 0, err
+		return 0, c.redact(err)
 	}
 	defer db.Close() //nolint:errcheck
 
 	var version int
 	err = db.QueryRowContext(ctx, "SELECT current_setting('server_version_num')::int").Scan(&version)
-	return version, err
+	return version, c.redact(err)
 }
 
 // IsInvalidCatalog returns true if passed a pq error indicating

pkg/clients/postgresql/postgresql_test.go

@@ -1,7 +1,18 @@
 package postgresql
 
 import (
+	"context"
+	"database/sql"
+	"errors"
+	"fmt"
+	"net/url"
+	"strings"
 	"testing"
+
+	"github.com/crossplane-contrib/provider-sql/pkg/clients/xsql"
+	"github.com/lib/pq"
+
+	xpv2 "github.com/crossplane/crossplane/apis/v2/core/v2"
 )
 
 func TestDSNURLEscaping(t *testing.T) {
@@ -17,3 +28,96 @@ func TestDSNURLEscaping(t *testing.T) {
 		t.Errorf("DSN string did not match expected output with userinfo URL encoded")
 	}
 }
+
+// A stray newline in a connection Secret value, often left behind by
+// base64-encoding with echo, makes the DSN unparseable. That error is written
+// to the managed resource's status conditions and events, so it must not carry
+// the password.
+func TestDSNParseErrorOmitsPassword(t *testing.T) {
+	const password = "s3cr3t-pa55"
+	db := New(map[string][]byte{
+		xpv2.CredentialsSecretEndpointKey: []byte("db.example.org\n"),
+		xpv2.CredentialsSecretPortKey:     []byte("5432"),
+		xpv2.CredentialsSecretUserKey:     []byte("admin"),
+		xpv2.CredentialsSecretPasswordKey: []byte(password),
+	}, "postgres", "require")
+	q := xsql.Query{String: "SELECT 1"}
+
+	cases := map[string]func(ctx context.Context) error{
+		"Exec":   func(ctx context.Context) error { return db.Exec(ctx, q) },
+		"ExecTx": func(ctx context.Context) error { return db.ExecTx(ctx, []xsql.Query{q}) },
+		"Query": func(ctx context.Context) error {
+			rows, err := db.Query(ctx, q)
+			if err == nil {
+				_ = rows.Close()
+			}
+			return err
+		},
+		"Scan": func(ctx context.Context) error {
+			var v int
+			return db.Scan(ctx, q, &v)
+		},
+		"GetServerVersion": func(ctx context.Context) error {
+			_, err := db.GetServerVersion(ctx)
+			return err
+		},
+	}
+	for name, call := range cases {
+		t.Run(name, func(t *testing.T) {
+			err := call(t.Context())
+			if err == nil {
+				t.Fatalf("%s(): got nil error, want a DSN parse error", name)
+			}
+			if strings.Contains(err.Error(), password) {
+				t.Errorf("%s(): error %q contains the password", name, err)
+			}
+			if !strings.Contains(err.Error(), "invalid control character in URL") {
+				t.Errorf("%s(): error %q no longer says why the DSN is invalid", name, err)
+			}
+		})
+	}
+}
+
+// Callers branch on some of the errors the client returns, such as
+// xsql.IsNoRows and IsInvalidCatalog, so redact must leave everything except
+// a DSN parse error untouched.
+func TestRedactPassesOtherErrorsThrough(t *testing.T) {
+	c := postgresDB{redacted: "postgres://admin:xxxxx@db.example.org/postgres"}
+
+	cases := map[string]struct {
+		err  error
+		want func(error) bool
+	}{
+		"Nil":            {err: nil, want: func(err error) bool { return err == nil }},
+		"NoRows":         {err: sql.ErrNoRows, want: xsql.IsNoRows},
+		"InvalidCatalog": {err: &pq.Error{Code: pqInvalidCatalog}, want: IsInvalidCatalog},
+	}
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			if got := c.redact(tc.err); !tc.want(got) {
+				t.Errorf("redact(%v) = %v, want the same error back", tc.err, got)
+			}
+		})
+	}
+}
+
+// A DSN parse error that reaches redact wrapped by another error keeps the
+// wrapper's text, and errors.As still finds the *url.Error, with the password
+// masked.
+func TestRedactKeepsWrapperText(t *testing.T) {
+	c := postgresDB{redacted: "postgres://admin:xxxxx@db.example.org/postgres"}
+	parseErr := &url.Error{Op: "parse", URL: "postgres://admin:s3cr3t@db.example.org/postgres", Err: errors.New("invalid port")}
+
+	err := c.redact(fmt.Errorf("connecting: %w", parseErr))
+	want := `connecting: parse "postgres://admin:xxxxx@db.example.org/postgres": invalid port`
+	if err.Error() != want {
+		t.Errorf("redact(...).Error() = %q, want %q", err.Error(), want)
+	}
+	var uerr *url.Error
+	if !errors.As(err, &uerr) {
+		t.Fatalf("errors.As(redact(...), *url.Error) = false, want true")
+	}
+	if uerr.URL != c.redacted {
+		t.Errorf("unwrapped *url.Error URL = %q, want %q", uerr.URL, c.redacted)
+	}
+}