LDAP: Authenticated Searches without a manager password #162

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@j3rem1e
Contributor

j3rem1e commented Mar 22, 2014

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.

LDAP: Authenticated Searches without a manager password
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.
@gitblit

This comment has been minimized.

Show comment
Hide comment
@gitblit

gitblit Mar 27, 2014

Owner

I have manually merged this. Thanks!

Owner

gitblit commented Mar 27, 2014

I have manually merged this. Thanks!

@fzs

This comment has been minimized.

Show comment
Hide comment
@fzs

fzs Nov 1, 2016

Collaborator

I need to ask: what problem does this change try to solve? Since Gitblit will always bind as the manager first, a manager password will always have to be provided. What is the intented benefit of searching for the user account as the user instead of the manager? That should render the manager account obsolete, but the commit doesn't actually do that.

Collaborator

fzs commented Nov 1, 2016

I need to ask: what problem does this change try to solve? Since Gitblit will always bind as the manager first, a manager password will always have to be provided. What is the intented benefit of searching for the user account as the user instead of the manager? That should render the manager account obsolete, but the commit doesn't actually do that.

@j3rem1e

This comment has been minimized.

Show comment
Hide comment
@j3rem1e

j3rem1e Nov 1, 2016

Contributor

When this patch was proposed, the manager password was not mandatory. you can provide an empty user/password. I don't know however if something has changed since v1.6

This patch allow to use an ldap server without the manager password, when the server doesn't allow anonymous search : it binds as the user. I use it every day with my gitblit instance with a corporate active directory.

Contributor

j3rem1e commented Nov 1, 2016

When this patch was proposed, the manager password was not mandatory. you can provide an empty user/password. I don't know however if something has changed since v1.6

This patch allow to use an ldap server without the manager password, when the server doesn't allow anonymous search : it binds as the user. I use it every day with my gitblit instance with a corporate active directory.

@fzs

This comment has been minimized.

Show comment
Hide comment
@fzs

fzs Nov 1, 2016

Collaborator

So you use this with an empty username and password or just an empty password? Does your manager account exist and have an empty password?

I haven't understood yet how this works. In your version, too, authenticate will first call getLdapConnection. Only if it gets back a connection will it run your new code. But getLdapConnection already binds either anonymously (empty username and empty password) or with the username. So if there is a username to bind against but no password, and the bind thus fails, authenticate will stop.

Collaborator

fzs commented Nov 1, 2016

So you use this with an empty username and password or just an empty password? Does your manager account exist and have an empty password?

I haven't understood yet how this works. In your version, too, authenticate will first call getLdapConnection. Only if it gets back a connection will it run your new code. But getLdapConnection already binds either anonymously (empty username and empty password) or with the username. So if there is a username to bind against but no password, and the bind thus fails, authenticate will stop.

@fzs

This comment has been minimized.

Show comment
Hide comment
@fzs

fzs Nov 3, 2016

Collaborator

Let me ask differently: Do you expect that, with this change, when the bindpattern property is not empty, the username and password properties are ignored, and Gitblit never binds as the manager but only as the user?

Collaborator

fzs commented Nov 3, 2016

Let me ask differently: Do you expect that, with this change, when the bindpattern property is not empty, the username and password properties are ignored, and Gitblit never binds as the manager but only as the user?

fzs added a commit to fzs/gitblit that referenced this pull request Nov 11, 2016

Clean up `LdapAuthProvider` to properly cover different LDAP search s…
…cenarios.

Gitblit allows in its configuration to set a "manager" user (and password) which can be used
to search for the entry of a user wanting to log in. If they are both not set, an anonymous search
is attempted. In the description below, when I say "...as manager", it is either as manager or
anonymous.
So far the behaviour of Gitblit, with respect to binding to and searching in LDAP,
has been the following when a user logs in:

**bind as manager**
**search for the user**
_bind as the user_
_search for the teams_

I'll call this code flow A.

Later an additional configuration option had been added: `realm.ldap.bindpattern`.
(PR gitblit/gitblit#162) It was meant to allow for not using a manager nor anonymous binds,
by searching the directory as the user logging in.
This is done in code flow B:

**bind as manager**
_bind as user_
_search for user_
_search for teams_

Both A and B are flawed, I think. In A, it looks like a mistake to me that the binding stays with the
user after authentication. The problem that this causes is, that in LDAP server configurations
where normal users are not allowed to read groups, the team information cannot be retrieved.
I tried but failed to understand how B is supposed to work. There will always be a bind request
as either anonymous or the manager DN when the LDAP connection is created. If neither is
possible, the authentication process will fail and the user cannot log in.

When synchronizing users and teams from LDAP, the following code flow is exercised:

F:
**bind as manager**
**search for users**
**search for teams**

This patch fixes both code flows by introducing a new flow.

C:
**bind as manager**
**search for user**
_bind as user to authenticate_
**bind as manager**
**search for teams**

And it changes code flow B to the following code flow D:

_bind as user_
_search for user_
_search for teams_

With code flows A, C, D and F the following usage (and authentication) scenarios are covered.
They are described from the view of a Gitblit administrator's intent and his LDAP setup.

* Users and team should be snychronized with LDAP
	This means anonymous or a fixed account must be able to read users and groups.
	=> covered by C and F

As the above allows for authentication and is required for synchronisation, all the others below
do not cover synchronization.

* No anonymous binding allowed and no special manager binding required
	This means that users must be able to read user an group entries.
	=> covered by D

* The user DN needs to be searched, e.g. because they are not all under the same parent DN.
	This means that anonymous or a fixed account must be able to read users.
		-- anonymous or the "manager" account can also read groups
			=> covered by C
		-- anonymous or the "manager" account cannot read groups but a user can
			=> covered by A

I therefore believe that the new code will cover all common use cases. The implementation
either directly binds as the user, when `bindpattern` is not empty, or it binds anonymous or
against the manger DN to search for the user DN entry.

If it directly bound against the user DN, the user is already authenticated. It will then only check
that the user DN it found in the search is identical to the one it is currently bound against. If it
was bound against a manager DN (or anonymously) it will bind against the found user DN to
authenticate the user logging in, and will then rebind against the manager DN.

When searching for groups in LDAP, if the search fails with a result code other than SUCCESS,
the implementation will bind against the user DN, if it isn't already bound against it. It will then
repeat the search for groups under the user authorization. This is to keep backwards
compatible with the original behaviour A, in order to not break cases where the LDAP setup
would deny a manager account to search for groups but allow it for normal users.

To achieve this the implementation introduces an internal `LdapConnection` class that wraps
the connection and keeps bind state, so that a rebind as a user is possible.
This also fixes a resource leak where the connection was not closed in case that the initial bind
as the manager account did not succeed.

This commit would fix gitblit/gitblit#920
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment