diff --git a/common/src/main/java/org/onehippo/forge/ipfilter/common/AuthObject.java b/common/src/main/java/org/onehippo/forge/ipfilter/common/AuthObject.java index 050825f..1ed70e7 100644 --- a/common/src/main/java/org/onehippo/forge/ipfilter/common/AuthObject.java +++ b/common/src/main/java/org/onehippo/forge/ipfilter/common/AuthObject.java @@ -43,7 +43,9 @@ public class AuthObject { private final Set hosts; private final Set ranges; private final Set ignoredPaths; + private final Set trustedProxies; private final Set ipMatchers; + private final Set trustedProxyMatchers; private final Map> ignoredHeaders; private final List hostPatterns; private final List ignoredPathPatterns; @@ -55,10 +57,12 @@ private AuthObject() { this.ignoredPaths = Collections.emptySet(); this.hosts = Collections.emptySet(); this.ranges = Collections.emptySet(); + this.trustedProxies = Collections.emptySet(); this.ignoredHeaders = Collections.emptyMap(); this.ignoredPathPatterns = Collections.emptyList(); this.hostPatterns = Collections.emptyList(); this.ipMatchers = Collections.emptySet(); + this.trustedProxyMatchers = Collections.emptySet(); this.allowCmsUsers = false; this.mustMatchAll = false; this.forwardedForHeader = IpFilterConstants.HEADER_X_FORWARDED_FOR; @@ -66,12 +70,15 @@ private AuthObject() { public AuthObject(final Set ignoredPaths, final Set hosts, final Set ranges, final Map> ignoredHeaders, - final boolean allowCmsUsers, final String forwardHeader, final boolean cacheEnabled, final boolean mustMatchAll) { + final boolean allowCmsUsers, final String forwardHeader, + final Set trustedProxies, + final boolean cacheEnabled, final boolean mustMatchAll) { this.valid = true; this.cacheEnabled = cacheEnabled; this.ignoredPaths = ignoredPaths; this.hosts = hosts; this.ranges = ranges; + this.trustedProxies = trustedProxies; this.allowCmsUsers = allowCmsUsers; this.mustMatchAll = mustMatchAll; this.forwardedForHeader = forwardHeader; @@ -79,6 +86,7 @@ public AuthObject(final Set ignoredPaths, final Set hosts, this.ignoredPathPatterns = parsePatterns(); this.hostPatterns = parseHostPatterns(); this.ipMatchers = parseIpMatchers(); + this.trustedProxyMatchers = parseTrustedProxyMatchers(); } public boolean isValid() { @@ -111,6 +119,10 @@ public String getForwardedForHeader() { return forwardedForHeader; } + public Set getTrustedProxyMatchers() { + return trustedProxyMatchers; + } + public Map> getIgnoredHeaders() { return ignoredHeaders; @@ -156,6 +168,21 @@ private Set parseIpMatchers() { return ImmutableSet.copyOf(ipMatchers); } + private Set parseTrustedProxyMatchers() { + final Set matchers = new HashSet<>(); + if (trustedProxies != null) { + for (String proxy : trustedProxies) { + final IpMatcher matcher = IpMatcher.valueOf(proxy); + if (matcher != null) { + matchers.add(matcher); + } else { + log.warn("Skipping invalid trusted proxy value: {}", proxy); + } + } + } + return ImmutableSet.copyOf(matchers); + } + private List parsePatterns() { final List patterns = new ArrayList<>(); for (String ignored : ignoredPaths) { diff --git a/common/src/main/java/org/onehippo/forge/ipfilter/common/BaseIpFilter.java b/common/src/main/java/org/onehippo/forge/ipfilter/common/BaseIpFilter.java index 2a61a37..90523df 100644 --- a/common/src/main/java/org/onehippo/forge/ipfilter/common/BaseIpFilter.java +++ b/common/src/main/java/org/onehippo/forge/ipfilter/common/BaseIpFilter.java @@ -158,7 +158,7 @@ private Status allowed(final HttpServletRequest request) { return Status.OK; } - final String ip = IpFilterUtils.getIp(request, authObject.getForwardedForHeader()); + final String ip = IpFilterUtils.getIp(request, authObject.getForwardedForHeader(), authObject.getTrustedProxyMatchers()); if (Strings.isNullOrEmpty(ip)) { // shouldn't happen log.warn("{}: IP was null or empty. Host is {}", this.getClass().getSimpleName(), host); diff --git a/common/src/main/java/org/onehippo/forge/ipfilter/common/IpFilterConfigLoader.java b/common/src/main/java/org/onehippo/forge/ipfilter/common/IpFilterConfigLoader.java index 773a32b..e52d5e4 100644 --- a/common/src/main/java/org/onehippo/forge/ipfilter/common/IpFilterConfigLoader.java +++ b/common/src/main/java/org/onehippo/forge/ipfilter/common/IpFilterConfigLoader.java @@ -359,7 +359,11 @@ private AuthObject parse(final Node node, final Multimap globalS final boolean matchAll = JcrUtils.getBooleanProperty(node, IpFilterConstants.CONFIG_MATCH_ALL, false); final boolean cacheEnabled = JcrUtils.getBooleanProperty(node, IpFilterConstants.CONFIG_CACHE_ENABLED, true); - return new AuthObject(ignoredPathSet, hostSet, rangesSet, ignoredHeaders, allowCmsUsers, forwardHeader, cacheEnabled, matchAll); + final Set trustedProxiesSet = new HashSet<>(); + final String[] trustedProxies = JcrUtils.getMultipleStringProperty(node, IpFilterConstants.CONFIG_TRUSTED_PROXIES, ArrayUtils.EMPTY_STRING_ARRAY); + Collections.addAll(trustedProxiesSet, trustedProxies); + + return new AuthObject(ignoredPathSet, hostSet, rangesSet, ignoredHeaders, allowCmsUsers, forwardHeader, trustedProxiesSet, cacheEnabled, matchAll); } private Map> parseHeaders(final Node root) throws RepositoryException { diff --git a/common/src/main/java/org/onehippo/forge/ipfilter/common/IpFilterConstants.java b/common/src/main/java/org/onehippo/forge/ipfilter/common/IpFilterConstants.java index 1553525..7f84908 100644 --- a/common/src/main/java/org/onehippo/forge/ipfilter/common/IpFilterConstants.java +++ b/common/src/main/java/org/onehippo/forge/ipfilter/common/IpFilterConstants.java @@ -28,6 +28,7 @@ public final class IpFilterConstants { public static final String CONFIG_CACHE_ENABLED = "cache-enabled"; public static final String CONFIG_FORWARDED_FOR_HEADER = "forwarded-for-header"; public static final String CONFIG_FORWARDED_HOST_HEADER = "forwarded-host-header"; + public static final String CONFIG_TRUSTED_PROXIES = "trusted-proxies"; public static final int BASIC_AUTH_PREFIX_LENGTH = "Basic ".length(); diff --git a/common/src/main/java/org/onehippo/forge/ipfilter/common/IpFilterUtils.java b/common/src/main/java/org/onehippo/forge/ipfilter/common/IpFilterUtils.java index 8f57dad..cb7efe5 100644 --- a/common/src/main/java/org/onehippo/forge/ipfilter/common/IpFilterUtils.java +++ b/common/src/main/java/org/onehippo/forge/ipfilter/common/IpFilterUtils.java @@ -25,6 +25,7 @@ import jakarta.servlet.http.HttpServletResponse; import java.io.IOException; import java.util.Iterator; +import java.util.Set; public final class IpFilterUtils { @@ -47,19 +48,38 @@ public static String getParameter(final FilterConfig filterConfig, final String } - public static String getIp(HttpServletRequest request, final String name) { + public static String getIp(final HttpServletRequest request, final String name, final Set trustedProxies) { + final String remoteAddr = request.getRemoteAddr(); final String headerName = Strings.isNullOrEmpty(name) ? IpFilterConstants.HEADER_X_FORWARDED_FOR : name; final String header = request.getHeader(headerName); if (Strings.isNullOrEmpty(header)) { log.debug("Header: {} was empty", headerName); - return request.getRemoteAddr(); + return remoteAddr; + } + if (trustedProxies.isEmpty()) { + log.warn("Header '{}' is present but no trusted-proxies configured; using remote address to prevent IP spoofing. " + + "Configure '{}' to enable X-Forwarded-For support.", headerName, IpFilterConstants.CONFIG_TRUSTED_PROXIES); + return remoteAddr; + } + if (!isTrustedProxy(remoteAddr, trustedProxies)) { + log.debug("Remote address '{}' is not a trusted proxy; ignoring header '{}'", remoteAddr, headerName); + return remoteAddr; } final Iterable ipAddresses = COMMA_SPLITTER.split(header); final Iterator iterator = ipAddresses.iterator(); if (iterator.hasNext()) { return iterator.next(); } - return request.getRemoteAddr(); + return remoteAddr; + } + + private static boolean isTrustedProxy(final String remoteAddr, final Set trustedProxies) { + for (final IpMatcher matcher : trustedProxies) { + if (matcher.matches(remoteAddr)) { + return true; + } + } + return false; } diff --git a/common/src/test/java/org/onehippo/forge/ipfilter/common/IpFilterUtilsTest.java b/common/src/test/java/org/onehippo/forge/ipfilter/common/IpFilterUtilsTest.java index 6afe4ba..45e6f4d 100644 --- a/common/src/test/java/org/onehippo/forge/ipfilter/common/IpFilterUtilsTest.java +++ b/common/src/test/java/org/onehippo/forge/ipfilter/common/IpFilterUtilsTest.java @@ -25,26 +25,100 @@ import static org.easymock.EasyMock.*; import static org.junit.Assert.*; + + public class IpFilterUtilsTest { @Test public void testGetIp() { final Set E = Collections.emptySet(); - final AuthObject object = new AuthObject(E, E, E, Collections.emptyMap(), true, null,true, true); + final AuthObject object = new AuthObject(E, E, E, Collections.emptyMap(), true, null, Collections.emptySet(), true, true); HttpServletRequest request = createMock(HttpServletRequest.class); expect(request.getRemoteAddr()).andReturn("127.0.0.1").anyTimes(); expect(request.getHeader(IpFilterConstants.HEADER_X_FORWARDED_FOR)).andReturn(null).anyTimes(); replay(request); - String header = IpFilterUtils.getIp(request, object.getForwardedForHeader()); + String header = IpFilterUtils.getIp(request, object.getForwardedForHeader(), object.getTrustedProxyMatchers()); assertEquals("127.0.0.1", header); - // recreate + // recreate: XFF present but no trusted proxies configured -> remoteAddr is returned (spoofing prevention) request = createMock(HttpServletRequest.class); + expect(request.getRemoteAddr()).andReturn("127.0.0.1").anyTimes(); expect(request.getHeader(IpFilterConstants.HEADER_X_FORWARDED_FOR)).andReturn("localhost").anyTimes(); replay(request); - header = IpFilterUtils.getIp(request, object.getForwardedForHeader()); - assertEquals("localhost", header); + header = IpFilterUtils.getIp(request, object.getForwardedForHeader(), object.getTrustedProxyMatchers()); + assertEquals("127.0.0.1", header); + } + + @Test + public void testGetIpXffIgnoredWithNoTrustedProxies() { + // XFF present, no trusted proxies configured -> remoteAddr returned to prevent spoofing + final HttpServletRequest request = createMock(HttpServletRequest.class); + expect(request.getRemoteAddr()).andReturn("192.0.2.100").anyTimes(); + expect(request.getHeader(IpFilterConstants.HEADER_X_FORWARDED_FOR)).andReturn("203.0.113.50").anyTimes(); + replay(request); + + final String ip = IpFilterUtils.getIp(request, IpFilterConstants.HEADER_X_FORWARDED_FOR, Collections.emptySet()); + assertEquals("192.0.2.100", ip); + } + + @Test + public void testGetIpXffTrustedWhenRemoteAddrIsTrustedProxy() { + // XFF present, remoteAddr is a configured trusted proxy -> XFF IP returned + final IpMatcher proxyMatcher = IpMatcher.valueOf("10.0.0.1"); + final Set trustedProxies = Collections.singleton(proxyMatcher); + + final HttpServletRequest request = createMock(HttpServletRequest.class); + expect(request.getRemoteAddr()).andReturn("10.0.0.1").anyTimes(); + expect(request.getHeader(IpFilterConstants.HEADER_X_FORWARDED_FOR)).andReturn("203.0.113.50").anyTimes(); + replay(request); + + final String ip = IpFilterUtils.getIp(request, IpFilterConstants.HEADER_X_FORWARDED_FOR, trustedProxies); + assertEquals("203.0.113.50", ip); + } + + @Test + public void testGetIpXffIgnoredWhenRemoteAddrIsNotTrustedProxy() { + // XFF present, remoteAddr is NOT a trusted proxy -> remoteAddr returned (prevent spoofing) + final IpMatcher proxyMatcher = IpMatcher.valueOf("10.0.0.1"); + final Set trustedProxies = Collections.singleton(proxyMatcher); + + final HttpServletRequest request = createMock(HttpServletRequest.class); + expect(request.getRemoteAddr()).andReturn("192.0.2.100").anyTimes(); + expect(request.getHeader(IpFilterConstants.HEADER_X_FORWARDED_FOR)).andReturn("203.0.113.50").anyTimes(); + replay(request); + + final String ip = IpFilterUtils.getIp(request, IpFilterConstants.HEADER_X_FORWARDED_FOR, trustedProxies); + assertEquals("192.0.2.100", ip); + } + + @Test + public void testGetIpXffTrustedWhenRemoteAddrMatchesTrustedProxyCidrRange() { + // XFF present, remoteAddr falls within a trusted proxy CIDR range -> XFF IP returned + final IpMatcher cidrMatcher = IpMatcher.valueOf("10.0.0.0/24"); + final Set trustedProxies = Collections.singleton(cidrMatcher); + + final HttpServletRequest request = createMock(HttpServletRequest.class); + expect(request.getRemoteAddr()).andReturn("10.0.0.5").anyTimes(); + expect(request.getHeader(IpFilterConstants.HEADER_X_FORWARDED_FOR)).andReturn("203.0.113.50").anyTimes(); + replay(request); + + final String ip = IpFilterUtils.getIp(request, IpFilterConstants.HEADER_X_FORWARDED_FOR, trustedProxies); + assertEquals("203.0.113.50", ip); + } + + @Test + public void testGetIpXffIgnoredWhenRemoteAddrOutsideTrustedProxyCidrRange() { + // XFF present, remoteAddr outside the trusted proxy CIDR range -> remoteAddr returned + final IpMatcher cidrMatcher = IpMatcher.valueOf("10.0.0.0/24"); + final Set trustedProxies = Collections.singleton(cidrMatcher); + + final HttpServletRequest request = createMock(HttpServletRequest.class); + expect(request.getRemoteAddr()).andReturn("10.0.1.1").anyTimes(); + expect(request.getHeader(IpFilterConstants.HEADER_X_FORWARDED_FOR)).andReturn("203.0.113.50").anyTimes(); + replay(request); + final String ip = IpFilterUtils.getIp(request, IpFilterConstants.HEADER_X_FORWARDED_FOR, trustedProxies); + assertEquals("10.0.1.1", ip); } @Test diff --git a/demo/pom.xml b/demo/pom.xml index 05dcd5e..bee4dad 100644 --- a/demo/pom.xml +++ b/demo/pom.xml @@ -60,7 +60,7 @@ - 5.0.2-SNAPSHOT + 5.0.3-SNAPSHOT shared/lib ${project.basedir}/target/postgres-data:/var/lib/postgresql/data diff --git a/demo/repository-data/application/src/main/resources/hcm-config/configuration/modules/ipfilter.yaml b/demo/repository-data/application/src/main/resources/hcm-config/configuration/modules/ipfilter.yaml index ff4cce0..4efee68 100644 --- a/demo/repository-data/application/src/main/resources/hcm-config/configuration/modules/ipfilter.yaml +++ b/demo/repository-data/application/src/main/resources/hcm-config/configuration/modules/ipfilter.yaml @@ -3,5 +3,7 @@ definitions: /hippo:configuration/hippo:modules/ipfilter/hippo:moduleconfig: forwarded-host-header: [X-Forwarded-Host-2] /hippo:configuration/hippo:modules/ipfilter/hippo:moduleconfig/localhost: - match-all: true - cache-enabled: false \ No newline at end of file + hostnames: [.*] + match-all: false + cache-enabled: false + allowed-ip-ranges: [10.0.0.100, 203.0.113.50, 127.0.0.1]