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);
```
--
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]
