Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(src/handler.ts): drop role with permissions #23

Merged

Conversation

ww-daniel-mora
Copy link
Contributor

@ww-daniel-mora ww-daniel-mora commented Jun 12, 2024

  • Added logging to failed sql commands
  • Allowed retry for concurrent delete
  • Increased retry for concurrency error

* Added logging to failed sql commands
* Allowed retry for concurrent delete
* Hardened drop role logic
* Increased retry for concurrency errors
@ww-daniel-mora
Copy link
Contributor Author

example concurrent delete error

2024-06-12 17:52:38 UTC:10.255.6.210(19770):postgres@anonymizedData:[2425]:ERROR:  tuple concurrently deleted

src/handler.ts Outdated
`DO $$
BEGIN
IF EXISTS (select from pg_catalog.pg_roles WHERE rolname = '%s') THEN
reassign OWNED by %I to posgres;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have a hard-coded role here, which people may not have. Also from that discussion reassigning can lead to privilege escalation, so not keen on that approach. Can you remove this from the PR? I can merge most of it, but not this bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From that thread, I suggest you add something like this:

DO $$ 
DECLARE
    r RECORD;
BEGIN
    FOR r IN (SELECT schemaname FROM pg_schemas) LOOP
      EXECUTE 'REVOKE ALL PRIVILEGES ON ALL TABLES IN SCHEMA ' || quote_ident(r.schemaname) || ' FROM %s';
      EXECUTE 'REVOKE ALL PRIVILEGES ON ALL SEQUENCES IN SCHEMA ' || quote_ident(r.schemaname) || ' FROM %s';
      EXECUTE 'REVOKE ALL PRIVILEGES ON ALL FUNCTIONS IN SCHEMA ' || quote_ident(r.schemaname) || ' FROM %s';
      EXECUTE 'REVOKE ALL ON SCHEMA ' || quote_ident(r.schemaname) || ' FROM %I';
    END LOOP;
END $$;

Typing this from memory, and needs testing.

And you could add:

REVOKE CONNECT ON DATABASE db_name FROM role_name;

Keep the drop OWNED by %I as the first statement.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this all needs a bit of work, I prefer it if you move this to a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sequences specifically is what cause my failure but I can work that into a seperate PR

* Use previous role removal behavior
@ww-daniel-mora
Copy link
Contributor Author

Ok for now I have reverted to the existing deletion behavior.

This change now

  • Adds logging when retrying
  • Expands retry filter to include concurrent delete
  • Doubles the max retry count

@berenddeboer berenddeboer merged commit d8dd27c into berenddeboer:main Jun 13, 2024
5 checks passed
@ww-daniel-mora ww-daniel-mora deleted the fix/drop-role-with-permissions branch June 13, 2024 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants