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

Support separate logger for sensitive information (take 2) #168

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mkienenb
Copy link
Contributor

@mkienenb mkienenb commented Jun 5, 2020

This PR takes the flexibility of PR #88 and merges it with the configurable masking provided by PR #139 without creating a hard dependency on log4j layout configuration.

This is done by replacing the SensitiveFilterLayout with a wrapped Logger class SensitiveFilterLogWrapper. If the sensitive logger is not enabled, then we log to the filtered masked logger.

This PR doesn't change every logger reference to be filtered, but only the two most problematic classes of HttpCallTask and HttpUtility. However, it would be a good idea to expend this to all loggers.

There's also a tradeoff between having unfiltered loggers defined for each class or having a single centralized logger. For end-users, having a single logger makes the most sense as only one logging configuration line has to be used and end-users don't care where internally a message is being logged. But sdk-java developers might prefer to have logging messages per class.

One possible solution to this would be to create individual loggers for each class in a single package, so only the package has to be configured. Another would be to pick logger names manually instead of using classes, such as "net.authorize.sensitive.util.HttpCallTask" so that a single configuration line could still disable all sensitive logging, yet individual logging is also configurable and the name of the class doing the logging still appears in the logger name.

Switch from unfiltered logging of sensitive output to masked output in this implementation by using

log4j.logger.net.authorize.util.SensitiveLogger=OFF

@mkienenb
Copy link
Contributor Author

mkienenb commented Jun 5, 2020

Rational why this is needed (From PR #88) instead of the current Sensitive Logger:

Unfortunately, while clever, the SensitiveFilterLayout approach to fixing this issue is flawed.

It requires the log4j be used -- not the case for some of our apps.

It requires that the SensitiveFilterLayout be used, which is also not the case for some of our logging.

There's also the chance, although slim, that the masked credit card regexs might mask some other data that just looks like a credit card.

I think a better solution using this approach would be to apply the same logic to the logger directly. Then there'd be no other logging affected by the filtering, and no special layout would be needed. The downside might be toggling between showing and not showing the sensitive data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant