> On 11 Nov 2018, at 18:43, Claudio Jeker <cje...@diehard.n-r-g.com> wrote:
>
>> On Sun, Nov 11, 2018 at 06:32:53PM +0100, Bruno Flueckiger wrote:
>>> On 11.11.18 15:29, Florian Obser wrote:
>>>> On Sun, Nov 11, 2018 at 01:46:06PM +0100, Sebastian Benoit wrote:
>>>> Bruno Flueckiger(inform...@gmx.net) on 2018.11.11 10:31:34 +0100:
>>>>> Hi
>>>>>
>>>>> When I run httpd(8) behind relayd(8) the access log of httpd contains
>>>>> the IP address of relayd, but not the IP address of the client. I've
>>>>> tried to match the logs of relayd(8) and httpd(8) using some scripting
>>>>> and failed.
>>>>>
>>>>> So I've written a patch for httpd(8). It stores the content of the
>>>>> X-Forwarded-For header in the third field of the log entry:
>>>>>
>>>>> www.example.com 192.0.2.99 192.0.2.134 - [11/Nov/2018:09:28:48 ...
>>>>>
>>>>> Cheers,
>>>>> Bruno
>>>>
>>>> I'm not sure we should do this unconditionally. With no relayd or other
>>>> proxy infront of httpd, this is (more) data the client controls.
>>>
>>> Isn't what httpd(8) currently logs apache's common log format? If
>>> people are shoving that through webalizer or something that will
>>> break. I don't think we can do this without a config option.
>>> Do we need LogFormat?
>>>
>>>>
>>>> Could this be a problem?
>>>>
>>>> code reads ok.
>>>>
>>>> /Benno
>>>>
>>
>> I've extended my patch with an option to the log directive. Both log xff
>> and log combined must be set for a server to log the content of the
>> X-Forwarded-For header. If only log combined is set the log entries
>> remain in the well-known format.
>>
>> This prevents clients from getting unwanted data in the log by default.
>> And it makes sure the log format remains default until one decides
>> actively to change it.
>
> From my experience with webservices is that today logging the IP is
> seldomly good enough. Please also include X-Forwarded-Port and maybe
> X-Forwarded-Proto.
> In general thanks to CG-NAT logging only IP is a bit pointless.
Or with relayd in front of it. :)
Welcome addition to httpd.
Mischa
>
> --
> :wq Claudio
>
>> Index: usr.sbin/httpd/config.c
>> ===================================================================
>> RCS file: /cvs/src/usr.sbin/httpd/config.c,v
>> retrieving revision 1.55
>> diff -u -p -r1.55 config.c
>> --- usr.sbin/httpd/config.c 20 Jun 2018 16:43:05 -0000 1.55
>> +++ usr.sbin/httpd/config.c 11 Nov 2018 14:45:47 -0000
>> @@ -427,6 +427,10 @@ config_getserver_config(struct httpd *en
>> if ((srv_conf->flags & f) == 0)
>> srv_conf->flags |= parent->flags & f;
>>
>> + f = SRVFLAG_XFF|SRVFLAG_NO_XFF;
>> + if ((srv_conf->flags & f) == 0)
>> + srv_conf->flags |= parent->flags & f;
>> +
>> f = SRVFLAG_AUTH|SRVFLAG_NO_AUTH;
>> if ((srv_conf->flags & f) == 0) {
>> srv_conf->flags |= parent->flags & f;
>> Index: usr.sbin/httpd/httpd.conf.5
>> ===================================================================
>> RCS file: /cvs/src/usr.sbin/httpd/httpd.conf.5,v
>> retrieving revision 1.101
>> diff -u -p -r1.101 httpd.conf.5
>> --- usr.sbin/httpd/httpd.conf.5 20 Jun 2018 16:43:05 -0000 1.101
>> +++ usr.sbin/httpd/httpd.conf.5 11 Nov 2018 14:45:47 -0000
>> @@ -455,6 +455,14 @@ If not specified, the default is
>> Enable or disable logging to
>> .Xr syslog 3
>> instead of the log files.
>> +.It Oo Ic no Oc Ic xff
>> +Enable or disable logging of the request header
>> +.Ar X-Forwarded-For
>> +if
>> +.Cm log combined
>> +is set. This header can be set by a reverse proxy like
>> +.Xr relayd 8
>> +and should contain the IP address of the client.
>> .El
>> .It Ic pass
>> Disable any previous
>> Index: usr.sbin/httpd/httpd.h
>> ===================================================================
>> RCS file: /cvs/src/usr.sbin/httpd/httpd.h,v
>> retrieving revision 1.142
>> diff -u -p -r1.142 httpd.h
>> --- usr.sbin/httpd/httpd.h 11 Oct 2018 09:52:22 -0000 1.142
>> +++ usr.sbin/httpd/httpd.h 11 Nov 2018 14:45:47 -0000
>> @@ -400,6 +400,8 @@ SPLAY_HEAD(client_tree, client);
>> #define SRVFLAG_DEFAULT_TYPE 0x00800000
>> #define SRVFLAG_PATH_REWRITE 0x01000000
>> #define SRVFLAG_NO_PATH_REWRITE 0x02000000
>> +#define SRVFLAG_XFF 0x04000000
>> +#define SRVFLAG_NO_XFF 0x08000000
>>
>> #define SRVFLAG_BITS \
>> "\10\01INDEX\02NO_INDEX\03AUTO_INDEX\04NO_AUTO_INDEX" \
>> Index: usr.sbin/httpd/parse.y
>> ===================================================================
>> RCS file: /cvs/src/usr.sbin/httpd/parse.y,v
>> retrieving revision 1.107
>> diff -u -p -r1.107 parse.y
>> --- usr.sbin/httpd/parse.y 1 Nov 2018 00:18:44 -0000 1.107
>> +++ usr.sbin/httpd/parse.y 11 Nov 2018 14:45:47 -0000
>> @@ -139,7 +139,7 @@ typedef struct {
>> %token LISTEN LOCATION LOG LOGDIR MATCH MAXIMUM NO NODELAY OCSP ON PORT
>> PREFORK
>> %token PROTOCOLS REQUESTS ROOT SACK SERVER SOCKET STRIP STYLE SYSLOG TCP
>> TICKET
>> %token TIMEOUT TLS TYPE TYPES HSTS MAXAGE SUBDOMAINS DEFAULT PRELOAD
>> REQUEST
>> -%token ERROR INCLUDE AUTHENTICATE WITH BLOCK DROP RETURN PASS REWRITE
>> +%token ERROR INCLUDE AUTHENTICATE WITH BLOCK DROP RETURN PASS REWRITE XFF
>> %token CA CLIENT CRL OPTIONAL
>> %token <v.string> STRING
>> %token <v.number> NUMBER
>> @@ -979,6 +979,10 @@ logflags : STYLE logstyle
>> free($2);
>> srv_conf->flags |= SRVFLAG_ERROR_LOG;
>> }
>> + | XFF {
>> + srv_conf->flags &= ~SRVFLAG_NO_XFF;
>> + srv_conf->flags |= SRVFLAG_XFF;
>> + }
>> ;
>>
>> logstyle : COMMON {
>> @@ -1308,7 +1312,8 @@ lookup(char *s)
>> { "tls", TLS },
>> { "type", TYPE },
>> { "types", TYPES },
>> - { "with", WITH }
>> + { "with", WITH },
>> + { "xff", XFF }
>> };
>> const struct keywords *p;
>>
>> Index: usr.sbin/httpd/server_http.c
>> ===================================================================
>> RCS file: /cvs/src/usr.sbin/httpd/server_http.c,v
>> retrieving revision 1.127
>> diff -u -p -r1.127 server_http.c
>> --- usr.sbin/httpd/server_http.c 4 Nov 2018 05:56:45 -0000 1.127
>> +++ usr.sbin/httpd/server_http.c 11 Nov 2018 14:45:47 -0000
>> @@ -1632,7 +1632,7 @@ server_log_http(struct client *clt, unsi
>> static char tstamp[64];
>> static char ip[INET6_ADDRSTRLEN];
>> time_t t;
>> - struct kv key, *agent, *referrer;
>> + struct kv key, *agent, *referrer, *xff;
>> struct tm *tm;
>> struct server_config *srv_conf;
>> struct http_descriptor *desc;
>> @@ -1642,6 +1642,7 @@ server_log_http(struct client *clt, unsi
>> char *version = NULL;
>> char *referrer_v = NULL;
>> char *agent_v = NULL;
>> + char *xff_v = NULL;
>>
>> if ((srv_conf = clt->clt_srv_conf) == NULL)
>> return (-1);
>> @@ -1666,7 +1667,7 @@ server_log_http(struct client *clt, unsi
>> *
>> * httpd's format is similar to these Apache LogFormats:
>> * "%v %h %l %u %t \"%r\" %>s %B"
>> - * "%v %h %l %u %t \"%r\" %>s %B \"%{Referer}i\" \"%{User-agent}i\""
>> + * "%v %h %a %u %t \"%r\" %>s %B \"%{Referer}i\" \"%{User-agent}i\""
>> */
>> switch (srv_conf->logformat) {
>> case LOG_FORMAT_COMMON:
>> @@ -1708,6 +1709,14 @@ server_log_http(struct client *clt, unsi
>> agent->kv_value == NULL)
>> agent = NULL;
>>
>> + xff = NULL;
>> + if (srv_conf->flags & SRVFLAG_XFF) {
>> + key.kv_key = "X-Forwarded-For";
>> + if (((xff = kv_find(&desc->http_headers, &key)) != NULL
>> + && xff->kv_value == NULL))
>> + xff = NULL;
>> + }
>> +
>> /* Use vis to encode input values from the header */
>> if (clt->clt_remote_user &&
>> stravis(&user, clt->clt_remote_user, HTTPD_LOGVIS) == -1)
>> @@ -1718,6 +1727,9 @@ server_log_http(struct client *clt, unsi
>> if (agent &&
>> stravis(&agent_v, agent->kv_value, HTTPD_LOGVIS) == -1)
>> goto done;
>> + if (xff &&
>> + stravis(&xff_v, xff->kv_value, HTTPD_LOGVIS) == -1)
>> + goto done;
>>
>> /* The following should be URL-encoded */
>> if (desc->http_path &&
>> @@ -1728,10 +1740,10 @@ server_log_http(struct client *clt, unsi
>> goto done;
>>
>> ret = evbuffer_add_printf(clt->clt_log,
>> - "%s %s - %s [%s] \"%s %s%s%s%s%s\""
>> + "%s %s %s %s [%s] \"%s %s%s%s%s%s\""
>> " %03d %zu \"%s\" \"%s\"\n",
>> - srv_conf->name, ip, clt->clt_remote_user == NULL ? "-" :
>> - user, tstamp,
>> + srv_conf->name, ip, xff == NULL ? "-" : xff_v,
>> + clt->clt_remote_user == NULL ? "-" : user, tstamp,
>> server_httpmethod_byid(desc->http_method),
>> desc->http_path == NULL ? "" : path,
>> desc->http_query == NULL ? "" : "?",
>> @@ -1762,6 +1774,7 @@ done:
>> free(version);
>> free(referrer_v);
>> free(agent_v);
>> + free(xff_v);
>>
>> return (ret);
>> }
>>
>
