This is an automated email from the ASF dual-hosted git repository.
jerryshao pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 16af3f188f [#14021] fix(server): Honor X-Forwarded-For only from
trusted proxies (#14024)
16af3f188f is described below
commit 16af3f188fc9868c4d9d3a293398cfb46ddb1164
Author: Jerry Shao <[email protected]>
AuthorDate: Thu Oct 22 19:48:28 2026 +0800
[#14021] fix(server): Honor X-Forwarded-For only from trusted proxies
(#14024)
### What changes were proposed in this pull request?
- Add a `trustedProxies` web server setting to `JettyServerConfig`: a
comma-separated list of IP literals or CIDR blocks (IPv4 or IPv6), empty
by default. Because it lives in `JettyServerConfig`, it applies to
`gravitino.server.webserver.trustedProxies`,
`gravitino.iceberg-rest.trustedProxies` and
`gravitino.lance-rest.trustedProxies`. Entries are parsed with Guava
`InetAddresses`, so host names are never resolved. An invalid entry
fails startup with an error that names the full key.
- Add `ClientAddressResolver` in server-common:
- If the TCP peer is not a trusted proxy, return the peer address and
ignore `X-Forwarded-For`.
- Otherwise, combine all `X-Forwarded-For` headers, walk the entries
right to left skipping trusted proxies, and return the first untrusted
entry. If every entry is trusted, return the left-most one.
- Strip ports and IPv6 brackets. A malformed or `unknown` entry stops
the walk at the last trusted hop.
- Return addresses in canonical form: IPv6 compressed per RFC 5952 and
IPv4-mapped IPv6 as plain IPv4. This also covers Jetty's bracketed IPv6
peer form, `[0:0:0:0:0:0:0:1]`.
- Use the resolver in `RequestContextFilter`, in the `HttpAuditFilter`
fallback and in `IcebergRequestContext`. `IcebergRequestContext` now
prefers the address in `RequestContext` and no longer calls
`getRemoteHost()`. `GravitinoServer`, the Iceberg `RESTService` and
`LanceRESTService` pass the configured resolver to their filters.
- Fix stale Javadoc and comments. Document the key in the server,
Iceberg REST and Lance REST docs, and add a migration-guide note.
### Why are the changes needed?
Audit events used the first `X-Forwarded-For` entry as the client
address whenever the header was present, without checking which proxy
the request actually came from. The recorded address was therefore
reliable only when every request passed through a reverse proxy that
overwrote the header.
Fix: #14021
### Does this PR introduce _any_ user-facing change?
Yes.
- New property keys: `gravitino.server.webserver.trustedProxies`,
`gravitino.iceberg-rest.trustedProxies` and
`gravitino.lance-rest.trustedProxies`.
- `X-Forwarded-For` is now ignored unless the TCP peer is a listed
proxy. Deployments behind a load balancer or ingress should set
`trustedProxies`; otherwise the audit log records the proxy address.
- IPv6 client addresses are recorded in compressed form, for example
`::1` instead of `[0:0:0:0:0:0:0:1]`.
### How was this patch tested?
- New `TestClientAddressResolver` cases cover:
- an untrusted peer with a forged header;
- a trusted proxy chain;
- CIDR, IPv6 and IPv4-mapped addresses;
- multiple headers, ports and brackets;
- malformed entries;
- canonical output.
- New `TestJettyServerConfig` cases cover parsing and full-key error
messages.
- New `TestJettyServer` cases send real requests over IPv4 and IPv6
loopback.
- Updated `TestRequestContextFilter`, `TestHttpAuditFilter` and
`TestIcebergRequestContext`.
- Ran `./gradlew :server-common:test :server:test
:iceberg:iceberg-rest-server:test :lance:lance-rest-server:test
-PskipITs`.
- Manually started a server built from this branch with the audit log
enabled. Over IPv4 and IPv6, I checked the recorded address for the
Gravitino server, the Iceberg REST and Lance REST aux services, default
and trusted configurations, and invalid values.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5.5 <[email protected]>
---
.../api/event/server/HttpRequestEvent.java | 13 +-
.../api/event/server/HttpRequestFailureEvent.java | 5 +-
.../org/apache/gravitino/utils/RequestContext.java | 7 +-
docs/gravitino-server-config.md | 23 ++
docs/iceberg-rest-service.md | 6 +
docs/lance-rest-service.md | 6 +
docs/migration-guide.md | 17 ++
.../org/apache/gravitino/iceberg/RESTService.java | 41 ++-
.../listener/api/event/IcebergRequestContext.java | 28 +-
.../TestIcebergTableEventDispatcher.java | 2 +-
.../iceberg/service/rest/IcebergRestTestUtil.java | 2 +-
.../api/event/TestIcebergRequestContext.java | 40 ++-
.../apache/gravitino/lance/LanceRESTService.java | 35 ++-
.../server/web/ClientAddressResolver.java | 320 +++++++++++++++++++
.../gravitino/server/web/HttpAuditFilter.java | 41 ++-
.../gravitino/server/web/JettyServerConfig.java | 78 ++++-
.../gravitino/server/web/RequestContextFilter.java | 42 +--
.../server/web/TestClientAddressResolver.java | 338 +++++++++++++++++++++
.../gravitino/server/web/TestHttpAuditFilter.java | 48 ++-
.../gravitino/server/web/TestJettyServer.java | 84 +++++
.../server/web/TestJettyServerConfig.java | 82 +++++
.../server/web/TestRequestContextFilter.java | 46 ++-
.../apache/gravitino/server/GravitinoServer.java | 19 +-
23 files changed, 1209 insertions(+), 114 deletions(-)
diff --git
a/core/src/main/java/org/apache/gravitino/listener/api/event/server/HttpRequestEvent.java
b/core/src/main/java/org/apache/gravitino/listener/api/event/server/HttpRequestEvent.java
index 3347574c38..e7fc9c83f2 100644
---
a/core/src/main/java/org/apache/gravitino/listener/api/event/server/HttpRequestEvent.java
+++
b/core/src/main/java/org/apache/gravitino/listener/api/event/server/HttpRequestEvent.java
@@ -55,12 +55,13 @@ public final class HttpRequestEvent extends Event {
* Constructs an {@code HttpRequestEvent}.
*
* @param user the authenticated user, or {@code "unknown"} if
authentication had not completed.
- * @param remoteAddress the client IP resolved by the filter
(X-Forwarded-For or raw socket
- * address). Stored explicitly, rather than relying on the base {@link
Event} behaviour of
- * reading {@code RequestContext.getRemoteAddress()} at construction
time, so this event still
- * carries a correct address even on a server that does not install
{@code
- * RequestContextFilter} at all (all servers in this codebase currently
do, but this keeps the
- * event resilient to one that doesn't).
+ * @param remoteAddress the client IP resolved by the filter (the TCP peer
address, or the
+ * right-most untrusted X-Forwarded-For entry behind a configured
trusted proxy). Stored
+ * explicitly, rather than relying on the base {@link Event} behaviour
of reading {@code
+ * RequestContext.getRemoteAddress()} at construction time, so this
event still carries a
+ * correct address even on a server that does not install {@code
RequestContextFilter} at all
+ * (all servers in this codebase currently do, but this keeps the event
resilient to one that
+ * doesn't).
* @param httpMethod the HTTP method (e.g. {@code "GET"}, {@code "POST"}).
* @param requestUri the request URI path (e.g. {@code "/search/query"}).
* @param statusCode the HTTP response status code (e.g. {@code 200}).
diff --git
a/core/src/main/java/org/apache/gravitino/listener/api/event/server/HttpRequestFailureEvent.java
b/core/src/main/java/org/apache/gravitino/listener/api/event/server/HttpRequestFailureEvent.java
index 22485e12f9..8fd87a257d 100644
---
a/core/src/main/java/org/apache/gravitino/listener/api/event/server/HttpRequestFailureEvent.java
+++
b/core/src/main/java/org/apache/gravitino/listener/api/event/server/HttpRequestFailureEvent.java
@@ -54,8 +54,9 @@ public final class HttpRequestFailureEvent extends
FailureEvent {
* Constructs an {@code HttpRequestFailureEvent}.
*
* @param user the authenticated user, or {@code "unknown"} if
authentication had not completed.
- * @param remoteAddress the client IP resolved by the filter
(X-Forwarded-For or raw socket
- * address). Stored explicitly, rather than relying on the base {@link
+ * @param remoteAddress the client IP resolved by the filter (the TCP peer
address, or the
+ * right-most untrusted X-Forwarded-For entry behind a configured
trusted proxy). Stored
+ * explicitly, rather than relying on the base {@link
* org.apache.gravitino.listener.api.event.Event} behaviour of reading
{@code
* RequestContext.getRemoteAddress()} at construction time, so this
event still carries a
* correct address even on a server that does not install {@code
RequestContextFilter} at all
diff --git a/core/src/main/java/org/apache/gravitino/utils/RequestContext.java
b/core/src/main/java/org/apache/gravitino/utils/RequestContext.java
index 01b6e4f607..b353aaf22c 100644
--- a/core/src/main/java/org/apache/gravitino/utils/RequestContext.java
+++ b/core/src/main/java/org/apache/gravitino/utils/RequestContext.java
@@ -29,8 +29,9 @@ import java.util.Map;
* <p>Currently tracks four pieces of state:
*
* <ul>
- * <li><b>remoteAddress</b> — the client IP resolved from {@code
X-Forwarded-For} or {@link
- * javax.servlet.http.HttpServletRequest#getRemoteAddr()}.
+ * <li><b>remoteAddress</b> — the client IP: {@link
+ * javax.servlet.http.HttpServletRequest#getRemoteAddr()}, or the
right-most untrusted {@code
+ * X-Forwarded-For} entry when the request came through a configured
trusted proxy.
* <li><b>operationOutcome</b> — set by {@link
org.apache.gravitino.listener.EventBus} when an
* operation-layer {@link org.apache.gravitino.listener.api.event.Event}
or {@link
* org.apache.gravitino.listener.api.event.FailureEvent} is dispatched,
so that {@code
@@ -76,7 +77,7 @@ public class RequestContext {
/**
* Sets the client remote address for the current request thread.
*
- * @param remoteAddress the client IP address (or the first entry of {@code
X-Forwarded-For}).
+ * @param remoteAddress the client IP address, resolved with the server's
trusted-proxy rules.
*/
public static void setRemoteAddress(String remoteAddress) {
REMOTE_ADDRESS.set(remoteAddress);
diff --git a/docs/gravitino-server-config.md b/docs/gravitino-server-config.md
index 2df4374ed7..a96db19343 100644
--- a/docs/gravitino-server-config.md
+++ b/docs/gravitino-server-config.md
@@ -180,6 +180,7 @@ empty string or list; `(none)` means it has no default at
all.
| `gravitino.server.webserver.requestHeaderSize` | Maximum size in bytes
of an HTTP request header.
| `131072` |
| `gravitino.server.webserver.responseHeaderSize` | Maximum size in bytes
of an HTTP response header.
| `131072` |
| `gravitino.server.webserver.customFilters` | Comma-separated list
of servlet filter class names to apply to the API.
| (empty) |
+| `gravitino.server.webserver.trustedProxies` | Comma-separated list
of IP addresses or CIDR blocks (IPv4 or IPv6) of reverse proxies whose
`X-Forwarded-For` header is trusted when recording the client address. Host
names are not allowed. | (empty) |
| `gravitino.server.rest.extensionPackages` | Comma-separated list
of packages to scan for additional REST resources.
| (empty) |
| `gravitino.server.visibleConfigs` | Comma-separated list
of extra properties to expose on the unauthenticated `GET /configs` endpoint,
on top of the fixed set it always returns. Additive, so each entry widens what
is public. | (empty) |
| `gravitino.server.bulk.maxItems` | Maximum number of
items allowed in a single bulk request.
| `100` |
@@ -189,6 +190,28 @@ Filters named in `customFilters` must be standard
`javax.servlet` filters. Pass
filter with properties of the form
`gravitino.server.webserver.{filter_class_name}.param.{param_name} = {value}`.
+`trustedProxies` controls the client address recorded in audit events. A
caller can set
+`X-Forwarded-For` to any value, so the server honors it only when the TCP peer
is a trusted proxy.
+In that case the server reads every `X-Forwarded-For` header from right to
left, skips trusted
+proxies, and records the first address that is not one. Entries to the left of
that address were
+supplied by the client and are ignored. Empty list elements are skipped. If an
entry is not an IP address, the server
+stops and records the last trusted hop. When `trustedProxies` is empty, the
default, `X-Forwarded-For` is
+ignored and the TCP peer address is recorded. Recorded addresses are in
canonical form, with IPv6
+compressed and without brackets, for example `::1`. An entry that is not an IP
address or CIDR
+block fails server startup. IPv6 link-local addresses (`fe80::/10`) cannot be
trusted proxies: the
+same link-local address can name different hosts on different network links,
so an entry inside
+`fe80::/10` also fails startup, and a link-local peer is never trusted even
when a wider entry such
+as `::/0` covers it. Use a global or unique local address (for example
`fd00::/8`) for the proxy.
+
+If Gravitino runs behind a load balancer, ingress controller, or other reverse
proxy, list their
+addresses here. Otherwise the audit log records the proxy address for every
request. List only
+the proxies themselves: trusting a range that untrusted clients can connect
from, such as a whole
+Kubernetes pod CIDR, lets those clients choose the recorded address again.
+
+```properties
+gravitino.server.webserver.trustedProxies = 10.0.0.10, 10.0.1.0/24,
fd00:10::/64
+```
+
`GET /configs` backs the Web UI, so it answers without authentication and
always returns
`gravitino.authenticators`, `gravitino.authorization.enable`, and
`gravitino.schema.separator`.
It adds the OAuth client settings when `oauth` is among the authenticators.
Treat anything you add
diff --git a/docs/iceberg-rest-service.md b/docs/iceberg-rest-service.md
index 72feb3bd52..60584f2e9f 100644
--- a/docs/iceberg-rest-service.md
+++ b/docs/iceberg-rest-service.md
@@ -142,11 +142,17 @@ distribution published under the `com.datastrato` Maven
group. Its version is pi
| `gravitino.iceberg-rest.includeErrorStackTrace` | Whether error responses
include server-side stack traces. Set this to `false` in new deployments
because responses can expose internal implementation details.
| `true`
| No |
| `gravitino.iceberg-rest.metrics.enabled` | Whether standalone mode
serves metrics endpoints. Ignored in auxiliary mode; use the Gravitino server
endpoint there. Enable only on a trusted network.
| `false`
| No |
| `gravitino.iceberg-rest.customFilters` | Comma-separated list of
filter class names to apply to the APIs.
| (none)
| No |
+| `gravitino.iceberg-rest.trustedProxies` | Comma-separated list of
IP addresses or CIDR blocks of reverse proxies whose `X-Forwarded-For` header
is trusted when recording the client address in audit events.
| (none)
| No |
| `gravitino.iceberg-rest.advertised-uri` | The public endpoint
reported to clients that discover the service through the Gravitino server.
Must be an absolute `http`/`https` URI with a host and no query or fragment.
| (none)
| No |
The filter in `customFilters` should be a standard javax servlet filter.
Specify filter parameters by setting configuration entries in the style
`gravitino.iceberg-rest.<class name of filter>.param.<param name>=<value>`.
+`trustedProxies` works as described for
+[`gravitino.server.webserver.trustedProxies`](./gravitino-server-config.md#http-server).
When it is
+empty, `X-Forwarded-For` is ignored and the TCP peer address is recorded. The
service listens on its
+own port even in auxiliary mode, so set this key separately from the Gravitino
server's.
+
Set `gravitino.iceberg-rest.advertised-uri` when clients such as the Trino
connector reach the service through a reverse proxy whose scheme, host, port or
path differs from the listener's, for example
`https://iceberg.example.com/iceberg/`.
An explicit port must be in the range 1-65535; an invalid value makes
discovery requests fail instead of advertising an unreachable endpoint.
It only affects the advertised endpoint, not the listener; when unset, the
endpoint is derived from `host`, `httpPort`/`httpsPort` and `enableHttps`.
diff --git a/docs/lance-rest-service.md b/docs/lance-rest-service.md
index 92567a6aed..1064d77f0f 100644
--- a/docs/lance-rest-service.md
+++ b/docs/lance-rest-service.md
@@ -138,6 +138,7 @@ To enable the Lance REST service within Gravitino server,
configure the followin
| `gravitino.lance-rest.httpPort` | Port number for Lance REST
service | 9101 |
No |
| `gravitino.lance-rest.host` | Hostname for Lance REST service
| 0.0.0.0 | No
|
| `gravitino.lance-rest.includeErrorStackTrace` | Whether error responses
include server-side stack traces in `detail`. Set this to `false` in new
deployments | true | No |
+| `gravitino.lance-rest.trustedProxies` | Comma-separated IP addresses or
CIDR blocks of reverse proxies whose `X-Forwarded-For` is trusted for audit
client addresses | (none) | No |
| `gravitino.lance-rest.namespace-backend` | Namespace metadata backend
(currently only `gravitino` is supported) | gravitino |
Yes |
| `gravitino.lance-rest.gravitino-uri` | Gravitino server URI. Not
required in auxiliary mode. | http://localhost:8090 |
No |
| `gravitino.lance-rest.gravitino-metalake` | Gravitino metalake name
(required when namespace-backend is `gravitino`) | (none)
| Yes |
@@ -193,6 +194,11 @@ Configure the service by editing
`{GRAVITINO_HOME}/conf/gravitino-lance-rest-ser
| `gravitino.lance-rest.host` | Service hostname
| 0.0.0.0 | No |
| `gravitino.lance-rest.includeErrorStackTrace` | Whether error responses
include stack traces | true | No |
| `gravitino.lance-rest.metrics.enabled` | Enable standalone metrics
endpoints | false | No |
+| `gravitino.lance-rest.trustedProxies` | Reverse proxies trusted for
X-Forwarded-For | (none) | No |
+
+`trustedProxies` works as described for
+[`gravitino.server.webserver.trustedProxies`](./gravitino-server-config.md#http-server).
When it is
+empty, `X-Forwarded-For` is ignored and the audit log records the TCP peer
address.
:::tip
In standalone deployments, you only need to configure
`gravitino.lance-rest.gravitino-metalake`,
diff --git a/docs/migration-guide.md b/docs/migration-guide.md
index 9bbecb698c..c66233a377 100644
--- a/docs/migration-guide.md
+++ b/docs/migration-guide.md
@@ -11,6 +11,23 @@ existing deployments. For database backup, schema scripts,
and rollback commands
## Upgrading from Gravitino 1.3 to 2.0
+### Audit client address behind a reverse proxy
+
+- Gravitino 1.3 used the first `X-Forwarded-For` entry as the audit client
address whenever
+ the header was present. The server now ignores `X-Forwarded-For` unless the
request comes from
+ a proxy listed in `trustedProxies`.
+- If the Gravitino server, the Iceberg REST service, or the Lance REST service
runs behind a load
+ balancer, ingress controller, or other reverse proxy, set
+ `gravitino.server.webserver.trustedProxies`,
`gravitino.iceberg-rest.trustedProxies`, or
+ `gravitino.lance-rest.trustedProxies` to the proxy addresses. Without it,
the audit log records
+ the proxy address instead of the client address.
+- IPv6 client addresses in audit events are now recorded in compressed form
without brackets,
+ for example `::1` instead of `[0:0:0:0:0:0:0:1]`. Update log queries or
parsers that match the
+ old form.
+- List only the proxies. Trusting a range that untrusted clients can also
connect from, such as a
+ whole Kubernetes pod CIDR, lets those clients choose the recorded address
again. See
+ [HTTP Server](./gravitino-server-config.md#http-server).
+
### Policies are selected through tags
- Direct policy associations with metadata objects are no longer read or
written. The upgrade
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java
index b54434b3e0..577bb29ab7 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java
@@ -57,6 +57,7 @@ import org.apache.gravitino.listener.EventBus;
import org.apache.gravitino.listener.api.event.EventSource;
import org.apache.gravitino.metrics.MetricsSystem;
import org.apache.gravitino.metrics.source.MetricsSource;
+import org.apache.gravitino.server.web.ClientAddressResolver;
import org.apache.gravitino.server.web.HealthAliasServlet;
import org.apache.gravitino.server.web.HttpAuditFilter;
import org.apache.gravitino.server.web.HttpServerMetricsSource;
@@ -92,7 +93,8 @@ public class RESTService implements GravitinoAuxiliaryService
{
private boolean auxMode;
private void initServer(IcebergConfig icebergConfig) {
- JettyServerConfig serverConfig =
JettyServerConfig.fromConfig(icebergConfig);
+ JettyServerConfig serverConfig =
+ JettyServerConfig.fromServiceConfig(icebergConfig,
IcebergConfig.ICEBERG_CONFIG_PREFIX);
IcebergRESTUtils.setIncludeErrorStackTrace(serverConfig.isIncludeErrorStackTrace());
server =
new JettyServer() {
@@ -217,28 +219,14 @@ public class RESTService implements
GravitinoAuxiliaryService {
server.addServlet(servlet, ICEBERG_SPEC);
// Registered before HttpAuditFilter so audit events dispatched during
this request carry the
// request's query parameters and remote address, exactly as on the main
server.
- server.addFilter(new RequestContextFilter(eventBus), ICEBERG_SPEC);
- server.addFilter(
- new HttpAuditFilter(
- eventBus,
- EventSource.GRAVITINO_ICEBERG_REST_SERVER,
- new IcebergHealthCheckPathMatcher()),
- ICEBERG_SPEC);
+ ClientAddressResolver clientAddressResolver =
serverConfig.getClientAddressResolver();
+ server.addFilter(new RequestContextFilter(eventBus,
clientAddressResolver), ICEBERG_SPEC);
+ server.addFilter(createHttpAuditFilter(eventBus, clientAddressResolver),
ICEBERG_SPEC);
if (metricsEnabled) {
- server.addFilter(new RequestContextFilter(eventBus),
JettyServer.METRICS_PATH_SPECS.get(0));
- server.addFilter(new RequestContextFilter(eventBus),
JettyServer.METRICS_PATH_SPECS.get(1));
- server.addFilter(
- new HttpAuditFilter(
- eventBus,
- EventSource.GRAVITINO_ICEBERG_REST_SERVER,
- new IcebergHealthCheckPathMatcher()),
- JettyServer.METRICS_PATH_SPECS.get(0));
- server.addFilter(
- new HttpAuditFilter(
- eventBus,
- EventSource.GRAVITINO_ICEBERG_REST_SERVER,
- new IcebergHealthCheckPathMatcher()),
- JettyServer.METRICS_PATH_SPECS.get(1));
+ for (String pathSpec : JettyServer.METRICS_PATH_SPECS) {
+ server.addFilter(new RequestContextFilter(eventBus,
clientAddressResolver), pathSpec);
+ server.addFilter(createHttpAuditFilter(eventBus,
clientAddressResolver), pathSpec);
+ }
}
server.addSystemFilters(ICEBERG_SPEC);
@@ -328,4 +316,13 @@ public class RESTService implements
GravitinoAuxiliaryService {
packages.addAll(icebergConfig.get(IcebergConfig.REST_API_EXTENSION_PACKAGES));
return packages.toArray(new String[0]);
}
+
+ private static HttpAuditFilter createHttpAuditFilter(
+ EventBus eventBus, ClientAddressResolver clientAddressResolver) {
+ return new HttpAuditFilter(
+ eventBus,
+ EventSource.GRAVITINO_ICEBERG_REST_SERVER,
+ new IcebergHealthCheckPathMatcher(),
+ clientAddressResolver);
+ }
}
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergRequestContext.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergRequestContext.java
index d03cbde592..5f28060345 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergRequestContext.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergRequestContext.java
@@ -25,7 +25,9 @@ import java.util.Map;
import javax.servlet.http.HttpServletRequest;
import org.apache.commons.lang3.StringUtils;
import org.apache.gravitino.iceberg.service.IcebergRESTUtils;
+import org.apache.gravitino.server.web.ClientAddressResolver;
import org.apache.gravitino.utils.PrincipalUtils;
+import org.apache.gravitino.utils.RequestContext;
/**
* The general request context information for Iceberg REST operations.
@@ -38,6 +40,8 @@ public class IcebergRequestContext {
/** Header that opts a drop purge request into asynchronous file cleanup. */
public static final String ASYNC_PURGE_HEADER = "X-Gravitino-Async-Purge";
+ private static final String UNKNOWN_ADDRESS = "unknown";
+
/**
* @deprecated Kept only for backward-compatibility and will be removed in
the next major release.
*/
@@ -97,14 +101,16 @@ public class 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();
- }
- return request.getRemoteHost();
+ // 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. A missing address is recorded as
"unknown", as other events do.
+ String contextAddress = RequestContext.getRemoteAddress();
+ String address =
+ contextAddress != null
+ ? contextAddress
+ : ClientAddressResolver.noTrustedProxies().resolve(request);
+ return StringUtils.isBlank(address) ? UNKNOWN_ADDRESS : address;
}
/**
@@ -126,9 +132,11 @@ public class IcebergRequestContext {
}
/**
- * Returns the hostname of the HTTP client.
+ * Returns the address of the HTTP client, resolved with the server's
trusted-proxy rules (see
+ * {@code trustedProxies} in the Jetty server configuration). Despite the
method name, this is an
+ * IP address, never a resolved host name.
*
- * @return The remote host name.
+ * @return The remote client address.
*/
public String remoteHostName() {
return remoteHostName;
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/dispatcher/TestIcebergTableEventDispatcher.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/dispatcher/TestIcebergTableEventDispatcher.java
index 8456cef293..5263bf5a36 100644
---
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/dispatcher/TestIcebergTableEventDispatcher.java
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/dispatcher/TestIcebergTableEventDispatcher.java
@@ -320,7 +320,7 @@ public class TestIcebergTableEventDispatcher {
private static HttpServletRequest requestWithHeader(String name, String
value) {
HttpServletRequest request = mock(HttpServletRequest.class);
Enumeration<String> headerNames =
Collections.enumeration(Collections.singleton(name));
- when(request.getRemoteHost()).thenReturn("localhost");
+ when(request.getRemoteAddr()).thenReturn("127.0.0.1");
when(request.getHeaderNames()).thenReturn(headerNames);
when(request.getHeader(name)).thenReturn(value);
return request;
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/IcebergRestTestUtil.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/IcebergRestTestUtil.java
index c10a6140b3..84d8a0bab6 100644
---
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/IcebergRestTestUtil.java
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/IcebergRestTestUtil.java
@@ -204,7 +204,7 @@ public class IcebergRestTestUtil {
static HttpServletRequest createMockHttpRequest() {
HttpServletRequest mockRequest = mock(HttpServletRequest.class);
- when(mockRequest.getRemoteHost()).thenReturn("localhost");
+ when(mockRequest.getRemoteAddr()).thenReturn("127.0.0.1");
when(mockRequest.getHeaderNames()).thenReturn(Collections.emptyEnumeration());
return mockRequest;
}
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/listener/api/event/TestIcebergRequestContext.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/listener/api/event/TestIcebergRequestContext.java
index aacb26bfd8..71231880d3 100644
---
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/listener/api/event/TestIcebergRequestContext.java
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/listener/api/event/TestIcebergRequestContext.java
@@ -27,6 +27,7 @@ import java.util.Collections;
import java.util.Enumeration;
import javax.servlet.http.HttpServletRequest;
import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.utils.RequestContext;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
@@ -143,9 +144,44 @@ class TestIcebergRequestContext {
Assertions.assertEquals(context.httpHeaders(), event.customInfo());
}
+ @Test
+ void testRemoteAddressComesFromRequestContext() {
+ // RequestContextFilter has already applied the trusted-proxy rules; a
forged header that
+ // reaches the resource method must not override that result.
+ HttpServletRequest request = requestWithHeader("X-Forwarded-For",
"198.51.100.7");
+ RequestContext.setRemoteAddress("203.0.113.5");
+ try {
+ Assertions.assertEquals(
+ "203.0.113.5", new IcebergRequestContext(request,
"cat").remoteHostName());
+ } finally {
+ RequestContext.clear();
+ }
+ }
+
+ @Test
+ void testFallbackPeerAddressIsCanonical() {
+ HttpServletRequest request = requestWithoutHeader();
+ when(request.getRemoteAddr()).thenReturn("[0:0:0:0:0:0:0:1]");
+ Assertions.assertEquals("::1", new IcebergRequestContext(request,
"cat").remoteHostName());
+ }
+
+ @Test
+ void testMissingPeerAddressIsRecordedAsUnknown() {
+ HttpServletRequest request = requestWithoutHeader();
+ when(request.getRemoteAddr()).thenReturn(null);
+ Assertions.assertEquals("unknown", new IcebergRequestContext(request,
"cat").remoteHostName());
+ }
+
+ @Test
+ void testRemoteAddressFallsBackToPeerAndIgnoresForwardedFor() {
+ HttpServletRequest request = requestWithHeader("X-Forwarded-For",
"198.51.100.7");
+ Assertions.assertEquals(
+ "127.0.0.1", new IcebergRequestContext(request,
"cat").remoteHostName());
+ }
+
private static HttpServletRequest requestWithoutHeader() {
HttpServletRequest request = mock(HttpServletRequest.class);
- when(request.getRemoteHost()).thenReturn("localhost");
+ when(request.getRemoteAddr()).thenReturn("127.0.0.1");
when(request.getHeaderNames()).thenReturn(Collections.emptyEnumeration());
return request;
}
@@ -157,7 +193,7 @@ class TestIcebergRequestContext {
private static HttpServletRequest requestWithHeader(String name, String
value) {
HttpServletRequest request = mock(HttpServletRequest.class);
Enumeration<String> headerNames =
Collections.enumeration(Collections.singleton(name));
- when(request.getRemoteHost()).thenReturn("localhost");
+ when(request.getRemoteAddr()).thenReturn("127.0.0.1");
when(request.getHeaderNames()).thenReturn(headerNames);
when(request.getHeader(name)).thenReturn(value);
return request;
diff --git
a/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceRESTService.java
b/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceRESTService.java
index c977835286..01cc81a082 100644
---
a/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceRESTService.java
+++
b/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceRESTService.java
@@ -41,6 +41,7 @@ import org.apache.gravitino.listener.EventBus;
import org.apache.gravitino.listener.api.event.EventSource;
import org.apache.gravitino.metrics.MetricsSystem;
import org.apache.gravitino.metrics.source.MetricsSource;
+import org.apache.gravitino.server.web.ClientAddressResolver;
import org.apache.gravitino.server.web.HealthAliasServlet;
import org.apache.gravitino.server.web.HttpAuditFilter;
import org.apache.gravitino.server.web.HttpServerMetricsSource;
@@ -77,7 +78,8 @@ public class LanceRESTService implements
GravitinoAuxiliaryService {
@Override
public void serviceInit(Map<String, String> properties, boolean auxMode) {
LanceConfig lanceConfig = new LanceConfig(new HashMap<>(properties));
- JettyServerConfig serverConfig = JettyServerConfig.fromConfig(lanceConfig);
+ JettyServerConfig serverConfig =
+ JettyServerConfig.fromServiceConfig(lanceConfig,
LanceConfig.LANCE_CONFIG_PREFIX);
LanceExceptionMapper.setIncludeErrorStackTrace(serverConfig.isIncludeErrorStackTrace());
server = new LanceJettyServer();
@@ -141,22 +143,14 @@ public class LanceRESTService implements
GravitinoAuxiliaryService {
server.addServlet(container, LANCE_SPEC);
// Registered before HttpAuditFilter so audit events dispatched during
this request carry the
// request's query parameters and remote address, exactly as on the main
server.
- server.addFilter(new RequestContextFilter(eventBus), LANCE_SPEC);
- server.addFilter(
- new HttpAuditFilter(
- eventBus, EventSource.GRAVITINO_LANCE_REST_SERVER, new
LanceHealthCheckPathMatcher()),
- LANCE_SPEC);
+ ClientAddressResolver clientAddressResolver =
serverConfig.getClientAddressResolver();
+ server.addFilter(new RequestContextFilter(eventBus,
clientAddressResolver), LANCE_SPEC);
+ server.addFilter(createHttpAuditFilter(eventBus, clientAddressResolver),
LANCE_SPEC);
if (metricsEnabled) {
- server.addFilter(new RequestContextFilter(eventBus),
JettyServer.METRICS_PATH_SPECS.get(0));
- server.addFilter(new RequestContextFilter(eventBus),
JettyServer.METRICS_PATH_SPECS.get(1));
- server.addFilter(
- new HttpAuditFilter(
- eventBus, EventSource.GRAVITINO_LANCE_REST_SERVER, new
LanceHealthCheckPathMatcher()),
- JettyServer.METRICS_PATH_SPECS.get(0));
- server.addFilter(
- new HttpAuditFilter(
- eventBus, EventSource.GRAVITINO_LANCE_REST_SERVER, new
LanceHealthCheckPathMatcher()),
- JettyServer.METRICS_PATH_SPECS.get(1));
+ for (String pathSpec : JettyServer.METRICS_PATH_SPECS) {
+ server.addFilter(new RequestContextFilter(eventBus,
clientAddressResolver), pathSpec);
+ server.addFilter(createHttpAuditFilter(eventBus,
clientAddressResolver), pathSpec);
+ }
}
server.addSystemFilters(LANCE_SPEC);
if (auxMode) {
@@ -230,4 +224,13 @@ public class LanceRESTService implements
GravitinoAuxiliaryService {
throw new RuntimeException("Failed to load namespace implementation", e);
}
}
+
+ private static HttpAuditFilter createHttpAuditFilter(
+ EventBus eventBus, ClientAddressResolver clientAddressResolver) {
+ return new HttpAuditFilter(
+ eventBus,
+ EventSource.GRAVITINO_LANCE_REST_SERVER,
+ new LanceHealthCheckPathMatcher(),
+ clientAddressResolver);
+ }
}
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/web/ClientAddressResolver.java
b/server-common/src/main/java/org/apache/gravitino/server/web/ClientAddressResolver.java
new file mode 100644
index 0000000000..009b3df436
--- /dev/null
+++
b/server-common/src/main/java/org/apache/gravitino/server/web/ClientAddressResolver.java
@@ -0,0 +1,320 @@
+/*
+ * 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.Inet6Address;
+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>IPv6 link-local addresses ({@code fe80::/10}) are never trusted. The
same link-local address
+ * can name different hosts on different links, told apart only by the zone ID
(RFC 4007, section
+ * 12), and matching zones reliably is not supported. A trusted proxy entry
inside {@code fe80::/10}
+ * fails at construction, and a link-local peer is not trusted even when a
wider entry such as
+ * {@code ::/0} covers it.
+ *
+ * <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>Empty list elements, such as the one in {@code 203.0.113.5, ,
10.2.2.2}, are ignored.
+ * <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() || isIpv6LinkLocal(address.get())) {
+ 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(",")) {
+ String trimmed = entry.trim();
+ // RFC 7230 section 7: empty list elements must be ignored, not
treated as malformed.
+ if (!trimmed.isEmpty()) {
+ entries.add(trimmed);
+ }
+ }
+ }
+ 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 (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);
+ }
+ // Drop an IPv6 zone ID, such as "fe80::1%eth0". Only link-local addresses
carry one, and those
+ // are never trusted, so dropping it cannot make an address trusted.
+ 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));
+ }
+
+ private static boolean isIpv6LinkLocal(InetAddress address) {
+ return address instanceof Inet6Address && address.isLinkLocalAddress();
+ }
+
+ /** 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);
+ if (isIpv6LinkLocal(address)) {
+ throw invalid(
+ text,
+ "IPv6 link-local addresses cannot be trusted proxies, because the
same address can"
+ + " name different hosts on different links");
+ }
+ // 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)) {
+ throw invalid(text, "the prefix length must be a number");
+ }
+ int parsed;
+ try {
+ parsed = Integer.parseInt(prefixPart);
+ } catch (NumberFormatException overflow) {
+ throw invalid(text, "the prefix length must be between 0 and " +
maxBits);
+ }
+ if (parsed > maxBits) {
+ throw invalid(text, "the prefix length must be between 0 and " +
maxBits);
+ }
+ prefix = parsed;
+ }
+ int prefixBits = writtenAsIpv4 ? prefix + IPV4_MAPPED_PREFIX_BITS :
prefix;
+ return new IpRange(mask(toIpv6Bytes(address), prefixBits), prefixBits);
+ }
+
+ boolean contains(byte[] ipv6Bytes) {
+ return Arrays.equals(network, mask(ipv6Bytes, prefixBits));
+ }
+
+ private static byte[] mask(byte[] bytes, int prefixBits) {
+ byte[] masked = bytes.clone();
+ for (int i = 0; i < masked.length; i++) {
+ int bitsInByte = Math.max(0, Math.min(8, prefixBits - i * 8));
+ masked[i] &= (byte) (0xff << (8 - bitsInByte));
+ }
+ return masked;
+ }
+
+ private static IllegalArgumentException invalid(String text, String
reason) {
+ return new IllegalArgumentException(
+ String.format("Invalid trusted proxy '%s': %s", text, reason));
+ }
+ }
+}
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/web/HttpAuditFilter.java
b/server-common/src/main/java/org/apache/gravitino/server/web/HttpAuditFilter.java
index 29f401bd92..2acce692d3 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/web/HttpAuditFilter.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/web/HttpAuditFilter.java
@@ -33,7 +33,6 @@ import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpServletResponseWrapper;
-import org.apache.commons.lang3.StringUtils;
import org.apache.gravitino.auth.AuthConstants;
import org.apache.gravitino.listener.EventBus;
import org.apache.gravitino.listener.api.event.EventSource;
@@ -54,9 +53,8 @@ import org.slf4j.LoggerFactory;
* RequestContextFilter} (so the remote address is already populated) but
<em>before</em> both
* custom filters and {@link
org.apache.gravitino.server.authentication.AuthenticationFilter} (so it
* wraps every downstream filter and observes 401 authentication failures,
custom-filter 403s, and
- * other downstream 4xx/5xx responses). It is safe to install on servers that
do not use {@code
- * RequestContextFilter} (Iceberg REST, Lance REST) because it resolves the
client address
- * independently.
+ * other downstream 4xx/5xx responses). If {@code RequestContextFilter} did
not run, it resolves the
+ * client address itself with the same {@link ClientAddressResolver} rules.
*
* <p><strong>DispatcherType.ERROR skip:</strong> when an uncaught exception
escapes the filter
* chain, this filter fires an {@link HttpRequestFailureEvent} (status 500) in
its {@code finally}
@@ -94,11 +92,11 @@ import org.slf4j.LoggerFactory;
public class HttpAuditFilter implements Filter {
private static final Logger LOG =
LoggerFactory.getLogger(HttpAuditFilter.class);
- private static final String X_FORWARDED_FOR = "X-Forwarded-For";
private final Optional<EventBus> eventBus;
private final EventSource eventSource;
private final HealthCheckPathMatcher healthCheckMatcher;
+ private final ClientAddressResolver clientAddressResolver;
/**
* Constructs an {@code HttpAuditFilter} using the default {@link
HealthCheckPathMatcher}.
@@ -126,9 +124,30 @@ public class HttpAuditFilter implements Filter {
@Nullable EventBus eventBus,
EventSource eventSource,
HealthCheckPathMatcher healthCheckMatcher) {
+ this(eventBus, eventSource, healthCheckMatcher,
ClientAddressResolver.noTrustedProxies());
+ }
+
+ /**
+ * Constructs an {@code HttpAuditFilter} with a custom {@link
HealthCheckPathMatcher} and {@link
+ * ClientAddressResolver}.
+ *
+ * @param eventBus the event bus used to dispatch {@link
HttpRequestFailureEvent}s and {@link
+ * HttpRequestEvent}s; may be {@code null}, in which case the filter is
a pass-through no-op.
+ * @param eventSource identifies which server this filter instance is
installed on.
+ * @param healthCheckMatcher determines which URI paths are health check
probes; those paths are
+ * excluded from audit logging to avoid polluting the audit log with
probe traffic.
+ * @param clientAddressResolver resolves the client address when {@link
RequestContextFilter} has
+ * not already stored one, honoring {@code X-Forwarded-For} only from
trusted proxies.
+ */
+ public HttpAuditFilter(
+ @Nullable EventBus eventBus,
+ EventSource eventSource,
+ HealthCheckPathMatcher healthCheckMatcher,
+ ClientAddressResolver clientAddressResolver) {
this.eventBus = Optional.ofNullable(eventBus);
this.eventSource = eventSource;
this.healthCheckMatcher = healthCheckMatcher;
+ this.clientAddressResolver = clientAddressResolver;
}
@Override
@@ -251,18 +270,14 @@ public class HttpAuditFilter implements Filter {
}
private String resolveClientAddress(HttpServletRequest request) {
- // Prefer the address already resolved by RequestContextFilter (installed
on GravitinoServer).
- // For servers that do not install RequestContextFilter (Iceberg REST,
Lance REST), fall back to
- // independent header parsing so the event still carries a meaningful
client address.
+ // Prefer the address already resolved by RequestContextFilter, which
every server installs
+ // ahead of this filter. Fall back to resolving it here, with the same
trusted-proxy rules, in
+ // case this filter is ever installed on a path without
RequestContextFilter.
String contextAddress = RequestContext.getRemoteAddress();
if (contextAddress != null) {
return contextAddress;
}
- String xForwardedFor = request.getHeader(X_FORWARDED_FOR);
- if (StringUtils.isNotBlank(xForwardedFor)) {
- return xForwardedFor.split(",")[0].trim();
- }
- return request.getRemoteAddr();
+ return clientAddressResolver.resolve(request);
}
/**
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/web/JettyServerConfig.java
b/server-common/src/main/java/org/apache/gravitino/server/web/JettyServerConfig.java
index 825716faa3..7f289443d0 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/web/JettyServerConfig.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/web/JettyServerConfig.java
@@ -312,6 +312,26 @@ public final class JettyServerConfig {
.booleanConf()
.createWithDefault(true);
+ /**
+ * The proxies whose {@code X-Forwarded-For} header is honored when
resolving the client address
+ * for audit events. Empty by default, which means the header is ignored and
the TCP peer address
+ * is recorded.
+ */
+ public static final ConfigEntry<List<String>> TRUSTED_PROXIES =
+ new ConfigBuilder("trustedProxies")
+ .doc(
+ "Comma separated list of IP addresses or CIDR blocks (IPv4 or
IPv6) of reverse "
+ + "proxies whose X-Forwarded-For header is trusted when
resolving the client "
+ + "address recorded in audit events. Host names are not
allowed. When the "
+ + "request comes from a trusted proxy, the right-most
X-Forwarded-For entry "
+ + "that is not a trusted proxy is used as the client
address. Empty by "
+ + "default, which means X-Forwarded-For is ignored and the
address of the TCP "
+ + "peer is used")
+ .version(ConfigConstants.VERSION_2_0_0)
+ .stringConf()
+ .toSequence()
+ .createWithDefault(Collections.emptyList());
+
private final String host;
private final int httpPort;
@@ -354,10 +374,12 @@ public final class JettyServerConfig {
private final boolean allowCredentials;
private final String exposedHeaders;
private final boolean chainPreflight;
+ private final List<String> trustedProxies;
+ private final ClientAddressResolver clientAddressResolver;
private final Config internalConfig;
- private JettyServerConfig(Map<String, String> configs) {
+ private JettyServerConfig(Map<String, String> configs, String keyPrefix) {
this.internalConfig = new Config(false) {};
internalConfig.loadFromMap(configs, t -> true);
@@ -432,20 +454,37 @@ public final class JettyServerConfig {
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("%s%s: %s", keyPrefix, TRUSTED_PROXIES.getKey(),
e.getMessage()), e);
+ }
}
public static JettyServerConfig fromConfig(Config config, String prefix) {
- Map<String, String> configs = config.getConfigsWithPrefix(prefix);
- if (config instanceof OverwriteDefaultConfig) {
- configs = overwriteJettyDefaultConfig(configs, (OverwriteDefaultConfig)
config);
- }
- return new JettyServerConfig(configs);
+ return fromConfig(config, prefix, prefix);
}
public static JettyServerConfig fromConfig(Config config) {
return fromConfig(config, "");
}
+ /**
+ * Creates the Jetty server configuration of an auxiliary service whose
configuration keys have
+ * already had the service prefix removed, such as an Iceberg or Lance REST
service configuration.
+ *
+ * @param config the service configuration, with keys relative to {@code
servicePrefix}
+ * @param servicePrefix the prefix the keys have in the configuration file,
for example {@code
+ * gravitino.iceberg-rest.}; only used to name the full key in error
messages
+ * @return the Jetty server configuration
+ */
+ public static JettyServerConfig fromServiceConfig(Config config, String
servicePrefix) {
+ return fromConfig(config, "", servicePrefix);
+ }
+
public String getHost() {
return host;
}
@@ -589,6 +628,25 @@ public final class JettyServerConfig {
return allowedHeaders;
}
+ /**
+ * Returns the configured trusted proxies, each entry trimmed and blank
entries dropped.
+ *
+ * @return the trusted proxy IP addresses and CIDR blocks; empty when none
are configured
+ */
+ public List<String> getTrustedProxies() {
+ return trustedProxies;
+ }
+
+ /**
+ * Returns the resolver that derives the client address of a request from
the configured trusted
+ * proxies.
+ *
+ * @return the client address resolver for this server
+ */
+ public ClientAddressResolver getClientAddressResolver() {
+ return clientAddressResolver;
+ }
+
private SSLContext getDefaultSSLContext() {
try {
return SSLContext.getDefault();
@@ -617,6 +675,14 @@ public final class JettyServerConfig {
return
Sets.newHashSet(context.getServerSocketFactory().getSupportedCipherSuites());
}
+ private static JettyServerConfig fromConfig(Config config, String prefix,
String keyPrefix) {
+ Map<String, String> configs = config.getConfigsWithPrefix(prefix);
+ if (config instanceof OverwriteDefaultConfig) {
+ configs = overwriteJettyDefaultConfig(configs, (OverwriteDefaultConfig)
config);
+ }
+ return new JettyServerConfig(configs, keyPrefix);
+ }
+
private static Map<String, String> overwriteJettyDefaultConfig(
Map<String, String> properties, OverwriteDefaultConfig config) {
if (config.getOverwriteDefaultConfig().isEmpty()) return properties;
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/web/RequestContextFilter.java
b/server-common/src/main/java/org/apache/gravitino/server/web/RequestContextFilter.java
index 78016bcedb..6b9abdf903 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/web/RequestContextFilter.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/web/RequestContextFilter.java
@@ -47,11 +47,11 @@ import org.apache.gravitino.utils.RequestContext;
* from each HTTP request and stores them in {@link RequestContext} so that
audit event constructors
* can read them on the same thread.
*
- * <p>When a reverse proxy is in use, the real client IP is taken from the
first entry of the {@code
- * X-Forwarded-For} header (a de-facto standard header set by reverse proxies;
note that it is
- * trusted unconditionally — deployments where the server is reachable
directly, without a trusted
- * reverse proxy, should be aware that clients can spoof this header). If the
header is absent,
- * {@link HttpServletRequest#getRemoteAddr()} is used instead.
+ * <p>The client address is resolved by a {@link ClientAddressResolver}. The
{@code X-Forwarded-For}
+ * header is honored only when the request arrives from a configured trusted
proxy ({@code
+ * trustedProxies} in {@link JettyServerConfig}); otherwise {@link
+ * HttpServletRequest#getRemoteAddr()} is used, so a caller cannot choose the
address that audit
+ * events record by sending the header itself.
*
* <p>Query parameters are parsed from {@link
HttpServletRequest#getQueryString()}, not from {@link
* HttpServletRequest#getParameterMap()}. The latter also parses an {@code
@@ -77,8 +77,6 @@ import org.apache.gravitino.utils.RequestContext;
*/
public class RequestContextFilter implements Filter {
- private static final String X_FORWARDED_FOR = "X-Forwarded-For";
-
/** Maximum number of distinct query-parameter names captured per request. */
static final int MAX_PARAMETERS = 50;
@@ -89,8 +87,12 @@ public class RequestContextFilter implements Filter {
static final String TRUNCATED_SUFFIX = "...(truncated)";
private final Optional<EventBus> eventBus;
+ private final ClientAddressResolver clientAddressResolver;
- /** Constructs a {@code RequestContextFilter} that never captures query
parameters. */
+ /**
+ * Constructs a {@code RequestContextFilter} that never captures query
parameters and trusts no
+ * proxy.
+ */
public RequestContextFilter() {
this(null);
}
@@ -103,7 +105,21 @@ public class RequestContextFilter implements Filter {
* happens, since {@code HttpAuditFilter}'s own fallback events need it
regardless).
*/
public RequestContextFilter(@Nullable EventBus eventBus) {
+ this(eventBus, ClientAddressResolver.noTrustedProxies());
+ }
+
+ /**
+ * Constructs a {@code RequestContextFilter}.
+ *
+ * @param eventBus the event bus that will consume the captured query
parameters; may be {@code
+ * null}, in which case query-parameter capture is skipped.
+ * @param clientAddressResolver resolves the client address from the
request, honoring {@code
+ * X-Forwarded-For} only from trusted proxies.
+ */
+ public RequestContextFilter(
+ @Nullable EventBus eventBus, ClientAddressResolver
clientAddressResolver) {
this.eventBus = Optional.ofNullable(eventBus);
+ this.clientAddressResolver = clientAddressResolver;
}
@Override
@@ -115,7 +131,7 @@ public class RequestContextFilter implements Filter {
try {
if (request instanceof HttpServletRequest) {
HttpServletRequest httpRequest = (HttpServletRequest) request;
- RequestContext.setRemoteAddress(resolveClientAddress(httpRequest));
+
RequestContext.setRemoteAddress(clientAddressResolver.resolve(httpRequest));
if (eventBus.isPresent()) {
RequestContext.setRequestQueryParams(parseQueryString(httpRequest.getQueryString()));
}
@@ -129,14 +145,6 @@ public class RequestContextFilter implements Filter {
@Override
public void destroy() {}
- private String resolveClientAddress(HttpServletRequest request) {
- String xForwardedFor = request.getHeader(X_FORWARDED_FOR);
- if (StringUtils.isNotBlank(xForwardedFor)) {
- return xForwardedFor.split(",")[0].trim();
- }
- return request.getRemoteAddr();
- }
-
/**
* Parses a raw HTTP query string into a name-to-value map (joining
multi-valued parameters with a
* comma), with no redaction — see the class doc for why that happens later.
Bounded by {@link
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/web/TestClientAddressResolver.java
b/server-common/src/test/java/org/apache/gravitino/server/web/TestClientAddressResolver.java
new file mode 100644
index 0000000000..c67251859f
--- /dev/null
+++
b/server-common/src/test/java/org/apache/gravitino/server/web/TestClientAddressResolver.java
@@ -0,0 +1,338 @@
+/*
+ * 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 static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import com.google.common.collect.ImmutableList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import javax.servlet.http.HttpServletRequest;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class TestClientAddressResolver {
+
+ private static final ClientAddressResolver TRUST_PRIVATE_V4 =
+ new ClientAddressResolver(ImmutableList.of("10.0.0.0/8",
"192.168.1.10"));
+
+ @Test
+ public void testNoTrustedProxiesIgnoresForwardedFor() {
+ HttpServletRequest request = request("10.0.0.1", "203.0.113.5");
+ Assertions.assertEquals("10.0.0.1",
ClientAddressResolver.noTrustedProxies().resolve(request));
+ Assertions.assertEquals(
+ "10.0.0.1", new
ClientAddressResolver(Collections.emptyList()).resolve(request));
+ }
+
+ @Test
+ public void testUntrustedPeerWithSpoofedForwardedForIsIgnored() {
+ // A client connecting directly cannot choose the recorded address by
sending the header.
+ HttpServletRequest request = request("198.51.100.9", "10.0.0.1,
203.0.113.5");
+ Assertions.assertEquals("198.51.100.9", TRUST_PRIVATE_V4.resolve(request));
+ }
+
+ @Test
+ public void testTrustedPeerWithoutForwardedForReturnsPeer() {
+ Assertions.assertEquals("10.0.0.1",
TRUST_PRIVATE_V4.resolve(request("10.0.0.1")));
+ }
+
+ @Test
+ public void testSingleTrustedProxy() {
+ Assertions.assertEquals(
+ "203.0.113.5", TRUST_PRIVATE_V4.resolve(request("192.168.1.10",
"203.0.113.5")));
+ }
+
+ @Test
+ public void testTrustedProxyChainReturnsRightMostUntrusted() {
+ // The client prepended a forged 198.51.100.7; the real client is the
entry the first trusted
+ // proxy appended.
+ HttpServletRequest request =
+ request("10.0.0.1", "198.51.100.7, 203.0.113.5, 10.2.2.2,
192.168.1.10");
+ Assertions.assertEquals("203.0.113.5", TRUST_PRIVATE_V4.resolve(request));
+ }
+
+ @Test
+ public void testCidrMatching() {
+ ClientAddressResolver resolver = new
ClientAddressResolver(ImmutableList.of("172.16.0.0/12"));
+ Assertions.assertEquals(
+ "203.0.113.5", resolver.resolve(request("172.31.255.254",
"203.0.113.5")));
+ Assertions.assertEquals("172.32.0.1",
resolver.resolve(request("172.32.0.1", "203.0.113.5")));
+ Assertions.assertEquals(
+ "172.15.255.255", resolver.resolve(request("172.15.255.255",
"203.0.113.5")));
+ }
+
+ @Test
+ public void testNonOctetAlignedCidr() {
+ ClientAddressResolver resolver = new
ClientAddressResolver(ImmutableList.of("10.1.2.64/27"));
+ Assertions.assertEquals("203.0.113.5",
resolver.resolve(request("10.1.2.95", "203.0.113.5")));
+ Assertions.assertEquals("10.1.2.96", resolver.resolve(request("10.1.2.96",
"203.0.113.5")));
+ }
+
+ @Test
+ public void testZeroPrefixTrustsEverything() {
+ ClientAddressResolver resolver = new
ClientAddressResolver(ImmutableList.of("0.0.0.0/0"));
+ // Every entry is trusted, so the left-most one is returned.
+ Assertions.assertEquals(
+ "198.51.100.7", resolver.resolve(request("203.0.113.1", "198.51.100.7,
203.0.113.5")));
+ }
+
+ @Test
+ public void testAllEntriesTrustedReturnsLeftMost() {
+ HttpServletRequest request = request("10.0.0.1", "10.3.3.3, 10.2.2.2");
+ Assertions.assertEquals("10.3.3.3", TRUST_PRIVATE_V4.resolve(request));
+ }
+
+ @Test
+ public void testIpv6() {
+ ClientAddressResolver resolver = new
ClientAddressResolver(ImmutableList.of("fd00::/8", "::1"));
+ // Jetty reports an IPv6 peer in brackets and uncompressed.
+ Assertions.assertEquals(
+ "2001:db8::1",
+ resolver.resolve(request("[fd12:3456:0:0:0:0:0:1]",
"2001:db8:0:0:0:0:0:1")));
+ Assertions.assertEquals(
+ "2001:db8::1", resolver.resolve(request("[0:0:0:0:0:0:0:1]",
"2001:db8::1, fd00::2")));
+ Assertions.assertEquals(
+ "fe80::1", resolver.resolve(request("[fe80:0:0:0:0:0:0:1]",
"2001:db8::1")));
+ // The unbracketed form is accepted too.
+ Assertions.assertEquals("2001:db8::1", resolver.resolve(request("::1",
"2001:db8::1")));
+ }
+
+ @Test
+ public void testPeerAddressIsCanonical() {
+ // Without trusted proxies too, so the audit log never mixes forms of the
same address.
+ ClientAddressResolver none = ClientAddressResolver.noTrustedProxies();
+ Assertions.assertEquals("::1", none.resolve(request("[0:0:0:0:0:0:0:1]",
"203.0.113.5")));
+ Assertions.assertEquals("::1", none.resolve(request("0:0:0:0:0:0:0:1")));
+ Assertions.assertEquals("fe80::1",
none.resolve(request("[fe80:0:0:0:0:0:0:1%1]")));
+ Assertions.assertEquals("10.0.0.1",
none.resolve(request("::ffff:10.0.0.1")));
+ Assertions.assertEquals("10.0.0.1", none.resolve(request("10.0.0.1")));
+ // Untrusted peer and trusted peer without X-Forwarded-For.
+ Assertions.assertEquals(
+ "2001:db8::5",
TRUST_PRIVATE_V4.resolve(request("[2001:db8:0:0:0:0:0:5]", "1.2.3.4")));
+ ClientAddressResolver trustLoopback = new
ClientAddressResolver(ImmutableList.of("::1"));
+ Assertions.assertEquals("::1",
trustLoopback.resolve(request("[0:0:0:0:0:0:0:1]")));
+ Assertions.assertEquals(
+ "::1", trustLoopback.resolve(request("[0:0:0:0:0:0:0:1]",
"203.0.113.5, unknown")));
+ }
+
+ @Test
+ public void testLinkLocalPeerIsNeverTrusted() {
+ // fe80::1%1 and fe80::1%2 can be different hosts on different links (RFC
4007, section 12),
+ // so a link-local peer must not inherit trust from a wider range that
covers it.
+ ClientAddressResolver allIpv6 = new
ClientAddressResolver(ImmutableList.of("::/0"));
+ Assertions.assertEquals(
+ "fe80::1", allIpv6.resolve(request("[fe80:0:0:0:0:0:0:1%1]",
"203.0.113.77")));
+ Assertions.assertEquals(
+ "fe80::1", allIpv6.resolve(request("[fe80:0:0:0:0:0:0:1%2]",
"203.0.113.77")));
+ Assertions.assertEquals("fe80::1", allIpv6.resolve(request("fe80::1",
"203.0.113.77")));
+ // Other IPv6 peers in the same range are still trusted.
+ Assertions.assertEquals(
+ "203.0.113.77", allIpv6.resolve(request("[fd00:0:0:0:0:0:0:1]",
"203.0.113.77")));
+ // A link-local entry in the header is reported as the client, never
skipped as a proxy.
+ Assertions.assertEquals(
+ "fe80::5", allIpv6.resolve(request("[fd00:0:0:0:0:0:0:1]",
"203.0.113.77, fe80::5")));
+ }
+
+ @Test
+ public void testLinkLocalTrustedProxyIsRejected() {
+ for (String invalid : Arrays.asList("fe80::1", "fe80::/10", "febf::1/128",
"fe80::/9")) {
+ IllegalArgumentException e =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> new ClientAddressResolver(ImmutableList.of(invalid)),
+ "Expected rejection of " + invalid);
+ Assertions.assertTrue(e.getMessage().contains("link-local"),
e.getMessage());
+ }
+ // A zone ID is not accepted either.
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> new ClientAddressResolver(ImmutableList.of("fe80::1%1")));
+ }
+
+ @Test
+ public void testIpv4MappedIpv6() {
+ // An IPv4 proxy entry matches the IPv4-mapped IPv6 form a dual-stack
socket reports.
+ Assertions.assertEquals(
+ "203.0.113.5", TRUST_PRIVATE_V4.resolve(request("::ffff:10.0.0.1",
"203.0.113.5")));
+ Assertions.assertEquals(
+ "203.0.113.5", TRUST_PRIVATE_V4.resolve(request("[::ffff:10.0.0.1]",
"203.0.113.5")));
+
+ // An IPv4-mapped IPv6 CIDR matches the plain IPv4 form.
+ ClientAddressResolver mapped =
+ new ClientAddressResolver(ImmutableList.of("::ffff:10.0.0.0/104"));
+ Assertions.assertEquals("203.0.113.5", mapped.resolve(request("10.9.8.7",
"203.0.113.5")));
+ Assertions.assertEquals("11.0.0.1", mapped.resolve(request("11.0.0.1",
"203.0.113.5")));
+
+ // A mapped entry in the header is returned in canonical IPv4 form.
+ Assertions.assertEquals(
+ "203.0.113.5", TRUST_PRIVATE_V4.resolve(request("10.0.0.1",
"::ffff:203.0.113.5")));
+ }
+
+ @Test
+ public void testIpv4AndIpv6RangesDoNotOverlap() {
+ // 10.0.0.0/8 must not be read as an IPv6 /8 that would cover unrelated
addresses.
+ Assertions.assertEquals("::a00:1",
TRUST_PRIVATE_V4.resolve(request("::a00:1", "203.0.113.5")));
+ }
+
+ @Test
+ public void testMultipleHeadersAreCombinedInOrder() {
+ HttpServletRequest request =
+ request("10.0.0.1", Arrays.asList("198.51.100.7, 203.0.113.5",
"10.2.2.2"));
+ Assertions.assertEquals("203.0.113.5", TRUST_PRIVATE_V4.resolve(request));
+
+ HttpServletRequest oneEntryPerHeader =
+ request("10.0.0.1", Arrays.asList("203.0.113.5", "10.2.2.2"));
+ Assertions.assertEquals("203.0.113.5",
TRUST_PRIVATE_V4.resolve(oneEntryPerHeader));
+ }
+
+ @Test
+ public void testPortsAndBracketsAreStripped() {
+ Assertions.assertEquals(
+ "203.0.113.5", TRUST_PRIVATE_V4.resolve(request("10.0.0.1",
"203.0.113.5:4711")));
+ Assertions.assertEquals(
+ "2001:db8::1", TRUST_PRIVATE_V4.resolve(request("10.0.0.1",
"[2001:db8::1]:4711")));
+ Assertions.assertEquals(
+ "2001:db8::1", TRUST_PRIVATE_V4.resolve(request("10.0.0.1",
"[2001:db8::1]")));
+ Assertions.assertEquals(
+ "203.0.113.5", TRUST_PRIVATE_V4.resolve(request("10.0.0.1",
"203.0.113.5, 10.2.2.2:80")));
+ }
+
+ @Test
+ public void testMalformedEntryStopsAtLastTrustedHop() {
+ // Malformed right after the peer: fall back to the peer itself.
+ Assertions.assertEquals(
+ "10.0.0.1", TRUST_PRIVATE_V4.resolve(request("10.0.0.1", "203.0.113.5,
not-an-ip")));
+ // Malformed behind a trusted hop: return that hop, never anything left of
the bad entry.
+ Assertions.assertEquals(
+ "10.2.2.2",
+ TRUST_PRIVATE_V4.resolve(request("10.0.0.1", "203.0.113.5, unknown,
10.2.2.2")));
+ Assertions.assertEquals(
+ "10.2.2.2", TRUST_PRIVATE_V4.resolve(request("10.0.0.1", "203.0.113.5,
bad, 10.2.2.2")));
+ Assertions.assertEquals(
+ "10.0.0.1", TRUST_PRIVATE_V4.resolve(request("10.0.0.1",
"203.0.113.5:http")));
+ Assertions.assertEquals(
+ "10.0.0.1", TRUST_PRIVATE_V4.resolve(request("10.0.0.1",
"[2001:db8::1")));
+ Assertions.assertEquals(
+ "10.0.0.1", TRUST_PRIVATE_V4.resolve(request("10.0.0.1",
"[2001:db8::1]x")));
+ Assertions.assertEquals("10.0.0.1",
TRUST_PRIVATE_V4.resolve(request("10.0.0.1", "UNKNOWN")));
+ // Host names are never resolved.
+ Assertions.assertEquals("10.0.0.1",
TRUST_PRIVATE_V4.resolve(request("10.0.0.1", "localhost")));
+ }
+
+ @Test
+ public void testEmptyListElementsAreIgnored() {
+ // RFC 7230 section 7: empty elements in a comma-separated list are
ignored.
+ Assertions.assertEquals(
+ "203.0.113.5", TRUST_PRIVATE_V4.resolve(request("10.0.0.1",
"203.0.113.5, , 10.2.2.2")));
+ Assertions.assertEquals(
+ "203.0.113.5", TRUST_PRIVATE_V4.resolve(request("10.0.0.1",
"203.0.113.5,")));
+ Assertions.assertEquals(
+ "203.0.113.5", TRUST_PRIVATE_V4.resolve(request("10.0.0.1",
",203.0.113.5,,")));
+ Assertions.assertEquals(
+ "203.0.113.5", TRUST_PRIVATE_V4.resolve(request("10.0.0.1",
"203.0.113.5", " ", "")));
+ Assertions.assertEquals("10.0.0.1",
TRUST_PRIVATE_V4.resolve(request("10.0.0.1", " , ")));
+ }
+
+ @Test
+ public void testUnparsablePeerIsNotTrusted() {
+ Assertions.assertEquals(
+ "some-host", TRUST_PRIVATE_V4.resolve(request("some-host",
"203.0.113.5")));
+ Assertions.assertNull(TRUST_PRIVATE_V4.resolve(request(null,
"203.0.113.5")));
+ Assertions.assertEquals("[", TRUST_PRIVATE_V4.resolve(request("[",
"203.0.113.5")));
+ Assertions.assertEquals("[]", TRUST_PRIVATE_V4.resolve(request("[]",
"203.0.113.5")));
+ }
+
+ @Test
+ public void testNullHeaderEnumerationIsTreatedAsAbsent() {
+ HttpServletRequest request = mock(HttpServletRequest.class);
+ when(request.getRemoteAddr()).thenReturn("10.0.0.1");
+
when(request.getHeaders(ClientAddressResolver.X_FORWARDED_FOR)).thenReturn(null);
+ Assertions.assertEquals("10.0.0.1", TRUST_PRIVATE_V4.resolve(request));
+ }
+
+ @Test
+ public void testInvalidTrustedProxyIsRejected() {
+ for (String invalid :
+ Arrays.asList(
+ "proxy.example.com",
+ "10.0.0.256",
+ "10.0.0.0/33",
+ "10.0.0.0/-1",
+ "fd00::/129",
+ "10.0.0.0/8/8",
+ "::ffff:10.0.0.0/129",
+ "fe80::1%eth0")) {
+ IllegalArgumentException e =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> new ClientAddressResolver(ImmutableList.of(invalid)),
+ "Expected rejection of " + invalid);
+ Assertions.assertTrue(e.getMessage().contains(invalid), e.getMessage());
+ }
+ }
+
+ @Test
+ public void testOutOfRangePrefixReportsRange() {
+ for (String invalid :
+ Arrays.asList("10.0.0.0/0128", "10.0.0.0/33", "fd00::/1290",
"10.0.0.0/99999999999")) {
+ IllegalArgumentException e =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> new ClientAddressResolver(ImmutableList.of(invalid)));
+ Assertions.assertTrue(e.getMessage().contains("must be between 0 and"),
e.getMessage());
+ }
+ IllegalArgumentException e =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> new ClientAddressResolver(ImmutableList.of("10.0.0.0/abc")));
+ Assertions.assertTrue(e.getMessage().contains("must be a number"),
e.getMessage());
+ }
+
+ @Test
+ public void testZeroPaddedPrefixIsAccepted() {
+ // The value decides, not the digit count.
+ for (String range : Arrays.asList("10.0.0.0/024", "10.0.0.0/0024")) {
+ ClientAddressResolver resolver = new
ClientAddressResolver(ImmutableList.of(range));
+ Assertions.assertEquals("203.0.113.5",
resolver.resolve(request("10.0.0.5", "203.0.113.5")));
+ Assertions.assertEquals("10.0.1.5", resolver.resolve(request("10.0.1.5",
"203.0.113.5")));
+ }
+ }
+
+ @Test
+ public void testBlankTrustedProxyEntriesAreIgnored() {
+ ClientAddressResolver resolver =
+ new ClientAddressResolver(Arrays.asList(" ", "", " 10.0.0.1 "));
+ Assertions.assertEquals("203.0.113.5",
resolver.resolve(request("10.0.0.1", "203.0.113.5")));
+ }
+
+ private static HttpServletRequest request(String remoteAddr, String...
forwardedFor) {
+ return request(remoteAddr, Arrays.asList(forwardedFor));
+ }
+
+ private static HttpServletRequest request(String remoteAddr, List<String>
forwardedFor) {
+ HttpServletRequest request = mock(HttpServletRequest.class);
+ when(request.getRemoteAddr()).thenReturn(remoteAddr);
+ when(request.getHeaders(ClientAddressResolver.X_FORWARDED_FOR))
+ .thenReturn(Collections.enumeration(forwardedFor));
+ return request;
+ }
+}
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/web/TestHttpAuditFilter.java
b/server-common/src/test/java/org/apache/gravitino/server/web/TestHttpAuditFilter.java
index dd025492ab..9d634bc816 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/web/TestHttpAuditFilter.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/web/TestHttpAuditFilter.java
@@ -25,8 +25,10 @@ import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
+import com.google.common.collect.ImmutableList;
import java.io.IOException;
import java.security.Principal;
+import java.util.Collections;
import javax.servlet.DispatcherType;
import javax.servlet.FilterChain;
import javax.servlet.ServletException;
@@ -366,7 +368,7 @@ public class TestHttpAuditFilter {
// ─── Remote address resolution
───────────────────────────────────────────────
@Test
- public void testXForwardedForFirstEntryUsedAsRemoteAddress() throws
Exception {
+ public void testXForwardedForIgnoredFromUntrustedPeer() throws Exception {
EventBus eventBus = mock(EventBus.class);
HttpAuditFilter filter = new HttpAuditFilter(eventBus,
EventSource.GRAVITINO_SERVER);
HttpServletRequest req =
@@ -376,6 +378,45 @@ public class TestHttpAuditFilter {
filter.doFilter(req, resp, chain);
+ ArgumentCaptor<BaseEvent> captor =
ArgumentCaptor.forClass(BaseEvent.class);
+ verify(eventBus).dispatchEvent(captor.capture());
+ HttpRequestFailureEvent event = (HttpRequestFailureEvent)
captor.getValue();
+ Assertions.assertEquals("10.0.0.1", event.remoteAddress());
+ }
+
+ @Test
+ public void testXForwardedForHonoredFromTrustedProxy() throws Exception {
+ EventBus eventBus = mock(EventBus.class);
+ HttpAuditFilter filter =
+ new HttpAuditFilter(
+ eventBus,
+ EventSource.GRAVITINO_SERVER,
+ new HealthCheckPathMatcher(),
+ new ClientAddressResolver(ImmutableList.of("10.0.0.0/8")));
+ HttpServletRequest req =
+ mockRequest("GET", "/api/metalakes", "198.51.100.7, 203.0.113.5,
10.1.1.1", "10.0.0.1");
+ HttpServletResponse resp = mock(HttpServletResponse.class);
+ FilterChain chain = (request, response) -> ((HttpServletResponse)
response).sendError(401);
+
+ filter.doFilter(req, resp, chain);
+
+ ArgumentCaptor<BaseEvent> captor =
ArgumentCaptor.forClass(BaseEvent.class);
+ verify(eventBus).dispatchEvent(captor.capture());
+ HttpRequestFailureEvent event = (HttpRequestFailureEvent)
captor.getValue();
+ Assertions.assertEquals("203.0.113.5", event.remoteAddress());
+ }
+
+ @Test
+ public void testRequestContextAddressPreferredOverResolver() throws
Exception {
+ EventBus eventBus = mock(EventBus.class);
+ HttpAuditFilter filter = new HttpAuditFilter(eventBus,
EventSource.GRAVITINO_SERVER);
+ HttpServletRequest req = mockRequest("GET", "/api/metalakes", null,
"10.0.0.1");
+ HttpServletResponse resp = mock(HttpServletResponse.class);
+ FilterChain chain = (request, response) -> ((HttpServletResponse)
response).sendError(401);
+
+ RequestContext.setRemoteAddress("203.0.113.5");
+ filter.doFilter(req, resp, chain);
+
ArgumentCaptor<BaseEvent> captor =
ArgumentCaptor.forClass(BaseEvent.class);
verify(eventBus).dispatchEvent(captor.capture());
HttpRequestFailureEvent event = (HttpRequestFailureEvent)
captor.getValue();
@@ -565,6 +606,11 @@ public class TestHttpAuditFilter {
when(req.getMethod()).thenReturn(method);
when(req.getRequestURI()).thenReturn(uri);
when(req.getHeader("X-Forwarded-For")).thenReturn(xForwardedFor);
+ when(req.getHeaders("X-Forwarded-For"))
+ .thenReturn(
+ xForwardedFor == null
+ ? Collections.emptyEnumeration()
+ :
Collections.enumeration(Collections.singletonList(xForwardedFor)));
when(req.getRemoteAddr()).thenReturn(remoteAddr);
return req;
}
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/web/TestJettyServer.java
b/server-common/src/test/java/org/apache/gravitino/server/web/TestJettyServer.java
index fab893990d..b48b527f3d 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/web/TestJettyServer.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/web/TestJettyServer.java
@@ -23,6 +23,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assumptions.assumeTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.mockStatic;
@@ -31,8 +32,11 @@ import java.io.IOException;
import java.io.InputStreamReader;
import java.io.Reader;
import java.net.HttpURLConnection;
+import java.net.InetSocketAddress;
+import java.net.Socket;
import java.net.URL;
import java.nio.charset.StandardCharsets;
+import java.util.Arrays;
import javax.servlet.Filter;
import javax.servlet.Servlet;
import javax.servlet.http.HttpServlet;
@@ -41,6 +45,7 @@ import javax.servlet.http.HttpServletResponse;
import org.apache.gravitino.Config;
import org.apache.gravitino.metrics.MetricsSystem;
import org.apache.gravitino.rest.RESTUtils;
+import org.apache.gravitino.utils.RequestContext;
import org.eclipse.jetty.util.thread.QueuedThreadPool;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
@@ -174,6 +179,76 @@ public class TestJettyServer {
}
}
+ /**
+ * Sends real requests over IPv4 and IPv6 loopback, so the peer address
comes from Jetty itself
+ * (which brackets IPv6 peers) rather than from a mock.
+ */
+ @Test
+ public void testTrustedProxyResolvesClientAddressOverRealConnections()
throws IOException {
+ int port = startServerWithClientAddressEcho("127.0.0.1, ::1");
+
+ assertEquals("1.2.3.4", requestClientAddress("127.0.0.1", port, "9.9.9.9,
1.2.3.4"));
+ assertEquals("127.0.0.1", requestClientAddress("127.0.0.1", port, null));
+ assumeTrue(isIpv6LoopbackReachable(port), "IPv6 loopback is not
available");
+ assertEquals("1.2.3.4", requestClientAddress("[::1]", port, "9.9.9.9,
1.2.3.4"));
+ // Jetty reports the peer as "[0:0:0:0:0:0:0:1]"; the recorded form is
canonical.
+ assertEquals("::1", requestClientAddress("[::1]", port, null));
+ }
+
+ @Test
+ public void testForwardedForIgnoredByDefaultOverRealConnection() throws
IOException {
+ int port = startServerWithClientAddressEcho(null);
+
+ assertEquals("127.0.0.1", requestClientAddress("127.0.0.1", port,
"1.2.3.4"));
+ }
+
+ /**
+ * Starts the server with {@link RequestContextFilter} wired to the
configured resolver, as the
+ * real servers do, and a servlet that echoes the resolved client address.
+ */
+ private int startServerWithClientAddressEcho(String trustedProxies) throws
IOException {
+ int port = RESTUtils.findAvailablePort(5000, 6000);
+ Config config = new Config(false) {};
+ config.set(JettyServerConfig.WEBSERVER_HTTP_PORT, port);
+ if (trustedProxies != null) {
+ config.set(JettyServerConfig.TRUSTED_PROXIES,
Arrays.asList(trustedProxies.split(",")));
+ }
+ JettyServerConfig serverConfig = JettyServerConfig.fromConfig(config);
+ jettyServer.initialize(serverConfig, "test", false);
+ jettyServer.addServlet(new ClientAddressEchoServlet(), "/echo");
+ jettyServer.addFilter(
+ new RequestContextFilter(null,
serverConfig.getClientAddressResolver()), "/echo");
+ jettyServer.start();
+ return port;
+ }
+
+ private String requestClientAddress(String host, int port, String
xForwardedFor)
+ throws IOException {
+ HttpURLConnection connection =
+ (HttpURLConnection) new URL("http://"; + host + ":" + port +
"/echo").openConnection();
+ try {
+ if (xForwardedFor != null) {
+ connection.setRequestProperty(ClientAddressResolver.X_FORWARDED_FOR,
xForwardedFor);
+ }
+ assertEquals(HttpServletResponse.SC_OK, connection.getResponseCode());
+ try (Reader body =
+ new InputStreamReader(connection.getInputStream(),
StandardCharsets.UTF_8)) {
+ return CharStreams.toString(body);
+ }
+ } finally {
+ connection.disconnect();
+ }
+ }
+
+ private static boolean isIpv6LoopbackReachable(int port) {
+ try (Socket socket = new Socket()) {
+ socket.connect(new InetSocketAddress("::1", port), 1000);
+ return true;
+ } catch (IOException e) {
+ return false;
+ }
+ }
+
/** Starts the server with a servlet that throws, and returns Jetty's error
page for it. */
private String requestFailingServlet(Config config) throws IOException {
int port = RESTUtils.findAvailablePort(5000, 6000);
@@ -216,6 +291,15 @@ public class TestJettyServer {
}
}
+ private static class ClientAddressEchoServlet extends HttpServlet {
+ @Override
+ protected void doGet(HttpServletRequest request, HttpServletResponse
response)
+ throws IOException {
+ response.setContentType("text/plain");
+ response.getWriter().write(RequestContext.getRemoteAddress());
+ }
+ }
+
private static class FailingServlet extends HttpServlet {
@Override
protected void doGet(HttpServletRequest request, HttpServletResponse
response) {
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/web/TestJettyServerConfig.java
b/server-common/src/test/java/org/apache/gravitino/server/web/TestJettyServerConfig.java
index 5c33f9d486..f8230519ed 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/web/TestJettyServerConfig.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/web/TestJettyServerConfig.java
@@ -18,6 +18,7 @@
*/
package org.apache.gravitino.server.web;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Sets;
import java.util.Arrays;
import java.util.Collections;
@@ -110,4 +111,85 @@ public class TestJettyServerConfig {
Assertions.assertTrue(jettyServerConfig.getAllWithPrefix("2.").isEmpty());
Assertions.assertEquals(2,
jettyServerConfig.getAllWithPrefix("1.").size());
}
+
+ @Test
+ public void testTrustedProxiesDefaultIsEmpty() {
+ JettyServerConfig jettyServerConfig = JettyServerConfig.fromConfig(new
Config() {}, "");
+ Assertions.assertTrue(jettyServerConfig.getTrustedProxies().isEmpty());
+ Assertions.assertNotNull(jettyServerConfig.getClientAddressResolver());
+ }
+
+ @Test
+ public void testTrustedProxiesParsedWithPrefix() {
+ Config config = new Config(false) {};
+ config.loadFromMap(
+ ImmutableMap.of(
+ JettyServerConfig.GRAVITINO_SERVER_CONFIG_PREFIX +
"trustedProxies",
+ " 10.0.0.1, 172.16.0.0/12 ,, fd00::/8, ::ffff:192.168.0.0/112 "),
+ k -> true);
+
+ JettyServerConfig jettyServerConfig =
+ JettyServerConfig.fromConfig(config,
JettyServerConfig.GRAVITINO_SERVER_CONFIG_PREFIX);
+
+ Assertions.assertEquals(
+ Arrays.asList("10.0.0.1", "172.16.0.0/12", "fd00::/8",
"::ffff:192.168.0.0/112"),
+ jettyServerConfig.getTrustedProxies());
+ }
+
+ @Test
+ public void testInvalidTrustedProxyFailsWithClearMessage() {
+ for (String invalid :
+ Arrays.asList(
+ "proxy.example.com",
+ "10.0.0.0/33",
+ "fd00::/129",
+ "10.0.0.0/abc",
+ "10.0.0",
+ "10.0.0.1/")) {
+ Config config = new Config() {};
+ config.set(JettyServerConfig.TRUSTED_PROXIES, List.of("10.0.0.1",
invalid));
+ IllegalArgumentException e =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> JettyServerConfig.fromConfig(config, ""),
+ "Expected failure for " + invalid);
+ Assertions.assertTrue(
+ e.getMessage().contains(JettyServerConfig.TRUSTED_PROXIES.getKey()),
e.getMessage());
+ Assertions.assertTrue(e.getMessage().contains(invalid), e.getMessage());
+ }
+ }
+
+ @Test
+ public void testInvalidTrustedProxyErrorNamesFullKey() {
+ String serverKey = JettyServerConfig.GRAVITINO_SERVER_CONFIG_PREFIX +
"trustedProxies";
+ Config serverConfig = new Config(false) {};
+ serverConfig.loadFromMap(ImmutableMap.of(serverKey, "proxy.example.com"),
k -> true);
+ IllegalArgumentException e =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ JettyServerConfig.fromConfig(
+ serverConfig,
JettyServerConfig.GRAVITINO_SERVER_CONFIG_PREFIX));
+ Assertions.assertTrue(e.getMessage().contains(serverKey), e.getMessage());
+
+ // An auxiliary service configuration has its prefix stripped already.
+ Config serviceConfig = new Config(false) {};
+ serviceConfig.loadFromMap(ImmutableMap.of("trustedProxies",
"10.0.0.0/33"), k -> true);
+ e =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> JettyServerConfig.fromServiceConfig(serviceConfig,
"gravitino.iceberg-rest."));
+ Assertions.assertTrue(
+ e.getMessage().contains("gravitino.iceberg-rest.trustedProxies"),
e.getMessage());
+ }
+
+ @Test
+ public void testFromServiceConfigReadsUnprefixedKeys() {
+ Config serviceConfig = new Config(false) {};
+ serviceConfig.loadFromMap(ImmutableMap.of("trustedProxies", "10.0.0.1"), k
-> true);
+ Assertions.assertEquals(
+ Arrays.asList("10.0.0.1"),
+ JettyServerConfig.fromServiceConfig(serviceConfig,
"gravitino.lance-rest.")
+ .getTrustedProxies());
+ }
}
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/web/TestRequestContextFilter.java
b/server-common/src/test/java/org/apache/gravitino/server/web/TestRequestContextFilter.java
index 14ab58acaf..c2a0cf9928 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/web/TestRequestContextFilter.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/web/TestRequestContextFilter.java
@@ -22,6 +22,7 @@ package org.apache.gravitino.server.web;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
+import com.google.common.collect.ImmutableList;
import java.io.IOException;
import java.util.Collections;
import java.util.Map;
@@ -67,31 +68,46 @@ public class TestRequestContextFilter {
}
@Test
- public void testXForwardedForSingleEntry() throws IOException,
ServletException {
- HttpServletRequest req = mock(HttpServletRequest.class);
+ public void testXForwardedForIgnoredByDefault() throws IOException,
ServletException {
+ HttpServletRequest req = mockRequestWithForwardedFor("10.0.0.1",
"203.0.113.5");
HttpServletResponse resp = mock(HttpServletResponse.class);
- when(req.getHeader("X-Forwarded-For")).thenReturn("203.0.113.5");
- when(req.getRemoteAddr()).thenReturn("10.0.0.1");
AtomicReference<String> captured = new AtomicReference<>();
FilterChain chain = (request, response) ->
captured.set(RequestContext.getRemoteAddress());
filter.doFilter(req, resp, chain);
- Assertions.assertEquals("203.0.113.5", captured.get());
+ Assertions.assertEquals("10.0.0.1", captured.get());
}
@Test
- public void testXForwardedForMultipleEntriesUsesFirst() throws IOException,
ServletException {
- HttpServletRequest req = mock(HttpServletRequest.class);
+ public void testXForwardedForIgnoredFromUntrustedPeer() throws IOException,
ServletException {
+ RequestContextFilter trustingFilter =
+ new RequestContextFilter(null, new
ClientAddressResolver(ImmutableList.of("10.0.0.0/8")));
+ HttpServletRequest req = mockRequestWithForwardedFor("198.51.100.9",
"203.0.113.5");
HttpServletResponse resp = mock(HttpServletResponse.class);
- when(req.getHeader("X-Forwarded-For")).thenReturn("203.0.113.5, 10.1.1.1,
10.2.2.2");
- when(req.getRemoteAddr()).thenReturn("10.0.0.1");
AtomicReference<String> captured = new AtomicReference<>();
FilterChain chain = (request, response) ->
captured.set(RequestContext.getRemoteAddress());
- filter.doFilter(req, resp, chain);
+ trustingFilter.doFilter(req, resp, chain);
+
+ Assertions.assertEquals("198.51.100.9", captured.get());
+ }
+
+ @Test
+ public void testXForwardedForFromTrustedProxyUsesRightMostUntrusted()
+ throws IOException, ServletException {
+ RequestContextFilter trustingFilter =
+ new RequestContextFilter(null, new
ClientAddressResolver(ImmutableList.of("10.0.0.0/8")));
+ HttpServletRequest req =
+ mockRequestWithForwardedFor("10.0.0.1", "198.51.100.7, 203.0.113.5,
10.1.1.1, 10.2.2.2");
+ HttpServletResponse resp = mock(HttpServletResponse.class);
+
+ AtomicReference<String> captured = new AtomicReference<>();
+ FilterChain chain = (request, response) ->
captured.set(RequestContext.getRemoteAddress());
+
+ trustingFilter.doFilter(req, resp, chain);
Assertions.assertEquals("203.0.113.5", captured.get());
}
@@ -316,4 +332,14 @@ public class TestRequestContextFilter {
Assertions.assertTrue(capturedParams.get().isEmpty());
Assertions.assertEquals("1.2.3.4", capturedAddress.get());
}
+
+ private static HttpServletRequest mockRequestWithForwardedFor(
+ String remoteAddr, String xForwardedFor) {
+ HttpServletRequest req = mock(HttpServletRequest.class);
+ when(req.getHeader("X-Forwarded-For")).thenReturn(xForwardedFor);
+ when(req.getHeaders("X-Forwarded-For"))
+
.thenReturn(Collections.enumeration(Collections.singletonList(xForwardedFor)));
+ when(req.getRemoteAddr()).thenReturn(remoteAddr);
+ return req;
+ }
}
diff --git
a/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
b/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
index 406e1c7074..628a8573a5 100644
--- a/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
+++ b/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
@@ -54,8 +54,10 @@ import
org.apache.gravitino.secret.SecretPropertyOperationDispatcher;
import org.apache.gravitino.secret.SecretProviderRegistry;
import org.apache.gravitino.server.authentication.ServerAuthenticator;
import org.apache.gravitino.server.authorization.GravitinoAuthorizerProvider;
+import org.apache.gravitino.server.web.ClientAddressResolver;
import org.apache.gravitino.server.web.ConfigServlet;
import org.apache.gravitino.server.web.HealthAliasServlet;
+import org.apache.gravitino.server.web.HealthCheckPathMatcher;
import org.apache.gravitino.server.web.HttpAuditFilter;
import org.apache.gravitino.server.web.HttpServerMetricsSource;
import org.apache.gravitino.server.web.JettyServer;
@@ -232,9 +234,10 @@ public class GravitinoServer extends ResourceConfig {
// API_ANY_PATH keeps its original, unabridged filter set: this is the
only pathspec that is
// part of the Jersey-managed REST surface, so it's the only one
VersioningFilter applies to.
- server.addFilter(new RequestContextFilter(gravitinoEnv.eventBus()),
API_ANY_PATH);
+ ClientAddressResolver clientAddressResolver =
jettyServerConfig.getClientAddressResolver();
server.addFilter(
- new HttpAuditFilter(gravitinoEnv.eventBus(),
EventSource.GRAVITINO_SERVER), API_ANY_PATH);
+ new RequestContextFilter(gravitinoEnv.eventBus(),
clientAddressResolver), API_ANY_PATH);
+ server.addFilter(createHttpAuditFilter(clientAddressResolver),
API_ANY_PATH);
server.addFilter(
new VersioningFilter(jettyServerConfig.isIncludeErrorStackTrace()),
API_ANY_PATH);
@@ -247,9 +250,9 @@ public class GravitinoServer extends ResourceConfig {
// given gravitinoEnv.eventBus() here too (not just API_ANY_PATH) so
query-parameter capture
// (see RequestContextFilter's class doc) applies uniformly, not only to
the REST API.
for (String pathSpec : ROOT_MOUNTED_PATHS) {
- server.addFilter(new RequestContextFilter(gravitinoEnv.eventBus()),
pathSpec);
server.addFilter(
- new HttpAuditFilter(gravitinoEnv.eventBus(),
EventSource.GRAVITINO_SERVER), pathSpec);
+ new RequestContextFilter(gravitinoEnv.eventBus(),
clientAddressResolver), pathSpec);
+ server.addFilter(createHttpAuditFilter(clientAddressResolver), pathSpec);
}
// Custom filters are registered once, across every filtered path in a
single call, so a
@@ -269,6 +272,14 @@ public class GravitinoServer extends ResourceConfig {
}
}
+ private HttpAuditFilter createHttpAuditFilter(ClientAddressResolver
clientAddressResolver) {
+ return new HttpAuditFilter(
+ gravitinoEnv.eventBus(),
+ EventSource.GRAVITINO_SERVER,
+ new HealthCheckPathMatcher(),
+ clientAddressResolver);
+ }
+
public void start() throws Exception {
gravitinoEnv.start();
server.start();
