My favorites | Sign in
Project Home Downloads Wiki Issues Source
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 694: LDAP should mark users inactive
9 people starred this issue and may be notified of changes. Back to list
Status:  Started
Owner:  ----


Sign in to add a comment
 
Project Member Reported by sop@google.com, Aug 28, 2010
Some organizational directories keep accounts inside of
their LDAP servers, but mark them as inactive when an
intern or employee exits the company.

Gerrit should be configurable to copy that flag into its
own accounts inactive flag, so these users aren't shown
as suggestions for reviewers.
Jun 7, 2011
#1 ben.ma...@gmail.com
I've written this functionality into DatabasePubKey by writing a few additional classes to query the database and LDAP. 

Currently, it will collect a list of deactivated accounts from LDAP every 15 minutes and compare these accounts with incoming ssh requests. If the user is on the list they are marked inactive and also flagged in the database as inactive. 

I do not have a good grasp on Guice and therefore the code doesn't conform well with the rest of Gerrit. Also, I do not understand how to invoke objects of a class in Guice. It would be very helpful if I could have access to the config file path but I was unable to find where it is read in during runtime. Any advice or information is highly appreciated- I've read most of the information available for Guice but Gerrit is highly confusing for me.
Jun 7, 2011
#2 sop@google.com
Publish a change for review, its easier to make direct comments about code than try to pontificate about generalities on the issue tracker.
Jun 10, 2011
#3 ben.ma...@gmail.com
Ok, I'll go ahead and submit what I have. Most of the code is redundant if I could figure out how to access the classes LDAPRealm and reviewDB but hopefully you guys we'll be able to point me in the right direction and I can see how its supposed to be implemented in the Guice framework. 

An issue I noticed was that if the user had a valid SSH key they were granted access regardless of the LDAP account status. Once I had fixed this issue, the database's 'inactive' column was not being changed to match the isActive value for the account. This all takes place inside DatabasePubAuth.authenticate() because the security hole we observed was only in the ssh interface with gerrit. The UI Login works well although it may not be deactivating an account that fails authentication- this patch rigs that hole. To address overloading the LDAP with requests- I set a very simple 'timer' that will only query the ldap once the difference between 2 ssh requests is greater than a set time limit (TIME_LIMIT in LDAPLockedAccounts).

All comments/suggests/concerns are appreciated- feel free to tear my code apart as I'm just picking up Java and also very new to Guice. This is the first project I've been assigned as an intern so it is very specific to our infrastructure and I've got it working effectively but its obviously not ideal.

Regards,

Benjamin Mays
Jun 28, 2011
#4 ben.ma...@gmail.com
I have since refactored the code to embrace Guice and integrate more smoothly into Gerrit. I am currently awaiting manager approval to submit the code. I have run into another issue though, I would like to make the ldap search filter for disabled accounts and also the time in between ldap queries to be configurable within the gerrit.config file. 

Where would be the best place to make these changes? It seems that JGit contains the config class used to parse the file- should I look here to add in a few extra optional arguments or is that handled somewhere within Gerrit that I've overlooked?

Regards,

Benjamin Mays
Jun 28, 2011
#5 sop@google.com
Add to your class' constructor

  @GerritServerConfig Config cfg

This will set cfg to the read copy of gerrit.config. Now you can use getString(), getBoolean(), getInt(), or getEnum() to read from the file. If you want to read a time unit, use ConfigUtil.getTimeUnit(cfg, ...).
Status: Started
Labels: -Milestone-2.1.6
Jun 28, 2011
#6 ben.ma...@gmail.com
Ahh, you may have misunderstood my question but I do appreciate the quick response. That is definitely a valid method for using the config class but I want to add to the config schema. Example:

[auth]
   type = LDAP
[ldap]
   server = ...
   username = ...
   ..
 + query_interval = 15
 + disabled_acct_filter = (|(case1)(case2)(etc))

and then be able to access it via the config class like so:

config.getString("ldap", null, "query_interval") etc.

I'd rather not hard code these fields for obvious reasons. Specifically for debugging, it is a pain to wait 15 minutes to see if an account has been set to inactive. Also, for different LDAP configurations the filter used to determine a locked account should be configurable due to different LDAP environments.

Also, this addresses issues brought up in this discussion: http://bit.ly/m8FPiM

