Cassandra
  1. Cassandra
  2. CASSANDRA-1237

Store AccessLevels externally to IAuthenticator

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Fix Version/s: 0.7 beta 2
    • Component/s: Core
    • Labels:
      None

      Description

      Currently, the concept of authentication (proving the identity of a user) is mixed up with permissions (determining whether a user is able to create/read/write databases). Rather than determining the permissions that a user has, the IAuthenticator should only be capable of authenticating a user, and permissions (specifically, an AccessLevel) should be stored consistently by Cassandra.

      EDIT: Adding summary


      In summary, there appear to be 3 distinct options for how to move forward with authorization. Remember that this ticket is about disconnecting authorization (permissions) from authentication (user/group identification), and its goal is to leave authentication pluggable.

      Options:

      1. Leave authentication and authorization in the same interface. If we choose this option, this ticket is invalid, and CASSANDRA-1271 and CASSANDRA-1320 will add-to/improve IAuthenticator
        • Pros:
          • Least change
        • Cons:
          • Very little actually implemented by Cassandra: burden is on the backend implementors
          • Each combination of authz and authc backends would require a new implementation (PAM for authc + permissions keyspace for authz, for instance), causing an explosion of implementations
      2. Separate out a pluggable IAuthority interface to implement authorization
        1. IAuthenticator interface would be called at login time to determine user/groups membership
        2. IAuthority would be called at operation time with the user/groups determined earlier, and the required permission for the operation
        • Pros:
          • Provides the cleanest separation of concerns
          • Allows plugability for permissions
        • Cons:
          • Pluggability of permissions gains limited benefit
          • IAuthority would need to support callbacks for keyspace/cf creation and removal to keep existing keyspaces in sync with their permissions (although technically, option 1 suffers from this as well)
      3. Separate authorization, but do not make it pluggable
        • This option is implemented by the existing patchset by attaching permissions to metadata, but could have an alternative implementation that stores permissions in a permissions keyspace.
        • Pros:
          • Cassandra controls the scalability of authorization, and can ensure it does not become a bottleneck
        • Cons:
          • Would need to support callbacks for user creation and removal to keep existing users in sync with their permissions

        Issue Links

          Activity

          Hide
          Eric Evans added a comment -

          committed; thanks!

          Show
          Eric Evans added a comment - committed; thanks!
          Hide
          Stu Hood added a comment -

          Optimistically targetting back to 0.7-beta2 (sorry for the flapping).

          Show
          Stu Hood added a comment - Optimistically targetting back to 0.7-beta2 (sorry for the flapping).
          Hide
          Stu Hood added a comment -

          Implementation of option 2:

          v2-0001 - Removes AccessLevel return value from login in the client APIs
          v2-0002 - Splits IAuthority out of IAuthenticator: SimpleAuthority and SimpleAuthenticator are backwards compatible
          v2-0003 - Removes some reflection-centered duplication
          v2-0004 - Allows configuration of an IAuthority, and specifies SimpleAuthority for upgraders who have configured SimpleAuthenticator
          v2-0005 - Splits the authc/z ThreadLocals out of the CassandraServers and into a ClientState object

          Show
          Stu Hood added a comment - Implementation of option 2: v2-0001 - Removes AccessLevel return value from login in the client APIs v2-0002 - Splits IAuthority out of IAuthenticator: SimpleAuthority and SimpleAuthenticator are backwards compatible v2-0003 - Removes some reflection-centered duplication v2-0004 - Allows configuration of an IAuthority, and specifies SimpleAuthority for upgraders who have configured SimpleAuthenticator v2-0005 - Splits the authc/z ThreadLocals out of the CassandraServers and into a ClientState object
          Hide
          Eric Evans added a comment -

          Does this sound reasonable?

          It sounds reasonable to me.

          Show
          Eric Evans added a comment - Does this sound reasonable? It sounds reasonable to me.
          Hide
          Stu Hood added a comment -

          I think it should be possible to implement option #2 before 0.7 final, if we defer implementing the 'callbacks' until we have a backend that can support modifying its permissions (SimpleAuthenticator cannot).

          Split AccessLevel IAuthenticator.login(String keyspace, Map credentials) into:

          • AuthenticatedUser IAuthenticator.authenticate(Map credentials)
            • Where AuthenticatedUser is similar to the implementation in the existing patchset, but without a 'isSuper' flag, since the IAuthority should make that decision
          • AccessLevel IAuthority.authorize(AuthenticatedUser user, String keyspace)
            • In CASSANDRA-1320, AccessLevel will be replaced with a Set<Permission>
            • In CASSANDRA-1271, keyspace will be replaced with a generic 'resource' hierarchy

          Does this sound reasonable?

          Show
          Stu Hood added a comment - I think it should be possible to implement option #2 before 0.7 final, if we defer implementing the 'callbacks' until we have a backend that can support modifying its permissions (SimpleAuthenticator cannot). Split AccessLevel IAuthenticator.login(String keyspace, Map credentials) into: AuthenticatedUser IAuthenticator.authenticate(Map credentials) Where AuthenticatedUser is similar to the implementation in the existing patchset, but without a 'isSuper' flag, since the IAuthority should make that decision AccessLevel IAuthority.authorize(AuthenticatedUser user, String keyspace) In CASSANDRA-1320 , AccessLevel will be replaced with a Set<Permission> In CASSANDRA-1271 , keyspace will be replaced with a generic 'resource' hierarchy Does this sound reasonable?
          Hide
          Nirmal Ranganathan added a comment -

          Here's some probable use cases for Authz:

          • User A / Group B -> read/write KS1, KS2: read KS3
          • User C / Group D -> create/rename/drop KS/CF
          • User E -> read/write KS1-CF1, KS1-CF2: read KS1-CF3
          • Admin -> add/modify/delete Users/groups/permissions
          Show
          Nirmal Ranganathan added a comment - Here's some probable use cases for Authz: User A / Group B -> read/write KS1, KS2: read KS3 User C / Group D -> create/rename/drop KS/CF User E -> read/write KS1-CF1, KS1-CF2: read KS1-CF3 Admin -> add/modify/delete Users/groups/permissions
          Hide
          Eric Evans added a comment -

          I am in favor of 3. I like that it keeps scalability over checking permissions, and I like it is made pluggable for people to integrate with LDAP or their own framework.

          3 is decidedly not pluggable with respect to authorization (only authentication). Also, there is also nothing to prevent you from implementing a back-end that stored credentials and/or permissions in Cassandra.

          Show
          Eric Evans added a comment - I am in favor of 3. I like that it keeps scalability over checking permissions, and I like it is made pluggable for people to integrate with LDAP or their own framework. 3 is decidedly not pluggable with respect to authorization (only authentication). Also, there is also nothing to prevent you from implementing a back-end that stored credentials and/or permissions in Cassandra.
          Hide
          Antoine Toulme added a comment -

          I am in favor of 3. I like that it keeps scalability over checking permissions, and I like it is made pluggable for people to integrate with LDAP or their own framework.

          Show
          Antoine Toulme added a comment - I am in favor of 3. I like that it keeps scalability over checking permissions, and I like it is made pluggable for people to integrate with LDAP or their own framework.
          Hide
          Stu Hood added a comment -

          > The pressing problem is that the current implementation predates dynamically created keyspaces/column families
          Correct.. CASSANDRA-1271 addresses that issue. I think it is too late to make any of these changes in 0.7, so I'm interested in what you think we can/should do for 0.8.

          Show
          Stu Hood added a comment - > The pressing problem is that the current implementation predates dynamically created keyspaces/column families Correct.. CASSANDRA-1271 addresses that issue. I think it is too late to make any of these changes in 0.7, so I'm interested in what you think we can/should do for 0.8.
          Hide
          Eric Evans added a comment -

          The pressing problem is that the current implementation predates dynamically created keyspaces/column families and as a result it's not possible to add/remove keyspaces with auth enabled. It's also quite late in the 0.7 cycle so "least change" from #1 seems quite appealing.

          Show
          Eric Evans added a comment - The pressing problem is that the current implementation predates dynamically created keyspaces/column families and as a result it's not possible to add/remove keyspaces with auth enabled. It's also quite late in the 0.7 cycle so "least change" from #1 seems quite appealing.
          Hide
          Stu Hood added a comment -

          In summary, there appear to be 3 distinct options for how to move forward with authorization. Remember that this ticket is about disconnecting authorization (permissions) from authentication (user/group identification), and its goal is to leave authentication pluggable.

          Options:

          1. Leave authentication and authorization in the same interface. If we choose this option, this ticket is invalid, and CASSANDRA-1271 and CASSANDRA-1320 will add-to/improve IAuthenticator
            • Pros:
              • Least change
            • Cons:
              • Very little actually implemented by Cassandra: burden is on the backend implementors
              • Each combination of authz and authc backends would require a new implementation (PAM for authc + permissions keyspace for authz, for instance), causing an explosion of implementations
          2. Separate out a pluggable IAuthority interface to implement authorization
            1. IAuthenticator interface would be called at login time to determine user/groups membership
            2. IAuthority would be called at operation time with the user/groups determined earlier, and the required permission for the operation
            • Pros:
              • Provides the cleanest separation of concerns
              • Allows plugability for permissions
            • Cons:
              • Pluggability of permissions gains limited benefit
              • IAuthority would need to support callbacks for keyspace/cf creation and removal to keep existing keyspaces in sync with their permissions (although technically, option 1 suffers from this as well)
          3. Separate authorization, but do not make it pluggable
            • This option is implemented by the existing patchset by attaching permissions to metadata, but could have an alternative implementation that stores permissions in a permissions keyspace.
            • Pros:
              • Cassandra controls the scalability of authorization, and can ensure it does not become a bottleneck
            • Cons:
              • Would need to support callbacks for user creation and removal to keep existing users in sync with their permissions
          Show
          Stu Hood added a comment - In summary, there appear to be 3 distinct options for how to move forward with authorization. Remember that this ticket is about disconnecting authorization (permissions) from authentication (user/group identification), and its goal is to leave authentication pluggable. Options: Leave authentication and authorization in the same interface. If we choose this option, this ticket is invalid, and CASSANDRA-1271 and CASSANDRA-1320 will add-to/improve IAuthenticator Pros: Least change Cons: Very little actually implemented by Cassandra: burden is on the backend implementors Each combination of authz and authc backends would require a new implementation (PAM for authc + permissions keyspace for authz, for instance), causing an explosion of implementations Separate out a pluggable IAuthority interface to implement authorization IAuthenticator interface would be called at login time to determine user/groups membership IAuthority would be called at operation time with the user/groups determined earlier, and the required permission for the operation Pros: Provides the cleanest separation of concerns Allows plugability for permissions Cons: Pluggability of permissions gains limited benefit IAuthority would need to support callbacks for keyspace/cf creation and removal to keep existing keyspaces in sync with their permissions (although technically, option 1 suffers from this as well) Separate authorization, but do not make it pluggable This option is implemented by the existing patchset by attaching permissions to metadata, but could have an alternative implementation that stores permissions in a permissions keyspace. Pros: Cassandra controls the scalability of authorization, and can ensure it does not become a bottleneck Cons: Would need to support callbacks for user creation and removal to keep existing users in sync with their permissions
          Hide
          Hudson added a comment -

          Integrated in Cassandra #504 (See http://hudson.zones.apache.org/hudson/job/Cassandra/504/)
          revert 980215, 980217, 980220, 980222, 980225, 980226. CASSANDRA-1237

          Show
          Hudson added a comment - Integrated in Cassandra #504 (See http://hudson.zones.apache.org/hudson/job/Cassandra/504/ ) revert 980215, 980217, 980220, 980222, 980225, 980226. CASSANDRA-1237
          Hide
          Folke Behrens added a comment -

          I don't think you can make authorization pluggable: authorization is very specific, it requires predefined permissions and/or group/role names, and authz is done at many places in the code. I recommend an authorization infrastructure that does not authorize users directly but groups or roles. An authenticator must return a list of groups a user belongs to and the authz infrastructure renders a list of permissions. In Cassandra you just ask if UserX.isAllowedTo(READ, "Keyspace1");

          Show
          Folke Behrens added a comment - I don't think you can make authorization pluggable: authorization is very specific, it requires predefined permissions and/or group/role names, and authz is done at many places in the code. I recommend an authorization infrastructure that does not authorize users directly but groups or roles. An authenticator must return a list of groups a user belongs to and the authz infrastructure renders a list of permissions. In Cassandra you just ask if UserX.isAllowedTo(READ, "Keyspace1");
          Hide
          Eric Evans added a comment -

          > If authorization should be pluggable (I've argued that is should be)
          I'd be interested in seeing the reasons for making permissions pluggable, if you know where I can find the thread.

          Directory services like LDAP and Active Directory seem like prominent examples of existing systems that people might want to integrate with for authorization, (as well as authentication). And, I'm sure there are plenty of people with existing databases, web services, etc that would appreciate the opportunity to integrate instead of duplicating that information.

          Show
          Eric Evans added a comment - > If authorization should be pluggable (I've argued that is should be) I'd be interested in seeing the reasons for making permissions pluggable, if you know where I can find the thread. Directory services like LDAP and Active Directory seem like prominent examples of existing systems that people might want to integrate with for authorization, (as well as authentication). And, I'm sure there are plenty of people with existing databases, web services, etc that would appreciate the opportunity to integrate instead of duplicating that information.
          Hide
          Stu Hood added a comment -

          > Separation of concerns. I tried to make the argument yesterday that mixing KS definitions with KS permissions was not the right design.
          Separation of concerns is not an argument for making it pluggable (pluggable implies multiple implementations), although it is an argument for storing the permissions somewhere other than in the metadata for the keyspace.

          Show
          Stu Hood added a comment - > Separation of concerns. I tried to make the argument yesterday that mixing KS definitions with KS permissions was not the right design. Separation of concerns is not an argument for making it pluggable (pluggable implies multiple implementations), although it is an argument for storing the permissions somewhere other than in the metadata for the keyspace.
          Hide
          Gary Dusbabek added a comment -

          >> If authorization should be pluggable (I've argued that is should be)
          >I'd be interested in seeing the reasons for making permissions pluggable, if you know where I can find the thread.

          Separation of concerns. I tried to make the argument yesterday that mixing KS definitions with KS permissions was not the right design.

          Show
          Gary Dusbabek added a comment - >> If authorization should be pluggable (I've argued that is should be) >I'd be interested in seeing the reasons for making permissions pluggable, if you know where I can find the thread. Separation of concerns. I tried to make the argument yesterday that mixing KS definitions with KS permissions was not the right design.
          Hide
          Stu Hood added a comment -

          > If authorization should be pluggable (I've argued that is should be)
          I'd be interested in seeing the reasons for making permissions pluggable, if you know where I can find the thread.

          Show
          Stu Hood added a comment - > If authorization should be pluggable (I've argued that is should be) I'd be interested in seeing the reasons for making permissions pluggable, if you know where I can find the thread.
          Hide
          Jonathan Ellis added a comment -

          I agree that committing this was premature, and also apologize for not looking at it earlier.

          Show
          Jonathan Ellis added a comment - I agree that committing this was premature, and also apologize for not looking at it earlier.
          Hide
          Folke Behrens added a comment -

          I'm sorry I deleted my patch too early. I wanted to open a new ticket but when I looked over my code I thought that it's not ready and I want to work on it over the weekend.

          The attached patch adds several classes:

          • JAASAuthenticator: an IAuthenticator implemention that uses a LoginContext to request authentication against configured LoginModules. Very simple stuff.
          • SimpleLoginModule: LoginModule impl that could replace SimpleAuthenticator.
          • SimpleLoginModuleConfiguration: programmatic configuration of LoginModules.
          • User/Group: implementations of java.security.Principal added by SimpleLoginModule to the Subject that is passed from the LoginContext to all configured LoginModules.

          This is only proof-of-concept code. I hope I have something cleaner and more robust ready by Sunday.

          Show
          Folke Behrens added a comment - I'm sorry I deleted my patch too early. I wanted to open a new ticket but when I looked over my code I thought that it's not ready and I want to work on it over the weekend. The attached patch adds several classes: JAASAuthenticator: an IAuthenticator implemention that uses a LoginContext to request authentication against configured LoginModules. Very simple stuff. SimpleLoginModule: LoginModule impl that could replace SimpleAuthenticator. SimpleLoginModuleConfiguration: programmatic configuration of LoginModules. User/Group: implementations of java.security.Principal added by SimpleLoginModule to the Subject that is passed from the LoginContext to all configured LoginModules. This is only proof-of-concept code. I hope I have something cleaner and more robust ready by Sunday.
          Hide
          Eric Evans added a comment -

          ...the reasoning behind this ticket is that backends like PAM aren't designed to provide storage for permissions.

          PAM is a bad example here, and there are plenty of other (more relevant) services and frameworks that do, (think LDAP, radius, tacacs, etc).

          I think that would be a mistake, without a working backend that filled the interface.

          Right, you'd need at least the equivalent of SimpleAuthenticator.

          Then, imagine how that backend might fill that interface, and I expect you'll come up with either storing the permissions in their own Keyspace, storing them in a backend specific manner, or attaching them as metadata to the keyspace. The first option is an alternative that should only be implemented once, and therefore shouldn't have an interface. The second could be handled by the IAuthenticator, and is what we have now. The third option is what is implemented here.

          If authorization should be pluggable (I've argued that is should be), then "backend specific" is the only option that makes sense.

          Show
          Eric Evans added a comment - ...the reasoning behind this ticket is that backends like PAM aren't designed to provide storage for permissions. PAM is a bad example here, and there are plenty of other (more relevant) services and frameworks that do, (think LDAP, radius, tacacs, etc). I think that would be a mistake, without a working backend that filled the interface. Right, you'd need at least the equivalent of SimpleAuthenticator. Then, imagine how that backend might fill that interface, and I expect you'll come up with either storing the permissions in their own Keyspace, storing them in a backend specific manner, or attaching them as metadata to the keyspace. The first option is an alternative that should only be implemented once, and therefore shouldn't have an interface. The second could be handled by the IAuthenticator, and is what we have now. The third option is what is implemented here. If authorization should be pluggable (I've argued that is should be), then "backend specific" is the only option that makes sense.
          Hide
          Stu Hood added a comment -

          > the approach of pluggable authenticators to PAM
          > we want to push that into the authenticator rather than hard-coding it somewhere
          I feel like you're mixing up 'authentication' with 'permissions/authorization'... the reasoning behind this ticket is that backends like PAM aren't designed to provide storage for permissions. Folke's JAAS example is a great example of authentication (and what he coded will still apply post 1237), but I haven't seen any JAAS/PAM backends that implement permissions storage.

          > Wouldn't it be reasonable to create another interface (IAuthority or whatever) and off-load how access levels are persisted
          I think that would be a mistake, without a working backend that filled the interface. Then, imagine how that backend might fill that interface, and I expect you'll come up with either storing the permissions in their own Keyspace, storing them in a backend specific manner, or attaching them as metadata to the keyspace. The first option is an alternative that should only be implemented once, and therefore shouldn't have an interface. The second could be handled by the IAuthenticator, and is what we have now. The third option is what is implemented here.

          Show
          Stu Hood added a comment - > the approach of pluggable authenticators to PAM > we want to push that into the authenticator rather than hard-coding it somewhere I feel like you're mixing up 'authentication' with 'permissions/authorization'... the reasoning behind this ticket is that backends like PAM aren't designed to provide storage for permissions. Folke's JAAS example is a great example of authentication (and what he coded will still apply post 1237), but I haven't seen any JAAS/PAM backends that implement permissions storage. > Wouldn't it be reasonable to create another interface (IAuthority or whatever) and off-load how access levels are persisted I think that would be a mistake, without a working backend that filled the interface. Then, imagine how that backend might fill that interface, and I expect you'll come up with either storing the permissions in their own Keyspace, storing them in a backend specific manner, or attaching them as metadata to the keyspace. The first option is an alternative that should only be implemented once, and therefore shouldn't have an interface. The second could be handled by the IAuthenticator, and is what we have now. The third option is what is implemented here.
          Hide
          Eric Evans added a comment -

          Attached patch shows a simple IAuthenticator for JAAS. With JAAS you can configure LoginModules for Unix users or PAM, for LDAP or Kerberos and you can also write your own LoginModule reading passwd.properties files or even column families. In fact, I have a SimpleLoginModule (similar to SimpleAuthenticator) half ready.
          This authenticator is also not finished, yet. I submitted it because I hope it's not too late to urge you to make/keep IAuthenticator as lightweight as possible.
          The proposed defaultUser() would make IAuthenticators somewhat stateful. Not good.

          If you're interested I can open a new issue and submit my JAAS classes there including sample config files and a programmatic JAAS configuration.

          What happened to this? This sounds quite interesting.

          Show
          Eric Evans added a comment - Attached patch shows a simple IAuthenticator for JAAS. With JAAS you can configure LoginModules for Unix users or PAM, for LDAP or Kerberos and you can also write your own LoginModule reading passwd.properties files or even column families. In fact, I have a SimpleLoginModule (similar to SimpleAuthenticator) half ready. This authenticator is also not finished, yet. I submitted it because I hope it's not too late to urge you to make/keep IAuthenticator as lightweight as possible. The proposed defaultUser() would make IAuthenticators somewhat stateful. Not good. If you're interested I can open a new issue and submit my JAAS classes there including sample config files and a programmatic JAAS configuration. What happened to this? This sounds quite interesting.
          Hide
          Eric Evans added a comment - - edited

          On the one hand, decoupling access levels seems like a Good Move, and from that stand-point this is an improvement over the status quo, but I disagree with the decision to bundle the access levels in keyspace definitions.

          Wouldn't it be reasonable to create another interface (IAuthority or whatever) and off-load how access levels are persisted to implementations (similar to how it is done with IAuthenticator)?

          -1 (in the ASF-sense), I believe this should be rolled back.

          EDIT: and I do apologize for coming into this late, as opposed to when it was actively being discussed.

          Show
          Eric Evans added a comment - - edited On the one hand, decoupling access levels seems like a Good Move, and from that stand-point this is an improvement over the status quo, but I disagree with the decision to bundle the access levels in keyspace definitions. Wouldn't it be reasonable to create another interface (IAuthority or whatever) and off-load how access levels are persisted to implementations (similar to how it is done with IAuthenticator)? -1 (in the ASF-sense), I believe this should be rolled back. EDIT: and I do apologize for coming into this late, as opposed to when it was actively being discussed.
          Hide
          Hudson added a comment -

          Integrated in Cassandra #503 (See http://hudson.zones.apache.org/hudson/job/Cassandra/503/)
          apply access.properties to KSM during loadSchemaFromYaml. patch by stuhood, reviewed by gdusbabek. CASSANDRA-1237
          move CS threadlocals into single object. patch by stuhood, reviewed by gdusbabek. CASSANDRA-1237
          new IAuthenticator interface. patch by stuhood, reviewed by gdusbabek. CASSANDRA-1237
          push access structures into KSM. patch by stuhood, reviewed by gdusbabek. CASSANDRA-1237
          put access structures in KsDef. patch by stuhood, reviewed by gdusbabek. CASSANDRA-1237
          move KSM modification code into copy methods. patch by stuhood, reviewed by gdusbabek. CASSANDRA-1237

          Show
          Hudson added a comment - Integrated in Cassandra #503 (See http://hudson.zones.apache.org/hudson/job/Cassandra/503/ ) apply access.properties to KSM during loadSchemaFromYaml. patch by stuhood, reviewed by gdusbabek. CASSANDRA-1237 move CS threadlocals into single object. patch by stuhood, reviewed by gdusbabek. CASSANDRA-1237 new IAuthenticator interface. patch by stuhood, reviewed by gdusbabek. CASSANDRA-1237 push access structures into KSM. patch by stuhood, reviewed by gdusbabek. CASSANDRA-1237 put access structures in KsDef. patch by stuhood, reviewed by gdusbabek. CASSANDRA-1237 move KSM modification code into copy methods. patch by stuhood, reviewed by gdusbabek. CASSANDRA-1237
          Hide
          Jonathan Ellis added a comment -

          similarly, the AccessLevel change is moving 180 degrees in the wrong direction – we want to push that into the authenticator rather than hard-coding it somewhere

          Show
          Jonathan Ellis added a comment - similarly, the AccessLevel change is moving 180 degrees in the wrong direction – we want to push that into the authenticator rather than hard-coding it somewhere
          Hide
          Jonathan Ellis added a comment -

          committing to a users-and-groups approach seems very wrongheaded to me. the approach of pluggable authenticators to PAM etc seems much better to me (although it's hard to say now since that patch was deleted – bad practice there...)

          Show
          Jonathan Ellis added a comment - committing to a users-and-groups approach seems very wrongheaded to me. the approach of pluggable authenticators to PAM etc seems much better to me (although it's hard to say now since that patch was deleted – bad practice there...)
          Hide
          Gary Dusbabek added a comment -

          +1 committed.

          Show
          Gary Dusbabek added a comment - +1 committed.
          Hide
          Folke Behrens added a comment -

          I see.

          A few suggestions then:

          • rename "AuthenticatedUser" to just "User".
          • add "isAuthenticated" state to User.
          • add package private getter(s) and setter(s) that allow Authenticators to manipulate their User objects.
          • add "User" as argument to "login" (and "logout"). Authenticators use the credentials to fill in the details.
          • rename "defaultUser" to "createUser" to make it clear that it's a factory method that must always return a (new) User who may or may not already be authenticated.

          I know. It's probably too much.

          Show
          Folke Behrens added a comment - I see. A few suggestions then: rename "AuthenticatedUser" to just "User". add "isAuthenticated" state to User. add package private getter(s) and setter(s) that allow Authenticators to manipulate their User objects. add "User" as argument to "login" (and "logout"). Authenticators use the credentials to fill in the details. rename "defaultUser" to "createUser" to make it clear that it's a factory method that must always return a (new) User who may or may not already be authenticated. I know. It's probably too much.
          Hide
          Stu Hood added a comment -
          • 0003: Fixed unavronateAccessMap null check (good eye!)
          • 0004: Moved 'isSuper' onto AuthenticatedUser
          • 0004: Fixed static/instance String.format usage
          • 0005: Fixed an error where calling set_keyspace before login would fail

          I can't think of a good way to remove defaultUser: it replaced lots of (authenticator instanceof AllowAllAuthenticator) calls, which existed to check whether it was necessary for the user to login.

          Show
          Stu Hood added a comment - 0003: Fixed unavronateAccessMap null check (good eye!) 0004: Moved 'isSuper' onto AuthenticatedUser 0004: Fixed static/instance String.format usage 0005: Fixed an error where calling set_keyspace before login would fail I can't think of a good way to remove defaultUser: it replaced lots of (authenticator instanceof AllowAllAuthenticator) calls, which existed to check whether it was necessary for the user to login.
          Hide
          Folke Behrens added a comment -

          In AuthenticatedUser.toString(): String.format() is a static method that takes the format as its first parameter.

          Again, please drop defaultUser() and put the "super admin" status directly into AuthenticatedUser or use a not configurable pseudo group for super admins.

          Show
          Folke Behrens added a comment - In AuthenticatedUser.toString(): String.format() is a static method that takes the format as its first parameter. Again, please drop defaultUser() and put the "super admin" status directly into AuthenticatedUser or use a not configurable pseudo group for super admins.
          Hide
          Gary Dusbabek added a comment -

          0003: unavronateAccessMap is checking for null on the wrong variable.
          0004: is there a way to not have a default user? I think it adds some noise to the interface.

          Feel free to make breaking changes (no need to support access.properties) if it simplifies things. Our authentication API has been explicitly 'experimental' from day one.

          Show
          Gary Dusbabek added a comment - 0003: unavronateAccessMap is checking for null on the wrong variable. 0004: is there a way to not have a default user? I think it adds some noise to the interface. Feel free to make breaking changes (no need to support access.properties) if it simplifies things. Our authentication API has been explicitly 'experimental' from day one.
          Hide
          Gary Dusbabek added a comment -

          Folke, I'd be interested in seeing your classes (on a separate ticket).

          Show
          Gary Dusbabek added a comment - Folke, I'd be interested in seeing your classes (on a separate ticket).
          Hide
          Folke Behrens added a comment -

          By the way, I think the constants USERNAME_KEY and PASSWORD_KEY should go into IAuthenticator because these are common keys used by all authenticators.

          Show
          Folke Behrens added a comment - By the way, I think the constants USERNAME_KEY and PASSWORD_KEY should go into IAuthenticator because these are common keys used by all authenticators.
          Hide
          Folke Behrens added a comment -

          Attached patch shows a simple IAuthenticator for JAAS. With JAAS you can configure LoginModules for Unix users or PAM, for LDAP or Kerberos and you can also write your own LoginModule reading passwd.properties files or even column families. In fact, I have a SimpleLoginModule (similar to SimpleAuthenticator) half ready.

          This authenticator is also not finished, yet. I submitted it because I hope it's not too late to urge you to make/keep IAuthenticator as lightweight as possible.
          The proposed defaultUser() would make IAuthenticators somewhat stateful. Not good.

          If you're interested I can open a new issue and submit my JAAS classes there including sample config files and a programmatic JAAS configuration.

          Show
          Folke Behrens added a comment - Attached patch shows a simple IAuthenticator for JAAS. With JAAS you can configure LoginModules for Unix users or PAM, for LDAP or Kerberos and you can also write your own LoginModule reading passwd.properties files or even column families. In fact, I have a SimpleLoginModule (similar to SimpleAuthenticator) half ready. This authenticator is also not finished, yet. I submitted it because I hope it's not too late to urge you to make/keep IAuthenticator as lightweight as possible. The proposed defaultUser() would make IAuthenticators somewhat stateful. Not good. If you're interested I can open a new issue and submit my JAAS classes there including sample config files and a programmatic JAAS configuration.
          Hide
          Stu Hood added a comment -

          Rebased for trunk.

          Show
          Stu Hood added a comment - Rebased for trunk.
          Hide
          Stu Hood added a comment -

          Rebased for trunk, and added 0006 to handle upgrades by parsing the deprecated 'access.properties' when users call readTablesFromYaml.

          This should be ready for review.

          Show
          Stu Hood added a comment - Rebased for trunk, and added 0006 to handle upgrades by parsing the deprecated 'access.properties' when users call readTablesFromYaml. This should be ready for review.
          Hide
          Stu Hood added a comment -

          > Failing to authenticate correctly is not an exception.
          (Keeping in mind that I didn't change any of the Exception handling/throwing in this patch) I think I agree with the original decision. A failed authentication should be extremely rare, and therefore exceptional.

          > All RPC methods except "login" should throw AuthorizationException.
          Agreed, but out of the scope of this ticket.

          > While you're at it, please look at CASSANDRA-974. I suggest renaming "login" to "authenticate"
          I'm thinking of the IAuthenticator interface and login() methods as stopgaps until Avro adds support for SASL in their custom protocol (AVRO-341), so I don't think breaking the Thrift API right now is worth it.

          Show
          Stu Hood added a comment - > Failing to authenticate correctly is not an exception. (Keeping in mind that I didn't change any of the Exception handling/throwing in this patch) I think I agree with the original decision. A failed authentication should be extremely rare, and therefore exceptional. > All RPC methods except "login" should throw AuthorizationException. Agreed, but out of the scope of this ticket. > While you're at it, please look at CASSANDRA-974 . I suggest renaming "login" to "authenticate" I'm thinking of the IAuthenticator interface and login() methods as stopgaps until Avro adds support for SASL in their custom protocol ( AVRO-341 ), so I don't think breaking the Thrift API right now is worth it.
          Hide
          Folke Behrens added a comment -

          Failing to authenticate correctly is not an exception. OTOH, accessing a keyspace you're not authorized for is. All RPC methods except "login" should throw AuthorizationException.

          While you're at it, please look at CASSANDRA-974. I suggest renaming "login" to "authenticate" and have it return Map<String,String> to make it future-proof for SASL authentication schemes, like DIGEST-MD5.

          Show
          Folke Behrens added a comment - Failing to authenticate correctly is not an exception. OTOH, accessing a keyspace you're not authorized for is. All RPC methods except "login" should throw AuthorizationException. While you're at it, please look at CASSANDRA-974 . I suggest renaming "login" to "authenticate" and have it return Map<String,String> to make it future-proof for SASL authentication schemes, like DIGEST-MD5.
          Hide
          Stu Hood added a comment -

          This should be ready for review.

          One thing that might block it getting committed, though, is that we probably need to automate the conversion from 'access.properties' to per-keyspace access maps. Perhaps during loadFromYaml, we could check for the existence of an 'access.properties' file, and apply the properties found there to the new keyspaces?

          Show
          Stu Hood added a comment - This should be ready for review. One thing that might block it getting committed, though, is that we probably need to automate the conversion from 'access.properties' to per-keyspace access maps. Perhaps during loadFromYaml, we could check for the existence of an 'access.properties' file, and apply the properties found there to the new keyspaces?
          Hide
          Stu Hood added a comment -

          Rebased post 1186.

          Show
          Stu Hood added a comment - Rebased post 1186.
          Hide
          Stu Hood added a comment -

          The set currently uses our old-school KSMetaData serialization, so it will need a rebase when CASSANDRA-1186 makes it in.

          Show
          Stu Hood added a comment - The set currently uses our old-school KSMetaData serialization, so it will need a rebase when CASSANDRA-1186 makes it in.
          Hide
          Stu Hood added a comment -

          Patchset to separate permissions from authentication.

          Show
          Stu Hood added a comment - Patchset to separate permissions from authentication.

            People

            • Assignee:
              Stu Hood
              Reporter:
              Stu Hood
              Reviewer:
              Eric Evans
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development