Cassandra
  1. Cassandra
  2. CASSANDRA-4295

Implement caching of authorization results

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Fix Version/s: 1.2.2
    • Component/s: API
    • Labels:
      None

      Description

      1.2 will come with default IAuthority implementation that stores permissions in Cassandra, and each permission check will involve at least 1 Cassandra read. Some form of authorization result caching is very important for this scenario.

      1. CASSANDRA-4295.patch
        19 kB
        Pavel Yaskevich
      2. 4295.txt
        6 kB
        Aleksey Yeschenko
      3. 4295-v2.txt
        9 kB
        Aleksey Yeschenko

        Activity

        Hide
        Sylvain Lebresne added a comment -

        Couldn't we imagine that the access themselves may change? I could imagine wanting to upgrade/downgrade access authorizations over time.

        Show
        Sylvain Lebresne added a comment - Couldn't we imagine that the access themselves may change? I could imagine wanting to upgrade/downgrade access authorizations over time.
        Hide
        Jonathan Ellis added a comment -

        I suppose, but they'd be relatively static, so adding a "validated" boolean that we can negate when permissions change should be adequate.

        Most (all?) production implementations of the auth API are going to call into a remote service to validate credentials, so that feels like something worth optimizing away if we can.

        Show
        Jonathan Ellis added a comment - I suppose, but they'd be relatively static, so adding a "validated" boolean that we can negate when permissions change should be adequate. Most (all?) production implementations of the auth API are going to call into a remote service to validate credentials, so that feels like something worth optimizing away if we can.
        Hide
        Sylvain Lebresne added a comment -

        Most (all?) production implementations of the auth API are going to call into a remote service to validate credentials, so that feels like something worth optimizing away if we can.

        Make sense. Though the "validated" business does suppose that we're able to say when permission changes (I'm not sure all "production implementation of the auth API" will provide that). Anyway, I'll give that a shot and we can readapt a bit if that's not good enough for some auth implementation.

        Show
        Sylvain Lebresne added a comment - Most (all?) production implementations of the auth API are going to call into a remote service to validate credentials, so that feels like something worth optimizing away if we can. Make sense. Though the "validated" business does suppose that we're able to say when permission changes (I'm not sure all "production implementation of the auth API" will provide that). Anyway, I'll give that a shot and we can readapt a bit if that's not good enough for some auth implementation.
        Hide
        Sylvain Lebresne added a comment -

        So I will admit I'm a little uncomfortable committing something without having a solution for the case where access right change over time (which the patch don't tackle unless I'm misreading it). Access rights are a security feature, so it feel to me we don't want to start to get too sloppy.

        I'm almost inclined to say that the caching of access rights (which is basically what we're talking about) is something the auth API should be in charge of doing (since it can do it better).

        Or we make it part of the spec that access for prepared statement are checked during preparation only, which is an option, but that sounds a bit weak to me. At least we shouldn't make that decision lightly.

        Show
        Sylvain Lebresne added a comment - So I will admit I'm a little uncomfortable committing something without having a solution for the case where access right change over time (which the patch don't tackle unless I'm misreading it). Access rights are a security feature, so it feel to me we don't want to start to get too sloppy. I'm almost inclined to say that the caching of access rights (which is basically what we're talking about) is something the auth API should be in charge of doing (since it can do it better). Or we make it part of the spec that access for prepared statement are checked during preparation only, which is an option, but that sounds a bit weak to me. At least we shouldn't make that decision lightly.
        Hide
        Sylvain Lebresne added a comment -

        The current IAuthority API is:

        public EnumSet<Permission> authorize(AuthenticatedUser user, List<Object> resource);
        

        What about changing that to something like:

        public interface PermissionToken {}
        
        public Pair<EnumSet<Permission>, PermissionToken> authorize(AuthenticatedUser user, List<Object> resource);
        public boolean isStillValid(PermissionToken token);
        

        The semantic being that authorize would give us a token (every implementation could make that be whatever they want) and isStillValid would validate whether the authorization that returned the token is still valid at the time of the call to isStillValid.

        Implementations that don't want to get fancy could just return null as the token and have isStillValid return either:

        • always true if an authorization is valid indefinitely
        • always false to force redoing an authorization every time

        And more fancier policy (like authorization is valid for X minutes only, ...) can be easily implemented too.

        Then in CQL we would call authorize during preparation and keep the token around, and during execution we would check the validity of the token and redo the authorization only if it's not valid anymore. It does complicate think a bit, but not too much either.

        Or we just leave things like they are and consider that caching the result of authorize should be the business of the IAuthority (I'm personally not against that). But the more I think about it, the more I'm convince that forcing users and their access rights to be immutable is A Bad Idea.

        Show
        Sylvain Lebresne added a comment - The current IAuthority API is: public EnumSet<Permission> authorize(AuthenticatedUser user, List<Object> resource); What about changing that to something like: public interface PermissionToken {} public Pair<EnumSet<Permission>, PermissionToken> authorize(AuthenticatedUser user, List<Object> resource); public boolean isStillValid(PermissionToken token); The semantic being that authorize would give us a token (every implementation could make that be whatever they want) and isStillValid would validate whether the authorization that returned the token is still valid at the time of the call to isStillValid . Implementations that don't want to get fancy could just return null as the token and have isStillValid return either: always true if an authorization is valid indefinitely always false to force redoing an authorization every time And more fancier policy (like authorization is valid for X minutes only, ...) can be easily implemented too. Then in CQL we would call authorize during preparation and keep the token around, and during execution we would check the validity of the token and redo the authorization only if it's not valid anymore. It does complicate think a bit, but not too much either. Or we just leave things like they are and consider that caching the result of authorize should be the business of the IAuthority (I'm personally not against that). But the more I think about it, the more I'm convince that forcing users and their access rights to be immutable is A Bad Idea.
        Hide
        Jonathan Ellis added a comment -

        Not a fan of splitting the API up. Feels like we're shoving implementation up to the client level.

        Not a fan of saying "each provider should reimplement caching internally," either, since by doing it once at the server level we can do a better job of QA and DRY.

        Show
        Jonathan Ellis added a comment - Not a fan of splitting the API up. Feels like we're shoving implementation up to the client level. Not a fan of saying "each provider should reimplement caching internally," either, since by doing it once at the server level we can do a better job of QA and DRY.
        Hide
        Sylvain Lebresne added a comment -

        Maybe doing the caching on our side with a configurable cache expiration time setting would be good enough.

        Show
        Sylvain Lebresne added a comment - Maybe doing the caching on our side with a configurable cache expiration time setting would be good enough.
        Hide
        Jonathan Ellis added a comment -

        That's probably reasonable to start with.

        Could we add an invalidation hook to the API so that when permissions are changed, it can notify us and we can go through the cache and throw out obsolete entries?

        Show
        Jonathan Ellis added a comment - That's probably reasonable to start with. Could we add an invalidation hook to the API so that when permissions are changed, it can notify us and we can go through the cache and throw out obsolete entries?
        Hide
        Aleksey Yeschenko added a comment -

        Could we add an invalidation hook to the API so that when permissions are changed, it can notify us and we can go through the cache and throw out obsolete entries?

        What if we enable caching for IAuthority2 only? And then invalidate the cache after calling IAuthority2.grant() and revoke() in GrantStatement/RevokeStatement?

        Show
        Aleksey Yeschenko added a comment - Could we add an invalidation hook to the API so that when permissions are changed, it can notify us and we can go through the cache and throw out obsolete entries? What if we enable caching for IAuthority2 only? And then invalidate the cache after calling IAuthority2.grant() and revoke() in GrantStatement/RevokeStatement?
        Hide
        Jonathan Ellis added a comment -

        Sounds reasonable.

        Show
        Jonathan Ellis added a comment - Sounds reasonable.
        Hide
        Aleksey Yeschenko added a comment -

        Reasonable, but probably not easily implementable, now that I think of it. Will only work for a single-node 'cluster' since GRANT/REVOKE are executed on the coordinator node only. The coordinator will have to somehow signal the event to other nodes and that's gossip abuse.

        An invalidation hook won't work for the same reason.

        Show
        Aleksey Yeschenko added a comment - Reasonable, but probably not easily implementable, now that I think of it. Will only work for a single-node 'cluster' since GRANT/REVOKE are executed on the coordinator node only. The coordinator will have to somehow signal the event to other nodes and that's gossip abuse. An invalidation hook won't work for the same reason.
        Hide
        Aleksey Yeschenko added a comment -

        But time-based caching would be helpful, as long as the period is configurable or reasonably low (I'm thinking a couple seconds/tenths of a second maybe).

        Show
        Aleksey Yeschenko added a comment - But time-based caching would be helpful, as long as the period is configurable or reasonably low (I'm thinking a couple seconds/tenths of a second maybe).
        Hide
        Aleksey Yeschenko added a comment -

        So the proposed solution is to use Guava's CacheBuilder<IResource,Set<Permission>> for a map in ClientState. TTL will be configurable through cassandra.yaml and will be set to 1 second by default.

        Show
        Aleksey Yeschenko added a comment - So the proposed solution is to use Guava's CacheBuilder<IResource,Set<Permission>> for a map in ClientState. TTL will be configurable through cassandra.yaml and will be set to 1 second by default.
        Hide
        Jonathan Ellis added a comment -

        Bumping to 1.2.1, it's not worth holding the release for.

        Show
        Jonathan Ellis added a comment - Bumping to 1.2.1, it's not worth holding the release for.
        Hide
        Jonathan Ellis added a comment -

        Oops, should have seen the patch before doing that. Marking Patch Available.

        Show
        Jonathan Ellis added a comment - Oops, should have seen the patch before doing that. Marking Patch Available.
        Hide
        Aleksey Yeschenko added a comment -

        The patch implements the original suggestion (move checkaccess into statement.prepare) which we decided is not a good idea.
        This is why I changed the status to Open on Saturday.

        Show
        Aleksey Yeschenko added a comment - The patch implements the original suggestion (move checkaccess into statement.prepare) which we decided is not a good idea. This is why I changed the status to Open on Saturday.
        Hide
        Aleksey Yeschenko added a comment -

        And I want to have it done together with CASSANDRA-4874 and CASSANDRA-4875, while I'm at it and the context is fresh in my head (will be ready in a few days).
        Can we set fixver to 1.2.0, please?

        Show
        Aleksey Yeschenko added a comment - And I want to have it done together with CASSANDRA-4874 and CASSANDRA-4875 , while I'm at it and the context is fresh in my head (will be ready in a few days). Can we set fixver to 1.2.0, please?
        Hide
        Jonathan Ellis added a comment -

        Eh, let's leave it alone and we can edit again if we need to. "A few days" will be rc1 still.

        Show
        Jonathan Ellis added a comment - Eh, let's leave it alone and we can edit again if we need to. "A few days" will be rc1 still.
        Hide
        Aleksey Yeschenko added a comment -

        Moving this one to 1.2.1 - it's not as critical now that NativeAuthorizer isn't going to be part of 1.2.0.

        Show
        Aleksey Yeschenko added a comment - Moving this one to 1.2.1 - it's not as critical now that NativeAuthorizer isn't going to be part of 1.2.0.
        Hide
        Aleksey Yeschenko added a comment -

        v2 removes authz result caching in CQL3 BatchStatement and thrift's batch_mutate + atomic_batch_mutate.

        Show
        Aleksey Yeschenko added a comment - v2 removes authz result caching in CQL3 BatchStatement and thrift's batch_mutate + atomic_batch_mutate.
        Hide
        Jonathan Ellis added a comment -

        v2 LGTM.

        Show
        Jonathan Ellis added a comment - v2 LGTM.
        Hide
        Aleksey Yeschenko added a comment -

        Committed, thanks.

        Show
        Aleksey Yeschenko added a comment - Committed, thanks.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development