-
Notifications
You must be signed in to change notification settings - Fork 257
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
Fix unintended data modification when redacted environment variables #1657
Conversation
When I did the change to redact environment variable values in policy engine error messages, I create a "false positive error message" bug. If any policy check were to be denied that involved environment variables in the check, then environment variables would be listed as a cause even if they weren't. This was because the previous redacting code was changing the data object used to determine the errors. The updated data object had the redacted environment variables which would never match so all error messages would include that the envs were invalid. This commit fixes that issue but creating a new object when redacting, the original object is still used to data checking and if redaction has been done, the new data object is used for generating the error message. The current test system makes this somewhat hard to test for. I will be adding tests to cover "false positive error messages" in the not so distant future. In the meantime, this commit addresses the bug before it makes to production. Signed-off-by: Sean T. Allen <seanallen@microsoft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also made the same mistake when writing the scrubbing portions...
@@ -318,6 +318,11 @@ func (policy *regoEnforcer) getReasonNotAllowed(enforcementPoint string, input i | |||
|
|||
func (policy *regoEnforcer) redactSensitiveData(input inputData) inputData { | |||
if v, k := input["envList"]; k { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated, should this be v, ok
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. shall i do another PR for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it be ok
or exists
or what? i cant remember the go convention.
I felt very "DUR" when I found this. |
…icrosoft#1657) When I did the change to redact environment variable values in policy engine error messages, I create a "false positive error message" bug. If any policy check were to be denied that involved environment variables in the check, then environment variables would be listed as a cause even if they weren't. This was because the previous redacting code was changing the data object used to determine the errors. The updated data object had the redacted environment variables which would never match so all error messages would include that the envs were invalid. This commit fixes that issue but creating a new object when redacting, the original object is still used to data checking and if redaction has been done, the new data object is used for generating the error message. The current test system makes this somewhat hard to test for. I will be adding tests to cover "false positive error messages" in the not so distant future. In the meantime, this commit addresses the bug before it makes to production. Signed-off-by: Sean T. Allen <seanallen@microsoft.com> (cherry picked from commit 3489fc4)
…ironment variables (microsoft#1657) Fix unintended data modification when redacted environment variables (microsoft#1657) When I did the change to redact environment variable values in policy engine error messages, I create a "false positive error message" bug. If any policy check were to be denied that involved environment variables in the check, then environment variables would be listed as a cause even if they weren't. This was because the previous redacting code was changing the data object used to determine the errors. The updated data object had the redacted environment variables which would never match so all error messages would include that the envs were invalid. This commit fixes that issue but creating a new object when redacting, the original object is still used to data checking and if redaction has been done, the new data object is used for generating the error message. The current test system makes this somewhat hard to test for. I will be adding tests to cover "false positive error messages" in the not so distant future. In the meantime, this commit addresses the bug before it makes to production. Signed-off-by: Sean T. Allen <seanallen@microsoft.com> (cherry picked from commit 3489fc4) Related work items: microsoft#1657
When I did the change to redact environment variable values in policy engine error messages, I create a "false positive error message" bug.
If any policy check were to be denied that involved environment variables in the check, then environment variables would be listed as a cause even if they weren't. This was because the previous redacting code was changing the data object used to determine the errors.
The updated data object had the redacted environment variables which would never match so all error messages would include that the envs were invalid.
This commit fixes that issue but creating a new object when redacting, the original object is still used to data checking and if redaction has been done, the new data object is used for generating the error message.
The current test system makes this somewhat hard to test for. I will be adding tests to cover "false positive error messages" in the not so distant future. In the meantime, this commit addresses the bug before it makes to production.
Signed-off-by: Sean T. Allen seanallen@microsoft.com