Skip to content

Commit

Permalink
feat: Add support for grant role with admin option if asked
Browse files Browse the repository at this point in the history
  • Loading branch information
oxyno-zeta committed Apr 8, 2024
1 parent 0ed117c commit 5ac5247
Show file tree
Hide file tree
Showing 12 changed files with 982 additions and 165 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@ type PostgresqlEngineConfigurationSpec struct {
DefaultDatabase string `json:"defaultDatabase,omitempty"`
// Duration between two checks for valid engine
CheckInterval string `json:"checkInterval,omitempty"`
// Allow grant admin on every created roles (group or user) for provided PGEC user in order to
// have power to administrate those roles even with a less powered "admin" user.
// Operator will create role and after grant PGEC provided user on those roles with admin option if enabled.
AllowGrantAdminOption bool `json:"allowGrantAdminOption,omitempty"`
// Wait for linked resource to be deleted
WaitLinkedResourcesDeletion bool `json:"waitLinkedResourcesDeletion,omitempty"`
// User and password secret
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,12 @@ spec:
description: PostgresqlEngineConfigurationSpec defines the desired state
of PostgresqlEngineConfiguration.
properties:
allowGrantAdminOption:
description: |-
Allow grant admin on every created roles (group or user) for provided PGEC user in order to
have power to administrate those roles even with a less powered "admin" user.
Operator will create role and after grant PGEC provided user on those roles with admin option if enabled.
type: boolean
checkInterval:
description: Duration between two checks for valid engine
type: string
Expand Down
4 changes: 4 additions & 0 deletions config/samples/engineconfiguration/full-example.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ spec:
# Check interval
# Default to 30s
checkInterval: 30s
# Allow grant admin on every created roles (group or user) for provided PGEC user in order to
# have power to administrate those roles even with a less powered "admin" user.
# Operator will create role and after grant PGEC provided user on those roles with admin option if enabled.
allowGrantAdminOption: true
# Wait for linked resource to be deleted
# Default to false
waitLinkedResourcesDeletion: true
Expand Down
10 changes: 5 additions & 5 deletions internal/controller/postgresql/postgres/aws.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ func newAWSPG(postgres *pg) PG {
func (c *awspg) AlterDefaultLoginRole(role, setRole string) error {
// On AWS RDS the postgres user isn't really superuser so he doesn't have permissions
// to ALTER USER unless he belongs to both roles
err := c.GrantRole(role, c.user)
err := c.GrantRole(role, c.user, false)
if err != nil {
return err
}
Expand Down Expand Up @@ -67,7 +67,7 @@ func (c *awspg) CreateDB(dbname, role string) error {
func (c *awspg) DropRoleAndDropAndChangeOwnedBy(role, newOwner, database string) error {
// On AWS RDS the postgres user isn't really superuser so he doesn't have permissions
// to REASSIGN OWNED BY unless he belongs to both roles
err := c.GrantRole(role, c.user)
err := c.GrantRole(role, c.user, false)
// Check error
if err != nil {
// Try to cast error
Expand All @@ -85,7 +85,7 @@ func (c *awspg) DropRoleAndDropAndChangeOwnedBy(role, newOwner, database string)
}
}

err = c.GrantRole(newOwner, c.user)
err = c.GrantRole(newOwner, c.user, false)
// Check error
if err != nil {
// Try to cast error
Expand Down Expand Up @@ -119,7 +119,7 @@ func (c *awspg) DropRoleAndDropAndChangeOwnedBy(role, newOwner, database string)
func (c *awspg) ChangeAndDropOwnedBy(role, newOwner, database string) error {
// On AWS RDS the postgres user isn't really superuser so he doesn't have permissions
// to REASSIGN OWNED BY unless he belongs to both roles
err := c.GrantRole(role, c.user)
err := c.GrantRole(role, c.user, false)
// Check error
if err != nil {
// Try to cast error
Expand All @@ -137,7 +137,7 @@ func (c *awspg) ChangeAndDropOwnedBy(role, newOwner, database string) error {
}
}

err = c.GrantRole(newOwner, c.user)
err = c.GrantRole(newOwner, c.user, false)
// Check error
if err != nil {
// Try to cast error
Expand Down
2 changes: 1 addition & 1 deletion internal/controller/postgresql/postgres/azure.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func (azpg *azurepg) GetRoleForLogin(login string) string {

func (azpg *azurepg) CreateDB(dbname, role string) error {
// Have to add the master role to the group role before we can transfer the database owner
err := azpg.GrantRole(role, azpg.GetRoleForLogin(azpg.user))
err := azpg.GrantRole(role, azpg.GetRoleForLogin(azpg.user), false)
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion internal/controller/postgresql/postgres/postgres.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ type PG interface { //nolint:interfacebloat // This is needed
IsRoleExist(role string) (bool, error)
RenameRole(oldname, newname string) error
UpdatePassword(role, password string) error
GrantRole(role, grantee string) error
GrantRole(role, grantee string, withAdminOption bool) error
SetSchemaPrivileges(db, creator, role, schema, privs string) error
RevokeRole(role, userRole string) error
AlterDefaultLoginRole(role, setRole string) error
Expand Down
11 changes: 9 additions & 2 deletions internal/controller/postgresql/postgres/role.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const (
CreateGroupRoleSQLTemplate = `CREATE ROLE "%s"`
CreateUserRoleSQLTemplate = `CREATE ROLE "%s" WITH LOGIN PASSWORD '%s'`
GrantRoleSQLTemplate = `GRANT "%s" TO "%s"`
GrantRoleWithAdminOptionSQLTemplate = `GRANT "%s" TO "%s" WITH ADMIN OPTION`
AlterUserSetRoleSQLTemplate = `ALTER USER "%s" SET ROLE "%s"`
AlterUserSetRoleOnDatabaseSQLTemplate = `ALTER ROLE "%s" IN DATABASE "%s" SET ROLE "%s"`
RevokeUserSetRoleOnDatabaseSQLTemplate = `ALTER ROLE "%s" IN DATABASE "%s" RESET role`
Expand Down Expand Up @@ -104,13 +105,19 @@ func (c *pg) CreateUserRole(role, password string) (string, error) {
return role, nil
}

func (c *pg) GrantRole(role, grantee string) error {
func (c *pg) GrantRole(role, grantee string, withAdminOption bool) error {
err := c.connect(c.defaultDatabase)
if err != nil {
return err
}

_, err = c.db.Exec(fmt.Sprintf(GrantRoleSQLTemplate, role, grantee))
// Select right SQL template
tpl := GrantRoleSQLTemplate
if withAdminOption {
tpl = GrantRoleWithAdminOptionSQLTemplate
}

_, err = c.db.Exec(fmt.Sprintf(tpl, role, grantee))
if err != nil {
return err
}
Expand Down
18 changes: 9 additions & 9 deletions internal/controller/postgresql/postgresqldatabase_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ func (r *PostgresqlDatabaseReconciler) Reconcile(ctx context.Context, req ctrl.R
}

// Create owner role
err = r.manageOwnerRole(pg, owner, instance)
err = r.manageOwnerRole(pg, owner, instance, pgEngCfg.Spec.AllowGrantAdminOption)
if err != nil {
return r.manageError(ctx, reqLogger, instance, originalPatch, errors.NewInternalError(err))
}
Expand All @@ -203,13 +203,13 @@ func (r *PostgresqlDatabaseReconciler) Reconcile(ctx context.Context, req ctrl.R
}

// Create reader role
err = r.manageReaderRole(pg, reader, instance)
err = r.manageReaderRole(pg, reader, instance, pgEngCfg.Spec.AllowGrantAdminOption)
if err != nil {
return r.manageError(ctx, reqLogger, instance, originalPatch, errors.NewInternalError(err))
}

// Create writer role
err = r.manageWriterRole(pg, writer, instance)
err = r.manageWriterRole(pg, writer, instance, pgEngCfg.Spec.AllowGrantAdminOption)
if err != nil {
return r.manageError(ctx, reqLogger, instance, originalPatch, errors.NewInternalError(err))
}
Expand Down Expand Up @@ -511,7 +511,7 @@ func (*PostgresqlDatabaseReconciler) manageExtensions(pg postgres.PG, instance *
return nil
}

func (*PostgresqlDatabaseReconciler) manageReaderRole(pg postgres.PG, reader string, instance *postgresqlv1alpha1.PostgresqlDatabase) error {
func (*PostgresqlDatabaseReconciler) manageReaderRole(pg postgres.PG, reader string, instance *postgresqlv1alpha1.PostgresqlDatabase, allowGrantAdminOption bool) error {
// Check if role was already created in the past
if instance.Status.Roles.Reader != "" {
// Check if role doesn't already exists
Expand Down Expand Up @@ -548,7 +548,7 @@ func (*PostgresqlDatabaseReconciler) manageReaderRole(pg postgres.PG, reader str
}

// Grant role to current role
err = pg.GrantRole(reader, pg.GetUser())
err = pg.GrantRole(reader, pg.GetUser(), allowGrantAdminOption)
// Check error
if err != nil {
return err
Expand All @@ -560,7 +560,7 @@ func (*PostgresqlDatabaseReconciler) manageReaderRole(pg postgres.PG, reader str
return nil
}

func (*PostgresqlDatabaseReconciler) manageWriterRole(pg postgres.PG, writer string, instance *postgresqlv1alpha1.PostgresqlDatabase) error {
func (*PostgresqlDatabaseReconciler) manageWriterRole(pg postgres.PG, writer string, instance *postgresqlv1alpha1.PostgresqlDatabase, allowGrantAdminOption bool) error {
// Check if role was already created in the past
if instance.Status.Roles.Writer != "" {
// Check if role doesn't already exists
Expand Down Expand Up @@ -597,7 +597,7 @@ func (*PostgresqlDatabaseReconciler) manageWriterRole(pg postgres.PG, writer str
}

// Grant role to current role
err = pg.GrantRole(writer, pg.GetUser())
err = pg.GrantRole(writer, pg.GetUser(), allowGrantAdminOption)
// Check error
if err != nil {
return err
Expand All @@ -609,7 +609,7 @@ func (*PostgresqlDatabaseReconciler) manageWriterRole(pg postgres.PG, writer str
return nil
}

func (*PostgresqlDatabaseReconciler) manageOwnerRole(pg postgres.PG, owner string, instance *postgresqlv1alpha1.PostgresqlDatabase) error {
func (*PostgresqlDatabaseReconciler) manageOwnerRole(pg postgres.PG, owner string, instance *postgresqlv1alpha1.PostgresqlDatabase, allowGrantAdminOption bool) error {
// Check if role was already created in the past
if instance.Status.Roles.Owner != "" {
// Check if role doesn't already exists
Expand Down Expand Up @@ -646,7 +646,7 @@ func (*PostgresqlDatabaseReconciler) manageOwnerRole(pg postgres.PG, owner strin
}

// Grant role to current role
err = pg.GrantRole(owner, pg.GetUser())
err = pg.GrantRole(owner, pg.GetUser(), allowGrantAdminOption)
// Check error
if err != nil {
return err
Expand Down
105 changes: 90 additions & 15 deletions internal/controller/postgresql/postgresqldatabase_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,17 +174,28 @@ var _ = Describe("PostgresqlDatabase tests", func() {
Expect(exists).To(BeTrue())

// Check if roles exists and are granted to default user
checkRoleInSQLDb(postgresUser, ownerRole)
checkRoleInSQLDb(postgresUser, readerRole)
checkRoleInSQLDb(postgresUser, writerRole)
checkRoleInSQLDb(ownerRole)
checkRoleInSQLDb(readerRole)
checkRoleInSQLDb(writerRole)

// Check DB ownership
isOwner, err := isRoleOwnerofSQLDB(pgdbDBName, ownerRole)
Expect(err).ToNot(HaveOccurred())
Expect(isOwner).To(BeTrue())

// Check role members and rights
ownerMemberWithAdminOption, err := getSQLRoleMembershipWithAdminOption(item.Status.Roles.Owner)
Expect(err).ToNot(HaveOccurred())
Expect(ownerMemberWithAdminOption).To(Equal(map[string]bool{postgresUser: false}))
writerMemberWithAdminOption, err := getSQLRoleMembershipWithAdminOption(item.Status.Roles.Writer)
Expect(err).ToNot(HaveOccurred())
Expect(writerMemberWithAdminOption).To(Equal(map[string]bool{postgresUser: false}))
readerMemberWithAdminOption, err := getSQLRoleMembershipWithAdminOption(item.Status.Roles.Reader)
Expect(err).ToNot(HaveOccurred())
Expect(readerMemberWithAdminOption).To(Equal(map[string]bool{postgresUser: false}))
})

It("should be ok to set all values (required & optional)", func() {
It("should be ok to set all values (required & optional & with a custom owner role name)", func() {
// Create pgec
prov, _ := setupPGEC("10s", false)

Expand Down Expand Up @@ -263,9 +274,20 @@ var _ = Describe("PostgresqlDatabase tests", func() {
Expect(exists).To(BeTrue())

// Check if roles exists and are granted to default user
checkRoleInSQLDb(postgresUser, ownerRole)
checkRoleInSQLDb(postgresUser, readerRole)
checkRoleInSQLDb(postgresUser, writerRole)
checkRoleInSQLDb(ownerRole)
checkRoleInSQLDb(readerRole)
checkRoleInSQLDb(writerRole)

// Check role members and rights
ownerMemberWithAdminOption, err := getSQLRoleMembershipWithAdminOption(item.Status.Roles.Owner)
Expect(err).ToNot(HaveOccurred())
Expect(ownerMemberWithAdminOption).To(Equal(map[string]bool{postgresUser: false}))
writerMemberWithAdminOption, err := getSQLRoleMembershipWithAdminOption(item.Status.Roles.Writer)
Expect(err).ToNot(HaveOccurred())
Expect(writerMemberWithAdminOption).To(Equal(map[string]bool{postgresUser: false}))
readerMemberWithAdminOption, err := getSQLRoleMembershipWithAdminOption(item.Status.Roles.Reader)
Expect(err).ToNot(HaveOccurred())
Expect(readerMemberWithAdminOption).To(Equal(map[string]bool{postgresUser: false}))
})

It("should drop database on crd deletion if DropOnDelete set to true", func() {
Expand Down Expand Up @@ -436,6 +458,48 @@ var _ = Describe("PostgresqlDatabase tests", func() {
Expect(stillExists).To(BeTrue())
})

It("should be ok to have a pgdb with a pgec with allow grant admin option on roles", func() {
// Create pgec
setupPGECWithAllowGrantAdminOption("10s", false)

// Create pgdb
item := setupPGDB(true)

// Checks
ownerRole := fmt.Sprintf("%s-owner", pgdbDBName)
readerRole := fmt.Sprintf("%s-reader", pgdbDBName)
writerRole := fmt.Sprintf("%s-writer", pgdbDBName)

Expect(item.Status.Ready).To(BeTrue())
Expect(item.Status.Phase).To(Equal(postgresqlv1alpha1.DatabaseCreatedPhase))
Expect(item.Status.Database).To(Equal(pgdbDBName))
Expect(item.Status.Message).To(BeEmpty())
Expect(item.Status.Roles.Owner).To(Equal(ownerRole))
Expect(item.Status.Roles.Reader).To(Equal(readerRole))
Expect(item.Status.Roles.Writer).To(Equal(writerRole))

// Check if DB exists
exists, err := isSQLDBExists(pgdbDBName)
Expect(err).ToNot(HaveOccurred())
Expect(exists).To(BeTrue())

// Check if roles exists and are granted to default user
checkRoleInSQLDb(ownerRole)
checkRoleInSQLDb(readerRole)
checkRoleInSQLDb(writerRole)

// Check role members and rights
ownerMemberWithAdminOption, err := getSQLRoleMembershipWithAdminOption(item.Status.Roles.Owner)
Expect(err).ToNot(HaveOccurred())
Expect(ownerMemberWithAdminOption).To(Equal(map[string]bool{postgresUser: true}))
writerMemberWithAdminOption, err := getSQLRoleMembershipWithAdminOption(item.Status.Roles.Writer)
Expect(err).ToNot(HaveOccurred())
Expect(writerMemberWithAdminOption).To(Equal(map[string]bool{postgresUser: true}))
readerMemberWithAdminOption, err := getSQLRoleMembershipWithAdminOption(item.Status.Roles.Reader)
Expect(err).ToNot(HaveOccurred())
Expect(readerMemberWithAdminOption).To(Equal(map[string]bool{postgresUser: true}))
})

It("should be ok to have a pgdb referencing an existing PG database", func() {
// Create SQL db
errDB := createSQLDB(pgdbDBName, postgresUser)
Expand All @@ -455,13 +519,24 @@ var _ = Describe("PostgresqlDatabase tests", func() {

// Check if roles exists and are granted to default user
ownerRole := fmt.Sprintf("%s-owner", pgdbDBName)
checkRoleInSQLDb(postgresUser, ownerRole)
checkRoleInSQLDb(ownerRole)

readerRole := fmt.Sprintf("%s-reader", pgdbDBName)
checkRoleInSQLDb(postgresUser, readerRole)
checkRoleInSQLDb(readerRole)

writerRole := fmt.Sprintf("%s-writer", pgdbDBName)
checkRoleInSQLDb(postgresUser, writerRole)
checkRoleInSQLDb(writerRole)

// Check role members and rights
ownerMemberWithAdminOption, err := getSQLRoleMembershipWithAdminOption(item.Status.Roles.Owner)
Expect(err).ToNot(HaveOccurred())
Expect(ownerMemberWithAdminOption).To(Equal(map[string]bool{postgresUser: false}))
writerMemberWithAdminOption, err := getSQLRoleMembershipWithAdminOption(item.Status.Roles.Writer)
Expect(err).ToNot(HaveOccurred())
Expect(writerMemberWithAdminOption).To(Equal(map[string]bool{postgresUser: false}))
readerMemberWithAdminOption, err := getSQLRoleMembershipWithAdminOption(item.Status.Roles.Reader)
Expect(err).ToNot(HaveOccurred())
Expect(readerMemberWithAdminOption).To(Equal(map[string]bool{postgresUser: false}))
})

It("should be ok to have a pgdb referencing an existing master role", func() {
Expand Down Expand Up @@ -522,7 +597,7 @@ var _ = Describe("PostgresqlDatabase tests", func() {
Expect(item.Status.Roles.Owner).To(Equal(sqlRole))

// Check owner role in DB
checkRoleInSQLDb(postgresUser, sqlRole)
checkRoleInSQLDb(sqlRole)

// Check DB ownership
isOwner, err := isRoleOwnerofSQLDB(pgdbDBName, sqlRole)
Expand All @@ -547,7 +622,7 @@ var _ = Describe("PostgresqlDatabase tests", func() {
Expect(item.Status.Roles.Writer).To(Equal(sqlRole))

// Check writer role in DB
checkRoleInSQLDb(postgresUser, sqlRole)
checkRoleInSQLDb(sqlRole)
})

It("should be ok to declare 1 schema", func() {
Expand Down Expand Up @@ -1650,7 +1725,7 @@ var _ = Describe("PostgresqlDatabase tests", func() {
Expect(ownerRoleExists).To(BeTrue())

// Check if default user has owner role in DB
checkRoleInSQLDb(postgresUser, masterRole)
checkRoleInSQLDb(masterRole)

// Check DB ownership
isOwner, err := isRoleOwnerofSQLDB(pgdbDBName, masterRole)
Expand Down Expand Up @@ -1751,7 +1826,7 @@ var _ = Describe("PostgresqlDatabase tests", func() {
Expect(exists).To(BeFalse())

// Check if default user has owner role in DB
checkRoleInSQLDb(postgresUser, masterRole)
checkRoleInSQLDb(masterRole)

// Check DB ownership
isOwner, err := isRoleOwnerofSQLDB(pgdbDBName, masterRole)
Expand Down Expand Up @@ -1848,7 +1923,7 @@ var _ = Describe("PostgresqlDatabase tests", func() {
Expect(ownerRoleExists).To(BeTrue())

// Check if default user has owner role in DB
checkRoleInSQLDb(postgresUser, masterRoleBis)
checkRoleInSQLDb(masterRoleBis)

// Check DB ownership
isOwner, err := isRoleOwnerofSQLDB(pgdbDBName, masterRoleBis)
Expand Down
Loading

0 comments on commit 5ac5247

Please sign in to comment.