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.
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);
}
