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

Potential issue with longest matching branch namespace control rules #8622

Closed
pilt opened this issue Dec 2, 2024 · 2 comments · Fixed by #8630
Closed

Potential issue with longest matching branch namespace control rules #8622

pilt opened this issue Dec 2, 2024 · 2 comments · Fixed by #8630
Labels

Comments

@pilt
Copy link

pilt commented Dec 2, 2024

Version: 1.43.19.

Setup:

CREATE DATABASE t;
CREATE USER IF NOT EXISTS 'tadmin'@'%' IDENTIFIED BY 't';
CREATE USER IF NOT EXISTS 'ttask'@'%' IDENTIFIED BY 't';
GRANT ALL ON t.* TO 'tadmin'@'%';
GRANT ALL ON t.* TO 'ttask'@'%';

INSERT INTO t.dolt_branch_namespace_control (`database`, `branch`, `user`, `host`) VALUES ('t', '%', '', '');
INSERT INTO t.dolt_branch_namespace_control (`database`, `branch`, `user`, `host`) VALUES ('t', 'task%', 'ttask', '%');
INSERT INTO t.dolt_branch_namespace_control (`database`, `branch`, `user`, `host`) VALUES ('t', '%', 'tadmin', '%');

I would expect ttask to be allowed to create a branch named "task_feature" but that results in an error:

-- connected as ttask
CALL DOLT_BRANCH('task_feature');
-- [HY000][1105] `ttask`@`%` cannot create a branch named `task_feature`

However tadmin can:

-- connected as tadmin
CALL DOLT_BRANCH('task_feature');
-- status 0
@timsehn timsehn added bug Something isn't working version control labels Dec 2, 2024
@Hydrocharged Hydrocharged linked a pull request Dec 3, 2024 that will close this issue
@Hydrocharged
Copy link
Contributor

The exact details are mentioned in the linked PR. There was a bug with longest matching, where we weren't properly handling the check for the longest match. With the correct logic, ttask is able to successfully create the new branch, while tadmin is restricted. This is the intended behavior, as the longest match is specifically for the longest branch name. This is not clear from the documentation, and I will be updating that shortly.

To explain a little more, the algorithm first checks the database and branch names. In this case, it will match all three rows:

database branch user host
t %
t task% ttask %
t % tadmin %

Once longest match is applied, it will narrow it down to the longest branch.

database branch user host
t task% ttask %

The algorithm will now check for the user and host combination.
tadmin cannot match with the rule ttask, and therefore the user tadmin will fail the control check.

I'm assuming the intention was for tadmin to be able to create any branch, while ttask is limited to only the task% namespace. This will require tadmin to have the existing entry, and also to add another entry for task%, such that the longest match rule will include it. This was decided so that critical branch namespaces can still be restricted inside otherwise permissive regions, but it has the consequence of requiring more entries. We also decided against implicitly allowing superusers to create anything that they want, since they have the ability to freely modify the branch control tables, and can therefore add themselves to the rules. This allows for the tables to serve as the single point of truth, rather than having to cross-reference the branch control tables and privilege tables to determine what a user can do for branches.

I hope this helps!

@pilt
Copy link
Author

pilt commented Dec 3, 2024

I'm assuming the intention was for tadmin to be able to create any branch, while ttask is limited to only the task% namespace.

That was the intention. It wouldn't matter if only ttask is allowed to create task branches though, what's important is that this user cannot make changes to main.

Background: I plan to let untrusted agents (gen AI) perform tasks by generating and executing SQL on short-lived task branches as the ttask user. Then trusted operators (humans) will preview and merge changes with main, or reject changes with instructions to the agent on what needs to be fixed. In short, pull requests.

This will require tadmin to have the existing entry, and also to add another entry for task%, such that the longest match rule will include it.

Alright, thanks, that makes sense. I don't know why, probably because I don't work with security that much, but my brain isn't wired to think in terms of longest matching expressions intuitively it seems.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants