Re: svn commit: r1828926 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_proxy.xml include/ap_mmn.h modules/proxy/mod_proxy.c modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c

[Yann Ylavic]

On Wed, Apr 11, 2018 at 10:05 PM, Yann Ylavic  wrote:
> On Wed, Apr 11, 2018 at 9:14 PM, Eric Covener  wrote:
>>> --- httpd/httpd/trunk/modules/proxy/mod_proxy.h (original)
>>> +++ httpd/httpd/trunk/modules/proxy/mod_proxy.h Wed Apr 11 19:11:52 2018
>>> @@ -459,6 +459,8 @@ typedef struct {
>>>  char  secret[PROXY_WORKER_MAX_SECRET_SIZE]; /* authentication 
>>> secret (e.g. AJP13) */
>>>  char  upgrade[PROXY_WORKER_MAX_SCHEME_SIZE];/* upgrade protocol 
>>> used by mod_proxy_wstunnel */
>>>  char  hostname_ex[PROXY_RFC1035_HOSTNAME_SIZE];  /* RFC1035 
>>> compliant version of the remote backend address */
>>> +apr_size_t   response_field_size; /* Size of proxy response buffer in 
>>> bytes. */
>>> +unsigned int response_field_size_set:1;
>>>  } proxy_worker_shared;
>>
>>
>> If this is for trunk only, should I move the bit field up and call it
>> major?  I don't plan to backport it.
>
> Maybe the backport is needed to resolve PR 62196 altogether?

Scratch that, this commit doesn't fix the case where '\n' is within
the ENOSPC brigade but only comments on the issue.

Re: 2.4.3x regression w/SSL vhost configs

[Yann Ylavic]

On Wed, Apr 11, 2018 at 7:54 PM, Joe Orton  wrote:
> On Wed, Apr 11, 2018 at 01:37:22PM -0400, Eric Covener wrote:
>> On Wed, Apr 11, 2018 at 1:07 PM, Yann Ylavic  wrote:
>> > On Wed, Apr 11, 2018 at 7:03 PM, Joe Orton  wrote:
>> >> Like this?  Is this likely to break some other currently-working config?
>> >>
>> >> Index: modules/ssl/ssl_engine_init.c
>> >> ===
>> >> --- modules/ssl/ssl_engine_init.c   (revision 1828914)
>> >> +++ modules/ssl/ssl_engine_init.c   (working copy)
>> >> @@ -261,7 +261,8 @@
>> >>   * the protocol is https. */
>> >>  if (ap_get_server_protocol(s)
>> >>  && strcmp("https", ap_get_server_protocol(s)) == 0
>> >> -&& sc->enabled == SSL_ENABLED_UNSET) {
>> >> +&& sc->enabled == SSL_ENABLED_UNSET
>> >> +&& (!apr_is_empty_array(sc->server->pks->cert_files))) {
>> >>  sc->enabled = SSL_ENABLED_TRUE;
>> >>  }
>> >
>> > So now your configuration would work because the second vhost wouldn't
>> > have SSL enabled?
>> > But doesn't the user want SSL on this vhost in the first place?
>>
>> If they worked before, it seems like they were relying on a handshake
>> with the default VH for the NVH -- which they still get?
>
> Yes, exactly - and for affected configs the defining feature is the
> absence of SSL* in the second vhost.  The non-SSL config still takes
> effect as before.

Does it still work with SNI sent by the client (i.e. when negotiation
should be based on the second NVH's SSL config)?

>
> This seems to work for the trivial test cases I have based off user
> reports, but I'm worried this is going to based some other case for
> which the implicit-on is still needed.

Maybe the test could be based off the "base server" (read future
c->base_server, or first of the NVH, not the base_server pointer in
ssl_init_Module() which is really the main server) if we could
determine that at ssl_init_Module() time? Something like
(!apr_is_empty_array(sc->server->pks->cert_files) || "base
server"->sc->enabled), but I don't see another example where "base
server" is determined/needed at load time...

>
> Is mod_md expected to work for vhosts without "SSLEngine on/optional"
> configured explicitly?  Didn't get a clear answer to this before.

Dunno, but wouldn't be worried to much is that were a new requirement
for it to work explicitely.


Regards,
Yann.

Re: svn commit: r1828926 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_proxy.xml include/ap_mmn.h modules/proxy/mod_proxy.c modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c

[Yann Ylavic]

On Wed, Apr 11, 2018 at 9:14 PM, Eric Covener  wrote:
>> --- httpd/httpd/trunk/modules/proxy/mod_proxy.h (original)
>> +++ httpd/httpd/trunk/modules/proxy/mod_proxy.h Wed Apr 11 19:11:52 2018
>> @@ -459,6 +459,8 @@ typedef struct {
>>  char  secret[PROXY_WORKER_MAX_SECRET_SIZE]; /* authentication 
>> secret (e.g. AJP13) */
>>  char  upgrade[PROXY_WORKER_MAX_SCHEME_SIZE];/* upgrade protocol 
>> used by mod_proxy_wstunnel */
>>  char  hostname_ex[PROXY_RFC1035_HOSTNAME_SIZE];  /* RFC1035 
>> compliant version of the remote backend address */
>> +apr_size_t   response_field_size; /* Size of proxy response buffer in 
>> bytes. */
>> +unsigned int response_field_size_set:1;
>>  } proxy_worker_shared;
>
>
> If this is for trunk only, should I move the bit field up and call it
> major?  I don't plan to backport it.

Maybe the backport is needed to resolve PR 62196 altogether?

> Whether I move it or not, should I reserve the next range of bytes after it?

Would be nice to rearrange the struct for trunk/2.next.
As for bitfields I'm not sure it helps reserving names for unused bits
since we can't extend them in stable versions anyway (I wish we could
given that it doesn't change any size/address and bits themselves have
no address, but IIRC from an earlier discussion with Bill it's not an
option). It doesn't prevent us from joining the existing ones for
2.next, though.
So +1 for me, including for an overall (optimized for size/holes)
moving of fields.

Re: svn commit: r1828926 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_proxy.xml include/ap_mmn.h modules/proxy/mod_proxy.c modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c

[Eric Covener]

> --- httpd/httpd/trunk/modules/proxy/mod_proxy.h (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy.h Wed Apr 11 19:11:52 2018
> @@ -459,6 +459,8 @@ typedef struct {
>  char  secret[PROXY_WORKER_MAX_SECRET_SIZE]; /* authentication secret 
> (e.g. AJP13) */
>  char  upgrade[PROXY_WORKER_MAX_SCHEME_SIZE];/* upgrade protocol used 
> by mod_proxy_wstunnel */
>  char  hostname_ex[PROXY_RFC1035_HOSTNAME_SIZE];  /* RFC1035 
> compliant version of the remote backend address */
> +apr_size_t   response_field_size; /* Size of proxy response buffer in 
> bytes. */
> +unsigned int response_field_size_set:1;
>  } proxy_worker_shared;


If this is for trunk only, should I move the bit field up and call it
major?  I don't plan to backport it.
Whether I move it or not, should I reserve the next range of bytes after it?

Re: TLSv1.3

[Bernard Spil]

Sorry William, didn't mean to disturb you. Just noticed some 2.5.1 and
2.5.1-dev strings appearing in commits.

Hope the project will roll another 2.5-dev soon. OpenSSL 1.1.1 is
nearing and a TLSv1.3 Apache server to go with that would be most
excellent!

Thanks! Bernard.

2018-04-10 17:35 GMT+02:00 William A Rowe Jr :
> On Sun, Apr 8, 2018 at 11:37 AM, Bernard Spil  wrote:
>> Hi Stefan, Mario,
>>
>> I saw that 2.5.1-dev was tagged, is another release coming some time soon?
>
> Worried me enough to look; http://svn.apache.org/repos/asf/httpd/httpd/tags/
>
> Thankfully nobody made such a tag. You'll note 2.5.0-alpha from a number
> of months ago; this was the first in development of new tag and release
> tooling, and as such it never made the cut. No word at this moment of the
> next 2.5.1-alpha attempt.

Re: 2.4.3x regression w/SSL vhost configs

[Joe Orton]

On Wed, Apr 11, 2018 at 01:37:22PM -0400, Eric Covener wrote:
> On Wed, Apr 11, 2018 at 1:07 PM, Yann Ylavic  wrote:
> > On Wed, Apr 11, 2018 at 7:03 PM, Joe Orton  wrote:
> >> Like this?  Is this likely to break some other currently-working config?
> >>
> >> Index: modules/ssl/ssl_engine_init.c
> >> ===
> >> --- modules/ssl/ssl_engine_init.c   (revision 1828914)
> >> +++ modules/ssl/ssl_engine_init.c   (working copy)
> >> @@ -261,7 +261,8 @@
> >>   * the protocol is https. */
> >>  if (ap_get_server_protocol(s)
> >>  && strcmp("https", ap_get_server_protocol(s)) == 0
> >> -&& sc->enabled == SSL_ENABLED_UNSET) {
> >> +&& sc->enabled == SSL_ENABLED_UNSET
> >> +&& (!apr_is_empty_array(sc->server->pks->cert_files))) {
> >>  sc->enabled = SSL_ENABLED_TRUE;
> >>  }
> >
> > So now your configuration would work because the second vhost wouldn't
> > have SSL enabled?
> > But doesn't the user want SSL on this vhost in the first place?
> 
> If they worked before, it seems like they were relying on a handshake
> with the default VH for the NVH -- which they still get?

Yes, exactly - and for affected configs the defining feature is the 
absence of SSL* in the second vhost.  The non-SSL config still takes 
effect as before.

This seems to work for the trivial test cases I have based off user 
reports, but I'm worried this is going to based some other case for 
which the implicit-on is still needed.

Is mod_md expected to work for vhosts without "SSLEngine on/optional" 
configured explicitly?  Didn't get a clear answer to this before.

Regards, Joe

Re: 2.4.3x regression w/SSL vhost configs

[Eric Covener]

On Wed, Apr 11, 2018 at 1:07 PM, Yann Ylavic  wrote:
> On Wed, Apr 11, 2018 at 7:03 PM, Joe Orton  wrote:
>> Like this?  Is this likely to break some other currently-working config?
>>
>> Index: modules/ssl/ssl_engine_init.c
>> ===
>> --- modules/ssl/ssl_engine_init.c   (revision 1828914)
>> +++ modules/ssl/ssl_engine_init.c   (working copy)
>> @@ -261,7 +261,8 @@
>>   * the protocol is https. */
>>  if (ap_get_server_protocol(s)
>>  && strcmp("https", ap_get_server_protocol(s)) == 0
>> -&& sc->enabled == SSL_ENABLED_UNSET) {
>> +&& sc->enabled == SSL_ENABLED_UNSET
>> +&& (!apr_is_empty_array(sc->server->pks->cert_files))) {
>>  sc->enabled = SSL_ENABLED_TRUE;
>>  }
>
> So now your configuration would work because the second vhost wouldn't
> have SSL enabled?
> But doesn't the user want SSL on this vhost in the first place?

If they worked before, it seems like they were relying on a handshake
with the default VH for the NVH -- which they still get?




-- 
Eric Covener
cove...@gmail.com

Re: 2.4.3x regression w/SSL vhost configs

[Yann Ylavic]

On Wed, Apr 11, 2018 at 7:03 PM, Joe Orton  wrote:
> Like this?  Is this likely to break some other currently-working config?
>
> Index: modules/ssl/ssl_engine_init.c
> ===
> --- modules/ssl/ssl_engine_init.c   (revision 1828914)
> +++ modules/ssl/ssl_engine_init.c   (working copy)
> @@ -261,7 +261,8 @@
>   * the protocol is https. */
>  if (ap_get_server_protocol(s)
>  && strcmp("https", ap_get_server_protocol(s)) == 0
> -&& sc->enabled == SSL_ENABLED_UNSET) {
> +&& sc->enabled == SSL_ENABLED_UNSET
> +&& (!apr_is_empty_array(sc->server->pks->cert_files))) {
>  sc->enabled = SSL_ENABLED_TRUE;
>  }

So now your configuration would work because the second vhost wouldn't
have SSL enabled?
But doesn't the user want SSL on this vhost in the first place?

Re: 2.4.3x regression w/SSL vhost configs

[Yann Ylavic]

On Wed, Apr 11, 2018 at 6:41 PM, Joe Orton  wrote:
> On Wed, Apr 11, 2018 at 05:49:47PM +0200, Yann Ylavic wrote:
>> I agree... to both Stefan's and your points of view here :p
>
> YOU FENCE SITTER! :)

:D

>
> I feel like it should be possible to restore the old behaviour simply by
> disabling the implicit-SSLEngine-on in the cases where we'd never get a
> separate SSLSrvConfigRec before.
>
> e.g. could we suppress default-on if pks->cert_files is empty?  (plus
> some mod_md fudge factor??)

I'm not sure to understand how this'd help, there may still be
multiple vhosts with mod_md.
I'd like this approach if it works, but don't see the link for now.

As for my proposal, maybe the other way around then: for 2.4.x, we
could require that mod_md's LoadModule precedes mod_ssl's so that it
can reset ap_module_flags_umask before (ap_module_flags_umask would be
internal only, defaulting to -1).
Since mod_md is experimental, maybe we can afford this requirement...

Re: 2.4.3x regression w/SSL vhost configs

[Joe Orton]

Like this?  Is this likely to break some other currently-working config?

Index: modules/ssl/ssl_engine_init.c
===
--- modules/ssl/ssl_engine_init.c   (revision 1828914)
+++ modules/ssl/ssl_engine_init.c   (working copy)
@@ -261,7 +261,8 @@
  * the protocol is https. */
 if (ap_get_server_protocol(s) 
 && strcmp("https", ap_get_server_protocol(s)) == 0
-&& sc->enabled == SSL_ENABLED_UNSET) {
+&& sc->enabled == SSL_ENABLED_UNSET
+&& (!apr_is_empty_array(sc->server->pks->cert_files))) {
 sc->enabled = SSL_ENABLED_TRUE;
 }
 

Re: 2.4.3x regression w/SSL vhost configs

[Joe Orton]

On Wed, Apr 11, 2018 at 05:49:47PM +0200, Yann Ylavic wrote:
> I agree... to both Stefan's and your points of view here :p

YOU FENCE SITTER! :)

> We can hardly break existing configurations, even if they rely on a
> bug (our bug...).
> But another user (or the same!) may open a bug when her/his
> SSLCertificate stops working as soon as any other SSL directive is
> added to the second vhost (as explained by Stefan), and we should also
> fix it.
> 
> Maybe we need a global EXEC_ON_READ setting (before any vhost) to
> control/ignore AP_MODULE_FLAG_ALWAYS_MERGE?
> Otherwise broken users are stuck with mod_ssl < 2.4.33 (which may be
> also an option for dubious compatibility issues).

Seems like a poor compromise since we get lumbered with maintaining 
another config option and users will still have to change their configs 
after upgrading to 2.4.33.

I feel like it should be possible to restore the old behaviour simply by 
disabling the implicit-SSLEngine-on in the cases where we'd never get a 
separate SSLSrvConfigRec before.

e.g. could we suppress default-on if pks->cert_files is empty?  (plus 
some mod_md fudge factor??)

Regards, Joe

Re: 2.4.3x regression w/SSL vhost configs

[Yann Ylavic]

On Wed, Apr 11, 2018 at 5:49 PM, Yann Ylavic  wrote:
> Maybe we need a global EXEC_ON_READ setting (before any vhost) to
> control/ignore AP_MODULE_FLAG_ALWAYS_MERGE?

