Cassandra
  1. Cassandra
  2. CASSANDRA-4874

Possible authorizaton handling impovements

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Fix Version/s: 1.2.0 beta 3
    • Component/s: None
    • Labels:

      Description

      I'll create another issue with my suggestions about fixing/improving IAuthority interfaces. This one lists possible improvements that aren't related to grant/revoke methods.

      Inconsistencies:

      • CREATE COLUMNFAMILY: P.CREATE on the KS in CQL2 vs. P.CREATE on the CF in CQL3 and Thrift
      • BATCH: P.UPDATE or P.DELETE on CF in CQL2 vs. P.UPDATE in CQL3 and Thrift (despite remove* in Thrift asking for P.DELETE)
      • DELETE: P.DELETE in CQL2 and Thrift vs. P.UPDATE in CQL3
      • DROP INDEX: no checks in CQL2 vs. P.ALTER on the CF in CQL3

      Other issues/suggestions

      • CQL2 DROP INDEX should require authorization
      • current permission checks are inconsistent since they are performed separately by CQL2 query processor, Thrift CassandraServer and CQL3 statement classes.
        We should move it to one place. SomeClassWithABetterName.authorize(Operation, KS, CF, User), where operation would be a enum
        (ALTER_KEYSPACE, ALTER_TABLE, CREATE_TABLE, CREATE, USE, UPDATE etc.), CF should be nullable.
      • we don't respect the hierarchy when checking for permissions, or, to be more specific, we are doing it wrong. take CQL3 INSERT as an example:
        we require P.UPDATE on the CF or FULL_ACCESS on either KS or CF. However, having P.UPDATE on the KS won't allow you to perform the statement, only FULL_ACCESS will do.
        I doubt this was intentional, and if it was, I say it's wrong. P.UPDATE on the KS should allow you to do updates on KS's cfs.
        Examples in http://www.datastax.com/dev/blog/dynamic-permission-allocation-in-cassandra-1-1 point to it being a bug, since REVOKE UPDATE ON ks FROM omega is there.
      • currently we lack a way to set permission on cassandra/keyspaces resource. I think we should be able to do it. See the following point on why.
      • currently to create a keyspace you must have a P.CREATE permission on that keyspace THAT DOESN'T EVEN EXIST YET. So only a superuser can create a keyspace,
        or a superuser must first grant you a permission to create it. Which doesn't look right to me. P.CREATE on cassandra/keyspaces should allow you to create new
        keyspaces without an explicit permission for each of them.
      • same goes for CREATE TABLE. you need P.CREATE on that not-yet-existing CF of FULL_ACCESS on the whole KS. P.CREATE on the KS won't do. this is wrong.
      • since permissions don't map directly to statements, we should describe clearly in the documentation what permissions are required by what cql statement/thrift method.

      Full list of current permission requirements: https://gist.github.com/3978182

      1. warn-authority.txt
        2 kB
        Aleksey Yeschenko
      2. v1-v2.diff
        1 kB
        Aleksey Yeschenko
      3. remove-consistency-vestigest-cqlsh-and-cqlg.txt
        5 kB
        Aleksey Yeschenko
      4. move-resource-on-to-iauthorizer.txt
        6 kB
        Aleksey Yeschenko
      5. 4874-v4.txt
        147 kB
        Aleksey Yeschenko
      6. 4874-v3.txt
        119 kB
        Aleksey Yeschenko
      7. 4874-v2.txt
        139 kB
        Aleksey Yeschenko
      8. 4874-4875.txt
        139 kB
        Aleksey Yeschenko

        Issue Links

          Activity

          Hide
          Aleksey Yeschenko added a comment -

          Committed, thanks.

          Show
          Aleksey Yeschenko added a comment - Committed, thanks.
          Hide
          Jonathan Ellis added a comment -

          +1

          Show
          Jonathan Ellis added a comment - +1
          Hide
          Aleksey Yeschenko added a comment -

          Prefer just returning Permission.NONE|ALL instead of copyOf; copying makes me think that the original (or possibly the copy) should be mutable, which is not the case here

          This is only the case with SimpleAuthorizer. The old IAuthority had authorize() return a EnumSet and Permission.ALL|NONE don't return a EnumSet anymore (EnumsSet is mutable). copyOf is there to convert Set<Permission> to EnumSet<Permission>.

          Why the rewrite of DropIndexStatement?

          There used to be no way to get the CF from DropIndexStatement and I needed one to do a permission check for DROP INDEX (alter on parent cf). Now there is a way, but that required a rewrite.

          Do we want to support "show me all the permissions that have been granted to object X?"

          We do, of cource. The latest (last?) patch makes this more explicit.

          Show
          Aleksey Yeschenko added a comment - Prefer just returning Permission.NONE|ALL instead of copyOf; copying makes me think that the original (or possibly the copy) should be mutable, which is not the case here This is only the case with SimpleAuthorizer. The old IAuthority had authorize() return a EnumSet and Permission.ALL|NONE don't return a EnumSet anymore (EnumsSet is mutable). copyOf is there to convert Set<Permission> to EnumSet<Permission>. Why the rewrite of DropIndexStatement? There used to be no way to get the CF from DropIndexStatement and I needed one to do a permission check for DROP INDEX (alter on parent cf). Now there is a way, but that required a rewrite. Do we want to support "show me all the permissions that have been granted to object X?" We do, of cource. The latest (last?) patch makes this more explicit.
          Hide
          Aleksey Yeschenko added a comment -

          attached 'move-resource-on-to-iauthorizer.txt' that moves filtering on resource to IAuthorizer itself away from ListPermissionsStatement#execute.

          Show
          Aleksey Yeschenko added a comment - attached 'move-resource-on-to-iauthorizer.txt' that moves filtering on resource to IAuthorizer itself away from ListPermissionsStatement#execute.
          Hide
          Jonathan Ellis added a comment -

          Do we want to support "show me all the permissions that have been granted to object X?"

          Show
          Jonathan Ellis added a comment - Do we want to support "show me all the permissions that have been granted to object X?"
          Hide
          Jonathan Ellis added a comment -

          Looks good overall. A couple comments:

          • Prefer just returning Permission.NONE|ALL instead of copyOf; copying makes me think that the original (or possibly the copy) should be mutable, which is not the case here
          • Why the rewrite of DropIndexStatement?
          Show
          Jonathan Ellis added a comment - Looks good overall. A couple comments: Prefer just returning Permission.NONE|ALL instead of copyOf; copying makes me think that the original (or possibly the copy) should be mutable, which is not the case here Why the rewrite of DropIndexStatement?
          Hide
          Aleksey Yeschenko added a comment -

          Attached 'warn-authority.txt' patch that
          1. Prints a warning on startup if it detects 'authority' param
          2. Throws ConfigurationException if 'authority' isn't set to 'AllowAllAuthority'

          This way upgrading users who didn't care about authorization in the first place won't have to update anything.
          This is friendlier than just exiting, but I'm not 100% sure if we should be friendly. Not 100% but pretty sure still.

          Show
          Aleksey Yeschenko added a comment - Attached 'warn-authority.txt' patch that 1. Prints a warning on startup if it detects 'authority' param 2. Throws ConfigurationException if 'authority' isn't set to 'AllowAllAuthority' This way upgrading users who didn't care about authorization in the first place won't have to update anything. This is friendlier than just exiting, but I'm not 100% sure if we should be friendly. Not 100% but pretty sure still.
          Hide
          Aleksey Yeschenko added a comment -

          Not to self: one authentication rewrite is complete, make sure to:
          1) Validate user existence in GRANT/REVOKE/LIST PERMISSIONS
          2) Call IAuthority#revokeAll(String droppedUser) when a user is dropped

          Show
          Aleksey Yeschenko added a comment - Not to self: one authentication rewrite is complete, make sure to: 1) Validate user existence in GRANT/REVOKE/LIST PERMISSIONS 2) Call IAuthority#revokeAll(String droppedUser) when a user is dropped
          Hide
          Aleksey Yeschenko added a comment -

          v3 vs. v4 changes:

          • moved cqlsh and Cql.g cleanup to a separate patch (remove-consistency-vestigest-cqlsh-and-cqlg.txt) - apply it first
          • renamed IAuthority to IAuthorizer to be consistent with IAuthenticator
          • renamed {AllowAll,Simple,Legacy}

            Authority to *Authorizer

          • Permission.AUTHORIZE replaced WITH GRANT OPTION
          • CREATE KEYSPACE now requires CREATE on ALL KEYSPACES (used to require CREATE on the not-yet-existing keyspace)
          • CREATE TABLE now requires CREATE on the parent keyspace (used to require CREATE on the not-yet-existing table)
          • new IAuthorizer.revokeAll(IResource droppedResource) method is called for cleanup when a keyspace/table gets dropped
          • IAuthorizer.protectedResources now returns a set of IResource, not DataResource (future-proofing the interface)
          • QueryProcessor.processStatement() now calls validate() first and then checkAccess() (used to be the other way around)
          • GRANT, REVOKE and LIST all check the existence of the resource in question (boolean exists() method has been added to IResource)
          • updated NEWS.txt
          • modified CQL3 syntax (cqlsh autocompletion has been updated as well)

          The new CQL3 statements:

          • LIST { ALL [PERMISSIONS] | <perm> [PERMISSION] } [OF <user>]
            - LIST { ALL [PERMISSIONS] | <perm> [PERMISSION] }

            ON ALL KEYSPACES [OF <user>] [NORECURSIVE]

          • LIST { ALL [PERMISSIONS] | <perm> [PERMISSION] } ON KEYSPACE <keyspace> [of <user>] [NORECURSIVE]
            - LIST { ALL [PERMISSIONS] | <perm> [PERMISSION] }

            ON [TABLE] [<keyspace>.]<table> [of <user>] [NORECURSIVE]

          • GRANT { ALL [PERMISSIONS] | <perm> [PERMISSION] } ON ALL KEYSPACES TO <user>
            - GRANT { ALL [PERMISSIONS] | <perm> [PERMISSION] }

            ON KEYSPACE <keyspace> TO <user>

          • GRANT { ALL [PERMISSIONS] | <perm> [PERMISSION] } ON [TABLE] [<keyspace>.]<table> TO <user>
            - REVOKE { ALL [PERMISSIONS] | <perm> [PERMISSION] }

            ON ALL KEYSPACES FROM <user>

          • REVOKE { ALL [PERMISSIONS] | <perm> [PERMISSION] } ON KEYSPACE <keyspace> FROM <user>
            - REVOKE { ALL [PERMISSIONS] | <perm> [PERMISSION] }

            ON [TABLE] [<keyspace>.]<table> FROM <user>

          Show
          Aleksey Yeschenko added a comment - v3 vs. v4 changes: moved cqlsh and Cql.g cleanup to a separate patch (remove-consistency-vestigest-cqlsh-and-cqlg.txt) - apply it first renamed IAuthority to IAuthorizer to be consistent with IAuthenticator renamed {AllowAll,Simple,Legacy} Authority to *Authorizer Permission.AUTHORIZE replaced WITH GRANT OPTION CREATE KEYSPACE now requires CREATE on ALL KEYSPACES (used to require CREATE on the not-yet-existing keyspace) CREATE TABLE now requires CREATE on the parent keyspace (used to require CREATE on the not-yet-existing table) new IAuthorizer.revokeAll(IResource droppedResource) method is called for cleanup when a keyspace/table gets dropped IAuthorizer.protectedResources now returns a set of IResource, not DataResource (future-proofing the interface) QueryProcessor.processStatement() now calls validate() first and then checkAccess() (used to be the other way around) GRANT, REVOKE and LIST all check the existence of the resource in question (boolean exists() method has been added to IResource) updated NEWS.txt modified CQL3 syntax (cqlsh autocompletion has been updated as well) The new CQL3 statements: LIST { ALL [PERMISSIONS] | <perm> [PERMISSION] } [OF <user>] - LIST { ALL [PERMISSIONS] | <perm> [PERMISSION] } ON ALL KEYSPACES [OF <user>] [NORECURSIVE] LIST { ALL [PERMISSIONS] | <perm> [PERMISSION] } ON KEYSPACE <keyspace> [of <user>] [NORECURSIVE] - LIST { ALL [PERMISSIONS] | <perm> [PERMISSION] } ON [TABLE] [<keyspace>.] <table> [of <user>] [NORECURSIVE] GRANT { ALL [PERMISSIONS] | <perm> [PERMISSION] } ON ALL KEYSPACES TO <user> - GRANT { ALL [PERMISSIONS] | <perm> [PERMISSION] } ON KEYSPACE <keyspace> TO <user> GRANT { ALL [PERMISSIONS] | <perm> [PERMISSION] } ON [TABLE] [<keyspace>.] <table> TO <user> - REVOKE { ALL [PERMISSIONS] | <perm> [PERMISSION] } ON ALL KEYSPACES FROM <user> REVOKE { ALL [PERMISSIONS] | <perm> [PERMISSION] } ON KEYSPACE <keyspace> FROM <user> - REVOKE { ALL [PERMISSIONS] | <perm> [PERMISSION] } ON [TABLE] [<keyspace>.] <table> FROM <user>
          Hide
          Jonathan Ellis added a comment -

          Limiting it to the permissions you already have at least means that you won't be able to re-grant the revoked permission back to yourself on your own.

          Makes sense to me.

          Show
          Jonathan Ellis added a comment - Limiting it to the permissions you already have at least means that you won't be able to re-grant the revoked permission back to yourself on your own. Makes sense to me.
          Hide
          Aleksey Yeschenko added a comment -

          I previously did overthink 2.2 in https://issues.apache.org/jira/browse/CASSANDRA-4874?focusedCommentId=13500289&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13500289
          It's not really an escalation - these two users will still be limited to the permissions they collectively own. So 2.2 should actually work.

          Show
          Aleksey Yeschenko added a comment - I previously did overthink 2.2 in https://issues.apache.org/jira/browse/CASSANDRA-4874?focusedCommentId=13500289&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13500289 It's not really an escalation - these two users will still be limited to the permissions they collectively own. So 2.2 should actually work.
          Hide
          Aleksey Yeschenko added a comment -

          Limiting it to the permissions you already have at least means that you won't be able to re-grant the revoked permission back to yourself on your own.

          Show
          Aleksey Yeschenko added a comment - Limiting it to the permissions you already have at least means that you won't be able to re-grant the revoked permission back to yourself on your own.
          Hide
          Aleksey Yeschenko added a comment -

          This will allow to get rid of grantOption and the permission checks in grant() and authorize() in IAuthority implementations. I'll just add two checks to grant statement and revokestatement: hasAccess(AUTHORIZE, resource) and hasAccees(permissiionToBeGratnedOrRevoked, resource).

          Show
          Aleksey Yeschenko added a comment - This will allow to get rid of grantOption and the permission checks in grant() and authorize() in IAuthority implementations. I'll just add two checks to grant statement and revokestatement: hasAccess(AUTHORIZE, resource) and hasAccees(permissiionToBeGratnedOrRevoked, resource).
          Hide
          Aleksey Yeschenko added a comment -

          Jonathan Ellis
          Now I do think you are more right than I am.
          What do you think about this option: introduce Permission.AUTHORIZE. Let users with AUTHORIZE on a resource GRANT and REVOKE permissions on that resource to/from others, but only the permissions they already have (on that resource or its parent, recursively), including AUTHORIZE itself? I considered this before, but I did overthink it back then.

          Show
          Aleksey Yeschenko added a comment - Jonathan Ellis Now I do think you are more right than I am. What do you think about this option: introduce Permission.AUTHORIZE. Let users with AUTHORIZE on a resource GRANT and REVOKE permissions on that resource to/from others, but only the permissions they already have (on that resource or its parent, recursively), including AUTHORIZE itself? I considered this before, but I did overthink it back then.
          Hide
          Jonathan Ellis added a comment -

          To match your example, yes.

          Show
          Jonathan Ellis added a comment - To match your example, yes.
          Hide
          Aleksey Yeschenko added a comment -

          I probably am. I'll think about it.
          But, to clarify - by LIST you meant ALTER, right?

          Show
          Aleksey Yeschenko added a comment - I probably am. I'll think about it. But, to clarify - by LIST you meant ALTER, right?
          Hide
          Jonathan Ellis added a comment - - edited

          You're overthinking it. You still have that problem with fine-grained grant, it's just more complex. Nor does "include user with GRANT as a user with LIST" make sense, because if you want to include users who could POTENTIALLY have LIST, well, a user with GRANT can give LIST to anyone, not just himself.

          Show
          Jonathan Ellis added a comment - - edited You're overthinking it. You still have that problem with fine-grained grant, it's just more complex. Nor does "include user with GRANT as a user with LIST" make sense, because if you want to include users who could POTENTIALLY have LIST, well, a user with GRANT can give LIST to anyone, not just himself.
          Hide
          Aleksey Yeschenko added a comment -

          Also queries like "LIST ALTER PERMISSIONS ON data/ks" now won't show everybody capable of altering data/ks. Will have to list grant permissions as well (since a user with P.GRANT will be able to just give P.ALTER to himself at any moment).

          Show
          Aleksey Yeschenko added a comment - Also queries like "LIST ALTER PERMISSIONS ON data/ks" now won't show everybody capable of altering data/ks. Will have to list grant permissions as well (since a user with P.GRANT will be able to just give P.ALTER to himself at any moment).
          Hide
          Aleksey Yeschenko added a comment -

          It's a meta-permission really (permission to grant permissions). It just doesn't belong there.

          Show
          Aleksey Yeschenko added a comment - It's a meta-permission really (permission to grant permissions). It just doesn't belong there.
          Hide
          Aleksey Yeschenko added a comment -

          Of course you'd need to revoke GRANT as well. Works as designed!

          I don't like this design P.GRANT (P.AUTHORIZE) would be an equivalent of the old FULL_ACCESS, but on steroids. A super-permission that includes other permissions.
          No, permissions should not intersect.

          Show
          Aleksey Yeschenko added a comment - Of course you'd need to revoke GRANT as well. Works as designed! I don't like this design P.GRANT (P.AUTHORIZE) would be an equivalent of the old FULL_ACCESS, but on steroids. A super-permission that includes other permissions. No, permissions should not intersect.
          Hide
          Jonathan Ellis added a comment -

          REVOKE <perm> won't work as expected if a user still has GRANT permission on the resource).

          Of course you'd need to revoke GRANT as well. Works as designed!

          I vote for leaving IResource alone

          WFM.

          Show
          Jonathan Ellis added a comment - REVOKE <perm> won't work as expected if a user still has GRANT permission on the resource). Of course you'd need to revoke GRANT as well. Works as designed! I vote for leaving IResource alone WFM.
          Hide
          Aleksey Yeschenko added a comment -

          As for IResource and functions - my idea was that each resource type will have its own root (no common global root). DataResources start with 'data', FunctionResources will be represented as 'functions[/..]. So there will usually be at least two levels independent of the resource type. This might even allow user-specified resource types as long as they follow the convention and as long as we have a map root name -> resource class.
          So I vote for leaving IResource alone.

          Show
          Aleksey Yeschenko added a comment - As for IResource and functions - my idea was that each resource type will have its own root (no common global root). DataResources start with 'data', FunctionResources will be represented as 'functions [/..] . So there will usually be at least two levels independent of the resource type. This might even allow user-specified resource types as long as they follow the convention and as long as we have a map root name -> resource class. So I vote for leaving IResource alone.
          Hide
          Aleksey Yeschenko added a comment -

          So WITH GRANT OPTION is here to stay. It's simple to understand and doesn't leak permissions.

          Show
          Aleksey Yeschenko added a comment - So WITH GRANT OPTION is here to stay. It's simple to understand and doesn't leak permissions.
          Hide
          Aleksey Yeschenko added a comment -

          Actually, that wouldn't work (a single GRANT permission).
          I looked at two options, but both don't work:
          1. Allow GRANT owners GRANT and REVOKE any permission on the resource (even those they don't have) - now we get the old FULL_ACCESS and all the associated problems (you have to check for GRANT and not just the requested permission and, secondly, REVOKE <perm> won't work as expected if a user still has GRANT permission on the resource).
          2. Allow GRANT owners GRANT and REVOKE only the permissions they already have on the resource. This one is trickier:
          2.1 Make P.GRANT non-recursive (disallow granting P.GRANT). Now you need to involve a superuser every time you want to allow someone grant permissions. This is bad since it involves superusers unnecessarily.
          2.2 Make P.GRANT recursive (allow granting P.GRANT). Now we've got ourselves easy permission escalation. User A has every permission but P.GRANT. User B has P.SELECT and P.GRANT (or just P.GRANT). User B grants P.GRANT to user A. A grants B every permission he has. Now the two of them together have more permissions than the sum of their old ones.

          Show
          Aleksey Yeschenko added a comment - Actually, that wouldn't work (a single GRANT permission). I looked at two options, but both don't work: 1. Allow GRANT owners GRANT and REVOKE any permission on the resource (even those they don't have) - now we get the old FULL_ACCESS and all the associated problems (you have to check for GRANT and not just the requested permission and, secondly, REVOKE <perm> won't work as expected if a user still has GRANT permission on the resource). 2. Allow GRANT owners GRANT and REVOKE only the permissions they already have on the resource. This one is trickier: 2.1 Make P.GRANT non-recursive (disallow granting P.GRANT). Now you need to involve a superuser every time you want to allow someone grant permissions. This is bad since it involves superusers unnecessarily. 2.2 Make P.GRANT recursive (allow granting P.GRANT). Now we've got ourselves easy permission escalation. User A has every permission but P.GRANT. User B has P.SELECT and P.GRANT (or just P.GRANT). User B grants P.GRANT to user A. A grants B every permission he has. Now the two of them together have more permissions than the sum of their old ones.
          Hide
          Jonathan Ellis added a comment -

          I assume having P.GRANT would give a user ability to grant and revoke any permission on the resource (including P.GRANT itself)

          Right.

          The idea was to avoid breaking IAuthority interface in the future

          That makes sense.

          Show
          Jonathan Ellis added a comment - I assume having P.GRANT would give a user ability to grant and revoke any permission on the resource (including P.GRANT itself) Right. The idea was to avoid breaking IAuthority interface in the future That makes sense.
          Hide
          Aleksey Yeschenko added a comment -

          BTW unfortunately just replacing "implements IAuthority" with "extends LegacyAuthority" won't be enough if a particular implementation used Permission.ALL and/or Permission.NONE. They were returning (mutable) EnumSet-s and I had to replace them with ImmutableSet-s. I wish there was an immutable subclass of EnumSet, but there isn't, because Java. That's the only issue though.

          Show
          Aleksey Yeschenko added a comment - BTW unfortunately just replacing "implements IAuthority" with "extends LegacyAuthority" won't be enough if a particular implementation used Permission.ALL and/or Permission.NONE. They were returning (mutable) EnumSet-s and I had to replace them with ImmutableSet-s. I wish there was an immutable subclass of EnumSet, but there isn't, because Java. That's the only issue though.
          Hide
          Aleksey Yeschenko added a comment -

          Thanks.

          Should document what happened to IAuth/LegacyAuth in News

          Will do.

          grantOption feels like premature complexity to me – why not just a single GRANT permission?

          I assume having P.GRANT would give a user ability to grant and revoke any permission on the resource (including P.GRANT itself). This would also simplify grant and revoke implementations (check for P.GRANT and that's all). If so then I agree and I like it.

          unrelated cqlsh cleanup should be separate.

          Sorry. Couldn't resist.

          Not 100% sure that IResource is going to be useful in its present form (for e.g. functions) so I'd be inclined to just use DataResource everywhere for now, but it's probably okay the way it is for now.

          The idea was to avoid breaking IAuthority interface in the future. Hence the only assumptions were that any new resource type will be hierarchical, and every level will have a printable name. TBH I don't have a strong opinion on this point. We can just see what happens (if?) to functions/triggers/what not and if necessary slightly alter IAuthority in 1.3/1.4. It's YAGNI vs. not breaking stuff. Just using DataResource everywhere for now is fine by me. A second opinion maybe?

          Show
          Aleksey Yeschenko added a comment - Thanks. Should document what happened to IAuth/LegacyAuth in News Will do. grantOption feels like premature complexity to me – why not just a single GRANT permission? I assume having P.GRANT would give a user ability to grant and revoke any permission on the resource (including P.GRANT itself). This would also simplify grant and revoke implementations (check for P.GRANT and that's all). If so then I agree and I like it. unrelated cqlsh cleanup should be separate. Sorry. Couldn't resist. Not 100% sure that IResource is going to be useful in its present form (for e.g. functions) so I'd be inclined to just use DataResource everywhere for now, but it's probably okay the way it is for now. The idea was to avoid breaking IAuthority interface in the future. Hence the only assumptions were that any new resource type will be hierarchical, and every level will have a printable name. TBH I don't have a strong opinion on this point. We can just see what happens (if?) to functions/triggers/what not and if necessary slightly alter IAuthority in 1.3/1.4. It's YAGNI vs. not breaking stuff. Just using DataResource everywhere for now is fine by me. A second opinion maybe?
          Hide
          Jonathan Ellis added a comment -

          Looks pretty good to me. Comments:

          • Should document what happened to IAuth/LegacyAuth in News
          • grantOption feels like premature complexity to me – why not just a single GRANT permission?

          Nits:

          • unrelated cqlsh cleanup should be separate.
          • Not 100% sure that IResource is going to be useful in its present form (for e.g. functions) so I'd be inclined to just use DataResource everywhere for now, but it's probably okay the way it is for now.
          Show
          Jonathan Ellis added a comment - Looks pretty good to me. Comments: Should document what happened to IAuth/LegacyAuth in News grantOption feels like premature complexity to me – why not just a single GRANT permission? Nits: unrelated cqlsh cleanup should be separate. Not 100% sure that IResource is going to be useful in its present form (for e.g. functions) so I'd be inclined to just use DataResource everywhere for now, but it's probably okay the way it is for now.
          Hide
          Aleksey Yeschenko added a comment -

          v3: Pulled out NativeAuthority. Still, please have a look at it while reviewing the issue - it might explain some implementation decisions.

          Show
          Aleksey Yeschenko added a comment - v3: Pulled out NativeAuthority. Still, please have a look at it while reviewing the issue - it might explain some implementation decisions.
          Hide
          Aleksey Yeschenko added a comment - - edited

          v2 is almost identical to v1 (differs by 2 lines, one of which is a comment).
          This way it will work with SimpleAuthenticator.

          Show
          Aleksey Yeschenko added a comment - - edited v2 is almost identical to v1 (differs by 2 lines, one of which is a comment). This way it will work with SimpleAuthenticator.
          Hide
          Aleksey Yeschenko added a comment -

          Single patch for CASSANDRA-4874 and CASSANDRA-4875.

          Show
          Aleksey Yeschenko added a comment - Single patch for CASSANDRA-4874 and CASSANDRA-4875 .
          Hide
          Aleksey Yeschenko added a comment -

          Also, if we are getting rid of P.DELETE in favor of one unified permission for insert/update/delete, then I'd rather add some new name that won't match any of the operations, to avoid confusion. Say, P.MODIFY.

          Show
          Aleksey Yeschenko added a comment - Also, if we are getting rid of P.DELETE in favor of one unified permission for insert/update/delete, then I'd rather add some new name that won't match any of the operations, to avoid confusion. Say, P.MODIFY.
          Hide
          Sylvain Lebresne added a comment -

          Ok, I understand that example. It's a fairly specific use case imo (in term of security I mean), and there will always been cases where whatever permissions we allow won't be precise enough for someone. I'm still of the opinion that it's not worth the potential foot shooting of setting P.DELETE without P.UPDATE by mistake, but that's just an opinion.

          Show
          Sylvain Lebresne added a comment - Ok, I understand that example. It's a fairly specific use case imo (in term of security I mean), and there will always been cases where whatever permissions we allow won't be precise enough for someone. I'm still of the opinion that it's not worth the potential foot shooting of setting P.DELETE without P.UPDATE by mistake, but that's just an opinion.
          Hide
          Aleksey Yeschenko added a comment -

          Btw I'm not advocating for keeping P.DELETE, just saying that there is sometimes a difference from security standpoint.

          Show
          Aleksey Yeschenko added a comment - Btw I'm not advocating for keeping P.DELETE, just saying that there is sometimes a difference from security standpoint.
          Hide
          Aleksey Yeschenko added a comment -

          Imagine a wide row representing a time series, where every column's name is a timestamp and every column's value is already null - you don't care about the value. In this case there is a difference between overwriting the value with crap (doesn't matter) and removing the column entirely (matters).

          Show
          Aleksey Yeschenko added a comment - Imagine a wide row representing a time series, where every column's name is a timestamp and every column's value is already null - you don't care about the value. In this case there is a difference between overwriting the value with crap (doesn't matter) and removing the column entirely (matters).
          Hide
          Sylvain Lebresne added a comment -

          unless you have a null-column in the first place and only care about the column name

          Not sure I follow.

          Show
          Sylvain Lebresne added a comment - unless you have a null-column in the first place and only care about the column name Not sure I follow.
          Hide
          Aleksey Yeschenko added a comment -

          I don't see the difference between updating a value with crap or deleting it

          Most time there is no difference, unless you have a null-column in the first place and only care about the column name.

          Show
          Aleksey Yeschenko added a comment - I don't see the difference between updating a value with crap or deleting it Most time there is no difference, unless you have a null-column in the first place and only care about the column name.
          Hide
          Sylvain Lebresne added a comment -

          I'm not 100% sure it makes sense to distinguish between UPDATE and DELETE at all

          I'd agree with that. From a security perspective I don't see the difference between updating a value with crap or deleting it, so imo both permission will always be set together and so it seems to me that having both only help people at making the mistake of revoking one permission without the other. Just my 2 cents though.

          Show
          Sylvain Lebresne added a comment - I'm not 100% sure it makes sense to distinguish between UPDATE and DELETE at all I'd agree with that. From a security perspective I don't see the difference between updating a value with crap or deleting it, so imo both permission will always be set together and so it seems to me that having both only help people at making the mistake of revoking one permission without the other. Just my 2 cents though.
          Hide
          Jonathan Ellis added a comment -

          Should be P.DELETE in Thrift and CQL3?

          Yes.

          If so, then what do you think about requiring both P.UPDATE and P.DELETE for inserts/updates with TTL set?

          Yes, we should. (This is why I'm not 100% sure it makes sense to distinguish between UPDATE and DELETE at all, but "require both for ttl" is probably the best compromise.)

          Show
          Jonathan Ellis added a comment - Should be P.DELETE in Thrift and CQL3? Yes. If so, then what do you think about requiring both P.UPDATE and P.DELETE for inserts/updates with TTL set? Yes, we should. (This is why I'm not 100% sure it makes sense to distinguish between UPDATE and DELETE at all, but "require both for ttl" is probably the best compromise.)
          Hide
          Aleksey Yeschenko added a comment -

          DELETE: P.DELETE in CQL2 and Thrift vs. P.UPDATE in CQL3

          Seems obvious.

          Not to me. Should be P.DELETE in Thrift and CQL3? If so, then what do you think about requiring both P.UPDATE and P.DELETE for inserts/updates with TTL set?

          We should move it to one place. SomeClassWithABetterName.authorize

          I'm not sure this really improves things, you've just created an abstraction layer with different names but fundamentally you still have to insert the correct auth call in each query processing path.

          You are right. This won't be needed once we fix permission inheritance. Then one method in ClientState (modified current hasAccees) will be sufficient. No need for another enum.

          Jonathan Ellis Can you look at CASSANDRA-4875 as well please?

          Show
          Aleksey Yeschenko added a comment - DELETE: P.DELETE in CQL2 and Thrift vs. P.UPDATE in CQL3 Seems obvious. Not to me. Should be P.DELETE in Thrift and CQL3? If so, then what do you think about requiring both P.UPDATE and P.DELETE for inserts/updates with TTL set? We should move it to one place. SomeClassWithABetterName.authorize I'm not sure this really improves things, you've just created an abstraction layer with different names but fundamentally you still have to insert the correct auth call in each query processing path. You are right. This won't be needed once we fix permission inheritance. Then one method in ClientState (modified current hasAccees) will be sufficient. No need for another enum. Jonathan Ellis Can you look at CASSANDRA-4875 as well please?
          Hide
          Jonathan Ellis added a comment -

          CREATE COLUMNFAMILY: P.CREATE on the KS in CQL2 vs. P.CREATE on the CF in CQL3 and Thrift

          CQL2 sounds correct to me, how can you have permissions on an object that doesn't exist yet?

          But that would imply that KS create permissioning is also broken, which you mention.

          Maybe we should have a "cluster" or "all" top-level permission: having create on all, allows creating keyspaces. This would fit with the heirarchy design you describe too (GRANT UPDATE ON ALL TO foo), and gives a nice shorthand for granting system-wide permissions (or a subset of them) w/o making someone a superuser.

          BATCH: P.UPDATE or P.DELETE on CF in CQL2 vs. P.UPDATE in CQL3 and Thrift (despite remove* in Thrift asking for P.DELETE)

          ISTM that the correct behavior is to permission-check each statement in the batch separately.

          DELETE: P.DELETE in CQL2 and Thrift vs. P.UPDATE in CQL3

          Seems obvious.

          DROP INDEX: no checks in CQL2 vs. P.ALTER on the CF in CQL3

          ALTER sounds reasonable.

          We should move it to one place. SomeClassWithABetterName.authorize

          I'm not sure this really improves things, you've just created an abstraction layer with different names but fundamentally you still have to insert the correct auth call in each query processing path.

          P.UPDATE on the KS should allow you to do updates on KS's cfs

          +1

          Show
          Jonathan Ellis added a comment - CREATE COLUMNFAMILY: P.CREATE on the KS in CQL2 vs. P.CREATE on the CF in CQL3 and Thrift CQL2 sounds correct to me, how can you have permissions on an object that doesn't exist yet? But that would imply that KS create permissioning is also broken, which you mention. Maybe we should have a "cluster" or "all" top-level permission: having create on all, allows creating keyspaces. This would fit with the heirarchy design you describe too (GRANT UPDATE ON ALL TO foo), and gives a nice shorthand for granting system-wide permissions (or a subset of them) w/o making someone a superuser. BATCH: P.UPDATE or P.DELETE on CF in CQL2 vs. P.UPDATE in CQL3 and Thrift (despite remove* in Thrift asking for P.DELETE) ISTM that the correct behavior is to permission-check each statement in the batch separately. DELETE: P.DELETE in CQL2 and Thrift vs. P.UPDATE in CQL3 Seems obvious. DROP INDEX: no checks in CQL2 vs. P.ALTER on the CF in CQL3 ALTER sounds reasonable. We should move it to one place. SomeClassWithABetterName.authorize I'm not sure this really improves things, you've just created an abstraction layer with different names but fundamentally you still have to insert the correct auth call in each query processing path. P.UPDATE on the KS should allow you to do updates on KS's cfs +1

            People

            • Assignee:
              Aleksey Yeschenko
              Reporter:
              Aleksey Yeschenko
              Reviewer:
              Jonathan Ellis
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development