[
https://issues.apache.org/jira/browse/DRILL-8540?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18037801#comment-18037801
]
ASF GitHub Bot commented on DRILL-8540:
---------------------------------------
joakime commented on code in PR #3031:
URL: https://github.com/apache/drill/pull/3031#discussion_r2518795093
##########
exec/java-exec/src/test/java/org/apache/drill/exec/server/rest/spnego/TestDrillSpnegoAuthenticator.java:
##########
@@ -181,21 +147,8 @@ public void testAuthClientRequestForSpnegoLoginResource()
throws Exception {
*/
@Test
public void testAuthClientRequestForOtherPage() throws Exception {
-
- final HttpServletRequest request = Mockito.mock(HttpServletRequest.class);
- final HttpServletResponse response =
Mockito.mock(HttpServletResponse.class);
- final HttpSession session = Mockito.mock(HttpSession.class);
- final Authentication authentication =
Mockito.mock(UserAuthentication.class);
-
- Mockito.when(request.getSession(true)).thenReturn(session);
-
Mockito.when(request.getRequestURI()).thenReturn(WebServerConstants.WEBSERVER_ROOT_PATH);
-
Mockito.when(session.getAttribute(SessionAuthentication.__J_AUTHENTICATED)).thenReturn(authentication);
-
- final UserAuthentication returnedAuthentication = (UserAuthentication)
spnegoAuthenticator.validateRequest
- (request, response, false);
- assertEquals(authentication, returnedAuthentication);
- verify(response, never()).sendError(401);
- verify(response,
never()).setHeader(HttpHeader.WWW_AUTHENTICATE.asString(),
HttpHeader.NEGOTIATE.asString());
+ // This test needs to be rewritten for Jetty 12 API
+ // TODO: Rewrite for Jetty 12
Review Comment:
Directly mocking the HttpServletRequest isn't a great choice for testing.
It doesn't test the actual implementation, which depends heavily on internal
HttpServletRequest instance types.
Using a real server, and real HTTP requests, is often faster to setup, and
test.
##########
exec/java-exec/src/test/java/org/apache/drill/exec/server/rest/spnego/TestDrillSpnegoAuthenticator.java:
##########
@@ -262,17 +202,7 @@ public void testSpnegoLoginInvalidToken() throws Exception
{
}
});
- Mockito.when(request.getSession(true)).thenReturn(session);
-
- final String httpReqAuthHeader = String.format("%s:%s",
HttpHeader.NEGOTIATE.asString(), String.format
- ("%s%s","1234", token));
-
Mockito.when(request.getHeader(HttpHeader.AUTHORIZATION.asString())).thenReturn(httpReqAuthHeader);
-
Mockito.when(request.getRequestURI()).thenReturn(WebServerConstants.SPENGO_LOGIN_RESOURCE_PATH);
-
- assertEquals(spnegoAuthenticator.validateRequest(request, response,
false), Authentication.UNAUTHENTICATED);
-
- verify(session,
never()).setAttribute(SessionAuthentication.__J_AUTHENTICATED, null);
- verify(response, never()).sendError(401);
- verify(response,
never()).setHeader(HttpHeader.WWW_AUTHENTICATE.asString(),
HttpHeader.NEGOTIATE.asString());
+ // This test needs to be rewritten for Jetty 12 API
+ // TODO: Rewrite for Jetty 12
Review Comment:
It's often easier, and faster to execute, using a real server instance +
using an HTTP client to connect to it.
##########
pom.xml:
##########
@@ -98,8 +98,8 @@
<javassist.version>3.29.2-GA</javassist.version>
<javax.el.version>3.0.0</javax.el.version>
<javax.validation.api>2.0.1.Final</javax.validation.api>
- <jersey.version>2.40</jersey.version>
- <jetty.version>9.4.56.v20240826</jetty.version>
+ <jersey.version>3.1.9</jersey.version>
+ <jetty.version>12.0.15</jetty.version>
Review Comment:
btw, Jetty has BOM dependencies that can be used as well.
One for core, and one for the environment you choose to use.
Would you be interested in that?
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/DrillUserPrincipal.java:
##########
@@ -38,11 +37,11 @@ public class DrillUserPrincipal implements Principal {
public static final String[] NON_ADMIN_USER_ROLES = new
String[]{AUTHENTICATED_ROLE};
- public static final List<RolePrincipal> ADMIN_PRINCIPALS =
- ImmutableList.of(new RolePrincipal(AUTHENTICATED_ROLE), new
RolePrincipal(ADMIN_ROLE));
+ public static final List<String> ADMIN_PRINCIPALS =
Review Comment:
Use ...
``` java
import org.eclipse.jetty.security.RolePrincipal;
```
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/DrillHttpSecurityHandlerProvider.java:
##########
@@ -173,23 +177,23 @@ public void setHandler(Handler handler) {
}
@Override
- public void doStop() throws Exception {
+ protected void doStop() throws Exception {
super.doStop();
for (DrillHttpConstraintSecurityHandler securityHandler :
securityHandlers.values()) {
securityHandler.doStop();
}
}
public boolean isSpnegoEnabled() {
- return securityHandlers.containsKey(Constraint.__SPNEGO_AUTH);
+ return securityHandlers.containsKey("SPNEGO");
}
public boolean isFormEnabled() {
- return securityHandlers.containsKey(Constraint.__FORM_AUTH);
+ return securityHandlers.containsKey("FORM");
Review Comment:
```suggestion
return
securityHandlers.containsKey(org.eclipse.jetty.security.Authenticator.FORM_AUTH);
```
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/DrillHttpSecurityHandlerProvider.java:
##########
@@ -114,23 +112,27 @@ public DrillHttpSecurityHandlerProvider(DrillConfig
config, DrillbitContext dril
}
@Override
- public void doStart() throws Exception {
+ protected void doStart() throws Exception {
super.doStart();
for (DrillHttpConstraintSecurityHandler securityHandler :
securityHandlers.values()) {
securityHandler.doStart();
}
}
@Override
- public void handle(String target, Request baseRequest, HttpServletRequest
request, HttpServletResponse response)
- throws IOException, ServletException {
+ public boolean handle(Request request, Response response, Callback callback)
throws Exception {
+ // Get servlet request/response from the core request/response
+ org.eclipse.jetty.ee10.servlet.ServletContextRequest servletContextRequest
=
+ org.eclipse.jetty.server.Request.as(request,
org.eclipse.jetty.ee10.servlet.ServletContextRequest.class);
+ HttpServletRequest httpServletRequest =
servletContextRequest.getServletApiRequest();
+ HttpServletResponse httpServletResponse =
servletContextRequest.getHttpServletResponse();
Review Comment:
This class is a `Handler.Wrapper`
It has no access to the `ee10` implementation.
Converting from core `Request` / `Response` to Servlet `HttpServletRequest`
and `HttpServletResponse` can only be done within a `ServletContext`, which
this handler does not participate in.
If you need this, then extend from the `ConstraintSecurityHandler` from the
ee10 layer.
The `org.eclipse.jetty.ee10.servlet.security.ConstraintSecurityHandler` is
the appropriate `ee10` specific implementation of
`org.eclipse.jetty.security.SecurityHandler` and will be inserted into the
`WebAppContext` (or `ServletContextHandler`) in the right place if your class
uses the correct type.
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/DrillErrorHandler.java:
##########
@@ -18,25 +18,49 @@
package org.apache.drill.exec.server.rest.auth;
import org.apache.drill.exec.server.rest.WebServerConstants;
-import org.eclipse.jetty.server.handler.ErrorHandler;
+import org.eclipse.jetty.ee10.servlet.ErrorPageErrorHandler;
+import org.eclipse.jetty.server.Request;
+import org.eclipse.jetty.util.Callback;
-import javax.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletRequest;
import java.io.IOException;
import java.io.Writer;
/**
- * Custom ErrorHandler class for Drill's WebServer to have better error
message in case when SPNEGO login failed and
- * what to do next. In all other cases this would use the generic error page.
+ * Custom ErrorHandler class for Drill's WebServer to handle errors
appropriately based on the request type.
+ * - For JSON API endpoints (*.json), returns JSON error responses
+ * - For SPNEGO login failures, provides helpful HTML error page
+ * - For all other cases, returns standard HTML error page
*/
-public class DrillErrorHandler extends ErrorHandler {
+public class DrillErrorHandler extends ErrorPageErrorHandler {
+
+ @Override
+ public boolean handle(Request target, org.eclipse.jetty.server.Response
response, Callback callback) throws Exception {
+ // Check if this is a JSON API request
+ String pathInContext = Request.getPathInContext(target);
Review Comment:
Be careful here.
This will return the raw path in context.
There's no decoding, there's no normalization of the path, no collapsing of
path navigation, etc ...
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java:
##########
@@ -296,11 +309,11 @@ public void sessionDestroyed(HttpSessionEvent se) {
return;
}
- final Object authCreds =
session.getAttribute(SessionAuthentication.__J_AUTHENTICATED);
+ final Object authCreds =
session.getAttribute(SessionAuthentication.AUTHENTICATED_ATTRIBUTE);
if (authCreds != null) {
final SessionAuthentication sessionAuth = (SessionAuthentication)
authCreds;
- securityHandler.logout(sessionAuth);
- session.removeAttribute(SessionAuthentication.__J_AUTHENTICATED);
+ // In Jetty 12, logout is handled differently - we just remove the
attribute
Review Comment:
logout is still important to cleanup runAsRole entries.
It is also used by some authenticator types (eg: OpenID)
##########
docs/dev/Jetty12Migration.md:
##########
@@ -0,0 +1,183 @@
+# Jetty 12 Migration Guide
+
+## Overview
+
+Apache Drill has been upgraded from Jetty 9 to Jetty 12 to address security
vulnerabilities and maintain compatibility with modern Java versions. This
document describes the changes made, known limitations, and guidance for
developers.
+
+## What Changed
+
+### Core API Changes
+
+Jetty 12 introduced significant API changes as part of the Jakarta EE 10
migration:
+
+1. **Servlet API Migration**: `javax.servlet.*` → `jakarta.servlet.*`
+2. **Package Restructuring**: Servlet components moved to
`org.eclipse.jetty.ee10.servlet.*`
+3. **Handler API Redesign**: New `org.eclipse.jetty.server.Handler` interface
+4. **Resource Loading**: New `ResourceFactory` API replaces old `Resource` API
+5. **Authentication APIs**: `LoginService.login()` signature changed
+
+### Modified Files
+
+#### Production Code
+
+-
**exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java**
+ - Updated to use `ResourceFactory.root()` for resource loading
+ - Fixed null pointer issues when HTTP server is disabled
+
+-
**drill-yarn/src/main/java/org/apache/drill/yarn/appMaster/http/WebServer.java**
+ - Updated all imports to `org.eclipse.jetty.ee10.servlet.*`
+ - Modified `LoginService.login()` to new signature with `Function<Boolean,
Session>` parameter
+ - Changed to use `IdentityService.newUserIdentity()` for user identity
creation
+ - Updated `ResourceFactory` API usage
+ - Updated `SessionAuthentication` constants
+ - Fixed `ServletContextHandler` constructor usage
+
+#### Test Code
+
+The following test classes are temporarily disabled (see Known Limitations
below):
+- `TestImpersonationDisabledWithMiniDFS.java`
+- `TestImpersonationMetadata.java`
+- `TestImpersonationQueries.java`
+- `TestInboundImpersonation.java`
+
+## Known Limitations
+
+### Hadoop MiniDFSCluster Test Incompatibility
+
+**Issue**: Tests using Hadoop's MiniDFSCluster cannot run due to Jetty version
conflicts.
+
+**Root Cause**: Apache Hadoop 3.x depends on Jetty 9, while Drill now uses
Jetty 12. When tests attempt to start both:
+- Drill's embedded web server (Jetty 12)
+- Hadoop's MiniDFSCluster (Jetty 9)
+
+The conflicting Jetty versions on the classpath cause `NoClassDefFoundError`
exceptions.
+
+**Affected Tests**:
+- Impersonation tests with HDFS
+- Any tests requiring MiniDFSCluster with Drill's HTTP server enabled
+
+**Resolution Timeline**: These tests will be re-enabled when:
+- Apache Hadoop 4.x is released with Jetty 12 support
+- A Hadoop 3.x maintenance release upgrades to Jetty 12 (tracked in
[HADOOP-19625](https://issues.apache.org/jira/browse/HADOOP-19625))
+
+**Current Status**: HADOOP-19625 is open and targets Jetty 12 EE10, but
requires Java 17 baseline (tracked in HADOOP-17177). No specific release
version or timeline is available yet.
+
+### Why Alternative Solutions Failed
+
+Several approaches were attempted to resolve the Jetty conflict:
+
+1. **Dual Jetty versions in test scope**: Failed because Maven cannot have two
different versions of the same artifact on the classpath simultaneously, and
the Jetty BOM forces all Jetty artifacts to the same version.
+
+2. **Disabling Drill's HTTP server in tests**: Failed because drill-java-exec
classes are compiled against Jetty 12, and the bytecode contains hard
references to Jetty 12 classes that fail to load even when the HTTP server is
disabled.
+
+3. **Separate test module with Jetty 9**: Failed because depending on the
drill-java-exec JAR (compiled with Jetty 12) brings Jetty 12 class references
into the test classpath.
+
+### Workarounds for Developers
+
+If you need to test HDFS impersonation functionality:
+
+1. **Integration tests**: Use a real Hadoop cluster instead of MiniDFSCluster
+2. **Manual testing**: Test in HDFS-enabled environments
+3. **Alternative tests**: Use tests with local filesystem instead of
MiniDFSCluster (see other impersonation tests that don't require HDFS)
+
+## Developer Guidelines
+
+### Writing New Web Server Code
+
+When adding new HTTP/servlet functionality:
+
+1. Use Jakarta EE 10 imports:
+ ```java
+ import jakarta.servlet.http.HttpServletRequest;
+ import jakarta.servlet.http.HttpServletResponse;
+ ```
+
+2. Use Jetty 12 EE10 servlet packages:
+ ```java
+ import org.eclipse.jetty.ee10.servlet.ServletContextHandler;
+ import org.eclipse.jetty.ee10.servlet.ServletHolder;
+ ```
+
+3. Use `ResourceFactory.root()` for resource loading:
+ ```java
+ ResourceFactory rf = ResourceFactory.root();
+ InputStream stream =
rf.newClassLoaderResource("/path/to/resource").newInputStream();
+ ```
+
+4. Use new `ServletContextHandler` constructor pattern:
+ ```java
+ ServletContextHandler handler = new
ServletContextHandler(ServletContextHandler.SESSIONS);
+ handler.setContextPath("/");
+ ```
+
+### Writing Tests
+
+1. Tests that start Drill's HTTP server should **not** use Hadoop
MiniDFSCluster
+2. If HDFS testing is required, use local filesystem or mark test with
`@Ignore` and add comprehensive documentation
+3. When adding `@Ignore` for Jetty conflicts, reference
`TestImpersonationDisabledWithMiniDFS` for standard explanation
+
+### Debugging Jetty Issues
+
+Common issues and solutions:
+
+- **NoClassDefFoundError for Jetty classes**: Check that all Jetty
dependencies are Jetty 12, not Jetty 9
+- **ClassNotFoundException for javax.servlet**: Should be `jakarta.servlet`
with Jetty 12
+- **NullPointerException in ResourceFactory**: Use `ResourceFactory.root()`
instead of `ResourceFactory.of(server)`
Review Comment:
The use of `ResourceFactory.root()` should be avoided at all costs.
It's a very problematic API and is likely going to be deprecated with
suggestions to use the other instantiation methods. (`ResourceFactory.of(...)`,
`ResourceFactory.closable()`, `ResourceFactory.lifecycle()`, etc...)
Pass around the `ResourceFactory` created from a `LifeCycle` or `Container`.
You can optionally pass around the `LifeCycle` or `Container` object itself,
and just use `ResourceFactory.of(container)` or ...
``` java
// As a raw LifeCycle ...
ResourceFactory rf = ResourceFactory.lifecycle();
// Let container manage this child lifecycle
container.addBean(rf);
```
##########
docs/dev/Jetty12Migration.md:
##########
@@ -0,0 +1,183 @@
+# Jetty 12 Migration Guide
+
+## Overview
+
+Apache Drill has been upgraded from Jetty 9 to Jetty 12 to address security
vulnerabilities and maintain compatibility with modern Java versions. This
document describes the changes made, known limitations, and guidance for
developers.
+
+## What Changed
+
+### Core API Changes
+
+Jetty 12 introduced significant API changes as part of the Jakarta EE 10
migration:
+
+1. **Servlet API Migration**: `javax.servlet.*` → `jakarta.servlet.*`
+2. **Package Restructuring**: Servlet components moved to
`org.eclipse.jetty.ee10.servlet.*`
+3. **Handler API Redesign**: New `org.eclipse.jetty.server.Handler` interface
+4. **Resource Loading**: New `ResourceFactory` API replaces old `Resource` API
+5. **Authentication APIs**: `LoginService.login()` signature changed
+
+### Modified Files
+
+#### Production Code
+
+-
**exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java**
+ - Updated to use `ResourceFactory.root()` for resource loading
+ - Fixed null pointer issues when HTTP server is disabled
+
+-
**drill-yarn/src/main/java/org/apache/drill/yarn/appMaster/http/WebServer.java**
+ - Updated all imports to `org.eclipse.jetty.ee10.servlet.*`
+ - Modified `LoginService.login()` to new signature with `Function<Boolean,
Session>` parameter
+ - Changed to use `IdentityService.newUserIdentity()` for user identity
creation
+ - Updated `ResourceFactory` API usage
+ - Updated `SessionAuthentication` constants
+ - Fixed `ServletContextHandler` constructor usage
+
+#### Test Code
+
+The following test classes are temporarily disabled (see Known Limitations
below):
+- `TestImpersonationDisabledWithMiniDFS.java`
+- `TestImpersonationMetadata.java`
+- `TestImpersonationQueries.java`
+- `TestInboundImpersonation.java`
+
+## Known Limitations
+
+### Hadoop MiniDFSCluster Test Incompatibility
+
+**Issue**: Tests using Hadoop's MiniDFSCluster cannot run due to Jetty version
conflicts.
+
+**Root Cause**: Apache Hadoop 3.x depends on Jetty 9, while Drill now uses
Jetty 12. When tests attempt to start both:
+- Drill's embedded web server (Jetty 12)
+- Hadoop's MiniDFSCluster (Jetty 9)
+
+The conflicting Jetty versions on the classpath cause `NoClassDefFoundError`
exceptions.
+
+**Affected Tests**:
+- Impersonation tests with HDFS
+- Any tests requiring MiniDFSCluster with Drill's HTTP server enabled
+
+**Resolution Timeline**: These tests will be re-enabled when:
+- Apache Hadoop 4.x is released with Jetty 12 support
+- A Hadoop 3.x maintenance release upgrades to Jetty 12 (tracked in
[HADOOP-19625](https://issues.apache.org/jira/browse/HADOOP-19625))
+
+**Current Status**: HADOOP-19625 is open and targets Jetty 12 EE10, but
requires Java 17 baseline (tracked in HADOOP-17177). No specific release
version or timeline is available yet.
+
+### Why Alternative Solutions Failed
+
+Several approaches were attempted to resolve the Jetty conflict:
+
+1. **Dual Jetty versions in test scope**: Failed because Maven cannot have two
different versions of the same artifact on the classpath simultaneously, and
the Jetty BOM forces all Jetty artifacts to the same version.
+
+2. **Disabling Drill's HTTP server in tests**: Failed because drill-java-exec
classes are compiled against Jetty 12, and the bytecode contains hard
references to Jetty 12 classes that fail to load even when the HTTP server is
disabled.
+
+3. **Separate test module with Jetty 9**: Failed because depending on the
drill-java-exec JAR (compiled with Jetty 12) brings Jetty 12 class references
into the test classpath.
+
+### Workarounds for Developers
+
+If you need to test HDFS impersonation functionality:
+
+1. **Integration tests**: Use a real Hadoop cluster instead of MiniDFSCluster
+2. **Manual testing**: Test in HDFS-enabled environments
+3. **Alternative tests**: Use tests with local filesystem instead of
MiniDFSCluster (see other impersonation tests that don't require HDFS)
+
+## Developer Guidelines
+
+### Writing New Web Server Code
+
+When adding new HTTP/servlet functionality:
+
+1. Use Jakarta EE 10 imports:
+ ```java
+ import jakarta.servlet.http.HttpServletRequest;
+ import jakarta.servlet.http.HttpServletResponse;
+ ```
+
+2. Use Jetty 12 EE10 servlet packages:
+ ```java
+ import org.eclipse.jetty.ee10.servlet.ServletContextHandler;
+ import org.eclipse.jetty.ee10.servlet.ServletHolder;
+ ```
+
+3. Use `ResourceFactory.root()` for resource loading:
+ ```java
+ ResourceFactory rf = ResourceFactory.root();
+ InputStream stream =
rf.newClassLoaderResource("/path/to/resource").newInputStream();
+ ```
+
+4. Use new `ServletContextHandler` constructor pattern:
+ ```java
+ ServletContextHandler handler = new
ServletContextHandler(ServletContextHandler.SESSIONS);
+ handler.setContextPath("/");
+ ```
+
+### Writing Tests
+
+1. Tests that start Drill's HTTP server should **not** use Hadoop
MiniDFSCluster
+2. If HDFS testing is required, use local filesystem or mark test with
`@Ignore` and add comprehensive documentation
+3. When adding `@Ignore` for Jetty conflicts, reference
`TestImpersonationDisabledWithMiniDFS` for standard explanation
+
+### Debugging Jetty Issues
+
+Common issues and solutions:
+
+- **NoClassDefFoundError for Jetty classes**: Check that all Jetty
dependencies are Jetty 12, not Jetty 9
+- **ClassNotFoundException for javax.servlet**: Should be `jakarta.servlet`
with Jetty 12
Review Comment:
You can use the maven `maven-enforcer-plugin` and it's `enforce` goal with
the `<bannedDependencies>` configuration to ensure that `javax.servlet` and
other EE8 packages and/or classes do not exist in your project. (even
transitively)
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/SpnegoSecurityHandler.java:
##########
@@ -19,17 +19,17 @@
import org.apache.drill.common.exceptions.DrillException;
import org.apache.drill.exec.server.DrillbitContext;
-import org.eclipse.jetty.util.security.Constraint;
+@SuppressWarnings({"rawtypes", "unchecked"})
public class SpnegoSecurityHandler extends DrillHttpConstraintSecurityHandler {
@Override
public String getImplName() {
- return Constraint.__SPNEGO_AUTH;
+ return "SPNEGO";
Review Comment:
```suggestion
return org.eclipse.jetty.security.Authenticator.SPNEGO_AUTH;
```
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/RolePrincipal.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.drill.exec.server.rest.auth;
+
+import java.io.Serializable;
+import java.security.Principal;
+
+/**
+ * Role principal implementation for Jetty 11+.
+ * Replaced the removed RolePrincipal from AbstractLoginService.
Review Comment:
This exists as `org.eclipse.jetty.security.RolePrincipal`
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/FormSecurityHandler.java:
##########
@@ -22,12 +22,11 @@
import org.apache.drill.exec.server.DrillbitContext;
import org.apache.drill.exec.server.rest.WebServerConstants;
import org.eclipse.jetty.security.authentication.FormAuthenticator;
-import org.eclipse.jetty.util.security.Constraint;
public class FormSecurityHandler extends DrillHttpConstraintSecurityHandler {
@Override
public String getImplName() {
- return Constraint.__FORM_AUTH;
+ return "FORM";
Review Comment:
```suggestion
return org.eclipse.jetty.security.Authenticator.FORM_AUTH;
```
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/HttpBasicAuthSecurityHandler.java:
##########
@@ -21,15 +21,14 @@
import org.apache.drill.exec.rpc.security.plain.PlainFactory;
import org.apache.drill.exec.server.DrillbitContext;
import org.eclipse.jetty.security.authentication.BasicAuthenticator;
-import org.eclipse.jetty.util.security.Constraint;
/**
* Implement HTTP Basic authentication for REST API access
*/
public class HttpBasicAuthSecurityHandler extends
DrillHttpConstraintSecurityHandler {
@Override
public String getImplName() {
- return Constraint.__BASIC_AUTH;
+ return "BASIC";
Review Comment:
```suggestion
return org.eclipse.jetty.security.Authenticator.BASIC_AUTH;
```
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/DrillSpnegoLoginService.java:
##########
@@ -36,32 +39,30 @@
import org.ietf.jgss.Oid;
import javax.security.auth.Subject;
-import javax.servlet.ServletRequest;
import java.io.IOException;
-import java.lang.reflect.Field;
import java.security.Principal;
import java.security.PrivilegedExceptionAction;
import java.util.Base64;
+import java.util.function.Function;
/**
* Custom implementation of DrillSpnegoLoginService to avoid the need of
passing targetName in a config file,
* to include the SPNEGO OID and the way UserIdentity is created.
*/
-public class DrillSpnegoLoginService extends SpnegoLoginService {
+public class DrillSpnegoLoginService implements LoginService {
Review Comment:
There's already a `org.eclipse.jetty.security.SPNEGOLoginService`
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/DrillSpnegoAuthenticator.java:
##########
@@ -20,165 +20,142 @@
import org.apache.drill.exec.server.rest.WebServerConstants;
import org.apache.parquet.Strings;
+import org.eclipse.jetty.ee10.servlet.ServletContextRequest;
+import org.eclipse.jetty.http.HttpField;
+import org.eclipse.jetty.http.HttpFields;
import org.eclipse.jetty.http.HttpHeader;
-import org.eclipse.jetty.http.HttpVersion;
+import org.eclipse.jetty.http.HttpStatus;
+import org.eclipse.jetty.security.AuthenticationState;
import org.eclipse.jetty.security.ServerAuthException;
-import org.eclipse.jetty.security.UserAuthentication;
-import org.eclipse.jetty.security.authentication.DeferredAuthentication;
+import org.eclipse.jetty.security.UserIdentity;
+import org.eclipse.jetty.security.authentication.LoginAuthenticator;
import org.eclipse.jetty.security.authentication.SessionAuthentication;
-import org.eclipse.jetty.security.authentication.SpnegoAuthenticator;
-import org.eclipse.jetty.server.Authentication;
import org.eclipse.jetty.server.Request;
-import org.eclipse.jetty.server.UserIdentity;
+import org.eclipse.jetty.server.Response;
+import org.eclipse.jetty.util.Callback;
-import javax.servlet.ServletRequest;
-import javax.servlet.ServletResponse;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
-import javax.servlet.http.HttpSession;
-import java.io.IOException;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpSession;
/**
- * Custom SpnegoAuthenticator for Drill
+ * Custom SpnegoAuthenticator for Drill - Jetty 12 version
+ *
+ * This class extends LoginAuthenticator and provides SPNEGO authentication
support.
*/
-public class DrillSpnegoAuthenticator extends SpnegoAuthenticator {
+public class DrillSpnegoAuthenticator extends LoginAuthenticator {
Review Comment:
Use `org.eclipse.jetty.security.authentication.SPNEGOAuthenticator`
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/DrillHttpSecurityHandlerProvider.java:
##########
@@ -173,23 +177,23 @@ public void setHandler(Handler handler) {
}
@Override
- public void doStop() throws Exception {
+ protected void doStop() throws Exception {
super.doStop();
for (DrillHttpConstraintSecurityHandler securityHandler :
securityHandlers.values()) {
securityHandler.doStop();
}
}
public boolean isSpnegoEnabled() {
- return securityHandlers.containsKey(Constraint.__SPNEGO_AUTH);
+ return securityHandlers.containsKey("SPNEGO");
Review Comment:
```suggestion
return
securityHandlers.containsKey(org.eclipse.jetty.security.Authenticator.SPNEGO_AUTH);
```
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/DrillSpnegoAuthenticator.java:
##########
@@ -20,165 +20,142 @@
import org.apache.drill.exec.server.rest.WebServerConstants;
import org.apache.parquet.Strings;
+import org.eclipse.jetty.ee10.servlet.ServletContextRequest;
+import org.eclipse.jetty.http.HttpField;
+import org.eclipse.jetty.http.HttpFields;
import org.eclipse.jetty.http.HttpHeader;
-import org.eclipse.jetty.http.HttpVersion;
+import org.eclipse.jetty.http.HttpStatus;
+import org.eclipse.jetty.security.AuthenticationState;
import org.eclipse.jetty.security.ServerAuthException;
-import org.eclipse.jetty.security.UserAuthentication;
-import org.eclipse.jetty.security.authentication.DeferredAuthentication;
+import org.eclipse.jetty.security.UserIdentity;
+import org.eclipse.jetty.security.authentication.LoginAuthenticator;
import org.eclipse.jetty.security.authentication.SessionAuthentication;
-import org.eclipse.jetty.security.authentication.SpnegoAuthenticator;
-import org.eclipse.jetty.server.Authentication;
import org.eclipse.jetty.server.Request;
-import org.eclipse.jetty.server.UserIdentity;
+import org.eclipse.jetty.server.Response;
+import org.eclipse.jetty.util.Callback;
-import javax.servlet.ServletRequest;
-import javax.servlet.ServletResponse;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
-import javax.servlet.http.HttpSession;
-import java.io.IOException;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpSession;
/**
- * Custom SpnegoAuthenticator for Drill
+ * Custom SpnegoAuthenticator for Drill - Jetty 12 version
+ *
+ * This class extends LoginAuthenticator and provides SPNEGO authentication
support.
*/
-public class DrillSpnegoAuthenticator extends SpnegoAuthenticator {
+public class DrillSpnegoAuthenticator extends LoginAuthenticator {
private static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(DrillSpnegoAuthenticator.class);
+ private static final String AUTH_METHOD = "SPNEGO";
- public DrillSpnegoAuthenticator(String authMethod) {
- super(authMethod);
+ public DrillSpnegoAuthenticator() {
+ super();
}
+
/**
- * Updated logic as compared to default implementation in
- * {@link SpnegoAuthenticator#validateRequest(ServletRequest,
ServletResponse, boolean)} to handle below cases:
- * 1) Perform SPNEGO authentication only when spnegoLogin resource is
requested. This helps to avoid authentication
- * for each and every resource which the JETTY provided authenticator
does.
- * 2) Helps to redirect to the target URL after authentication is done
successfully.
- * 3) Clear-Up in memory session information once LogOut is triggered such
that any future request also triggers SPNEGO
- * authentication.
- * @param request
- * @param response
- * @param mandatoryAuth
- * @return
- * @throws ServerAuthException
+ * Jetty 12 validateRequest implementation using core
Request/Response/Callback API.
+ * Handles:
+ * 1) Perform SPNEGO authentication only when spnegoLogin resource is
requested
+ * 2) Redirect to target URL after authentication
+ * 3) Clear session information on logout
*/
@Override
- public Authentication validateRequest(ServletRequest request,
ServletResponse response, boolean mandatoryAuth)
+ public AuthenticationState validateRequest(Request request, Response
response, Callback callback)
throws ServerAuthException {
- final HttpServletRequest req = (HttpServletRequest) request;
- final HttpSession session = req.getSession(true);
- final Authentication authentication = (Authentication)
session.getAttribute(SessionAuthentication.__J_AUTHENTICATED);
- final String uri = req.getRequestURI();
+ // Get the servlet request from the core request
+ ServletContextRequest servletContextRequest = Request.as(request,
ServletContextRequest.class);
+ if (servletContextRequest == null) {
+ return AuthenticationState.CHALLENGE;
Review Comment:
Returning CHALLENGE requires you to trigger the challenge.
This also means you have to handle the `callback` properly.
See the implementation at
`org.eclipse.jetty.security.authentication.SPNEGOAuthenticator` and it's
`sendChallenge` method.
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/LogInLogOutResources.java:
##########
@@ -168,11 +167,11 @@ public class MainLoginPageModel {
}
public boolean isSpnegoEnabled() {
- return authEnabled && configuredMechs.contains(Constraint.__SPNEGO_AUTH);
+ return authEnabled && configuredMechs.contains("SPNEGO");
}
public boolean isFormEnabled() {
- return authEnabled && configuredMechs.contains(Constraint.__FORM_AUTH);
+ return authEnabled && configuredMechs.contains("FORM");
Review Comment:
```suggestion
return authEnabled &&
configuredMechs.contains(org.eclipse.jetty.security.Authenticator.FORM_AUTH);
```
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/DrillHttpSecurityHandlerProvider.java:
##########
@@ -173,23 +177,23 @@ public void setHandler(Handler handler) {
}
@Override
- public void doStop() throws Exception {
+ protected void doStop() throws Exception {
super.doStop();
for (DrillHttpConstraintSecurityHandler securityHandler :
securityHandlers.values()) {
securityHandler.doStop();
}
}
public boolean isSpnegoEnabled() {
- return securityHandlers.containsKey(Constraint.__SPNEGO_AUTH);
+ return securityHandlers.containsKey("SPNEGO");
}
public boolean isFormEnabled() {
- return securityHandlers.containsKey(Constraint.__FORM_AUTH);
+ return securityHandlers.containsKey("FORM");
}
public boolean isBasicEnabled() {
- return securityHandlers.containsKey(Constraint.__BASIC_AUTH);
+ return securityHandlers.containsKey("BASIC");
Review Comment:
```suggestion
return
securityHandlers.containsKey(org.eclipse.jetty.security.Authenticator.BASIC_AUTH);
```
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/OAuthRequests.java:
##########
@@ -162,7 +162,7 @@ public static Response updateAuthToken(String name, String
code, HttpServletRequ
// Get success page
String successPage = null;
- try (InputStream inputStream =
Resource.newClassPathResource(OAUTH_SUCCESS_PAGE).getInputStream()) {
+ try (InputStream inputStream =
ResourceFactory.root().newClassLoaderResource(OAUTH_SUCCESS_PAGE).newInputStream())
{
Review Comment:
Don't use `ResourceFactory.root()`, use a proper one tied to a `LifeCycle`
or `Container`.
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/auth/DrillErrorHandler.java:
##########
@@ -18,25 +18,49 @@
package org.apache.drill.exec.server.rest.auth;
import org.apache.drill.exec.server.rest.WebServerConstants;
-import org.eclipse.jetty.server.handler.ErrorHandler;
+import org.eclipse.jetty.ee10.servlet.ErrorPageErrorHandler;
+import org.eclipse.jetty.server.Request;
+import org.eclipse.jetty.util.Callback;
-import javax.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletRequest;
import java.io.IOException;
import java.io.Writer;
/**
- * Custom ErrorHandler class for Drill's WebServer to have better error
message in case when SPNEGO login failed and
- * what to do next. In all other cases this would use the generic error page.
+ * Custom ErrorHandler class for Drill's WebServer to handle errors
appropriately based on the request type.
+ * - For JSON API endpoints (*.json), returns JSON error responses
+ * - For SPNEGO login failures, provides helpful HTML error page
+ * - For all other cases, returns standard HTML error page
*/
-public class DrillErrorHandler extends ErrorHandler {
+public class DrillErrorHandler extends ErrorPageErrorHandler {
+
+ @Override
+ public boolean handle(Request target, org.eclipse.jetty.server.Response
response, Callback callback) throws Exception {
+ // Check if this is a JSON API request
+ String pathInContext = Request.getPathInContext(target);
+ if (pathInContext != null && pathInContext.endsWith(".json")) {
+ // For JSON API endpoints, return JSON error response instead of HTML
+ response.getHeaders().put("Content-Type", "application/json");
Review Comment:
This is only valid if the request has the appropriate `Accept` header to
allow it.
You should probably look at the `ErrorHandler` implementation and look at
how it's done with the `generateAcceptableResponse` methods.
##########
drill-yarn/src/main/java/org/apache/drill/yarn/appMaster/http/WebServer.java:
##########
@@ -213,18 +215,20 @@ private void setupStaticResources(
// non-Servlet
// version.)
+ ResourceFactory resourceFactory =
ResourceFactory.of(servletContextHandler);
Review Comment:
:+1: this is the correct way to initialize a `ResourceFactory`.
The `ServletContextHandler` LifeCycle will cleanup the held resources from
that `ResourceFactory` when that `ServletContextHandler` is stopped.
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/LogInLogOutResources.java:
##########
@@ -168,11 +167,11 @@ public class MainLoginPageModel {
}
public boolean isSpnegoEnabled() {
- return authEnabled && configuredMechs.contains(Constraint.__SPNEGO_AUTH);
+ return authEnabled && configuredMechs.contains("SPNEGO");
Review Comment:
```suggestion
return authEnabled &&
configuredMechs.contains(org.eclipse.jetty.security.Authenticator.SPNEGO_AUTH);
```
##########
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java:
##########
@@ -443,7 +456,8 @@ private void generateOptionsDescriptionJSFile() throws
IOException {
int numLeftToWrite = options.size();
// Template source Javascript file
- InputStream optionsDescribeTemplateStream =
Resource.newClassPathResource(OPTIONS_DESCRIBE_TEMPLATE_JS).getInputStream();
+ ResourceFactory rf = ResourceFactory.root();
Review Comment:
Avoid using `ResourceFactory.root()` for anything.
Bind it to a valid `LifeCycle` (eg: the `WebAppContext` or the `Server`) to
have the Resources be cleaned up properly when the `LifeCycle` stops.
Eg:
``` java
ResourceFactory rf = webappContext.getResourceFactory();
// or
ResourceFactory rf = ResourceFactory.of(webappContext);
// or
ResourceFactory rf = ResourceFactory.of(server);
```
> Update Jetty to Version 12
> --------------------------
>
> Key: DRILL-8540
> URL: https://issues.apache.org/jira/browse/DRILL-8540
> Project: Apache Drill
> Issue Type: Task
> Components: Web Server
> Affects Versions: 1.22.0
> Reporter: Charles Givre
> Assignee: Charles Givre
> Priority: Major
> Fix For: 1.23.0
>
>
> Drill was using Jetty version 9.X which was EOL some time ago. This PR bumps
> Jetty to version 12. Jetty 12 is not compatible with with Java 11, so this
> PR also requires that Drill remove support for Java 11. At this state, this
> seems reasonable.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
