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

OPA decision masks do not apply correctly to paths containing a character that would be URL encoded #4717

Closed
pauly4it opened this issue May 27, 2022 · 0 comments · Fixed by #4756
Assignees
Labels

Comments

@pauly4it
Copy link
Contributor

Short description

While building a decision mask specifically for Envoy input data which uses a JWT access_token URL query param, I noticed that the query param and value show up in 3 places: input.parsed_query, input.attributes.request.http.path, and input.attributes.request.http.headers[":path"].

I looked into how OPA would handle masking the input.attributes.request.http.headers[":path"] param, and it appears that it won't correctly apply the mask because the path parts are escaped by OPA (reference: https://github.com/open-policy-agent/opa/blob/v0.40.0/plugins/logs/mask.go#L78). As a result, if I pass in a masking path of "/input/attributes/request/http/headers/:path", OPA attempts to mask "/input/attributes/request/http/headers/%3Apath".

Here's a rego playground link with example input and a simple upsert mask: https://play.openpolicyagent.org/p/IKGpsHcXAB. The output for this playground example, however, does not represent the actual masking operation. To do that, I added two new test cases to mask_test.go locally on my machine and got a failure for each (details in Steps To Reproduce section).

Steps To Reproduce

  1. Using OPA v0.40.0, add the following test case to TestMaskRuleMask in mask_test.go at line 543 (https://github.com/open-policy-agent/opa/blob/v0.40.0/plugins/logs/mask_test.go#L543):
{
	note: "erase: :path",
	ptr: &maskRule{
		OP:   maskOPRemove,
		Path: "/input/:path",
	},
	event: `{"input": {"bar": 1, ":path": "token"}}`,
	exp:   `{"input": {"bar": 1}, "erased": ["/input/:path"]}`,
},
{
	note: "upsert in :path",
	ptr: &maskRule{
		OP:    maskOPUpsert,
		Path:  "/input/:path",
		Value: "upserted",
	},
	event: `{"input": {"bar": 1, ":path": "token"}}`,
	exp:   `{"input": {"bar": 1, ":path": "upserted"}, "masked": ["/input/:path"]}`,
},
  1. Run make test

  2. Receive a github.com/open-policy-agent/opa/plugins/logs test failure:

--- FAIL: TestMaskRuleMask (0.00s)
    --- FAIL: TestMaskRuleMask/erase:_:path (0.00s)
        mask_test.go:602: Expected: {
              "labels": null,
              "decision_id": "",
              "input": {
                "bar": 1
              },
              "erased": [
                "/input/:path"
              ],
              "timestamp": "0001-01-01T00:00:00Z"
            }
            Got: {
              "labels": null,
              "decision_id": "",
              "input": {
                ":path": "token",
                "bar": 1
              },
              "timestamp": "0001-01-01T00:00:00Z"
            }
    --- FAIL: TestMaskRuleMask/upsert_in_:path (0.00s)
        mask_test.go:602: Expected: {
              "labels": null,
              "decision_id": "",
              "input": {
                ":path": "upserted",
                "bar": 1
              },
              "masked": [
                "/input/:path"
              ],
              "timestamp": "0001-01-01T00:00:00Z"
            }
            Got: {
              "labels": null,
              "decision_id": "",
              "input": {
                "%3Apath": "upserted",
                ":path": "token",
                "bar": 1
              },
              "masked": [
                "/input/%3Apath"
              ],
              "timestamp": "0001-01-01T00:00:00Z"
            }

Expected behavior

OPA should be able to properly handle mask paths which contain a character that would be URL encoded (see the example test case above).

Additional context

This issue is not confined just to Envoy input data, as other systems provide inputs with headers[":path"] along with :authority and :method. At the moment, users cannot use OPA decision masking to mask just those parts of the headers input object and would need to mask the entire headers object, which may remove useful data from the decision log.

@pauly4it pauly4it added the bug label May 27, 2022
@ashutosh-narkar ashutosh-narkar self-assigned this Jun 1, 2022
@ashutosh-narkar ashutosh-narkar added this to Backlog in Open Policy Agent via automation Jun 8, 2022
@ashutosh-narkar ashutosh-narkar moved this from Backlog to Planned - v0.42 in Open Policy Agent Jun 8, 2022
@ashutosh-narkar ashutosh-narkar moved this from Planned - v0.42 to In Progress in Open Policy Agent Jun 8, 2022
ashutosh-narkar added a commit to ashutosh-narkar/opa that referenced this issue Jun 8, 2022
Earlier the paths to the field to perform an upsert or
remove operation on were escaped using Go's url.QueryEscape
method. This results in incorrect behavior when the paths contain
a reserved character like ":". This change updates to using
url.PathEscape instead to escape the input and result paths.

Fixes: open-policy-agent#4717

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
Open Policy Agent automation moved this from In Progress to Done Jun 9, 2022
srenatus pushed a commit that referenced this issue Jun 9, 2022
Earlier the paths to the field to perform an upsert or
remove operation on were escaped using Go's url.QueryEscape
method. This results in incorrect behavior when the paths contain
a reserved character like ":". This change updates to using
url.PathEscape instead to escape the input and result paths.

Fixes: #4717

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging a pull request may close this issue.

2 participants