[PR] KNOX-3039 - Add error message sanitization to GatewayServlet [knox]

["smolnar82 (via GitHub)" <gi...@apache.org>]

smolnar82 opened a new pull request, #1062:
URL: https://github.com/apache/knox/pull/1062

   :exclamation::exclamation::exclamation:IMPORTANT:exclamation::exclamation::exclamation:
   This PR is an almost identical copy of #914 (including this description), except:
   - sanitizer configuration init is extracted to a private method
   - a new factory method was created in a singleton class to ensure the new `SanitizedException` still complies with the Single Responsibility principal, but sanitization is centralized and reusable.
   - new test cases added to the sanitization logic
   - new exception constructor w/o message
   
   ## 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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] KNOX-3039 - Add error message sanitization to GatewayServlet [knox]

["smolnar82 (via GitHub)" <gi...@apache.org>]

smolnar82 closed pull request #1062: KNOX-3039 - Add error message sanitization to GatewayServlet
URL: https://github.com/apache/knox/pull/1062


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] KNOX-3039 - Add error message sanitization to GatewayServlet [knox]

["moresandeep (via GitHub)" <gi...@apache.org>]

moresandeep commented on code in PR #1062:
URL: https://github.com/apache/knox/pull/1062#discussion_r2219428603


##########
gateway-server/src/main/java/org/apache/knox/gateway/util/ExceptionSanitizer.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.util;
+
+import org.apache.knox.gateway.SanitizedException;
+
+import java.util.Objects;
+
+public class ExceptionSanitizer {
+
+    private ExceptionSanitizer() {
+    }
+
+    public static SanitizedException from(final Exception e, final boolean isSanitizationEnabled, final String pattern) {
+        if (Objects.isNull(e) || Objects.isNull(e.getMessage())) {
+            return new SanitizedException();

Review Comment:
   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.



##########
gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java:
##########
@@ -45,11 +45,17 @@
 import java.net.URISyntaxException;
 import java.nio.charset.StandardCharsets;
 import java.util.Enumeration;
+import java.util.Objects;
+
+import static org.apache.knox.gateway.util.ExceptionSanitizer.from;
 
 public class GatewayServlet implements Servlet, Filter {
   public static final String GATEWAY_DESCRIPTOR_LOCATION_DEFAULT = "gateway.xml";
   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";

Review Comment:
   Should we also add support for ipv6?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org