Something like the attached possibly...
Index: include/http_config.h
===
--- include/http_config.h	(revision 1828882)
+++ include/http_config.h	(working copy)
@@ -545,6 +545,12 @@ AP_DECLARE(void) ap_set_module_config(ap_conf_vect
  */
 AP_DECLARE(int) ap_get_module_flags(const module *m);
 
+/**
+ * Module flags to be ignored
+ * @var module ap_module_flags_umask
+ */
+AP_DECLARE_DATA extern int ap_module_flags_umask;
+
 #if !defined(AP_DEBUG)
 
 #define ap_get_module_config(v,m)   \
Index: server/core.c
===
--- server/core.c	(revision 1828882)
+++ server/core.c	(working copy)
@@ -4018,6 +4018,21 @@ static const char *set_http_method(cmd_parms *cmd,
 return NULL;
 }
 
+static const char *set_module_flags_umask(cmd_parms *cmd, void *dummy,
+  const char *arg)
+{
+long umask;
+char *end;
+
+umask = strtol(arg, , 0);
+if (*end || (umask & ~APR_INT32_MIN)) {
+return "Invalid ModuleFlagsUmask";
+}
+
+ap_module_flags_umask = (int)umask;
+return NULL;
+}
+
 static apr_hash_t *errorlog_hash;
 
 static int log_constant_item(const ap_errorlog_info *info, const char *arg,
@@ -4551,6 +4566,8 @@ AP_INIT_ITERATE("HttpProtocolOptions", set_http_pr
 "'Unsafe' or 'Strict' (default). Sets HTTP acceptance rules"),
 AP_INIT_ITERATE("RegisterHttpMethod", set_http_method, NULL, RSRC_CONF,
 "Registers non-standard HTTP methods"),
+AP_INIT_TAKE1("ModuleFlagsUmask", set_module_flags_umask, NULL, RSRC_CONF | EXEC_ON_READ,
+  "Umask applied to module flags"),
 { NULL }
 };
 
Index: server/util_debug.c
===
--- server/util_debug.c	(revision 1828882)
+++ server/util_debug.c	(working copy)
@@ -34,6 +34,7 @@
 #undef strstr
 #endif
 
+AP_DECLARE_DATA int ap_module_flags_umask;
 
 #if defined(ap_strchr)
 #undef ap_strchr
@@ -115,7 +116,7 @@ AP_DECLARE(int) ap_get_module_flags(const module *
 return 0;
 }
 
-return m->flags;
+return m->flags & ~ap_module_flags_umask;
 }
 
 #if defined(ap_get_core_module_config)

Re: 2.4.3x regression w/SSL vhost configs

[Yann Ylavic]

On Wed, Apr 11, 2018 at 4:56 PM, Joe Orton  wrote:
> On Wed, Apr 11, 2018 at 02:10:27PM +0200, Stefan Eissing wrote:
>> What we fixed here is a bug, plain and simple. And if installations rely
>> on a bug, they might break on an update. Seems unavoidable.
>>
>> Nowhere is this "a certificate is visible in other vhosts if it is 
>> configured in the
>> first vhost and the other have no own SSL configuration" documented or even 
>> specified.
>> Quite the opposite, I think.
>
> I certainly don't find it plain or simple and if that's true for me
> I'd bet at least some mod_ssl users are even worse off!
> (I had two bug reports from Fedora users already in the few days after
> pushing the 2.4.33 update)
>
> Given that:
>
> a) the configs worked forever in 2.4, *and*
> b) we had to extend the module struct (!!) and a core merging function (!!!) 
> to
> change the behaviour, *and*
> c) there was never any warning for this config
>
> ...I think it quite uncharitable to our users to argue this is not a
> regression because
> users should have known the old behaviour is a "bug".

I agree... to both Stefan's and your points of view here :p
We can hardly break existing configurations, even if they rely on a
bug (our bug...).
But another user (or the same!) may open a bug when her/his
SSLCertificate stops working as soon as any other SSL directive is
added to the second vhost (as explained by Stefan), and we should also
fix it.

Maybe we need a global EXEC_ON_READ setting (before any vhost) to
control/ignore AP_MODULE_FLAG_ALWAYS_MERGE?
Otherwise broken users are stuck with mod_ssl < 2.4.33 (which may be
also an option for dubious compatibility issues).


Regards,
Yann.

Re: Future of hot standby in balancers [was Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/]

[Jim Jagielski]

It's been awhile but I think hot standbys were added before we came
up with the idea of lb sets... And since people were using hot
standbys, we didn't want to force them to change their configs.

We can drop hot-standbys in trunk/2.5/2.6, but we'll need to keep
them in 2.4.

> On Apr 11, 2018, at 10:57 AM, Jim Riggs  wrote:
> 
> On 11 Apr 2018, at 07:11, jhri...@apache.org wrote:
>> 
>> Author: jhriggs
>> Date: Wed Apr 11 12:11:05 2018
>> New Revision: 1828890
>> 
>> URL: http://svn.apache.org/viewvc?rev=1828890=rev
>> Log:
>> mod_proxy_balancer: Add hot spare member type and corresponding flag (R). 
>> Hot spare members are
>> used as drop-in replacements for unusable workers in the same load balancer 
>> set. This differs
>> from hot standbys which are only used when all workers in a set are 
>> unusable. PR 61140.
> 
> Speaking of balancer member types, hot spares (new), and hot standbys 
> (existing), is there really a need for hot standbys? When I first started 
> using mod_proxy_balancer many years ago, I actually thought/assumed that "hot 
> standbys" had the behavior that the new spares now have (i.e. drop-in 
> replacements for unavailable workers) until I studied the documentation more 
> carefully.
> 
> Standbys are really superfluous, since the same behavior can be achieved by 
> sets. For example, the behavior of the following will be exactly the same:
> 
> 
>  ProxySet lbmethod=bytraffic
>  BalancerMember "http://server1/; lbset=0
>  BalancerMember "http://server2/; lbset=0
>  BalancerMember "http://server3/; lbset=0 status=+H
>  BalancerMember "http://server4/; lbset=1
>  BalancerMember "http://server5/; lbset=1
>  BalancerMember "http://server6/; lbset=1 status=+H
> 
> 
> 
>  ProxySet lbmethod=bytraffic
>  BalancerMember "http://server1/; lbset=0
>  BalancerMember "http://server2/; lbset=0
>  BalancerMember "http://server3/; lbset=1
>  BalancerMember "http://server4/; lbset=2
>  BalancerMember "http://server5/; lbset=2
>  BalancerMember "http://server6/; lbset=3
> 
> 
> So, is there really any reason to keep hot standbys? Do we consider the 
> concept of the +H flag to be easier to understand or use than LB sets? If so, 
> do we just need better docs? Am I missing any nuances of the H flag?
> 
> I would love to see standbys go away, personally.
> 
> - Jim
> 

Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/

[Yann Ylavic]

I did not advocate refactor for refactoring sake, was just a remark
about (possibly) staging "big" changes.
Here for instance ap_proxy_balancer_get_best_worker() refactor could
have been done on the existing code first, and then the code needed
for hot spare members be added to that common function next.

That could have helped both review and bissection: if something stops
working, does it come from the refactor or the new feature?
But I'm not asking to revert and redo staged commits though, will try
to figure out ;)


On Wed, Apr 11, 2018 at 4:23 PM, Jim Jagielski  wrote:
> +1 on keeping the patch as is and not breaking it out as a "refactor" plus
> a new feature.
>
> We have seen WAY too many cases where a refactor has caused
> issues. I'm not saying we shouldn't fix things, but major refactors
> should, IMO, have a real-world need, and tangible improvement,
> other than "I want it to work this way."
>
> Too often people just refactor for refactoring sake. That's not a
> Good Idea, IMO.
>
>
> On Apr 11, 2018, at 10:09 AM, Jim Riggs  wrote:
>
> On 11 Apr 2018, at 08:28, Yann Ylavic  wrote:
>
> On Wed, Apr 11, 2018 at 2:11 PM,   wrote:
>
> Author: jhriggs
> Date: Wed Apr 11 12:11:05 2018
> New Revision: 1828890
>
> URL: http://svn.apache.org/viewvc?rev=1828890=rev
> Log:
> mod_proxy_balancer: Add hot spare member type and corresponding flag (R).
> Hot spare members are
> used as drop-in replacements for unusable workers in the same load balancer
> set. This differs
> from hot standbys which are only used when all workers in a set are
> unusable. PR 61140.
>
>
> Nice ap_proxy_balancer_get_best_worker() simplification Jim.
>
> Maybe it could have been a separate commit than the spare members
> addition though, not mixing refactoring and features.
> Staging helps review IHMO.
>
>
> Thanks for the feedback, Yann!
>
> I never really viewed ap_proxy_balancer_get_best_worker() as a refactor
> separate from the hot spare change that required it, though I suppose it
> definitely could have stood alone. It did remove a lot of duplicate code.
> Basically, that commit is just an existing patch that has been out there
> floating around since ACNA last year that I just updated to the current
> codebase. In the dev@ thread at the time, we never discussed splitting them
> either.
>
> Regardless, I'm glad to have the hot spare functionality that several of us
> have always thought was missing, and I'm glad I didn't appear to b0rk the
> entire repo in the process.
>
> Thanks for taking it easy on the new guy. ;-)
>
> - Jim
>
>

