From b89dbe5db688f09b13e63279769fe51632e76038 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Wed, 5 Oct 2016 13:09:51 -0400 Subject: [PATCH 1/3] Postgres revocation sql, beta mode --- builtin/logical/mysql/backend_test.go | 6 +- builtin/logical/mysql/path_roles.go | 10 +- builtin/logical/mysql/secret_creds.go | 17 +- builtin/logical/postgresql/backend_test.go | 60 ++++- .../logical/postgresql/path_role_create.go | 1 + builtin/logical/postgresql/path_roles.go | 27 ++- builtin/logical/postgresql/secret_creds.go | 228 +++++++++++------- 7 files changed, 238 insertions(+), 111 deletions(-) diff --git a/builtin/logical/mysql/backend_test.go b/builtin/logical/mysql/backend_test.go index 0d3442166e419..adbb5749c9b93 100644 --- a/builtin/logical/mysql/backend_test.go +++ b/builtin/logical/mysql/backend_test.go @@ -261,8 +261,8 @@ func testAccStepRole(t *testing.T, wildCard bool) logicaltest.TestStep { } } else { pathData = map[string]interface{}{ - "sql": testRoleHost, - "revoke_sql": testRevokeSQL, + "sql": testRoleHost, + "revocation_sql": testRevocationSQL, } } @@ -362,7 +362,7 @@ const testRoleHost = ` CREATE USER '{{name}}'@'10.1.1.2' IDENTIFIED BY '{{password}}'; GRANT SELECT ON *.* TO '{{name}}'@'10.1.1.2'; ` -const testRevokeSQL = ` +const testRevocationSQL = ` REVOKE ALL PRIVILEGES, GRANT OPTION FROM '{{name}}'@'10.1.1.2'; DROP USER '{{name}}'@'10.1.1.2'; ` diff --git a/builtin/logical/mysql/path_roles.go b/builtin/logical/mysql/path_roles.go index b6094fa3c3f31..97feccd2850d3 100644 --- a/builtin/logical/mysql/path_roles.go +++ b/builtin/logical/mysql/path_roles.go @@ -37,7 +37,7 @@ func pathRoles(b *backend) *framework.Path { Description: "SQL string to create a user. See help for more info.", }, - "revoke_sql": { + "revocation_sql": { Type: framework.TypeString, Description: "SQL string to revoke a user. See help for more info.", }, @@ -117,8 +117,8 @@ func (b *backend) pathRoleRead( return &logical.Response{ Data: map[string]interface{}{ - "sql": role.SQL, - "revoke_sql": role.RevokeSQL, + "sql": role.SQL, + "revocation_sql": role.RevocationSQL, }, }, nil } @@ -165,7 +165,7 @@ func (b *backend) pathRoleCreate( // Store it entry, err := logical.StorageEntryJSON("role/"+name, &roleEntry{ SQL: sql, - RevokeSQL: data.Get("revoke_sql").(string), + RevocationSQL: data.Get("revocation_sql").(string), UsernameLength: data.Get("username_length").(int), DisplaynameLength: data.Get("displayname_length").(int), RolenameLength: data.Get("rolename_length").(int), @@ -181,7 +181,7 @@ func (b *backend) pathRoleCreate( type roleEntry struct { SQL string `json:"sql" mapstructure:"sql" structs:"sql"` - RevokeSQL string `json:"revoke_sql" mapstructure:"revoke_sql" structs:"revoke_sql"` + RevocationSQL string `json:"revocation_sql" mapstructure:"revocation_sql" structs:"revocation_sql"` UsernameLength int `json:"username_length" mapstructure:"username_length" structs:"username_length"` DisplaynameLength int `json:"displayname_length" mapstructure:"displayname_length" structs:"displayname_length"` RolenameLength int `json:"rolename_length" mapstructure:"rolename_length" structs:"rolename_length"` diff --git a/builtin/logical/mysql/secret_creds.go b/builtin/logical/mysql/secret_creds.go index ecd90f233aa82..b8d651384e4ac 100644 --- a/builtin/logical/mysql/secret_creds.go +++ b/builtin/logical/mysql/secret_creds.go @@ -11,12 +11,12 @@ import ( const SecretCredsType = "creds" -// defaultRevokeSQL is a default SQL statement for revoking a user. Revoking +// defaultRevocationSQL is a default SQL statement for revoking a user. Revoking // permissions for the user is done before the drop, because MySQL explicitly // documents that open user connections will not be closed. By revoking all // grants, at least we ensure that the open connection is useless. Dropping the // user will only affect the next connection. -const defaultRevokeSQL = ` +const defaultRevocationSQL = ` REVOKE ALL PRIVILEGES, GRANT OPTION FROM '{{name}}'@'%'; DROP USER '{{name}}'@'%' ` @@ -34,11 +34,6 @@ func secretCreds(b *backend) *framework.Secret { Type: framework.TypeString, Description: "Password", }, - - "role": &framework.FieldSchema{ - Type: framework.TypeString, - Description: "Role", - }, }, Renew: b.secretCredsRenew, @@ -93,10 +88,10 @@ func (b *backend) secretCredsRevoke( } // Use a default SQL statement for revocation if one cannot be fetched from the role - revokeSQL := defaultRevokeSQL + revocationSQL := defaultRevocationSQL - if role != nil && role.RevokeSQL != "" { - revokeSQL = role.RevokeSQL + if role != nil && role.RevocationSQL != "" { + revocationSQL = role.RevocationSQL } else { if resp == nil { resp = &logical.Response{} @@ -111,7 +106,7 @@ func (b *backend) secretCredsRevoke( } defer tx.Rollback() - for _, query := range strutil.ParseArbitraryStringSlice(revokeSQL, ";") { + for _, query := range strutil.ParseArbitraryStringSlice(revocationSQL, ";") { query = strings.TrimSpace(query) if len(query) == 0 { continue diff --git a/builtin/logical/postgresql/backend_test.go b/builtin/logical/postgresql/backend_test.go index b9b64a6f2775b..2a632192ad4d5 100644 --- a/builtin/logical/postgresql/backend_test.go +++ b/builtin/logical/postgresql/backend_test.go @@ -233,6 +233,39 @@ func TestBackend_roleReadOnly(t *testing.T) { }) } +func TestBackend_roleReadOnly_revocationSQL(t *testing.T) { + config := logical.TestBackendConfig() + config.StorageView = &logical.InmemStorage{} + b, err := Factory(config) + if err != nil { + t.Fatal(err) + } + + cid, connURL := prepareTestContainer(t, config.StorageView, b) + if cid != "" { + defer cleanupTestContainer(t, cid) + } + connData := map[string]interface{}{ + "connection_url": connURL, + } + + logicaltest.Test(t, logicaltest.TestCase{ + Backend: b, + Steps: []logicaltest.TestStep{ + testAccStepConfig(t, connData, false), + testAccStepCreateRoleWithRevocationSQL(t, "web", testRole, defaultRevocationSQL, false), + testAccStepCreateRoleWithRevocationSQL(t, "web-readonly", testReadOnlyRole, defaultRevocationSQL, false), + testAccStepReadRole(t, "web-readonly", testReadOnlyRole), + testAccStepCreateTable(t, b, config.StorageView, "web", connURL), + testAccStepReadCreds(t, b, config.StorageView, "web-readonly", connURL), + testAccStepDropTable(t, b, config.StorageView, "web", connURL), + testAccStepDeleteRole(t, "web-readonly"), + testAccStepDeleteRole(t, "web"), + testAccStepReadRole(t, "web-readonly", ""), + }, + }) +} + func testAccStepConfig(t *testing.T, d map[string]interface{}, expectError bool) logicaltest.TestStep { return logicaltest.TestStep{ Operation: logical.UpdateOperation, @@ -273,6 +306,18 @@ func testAccStepCreateRole(t *testing.T, name string, sql string, expectFail boo } } +func testAccStepCreateRoleWithRevocationSQL(t *testing.T, name, sql, revocationSQL string, expectFail bool) logicaltest.TestStep { + return logicaltest.TestStep{ + Operation: logical.UpdateOperation, + Path: path.Join("roles", name), + Data: map[string]interface{}{ + "sql": sql, + "revocation_sql": revocationSQL, + }, + ErrorOk: expectFail, + } +} + func testAccStepDeleteRole(t *testing.T, name string) logicaltest.TestStep { return logicaltest.TestStep{ Operation: logical.DeleteOperation, @@ -341,6 +386,7 @@ func testAccStepReadCreds(t *testing.T, b logical.Backend, s logical.Storage, na InternalData: map[string]interface{}{ "secret_type": "creds", "username": d.Username, + "role": name, }, }, }) @@ -543,7 +589,8 @@ GRANT CONNECT ON DATABASE "postgres" TO "{{name}}"; ` var testBlockStatementRoleSlice = []string{ - `DO $$ + ` +DO $$ BEGIN IF NOT EXISTS (SELECT * FROM pg_catalog.pg_roles WHERE rolname='foo-role') THEN CREATE ROLE "foo-role"; @@ -556,9 +603,18 @@ BEGIN GRANT ALL PRIVILEGES ON ALL FUNCTIONS IN SCHEMA foo TO "foo-role"; END IF; END -$$`, +$$ +`, `CREATE ROLE "{{name}}" WITH LOGIN PASSWORD '{{password}}' VALID UNTIL '{{expiration}}';`, `GRANT "foo-role" TO "{{name}}";`, `ALTER ROLE "{{name}}" SET search_path = foo;`, `GRANT CONNECT ON DATABASE "postgres" TO "{{name}}";`, } + +const defaultRevocationSQL = ` +REVOKE ALL PRIVILEGES ON ALL TABLES IN SCHEMA public FROM {{name}}; +REVOKE ALL PRIVILEGES ON ALL SEQUENCES IN SCHEMA public FROM {{name}}; +REVOKE USAGE ON SCHEMA public FROM {{name}}; + +DROP ROLE IF EXISTS {{name}}; +` diff --git a/builtin/logical/postgresql/path_role_create.go b/builtin/logical/postgresql/path_role_create.go index 38a4c5fb6ef13..5ca92c431bc2c 100644 --- a/builtin/logical/postgresql/path_role_create.go +++ b/builtin/logical/postgresql/path_role_create.go @@ -140,6 +140,7 @@ func (b *backend) pathRoleCreateRead( "password": password, }, map[string]interface{}{ "username": username, + "role": name, }) resp.Secret.TTL = lease.Lease return resp, nil diff --git a/builtin/logical/postgresql/path_roles.go b/builtin/logical/postgresql/path_roles.go index e1851ee2eeea3..0fd1ff8968f0d 100644 --- a/builtin/logical/postgresql/path_roles.go +++ b/builtin/logical/postgresql/path_roles.go @@ -26,15 +26,20 @@ func pathRoles(b *backend) *framework.Path { return &framework.Path{ Pattern: "roles/" + framework.GenericNameRegex("name"), Fields: map[string]*framework.FieldSchema{ - "name": &framework.FieldSchema{ + "name": { Type: framework.TypeString, Description: "Name of the role.", }, - "sql": &framework.FieldSchema{ + "sql": { Type: framework.TypeString, Description: "SQL string to create a user. See help for more info.", }, + + "revocation_sql": { + Type: framework.TypeString, + Description: "SQL string to revoke a user. This is in beta; use with caution.", + }, }, Callbacks: map[logical.Operation]framework.OperationFunc{ @@ -85,11 +90,19 @@ func (b *backend) pathRoleRead( return nil, nil } - return &logical.Response{ + resp := &logical.Response{ Data: map[string]interface{}{ "sql": role.SQL, }, - }, nil + } + + // This is separate because this is in beta in 0.6.2, so we don't want it + // to show up in the general case. + if role.RevocationSQL != "" { + resp.Data["revocation_sql"] = role.RevocationSQL + } + + return resp, nil } func (b *backend) pathRoleList( @@ -134,7 +147,8 @@ func (b *backend) pathRoleCreate( // Store it entry, err := logical.StorageEntryJSON("role/"+name, &roleEntry{ - SQL: sql, + SQL: sql, + RevocationSQL: data.Get("revocation_sql").(string), }) if err != nil { return nil, err @@ -147,7 +161,8 @@ func (b *backend) pathRoleCreate( } type roleEntry struct { - SQL string `json:"sql"` + SQL string `json:"sql" mapstructure:"sql" structs:"sql"` + RevocationSQL string `json:"revocation_sql" mapstructure:"revocation_sql" structs:"revocation_sql"` } const pathRoleHelpSyn = ` diff --git a/builtin/logical/postgresql/secret_creds.go b/builtin/logical/postgresql/secret_creds.go index 3c771457cb4eb..120104500d01d 100644 --- a/builtin/logical/postgresql/secret_creds.go +++ b/builtin/logical/postgresql/secret_creds.go @@ -3,7 +3,9 @@ package postgresql import ( "database/sql" "fmt" + "strings" + "github.com/hashicorp/vault/helper/strutil" "github.com/hashicorp/vault/logical" "github.com/hashicorp/vault/logical/framework" "github.com/lib/pq" @@ -91,120 +93,178 @@ func (b *backend) secretCredsRevoke( } username, ok := usernameRaw.(string) + var revocationSQL string + var resp *logical.Response + + roleNameRaw, ok := req.Secret.InternalData["role"] + if ok { + role, err := b.Role(req.Storage, roleNameRaw.(string)) + if err != nil { + return nil, err + } + if role == nil { + if resp == nil { + resp = &logical.Response{} + } + resp.AddWarning(fmt.Sprintf("Role %q cannot be found. Using default revocation SQL.", roleNameRaw.(string))) + } else { + revocationSQL = role.RevocationSQL + } + } + // Get our connection db, err := b.DB(req.Storage) if err != nil { return nil, err } - // Check if the role exists - var exists bool - err = db.QueryRow("SELECT exists (SELECT rolname FROM pg_roles WHERE rolname=$1);", username).Scan(&exists) - if err != nil && err != sql.ErrNoRows { - return nil, err - } + switch revocationSQL { - if exists == false { - return nil, nil - } + // This is the default revocation logic. If revocation SQL is provided it + // is simply executed as-is. + case "": + // Check if the role exists + var exists bool + err = db.QueryRow("SELECT exists (SELECT rolname FROM pg_roles WHERE rolname=$1);", username).Scan(&exists) + if err != nil && err != sql.ErrNoRows { + return nil, err + } - // Query for permissions; we need to revoke permissions before we can drop - // the role - // This isn't done in a transaction because even if we fail along the way, - // we want to remove as much access as possible - stmt, err := db.Prepare("SELECT DISTINCT table_schema FROM information_schema.role_column_grants WHERE grantee=$1;") - if err != nil { - return nil, err - } - defer stmt.Close() + if exists == false { + return resp, nil + } - rows, err := stmt.Query(username) - if err != nil { - return nil, err - } - defer rows.Close() + // Query for permissions; we need to revoke permissions before we can drop + // the role + // This isn't done in a transaction because even if we fail along the way, + // we want to remove as much access as possible + stmt, err := db.Prepare("SELECT DISTINCT table_schema FROM information_schema.role_column_grants WHERE grantee=$1;") + if err != nil { + return nil, err + } + defer stmt.Close() - const initialNumRevocations = 16 - revocationStmts := make([]string, 0, initialNumRevocations) - for rows.Next() { - var schema string - err = rows.Scan(&schema) + rows, err := stmt.Query(username) if err != nil { - // keep going; remove as many permissions as possible right now - continue + return nil, err } + defer rows.Close() + + const initialNumRevocations = 16 + revocationStmts := make([]string, 0, initialNumRevocations) + for rows.Next() { + var schema string + err = rows.Scan(&schema) + if err != nil { + // keep going; remove as many permissions as possible right now + continue + } + revocationStmts = append(revocationStmts, fmt.Sprintf( + `REVOKE ALL PRIVILEGES ON ALL TABLES IN SCHEMA %s FROM %s;`, + pq.QuoteIdentifier(schema), + pq.QuoteIdentifier(username))) + + revocationStmts = append(revocationStmts, fmt.Sprintf( + `REVOKE USAGE ON SCHEMA %s FROM %s;`, + pq.QuoteIdentifier(schema), + pq.QuoteIdentifier(username))) + } + + // for good measure, revoke all privileges and usage on schema public revocationStmts = append(revocationStmts, fmt.Sprintf( - `REVOKE ALL PRIVILEGES ON ALL TABLES IN SCHEMA %s FROM %s;`, - pq.QuoteIdentifier(schema), + `REVOKE ALL PRIVILEGES ON ALL TABLES IN SCHEMA public FROM %s;`, pq.QuoteIdentifier(username))) revocationStmts = append(revocationStmts, fmt.Sprintf( - `REVOKE USAGE ON SCHEMA %s FROM %s;`, - pq.QuoteIdentifier(schema), + "REVOKE ALL PRIVILEGES ON ALL SEQUENCES IN SCHEMA public FROM %s;", pq.QuoteIdentifier(username))) - } - // for good measure, revoke all privileges and usage on schema public - revocationStmts = append(revocationStmts, fmt.Sprintf( - `REVOKE ALL PRIVILEGES ON ALL TABLES IN SCHEMA public FROM %s;`, - pq.QuoteIdentifier(username))) + revocationStmts = append(revocationStmts, fmt.Sprintf( + "REVOKE USAGE ON SCHEMA public FROM %s;", + pq.QuoteIdentifier(username))) - revocationStmts = append(revocationStmts, fmt.Sprintf( - "REVOKE ALL PRIVILEGES ON ALL SEQUENCES IN SCHEMA public FROM %s;", - pq.QuoteIdentifier(username))) + // get the current database name so we can issue a REVOKE CONNECT for + // this username + var dbname sql.NullString + if err := db.QueryRow("SELECT current_database();").Scan(&dbname); err != nil { + return nil, err + } - revocationStmts = append(revocationStmts, fmt.Sprintf( - "REVOKE USAGE ON SCHEMA public FROM %s;", - pq.QuoteIdentifier(username))) + if dbname.Valid { + revocationStmts = append(revocationStmts, fmt.Sprintf( + `REVOKE CONNECT ON DATABASE %s FROM %s;`, + pq.QuoteIdentifier(dbname.String), + pq.QuoteIdentifier(username))) + } - // get the current database name so we can issue a REVOKE CONNECT for - // this username - var dbname sql.NullString - if err := db.QueryRow("SELECT current_database();").Scan(&dbname); err != nil { - return nil, err - } + // again, here, we do not stop on error, as we want to remove as + // many permissions as possible right now + var lastStmtError error + for _, query := range revocationStmts { + stmt, err := db.Prepare(query) + if err != nil { + lastStmtError = err + continue + } + defer stmt.Close() + _, err = stmt.Exec() + if err != nil { + lastStmtError = err + } + } - if dbname.Valid { - revocationStmts = append(revocationStmts, fmt.Sprintf( - `REVOKE CONNECT ON DATABASE %s FROM %s;`, - pq.QuoteIdentifier(dbname.String), - pq.QuoteIdentifier(username))) - } + // can't drop if not all privileges are revoked + if rows.Err() != nil { + return nil, fmt.Errorf("could not generate revocation statements for all rows: %s", rows.Err()) + } + if lastStmtError != nil { + return nil, fmt.Errorf("could not perform all revocation statements: %s", lastStmtError) + } - // again, here, we do not stop on error, as we want to remove as - // many permissions as possible right now - var lastStmtError error - for _, query := range revocationStmts { - stmt, err := db.Prepare(query) + // Drop this user + stmt, err = db.Prepare(fmt.Sprintf( + `DROP ROLE IF EXISTS %s;`, pq.QuoteIdentifier(username))) if err != nil { - lastStmtError = err - continue + return nil, err } defer stmt.Close() - _, err = stmt.Exec() + if _, err := stmt.Exec(); err != nil { + return nil, err + } + + // We have revocation SQL, execute directly, within a transaction + default: + tx, err := db.Begin() if err != nil { - lastStmtError = err + return nil, err } - } + defer func() { + tx.Rollback() + }() - // can't drop if not all privileges are revoked - if rows.Err() != nil { - return nil, fmt.Errorf("could not generate revocation statements for all rows: %s", rows.Err()) - } - if lastStmtError != nil { - return nil, fmt.Errorf("could not perform all revocation statements: %s", lastStmtError) - } + for _, query := range strutil.ParseArbitraryStringSlice(revocationSQL, ";") { + query = strings.TrimSpace(query) + if len(query) == 0 { + continue + } - // Drop this user - stmt, err = db.Prepare(fmt.Sprintf( - `DROP ROLE IF EXISTS %s;`, pq.QuoteIdentifier(username))) - if err != nil { - return nil, err - } - defer stmt.Close() - if _, err := stmt.Exec(); err != nil { - return nil, err + stmt, err := tx.Prepare(Query(query, map[string]string{ + "name": pq.QuoteIdentifier(username), + })) + if err != nil { + return nil, err + } + defer stmt.Close() + + if _, err := stmt.Exec(); err != nil { + return nil, err + } + } + + if err := tx.Commit(); err != nil { + return nil, err + } } - return nil, nil + return resp, nil } From 7e2add9fbb71f21cc516a417aca163f28802fd53 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Wed, 5 Oct 2016 13:17:45 -0400 Subject: [PATCH 2/3] Add revocation_sql parameter to mysql website docs --- website/source/docs/secrets/mysql/index.html.md | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/website/source/docs/secrets/mysql/index.html.md b/website/source/docs/secrets/mysql/index.html.md index 9640b247b920f..7703436e51335 100644 --- a/website/source/docs/secrets/mysql/index.html.md +++ b/website/source/docs/secrets/mysql/index.html.md @@ -248,12 +248,20 @@ the default on versions prior to that.
  • sql required - The SQL statements executed to create and configure the role. Must be - a semicolon-separated string, a base64-encoded semicolon-separated + The SQL statements executed to create and configure a user. Must be a + semicolon-separated string, a base64-encoded semicolon-separated string, a serialized JSON string array, or a base64-encoded serialized JSON string array. The '{{name}}' and '{{password}}' values will be substituted.
    • +
  • + revocation_sql + optional + The SQL statements executed to revoke a user. Must be a + semicolon-separated string, a base64-encoded semicolon-separated + string, a serialized JSON string array, or a base64-encoded serialized + JSON string array. The '{{name}}' value will be substituted. +
  • rolename_length optional From dc158c3272eb673cc90d43475d9c0896d1f11138 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Wed, 5 Oct 2016 13:20:39 -0400 Subject: [PATCH 3/3] Update comment --- builtin/logical/postgresql/path_roles.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/logical/postgresql/path_roles.go b/builtin/logical/postgresql/path_roles.go index 0fd1ff8968f0d..6eb00ffb68825 100644 --- a/builtin/logical/postgresql/path_roles.go +++ b/builtin/logical/postgresql/path_roles.go @@ -96,8 +96,8 @@ func (b *backend) pathRoleRead( }, } - // This is separate because this is in beta in 0.6.2, so we don't want it - // to show up in the general case. + // TODO: This is separate because this is in beta in 0.6.2, so we don't + // want it to show up in the general case. if role.RevocationSQL != "" { resp.Data["revocation_sql"] = role.RevocationSQL }