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

[Jan Ehrhardt]

Jacob Champion in gmane.comp.apache.devel (Tue, 20 Jun 2017 09:07:44
-0700):
>On 02/08/2017 07:56 PM, Eric Covener wrote:
>> Assuming there's some alternate path that actually does change
>> SCRIPT_NAME by default, we a) don't have any complaint about
>> SCRIPT_NAME and b) have the SetEnv thing.  If we want more options,
>> maybe we can stash this older SCRIPT_NAME into a new variable and show
>> how to copy it over SCRIPT_NAME?
>
>...and restarting this conversation, since the new behavior seems to 
>have a bug:
>
>https://bz.apache.org/bugzilla/show_bug.cgi?id=61202

BTW: the developers at Directadmin are aware of this bug.
https://forum.directadmin.com/showthread.php?t=54952=2=281618#post281618
I ran into it while updating my Centos 6 servers.
-- 
Jan

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

[Jacob Champion]

On 06/20/2017 01:35 PM, Jacob Champion wrote:

What requests and server config did you use with this test setup?


Here's why I'm asking: if I were to propose the attached patch for 
backport, what is the test case that *should* fail but doesn't? 
(proxy_fcgi.t passes, no problem.) And once we get that test case, can 
we show that it actually addresses a valid PHP-FPM use case, or is it a 
feature without a user?


--Jacob
diff --git modules/mappers/mod_actions.c modules/mappers/mod_actions.c
index ac9c3b7..2a67a27 100644
--- modules/mappers/mod_actions.c
+++ modules/mappers/mod_actions.c
@@ -186,8 +186,7 @@ static int action_handler(request_rec *r)
 ap_field_noparam(r->pool, r->content_type);
 
 if (action && (t = apr_table_get(conf->action_types, action))) {
-int virtual = (*t++ == '0' ? 0 : 1);
-if (!virtual && r->finfo.filetype == APR_NOFILE) {
+if (*t++ == '0' && r->finfo.filetype == APR_NOFILE) {
 ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00652)
   "File does not exist: %s", r->filename);
 return HTTP_NOT_FOUND;
@@ -198,9 +197,6 @@ static int action_handler(request_rec *r)
  * (will be REDIRECT_HANDLER there)
  */
 apr_table_setn(r->subprocess_env, "HANDLER", action);
-if (virtual) {
-apr_table_setn(r->notes, "virtual_script", "1");
-}
 }
 
 if (script == NULL)
diff --git modules/proxy/mod_proxy_fcgi.c modules/proxy/mod_proxy_fcgi.c
index a268556..41292e8 100644
--- modules/proxy/mod_proxy_fcgi.c
+++ modules/proxy/mod_proxy_fcgi.c
@@ -321,7 +321,6 @@ static apr_status_t send_environment(proxy_conn_rec *conn, request_rec *r,
 apr_status_t rv;
 apr_size_t avail_len, len, required_len;
 int next_elem, starting_elem;
-int fpm = 0;
 fcgi_req_config_t *rconf = ap_get_module_config(r->request_config, _fcgi_module);
 fcgi_dirconf_t *dconf = ap_get_module_config(r->per_dir_config, _fcgi_module);
 
@@ -354,8 +353,6 @@ static apr_status_t send_environment(proxy_conn_rec *conn, request_rec *r,
 *qs = '\0';
 }
 }
-} else {
-fpm = 1;
 }
 
 if (newfname) {
@@ -364,38 +361,9 @@ static apr_status_t send_environment(proxy_conn_rec *conn, request_rec *r,
 }
 }
 
-#if 0
-ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(0)
-  "r->filename: %s", (r->filename ? r->filename : "nil"));
-ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(0)
-  "r->uri: %s", (r->uri ? r->uri : "nil"));
-ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(0)
-  "r->path_info: %s", (r->path_info ? r->path_info : "nil"));
-#endif
-
 ap_add_common_vars(r);
 ap_add_cgi_vars(r);
 
-if (fpm || apr_table_get(r->notes, "virtual_script")) {
-/*
- * Adjust SCRIPT_NAME, PATH_INFO and PATH_TRANSLATED for PHP-FPM
- * TODO: Right now, PATH_INFO and PATH_TRANSLATED look OK...
- */
-const char *pend;
-const char *script_name = apr_table_get(r->subprocess_env, "SCRIPT_NAME");
-pend = script_name + strlen(script_name);
-if (r->path_info && *r->path_info) {
-pend = script_name + ap_find_path_info(script_name, r->path_info) - 1;
-}
-while (pend != script_name && *pend != '/') {
-pend--;
-}
-apr_table_setn(r->subprocess_env, "SCRIPT_NAME", pend);
-ap_log_rerror(APLOG_MARK, APLOG_TRACE4, 0, r,
-  "fpm:virtual_script: Modified SCRIPT_NAME to: %s",
-  pend);
-}
-
 /* XXX are there any FastCGI specific env vars we need to send? */
 
 /* Give admins final option to fine-tune env vars */

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

[Jacob Champion]

On 06/20/2017 12:23 PM, Jim Jagielski wrote:

% cat fcgi.pl
#!/usr/bin/env perl



What requests and server config did you use with this test setup?

--Jacob

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

[Jim Jagielski]

% cat fcgi.pl
#!/usr/bin/env perl
use FCGI;
use Socket;
use FCGI::ProcManager;
use Data::Dumper;

$num_args = $#ARGV + 1;
if ($num_args != 1) {
  print "\nUsage: fcgi.pl \n";
  exit 1;
}

$proc_manager = FCGI::ProcManager->new( {n_processes => 1} );
$socket = FCGI::OpenSocket( $ARGV[0], 10 );
$request = FCGI::Request( \*STDIN, \*STDOUT, \*STDERR, \%req_params,
$socket, ::FAIL_ACCEPT_ON_INTR );
$proc_manager->pm_manage();
if ($request) {
  while ( $request->Accept() >= 0 ) {
$proc_manager->pm_pre_dispatch();
print("Content-type: text/plain\r\n\r\n");
print Dumper(\%req_params);
  }
}
FCGI::CloseSocket($socket);




