Details

      Description

      We need to think about how to handle security wrt materialized views. Since they are based on a source table we should possibly inherit the same security model as that table.

      However I can see cases where users would want to create different security auth for different views. esp once we have CASSANDRA-9664 and users can filter out sensitive data.

        Issue Links

          Activity

          Hide
          beobal Sam Tunnicliffe added a comment -
          Separating view permissions from the base table

          It makes sense to me for the grants on an MV to be independent of the base table, for the reasons suggested by by Carl Yeksigian on CASSANDRA-6477 :

          it's possible for a less restrictive set of values to be present in the MV, so the set of permissions should be accordingly more granular.

          particularly in light of CASSANDRA-9664.

          Applicable/Valid permissions for views

          Also on #6477, there's some discussion around MODIFY permissions; my $0.02 there is that MODIFY shouldn't even be applicable to MV's. Much like secondary indexes, these are system maintained and direct modification is not/should not be possible (possibly only truncate should be supported - CASSANDRA-8082?). Counterfactually, what would be the implication of a role with MODIFY on the base table, but not the MV?

          Resource hierarchy

          MVs could be bundled together with Tables as DataResources within a keyspace. From this it would follow that GRANT <permission> ON <keyspace> TO <role> would apply to all current & future tables and views in the keyspace, meaning we lose the ability to distinguish between them when operating at the keyspace level. This probably isn't a issue for DML, but maybe more so for DDL. i.e. It's probably ok for a role with SELECT at the keyspace level to be able to read from all tables and views, but would we want to be more granular about CREATE TABLE and CREATE MATERIALIZED VIEW?

          When we're not working at the keyspace level, just adding a new MATERIALIZED_VIEW level in DataResource would enable a specific view to be referenced in a grant statement (with a minor syntax tweak) GRANT <permission> ON VIEW <view> TO <role>. A new level would also make it easy to apply appropriate perms to views when using ALL. e.g. if we agree that MODIFY shouldn't be valid on a view then this would enable it to be omitted when doing GRANT ALL ON VIEW <view> TO <role>.

          The alternative approach is to have a new IResource implementation & hierarchy for views. There would probably be a bit of duplication between this and DataResource, but the main benefit would be to provide a container level resource just for views, enabling statements like GRANT <permission> TO ALL VIEWS IN KEYSPACE <keyspace> TO <role>. The ability to act on the collections of views and tables for a keyspace independently, rather than lumping them together. In this case, we'd be able to separate DDL permissions on tables & views, if that's a goal.

          If we were to go down this route, we should probably make it more explicit that (legacy) statements of the form GRANT <permission> ON KEYSPACE <keyspace> TO <role> apply only to tables, and so change them to GRANT <permission> ON ALL TABLES IN KEYSPACE <keyspace> TO <role> (and continue to support the original form but deprecate it).

          Default permissions for view creator

          One final comment, we should be automatically granting permissions on newly created views to whoever creates them like we do with keyspaces, tables, roles, functions etc. This just needs an override grantPermissionsToCreator in CreateMaterializedViewStatement.

          Show
          beobal Sam Tunnicliffe added a comment - Separating view permissions from the base table It makes sense to me for the grants on an MV to be independent of the base table, for the reasons suggested by by Carl Yeksigian on CASSANDRA-6477 : it's possible for a less restrictive set of values to be present in the MV, so the set of permissions should be accordingly more granular. particularly in light of CASSANDRA-9664 . Applicable/Valid permissions for views Also on #6477, there's some discussion around MODIFY permissions; my $0.02 there is that MODIFY shouldn't even be applicable to MV's. Much like secondary indexes, these are system maintained and direct modification is not/should not be possible (possibly only truncate should be supported - CASSANDRA-8082 ?). Counterfactually, what would be the implication of a role with MODIFY on the base table, but not the MV? Resource hierarchy MVs could be bundled together with Tables as DataResources within a keyspace. From this it would follow that GRANT <permission> ON <keyspace> TO <role> would apply to all current & future tables and views in the keyspace, meaning we lose the ability to distinguish between them when operating at the keyspace level. This probably isn't a issue for DML, but maybe more so for DDL. i.e. It's probably ok for a role with SELECT at the keyspace level to be able to read from all tables and views, but would we want to be more granular about CREATE TABLE and CREATE MATERIALIZED VIEW ? When we're not working at the keyspace level, just adding a new MATERIALIZED_VIEW level in DataResource would enable a specific view to be referenced in a grant statement (with a minor syntax tweak) GRANT <permission> ON VIEW <view> TO <role> . A new level would also make it easy to apply appropriate perms to views when using ALL . e.g. if we agree that MODIFY shouldn't be valid on a view then this would enable it to be omitted when doing GRANT ALL ON VIEW <view> TO <role> . The alternative approach is to have a new IResource implementation & hierarchy for views. There would probably be a bit of duplication between this and DataResource , but the main benefit would be to provide a container level resource just for views, enabling statements like GRANT <permission> TO ALL VIEWS IN KEYSPACE <keyspace> TO <role> . The ability to act on the collections of views and tables for a keyspace independently, rather than lumping them together. In this case, we'd be able to separate DDL permissions on tables & views, if that's a goal. If we were to go down this route, we should probably make it more explicit that (legacy) statements of the form GRANT <permission> ON KEYSPACE <keyspace> TO <role> apply only to tables, and so change them to GRANT <permission> ON ALL TABLES IN KEYSPACE <keyspace> TO <role> (and continue to support the original form but deprecate it). Default permissions for view creator One final comment, we should be automatically granting permissions on newly created views to whoever creates them like we do with keyspaces, tables, roles, functions etc. This just needs an override grantPermissionsToCreator in CreateMaterializedViewStatement .
          Hide
          jbellis Jonathan Ellis added a comment -

          Why can't we just inherit base table permissions for 3.0?

          Show
          jbellis Jonathan Ellis added a comment - Why can't we just inherit base table permissions for 3.0?
          Hide
          iamaleksey Aleksey Yeschenko added a comment -

          Why can't we just inherit base table permissions for 3.0?

          That was the initial suggestion, still in CASSANDRA-6477 comments. Jake raised a valid point that a user might want to grant access to the view that is a subset of the columns in the base to more users (if a view doesn't have anything sensitive, but the base table does).

          What do we want to do in the long term?

          Show
          iamaleksey Aleksey Yeschenko added a comment - Why can't we just inherit base table permissions for 3.0? That was the initial suggestion, still in CASSANDRA-6477 comments. Jake raised a valid point that a user might want to grant access to the view that is a subset of the columns in the base to more users (if a view doesn't have anything sensitive, but the base table does). What do we want to do in the long term?
          Hide
          jbellis Jonathan Ellis added a comment -

          Long term, we do want to support this. But it's pretty late to start design for 3.0.

          Show
          jbellis Jonathan Ellis added a comment - Long term, we do want to support this. But it's pretty late to start design for 3.0.
          Hide
          iamaleksey Aleksey Yeschenko added a comment -

          T Jake Luciani Carl Yeksigian actually, how is the ticket description different from what's already in trunk?

          Show
          iamaleksey Aleksey Yeschenko added a comment - T Jake Luciani Carl Yeksigian actually, how is the ticket description different from what's already in trunk?
          Hide
          tjake T Jake Luciani added a comment - - edited

          I don't think they inherit the base tables permissions currently.

          It would be trivial to add the current role to the MV at the time of MV creation. However it's not clear if you want the role pinned so any changes to the base affect the view. That would go against the let users add different roles for different views.

          Show
          tjake T Jake Luciani added a comment - - edited I don't think they inherit the base tables permissions currently. It would be trivial to add the current role to the MV at the time of MV creation. However it's not clear if you want the role pinned so any changes to the base affect the view. That would go against the let users add different roles for different views.
          Hide
          jbellis Jonathan Ellis added a comment - - edited

          I'm happy with leaving MV permissions to be set explicitly. Inheriting base permissions is definitely not the right thing in all situations.

          NB: Aleksey pointed out that we do to require SELECT on the base table when CREATEing an MV.

          Show
          jbellis Jonathan Ellis added a comment - - edited I'm happy with leaving MV permissions to be set explicitly. Inheriting base permissions is definitely not the right thing in all situations. NB: Aleksey pointed out that we do to require SELECT on the base table when CREATEing an MV.
          Hide
          iamaleksey Aleksey Yeschenko added a comment -

          Requiring SELECT on the base table for CREATE MV is what the initial implementation did - I'm not sure that it's the right thing to do, or sufficient.

          I'm actually leaning towards validating all permissions against the base table - treat them like slightly more visible indexes. For v1.

          Show
          iamaleksey Aleksey Yeschenko added a comment - Requiring SELECT on the base table for CREATE MV is what the initial implementation did - I'm not sure that it's the right thing to do, or sufficient. I'm actually leaning towards validating all permissions against the base table - treat them like slightly more visible indexes. For v1.
          Hide
          jbellis Jonathan Ellis added a comment - - edited

          I'm okay with either "require explicit grants" or "always validate against base table" for 3.0. So let's go with the latter.

          Show
          jbellis Jonathan Ellis added a comment - - edited I'm okay with either "require explicit grants" or "always validate against base table" for 3.0. So let's go with the latter.
          Hide
          iamaleksey Aleksey Yeschenko added a comment -

          So let's go with the latter.

          +1

          Show
          iamaleksey Aleksey Yeschenko added a comment - So let's go with the latter. +1
          Hide
          pauloricardomg Paulo Motta added a comment -

          Finished initial implementation of materialized views authentication. 3.0 v1 patch is available here for review.

          In this initial implementation, the following permissions are necessary for each operation:

          • CREATE MATERIALIZED VIEW: CREATE and SELECT permissions on base table or parent keyspace.
          • ALTER MATERIALIZED VIEW: ALTER permission on base table or parent keyspace.
          • DROP MATERIALIZED VIEW: DROP permission on base table or parent keyspace.

          Tests will be available shortly on the following links:

          Show
          pauloricardomg Paulo Motta added a comment - Finished initial implementation of materialized views authentication. 3.0 v1 patch is available here for review. In this initial implementation, the following permissions are necessary for each operation: CREATE MATERIALIZED VIEW : CREATE and SELECT permissions on base table or parent keyspace. ALTER MATERIALIZED VIEW : ALTER permission on base table or parent keyspace. DROP MATERIALIZED VIEW : DROP permission on base table or parent keyspace. Tests will be available shortly on the following links: 3.0 dtest 3.0 testall trunk dtest trunk testall
          Hide
          iamaleksey Aleksey Yeschenko added a comment -

          CREATE is not a table-level permission, so you should change CREATE MV to require SELECT on the table and CREATE on the keyspace.

          More importantly, you should alter SelectStatement to check for SELECT on the base table.

          Show
          iamaleksey Aleksey Yeschenko added a comment - CREATE is not a table-level permission, so you should change CREATE MV to require SELECT on the table and CREATE on the keyspace. More importantly, you should alter SelectStatement to check for SELECT on the base table.
          Hide
          pauloricardomg Paulo Motta added a comment -

          CREATE is not a table-level permission, so you should change CREATE MV to require SELECT on the table and CREATE on the keyspace.

          Really? I saw in this doc CREATE being listed as a table permission. If it's really forbidden, shouldn't we rethink that for MVs ? It allows for a more fine-grained authorization, since you may want to allow creation of MVs of a given table, but not on the whole keyspace.

          More importantly, you should alter SelectStatement to check for SELECT on the base table.

          How can I miss that? My filtering algorithm for implementing the policies was statement.contains("MATERIALIZED VIEW"). Will fix that in v2.

          Show
          pauloricardomg Paulo Motta added a comment - CREATE is not a table-level permission, so you should change CREATE MV to require SELECT on the table and CREATE on the keyspace. Really? I saw in this doc CREATE being listed as a table permission. If it's really forbidden, shouldn't we rethink that for MVs ? It allows for a more fine-grained authorization, since you may want to allow creation of MVs of a given table, but not on the whole keyspace. More importantly, you should alter SelectStatement to check for SELECT on the base table. How can I miss that? My filtering algorithm for implementing the policies was statement.contains("MATERIALIZED VIEW"). Will fix that in v2.
          Hide
          iamaleksey Aleksey Yeschenko added a comment -

          The doc is for 2.0 and 2.1, it says so in the title. In 2.2 and 3.0 that is no longer true.

          What base permissions we should require for CREATE MV is the only minor detail I'm not certain about. Maybe we should just treat them as indexes for now and require ALTER on the base table for both CREATE MV and DROP MV. Sam Tunnicliffe ?

          Show
          iamaleksey Aleksey Yeschenko added a comment - The doc is for 2.0 and 2.1, it says so in the title. In 2.2 and 3.0 that is no longer true. What base permissions we should require for CREATE MV is the only minor detail I'm not certain about. Maybe we should just treat them as indexes for now and require ALTER on the base table for both CREATE MV and DROP MV . Sam Tunnicliffe ?
          Hide
          beobal Sam Tunnicliffe added a comment -

          Maybe we should just treat them as indexes for now

          sgtm for now

          Show
          beobal Sam Tunnicliffe added a comment - Maybe we should just treat them as indexes for now sgtm for now
          Hide
          pauloricardomg Paulo Motta added a comment -

          Updated branches with suggested modifications:

          Updated policies:

          • CREATE MATERIALIZED VIEW: ALTER and SELECT permissions on base table or parent keyspace.
          • ALTER MATERIALIZED VIEW: ALTER permission on base table or parent keyspace.
          • DROP MATERIALIZED VIEW: DROP permission on base table or parent keyspace.
          • SELECT: SELECT permissions on base table or parent keyspace.

          Tests will be available shortly on the following links:

          Show
          pauloricardomg Paulo Motta added a comment - Updated branches with suggested modifications: 3.0 patch trunk patch Updated policies: CREATE MATERIALIZED VIEW : ALTER and SELECT permissions on base table or parent keyspace. ALTER MATERIALIZED VIEW : ALTER permission on base table or parent keyspace. DROP MATERIALIZED VIEW : DROP permission on base table or parent keyspace. SELECT : SELECT permissions on base table or parent keyspace. Tests will be available shortly on the following links: 3.0 dtest 3.0 testall trunk dtest trunk testall
          Hide
          iamaleksey Aleksey Yeschenko added a comment -

          CreateMaterializedViewStatement should only require ALTER on the baste table, so should DropMaterializedViewStatement. Mimic CREATE INDEX and DROP INDEX.

          Show
          iamaleksey Aleksey Yeschenko added a comment - CreateMaterializedViewStatement should only require ALTER on the baste table, so should DropMaterializedViewStatement . Mimic CREATE INDEX and DROP INDEX .
          Hide
          pauloricardomg Paulo Motta added a comment -

          Updated branches with suggested modifications:

          Updated policies:

          • CREATE MATERIALIZED VIEW: ALTER permissions on base table or parent keyspace.
          • ALTER MATERIALIZED VIEW: ALTER permission on base table or parent keyspace.
          • DROP MATERIALIZED VIEW: ALTER permission on base table or parent keyspace.
          • SELECT: SELECT permissions on base table or parent keyspace.

          Tests will be available shortly on the following links:

          Show
          pauloricardomg Paulo Motta added a comment - Updated branches with suggested modifications: 3.0 patch trunk patch Updated policies: CREATE MATERIALIZED VIEW : ALTER permissions on base table or parent keyspace. ALTER MATERIALIZED VIEW : ALTER permission on base table or parent keyspace. DROP MATERIALIZED VIEW : ALTER permission on base table or parent keyspace. SELECT : SELECT permissions on base table or parent keyspace. Tests will be available shortly on the following links: 3.0 dtest 3.0 testall trunk dtest trunk testall
          Hide
          iamaleksey Aleksey Yeschenko added a comment -

          This is the desired logic, thank you.

          That said, it doesn't build when applied to current cassandra-3.0 (can you fix and rebase?)

          Also the tests don't build. CQLAuthTester is missing, and, in general, MaterializedViewAuthorizationTest doesn't belong where it was put.

          All auth test currently belong to dtests, here. Can you please convert the tests to dtests, where the other auth tests are?

          Show
          iamaleksey Aleksey Yeschenko added a comment - This is the desired logic, thank you. That said, it doesn't build when applied to current cassandra-3.0 (can you fix and rebase?) Also the tests don't build. CQLAuthTester is missing, and, in general, MaterializedViewAuthorizationTest doesn't belong where it was put. All auth test currently belong to dtests, here . Can you please convert the tests to dtests, where the other auth tests are?
          Hide
          iamaleksey Aleksey Yeschenko added a comment -

          My apologies. I didn't notice the other 2 commits in the branch. Really my bad.

          I can commit as is, or, if you don't mind migrating tests to dtest, wait till then.

          Show
          iamaleksey Aleksey Yeschenko added a comment - My apologies. I didn't notice the other 2 commits in the branch. Really my bad. I can commit as is, or, if you don't mind migrating tests to dtest, wait till then.
          Hide
          pauloricardomg Paulo Motta added a comment -

          Updated branches with suggested modifications:

          Materialized views authentication dtests available here: PR451

          When ready, tests will be available here:

          Show
          pauloricardomg Paulo Motta added a comment - Updated branches with suggested modifications: 3.0 patch trunk patch Materialized views authentication dtests available here: PR451 When ready, tests will be available here: 3.0 dtest 3.0 testall trunk dtest trunk testall
          Hide
          iamaleksey Aleksey Yeschenko added a comment -

          Committed as 1a9286c07a5c168df677da9d6be7178d087ea005 to cassandra-3.0 and merged into trunk, thanks.

          Show
          iamaleksey Aleksey Yeschenko added a comment - Committed as 1a9286c07a5c168df677da9d6be7178d087ea005 to cassandra-3.0 and merged into trunk, thanks.

            People

            • Assignee:
              pauloricardomg Paulo Motta
              Reporter:
              tjake T Jake Luciani
              Reviewer:
              Aleksey Yeschenko
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development