Future of hot standby in balancers [was Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/]

[Jim Riggs]

On 11 Apr 2018, at 07:11, jhri...@apache.org wrote:
> 
> Author: jhriggs
> Date: Wed Apr 11 12:11:05 2018
> New Revision: 1828890
> 
> URL: http://svn.apache.org/viewvc?rev=1828890=rev
> Log:
> mod_proxy_balancer: Add hot spare member type and corresponding flag (R). Hot 
> spare members are
> used as drop-in replacements for unusable workers in the same load balancer 
> set. This differs
> from hot standbys which are only used when all workers in a set are unusable. 
> PR 61140.

Speaking of balancer member types, hot spares (new), and hot standbys 
(existing), is there really a need for hot standbys? When I first started using 
mod_proxy_balancer many years ago, I actually thought/assumed that "hot 
standbys" had the behavior that the new spares now have (i.e. drop-in 
replacements for unavailable workers) until I studied the documentation more 
carefully.

Standbys are really superfluous, since the same behavior can be achieved by 
sets. For example, the behavior of the following will be exactly the same:


  ProxySet lbmethod=bytraffic
  BalancerMember "http://server1/; lbset=0
  BalancerMember "http://server2/; lbset=0
  BalancerMember "http://server3/; lbset=0 status=+H
  BalancerMember "http://server4/; lbset=1
  BalancerMember "http://server5/; lbset=1
  BalancerMember "http://server6/; lbset=1 status=+H



  ProxySet lbmethod=bytraffic
  BalancerMember "http://server1/; lbset=0
  BalancerMember "http://server2/; lbset=0
  BalancerMember "http://server3/; lbset=1
  BalancerMember "http://server4/; lbset=2
  BalancerMember "http://server5/; lbset=2
  BalancerMember "http://server6/; lbset=3


So, is there really any reason to keep hot standbys? Do we consider the concept 
of the +H flag to be easier to understand or use than LB sets? If so, do we 
just need better docs? Am I missing any nuances of the H flag?

I would love to see standbys go away, personally.

- Jim

Re: 2.4.3x regression w/SSL vhost configs

[Joe Orton]

On Wed, Apr 11, 2018 at 02:10:27PM +0200, Stefan Eissing wrote:
> What we fixed here is a bug, plain and simple. And if installations rely
> on a bug, they might break on an update. Seems unavoidable.
>
> Nowhere is this "a certificate is visible in other vhosts if it is configured 
> in the
> first vhost and the other have no own SSL configuration" documented or even 
> specified.
> Quite the opposite, I think.

I certainly don't find it plain or simple and if that's true for me
I'd bet at least some mod_ssl users are even worse off!
(I had two bug reports from Fedora users already in the few days after
pushing the 2.4.33 update)

Given that:

a) the configs worked forever in 2.4, *and*
b) we had to extend the module struct (!!) and a core merging function (!!!) to
change the behaviour, *and*
c) there was never any warning for this config

...I think it quite uncharitable to our users to argue this is not a
regression because
users should have known the old behaviour is a "bug".

Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/

[Jim Jagielski]

+1 on keeping the patch as is and not breaking it out as a "refactor" plus
a new feature.

We have seen WAY too many cases where a refactor has caused
issues. I'm not saying we shouldn't fix things, but major refactors
should, IMO, have a real-world need, and tangible improvement,
other than "I want it to work this way."

Too often people just refactor for refactoring sake. That's not a
Good Idea, IMO.

> On Apr 11, 2018, at 10:09 AM, Jim Riggs  wrote:
> 
>> On 11 Apr 2018, at 08:28, Yann Ylavic  wrote:
>> 
>> On Wed, Apr 11, 2018 at 2:11 PM,   wrote:
>>> Author: jhriggs
>>> Date: Wed Apr 11 12:11:05 2018
>>> New Revision: 1828890
>>> 
>>> URL: http://svn.apache.org/viewvc?rev=1828890=rev
>>> Log:
>>> mod_proxy_balancer: Add hot spare member type and corresponding flag (R). 
>>> Hot spare members are
>>> used as drop-in replacements for unusable workers in the same load balancer 
>>> set. This differs
>>> from hot standbys which are only used when all workers in a set are 
>>> unusable. PR 61140.
>> 
>> Nice ap_proxy_balancer_get_best_worker() simplification Jim.
>> 
>> Maybe it could have been a separate commit than the spare members
>> addition though, not mixing refactoring and features.
>> Staging helps review IHMO.
> 
> Thanks for the feedback, Yann!
> 
> I never really viewed ap_proxy_balancer_get_best_worker() as a refactor 
> separate from the hot spare change that required it, though I suppose it 
> definitely could have stood alone. It did remove a lot of duplicate code. 
> Basically, that commit is just an existing patch that has been out there 
> floating around since ACNA last year that I just updated to the current 
> codebase. In the dev@ thread at the time, we never discussed splitting them 
> either.
> 
> Regardless, I'm glad to have the hot spare functionality that several of us 
> have always thought was missing, and I'm glad I didn't appear to b0rk the 
> entire repo in the process.
> 
> Thanks for taking it easy on the new guy. ;-)
> 
> - Jim

Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/

[Jim Riggs]

> On 11 Apr 2018, at 08:28, Yann Ylavic  wrote:
> 
> On Wed, Apr 11, 2018 at 2:11 PM,   wrote:
>> Author: jhriggs
>> Date: Wed Apr 11 12:11:05 2018
>> New Revision: 1828890
>> 
>> URL: http://svn.apache.org/viewvc?rev=1828890=rev
>> Log:
>> mod_proxy_balancer: Add hot spare member type and corresponding flag (R). 
>> Hot spare members are
>> used as drop-in replacements for unusable workers in the same load balancer 
>> set. This differs
>> from hot standbys which are only used when all workers in a set are 
>> unusable. PR 61140.
> 
> Nice ap_proxy_balancer_get_best_worker() simplification Jim.
> 
> Maybe it could have been a separate commit than the spare members
> addition though, not mixing refactoring and features.
> Staging helps review IHMO.

Thanks for the feedback, Yann!

I never really viewed ap_proxy_balancer_get_best_worker() as a refactor 
separate from the hot spare change that required it, though I suppose it 
definitely could have stood alone. It did remove a lot of duplicate code. 
Basically, that commit is just an existing patch that has been out there 
floating around since ACNA last year that I just updated to the current 
codebase. In the dev@ thread at the time, we never discussed splitting them 
either.

Regardless, I'm glad to have the hot spare functionality that several of us 
have always thought was missing, and I'm glad I didn't appear to b0rk the 
entire repo in the process.

Thanks for taking it easy on the new guy. ;-)

- Jim

Re: svn commit: r1828890 - in /httpd/httpd/trunk: ./ docs/log-message-tags/ docs/manual/howto/ docs/manual/mod/ modules/proxy/ modules/proxy/balancers/

[Yann Ylavic]

On Wed, Apr 11, 2018 at 2:11 PM,   wrote:
> Author: jhriggs
> Date: Wed Apr 11 12:11:05 2018
> New Revision: 1828890
>
> URL: http://svn.apache.org/viewvc?rev=1828890=rev
> Log:
> mod_proxy_balancer: Add hot spare member type and corresponding flag (R). Hot 
> spare members are
> used as drop-in replacements for unusable workers in the same load balancer 
> set. This differs
> from hot standbys which are only used when all workers in a set are unusable. 
> PR 61140.

Nice ap_proxy_balancer_get_best_worker() simplification Jim.

Maybe it could have been a separate commit than the spare members
addition though, not mixing refactoring and features.
Staging helps review IHMO.

Thanks anyway,
Yann.

Re: 2.4.3x regression w/SSL vhost configs

[Stefan Eissing]

> Am 11.04.2018 um 13:57 schrieb Joe Orton :
> 
> Thanks for the responses, Stefan.  I understand this a bit better now, 
> but it still seems very clearly like a regression.
> 
> On Wed, Apr 11, 2018 at 10:28:05AM +0200, Stefan Eissing wrote:
>> Additional example of the brokeness. If you add
>> a SSL directive to the 2nd vhost in your example
>> - without the new MERGE feature:
> ..
>>> Listen 443 https
>>> 
>>> 
>>>  ServerName www.example.com:443
>>>  SSLCertificateFile ...
>>> 
>>> 
>>> 
>>>  ServerName other.example.com:443
>>> SSLEngine on
>>> 
>> 
>> the certificate is also no longer visible there. 
> 
> That configuration would fail in 2.4.29 as well, so there is no 
> regression in that case.
> 
> The key issue seems to be the implicit "SSLEngine on" for a vhost where 
> that would not happen previously.  Is that behaviour necessary to make 
> mod_md work or is it simply fall-out from the change in merging?  (i.e. 
> can revert that behaviour alone)

For mod_md to work correctly, mod_ssl needs a unique config record instance
internally for any vhost. This is also necessary if mod_ssl wanted to add/remove
to vhost configs in the post_config phase. mod_md is only the first to trigger
such changes. This has nothing to do with ACME or Let's Encrypt per se.

What we fixed here is a bug, plain and simple. And if installations rely
on a bug, they might break on an update. Seems unavoidable.

Nowhere is this "a certificate is visible in other vhosts if it is configured 
in the
first vhost and the other have no own SSL configuration" documented or even 
specified.
Quite the opposite, I think.

My view on things, others might differ. Please correct me where I'm wrong here.

Cheers,

Stefan

Re: 2.4.3x regression w/SSL vhost configs

[Joe Orton]

Thanks for the responses, Stefan.  I understand this a bit better now, 
but it still seems very clearly like a regression.

On Wed, Apr 11, 2018 at 10:28:05AM +0200, Stefan Eissing wrote:
> Additional example of the brokeness. If you add
> a SSL directive to the 2nd vhost in your example
> - without the new MERGE feature:
..
> > Listen 443 https
> > 
> > 
> >   ServerName www.example.com:443
> >   SSLCertificateFile ...
> > 
> > 
> > 
> >   ServerName other.example.com:443
> >  SSLEngine on
> > 
> 
> the certificate is also no longer visible there. 

That configuration would fail in 2.4.29 as well, so there is no 
regression in that case.

The key issue seems to be the implicit "SSLEngine on" for a vhost where 
that would not happen previously.  Is that behaviour necessary to make 
mod_md work or is it simply fall-out from the change in merging?  (i.e. 
can revert that behaviour alone)

Regards, Joe

Re: svn commit: r1828883 - /httpd/httpd/branches/2.4.x/

[Yann Ylavic]

On Wed, Apr 11, 2018 at 1:27 PM, Marion et Christophe JAILLET
 wrote:
> Trying before proposing is always a good practice.

Yes of course, was a joke, one even said to reviewing commits was good :)

>
>svn merge --dry-run
>
> can help in this situation.

Thanks! Never thought about it, will help *me* for sure.


Regards,
Yann.

Re: svn commit: r1828883 - /httpd/httpd/branches/2.4.x/

[Stefan Eissing]

Good hint. Unfortunately, I also try to compile it as well, sometimes even run 
tests... ;-)

> Am 11.04.2018 um 13:27 schrieb Marion et Christophe JAILLET 
> :
> 
> Trying before proposing is always a good practice.
> 
>svn merge --dry-run
> 
> can help in this situation.
> 
>  
> CJ
> 
>  
> > Message du 11/04/18 11:25
> > De : "Yann Ylavic" 
> > A : "httpd-dev" 
> > Copie à : 
> > Objet : Re: svn commit: r1828883 - /httpd/httpd/branches/2.4.x/
> > 
> > It happens when trying to do the real merge before proposing ;)
> > 
> > On Wed, Apr 11, 2018 at 11:18 AM, ste...@eissing.org  
> > wrote:
> > > urgs, sorry.
> > >
> > >> Am 11.04.2018 um 11:16 schrieb yla...@apache.org:
> > >>
> > >> Author: ylavic
> > >> Date: Wed Apr 11 09:16:56 2018
> > >> New Revision: 1828883
> > >>
> > >> URL: http://svn.apache.org/viewvc?rev=1828883=rev
> > >> Log:
> > >> Revert r1828882's mergeinfo.
> > >>
> > >> Modified:
> > >> httpd/httpd/branches/2.4.x/ (props changed)
> > >>
> > >> Propchange: httpd/httpd/branches/2.4.x/
> > >> --
> > >> --- svn:mergeinfo (original)
> > >> +++ svn:mergeinfo Wed Apr 11 09:16:56 2018
> > >> @@ -8,4 +8,4 @@
> > >> /httpd/httpd/branches/trunk-md:1804087-1804529
> > >> /httpd/httpd/branches/trunk-override-index:1793921-1793931
> > >> /httpd/httpd/branches/wombat-integration:723609-723841
> > >> -/httpd/httpd/trunk:1200475,1200478,1200482,1200491,1200496,1200513,1200550,1200556,1200580,1200605,1200612,1200614,1200639,1200646,1200656,1200667,1200679,1200699,1200702,1200955,1200957,1200961,1200963,1200968,1200975,1200977,1201032,1201042,120,1201194,1201198,1201202,1201443,1201450,1201460,1201956,1202236,1202453,1202456,1202886,1203400,1203491,1203632,1203714,1203859,1203980,1204630,1204968,1204990,1205061,1205075,1205379,1205885,1206291,1206472,1206587,1206850,1206940,1206978,1207719,1208753,1208835,1209053,1209085,1209417,1209432,1209461,1209601,1209603,1209618,1209623,1209741,1209754,1209766,1209776,1209797-1209798,1209811-1209812,1209814,1209908,1209910,1209913,1209916-1209917,1209947,1209952,1210067,1210080,1210120,1210124,1210130,1210148,1210219,1210221,1210252,1210284,1210336,1210378,1210725,1210892,1210951,1210954,1211351-1211352,1211364,1211490,1211495,1211528,1211663,1211680,1212872,1212883,1213338,1213380-1213381,1213391,1213399,1213567,1214003,1214005,1214015,12
> > >> 15514,1220462,1220467,1220493,1220524,1220570,1220768,1220794,1220826,1220846,1221205,1221292,1222335,1222370,1222473,1222915,1222917,1222921,1222930,1223048,1225060,1225197-1225199,1225223,1225380,1225476,1225478,1225791,1225795-1225796,1226339,1226375,1227910,1228700,1228816,1229024,1229059,1229099,1229116,1229134,1229136,1229930,1230286,1231255,1231257,1231442,1231446,1231508,1231510,1231518,1232575,1232594,1232630,1232838,1234180,1234297,1234479,1234511,1234565,1234574,1234642-1234643,1234876,1234899,1235019,1236122,1236701,1237407,1238545,1238768,1239029-1239030,1239071,1239565,1240315,1240470,1240778,1241069,1241071,1242089,1242798,1242967,1243176,1243246,1243797,1243799,1244211,1245717,1290823,1290835,1291819-1291820,1291834,1291840,1292043,1293405,1293534-1293535,1293658,1293678,1293708,1294306,1294349,1294356,1294358,1294372,1294471,1297560,1299718,1299786,1300766,130,1301725,1302444,1302483,1302653,1302665,1302674,1303201,1303435,1303827,1304087,1304874-1304875,1305167
> > >> ,1305586,1306350,1306409,1306426,1306841,1307790,1308327,1308459,1309536,1309567,1311468,1324760,1325218,1325227,1325250,1325265,1325275,1325632,1325724,1326980,1326984,1326991,1327689,1328325-1328326,1328339,1328345,1328950,1330189,1330964,1331110,1331115,1331942,1331977,1332378,1333969,1334343,1335882,1337344,1341906,1341913,1343085,1343087,1343094,1343099,1343109,1343935,1345319,1345329,1346905,1347980,1348036,1348653,1348656,1348660,1349905,1351012-1351020,1351071-1351072,1351074,1351737,1352047,1352534,1352909-1352912,1357685,1358061,1359057,1359881,1359884,1361153,1361298,1361766,1361773,1361778,1361784,1361791-1361792,1361801,1361803,1362020,1362538,1362707,1363035,1363183,1363186,1363312,1363440,1363557,1363589,1363829,1363832,1363836-1363837,1363853,1364133,1364138,1364229,1364601,1364695,1365001,1365020,1365029,1365479,1366319,1366344,1366621,1367778,1367819,1368053,1368058,1368094,1368121,1368131,1368393,1368396,1369419,1369568,1369604,1369618,1369904,1369995,136,1370
> > >> 

Re: svn commit: r1828883 - /httpd/httpd/branches/2.4.x/

[Marion et Christophe JAILLET]

Trying before proposing is always a good practice.

   svn merge --dry-run 

can help in this situation.

 

CJ

 

> Message du 11/04/18 11:25
> De : "Yann Ylavic" 
> A : "httpd-dev" 
> Copie à : 
> Objet : Re: svn commit: r1828883 - /httpd/httpd/branches/2.4.x/
> 
> It happens when trying to do the real merge before proposing ;)
> 
> On Wed, Apr 11, 2018 at 11:18 AM, ste...@eissing.org  wrote:
> > urgs, sorry.
> >
> >> Am 11.04.2018 um 11:16 schrieb yla...@apache.org:
> >>
> >> Author: ylavic
> >> Date: Wed Apr 11 09:16:56 2018
> >> New Revision: 1828883
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1828883=rev
> >> Log:
> >> Revert r1828882's mergeinfo.
> >>
> >> Modified:
> >> httpd/httpd/branches/2.4.x/ (props changed)
> >>
> >> Propchange: httpd/httpd/branches/2.4.x/
> >> --
> >> --- svn:mergeinfo (original)
> >> +++ svn:mergeinfo Wed Apr 11 09:16:56 2018
> >> @@ -8,4 +8,4 @@
> >> /httpd/httpd/branches/trunk-md:1804087-1804529
> >> /httpd/httpd/branches/trunk-override-index:1793921-1793931
> >> /httpd/httpd/branches/wombat-integration:723609-723841
> >> -/httpd/httpd/trunk:1200475,1200478,1200482,1200491,1200496,1200513,1200550,1200556,1200580,1200605,1200612,1200614,1200639,1200646,1200656,1200667,1200679,1200699,1200702,1200955,1200957,1200961,1200963,1200968,1200975,1200977,1201032,1201042,120,1201194,1201198,1201202,1201443,1201450,1201460,1201956,1202236,1202453,1202456,1202886,1203400,1203491,1203632,1203714,1203859,1203980,1204630,1204968,1204990,1205061,1205075,1205379,1205885,1206291,1206472,1206587,1206850,1206940,1206978,1207719,1208753,1208835,1209053,1209085,1209417,1209432,1209461,1209601,1209603,1209618,1209623,1209741,1209754,1209766,1209776,1209797-1209798,1209811-1209812,1209814,1209908,1209910,1209913,1209916-1209917,1209947,1209952,1210067,1210080,1210120,1210124,1210130,1210148,1210219,1210221,1210252,1210284,1210336,1210378,1210725,1210892,1210951,1210954,1211351-1211352,1211364,1211490,1211495,1211528,1211663,1211680,1212872,1212883,1213338,1213380-1213381,1213391,1213399,1213567,1214003,1214005,1214015,12
> >> 15514,1220462,1220467,1220493,1220524,1220570,1220768,1220794,1220826,1220846,1221205,1221292,1222335,1222370,1222473,1222915,1222917,1222921,1222930,1223048,1225060,1225197-1225199,1225223,1225380,1225476,1225478,1225791,1225795-1225796,1226339,1226375,1227910,1228700,1228816,1229024,1229059,1229099,1229116,1229134,1229136,1229930,1230286,1231255,1231257,1231442,1231446,1231508,1231510,1231518,1232575,1232594,1232630,1232838,1234180,1234297,1234479,1234511,1234565,1234574,1234642-1234643,1234876,1234899,1235019,1236122,1236701,1237407,1238545,1238768,1239029-1239030,1239071,1239565,1240315,1240470,1240778,1241069,1241071,1242089,1242798,1242967,1243176,1243246,1243797,1243799,1244211,1245717,1290823,1290835,1291819-1291820,1291834,1291840,1292043,1293405,1293534-1293535,1293658,1293678,1293708,1294306,1294349,1294356,1294358,1294372,1294471,1297560,1299718,1299786,1300766,130,1301725,1302444,1302483,1302653,1302665,1302674,1303201,1303435,1303827,1304087,1304874-1304875,1305167
> >> ,1305586,1306350,1306409,1306426,1306841,1307790,1308327,1308459,1309536,1309567,1311468,1324760,1325218,1325227,1325250,1325265,1325275,1325632,1325724,1326980,1326984,1326991,1327689,1328325-1328326,1328339,1328345,1328950,1330189,1330964,1331110,1331115,1331942,1331977,1332378,1333969,1334343,1335882,1337344,1341906,1341913,1343085,1343087,1343094,1343099,1343109,1343935,1345319,1345329,1346905,1347980,1348036,1348653,1348656,1348660,1349905,1351012-1351020,1351071-1351072,1351074,1351737,1352047,1352534,1352909-1352912,1357685,1358061,1359057,1359881,1359884,1361153,1361298,1361766,1361773,1361778,1361784,1361791-1361792,1361801,1361803,1362020,1362538,1362707,1363035,1363183,1363186,1363312,1363440,1363557,1363589,1363829,1363832,1363836-1363837,1363853,1364133,1364138,1364229,1364601,1364695,1365001,1365020,1365029,1365479,1366319,1366344,1366621,1367778,1367819,1368053,1368058,1368094,1368121,1368131,1368393,1368396,1369419,1369568,1369604,1369618,1369904,1369995,136,1370
> >> 

Re: svn commit: r1811831 - /httpd/httpd/trunk/server/util_script.c

[Yann Ylavic]

On Mon, Apr 9, 2018 at 10:07 AM, Joe Orton  wrote:
> On Thu, Apr 05, 2018 at 01:38:05PM +0200, Yann Ylavic wrote:
>> On Wed, Oct 11, 2017 at 4:48 PM,   wrote:
>> > Author: jorton
>> > Date: Wed Oct 11 14:48:55 2017
>> > New Revision: 1811831
>> >
>> > URL: http://svn.apache.org/viewvc?rev=1811831=rev
>> > Log:
>> > * server/util_script.c (ap_add_common_vars): Allow mod_env to override
>> >   all system path environment variables, not just PATH.  (The
>> >   behaviour for PATH alone was changed in r965679 for PR 43906.)
>>
>> Since SetEnv* are usable from htaccess, don't we open a risky door here?
>
> If we allow control over PATH (which we do already) I am struggling to
> imagine how it would be worse to allow control of anything other env
> var.

LD_LIBRARY_PATH (and alike) look even more "fun" to play with, possibly.
Whilst applications may not need/use PATH, they can't do much about
LD_LIBRARY_PATH.

PR 43906 states that one can already overwrite LD_LIBRARY_PATH, it's
not the case anymore today I think (w/o this commit).

>
> Can you think of a scenario where it would be a problem?

Custom .so in /path/to/my/htaccess-ed/htdocs for instance which would
be loaded underneath the app (with the same rights).
Although I agree that PATH may already be an issue, so it all depends
on the "trust" given to htaccess files I suppose...
How about a directive to control that? yes it sucks, but...


Regards,
Yann.

Re: SNI normalization?

[Yann Ylavic]

ISTR that the RFC about SNI forbids port numbers (I find it
unfortunate as a matter of fact, given that host names may contain
ports...).
Just to say that normalization may come with ports handling/relaxing
in several places, which I support!

On Wed, Apr 11, 2018 at 11:52 AM, Plüm, Rüdiger, Vodafone Group
 wrote:
> I guess this makes sense to avoid these kind of issues.
>
> Regards
>
> Rüdiger
>
>> -Ursprüngliche Nachricht-
>> Von: Stefan Eissing [mailto:stefan.eiss...@greenbytes.de]
>> Gesendet: Mittwoch, 11. April 2018 11:49
>> An: dev@httpd.apache.org
>> Betreff: SNI normalization?
>>
>> Feedback desired:
>>
>> Checking my server logs, I regularly see clients using SNI with port
>> identifier,
>> as in: test.example.org:443
>>
>> I am not sure what client that is, but we do not identify the vhost that
>> is
>> (probably) intended. Then the request comes in, and there we have magic
>> that
>> finds the correct r->server. Then we mod_ssl sees that sslconn->server
>> != r->server
>> and does some compatibility checks. If the base server and vhost have
>> incompatible
>> settings (e.g. other certs/ciphers etc.), the request fails.
>>
>> This seems to be wrong. Do we need the same normalization that we have
>> in Host: header
>> parsing in SNI?
>>
>> -Stefan

