Re: svn commit: r1901485 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
On 6/1/22 12:44 PM, Yann Ylavic wrote:
> On Wed, Jun 1, 2022 at 12:39 PM Ruediger Pluem wrote:
>>
>> On 6/1/22 11:56 AM, yla...@apache.org wrote:
>>>
>>> +/* Compute Host header */
>>> if (dconf->preserve_host == 0) {
>>> if (ap_strchr_c(uri->hostname, ':')) { /* if literal IPv6 address
>>> */
>>> if (uri->port_str && uri->port != DEFAULT_HTTP_PORT) {
>>> @@ -3994,10 +3992,11 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbr
>>> host = uri->hostname;
>>> }
>>> }
>>> +apr_table_setn(r->headers_in, "Host", host);
>>> }
>>> else {
>>> -/* don't want to use r->hostname, as the incoming header might
>>> have a
>>> - * port attached
>>> +/* don't want to use r->hostname as the incoming header might have
>>> a
>>> + * port attached, let's use the original header.
>>> */
>>> host = saved_host;
>>> if (!host) {
>>> @@ -4007,10 +4006,9 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbr
>>>"on incoming request and preserve host set "
>>>"forcing hostname to be %s for uri %s",
>>>host, r->uri);
>>> +apr_table_setn(r->headers_in, "Host", host);
>>> }
>>> }
>>
>> Nitpick: Can't we do the apr_table_setn(r->headers_in, "Host", host); here
>> instead of in each if/else branch?
>
> This is a small optimization where if we reuse the existing Host
> header (saved_host) we don't need to set it again.
> But if it harms readability I can certainly change it as you say.
No worries. Leave it as is then. I vote for the performance benefit over the
readability benefit in this case :-)
Regards
Rüdiger
Re: svn commit: r1901485 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
On Wed, Jun 1, 2022 at 12:39 PM Ruediger Pluem wrote:
>
> On 6/1/22 11:56 AM, yla...@apache.org wrote:
> >
> > +/* Compute Host header */
> > if (dconf->preserve_host == 0) {
> > if (ap_strchr_c(uri->hostname, ':')) { /* if literal IPv6 address
> > */
> > if (uri->port_str && uri->port != DEFAULT_HTTP_PORT) {
> > @@ -3994,10 +3992,11 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbr
> > host = uri->hostname;
> > }
> > }
> > +apr_table_setn(r->headers_in, "Host", host);
> > }
> > else {
> > -/* don't want to use r->hostname, as the incoming header might
> > have a
> > - * port attached
> > +/* don't want to use r->hostname as the incoming header might have
> > a
> > + * port attached, let's use the original header.
> > */
> > host = saved_host;
> > if (!host) {
> > @@ -4007,10 +4006,9 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbr
> >"on incoming request and preserve host set "
> >"forcing hostname to be %s for uri %s",
> >host, r->uri);
> > +apr_table_setn(r->headers_in, "Host", host);
> > }
> > }
>
> Nitpick: Can't we do the apr_table_setn(r->headers_in, "Host", host); here
> instead of in each if/else branch?
This is a small optimization where if we reuse the existing Host
header (saved_host) we don't need to set it again.
But if it harms readability I can certainly change it as you say.
Regards;
Yann.
Re: svn commit: r1901485 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
On 6/1/22 11:56 AM, yla...@apache.org wrote:
> Author: ylavic
> Date: Wed Jun 1 09:56:43 2022
> New Revision: 1901485
>
> URL: http://svn.apache.org/viewvc?rev=1901485&view=rev
> Log:
> mod_proxy: Let fixup hooks know about the Host header (and eventually
> overwrite it).
>
> If proxy_run_fixups() sets a Host header there will be two ones sent to the
> origin server.
>
> Instead, let the hooks know about the Host by setting it in the r->headers_in
> passed to proxy_run_fixups(), and use the actual value afterwards.
> Note: if proxy_run_fixups() unsets the Host we'll keep ours.
>
> Suggested by: rpluem
>
>
> Modified:
> httpd/httpd/trunk/modules/proxy/proxy_util.c
>
> Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1901485&r1=1901484&r2=1901485&view=diff
> ==
> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Wed Jun 1 09:56:43 2022
> @@ -3975,9 +3975,7 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbr
> if (force10)
> apr_table_unset(r->headers_in, "Trailer");
>
> -/* We used to send `Host: ` always first, so let's keep it that
> - * way. No telling which legacy backend is relying no this.
> - */
> +/* Compute Host header */
> if (dconf->preserve_host == 0) {
> if (ap_strchr_c(uri->hostname, ':')) { /* if literal IPv6 address */
> if (uri->port_str && uri->port != DEFAULT_HTTP_PORT) {
> @@ -3994,10 +3992,11 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbr
> host = uri->hostname;
> }
> }
> +apr_table_setn(r->headers_in, "Host", host);
> }
> else {
> -/* don't want to use r->hostname, as the incoming header might have a
> - * port attached
> +/* don't want to use r->hostname as the incoming header might have a
> + * port attached, let's use the original header.
> */
> host = saved_host;
> if (!host) {
> @@ -4007,10 +4006,9 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbr
>"on incoming request and preserve host set "
>"forcing hostname to be %s for uri %s",
>host, r->uri);
> +apr_table_setn(r->headers_in, "Host", host);
> }
> }
Nitpick: Can't we do the apr_table_setn(r->headers_in, "Host", host); here
instead of in each if/else branch?
> -ap_h1_append_header(header_brigade, r->pool, "Host", host);
> -apr_table_unset(r->headers_in, "Host");
>
> /* handle Via */
> if (conf->viaopt == via_block) {
Regards
Rüdiger
Re: svn commit: r1901460 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
On Wed, Jun 1, 2022 at 11:21 AM Ruediger Pluem wrote:
>
> On 6/1/22 11:10 AM, Yann Ylavic wrote:
> > On Wed, Jun 1, 2022 at 8:29 AM Ruediger Pluem wrote:
> >>
> >> On 5/31/22 5:06 PM, yla...@apache.org wrote:
>
> >>> }
> >>> +ap_h1_append_header(header_brigade, r->pool, "Host", host);
> >>> +apr_table_unset(r->headers_in, "Host");
> >>
> >> This is nothing introduced by this change but something I noticed. If the
> >> fixup hook adds back a Host header,
> >> we would sent two Host headers to the backend.
> >
> > I thought about it, and also that proxy_run_fixups doesn't know about
> > the forwarded Host header.
> > I hesitated between simply ignoring any Host set by proxy_run_fixups
> > (i.e. move the apr_table_unset("Host") after that) or take it into
> > account.
> > The latter would be something like this:
>
> +1. This is more safe.
r1901485, thanks for the review!
Regards;
Yann.
Re: svn commit: r1901460 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
On 6/1/22 11:10 AM, Yann Ylavic wrote:
> On Wed, Jun 1, 2022 at 8:29 AM Ruediger Pluem wrote:
>>
>> On 5/31/22 5:06 PM, yla...@apache.org wrote:
>>> }
>>> +ap_h1_append_header(header_brigade, r->pool, "Host", host);
>>> +apr_table_unset(r->headers_in, "Host");
>>
>> This is nothing introduced by this change but something I noticed. If the
>> fixup hook adds back a Host header,
>> we would sent two Host headers to the backend.
>
> I thought about it, and also that proxy_run_fixups doesn't know about
> the forwarded Host header.
> I hesitated between simply ignoring any Host set by proxy_run_fixups
> (i.e. move the apr_table_unset("Host") after that) or take it into
> account.
> The latter would be something like this:
+1. This is more safe.
Regards
Rüdiger
Re: svn commit: r1901460 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
On Wed, Jun 1, 2022 at 8:29 AM Ruediger Pluem wrote:
>
> On 5/31/22 5:06 PM, yla...@apache.org wrote:
> >
> > /* We used to send `Host: ` always first, so let's keep it that
> > * way. No telling which legacy backend is relying no this.
> > */
> > if (dconf->preserve_host == 0) {
> > -const char *nhost;
> > if (ap_strchr_c(uri->hostname, ':')) { /* if literal IPv6 address
> > */
> > if (uri->port_str && uri->port != DEFAULT_HTTP_PORT) {
> > -nhost = apr_pstrcat(r->pool, "[", uri->hostname, "]:",
> > -uri->port_str, NULL);
> > +host = apr_pstrcat(r->pool, "[", uri->hostname, "]:",
> > + uri->port_str, NULL);
> > } else {
> > -nhost = apr_pstrcat(r->pool, "[", uri->hostname, "]",
> > NULL);
> > +host = apr_pstrcat(r->pool, "[", uri->hostname, "]", NULL);
> > }
> > } else {
> > if (uri->port_str && uri->port != DEFAULT_HTTP_PORT) {
> > -nhost = apr_pstrcat(r->pool, uri->hostname, ":",
> > -uri->port_str, NULL);
> > +host = apr_pstrcat(r->pool, uri->hostname, ":",
> > + uri->port_str, NULL);
> > } else {
> > -nhost = uri->hostname;
> > +host = uri->hostname;
> > }
> > }
> > -ap_h1_append_header(header_brigade, r->pool, "Host", nhost);
> > -apr_table_unset(request_headers, "Host");
> > }
> > else {
> > /* don't want to use r->hostname, as the incoming header might
> > have a
> > * port attached
> > */
> > -const char* hostname = apr_table_get(request_headers, "Host");
> > -if (!hostname) {
> > -hostname = r->server->server_hostname;
> > +host = saved_host;
> > +if (!host) {
> > +host = r->server->server_hostname;
> > ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01092)
> >"no HTTP 0.9 request (with no host line) "
> >"on incoming request and preserve host set "
> >"forcing hostname to be %s for uri %s",
> > - hostname, r->uri);
> > + host, r->uri);
> > }
> > -ap_h1_append_header(header_brigade, r->pool, "Host", hostname);
> > -apr_table_unset(request_headers, "Host");
> > }
> > +ap_h1_append_header(header_brigade, r->pool, "Host", host);
> > +apr_table_unset(r->headers_in, "Host");
>
> This is nothing introduced by this change but something I noticed. If the
> fixup hook adds back a Host header,
> we would sent two Host headers to the backend.
I thought about it, and also that proxy_run_fixups doesn't know about
the forwarded Host header.
I hesitated between simply ignoring any Host set by proxy_run_fixups
(i.e. move the apr_table_unset("Host") after that) or take it into
account.
The latter would be something like this:
Index: modules/proxy/proxy_util.c
===
--- modules/proxy/proxy_util.c(revision 1901461)
+++ modules/proxy/proxy_util.c(working copy)
@@ -3895,7 +3895,7 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
apr_bucket *e;
int force10 = 0, do_100_continue = 0;
conn_rec *origin = p_conn->connection;
-const char *host, *creds;
+const char *host, *creds, *val;
proxy_dir_conf *dconf = ap_get_module_config(r->per_dir_config,
&proxy_module);
/*
@@ -3975,9 +3975,7 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
if (force10)
apr_table_unset(r->headers_in, "Trailer");
-/* We used to send `Host: ` always first, so let's keep it that
- * way. No telling which legacy backend is relying no this.
- */
+/* Compute Host header */
if (dconf->preserve_host == 0) {
if (ap_strchr_c(uri->hostname, ':')) { /* if literal IPv6 address */
if (uri->port_str && uri->port != DEFAULT_HTTP_PORT) {
@@ -4009,8 +4007,7 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
host, r->uri);
}
}
-ap_h1_append_header(header_brigade, r->pool, "Host", host);
-apr_table_unset(r->headers_in, "Host");
+apr_table_setn(r->headers_in, "Host", host);
/* handle Via */
if (conf->viaopt == via_block) {
@@ -4132,6 +4129,18 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
/* run hook to fixup the request we are about to send */
proxy_run_fixups(r);
+/* We used to send `Host: ` always first, so let's keep it that
+ * way. No telling which legacy backend is relying no this.
+ * If proxy_run_fixups() changed the value, use it (though removal
+ * is ignored).
+ */
+val = apr_table_
