From 3715d75d39fd18fe1390b188daaf744a035e21fa Mon Sep 17 00:00:00 2001 From: Ovidijus Narkevicius Date: Thu, 25 Apr 2024 15:03:07 +0300 Subject: [PATCH 1/3] fix: Don't reveal DB data in error response Closes: XRDDEV-2620 --- .../main/java/ee/ria/xroad/common/ErrorCodes.java | 4 ++-- .../ee/ria/xroad/common/db/HibernateUtil.java | 2 +- .../ee/ria/xroad/common/util/HandlerBase.java | 10 ++++------ .../ee/ria/xroad/common/util/RequestWrapper.java | 12 ++++++++++++ .../xroad/common/message/SaxSoapParserImpl.java | 5 +++-- .../ee/ria/xroad/common/message/SoapFault.java | 2 +- .../ria/xroad/common/message/SoapParserImpl.java | 7 +++---- .../clientproxy/AbstractClientProxyHandler.java | 8 ++++---- .../proxy/serverproxy/ServerProxyHandler.java | 15 ++------------- .../proxy/util/CertHashBasedOcspResponder.java | 2 +- 10 files changed, 33 insertions(+), 34 deletions(-) diff --git a/src/common/common-core/src/main/java/ee/ria/xroad/common/ErrorCodes.java b/src/common/common-core/src/main/java/ee/ria/xroad/common/ErrorCodes.java index 6c56c86a25..86c4d6f93a 100644 --- a/src/common/common-core/src/main/java/ee/ria/xroad/common/ErrorCodes.java +++ b/src/common/common-core/src/main/java/ee/ria/xroad/common/ErrorCodes.java @@ -202,8 +202,8 @@ public final class ErrorCodes { */ @SuppressWarnings("squid:S1872") public static CodedException translateException(Throwable ex) { - if (ex instanceof CodedException) { - return (CodedException) ex; + if (ex instanceof CodedException cex) { + return cex; } else if (ex instanceof UnknownHostException || ex instanceof MalformedURLException || ex instanceof SocketException diff --git a/src/common/common-db/src/main/java/ee/ria/xroad/common/db/HibernateUtil.java b/src/common/common-db/src/main/java/ee/ria/xroad/common/db/HibernateUtil.java index 225e0563d0..da9ebdaf85 100644 --- a/src/common/common-db/src/main/java/ee/ria/xroad/common/db/HibernateUtil.java +++ b/src/common/common-db/src/main/java/ee/ria/xroad/common/db/HibernateUtil.java @@ -92,7 +92,7 @@ public static synchronized SessionFactory getSessionFactory(String name, Interce } catch (Exception e) { log.error("Failed to create session factory", e); - throw new CodedException(X_DATABASE_ERROR, e); + throw new CodedException(X_DATABASE_ERROR, e, "Error accessing database (%s)", name); } } } diff --git a/src/common/common-jetty/src/main/java/ee/ria/xroad/common/util/HandlerBase.java b/src/common/common-jetty/src/main/java/ee/ria/xroad/common/util/HandlerBase.java index e206be9cbf..c7b44f2c41 100644 --- a/src/common/common-jetty/src/main/java/ee/ria/xroad/common/util/HandlerBase.java +++ b/src/common/common-jetty/src/main/java/ee/ria/xroad/common/util/HandlerBase.java @@ -36,6 +36,7 @@ import java.io.IOException; import java.nio.ByteBuffer; +import java.nio.charset.StandardCharsets; import static ee.ria.xroad.common.util.JettyUtils.setContentLength; import static ee.ria.xroad.common.util.JettyUtils.setContentType; @@ -58,8 +59,7 @@ public void sendErrorResponse(Request request, Response response, Callback callback, CodedException ex) throws IOException { - String faultXml = ex instanceof CodedException.Fault - ? ((CodedException.Fault) ex).getFaultXml() : SoapFault.createFaultXml(ex); + String faultXml = SoapFault.createFaultXml(ex); String encoding = MimeUtils.UTF8; byte[] messageBytes = faultXml.getBytes(encoding); @@ -76,11 +76,9 @@ public void sendErrorResponse(Request request, * @param response HTTP servlet response for sending the plain fault * @param status HTTP status code * @param message fault message - * @throws IOException if an I/O error occurred */ - public void sendPlainTextErrorResponse(Response response, Callback callback, int status, String message) - throws IOException { - byte[] messageBytes = message.getBytes("UTF-8"); + public void sendPlainTextErrorResponse(Response response, Callback callback, int status, String message) { + byte[] messageBytes = message.getBytes(StandardCharsets.UTF_8); response.setStatus(status); setContentType(response, MimeTypes.TEXT_PLAIN_UTF8); setContentLength(response, messageBytes.length); diff --git a/src/common/common-jetty/src/main/java/ee/ria/xroad/common/util/RequestWrapper.java b/src/common/common-jetty/src/main/java/ee/ria/xroad/common/util/RequestWrapper.java index 5c702ed5a8..53eb80004a 100644 --- a/src/common/common-jetty/src/main/java/ee/ria/xroad/common/util/RequestWrapper.java +++ b/src/common/common-jetty/src/main/java/ee/ria/xroad/common/util/RequestWrapper.java @@ -28,11 +28,14 @@ import org.apache.commons.io.input.ProxyInputStream; import org.eclipse.jetty.http.HttpFields; import org.eclipse.jetty.http.HttpURI; +import org.eclipse.jetty.io.EndPoint; import org.eclipse.jetty.server.Request; import java.io.IOException; import java.io.InputStream; +import java.security.cert.X509Certificate; import java.util.Map; +import java.util.Optional; public interface RequestWrapper { InputStream getInputStream(); @@ -51,6 +54,8 @@ public interface RequestWrapper { Map getParametersMap() throws Exception; + Optional getPeerCertificates(); + static RequestWrapper of(Request request) { var in = new ProxyInputStream(Request.asInputStream(request)) { @Override @@ -101,6 +106,13 @@ public String getParameter(String name) throws Exception { public Map getParametersMap() throws Exception { return Request.getParameters(request).toStringArrayMap(); } + + @Override + public Optional getPeerCertificates() { + var ssd = (EndPoint.SslSessionData) request.getAttribute(EndPoint.SslSessionData.ATTRIBUTE); + return Optional.ofNullable(ssd) + .map(EndPoint.SslSessionData::peerCertificates); + } }; } } diff --git a/src/common/common-message/src/main/java/ee/ria/xroad/common/message/SaxSoapParserImpl.java b/src/common/common-message/src/main/java/ee/ria/xroad/common/message/SaxSoapParserImpl.java index c30d8fe5b4..a9a344374a 100644 --- a/src/common/common-message/src/main/java/ee/ria/xroad/common/message/SaxSoapParserImpl.java +++ b/src/common/common-message/src/main/java/ee/ria/xroad/common/message/SaxSoapParserImpl.java @@ -56,6 +56,7 @@ import java.io.BufferedWriter; import java.io.ByteArrayOutputStream; +import java.io.IOException; import java.io.InputStream; import java.io.OutputStreamWriter; import java.io.Writer; @@ -263,8 +264,8 @@ protected boolean isProcessedXmlRequired() { return false; } - private InputStream excludeUtf8Bom(String contentType, InputStream soapStream) { - return hasUtf8Charset(contentType) ? new BOMInputStream(soapStream) : soapStream; + private InputStream excludeUtf8Bom(String contentType, InputStream soapStream) throws IOException { + return hasUtf8Charset(contentType) ? BOMInputStream.builder().setInputStream(soapStream).get() : soapStream; } @SneakyThrows diff --git a/src/common/common-message/src/main/java/ee/ria/xroad/common/message/SoapFault.java b/src/common/common-message/src/main/java/ee/ria/xroad/common/message/SoapFault.java index b60bfd3659..bc691ed1c1 100644 --- a/src/common/common-message/src/main/java/ee/ria/xroad/common/message/SoapFault.java +++ b/src/common/common-message/src/main/java/ee/ria/xroad/common/message/SoapFault.java @@ -29,7 +29,7 @@ import jakarta.xml.soap.SOAPFault; import lombok.SneakyThrows; -import org.apache.commons.lang3.StringEscapeUtils; +import org.apache.commons.text.StringEscapeUtils; /** * Soap interface implementation representing an error message. diff --git a/src/common/common-message/src/main/java/ee/ria/xroad/common/message/SoapParserImpl.java b/src/common/common-message/src/main/java/ee/ria/xroad/common/message/SoapParserImpl.java index 551e87483e..a97f9da8d7 100644 --- a/src/common/common-message/src/main/java/ee/ria/xroad/common/message/SoapParserImpl.java +++ b/src/common/common-message/src/main/java/ee/ria/xroad/common/message/SoapParserImpl.java @@ -46,6 +46,7 @@ import javax.xml.namespace.QName; import java.io.ByteArrayInputStream; +import java.io.IOException; import java.io.InputStream; import java.util.HashSet; import java.util.Iterator; @@ -105,10 +106,8 @@ protected Soap parseMessage(String contentType, InputStream is) return parseMessage(rawXml, soap, charset, contentType); } - private InputStream excludeUtf8Bom(String contentType, - InputStream soapStream) { - return hasUtf8Charset(contentType) - ? new BOMInputStream(soapStream) : soapStream; + private InputStream excludeUtf8Bom(String contentType, InputStream soapStream) throws IOException { + return hasUtf8Charset(contentType) ? BOMInputStream.builder().setInputStream(soapStream).get() : soapStream; } protected Soap parseMessage(byte[] rawXml, SOAPMessage soap, diff --git a/src/proxy/src/main/java/ee/ria/xroad/proxy/clientproxy/AbstractClientProxyHandler.java b/src/proxy/src/main/java/ee/ria/xroad/proxy/clientproxy/AbstractClientProxyHandler.java index a55e0fc482..8b4cf522c3 100644 --- a/src/proxy/src/main/java/ee/ria/xroad/proxy/clientproxy/AbstractClientProxyHandler.java +++ b/src/proxy/src/main/java/ee/ria/xroad/proxy/clientproxy/AbstractClientProxyHandler.java @@ -40,7 +40,6 @@ import lombok.extern.slf4j.Slf4j; import org.apache.commons.lang3.ArrayUtils; import org.apache.http.client.HttpClient; -import org.eclipse.jetty.io.EndPoint; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.Response; import org.eclipse.jetty.util.Callback; @@ -174,10 +173,11 @@ static boolean isPostRequest(RequestWrapper request) { } static IsAuthenticationData getIsAuthenticationData(RequestWrapper request) { - var ssd = (EndPoint.SslSessionData) request.getAttribute(EndPoint.SslSessionData.ATTRIBUTE); - var isPlaintextConnection = !"https".equals(request.getHttpURI().getScheme()); // if not HTTPS, it's plaintext - var cert = (ssd == null || ArrayUtils.isEmpty(ssd.peerCertificates())) ? null : ssd.peerCertificates()[0]; + var cert = request.getPeerCertificates() + .filter(ArrayUtils::isNotEmpty) + .map(arr -> arr[0]) + .orElse(null); return new IsAuthenticationData(cert, isPlaintextConnection); } diff --git a/src/proxy/src/main/java/ee/ria/xroad/proxy/serverproxy/ServerProxyHandler.java b/src/proxy/src/main/java/ee/ria/xroad/proxy/serverproxy/ServerProxyHandler.java index 31f964008a..e07f5f2915 100644 --- a/src/proxy/src/main/java/ee/ria/xroad/proxy/serverproxy/ServerProxyHandler.java +++ b/src/proxy/src/main/java/ee/ria/xroad/proxy/serverproxy/ServerProxyHandler.java @@ -38,13 +38,11 @@ import lombok.extern.slf4j.Slf4j; import org.apache.http.client.HttpClient; -import org.eclipse.jetty.io.EndPoint; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.Response; import org.eclipse.jetty.util.Callback; import java.io.IOException; -import java.security.cert.X509Certificate; import static ee.ria.xroad.common.ErrorCodes.SERVER_SERVERPROXY_X; import static ee.ria.xroad.common.ErrorCodes.X_INVALID_HTTP_METHOD; @@ -114,10 +112,10 @@ private MessageProcessorBase createRequestProcessor(RequestWrapper request, Resp OpMonitoringData opMonitoringData) { if (VALUE_MESSAGE_TYPE_REST.equals(request.getHeaders().get(HEADER_MESSAGE_TYPE))) { - return new ServerRestMessageProcessor(request, response, client, getClientSslCertChain(request), + return new ServerRestMessageProcessor(request, response, client, request.getPeerCertificates().orElse(null), opMonitoringData); } else { - return new ServerMessageProcessor(request, response, client, getClientSslCertChain(request), + return new ServerMessageProcessor(request, response, client, request.getPeerCertificates().orElse(null), opMonitorClient, opMonitoringData); } } @@ -127,13 +125,4 @@ protected void failure(Request request, Response response, Callback callback, Co throws IOException { sendErrorResponse(request, response, callback, e); } - - private static X509Certificate[] getClientSslCertChain(RequestWrapper request) { - Object attribute = request.getAttribute(EndPoint.SslSessionData.ATTRIBUTE); - if (attribute != null) { - return ((EndPoint.SslSessionData) attribute).peerCertificates(); - } else { - return null; - } - } } diff --git a/src/proxy/src/main/java/ee/ria/xroad/proxy/util/CertHashBasedOcspResponder.java b/src/proxy/src/main/java/ee/ria/xroad/proxy/util/CertHashBasedOcspResponder.java index 7cd59b9d33..845085a44d 100644 --- a/src/proxy/src/main/java/ee/ria/xroad/proxy/util/CertHashBasedOcspResponder.java +++ b/src/proxy/src/main/java/ee/ria/xroad/proxy/util/CertHashBasedOcspResponder.java @@ -170,7 +170,7 @@ private void doHandleRequest(Request request, Response response) throws Exceptio private final class RequestHandler extends Handler.Abstract { @Override - public boolean handle(Request request, Response response, Callback callback) throws Exception { + public boolean handle(Request request, Response response, Callback callback) { log.trace("Received {} request from {}", request.getMethod(), getRemoteAddr(request)); try { From 36aa3a56b20e765dbebad7f23044be1decabf6e1 Mon Sep 17 00:00:00 2001 From: Ovidijus Narkevicius Date: Wed, 8 May 2024 12:38:22 +0300 Subject: [PATCH 2/3] fix: add small test Closes: XRDDEV-2620 --- .../ee/ria/xroad/common/db/HibernateUtil.java | 6 +- .../xroad/common/db/HibernateUtilTest.java | 75 +++++++++++++++++++ .../src/test/resources/empty_db.properties | 0 3 files changed, 80 insertions(+), 1 deletion(-) create mode 100644 src/common/common-db/src/test/java/ee/ria/xroad/common/db/HibernateUtilTest.java create mode 100644 src/common/common-db/src/test/resources/empty_db.properties diff --git a/src/common/common-db/src/main/java/ee/ria/xroad/common/db/HibernateUtil.java b/src/common/common-db/src/main/java/ee/ria/xroad/common/db/HibernateUtil.java index da9ebdaf85..df483e0e3e 100644 --- a/src/common/common-db/src/main/java/ee/ria/xroad/common/db/HibernateUtil.java +++ b/src/common/common-db/src/main/java/ee/ria/xroad/common/db/HibernateUtil.java @@ -144,7 +144,7 @@ private static void closeSessionFactory(SessionFactoryCtx ctx) { private static SessionFactoryCtx createSessionFactoryCtx(String name, Interceptor interceptor) throws Exception { log.trace("Creating session factory for '{}'...", name); - Configuration configuration = new Configuration(); + Configuration configuration = createEmptyConfiguration(); if (interceptor != null) { configuration.setInterceptor(interceptor); } @@ -160,6 +160,10 @@ private static SessionFactoryCtx createSessionFactoryCtx(String name, Intercepto return new SessionFactoryCtx(sessionFactory); } + static Configuration createEmptyConfiguration() { + return new Configuration(); + } + private static void applySystemProperties(Configuration configuration, String name) { final String prefix = name + ".hibernate."; for (String key : System.getProperties().stringPropertyNames()) { diff --git a/src/common/common-db/src/test/java/ee/ria/xroad/common/db/HibernateUtilTest.java b/src/common/common-db/src/test/java/ee/ria/xroad/common/db/HibernateUtilTest.java new file mode 100644 index 0000000000..b2c818ae51 --- /dev/null +++ b/src/common/common-db/src/test/java/ee/ria/xroad/common/db/HibernateUtilTest.java @@ -0,0 +1,75 @@ +/* + * The MIT License + * Copyright (c) 2019- Nordic Institute for Interoperability Solutions (NIIS) + * Copyright (c) 2018 Estonian Information System Authority (RIA), + * Nordic Institute for Interoperability Solutions (NIIS), Population Register Centre (VRK) + * Copyright (c) 2015-2017 Estonian Information System Authority (RIA), Population Register Centre (VRK) + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +package ee.ria.xroad.common.db; + +import ee.ria.xroad.common.CodedException; +import ee.ria.xroad.common.SystemProperties; + +import org.hibernate.HibernateException; +import org.hibernate.cfg.Configuration; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.MockedStatic; +import org.mockito.Mockito; +import org.mockito.junit.jupiter.MockitoExtension; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.fail; +import static org.mockito.Mockito.CALLS_REAL_METHODS; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +class HibernateUtilTest { + private static final String TEST_SESSION_FACTORY_NAME = "testSessionFactory"; + + @Test + void getSessionFactoryHideErrorDetails() { + + try ( + MockedStatic db = Mockito.mockStatic(HibernateUtil.class, CALLS_REAL_METHODS); + MockedStatic system = Mockito.mockStatic(SystemProperties.class) + ) { + var config = mock(Configuration.class); + db.when(HibernateUtil::createEmptyConfiguration).thenReturn(config); + system.when(SystemProperties::getDatabasePropertiesFile) + .thenReturn(HibernateUtilTest.class.getResource("/empty_db.properties").getFile()); + when(config.configure()) + .thenReturn(config); + when(config.buildSessionFactory()) + .thenThrow(new HibernateException("username and ip address")); + HibernateUtil.getSessionFactory(TEST_SESSION_FACTORY_NAME); + + fail("Should have thrown an exception"); + } catch (Exception e) { + assertThat(e) + .isInstanceOf(CodedException.class) + .hasMessageContaining("DatabaseError: Error accessing database (testSessionFactory)"); + } + + + } +} diff --git a/src/common/common-db/src/test/resources/empty_db.properties b/src/common/common-db/src/test/resources/empty_db.properties new file mode 100644 index 0000000000..e69de29bb2 From 4f2aa4868fddb95ef709d9289b47cfe8e60d8654 Mon Sep 17 00:00:00 2001 From: Ovidijus Narkevicius Date: Wed, 8 May 2024 14:27:01 +0300 Subject: [PATCH 3/3] fix: make sonar happy Closes: XRDDEV-2620 --- .../xroad/common/db/HibernateUtilTest.java | 20 +++++++++++++------ .../AbstractClientProxyHandler.java | 2 +- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/common/common-db/src/test/java/ee/ria/xroad/common/db/HibernateUtilTest.java b/src/common/common-db/src/test/java/ee/ria/xroad/common/db/HibernateUtilTest.java index b2c818ae51..cbe8c18fd9 100644 --- a/src/common/common-db/src/test/java/ee/ria/xroad/common/db/HibernateUtilTest.java +++ b/src/common/common-db/src/test/java/ee/ria/xroad/common/db/HibernateUtilTest.java @@ -30,22 +30,33 @@ import org.hibernate.HibernateException; import org.hibernate.cfg.Configuration; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; import org.mockito.MockedStatic; import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.fail; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.CALLS_REAL_METHODS; -import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) class HibernateUtilTest { private static final String TEST_SESSION_FACTORY_NAME = "testSessionFactory"; + @Mock + private Configuration configuration; + + @BeforeEach + public void setUp() { + when(configuration.configure()).thenReturn(configuration); + when(configuration.configure(anyString())).thenReturn(configuration); + } + @Test void getSessionFactoryHideErrorDetails() { @@ -53,13 +64,10 @@ void getSessionFactoryHideErrorDetails() { MockedStatic db = Mockito.mockStatic(HibernateUtil.class, CALLS_REAL_METHODS); MockedStatic system = Mockito.mockStatic(SystemProperties.class) ) { - var config = mock(Configuration.class); - db.when(HibernateUtil::createEmptyConfiguration).thenReturn(config); + db.when(HibernateUtil::createEmptyConfiguration).thenReturn(configuration); system.when(SystemProperties::getDatabasePropertiesFile) .thenReturn(HibernateUtilTest.class.getResource("/empty_db.properties").getFile()); - when(config.configure()) - .thenReturn(config); - when(config.buildSessionFactory()) + when(configuration.buildSessionFactory()) .thenThrow(new HibernateException("username and ip address")); HibernateUtil.getSessionFactory(TEST_SESSION_FACTORY_NAME); diff --git a/src/proxy/src/main/java/ee/ria/xroad/proxy/clientproxy/AbstractClientProxyHandler.java b/src/proxy/src/main/java/ee/ria/xroad/proxy/clientproxy/AbstractClientProxyHandler.java index 8b4cf522c3..729fd16215 100644 --- a/src/proxy/src/main/java/ee/ria/xroad/proxy/clientproxy/AbstractClientProxyHandler.java +++ b/src/proxy/src/main/java/ee/ria/xroad/proxy/clientproxy/AbstractClientProxyHandler.java @@ -157,7 +157,7 @@ protected void failure(MessageProcessorBase processor, Request request, Response } protected void failure(Response response, Callback callback, CodedExceptionWithHttpStatus e, - OpMonitoringData opMonitoringData) throws IOException { + OpMonitoringData opMonitoringData) { updateOpMonitoringResponseOutTs(opMonitoringData);