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.

Sign up for GitHub

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

Jump to bottom

Allow non-superusers to run ALTER TABLE REROUTE commands #15891

Closed
hlcianfagna opened this issue Apr 23, 2024 · 5 comments · Fixed by #16139
Closed

Allow non-superusers to run ALTER TABLE REROUTE commands #15891

hlcianfagna opened this issue Apr 23, 2024 · 5 comments · Fixed by #16139
Assignees
@matriv
Labels
bug Clear identification of incorrect behaviour

Comments

@hlcianfagna
Copy link
Contributor

hlcianfagna commented Apr 23, 2024 •
edited

Problem Statement

In CrateDB Cloud and other security-sensitive setups, database admins may not routinely have access to the crate superuser account.
The ALTER TABLE ... REROUTE MOVE SHARD commands are useful to move around "hot shards" as described in the documentation.
Currently these operations are not allowed even when a user account has AL and DDL permissions at cluster level.

Possible Solutions

  • Allow the operations for accounts with certain permissions so that the task can be delegated and run without connecting as crate
  • Cover REROUTE use cases in some other way so that manual rerouting is not necessary

Considered Alternatives

Get an admin with access to the CrateDB nodes to connect locally as crate and run the required commands.
@mfussenegger
Copy link
Member

mfussenegger commented May 27, 2024 •
edited

TBD which option to use:

@proddata proddata mentioned this issue May 27, 2024
Open
@matriv
Copy link
Contributor

matriv commented Jun 7, 2024

Imho, users with AL should be able to execute this. The ability to inherit superuser to a role/user, would be a way to do that, but I believe explicit granting of AL should also work.

@BaurzhanSakhariev
Copy link
Contributor

BaurzhanSakhariev commented Jun 10, 2024 •
edited

Imho, users with AL should be able to execute this. The ability to inherit superuser to a role/user, would be a way to do that, but I believe explicit granting of AL should also work.

