jerryshao commented on code in PR #14024: URL: https://github.com/apache/gravitino/pull/14024#discussion_r4332424726
########## 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: Empty list elements are now skipped instead of stopping the walk, so `203.0.113.5,` and `203.0.113.5, , 10.2.2.2` record `203.0.113.5`. Flipped the interior assertion and added `testEmptyListElementsAreIgnored` for the leading, trailing, interior and whole-header-empty cases (ec8a0934bf). _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_ ########## 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: `IcebergRequestContext` now records `"unknown"` when the resolved address is null or blank, matching `Event`; added `testMissingPeerAddressIsRecordedAsUnknown` (ec8a0934bf). _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_ ########## 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: Split the checks as suggested: a non-numeric prefix reports "must be a number", and a numeric one that is too long or above the limit reports the range. The length check still runs before `parseInt`. Covered by `testOutOfRangePrefixReportsRange` (ec8a0934bf). _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_ -- 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]
