jtblin / go-ldap-client

Simple ldap client to authenticate, retrieve basic information and groups for a user.
Other
261 stars 90 forks source link

Connection Timeouts #1

Closed warmans closed 8 years ago

warmans commented 8 years ago

There would seem to be an issue with this library where depending on the LDAP server configuration the connection can time out. In this case the client will never manage to re-establish the connection and fail with the following error for all subsequent attempts to Authenticate a user.

LDAP Result Code 200 "": ldap: connection closed

For my purposes I was able to fix this by just forcing the connection to be re-established every time connect was called:

https://github.com/warmans/go-ldap-client/commit/6812274f799ed595e50829cf3032948946aab01d

Though this may be undesirable if you're doing something weird like calling GetGroupsOfUser for every web request or something.

jtblin commented 8 years ago

Thanks for the report. Were you closing the connection in case of error as per the readme?

# It is the responsibility of the caller to close the connection
defer client.Close()

I would have assumed that this would have been sufficient in case of a timeout and trying again later on or am I missing something else? Maybe could you paste an example of your code?

warmans commented 8 years ago

In my case there is a http.ListenAndServe(...) at the bottom of main so the connection is only explicitly closed when the programme exists. It's not clear to me from the readme that you should always close the connection on error, rather just that you should close it manually at some point.

I'm not 100% sure this would fix the issue though since in Connect it checks Conn is nil before re-connecting and think Close won't make this true.

jtblin commented 8 years ago

You're right about the nil stuff, I'll fix that. Thanks :+1:

jtblin commented 8 years ago

Sorry for the lag. I've fixed the nil issue.

Even if you only have one handler, you should be able to have a defer client.Close() in it no?

http.HandleFunc("/", func (w http.ResponseWriter, r *http.Request) {
   ok, user, err := client.Authenticate("username", "password")
    client := &ldap.LDAPClient{...}
    defer client.Close()
    ...
})
http.ListenAndServe(":8080", nil)
warmans commented 8 years ago

To be honest this is most probably a problem with our LDAP host rather an issue with the library. The work around is just to re/connect in the auth handler as you suggest but I think most people wouldn't need to worry about this.