FrankChen021 commented on code in PR #20268: URL: https://github.com/apache/druid/pull/20268#discussion_r3940633632
########## server/src/main/java/org/apache/druid/server/initialization/jetty/ResponseIdentityHeaderHandler.java: ########## @@ -0,0 +1,79 @@ +/* + * 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.druid.server.initialization.jetty; + +import org.apache.druid.server.DruidNode; +import org.eclipse.jetty.client.Response; +import org.eclipse.jetty.http.HttpField; +import org.eclipse.jetty.http.HttpFields; +import org.eclipse.jetty.rewrite.handler.HeaderPatternRule; +import org.eclipse.jetty.rewrite.handler.RewriteHandler; +import org.eclipse.jetty.server.Handler; + +import javax.servlet.http.HttpServletResponse; + +public class ResponseIdentityHeaderHandler extends RewriteHandler +{ + public static final String RESPONSE_SERVER_HEADER = "X-Druid-Server"; + public static final String RESPONSE_SERVICE_HEADER = "X-Druid-Service"; + public static final String RESPONSE_VERSION_HEADER = "X-Druid-Version"; + + public ResponseIdentityHeaderHandler(final DruidNode selfNode, final Handler handler) + { + super(handler); + addRule(new HeaderPatternRule("*", RESPONSE_SERVER_HEADER, selfNode.getHostAndPortToUse())); Review Comment: [P2] Response resets can discard the identity headers HeaderPatternRule writes these fields before invoking the wrapped handler. A downstream handler can legally reset the response and Jetty clears its headers; Druid's PreResponseAuthorizationCheckFilter does this on its 403 error path (server/src/main/java/org/apache/druid/server/security/PreResponseAuthorizationCheckFilter.java:169-172). With this feature enabled, those generated responses therefore omit the identity triple despite the documented every-response guarantee. Inject the headers at final response commit (or reapply them after reset) and cover the reset path. ########## server/src/main/java/org/apache/druid/server/initialization/jetty/JettyServerModule.java: ########## @@ -404,6 +404,9 @@ public void lifeCycleStopped(LifeCycle event) JettyServerInitializer initializer = injector.getInstance(JettyServerInitializer.class); try { initializer.initialize(server, injector); + if (config.isEnableResponseIdentityHeaders()) { + server.setHandler(new ResponseIdentityHeaderHandler(node, server.getHandler())); Review Comment: [P2] Legacy Overlord proxy bypasses identity filtering This wraps the entire handler chain, including the Coordinator's /druid/indexer/* OverlordProxyServlet (services/src/main/java/org/apache/druid/cli/CoordinatorJettyServerInitializer.java:131-133), but that servlet still inherits Jetty's default response-header forwarding. When the Coordinator feature is disabled, an upstream-enabled Overlord's X-Druid-* headers are forwarded anyway; when the Coordinator feature is enabled and the upstream returns only a partial triple, the proxy overwrites only those fields while the outer handler leaves the Coordinator values for the rest. This bypasses the all-or-nothing identity contract implemented for Router proxies and can expose or emit an ambiguous identity. Apply the same clearing/filtering to OverlordProxyServlet or explicitly scope and document this behavior. ########## server/src/main/java/org/apache/druid/server/initialization/ServerConfig.java: ########## @@ -206,6 +206,9 @@ public ServerConfig(@NotNull ErrorResponseTransformStrategy errorResponseTransfo @JsonProperty private boolean enableHSTS = false; + @JsonProperty + private boolean enableResponseIdentityHeaders = false; Review Comment: [P2] Preserve the flag in the CliIndexer config copy CliIndexerServerModule.makeAdjustedServerConfig constructs the ServerConfig passed to JettyServerModule.makeAndInitializeServer, but its explicit constructor call (server/src/main/java/org/apache/druid/server/initialization/jetty/CliIndexerServerModule.java:142-166) does not copy enableResponseIdentityHeaders. Thus druid.server.http.enableResponseIdentityHeaders=true is lost for the CliIndexer HTTP server, so that service never installs the new handler even though the documentation says the setting applies to all Druid services. Preserve the flag in the adjusted config and cover this server type. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
