Mailing List Archive

On Wed, Jun 1, 2022 at 8:29 AM Ruediger Pluem <rpluem@apache.org> wrote:
>
> On 5/31/22 5:06 PM, ylavic@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_get(r->headers_in, "Host");
+ if (val) {
+ host = val;
+ }
+ ap_h1_append_header(header_brigade, r->pool, "Host", host);
+ apr_table_unset(r->headers_in, "Host");
+
/* Append the (remaining) headers to the brigade */
ap_h1_append_headers(header_brigade, r, r->headers_in);

--

Regards;
Yann.

On 6/1/22 11:10 AM, Yann Ylavic wrote:
> On Wed, Jun 1, 2022 at 8:29 AM Ruediger Pluem <rpluem@apache.org> wrote:
>>
>> On 5/31/22 5:06 PM, ylavic@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

On Wed, Jun 1, 2022 at 11:21 AM Ruediger Pluem <rpluem@apache.org> wrote:
>
> On 6/1/22 11:10 AM, Yann Ylavic wrote:
> > On Wed, Jun 1, 2022 at 8:29 AM Ruediger Pluem <rpluem@apache.org> wrote:
> >>
> >> On 5/31/22 5:06 PM, ylavic@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.