Fix GRANT/REVOKE ON quoting for ClickHouse wildcard patterns#141
Fix GRANT/REVOKE ON quoting for ClickHouse wildcard patterns#141gregtuc wants to merge 2 commits intoClickHouse:mainfrom
Conversation
|
Thanks for the contribution @gregtuc . I will test and review it. |
nunoadrego
left a comment
There was a problem hiding this comment.
@gregtuc, we are missing the identifierOrPattern in GetGrantPrivilege, so we can't read from system.grants in the new format.
|
|
||
| // Only treat a trailing `*` as a wildcard pattern (prefix match), in line with | ||
| // ClickHouse wildcard grant rules. | ||
| if strings.HasSuffix(s, "*") && strings.Count(s, "*") == 1 && !strings.HasPrefix(s, "*") { |
There was a problem hiding this comment.
Why this: strings.Count(s, "*") == 1
What happens if we have something like: db*.*?
There was a problem hiding this comment.
Thanks for the review @nunoadrego!
On your first point: identifierOrPattern in GetGrantPrivilege
I don't think we need it there. The read path goes through ClassicGrantMatcher, which queries system.grants with a WHERE clause:
`database` = 'prefix_*'That comes from WhereEquals -> quote(), so it's a single-quoted value comparison. identifierOrPattern is specifically for the GRANT/REVOKE ON clause where backtick-quoting would break wildcard matching. In a WHERE clause, 'prefix_*' is just a string literal being compared against what ClickHouse stored, and ClickHouse stores wildcard patterns in system.grants.
You can see this already working in overlaps.go, which reads grants back via GetAllGrantsForGrantee and checks for strings.HasSuffix(*existing.DatabaseName, "*"). That code only makes sense if ClickHouse returns the literal prefix_* string, which it does.
On your second point: strings.Count(s, "*") == 1 and db*.*
identifierOrPattern only gets called with individual components, never a full db.table string. The call site splits them up:
fmt.Sprintf("%s.%s", identifierOrPattern(*q.database), identifierOrPattern(*q.table))So for db*.*, the function sees "db*" (trailing *, count is 1, returned unquoted) and the .* comes from the format string. The Count == 1 check is just to catch weird inputs like "pre*fix*" where someone has multiple wildcards in a single name.
|
Hi @gregtuc, thanks for taking a second look.
I redid my tests, and I don't think this happens. Can you share the CH version you used to test that behavior? I found an issue with the same behavior I observed - ClickHouse/ClickHouse#92835 With the current code of this PR, grants are created successfully, but cannot be retrieved. Example: resource "clickhousedbops_database" "example" {
name = "my_db_3"
}
resource "clickhousedbops_role" "my_role" {
name = "my_role"
}
resource "clickhousedbops_grant_privilege" "my_role" {
for_each = {for privilege in ["CREATE TABLE","CLUSTER","SELECT","ALTER TABLE","DROP TABLE","INSERT","SHOW TABLES","TRUNCATE"]: privilege => privilege}
privilege_name = each.value
database_name = each.value != "CLUSTER" ? "my_db_*": null
grantee_role_name = "my_role"
}
Suggestion after fix: let's add an acceptance test in |

Summary
ClickHouse supports wildcard/prefix grants (e.g. GRANT SELECT ON db*.* ...). Quoting the pattern (e.g.
db*) turns it into a literal identifier and breaks wildcard matching, causing missing privileges.Fixes #94.
Test plan
GOTOOLCHAIN=auto go test ./...
References
https://clickhouse.com/docs/sql-reference/statements/grant