jerryshao commented on code in PR #14024: URL: https://github.com/apache/gravitino/pull/14024#discussion_r4332386234
########## server-common/src/main/java/org/apache/gravitino/server/web/ClientAddressResolver.java: ########## @@ -0,0 +1,291 @@ +/* + * 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.gravitino.server.web; + +import com.google.common.collect.ImmutableList; +import com.google.common.net.InetAddresses; +import java.net.Inet4Address; +import java.net.InetAddress; +import java.util.Arrays; +import java.util.Collections; +import java.util.Enumeration; +import java.util.List; +import java.util.Optional; +import javax.annotation.Nullable; +import javax.servlet.http.HttpServletRequest; +import org.apache.commons.lang3.StringUtils; + +/** + * Resolves the address of the client that sent an HTTP request, honoring {@code X-Forwarded-For} + * only when the request reached the server through a configured trusted proxy. + * + * <p>Each trusted proxy is an IP literal or a CIDR block, IPv4 or IPv6. Entries are parsed with + * {@link InetAddresses#forString(String)}, which never performs a DNS lookup. IPv4 and IPv4-mapped + * IPv6 forms of the same address match each other. + * + * <p>The resolution rules are: + * + * <ol> + * <li>If the TCP peer ({@link HttpServletRequest#getRemoteAddr()}) is not a trusted proxy, return + * it and ignore {@code X-Forwarded-For} entirely: any caller can set that header, so it says + * nothing about who the caller is unless a proxy we trust wrote it. + * <li>Otherwise combine every {@code X-Forwarded-For} header in the order received and walk the + * entries from right to left, skipping trusted proxies. The first untrusted entry is the + * client. Entries left of it were supplied by the client and are not trusted. + * <li>If every entry is a trusted proxy, return the left-most one. + * <li>If an entry is malformed (not an IP literal, or {@code unknown}), stop and return the last + * trusted hop seen so far, which may be the TCP peer itself. + * </ol> + * + * <p>Ports and IPv6 brackets ({@code 203.0.113.5:4711}, {@code [2001:db8::1]:4711}) are stripped + * from {@code X-Forwarded-For} entries. Every returned address, the TCP peer included, is in + * canonical form: IPv6 compressed as in RFC 5952 and without brackets or a zone ID (Jetty reports + * {@code [0:0:0:0:0:0:0:1]}, this returns {@code ::1}), and an IPv4-mapped IPv6 address as plain + * IPv4. A peer address that is not an IP literal is returned unchanged. + * + * <p>With no trusted proxies configured (the default), {@code X-Forwarded-For} is never honored and + * the TCP peer address is always returned. + */ +public final class ClientAddressResolver { + + /** The name of the de-facto standard header that reverse proxies append the client address to. */ + public static final String X_FORWARDED_FOR = "X-Forwarded-For"; + + private static final String UNKNOWN = "unknown"; + private static final int IPV6_BITS = 128; + private static final int IPV4_BITS = 32; + private static final int IPV4_MAPPED_PREFIX_BITS = IPV6_BITS - IPV4_BITS; + + private static final ClientAddressResolver NO_TRUSTED_PROXIES = + new ClientAddressResolver(Collections.emptyList()); + + private final List<IpRange> trustedProxies; + + /** + * Constructs a resolver that trusts the given proxies. + * + * @param trustedProxies IP literals or CIDR blocks, for example {@code 10.0.0.1}, {@code + * 10.0.0.0/8} or {@code fd00::/8}; blank entries are ignored. + * @throws IllegalArgumentException if an entry is not a valid IP literal or CIDR block. + */ + public ClientAddressResolver(List<String> trustedProxies) { + ImmutableList.Builder<IpRange> ranges = ImmutableList.builder(); + for (String proxy : trustedProxies) { + if (StringUtils.isNotBlank(proxy)) { + ranges.add(IpRange.parse(proxy.trim())); + } + } + this.trustedProxies = ranges.build(); + } + + /** + * Returns a resolver with no trusted proxies, which always returns the TCP peer address and never + * honors {@code X-Forwarded-For}. + * + * @return a resolver with no trusted proxies + */ + public static ClientAddressResolver noTrustedProxies() { + return NO_TRUSTED_PROXIES; + } + + /** + * Resolves the client address of the given request. See the class doc for the rules. + * + * @param request the HTTP request + * @return the client address in canonical form; the TCP peer address when {@code X-Forwarded-For} + * is not honored; {@code null} only when the container reports no peer address and {@code + * X-Forwarded-For} is not honored + */ + @Nullable + public String resolve(HttpServletRequest request) { + String remoteAddr = request.getRemoteAddr(); + Optional<InetAddress> peer = parseAddress(remoteAddr); + String peerAddress = peer.map(InetAddresses::toAddrString).orElse(remoteAddr); + if (trustedProxies.isEmpty() || !isTrusted(peer)) { + return peerAddress; + } + + List<String> entries = forwardedForEntries(request); + String lastTrustedHop = peerAddress; + for (int i = entries.size() - 1; i >= 0; i--) { + Optional<InetAddress> address = parseForwardedEntry(entries.get(i)); + if (!address.isPresent()) { + return lastTrustedHop; + } + String canonical = InetAddresses.toAddrString(address.get()); + if (!isTrusted(address)) { + return canonical; + } + lastTrustedHop = canonical; + } + return lastTrustedHop; + } + + private boolean isTrusted(Optional<InetAddress> address) { + if (!address.isPresent()) { + return false; + } + byte[] bytes = toIpv6Bytes(address.get()); + return trustedProxies.stream().anyMatch(range -> range.contains(bytes)); + } + + private static List<String> forwardedForEntries(HttpServletRequest request) { + ImmutableList.Builder<String> entries = ImmutableList.builder(); + Enumeration<String> headers = request.getHeaders(X_FORWARDED_FOR); + while (headers != null && headers.hasMoreElements()) { + String header = headers.nextElement(); + if (header == null) { + continue; + } + for (String entry : header.split(",", -1)) { + entries.add(entry.trim()); Review Comment: [Important] Empty `X-Forwarded-For` list elements are treated as malformed and stop the right-to-left walk, instead of being skipped. `split(",", -1)` keeps empty elements, each becomes `""` after `trim()`, and `parseForwardedEntry` returns empty for `""` (line 170), which makes `resolve` return the last trusted hop (line 130). So a header the last trusted proxy emits as `203.0.113.5,` or `203.0.113.5, , 10.2.2.2` records a proxy address rather than the client. RFC 7230 section 7 says the opposite: a recipient must parse and ignore a reasonable number of empty list elements in a comma-delimited list. It fails safe (the result is a trusted hop, never a forged client), and it only bites when a hop to the right of the real client emits an empty element, so this is correctness rather than a security hole. Suggest filtering blanks out here, so only genuinely unparsable tokens such as `unknown` stop the walk: for (String entry : header.split(",")) { String trimmed = entry.trim(); if (!trimmed.isEmpty()) { entries.add(trimmed); } } `TestClientAddressResolver#testMalformedEntryStopsAtLastTrustedHop` currently pins the present behavior for the interior case (`"203.0.113.5, , 10.2.2.2"` expects `10.2.2.2`), so that assertion would need to flip to `203.0.113.5` along with the fix. Verified by: reading `forwardedForEntries` (lines 149-162), `parseForwardedEntry` (line 170) and `resolve` (lines 126-138) in full on this head, and tracing the existing test that encodes the interior-empty case. ########## iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergRequestContext.java: ########## @@ -97,14 +98,15 @@ private IcebergRequestContext( } private static String resolveClientAddress(HttpServletRequest request) { - // X-Forwarded-For is trusted unconditionally; callers in environments where the server is - // reachable directly (not only via a trusted proxy) should be aware that this header can be - // spoofed by clients. - String xForwardedFor = request.getHeader("X-Forwarded-For"); - if (StringUtils.isNotBlank(xForwardedFor)) { - return xForwardedFor.split(",")[0].trim(); + // RequestContextFilter, installed ahead of every Iceberg REST endpoint, has already resolved + // the client address with the server's trusted-proxy rules. Without it, use the TCP peer + // address, in the same canonical form: X-Forwarded-For cannot be trusted here because the + // trusted proxies are unknown. + String contextAddress = RequestContext.getRemoteAddress(); + if (contextAddress != null) { + return contextAddress; } - return request.getRemoteHost(); + return ClientAddressResolver.noTrustedProxies().resolve(request); Review Comment: [Nit] This can return `null`, and unlike every other event path that null is never normalized to `"unknown"`. `ClientAddressResolver.resolve` is `@Nullable` and returns `null` when the container reports no peer address. `IcebergEvent`, `IcebergPreEvent` and `IcebergFailureEvent` all override `remoteAddress()` to return `icebergRequestContext.remoteHostName()` directly (each at line 53 of its file), so they would surface that `null`, whereas the base `Event` constructor maps a blank address to `"unknown"` (`core/.../event/Event.java:65-66`). Reaching it needs `getRemoteAddr()` to come back null or blank, which Jetty does not do in practice, so this is housekeeping rather than a live bug — but `getRemoteHost()`, which this replaced, had the same practical guarantee, so defaulting here keeps the audit record uniform: return contextAddress != null ? contextAddress : "unknown"; applied to the resolver result as well. Verified by: `@Nullable` on `ClientAddressResolver.resolve` (line 116-117), `TestClientAddressResolver#testUnparsablePeerIsNotTrusted` which asserts the null return, and reading the three Iceberg event classes plus `Event`. ########## server-common/src/main/java/org/apache/gravitino/server/web/ClientAddressResolver.java: ########## @@ -0,0 +1,291 @@ +/* + * 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.gravitino.server.web; + +import com.google.common.collect.ImmutableList; +import com.google.common.net.InetAddresses; +import java.net.Inet4Address; +import java.net.InetAddress; +import java.util.Arrays; +import java.util.Collections; +import java.util.Enumeration; +import java.util.List; +import java.util.Optional; +import javax.annotation.Nullable; +import javax.servlet.http.HttpServletRequest; +import org.apache.commons.lang3.StringUtils; + +/** + * Resolves the address of the client that sent an HTTP request, honoring {@code X-Forwarded-For} + * only when the request reached the server through a configured trusted proxy. + * + * <p>Each trusted proxy is an IP literal or a CIDR block, IPv4 or IPv6. Entries are parsed with + * {@link InetAddresses#forString(String)}, which never performs a DNS lookup. IPv4 and IPv4-mapped + * IPv6 forms of the same address match each other. + * + * <p>The resolution rules are: + * + * <ol> + * <li>If the TCP peer ({@link HttpServletRequest#getRemoteAddr()}) is not a trusted proxy, return + * it and ignore {@code X-Forwarded-For} entirely: any caller can set that header, so it says + * nothing about who the caller is unless a proxy we trust wrote it. + * <li>Otherwise combine every {@code X-Forwarded-For} header in the order received and walk the + * entries from right to left, skipping trusted proxies. The first untrusted entry is the + * client. Entries left of it were supplied by the client and are not trusted. + * <li>If every entry is a trusted proxy, return the left-most one. + * <li>If an entry is malformed (not an IP literal, or {@code unknown}), stop and return the last + * trusted hop seen so far, which may be the TCP peer itself. + * </ol> + * + * <p>Ports and IPv6 brackets ({@code 203.0.113.5:4711}, {@code [2001:db8::1]:4711}) are stripped + * from {@code X-Forwarded-For} entries. Every returned address, the TCP peer included, is in + * canonical form: IPv6 compressed as in RFC 5952 and without brackets or a zone ID (Jetty reports + * {@code [0:0:0:0:0:0:0:1]}, this returns {@code ::1}), and an IPv4-mapped IPv6 address as plain + * IPv4. A peer address that is not an IP literal is returned unchanged. + * + * <p>With no trusted proxies configured (the default), {@code X-Forwarded-For} is never honored and + * the TCP peer address is always returned. + */ +public final class ClientAddressResolver { + + /** The name of the de-facto standard header that reverse proxies append the client address to. */ + public static final String X_FORWARDED_FOR = "X-Forwarded-For"; + + private static final String UNKNOWN = "unknown"; + private static final int IPV6_BITS = 128; + private static final int IPV4_BITS = 32; + private static final int IPV4_MAPPED_PREFIX_BITS = IPV6_BITS - IPV4_BITS; + + private static final ClientAddressResolver NO_TRUSTED_PROXIES = + new ClientAddressResolver(Collections.emptyList()); + + private final List<IpRange> trustedProxies; + + /** + * Constructs a resolver that trusts the given proxies. + * + * @param trustedProxies IP literals or CIDR blocks, for example {@code 10.0.0.1}, {@code + * 10.0.0.0/8} or {@code fd00::/8}; blank entries are ignored. + * @throws IllegalArgumentException if an entry is not a valid IP literal or CIDR block. + */ + public ClientAddressResolver(List<String> trustedProxies) { + ImmutableList.Builder<IpRange> ranges = ImmutableList.builder(); + for (String proxy : trustedProxies) { + if (StringUtils.isNotBlank(proxy)) { + ranges.add(IpRange.parse(proxy.trim())); + } + } + this.trustedProxies = ranges.build(); + } + + /** + * Returns a resolver with no trusted proxies, which always returns the TCP peer address and never + * honors {@code X-Forwarded-For}. + * + * @return a resolver with no trusted proxies + */ + public static ClientAddressResolver noTrustedProxies() { + return NO_TRUSTED_PROXIES; + } + + /** + * Resolves the client address of the given request. See the class doc for the rules. + * + * @param request the HTTP request + * @return the client address in canonical form; the TCP peer address when {@code X-Forwarded-For} + * is not honored; {@code null} only when the container reports no peer address and {@code + * X-Forwarded-For} is not honored + */ + @Nullable + public String resolve(HttpServletRequest request) { + String remoteAddr = request.getRemoteAddr(); + Optional<InetAddress> peer = parseAddress(remoteAddr); + String peerAddress = peer.map(InetAddresses::toAddrString).orElse(remoteAddr); + if (trustedProxies.isEmpty() || !isTrusted(peer)) { + return peerAddress; + } + + List<String> entries = forwardedForEntries(request); + String lastTrustedHop = peerAddress; + for (int i = entries.size() - 1; i >= 0; i--) { + Optional<InetAddress> address = parseForwardedEntry(entries.get(i)); + if (!address.isPresent()) { + return lastTrustedHop; + } + String canonical = InetAddresses.toAddrString(address.get()); + if (!isTrusted(address)) { + return canonical; + } + lastTrustedHop = canonical; + } + return lastTrustedHop; + } + + private boolean isTrusted(Optional<InetAddress> address) { + if (!address.isPresent()) { + return false; + } + byte[] bytes = toIpv6Bytes(address.get()); + return trustedProxies.stream().anyMatch(range -> range.contains(bytes)); + } + + private static List<String> forwardedForEntries(HttpServletRequest request) { + ImmutableList.Builder<String> entries = ImmutableList.builder(); + Enumeration<String> headers = request.getHeaders(X_FORWARDED_FOR); + while (headers != null && headers.hasMoreElements()) { + String header = headers.nextElement(); + if (header == null) { + continue; + } + for (String entry : header.split(",", -1)) { + entries.add(entry.trim()); + } + } + return entries.build(); + } + + /** + * Parses one {@code X-Forwarded-For} entry, stripping an optional port and IPv6 brackets. Returns + * empty for anything that is not an IP literal, including {@code unknown} and obfuscated + * identifiers. + */ + private static Optional<InetAddress> parseForwardedEntry(String entry) { + if (entry.isEmpty() || UNKNOWN.equalsIgnoreCase(entry)) { + return Optional.empty(); + } + String host; + if (entry.startsWith("[")) { + int closing = entry.indexOf(']'); + if (closing < 0) { + return Optional.empty(); + } + String rest = entry.substring(closing + 1); + if (!rest.isEmpty() && !isPortSuffix(rest)) { + return Optional.empty(); + } + host = entry.substring(1, closing); + } else if (entry.indexOf(':') >= 0 && entry.indexOf(':') == entry.lastIndexOf(':')) { + // Exactly one colon: an IPv4 address with a port. A bare IPv6 address has at least two. + int colon = entry.indexOf(':'); + if (!isPortSuffix(entry.substring(colon))) { + return Optional.empty(); + } + host = entry.substring(0, colon); + } else { + host = entry; + } + return parseAddress(host); + } + + private static boolean isPortSuffix(String suffix) { + return suffix.length() > 1 + && suffix.charAt(0) == ':' + && StringUtils.isNumeric(suffix.substring(1)); + } + + private static Optional<InetAddress> parseAddress(@Nullable String text) { + if (StringUtils.isBlank(text)) { + return Optional.empty(); + } + String host = text.trim(); + // Jetty reports an IPv6 peer in brackets, such as "[0:0:0:0:0:0:0:1]". + if (host.length() > 1 && host.charAt(0) == '[' && host.charAt(host.length() - 1) == ']') { + host = host.substring(1, host.length() - 1); + } + // Strip an IPv6 zone ID, such as "fe80::1%eth0"; it never identifies a different host. + int zone = host.indexOf('%'); + if (zone >= 0) { + host = host.substring(0, zone); + } + if (!InetAddresses.isInetAddress(host)) { + return Optional.empty(); + } + return Optional.of(InetAddresses.forString(host)); + } + + /** Returns the 16-byte IPv6 form of an address, mapping IPv4 to {@code ::ffff:a.b.c.d}. */ + private static byte[] toIpv6Bytes(InetAddress address) { + byte[] raw = address.getAddress(); + if (raw.length == 16) { + return raw; + } + byte[] mapped = new byte[16]; + mapped[10] = (byte) 0xff; + mapped[11] = (byte) 0xff; + System.arraycopy(raw, 0, mapped, 12, 4); + return mapped; + } + + /** An IP literal or CIDR block, stored in 16-byte IPv6 form so IPv4 and IPv6 compare alike. */ + private static final class IpRange { + private final byte[] network; + private final int prefixBits; + + private IpRange(byte[] network, int prefixBits) { + this.network = network; + this.prefixBits = prefixBits; + } + + static IpRange parse(String text) { + int slash = text.indexOf('/'); + String addressPart = slash >= 0 ? text.substring(0, slash).trim() : text; + if (addressPart.indexOf('%') >= 0 || !InetAddresses.isInetAddress(addressPart)) { + throw invalid(text, "not an IP literal or CIDR block"); + } + InetAddress address = InetAddresses.forString(addressPart); + // Java turns an IPv4-mapped IPv6 literal into an Inet4Address, so look at how the address + // was written to decide whether a prefix length counts IPv4 or IPv6 bits. + boolean writtenAsIpv4 = address instanceof Inet4Address && addressPart.indexOf(':') < 0; + int maxBits = writtenAsIpv4 ? IPV4_BITS : IPV6_BITS; + + int prefix = maxBits; + if (slash >= 0) { + String prefixPart = text.substring(slash + 1).trim(); + if (!StringUtils.isNumeric(prefixPart) || prefixPart.length() > 3) { + throw invalid(text, "the prefix length must be a number"); Review Comment: [Nit] The length guard reports the wrong reason for a numeric prefix. `10.0.0.0/0128` has a numeric prefix part, so `StringUtils.isNumeric` passes and only `prefixPart.length() > 3` rejects it, with the message "the prefix length must be a number" — which is confusing for a value that plainly is one. Folding the guard into the range check below reads better: if (!StringUtils.isNumeric(prefixPart)) { throw invalid(text, "the prefix length must be a number"); } if (prefixPart.length() > 3 || Integer.parseInt(prefixPart) > maxBits) { throw invalid(text, "the prefix length must be between 0 and " + maxBits); } The length cap still has to come before `parseInt` to keep a very long digit string from overflowing. Verified by: reading `IpRange.parse` lines 258-266 and walking `"0128"` through `StringUtils.isNumeric` (true for all-digit strings) by hand. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
