From a1f2bd05b978b4dac384eb14a06a39f590cf5f57 Mon Sep 17 00:00:00 2001
From: Dmitry Dolgov <9erthalion6@gmail.com>
Date: Thu, 9 Apr 2020 09:21:45 +0200
Subject: [PATCH] Prevent superuser from being a connection pool user (#906)

* Protected and system users can't be a connection pool user

It's not supported, neither it's a best practice. Also fix potential null
pointer access. For protected users it makes sense by intent of protecting this
users (e.g. from being overriden or used as something else than supposed). For
system users the reason is the same as for superuser, it's about replicastion
user and it's under patroni control.

This is implemented on both levels, operator config and postgresql manifest.
For the latter we just use default name in this case, assuming that operator
config is always correct. For the former, since it's a serious
misconfiguration, operator panics.
---
 pkg/cluster/cluster.go            | 17 +++++++++++++---
 pkg/cluster/cluster_test.go       | 34 +++++++++++++++++++++++++++++++
 pkg/cluster/resources.go          |  5 +++++
 pkg/controller/operator_config.go |  5 +++++
 pkg/util/config/config.go         |  6 ++++++
 5 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go
index cde2dc260..51dd368fb 100644
--- a/pkg/cluster/cluster.go
+++ b/pkg/cluster/cluster.go
@@ -877,9 +877,20 @@ func (c *Cluster) initSystemUsers() {
 			c.Spec.ConnectionPooler = &acidv1.ConnectionPooler{}
 		}
 
-		username := util.Coalesce(
-			c.Spec.ConnectionPooler.User,
-			c.OpConfig.ConnectionPooler.User)
+		// Using superuser as pooler user is not a good idea. First of all it's
+		// not going to be synced correctly with the current implementation,
+		// and second it's a bad practice.
+		username := c.OpConfig.ConnectionPooler.User
+
+		isSuperUser := c.Spec.ConnectionPooler.User == c.OpConfig.SuperUsername
+		isProtectedUser := c.shouldAvoidProtectedOrSystemRole(
+			c.Spec.ConnectionPooler.User, "connection pool role")
+
+		if !isSuperUser && !isProtectedUser {
+			username = util.Coalesce(
+				c.Spec.ConnectionPooler.User,
+				c.OpConfig.ConnectionPooler.User)
+		}
 
 		// connection pooler application should be able to login with this role
 		connectionPoolerUser := spec.PgUser{
diff --git a/pkg/cluster/cluster_test.go b/pkg/cluster/cluster_test.go
index af3092ad5..432f53132 100644
--- a/pkg/cluster/cluster_test.go
+++ b/pkg/cluster/cluster_test.go
@@ -721,4 +721,38 @@ func TestInitSystemUsers(t *testing.T) {
 	if _, exist := cl.systemUsers[constants.ConnectionPoolerUserKeyName]; !exist {
 		t.Errorf("%s, connection pooler user is not present", testName)
 	}
+
+	// superuser is not allowed as connection pool user
+	cl.Spec.ConnectionPooler = &acidv1.ConnectionPooler{
+		User: "postgres",
+	}
+	cl.OpConfig.SuperUsername = "postgres"
+	cl.OpConfig.ConnectionPooler.User = "pooler"
+
+	cl.initSystemUsers()
+	if _, exist := cl.pgUsers["pooler"]; !exist {
+		t.Errorf("%s, Superuser is not allowed to be a connection pool user", testName)
+	}
+
+	// neither protected users are
+	delete(cl.pgUsers, "pooler")
+	cl.Spec.ConnectionPooler = &acidv1.ConnectionPooler{
+		User: "admin",
+	}
+	cl.OpConfig.ProtectedRoles = []string{"admin"}
+
+	cl.initSystemUsers()
+	if _, exist := cl.pgUsers["pooler"]; !exist {
+		t.Errorf("%s, Protected user are not allowed to be a connection pool user", testName)
+	}
+
+	delete(cl.pgUsers, "pooler")
+	cl.Spec.ConnectionPooler = &acidv1.ConnectionPooler{
+		User: "standby",
+	}
+
+	cl.initSystemUsers()
+	if _, exist := cl.pgUsers["pooler"]; !exist {
+		t.Errorf("%s, System users are not allowed to be a connection pool user", testName)
+	}
 }
diff --git a/pkg/cluster/resources.go b/pkg/cluster/resources.go
index 4c341aefe..b38458af8 100644
--- a/pkg/cluster/resources.go
+++ b/pkg/cluster/resources.go
@@ -105,7 +105,12 @@ func (c *Cluster) createConnectionPooler(lookup InstallFunction) (*ConnectionPoo
 	var msg string
 	c.setProcessName("creating connection pooler")
 
+	if c.ConnectionPooler == nil {
+		c.ConnectionPooler = &ConnectionPoolerObjects{}
+	}
+
 	schema := c.Spec.ConnectionPooler.Schema
+
 	if schema == "" {
 		schema = c.OpConfig.ConnectionPooler.Schema
 	}
diff --git a/pkg/controller/operator_config.go b/pkg/controller/operator_config.go
index a9c36f995..07be90f22 100644
--- a/pkg/controller/operator_config.go
+++ b/pkg/controller/operator_config.go
@@ -169,6 +169,11 @@ func (c *Controller) importConfigurationFromCRD(fromCRD *acidv1.OperatorConfigur
 		fromCRD.ConnectionPooler.User,
 		constants.ConnectionPoolerUserName)
 
+	if result.ConnectionPooler.User == result.SuperUsername {
+		msg := "Connection pool user is not allowed to be the same as super user, username: %s"
+		panic(fmt.Errorf(msg, result.ConnectionPooler.User))
+	}
+
 	result.ConnectionPooler.Image = util.Coalesce(
 		fromCRD.ConnectionPooler.Image,
 		"registry.opensource.zalan.do/acid/pgbouncer")
diff --git a/pkg/util/config/config.go b/pkg/util/config/config.go
index 0a7bac835..84a62c0fd 100644
--- a/pkg/util/config/config.go
+++ b/pkg/util/config/config.go
@@ -218,5 +218,11 @@ func validate(cfg *Config) (err error) {
 		msg := "number of connection pooler instances should be higher than %d"
 		err = fmt.Errorf(msg, constants.ConnectionPoolerMinInstances)
 	}
+
+	if cfg.ConnectionPooler.User == cfg.SuperUsername {
+		msg := "Connection pool user is not allowed to be the same as super user, username: %s"
+		err = fmt.Errorf(msg, cfg.ConnectionPooler.User)
+	}
+
 	return
 }