From 99a81435bcddcd9ca86ab82ec94476789fe77b9c Mon Sep 17 00:00:00 2001 From: Grant Shively Date: Tue, 20 Oct 2020 14:27:19 -0700 Subject: [PATCH] plugins/logs: Fixes unintended mutation of result When mask rules targeted /result, it was modifying both the result in the decision logs (intended) and the result in the API response (unintended). Added a step to deep copy the result only once, if there is at least one mask rule targeting the result. Fixes #2752 Signed-off-by: Grant Shively --- internal/deepcopy/deepcopy.go | 27 ++++++ internal/deepcopy/deepcopy_test.go | 31 ++++++ plugins/logs/mask.go | 38 ++++++-- plugins/logs/mask_test.go | 149 ++++++++++++++++++++++++++--- plugins/logs/plugin.go | 14 +-- storage/inmem/txn.go | 22 +---- 6 files changed, 236 insertions(+), 45 deletions(-) create mode 100644 internal/deepcopy/deepcopy.go create mode 100644 internal/deepcopy/deepcopy_test.go diff --git a/internal/deepcopy/deepcopy.go b/internal/deepcopy/deepcopy.go new file mode 100644 index 0000000000..cf8cc1cfe0 --- /dev/null +++ b/internal/deepcopy/deepcopy.go @@ -0,0 +1,27 @@ +// Copyright 2020 The OPA Authors. All rights reserved. +// Use of this source code is governed by an Apache2 +// license that can be found in the LICENSE file. + +package deepcopy + +// DeepCopy performs a recursive deep copy for nested slices/maps and +// returns the copied object. Supports []interface{} +// and map[string]interface{} only +func DeepCopy(val interface{}) interface{} { + switch val := val.(type) { + case []interface{}: + cpy := make([]interface{}, len(val)) + for i := range cpy { + cpy[i] = DeepCopy(val[i]) + } + return cpy + case map[string]interface{}: + cpy := make(map[string]interface{}, len(val)) + for k := range val { + cpy[k] = DeepCopy(val[k]) + } + return cpy + default: + return val + } +} diff --git a/internal/deepcopy/deepcopy_test.go b/internal/deepcopy/deepcopy_test.go new file mode 100644 index 0000000000..d0290bdb7d --- /dev/null +++ b/internal/deepcopy/deepcopy_test.go @@ -0,0 +1,31 @@ +// Copyright 2020 The OPA Authors. All rights reserved. +// Use of this source code is governed by an Apache2 +// license that can be found in the LICENSE file. + +package deepcopy + +import ( + "reflect" + "testing" +) + +func TestDeepCopyMapRoot(t *testing.T) { + target := map[string]interface{}{ + "a": map[string]interface{}{ + "b": []interface{}{ + "c", + "d", + }, + "e": "f", + }, + "x": "y", + } + result := DeepCopy(target).(map[string]interface{}) + if !reflect.DeepEqual(target, result) { + t.Fatal("Expected result of DeepCopy to be DeepEqual with original.") + } + result["a"] = "mutated" + if target["a"] == "mutated" { + t.Fatal("Expected target to remain unmutated when the DeepCopy result was mutated") + } +} diff --git a/plugins/logs/mask.go b/plugins/logs/mask.go index 1d5cdd40ce..b2d4dcda2a 100644 --- a/plugins/logs/mask.go +++ b/plugins/logs/mask.go @@ -11,6 +11,7 @@ import ( "strconv" "strings" + "github.com/open-policy-agent/opa/internal/deepcopy" "github.com/open-policy-agent/opa/util" ) @@ -34,6 +35,12 @@ type maskRule struct { failUndefinedPath bool } +type maskRuleSet struct { + OnRuleError func(*maskRule, error) + Rules []*maskRule + resultCopied bool +} + func (r maskRule) String() string { return "/" + strings.Join(r.escapedParts, "/") } @@ -254,20 +261,20 @@ func (r maskRule) mkdirp(node map[string]interface{}, path []string, value inter return nil } -func resultValueToMaskRules(rv interface{}) ([]*maskRule, error) { - +func newMaskRuleSet(rv interface{}, onRuleError func(*maskRule, error)) (*maskRuleSet, error) { bs, err := json.Marshal(rv) if err != nil { return nil, err } - + var mRuleSet = &maskRuleSet{ + OnRuleError: onRuleError, + } var rawRules []interface{} if err := util.Unmarshal(bs, &rawRules); err != nil { return nil, err } - mRules := []*maskRule{} for _, iface := range rawRules { switch v := iface.(type) { @@ -280,7 +287,7 @@ func resultValueToMaskRules(rv interface{}) ([]*maskRule, error) { return nil, err } - mRules = append(mRules, rule) + mRuleSet.Rules = append(mRuleSet.Rules, rule) case map[string]interface{}: @@ -307,12 +314,29 @@ func resultValueToMaskRules(rv interface{}) ([]*maskRule, error) { return nil, err } - mRules = append(mRules, rule) + mRuleSet.Rules = append(mRuleSet.Rules, rule) default: return nil, fmt.Errorf("invalid mask rule format encountered: %T", v) } } - return mRules, nil + return mRuleSet, nil +} + +func (rs maskRuleSet) Mask(event *EventV1) { + for _, mRule := range rs.Rules { + // result must be deep copied if there are any mask rules + // targeting it, to avoid modifying the result sent + // to the consumer + if mRule.escapedParts[0] == "result" && event.Result != nil && !rs.resultCopied { + resultCopy := deepcopy.DeepCopy(*event.Result) + event.Result = &resultCopy + rs.resultCopied = true + } + err := mRule.Mask(event) + if err != nil { + rs.OnRuleError(mRule, err) + } + } } diff --git a/plugins/logs/mask_test.go b/plugins/logs/mask_test.go index ef0e35322c..30a578cdc8 100644 --- a/plugins/logs/mask_test.go +++ b/plugins/logs/mask_test.go @@ -586,21 +586,23 @@ func TestMaskRuleMask(t *testing.T) { } } -func TestResultValueToMaskRules(t *testing.T) { - +func TestNewMaskRuleSet(t *testing.T) { tests := []struct { - note string - value interface{} - exp []maskRule - err error + onRuleError func(*maskRule, error) + note string + value interface{} + exp *maskRuleSet + err error }{ { - note: "invalid format: not []interface{}", - value: map[string]int{"invalid": 1}, - err: fmt.Errorf("json: cannot unmarshal object into Go value of type []interface {}"), + onRuleError: func(mRule *maskRule, err error) {}, + note: "invalid format: not []interface{}", + value: map[string]int{"invalid": 1}, + err: fmt.Errorf("json: cannot unmarshal object into Go value of type []interface {}"), }, { - note: "invalid format: nested type not string or map[string]interface{}", + onRuleError: func(mRule *maskRule, err error) {}, + note: "invalid format: nested type not string or map[string]interface{}", value: []interface{}{ []int{1, 2}, }, @@ -611,7 +613,7 @@ func TestResultValueToMaskRules(t *testing.T) { for _, tc := range tests { t.Run(tc.note, func(t *testing.T) { - _, err := resultValueToMaskRules(tc.value) + _, err := newMaskRuleSet(tc.value, func(mRule *maskRule, err error) {}) if err != nil { if tc.err.Error() != err.Error() { @@ -621,3 +623,128 @@ func TestResultValueToMaskRules(t *testing.T) { }) } } + +func TestMaskRuleSetMask(t *testing.T) { + + tests := []struct { + note string + rules []*maskRule + event string + exp string + expErr error + }{ + { + note: "erase input", + rules: []*maskRule{ + &maskRule{ + OP: maskOPRemove, + Path: "/input", + }, + }, + event: `{"input": {"a": 1}}`, + exp: `{"erased": ["/input"]}`, + }, + { + note: "erase result", + rules: []*maskRule{ + &maskRule{ + OP: maskOPRemove, + Path: "/result", + }, + }, + event: `{"result": {"a": 1}}`, + exp: `{"erased": ["/result"]}`, + }, + { + note: "erase input and result nested", + rules: []*maskRule{ + &maskRule{ + OP: maskOPRemove, + Path: "/input/a/b", + }, + &maskRule{ + OP: maskOPRemove, + Path: "/result/c/d", + }, + }, + event: `{"input":{"a":{"b":"removeme","y":"stillhere"}},"result":{"c":{"d":"removeme","z":"stillhere"}}}`, + exp: `{"input":{"a":{"y":"stillhere"}},"result":{"c":{"z":"stillhere"}},"erased":["/input/a/b", "/result/c/d"]}`, + }, + { + note: "expected rule error", + rules: []*maskRule{ + &maskRule{ + OP: maskOPRemove, + Path: "/result", + failUndefinedPath: true, + }, + }, + event: `{"input":"foo"}`, + exp: `{"input":"foo"}`, + expErr: errMaskInvalidObject, + }, + } + + for _, tc := range tests { + t.Run(tc.note, func(t *testing.T) { + ptr := &maskRuleSet{} + var ruleErr error + if tc.expErr != nil { + ptr.OnRuleError = func(mRule *maskRule, err error) { + ruleErr = err + } + } else { + ptr.OnRuleError = func(mRule *maskRule, err error) { + t.Fatalf(fmt.Sprintf("unexpected rule error, rule: %s, error: %s", mRule.String(), err.Error())) + } + } + for _, rule := range tc.rules { + var mRule *maskRule + var err error + if rule.failUndefinedPath { + mRule, err = newMaskRule(rule.Path, withOP(rule.OP), withValue(rule.Value), withFailUndefinedPath()) + } else { + mRule, err = newMaskRule(rule.Path, withOP(rule.OP), withValue(rule.Value)) + } + if err != nil { + panic(err) + } + ptr.Rules = append(ptr.Rules, mRule) + } + + var exp EventV1 + if err := util.UnmarshalJSON([]byte(tc.exp), &exp); err != nil { + panic(err) + } + + var event EventV1 + var origEvent EventV1 + if err := util.UnmarshalJSON([]byte(tc.event), &event); err != nil { + panic(err) + } + origEvent = event + + ptr.Mask(&event) + + // compare via json marshall to map tc input types + bs1, _ := json.MarshalIndent(exp, "", " ") + bs2, _ := json.MarshalIndent(event, "", " ") + if !bytes.Equal(bs1, bs2) { + t.Fatalf("Expected: %s\nGot: %s", string(bs1), string(bs2)) + } + + if origEvent.Result != nil && reflect.DeepEqual(origEvent.Result, event.Result) { + t.Fatal("Expected event.Result to be deep copied during masking, so that the event's original Result is not modified") + } + + if tc.expErr != nil { + if ruleErr == nil { + t.Fatalf("Expected: %s\nGot:%s", tc.expErr.Error(), "nil") + } + if tc.expErr != ruleErr { + t.Fatalf("Expected: %s\nGot:%s", tc.expErr.Error(), ruleErr.Error()) + } + } + }) + } +} diff --git a/plugins/logs/plugin.go b/plugins/logs/plugin.go index cb57d7d988..2e4b5a6ede 100644 --- a/plugins/logs/plugin.go +++ b/plugins/logs/plugin.go @@ -706,17 +706,17 @@ func (p *Plugin) maskEvent(ctx context.Context, txn storage.Transaction, event * return nil } - mRules, err := resultValueToMaskRules(rs[0].Expressions[0].Value) + mRuleSet, err := newMaskRuleSet( + rs[0].Expressions[0].Value, + func(mRule *maskRule, err error) { + p.logError("mask rule skipped: %s: %s", mRule.String(), err.Error()) + }, + ) if err != nil { return err } - for _, mRule := range mRules { - err := mRule.Mask(event) - if err != nil { - p.logError("mask rule skipped: %s: %s", mRule.String(), err.Error()) - } - } + mRuleSet.Mask(event) return nil } diff --git a/storage/inmem/txn.go b/storage/inmem/txn.go index ca3b0f0231..f97de6b0ba 100644 --- a/storage/inmem/txn.go +++ b/storage/inmem/txn.go @@ -9,6 +9,7 @@ import ( "encoding/json" "strconv" + "github.com/open-policy-agent/opa/internal/deepcopy" "github.com/open-policy-agent/opa/storage" ) @@ -204,7 +205,7 @@ func (txn *transaction) Read(path storage.Path) (interface{}, error) { return data, nil } - cpy := deepCopy(data) + cpy := deepcopy.DeepCopy(data) for _, update := range merge { cpy = update.Relative(path).Apply(cpy) @@ -388,25 +389,6 @@ func (u *update) Relative(path storage.Path) *update { return &cpy } -func deepCopy(val interface{}) interface{} { - switch val := val.(type) { - case []interface{}: - cpy := make([]interface{}, len(val)) - for i := range cpy { - cpy[i] = deepCopy(val[i]) - } - return cpy - case map[string]interface{}: - cpy := make(map[string]interface{}, len(val)) - for k := range val { - cpy[k] = deepCopy(val[k]) - } - return cpy - default: - return val - } -} - func ptr(data interface{}, path storage.Path) (interface{}, error) { node := data for i := range path {