What I've noticed is that the inactive flag in the db is not used for authentication- at least not when LDAP is configured. I was going to sync the flags- LDAP and DB but noticed no gain in security. What role does the inactive flag play in Gerrit? When is it set/unset? I read something to the effect that it is used internally, but it doesn't even match a current Account object's isActive member, if I remember correctly. I'm working on getting permission to release some of these changes, hopefully it will improve the overall cohesion of Gerrit's security features.

Regards,

Benjamin Mays
Jun 28, 2011
#7 sop@google.com
There is no "config schema". The configuration file is just an untyped dictionary of string values. We have some conventions about what strings should appear, and after that, we cross our fingers that its right, and throw exceptions when its not. :-)

So all you need to do is access the value using getString() like you wished in your comment. But for interval things I would encourage using the ConfigUtil.getTimeUnit() helper method, to convert the String into a long that has a known TimeUnit value (MILLISECONDS or SECONDS).

The inactive flag does lock out the SSH connection, but as you noticed it is not tied to LDAP. Currently it must be updated manually. Originally it was added to prevent the user from appearing in the suggestion list in the add reviewer box on a change. Its been extended (slowly) towards the security model too.

As far as getting the inactive account data from LDAP... I would create a new cache inside of Gerrit called "ldap_inactive". Have the default maxAge for this cache be 15 minutes. Instead of using an ldap.query_interval to define how often to query, use the cache. During SSH or HTTP login, lookup the user in the cache. If the cache entry is valid, it will be returned as-is. If the cache entry is invalid, the cache's loader can trigger and query LDAP for that user to see if the inactive field has been set on the account. If it has, we mark the account inactive in the accounts table, and store the inactive status in the cache.

This way we do a lookup every 15 minutes per user account, but only for users that are actively trying to login. This avoids the mess of an organization having 5000 inactive accounts, and trying to scan through all 5000 inactive accounts every 15 minutes to see who has suddenly become inactive.

Site administrators can increase the inactive check window to say once a day in their gerrit.config file with:

  [cache "ldap_inactive"]
    maxAge = 1 day

and have Gerrit only check each user once per day, or after server restarts.


I guess we would still need a batch task to check all inactive accounts against all known gerrit accounts to mark the account as inactive, in case the user leaves and never tries to sign-in again. This way they stop getting suggested as a reviewer, etc. But this could run with a less frequent check interval, and it may be faster to read the local Gerrit accounts table and query LDAP once for each account, than to query the entire directory. E.g. the directory of inactive accounts might be 8000+ people, but this Gerrit server might only have 1000 users on it. Consider a large company with a lot of seasonal/temporary workers who get LDAP accounts, but then the company has this Gerrit server with a largely stable developer staff. Those temporary workers won't ever login to Gerrit, and thus we don't need to worry about their inactive account status.

An advantage of the "ldap_inactive" cache is, a site administrator can force a cache flush and make every user revalidate their LDAP account status on the next login. This is also why "web_sessions" is a cache. If there were some especially sensitive accounts being locked in LDAP, the Gerrit administrator could flush both of these caches with a single `gerrit flush-caches --cache ldap_inactive --cache web_sessions` call and make the LDAP changes immediately take effect, rather than waiting for the query interval to come around.

For some examples look at the GROUP_CACHE in LdapModule:

    core(groups, GROUP_CACHE).maxAge(1, HOURS) //
        .populateWith(LdapRealm.MemberLoader.class);

And its loader, LdapRealm.MemberLoader. "ldap_inactive" is similar to "ldap_groups".
Jun 28, 2011
#8 ben.ma...@gmail.com
dictionary of string values. We have some conventions about what strings
should appear, and after that, we cross our fingers that its >right, and
throw exceptions when its not. :-)

Hah, I had assumed as much but exceptions were thrown after adding the
values. I guess they didn't meet the standard. Regardless, the requirement
is obsolete if we implement the suggestions you've made. I'll definitely
take a look tomorrow and attempt to work some of them into the project as
the fit into our requirements.

Thank you, again.
Jun 28, 2011
#9 sop@google.com
I missed what was wrong with your earlier attempt. An "_" is not allowed in a key name in the configuration file. "-" is allowed, but "_" is not. By convention we use camelCaseNames for keys in the configuration. :-)
Sign in to add a comment

Powered by Google Project Hosting