AW: SNI normalization?

[Plüm , Rüdiger , Vodafone Group]

I guess this makes sense to avoid these kind of issues.

Regards

Rüdiger

> -Ursprüngliche Nachricht-
> Von: Stefan Eissing [mailto:stefan.eiss...@greenbytes.de]
> Gesendet: Mittwoch, 11. April 2018 11:49
> An: dev@httpd.apache.org
> Betreff: SNI normalization?
> 
> Feedback desired:
> 
> Checking my server logs, I regularly see clients using SNI with port
> identifier,
> as in: test.example.org:443
> 
> I am not sure what client that is, but we do not identify the vhost that
> is
> (probably) intended. Then the request comes in, and there we have magic
> that
> finds the correct r->server. Then we mod_ssl sees that sslconn->server
> != r->server
> and does some compatibility checks. If the base server and vhost have
> incompatible
> settings (e.g. other certs/ciphers etc.), the request fails.
> 
> This seems to be wrong. Do we need the same normalization that we have
> in Host: header
> parsing in SNI?
> 
> -Stefan

SNI normalization?

[Stefan Eissing]

Feedback desired:

Checking my server logs, I regularly see clients using SNI with port 
identifier, 
as in: test.example.org:443

I am not sure what client that is, but we do not identify the vhost that is
(probably) intended. Then the request comes in, and there we have magic that
finds the correct r->server. Then we mod_ssl sees that sslconn->server != 
r->server
and does some compatibility checks. If the base server and vhost have 
incompatible
settings (e.g. other certs/ciphers etc.), the request fails.

This seems to be wrong. Do we need the same normalization that we have in Host: 
header
parsing in SNI?

-Stefan

Re: svn commit: r1828883 - /httpd/httpd/branches/2.4.x/

[Yann Ylavic]

It happens when trying to do the real merge before proposing ;)

On Wed, Apr 11, 2018 at 11:18 AM, ste...@eissing.org  wrote:
> urgs, sorry.
>
>> Am 11.04.2018 um 11:16 schrieb yla...@apache.org:
>>
>> Author: ylavic
>> Date: Wed Apr 11 09:16:56 2018
>> New Revision: 1828883
>>
>> URL: http://svn.apache.org/viewvc?rev=1828883=rev
>> Log:
>> Revert r1828882's mergeinfo.
>>
>> Modified:
>>httpd/httpd/branches/2.4.x/   (props changed)
>>
>> Propchange: httpd/httpd/branches/2.4.x/
>> --
>> --- svn:mergeinfo (original)
>> +++ svn:mergeinfo Wed Apr 11 09:16:56 2018
>> @@ -8,4 +8,4 @@
>> /httpd/httpd/branches/trunk-md:1804087-1804529
>> /httpd/httpd/branches/trunk-override-index:1793921-1793931
>> /httpd/httpd/branches/wombat-integration:723609-723841
>> -/httpd/httpd/trunk:1200475,1200478,1200482,1200491,1200496,1200513,1200550,1200556,1200580,1200605,1200612,1200614,1200639,1200646,1200656,1200667,1200679,1200699,1200702,1200955,1200957,1200961,1200963,1200968,1200975,1200977,1201032,1201042,120,1201194,1201198,1201202,1201443,1201450,1201460,1201956,1202236,1202453,1202456,1202886,1203400,1203491,1203632,1203714,1203859,1203980,1204630,1204968,1204990,1205061,1205075,1205379,1205885,1206291,1206472,1206587,1206850,1206940,1206978,1207719,1208753,1208835,1209053,1209085,1209417,1209432,1209461,1209601,1209603,1209618,1209623,1209741,1209754,1209766,1209776,1209797-1209798,1209811-1209812,1209814,1209908,1209910,1209913,1209916-1209917,1209947,1209952,1210067,1210080,1210120,1210124,1210130,1210148,1210219,1210221,1210252,1210284,1210336,1210378,1210725,1210892,1210951,1210954,1211351-1211352,1211364,1211490,1211495,1211528,1211663,1211680,1212872,1212883,1213338,1213380-1213381,1213391,1213399,1213567,1214003,1214005,1214015,12
>> 15514,1220462,1220467,1220493,1220524,1220570,1220768,1220794,1220826,1220846,1221205,1221292,1222335,1222370,1222473,1222915,1222917,1222921,1222930,1223048,1225060,1225197-1225199,1225223,1225380,1225476,1225478,1225791,1225795-1225796,1226339,1226375,1227910,1228700,1228816,1229024,1229059,1229099,1229116,1229134,1229136,1229930,1230286,1231255,1231257,1231442,1231446,1231508,1231510,1231518,1232575,1232594,1232630,1232838,1234180,1234297,1234479,1234511,1234565,1234574,1234642-1234643,1234876,1234899,1235019,1236122,1236701,1237407,1238545,1238768,1239029-1239030,1239071,1239565,1240315,1240470,1240778,1241069,1241071,1242089,1242798,1242967,1243176,1243246,1243797,1243799,1244211,1245717,1290823,1290835,1291819-1291820,1291834,1291840,1292043,1293405,1293534-1293535,1293658,1293678,1293708,1294306,1294349,1294356,1294358,1294372,1294471,1297560,1299718,1299786,1300766,130,1301725,1302444,1302483,1302653,1302665,1302674,1303201,1303435,1303827,1304087,1304874-1304875,1305167
>> ,1305586,1306350,1306409,1306426,1306841,1307790,1308327,1308459,1309536,1309567,1311468,1324760,1325218,1325227,1325250,1325265,1325275,1325632,1325724,1326980,1326984,1326991,1327689,1328325-1328326,1328339,1328345,1328950,1330189,1330964,1331110,1331115,1331942,1331977,1332378,1333969,1334343,1335882,1337344,1341906,1341913,1343085,1343087,1343094,1343099,1343109,1343935,1345319,1345329,1346905,1347980,1348036,1348653,1348656,1348660,1349905,1351012-1351020,1351071-1351072,1351074,1351737,1352047,1352534,1352909-1352912,1357685,1358061,1359057,1359881,1359884,1361153,1361298,1361766,1361773,1361778,1361784,1361791-1361792,1361801,1361803,1362020,1362538,1362707,1363035,1363183,1363186,1363312,1363440,1363557,1363589,1363829,1363832,1363836-1363837,1363853,1364133,1364138,1364229,1364601,1364695,1365001,1365020,1365029,1365479,1366319,1366344,1366621,1367778,1367819,1368053,1368058,1368094,1368121,1368131,1368393,1368396,1369419,1369568,1369604,1369618,1369904,1369995,136,1370
>> 001,1370466,1370592,1370615-1370616,1370763,1371387,1371791,1371801,1371878,1371903,1373270,1373447,1373898,1373955,1374157,1374199,1374214,1374216,1374247,1374874,1374877,1374880,1375006,1375009,1375011,1375013,1375445,1375584,1376695,1376700,1378178,1383490,1384408,1384913,1386576,1386578,1386726,1386822,1386880,1386913,1387085,1387088,1387110,1387389,1387444,1387603,1387607,1387633,1387693,1387979,1388029,1388445,1388447,1388648,1388660,1388825,1388899,1389316,1389339,1389481,1389506,1389564,1389566-1389569,1390562,1390564,1391396,1391398,1391771,1392120,1392122,1392150,1392214,1392345-1392347,1392850,1393033,1393058,1393152,1393338,1393564,1394079,1395225,1395253-1395256,1395792,1396440,1397172,1397320,1397636,1397687,1397710,1397716,1398025,1398040,1398066,1398478,1398480-1398481,1398970,1399413,1399687,1399708,1400700,1401448,1402924,1403476,1403483,1403492,1404653,1405407,1405856,1405973,1406068,1406493,1406495,1406616,1406646,1406760,1407004,1407006,1407085,1407088,1407248,1
>> 

Re: svn commit: r1828883 - /httpd/httpd/branches/2.4.x/

[ste...@eissing.org]

urgs, sorry.

