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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

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)

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 {

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

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)
+			}
+		})
+	}
+}