Allow system tables to do system level access control#24726
Allow system tables to do system level access control#24726kokosing merged 1 commit intotrinodb:masterfrom
Conversation
lukasz-stec
requested review from
ebyhr,
electrum,
martint,
mosabua and
raunaqmorarka
lukasz-stec
force-pushed
the
ls/2501/03-system-table-access-control
branch
from
427247a to
0f82a5c
Compare
lukasz-stec
force-pushed
the
ls/2501/03-system-table-access-control
branch
from
0f82a5c to
19fa449
Compare
|
I noticed that we already cast |
lukasz-stec
force-pushed
the
ls/2501/03-system-table-access-control
branch
from
19fa449 to
3dfed42
Compare
| catalogName); | ||
| try { | ||
| return new MappedPageSource(systemTable.pageSource(systemTransaction.getConnectorTransactionHandle(), session, newConstraint), userToSystemFieldIndex.build()); | ||
| // Do not pass access control for tables that execute on workers |
There was a problem hiding this comment.
Why so? I think we should put pass it too. Workers have access control, which could be different than on coordinator but still.
Or maybe we could pass DenyAll* with the error message that it is not yet supported on worker.
Also I think we should not care here about the access control. It is not the proper layer. The thing is in the guice context, and it is used in other places so I think we are fine to use it too here.
There was a problem hiding this comment.
Why so? I think we should put pass it too. Workers have access control, which could be different than on coordinator but still.
- I wanted to be on the safe side in terms of security. Even though access control is available on workers, people normally don't configure it because it is almost not used.
The only place I know it is used isManagementAuthorizationFilter. Allowing access control on workers would be a subtle security gap, thus, because the system table creator could expect the same access control everywhere, and that may not be true in practice, and I don't think we can defend against that. - Currently, table functions allow access control only on the coordinator during analysis, so allowing only coordinator for system tables makes sense from the consistency perspective.
- System tables that need to use access control are usually working on metadata available only on coordinator, so allowing this on workers will bring minimal to no benefit.
Or maybe we could pass DenyAll* with the error message that it is not yet supported on worker.
Effectively, this works like that because UnsupportedOperationException will be thrown in when the incorrect api is used. I think that is better because it makes the SystemTable implementation less likely to make a mistake.
Also I think we should not care here about the access control. It is not the proper layer. The thing is in the guice context, and it is used in other places so I think we are fine to use it too here.
I'm sorry, I don't understand this comment. Where should we not care about access control? in the SystemPageSourceProvider?
There was a problem hiding this comment.
I'm sorry, I don't understand this comment. Where should we not care about access control? in the SystemPageSourceProvider?
I believe that is a guice context responsiblity to provide proper implementation of access control. I access control is incorrect on worker, it should not be bounded in guice. If worker is using only part of the of the access control, then it should maybe use something else. My point is that here we are a dealing with a bigger design problem. Basically, I believe no access control should be available on the worker. However it is a preexisting issue that goes well beyond this PR.
lukasz-stec
left a comment
lukasz-stec
left a comment
There was a problem hiding this comment.
ansered comments. Since there are no tests for system connector (am I missing something?) I want to wait with tests until the aproach is confirmned as this will require some more effort to add.
| catalogName); | ||
| try { | ||
| return new MappedPageSource(systemTable.pageSource(systemTransaction.getConnectorTransactionHandle(), session, newConstraint), userToSystemFieldIndex.build()); | ||
| // Do not pass access control for tables that execute on workers |
There was a problem hiding this comment.
Why so? I think we should put pass it too. Workers have access control, which could be different than on coordinator but still.
- I wanted to be on the safe side in terms of security. Even though access control is available on workers, people normally don't configure it because it is almost not used.
The only place I know it is used isManagementAuthorizationFilter. Allowing access control on workers would be a subtle security gap, thus, because the system table creator could expect the same access control everywhere, and that may not be true in practice, and I don't think we can defend against that. - Currently, table functions allow access control only on the coordinator during analysis, so allowing only coordinator for system tables makes sense from the consistency perspective.
- System tables that need to use access control are usually working on metadata available only on coordinator, so allowing this on workers will bring minimal to no benefit.
Or maybe we could pass DenyAll* with the error message that it is not yet supported on worker.
Effectively, this works like that because UnsupportedOperationException will be thrown in when the incorrect api is used. I think that is better because it makes the SystemTable implementation less likely to make a mistake.
Also I think we should not care here about the access control. It is not the proper layer. The thing is in the guice context, and it is used in other places so I think we are fine to use it too here.
I'm sorry, I don't understand this comment. Where should we not care about access control? in the SystemPageSourceProvider?
|
Merged, thank you! |
|
I think this doesnt need release notes at this stage.. |
|
Yes. It is just a framework at this point. |
3 participants
Description
This is an initial proposal to add an option to do access control checks during system table reads.
The goal here is to open this for discussion with some concrete option on a table.
Additional context and related issues
This is related to an effort to add an iceberg system table that lists only iceberg tables (#24469).
A system table like that has to filter the table list using the
AccessControl#filterTablesmethod but up until now,AccessControlwas not available for system tables.Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: