Rather than paste in the entire text, I'm just linking to the wiki writeup. Please read the whole thing first and if you want to comment on anything specific, quote it and comment on this thread. https://www.aeolusproject.org/redmine/projects/aeolus/wiki/Adding_Permiss... Scott

In "LDAP Groups" mode we won't pull in all user groups from the LDAP server, as there may well be many groups that aren't related to Conductor use, so we'll have an "Import LDAP User Group" action, analogous to "Create User Group" action for standalone. This will bring up a list of LDAP groups exposed by the LDAP server, allowing the admin to choose one (or more? -- not sure if we'll allow multiple groups at once yet) to import. The UserGroup will be created in conductor, with the memberships pulled into Conductor at the same time. Any group members that are not yet imported into the Conductor database as Conductor users will be pulled in at the same time.

For sync-on-use, the idea is we would store a "last sync time" every time we re-sync group memberships. Any time we make a permissions check, the checking code would call a method on Group to refresh if necessary (i.e. refresh if last refresh was longer ago than $refresh-interval, otherwise return and do nothing). Group.refresh_from_ldap_if_necessary would block while checking last sync time and (if necessary) while performing the update using an ActiveRecord pessimistic lock on the "last refresh time" data element in order to prevent more than one web UI front end process (or thread) from attempting to re-sync at the same time.

On 04/03/2012 08:08 AM, Jan Provaznik wrote: > On 03/30/2012 03:52 PM, Scott Seago wrote: >> Rather than paste in the entire text, I'm just linking to the wiki >> writeup. Please read the whole thing first and if you want to >> comment on anything specific, quote it and comment on this thread. >> >> https://www.aeolusproject.org/redmine/projects/aeolus/wiki/Adding_Permiss... >> >> >> >> >> >> Scott > > >> In "LDAP Groups" mode we won't pull in all user groups from the LDAP >> server, as there may well be many groups that aren't related to >> Conductor use, so we'll have an "Import LDAP User Group" action, >> analogous to "Create User Group" action for standalone. This will >> bring up a list of LDAP groups exposed by the LDAP server, allowing >> the admin to choose one (or more? -- not sure if we'll allow >> multiple groups at once yet) to import. The UserGroup will be created >> in conductor, with the memberships pulled into Conductor at the same >> time. Any group members that are not yet imported into the Conductor >> database as Conductor users will be pulled in at the same time. >> > > "Import LDAP User Group" action implies that an admin has to manually > add a new group in conductor anytime the group is added on LDAP server? > It would be nice to have both user and group lists configurable just by > setting ldap query (in config file). > This is correct. I don't think we want to automatically pull in all LDAP groups, since most of them will probably be not relevant for Conductor purposes. We _could_ add a global "automatically import _every_ group from LDAP" flag at some future point -- it wouldn't really affect much of the design here, but we don't want that to be mandatory, so we still need the 'import' feature.

You bring up another point -- we could possibly generate a custom query to generate the group list. Again this would probably need to be in addition to the manual import, and possibly not for the first version here (unless there's a compelling need for it) >> For sync-on-use, the idea is we would store a "last sync time" every >> time we re-sync group memberships. Any time we make a permissions >> check, the checking code would call a method on Group to refresh if >> necessary (i.e. refresh if last refresh was longer ago than >> $refresh-interval, otherwise return and do nothing). >> Group.refresh_from_ldap_if_necessary would block while checking last >> sync time and (if necessary) while performing the update using an >> ActiveRecord pessimistic lock on the "last refresh time" data element >> in order to prevent more than one web UI front end process (or >> thread) from attempting to re-sync at the same time. >> > > -1 for this (+1 for doing this on background), I experienced many > situations when customer's LDAP server was responding too slow. > Typical real-life situation is: LDAP is used for SMTP auth, SMTP > server is overloaded -> LDAP responses are slow. IOW slow LDAP is much > more common then it would be expected. > > We have other background tasks anyway, so there is not much additional > work to do this on background. > > Jan OK, yeah performance was my main concern with the sync-on-use. Jay was more firmly of the opinion that the polling solution was less desirable, but if slow LDAP server response is going to be a common occurance, I'd prefer the background polling as well. Scott

On Tue, Apr 03, 2012 at 02:08:53PM +0200, Jan Provaznik wrote: > > -1 for this (+1 for doing this on background), I experienced many > situations when customer's LDAP server was responding too slow. > Typical real-life situation is: LDAP is used for SMTP auth, SMTP > server is overloaded -> LDAP responses are slow. IOW slow LDAP is > much more common then it would be expected. I think I actually agree with Jan, but just to play devil's advocate... I don't believe it's ever safe to cache LDAP data, because we have no way of knowing when it changes. We could cache things for the duration of a method call (like Rails does with SQL in controllers), but I think anything beyond that is technically unsafe. And it's not like we're concerned about possibly serving a blog post a couple of seconds after it's deleted -- we're talking about caching security information with having no way of knowing when what we've cached becomes out of date. Continuing as devil's advocate for a moment, I would contend that if someone's LDAP server is slow, it's not our fault, and is in fact unavoidable, that our application will be slow during permission checks. And building on the SMTP auth example -- this implies that the mailserver is not caching LDAP information, because they deemed it unacceptable for spam to go to a user once their account is turned off, or for a new user to not receive mail for a few minutes. So surely, we shouldn't allow user to spin up (or terminate!) cloud instances after they were fired. At a previous employer, the practice was to move employees who left the company to the "Exited Users" Active Directory group, so even caching group membership is unsafe. Poking around a bit, I'm seeing other projects where their "cache" was per-pageload -- along the lines of what I was saying with the Rails SQL cache, which only lives as long as the controller action. The next pageload will do the same queries. I started this off mostly playing devil's advocate, and being a big fan of caching. But in the course of doing so, I think I actually changed my mind, and come to the opinion that we really shouldn't be caching user authentication data. I think we need to re-query on every permission check, even if this is going to slow things down considerably. In practice this information changes relatively infrequently, but since we have no way of knowing WHEN it changes, I think that caching it in our database is unsafe and improper. -- Matt

On Wed, Apr 04, 2012 at 03:27:32PM -0400, Jason Guiditta wrote: > On 04/04/12 15:18 -0400, Matt Wagner wrote: >> On Tue, Apr 03, 2012 at 02:08:53PM +0200, Jan Provaznik wrote: >>> -1 for this (+1 for doing this on background), I experienced many >>> situations when customer's LDAP server was responding too slow. >>> Typical real-life situation is: LDAP is used for SMTP auth, SMTP >>> server is overloaded -> LDAP responses are slow. IOW slow LDAP is >>> much more common then it would be expected. >> I think I actually agree with Jan, but just to play devil's advocate... >> >> I don't believe it's ever safe to cache LDAP data, because we have no >> way of knowing when it changes. We could cache things for the duration >> of a method call (like Rails does with SQL in controllers), but I think >> anything beyond that is technically unsafe. And it's not like we're >> concerned about possibly serving a blog post a couple of seconds after >> it's deleted -- we're talking about caching security information with >> having no way of knowing when what we've cached becomes out of date. >> >> Continuing as devil's advocate for a moment, I would contend that if >> someone's LDAP server is slow, it's not our fault, and is in fact >> unavoidable, that our application will be slow during permission checks. >> And building on the SMTP auth example -- this implies that the >> mailserver is not caching LDAP information, because they deemed it >> unacceptable for spam to go to a user once their account is turned off, >> or for a new user to not receive mail for a few minutes. So surely, we >> shouldn't allow user to spin up (or terminate!) cloud instances after >> they were fired. >> >> At a previous employer, the practice was to move employees who left the >> company to the "Exited Users" Active Directory group, so even caching >> group membership is unsafe. >> >> Poking around a bit, I'm seeing other projects where their "cache" was >> per-pageload -- along the lines of what I was saying with the Rails SQL >> cache, which only lives as long as the controller action. The next >> pageload will do the same queries. >> >> I started this off mostly playing devil's advocate, and being a big fan >> of caching. But in the course of doing so, I think I actually changed my >> mind, and come to the opinion that we really shouldn't be caching user >> authentication data. I think we need to re-query on every permission >> check, even if this is going to slow things down considerably. In >> practice this information changes relatively infrequently, but since we >> have no way of knowing WHEN it changes, I think that caching it in our >> database is unsafe and improper. >> >> -- Matt > Matt, these are excellent points. I was pretty much on board with > moving this to some tbd background process, but this makes me > seriously question that direction. Perhaps if a company cannot > maintain a performant ldap server, they should just choose the 'local > groups' option for conductor? Anecdotally, I use the rh ldap all the > time to auto-complete email addresses in mutt. I am at least one > state away from the closest office, and have never had any significant > wait on such calls. > > -j You guys might want to check with someone in the IPA group to get a sense of what standard practice is here, but I'm pretty sure caching identity is a no-no. --H

On 04/04/2012 03:18 PM, Matt Wagner wrote: So we're not talking about authentication data here -- that's all in the session still. But for group membership, with a short enough timeout/refresh interval we're _probably_ safe -- we're talking about "propagation time" rather than erroneous permission results, really. That said, we could do it in another way that avoid caching group membership entirely (but it does complicate the permission checks, preventing us from gaining the full benefit of the denormalization). Essentially we could do the following: 1) denormalize _inherited_ permission relations as planned (i.e. explicit records on the instance for users and groups with access from whatever level in the hierarchy 2) do _not_ denormalize group members in the above -- i.e. if "Joe" is in the "Engineering Management" group, and "Engineering Management" has the "Pool Administrator" role in the Engineering pool, and we have a "Redmine" deployment in that pool then we would not have a denormalized permission record for "(Joe, Pool Administrator, Redmine)" -- we would only have one for "(Engineering Management Group, Pool Administrator, Redmine)" 3) for permissions checks, instead of simply doing a lookup on the denormalized table for Joe, we'd have to look for both Group and invididual grants, after pulling the current group assignments for the current user i.e. my proposal would leave just this for base permission checks: return true obj.derived_permissions.find(:first, :include => [:role => :privileges], :conditions => ["derived_permissions.user_id=:user and privileges.target_type=:target_type and privileges.action=:action", { :user => user.id, :target_type => target_type.name, :action => action}]) Instead we'd first need to pull the user's LDAP group list, and then join _twice_ with permissions, once verifying user => user.id, and again verifying group in user.groups Scott

