-
Notifications
You must be signed in to change notification settings - Fork 4.8k
[WIP]HIVE-29299: Upgrade Spring to 6.2.12 and spring-ldap-core to 3.3.4 to resolve CVE-2025-41249 #6401
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP]HIVE-29299: Upgrade Spring to 6.2.12 and spring-ldap-core to 3.3.4 to resolve CVE-2025-41249 #6401
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,12 @@ | |
| <artifactId>hive-standalone-metastore-server</artifactId> | ||
| <version>${hive.version}</version> | ||
| <scope>provided</scope> | ||
| <exclusions> | ||
| <exclusion> | ||
| <groupId>javax.servlet</groupId> | ||
| <artifactId>javax.servlet-api</artifactId> | ||
| </exclusion> | ||
| </exclusions> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.apache.hive</groupId> | ||
|
|
@@ -70,6 +76,12 @@ | |
| <artifactId>micrometer-registry-prometheus</artifactId> | ||
| <version>1.9.17</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>jakarta.servlet</groupId> | ||
| <artifactId>jakarta.servlet-api</artifactId> | ||
| <version>6.0.0</version> | ||
| <scope>provided</scope> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. who is supposed to provide this dependency? we are not building a war and deploying to the app server |
||
| </dependency> | ||
| <!-- Test dependencies --> | ||
| <dependency> | ||
| <groupId>org.apache.hive</groupId> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,8 +17,6 @@ | |
| */ | ||
| package org.apache.iceberg.rest.standalone; | ||
|
|
||
| import javax.servlet.http.HttpServlet; | ||
|
|
||
| import org.apache.hadoop.conf.Configuration; | ||
| import org.apache.hadoop.hive.metastore.ServletServerBuilder; | ||
| import org.apache.hadoop.hive.metastore.conf.MetastoreConf; | ||
|
|
@@ -54,14 +52,14 @@ public Configuration hadoopConfiguration(ApplicationArguments args) { | |
| } | ||
|
|
||
| @Bean | ||
| public ServletRegistrationBean<HttpServlet> restCatalogServlet(Configuration conf) { | ||
| public ServletRegistrationBean<?> restCatalogServlet(Configuration conf) { | ||
| return createRestCatalogServlet(conf); | ||
| } | ||
|
|
||
| /** | ||
| * Creates the REST Catalog servlet registration. Shared by production config and tests. | ||
| */ | ||
| public static ServletRegistrationBean<HttpServlet> createRestCatalogServlet(Configuration conf) { | ||
| public static ServletRegistrationBean<?> createRestCatalogServlet(Configuration conf) { | ||
| String servletPath = MetastoreConf.getVar(conf, ConfVars.ICEBERG_CATALOG_SERVLET_PATH); | ||
| if (servletPath == null || servletPath.isEmpty()) { | ||
| servletPath = DEFAULT_SERVLET_PATH; | ||
|
|
@@ -80,9 +78,8 @@ public static ServletRegistrationBean<HttpServlet> createRestCatalogServlet(Conf | |
| if (descriptor == null || descriptor.getServlet() == null) { | ||
| throw new IllegalStateException("Failed to create Iceberg REST Catalog servlet"); | ||
| } | ||
|
|
||
| ServletRegistrationBean<HttpServlet> registration = | ||
| new ServletRegistrationBean<>(descriptor.getServlet(), "/" + servletPath + "/*"); | ||
| ServletRegistrationBean<?> registration = | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not simply replace the import?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At first, IntelliJ was showing red lines when I tried the jakarta import. I didn't see the 'Add to classpath' option initially when I hovered over the error, so I used the generic <?> as a temporary fix to get the code to compile locally. I see how to fix the classpath now so I'll restore the specific HttpServlet with jakarta import |
||
| new ServletRegistrationBean(descriptor.getServlet(), "/" + servletPath + "/*"); | ||
| registration.setName("IcebergRESTCatalog"); | ||
| registration.setLoadOnStartup(1); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -288,6 +288,7 @@ | |
| <dependency> | ||
| <groupId>org.apache.thrift</groupId> | ||
| <artifactId>libthrift</artifactId> | ||
| <version>0.20.0</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.datanucleus</groupId> | ||
|
|
@@ -359,17 +360,23 @@ | |
| <dependency> | ||
| <groupId>org.eclipse.jetty</groupId> | ||
| <artifactId>jetty-util</artifactId> | ||
| <version>${jetty.version}</version> | ||
| <version>11.0.25</version> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please do not hardcode
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah sure!! |
||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.eclipse.jetty</groupId> | ||
| <artifactId>jetty-server</artifactId> | ||
| <version>${jetty.version}</version> | ||
| <version>11.0.25</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.eclipse.jetty</groupId> | ||
| <artifactId>jetty-servlet</artifactId> | ||
| <version>${jetty.version}</version> | ||
| <version>11.0.25</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>jakarta.servlet</groupId> | ||
| <artifactId>jakarta.servlet-api</artifactId> | ||
| <version>6.0.0</version> | ||
| <scope>provided</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.springframework</groupId> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,22 +33,18 @@ | |
| import org.apache.hadoop.security.SecurityUtil; | ||
| import org.apache.hadoop.security.UserGroupInformation; | ||
| import org.eclipse.jetty.util.ssl.SslContextFactory; | ||
| import org.pac4j.core.context.JEEContext; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why drop this? |
||
| import org.pac4j.core.credentials.TokenCredentials; | ||
| import org.pac4j.core.credentials.extractor.BearerAuthExtractor; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import javax.servlet.ServletException; | ||
| import javax.servlet.http.HttpServlet; | ||
| import javax.servlet.http.HttpServletRequest; | ||
| import javax.servlet.http.HttpServletResponse; | ||
| import jakarta.servlet.ServletException; | ||
| import jakarta.servlet.http.HttpServlet; | ||
| import jakarta.servlet.http.HttpServletRequest; | ||
| import jakarta.servlet.http.HttpServletResponse; | ||
| import java.io.IOException; | ||
| import java.security.PrivilegedAction; | ||
| import java.security.PrivilegedExceptionAction; | ||
| import java.util.Arrays; | ||
| import java.util.Enumeration; | ||
| import java.util.Optional; | ||
|
|
||
| /** | ||
| * Secures servlet processing. | ||
|
|
@@ -305,10 +301,15 @@ private String extractUserName(HttpServletRequest request, HttpServletResponse r | |
| */ | ||
| private String extractBearerToken(HttpServletRequest request, | ||
| HttpServletResponse response) { | ||
| BearerAuthExtractor extractor = new BearerAuthExtractor(); | ||
| Optional<TokenCredentials> tokenCredentials = extractor.extract(new JEEContext( | ||
| request, response)); | ||
| return tokenCredentials.map(TokenCredentials::getToken).orElse(null); | ||
| String authorization = request.getHeader("Authorization"); | ||
| if (authorization == null) { | ||
| return null; | ||
| } | ||
| if (authorization.regionMatches(true, 0, "Bearer ", 0, 7)) { | ||
| String token = authorization.substring(7).trim(); | ||
| return token.isEmpty() ? null : token; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -338,7 +339,7 @@ static void loginServerPrincipal(Configuration conf) throws IOException { | |
| * @return null if no ssl in config, an instance otherwise | ||
| * @throws IOException if getting password fails | ||
| */ | ||
| static SslContextFactory createSslContextFactory(Configuration conf) throws IOException { | ||
| static SslContextFactory.Server createSslContextFactory(Configuration conf) throws IOException { | ||
| final boolean useSsl = MetastoreConf.getBoolVar(conf, MetastoreConf.ConfVars.USE_SSL); | ||
| if (!useSsl) { | ||
| return null; | ||
|
|
@@ -359,7 +360,7 @@ static SslContextFactory createSslContextFactory(Configuration conf) throws IOEx | |
| if (LOG.isInfoEnabled()) { | ||
| LOG.info("HTTP Server SSL: adding excluded protocols: {}", Arrays.toString(excludedProtocols)); | ||
| } | ||
| SslContextFactory factory = new SslContextFactory.Server(); | ||
| SslContextFactory.Server factory = new SslContextFactory.Server(); | ||
| factory.addExcludeProtocols(excludedProtocols); | ||
| if (LOG.isInfoEnabled()) { | ||
| LOG.info("HTTP Server SSL: SslContextFactory.getExcludeProtocols = {}", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -149,7 +149,12 @@ public Void execute(MultiDataSourceJdbcResource jdbcResource) throws MetaExcepti | |
| // caught and either swallowed or wrapped in MetaException. Also, only a single test fails without this block: | ||
| // org.apache.hadoop.hive.metastore.client.TestDatabases.testAlterDatabaseNotNullableFields | ||
| // It may worth investigate if this catch block is really needed. | ||
| if (e.getMessage() != null && e.getMessage().contains("does not exist")) { | ||
| String msg = e.getMessage(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. change is unrelated to the dependency upgrade
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason I included this change is that TestDatabases#testAlterDatabaseNotNullableFields started failing immediately after the move to Spring 6 in my first precommit run. |
||
| Throwable cause = e.getCause(); | ||
| boolean isMissingTable = (msg != null && msg.contains("does not exist")) || | ||
| (cause != null && cause.getMessage() != null && cause.getMessage().contains("does not exist")); | ||
|
|
||
| if (isMissingTable) { | ||
| LOG.warn("Cannot perform {} since metastore table does not exist", callSig); | ||
| } else { | ||
| throw new MetaException("Unable to " + callSig + ":" + org.apache.hadoop.util.StringUtils.stringifyException(e)); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scope is provided, why do any exclusions here?