-
Notifications
You must be signed in to change notification settings - Fork 265
KNOX-3039 - Add error message sanitization to GatewayServlet #1062
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
base: master
Are you sure you want to change the base?
Conversation
hanicz
left a comment
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.
Why is there a need to sanitise log messages? I believe it would be more beneficial to use safer logging practices in the first place.
That aside I think using log4j appenders could be better to apply sanitisation globally. I don't like the fact that a class has to prepare for sanitisation which suggests that its aware of the problem and the security risk.
moresandeep
left a comment
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.
Thanks @smolnar82 for picking this up!
The PR looks good, I have a few questions/comments I marked up on the code.
Also, looks like this only deals with GatewayServlet, what about other places which might leak IP addresses?
| public static final String GATEWAY_DESCRIPTOR_LOCATION_PARAM = "gatewayDescriptorLocation"; | ||
|
|
||
| private static boolean isErrorMessageSanitizationEnabled = true; | ||
| private static String errorMessageSanitizationPattern = "\\b(?:\\d{1,3}\\.){3}\\d{1,3}\\b"; |
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 we also add support for ipv6?
|
|
||
| public static SanitizedException from(final Exception e, final boolean isSanitizationEnabled, final String pattern) { | ||
| if (Objects.isNull(e) || Objects.isNull(e.getMessage())) { | ||
| return new SanitizedException(); |
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.
In case there is no offending message (i.e. the message does not contain IP address) shouldn't we just throw the same exception instead of SanitizedException? looks like this might a) hide the original exception b) add new stack trace that won't be useful.
❗❗❗IMPORTANT❗❗❗
This PR is an almost identical copy of #914 (including this description), except:
SanitizedExceptionstill complies with the Single Responsibility principal, but sanitization is centralized and reusable.What changes were proposed in this pull request?
This pull request introduces a mechanism to sanitize error messages in the
GatewayServletto improve security by hiding IP addresses from exception messages. The following changes were made:isErrorMessageSanitizationEnabledflag to theGatewayServletto control whether error messages should be sanitized.sanitizeExceptionandsanitizeAndRethrowmethods inGatewayServletto handle exception sanitization.GatewayConfiginterface and its implementationGatewayConfigImplto include a new methodisErrorMessageSanitizationEnabled.GatewayServletTestclass to parameterize tests for scenarios where sanitization is enabled and disabled.How was this patch tested?
This patch was tested using the following methods:
GatewayServletTestto cover both scenarios where error message sanitization is enabled and disabled.Test steps:
GatewayServletTestto check for sanitized and non-sanitized error messages.