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

clarify ldap schema calls #655

Closed
cornelinux opened this issue Mar 16, 2017 · 3 comments
Closed

clarify ldap schema calls #655

cornelinux opened this issue Mar 16, 2017 · 3 comments
Labels
Topic: LDAP LDAP resolver and LDAP related issues
Milestone

Comments

@cornelinux
Copy link
Member

I also find that the schema queries are still remaining in most operations...
Would it be sensible in your opinion to have the ldap3.NONE as the default behavior for get_serverpool method instead? @quynh-axiadids (taken from #650)

ldap3 version 2.2.1

@cornelinux cornelinux added this to the 2.19 milestone Mar 16, 2017
@cornelinux cornelinux added the Topic: LDAP LDAP resolver and LDAP related issues label Mar 16, 2017
@fredreichbier
Copy link
Contributor

fredreichbier commented Mar 17, 2017

I did some tests on the current master (070c4b3) with otppin=userstore and the /validate/check request, using the Flask integrated webserver. It seems to me that the schema queries are issued in refresh_server_info, so we should minimize the number of calls to that method.

  • If we do not use STARTTLS, it seems like refresh_server_info is called 4 times in the first request, but only once in every subsequent request (EDIT: during the timeframe specified by CACHE_TIMEOUT, as it turns out)
  • If we use STARTTLS, the method is called 12 times in the first request and 3 times in subsequent requests. In my understanding, we could pass read_server_info=False to both open() and start_tls() in create_connection to optimize this. Then, the method is called 4 times in the first request, but only once in subsequent requests (EDIT: during the CACHE_TIMEOUT timeframe). The ldap3 source code references Section 3.1.5 of RFC 4513, which states that clients should refresh the server capabilities after having established STARTTLS to mitigate MITM attacks. However, as the refresh_server_info call is performed in the bind (which happens with STARTTLS enabled), I think we should be good.

What do you think?

@quynh-axiadids
Copy link
Contributor

quynh-axiadids commented Mar 17, 2017 via email

@fredreichbier
Copy link
Contributor

I think we have multiple questions here:

  • The number of calls to refresh_server_info influences how often the server information is fetched. If we are not using STARTTLS, this is once per connection (and we have four connections per authentication request (see Reduce number of LDAP connections per authentication request #664) if uncached, and only one if the user information are found in the cache). If we are using STARTTLS, this is three times per connection (open, start_tls, bind).
  • The get_info option influences what is queried when the server information is fetched. If we set this to ldap3.NONE, refresh_server_info does not query schema information at all.

So, to reduce the number of schema queries, we should reduce the number of LDAP connections, for which I opened #664. For use cases that need schema information, I guess it makes sense to optimize the number of calls to refresh_server_info per connection in the case of STARTTLS. Finally, for the case that schema information are not needed at all, it may also be useful to make the value of get_info configurable. @cornelinux, what do you think?

fredreichbier pushed a commit to fredreichbier/privacyidea that referenced this issue Mar 23, 2017
Previously, `refresh_server_info` was called for `open()`,
`start_tls()` and `bind()`. With this patch, it is only called
for `bind()`. See privacyidea#655.
cornelinux pushed a commit that referenced this issue Mar 27, 2017
Previously, `refresh_server_info` was called for `open()`,
`start_tls()` and `bind()`. With this patch, it is only called
for `bind()`. See #655.
fredreichbier pushed a commit that referenced this issue Mar 29, 2017
This can be used to tune the performance of LDAP connections.
This commit adds a new NOSCHEMAS configuration option, but
does not add it to the web interface (yet).
See #655.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Topic: LDAP LDAP resolver and LDAP related issues
Projects
None yet
Development

No branches or pull requests

3 participants