diff --git a/docker-compose.yml b/docker-compose.yml index 44f17fa..43d8e91 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -44,6 +44,12 @@ services: FRONTEND_BASEURL: ${FRONTEND_BASEURL} MUSALLAHBOARD_BASEURL: ${MUSALLAHBOARD_BASEURL} SPRINGDOC_API_DOCS_ENABLED: ${SPRINGDOC_API_DOCS_ENABLED:-false} + # Only nginx can reach this container (it is not port-published), so the compose bridge + # network is the trusted-proxy range. Without this the app ignores X-Forwarded-For and + # rate-limits every caller under nginx's single address. See lensbridge.client-ip.* in + # application.properties.example. + LENSBRIDGE_CLIENTIP_TRUSTEDPROXIES: ${CLIENT_IP_TRUSTED_PROXIES:-172.16.0.0/12} + LENSBRIDGE_CLIENTIP_TRUSTEDPROXYCOUNT: ${CLIENT_IP_TRUSTED_PROXY_COUNT:-1} volumes: - thumbnails:/app/thumbnails networks: diff --git a/docker/nginx/nginx.conf b/docker/nginx/nginx.conf index dff1b4c..a520009 100644 --- a/docker/nginx/nginx.conf +++ b/docker/nginx/nginx.conf @@ -93,7 +93,10 @@ http { proxy_http_version 1.1; proxy_set_header Host $host; proxy_set_header X-Real-IP $remote_addr; - proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; + # Overwrite rather than append ($proxy_add_x_forwarded_for): the client's own + # X-Forwarded-For must never survive to the app, or it can pre-seed the header + # and forge its own identity for per-IP rate limiting and audit records. + proxy_set_header X-Forwarded-For $remote_addr; proxy_set_header X-Forwarded-Proto $scheme; proxy_set_header X-Forwarded-Host $host; proxy_set_header X-Forwarded-Port $server_port; diff --git a/src/main/java/com/ibrasoft/lensbridge/config/ClientIpProperties.java b/src/main/java/com/ibrasoft/lensbridge/config/ClientIpProperties.java new file mode 100644 index 0000000..846c56e --- /dev/null +++ b/src/main/java/com/ibrasoft/lensbridge/config/ClientIpProperties.java @@ -0,0 +1,39 @@ +package com.ibrasoft.lensbridge.config; + +import lombok.Data; +import org.springframework.boot.context.properties.ConfigurationProperties; +import org.springframework.stereotype.Component; + +import java.util.ArrayList; +import java.util.List; + +/** + * Controls how {@link com.ibrasoft.lensbridge.security.ClientIpResolver} derives the originating + * client IP from an inbound request. + * + *