> Am 11.04.2018 um 11:16 schrieb yla...@apache.org:
> 
> Author: ylavic
> Date: Wed Apr 11 09:16:56 2018
> New Revision: 1828883
> 
> URL: http://svn.apache.org/viewvc?rev=1828883=rev
> Log:
> Revert r1828882's mergeinfo.
> 
> Modified:
>httpd/httpd/branches/2.4.x/   (props changed)
> 
> Propchange: httpd/httpd/branches/2.4.x/
> --
> --- svn:mergeinfo (original)
> +++ svn:mergeinfo Wed Apr 11 09:16:56 2018
> @@ -8,4 +8,4 @@
> /httpd/httpd/branches/trunk-md:1804087-1804529
> /httpd/httpd/branches/trunk-override-index:1793921-1793931
> /httpd/httpd/branches/wombat-integration:723609-723841
> -/httpd/httpd/trunk:1200475,1200478,1200482,1200491,1200496,1200513,1200550,1200556,1200580,1200605,1200612,1200614,1200639,1200646,1200656,1200667,1200679,1200699,1200702,1200955,1200957,1200961,1200963,1200968,1200975,1200977,1201032,1201042,120,1201194,1201198,1201202,1201443,1201450,1201460,1201956,1202236,1202453,1202456,1202886,1203400,1203491,1203632,1203714,1203859,1203980,1204630,1204968,1204990,1205061,1205075,1205379,1205885,1206291,1206472,1206587,1206850,1206940,1206978,1207719,1208753,1208835,1209053,1209085,1209417,1209432,1209461,1209601,1209603,1209618,1209623,1209741,1209754,1209766,1209776,1209797-1209798,1209811-1209812,1209814,1209908,1209910,1209913,1209916-1209917,1209947,1209952,1210067,1210080,1210120,1210124,1210130,1210148,1210219,1210221,1210252,1210284,1210336,1210378,1210725,1210892,1210951,1210954,1211351-1211352,1211364,1211490,1211495,1211528,1211663,1211680,1212872,1212883,1213338,1213380-1213381,1213391,1213399,1213567,1214003,1214005,1214015,12
> 15514,1220462,1220467,1220493,1220524,1220570,1220768,1220794,1220826,1220846,1221205,1221292,1222335,1222370,1222473,1222915,1222917,1222921,1222930,1223048,1225060,1225197-1225199,1225223,1225380,1225476,1225478,1225791,1225795-1225796,1226339,1226375,1227910,1228700,1228816,1229024,1229059,1229099,1229116,1229134,1229136,1229930,1230286,1231255,1231257,1231442,1231446,1231508,1231510,1231518,1232575,1232594,1232630,1232838,1234180,1234297,1234479,1234511,1234565,1234574,1234642-1234643,1234876,1234899,1235019,1236122,1236701,1237407,1238545,1238768,1239029-1239030,1239071,1239565,1240315,1240470,1240778,1241069,1241071,1242089,1242798,1242967,1243176,1243246,1243797,1243799,1244211,1245717,1290823,1290835,1291819-1291820,1291834,1291840,1292043,1293405,1293534-1293535,1293658,1293678,1293708,1294306,1294349,1294356,1294358,1294372,1294471,1297560,1299718,1299786,1300766,130,1301725,1302444,1302483,1302653,1302665,1302674,1303201,1303435,1303827,1304087,1304874-1304875,1305167
> ,1305586,1306350,1306409,1306426,1306841,1307790,1308327,1308459,1309536,1309567,1311468,1324760,1325218,1325227,1325250,1325265,1325275,1325632,1325724,1326980,1326984,1326991,1327689,1328325-1328326,1328339,1328345,1328950,1330189,1330964,1331110,1331115,1331942,1331977,1332378,1333969,1334343,1335882,1337344,1341906,1341913,1343085,1343087,1343094,1343099,1343109,1343935,1345319,1345329,1346905,1347980,1348036,1348653,1348656,1348660,1349905,1351012-1351020,1351071-1351072,1351074,1351737,1352047,1352534,1352909-1352912,1357685,1358061,1359057,1359881,1359884,1361153,1361298,1361766,1361773,1361778,1361784,1361791-1361792,1361801,1361803,1362020,1362538,1362707,1363035,1363183,1363186,1363312,1363440,1363557,1363589,1363829,1363832,1363836-1363837,1363853,1364133,1364138,1364229,1364601,1364695,1365001,1365020,1365029,1365479,1366319,1366344,1366621,1367778,1367819,1368053,1368058,1368094,1368121,1368131,1368393,1368396,1369419,1369568,1369604,1369618,1369904,1369995,136,1370
> 001,1370466,1370592,1370615-1370616,1370763,1371387,1371791,1371801,1371878,1371903,1373270,1373447,1373898,1373955,1374157,1374199,1374214,1374216,1374247,1374874,1374877,1374880,1375006,1375009,1375011,1375013,1375445,1375584,1376695,1376700,1378178,1383490,1384408,1384913,1386576,1386578,1386726,1386822,1386880,1386913,1387085,1387088,1387110,1387389,1387444,1387603,1387607,1387633,1387693,1387979,1388029,1388445,1388447,1388648,1388660,1388825,1388899,1389316,1389339,1389481,1389506,1389564,1389566-1389569,1390562,1390564,1391396,1391398,1391771,1392120,1392122,1392150,1392214,1392345-1392347,1392850,1393033,1393058,1393152,1393338,1393564,1394079,1395225,1395253-1395256,1395792,1396440,1397172,1397320,1397636,1397687,1397710,1397716,1398025,1398040,1398066,1398478,1398480-1398481,1398970,1399413,1399687,1399708,1400700,1401448,1402924,1403476,1403483,1403492,1404653,1405407,1405856,1405973,1406068,1406493,1406495,1406616,1406646,1406760,1407004,1407006,1407085,1407088,1407248,1
> 

Re: 2.4.3x regression w/SSL vhost configs

[Stefan Eissing]

Additional example of the brokeness. If you add
a SSL directive to the 2nd vhost in your example
- without the new MERGE feature:

> Am 10.04.2018 um 18:22 schrieb Joe Orton :
> 
> Listen 443 https
> 
> 
>   ServerName www.example.com:443
>   SSLCertificateFile ...
> 
> 
> 
>   ServerName other.example.com:443
   SSLEngine on
> 

the certificate is also no longer visible there. 

Re: 2.4.3x regression w/SSL vhost configs

[Stefan Eissing]

> Am 10.04.2018 um 18:22 schrieb Joe Orton :
> 
> I don't quite understand the new AP_MODULE_FLAG_ALWAYS_MERGE logic so 
> I'm struggling to debug this, but I've had a couple of reports that 
> configurations like:
> 
> Listen 443 https
> 
> 
>   ServerName www.example.com:443
>   SSLCertificateFile ...
> 
> 
> 
>   ServerName other.example.com:443
> 
> 
> worked in 2.4.29 but now fail to start with:
> 
> AH02572: Failed to configure at least one certificate and key for 
> other.example.com
> 
> For the second vhost ap_get_server_protocol(s) returns "https" and 
> triggers the "implicit SSLEngine on" logic.
> 
> Removing the AP_MODULE_FLAG_ALWAYS_MERGE from the module struct flags 
> (i.e. setting flags to zero) fixes it; I see the 10104 warning in 
> ssl_init_Module is also triggered because the SSLSrvConfigRec is shared 
> across vhosts.
> 
> Any clues?

Without the MERGE flag, the config recs of a module are the very
same instance across all virtual hosts - unless a host carries
module specific directives itself.

In your example, the first vhost creates a new mod_ssl config rec
because of "SSLCertificateFile" and the following vhost has no
"SSL*" config and uses the same config rec instance as the first.
That makes it see the certificate file, even though it was never
configured for that vhost.

This would break, if you introduce another vhost before
the www.example.com. Then other.example.com would share that one
and no longer see the certificate file of the - then - 2nd vhost.

In short: the config relies on buggy behaviour outside the normal
"global server to vhost" inheritance. 

The necessity to do so arose during development of mod_md, which
affects config recs during the post_config phase. Without this
fix, any mod_ssl config flag like "this vhost is going to 503
for the time being", would also disable other vhosts that,
unpredictable to the user, share the same config rec instance.

Not sure I explained that well...

Cheers,

Stefan