Re: [PATCH 1 of 2] HTTP: Add client source port to any error that is logged
Hello! On Thu, Apr 24, 2014 at 04:37:31PM -0700, Quanah Gibson-Mount wrote: --On April 24, 2014 at 10:26:07 PM +0400 Maxim Dounin mdou...@mdounin.ru wrote: Yes, that is true, but why only implement a partial solution? With CGN, only logging the IP is fairly useless in all cases. To truly get useful information going forward, the IP + PORT of the client should logged in all cases. Access log certainly can be configured to provide enough enformation to match any given error log message to a port if needed. There is no need to implement anything, solution is already here. The error log currently only provides the IP. While I'm guessing you could do things like correlate timestamps, it's still going to be a pain. Having the port readily available everywhere makes tracking a specific user much easier to do. Each error log message contains connection id. And, by asking about why implement a partical solution you are overlooking the fact that proposed solution is partial as well - it doesn't change c-addr_text to ensure proper logging in all places (this would be a bad idea for other reasons, but it's another question), but rather tries to hack on the http error logging code to introduce remote port logging. This is far from being a complete solution. I'm certainly willing to address any deficiencies, but I'd want to make sure it would follow whatever you want in the product before investing more time on it. ;) For now it meets the needs of our customer in Belgium who has to start dealing with the legal requirements of client port logging sooner than later. Fair enough. As I already said in my first messages, I tend to think that there are no changes needed here. -- Maxim Dounin http://nginx.org/ ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
[PATCH 1 of 2] HTTP: Add client source port to any error that is logged
# HG changeset patch
# User Quanah Gibson-Mount qua...@zimbra.com
# Date 1398357557 18000
# Node ID 4b7d2e503c06758330aabcc21ffbbab77f09568e
# Parent 1b0c55d38d0b7ba69dcad79760a3fadc30696a9d
HTTP: Add client source port to any error that is logged
For TRAC ticket 531
diff -r 1b0c55d38d0b -r 4b7d2e503c06 src/http/ngx_http_request.c
--- a/src/http/ngx_http_request.c Thu Apr 24 16:54:23 2014 +0400
+++ b/src/http/ngx_http_request.c Thu Apr 24 11:39:17 2014 -0500
@@ -3548,6 +3548,11 @@
u_char *p;
ngx_http_request_t *r;
ngx_http_log_ctx_t *ctx;
+ngx_uint_t remote_port=0;
+struct sockaddr_in *sin;
+#if (NGX_HAVE_INET6)
+struct sockaddr_in6*sin6;
+#endif
if (log-action) {
p = ngx_snprintf(buf, len, while %s, log-action);
@@ -3557,15 +3562,32 @@
ctx = log-data;
-p = ngx_snprintf(buf, len, , client: %V, ctx-connection-addr_text);
-len -= p - buf;
-
r = ctx-request;
-
if (r) {
+switch (r-connection-sockaddr-sa_family) {
+#if (NGX_HAVE_INET6)
+case AF_INET6:
+sin6 = (struct sockaddr_in6 *) r-connection-sockaddr;
+remote_port = ntohs(sin6-sin6_port);
+break;
+#endif
+
+default: /* AF_INET */
+sin = (struct sockaddr_in *) r-connection-sockaddr;
+remote_port = ntohs(sin-sin_port);
+break;
+}
+
+if (remote_port remote_port 65536) {
+ p = ngx_snprintf(buf, len, , client: %V:%ui,
ctx-connection-addr_text,remote_port);
+} else {
+ p = ngx_snprintf(buf, len, , client: %V,
ctx-connection-addr_text);
+}
+len -= p - buf;
+
return r-log_handler(r, ctx-current_request, p, len);
-
} else {
+p = ngx_snprintf(buf, len, , client: %V,
ctx-connection-addr_text);
p = ngx_snprintf(p, len, , server: %V,
ctx-connection-listening-addr_text);
}
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH 1 of 2] HTTP: Add client source port to any error that is logged
Hello! On Thu, Apr 24, 2014 at 12:19:46PM -0500, Quanah Gibson-Mount wrote: # HG changeset patch # User Quanah Gibson-Mount qua...@zimbra.com # Date 1398357557 18000 # Node ID 4b7d2e503c06758330aabcc21ffbbab77f09568e # Parent 1b0c55d38d0b7ba69dcad79760a3fadc30696a9d HTTP: Add client source port to any error that is logged For TRAC ticket 531 I tend to say No, thanks. If needed due to local regulations, $remote_port can be added to log_format. -- Maxim Dounin http://nginx.org/ ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH 1 of 2] HTTP: Add client source port to any error that is logged
--On April 24, 2014 at 9:37:54 PM +0400 Maxim Dounin mdou...@mdounin.ru wrote: I tend to say No, thanks. If needed due to local regulations, $remote_port can be added to log_format. $remote_port in the log format section only covers errors logged to the access log, it does not cover errors in the error log. The submitted patch handles the error log. --Quanah -- Quanah Gibson-Mount Principal Software Engineer Zimbra, Inc Zimbra :: the leader in open source messaging and collaboration ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH 1 of 2] HTTP: Add client source port to any error that is logged
--On April 24, 2014 at 10:41:43 AM -0700 Quanah Gibson-Mount qua...@zimbra.com wrote: --On April 24, 2014 at 9:37:54 PM +0400 Maxim Dounin mdou...@mdounin.ru wrote: I tend to say No, thanks. If needed due to local regulations, $remote_port can be added to log_format. $remote_port in the log format section only covers errors logged to the access log, accesses even.. :P --Quanah -- Quanah Gibson-Mount Principal Software Engineer Zimbra, Inc Zimbra :: the leader in open source messaging and collaboration ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH 1 of 2] HTTP: Add client source port to any error that is logged
Hello! On Thu, Apr 24, 2014 at 10:41:43AM -0700, Quanah Gibson-Mount wrote: --On April 24, 2014 at 9:37:54 PM +0400 Maxim Dounin mdou...@mdounin.ru wrote: I tend to say No, thanks. If needed due to local regulations, $remote_port can be added to log_format. $remote_port in the log format section only covers errors logged to the access log, it does not cover errors in the error log. The submitted patch handles the error log. I understand the difference, thank you. The ticket in question only talked about error_log in context of mail module, where is no separate access logging to meet the alleged regulations. -- Maxim Dounin http://nginx.org/ ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH 1 of 2] HTTP: Add client source port to any error that is logged
--On April 24, 2014 at 9:56:48 PM +0400 Maxim Dounin mdou...@mdounin.ru wrote: $remote_port in the log format section only covers errors logged to the access log, it does not cover errors in the error log. The submitted patch handles the error log. I understand the difference, thank you. The ticket in question only talked about error_log in context of mail module, where is no separate access logging to meet the alleged regulations. Yes, that is true, but why only implement a partial solution? With CGN, only logging the IP is fairly useless in all cases. To truly get useful information going forward, the IP + PORT of the client should logged in all cases. --Quanah -- Quanah Gibson-Mount Principal Software Engineer Zimbra, Inc Zimbra :: the leader in open source messaging and collaboration ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH 1 of 2] HTTP: Add client source port to any error that is logged
Hello! On Thu, Apr 24, 2014 at 11:06:29AM -0700, Quanah Gibson-Mount wrote: --On April 24, 2014 at 9:56:48 PM +0400 Maxim Dounin mdou...@mdounin.ru wrote: $remote_port in the log format section only covers errors logged to the access log, it does not cover errors in the error log. The submitted patch handles the error log. I understand the difference, thank you. The ticket in question only talked about error_log in context of mail module, where is no separate access logging to meet the alleged regulations. Yes, that is true, but why only implement a partial solution? With CGN, only logging the IP is fairly useless in all cases. To truly get useful information going forward, the IP + PORT of the client should logged in all cases. Access log certainly can be configured to provide enough enformation to match any given error log message to a port if needed. There is no need to implement anything, solution is already here. And, by asking about why implement a partical solution you are overlooking the fact that proposed solution is partial as well - it doesn't change c-addr_text to ensure proper logging in all places (this would be a bad idea for other reasons, but it's another question), but rather tries to hack on the http error logging code to introduce remote port logging. This is far from being a complete solution. -- Maxim Dounin http://nginx.org/ ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH 1 of 2] HTTP: Add client source port to any error that is logged
--On April 24, 2014 at 10:26:07 PM +0400 Maxim Dounin mdou...@mdounin.ru wrote: Yes, that is true, but why only implement a partial solution? With CGN, only logging the IP is fairly useless in all cases. To truly get useful information going forward, the IP + PORT of the client should logged in all cases. Access log certainly can be configured to provide enough enformation to match any given error log message to a port if needed. There is no need to implement anything, solution is already here. The error log currently only provides the IP. While I'm guessing you could do things like correlate timestamps, it's still going to be a pain. Having the port readily available everywhere makes tracking a specific user much easier to do. And, by asking about why implement a partical solution you are overlooking the fact that proposed solution is partial as well - it doesn't change c-addr_text to ensure proper logging in all places (this would be a bad idea for other reasons, but it's another question), but rather tries to hack on the http error logging code to introduce remote port logging. This is far from being a complete solution. I'm certainly willing to address any deficiencies, but I'd want to make sure it would follow whatever you want in the product before investing more time on it. ;) For now it meets the needs of our customer in Belgium who has to start dealing with the legal requirements of client port logging sooner than later. --Quanah -- Quanah Gibson-Mount Server Architect Zimbra, Inc Zimbra :: the leader in open source messaging and collaboration ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
