From 514836c540723f53cfc080dec4a28a234798eec1 Mon Sep 17 00:00:00 2001 From: Moritz Poldrack Date: Sat, 21 Oct 2023 22:18:44 +0200 Subject: [PATCH 1/5] refactor Postgres connection string builder to use net.URL URLs are notoriously difficult to work with, as the correct definition is so broad as to be functionally useless, having unescaped special characters can pose a security risk, or parsing issues. Use the net.URL type to ensure everything is escaped as required. This also fixes the existing unit tests which were invalid URLs and likely wrong in the first place, due to them combining query and database name. --- modules/setting/database.go | 22 +++++++++++++++------- modules/setting/database_test.go | 4 ++-- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/modules/setting/database.go b/modules/setting/database.go index 709655368c67b..207d298267930 100644 --- a/modules/setting/database.go +++ b/modules/setting/database.go @@ -155,14 +155,22 @@ func parsePostgreSQLHostPort(info string) (string, string) { func getPostgreSQLConnectionString(dbHost, dbUser, dbPasswd, dbName, dbParam, dbsslMode string) (connStr string) { host, port := parsePostgreSQLHostPort(dbHost) - if host[0] == '/' { // looks like a unix socket - connStr = fmt.Sprintf("postgres://%s:%s@:%s/%s%ssslmode=%s&host=%s", - url.PathEscape(dbUser), url.PathEscape(dbPasswd), port, dbName, dbParam, dbsslMode, host) - } else { - connStr = fmt.Sprintf("postgres://%s:%s@%s:%s/%s%ssslmode=%s", - url.PathEscape(dbUser), url.PathEscape(dbPasswd), host, port, dbName, dbParam, dbsslMode) + connURL := url.URL{ + Scheme: "postgres", + User: url.UserPassword(dbUser, dbPasswd), + Host: net.JoinHostPort(host, port), + Path: dbName, + OmitHost: false, + RawQuery: dbParam, + } + query := connURL.Query() + if dbHost[0] == '/' { // looks like a unix socket + query.Add("host", dbHost) + connURL.Host = ":" + port } - return connStr + query.Set("sslmode", dbsslMode) + connURL.RawQuery = query.Encode() + return connURL.String() } // ParseMSSQLHostPort splits the host into host and port diff --git a/modules/setting/database_test.go b/modules/setting/database_test.go index 481ca969b1fb2..10451b1f30be0 100644 --- a/modules/setting/database_test.go +++ b/modules/setting/database_test.go @@ -72,7 +72,7 @@ func Test_getPostgreSQLConnectionString(t *testing.T) { Name: "gitea", Param: "", SSLMode: "false", - Output: "postgres://testuser:space%20space%20%21%23$%25%5E%5E%25%5E%60%60%60-=%3F=@:5432/giteasslmode=false&host=/tmp/pg.sock", + Output: "postgres://testuser:space%20space%20%21%23$%25%5E%5E%25%5E%60%60%60-=%3F=@:5432/gitea?host=%2Ftmp%2Fpg.sock&sslmode=false", }, { Host: "localhost", @@ -82,7 +82,7 @@ func Test_getPostgreSQLConnectionString(t *testing.T) { Name: "gitea", Param: "", SSLMode: "true", - Output: "postgres://pgsqlusername:I%20love%20Gitea%21@localhost:5432/giteasslmode=true", + Output: "postgres://pgsqlusername:I%20love%20Gitea%21@localhost:5432/gitea?sslmode=true", }, } From 81c0601fef9d5d03e78b436b146fae50a93cd72b Mon Sep 17 00:00:00 2001 From: Moritz Poldrack Date: Sat, 21 Oct 2023 22:09:21 +0200 Subject: [PATCH 2/5] refactor Postgres host/port parser to use stdlib splitter The existing solution is rather inelegant and does not use a stdlib function that does basically the same thing. The test has been adapted to allow for unbracketed IPv6 addresses, as these are added by net.JoinHostPort. --- modules/setting/database.go | 21 +++++++++++++-------- modules/setting/database_test.go | 29 ++++++++++++++++------------- 2 files changed, 29 insertions(+), 21 deletions(-) diff --git a/modules/setting/database.go b/modules/setting/database.go index 207d298267930..2fee06b1d70e3 100644 --- a/modules/setting/database.go +++ b/modules/setting/database.go @@ -6,6 +6,7 @@ package setting import ( "errors" "fmt" + "net" "net/url" "os" "path" @@ -135,15 +136,19 @@ func DBConnStr() (string, error) { // parsePostgreSQLHostPort parses given input in various forms defined in // https://www.postgresql.org/docs/current/static/libpq-connect.html#LIBPQ-CONNSTRING // and returns proper host and port number. -func parsePostgreSQLHostPort(info string) (string, string) { - host, port := "127.0.0.1", "5432" - if strings.Contains(info, ":") && !strings.HasSuffix(info, "]") { - idx := strings.LastIndex(info, ":") - host = info[:idx] - port = info[idx+1:] - } else if len(info) > 0 { - host = info +func parsePostgreSQLHostPort(info string) (host, port string) { + host = info + h, p, err := net.SplitHostPort(info) + if err == nil { + host, port = h, p } + + // if it's an IPv6 address, remove the wrapper + if strings.HasPrefix(host, "[") && strings.HasSuffix(host, "]") { + host = host[1 : len(host)-1] + } + + // set fallback values if host == "" { host = "127.0.0.1" } diff --git a/modules/setting/database_test.go b/modules/setting/database_test.go index 10451b1f30be0..85271c36cb388 100644 --- a/modules/setting/database_test.go +++ b/modules/setting/database_test.go @@ -10,46 +10,49 @@ import ( ) func Test_parsePostgreSQLHostPort(t *testing.T) { - tests := []struct { + tests := map[string]struct { HostPort string Host string Port string }{ - { + "host-port": { HostPort: "127.0.0.1:1234", Host: "127.0.0.1", Port: "1234", }, - { + "no-port": { HostPort: "127.0.0.1", Host: "127.0.0.1", Port: "5432", }, - { + "ipv6-port": { HostPort: "[::1]:1234", - Host: "[::1]", + Host: "::1", Port: "1234", }, - { + "ipv6-no-port": { HostPort: "[::1]", - Host: "[::1]", + Host: "::1", Port: "5432", }, - { + "unix-socket": { HostPort: "/tmp/pg.sock:1234", Host: "/tmp/pg.sock", Port: "1234", }, - { + "unix-socket-no-port": { HostPort: "/tmp/pg.sock", Host: "/tmp/pg.sock", Port: "5432", }, } - for _, test := range tests { - host, port := parsePostgreSQLHostPort(test.HostPort) - assert.Equal(t, test.Host, host) - assert.Equal(t, test.Port, port) + for k, test := range tests { + t.Run(k, func(t *testing.T) { + t.Log(test.HostPort) + host, port := parsePostgreSQLHostPort(test.HostPort) + assert.Equal(t, test.Host, host) + assert.Equal(t, test.Port, port) + }) } } From 5dd4cbc5419e19f95e799891e8f9d9d2c54751b3 Mon Sep 17 00:00:00 2001 From: Moritz Poldrack Date: Sat, 21 Oct 2023 22:30:19 +0200 Subject: [PATCH 3/5] add mention of optional socket port for postgres Fixes: #24552 --- docs/content/administration/config-cheat-sheet.en-us.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/content/administration/config-cheat-sheet.en-us.md b/docs/content/administration/config-cheat-sheet.en-us.md index 617715fbaa12e..715bd6d325f59 100644 --- a/docs/content/administration/config-cheat-sheet.en-us.md +++ b/docs/content/administration/config-cheat-sheet.en-us.md @@ -424,7 +424,7 @@ The following configuration set `Content-Type: application/vnd.android.package-a ## Database (`database`) - `DB_TYPE`: **mysql**: The database type in use \[mysql, postgres, mssql, sqlite3\]. -- `HOST`: **127.0.0.1:3306**: Database host address and port or absolute path for unix socket \[mysql, postgres\] (ex: /var/run/mysqld/mysqld.sock). +- `HOST`: **127.0.0.1:3306**: Database host address and port or absolute path for unix socket \[mysql, postgres[^1]\] (ex: /var/run/mysqld/mysqld.sock). - `NAME`: **gitea**: Database name. - `USER`: **root**: Database username. - `PASSWD`: **_empty_**: Database user password. Use \`your password\` or """your password""" for quoting if you use special characters in the password. @@ -455,6 +455,8 @@ The following configuration set `Content-Type: application/vnd.android.package-a - `CONN_MAX_LIFETIME` **0 or 3s**: Sets the maximum amount of time a DB connection may be reused - default is 0, meaning there is no limit (except on MySQL where it is 3s - see #6804 & #7071). - `AUTO_MIGRATION` **true**: Whether execute database models migrations automatically. +[^1]: It may be necessary to specify a hostport even when listening on a unix socket, as the port is part of the socket name. see [#24552](https://github.com/go-gitea/gitea/issues/24552#issuecomment-1681649367) for additional details. + Please see #8540 & #8273 for further discussion of the appropriate values for `MAX_OPEN_CONNS`, `MAX_IDLE_CONNS` & `CONN_MAX_LIFETIME` and their relation to port exhaustion. From fa39767a8d025a4e5f430e6c7cfc1f0d9f818584 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 2 Nov 2023 01:13:13 +0800 Subject: [PATCH 4/5] Update database.go --- modules/setting/database.go | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/modules/setting/database.go b/modules/setting/database.go index 2fee06b1d70e3..951ffd98367cd 100644 --- a/modules/setting/database.go +++ b/modules/setting/database.go @@ -137,17 +137,16 @@ func DBConnStr() (string, error) { // https://www.postgresql.org/docs/current/static/libpq-connect.html#LIBPQ-CONNSTRING // and returns proper host and port number. func parsePostgreSQLHostPort(info string) (host, port string) { - host = info - h, p, err := net.SplitHostPort(info) - if err == nil { + if h, p, err := net.SplitHostPort(info); err == nil { host, port = h, p + } else { + // treat the "info" as "host", if it's an IPv6 address, remove the wrapper + host = info + if strings.HasPrefix(host, "[") && strings.HasSuffix(host, "]") { + host = host[1 : len(host)-1] + } } - - // if it's an IPv6 address, remove the wrapper - if strings.HasPrefix(host, "[") && strings.HasSuffix(host, "]") { - host = host[1 : len(host)-1] - } - + // set fallback values if host == "" { host = "127.0.0.1" From 00b601ed9c70f3d1f5daf22552b211a7bb01ba97 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 2 Nov 2023 01:28:15 +0800 Subject: [PATCH 5/5] Update database.go --- modules/setting/database.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/setting/database.go b/modules/setting/database.go index 951ffd98367cd..aa42f506bc51b 100644 --- a/modules/setting/database.go +++ b/modules/setting/database.go @@ -146,7 +146,7 @@ func parsePostgreSQLHostPort(info string) (host, port string) { host = host[1 : len(host)-1] } } - + // set fallback values if host == "" { host = "127.0.0.1"