Re: mod_proxy_spawn: Review request
On Sun, 29 Nov 2020 17:01:42 +
Nick Kew wrote:
> > On 29 Nov 2020, at 14:48, Florian Wagner wrote:
> >
> > Hi everyone!
> >
> > I was wondering if someone with a better understanding of httpd and
> > mod_proxy could review my module idea and prototype implementation and
> > warn me of any unforeseen pitfalls looming ahead before I commit to
> > implementing this fully.
>
> The main thing that springs to mind is security when you spawn a process.
> CGI and fastcgi are battle-hardened by decades of operational use.
Hey Nick, thanks for your comments.
I'd never want to build the process management and supervision part
completely from the ground up. Especially since the tried and true
implementation in mod_fcgid seems to be rather easy to lift out.
> [...]
>
> > While fully understanding that one can be of the opinion that process
> > management is best kept out of httpd, I personally like the convenience
> > and more importantly clarity offered by having the complete command,
> > arguments and environment required to run the backend application in
> > the httpd configuration. Authentication, URL rewriting and whatelse
> > will already be setup there, anyway.
>
> If it's in the config then at least it's [probably] coming from a trusted
> source,
> but then why run per-request?
Let me apologize for the choice of function name (start_process) in my
prototype. That probably made you think I'd want to start the backend
process once for each request (CGI style). Rather I'm aiming at long
running backends. That function should probably be called
start_process_if_its_not_already_running_otherwise_just_return_its_uds_path
instead... :-D
Is that what you were hinting at with your question?
> > So I took a shot at seeing if I could implement a module to do just
> > that. My current idea/prototype:
> >
> > 1. Register a hook to run before mod_proxy.c:proxy_handler and have a
> >look at the request filename and handler to see if they start with
> >"proxy:spawn://".
>
> Big red flag for security there. Hope you're paying very careful attention
> to your input: there's nothing to that effect in what you attached.
My naive understanding of the httpd request pipeline made me try
curl 'http://.../proxy:spawn://${MALICIOUS}|http://localhost/'
For this r->filename ended up being "${DocumentRoot}/proxy:spawn:" as
there is no matching ProxyPass/RewriteRule, mod_proxy and my
proxy_spawn_handler skip this one as the filename prefix is not right
and finally the file handler gets invoked returning a 404. Next:
curl 'http://.../pass/proxy:spawn://${MALICIOUS}|http://localhost/'
with "ProxyPass /pass spawn://${CMD}|http://localhost/; in the httpd.conf.
That got rewritten by proxy_trans to
proxy:spawn://${CMD}|http://localhost/pass/proxy:spawn:${MALICIOUS}|http:/localhost
which also doesn't seem like an issue. That would spawn ${CMD} and
simply pass the "proxy:spawn:${MALICIOUS}" part on to the backend
service as the request URL.
For sure there are RewriteRules that could lead to external clients
being able to execute arbitrary command, like
RewriteRule ^(.*)$ spawn://$1|http://localhost/
but these all seem to me like they only would be caused by erroneous
httpd configurations.
I probably missed something to consider here. Help and pointers would
be appreciated!
> Also I'd consider hooking it earlier in the request cycle
Yeah since rewriting the name is what this does, it's probably better
fitted for a translate_name hook. With APR_HOOK_FIRST and aszPred = {
"mod_proxy.c", NULL } so it gets called after mod_proxy and proxy_trans
has the chance to match the filename against ProxyPass rules.
> or into mod_proxy instead.
That's what I tried first. After reading the code for half a day I gave
up finding a way to hook this into. Explaining the various ways I tried
and failed would be half a novel so I'll spare you that.
But if anyone has a hint how I could accomplish it that way I'll
happily have a look and try again.
> How does mod_proxy_fcgi fit your vision?
Like any of the protocol implementations of mod_proxy that support
AF_UNIX sockets it should work nicely:
ProxyPass / spawn://...|fcgi://localhost/
The process management/supervision being separate from the actual
protocol proxying is rather important to me. That way I can continue to
deploy legacy FastCGI services through mod_proxy_fcgi while newer apps
that use WebSockets use mod_proxy_http and mod_proxy_wstunnel.
And all the while the backend processes are started and supervised by
my module.
Regards
Florian
Re: mod_proxy_spawn: Review request
> On 29 Nov 2020, at 14:48, Florian Wagner wrote: > > Hi everyone! > > I was wondering if someone with a better understanding of httpd and > mod_proxy could review my module idea and prototype implementation and > warn me of any unforeseen pitfalls looming ahead before I commit to > implementing this fully. The main thing that springs to mind is security when you spawn a process. CGI and fastcgi are battle-hardened by decades of operational use. > Wanting to switch to mod_proxy_http for deploying backend applications > (and opening the way for WebSockets through mod_proxy_wstunnel) I'm > missing the process management provided by mod_fastcgi [1]. fastcgi is an unusual model, with a very specific purpose. It's more usual for backends to be self-managing. > While fully understanding that one can be of the opinion that process > management is best kept out of httpd, I personally like the convenience > and more importantly clarity offered by having the complete command, > arguments and environment required to run the backend application in > the httpd configuration. Authentication, URL rewriting and whatelse > will already be setup there, anyway. If it's in the config then at least it's [probably] coming from a trusted source, but then why run per-request? > So I took a shot at seeing if I could implement a module to do just > that. My current idea/prototype: > > 1. Register a hook to run before mod_proxy.c:proxy_handler and have a >look at the request filename and handler to see if they start with >"proxy:spawn://". Big red flag for security there. Hope you're paying very careful attention to your input: there's nothing to that effect in what you attached. Also I'd consider hooking it earlier in the request cycle, or into mod_proxy instead. How does mod_proxy_fcgi fit your vision? -- Nick Kew
mod_proxy_spawn: Review request
Hi everyone!
I was wondering if someone with a better understanding of httpd and
mod_proxy could review my module idea and prototype implementation and
warn me of any unforeseen pitfalls looming ahead before I commit to
implementing this fully.
Following is a short text on my motivation and I've attached the 62
lines of prototype code. I fully understand if no one's in the mood to
have a look at this and apologize for having wasted some of your time.
Otherwise: Thank you for reading on.
Wanting to switch to mod_proxy_http for deploying backend applications
(and opening the way for WebSockets through mod_proxy_wstunnel) I'm
missing the process management provided by mod_fastcgi [1].
While fully understanding that one can be of the opinion that process
management is best kept out of httpd, I personally like the convenience
and more importantly clarity offered by having the complete command,
arguments and environment required to run the backend application in
the httpd configuration. Authentication, URL rewriting and whatelse
will already be setup there, anyways.
So I took a shot at seeing if I could implement a module to do just
that. My current idea/prototype:
1. Register a hook to run before mod_proxy.c:proxy_handler and have a
look at the request filename and handler to see if they start with
"proxy:spawn://".
2. Use everything after that and until a pipe character as the command
to spawn. No process management in that module, yet, of course, but
that could easily be lifted from mod_fastcgi or mod_fcgid. As done
in those implementations the process manager will create a AF_UNIX
socket and pass its file descriptor to the spawned process.
3. Rewrite the request filename/handler to "unix://uds_path|rest" form
using the AF_UNIX socket from the process manager as well as the
proxy configuration included in the filename/handler after the
pipe. Then let httpd/mod_proxy continue onward.
Repository with code and httpd.conf for testing this is at [2].
Thanks in advance and regards
Florian
[1] https://fastcgi-archives.github.io/mod_fastcgi.html#FastCgiServer
[2] https://github.com/wagnerflo/mod_proxy_spawn
#include "http_core.h"
#include "mod_proxy.h"
static int start_process(request_rec* r, const char* name, char** uds_path) {
ap_log_rerror(APLOG_MARK, APLOG_TRACE3, 0, r, "start_process(\"%s\", ...)", name);
*uds_path = "...";
return OK;
}
static int proxy_spawn_handler(request_rec* r) {
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
"proxy_handler({ filename=\"%s\", handler=\"%s\", ... })",
r->filename, r->handler);
char** url;
// forced proxy handler by SetHandler
if (!r->proxyreq && r->handler && strncmp(r->handler, "proxy:", 6) == 0)
url = (char**) >handler;
// filename rewritten by proxy_trans
else if (strncmp(r->filename, "proxy:", 6) == 0)
url = >filename;
else
return DECLINED;
if (ap_cstr_casecmpn(*url + 6, "spawn://", 8) != 0)
return DECLINED;
char* name = *url + 14;
char* real = ap_strchr_c(name, '|');
if (real == NULL) {
return HTTP_INTERNAL_SERVER_ERROR;
}
char* uds_path;
if (start_process(r, apr_pstrndup(r->pool, name, real - name), _path)) {
return HTTP_INTERNAL_SERVER_ERROR;
}
*url = apr_pstrcat(r->pool, "proxy:unix://", uds_path, real, NULL);
ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, "rewrite proxy url to %s", *url);
return DECLINED;
}
static void register_hooks(apr_pool_t* pool) {
// make sure we get called before proxy_handler
static const char* const aszSucc[] = { "mod_proxy.c", NULL };
ap_hook_handler(proxy_spawn_handler, NULL, aszSucc, APR_HOOK_FIRST);
}
AP_DECLARE_MODULE(proxy_spawn) = {
STANDARD20_MODULE_STUFF,
NULL,
NULL,
NULL,
NULL,
NULL,
register_hooks
};
Review request
Hi, can somebody check simple patch in https://issues.apache.org/bugzilla/show_bug.cgi?id=54474 ? It solves my problem and is backed up by RFC2817. It would be nice to have it in 2.4.4. Thanks -- Pavel Mateja
Patch review request
Could someone with commit access take a look at the following patches? Neither of these are high priority, but I don't want to let them get too far out of date. https://issues.apache.org/bugzilla/show_bug.cgi?id=51077 Fixes two issues with how mod_rewrite handles rules with the [P] flag: - Makes query string handling for requests destined for mod_proxy_fcgi and mod_proxy_scgi consistent with how query strings are already handled for mod_proxy_ajp and mod_proxy_http. - Makes logic for handling query strings in directory context the same as in server context. https://issues.apache.org/bugzilla/show_bug.cgi?id=50880 Prevents mod_proxy_scgi from setting PATH_INFO unless requested, for better compliance with RFC 3875. This will hopefully be an easy patch to review, since it was just submitted for consistency with a patch which was already committed for mod_proxy_fcgi, https://issues.apache.org/bugzilla/show_bug.cgi?id=50851 Thanks in advance. -- Mark Montague m...@catseye.org
Re: Patch review request
I'll take a look... thx! On Apr 27, 2011, at 2:03 PM, Mark Montague wrote: Could someone with commit access take a look at the following patches? Neither of these are high priority, but I don't want to let them get too far out of date. https://issues.apache.org/bugzilla/show_bug.cgi?id=51077 Fixes two issues with how mod_rewrite handles rules with the [P] flag: - Makes query string handling for requests destined for mod_proxy_fcgi and mod_proxy_scgi consistent with how query strings are already handled for mod_proxy_ajp and mod_proxy_http. - Makes logic for handling query strings in directory context the same as in server context. https://issues.apache.org/bugzilla/show_bug.cgi?id=50880 Prevents mod_proxy_scgi from setting PATH_INFO unless requested, for better compliance with RFC 3875. This will hopefully be an easy patch to review, since it was just submitted for consistency with a patch which was already committed for mod_proxy_fcgi, https://issues.apache.org/bugzilla/show_bug.cgi?id=50851 Thanks in advance. -- Mark Montague m...@catseye.org
[PATCH 30730] mod_actions and Server-Status (Patch review request).
Hi, I am Basant and I work for Sun Microsystems Inc. in web tier group. I have submitted the patch for 30730. Bug Id : 30730 http://issues.apache.org/bugzilla/show_bug.cgi?id=30730 Summary : mod_actions and Server-Status Patch uri : http://issues.apache.org/bugzilla/attachment.cgi?id=19706 I would like to make a humble review request for the patch. Thanks and Regards, Basant.
[PATCH 11035] Patch review request
Hi, I am Basant. I work in web tier group of Sun Microsystems Inc. Can some of the committers kindly review the patch for bug 11035 please? Subject : Apache adds double entries to headers generated by CGI URI : http://issues.apache.org/bugzilla/show_bug.cgi?id=11035 Thanks and Regards, Basant.
Re: [PATCH 39299] - Revised Patch review request
Hi, I have revised the patch. Can some of the commiter kindly review the patch? Regards, Basant. On Fri, Mar 02, 2007 at 10:53:29AM -0800, Basant Kukreja wrote: On Fri, Mar 02, 2007 at 08:39:25AM +0100, Plüm, Rüdiger, VF EITO wrote: -Ursprüngliche Nachricht- Von: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] Gesendet: Freitag, 2. März 2007 02:15 An: dev@httpd.apache.org Betreff: Re: [PATCH 39299] - Patch review request Thanks Nick for responding to my request. My comments are in between. On Wed, Feb 28, 2007 at 10:49:48PM +, Nick Kew wrote: On Wed, 28 Feb 2007 14:31:19 -0800 Basant Kukreja [EMAIL PROTECTED] wrote: Hi, I am Basant. I work in web tier group in Sun Microsystems Inc. I have submitted the patch for bug 39299. Summary : Internal Server Error (500) on COPY URI : http://issues.apache.org/bugzilla/show_bug.cgi?id=39299 Can some of the committer kindly review my patch please to see if it is acceptable or not? Patch is against 2.2.x branch. 409 implies a condition the client can fix. Your patch tests for a particular condition that is likely to be fixable in a server with DAV uprunning. But AFAICS it could also give a bogus 409, for example in the case of a newly-installed and misconfigured server. Can you kindly elaborate more? How newly misconfigured server can send 409? Here is my test case : DavLockDB /disk/apache/apache2/var/DAVLockFs Directory /disk/apache/apache2/htdocs/DAVtest Options Indexes FollowSymLinks AllowOverride None order allow,deny allow from all AuthName SMA Development server AuthType Basic DAV On /Directory Now assuming, I misconfigured the server and I intended to configure /DAVtest1 instead of /DAVtest, if I send a request. -- COPY /DAVtest1/litmus/copysrc HTTP/1.1 Host: myhostname.mydomain:4004 User-Agent: litmus/0.11 neon/0.25.5 Connection: TE TE: trailers Depth: 0 Destination: http://myhostname.mydomain:4004/DAVtest/litmus/nonesuch/foo Overwrite: F X-Litmus: copymove: 5 (copy_nodestcoll I guess Nicks Idea was the other way round: COPY /DAVtest/litmus/copysrc Destination: http://myhostname.mydomain:4004/DAVtest1/litmus/nonesuch/foo There is a crash issue in that case. I need to address the issue along with the current fix. I have update the PR. Regards, Basant. IMHO this direction would also better match the problem described in PR39299. Regards Rüdiger
Re: [PATCH 39299] - Patch review request
On Fri, Mar 02, 2007 at 08:39:25AM +0100, Plüm, Rüdiger, VF EITO wrote: -Ursprüngliche Nachricht- Von: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] Gesendet: Freitag, 2. März 2007 02:15 An: dev@httpd.apache.org Betreff: Re: [PATCH 39299] - Patch review request Thanks Nick for responding to my request. My comments are in between. On Wed, Feb 28, 2007 at 10:49:48PM +, Nick Kew wrote: On Wed, 28 Feb 2007 14:31:19 -0800 Basant Kukreja [EMAIL PROTECTED] wrote: Hi, I am Basant. I work in web tier group in Sun Microsystems Inc. I have submitted the patch for bug 39299. Summary : Internal Server Error (500) on COPY URI : http://issues.apache.org/bugzilla/show_bug.cgi?id=39299 Can some of the committer kindly review my patch please to see if it is acceptable or not? Patch is against 2.2.x branch. 409 implies a condition the client can fix. Your patch tests for a particular condition that is likely to be fixable in a server with DAV uprunning. But AFAICS it could also give a bogus 409, for example in the case of a newly-installed and misconfigured server. Can you kindly elaborate more? How newly misconfigured server can send 409? Here is my test case : DavLockDB /disk/apache/apache2/var/DAVLockFs Directory /disk/apache/apache2/htdocs/DAVtest Options Indexes FollowSymLinks AllowOverride None order allow,deny allow from all AuthName SMA Development server AuthType Basic DAV On /Directory Now assuming, I misconfigured the server and I intended to configure /DAVtest1 instead of /DAVtest, if I send a request. -- COPY /DAVtest1/litmus/copysrc HTTP/1.1 Host: myhostname.mydomain:4004 User-Agent: litmus/0.11 neon/0.25.5 Connection: TE TE: trailers Depth: 0 Destination: http://myhostname.mydomain:4004/DAVtest/litmus/nonesuch/foo Overwrite: F X-Litmus: copymove: 5 (copy_nodestcoll I guess Nicks Idea was the other way round: COPY /DAVtest/litmus/copysrc Destination: http://myhostname.mydomain:4004/DAVtest1/litmus/nonesuch/foo There is a crash issue in that case. I need to address the issue along with the current fix. I have update the PR. Regards, Basant. IMHO this direction would also better match the problem described in PR39299. Regards Rüdiger
Re: [PATCH 39299] - Patch review request
Thanks Nick for responding to my request. My comments are in between. On Wed, Feb 28, 2007 at 10:49:48PM +, Nick Kew wrote: On Wed, 28 Feb 2007 14:31:19 -0800 Basant Kukreja [EMAIL PROTECTED] wrote: Hi, I am Basant. I work in web tier group in Sun Microsystems Inc. I have submitted the patch for bug 39299. Summary : Internal Server Error (500) on COPY URI : http://issues.apache.org/bugzilla/show_bug.cgi?id=39299 Can some of the committer kindly review my patch please to see if it is acceptable or not? Patch is against 2.2.x branch. 409 implies a condition the client can fix. Your patch tests for a particular condition that is likely to be fixable in a server with DAV uprunning. But AFAICS it could also give a bogus 409, for example in the case of a newly-installed and misconfigured server. Can you kindly elaborate more? How newly misconfigured server can send 409? Here is my test case : DavLockDB /disk/apache/apache2/var/DAVLockFs Directory /disk/apache/apache2/htdocs/DAVtest Options Indexes FollowSymLinks AllowOverride None order allow,deny allow from all AuthName SMA Development server AuthType Basic DAV On /Directory Now assuming, I misconfigured the server and I intended to configure /DAVtest1 instead of /DAVtest, if I send a request. -- COPY /DAVtest1/litmus/copysrc HTTP/1.1 Host: myhostname.mydomain:4004 User-Agent: litmus/0.11 neon/0.25.5 Connection: TE TE: trailers Depth: 0 Destination: http://myhostname.mydomain:4004/DAVtest/litmus/nonesuch/foo Overwrite: F X-Litmus: copymove: 5 (copy_nodestcoll) -- I will get a 405 response. -- HTTP/1.1 405 Method Not Allowed Date: Thu, 01 Mar 2007 04:12:59 GMT Server: Apache/2.2.5-dev (Unix) mod_ssl/2.2.5-dev OpenSSL/0.9.8a DAV/2 SVN/1.4.3 mod_perl/2.0.4-dev Perl/v5.8.8 Allow: GET,HEAD,POST,OPTIONS,TRACE Content-Length: 245 Content-Type: text/html; charset=iso-8859-1 !DOCTYPE HTML PUBLIC -//IETF//DTD HTML 2.0//EN htmlhead title405 Method Not Allowed/title /headbody h1Method Not Allowed/h1 pThe requested method COPY is not allowed for the URL /DAVtest1/litmus/copysrc./p /body/html -- Does the DAV RFC explicitly tell us to use 409 in this instance? I didn't find RFC explictly stating 409 response but it is one of the responses returned by COPY method. I will dig more and return back on this. Regards, Basant. -- Nick Kew Application Development with Apache - the Apache Modules Book http://www.apachetutor.org/
Re: [PATCH 39299] - Patch review request
-Ursprüngliche Nachricht- Von: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] Gesendet: Freitag, 2. März 2007 02:15 An: dev@httpd.apache.org Betreff: Re: [PATCH 39299] - Patch review request Thanks Nick for responding to my request. My comments are in between. On Wed, Feb 28, 2007 at 10:49:48PM +, Nick Kew wrote: On Wed, 28 Feb 2007 14:31:19 -0800 Basant Kukreja [EMAIL PROTECTED] wrote: Hi, I am Basant. I work in web tier group in Sun Microsystems Inc. I have submitted the patch for bug 39299. Summary : Internal Server Error (500) on COPY URI : http://issues.apache.org/bugzilla/show_bug.cgi?id=39299 Can some of the committer kindly review my patch please to see if it is acceptable or not? Patch is against 2.2.x branch. 409 implies a condition the client can fix. Your patch tests for a particular condition that is likely to be fixable in a server with DAV uprunning. But AFAICS it could also give a bogus 409, for example in the case of a newly-installed and misconfigured server. Can you kindly elaborate more? How newly misconfigured server can send 409? Here is my test case : DavLockDB /disk/apache/apache2/var/DAVLockFs Directory /disk/apache/apache2/htdocs/DAVtest Options Indexes FollowSymLinks AllowOverride None order allow,deny allow from all AuthName SMA Development server AuthType Basic DAV On /Directory Now assuming, I misconfigured the server and I intended to configure /DAVtest1 instead of /DAVtest, if I send a request. -- COPY /DAVtest1/litmus/copysrc HTTP/1.1 Host: myhostname.mydomain:4004 User-Agent: litmus/0.11 neon/0.25.5 Connection: TE TE: trailers Depth: 0 Destination: http://myhostname.mydomain:4004/DAVtest/litmus/nonesuch/foo Overwrite: F X-Litmus: copymove: 5 (copy_nodestcoll I guess Nicks Idea was the other way round: COPY /DAVtest/litmus/copysrc Destination: http://myhostname.mydomain:4004/DAVtest1/litmus/nonesuch/foo IMHO this direction would also better match the problem described in PR39299. Regards Rüdiger
Re: [PATCH 38014] - Patch review request
Revised patch after incorporating Will Rowe's suggestion. Regards, Basant. On Tue, Feb 27, 2007 at 05:06:57PM -0800, Basant Kukreja wrote: Hi, I work in the web tier group of Sun Microsystems Inc. I have submitted the patch for bug 38014 (The status '100 Continue' will be sent after the final status code) http://issues.apache.org/bugzilla/show_bug.cgi?id=38014 Can some of the committer kindly review my patch please to see if it is acceptable or not? Patch is against 2.2.x branch. Regards, Basant.
[PATCH 39299] - Patch review request
Hi, I am Basant. I work in web tier group in Sun Microsystems Inc. I have submitted the patch for bug 39299. Summary : Internal Server Error (500) on COPY URI : http://issues.apache.org/bugzilla/show_bug.cgi?id=39299 Can some of the committer kindly review my patch please to see if it is acceptable or not? Patch is against 2.2.x branch. Regards, Basant.
Re: [PATCH 39299] - Patch review request
On Wed, 28 Feb 2007 14:31:19 -0800 Basant Kukreja [EMAIL PROTECTED] wrote: Hi, I am Basant. I work in web tier group in Sun Microsystems Inc. I have submitted the patch for bug 39299. Summary : Internal Server Error (500) on COPY URI : http://issues.apache.org/bugzilla/show_bug.cgi?id=39299 Can some of the committer kindly review my patch please to see if it is acceptable or not? Patch is against 2.2.x branch. 409 implies a condition the client can fix. Your patch tests for a particular condition that is likely to be fixable in a server with DAV uprunning. But AFAICS it could also give a bogus 409, for example in the case of a newly-installed and misconfigured server. Does the DAV RFC explicitly tell us to use 409 in this instance? -- Nick Kew Application Development with Apache - the Apache Modules Book http://www.apachetutor.org/
[PATCH 38014] - Patch review request
Hi, I work in the web tier group of Sun Microsystems Inc. I have submitted the patch for bug 38014 (The status '100 Continue' will be sent after the final status code) http://issues.apache.org/bugzilla/show_bug.cgi?id=38014 Can some of the committer kindly review my patch please to see if it is acceptable or not? Patch is against 2.2.x branch. Regards, Basant.
Re: mod_proxy http request body code review request
On 9/8/05, William A. Rowe, Jr. [EMAIL PROTECTED] wrote:
As many of the proxy reviewers could not follow
http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230744r1=230703r2=230744
I've provided the following fork of that codebase, to try to repair the
damage from the vetoed 171205 commit, in a piece by piece analysis of
what's changed and why.
any particular reason we lost the sendunchangedcl setting?
+ if (...
+ (r-input_filters == r-proto_input_filters
+ ...
+ || apr_table_get(r-subprocess_env, proxy-sendunchangedcl))) {
+ rb_method = RB_STREAM_CL;
+ }
Re: mod_proxy http request body code review request
Jeff Trawick wrote:
On 9/8/05, William A. Rowe, Jr. [EMAIL PROTECTED] wrote:
I've provided the following fork of that codebase, to try to repair the
damage from the vetoed 171205 commit, in a piece by piece analysis of
what's changed and why.
any particular reason we lost the sendunchangedcl setting?
+ if (...
+ (r-input_filters == r-proto_input_filters
+ ...
+ || apr_table_get(r-subprocess_env, proxy-sendunchangedcl))) {
+ rb_method = RB_STREAM_CL;
+ }
Yes; sendunchangedcl implies that the *Administrator* (as opposed to
module developers) have this information. In fact, for all intents
and purposes, Administrators do not. The issue comes down to the fact
that the Administrator is the last person who is going to have success
shooting this down when they misconfigure it. And once misconfigured,
you will end up with hangs (short bodies) or split request bodies.
The only way to determine that the length will be assuredly unchanged,
is to modify our API to record that the filter will, or will not, modify
the body length. And, if it will, and the filter can predetermine the
modification (say for example, one byte code page to unicode two-byte
input) then it should have the opportunity to reflect that in some new
'effective input CL' req_rec field.
But there's a second justification; because the new patch pre-reads up
to some fixed bytes of input, and handle the vast majority of POST
bodies in memory with a trusted CL, the necessity is somewhat lessened.
proxy-sendunchangedcl is one aspect of the original patch that yes, I
believed should be vetoed. There has to be a third-way here, and that's
to fix the API and handling CL's - both request and response body CL.
Bill
Re: mod_proxy http request body code review request
William A. Rowe, Jr. wrote:
Jeff Trawick wrote:
On 9/8/05, William A. Rowe, Jr. [EMAIL PROTECTED] wrote:
I've provided the following fork of that codebase, to try to repair the
damage from the vetoed 171205 commit, in a piece by piece analysis of
what's changed and why.
any particular reason we lost the sendunchangedcl setting?
+ if (...
+ (r-input_filters == r-proto_input_filters
+ ...
+ || apr_table_get(r-subprocess_env,
proxy-sendunchangedcl))) {
+ rb_method = RB_STREAM_CL;
+ }
But there's a second justification; because the new patch pre-reads up
to some fixed bytes of input, and handle the vast majority of POST
bodies in memory with a trusted CL, the necessity is somewhat lessened.
And I missed the third point; if the *protocol* input filter chain has
not been modified (no input request filters injected) then we continue
to presume that the CL is unchanged. So really the only code that
triggers spool is the situation where we have injected an input filter,
and we cannot (will not) chunk the request body, and the body is larger
than the read-ahead buffer. So this is most definately the exception,
not the rule, and the 'advantages' of some proxy-sendunchangedcl envvar
just seem not to justify the probability of misconfiguration and the
unhopefull task of diagnosing the actual symptoms.
Bill
Re: mod_proxy http request body code review request
Graham Leggett wrote: William A. Rowe, Jr. said: Please test, and vote on either the entire patch or nak specific commits below with technical justification, so that we might release 2.0.55 and avoid backing out 171205 once again. AIUI, JimJ and myself are +1 so far, which are all the votes collected (I'd addressed several -1's to address the technical concerns within the fixes below). To those who take the time to review, Thank You! Went through each patch in detail, and it all seems fine so far. Next step is to do some testing of the patch, which I'll do this weekend between getting on and off planes. To those who worked on getting this patch together, thank you :) NP - and thank you for your ongoing review; looking forward to your results so we can clear this showstopper - was all set to start pulling off an httpd 2.0.55 candidate today, and I'll check back in later to see if it can still come together. Thanks to all, esp. Jeff, who are busilly reviewing and backporting our final bug fixes for 2.0.55. Bill
Re: mod_proxy http request body code review request
On 9/8/05, William A. Rowe, Jr. [EMAIL PROTECTED] wrote: As many of the proxy reviewers could not follow http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230744r1=230703r2=230744 I've provided the following fork of that codebase, to try to repair the damage from the vetoed 171205 commit, in a piece by piece analysis of what's changed and why. Here's the why for normally using chunked encoding to origin server if client sent to our proxy using chunked encoding: If origin server doesn't support chunked, it wasn't going to work bypassing this proxy or using some other proxy implementations anyway; therefore unlikely that client code would be chunking to a server that doesn't support it (if this configuration only works when an uplevel Apache proxy is in the middle, admin can go a step further and use the sendcl setting to convert from chunked to cl) For the record, what is wrong with that? This is of interest because using chunked potentially results in lower resource use.
Re: mod_proxy http request body code review request
Jeff Trawick wrote: On 9/8/05, William A. Rowe, Jr. [EMAIL PROTECTED] wrote: As many of the proxy reviewers could not follow http://svn.apache.org/viewcvs.cgi/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c?rev=230744r1=230703r2=230744 I've provided the following fork of that codebase, to try to repair the damage from the vetoed 171205 commit, in a piece by piece analysis of what's changed and why. Here's the why for normally using chunked encoding to origin server if client sent to our proxy using chunked encoding: If origin server doesn't support chunked, it wasn't going to work bypassing this proxy or using some other proxy implementations anyway; therefore unlikely that client code would be chunking to a server that doesn't support it (if this configuration only works when an uplevel Apache proxy is in the middle, admin can go a step further and use the sendcl setting to convert from chunked to cl) For the record, what is wrong with that? Nothing by me, but do, go back over Roy's original response. Note that we can proxy a 1.0 server for a 1.1 client, and that client is welcome to pass us CL chunked, we'll do the homework. This is of interest because using chunked potentially results in lower resource use. This has not changed. If the body is smaller than the readahead buffer, we will send it with C-L, however if the body is larger, it prefers to be sent on in the original chunked format, unless the user configures to explicitly use sendcl. There's no significant increase in resource consumption, AFAICT. Does that make more sense? Bill
Re: mod_proxy http request body code review request
Further details (as I've promised for any query or objection)...
Jeff Trawick wrote:
Here's the why for normally using chunked encoding to origin server
if client sent to our proxy using chunked encoding:
If origin server doesn't support chunked, it wasn't going to work
bypassing this proxy or using some other proxy implementations anyway;
therefore unlikely that client code would be chunking to a server that
doesn't support it (if this configuration only works when an uplevel
Apache proxy is in the middle, admin can go a step further and use the
sendcl setting to convert from chunked to cl)
Let's take one quick look;
if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(input_brigade))) {
/* The whole thing fit, so our decision is trivial, use
* the filtered bytes read from the client for the request
* body Content-Length.
*
* If we expected no body, and read no body, do not set
* the Content-Length.
*/
if (old_cl_val || old_te_val || bytes_read) {
old_cl_val = apr_off_t_toa(r-pool, bytes_read);
}
rb_method = RB_STREAM_CL;
}
this case above is by Roy's insistance that, once we preread the small
in-memory buffer, we elect C-L always. Every HTTP app must be able to
handle a C-L request. Note we ignore other T-E flavors here (other than
'chunked') because we rejected them outright earlier, and the httpd
server API doesn't have a mechanism to handle non-chunked T-E bodies.
else if (old_te_val) {
if (force10
|| (apr_table_get(r-subprocess_env, proxy-sendcl)
!apr_table_get(r-subprocess_env,
proxy-sendchunks))) {
rb_method = RB_SPOOL_CL;
}
else {
rb_method = RB_STREAM_CHUNKED;
}
}
this case above is what I mentioned, that in almost any case, if we had
a T-E in the first place, and no demand (e.g. HTTP/1.0, or proxy-sendcl)
to use C-L, we will continue to use T-E. Note that if the user toggles
both proxy-sendcl and proxy-sendchunks, sendchunks will win.
This is of interest because using chunked potentially results in lower
resource use.
Could you provide an example of how a 16384 byte or smaller body could
ever be handled more quickly with T-E: chunked, rather than C-L: n
(=16384)? I can think of many cases (mostly our own) which are more
efficient when responding with a T-E, but never a case that it's faster
for the client to handle T-E rather than C-L.
Bill
Re: mod_proxy http request body code review request
William A. Rowe, Jr. said: Please test, and vote on either the entire patch or nak specific commits below with technical justification, so that we might release 2.0.55 and avoid backing out 171205 once again. AIUI, JimJ and myself are +1 so far, which are all the votes collected (I'd addressed several -1's to address the technical concerns within the fixes below). To those who take the time to review, Thank You! Went through each patch in detail, and it all seems fine so far. Next step is to do some testing of the patch, which I'll do this weekend between getting on and off planes. To those who worked on getting this patch together, thank you :) Regards, Graham --
