From 494e2a6808986cc67242ecff670d94c0948ea7a9 Mon Sep 17 00:00:00 2001 From: Michael Lopez Date: Thu, 22 Oct 2026 22:36:04 +0200 Subject: [PATCH] db: quote postgres connection parameters (#3477) * db: quote postgres connection parameters The DSN was built by string concatenation, so a password containing a space, quote or backslash truncated it or injected extra parameters. Build it with libpq keyword/value quoting and keep the password out of the startup log line. * db: mark the DSN test fixture password as not a credential gosec G101 flags the literal assigned to Pass in TestBuildPostgresDSN. The value exists to exercise quoting of spaces, quotes and backslashes and is not a secret. Co-Authored-By: Claude Fable 5.1 * types, db: build the postgres DSN as a URL from PostgresConfig Review feedback: move the DSN construction onto types.PostgresConfig as DSN(), so the database layer calls cfg.Postgres.DSN() and logs redacted.DSN(), and build it as a postgres:// URL with net/url instead of hand-rolled libpq quoting. Unix socket directories travel as the host query parameter; bare IPv6 literals are bracketed. Tests parse the URL back and check each component rather than the encoded string. * types: silence gosec on the whole DSN test fixtures gosec reports G101 on the composite literal that holds a Pass value, not on the Pass line itself, so the previous nolint did not cover it. Place the directive before each fixture element. * db: drop imports left unused by the merge with main The merge of main reintroduced the slices and strconv imports in db.go, but the postgres DSN is now built by PostgresConfig.DSN and nothing in the file uses them, so the package failed to compile and every CI check failed on typecheck. Also link the CHANGELOG entry to the PR. * db: restore tests and formatting lost in the rebase onto main Resolving the rebase conflicts in db_test.go kept this branch's side and dropped the credential tests main had added, including the requireCredentialConstraints helper another test still calls, so the db test package no longer compiled. Restore the file from main; this branch does not change it. The resolution also indented the package clause in db.go and removed the blank line between the standard library and module imports. --------- Co-authored-by: Claude Fable 5.1 --- CHANGELOG.md | 1 + hscontrol/db/db.go | 33 +++--------- hscontrol/types/config.go | 56 ++++++++++++++++++++ hscontrol/types/config_test.go | 95 ++++++++++++++++++++++++++++++++++ 4 files changed, 159 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 80804ab39..9b9d7a19f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -123,6 +123,7 @@ the ACME DNS-01 challenge through a built-in DNS provider: `acmedns`, ### Changes +- PostgreSQL connection settings are assembled into a `postgres://` URL with proper escaping, so a password (or host, user, database name) containing spaces, quotes or other reserved characters no longer truncates the DSN [#3477](https://github.com/juanfont/headscale/pull/3477) - Expiring or deleting a non-existent pre-auth key now returns an error instead of silently succeeding [#3324](https://github.com/juanfont/headscale/pull/3324) - Improve systemd service file hardening [#3341](https://github.com/juanfont/headscale/pull/3341) - Fix `headscale users destroy`/`rename` reporting "multiple users match query" when no user matches; an ambiguous match now lists the matching users [#3476](https://github.com/juanfont/headscale/pull/3476) diff --git a/hscontrol/db/db.go b/hscontrol/db/db.go index 3feacb36c..aadbb3f7f 100644 --- a/hscontrol/db/db.go +++ b/hscontrol/db/db.go @@ -6,7 +6,6 @@ import ( "errors" "fmt" "path/filepath" - "strconv" "time" "github.com/glebarez/sqlite" @@ -14,6 +13,7 @@ import ( "github.com/juanfont/headscale/hscontrol/db/sqliteconfig" "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/util" + "github.com/juanfont/headscale/hscontrol/util/zlog/zf" "github.com/rs/zerolog/log" "github.com/tailscale/squibble" "gorm.io/driver/postgres" @@ -465,35 +465,16 @@ func openDB(cfg types.DatabaseConfig) (*gorm.DB, error) { return db, nil case types.DatabasePostgres: - dbString := fmt.Sprintf( - "host=%s dbname=%s user=%s", - cfg.Postgres.Host, - cfg.Postgres.Name, - cfg.Postgres.User, - ) + // Log the DSN without the password. + redacted := cfg.Postgres + redacted.Pass = "" log.Info(). - Str("database", types.DatabasePostgres). - Str("path", dbString). + Str(zf.Database, types.DatabasePostgres). + Str(zf.Path, redacted.DSN()). Msg("Opening database") - if sslEnabled, err := strconv.ParseBool(cfg.Postgres.Ssl); err == nil { //nolint:noinlineerr - if !sslEnabled { - dbString += " sslmode=disable" - } - } else { - dbString += " sslmode=" + cfg.Postgres.Ssl - } - - if cfg.Postgres.Port != 0 { - dbString += fmt.Sprintf(" port=%d", cfg.Postgres.Port) - } - - if cfg.Postgres.Pass != "" { - dbString += " password=" + cfg.Postgres.Pass - } - - db, err := gorm.Open(postgres.Open(dbString), &gorm.Config{ + db, err := gorm.Open(postgres.Open(cfg.Postgres.DSN()), &gorm.Config{ Logger: dbLogger, }) if err != nil { diff --git a/hscontrol/types/config.go b/hscontrol/types/config.go index 6712222e8..7718181d7 100644 --- a/hscontrol/types/config.go +++ b/hscontrol/types/config.go @@ -11,6 +11,7 @@ import ( "net/url" "os" "slices" + "strconv" "strings" "sync" "time" @@ -224,6 +225,61 @@ type PostgresConfig struct { ConnMaxIdleTimeSecs int } +// DSN returns the connection string for this configuration as a +// postgres:// URL, the form lib/pq and pgx both accept. Building it with +// net/url escapes every component, so a password (or host, user or +// database name) containing spaces, quotes or other reserved characters +// is passed through intact instead of truncating the DSN or injecting +// extra parameters. A Host starting with "/" is a Unix socket directory +// and travels as the host query parameter, as the URL form requires. +// +// Ssl keeps its historical meaning: "true" leaves the driver default, +// "false" sets sslmode=disable, and any other value is used as sslmode. +func (cfg PostgresConfig) DSN() string { + u := url.URL{ + Scheme: "postgres", + Path: "/" + cfg.Name, + } + + switch { + case cfg.Pass != "": + u.User = url.UserPassword(cfg.User, cfg.Pass) + case cfg.User != "": + u.User = url.User(cfg.User) + } + + query := url.Values{} + port := strconv.Itoa(cfg.Port) + + switch { + case strings.HasPrefix(cfg.Host, "/"): + query.Set("host", cfg.Host) + + if cfg.Port != 0 { + query.Set("port", port) + } + case cfg.Port != 0: + u.Host = net.JoinHostPort(cfg.Host, port) + case strings.Contains(cfg.Host, ":"): + u.Host = "[" + cfg.Host + "]" // bare IPv6 literal + default: + u.Host = cfg.Host + } + + sslEnabled, err := strconv.ParseBool(cfg.Ssl) + + switch { + case err == nil && !sslEnabled: + query.Set("sslmode", "disable") + case err != nil && cfg.Ssl != "": + query.Set("sslmode", cfg.Ssl) + } + + u.RawQuery = query.Encode() + + return u.String() +} + type GormConfig struct { Debug bool SlowThreshold time.Duration diff --git a/hscontrol/types/config_test.go b/hscontrol/types/config_test.go index bb2740ae6..dee5eeb7e 100644 --- a/hscontrol/types/config_test.go +++ b/hscontrol/types/config_test.go @@ -5,6 +5,7 @@ import ( "fmt" "io/fs" "net/netip" + "net/url" "os" "path/filepath" "strings" @@ -1478,3 +1479,97 @@ tls_letsencrypt_cache_dir: /var/lib/headscale/.cache }) } } + +func TestPostgresConfigDSN(t *testing.T) { + tests := []struct { + name string + cfg PostgresConfig + host string + port string + user string + pass string + path string + want map[string]string // expected query parameters + }{ + //nolint:gosec // test fixture, not a credential + { + name: "ssl false disables sslmode", + cfg: PostgresConfig{ + Host: "localhost", + Port: 5432, + Name: "headscale", + User: "hs", + Pass: "secret", + Ssl: "false", + }, + host: "localhost", port: "5432", user: "hs", pass: "secret", path: "/headscale", + want: map[string]string{"sslmode": "disable"}, + }, + { + name: "ssl true leaves the driver default", + cfg: PostgresConfig{Host: "db", Name: "headscale", User: "hs", Ssl: "true"}, + host: "db", user: "hs", path: "/headscale", + want: map[string]string{}, + }, + { + name: "raw sslmode is passed through", + cfg: PostgresConfig{Host: "db", Name: "headscale", User: "hs", Ssl: "verify-full"}, + host: "db", user: "hs", path: "/headscale", + want: map[string]string{"sslmode": "verify-full"}, + }, + // Regression: the DSN used to be concatenated, so a space, quote + // or backslash in the password truncated it or injected parameters. + //nolint:gosec // test fixture, not a credential + { + name: "password with reserved characters survives a round trip", + cfg: PostgresConfig{ + Host: "db", + Name: "head scale", + User: "h's", + Pass: `p a'ss\w sslmode=disable #?&@`, + }, + host: "db", user: "h's", pass: `p a'ss\w sslmode=disable #?&@`, path: "/head scale", + want: map[string]string{}, + }, + { + name: "unix socket directory goes in the host parameter", + cfg: PostgresConfig{ + Host: "/var/run/postgresql", + Port: 5433, + Name: "headscale", + User: "hs", + }, + user: "hs", path: "/headscale", + want: map[string]string{"host": "/var/run/postgresql", "port": "5433"}, + }, + { + name: "ipv6 host is bracketed", + cfg: PostgresConfig{Host: "::1", Port: 5432, Name: "headscale", User: "hs"}, + host: "::1", port: "5432", user: "hs", path: "/headscale", + want: map[string]string{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + u, err := url.Parse(tt.cfg.DSN()) + require.NoError(t, err) + + assert.Equal(t, "postgres", u.Scheme) + assert.Equal(t, tt.host, u.Hostname()) + assert.Equal(t, tt.port, u.Port()) + assert.Equal(t, tt.user, u.User.Username()) + assert.Equal(t, tt.path, u.Path) + + pass, _ := u.User.Password() + assert.Equal(t, tt.pass, pass) + + query := u.Query() + assert.Len(t, query, len(tt.want)) + + for k, v := range tt.want { + assert.Equal(t, v, query.Get(k), k) + } + }) + } +}