reuse ldap connection #77
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,7 @@ import ( | |
| "net/url" | ||
| "strconv" | ||
| "strings" | ||
| "sync" | ||
| "time" | ||
|
|
||
| "github.com/go-ldap/ldap/v3" | ||
|
|
@@ -71,6 +72,9 @@ type LDAPIdentifierBackend struct { | |
|
|
||
| timeout int | ||
| limiter *rate.Limiter | ||
|
|
||
| mutex sync.Mutex | ||
| conn *ldap.Conn | ||
| } | ||
|
|
||
| type ldapAttributeMapping map[string]string | ||
|
|
@@ -115,7 +119,7 @@ func newLdapUser(entryID string, mapping ldapAttributeMapping, entry *ldap.Entry | |
| for n, mapped := range mapping { | ||
| // LDAP attribute descriptors / short names are case insensitive. See | ||
| // https://tools.ietf.org/html/rfc4512#page-4. | ||
| if strings.EqualFold(attribute.Name, mapped) { | ||
| // Check if we need conversion. | ||
| switch mapping[fmt.Sprintf("%s_type", n)] { | ||
| case AttributeValueTypeBinary: | ||
|
|
@@ -370,9 +374,8 @@ func (b *LDAPIdentifierBackend) Logon(ctx context.Context, audience, username, p | |
|
|
||
| l, err := b.connect(ctx) | ||
| if err != nil { | ||
| return false, nil, nil, nil, fmt.Errorf("ldap identifier backend logon global connect error: %v", err) | ||
| } | ||
|
|
||
| // Search for the given username. | ||
| entry, err := b.searchUsername(l, username, b.attributeMapping.attributes()) | ||
|
|
@@ -387,8 +390,15 @@ func (b *LDAPIdentifierBackend) Logon(ctx context.Context, audience, username, p | |
| return false, nil, nil, nil, fmt.Errorf("ldap identifier backend logon search returned wrong user") | ||
| } | ||
|
|
||
| // Use a temporary connection for the user Bind to verify the password. | ||
| ul, err := b.createConnection(ctx, entry.DN, password) | ||
| if err != nil { | ||
| return false, nil, nil, nil, fmt.Errorf("ldap identifier backend logon user connect error: %v", err) | ||
| } | ||
| // Always close the temporary connection | ||
| defer ul.Close() | ||
|
|
||
| err = ul.Bind(entry.DN, password) | ||
| switch { | ||
| case ldap.IsErrorWithCode(err, ldap.LDAPResultInvalidCredentials): | ||
| return false, nil, nil, nil, nil | ||
|
|
@@ -431,7 +441,6 @@ func (b *LDAPIdentifierBackend) ResolveUserByUsername(ctx context.Context, usern | |
| if err != nil { | ||
| return nil, fmt.Errorf("ldap identifier backend resolve connect error: %v", err) | ||
| } | ||
|
|
||
| // Search for the given username. | ||
| entry, err := b.searchUsername(l, username, b.attributeMapping.attributes()) | ||
|
|
@@ -464,7 +473,6 @@ func (b *LDAPIdentifierBackend) GetUser(ctx context.Context, entryID string, ses | |
| if err != nil { | ||
| return nil, fmt.Errorf("ldap identifier backend get user connect error: %v", err) | ||
| } | ||
|
|
||
| entry, err := b.getUser(l, entryID, b.attributeMapping.attributes()) | ||
| if err != nil { | ||
|
|
@@ -517,7 +525,7 @@ func (b *LDAPIdentifierBackend) Name() string { | |
| return ldapIdentifierBackendName | ||
| } | ||
|
|
||
| func (b *LDAPIdentifierBackend) createConnection(parentCtx context.Context, bindDN, bindPassword string) (*ldap.Conn, error) { | ||
| // A timeout for waiting for a limiter slot. The timeout also includes the | ||
| // time to connect to the LDAP server which as a consequence means that both | ||
| // getting a free slot and establishing the connection are one timeout. | ||
|
|
@@ -550,9 +558,8 @@ func (b *LDAPIdentifierBackend) connect(parentCtx context.Context) (*ldap.Conn, | |
|
|
||
| l.Start() | ||
|
|
||
| if bindDN != "" { | ||
| err = l.Bind(bindDN, bindPassword) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
@@ -561,6 +568,20 @@ func (b *LDAPIdentifierBackend) connect(parentCtx context.Context) (*ldap.Conn, | |
| return l, nil | ||
| } | ||
|
|
||
| // connect establishes a global connection that is used for all requests except the user Bind to check the password in the Logon call. | ||
longsleep marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| func (b *LDAPIdentifierBackend) connect(parentCtx context.Context) (*ldap.Conn, error) { | ||
| b.mutex.Lock() | ||
| defer b.mutex.Unlock() | ||
|
|
||
| if b.conn != nil && !b.conn.IsClosing() { | ||
|
Collaborator
There was a problem hiding this comment. This does not really know if the connection actually still works and might result in error and thus logon failure when getting used. Does the ldap connection do anything (like in the background) to ensure that does not happen? Normally that would be the job of the pool before giving the connection to the caller.
Author
There was a problem hiding this comment. Hm, the connection closes itself only when it cannot read a response. We could port the reconnect wrapper from https://github.com/cs3org/reva/blob/edge/pkg/utils/ldap/reconnect.go
Collaborator
There was a problem hiding this comment. I think this needs to be solved before this can be merged. One way or another the connection needs to be "checked" if it is still usable (or some retry logic). |
||
| return b.conn, nil | ||
| } | ||
| var err error | ||
| b.conn, err = b.createConnection(parentCtx, b.bindDN, b.bindPassword) | ||
|
|
||
| return b.conn, err | ||
| } | ||
|
|
||
| func (b *LDAPIdentifierBackend) searchUsername(l *ldap.Conn, username string, attributes []string) (*ldap.Entry, error) { | ||
| base, filter := b.baseAndSearchFilterFromUsername(username) | ||
| // Search for the given username. | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.