yuqi1129 commented on code in PR #12891:
URL: https://github.com/apache/gravitino/pull/12891#discussion_r3931730895
##########
core/src/main/java/org/apache/gravitino/audit/AuditLogRedactor.java:
##########
@@ -61,10 +105,18 @@ public static Map<String, String>
redactCustomInfo(Map<String, String> customInf
* @return the redacted value for sensitive keys, otherwise the original
value
*/
public static String redactValue(String key, String value) {
- return isSensitiveCustomInfoKey(key) ? REDACTED_VALUE : value;
+ return isSensitiveKey(key) ? REDACTED_VALUE : value;
}
- private static boolean isSensitiveCustomInfoKey(String key) {
- return key != null &&
MASKED_CUSTOM_INFO_KEYS.contains(key.toLowerCase(Locale.ROOT));
+ private static boolean isSensitiveKey(String key) {
+ if (key == null) {
+ return false;
+ }
+ String lowerCaseKey = key.toLowerCase(Locale.ROOT);
+ if (NEVER_SENSITIVE_KEYS.contains(lowerCaseKey)) {
+ return false;
+ }
+ return MASKED_CUSTOM_INFO_KEYS.contains(lowerCaseKey)
+ || SENSITIVE_KEY_SUBSTRINGS.stream().anyMatch(lowerCaseKey::contains);
Review Comment:
**P1, please fix before merge.** The substring check only lowercases the
key, it does not strip separators, so the three multi-word entries (`apikey`,
`accesskey`, `privatekey`) never match the hyphenated or underscored spellings,
which are the ones people actually use. I fed a set of realistic key names
through `AuditLogRedactor.redactValue` on this branch:
```
api-key -> SECRETVALUE <- not redacted
api_key -> SECRETVALUE
x-api-key -> SECRETVALUE <- the most common API key spelling
access-key -> SECRETVALUE
access_key -> SECRETVALUE
access-key-id -> SECRETVALUE
private-key -> SECRETVALUE
private_key -> SECRETVALUE
apikey / accesskey / accessKeyId / privatekey -> *** (only the
run-together forms match)
author -> *** <- the other direction, false positive
from "auth"
```
`s3.secret-access-key` is only caught because it contains `secret`, not
because of `accesskey`. Something like `oss.access-key-id` would depend
entirely on the exact-match list, and it is not in it.
This matters more here than it would have before, because this PR is what
starts putting caller-chosen parameter names into the audit log.
Minimal fix, normalise before the substring pass. The exemption list is
checked earlier against the raw key, so `auth.method` still works:
```java
String normalized = lowerCaseKey.replaceAll("[^a-z0-9]", "");
return MASKED_CUSTOM_INFO_KEYS.contains(lowerCaseKey)
|| SENSITIVE_KEY_SUBSTRINGS.stream().anyMatch(normalized::contains);
```
##########
core/src/main/java/org/apache/gravitino/listener/api/event/Event.java:
##########
@@ -51,4 +69,34 @@ protected Event(String user, NameIdentifier identifier) {
public String remoteAddress() {
return remoteAddress;
}
+
+ /**
+ * Returns the current request's (raw) query parameters merged with this
event's own facts (from
+ * {@link #ownCustomInfo()}); the subclass's own values win on a key
collision. This method is
+ * {@code final} — override {@link #ownCustomInfo()} to contribute
subclass-specific facts.
+ *
+ * @return the merged custom info, or just the automatically captured query
parameters if the
+ * subclass contributes nothing of its own.
+ */
+ @Override
+ public final Map<String, String> customInfo() {
Review Comment:
`customInfo()` now returns the **raw**, unredacted query parameters, and
redaction only happens in `JsonAuditFormatter` and `SimpleAuditLogV2`. Every
`EventListenerPlugin` receives this map as-is, including third-party ones that
forward events elsewhere.
Before this PR query parameters were not in events at all, so this is a new
exposure surface, and the `AuditLogRedactor` class doc presents the single-pass
design as a pure win without mentioning it.
Two acceptable ways out, both small:
- redact caller-supplied keys at capture time (in `RequestContextFilter` or
here) and leave the renderer pass for everything else, or
- keep the current design but state plainly in
`docs/gravitino-server-config.md` and the release notes that a custom
`EventListenerPlugin` receives unredacted request parameters.
I do not think the single-pass design needs to be reworked, but the security
semantics should not be left for the reader to infer.
##########
core/src/main/java/org/apache/gravitino/listener/api/event/Event.java:
##########
@@ -30,16 +33,31 @@
* <p>The client remote address is captured from {@link RequestContext} at
construction time on the
* servlet thread, so async listener threads can safely call {@link
#remoteAddress()} without
* accessing thread-local storage.
+ *
+ * <p>The current request's query parameters are captured the same way, raw
(not redacted — see
+ * {@code org.apache.gravitino.audit.AuditLogRedactor}), and merged into
{@link #customInfo()}
+ * automatically. This gives every event in the system — including ones with
no subclass-specific
+ * {@code customInfo} — automatic audit coverage of the parameters that
produced it, with no
+ * per-event-class wiring required.
+ *
+ * <p>{@link #customInfo()} itself is {@code final}: a subclass that wants to
contribute its own
+ * facts must override {@link #ownCustomInfo()} instead, never {@code
customInfo()} directly. This
+ * is deliberate — an earlier version of this class let subclasses override
{@code customInfo()}
+ * directly, which let several of them (accidentally) discard the
automatically captured query
+ * parameters instead of merging with them. Sealing the merge here makes that
class of bug
+ * impossible to reintroduce.
*/
@DeveloperApi
public abstract class Event extends BaseEvent {
Review Comment:
`Event` is `@DeveloperApi` and this PR makes `customInfo()` `final`. For
anything outside this repo that subclasses `Event` and overrides it, that is a
source break (compile error) and a binary break (`IncompatibleClassChangeError`
for already-compiled code). The PR is labelled `branch-1.3`.
The description frames this as an internal cleanup ("caught and fixed in 6
existing classes"), but it belongs under "user-facing change" as an API break,
and someone should confirm whether sealing a `@DeveloperApi` method is
acceptable in a patch line.
Minor, related: `BaseEvent.customInfo()` stays non-final and `PreEvent
extends BaseEvent`, so the claim in the class doc that this makes the bug
"impossible to reintroduce" holds only for the `Event` subtree. Worth softening
the wording.
##########
server-common/src/main/java/org/apache/gravitino/server/web/RequestContextFilter.java:
##########
@@ -54,7 +86,11 @@ public void doFilter(ServletRequest request, ServletResponse
response, FilterCha
throws IOException, ServletException {
try {
if (request instanceof HttpServletRequest) {
-
RequestContext.setRemoteAddress(resolveClientAddress((HttpServletRequest)
request));
+ HttpServletRequest httpRequest = (HttpServletRequest) request;
+ RequestContext.setRemoteAddress(resolveClientAddress(httpRequest));
+ if (eventBus.isPresent()) {
+
RequestContext.setRequestQueryParams(flattenParameterMap(httpRequest.getParameterMap()));
Review Comment:
`getParameterMap()` is not "query parameters". Per the servlet spec it is
query-string parameters **plus** `application/x-www-form-urlencoded` body
parameters, and for the latter it reads and consumes the request input stream.
I checked `server/`, `iceberg/`, `lance/` and `core/` for
`APPLICATION_FORM_URLENCODED` and `@FormParam` and there are none today, so
nothing breaks right now. But an OAuth2 token endpoint is the classic
form-encoded endpoint, and the day one is added this filter will silently eat
the body before the resource method sees it, with a symptom ("request body is
empty") that points nowhere near here.
Since the goal, the docs and the field name (`requestQueryParams`) all say
query parameters, parsing `httpRequest.getQueryString()` is both closer to the
intent and removes the hazard. If you would rather keep `getParameterMap()`,
please add a comment recording the constraint.
##########
core/src/main/java/org/apache/gravitino/utils/RequestContext.java:
##########
@@ -87,16 +104,50 @@ public static void markOperationFailureFired() {
* @return {@code true} if the flag is set, {@code false} otherwise.
*/
public static boolean isOperationFailureFired() {
- return Boolean.TRUE.equals(OPERATION_FAILURE_FIRED.get());
+ return OPERATION_OUTCOME.get() == OperationOutcome.FAILURE;
}
/**
- * Clears the operation-failure flag for the current request thread. Must be
called in a {@code
- * finally} block by {@code HttpAuditFilter} at the end of each request to
prevent stale values
- * from leaking to the next request on the same Jetty thread.
+ * Clears the recorded operation outcome for the current request thread.
Must be called in a
+ * {@code finally} block by {@code HttpAuditFilter} at the end of each
request to prevent stale
+ * values from leaking to the next request on the same Jetty thread.
+ *
+ * <p>Named after the failure flag for symmetry with {@link
#resetOperationSuccessFired()}, but
+ * clears the single underlying outcome either way — see the class-level doc.
*/
public static void resetOperationFailureFired() {
- OPERATION_FAILURE_FIRED.remove();
+ OPERATION_OUTCOME.remove();
+ }
+
+ /**
+ * Marks that an operation-layer success {@code Event} has been dispatched
for the current
+ * request. Called by {@code EventBus.dispatchPostEvent()} for every success
event that is not
+ * itself an {@code HttpRequestEvent}.
+ */
+ public static void markOperationSuccessFired() {
+ OPERATION_OUTCOME.set(OperationOutcome.SUCCESS);
Review Comment:
This makes the outcome last-write-wins, where the previous
`OPERATION_FAILURE_FIRED` boolean was monotonic: once a failure was recorded
nothing could clear it until the reset in `HttpAuditFilter`'s `finally`.
Now a success event dispatched after a failure event on the same request
erases the failure. If the HTTP status is then 4xx, `HttpAuditFilter` sees
`isOperationFailureFired() == false` and emits an `HttpRequestFailureEvent` on
top of the operation-layer failure event, which is exactly the double-logging
this flag exists to prevent.
The class doc asserts "a request's operation layer dispatches at most one
terminal event (success or failure), never both", but nothing enforces it. I
went through the three branches in `EventBus.dispatchEvent` and several hook
dispatchers and could **not** construct a live path today, so this is not a bug
report, it is an invariant that moved from being guaranteed by the code to
being guaranteed by a comment.
One line restores it:
```java
public static void markOperationSuccessFired() {
if (OPERATION_OUTCOME.get() != OperationOutcome.FAILURE) {
OPERATION_OUTCOME.set(OperationOutcome.SUCCESS);
}
}
```
##########
server-common/src/main/java/org/apache/gravitino/server/web/HttpAuditFilter.java:
##########
@@ -183,6 +192,19 @@ public void doFilter(ServletRequest request,
ServletResponse response, FilterCha
eventSource);
eventBus.get().dispatchEvent(event);
}
+ } else if (!RequestContext.isOperationFailureFired()
Review Comment:
This branch now emits an audit entry for every 2xx request that has no
structured event, on all three servers, with no way to turn it off.
On the main server that is mostly fine. On the Iceberg REST server it is the
path Spark hammers, and the endpoints least likely to be wired into the
operation-event system (`/v1/config`, metrics reporting) are exactly the ones
that will now produce a fallback entry each time. Combined with the unbounded
query-parameter capture I noted in `RequestContextFilter`, audit volume could
step up noticeably.
I am not asking for this to be reverted, the "every request produces at
least one entry" property is worth having. But the volume impact deserves
either a config switch or an explicit note in the docs, so an operator
upgrading is not surprised.
##########
core/src/test/java/org/apache/gravitino/audit/TestAuditLogRedactor.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * 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.audit;
+
+import java.util.Collections;
+import java.util.Map;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class TestAuditLogRedactor {
+
+ @Test
+ public void testNullCustomInfoReturnsEmptyMap() {
+ Assertions.assertTrue(AuditLogRedactor.redactCustomInfo(null).isEmpty());
+ }
+
+ @Test
+ public void testNonSensitiveKeyKeptAsIs() {
+ Map<String, String> info = Collections.singletonMap("details", "true");
+ Assertions.assertEquals("true",
AuditLogRedactor.redactCustomInfo(info).get("details"));
+ }
+
+ // ─── exact-match (MASKED_CUSTOM_INFO_KEYS)
───────────────────────────────────
+
+ @Test
+ public void testExactMatchKeyIsRedacted() {
+ Map<String, String> info = Collections.singletonMap("authorization",
"Bearer secret-token");
+ Assertions.assertEquals(
+ AuditLogRedactor.REDACTED_VALUE,
+ AuditLogRedactor.redactCustomInfo(info).get("authorization"));
+ }
+
+ @Test
+ public void testExactMatchIsCaseInsensitive() {
+ Map<String, String> info = Collections.singletonMap("AUTHORIZATION",
"Bearer secret-token");
+ Assertions.assertEquals(
+ AuditLogRedactor.REDACTED_VALUE,
+ AuditLogRedactor.redactCustomInfo(info).get("AUTHORIZATION"));
+ }
+
+ /**
+ * "cookie" is a known internal key with no sensitive substring in its name
(it doesn't contain
+ * "password", "token", etc.), so only the exact-match list catches it.
+ */
+ @Test
+ public void testExactMatchCatchesKeyWithNoSensitiveSubstring() {
+ Map<String, String> info = Collections.singletonMap("cookie",
"session=abc123");
+ Assertions.assertEquals(
+ AuditLogRedactor.REDACTED_VALUE,
AuditLogRedactor.redactCustomInfo(info).get("cookie"));
+ }
+
+ // ─── substring match (SENSITIVE_KEY_SUBSTRINGS)
──────────────────────────────
Review Comment:
Related to the redaction gap I left on `AuditLogRedactor`: every key
exercised here (`accessToken`, `password`, `token`, `authorization`, `cookie`,
`s3-secret-access-key`, `auth*`) is caught either by the exact-match list or by
a single-word substring, so the table passes without ever touching the
multi-word entries.
Adding `api-key`, `x-api-key`, `access-key-id`, `private_key` to this test
is what would have caught it, and is worth having regardless of how the fix is
done.
##########
server-common/src/main/java/org/apache/gravitino/server/web/RequestContextFilter.java:
##########
@@ -72,4 +108,20 @@ private String resolveClientAddress(HttpServletRequest
request) {
}
return request.getRemoteAddr();
}
+
+ /**
+ * Flattens a servlet-style parameter map to a single value per name
(joining multi-valued
+ * parameters with a comma), with no redaction — see the class doc for why
that happens later.
+ */
+ private static Map<String, String> flattenParameterMap(Map<String, String[]>
parameterMap) {
Review Comment:
No bound on either the number of parameters or the length of a value. A
caller can put 10 KB of query string on a request and it lands in **every**
event constructed during that request and in every audit line written for it.
Cheap fix while you are here: cap the entry count and truncate individual
values (with an explicit marker), rather than copying whatever arrived.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