Makes sense to me, as we already allow non-superusers to run ALTER CLUSTER commands (#11283)

ALTER CLUSTER is a more "global" command so it's probably logical for users to expect
that they can also run ALTER TABLE REROUTE with AL privileges.

@matriv
Copy link
Contributor

matriv commented Jun 11, 2024

As discussed, we should allow ALTER TABLE <table> REROUTE... for DDL privs on the table.
Since you can already do other admin actions with DDL on a table (increase, decrease shards, change settings that affect the shard distribution), it makes sense to also allow REROUTE.

We decided also to treat this as a bug, as according to our current docs, we allow users to DDL to perform any ALTER TABLE operation.

@matriv matriv added bug Clear identification of incorrect behaviour and removed needs discussion feature: administration labels Jun 11, 2024
@matriv matriv self-assigned this Jun 11, 2024
matriv added a commit that referenced this issue Jun 11, 2024
@matriv
Fix: allow users with DDL on table to run ALTER TABLE REROUTE
661e261 
In docs, we don't mention any exception for the REROUTE statements,
we've just missed to check for the correct privileges for this type
of statements.

- Add the required methods to the access to
`AccessControlImpl#StatementVisitor`.

- Add `ident()` to `ShardedTable` iface used in the corresponding
  analyzed statements.

- Extracted a method which ensures DDL on table, as it's being used
  in multiple places

Fixes: #15891
@matriv matriv mentioned this issue Jun 11, 2024
Merged
matriv added a commit that referenced this issue Jun 11, 2024
@matriv
Fix: allow users with DDL on table to run ALTER TABLE REROUTE
5873557 
In docs, we don't mention any exception for the REROUTE statements,
we've just missed to check for the correct privileges for this type
of statements.

- Add the required methods to the access to
`AccessControlImpl#StatementVisitor`.

- Add `ident()` to `ShardedTable` iface used in the corresponding
  analyzed statements.

- Extracted a method which ensures DDL on table, as it's being used
  in multiple places

Fixes: #15891
matriv added a commit that referenced this issue Jun 11, 2024
@matriv
Fix: allow users with DDL on table to run ALTER TABLE REROUTE
ae59b88 
In docs, we don't mention any exception for the REROUTE statements,
we've just missed to check for the correct privileges for this type
of statements.

- Add the required methods to the access to
`AccessControlImpl#StatementVisitor`.

- Add `ident()` to `ShardedTable` iface used in the corresponding
  analyzed statements.

- Extracted a method which ensures DDL on table, as it's being used
  in multiple places

Fixes: #15891
@mergify mergify bot closed this as completed in #16139 Jun 11, 2024
mergify bot pushed a commit that referenced this issue Jun 11, 2024
@matriv @mergify
Fix: allow users with DDL on table to run ALTER TABLE REROUTE
3b37290 
In docs, we don't mention any exception for the REROUTE statements,
we've just missed to check for the correct privileges for this type
of statements.

- Add the required methods to the access to
`AccessControlImpl#StatementVisitor`.

- Add `ident()` to `ShardedTable` iface used in the corresponding
  analyzed statements.

- Extracted a method which ensures DDL on table, as it's being used
  in multiple places

Fixes: #15891
mergify bot pushed a commit that referenced this issue Jun 11, 2024
@matriv @mergify
Fix: allow users with DDL on table to run ALTER TABLE REROUTE
ec04821 
In docs, we don't mention any exception for the REROUTE statements,
we've just missed to check for the correct privileges for this type
of statements.

- Add the required methods to the access to
`AccessControlImpl#StatementVisitor`.

- Add `ident()` to `ShardedTable` iface used in the corresponding
  analyzed statements.

- Extracted a method which ensures DDL on table, as it's being used
  in multiple places

Fixes: #15891
(cherry picked from commit 3b37290)

# Conflicts:
#	server/src/main/java/io/crate/auth/AccessControlImpl.java
@mergify mergify bot mentioned this issue Jun 11, 2024
Merged
@matriv
Copy link
Contributor

matriv commented Jun 11, 2024 •
edited

Thank you @hlcianfagna for reporting this. It has been addressed as a bug fix and will be available with the next hotfix release.

matriv added a commit that referenced this issue Jun 11, 2024
@matriv
Fix: allow users with DDL on table to run ALTER TABLE REROUTE
3380613 
In docs, we don't mention any exception for the REROUTE statements,
we've just missed to check for the correct privileges for this type
of statements.

- Add the required methods to the access to
`AccessControlImpl#StatementVisitor`.

- Add `ident()` to `ShardedTable` iface used in the corresponding
  analyzed statements.

- Extracted a method which ensures DDL on table, as it's being used
  in multiple places

Fixes: #15891
(cherry picked from commit 3b37290)

# Conflicts:
#	server/src/main/java/io/crate/auth/AccessControlImpl.java
matriv added a commit that referenced this issue Jun 11, 2024
@matriv
Fix: allow users with DDL on table to run ALTER TABLE REROUTE
736f45d 
In docs, we don't mention any exception for the REROUTE statements,
we've just missed to check for the correct privileges for this type
of statements.

- Add the required methods to the access to
`AccessControlImpl#StatementVisitor`.

- Add `ident()` to `ShardedTable` iface used in the corresponding
  analyzed statements.

- Extracted a method which ensures DDL on table, as it's being used
  in multiple places

Fixes: #15891
(cherry picked from commit 3b37290)

# Conflicts:
#	server/src/main/java/io/crate/auth/AccessControlImpl.java
matriv added a commit that referenced this issue Jun 11, 2024
@matriv
Fix: allow users with DDL on table to run ALTER TABLE REROUTE
b2d9b6c 
In docs, we don't mention any exception for the REROUTE statements,
we've just missed to check for the correct privileges for this type
of statements.

- Add the required methods to the access to
`AccessControlImpl#StatementVisitor`.

- Add `ident()` to `ShardedTable` iface used in the corresponding
  analyzed statements.

- Extracted a method which ensures DDL on table, as it's being used
  in multiple places

Fixes: #15891
(cherry picked from commit 3b37290)

# Conflicts:
#	server/src/main/java/io/crate/auth/AccessControlImpl.java
mergify bot pushed a commit that referenced this issue Jun 11, 2024
@matriv @mergify
Fix: allow users with DDL on table to run ALTER TABLE REROUTE
db3e8a7 
In docs, we don't mention any exception for the REROUTE statements,
we've just missed to check for the correct privileges for this type
of statements.

- Add the required methods to the access to
`AccessControlImpl#StatementVisitor`.

- Add `ident()` to `ShardedTable` iface used in the corresponding
  analyzed statements.

- Extracted a method which ensures DDL on table, as it's being used
  in multiple places

Fixes: #15891
(cherry picked from commit 3b37290)

# Conflicts:
#	server/src/main/java/io/crate/auth/AccessControlImpl.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@matriv matriv

Labels
bug Clear identification of incorrect behaviour
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix: allow users with DDL on table to run ALTER TABLE REROUTE crate/crate
4 participants
@mfussenegger @matriv @BaurzhanSakhariev @hlcianfagna