From 2ebac5226cbb901b928817bd3313b3cd97bb4063 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Wed, 31 May 2017 10:03:32 -0700 Subject: [PATCH] PostgreSQL: leaked pg privs (#14817) * Fix doc bug. Spell `collation` like `lc_collate`. * Whitespace nit in error message * Use %q as the format verb for error messages in postgresql_database resource messages. * REVOKE the `GRANT` given to the connection user when creating a database. For `ROLE`s who have been delegated `CREATEDB` privileges and are not a superuser, in order for them to `CREATE DATABASE` they need to be a member of the `ROLE` who will be `OWNER` for the new database. Once the `CREATE DATABASE` is complete, `REVOKE` the `GRANT` that was given to role so that the user who ran the `CREATE DATABASE` looses all privileges to the target database (unless of course they're a superuser). Fixes a regression introduced in #11452 * Delegated DBA ROLEs can now fix OWNER drift for PostgreSQL databases. Uses the helper functions introduced in #11452 --- .../resource_postgresql_database.go | 50 ++++++++++++++++--- .../r/postgresql_database.html.markdown | 2 +- 2 files changed, 43 insertions(+), 9 deletions(-) diff --git a/builtin/providers/postgresql/resource_postgresql_database.go b/builtin/providers/postgresql/resource_postgresql_database.go index 66f59fca1..81744d4ee 100644 --- a/builtin/providers/postgresql/resource_postgresql_database.go +++ b/builtin/providers/postgresql/resource_postgresql_database.go @@ -125,8 +125,15 @@ func resourcePostgreSQLDatabaseCreate(d *schema.ResourceData, meta interface{}) //needed in order to set the owner of the db if the connection user is not a superuser err = grantRoleMembership(conn, d.Get(dbOwnerAttr).(string), c.username) if err != nil { - return errwrap.Wrapf(fmt.Sprintf("Error granting role membership on database %s: {{err}}", dbName), err) + return errwrap.Wrapf(fmt.Sprintf("Error adding connection user (%q) to ROLE %q: {{err}}", c.username, d.Get(dbOwnerAttr).(string)), err) } + defer func() { + //undo the grant if the connection user is not a superuser + err = revokeRoleMembership(conn, d.Get(dbOwnerAttr).(string), c.username) + if err != nil { + err = errwrap.Wrapf(fmt.Sprintf("Error removing connection user (%q) from ROLE %q: {{err}}", c.username, d.Get(dbOwnerAttr).(string)), err) + } + }() // Handle each option individually and stream results into the query // buffer. @@ -190,12 +197,15 @@ func resourcePostgreSQLDatabaseCreate(d *schema.ResourceData, meta interface{}) query := b.String() _, err = conn.Query(query) if err != nil { - return errwrap.Wrapf(fmt.Sprintf("Error creating database %s: {{err}}", dbName), err) + return errwrap.Wrapf(fmt.Sprintf("Error creating database %q: {{err}}", dbName), err) } d.SetId(dbName) - return resourcePostgreSQLDatabaseReadImpl(d, meta) + // Set err outside of the return so that the deferred revoke can override err + // if necessary. + err = resourcePostgreSQLDatabaseReadImpl(d, meta) + return err } func resourcePostgreSQLDatabaseDelete(d *schema.ResourceData, meta interface{}) error { @@ -278,7 +288,7 @@ func resourcePostgreSQLDatabaseReadImpl(d *schema.ResourceData, meta interface{} err = conn.QueryRow("SELECT d.datname, pg_catalog.pg_get_userbyid(d.datdba) from pg_database d WHERE datname=$1", dbId).Scan(&dbName, &ownerName) switch { case err == sql.ErrNoRows: - log.Printf("[WARN] PostgreSQL database (%s) not found", dbId) + log.Printf("[WARN] PostgreSQL database (%q) not found", dbId) d.SetId("") return nil case err != nil: @@ -295,7 +305,7 @@ func resourcePostgreSQLDatabaseReadImpl(d *schema.ResourceData, meta interface{} ) switch { case err == sql.ErrNoRows: - log.Printf("[WARN] PostgreSQL database (%s) not found", dbId) + log.Printf("[WARN] PostgreSQL database (%q) not found", dbId) d.SetId("") return nil case err != nil: @@ -334,7 +344,7 @@ func resourcePostgreSQLDatabaseUpdate(d *schema.ResourceData, meta interface{}) return err } - if err := setDBOwner(conn, d); err != nil { + if err := setDBOwner(c, conn, d); err != nil { return err } @@ -380,7 +390,7 @@ func setDBName(conn *sql.DB, d *schema.ResourceData) error { return nil } -func setDBOwner(conn *sql.DB, d *schema.ResourceData) error { +func setDBOwner(c *Client, conn *sql.DB, d *schema.ResourceData) error { if !d.HasChange(dbOwnerAttr) { return nil } @@ -390,13 +400,26 @@ func setDBOwner(conn *sql.DB, d *schema.ResourceData) error { return nil } + //needed in order to set the owner of the db if the connection user is not a superuser + err := grantRoleMembership(conn, d.Get(dbOwnerAttr).(string), c.username) + if err != nil { + return errwrap.Wrapf(fmt.Sprintf("Error adding connection user (%q) to ROLE %q: {{err}}", c.username, d.Get(dbOwnerAttr).(string)), err) + } + defer func() { + // undo the grant if the connection user is not a superuser + err = revokeRoleMembership(conn, d.Get(dbOwnerAttr).(string), c.username) + if err != nil { + err = errwrap.Wrapf(fmt.Sprintf("Error removing connection user (%q) from ROLE %q: {{err}}", c.username, d.Get(dbOwnerAttr).(string)), err) + } + }() + dbName := d.Get(dbNameAttr).(string) query := fmt.Sprintf("ALTER DATABASE %s OWNER TO %s", pq.QuoteIdentifier(dbName), pq.QuoteIdentifier(owner)) if _, err := conn.Query(query); err != nil { return errwrap.Wrapf("Error updating database OWNER: {{err}}", err) } - return nil + return err } func setDBTablespace(conn *sql.DB, d *schema.ResourceData) error { @@ -485,3 +508,14 @@ func grantRoleMembership(conn *sql.DB, dbOwner string, connUsername string) erro } return nil } + +func revokeRoleMembership(conn *sql.DB, dbOwner string, connUsername string) error { + if dbOwner != "" && dbOwner != connUsername { + query := fmt.Sprintf("REVOKE %s FROM %s", pq.QuoteIdentifier(dbOwner), pq.QuoteIdentifier(connUsername)) + _, err := conn.Query(query) + if err != nil { + return errwrap.Wrapf("Error revoking membership: {{err}}", err) + } + } + return nil +} diff --git a/website/source/docs/providers/postgresql/r/postgresql_database.html.markdown b/website/source/docs/providers/postgresql/r/postgresql_database.html.markdown index 3ac0391c3..5110e0f50 100644 --- a/website/source/docs/providers/postgresql/r/postgresql_database.html.markdown +++ b/website/source/docs/providers/postgresql/r/postgresql_database.html.markdown @@ -20,7 +20,7 @@ resource "postgresql_database" "my_db" { name = "my_db" owner = "my_role" template = "template0" - collation = "C" + lc_collate = "C" connection_limit = -1 allow_connections = true }