Prevent superuser from being a connection pool user (zalando#906)

erthalion · web-flow · commit a1f2bd05b978 · 2020-04-09T09:21:45.000+02:00
* 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.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
 }

Original file line number	Diff line number	Diff line change
`@@ -218,5 +218,11 @@ func validate(cfg *Config) (err error) {`
`218`	`218`	`msg := "number of connection pooler instances should be higher than %d"`
`219`	`219`	`err = fmt.Errorf(msg, constants.ConnectionPoolerMinInstances)`
`220`	`220`	`}`
	`221`	`+`
	`222`	`+ if cfg.ConnectionPooler.User == cfg.SuperUsername {`
	`223`	`+ msg := "Connection pool user is not allowed to be the same as super user, username: %s"`
	`224`	`+ err = fmt.Errorf(msg, cfg.ConnectionPooler.User)`
	`225`	`+ }`
	`226`	`+`
`221`	`227`	`return`
`222`	`228`	`}`