From b029eff60e4a863479f124c6104f910ba8fb188e Mon Sep 17 00:00:00 2001 From: kardolus Date: Mon, 3 Jun 2024 09:49:31 -0400 Subject: [PATCH 1/7] KNOX-3039 Add error message sanitization to GatewayServlet --- .../apache/knox/gateway/GatewayServlet.java | 53 ++++++-- .../config/impl/GatewayConfigImpl.java | 7 + .../knox/gateway/GatewayServletTest.java | 122 ++++++++++++++++++ .../knox/gateway/GatewayTestConfig.java | 5 + .../knox/gateway/config/GatewayConfig.java | 5 + 5 files changed, 181 insertions(+), 11 deletions(-) create mode 100644 gateway-server/src/test/java/org/apache/knox/gateway/GatewayServletTest.java diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java b/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java index a1109d850c..f54a76dcd0 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java @@ -42,6 +42,7 @@ import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; +import java.lang.reflect.Constructor; import java.net.URISyntaxException; import java.nio.charset.StandardCharsets; import java.util.Enumeration; @@ -50,6 +51,8 @@ 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 final GatewayResources res = ResourcesFactory.get( GatewayResources.class ); private static final GatewayMessages LOG = MessagesFactory.get( GatewayMessages.class ); @@ -83,6 +86,8 @@ public synchronized void setFilter( GatewayFilter filter ) throws ServletExcepti @Override public synchronized void init( ServletConfig servletConfig ) throws ServletException { + GatewayConfig gatewayConfig = (GatewayConfig) servletConfig.getServletContext().getAttribute(GatewayConfig.GATEWAY_CONFIG_ATTRIBUTE); + isErrorMessageSanitizationEnabled = gatewayConfig.isErrorMessageSanitizationEnabled(); try { if( filter == null ) { filter = createFilter( servletConfig ); @@ -92,8 +97,7 @@ public synchronized void init( ServletConfig servletConfig ) throws ServletExcep filter.init( filterConfig ); } } catch( ServletException | RuntimeException e ) { - LOG.failedToInitializeServletInstace( e ); - throw e; + throw sanitizeAndRethrow(e); } } @@ -101,14 +105,15 @@ public synchronized void init( ServletConfig servletConfig ) throws ServletExcep public void init( FilterConfig filterConfig ) throws ServletException { try { if( filter == null ) { + GatewayConfig gatewayConfig = (GatewayConfig) filterConfig.getServletContext().getAttribute(GatewayConfig.GATEWAY_CONFIG_ATTRIBUTE); + isErrorMessageSanitizationEnabled = gatewayConfig.isErrorMessageSanitizationEnabled(); filter = createFilter( filterConfig ); } if( filter != null ) { filter.init( filterConfig ); } } catch( ServletException | RuntimeException e ) { - LOG.failedToInitializeServletInstace( e ); - throw e; + throw sanitizeAndRethrow(e); } } @@ -126,8 +131,7 @@ public void service( ServletRequest servletRequest, ServletResponse servletRespo try { f.doFilter( servletRequest, servletResponse, null ); } catch( IOException | RuntimeException | ServletException e ) { - LOG.failedToExecuteFilter( e ); - throw e; + throw sanitizeAndRethrow(e); } } else { ((HttpServletResponse)servletResponse).setStatus( HttpServletResponse.SC_SERVICE_UNAVAILABLE ); @@ -153,10 +157,8 @@ public void doFilter( ServletRequest servletRequest, ServletResponse servletResp //TODO: This should really happen naturally somehow as part of being a filter. This way will cause problems eventually. chain.doFilter( servletRequest, servletResponse ); } - - } catch( IOException | RuntimeException | ServletException e ) { - LOG.failedToExecuteFilter( e ); - throw e; + } catch (Exception e) { + throw sanitizeAndRethrow(e); } } else { ((HttpServletResponse)servletResponse).setStatus( HttpServletResponse.SC_SERVICE_UNAVAILABLE ); @@ -168,7 +170,6 @@ public void doFilter( ServletRequest servletRequest, ServletResponse servletResp } } - @Override public String getServletInfo() { return res.gatewayServletInfo(); @@ -277,4 +278,34 @@ public Enumeration getInitParameterNames() { return config.getInitParameterNames(); } } + + private Exception sanitizeException(Exception e) { + if (e == null || e.getMessage() == null) { + return e; + } + if (!isErrorMessageSanitizationEnabled || e.getMessage() == null) { + return e; + } + String sanitizedMessage = e.getMessage().replaceAll("\\b(?:\\d{1,3}\\.){3}\\d{1,3}\\b", "[hidden]"); + return createNewException(e, sanitizedMessage); + } + + private T createNewException(T e, String sanitizedMessage) { + try { + Constructor constructor = e.getClass().getConstructor(String.class, Throwable.class); + T sanitizedException = (T) constructor.newInstance(sanitizedMessage, e.getCause()); + sanitizedException.setStackTrace(e.getStackTrace()); + return sanitizedException; + } catch (Exception ex) { + Exception genericException = new Exception(sanitizedMessage, e.getCause()); + genericException.setStackTrace(e.getStackTrace()); + return (T) genericException; + } + } + + private T sanitizeAndRethrow(Exception e) throws T { + Exception sanitizedException = sanitizeException(e); + LOG.failedToExecuteFilter(sanitizedException); + throw (T) sanitizedException; + } } diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java b/gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java index 1b6e967a7f..b08965d325 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java @@ -176,6 +176,8 @@ public class GatewayConfigImpl extends Configuration implements GatewayConfig { public static final String CLUSTER_CONFIG_MONITOR_INTERVAL_SUFFIX = ".interval"; public static final String CLUSTER_CONFIG_MONITOR_ENABLED_SUFFIX = ".enabled"; + private static final String ERROR_MESSAGE_SANITIZATION_ENABLED = GATEWAY_CONFIG_FILE_PREFIX + ".error.sanitization.enabled"; + private static final boolean DEFAULT_ERROR_MESSAGE_SANITIZATION_ENABLED = true; // These config property names are not inline with the convention of using the // GATEWAY_CONFIG_FILE_PREFIX as is done by those above. These are left for @@ -933,6 +935,11 @@ public List getGlobalRulesServices() { return DEFAULT_GLOBAL_RULES_SERVICES; } + @Override + public boolean isErrorMessageSanitizationEnabled() { + return getBoolean(ERROR_MESSAGE_SANITIZATION_ENABLED, DEFAULT_ERROR_MESSAGE_SANITIZATION_ENABLED); + } + @Override public boolean isMetricsEnabled() { return Boolean.parseBoolean(get( METRICS_ENABLED, "false" )); diff --git a/gateway-server/src/test/java/org/apache/knox/gateway/GatewayServletTest.java b/gateway-server/src/test/java/org/apache/knox/gateway/GatewayServletTest.java new file mode 100644 index 0000000000..a3cb93cd1d --- /dev/null +++ b/gateway-server/src/test/java/org/apache/knox/gateway/GatewayServletTest.java @@ -0,0 +1,122 @@ +/* + * 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; + +import org.apache.knox.gateway.config.GatewayConfig; +import org.easymock.EasyMock; +import org.easymock.IMocksControl; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +import javax.servlet.FilterConfig; +import javax.servlet.ServletConfig; +import javax.servlet.ServletContext; +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import java.io.IOException; +import java.util.Arrays; +import java.util.Collection; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; + +@RunWith(Parameterized.class) +public class GatewayServletTest { + + @Parameterized.Parameters(name = "{index}: SanitizationEnabled={2}, Exception={0}, ExpectedMessage={1}") + public static Collection data() { + return Arrays.asList(new Object[][] { + { new IOException("Connection to 192.168.1.1 failed"), "Connection to [hidden] failed", true }, + { new RuntimeException("Connection to 192.168.1.1 failed"), "Connection to [hidden] failed", true }, + { new NullPointerException(), null, true }, + { new IOException("General failure"), "General failure", true }, + { new IOException("Connection to 192.168.1.1 failed"), "Connection to 192.168.1.1 failed", false }, + { new RuntimeException("Connection to 192.168.1.1 failed"), "Connection to 192.168.1.1 failed", false }, + { new NullPointerException(), null, false }, + { new IOException("General failure"), "General failure", false } + }); + } + + private final Exception exception; + private final String expectedMessage; + private final boolean isSanitizationEnabled; + + public GatewayServletTest(Exception exception, String expectedMessage, boolean isSanitizationEnabled) { + this.exception = exception; + this.expectedMessage = expectedMessage; + this.isSanitizationEnabled = isSanitizationEnabled; + } + + private IMocksControl mockControl; + private ServletConfig servletConfig; + private ServletContext servletContext; + private GatewayConfig gatewayConfig; + private HttpServletRequest request; + private HttpServletResponse response; + private GatewayFilter filter; + + @Before + public void setUp() throws ServletException { + mockControl = EasyMock.createControl(); + servletConfig = mockControl.createMock(ServletConfig.class); + servletContext = mockControl.createMock(ServletContext.class); + gatewayConfig = mockControl.createMock(GatewayConfig.class); + request = mockControl.createMock(HttpServletRequest.class); + response = mockControl.createMock(HttpServletResponse.class); + filter = mockControl.createMock(GatewayFilter.class); + + EasyMock.expect(servletConfig.getServletName()).andStubReturn("default"); + EasyMock.expect(servletConfig.getServletContext()).andStubReturn(servletContext); + EasyMock.expect(servletContext.getAttribute("org.apache.knox.gateway.config")).andStubReturn(gatewayConfig); + } + + @Test + public void testExceptionSanitization() throws ServletException, IOException { + GatewayServlet servlet = initializeServletWithSanitization(isSanitizationEnabled); + + try { + servlet.service(request, response); + } catch (Exception e) { + if (expectedMessage != null) { + assertEquals(expectedMessage, e.getMessage()); + } else { + assertNull(e.getMessage()); + } + } + + mockControl.verify(); + } + + private GatewayServlet initializeServletWithSanitization(boolean isErrorMessageSanitizationEnabled) throws ServletException, IOException { + EasyMock.expect(gatewayConfig.isErrorMessageSanitizationEnabled()).andStubReturn(isErrorMessageSanitizationEnabled); + + filter.init(EasyMock.anyObject(FilterConfig.class)); + EasyMock.expectLastCall().once(); + filter.doFilter(EasyMock.eq(request), EasyMock.eq(response), EasyMock.isNull()); + EasyMock.expectLastCall().andThrow(exception).once(); + + mockControl.replay(); + + GatewayServlet servlet = new GatewayServlet(filter); + servlet.init(servletConfig); + return servlet; + } +} diff --git a/gateway-spi-common/src/main/java/org/apache/knox/gateway/GatewayTestConfig.java b/gateway-spi-common/src/main/java/org/apache/knox/gateway/GatewayTestConfig.java index e3f2ddab51..ec46561b90 100644 --- a/gateway-spi-common/src/main/java/org/apache/knox/gateway/GatewayTestConfig.java +++ b/gateway-spi-common/src/main/java/org/apache/knox/gateway/GatewayTestConfig.java @@ -619,6 +619,11 @@ public int getWebsocketMaxWaitBufferCount() { return DEFAULT_WEBSOCKET_MAX_WAIT_BUFFER_COUNT; } + @Override + public boolean isErrorMessageSanitizationEnabled() { + return true; + } + @Override public boolean isMetricsEnabled() { return false; diff --git a/gateway-spi/src/main/java/org/apache/knox/gateway/config/GatewayConfig.java b/gateway-spi/src/main/java/org/apache/knox/gateway/config/GatewayConfig.java index 352aabbe5b..cdc1f8da56 100644 --- a/gateway-spi/src/main/java/org/apache/knox/gateway/config/GatewayConfig.java +++ b/gateway-spi/src/main/java/org/apache/knox/gateway/config/GatewayConfig.java @@ -901,6 +901,11 @@ public interface GatewayConfig { */ boolean isAsyncSupported(); + /** + * @return true if error message sanitization is enabled; false otherwise (defaults to true) + */ + boolean isErrorMessageSanitizationEnabled(); + /** * @return true if the supplied user is allowed to see all tokens * (i.e. not only tokens where userName or createdBy equals to the From 0f5f6cd832d3aaa760be6314fcd054304da2aa59 Mon Sep 17 00:00:00 2001 From: kardolus Date: Tue, 4 Jun 2024 12:00:25 -0400 Subject: [PATCH 2/7] Touch up --- .../src/main/java/org/apache/knox/gateway/GatewayServlet.java | 4 ++-- .../apache/knox/gateway/config/impl/GatewayConfigImpl.java | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java b/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java index f54a76dcd0..064d983a51 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java @@ -287,10 +287,10 @@ private Exception sanitizeException(Exception e) { return e; } String sanitizedMessage = e.getMessage().replaceAll("\\b(?:\\d{1,3}\\.){3}\\d{1,3}\\b", "[hidden]"); - return createNewException(e, sanitizedMessage); + return createSanitizedException(e, sanitizedMessage); } - private T createNewException(T e, String sanitizedMessage) { + private T createSanitizedException(T e, String sanitizedMessage) { try { Constructor constructor = e.getClass().getConstructor(String.class, Throwable.class); T sanitizedException = (T) constructor.newInstance(sanitizedMessage, e.getCause()); diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java b/gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java index b08965d325..dd01870206 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java @@ -177,7 +177,7 @@ public class GatewayConfigImpl extends Configuration implements GatewayConfig { public static final String CLUSTER_CONFIG_MONITOR_ENABLED_SUFFIX = ".enabled"; private static final String ERROR_MESSAGE_SANITIZATION_ENABLED = GATEWAY_CONFIG_FILE_PREFIX + ".error.sanitization.enabled"; - private static final boolean DEFAULT_ERROR_MESSAGE_SANITIZATION_ENABLED = true; + private static final boolean ERROR_MESSAGE_SANITIZATION_ENABLED_DEFAULT = true; // These config property names are not inline with the convention of using the // GATEWAY_CONFIG_FILE_PREFIX as is done by those above. These are left for @@ -937,7 +937,7 @@ public List getGlobalRulesServices() { @Override public boolean isErrorMessageSanitizationEnabled() { - return getBoolean(ERROR_MESSAGE_SANITIZATION_ENABLED, DEFAULT_ERROR_MESSAGE_SANITIZATION_ENABLED); + return getBoolean(ERROR_MESSAGE_SANITIZATION_ENABLED, ERROR_MESSAGE_SANITIZATION_ENABLED_DEFAULT); } @Override From 13cca6e61be76e9fba8dd00c6478e58345ec3360 Mon Sep 17 00:00:00 2001 From: kardolus Date: Tue, 4 Jun 2024 12:31:25 -0400 Subject: [PATCH 3/7] Log original error message --- .../main/java/org/apache/knox/gateway/GatewayServlet.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java b/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java index 064d983a51..fbe4808628 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java @@ -304,8 +304,7 @@ private T createSanitizedException(T e, String sanitizedMe } private T sanitizeAndRethrow(Exception e) throws T { - Exception sanitizedException = sanitizeException(e); - LOG.failedToExecuteFilter(sanitizedException); - throw (T) sanitizedException; + LOG.failedToExecuteFilter(e); + throw (T) sanitizeException(e); } } From 4d4687ae7e8aab1192c34d7b5891f1ef278f7928 Mon Sep 17 00:00:00 2001 From: kardolus Date: Wed, 5 Jun 2024 09:25:00 -0400 Subject: [PATCH 4/7] Return instead of throw --- .../java/org/apache/knox/gateway/GatewayServlet.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java b/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java index fbe4808628..cbf7cff39b 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java @@ -97,7 +97,7 @@ public synchronized void init( ServletConfig servletConfig ) throws ServletExcep filter.init( filterConfig ); } } catch( ServletException | RuntimeException e ) { - throw sanitizeAndRethrow(e); + throw sanitizeAndLogException(e); } } @@ -113,7 +113,7 @@ public void init( FilterConfig filterConfig ) throws ServletException { filter.init( filterConfig ); } } catch( ServletException | RuntimeException e ) { - throw sanitizeAndRethrow(e); + throw sanitizeAndLogException(e); } } @@ -131,7 +131,7 @@ public void service( ServletRequest servletRequest, ServletResponse servletRespo try { f.doFilter( servletRequest, servletResponse, null ); } catch( IOException | RuntimeException | ServletException e ) { - throw sanitizeAndRethrow(e); + throw sanitizeAndLogException(e); } } else { ((HttpServletResponse)servletResponse).setStatus( HttpServletResponse.SC_SERVICE_UNAVAILABLE ); @@ -158,7 +158,7 @@ public void doFilter( ServletRequest servletRequest, ServletResponse servletResp chain.doFilter( servletRequest, servletResponse ); } } catch (Exception e) { - throw sanitizeAndRethrow(e); + throw sanitizeAndLogException(e); } } else { ((HttpServletResponse)servletResponse).setStatus( HttpServletResponse.SC_SERVICE_UNAVAILABLE ); @@ -303,8 +303,8 @@ private T createSanitizedException(T e, String sanitizedMe } } - private T sanitizeAndRethrow(Exception e) throws T { + private T sanitizeAndLogException(Exception e) throws T { LOG.failedToExecuteFilter(e); - throw (T) sanitizeException(e); + return (T) sanitizeException(e); } } From e11912a281eb33f497329eaf397a4419216ed751 Mon Sep 17 00:00:00 2001 From: kardolus Date: Thu, 6 Jun 2024 09:49:26 -0400 Subject: [PATCH 5/7] Make the pattern configurable --- .../apache/knox/gateway/GatewayServlet.java | 18 ++++++---- .../config/impl/GatewayConfigImpl.java | 7 ++++ .../knox/gateway/GatewayServletTest.java | 33 ++++++++++++------- .../knox/gateway/GatewayTestConfig.java | 6 ++++ .../knox/gateway/config/GatewayConfig.java | 5 +++ 5 files changed, 50 insertions(+), 19 deletions(-) diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java b/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java index cbf7cff39b..b8b8cd2f17 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java @@ -52,6 +52,7 @@ public class GatewayServlet implements Servlet, Filter { 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"; private static final GatewayResources res = ResourcesFactory.get( GatewayResources.class ); private static final GatewayMessages LOG = MessagesFactory.get( GatewayMessages.class ); @@ -88,6 +89,7 @@ public synchronized void setFilter( GatewayFilter filter ) throws ServletExcepti public synchronized void init( ServletConfig servletConfig ) throws ServletException { GatewayConfig gatewayConfig = (GatewayConfig) servletConfig.getServletContext().getAttribute(GatewayConfig.GATEWAY_CONFIG_ATTRIBUTE); isErrorMessageSanitizationEnabled = gatewayConfig.isErrorMessageSanitizationEnabled(); + errorMessageSanitizationPattern = gatewayConfig.getErrorMessageSanitizationPattern(); try { if( filter == null ) { filter = createFilter( servletConfig ); @@ -97,7 +99,7 @@ public synchronized void init( ServletConfig servletConfig ) throws ServletExcep filter.init( filterConfig ); } } catch( ServletException | RuntimeException e ) { - throw sanitizeAndLogException(e); + throw logAndSanitizeException(e); } } @@ -107,13 +109,14 @@ public void init( FilterConfig filterConfig ) throws ServletException { if( filter == null ) { GatewayConfig gatewayConfig = (GatewayConfig) filterConfig.getServletContext().getAttribute(GatewayConfig.GATEWAY_CONFIG_ATTRIBUTE); isErrorMessageSanitizationEnabled = gatewayConfig.isErrorMessageSanitizationEnabled(); + errorMessageSanitizationPattern = gatewayConfig.getErrorMessageSanitizationPattern(); filter = createFilter( filterConfig ); } if( filter != null ) { filter.init( filterConfig ); } } catch( ServletException | RuntimeException e ) { - throw sanitizeAndLogException(e); + throw logAndSanitizeException(e); } } @@ -131,7 +134,7 @@ public void service( ServletRequest servletRequest, ServletResponse servletRespo try { f.doFilter( servletRequest, servletResponse, null ); } catch( IOException | RuntimeException | ServletException e ) { - throw sanitizeAndLogException(e); + throw logAndSanitizeException(e); } } else { ((HttpServletResponse)servletResponse).setStatus( HttpServletResponse.SC_SERVICE_UNAVAILABLE ); @@ -158,7 +161,7 @@ public void doFilter( ServletRequest servletRequest, ServletResponse servletResp chain.doFilter( servletRequest, servletResponse ); } } catch (Exception e) { - throw sanitizeAndLogException(e); + throw logAndSanitizeException(e); } } else { ((HttpServletResponse)servletResponse).setStatus( HttpServletResponse.SC_SERVICE_UNAVAILABLE ); @@ -286,7 +289,8 @@ private Exception sanitizeException(Exception e) { if (!isErrorMessageSanitizationEnabled || e.getMessage() == null) { return e; } - String sanitizedMessage = e.getMessage().replaceAll("\\b(?:\\d{1,3}\\.){3}\\d{1,3}\\b", "[hidden]"); + String sanitizedMessage = e.getMessage().replaceAll(errorMessageSanitizationPattern, "[hidden]"); + return createSanitizedException(e, sanitizedMessage); } @@ -303,8 +307,8 @@ private T createSanitizedException(T e, String sanitizedMe } } - private T sanitizeAndLogException(Exception e) throws T { + private T logAndSanitizeException(Exception e) throws T { LOG.failedToExecuteFilter(e); - return (T) sanitizeException(e); + throw (T) sanitizeException(e); } } diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java b/gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java index dd01870206..b7544befd9 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java @@ -178,6 +178,8 @@ public class GatewayConfigImpl extends Configuration implements GatewayConfig { private static final String ERROR_MESSAGE_SANITIZATION_ENABLED = GATEWAY_CONFIG_FILE_PREFIX + ".error.sanitization.enabled"; private static final boolean ERROR_MESSAGE_SANITIZATION_ENABLED_DEFAULT = true; + private static final String ERROR_MESSAGE_SANITIZATION_PATTERN = GATEWAY_CONFIG_FILE_PREFIX + ".error.sanitization.pattern"; + private static final String ERROR_MESSAGE_SANITIZATION_PATTERN_DEFAULT = "\\b(?:\\d{1,3}\\.){3}\\d{1,3}\\b"; // These config property names are not inline with the convention of using the // GATEWAY_CONFIG_FILE_PREFIX as is done by those above. These are left for @@ -940,6 +942,11 @@ public boolean isErrorMessageSanitizationEnabled() { return getBoolean(ERROR_MESSAGE_SANITIZATION_ENABLED, ERROR_MESSAGE_SANITIZATION_ENABLED_DEFAULT); } + @Override + public String getErrorMessageSanitizationPattern() { + return get(ERROR_MESSAGE_SANITIZATION_PATTERN, ERROR_MESSAGE_SANITIZATION_PATTERN_DEFAULT); + } + @Override public boolean isMetricsEnabled() { return Boolean.parseBoolean(get( METRICS_ENABLED, "false" )); diff --git a/gateway-server/src/test/java/org/apache/knox/gateway/GatewayServletTest.java b/gateway-server/src/test/java/org/apache/knox/gateway/GatewayServletTest.java index a3cb93cd1d..8edb035e2a 100644 --- a/gateway-server/src/test/java/org/apache/knox/gateway/GatewayServletTest.java +++ b/gateway-server/src/test/java/org/apache/knox/gateway/GatewayServletTest.java @@ -41,28 +41,36 @@ @RunWith(Parameterized.class) public class GatewayServletTest { - @Parameterized.Parameters(name = "{index}: SanitizationEnabled={2}, Exception={0}, ExpectedMessage={1}") + private static final String IP_ADDRESS_PATTERN = "\\b(?:\\d{1,3}\\.){3}\\d{1,3}\\b"; + private static final String EMAIL_ADDRESS_PATTERN = "\\b[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\\.[A-Z|a-z]{2,}\\b"; + private static final String CREDIT_CARD_PATTERN = "\\b\\d{4}-\\d{4}-\\d{4}-\\d{4}\\b"; + + @Parameterized.Parameters(name = "{index}: SanitizationEnabled={2}, Pattern={3}, Exception={0}, ExpectedMessage={1}") public static Collection data() { return Arrays.asList(new Object[][] { - { new IOException("Connection to 192.168.1.1 failed"), "Connection to [hidden] failed", true }, - { new RuntimeException("Connection to 192.168.1.1 failed"), "Connection to [hidden] failed", true }, - { new NullPointerException(), null, true }, - { new IOException("General failure"), "General failure", true }, - { new IOException("Connection to 192.168.1.1 failed"), "Connection to 192.168.1.1 failed", false }, - { new RuntimeException("Connection to 192.168.1.1 failed"), "Connection to 192.168.1.1 failed", false }, - { new NullPointerException(), null, false }, - { new IOException("General failure"), "General failure", false } + { new IOException("Connection to 192.168.1.1 failed"), "Connection to [hidden] failed", true, IP_ADDRESS_PATTERN }, + { new RuntimeException("Connection to 192.168.1.1 failed"), "Connection to [hidden] failed", true, IP_ADDRESS_PATTERN }, + { new NullPointerException(), null, true, IP_ADDRESS_PATTERN }, + { new IOException("General failure"), "General failure", true, IP_ADDRESS_PATTERN }, + { new IOException("Connection to 192.168.1.1 failed"), "Connection to 192.168.1.1 failed", false, IP_ADDRESS_PATTERN }, + { new RuntimeException("Connection to 192.168.1.1 failed"), "Connection to 192.168.1.1 failed", false, IP_ADDRESS_PATTERN }, + { new NullPointerException(), null, false, IP_ADDRESS_PATTERN }, + { new IOException("General failure"), "General failure", false, IP_ADDRESS_PATTERN }, + { new IOException("User email: user@example.com"), "User email: [hidden]", true, EMAIL_ADDRESS_PATTERN }, + { new RuntimeException("Credit card number: 1234-5678-9101-1121"), "Credit card number: [hidden]", true, CREDIT_CARD_PATTERN }, }); } private final Exception exception; private final String expectedMessage; private final boolean isSanitizationEnabled; + private final String sanitizationPattern; - public GatewayServletTest(Exception exception, String expectedMessage, boolean isSanitizationEnabled) { + public GatewayServletTest(Exception exception, String expectedMessage, boolean isSanitizationEnabled, String sanitizationPattern) { this.exception = exception; this.expectedMessage = expectedMessage; this.isSanitizationEnabled = isSanitizationEnabled; + this.sanitizationPattern = sanitizationPattern; } private IMocksControl mockControl; @@ -90,7 +98,7 @@ public void setUp() throws ServletException { @Test public void testExceptionSanitization() throws ServletException, IOException { - GatewayServlet servlet = initializeServletWithSanitization(isSanitizationEnabled); + GatewayServlet servlet = initializeServletWithSanitization(isSanitizationEnabled, sanitizationPattern); try { servlet.service(request, response); @@ -105,8 +113,9 @@ public void testExceptionSanitization() throws ServletException, IOException { mockControl.verify(); } - private GatewayServlet initializeServletWithSanitization(boolean isErrorMessageSanitizationEnabled) throws ServletException, IOException { + private GatewayServlet initializeServletWithSanitization(boolean isErrorMessageSanitizationEnabled, String sanitizationPattern) throws ServletException, IOException { EasyMock.expect(gatewayConfig.isErrorMessageSanitizationEnabled()).andStubReturn(isErrorMessageSanitizationEnabled); + EasyMock.expect(gatewayConfig.getErrorMessageSanitizationPattern()).andStubReturn(sanitizationPattern); filter.init(EasyMock.anyObject(FilterConfig.class)); EasyMock.expectLastCall().once(); diff --git a/gateway-spi-common/src/main/java/org/apache/knox/gateway/GatewayTestConfig.java b/gateway-spi-common/src/main/java/org/apache/knox/gateway/GatewayTestConfig.java index ec46561b90..7a7a7c1c56 100644 --- a/gateway-spi-common/src/main/java/org/apache/knox/gateway/GatewayTestConfig.java +++ b/gateway-spi-common/src/main/java/org/apache/knox/gateway/GatewayTestConfig.java @@ -79,6 +79,7 @@ public class GatewayTestConfig extends Configuration implements GatewayConfig { private ConcurrentMap topologyPortMapping = new ConcurrentHashMap<>(); private int backupVersionLimit = -1; private long backupAgeLimit = -1; + private String sanitizationPattern = "sanitizationPattern"; public GatewayTestConfig(Properties props) { super.getProps().putAll(props); @@ -624,6 +625,11 @@ public boolean isErrorMessageSanitizationEnabled() { return true; } + @Override + public String getErrorMessageSanitizationPattern() { + return sanitizationPattern; + } + @Override public boolean isMetricsEnabled() { return false; diff --git a/gateway-spi/src/main/java/org/apache/knox/gateway/config/GatewayConfig.java b/gateway-spi/src/main/java/org/apache/knox/gateway/config/GatewayConfig.java index cdc1f8da56..f7261d9f9e 100644 --- a/gateway-spi/src/main/java/org/apache/knox/gateway/config/GatewayConfig.java +++ b/gateway-spi/src/main/java/org/apache/knox/gateway/config/GatewayConfig.java @@ -906,6 +906,11 @@ public interface GatewayConfig { */ boolean isErrorMessageSanitizationEnabled(); + /** + * @return the error message sanitization pattern + */ + String getErrorMessageSanitizationPattern(); + /** * @return true if the supplied user is allowed to see all tokens * (i.e. not only tokens where userName or createdBy equals to the From d9df195fd634b4816de2dfdebc77bbbdc29c9e5f Mon Sep 17 00:00:00 2001 From: kardolus Date: Sat, 8 Jun 2024 09:08:52 -0400 Subject: [PATCH 6/7] Refactor exception handling to use SanitizedException in GatewayServlet --- .../apache/knox/gateway/GatewayServlet.java | 29 ++++--------------- .../knox/gateway/SanitizedException.java | 9 ++++++ 2 files changed, 14 insertions(+), 24 deletions(-) create mode 100644 gateway-server/src/main/java/org/apache/knox/gateway/SanitizedException.java diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java b/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java index b8b8cd2f17..181783d885 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java @@ -42,7 +42,6 @@ import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; -import java.lang.reflect.Constructor; import java.net.URISyntaxException; import java.nio.charset.StandardCharsets; import java.util.Enumeration; @@ -282,33 +281,15 @@ public Enumeration getInitParameterNames() { } } - private Exception sanitizeException(Exception e) { + private SanitizedException logAndSanitizeException(Exception e) { + LOG.failedToExecuteFilter(e); if (e == null || e.getMessage() == null) { - return e; + return new SanitizedException(e.getMessage()); } if (!isErrorMessageSanitizationEnabled || e.getMessage() == null) { - return e; + return new SanitizedException(e.getMessage()); } String sanitizedMessage = e.getMessage().replaceAll(errorMessageSanitizationPattern, "[hidden]"); - - return createSanitizedException(e, sanitizedMessage); - } - - private T createSanitizedException(T e, String sanitizedMessage) { - try { - Constructor constructor = e.getClass().getConstructor(String.class, Throwable.class); - T sanitizedException = (T) constructor.newInstance(sanitizedMessage, e.getCause()); - sanitizedException.setStackTrace(e.getStackTrace()); - return sanitizedException; - } catch (Exception ex) { - Exception genericException = new Exception(sanitizedMessage, e.getCause()); - genericException.setStackTrace(e.getStackTrace()); - return (T) genericException; - } - } - - private T logAndSanitizeException(Exception e) throws T { - LOG.failedToExecuteFilter(e); - throw (T) sanitizeException(e); + return new SanitizedException(sanitizedMessage); } } diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/SanitizedException.java b/gateway-server/src/main/java/org/apache/knox/gateway/SanitizedException.java new file mode 100644 index 0000000000..f7ed7984fa --- /dev/null +++ b/gateway-server/src/main/java/org/apache/knox/gateway/SanitizedException.java @@ -0,0 +1,9 @@ +package org.apache.knox.gateway; + +import javax.servlet.ServletException; + +public class SanitizedException extends ServletException { + public SanitizedException(String message) { + super(message); + } +} From 3253622c12ecdacafc093f439de70a5ba193ceb4 Mon Sep 17 00:00:00 2001 From: kardolus Date: Thu, 13 Jun 2024 13:19:47 -0400 Subject: [PATCH 7/7] Add Apache license --- .../apache/knox/gateway/SanitizedException.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/SanitizedException.java b/gateway-server/src/main/java/org/apache/knox/gateway/SanitizedException.java index f7ed7984fa..b03ab989ce 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/SanitizedException.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/SanitizedException.java @@ -1,3 +1,19 @@ +/* + * 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; import javax.servlet.ServletException;