% cat fpm.sh
php-fpm70 -p /usr/local2/php-fpm


% ls  /usr/local2/php-fpm
docs/etc/ fcgi.pl* fpm/ fpm.sh*  log/ pools/   run/ var/
 www/

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

[Jacob Champion]

On 06/20/2017 11:48 AM, Jim Jagielski wrote:

Ahh... the tests from the orig bug report


There have been several reports. I'm hoping to get the test case you 
were using to test the new `if (fpm ...` logic in mod_proxy_fcgi. Even 
if it was just a manual test; I just want to get it recorded in the suite.


--Jacob

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

[Jacob Champion]

On 06/20/2017 10:55 AM, Jim Jagielski wrote:

It's already in the wild.


We do not guarantee bug compatibility. That's our judgment call based on 
adoption, expected user disruption, time in the wild, etc.


The purpose of my Showstopper was to revert to known-good behavior. That 
didn't happen, not least because I didn't catch this bug in the 
backport, I wanted to keep things moving, and I +1'd it despite the 
presence of the rider patch that I tried unsuccessfully to keep out [1]. 
So that's on me (and that's why I'm grinding this particular axe). I 
want to fix the mistake I started last year, and not punish our users 
with yet another unusable default for the third release in a row.


--Jacob

[1] 
https://lists.apache.org/thread.html/539645ba41021f85a87ad397861cb8d09fb057f7fb7e9ed768781e85@%3Cdev.httpd.apache.org%3E

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

[Jim Jagielski]

Ahh... the tests from the orig bug report

> On Jun 20, 2017, at 2:39 PM, Jacob Champion  wrote:
> 
> SCRIPT_NAME

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

[Jacob Champion]

On 06/20/2017 10:55 AM, Jim Jagielski wrote:

Last I checked, it's in the test framework...
Quick grep for "SCRIPT_NAME" in the tests doesn't turn up anything; can 
you point me to it?


--Jacob

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

[William A Rowe Jr]

On Tue, Jun 20, 2017 at 1:32 PM, William A Rowe Jr  wrote:
> On Tue, Jun 20, 2017 at 12:12 PM, Jim Jagielski  wrote:
>>
>>> On Jun 20, 2017, at 1:03 PM, Jacob Champion  wrote:
>>>
>>> On 06/20/2017 10:00 AM, William A Rowe Jr wrote:
 You must presume it is in the wild, and shortening the exposure
 by a matter of days isn't significant.
>>>
>>> My point is that we should fix it ASAP. Days vs. more days may not be 
>>> significant, but days vs. months is definitely significant when it comes to 
>>> bug-compatibility and user expectations.
>>
>> All this is, IMO, moot until we have a *patch*. Right now there
>> is a work-around, which, again IMO, reduces the "need" to release
>> something "now"... the only question is whether the patch changes
>> the behavior of the 2 current settings or whether it adds a 3rd
>> option (the latter is my recommendation).
>
> To your point and Jacob's concern, whatever we settle on, we can
> surely add that upon adoption to

... the http://www.apache.org/dist/httpd/patches/apply_to_2.4.26/ tree

and provide the appropriate pointer on users@ and mention on the
various bugzilla tickets that arise.

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

[William A Rowe Jr]

On Tue, Jun 20, 2017 at 12:12 PM, Jim Jagielski  wrote:
>
>> On Jun 20, 2017, at 1:03 PM, Jacob Champion  wrote:
>>
>> On 06/20/2017 10:00 AM, William A Rowe Jr wrote:
>>> You must presume it is in the wild, and shortening the exposure
>>> by a matter of days isn't significant.
>>
>> My point is that we should fix it ASAP. Days vs. more days may not be 
>> significant, but days vs. months is definitely significant when it comes to 
>> bug-compatibility and user expectations.
>
> All this is, IMO, moot until we have a *patch*. Right now there
> is a work-around, which, again IMO, reduces the "need" to release
> something "now"... the only question is whether the patch changes
> the behavior of the 2 current settings or whether it adds a 3rd
> option (the latter is my recommendation).

To your point and Jacob's concern, whatever we settle on, we can
surely add that upon adoption to

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

[Jacob Perkins]

As a general FYI: cPanel can’t release 2.4.26 until this is patched / fixed. We 
were bitten *hard* by the SCRIPT_URI breakage a while back, and this is going 
to put a blocker on our release.

—
Jacob Perkins
Product Owner
cPanel Inc.

jacob.perk...@cpanel.net 
Office:  713-529-0800 x 4046
Cell:  713-560-8655

> On Jun 20, 2017, at 12:12 PM, Jim Jagielski  wrote:
> 
> 
>> On Jun 20, 2017, at 1:03 PM, Jacob Champion  wrote:
>> 
>> On 06/20/2017 10:00 AM, William A Rowe Jr wrote:
>>> You must presume it is in the wild, and shortening the exposure
>>> by a matter of days isn't significant.
>> 
>> My point is that we should fix it ASAP. Days vs. more days may not be 
>> significant, but days vs. months is definitely significant when it comes to 
>> bug-compatibility and user expectations.
> 
> All this is, IMO, moot until we have a *patch*. Right now there
> is a work-around, which, again IMO, reduces the "need" to release
> something "now"... the only question is whether the patch changes
> the behavior of the 2 current settings or whether it adds a 3rd
> option (the latter is my recommendation).
> 
> And we should update ./t/modules/proxy_fcgi.t and maybe create
> a test which runs if PHP is available.



smime.p7s
Description: S/MIME cryptographic signature

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

[Jim Jagielski]

