From e38303935b92247770c1981a60d6e558f89e1229 Mon Sep 17 00:00:00 2001 From: Avgustin Marinov Date: Wed, 22 Nov 2023 10:22:24 +0200 Subject: [PATCH] Improves address resolution (#1483) Signed-off-by: Marinov Avgustin --- .../ddi/rest/resource/DosFilterTest.java | 13 +++-- .../java/org/eclipse/hawkbit/util/IpUtil.java | 45 ++++++++------- .../org/eclipse/hawkbit/util/IpUtilTest.java | 57 +++++++++++++++++-- 3 files changed, 81 insertions(+), 34 deletions(-) diff --git a/hawkbit-rest/hawkbit-ddi-resource/src/test/java/org/eclipse/hawkbit/ddi/rest/resource/DosFilterTest.java b/hawkbit-rest/hawkbit-ddi-resource/src/test/java/org/eclipse/hawkbit/ddi/rest/resource/DosFilterTest.java index 2db999945..a7039e872 100644 --- a/hawkbit-rest/hawkbit-ddi-resource/src/test/java/org/eclipse/hawkbit/ddi/rest/resource/DosFilterTest.java +++ b/hawkbit-rest/hawkbit-ddi-resource/src/test/java/org/eclipse/hawkbit/ddi/rest/resource/DosFilterTest.java @@ -46,12 +46,14 @@ class DosFilterTest extends AbstractDDiApiIntegrationTest { @Override protected DefaultMockMvcBuilder createMvcWebAppContext(final WebApplicationContext context) { - return super.createMvcWebAppContext(context).addFilter(new DosFilter(null, 10, 10, - "127\\.0\\.0\\.1|\\[0:0:0:0:0:0:0:1\\]", "(^192\\.168\\.)", "X-Forwarded-For")); + return super.createMvcWebAppContext(context).addFilter( + new DosFilter(null, 10, 10, + "127\\.0\\.0\\.1|\\[0:0:0:0:0:0:0:1\\]", "(^192\\.168\\.)", + "X-Forwarded-For")); } @Test - @Description("Ensures that clients that are on the blacklist are forbidded ") + @Description("Ensures that clients that are on the blacklist are forbidden") void blackListedClientIsForbidden() throws Exception { mvc.perform(get("/{tenant}/controller/v1/4711", tenantAware.getCurrentTenant()) .header(HttpHeaders.X_FORWARDED_FOR, "192.168.0.4 , 10.0.0.1 ")).andExpect(status().isForbidden()); @@ -59,7 +61,7 @@ class DosFilterTest extends AbstractDDiApiIntegrationTest { @Test @Description("Ensures that a READ DoS attempt is blocked ") - void getFloddingAttackThatisPrevented() throws Exception { + void getFloodingAttackThatIsPrevented() throws Exception { MvcResult result = null; @@ -168,5 +170,4 @@ class DosFilterTest extends AbstractDDiApiIntegrationTest { return uaction.getId(); } - -} +} \ No newline at end of file diff --git a/hawkbit-security-core/src/main/java/org/eclipse/hawkbit/util/IpUtil.java b/hawkbit-security-core/src/main/java/org/eclipse/hawkbit/util/IpUtil.java index a372c52b2..37919ef5d 100644 --- a/hawkbit-security-core/src/main/java/org/eclipse/hawkbit/util/IpUtil.java +++ b/hawkbit-security-core/src/main/java/org/eclipse/hawkbit/util/IpUtil.java @@ -28,13 +28,16 @@ import org.eclipse.hawkbit.security.HawkbitSecurityProperties; public final class IpUtil { private static final String HIDDEN_IP = "***"; - private static final String SCHEME_SEPERATOR = "://"; + private static final String SCHEME_SEPARATOR = "://"; private static final String HTTP_SCHEME = "http"; private static final String AMQP_SCHEME = "amqp"; - private static final Pattern IPV4_ADDRESS_PATTERN = Pattern - .compile("([0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3})"); + // v4 address with (optionally) port + private static final Pattern IPV4_ADDRESS_PATTERN = Pattern + .compile("([0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3})(:[0-9]{1,5})?"); private static final Pattern IPV6_ADDRESS_PATTERN = Pattern.compile("([0-9a-f]{1,4}:){7}([0-9a-f]){1,4}"); + // v6 address with [] amd (optionally) port + private static final Pattern IPV6_ADDRESS_WITH_PORT_PATTERN = Pattern.compile("\\[(?
([0-9a-f]{1,4}:){7}([0-9a-f]){1,4})](:[0-9]{1,5})?"); private IpUtil() { @@ -42,9 +45,8 @@ public final class IpUtil { /** * Retrieves the string based IP address from a given - * {@link HttpServletRequest} by either the - * {@link HttpHeaders#X_FORWARDED_FOR} or by the - * {@link HttpServletRequest#getRemoteAddr()} methods. + * {@link HttpServletRequest} by either the configured {@link HawkbitSecurityProperties.Clients#getRemoteIpHeader()} + * (by default X-Forwarded-For) or by the {@link HttpServletRequest#getRemoteAddr()} method. * * @param request * the {@link HttpServletRequest} to determine the IP address @@ -62,10 +64,8 @@ public final class IpUtil { } /** - * Retrieves the string based IP address from a given - * {@link HttpServletRequest} by either the - * {@link HttpHeaders#X_FORWARDED_FOR} or by the - * {@link HttpServletRequest#getRemoteAddr()} methods. + * Retrieves the string based IP address from a given {@link HttpServletRequest} by either the + * forward header or by the {@link HttpServletRequest#getRemoteAddr()} method. * * @param request * the {@link HttpServletRequest} to determine the IP address @@ -82,7 +82,6 @@ public final class IpUtil { private static URI getClientIpFromRequest(final HttpServletRequest request, final String forwardHeader, final boolean trackRemoteIp) { - String ip; if (trackRemoteIp) { @@ -98,17 +97,19 @@ public final class IpUtil { } private static String findClientIpAddress(final String s) { - - final Matcher matcherv4 = IPV4_ADDRESS_PATTERN.matcher(s); - - if (matcherv4.find()) { - return matcherv4.group(0); + Matcher matcher = IPV4_ADDRESS_PATTERN.matcher(s); + if (matcher.find()) { + return matcher.group(1); } - final Matcher matcherv6 = IPV6_ADDRESS_PATTERN.matcher(s); + matcher = IPV6_ADDRESS_PATTERN.matcher(s); + if (matcher.find()) { + return matcher.group(0); + } - if (matcherv6.find()) { - return matcherv6.group(0); + matcher = IPV6_ADDRESS_WITH_PORT_PATTERN.matcher(s); + if (matcher.find()) { + return matcher.group("address"); } return null; @@ -126,11 +127,11 @@ public final class IpUtil { * If the given string not parsable */ public static URI createUri(final String scheme, final String host) { - final boolean isIpV6 = host.indexOf(':') >= 0 && host.charAt(0) != '['; + final boolean isIpV6 = host.indexOf(':') >= 0 && host.indexOf('.') == -1 && host.charAt(0) != '['; if (isIpV6) { - return URI.create(scheme + SCHEME_SEPERATOR + "[" + host + "]"); + return URI.create(scheme + SCHEME_SEPARATOR + "[" + host + "]"); } - return URI.create(scheme + SCHEME_SEPERATOR + host); + return URI.create(scheme + SCHEME_SEPARATOR + host); } /** diff --git a/hawkbit-security-core/src/test/java/org/eclipse/hawkbit/util/IpUtilTest.java b/hawkbit-security-core/src/test/java/org/eclipse/hawkbit/util/IpUtilTest.java index 9685aa79d..47321edd2 100644 --- a/hawkbit-security-core/src/test/java/org/eclipse/hawkbit/util/IpUtilTest.java +++ b/hawkbit-security-core/src/test/java/org/eclipse/hawkbit/util/IpUtilTest.java @@ -11,6 +11,7 @@ package org.eclipse.hawkbit.util; import static com.google.common.net.HttpHeaders.X_FORWARDED_FOR; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.reset; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -49,8 +50,7 @@ public class IpUtilTest { @Test @Description("Tests create uri from request") - public void getRemoteAddrFromRequestIfForwaredHeaderNotPresent() { - + public void getRemoteAddrFromRequestIfForwardedHeaderNotPresent() { final URI knownRemoteClientIP = IpUtil.createHttpUri("127.0.0.1"); when(requestMock.getRemoteAddr()).thenReturn(knownRemoteClientIP.getHost()); @@ -66,7 +66,6 @@ public class IpUtilTest { @Test @Description("Tests create uri from request with masked IP when IP tracking is disabled") public void maskRemoteAddrIfDisabled() { - final URI knownRemoteClientIP = IpUtil.createHttpUri("***"); when(securityPropertyMock.getClients()).thenReturn(clientMock); when(clientMock.getRemoteIpHeader()).thenReturn(KNOWN_REQUEST_HEADER); @@ -83,7 +82,6 @@ public class IpUtilTest { @Test @Description("Tests create uri from x forward header") public void getRemoteAddrFromXForwardedForHeader() { - final URI knownRemoteClientIP = IpUtil.createHttpUri("10.99.99.1"); when(requestMock.getHeader(X_FORWARDED_FOR)).thenReturn(knownRemoteClientIP.getHost()); @@ -95,10 +93,47 @@ public class IpUtilTest { verify(requestMock, times(0)).getRemoteAddr(); } + @Test + @Description("Tests client uri from request") + public void testCreateClientHttpUri() { + checkHostInfoResolution("0:0:0:0:0:0:0:1", "[0:0:0:0:0:0:0:1]", true); + checkHostInfoResolution("127.0.0.1", "127.0.0.1", true); + checkHostInfoResolution("127.0.0.1:93493", "127.0.0.1", true); + checkHostInfoResolution("myhost", "myhost", true); + checkHostInfoResolution("myhost.my", "myhost.my", true); + checkHostInfoResolution("myhost.my:4233", "myhost.my", true); + checkHostInfoResolution("[0:0:0:0:0:0:0:1]", "[0:0:0:0:0:0:0:1]", true); + checkHostInfoResolution("[0:0:0:0:0:0:0:1]:4233", "[0:0:0:0:0:0:0:1]", true); + } + + @Test + @Description("Tests client uri from request") + public void testResolveClientIpFromHeader() { + checkHostInfoResolution("0:0:0:0:0:0:0:1", "[0:0:0:0:0:0:0:1]", false); + checkHostInfoResolution("127.0.0.1", "127.0.0.1", false); + checkHostInfoResolution("127.0.0.1:93493", "127.0.0.1", false); + checkHostInfoResolution("[0:0:0:0:0:0:0:1]", "[0:0:0:0:0:0:0:1]", false); + checkHostInfoResolution("[0:0:0:0:0:0:0:1]:4233", "[0:0:0:0:0:0:0:1]", false); + } + + private void checkHostInfoResolution(final String hostInfo, final String expectedHost, final boolean remoteAddress) { + reset(requestMock); + when(remoteAddress ? requestMock.getRemoteAddr() : requestMock.getHeader(KNOWN_REQUEST_HEADER)).thenReturn(hostInfo); + + final URI remoteAddr = IpUtil.getClientIpFromRequest(requestMock, KNOWN_REQUEST_HEADER); + + // verify + assertThat(remoteAddr.getHost()).as("The remote address should be as the known client IP address") + .isEqualTo(expectedHost); + verify(requestMock, times(1)).getHeader(KNOWN_REQUEST_HEADER); + if (remoteAddress) { + verify(requestMock, times(1)).getRemoteAddr(); + } + } + @Test @Description("Tests create http uri ipv4 and ipv6") public void testCreateHttpUri() { - final String ipv4 = "10.99.99.1"; URI httpUri = IpUtil.createHttpUri(ipv4); assertHttpUri(ipv4, httpUri); @@ -122,18 +157,28 @@ public class IpUtilTest { @Test @Description("Tests create amqp uri ipv4 and ipv6") public void testCreateAmqpUri() { - final String ipv4 = "10.99.99.1"; URI amqpUri = IpUtil.createAmqpUri(ipv4, "path"); assertAmqpUri(ipv4, amqpUri); + final String ipv4Port = ipv4 + ":12000"; + amqpUri = IpUtil.createAmqpUri(ipv4Port, "path"); + assertAmqpUri(ipv4, amqpUri); final String host = "myhost"; amqpUri = IpUtil.createAmqpUri(host, "path"); assertAmqpUri(host, amqpUri); + final String hostDots = "myhost.my"; + amqpUri = IpUtil.createAmqpUri(hostDots, "path"); + assertAmqpUri(hostDots, amqpUri); + final String ipv6 = "0:0:0:0:0:0:0:1"; amqpUri = IpUtil.createAmqpUri(ipv6, "path"); assertAmqpUri("[" + ipv6 + "]", amqpUri); + + final String ipv6Braces = "[0:0:0:0:0:0:0:1]"; + amqpUri = IpUtil.createAmqpUri(ipv6Braces, "path"); + assertAmqpUri(ipv6Braces, amqpUri); } private void assertAmqpUri(final String host, final URI amqpUri) {