From 74b564ff9a6652a296e1d5c315643d39de7df4ed Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Thu, 27 Feb 2025 14:37:07 -0600 Subject: [PATCH 1/2] Add a validated field for Postgres HBA rules These structured fields are easier and safer to use than raw HBA records. The validation rules of Kubernetes 1.29 (Beta in 1.25) allow for this kind of structure. Co-authored-by: TJ Moore Issue: PGO-2263 --- ...ator.crunchydata.com_postgresclusters.yaml | 80 +++++++++++++++ .../controller/postgrescluster/postgres.go | 46 ++++++++- .../postgrescluster/postgres_test.go | 93 ++++++++++++++++- internal/pgbouncer/postgres.go | 4 +- internal/pgbouncer/postgres_test.go | 4 +- internal/pgmonitor/postgres.go | 6 +- internal/pgmonitor/postgres_test.go | 6 +- internal/postgres/hba.go | 84 +++++++++++++--- internal/postgres/hba_test.go | 62 +++++++++--- .../validation/postgrescluster_test.go | 99 +++++++++++++++++++ .../v1beta1/postgres_types.go | 73 ++++++++++++++ .../v1beta1/postgrescluster_types.go | 3 + .../v1beta1/zz_generated.deepcopy.go | 75 ++++++++++++++ 13 files changed, 591 insertions(+), 44 deletions(-) diff --git a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml index abd625f827..8b8bfee823 100644 --- a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml +++ b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml @@ -39,6 +39,86 @@ spec: spec: description: PostgresClusterSpec defines the desired state of PostgresCluster properties: + authentication: + properties: + rules: + description: 'More info: https://www.postgresql.org/docs/current/auth-pg-hba-conf.html' + items: + properties: + connection: + description: |- + The connection transport this rule matches. Typical values are: + 1. "host" for network connections that may or may not be encrypted. + 2. "hostssl" for network connections encrypted using TLS. + 3. "hostgssenc" for network connections encrypted using GSSAPI. + maxLength: 20 + minLength: 1 + pattern: ^[-a-z0-9]+$ + type: string + databases: + description: Which databases this rule matches. When omitted + or empty, this rule matches all databases. + items: + maxLength: 63 + minLength: 1 + type: string + maxItems: 20 + type: array + x-kubernetes-list-type: atomic + hba: + description: One line of the "pg_hba.conf" file. Changes + to this value will be automatically reloaded without validation. + maxLength: 100 + minLength: 1 + pattern: ^[[:print:]]+$ + type: string + x-kubernetes-validations: + - message: cannot include other files + rule: '!self.trim().startsWith("include")' + method: + description: |- + The authentication method to use when a connection matches this rule. + The special value "reject" refuses connections that match this rule. + More info: https://www.postgresql.org/docs/current/auth-methods.html + maxLength: 20 + minLength: 1 + pattern: ^[-a-z0-9]+$ + type: string + x-kubernetes-validations: + - message: the "trust" method is unsafe + rule: self != "trust" + options: + additionalProperties: + anyOf: + - type: integer + - type: string + x-kubernetes-int-or-string: true + maxProperties: 20 + type: object + x-kubernetes-map-type: atomic + users: + description: Which user names this rule matches. When omitted + or empty, this rule matches all users. + items: + maxLength: 63 + minLength: 1 + type: string + maxItems: 20 + type: array + x-kubernetes-list-type: atomic + type: object + x-kubernetes-map-type: atomic + x-kubernetes-validations: + - message: '"hba" cannot be combined with other fields' + rule: 'has(self.hba) ? !has(self.connection) && !has(self.databases) + && !has(self.method) && !has(self.options) && !has(self.users) + : true' + - message: '"connection" and "method" are required' + rule: 'has(self.hba) ? true : has(self.connection) && has(self.method)' + maxItems: 10 + type: array + x-kubernetes-list-type: atomic + type: object backups: description: PostgreSQL backup configuration properties: diff --git a/internal/controller/postgrescluster/postgres.go b/internal/controller/postgrescluster/postgres.go index a2f6fb8611..74547f5d5e 100644 --- a/internal/controller/postgrescluster/postgres.go +++ b/internal/controller/postgrescluster/postgres.go @@ -42,12 +42,40 @@ import ( "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" ) +// generatePostgresHBA converts one API rule into a structured HBA rule that +// safely formats its values. +func (*Reconciler) generatePostgresHBA(spec *v1beta1.PostgresHBARule) *postgres.HostBasedAuthentication { + if spec == nil { + return nil + } + + result := postgres.NewHBA() + result.Origin(spec.Connection) + result.Method(spec.Method) + + if len(spec.Databases) > 0 { + result.Databases(spec.Databases[0], spec.Databases[1:]...) + } + if len(spec.Users) > 0 { + result.Users(spec.Users[0], spec.Users[1:]...) + } + if len(spec.Options) > 0 { + opts := make(map[string]string, len(spec.Options)) + for k, v := range spec.Options { + opts[k] = v.String() + } + result.Options(opts) + } + + return result +} + // generatePostgresHBAs produces the HBA rules for cluster that incorporates, // from highest to lowest precedence: // 1. mandatory rules determined by controllers // 2. rules in cluster.spec.patroni.dynamicConfiguration // 3. default rules, when none were in cluster.spec -func (*Reconciler) generatePostgresHBAs( +func (r *Reconciler) generatePostgresHBAs( ctx context.Context, cluster *v1beta1.PostgresCluster, ) *postgres.OrderedHBAs { builtin := postgres.NewHBAs() @@ -58,13 +86,25 @@ func (*Reconciler) generatePostgresHBAs( // so connections are matched against them first. result := new(postgres.OrderedHBAs) result.Append(builtin.Mandatory...) + mandatory := result.Length() + + // Append any rules specified in the Authentication section. + // These take precedence over any in the Patroni section. + if authn := cluster.Spec.Authentication; authn != nil { + for _, in := range authn.Rules { + if len(in.HBA) > 0 { + result.AppendUnstructured(in.HBA) + } else { + result.Append(r.generatePostgresHBA(&in.PostgresHBARule)) + } + } + } // Append any rules specified in the Patroni section. - before := result.Length() result.AppendUnstructured(patroni.PostgresHBAs(cluster.Spec.Patroni)...) // When there are no specified rules, include the recommended defaults. - if result.Length() == before { + if result.Length() == mandatory { result.Append(builtin.Default...) } diff --git a/internal/controller/postgrescluster/postgres_test.go b/internal/controller/postgrescluster/postgres_test.go index 4f967d1088..edb203ecd0 100644 --- a/internal/controller/postgrescluster/postgres_test.go +++ b/internal/controller/postgrescluster/postgres_test.go @@ -34,6 +34,40 @@ import ( "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" ) +func TestGeneratePostgresHBA(t *testing.T) { + reconciler := &Reconciler{} + + assert.Assert(t, reconciler.generatePostgresHBA(nil) == nil, + "expected nil to return nil") + + for _, tt := range []struct { + rule, expected string + }{ + { + rule: `{ connection: host, method: scram }`, + expected: `"host" all all all "scram"`, + }, + { + rule: `{ connection: local, method: peer, databases: [one, two] }`, + expected: `"local" "one","two" all all "peer"`, + }, + { + rule: `{ connection: local, method: peer, users: [alice, bob] }`, + expected: `"local" all "alice","bob" all "peer"`, + }, + { + rule: `{ connection: hostssl, method: md5, options: { clientcert: verify-ca } }`, + expected: `"hostssl" all all all "md5" "clientcert"="verify-ca"`, + }, + } { + var rule *v1beta1.PostgresHBARule + require.UnmarshalInto(t, &rule, tt.rule) + + hba := reconciler.generatePostgresHBA(rule) + assert.Equal(t, hba.String(), tt.expected, "\n%#v", rule) + } +} + func TestGeneratePostgresHBAs(t *testing.T) { ctx := context.Background() reconciler := &Reconciler{} @@ -50,12 +84,35 @@ func TestGeneratePostgresHBAs(t *testing.T) { assert.Assert(t, len(required) > 0, "expected at least one mandatory rule") + t.Run("Authentication", func(t *testing.T) { + cluster := v1beta1.NewPostgresCluster() + require.UnmarshalInto(t, &cluster.Spec.Authentication, `{ + rules: [ + { connection: host, method: scram }, + { connection: local, method: peer, users: [alice, bob] }, + ], + }`) + + result := reconciler.generatePostgresHBAs(ctx, cluster).AsStrings() + assert.Assert(t, cmp.Len(result, len(required)+2), + "expected two rules from the Authentication section and no defaults") + + // mandatory rules should be first + assert.DeepEqual(t, result[:len(required)], required) + + // specified rules should be last and in their original order + assert.DeepEqual(t, result[len(required):], []string{ + `"host" all all all "scram"`, + `"local" all "alice","bob" all "peer"`, + }) + }) + t.Run("Patroni", func(t *testing.T) { cluster := v1beta1.NewPostgresCluster() require.UnmarshalInto(t, &cluster.Spec.Patroni, `{ - dynamicConfiguration: { - postgresql: { pg_hba: [ "first custom", "another" ] }, - }, + dynamicConfiguration: { + postgresql: { pg_hba: [ "first custom", "another" ] }, + }, }`) result := reconciler.generatePostgresHBAs(ctx, cluster).AsStrings() @@ -68,6 +125,36 @@ func TestGeneratePostgresHBAs(t *testing.T) { // specified rules should be last and in their original order assert.DeepEqual(t, result[len(required):], []string{`first custom`, `another`}) }) + + t.Run("Precedence", func(t *testing.T) { + cluster := v1beta1.NewPostgresCluster() + require.UnmarshalInto(t, &cluster.Spec.Authentication, `{ + rules: [ + { connection: host, method: scram }, + { connection: local, method: peer, users: [alice, bob] }, + ], + }`) + require.UnmarshalInto(t, &cluster.Spec.Patroni, `{ + dynamicConfiguration: { + postgresql: { pg_hba: [ "another" ] }, + }, + }`) + + result := reconciler.generatePostgresHBAs(ctx, cluster).AsStrings() + assert.Assert(t, cmp.Len(result, len(required)+2+1), + "expected two rules from the Authentication section"+ + " plus one from the Patroni section") + + // mandatory rules should be first + assert.DeepEqual(t, result[:len(required)], required) + + // specified rules are next, no defaults + assert.DeepEqual(t, result[len(required):], []string{ + `"host" all all all "scram"`, // Authentication + `"local" all "alice","bob" all "peer"`, // Authentication + `another`, // Patroni + }) + }) } func TestGeneratePostgresParameters(t *testing.T) { diff --git a/internal/pgbouncer/postgres.go b/internal/pgbouncer/postgres.go index d7d2bae5cf..87b915caac 100644 --- a/internal/pgbouncer/postgres.go +++ b/internal/pgbouncer/postgres.go @@ -225,7 +225,7 @@ func postgresqlHBAs() []*postgres.HostBasedAuthentication { // - https://www.postgresql.org/docs/current/auth-password.html return []*postgres.HostBasedAuthentication{ - postgres.NewHBA().User(PostgresqlUser).TLS().Method("scram-sha-256"), - postgres.NewHBA().User(PostgresqlUser).TCP().Method("reject"), + postgres.NewHBA().Users(PostgresqlUser).TLS().Method("scram-sha-256"), + postgres.NewHBA().Users(PostgresqlUser).TCP().Method("reject"), } } diff --git a/internal/pgbouncer/postgres_test.go b/internal/pgbouncer/postgres_test.go index eb3bb65818..3a4c1c3ef8 100644 --- a/internal/pgbouncer/postgres_test.go +++ b/internal/pgbouncer/postgres_test.go @@ -186,6 +186,6 @@ COMMIT;`)) func TestPostgreSQLHBAs(t *testing.T) { rules := postgresqlHBAs() assert.Equal(t, len(rules), 2) - assert.Equal(t, rules[0].String(), `hostssl all "_crunchypgbouncer" all scram-sha-256`) - assert.Equal(t, rules[1].String(), `host all "_crunchypgbouncer" all reject`) + assert.Equal(t, rules[0].String(), `hostssl all "_crunchypgbouncer" all "scram-sha-256"`) + assert.Equal(t, rules[1].String(), `host all "_crunchypgbouncer" all "reject"`) } diff --git a/internal/pgmonitor/postgres.go b/internal/pgmonitor/postgres.go index 08a428d465..1d7817c9a3 100644 --- a/internal/pgmonitor/postgres.go +++ b/internal/pgmonitor/postgres.go @@ -27,9 +27,9 @@ func PostgreSQLHBAs(ctx context.Context, inCluster *v1beta1.PostgresCluster, out if ExporterEnabled(ctx, inCluster) || feature.Enabled(ctx, feature.OpenTelemetryMetrics) { // Limit the monitoring user to local connections using SCRAM. outHBAs.Mandatory = append(outHBAs.Mandatory, - postgres.NewHBA().TCP().User(MonitoringUser).Method("scram-sha-256").Network("127.0.0.0/8"), - postgres.NewHBA().TCP().User(MonitoringUser).Method("scram-sha-256").Network("::1/128"), - postgres.NewHBA().TCP().User(MonitoringUser).Method("reject")) + postgres.NewHBA().TCP().Users(MonitoringUser).Method("scram-sha-256").Network("127.0.0.0/8"), + postgres.NewHBA().TCP().Users(MonitoringUser).Method("scram-sha-256").Network("::1/128"), + postgres.NewHBA().TCP().Users(MonitoringUser).Method("reject")) } } diff --git a/internal/pgmonitor/postgres_test.go b/internal/pgmonitor/postgres_test.go index 3b6bff58de..4c1acc1dcf 100644 --- a/internal/pgmonitor/postgres_test.go +++ b/internal/pgmonitor/postgres_test.go @@ -39,9 +39,9 @@ func TestPostgreSQLHBA(t *testing.T) { PostgreSQLHBAs(ctx, inCluster, &outHBAs) assert.Equal(t, len(outHBAs.Mandatory), 3) - assert.Equal(t, outHBAs.Mandatory[0].String(), `host all "ccp_monitoring" "127.0.0.0/8" scram-sha-256`) - assert.Equal(t, outHBAs.Mandatory[1].String(), `host all "ccp_monitoring" "::1/128" scram-sha-256`) - assert.Equal(t, outHBAs.Mandatory[2].String(), `host all "ccp_monitoring" all reject`) + assert.Equal(t, outHBAs.Mandatory[0].String(), `host all "ccp_monitoring" "127.0.0.0/8" "scram-sha-256"`) + assert.Equal(t, outHBAs.Mandatory[1].String(), `host all "ccp_monitoring" "::1/128" "scram-sha-256"`) + assert.Equal(t, outHBAs.Mandatory[2].String(), `host all "ccp_monitoring" all "reject"`) }) } diff --git a/internal/postgres/hba.go b/internal/postgres/hba.go index 875a476ee2..f4fe83d114 100644 --- a/internal/postgres/hba.go +++ b/internal/postgres/hba.go @@ -6,6 +6,8 @@ package postgres import ( "fmt" + "maps" + "regexp" "slices" "strings" ) @@ -15,15 +17,15 @@ func NewHBAs() HBAs { return HBAs{ Mandatory: []*HostBasedAuthentication{ // The "postgres" superuser must always be able to connect locally. - NewHBA().Local().User("postgres").Method("peer"), + NewHBA().Local().Users("postgres").Method("peer"), // The replication user must always connect over TLS using certificate // authentication. Patroni also connects to the "postgres" database // when calling `pg_rewind`. // - https://www.postgresql.org/docs/current/warm-standby.html#STREAMING-REPLICATION-AUTHENTICATION - NewHBA().TLS().User(ReplicationUser).Method("cert").Replication(), - NewHBA().TLS().User(ReplicationUser).Method("cert").Database("postgres"), - NewHBA().TCP().User(ReplicationUser).Method("reject"), + NewHBA().TLS().Users(ReplicationUser).Method("cert").Replication(), + NewHBA().TLS().Users(ReplicationUser).Method("cert").Databases("postgres"), + NewHBA().TCP().Users(ReplicationUser).Method("reject"), }, Default: []*HostBasedAuthentication{ @@ -50,10 +52,51 @@ func NewHBA() *HostBasedAuthentication { return new(HostBasedAuthentication).AllDatabases().AllNetworks().AllUsers() } +// hbaRegexSpecialCharacters matches a superset of the special characters in +// PostgreSQL [regular expressions] for: +// +// - [HostBasedAuthentication.quoteDatabase] +// - [HostBasedAuthentication.quoteUser] +// +// [regular expressions]: https://www.postgresql.org/docs/current/functions-matching.html#POSIX-SYNTAX-DETAILS +var hbaRegexSpecialCharacters = regexp.MustCompile(`[^\pL\pN_]`) + func (*HostBasedAuthentication) quote(value string) string { return `"` + strings.ReplaceAll(value, `"`, `""`) + `"` } +func (hba *HostBasedAuthentication) quoteDatabase(name string) string { + // Since PostgreSQL 16, a quoted string beginning with slash U+002F is + // interpreted as a regular expression. Express these names as a Postgres + // regex that exactly matches the entire name. + if len(name) > 0 && name[0] == '/' { + name = "/^" + + hbaRegexSpecialCharacters.ReplaceAllStringFunc(name, + func(match string) string { return "[" + match + "]" }) + + "$" + } + + // Quotes indicate the value is NOT a keyword (all, sameuser, etc.) + // and NOT to be expanded as a filename (at sign U+0040). + return hba.quote(name) +} + +func (hba *HostBasedAuthentication) quoteUser(name string) string { + // Since PostgreSQL 16, a quoted string beginning with slash U+002F is + // interpreted as a regular expression. Express these names as a Postgres + // regex that exactly matches the entire name. + if len(name) > 0 && name[0] == '/' { + name = "/^" + + hbaRegexSpecialCharacters.ReplaceAllStringFunc(name, + func(match string) string { return "[" + match + "]" }) + + "$" + } + + // Quotes indicate the value is NOT a keyword (all), NOT a group (plus U+002B), + // and NOT to be expanded as a filename (at sign U+0040). + return hba.quote(name) +} + // AllDatabases makes hba match connections made to any database. func (hba *HostBasedAuthentication) AllDatabases() *HostBasedAuthentication { hba.database = "all" @@ -72,9 +115,12 @@ func (hba *HostBasedAuthentication) AllUsers() *HostBasedAuthentication { return hba } -// Database makes hba match connections made to a specific database. -func (hba *HostBasedAuthentication) Database(name string) *HostBasedAuthentication { - hba.database = hba.quote(name) +// Databases makes hba match connections made to specific databases. +func (hba *HostBasedAuthentication) Databases(name string, names ...string) *HostBasedAuthentication { + hba.database = hba.quoteDatabase(name) + for _, n := range names { + hba.database += "," + hba.quoteDatabase(n) + } return hba } @@ -86,7 +132,12 @@ func (hba *HostBasedAuthentication) Local() *HostBasedAuthentication { // Method specifies the authentication method to use when a connection matches hba. func (hba *HostBasedAuthentication) Method(name string) *HostBasedAuthentication { - hba.method = name + hba.method = hba.quote(name) + return hba +} + +func (hba *HostBasedAuthentication) Origin(name string) *HostBasedAuthentication { + hba.origin = hba.quote(name) return hba } @@ -105,8 +156,8 @@ func (hba *HostBasedAuthentication) NoSSL() *HostBasedAuthentication { // Options specifies any options for the authentication method. func (hba *HostBasedAuthentication) Options(opts map[string]string) *HostBasedAuthentication { hba.options = "" - for k, v := range opts { - hba.options = fmt.Sprintf("%s %s=%s", hba.options, k, hba.quote(v)) + for _, k := range slices.Sorted(maps.Keys(opts)) { + hba.options = fmt.Sprintf("%s %s=%s", hba.options, hba.quote(k), hba.quote(opts[k])) } return hba } @@ -136,9 +187,12 @@ func (hba *HostBasedAuthentication) TCP() *HostBasedAuthentication { return hba } -// User makes hba match connections by a specific user. -func (hba *HostBasedAuthentication) User(name string) *HostBasedAuthentication { - hba.user = hba.quote(name) +// Users makes hba match connections by specific users. +func (hba *HostBasedAuthentication) Users(name string, names ...string) *HostBasedAuthentication { + hba.user = hba.quoteUser(name) + for _, n := range names { + hba.user += "," + hba.quoteUser(n) + } return hba } @@ -175,7 +229,9 @@ func (o *OrderedHBAs) AppendUnstructured(hbas ...string) { // control characters, space, and backslash return r > '~' || r < '!' || r == '\\' }) - if len(hba) > 0 { + + // NOTE: Skipping "include" directives here is a security measure. + if len(hba) > 0 && !strings.HasPrefix(hba, "include") { o.records = append(o.records, hba) } } diff --git a/internal/postgres/hba_test.go b/internal/postgres/hba_test.go index c15cf5cc77..737d530024 100644 --- a/internal/postgres/hba_test.go +++ b/internal/postgres/hba_test.go @@ -30,35 +30,53 @@ func TestNewHBAs(t *testing.T) { hba := NewHBAs() assert.Assert(t, matches(hba.Mandatory, ` -local all "postgres" peer -hostssl replication "_crunchyrepl" all cert -hostssl "postgres" "_crunchyrepl" all cert -host all "_crunchyrepl" all reject +local all "postgres" "peer" +hostssl replication "_crunchyrepl" all "cert" +hostssl "postgres" "_crunchyrepl" all "cert" +host all "_crunchyrepl" all "reject" `)) assert.Assert(t, matches(hba.Default, ` -hostssl all all all md5 +hostssl all all all "md5" `)) } func TestHostBasedAuthentication(t *testing.T) { - assert.Equal(t, `local all "postgres" peer`, - NewHBA().Local().User("postgres").Method("peer").String()) + assert.Equal(t, `local all "postgres","pgo" "peer"`, + NewHBA().Local().Users("postgres", "pgo").Method("peer").String()) - assert.Equal(t, `host all all "::1/128" trust`, + assert.Equal(t, `host all all "::1/128" "trust"`, NewHBA().TCP().Network("::1/128").Method("trust").String()) - assert.Equal(t, `host replication "KD6-3.7" samenet scram-sha-256`, + assert.Equal(t, `host replication "KD6-3.7" samenet "scram-sha-256"`, NewHBA().TCP().SameNetwork().Replication(). - User("KD6-3.7").Method("scram-sha-256"). + Users("KD6-3.7").Method("scram-sha-256"). String()) - assert.Equal(t, `hostssl "data" all all md5 clientcert="verify-ca"`, - NewHBA().TLS().Database("data"). + assert.Equal(t, `hostssl "data","bits" all all "md5" "clientcert"="verify-ca"`, + NewHBA().TLS().Databases("data", "bits"). Method("md5").Options(map[string]string{"clientcert": "verify-ca"}). String()) - assert.Equal(t, `hostnossl all all all reject`, + assert.Equal(t, `hostnossl all all all "reject"`, NewHBA().NoSSL().Method("reject").String()) + + t.Run("OptionsSorted", func(t *testing.T) { + assert.Equal(t, `hostssl all all all "ldap" "ldapbasedn"="dc=example,dc=org" "ldapserver"="example.org"`, + NewHBA().TLS().Method("ldap").Options(map[string]string{ + "ldapserver": "example.org", + "ldapbasedn": "dc=example,dc=org", + }).String()) + }) + + t.Run("SpecialCharactersEscaped", func(t *testing.T) { + // Databases; slash U+002F triggers regex escaping; regex characters themselves do not + assert.Equal(t, `local "/^[/]asdf_[+][?]1234$","/^[/][*][$]$","+*$" all`, + NewHBA().Local().Databases(`/asdf_+?1234`, `/*$`, `+*$`).String()) + + // Users; slash U+002F triggers regex escaping; regex characters themselves do not + assert.Equal(t, `local all "/^[/]asdf_[+][?]1234$","/^[/][*][$]$","+*$"`, + NewHBA().Local().Users(`/asdf_+?1234`, `/*$`, `+*$`).String()) + }) } func TestOrderedHBAs(t *testing.T) { @@ -89,7 +107,7 @@ func TestOrderedHBAs(t *testing.T) { }) // Append and AppendUnstructured do not have a separate order. - ordered.Append(NewHBA().User("zoro")) + ordered.Append(NewHBA().Users("zoro")) assert.Equal(t, ordered.Length(), 3) assert.DeepEqual(t, ordered.AsStrings(), []string{ `all all all`, @@ -109,6 +127,22 @@ func TestOrderedHBAs(t *testing.T) { }) }) + // See [internal/testing/validation.TestPostgresAuthenticationRules] + t.Run("NoInclude", func(t *testing.T) { + rules := new(OrderedHBAs) + rules.AppendUnstructured( + `one`, + `include "/etc/passwd"`, + ` include_dir /tmp`, + `include_if_exists postgresql.auto.conf`, + `two`, + ) + assert.DeepEqual(t, rules.AsStrings(), []string{ + `one`, + `two`, + }) + }) + t.Run("SpecialCharactersStripped", func(t *testing.T) { rules := new(OrderedHBAs) rules.AppendUnstructured( diff --git a/internal/testing/validation/postgrescluster_test.go b/internal/testing/validation/postgrescluster_test.go index 5c8bd9f0e3..18a17de069 100644 --- a/internal/testing/validation/postgrescluster_test.go +++ b/internal/testing/validation/postgrescluster_test.go @@ -21,6 +21,105 @@ import ( "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" ) +func TestPostgresAuthenticationRules(t *testing.T) { + ctx := context.Background() + cc := require.Kubernetes(t) + t.Parallel() + + namespace := require.Namespace(t, cc) + base := v1beta1.NewPostgresCluster() + + // Start with a bunch of required fields. + require.UnmarshalInto(t, &base.Spec, `{ + postgresVersion: 16, + backups: { + pgbackrest: { + repos: [{ name: repo1 }], + }, + }, + instances: [{ + dataVolumeClaimSpec: { + accessModes: [ReadWriteOnce], + resources: { requests: { storage: 1Mi } }, + }, + }], + }`) + + base.Namespace = namespace.Name + base.Name = "postgres-authentication-rules" + + assert.NilError(t, cc.Create(ctx, base.DeepCopy(), client.DryRunAll), + "expected this base cluster to be valid") + + t.Run("OneTopLevel", func(t *testing.T) { + cluster := base.DeepCopy() + require.UnmarshalInto(t, &cluster.Spec.Authentication, `{ + rules: [ + { connection: host, hba: anything }, + { users: [alice, bob], hba: anything }, + ], + }`) + + err := cc.Create(ctx, cluster, client.DryRunAll) + assert.Assert(t, apierrors.IsInvalid(err)) + + status := require.StatusError(t, err) + assert.Assert(t, status.Details != nil) + assert.Assert(t, cmp.Len(status.Details.Causes, 2)) + + for i, cause := range status.Details.Causes { + assert.Equal(t, cause.Field, fmt.Sprintf("spec.authentication.rules[%d]", i)) + assert.Assert(t, cmp.Contains(cause.Message, "cannot be combined")) + } + }) + + t.Run("NoInclude", func(t *testing.T) { + cluster := base.DeepCopy() + require.UnmarshalInto(t, &cluster.Spec.Authentication, `{ + rules: [ + { hba: 'include "/etc/passwd"' }, + { hba: ' include_dir /tmp' }, + { hba: 'include_if_exists postgresql.auto.conf' }, + ], + }`) + + err := cc.Create(ctx, cluster, client.DryRunAll) + assert.Assert(t, apierrors.IsInvalid(err)) + + status := require.StatusError(t, err) + assert.Assert(t, status.Details != nil) + assert.Assert(t, cmp.Len(status.Details.Causes, 3)) + + for i, cause := range status.Details.Causes { + assert.Equal(t, cause.Field, fmt.Sprintf("spec.authentication.rules[%d].hba", i)) + assert.Assert(t, cmp.Contains(cause.Message, "cannot include")) + } + }) + + t.Run("NoStructuredTrust", func(t *testing.T) { + cluster := base.DeepCopy() + require.UnmarshalInto(t, &cluster.Spec.Authentication, `{ + rules: [ + { connection: local, method: trust }, + { connection: hostssl, method: trust }, + { connection: hostgssenc, method: trust }, + ], + }`) + + err := cc.Create(ctx, cluster, client.DryRunAll) + assert.Assert(t, apierrors.IsInvalid(err)) + + status := require.StatusError(t, err) + assert.Assert(t, status.Details != nil) + assert.Assert(t, cmp.Len(status.Details.Causes, 3)) + + for i, cause := range status.Details.Causes { + assert.Equal(t, cause.Field, fmt.Sprintf("spec.authentication.rules[%d].method", i)) + assert.Assert(t, cmp.Contains(cause.Message, "unsafe")) + } + }) +} + func TestPostgresConfigParameters(t *testing.T) { ctx := context.Background() cc := require.Kubernetes(t) diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgres_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgres_types.go index c2f5cc8d0b..8f950dbfa9 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgres_types.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgres_types.go @@ -9,6 +9,15 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" ) +type PostgresAuthenticationSpec struct { + // More info: https://www.postgresql.org/docs/current/auth-pg-hba-conf.html + // --- + // +kubebuilder:validation:MaxItems=10 + // +listType=atomic + // +optional + Rules []PostgresHBARuleSpec `json:"rules,omitempty"` +} + type PostgresConfig struct { // Files to mount under "/etc/postgres". // --- @@ -68,6 +77,70 @@ type PostgresConfig struct { Parameters map[string]intstr.IntOrString `json:"parameters,omitempty"` } +// --- +type PostgresHBARule struct { + // The connection transport this rule matches. Typical values are: + // 1. "host" for network connections that may or may not be encrypted. + // 2. "hostssl" for network connections encrypted using TLS. + // 3. "hostgssenc" for network connections encrypted using GSSAPI. + // --- + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=20 + // +kubebuilder:validation:Pattern=`^[-a-z0-9]+$` + // +optional + Connection string `json:"connection,omitempty"` + + // Which databases this rule matches. When omitted or empty, this rule matches all databases. + // --- + // +kubebuilder:validation:MaxItems=20 + // +listType=atomic + // +optional + Databases []PostgresIdentifier `json:"databases,omitempty"` + + // The authentication method to use when a connection matches this rule. + // The special value "reject" refuses connections that match this rule. + // More info: https://www.postgresql.org/docs/current/auth-methods.html + // --- + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=20 + // +kubebuilder:validation:Pattern=`^[-a-z0-9]+$` + // +kubebuilder:validation:XValidation:rule=`self != "trust"`,message=`the "trust" method is unsafe` + // +optional + Method string `json:"method,omitempty"` + + // --- + // +kubebuilder:validation:MaxProperties=20 + // +mapType=atomic + // +optional + Options map[string]intstr.IntOrString `json:"options,omitempty"` + + // Which user names this rule matches. When omitted or empty, this rule matches all users. + // --- + // +kubebuilder:validation:MaxItems=20 + // +listType=atomic + // +optional + Users []PostgresIdentifier `json:"users,omitempty"` +} + +// --- +// Emulate OpenAPI "anyOf" aka Kubernetes union. +// +kubebuilder:validation:XValidation:rule=`has(self.hba) ? !has(self.connection) && !has(self.databases) && !has(self.method) && !has(self.options) && !has(self.users) : true`,message=`"hba" cannot be combined with other fields` +// +kubebuilder:validation:XValidation:rule=`has(self.hba) ? true : has(self.connection) && has(self.method)`,message=`"connection" and "method" are required` +// +// +structType=atomic +type PostgresHBARuleSpec struct { + // One line of the "pg_hba.conf" file. Changes to this value will be automatically reloaded without validation. + // --- + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=100 + // +kubebuilder:validation:Pattern=`^[[:print:]]+$` + // +kubebuilder:validation:XValidation:rule=`!self.trim().startsWith("include")`,message=`cannot include other files` + // +optional + HBA string `json:"hba,omitempty"` + + PostgresHBARule `json:",inline"` +} + // --- // PostgreSQL identifiers are limited in length but may contain any character. // - https://www.postgresql.org/docs/current/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go index 9f661b0640..2a9f982caf 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go @@ -21,6 +21,9 @@ type PostgresClusterSpec struct { // +optional DataSource *DataSource `json:"dataSource,omitempty"` + // +optional + Authentication *PostgresAuthenticationSpec `json:"authentication,omitempty"` + // PostgreSQL backup configuration // +optional Backups Backups `json:"backups,omitempty"` diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/zz_generated.deepcopy.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/zz_generated.deepcopy.go index 875d1ce000..677a1a1fe9 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/zz_generated.deepcopy.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/zz_generated.deepcopy.go @@ -1754,6 +1754,28 @@ func (in *PatroniSwitchover) DeepCopy() *PatroniSwitchover { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *PostgresAuthenticationSpec) DeepCopyInto(out *PostgresAuthenticationSpec) { + *out = *in + if in.Rules != nil { + in, out := &in.Rules, &out.Rules + *out = make([]PostgresHBARuleSpec, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PostgresAuthenticationSpec. +func (in *PostgresAuthenticationSpec) DeepCopy() *PostgresAuthenticationSpec { + if in == nil { + return nil + } + out := new(PostgresAuthenticationSpec) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PostgresCluster) DeepCopyInto(out *PostgresCluster) { *out = *in @@ -1864,6 +1886,11 @@ func (in *PostgresClusterSpec) DeepCopyInto(out *PostgresClusterSpec) { *out = new(DataSource) (*in).DeepCopyInto(*out) } + if in.Authentication != nil { + in, out := &in.Authentication, &out.Authentication + *out = new(PostgresAuthenticationSpec) + (*in).DeepCopyInto(*out) + } in.Backups.DeepCopyInto(&out.Backups) if in.Config != nil { in, out := &in.Config, &out.Config @@ -2067,6 +2094,54 @@ func (in *PostgresConfig) DeepCopy() *PostgresConfig { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *PostgresHBARule) DeepCopyInto(out *PostgresHBARule) { + *out = *in + if in.Databases != nil { + in, out := &in.Databases, &out.Databases + *out = make([]PostgresIdentifier, len(*in)) + copy(*out, *in) + } + if in.Options != nil { + in, out := &in.Options, &out.Options + *out = make(map[string]intstr.IntOrString, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } + if in.Users != nil { + in, out := &in.Users, &out.Users + *out = make([]PostgresIdentifier, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PostgresHBARule. +func (in *PostgresHBARule) DeepCopy() *PostgresHBARule { + if in == nil { + return nil + } + out := new(PostgresHBARule) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *PostgresHBARuleSpec) DeepCopyInto(out *PostgresHBARuleSpec) { + *out = *in + in.PostgresHBARule.DeepCopyInto(&out.PostgresHBARule) +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PostgresHBARuleSpec. +func (in *PostgresHBARuleSpec) DeepCopy() *PostgresHBARuleSpec { + if in == nil { + return nil + } + out := new(PostgresHBARuleSpec) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PostgresInstanceSetSpec) DeepCopyInto(out *PostgresInstanceSetSpec) { *out = *in From 1afe453e6470d38326c22de90c2ade6f42dc3b60 Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Fri, 28 Feb 2025 14:39:04 -0600 Subject: [PATCH 2/2] Send the "password" method to Postgres as "md5" instead The differences between "password," "md5," and "scram-sha-256" are not interesting to Postgres novices. This allows one to say "password" in the API and have secure authentication using usernames and passwords. The PGO default "password_encryption" has always been "scram-sha-256". Issue: PGO-2263 --- internal/controller/postgrescluster/postgres.go | 11 ++++++++++- internal/controller/postgrescluster/postgres_test.go | 5 +++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/internal/controller/postgrescluster/postgres.go b/internal/controller/postgrescluster/postgres.go index 74547f5d5e..8a3a73c8bd 100644 --- a/internal/controller/postgrescluster/postgres.go +++ b/internal/controller/postgrescluster/postgres.go @@ -51,7 +51,16 @@ func (*Reconciler) generatePostgresHBA(spec *v1beta1.PostgresHBARule) *postgres. result := postgres.NewHBA() result.Origin(spec.Connection) - result.Method(spec.Method) + + // The "password" method is not recommended. More likely, the user wants to + // use passwords generally. The most compatible method for that is "md5" + // which accepts a password in the format in which it is hashed in the database. + // - https://www.postgresql.org/docs/current/auth-password.html + if spec.Method == "password" { + result.Method("md5") + } else { + result.Method(spec.Method) + } if len(spec.Databases) > 0 { result.Databases(spec.Databases[0], spec.Databases[1:]...) diff --git a/internal/controller/postgrescluster/postgres_test.go b/internal/controller/postgrescluster/postgres_test.go index edb203ecd0..86310c717b 100644 --- a/internal/controller/postgrescluster/postgres_test.go +++ b/internal/controller/postgrescluster/postgres_test.go @@ -59,6 +59,11 @@ func TestGeneratePostgresHBA(t *testing.T) { rule: `{ connection: hostssl, method: md5, options: { clientcert: verify-ca } }`, expected: `"hostssl" all all all "md5" "clientcert"="verify-ca"`, }, + // "password" input should be "md5" output + { + rule: `{ connection: hostssl, method: password }`, + expected: `"hostssl" all all all "md5"`, + }, } { var rule *v1beta1.PostgresHBARule require.UnmarshalInto(t, &rule, tt.rule)