From 335b49fec9495f50c4bd8e170403a7e2b7d48361 Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Mon, 1 Nov 2021 01:47:48 -0400 Subject: [PATCH] caddyhttp: Split up remote address into IP and port Closes #4396, related to #4148 This is a breaking change, but we're already planning on removing the`common_log` field as well at the same time, so might as well all do it together. In general, the remote IP is the useful part of the remote address. The port is rarely useful, because it only identifies ephemeral information about the connection from the client. But we were logging both in one field, so certain tooling that would want to only get the remote IP would need to split it up. Splitting is non-trivial, because of IPv4, IPv6, shenanigans. So it's best if we split it up-front before logging. If the log consumer actually cares about the remote port, it can re-assemble it. --- admin.go | 8 +++++++- caddyconfig/httpcaddyfile/builtins_test.go | 4 ++-- .../caddyfile_adapt/global_options_log_custom.txt | 4 ++-- caddytest/integration/caddyfile_adapt/log_filters.txt | 4 ++-- modules/caddyhttp/marshalers.go | 10 +++++++++- 5 files changed, 22 insertions(+), 8 deletions(-) diff --git a/admin.go b/admin.go index 684526dd068..6960de52ab3 100644 --- a/admin.go +++ b/admin.go @@ -659,11 +659,17 @@ type adminHandler struct { // ServeHTTP is the external entry point for API requests. // It will only be called once per request. func (h adminHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { + ip, port, err := net.SplitHostPort(r.RemoteAddr) + if err != nil { + ip = r.RemoteAddr + port = "" + } log := Log().Named("admin.api").With( zap.String("method", r.Method), zap.String("host", r.Host), zap.String("uri", r.RequestURI), - zap.String("remote_addr", r.RemoteAddr), + zap.String("remote_ip", ip), + zap.String("remote_port", port), zap.Reflect("headers", r.Header), ) if r.TLS != nil { diff --git a/caddyconfig/httpcaddyfile/builtins_test.go b/caddyconfig/httpcaddyfile/builtins_test.go index a28229b41a0..991c649c486 100644 --- a/caddyconfig/httpcaddyfile/builtins_test.go +++ b/caddyconfig/httpcaddyfile/builtins_test.go @@ -37,7 +37,7 @@ func TestLogDirectiveSyntax(t *testing.T) { format filter { wrap console fields { - request>remote_addr ip_mask { + request>remote_ip ip_mask { ipv4 24 ipv6 32 } @@ -46,7 +46,7 @@ func TestLogDirectiveSyntax(t *testing.T) { } } `, - output: `{"logging":{"logs":{"default":{"exclude":["http.log.access.log0"]},"log0":{"encoder":{"fields":{"request\u003eremote_addr":{"filter":"ip_mask","ipv4_cidr":24,"ipv6_cidr":32}},"format":"filter","wrap":{"format":"console"}},"include":["http.log.access.log0"]}}},"apps":{"http":{"servers":{"srv0":{"listen":[":8080"],"logs":{"default_logger_name":"log0"}}}}}}`, + output: `{"logging":{"logs":{"default":{"exclude":["http.log.access.log0"]},"log0":{"encoder":{"fields":{"request\u003eremote_ip":{"filter":"ip_mask","ipv4_cidr":24,"ipv6_cidr":32}},"format":"filter","wrap":{"format":"console"}},"include":["http.log.access.log0"]}}},"apps":{"http":{"servers":{"srv0":{"listen":[":8080"],"logs":{"default_logger_name":"log0"}}}}}}`, expectError: false, }, { diff --git a/caddytest/integration/caddyfile_adapt/global_options_log_custom.txt b/caddytest/integration/caddyfile_adapt/global_options_log_custom.txt index e37e75e1e89..b39cce9e377 100644 --- a/caddytest/integration/caddyfile_adapt/global_options_log_custom.txt +++ b/caddytest/integration/caddyfile_adapt/global_options_log_custom.txt @@ -3,7 +3,7 @@ format filter { wrap console fields { - request>remote_addr ip_mask { + request>remote_ip ip_mask { ipv4 24 ipv6 32 } @@ -18,7 +18,7 @@ "custom-logger": { "encoder": { "fields": { - "request\u003eremote_addr": { + "request\u003eremote_ip": { "filter": "ip_mask", "ipv4_cidr": 24, "ipv6_cidr": 32 diff --git a/caddytest/integration/caddyfile_adapt/log_filters.txt b/caddytest/integration/caddyfile_adapt/log_filters.txt index 1bef4c21dc5..5253529fea2 100644 --- a/caddytest/integration/caddyfile_adapt/log_filters.txt +++ b/caddytest/integration/caddyfile_adapt/log_filters.txt @@ -15,7 +15,7 @@ log { replace foo REDACTED delete bar } - request>remote_addr ip_mask { + request>remote_ip ip_mask { ipv4 24 ipv6 32 } @@ -64,7 +64,7 @@ log { "request\u003eheaders\u003eServer": { "filter": "delete" }, - "request\u003eremote_addr": { + "request\u003eremote_ip": { "filter": "ip_mask", "ipv4_cidr": 24, "ipv6_cidr": 32 diff --git a/modules/caddyhttp/marshalers.go b/modules/caddyhttp/marshalers.go index bbb703ccdb1..c99c94ee3cd 100644 --- a/modules/caddyhttp/marshalers.go +++ b/modules/caddyhttp/marshalers.go @@ -16,6 +16,7 @@ package caddyhttp import ( "crypto/tls" + "net" "net/http" "strings" @@ -27,7 +28,14 @@ type LoggableHTTPRequest struct{ *http.Request } // MarshalLogObject satisfies the zapcore.ObjectMarshaler interface. func (r LoggableHTTPRequest) MarshalLogObject(enc zapcore.ObjectEncoder) error { - enc.AddString("remote_addr", r.RemoteAddr) + ip, port, err := net.SplitHostPort(r.RemoteAddr) + if err != nil { + ip = r.RemoteAddr + port = "" + } + + enc.AddString("remote_ip", ip) + enc.AddString("remote_port", port) enc.AddString("proto", r.Proto) enc.AddString("method", r.Method) enc.AddString("host", r.Host)