Skip to content

Commit 88ac6e6

Browse files
cbandycbandy
committed
Remove invalid characters from a Postgres user spec
This uses a Postgres SQL parser to remove the PASSWORD option and any SQL comments. These values will be rejected in the future. Issue: PGO-1094
1 parent 26bb7c8 commit 88ac6e6

File tree

4 files changed

+62
-3
lines changed

4 files changed

+62
-3
lines changed

‎go.mod

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ require (
1010
github.com/google/uuid v1.3.1
1111
github.com/onsi/ginkgo/v2 v2.0.0
1212
github.com/onsi/gomega v1.18.1
13+
github.com/pganalyze/pg_query_go/v5 v5.1.0
1314
github.com/pkg/errors v0.9.1
1415
github.com/sirupsen/logrus v1.9.3
1516
github.com/xdg-go/stringprep v1.0.2

‎go.sum

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -401,6 +401,8 @@ github.com/opentracing/opentracing-go v1.1.0/go.mod h1:UkNAQd3GIcIGf0SeVgPpRdFSt
401401
github.com/pascaldekloe/goe v0.0.0-20180627143212-57f6aae5913c/go.mod h1:lzWF7FIEvWOWxwDKqyGYQf6ZUaNfKdP144TG7ZOy1lc=
402402
github.com/pelletier/go-toml v1.2.0/go.mod h1:5z9KED0ma1S8pY6P1sdut58dfprrGBbd/94hg7ilaic=
403403
github.com/peterbourgon/diskv v2.0.1+incompatible/go.mod h1:uqqh8zWWbv1HBMNONnaR/tNboyR3/BZd58JJSHlUSCU=
404+
github.com/pganalyze/pg_query_go/v5 v5.1.0 h1:MlxQqHZnvA3cbRQYyIrjxEjzo560P6MyTgtlaf3pmXg=
405+
github.com/pganalyze/pg_query_go/v5 v5.1.0/go.mod h1:FsglvxidZsVN+Ltw3Ai6nTgPVcK2BPukH3jCDEqc1Ug=
404406
github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
405407
github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
406408
github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4=
@@ -956,6 +958,7 @@ google.golang.org/protobuf v1.25.0/go.mod h1:9JNX74DMeImyA3h4bdi1ymwjUzf21/xIlba
956958
google.golang.org/protobuf v1.26.0-rc.1/go.mod h1:jlhhOSvTdKEhbULTjvd4ARK9grFBp09yW+WbY/TyQbw=
957959
google.golang.org/protobuf v1.26.0/go.mod h1:9q0QmTI4eRPtz6boOQmLYwt+qCgq0jsYwAQnmE0givc=
958960
google.golang.org/protobuf v1.27.1/go.mod h1:9q0QmTI4eRPtz6boOQmLYwt+qCgq0jsYwAQnmE0givc=
961+
google.golang.org/protobuf v1.31.0/go.mod h1:HV8QOd/L58Z+nl8r43ehVNZIU/HEI6OcFqwMG9pJV4I=
959962
google.golang.org/protobuf v1.33.0 h1:uNO2rsAINq/JlFpSdYEKIZ0uKD/R9cpdv0T+yoGwGmI=
960963
google.golang.org/protobuf v1.33.0/go.mod h1:c6P6GXX6sHbq/GpV6MGZEdwhWPcYBgnhAHhKbcUYpos=
961964
gopkg.in/alecthomas/kingpin.v2 v2.2.6/go.mod h1:FMv+mEhP44yOT+4EoQTLFTRgOQ1FBLkstjWtayDeSgw=

‎internal/postgres/users.go

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,43 @@ import (
1919
"bytes"
2020
"context"
2121
"encoding/json"
22+
"strings"
23+
24+
pg_query "github.com/pganalyze/pg_query_go/v5"
2225

2326
"github.com/crunchydata/postgres-operator/internal/logging"
2427
"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
2528
)
2629

30+
func sanitizeAlterRoleOptions(options string) string {
31+
const AlterRolePrefix = `ALTER ROLE "any" WITH `
32+
33+
// Parse the options and discard them completely when incoherent.
34+
parsed, err := pg_query.Parse(AlterRolePrefix + options)
35+
if err != nil || len(parsed.GetStmts()) != 1 {
36+
return ""
37+
}
38+
39+
// Rebuild the options list without invalid options. TODO(go1.21) TODO(slices)
40+
orig := parsed.GetStmts()[0].GetStmt().GetAlterRoleStmt().GetOptions()
41+
next := make([]*pg_query.Node, 0, len(orig))
42+
for i, option := range orig {
43+
if strings.EqualFold(option.GetDefElem().GetDefname(), "password") {
44+
continue
45+
}
46+
next = append(next, orig[i])
47+
}
48+
if len(next) > 0 {
49+
parsed.GetStmts()[0].GetStmt().GetAlterRoleStmt().Options = next
50+
} else {
51+
return ""
52+
}
53+
54+
// Turn the modified statement back into SQL and remove the ALTER ROLE portion.
55+
sql, _ := pg_query.Deparse(parsed)
56+
return strings.TrimPrefix(sql, AlterRolePrefix)
57+
}
58+
2759
// WriteUsersInPostgreSQL calls exec to create users that do not exist in
2860
// PostgreSQL. Once they exist, it updates their options and passwords and
2961
// grants them access to their specified databases. The databases must already
@@ -56,7 +88,7 @@ CREATE TEMPORARY TABLE input (id serial, data json);
5688
spec := users[i]
5789

5890
databases := spec.Databases
59-
options := spec.Options
91+
options := sanitizeAlterRoleOptions(spec.Options)
6092

6193
// The "postgres" user must always be a superuser that can login to
6294
// the "postgres" database.

‎internal/postgres/users_test.go

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,24 @@ import (
2828
"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
2929
)
3030

31+
func TestSanitizeAlterRoleOptions(t *testing.T) {
32+
assert.Equal(t, sanitizeAlterRoleOptions(""), "")
33+
assert.Equal(t, sanitizeAlterRoleOptions(" login other stuff"), "",
34+
"expected non-options to be removed")
35+
36+
t.Run("RemovesPassword", func(t *testing.T) {
37+
assert.Equal(t, sanitizeAlterRoleOptions("password 'anything'"), "")
38+
assert.Equal(t, sanitizeAlterRoleOptions("password $wild$ dollar quoting $wild$ login"), "LOGIN")
39+
assert.Equal(t, sanitizeAlterRoleOptions(" login password '' replication "), "LOGIN REPLICATION")
40+
})
41+
42+
t.Run("RemovesComments", func(t *testing.T) {
43+
assert.Equal(t, sanitizeAlterRoleOptions("login -- asdf"), "LOGIN")
44+
assert.Equal(t, sanitizeAlterRoleOptions("login /*"), "")
45+
assert.Equal(t, sanitizeAlterRoleOptions("login /* createdb */ createrole"), "LOGIN CREATEROLE")
46+
})
47+
}
48+
3149
func TestWriteUsersInPostgreSQL(t *testing.T) {
3250
ctx := context.Background()
3351

@@ -108,8 +126,9 @@ COMMIT;`))
108126
assert.Assert(t, cmp.Contains(string(b), `
109127
\copy input (data) from stdin with (format text)
110128
{"databases":["db1"],"options":"","username":"user-no-options","verifier":""}
111-
{"databases":null,"options":"some options here","username":"user-no-databases","verifier":""}
129+
{"databases":null,"options":"CREATEDB CREATEROLE","username":"user-no-databases","verifier":""}
112130
{"databases":null,"options":"","username":"user-with-verifier","verifier":"some$verifier"}
131+
{"databases":null,"options":"LOGIN","username":"user-invalid-options","verifier":""}
113132
\.
114133
`))
115134
return nil
@@ -123,11 +142,15 @@ COMMIT;`))
123142
},
124143
{
125144
Name: "user-no-databases",
126-
Options: "some options here",
145+
Options: "createdb createrole",
127146
},
128147
{
129148
Name: "user-with-verifier",
130149
},
150+
{
151+
Name: "user-invalid-options",
152+
Options: "login password 'doot' --",
153+
},
131154
},
132155
map[string]string{
133156
"no-user": "ignored",

0 commit comments

Comments
 (0)