sboorlagadda commented on code in PR #7940:
URL: https://github.com/apache/geode/pull/7940#discussion_r2442437552
##########
TESTING.md:
##########
@@ -13,8 +13,10 @@ Tests are broken up into five types - unit, integration,
distributed, acceptance
`./gradlew distributedTest`
* Acceptance tests: test Geode from end user perspective
`./gradlew acceptanceTest`
-* Upgrade tests: test compatibility between versions of Geode and rolling
upgrades
+* Upgrade tests: test backwards compatibility and rolling upgrades between
versions of Geode
`./gradlew upgradeTest`
+
+ **Note**: Rolling upgrades are **NOT supported** across the Jakarta EE 10
migration boundary (pre-migration → post-migration) for Tomcat session
replication due to the javax.servlet → jakarta.servlet API incompatibility.
Rolling upgrades within the same API era continue to work.
Review Comment:
How are we planning to document this breaking change for our users? May be
we should document a clear migration path by adding
1. Specific version requirements for Tomcat 10.1+ vs 11.x
2. Clear timeline for when Tomcat 6/7/8/9 support will be completely
removed
##########
geode-web-management/src/main/java/org/apache/geode/management/internal/rest/security/RestSecurityConfiguration.java:
##########
@@ -16,98 +16,351 @@
import java.io.IOException;
-import java.util.Arrays;
-
-import javax.servlet.ServletException;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
import com.fasterxml.jackson.databind.ObjectMapper;
+import jakarta.servlet.ServletException;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.ComponentScan;
import org.springframework.context.annotation.Configuration;
import org.springframework.http.MediaType;
import org.springframework.security.authentication.AuthenticationManager;
-import
org.springframework.security.config.annotation.authentication.builders.AuthenticationManagerBuilder;
-import
org.springframework.security.config.annotation.method.configuration.EnableGlobalMethodSecurity;
+import org.springframework.security.authentication.ProviderManager;
+import
org.springframework.security.config.annotation.method.configuration.EnableMethodSecurity;
import
org.springframework.security.config.annotation.web.builders.HttpSecurity;
import
org.springframework.security.config.annotation.web.configuration.EnableWebSecurity;
-import
org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter;
import org.springframework.security.config.http.SessionCreationPolicy;
import org.springframework.security.core.AuthenticationException;
import org.springframework.security.web.AuthenticationEntryPoint;
+import org.springframework.security.web.SecurityFilterChain;
import
org.springframework.security.web.authentication.www.BasicAuthenticationFilter;
+import org.springframework.security.web.util.matcher.AntPathRequestMatcher;
import org.springframework.web.multipart.MultipartResolver;
-import org.springframework.web.multipart.commons.CommonsMultipartResolver;
+import
org.springframework.web.multipart.support.StandardServletMultipartResolver;
import org.apache.geode.management.api.ClusterManagementResult;
-import org.apache.geode.management.configuration.Links;
+/**
+ * Spring Security 6.x migration changes:
+ *
+ * <p>
+ * <b>Architecture Changes:</b>
+ * </p>
+ * <ul>
+ * <li>WebSecurityConfigurerAdapter → Component-based configuration (adapter
deprecated in Spring
+ * Security 5.7, removed in 6.0)</li>
+ * <li>Override methods → Bean-based SecurityFilterChain configuration</li>
+ * <li>ProviderManager constructor replaces AuthenticationManagerBuilder
pattern</li>
+ * </ul>
+ *
+ * <p>
+ * <b>API Modernization:</b>
+ * </p>
+ * <ul>
+ * <li>@EnableGlobalMethodSecurity → @EnableMethodSecurity (new annotation
name)</li>
+ * <li>antMatchers() → requestMatchers() with AntPathRequestMatcher
(deprecated method removed)</li>
+ * <li>Method chaining (.and()) → Lambda DSL configuration (modern fluent
API)</li>
+ * <li>authorizeRequests() → authorizeHttpRequests() (new method name)</li>
+ * </ul>
+ *
+ * <p>
+ * <b>Multipart Resolver:</b>
+ * </p>
+ * <ul>
+ * <li>CommonsMultipartResolver → StandardServletMultipartResolver</li>
+ * <li>Reason: Spring 6.x standardized on Servlet 3.0+ native multipart
support</li>
+ * <li>Note: Custom isMultipart() logic removed -
StandardServletMultipartResolver handles PUT/POST
+ * automatically</li>
+ * </ul>
+ *
+ * <p>
+ * <b>JWT Authentication Failure Handler:</b>
+ * </p>
+ * <ul>
+ * <li>Added explicit error response handling in
authenticationFailureHandler</li>
+ * <li>Returns proper HTTP 401 with JSON ClusterManagementResult for
UNAUTHENTICATED status</li>
+ * <li>Previously relied on default behavior; now explicitly defined for
clarity</li>
+ * </ul>
+ *
+ * <p>
+ * <b>Security Filter Chain:</b>
+ * </p>
+ * <ul>
+ * <li>configure(HttpSecurity) → filterChain(HttpSecurity) returning
SecurityFilterChain</li>
+ * <li>SecurityFilterChain bean is Spring Security 6.x's recommended
approach</li>
+ * <li>setAuthenticationManager() explicitly called on JwtAuthenticationFilter
(required in
+ * 6.x)</li>
+ * </ul>
+ */
@Configuration
@EnableWebSecurity
-@EnableGlobalMethodSecurity(prePostEnabled = true)
+@EnableMethodSecurity(prePostEnabled = true)
// this package name needs to be different than the admin rest controller's
package name
// otherwise this component scan will pick up the admin rest controllers as
well.
-@ComponentScan("org.apache.geode.management.internal.rest")
-public class RestSecurityConfiguration extends WebSecurityConfigurerAdapter {
+@ComponentScan(basePackages = "org.apache.geode.management.internal.rest")
+public class RestSecurityConfiguration {
@Autowired
private GeodeAuthenticationProvider authProvider;
@Autowired
private ObjectMapper objectMapper;
- @Override
- protected void configure(AuthenticationManagerBuilder auth) {
- auth.authenticationProvider(authProvider);
- }
-
@Bean
- @Override
- public AuthenticationManager authenticationManagerBean() throws Exception {
- return super.authenticationManagerBean();
+ public AuthenticationManager authenticationManager() {
+ return new ProviderManager(authProvider);
}
@Bean
public MultipartResolver multipartResolver() {
- return new CommonsMultipartResolver() {
- @Override
- public boolean isMultipart(HttpServletRequest request) {
- String method = request.getMethod().toLowerCase();
- // By default, only POST is allowed. Since this is an 'update' we
should accept PUT.
- if (!Arrays.asList("put", "post").contains(method)) {
- return false;
- }
- String contentType = request.getContentType();
- return (contentType != null &&
contentType.toLowerCase().startsWith("multipart/"));
- }
- };
+ // Spring 6.x uses StandardServletMultipartResolver instead of
CommonsMultipartResolver
+ return new StandardServletMultipartResolver();
}
- protected void configure(HttpSecurity http) throws Exception {
-
http.sessionManagement().sessionCreationPolicy(SessionCreationPolicy.STATELESS).and()
- .authorizeRequests()
- .antMatchers("/docs/**", "/swagger-ui.html", "/swagger-ui/index.html",
"/swagger-ui/**",
- "/", Links.URI_VERSION + "/api-docs/**",
"/webjars/springdoc-openapi-ui/**",
- "/v3/api-docs/**", "/swagger-resources/**")
- .permitAll()
- .and().csrf().disable();
+ @Bean
+ public SecurityFilterChain filterChain(HttpSecurity http) throws Exception {
+ http.sessionManagement(
+ session ->
session.sessionCreationPolicy(SessionCreationPolicy.STATELESS))
+ .authorizeHttpRequests(authorize -> authorize
+ .requestMatchers(new AntPathRequestMatcher("/docs/**"),
+ new AntPathRequestMatcher("/swagger-ui.html"),
+ new AntPathRequestMatcher("/swagger-ui/index.html"),
+ new AntPathRequestMatcher("/swagger-ui/**"),
+ new AntPathRequestMatcher("/"),
+ new AntPathRequestMatcher("/v1/api-docs/**"),
+ new AntPathRequestMatcher("/webjars/springdoc-openapi-ui/**"),
+ new AntPathRequestMatcher("/v3/api-docs/**"),
+ new AntPathRequestMatcher("/swagger-resources/**"))
+ .permitAll())
Review Comment:
Isn't this security configuration is incomplete? After defining permitAll()
for documentation endpoints, there's no catch-all rule for other endpoints.
This means:
1. All other endpoints are implicitly accessible without authentication
2. Missing `.anyRequest().authenticated()` or similar authorization rule
3. This creates a security bypass for the management REST API
```
.authorizeHttpRequests(authorize -> authorize
.requestMatchers(/* documentation paths */).permitAll()
.anyRequest().authenticated()) <-- Should we add this line?
```
##########
geode-http-service/src/main/java/org/apache/geode/internal/cache/http/service/InternalHttpService.java:
##########
@@ -172,46 +254,88 @@ public synchronized void addWebApplication(String
webAppContext, Path warFilePat
Map<String, Object> attributeNameValuePairs)
throws Exception {
if (httpServer == null) {
- logger.info(
- String.format("unable to add %s webapp. Http service is not started
on this member.",
- webAppContext));
+ logger.warn(WEBAPP, "Cannot add webapp, HTTP service not started:
webapp={}", webAppContext);
return;
}
+ logger.info(WEBAPP, "Adding webapp {}", webAppContext);
+
WebAppContext webapp = new WebAppContext();
webapp.setContextPath(webAppContext);
webapp.setWar(warFilePath.toString());
+
+ // Required for Spring Boot initialization: AnnotationConfiguration
triggers Jetty's annotation
+ // scanning during webapp.configure(), which discovers
SpringServletContainerInitializer via
+ // ServiceLoader from META-INF/services. Without this, Spring's
WebApplicationInitializer
+ // chain never starts, causing 404 errors for all REST endpoints.
+ // Reference:
jetty-ee10-demos/embedded/src/main/java/ServerWithAnnotations.java
+ webapp.addConfiguration(new AnnotationConfiguration());
+
+ // Child-first classloading prevents parent classloader's Jackson from
conflicting with
+ // webapp's bundled version, avoiding NoSuchMethodError during JSON
serialization.
webapp.setParentLoaderPriority(false);
// GEODE-7334: load all jackson classes from war file except jackson
annotations
-
webapp.getSystemClasspathPattern().add("com.fasterxml.jackson.annotation.");
- webapp.getServerClasspathPattern().add("com.fasterxml.jackson.",
- "-com.fasterxml.jackson.annotation.");
+ // Jetty 12: Attribute names changed to ee10.webapp namespace
+
webapp.setAttribute("org.eclipse.jetty.ee10.webapp.ContainerIncludeJarPattern",
+ ".*/jakarta\\.servlet-api-[^/]*\\.jar$|" +
+ ".*/jakarta\\.servlet\\.jsp\\.jstl-.*\\.jar$|" +
+ ".*/com\\.fasterxml\\.jackson\\.annotation\\..*\\.jar$");
+
webapp.setAttribute("org.eclipse.jetty.ee10.webapp.WebInfIncludeJarPattern",
+ ".*/com\\.fasterxml\\.jackson\\.(?!annotation).*\\.jar$");
+
// add the member's working dir as the extra classpath
webapp.setExtraClasspath(new File(".").getAbsolutePath());
webapp.setInitParameter("org.eclipse.jetty.servlet.Default.dirAllowed",
"false");
webapp.addAliasCheck(new SymlinkAllowedResourceAliasChecker(webapp));
+ // Store attributes on WebAppContext for backward compatibility
if (attributeNameValuePairs != null) {
attributeNameValuePairs.forEach(webapp::setAttribute);
+
+ // Listener must be registered as Source.EMBEDDED to execute during
ServletContext
+ // initialization, BEFORE DispatcherServlet starts. This timing
guarantees Spring's
+ // dependency injection finds SecurityService when initializing
LoginHandlerInterceptor.
+ // Using Source.JAVAX_API or adding via web.xml would execute too late
in the lifecycle.
+ // Pattern reference:
jetty-ee10/jetty-ee10-servlet/OneServletContext.java
+ ListenerHolder listenerHolder = new ListenerHolder(Source.EMBEDDED);
+ listenerHolder.setListener(
+ new ServletContextAttributeListener(attributeNameValuePairs,
webAppContext));
+ webapp.getServletHandler().addListener(listenerHolder);
}
File tmpPath = new File(getWebAppBaseDirectory(webAppContext));
tmpPath.mkdirs();
webapp.setTempDirectory(tmpPath);
- logger.info("Adding webapp " + webAppContext);
- ((HandlerCollection) httpServer.getHandler()).addHandler(webapp);
-
- // if the server is not started yet start the server, otherwise, start the
webapp alone
- if (!httpServer.isStarted()) {
- logger.info("Attempting to start HTTP service on port ({}) at
bind-address ({})...",
- port, bindAddress);
- httpServer.start();
- } else {
- webapp.start();
+
+ if (logger.isDebugEnabled()) {
+ ClassLoader webappClassLoader = webapp.getClassLoader();
+ ClassLoader parentClassLoader =
+ (webappClassLoader != null) ? webappClassLoader.getParent() : null;
+ logger.debug(CONFIG, "Webapp configuration: {}",
+ new LogContext()
+ .add("context", webAppContext)
+ .add("tempDir", tmpPath.getAbsolutePath())
+ .add("parentLoaderPriority", webapp.isParentLoaderPriority())
+ .add("webappClassLoader", webappClassLoader)
+ .add("parentClassLoader", parentClassLoader)
+ .add("annotationConfigEnabled", true)
+ .add("servletContextListenerAdded", attributeNameValuePairs !=
null));
}
+
+ // In Jetty 12, Handler.Sequence replaced HandlerCollection for dynamic
handler lists
+ ((Handler.Sequence) httpServer.getHandler()).addHandler(webapp);
+
+ // Server start deferred to restartHttpServer() to batch all webapp
configurations,
+ // avoiding multiple restart cycles and ensuring all webapps initialize
together.
webApps.add(webapp);
Review Comment:
Can this lead to a timing dependency where:
1. Webapps are added to Handler.Sequence immediately (line 328)
2. But server restart is deferred until restartHttpServer() is called
3. If server is already running, webapps might be accessible before
proper initialization
Should we consider adding webapps to a staging list first, then batch-add
them during restartHttpServer() to ensure atomic deployment?
##########
geode-http-service/src/main/java/org/apache/geode/internal/cache/http/service/InternalHttpService.java:
##########
@@ -64,18 +82,77 @@ public class InternalHttpService implements HttpService {
private final List<WebAppContext> webApps = new ArrayList<>();
+ /**
+ * Bridges WebAppContext and ServletContext attribute namespaces in Jetty 12.
+ *
+ * <p>
+ * Why needed: In Jetty 12, WebAppContext.setAttribute() stores attributes
in the webapp's
+ * context, but Spring's ServletContextAware beans (like
LoginHandlerInterceptor) retrieve
+ * from ServletContext.getAttribute(). These are separate namespaces that
don't auto-sync.
+ *
+ * <p>
+ * Timing: contextInitialized() is invoked BEFORE Spring's DispatcherServlet
initializes,
+ * guaranteeing attributes are present when Spring beans request them during
dependency injection.
+ * Without this, SecurityService would be null in LoginHandlerInterceptor,
causing 503 errors.
+ */
+ private static class ServletContextAttributeListener implements
ServletContextListener {
Review Comment:
Having ServletContextAttributeListener is a clever solution to handle the
namespace separation between WebAppContext and ServletContext in Jetty 12. This
prevents Spring dependency injection failures. Excellent Solution
##########
build.gradle:
##########
@@ -58,7 +58,30 @@ allprojects {
repositories {
mavenCentral()
- maven { url "https://repo.spring.io/release"; }
+ maven {
+ url "https://jakarta.oss.sonatype.org/content/repositories/releases/";
+ name "Jakarta EE Releases"
+ }
+ }
+
+ // CRITICAL: Fix for Log4j/SLF4J circular binding conflict introduced during
Jakarta/Spring Boot 3.x migration
+ //
+ // Root Cause:
+ // - Geode uses Log4j Core directly and includes 'log4j-slf4j-impl' (routes
SLF4J calls → Log4j)
+ // - Spring Boot 3.x includes 'spring-boot-starter-logging' which brings in
'log4j-to-slf4j' (routes Log4j calls → SLF4J)
+ // - Having BOTH creates a circular binding: SLF4J → Log4j → SLF4J
+ //
+ // Symptoms:
+ // - ClassCastException: org.apache.logging.slf4j.SLF4JLogger cannot be cast
to org.apache.logging.log4j.core.Logger
+ // - ClassCastException: org.apache.logging.slf4j.SLF4JLoggerContext cannot
be cast to org.apache.logging.log4j.core.LoggerContext
+ // - Occurs in Log4jLoggingProvider.getRootLoggerContext() when
LogManager.getRootLogger() returns SLF4J logger instead of Log4j logger
+ //
+ // Solution:
+ // Exclude 'log4j-to-slf4j' globally. Geode's logging architecture requires
Log4j Core to be the primary logging implementation,
+ // with SLF4J calls being routed TO Log4j (via log4j-slf4j-impl), not the
other way around.
+ //
+ configurations.all {
+ exclude group: 'org.apache.logging.log4j', module: 'log4j-to-slf4j'
Review Comment:
Excellent Fix for Logging Conflict. This global exclusion prevents
ClassCastException issues that would occur with circular logging bindings
##########
geode-web-api/src/main/java/org/apache/geode/rest/internal/web/security/RestSecurityConfiguration.java:
##########
@@ -19,47 +19,159 @@
import org.springframework.context.annotation.ComponentScan;
import org.springframework.context.annotation.Configuration;
import org.springframework.security.authentication.AuthenticationManager;
-import
org.springframework.security.config.annotation.authentication.builders.AuthenticationManagerBuilder;
-import
org.springframework.security.config.annotation.method.configuration.EnableGlobalMethodSecurity;
+import org.springframework.security.authentication.ProviderManager;
+import
org.springframework.security.config.annotation.method.configuration.EnableMethodSecurity;
import
org.springframework.security.config.annotation.web.builders.HttpSecurity;
import
org.springframework.security.config.annotation.web.configuration.EnableWebSecurity;
-import
org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter;
import org.springframework.security.config.http.SessionCreationPolicy;
+import org.springframework.security.web.SecurityFilterChain;
+import org.springframework.security.web.util.matcher.AntPathRequestMatcher;
+
-import org.apache.geode.management.configuration.Links;
@Configuration
@EnableWebSecurity
-@EnableGlobalMethodSecurity(prePostEnabled = true)
+// Spring Security 6.x migration: @EnableGlobalMethodSecurity deprecated,
replaced by
+// @EnableMethodSecurity
+@EnableMethodSecurity(prePostEnabled = true, securedEnabled = true,
jsr250Enabled = true)
@ComponentScan("org.apache.geode.rest.internal.web")
-public class RestSecurityConfiguration extends WebSecurityConfigurerAdapter {
+public class RestSecurityConfiguration {
@Autowired
private GeodeAuthenticationProvider authProvider;
- @Override
- protected void configure(AuthenticationManagerBuilder auth) {
- auth.authenticationProvider(authProvider);
- }
-
+ /**
+ * Spring Security 6.x migration: Create AuthenticationManager bean using
ProviderManager.
+ * Previously configured via AuthenticationManagerBuilder in configure()
method.
+ */
@Bean
- @Override
- public AuthenticationManager authenticationManagerBean() throws Exception {
- return super.authenticationManagerBean();
+ public AuthenticationManager authenticationManager() {
+ return new ProviderManager(authProvider);
}
- @Override
- protected void configure(HttpSecurity http) throws Exception {
+ /**
+ * Spring Security 6.x migration: Configure security using
SecurityFilterChain bean.
+ * Replaces WebSecurityConfigurerAdapter's configure(HttpSecurity) method.
+ * Uses lambda-based configuration and authorizeHttpRequests() instead of
authorizeRequests().
+ */
+ @Bean
+ public SecurityFilterChain filterChain(HttpSecurity http) throws Exception {
-
http.sessionManagement().sessionCreationPolicy(SessionCreationPolicy.STATELESS).and()
- .authorizeRequests()
- .antMatchers("/docs/**", "/swagger-ui.html", "/swagger-ui/index.html",
"/swagger-ui/**",
- Links.URI_VERSION + "/api-docs/**",
"/webjars/springdoc-openapi-ui/**",
- "/v3/api-docs/**", "/swagger-resources/**")
- .permitAll().and().csrf().disable();
+ http.sessionManagement(
+ session ->
session.sessionCreationPolicy(SessionCreationPolicy.STATELESS))
+ .authorizeHttpRequests(authorize -> authorize
+ .requestMatchers(new AntPathRequestMatcher("/docs/**"),
+ new AntPathRequestMatcher("/swagger-ui.html"),
+ new AntPathRequestMatcher("/swagger-ui/index.html"),
+ new AntPathRequestMatcher("/swagger-ui/**"),
+ new AntPathRequestMatcher("/v1/api-docs/**"),
+ new AntPathRequestMatcher("/webjars/springdoc-openapi-ui/**"),
+ new AntPathRequestMatcher("/v3/api-docs/**"),
+ new AntPathRequestMatcher("/swagger-resources/**"))
+ .permitAll())
Review Comment:
Isn't this security configuration is incomplete? After defining permitAll()
for documentation endpoints, there's no catch-all rule for other endpoints.
This means:
1. All other endpoints are implicitly accessible without authentication
2. Missing `.anyRequest().authenticated()` or similar authorization rule
3. This creates a security bypass for the management REST API
```
.authorizeHttpRequests(authorize -> authorize
.requestMatchers(/* documentation paths */).permitAll()
.anyRequest().authenticated()) <-- Should we add this line?
```
--
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]
