KNOX-3039 Add error message sanitization to GatewayServlet

[kardolus]

What changes were proposed in this pull request?

This pull request introduces a mechanism to sanitize error messages in the GatewayServlet to improve security by hiding IP addresses from exception messages. The following changes were made:

• Added a isErrorMessageSanitizationEnabled flag to the GatewayServlet to control whether error messages should be sanitized.
• Implemented the sanitizeException and sanitizeAndRethrow methods in GatewayServlet to handle exception sanitization.
• Updated the GatewayConfig interface and its implementation GatewayConfigImpl to include a new method isErrorMessageSanitizationEnabled.
• Created the GatewayServletTest class to parameterize tests for scenarios where sanitization is enabled and disabled.

How was this patch tested?

This patch was tested using the following methods:

• Parameterized unit tests were added to GatewayServletTest to cover both scenarios where error message sanitization is enabled and disabled.
• Manual review and inspection of the code changes to ensure accuracy and completeness.

Test steps:

1. Added unit tests in GatewayServletTest to check for sanitized and non-sanitized error messages.
2. Verified the new tests pass successfully, ensuring error messages are appropriately sanitized or left unchanged based on the configuration.

[moresandeep]

Looks good!

[moresandeep]

It would be nice to have a 4xx and 5xx page for errors. So instead of showing the ugly page with an exception we can show a custom page with a UUID corresponding the the error. This UUID would be the request-id for which the error is thrown so we can cross reference it in the logs. Just a though :) can be a new feature.

[kardolus]

@moresandeep I think that is a great idea! I will start a discussion on the Apache mailing list to explore implementing custom 4xx and 5xx error pages.

[kardolus]

@pzampino @moresandeep I implemented the SanitizedException. It makes the code a lot cleaner. I am not happy with the fact though that an IOException or a RuntimeException can be converted to a ServletException (a SanitizedException IS-A ServletException) through the sanitization flow.

But maybe I am overthinking things and overengineering. What do y'all think? Is it fine?

[pzampino]

Why not perform the message sanitization in the SanitizationException class?

[kardolus]

@pzampino There are some downsides in moving the logic over to the SanitizationException class. It makes the code less flexible and potentially harder to maintain (e.g. what if other classes want to throw a SanitizedException but with different logic?). It's also a bit of an effort to refactor the tests. We haven't decided yet that this pattern is better than keeping the original Exception type (see my previous comment). Finally, but less important IMO, moving the logic over to SanitizedException can be seen as a violation of the Single Responsibility Principle.

[pzampino]

I'm not sure I agree about the Single Responsibility Principle. What is the responsibility of the SanitizationException if not to provide a sanitized the message? Otherwise, the burden of message sanitization is on all entities that throw it, AND there is no guarantee that a thrower will have sanitized the message.

If there is a need to customize the sanitization, IMO that is what extensions are for. Nothing would prevent the subsequent creation of classes that extends SanitizationException to override the sanitizing logic.

Personally, I don't think level of effort for refactoring counts for much in these types of decisions, especially refactoring of test code.

[kardolus]

@pzampino Yeah, I am bummed that I didn't receive any feedback on the tests. This class had none until I wrote some. Refactoring could mean I need tests on both classes, and since we didn't discuss whether this new pattern is actually better than the old one, it seems like wasted work. Feel free to commit a patch if you can. I am going to be a bit busy with some other work over the next couple of weeks.

[kardolus]

@pzampino The Single Responsibility Principle would be violated because it would make the SanitizedException class responsible for both defining an exception and performing sanitization logic, thus giving it more than one reason to change.

[pzampino]

@pzampino @moresandeep I implemented the SanitizedException. It makes the code a lot cleaner. I am not happy with the fact though that an IOException or a RuntimeException can be converted to a ServletException (a SanitizedException IS-A ServletException) through the sanitization flow.

But maybe I am overthinking things and overengineering. What do y'all think? Is it fine?

Unless the servlet engine treats them differently in some meaningful way, I think it's ok, so long as the exception does not get lost entirely, which in this case, it does not.