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

allow custom masking of fields #2379

Closed
daniel-garcia opened this issue May 7, 2020 · 5 comments · Fixed by #2433
Closed

allow custom masking of fields #2379

daniel-garcia opened this issue May 7, 2020 · 5 comments · Fixed by #2433

Comments

@daniel-garcia
Copy link

Expected Behavior

Masking sensitive fields should allow the field to be mutated instead of just dropped.

Actual Behavior

Masking only allows sensitive fields to be dropped.

Steps to Reproduce the Problem

https://www.openpolicyagent.org/docs/v0.13.5/decision-logs/#masking-sensitive-data

Not possible to modify a field... only drop it.

Additional Info

My use case is:
I am passing the entire JWT and other information to evaluate a decision. The JWT contains a signature that is being verified in OPA. The decision logs, however, expose the entire JWT which can be used in a playblack attack. If I drop the field, i don't have the entire context that was using for evaluating the decision. What I really want to do is just drop the JWT signature.

@tsandall
Copy link
Member

tsandall commented May 7, 2020

Thanks for filing this @daniel-garcia ! Just adding a note here that another similar use case would be additive changes like appending a signature based on the some fields (or all fields) in the decision log to ensure integrity.

@dkiser
Copy link
Contributor

dkiser commented May 17, 2020

Any suggestions on where/how to start picking up this work? I see most of the logic is contained in /plugins/logs/plugin.go and /plugins/logs/ptr.go.

@tsandall
Copy link
Member

@dkiser most (or all) of the logic is implemented inside of the decision log plugin like you found. I would start by figuring out the interface we want to expose to admins configuring OPA.

Currently admins can implement a rule/decision that defines a set of paths in the decision log event to remove. The paths must be prefixed with input/ or result/, i.e., they cannot remove other event fields. The generated paths are represented as slash-separated strings. For example:

package system.log

mask["/input/ssn"]  # mask out the ssn field in the input document

The decision path is configurable and it defaults to system/log/mask.

A few thoughts:

  • We need to maintain backwards compatibility. The default needs to remain system/log/mask. Moreover deployments configured with a masking policy must continue to work which means the existing decision structure must be supported. In terms of the interface, this means we could either (i) overload the 'mask' decision to support non-string values (see below) or (ii) introduce a new decision path that admins can opt into (e.g., system/log/transform). My preference would be for (i) to avoid maintaining two versions or having to deprecate and eventually remove support for the mask version.

  • Masking and other transforms add additional overhead to the decision path because the mask is applied to the event before the decision is returned to the caller. We want to reduce the latency impact as much as possible. Ideally we can gather all of the transforms in one shot, i.e., we don't want to execute multiple queries if possible.

I propose we extend the implementation to support structured values in the mask set. For example:

mask[{"op": "remove", "path": "/input/ssn"}]

mask[{"op": "upsert", "path": "/input/decision_signature", "value": x}] {
   # logic to compute signature value
}

This way the mask can generate a set of instructions for OPA. Each instruction specifies op (the operation type), path (the value to apply the transform to), and optionally value (some operations may require a value. The remove op is treated the same as the strings in the mask set today. The upsert op inserts or updates the path with the specified value (creating any parent nodes required in the event.)

Internally, I'd modify the implementation to map old mask strings into the structure above and then refactor the rest of the implementation accordingly.

cc @timothyhinrichs

@timothyhinrichs
Copy link
Member

Something like this has been on my wish list for a while, so thanks for picking it up!

I could see it being valuable to create new fields in the decision log outside of result and input. I'd suggest we require the user to put new fields under something like custom to ensure forwards compatibility for when we want to extend the decision-log format in the future.

@dkiser
Copy link
Contributor

dkiser commented May 18, 2020

@timothyhinrichs Maybe we can talk through this on our call today, and we'll take a stab at knocking this one out!

@dkiser dkiser mentioned this issue May 22, 2020
dkiser added a commit to dkiser/opa that referenced this issue Jun 10, 2020
…erase masking feature.

This feature adds the ability to mutate decision logs in addition to the default behavior
of erasing object paths.  A new upsert command was added to a structured way to define
mask rules in a backwards compatible manner.

Fixes: open-policy-agent#2379
Signed-off-by: Domingo Kiser <[email protected]>
tsandall pushed a commit that referenced this issue Jun 11, 2020
…erase masking feature.

This feature adds the ability to mutate decision logs in addition to the default behavior
of erasing object paths.  A new upsert command was added to a structured way to define
mask rules in a backwards compatible manner.

Fixes: #2379
Signed-off-by: Domingo Kiser <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants