Copilot commented on code in PR #14024: URL: https://github.com/apache/gravitino/pull/14024#discussion_r4324657005
########## server-common/src/main/java/org/apache/gravitino/server/web/ClientAddressResolver.java: ########## @@ -0,0 +1,289 @@ +/* + * 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 + */ + 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; + } Review Comment: `resolve(...)` can return `null` when `request.getRemoteAddr()` is `null` (confirmed by `TestClientAddressResolver#testUnparsablePeerIsNotTrusted`). Since the method signature and Javadoc imply a non-null `String`, this is easy to misuse. Consider (mandatory) either annotating the return type as `@Nullable` and updating the Javadoc, or (optional) returning a stable sentinel like `\"unknown\"` to guarantee non-null behavior. ########## server-common/src/main/java/org/apache/gravitino/server/web/JettyServerConfig.java: ########## @@ -432,20 +454,40 @@ private JettyServerConfig(Map<String, String> configs) { this.allowCredentials = internalConfig.get(ALLOW_CREDENTIALS); this.exposedHeaders = internalConfig.get(EXPOSED_HEADERS); this.chainPreflight = internalConfig.get(CHAIN_PREFLIGHT); + + this.trustedProxies = Collections.unmodifiableList(internalConfig.get(TRUSTED_PROXIES)); + try { + this.clientAddressResolver = new ClientAddressResolver(trustedProxies); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException( + String.format( + "Invalid value for %s%s: %s. Each entry must be an IP address or a CIDR block", + keyPrefix, TRUSTED_PROXIES.getKey(), e.getMessage()), + e); + } Review Comment: The wrapped message is a bit redundant because `ClientAddressResolver` already formats errors as `Invalid trusted proxy '...': ...`, so this becomes `Invalid value for ...: Invalid trusted proxy ...`. Consider simplifying by either removing the leading `Invalid` from the nested message (e.g., use `e.getMessage()` without repeating `Invalid`), or changing `ClientAddressResolver` to throw a message without the `Invalid trusted proxy` prefix so the outer message stays clean and consistent. ########## server-common/src/main/java/org/apache/gravitino/server/web/JettyServerConfig.java: ########## @@ -589,6 +631,25 @@ public String getAllowedHeaders() { return allowedHeaders; } + /** + * Returns the configured trusted proxies, as written in the configuration. + * + * @return the trusted proxy IP addresses and CIDR blocks; empty when none are configured + */ + public List<String> getTrustedProxies() { + return trustedProxies; + } Review Comment: The Javadoc says \"as written in the configuration\", but the parsing path appears to normalize entries (e.g., trimming whitespace and dropping blanks per tests like `testTrustedProxiesParsedWithPrefix`). Please update the Javadoc to reflect what callers actually receive (e.g., \"as parsed/normalized\"), or adjust parsing/storage to truly preserve the original raw tokens if that’s the intent. -- 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]