> On Jun 20, 2017, at 1:18 PM, Jacob Champion  wrote:
> 
> On 06/20/2017 10:12 AM, Jim Jagielski wrote:
>> All this is, IMO, moot until we have a *patch*.
> 
> Agreed. See my other fork of this thread for my questions on that.
> 
>> Right now there
>> is a work-around, which, again IMO, reduces the "need" to release
>> something "now"... the only question is whether the patch changes
>> the behavior of the 2 current settings or whether it adds a 3rd
>> option (the latter is my recommendation).
> 
> The faster we can patch, the less there is a need to introduce yet another 
> "ProxyFcgiBackendType FPM-But-Better-And-Without-Bugs-This-Time" option.
> 

It's already in the wild.

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

[Jim Jagielski]

Last I checked, it's in the test framework... 
> On Jun 20, 2017, at 1:42 PM, Jacob Champion  wrote:
> 
> On 06/20/2017 09:47 AM, Jacob Champion wrote:
>> I think. Still trying to context switch back three months.
> 
> Jim, do you have a good test case for the current FPM logic so we don't break 
> that with a fix?
> 
> --Jacob

Re: svn commit: r1799356 - in /httpd/httpd/branches/2.2.x: ./ server/scoreboard.c

[William A Rowe Jr]

On Jun 20, 2017 12:00, "Jacob Champion"  wrote:

On 06/20/2017 09:47 AM, wr...@apache.org wrote:

> Log:
> Make the range test legible
>
Hmm, out of curiosity, is the legibility you mention from the
parenthesization change or the switch to greater-than-or-equal for one side?



I kind of like reading code that has all less-than comparisons, instead of
mixed less-than and greater-than, because it means the logic is closer to
the mathematics and the number line. For example,


I prefer comparisons where the lhs is the variant and rhs is the longer
lived/fixed quantum.


0 < x < 5
becomes
((0 < x) && (x < 5))

y < 1 or 5 < y
becomes
((y < 1) || (5 < y))

This is not a big deal; I just feel like typing about something trivial
this morning. I realize the point of this patch is to fix the off-by-one.


Neither logic would be 'wrong'.

On that note, the extra parens aren't needed for C/C++, OOO rules that ||
and && are the lowest order, but I still find it less obvious. So asking
the compiler to go to some extra effort seems worthwhile. One can argue
that this misleads new programmers into a belief that the parens had an
actual parsing impact, so again there is no one right answer.

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

[Jacob Champion]

On 06/20/2017 09:47 AM, Jacob Champion wrote:

I think. Still trying to context switch back three months.


Jim, do you have a good test case for the current FPM logic so we don't 
break that with a fix?


--Jacob

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

[Jacob Champion]

On 06/20/2017 10:12 AM, Jim Jagielski wrote:

All this is, IMO, moot until we have a *patch*.


Agreed. See my other fork of this thread for my questions on that.


Right now there
is a work-around, which, again IMO, reduces the "need" to release
something "now"... the only question is whether the patch changes
the behavior of the 2 current settings or whether it adds a 3rd
option (the latter is my recommendation).


The faster we can patch, the less there is a need to introduce yet 
another "ProxyFcgiBackendType FPM-But-Better-And-Without-Bugs-This-Time" 
option.


--Jacob

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

[Jim Jagielski]

> On Jun 20, 2017, at 1:03 PM, Jacob Champion  wrote:
> 
> On 06/20/2017 10:00 AM, William A Rowe Jr wrote:
>> You must presume it is in the wild, and shortening the exposure
>> by a matter of days isn't significant.
> 
> My point is that we should fix it ASAP. Days vs. more days may not be 
> significant, but days vs. months is definitely significant when it comes to 
> bug-compatibility and user expectations.

All this is, IMO, moot until we have a *patch*. Right now there
is a work-around, which, again IMO, reduces the "need" to release
something "now"... the only question is whether the patch changes
the behavior of the 2 current settings or whether it adds a 3rd
option (the latter is my recommendation).

And we should update ./t/modules/proxy_fcgi.t and maybe create
a test which runs if PHP is available.

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

[Jacob Champion]

On 06/20/2017 10:00 AM, William A Rowe Jr wrote:

You must presume it is in the wild, and shortening the exposure
by a matter of days isn't significant.


My point is that we should fix it ASAP. Days vs. more days may not be 
significant, but days vs. months is definitely significant when it comes 
to bug-compatibility and user expectations.


--Jacob

Re: svn commit: r1799356 - in /httpd/httpd/branches/2.2.x: ./ server/scoreboard.c

[Jacob Champion]

On 06/20/2017 09:47 AM, wr...@apache.org wrote:

Log:
Make the range test legible
Hmm, out of curiosity, is the legibility you mention from the 
parenthesization change or the switch to greater-than-or-equal for one side?




I kind of like reading code that has all less-than comparisons, instead 
of mixed less-than and greater-than, because it means the logic is 
closer to the mathematics and the number line. For example,


0 < x < 5
becomes
((0 < x) && (x < 5))

y < 1 or 5 < y
becomes
((y < 1) || (5 < y))

This is not a big deal; I just feel like typing about something trivial 
this morning. I realize the point of this patch is to fix the off-by-one.




--Jacob

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

[William A Rowe Jr]

On Tue, Jun 20, 2017 at 11:17 AM, Jacob Champion  wrote:
> On 06/20/2017 09:16 AM, William A Rowe Jr wrote:
>>
>> It's released into the wild, what is done is done.
>
>
> Of course. But having it in the wild for three days is different than having
> it in the wild for six months.

Unless you are classifying this a security fix, it may prove to be
a less popular update given what we announced as fixed in 2.4.26.
The same isn't expected for 2.4.27, since some users prioritize
security updates and let others sit for a while, or undergo added
scrutiny for non-critical updates.

In any case, it will be in the wild for >3 days. It's at five days and
counting, if you tag and roll today. Vote is 72 hrs long, and we
always give our mirrors a day to catch up with us.

You must presume it is in the wild, and shortening the exposure
by a matter of days isn't significant. It just needs the appropriate
clear note in CHANGES with 2.4.27 so that users are prepared
for the change (or reversion).

