Re: svn commit: r1914067 - in /httpd/httpd/trunk: changes-entries/ldap-optimise.txt modules/aaa/mod_authnz_ldap.c
On 23 Nov 2023, at 13:28, Yann Ylavic wrote: >> Unfortunately the message is correct and we do just that via >> get_dn_for_nonldap_authn (line 1451). The DN you mention we look for >> later in line 1480 via util_ldap_cache_getuserdn. > > My guess was that if r->user is set we always want to use it for ldap > requests/authn? Or maybe there should be another Require ldap-!search > configured to achieve that (if wanted) since ldap-search is special.. We already have that, it’s ldap-filter. In more detail, ldap-filter takes an LDAP filter, for example (objectclass=mailRecipient), and then it takes an r->user, for example f...@example.com, and then it combines them to create a new filter (&(uid=f...@example.com)(objectclass=mailRecipient)), and if exactly one (not zero, not two) objects exist, it is a match. ldap-search ignores the r->user special case and broadens this to any expression you can image. >>> On a side note, ldap-search requires that exactly one LDAP object matches, >>> we might want to support many matches, or a specific >>> number of matches. > > I still don't get where "Make sure that the filtered search returned a > single dn" is enforced, is it because result != LDAP_SUCCESS > otherwise, or dn == NULL if there are multiple objects/DNs? It’s enforced here: https://github.com/apache/httpd/blob/trunk/modules/ldap/util_ldap.c#L2118 For historic reasons, the function is called uldap_cache_getuserdn(), but what the function actually does is run a single LDAP query, and expect one and only one object in return. ldap-search runs a single LDAP query, and expects one and only one object in return, it just isn’t a user. Regards, Graham —
Re: svn commit: r1589993 - in /httpd/httpd/trunk: CHANGES docs/manual/expr.xml docs/manual/mod/mod_authnz_ldap.xml modules/aaa/mod_authnz_ldap.c
Le 18/11/2023 à 20:52, Yann Ylavic a écrit : On Wed, Apr 30, 2014 at 1:02 AM Yann Ylavic wrote: On Tue, Apr 29, 2014 at 10:54 PM, Christophe JAILLET wrote: Hi, doc does not build because of below: CJ Le 25/04/2014 13:14, minf...@apache.org a écrit : + +LocationMatch ^/dav/(?[^/]+)/ ^ There Hmm, won't LocationMatch itself be broken by the inner <>s ? Wow, fortunately I didn't hold my breath on this one :) Someone needs to answer to this former/younger/naive me though and since I'm on this commit again: look Yann, this match is double-quoted now so we should be fine! In fact, at that time, another solution was provided in r1591113. But what you propose above, should have worked as well, I guess. :). CJ
Re: mod_dav_fs locking / Re: apr_dbm and concurrency
On Thu, Nov 23, 2023 at 05:36:06PM +, Joe Orton wrote: > 3) in the mean time I worked up a PR for mod_dav_fs which adds a global > mutex around the dbm lockdb use. This passes my stress tests for the > first time. How concurent is the stress test? In the past, I have been badly hurt by a few WebDAV clients proactively exploring the filesystem using locksdiscovery. That compeltely killed the service. I introduced the DavLockDiscovery directive to work it around. -- Emmanuel Dreyfus m...@netbsd.org
mod_dav_fs locking / Re: apr_dbm and concurrency
Adding dev@httpd to a dev@apr thread about apr_dbm locking being broken. On Sun, Nov 12, 2023 at 07:43:13AM -0600, Greg Stein wrote: > Or, apps can stick to an older APR. ... we don't have to carry this forward > into future versions of APR (IMO). > > To your point, it is probably a single page with code samples to show how > to do K/V with sqlite, to replace the apr_dbm calls. That's gonna be way > easier than locking code integrated into apr_dbm. This seems reasonable to me for the mod_dav use case where the database is an implementation detail which users don't care about. For other use cases in httpd I'm not so sure, but saying "dbm is dead, sqlite is the way" is probably possible for all of them. It does mean someone writing a lot of new modules to replace mod_auth*dbm etc tho ;) There are a few alternative approaches I looked at: 1) we could hack fcntl-based locks to work on Linux by using F_OFD_SETLK etc which a sane locking model rather than the weird/dumb F_SETLK which has the traditional/POSIX non-thread-safe model. Linux-specific, so... 2) try to shoehorn "proper" locking into apr_dbm is hard because there isn't a suitable locking primitive that can be used at this level. Maybe the only approach that might work would be filesystem-based locking based off open/O_EXCL but this is not exactly reliable. 3) in the mean time I worked up a PR for mod_dav_fs which adds a global mutex around the dbm lockdb use. This passes my stress tests for the first time. Kind of ugly but this seems like the least ugly option at this point other than the "start again with sqlite": https://github.com/apache/httpd/pull/395 Any comments/review/flames from either dev@ welcome. Regards, Joe
Re: svn commit: r1912245 - in /httpd/httpd/trunk/modules/proxy: balancers/mod_lbmethod_bybusyness.c mod_proxy_balancer.c proxy_util.c proxy_util.h
On 11/23/23 5:05 PM, Yann Ylavic wrote:
> On Thu, Nov 23, 2023 at 4:46 PM Yann Ylavic wrote:
>>
>> On Thu, Nov 23, 2023 at 4:17 PM Ruediger Pluem wrote:
>>>
>>> On 11/23/23 3:53 PM, Yann Ylavic wrote:
>>> I am asking because I guess I am hit now by races in the byrequests LB
>>> with the worker->s->lbstatus.
>>> I probably want to look for a way to solve this in a more generic way
>>> for various shared worker fields.
>>
>> lbstatus is an int so the 32bit apr_atomic API could do I think.
>
> I mean, if lbstatus becomes inconsistent because of the race then we
My current theory is that this is the case.
> can do something, but if it is e.g. that a worker switches from error
> state to non-error become some threads can connect while some others
> can't this is something we should address elsewhere (like a failure
> threshold).
>
>> Maybe we need some ap_atomic_{int,long,size_t,..}_*() helpers for
>> system dependent widths. It preferably would be implemented in APR but
>> in the meantime we could have something in httpd already.
>> What we should avoid IMO is needing 64bit atomics on 32bit systems
>> (because we'd have to reimplement the mutexes etc), but apart from
>> that I think we can wrap anything using the existing apr atomics.
>
> What we need for lbstatus is ap_atomic_int_or() and
> ap_atomic_int_and_not() it seems, both could be implemented with a
> cas32 loop.
What functionality are ap_atomic_int_or() / ap_atomic_int_and_not() are
supposed to deliver?
I am having a hard time guessing it from the name :-).
Regards
Rüdiger
Re: svn commit: r1912245 - in /httpd/httpd/trunk/modules/proxy: balancers/mod_lbmethod_bybusyness.c mod_proxy_balancer.c proxy_util.c proxy_util.h
On Thu, Nov 23, 2023 at 4:46 PM Yann Ylavic wrote:
>
> On Thu, Nov 23, 2023 at 4:17 PM Ruediger Pluem wrote:
> >
> > On 11/23/23 3:53 PM, Yann Ylavic wrote:
> > >
> > > As for the 64bit atomics vs APR version dance, it's because the former
> > > are not available before apr-1.7 and likely not reliable before 1.7.4
> > > (at least for some architectures where builtins are not available). In
> > > any case we need a fallback for apr < 1.7, but maybe to keep this
> > > simpler we should fall back to non-atomics (as before) in this case.
> > > It all looks over complicated for the feature, but I can't think of
> > > something simpler and still correct..
> >
> > Understood.
> >
> > I am asking because I guess I am hit now by races in the byrequests LB
> > with the worker->s->lbstatus.
> > I probably want to look for a way to solve this in a more generic way
> > for various shared worker fields.
>
> lbstatus is an int so the 32bit apr_atomic API could do I think.
I mean, if lbstatus becomes inconsistent because of the race then we
can do something, but if it is e.g. that a worker switches from error
state to non-error become some threads can connect while some others
can't this is something we should address elsewhere (like a failure
threshold).
> Maybe we need some ap_atomic_{int,long,size_t,..}_*() helpers for
> system dependent widths. It preferably would be implemented in APR but
> in the meantime we could have something in httpd already.
> What we should avoid IMO is needing 64bit atomics on 32bit systems
> (because we'd have to reimplement the mutexes etc), but apart from
> that I think we can wrap anything using the existing apr atomics.
What we need for lbstatus is ap_atomic_int_or() and
ap_atomic_int_and_not() it seems, both could be implemented with a
cas32 loop.
>
> Regards;
> Yann.
Re: svn commit: r1912245 - in /httpd/httpd/trunk/modules/proxy: balancers/mod_lbmethod_bybusyness.c mod_proxy_balancer.c proxy_util.c proxy_util.h
On Thu, Nov 23, 2023 at 4:17 PM Ruediger Pluem wrote:
>
> On 11/23/23 3:53 PM, Yann Ylavic wrote:
> >
> > As for the 64bit atomics vs APR version dance, it's because the former
> > are not available before apr-1.7 and likely not reliable before 1.7.4
> > (at least for some architectures where builtins are not available). In
> > any case we need a fallback for apr < 1.7, but maybe to keep this
> > simpler we should fall back to non-atomics (as before) in this case.
> > It all looks over complicated for the feature, but I can't think of
> > something simpler and still correct..
>
> Understood.
>
> I am asking because I guess I am hit now by races in the byrequests LB
> with the worker->s->lbstatus.
> I probably want to look for a way to solve this in a more generic way
> for various shared worker fields.
lbstatus is an int so the 32bit apr_atomic API could do I think.
Maybe we need some ap_atomic_{int,long,size_t,..}_*() helpers for
system dependent widths. It preferably would be implemented in APR but
in the meantime we could have something in httpd already.
What we should avoid IMO is needing 64bit atomics on 32bit systems
(because we'd have to reimplement the mutexes etc), but apart from
that I think we can wrap anything using the existing apr atomics.
Regards;
Yann.
Re: svn commit: r1912245 - in /httpd/httpd/trunk/modules/proxy: balancers/mod_lbmethod_bybusyness.c mod_proxy_balancer.c proxy_util.c proxy_util.h
On 11/23/23 3:53 PM, Yann Ylavic wrote: > On Thu, Nov 23, 2023 at 12:11 PM Ruediger Pluem wrote: >> >> On 9/11/23 3:50 PM, jfcl...@apache.org wrote: >>> Author: jfclere >>> Date: Mon Sep 11 13:50:21 2023 >>> New Revision: 1912245 >>> >>> URL: http://svn.apache.org/viewvc?rev=1912245=rev >>> Log: >>> Arrange the bybusyness logic and prevent bad busy values >>> this closes #383 > [] >>> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original) >>> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Mon Sep 11 13:50:21 2023 >>> @@ -21,6 +21,7 @@ >>> #include "apr_version.h" >>> #include "apr_strings.h" >>> #include "apr_hash.h" >>> +#include "apr_atomic.h" >>> #include "http_core.h" >>> #include "proxy_util.h" >>> #include "ajp.h" >>> @@ -4984,6 +4985,124 @@ PROXY_DECLARE(apr_status_t) ap_proxy_tun >>> return APR_SUCCESS; >>> } >>> >>> +PROXY_DECLARE(apr_status_t) decrement_busy_count(void *worker_) >> >> Why can't we use the respective apr_atomic_sub/add/inc/dec* functions here >> instead of the logic below? > > This has been discussed in https://github.com/apache/httpd/pull/383, > at least for why the "if atomic_dec() < 0 then atomic_inc()" > proposed originally was racy. > So the below implements dec_not_zero and inc_not_at_max to keep ->busy > in the bounds, like the original non-atomic functions. > I think the under/overflows could still happen without this because, > even if we inc per request and dec on request pool cleanup, there is a > balancer->lbmethod->reset() which may clear ->busy at any time (IIUC), > not to talk about a child crash (though then ->busy may not be very > reliable anyway, under/overflow or not..). Thanks for the pointer. I missed this. > > As for the 64bit atomics vs APR version dance, it's because the former > are not available before apr-1.7 and likely not reliable before 1.7.4 > (at least for some architectures where builtins are not available). In > any case we need a fallback for apr < 1.7, but maybe to keep this > simpler we should fall back to non-atomics (as before) in this case. > It all looks over complicated for the feature, but I can't think of > something simpler and still correct.. Understood. I am asking because I guess I am hit now by races in the byrequests LB with the worker->s->lbstatus. I probably want to look for a way to solve this in a more generic way for various shared worker fields. Regards Rüdiger
Re: svn commit: r1912245 - in /httpd/httpd/trunk/modules/proxy: balancers/mod_lbmethod_bybusyness.c mod_proxy_balancer.c proxy_util.c proxy_util.h
On Thu, Nov 23, 2023 at 12:11 PM Ruediger Pluem wrote:
>
> On 9/11/23 3:50 PM, jfcl...@apache.org wrote:
> > Author: jfclere
> > Date: Mon Sep 11 13:50:21 2023
> > New Revision: 1912245
> >
> > URL: http://svn.apache.org/viewvc?rev=1912245=rev
> > Log:
> > Arrange the bybusyness logic and prevent bad busy values
> > this closes #383
[]
> > --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> > +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Mon Sep 11 13:50:21 2023
> > @@ -21,6 +21,7 @@
> > #include "apr_version.h"
> > #include "apr_strings.h"
> > #include "apr_hash.h"
> > +#include "apr_atomic.h"
> > #include "http_core.h"
> > #include "proxy_util.h"
> > #include "ajp.h"
> > @@ -4984,6 +4985,124 @@ PROXY_DECLARE(apr_status_t) ap_proxy_tun
> > return APR_SUCCESS;
> > }
> >
> > +PROXY_DECLARE(apr_status_t) decrement_busy_count(void *worker_)
>
> Why can't we use the respective apr_atomic_sub/add/inc/dec* functions here
> instead of the logic below?
This has been discussed in https://github.com/apache/httpd/pull/383,
at least for why the "if atomic_dec() < 0 then atomic_inc()"
proposed originally was racy.
So the below implements dec_not_zero and inc_not_at_max to keep ->busy
in the bounds, like the original non-atomic functions.
I think the under/overflows could still happen without this because,
even if we inc per request and dec on request pool cleanup, there is a
balancer->lbmethod->reset() which may clear ->busy at any time (IIUC),
not to talk about a child crash (though then ->busy may not be very
reliable anyway, under/overflow or not..).
As for the 64bit atomics vs APR version dance, it's because the former
are not available before apr-1.7 and likely not reliable before 1.7.4
(at least for some architectures where builtins are not available). In
any case we need a fallback for apr < 1.7, but maybe to keep this
simpler we should fall back to non-atomics (as before) in this case.
It all looks over complicated for the feature, but I can't think of
something simpler and still correct..
>
> > +{
> > +apr_size_t val;
> > +proxy_worker *worker = worker_;
> > +
> > +#if APR_SIZEOF_VOIDP == 4
> > +AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(apr_uint32_t));
> > +val = apr_atomic_read32(>s->busy);
> > +while (val > 0) {
> > +apr_size_t old = val;
> > +val = apr_atomic_cas32(>s->busy, val - 1, old);
> > +if (val == old) {
> > +break;
> > +}
> > +}
> > +#elif APR_VERSION_AT_LEAST(1,7,4) /* APR 64bit atomics not safe before
> > 1.7.4 */
> > +AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(apr_uint64_t));
> > +val = apr_atomic_read64(>s->busy);
> > +while (val > 0) {
> > +apr_size_t old = val;
> > +val = apr_atomic_cas64(>s->busy, val - 1, old);
> > +if (val == old) {
> > +break;
> > +}
> > +}
> > +#else /* Use atomics for (64bit) pointers */
> > +void *volatile *busy_p = (void *)>s->busy;
> > +AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(void*));
> > +AP_DEBUG_ASSERT((apr_uintptr_t)busy_p % sizeof(void*) == 0);
> > +val = (apr_uintptr_t)apr_atomic_casptr((void *)busy_p, NULL, NULL);
> > +while (val > 0) {
> > +apr_size_t old = val;
> > +val = (apr_uintptr_t)apr_atomic_casptr((void *)busy_p,
> > + (void *)(apr_uintptr_t)(val
> > - 1),
> > + (void *)(apr_uintptr_t)old);
> > +if (val == old) {
> > +break;
> > +}
> > +}
> > +#endif
> > +return APR_SUCCESS;
> > +}
> > +
> > +PROXY_DECLARE(void) increment_busy_count(proxy_worker *worker)
> > +{
> > +apr_size_t val;
> > +#if APR_SIZEOF_VOIDP == 4
> > +AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(apr_uint32_t));
> > +val = apr_atomic_read32(>s->busy);
> > +while (val < APR_INT32_MAX) {
> > +apr_size_t old = val;
> > +val = apr_atomic_cas32(>s->busy, val + 1, old);
> > +if (val == old) {
> > +break;
> > +}
> > +}
> > +#elif APR_VERSION_AT_LEAST(1,7,4) /* APR 64bit atomics not safe before
> > 1.7.4 */
> > +AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(apr_uint64_t));
> > +val = apr_atomic_read64(>s->busy);
> > +while (val < APR_INT64_MAX) {
> > +apr_size_t old = val;
> > +val = apr_atomic_cas64(>s->busy, val + 1, old);
> > +if (val == old) {
> > +break;
> > +}
> > +}
> > +#else /* Use atomics for (64bit) pointers */
> > +void *volatile *busy_p = (void *)>s->busy;
> > +AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(void*));
> > +AP_DEBUG_ASSERT((apr_uintptr_t)busy_p % sizeof(void*) == 0);
> > +val = (apr_uintptr_t)apr_atomic_casptr((void *)busy_p, NULL, NULL);
> > +while (val < APR_INT64_MAX) {
> > +apr_size_t old = val;
> > +val = (apr_uintptr_t)apr_atomic_casptr((void *)busy_p,
> > +
Re: svn commit: r1914067 - in /httpd/httpd/trunk: changes-entries/ldap-optimise.txt modules/aaa/mod_authnz_ldap.c
On 11/23/23 2:28 PM, Yann Ylavic wrote:
> On Thu, Nov 23, 2023 at 1:33 PM Ruediger Pluem wrote:
>>
>>
>>
>> On 11/23/23 12:52 PM, Graham Leggett via dev wrote:
>>> On 23 Nov 2023, at 11:25, Ruediger Pluem wrote:
>>>
> -if (req->dn == NULL || !*req->dn) {
> -ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02636)
> - "auth_ldap authorize: require ldap-filter: user's
> DN "
> - "has not been defined; failing authorization");
> -return AUTHZ_DENIED;
> +if (req->dn == NULL || !*req->dn) {
> +ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02636)
> + "auth_ldap authorize: require ldap-search:
> user's DN "
> + "has not been defined; failing authorization");
> +return AUTHZ_DENIED;
> +}
Why do we need to get the dn in case that r->user is not NULL and why is
it a reason to fail if we don't get a dn for this user?
>>>
>>> This message is misleading, the DN we care about is not the DN of the user,
>>> but rather the DN of the object returned in the
>>> ldap-search, which may or may not bear a relation to the user.
>>
>> Unfortunately the message is correct and we do just that via
>> get_dn_for_nonldap_authn (line 1451). The DN you mention we look for
>> later in line 1480 via util_ldap_cache_getuserdn.
>
> My guess was that if r->user is set we always want to use it for ldap
> requests/authn? Or maybe there should be another Require ldap-!search
> configured to achieve that (if wanted) since ldap-search is special..
I think for ldap-search things depend on the configured filter expression. It
may involve a previously authenticated user, in
which case it has similar approaches like ldap-filter or even require
valid-user or it does not include that and may remind more
of a mod_authz_host with a dynamic IP list stored in LDAP. Or the certificate
example Graham gave.
>
>> BTW: I guess the following patch is missing as we don't store the dn / vals
>> we get from util_ldap_cache_getuserdn but just
>> "forget" it once we leave the function.
>>
>>
>> Index: modules/aaa/mod_authnz_ldap.c
>> ===
>> --- modules/aaa/mod_authnz_ldap.c (revision 1914069)
>> +++ modules/aaa/mod_authnz_ldap.c (working copy)
>> @@ -1482,10 +1482,12 @@
>>
>> /* Make sure that the filtered search returned a single dn */
>> if (result == LDAP_SUCCESS && dn) {
>> +req->dn = dn;
>> +req->vals = vals;
>> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02631)
>>"auth_ldap authorize: require ldap-search: "
>>"authorization successful");
>> -set_request_vars(r, LDAP_AUTHZ, req->vals);
>> +set_request_vars(r, LDAP_AUTHZ, vals);
>> return AUTHZ_GRANTED;
>> }
>> else {
>>
>>
>>>
>>> For example the ldap-search might be doing a lookup on (sucks thumb) the
>>> issuer of a certificate, which if it matches some LDAP
>>> object means it is allowed. The DN will not refer to the user, but
>>> something else.
>>
>> I get that. Hence I wonder why we care of the dn fitting to r->user set
>> before.
>>
>>>
>>> On a side note, ldap-search requires that exactly one LDAP object matches,
>>> we might want to support many matches, or a specific
>>> number of matches.
>
> I still don't get where "Make sure that the filtered search returned a
> single dn" is enforced, is it because result != LDAP_SUCCESS
> otherwise, or dn == NULL if there are multiple objects/DNs?
Have a look at line 2117 and following in modules/ldap/util_ldap.c. We call it
in line 1480 of modules/aaa/mod_authnz_ldap.c via
util_ldap_cache_getuserdn. Result will be LDAP_NO_SUCH_OBJECT and dn will stay
the same as before the call.
Regards
Rüdiger
Re: svn commit: r1914067 - in /httpd/httpd/trunk: changes-entries/ldap-optimise.txt modules/aaa/mod_authnz_ldap.c
On Thu, Nov 23, 2023 at 1:33 PM Ruediger Pluem wrote:
>
>
>
> On 11/23/23 12:52 PM, Graham Leggett via dev wrote:
> > On 23 Nov 2023, at 11:25, Ruediger Pluem wrote:
> >
> >>> -if (req->dn == NULL || !*req->dn) {
> >>> -ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02636)
> >>> - "auth_ldap authorize: require ldap-filter: user's
> >>> DN "
> >>> - "has not been defined; failing authorization");
> >>> -return AUTHZ_DENIED;
> >>> +if (req->dn == NULL || !*req->dn) {
> >>> +ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02636)
> >>> + "auth_ldap authorize: require ldap-search:
> >>> user's DN "
> >>> + "has not been defined; failing authorization");
> >>> +return AUTHZ_DENIED;
> >>> +}
> >>
> >> Why do we need to get the dn in case that r->user is not NULL and why is
> >> it a reason to fail if we don't get a dn for this user?
> >
> > This message is misleading, the DN we care about is not the DN of the user,
> > but rather the DN of the object returned in the
> > ldap-search, which may or may not bear a relation to the user.
>
> Unfortunately the message is correct and we do just that via
> get_dn_for_nonldap_authn (line 1451). The DN you mention we look for
> later in line 1480 via util_ldap_cache_getuserdn.
My guess was that if r->user is set we always want to use it for ldap
requests/authn? Or maybe there should be another Require ldap-!search
configured to achieve that (if wanted) since ldap-search is special..
> BTW: I guess the following patch is missing as we don't store the dn / vals
> we get from util_ldap_cache_getuserdn but just
> "forget" it once we leave the function.
>
>
> Index: modules/aaa/mod_authnz_ldap.c
> ===
> --- modules/aaa/mod_authnz_ldap.c (revision 1914069)
> +++ modules/aaa/mod_authnz_ldap.c (working copy)
> @@ -1482,10 +1482,12 @@
>
> /* Make sure that the filtered search returned a single dn */
> if (result == LDAP_SUCCESS && dn) {
> +req->dn = dn;
> +req->vals = vals;
> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02631)
>"auth_ldap authorize: require ldap-search: "
>"authorization successful");
> -set_request_vars(r, LDAP_AUTHZ, req->vals);
> +set_request_vars(r, LDAP_AUTHZ, vals);
> return AUTHZ_GRANTED;
> }
> else {
>
>
> >
> > For example the ldap-search might be doing a lookup on (sucks thumb) the
> > issuer of a certificate, which if it matches some LDAP
> > object means it is allowed. The DN will not refer to the user, but
> > something else.
>
> I get that. Hence I wonder why we care of the dn fitting to r->user set
> before.
>
> >
> > On a side note, ldap-search requires that exactly one LDAP object matches,
> > we might want to support many matches, or a specific
> > number of matches.
I still don't get where "Make sure that the filtered search returned a
single dn" is enforced, is it because result != LDAP_SUCCESS
otherwise, or dn == NULL if there are multiple objects/DNs?
Regards;
Yann.
Re: svn commit: r1914067 - in /httpd/httpd/trunk: changes-entries/ldap-optimise.txt modules/aaa/mod_authnz_ldap.c
On 11/23/23 12:52 PM, Graham Leggett via dev wrote:
> On 23 Nov 2023, at 11:25, Ruediger Pluem wrote:
>
>>> - if (req->dn == NULL || !*req->dn) {
>>> - ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02636)
>>> - "auth_ldap authorize: require ldap-filter: user's DN
>>> "
>>> - "has not been defined; failing authorization");
>>> - return AUTHZ_DENIED;
>>> + if (req->dn == NULL || !*req->dn) {
>>> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02636)
>>> + "auth_ldap authorize: require ldap-search:
>>> user's DN "
>>> + "has not been defined; failing authorization");
>>> + return AUTHZ_DENIED;
>>> + }
>>
>> Why do we need to get the dn in case that r->user is not NULL and why is it
>> a reason to fail if we don't get a dn for this user?
>
> This message is misleading, the DN we care about is not the DN of the user,
> but rather the DN of the object returned in the
> ldap-search, which may or may not bear a relation to the user.
Unfortunately the message is correct and we do just that via
get_dn_for_nonldap_authn (line 1451). The DN you mention we look for
later in line 1480 via util_ldap_cache_getuserdn.
BTW: I guess the following patch is missing as we don't store the dn / vals we
get from util_ldap_cache_getuserdn but just
"forget" it once we leave the function.
Index: modules/aaa/mod_authnz_ldap.c
===
--- modules/aaa/mod_authnz_ldap.c (revision 1914069)
+++ modules/aaa/mod_authnz_ldap.c (working copy)
@@ -1482,10 +1482,12 @@
/* Make sure that the filtered search returned a single dn */
if (result == LDAP_SUCCESS && dn) {
+req->dn = dn;
+req->vals = vals;
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02631)
"auth_ldap authorize: require ldap-search: "
"authorization successful");
-set_request_vars(r, LDAP_AUTHZ, req->vals);
+set_request_vars(r, LDAP_AUTHZ, vals);
return AUTHZ_GRANTED;
}
else {
>
> For example the ldap-search might be doing a lookup on (sucks thumb) the
> issuer of a certificate, which if it matches some LDAP
> object means it is allowed. The DN will not refer to the user, but something
> else.
I get that. Hence I wonder why we care of the dn fitting to r->user set before.
>
> On a side note, ldap-search requires that exactly one LDAP object matches, we
> might want to support many matches, or a specific
> number of matches.
>
Regards
Rüdiger
Re: svn commit: r1914067 - in /httpd/httpd/trunk: changes-entries/ldap-optimise.txt modules/aaa/mod_authnz_ldap.c
On 23 Nov 2023, at 11:25, Ruediger Pluem wrote:
>> -if (req->dn == NULL || !*req->dn) {
>> -ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02636)
>> - "auth_ldap authorize: require ldap-filter: user's DN "
>> - "has not been defined; failing authorization");
>> -return AUTHZ_DENIED;
>> +if (req->dn == NULL || !*req->dn) {
>> +ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02636)
>> + "auth_ldap authorize: require ldap-search: user's
>> DN "
>> + "has not been defined; failing authorization");
>> +return AUTHZ_DENIED;
>> +}
>
> Why do we need to get the dn in case that r->user is not NULL and why is it a
> reason to fail if we don't get a dn for this user?
This message is misleading, the DN we care about is not the DN of the user, but
rather the DN of the object returned in the ldap-search, which may or may not
bear a relation to the user.
For example the ldap-search might be doing a lookup on (sucks thumb) the issuer
of a certificate, which if it matches some LDAP object means it is allowed. The
DN will not refer to the user, but something else.
On a side note, ldap-search requires that exactly one LDAP object matches, we
might want to support many matches, or a specific number of matches.
Regards,
Graham
—
Re: svn commit: r1914067 - in /httpd/httpd/trunk: changes-entries/ldap-optimise.txt modules/aaa/mod_authnz_ldap.c
On 11/23/23 11:22 AM, minf...@apache.org wrote:
> Author: minfrin
> Date: Thu Nov 23 10:22:58 2023
> New Revision: 1914067
>
> URL: http://svn.apache.org/viewvc?rev=1914067=rev
> Log:
> Optimise handling LDAP authorization where LDAP was not used
> previously for LDAP authentication.
>
> Added:
> httpd/httpd/trunk/changes-entries/ldap-optimise.txt
> Modified:
> httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
>
> Modified: httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c?rev=1914067=1914066=1914067=diff
> ==
> --- httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c (original)
> @@ -1440,24 +1437,27 @@ static authz_status ldapsearch_check_aut
> * the req structure needed for authorization needs to be created
> * and populated with the userid and DN of the account in LDAP
> */
> -
> if (!req) {
> -authz_status rv = AUTHZ_DENIED;
> req = build_request_config(r);
> -ldc = get_connection_for_authz(r, LDAP_SEARCH);
> -if (AUTHZ_GRANTED != (rv = get_dn_for_nonldap_authn(r, ldc))) {
> +}
> +ldc = get_connection_for_authz(r, LDAP_SEARCH);
> +if (!req->dn && r->user) {
> +authz_status rv;
> +if (!*r->user) {
> +ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(10487)
> + "ldap authorize: Userid is blank, AuthType=%s",
> + r->ap_auth_type);
> +}
> +rv = get_dn_for_nonldap_authn(r, ldc);
> +if (rv != AUTHZ_GRANTED) {
> return rv;
> }
> -}
> -else {
> -ldc = get_connection_for_authz(r, LDAP_SEARCH);
> -}
> -
> -if (req->dn == NULL || !*req->dn) {
> -ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02636)
> - "auth_ldap authorize: require ldap-filter: user's DN "
> - "has not been defined; failing authorization");
> -return AUTHZ_DENIED;
> +if (req->dn == NULL || !*req->dn) {
> +ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02636)
> + "auth_ldap authorize: require ldap-search: user's
> DN "
> + "has not been defined; failing authorization");
> +return AUTHZ_DENIED;
> +}
Why do we need to get the dn in case that r->user is not NULL and why is it a
reason to fail if we don't get a dn for this user?
> }
>
> require = ap_expr_str_exec(r, expr, );
>
>
>
Regards
Rüdiger
Re: svn commit: r1912245 - in /httpd/httpd/trunk/modules/proxy: balancers/mod_lbmethod_bybusyness.c mod_proxy_balancer.c proxy_util.c proxy_util.h
On 9/11/23 3:50 PM, jfcl...@apache.org wrote:
> Author: jfclere
> Date: Mon Sep 11 13:50:21 2023
> New Revision: 1912245
>
> URL: http://svn.apache.org/viewvc?rev=1912245=rev
> Log:
> Arrange the bybusyness logic and prevent bad busy values
> this closes #383
>
> Modified:
> httpd/httpd/trunk/modules/proxy/balancers/mod_lbmethod_bybusyness.c
> httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
> httpd/httpd/trunk/modules/proxy/proxy_util.c
> httpd/httpd/trunk/modules/proxy/proxy_util.h
>
> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c?rev=1912245=1912244=1912245=diff
> ==
> --- httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_balancer.c Mon Sep 11 13:50:21
> 2023
> @@ -17,6 +17,7 @@
> /* Load balancer module for Apache proxy */
>
> #include "mod_proxy.h"
> +#include "proxy_util.h"
> #include "scoreboard.h"
> #include "ap_mpm.h"
> #include "apr_version.h"
> @@ -486,17 +487,6 @@ static void force_recovery(proxy_balance
> }
> }
>
> -static apr_status_t decrement_busy_count(void *worker_)
> -{
> -proxy_worker *worker = worker_;
> -
> -if (worker->s->busy) {
> -worker->s->busy--;
> -}
> -
> -return APR_SUCCESS;
> -}
> -
> static int proxy_balancer_pre_request(proxy_worker **worker,
>proxy_balancer **balancer,
>request_rec *r,
> @@ -635,7 +625,7 @@ static int proxy_balancer_pre_request(pr
> *worker = runtime;
> }
>
> -(*worker)->s->busy++;
> +increment_busy_count(*worker);
> apr_pool_cleanup_register(r->pool, *worker, decrement_busy_count,
>apr_pool_cleanup_null);
>
> @@ -1575,7 +1565,7 @@ static void balancer_display_page(reques
>"\n", NULL);
> ap_rprintf(r,
> " %" APR_SIZE_T_FMT
> "\n",
> - worker->s->busy);
> + getbusy_count(worker));
> ap_rprintf(r, " %d\n",
> worker->s->lbset);
> /* End proxy_worker_stat */
> @@ -1748,7 +1738,7 @@ static void balancer_display_page(reques
> ap_rvputs(r, ap_proxy_parse_wstatus(r->pool, worker), NULL);
> ap_rputs("", r);
> ap_rprintf(r, "%" APR_SIZE_T_FMT "",
> worker->s->elected);
> -ap_rprintf(r, "%" APR_SIZE_T_FMT "",
> worker->s->busy);
> +ap_rprintf(r, "%" APR_SIZE_T_FMT "",
> getbusy_count(worker));
> ap_rprintf(r, "%d", worker->s->lbstatus);
> ap_rputs(apr_strfsize(worker->s->transferred, fbuf), r);
> ap_rputs("", r);
>
> Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1912245=1912244=1912245=diff
> ==
> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Mon Sep 11 13:50:21 2023
> @@ -21,6 +21,7 @@
> #include "apr_version.h"
> #include "apr_strings.h"
> #include "apr_hash.h"
> +#include "apr_atomic.h"
> #include "http_core.h"
> #include "proxy_util.h"
> #include "ajp.h"
> @@ -4984,6 +4985,124 @@ PROXY_DECLARE(apr_status_t) ap_proxy_tun
> return APR_SUCCESS;
> }
>
> +PROXY_DECLARE(apr_status_t) decrement_busy_count(void *worker_)
Why can't we use the respective apr_atomic_sub/add/inc/dec* functions here
instead of the logic below?
> +{
> +apr_size_t val;
> +proxy_worker *worker = worker_;
> +
> +#if APR_SIZEOF_VOIDP == 4
> +AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(apr_uint32_t));
> +val = apr_atomic_read32(>s->busy);
> +while (val > 0) {
> +apr_size_t old = val;
> +val = apr_atomic_cas32(>s->busy, val - 1, old);
> +if (val == old) {
> +break;
> +}
> +}
> +#elif APR_VERSION_AT_LEAST(1,7,4) /* APR 64bit atomics not safe before 1.7.4
> */
> +AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(apr_uint64_t));
> +val = apr_atomic_read64(>s->busy);
> +while (val > 0) {
> +apr_size_t old = val;
> +val = apr_atomic_cas64(>s->busy, val - 1, old);
> +if (val == old) {
> +break;
> +}
> +}
> +#else /* Use atomics for (64bit) pointers */
> +void *volatile *busy_p = (void *)>s->busy;
> +AP_DEBUG_ASSERT(sizeof(apr_size_t) == sizeof(void*));
> +AP_DEBUG_ASSERT((apr_uintptr_t)busy_p % sizeof(void*) == 0);
> +val = (apr_uintptr_t)apr_atomic_casptr((void *)busy_p, NULL, NULL);
> +while (val > 0) {
> +
