Reject invalid Postgres user specs

cbandy · cbandy · commit 990631fefc58 · 2024-04-25T08:43:01.000-05:00
The PASSWORD option was never effective. CEL validation rules, available in beta since Kubernetes 1.25, can detect and reject values that the RE2 pattern validation of "github.com/go-openapi" could not. Issue: PGO-1094 See: https://pkg.go.dev/github.com/go-openapi/validate@v0.24.0#hdr-Known_limitations
diff --git a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml
@@ -15080,8 +15080,14 @@ spec:
                     options:
                       description: 'ALTER ROLE options except for PASSWORD. This field
                         is ignored for the "postgres" user. More info: https://www.postgresql.org/docs/current/role-attributes.html'
+                      maxLength: 200
                       pattern: ^[^;]*$
                       type: string
+                      x-kubernetes-validations:
+                      - message: cannot assign password
+                        rule: '!self.matches("(?i:PASSWORD)")'
+                      - message: cannot contain comments
+                        rule: '!self.matches("(?:--|/[*]|[*]/)")'
                     password:
                       description: Properties of the password generated for this user.
                       properties:
@@ -15102,6 +15108,7 @@ spec:
                   required:
                   - name
                   type: object
+                maxItems: 64
                 type: array
                 x-kubernetes-list-map-keys:
                 - name
diff --git a/internal/controller/postgrescluster/postgres_test.go b/internal/controller/postgrescluster/postgres_test.go
@@ -679,6 +679,8 @@ func TestValidatePostgresUsers(t *testing.T) {
 		assert.Equal(t, len(recorder.Events), 0)
 	})
 
