diff --git a/pkg/clients/postgresql/postgresql.go b/pkg/clients/postgresql/postgresql.go index 9dd60287..3b9afb05 100644 --- a/pkg/clients/postgresql/postgresql.go +++ b/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 diff --git a/pkg/clients/postgresql/postgresql_test.go b/pkg/clients/postgresql/postgresql_test.go index a2056b91..5b2a3894 100644 --- a/pkg/clients/postgresql/postgresql_test.go +++ b/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) + } +}