Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New filter: maskAccessLogQuery #2674

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 34 additions & 1 deletion filters/accesslog/control.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,12 @@ const (

// Common filter struct for holding access log state
type AccessLogFilter struct {
Enable bool
// Enable represents whether or not the access log is enabled.
Enable bool
// Prefixes contains the list of response code prefixes.
Prefixes []int
// MaskedQueryParams contains the list of query parameters (keys) that are masked/obfuscated in the access log.
MaskedQueryParams []string
}

func (al *AccessLogFilter) Request(ctx filters.FilterContext) {
Expand Down Expand Up @@ -87,3 +91,32 @@ func (*enableAccessLog) Name() string { return filters.EnableAccessLogName }
func (al *enableAccessLog) CreateFilter(args []interface{}) (filters.Filter, error) {
return extractFilterValues(args, true)
}

type maskAccessLogQuery struct{}

// NewMaskAccessLogQuery creates a filter spec to mask specific query parameters from the access log for a specific route.
// Takes in query param keys as arguments. When provided, the value of these keys are masked (i.e., hashed).
//
// maskAccessLogQuery("key_1", "key_2") to mask the value of provided keys in the access log.
func NewMaskAccessLogQuery() filters.Spec {
return &maskAccessLogQuery{}
}

func (*maskAccessLogQuery) Name() string { return filters.MaskAccessLogQueryName }

func (al *maskAccessLogQuery) CreateFilter(args []interface{}) (filters.Filter, error) {
if len(args) == 0 {
return nil, filters.ErrInvalidFilterParameters
}

keys := make([]string, len(args))
for i := range args {
if key, ok := args[i].(string); ok && key != "" {
keys[i] = key
} else {
return nil, filters.ErrInvalidFilterParameters
}
}

return &AccessLogFilter{Enable: true, MaskedQueryParams: keys}, nil
Copy link
Member

@AlexanderYastrebov AlexanderYastrebov Oct 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will override any preceding filter configuration.

Maybe we should think about supporting a usecase where we could specify default masked values

disableAccessLog(2,3,404,429) -> accessLogMaskQueryParams("access_token") -> fifo(2000,20,"1s")

and then user may specify additional values.
OTOH proposed solution is simple and in line with what already exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @AlexanderYastrebov

Yeah, I wanted to keep it simple. In hindsight, I am not sure my thinking is right.

If the current implementation is a no-go, I will try and spend a bit more time and see if I can come up with something when I get back from vacay (from tomorrow till the beginning of next month.)

}
27 changes: 21 additions & 6 deletions filters/accesslog/control_test.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
package accesslog

import (
"github.com/google/go-cmp/cmp"
"testing"

"github.com/google/go-cmp/cmp"

"github.com/zalando/skipper/filters"
"github.com/zalando/skipper/filters/filtertest"
)
Expand All @@ -20,14 +21,14 @@ func TestAccessLogControl(t *testing.T) {
msg: "enables-access-log",
state: NewEnableAccessLog(),
args: nil,
result: AccessLogFilter{true, make([]int, 0)},
result: AccessLogFilter{Enable: true, Prefixes: make([]int, 0)},
isError: false,
},
{
msg: "enable-access-log-selective",
state: NewEnableAccessLog(),
args: []interface{}{2, 4, 300},
result: AccessLogFilter{true, []int{2, 4, 300}},
result: AccessLogFilter{Enable: true, Prefixes: []int{2, 4, 300}},
isError: false,
},
{
Expand All @@ -41,23 +42,37 @@ func TestAccessLogControl(t *testing.T) {
msg: "disables-access-log",
state: NewDisableAccessLog(),
args: nil,
result: AccessLogFilter{false, make([]int, 0)},
result: AccessLogFilter{Enable: false, Prefixes: make([]int, 0)},
isError: false,
},
{
msg: "disables-access-log-selective",
state: NewDisableAccessLog(),
args: []interface{}{1, 201, 30},
result: AccessLogFilter{false, []int{1, 201, 30}},
result: AccessLogFilter{Enable: false, Prefixes: []int{1, 201, 30}},
isError: false,
},
{
msg: "disables-access-log-convert-float",
state: NewDisableAccessLog(),
args: []interface{}{1.0, 201},
result: AccessLogFilter{false, []int{1, 201}},
result: AccessLogFilter{Enable: false, Prefixes: []int{1, 201}},
isError: false,
},
{
msg: "mask-access-log-query",
state: NewMaskAccessLogQuery(),
args: []interface{}{"key_1"},
result: AccessLogFilter{Enable: true, MaskedQueryParams: []string{"key_1"}},
isError: false,
},
{
msg: "mask-access-log-query-convert-int",
state: NewMaskAccessLogQuery(),
args: []interface{}{1},
result: AccessLogFilter{Enable: true, MaskedQueryParams: []string{"key_1"}},
isError: true,
},
} {
t.Run(ti.msg, func(t *testing.T) {
f, err := ti.state.CreateFilter(ti.args)
Expand Down
2 changes: 1 addition & 1 deletion filters/accesslog/disable.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func (*accessLogDisabled) CreateFilter(args []interface{}) (filters.Filter, erro

func (al *accessLogDisabled) Request(ctx filters.FilterContext) {
bag := ctx.StateBag()
bag[AccessLogEnabledKey] = &AccessLogFilter{!al.disabled, nil}
bag[AccessLogEnabledKey] = &AccessLogFilter{!al.disabled, nil, nil}
}

func (*accessLogDisabled) Response(filters.FilterContext) {}
13 changes: 7 additions & 6 deletions filters/accesslog/disable_test.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
package accesslog

import (
"github.com/google/go-cmp/cmp"
"testing"

"github.com/google/go-cmp/cmp"

"github.com/zalando/skipper/filters"
"github.com/zalando/skipper/filters/filtertest"
)
Expand All @@ -18,31 +19,31 @@ func TestAccessLogDisabled(t *testing.T) {
{
msg: "false-value-enables-access-log",
state: []interface{}{"false"},
result: AccessLogFilter{true, nil},
result: AccessLogFilter{Enable: true, Prefixes: nil},
err: nil,
},
{
msg: "true-value-disables-access-log",
state: []interface{}{"true"},
result: AccessLogFilter{false, nil},
result: AccessLogFilter{Enable: false, Prefixes: nil},
err: nil,
},
{
msg: "unknown-argument-leads-to-error",
state: []interface{}{"unknownValue"},
result: AccessLogFilter{true, nil},
result: AccessLogFilter{Enable: true, Prefixes: nil},
err: filters.ErrInvalidFilterParameters,
},
{
msg: "no-arguments-lead-to-error",
state: []interface{}{},
result: AccessLogFilter{true, nil},
result: AccessLogFilter{Enable: true, Prefixes: nil},
err: filters.ErrInvalidFilterParameters,
},
{
msg: "multiple-arguments-lead-to-error",
state: []interface{}{"true", "second"},
result: AccessLogFilter{true, nil},
result: AccessLogFilter{Enable: true, Prefixes: nil},
err: filters.ErrInvalidFilterParameters,
},
} {
Expand Down
1 change: 1 addition & 0 deletions filters/builtin/builtin.go
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ func Filters() []filters.Spec {
//lint:ignore SA1019 due to backward compatibility
accesslog.NewAccessLogDisabled(),
accesslog.NewDisableAccessLog(),
accesslog.NewMaskAccessLogQuery(),
accesslog.NewEnableAccessLog(),
auth.NewForwardToken(),
auth.NewForwardTokenField(),
Expand Down
1 change: 1 addition & 0 deletions filters/filters.go
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,7 @@ const (
HeaderToQueryName = "headerToQuery"
QueryToHeaderName = "queryToHeader"
DisableAccessLogName = "disableAccessLog"
MaskAccessLogQueryName = "maskAccessLogQuery"
EnableAccessLogName = "enableAccessLog"
AuditLogName = "auditLog"
UnverifiedAuditLogName = "unverifiedAuditLog"
Expand Down
33 changes: 33 additions & 0 deletions logging/access.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"strings"
"time"

"github.com/cespare/xxhash/v2"
"github.com/sirupsen/logrus"

flowidFilter "github.com/zalando/skipper/filters/flowid"
Expand All @@ -21,6 +22,10 @@ const (
combinedLogFormat = commonLogFormat + ` "%s" "%s"`
// We add the duration in ms, a requested host and a flow id and audit log
accessLogFormat = combinedLogFormat + " %d %s %s %s\n"

// KeyMaskedQueryParams represents the key used to store and retrieve masked query parameters
// from the additional data.
KeyMaskedQueryParams = "maskedQueryParams"
)

type accessLogFormatter struct {
Expand Down Expand Up @@ -109,6 +114,29 @@ func stripQueryString(u string) string {
}
}

// maskQueryParams masks (i.e., hashing) specific query parameters in the provided request's URI.
// Returns the obfuscated URI.
func maskQueryParams(req *http.Request, maskedQueryParams []string) string {
strippedURI := stripQueryString(req.RequestURI)

params := req.URL.Query()
for i := range maskedQueryParams {
val := params.Get(maskedQueryParams[i])
if val == "" {
continue
}

hashed := hash(val)
params.Set(maskedQueryParams[i], fmt.Sprintf("%d", hashed))
}

return fmt.Sprintf("%s?%s", strippedURI, params.Encode())
}

func hash(val string) uint64 {
return xxhash.Sum64String(val)
}

// Logs an access event in Apache combined log format (with a minor customization with the duration).
// Additional allows to provide extra data that may be also logged, depending on the specific log format.
func LogAccess(entry *AccessEntry, additional map[string]interface{}) {
Expand Down Expand Up @@ -144,6 +172,10 @@ func LogAccess(entry *AccessEntry, additional map[string]interface{}) {
uri = entry.Request.RequestURI
if stripQuery {
uri = stripQueryString(uri)
} else if keys, ok := additional[KeyMaskedQueryParams].([]string); ok {
if len(keys) > 0 {
uri = maskQueryParams(entry.Request, keys)
}
}

auditHeader = entry.Request.Header.Get(logFilter.UnverifiedAuditHeader)
Expand All @@ -166,6 +198,7 @@ func LogAccess(entry *AccessEntry, additional map[string]interface{}) {
"auth-user": authUser,
}

delete(additional, KeyMaskedQueryParams)
for k, v := range additional {
logData[k] = v
}
Expand Down
53 changes: 49 additions & 4 deletions logging/access_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package logging
import (
"bytes"
"net/http"
"net/url"
"testing"
"time"

Expand All @@ -15,10 +16,21 @@ const logOutput = `127.0.0.1 - - [10/Oct/2000:13:55:36 -0700] "GET /apache_pb.gi
const logJSONOutput = `{"audit":"","auth-user":"","duration":42,"flow-id":"","host":"127.0.0.1","level":"info","method":"GET","msg":"","proto":"HTTP/1.1","referer":"","requested-host":"example.com","response-size":2326,"status":418,"timestamp":"10/Oct/2000:13:55:36 -0700","uri":"/apache_pb.gif","user-agent":""}`
const logExtendedJSONOutput = `{"audit":"","auth-user":"","duration":42,"extra":"extra","flow-id":"","host":"127.0.0.1","level":"info","method":"GET","msg":"","proto":"HTTP/1.1","referer":"","requested-host":"example.com","response-size":2326,"status":418,"timestamp":"10/Oct/2000:13:55:36 -0700","uri":"/apache_pb.gif","user-agent":""}`

func testRequest() *http.Request {
r, _ := http.NewRequest("GET", "http://[email protected]", nil)
func testRequest(params url.Values) *http.Request {
fullURL := url.URL{
Scheme: "http",
Host: "example.com",
RawQuery: params.Encode(),
}

r, err := http.NewRequest(http.MethodGet, fullURL.String(), nil)
if err != nil {
panic(err)
}

r.RequestURI = "/apache_pb.gif"
r.RemoteAddr = "127.0.0.1"

return r
}

Expand All @@ -29,19 +41,30 @@ func testDate() time.Time {

func testAccessEntry() *AccessEntry {
return &AccessEntry{
Request: testRequest(),
Request: testRequest(nil),
ResponseSize: 2326,
StatusCode: http.StatusTeapot,
RequestTime: testDate(),
Duration: 42 * time.Millisecond,
AuthUser: ""}
}

func testAccessEntryWithQueryParameters(params url.Values) *AccessEntry {
testAccessEntry := testAccessEntry()
testAccessEntry.Request = testRequest(params)

return testAccessEntry
}

func testAccessLog(t *testing.T, entry *AccessEntry, expectedOutput string, o Options) {
testAccessLogExtended(t, entry, nil, expectedOutput, o)
}

func testAccessLogExtended(t *testing.T, entry *AccessEntry, additional map[string]interface{}, expectedOutput string, o Options) {
func testAccessLogExtended(t *testing.T, entry *AccessEntry,
additional map[string]interface{},
expectedOutput string,
o Options,
) {
var buf bytes.Buffer
o.AccessLogOutput = &buf
Init(o)
Expand Down Expand Up @@ -74,6 +97,19 @@ func TestAccessLogFormatJSONWithAdditionalData(t *testing.T) {
testAccessLogExtended(t, testAccessEntry(), map[string]interface{}{"extra": "extra"}, logExtendedJSONOutput, Options{AccessLogJSONEnabled: true})
}

func TestAccessLogFormatJSONWithMaskedQueryParameters(t *testing.T) {
additional := map[string]interface{}{KeyMaskedQueryParams: []string{"foo"}}

params := url.Values{}
params.Add("foo", "bar")
testAccessLogExtended(t,
testAccessEntryWithQueryParameters(params),
additional,
`{"audit":"","auth-user":"","duration":42,"flow-id":"","host":"127.0.0.1","level":"info","method":"GET","msg":"","proto":"HTTP/1.1","referer":"","requested-host":"example.com","response-size":2326,"status":418,"timestamp":"10/Oct/2000:13:55:36 -0700","uri":"/apache_pb.gif?foo=5234164152756840025","user-agent":""}`,
Options{AccessLogJSONEnabled: true},
)
}

func TestAccessLogIgnoresEmptyEntry(t *testing.T) {
testAccessLogDefault(t, nil, "")
}
Expand Down Expand Up @@ -263,3 +299,12 @@ func TestAccessLogStripQuery(t *testing.T) {
entry.Request.RequestURI += "?foo=bar"
testAccessLog(t, entry, logOutput, Options{AccessLogStripQuery: true})
}

func TestHashQueryParamValue(t *testing.T) {
want := uint64(3728699739546630719)
got := hash("foo")

if got != want {
t.Errorf("\ngot %v\nwant %v", got, want)
}
}
8 changes: 7 additions & 1 deletion proxy/proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -1421,7 +1421,6 @@ func (p *Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request) {

defer func() {
accessLogEnabled, ok := ctx.stateBag[al.AccessLogEnabledKey].(*al.AccessLogFilter)

if !ok {
if p.accessLogDisabled {
accessLogEnabled = &disabledAccessLog
Expand All @@ -1444,6 +1443,13 @@ func (p *Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request) {
}

additionalData, _ := ctx.stateBag[al.AccessLogAdditionalDataKey].(map[string]interface{})
if len(accessLogEnabled.MaskedQueryParams) > 0 {
if additionalData == nil {
additionalData = make(map[string]interface{})
}

additionalData[logging.KeyMaskedQueryParams] = accessLogEnabled.MaskedQueryParams
}

logging.LogAccess(entry, additionalData)
}
Expand Down
Loading