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