Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LDAP login fails, resulting in incorrect user permissions until the next sync #920

Closed
rconaway opened this issue Sep 18, 2015 · 14 comments · Fixed by #1149
Closed

LDAP login fails, resulting in incorrect user permissions until the next sync #920

rconaway opened this issue Sep 18, 2015 · 14 comments · Fixed by #1149

Comments

@rconaway
Copy link

rconaway commented Sep 18, 2015

We are using LDAP for authentication. When a user logs in, an error is logged and the user seems to be logged in correctly, but his permissions are not correct until the next LDAP sync.

We've debugged the code to line 523 in class LDAPAuthProvider. When we take the LDAP arguments (base, filter, attributes) from that point and manually make the LDAP call using Softerra LDAP browser, the manual call works fine.

The production instance in Linux, but I can reproduce on a machine running Windows 7, GitBlit go version 1.6.2.

Our next step will be to try to build from source and reproduce. We'd appreciate any suggestions about what might be wrong or how best to pinpoint the problem.

Rob Conaway


The following is the (redacted) log message:

2015-09-18 09:26:09 [INFO ] Running on Windows 7 (6.1)
2015-09-18 09:26:09 [INFO ] Logging initialized @461ms
2015-09-18 09:26:09 [INFO ] Using JCE Standard Encryption Policy files, encryption key lengths will be limited
2015-09-18 09:26:09 [INFO ] Setting up HTTPS transport on port 443
2015-09-18 09:26:09 [INFO ]    certificate alias = localhost
2015-09-18 09:26:09 [INFO ]    keyStorePath   = C:\REDACTED\gitblit-1.6.2\data\serverKeyStore.jks
2015-09-18 09:26:09 [INFO ]    trustStorePath = C:\REDACTED\gitblit-1.6.2\data\serverTrustStore.jks
2015-09-18 09:26:09 [INFO ]    crlPath        = C:\REDACTED\gitblit-1.6.2\data\certs\caRevocationList.crl
2015-09-18 09:26:09 [INFO ] Setting up HTTP transport on port 80
2015-09-18 09:26:09 [INFO ] Shutdown Monitor listening on port 8081
2015-09-18 09:26:09 [INFO ] jetty-9.2.3.v20140905
2015-09-18 09:26:12 [INFO ] NO JSP Support for /, did not find org.apache.jasper.servlet.JspServlet
2015-09-18 09:26:12 [INFO ] 
  . . .
