From d2903a2b9f16fd91cf8cabede7af990342c7c337 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Wed, 17 Nov 2021 09:09:45 +0100 Subject: [PATCH 01/10] feat(logging): add a filter for query parameters --- .../caddyfile_adapt/log_filters.txt | 18 ++++ modules/logging/filters.go | 94 +++++++++++++++++++ modules/logging/filters_test.go | 19 ++++ 3 files changed, 131 insertions(+) create mode 100644 modules/logging/filters_test.go diff --git a/caddytest/integration/caddyfile_adapt/log_filters.txt b/caddytest/integration/caddyfile_adapt/log_filters.txt index 0949c1d40a8..7873b1c9b44 100644 --- a/caddytest/integration/caddyfile_adapt/log_filters.txt +++ b/caddytest/integration/caddyfile_adapt/log_filters.txt @@ -5,6 +5,10 @@ log { format filter { wrap console fields { + uri query { + replace foo REDACTED + delete bar + } request>headers>Authorization replace REDACTED request>headers>Server delete request>remote_addr ip_mask { @@ -40,6 +44,20 @@ log { "filter": "ip_mask", "ipv4_cidr": 24, "ipv6_cidr": 32 + }, + "uri": { + "actions": [ + { + "parameter": "foo", + "type": "replace", + "value": "REDACTED" + }, + { + "parameter": "bar", + "type": "delete" + } + ], + "filter": "query" } }, "format": "filter", diff --git a/modules/logging/filters.go b/modules/logging/filters.go index ef5a4cb9e2a..ba90bb3b69c 100644 --- a/modules/logging/filters.go +++ b/modules/logging/filters.go @@ -16,6 +16,7 @@ package logging import ( "net" + "net/url" "strconv" "github.com/caddyserver/caddy/v2" @@ -27,6 +28,7 @@ func init() { caddy.RegisterModule(DeleteFilter{}) caddy.RegisterModule(ReplaceFilter{}) caddy.RegisterModule(IPMaskFilter{}) + caddy.RegisterModule(QueryFilter{}) } // LogFieldFilter can filter (or manipulate) @@ -185,15 +187,107 @@ func (m IPMaskFilter) Filter(in zapcore.Field) zapcore.Field { return in } +type ActionType string + +const ( + ReplaceType ActionType = "replace" + DeleteType ActionType = "delete" +) + +type queryFilterAction struct { + Type ActionType `json:"type"` + Parameter string `json:"parameter"` + Value string `json:"value,omitempty"` +} + +// QueryFilter is a Caddy log field filter that +// filters query parameters. +type QueryFilter struct { + Actions []queryFilterAction `json:"actions"` +} + +// CaddyModule returns the Caddy module information. +func (QueryFilter) CaddyModule() caddy.ModuleInfo { + return caddy.ModuleInfo{ + ID: "caddy.logging.encoders.filter.query", + New: func() caddy.Module { return new(QueryFilter) }, + } +} + +// UnmarshalCaddyfile sets up the module from Caddyfile tokens. +func (m *QueryFilter) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { + for d.Next() { + for d.NextBlock(0) { + qfa := queryFilterAction{} + switch d.Val() { + case "replace": + if !d.NextArg() { + return d.ArgErr() + } + + qfa.Type = ReplaceType + qfa.Parameter = d.Val() + + if !d.NextArg() { + return d.ArgErr() + } + qfa.Value = d.Val() + + case "delete": + if !d.NextArg() { + return d.ArgErr() + } + + qfa.Type = DeleteType + qfa.Parameter = d.Val() + + default: + return d.Errf("unrecognized subdirective %s", d.Val()) + } + + m.Actions = append(m.Actions, qfa) + } + } + return nil +} + +// Filter filters the input field. +func (m QueryFilter) Filter(in zapcore.Field) zapcore.Field { + u, err := url.Parse(in.String) + if err != nil { + return in + } + + q := u.Query() + for _, a := range m.Actions { + switch a.Type { + case ReplaceType: + if q.Has(a.Parameter) { + q.Set(a.Parameter, a.Value) + } + + case DeleteType: + q.Del(a.Parameter) + } + } + + u.RawQuery = q.Encode() + in.String = u.String() + + return in +} + // Interface guards var ( _ LogFieldFilter = (*DeleteFilter)(nil) _ LogFieldFilter = (*ReplaceFilter)(nil) _ LogFieldFilter = (*IPMaskFilter)(nil) + _ LogFieldFilter = (*QueryFilter)(nil) _ caddyfile.Unmarshaler = (*DeleteFilter)(nil) _ caddyfile.Unmarshaler = (*ReplaceFilter)(nil) _ caddyfile.Unmarshaler = (*IPMaskFilter)(nil) + _ caddyfile.Unmarshaler = (*QueryFilter)(nil) _ caddy.Provisioner = (*IPMaskFilter)(nil) ) diff --git a/modules/logging/filters_test.go b/modules/logging/filters_test.go new file mode 100644 index 00000000000..2144f0b8bef --- /dev/null +++ b/modules/logging/filters_test.go @@ -0,0 +1,19 @@ +package logging + +import ( + "testing" + + "go.uber.org/zap/zapcore" +) + +func TestQueryFilter(t *testing.T) { + f := QueryFilter{[]queryFilterAction{ + {ReplaceType, "foo", "REDACTED"}, + {DeleteType, "bar", ""}, + }} + + out := f.Filter(zapcore.Field{String: "/path?foo=a&foo=b&bar=c&bar=d&baz=e"}) + if out.String != "/path?baz=e&foo=REDACTED" { + t.Fatalf("query parameters have not been filtered: %s", out.String) + } +} From c81696bbd364ac63ea79810e025474e8617abec6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Wed, 17 Nov 2021 09:27:20 +0100 Subject: [PATCH 02/10] fix(logging): compatibility with Go 1.16 --- modules/logging/filters.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/logging/filters.go b/modules/logging/filters.go index ba90bb3b69c..7d889591b64 100644 --- a/modules/logging/filters.go +++ b/modules/logging/filters.go @@ -262,7 +262,7 @@ func (m QueryFilter) Filter(in zapcore.Field) zapcore.Field { for _, a := range m.Actions { switch a.Type { case ReplaceType: - if q.Has(a.Parameter) { + if _, ok := q[a.Parameter]; ok { q.Set(a.Parameter, a.Value) } From e08a432fa7dbe5ef8bff1992cf52cb139dd16158 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Wed, 17 Nov 2021 22:57:35 +0100 Subject: [PATCH 03/10] fix: preserve number of variables --- modules/logging/filters.go | 4 ++-- modules/logging/filters_test.go | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/modules/logging/filters.go b/modules/logging/filters.go index 7d889591b64..77d1df356fe 100644 --- a/modules/logging/filters.go +++ b/modules/logging/filters.go @@ -262,8 +262,8 @@ func (m QueryFilter) Filter(in zapcore.Field) zapcore.Field { for _, a := range m.Actions { switch a.Type { case ReplaceType: - if _, ok := q[a.Parameter]; ok { - q.Set(a.Parameter, a.Value) + for i := range q[a.Parameter] { + q[a.Parameter][i] = a.Value } case DeleteType: diff --git a/modules/logging/filters_test.go b/modules/logging/filters_test.go index 2144f0b8bef..cf97338e73f 100644 --- a/modules/logging/filters_test.go +++ b/modules/logging/filters_test.go @@ -9,11 +9,13 @@ import ( func TestQueryFilter(t *testing.T) { f := QueryFilter{[]queryFilterAction{ {ReplaceType, "foo", "REDACTED"}, + {ReplaceType, "notexist", "REDACTED"}, {DeleteType, "bar", ""}, + {DeleteType, "notexist", ""}, }} out := f.Filter(zapcore.Field{String: "/path?foo=a&foo=b&bar=c&bar=d&baz=e"}) - if out.String != "/path?baz=e&foo=REDACTED" { + if out.String != "/path?baz=e&foo=REDACTED&foo=REDACTED" { t.Fatalf("query parameters have not been filtered: %s", out.String) } } From 31a2a83360dc1e2674607f46159a18bff4642bdb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Thu, 18 Nov 2021 13:31:19 +0100 Subject: [PATCH 04/10] docs: improve godoc --- modules/logging/filters.go | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/modules/logging/filters.go b/modules/logging/filters.go index 77d1df356fe..5070efeb988 100644 --- a/modules/logging/filters.go +++ b/modules/logging/filters.go @@ -190,19 +190,29 @@ func (m IPMaskFilter) Filter(in zapcore.Field) zapcore.Field { type ActionType string const ( + // Replace value(s) of query parameter(s). ReplaceType ActionType = "replace" - DeleteType ActionType = "delete" + // Delete query parameter(s). + DeleteType ActionType = "delete" ) type queryFilterAction struct { - Type ActionType `json:"type"` - Parameter string `json:"parameter"` - Value string `json:"value,omitempty"` + // `replace` to replace the value(s) associated with the parameter(s) or `delete` to remove them entirely. + Type ActionType `json:"type"` + // The name of the query parameter. + Parameter string `json:"parameter"` + // The value to use as replacement if the action is `replace`. + Value string `json:"value,omitempty"` } // QueryFilter is a Caddy log field filter that -// filters query parameters. +// filters query parameters from an URL. +// It updates the filtered URL to remove or replace query parameters containing sensitive data. +// For instance, it can be used to redact OAuth access tokens, session IDs, magic links tokens +// or JWT when passed as query parameters (which is generally considered a bad practice, but cannot +// always be avoided). type QueryFilter struct { + // A list of actions to apply to the query parameters of the URL. Actions []queryFilterAction `json:"actions"` } From bd8c3a3587b85a2a931872ee784cb17d30dac52c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Mon, 22 Nov 2021 22:34:59 +0100 Subject: [PATCH 05/10] feat: add validation --- modules/logging/filters.go | 23 +++++++++++++++++++++++ modules/logging/filters_test.go | 20 ++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/modules/logging/filters.go b/modules/logging/filters.go index 5070efeb988..028e1ef4707 100644 --- a/modules/logging/filters.go +++ b/modules/logging/filters.go @@ -15,6 +15,7 @@ package logging import ( + "errors" "net" "net/url" "strconv" @@ -196,6 +197,15 @@ const ( DeleteType ActionType = "delete" ) +func (a ActionType) IsValid() error { + switch a { + case ReplaceType, DeleteType: + return nil + } + + return errors.New("invalid action type") +} + type queryFilterAction struct { // `replace` to replace the value(s) associated with the parameter(s) or `delete` to remove them entirely. Type ActionType `json:"type"` @@ -216,6 +226,17 @@ type QueryFilter struct { Actions []queryFilterAction `json:"actions"` } +// Validate checks that action types are correct. +func (f *QueryFilter) Validate() error { + for _, a := range f.Actions { + if err := a.Type.IsValid(); err != nil { + return err + } + } + + return nil +} + // CaddyModule returns the Caddy module information. func (QueryFilter) CaddyModule() caddy.ModuleInfo { return caddy.ModuleInfo{ @@ -300,4 +321,6 @@ var ( _ caddyfile.Unmarshaler = (*QueryFilter)(nil) _ caddy.Provisioner = (*IPMaskFilter)(nil) + + _ caddy.Validator = (*QueryFilter)(nil) ) diff --git a/modules/logging/filters_test.go b/modules/logging/filters_test.go index cf97338e73f..dc83e9fce42 100644 --- a/modules/logging/filters_test.go +++ b/modules/logging/filters_test.go @@ -14,8 +14,28 @@ func TestQueryFilter(t *testing.T) { {DeleteType, "notexist", ""}, }} + if f.Validate() != nil { + t.Fatalf("the filter must be valid") + } + out := f.Filter(zapcore.Field{String: "/path?foo=a&foo=b&bar=c&bar=d&baz=e"}) if out.String != "/path?baz=e&foo=REDACTED&foo=REDACTED" { t.Fatalf("query parameters have not been filtered: %s", out.String) } } + +func TestValidateQueryFilter(t *testing.T) { + f := QueryFilter{[]queryFilterAction{ + {}, + }} + if f.Validate() == nil { + t.Fatalf("empty action type must be invalid") + } + + f = QueryFilter{[]queryFilterAction{ + {Type: "foo"}, + }} + if f.Validate() == nil { + t.Fatalf("unknown action type must be invalid") + } +} From 9b98a41e2c2359914e41af5dab1568912cfcc3dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Mon, 22 Nov 2021 23:06:35 +0100 Subject: [PATCH 06/10] add back lost suggestions --- modules/logging/filters.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/modules/logging/filters.go b/modules/logging/filters.go index 028e1ef4707..04ccc3b5466 100644 --- a/modules/logging/filters.go +++ b/modules/logging/filters.go @@ -209,18 +209,21 @@ func (a ActionType) IsValid() error { type queryFilterAction struct { // `replace` to replace the value(s) associated with the parameter(s) or `delete` to remove them entirely. Type ActionType `json:"type"` + // The name of the query parameter. Parameter string `json:"parameter"` + // The value to use as replacement if the action is `replace`. Value string `json:"value,omitempty"` } -// QueryFilter is a Caddy log field filter that -// filters query parameters from an URL. -// It updates the filtered URL to remove or replace query parameters containing sensitive data. -// For instance, it can be used to redact OAuth access tokens, session IDs, magic links tokens -// or JWT when passed as query parameters (which is generally considered a bad practice, but cannot -// always be avoided). +// QueryFilter is a Caddy log field filter that filters +// query parameters from a URL. +// +// This filter updates the logged URL string to remove or replace query +// parameters containing sensitive data. For instance, it can be used +// to redact any kind of secrets which were passed as query parameters, +// such as OAuth access tokens, session IDs, magic link tokens, etc. type QueryFilter struct { // A list of actions to apply to the query parameters of the URL. Actions []queryFilterAction `json:"actions"` From db3c780879e9223bca19cbb7a60b6525af9aa0a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Tue, 23 Nov 2021 09:41:57 +0100 Subject: [PATCH 07/10] Update modules/logging/filters.go Co-authored-by: Matt Holt --- modules/logging/filters.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/logging/filters.go b/modules/logging/filters.go index 04ccc3b5466..4f189316954 100644 --- a/modules/logging/filters.go +++ b/modules/logging/filters.go @@ -192,9 +192,9 @@ type ActionType string const ( // Replace value(s) of query parameter(s). - ReplaceType ActionType = "replace" + ReplaceAction ActionType = "replace" // Delete query parameter(s). - DeleteType ActionType = "delete" + DeleteAction ActionType = "delete" ) func (a ActionType) IsValid() error { From bdb2f1b07327d43dc4555739ef5c8d3a5ac467aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Tue, 23 Nov 2021 09:42:02 +0100 Subject: [PATCH 08/10] Update modules/logging/filters.go Co-authored-by: Matt Holt --- modules/logging/filters.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/logging/filters.go b/modules/logging/filters.go index 4f189316954..47cc7d189fd 100644 --- a/modules/logging/filters.go +++ b/modules/logging/filters.go @@ -188,7 +188,7 @@ func (m IPMaskFilter) Filter(in zapcore.Field) zapcore.Field { return in } -type ActionType string +type FilterAction string const ( // Replace value(s) of query parameter(s). From 8ceed5003b489d2a8ea5e54ae21a99b528910797 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Tue, 23 Nov 2021 09:46:00 +0100 Subject: [PATCH 09/10] fix suggestions --- modules/logging/filters.go | 18 +++++++++--------- modules/logging/filters_test.go | 8 ++++---- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/modules/logging/filters.go b/modules/logging/filters.go index 47cc7d189fd..443fe17270f 100644 --- a/modules/logging/filters.go +++ b/modules/logging/filters.go @@ -192,14 +192,14 @@ type FilterAction string const ( // Replace value(s) of query parameter(s). - ReplaceAction ActionType = "replace" + ReplaceAction FilterAction = "replace" // Delete query parameter(s). - DeleteAction ActionType = "delete" + DeleteAction FilterAction = "delete" ) -func (a ActionType) IsValid() error { +func (a FilterAction) IsValid() error { switch a { - case ReplaceType, DeleteType: + case ReplaceAction, DeleteAction: return nil } @@ -208,7 +208,7 @@ func (a ActionType) IsValid() error { type queryFilterAction struct { // `replace` to replace the value(s) associated with the parameter(s) or `delete` to remove them entirely. - Type ActionType `json:"type"` + Type FilterAction `json:"type"` // The name of the query parameter. Parameter string `json:"parameter"` @@ -259,7 +259,7 @@ func (m *QueryFilter) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { return d.ArgErr() } - qfa.Type = ReplaceType + qfa.Type = ReplaceAction qfa.Parameter = d.Val() if !d.NextArg() { @@ -272,7 +272,7 @@ func (m *QueryFilter) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { return d.ArgErr() } - qfa.Type = DeleteType + qfa.Type = DeleteAction qfa.Parameter = d.Val() default: @@ -295,12 +295,12 @@ func (m QueryFilter) Filter(in zapcore.Field) zapcore.Field { q := u.Query() for _, a := range m.Actions { switch a.Type { - case ReplaceType: + case ReplaceAction: for i := range q[a.Parameter] { q[a.Parameter][i] = a.Value } - case DeleteType: + case DeleteAction: q.Del(a.Parameter) } } diff --git a/modules/logging/filters_test.go b/modules/logging/filters_test.go index dc83e9fce42..7cc997fcbb2 100644 --- a/modules/logging/filters_test.go +++ b/modules/logging/filters_test.go @@ -8,10 +8,10 @@ import ( func TestQueryFilter(t *testing.T) { f := QueryFilter{[]queryFilterAction{ - {ReplaceType, "foo", "REDACTED"}, - {ReplaceType, "notexist", "REDACTED"}, - {DeleteType, "bar", ""}, - {DeleteType, "notexist", ""}, + {ReplaceAction, "foo", "REDACTED"}, + {ReplaceAction, "notexist", "REDACTED"}, + {DeleteAction, "bar", ""}, + {DeleteAction, "notexist", ""}, }} if f.Validate() != nil { From bf1b574391d2b9b2673b49c00a045e6cb818389e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Tue, 23 Nov 2021 09:47:17 +0100 Subject: [PATCH 10/10] unexport actions --- modules/logging/filters.go | 20 ++++++++++---------- modules/logging/filters_test.go | 8 ++++---- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/modules/logging/filters.go b/modules/logging/filters.go index 443fe17270f..ceb0d8ac353 100644 --- a/modules/logging/filters.go +++ b/modules/logging/filters.go @@ -188,18 +188,18 @@ func (m IPMaskFilter) Filter(in zapcore.Field) zapcore.Field { return in } -type FilterAction string +type filterAction string const ( // Replace value(s) of query parameter(s). - ReplaceAction FilterAction = "replace" + replaceAction filterAction = "replace" // Delete query parameter(s). - DeleteAction FilterAction = "delete" + deleteAction filterAction = "delete" ) -func (a FilterAction) IsValid() error { +func (a filterAction) IsValid() error { switch a { - case ReplaceAction, DeleteAction: + case replaceAction, deleteAction: return nil } @@ -208,7 +208,7 @@ func (a FilterAction) IsValid() error { type queryFilterAction struct { // `replace` to replace the value(s) associated with the parameter(s) or `delete` to remove them entirely. - Type FilterAction `json:"type"` + Type filterAction `json:"type"` // The name of the query parameter. Parameter string `json:"parameter"` @@ -259,7 +259,7 @@ func (m *QueryFilter) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { return d.ArgErr() } - qfa.Type = ReplaceAction + qfa.Type = replaceAction qfa.Parameter = d.Val() if !d.NextArg() { @@ -272,7 +272,7 @@ func (m *QueryFilter) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { return d.ArgErr() } - qfa.Type = DeleteAction + qfa.Type = deleteAction qfa.Parameter = d.Val() default: @@ -295,12 +295,12 @@ func (m QueryFilter) Filter(in zapcore.Field) zapcore.Field { q := u.Query() for _, a := range m.Actions { switch a.Type { - case ReplaceAction: + case replaceAction: for i := range q[a.Parameter] { q[a.Parameter][i] = a.Value } - case DeleteAction: + case deleteAction: q.Del(a.Parameter) } } diff --git a/modules/logging/filters_test.go b/modules/logging/filters_test.go index 7cc997fcbb2..883a1384466 100644 --- a/modules/logging/filters_test.go +++ b/modules/logging/filters_test.go @@ -8,10 +8,10 @@ import ( func TestQueryFilter(t *testing.T) { f := QueryFilter{[]queryFilterAction{ - {ReplaceAction, "foo", "REDACTED"}, - {ReplaceAction, "notexist", "REDACTED"}, - {DeleteAction, "bar", ""}, - {DeleteAction, "notexist", ""}, + {replaceAction, "foo", "REDACTED"}, + {replaceAction, "notexist", "REDACTED"}, + {deleteAction, "bar", ""}, + {deleteAction, "notexist", ""}, }} if f.Validate() != nil {