From 7a90077d7c0b344efaa189f46cd0d9c77269d107 Mon Sep 17 00:00:00 2001 From: Myrian Koumgang Date: Thu, 8 Oct 2026 16:41:35 +0200 Subject: [PATCH 1/2] fix(postgresql): mask the password in DSN parse errors lib/pq reports a DSN it cannot parse as a *url.Error that carries the whole DSN, password included. A connection Secret value with a stray newline is enough to trigger it, and the error then ends up in the managed resource's status conditions and events. Replace the URL in those errors with the same DSN with the password masked, so the message still shows what is wrong with it. Every other error passes through unchanged, since callers check for sql.ErrNoRows and pq.Error. Signed-off-by: Myrian Koumgang --- pkg/clients/postgresql/postgresql.go | 21 +++++-- pkg/clients/postgresql/postgresql_test.go | 76 +++++++++++++++++++++++ 2 files changed, 93 insertions(+), 4 deletions(-) diff --git a/pkg/clients/postgresql/postgresql.go b/pkg/clients/postgresql/postgresql.go index 9dd60287..91c0eec5 100644 --- a/pkg/clients/postgresql/postgresql.go +++ b/pkg/clients/postgresql/postgresql.go @@ -22,6 +22,7 @@ const ( type postgresDB struct { dsn string + redacted string // dsn with the password masked, for error messages endpoint string port string sslmode string @@ -40,12 +41,24 @@ func New(creds map[string][]byte, database, sslmode string) xsql.DB { return postgresDB{ dsn: dsn, + redacted: DSN(username, "xxxxx", 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. +func (c postgresDB) redact(err error) error { + var uerr *url.Error + if !errors.As(err, &uerr) { + return err + } + return &url.Error{Op: uerr.Op, URL: c.redacted, Err: uerr.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 @@ -70,7 +83,7 @@ func (c postgresDB) ExecTx(ctx context.Context, ql []xsql.Query) error { 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 @@ -102,7 +115,7 @@ func (c postgresDB) Exec(ctx context.Context, q xsql.Query) error { defer d.Close() //nolint:errcheck _, err = d.ExecContext(ctx, q.String, q.Parameters...) - return err + return c.redact(err) } // Query the supplied query. @@ -114,7 +127,7 @@ func (c postgresDB) Query(ctx context.Context, q xsql.Query) (*sql.Rows, error) 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. @@ -125,7 +138,7 @@ func (c postgresDB) Scan(ctx context.Context, q xsql.Query, dest ...interface{}) } 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 diff --git a/pkg/clients/postgresql/postgresql_test.go b/pkg/clients/postgresql/postgresql_test.go index a2056b91..b21aeaa0 100644 --- a/pkg/clients/postgresql/postgresql_test.go +++ b/pkg/clients/postgresql/postgresql_test.go @@ -1,7 +1,15 @@ package postgresql import ( + "context" + "database/sql" + "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 +25,71 @@ 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) + }, + } + 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) + } + }) + } +} From 4948458044c286fbfe90c5c394c5dff5d4f66d5e Mon Sep 17 00:00:00 2001 From: Myrian Koumgang Date: Fri, 30 Oct 2026 10:20:39 +0100 Subject: [PATCH 2/2] fix(postgresql): redact every DSN error and keep its wrapper text redact rebuilt the *url.Error, so any text wrapping it was lost. It now keeps the message around the *url.Error and unwraps to the masked one. The sql.Open errors and GetServerVersion go through redact too, and the mask is a constant. Signed-off-by: Myrian Koumgang --- pkg/clients/postgresql/postgresql.go | 42 ++++++++++++++++++----- pkg/clients/postgresql/postgresql_test.go | 28 +++++++++++++++ 2 files changed, 61 insertions(+), 9 deletions(-) diff --git a/pkg/clients/postgresql/postgresql.go b/pkg/clients/postgresql/postgresql.go index 91c0eec5..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,6 +19,9 @@ 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 { @@ -41,7 +45,7 @@ func New(creds map[string][]byte, database, sslmode string) xsql.DB { return postgresDB{ dsn: dsn, - redacted: DSN(username, "xxxxx", endpoint, port, database, sslmode), + redacted: DSN(username, redactedPassword, endpoint, port, database, sslmode), endpoint: endpoint, port: port, sslmode: sslmode, @@ -50,15 +54,35 @@ func New(creds map[string][]byte, database, sslmode string) xsql.DB { // 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. +// 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 } - return &url.Error{Op: uerr.Op, URL: c.redacted, Err: uerr.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 @@ -78,7 +102,7 @@ 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) @@ -110,7 +134,7 @@ 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 @@ -122,7 +146,7 @@ func (c postgresDB) Exec(ctx context.Context, q xsql.Query) error { 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 @@ -134,7 +158,7 @@ func (c postgresDB) Query(ctx context.Context, q xsql.Query) (*sql.Rows, error) 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 @@ -156,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 b21aeaa0..5b2a3894 100644 --- a/pkg/clients/postgresql/postgresql_test.go +++ b/pkg/clients/postgresql/postgresql_test.go @@ -3,6 +3,9 @@ package postgresql import ( "context" "database/sql" + "errors" + "fmt" + "net/url" "strings" "testing" @@ -54,6 +57,10 @@ func TestDSNParseErrorOmitsPassword(t *testing.T) { 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) { @@ -93,3 +100,24 @@ func TestRedactPassesOtherErrorsThrough(t *testing.T) { }) } } + +// 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) + } +}