diff --git a/modules/postgres/README.md b/modules/postgres/README.md index 7593f125..10a9ad2e 100644 --- a/modules/postgres/README.md +++ b/modules/postgres/README.md @@ -68,8 +68,11 @@ for, so one removed by hand is installed again. what stands in the way. Declare columns by name where a table mixes the two. Every minute, on every database, the reader's role memberships are revoked and its grants brought to exactly that — each grant revoked as its own grantor, since a superuser's revoke acts as the table's - owner and leaves a grant another role made — and then read back: anything the reader still holds that - is not declared is reported as a failure, never as revoked. An existing install is narrowed without + owner and leaves a grant another role made — and then read back: only what the read-back shows gone + is reported as revoked, and anything the reader still holds that is not declared is reported as a + failure, with the reason. A reader that was given a privilege with grant option and passed it on makes + a plain revoke fail ("dependent privileges exist"); the pass then revokes with CASCADE, which reaches + only the grants that came from the reader. An existing install is narrowed without anybody acting, and a narrowed declaration takes effect at the next pass. The role itself is altered only when an attribute, a setting or its password has drifted, so the password is not sent every minute. Before a caller's statement runs, the reader's own session is asked which secret-named diff --git a/modules/postgres/cmd/postgres-provider/readable.go b/modules/postgres/cmd/postgres-provider/readable.go index aa284f1f..41d241d5 100644 --- a/modules/postgres/cmd/postgres-provider/readable.go +++ b/modules/postgres/cmd/postgres-provider/readable.go @@ -35,6 +35,7 @@ package main import ( "context" "encoding/json" + "errors" "fmt" "os" "regexp" @@ -42,6 +43,8 @@ import ( "strings" "sync" "time" + + "github.com/jackc/pgx/v5/pgconn" ) // ReadableTable is one entry of a contribution's `readable`. @@ -543,6 +546,40 @@ func (c *Client) LastReaderReport() ReaderReport { return c.reader.last } +// dependentPrivileges says whether the server refused a revoke because grants depend on it +// (dependent_privileges_exist): the reader passed on a privilege it holds with grant option. +func dependentPrivileges(err error) bool { + var pg *pgconn.PgError + return errors.As(err, &pg) && pg.Code == "2BP01" +} + +// cascaded is the change with every REVOKE made CASCADE. +func cascaded(stmts []string) []string { + out := make([]string, len(stmts)) + for i, s := range stmts { + if strings.HasPrefix(s, "REVOKE ") { + s += " CASCADE" + } + out[i] = s + } + return out +} + +// gone is what was held before and is not held now. +func gone(before, left []string) []string { + still := map[string]bool{} + for _, l := range left { + still[l] = true + } + var out []string + for _, b := range before { + if !still[b] { + out = append(out, b) + } + } + return out +} + // readHeld is every privilege the reader holds in one database, table-level and column-level, each // with its grantor. func (c *Client) readHeld(ctx context.Context, db string) (held, error) { @@ -612,21 +649,34 @@ func (c *Client) reconcileDatabase(ctx context.Context, db string, decl []Readab } stmts, rep := planDatabase(decl, existing, h, noUsage) rep.Database, rep.Declared = db, decl != nil + // What was revoked is what the read-back shows gone, never what was asked for. + rep.Revoked = nil if len(stmts) > 0 { + want := wantedColumns(decl, existing, &DatabaseReport{}) + before := unwanted(h, want) // One string: the simple protocol runs it as one implicit transaction, all or nothing. - if _, err := c.Query(ctx, db, strings.Join(stmts, "; ")); err != nil { - rep.Error = "changing the reader's grants: " + err.Error() - return rep + _, changeErr := c.Query(ctx, db, strings.Join(stmts, "; ")) + if changeErr != nil && dependentPrivileges(changeErr) { + // The reader holds a grant option and passed the privilege on: a plain revoke refuses + // while those grants stand. CASCADE takes them too — they all came from the reader, since + // only grants depending on the reader's grant option depend on what is revoked here. + _, changeErr = c.Query(ctx, db, strings.Join(cascaded(stmts), "; ")) } - // Read back: what the reader still holds beyond what is wanted is a failure, said as one — - // never reported as revoked while it stands. after, err := c.readHeld(ctx, db) if err != nil { rep.Error = "reading the reader's grants back: " + err.Error() return rep } - if left := unwanted(after, wantedColumns(decl, existing, &DatabaseReport{})); len(left) > 0 { - rep.Error = "STILL HELD after revoking: the reader can still use " + strings.Join(left, ", ") + left := unwanted(after, want) + rep.Revoked = gone(before, left) + if len(left) > 0 { + why := "the revoke ran and they stand" + if changeErr != nil { + why = "changing the reader's grants failed: " + changeErr.Error() + } + rep.Error = "STILL HELD: the reader can still use " + strings.Join(left, ", ") + " (" + why + ")" + } else if changeErr != nil { + rep.Error = "changing the reader's grants: " + changeErr.Error() } } // Asked as the admin on the reader's behalf: whatever else gives the reader a secret (PUBLIC, a diff --git a/modules/postgres/cmd/postgres-provider/readable_live_test.go b/modules/postgres/cmd/postgres-provider/readable_live_test.go index ebef9714..de746e5f 100644 --- a/modules/postgres/cmd/postgres-provider/readable_live_test.go +++ b/modules/postgres/cmd/postgres-provider/readable_live_test.go @@ -181,3 +181,51 @@ func TestLiveAGrantByAnotherGrantorIsTakenBackAndDefinersAreSaid(t *testing.T) { } } } + +// The reader was given a table WITH GRANT OPTION and passed it on. A plain revoke then fails with +// "dependent privileges exist" and changes nothing; the pass must take back the reader's grant and +// what the reader passed on (CASCADE reaches only grants that came from the reader), report as +// revoked only what the read-back shows gone, and say what is still held and why if anything is. +func TestLiveAGrantTheReaderPassedOnIsTakenBack(t *testing.T) { + c := liveClient(t) + freshStore(t, c) + defer dropStore(c) + defer func() { _, _ = c.Query(ctx, "", "DROP ROLE IF EXISTS mesh_test_onward") }() + if _, err := c.Query(ctx, "", "DROP ROLE IF EXISTS mesh_test_onward; CREATE ROLE mesh_test_onward"); err != nil { + t.Fatal(err) + } + for _, sql := range []string{ + "REVOKE ALL ON vault FROM " + Ident(Reader), + "GRANT SELECT ON notes TO " + Ident(Reader) + " WITH GRANT OPTION", + "SET ROLE " + Ident(Reader) + "; GRANT SELECT ON notes TO mesh_test_onward; RESET ROLE", + } { + if _, err := c.Query(ctx, liveStore, sql); err != nil { + t.Fatal(sql, err) + } + } + var mine DatabaseReport + for _, db := range c.ReconcileReader(ctx, Declarations{}, "").Databases { + if db.Database == liveStore { + mine = db + } + } + if mine.Error != "" { + t.Fatal("left standing:", mine.Error) + } + if columnReadable(t, c, liveStore, "notes", "body") { + t.Fatalf("the reader still reads notes, and the pass said revoked %v", mine.Revoked) + } + if strings.Join(mine.Revoked, ",") != "public.notes" { + t.Fatalf("revoked %v", mine.Revoked) + } + r, err := c.Query(ctx, liveStore, "SELECT has_table_privilege('mesh_test_onward', 'notes', 'SELECT')") + if err != nil || cell(r.Rows[0], 0) != "f" { + t.Fatal("what the reader passed on survived:", r.Rows, err) + } + // The next pass finds nothing held, and says nothing revoked. + for _, db := range c.ReconcileReader(ctx, Declarations{}, "").Databases { + if db.Database == liveStore && (len(db.Revoked) > 0 || db.Error != "") { + t.Fatalf("%+v", db) + } + } +} diff --git a/modules/postgres/cmd/postgres-provider/readable_test.go b/modules/postgres/cmd/postgres-provider/readable_test.go index 28f81095..aa5ea0fb 100644 --- a/modules/postgres/cmd/postgres-provider/readable_test.go +++ b/modules/postgres/cmd/postgres-provider/readable_test.go @@ -6,6 +6,7 @@ package main import ( "encoding/json" + "errors" "os" "path/filepath" "strings" @@ -354,3 +355,26 @@ func TestTheCataloguesReadableDeclarationsParse(t *testing.T) { } } } + +// A change that fails reports nothing revoked, and says what is still held and why — every pass, not +// "revoked" every pass. +func TestAFailedChangeSaysWhatIsStillHeldAndRevokesNothing(t *testing.T) { + conn := admin + conn.ReaderPassword = "r" + f, c := newFake(conn) + f.answer(`^SELECT rolsuper`, []string{"a", "b", "c", "d", "e", "f", "g", "h", "i"}, + []string{"f", "f", "f", "f", "f", "t", "t", "t", "default_transaction_read_only=on,statement_timeout=60s"}) + f.answer(`SELECT datname FROM pg_database WHERE datallowconn`, []string{"datname"}, []string{"app"}) + f.answer(`aclexplode\(c\.relacl\)`, []string{"nspname", "relname", "relkind", "privilege_type", "grantor"}, + []string{"public", "vault", "r", "SELECT", "owner1"}) + f.answer(`aclexplode\(a\.attacl\)`, []string{"nspname", "relname", "attname", "privilege_type", "grantor"}) + f.fail(`^SET ROLE`, errors.New("server busy")) + rep := c.ReconcileReader(ctx, Declarations{}, "") + db := rep.Databases[0] + if len(db.Revoked) != 0 { + t.Fatalf("said revoked: %v", db.Revoked) + } + if !strings.Contains(db.Error, "STILL HELD") || !strings.Contains(db.Error, "public.vault") || !strings.Contains(db.Error, "server busy") { + t.Fatal(db.Error) + } +}