(ticket-537) user bindUser for group detection

[Rainer Weinhold]

Hi,

as mentioned for my pull request ( https://github.com/gitblit/gitblit/pull/247 ) there is discussion needed.

> [ gitblit comment :]
> At a quick glance you are changing the existing behavior to solve your issue, but you may be breaking other installs. This needs some discussion on the mailing list. Clearly with LDAP what works for some may not work for all - hence the reason for your PR.

I agree this will change behavior,  would suggest to add a new settings parameter for this behavior.

These are the probable scenarios:

Scenario 1: search user and normal user can see everything
 @sync = true / false
  ->old/new: behavior should be the same

Scenario 2: search user can see everything, but normal user cannot see groups
 @sync = false
   -> old: on login all user's teams will be written to [] because ldap thinks the user does not have any group.
   -> new: on login user group will be set correctly
 @sync = true
  -> old: on login all user's teams will be written to [] because ldap thinks the user does not have any group -> This is a Bug, because at each sync intervall the user groups were red correctly
  -> new: on login user group will be set correctly, so there is no difference in the sync/nosync behavior

Scenario 3: search user cannot see groups, but normal user can
 @sync = false
  -> old: after login user's teams would be red correctly
  -> new : after login user's teams would be []
 @sync = true
  -> old: after login user's teams would be red correctly
  -> new : after login user's teams would be []

Cheers Rainer

[Florian Zschocke]

I would like to ask what the intent is/was behind certain LDAP settings.
I am asking this because there is a pull request 162 which introduced the realm.ldap.bindpattern setting. The description in the properties file and the description in the pull request do not quite match.
The description of the setting says

# Bind pattern for Authentication.
# Allow to directly authenticate an user without LDAP Searches.
#
# e.g. CN=${username},OU=Users,OU=UserControl,OU=MyOrganization,DC=MyDomain

This suggests that there is no search for the user entry before the user is authenticated.
The pull request says:

Allow to use the LDAP AuthProvider with a LDAP Server
prohibiting anonymous searches but without providing
a manager password : searches are made on behalf of
the authenticated user.

This sounds different. This sounds as if the intent was to get rid of the manager bind in order to search for users. If the user can read users and groups, this will work. It will also work if the user can read only groups and sync is off. But is is different from the property description which only talks about authenticating without a search, not getting rid of the manager.

The problem with this pull request is, that the intended usage doesn't get clear, because the code works for neither of the interpretations. Gitblit will *always* try to bind to the manager account first. If that doesn't work, e.g. because no manager password is provided, authentication stops. So there is always the need for a manager password.
If the intent was to save a search for the user entryby directly binding the user to it, then that is not implemented either since the user entry is searched in any case, just under two different authentications.

Which leaves me a bit confused as to what the intention was behind this pull request and what problem it tried to fix.

To get that into context of the problem mentioned above: I would ask what the intent of the manager binding was originally. To me the code looks like it doesn't properly distinguish between bindings, takes over the user binding for group search by accident (because the team code was added later) and worked for most installations with lax access restrictions but not for some installations with stricter restrictions.

[Florian Zschocke]

What I am trying to get at, is, what are the use case scenarios and what is the intend behind the manager account (real.ldap.username) in the first place. Is Gitblit still behaving accordingly or has it been behaving incorrect for a long time now.

I would rather avoid another setting just to keep backward compatibility if no one is depending on that compatibility. Usually hard to know, I know. :-)

What is missing above is Scenario 4: the search user can see everything, the normal user cannot see users.

Scenario 4 seems more likely than Scenario 2.
Scenario 3 seems rather unlikely to me. Why would I have an extra account that can read users but not groups, while normal users can read groups? Is this really something that needs to be considered?

[Florian Zschocke]

These are the currently implemented code paths for authentication:
A:
bind anonymous or as manager
search for user
bind as user
search for teams

B:
bind anonymous or as manager
bind as user
search for user
search for teams

Both are problematic. A seems to use the user binding by accident to search for teams. B wants to get rid of the manager binding but doesn't succeed. I would consider the following two code paths as corrections:

C:
bind anonymous or as manager
search for user
bind as user to authenticate
bind anonymous or as manager
search for teams

D:
bind as user
search for user
search for team

Not at all considered is the following so far:

E:
bind as user
search for user
bind anonymous or as manager
search for team

The following is the currently implemented code path for LDAP synchronization:
F:
bind anonymous or as manager
search for users
search for teams


I would completely reconsider the LDAP implementation to fix the current problems. Looking at use cases and following constraints I find:

i) I want synchronisation:
    -> anonymous or a manager must be able to read users and groups
          => C, F

Any other use case excludes synchronisation.
ii) I want not anonymous and no manager binding
   -> the user logging in must be able to read users and groups
          => D

iii) I need to search for the logging in user's DN
    -> anonymous or a manager must be able to read users
        a) anonymous or the manager can read groups
            => C
        b) anonymous or manager cannot read groups but the logging in user can
           => A

iv) The logging in user can read users but not groups and
     anonymous or a manager can only read groups but not users
         => E


Case iv) seems not very likely and has not been supported so far, so I won't consider it any further.

Case i) and ii) as well as iii.a) have clear solutions.

Case iii.b) again seems rather unlikely to me. Does anyone have that setting where a specific "manager" account can read users but not groups? Maybe when using anonymous searches?
It does have the problem that it is current behaviour (even though I think by accident) and that maybe one wants to keep that behaviour.
I prefer not adding more properties and would hope that it could be solved without that. A suggestion would be to try C, and if the search for teams fails, try binding as the user again and searching for teams again. The only problem I could see is that different LDAP servers might answer with different result codes, so any result but OK would need to trigger falling back to searching teams as the user.

Other suggestions?


By the way, if bindpattern is set, and the intention is to get rid of anonymous or manager access, the synchronization should be disabled automatically. Because it will never work. Otherwise I don't know what the sense of having bindpattern set is.

Cheers,
Florian

[James Moger]

That's a really great writeup Florian and a perfect example of why I am careful to touch the LDAP provider since I don't have a ton of experience with using those services.

Would you be willing to write a replacement?  We could give it a new name (e.g. ldap2) so that we don't break existing users who are working or we could just gut it completely and carefully document the change.

-J

--
You received this message because you are subscribed to the Google Groups "gitblit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to gitblit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Florian Zschocke]

> Would you be willing to write a replacement?

Certainly. I need the fix myself, so I'll get to work on it.

I tried to find out from Jeremie what his intention was behind pull request #162 and if he wanted to get rid of the manager bind completely. I didn't really succeed. But since I didn't get it to work as described, and fail to see why there still is a initial manager bind in his current implementation, I will change the flow B into D. I'll just change the current implementation and document it. Maybe leaving an undocumented option to revert back as a quick fix in case some issue is raised due to the change within the next year or so.