Re: [WIP] mod_authnz_ldap / mod_ldap support for APR v2 LDAP API

[Graham Leggett via dev]

On 19 Apr 2024, at 09:01, Ruediger Pluem  wrote:

> First of all I haven't gone through the patch. Hence I may be off below.
> 
> I think the core issue with the LDAP API we have in APR-UTIL 1.x today and 
> that let to the
> removal discussions on APR side in 2010 
> (https://lists.apache.org/thread/pz5c28g0gf248fvpds53zl1f8by9n8n4)
> and 2011 (https://lists.apache.org/thread/cslw4gj0jgx3bmr8trz0jtfq5bmv1tfl) 
> is that it is an incomplete
> abstraction of the LDAP libraries. mod_authnz_ldap and mod_ldap as consumers 
> of the current API
> have LDAP library specific code and need to link directly against the 
> specific LDAP library.

What you've described above is the exact thing that has been fixed.

This is the core of the patch:

https://github.com/apache/httpd/pull/438/files#diff-f309c2fcd650f484a21c930ddde995a646387421b671232f5b643328f7d2da06

diff --git a/modules/aaa/config.m4 b/modules/aaa/config.m4
index 1b59f99f492..b12e03b287d 100644
--- a/modules/aaa/config.m4
+++ b/modules/aaa/config.m4
@@ -49,21 +49,7 @@ APACHE_MODULE(authz_core, core authorization provider vector 
module, , , yes)
 
 dnl LDAP authentication module. This module has both the authn and authz
 dnl modules in one, so as to share the LDAP server config directives.
-APACHE_MODULE(authnz_ldap, LDAP based authentication, , , most, [
-  APACHE_CHECK_APR_HAS_LDAP
-  if test "$ac_cv_APR_HAS_LDAP" = "yes" ; then
-if test -z "$apu_config" ; then
-  LDAP_LIBS="`$apr_config --ldap-libs`"
-else
-  LDAP_LIBS="`$apu_config --ldap-libs`"
-fi
-APR_ADDTO(MOD_AUTHNZ_LDAP_LDADD, [$LDAP_LIBS])
-AC_SUBST(MOD_AUTHNZ_LDAP_LDADD)
-  else
-AC_MSG_WARN([apr/apr-util is compiled without ldap support])
-enable_authnz_ldap=no
-  fi
-])
+APACHE_MODULE(authnz_ldap, LDAP based authentication, , , most)
 
 dnl FastCGI authorizer interface, supporting authn and authz.
 APACHE_MODULE(authnz_fcgi,

LDAP is no longer special. DBD, DBM, SQL, LDAP, all the same.

> I agree that it is of limited sense to decide during runtime which LDAP 
> library to use as typically there is
> only one available on a particular system. This is substantially different 
> from the situation with databases.

This isn't the whole story.

Like databases, we have lots of different drivers with wildly different and 
incompatible implementations. Unlike databases, the majority of these drivers 
are based on a C API published as an RFC 30 years ago, and therefore are about 
80% common code / clashing symbols. It is the way that it is. The driver 
specific code involves SSL (which evolved) and SASL binding (which evolved), 
but the vast majority of the code is still standard.

For this reason it's pointless coming up with five separate drivers for five 
separate libraries. You can never load two at the same time because clashing 
symbols. This is true of all code, it has nothing to do with httpd 
specifically. Someone binds to library A in a module, and then someone else 
binds to library B in a different module, you are not loading those two modules 
into the server at the same time, it's just not happening.

A further problem is that the libraries diverged.

Many of the functions in OpenLDAP and Windows have been deprecated, replaced 
with _ext functions. The LDAP_DEPRECATED define is now gone.

Rebinding for example works totally differently if you're on IBM, or on 
OpenLDAP. One does not block, but has limited SASL capability. The other does 
SASL for you, but blocks. Windows expects you to rebind yourself.

SASL is wild west. OpenLDAP does SASL for you, but you have to do an intricate 
dance between two functions to make it work. Windows says "here you go, have a 
binary blob, pass it to the Windows specific SASL implementation will you". 
Again, different approaches, it is what it is.
 
Non-blocking behaviour is inconsistent. Some functions in some libraries block 
in some circumstances. You've got to be a masochist if every single consumer of 
LDAP has to deal with all of these inconsistencies, over and over again.

The "P" in APR stands for portability, and that is exactly what I as a consumer 
of an LDAP API want.

> The point back then on APR side was that this incomplete API should be either 
> "completed" or removed from APR
> and that its code should move to httpd which should deal then directly with 
> all the gory details of the various
> LDAP libraries not just with a subset. This happened on APR trunk side, but 
> not on httpd trunk side.

What happened in the past was that code was removed from APR without a 
replacement implementation having been provided first. The expectation was that 
other people (me) would provide free labour to replace that removed 
implementation to make the server whole again.

I have been working on this on and off for years, providing custom patched 
httpd's to get the immediate job done. It has reached the point where I can't 
be supporting custom patches and this needed finishing.

People have been very 

Re: [WIP] mod_authnz_ldap / mod_ldap support for APR v2 LDAP API

[Graham Leggett via dev]

On 19 Apr 2024, at 09:07, Ruediger Pluem  wrote:

> Would it be possible to provide this patch as a PR against trunk on Github? I 
> think that would ease reviewing it.
> I am quite aware that this moves some gory detail discussion of that patch 
> away from the dev list here and over
> to the PR where it is likely harder to find later. Nevertheless I think that 
> the easier way to review the patch combined with
> the tests applied to the PR via Github actions outweigh this drawback.

Take a look here:

https://github.com/apache/httpd/pull/438

The key change is that the new apr-ldap API is 100% asynchronous, with helpful 
patterns to make it easy to use in synchronous code like present httpd. The 
intention is that future code will allow us to have an LDAP connection 
controlled by the MPM, with "guest" requests added by httpd requests and 
removed by pool cleanups. This is useful by anybody, not just httpd.

RFC1823 is 30-ish years old, we're not exposing this old API any more.

https://www.rfc-editor.org/rfc/rfc1823.html

Regards,
Graham
--

Re: [WIP] mod_authnz_ldap / mod_ldap support for APR v2 LDAP API

[Graham Leggett via dev]

On 18 Apr 2024, at 14:21, Joe Orton  wrote:

> This design decision seems surprising to me, what does this add? Adding 
> another abstraction layer to allow runtime selection of the LDAP library 
> seems like a step backwards (a lot of complexity with no benefit). 
> Unlike with e.g. a database backend selection users don't care about 
> picking/configuring among many LDAP libraries.

Can you read through the following and confirm whether your questions are 
answered?

https://github.com/apache/apr/blob/trunk/include/apr_ldap.h#L27

If any specific questions aren't answered, can you confirm so I can make these 
clear in the docs?

Regards,
Graham
--

Re: [VOTE] Release httpd-2.4.59-rc1 as httpd-2.4.59

[Graham Leggett via dev]

On 03 Apr 2024, at 13:26, Eric Covener  wrote:

> [ ] +1: It's not just good, it's good enough!

+1 on RHEL9.

Regards,
Graham
--

Re: mod_systemd: refactor to get rid of libsystemd dependency?

[Graham Leggett via dev]

On 02 Apr 2024, at 11:25, Rainer Jung  wrote:

> in the light of the recent xz attack I was wondering, whether we should also 
> reduce our library dependencies by no longer using sd_notify() in mod_systemd 
> (thus loading libsystemd and all of its dependencies), but instead taking the 
> approach to hard code sd_notify functionality.
> 
> I guess the Linux distributors who patched sshd to use libsystemd for 
> notification are on their way to do the same for their sshd patches, so we 
> might soon get an idea how to do that properly.
> 
> This is not meant to become part of out next release (this week), but 
> hopefully we can manage to code it for the next one.
> 
> WDYT: does this make sense?

Definite +1.

The attack surface on systemd has always been too big, now is the time to fix 
that.

Regards,
Graham
--

Re: mod_ssl: Add support for loading keys from OpenSSL 3.x providers via STORE

[Graham Leggett via dev]

On 27 Nov 2023, at 15:02, Ingo Franzki  wrote:

> The mod_ssl module has support for loading keys and certificates from OpenSSL 
> engines via PKCS#11 URIs at SSLCertificateFile and SSLCertificateKeyFile, 
> e.g. using the PKCS#11 engine part of libp11 
> (https://github.com/OpenSC/libp11). 
> 
> This works fine, but with OpenSSL 3.0 engines got deprecated, and a new 
> provider concept is used.
> OpenSSL 1.1.1 is no longer supported by the OpenSSL organization 
> (https://www.openssl.org/blog/blog/2023/09/11/eol-111/), 
> and newer distributions all have OpenSSL 3.x included.
> Currently, engines do still work, bit since they are deprecated, they will at 
> some point in time no longer be working.
> 
> With OpenSSL 3.x providers one can implements loading of keys and 
> certificates by implementing a STORE method.
> With this, keys and certificates can be loaded for example from PKCS#11 
> modules via PKCS#11 URIs, just like it was possible with an PKCS#11 engine. 
> 
> Please find below some code changes required to support loading the server 
> private key and certificates from a PKCS#11 provider using OpenSSL STORE 
> providers. 

Definite +1 in principle.

> Index: docs/manual/mod/mod_ssl.html.en.utf8
> ===
> --- docs/manual/mod/mod_ssl.html.en.utf8  (revision 1914150)
> +++ docs/manual/mod/mod_ssl.html.en.utf8  (working copy)
> @@ -666,7 +666,7 @@

Would it be possible to patch mod_ssl.xml instead of the html file, the html is 
autogenerated.

> Index: modules/ssl/ssl_engine_config.c
> ===
> --- modules/ssl/ssl_engine_config.c   (revision 1914150)
> +++ modules/ssl/ssl_engine_config.c   (working copy)
> @@ -689,6 +689,11 @@
> if (strcEQ(arg, "builtin")) {
> mc->szCryptoDevice = NULL;
> }
> +#if MODSSL_USE_OPENSSL_STORE
> +else if (strcEQ(arg, "provider")) {
> +mc->szCryptoDevice = arg;
> +}
> +#endif
> #if MODSSL_HAVE_ENGINE_API

This patch isn’t applying for me, looks like the leading spaces have been lost. 
Would it be possible to try attach it as a file?

Regards,
Graham
—

Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c

[Graham Leggett via dev]

On 24 Nov 2023, at 19:26, Ruediger Pluem  wrote:

>> We do - and we also need to apr_pstrdup() the dn to be consistent with the 
>> rest.
> 
> Why? dn is already apr_pstrdup from r->pool:
> 
> https://github.com/apache/httpd/blob/dc76ce4c43efb8c0c36a5990aeb0468a87458087/modules/ldap/util_ldap.c#L2133

Looks like we were consistently wasteful due to lack of const, fixed in 
r1914281.

Regards,
Graham
—

Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c

[Graham Leggett via dev]

On 24 Nov 2023, at 14:25, Ruediger Pluem  wrote:

>> +req->dn = dn;
> 
> Don't we need to save the vals as well?

We do - and we also need to apr_pstrdup() the dn to be consistent with the rest.


Index: modules/aaa/mod_authnz_ldap.c
===
--- modules/aaa/mod_authnz_ldap.c   (revision 1914090)
+++ modules/aaa/mod_authnz_ldap.c   (working copy)
@@ -1453,7 +1453,6 @@
 t = require;
 
 if (t[0]) {
-const char **vals;
 
 ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02630)
   "auth_ldap authorize: checking filter %s", t);
@@ -1460,11 +1459,11 @@
 
 /* Search for the user DN */
 result = util_ldap_cache_getuserdn(r, ldc, sec->url, sec->basedn,
- sec->scope, sec->attributes, t, , );
+ sec->scope, sec->attributes, t, , &(req->vals));
 
 /* Make sure that the filtered search returned a single dn */
 if (result == LDAP_SUCCESS && dn) {
-req->dn = dn;
+req->dn = apr_pstrdup(r->pool, dn);
 ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02631)
   "auth_ldap authorize: require ldap-search: "
   "authorization successful");

Regards,
Graham
—

Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c

[Graham Leggett via dev]

On 24 Nov 2023, at 10:03, Yann Ylavic  wrote:

> +1 this is pretty much what Rüdiger proposed earlier and it aligns
> with the proposed 2.4.x backport so I understand better :)

Rüdiger’s patch had conflicts in the time he posted it and now, I reapplied his 
changes in the above patch and confirmed they worked.

Who wants to commit? (Am happy to)

Regards,
Graham
—

Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c

[Graham Leggett via dev]

On 20 Nov 2023, at 15:10, Ruediger Pluem  wrote:

> Revisiting this again. I guess the below patch should fix it.
> I assume that only the checking on req != NULL is relevant.
> r->user and req->dn do not matter. But we should set req->dn to dn
> if util_ldap_cache_getuserdn found one.
> 
> BTW: get_connection_for_authz(r, LDAP_SEARCH); requires a valid req. It 
> cannot deal with req == NULL.
> 
> 
> Index: modules/aaa/mod_authnz_ldap.c
> ===
> --- modules/aaa/mod_authnz_ldap.c (revision 1913980)
> +++ modules/aaa/mod_authnz_ldap.c (working copy)
> @@ -1442,23 +1442,10 @@
>  */
> 
> if (!req) {
> -authz_status rv = AUTHZ_DENIED;
> req = build_request_config(r);

I completely misunderstood this - I had the idea that build_request_config() 
was being removed, when it was being left behind, sorry about that.

The patch that applies to trunk looks like this, and I just tested it and it 
works:

Index: modules/aaa/mod_authnz_ldap.c
===
--- modules/aaa/mod_authnz_ldap.c   (revision 1914067)
+++ modules/aaa/mod_authnz_ldap.c   (working copy)
@@ -1441,24 +1441,6 @@
 req = build_request_config(r);
 }
 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;
-}
-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;
-}
-}
 
 require = ap_expr_str_exec(r, expr, );
 if (err) {
@@ -1482,6 +1464,7 @@
 
 /* Make sure that the filtered search returned a single dn */
 if (result == LDAP_SUCCESS && dn) {
+req->dn = dn;
 ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02631)
   "auth_ldap authorize: require ldap-search: "
   "authorization successful");

Regards,
Graham
—

Re: svn commit: r1914067 - in /httpd/httpd/trunk: changes-entries/ldap-optimise.txt modules/aaa/mod_authnz_ldap.c

[Graham Leggett via dev]

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: r1914067 - in /httpd/httpd/trunk: changes-entries/ldap-optimise.txt modules/aaa/mod_authnz_ldap.c

[Graham Leggett via dev]

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: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c

[Graham Leggett via dev]

On 20 Nov 2023, at 15:32, Yann Ylavic  wrote:

> OK, I drop a new v3 here just in case and let those who know how LDAP
> authn/authz work take whatever :)
> This is just to show that there is some room for
> factorization/disambiguation in this code..
> 
> 
> Regards;
> Yann.
> 

Just tested this, and it appears to work fine.

One tiny detail, these is a logging line that refers to ldap-filter inside 
ldap-search, but other than that, +1.

+  "auth_ldap authorize: require ldap-filter: user's DN 
"

Regards,
Graham
—

Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c

[Graham Leggett via dev]

On 20 Nov 2023, at 15:10, Ruediger Pluem  wrote:

> Revisiting this again. I guess the below patch should fix it.
> I assume that only the checking on req != NULL is relevant.
> r->user and req->dn do not matter. But we should set req->dn to dn
> if util_ldap_cache_getuserdn found one.
> 
> BTW: get_connection_for_authz(r, LDAP_SEARCH); requires a valid req. It 
> cannot deal with req == NULL.
> 
> 
> Index: modules/aaa/mod_authnz_ldap.c
> ===
> --- modules/aaa/mod_authnz_ldap.c (revision 1913980)
> +++ modules/aaa/mod_authnz_ldap.c (working copy)
> @@ -1442,23 +1442,10 @@
>  */
> 
> 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))) {
> -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;
> -}
> +ldc = get_connection_for_authz(r, LDAP_SEARCH);

Unfortunately removing this makes it impossible to use ldapsearch for authz 
only, as the setup process normally done in the authn phase never happens.

The typical use case for ldapsearch is without authn, because you’re allowing 
someone in based on something other than a user login, for example a property 
of the SSL connection, or an IP address, etc.

Regards,
Graham
—

Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c

[Graham Leggett via dev]

On 20 Nov 2023, at 12:26, Ruediger Pluem  wrote:

> Or we need to ensure that authn_ldap_build_filter is NULL safe and returns in 
> a sensible way if user == NULL.

This is the option we need I think - it’s possible that ldapsearch could be 
used without a user.

Regards,
Graham
—

Re: svn commit: r1913962 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c

[Graham Leggett via dev]

On 20 Nov 2023, at 12:10, Yann Ylavic  wrote:

>> Fine, but if r->user is NULL here we'll segfault (NULL dereference) in
>> "if (!*r->user)" here.
> 
> Probably an unfortunate copy/paste in trunk only (not in your backport
> patch3), fixed in r1913977.

Thanks for this - it’s been making my head bleed.

Regards,
Graham
—

Re: svn commit: r1913962 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c

[Graham Leggett via dev]

On 20 Nov 2023, at 09:32, Ruediger Pluem  wrote:

>> -if (sec->host) {
>> +if (!sec->host) {
>> +ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01738)
> 
> This log message had the number 02636 before.

I’ve fixed these.

They don’t appear to affect the ldapsearch backport proposal, all the codes 
there are unique.

Regards,
Graham
—

Re: svn commit: r1913962 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c

[Graham Leggett via dev]

On 20 Nov 2023, at 10:44, Yann Ylavic  wrote:

>> URL: http://svn.apache.org/viewvc?rev=1913962=rev
>> Log:
>> Apply earlier fix to the ldapsearch case:
>> 
>> Arrange for backend LDAP connections to be returned
>> to the pool by a fixup hook rather than staying locked
>> until the end of (a potentially slow) request.
> 
> It seems that this commit aligns the checks/setup of ldapsearch with
> the ones of ldapfilter, but nothing about LDAP connections
> recycling/reuse?

That’s correct - the recycling/reuse code is being backported separately, it 
all depends on this code.

The end goal is for all trunk changes to be applied to v2.4 and each is aligned.

> In ldapfilter_check_authorization() we bail out early if r->user is
> NULL but not here in ldapsearch_check_authorization(), can't it
> happen?

In ldapsearch we don’t care about the user, it’s purely whether the filter 
expression being applied in the search returns exactly one result.

In ldapfilter we amend the filter to add the user, which is what we don’t want 
in ldapsearch.

Regards,
Graham
—

Re: svn commit: r1591012 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number modules/aaa/mod_authnz_ldap.c

[Graham Leggett via dev]

On 18 Nov 2023, at 17:14, Yann Ylavic  wrote:

> Oh it seems that the callers want the "filtbuf" to be \0 terminated
> (even in case of error), so this v2 probably..

Looking at this now.

Seems to be some differences between trunk and v2.4, seeing how they can be 
aligned to make this easier.

Regards,
Graham
—

Re: svn commit: r1913936 - /httpd/httpd/branches/2.4.x/STATUS

[Graham Leggett via dev]

On 18 Nov 2023, at 14:43, Yann Ylavic  wrote:

> Do we need an MMN bump for some ap_proxy_ functions added to
> proxy_util.h (non public header), though AP_PROXY_DECLARE()d because
> they need to be accessible from several proxy modules SSOs?

I double checked, thought proxy_util.h was public when it isn’t, looks fine 
without the bump.

Regards,
Graham
—

Re: svn commit: r1909137 - in /httpd/httpd/trunk: CHANGES modules/mappers/mod_alias.c

[Graham Leggett via dev]

On 17 Apr 2023, at 12:07, Yann Ylavic  wrote:

>> The question is if we find an approach to
>> make it backportable e.g. by adding some kind of 2.4.x only configuration 
>> switch to get the behavior of r1909137.
> 
> Maybe "AliasPerDirRelative on" (default: "off", context: "server
> config, virtual host, directory").
> And then we change the default to "on" in trunk.

I called the directive AliasPreservePath which seemed a more accurate fit, and 
added it in r1911067.

Regards,
Graham
—

Re: [VOTE] Switch read/write repository from Subversion to Git

[Graham Leggett via dev]

On 04 May 2023, at 09:34, Ruediger Pluem  wrote:

> This is a formal vote on whether we should move our read/write repository 
> from Subversion to Git.
> This means that our latest read/write repository will be no longer available 
> via svn.apache.org. It
> will be available via Git at 
> https://gitbox.apache.org/repos/asf/httpd-site.git and 
> https://github.com/apache/httpd.git.
> Github also offers the possibility to use a Subversion client:
> https://docs.github.com/en/get-started/working-with-subversion-on-github/support-for-subversion-clients
> 
> 
> [ ]: Move the read/write repository from Subversion to Git and leverage the 
> features of Github (for now Actions and PR).
> [ ]: Move the read/write repository from Subversion to Git, but I don't want 
> to work with Github and I will only work with
> what gitbox.apache.org offers.
> [X]: Leave everything as is.

I would rather see proper SVN integration with Github. This is a vote of no 
confidence in our own projects.

Regards,
Graham
—

Re: ci vs PR approvals? (was: [apache/httpd] Fix a possible NULL pointer dereference in hook_uri2file (PR #355))

[Graham Leggett via dev]

On 25 Apr 2023, at 07:45, Ruediger Pluem  wrote:

> 2. Switching from Subversion to Git is mostly an emotional problem for me. We 
> have some closer ties to Subversion by some
>   overlaps in the community and via mod_dav_svn we kind of partially eat our 
> very own dogfood here by using Subversion.
>   We wouldn't do that any longer with Git. Plus it would switch another of 
> our development tools from an Apache license to GPL.
>   Apart from technical aspects that this change would create we should check 
> if all of the current active committers are fine
>   using Github. While people could use Gitbox and thus avoid Github when we 
> use Git I would like us to leverage the features of
>   Github when we would do this switch and I think this cannot be done if 
> active committers would have issues with Github.

+1.

I've always found the fight about “must be git” to be really tedious. Github 
supports both git and svn to this day, and people are free to use what they 
prefer by using the interface they are most familiar with.

While Github is popular today, this is only because the goals of the owners of 
Github are presently aligned with our goals. As Twitter has taught us, goals 
change at any time and without warning.

Regards,
Graham
—

Re: [Patch] mod_auth_bearer / mod_autht_jwt: An alternative to AJP

[Graham Leggett via dev]

On 19 Mar 2020, at 12:26, Graham Leggett  wrote:On 19 Mar 2020, at 02:40, Eric Covener <cove...@gmail.com> wrote:Neat, have you thought about mod_auth_form in relation to this?Something on my wishlist has been to not put the password in thesession / not continue to call the original auth provider.Yes - the two modules that will benefit from token support are mod_session (which mod_auth_form is just one possible “onramp” to obtain a session token), and mod_ssl, where the token is the cert.Getting back to this.Added in r1909409 and r1909411.There is a corresponding library for tomcat that allows it to receive bearer auth here: https://github.com/minfrin/tomcat-jwt-authenticatorRegards,Graham—

Re: svn commit: r1909135 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h server/core.c

[Graham Leggett via dev]

On 19 Apr 2023, at 19:08, Yann Ylavic  wrote:

> Currently all the tests (framework) are broken due to cmd->regex not
> being properly stacked/scoped.
> If cmd->regex is to be used to store the enclosing section's regex, it
> should be saved and restored by *all* the the sections (much like
> cmd->override is saved/restored using the old_overrides stack variable
> in most sections). It should also be set to NULL when parsing
> (sub-)sections with no possible regex, unless we want to inherit
> cmd->regex there.

This is done in r1909356.

> 
> But for instance some like:
>   
>  
># use /fake from 
>Alias /real
>  
>   
> won't work for Alias because  overwrites cmd->path with "*If".

I just found that - that’s definitely broken, it means you can’t do this:


  
DAV svn
SVNPath /home/trac/svn/extra
  


Looks like the same effect as the Alias bug, where every url is mapped to “*If”.

> Also, what about:
>   
>  
># which regex here?
>  
>   

In the above “which regex here” would be FilesMatch, which wouldn’t change.

> Or:
>   
>  
># 's regex usable here?
>  
>   
> ?

Combining regexes is probably a step too far. Right now DirectoryMatch’s regex 
isn’t usable at the point as the DirectoryMatch regex is not passed across 
Files. This behaviour doesn't change.

> Or third-party sections (unaware of cmd->regex) which contain
> directives that depend on cmd->regex?

Again there would be no change to behaviour that I can see, the regex was 
hidden before and the raw path (containing the regex) would be used, now 
cmd->regex would be set to NULL and the raw path (containing the regex) would 
be used.

> I'm not saying it's a bad idea but it needs more changes than this
> commit, changes that spread all over the code base it seems (modules
> can have their sections too), something like:
> $ grep -r 'AP_INIT\w\+("<' server/ modules/ 2>/dev/null
> server/core.c:AP_INIT_RAW_ARGS(" server/core.c:AP_INIT_RAW_ARGS(" server/core.c:AP_INIT_RAW_ARGS(" NULL, RSRC_CONF,
[snip]

cmd->regex is a function of cmd->path, so only places that touch cmd->path need 
touch cmd->regex, and this seems to be directory, location and file.

 seems broken right now, need to fix that separately.

> Maybe we only want to check that the parent directive is a 
> with Alias for now?

Right now it’s limited to not-in-directory:

https://github.com/apache/httpd/blob/trunk/modules/mappers/mod_alias.c#L146

If this is too difficult to backport it isn't the end of the world, the main 
thing is that it's fixed for the future.

Regards,
Graham
—

POC: updated ldapi:// support for mod_ldap

[Graham Leggett via dev]

Hi all,

This is a patch for httpd that adds support for ldapi:// URLs to mod_ldap and 
friends.

It depends on a patch for apr-util posted to the dev@apr list.

Regards,
Graham
—


Index: include/util_ldap.h
===
--- include/util_ldap.h (revision 1909117)
+++ include/util_ldap.h (working copy)
@@ -45,6 +45,10 @@
 /* this whole thing disappears if LDAP is not enabled */
 #if APR_HAS_LDAP
 
+#ifndef APR_HAS_LDAP_INITIALIZE
+#define APR_HAS_LDAP_INITIALIZE 0
+#endif
+
 #if defined(LDAP_UNAVAILABLE) || APR_HAS_MICROSOFT_LDAPSDK
 #define AP_LDAP_IS_SERVER_DOWN(s)((s) == LDAP_SERVER_DOWN \
 ||(s) == LDAP_UNAVAILABLE)
@@ -126,8 +130,8 @@
 const char *reason; /* Reason for an error failure */
 
 struct util_ldap_connection_t *next;
-struct util_ldap_state_t *st;/* The LDAP vhost config this 
connection belongs to */
-int keep;/* Will this connection be kept when 
it's unlocked */
+struct util_ldap_state_t *st;   /* The LDAP vhost config this 
connection belongs to */
+int keep;   /* Will this connection be kept when 
it's unlocked */
 
 int ChaseReferrals; /* [on|off] (default = 
AP_LDAP_CHASEREFERRALS_ON)*/
 int ReferralHopLimit;   /* # of referral hops to follow 
(default = AP_LDAP_DEFAULT_HOPLIMIT) */
@@ -136,6 +140,12 @@
 int must_rebind;/* The connection was last bound with 
other then binddn/bindpw */
 request_rec *r; /* request_rec used to find this 
util_ldap_connection_t */
 apr_time_t last_backend_conn;   /* the approximate time of the last 
backend LDAP request */
+
+#if APR_HAS_LDAP_INITIALIZE
+apr_ldap_err_t result;  /* result of prior operations on this 
connection */
+const char *url;/* URL of the LDAP server (or space 
separated list) */
+apr_ldap_t *ld;
+#endif
 } util_ldap_connection_t;
 
 typedef struct util_ldap_config_t {
@@ -241,6 +251,7 @@
  * @fn util_ldap_connection_t *util_ldap_connection_find(request_rec *r, const 
char *host, int port,
  *   const char 
*binddn, const char *bindpw, deref_options deref,
  *   int netscapessl, 
int starttls)
+ * @deprecated Replaced by uldap_connection_find_ex()
  */
 APR_DECLARE_OPTIONAL_FN(util_ldap_connection_t 
*,uldap_connection_find,(request_rec *r, const char *host, int port,
   const char *binddn, const 
char *bindpw, deref_options deref,
@@ -247,6 +258,26 @@
   int secure));
 
 /**
+ * Find a connection in a list of connections
+ * @param r The request record
+ * @param url The URL to connect to (multiple URLs space separated)
+ * @param binddn The DN to bind with
+ * @param bindpw The password to bind with
+ * @param deref The dereferencing behavior
+ * @param secure use SSL on the connection
+ * @tip Once a connection is found and returned, a lock will be acquired to
+ *  lock that particular connection, so that another thread does not try 
and
+ *  use this connection while it is busy. Once you are finished with a 
connection,
+ *  apr_ldap_connection_close() must be called to release this connection.
+ * @fn util_ldap_connection_t *util_ldap_connection_find_ex(request_rec *r, 
const char *url,
+ *   const char 
*binddn, const char *bindpw, deref_options deref,
+ *   int netscapessl, 
int starttls)
+ */
+APR_DECLARE_OPTIONAL_FN(util_ldap_connection_t 
*,uldap_connection_find_ex,(request_rec *r, const char *url,
+  const char *binddn, const 
char *bindpw, deref_options deref,
+  int secure));
+
+/**
  * Compare two DNs for sameness
  * @param r The request record
  * @param ldc The LDAP connection being used.
Index: modules/aaa/mod_authnz_ldap.c
===
--- modules/aaa/mod_authnz_ldap.c   (revision 1909117)
+++ modules/aaa/mod_authnz_ldap.c   (working copy)
@@ -104,7 +104,7 @@
 module AP_MODULE_DECLARE_DATA authnz_ldap_module;
 
 static APR_OPTIONAL_FN_TYPE(uldap_connection_close) 
*util_ldap_connection_close;
-static APR_OPTIONAL_FN_TYPE(uldap_connection_find) *util_ldap_connection_find;
+static APR_OPTIONAL_FN_TYPE(uldap_connection_find_ex) 
*util_ldap_connection_find_ex;
 static APR_OPTIONAL_FN_TYPE(uldap_cache_comparedn) *util_ldap_cache_comparedn;
 static APR_OPTIONAL_FN_TYPE(uldap_cache_compare) *util_ldap_cache_compare;
 static APR_OPTIONAL_FN_TYPE(uldap_cache_check_subgroups) 
*util_ldap_cache_check_subgroups;
@@ -453,9 

Re: svn commit: r1909137 - in /httpd/httpd/trunk: CHANGES modules/mappers/mod_alias.c

[Graham Leggett via dev]

On 14 Apr 2023, at 17:18, Ruediger Pluem  wrote:

> Would that break configs like
> 
> 
> 
> Alias /filesystemprefix/%{HTTP:X-example-header}
> 
> 
> 
> where the expression evaluation determines the complete filesystem path 
> without adding the remainder of the URL?

It would, which alas might mean we can't backport this, which isn’t great.

> I admit that the above looks like a strange setup and is probably a bad 
> example, but the parameter to Alias could be an arbitrary
> complex expression that evaluates to the final filesystem resource (like 
> AliasMatch). Wouldn't we need a kind of way to figure out
> if the users wants the remainder of the URI added or not even if we do not 
> use a regular expression in the surrounding Location block?

LocationMatch is the correct way to do this - the config explicitly declares 
the exact URL to match, and the Alias gives the exact unambiguous mapping.

Having a prefix in the “Alias /foo /bar” case and then arbitrarily not having a 
prefix in the “Alias /bar” case sent me on a huge wild goose chase - what made 
it worse was the client was a webdav client, so the behaviour just made no 
sense. I am seeing people asking why they’re getting a 405 and not getting 
answers, so I think this is tripping up other people too.

Regards,
Graham
—

Re: mod_dav and PATH_INFO - MKCOL silently creates the parent directory

[Graham Leggett via dev]

On 13 Apr 2023, at 16:27, Graham Leggett  wrote:

> Changing the config to this makes it work:
> 
> Alias /storage /var/www/dav/storage
> 
> It looks like the detail that’s missing that we don’t identify the rest of 
> the URL after the alias and pass it on by calling alias_matches() to identify 
> the prefix:
> 
> https://github.com/apache/httpd/blob/trunk/modules/mappers/mod_alias.c#L549 
> <https://github.com/apache/httpd/blob/trunk/modules/mappers/mod_alias.c#L549>
This is fixed in http://svn.apache.org/viewvc?rev=1909137=rev 
<http://svn.apache.org/viewvc?rev=1909137=rev>.

Regards,
Graham
—

Re: mod_dav and PATH_INFO - MKCOL silently creates the parent directory

[Graham Leggett via dev]

On 13 Apr 2023, at 14:55, Graham Leggett via dev  wrote:

> Config looks like this:
> 
>  
>Alias /var/www/dav/storage
>Dav on
>AcceptPathInfo On
>  

Changing the config to this makes it work:

Alias /storage /var/www/dav/storage

It looks like the detail that’s missing that we don’t identify the rest of the 
URL after the alias and pass it on by calling alias_matches() to identify the 
prefix:

https://github.com/apache/httpd/blob/trunk/modules/mappers/mod_alias.c#L549

Regards,
Graham
—

mod_dav and PATH_INFO - MKCOL silently creates the parent directory

[Graham Leggett via dev]

Hi all,

I am having super fun time trying to turn on mod_dav on an httpd v2.4.53-11.el9 
(as shipped by RHEL9).

The request “MKCOL /storage/foo” returns a 201, but doesn't actually do 
anything. The “foo” directory is silently not created.

Stepping through the code, the code that creates the directory is creating the 
parent directory “/storage”, and not the actual directory in the request 
"/storage/foo”:

1137status = apr_dir_make(ctx->pathname, APR_OS_DEFAULT, ctx->pool);
(gdb) print ctx->pathname
$6 = 0x7fffbc013450 "/var/www/dav/storage"

This explains the “it’s doing nothing” - it created the parent directory, which 
already exists.

Looking at the whole request, it looks like the PATH_INFO mechanism hasn't done 
anything:

(gdb) print *r
$8 = {pool = 0x7fffb801a898, connection = 0x7fffe4033630, server = 
0x55689e28, next = 0x0, prev = 0x0, main = 0x0, 
  the_request = 0x7fffb801be48 "MKCOL /storage/foo/ HTTP/1.1", assbackwards = 
0, proxyreq = 0, header_only = 0, 
  proto_num = 1001, protocol = 0x7fffb801be3c "HTTP/1.1", hostname = 
0x7fffb801c028 “seawitch.xxx.xxx", 
  request_time = 1681392063530545, status_line = 0x0, status = 200, 
method_number = 10, method = 0x7fffb801be28 "MKCOL", 
  allowed = 67141423, allowed_xmethods = 0x0, allowed_methods = 0x7fffb801abf0, 
sent_bodyct = 0, bytes_sent = 0, mtime = 0, 
  range = 0x0, clength = 0, chunked = 0, read_body = 0, read_chunked = 0, 
expecting_100 = 0, kept_body = 0x0, body_table = 0x0, 
  remaining = 0, read_length = 0, headers_in = 0x7fffb801ac30, headers_out = 
0x7fffb801b4d0, err_headers_out = 0x7fffb801b718, 
  subprocess_env = 0x7fffb801b150, notes = 0x7fffb801ba58, content_type = 0x0, 
handler = 0x77701b49 "dav-handler", 
  content_encoding = 0x0, content_languages = 0x0, vlist_validator = 0x0, user 
= 0x0, ap_auth_type = 0x0, 
  unparsed_uri = 0x7fffb801be68 "/storage/foo/", uri = 0x7fffb801be78 
"/storage/foo/", 
  filename = 0x7fffb801c540 "/var/www/dav/storage", canonical_filename = 
0x7fffb801c540 "/var/www/dav/storage", 
  path_info = 0x7fffb801c484 "", args = 0x0, used_path_info = 0, eos_sent = 0, 
per_dir_config = 0x7fffbc013090, 
  request_config = 0x7fffb801bbf8, log = 0x55689e48, log_id = 
0x7fffb801c070 "McasmB+dDPA", htaccess = 0x0, 
  output_filters = 0x7fffb801bd28, input_filters = 0x7fffb801c000, 
proto_output_filters = 0x7fffb801bd28, 
  proto_input_filters = 0x7fffb801c000, no_cache = 0, no_local_copy = 0, 
invoke_mtx = 0x7fffb801c080, parsed_uri = {
scheme = 0x0, hostinfo = 0x0, user = 0x0, password = 0x0, hostname = 0x0, 
port_str = 0x0, 
path = 0x7fffb801be78 "/storage/foo/", query = 0x0, fragment = 0x0, hostent 
= 0x0, port = 0, is_initialized = 1, 
dns_looked_up = 0, dns_resolved = 0}, finfo = {pool = 0x7fffb801a898, valid 
= 7598960, protection = 18288, 
filetype = APR_NOFILE, user = 0, group = 48, inode = 17736438, device = 
64768, nlink = 2, size = 6, csize = 0, 
atime = 1681391795173106, mtime = 1681391869558474, ctime = 
1681391869558474, 
fname = 0x7fffb801c540 "/var/www/dav/storage", 
name = 0x77e517b1  
"H\203\304\bL\211\340]A\\\303\017\037@", filehand = 0x7fffb801c1e0}, 
  useragent_addr = 0x7fffe4033490, useragent_ip = 0x7fffe4033968 
“:::::::", 
  trailers_in = 0x7fffb801afb0, trailers_out = 0x7fffb801b8b8, useragent_host = 
0x0, double_reverse = 0, bnotes = 0}

Most specifically r->path_info is “", and r->used_path_info is 0.

Does this look familiar to anyone?

Config looks like this:

  
Alias /var/www/dav/storage
Dav on
AcceptPathInfo On
  

  
Options +Indexes

IndexOptions FancyIndexing HTMLTable VersionSort XHTML

AuthzSendForbiddenOnFailure on

SSLUserName SSL_CLIENT_CERT_RFC4523_CEA


  require valid-user


  

Regards,
Graham
—

Re: backports

[Graham Leggett]

On 08 Mar 2022, at 10:29, Joe Orton  wrote:

>> “No need to patch/compile locally" is not a good idea - currently the 
>> travis tests target Ubuntu only, and this is a practical limitation 
>> forced upon us by the nature of the Travis service. I want to see 
>> reviewers try out the patch on different architectures. I for example 
>> work on MacOS, but deploy to Redhat, so those are my two environments. 
>> Others will have different environments.
>> 
>> Our testing needs to be wide and diverse.
> 
> Definitely +1 on the sentiment and I'm keen to help with any effort to 
> add more diversity to the CI.  Travis natively supports a bunch of 
> non-Linux platforms so if someone cares about that they should be able 
> to hook it up by tweaking the .travis.yml. 
> https://docs.travis-ci.com/user/reference/overview/ 
> 

Please don’t underestimate just how much ongoing work is involved first of all 
achieving the diversity of platforms, and then keeping them working on an 
ongoing basis.

I have spent a very long time getting code to work on COPR, which targets 
Fedora/Redhat derivatives. The COPR service is excellent, but I have yet to get 
any non trivial piece of code to build on all targets. Apart from the arbitrary 
differences between platforms, there is regular instability. In some cases 
there is platform breakage (dependencies suddenly break), sometimes network 
outages, in other cases there is an inability to compile with no obvious reason.

Using Github/Travis should inform us on the status of a proposal.

Github/Travis should definitely not be invited onto our PMC and given a veto 
vote on backports and releases.

I spend a lot of time fixing bugs in open source projects, the pattern is I 
find something broken, I dive in, rip it to bits, fix the error handling, then 
fix the problem, and then submit upstream, then to distros. In some cases I 
have been waiting years for bugfixes to be picked up by projects, and it’s 
often because their CI is broken and so nobody will give the PR a second glance 
without significant prodding. In the collectd case the whole project was locked 
out.

Having one less place where “broken computer says no” would make our project 
far more welcoming.

> (I'll note the empty APLOGNO() check in question here was added because 
> some Windows build broke for that case and it blocked a release, IIRC.)

I would far rather the empty APLOGNO check was part of the build.

Vastly simpler.

Regards,
Graham
—

Re: backports

[Graham Leggett]

On 07 Mar 2022, at 11:21, Stefan Eissing  wrote:

> I'd really like, as a reviewer of backports, you can:
> - see that it passes all our tests. No need to patch/compile/test locally.

“No need to patch/compile locally" is not a good idea - currently the travis 
tests target Ubuntu only, and this is a practical limitation forced upon us by 
the nature of the Travis service. I want to see reviewers try out the patch on 
different architectures. I for example work on MacOS, but deploy to Redhat, so 
those are my two environments. Others will have different environments.

Our testing needs to be wide and diverse.

> - see the diff and comment/question on specific lines

We do that now.

I am very happy to be inclusive and allow the use of tools like Github, but I’d 
like the those comments for example gated into our mailing lists so that it is 
not required to use Github.

> - have exactly merged what you looked at

We do that now.

> This is a fair point. github has suspended accounts in the past
> based on criteria the ASF may not find agreeable. 

Going further, if we mandate Github as part of our workflow, we’re telling our 
contributors that they must accept the terms and conditions of an arbitrary 
commercial corporation that is in no way related to us, and has its own mission 
and values. It is not fair on our contributors to force this decision upon them.

> With the 2.4.* and trunk* patterns, we should run the Travis CIs for
> all matching branches. In STATUS, we link the branch/PR.

What I want[1] is to skip Github entirely and run CI off the STATUS file.

It would be far more useful to run each build as 2.4.x branch + proposed patch 
on every v2.4 change. That proposed patch can be a Github PR with no problem, 
but does not have to be.

This way we learn far more than what Github will give to us. Has my patch been 
broken by another backport ahead of it and is now stale? Very useful to know. 
It would be nice to be told “your patch is stale” in CI rather than finding 
that out when the backport is applied.

Right now (to my knowledge) Github runs CI on changes to the PR, not changes to 
the underlying branch.

[1] I am up to my eyeballs in bugs right now, so I recognise talk is cheap and 
I can’t make this happen right now. I do however run CI hardware that can be 
used for this, but will need time to make it happen.

Regards,
Graham
—

Re: backports

[Graham Leggett]

On 04 Mar 2022, at 12:08, Stefan Eissing  wrote:

> We should improve our backport procedure and provide guidance on how to use 
> it after the next release.
> 
> Post-mortem dbm backport:
> - the patch pointed to the in 2.4.x/STATUS was incomplete, lacking APLOGNO()
> - the incomplete patch was voted on and accepted, as local tests do not have 
> the full CI checks
> - Jim applied the voted on backport patch
> - CI failed
> No one did anything really wrong. We did just not apply the CI tools 
> available until the damage was done.

Just to confirm - no damage was done.

We have a thorough, multistep process that ensures damage is not done. We do 
the work first on trunk and our CI, then we propose it for backport on trunk 
and it is tested again by multiple people through their votes. Then it hits our 
stable branch and then it gets tested by even more people, and our CI. Then 
it’s tested again when we propose a release. If we slip through all that, only 
then is damage done.

> As noted in the related thread, backport proposal should really be PRs on 
> github. Those are checked by
> our CI and a PR can easily be improved by adding submits to the branch it is 
> based on. In addition, we
> get the github UI for review and comments. Should be a win for all.

Github PRs are easy to use, and it’s entirely reasonable to use github to 
handle a backport if you choose to do so.

Github PRs also have simple interfaces, like this one:

curl https://[url-of-pr].diff | patch -p1

I am however strongly opposed for Github to be our only promotion process.

CI is great right until the point you get your first unrelated test failure, 
then it is a nightmare. The collectd project was completely stuck unable to 
merge a single PR for months and months because their CI broke and nobody had 
access to fix it. Github presented a “computer says no” button and the project 
ground to a complete halt. The project is now so backlogged that the chances of 
getting anything reviewed are slim.

https://www.mail-archive.com/search?l=colle...@verplant.org=subject:%22Re%5C%3A+%5C%5Bcollectd%5C%5D+SNMPv3%5C%2BDTLS+support+for+collectd%5C-snmp%22=newest=1

It is inevitable that at some point the generosity of the people supplying the 
CI will run out, and CI will stop working. We cannot allow ourselves to be 
jammed up because of this.

To sum up:

+1 on the option to use Github PRs for backports.
-1 to mandating the use of Github PRs for backports.

Regards,
Graham
—

Re: module threads and mpm lifecycle

[Graham Leggett]

On 23 Feb 2022, at 12:39, Stefan Eissing  wrote:

> One thing that is currently missing is a way to shutdown/reap/join module 
> threads when the mpm exits.
> 
> Example: 
> 
> mod_watchdog creates several threads post_config which are only joined on 
> pool destruction.
> 
> Problem:
> 
> pool destruction is not ordered between modules and dependencies on order are 
> not
> fully known. This leads to crashes in OpenSSL, for example, when mod_ssl is 
> destructed
> before all watchdogs using OpenSSL are joined (OpenSSL 3.x seems to do better 
> on this, 
> but the point remains valid).

A note on OpenSSL termination - in old versions of OpenSSL, there was this 
wrong assumption that an application would only ever initialise OpenSSL once, 
and in turn shut down OpenSSL once, but this scenario doesn’t exist in the real 
world (like ours, where modules that have no idea about each other 
independently use OpenSSL).

OpenSSL 3 (and I think earlier versions, I don’t remember) have gone further 
than “do better”, they have (to my knowledge) 100% fixed this problem by 
turning all OpenSSL init functions into noops and using reference counting 
internally to ensure sanity is maintained at all times.

If any of this doesn't work, we need to let OpenSSL know.

> There has been attempts by Yann to make a patch that make the OpenSSL 
> termination
> way later (adding it to a 'higher' pool destruction). But that would only 
> solve
> this particular problem and not any other 3rd party dependency.
> 
> Proposal:
> 
> Add a hook 'child_exiting(int graceful)' where modules can register
> and do their own thread join/reap.
> 
> 
> On a graceful shutdown, this would allow watchdogs to fully complete any 
> ongoing task
> before things start to disappear in the process.

+1 - will be very useful.

Regards,
Graham
—

Re: mpm event assertion failures

[Graham Leggett]

On 09 Feb 2022, at 17:43, Yann Ylavic  wrote:

> So ab is (appropriately?) reporting errors for kept-alive connections
> that had to be shutdown earlier than KeepAliveTimeout, although I
> suspect that it would report the same after the KeepAliveTimeout.

ab has a code path that checks for this case and ignores the closed connection, 
but the test for this was based on whether the current connection had keepalive 
headers - which it could never be, because, the connection closed before the 
headers arrived. I added a check as to whether the connection had been kept 
alive in the past, and now the errors are all gone.

Switching from MacOS to Rawhide (on a very low powered server that is designed 
to save electricity), httpd with your PR ticks along nicely at 10 000 
concurrent requests. Will try some more testing on a Linux VM on my Macbook 
Pro, it should be quicker.

[minfrin@kingdom httpd-trunk]$ /tmp/httpd-trunk/bin/ab -R 10 -n 4096000 -c 
1 https://localhost/index.html
This is ApacheBench, Version 2.3 <$Revision: 1897912 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, 
http://web.archive.org/web/2304112933/http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient)
^C

Server Software:Apache/2.5.1-dev
Server Hostname:localhost
Server Port:443
SSL/TLS Protocol:   TLSv1.2,ECDHE-RSA-AES256-GCM-SHA384,2048,256
Server Temp Key:X25519 253 bits
TLS Server Name:localhost

Document Path:  /index.html
Document Length:191 bytes

Concurrency Level:  1
Concurrency achieved:   1
Rampup delay:   10 [ms]
Time taken for tests:   391.854 seconds
Complete requests:  136274
Failed requests:0
Total transferred:  58427992 bytes
HTML transferred:   26074174 bytes
Requests per second:347.77 [#/sec] (mean)
Time per request:   28754.873 [ms] (mean)
Time per request:   2.875 [ms] (mean, across all concurrent requests)
Transfer rate:  145.61 [Kbytes/sec] received

Connection Times (ms)
  min  mean[+/-sd] median   max
Connect:   16 15427 5534.2  16315   33304
Processing: 5 8640 2503.2   9722   11456
Waiting:1 4313 1306.0   48906407
Total: 21 24067 7782.9  26160   41561

Percentage of the requests served within a certain time (ms)
  50%  26160
  66%  26394
  75%  26633
  80%  31010
  90%  31586
  95%  31837
  98%  32071
  99%  32326
 100%  41561 (longest request)

Regards,
Graham
—

Re: mpm event assertion failures

[Graham Leggett]

On 09 Feb 2022, at 15:08, Ruediger Pluem  wrote:

> Probably you get hit by this?
> 
> https://github.com/apache/httpd/blob/46a9db4c6f9fadca4e362872021fa62a37908ece/server/mpm/event/event.c#L2096-L2101
> 
> If a client sends a request on a keepalive connection it should be prepared 
> that the connection gets closed without a reply and it
> should retry if the request is idempotent. This is how I read RFC7230 6.3.1:
> 
> https://datatracker.ietf.org/doc/html/rfc7230#section-6.3.1

This seems to be it - ab was trying to test whether the current request was 
keepalive, which it could never be given the request had been closed before 
read. The fix is to keep track of whether this connection has ever been kept 
alive, and check that instead.

Fixed in r1897912.

Regards,
Graham
—

Re: mpm event assertion failures

[Graham Leggett]

On 08 Feb 2022, at 19:01, Yann Ylavic  wrote:

>> The most concurrent requests I can get out of MacOS appears to be roughly 
>> 2000 concurrent requests before “connection reset by peer” kills the test.
>> 
>> One small detail I have uncovered is that a small percentage of requests 
>> fail, caused by httpd gracefully shutting the connection down before 
>> returning a response. Stopping ab in a debugger confirms that openssl 
>> returns SSL_ERROR_ZERO_RETURN on read, which is what openssl does when the 
>> other side gracefully triggers close notify.
>> 
>> Is this something anyone has seen before while I go digging?
> 
> I tried with ab too and indeed there was some (dead-)locking issue at
> some point, could you please retry with the latest pushes I just made
> to the PR?

I tried with the latest patch, then I tried with no patch (vanilla trunk) and 
got the same results.

Working backwards looking for graceful attempts to shut down the SSL 
connection, I found this:

::1:56237 ::1:56237 [Wed Feb 09 09:52:21 2022] [info] [pid 8747] 
mod_ssl.c(681): [client ::1:56237] AH01964: Connection to child 29 established 
(server localhost:443)
::1:56237 ::1:56237 [Wed Feb 09 09:52:21 2022] [debug] [pid 8747] 
ssl_engine_kernel.c(2402): [client ::1:56237] AH02043: SSL virtual host for 
servername localhost found
::1:56237 ::1:56237 [Wed Feb 09 09:52:21 2022] [debug] [pid 8747] 
ssl_engine_kernel.c(2262): [client ::1:56237] AH02041: Protocol: TLSv1.2, 
Cipher: ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)
::1:56237 ::1:56237 [Wed Feb 09 09:52:21 2022] [debug] [pid 8747] 
ssl_engine_kernel.c(415): [client ::1:56237] AH02034: Initial (No.1) HTTPS 
request received for child 29 (server localhost:443)
::1:56237 ::1:56237 [Wed Feb 09 09:52:21 2022] [debug] [pid 8747] 
mod_authz_core.c(843): [client ::1:56237] AH01628: authorization result: 
granted (no directives)
::1:56237 ::1:56237 [Wed Feb 09 09:52:21 2022] [debug] [pid 8747] 
ssl_engine_kernel.c(415): [client ::1:56237] AH02034: Subsequent (No.2) HTTPS 
request received for child 27 (server localhost:443)
::1:56237 ::1:56237 [Wed Feb 09 09:52:21 2022] [debug] [pid 8747] 
mod_authz_core.c(843): [client ::1:56237] AH01628: authorization result: 
granted (no directives)
::1:56237 ::1:56237 [Wed Feb 09 09:52:21 2022] [debug] [pid 8747] 
ssl_engine_kernel.c(415): [client ::1:56237] AH02034: Subsequent (No.3) HTTPS 
request received for child 37 (server localhost:443)
::1:56237 ::1:56237 [Wed Feb 09 09:52:21 2022] [debug] [pid 8747] 
mod_authz_core.c(843): [client ::1:56237] AH01628: authorization result: 
granted (no directives)
::1:56237 ::1:56237 [Wed Feb 09 09:52:21 2022] [debug] [pid 8747] 
ssl_engine_io.c(1184): [client ::1:56237] AH02001: Connection closed to child 
29 with standard shutdown (server localhost:443)

It seems reasonably normal, except what looks like is happening is when 
"Connection closed to child 29 with standard shutdown” is run, this seems to 
happen before a response is sent to the client.

ab then goes “that’s weird, I was trying to read a response, but got a 
connection closed instead”, and marks that request as failed.

This is limited to keepalive requests, and happens after a number of keepalive 
requests have been correctly handled.

Regards,
Graham
—

Re: mpm event assertion failures

[Graham Leggett]

On 07 Feb 2022, at 18:13, Yann Ylavic  wrote:

> All green: https://app.travis-ci.com/github/apache/httpd/builds/246021482 
>   \o/

I’ve been load testing these changes with the fixed trunk version of ab, most 
recently modified at have a ramp delay to stop the OS kicking in and protecting 
itself from denial of service. The most concurrent requests I can get out of 
MacOS appears to be roughly 2000 concurrent requests before “connection reset 
by peer” kills the test.

One small detail I have uncovered is that a small percentage of requests fail, 
caused by httpd gracefully shutting the connection down before returning a 
response. Stopping ab in a debugger confirms that openssl returns 
SSL_ERROR_ZERO_RETURN on read, which is what openssl does when the other side 
gracefully triggers close notify.

Is this something anyone has seen before while I go digging?

Little-Net:~ root# ulimit -n 10240; /tmp/httpd-trunk/bin/ab -k -R 20 -n 4096000 
-c 4096 https://localhost:9443/blah.txt
This is ApacheBench, Version 2.3 <$Revision: 1897866 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, 
http://web.archive.org/web/2304112933/http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient)
Completed 409600 requests

Test aborted after 10 failures

apr_socket_connect(): Connection reset by peer (54)
Total of 498014 requests completed


Server Software:Apache/2.5.1-dev
Server Hostname:localhost
Server Port:9443
SSL/TLS Protocol:   TLSv1.2,ECDHE-RSA-AES256-GCM-SHA384,2048,256
Server Temp Key:X25519 253 bits
TLS Server Name:localhost

Document Path:  /blah.txt
Document Length:9 bytes

Concurrency Level:  4096
Concurrency achieved:   2028
Rampup delay:   20 [ms]
Time taken for tests:   40.629 seconds
Complete requests:  498014
Failed requests:13102
   (Connect: 1, Receive: 0, Length: 13101, Exceptions: 0)
Keep-Alive requests:482963
Total transferred:  135219777 bytes
HTML transferred:   4364253 bytes
Requests per second:12257.69 [#/sec] (mean)
Time per request:   334.158 [ms] (mean)
Time per request:   0.082 [ms] (mean, across all concurrent requests)
Transfer rate:  3250.18 [Kbytes/sec] received

Connection Times (ms)
  min  mean[+/-sd] median   max
Connect:0   10  60.8  0 847
Processing: 0   73  70.5 54 611
Waiting:0   70  69.9 52 487
Total:  0   82  96.4 561192

Percentage of the requests served within a certain time (ms)
  50% 56
  66% 91
  75%109
  80%125
  90%171
  95%224
  98%410
  99%485
 100%   1192 (longest request)

Regards,
Graham
—

Re: Python test suite - failures on Fedora Rawhide / MacOS

[Graham Leggett]

On 07 Feb 2022, at 12:42, Stefan Eissing  wrote:

>> curl -V
> curl 7.77.0 (x86_64-apple-darwin21.0) libcurl/7.77.0 (SecureTransport) 
> LibreSSL/2.8.3 zlib/1.2.11 nghttp2/1.42.0
> Release-Date: 2021-05-26
> Protocols: dict file ftp ftps gopher gophers http https imap imaps ldap ldaps 
> mqtt pop3 pop3s rtsp smb smbs smtp smtps telnet tftp
> Features: alt-svc AsynchDNS GSS-API HSTS HTTP2 HTTPS-proxy IPv6 Kerberos 
> Largefile libz MultiSSL NTLM NTLM_WB SPNEGO SSL UnixSockets
> 
> It should list "HTTP2" in the Features line.

I don’t have HTTP2:

Little-Net:httpd-trunk6 minfrin$ which curl
/opt/local/bin/curl
Little-Net:httpd-trunk6 minfrin$ curl -V
curl 7.80.0 (x86_64-apple-darwin20.6.0) libcurl/7.80.0 OpenSSL/3.0.1 
zlib/1.2.11 zstd/1.5.2 libidn2/2.3.2 libpsl/0.21.1 (+libidn2/2.3.2)
Release-Date: 2021-11-10
Protocols: dict file ftp ftps gopher gophers http https imap imaps mqtt pop3 
pop3s rtsp smb smbs smtp smtps telnet tftp 
Features: alt-svc AsynchDNS HSTS HTTPS-proxy IDN IPv6 Largefile libz NTLM 
NTLM_WB PSL SSL TLS-SRP UnixSockets zstd

Looks like you need to explicitly request it:

Little-Net:httpd-trunk6 minfrin$ sudo port install curl +http2
Password:
Warning: The macOS 11 SDK does not appear to be installed. Ports may not build 
correctly.
Warning: You can install it as part of the Xcode Command Line Tools package by 
running `xcode-select --install'.
--->  Computing dependencies for curl
--->  Fetching archive for curl
--->  Attempting to fetch curl-7.80.0_0+http2+ssl.darwin_20.x86_64.tbz2 from 
https://mse.uk.packages.macports.org/curl
--->  Attempting to fetch curl-7.80.0_0+http2+ssl.darwin_20.x86_64.tbz2 from 
https://packages.macports.org/curl
--->  Attempting to fetch curl-7.80.0_0+http2+ssl.darwin_20.x86_64.tbz2 from 
https://ema.uk.packages.macports.org/curl
--->  Fetching distfiles for curl
--->  Attempting to fetch curl-7.80.0.tar.xz from 
https://distfiles.macports.org/curl
--->  Verifying checksums for curl  
 
--->  Extracting curl
--->  Applying patches to curl
--->  Configuring curl
Warning: Configuration logfiles contain indications of 
-Wimplicit-function-declaration; check that features were not accidentally 
disabled:
  getpass_r: found in curl-7.80.0/config.log
  malloc: found in curl-7.80.0/config.log
  strchr: found in curl-7.80.0/config.log
  memrchr: found in curl-7.80.0/config.log
  free: found in curl-7.80.0/config.log
  calloc: found in curl-7.80.0/config.log
  CloseSocket: found in curl-7.80.0/config.log
  closesocket: found in curl-7.80.0/config.log
--->  Building curl
--->  Staging curl into destroot 
--->  Installing curl @7.80.0_0+http2+ssl
--->  Deactivating curl @7.80.0_0+ssl
--->  Cleaning curl
--->  Activating curl @7.80.0_0+http2+ssl
--->  Cleaning curl
--->  Scanning binaries for linking errors
--->  No broken files found. 
--->  No broken ports found.

I suspect we need tests at the very beginning of the test suite to fail if the 
required tools are missing / incomplete.

Regards,
Graham
—

Re: mpm event assertion failures

[Graham Leggett]

On 07 Feb 2022, at 12:35, Stefan Eissing  wrote:

>> There are two parts that hook into the process_connection hook, the code 
>> you’ve cited above, and this code:
>> 
>> void h2_c2_register_hooks(void)
>> {
>>/* When the connection processing actually starts, we might
>> * take over, if the connection is for a h2 stream.
>> */
>>ap_hook_process_connection(h2_c2_hook_process,
>>   NULL, NULL, APR_HOOK_FIRST);
>> 
>> Looks like this code is running before mod_ssl somehow.
>> 
>> Is there a way to run the httpd under test in either lldb or gdb?
> 
> The names "h2_c1..." and "h2_c2..." indicate, that the former operates on 
> "primary" connections, e.g. the ones from a client where SSL is applied, and 
> "secondary" connections (c->master != NULL), e.g. where h2 requests are 
> processed and SSL is not involved.

In theory it’s all the same hook though, so even though h2_c2_hook_process() is 
standing out of the way of anything not a secondary connection, it’s still 
running before mod_ssl, and it still throws away the AGAIN, leaving mod_ssl in 
an undefined state.

I’m assuming h2_c2_hook_process just wants to run before 
h2_c1_hook_process_connection?

I just tried this patch on rawhide (where I’m getting success with the http2 
tests) and we still work:

Index: modules/http2/h2_c1.c
===
--- modules/http2/h2_c1.c   (revision 1897807)
+++ modules/http2/h2_c1.c   (working copy)
@@ -312,6 +312,7 @@
 
 static const char* const mod_ssl[]= { "mod_ssl.c", NULL};
 static const char* const mod_reqtimeout[] = { "mod_ssl.c", "mod_reqtimeout.c", 
NULL};
+static const char* const mod_h2_c2[]  = { "mod_ssl.c", "mod_reqtimeout.c", 
"h2_c2.c", NULL};
 
 void h2_c1_register_hooks(void)
 {
@@ -322,7 +323,7 @@
  * a chance to take over before it.
  */
 ap_hook_process_connection(h2_c1_hook_process_connection,
-   mod_reqtimeout, NULL, APR_HOOK_LAST);
+   mod_h2_c2, NULL, APR_HOOK_LAST);
 
 /* One last chance to properly say goodbye if we have not done so
  * already. */
Index: modules/http2/h2_c2.c
===
--- modules/http2/h2_c2.c   (revision 1897807)
+++ modules/http2/h2_c2.c   (working copy)
@@ -707,7 +707,7 @@
  * take over, if the connection is for a h2 stream.
  */
 ap_hook_process_connection(h2_c2_hook_process,
-   NULL, NULL, APR_HOOK_FIRST);
+   NULL, NULL, APR_HOOK_LAST);
 /* We need to manipulate the standard HTTP/1.1 protocol filters and
  * install our own. This needs to be done very early. */
 ap_hook_post_read_request(h2_c2_hook_post_read_request, NULL, NULL, 
APR_HOOK_REALLY_FIRST);

Regards,
Graham
—

Re: mpm event assertion failures

[Graham Leggett]

On 07 Feb 2022, at 12:18, Stefan Eissing  wrote:

>>> Is the http2 code doing anything to work around mod_ssl trying to read, 
>>> failing, throwing away the error, and then pretending nothing happened?
>> 
>> The http2 code doesn't try to work around mod_ssl, instead it does the same 
>> as mod_ssl, which is that it performs an operation but throws away the error:
>> 
>> https://github.com/apache/httpd/blob/1598f7aebd59acc7494389a3903a33c5e38a5460/modules/http2/h2_c2.c#L632
>> 
>> This hook is sorted APR_HOOK_FIRST, which means h2_c2_hook_process() runs 
>> first, then passes the connection to mod_ssl, which then returns AGAIN, but 
>> AGAIN is now thrown away inside h2_c2_hook_process(), and now we’re off the 
>> rails from this point on and we can expect our socket to be stuck in a weird 
>> state.
>> 
>> The http1 code has the same problem, but runs APR_HOOK_LAST, which means 
>> mod_ssl’s handshake is completely done before http1 touches the connection.
>> 
>> I think to fix this test, we need to make sure http2 runs after mod_ssl.
> 
> It does.
> 
> h2_c1.c:
> 
> static const char* const mod_reqtimeout[] = { "mod_ssl.c", 
> "mod_reqtimeout.c", NULL};
> 
> void h2_c1_register_hooks(void)
> {
>/* Our main processing needs to run quite late. Definitely after mod_ssl,
> * as we need its connection filters, but also before reqtimeout as its
> * method of timeouts is specific to HTTP/1.1 (as of now).
> * The core HTTP/1 processing run as REALLY_LAST, so we will have
> * a chance to take over before it.
> */
>ap_hook_process_connection(h2_c1_hook_process_connection,
>   mod_reqtimeout, NULL, APR_HOOK_LAST);
>...
> 
> 
> In my understanding this is how it should work. When mod_ssl's 
> process_connection fails, it should return OK to prevent further hooks to run.
> 
> AP_IMPLEMENT_HOOK_RUN_FIRST(int,process_connection,(conn_rec *c),(c),DECLINED)
> 
> process_connection is a RUN_FIRST. So the first hook that returns OK, 
> terminates it.
> 
> If mod_ssl returns OK in case of handshake errors, the connection is done. 
> That is my read of it.

There are two parts that hook into the process_connection hook, the code you’ve 
cited above, and this code:

void h2_c2_register_hooks(void)
{
/* When the connection processing actually starts, we might
 * take over, if the connection is for a h2 stream.
 */
ap_hook_process_connection(h2_c2_hook_process,
   NULL, NULL, APR_HOOK_FIRST);

Looks like this code is running before mod_ssl somehow.

Is there a way to run the httpd under test in either lldb or gdb?

Regards,
Graham
—

Re: Python test suite - failures on Fedora Rawhide / MacOS

[Graham Leggett]

On 07 Feb 2022, at 12:23, Stefan Eissing  wrote:

> could you attach a complete failed output of your pytest run? I use it on 
> MacOS all the time and
> it seems most likely that some prerequisites have not been documented well 
> enough or checks must
> add more information in output.

I reduced the tests to http2 only:

pytest test/modules/http2

and got this as a short summary:

 short test summary 
info =
FAILED 
test/modules/http2/test_002_curl_basics.py::TestCurlBasics::test_h2_002_04 - 
AssertionError: assert 'HTTP/1.1' == 'HTTP/2'
FAILED 
test/modules/http2/test_002_curl_basics.py::TestCurlBasics::test_h2_002_04b - 
AssertionError: assert 'HTTP/1.1' == 'HTTP/2'
FAILED test/modules/http2/test_003_get.py::TestGet::test_h2_003_01 - 
AssertionError: assert 'HTTP/1.1' == 'HTTP/2.0'
FAILED test/modules/http2/test_003_get.py::TestGet::test_h2_003_02 - 
AssertionError: assert 'HTTP/2' == 'HTTP/1.1'
FAILED test/modules/http2/test_003_get.py::TestGet::test_h2_003_21 - 
AssertionError: assert 'HTTP/2' == 'HTTP/1.1'
FAILED test/modules/http2/test_003_get.py::TestGet::test_h2_003_30[/004.html] - 
AssertionError: assert 'HTTP/2' == 'HTTP/1.1'
FAILED 
test/modules/http2/test_003_get.py::TestGet::test_h2_003_30[/proxy/004.html] - 
AssertionError: assert 'HTTP/2' == 'HTTP/1.1'
FAILED 
test/modules/http2/test_003_get.py::TestGet::test_h2_003_30[/h2proxy/004.html] 
- AssertionError: assert 'HTTP/2' == 'HTTP/1.1'
FAILED test/modules/http2/test_003_get.py::TestGet::test_h2_003_31[/004.html] - 
AssertionError: assert 'HTTP/2' == 'HTTP/1.1'
FAILED 
test/modules/http2/test_003_get.py::TestGet::test_h2_003_31[/proxy/004.html] - 
AssertionError: assert 'HTTP/2' == 'HTTP/1.1'
FAILED 
test/modules/http2/test_003_get.py::TestGet::test_h2_003_31[/h2proxy/004.html] 
- AssertionError: assert 'HTTP/2' == 'HTTP/1.1'
FAILED test/modules/http2/test_003_get.py::TestGet::test_h2_003_40 - 
AssertionError: assert 'HTTP/2' == 'HTTP/1.1'
FAILED test/modules/http2/test_003_get.py::TestGet::test_h2_003_41[0] - 
AssertionError: assert 'HTTP/2' == 'HTTP/1.1'
FAILED test/modules/http2/test_003_get.py::TestGet::test_h2_003_41[1] - 
AssertionError: assert 'HTTP/2' == 'HTTP/1.1'
FAILED test/modules/http2/test_003_get.py::TestGet::test_h2_003_41[1291] - 
AssertionError: assert 'HTTP/2' == 'HTTP/1.1'
FAILED test/modules/http2/test_003_get.py::TestGet::test_h2_003_41[1292] - 
AssertionError: assert 'HTTP/2' == 'HTTP/1.1'
FAILED test/modules/http2/test_003_get.py::TestGet::test_h2_003_41[8] - 
AssertionError: assert 'HTTP/2' == 'HTTP/1.1'
FAILED test/modules/http2/test_003_get.py::TestGet::test_h2_003_41[80123] - 
AssertionError: assert 'HTTP/2' == 'HTTP/1.1'
FAILED test/modules/http2/test_003_get.py::TestGet::test_h2_003_41[81087] - 
AssertionError: assert 'HTTP/2' == 'HTTP/1.1'
FAILED test/modules/http2/test_003_get.py::TestGet::test_h2_003_41[98452] - 
AssertionError: assert 'HTTP/2' == 'HTTP/1.1'
FAILED test/modules/http2/test_003_get.py::TestGet::test_h2_003_50[/004.html] - 
AssertionError: assert 'HTTP/2' == 'HTTP/1.1'
FAILED 
test/modules/http2/test_003_get.py::TestGet::test_h2_003_50[/proxy/004.html] - 
AssertionError: assert 'HTTP/2' == 'HTTP/1.1'
FAILED 
test/modules/http2/test_003_get.py::TestGet::test_h2_003_50[/h2proxy/004.html] 
- AssertionError: assert 'HTTP/2' == 'HTTP/1.1'
FAILED test/modules/http2/test_004_post.py::TestPost::test_h2_004_07[HTTP2-on] 
- AssertionError: assert None
FAILED 
test/modules/http2/test_004_post.py::TestPost::test_h2_004_07[H2PUSH-off] - 
AssertionError: assert None
FAILED 
test/modules/http2/test_004_post.py::TestPost::test_h2_004_07[H2_STREAM_ID-1] - 
AssertionError: assert None
FAILED 
test/modules/http2/test_004_post.py::TestPost::test_h2_004_07[H2_STREAM_TAG-\\d+-1]
 - AssertionError: assert None
FAILED test/modules/http2/test_100_conn_reuse.py::TestConnReuse::test_h2_100_01 
- AssertionError: assert '2' == '1.1'
FAILED test/modules/http2/test_100_conn_reuse.py::TestConnReuse::test_h2_100_02 
- AssertionError: assert 'HTTP/2' == 'HTTP/1.1'
FAILED test/modules/http2/test_100_conn_reuse.py::TestConnReuse::test_h2_100_03 
- AssertionError: assert 'HTTP/2' == 'HTTP/1.1'
FAILED 
test/modules/http2/test_101_ssl_reneg.py::TestSslRenegotiation::test_h2_101_02 
- AssertionError: assert None
FAILED 
test/modules/http2/test_101_ssl_reneg.py::TestSslRenegotiation::test_h2_101_03 
- AssertionError: assert None
FAILED 
test/modules/http2/test_101_ssl_reneg.py::TestSslRenegotiation::test_h2_101_04 
- AssertionError: assert None
FAILED 
test/modules/http2/test_101_ssl_reneg.py::TestSslRenegotiation::test_h2_101_11 
- AssertionError: assert None
FAILED test/modules/http2/test_102_require.py::TestRequire::test_h2_102_01 - 
assert 404 == 403
FAILED test/modules/http2/test_102_require.py::TestRequire::test_h2_102_02 - 
assert 403 == 404
FAILED 

Python test suite - failures on Fedora Rawhide / MacOS

[Graham Leggett]

Hi all,

I am not having any luck getting the python test suite to run on either MacOS 
or Rawhide.

In the case of MacOS I see this:

== 50 failed, 214 passed, 239 skipped, 195 
errors in 282.97s (0:04:42) ===

A selection of errors:

E   AssertionError: received response not in 3 chunks, but 4

E   AssertionError: assert 'keep-alive' not in {'accept-ranges': 'bytes', 
'connection': 'Upgrade, Keep-Alive', 'content-length': '543', 'content-type': 
'text/html', …}

E   assert 431 == 400

E   AssertionError: assert 'HTTP/2' == 'HTTP/1.1'
E - HTTP/1.1
E + HTTP/2

E   AssertionError: assert None
E+  where None = ('HTTP_1_1_REQUIRED 
\\(err 13\\)', '* Added ssl.tests.httpd.apache.org:5001:127.0.0.1 to DNS 
cache\n* Hostname ssl.tests.httpd.apache.org was found in DN...ta]\n* OpenSSL 
SSL_read: error:0A000410:SSL routines::sslv3 alert handshake failure, errno 
0\n* Closing connection 0\n')
E+where  = re.search
E+and   '* Added ssl.tests.httpd.apache.org:5001:127.0.0.1 to DNS 
cache\n* Hostname ssl.tests.httpd.apache.org was found in DN...ta]\n* OpenSSL 
SSL_read: error:0A000410:SSL routines::sslv3 alert handshake failure, errno 
0\n* Closing connection 0\n' = ExecResult[code=56, args=['curl', '-s', 
'--path-as-is', '-D', 
'/Users/minfrin/src/apache/sandbox/httpd/httpd-trunk6/te... data]\n* OpenSSL 
SSL_read: error:0A000410:SSL routines::sslv3 alert handshake failure, errno 
0\n* Closing connection 0\n].stderr

In the case of Rawhide we have this:

= 3 failed, 262 passed, 238 
skipped, 195 errors in 539.82s (0:08:59) 
==

E   FileNotFoundError: [Errno 2] No such file or directory: 
‘openssl'

E   FileNotFoundError: [Errno 2] No such file or directory: ‘pebble'

Has anyone else had any luck?

Regards,
Graham
—

Re: svn commit: r1897458 - in /httpd/httpd/trunk: changes-entries/ab-ssl-sense-fix.txt support/ab.c

[Graham Leggett]

On 27 Jan 2022, at 09:53, Ruediger Pluem  wrote:

>> Modified: httpd/httpd/trunk/support/ab.c
>> URL: 
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/support/ab.c?rev=1897458=1897457=1897458=diff
>>  
>> 
>> ==
>> --- httpd/httpd/trunk/support/ab.c (original)
>> +++ httpd/httpd/trunk/support/ab.c Tue Jan 25 15:54:22 2022
> 
>> @@ -810,9 +811,6 @@ static void ssl_proceed_handshake(struct
>> 
>> static void write_request(struct connection * c)
>> {
>> -if (started >= requests) {
>> -return;
>> -}
> 
> Why is this no longer needed?

It’s in the wrong place, this has been moved one level up. 

>> do {
>> apr_time_t tnow;
> 
>> @@ -1461,7 +1465,6 @@ static void start_connect(struct connect
>> }
>> 
>> /* connected first time */
>> -set_conn_state(c, STATE_CONNECTED);
> 
> Why don't we set the state to connected any longer?
> 
>> #ifdef USE_SSL
>> if (c->ssl) {
>> ssl_proceed_handshake(c);

…because directly after being set, ssl_proceed_handshake() or read_connection() 
sets the state to something else.

Part of the confusion is that these states represent how the code needs to 
react after the poll. It seems in a number of places they were being set 
arbitrarily where it didn’t make sense.

>> @@ -1786,7 +1799,7 @@ read_more:
>> c->read = c->bread = 0;
>> /* zero connect time with keep-alive */
>> c->start = c->connect = lasttime = apr_time_now();
>> -set_conn_state(c, STATE_CONNECTED);
> 
> Why don't we set the state to connected any longer?
> 
>> +
>> write_request(c);

Again, directly after being set, write_request() sets it to something else.

>> }
>> }
> 
>> @@ -2048,7 +2077,7 @@ static void test(void)
>> continue;
>> }
>> else {
>> -set_conn_state(c, STATE_CONNECTED);
> 
> Why don't we set the state to connected any longer?
> 
>> +
>> #ifdef USE_SSL
>> if (c->ssl)
>> ssl_proceed_handshake(c);

Same reason as above.

Regards,
Graham
—

Re: mpm event assertion failures

[Graham Leggett]

On 25 Jan 2022, at 23:25, Graham Leggett  wrote:

> Is the http2 code doing anything to work around mod_ssl trying to read, 
> failing, throwing away the error, and then pretending nothing happened?

The http2 code doesn't try to work around mod_ssl, instead it does the same as 
mod_ssl, which is that it performs an operation but throws away the error:

https://github.com/apache/httpd/blob/1598f7aebd59acc7494389a3903a33c5e38a5460/modules/http2/h2_c2.c#L632

This hook is sorted APR_HOOK_FIRST, which means h2_c2_hook_process() runs 
first, then passes the connection to mod_ssl, which then returns AGAIN, but 
AGAIN is now thrown away inside h2_c2_hook_process(), and now we’re off the 
rails from this point on and we can expect our socket to be stuck in a weird 
state.

The http1 code has the same problem, but runs APR_HOOK_LAST, which means 
mod_ssl’s handshake is completely done before http1 touches the connection.

I think to fix this test, we need to make sure http2 runs after mod_ssl.

Regards,
Graham
—

Re: SSL handshake nonblocking as PR

[Graham Leggett]

On 04 Feb 2022, at 14:49, Stefan Eissing  wrote:

> https://github.com/apache/httpd/pull/293
> 
> is the PR that contains the changes I just reverted 
> in trunk regarding the non-blocking SSL handshake.
> 
> I did not like to revert a set of changes by anyone
> here. But our trunk CI is failing for some time now
> and I felt this needs to be analyzed without interfering
> with other ongoing work.

Unfortunately this doesn't appear to be a clean revert, as a whole lot of 
unrelated commits seem to have got caught up in things.

All the bugfixes made to ab for example are now gone:

Little-Net:httpd-trunk6 minfrin$ svn log support/ab.c
r1897760 | icing | 2022-02-04 14:22:26 +0200 (Fri, 04 Feb 2022) | 6 lines

  *) core/mod_ssl/mpm_event: reverting changes to nonblocing SSL handshakes
 to stabilize CI tests again. Previous revision of trunk has been copied
 to branches/trunk-ssl-handshake-unblocking to make those into a PR where
 changes can be discussed and tested separately.

You’ve also reported conflicts while reverting, I’m assuming they weren't 
easily explained like log tags and mmn bumps?

I’m stuck at this point, what do I do to move forward?

Regards,
Graham
—

Re: mpm event assertion failures

[Graham Leggett]

On 26 Jan 2022, at 13:49, Stefan Eissing  wrote:

> Guys, we have changes in a central part of the server and our CI fails.=20=
> 
> It is not good enough to give other people unspecific homework to fix =
> it.=20
> 
> Analyze what you broke and if we can help, we'll happily do that. But
> you need to give some more details here.

We need to clarify expectations at this point.

The trunk of httpd has a policy called “commit then review” (CTR), meaning that 
changes are applied first, and then review is done to see what the 
ramifications of those changes are. Some changes are at a high level and very 
well contained, some changes such as this one are at a very low level and 
affect the whole server. Obviously there is an expectation that one must think 
it works before committing, but none of our contributors have access to even a 
fraction of the number of platforms that httpd runs on, and so we must rely on 
both our CI and the review of others (thus the “then review”) to show us where 
things have gone wrong. Our CI is a tool to tell us what potentially has gone 
wrong across a wide set of scenarios, far beyond the capability of what a 
single person has access to.

The next issue is “Analyze what you broke”.

I have been working on this code morning, day and night for many days now. A 
lot of time was spent chasing what I thought was an infinite loop complaining 
about EOF, but actually was a harmless error that should have been a debug 
triggered every time the client disconnected. Then more time was spent trying 
to get to the bottom of why the timeouts weren’t working, only to discover that 
the timeouts weren’t implemented. The accusation that I have been careless is 
highly offensive.

In open source we don’t bark accusations at each other, particularly when noone 
has seen just how much time and effort has gone into this. I have been working 
on this code for 25 years and am not afraid to call this out, but new people to 
open source or new to this project are going to be intimidated and leave. This 
must not happen.

Please be mindful of others working on this project, and be helpful where 
possible.

Regards,
Graham
—

Re: mpm event assertion failures

[Graham Leggett]

On 25 Jan 2022, at 14:11, Stefan Eissing  wrote:

> Also, while running the http2 test suite, I get repeated assert failures in 
> event.c:1230
> 
> if (rv != APR_SUCCESS && !APR_STATUS_IS_EEXIST(rv)) {
> ->  AP_DEBUG_ASSERT(0);
>TO_QUEUE_REMOVE(cs->sc->wc_q, cs);
>apr_thread_mutex_unlock(timeout_mutex);
>ap_log_error(APLOG_MARK, APLOG_ERR, rv, ap_server_conf, APLOGNO(03465)
> "process_socket: apr_pollset_add failure for "
> "write completion");
>close_connection(cs);
>signal_threads(ST_GRACEFUL);
> }

One line above is this:

rv = apr_pollset_add(event_pollset, >pfd);

and looking up the log we fail with a bad file descriptor.

This looks like it’s being triggered by this:

https://github.com/apache/apr/blob/trunk/poll/unix/poll.c#L194

which in turn looks like an apr_file_t is being added to the connection. Does 
that seem familiar?

To explain what’s changed in mod_ssl, a bug has been fixed. In the old mod_ssl, 
directly after connecting we make one single attempt to read zero bytes in a 
blocking connection. OpenSSL kicks in and performs reads and writes to finish 
the handshake. And then if that failed for whatever reason - brokenly - we 
throw the errors away and pretend nothing happened.

https://github.com/apache/httpd/blob/2.4.x/modules/ssl/mod_ssl.c#L695

We then get into the protocol handler, and in http/1.1 we read the now broken 
connection a second time here:

https://github.com/apache/httpd/blob/2.4.x/modules/http/http_core.c#L148

Is the http2 code doing anything to work around mod_ssl trying to read, 
failing, throwing away the error, and then pretending nothing happened? To fix 
this behaviour, I suspect whatever you needed to do to work around the mod_ssl 
bug needs to be undone.

Regards,
Graham
—

Re: trunk test failure, SSL handshake

[Graham Leggett]

On 25 Jan 2022, at 13:57, Stefan Eissing  wrote:

> Failure in https://app.travis-ci.com/github/apache/httpd/jobs/556778281
> indicates that the SSL handshake timeout no longer is working.
> Also logs mpm:error several times.
> 
> The test opens a socket and sends one byte, then waits for
> the connection to close with the configured timeout.

Alas there is no SSL handshake timeout, but rather a timeout implemented by 
mod_reqtimeout, which in turn steps out of the way as soon as the connection is 
non blocking. Problem is on this line:

https://github.com/apache/httpd/blob/trunk/modules/filters/mod_reqtimeout.c#L220

mod_reqtimeout needs to be taught how to not-block.

Regards,
Graham
—

Re: http and http/1.x separation

[Graham Leggett]

On 24 Jan 2022, at 16:53, Stefan Eissing  wrote:

> Yes, it's 0 length like ERROR and has a struct as that holds:
> - int status, opt 0 
> - const char *reason, opt NULL
> - apr_table_t *headers
> - apr_table_t *notes
> 
> on responses, shallow copied from the request_rec, etc.
> 
> The nice thing is that the HTTP/1.x transcode out filter can
> now check in one place that
> - there is indeed a HEADERS before data buckets
> - there is only one HEADERS with status >= 200
> 
> And "above" the transcode layer, a data bucket is content, period.

I have a module that parses multipart/mime headers and puts them into a 
metadata bucket, that has a lot of similarity to what you’re after:

https://github.com/minfrin/mod_multipart/blob/main/mod_multipart.c#L514

Some of the functions above would be useful in httpd.

Regards,
Graham
—

Re: svn commit: r1897281 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/scoreboard.h modules/generators/mod_status.c modules/ssl/mod_ssl.c modules/ssl/ssl_engine_io.c server/mpm/event/event

[Graham Leggett]

On 24 Jan 2022, at 19:39, Yann Ylavic  wrote:

 --- httpd/httpd/trunk/server/mpm/event/event.c (original)
 +++ httpd/httpd/trunk/server/mpm/event/event.c Fri Jan 21 00:09:24 2022
 @@ -1142,6 +1145,7 @@ read_request:
 */
if (rc != OK || (cs->pub.state >= CONN_STATE_NUM)
 || (cs->pub.state < CONN_STATE_LINGER
 + && cs->pub.state != CONN_STATE_READ_REQUEST_LINE
 && cs->pub.state != CONN_STATE_WRITE_COMPLETION
 && cs->pub.state != 
 CONN_STATE_CHECK_REQUEST_LINE_READABLE
 && cs->pub.state != CONN_STATE_SUSPENDED)) {
>>> 
>>> The issue is that we've never asked process_connection hooks that
>>> don't care about async and co to change cs->pub.state when they are
>>> done, and for instance mod_echo's process_echo_connection() will
>>> consume the whole connection but leave CONN_STATE_READ_REQUEST_LINE as
>>> is, and here we'll no longer turn it to CONN_STATE_LINGER to close the
>>> connection as before hence loop indefinitely between mod_echo and
>>> mpm_event (eating a CPU) for a connection that is EOF already.
>> 
>> True - fixed this in r1897423.
>> 
>> Instead of assuming that all async MPM’s can handle AGAIN, added a dedicated 
>> MPM check for it. This mean no surprises for existing code.
> 
> Why? Really I think you are complicating this. Every async MPM should
> be able to handle EAGAIN, i.e. AP_MPMQ_IS_ASYNC == AP_MPM_CAN_AGAIN.

Alas no, for the reason you pointed out above.

There are three MPMs in trunk that can AP_MPMQ_IS_ASYNC, only one of them 
understands AGAIN.

Little-Net:httpd-trunk6 minfrin$ grep -r IS_ASYNC server/mpm
server/mpm/motorz/motorz.c:case AP_MPMQ_IS_ASYNC:
server/mpm/simple/simple_api.c:case AP_MPMQ_IS_ASYNC:
server/mpm/event/event.c:case AP_MPMQ_IS_ASYNC:

Remember that the SUSPEND function and the AGAIN function are two completely 
separate functions. SUSPEND is all about making sure that some other task can 
happen without blocking (proxy, DNS, etc). AGAIN is about making sure the 
primary connection itself doesn't block.

Regards,
Graham
—

Re: svn commit: r1897281 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/scoreboard.h modules/generators/mod_status.c modules/ssl/mod_ssl.c modules/ssl/ssl_engine_io.c server/mpm/event/event

[Graham Leggett]

On 24 Jan 2022, at 18:07, Yann Ylavic  wrote:

>> Well, this is after check_time_left()..
> 
> Yet we should update the socket timeout for AP_MODE_INIT since it's
> how handshakes are performed..
> Done in r1897422.

Thank you for this.

Regards,
Graham
—

Re: svn commit: r1897281 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/scoreboard.h modules/generators/mod_status.c modules/ssl/mod_ssl.c modules/ssl/ssl_engine_io.c server/mpm/event/event

[Graham Leggett]

On 24 Jan 2022, at 13:56, Yann Ylavic  wrote:

>> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
>> +++ httpd/httpd/trunk/server/mpm/event/event.c Fri Jan 21 00:09:24 2022
>> @@ -1142,6 +1145,7 @@ read_request:
>>  */
>> if (rc != OK || (cs->pub.state >= CONN_STATE_NUM)
>>  || (cs->pub.state < CONN_STATE_LINGER
>> + && cs->pub.state != CONN_STATE_READ_REQUEST_LINE
>>  && cs->pub.state != CONN_STATE_WRITE_COMPLETION
>>  && cs->pub.state != 
>> CONN_STATE_CHECK_REQUEST_LINE_READABLE
>>  && cs->pub.state != CONN_STATE_SUSPENDED)) {
> 
> The issue is that we've never asked process_connection hooks that
> don't care about async and co to change cs->pub.state when they are
> done, and for instance mod_echo's process_echo_connection() will
> consume the whole connection but leave CONN_STATE_READ_REQUEST_LINE as
> is, and here we'll no longer turn it to CONN_STATE_LINGER to close the
> connection as before hence loop indefinitely between mod_echo and
> mpm_event (eating a CPU) for a connection that is EOF already.

True - fixed this in r1897423.

Instead of assuming that all async MPM’s can handle AGAIN, added a dedicated 
MPM check for it. This mean no surprises for existing code.

Regards,
Graham
—

Re: svn commit: r1897281 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/scoreboard.h modules/generators/mod_status.c modules/ssl/mod_ssl.c modules/ssl/ssl_engine_io.c server/mpm/event/event

[Graham Leggett]

On 24 Jan 2022, at 17:21, Yann Ylavic  wrote:

> Maybe the resume_suspended hook is a misnomer? We also have the
> suspend_connection and resume_connection hooks which mean the opposite
> (called when added to and removed from the queues respectively).

The ap_mpm_resume_suspended() is a function rather than a hook. This is what 
gets called when you’ve decided your external condition has been met (DNS 
lookup finished, etc), and the connection should carry on back to pumping data 
like it did before. The two hooks suspend_connection and resume_connection 
allow code that cares to know when something is being paused and unpaused.

Right now what’s confusing is there is no ap_mpm_suspend() function, the module 
is expected to do the suspend stuff by hand:

https://github.com/apache/httpd/blob/3ec0ffb9e1ac05622b97a7afd6992dd2bd41ce38/modules/http/http_request.c#L476

> It sounds more natural to me to think that the connection (processing)
> is suspended when it's in the MPM, than suspended from any MPM action
> when it's outside the MPM (all the connections being processed outside
> the MPM are not in any MPM queue anyway), but YMMV ;)

There are two types of suspended:

- AGAIN/APR_EAGAIN: The socket does not have data for me to read, or the write 
buffer is full and I can no longer write, go into select/poll in the MPM and 
when I can either read or write call me again so I can carry on. In the mean 
time, I’m not hogging the connection, other requests get a go. This is new, 
previously we just blocked and clogged up everything, hogging the slot. Yuck.

- SUSPENDED/CONN_STATE_SUSPENDED: I don't care about my socket, don’t call me. 
If there is data there to read or space on the buffer for me to write I don’t 
care, completely ignore me for now. Maybe I am throttling a connection, maybe 
I’m waiting for DNS, maybe it’s the backend proxy’s turn to read. At some point 
in the future a condition will get fulfilled, and I’ll un-suspend/resume this 
connection, but in the mean time, pretend I don’t exist.

Here is mod_dialup telling the core that it wants to go to sleep:

https://github.com/apache/httpd/blob/1032155c5720e167446efceb4b8e0b545851b7a4/modules/test/mod_dialup.c#L99

Here is mod_dialup telling the core that enough delay has passed, it wants to 
wake back up:

https://github.com/apache/httpd/blob/1032155c5720e167446efceb4b8e0b545851b7a4/modules/test/mod_dialup.c#L138

Regards,
Graham
—

Re: svn commit: r1897387 - in /httpd/httpd/trunk: docs/log-message-tags/next-number modules/ssl/mod_ssl.c

[Graham Leggett]

On 24 Jan 2022, at 16:45, Yann Ylavic  wrote:

>> True - changed to AP_FILTER_ERROR in r1897418, which is the correct code for 
>> this.
> 
> Would it work if ssl_hook_process_connection() handled only EAGAIN and
> otherwise always returned DECLINED (expecting that the usual
> processing would re-catch the error and still behave correctly)?

It’s not ideal.

This means on error, we would then attempt to read again (this time a blocking 
read), and then possibly error again (but maybe not). The existing behaviour 
where we read and then throw away the error result is definitely a bug.

Regards,
Graham
—

Re: svn commit: r1897281 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/scoreboard.h modules/generators/mod_status.c modules/ssl/mod_ssl.c modules/ssl/ssl_engine_io.c server/mpm/event/event

[Graham Leggett]

On 24 Jan 2022, at 15:52, Yann Ylavic  wrote:

>> SUSPENDED already has the opposite meaning, in that a suspended connection 
>> will be excluded from run_process_connection() until such time as some other 
>> event has occurred (response on DNS, a proxy connection received something, 
>> etc) and the connection resumed.
> 
> Hm, where is that? (All the uses of SUSPENDED I see in httpd seem to
> be "defer to the MPM".)

It does defer to the MPM, which then makes sure that connection stays out of 
the queues until some other out-of-band process is triggered for the connection 
to put back in the queue using ap_mpm_resume_suspended(). In other words “don’t 
call me ever, even if data is available, as I’m waiting for something else to 
happen, like say the result of a DNS lookup or a response from a backend 
connection”.

In the case of AGAIN, it means “call me again when you have more data, serve 
other connections while we wait”.

Regards,
Graham
—

Re: svn commit: r1897387 - in /httpd/httpd/trunk: docs/log-message-tags/next-number modules/ssl/mod_ssl.c

[Graham Leggett]

On 24 Jan 2022, at 14:33, Yann Ylavic  wrote:

> I mean not only mod_ssl could return an error for
> ap_get_brigade(AP_MODE_INIT), APR_EGENERAL is not distinctive enough
> IMHO.

True - changed to AP_FILTER_ERROR in r1897418, which is the correct code for 
this.

Regards,
Graham
—

Re: http and http/1.x separation

[Graham Leggett]

On 24 Jan 2022, at 16:28, Stefan Eissing  wrote:

> The main idea is to introduce a META "HEADERS" bucket that is used for
> final/interim responses and footers as well. This will safely pass through
> all filters that do not know about it. This is similar to the ERROR bucket 
> type we already have.

Big +1.

We don’t use metadata buckets enough, and life is made significantly easier 
when used.

Regards,
Graham
—

Re: svn commit: r1897281 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/scoreboard.h modules/generators/mod_status.c modules/ssl/mod_ssl.c modules/ssl/ssl_engine_io.c server/mpm/event/event

[Graham Leggett]

On 24 Jan 2022, at 13:56, Yann Ylavic  wrote:

>> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
>> +++ httpd/httpd/trunk/server/mpm/event/event.c Fri Jan 21 00:09:24 2022
>> @@ -1142,6 +1145,7 @@ read_request:
>>  */
>> if (rc != OK || (cs->pub.state >= CONN_STATE_NUM)
>>  || (cs->pub.state < CONN_STATE_LINGER
>> + && cs->pub.state != CONN_STATE_READ_REQUEST_LINE
>>  && cs->pub.state != CONN_STATE_WRITE_COMPLETION
>>  && cs->pub.state != 
>> CONN_STATE_CHECK_REQUEST_LINE_READABLE
>>  && cs->pub.state != CONN_STATE_SUSPENDED)) {
> 
> The issue is that we've never asked process_connection hooks that
> don't care about async and co to change cs->pub.state when they are
> done, and for instance mod_echo's process_echo_connection() will
> consume the whole connection but leave CONN_STATE_READ_REQUEST_LINE as
> is, and here we'll no longer turn it to CONN_STATE_LINGER to close the
> connection as before hence loop indefinitely between mod_echo and
> mpm_event (eating a CPU) for a connection that is EOF already.

This is true.

> That's why I'm proposing above that ssl_hook_process_connection()
> returns SUSPENDED and that we handle it as keep calling
> run_process_connection() after poll()ing here in the MPM. Or have a
> new CONN_STATE_WAIT_IO or something set by the hooks, though since we
> have SUSPENDED already it looks unnecessary.

SUSPENDED already has the opposite meaning, in that a suspended connection will 
be excluded from run_process_connection() until such time as some other event 
has occurred (response on DNS, a proxy connection received something, etc) and 
the connection resumed.

I think we need an AGAIN, like this:

Index: include/httpd.h
===
--- include/httpd.h (revision 1897407)
+++ include/httpd.h (working copy)
@@ -464,6 +464,9 @@
  */
 #define SUSPENDED -3 /**< Module will handle the remainder of the request.
   * The core will never invoke the request again, */
+#define AGAIN -4/**< Module wants to be called again when
+  *  more data is availble.
+  */
 
 /** Returned by the bottom-most filter if no data was written.
  *  @see ap_pass_brigade(). */

Regards,
Graham
—

Re: svn commit: r1897281 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/scoreboard.h modules/generators/mod_status.c modules/ssl/mod_ssl.c modules/ssl/ssl_engine_io.c server/mpm/event/event

[Graham Leggett]

On 24 Jan 2022, at 14:13, Yann Ylavic  wrote:

>> Maybe it is not needed for the handshake any longer, but it allows fine 
>> grained timeouts on reading speed for headers and request
>> bodies which I am not sure event could deal with. At least it cannot now.
> 
> It's not needed for the handshake *if* we have a HandshakeTimeout or
> something (accept() is already done at this point so AcceptTimeout
> would be a bit misleading), but currently using s->timeout is not
> really the same.
> 
> So far this series is only about handshakes, but indeed if/when all
> the request read phases can go async we probably want the fine grained
> timeouts/rates from mod_reqtimeout to be available still (so either a
> better interaction between the MPM and reqtimeout, or general
> HeaderTimeout/BodyTimeout/... directives applicable by the MPM but
> we'd still need something to count for bytes/rates in the different
> states).

Looks like mod_reqtimeout’s capabilities are overstated at this point - the 
module neatly steps out the way and does nothing the moment it sees a non 
blocking connection:

https://github.com/apache/httpd/blob/trunk/modules/filters/mod_reqtimeout.c#L226

I imagine what we need in the async case is to have the ability to override the 
timeout on the connection, and have the MPM react to that timeout. The pattern 
would then be set the timeout and return APR_EAGAIN, let the mpm handle the 
timeout.

We also need to teach mod_reqtimeout’s code that SSL/TLS libraries sometimes 
want to write:

https://github.com/apache/httpd/blob/trunk/modules/filters/mod_reqtimeout.c#L293

Regards,
Graham
—

Re: svn commit: r1897336 - in /httpd/httpd/trunk: modules/ssl/mod_ssl.c support/ab.c

[Graham Leggett]

On 24 Jan 2022, at 11:37, Ruediger Pluem  wrote:

> How is the above related to this commit?

Yuck, it’s not. Taken out in r1897408.

Regards,
Graham
—

Re: svn commit: r1897281 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/scoreboard.h modules/generators/mod_status.c modules/ssl/mod_ssl.c modules/ssl/ssl_engine_io.c server/mpm/event/event

[Graham Leggett]

On 22 Jan 2022, at 15:01, Yann Ylavic  wrote:

>> @@ -268,12 +268,14 @@ struct timeout_queue {
>> /*
>>  * Several timeout queues that use different timeouts, so that we always can
>>  * simply append to the end.
>> + *   read_line_quses vhost's TimeOut FIXME - we can use a short 
>> timeout here
> 
> I think we need a better interaction with mod_reqtimeout if we add
> client side read timeouts in the MPM.
> For instance we document handshake=5 in mod_reqtimeout and it does not
> fit the fixed s->timeout here (handshake= is disabled by default for
> compatibility because it was added late(ly) in 2.4, but if it's
> configured we should honor it in the MPM too).
> 
> Not sure how to do that but the single timeout per timeout_queue model
> is probably not the right one here.
> Maybe we could queue a timer event based on the actual connection's
> socket timeout?

The present approach is to extend the existing model of queues in the MPM from 
two queues to three, I don’t want to make too many changes to the existing 
structure of the event MPM at this stage if I can get away with it.

In theory, there should be an AcceptTimeout directive alongside Timeout and 
KeepaliveTimeout that mod_reqtimeout should defer to. Or even a case where if 
an async MPM exists, mod_reqtimeout shouldn’t be needed at all?

> I think we should shutdown_connection() directly if READ_REQUEST_LINE times 
> out.

That was done.

Regards,
Graham
—

Re: [GitHub] [httpd-site] rbowen merged pull request #7: Drops link to 1.3 docs, which are gone.

[Graham Leggett]

On 23 Jan 2022, at 20:13, Yehuda Katz  wrote:

> Since 1.3 isn't supported, it would probably be better to remove the entries 
> from Wikipedia or point them to the Internet Archive. Happy to do that.

Wikipedia is an encyclopaedia and therefore it is entirely valid to document 
historical information, please don't do that.

I have been slowly working through that page making sure each entry is up do 
date, don’t want everything that’s just been put in to be ripped back out.

Regards,
Graham
—

Re: [GitHub] [httpd-site] rbowen merged pull request #7: Drops link to 1.3 docs, which are gone.

[Graham Leggett]

On 21 Jan 2022, at 19:48, GitBox  wrote:

> rbowen merged pull request #7:
> URL: https://github.com/apache/httpd-site/pull/7

Can we put the 1.3 docs back?

Pages like this make extensive reference to them: 
https://en.wikipedia.org/wiki/List_of_Apache_modules

Regards,
Graham
—

Re: svn commit: r1897156 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number docs/manual/mod/core.xml modules/dav/main/mod_dav.c server/core.c

[Graham Leggett]

On 19 Jan 2022, at 09:40, Ruediger Pluem  wrote:

>> @@ -723,6 +737,57 @@ static int dav_get_overwrite(request_rec
>> return -1;
>> }
>> 
>> +static int uripath_is_canonical(const char *uripath)
> 
> Isn't this a filesystem path we are talking about in the  case?
> How does this function work on Windows when people might use '\' instead of 
> '/'?

This comes from svn at 
https://github.com/apache/subversion/blob/0f4de5d963ce3b43084f66efab0901be15d2eec2/subversion/libsvn_subr/dirent_uri.c#L1856

If you’ve gone through the expression parser you need a sanity check to verify 
you’re still canonical. You don;t need to canonicalise, as you;ve done this 
already.

You would need to do the same step for directories, but canonicalise to a file 
path.

I am going to look at this again, as you extra-parameter-to-dav is a lot 
cleaner.

Regards,
Graham
—

Re: svn commit: r1897156 - in /httpd/httpd/trunk: CHANGES docs/log-message-tags/next-number docs/manual/mod/core.xml modules/dav/main/mod_dav.c server/core.c

[Graham Leggett]

On 18 Jan 2022, at 12:31, Ruediger Pluem  wrote:

> Not sure I get the intention correct, but this looks like a change to core 
> for a mod_dav specific short coming.

It's not specific to mod_dav, no.

I created a patch ages ago for mod_dav_svn that depended on this patch and this 
got buried under other things, and I have other modules that need the same 
thing: http://svn.apache.org/viewvc/subversion/branches/mod-dav-svn-expressions/

At the core of the problem is for many modules to work, they need to know the 
“root” of the URL space, so that a PATH_INFO can be accurately calculated. As 
soon as you’re in a LocationMatch or DirectoryMatch the root of the URL space 
is replaced with the regex, and you’re lost.

> e.g. in mod_proxy similar problems are solved for ProxyPass that can be used 
> in and outside Directory / Location elements.

The ProxyPass was originally outside of Directory / Location only, I added the 
option of the one parameter version of ProxyPass that inherited from the 
Directory / Location, but this (as I recall) doesn't work for regexes.

It would be great if the two parameter ProxyPass can go away in future to 
simplify.

> I would be in favor for a similar approach here, by either allowing a second 
> optional parameter to the 'DAV' directive that sets
> this or for a separate directive that allows to set this. In case nothing is 
> set the cmd->path could be used as currently.
> This also allows to use this feature in 'if' sections as well.
> Another option would be to provide this information via an environment 
> variable that can set via SetEnvIfExpr or a RewriteRule.

H…

Adding an extra parameter to DAV, and then to something generic like SetHandler 
may do the trick. This gives most modules access to a sane path when used 
inside a Match.

Let me think about this for a bit.

Regards,
Graham
—

Re: TLS neverbleed design

[Graham Leggett]

On 17 Jan 2022, at 19:17, Ruediger Pluem  wrote:

>> I see. Thanks for the clarification. Did not really now about that 
>> interface. Then I see no pressing point in adding an additional API, indeed.
> 
> I haven't looked deeply in this, but based on the pointers from Joe I guess
> this can be done with
> 
> - p11-kit
> - softhsm
> - Some configuration on OpenSSL side to use the p11-kit client as pkcs11 
> provider for accessing the p11-kit server over a Unix
> domain socket
> 
> p11-kit and softhsm seem to be readily available at least on later versions 
> of Ubuntu and RedHat.
> 
> As the pkcs11 engine requires some configuration it could be nice adding a 
> feature to mod_ssl that allows to load a different
> configuration file than the standard Openssl configuration file.

No need - as of recently this has been simplified and is available in v2.4.

Start by passing pkcs11 URLs into mod_ssl as follows:

SSLCertificateFile 
pkcs11:token=Local%20Tokens;id=%B0%90%5A%36%1A%E0%2F%DA%13%14%81%FC%E1%DE%9D%86%9E%30%8F%AE;type=cert
SSLCertificateKeyFile 
pkcs11:token=Local%20Tokens;id=%B0%90%5A%36%1A%E0%2F%DA%13%14%81%FC%E1%DE%9D%86%9E%30%8F%AE;object=;type=private?pin-value=

The pkcs11 URL prefix is enough to get mod_ssl to tell openssl it wants the 
pkcs11 engine, provided by the openssl-pkcs11 RPM package on Redhat derivatives 
(native package is called https://github.com/OpenSC/libp11). No need to mess 
about with manual engine configs, mod_ssl handles the details for you.

In turn, if p11-kit is installed then all pkcs11 driver packages should “just 
work” on installation. I have had the most success with the opencryptoki-swtok 
package, a software HSM made by IBM.

The softhsm package by Redhat is packaged in such a way that it cannot be used 
by anything other than opendnssec, which is a shame.

If you want to store more than one certificate in your HSM / on your token, you 
need this fix: https://github.com/OpenSC/libp11/pull/433

Regards,
Graham
—

Re: svn commit: r1897123 - /httpd/httpd/patches/2.4.x/httpd-2.4-ldap-expr.patch

[Graham Leggett]

On 16 Jan 2022, at 18:54, Yann Ylavic  wrote:

> Maybe "ldap_escape" would be a more appropriate name, should there be
> a need for another ldap function (e.g. "ldap_unescape") later?

This doesn’t follow the existing “short” naming pattern of the existing 
entries. I image that we’d add something like “unldap" to match “unbase64”.

Regards,
Graham
—

Broken links at https://httpd.apache.org/docs/

[Graham Leggett]

Hi all,

The v1.3 historical docs link at https://httpd.apache.org/docs/ currently leads 
to a 404, is that intentional?

Regards,
Graham
—

Re: mod_tls as experimental module?

[Graham Leggett]

On 18 Nov 2021, at 19:48, ste...@eissing.org wrote:

> How would you feel about adding mod_tls 
> (https://github.com/abetterinternet/mod_tls) as an experimental module to 
> Apache httpd?

Having more choice is excellent.

> For people who have not followed that development:
> - it is a TLS 1.2/1.3 implementation based on rustls, 
> https://github.com/rustls/rustls
> - the C API is rustls-ffi, found at https://github.com/rustls/rustls-ffi
> - it is itself written in C, linking all the Rust things from the rustls-ffi 
> library
> - it does not bring any Rust into our code base
> - functionality wise, it is a clear subset of what mod_ssl offers via openssl
>  (e.g. no client certificates now and not as tweakable - at least for now)

Client certs is a must for me. I know that they’re political, but my clients in 
finance don’t care.

> - it can be co-loaded and co-used with mod_ssl on different ports or 
> front-/backend roles
> - performance-wise, according to my plain vanilla tests, it is on par with 
> mod_ssl
> 
> The decision to offer it downstream is of course then made by the distros, as 
> usual with experimental modules. And if and how it is then used rests with 
> the users. It is an offered alternative for people.
> 
> What would be the benefit to the project?
> - we offer people an alternative. If they feel the memory safety that Rust 
> offers is important to them, they can do it with Apache httpd for the TLS 
> stack.
> - we could see how people react to this and adapt our TLS offering 
> accordingly. It being experimental, we remain free to change it. Or remove it 
> again.
> 
> Organizational Things:
> - the development was done by myself
> - the work was sponsored by the ISRG (https://www.abetterinternet.org), the 
> org behind Let's Encrypt, as part of they "memory safety" initiative 
> (https://www.memorysafety.org)
> 
> 
> Feedback appreciated,

I lean towards adding it as an httpd subproject at this level:

https://svn.apache.org/viewvc/httpd/

This frees you from all the complexity around experimental bits of httpd 
compared to fully supported bits.

This means that practically, it can be packaged through channels like EPEL 
until it becomes stable, at which point the distros will pick it up.

It also insulates us against external dependencies like rust. Languages wax and 
wane, should a rust2 coming along, or another language called oxide that’s 
better, we’re not left with aging code in our codebase. This is a problem that 
APR solves for us for operating systems.

Regards,
Graham
—

Re: [VOTE] Release httpd-2.4.50-rc1 as httpd-2.4.50

[Graham Leggett]

On 1 Oct 2021, at 15:41, ste...@eissing.org wrote:

> I would like to call a VOTE over the next few days to release
> this candidate tarball httpd-2.4.50-rc1 as 2.4.50:
> [ ] +1: It's not just good, it's good enough!

+1 on CentOS7.

Regards,
Graham
—

Re: release?

[Graham Leggett]

On 30 Aug 2021, at 12:35, ste...@eissing.org wrote:

> In what state is our release handling? Given someone holding my hand, could 
> I do it? Or is it better to look someone over the shoulder while he does it?

When I did it in the past, I walked through the commit emails of previous 
releases, and performed the same steps.

Regards,
Graham
—

Re: Question about APR trunk and httpd ldap modules

[Graham Leggett]

On 27 May 2021, at 14:45, Rainer Jung  wrote:

> is my understanding correct, that even httpd trunk (and then also 2.4.x) 
> needs LDAP support in APR/APU to build mod_ldap and mod_authnz_ldap?
> 
> So since we removed LDAP support from APR trunk, that means those modules 
> currently can not be build using APR trunk, neither in httpd trunk nor in 
> httpd 2.4.x. Correct?

Correct.

> Just trying to understand the current situation about APR trunk and its 
> implications.

I have a partially finished attempt at a new API for APR called apr_ldapx, I 
need to dig it out. Worth moving this to APR and working on a branch there?

Regards,
Graham
—

Re: svn commit: r1890947 - in /httpd/httpd/branches/2.4.x: ./ CHANGES STATUS include/ap_mmn.h modules/proxy/mod_proxy.h modules/proxy/mod_proxy_ajp.c modules/proxy/mod_proxy_balancer.c modules/proxy/m

[Graham Leggett]

On 22 Jun 2021, at 09:09, Ruediger Pluem  wrote:

>> +timeout = apr_time_from_sec(10); /* 10 seconds */
> 
> Sorry for jumping on this late, but as I already mentioned on the original 
> trunk revision a hard coded timeout is bad.
> Please also backport r1887439. For the record I am +1 on backporting r1887439.

+1 to this.

I’ve proposed it here: http://svn.apache.org/viewvc?rev=1890960=rev 


Regards,
Graham
—

Re: Broken: apache/httpd#1394 (2.4.x - fa22b50)

[Graham Leggett]

Hi all,

https://travis-ci.com/github/apache/httpd/builds/213431494?utm_medium=notification_source=email
 failed for what looks like missing perl libraries:

t/modules/proxy_websockets.t  Can't locate AnyEvent/WebSocket/Client.pm 
in @INC (you may need to install the AnyEvent::WebSocket::Client module) (@INC 
contains: /home/travis/build/apache/httpd/test/perl-framework/blib/lib 
/home/travis/build/apache/httpd/test/perl-framework/blib/arch 
/home/travis/build/apache/httpd/test/perl-framework 
/home/travis/perl5/lib/perl5/5.26.1/x86_64-linux-gnu-thread-multi 
/home/travis/perl5/lib/perl5/5.26.1/x86_64-linux-gnu-thread-multi 
/home/travis/perl5/lib/perl5/5.26.1 
/home/travis/perl5/lib/perl5/x86_64-linux-gnu-thread-multi 
/home/travis/perl5/lib/perl5/5.26.1/x86_64-linux-gnu-thread-multi 
/home/travis/perl5/lib/perl5/5.26.1 
/home/travis/perl5/lib/perl5/x86_64-linux-gnu-thread-multi 
/home/travis/perl5/lib/perl5 
/home/travis/perl5/lib/perl5/5.26.0/x86_64-linux-gnu-thread-multi 
/home/travis/perl5/lib/perl5/5.26.0 
/home/travis/perl5/lib/perl5/5.26.0/x86_64-linux-gnu-thread-multi 
/home/travis/perl5/lib/perl5/5.26.1/x86_64-linux-gnu-thread-multi 
/home/travis/perl5/lib/perl5/5.26.1 
/home/travis/perl5/lib/perl5/x86_64-linux-gnu-thread-multi 
/home/travis/perl5/lib/perl5 /etc/perl 
/usr/local/lib/x86_64-linux-gnu/perl/5.26.1 /usr/local/share/perl/5.26.1 
/usr/lib/x86_64-linux-gnu/perl5/5.26 /usr/share/perl5 
/usr/lib/x86_64-linux-gnu/perl/5.26 /usr/share/perl/5.26 
/home/travis/perl5/lib/perl5/5.26.0 
/home/travis/perl5/lib/perl5/5.26.0/x86_64-linux-gnu-thread-multi 
/home/travis/perl5/lib/perl5/5.26.0 
/home/travis/perl5/lib/perl5/5.26.0/x86_64-linux-gnu-thread-multi 
/usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base .) at 
t/modules/proxy_websockets.t line 10.
2718 <>BEGIN failed--compilation aborted at t/modules/proxy_websockets.t line 
10.
2719 <>t/modules/proxy_websockets.t  Dubious, test returned 2 (wstat 
512, 0x200)
2720 <>No subtests run 




Regards,
Graham
—

Re: svn commit: r1885248 - /httpd/httpd/branches/2.4.x/STATUS

[Graham Leggett]

On 07 Jan 2021, at 19:54, yla...@apache.org wrote:

> + ylavic: +1 from covener dropped after adding r1885239, r1885240 and
> + r1885244 in v2.

Would it be possible to take a second look at this @covener?

I am imagining proposals after this one might conflict if not done in order.

Regards,
Graham
—

make update-changes failure on MacOS

[Graham Leggett]

Hi all,

A quick heads up on the update-changes mechanism, on MacOS the mechanism fails 
as follows, and the CHANGES file is lost (but retrievable from svn):

Little-Net:httpd-2.4.x-3 minfrin$ make update-changes
awk: invalid -v option

awk: invalid -v option

It looks like the -v option is not portable?

Regards,
Graham
—

Re: svn commit: r1884280 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_ftp.c modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c

[Graham Leggett]

On 12 Dec 2020, at 01:59, Roy T. Fielding  wrote:

> That is too many questions. The purpose of the cache requirement is so that 
> the cache
> does not deliver a non-validated entry after receiving a failed validation. 
> It doesn't really
> matter what code is returned so long as it is 5xx, so the specs are 
> over-constraining here.
> 
> The CoAdvisor test suite is overly pedantic.
> 
> And, as stated, this only applies when the request contains max-revalidate 
> and the
> action being done is a cache revalidation. No other code should behave this 
> way.

To clarify, the change under discussion covers behaviour when a proxy of any 
kind (including an ftp proxy) suffers a low level network error of any kind, 
from DNS errors to low level network errors.

Whether ultimately correct or not, CoAdvisor was very specific as to the codes 
to be returned in these cases.

> Reverting the change is the correct call, regardless, but it is also the 
> right choice.
> I have filed a bug on the Cache spec to change that MUST send 504 to a MUST 
> send 5xx.
> 
>https://github.com/httpwg/http-core/issues/608 
> 
> 
> If you think we need to change other things in the specs, please file bugs 
> now.

The above issue relates to must-revalidate only, while the wider issue is for 
which 5xx error to be returned in which situation. Can you clarify?

The patch in this thread covers each case, and basically boils down to a choice 
between “Bad Gateway” or “Timed Out”.

Given the long list here http://coad.measurement-factory.com/clients.html I 
would be keen to make sure httpd worked the same way as all those other servers.

Regards,
Graham
—

Re: svn commit: r1884280 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_ftp.c modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c

[Graham Leggett]

On 11 Dec 2020, at 14:13, Yann Ylavic  wrote:

> Where is this test suite?

To fill you in, the Co-Advisor test suite is a commercial HTTP suite available 
here: http://coad.measurement-factory.com

A number of years ago they donated to our project one year access to their 
suite for free, a service worth many thousands of dollars, and I used their 
test suite within the time limit they gave us to take httpd from many hundreds 
of protocol violations down to zero.

All violations were backported to v2.4 but this one, and as a result Apache is 
not listed here: http://coad.measurement-factory.com/clients.html

> Which RFC violation, a proxy socket connection error should return 504
> Gateway Timeout??

The RFC violation that was flagged by the test suite as described above.

> I see that RFC2616 14.9.4 is about cache, why don't you fix this in mod_cac=
> he?

The fix applied consisted of the required changes to make the Co-Advisor suite 
resolve the violation.

>> Please resolve the discussion above.
> 
> You should do that, it's not my veto. Failing to resolve the
> discussion, the commit should be reverted right?

It should not be reverted, no.

The commit was not vetoed, the backport to 2.4 was, and for a good reason - a 
change to the response code in a point release would have destabilised some 
people. Fixing this issue on trunk for a future release is entirely fine.

The problem you’re really trying to solve is the inconvenience of having trunk 
and v2.4 being different.

The fix to this is to replace HTTP_BAD_GATEWAY with a neural macro like 
PROXY_TIMEOUT, and then #define PROXY_TIMEOUT to be HTTP_GATEWAY_TIME_OUT on 
trunk, and HTTP_BAD_GATEWAY on v2.4.

Please don’t back out protocol behaviour without checking the origin of the 
change first. All of what I describe above is in our commit history and mailing 
lists.

Regards,
Graham
—

Re: svn commit: r1884280 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_ftp.c modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c

[Graham Leggett]

On 10 Dec 2020, at 18:04, yla...@apache.org wrote:

> Author: ylavic
> Date: Thu Dec 10 16:04:34 2020
> New Revision: 1884280
> 
> URL: http://svn.apache.org/viewvc?rev=1884280=rev
> Log:
> Revert r1480058, -1'ed on dev@ and STATUS.
> 
> Never backported (and never will supposedly), while often creating
> merge conflicts.

You’ve just reverted a fix to an RFC violation that was picked up by the 
CoAdvisor test suite.

“Creating merge conflicts” is not a sufficient technical justification for a 
veto that results in the re-introduction of an RFC violation.

Please back this out.

> See 
> https://lists.apache.org/thread.html/be0e7bdc3510fddd2dd80accece44917eba361ef4fcc713dd0f7f7fa%401367999236%40%3Cdev.httpd.apache.org%3E
> and 
> https://lists.apache.org/thread.html/6e63271b308a2723285d288857318e7bb51b6756690514d9bc75a71b%401371148914%40%3Ccvs.httpd.apache.org%3E
>  
> 

Please resolve the discussion above.

The last on the thread is that Roy was asked for advice, but he was busy. In 
the light of RFC7230 it would be useful to get a new answer on this question.

Regards,
Graham
—

Re: Xcode 12

[Graham Leggett]

On 29 Oct 2020, at 14:05, Jim Jagielski  wrote:

> Anyone hacking away on httpd and/or APR w/ Xcode 12? On my system at least 
> it is throwing errors about -Werror,-Wimplicit-function-declaration, and not 
> enabling IPv6:
> 
>checking if APR supports IPv6... no -- no working getaddrinfo
> 
> also likely due to an error when compiling because of stdlib.h not being 
> included...

I’m not hacking for lack of time - I want to help when I do though.

Regards,
Graham
—

ap_expr_exec() and Vary: Vary remains unset if we parse a string

[Graham Leggett]

Hi all,

I just been digging into why the Vary header is not set when generating a 
string expression with ap_expr_exec().

We currently set the Vary header when evaluating a condition:

https://github.com/apache/httpd/blob/53b5c234e90db489197935b84c0066c9ec083038/server/util_expr_eval.c#L1182

but we calculate the Vary header and then discard it when evaluating a string:

https://github.com/apache/httpd/blob/53b5c234e90db489197935b84c0066c9ec083038/server/util_expr_eval.c#L1166

The fix appears to be this simple:

Index: server/util_expr_eval.c
===
--- server/util_expr_eval.c (revision 1880567)
+++ server/util_expr_eval.c (working copy)
@@ -1163,6 +1163,10 @@
   "Evaluation of string expression from %s:%d gave: 
%s",
   ctx->info->filename, ctx->info->line_number,
   *ctx->result_string);
+
+if (ctx->r && ctx->vary_this && *ctx->vary_this)
+apr_table_merge(ctx->r->headers_out, "Vary", *ctx->vary_this);
+
 return 1;
 }
 }

Apart from a sudden outbreak of RFC compliance, what effect might this have on 
a backport to v2.4?

Regards,
Graham
—

Re: [VOTE] Release httpd-2.4.45

[Graham Leggett]

On 30 Jul 2020, at 11:16, Steffen Land  wrote:

> +1 on Windows.
> 
> I am in doubt for a -0 :
> 
> Still quite some (more) warnings, now 432, attached Win64 warnings with the 
> ones from APR-UTIL.
> 
> I think a goal is (must be) that we get warning free on all platforms, now it 
> looks bad on Windows.
> 
> I reported here a few times. APR is warning free, thanks to Yann.

Apr-util is a library from the APR project, not httpd, and so warnings from APR 
wouldn’t be relevant for an httpd release, or for the httpd project.

That said there is definite need for more Windows testing over at APR, if you 
or members of the Apachelounge community are in the position to contribute 
patches this will be very much appreciated.

Regards,
Graham
—

Re: NOTICE: Intent to T late this week

[Graham Leggett]

On 23 Jul 2020, at 01:32, Daniel Ruggeri  wrote:

> It's been a while since we've rolled a release and gotten fixes/etc in our 
> community's hands. Apologies for not suggesting this sooner. How about a T 
> Friday? That will let vote run through the weekend.

+1.

Regards,
Graham
—

Re: svn commit: r1879591 - in /httpd/httpd/trunk: configure.in server/log.c

[Graham Leggett]

On 15 Jul 2020, at 16:27, Joe Orton  wrote:

>> Looks like ac_cv_func_gettid is no but ap_cv_gettid is yes.
> 
> This is not consistent with what you posted before.  Looking again, the 
> old configure output you posted has:
> 
> checking for gettid()... yes
> 
> note the (), which is only possible if you haven't re-run buildconf.  
> Re-run buildconf and configure, and if the build still fails please 
> provide the full config.log somewhere so I can try to understand what's 
> going on.

Double checked the terminal history, and I had indeed run buildconf - this 
wasn't enough though.

As rpluem suggested "make extraclean” was the secret, this is needed over and 
above the buildconf. The build is now clean.

Regards,
Graham
—

Re: svn commit: r1879888 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h modules/dav/main/mod_dav.h modules/dav/main/util.c

[Graham Leggett]

On 15 Jul 2020, at 15:33, Greg Stein  wrote:

> Seems these helper functions would be a better fit within apr_xml, rather 
> than httpd.

In the long term, yes.

In the “it must be practical to use this with v2.4” term, I’d rather continue 
with the established pattern, and then move the lot of these functions as a 
future mod_dav cleanup. I don't want to boil the ocean at this point, there has 
already been a lot of change to make things work.

Regards,
Graham
—

Re: svn commit: r1879889 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h modules/dav/main/mod_dav.h modules/dav/main/props.c

[Graham Leggett]

On 15 Jul 2020, at 16:18, Greg Stein  wrote:

> You're already changing the API ... why not simply introduce insert_prop_v2() 
> with the appropriate parameters? This concept of "pass parameters via the 
> pool" is very disturbing.

Changes to these provider APIs can’t be backported, thus the pool.

Separately I’d like to change the signature of insert_prop(), but that’s a 
separate patch. Not seeing any point in insert_prop_v2() if backporting isn't 
possible, rather keep it clean and have one API that does the right thing in 
v2.6 onwards.

Regards,
Graham
—

Re: svn commit: r1879591 - in /httpd/httpd/trunk: configure.in server/log.c

[Graham Leggett]

On 15 Jul 2020, at 15:35, Joe Orton  wrote:

>> checking for gettid()... yes
> 
> Interesting, can you provide the config.log and "rpm -q glibc"?  With a 
> RHEL8 vm here it does not detect gettid (as I'd expect for glibc 2.28) 
> and builds fine.

A quick and dirty search across config.log shows this:

[minfrin@bob httpd-trunk]$ cat config.log | grep gettid
configure:8059: checking for gettid
conftest.c:(.text+0xa): undefined reference to `gettid'
| /* Define gettid to an innocuous variant, in case  declares gettid.
| #define gettid innocuous_gettid
| which can conflict with char gettid (); below.
| #undef gettid
| char gettid ();
| #if defined __stub_gettid || defined __stub___gettid
| return gettid ();
configure:8107: checking for gettid() via syscall
ac_cv_func_gettid=no
ap_cv_gettid=yes

Looks like ac_cv_func_gettid is no but ap_cv_gettid is yes.

[minfrin@bob httpd-trunk]$ rpm -q glibc
glibc-2.28-72.el8_1.1.x86_64

A search for what ends up in the ap_config_auto.h shows this:

[minfrin@bob httpd-trunk]$ grep -r GETTID include/
include/ap_config_auto.h:#define HAVE_GETTID 1
include/ap_config_auto.h.in:#undef HAVE_GETTID
include/ap_config_auto.h.in:#undef HAVE_SYS_GETTID

Regards,
Graham
—

Re: svn commit: r1879591 - in /httpd/httpd/trunk: configure.in server/log.c

[Graham Leggett]

On 07 Jul 2020, at 15:40, jor...@apache.org wrote:

> Author: jorton
> Date: Tue Jul  7 13:40:15 2020
> New Revision: 1879591
> 
> URL: http://svn.apache.org/viewvc?rev=1879591=rev
> Log:
> Check for and use gettid() directly if available; glibc 2.30 and later
> provides a wrapper for the system call:
> 
> * configure.in: Check for gettid() and define HAVE_SYS_GETTID if
>  gettid() is only usable via syscall().
> 
> * server/log.c (log_tid): Use gettid() directly if available.

This is not working for me on CentOS8.

./configure says this:

checking for gettid()... yes

but the build fails like this:

log.c: In function 'log_tid':
log.c:544:21: warning: implicit declaration of function 'gettid'; did you mean 
getgid'? [-Wimplicit-function-declaration]
 pid_t tid = gettid();
 ^~
 getgid

Is there maybe a missing header file needed somewhere?

MacOS says this and works:

checking for gettid()... no

Regards,
Graham
—

Re: svn commit: r1879488 - /httpd/httpd/trunk/server/util_etag.c

[Graham Leggett]

On 06 Jul 2020, at 09:28, Ruediger Pluem  wrote:

>> +apr_sha1_init();
>> +nbytes = sizeof(buf);
>> +while ((status = apr_file_read(fd, buf, )) == APR_SUCCESS) {
> 
> Why do we still use apr_file_read in case we use MMAP? IMHO we should use 
> apr_mmap_offset.
> But we would need to consider the amount of memory we map at the same time 
> for large files
> in order to avoid exhausting the memory. So I would propose that we create a 
> brigade with one
> filebucket and do bucket reads. So like the following code from the default 
> handler (excluding the reading):
> 
>bb = apr_brigade_create(r->pool, c->bucket_alloc);
> 
>e = apr_brigade_insert_file(bb, fd, 0, r->finfo.size, r->pool);
> 
> #if APR_HAS_MMAP
>if (d->enable_mmap == ENABLE_MMAP_OFF) {
>(void)apr_bucket_file_enable_mmap(e, 0);
>}
> #endif
> 
> Of course that would require to get the finfo in case we open the file 
> descriptor on our own.
> 
> The reading would then be something like (without error handling):
> 
> while (!APR_BRIGADE_EMPTY(bb))
> {
>  e = APR_BUCKET_FIRST(bb)
>  apr_bucket_read(e, , );
>  apr_sha1_update_binary(, str, len);
>  apr_bucket_delete(e);
> }

Makes much more sense - r1879541.

Regards,
Graham
—

Re: iOS 14 / macOS 11 and HTTP/3 support

[Graham Leggett]

On 29 Jun 2020, at 10:16, Stefan Eissing  wrote:

>> The main point is that it must be done carefully and properly, but this is 
>> not a reason to not do it at all.
> 
> Apache httpd is a very strong HTTP/1.x server. And your efforts to make 
> response streaming more event driven have been great. 
> 
> However it has resulted in code that is, in my impression on talks on the dev 
> list regarding Yann's changes, very hard to read and improve. Not impossible, 
> but not for the meek. And not because it is written badly, but because the 
> state handling and interactions between buckets and pools is complex and full 
> of pitfalls.
> But if it's done properly, as you say, it works nicely.

Where the h2 effort was made hard, was that it was done within the limitation 
that httpd wasn’t changed to accommodate your needs.

H2 exists with the existing event MPM, the existing network filter, and the 
existing mod_ssl filter. Hats off to you for what you’ve achieved, but there 
reaches a point where you have to stop and say “what does httpd need to do for 
me that it doesn't do now”. You then ask a second question - “if I need to 
change httpd, where would I change it”.

Right now, today, we have the following tools at your disposal:

- An MPM. This translates between the architecture specific to each client, and 
httpd.
- The network filter. This creates a TCP connection on any platform.
- mod_ssl. This adds an SSL layer to a TCP connection.
- mod_http. Turns a connection (conn_rec) into a request (request_rec).

So, lets see what you might need.

- A UDP based protocol needs UDP based services from the operating system. I 
predict a new MPM called mpm_event+ that can do generic TCP and UDP. You don't 
write this from scratch - fork mpm_event, and work from there.
- The network filter. This might make your life hard and get in the way, you 
might choose to scrap the network filter, or move the network filter further 
down.
- mod_ssl. Looks like this would be replaced completely with a mod_h3.
- mod_http - maybe that’s still here, maybe it’s mod_h3’s job, not sure.

What you need to acknowledge:

- People want to write there own server extensions, be they modules, python 
code, etc etc. That code may not work.
- People will call blocking code.
- H3 code will need to cater for when - not if - people write either bad code 
or blocking code.
- This is not a showstopper, this just needs to be catered for. Prefork catered 
for it by giving a dedicated process to every bit of code. Worker gave a thread 
to each bit of code, and accepted if one thread crashed so would the others. 
Event catered by being Worker, with the option of being async after the initial 
work was done.
- Having an async “core” interacting over unix domain sockets or the like with 
code that is free to block, crash, leak, etc means we remain bulletproof in the 
real world.

> For anyone thinking about bringing h3 into the server, consider your options:
> 1. Think of a mod_h3 as its own h3->h1 proxy. Open a h1 connection for every 
> quic stream, write a h1 request, read (often event based, often not) a h1 
> response from it, transform this to your h3 answer. Stay away from touching 
> the server's h1 stasis field - cope with it.
> 2. Redesign a FOSS httpd v3.x with a new architecture, embracing a 
> non-blocking processing model. Maybe under a different name.

Option 2 has been done, over and over.

Where option 2 falls flat is that after the initial euphoric implementation is 
made, the really non sexy work of protocol compliance begins, testing begins, 
and so on.

Apache httpd started to support RFC2616 in 1999 when it was published, but 
httpd only properly supported RFC2616 14 years later in 2013 when we were 
generously gifted access to a compliance test suite, and I spent 2 solid weeks 
of my life killing every protocol violation reported.

You don’t want to start from scratch, trust me.

I recommend the following:

- Start the work on a branch, but ultimately you want this to be in httpd 
trunk. No rewrites.
- Look at what a mod_event+ MPM might need to do to provide the UDP and TCP 
services you need. No sacred cows, if you need to change something, change it.
- Look at mod_ssl and mod_http, and ask what a mod_h3 might do to replace them 
both.
- Look at how a mod_h3 might create a request (request_rec), and how it might 
process it. Do you want to insulate people from blocking/crashing code here or 
higher up? How might the MPM help here?

Regards,
Graham
—

Re: svn commit: r1879285 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/core.xml include/ap_mmn.h include/http_core.h include/http_protocol.h include/httpd.h modules/dav/fs/repos.c modules/test/mod_

[Graham Leggett]

On 03 Jul 2020, at 11:19, Ruediger Pluem  wrote:

> Thanks for the pointer. Is Content-MD5 really used? And given that it has 
> been removed in the RFC my approach would be as follows:
> 
> 1. Continue with your new additions as is. Do not try to merge any of this 
> code with Content-MD5 related content.
> 2. Backport them.
> 3. Leave Content-MD5 untouched in 2.4.x.
> 4. Remove Content-MD5 in trunk.

Thought about it some more and was going to suggest the above instead, +1.

Regards,
Graham
—

Re: svn commit: r1879465 - /httpd/httpd/trunk/server/util_etag.c

[Graham Leggett]

On 03 Jul 2020, at 11:02, Ruediger Pluem  wrote:

>> apr_sha1_init();
>> nbytes = sizeof(buf);
>> -while (apr_file_read(fd, buf, ) == APR_SUCCESS) {
>> +while ((status = apr_file_read(fd, buf, )) == APR_SUCCESS) {
> 
> Don't we need to set the fp in fd via apr_file_seek(fd, APR_CUR, to 0?
> In case we did not open the fd on our own it might be somewhere in the file 
> and the digest would only base on a part of the file?

We do indeed - r1879469.

Regards,
Graham
—

Re: svn commit: r1879285 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/core.xml include/ap_mmn.h include/http_core.h include/http_protocol.h include/httpd.h modules/dav/fs/repos.c modules/test/mod_

[Graham Leggett]

On 29 Jun 2020, at 16:37, Ruediger Pluem  wrote:

> Makes sense.
> Do you see a possibility to merge this code and the one of ap_md5digest to a 
> more generic procedure that
> allows to choose the digest algorithm while using 'MMAPED' reads?
> BTW: Is sha1 mandatory for strong etags? If not wouldn't MD5 be enough and if 
> MD5 is seen as too insecure
> why isn't sha1?

I chose sha1 as it was a) widely available in APR and b) better than md5, but 
that was it.

I am wondering if for 2.4 if we use md5 instead, and then set Content-MD5 at 
the same time in the same code instead of calculating the md5 twice.

Then - as per the removal of Content-MD5 from 
https://tools.ietf.org/html/rfc7231#appendix-B - we separately switch it to 
sha1 and remove Content-MD5 in trunk.

Is that sane?

Regards,
Graham
—

Re: svn commit: r1862270 - in /httpd/httpd/trunk/modules/dav: fs/dbm.c fs/repos.c main/mod_dav.c main/props.c main/std_liveprop.c main/util.c

[Graham Leggett]

On 28 Jun 2019, at 10:50, rpl...@apache.org wrote:

> * Replace apr_psprintf with apr_pstrcat where the format strings only
>  contain %s to improve efficiency. Leave out error messages as they
>  are not on a crtical code path and error message become less readable
>  when taking out the format specifiers.

I’ve proposed this for backport, as it blocks other dav changes. Would it be 
possible to take a look?

Regards,
Graham
—

Re: svn commit: r1879285 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/core.xml include/ap_mmn.h include/http_core.h include/http_protocol.h include/httpd.h modules/dav/fs/repos.c modules/test/mod_

[Graham Leggett]

On 29 Jun 2020, at 14:49, Yann Ylavic  wrote:

>> Yes we can and should (but in separate commits).
>> 
>> I have my eye on the r->proxyreq flag, we can pack this into the binary 
>> notes too, values don’t need to be one bit wide.
> 
> Actually I was thinking the other way around, have the new "unsigned
> int strong_etag:1" bitfield rather than changing the existing ones...
> Why adding complexity with bit(s) macros while bitfields exist?

The problem with bitfields in the public APIs is that they’re not binary 
compatible across compilers. While it is very rare that a module will be built 
with a different compiler than httpd was, it’s still theoretically possible, 
and we should probably avoid it. Bitfields aren’t a problem for in-module or 
in-core code.

Regards,
Graham
—

Re: svn commit: r1879285 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/core.xml include/ap_mmn.h include/http_core.h include/http_protocol.h include/httpd.h modules/dav/fs/repos.c modules/test/mod_

[Graham Leggett]

On 29 Jun 2020, at 11:41, Ruediger Pluem  wrote:

>> +etag = apr_palloc(r->pool, weak_len + sizeof("\"\"") +
>> +4*(APR_SHA1_DIGESTSIZE/3) + vlv_len + 4);
> 
> Using apr_base64_encode_len in the formula above would make it easier to 
> understand and read IMHO.

It would also mean it could no longer be optimised to a constant. What side do 
we want to fall on?

>> +
>> +apr_sha1_init();
>> +nbytes = sizeof(buf);
>> +while (apr_file_read(fd, buf, ) == APR_SUCCESS) {
> 
> I assume that the code has been taken from ap_md5digest, but
> if we have MMAP available and if we have not disabled MMAP we use MMAP to 
> read the contents of the file
> if sendfile is not possible (e.g. SSL connections).
> Wouldn't using MMAP more performant here especially in case of larger files?

It makes sense, but I would want to change something like this separately.

>> +apr_sha1_update_binary(, buf, nbytes);
>> +nbytes = sizeof(buf);
>> +}
>> +apr_file_seek(fd, APR_SET, );
> 
> Why do we always reset the file pointer to 0? Why don't we get the actual 
> file pointer of fd before we do the reading
> and restore it afterwards?

I am guessing that the why is lost in the mists of time. As a guess, it avoids 
an additional call.

Using the actual file pointer is cleaner, as there is a guarantee that the 
function does not have any side effects.

http://svn.apache.org/viewvc?rev=1879332=rev 


Regards,
Graham
—

Re: svn commit: r1879285 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/core.xml include/ap_mmn.h include/http_core.h include/http_protocol.h include/httpd.h modules/dav/fs/repos.c modules/test/mod_

[Graham Leggett]

On 29 Jun 2020, at 13:08, Yann Ylavic  wrote:

>> /**
>>  * @defgroup module_magic Module Magic mime types
>> @@ -1097,6 +1138,11 @@ struct request_rec {
>>  *  TODO: compact elsewhere
>>  */
>> unsigned int flushed:1;
>> +/** Request flags associated with this request. Use
>> + * AP_REQUEST_GET_FLAGS() and AP_REQUEST_SET_FLAGS() to access
>> + * the elements of this field.
>> + */
>> +ap_request_bnotes_t bnotes;
>> };
> 
> Can't we use a single bitfield (like "flushed" above) for the single
> AP_REQUEST_STRONG_ETAG flag?

Yes we can and should (but in separate commits).

I have my eye on the r->proxyreq flag, we can pack this into the binary notes 
too, values don’t need to be one bit wide.

Regards,
Graham
—

Re: svn commit: r1879307 - /httpd/httpd/trunk/modules/dav/main/mod_dav.c

[Graham Leggett]

On 29 Jun 2020, at 12:10, Ruediger Pluem  wrote:

>> +result = dav_run_deliver_report(r, resource, doc,
>> +r->output_filters, );
>> +switch (result) {
>> +case OK:
>> return DONE;
> 
> What happens if err != NULL. Should we handle that here like in the default 
> case or
> should the hook implementer ensure to not return OK if err != NULL?

Hmmm… this needs tightening up.

>> +case DECLINED:
>> +/* No one handled the report */
>> +return HTTP_NOT_IMPLEMENTED;
> 
> Previously we were returning DECLINED in case there was no vsn_hooks and thus 
> nothing to report. Now we return
> HTTP_NOT_IMPLEMENTED. Why this change?

When mod_dav was written, it was assumed that the REPORT method was exclusive 
to https://tools.ietf.org/html/rfc3253. As a result, if a versioning 
implementation was present, the versioning implementation (eg svn) was expected 
to handle REPORT, and no other.

Other WebDAV extensions arrived, and they also used REPORT. But if a versioning 
implementation was present, that implementation would consume the report, not 
recognise the report, and then return an error. The mod_caldav code that Nokia 
worked on in the late 2000s hacked around this by reading the REPORT body 
first, then creating an input filter to re-insert the body if mod_caldav 
realised the REPORT wasn’t for it. Parsing XML bodies over and over is very 
ugly.

What this change does is formally recognise in code that multiple RFCs handle 
the REPORT method. We parse the XML body, then offer this parsed body to anyone 
who wants it via the deliver_report hook. Modules get their chance to handle 
the report. As a last resort, the mod_dav core passes the body to the 
versioning implementaton (eg svn) which then responds as previous, maintaining 
existing behaviour.

To answer you direct question above, at the point we return 
HTTP_NOT_IMPLEMENTED we have consumed the REPORT body. We can’t return DECLINED 
in this case.

There are a significant number of RFCs that mod_dav doesn't support, such as 
https://tools.ietf.org/html/rfc5689. We have RFC compliance work to do.

Regards,
Graham
—

Re: Still Failing: apache/httpd#896 (trunk - 9af2218)

[Graham Leggett]

On 29 Jun 2020, at 09:19, Joe Orton  wrote:

> The litmus tests are failing, not the perl-framework tests:
> 
> https://travis-ci.org/github/apache/httpd/jobs/702768269#L2491

Ah - that’s where  I was going wrong.

> You can also see that there are segfaults logged from line 2519 onwards:
> 
> https://travis-ci.org/github/apache/httpd/jobs/702768269#L2519
> 
> Running litmus locally, the backtrace looks like this:

So the simple workaround is to be defensive against ctx->r being NULL, but I 
need to check - is there ever a legitimate reason for there to be a filled out 
ctx structure including a filename, but with no request?

Process 9883 stopped
* thread #62, stop reason = EXC_BAD_ACCESS (code=1, address=0x58)
frame #0: 0x000100203e09 
httpd`dav_fs_getetag(resource=0x00010106f1e0) at repos.c:1869:31
   1866 }
   1867 
   1868 er.vlist_validator = NULL;
-> 1869 er.request_time = ctx->r->request_time;
   1870 er.finfo = >finfo;
   1871 er.pathname = ctx->pathname;
   1872 er.fd = NULL;
Target 0: (httpd) stopped.
(lldb) print ctx
(dav_resource_private *) $0 = 0x00010106f110
(lldb) print *ctx
(dav_resource_private) $1 = {
  pool = 0x000105098628
  pathname = 0x00010106f198 
"/Users/minfrin/src/apache/sandbox/proxy/htdocs/dav/litmus/lockcoll"
  finfo = {
pool = 0x000105098628
valid = 7598960
protection = 1877
filetype = APR_DIR
user = 501
group = 20
inode = 61244902
device = 16777220
nlink = 2
size = 64
csize = 0
atime = 159342121800
mtime = 159342121800
ctime = 159342121800
fname = 0x00010106f198 
"/Users/minfrin/src/apache/sandbox/proxy/htdocs/dav/litmus/lockcoll"
name = 0x
filehand = 0x
  }
  r = 0x
}
(lldb) 

Regards,
Graham
—