2015-09-18 09:26:23 [ERROR] Problem Searching LDAP
LDAPSearchException(resultCode=32 (no such object), numEntries=0, numReferences=0, errorMessage='0000208D: NameErr: DSID-0315258B, problem 2001 (NO_OBJECT), data 0, best match of:
  'O=REDACTED,C=US'
', matchedDN='O=REDACTED,C=US')
  at com.unboundid.ldap.sdk.LDAPConnection.search(LDAPConnection.java:3091)
  at com.gitblit.auth.LdapAuthProvider.doSearch(LdapAuthProvider.java:523)
  at com.gitblit.auth.LdapAuthProvider.getTeamsFromLdap(LdapAuthProvider.java:456)
  at com.gitblit.auth.LdapAuthProvider.authenticate(LdapAuthProvider.java:340)
  at com.gitblit.manager.AuthenticationManager.authenticate(AuthenticationManager.java:380)
  at com.gitblit.wicket.pages.RootPage$LoginForm$1.onSubmit(RootPage.java:567)
  at org.apache.wicket.markup.html.form.Form.delegateSubmit(Form.java:1595)
  at org.apache.wicket.markup.html.form.Form.process(Form.java:960)
  at org.apache.wicket.markup.html.form.Form.onFormSubmitted(Form.java:922)
  at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
  at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
  at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
  at java.lang.reflect.Method.invoke(Unknown Source)
  at org.apache.wicket.RequestListenerInterface.invoke(RequestListenerInterface.java:182)
  . . .

Here is the LDAP portion of the gitblit.properties

realm.authenticationProviders = ldap
realm.ldap.server = ldaps://REDACTED.com:636
realm.ldap.username = REDACTED=2031441, ou=Employee, ou=People, o=REDACTED, c=US
realm.ldap.password = gv3$0819
realm.ldap.maintainTeams = true
realm.ldap.accountBase = ou=Employee, ou=People, o=REDACTED, c=US
realm.ldap.accountPattern = (&(objectClass=person)(uid=rconawa2)(|(memberOf=CN=REDACTED-DEVELOPERS,OU=Groups,O=REDACTED,C=US)(memberOf=CN=REDACTED-DEVELOPERS,OU=Groups,O=REDACTED,C=US)))
realm.ldap.groupBase = OU=Groups,O=REDACTED,C=US
realm.ldap.groupMemberPattern = (&(objectClass=group)(member=REDACTEDGID=1988870,OU=Employee,OU=People,O=REDACTED,C=US))
realm.ldap.groupEmptyMemberPattern = (&(objectClass=group)(!(member=*)))
realm.ldap.admins = (CN=REDACTED-GIT-ADMINISTRATORS,OU=Groups,O=REDACTED,C=US)
realm.ldap.displayName = REDACTEDDisplayName 
realm.ldap.email = mail
realm.ldap.synchronize = true
realm.ldap.syncPeriod = 5 MINUTES
realm.ldap.removeDeletedUsers = true
@gitblit
Copy link
Owner

gitblit commented Sep 18, 2015

Hi Rob,

Thanks for the report. If you can build from source then I would say that directly debugging the authentication code is the way to go.

Another perhaps useful option would be to enable UnboundID (LDAP library) logging. If you startup Gitblit with some -D VM args then you should be able to enable this:

-Dcom.unboundid.ldap.sdk.debug.enabled=true
-Dcom.unboundid.ldap.sdk.debug.level=INFO

You'll have to play with the log levels.
"ALL", "SEVERE", "WARNING", "INFO", "CONFIG", "FINE", "FINER", "FINEST", or "OFF".

@rconaway
Copy link
Author

rconaway commented Sep 23, 2015

After several days of off and on debugging, we've discovered that we can
"fix" the problem by using fresh LDAP connections when searching for the
teams during authentication. Specifically, we changed
LdapAuthProvider.java as follows:

private void getTeamsFromLdap(LDAPConnection ldapConnection, String
simpleUsername, SearchResultEntry loggingInUser, UserModel user) {
ldapConnection = getLdapConnection();
if (ldapConnection != null) {
try {
String loggingInUserDN = loggingInUser.getDN();
...
}
} finally {
ldapConnection.close();
}
}
}

Before this change, sync() worked fine, but authenticate() failed. After
the change, they both seem to work fine.

Obviously, there is something going on with the state of the LDAP
connection that I cannot see.

Any insights? Is there still some way to work around this without a code
change?

BTW, we've beat our heads on this problem for a while because GitBlit
really solves a problem for us and we'd love to make it work. Thanks for
the time you've put into it.

Rob

On Fri, Sep 18, 2015, 12:54 PM James Moger notifications@github.com wrote:

Hi Rob,

Thanks for the report. If you can build from source then I would say that
directly debugging the authentication code is the way to go.

Another perhaps useful option would be to enable UnboundID (LDAP library)
logging. If you startup Gitblit with some -D VM args then you should be
able to enable this:

-Dcom.unboundid.ldap.sdk.debug.enabled=true
-Dcom.unboundid.ldap.sdk.debug.level=INFO

You'll have to play with the log levels.
"ALL", "SEVERE", "WARNING", "INFO", "CONFIG", "FINE", "FINER", "FINEST",
or "OFF".


Reply to this email directly or view it on GitHub
#920 (comment).

@gitblit
Copy link
Owner

gitblit commented Sep 25, 2015

Sorry Rob, I'm not an LDAP expert. It does seem suspicious that reusing the connection fails. When you get a fresh connection, the connection is either bound to fixed credentials specified in the config or is not bound at all - depends on your config. When you re-use the connection from authenticate, the connection is bound to the logging-in user, I think. I'm sure you already know that but I'm wondering if that detail is significant here.

@rconaway
Copy link
Author

rconaway commented Sep 25, 2015

Your comment about the rebinding was very helpful. There is a powerful
account initially connecting and doing the authenticate, but the logging-in
user does not have the permissions to list his teams. It seems like this
would be a typical situation, but maybe we're using LDAP differently.

At any rate, I don't see any way to work around without a code change.
We're prepared to make a change in our installation if absolutely
necessary, but are you open to a change in the code base?

I see two ways of approaching it:

  1. Save and revert to the original binding at the end of isAuthenticate.
    This seems to be the "correct" behavior, but could break anyone depending
    on the previous behavior.

  2. Do the change, but make it conditional on a property that defaults to
    false. This would maintain backward compatibility.

If you agree, is this something I can change then make a pull request?

Rob

