flyrain commented on code in PR #4225: URL: https://github.com/apache/polaris/pull/4225#discussion_r3358819148
########## runtime/service/src/main/java/org/apache/polaris/service/events/EventAttributeFilter.java: ########## @@ -0,0 +1,30 @@ +/* + * 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.polaris.service.events; + +/** + * Global event attribute filter applied before delivery to all listeners. Determines which {@link + * AttributeKey}s from an {@link EventAttributeMap} are safe to pass downstream. Sensitive + * attributes are stripped globally so all listeners receive only sanitized events by default. + */ +public interface EventAttributeFilter { Review Comment: Thanks for adding sanitizer interface. Do we actually need a separate EventAttributeFilter interface? Right now it has one impl (DefaultEventAttributeFilter) and one consumer (DefaultEventSanitizer), the dispatcher only ever talks to EventSanitizer. I'd lean toward folding it into DefaultEventSanitizer: - The common customization("deny extra keys") is already handled by polaris.event-listener.denylisted-attributes, no bean swap needed. The separate filter bean only buys swapping the denial policy wholesale (e.g. allowlist), which is just as easily done by replacing the one EventSanitizer bean. - Deny and derive aren't cleanly separable anyway: extractDerivedAttributes reaches around the filter to read the denied CATALOG object so it can derive CATALOG_NAME. The sanitizer already has to know what the filter drops, so the two belong together. - The predicate is a one-liner (!denylist.contains(key)); the real logic is the config-driven denylist resolution, which is already the static resolveAdditionalDenylist(...) helper and can stay static. One caveat: if the allowlist-vs-denylist question from the dev@ thread is still open and you'd want operators to pick the posture, a swappable policy has some value, but even then I'd make it the single EventSanitizer bean rather than keep two interfaces. ########## runtime/service/src/main/java/org/apache/polaris/service/events/EventSanitizer.java: ########## @@ -0,0 +1,38 @@ +/* + * 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.polaris.service.events; + +/** + * Global, dispatcher-enforced sanitizer for events. Invoked exactly once at the choke point in Review Comment: Doc nit: `deliverEvent` is called per-listener, so `sanitize()` actually runs (and re-clones) once per listener, not "exactly once at the choke point." Harmless but the javadoc overstates it. ########## runtime/service/src/main/java/org/apache/polaris/service/events/PolarisEventListeners.java: ########## @@ -80,8 +82,13 @@ public void onStartup(@Observes StartupEvent event) { private void deliverEvent( PolarisEvent event, String listenerName, PolarisEventListener listener) { LOGGER.debug("Delivering {} event to listener '{}' ({})", event.type(), listenerName, listener); + // TODO: Add an opt-in mechanism (e.g., a marker interface or annotation) for specialized Review Comment: I believe some audit use cases need sensitive data, including Principal, and others. Both maker interface and annotation should work. Example of maker interface: ``` /** Opt-in: listeners implementing this receive the raw, unsanitized event. */ public interface RawEventAccess {} A listener opts in by implementing it: @Identifier("forensic-audit") public class ForensicAuditListener implements PolarisEventListener, RawEventAccess { @Override public void onEvent(PolarisEvent event) { /* sees raw event */ } } Dispatcher check in deliverEvent: PolarisEvent toDeliver = (listener instanceof RawEventAccess) ? event : eventSanitizer.sanitize(event); listener.onEvent(toDeliver); ``` I think we are not plan to apply different sanitizer to different event listener, right? @sririshindra, @nandorKollar . At least the current impl. can't support it. -- 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]
