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

Implement Log Masking #1732

Merged
merged 24 commits into from
May 12, 2021
Merged

Implement Log Masking #1732

merged 24 commits into from
May 12, 2021

Conversation

4e6
Copy link
Contributor

@4e6 4e6 commented May 7, 2021

Pull Request Description

close #1720

Changelog:

  • feat: add ToMaskedString interface that indicates that the object has a custom string representation masking some PII.
  • feat: add --no-log-masking CLI flag that disables log masking

Important Notes

What is masked:

  • Literals containing code
  • Values of executed expressions
  • Values of the user environment variables
  • Paths inside the user project

What is not masked:

  • Names of functions and modules
  • Keys (names) of the user environment variables
  • System and distribution paths, including a path to the user project

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the Scala, Java, and Rust style guides.
  • All documentation and configuration conforms to the markdown and YAML style guides.
  • All code has been tested where possible.

@4e6 4e6 added Type: Enhancement p-high Should be completed in the next sprint labels May 7, 2021
@4e6 4e6 self-assigned this May 7, 2021
@4e6 4e6 marked this pull request as ready for review May 12, 2021 05:46
Copy link
Contributor

@iamrecursion iamrecursion left a comment

Choose a reason for hiding this comment

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

Please document this whole thing in the docs. It needs to be somewhere that users can find.

Additionally, once this is merged please get a variety of people to test it and send the logs. We want to check that we've not missed masking anything important.

I also think that any component that is running with log masking disabled should warn in the console.

RELEASES.md Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
ex,
"Failure during [{}] operation: {}",
CreateFile,
ex.getMessage
Copy link
Contributor

Choose a reason for hiding this comment

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

Could the exception message contain sensitive data? The same question for the other exception messages we log.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, IO errors can potentially leak paths inside the user project. I'll mask them as well

Copy link
Contributor

@iamrecursion iamrecursion left a comment

Choose a reason for hiding this comment

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

Approved, conditional on moving the logging docs into logging.md.

@iamrecursion iamrecursion merged commit 1b63887 into main May 12, 2021
@iamrecursion iamrecursion deleted the wip/db/log-masking branch May 12, 2021 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p-high Should be completed in the next sprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mask Sensitive Data in Logs
2 participants