+	// See [internal/testing/validation.TestPostgresUserOptions]
+
 	t.Run("NoComments", func(t *testing.T) {
 		cluster := v1beta1.NewPostgresCluster()
 		cluster.Name = "pg1"
diff --git a/internal/testing/validation/postgrescluster_test.go b/internal/testing/validation/postgrescluster_test.go
@@ -0,0 +1,136 @@
+/*
+ Copyright 2021 - 2024 Crunchy Data Solutions, Inc.
+ Licensed under the Apache License, Version 2.0 (the "License");
+ you may not use this file except in compliance with the License.
+ You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+package validation
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	"gotest.tools/v3/assert"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/yaml"
+
+	"github.com/crunchydata/postgres-operator/internal/testing/cmp"
+	"github.com/crunchydata/postgres-operator/internal/testing/require"
+	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
+)
+
+func TestPostgresUserOptions(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.
+	assert.NilError(t, yaml.Unmarshal([]byte(`{
+		postgresVersion: 16,
+		backups: {
+			pgbackrest: {
+				repos: [{ name: repo1 }],
+			},
+		},
+		instances: [{
+			dataVolumeClaimSpec: {
+				accessModes: [ReadWriteOnce],
+				resources: { requests: { storage: 1Mi } },
+			},
+		}],
+	}`), &base.Spec))
+
+	base.Namespace = namespace.Name
+	base.Name = "postgres-user-options"
+
+	assert.NilError(t, cc.Create(ctx, base.DeepCopy(), client.DryRunAll),
+		"expected this base cluster to be valid")
+
+	// See [internal/controller/postgrescluster.TestValidatePostgresUsers]
+
+	t.Run("NoComments", func(t *testing.T) {
+		cluster := base.DeepCopy()
+		cluster.Spec.Users = []v1beta1.PostgresUserSpec{
+			{Name: "dashes", Options: "ANY -- comment"},
+			{Name: "block-open", Options: "/* asdf"},
+			{Name: "block-close", Options: " qw */ rt"},
+		}
+
+		err := cc.Create(ctx, cluster, client.DryRunAll)
+		assert.Assert(t, apierrors.IsInvalid(err))
+		assert.ErrorContains(t, err, "cannot contain comments")
+
+		//nolint:errorlint // This is a test, and a panic is unlikely.
+		status := err.(apierrors.APIStatus).Status()
+		assert.Assert(t, status.Details != nil)
+		assert.Equal(t, len(status.Details.Causes), 3)
+
+		for i, cause := range status.Details.Causes {
+			assert.Equal(t, cause.Field, fmt.Sprintf("spec.users[%d].options", i))
+			assert.Assert(t, cmp.Contains(cause.Message, "cannot contain comments"))
+		}
+	})
+
+	t.Run("NoPassword", func(t *testing.T) {
+		cluster := base.DeepCopy()
+		cluster.Spec.Users = []v1beta1.PostgresUserSpec{
+			{Name: "uppercase", Options: "SUPERUSER PASSWORD ''"},
+			{Name: "lowercase", Options: "password 'asdf'"},
+		}
+
+		err := cc.Create(ctx, cluster, client.DryRunAll)
+		assert.Assert(t, apierrors.IsInvalid(err))
+		assert.ErrorContains(t, err, "cannot assign password")
+
+		//nolint:errorlint // This is a test, and a panic is unlikely.
+		status := err.(apierrors.APIStatus).Status()
+		assert.Assert(t, status.Details != nil)
+		assert.Equal(t, len(status.Details.Causes), 2)
+
+		for i, cause := range status.Details.Causes {
+			assert.Equal(t, cause.Field, fmt.Sprintf("spec.users[%d].options", i))
+			assert.Assert(t, cmp.Contains(cause.Message, "cannot assign password"))
+		}
+	})
+
+	t.Run("NoTerminators", func(t *testing.T) {
+		cluster := base.DeepCopy()
+		cluster.Spec.Users = []v1beta1.PostgresUserSpec{
+			{Name: "semicolon", Options: "some ;where"},
+		}
+
+		err := cc.Create(ctx, cluster, client.DryRunAll)
+		assert.Assert(t, apierrors.IsInvalid(err))
+		assert.ErrorContains(t, err, "should match")
+
+		//nolint:errorlint // This is a test, and a panic is unlikely.
+		status := err.(apierrors.APIStatus).Status()
+		assert.Assert(t, status.Details != nil)
+		assert.Equal(t, len(status.Details.Causes), 1)
+		assert.Equal(t, status.Details.Causes[0].Field, "spec.users[0].options")
+	})
+
+	t.Run("Valid", func(t *testing.T) {
+		cluster := base.DeepCopy()
+		cluster.Spec.Users = []v1beta1.PostgresUserSpec{
+			{Name: "normal", Options: "CREATEDB valid until '2006-01-02'"},
+			{Name: "very-full", Options: "NOSUPERUSER NOCREATEDB NOCREATEROLE NOINHERIT NOLOGIN NOREPLICATION NOBYPASSRLS CONNECTION LIMIT 5"},
+		}
+
+		assert.NilError(t, cc.Create(ctx, cluster, client.DryRunAll))
+	})
+}
diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgres_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgres_types.go
@@ -60,7 +60,10 @@ type PostgresUserSpec struct {
 	// ALTER ROLE options except for PASSWORD. This field is ignored for the
 	// "postgres" user.
 	// More info: https://www.postgresql.org/docs/current/role-attributes.html
+	// +kubebuilder:validation:MaxLength=200
 	// +kubebuilder:validation:Pattern=`^[^;]*$`
+	// +kubebuilder:validation:XValidation:rule=`!self.matches("(?i:PASSWORD)")`,message="cannot assign password"
+	// +kubebuilder:validation:XValidation:rule=`!self.matches("(?:--|/[*]|[*]/)")`,message="cannot contain comments"
 	// +optional
 	Options string `json:"options,omitempty"`
 
diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/postgrescluster_types.go
@@ -175,6 +175,7 @@ type PostgresClusterSpec struct {
 	// from this list does NOT drop the user nor revoke their access.
 	// +listType=map
 	// +listMapKey=name
+	// +kubebuilder:validation:MaxItems=64
 	// +optional
 	Users []PostgresUserSpec `json:"users,omitempty"`
 