Defaults are deliberately fail-safe: with {@link #trustedProxies} empty, no + * {@code X-Forwarded-For} header is honoured at all and every caller is identified by its real TCP + * peer address. A deployment that actually sits behind a reverse proxy has to opt in by listing the + * proxy's address range, which means a forgotten configuration degrades to over-counting (one shared + * bucket) rather than to a trivially spoofable rate-limit key. + */ +@Component +@ConfigurationProperties(prefix = "lensbridge.client-ip") +@Data +public class ClientIpProperties { + + /** + * CIDR blocks (or bare IP literals) of reverse proxies that are allowed to supply + * {@code X-Forwarded-For}. Requests arriving from any other peer have the header ignored. + * Empty (the default) means no proxy is trusted. + */ + private List trustedProxies = new ArrayList<>(); + + /** + * Number of reverse proxies we operate between the public internet and this application. + * The client IP is read that many entries from the right-hand end of {@code X-Forwarded-For}, + * so hops a client prepends itself are skipped. {@code 1} matches the single nginx in + * {@code docker/nginx/nginx.conf}. {@code 0} disables header parsing entirely. + */ + private int trustedProxyCount = 1; +} diff --git a/src/main/java/com/ibrasoft/lensbridge/config/RateLimitingFilter.java b/src/main/java/com/ibrasoft/lensbridge/config/RateLimitingFilter.java index c96d756..5c6ff4c 100644 --- a/src/main/java/com/ibrasoft/lensbridge/config/RateLimitingFilter.java +++ b/src/main/java/com/ibrasoft/lensbridge/config/RateLimitingFilter.java @@ -3,10 +3,12 @@ import com.github.benmanes.caffeine.cache.Cache; import com.github.benmanes.caffeine.cache.Caffeine; import com.ibrasoft.lensbridge.model.auth.Role; +import com.ibrasoft.lensbridge.security.ClientIpResolver; import io.github.bucket4j.Bandwidth; import io.github.bucket4j.Bucket; import io.github.bucket4j.ConsumptionProbe; +import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.beans.factory.annotation.Value; import org.springframework.http.HttpStatus; @@ -27,8 +29,11 @@ @Slf4j @Component +@RequiredArgsConstructor public class RateLimitingFilter extends OncePerRequestFilter { + private final ClientIpResolver clientIpResolver; + @Value("${ratelimit.requests:10}") private int maxRequests; @@ -53,7 +58,7 @@ protected void doFilterInternal(HttpServletRequest request, filterChain.doFilter(request, response); return; } - String ip = getClientIp(request); + String ip = clientIpResolver.resolveClientIp(request); Bucket bucket = bucketCache.get(ip, k -> newBucket()); ConsumptionProbe probe = bucket.tryConsumeAndReturnRemaining(1); @@ -74,14 +79,6 @@ private Bucket newBucket() { .build(); } - private String getClientIp(HttpServletRequest request) { - String xfHeader = request.getHeader("X-Forwarded-For"); - if (xfHeader != null && !xfHeader.isEmpty()) { - return xfHeader.split(",")[0].trim(); - } - return request.getRemoteAddr(); - } - private boolean isUserExemptFromRateLimit() { Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); diff --git a/src/main/java/com/ibrasoft/lensbridge/controller/AgentEnrollmentController.java b/src/main/java/com/ibrasoft/lensbridge/controller/AgentEnrollmentController.java index f51708f..bacecf8 100644 --- a/src/main/java/com/ibrasoft/lensbridge/controller/AgentEnrollmentController.java +++ b/src/main/java/com/ibrasoft/lensbridge/controller/AgentEnrollmentController.java @@ -4,6 +4,7 @@ import com.ibrasoft.lensbridge.dto.board.response.AgentEnrollResponse; import com.ibrasoft.lensbridge.dto.auth.response.MessageResponse; import com.ibrasoft.lensbridge.exception.ApiResponseException; +import com.ibrasoft.lensbridge.security.ClientIpResolver; import com.ibrasoft.lensbridge.service.agent.DeviceEnrollmentService; import com.ibrasoft.lensbridge.service.agent.DeviceEnrollmentService.Outcome; import io.swagger.v3.oas.annotations.Operation; @@ -36,6 +37,7 @@ public class AgentEnrollmentController { private final DeviceEnrollmentService enrollmentService; + private final ClientIpResolver clientIpResolver; @Value("${musallahboard.agent.websocketUrl}") private String websocketUrl; @@ -54,7 +56,7 @@ public class AgentEnrollmentController { @PostMapping("/enroll") public ResponseEntity enroll(@Valid @RequestBody AgentEnrollRequest request, HttpServletRequest httpRequest) { - String remoteIp = resolveClientIp(httpRequest); + String remoteIp = clientIpResolver.resolveClientIp(httpRequest); Outcome outcome = enrollmentService.enroll( request.getToken(), @@ -76,13 +78,4 @@ public ResponseEntity enroll(@Valid @RequestBody AgentEnrol new MessageResponse("Enrollment token is invalid, expired, or already used")); }; } - - private static String resolveClientIp(HttpServletRequest request) { - String forwarded = request.getHeader("X-Forwarded-For"); - if (forwarded != null && !forwarded.isBlank()) { - int comma = forwarded.indexOf(','); - return (comma > 0 ? forwarded.substring(0, comma) : forwarded).trim(); - } - return request.getRemoteAddr(); - } } diff --git a/src/main/java/com/ibrasoft/lensbridge/security/ClientIpResolver.java b/src/main/java/com/ibrasoft/lensbridge/security/ClientIpResolver.java new file mode 100644 index 0000000..1da8d41 --- /dev/null +++ b/src/main/java/com/ibrasoft/lensbridge/security/ClientIpResolver.java @@ -0,0 +1,179 @@ +package com.ibrasoft.lensbridge.security; + +import com.ibrasoft.lensbridge.config.ClientIpProperties; +import jakarta.annotation.PostConstruct; +import jakarta.servlet.http.HttpServletRequest; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.springframework.security.web.util.matcher.IpAddressMatcher; +import org.springframework.stereotype.Component; + +import java.util.ArrayList; +import java.util.List; +import java.util.regex.Pattern; + +/** + * Single source of truth for "which IP is this request really from". + * + *

{@code X-Forwarded-For} is a client-supplied header. nginx's {@code $proxy_add_x_forwarded_for} + * appends the peer address to whatever the client already sent, so the left-hand entries are + * entirely attacker-controlled and only the right-hand end is trustworthy. Reading + * {@code split(",")[0]} therefore lets any caller mint a fresh identity per request, which defeats + * every per-IP control built on top of it (rate limiting, lockout counters, audit trails). + * + *

This resolver instead: + *

    + *
  1. ignores the header outright unless the request's real TCP peer is a configured trusted + * proxy ({@code lensbridge.client-ip.trusted-proxies}), and
    2. + *
  2. counts from the right, skipping {@code lensbridge.client-ip.trusted-proxy-count} + * hops, so entries a client prepended are never selected, and
    3. + *
  3. falls back to {@link HttpServletRequest#getRemoteAddr()} whenever the header is absent, + * shorter than expected, or does not hold a syntactically valid IP literal.
    4. + *
+ * + *

Injected wherever a caller identity is needed; do not re-implement header parsing locally. + */ +@Slf4j +@Component +@RequiredArgsConstructor +public class ClientIpResolver { + + static final String FORWARDED_FOR_HEADER = "X-Forwarded-For"; + + /** Used only when the container cannot report a peer address at all. */ + static final String UNKNOWN_IP = "unknown"; + + private static final Pattern IPV4 = Pattern.compile( + "^(25[0-5]|2[0-4]\\d|1\\d\\d|[1-9]?\\d)(\\.(25[0-5]|2[0-4]\\d|1\\d\\d|[1-9]?\\d)){3}$"); + + /** Longest possible textual IPv6 address (IPv4-mapped form), used as a cheap sanity bound. */ + private static final int MAX_IP_LITERAL_LENGTH = 45; + + private final ClientIpProperties properties; + + private List trustedProxyMatchers = List.of(); + + @PostConstruct + void initialiseTrustedProxies() { + List matchers = new ArrayList<>(); + for (String cidr : properties.getTrustedProxies()) { + if (cidr == null || cidr.isBlank()) { + continue; + } + try { + matchers.add(new IpAddressMatcher(cidr.trim())); + } catch (IllegalArgumentException e) { + throw new IllegalStateException( + "lensbridge.client-ip.trusted-proxies contains an invalid CIDR/IP: '" + cidr + "'", e); + } + } + this.trustedProxyMatchers = List.copyOf(matchers); + + if (trustedProxyMatchers.isEmpty()) { + log.info("Client IP resolution: no trusted proxies configured; {} will be ignored and the " + + "TCP peer address used instead.", FORWARDED_FOR_HEADER); + } else { + log.info("Client IP resolution: trusting {} for requests from {}, reading hop {} from the right.", + FORWARDED_FOR_HEADER, properties.getTrustedProxies(), properties.getTrustedProxyCount()); + } + } + + /** + * Resolves the originating client IP for {@code request}. + * + * @return a syntactically valid IP literal, or {@value #UNKNOWN_IP} if the container reported no + * peer address. Never {@code null}, so it is safe to use directly as a cache/lookup key. + */ + public String resolveClientIp(HttpServletRequest request) { + String remoteAddr = request.getRemoteAddr(); + if (remoteAddr == null || remoteAddr.isBlank()) { + return UNKNOWN_IP; + } + + int hopsToSkip = properties.getTrustedProxyCount(); + if (hopsToSkip <= 0 || trustedProxyMatchers.isEmpty() || !isTrustedProxy(remoteAddr)) { + // Either header parsing is disabled, or this request did not come through a proxy we + // operate, so nothing it claims about its own origin can be believed. + return remoteAddr; + } + + String header = request.getHeader(FORWARDED_FOR_HEADER); + if (header == null || header.isBlank()) { + return remoteAddr; + } + + List hops = new ArrayList<>(); + for (String hop : header.split(",")) { + String trimmed = hop.trim(); + if (!trimmed.isEmpty()) { + hops.add(trimmed); + } + } + + int index = hops.size() - hopsToSkip; + if (index < 0) { + log.debug("{} has {} usable entries but {} trusted hops are configured; falling back to peer address.", + FORWARDED_FOR_HEADER, hops.size(), hopsToSkip); + return remoteAddr; + } + + String candidate = stripPort(hops.get(index)); + if (!isIpLiteral(candidate)) { + log.debug("{} hop {} is not a valid IP literal; falling back to peer address.", + FORWARDED_FOR_HEADER, index); + return remoteAddr; + } + return candidate; + } + + private boolean isTrustedProxy(String address) { + for (IpAddressMatcher matcher : trustedProxyMatchers) { + try { + if (matcher.matches(address)) { + return true; + } + } catch (IllegalArgumentException e) { + // Address family mismatch or an unparseable peer address: simply not a match. + log.trace("Trusted-proxy matcher rejected peer address '{}'", address); + } + } + return false; + } + + /** + * Removes a {@code :port} suffix and IPv6 brackets. Proxies occasionally emit {@code 1.2.3.4:5678} + * or {@code [2001:db8::1]:443}; a bare IPv6 address (which is full of colons) is left untouched. + */ + private static String stripPort(String value) { + if (value.startsWith("[")) { + int close = value.indexOf(']'); + return close > 0 ? value.substring(1, close) : value; + } + int colon = value.indexOf(':'); + if (colon > 0 && value.indexOf(':', colon + 1) < 0) { + return value.substring(0, colon); + } + return value; + } + + /** + * Lexical-only IP check. Deliberately avoids {@code InetAddress.getByName}, which would perform a + * DNS lookup for anything that is not an IP literal — an attacker-triggered outbound request. + */ + private static boolean isIpLiteral(String value) { + if (value.isEmpty() || value.length() > MAX_IP_LITERAL_LENGTH) { + return false; + } + if (value.indexOf(':') < 0) { + return IPV4.matcher(value).matches(); + } + // IPv6: hex groups separated by colons, optionally ending in a dotted-quad tail. + for (int i = 0; i < value.length(); i++) { + char c = value.charAt(i); + if (Character.digit(c, 16) < 0 && c != ':' && c != '.') { + return false; + } + } + return true; + } +} diff --git a/src/main/resources/application.properties.example b/src/main/resources/application.properties.example index 165d227..b9f6784 100644 --- a/src/main/resources/application.properties.example +++ b/src/main/resources/application.properties.example @@ -76,6 +76,19 @@ ratelimit.requests=100 ratelimit.duration.minutes=1 ratelimit.exempt-roles=ROLE_ROOT +# Client IP resolution (used for rate-limit buckets, enrollment lastSeenIp, audit records). +# +# X-Forwarded-For is client-supplied. It is honoured ONLY when the request's real TCP peer matches +# one of the CIDRs below, and even then the client IP is read `trusted-proxy-count` entries from the +# RIGHT-hand end so hops the client prepended are skipped. Leave trusted-proxies empty (the default) +# when the app is exposed directly -- the header is then ignored entirely and the peer address wins. +# +# Behind the docker-compose nginx, set this to the compose bridge network, e.g.: +# lensbridge.client-ip.trusted-proxies=172.16.0.0/12 +# The app container is not port-published, so only nginx can reach it on that network. +lensbridge.client-ip.trusted-proxies= +lensbridge.client-ip.trusted-proxy-count=1 + # Spring multipart cap spring.servlet.multipart.max-file-size=250MB spring.servlet.multipart.max-request-size=5000MB diff --git a/src/test/java/com/ibrasoft/lensbridge/config/RateLimitingFilterTest.java b/src/test/java/com/ibrasoft/lensbridge/config/RateLimitingFilterTest.java index 14026b3..d26aac7 100644 --- a/src/test/java/com/ibrasoft/lensbridge/config/RateLimitingFilterTest.java +++ b/src/test/java/com/ibrasoft/lensbridge/config/RateLimitingFilterTest.java @@ -1,6 +1,7 @@ package com.ibrasoft.lensbridge.config; import com.ibrasoft.lensbridge.model.auth.Role; +import com.ibrasoft.lensbridge.security.ClientIpResolver; import jakarta.servlet.FilterChain; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; @@ -32,8 +33,11 @@ *

  • The {@code @Value} default for {@code ratelimit.requests} is 10, not 100. * Tests set the limit explicitly via reflection so they don't depend on the default.
    • *
  • Exempt-role detection reads from the {@link SecurityContextHolder}, not the request.
    • - *
  • Per-client isolation keys on {@code X-Forwarded-For} (first hop) falling back to - * {@code remoteAddr}.
    • + *
  • Per-client isolation keys on the client IP as resolved by {@link ClientIpResolver}: + * the forwarded header is honoured only for peers inside the configured trusted-proxy + * range, and otherwise ignored in favour of {@code remoteAddr}. These tests configure + * {@code 10.0.0.0/8} as the trusted range, so {@code 10.x} peers are treated as our + * proxy and everything else as a direct caller.
    • *
  • On a successful pass-through an {@code X-Rate-Limit-Remaining} header is added; on a * block the status is set to 429 and a plain-text body is written (chain not invoked).
    • * @@ -45,7 +49,13 @@ class RateLimitingFilterTest { @BeforeEach void setUp() { - filter = new RateLimitingFilter(); + ClientIpProperties clientIpProperties = new ClientIpProperties(); + clientIpProperties.setTrustedProxies(List.of("10.0.0.0/8")); + clientIpProperties.setTrustedProxyCount(1); + ClientIpResolver clientIpResolver = new ClientIpResolver(clientIpProperties); + ReflectionTestUtils.invokeMethod(clientIpResolver, "initialiseTrustedProxies"); + + filter = new RateLimitingFilter(clientIpResolver); ReflectionTestUtils.setField(filter, "maxRequests", 3); ReflectionTestUtils.setField(filter, "durationMinutes", 1); ReflectionTestUtils.setField(filter, "exemptRoles", List.of(Role.ROOT)); @@ -165,25 +175,60 @@ void bucketsAreIsolatedPerClientIp() throws Exception { } @Test - void clientIpIsTakenFromXForwardedForFirstHop() throws Exception { + void forwardedHeaderFromATrustedProxyIdentifiesTheClient() throws Exception { FilterChain chain = mock(FilterChain.class); - // Two requests with different remoteAddr but the same X-Forwarded-For client - // share a bucket, proving the forwarded header is the bucket key. + // 10.0.0.99 is inside the configured trusted range, so it is our proxy and the entry it + // appended -- the last one -- is the real client. Two requests arriving through different + // proxy instances for the same client must therefore share a bucket. for (int i = 0; i < 3; i++) { MockHttpServletRequest request = requestFromIp("10.0.0.99"); - request.addHeader("X-Forwarded-For", "203.0.113.7, 10.0.0.99"); + request.addHeader("X-Forwarded-For", "203.0.113.7"); filter.doFilter(request, new MockHttpServletResponse(), chain); } MockHttpServletRequest blockedReq = requestFromIp("10.0.0.100"); - blockedReq.addHeader("X-Forwarded-For", "203.0.113.7, 10.0.0.100"); + blockedReq.addHeader("X-Forwarded-For", "203.0.113.7"); + MockHttpServletResponse blocked = new MockHttpServletResponse(); + filter.doFilter(blockedReq, blocked, chain); + + assertThat(blocked.getStatus()).isEqualTo(HttpStatus.TOO_MANY_REQUESTS.value()); + } + + @Test + void spoofedForwardedHeaderFromAnUntrustedPeerCannotEvadeTheBucket() throws Exception { + FilterChain chain = mock(FilterChain.class); + + // The whole point of the fix. 198.51.100.5 is a direct caller, not a proxy we operate, so + // nothing it claims about its own origin is believed. Rotating a fresh forged header on + // every request used to mint a fresh bucket each time and made the limiter a no-op. + for (int i = 0; i < 3; i++) { + MockHttpServletRequest request = requestFromIp("198.51.100.5"); + request.addHeader("X-Forwarded-For", "203.0.113." + i); + filter.doFilter(request, new MockHttpServletResponse(), chain); + } + + MockHttpServletRequest blockedReq = requestFromIp("198.51.100.5"); + blockedReq.addHeader("X-Forwarded-For", "203.0.113.250"); MockHttpServletResponse blocked = new MockHttpServletResponse(); filter.doFilter(blockedReq, blocked, chain); assertThat(blocked.getStatus()).isEqualTo(HttpStatus.TOO_MANY_REQUESTS.value()); } + @Test + void trustedProxyWithoutAForwardedHeaderFallsBackToThePeerAddress() throws Exception { + FilterChain chain = mock(FilterChain.class); + + for (int i = 0; i < 3; i++) { + filter.doFilter(requestFromIp("10.1.2.3"), new MockHttpServletResponse(), chain); + } + MockHttpServletResponse blocked = new MockHttpServletResponse(); + filter.doFilter(requestFromIp("10.1.2.3"), blocked, chain); + + assertThat(blocked.getStatus()).isEqualTo(HttpStatus.TOO_MANY_REQUESTS.value()); + } + @Test void unauthenticatedRequestIsNotExempt() throws Exception { // No authentication in the security context. diff --git a/src/test/java/com/ibrasoft/lensbridge/controller/MethodSecurityTestConfig.java b/src/test/java/com/ibrasoft/lensbridge/controller/MethodSecurityTestConfig.java index 2d34cbc..00f0c82 100644 --- a/src/test/java/com/ibrasoft/lensbridge/controller/MethodSecurityTestConfig.java +++ b/src/test/java/com/ibrasoft/lensbridge/controller/MethodSecurityTestConfig.java @@ -1,5 +1,7 @@ package com.ibrasoft.lensbridge.controller; +import com.ibrasoft.lensbridge.config.ClientIpProperties; +import com.ibrasoft.lensbridge.security.ClientIpResolver; import org.springframework.boot.test.context.TestConfiguration; import org.springframework.context.annotation.Bean; import org.springframework.http.HttpStatus; @@ -18,11 +20,26 @@ * enforced (yielding 403 for wrong roles), and a filter chain that requires authentication for all * requests (yielding 401 for anonymous, matching the production entry-point behaviour). CSRF is * disabled, mirroring production. + * + *

    It also supplies {@link ClientIpResolver}. {@code @WebMvcTest} picks up + * {@link com.ibrasoft.lensbridge.config.RateLimitingFilter} because it is a servlet filter, + * but not the resolver it depends on, which lives outside the slice. Without this bean every + * importer of this config fails to start its context. No trusted proxies are configured, so + * the resolver ignores {@code X-Forwarded-For} entirely and keys on the peer address -- + * the safe default, and the right one for a slice test with no proxy in front of it. */ @TestConfiguration @EnableMethodSecurity class MethodSecurityTestConfig { + @Bean + ClientIpResolver testClientIpResolver() { + // @PostConstruct on the resolver is honoured by the container for @Bean instances, + // so the trusted-proxy matchers are initialised without calling the (package-private) + // initialiser from here. + return new ClientIpResolver(new ClientIpProperties()); + } + @Bean SecurityFilterChain testFilterChain(HttpSecurity http) throws Exception { http.csrf(csrf -> csrf.disable())