On Thu, Sep 24, 2015 at 10:08 PM, James Moger notifications@github.com
wrote:

Sorry Rob, I'm not an LDAP expert. It does seem suspicious that reusing
the connection fails. When you get a fresh connection, the connection is
either bound to fixed credentials specified in the config or is not bound
at all - depends on your config. When you re-use the connection from
authenticate, the connection is bound to the logging-in user, I think. I'm
sure you already know that but I'm wondering if that detail is significant
here.


Reply to this email directly or view it on GitHub
#920 (comment).

@paulsputer
Copy link
Collaborator

paulsputer commented Sep 25, 2015

Hi Rob, I'm not that familiar with LDAP but have successfully configured GitBlit via an LDAP service with 20k+ users. Whilst setting that up I found the Active Directory Explorer tool very useful for debugging (https://technet.microsoft.com/en-us/library/bb963907.aspx).

A few things I notice with your config though I'm not sure how much is due to different LDAP structures:

  • Your username config defines an ad path, I didn't have any success with this style of config but did have when I define it as ad\\{service-user-account-name}
  • Your accountBase config is specifying down to the OU level, I ran into difficulties when doing this so just specified to DC level. Apparently this may slow searches but I haven't noticed any difference.
  • Your accountPattern contains an actual username, I use %{username} also I did have fun identifying the id to use, I found that uid wasn't always correct, so use sAMAccountName instead.
  • Your groupBase and accountBase paths differ, I found I had to have the higher level parts of the path matching.
  • Your groupMemberPattern contains an actual username for member, I had to use ${dn}
  • Also, you may need to check what your realm.ldap.uid is set to and if it matches above

As I don't have admin access to LDAP to manage teams I simply use OU's which thankfully are granular enough for my requirements. It does sound a bit odd for a user not to be able to list the teams it is a member of, could it be perhaps the user doesn't have access to the particular AD path defined such as groupBase. If you log in using the user (not service) credentials to AD explorer maybe you will find a different path to use which works for all accounts.

HTH

Paul

@RainerW
Copy link
Contributor

RainerW commented Oct 2, 2015

Hi, i think this is the same issue as #247

@gitblit
Copy link
Owner

gitblit commented Oct 2, 2015

@RainerW Yes, I think it is the same (or at least very similar) issue.

@rconaway Did Paul's comments move you further along?

@rconaway
Copy link
Author

rconaway commented Nov 20, 2015

There was a pull request for #247 that would have solved this problem. The pull request was refused then #247 was closed. Is there anything else going on? Should I make a pull request?

Rob

@gitblit
Copy link
Owner

gitblit commented Nov 20, 2015

Hi Rob, a PR would be nice. No one is actively working on this issue.

@mstaffeld
Copy link

mstaffeld commented Jan 12, 2016

@rconaway 👍

We are experiencing the issue at a large enterprise client where the LDAP user does not have the permission to view LDAP groups. Go figure. Yes I am serious...

The LDAP connection would need to be an elevated user that can query groups.

@kellerkindt
Copy link

kellerkindt commented Sep 30, 2016

We are also experiencing the same issue. The normal LDAP users are not allowed to list all LDAP groups while the user from the configuration file is allowed to do so.

Any chance for a fix? Building gitblit on our own just for that does not seem to be a good solution at all.

@gitblit Would you accept a new pull request of #247 for the develop branch?

@gitblit
Copy link
Owner

gitblit commented Oct 3, 2016

@kellerkindt My recollection is the proposed fix solved one problem while creating a new one. The new problem was either backwards-incompatibility for config or unexpected behavior changes or both - can't recall. If you can propose a fix that doesn't break others then I'm all for it.

@flaix
Copy link
Collaborator

flaix commented Nov 1, 2016

I, too, have that same problem now. From what I see this has come up a few times now.
I would consider the current behavior as wrong and thus wouldn't object to a change.
Since there is activity on this issue but not so much on the thread in the discussion group, I'm providing a link there so that the conversation can be merged.

flaix pushed a commit to flaix/gitblit that referenced this issue Nov 11, 2016
…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#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#920
@flaix
Copy link
Collaborator

flaix commented Nov 12, 2016

Hi everyone!
I have implemented a fix for this issue by changing the bind behaviour of LdapAuthProvider in commit a4ad77f. It keeps backward compatibility to the original behaviour of searching for teams under the user authorization. I'll create a pull request from it later, after adjusting documentation, too. But I wanted to offer it here for review and comment first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants