> On 12 Feb 2019, at 14:52, Bruno Flueckiger <inform...@gmx.net> wrote:
> 
> On 12.11.18 12:40, Bruno Flueckiger wrote:
>> On 11.11.18 18:43, Claudio Jeker 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.
>>> 
>>> -- 
>>> :wq Claudio
>>> 
>> 
>> Thanks for the hint, Claudio.
>> 
>> This version of my diff includes the two headers X-Forwarded-For and
>> X-Forwarded-Port. Instead of using the third field of the log entry I
>> add two additional fileds to it. The first one contains the value of
>> X-Forwarded-For and the second one that of X-Forwarded-Port.
>> 
>> I think that appending two fields might do less harm than replacing one
>> field at the beginning of the log entry. I'm not sure that adding
>> X-Forwarded-Proto to the log really brings a benefit, so I left it away
>> in this diff.
> 
> In the meantime I've run my diff on a webserver. In my experience
> webalizer has no problem with the modified log format. GoAccess on the
> other hand has troubles reading access.log, but that happens for me with
> and without the diff applied.
> 
> I think that most admins would profit from the diff. The setting is
> optional so it doesn't affect everybody rightaway. And I believe that
> those who enable it are ready to reconfigure whatever log parser they
> use.
> 
> Therefore I have reworked my diff so it applies to the -current tree.

Would be a very welcome addition to httpd.

Mischa

> 
> 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   12 Feb 2019 13:37:55 -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.102
> diff -u -p -r1.102 httpd.conf.5
> --- usr.sbin/httpd/httpd.conf.5       8 Feb 2019 11:46:07 -0000       1.102
> +++ usr.sbin/httpd/httpd.conf.5       12 Feb 2019 13:37:55 -0000
> @@ -464,6 +464,16 @@ 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 headers
> +.Ar X-Forwarded-For
> +and
> +.Ar X-Forwarded-Port
> +if
> +.Cm log combined
> +is set. These headers can be set by a reverse proxy like
> +.Xr relayd 8
> +and should contain the IP address and TCP port 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    12 Feb 2019 13:37:55 -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.108
> diff -u -p -r1.108 parse.y
> --- usr.sbin/httpd/parse.y    8 Jan 2019 18:35:27 -0000       1.108
> +++ usr.sbin/httpd/parse.y    12 Feb 2019 13:37:55 -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
> @@ -976,6 +976,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                {
> @@ -1305,7 +1309,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.129
> diff -u -p -r1.129 server_http.c
> --- usr.sbin/httpd/server_http.c      10 Feb 2019 13:41:27 -0000      1.129
> +++ usr.sbin/httpd/server_http.c      12 Feb 2019 13:37:55 -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, *xfp;
>       struct tm               *tm;
>       struct server_config    *srv_conf;
>       struct http_descriptor  *desc;
> @@ -1642,6 +1642,8 @@ server_log_http(struct client *clt, unsi
>       char                    *version = NULL;
>       char                    *referrer_v = NULL;
>       char                    *agent_v = NULL;
> +     char                    *xff_v = NULL;
> +     char                    *xfp_v = NULL;
> 
>       if ((srv_conf = clt->clt_srv_conf) == NULL)
>               return (-1);
> @@ -1667,6 +1669,9 @@ 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\""
> +      *
> +      * Setting the log option xff will change the combined format to:
> +      * "%v %h %l %u %t \"%r\" %>s %B \"%{Referer}i\" \"%{User-agent}i\" %a 
> %{remote}p"
>        */
>       switch (srv_conf->logformat) {
>       case LOG_FORMAT_COMMON:
> @@ -1708,6 +1713,19 @@ server_log_http(struct client *clt, unsi
>                   agent->kv_value == NULL)
>                       agent = NULL;
> 
> +             xff = xfp = 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;
> +
> +                     key.kv_key = "X-Forwarded-Port";
> +                     if (((xfp = kv_find(&desc->http_headers, &key)) != NULL
> +                         && xfp->kv_value == NULL))
> +                             xfp = NULL;
> +             }
> +
>               /* Use vis to encode input values from the header */
>               if (clt->clt_remote_user &&
>                   stravis(&user, clt->clt_remote_user, HTTPD_LOGVIS) == -1)
> @@ -1726,6 +1744,14 @@ server_log_http(struct client *clt, unsi
>                   stravis(&agent_v, agent->kv_value, HTTPD_LOGVIS) == -1)
>                       goto done;
> 
> +             if (xff &&
> +                 stravis(&xff_v, xff->kv_value, HTTPD_LOGVIS) == -1)
> +                     goto done;
> +
> +             if (xfp &&
> +                 stravis(&xfp_v, xfp->kv_value, HTTPD_LOGVIS) == -1)
> +                     goto done;
> +
>               /* The following should be URL-encoded */
>               if (desc->http_path &&
>                   (path = url_encode(desc->http_path)) == NULL)
> @@ -1736,7 +1762,7 @@ server_log_http(struct client *clt, unsi
> 
>               ret = evbuffer_add_printf(clt->clt_log,
>                   "%s %s - %s [%s] \"%s %s%s%s%s%s\""
> -                 " %03d %zu \"%s\" \"%s\"\n",
> +                 " %03d %zu \"%s\" \"%s\" %s %s\n",
>                   srv_conf->name, ip, user == NULL ? "-" :
>                   user, tstamp,
>                   server_httpmethod_byid(desc->http_method),
> @@ -1747,7 +1773,9 @@ server_log_http(struct client *clt, unsi
>                   desc->http_version == NULL ? "" : version,
>                   code, len,
>                   referrer == NULL ? "" : referrer_v,
> -                 agent == NULL ? "" : agent_v);
> +                 agent == NULL ? "" : agent_v,
> +                 xff == NULL ? "-" : xff_v,
> +                 xfp == NULL ? "-" : xfp_v);
> 
>               break;
> 
> @@ -1769,6 +1797,8 @@ done:
>       free(version);
>       free(referrer_v);
>       free(agent_v);
> +     free(xff_v);
> +     free(xfp_v);
> 
>       return (ret);
> }

Reply via email to