On Wed, Apr 04, 2012 at 04:01:49PM -0400, Scott Seago wrote: > On 04/04/2012 03:18 PM, Matt Wagner wrote: >> On Tue, Apr 03, 2012 at 02:08:53PM +0200, Jan Provaznik wrote: >>> -1 for this (+1 for doing this on background), I experienced many >>> situations when customer's LDAP server was responding too slow. >>> Typical real-life situation is: LDAP is used for SMTP auth, SMTP >>> server is overloaded -> LDAP responses are slow. IOW slow LDAP is >>> much more common then it would be expected. >> I think I actually agree with Jan, but just to play devil's advocate... >> >> I don't believe it's ever safe to cache LDAP data, because we have no >> way of knowing when it changes. We could cache things for the duration >> of a method call (like Rails does with SQL in controllers), but I think >> anything beyond that is technically unsafe. And it's not like we're >> concerned about possibly serving a blog post a couple of seconds after >> it's deleted -- we're talking about caching security information with >> having no way of knowing when what we've cached becomes out of date. >> > You're acting as devil's advocate for a proposal that wasn't one of > the two options I'd proposed :-) I am practicing in case I decide to run for political office. > Both of my suggestions involved caching the results, to be considered > valid for some period of time. The only difference was whether we do > it on a set polling interval or if we cache it "on demand" -- i.e. on > the first demand for it, then use it for a bit until it's considered > "stale" again. > > If we don't cache LDAP data, the whole proposal for denormalizing > permission design becomes invalid, since the idea is to have a direct > "grantee" list per object to filter on for list_for_user, etc. I'm still not convinced that caching LDAP group memberships is ever safe, though I suppose if the caching interval is very small (e.g., one minute), it's not exactly at "juggling chainsaws" levels. >> Continuing as devil's advocate for a moment, I would contend that if >> someone's LDAP server is slow, it's not our fault, and is in fact >> unavoidable, that our application will be slow during permission checks. >> And building on the SMTP auth example -- this implies that the >> mailserver is not caching LDAP information, because they deemed it >> unacceptable for spam to go to a user once their account is turned off, >> or for a new user to not receive mail for a few minutes. So surely, we >> shouldn't allow user to spin up (or terminate!) cloud instances after >> they were fired. >> > Hmm. Again my proposal (that Jan was objecting to) still cached -- > but we cached "on demand" -- so every so often we'd potentially slow > down on a permissions check, then we'd be fast for a while until > things were considering stale. So on some level, I suppose, rather than objecting to one proposal, I was objecting to both of them. Although having talked about this a bit on IRC, I think we may have found a way to make this workable. (More further down the email.) >> At a previous employer, the practice was to move employees who left the >> company to the "Exited Users" Active Directory group, so even caching >> group membership is unsafe. >> > Well if you're caching for 30 seconds (or something similarly small) > you could argue that it's not unsafe -- would it really have mattered > if the admin had been 30 seconds later in moving the user to te > "exited users" group? I still think that it's ideologically wrong to cache this information for any length of time, but I have to agree that the pragmatic implications of caching something for under a minute are probably minimal. (Except for cases where an administrator has JUST added a user for the sole purpose of seeing the change in Conductor.) >> Poking around a bit, I'm seeing other projects where their "cache" was >> per-pageload -- along the lines of what I was saying with the Rails SQL >> cache, which only lives as long as the controller action. The next >> pageload will do the same queries. >> >> I started this off mostly playing devil's advocate, and being a big fan >> of caching. But in the course of doing so, I think I actually changed my >> mind, and come to the opinion that we really shouldn't be caching user >> authentication data. I think we need to re-query on every permission >> check, even if this is going to slow things down considerably. In >> practice this information changes relatively infrequently, but since we >> have no way of knowing WHEN it changes, I think that caching it in our >> database is unsafe and improper. >> > So we're not talking about authentication data here -- that's all in > the session still. But for group membership, with a short enough > timeout/refresh interval we're _probably_ safe -- we're talking about > "propagation time" rather than erroneous permission results, really. I'm not sure that group membership is any less of a big deal than a user's status in LDAP -- such as my example about a user who might move from the "IT Administrator" group to the "Terminated Employees" group. But in either case, if it's in the level that it's a "propagation time" I think we can probably get away with it. > That said, we could do it in another way that avoid caching group > membership entirely (but it does complicate the permission checks, > preventing us from gaining the full benefit of the denormalization). I should use this opportunity to note that I'm talking about what I think is "right" in an academic/theoretical sense, not necessarily the pragmatically-best solution. So please don't take this as vociferous disagreement. > Essentially we could do the following: > 1) denormalize _inherited_ permission relations as planned (i.e. > explicit records on the instance for users and groups with access > from whatever level in the hierarchy > 2) do _not_ denormalize group members in the above -- i.e. if "Joe" > is in the "Engineering Management" group, and "Engineering > Management" has the "Pool Administrator" role in the Engineering > pool, and we have a "Redmine" deployment in that pool then we would > not have a denormalized permission record for "(Joe, Pool > Administrator, Redmine)" -- we would only have one for "(Engineering > Management Group, Pool Administrator, Redmine)" Not to bleed both of your tasks together, but I also have a feature planning page for caching various data in memcache. It may very well be much better to denormalize this data as you have proposed, but just thought I'd mention that stuffing group lists in memcache may be another option. (Although since you can't exactly do joins on memcache keys, I'm not sure this would buy us anything here.) > 3) for permissions checks, instead of simply doing a lookup on the > denormalized table for Joe, we'd have to look for both Group and > invididual grants, after pulling the current group assignments for > the current user > > i.e. my proposal would leave just this for base permission checks: > return true obj.derived_permissions.find(:first, :include => [:role > => :privileges], > :conditions => ["derived_permissions.user_id=:user and > privileges.target_type=:target_type and > privileges.action=:action", > { :user => user.id, > :target_type => target_type.name, > :action => action}]) > > Instead we'd first need to pull the user's LDAP group list, and then > join _twice_ with permissions, once verifying user => user.id, and > again verifying group in user.groups Indeed, this bit sounds like it would just make things a huge nightmare, both to code and performance-wise. I do want to conclude by noting that I'm not necessarily opposed to this idea, and I certainly don't vehemently oppose it. It's just that the notion of caching LDAP data "feels wrong" and seems like it could present some challenges, so I want to make sure it is, indeed, a necessarily evil. -- Matt