Re: [PATCH 2.2] fix ap_get_scoreboard_*

[William A Rowe Jr]

Joe, I compromised on your fix and retained parens for legibility,
following the pattern of the other fix.

Committed as r1799356, thanks

On Mon, Jun 19, 2017 at 7:08 AM, Joe Orton  wrote:
> The limit checking is broken in 2.2's ap_get_scoreboard_*.  This was
> fixed in 2.4 in http://svn.apache.org/viewvc?view=revision=417252
>
> Patch below backports that, plus fixes the additional broken comparison
> in ap_get_scoreboard_lb(), discovered by Hisanobu Okuda.
>
> Can I get +1s for this for 2.2?
>
> Submitted by: wrowe, jorton
>
> Index: server/scoreboard.c
> ===
> --- server/scoreboard.c (revision 1799181)
> +++ server/scoreboard.c (working copy)
> @@ -503,8 +503,8 @@
>
>  AP_DECLARE(worker_score *) ap_get_scoreboard_worker(int x, int y)
>  {
> -if (((x < 0) || (server_limit < x)) ||
> -((y < 0) || (thread_limit < y))) {
> +if (((x < 0) || (x >= server_limit)) ||
> +((y < 0) || (y >= thread_limit))) {
>  return(NULL); /* Out of range */
>  }
>  return _scoreboard_image->servers[x][y];
> @@ -527,7 +527,7 @@
>
>  AP_DECLARE(process_score *) ap_get_scoreboard_process(int x)
>  {
> -if ((x < 0) || (server_limit < x)) {
> +if ((x < 0) || (x >= server_limit)) {
>  return(NULL); /* Out of range */
>  }
>  return _scoreboard_image->parent[x];
> @@ -540,7 +540,7 @@
>
>  AP_DECLARE(lb_score *) ap_get_scoreboard_lb(int lb_num)
>  {
> -if (((lb_num < 0) || (lb_limit < lb_num))) {
> +if (lb_num < 0 || lb_num >= lb_limit) {
>  return(NULL); /* Out of range */
>  }
>  return _scoreboard_image->balancers[lb_num];
>

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

[Jacob Champion]

On 06/20/2017 09:07 AM, Jacob Champion wrote:

Can we do a quick fixup and reroll before it's too late?


And... what *is* the fixup?

It looks like the logic here -- where we start at the end of SCRIPT_NAME 
(or the beginning of PATH_INFO) and work backwards -- is designed to 
stop at the first directory separator no matter what, which I completely 
missed in my review. It needs to stop wherever the "URL" part of the 
path ends and the "filesystem" part of the path begins.


I think. Still trying to context switch back three months.

--Jacob

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

[Jacob Champion]

On 06/20/2017 09:16 AM, William A Rowe Jr wrote:

It's released into the wild, what is done is done.


Of course. But having it in the wild for three days is different than 
having it in the wild for six months.


--Jacob

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

[William A Rowe Jr]

On Tue, Jun 20, 2017 at 11:15 AM, Jacob Champion  wrote:
> On 06/20/2017 09:14 AM, William A Rowe Jr wrote:
>>
>> Would encourage us to wait at least a couple more days for
>> other, unrelated regression reports to filter in while fixing this
>> defect. But there is nothing stopping a 2.4.27 in rapid
>> succession, we simply don't retroactively  "retract" releases.
>
>
> Right, re-issuing 2.4.26 isn't what I meant. I just want to patch this up as
> quickly as possible so people don't standardize on the new behavior.

It's released into the wild, what is done is done.

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

[Jacob Champion]

On 06/20/2017 09:14 AM, William A Rowe Jr wrote:

Would encourage us to wait at least a couple more days for
other, unrelated regression reports to filter in while fixing this
defect. But there is nothing stopping a 2.4.27 in rapid
succession, we simply don't retroactively  "retract" releases.


Right, re-issuing 2.4.26 isn't what I meant. I just want to patch this 
up as quickly as possible so people don't standardize on the new behavior.


--Jaccob

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

[William A Rowe Jr]

On Tue, Jun 20, 2017 at 11:07 AM, Jacob Champion  wrote:
> On 02/08/2017 07:56 PM, Eric Covener wrote:
>>
>> Assuming there's some alternate path that actually does change
>> SCRIPT_NAME by default, we a) don't have any complaint about
>> SCRIPT_NAME and b) have the SetEnv thing.  If we want more options,
>> maybe we can stash this older SCRIPT_NAME into a new variable and show
>> how to copy it over SCRIPT_NAME?
>
>
> ...and restarting this conversation, since the new behavior seems to have a
> bug:
>
> https://bz.apache.org/bugzilla/show_bug.cgi?id=61202
>
> Can we do a quick fixup and reroll before it's too late?

Define reroll? too late?

I know what you mean by fixup, hopefully not so quick as to
overlook other tangential issues. But we never 're-roll', we
discard the candidate and roll the next version (this assures
that whomever has httpd-2.4.26.tar.gz in their hands has the
one and only one package of that name that was ever created.)
And 2.4.26 is released, announcement is away.

Would encourage us to wait at least a couple more days for
other, unrelated regression reports to filter in while fixing this
defect. But there is nothing stopping a 2.4.27 in rapid
succession, we simply don't retroactively  "retract" releases.

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

[Jacob Champion]

On 02/08/2017 07:56 PM, Eric Covener wrote:

Assuming there's some alternate path that actually does change
SCRIPT_NAME by default, we a) don't have any complaint about
SCRIPT_NAME and b) have the SetEnv thing.  If we want more options,
maybe we can stash this older SCRIPT_NAME into a new variable and show
how to copy it over SCRIPT_NAME?


...and restarting this conversation, since the new behavior seems to 
have a bug:


https://bz.apache.org/bugzilla/show_bug.cgi?id=61202

Can we do a quick fixup and reroll before it's too late?

--Jacob

Re: ocsp stapling improvements

[Stefan Eissing]

> Am 20.06.2017 um 17:43 schrieb William A Rowe Jr :
> 
> On Tue, Jun 20, 2017 at 6:39 AM, Stefan Eissing
>  wrote:
>> 
>>> Am 12.06.2017 um 21:35 schrieb Ruediger Pluem :
>>> 
 2. Persist responses (is this just a config/default issue?)
>>> 
>>> This could become tricky given the various so cache implementations we 
>>> have. I could
>>> only think of persisting the whole cache when Apache is shutdown.
>> 
>> I am not very experienced with those. Would the current Stapling 
>> implementation not work with a "socache_dbm"? And what would be the drawback 
>> of that?
>> 
>> As an alternative, use of mod_watchdog offers advantages here. If we have 
>> only one thread (for all or for a particular certificate) that writes the 
>> cache in a server (all processes), it becomes easy to use the file system, I 
>> think. Write per tmpfile+rename should be good enough and it should no 
>> longer need a global mutex. Server names are distinct and make for an easy 
>> directory tree.
>> 
>> The question then is if mod_watchdog jobs still have privileges or if those 
>> files have common ownership and if that is a problem.
>> 
>> Does this makes sense or am I insane?
> 
> I consider it very unwise to use root to perform client queries, almost
> as unwise as serving any responses. The problem in both cases is
> that every defect in parsing (and there is 'input' in either case) opens
> up the remote possibility of a root exploit.
> 
> All client requests, for stapling, for health check, for any purposes
> whatsoever should be performed as httpd configured user. Whether
> that happens in an ombudsman resource process or the typical
> worker process is a design question.
> 
> Yes, a dbm certainly should survive a server restart, that's why one
> would use dbm for the SSL Session Cache (before session tickets
> offered a fresh solution - although these suffer just as many issues
> as our OCSP implementation so far.)

I share your point of view. I do not want to make OCSP requests as root.
As far as I can tell, that would mean the dbm file is owned by the configured
user.

My question is: is this acceptable or do we have an alternative?

Re: ocsp stapling improvements

[William A Rowe Jr]

On Tue, Jun 20, 2017 at 6:39 AM, Stefan Eissing
 wrote:
>
>> Am 12.06.2017 um 21:35 schrieb Ruediger Pluem :
>>
>>> 2. Persist responses (is this just a config/default issue?)
>>
>> This could become tricky given the various so cache implementations we have. 
>> I could
>> only think of persisting the whole cache when Apache is shutdown.
>
> I am not very experienced with those. Would the current Stapling 
> implementation not work with a "socache_dbm"? And what would be the drawback 
> of that?
>
> As an alternative, use of mod_watchdog offers advantages here. If we have 
> only one thread (for all or for a particular certificate) that writes the 
> cache in a server (all processes), it becomes easy to use the file system, I 
> think. Write per tmpfile+rename should be good enough and it should no longer 
> need a global mutex. Server names are distinct and make for an easy directory 
> tree.
>
> The question then is if mod_watchdog jobs still have privileges or if those 
> files have common ownership and if that is a problem.
>
> Does this makes sense or am I insane?

I consider it very unwise to use root to perform client queries, almost
as unwise as serving any responses. The problem in both cases is
that every defect in parsing (and there is 'input' in either case) opens
up the remote possibility of a root exploit.

All client requests, for stapling, for health check, for any purposes
whatsoever should be performed as httpd configured user. Whether
that happens in an ombudsman resource process or the typical
worker process is a design question.

Yes, a dbm certainly should survive a server restart, that's why one
would use dbm for the SSL Session Cache (before session tickets
offered a fresh solution - although these suffer just as many issues
as our OCSP implementation so far.)

Re: ocsp stapling improvements

[Stefan Eissing]

> Am 20.06.2017 um 17:19 schrieb Hanno Böck :
> 
> On Tue, 20 Jun 2017 13:39:56 +0200
> Stefan Eissing  wrote:
> 
>> Can we push the burden of getting a OCSP response to the client, even
>> for must-staple certificates?
> 
> No, you can't.
> The whole point is that must staple enforces stapling.
> 
> This has a bit to do with the history of certificate revocation and why
> it's broken.
> 
> All browsers do OCSP checks in a soft-fail mode (or not at all). This
> basically makes it pointless, as an attacker can just block OCSP
> requests.
> 
> OCSP stapling was invented to move away from that unreliable mechanism.
> Must-staple enforces that mechanism. There is no way to fall back to
> the old unreliable mechanism if you want to have it secure.

So, the extension protects clients with incomplete or silently graceful
fallbacks from exposing their users. Understood. Not sure if I share this
strategy 100%, but it is what it is.

If httpd persists responses and tries to renew a good amount of time before
they expire (btw. do you know what common validity durations are?), this
hopefully does not become a huge DoS opportunity.

-Stefan

Re: ocsp stapling improvements

[Hanno Böck]

On Tue, 20 Jun 2017 13:39:56 +0200
Stefan Eissing  wrote:

> Can we push the burden of getting a OCSP response to the client, even
> for must-staple certificates?

No, you can't.
The whole point is that must staple enforces stapling.

This has a bit to do with the history of certificate revocation and why
it's broken.

All browsers do OCSP checks in a soft-fail mode (or not at all). This
basically makes it pointless, as an attacker can just block OCSP
requests.

OCSP stapling was invented to move away from that unreliable mechanism.
Must-staple enforces that mechanism. There is no way to fall back to
the old unreliable mechanism if you want to have it secure.


-- 
Hanno Böck
https://hboeck.de/

mail/jabber: ha...@hboeck.de
GPG: FE73757FA60E4E21B937579FA5880072BBB51E42

Re: ocsp stapling improvements

[Stefan Eissing]

> Am 20.06.2017 um 14:35 schrieb Plüm, Rüdiger, Vodafone Group 
> :
> 
> It might cause I/O overhead compared with socache_shmcb, but it might be a 
> good solution
> for those who want to have persisted OCSP responses. Other people might 
> priorize
> a distributed cache like dc or memcache. So I like the idea of just staying
> with the current approach to define the socache provider used for caching.
> Who knows? Maybe someone writes a socache_staggered that allows to go through 
> several
> socache providers one after another in case of a cache miss? That would allow 
> to have
> a shmcb first and a dbm second.

I agree to the re-use and the more flexible architecture we already have in 
place here.

The only bone I have with this is that it has "only" cache semantics and is not 
a store. Sure, we can document that an admin should make the cache "large 
enough", but it's not totally under his control as the size of the answers can 
vary. 

If we update the answers regularly by watchdog, we should remove the request 
trigger during connection setup (e.g. complexity, race conditions). A too small 
cache could really hurt.

OTOH: is dbm really size limited? Maybe it is not. Then this would be my 
preference.

>> 
>> As an alternative, use of mod_watchdog offers advantages here. If we
>> have only one thread (for all or for a particular certificate) that
>> writes the cache in a server (all processes), it becomes easy to use the
>> file system, I think. Write per tmpfile+rename should be good enough and
>> it should no longer need a global mutex. Server names are distinct and
>> make for an easy directory tree.
>> 
>> The question then is if mod_watchdog jobs still have privileges or if
>> those files have common ownership and if that is a problem.
>> 
>> Does this makes sense or am I insane?
> 
> I guess this is all solvable, but as stated above I am in favor of just using 
> the
> socache API for that and let our requirements be solved by an appropriate 
> socache
> provider.

"staggered" cache with store semantics would be cool.

-Stefan

Re: Timeouts and other time-related granularity

[Stefan Eissing]

I live and learn - hopefully.

> Am 20.06.2017 um 15:32 schrieb Plüm, Rüdiger, Vodafone Group 
> :
> 
> 
> 
>> -Ursprüngliche Nachricht-
>> Von: Stefan Eissing [mailto:stefan.eiss...@greenbytes.de]
>> Gesendet: Dienstag, 20. Juni 2017 15:13
>> An: dev@httpd.apache.org
>> Betreff: Re: Timeouts and other time-related granularity
>> 
>> 
>>> Am 20.06.2017 um 14:56 schrieb Jim Jagielski :
>>> 
>>> Most of our time-related directives accept seconds as their
>>> parameters, but I'm thinking that allowing milliseconds is
>>> likely a better option... From what I can see, most of what
>>> would be interesting/useful to change are stored as interval/apr_time
>>> anyway, so there would be no real API/struct changes required.
>>> 
>>> Thoughts? Comments?
>> 
>> +1
>> 
>> I would suggest a common duration syntax for better readability and a
>> common conversion util function. We can stick to seconds if it is
>> just a number, but allow fractions for millis etc. So
>> 
>> "1" is 1 second "0.001" is one millisecond, as is "1ms".
>> "1.5m" is 90 seconds, as is "1:30m"
>> "1:15h" is 75 minutes
>> "2:20:33.1h" is 140 minutes, 33 seconds and 100ms.
>> "1:60h" is invalid, as is "1:00:00m" and "1:00:00:00" etc.
> 
> We already have such a similar function used in various places of the config:
> 
> /**
> * Parse a given timeout parameter string into an apr_interval_time_t value.
> * The unit of the time interval is given as postfix string to the numeric
> * string. Currently the following units are understood:
> *
> * ms: milliseconds
> * s : seconds
> * mi[n] : minutes
> * h : hours
> *
> * If no unit is contained in the given timeout parameter the default_time_unit
> * will be used instead.
> * @param timeout_parameter The string containing the timeout parameter.
> * @param timeout The timeout value to be returned.
> * @param default_time_unit The default time unit to use if none is specified
> * in timeout_parameter.
> * @return Status value indicating whether the parsing was successful or not.
> */
> AP_DECLARE(apr_status_t) ap_timeout_parameter_parse(
>   const char *timeout_parameter,
>   apr_interval_time_t *timeout,
>   const char *default_time_unit);
> 
> Regards
> 
> Rüdiger

Re: Timeouts and other time-related granularity

[Jim Jagielski]

We already have a format which is used by a few directives via 
ap_timeout_parameter_parse()

If desired, we could extend that for the fractional stuff.
> On Jun 20, 2017, at 9:12 AM, Stefan Eissing  
> wrote:
> 
> 
>> Am 20.06.2017 um 14:56 schrieb Jim Jagielski :
>> 
>> Most of our time-related directives accept seconds as their
>> parameters, but I'm thinking that allowing milliseconds is
>> likely a better option... From what I can see, most of what
>> would be interesting/useful to change are stored as interval/apr_time
>> anyway, so there would be no real API/struct changes required.
>> 
>> Thoughts? Comments?
> 
> +1 
> 
> I would suggest a common duration syntax for better readability and a 
> common conversion util function. We can stick to seconds if it is
> just a number, but allow fractions for millis etc. So
> 
> "1" is 1 second "0.001" is one millisecond, as is "1ms". 
> "1.5m" is 90 seconds, as is "1:30m"
> "1:15h" is 75 minutes
> "2:20:33.1h" is 140 minutes, 33 seconds and 100ms.
> "1:60h" is invalid, as is "1:00:00m" and "1:00:00:00" etc.
> 

RE: Timeouts and other time-related granularity

[Houser, Rick]

> "2:20:33.1h" is 140 minutes, 33 seconds and 100ms.

This one seems very backwards to me, as leading zeros could change the meaning. 
 Ex. 0:1:15h vs 1:15h.  If allowing combination units like that (personally, it 
seems overkill), I think it would be much safer to require an explicit 
specification as 'hms', 'ms', etc. which must match the delimiter count, or to 
just use the least significant unit as in '1:30:15.345s'.


Rick Houser
Web Administration

> -Original Message-
> From: Stefan Eissing [mailto:stefan.eiss...@greenbytes.de]
> Sent: Tuesday, June 20, 2017 09:13
> To: dev@httpd.apache.org
> Subject: Re: Timeouts and other time-related granularity
> 
> 
> > Am 20.06.2017 um 14:56 schrieb Jim Jagielski :
> >
> > Most of our time-related directives accept seconds as their
> > parameters, but I'm thinking that allowing milliseconds is
> > likely a better option... From what I can see, most of what
> > would be interesting/useful to change are stored as interval/apr_time
> > anyway, so there would be no real API/struct changes required.
> >
> > Thoughts? Comments?
> 
> +1
> 
> I would suggest a common duration syntax for better readability and a
> common conversion util function. We can stick to seconds if it is
> just a number, but allow fractions for millis etc. So
> 
> "1" is 1 second "0.001" is one millisecond, as is "1ms".
> "1.5m" is 90 seconds, as is "1:30m"
> "1:15h" is 75 minutes
> "2:20:33.1h" is 140 minutes, 33 seconds and 100ms.
> "1:60h" is invalid, as is "1:00:00m" and "1:00:00:00" etc.

AW: Timeouts and other time-related granularity

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

> -Ursprüngliche Nachricht-
> Von: Stefan Eissing [mailto:stefan.eiss...@greenbytes.de]
> Gesendet: Dienstag, 20. Juni 2017 15:13
> An: dev@httpd.apache.org
> Betreff: Re: Timeouts and other time-related granularity
> 
> 
> > Am 20.06.2017 um 14:56 schrieb Jim Jagielski :
> >
> > Most of our time-related directives accept seconds as their
> > parameters, but I'm thinking that allowing milliseconds is
> > likely a better option... From what I can see, most of what
> > would be interesting/useful to change are stored as interval/apr_time
> > anyway, so there would be no real API/struct changes required.
> >
> > Thoughts? Comments?
> 
> +1
> 
> I would suggest a common duration syntax for better readability and a
> common conversion util function. We can stick to seconds if it is
> just a number, but allow fractions for millis etc. So
> 
> "1" is 1 second "0.001" is one millisecond, as is "1ms".
> "1.5m" is 90 seconds, as is "1:30m"
> "1:15h" is 75 minutes
> "2:20:33.1h" is 140 minutes, 33 seconds and 100ms.
> "1:60h" is invalid, as is "1:00:00m" and "1:00:00:00" etc.

We already have such a similar function used in various places of the config:

/**
 * Parse a given timeout parameter string into an apr_interval_time_t value.
 * The unit of the time interval is given as postfix string to the numeric
 * string. Currently the following units are understood:
 *
 * ms: milliseconds
 * s : seconds
 * mi[n] : minutes
 * h : hours
 *
 * If no unit is contained in the given timeout parameter the default_time_unit
 * will be used instead.
 * @param timeout_parameter The string containing the timeout parameter.
 * @param timeout The timeout value to be returned.
 * @param default_time_unit The default time unit to use if none is specified
 * in timeout_parameter.
 * @return Status value indicating whether the parsing was successful or not.
 */
AP_DECLARE(apr_status_t) ap_timeout_parameter_parse(
   const char *timeout_parameter,
   apr_interval_time_t *timeout,
   const char *default_time_unit);

Regards

Rüdiger

Re: Timeouts and other time-related granularity

[Stefan Eissing]

> Am 20.06.2017 um 14:56 schrieb Jim Jagielski :
> 
> Most of our time-related directives accept seconds as their
> parameters, but I'm thinking that allowing milliseconds is
> likely a better option... From what I can see, most of what
> would be interesting/useful to change are stored as interval/apr_time
> anyway, so there would be no real API/struct changes required.
> 
> Thoughts? Comments?

+1 

I would suggest a common duration syntax for better readability and a 
common conversion util function. We can stick to seconds if it is
just a number, but allow fractions for millis etc. So

"1" is 1 second "0.001" is one millisecond, as is "1ms". 
"1.5m" is 90 seconds, as is "1:30m"
"1:15h" is 75 minutes
"2:20:33.1h" is 140 minutes, 33 seconds and 100ms.
"1:60h" is invalid, as is "1:00:00m" and "1:00:00:00" etc.

Timeouts and other time-related granularity

[Jim Jagielski]

Most of our time-related directives accept seconds as their
parameters, but I'm thinking that allowing milliseconds is
likely a better option... From what I can see, most of what
would be interesting/useful to change are stored as interval/apr_time
anyway, so there would be no real API/struct changes required.

Thoughts? Comments?

AW: ocsp stapling improvements

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

> -Ursprüngliche Nachricht-
> Von: Stefan Eissing [mailto:stefan.eiss...@greenbytes.de]
> Gesendet: Dienstag, 20. Juni 2017 13:40
> An: dev@httpd.apache.org
> Betreff: Re: ocsp stapling improvements
> 
> 
> > Am 12.06.2017 um 21:35 schrieb Ruediger Pluem :
> >
> > I guess the core mistake we do today is that we expire the entries in
> the cache
> > after SSLStaplingStandardCacheTimeout. But we should keep them in the
> cache as long as
> > they are valid (so either whats in the next update field of the
> response or this update
> > + SSLStaplingResponseMaxAge).
> 
> +1
> 
> > Instead we should have a refresh parameter that I would set as
> percentage of the
> > expired time (so between this update and next update or as percentage
> of already
> > expired SSLStaplingResponseMaxAge). Once this refresh time is passed
> OCSP responses
> > should get refreshed by a background job (possibly implemented by
> mod_watchdog).
> 
> +1
> 
> >> 2. Persist responses (is this just a config/default issue?)
> >
> > This could become tricky given the various so cache implementations we
> have. I could
> > only think of persisting the whole cache when Apache is shutdown.
> 
> I am not very experienced with those. Would the current Stapling
> implementation not work with a "socache_dbm"? And what would be the
> drawback of that?

It might cause I/O overhead compared with socache_shmcb, but it might be a good 
solution
for those who want to have persisted OCSP responses. Other people might priorize
a distributed cache like dc or memcache. So I like the idea of just staying
with the current approach to define the socache provider used for caching.
Who knows? Maybe someone writes a socache_staggered that allows to go through 
several
socache providers one after another in case of a cache miss? That would allow 
to have
a shmcb first and a dbm second.

> 
> As an alternative, use of mod_watchdog offers advantages here. If we
> have only one thread (for all or for a particular certificate) that
> writes the cache in a server (all processes), it becomes easy to use the
> file system, I think. Write per tmpfile+rename should be good enough and
> it should no longer need a global mutex. Server names are distinct and
> make for an easy directory tree.
> 
> The question then is if mod_watchdog jobs still have privileges or if
> those files have common ownership and if that is a problem.
> 
> Does this makes sense or am I insane?

I guess this is all solvable, but as stated above I am in favor of just using 
the
socache API for that and let our requirements be solved by an appropriate 
socache
provider.

> 
> >> 3. Start update responses at server start/regular intervals
> >
> > What I want to avoid is that the server "hangs" at start because of a
> "hanging" OCSP server.
> > I admit that this can happen already today on the very first SSL
> request with stapling turned
> > on, but IMHO this is a bad behavior. So either just do the stuff on a
> regular basis in the background
> > and do not staple if there is no valid OCSP response yet (I know Hanno
> won't like that :-))
> > Or have an initial (valid) OCSP response being loaded from a file
> during startup. It would be up to
> > the admin to fill this file with a valid OCSP response before it
> starts httpd.
> 
> Can we push the burden of getting a OCSP response to the client, even
> for must-staple certificates? This would be nice as a fall back in case
> of errors. The error might be in the data center for outgoing
> connections, so the client might succeed where the server fails.

I would be in favor of this. I think it is still reasonable to have the client
try the OCSP server if the server cannot get a valid response to staple.
But this is my personal opinion, so others might have different requirements.
The question to me is what do we need to do in mod_ssl code to offer this?
I mean the changes that are now on the table should ensure that we get a more
reliable stapling infrastructure within mod_ssl that allows people to try
must-staple. Anything else we need or can do?

Regards

Rüdiger

Re: ocsp stapling improvements

[Stefan Eissing]

> Am 12.06.2017 um 21:35 schrieb Ruediger Pluem :
> 
> I guess the core mistake we do today is that we expire the entries in the 
> cache
> after SSLStaplingStandardCacheTimeout. But we should keep them in the cache 
> as long as
> they are valid (so either whats in the next update field of the response or 
> this update
> + SSLStaplingResponseMaxAge).

+1

> Instead we should have a refresh parameter that I would set as percentage of 
> the
> expired time (so between this update and next update or as percentage of 
> already
> expired SSLStaplingResponseMaxAge). Once this refresh time is passed OCSP 
> responses
> should get refreshed by a background job (possibly implemented by 
> mod_watchdog).

+1

>> 2. Persist responses (is this just a config/default issue?)
> 
> This could become tricky given the various so cache implementations we have. 
> I could
> only think of persisting the whole cache when Apache is shutdown.

I am not very experienced with those. Would the current Stapling implementation 
not work with a "socache_dbm"? And what would be the drawback of that?

As an alternative, use of mod_watchdog offers advantages here. If we have only 
one thread (for all or for a particular certificate) that writes the cache in a 
server (all processes), it becomes easy to use the file system, I think. Write 
per tmpfile+rename should be good enough and it should no longer need a global 
mutex. Server names are distinct and make for an easy directory tree.

The question then is if mod_watchdog jobs still have privileges or if those 
files have common ownership and if that is a problem.

Does this makes sense or am I insane?

>> 3. Start update responses at server start/regular intervals
> 
> What I want to avoid is that the server "hangs" at start because of a 
> "hanging" OCSP server.
> I admit that this can happen already today on the very first SSL request with 
> stapling turned
> on, but IMHO this is a bad behavior. So either just do the stuff on a regular 
> basis in the background
> and do not staple if there is no valid OCSP response yet (I know Hanno won't 
> like that :-))
> Or have an initial (valid) OCSP response being loaded from a file during 
> startup. It would be up to
> the admin to fill this file with a valid OCSP response before it starts httpd.

Can we push the burden of getting a OCSP response to the client, even for 
must-staple certificates? This would be nice as a fall back in case of errors. 
The error might be in the data center for outgoing connections, so the client 
might succeed where the server fails.

-Stefan

Re: ocsp stapling improvements

[Stefan Eissing]

> Am 13.06.2017 um 00:48 schrieb Hanno Böck :
> 
> What I think needs also be handled:
> * There's
>  https://bz.apache.org/bugzilla/show_bug.cgi?id=59049
>  which indicates that faulty responses from the OCSP server may bring
>  the server into a faulty state from which it doesn't recover. I
>  haven't tried to reproduce this, but it certianly should be fixed as
>  well, probably just some missing error check tough.

I would expect to catch that with the change in handling responses. If we
never replace an existing OCSP response one with an error, the problem
should disappear. As I read the code, we currently use the response cache
to signal communication errors to other connections to avoid spamming
the OCSP responder.

If we have only one mod_watchdog thread querying responders, there no
longer is need for that, it seems.

> * Some of the existing options imho don't make any sense and should
>  default to off and maybe even be forced off (so that setting them to
>  "on" doesn't do anything). That includes SSLStaplingFakeTryLater and
>  SSLStaplingReturnResponderErrors. Unless I'm missing something I
>  don't see any situation in which stapling OCSP errors is desirable.

I think we want any enhancements to be applicable to 2.4.x and there
we cannot silently change the defaults. I propose to add a new stapling
directive that changes to a different default set for the other ones.

SSLStaplingResilience   on|off|enforce

with "on" and "off" to change defaults to new/old ones. And "enforce"
to only allow new ones and log warnings when old are ignored. Something
to detect/verify settings in a large configuration.