Bug 66355 - Wrong unit for LDAP/LDAPRetryDelay
Summary: Wrong unit for LDAP/LDAPRetryDelay
Status: RESOLVED INVALID
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: mod_auth_ldap (show other bugs)
Version: 2.4.54
Hardware: PC All
: P2 normal (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-11-18 10:20 UTC by Stephen Blott
Modified: 2022-11-19 16:32 UTC (History)
1 user (show)



Attachments
Just the bug notes. (1.67 KB, text/plain)
2022-11-18 10:22 UTC, Stephen Blott
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen Blott 2022-11-18 10:20:24 UTC
The documentation suggests `LDAPRetryDelay` is in seconds.

The code appears to treat it as microseconds.

The effect is that Apache sometimes issues rapid bursts of almost simultaneous LDAP search/bind requests.

The bug fix is simply to replace

   apr_sleep(st->retry_delay);

with

   apr_sleep(apr_time_from_sec(st->retry_delay));

(httpd/modules/ldap/util_ldap.c:668).

Here's my research:

Issue:
  LDAP configuration option LDAPRetryDelay...

  The Documentation suggests that the unit is seconds:
    https://httpd.apache.org/docs/2.4/mod/mod_ldap.html#ldapretrydelay

  The code suggests that the unit is microseconds (see code chase, below).

Effect:
  Apache sometimes issues a burst of almost simultaneous LDAP requests.

  (In my organisation, this is "catastrophic" since, if the password is
   incorrect, it appears as N failed login attempts, and the account is
   instantly blocked (after just a single attempt). In practice,
   I've observed N in the region of 5 to 7.)

Configuration option:

  LDAPRetryDelay 5   (for example)

  This sets the retry delay for LDAP connections.

  In the code, this ends up here...

In util_ldap_set_retry_delay (util_ldap.c:2859):

  st->retry_delay = timeout;

  Note... no unit conversion takes place; the code just checks that it's
  a non-negative integer and notes the value for later.

The delay is implemented in httpd/modules/ldap/util_ldap.c:668:

  apr_sleep(st->retry_delay);

  Note... we still appear to have the raw value from the configuration
  file (nominally in seconds).

If you search the code, you will find that apr_sleep is ALMOST ALWAYS
called like this:

  apr_sleep(apr_time_from_sec(XXXX))

  That is, the unit expected is whatever is returned by apr_time_from_sec().

In APR, apr_time_from_sec() is defined like this (apr/include/apr_time.h):

  /** number of microseconds per second */
  #define APR_USEC_PER_SEC APR_TIME_C(1000000)

  .
  .
  .

  /** @return seconds as an apr_time_t */
  #define apr_time_from_sec(sec) ((apr_time_t)(sec) * APR_USEC_PER_SEC)

  So, the result of apr_time_from_sec is in microseconds.

  So the call in util_ldap.c needs to be fixed (as stated at the top).
Comment 1 Stephen Blott 2022-11-18 10:22:30 UTC
Created attachment 38436 [details]
Just the bug notes.

This is just the same text as before.

Bugzilla makes me provide an attachment.
Comment 2 Ruediger Pluem 2022-11-18 12:26:54 UTC
(In reply to Stephen Blott from comment #0)

>    I've observed N in the region of 5 to 7.)
> 
> Configuration option:
> 
>   LDAPRetryDelay 5   (for example)
> 
>   This sets the retry delay for LDAP connections.
> 
>   In the code, this ends up here...
> 
> In util_ldap_set_retry_delay (util_ldap.c:2859):

This line number does not match the 2.4.54 code.

> 
>   st->retry_delay = timeout;

But timeout is not the value given by LDAPRetryDelay but the value created by
ap_timeout_parameter_parse (util_ldap.c:2786) from the value set by LDAPRetryDelay 
 which already does the correct scaling (seconds by default)

http://svn.apache.org/viewvc/httpd/httpd/tags/2.4.54/modules/ldap/util_ldap.c?revision=1901749&view=markup#l2786