Re: svn commit: r1895955 - in /httpd/httpd/branches/2.4.x: ./ CHANGES include/ap_mmn.h include/http_protocol.h modules/http/http_request.c modules/http2/h2_request.c modules/proxy/mod_proxy.c modules/
On Wed, Dec 15, 2021 at 7:54 AM Roy T. Fielding wrote: > > > On Dec 14, 2021, at 2:22 PM, Yann Ylavic wrote: > > > > On Tue, Dec 14, 2021 at 6:26 PM Roy T. Fielding wrote: > >> > >> This is a little confusing. It looks from the comment and code like the > >> change is restricting the request target that can be sent through a proxy, > >> which would be wrong. > > > > Yeah, the hunk I pointed to in the other message is doing this and I'm > > going to fix it. > > Thanks. Restored in r1895981, with some changes in ap_proxy_pre_request() to preserve the original URI when forward proxying. Up to some interested proxy module to catch it now, otherwise an error 500 is returned. Btw, 500 looks like the correct error status when reverse proxying (because we set the scheme according to the configuration in this case), but not really for the forward proxy case. Should we return something else for PROXYREQ_PROXY if no module handles the request? Maybe HTTP_NOT_IMPLEMENTED (but it's not about the method here..) or a client error like HTTP_NOT_ACCEPTABLE (not an Accept/Accept-Encoding thing either..) or yet HTTP_NOT_FOUND or the generic HTTP_BAD_REQUEST? Regards; Yann.
Re: svn commit: r1895955 - in /httpd/httpd/branches/2.4.x: ./ CHANGES include/ap_mmn.h include/http_protocol.h modules/http/http_request.c modules/http2/h2_request.c modules/proxy/mod_proxy.c modules/
> On Dec 14, 2021, at 2:22 PM, Yann Ylavic wrote: > > On Tue, Dec 14, 2021 at 6:26 PM Roy T. Fielding wrote: >> >> This is a little confusing. It looks from the comment and code like the >> change is restricting the request target that can be sent through a proxy, >> which would be wrong. > > Yeah, the hunk I pointed to in the other message is doing this and I'm > going to fix it. Thanks. >> OTOH, it would make sense that the proxy itself (the thing to which the >> proxied request is being sent) is limited to http(s) because that is a >> feature >> of HTTP. Is that what was intended? > > And the ap_post_read_request() part of the patch is enforcing that > yes, so keeping it would be fine right? Yep, that should be fine, at least until someone implements proxying via some other protocol. Roy
Re: svn commit: r1895955 - in /httpd/httpd/branches/2.4.x: ./ CHANGES include/ap_mmn.h include/http_protocol.h modules/http/http_request.c modules/http2/h2_request.c modules/proxy/mod_proxy.c modules/
On Tue, Dec 14, 2021 at 6:26 PM Roy T. Fielding wrote: > > This is a little confusing. It looks from the comment and code like the > change is restricting the request target that can be sent through a proxy, > which would be wrong. Yeah, the hunk I pointed to in the other message is doing this and I'm going to fix it. > > OTOH, it would make sense that the proxy itself (the thing to which the > proxied request is being sent) is limited to http(s) because that is a feature > of HTTP. Is that what was intended? And the ap_post_read_request() part of the patch is enforcing that yes, so keeping it would be fine right?
Re: svn commit: r1895955 - in /httpd/httpd/branches/2.4.x: ./ CHANGES include/ap_mmn.h include/http_protocol.h modules/http/http_request.c modules/http2/h2_request.c modules/proxy/mod_proxy.c modules/
On Tue, Dec 14, 2021 at 6:11 PM Roy T. Fielding wrote:
>
> I am pretty sure that this isn't correct, or at least seems like overkill.
> We should definitely block unix: from being forwarded, but why would
> we want to block things like a urn: resolver?
Oh I realize now that you are probably talking about the below hunk here:
--- httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c (original)
+++ httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c Tue Dec 14
15:35:56 2021
@@ -775,13 +775,13 @@ static int proxy_detect(request_rec *r)
/* Ick... msvc (perhaps others) promotes ternary short results to int */
-if (conf->req && r->parsed_uri.scheme) {
+if (conf->req && r->parsed_uri.scheme && r->parsed_uri.hostname) {
/* but it might be something vhosted */
-if (!(r->parsed_uri.hostname
- && !ap_cstr_casecmp(r->parsed_uri.scheme, ap_http_scheme(r))
- && ap_matches_request_vhost(r, r->parsed_uri.hostname,
-
(apr_port_t)(r->parsed_uri.port_str ? r->parsed_uri.port
- :
ap_default_port(r) {
+if (ap_cstr_casecmp(r->parsed_uri.scheme, ap_http_scheme(r)) != 0
+|| !ap_matches_request_vhost(r, r->parsed_uri.hostname,
+ (apr_port_t)(r->parsed_uri.port_str
+ ? r->parsed_uri.port
+ : ap_default_port(r {
And indeed this breaks a potential forward proxy module which would
handle an "urn:" like scheme.
We'd better check for a hostname in *our* proxy modules only, where it's needed.
Thanks;
Yann.
Re: svn commit: r1895955 - in /httpd/httpd/branches/2.4.x: ./ CHANGES include/ap_mmn.h include/http_protocol.h modules/http/http_request.c modules/http2/h2_request.c modules/proxy/mod_proxy.c modules/
I may have messed up, but this change is meant to return 400 early for requests with a fully qualified URI that is not http/https and that no forward-proxy module wants to handle (i.e. none did set r->proxyreq = PROXYREQ_PROXY at post_read_request time, like forward proxy modules do). Those requests end up being handled by the http module as if they were http, which looks wrong to me. Regards; Yann. On Tue, Dec 14, 2021 at 6:26 PM Roy T. Fielding wrote: > > This is a little confusing. It looks from the comment and code like the > change is restricting the request target that can be sent through a proxy, > which would be wrong. > > OTOH, it would make sense that the proxy itself (the thing to which the > proxied request is being sent) is limited to http(s) because that is a feature > of HTTP. Is that what was intended? > > Roy > > > On Dec 14, 2021, at 9:10 AM, Roy T. Fielding wrote: > > > > I am pretty sure that this isn't correct, or at least seems like overkill. > > We should definitely block unix: from being forwarded, but why would > > we want to block things like a urn: resolver? > > > > To be clear, I'd rather remove all proxy functionality from httpd than > > suggest to the world that http(s) schemes are the only names that > > can be proxied through HTTP. It would break the Web architecture, > > so -1 to that. > > > > Roy >
Re: svn commit: r1895955 - in /httpd/httpd/branches/2.4.x: ./ CHANGES include/ap_mmn.h include/http_protocol.h modules/http/http_request.c modules/http2/h2_request.c modules/proxy/mod_proxy.c modules/
This is a little confusing. It looks from the comment and code like the change is restricting the request target that can be sent through a proxy, which would be wrong. OTOH, it would make sense that the proxy itself (the thing to which the proxied request is being sent) is limited to http(s) because that is a feature of HTTP. Is that what was intended? Roy > On Dec 14, 2021, at 9:10 AM, Roy T. Fielding wrote: > > I am pretty sure that this isn't correct, or at least seems like overkill. > We should definitely block unix: from being forwarded, but why would > we want to block things like a urn: resolver? > > To be clear, I'd rather remove all proxy functionality from httpd than > suggest to the world that http(s) schemes are the only names that > can be proxied through HTTP. It would break the Web architecture, > so -1 to that. > > Roy
Re: svn commit: r1895955 - in /httpd/httpd/branches/2.4.x: ./ CHANGES include/ap_mmn.h include/http_protocol.h modules/http/http_request.c modules/http2/h2_request.c modules/proxy/mod_proxy.c modules/
I am pretty sure that this isn't correct, or at least seems like overkill. We should definitely block unix: from being forwarded, but why would we want to block things like a urn: resolver? To be clear, I'd rather remove all proxy functionality from httpd than suggest to the world that http(s) schemes are the only names that can be proxied through HTTP. It would break the Web architecture, so -1 to that. Roy > On Dec 14, 2021, at 7:35 AM, yla...@apache.org wrote: > > Author: ylavic > Date: Tue Dec 14 15:35:56 2021 > New Revision: 1895955 > > URL: http://svn.apache.org/viewvc?rev=1895955=rev > Log: > Merge r1895914, r1895921 from trunk: > > *) http: Enforce that fully qualified uri-paths not to be forward-proxied > have an http(s) scheme, and that the ones to be forward proxied have a > hostname, per HTTP specifications. > trunk patch: http://svn.apache.org/r1895914 > http://svn.apache.org/r1895921 > 2.4.x patch: > https://patch-diff.githubusercontent.com/raw/apache/httpd/pull/286.patch > backport PR: https://github.com/apache/httpd/pull/286 > +1: ylavic, minfrin, gbechis > > > mod_proxy: Detect unix: scheme syntax errors at load time. > > * modules/proxy/mod_proxy.c(add_pass, add_member, set_proxy_param, >proxysection): > Check return value of ap_proxy_de_socketfy(). > > * modules/proxy/proxy_util.c(ap_proxy_get_worker_ex): > Check return value of ap_proxy_de_socketfy(). > > > > http: Enforce that fully qualified uri-paths not to be forward-proxied > have an http(s) scheme, and that the ones to be forward proxied have a > hostname, per HTTP specifications. > > The early checks avoid failing the request later on and thus save cycles > for those invalid cases. > > > Submitted by: ylavic > Reviewed by: ylavic, minfrin, gbechis > Closes #286 > > Modified: >httpd/httpd/branches/2.4.x/ (props changed) >httpd/httpd/branches/2.4.x/CHANGES >httpd/httpd/branches/2.4.x/include/ap_mmn.h >httpd/httpd/branches/2.4.x/include/http_protocol.h >httpd/httpd/branches/2.4.x/modules/http/http_request.c >httpd/httpd/branches/2.4.x/modules/http2/h2_request.c >httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c >httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c >httpd/httpd/branches/2.4.x/server/protocol.c > > Propchange: httpd/httpd/branches/2.4.x/ > -- > Merged /httpd/httpd/trunk:r1895914,1895921 > > Modified: httpd/httpd/branches/2.4.x/CHANGES > URL: > http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/CHANGES?rev=1895955=1895954=1895955=diff > == > --- httpd/httpd/branches/2.4.x/CHANGES [utf-8] (original) > +++ httpd/httpd/branches/2.4.x/CHANGES [utf-8] Tue Dec 14 15:35:56 2021 > @@ -1,6 +1,10 @@ > -*- coding: utf-8 -*- > Changes with Apache 2.4.52 > > + *) http: Enforce that fully qualified uri-paths not to be forward-proxied > + have an http(s) scheme, and that the ones to be forward proxied have a > + hostname, per HTTP specifications. [Ruediger Pluem, Yann Ylavic] > + > *) OpenSSL autoconf detection improvement: pick up openssl.pc in the > specified openssl path. [Joe Orton] > > > Modified: httpd/httpd/branches/2.4.x/include/ap_mmn.h > URL: > http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/include/ap_mmn.h?rev=1895955=1895954=1895955=diff > == > --- httpd/httpd/branches/2.4.x/include/ap_mmn.h (original) > +++ httpd/httpd/branches/2.4.x/include/ap_mmn.h Tue Dec 14 15:35:56 2021 > @@ -586,6 +586,7 @@ > * dav_find_attr(). > * 20120211.120 (2.4.51-dev) Add dav_liveprop_elem structure and > * dav_get_liveprop_element(). > + * 20120211.121 (2.4.51-dev) Add ap_post_read_request() > * > */ > > @@ -594,7 +595,7 @@ > #ifndef MODULE_MAGIC_NUMBER_MAJOR > #define MODULE_MAGIC_NUMBER_MAJOR 20120211 > #endif > -#define MODULE_MAGIC_NUMBER_MINOR 120 /* 0...n */ > +#define MODULE_MAGIC_NUMBER_MINOR 121 /* 0...n */ > > /** > * Determine if the server's current MODULE_MAGIC_NUMBER is at least a > > Modified: httpd/httpd/branches/2.4.x/include/http_protocol.h > URL: > http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/include/http_protocol.h?rev=1895955=1895954=1895955=diff > == > --- httpd/httpd/branches/2.4.x/include/http_protocol.h (original) > +++ httpd/httpd/branches/2.4.x/include/http_protocol.h Tue Dec 14 15:35:56 > 2021 > @@ -96,6 +96,13 @@ AP_DECLARE(void) ap_get_mime_headers(req > AP_DECLARE(void) ap_get_mime_headers_core(request_rec *r, >
